Re: [Qemu-devel] [PATCH v6 03/11] cli: add --preconfig option

[Eric Blake]

On 04/27/2018 10:05 AM, Igor Mammedov wrote:
> This option allows pausing QEMU in the new RUN_STATE_PRECONFIG state,
> allowing the configuration of QEMU from QMP before the machine jumps
> into board initialization code of machine_run_board_init()
> 
> The intent is to allow management to query machine state and additionally
> configure it using previous query results within one QEMU instance
> (i.e. eliminate the need to start QEMU twice, 1st to query board specific
> parameters and 2nd for actual VM start using query results for
> additional parameters).
> 
> The new option complements -S option and could be used with or without
> it. The difference is that -S pauses QEMU when the machine is completely
> initialized with all devices wired up and ready to execute guest code
> (QEMU needs only to unpause VCPUs to let guest execute its code),
> while the "preconfig" option pauses QEMU early before board specific init
> callback (machine_run_board_init) is executed and allows the configuration
> of machine parameters which will be used by board init code.
> 
> When early introspection/configuration is done, command 'exit-preconfig'
> should be used to exit RUN_STATE_PRECONFIG and transition to the next
> requested state (i.e. if -S is used then QEMU will pause the second
> time when board/device initialization is completed or start guest
> execution if -S isn't provided on CLI)
> 
> PS:
> Initially 'preconfig' is planned to be used for configuring numa
> topology depending on board specified possible cpus layout.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---

> +++ b/include/qapi/qmp/dispatch.h
> @@ -23,6 +23,7 @@ typedef enum QmpCommandOptions
>      QCO_NO_OPTIONS            =  0x0,
>      QCO_NO_SUCCESS_RESP       =  (1U << 0),
>      QCO_ALLOW_OOB             =  (1U << 1),
> +    QCO_ALLOWED_IN_PRECONFIG  =  (1U << 2),
>  } QmpCommandOptions;

I'm thinking patch 3 and 5 should be swapped, at least for
bisectability.  Otherwise, you have a window where --preconfig works
from the command line, but there is nothing registering any commands as
allowed in preconfig, which either lets qemu get stuck (if you actually
check for this flag) or lets you break things during preconfig (if you
don't check for this flag, because then ANY command can be run); I'll
see which as I read further.

> +++ b/qapi/misc.json
> @@ -1209,6 +1209,28 @@
>  { 'command': 'cont' }
>  
>  ##
> +# @exit-preconfig:
> +#
> +# Exit from "preconfig" state

[1]

> +#
> +# Since 2.13
> +#
> +# Returns: nothing
> +#
> +# Notes: Command makes QEMU exit from preconfig state and proceeds with
> +# VM initialization using configuration data provided on command line
> +# and via QMP monitor at preconfig state. Command is available only at
> +# preconfig state (i.e. if --preconfig command line option).

Wording suggestion:

Notes: This command makes QEMU exit the preconfig state and proceed with
VM initialization using configuration data provided on the command line
and via the QMP monitor during the preconfig stat.  The command is only
available during the preconfig state (i.e. when the --preconfig command
line option was in use).

Also, I wonder if you need a separate Notes: paragraph, or if this could
be used as the overall command description at [1].

> +#
> +# Example:
> +#
> +# -> { "execute": "exit-preconfig" }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'exit-preconfig' }
> +

Missing the 'allowed-in-preconfig' tag that isn't introduced until patch
5...

> @@ -101,6 +102,13 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, 
> QObject *request,
>          return NULL;
>      }
>  
> +    if (runstate_check(RUN_STATE_PRECONFIG) &&
> +        !(cmd->options & QCO_ALLOWED_IN_PRECONFIG)) {
> +        error_setg(errp, "The command '%s' isn't permitted in '%s' state",
> +                   cmd->name, RunState_str(RUN_STATE_PRECONFIG));
> +        return NULL;
> +    }
> +

...but you check the flag, therefore with just this patch, you can't run
exit-preconfig, and any command line using --preconfig is now hung.


> +++ b/qemu-options.hx
> @@ -3304,6 +3304,19 @@ STEXI
>  Run the emulation in single step mode.
>  ETEXI
>  
> +DEF("preconfig", 0, QEMU_OPTION_preconfig, \
> +    "--preconfig     pause QEMU before machine is initialized\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> address@hidden --preconfig
> address@hidden --preconfig
> +Pause QEMU for interactive configuration before the machine is created,
> +which allows querying and configuring properties that will affect
> +machine initialization. Use the QMP command 'cont' to exit the preconfig

s/cont/exit-preconfig/

> +state and move to the next state (ie. run guest if -S isn't used or
> +pause the second time if -S is used).

> address@hidden Managed start up options
> address@hidden Managed start up options
> +
> +In system mode emulation, it's possible to create VM in paused state using

s/create/create a/
s/in paused/in a paused/
s/using/using the/

> +-S command line option. In this state the machine is completely initialized
> +according to command line options and ready to execute VM code but VCPU 
> threads
> +are not executing any code. VM state in this paused state depends on way QEMU

s/VM/The VM/
s/on way/on the way/

> +was started. It could be in:
> address@hidden @asis
> address@hidden initial state (after reset/power on state)
> address@hidden with direct kernel loading initial state could be amended to 
> execute

Run-on; I'm less sure if this is what you intended, but:
s/loading initial/loading, the initial/

> +code loaded by QEMU in VM's RAM and with incoming migration

s/VM's/the VM's/

> address@hidden with incoming migration, initial state will by ammended with 
> the migrated

s/ammended/amended/

> +machine state after migration completes.
> address@hidden table
> +
> +This paused state is typically used by users to query machine state and/or
> +additionally configure machine (hotplug devices) in runtime before allowing

s/machine (hotplug/the machine (by hotplugging/

> +VM code to run.
> +
> +However at -S pause point it's impossible to configure options that affect

s/However at -S pause point/However, at the -S pause point,/

> +initial VM creation (like: -smp/-m/-numa ...) or cold plug devices. That's
> +when -preconfig command line option should be used. It allows pausing QEMU

s/-preconfig/the --preconfig/

> +before the initial VM creation, in a new preconfig state, where additional
> +queries and configuration can be performed via QMP before moving on to
> +the resulting configuration startup. In the preconfig state, QEMU only allows
> +a limited set of commands over the QMP monitor, where the commands do not
> +depend on an initialized machine, including but not limited to:
> address@hidden @asis
> address@hidden qmp_capabilities
> address@hidden query-qmp-schema
> address@hidden query-commands
> address@hidden query-status
> address@hidden exit-preconfig
> address@hidden table
> +The full list of commands is in QMP schema which could be queried with
> +query-qmp-schema, where commands supported at preconfig state have option
> +'allowed-in-preconfig' set to true.

This sentence is not true until later in the series, but I'm fine with
documentation patches going in out-of-order to avoid churn if they are
correct by the end of the series (of course, you may be reordering
things anyways, based on my earlier comments).

But overall this is looking a lot better, with documentation explaining
the need, and the new command (rather than overloading 'cont') for
switching back to the rest of the command line behavior.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature