[PATCH v2 0/7] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread

[PATCH v2 0/7] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread

[Maxim Levitsky]

Hi!

This is a patch series that is a result of my discussion with Paulo on
how to correctly fix the root cause of the BZ #1812399.

The root cause of this bug is the fact that IO thread is running mostly
unlocked versus main thread on which device hotplug is done.

qdev_device_add first creates the device object, then places it on the bus,
and only then realizes it.

However some drivers and currently only virtio-scsi enumerate its child bus
devices on each request that is received from the guest and that can happen on the IO
thread.

Thus we have a window when new device is on the bus but not realized and can be accessed
by the virtio-scsi driver in that state.

Fix that by doing two things:

1. Add partial RCU protection to the list of a bus's child devices.
This allows the scsi IO thread to safely enumerate the child devices
while it races with the hotplug placing the device on the bus.

2. Make the virtio-scsi driver check .realized property of the scsi device
and avoid touching the device if it isn't

Note that in the particular bug report the issue wasn't a race but rather due
to combination of things, the .realize code in the middle managed to trigger IO on the virtqueue
which caused the virtio-scsi driver to access the half realized device. However
since this can happen as well with real IO thread, this patch series was done,
which fixes this as well.

Changes from V1:
  * Patch 2 is new, as suggested by Stefan, added drain_call_rcu() to fix the failing unit test,
    make check pass now

  * Patches 6,7 are new as well: I added scsi_device_get as suggested by Stefan as well, although
    this is more a refactoring that anything else as it doesn't solve
    an existing race.

  * Addressed most of the review feedback from V1
    - still need to decide if we need QTAILQ_FOREACH_WITH_RCU_READ_LOCK

The series were tested with make check and smoke tested by booting a VM and
adding/removing few virtio-scsi disks from it in a loop.

Best regards,
	Maxim Levitsky

Maxim Levitsky (7):
  scsi/scsi_bus: switch search direction in scsi_device_find
  Implement drain_call_rcu and use it in hmp_device_del
  device-core: use RCU for list of childs of a bus
  device-core: use atomic_set on .realized property
  virtio-scsi: don't touch scsi devices that are not yet realized or
    about to be un-realized
  scsi: Add scsi_device_get
  virtio-scsi: use scsi_device_get

 hw/core/bus.c          | 36 ++++++++++++++++++-----------
 hw/core/qdev.c         | 52 ++++++++++++++++++++++++++++++------------
 hw/scsi/scsi-bus.c     | 48 +++++++++++++++++++++++++++++++++-----
 hw/scsi/virtio-scsi.c  | 46 +++++++++++++++++++++++++++++--------
 include/hw/qdev-core.h |  3 +++
 include/hw/scsi/scsi.h |  2 ++
 include/qemu/rcu.h     |  1 +
 qdev-monitor.c         |  3 +++
 util/rcu.c             | 33 +++++++++++++++++++++++++++
 9 files changed, 181 insertions(+), 43 deletions(-)

-- 
2.17.2

[PATCH v2 1/7] scsi/scsi_bus: switch search direction in scsi_device_find

[Maxim Levitsky]

This change will allow us to convert the bus children list to RCU,
while not changing the logic of this function

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/scsi/scsi-bus.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 1c980cab38..7bbc37acec 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1584,7 +1584,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
     BusChild *kid;
     SCSIDevice *target_dev = NULL;
 
-    QTAILQ_FOREACH_REVERSE(kid, &bus->qbus.children, sibling) {
+    QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         SCSIDevice *dev = SCSI_DEVICE(qdev);
 
@@ -1592,7 +1592,15 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
             if (dev->lun == lun) {
                 return dev;
             }
-            target_dev = dev;
+
+            /*
+             * If we don't find exact match (channel/bus/lun),
+             * we will return the first device which matches channel/bus
+             */
+
+            if (!target_dev) {
+                target_dev = dev;
+            }
         }
     }
     return target_dev;
-- 
2.17.2

[PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del

[Maxim Levitsky]

This allows to preserve the semantics of hmp_device_del,
that the device is deleted immediatly which was changed by previos
patch that delayed this to RCU callback

Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 include/qemu/rcu.h |  1 +
 qdev-monitor.c     |  3 +++
 util/rcu.c         | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+)

diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
index 570aa603eb..0e375ebe13 100644
--- a/include/qemu/rcu.h
+++ b/include/qemu/rcu.h
@@ -133,6 +133,7 @@ struct rcu_head {
 };
 
 extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
+extern void drain_call_rcu(void);
 
 /* The operands of the minus operator must have the same type,
  * which must be the one that we specify in the cast.
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 56cee1483f..70877840a2 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
         return;
     }
     dev = qdev_device_add(opts, &local_err);
+    drain_call_rcu();
+
     if (!dev) {
         error_propagate(errp, local_err);
         qemu_opts_del(opts);
@@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp)
         }
 
         qdev_unplug(dev, errp);
+        drain_call_rcu();
     }
 }
 
diff --git a/util/rcu.c b/util/rcu.c
index 60a37f72c3..e8b1c4d6c5 100644
--- a/util/rcu.c
+++ b/util/rcu.c
@@ -293,6 +293,39 @@ void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node))
     qemu_event_set(&rcu_call_ready_event);
 }
 
+
+struct rcu_drain {
+    struct rcu_head rcu;
+    QemuEvent drain_complete_event;
+};
+
+static void drain_rcu_callback(struct rcu_head *node)
+{
+    struct rcu_drain *event = (struct rcu_drain *)node;
+    qemu_event_set(&event->drain_complete_event);
+}
+
+void drain_call_rcu(void)
+{
+    struct rcu_drain rcu_drain;
+    bool locked = qemu_mutex_iothread_locked();
+
+    memset(&rcu_drain, 0, sizeof(struct rcu_drain));
+
+    if (locked) {
+        qemu_mutex_unlock_iothread();
+    }
+
+    qemu_event_init(&rcu_drain.drain_complete_event, false);
+    call_rcu1(&rcu_drain.rcu, drain_rcu_callback);
+    qemu_event_wait(&rcu_drain.drain_complete_event);
+
+    if (locked) {
+        qemu_mutex_lock_iothread();
+    }
+
+}
+
 void rcu_register_thread(void)
 {
     assert(rcu_reader.ctr == 0);
-- 
2.17.2

[PATCH v2 3/7] device-core: use RCU for list of childs of a bus

[Maxim Levitsky]

This fixes the race between device emulation code that tries to find
a child device to dispatch the request to (e.g a scsi disk),
and hotplug of a new device to that bus.

Note that this doesn't convert all the readers of the list
but only these that might go over that list without BQL held.

This is a very small first step to make this code thread safe.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/core/bus.c          | 36 +++++++++++++++++++++++-------------
 hw/core/qdev.c         | 37 +++++++++++++++++++++++--------------
 hw/scsi/scsi-bus.c     | 17 ++++++++++++++---
 hw/scsi/virtio-scsi.c  |  6 +++++-
 include/hw/qdev-core.h |  3 +++
 5 files changed, 68 insertions(+), 31 deletions(-)

diff --git a/hw/core/bus.c b/hw/core/bus.c
index 3dc0a825f0..dd6b155ff1 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -49,12 +49,14 @@ int qbus_walk_children(BusState *bus,
         }
     }
 
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        err = qdev_walk_children(kid->child,
-                                 pre_devfn, pre_busfn,
-                                 post_devfn, post_busfn, opaque);
-        if (err < 0) {
-            return err;
+    WITH_RCU_READ_LOCK_GUARD() {
+        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+            err = qdev_walk_children(kid->child,
+                                     pre_devfn, pre_busfn,
+                                     post_devfn, post_busfn, opaque);
+            if (err < 0) {
+                return err;
+            }
         }
     }
 
@@ -90,9 +92,13 @@ static void bus_reset_child_foreach(Object *obj, ResettableChildCallback cb,
     BusState *bus = BUS(obj);
     BusChild *kid;
 
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
+    rcu_read_lock();
+
+    QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
         cb(OBJECT(kid->child), opaque, type);
     }
+
+    rcu_read_unlock();
 }
 
 static void qbus_realize(BusState *bus, DeviceState *parent, const char *name)
@@ -185,14 +191,18 @@ static void bus_set_realized(Object *obj, bool value, Error **errp)
 
         /* TODO: recursive realization */
     } else if (!value && bus->realized) {
-        QTAILQ_FOREACH(kid, &bus->children, sibling) {
-            DeviceState *dev = kid->child;
-            object_property_set_bool(OBJECT(dev), false, "realized",
-                                     &local_err);
-            if (local_err != NULL) {
-                break;
+
+        WITH_RCU_READ_LOCK_GUARD() {
+            QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+                DeviceState *dev = kid->child;
+                object_property_set_bool(OBJECT(dev), false, "realized",
+                                         &local_err);
+                if (local_err != NULL) {
+                    break;
+                }
             }
         }
+
         if (bc->unrealize && local_err == NULL) {
             bc->unrealize(bus, &local_err);
         }
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index dd77a56067..732789e2b7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -51,6 +51,12 @@ const VMStateDescription *qdev_get_vmsd(DeviceState *dev)
     return dc->vmsd;
 }
 
+static void bus_free_bus_child(BusChild *kid)
+{
+    object_unref(OBJECT(kid->child));
+    g_free(kid);
+}
+
 static void bus_remove_child(BusState *bus, DeviceState *child)
 {
     BusChild *kid;
@@ -60,15 +66,16 @@ static void bus_remove_child(BusState *bus, DeviceState *child)
             char name[32];
 
             snprintf(name, sizeof(name), "child[%d]", kid->index);
-            QTAILQ_REMOVE(&bus->children, kid, sibling);
+            QTAILQ_REMOVE_RCU(&bus->children, kid, sibling);
 
             bus->num_children--;
 
             /* This gives back ownership of kid->child back to us.  */
             object_property_del(OBJECT(bus), name, NULL);
-            object_unref(OBJECT(kid->child));
-            g_free(kid);
-            return;
+
+            /* free the bus kid, when it is safe to do so*/
+            call_rcu(kid, bus_free_bus_child, rcu);
+            break;
         }
     }
 }
@@ -83,7 +90,7 @@ static void bus_add_child(BusState *bus, DeviceState *child)
     kid->child = child;
     object_ref(OBJECT(kid->child));
 
-    QTAILQ_INSERT_HEAD(&bus->children, kid, sibling);
+    QTAILQ_INSERT_HEAD_RCU(&bus->children, kid, sibling);
 
     /* This transfers ownership of kid->child to the property.  */
     snprintf(name, sizeof(name), "child[%d]", kid->index);
@@ -682,17 +689,19 @@ DeviceState *qdev_find_recursive(BusState *bus, const char *id)
     DeviceState *ret;
     BusState *child;
 
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        DeviceState *dev = kid->child;
+    WITH_RCU_READ_LOCK_GUARD() {
+        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+            DeviceState *dev = kid->child;
 
-        if (dev->id && strcmp(dev->id, id) == 0) {
-            return dev;
-        }
+            if (dev->id && strcmp(dev->id, id) == 0) {
+                return dev;
+            }
 
-        QLIST_FOREACH(child, &dev->child_bus, sibling) {
-            ret = qdev_find_recursive(child, id);
-            if (ret) {
-                return ret;
+            QLIST_FOREACH(child, &dev->child_bus, sibling) {
+                ret = qdev_find_recursive(child, id);
+                if (ret) {
+                    return ret;
+                }
             }
         }
     }
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 7bbc37acec..2bf6d005f3 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -412,7 +412,10 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
     id = r->req.dev->id;
     found_lun0 = false;
     n = 0;
-    QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
+
+    rcu_read_lock();
+
+    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         SCSIDevice *dev = SCSI_DEVICE(qdev);
 
@@ -433,7 +436,7 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
     memset(r->buf, 0, len);
     stl_be_p(&r->buf[0], n);
     i = found_lun0 ? 8 : 16;
-    QTAILQ_FOREACH(kid, &r->req.bus->qbus.children, sibling) {
+    QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         SCSIDevice *dev = SCSI_DEVICE(qdev);
 
@@ -442,6 +445,9 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
             i += 8;
         }
     }
+
+    rcu_read_unlock();
+
     assert(i == n + 8);
     r->len = len;
     return true;
@@ -1584,12 +1590,15 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
     BusChild *kid;
     SCSIDevice *target_dev = NULL;
 
-    QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
+    rcu_read_lock();
+
+    QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         SCSIDevice *dev = SCSI_DEVICE(qdev);
 
         if (dev->channel == channel && dev->id == id) {
             if (dev->lun == lun) {
+                rcu_read_unlock();
                 return dev;
             }
 
@@ -1603,6 +1612,8 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
             }
         }
     }
+
+    rcu_read_unlock();
     return target_dev;
 }
 
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 472bbd233b..b0f4a35f81 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -367,12 +367,16 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
     case VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET:
         target = req->req.tmf.lun[1];
         s->resetting++;
-        QTAILQ_FOREACH(kid, &s->bus.qbus.children, sibling) {
+
+        rcu_read_lock();
+        QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
              d = SCSI_DEVICE(kid->child);
              if (d->channel == 0 && d->id == target) {
                 qdev_reset_all(&d->qdev);
              }
         }
+        rcu_read_unlock();
+
         s->resetting--;
         break;
 
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index d87d989e72..ef47cb2d9c 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -3,6 +3,8 @@
 
 #include "qemu/queue.h"
 #include "qemu/bitmap.h"
+#include "qemu/rcu.h"
+#include "qemu/rcu_queue.h"
 #include "qom/object.h"
 #include "hw/hotplug.h"
 #include "hw/resettable.h"
@@ -230,6 +232,7 @@ struct BusClass {
 };
 
 typedef struct BusChild {
+    struct rcu_head rcu;
     DeviceState *child;
     int index;
     QTAILQ_ENTRY(BusChild) sibling;
-- 
2.17.2

[PATCH v2 4/7] device-core: use atomic_set on .realized property

[Maxim Levitsky]

Some code might race with placement of new devices on a bus.
We currently first place a (unrealized) device on the bus
and then realize it.

As a workaround, users that scan the child device list, can
check the realized property to see if it is safe to access such a device.
Use an atomic write here too to aid with this.

A separate discussion is what to do with devices that are unrealized:
It looks like for this case we only call the hotplug handler's unplug
callback and its up to it to unrealize the device.
An atomic operation doesn't cause harm for this code path though.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/core/qdev.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 732789e2b7..d530c5922f 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -964,7 +964,20 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
             }
        }
 
+       atomic_store_release(&dev->realized, value);
+
     } else if (!value && dev->realized) {
+
+        /*
+         * Change the value so that any concurrent users are aware
+         * that the device is going to be unrealized
+         *
+         * TODO: change .realized property to enum that states
+         * each phase of the device realization/unrealization
+         */
+
+        atomic_store_release(&dev->realized, value);
+
         /* We want local_err to track only the first error */
         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
             object_property_set_bool(OBJECT(bus), false, "realized",
@@ -980,12 +993,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
 
         if (local_err != NULL) {
+            atomic_store_release(&dev->realized, true);
             goto fail;
         }
     }
 
     assert(local_err == NULL);
-    dev->realized = value;
     return;
 
 child_realize_fail:
-- 
2.17.2

[PATCH v2 5/7] virtio-scsi: don't touch scsi devices that are not yet realized or about to be un-realized

[Maxim Levitsky]

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1812399

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/scsi/virtio-scsi.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index b0f4a35f81..1cc1fc557c 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -35,13 +35,30 @@ static inline int virtio_scsi_get_lun(uint8_t *lun)
 
 static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
 {
+    SCSIDevice *device = NULL;
+
     if (lun[0] != 1) {
         return NULL;
     }
     if (lun[2] != 0 && !(lun[2] >= 0x40 && lun[2] < 0x80)) {
         return NULL;
     }
-    return scsi_device_find(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
+
+    device = scsi_device_find(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
+
+    /*
+     * This function might run on the IO thread and we might race against
+     * main thread hot-plugging the device.
+     *
+     * We assume that as soon as .realized is set to true we can let
+     * the user access the device.
+     */
+
+    if (!device || !atomic_load_acquire(&device->qdev.realized)) {
+        return NULL;
+    }
+
+    return device;
 }
 
 void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
-- 
2.17.2

[PATCH v2 6/7] scsi: Add scsi_device_get

[Maxim Levitsky]

Add scsi_device_get which finds the scsi device
and takes a reference to it.

Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/scsi/scsi-bus.c     | 31 ++++++++++++++++++++++++-------
 include/hw/scsi/scsi.h |  2 ++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 2bf6d005f3..4e15de0bd7 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1584,8 +1584,13 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
     return g_strdup_printf("channel@%x/%s@%x,%x", d->channel,
                            qdev_fw_name(dev), d->id, d->lun);
 }
-
-SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
+/*
+ * Finds a matching channel/id/lun device on scsi bus, and
+ * takes a reference to it and returns it.
+ * If we don't find exact match (channel/bus/lun),
+ * we will return the first device which matches channel/bus
+ * */
+SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun)
 {
     BusChild *kid;
     SCSIDevice *target_dev = NULL;
@@ -1598,25 +1603,37 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
 
         if (dev->channel == channel && dev->id == id) {
             if (dev->lun == lun) {
+                object_ref(OBJECT(dev));
                 rcu_read_unlock();
                 return dev;
             }
 
-            /*
-             * If we don't find exact match (channel/bus/lun),
-             * we will return the first device which matches channel/bus
-             */
-
             if (!target_dev) {
                 target_dev = dev;
             }
         }
     }
 
+    object_ref(OBJECT(target_dev));
     rcu_read_unlock();
     return target_dev;
 }
 
