Re: [Qemu-devel] [PATCH 05/11] qdev: add "hotplugable" property to Device

[Igor Mammedov]

On Wed, 11 Dec 2013 18:02:11 +0100
Paolo Bonzini <address@hidden> wrote:

> Il 11/12/2013 17:57, Igor Mammedov ha scritto:
> > On Wed, 11 Dec 2013 17:29:06 +0100
> > Paolo Bonzini <address@hidden> wrote:
> > 
> >> Il 11/12/2013 02:01, Igor Mammedov ha scritto:
> >>> Currently it's possible to make PCIDevice not hotplugable by using
> >>> no_hotplug field of PCIDeviceClass. However it limits this
> >>> only to PCI devices and prevents from generalizing hotplug code.
> >>>
> >>> So add similar field to DeviceClass so it could be reused with other
> >>> Devices and would allow to replace PCI specific hotplug callbacks
> >>> with generic implementation.
> >>>
> >>> In addition expose field as "hotplugable" readonly property, to make
> >>> it possible to get it via QOM interface.
> >>>
> >>> Signed-off-by: Igor Mammedov <address@hidden>
> >>> ---
> >>>  hw/core/qdev.c         |    8 ++++++++
> >>>  include/hw/qdev-core.h |    3 +++
> >>>  2 files changed, 11 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>> index 25c2d2c..1b1a684 100644
> >>> --- a/hw/core/qdev.c
> >>> +++ b/hw/core/qdev.c
> >>> @@ -719,6 +719,12 @@ static void device_set_realized(Object *obj, bool 
> >>> value, Error **err)
> >>>      dev->realized = value;
> >>>  }
> >>>  
> >>> +static bool device_get_hotplugable(Object *obj, Error **err)
> >>> +{
> >>> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> >>> +    return dc->hotplugable;
> >>> +}
> >>> +
> >>>  static void device_initfn(Object *obj)
> >>>  {
> >>>      DeviceState *dev = DEVICE(obj);
> >>> @@ -736,6 +742,8 @@ static void device_initfn(Object *obj)
> >>>  
> >>>      object_property_add_bool(obj, "realized",
> >>>                               device_get_realized, device_set_realized, 
> >>> NULL);
> >>> +    object_property_add_bool(obj, "hotplugable",
> >>> +                             device_get_hotplugable, NULL, NULL);
> >>>  
> >>>      class = object_get_class(OBJECT(dev));
> >>>      do {
> >>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> >>> index 684a5da..5d4a9c8 100644
> >>> --- a/include/hw/qdev-core.h
> >>> +++ b/include/hw/qdev-core.h
> >>> @@ -50,6 +50,8 @@ struct VMStateDescription;
> >>>   * is changed to %true. Deprecated, new types inheriting directly from
> >>>   * TYPE_DEVICE should use @realize instead, new leaf types should consult
> >>>   * their respective parent type.
> >>> + * @hotplugable: booleean indicating if #DeviceClass is hotplugable, 
> >>> available
> >>> + * as readonly "hotplugable" property of #DeviceState instance
> >>>   *
> >>>   * # Realization #
> >>>   * Devices are constructed in two stages,
> >>> @@ -99,6 +101,7 @@ typedef struct DeviceClass {
> >>>      const char *desc;
> >>>      Property *props;
> >>>      int no_user;
> >>> +    bool hotplugable;
> >>>  
> >>>      /* callbacks */
> >>>      void (*reset)(DeviceState *dev);
> >>>
> >>
> >> Does this patch break SCSI and virtio-serial hotplug?
> > Ouch,
> > this  patch doesn't but 
> > "[PATCH 07/11] qdev:pci: refactor PCIDevice to use generic "hotplugable"
> > property" does. See 2 hunks in hw/core/qdev.c
> 
> If you set dc->hotpluggable = true, those two hunks could also be moved
> here.
sure. posting as reply to this thread so not to spam list with series respin.
tested with CPU hotplug which was broken too.

> 
> Paolo
> 
> >>
> >> Perhaps you could initialize dc->hotpluggable to true, and return
> > that's probably the easiest way, since before series all devices were
> > considered hotplugable from qdev POV, so it makes sense to make DeviceClass
> > hotplugable by default to avoid unexpected regressions.
> > 
> >> "dc->hotplugable && device->parent_bus->allow_hotplug" from the property
> >> getter? 
> > sure, since there is no bus-less hotlpug it won't hurt.
> > I'll respin amended patch as reply to this thread if it's ok.
> > 
> >> Or just set the class to true in the common superclasses of all
> >> buses that support hotplug, but that's a bit harder.
> > perhaps exercise for the later.
> > 
> >>
> >> Paolo
> > 
> > 
> 


-- 
Regards,
  Igor