| Message ID | 20210513082549.114275-8-mirela.grujic@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | Initial support for machine creation via QMP | expand |
On 13/05/21 10:25, Mirela Grujic wrote: > The existing code had to be restructured to make room for adding > checks that are specific to the machine phases. > > The fix is related to the way that commands with the 'allow-preconfig' > option are treated. > > Commands labelled with the 'allow-preconfig' option were meant to be allowed > during the 'preconfig' state, i.e. before the machine is initialized. > The equivalent of 'preconfig' state (after its removal) is machine init > phase MACHINE_INIT_PHASE_ACCEL_CREATED. Therefore, commands with > 'allow-preconfig' option should be allowed to run while the machine is > in MACHINE_INIT_PHASE_ACCEL_CREATED phase. > Before this patch, those commands were allowed to run if the machine is > not ready, which includes three more phases besides the accel-created > phase. Since there were no means to enter other phases via QMP, the > implementation was fine. However, with the introduction of > 'next-machine-phase' and 'advance-machine-phase' commands, one could > enter machine 'initialized' phase and still have available 'preconfig' > commands, which is incorrect. > > This patch makes available 'allow-preconfig' commands only when they're > needed - before the machine is initialized, in the accel-created phase. > To enable a command after the machine gets initialized and before it > becomes ready, one should use 'allow-init-config' option that will be > introduced in the following patch. There aren't many commands that are valid only for the accel created or machine initialized phase. I think adding allow-init-config is more churn than keeping only allow-preconfig, and calling phase_check in the individual commands. (Or even better, in the internal APIs that they call, so that QMP is completely oblivious to phases and just gets the Error* back). In other words, allow-preconfig is there because there are many commands that are allowed only after the machine-ready phase, but anything in the middle can be handled just fine from C code. Paolo
On 5/13/21 7:43 PM, Paolo Bonzini wrote: > On 13/05/21 10:25, Mirela Grujic wrote: >> The existing code had to be restructured to make room for adding >> checks that are specific to the machine phases. >> >> The fix is related to the way that commands with the 'allow-preconfig' >> option are treated. >> >> Commands labelled with the 'allow-preconfig' option were meant to be >> allowed >> during the 'preconfig' state, i.e. before the machine is initialized. >> The equivalent of 'preconfig' state (after its removal) is machine init >> phase MACHINE_INIT_PHASE_ACCEL_CREATED. Therefore, commands with >> 'allow-preconfig' option should be allowed to run while the machine is >> in MACHINE_INIT_PHASE_ACCEL_CREATED phase. >> Before this patch, those commands were allowed to run if the machine is >> not ready, which includes three more phases besides the accel-created >> phase. Since there were no means to enter other phases via QMP, the >> implementation was fine. However, with the introduction of >> 'next-machine-phase' and 'advance-machine-phase' commands, one could >> enter machine 'initialized' phase and still have available 'preconfig' >> commands, which is incorrect. >> >> This patch makes available 'allow-preconfig' commands only when they're >> needed - before the machine is initialized, in the accel-created phase. >> To enable a command after the machine gets initialized and before it >> becomes ready, one should use 'allow-init-config' option that will be >> introduced in the following patch. > > There aren't many commands that are valid only for the accel created > or machine initialized phase. I think adding allow-init-config is > more churn than keeping only allow-preconfig, and calling phase_check > in the individual commands. (Or even better, in the internal APIs > that they call, so that QMP is completely oblivious to phases and just > gets the Error* back). > > In other words, allow-preconfig is there because there are many > commands that are allowed only after the machine-ready phase, but > anything in the middle can be handled just fine from C code. To summarize, 'allow-preconfig' specifies whether a command is allowed to run before the machine is ready, so any command that should be allowed to run at phase < 'machine-ready' must have this flag set. For those commands, one should check the current machine phase in the implementation of the command to determine whether the command should run or not, and return an error if not. Ok, that's fine. > > Paolo >
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c index be8a892517..448f9dbb6f 100644 --- a/softmmu/qdev-monitor.c +++ b/softmmu/qdev-monitor.c @@ -998,10 +998,19 @@ int qemu_global_option(const char *str) bool qmp_command_available(const QmpCommand *cmd, Error **errp) { - if (!machine_is_ready() && !(cmd->options & QCO_ALLOW_PRECONFIG)) { - error_setg(errp, "The command '%s' is permitted only after machine initialization has completed", - cmd->name); - return false; + switch (phase_get()) { + case MACHINE_INIT_PHASE_ACCEL_CREATED: + if (cmd->options & QCO_ALLOW_PRECONFIG) { + return true; + } + break; + case MACHINE_INIT_PHASE_READY: + /* All commands are available when the machine is ready */ + return true; + default: + break; } - return true; + error_setg(errp, "The command '%s' is not permitted at this phase", + cmd->name); + return false; }
The existing code had to be restructured to make room for adding checks that are specific to the machine phases. The fix is related to the way that commands with the 'allow-preconfig' option are treated. Commands labelled with the 'allow-preconfig' option were meant to be allowed during the 'preconfig' state, i.e. before the machine is initialized. The equivalent of 'preconfig' state (after its removal) is machine init phase MACHINE_INIT_PHASE_ACCEL_CREATED. Therefore, commands with 'allow-preconfig' option should be allowed to run while the machine is in MACHINE_INIT_PHASE_ACCEL_CREATED phase. Before this patch, those commands were allowed to run if the machine is not ready, which includes three more phases besides the accel-created phase. Since there were no means to enter other phases via QMP, the implementation was fine. However, with the introduction of 'next-machine-phase' and 'advance-machine-phase' commands, one could enter machine 'initialized' phase and still have available 'preconfig' commands, which is incorrect. This patch makes available 'allow-preconfig' commands only when they're needed - before the machine is initialized, in the accel-created phase. To enable a command after the machine gets initialized and before it becomes ready, one should use 'allow-init-config' option that will be introduced in the following patch. Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com> --- softmmu/qdev-monitor.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)