| Message ID | 1392050814-31814-14-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 10.02.2014 17:48, schrieb Michael S. Tsirkin: > From: Igor Mammedov <imammedo@redhat.com> > > 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. Following > patches will replace PCIDeviceClass.no_hotplug with this > new property. > > In addition expose field as "hotpluggable" readonly property, > to make it possible to read its value via QOM interface. > > Make DeviceClass hotpluggable by default as it was assumed > before. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/hw/qdev-core.h | 3 +++ > hw/core/qdev.c | 29 +++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 41ec533..08d329d 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: indicates if #DeviceClass is hotpluggable, available > + * as readonly "hotpluggable" property of #DeviceState instance > * > * # Realization # > * Devices are constructed in two stages, > @@ -110,6 +112,7 @@ typedef struct DeviceClass { > * TODO remove once we're there > */ > bool cannot_instantiate_with_device_add_yet; > + bool hotpluggable; > > /* callbacks */ > void (*reset)(DeviceState *dev); > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index c9f0c33..5c864db 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) { > @@ -694,6 +700,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; > @@ -734,6 +745,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); > @@ -750,6 +769,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 { > @@ -786,6 +807,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 hotpluggable, > + * should override it in their class_init() > + */ > + klass->hotpluggable = true; Is this intentionally being added to class_base_init or should this have gone into class_init? As seen above, class_base_init has so far been used to "erase" stuff set by the parent classes, not to set random defaults. Regards, Andreas > } > > static void device_unparent(Object *obj)
On Tue, 18 Feb 2014 17:35:16 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 10.02.2014 17:48, schrieb Michael S. Tsirkin: > > From: Igor Mammedov <imammedo@redhat.com> > > > > 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. Following > > patches will replace PCIDeviceClass.no_hotplug with this > > new property. > > > > In addition expose field as "hotpluggable" readonly property, > > to make it possible to read its value via QOM interface. > > > > Make DeviceClass hotpluggable by default as it was assumed > > before. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > include/hw/qdev-core.h | 3 +++ > > hw/core/qdev.c | 29 +++++++++++++++++++++++++++++ > > 2 files changed, 32 insertions(+) > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > index 41ec533..08d329d 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: indicates if #DeviceClass is hotpluggable, available > > + * as readonly "hotpluggable" property of #DeviceState instance > > * > > * # Realization # > > * Devices are constructed in two stages, > > @@ -110,6 +112,7 @@ typedef struct DeviceClass { > > * TODO remove once we're there > > */ > > bool cannot_instantiate_with_device_add_yet; > > + bool hotpluggable; > > > > /* callbacks */ > > void (*reset)(DeviceState *dev); > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > > index c9f0c33..5c864db 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) { > > @@ -694,6 +700,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; > > @@ -734,6 +745,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); > > @@ -750,6 +769,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 { > > @@ -786,6 +807,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 hotpluggable, > > + * should override it in their class_init() > > + */ > > + klass->hotpluggable = true; > > Is this intentionally being added to class_base_init or should this have > gone into class_init? As seen above, class_base_init has so far been > used to "erase" stuff set by the parent classes, not to set random defaults. You are right it should be in class_init(), I'll post fix shortly. > Regards, > Andreas > > > } > > > > static void device_unparent(Object *obj) >
Am 10.02.2014 17:48, schrieb Michael S. Tsirkin: > From: Igor Mammedov <imammedo@redhat.com> > > 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. Following > patches will replace PCIDeviceClass.no_hotplug with this > new property. > > In addition expose field as "hotpluggable" readonly property, > to make it possible to read its value via QOM interface. > > Make DeviceClass hotpluggable by default as it was assumed > before. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > include/hw/qdev-core.h | 3 +++ > hw/core/qdev.c | 29 +++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 41ec533..08d329d 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: indicates if #DeviceClass is hotpluggable, available > + * as readonly "hotpluggable" property of #DeviceState instance > * > * # Realization # > * Devices are constructed in two stages, > @@ -110,6 +112,7 @@ typedef struct DeviceClass { > * TODO remove once we're there > */ > bool cannot_instantiate_with_device_add_yet; > + bool hotpluggable; > > /* callbacks */ > void (*reset)(DeviceState *dev); > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index c9f0c33..5c864db 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) { > @@ -694,6 +700,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; > @@ -734,6 +745,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; This is breaking my extended qom-test. You add this property to each device, irrespective of whether it has a non-NULL parent_bus, which results in a SIGSEGV when qom-get'ting it. Will post a fix. Next time please ping for review of such core changes. Andreas > +} > + > static void device_initfn(Object *obj) > { > DeviceState *dev = DEVICE(obj); > @@ -750,6 +769,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 { > @@ -786,6 +807,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 hotpluggable, > + * 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 41ec533..08d329d 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: indicates if #DeviceClass is hotpluggable, available + * as readonly "hotpluggable" property of #DeviceState instance * * # Realization # * Devices are constructed in two stages, @@ -110,6 +112,7 @@ typedef struct DeviceClass { * TODO remove once we're there */ bool cannot_instantiate_with_device_add_yet; + bool hotpluggable; /* callbacks */ void (*reset)(DeviceState *dev); diff --git a/hw/core/qdev.c b/hw/core/qdev.c index c9f0c33..5c864db 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) { @@ -694,6 +700,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; @@ -734,6 +745,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); @@ -750,6 +769,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 { @@ -786,6 +807,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 hotpluggable, + * should override it in their class_init() + */ + klass->hotpluggable = true; } static void device_unparent(Object *obj)