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

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.

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

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

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