| Message ID | 20200925172604.2142227-2-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand |
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.
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
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. */
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
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;
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(-)