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

[Igor Mammedov <imammedo@redhat.com>]

On Sat, 14 Dec 2013 17:10:35 +1000
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > Currently it's possible to make PCIDevice not hotpluggable 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 "hotpluggable" readonly property, to make
> > it possible to get it via QOM interface.
> >
> > Make DeviceClass hotpluggable by default as it was assumed before.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v4:
> > * s/hotplugable/hotpluggable/
> >
> > v3:
> > * make DeviceClass hotpluggable by default
> >   Since PCIDevice still uses internal no_hotlpug checks it shouldn't
> >   reggress. And follow up patch that converts PCIDevices to use
> >   "hotpluggable" property will take care about not hotpluggable PCI
> >   devices explicitly setting "hotpluggable" to false in their class_init().
> >
> > * move generic hotplug checks from
> >   "7/11 qdev:pci: refactor PCIDevice to use generic "hotplugable" property"
> >   to this patch
> > ---
> >  hw/core/qdev.c         | 29 +++++++++++++++++++++++++++++
> >  include/hw/qdev-core.h |  3 +++
> >  2 files changed, 32 insertions(+)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 25c2d2c..9418fea 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -215,6 +215,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
> >      }
> >      assert(dc->unplug != NULL);
> >
> > +    if (!dc->hotpluggable) {
> > +        error_set(errp, QERR_DEVICE_NO_HOTPLUG,
> > +                  object_get_typename(OBJECT(dev)));
> > +        return;
> > +    }
> > +
> >      qdev_hot_removed = true;
> >
> >      if (dc->unplug(dev) < 0) {
> > @@ -679,6 +685,11 @@ static void device_set_realized(Object *obj, bool value, Error **err)
> >      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> >      Error *local_err = NULL;
> >
> > +    if (dev->hotplugged && !dc->hotpluggable) {
> > +        error_set(err, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> > +        return;
> > +    }
> > +
> >      if (value && !dev->realized) {
> >          if (!obj->parent && local_err == NULL) {
> >              static int unattached_count;
> > @@ -719,6 +730,14 @@ static void device_set_realized(Object *obj, bool value, Error **err)
> >      dev->realized = value;
> >  }
> >
> > +static bool device_get_hotpluggable(Object *obj, Error **err)
> > +{
> > +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> > +    DeviceState *dev = DEVICE(obj);
> > +
> > +    return dc->hotpluggable && dev->parent_bus->allow_hotplug;
> > +}
> > +
> >  static void device_initfn(Object *obj)
> >  {
> >      DeviceState *dev = DEVICE(obj);
> > @@ -736,6 +755,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, "hotpluggable",
> > +                             device_get_hotpluggable, NULL, NULL);
> >
> >      class = object_get_class(OBJECT(dev));
> >      do {
> > @@ -784,6 +805,14 @@ static void device_class_base_init(ObjectClass *class, void *data)
> >       * so do not propagate them to the subclasses.
> >       */
> >      klass->props = NULL;
> > +
> > +    /* by default all devices were considered as hotpluggable,
> > +     * so with intent to check it in generic qdev_unplug() /
> > +     * device_set_realized() functions make every device
> > +     * hotpluggable. Devices that shouldn't be hoplugable,
> > +     * should override it in their class_init()
> > +     */
> > +    klass->hotpluggable = true;
> >  }
> >
> >  static void device_unparent(Object *obj)
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 684a5da..04bbef4 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.
> > + * @hotpluggable: booleean indicating if #DeviceClass is hotpluggable, available
> 
> "boolean". Although the fact that its bool is implicit just by saying:
> 
> "@hotpluggable: indicates if #DeviceClass is hotpluggable ...
sure

> 
> Regards,
> Peter
> 
> > + * as readonly "hotpluggable" 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 hotpluggable;
> >
> >      /* callbacks */
> >      void (*reset)(DeviceState *dev);
> > --
> > 1.8.3.1
> >
> >