Re: [Qemu-devel] [PATCH 21/26] kvmclock: use realize for kvmclock

[Andreas Färber]

Am 01.07.2013 11:31, schrieb Hu Tao:
> On Sun, Jun 30, 2013 at 04:36:13PM +0200, Andreas Färber wrote:
>> Am 25.06.2013 19:45, schrieb Eduardo Habkost:
>>> On Tue, Jun 25, 2013 at 10:20:08AM +0800, Hu Tao wrote:
>>> [...]
>>>>> Is TYPE_SYS_BUS_DEVICE guaranteed to never override ->realize() itself?
>>>>>
>>>>> From DeviceClass documentation:
>>>>>
>>>>>  * If a type derived directly from TYPE_DEVICE implements @realize, it 
>>>>> does
>>>>>  * not need to implement @init and therefore does not need to store and 
>>>>> call
>>>>>  * #DeviceClass' default @realize callback.
>>>>>  * For other types consult the documentation and implementation of the
>>>>>  * respective parent types.
>>>>>
>>>>> The problem is that there's no documentation about ->realize() on
>>>>> SysBusDeviceClass. Can we please explicitly document SysBusDeviceClass
>>>>> expectations about ->realize() first, before making those changes?
>>
>> If someone wants to add a paragraph to sysbus.h:SysBusDeviceClass
>> documentation I would happily ack or pick that up. :)
>>
>>>> IIUC, subclass's overriding ->realize should call parent's ->realize at
>>>> some point. Peter Crosthwaite has a patchset to access a object's parent
>>>> class at 
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2013-06/msg02982.html
>>>>
>>>> Regarding SysBusDevice::init and SysBusDevice::realize, I think it's the
>>>> same as in the case of DeviceClass. If you agree I'll document it as in
>>>> DeviceClass.
>>>
>>> I believe it is reasonable to document that SysBusDevice subclasses
>>> don't need to call the parent ->realize() method, like on DeviceClass.
>>> This is exactly what this patch set does, after all, and I haven't seen
>>> anybody complaining about it yet.
>>
>> So the thing is that SysBusDevice's DeviceClass::init implementation,
>> called by DeviceState's DeviceClass::realize implementation, just calls
>> SysBusDeviceClass::init if non-NULL. When we introduce our own device's
>> realizefn to replace our SysBusDeviceClass::init implementation, there
>> is no point calling that effectively no-op DeviceClass::realize
>> implementation.
> 
> This is true because we are in transition from DeviceClass:init to
> DeviceClass:realize, by calling sub-class's DeviceClass:init in
> DeviceClass's realize.

Correct.

> But once the transition is done, and
> DeviceClass's (and any intermediate devired classes') realize does
> do something, we can't just ignore it in overriding realize.

We have the following hierarchy:

Object
+Device
  + SysBusDevice
    + EHCI
      + FaradayEHCI

Object does not know about realize.

Device has a realizefn that calls DeviceClass::init today, nothing more.
Therefore SysBusDevice doesn't need to additionally call that today.

Since, e.g., EHCI implements a realizefn, derived types need to call
their parent's realizefn, i.e. FaradayEHCI EHCI's or if there were
another model F derived from Faraday, then F FaradayEHCI's. Correct.

Once the transition is done, I expect those four lines to go away, with
Device's realizefn seriously doing nothing, as a fallback to avoid if
(dc->realized) {...} type code.
The sysbus_get_default() assignment could easily be moved to
SysBusDevice's instance_init, so I don't see anything from qdev_create()
/ qdev_init() that would need to be moved there. Do you?

The way Paolo proposed it, realize_children would be separate from
realize and called directly from DeviceState's property setter, so it
could be overridden independently.

Andreas

>>                 And if we tried to, ...
>> * ... how would we decide whether to call the parent's implementation
>> first or last? It's not yes or no, it's no or when. Changing between
>> either is more than just moving one line, it affects error handling and
>> with knowledge about parent implementation never failing we could so far
>> save us some work.
> 
> Agreed.
> 
>> * ... with the current QOM method scheme we'd go insane introducing a
>> FooClass per device to save SysBusDevice's DeviceClass::realize in
>> FooClass::parent_realize. Still waiting for Anthony on whether and how
>> we want to change our scheme.
>>
>> Long story short: If someone wants to mess with base classes they get to
>> check derived classes for compatibility with their change.
>>
>> Ideally qtest would help automate that to some degree.
>> I would be all in favor if someone wanted to add a dummy test case per
>> non-default, non-KVM device converted so that we can see that we didn't
>> screw up -device instantiation too badly.
>>
>> Regards,
>> Andreas
>>
>> -- 
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


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