Patchwork [21/26] kvmclock: use realize for kvmclock

login
register
mail settings
Submitter Hu Tao
Date June 22, 2013, 8:50 a.m.
Message ID <e8f3a6058ada9550dc73b825158fbd0248c2606c.1371804804.git.hutao@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/253375/
State New
Headers show

Comments

Hu Tao - June 22, 2013, 8:50 a.m.
Cc: qemu-devel@nongnu.org
Cc: "Andreas Färber" <afaerber@suse.de>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/i386/kvm/clock.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
Igor Mammedov - June 24, 2013, 10:14 a.m.
On Sat, 22 Jun 2013 16:50:33 +0800
Hu Tao <hutao@cn.fujitsu.com> wrote:

> Cc: qemu-devel@nongnu.org
> Cc: "Andreas Färber" <afaerber@suse.de>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/i386/kvm/clock.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 87d4d0f..74aa240 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -87,12 +87,11 @@ static void kvmclock_vm_state_change(void *opaque, int running,
>      }
>  }
>  
> -static int kvmclock_init(SysBusDevice *dev)
> +static void kvmclock_realize(DeviceState *dev, Error **errp)
>  {
> -    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
> +    KVMClockState *s = DO_UPCAST(KVMClockState, busdev.qdev, dev);
it would be better to swap this patch with 22/26 and use KVM_CLOCK() cast here

>  
>      qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> -    return 0;
>  }
>  
>  static const VMStateDescription kvmclock_vmsd = {
> @@ -111,9 +110,8 @@ static const VMStateDescription kvmclock_vmsd = {
>  static void kvmclock_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
> -    k->init = kvmclock_init;
> +    dc->realize = kvmclock_realize;
>      dc->no_user = 1;
>      dc->vmsd = &kvmclock_vmsd;
>  }
Eduardo Habkost - June 24, 2013, 2:01 p.m.
On Sat, Jun 22, 2013 at 04:50:33PM +0800, Hu Tao wrote:
> Cc: qemu-devel@nongnu.org
> Cc: "Andreas Färber" <afaerber@suse.de>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/i386/kvm/clock.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> index 87d4d0f..74aa240 100644
> --- a/hw/i386/kvm/clock.c
> +++ b/hw/i386/kvm/clock.c
> @@ -87,12 +87,11 @@ static void kvmclock_vm_state_change(void *opaque, int running,
>      }
>  }
>  
> -static int kvmclock_init(SysBusDevice *dev)
> +static void kvmclock_realize(DeviceState *dev, Error **errp)
>  {
> -    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
> +    KVMClockState *s = DO_UPCAST(KVMClockState, busdev.qdev, dev);
>  
>      qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> -    return 0;
>  }
>  
>  static const VMStateDescription kvmclock_vmsd = {
> @@ -111,9 +110,8 @@ static const VMStateDescription kvmclock_vmsd = {
>  static void kvmclock_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>  
> -    k->init = kvmclock_init;
> +    dc->realize = kvmclock_realize;

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?


>      dc->no_user = 1;
>      dc->vmsd = &kvmclock_vmsd;
>  }
> -- 
> 1.8.3.1
> 
>
Hu Tao - June 25, 2013, 1:55 a.m.
On Mon, Jun 24, 2013 at 12:14:07PM +0200, Igor Mammedov wrote:
> On Sat, 22 Jun 2013 16:50:33 +0800
> Hu Tao <hutao@cn.fujitsu.com> wrote:
> 
> > Cc: qemu-devel@nongnu.org
> > Cc: "Andreas Färber" <afaerber@suse.de>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Anthony Liguori <aliguori@us.ibm.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/i386/kvm/clock.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> > index 87d4d0f..74aa240 100644
> > --- a/hw/i386/kvm/clock.c
> > +++ b/hw/i386/kvm/clock.c
> > @@ -87,12 +87,11 @@ static void kvmclock_vm_state_change(void *opaque, int running,
> >      }
> >  }
> >  
> > -static int kvmclock_init(SysBusDevice *dev)
> > +static void kvmclock_realize(DeviceState *dev, Error **errp)
> >  {
> > -    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
> > +    KVMClockState *s = DO_UPCAST(KVMClockState, busdev.qdev, dev);
> it would be better to swap this patch with 22/26 and use KVM_CLOCK() cast here

OK. I'll swap them in next version.
Hu Tao - June 25, 2013, 2:20 a.m.
On Mon, Jun 24, 2013 at 11:01:54AM -0300, Eduardo Habkost wrote:
> On Sat, Jun 22, 2013 at 04:50:33PM +0800, Hu Tao wrote:
> > Cc: qemu-devel@nongnu.org
> > Cc: "Andreas Färber" <afaerber@suse.de>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Anthony Liguori <aliguori@us.ibm.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  hw/i386/kvm/clock.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
> > index 87d4d0f..74aa240 100644
> > --- a/hw/i386/kvm/clock.c
> > +++ b/hw/i386/kvm/clock.c
> > @@ -87,12 +87,11 @@ static void kvmclock_vm_state_change(void *opaque, int running,
> >      }
> >  }
> >  
> > -static int kvmclock_init(SysBusDevice *dev)
> > +static void kvmclock_realize(DeviceState *dev, Error **errp)
> >  {
> > -    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
> > +    KVMClockState *s = DO_UPCAST(KVMClockState, busdev.qdev, dev);
> >  
> >      qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
> > -    return 0;
> >  }
> >  
> >  static const VMStateDescription kvmclock_vmsd = {
> > @@ -111,9 +110,8 @@ static const VMStateDescription kvmclock_vmsd = {
> >  static void kvmclock_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > -    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> >  
> > -    k->init = kvmclock_init;
> > +    dc->realize = kvmclock_realize;
> 
> 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?

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.
Eduardo Habkost - June 25, 2013, 5:45 p.m.
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?
> 
> 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.
Andreas Färber - June 30, 2013, 2:36 p.m.
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. 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.
* ... 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
Hu Tao - July 1, 2013, 9:31 a.m.
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. 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.

Fix me if i'm wrong.

>                 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
Andreas Färber - July 1, 2013, 10:20 a.m.
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

Patch

diff --git a/hw/i386/kvm/clock.c b/hw/i386/kvm/clock.c
index 87d4d0f..74aa240 100644
--- a/hw/i386/kvm/clock.c
+++ b/hw/i386/kvm/clock.c
@@ -87,12 +87,11 @@  static void kvmclock_vm_state_change(void *opaque, int running,
     }
 }
 
-static int kvmclock_init(SysBusDevice *dev)
+static void kvmclock_realize(DeviceState *dev, Error **errp)
 {
-    KVMClockState *s = FROM_SYSBUS(KVMClockState, dev);
+    KVMClockState *s = DO_UPCAST(KVMClockState, busdev.qdev, dev);
 
     qemu_add_vm_change_state_handler(kvmclock_vm_state_change, s);
-    return 0;
 }
 
 static const VMStateDescription kvmclock_vmsd = {
@@ -111,9 +110,8 @@  static const VMStateDescription kvmclock_vmsd = {
 static void kvmclock_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-    k->init = kvmclock_init;
+    dc->realize = kvmclock_realize;
     dc->no_user = 1;
     dc->vmsd = &kvmclock_vmsd;
 }