Re: [Qemu-devel] [PATCH v2 1/1] spapr: Set compat type for CPUs of a core

[Bharata B Rao]

On Mon, Jun 27, 2016 at 04:51:34PM +1000, David Gibson wrote:
> On Mon, Jun 27, 2016 at 11:49:46AM +0530, Bharata B Rao wrote:
> > Compat CPU type is typically specified on -cpu cmdline option like:
> > -cpu host,compat=power7 or -cpu POWER8E,compat=power7 etc. When
> > compat is specified on -cpu cmdline, apply the same to all the
> > CPUs that get created as part of CPU core devices.
> > 
> > This patch takes care of compat property that is specified with
> > -cpu cmdline option only. The other way to specify this property
> > is via -global cmdline option and that usage isn't addressed by
> > this patch because -global is already broken for some CPU class
> > in PowerPC. There are two issues with using -global on CPU class.
> > 
> > - We specify alias names for CPUs commonly and use of alias names
> >   won't work with -global seamlessly. For eg, When "-cpu host" is
> >   specified, the actual CPU type that gets created
> >   is host-powerpc64-cpu. Hence specifying -global host.compat=power7
> >   will not work, so it has to be -global host-powerpc64-cpu.compat=power7.
> > 
> > - PowerPC class names have . (dot) and opts parsing doesn't like it.
> >   Specifying -global POWER8E_v2.1-powerpc64-cpu.compat=power7 will not
> >   work as the driver name gets extracted as POWER8E_v2 and hence setting
> >   of global property fails.
> > 
> > The above two aspects could be considered/fixed separately from this
> > patch as this patch allows existing uses of -cpu cpuname,compat= to
> > work correctly after the introducton of sPAPR CPU cores.
> > 
> > Signed-off-by: Bharata B Rao <address@hidden>
> > ---
> > Changes in v2:
> > - No need for a separate property named compat for cores.
> > - Simplified spapr_get_cpu_compat_type() based on David's review.
> 
> I'm still not seeing why this is actually necessary.  If the compat
> property is specified with -global, it will automatically be
> propagated to all threads without core intervention, yes?
> 
> So shouldn't we just make the -cpu fallback effectively set the same
> -global property, rather than bouncing it through the core object?

Yes and the below patch against Igor's cpu_parse_into_global_props_V2
branch exactly does that for sPAPR.

===========
spapr: Parse CPU feature only once

From: Bharata B Rao <address@hidden>

Signed-off-by: Bharata B Rao <address@hidden>
---
 hw/ppc/spapr.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0636642..be24a37 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1727,6 +1727,10 @@ static void ppc_spapr_init(MachineState *machine)
     long load_limit, fw_size;
     bool kernel_le = false;
     char *filename;
+    CPUClass *cc;
+    ObjectClass *oc;
+    const char *typename;
+    gchar **model_pieces;
 
     msi_nonbroken = true;
 
@@ -1785,6 +1789,24 @@ static void ppc_spapr_init(MachineState *machine)
     if (machine->cpu_model == NULL) {
         machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
     }
+
+    model_pieces = g_strsplit(machine->cpu_model, ",", 2);
+    if (!model_pieces[0]) {
+        error_report("Invalid/empty CPU model name");
+        exit(1);
+    }
+
+    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
+    if (oc == NULL) {
+        error_report("Unable to find CPU definition: %s", model_pieces[0]);
+        exit(1);
+    }
+
+    typename = object_class_get_name(oc);
+    cc = CPU_CLASS(oc);
+    cc->parse_features(typename, model_pieces[1], &error_fatal);
+    g_strfreev(model_pieces);
+
     for (i = 0; i < smp_cpus; i++) {
         cpu = cpu_ppc_init(machine->cpu_model);
         if (cpu == NULL) {

===========

So essentially this uses cc->parse_features() to parse the cpu feature string
even before any CPU is initialized and cpu_common_parse_features() will
ensure that compat= and any other properties that are part of -cpu will
be set globally only once. I am following what Igor did for x86 here.

Igor's tree doesn't yet contain PowerPC hotplug patches, hence I couldn't
verify this with spapr cpu cores, but I believe that this is all that is
required (on top of Igor's cpu features work) to ensure that compat= is
applied automatically to all the created CPU threads.

However as I note in the patch description of the original patch in this
mail, -global is broken for certain cpu type names.

-cpu host -global host-powerpc64-cpu.compat=power7
-cpu POWER8 -global POWER8-powerpc64-cpu.compat=power7

work, but

-cpu POWER8E -global POWER8E_v2.1-powerpc64-cpu.compat=power7

doesn't since cpu typename has . (dot) and this needs to be fixed.

Regards,
Bharata.