| Message ID | 20200715150159.95050-5-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix scsi devices plug/unplug races w.r.t virtio-scsi iothread | expand |
On Wed, Jul 15, 2020 at 06:01:56PM +0300, Maxim Levitsky wrote: > Some code might race with placement of new devices on a bus. > We currently first place a (unrealized) device on the bus > and then realize it. > > As a workaround, users that scan the child device list, can > check the realized property to see if it is safe to access such a device. > Use an atomic write here too to aid with this. > > A separate discussion is what to do with devices that are unrealized: > It looks like for this case we only call the hotplug handler's unplug > callback and its up to it to unrealize the device. > An atomic operation doesn't cause harm for this code path though. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > hw/core/qdev.c | 19 ++++++++++++++++++- > include/hw/qdev-core.h | 2 ++ > 2 files changed, 20 insertions(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index ee69b11d11..2ec4971aec 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -966,7 +966,25 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } } + atomic_store_release(&dev->realized, value); + } else if (!value && dev->realized) { + + /* + * Change the value so that any concurrent users are aware + * that the device is going to be unrealized + * + * TODO: change .realized property to enum that states + * each phase of the device realization/unrealization + */ + + atomic_set(&dev->realized, value); + /* + * execute full memory barrier to ensure that concurrent users + * see this update prior to any other changes to the device + */ + smp_mb(); + QLIST_FOREACH(bus, &dev->child_bus, sibling) { qbus_unrealize(bus); } @@ -981,7 +999,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 e252640c1a..5faa715449 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -165,6 +165,8 @@ struct NamedClockList { /** * DeviceState: * @realized: Indicates whether the device has been fully constructed. + * When accessed outsize big qemu lock, must be accessed with + * atomic_load_acquire() * @reset: ResettableState for the device; handled by Resettable interface. * * This structure should not be accessed directly. We declare it here
Some code might race with placement of new devices on a bus. We currently first place a (unrealized) device on the bus and then realize it. As a workaround, users that scan the child device list, can check the realized property to see if it is safe to access such a device. Use an atomic write here too to aid with this. A separate discussion is what to do with devices that are unrealized: It looks like for this case we only call the hotplug handler's unplug callback and its up to it to unrealize the device. An atomic operation doesn't cause harm for this code path though. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- hw/core/qdev.c | 19 ++++++++++++++++++- include/hw/qdev-core.h | 2 ++ 2 files changed, 20 insertions(+), 1 deletion(-)