[v9,02/17] qdev: unplug blocker for devices

Commit Message

Jag Raman May 3, 2022, 2:16 p.m. UTC

Add blocker to prevent hot-unplug of devices

TYPE_VFIO_USER_SERVER, which is introduced shortly, attaches itself to a
PCIDevice on which it depends. If the attached PCIDevice gets removed
while the server in use, it could cause it crash. To prevent this,
TYPE_VFIO_USER_SERVER adds an unplug blocker for the PCIDevice.

Signed-off-by: Elena Ufimtseva <[e-mail]>
Signed-off-by: John G Johnson <[e-mail]>
Signed-off-by: Jagannathan Raman <[e-mail]>
Reviewed-by: Stefan Hajnoczi <[e-mail]>
---
 include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++
 hw/core/qdev.c         | 24 ++++++++++++++++++++++++
 softmmu/qdev-monitor.c |  4 ++++
 3 files changed, 57 insertions(+)

Comments

Markus Armbruster May 4, 2022, 11:13 a.m. UTC | #1

Jagannathan Raman <[e-mail]> writes:

> Add blocker to prevent hot-unplug of devices
>
> TYPE_VFIO_USER_SERVER, which is introduced shortly, attaches itself to a
> PCIDevice on which it depends. If the attached PCIDevice gets removed
> while the server in use, it could cause it crash. To prevent this,
> TYPE_VFIO_USER_SERVER adds an unplug blocker for the PCIDevice.

Appreciate the explanation :)

