Re: [Qemu-devel] [PATCH 17/23] ppc: spapr: use generic cpu_model parsing

[Igor Mammedov]

On Fri, 6 Oct 2017 16:04:39 +1100
David Gibson <address@hidden> wrote:

> On Thu, Oct 05, 2017 at 06:24:44PM +0200, Igor Mammedov wrote:
> > use generic cpu_model parsing introduced by
> >  (6063d4c0f vl.c: convert cpu_model to cpu type and set of global 
> > properties before machine_init())
> > 
> > it allows to:
> >   * replace sPAPRMachineClass::tcg_default_cpu with
> >     MachineClass::default_cpu_type
> >   * drop cpu_parse_cpu_model() from hw/ppc/spapr.c and reuse
> >     one in vl.c
> >   * simplify spapr_get_cpu_core_type() by removing
> >     not needed anymore recurrsion since alias look up
> >     happens earlier at vl.c and spapr_get_cpu_core_type()
> >     works only with resulted from that cpu type.
> >   * spapr no more needs to parse/depend on being phased out
> >     MachineState::cpu_model, all tha parsing done by generic
> >     code and target specific callback.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > PS:
> >   patch is a little bit big but it's hard to split it due
> >   to dependencies, so pls bear with it.  
> 
> [snip]
> > -static int kvm_ppc_register_host_cpu_type(void)
> > +static int kvm_ppc_register_host_cpu_type(MachineState *ms)
> >  {
> >      TypeInfo type_info = {
> >          .name = TYPE_HOST_POWERPC_CPU,
> >          .class_init = kvmppc_host_cpu_class_init,
> >      };
> > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> >      PowerPCCPUClass *pvr_pcc;
> >      ObjectClass *oc;
> >      DeviceClass *dc;
> > @@ -2504,6 +2505,8 @@ static int kvm_ppc_register_host_cpu_type(void)
> >      }
> >      type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
> >      type_register(&type_info);
> > +    /* override TCG default cpu type with 'host' cpu model */
> > +    mc->default_cpu_type = TYPE_HOST_POWERPC_CPU;  
> 
> Ugh.  I dislike this, it changes the default cpu type to 'host' for
> *all* machine types, whereas previously it was just true for pseries.
> That wasn't by accident.
> 
> In general, I think we want there to be a single default CPU type for
> a machine, regardless of TCG vs. KVM.  This is particularly true for
> many of the ppc machine types - they will only work with a CPU in the
> right "family" (e500 / 440 / POWERx, wahtever); in many of these cases
> KVM PR is usable, but using -cpu host could easily break the machine
> type.
> 
> 'pseries' is different, it (and only it) can be used with KVM HV - and
> usually is.  And KVM HV *only* works with -cpu host, due to hardware
> limitations.
I'd keep override in spapr_machine_class_init() but it's not possible
since it's called before kvm option is parsed and machine is prerequisite for 
kvm option

how about following fixup?

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index abc222e..48dc3f7 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2505,8 +2505,10 @@ static int kvm_ppc_register_host_cpu_type(MachineState 
*ms)
     }
     type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
     type_register(&type_info);
-    /* override TCG default cpu type with 'host' cpu model */
-    mc->default_cpu_type = TYPE_HOST_POWERPC_CPU;
+    if (object_dynamic_cast(ms, TYPE_SPAPR_MACHINE)) {
+        /* override TCG default cpu type with 'host' cpu model */
+        mc->default_cpu_type = TYPE_HOST_POWERPC_CPU;
+    }
 
     oc = object_class_by_name(type_info.name);
     g_assert(oc);


> >      oc = object_class_by_name(type_info.name);
> >      g_assert(oc);
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index 5ee91e8..a968e86 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -10277,6 +10277,19 @@ PowerPCCPUClass 
> > *ppc_cpu_class_by_pvr_mask(uint32_t pvr)
> >      return pcc;
> >  }
> >  
> > +static const char *ppc_cpu_lookup_alias(const char *alias)
> > +{
> > +    int ai;
> > +
> > +    for (ai = 0; ppc_cpu_aliases[ai].alias != NULL; ai++) {
> > +        if (strcmp(ppc_cpu_aliases[ai].alias, alias) == 0) {
> > +            return ppc_cpu_aliases[ai].model;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> >  static ObjectClass *ppc_cpu_class_by_name(const char *name)
> >  {
> >      char *cpu_model, *typename;
> > @@ -10370,19 +10383,6 @@ static void ppc_cpu_parse_featurestr(const char 
> > *typename, char *features,
> >      pcc->parent_parse_features(typename, features, errp);
> >  }
> >  
> > -const char *ppc_cpu_lookup_alias(const char *alias)
> > -{
> > -    int ai;
> > -
> > -    for (ai = 0; ppc_cpu_aliases[ai].alias != NULL; ai++) {
> > -        if (strcmp(ppc_cpu_aliases[ai].alias, alias) == 0) {
> > -            return ppc_cpu_aliases[ai].model;
> > -        }
> > -    }
> > -
> > -    return NULL;
> > -}
> > -  
> 
> This code motion of ppc_cpu_lookup_alias appears to be unrelated to
> the rest of the patch.
> 
> >  PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc)
> >  {
> >      ObjectClass *oc = OBJECT_CLASS(pcc);  
>