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

["Andreas Färber" <afaerber@suse.de>]

Am 17.01.2013 00:43, schrieb 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".

Sorry, and I thought my documentation was too verbose already. ;)

> 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've sent out a patch improving QOM and DeviceClass documentation. :)

>> 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.

We could do that if we fix up the respective DeviceClass::init,
SysBusDeviceClass::init etc. code. Question is (just as with some x86
CPU code) whether it's worth cleaning up when we know that it is to be
refactored later.

> 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".
[snip]

Actually that particular piece of code is unrelated to this discussion
since qemu_init_vcpu() still operates on CPUArchState and thus cannot be
moved into CPUClass yet. The reason is that
cpus.c:qemu_kvm_cpu_thread_fn sets cpu_single_env, and I do not see a
solution for that - suggestions or patches welcome.

However, I see that kvm-all.c:kvm_on_sigbus_vcpu() can be switched to
CPUState now, so that cpus.c:qemu_kvm_eat_signals() can be changed to
CPUState, used from cpus.c:qemu_kvm_wait_io_event().
But cpus.c:cpu_thread_is_idle() still uses env->halted, which is blocked
by the search for an acceptable solution to flush the TLB at CPUState
level (exec.c:cpu_common_post_load()).

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg