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 <pbonz...@redhat.com>
Signed-off-by: Maxim Levitsky <mlevi...@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
