Re: [Qemu-devel] [PATCH 02/11] spapr_hcall: use spapr_ovec_* interfaces for CAS options

[David Gibson]

On Fri, Oct 14, 2016 at 02:02:31PM +1100, David Gibson wrote:
> On Wed, Oct 12, 2016 at 06:13:50PM -0500, Michael Roth wrote:
> > Currently we access individual bytes of an option vector via
> > ldub_phys() to test for the presence of a particular capability
> > within that byte. Currently this is only done for the "dynamic
> > reconfiguration memory" capability bit. If that bit is present,
> > we pass a boolean value to spapr_h_cas_compose_response()
> > to generate a modified device tree segment with the additional
> > properties required to enable this functionality.
> > 
> > As more capability bits are added, will would need to modify the
> > code to add additional option vector accesses and extend the
> > param list for spapr_h_cas_compose_response() to include similar
> > boolean values for these parameters.
> > 
> > Avoid this by switching to spapr_ovec_* helpers so we can do all
> > the parsing in one shot and then test for these additional bits
> > within spapr_h_cas_compose_response() directly.
> > 
> > Cc: Bharata B Rao <address@hidden>
> > Signed-off-by: Michael Roth <address@hidden>
> 
> Reviewed-by: David Gibson <address@hidden>

That said.. some comments making the overall scheme here might be
helpful.

Specifically..

[snip]
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 39dadaa..6c20d28 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -6,6 +6,7 @@
> >  #include "hw/ppc/xics.h"
> >  #include "hw/ppc/spapr_drc.h"
> >  #include "hw/mem/pc-dimm.h"
> > +#include "hw/ppc/spapr_ovec.h"
> >  
> >  struct VIOsPAPRBus;
> >  struct sPAPRPHBState;
> > @@ -66,6 +67,8 @@ struct sPAPRMachineState {
> >      uint64_t rtc_offset; /* Now used only during incoming migration */
> >      struct PPCTimebase tb;
> >      bool has_graphics;
> > +    sPAPROptionVector *ov5;
> > +    sPAPROptionVector *ov5_cas;

IIUC, the ov5 represents all the features qemu is capable of
supporting, and ov5_cas records the ones that were actually negotiated
during CAS.  Some descriptions here could make that much easier to follow.


> >      uint32_t check_exception_irq;
> >      Notifier epow_notifier;
> > @@ -577,7 +580,7 @@ void spapr_events_init(sPAPRMachineState *sm);
> >  void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
> >  int spapr_h_cas_compose_response(sPAPRMachineState *sm,
> >                                   target_ulong addr, target_ulong size,
> > -                                 bool cpu_update, bool memory_update);
> > +                                 bool cpu_update);
> >  sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
> >  void spapr_tce_table_enable(sPAPRTCETable *tcet,
> >                              uint32_t page_shift, uint64_t bus_offset,
> > diff --git a/include/hw/ppc/spapr_ovec.h b/include/hw/ppc/spapr_ovec.h
> > index fba2d98..09afd59 100644
> > --- a/include/hw/ppc/spapr_ovec.h
> > +++ b/include/hw/ppc/spapr_ovec.h
> > @@ -42,6 +42,9 @@ typedef struct sPAPROptionVector sPAPROptionVector;
> >  
> >  #define OV_BIT(byte, bit) ((byte - 1) * BITS_PER_BYTE + bit)
> >  
> > +/* option vector 5 */
> > +#define OV5_DRCONF_MEMORY       OV_BIT(2, 2)
> > +
> >  /* interfaces */
> >  sPAPROptionVector *spapr_ovec_new(void);
> >  sPAPROptionVector *spapr_ovec_clone(sPAPROptionVector *ov_orig);
> 



-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature