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

On 22/7/26 13:08, Akihiko Odaki wrote:
> 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.

Igor, could you provide your advices here?

> 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.

I totally concur here.

> 
> 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/64bc4a38-f1d2-45ff-8f4c- 
> [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;
>>>>>   }
> 
>