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

[Igor Mammedov]

On Fri, 6 Oct 2017 20:35:41 +1100
David Gibson <address@hidden> wrote:

> On Fri, Oct 06, 2017 at 11:20:05AM +0200, Igor Mammedov wrote:
> > 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);  
> 
> Hm, ugly.  But if that's what we have to do, that's what we have to
> do.
Unfortunately, I couldn't come with another way to do it (and not for the lack 
of trying)
I did similar override for s390 (though it has only one machine so it wasn't an 
issue there)