Re: [Qemu-ppc] [Qemu-devel] [RFC PATCH v0 1/1] spapr: Support setting of compat CPU type for CPU cores

[Eduardo Habkost]

On Fri, Jun 24, 2016 at 07:10:06AM +0530, Bharata B Rao wrote:
> On Thu, Jun 23, 2016 at 12:55:36PM -0300, Eduardo Habkost wrote:
> > On Wed, Jun 22, 2016 at 09:09:46AM +0200, Igor Mammedov wrote:
> > > On Wed, 22 Jun 2016 08:00:28 +0530
> > > Bharata B Rao <address@hidden> wrote:
> > > 
> > > > On Tue, Jun 21, 2016 at 09:09:57AM +0200, Igor Mammedov wrote:
> > > > > On Sat, 18 Jun 2016 14:04:06 +0530
> > > > > Bharata B Rao <address@hidden> wrote:
> > > > > 
> > > > > > Compat CPU type is typically specified on -cpu cmdline option
> > > > > > like: -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc.
> > > > > > With the introduction of sPAPR CPU core devices, we need to
> > > > > > support the same for core devices too.
> > > > > > 
> > > > > > Support the specification of CPU compat type on device_add
> > > > > > command for sPAPRCPUCore devices like:
> > > > > > (qemu) device_add
> > > > > > POWER8E-spapr-cpu-core,id=core3,compat=power7,core-id=24
> > > > > > 
> > > > > > Signed-off-by: Bharata B Rao <address@hidden>
> > > > > > ---
> > > > > > Applies on ppc-for-2.7 branch of David Gibson's tree.
> > > > > > 
> > > > > >  hw/ppc/spapr.c                  |  8 +++++
> > > > > >  hw/ppc/spapr_cpu_core.c         | 73
> > > > > > +++++++++++++++++++++++++++++++++++++++++
> > > > > > include/hw/ppc/spapr_cpu_core.h |  2 ++ 3 files changed, 83
> > > > > > insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > > index 778fa25..2049d7d 100644
> > > > > > --- a/hw/ppc/spapr.c
> > > > > > +++ b/hw/ppc/spapr.c
> > > > > > @@ -1807,6 +1807,7 @@ static void ppc_spapr_init(MachineState
> > > > > > *machine) if (i < spapr_cores) {
> > > > > >                  char *type =
> > > > > > spapr_get_cpu_core_type(machine->cpu_model); Object *core;
> > > > > > +                char *compat;
> > > > > >  
> > > > > >                  if (!object_class_by_name(type)) {
> > > > > >                      error_report("Unable to find sPAPR CPU Core
> > > > > > definition"); @@ -1818,6 +1819,13 @@ static void
> > > > > > ppc_spapr_init(MachineState *machine) &error_fatal);
> > > > > >                  object_property_set_int(core, core_dt_id,
> > > > > > CPU_CORE_PROP_CORE_ID, &error_fatal);
> > > > > > +                compat =
> > > > > > spapr_get_cpu_compat_type(machine->cpu_model);
> > > > > > +                if (compat) {
> > > > > > +                    object_property_set_str(core, compat,
> > > > > > "compat",
> > > > > > +                                            &error_fatal);
> > > > > > +                    g_free(compat);
> > > > > > +                }
> > > > > > +
> > > > > >                  object_property_set_bool(core, true, "realized",
> > > > > > &error_fatal); }
> > > > > >          }
> > > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > > > index 3a5da09..9eb63cc 100644
> > > > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > > > @@ -96,6 +96,24 @@ char *spapr_get_cpu_core_type(const char
> > > > > > *model) return core_type;
> > > > > >  }
> > > > > >  
> > > > > > +/*
> > > > > > + * Returns the CPU compat type specified in -cpu @model.
> > > > > > + */
> > > > > > +char *spapr_get_cpu_compat_type(const char *model)
> > > > > CPUClass already has such parser and hook to override it in case of
> > > > > target does legacy parsing, see CPUClass->parse_features.
> > > > > Maybe extending generic core in similar way and reusing generic
> > > > > parser will do the job.
> > > > 
> > > > In fact, I am already using CPUClass->parse_features() to set the
> > > > feature properties (which right now is only compat= for us) for core
> > > > device.
> > > > 
> > > > What I need in the above routine spapr_get_cpu_compat_type() is to
> > > > extract "compat=", so that I can verify that the compat type specified
> > > > with -device matches with what was specified with -cpu.
> > > > 
> > > > > 
> > > > > However we are in progress [1] of converting legacy
> > > > >  -cpu cpuname,feat1=x,feat2=y,...
> > > > > into a set of global properties
> > > > >  -global cputype.feat1=x ...
> > > > > 
> > > > > it would be better if you wouldn't start using -cpu for features
> > > > > but use directly -global mechanism,
> > > > > for that you need only have a corresponding property in spapr_core
> > > > > type.
> > > > 
> > > > Will this work be part of 2.7 ? If not, we will have to use -cpu to
> > > > extract the compat type since powerpc core hotplug is already
> > > > upstream.
> > > You can use -global without that work but so far it looks like it will
> > > make into 2.7.
> > 
> > I expect the -cpu => globals series to be included soon, unless
> > there are objections by others.
> > 
> > But as Igor said, globals are already supported. If you want to
> > set a property in the CPU core objects, you just need to use:
> > 
> >   -global spapr-cpu-core.compat=XXX
> > 
> > The conflict here seems to be that you want the "-cpu" option to
> > affect the cpu-core objects, not just the VCPU/thread objects.
> > But we have been assuming that "-cpu" would only affect the
> > VCPU/thread objects.
> 
> -cpu property affects the CPU thread objects in our case too. Since those
> CPU thread objects are aggregated under a core object, I was attempting
> to add compat= with core device and validate that with -cpu type,compat=
> from cmdline.
> 
> What we really need is this:
> 
> If -global cputype.compat= or -cpu cpuname,compat= is set, we need to
> apply the same to all the CPU thread objects of cpu core object. Hence
> I am thinking we don't need a separate property for this in the core object.

I assumed you really needed a spapr-cpu-core property for some
other reason. If all you need is to affect the VCPU thread
objects, then you only need cpu_common_parse_features() (the
default implementation of CPUClass::parse_features()).

Note that Igor's series is changing the
CPUClass::parse_features() interface. The current version needs
to be called once for each CPU, the version by Igor will call it
only once, before CPU creation. I suggest using his series as
base, so you don't need to redo your work later.

If everything goes well, I plan to submit a pull request
including his series on Monday.

-- 
Eduardo