Re: [Qemu-devel] [RFC PATCH v0] spapr: Introduce sPAPRCPUCoreClass

[David Gibson]

On Thu, Sep 08, 2016 at 09:56:31AM +0530, Bharata B Rao wrote:
> On Thu, Sep 08, 2016 at 11:51:32AM +1000, David Gibson wrote:
> > On Fri, Sep 02, 2016 at 03:06:38PM +0530, Bharata B Rao wrote:
> > > Each spapr cpu core type defines an instance_init routine which just
> > > populates the CPU class name. This can be done in the class_init
> > > commonly for all core types which simplifies the registration.
> > > This is inspired by how PowerNV core types are registered.
> > > 
> > > Certain types of spapr cpu cores ('host' and generic type based on host
> > > CPU) are initialized in target-ppc/kvm.c. To convert these type
> > > registrations to use class_init, we need to expose
> > > spapr_cpu_core_class_init() outside of spapr_cpu_core.c.
> > > 
> > > Commit d11b268e1765 added a generic sPAPR CPU core family
> > > type to support cases like POWER8 CPU type on POWER8E host CPU.
> > > Switching to class_init would fix such scenarios to use the right
> > > CPU thread type instead of defaulting to host-powerpc64-cpu.
> > > 
> > > In an unrelated cleanup, fix a typo in .get_hotplug_handler routine.
> > > 
> > > Signed-off-by: Bharata B Rao <address@hidden>
> > 
> > I like the concept, but...
> > 
> > [snip]
> > >  static const TypeInfo spapr_cpu_core_type_info = {
> > > @@ -415,17 +377,26 @@ static const TypeInfo spapr_cpu_core_type_info = {
> > >      .parent = TYPE_CPU_CORE,
> > >      .abstract = true,
> > >      .instance_size = sizeof(sPAPRCPUCore),
> > > -    .class_init = spapr_cpu_core_class_init,
> > 
> > .. I'm pretty sure you need .class_size = sizeof(sPAPRCPUCoreClass)
> > here, or initializing the cpu_class field will corrupt memory.
> 
> Ok.
> 
> The other thing I am not comfortable with is exposing
> .class_init routine outside of spapr_cpu_core.c so that cpu cores
> can be initialized from target-ppc/kvm.c. Any opinion on that ?

Not a strong one.  I guess my feeling is that it's a bit ugly, but
worth it for the other advantages of this approach.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature