Re: [Qemu-devel] [RFC qom-cpu 03/15] target-i386: Update CPU to QOM realizefn

[Eduardo Habkost]

On Wed, Jan 16, 2013 at 11:52:47PM +0100, Andreas Färber wrote:
> Am 16.01.2013 17:04, schrieb Eduardo Habkost:
> > On Wed, Jan 16, 2013 at 06:32:48AM +0100, Andreas Färber wrote:
> > [...]
> >> @@ -2247,6 +2247,9 @@ static void x86_cpu_common_class_init(ObjectClass 
> >> *oc, void *data)
> >>  {
> >>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> >>      CPUClass *cc = CPU_CLASS(oc);
> >> +    DeviceClass *dc = DEVICE_CLASS(oc);
> >> +
> >> +    dc->realize = x86_cpu_realizefn;
> > 
> > The DeviceClass documenation says:
> > 
> > "Any type may override the @realize and/or @unrealize callbacks but
> > needs to call (and thus save) the parent type's implementation if so
> > desired."
> > 
> > Why are you not following it?
> 
> "if so desired" - I didn't desire or need to call code that calls an
> initfn that no longer exists after this patch. Same as the ISADevice
> conversion series did not unnecessarily call the DeviceClass-level
> backwards-compatibility realizefn: to save time-consuming
> ...Class::parent_realizefn field additions and to not in the end call
> code that doesn't NULL-check ...DeviceClass::init. That's qdev's old
> "leaf type" concept mentioned in the same documentation.

I had read "if so desired" as "if it's desired to override the realize
callback", not as "if it's desired to call the parent realize function".

I believed every class could assume that subclasses would never override
realize() without calling the parent class' realize function (so we
could add stuff to DeviceClass.realize and CPUClass.realize in the
future and be sure that the code would be always called).

But from the documentation mentioning "new leaf types should consult
their respective parent type", it looks like this decision would be
taken/documented in each base class. If that's the case, then OK.


> 
> I mentioned in the cover letter that this needs to be changed once a
> CPUClass-level realizefn is introduced. I could introduce a no-op
> realizefn there and do the regular store+call.

That was the semantics I was expecting: base classes would safely
introduce realize functions without worrying if subclasses would
override it incorrectly and break it.

Anyway, saving the parent function in every subclass is so cumbersome
that simply documenting it as "CPUClass subclasses must call
qemu_init_vcpu()" sounds easier than "CPUClass subclasses must save the
parent's realize() and call it". So:

Reviewed-by: Eduardo Habkost <address@hidden>

> 
> Note that wherever we set realized = true on CPUs, we need to test and
> re-review for further unchecked uses of parent_bus. It has been pushed
> to qom-cpu-realize branch for anyone that wants to play with the latest
> version.
> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo