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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 11 Dec 2013 17:29:06 +0100
Paolo Bonzini <pbonzini@redhat.com> 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 <imammedo@redhat.com>
> > ---
> >  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

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