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

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.

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

> 
>>   {
>>       DeviceState *dev = DEVICE(obj);
>> -    return dev->realized;
>> +    return dev->phase;
>>   }
> 
> 
>> @@ -670,7 +702,6 @@ static void device_initfn(Object *obj)
>>       }
>>       dev->instance_id_alias = -1;
>> -    dev->realized = false;
> 
> Can we keep the initialization explicit? This helps when navigating
> the code base.

Yes, I will make that change in the next version.

Regards,
Akihiko Odaki

> 
>>       dev->allow_unplug_during_migration = false;
>>       QLIST_INIT(&dev->gpios);