Hi,
Old patch committed as a23151e8cc8 (Oct 2020).
I'm revisiting it in the context of dynamic heterogeneous machines.
On 6/10/20 14:39, Maxim Levitsky wrote:
Some code might race with placement of new devices on a bus.
So IIUC at least 2 vCPUs plug distinct devices on the same bus,
and the problem is the bus slot assigned for the device. What is
deciding the free slot, the guest code or QEMU?
We currently first place a (unrealized) device on the bus
and then realize it.
Sound like ill design. Maybe resulting from the unclear definition
on what "realized" means. "Guest visible"? "Wired on some bus"?
Something I tried to clarify once (now outdated):
https://lore.kernel.org/qemu-devel/[email protected]/
A vCPU thread shouldn't poke at QOM/QDev internals. When it accesses
a device (to check "is it out of reset?"), this is already existing
realized.
Actually, when the guest asks to assign the device to the bus, the
device is already existing (from guest PoV), so must be realized.
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.
I'd appreciate to be pointed at real use case we fixed here, to
better figure what could be reworked on the QBus / QDev layer in
order to not depend anymore on this workaround.
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.
Likely the hotplugging path is what needs to be improved (I'm seeing
a similar problem with vCPU hotplug).
Thanks,
Phil.
An atomic operation doesn't cause harm for this code path though.
Signed-off-by: Maxim Levitsky <[email protected]>
Reviewed-by: Stefan Hajnoczi <[email protected]>
Message-Id: <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
hw/core/qdev.c | 19 ++++++++++++++++++-
include/hw/qdev-core.h | 2 ++
2 files changed, 20 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 2c6307e3ed..868973319e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -163,6 +163,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
