* [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
@ 2020-09-25 17:25 Paolo Bonzini
2020-09-25 17:25 ` [PATCH 01/10] qdev: add "check if address free" callback for buses Paolo Bonzini
` (14 more replies)
0 siblings, 15 replies; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:25 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha, mlevitsk
This is my take on Maxim's patches. Mostly I am avoiding the problems with
"scsi/scsi_bus: switch search direction in scsi_device_find" by adding
a pre-realize callback for BusState that checks for the device address
being in use.
This makes it possible to avoid the tricky search for a preexisting device.
Maxim Levitsky (7):
scsi/scsi_bus: switch search direction in scsi_device_find
device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add
device-core: use RCU for list of children of a bus
device-core: use atomic_set on .realized property
scsi/scsi_bus: Add scsi_device_get
virtio-scsi: use scsi_device_get
scsi/scsi_bus: fix races in REPORT LUNS
Paolo Bonzini (3):
qdev: add "check if address free" callback for buses
scsi: switch to bus->check_address
scsi/scsi-bus: scsi_device_find: don't return unrealized devices
hw/core/bus.c | 28 +++--
hw/core/qdev.c | 73 +++++++++---
hw/net/virtio-net.c | 2 +-
hw/scsi/scsi-bus.c | 262 ++++++++++++++++++++++++++---------------
hw/scsi/virtio-scsi.c | 27 +++--
hw/sd/core.c | 3 +-
include/hw/qdev-core.h | 15 ++-
include/hw/scsi/scsi.h | 1 +
qdev-monitor.c | 22 ++++
9 files changed, 299 insertions(+), 134 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 01/10] qdev: add "check if address free" callback for buses
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
@ 2020-09-25 17:25 ` Paolo Bonzini
2020-09-28 9:30 ` Stefan Hajnoczi
2020-09-30 14:27 ` Maxim Levitsky
2020-09-25 17:25 ` [PATCH 02/10] scsi: switch to bus->check_address Paolo Bonzini
` (13 subsequent siblings)
14 siblings, 2 replies; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:25 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha, mlevitsk
Check if an address is free on the bus before plugging in the
device. This makes it possible to do the check without any
side effects, and to detect the problem early without having
to do it in the realize callback.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/core/qdev.c | 17 +++++++++++++++--
hw/net/virtio-net.c | 2 +-
hw/sd/core.c | 3 ++-
include/hw/qdev-core.h | 3 ++-
4 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 96772a15bd..74db78df36 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -94,13 +94,23 @@ static void bus_add_child(BusState *bus, DeviceState *child)
0);
}
-void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
+static bool bus_check_address(BusState *bus, DeviceState *child, Error **errp)
+{
+ BusClass *bc = BUS_GET_CLASS(bus);
+ return !bc->check_address || bc->check_address(bus, child, errp);
+}
+
+bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
{
BusState *old_parent_bus = dev->parent_bus;
DeviceClass *dc = DEVICE_GET_CLASS(dev);
assert(dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type));
+ if (!bus_check_address(bus, dev, errp)) {
+ return false;
+ }
+
if (old_parent_bus) {
trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)),
old_parent_bus, object_get_typename(OBJECT(old_parent_bus)),
@@ -126,6 +136,7 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
object_unref(OBJECT(old_parent_bus));
object_unref(OBJECT(dev));
}
+ return true;
}
DeviceState *qdev_new(const char *name)
@@ -371,7 +382,9 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
assert(!dev->realized && !dev->parent_bus);
if (bus) {
- qdev_set_parent_bus(dev, bus);
+ if (!qdev_set_parent_bus(dev, bus, errp)) {
+ return false;
+ }
} else {
assert(!DEVICE_GET_CLASS(dev)->bus_type);
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7bf27b9db7..268cecc498 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3142,7 +3142,7 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
error_setg(errp, "virtio_net: couldn't find primary bus");
return false;
}
- qdev_set_parent_bus(n->primary_dev, n->primary_bus);
+ qdev_set_parent_bus(n->primary_dev, n->primary_bus, &error_abort);
n->primary_should_be_hidden = false;
if (!qemu_opt_set_bool(n->primary_device_opts,
"partially_hotplugged", true, errp)) {
diff --git a/hw/sd/core.c b/hw/sd/core.c
index 957d116f1a..08c93b5903 100644
--- a/hw/sd/core.c
+++ b/hw/sd/core.c
@@ -23,6 +23,7 @@
#include "hw/qdev-core.h"
#include "hw/sd/sd.h"
#include "qemu/module.h"
+#include "qapi/error.h"
#include "trace.h"
static inline const char *sdbus_name(SDBus *sdbus)
@@ -240,7 +241,7 @@ void sdbus_reparent_card(SDBus *from, SDBus *to)
readonly = sc->get_readonly(card);
sdbus_set_inserted(from, false);
- qdev_set_parent_bus(DEVICE(card), &to->qbus);
+ qdev_set_parent_bus(DEVICE(card), &to->qbus, &error_abort);
sdbus_set_inserted(to, true);
sdbus_set_readonly(to, readonly);
}
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 72064f4dd4..e62da68a26 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -217,6 +217,7 @@ struct BusClass {
*/
char *(*get_fw_dev_path)(DeviceState *dev);
void (*reset)(BusState *bus);
+ bool (*check_address)(BusState *bus, DeviceState *dev, Error **errp);
BusRealize realize;
BusUnrealize unrealize;
@@ -788,7 +789,7 @@ const char *qdev_fw_name(DeviceState *dev);
Object *qdev_get_machine(void);
/* FIXME: make this a link<> */
-void qdev_set_parent_bus(DeviceState *dev, BusState *bus);
+bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
extern bool qdev_hotplug;
extern bool qdev_hot_removed;
--
2.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 02/10] scsi: switch to bus->check_address
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
2020-09-25 17:25 ` [PATCH 01/10] qdev: add "check if address free" callback for buses Paolo Bonzini
@ 2020-09-25 17:25 ` Paolo Bonzini
2020-09-28 9:43 ` Stefan Hajnoczi
2020-09-30 14:28 ` Maxim Levitsky
2020-09-25 17:25 ` [PATCH 03/10] scsi/scsi_bus: switch search direction in scsi_device_find Paolo Bonzini
` (12 subsequent siblings)
14 siblings, 2 replies; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:25 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha, mlevitsk
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/scsi-bus.c | 122 ++++++++++++++++++++++++++++-----------------
1 file changed, 75 insertions(+), 47 deletions(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 3284a5d1fb..94921c04b1 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -22,33 +22,6 @@ static void scsi_req_dequeue(SCSIRequest *req);
static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len);
static void scsi_target_free_buf(SCSIRequest *req);
-static Property scsi_props[] = {
- DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
- DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
- DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
- DEFINE_PROP_END_OF_LIST(),
-};
-
-static void scsi_bus_class_init(ObjectClass *klass, void *data)
-{
- BusClass *k = BUS_CLASS(klass);
- HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
-
- k->get_dev_path = scsibus_get_dev_path;
- k->get_fw_dev_path = scsibus_get_fw_dev_path;
- hc->unplug = qdev_simple_device_unplug_cb;
-}
-
-static const TypeInfo scsi_bus_info = {
- .name = TYPE_SCSI_BUS,
- .parent = TYPE_BUS,
- .instance_size = sizeof(SCSIBus),
- .class_init = scsi_bus_class_init,
- .interfaces = (InterfaceInfo[]) {
- { TYPE_HOTPLUG_HANDLER },
- { }
- }
-};
static int next_scsi_bus;
static void scsi_device_realize(SCSIDevice *s, Error **errp)
@@ -160,35 +133,68 @@ static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
}
}
-static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
+static bool scsi_bus_is_address_free(SCSIBus *bus,
+ int channel, int target, int lun,
+ SCSIDevice **p_dev)
+{
+ SCSIDevice *d = scsi_device_find(bus, channel, target, lun);
+ if (d && d->lun == lun) {
+ if (p_dev) {
+ *p_dev = d;
+ }
+ return false;
+ }
+ if (p_dev) {
+ *p_dev = NULL;
+ }
+ return true;
+}
+
+static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)
{
SCSIDevice *dev = SCSI_DEVICE(qdev);
- SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
- SCSIDevice *d;
- Error *local_err = NULL;
+ SCSIBus *bus = SCSI_BUS(qbus);
if (dev->channel > bus->info->max_channel) {
error_setg(errp, "bad scsi channel id: %d", dev->channel);
- return;
+ return false;
}
if (dev->id != -1 && dev->id > bus->info->max_target) {
error_setg(errp, "bad scsi device id: %d", dev->id);
- return;
+ return false;
}
if (dev->lun != -1 && dev->lun > bus->info->max_lun) {
error_setg(errp, "bad scsi device lun: %d", dev->lun);
- return;
+ return false;
+ }
+
+ if (dev->id != -1 && dev->lun != -1) {
+ SCSIDevice *d;
+ if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {
+ error_setg(errp, "lun already used by '%s'", d->qdev.id);
+ return false;
+ }
}
+ return true;
+}
+
+static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
+{
+ SCSIDevice *dev = SCSI_DEVICE(qdev);
+ SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
+ bool is_free;
+ Error *local_err = NULL;
+
if (dev->id == -1) {
int id = -1;
if (dev->lun == -1) {
dev->lun = 0;
}
do {
- d = scsi_device_find(bus, dev->channel, ++id, dev->lun);
- } while (d && d->lun == dev->lun && id < bus->info->max_target);
- if (d && d->lun == dev->lun) {
+ is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);
+ } while (!is_free && id < bus->info->max_target);
+ if (!is_free) {
error_setg(errp, "no free target");
return;
}
@@ -196,20 +202,13 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
} else if (dev->lun == -1) {
int lun = -1;
do {
- d = scsi_device_find(bus, dev->channel, dev->id, ++lun);
- } while (d && d->lun == lun && lun < bus->info->max_lun);
- if (d && d->lun == lun) {
+ is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);
+ } while (!is_free && lun < bus->info->max_lun);
+ if (!is_free) {
error_setg(errp, "no free lun");
return;
}
dev->lun = lun;
- } else {
- d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
- assert(d);
- if (d->lun == dev->lun && dev != d) {
- error_setg(errp, "lun already used by '%s'", d->qdev.id);
- return;
- }
}
QTAILQ_INIT(&dev->requests);
@@ -1709,6 +1708,13 @@ const VMStateDescription vmstate_scsi_device = {
}
};
+static Property scsi_props[] = {
+ DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
+ DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
+ DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
static void scsi_device_class_init(ObjectClass *klass, void *data)
{
DeviceClass *k = DEVICE_CLASS(klass);
@@ -1739,6 +1745,28 @@ static const TypeInfo scsi_device_type_info = {
.instance_init = scsi_dev_instance_init,
};
+static void scsi_bus_class_init(ObjectClass *klass, void *data)
+{
+ BusClass *k = BUS_CLASS(klass);
+ HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
+
+ k->get_dev_path = scsibus_get_dev_path;
+ k->get_fw_dev_path = scsibus_get_fw_dev_path;
+ k->check_address = scsi_bus_check_address;
+ hc->unplug = qdev_simple_device_unplug_cb;
+}
+
+static const TypeInfo scsi_bus_info = {
+ .name = TYPE_SCSI_BUS,
+ .parent = TYPE_BUS,
+ .instance_size = sizeof(SCSIBus),
+ .class_init = scsi_bus_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_HOTPLUG_HANDLER },
+ { }
+ }
+};
+
static void scsi_register_types(void)
{
type_register_static(&scsi_bus_info);
--
2.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 03/10] scsi/scsi_bus: switch search direction in scsi_device_find
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
2020-09-25 17:25 ` [PATCH 01/10] qdev: add "check if address free" callback for buses Paolo Bonzini
2020-09-25 17:25 ` [PATCH 02/10] scsi: switch to bus->check_address Paolo Bonzini
@ 2020-09-25 17:25 ` Paolo Bonzini
2020-09-25 17:25 ` [PATCH 04/10] device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add Paolo Bonzini
` (11 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:25 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha, mlevitsk
From: Maxim Levitsky <mlevitsk@redhat.com>
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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-2-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@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 94921c04b1..69d7c3f90c 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -1571,7 +1571,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);
@@ -1579,7 +1579,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.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 04/10] device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (2 preceding siblings ...)
2020-09-25 17:25 ` [PATCH 03/10] scsi/scsi_bus: switch search direction in scsi_device_find Paolo Bonzini
@ 2020-09-25 17:25 ` Paolo Bonzini
2020-09-25 17:25 ` [PATCH 05/10] device-core: use RCU for list of children of a bus Paolo Bonzini
` (10 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:25 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefan Hajnoczi, stefanha, mlevitsk
From: Maxim Levitsky <mlevitsk@redhat.com>
Soon, a device removal might only happen on RCU callback execution,
but to avoid chanding HMP semantics, just drain all pending RCU
callbacks
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-4-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
qdev-monitor.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e9b7228480..c66ba13780 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -803,6 +803,18 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
return;
}
dev = qdev_device_add(opts, errp);
+
+ /*
+ * Drain all pending RCU callbacks. This is done because
+ * some bus related operations can delay a device removal
+ * (in this case this can happen if device is added and then
+ * removed due to a configuration error)
+ * to a RCU callback, but user might expect that this interface
+ * will finish its job completely once qmp command returns result
+ * to the user
+ */
+ drain_call_rcu();
+
if (!dev) {
qemu_opts_del(opts);
return;
@@ -894,6 +906,16 @@ void qmp_device_del(const char *id, Error **errp)
}
qdev_unplug(dev, errp);
+
+ /*
+ * Drain all pending RCU callbacks. This is done because
+ * some bus related operations can delay a device removal
+ * to a RCU callback, but user might expect that this interface
+ * will finish its job completely once qmp command returns result
+ * to the user
+ */
+
+ drain_call_rcu();
}
}
--
2.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 05/10] device-core: use RCU for list of children of a bus
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (3 preceding siblings ...)
2020-09-25 17:25 ` [PATCH 04/10] device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add Paolo Bonzini
@ 2020-09-25 17:25 ` Paolo Bonzini
2020-09-30 14:29 ` Maxim Levitsky
2020-09-25 17:26 ` [PATCH 06/10] device-core: use atomic_set on .realized property Paolo Bonzini
` (9 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:25 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha, mlevitsk
From: Maxim Levitsky <mlevitsk@redhat.com>
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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-5-mlevitsk@redhat.com>
[Use RCU_READ_LOCK_GUARD in more places. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/core/bus.c | 28 +++++++++++++++++-----------
hw/core/qdev.c | 37 +++++++++++++++++++++++--------------
hw/scsi/scsi-bus.c | 12 +++++++++---
hw/scsi/virtio-scsi.c | 6 +++++-
include/hw/qdev-core.h | 9 +++++++++
5 files changed, 63 insertions(+), 29 deletions(-)
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 6b987b6946..a0483859ae 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,8 +92,10 @@ static void bus_reset_child_foreach(Object *obj, ResettableChildCallback cb,
BusState *bus = BUS(obj);
BusChild *kid;
- QTAILQ_FOREACH(kid, &bus->children, sibling) {
- cb(OBJECT(kid->child), opaque, type);
+ WITH_RCU_READ_LOCK_GUARD() {
+ QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+ cb(OBJECT(kid->child), opaque, type);
+ }
}
}
@@ -194,9 +198,11 @@ 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;
- qdev_unrealize(dev);
+ WITH_RCU_READ_LOCK_GUARD() {
+ QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+ DeviceState *dev = kid->child;
+ qdev_unrealize(dev);
+ }
}
if (bc->unrealize) {
bc->unrealize(bus);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 74db78df36..59e5e710b7 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);
- 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);
@@ -672,17 +679,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 69d7c3f90c..4ab9811cd8 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -399,7 +399,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_GUARD();
+
+ QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
DeviceState *qdev = kid->child;
SCSIDevice *dev = SCSI_DEVICE(qdev);
@@ -420,7 +423,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);
@@ -429,6 +432,7 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
i += 8;
}
}
+
assert(i == n + 8);
r->len = len;
return true;
@@ -1571,7 +1575,8 @@ 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_GUARD();
+ QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) {
DeviceState *qdev = kid->child;
SCSIDevice *dev = SCSI_DEVICE(qdev);
@@ -1590,6 +1595,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
}
}
}
+
return target_dev;
}
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3a71ea7097..971afbb217 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 e62da68a26..8067497074 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"
@@ -228,6 +230,7 @@ struct BusClass {
};
typedef struct BusChild {
+ struct rcu_head rcu;
DeviceState *child;
int index;
QTAILQ_ENTRY(BusChild) sibling;
@@ -248,6 +251,12 @@ struct BusState {
int max_index;
bool realized;
int num_children;
+
+ /*
+ * children is a RCU QTAILQ, thus readers must use RCU to access it,
+ * and writers must hold the big qemu lock
+ */
+
QTAILQ_HEAD(, BusChild) children;
QLIST_ENTRY(BusState) sibling;
ResettableState reset;
--
2.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 06/10] device-core: use atomic_set on .realized property
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (4 preceding siblings ...)
2020-09-25 17:25 ` [PATCH 05/10] device-core: use RCU for list of children of a bus Paolo Bonzini
@ 2020-09-25 17:26 ` Paolo Bonzini
2020-09-30 14:31 ` Maxim Levitsky
2020-09-25 17:26 ` [PATCH 07/10] scsi/scsi-bus: scsi_device_find: don't return unrealized devices Paolo Bonzini
` (8 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:26 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha, mlevitsk
From: Maxim Levitsky <mlevitsk@redhat.com>
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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-6-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/core/qdev.c | 19 ++++++++++++++++++-
include/hw/qdev-core.h | 3 +++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 59e5e710b7..fc4daa36fa 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -946,7 +946,25 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
}
}
+ qatomic_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
+ */
+
+ qatomic_set(&dev->realized, value);
+ /*
+ * Ensure that concurrent users see this update prior to
+ * any other changes done by unrealize.
+ */
+ smp_wmb();
+
QLIST_FOREACH(bus, &dev->child_bus, sibling) {
qbus_unrealize(bus);
}
@@ -961,7 +979,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
}
assert(local_err == NULL);
- dev->realized = value;
return;
child_realize_fail:
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 8067497074..39490e76ee 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -163,6 +163,9 @@ struct NamedClockList {
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
+ * When accessed without the iothread mutex, consider using
+ * qatomic_load_acquire() before accessing any other field in
+ * the device.
* @reset: ResettableState for the device; handled by Resettable interface.
*
* This structure should not be accessed directly. We declare it here
--
2.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 07/10] scsi/scsi-bus: scsi_device_find: don't return unrealized devices
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (5 preceding siblings ...)
2020-09-25 17:26 ` [PATCH 06/10] device-core: use atomic_set on .realized property Paolo Bonzini
@ 2020-09-25 17:26 ` Paolo Bonzini
2020-09-30 14:32 ` Maxim Levitsky
2020-09-25 17:26 ` [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get Paolo Bonzini
` (7 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:26 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha, mlevitsk
From: Maxim Levitsky <mlevitsk@redhat.com>
The device core first places a device on the bus and then realizes it.
Make scsi_device_find avoid returing such devices to avoid
races in drivers that use an iothread (currently virtio-scsi)
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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-7-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/scsi-bus.c | 83 +++++++++++++++++++++++++++++-----------------
1 file changed, 53 insertions(+), 30 deletions(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 4ab9811cd8..7599113efe 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -24,6 +24,55 @@ static void scsi_target_free_buf(SCSIRequest *req);
static int next_scsi_bus;
+static SCSIDevice *do_scsi_device_find(SCSIBus *bus,
+ int channel, int id, int lun,
+ bool include_unrealized)
+{
+ BusChild *kid;
+ SCSIDevice *retval = NULL;
+
+ 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) {
+ retval = dev;
+ break;
+ }
+
+ /*
+ * If we don't find exact match (channel/bus/lun),
+ * we will return the first device which matches channel/bus
+ */
+
+ if (!retval) {
+ retval = dev;
+ }
+ }
+ }
+
+ /*
+ * 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 (retval && !include_unrealized &&
+ !qatomic_load_acquire(&retval->qdev.realized)) {
+ retval = NULL;
+ }
+
+ return retval;
+}
+
+SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
+{
+ RCU_READ_LOCK_GUARD();
+ return do_scsi_device_find(bus, channel, id, lun, false);
+}
+
static void scsi_device_realize(SCSIDevice *s, Error **errp)
{
SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
@@ -137,7 +186,10 @@ static bool scsi_bus_is_address_free(SCSIBus *bus,
int channel, int target, int lun,
SCSIDevice **p_dev)
{
- SCSIDevice *d = scsi_device_find(bus, channel, target, lun);
+ SCSIDevice *d;
+
+ RCU_READ_LOCK_GUARD();
+ d = do_scsi_device_find(bus, channel, target, lun, true);
if (d && d->lun == lun) {
if (p_dev) {
*p_dev = d;
@@ -1570,35 +1622,6 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
qdev_fw_name(dev), d->id, d->lun);
}
-SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
-{
- BusChild *kid;
- SCSIDevice *target_dev = NULL;
-
- RCU_READ_LOCK_GUARD();
- 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) {
- 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;
- }
- }
- }
-
- return target_dev;
-}
-
/* SCSI request list. For simplicity, pv points to the whole device */
static int put_scsi_requests(QEMUFile *f, void *pv, size_t size,
--
2.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (6 preceding siblings ...)
2020-09-25 17:26 ` [PATCH 07/10] scsi/scsi-bus: scsi_device_find: don't return unrealized devices Paolo Bonzini
@ 2020-09-25 17:26 ` Paolo Bonzini
2020-09-30 14:32 ` Maxim Levitsky
2020-09-25 17:26 ` [PATCH 09/10] virtio-scsi: use scsi_device_get Paolo Bonzini
` (6 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefan Hajnoczi, stefanha, mlevitsk
From: Maxim Levitsky <mlevitsk@redhat.com>
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>
Message-Id: <20200913160259.32145-8-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
Compared to Maxim's patch, I am avoiding the extra argument
to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD()
out of do_scsi_device_find itself.
hw/scsi/scsi-bus.c | 11 +++++++++++
include/hw/scsi/scsi.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 7599113efe..eda8cb7e70 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -73,6 +73,17 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
return do_scsi_device_find(bus, channel, id, lun, false);
}
+SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun)
+{
+ SCSIDevice *d;
+ RCU_READ_LOCK_GUARD();
+ d = do_scsi_device_find(bus, channel, id, lun, false);
+ if (d) {
+ object_ref(d);
+ }
+ return d;
+}
+
static void scsi_device_realize(SCSIDevice *s, Error **errp)
{
SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 7a55cdbd74..09fa5c9d2a 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -190,6 +190,7 @@ 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.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 09/10] virtio-scsi: use scsi_device_get
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (7 preceding siblings ...)
2020-09-25 17:26 ` [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get Paolo Bonzini
@ 2020-09-25 17:26 ` Paolo Bonzini
2020-09-25 17:26 ` [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS Paolo Bonzini
` (5 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:26 UTC (permalink / raw)
To: qemu-devel; +Cc: Stefan Hajnoczi, stefanha, mlevitsk
From: Maxim Levitsky <mlevitsk@redhat.com>
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>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-9-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/virtio-scsi.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 971afbb217..3db9a8aae9 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)
{
if (lun[0] != 1) {
return NULL;
@@ -41,7 +41,7 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
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));
+ return scsi_device_get(&s->bus, 0, lun[1], virtio_scsi_get_lun(lun));
}
void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
@@ -256,7 +256,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;
@@ -370,10 +370,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();
@@ -386,14 +386,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;
}
@@ -564,7 +567,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);
@@ -580,10 +583,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.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (8 preceding siblings ...)
2020-09-25 17:26 ` [PATCH 09/10] virtio-scsi: use scsi_device_get Paolo Bonzini
@ 2020-09-25 17:26 ` Paolo Bonzini
2020-09-30 14:34 ` Maxim Levitsky
2020-09-25 19:26 ` [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread no-reply
` (4 subsequent siblings)
14 siblings, 1 reply; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-25 17:26 UTC (permalink / raw)
To: qemu-devel; +Cc: stefanha, mlevitsk
From: Maxim Levitsky <mlevitsk@redhat.com>
Currently scsi_target_emulate_report_luns iterates over the child device list
twice, and there is no guarantee that this list is the same in both iterations.
The reason for iterating twice is that the first iteration calculates
how much memory to allocate. However if we use a dynamic array we can
avoid iterating twice, and therefore we avoid this race.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1866707
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20200913160259.32145-10-mlevitsk@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/scsi/scsi-bus.c | 68 ++++++++++++++++++++++------------------------
1 file changed, 33 insertions(+), 35 deletions(-)
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index eda8cb7e70..b901e701f0 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -438,19 +438,23 @@ struct SCSITargetReq {
static void store_lun(uint8_t *outbuf, int lun)
{
if (lun < 256) {
+ /* Simple logical unit addressing method*/
+ outbuf[0] = 0;
outbuf[1] = lun;
- return;
+ } else {
+ /* Flat space addressing method */
+ outbuf[0] = 0x40 | (lun >> 8);
+ outbuf[1] = (lun & 255);
}
- outbuf[1] = (lun & 255);
- outbuf[0] = (lun >> 8) | 0x40;
}
static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
{
BusChild *kid;
- int i, len, n;
int channel, id;
- bool found_lun0;
+ uint8_t tmp[8] = {0};
+ int len = 0;
+ GByteArray *buf;
if (r->req.cmd.xfer < 16) {
return false;
@@ -458,46 +462,40 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
if (r->req.cmd.buf[2] > 2) {
return false;
}
+
+ /* reserve space for 63 LUNs*/
+ buf = g_byte_array_sized_new(512);
+
channel = r->req.dev->channel;
id = r->req.dev->id;
- found_lun0 = false;
- n = 0;
- RCU_READ_LOCK_GUARD();
+ /* add size (will be updated later to correct value */
+ g_byte_array_append(buf, tmp, 8);
+ len += 8;
- QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
- DeviceState *qdev = kid->child;
- SCSIDevice *dev = SCSI_DEVICE(qdev);
+ /* add LUN0 */
+ g_byte_array_append(buf, tmp, 8);
+ len += 8;
- if (dev->channel == channel && dev->id == id) {
- if (dev->lun == 0) {
- found_lun0 = true;
+ WITH_RCU_READ_LOCK_GUARD() {
+ QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
+ DeviceState *qdev = kid->child;
+ SCSIDevice *dev = SCSI_DEVICE(qdev);
+
+ if (dev->channel == channel && dev->id == id && dev->lun != 0) {
+ store_lun(tmp, dev->lun);
+ g_byte_array_append(buf, tmp, 8);
+ len += 8;
}
- n += 8;
}
}
- if (!found_lun0) {
- n += 8;
- }
-
- scsi_target_alloc_buf(&r->req, n + 8);
-
- len = MIN(n + 8, r->req.cmd.xfer & ~7);
- memset(r->buf, 0, len);
- stl_be_p(&r->buf[0], n);
- i = found_lun0 ? 8 : 16;
- QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
- DeviceState *qdev = kid->child;
- SCSIDevice *dev = SCSI_DEVICE(qdev);
- if (dev->channel == channel && dev->id == id) {
- store_lun(&r->buf[i], dev->lun);
- i += 8;
- }
- }
+ r->buf_len = len;
+ r->buf = g_byte_array_free(buf, FALSE);
+ r->len = MIN(len, r->req.cmd.xfer & ~7);
- assert(i == n + 8);
- r->len = len;
+ /* store the LUN list length */
+ stl_be_p(&r->buf[0], len - 8);
return true;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (9 preceding siblings ...)
2020-09-25 17:26 ` [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS Paolo Bonzini
@ 2020-09-25 19:26 ` no-reply
2020-09-25 22:52 ` no-reply
` (3 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-09-25 19:26 UTC (permalink / raw)
To: pbonzini; +Cc: qemu-devel, stefanha, mlevitsk
Patchew URL: https://patchew.org/QEMU/20200925172604.2142227-1-pbonzini@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20200925172604.2142227-1-pbonzini@redhat.com
Subject: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
=== 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 ===
From https://github.com/patchew-project/qemu
* [new tag] patchew/20200925172604.2142227-1-pbonzini@redhat.com -> patchew/20200925172604.2142227-1-pbonzini@redhat.com
Switched to a new branch 'test'
21c2380 scsi/scsi_bus: fix races in REPORT LUNS
aacd230 virtio-scsi: use scsi_device_get
f9f8f08 scsi/scsi_bus: Add scsi_device_get
c66050f scsi/scsi-bus: scsi_device_find: don't return unrealized devices
bb9fbd3 device-core: use atomic_set on .realized property
bd87593 device-core: use RCU for list of children of a bus
51a8ebe device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add
a595aa2 scsi/scsi_bus: switch search direction in scsi_device_find
6739a20 scsi: switch to bus->check_address
c4ed079 qdev: add "check if address free" callback for buses
=== OUTPUT BEGIN ===
1/10 Checking commit c4ed0795a36d (qdev: add "check if address free" callback for buses)
2/10 Checking commit 6739a205e683 (scsi: switch to bus->check_address)
ERROR: code indent should never use tabs
#53: FILE: hw/scsi/scsi-bus.c:137:
+^I^I^I^I int channel, int target, int lun,$
ERROR: code indent should never use tabs
#54: FILE: hw/scsi/scsi-bus.c:138:
+^I^I^I^I SCSIDevice **p_dev)$
WARNING: line over 80 characters
#69: FILE: hw/scsi/scsi-bus.c:153:
+static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)
WARNING: line over 80 characters
#89: FILE: hw/scsi/scsi-bus.c:173:
+ if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {
WARNING: line over 80 characters
#128: FILE: hw/scsi/scsi-bus.c:195:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);
WARNING: line over 80 characters
#141: FILE: hw/scsi/scsi-bus.c:205:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);
total: 2 errors, 4 warnings, 182 lines checked
Patch 2/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/10 Checking commit a595aa288f19 (scsi/scsi_bus: switch search direction in scsi_device_find)
4/10 Checking commit 51a8ebe64c5f (device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add)
5/10 Checking commit bd87593ffb6e (device-core: use RCU for list of children of a bus)
6/10 Checking commit bb9fbd3d7a25 (device-core: use atomic_set on .realized property)
7/10 Checking commit c66050f657ae (scsi/scsi-bus: scsi_device_find: don't return unrealized devices)
8/10 Checking commit f9f8f0888e58 (scsi/scsi_bus: Add scsi_device_get)
9/10 Checking commit aacd2301cef8 (virtio-scsi: use scsi_device_get)
10/10 Checking commit 21c238042be7 (scsi/scsi_bus: fix races in REPORT LUNS)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200925172604.2142227-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (10 preceding siblings ...)
2020-09-25 19:26 ` [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread no-reply
@ 2020-09-25 22:52 ` no-reply
2020-09-26 0:28 ` no-reply
` (2 subsequent siblings)
14 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-09-25 22:52 UTC (permalink / raw)
To: pbonzini; +Cc: qemu-devel, stefanha, mlevitsk
Patchew URL: https://patchew.org/QEMU/20200925172604.2142227-1-pbonzini@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20200925172604.2142227-1-pbonzini@redhat.com
Subject: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
=== 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 ===
From https://github.com/patchew-project/qemu
- [tag update] patchew/20200925172604.2142227-1-pbonzini@redhat.com -> patchew/20200925172604.2142227-1-pbonzini@redhat.com
Switched to a new branch 'test'
fbea547 scsi/scsi_bus: fix races in REPORT LUNS
8732528 virtio-scsi: use scsi_device_get
4cb91a6 scsi/scsi_bus: Add scsi_device_get
c91de0a scsi/scsi-bus: scsi_device_find: don't return unrealized devices
f9b2cb5 device-core: use atomic_set on .realized property
7c7d163 device-core: use RCU for list of children of a bus
568c8ae device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add
f594f95 scsi/scsi_bus: switch search direction in scsi_device_find
42b132a scsi: switch to bus->check_address
f0891a9 qdev: add "check if address free" callback for buses
=== OUTPUT BEGIN ===
1/10 Checking commit f0891a96af1b (qdev: add "check if address free" callback for buses)
2/10 Checking commit 42b132a31e18 (scsi: switch to bus->check_address)
ERROR: code indent should never use tabs
#53: FILE: hw/scsi/scsi-bus.c:137:
+^I^I^I^I int channel, int target, int lun,$
ERROR: code indent should never use tabs
#54: FILE: hw/scsi/scsi-bus.c:138:
+^I^I^I^I SCSIDevice **p_dev)$
WARNING: line over 80 characters
#69: FILE: hw/scsi/scsi-bus.c:153:
+static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)
WARNING: line over 80 characters
#89: FILE: hw/scsi/scsi-bus.c:173:
+ if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {
WARNING: line over 80 characters
#128: FILE: hw/scsi/scsi-bus.c:195:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);
WARNING: line over 80 characters
#141: FILE: hw/scsi/scsi-bus.c:205:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);
total: 2 errors, 4 warnings, 182 lines checked
Patch 2/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/10 Checking commit f594f95b4779 (scsi/scsi_bus: switch search direction in scsi_device_find)
4/10 Checking commit 568c8aea7113 (device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add)
5/10 Checking commit 7c7d163add9c (device-core: use RCU for list of children of a bus)
6/10 Checking commit f9b2cb585972 (device-core: use atomic_set on .realized property)
7/10 Checking commit c91de0a97535 (scsi/scsi-bus: scsi_device_find: don't return unrealized devices)
8/10 Checking commit 4cb91a6d49f4 (scsi/scsi_bus: Add scsi_device_get)
9/10 Checking commit 87325282af2e (virtio-scsi: use scsi_device_get)
10/10 Checking commit fbea547fe6c6 (scsi/scsi_bus: fix races in REPORT LUNS)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200925172604.2142227-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (11 preceding siblings ...)
2020-09-25 22:52 ` no-reply
@ 2020-09-26 0:28 ` no-reply
2020-09-26 0:44 ` no-reply
2020-09-26 1:05 ` no-reply
14 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-09-26 0:28 UTC (permalink / raw)
To: pbonzini; +Cc: qemu-devel, stefanha, mlevitsk
Patchew URL: https://patchew.org/QEMU/20200925172604.2142227-1-pbonzini@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20200925172604.2142227-1-pbonzini@redhat.com
Subject: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
=== 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'
0791629 scsi/scsi_bus: fix races in REPORT LUNS
87daae1 virtio-scsi: use scsi_device_get
bca3b34 scsi/scsi_bus: Add scsi_device_get
f87edcf scsi/scsi-bus: scsi_device_find: don't return unrealized devices
4bcc453 device-core: use atomic_set on .realized property
ebe1f7c device-core: use RCU for list of children of a bus
6297cbe device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add
318e1e4 scsi/scsi_bus: switch search direction in scsi_device_find
5ad95de scsi: switch to bus->check_address
998aee1 qdev: add "check if address free" callback for buses
=== OUTPUT BEGIN ===
1/10 Checking commit 998aee1a14ad (qdev: add "check if address free" callback for buses)
2/10 Checking commit 5ad95de05950 (scsi: switch to bus->check_address)
ERROR: code indent should never use tabs
#53: FILE: hw/scsi/scsi-bus.c:137:
+^I^I^I^I int channel, int target, int lun,$
ERROR: code indent should never use tabs
#54: FILE: hw/scsi/scsi-bus.c:138:
+^I^I^I^I SCSIDevice **p_dev)$
WARNING: line over 80 characters
#69: FILE: hw/scsi/scsi-bus.c:153:
+static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)
WARNING: line over 80 characters
#89: FILE: hw/scsi/scsi-bus.c:173:
+ if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {
WARNING: line over 80 characters
#128: FILE: hw/scsi/scsi-bus.c:195:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);
WARNING: line over 80 characters
#141: FILE: hw/scsi/scsi-bus.c:205:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);
total: 2 errors, 4 warnings, 182 lines checked
Patch 2/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/10 Checking commit 318e1e496e9e (scsi/scsi_bus: switch search direction in scsi_device_find)
4/10 Checking commit 6297cbe94121 (device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add)
5/10 Checking commit ebe1f7cd3ce0 (device-core: use RCU for list of children of a bus)
6/10 Checking commit 4bcc453b4b1c (device-core: use atomic_set on .realized property)
7/10 Checking commit f87edcfc20e5 (scsi/scsi-bus: scsi_device_find: don't return unrealized devices)
8/10 Checking commit bca3b34e3e66 (scsi/scsi_bus: Add scsi_device_get)
9/10 Checking commit 87daae1b9d04 (virtio-scsi: use scsi_device_get)
10/10 Checking commit 079162965856 (scsi/scsi_bus: fix races in REPORT LUNS)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200925172604.2142227-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (12 preceding siblings ...)
2020-09-26 0:28 ` no-reply
@ 2020-09-26 0:44 ` no-reply
2020-09-26 1:05 ` no-reply
14 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-09-26 0:44 UTC (permalink / raw)
To: pbonzini; +Cc: qemu-devel, stefanha, mlevitsk
Patchew URL: https://patchew.org/QEMU/20200925172604.2142227-1-pbonzini@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20200925172604.2142227-1-pbonzini@redhat.com
Subject: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
=== 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 ===
From https://github.com/patchew-project/qemu
- [tag update] patchew/20200925172604.2142227-1-pbonzini@redhat.com -> patchew/20200925172604.2142227-1-pbonzini@redhat.com
Switched to a new branch 'test'
9932c24 scsi/scsi_bus: fix races in REPORT LUNS
1b75dae virtio-scsi: use scsi_device_get
5cdf821 scsi/scsi_bus: Add scsi_device_get
47cdfc0 scsi/scsi-bus: scsi_device_find: don't return unrealized devices
75ce260 device-core: use atomic_set on .realized property
a417bc2 device-core: use RCU for list of children of a bus
8e81334 device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add
ec790ac scsi/scsi_bus: switch search direction in scsi_device_find
b9c2f95 scsi: switch to bus->check_address
0519c6e qdev: add "check if address free" callback for buses
=== OUTPUT BEGIN ===
1/10 Checking commit 0519c6ec5850 (qdev: add "check if address free" callback for buses)
2/10 Checking commit b9c2f95892cb (scsi: switch to bus->check_address)
ERROR: code indent should never use tabs
#53: FILE: hw/scsi/scsi-bus.c:137:
+^I^I^I^I int channel, int target, int lun,$
ERROR: code indent should never use tabs
#54: FILE: hw/scsi/scsi-bus.c:138:
+^I^I^I^I SCSIDevice **p_dev)$
WARNING: line over 80 characters
#69: FILE: hw/scsi/scsi-bus.c:153:
+static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)
WARNING: line over 80 characters
#89: FILE: hw/scsi/scsi-bus.c:173:
+ if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {
WARNING: line over 80 characters
#128: FILE: hw/scsi/scsi-bus.c:195:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);
WARNING: line over 80 characters
#141: FILE: hw/scsi/scsi-bus.c:205:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);
total: 2 errors, 4 warnings, 182 lines checked
Patch 2/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/10 Checking commit ec790aca5ae9 (scsi/scsi_bus: switch search direction in scsi_device_find)
4/10 Checking commit 8e81334e7b46 (device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add)
5/10 Checking commit a417bc2b8f36 (device-core: use RCU for list of children of a bus)
6/10 Checking commit 75ce26036080 (device-core: use atomic_set on .realized property)
7/10 Checking commit 47cdfc0d2962 (scsi/scsi-bus: scsi_device_find: don't return unrealized devices)
8/10 Checking commit 5cdf82120022 (scsi/scsi_bus: Add scsi_device_get)
9/10 Checking commit 1b75daed11e0 (virtio-scsi: use scsi_device_get)
10/10 Checking commit 9932c24a76f7 (scsi/scsi_bus: fix races in REPORT LUNS)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200925172604.2142227-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
` (13 preceding siblings ...)
2020-09-26 0:44 ` no-reply
@ 2020-09-26 1:05 ` no-reply
14 siblings, 0 replies; 31+ messages in thread
From: no-reply @ 2020-09-26 1:05 UTC (permalink / raw)
To: pbonzini; +Cc: qemu-devel, stefanha, mlevitsk
Patchew URL: https://patchew.org/QEMU/20200925172604.2142227-1-pbonzini@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20200925172604.2142227-1-pbonzini@redhat.com
Subject: [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread
=== 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'
c3f0c8f scsi/scsi_bus: fix races in REPORT LUNS
9de4834 virtio-scsi: use scsi_device_get
7da82c3 scsi/scsi_bus: Add scsi_device_get
eb46533 scsi/scsi-bus: scsi_device_find: don't return unrealized devices
8c11273 device-core: use atomic_set on .realized property
939dcba device-core: use RCU for list of children of a bus
0002336 device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add
efc35ef scsi/scsi_bus: switch search direction in scsi_device_find
0deb2b0 scsi: switch to bus->check_address
f57e102 qdev: add "check if address free" callback for buses
=== OUTPUT BEGIN ===
1/10 Checking commit f57e10207f21 (qdev: add "check if address free" callback for buses)
2/10 Checking commit 0deb2b0d8478 (scsi: switch to bus->check_address)
ERROR: code indent should never use tabs
#53: FILE: hw/scsi/scsi-bus.c:137:
+^I^I^I^I int channel, int target, int lun,$
ERROR: code indent should never use tabs
#54: FILE: hw/scsi/scsi-bus.c:138:
+^I^I^I^I SCSIDevice **p_dev)$
WARNING: line over 80 characters
#69: FILE: hw/scsi/scsi-bus.c:153:
+static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)
WARNING: line over 80 characters
#89: FILE: hw/scsi/scsi-bus.c:173:
+ if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {
WARNING: line over 80 characters
#128: FILE: hw/scsi/scsi-bus.c:195:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);
WARNING: line over 80 characters
#141: FILE: hw/scsi/scsi-bus.c:205:
+ is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);
total: 2 errors, 4 warnings, 182 lines checked
Patch 2/10 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/10 Checking commit efc35ef5a2ad (scsi/scsi_bus: switch search direction in scsi_device_find)
4/10 Checking commit 000233693d95 (device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add)
5/10 Checking commit 939dcbab7a50 (device-core: use RCU for list of children of a bus)
6/10 Checking commit 8c112731188a (device-core: use atomic_set on .realized property)
7/10 Checking commit eb46533b2b49 (scsi/scsi-bus: scsi_device_find: don't return unrealized devices)
8/10 Checking commit 7da82c3d163f (scsi/scsi_bus: Add scsi_device_get)
9/10 Checking commit 9de4834b2871 (virtio-scsi: use scsi_device_get)
10/10 Checking commit c3f0c8f18f10 (scsi/scsi_bus: fix races in REPORT LUNS)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200925172604.2142227-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/10] qdev: add "check if address free" callback for buses
2020-09-25 17:25 ` [PATCH 01/10] qdev: add "check if address free" callback for buses Paolo Bonzini
@ 2020-09-28 9:30 ` Stefan Hajnoczi
2020-09-30 17:48 ` Paolo Bonzini
2020-09-30 14:27 ` Maxim Levitsky
1 sibling, 1 reply; 31+ messages in thread
From: Stefan Hajnoczi @ 2020-09-28 9:30 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, mlevitsk
[-- Attachment #1: Type: text/plain, Size: 474 bytes --]
On Fri, Sep 25, 2020 at 01:25:55PM -0400, Paolo Bonzini wrote:
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 72064f4dd4..e62da68a26 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -217,6 +217,7 @@ struct BusClass {
> */
> char *(*get_fw_dev_path)(DeviceState *dev);
> void (*reset)(BusState *bus);
> + bool (*check_address)(BusState *bus, DeviceState *dev, Error **errp);
Please document this function.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/10] scsi: switch to bus->check_address
2020-09-25 17:25 ` [PATCH 02/10] scsi: switch to bus->check_address Paolo Bonzini
@ 2020-09-28 9:43 ` Stefan Hajnoczi
2020-09-30 14:28 ` Maxim Levitsky
1 sibling, 0 replies; 31+ messages in thread
From: Stefan Hajnoczi @ 2020-09-28 9:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, mlevitsk
[-- Attachment #1: Type: text/plain, Size: 301 bytes --]
On Fri, Sep 25, 2020 at 01:25:56PM -0400, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/scsi/scsi-bus.c | 122 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 75 insertions(+), 47 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/10] qdev: add "check if address free" callback for buses
2020-09-25 17:25 ` [PATCH 01/10] qdev: add "check if address free" callback for buses Paolo Bonzini
2020-09-28 9:30 ` Stefan Hajnoczi
@ 2020-09-30 14:27 ` Maxim Levitsky
2020-09-30 23:37 ` Paolo Bonzini
1 sibling, 1 reply; 31+ messages in thread
From: Maxim Levitsky @ 2020-09-30 14:27 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha
On Fri, 2020-09-25 at 13:25 -0400, Paolo Bonzini wrote:
> Check if an address is free on the bus before plugging in the
> device. This makes it possible to do the check without any
> side effects, and to detect the problem early without having
> to do it in the realize callback.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/core/qdev.c | 17 +++++++++++++++--
> hw/net/virtio-net.c | 2 +-
> hw/sd/core.c | 3 ++-
> include/hw/qdev-core.h | 3 ++-
> 4 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 96772a15bd..74db78df36 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -94,13 +94,23 @@ static void bus_add_child(BusState *bus, DeviceState *child)
> 0);
> }
>
> -void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
> +static bool bus_check_address(BusState *bus, DeviceState *child, Error **errp)
> +{
> + BusClass *bc = BUS_GET_CLASS(bus);
> + return !bc->check_address || bc->check_address(bus, child, errp);
> +}
> +
> +bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
> {
> BusState *old_parent_bus = dev->parent_bus;
> DeviceClass *dc = DEVICE_GET_CLASS(dev);
>
> assert(dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type));
>
> + if (!bus_check_address(bus, dev, errp)) {
> + return false;
> + }
> +
> if (old_parent_bus) {
> trace_qdev_update_parent_bus(dev, object_get_typename(OBJECT(dev)),
> old_parent_bus, object_get_typename(OBJECT(old_parent_bus)),
> @@ -126,6 +136,7 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
> object_unref(OBJECT(old_parent_bus));
> object_unref(OBJECT(dev));
> }
> + return true;
> }
>
> DeviceState *qdev_new(const char *name)
> @@ -371,7 +382,9 @@ bool qdev_realize(DeviceState *dev, BusState *bus, Error **errp)
> assert(!dev->realized && !dev->parent_bus);
>
> if (bus) {
> - qdev_set_parent_bus(dev, bus);
> + if (!qdev_set_parent_bus(dev, bus, errp)) {
> + return false;
> + }
> } else {
> assert(!DEVICE_GET_CLASS(dev)->bus_type);
> }
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7bf27b9db7..268cecc498 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3142,7 +3142,7 @@ static bool failover_replug_primary(VirtIONet *n, Error **errp)
> error_setg(errp, "virtio_net: couldn't find primary bus");
> return false;
> }
> - qdev_set_parent_bus(n->primary_dev, n->primary_bus);
> + qdev_set_parent_bus(n->primary_dev, n->primary_bus, &error_abort);
> n->primary_should_be_hidden = false;
> if (!qemu_opt_set_bool(n->primary_device_opts,
> "partially_hotplugged", true, errp)) {
> diff --git a/hw/sd/core.c b/hw/sd/core.c
> index 957d116f1a..08c93b5903 100644
> --- a/hw/sd/core.c
> +++ b/hw/sd/core.c
> @@ -23,6 +23,7 @@
> #include "hw/qdev-core.h"
> #include "hw/sd/sd.h"
> #include "qemu/module.h"
> +#include "qapi/error.h"
> #include "trace.h"
>
> static inline const char *sdbus_name(SDBus *sdbus)
> @@ -240,7 +241,7 @@ void sdbus_reparent_card(SDBus *from, SDBus *to)
> readonly = sc->get_readonly(card);
>
> sdbus_set_inserted(from, false);
> - qdev_set_parent_bus(DEVICE(card), &to->qbus);
> + qdev_set_parent_bus(DEVICE(card), &to->qbus, &error_abort);
> sdbus_set_inserted(to, true);
> sdbus_set_readonly(to, readonly);
> }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 72064f4dd4..e62da68a26 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -217,6 +217,7 @@ struct BusClass {
> */
> char *(*get_fw_dev_path)(DeviceState *dev);
> void (*reset)(BusState *bus);
> + bool (*check_address)(BusState *bus, DeviceState *dev, Error **errp);
> BusRealize realize;
> BusUnrealize unrealize;
>
> @@ -788,7 +789,7 @@ const char *qdev_fw_name(DeviceState *dev);
> Object *qdev_get_machine(void);
>
> /* FIXME: make this a link<> */
> -void qdev_set_parent_bus(DeviceState *dev, BusState *bus);
> +bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
>
> extern bool qdev_hotplug;
> extern bool qdev_hot_removed;
I like that idea, however I wonder why this was needed.
My patch that switches the direction in scsi_device_find, is supposed to be completely equavalent,
based on the following train of thought:
If scsi_device_find finds an exact match it returns only it, as before.
Otherwise scsi_device_find were to scan from end of the list to the start, and every time,
it finds a device with same channel/id it would update the target_dev
and return it when it reaches the end of the list.
If I am not mistaken this means that it would return _first_ device in the
list that matches the channel/id.
This is exactly what new version of scsi_device_find does.
Anyway, since I don't see anything wrong with doing what this patch does other
than adding a documentation to the function as Stefan pointed out,
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/10] scsi: switch to bus->check_address
2020-09-25 17:25 ` [PATCH 02/10] scsi: switch to bus->check_address Paolo Bonzini
2020-09-28 9:43 ` Stefan Hajnoczi
@ 2020-09-30 14:28 ` Maxim Levitsky
1 sibling, 0 replies; 31+ messages in thread
From: Maxim Levitsky @ 2020-09-30 14:28 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha
On Fri, 2020-09-25 at 13:25 -0400, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/scsi/scsi-bus.c | 122 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 75 insertions(+), 47 deletions(-)
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 3284a5d1fb..94921c04b1 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -22,33 +22,6 @@ static void scsi_req_dequeue(SCSIRequest *req);
> static uint8_t *scsi_target_alloc_buf(SCSIRequest *req, size_t len);
> static void scsi_target_free_buf(SCSIRequest *req);
>
> -static Property scsi_props[] = {
> - DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
> - DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
> - DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
> - DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void scsi_bus_class_init(ObjectClass *klass, void *data)
> -{
> - BusClass *k = BUS_CLASS(klass);
> - HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> -
> - k->get_dev_path = scsibus_get_dev_path;
> - k->get_fw_dev_path = scsibus_get_fw_dev_path;
> - hc->unplug = qdev_simple_device_unplug_cb;
> -}
> -
> -static const TypeInfo scsi_bus_info = {
> - .name = TYPE_SCSI_BUS,
> - .parent = TYPE_BUS,
> - .instance_size = sizeof(SCSIBus),
> - .class_init = scsi_bus_class_init,
> - .interfaces = (InterfaceInfo[]) {
> - { TYPE_HOTPLUG_HANDLER },
> - { }
> - }
> -};
Very very minor nitpick: I'll would do the move of these,
in a separate patch.
> static int next_scsi_bus;
>
> static void scsi_device_realize(SCSIDevice *s, Error **errp)
> @@ -160,35 +133,68 @@ static void scsi_dma_restart_cb(void *opaque, int running, RunState state)
> }
> }
>
> -static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
> +static bool scsi_bus_is_address_free(SCSIBus *bus,
> + int channel, int target, int lun,
> + SCSIDevice **p_dev)
Alignment wrong on that '(' - we really need to make checkpatch complain
about it.
> +{
> + SCSIDevice *d = scsi_device_find(bus, channel, target, lun);
> + if (d && d->lun == lun) {
> + if (p_dev) {
> + *p_dev = d;
> + }
> + return false;
> + }
> + if (p_dev) {
> + *p_dev = NULL;
> + }
> + return true;
> +}
> +
> +static bool scsi_bus_check_address(BusState *qbus, DeviceState *qdev, Error **errp)
> {
> SCSIDevice *dev = SCSI_DEVICE(qdev);
> - SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> - SCSIDevice *d;
> - Error *local_err = NULL;
> + SCSIBus *bus = SCSI_BUS(qbus);
>
> if (dev->channel > bus->info->max_channel) {
> error_setg(errp, "bad scsi channel id: %d", dev->channel);
> - return;
> + return false;
> }
> if (dev->id != -1 && dev->id > bus->info->max_target) {
> error_setg(errp, "bad scsi device id: %d", dev->id);
> - return;
> + return false;
> }
> if (dev->lun != -1 && dev->lun > bus->info->max_lun) {
> error_setg(errp, "bad scsi device lun: %d", dev->lun);
> - return;
> + return false;
> + }
> +
> + if (dev->id != -1 && dev->lun != -1) {
> + SCSIDevice *d;
> + if (!scsi_bus_is_address_free(bus, dev->channel, dev->id, dev->lun, &d)) {
> + error_setg(errp, "lun already used by '%s'", d->qdev.id);
> + return false;
> + }
> }
>
> + return true;
> +}
> +
> +static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
> +{
> + SCSIDevice *dev = SCSI_DEVICE(qdev);
> + SCSIBus *bus = DO_UPCAST(SCSIBus, qbus, dev->qdev.parent_bus);
> + bool is_free;
> + Error *local_err = NULL;
> +
> if (dev->id == -1) {
> int id = -1;
> if (dev->lun == -1) {
> dev->lun = 0;
> }
> do {
> - d = scsi_device_find(bus, dev->channel, ++id, dev->lun);
> - } while (d && d->lun == dev->lun && id < bus->info->max_target);
> - if (d && d->lun == dev->lun) {
> + is_free = scsi_bus_is_address_free(bus, dev->channel, ++id, dev->lun, NULL);
> + } while (!is_free && id < bus->info->max_target);
> + if (!is_free) {
> error_setg(errp, "no free target");
> return;
> }
> @@ -196,20 +202,13 @@ static void scsi_qdev_realize(DeviceState *qdev, Error **errp)
> } else if (dev->lun == -1) {
> int lun = -1;
> do {
> - d = scsi_device_find(bus, dev->channel, dev->id, ++lun);
> - } while (d && d->lun == lun && lun < bus->info->max_lun);
> - if (d && d->lun == lun) {
> + is_free = scsi_bus_is_address_free(bus, dev->channel, dev->id, ++lun, NULL);
> + } while (!is_free && lun < bus->info->max_lun);
> + if (!is_free) {
> error_setg(errp, "no free lun");
> return;
> }
> dev->lun = lun;
> - } else {
> - d = scsi_device_find(bus, dev->channel, dev->id, dev->lun);
> - assert(d);
> - if (d->lun == dev->lun && dev != d) {
> - error_setg(errp, "lun already used by '%s'", d->qdev.id);
> - return;
> - }
> }
>
> QTAILQ_INIT(&dev->requests);
> @@ -1709,6 +1708,13 @@ const VMStateDescription vmstate_scsi_device = {
> }
> };
>
> +static Property scsi_props[] = {
> + DEFINE_PROP_UINT32("channel", SCSIDevice, channel, 0),
> + DEFINE_PROP_UINT32("scsi-id", SCSIDevice, id, -1),
> + DEFINE_PROP_UINT32("lun", SCSIDevice, lun, -1),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void scsi_device_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *k = DEVICE_CLASS(klass);
> @@ -1739,6 +1745,28 @@ static const TypeInfo scsi_device_type_info = {
> .instance_init = scsi_dev_instance_init,
> };
>
> +static void scsi_bus_class_init(ObjectClass *klass, void *data)
> +{
> + BusClass *k = BUS_CLASS(klass);
> + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
> +
> + k->get_dev_path = scsibus_get_dev_path;
> + k->get_fw_dev_path = scsibus_get_fw_dev_path;
> + k->check_address = scsi_bus_check_address;
> + hc->unplug = qdev_simple_device_unplug_cb;
> +}
> +
> +static const TypeInfo scsi_bus_info = {
> + .name = TYPE_SCSI_BUS,
> + .parent = TYPE_BUS,
> + .instance_size = sizeof(SCSIBus),
> + .class_init = scsi_bus_class_init,
> + .interfaces = (InterfaceInfo[]) {
> + { TYPE_HOTPLUG_HANDLER },
> + { }
> + }
> +};
> +
> static void scsi_register_types(void)
> {
> type_register_static(&scsi_bus_info);
With very minor nitpicks, which don't really have to be fixed,
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 05/10] device-core: use RCU for list of children of a bus
2020-09-25 17:25 ` [PATCH 05/10] device-core: use RCU for list of children of a bus Paolo Bonzini
@ 2020-09-30 14:29 ` Maxim Levitsky
0 siblings, 0 replies; 31+ messages in thread
From: Maxim Levitsky @ 2020-09-30 14:29 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha
On Fri, 2020-09-25 at 13:25 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
>
> 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>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20200913160259.32145-5-mlevitsk@redhat.com>
> [Use RCU_READ_LOCK_GUARD in more places. - Paolo]
This is a good idea which I don't yet use much.
Best regards,
Maxim Levitsky
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/core/bus.c | 28 +++++++++++++++++-----------
> hw/core/qdev.c | 37 +++++++++++++++++++++++--------------
> hw/scsi/scsi-bus.c | 12 +++++++++---
> hw/scsi/virtio-scsi.c | 6 +++++-
> include/hw/qdev-core.h | 9 +++++++++
> 5 files changed, 63 insertions(+), 29 deletions(-)
>
> diff --git a/hw/core/bus.c b/hw/core/bus.c
> index 6b987b6946..a0483859ae 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,8 +92,10 @@ static void bus_reset_child_foreach(Object *obj, ResettableChildCallback cb,
> BusState *bus = BUS(obj);
> BusChild *kid;
>
> - QTAILQ_FOREACH(kid, &bus->children, sibling) {
> - cb(OBJECT(kid->child), opaque, type);
> + WITH_RCU_READ_LOCK_GUARD() {
> + QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
> + cb(OBJECT(kid->child), opaque, type);
> + }
> }
> }
>
> @@ -194,9 +198,11 @@ 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;
> - qdev_unrealize(dev);
> + WITH_RCU_READ_LOCK_GUARD() {
> + QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
> + DeviceState *dev = kid->child;
> + qdev_unrealize(dev);
> + }
> }
> if (bc->unrealize) {
> bc->unrealize(bus);
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 74db78df36..59e5e710b7 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);
> - 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);
> @@ -672,17 +679,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 69d7c3f90c..4ab9811cd8 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -399,7 +399,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_GUARD();
> +
> + QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> DeviceState *qdev = kid->child;
> SCSIDevice *dev = SCSI_DEVICE(qdev);
>
> @@ -420,7 +423,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);
>
> @@ -429,6 +432,7 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
> i += 8;
> }
> }
> +
> assert(i == n + 8);
> r->len = len;
> return true;
> @@ -1571,7 +1575,8 @@ 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_GUARD();
> + QTAILQ_FOREACH_RCU(kid, &bus->qbus.children, sibling) {
> DeviceState *qdev = kid->child;
> SCSIDevice *dev = SCSI_DEVICE(qdev);
>
> @@ -1590,6 +1595,7 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
> }
> }
> }
> +
> return target_dev;
> }
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 3a71ea7097..971afbb217 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 e62da68a26..8067497074 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"
> @@ -228,6 +230,7 @@ struct BusClass {
> };
>
> typedef struct BusChild {
> + struct rcu_head rcu;
> DeviceState *child;
> int index;
> QTAILQ_ENTRY(BusChild) sibling;
> @@ -248,6 +251,12 @@ struct BusState {
> int max_index;
> bool realized;
> int num_children;
> +
> + /*
> + * children is a RCU QTAILQ, thus readers must use RCU to access it,
> + * and writers must hold the big qemu lock
> + */
> +
> QTAILQ_HEAD(, BusChild) children;
> QLIST_ENTRY(BusState) sibling;
> ResettableState reset;
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/10] device-core: use atomic_set on .realized property
2020-09-25 17:26 ` [PATCH 06/10] device-core: use atomic_set on .realized property Paolo Bonzini
@ 2020-09-30 14:31 ` Maxim Levitsky
2020-09-30 17:44 ` Paolo Bonzini
0 siblings, 1 reply; 31+ messages in thread
From: Maxim Levitsky @ 2020-09-30 14:31 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha
On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
>
> 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>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20200913160259.32145-6-mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/core/qdev.c | 19 ++++++++++++++++++-
> include/hw/qdev-core.h | 3 +++
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 59e5e710b7..fc4daa36fa 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -946,7 +946,25 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> }
> }
>
> + qatomic_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
> + */
> +
> + qatomic_set(&dev->realized, value);
> + /*
> + * Ensure that concurrent users see this update prior to
> + * any other changes done by unrealize.
> + */
> + smp_wmb();
I''l probably never fully understand where to use read/write/full barrier.
If I understand corrctly, read barrier prevents reads done by this thread to be reordered,
by the CPU and write barrier prevents writes done by this CPU to be re-ordered.
Both (depending on the macro) usually imply compiler barrier (to avoid compilier re-ordering
stuff...)
> +
> QLIST_FOREACH(bus, &dev->child_bus, sibling) {
> qbus_unrealize(bus);
> }
> @@ -961,7 +979,6 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> }
>
> assert(local_err == NULL);
> - dev->realized = value;
> return;
>
> child_realize_fail:
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 8067497074..39490e76ee 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -163,6 +163,9 @@ struct NamedClockList {
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> + * When accessed without the iothread mutex, consider using
> + * qatomic_load_acquire() before accessing any other field in
> + * the device.
This sounds way better that what I wrote. As you probably noticed recently
(but not this patchset yet) I started to review the spelling/comments
more throughfully, so hopefully we see less of stuff that I myself read
and wonder why I did that many spelling/grammar mistakes.... :-)
> * @reset: ResettableState for the device; handled by Resettable interface.
> *
> * This structure should not be accessed directly. We declare it here
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/10] scsi/scsi-bus: scsi_device_find: don't return unrealized devices
2020-09-25 17:26 ` [PATCH 07/10] scsi/scsi-bus: scsi_device_find: don't return unrealized devices Paolo Bonzini
@ 2020-09-30 14:32 ` Maxim Levitsky
0 siblings, 0 replies; 31+ messages in thread
From: Maxim Levitsky @ 2020-09-30 14:32 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha
On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
>
> The device core first places a device on the bus and then realizes it.
> Make scsi_device_find avoid returing such devices to avoid
> races in drivers that use an iothread (currently virtio-scsi)
>
> 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>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20200913160259.32145-7-mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/scsi/scsi-bus.c | 83 +++++++++++++++++++++++++++++-----------------
> 1 file changed, 53 insertions(+), 30 deletions(-)
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 4ab9811cd8..7599113efe 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -24,6 +24,55 @@ static void scsi_target_free_buf(SCSIRequest *req);
>
> static int next_scsi_bus;
>
> +static SCSIDevice *do_scsi_device_find(SCSIBus *bus,
> + int channel, int id, int lun,
> + bool include_unrealized)
> +{
> + BusChild *kid;
> + SCSIDevice *retval = NULL;
> +
> + 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) {
> + retval = dev;
> + break;
Indeed, here no need to use goto, I probably removed some code that needed it
before or just forgot about.
> + }
> +
> + /*
> + * If we don't find exact match (channel/bus/lun),
> + * we will return the first device which matches channel/bus
> + */
> +
> + if (!retval) {
> + retval = dev;
> + }
> + }
> + }
> +
> + /*
> + * 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 (retval && !include_unrealized &&
> + !qatomic_load_acquire(&retval->qdev.realized)) {
> + retval = NULL;
> + }
> +
> + return retval;
> +}
I guess you dropped the RCU locking here since you want the callers
of this function to do it. I think it makes lot of sense.
> +
> +SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
> +{
> + RCU_READ_LOCK_GUARD();
> + return do_scsi_device_find(bus, channel, id, lun, false);
> +}
> +
> static void scsi_device_realize(SCSIDevice *s, Error **errp)
> {
> SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
> @@ -137,7 +186,10 @@ static bool scsi_bus_is_address_free(SCSIBus *bus,
> int channel, int target, int lun,
> SCSIDevice **p_dev)
> {
> - SCSIDevice *d = scsi_device_find(bus, channel, target, lun);
> + SCSIDevice *d;
> +
> + RCU_READ_LOCK_GUARD();
> + d = do_scsi_device_find(bus, channel, target, lun, true);
> if (d && d->lun == lun) {
> if (p_dev) {
> *p_dev = d;
> @@ -1570,35 +1622,6 @@ static char *scsibus_get_fw_dev_path(DeviceState *dev)
> qdev_fw_name(dev), d->id, d->lun);
> }
>
> -SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
> -{
> - BusChild *kid;
> - SCSIDevice *target_dev = NULL;
> -
> - RCU_READ_LOCK_GUARD();
> - 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) {
> - 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;
> - }
> - }
> - }
> -
> - return target_dev;
> -}
> -
> /* SCSI request list. For simplicity, pv points to the whole device */
>
> static int put_scsi_requests(QEMUFile *f, void *pv, size_t size,
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get
2020-09-25 17:26 ` [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get Paolo Bonzini
@ 2020-09-30 14:32 ` Maxim Levitsky
2020-09-30 17:46 ` Paolo Bonzini
0 siblings, 1 reply; 31+ messages in thread
From: Maxim Levitsky @ 2020-09-30 14:32 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Stefan Hajnoczi, stefanha
On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
>
> 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>
> Message-Id: <20200913160259.32145-8-mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> Compared to Maxim's patch, I am avoiding the extra argument
> to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD()
> out of do_scsi_device_find itself.
Which is a good idea, although my mindset was like, I got a device,
lets just grab a ref to it before it disappears and then do
whatever I want. The extra argument was ugly no doubt though.
Best regards,
Maxim Levitsky
>
> hw/scsi/scsi-bus.c | 11 +++++++++++
> include/hw/scsi/scsi.h | 1 +
> 2 files changed, 12 insertions(+)
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 7599113efe..eda8cb7e70 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -73,6 +73,17 @@ SCSIDevice *scsi_device_find(SCSIBus *bus, int channel, int id, int lun)
> return do_scsi_device_find(bus, channel, id, lun, false);
> }
>
> +SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int id, int lun)
> +{
> + SCSIDevice *d;
> + RCU_READ_LOCK_GUARD();
> + d = do_scsi_device_find(bus, channel, id, lun, false);
> + if (d) {
> + object_ref(d);
> + }
> + return d;
> +}
> +
> static void scsi_device_realize(SCSIDevice *s, Error **errp)
> {
> SCSIDeviceClass *sc = SCSI_DEVICE_GET_CLASS(s);
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 7a55cdbd74..09fa5c9d2a 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -190,6 +190,7 @@ 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;
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS
2020-09-25 17:26 ` [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS Paolo Bonzini
@ 2020-09-30 14:34 ` Maxim Levitsky
0 siblings, 0 replies; 31+ messages in thread
From: Maxim Levitsky @ 2020-09-30 14:34 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha
On Fri, 2020-09-25 at 13:26 -0400, Paolo Bonzini wrote:
> From: Maxim Levitsky <mlevitsk@redhat.com>
>
> Currently scsi_target_emulate_report_luns iterates over the child device list
> twice, and there is no guarantee that this list is the same in both iterations.
>
> The reason for iterating twice is that the first iteration calculates
> how much memory to allocate. However if we use a dynamic array we can
> avoid iterating twice, and therefore we avoid this race.
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1866707
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Message-Id: <20200913160259.32145-10-mlevitsk@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> hw/scsi/scsi-bus.c | 68 ++++++++++++++++++++++------------------------
> 1 file changed, 33 insertions(+), 35 deletions(-)
>
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index eda8cb7e70..b901e701f0 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -438,19 +438,23 @@ struct SCSITargetReq {
> static void store_lun(uint8_t *outbuf, int lun)
> {
> if (lun < 256) {
> + /* Simple logical unit addressing method*/
> + outbuf[0] = 0;
> outbuf[1] = lun;
> - return;
> + } else {
> + /* Flat space addressing method */
> + outbuf[0] = 0x40 | (lun >> 8);
> + outbuf[1] = (lun & 255);
> }
> - outbuf[1] = (lun & 255);
> - outbuf[0] = (lun >> 8) | 0x40;
> }
>
> static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
> {
> BusChild *kid;
> - int i, len, n;
> int channel, id;
> - bool found_lun0;
> + uint8_t tmp[8] = {0};
> + int len = 0;
> + GByteArray *buf;
>
> if (r->req.cmd.xfer < 16) {
> return false;
> @@ -458,46 +462,40 @@ static bool scsi_target_emulate_report_luns(SCSITargetReq *r)
> if (r->req.cmd.buf[2] > 2) {
> return false;
> }
> +
> + /* reserve space for 63 LUNs*/
> + buf = g_byte_array_sized_new(512);
> +
> channel = r->req.dev->channel;
> id = r->req.dev->id;
> - found_lun0 = false;
> - n = 0;
>
> - RCU_READ_LOCK_GUARD();
> + /* add size (will be updated later to correct value */
My mistake here - I forgot closing ')' - as I said, there will
be significantly less issues like that in my patches soon.
> + g_byte_array_append(buf, tmp, 8);
> + len += 8;
>
> - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> - DeviceState *qdev = kid->child;
> - SCSIDevice *dev = SCSI_DEVICE(qdev);
> + /* add LUN0 */
> + g_byte_array_append(buf, tmp, 8);
> + len += 8;
>
> - if (dev->channel == channel && dev->id == id) {
> - if (dev->lun == 0) {
> - found_lun0 = true;
> + WITH_RCU_READ_LOCK_GUARD() {
> + QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> + DeviceState *qdev = kid->child;
> + SCSIDevice *dev = SCSI_DEVICE(qdev);
> +
> + if (dev->channel == channel && dev->id == id && dev->lun != 0) {
> + store_lun(tmp, dev->lun);
> + g_byte_array_append(buf, tmp, 8);
> + len += 8;
> }
> - n += 8;
> }
> }
> - if (!found_lun0) {
> - n += 8;
> - }
> -
> - scsi_target_alloc_buf(&r->req, n + 8);
> -
> - len = MIN(n + 8, r->req.cmd.xfer & ~7);
> - memset(r->buf, 0, len);
> - stl_be_p(&r->buf[0], n);
> - i = found_lun0 ? 8 : 16;
> - QTAILQ_FOREACH_RCU(kid, &r->req.bus->qbus.children, sibling) {
> - DeviceState *qdev = kid->child;
> - SCSIDevice *dev = SCSI_DEVICE(qdev);
>
> - if (dev->channel == channel && dev->id == id) {
> - store_lun(&r->buf[i], dev->lun);
> - i += 8;
> - }
> - }
> + r->buf_len = len;
> + r->buf = g_byte_array_free(buf, FALSE);
> + r->len = MIN(len, r->req.cmd.xfer & ~7);
>
> - assert(i == n + 8);
> - r->len = len;
> + /* store the LUN list length */
> + stl_be_p(&r->buf[0], len - 8);
> return true;
> }
>
No doubt that I will RCU_READ_LOCK_GUARD more from now on.
Best regards,
Maxim Levitsky
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/10] device-core: use atomic_set on .realized property
2020-09-30 14:31 ` Maxim Levitsky
@ 2020-09-30 17:44 ` Paolo Bonzini
2020-09-30 18:03 ` Maxim Levitsky
0 siblings, 1 reply; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-30 17:44 UTC (permalink / raw)
To: Maxim Levitsky, qemu-devel; +Cc: stefanha
On 30/09/20 16:31, Maxim Levitsky wrote:
>> +
>> + qatomic_set(&dev->realized, value);
>> + /*
>> + * Ensure that concurrent users see this update prior to
>> + * any other changes done by unrealize.
>> + */
>> + smp_wmb();
>
> I''l probably never fully understand where to use read/write/full barrier.
> If I understand corrctly, read barrier prevents reads done by this thread to be reordered,
> by the CPU and write barrier prevents writes done by this CPU to be re-ordered.
I must say that the above is not really satisfactory. The right thing
to do would be to say which changes are done by unrealize; then you
should make sure that *after* reading something that unrealize could
undo you check if dev->realized is still true.
scsi_device_find is one such case, but I'm not convinced it is enough.
Paolo
> Both (depending on the macro) usually imply compiler barrier (to avoid compilier re-ordering
> stuff...)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get
2020-09-30 14:32 ` Maxim Levitsky
@ 2020-09-30 17:46 ` Paolo Bonzini
2020-09-30 18:04 ` Maxim Levitsky
0 siblings, 1 reply; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-30 17:46 UTC (permalink / raw)
To: Maxim Levitsky, qemu-devel; +Cc: Stefan Hajnoczi, stefanha
On 30/09/20 16:32, Maxim Levitsky wrote:
>> Compared to Maxim's patch, I am avoiding the extra argument
>> to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD()
>> out of do_scsi_device_find itself.
> Which is a good idea, although my mindset was like, I got a device,
> lets just grab a ref to it before it disappears and then do
> whatever I want.
Understood, but "I got a device, I know I'm under RCU so it can't
disappear" is more efficient and just as common. This also explains the
difference in patch 7.
Paolo
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/10] qdev: add "check if address free" callback for buses
2020-09-28 9:30 ` Stefan Hajnoczi
@ 2020-09-30 17:48 ` Paolo Bonzini
0 siblings, 0 replies; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-30 17:48 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-devel, mlevitsk
[-- Attachment #1.1: Type: text/plain, Size: 401 bytes --]
On 28/09/20 11:30, Stefan Hajnoczi wrote:
>> + bool (*check_address)(BusState *bus, DeviceState *dev, Error **errp);
> Please document this function.
I will add this:
/*
* Return whether the device can be added to @bus,
* based on the address that was set (via device properties)
* before realize. If not, on return @errp contains the
* human-readable error message.
*/
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/10] device-core: use atomic_set on .realized property
2020-09-30 17:44 ` Paolo Bonzini
@ 2020-09-30 18:03 ` Maxim Levitsky
0 siblings, 0 replies; 31+ messages in thread
From: Maxim Levitsky @ 2020-09-30 18:03 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: stefanha
On Wed, 2020-09-30 at 19:44 +0200, Paolo Bonzini wrote:
> On 30/09/20 16:31, Maxim Levitsky wrote:
> > > +
> > > + qatomic_set(&dev->realized, value);
> > > + /*
> > > + * Ensure that concurrent users see this update prior to
> > > + * any other changes done by unrealize.
> > > + */
> > > + smp_wmb();
> >
> > I''l probably never fully understand where to use read/write/full barrier.
> > If I understand corrctly, read barrier prevents reads done by this thread to be reordered,
> > by the CPU and write barrier prevents writes done by this CPU to be re-ordered.
>
> I must say that the above is not really satisfactory. The right thing
> to do would be to say which changes are done by unrealize; then you
> should make sure that *after* reading something that unrealize could
> undo you check if dev->realized is still true.
I didn't fully understand this to be honest.
I just wanted to explain what I know and what I don't know about hardware barriers.
I know that read barriers should be paired with write barriers, like if one CPU has a write barrier,
which ensures the orders of writes to two memory locations, the other CPU can then use a read barrier to ensure
that it sees those writes in this order.
I thus think that reads of dev->realized should be paired with read barrier,
but here a full memory barrier isn't really needed.
Best regards,
Maxim Levitsky
>
> scsi_device_find is one such case, but I'm not convinced it is enough.
>
> Paolo
>
> > Both (depending on the macro) usually imply compiler barrier (to avoid compilier re-ordering
> > stuff...)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get
2020-09-30 17:46 ` Paolo Bonzini
@ 2020-09-30 18:04 ` Maxim Levitsky
0 siblings, 0 replies; 31+ messages in thread
From: Maxim Levitsky @ 2020-09-30 18:04 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Stefan Hajnoczi, stefanha
On Wed, 2020-09-30 at 19:46 +0200, Paolo Bonzini wrote:
> On 30/09/20 16:32, Maxim Levitsky wrote:
> > > Compared to Maxim's patch, I am avoiding the extra argument
> > > to do_scsi_device_find by moving the RCU_READ_LOCK_GUARD()
> > > out of do_scsi_device_find itself.
> > Which is a good idea, although my mindset was like, I got a device,
> > lets just grab a ref to it before it disappears and then do
> > whatever I want.
>
> Understood, but "I got a device, I know I'm under RCU so it can't
> disappear" is more efficient and just as common. This also explains the
> difference in patch 7.
Fair point. I am still learing to correctly use RCU.
Best regards,
Maxim Levitsky
>
> Paolo
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 01/10] qdev: add "check if address free" callback for buses
2020-09-30 14:27 ` Maxim Levitsky
@ 2020-09-30 23:37 ` Paolo Bonzini
0 siblings, 0 replies; 31+ messages in thread
From: Paolo Bonzini @ 2020-09-30 23:37 UTC (permalink / raw)
To: Maxim Levitsky, qemu-devel; +Cc: stefanha
On 30/09/20 16:27, Maxim Levitsky wrote:
> My patch that switches the direction in scsi_device_find, is supposed to be completely equavalent,
> based on the following train of thought:
>
> If scsi_device_find finds an exact match it returns only it, as before.
>
> Otherwise scsi_device_find were to scan from end of the list to the start, and every time,
> it finds a device with same channel/id it would update the target_dev
> and return it when it reaches the end of the list.
>
> If I am not mistaken this means that it would return _first_ device in the
> list that matches the channel/id.
> This is exactly what new version of scsi_device_find does.
Oh! I missed that subtlety. Thanks, that makes sense.
Paolo
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-09-30 23:37 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-25 17:25 [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread Paolo Bonzini
2020-09-25 17:25 ` [PATCH 01/10] qdev: add "check if address free" callback for buses Paolo Bonzini
2020-09-28 9:30 ` Stefan Hajnoczi
2020-09-30 17:48 ` Paolo Bonzini
2020-09-30 14:27 ` Maxim Levitsky
2020-09-30 23:37 ` Paolo Bonzini
2020-09-25 17:25 ` [PATCH 02/10] scsi: switch to bus->check_address Paolo Bonzini
2020-09-28 9:43 ` Stefan Hajnoczi
2020-09-30 14:28 ` Maxim Levitsky
2020-09-25 17:25 ` [PATCH 03/10] scsi/scsi_bus: switch search direction in scsi_device_find Paolo Bonzini
2020-09-25 17:25 ` [PATCH 04/10] device_core: use drain_call_rcu in in hmp_device_del/qmp_device_add Paolo Bonzini
2020-09-25 17:25 ` [PATCH 05/10] device-core: use RCU for list of children of a bus Paolo Bonzini
2020-09-30 14:29 ` Maxim Levitsky
2020-09-25 17:26 ` [PATCH 06/10] device-core: use atomic_set on .realized property Paolo Bonzini
2020-09-30 14:31 ` Maxim Levitsky
2020-09-30 17:44 ` Paolo Bonzini
2020-09-30 18:03 ` Maxim Levitsky
2020-09-25 17:26 ` [PATCH 07/10] scsi/scsi-bus: scsi_device_find: don't return unrealized devices Paolo Bonzini
2020-09-30 14:32 ` Maxim Levitsky
2020-09-25 17:26 ` [PATCH 08/10] scsi/scsi_bus: Add scsi_device_get Paolo Bonzini
2020-09-30 14:32 ` Maxim Levitsky
2020-09-30 17:46 ` Paolo Bonzini
2020-09-30 18:04 ` Maxim Levitsky
2020-09-25 17:26 ` [PATCH 09/10] virtio-scsi: use scsi_device_get Paolo Bonzini
2020-09-25 17:26 ` [PATCH 10/10] scsi/scsi_bus: fix races in REPORT LUNS Paolo Bonzini
2020-09-30 14:34 ` Maxim Levitsky
2020-09-25 19:26 ` [PATCH 00/10] Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread no-reply
2020-09-25 22:52 ` no-reply
2020-09-26 0:28 ` no-reply
2020-09-26 0:44 ` no-reply
2020-09-26 1:05 ` no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.