Re: [Qemu-ppc] [Qemu-devel] [PATCH] spapr: move registration of "host" CPU core type to machine code

[Igor Mammedov]

On Mon, 25 Sep 2017 23:47:30 +0200
Greg Kurz <address@hidden> wrote:

> On Mon, 25 Sep 2017 17:48:57 +0200
> Greg Kurz <address@hidden> wrote:
> 
> > On Mon, 25 Sep 2017 15:41:34 +0200
> > Igor Mammedov <address@hidden> wrote:
> >   
> > > On Mon, 25 Sep 2017 11:47:33 +0200
> > > Greg Kurz <address@hidden> wrote:
> > >     
> > > > The CPU core abstraction belongs to the machine code. This also gets
> > > > rid of some code duplication.
> > > > 
> > > > Signed-off-by: Greg Kurz <address@hidden>
> > > > ---
> > > > 
> > > > hw/ppc/spapr_cpu_core.h is also included elsewhere in target/ppc/kvm.c
> > > > but this is already handled by the following cleanup patch:
> > > > 
> > > > https://patchwork.ozlabs.org/patch/817598/
> > > > ---
> > > >  hw/ppc/spapr.c                  |    4 ++++
> > > >  hw/ppc/spapr_cpu_core.c         |   34 
> > > > ++++++++++++++++++++++------------
> > > >  include/hw/ppc/spapr_cpu_core.h |    2 +-
> > > >  target/ppc/kvm.c                |   12 ------------
> > > >  4 files changed, 27 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 0ce3ec87ac59..e82c8532ffb0 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -2349,6 +2349,10 @@ static void ppc_spapr_init(MachineState *machine)
> > > >      }
> > > >  
> > > >      /* init CPUs */
> > > > +    if (kvm_enabled()) {
> > > > +        spapr_cpu_core_register_host_type();
> > > > +    }      
> > > why don't we create it statically in hw/ppc/spapr_cpu_core.c
> > > like it's done in x86, i.e.
> > > 
> > >   static void x86_cpu_register_types(void)                                
> > >          
> > >   {                                                                       
> > >          
> > >   ...                              
> > >   #ifdef CONFIG_KVM                                                       
> > >          
> > >       type_register_static(&host_x86_cpu_type_info);                      
> > >          
> > >   #endif                                                                  
> > >          
> > >   } 
> > >   type_init(x86_cpu_register_types)
> > > 
> > > and do the same for host CPU as well?
> > >     
> > 
> > Hi Igor,
> > 
> > Not sure yet why we use dynamic types, but I'd be glad to dig a bit more.  
> 
> So the problem is that it was decided to make the host CPU class a
> subclass of the host's CPU model, and this requires all the CPU model
> classes to be registered beforehand.
> 
> commit 5ba4576b858c0d6056f59abb7e17a2b63f7905f3
> Author: Andreas Färber <address@hidden>
> Date:   Sat Feb 23 11:22:12 2013 +0000
> 
>     target-ppc: Make host CPU a subclass of the host's CPU model
>     
>     This avoids assigning individual class fields and contributors
>     forgetting to add field assignments in KVM-only code.
>     
>     ppc_cpu_class_find_by_pvr() requires the CPU model classes to be
>     registered, so defer host CPU type registration to kvm_arch_init().
>     
>     Only register the host CPU type if there is a class with matching PVR.
>     This lets us drop error handling from instance_init.
>     
>     Signed-off-by: Andreas Färber <address@hidden>
>     Signed-off-by: Alexander Graf <address@hidden>
> 
> I can't think of an alternate way to do this. Any suggestion ?
I don't see from this commit a reason why it can't be done in cpu-models.c
dependencies here are
  mfpvr() - which probably should work without KVM
  ppc_cpu_class_by_pvr() - should work fine if 'host' type is being
                           registered as the last among the other CPU types