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

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?

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