On Fri, 27 Nov 2020 06:19:51 +0100 Paolo Bonzini <pbonz...@redhat.com> wrote:
> On 26/11/20 19:55, Igor Mammedov wrote: > > On Mon, 23 Nov 2020 09:14:25 -0500 > > Paolo Bonzini <pbonz...@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 <pbonz...@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 >