On 2026/07/22 18:57, Philippe Mathieu-Daudé wrote:
On 22/7/26 07:12, Akihiko Odaki wrote:
On 2026/07/22 5:11, Philippe Mathieu-Daudé wrote:
Hi Akihiko,
On 21/7/26 10:17, Akihiko Odaki wrote:
qdev currently permits reentrant realization of the same device. It
also
permits another realization attempt after a device has been unrealized
or a previous attempt has failed. Either path can invoke
DeviceClass::realize() more than once. Supporting repeated realization
adds complexity to device implementations. It is untested and likely
broken.
Replace the bool DeviceState::realized field with the enum-valued
DeviceState::phase field. The enum has four values:
- initialized
- realizing
- realized
- retired
Excellent.
I have been working on something similar.
I'd start the first patch only including:
DEVICE_PHASE_UNREALIZED (false)
DEVICE_PHASE_REALIZED (true)
Then gradually rename DEVICE_PHASE_REALIZED -> DEVICE_PHASE_CREATED
and add the DEVICE_PHASE_REALIZING and DEVICE_PHASE_RETIRED phases,
so we can discuss them during the review process.
A gradual conversion makes sense. I kept the "realized" phase as-is
because it maps exactly to the current external behavior. This patch
splits the internal "unrealized" state into three distinct phases, but
the external concept of being "realized" remains unchanged. This
allows us to avoid a tree-wide refactoring, which is also why
qdev_is_realized() is preserved.
Realization can start only in the initialized phase. It moves the
device
to the realizing phase before invoking callbacks, preventing another
realization attempt. Successful realization moves it to the realized
phase; failure after realization has started moves it to the retired
phase. Unrealization also moves a realized device to the retired phase.
So what is the difference between 'initialized' and 'retired'?
The first statement in this paragraph differentiates 'initialized'
from everything else: realization can start only in the initialized
phase. A 'retired' device cannot be realized. This property avoids re-
entrancy.
But we do use unrealize -> realize again, in hotplug path.
So we need to be able to move from 'retired' to 'realizing'
again, thus my wonder what is the difference between 'realizing'
and 'initialized'.
I.e. this test should pass:
static void test_qdev_realize_hotplug(void)
{
Object *mt = object_new(TYPE_MY_DEV);
/* plug */
g_assert_false(qdev_realize(DEVICE(mt), NULL, NULL));
/* unplug */
qdev_unrealize(DEVICE(mt));
/* re-plug */
g_assert_false(qdev_realize(DEVICE(mt), NULL, NULL));
qdev_unrealize(DEVICE(mt));
object_unparent(mt);
object_unref(mt);
}
Maybe your 'retired' could be renamed as transient 'unrealizing',
similar to 'realizing' phase, then we could transition to the
'unrealized' initial phase?
I could not find an in-tree hotplug path that unrealizes and then
realizes the same DeviceState.
In the normal device_del path, the unplug handler unrealizes the device,
and completed unplug then unparents it. A later device_add calls
qdev_new(), so it realizes a new DeviceState. Reusing an ID or slot does
not reuse the object.
The virtio-net failover path does retain and replug the same object, but
it deliberately keeps the device realized. The partial-unplug path skips
the unplug handler, and replug invokes the pre_plug and plug callbacks
directly instead of qdev_realize().
More generally, I believe same-instance unrealize -> realize is unsafe.
Realize callbacks may create QOM children whose lifetime is tied to the
DeviceState rather than its realized state [1]. For example,
memory_region_init() initializes an embedded QOM object and adds it as a
child of the device. Unrealizing the device does not generally finalize
that MemoryRegion, so a second realization may try to initialize the
same object again.
So introducing an unrealizing -> unrealized transition would require
every realize/unrealize pair to restore the state of a fresh instance.
That contract is not tested, and is the complexity this series is meant
to remove.
[1]
https://lore.kernel.org/qemu-devel/[email protected]/
Regards,
Akihiko Odaki
The QOM realized property is an internal lifecycle property, not for
end users. Replace it with the enum-valued phase property.
Signed-off-by: Akihiko Odaki <[email protected]>
---
qapi/common.json | 19 ++++++++
include/hw/core/qdev.h | 12 ++---
hw/core/qdev-clock.c | 4 +-
hw/core/qdev-properties.c | 4 +-
hw/core/qdev.c | 98 +++++++++++++++++++++++++
+--------------
hw/scsi/scsi-bus.c | 4 +-
qom/qom-qmp-cmds.c | 2 +-
system/qdev-monitor.c | 5 ++-
tests/unit/test-qdev.c | 112 ++++++++++++++++++++++++++++++++++
+ + +++++++++-
9 files changed, 212 insertions(+), 48 deletions(-)
diff --git a/qapi/common.json b/qapi/common.json
index af7e3d618a7c..88a308cbd172 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -7,6 +7,25 @@
# *****************
##
+##
+# @DevicePhase:
+#
+# An enumeration of the device phases
+#
+# @initialized: the initial phase
+#
+# @realizing: the phase during realization
+#
+# @realized: the phase after realization
+#
+# @retired: the terminal phase entered when unrealization begins or
+# realization fails after starting
+#
+# Since: 11.1
+##
+{ 'enum': 'DevicePhase',
+ 'data': [ 'initialized', 'realizing', 'realized', 'retired' ] }
+
@@ -477,10 +477,10 @@ bool qdev_unplug_blocked(DeviceState *dev,
Error **errp)
return false;
}
-static bool device_get_realized(Object *obj, Error **errp)
+static int device_get_phase(Object *obj, Error **errp)
DevicePhase
device_get_phase() must return int to match the getter type required by
object_class_property_add_enum():
int (*get)(Object *, Error **)
Using DevicePhase there would not match the callback type.
Ah right.
{
DeviceState *dev = DEVICE(obj);
- return dev->realized;
+ return dev->phase;
}
