Re: [PATCH for-11.2 v3 15/15] hw/qdev: Prevent devices from being realized more than once

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;
>>>>   }