+/*
+ * This function works like scsi_device_get but doesn't take a refernce
+ * to the returned object. Intended for legacy code
+ */
+SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
+{
+    SCSIDevice *dev = scsi_device_get(bus, channel, id, lun);
+
+    if (!dev)
+        return NULL;
+
+    object_unref(OBJECT(dev));
+    return dev;
+}
+
 /* SCSI request list.  For simplicity, pv points to the whole device */
 
 static int put_scsi_requests(QEMUFile *f, void *pv, size_t size,
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 332ef602f4..5e1d31ab6d 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -193,7 +193,9 @@ void scsi_generic_read_device_inquiry(SCSIDevice *dev);
 int scsi_device_get_sense(SCSIDevice *dev, uint8_t *buf, int len, bool fixed);
 int scsi_SG_IO_FROM_DEV(BlockBackend *blk, uint8_t *cmd, uint8_t cmd_size,
                         uint8_t *buf, uint8_t buf_size);
+
 SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int target, int lun);
+SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int target, int lun);
 
 /* scsi-generic.c. */
 extern const SCSIReqOps scsi_generic_req_ops;
-- 
2.17.2

[PATCH v2 7/7] virtio-scsi: use scsi_device_get

[Maxim Levitsky]

This will help us to avoid the scsi device disappearing
after we took a reference to it.

It doesn't by itself forbid case when we try to access
an unrealized device

Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 hw/scsi/virtio-scsi.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 1cc1fc557c..65cd4186fe 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -33,7 +33,7 @@ static inline int virtio_scsi_get_lun(uint8_t *lun)
     return ((lun[2] << 8) | lun[3]) & 0x3FFF;
 }
 
-static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
+static inline SCSIDevice *virtio_scsi_device_get(VirtIOSCSI *s, uint8_t *lun)
 {
     SCSIDevice *device = NULL;
 
@@ -44,7 +44,7 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
         return NULL;
     }
 
-    device = scsi_device_find(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
+    device = scsi_device_get(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
 
     /*
      * This function might run on the IO thread and we might race against
@@ -61,6 +61,8 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
     return device;
 }
 
+
+
 void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -273,7 +275,7 @@ static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, SCSIDevice *d)
  *  case of async cancellation. */
 static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
 {
-    SCSIDevice *d = virtio_scsi_device_find(s, req->req.tmf.lun);
+    SCSIDevice *d = virtio_scsi_device_get(s, req->req.tmf.lun);
     SCSIRequest *r, *next;
     BusChild *kid;
     int target;
@@ -387,10 +389,10 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
 
         rcu_read_lock();
         QTAILQ_FOREACH_RCU(kid, &s->bus.qbus.children, sibling) {
-             d = SCSI_DEVICE(kid->child);
-             if (d->channel == 0 && d->id == target) {
-                qdev_reset_all(&d->qdev);
-             }
+            SCSIDevice *d1 = SCSI_DEVICE(kid->child);
+            if (d1->channel == 0 && d1->id == target) {
+                qdev_reset_all(&d1->qdev);
+            }
         }
         rcu_read_unlock();
 
@@ -403,14 +405,17 @@ static int virtio_scsi_do_tmf(VirtIOSCSI *s, VirtIOSCSIReq *req)
         break;
     }
 
+    object_unref(OBJECT(d));
     return ret;
 
 incorrect_lun:
     req->resp.tmf.response = VIRTIO_SCSI_S_INCORRECT_LUN;
+    object_unref(OBJECT(d));
     return ret;
 
 fail:
     req->resp.tmf.response = VIRTIO_SCSI_S_BAD_TARGET;
+    object_unref(OBJECT(d));
     return ret;
 }
 
@@ -581,7 +586,7 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
         }
     }
 
-    d = virtio_scsi_device_find(s, req->req.cmd.lun);
+    d = virtio_scsi_device_get(s, req->req.cmd.lun);
     if (!d) {
         req->resp.cmd.response = VIRTIO_SCSI_S_BAD_TARGET;
         virtio_scsi_complete_cmd_req(req);
@@ -597,10 +602,12 @@ static int virtio_scsi_handle_cmd_req_prepare(VirtIOSCSI *s, VirtIOSCSIReq *req)
             req->sreq->cmd.xfer > req->qsgl.size)) {
         req->resp.cmd.response = VIRTIO_SCSI_S_OVERRUN;
         virtio_scsi_complete_cmd_req(req);
+        object_unref(OBJECT(d));
         return -ENOBUFS;
     }
     scsi_req_ref(req->sreq);
     blk_io_plug(d->conf.blk);
+    object_unref(OBJECT(d));
     return 0;
 }
 
-- 
2.17.2

Re: [PATCH v2 0/7] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200511160951.8733-1-mlevitsk@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  TEST    iotest-qcow2: 111
  TEST    iotest-qcow2: 114
**
ERROR:/tmp/qemu-test/src/qom/object.c:1124:object_unref: assertion failed: (obj->ref > 0)
Broken pipe
/tmp/qemu-test/src/tests/qtest/libqtest.c:175: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
ERROR - too few tests run (expected 6, got 5)
make: *** [check-qtest-aarch64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 117
  TEST    iotest-qcow2: 120
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=f4bb7d21c4384634a322c2f5cd38b1bf', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-t97jyowh/src/docker-src.2020-05-11-13.48.16.8561:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=f4bb7d21c4384634a322c2f5cd38b1bf
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-t97jyowh/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    15m0.135s
user    0m8.625s


The full log is available at
http://patchew.org/logs/20200511160951.8733-1-mlevitsk@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 0/7] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200511160951.8733-1-mlevitsk@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200511160951.8733-1-mlevitsk@redhat.com
Subject: [PATCH v2 0/7] RFC/WIP: Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
f10acc6 virtio-scsi: use scsi_device_get
4b28e41 scsi: Add scsi_device_get
969c784 virtio-scsi: don't touch scsi devices that are not yet realized or about to be un-realized
c95c33f device-core: use atomic_set on .realized property
239a8ce device-core: use RCU for list of childs of a bus
dd0c3a8 Implement drain_call_rcu and use it in hmp_device_del
cc7a085 scsi/scsi_bus: switch search direction in scsi_device_find

=== OUTPUT BEGIN ===
1/7 Checking commit cc7a085c2c59 (scsi/scsi_bus: switch search direction in scsi_device_find)
2/7 Checking commit dd0c3a8cfd9b (Implement drain_call_rcu and use it in hmp_device_del)
3/7 Checking commit 239a8cee3c60 (device-core: use RCU for list of childs of a bus)
4/7 Checking commit c95c33f4a7dc (device-core: use atomic_set on .realized property)
5/7 Checking commit 969c784b8d8e (virtio-scsi: don't touch scsi devices that are not yet realized or about to be un-realized)
6/7 Checking commit 4b28e41ee772 (scsi: Add scsi_device_get)
WARNING: Block comments use a trailing */ on a separate line
#29: FILE: hw/scsi/scsi-bus.c:1592:
+ * */

ERROR: braces {} are necessary for all arms of this statement
#67: FILE: hw/scsi/scsi-bus.c:1630:
+    if (!dev)
[...]

total: 1 errors, 1 warnings, 66 lines checked

Patch 6/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

7/7 Checking commit f10acc631cf7 (virtio-scsi: use scsi_device_get)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200511160951.8733-1-mlevitsk@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 1/7] scsi/scsi_bus: switch search direction in scsi_device_find

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 398 bytes --]

On Mon, May 11, 2020 at 07:09:45PM +0300, Maxim Levitsky wrote:
> This change will allow us to convert the bus children list to RCU,
> while not changing the logic of this function
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/scsi/scsi-bus.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]

On Mon, May 11, 2020 at 07:09:46PM +0300, Maxim Levitsky wrote:
>  /* The operands of the minus operator must have the same type,
>   * which must be the one that we specify in the cast.
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 56cee1483f..70877840a2 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
>          return;
>      }
>      dev = qdev_device_add(opts, &local_err);
> +    drain_call_rcu();

Please include comments explaining what each drain waits for. Without
comments we'll quickly lose track of why drain_call_rcu() calls are
necessary (similar to documenting memory barrier or refcount inc/dec
pairing).

> diff --git a/util/rcu.c b/util/rcu.c
> index 60a37f72c3..e8b1c4d6c5 100644
> --- a/util/rcu.c
> +++ b/util/rcu.c
> @@ -293,6 +293,39 @@ void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node))
>      qemu_event_set(&rcu_call_ready_event);
>  }
>  
> +
> +struct rcu_drain {
> +    struct rcu_head rcu;
> +    QemuEvent drain_complete_event;
> +};
> +
> +static void drain_rcu_callback(struct rcu_head *node)
> +{
> +    struct rcu_drain *event = (struct rcu_drain *)node;
> +    qemu_event_set(&event->drain_complete_event);

A comment would be nice explaining that callbacks are invoked in
sequence so we're sure that all previously scheduled callbacks have
completed when drain_rcu_callback() is invoked.

> +}
> +
> +void drain_call_rcu(void)

Please document that the main loop mutex is dropped if it's held. This
will prevent surprises and allow callers to think about thread-safety
across this call.

Aside from the comment requests:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/7] device-core: use RCU for list of childs of a bus

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Mon, May 11, 2020 at 07:09:47PM +0300, Maxim Levitsky wrote:
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index d87d989e72..ef47cb2d9c 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -3,6 +3,8 @@
>  
>  #include "qemu/queue.h"
>  #include "qemu/bitmap.h"
> +#include "qemu/rcu.h"
> +#include "qemu/rcu_queue.h"
>  #include "qom/object.h"
>  #include "hw/hotplug.h"
>  #include "hw/resettable.h"
> @@ -230,6 +232,7 @@ struct BusClass {
>  };
>  
>  typedef struct BusChild {
> +    struct rcu_head rcu;
>      DeviceState *child;
>      int index;
>      QTAILQ_ENTRY(BusChild) sibling;

Please add a doc comment to struct BusState saying the children field is
an RCU QTAILQ and writers must hold the QEMU global mutex.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/7] device-core: use atomic_set on .realized property

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

On Mon, May 11, 2020 at 07:09:48PM +0300, Maxim Levitsky wrote:
> Some code might race with placement of new devices on a bus.
> We currently first place a (unrealized) device on the bus
> and then realize it.
> 
> As a workaround, users that scan the child device list, can
> check the realized property to see if it is safe to access such a device.
> Use an atomic write here too to aid with this.
> 
> A separate discussion is what to do with devices that are unrealized:
> It looks like for this case we only call the hotplug handler's unplug
> callback and its up to it to unrealize the device.
> An atomic operation doesn't cause harm for this code path though.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/core/qdev.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)

Please add a comment to struct DeviceState saying the realized field
must be accessed with atomic_load_acquire() when used outside the QEMU
global mutex.

> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 732789e2b7..d530c5922f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -964,7 +964,20 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>              }
>         }
>  
> +       atomic_store_release(&dev->realized, value);
> +
>      } else if (!value && dev->realized) {
> +
> +        /*
> +         * Change the value so that any concurrent users are aware
> +         * that the device is going to be unrealized
> +         *
> +         * TODO: change .realized property to enum that states
> +         * each phase of the device realization/unrealization
> +         */
> +
> +        atomic_store_release(&dev->realized, value);

I'm not sure if atomic_store_release() is strong enough in the true ->
false case:

  Operations coming after ``atomic_store_release()`` can still be
  reordered before it.

A reader may already seen changes made to unrealize the DeviceState even
though realized still appears to be true. A full write memory barrier
seems safer here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 5/7] virtio-scsi: don't touch scsi devices that are not yet realized or about to be un-realized

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

On Mon, May 11, 2020 at 07:09:49PM +0300, Maxim Levitsky wrote:
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1812399
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 6/7] scsi: Add scsi_device_get

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

On Mon, May 11, 2020 at 07:09:50PM +0300, Maxim Levitsky wrote:
> +/*
> + * This function works like scsi_device_get but doesn't take a refernce

s/refernce/reference/

> + * to the returned object. Intended for legacy code

The following explains this in more detail. It's not necessarily legacy
code but rather whether it runs under the QEMU global mutex or not:

Devices that run under the QEMU global mutex can use this function.
Devices that run outside the QEMU global mutex must use
scsi_device_get() instead.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 7/7] virtio-scsi: use scsi_device_get

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

On Mon, May 11, 2020 at 07:09:51PM +0300, Maxim Levitsky wrote:
> This will help us to avoid the scsi device disappearing
> after we took a reference to it.
> 
> It doesn't by itself forbid case when we try to access
> an unrealized device
> 
> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)

I'm not very familiar with the SCSI emulation code, but this looks
correct. My understanding of what this patch does:

This patch keeps SCSIDevice alive between scsi_device_find() and
scsi_req_new(). Previously no SCSIDevice ref was taken so the device
could have been freed before scsi_req_new() had a chance to take a ref.

The TMF case is similar: the SCSIDevice ref must be held during
virtio_scsi_do_tmf(). We don't need to worry about the async cancel
notifiers because the request being canceled already holds a ref.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 7/7] virtio-scsi: use scsi_device_get

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Mon, May 11, 2020 at 07:09:51PM +0300, Maxim Levitsky wrote:
> This will help us to avoid the scsi device disappearing
> after we took a reference to it.
> 
> It doesn't by itself forbid case when we try to access
> an unrealized device
> 
> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  hw/scsi/virtio-scsi.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del

[Maxim Levitsky]

On Wed, 2020-05-27 at 14:11 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:46PM +0300, Maxim Levitsky wrote:
> >  /* The operands of the minus operator must have the same type,
> >   * which must be the one that we specify in the cast.
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 56cee1483f..70877840a2 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> >          return;
> >      }
> >      dev = qdev_device_add(opts, &local_err);
> > +    drain_call_rcu();
> 
> Please include comments explaining what each drain waits for. Without
> comments we'll quickly lose track of why drain_call_rcu() calls are
> necessary (similar to documenting memory barrier or refcount inc/dec
> pairing).
> 
> > diff --git a/util/rcu.c b/util/rcu.c
> > index 60a37f72c3..e8b1c4d6c5 100644
> > --- a/util/rcu.c
> > +++ b/util/rcu.c
> > @@ -293,6 +293,39 @@ void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node))
> >      qemu_event_set(&rcu_call_ready_event);
> >  }
> >  
> > +
> > +struct rcu_drain {
> > +    struct rcu_head rcu;
> > +    QemuEvent drain_complete_event;
> > +};
> > +
> > +static void drain_rcu_callback(struct rcu_head *node)
> > +{
> > +    struct rcu_drain *event = (struct rcu_drain *)node;
> > +    qemu_event_set(&event->drain_complete_event);
> 
> A comment would be nice explaining that callbacks are invoked in
> sequence so we're sure that all previously scheduled callbacks have
> completed when drain_rcu_callback() is invoked.
> 
> > +}
> > +
> > +void drain_call_rcu(void)
> 
> Please document that the main loop mutex is dropped if it's held. This
> will prevent surprises and allow callers to think about thread-safety
> across this call.

Done.
> 
> Aside from the comment requests:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>


Best regards,
	Maxim levitsky

Re: [PATCH v2 3/7] device-core: use RCU for list of childs of a bus

[Maxim Levitsky]

On Wed, 2020-05-27 at 15:45 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:47PM +0300, Maxim Levitsky wrote:
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index d87d989e72..ef47cb2d9c 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -3,6 +3,8 @@
> >  
> >  #include "qemu/queue.h"
> >  #include "qemu/bitmap.h"
> > +#include "qemu/rcu.h"
> > +#include "qemu/rcu_queue.h"
> >  #include "qom/object.h"
> >  #include "hw/hotplug.h"
> >  #include "hw/resettable.h"
> > @@ -230,6 +232,7 @@ struct BusClass {
> >  };
> >  
> >  typedef struct BusChild {
> > +    struct rcu_head rcu;
> >      DeviceState *child;
> >      int index;
> >      QTAILQ_ENTRY(BusChild) sibling;
> 
> Please add a doc comment to struct BusState saying the children field is
> an RCU QTAILQ and writers must hold the QEMU global mutex.
> 
> Stefan
Done.


Best regards,
	Maxim Levitsky

Re: [PATCH v2 4/7] device-core: use atomic_set on .realized property

[Maxim Levitsky]

On Wed, 2020-05-27 at 16:00 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:48PM +0300, Maxim Levitsky wrote:
> > Some code might race with placement of new devices on a bus.
> > We currently first place a (unrealized) device on the bus
> > and then realize it.
> > 
> > As a workaround, users that scan the child device list, can
> > check the realized property to see if it is safe to access such a device.
> > Use an atomic write here too to aid with this.
> > 
> > A separate discussion is what to do with devices that are unrealized:
> > It looks like for this case we only call the hotplug handler's unplug
> > callback and its up to it to unrealize the device.
> > An atomic operation doesn't cause harm for this code path though.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  hw/core/qdev.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> Please add a comment to struct DeviceState saying the realized field
> must be accessed with atomic_load_acquire() when used outside the QEMU
> global mutex.
> 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 732789e2b7..d530c5922f 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -964,7 +964,20 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> >              }
> >         }
> >  
> > +       atomic_store_release(&dev->realized, value);
> > +
> >      } else if (!value && dev->realized) {
> > +
> > +        /*
> > +         * Change the value so that any concurrent users are aware
> > +         * that the device is going to be unrealized
> > +         *
> > +         * TODO: change .realized property to enum that states
> > +         * each phase of the device realization/unrealization
> > +         */
> > +
> > +        atomic_store_release(&dev->realized, value);
> 
> I'm not sure if atomic_store_release() is strong enough in the true ->
> false case:
> 
>   Operations coming after ``atomic_store_release()`` can still be
>   reordered before it.
> 
> A reader may already seen changes made to unrealize the DeviceState even
> though realized still appears to be true. A full write memory barrier
> seems safer here.
Done.

Best regards,
	Maxim Levitsky

Re: [PATCH v2 6/7] scsi: Add scsi_device_get

[Maxim Levitsky]

On Wed, 2020-05-27 at 16:27 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:50PM +0300, Maxim Levitsky wrote:
> > +/*
> > + * This function works like scsi_device_get but doesn't take a refernce
> 
> s/refernce/reference/
> 
> > + * to the returned object. Intended for legacy code
> 
> The following explains this in more detail. It's not necessarily legacy
> code but rather whether it runs under the QEMU global mutex or not:
> 
> Devices that run under the QEMU global mutex can use this function.
> Devices that run outside the QEMU global mutex must use
> scsi_device_get() instead.
Done.

Best regards,
	Maxim Levitsky

Re: [PATCH v2 7/7] virtio-scsi: use scsi_device_get

[Maxim Levitsky]

On Wed, 2020-05-27 at 16:50 +0100, Stefan Hajnoczi wrote:
> On Mon, May 11, 2020 at 07:09:51PM +0300, Maxim Levitsky wrote:
> > This will help us to avoid the scsi device disappearing
> > after we took a reference to it.
> > 
> > It doesn't by itself forbid case when we try to access
> > an unrealized device
> > 
> > Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  hw/scsi/virtio-scsi.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> I'm not very familiar with the SCSI emulation code, but this looks
> correct. My understanding of what this patch does:
> 
> This patch keeps SCSIDevice alive between scsi_device_find() and
> scsi_req_new(). Previously no SCSIDevice ref was taken so the device
> could have been freed before scsi_req_new() had a chance to take a ref.
Yep, I also verified now that this is what happens.

> 
> The TMF case is similar: the SCSIDevice ref must be held during
> virtio_scsi_do_tmf(). We don't need to worry about the async cancel
> notifiers because the request being canceled already holds a ref.
This code I understand less to be honest, but in the worst case,
the patch shoudn't make it worse.

Best regards,
	Maxim Levitsky

Re: [PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del

[Markus Armbruster]

Maxim Levitsky <mlevitsk@redhat.com> writes:

> This allows to preserve the semantics of hmp_device_del,
> that the device is deleted immediatly which was changed by previos
> patch that delayed this to RCU callback
>
> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  include/qemu/rcu.h |  1 +
>  qdev-monitor.c     |  3 +++
>  util/rcu.c         | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> index 570aa603eb..0e375ebe13 100644
> --- a/include/qemu/rcu.h
> +++ b/include/qemu/rcu.h
> @@ -133,6 +133,7 @@ struct rcu_head {
>  };
>  
>  extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> +extern void drain_call_rcu(void);
>  
>  /* The operands of the minus operator must have the same type,
>   * which must be the one that we specify in the cast.
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 56cee1483f..70877840a2 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
>          return;
>      }
>      dev = qdev_device_add(opts, &local_err);
> +    drain_call_rcu();
> +
>      if (!dev) {
>          error_propagate(errp, local_err);
>          qemu_opts_del(opts);
> @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp)
>          }
>  
>          qdev_unplug(dev, errp);
> +        drain_call_rcu();
>      }
>  }
>  

Subject claims "in hmp_device_del", code has it in qmp_device_add() and
qmp_device_del().  Please advise.

[...]

Re: [PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del

[Maxim Levitsky]

On Thu, 2020-07-09 at 13:42 +0200, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > This allows to preserve the semantics of hmp_device_del,
> > that the device is deleted immediatly which was changed by previos
> > patch that delayed this to RCU callback
> > 
> > Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  include/qemu/rcu.h |  1 +
> >  qdev-monitor.c     |  3 +++
> >  util/rcu.c         | 33 +++++++++++++++++++++++++++++++++
> >  3 files changed, 37 insertions(+)
> > 
> > diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> > index 570aa603eb..0e375ebe13 100644
> > --- a/include/qemu/rcu.h
> > +++ b/include/qemu/rcu.h
> > @@ -133,6 +133,7 @@ struct rcu_head {
> >  };
> >  
> >  extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
> > +extern void drain_call_rcu(void);
> >  
> >  /* The operands of the minus operator must have the same type,
> >   * which must be the one that we specify in the cast.
> > diff --git a/qdev-monitor.c b/qdev-monitor.c
> > index 56cee1483f..70877840a2 100644
> > --- a/qdev-monitor.c
> > +++ b/qdev-monitor.c
> > @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> >          return;
> >      }
> >      dev = qdev_device_add(opts, &local_err);
> > +    drain_call_rcu();
> > +
> >      if (!dev) {
> >          error_propagate(errp, local_err);
> >          qemu_opts_del(opts);
> > @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp)
> >          }
> >  
> >          qdev_unplug(dev, errp);
> > +        drain_call_rcu();
> >      }
> >  }
> >  
> 
> Subject claims "in hmp_device_del", code has it in qmp_device_add() and
> qmp_device_del().  Please advise.

I added it in both, because addition of a device can fail and trigger removal,
which can also be now delayed due to RCU.
Since both device_add and device_del aren't used often, the overhead won't
be a problem IMHO.

Best regards,
	Maxim Levitsky

> 
> [...]

Re: [PATCH v2 2/7] Implement drain_call_rcu and use it in hmp_device_del

[Paolo Bonzini]

On 09/07/20 13:56, Maxim Levitsky wrote:
> On Thu, 2020-07-09 at 13:42 +0200, Markus Armbruster wrote:
>> Maxim Levitsky <mlevitsk@redhat.com> writes:
>>
>>> This allows to preserve the semantics of hmp_device_del,
>>> that the device is deleted immediatly which was changed by previos
>>> patch that delayed this to RCU callback
>>>
>>> Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
>>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>> ---
>>>  include/qemu/rcu.h |  1 +
>>>  qdev-monitor.c     |  3 +++
>>>  util/rcu.c         | 33 +++++++++++++++++++++++++++++++++
>>>  3 files changed, 37 insertions(+)
>>>
>>> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
>>> index 570aa603eb..0e375ebe13 100644
>>> --- a/include/qemu/rcu.h
>>> +++ b/include/qemu/rcu.h
>>> @@ -133,6 +133,7 @@ struct rcu_head {
>>>  };
>>>  
>>>  extern void call_rcu1(struct rcu_head *head, RCUCBFunc *func);
>>> +extern void drain_call_rcu(void);
>>>  
>>>  /* The operands of the minus operator must have the same type,
>>>   * which must be the one that we specify in the cast.
>>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>>> index 56cee1483f..70877840a2 100644
>>> --- a/qdev-monitor.c
>>> +++ b/qdev-monitor.c
>>> @@ -812,6 +812,8 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
>>>          return;
>>>      }
>>>      dev = qdev_device_add(opts, &local_err);
>>> +    drain_call_rcu();
>>> +
>>>      if (!dev) {
>>>          error_propagate(errp, local_err);
>>>          qemu_opts_del(opts);
>>> @@ -904,6 +906,7 @@ void qmp_device_del(const char *id, Error **errp)
>>>          }
>>>  
>>>          qdev_unplug(dev, errp);
>>> +        drain_call_rcu();
>>>      }
>>>  }
>>>  
>>
>> Subject claims "in hmp_device_del", code has it in qmp_device_add() and
>> qmp_device_del().  Please advise.
> 
> I added it in both, because addition of a device can fail and trigger removal,
> which can also be now delayed due to RCU.
> Since both device_add and device_del aren't used often, the overhead won't
> be a problem IMHO.

Ok, just mention this in the commit message.  It may also be a good idea
to move it from qmp_device_add to the error-propagation section of
qdev_device_add.

Paolo