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

[Anthony Liguori]

On Fri, Dec 13, 2013 at 4:44 AM, Igor Mammedov <address@hidden> 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 <address@hidden>

Pushing stuff to a base class in order to reuse code isn't the right
way to model things.

hotplug is a bus concept.  It doesn't apply widely to all possible devices.

Regards,

Anthony Liguori

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