Re: [PATCH v4 02/14] machine&vl: introduce phase_until() to handle phase transitions

[Damien Hedde <damien.hedde@greensocs.com>]

Hi !

It would nice to have some feedback about this solution.
Basically it just introduces a new function 'qemu_until' which implement
the startup fsm.
It's bit like what Markus proposed in december (hence the name), only it 
is is only internal so we can change it if we find out it should be done 
otherwise regarding startup.

In practice it is just some code move from qmp_exit_preconfig() to 
phase_until() with more indentation (not sure if there is a way to make 
that easier to read).

In the following patches I use it in device_add() to ensure the startup 
is advanced.

Thanks,
--
Damien

On 2/23/22 10:06, Damien Hedde wrote:
> phase_until() is implemented in vl.c and is meant to be used
> to make startup progress up to a specified phase being reached().
> At this point, no behavior change is introduced: phase_until()
> only supports a single double transition corresponding
> to the functionality of qmp_exit_preconfig():
> + accel-created -> machine-initialized -> machine-ready
> 
> As a result qmp_exit_preconfig() now uses phase_until().
> 
> This commit is a preparation to support cold plugging a device
> using qapi command (which will be introduced in a following commit).
> For this we need fine grain control of the phase.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   include/hw/qdev-core.h | 14 ++++++++
>   softmmu/vl.c           | 78 ++++++++++++++++++++++++++++++++----------
>   2 files changed, 74 insertions(+), 18 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index e29c705b74..5f73d06408 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -909,4 +909,18 @@ extern bool phase_check(MachineInitPhase phase);
>    */
>   extern void phase_advance(MachineInitPhase phase);
>   
> +/**
> + * @phase_until:
> + * @phase: the target phase
> + * @errp: error report
> + *
> + * Make the machine init progress until the target phase is reached.
> + *
> + * Its is a no-op is the target phase is the current or an earlier
> + * phase.
> + *
> + * Returns true in case of success.
> + */
> +extern bool phase_until(MachineInitPhase phase, Error **errp);
> +
>   #endif
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 5e1b35ba48..5689d0be88 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2741,30 +2741,72 @@ void qmp_x_exit_preconfig(Error **errp)
>           error_setg(errp, "The command is permitted only before machine initialization");
>           return;
>       }
> +    phase_until(PHASE_MACHINE_READY, errp);
> +}
>   
> -    qemu_init_board();
> -    qemu_create_cli_devices();
> -    qemu_machine_creation_done();
> -
> -    if (loadvm) {
> -        load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
> -    }
> -    if (replay_mode != REPLAY_MODE_NONE) {
> -        replay_vmstate_init();
> +bool phase_until(MachineInitPhase phase, Error **errp)
> +{
> +    if (!phase_check(PHASE_ACCEL_CREATED)) {
> +        error_setg(errp, "Phase transition is not supported until accelerator"
> +                   " is created");
> +        return false;
>       }
>   
> -    if (incoming) {
> -        Error *local_err = NULL;
> -        if (strcmp(incoming, "defer") != 0) {
> -            qmp_migrate_incoming(incoming, &local_err);
> -            if (local_err) {
> -                error_reportf_err(local_err, "-incoming %s: ", incoming);
> -                exit(1);
> +    while (!phase_check(phase)) {
> +        MachineInitPhase cur_phase = phase_get();
> +
> +        switch (cur_phase) {
> +        case PHASE_ACCEL_CREATED:
> +            qemu_init_board();
> +            /* We are now in PHASE_MACHINE_INITIALIZED. */
> +            qemu_create_cli_devices();
> +            /*
> +             * At this point all CLI options are handled apart:
> +             * + -S (autostart)
> +             * + -incoming
> +             */
> +            qemu_machine_creation_done();
> +            /* We are now in PHASE_MACHINE_READY. */
> +
> +            if (loadvm) {
> +                load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
>               }
> +            if (replay_mode != REPLAY_MODE_NONE) {
> +                replay_vmstate_init();
> +            }
> +
> +            if (incoming) {
> +                Error *local_err = NULL;
> +                if (strcmp(incoming, "defer") != 0) {
> +                    qmp_migrate_incoming(incoming, &local_err);
> +                    if (local_err) {
> +                        error_reportf_err(local_err, "-incoming %s: ",
> +                                          incoming);
> +                        exit(1);
> +                    }
> +                }
> +            } else if (autostart) {
> +                qmp_cont(NULL);
> +            }
> +            break;
> +
> +        default:
> +            /*
> +             * If we end up here, it is because we miss a case above.
> +             */
> +            error_setg(&error_abort, "Requested phase transition is not"
> +                       " implemented");
> +            return false;
>           }
> -    } else if (autostart) {
> -        qmp_cont(NULL);
> +
> +        /*
> +         * Ensure we made some progress.
> +         * With the default case above, it should be enough to prevent
> +         * any infinite loop.
> +         */
> +        assert(cur_phase < phase_get());
>       }
> +    return true;
>   }
>   
>   void qemu_init(int argc, char **argv, char **envp)