>
> Signed-off-by: Elena Ufimtseva <[e-mail]>
> Signed-off-by: John G Johnson <[e-mail]>
> Signed-off-by: Jagannathan Raman <[e-mail]>
> Reviewed-by: Stefan Hajnoczi <[e-mail]>
> ---
>  include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++
>  hw/core/qdev.c         | 24 ++++++++++++++++++++++++
>  softmmu/qdev-monitor.c |  4 ++++
>  3 files changed, 57 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..1b9fa25e5c 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -193,6 +193,7 @@ struct DeviceState {
>      int instance_id_alias;
>      int alias_required_for_version;
>      ResettableState reset;
> +    GSList *unplug_blockers;
>  };
>  
>  struct DeviceListener {
> @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>  void qdev_machine_creation_done(void);
>  bool qdev_machine_modified(void);
>  
> +/*

Use /** here like we do in the other function comments nearby.

In case you're curious: it's GTK-Doc format.  It's intended for
generating documentation from doc comments.  Which we don't do, and
perhaps never will.  But let's be locally consistent.

> + * qdev_add_unplug_blocker: Adds an unplug blocker to a device

Recommend imperative mood for function comments: "Add an unplug
blocker to a device".

More of the same below.

[...]

Jag Raman May 4, 2022, 2 p.m. UTC | #2

> On May 4, 2022, at 7:13 AM, Markus Armbruster <[e-mail]> wrote:
> 
> Jagannathan Raman <[e-mail]> writes:
> 
>> Add blocker to prevent hot-unplug of devices
>> 
>> TYPE_VFIO_USER_SERVER, which is introduced shortly, attaches itself to a
>> PCIDevice on which it depends. If the attached PCIDevice gets removed
>> while the server in use, it could cause it crash. To prevent this,
>> TYPE_VFIO_USER_SERVER adds an unplug blocker for the PCIDevice.
> 
> Appreciate the explanation :)
> 
>> 
>> Signed-off-by: Elena Ufimtseva <[e-mail]>
>> Signed-off-by: John G Johnson <[e-mail]>
>> Signed-off-by: Jagannathan Raman <[e-mail]>
>> Reviewed-by: Stefan Hajnoczi <[e-mail]>
>> ---
>> include/hw/qdev-core.h | 29 +++++++++++++++++++++++++++++
>> hw/core/qdev.c         | 24 ++++++++++++++++++++++++
>> softmmu/qdev-monitor.c |  4 ++++
>> 3 files changed, 57 insertions(+)
>> 
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 92c3d65208..1b9fa25e5c 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -193,6 +193,7 @@ struct DeviceState {
>>     int instance_id_alias;
>>     int alias_required_for_version;
>>     ResettableState reset;
>> +    GSList *unplug_blockers;
>> };
>> 
>> struct DeviceListener {
>> @@ -419,6 +420,34 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
>> void qdev_machine_creation_done(void);
>> bool qdev_machine_modified(void);
>> 
>> +/*
> 
> Use /** here like we do in the other function comments nearby.
> 
> In case you're curious: it's GTK-Doc format.  It's intended for
> generating documentation from doc comments.  Which we don't do, and
> perhaps never will.  But let's be locally consistent.

Sure, will do.

> 
>> + * qdev_add_unplug_blocker: Adds an unplug blocker to a device
> 
> Recommend imperative mood for function comments: "Add an unplug
> blocker to a device".

OK, got it. Will update the comments. :)

--
Jag

> 
> More of the same below.
> 
> [...]
>

Patch

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 92c3d65208..1b9fa25e5c 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -193,6 +193,7 @@  struct DeviceState {
     int instance_id_alias;
     int alias_required_for_version;
     ResettableState reset;
+    GSList *unplug_blockers;
 };
 
 struct DeviceListener {
@@ -419,6 +420,34 @@  void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
 void qdev_machine_creation_done(void);
 bool qdev_machine_modified(void);
 
+/*
+ * qdev_add_unplug_blocker: Adds an unplug blocker to a device
+ *
+ * @dev: Device to be blocked from unplug
+ * @reason: Reason for blocking
+ */
+void qdev_add_unplug_blocker(DeviceState *dev, Error *reason);
+
+/*
+ * qdev_del_unplug_blocker: Removes an unplug blocker from a device
+ *
+ * @dev: Device to be unblocked
+ * @reason: Pointer to the Error used with qdev_add_unplug_blocker.
+ *          Used as a handle to lookup the blocker for deletion.
+ */
+void qdev_del_unplug_blocker(DeviceState *dev, Error *reason);
+
+/*
+ * qdev_unplug_blocked: Confirms if a device is blocked from unplug
+ *
+ * @dev: Device to be tested
+ * @reason: Returns one of the reasons why the device is blocked,
+ *          if any
+ *
+ * Returns: true if device is blocked from unplug, false otherwise
+ */
+bool qdev_unplug_blocked(DeviceState *dev, Error **errp);
+
 /**
  * GpioPolarity: Polarity of a GPIO line
  *
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 84f3019440..0806d8fcaa 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -468,6 +468,28 @@  char *qdev_get_dev_path(DeviceState *dev)
     return NULL;
 }
 
+void qdev_add_unplug_blocker(DeviceState *dev, Error *reason)
+{
+    dev->unplug_blockers = g_slist_prepend(dev->unplug_blockers, reason);
+}
+
+void qdev_del_unplug_blocker(DeviceState *dev, Error *reason)
+{
+    dev->unplug_blockers = g_slist_remove(dev->unplug_blockers, reason);
+}
+
+bool qdev_unplug_blocked(DeviceState *dev, Error **errp)
+{
+    ERRP_GUARD();
+
+    if (dev->unplug_blockers) {
+        error_propagate(errp, error_copy(dev->unplug_blockers->data));
+        return true;
+    }
+
+    return false;
+}
+
 static bool device_get_realized(Object *obj, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
@@ -704,6 +726,8 @@  static void device_finalize(Object *obj)
 
     DeviceState *dev = DEVICE(obj);
 
+    g_assert(!dev->unplug_blockers);
+
     QLIST_FOREACH_SAFE(ngl, &dev->gpios, node, next) {
         QLIST_REMOVE(ngl, node);
         qemu_free_irqs(ngl->in, ngl->num_in);
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 12fe60c467..9cfd59d17c 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -898,6 +898,10 @@  void qdev_unplug(DeviceState *dev, Error **errp)
     HotplugHandlerClass *hdc;
     Error *local_err = NULL;
 
+    if (qdev_unplug_blocked(dev, errp)) {
+        return;
+    }
+
     if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
         return;