Re: [PATCH 26/36] remove preconfig state

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 27 Nov 2020 06:19:51 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 26/11/20 19:55, Igor Mammedov wrote:
> > On Mon, 23 Nov 2020 09:14:25 -0500
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >   
> >> The preconfig state is only used if -incoming is not specified, which
> >> makes the RunState state machine more tricky than it need be.  However
> >> there is already an equivalent condition which works even with -incoming,
> >> namely qdev_hotplug.  Use it instead of a separate runstate.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>   hw/core/machine-qmp-cmds.c    |  5 ++---
> >>   include/qapi/qmp/dispatch.h   |  1 +
> >>   monitor/hmp.c                 |  7 ++++---
> >>   monitor/qmp-cmds.c            |  5 ++---
> >>   qapi/qmp-dispatch.c           |  5 +----
> >>   qapi/run-state.json           |  5 +----
> >>   softmmu/qdev-monitor.c        | 12 ++++++++++++
> >>   softmmu/vl.c                  | 13 ++-----------
> >>   stubs/meson.build             |  1 +
> >>   stubs/qmp-command-available.c |  7 +++++++
> >>   tests/qtest/qmp-test.c        |  2 +-
> >>   11 files changed, 34 insertions(+), 29 deletions(-)
> >>   create mode 100644 stubs/qmp-command-available.c
> >>
> >> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> >> index 5362c80a18..cb9387c5f5 100644
> >> --- a/hw/core/machine-qmp-cmds.c
> >> +++ b/hw/core/machine-qmp-cmds.c
> >> @@ -286,9 +286,8 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
> >>   
> >>   void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
> >>   {
> >> -    if (!runstate_check(RUN_STATE_PRECONFIG)) {
> >> -        error_setg(errp, "The command is permitted only in '%s' state",
> >> -                   RunState_str(RUN_STATE_PRECONFIG));
> >> +    if (qdev_hotplug) {  
> > 
> > that would work only as long as qemu_init_board() hasn't been called,
> > and fall apart as soon as we permit creating cold-pluged devices
> > (qemu_create_cli_devices()) at preconfig stage.
> > 
> > for qmp_set_numa_node() the better fit would something like
> >    if(is_board_created)
> >       error_out
> > so it won't break silently when we start extending list of
> > commands allowed at preconfig time.
> >   
> >> +         error_setg(errp, "The command is permitted only before the machine has been created");
> >>            return;
> >>       }  
> 
> I don't understand...  qdev_hotplug is a bad name for is_board_created, 
> it is set by qdev_machine_creation_done which is called after preconfig 
> is left.  As of this patch that happens after the early 
> qemu_main_loop(); the next patch moves it to qmp_x_exit_preconfig.
it works in context of this series since

  +    qemu_init_board();
  +    qemu_create_cli_devices();
  +    qemu_machine_creation_done();

are called within the same command qmp_x_exit_preconfig, 
if preconfig is enabled we happen to call qmp_set_numa_node()
and if (qdev_hotplug) {error} work as expected, since qemu_init_board()
hasn't been called yet.

but I'm thinking about what happens beyond this series, when we start
splitting qmp_x_exit_preconfig() after this series on separate stages.

By using qdev_hotplug here, we practically loose dependency tracking
on qemu_init_board() not being yet called. And if later we forget that,
then it would allow to call qmp_set_numa_node() after qemu_init_board()
but before qemu_machine_creation_done()

So for this intermediate stage, instead of abusing qdev_hotplug adding
a temporary is_board_created might be used. And when we introduce
new phases you've described below, is_board_created could be replaced
with appropriate phase check.


> Cold-plugged devices would (by definition) be created while qdev_hotplug 
> is false.  But before we get there, I will have replaced the two states 
> permitted by qdev_hotplug with five separate phases (PHASE_NO_MACHINE, 
> PHASE_MACHINE_CREATED, PHASE_ACCEL_CREATED, PHASE_MACHINE_INITIALIZED, 
> PHASE_MACHINE_READY) to clarify the QMP command implementation and to 
> assert that various functions are called in the right phase.
> 
> Thanks,
> 
> Paolo
>