Re: [RFC PATCH 4/9] qapi: Implement 'query-machine-phase' command

[Daniel P . Berrangé]

On Thu, May 13, 2021 at 07:44:34PM +0200, Paolo Bonzini wrote:
> On 13/05/21 10:25, Mirela Grujic wrote:
> > The command returns current machine initialization phase.
> >  From now on, the MachineInitPhase enum is generated.
> > 
> > Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
> > ---
> >   qapi/machine.json          | 54 ++++++++++++++++++++++++++++++++++++++
> >   include/hw/qdev-core.h     | 29 +-------------------
> >   hw/core/machine-qmp-cmds.c |  9 +++++++
> >   3 files changed, 64 insertions(+), 28 deletions(-)
> > 
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index 6e90d463fc..47bdbec817 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1274,3 +1274,57 @@
> >   ##
> >   { 'event': 'MEM_UNPLUG_ERROR',
> >     'data': { 'device': 'str', 'msg': 'str' } }
> > +
> > +##
> > +# @MachineInitPhase:
> > +#
> > +# Enumeration of machine initialization phases.
> > +#
> > +# @no-machine: machine does not exist
> > +#
> > +# @machine-created: machine is created, but its accelerator is not
> > +#
> > +# @accel-created: accelerator is created, but the machine properties have 
> > not
> > +#                 been validated and machine initialization is not done yet
> > +#
> > +# @initialized: machine is initialized, thus creating any embedded devices 
> > and
> > +#               validating machine properties. Devices created at this 
> > time are
> > +#               considered to be cold-plugged.
> > +#
> > +# @ready: QEMU is ready to start CPUs and devices created at this time are
> > +#         considered to be hot-plugged. The monitor is not restricted to
> > +#         "preconfig" commands.
> > +##
> > +{ 'enum': 'MachineInitPhase',
> > +  'data': [ 'no-machine', 'machine-created', 'accel-created', 
> > 'initialized',
> > +            'ready' ] }
> > +
> > +##
> > +# @MachineInitPhaseStatus:
> > +#
> > +# Information about machine initialization phase
> > +#
> > +# @phase: the machine initialization phase
> > +#
> > +# Since:  #FIXME
> > +##
> > +{ 'struct': 'MachineInitPhaseStatus',
> > +  'data': { 'phase': 'MachineInitPhase' } }
> > +
> > +##
> > +# @query-machine-phase:
> > +#
> > +# Return machine initialization phase
> > +#
> > +# Since: #FIXME
> > +#
> > +# Returns: MachineInitPhaseStatus
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "query-machine-phase" }
> > +# <- { "return": { "phase": "initialized" } }
> > +#
> > +##
> > +{ 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
> > +             'allow-preconfig': true }
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index dc2f63478b..ca39b77ae6 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -1,6 +1,7 @@
> >   #ifndef QDEV_CORE_H
> >   #define QDEV_CORE_H
> > +#include "qapi/qapi-types-machine.h"
> >   #include "qemu/queue.h"
> >   #include "qemu/bitmap.h"
> >   #include "qemu/rcu.h"
> > @@ -811,34 +812,6 @@ void device_listener_unregister(DeviceListener 
> > *listener);
> >    */
> >   bool qdev_should_hide_device(QemuOpts *opts);
> > -typedef enum MachineInitPhase {
> > -    /* current_machine is NULL.  */
> > -    MACHINE_INIT_PHASE_NO_MACHINE,
> > -
> > -    /* current_machine is not NULL, but current_machine->accel is NULL.  */
> > -    MACHINE_INIT_PHASE_MACHINE_CREATED,
> > -
> > -    /*
> > -     * current_machine->accel is not NULL, but the machine properties have
> > -     * not been validated and machine_class->init has not yet been called.
> > -     */
> > -    MACHINE_INIT_PHASE_ACCEL_CREATED,
> > -
> > -    /*
> > -     * machine_class->init has been called, thus creating any embedded
> > -     * devices and validating machine properties.  Devices created at
> > -     * this time are considered to be cold-plugged.
> > -     */
> > -    MACHINE_INIT_PHASE_INITIALIZED,
> > -
> > -    /*
> > -     * QEMU is ready to start CPUs and devices created at this time
> > -     * are considered to be hot-plugged.  The monitor is not restricted
> > -     * to "preconfig" commands.
> > -     */
> > -    MACHINE_INIT_PHASE_READY,
> > -} MachineInitPhase;
> > -
> >   extern bool phase_check(MachineInitPhase phase);
> >   extern void phase_advance(MachineInitPhase phase);
> >   extern MachineInitPhase phase_get(void);
> > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> > index be286882cc..23f837dadb 100644
> > --- a/hw/core/machine-qmp-cmds.c
> > +++ b/hw/core/machine-qmp-cmds.c
> > @@ -198,3 +198,12 @@ MemdevList *qmp_query_memdev(Error **errp)
> >       object_child_foreach(obj, query_memdev, &list);
> >       return list;
> >   }
> > +
> > +MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
> > +{
> > +    MachineInitPhaseStatus *status = g_malloc0(sizeof(*status));
> > +
> > +    status->phase = phase_get();
> > +
> > +    return status;
> > +}
> > 
> 
> This command is a good idea, and we can in fact even include it already in
> QEMU.

I worry that this is exposing an internal QEMU implementation detail.
If apps rely on a particular set of phases existing, and running in
particular order, it could easily hobble our ability to refactor
QEMU's startup procedure to cope with new requirements in future. 

I don't even much like the existing "pre-config" concept we have
for this same reason, and this is adding many more phases.

I think we'd be better off trying to make it possible for the mgmt
app to just provide all the config at once and let QEMU figure our
the phases in which it then instantiates stuff, so we can freedom
to change our minds at any time.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|