[Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Igor Mammedov]

v3->v4:
  * replace 'runstates' list in  QMP command with a single
    boolean 'ption allowed-in-preconfig' like it's done with
    'allow-oob'. Which allows to simplify intrusive QAPI
    changes quite a lot. (Eric Blake <eblake@redhat.com>)
  * Make sure HMP is disbled for real, v3 was just printing
    error mesage but allowing command to be executed
    ("Dr. David Alan Gilbert" <dgilbert@redhat.com>)
  * Improve "cli: add -preconfig option" commit message,
    explain a bit more on semantics of new state/option.
  * ithe rest of minor fixups suggested at v3 review
    (Eric Blake <eblake@redhat.com>)
    PS:
       havn't impl. test for new option in
         tests/qapi-schema/qapi-schema-test.json yet,
       can do it on top if approach is acceptable.
v1->v3:
  * introduce PRECONFIG runstate with -preconfig option.
    it's cleaner to manage transitions and do checks
    than reusing existing PRELAUNCH state.
  * extend QAPI schema commands with 'runstates' keyword,
    so that it would be possible to specify in command definition
    when it is valid to execute.
    (python changes a bit hackery, since I have little to
     no idea how it should work)
  * add preconfig QMP and set-numa-node tests
  * make mutually exclusive -preconfig and -incoming options,
    for simplicity sake. Shouldn't be problem as target can
    be starter with pure CLI, since mapping on source is
    already known.
  * Drop HMP part and leave only QMP in preconfig state.


Series allows to configure NUMA mapping at runtime using QMP
interface. For that to happen it introduces a new '-preconfig' CLI option
which allows to pause QEMU before machine_init() is run and
adds new set-numa-node QMP command which in conjunction with
query-hotpluggable-cpus allows to configure NUMA mapping for cpus.

Later we can modify other commands to run early, for example device_add.
I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
stage it's considered hotplug already), so SPAPR had to work around the issue.

Example of configuration session:
$QEMU -smp 2 -preconfig ...

QMP:
# get CPUs layout for current target/machine/CLI
-> {'execute': 'query-hotpluggable-cpus' }  
<- {'return': [
       {'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 1}, ... },
       {'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 0}, ... }
   ]}

# configure 1st node
-> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 0 } }  
<- {'return': {}}
-> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',   
       'node-id': 0, 'core-id': 0, 'thread-id': 0, 'socket-id': 1, }
   }
<- {'return': {}}

# configure 2nd node
-> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 1 } }
-> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',  
       'node-id': 1, 'core-id': 0, 'thread-id': 0, 'socket-id': 0 }
   }
<- {'return': {}}

# [optional] verify configuration
-> {'execute': 'query-hotpluggable-cpus' }  
<- {'return': [
       {'props': {'core-id': 0, 'thread-id': 0, 'node-id': 0, 'socket-id': 1}, ... },
       {'props': {'core-id': 0, 'thread-id': 0, 'node-id': 1, 'socket-id': 0}, ... }
   ]}


Git tree:
    https://github.com/imammedo/qemu.git qmp_preconfig_v3

Ref to v1:
    https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03583.html
    Message-Id: <1508170976-96869-1-git-send-email-imammedo@redhat.com>
  
CC: eblake@redhat.com
CC: armbru@redhat.com
CC: ehabkost@redhat.com
CC: pkrempa@redhat.com
CC: david@gibson.dropbear.id.au
CC: peter.maydell@linaro.org
CC: pbonzini@redhat.com
CC: cohuck@redhat.com


Igor Mammedov (9):
  numa: postpone options post-processing till machine_run_board_init()
  numa: split out NumaOptions parsing into parse_NumaOptions()
  cli: add -preconfig option
  hmp: disable monitor in preconfig state
  qapi: introduce new cmd option "allowed-in-preconfig"
  tests: extend qmp test with preconfig checks
  qmp: permit query-hotpluggable-cpus in preconfig state
  qmp: add set-numa-node command
  tests: functional tests for QMP command set-numa-node

 include/qapi/qmp/dispatch.h    |  3 +-
 include/sysemu/numa.h          |  2 ++
 include/sysemu/sysemu.h        |  1 +
 tests/libqtest.h               |  9 ++++++
 docs/devel/qapi-code-gen.txt   | 10 ++++++-
 hw/core/machine.c              |  5 ++--
 monitor.c                      | 10 +++++--
 numa.c                         | 66 +++++++++++++++++++++++++++---------------
 qapi/introspect.json           |  6 +++-
 qapi/misc.json                 | 24 ++++++++++++---
 qapi/qmp-dispatch.c            |  8 +++++
 qapi/run-state.json            |  8 +++--
 qemu-options.hx                | 13 +++++++++
 qmp.c                          |  5 ++++
 scripts/qapi/commands.py       | 19 ++++++++----
 scripts/qapi/common.py         | 15 ++++++----
 scripts/qapi/doc.py            |  2 +-
 scripts/qapi/introspect.py     | 10 +++++--
 tests/libqtest.c               |  7 +++++
 tests/numa-test.c              | 61 ++++++++++++++++++++++++++++++++++++++
 tests/qapi-schema/test-qapi.py |  2 +-
 tests/qmp-test.c               | 37 +++++++++++++++++++++++
 vl.c                           | 35 +++++++++++++++++++++-
 23 files changed, 306 insertions(+), 52 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v4 1/9] numa: postpone options post-processing till machine_run_board_init()

[Igor Mammedov]

in preparation for numa options to being handled via QMP before
machine_run_board_init(), move final numa configuration checks
and processing to machine_run_board_init() so it could take into
account both CLI (via parse_numa_opts()) and QMP input

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  - remove duplicate qemu_opts_foreach() in numa_complete_configuration()
    that was causing non explicitly IDed node "-numa node" parsed twice.
---
 include/sysemu/numa.h |  1 +
 hw/core/machine.c     |  5 +++--
 numa.c                | 13 ++++++++-----
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index d99e547..21713b7 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -23,6 +23,7 @@ struct NumaNodeMem {
 
 extern NodeInfo numa_info[MAX_NODES];
 void parse_numa_opts(MachineState *ms);
+void numa_complete_configuration(MachineState *ms);
 void query_numa_node_mem(NumaNodeMem node_mem[]);
 extern QemuOptsList qemu_numa_opts;
 void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 5e2bbcd..e1d9482 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -715,7 +715,7 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
     return g_string_free(s, false);
 }
 
-static void machine_numa_finish_init(MachineState *machine)
+static void machine_numa_finish_cpu_init(MachineState *machine)
 {
     int i;
     bool default_mapping;
@@ -770,7 +770,8 @@ void machine_run_board_init(MachineState *machine)
     MachineClass *machine_class = MACHINE_GET_CLASS(machine);
 
     if (nb_numa_nodes) {
-        machine_numa_finish_init(machine);
+        numa_complete_configuration(machine);
+        machine_numa_finish_cpu_init(machine);
     }
 
     /* If the machine supports the valid_cpu_types check and the user
diff --git a/numa.c b/numa.c
index 398e2c9..126c649 100644
--- a/numa.c
+++ b/numa.c
@@ -338,15 +338,11 @@ void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
     nodes[i].node_mem = size - usedmem;
 }
 
-void parse_numa_opts(MachineState *ms)
+void numa_complete_configuration(MachineState *ms)
 {
     int i;
     MachineClass *mc = MACHINE_GET_CLASS(ms);
 
-    if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, NULL)) {
-        exit(1);
-    }
-
     /*
      * If memory hotplug is enabled (slots > 0) but without '-numa'
      * options explicitly on CLI, guestes will break.
@@ -433,6 +429,13 @@ void parse_numa_opts(MachineState *ms)
     }
 }
 
+void parse_numa_opts(MachineState *ms)
+{
+    if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, NULL)) {
+        exit(1);
+    }
+}
+
 void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
 {
     int node_id = object_property_get_int(OBJECT(dev), "node-id", &error_abort);
-- 
2.7.4

[Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Igor Mammedov]

it will allow to reuse parse_NumaOptions() for parsing
configuration commands received via QMP interface

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/sysemu/numa.h |  1 +
 numa.c                | 48 +++++++++++++++++++++++++++++-------------------
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 21713b7..7a0ae75 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -22,6 +22,7 @@ struct NumaNodeMem {
 };
 
 extern NodeInfo numa_info[MAX_NODES];
+int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
 void parse_numa_opts(MachineState *ms);
 void numa_complete_configuration(MachineState *ms);
 void query_numa_node_mem(NumaNodeMem node_mem[]);
diff --git a/numa.c b/numa.c
index 126c649..2b1d292 100644
--- a/numa.c
+++ b/numa.c
@@ -169,28 +169,11 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)
     have_numa_distance = true;
 }
 
-static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
+static
+void parse_NumaOptions(MachineState *ms, NumaOptions *object, Error **errp)
 {
-    NumaOptions *object = NULL;
-    MachineState *ms = opaque;
     Error *err = NULL;
 
-    {
-        Visitor *v = opts_visitor_new(opts);
-        visit_type_NumaOptions(v, NULL, &object, &err);
-        visit_free(v);
-    }
-
-    if (err) {
-        goto end;
-    }
-
-    /* Fix up legacy suffix-less format */
-    if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
-        const char *mem_str = qemu_opt_get(opts, "mem");
-        qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
-    }
-
     switch (object->type) {
     case NUMA_OPTIONS_TYPE_NODE:
         parse_numa_node(ms, &object->u.node, &err);
@@ -224,6 +207,33 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
     }
 
 end:
+    if (err) {
+        error_propagate(errp, err);
+    }
+}
+
+int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
+{
+    NumaOptions *object = NULL;
+    MachineState *ms = MACHINE(opaque);
+    Error *err = NULL;
+    Visitor *v = opts_visitor_new(opts);
+
+    visit_type_NumaOptions(v, NULL, &object, &err);
+    visit_free(v);
+    if (err) {
+        goto end;
+    }
+
+    /* Fix up legacy suffix-less format */
+    if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
+        const char *mem_str = qemu_opt_get(opts, "mem");
+        qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
+    }
+
+    parse_NumaOptions(ms, object, &err);
+
+end:
     qapi_free_NumaOptions(object);
     if (err) {
         error_report_err(err);
-- 
2.7.4

[Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Igor Mammedov]

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()

Intent is to allow management to query machine state and additionally
configure it using previous query results within one QEMU instance
(i.e. eliminate need to start QEMU twice, 1st to query board specific
parameters and 2nd for actual VM start using query results for
additional parameters).

New option complements -S option and could be used with or without
it. Difference is that -S pauses QEMU when machine is completely
build with all devices wired up and ready run (QEMU need only to
unpause CPUs to let guest execute its code).
And "preconfig" option pauses QEMU early before board specific init
callback (machine_run_board_init) is executed and will allow to
configure machine parameters which will be used by board init code.

When early introspection/configuration is done, command 'cont' 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 <imammedo@redhat.com>
---
v4:
  * Explain more on behaviour in commit message and use suggested
    wording in message and patch (Eric Blake <eblake@redhat.com>)
---
 include/sysemu/sysemu.h |  1 +
 qapi/run-state.json     |  5 ++++-
 qemu-options.hx         | 13 +++++++++++++
 qmp.c                   |  5 +++++
 vl.c                    | 35 ++++++++++++++++++++++++++++++++++-
 5 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 356bfdc..996bc38 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -66,6 +66,7 @@ typedef enum WakeupReason {
     QEMU_WAKEUP_REASON_OTHER,
 } WakeupReason;
 
+void qemu_exit_preconfig_request(void);
 void qemu_system_reset_request(ShutdownCause reason);
 void qemu_system_suspend_request(void);
 void qemu_register_suspend_notifier(Notifier *notifier);
diff --git a/qapi/run-state.json b/qapi/run-state.json
index 1c9fff3..ce846a5 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -49,12 +49,15 @@
 # @colo: guest is paused to save/restore VM state under colo checkpoint,
 #        VM can not get into this state unless colo capability is enabled
 #        for migration. (since 2.8)
+# @preconfig: QEMU is paused before board specific init callback is executed.
+#             The state is reachable only if -preconfig CLI option is used.
+#             (Since 2.12)
 ##
 { 'enum': 'RunState',
   'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
             'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
             'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
-            'guest-panicked', 'colo' ] }
+            'guest-panicked', 'colo', 'preconfig' ] }
 
 ##
 # @StatusInfo:
diff --git a/qemu-options.hx b/qemu-options.hx
index 6585058..7c8aaa5 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3302,6 +3302,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
+@item -preconfig
+@findex -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
+state and move to the next state (ie. run guest if -S isn't used or
+pause the second time is -S is used).
+ETEXI
+
 DEF("S", 0, QEMU_OPTION_S, \
     "-S              freeze CPU at startup (use 'c' to start execution)\n",
     QEMU_ARCH_ALL)
diff --git a/qmp.c b/qmp.c
index 8c7d1cc..b38090d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -166,6 +166,11 @@ void qmp_cont(Error **errp)
     BlockBackend *blk;
     Error *local_err = NULL;
 
+    if (runstate_check(RUN_STATE_PRECONFIG)) {
+        qemu_exit_preconfig_request();
+        return;
+    }
+
     /* if there is a dump in background, we should wait until the dump
      * finished */
     if (dump_in_progress()) {
diff --git a/vl.c b/vl.c
index 3ef04ce..69b1997 100644
--- a/vl.c
+++ b/vl.c
@@ -593,7 +593,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
 /***********************************************************/
 /* QEMU state */
 
-static RunState current_run_state = RUN_STATE_PRELAUNCH;
+static RunState current_run_state = RUN_STATE_PRECONFIG;
 
 /* We use RUN_STATE__MAX but any invalid value will do */
 static RunState vmstop_requested = RUN_STATE__MAX;
@@ -606,6 +606,9 @@ typedef struct {
 
 static const RunStateTransition runstate_transitions_def[] = {
     /*     from      ->     to      */
+    { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
+    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },
+
     { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
     { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
     { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
@@ -1629,6 +1632,7 @@ static pid_t shutdown_pid;
 static int powerdown_requested;
 static int debug_requested;
 static int suspend_requested;
+static bool preconfig_exit_requested = true;
 static WakeupReason wakeup_reason;
 static NotifierList powerdown_notifiers =
     NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
@@ -1713,6 +1717,11 @@ static int qemu_debug_requested(void)
     return r;
 }
 
+void qemu_exit_preconfig_request(void)
+{
+    preconfig_exit_requested = true;
+}
+
 /*
  * Reset the VM. Issue an event unless @reason is SHUTDOWN_CAUSE_NONE.
  */
@@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
     RunState r;
     ShutdownCause request;
 
+    if (preconfig_exit_requested) {
+        if (runstate_check(RUN_STATE_PRECONFIG)) {
+            runstate_set(RUN_STATE_PRELAUNCH);
+        }
+        preconfig_exit_requested = false;
+        return true;
+    }
     if (qemu_debug_requested()) {
         vm_stop(RUN_STATE_DEBUG);
     }
@@ -3697,6 +3713,14 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
+            case QEMU_OPTION_preconfig:
+                if (runstate_check(RUN_STATE_INMIGRATE)) {
+                    error_report("option can not be used with "
+                                 "-incoming option");
+                    exit(EXIT_FAILURE);
+                }
+                preconfig_exit_requested = false;
+                break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse_noisily(olist, "accel=kvm", false);
@@ -3902,6 +3926,11 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_incoming:
+                if (!preconfig_exit_requested) {
+                    error_report("option can not be used with "
+                                 "-preconfig option");
+                    exit(EXIT_FAILURE);
+                }
                 if (!incoming) {
                     runstate_set(RUN_STATE_INMIGRATE);
                 }
@@ -4594,6 +4623,10 @@ int main(int argc, char **argv, char **envp)
     }
     parse_numa_opts(current_machine);
 
+    /* do monitor/qmp handling at preconfig state if requested */
+    main_loop();
+
+    /* from here on runstate is RUN_STATE_PRELAUNCH */
     machine_run_board_init(current_machine);
 
     realtime_init();
-- 
2.7.4

[Qemu-devel] [PATCH v4 4/9] hmp: disable monitor in preconfig state

[Igor Mammedov]

Ban it for now, if someone would need it to work early,
one would have to implement checks if HMP command is valid
at preconfig state.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
  * v3 was only printing error but not preventing command execution,
    Fix it by returning after printing error message.
    ("Dr. David Alan Gilbert" <dgilbert@redhat.com>)
---
 monitor.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/monitor.c b/monitor.c
index a4417f2..ea0ca57 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3104,6 +3104,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
 
     trace_handle_hmp_command(mon, cmdline);
 
+    if (runstate_check(RUN_STATE_PRECONFIG)) {
+        monitor_printf(mon, "HMP not available in preconfig state\n");
+        return;
+    }
+
     cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
     if (!cmd) {
         return;
-- 
2.7.4

[Qemu-devel] [PATCH v4 5/9] qapi: introduce new cmd option "allowed-in-preconfig"

[Igor Mammedov]

New option will be used to allow commands, which are prepared/need
to run run in preconfig state. Other commands that should be able
to run in preconfig state, should be ammeded to not expect machine
in initialized state or deal with it.

For compatibility reasons, commands, that don't use new flag
'allowed-in-preconfig' explicitly, are not permited to run in
preconfig state but allowed in all other states like they used
to be.

Within this patch allow following commands in preconfig state:
   qmp_capabilities
   query-qmp-schema
   query-commands
   query-status
   cont
to allow qmp connection, basic introspection and moving to the next
state.

PS:
set-numa-node and query-hotpluggable-cpus will be enabled later in
a separate patch.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
  * replaces complex "universal" approach
     "[PATCH v3 5/9] QAPI: allow to specify valid runstates  per command"
    with a simpler new command flag "allowed-in-preconfig".
    (Eric Blake <eblake@redhat.com>)
---
 include/qapi/qmp/dispatch.h    |  3 ++-
 docs/devel/qapi-code-gen.txt   | 10 +++++++++-
 monitor.c                      |  5 +++--
 qapi/introspect.json           |  6 +++++-
 qapi/misc.json                 |  7 ++++---
 qapi/qmp-dispatch.c            |  8 ++++++++
 qapi/run-state.json            |  3 ++-
 scripts/qapi/commands.py       | 19 ++++++++++++++-----
 scripts/qapi/common.py         | 15 ++++++++++-----
 scripts/qapi/doc.py            |  2 +-
 scripts/qapi/introspect.py     | 10 ++++++++--
 tests/qapi-schema/test-qapi.py |  2 +-
 12 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1e694b5..2d02b75 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -21,7 +21,8 @@ typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
 typedef enum QmpCommandOptions
 {
     QCO_NO_OPTIONS = 0x0,
-    QCO_NO_SUCCESS_RESP = 0x1,
+    QCO_NO_SUCCESS_RESP = 1U << 0,
+    QCO_ALLOWED_IN_PRECONFIG =  1U << 1,
 } QmpCommandOptions;
 
 typedef struct QmpCommand
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 25b7180..170f15f 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -556,7 +556,8 @@ following example objects:
 
 Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
          '*returns': TYPE-NAME, '*boxed': true,
-         '*gen': false, '*success-response': false }
+         '*gen': false, '*success-response': false,
+         '*allowed-in-preconfig': true }
 
 Commands are defined by using a dictionary containing several members,
 where three members are most common.  The 'command' member is a
@@ -636,6 +637,13 @@ possible, the command expression should include the optional key
 'success-response' with boolean value false.  So far, only QGA makes
 use of this member.
 
+A command may use optional 'allowed-in-preconfig' key to permit
+its execution at early runtime configuration stage (preconfig runstate).
+If not specified then a command defaults to 'allowed-in-preconfig: false'.
+
+An example of declaring preconfig enabled command:
+ { 'command': 'qmp_capabilities',
+   'allowed-in-preconfig': true }
 
 === Events ===
 
diff --git a/monitor.c b/monitor.c
index ea0ca57..0adf220 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1016,7 +1016,7 @@ void monitor_init_qmp_commands(void)
 
     qmp_register_command(&qmp_commands, "query-qmp-schema",
                          qmp_query_qmp_schema,
-                         QCO_NO_OPTIONS);
+                         QCO_ALLOWED_IN_PRECONFIG);
     qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
                          QCO_NO_OPTIONS);
     qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
@@ -1026,7 +1026,8 @@ void monitor_init_qmp_commands(void)
 
     QTAILQ_INIT(&qmp_cap_negotiation_commands);
     qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
-                         qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
+                         qmp_marshal_qmp_capabilities,
+                         QCO_ALLOWED_IN_PRECONFIG);
 }
 
 void qmp_qmp_capabilities(Error **errp)
diff --git a/qapi/introspect.json b/qapi/introspect.json
index 5b3e6e9..05faded 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -259,12 +259,16 @@
 #
 # @ret-type: the name of the command's result type.
 #
+# @allowed-in-preconfig: command could be executed  in preconfig runstate,
+#                        default: 'false' (Since 2.12)
+#
 # TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
 #
 # Since: 2.5
 ##
 { 'struct': 'SchemaInfoCommand',
-  'data': { 'arg-type': 'str', 'ret-type': 'str' } }
+  'data': { 'arg-type': 'str', 'ret-type': 'str',
+            'allowed-in-preconfig': 'bool' } }
 
 ##
 # @SchemaInfoEvent:
diff --git a/qapi/misc.json b/qapi/misc.json
index bcd5d10..1f48d35 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -24,7 +24,7 @@
 # Since: 0.13
 #
 ##
-{ 'command': 'qmp_capabilities' }
+{ 'command': 'qmp_capabilities', 'allowed-in-preconfig': true }
 
 ##
 # @VersionTriple:
@@ -127,7 +127,8 @@
 # Note: This example has been shortened as the real response is too long.
 #
 ##
-{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
+{ 'command': 'query-commands', 'returns': ['CommandInfo'],
+  'allowed-in-preconfig': true }
 
 ##
 # @LostTickPolicy:
@@ -1180,7 +1181,7 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'cont' }
+{ 'command': 'cont', 'allowed-in-preconfig': true }
 
 ##
 # @system_wakeup:
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index e31ac4b..81dbd19 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -17,6 +17,7 @@
 #include "qapi/qmp/json-parser.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
+#include "sysemu/sysemu.h"
 
 static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
 {
@@ -92,6 +93,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;
+    }
+
     if (!qdict_haskey(dict, "arguments")) {
         args = qdict_new();
     } else {
diff --git a/qapi/run-state.json b/qapi/run-state.json
index ce846a5..174a2a3 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -94,7 +94,8 @@
 #                  "status": "running" } }
 #
 ##
-{ 'command': 'query-status', 'returns': 'StatusInfo' }
+{ 'command': 'query-status', 'returns': 'StatusInfo',
+  'allowed-in-preconfig': true }
 
 ##
 # @SHUTDOWN:
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 21a7e0d..bd8dc5e 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -193,10 +193,18 @@ out:
     return ret
 
 
-def gen_register_command(name, success_response):
-    options = 'QCO_NO_OPTIONS'
+def gen_register_command(name, success_response, allowed_in_preconfig):
+    options = []
+
     if not success_response:
-        options = 'QCO_NO_SUCCESS_RESP'
+        options += ['QCO_NO_SUCCESS_RESP']
+    if allowed_in_preconfig:
+        options += ['QCO_ALLOWED_IN_PRECONFIG']
+
+    if not options:
+        options = ['QCO_NO_OPTIONS']
+
+    options = " | ".join(options)
 
     ret = mcgen('''
     qmp_register_command(cmds, "%(name)s",
@@ -268,7 +276,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
         genc.add(gen_registry(self._regy, self._prefix))
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allowed_in_preconfig):
         if not gen:
             return
         self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
@@ -277,7 +285,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
             self._genc.add(gen_marshal_output(ret_type))
         self._genh.add(gen_marshal_decl(name))
         self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
-        self._regy += gen_register_command(name, success_response)
+        self._regy += gen_register_command(name, success_response,
+                                           allowed_in_preconfig)
 
 
 def gen_commands(schema, output_dir, prefix):
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 97e9060..e92f514 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -921,7 +921,8 @@ def check_exprs(exprs):
         elif 'command' in expr:
             meta = 'command'
             check_keys(expr_elem, 'command', [],
-                       ['data', 'returns', 'gen', 'success-response', 'boxed'])
+                       ['data', 'returns', 'gen', 'success-response',
+                        'boxed', 'allowed-in-preconfig'])
         elif 'event' in expr:
             meta = 'event'
             check_keys(expr_elem, 'event', [], ['data', 'boxed'])
@@ -1044,7 +1045,7 @@ class QAPISchemaVisitor(object):
         pass
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allowed_in_preconfig):
         pass
 
     def visit_event(self, name, info, arg_type, boxed):
@@ -1421,7 +1422,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
 
 class QAPISchemaCommand(QAPISchemaEntity):
     def __init__(self, name, info, doc, arg_type, ret_type,
-                 gen, success_response, boxed):
+                 gen, success_response, boxed, allowed_in_preconfig):
         QAPISchemaEntity.__init__(self, name, info, doc)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -1432,6 +1433,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         self.gen = gen
         self.success_response = success_response
         self.boxed = boxed
+        self.allowed_in_preconfig = allowed_in_preconfig
 
     def check(self, schema):
         if self._arg_type_name:
@@ -1455,7 +1457,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
     def visit(self, visitor):
         visitor.visit_command(self.name, self.info,
                               self.arg_type, self.ret_type,
-                              self.gen, self.success_response, self.boxed)
+                              self.gen, self.success_response, self.boxed,
+                              self.allowed_in_preconfig)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1674,6 +1677,7 @@ class QAPISchema(object):
         gen = expr.get('gen', True)
         success_response = expr.get('success-response', True)
         boxed = expr.get('boxed', False)
+        allowed_in_preconfig = expr.get('allowed-in-preconfig', False)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, doc, 'arg', self._make_members(data, info))
@@ -1681,7 +1685,8 @@ class QAPISchema(object):
             assert len(rets) == 1
             rets = self._make_array_type(rets[0], info)
         self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
-                                           gen, success_response, boxed))
+                                           gen, success_response, boxed,
+                                           allowed_in_preconfig))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 0ea68bf..464c89b 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -228,7 +228,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
                                body=texi_entity(doc, 'Members')))
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allowed_in_preconfig):
         doc = self.cur_doc
         if boxed:
             body = texi_body(doc)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index f66c397..fa7dc01 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -29,6 +29,11 @@ def to_json(obj, level=0):
                               to_json(obj[key], level + 1))
                 for key in sorted(obj.keys())]
         ret = '{' + ', '.join(elts) + '}'
+    elif isinstance(obj, bool):
+        if obj:
+            ret = 'true'
+        else:
+            ret = 'false'
     else:
         assert False                # not implemented
     if level == 1:
@@ -160,12 +165,13 @@ const char %(c_name)s[] = %(c_string)s;
                                     for m in variants.variants]})
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allowed_in_preconfig):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         self._gen_json(name, 'command',
                        {'arg-type': self._use_type(arg_type),
-                        'ret-type': self._use_type(ret_type)})
+                        'ret-type': self._use_type(ret_type),
+                        'allowed-in-preconfig': allowed_in_preconfig})
 
     def visit_event(self, name, info, arg_type, boxed):
         arg_type = arg_type or self._schema.the_empty_object_type
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 67e417e..7b22c90 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -42,7 +42,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         self._print_variants(variants)
 
     def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed):
+                      gen, success_response, boxed, allowed_in_preconfig):
         print('command %s %s -> %s' % \
               (name, arg_type and arg_type.name, ret_type and ret_type.name))
         print('   gen=%s success_response=%s boxed=%s' % \
-- 
2.7.4

[Qemu-devel] [PATCH v4 6/9] tests: extend qmp test with preconfig checks

[Igor Mammedov]

Add permission checks for commands at 'preconfig' stage.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
  * s/is_err()/qmp_rsp_is_err()/
  * return true even if 'error' doesn't contain 'desc'
    (Eric Blake <eblake@redhat.com>)
---
 tests/qmp-test.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 22445d9..cff880f 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -305,6 +305,49 @@ static void add_query_tests(QmpSchema *schema)
     }
 }
 
+static bool qmp_rsp_is_err(QDict *rsp)
+{
+    QDict *error = qdict_get_qdict(rsp, "error");
+    QDECREF(rsp);
+    return !!error;
+}
+
+static void test_qmp_preconfig(void)
+{
+    QDict *rsp, *ret;
+    QTestState *qs = qtest_startf("%s -preconfig", common_args);
+
+    /* preconfig state */
+    /* enabled commands, no error expected  */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'query-commands' }")));
+
+    /* forbidden commands, expected error */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'query-cpus' }")));
+
+    /* check that query-status returns preconfig state */
+    rsp = qtest_qmp(qs, "{ 'execute': 'query-status' }");
+    ret = qdict_get_qdict(rsp, "return");
+    g_assert(ret);
+    g_assert_cmpstr(qdict_get_try_str(ret, "status"), ==, "preconfig");
+    QDECREF(rsp);
+
+    /* exit preconfig state */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'cont' }")));
+    qtest_qmp_eventwait(qs, "RESUME");
+
+    /* check that query-status returns running state */
+    rsp = qtest_qmp(qs, "{ 'execute': 'query-status' }");
+    ret = qdict_get_qdict(rsp, "return");
+    g_assert(ret);
+    g_assert_cmpstr(qdict_get_try_str(ret, "status"), ==, "running");
+    QDECREF(rsp);
+
+    /* enabled commands, no error expected  */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'query-cpus' }")));
+
+    qtest_quit(qs);
+}
+
 int main(int argc, char *argv[])
 {
     QmpSchema schema;
@@ -315,6 +358,7 @@ int main(int argc, char *argv[])
     qtest_add_func("qmp/protocol", test_qmp_protocol);
     qmp_schema_init(&schema);
     add_query_tests(&schema);
+    qtest_add_func("qmp/preconfig", test_qmp_preconfig);
 
     ret = g_test_run();
 
-- 
2.7.4

[Qemu-devel] [PATCH v4 7/9] qmp: permit query-hotpluggable-cpus in preconfig state

[Igor Mammedov]

it will allow mgmt to query possible CPUs, which depends on
used machine(version)/-smp options, without restarting
QEMU and use results to configure numa mapping or adding
CPUs with device_add* later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>

PS:
*) device_add is not allowed to run at preconfig in this series
   but later it could be dealt with by injecting -device
   in preconfig state and letting existing -device handling
   to actually plug devices
---
 qapi/misc.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 1f48d35..691c980 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3196,7 +3196,8 @@
 #    ]}
 #
 ##
-{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
+{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'],
+             'allowed-in-preconfig': true }
 
 ##
 # @GuidInfo:
-- 
2.7.4

[Qemu-devel] [PATCH v4 8/9] qmp: add set-numa-node command

[Igor Mammedov]

Command is allowed to run only in preconfig stage and
will allow to configure numa mapping for CPUs depending
on possible CPUs layout (query-hotpluggable-cpus) for
given machine instance.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 numa.c         |  5 +++++
 qapi/misc.json | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/numa.c b/numa.c
index 2b1d292..02e2631 100644
--- a/numa.c
+++ b/numa.c
@@ -446,6 +446,11 @@ void parse_numa_opts(MachineState *ms)
     }
 }
 
+void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
+{
+    parse_NumaOptions(MACHINE(qdev_get_machine()), cmd, errp);
+}
+
 void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
 {
     int node_id = object_property_get_int(OBJECT(dev), "node-id", &error_abort);
diff --git a/qapi/misc.json b/qapi/misc.json
index 691c980..d023ac7 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3218,3 +3218,17 @@
 # Since: 2.9
 ##
 { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
+
+##
+# @set-numa-node:
+#
+# Runtime equivalent of '-numa' CLI option, available at
+# preconfigure stage to configure numa mapping before initializing
+# machine.
+#
+# Since 2.12
+##
+{ 'command': 'set-numa-node', 'boxed': true,
+  'data': 'NumaOptions',
+  'allowed-in-preconfig': true
+}
-- 
2.7.4

[Qemu-devel] [PATCH v4 9/9] tests: functional tests for QMP command set-numa-node

[Igor Mammedov]

 * start QEMU with 2 unmapped cpus,
 * while in preconfig state
    * add 2 numa nodes
    * assign cpus to them
 * exit preconfig and in running state check that cpus
   are mapped correctly.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
  * drop duplicate is_err() and reuse qmp_rsp_is_err() wich is moved
    to generic file libqtest.c. (Eric Blake <eblake@redhat.com>)
---
 tests/libqtest.h  |  9 ++++++++
 tests/libqtest.c  |  7 +++++++
 tests/numa-test.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/qmp-test.c  |  7 -------
 4 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 8111694..2e1f9b2 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -969,4 +969,13 @@ void qtest_qmp_device_add(const char *driver, const char *id, const char *fmt,
  */
 void qtest_qmp_device_del(const char *id);
 
+/**
+ * qmp_rsp_is_err:
+ * @rsp: QMP response to check for error
+ *
+ * Test @rsp for error and discard @rsp.
+ * Returns 'true' if there is error in @rsp and 'false' otherwise.
+ */
+bool qmp_rsp_is_err(QDict *rsp);
+
 #endif
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 13c9100..a7e91c5 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -1096,3 +1096,10 @@ void qtest_qmp_device_del(const char *id)
     QDECREF(response1);
     QDECREF(response2);
 }
+
+bool qmp_rsp_is_err(QDict *rsp)
+{
+    QDict *error = qdict_get_qdict(rsp, "error");
+    QDECREF(rsp);
+    return !!error;
+}
diff --git a/tests/numa-test.c b/tests/numa-test.c
index 68aca9c..4955927 100644
--- a/tests/numa-test.c
+++ b/tests/numa-test.c
@@ -260,6 +260,66 @@ static void aarch64_numa_cpu(const void *data)
     g_free(cli);
 }
 
+static void pc_dynamic_cpu_cfg(const void *data)
+{
+    QObject *e;
+    QDict *resp;
+    QList *cpus;
+    QTestState *qs;
+
+    qs = qtest_startf("%s %s", data ? (char *)data : "",
+                              "-nodefaults -preconfig -smp 2");
+
+    /* create 2 numa nodes */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'node', 'nodeid': 0 } }")));
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'node', 'nodeid': 1 } }")));
+
+    /* map 2 cpus in non default reverse order
+     * i.e socket1->node0, socket0->node1
+     */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'cpu', 'node-id': 0, 'socket-id': 1 } }")));
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+        " 'arguments': { 'type': 'cpu', 'node-id': 1, 'socket-id': 0 } }")));
+
+    /* let machine initialization to complete and run */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'cont' }")));
+    qtest_qmp_eventwait(qs, "RESUME");
+
+    /* check that CPUs are mapped as expected */
+    resp = qtest_qmp(qs, "{ 'execute': 'query-hotpluggable-cpus'}");
+    g_assert(qdict_haskey(resp, "return"));
+    cpus = qdict_get_qlist(resp, "return");
+    g_assert(cpus);
+    while ((e = qlist_pop(cpus))) {
+        const QDict *cpu, *props;
+        int64_t socket, node;
+
+        cpu = qobject_to_qdict(e);
+        g_assert(qdict_haskey(cpu, "props"));
+        props = qdict_get_qdict(cpu, "props");
+
+        g_assert(qdict_haskey(props, "node-id"));
+        node = qdict_get_int(props, "node-id");
+        g_assert(qdict_haskey(props, "socket-id"));
+        socket = qdict_get_int(props, "socket-id");
+
+        if (socket == 0) {
+            g_assert_cmpint(node, ==, 1);
+        } else if (socket == 1) {
+            g_assert_cmpint(node, ==, 0);
+        } else {
+            g_assert(false);
+        }
+        qobject_decref(e);
+    }
+    QDECREF(resp);
+
+    qtest_quit(qs);
+}
+
 int main(int argc, char **argv)
 {
     const char *args = NULL;
@@ -278,6 +338,7 @@ int main(int argc, char **argv)
 
     if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {
         qtest_add_data_func("/numa/pc/cpu/explicit", args, pc_numa_cpu);
+        qtest_add_data_func("/numa/pc/dynamic/cpu", args, pc_dynamic_cpu_cfg);
     }
 
     if (!strcmp(arch, "ppc64")) {
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index cff880f..fad5bc6 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -305,13 +305,6 @@ static void add_query_tests(QmpSchema *schema)
     }
 }
 
-static bool qmp_rsp_is_err(QDict *rsp)
-{
-    QDict *error = qdict_get_qdict(rsp, "error");
-    QDECREF(rsp);
-    return !!error;
-}
-
 static void test_qmp_preconfig(void)
 {
     QDict *rsp, *ret;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 1/9] numa: postpone options post-processing till machine_run_board_init()

[Eduardo Habkost]

On Mon, Mar 12, 2018 at 02:11:07PM +0100, Igor Mammedov wrote:
> in preparation for numa options to being handled via QMP before
> machine_run_board_init(), move final numa configuration checks
> and processing to machine_run_board_init() so it could take into
> account both CLI (via parse_numa_opts()) and QMP input
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   - remove duplicate qemu_opts_foreach() in numa_complete_configuration()
>     that was causing non explicitly IDed node "-numa node" parsed twice.

So, this moves initialization from parse_numa_opts() to
machine_run_board_init().

Let's see what can happen between those two functions:

Current code on main():

    parse_numa_opts(current_machine);

    machine_run_board_init(current_machine);

Wonderful.  :)

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Eduardo Habkost]

On Mon, Mar 12, 2018 at 02:11:08PM +0100, Igor Mammedov wrote:
> it will allow to reuse parse_NumaOptions() for parsing
> configuration commands received via QMP interface
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/sysemu/numa.h |  1 +
>  numa.c                | 48 +++++++++++++++++++++++++++++-------------------
>  2 files changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 21713b7..7a0ae75 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -22,6 +22,7 @@ struct NumaNodeMem {
>  };
>  
>  extern NodeInfo numa_info[MAX_NODES];
> +int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
>  void parse_numa_opts(MachineState *ms);
>  void numa_complete_configuration(MachineState *ms);
>  void query_numa_node_mem(NumaNodeMem node_mem[]);
> diff --git a/numa.c b/numa.c
> index 126c649..2b1d292 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -169,28 +169,11 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)
>      have_numa_distance = true;
>  }
>  
> -static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> +static
> +void parse_NumaOptions(MachineState *ms, NumaOptions *object, Error **errp)

I wonder if we should rename the parse_numa_{node,distance}()
functions to configure_numa_{node,distance}(), and this one
configure_numa().  These functions don't parse anything, anymore.


>  {
> -    NumaOptions *object = NULL;
> -    MachineState *ms = opaque;
>      Error *err = NULL;
>  
> -    {
> -        Visitor *v = opts_visitor_new(opts);
> -        visit_type_NumaOptions(v, NULL, &object, &err);
> -        visit_free(v);
> -    }
> -
> -    if (err) {
> -        goto end;
> -    }
> -
> -    /* Fix up legacy suffix-less format */
> -    if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
> -        const char *mem_str = qemu_opt_get(opts, "mem");
> -        qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
> -    }
> -
>      switch (object->type) {
>      case NUMA_OPTIONS_TYPE_NODE:
>          parse_numa_node(ms, &object->u.node, &err);
> @@ -224,6 +207,33 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
>      }
>  
>  end:
> +    if (err) {
> +        error_propagate(errp, err);
> +    }

"if (err)" is not necessary here.  See
scripts/coccinelle/error_propagate_null.cocci.


> +}
> +
> +int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    NumaOptions *object = NULL;
> +    MachineState *ms = MACHINE(opaque);
> +    Error *err = NULL;
> +    Visitor *v = opts_visitor_new(opts);
> +
> +    visit_type_NumaOptions(v, NULL, &object, &err);
> +    visit_free(v);
> +    if (err) {
> +        goto end;
> +    }
> +
> +    /* Fix up legacy suffix-less format */
> +    if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
> +        const char *mem_str = qemu_opt_get(opts, "mem");
> +        qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
> +    }
> +
> +    parse_NumaOptions(ms, object, &err);
> +
> +end:
>      qapi_free_NumaOptions(object);
>      if (err) {
>          error_report_err(err);

We can fix this one too while at it.

The rest of the patch looks good.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Eric Blake]

On 03/23/2018 03:42 PM, Eduardo Habkost wrote:
> On Mon, Mar 12, 2018 at 02:11:08PM +0100, Igor Mammedov wrote:
>> it will allow to reuse parse_NumaOptions() for parsing
>> configuration commands received via QMP interface
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---

>>   end:
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +    }
> 
> "if (err)" is not necessary here.  See
> scripts/coccinelle/error_propagate_null.cocci.
> 

>> +    parse_NumaOptions(ms, object, &err);
>> +
>> +end:
>>       qapi_free_NumaOptions(object);
>>       if (err) {
>>           error_report_err(err);
> 
> We can fix this one too while at it.

Hmm - this is the same script mentioned here:
https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06293.html

Except that patch didn't pick up this file.  Why is Coccinelle not 
seeing this?

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

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Eric Blake]

On 03/12/2018 08:11 AM, Igor Mammedov wrote:

I know you wrote this before softfreeze, but I'm only just now getting a 
chance to review. ...[1]

> 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()
> 
> Intent is to allow management to query machine state and additionally

s/Intent/The intent/

> configure it using previous query results within one QEMU instance
> (i.e. eliminate need to start QEMU twice, 1st to query board specific

s/need/the need/

> parameters and 2nd for actual VM start using query results for
> additional parameters).
> 
> New option complements -S option and could be used with or without

s/New/The new/


> it. Difference is that -S pauses QEMU when machine is completely

s/Difference/The difference/
s/when/when the/

> build with all devices wired up and ready run (QEMU need only to

s/build/built/
s/ready/ready to/

> unpause CPUs to let guest execute its code).
> And "preconfig" option pauses QEMU early before board specific init

s/. And/; while the/

> callback (machine_run_board_init) is executed and will allow to
> configure machine parameters which will be used by board init code.

s/allow to configure/allow the configuration of/

> 
> When early introspection/configuration is done, command 'cont' 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 <imammedo@redhat.com>
> ---
> v4:
>    * Explain more on behaviour in commit message and use suggested
>      wording in message and patch (Eric Blake <eblake@redhat.com>)

Well, I'm still coming up with wording tweaks, but it is getting better ;)

> ---
>   include/sysemu/sysemu.h |  1 +
>   qapi/run-state.json     |  5 ++++-
>   qemu-options.hx         | 13 +++++++++++++
>   qmp.c                   |  5 +++++
>   vl.c                    | 35 ++++++++++++++++++++++++++++++++++-
>   5 files changed, 57 insertions(+), 2 deletions(-)
> 

> +++ b/qapi/run-state.json
> @@ -49,12 +49,15 @@
>   # @colo: guest is paused to save/restore VM state under colo checkpoint,
>   #        VM can not get into this state unless colo capability is enabled
>   #        for migration. (since 2.8)
> +# @preconfig: QEMU is paused before board specific init callback is executed.
> +#             The state is reachable only if -preconfig CLI option is used.
> +#             (Since 2.12)

[1]... So are you still trying to cram this in 2.12 as a bugfix? It 
feels enough like a feature that at this point, you'll want to change 
that to 2.13 on your v5 spin.  (Probably a similar comment throughout 
the series, so I'll only mention it this once).

s/if -preconfig/if the --preconfig/

spelling --preconfig with two dashes may make sense; we have a 
bite-sized task that mentions that common options like -object/--object 
should prefer the two-dash form, at which point consistency where all 
our other options use the two-dash form may be worth doing.  But even if 
you stick with the one-dash form, inserting 'the' sounds better to a 
native speaker.

>   ##
>   { 'enum': 'RunState',
>     'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
>               'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
>               'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> -            'guest-panicked', 'colo' ] }
> +            'guest-panicked', 'colo', 'preconfig' ] }
>   
>   ##
>   # @StatusInfo:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 6585058..7c8aaa5 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3302,6 +3302,19 @@ STEXI
>   Run the emulation in single step mode.
>   ETEXI
>   
> +DEF("preconfig", 0, QEMU_OPTION_preconfig, \
> +    "-preconfig      pause QEMU before machine is initialized\n",

More places for two-dash spelling consideration.

> +    QEMU_ARCH_ALL)
> +STEXI
> +@item -preconfig
> +@findex -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
> +state and move to the next state (ie. run guest if -S isn't used or
> +pause the second time is -S is used).

s/is -S/if -S/

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

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Eduardo Habkost]

On Mon, Mar 12, 2018 at 02:11:09PM +0100, 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()
> 
> Intent is to allow management to query machine state and additionally
> configure it using previous query results within one QEMU instance
> (i.e. eliminate need to start QEMU twice, 1st to query board specific
> parameters and 2nd for actual VM start using query results for
> additional parameters).
> 
> New option complements -S option and could be used with or without
> it. Difference is that -S pauses QEMU when machine is completely
> build with all devices wired up and ready run (QEMU need only to
> unpause CPUs to let guest execute its code).
> And "preconfig" option pauses QEMU early before board specific init
> callback (machine_run_board_init) is executed and will allow to
> configure machine parameters which will be used by board init code.
> 
> When early introspection/configuration is done, command 'cont' 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 <imammedo@redhat.com>

TL;DR: I was against this approach of adding a new "preconfig"
state and thought "-S" ought to be enough, but I'm now convinced
this is the best option we have.


Long version:

So, I was skeptical of this approach initially, because I thought
"machine->init() was run" and "machine->init() was not run yet"
is supposed to be internal QEMU state that no external component
should care about at all, because the vCPUs are not running yet.

In other words, if vCPUS were not started yet, we should be able
to reconfigure anything, and "-S" ought to be enough to what we
want.

...in theory.  In practice this is messy:

Currently initialization works this way:

  void vm_start()  /* this is delayed if -S is used */
  {
      resume_all_vcpus();
  }

  void qmp_cont()  /* "cont" command */
  {
      /* ... */
      vm_start();
  }
  
  void main()
  {
      /* ... */
      machine_run_board_init()
      if (autostart) {  /* -S option sets autotstart = 0 */
          vm_start();
      }
      main_loop();  /* QMP becomes available here */
  }

Then we would have to either do this:

  void vm_start()
  {
      machine_run_board_init()  /* <---- HERE */
      resume_all_vcpus();
  }

  void main()
  {
      /* ... */
      /* machine_run_board_init() moved from here */
      if (autostart) {
          vm_start();
      }
      main_loop();
  }

...and fix every single QMP command to not break if
machine_run_board_init() wasn't called yet.

I don't think that's feasible.


Or we could do this:

  void vm_start()
  {
      configure_numa()  /* <---- HERE */
      resume_all_vcpus();
  }

  void main()
  {
      /* ... */
      machine_run_board_init();
      if (autostart) {
          vm_start();
      }
      main_loop();
  }

...and slowly move code from machine_run_board_init() to
vm_start() (like configure_numa() above).

That's how I expected us to implement the NUMA QMP configuration
stuff.

But, really, the data and ordering dependencies we have in
machine initialization is insane, and simply moving
configure_numa() after machine_run_board_init() would require
moving almost all of machine_run_board_init() inside vm_start().

In practice this would be more complex than moving
machine_run_board_init() completely inside vm_start().  I don't
think that's feasible.

So I'm OK with your approach.

Now I will review the actual code in a separate e-mail.  :)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Eduardo Habkost]

On Fri, Mar 23, 2018 at 03:49:38PM -0500, Eric Blake wrote:
> On 03/23/2018 03:42 PM, Eduardo Habkost wrote:
> > On Mon, Mar 12, 2018 at 02:11:08PM +0100, Igor Mammedov wrote:
> > > it will allow to reuse parse_NumaOptions() for parsing
> > > configuration commands received via QMP interface
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> 
> > >   end:
> > > +    if (err) {
> > > +        error_propagate(errp, err);
> > > +    }
> > 
> > "if (err)" is not necessary here.  See
> > scripts/coccinelle/error_propagate_null.cocci.
> > 
> 
> > > +    parse_NumaOptions(ms, object, &err);
> > > +
> > > +end:
> > >       qapi_free_NumaOptions(object);
> > >       if (err) {
> > >           error_report_err(err);
> > 
> > We can fix this one too while at it.
> 
> Hmm - this is the same script mentioned here:
> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06293.html
> 
> Except that patch didn't pick up this file.  Why is Coccinelle not seeing
> this?

I don't know.  I've seen Coccinelle being confused by some of our
preprocessor magic before, and in those cases it simply skipped
some files.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/9] qapi: introduce new cmd option "allowed-in-preconfig"

[Eric Blake]

On 03/12/2018 08:11 AM, Igor Mammedov wrote:
> New option will be used to allow commands, which are prepared/need
> to run run in preconfig state. Other commands that should be able

s/run run in/run, during/

> to run in preconfig state, should be ammeded to not expect machine

s/ammeded/amended/

> in initialized state or deal with it.
> 
> For compatibility reasons, commands, that don't use new flag

s/commands,/commands/

> 'allowed-in-preconfig' explicitly, are not permited to run in

s/explicitly,/explicitly/
s/permited/permitted/

> preconfig state but allowed in all other states like they used
> to be.
> 
> Within this patch allow following commands in preconfig state:
>     qmp_capabilities
>     query-qmp-schema
>     query-commands
>     query-status
>     cont
> to allow qmp connection, basic introspection and moving to the next
> state.

Looks like a reasonable list.  Maybe also query-command-line-options 
should be here?

> 
> PS:
> set-numa-node and query-hotpluggable-cpus will be enabled later in
> a separate patch.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>    * replaces complex "universal" approach
>       "[PATCH v3 5/9] QAPI: allow to specify valid runstates  per command"
>      with a simpler new command flag "allowed-in-preconfig".
>      (Eric Blake <eblake@redhat.com>)

Thanks; it looks a lot more maintainable now.  However, you need to 
rebase, now that 'allow-oob' has already landed.

> +++ b/qapi/introspect.json
> @@ -259,12 +259,16 @@
>   #
>   # @ret-type: the name of the command's result type.
>   #
> +# @allowed-in-preconfig: command could be executed  in preconfig runstate,

s/could/can/
double space before in

> +#                        default: 'false' (Since 2.12)

/me must resist the urge to call out softfreeze ;)

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

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Eduardo Habkost]

On Mon, Mar 12, 2018 at 02:11:09PM +0100, 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()
> 
> Intent is to allow management to query machine state and additionally
> configure it using previous query results within one QEMU instance
> (i.e. eliminate need to start QEMU twice, 1st to query board specific
> parameters and 2nd for actual VM start using query results for
> additional parameters).
> 
> New option complements -S option and could be used with or without
> it. Difference is that -S pauses QEMU when machine is completely
> build with all devices wired up and ready run (QEMU need only to
> unpause CPUs to let guest execute its code).
> And "preconfig" option pauses QEMU early before board specific init
> callback (machine_run_board_init) is executed and will allow to
> configure machine parameters which will be used by board init code.
> 
> When early introspection/configuration is done, command 'cont' 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 <imammedo@redhat.com>
> ---
> v4:
>   * Explain more on behaviour in commit message and use suggested
>     wording in message and patch (Eric Blake <eblake@redhat.com>)
> ---
>  include/sysemu/sysemu.h |  1 +
>  qapi/run-state.json     |  5 ++++-
>  qemu-options.hx         | 13 +++++++++++++
>  qmp.c                   |  5 +++++
>  vl.c                    | 35 ++++++++++++++++++++++++++++++++++-
>  5 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 356bfdc..996bc38 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -66,6 +66,7 @@ typedef enum WakeupReason {
>      QEMU_WAKEUP_REASON_OTHER,
>  } WakeupReason;
>  
> +void qemu_exit_preconfig_request(void);
>  void qemu_system_reset_request(ShutdownCause reason);
>  void qemu_system_suspend_request(void);
>  void qemu_register_suspend_notifier(Notifier *notifier);
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 1c9fff3..ce846a5 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -49,12 +49,15 @@
>  # @colo: guest is paused to save/restore VM state under colo checkpoint,
>  #        VM can not get into this state unless colo capability is enabled
>  #        for migration. (since 2.8)
> +# @preconfig: QEMU is paused before board specific init callback is executed.
> +#             The state is reachable only if -preconfig CLI option is used.
> +#             (Since 2.12)
>  ##
>  { 'enum': 'RunState',
>    'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
>              'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
>              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> -            'guest-panicked', 'colo' ] }
> +            'guest-panicked', 'colo', 'preconfig' ] }
>  
>  ##
>  # @StatusInfo:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 6585058..7c8aaa5 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3302,6 +3302,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
> +@item -preconfig
> +@findex -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
> +state and move to the next state (ie. run guest if -S isn't used or
> +pause the second time is -S is used).
> +ETEXI
> +
>  DEF("S", 0, QEMU_OPTION_S, \
>      "-S              freeze CPU at startup (use 'c' to start execution)\n",
>      QEMU_ARCH_ALL)
> diff --git a/qmp.c b/qmp.c
> index 8c7d1cc..b38090d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -166,6 +166,11 @@ void qmp_cont(Error **errp)
>      BlockBackend *blk;
>      Error *local_err = NULL;
>  
> +    if (runstate_check(RUN_STATE_PRECONFIG)) {
> +        qemu_exit_preconfig_request();
> +        return;
> +    }
> +
>      /* if there is a dump in background, we should wait until the dump
>       * finished */
>      if (dump_in_progress()) {
> diff --git a/vl.c b/vl.c
> index 3ef04ce..69b1997 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -593,7 +593,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
>  /***********************************************************/
>  /* QEMU state */
>  
> -static RunState current_run_state = RUN_STATE_PRELAUNCH;
> +static RunState current_run_state = RUN_STATE_PRECONFIG;
>  
>  /* We use RUN_STATE__MAX but any invalid value will do */
>  static RunState vmstop_requested = RUN_STATE__MAX;
> @@ -606,6 +606,9 @@ typedef struct {
>  
>  static const RunStateTransition runstate_transitions_def[] = {
>      /*     from      ->     to      */
> +    { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> +    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },

Don't this mean -preconfig and -incoming could work together?

> +
>      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
>      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
>      { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
> @@ -1629,6 +1632,7 @@ static pid_t shutdown_pid;
>  static int powerdown_requested;
>  static int debug_requested;
>  static int suspend_requested;
> +static bool preconfig_exit_requested = true;
>  static WakeupReason wakeup_reason;
>  static NotifierList powerdown_notifiers =
>      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> @@ -1713,6 +1717,11 @@ static int qemu_debug_requested(void)
>      return r;
>  }
>  
> +void qemu_exit_preconfig_request(void)
> +{
> +    preconfig_exit_requested = true;
> +}
> +
>  /*
>   * Reset the VM. Issue an event unless @reason is SHUTDOWN_CAUSE_NONE.
>   */
> @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
>      RunState r;
>      ShutdownCause request;
>  
> +    if (preconfig_exit_requested) {
> +        if (runstate_check(RUN_STATE_PRECONFIG)) {

Is it possible to have preconfig_exit_request set outside of
RUN_STATE_PRECONFIG?  When and why?

> +            runstate_set(RUN_STATE_PRELAUNCH);
> +        }
> +        preconfig_exit_requested = false;
> +        return true;
> +    }
>      if (qemu_debug_requested()) {
>          vm_stop(RUN_STATE_DEBUG);
>      }
> @@ -3697,6 +3713,14 @@ int main(int argc, char **argv, char **envp)
>                      exit(1);
>                  }
>                  break;
> +            case QEMU_OPTION_preconfig:
> +                if (runstate_check(RUN_STATE_INMIGRATE)) {
> +                    error_report("option can not be used with "
> +                                 "-incoming option");
> +                    exit(EXIT_FAILURE);
> +                }

So -incoming changes runstate as soon as the option is parsed?

Ouch.

I would rather not rely on that behavior and just do
"if (incoming)".

Why exactly it's not possible to use -incoming with -preconfig?


> +                preconfig_exit_requested = false;
> +                break;
>              case QEMU_OPTION_enable_kvm:
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_parse_noisily(olist, "accel=kvm", false);
> @@ -3902,6 +3926,11 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_incoming:
> +                if (!preconfig_exit_requested) {
> +                    error_report("option can not be used with "
> +                                 "-preconfig option");
> +                    exit(EXIT_FAILURE);
> +                }

Instead of reimplementing the same check in two separate places,
why not validate options and check for (incoming && preconfig)
after the option parsing loop?

>                  if (!incoming) {
>                      runstate_set(RUN_STATE_INMIGRATE);
>                  }
> @@ -4594,6 +4623,10 @@ int main(int argc, char **argv, char **envp)
>      }
>      parse_numa_opts(current_machine);
>  
> +    /* do monitor/qmp handling at preconfig state if requested */
> +    main_loop();

Wouldn't it be simpler to do "if (!preconfig) { main_loop(); }"
instead of entering main_loop() just to exit immediately?

> +
> +    /* from here on runstate is RUN_STATE_PRELAUNCH */
>      machine_run_board_init(current_machine);
>  
>      realtime_init();
> -- 
> 2.7.4
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 4/9] hmp: disable monitor in preconfig state

[Eduardo Habkost]

On Mon, Mar 12, 2018 at 02:11:10PM +0100, Igor Mammedov wrote:
> Ban it for now, if someone would need it to work early,
> one would have to implement checks if HMP command is valid
> at preconfig state.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
>   * v3 was only printing error but not preventing command execution,
>     Fix it by returning after printing error message.
>     ("Dr. David Alan Gilbert" <dgilbert@redhat.com>)
> ---
>  monitor.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/monitor.c b/monitor.c
> index a4417f2..ea0ca57 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3104,6 +3104,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
>  
>      trace_handle_hmp_command(mon, cmdline);
>  
> +    if (runstate_check(RUN_STATE_PRECONFIG)) {
> +        monitor_printf(mon, "HMP not available in preconfig state\n");
> +        return;

Not even the "cont" command?  It would be useful for testing
-preconfig.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/9] qapi: introduce new cmd option "allowed-in-preconfig"

[Eduardo Habkost]

On Mon, Mar 12, 2018 at 02:11:11PM +0100, Igor Mammedov wrote:
> New option will be used to allow commands, which are prepared/need
> to run run in preconfig state. Other commands that should be able
> to run in preconfig state, should be ammeded to not expect machine
> in initialized state or deal with it.
> 
> For compatibility reasons, commands, that don't use new flag
> 'allowed-in-preconfig' explicitly, are not permited to run in
> preconfig state but allowed in all other states like they used
> to be.
> 
> Within this patch allow following commands in preconfig state:
>    qmp_capabilities
>    query-qmp-schema
>    query-commands
>    query-status
>    cont
> to allow qmp connection, basic introspection and moving to the next
> state.
> 
> PS:
> set-numa-node and query-hotpluggable-cpus will be enabled later in
> a separate patch.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I didn't review the code yet, but:

Shouldn't this be applied before patch 3/9, for bisectability?
Otherwise it will be very easy to crash QEMU after applying patch
3/9.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Laurent Vivier]

On 23/03/2018 21:49, Eric Blake wrote:
> On 03/23/2018 03:42 PM, Eduardo Habkost wrote:
>> On Mon, Mar 12, 2018 at 02:11:08PM +0100, Igor Mammedov wrote:
>>> it will allow to reuse parse_NumaOptions() for parsing
>>> configuration commands received via QMP interface
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
> 
>>>   end:
>>> +    if (err) {
>>> +        error_propagate(errp, err);
>>> +    }
>>
>> "if (err)" is not necessary here.  See
>> scripts/coccinelle/error_propagate_null.cocci.
>>
> 
>>> +    parse_NumaOptions(ms, object, &err);
>>> +
>>> +end:
>>>       qapi_free_NumaOptions(object);
>>>       if (err) {
>>>           error_report_err(err);
>>
>> We can fix this one too while at it.
> 
> Hmm - this is the same script mentioned here:
> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06293.html
> 
> Except that patch didn't pick up this file.  Why is Coccinelle not
> seeing this?
> 

The script only catch error_propagate(), not error_report_err(). And
error_report_err() doesn't check if err is NULL.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Eric Blake]

On 03/26/2018 03:38 AM, Laurent Vivier wrote:

>>>>    end:
>>>> +    if (err) {
>>>> +        error_propagate(errp, err);
>>>> +    }
>>>
>>> "if (err)" is not necessary here.  See
>>> scripts/coccinelle/error_propagate_null.cocci.
>>>
>>
>>>> +    parse_NumaOptions(ms, object, &err);
>>>> +
>>>> +end:
>>>>        qapi_free_NumaOptions(object);
>>>>        if (err) {
>>>>            error_report_err(err);
>>>
>>> We can fix this one too while at it.
>>
>> Hmm - this is the same script mentioned here:
>> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06293.html
>>
>> Except that patch didn't pick up this file.  Why is Coccinelle not
>> seeing this?
>>
> 
> The script only catch error_propagate(), not error_report_err(). And
> error_report_err() doesn't check if err is NULL.

Aha - chalk it up to reviewing late in the day; I saw an 'if (err)' but 
didn't pay close attention to what was being guarded in the two 
different conditionals.

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

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Igor Mammedov]

On Fri, 23 Mar 2018 17:42:18 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Mar 12, 2018 at 02:11:08PM +0100, Igor Mammedov wrote:
> > it will allow to reuse parse_NumaOptions() for parsing
> > configuration commands received via QMP interface
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/sysemu/numa.h |  1 +
> >  numa.c                | 48 +++++++++++++++++++++++++++++-------------------
> >  2 files changed, 30 insertions(+), 19 deletions(-)
> > 
> > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > index 21713b7..7a0ae75 100644
> > --- a/include/sysemu/numa.h
> > +++ b/include/sysemu/numa.h
> > @@ -22,6 +22,7 @@ struct NumaNodeMem {
> >  };
> >  
> >  extern NodeInfo numa_info[MAX_NODES];
> > +int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
> >  void parse_numa_opts(MachineState *ms);
> >  void numa_complete_configuration(MachineState *ms);
> >  void query_numa_node_mem(NumaNodeMem node_mem[]);
> > diff --git a/numa.c b/numa.c
> > index 126c649..2b1d292 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -169,28 +169,11 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)
> >      have_numa_distance = true;
> >  }
> >  
> > -static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> > +static
> > +void parse_NumaOptions(MachineState *ms, NumaOptions *object, Error **errp)  
> 
> I wonder if we should rename the parse_numa_{node,distance}()
> functions to configure_numa_{node,distance}(), and this one
> configure_numa().  These functions don't parse anything, anymore.
I'd preffer to do it in another patch on top of this series
(added it my TODO list)


> >  {
> > -    NumaOptions *object = NULL;
> > -    MachineState *ms = opaque;
> >      Error *err = NULL;
> >  
> > -    {
> > -        Visitor *v = opts_visitor_new(opts);
> > -        visit_type_NumaOptions(v, NULL, &object, &err);
> > -        visit_free(v);
> > -    }
> > -
> > -    if (err) {
> > -        goto end;
> > -    }
> > -
> > -    /* Fix up legacy suffix-less format */
> > -    if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
> > -        const char *mem_str = qemu_opt_get(opts, "mem");
> > -        qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
> > -    }
> > -
> >      switch (object->type) {
> >      case NUMA_OPTIONS_TYPE_NODE:
> >          parse_numa_node(ms, &object->u.node, &err);
> > @@ -224,6 +207,33 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> >      }
> >  
> >  end:
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +    }  
> 
> "if (err)" is not necessary here.  See
> scripts/coccinelle/error_propagate_null.cocci.
fixed
 
> > +}
> > +
> > +int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> > +{
> > +    NumaOptions *object = NULL;
> > +    MachineState *ms = MACHINE(opaque);
> > +    Error *err = NULL;
> > +    Visitor *v = opts_visitor_new(opts);
> > +
> > +    visit_type_NumaOptions(v, NULL, &object, &err);
> > +    visit_free(v);
> > +    if (err) {
> > +        goto end;
> > +    }
> > +
> > +    /* Fix up legacy suffix-less format */
> > +    if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
> > +        const char *mem_str = qemu_opt_get(opts, "mem");
> > +        qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
> > +    }
> > +
> > +    parse_NumaOptions(ms, object, &err);
> > +
> > +end:
> >      qapi_free_NumaOptions(object);
> >      if (err) {
> >          error_report_err(err);  
> 
> We can fix this one too while at it.
error_report_err() doesn't check for NULL value,
'if(err)' is needed here
 
> The rest of the patch looks good.
> 

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Igor Mammedov]

On Fri, 23 Mar 2018 18:25:08 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:
[...]
> > diff --git a/vl.c b/vl.c
> > index 3ef04ce..69b1997 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -593,7 +593,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
> >  /***********************************************************/
> >  /* QEMU state */
> >  
> > -static RunState current_run_state = RUN_STATE_PRELAUNCH;
> > +static RunState current_run_state = RUN_STATE_PRECONFIG;
> >  
> >  /* We use RUN_STATE__MAX but any invalid value will do */
> >  static RunState vmstop_requested = RUN_STATE__MAX;
> > @@ -606,6 +606,9 @@ typedef struct {
> >  
> >  static const RunStateTransition runstate_transitions_def[] = {
> >      /*     from      ->     to      */
> > +    { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> > +    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },  
> 
> Don't this mean -preconfig and -incoming could work together?
theoretically yes, but its not the reason why this transition is here.
It's mimicking existing approach where initial state
   { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
were allowed to move to the next possible (including RUN_STATE_INMIGRATE)

> > +
> >      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
> >      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> >      { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
> > @@ -1629,6 +1632,7 @@ static pid_t shutdown_pid;
> >  static int powerdown_requested;
> >  static int debug_requested;
> >  static int suspend_requested;
> > +static bool preconfig_exit_requested = true;
> >  static WakeupReason wakeup_reason;
> >  static NotifierList powerdown_notifiers =
> >      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> > @@ -1713,6 +1717,11 @@ static int qemu_debug_requested(void)
> >      return r;
> >  }
> >  
> > +void qemu_exit_preconfig_request(void)
> > +{
> > +    preconfig_exit_requested = true;
> > +}
> > +
> >  /*
> >   * Reset the VM. Issue an event unless @reason is SHUTDOWN_CAUSE_NONE.
> >   */
> > @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
> >      RunState r;
> >      ShutdownCause request;
> >  
> > +    if (preconfig_exit_requested) {
> > +        if (runstate_check(RUN_STATE_PRECONFIG)) {  
> 
> Is it possible to have preconfig_exit_request set outside of
> RUN_STATE_PRECONFIG?  When and why?
preconfig_exit_requested is initialized with TRUE and
in combo with '-inmigrate' we need this runstate check.
it's the same as it was with
 { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
which I probably should remove (I need to check it though)

> > +            runstate_set(RUN_STATE_PRELAUNCH);
> > +        }
> > +        preconfig_exit_requested = false;
> > +        return true;
> > +    }
> >      if (qemu_debug_requested()) {
> >          vm_stop(RUN_STATE_DEBUG);
> >      }
> > @@ -3697,6 +3713,14 @@ int main(int argc, char **argv, char **envp)
> >                      exit(1);
> >                  }
> >                  break;
> > +            case QEMU_OPTION_preconfig:
> > +                if (runstate_check(RUN_STATE_INMIGRATE)) {
> > +                    error_report("option can not be used with "
> > +                                 "-incoming option");
> > +                    exit(EXIT_FAILURE);
> > +                }  
> 
> So -incoming changes runstate as soon as the option is parsed?
> 
> Ouch.
yep and it's rather fragile (it's well out of scope of
this series to re-factor this, so I'm not changing it here)

> I would rather not rely on that behavior and just do
> "if (incoming)".
> 
> Why exactly it's not possible to use -incoming with -preconfig?
there are 2 reasons why I made options mutually exclusive
1. (excuse ) '-incoming' is an option with non explicit side effects
   on other parts of code. It's hard to predict behavior
   of preconfig commands in combination with inmigrate.
   I wouldn't try to touch/change anything related to it
   in this series.
   If we need to change how option is handled, it should
   be separate series that focuses on it.
2. (main reason) is to expose as minimal interface
   as possible. It's easier to extend/modify it future if
   necessary than cut it down after it was introduced.

   Not counting [1], I don't see a reason to permit
   'preconfig' while migration is in progress.
   Configuration commands that where used during 'preconfig'
   stage on source side, should use corresponding CLI options
   on target side. (it's the same behavior as with hotplugged
   devices, keeping migration work-flow the same)

In short I'd prefer to keep restriction until there will be
a real usecase for combo to work together.

> > +                preconfig_exit_requested = false;
> > +                break;
> >              case QEMU_OPTION_enable_kvm:
> >                  olist = qemu_find_opts("machine");
> >                  qemu_opts_parse_noisily(olist, "accel=kvm", false);
> > @@ -3902,6 +3926,11 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  break;
> >              case QEMU_OPTION_incoming:
> > +                if (!preconfig_exit_requested) {
> > +                    error_report("option can not be used with "
> > +                                 "-preconfig option");
> > +                    exit(EXIT_FAILURE);
> > +                }  
> 
> Instead of reimplementing the same check in two separate places,
> why not validate options and check for (incoming && preconfig)
> after the option parsing loop?
it could be done this way, but then we would lose specialized
error message.
Even though the way I did it, it is more code but that code
is close to related options and allows for specialized error
message in the order options are parsed.
Also it's easier to read as one doesn't have to jump around,
all error handling is in place where where an option is parsed.
But it's more style question, so if you prefer
(incoming && preconfig) approach I can easily switch to it
on respin.

> >                  if (!incoming) {
> >                      runstate_set(RUN_STATE_INMIGRATE);
> >                  }
> > @@ -4594,6 +4623,10 @@ int main(int argc, char **argv, char **envp)
> >      }
> >      parse_numa_opts(current_machine);
> >  
> > +    /* do monitor/qmp handling at preconfig state if requested */
> > +    main_loop();  
> 
> Wouldn't it be simpler to do "if (!preconfig) { main_loop(); }"
> instead of entering main_loop() just to exit immediately?
The thought didn't cross my mind, it might work and more readable
as one doesn't have to jump into main_loop() to find out that
it would exit immediately.
I'll try to it on respin.

> > +
> > +    /* from here on runstate is RUN_STATE_PRELAUNCH */
> >      machine_run_board_init(current_machine);
> >  
> >      realtime_init();
> > -- 
> > 2.7.4
> > 
> >   
> 

Re: [Qemu-devel] [PATCH v4 4/9] hmp: disable monitor in preconfig state

[Igor Mammedov]

On Fri, 23 Mar 2018 18:27:32 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Mar 12, 2018 at 02:11:10PM +0100, Igor Mammedov wrote:
> > Ban it for now, if someone would need it to work early,
> > one would have to implement checks if HMP command is valid
> > at preconfig state.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v4:
> >   * v3 was only printing error but not preventing command execution,
> >     Fix it by returning after printing error message.
> >     ("Dr. David Alan Gilbert" <dgilbert@redhat.com>)
> > ---
> >  monitor.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index a4417f2..ea0ca57 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3104,6 +3104,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
> >  
> >      trace_handle_hmp_command(mon, cmdline);
> >  
> > +    if (runstate_check(RUN_STATE_PRECONFIG)) {
> > +        monitor_printf(mon, "HMP not available in preconfig state\n");
> > +        return;  
> 
> Not even the "cont" command?  It would be useful for testing
> -preconfig.
As someone already said on the list it's very easy to test with
QMP nowdays, just use qmp-shell for that.
So if someone isn't willing to learn to use QMP, one can write
HMP part with proper white-listing.

I can extend error message like this:

"HMP not available in preconfig state, use QMP instead\n"

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Igor Mammedov]

On Tue, 27 Mar 2018 17:05:41 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Fri, 23 Mar 2018 18:25:08 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:  
> [...]
[...]
> > > @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
> > >      RunState r;
> > >      ShutdownCause request;
> > >  
> > > +    if (preconfig_exit_requested) {
> > > +        if (runstate_check(RUN_STATE_PRECONFIG)) {    
> > 
> > Is it possible to have preconfig_exit_request set outside of
> > RUN_STATE_PRECONFIG?  When and why?  
> preconfig_exit_requested is initialized with TRUE and
> in combo with '-inmigrate' we need this runstate check.
> it's the same as it was with
>  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> which I probably should remove (I need to check it though)
[...]

> > > @@ -4594,6 +4623,10 @@ int main(int argc, char **argv, char **envp)
> > >      }
> > >      parse_numa_opts(current_machine);
> > >  
> > > +    /* do monitor/qmp handling at preconfig state if requested */
> > > +    main_loop();    
> > 
> > Wouldn't it be simpler to do "if (!preconfig) { main_loop(); }"
> > instead of entering main_loop() just to exit immediately?  
> The thought didn't cross my mind, it might work and more readable
> as one doesn't have to jump into main_loop() to find out that
> it would exit immediately.
> I'll try to it on respin.
Well doing as suggested end ups more messy:

    @@static bool main_loop_should_exit(void)
    ...
    if (preconfig_exit_requested) {
        runstate_set(RUN_STATE_PRELAUNCH);                                        
        return true;
    }
   
    @@main
    /* do monitor/qmp handling at preconfig state if requested */
    if (!preconfig_exit_requested) {
        main_loop();
    } else if (runstate_check(RUN_STATE_PRECONFIG)) {
        runstate_set(RUN_STATE_PRELAUNCH);
    }
    preconfig_exit_requested = false;
    ...

I'd prefer original v4 approach, where only main_loop_should_exit()
has to deal with state transitions and book-keeping.

[...]

Re: [Qemu-devel] [PATCH v4 5/9] qapi: introduce new cmd option "allowed-in-preconfig"

[Igor Mammedov]

On Fri, 23 Mar 2018 18:28:37 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Mar 12, 2018 at 02:11:11PM +0100, Igor Mammedov wrote:
> > New option will be used to allow commands, which are prepared/need
> > to run run in preconfig state. Other commands that should be able
> > to run in preconfig state, should be ammeded to not expect machine
> > in initialized state or deal with it.
> > 
> > For compatibility reasons, commands, that don't use new flag
> > 'allowed-in-preconfig' explicitly, are not permited to run in
> > preconfig state but allowed in all other states like they used
> > to be.
> > 
> > Within this patch allow following commands in preconfig state:
> >    qmp_capabilities
> >    query-qmp-schema
> >    query-commands
> >    query-status
> >    cont
> > to allow qmp connection, basic introspection and moving to the next
> > state.
> > 
> > PS:
> > set-numa-node and query-hotpluggable-cpus will be enabled later in
> > a separate patch.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> I didn't review the code yet, but:
> 
> Shouldn't this be applied before patch 3/9, for bisectability?
> Otherwise it will be very easy to crash QEMU after applying patch
> 3/9.
no, it isn't going to work.
This patch depends on RUN_STATE_PRECONFIG that is introduced in 3/9.

It could be fine to merge into 3/9 during merge, but then history
wise it would be difficult to read it later with 2 big and mostly
separate changes within one patch.

Considering -preconfig if off by default it shouldn't affect
bisectability in general so I'd keep current patch order.

Re: [Qemu-devel] [PATCH v4 5/9] qapi: introduce new cmd option "allowed-in-preconfig"

[Igor Mammedov]

On Fri, 23 Mar 2018 16:11:53 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 03/12/2018 08:11 AM, Igor Mammedov wrote:
[...]
> 
> > preconfig state but allowed in all other states like they used
> > to be.
> > 
> > Within this patch allow following commands in preconfig state:
> >     qmp_capabilities
> >     query-qmp-schema
> >     query-commands
> >     query-status
> >     cont
> > to allow qmp connection, basic introspection and moving to the next
> > state.  
> 
> Looks like a reasonable list.  Maybe also query-command-line-options 
> should be here?
added
 
> > 
> > PS:
> > set-numa-node and query-hotpluggable-cpus will be enabled later in
> > a separate patch.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v4:
> >    * replaces complex "universal" approach
> >       "[PATCH v3 5/9] QAPI: allow to specify valid runstates  per command"
> >      with a simpler new command flag "allowed-in-preconfig".
> >      (Eric Blake <eblake@redhat.com>)  
> 
> Thanks; it looks a lot more maintainable now.  However, you need to 
> rebase, now that 'allow-oob' has already landed.
rebased

[...]

All other comments are addressed as well

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Eduardo Habkost]

On Tue, Mar 27, 2018 at 03:08:27PM +0200, Igor Mammedov wrote:
> On Fri, 23 Mar 2018 17:42:18 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Mar 12, 2018 at 02:11:08PM +0100, Igor Mammedov wrote:
> > > it will allow to reuse parse_NumaOptions() for parsing
> > > configuration commands received via QMP interface
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  include/sysemu/numa.h |  1 +
> > >  numa.c                | 48 +++++++++++++++++++++++++++++-------------------
> > >  2 files changed, 30 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > index 21713b7..7a0ae75 100644
> > > --- a/include/sysemu/numa.h
> > > +++ b/include/sysemu/numa.h
> > > @@ -22,6 +22,7 @@ struct NumaNodeMem {
> > >  };
> > >  
> > >  extern NodeInfo numa_info[MAX_NODES];
> > > +int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
> > >  void parse_numa_opts(MachineState *ms);
> > >  void numa_complete_configuration(MachineState *ms);
> > >  void query_numa_node_mem(NumaNodeMem node_mem[]);
> > > diff --git a/numa.c b/numa.c
> > > index 126c649..2b1d292 100644
> > > --- a/numa.c
> > > +++ b/numa.c
> > > @@ -169,28 +169,11 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)
> > >      have_numa_distance = true;
> > >  }
> > >  
> > > -static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> > > +static
> > > +void parse_NumaOptions(MachineState *ms, NumaOptions *object, Error **errp)  
> > 
> > I wonder if we should rename the parse_numa_{node,distance}()
> > functions to configure_numa_{node,distance}(), and this one
> > configure_numa().  These functions don't parse anything, anymore.
> I'd preffer to do it in another patch on top of this series
> (added it my TODO list)

I agree with renaming parse_numa*() later, but the new function
you are creating can have a more reasonable name as it doesn't
parse anything.


> 
> 
> > >  {
> > > -    NumaOptions *object = NULL;
> > > -    MachineState *ms = opaque;
> > >      Error *err = NULL;
> > >  
> > > -    {
> > > -        Visitor *v = opts_visitor_new(opts);
> > > -        visit_type_NumaOptions(v, NULL, &object, &err);
> > > -        visit_free(v);
> > > -    }
> > > -
> > > -    if (err) {
> > > -        goto end;
> > > -    }
> > > -
> > > -    /* Fix up legacy suffix-less format */
> > > -    if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
> > > -        const char *mem_str = qemu_opt_get(opts, "mem");
> > > -        qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
> > > -    }
> > > -
> > >      switch (object->type) {
> > >      case NUMA_OPTIONS_TYPE_NODE:
> > >          parse_numa_node(ms, &object->u.node, &err);
> > > @@ -224,6 +207,33 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> > >      }
> > >  
> > >  end:
> > > +    if (err) {
> > > +        error_propagate(errp, err);
> > > +    }  
> > 
> > "if (err)" is not necessary here.  See
> > scripts/coccinelle/error_propagate_null.cocci.
> fixed

Thanks!

>  
> > > +}
> > > +
> > > +int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> > > +{
> > > +    NumaOptions *object = NULL;
> > > +    MachineState *ms = MACHINE(opaque);
> > > +    Error *err = NULL;
> > > +    Visitor *v = opts_visitor_new(opts);
> > > +
> > > +    visit_type_NumaOptions(v, NULL, &object, &err);
> > > +    visit_free(v);
> > > +    if (err) {
> > > +        goto end;
> > > +    }
> > > +
> > > +    /* Fix up legacy suffix-less format */
> > > +    if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
> > > +        const char *mem_str = qemu_opt_get(opts, "mem");
> > > +        qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
> > > +    }
> > > +
> > > +    parse_NumaOptions(ms, object, &err);
> > > +
> > > +end:
> > >      qapi_free_NumaOptions(object);
> > >      if (err) {
> > >          error_report_err(err);  
> > 
> > We can fix this one too while at it.
> error_report_err() doesn't check for NULL value,
> 'if(err)' is needed here

Sorry, my mistake.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 4/9] hmp: disable monitor in preconfig state

[Eduardo Habkost]

On Wed, Mar 28, 2018 at 01:16:53PM +0200, Igor Mammedov wrote:
> On Fri, 23 Mar 2018 18:27:32 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Mar 12, 2018 at 02:11:10PM +0100, Igor Mammedov wrote:
> > > Ban it for now, if someone would need it to work early,
> > > one would have to implement checks if HMP command is valid
> > > at preconfig state.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > v4:
> > >   * v3 was only printing error but not preventing command execution,
> > >     Fix it by returning after printing error message.
> > >     ("Dr. David Alan Gilbert" <dgilbert@redhat.com>)
> > > ---
> > >  monitor.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/monitor.c b/monitor.c
> > > index a4417f2..ea0ca57 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -3104,6 +3104,11 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline)
> > >  
> > >      trace_handle_hmp_command(mon, cmdline);
> > >  
> > > +    if (runstate_check(RUN_STATE_PRECONFIG)) {
> > > +        monitor_printf(mon, "HMP not available in preconfig state\n");
> > > +        return;  
> > 
> > Not even the "cont" command?  It would be useful for testing
> > -preconfig.
> As someone already said on the list it's very easy to test with
> QMP nowdays, just use qmp-shell for that.
> So if someone isn't willing to learn to use QMP, one can write
> HMP part with proper white-listing.
> 
> I can extend error message like this:
> 
> "HMP not available in preconfig state, use QMP instead\n"

Sounds good enough to me.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Eduardo Habkost]

On Tue, Mar 27, 2018 at 05:05:41PM +0200, Igor Mammedov wrote:
> On Fri, 23 Mar 2018 18:25:08 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:
> [...]
> > > diff --git a/vl.c b/vl.c
> > > index 3ef04ce..69b1997 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -593,7 +593,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
> > >  /***********************************************************/
> > >  /* QEMU state */
> > >  
> > > -static RunState current_run_state = RUN_STATE_PRELAUNCH;
> > > +static RunState current_run_state = RUN_STATE_PRECONFIG;
> > >  
> > >  /* We use RUN_STATE__MAX but any invalid value will do */
> > >  static RunState vmstop_requested = RUN_STATE__MAX;
> > > @@ -606,6 +606,9 @@ typedef struct {
> > >  
> > >  static const RunStateTransition runstate_transitions_def[] = {
> > >      /*     from      ->     to      */
> > > +    { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> > > +    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },  
> > 
> > Don't this mean -preconfig and -incoming could work together?
> theoretically yes, but its not the reason why this transition is here.
> It's mimicking existing approach where initial state
>    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> were allowed to move to the next possible (including RUN_STATE_INMIGRATE)

I still don't get it.  Where this definition of "next possible"
comes from?  If -incoming and -preconfig don't work together, why
is PRECONFIG -> INMIGRATE migration considered possible?


> 
> > > +
> > >      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
> > >      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> > >      { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
> > > @@ -1629,6 +1632,7 @@ static pid_t shutdown_pid;
> > >  static int powerdown_requested;
> > >  static int debug_requested;
> > >  static int suspend_requested;
> > > +static bool preconfig_exit_requested = true;
> > >  static WakeupReason wakeup_reason;
> > >  static NotifierList powerdown_notifiers =
> > >      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> > > @@ -1713,6 +1717,11 @@ static int qemu_debug_requested(void)
> > >      return r;
> > >  }
> > >  
> > > +void qemu_exit_preconfig_request(void)
> > > +{
> > > +    preconfig_exit_requested = true;
> > > +}
> > > +
> > >  /*
> > >   * Reset the VM. Issue an event unless @reason is SHUTDOWN_CAUSE_NONE.
> > >   */
> > > @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
> > >      RunState r;
> > >      ShutdownCause request;
> > >  
> > > +    if (preconfig_exit_requested) {
> > > +        if (runstate_check(RUN_STATE_PRECONFIG)) {  
> > 
> > Is it possible to have preconfig_exit_request set outside of
> > RUN_STATE_PRECONFIG?  When and why?
> preconfig_exit_requested is initialized with TRUE and
> in combo with '-inmigrate' we need this runstate check.

I think this now makes sense to me.  It still looks confusing,
but I don't have a better suggestion right now.

Except...

Why exactly do you need to use main_loop() and
main_loop_should_exit() for the preconfig loop?  What about a
separate preconfig_loop() and preconfig_loop_should_exit()
function?


> it's the same as it was with
>  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> which I probably should remove (I need to check it though)
> 
> > > +            runstate_set(RUN_STATE_PRELAUNCH);
> > > +        }
> > > +        preconfig_exit_requested = false;

What happens if we don't set preconfig_exit_requested=false here?


> > > +        return true;
> > > +    }
> > >      if (qemu_debug_requested()) {
> > >          vm_stop(RUN_STATE_DEBUG);
> > >      }
> > > @@ -3697,6 +3713,14 @@ int main(int argc, char **argv, char **envp)
> > >                      exit(1);
> > >                  }
> > >                  break;
> > > +            case QEMU_OPTION_preconfig:
> > > +                if (runstate_check(RUN_STATE_INMIGRATE)) {
> > > +                    error_report("option can not be used with "
> > > +                                 "-incoming option");
> > > +                    exit(EXIT_FAILURE);
> > > +                }  
> > 
> > So -incoming changes runstate as soon as the option is parsed?
> > 
> > Ouch.
> yep and it's rather fragile (it's well out of scope of
> this series to re-factor this, so I'm not changing it here)
> 
> > I would rather not rely on that behavior and just do
> > "if (incoming)".
> > 
> > Why exactly it's not possible to use -incoming with -preconfig?
> there are 2 reasons why I made options mutually exclusive
> 1. (excuse ) '-incoming' is an option with non explicit side effects
>    on other parts of code. It's hard to predict behavior
>    of preconfig commands in combination with inmigrate.
>    I wouldn't try to touch/change anything related to it
>    in this series.
>    If we need to change how option is handled, it should
>    be separate series that focuses on it.
> 2. (main reason) is to expose as minimal interface
>    as possible. It's easier to extend/modify it future if
>    necessary than cut it down after it was introduced.
> 
>    Not counting [1], I don't see a reason to permit
>    'preconfig' while migration is in progress.
>    Configuration commands that where used during 'preconfig'
>    stage on source side, should use corresponding CLI options
>    on target side. (it's the same behavior as with hotplugged
>    devices, keeping migration work-flow the same)
> 
> In short I'd prefer to keep restriction until there will be
> a real usecase for combo to work together.

I understand the reasons, but I think we already have an
important use case: live-migrating a VM with non-trivial NUMA
config (that needs -preconfig).  Don't we?


> 
> > > +                preconfig_exit_requested = false;
> > > +                break;
> > >              case QEMU_OPTION_enable_kvm:
> > >                  olist = qemu_find_opts("machine");
> > >                  qemu_opts_parse_noisily(olist, "accel=kvm", false);
> > > @@ -3902,6 +3926,11 @@ int main(int argc, char **argv, char **envp)
> > >                  }
> > >                  break;
> > >              case QEMU_OPTION_incoming:
> > > +                if (!preconfig_exit_requested) {
> > > +                    error_report("option can not be used with "
> > > +                                 "-preconfig option");
> > > +                    exit(EXIT_FAILURE);
> > > +                }  
> > 
> > Instead of reimplementing the same check in two separate places,
> > why not validate options and check for (incoming && preconfig)
> > after the option parsing loop?
> it could be done this way, but then we would lose specialized
> error message.
> Even though the way I did it, it is more code but that code
> is close to related options and allows for specialized error
> message in the order options are parsed.

What do you mean by specialized user message?  Both have exactly
the same information: "-incoming and -preconfig can't be used
together", just written in a different way.


> Also it's easier to read as one doesn't have to jump around,
> all error handling is in place where where an option is parsed.
> But it's more style question, so if you prefer
> (incoming && preconfig) approach I can easily switch to it
> on respin.

I would prefer that.  We already have lots of configuration
validation after the option parsing loop, including but not
limited to:

    error_report("Invalid SMP CPUs %d. The min CPUs "
                 "supported by machine '%s' is %d", smp_cpus,
                 machine_class->name, machine_class->min_cpus);
    error_report("Invalid SMP CPUs %d. The max CPUs "
                 "supported by machine '%s' is %d", max_cpus,
                 machine_class->name, machine_class->max_cpus);
    error_report("-nographic cannot be used with -daemonize");
    error_report("curses display cannot be used with -daemonize");
    error_report("-no-frame, -alt-grab and -ctrl-grab are only valid "
                 "for SDL, ignoring option");
    error_report("-no-quit is only valid for GTK and SDL, "
                 "ignoring option");
    error_report("OpenGL is not supported by the display");
    error_report("OpenGL support is disabled");
    error_report("-append only allowed with -kernel option");
    error_report("-initrd only allowed with -kernel option");
    error_report("-icount is not allowed with hardware virtualization");
    error_report("at most 2047 MB RAM can be simulated");


I agree with the argument that validation of config options
should be done all in the same place.  But I disagree that the
body of the option parsing loop is the right place for that.

> 
> > >                  if (!incoming) {
> > >                      runstate_set(RUN_STATE_INMIGRATE);
> > >                  }
> > > @@ -4594,6 +4623,10 @@ int main(int argc, char **argv, char **envp)
> > >      }
> > >      parse_numa_opts(current_machine);
> > >  
> > > +    /* do monitor/qmp handling at preconfig state if requested */
> > > +    main_loop();  
> > 
> > Wouldn't it be simpler to do "if (!preconfig) { main_loop(); }"
> > instead of entering main_loop() just to exit immediately?
> The thought didn't cross my mind, it might work and more readable
> as one doesn't have to jump into main_loop() to find out that
> it would exit immediately.
> I'll try to it on respin.

Thanks!

> 
> > > +
> > > +    /* from here on runstate is RUN_STATE_PRELAUNCH */
> > >      machine_run_board_init(current_machine);
> > >  
> > >      realtime_init();
> > > -- 
> > > 2.7.4
> > > 
> > >   
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Eduardo Habkost]

On Wed, Mar 28, 2018 at 01:48:35PM +0200, Igor Mammedov wrote:
> On Tue, 27 Mar 2018 17:05:41 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Fri, 23 Mar 2018 18:25:08 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:  
> > [...]
> [...]
> > > > @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
> > > >      RunState r;
> > > >      ShutdownCause request;
> > > >  
> > > > +    if (preconfig_exit_requested) {
> > > > +        if (runstate_check(RUN_STATE_PRECONFIG)) {    
> > > 
> > > Is it possible to have preconfig_exit_request set outside of
> > > RUN_STATE_PRECONFIG?  When and why?  
> > preconfig_exit_requested is initialized with TRUE and
> > in combo with '-inmigrate' we need this runstate check.
> > it's the same as it was with
> >  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > which I probably should remove (I need to check it though)
> [...]
> 
> > > > @@ -4594,6 +4623,10 @@ int main(int argc, char **argv, char **envp)
> > > >      }
> > > >      parse_numa_opts(current_machine);
> > > >  
> > > > +    /* do monitor/qmp handling at preconfig state if requested */
> > > > +    main_loop();    
> > > 
> > > Wouldn't it be simpler to do "if (!preconfig) { main_loop(); }"
> > > instead of entering main_loop() just to exit immediately?  
> > The thought didn't cross my mind, it might work and more readable
> > as one doesn't have to jump into main_loop() to find out that
> > it would exit immediately.
> > I'll try to it on respin.
> Well doing as suggested end ups more messy:
> 
>     @@static bool main_loop_should_exit(void)
>     ...
>     if (preconfig_exit_requested) {
>         runstate_set(RUN_STATE_PRELAUNCH);                                        
>         return true;
>     }
>    
>     @@main
>     /* do monitor/qmp handling at preconfig state if requested */
>     if (!preconfig_exit_requested) {
>         main_loop();
>     } else if (runstate_check(RUN_STATE_PRECONFIG)) {
>         runstate_set(RUN_STATE_PRELAUNCH);
>     }

This doesn't make sense to me.  Why would we enter
RUN_STATE_PRECONFIG state if -preconfig is not used at all?


>     preconfig_exit_requested = false;
>     ...
> 
> I'd prefer original v4 approach, where only main_loop_should_exit()
> has to deal with state transitions and book-keeping.

If the above is unavoidable, I agree.  But I still don't
understand we have to enter PRECONFIG state if the user didn't
specify -preconfig.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/9] qapi: introduce new cmd option "allowed-in-preconfig"

[Eduardo Habkost]

On Wed, Mar 28, 2018 at 02:29:57PM +0200, Igor Mammedov wrote:
> On Fri, 23 Mar 2018 18:28:37 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Mar 12, 2018 at 02:11:11PM +0100, Igor Mammedov wrote:
> > > New option will be used to allow commands, which are prepared/need
> > > to run run in preconfig state. Other commands that should be able
> > > to run in preconfig state, should be ammeded to not expect machine
> > > in initialized state or deal with it.
> > > 
> > > For compatibility reasons, commands, that don't use new flag
> > > 'allowed-in-preconfig' explicitly, are not permited to run in
> > > preconfig state but allowed in all other states like they used
> > > to be.
> > > 
> > > Within this patch allow following commands in preconfig state:
> > >    qmp_capabilities
> > >    query-qmp-schema
> > >    query-commands
> > >    query-status
> > >    cont
> > > to allow qmp connection, basic introspection and moving to the next
> > > state.
> > > 
> > > PS:
> > > set-numa-node and query-hotpluggable-cpus will be enabled later in
> > > a separate patch.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > I didn't review the code yet, but:
> > 
> > Shouldn't this be applied before patch 3/9, for bisectability?
> > Otherwise it will be very easy to crash QEMU after applying patch
> > 3/9.
> no, it isn't going to work.
> This patch depends on RUN_STATE_PRECONFIG that is introduced in 3/9.
> 
> It could be fine to merge into 3/9 during merge, but then history
> wise it would be difficult to read it later with 2 big and mostly
> separate changes within one patch.

Yeah, I don't think squashing would be the right answer.

> 
> Considering -preconfig if off by default it shouldn't affect
> bisectability in general so I'd keep current patch order.

Well, it would affect bisectability if debugging a crash that
happens using -preconfig.

The only hunk in this patch that really depends on patch 3/9
seems to be:

  @@ -92,6 +93,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;
  +    }
  +
       if (!qdict_haskey(dict, "arguments")) {
           args = qdict_new();
       } else {
  

What about moving it to patch 3/9?


Or, an alternative is to move the following hunk from patch 3/9 to this patch:

  diff --git a/qapi/run-state.json b/qapi/run-state.json
  index 1c9fff3aef..ce846a570e 100644
  --- a/qapi/run-state.json
  +++ b/qapi/run-state.json
  @@ -49,12 +49,15 @@
   # @colo: guest is paused to save/restore VM state under colo checkpoint,
   #        VM can not get into this state unless colo capability is enabled
   #        for migration. (since 2.8)
  +# @preconfig: QEMU is paused before board specific init callback is executed.
  +#             The state is reachable only if -preconfig CLI option is used.
  +#             (Since 2.12)
   ##
   { 'enum': 'RunState',
     'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
               'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
               'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
  -            'guest-panicked', 'colo' ] }
  +            'guest-panicked', 'colo', 'preconfig' ] }
  
   ##
   # @StatusInfo:

Which could be an interesting idea, because the QAPI schema
changes would be all grouped inside a single patch, and then
followed by the actual implementation of the -preconfig option.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 5/9] qapi: introduce new cmd option "allowed-in-preconfig"

[Igor Mammedov]

On Wed, 28 Mar 2018 16:30:16 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Mar 28, 2018 at 02:29:57PM +0200, Igor Mammedov wrote:
> > On Fri, 23 Mar 2018 18:28:37 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Mon, Mar 12, 2018 at 02:11:11PM +0100, Igor Mammedov wrote:  
> > > > New option will be used to allow commands, which are prepared/need
> > > > to run run in preconfig state. Other commands that should be able
> > > > to run in preconfig state, should be ammeded to not expect machine
> > > > in initialized state or deal with it.
> > > > 
> > > > For compatibility reasons, commands, that don't use new flag
> > > > 'allowed-in-preconfig' explicitly, are not permited to run in
> > > > preconfig state but allowed in all other states like they used
> > > > to be.
> > > > 
> > > > Within this patch allow following commands in preconfig state:
> > > >    qmp_capabilities
> > > >    query-qmp-schema
> > > >    query-commands
> > > >    query-status
> > > >    cont
> > > > to allow qmp connection, basic introspection and moving to the next
> > > > state.
> > > > 
> > > > PS:
> > > > set-numa-node and query-hotpluggable-cpus will be enabled later in
> > > > a separate patch.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>    
> > > 
> > > I didn't review the code yet, but:
> > > 
> > > Shouldn't this be applied before patch 3/9, for bisectability?
> > > Otherwise it will be very easy to crash QEMU after applying patch
> > > 3/9.  
> > no, it isn't going to work.
> > This patch depends on RUN_STATE_PRECONFIG that is introduced in 3/9.
> > 
> > It could be fine to merge into 3/9 during merge, but then history
> > wise it would be difficult to read it later with 2 big and mostly
> > separate changes within one patch.  
> 
> Yeah, I don't think squashing would be the right answer.
> 
> > 
> > Considering -preconfig if off by default it shouldn't affect
> > bisectability in general so I'd keep current patch order.  
> 
> Well, it would affect bisectability if debugging a crash that
> happens using -preconfig.
> 
> The only hunk in this patch that really depends on patch 3/9
> seems to be:
> 
>   @@ -92,6 +93,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;
>   +    }
>   +
>        if (!qdict_haskey(dict, "arguments")) {
>            args = qdict_new();
>        } else {
>   
> 
> What about moving it to patch 3/9?
By itself it won't work but with moving following hunk with it

@@ -21,7 +21,8 @@ typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
 typedef enum QmpCommandOptions
 {
     QCO_NO_OPTIONS = 0x0,
-    QCO_NO_SUCCESS_RESP = 0x1,
+    QCO_NO_SUCCESS_RESP = 1U << 0,
+    QCO_ALLOWED_IN_PRECONFIG =  1U << 1,
 } QmpCommandOptions;

it should compile and do proper check in above hunk.

I'll try to do it and see if it works.

> Or, an alternative is to move the following hunk from patch 3/9 to this patch:
> 
>   diff --git a/qapi/run-state.json b/qapi/run-state.json
>   index 1c9fff3aef..ce846a570e 100644
>   --- a/qapi/run-state.json
>   +++ b/qapi/run-state.json
>   @@ -49,12 +49,15 @@
>    # @colo: guest is paused to save/restore VM state under colo checkpoint,
>    #        VM can not get into this state unless colo capability is enabled
>    #        for migration. (since 2.8)
>   +# @preconfig: QEMU is paused before board specific init callback is executed.
>   +#             The state is reachable only if -preconfig CLI option is used.
>   +#             (Since 2.12)
>    ##
>    { 'enum': 'RunState',
>      'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
>                'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
>                'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
>   -            'guest-panicked', 'colo' ] }
>   +            'guest-panicked', 'colo', 'preconfig' ] }
>   
>    ##
>    # @StatusInfo:
> 
> Which could be an interesting idea, because the QAPI schema
> changes would be all grouped inside a single patch, and then
> followed by the actual implementation of the -preconfig option.
it won't really work as this enum generates RUN_STATE_PRECONFIG,
which is used by 3/9

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Igor Mammedov]

On Wed, 28 Mar 2018 16:21:48 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Mar 28, 2018 at 01:48:35PM +0200, Igor Mammedov wrote:
> > On Tue, 27 Mar 2018 17:05:41 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> > > On Fri, 23 Mar 2018 18:25:08 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:    
> > > [...]  
> > [...]  
> > > > > @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
> > > > >      RunState r;
> > > > >      ShutdownCause request;
> > > > >  
> > > > > +    if (preconfig_exit_requested) {
> > > > > +        if (runstate_check(RUN_STATE_PRECONFIG)) {      
> > > > 
> > > > Is it possible to have preconfig_exit_request set outside of
> > > > RUN_STATE_PRECONFIG?  When and why?    
> > > preconfig_exit_requested is initialized with TRUE and
> > > in combo with '-inmigrate' we need this runstate check.
> > > it's the same as it was with
> > >  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > > which I probably should remove (I need to check it though)  
> > [...]
> >   
> > > > > @@ -4594,6 +4623,10 @@ int main(int argc, char **argv, char **envp)
> > > > >      }
> > > > >      parse_numa_opts(current_machine);
> > > > >  
> > > > > +    /* do monitor/qmp handling at preconfig state if requested */
> > > > > +    main_loop();      
> > > > 
> > > > Wouldn't it be simpler to do "if (!preconfig) { main_loop(); }"
> > > > instead of entering main_loop() just to exit immediately?    
> > > The thought didn't cross my mind, it might work and more readable
> > > as one doesn't have to jump into main_loop() to find out that
> > > it would exit immediately.
> > > I'll try to it on respin.  
> > Well doing as suggested end ups more messy:
> > 
> >     @@static bool main_loop_should_exit(void)
> >     ...
> >     if (preconfig_exit_requested) {
> >         runstate_set(RUN_STATE_PRELAUNCH);                                        
> >         return true;
> >     }
> >    
> >     @@main
> >     /* do monitor/qmp handling at preconfig state if requested */
> >     if (!preconfig_exit_requested) {
> >         main_loop();
> >     } else if (runstate_check(RUN_STATE_PRECONFIG)) {
> >         runstate_set(RUN_STATE_PRELAUNCH);
> >     }  
> 
> This doesn't make sense to me.  Why would we enter
> RUN_STATE_PRECONFIG state if -preconfig is not used at all?
because of RUN_STATE_PRECONFIG becomes new initial state of
our state machine where we start of (used to be RUN_STATE_PRELAUNCH)
Lets call it variant 1:

with this we have 2 possible transitions:
 RUN_STATE_PRECONFIG -> RUN_STATE_PRELAUNCH (machine_init)

and

 RUN_STATE_PRECONFIG -> RUN_STATE_INMIGRATE
   ugly but it was the same with RUN_STATE_PRELAUNCH initial transition

Another variant 2, in case we switch to RUN_STATE_PRECONFIG only on -preconfig
transitions would be
  RUN_STATE_PRELAUNCH -> RUN_STATE_PRECONFIG
    (allow switch from initial to -preconfig)

  RUN_STATE_PRECONFIG -> RUN_STATE_PRELAUNCH

while the last is valid transition, the 1st one isn't really
valid because of (beside of switching from initial state) it
allows bouncing back to RUN_STATE_PRECONFIG later.

If we consider only state machine transitions, I think it's
cleaner to start with variant 1 with the same
-inmigrate hack we already have (which potentially could
be fixed later), than allowing arbitrary bouncing to
RUN_STATE_PRECONFIG at later stage.

With this approach all processing before machine_init()
would run at RUN_STATE_PRECONFIG and then we would switch
to RUN_STATE_PRELAUNCH. Even though it is far reaching
goal but at least that's where we should be moving to
have sane initialization flow in vl.c

> >     preconfig_exit_requested = false;
> >     ...
> > 
> > I'd prefer original v4 approach, where only main_loop_should_exit()
> > has to deal with state transitions and book-keeping.  
> 
> If the above is unavoidable, I agree.  But I still don't
> understand we have to enter PRECONFIG state if the user didn't
> specify -preconfig.
> 

Re: [Qemu-devel] [PATCH v4 5/9] qapi: introduce new cmd option "allowed-in-preconfig"

[Eduardo Habkost]

On Thu, Mar 29, 2018 at 11:53:55AM +0200, Igor Mammedov wrote:
> On Wed, 28 Mar 2018 16:30:16 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Mar 28, 2018 at 02:29:57PM +0200, Igor Mammedov wrote:
> > > On Fri, 23 Mar 2018 18:28:37 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Mon, Mar 12, 2018 at 02:11:11PM +0100, Igor Mammedov wrote:  
> > > > > New option will be used to allow commands, which are prepared/need
> > > > > to run run in preconfig state. Other commands that should be able
> > > > > to run in preconfig state, should be ammeded to not expect machine
> > > > > in initialized state or deal with it.
> > > > > 
> > > > > For compatibility reasons, commands, that don't use new flag
> > > > > 'allowed-in-preconfig' explicitly, are not permited to run in
> > > > > preconfig state but allowed in all other states like they used
> > > > > to be.
> > > > > 
> > > > > Within this patch allow following commands in preconfig state:
> > > > >    qmp_capabilities
> > > > >    query-qmp-schema
> > > > >    query-commands
> > > > >    query-status
> > > > >    cont
> > > > > to allow qmp connection, basic introspection and moving to the next
> > > > > state.
> > > > > 
> > > > > PS:
> > > > > set-numa-node and query-hotpluggable-cpus will be enabled later in
> > > > > a separate patch.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>    
> > > > 
> > > > I didn't review the code yet, but:
> > > > 
> > > > Shouldn't this be applied before patch 3/9, for bisectability?
> > > > Otherwise it will be very easy to crash QEMU after applying patch
> > > > 3/9.  
> > > no, it isn't going to work.
> > > This patch depends on RUN_STATE_PRECONFIG that is introduced in 3/9.
> > > 
> > > It could be fine to merge into 3/9 during merge, but then history
> > > wise it would be difficult to read it later with 2 big and mostly
> > > separate changes within one patch.  
> > 
> > Yeah, I don't think squashing would be the right answer.
> > 
> > > 
> > > Considering -preconfig if off by default it shouldn't affect
> > > bisectability in general so I'd keep current patch order.  
> > 
> > Well, it would affect bisectability if debugging a crash that
> > happens using -preconfig.
> > 
> > The only hunk in this patch that really depends on patch 3/9
> > seems to be:
> > 
> >   @@ -92,6 +93,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;
> >   +    }
> >   +
> >        if (!qdict_haskey(dict, "arguments")) {
> >            args = qdict_new();
> >        } else {
> >   
> > 
> > What about moving it to patch 3/9?
> By itself it won't work but with moving following hunk with it
> 
> @@ -21,7 +21,8 @@ typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
>  typedef enum QmpCommandOptions
>  {
>      QCO_NO_OPTIONS = 0x0,
> -    QCO_NO_SUCCESS_RESP = 0x1,
> +    QCO_NO_SUCCESS_RESP = 1U << 0,
> +    QCO_ALLOWED_IN_PRECONFIG =  1U << 1,
>  } QmpCommandOptions;
> 
> it should compile and do proper check in above hunk.
> 
> I'll try to do it and see if it works.

Sounds OK to me.

> 
> > Or, an alternative is to move the following hunk from patch 3/9 to this patch:
> > 
> >   diff --git a/qapi/run-state.json b/qapi/run-state.json
> >   index 1c9fff3aef..ce846a570e 100644
> >   --- a/qapi/run-state.json
> >   +++ b/qapi/run-state.json
> >   @@ -49,12 +49,15 @@
> >    # @colo: guest is paused to save/restore VM state under colo checkpoint,
> >    #        VM can not get into this state unless colo capability is enabled
> >    #        for migration. (since 2.8)
> >   +# @preconfig: QEMU is paused before board specific init callback is executed.
> >   +#             The state is reachable only if -preconfig CLI option is used.
> >   +#             (Since 2.12)
> >    ##
> >    { 'enum': 'RunState',
> >      'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> >                'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> >                'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> >   -            'guest-panicked', 'colo' ] }
> >   +            'guest-panicked', 'colo', 'preconfig' ] }
> >   
> >    ##
> >    # @StatusInfo:
> > 
> > Which could be an interesting idea, because the QAPI schema
> > changes would be all grouped inside a single patch, and then
> > followed by the actual implementation of the -preconfig option.
> it won't really work as this enum generates RUN_STATE_PRECONFIG,
> which is used by 3/9

My suggestion in this case would be applying this patch
(including the above run-state.json change) before 3/9.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Igor Mammedov]

On Wed, 28 Mar 2018 16:17:32 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Mar 27, 2018 at 05:05:41PM +0200, Igor Mammedov wrote:
> > On Fri, 23 Mar 2018 18:25:08 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:  
> > [...]  
> > > > diff --git a/vl.c b/vl.c
> > > > index 3ef04ce..69b1997 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -593,7 +593,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
> > > >  /***********************************************************/
> > > >  /* QEMU state */
> > > >  
> > > > -static RunState current_run_state = RUN_STATE_PRELAUNCH;
> > > > +static RunState current_run_state = RUN_STATE_PRECONFIG;
> > > >  
> > > >  /* We use RUN_STATE__MAX but any invalid value will do */
> > > >  static RunState vmstop_requested = RUN_STATE__MAX;
> > > > @@ -606,6 +606,9 @@ typedef struct {
> > > >  
> > > >  static const RunStateTransition runstate_transitions_def[] = {
> > > >      /*     from      ->     to      */
> > > > +    { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> > > > +    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },    
> > > 
> > > Don't this mean -preconfig and -incoming could work together?  
> > theoretically yes, but its not the reason why this transition is here.
> > It's mimicking existing approach where initial state
> >    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > were allowed to move to the next possible (including RUN_STATE_INMIGRATE)  
> 
> I still don't get it.  Where this definition of "next possible"
> comes from?  If -incoming and -preconfig don't work together, why
> is PRECONFIG -> INMIGRATE migration considered possible?
I'd think it's the same (replacement) hack which we use now
   RUN_STATE_PRELAUNCH -> RUN_STATE_INMIGRATE
to allow following code to succeed:

      case QEMU_OPTION_incoming:
      if (!incoming) {                                                 
             runstate_set(RUN_STATE_INMIGRATE);                           
      }                                                                
      incoming = optarg;
 
I'd get rid of it and move state switching to the actual place
where migration starts if it were just that simple, but from
a quick look around it did look rather risky.
That's why I abandoned an idea of changing it within this series.

> > > >      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
> > > >      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> > > >      { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
> > > > @@ -1629,6 +1632,7 @@ static pid_t shutdown_pid;
> > > >  static int powerdown_requested;
> > > >  static int debug_requested;
> > > >  static int suspend_requested;
> > > > +static bool preconfig_exit_requested = true;
> > > >  static WakeupReason wakeup_reason;
> > > >  static NotifierList powerdown_notifiers =
> > > >      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> > > > @@ -1713,6 +1717,11 @@ static int qemu_debug_requested(void)
> > > >      return r;
> > > >  }
> > > >  
> > > > +void qemu_exit_preconfig_request(void)
> > > > +{
> > > > +    preconfig_exit_requested = true;
> > > > +}
> > > > +
> > > >  /*
> > > >   * Reset the VM. Issue an event unless @reason is SHUTDOWN_CAUSE_NONE.
> > > >   */
> > > > @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
> > > >      RunState r;
> > > >      ShutdownCause request;
> > > >  
> > > > +    if (preconfig_exit_requested) {
> > > > +        if (runstate_check(RUN_STATE_PRECONFIG)) {    
> > > 
> > > Is it possible to have preconfig_exit_request set outside of
> > > RUN_STATE_PRECONFIG?  When and why?  
> > preconfig_exit_requested is initialized with TRUE and
> > in combo with '-inmigrate' we need this runstate check.  
> 
> I think this now makes sense to me.  It still looks confusing,
> but I don't have a better suggestion right now.
> 
> Except...
> 
> Why exactly do you need to use main_loop() and
> main_loop_should_exit() for the preconfig loop?  What about a
> separate preconfig_loop() and preconfig_loop_should_exit()
> function?
that would duplicate main_loop() for practically no benefit at all,
hence I'm reusing existing main_loop()/main_loop_should_exit()
just by adding relevant exit condition. It also easier to read
when state transitions are kept close to each other.

 
> > it's the same as it was with
> >  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > which I probably should remove (I need to check it though)
> >   
> > > > +            runstate_set(RUN_STATE_PRELAUNCH);
> > > > +        }
> > > > +        preconfig_exit_requested = false;  
> 
> What happens if we don't set preconfig_exit_requested=false here?
nothing should go wrong due to 'if (runstate_check(RUN_STATE_PRECONFIG))'
condition. It's the same what qemu_reset_requested()/qemu_shutdown_requested()
do with their respective request variables but not wrapped
into a separate function as it's the only place it's used.

 
> > > > +        return true;
> > > > +    }
> > > >      if (qemu_debug_requested()) {
> > > >          vm_stop(RUN_STATE_DEBUG);
> > > >      }
> > > > @@ -3697,6 +3713,14 @@ int main(int argc, char **argv, char **envp)
> > > >                      exit(1);
> > > >                  }
> > > >                  break;
> > > > +            case QEMU_OPTION_preconfig:
> > > > +                if (runstate_check(RUN_STATE_INMIGRATE)) {
> > > > +                    error_report("option can not be used with "
> > > > +                                 "-incoming option");
> > > > +                    exit(EXIT_FAILURE);
> > > > +                }    
> > > 
> > > So -incoming changes runstate as soon as the option is parsed?
> > > 
> > > Ouch.  
> > yep and it's rather fragile (it's well out of scope of
> > this series to re-factor this, so I'm not changing it here)
> >   
> > > I would rather not rely on that behavior and just do
> > > "if (incoming)".
> > > 
> > > Why exactly it's not possible to use -incoming with -preconfig?  
> > there are 2 reasons why I made options mutually exclusive
> > 1. (excuse ) '-incoming' is an option with non explicit side effects
> >    on other parts of code. It's hard to predict behavior
> >    of preconfig commands in combination with inmigrate.
> >    I wouldn't try to touch/change anything related to it
> >    in this series.
> >    If we need to change how option is handled, it should
> >    be separate series that focuses on it.
> > 2. (main reason) is to expose as minimal interface
> >    as possible. It's easier to extend/modify it future if
> >    necessary than cut it down after it was introduced.
> > 
> >    Not counting [1], I don't see a reason to permit
> >    'preconfig' while migration is in progress.
> >    Configuration commands that where used during 'preconfig'
> >    stage on source side, should use corresponding CLI options
> >    on target side. (it's the same behavior as with hotplugged
> >    devices, keeping migration work-flow the same)
> > 
> > In short I'd prefer to keep restriction until there will be
> > a real usecase for combo to work together.  
> 
> I understand the reasons, but I think we already have an
> important use case: live-migrating a VM with non-trivial NUMA
> config (that needs -preconfig).  Don't we?
Not really,
whatever we have configured on source side using -preconfig
(discovering valid topology in process), we should be able
to replicate using only CLI options on target since we
already have all necessary values for it from source (it's
certainly the case with this series set-numa-node command).

As for the future, I agree it would be much more flexible
to allow both -preconfig and -incoming at the same time,
so we could start target with empty CLI, and then feed it
options from source. It would require audit/refactoring of
INMIGRATE state and making 'all' current CLI options
available via QMP interface.

But for now I'd prefer to keep using old way to start target.

> > > > +                preconfig_exit_requested = false;
> > > > +                break;
> > > >              case QEMU_OPTION_enable_kvm:
> > > >                  olist = qemu_find_opts("machine");
> > > >                  qemu_opts_parse_noisily(olist, "accel=kvm", false);
> > > > @@ -3902,6 +3926,11 @@ int main(int argc, char **argv, char **envp)
> > > >                  }
> > > >                  break;
> > > >              case QEMU_OPTION_incoming:
> > > > +                if (!preconfig_exit_requested) {
> > > > +                    error_report("option can not be used with "
> > > > +                                 "-preconfig option");
> > > > +                    exit(EXIT_FAILURE);
> > > > +                }    
> > > 
> > > Instead of reimplementing the same check in two separate places,
> > > why not validate options and check for (incoming && preconfig)
> > > after the option parsing loop?  
> > it could be done this way, but then we would lose specialized
> > error message.
> > Even though the way I did it, it is more code but that code
> > is close to related options and allows for specialized error
> > message in the order options are parsed.  
> 
> What do you mean by specialized user message?  Both have exactly
> the same information: "-incoming and -preconfig can't be used
> together", just written in a different way.
[...]
> 
> I agree with the argument that validation of config options
> should be done all in the same place.  But I disagree that the
> body of the option parsing loop is the right place for that.
Ok, I'll move it out of loop as you suggested.

[...]

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Igor Mammedov]

On Wed, 28 Mar 2018 15:54:28 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Mar 27, 2018 at 03:08:27PM +0200, Igor Mammedov wrote:
> > On Fri, 23 Mar 2018 17:42:18 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Mon, Mar 12, 2018 at 02:11:08PM +0100, Igor Mammedov wrote:  
> > > > it will allow to reuse parse_NumaOptions() for parsing
> > > > configuration commands received via QMP interface
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  include/sysemu/numa.h |  1 +
> > > >  numa.c                | 48 +++++++++++++++++++++++++++++-------------------
> > > >  2 files changed, 30 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > > index 21713b7..7a0ae75 100644
> > > > --- a/include/sysemu/numa.h
> > > > +++ b/include/sysemu/numa.h
> > > > @@ -22,6 +22,7 @@ struct NumaNodeMem {
> > > >  };
> > > >  
> > > >  extern NodeInfo numa_info[MAX_NODES];
> > > > +int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
> > > >  void parse_numa_opts(MachineState *ms);
> > > >  void numa_complete_configuration(MachineState *ms);
> > > >  void query_numa_node_mem(NumaNodeMem node_mem[]);
> > > > diff --git a/numa.c b/numa.c
> > > > index 126c649..2b1d292 100644
> > > > --- a/numa.c
> > > > +++ b/numa.c
> > > > @@ -169,28 +169,11 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)
> > > >      have_numa_distance = true;
> > > >  }
> > > >  
> > > > -static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> > > > +static
> > > > +void parse_NumaOptions(MachineState *ms, NumaOptions *object, Error **errp)    
> > > 
> > > I wonder if we should rename the parse_numa_{node,distance}()
> > > functions to configure_numa_{node,distance}(), and this one
> > > configure_numa().  These functions don't parse anything, anymore.  
> > I'd preffer to do it in another patch on top of this series
> > (added it my TODO list)  
> 
> I agree with renaming parse_numa*() later, but the new function
> you are creating can have a more reasonable name as it doesn't
> parse anything.


how about: s/parse_NumaOptions/set_NumaOptions/

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Eduardo Habkost]

On Thu, Mar 29, 2018 at 01:43:03PM +0200, Igor Mammedov wrote:
> On Wed, 28 Mar 2018 16:21:48 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Mar 28, 2018 at 01:48:35PM +0200, Igor Mammedov wrote:
> > > On Tue, 27 Mar 2018 17:05:41 +0200
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > >   
> > > > On Fri, 23 Mar 2018 18:25:08 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >   
> > > > > On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:    
> > > > [...]  
> > > [...]  
> > > > > > @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
> > > > > >      RunState r;
> > > > > >      ShutdownCause request;
> > > > > >  
> > > > > > +    if (preconfig_exit_requested) {
> > > > > > +        if (runstate_check(RUN_STATE_PRECONFIG)) {      
> > > > > 
> > > > > Is it possible to have preconfig_exit_request set outside of
> > > > > RUN_STATE_PRECONFIG?  When and why?    
> > > > preconfig_exit_requested is initialized with TRUE and
> > > > in combo with '-inmigrate' we need this runstate check.
> > > > it's the same as it was with
> > > >  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > > > which I probably should remove (I need to check it though)  
> > > [...]
> > >   
> > > > > > @@ -4594,6 +4623,10 @@ int main(int argc, char **argv, char **envp)
> > > > > >      }
> > > > > >      parse_numa_opts(current_machine);
> > > > > >  
> > > > > > +    /* do monitor/qmp handling at preconfig state if requested */
> > > > > > +    main_loop();      
> > > > > 
> > > > > Wouldn't it be simpler to do "if (!preconfig) { main_loop(); }"
> > > > > instead of entering main_loop() just to exit immediately?    
> > > > The thought didn't cross my mind, it might work and more readable
> > > > as one doesn't have to jump into main_loop() to find out that
> > > > it would exit immediately.
> > > > I'll try to it on respin.  
> > > Well doing as suggested end ups more messy:
> > > 
> > >     @@static bool main_loop_should_exit(void)
> > >     ...
> > >     if (preconfig_exit_requested) {
> > >         runstate_set(RUN_STATE_PRELAUNCH);                                        
> > >         return true;
> > >     }
> > >    
> > >     @@main
> > >     /* do monitor/qmp handling at preconfig state if requested */
> > >     if (!preconfig_exit_requested) {
> > >         main_loop();
> > >     } else if (runstate_check(RUN_STATE_PRECONFIG)) {
> > >         runstate_set(RUN_STATE_PRELAUNCH);
> > >     }  
> > 
> > This doesn't make sense to me.  Why would we enter
> > RUN_STATE_PRECONFIG state if -preconfig is not used at all?
> because of RUN_STATE_PRECONFIG becomes new initial state of
> our state machine where we start of (used to be RUN_STATE_PRELAUNCH)

Oh, I missed that part.

> Lets call it variant 1:
> 
> with this we have 2 possible transitions:
>  RUN_STATE_PRECONFIG -> RUN_STATE_PRELAUNCH (machine_init)
> 
> and
> 
>  RUN_STATE_PRECONFIG -> RUN_STATE_INMIGRATE
>    ugly but it was the same with RUN_STATE_PRELAUNCH initial transition

That explains a lot, thanks.

> 
> Another variant 2, in case we switch to RUN_STATE_PRECONFIG only on -preconfig
> transitions would be
>   RUN_STATE_PRELAUNCH -> RUN_STATE_PRECONFIG
>     (allow switch from initial to -preconfig)
> 
>   RUN_STATE_PRECONFIG -> RUN_STATE_PRELAUNCH
> 
> while the last is valid transition, the 1st one isn't really
> valid because of (beside of switching from initial state) it
> allows bouncing back to RUN_STATE_PRECONFIG later.
> 
> If we consider only state machine transitions, I think it's
> cleaner to start with variant 1 with the same
> -inmigrate hack we already have (which potentially could
> be fixed later), than allowing arbitrary bouncing to
> RUN_STATE_PRECONFIG at later stage.
> 
> With this approach all processing before machine_init()
> would run at RUN_STATE_PRECONFIG and then we would switch
> to RUN_STATE_PRELAUNCH. Even though it is far reaching
> goal but at least that's where we should be moving to
> have sane initialization flow in vl.c

Thanks, now variant 1 makes more sense to me.  But I really miss
here are very clear and explicit descriptions of what each state
really mean, and what are the differences between them.

It looks like the existing description for `prelaunch` isn't
accurate:

# @prelaunch: QEMU was started with -S and guest has not started

This is false, as QEMU can be in `prelaunch` state even if -S is
not used.


Also, this is the description you proposed for `preconfig`:

# @preconfig: QEMU is paused before board specific init callback is executed.
#             The state is reachable only if -preconfig CLI option is used.
#             (Since 2.12)

This seems wrong as well: the `prelaunch` state is reachable even
if `-preconfig` isn't used in the command-line (because it is the
initial state).


> 
> > >     preconfig_exit_requested = false;
> > >     ...
> > > 
> > > I'd prefer original v4 approach, where only main_loop_should_exit()
> > > has to deal with state transitions and book-keeping.  
> > 
> > If the above is unavoidable, I agree.  But I still don't
> > understand we have to enter PRECONFIG state if the user didn't
> > specify -preconfig.
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Eduardo Habkost]

On Thu, Mar 29, 2018 at 03:05:09PM +0200, Igor Mammedov wrote:
> On Wed, 28 Mar 2018 15:54:28 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Mar 27, 2018 at 03:08:27PM +0200, Igor Mammedov wrote:
> > > On Fri, 23 Mar 2018 17:42:18 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Mon, Mar 12, 2018 at 02:11:08PM +0100, Igor Mammedov wrote:  
> > > > > it will allow to reuse parse_NumaOptions() for parsing
> > > > > configuration commands received via QMP interface
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > >  include/sysemu/numa.h |  1 +
> > > > >  numa.c                | 48 +++++++++++++++++++++++++++++-------------------
> > > > >  2 files changed, 30 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > > > index 21713b7..7a0ae75 100644
> > > > > --- a/include/sysemu/numa.h
> > > > > +++ b/include/sysemu/numa.h
> > > > > @@ -22,6 +22,7 @@ struct NumaNodeMem {
> > > > >  };
> > > > >  
> > > > >  extern NodeInfo numa_info[MAX_NODES];
> > > > > +int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
> > > > >  void parse_numa_opts(MachineState *ms);
> > > > >  void numa_complete_configuration(MachineState *ms);
> > > > >  void query_numa_node_mem(NumaNodeMem node_mem[]);
> > > > > diff --git a/numa.c b/numa.c
> > > > > index 126c649..2b1d292 100644
> > > > > --- a/numa.c
> > > > > +++ b/numa.c
> > > > > @@ -169,28 +169,11 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)
> > > > >      have_numa_distance = true;
> > > > >  }
> > > > >  
> > > > > -static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> > > > > +static
> > > > > +void parse_NumaOptions(MachineState *ms, NumaOptions *object, Error **errp)    
> > > > 
> > > > I wonder if we should rename the parse_numa_{node,distance}()
> > > > functions to configure_numa_{node,distance}(), and this one
> > > > configure_numa().  These functions don't parse anything, anymore.  
> > > I'd preffer to do it in another patch on top of this series
> > > (added it my TODO list)  
> > 
> > I agree with renaming parse_numa*() later, but the new function
> > you are creating can have a more reasonable name as it doesn't
> > parse anything.
> 
> 
> how about: s/parse_NumaOptions/set_NumaOptions/

No strong preference.  "register", "configure", "set" would all
be good enough to me.  I would avoid the weird
underline_and_CamelCase naming style, though (this style seems to
be used only by generated QAPI code).

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Eduardo Habkost]

On Thu, Mar 29, 2018 at 03:01:12PM +0200, Igor Mammedov wrote:
> On Wed, 28 Mar 2018 16:17:32 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Mar 27, 2018 at 05:05:41PM +0200, Igor Mammedov wrote:
> > > On Fri, 23 Mar 2018 18:25:08 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:  
> > > [...]  
> > > > > diff --git a/vl.c b/vl.c
> > > > > index 3ef04ce..69b1997 100644
> > > > > --- a/vl.c
> > > > > +++ b/vl.c
> > > > > @@ -593,7 +593,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
> > > > >  /***********************************************************/
> > > > >  /* QEMU state */
> > > > >  
> > > > > -static RunState current_run_state = RUN_STATE_PRELAUNCH;
> > > > > +static RunState current_run_state = RUN_STATE_PRECONFIG;
> > > > >  
> > > > >  /* We use RUN_STATE__MAX but any invalid value will do */
> > > > >  static RunState vmstop_requested = RUN_STATE__MAX;
> > > > > @@ -606,6 +606,9 @@ typedef struct {
> > > > >  
> > > > >  static const RunStateTransition runstate_transitions_def[] = {
> > > > >      /*     from      ->     to      */
> > > > > +    { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> > > > > +    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },    
> > > > 
> > > > Don't this mean -preconfig and -incoming could work together?  
> > > theoretically yes, but its not the reason why this transition is here.
> > > It's mimicking existing approach where initial state
> > >    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > > were allowed to move to the next possible (including RUN_STATE_INMIGRATE)  
> > 
> > I still don't get it.  Where this definition of "next possible"
> > comes from?  If -incoming and -preconfig don't work together, why
> > is PRECONFIG -> INMIGRATE migration considered possible?
> I'd think it's the same (replacement) hack which we use now
>    RUN_STATE_PRELAUNCH -> RUN_STATE_INMIGRATE
> to allow following code to succeed:
> 
>       case QEMU_OPTION_incoming:
>       if (!incoming) {                                                 
>              runstate_set(RUN_STATE_INMIGRATE);                           
>       }                                                                
>       incoming = optarg;
>  
> I'd get rid of it and move state switching to the actual place
> where migration starts if it were just that simple, but from
> a quick look around it did look rather risky.
> That's why I abandoned an idea of changing it within this series.

Yeah, I now see that the initial state is PRECONFIG.

> 
> > > > >      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
> > > > >      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> > > > >      { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
> > > > > @@ -1629,6 +1632,7 @@ static pid_t shutdown_pid;
> > > > >  static int powerdown_requested;
> > > > >  static int debug_requested;
> > > > >  static int suspend_requested;
> > > > > +static bool preconfig_exit_requested = true;
> > > > >  static WakeupReason wakeup_reason;
> > > > >  static NotifierList powerdown_notifiers =
> > > > >      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> > > > > @@ -1713,6 +1717,11 @@ static int qemu_debug_requested(void)
> > > > >      return r;
> > > > >  }
> > > > >  
> > > > > +void qemu_exit_preconfig_request(void)
> > > > > +{
> > > > > +    preconfig_exit_requested = true;
> > > > > +}
> > > > > +
> > > > >  /*
> > > > >   * Reset the VM. Issue an event unless @reason is SHUTDOWN_CAUSE_NONE.
> > > > >   */
> > > > > @@ -1886,6 +1895,13 @@ static bool main_loop_should_exit(void)
> > > > >      RunState r;
> > > > >      ShutdownCause request;
> > > > >  
> > > > > +    if (preconfig_exit_requested) {
> > > > > +        if (runstate_check(RUN_STATE_PRECONFIG)) {    
> > > > 
> > > > Is it possible to have preconfig_exit_request set outside of
> > > > RUN_STATE_PRECONFIG?  When and why?  
> > > preconfig_exit_requested is initialized with TRUE and
> > > in combo with '-inmigrate' we need this runstate check.  
> > 
> > I think this now makes sense to me.  It still looks confusing,
> > but I don't have a better suggestion right now.
> > 
> > Except...
> > 
> > Why exactly do you need to use main_loop() and
> > main_loop_should_exit() for the preconfig loop?  What about a
> > separate preconfig_loop() and preconfig_loop_should_exit()
> > function?
> that would duplicate main_loop() for practically no benefit at all,
> hence I'm reusing existing main_loop()/main_loop_should_exit()
> just by adding relevant exit condition. It also easier to read
> when state transitions are kept close to each other.

I wouldn't say that main_loop_should_exit() is easy to read, but
I understand that this is the existing style, so no objection.


> 
>  
> > > it's the same as it was with
> > >  { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > > which I probably should remove (I need to check it though)
> > >   
> > > > > +            runstate_set(RUN_STATE_PRELAUNCH);
> > > > > +        }
> > > > > +        preconfig_exit_requested = false;  
> > 
> > What happens if we don't set preconfig_exit_requested=false here?
> nothing should go wrong due to 'if (runstate_check(RUN_STATE_PRECONFIG))'
> condition. It's the same what qemu_reset_requested()/qemu_shutdown_requested()
> do with their respective request variables but not wrapped
> into a separate function as it's the only place it's used.
> 
>  
> > > > > +        return true;
> > > > > +    }
> > > > >      if (qemu_debug_requested()) {
> > > > >          vm_stop(RUN_STATE_DEBUG);
> > > > >      }
> > > > > @@ -3697,6 +3713,14 @@ int main(int argc, char **argv, char **envp)
> > > > >                      exit(1);
> > > > >                  }
> > > > >                  break;
> > > > > +            case QEMU_OPTION_preconfig:
> > > > > +                if (runstate_check(RUN_STATE_INMIGRATE)) {
> > > > > +                    error_report("option can not be used with "
> > > > > +                                 "-incoming option");
> > > > > +                    exit(EXIT_FAILURE);
> > > > > +                }    
> > > > 
> > > > So -incoming changes runstate as soon as the option is parsed?
> > > > 
> > > > Ouch.  
> > > yep and it's rather fragile (it's well out of scope of
> > > this series to re-factor this, so I'm not changing it here)
> > >   
> > > > I would rather not rely on that behavior and just do
> > > > "if (incoming)".
> > > > 
> > > > Why exactly it's not possible to use -incoming with -preconfig?  
> > > there are 2 reasons why I made options mutually exclusive
> > > 1. (excuse ) '-incoming' is an option with non explicit side effects
> > >    on other parts of code. It's hard to predict behavior
> > >    of preconfig commands in combination with inmigrate.
> > >    I wouldn't try to touch/change anything related to it
> > >    in this series.
> > >    If we need to change how option is handled, it should
> > >    be separate series that focuses on it.
> > > 2. (main reason) is to expose as minimal interface
> > >    as possible. It's easier to extend/modify it future if
> > >    necessary than cut it down after it was introduced.
> > > 
> > >    Not counting [1], I don't see a reason to permit
> > >    'preconfig' while migration is in progress.
> > >    Configuration commands that where used during 'preconfig'
> > >    stage on source side, should use corresponding CLI options
> > >    on target side. (it's the same behavior as with hotplugged
> > >    devices, keeping migration work-flow the same)
> > > 
> > > In short I'd prefer to keep restriction until there will be
> > > a real usecase for combo to work together.  
> > 
> > I understand the reasons, but I think we already have an
> > important use case: live-migrating a VM with non-trivial NUMA
> > config (that needs -preconfig).  Don't we?
> Not really,
> whatever we have configured on source side using -preconfig
> (discovering valid topology in process), we should be able
> to replicate using only CLI options on target since we
> already have all necessary values for it from source (it's
> certainly the case with this series set-numa-node command).
> 
> As for the future, I agree it would be much more flexible
> to allow both -preconfig and -incoming at the same time,
> so we could start target with empty CLI, and then feed it
> options from source. It would require audit/refactoring of
> INMIGRATE state and making 'all' current CLI options
> available via QMP interface.
> 
> But for now I'd prefer to keep using old way to start target.

Well, if management software developers tell us that -preconfig
will be already useful without -incoming support, I won't object.

But it would be very nice for management software if they can
simply assume that -preconfig and -incoming will work together
since the first version.  Can we have this as a goal for 2.13?


> 
> > > > > +                preconfig_exit_requested = false;
> > > > > +                break;
> > > > >              case QEMU_OPTION_enable_kvm:
> > > > >                  olist = qemu_find_opts("machine");
> > > > >                  qemu_opts_parse_noisily(olist, "accel=kvm", false);
> > > > > @@ -3902,6 +3926,11 @@ int main(int argc, char **argv, char **envp)
> > > > >                  }
> > > > >                  break;
> > > > >              case QEMU_OPTION_incoming:
> > > > > +                if (!preconfig_exit_requested) {
> > > > > +                    error_report("option can not be used with "
> > > > > +                                 "-preconfig option");
> > > > > +                    exit(EXIT_FAILURE);
> > > > > +                }    
> > > > 
> > > > Instead of reimplementing the same check in two separate places,
> > > > why not validate options and check for (incoming && preconfig)
> > > > after the option parsing loop?  
> > > it could be done this way, but then we would lose specialized
> > > error message.
> > > Even though the way I did it, it is more code but that code
> > > is close to related options and allows for specialized error
> > > message in the order options are parsed.  
> > 
> > What do you mean by specialized user message?  Both have exactly
> > the same information: "-incoming and -preconfig can't be used
> > together", just written in a different way.
> [...]
> > 
> > I agree with the argument that validation of config options
> > should be done all in the same place.  But I disagree that the
> > body of the option parsing loop is the right place for that.
> Ok, I'll move it out of loop as you suggested.
> 
> [...]
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 3617 bytes --]

On Thu, Mar 29, 2018 at 13:57:54 -0300, Eduardo Habkost wrote:
> On Thu, Mar 29, 2018 at 03:01:12PM +0200, Igor Mammedov wrote:
> > On Wed, 28 Mar 2018 16:17:32 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > On Tue, Mar 27, 2018 at 05:05:41PM +0200, Igor Mammedov wrote:
> > > > On Fri, 23 Mar 2018 18:25:08 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > On Mon, Mar 12, 2018 at 02:11:09PM +0100, Igor Mammedov wrote:  

[...]

> > > > > Why exactly it's not possible to use -incoming with -preconfig?  
> > > > there are 2 reasons why I made options mutually exclusive
> > > > 1. (excuse ) '-incoming' is an option with non explicit side effects
> > > >    on other parts of code. It's hard to predict behavior
> > > >    of preconfig commands in combination with inmigrate.
> > > >    I wouldn't try to touch/change anything related to it
> > > >    in this series.
> > > >    If we need to change how option is handled, it should
> > > >    be separate series that focuses on it.
> > > > 2. (main reason) is to expose as minimal interface
> > > >    as possible. It's easier to extend/modify it future if
> > > >    necessary than cut it down after it was introduced.
> > > > 
> > > >    Not counting [1], I don't see a reason to permit
> > > >    'preconfig' while migration is in progress.
> > > >    Configuration commands that where used during 'preconfig'
> > > >    stage on source side, should use corresponding CLI options
> > > >    on target side. (it's the same behavior as with hotplugged
> > > >    devices, keeping migration work-flow the same)
> > > > 
> > > > In short I'd prefer to keep restriction until there will be
> > > > a real usecase for combo to work together.  
> > > 
> > > I understand the reasons, but I think we already have an
> > > important use case: live-migrating a VM with non-trivial NUMA
> > > config (that needs -preconfig).  Don't we?
> > Not really,
> > whatever we have configured on source side using -preconfig
> > (discovering valid topology in process), we should be able
> > to replicate using only CLI options on target since we
> > already have all necessary values for it from source (it's
> > certainly the case with this series set-numa-node command).
> > 
> > As for the future, I agree it would be much more flexible
> > to allow both -preconfig and -incoming at the same time,
> > so we could start target with empty CLI, and then feed it
> > options from source. It would require audit/refactoring of
> > INMIGRATE state and making 'all' current CLI options
> > available via QMP interface.
> > 
> > But for now I'd prefer to keep using old way to start target.
> 
> Well, if management software developers tell us that -preconfig
> will be already useful without -incoming support, I won't object.


Hmm, that depends on what we will be configured using the new interface.
We usually prefer to use the same approach to set up things when
starting a new VM and when starting a VM for migration.

Since we do have options to setup the vCPU topology stuff on the
command line once we are going to migrate, we certainly can use
-preconfig even when it will collide with -incoming

Ideally -preconfig should replace -incoming, so that we can swithc to
incomming migration operation after we configure everything.

> But it would be very nice for management software if they can
> simply assume that -preconfig and -incoming will work together
> since the first version.  Can we have this as a goal for 2.13?

It usually helps in reducing code clutter.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Igor Mammedov]

On Thu, 29 Mar 2018 13:57:54 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

[...]
> > As for the future, I agree it would be much more flexible
> > to allow both -preconfig and -incoming at the same time,
> > so we could start target with empty CLI, and then feed it
> > options from source. It would require audit/refactoring of
> > INMIGRATE state and making 'all' current CLI options
> > available via QMP interface.
> > 
> > But for now I'd prefer to keep using old way to start target.  
> 
> Well, if management software developers tell us that -preconfig
> will be already useful without -incoming support, I won't object.
As Peter said, mgmt can/will use -preconfig even without -incoming,
since it lets deal with initial (source) start-up problem (i.e.
avoid restarting QEMU) and lets us make numa configuration work
in qemu/libvirt stack without guess work. (that's the end goal of
the series)

> But it would be very nice for management software if they can
> simply assume that -preconfig and -incoming will work together
> since the first version.  Can we have this as a goal for 2.13?
I can't promise to refactor -incoming in near future, as I'm working
on ARM cpu-hotplug currently and next in queue is ARM memory hotplug.

Peter suggested an alternative idea, to abandon -incoming and
enable incoming migration from QMP after all configuration is done.
Amount of refactoring need probably would be the same but at least
interface and runstate transitions wise it looks cleaner.

[...]

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Eduardo Habkost]

On Tue, Apr 03, 2018 at 03:49:07PM +0200, Igor Mammedov wrote:
> On Thu, 29 Mar 2018 13:57:54 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> [...]
> > > As for the future, I agree it would be much more flexible
> > > to allow both -preconfig and -incoming at the same time,
> > > so we could start target with empty CLI, and then feed it
> > > options from source. It would require audit/refactoring of
> > > INMIGRATE state and making 'all' current CLI options
> > > available via QMP interface.
> > > 
> > > But for now I'd prefer to keep using old way to start target.  
> > 
> > Well, if management software developers tell us that -preconfig
> > will be already useful without -incoming support, I won't object.
> As Peter said, mgmt can/will use -preconfig even without -incoming,
> since it lets deal with initial (source) start-up problem (i.e.
> avoid restarting QEMU) and lets us make numa configuration work
> in qemu/libvirt stack without guess work. (that's the end goal of
> the series)
> 
> > But it would be very nice for management software if they can
> > simply assume that -preconfig and -incoming will work together
> > since the first version.  Can we have this as a goal for 2.13?
> I can't promise to refactor -incoming in near future, as I'm working
> on ARM cpu-hotplug currently and next in queue is ARM memory hotplug.
> 
> Peter suggested an alternative idea, to abandon -incoming and
> enable incoming migration from QMP after all configuration is done.
> Amount of refactoring need probably would be the same but at least
> interface and runstate transitions wise it looks cleaner.

Also, support for the "start incoming migration" QMP command
would be very easy to probe using existing mechanisms.  Sounds
good to me.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()

[Igor Mammedov]

On Thu, 29 Mar 2018 13:31:13 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Mar 29, 2018 at 03:05:09PM +0200, Igor Mammedov wrote:
> > On Wed, 28 Mar 2018 15:54:28 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Tue, Mar 27, 2018 at 03:08:27PM +0200, Igor Mammedov wrote:  
> > > > On Fri, 23 Mar 2018 17:42:18 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Mon, Mar 12, 2018 at 02:11:08PM +0100, Igor Mammedov wrote:    
> > > > > > it will allow to reuse parse_NumaOptions() for parsing
> > > > > > configuration commands received via QMP interface
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > ---
> > > > > >  include/sysemu/numa.h |  1 +
> > > > > >  numa.c                | 48 +++++++++++++++++++++++++++++-------------------
> > > > > >  2 files changed, 30 insertions(+), 19 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > > > > index 21713b7..7a0ae75 100644
> > > > > > --- a/include/sysemu/numa.h
> > > > > > +++ b/include/sysemu/numa.h
> > > > > > @@ -22,6 +22,7 @@ struct NumaNodeMem {
> > > > > >  };
> > > > > >  
> > > > > >  extern NodeInfo numa_info[MAX_NODES];
> > > > > > +int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
> > > > > >  void parse_numa_opts(MachineState *ms);
> > > > > >  void numa_complete_configuration(MachineState *ms);
> > > > > >  void query_numa_node_mem(NumaNodeMem node_mem[]);
> > > > > > diff --git a/numa.c b/numa.c
> > > > > > index 126c649..2b1d292 100644
> > > > > > --- a/numa.c
> > > > > > +++ b/numa.c
> > > > > > @@ -169,28 +169,11 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)
> > > > > >      have_numa_distance = true;
> > > > > >  }
> > > > > >  
> > > > > > -static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
> > > > > > +static
> > > > > > +void parse_NumaOptions(MachineState *ms, NumaOptions *object, Error **errp)      
> > > > > 
> > > > > I wonder if we should rename the parse_numa_{node,distance}()
> > > > > functions to configure_numa_{node,distance}(), and this one
> > > > > configure_numa().  These functions don't parse anything, anymore.    
> > > > I'd preffer to do it in another patch on top of this series
> > > > (added it my TODO list)    
> > > 
> > > I agree with renaming parse_numa*() later, but the new function
> > > you are creating can have a more reasonable name as it doesn't
> > > parse anything.  
> > 
> > 
> > how about: s/parse_NumaOptions/set_NumaOptions/  
> 
> No strong preference.  "register", "configure", "set" would all
> be good enough to me.  I would avoid the weird
> underline_and_CamelCase naming style, though (this style seems to
> be used only by generated QAPI code).
I'll fix it on respin

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Igor Mammedov]

On Thu, 29 Mar 2018 13:24:09 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Mar 29, 2018 at 01:43:03PM +0200, Igor Mammedov wrote:
> > On Wed, 28 Mar 2018 16:21:48 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Wed, Mar 28, 2018 at 01:48:35PM +0200, Igor Mammedov wrote:  
> > > > On Tue, 27 Mar 2018 17:05:41 +0200
> > > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > >     
> > > > > On Fri, 23 Mar 2018 18:25:08 -0300
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]
> > > This doesn't make sense to me.  Why would we enter
> > > RUN_STATE_PRECONFIG state if -preconfig is not used at all?  
> > because of RUN_STATE_PRECONFIG becomes new initial state of
> > our state machine where we start of (used to be RUN_STATE_PRELAUNCH)  
> 
> Oh, I missed that part.
> 
> > Lets call it variant 1:
> > 
> > with this we have 2 possible transitions:
> >  RUN_STATE_PRECONFIG -> RUN_STATE_PRELAUNCH (machine_init)
> > 
> > and
> > 
> >  RUN_STATE_PRECONFIG -> RUN_STATE_INMIGRATE
> >    ugly but it was the same with RUN_STATE_PRELAUNCH initial transition  
[...]
> 
> Thanks, now variant 1 makes more sense to me.  But I really miss
> here are very clear and explicit descriptions of what each state
> really mean, and what are the differences between them.
> 
> It looks like the existing description for `prelaunch` isn't
> accurate:
> 
> # @prelaunch: QEMU was started with -S and guest has not started
> 
> This is false, as QEMU can be in `prelaunch` state even if -S is
> not used.
> 
> 
> Also, this is the description you proposed for `preconfig`:
> 
> # @preconfig: QEMU is paused before board specific init callback is executed.
> #             The state is reachable only if -preconfig CLI option is used.
> #             (Since 2.12)
> 
> This seems wrong as well: the `prelaunch` state is reachable even
> if `-preconfig` isn't used in the command-line (because it is the
> initial state).
I'm not sure we should describe transitions/initial state here
(it's not the case now).

I think descriptions 'almost' match 'end' result of what QMP client
cloud see and the initial state is not something that QMP user could
observe.

[...]

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Eduardo Habkost]

On Tue, Apr 03, 2018 at 04:32:53PM +0200, Igor Mammedov wrote:
> On Thu, 29 Mar 2018 13:24:09 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Mar 29, 2018 at 01:43:03PM +0200, Igor Mammedov wrote:
> > > On Wed, 28 Mar 2018 16:21:48 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Wed, Mar 28, 2018 at 01:48:35PM +0200, Igor Mammedov wrote:  
> > > > > On Tue, 27 Mar 2018 17:05:41 +0200
> > > > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > > >     
> > > > > > On Fri, 23 Mar 2018 18:25:08 -0300
> > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> [...]
> > > > This doesn't make sense to me.  Why would we enter
> > > > RUN_STATE_PRECONFIG state if -preconfig is not used at all?  
> > > because of RUN_STATE_PRECONFIG becomes new initial state of
> > > our state machine where we start of (used to be RUN_STATE_PRELAUNCH)  
> > 
> > Oh, I missed that part.
> > 
> > > Lets call it variant 1:
> > > 
> > > with this we have 2 possible transitions:
> > >  RUN_STATE_PRECONFIG -> RUN_STATE_PRELAUNCH (machine_init)
> > > 
> > > and
> > > 
> > >  RUN_STATE_PRECONFIG -> RUN_STATE_INMIGRATE
> > >    ugly but it was the same with RUN_STATE_PRELAUNCH initial transition  
> [...]
> > 
> > Thanks, now variant 1 makes more sense to me.  But I really miss
> > here are very clear and explicit descriptions of what each state
> > really mean, and what are the differences between them.
> > 
> > It looks like the existing description for `prelaunch` isn't
> > accurate:
> > 
> > # @prelaunch: QEMU was started with -S and guest has not started
> > 
> > This is false, as QEMU can be in `prelaunch` state even if -S is
> > not used.
> > 
> > 
> > Also, this is the description you proposed for `preconfig`:
> > 
> > # @preconfig: QEMU is paused before board specific init callback is executed.
> > #             The state is reachable only if -preconfig CLI option is used.
> > #             (Since 2.12)
> > 
> > This seems wrong as well: the `prelaunch` state is reachable even
> > if `-preconfig` isn't used in the command-line (because it is the
> > initial state).
> I'm not sure we should describe transitions/initial state here
> (it's not the case now).
> 
> I think descriptions 'almost' match 'end' result of what QMP client
> cloud see and the initial state is not something that QMP user could
> observe.

That's my impression as well: `prelaunch` should be visible
externally only if `-S` was used, and `preconfig` should be
visible externally only if `-preconfig` was used.

However, I miss documentation on what are the
expectations/requirements internally.  e.g. there's no
explanation why a reset request moves QEMU to
RUN_STATE_PRELAUNCH.  Ideally, each line in
runstate_transition_def would have a clear explanation for
when/why each transition happens.

But this isn't a requirement for the new feature you are
implementing, anyway.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Igor Mammedov]

On Tue, 3 Apr 2018 12:31:29 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Apr 03, 2018 at 04:32:53PM +0200, Igor Mammedov wrote:
> > On Thu, 29 Mar 2018 13:24:09 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Mar 29, 2018 at 01:43:03PM +0200, Igor Mammedov wrote:  
> > > > On Wed, 28 Mar 2018 16:21:48 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Wed, Mar 28, 2018 at 01:48:35PM +0200, Igor Mammedov wrote:    
> > > > > > On Tue, 27 Mar 2018 17:05:41 +0200
> > > > > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > > > >       
> > > > > > > On Fri, 23 Mar 2018 18:25:08 -0300
> > > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > [...]  
> > > > > This doesn't make sense to me.  Why would we enter
> > > > > RUN_STATE_PRECONFIG state if -preconfig is not used at all?    
> > > > because of RUN_STATE_PRECONFIG becomes new initial state of
> > > > our state machine where we start of (used to be RUN_STATE_PRELAUNCH)    
> > > 
> > > Oh, I missed that part.
> > >   
> > > > Lets call it variant 1:
> > > > 
> > > > with this we have 2 possible transitions:
> > > >  RUN_STATE_PRECONFIG -> RUN_STATE_PRELAUNCH (machine_init)
> > > > 
> > > > and
> > > > 
> > > >  RUN_STATE_PRECONFIG -> RUN_STATE_INMIGRATE
> > > >    ugly but it was the same with RUN_STATE_PRELAUNCH initial transition    
> > [...]  
> > > 
> > > Thanks, now variant 1 makes more sense to me.  But I really miss
> > > here are very clear and explicit descriptions of what each state
> > > really mean, and what are the differences between them.
> > > 
> > > It looks like the existing description for `prelaunch` isn't
> > > accurate:
> > > 
> > > # @prelaunch: QEMU was started with -S and guest has not started
> > > 
> > > This is false, as QEMU can be in `prelaunch` state even if -S is
> > > not used.
> > > 
> > > 
> > > Also, this is the description you proposed for `preconfig`:
> > > 
> > > # @preconfig: QEMU is paused before board specific init callback is executed.
> > > #             The state is reachable only if -preconfig CLI option is used.
> > > #             (Since 2.12)
> > > 
> > > This seems wrong as well: the `prelaunch` state is reachable even
> > > if `-preconfig` isn't used in the command-line (because it is the
> > > initial state).  
> > I'm not sure we should describe transitions/initial state here
> > (it's not the case now).
> > 
> > I think descriptions 'almost' match 'end' result of what QMP client
> > cloud see and the initial state is not something that QMP user could
> > observe.  
> 
> That's my impression as well: `prelaunch` should be visible
> externally only if `-S` was used, and `preconfig` should be
> visible externally only if `-preconfig` was used.
> 
> However, I miss documentation on what are the
> expectations/requirements internally.  e.g. there's no
> explanation why a reset request moves QEMU to
> RUN_STATE_PRELAUNCH.  Ideally, each line in
> runstate_transition_def would have a clear explanation for
> when/why each transition happens.
> 
> But this isn't a requirement for the new feature you are
> implementing, anyway.
I'll add TODO comment to inmigrate to transition I'm adding,
to tag it for future work.

+    /* Early switch to inmigrate state to allow
+     * -incoming CLI option work as it used to.
+     * TODO: delay actual switching to inmigrate
+     * state to the point after machine is built
+     * and remove this hack.
+     */
+    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

[...]
> Series allows to configure NUMA mapping at runtime using QMP
> interface. For that to happen it introduces a new '-preconfig' CLI option
> which allows to pause QEMU before machine_init() is run and
> adds new set-numa-node QMP command which in conjunction with
> query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
>
> Later we can modify other commands to run early, for example device_add.
> I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> stage it's considered hotplug already), so SPAPR had to work around the issue.

That instance is just stupidity / laziness, I think: we consider any
plug after machine creation a hot plug.  Real machines remain cold until
you press the power button.  Our virtual machines should remain cold
until they start running, i.e. with -S until the first "cont".

I vaguely remember me asking this before, but your answer didn't make it
into this cover letter, which gives me a pretext to ask again instead of
looking it up in the archives: what exactly prevents us from keeping the
machine cold enough for numa configuration until the first "cont"?

>
> Example of configuration session:
> $QEMU -smp 2 -preconfig ...
>
> QMP:
> # get CPUs layout for current target/machine/CLI
> -> {'execute': 'query-hotpluggable-cpus' }  
> <- {'return': [
>        {'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 1}, ... },
>        {'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 0}, ... }
>    ]}
>
> # configure 1st node
> -> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 0 } }  
> <- {'return': {}}
> -> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',   
>        'node-id': 0, 'core-id': 0, 'thread-id': 0, 'socket-id': 1, }
>    }
> <- {'return': {}}
>
> # configure 2nd node
> -> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 1 } }
> -> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',  
>        'node-id': 1, 'core-id': 0, 'thread-id': 0, 'socket-id': 0 }
>    }
> <- {'return': {}}
>
> # [optional] verify configuration
> -> {'execute': 'query-hotpluggable-cpus' }  
> <- {'return': [
>        {'props': {'core-id': 0, 'thread-id': 0, 'node-id': 0, 'socket-id': 1}, ... },
>        {'props': {'core-id': 0, 'thread-id': 0, 'node-id': 1, 'socket-id': 0}, ... }
>    ]}
>
>
> Git tree:
>     https://github.com/imammedo/qemu.git qmp_preconfig_v3
>
> Ref to v1:
>     https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03583.html
>     Message-Id: <1508170976-96869-1-git-send-email-imammedo@redhat.com>

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Eduardo Habkost]

On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> [...]
> > Series allows to configure NUMA mapping at runtime using QMP
> > interface. For that to happen it introduces a new '-preconfig' CLI option
> > which allows to pause QEMU before machine_init() is run and
> > adds new set-numa-node QMP command which in conjunction with
> > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> >
> > Later we can modify other commands to run early, for example device_add.
> > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > stage it's considered hotplug already), so SPAPR had to work around the issue.
> 
> That instance is just stupidity / laziness, I think: we consider any
> plug after machine creation a hot plug.  Real machines remain cold until
> you press the power button.  Our virtual machines should remain cold
> until they start running, i.e. with -S until the first "cont".
> 
> I vaguely remember me asking this before, but your answer didn't make it
> into this cover letter, which gives me a pretext to ask again instead of
> looking it up in the archives: what exactly prevents us from keeping the
> machine cold enough for numa configuration until the first "cont"?

I also think this would be better, but it seems to be difficult
in practice, see:
http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Igor Mammedov]

On Tue, 17 Apr 2018 11:27:39 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:
> > Igor Mammedov <imammedo@redhat.com> writes:
> > 
> > [...]  
> > > Series allows to configure NUMA mapping at runtime using QMP
> > > interface. For that to happen it introduces a new '-preconfig' CLI option
> > > which allows to pause QEMU before machine_init() is run and
> > > adds new set-numa-node QMP command which in conjunction with
> > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> > >
> > > Later we can modify other commands to run early, for example device_add.
> > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > > stage it's considered hotplug already), so SPAPR had to work around the issue.  
> > 
> > That instance is just stupidity / laziness, I think: we consider any
> > plug after machine creation a hot plug.  Real machines remain cold until
> > you press the power button.  Our virtual machines should remain cold
> > until they start running, i.e. with -S until the first "cont".
It probably would be too risky to change semantics of -S from hotplug to coldplug.
But even if we were easy it won't matter in case if dynamic configuration
done properly. More on it below.

> > I vaguely remember me asking this before, but your answer didn't make it
> > into this cover letter, which gives me a pretext to ask again instead of
> > looking it up in the archives: what exactly prevents us from keeping the
> > machine cold enough for numa configuration until the first "cont"?  
> 
> I also think this would be better, but it seems to be difficult
> in practice, see:
> http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain

In addition to Eduardo's reply, here is what I've answered back
when you've asked question the 1st time (v2 late at -S pause point reconfig):
https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html

In short:
I think it's wrong in general doing fixups after machine is build
instead of getting correct configuration before building machine.
That's going to be complex and fragile and might be hard to do at
all depending on what we are fixing up.

BTW this is an outdated version of series and there is a newer one v5
https://patchwork.ozlabs.org/cover/895315/
so pleases review it.

Short diff vs 1:
 - only limited(minimum) set of commands is available at preconfig stage for now
 - use QAPI schema to mark commands as preconfig enabled,
   so mgmt could see when it can use commands.
 - added preconfig runstate state-machine instead of adding more global variables
   to cleanly keep track of where QEMU is paused and what it's allowed to do

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Eduardo Habkost]

On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:
> On Tue, 17 Apr 2018 11:27:39 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:
> > > Igor Mammedov <imammedo@redhat.com> writes:
> > > 
> > > [...]  
> > > > Series allows to configure NUMA mapping at runtime using QMP
> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
> > > > which allows to pause QEMU before machine_init() is run and
> > > > adds new set-numa-node QMP command which in conjunction with
> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> > > >
> > > > Later we can modify other commands to run early, for example device_add.
> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.  
> > > 
> > > That instance is just stupidity / laziness, I think: we consider any
> > > plug after machine creation a hot plug.  Real machines remain cold until
> > > you press the power button.  Our virtual machines should remain cold
> > > until they start running, i.e. with -S until the first "cont".
> It probably would be too risky to change semantics of -S from hotplug to coldplug.
> But even if we were easy it won't matter in case if dynamic configuration
> done properly. More on it below.
> 
> > > I vaguely remember me asking this before, but your answer didn't make it
> > > into this cover letter, which gives me a pretext to ask again instead of
> > > looking it up in the archives: what exactly prevents us from keeping the
> > > machine cold enough for numa configuration until the first "cont"?  
> > 
> > I also think this would be better, but it seems to be difficult
> > in practice, see:
> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain
> 
> In addition to Eduardo's reply, here is what I've answered back
> when you've asked question the 1st time (v2 late at -S pause point reconfig):
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
> 
> In short:
> I think it's wrong in general doing fixups after machine is build
> instead of getting correct configuration before building machine.
> That's going to be complex and fragile and might be hard to do at
> all depending on what we are fixing up.

What "building the machine" should mean, exactly, for external
users?

The main question I'd like to see answered is: why exactly we
must "build" the machine before the first "cont" is issued when
using -S?  Why can't we delay everything to "cont" when using -S?

Is it just because it's a long and complex task?  Does that mean
we might still do that eventually, and eliminate the
prelaunch/preconfig distinction in the distant future?

Even if we follow your approach, we need to answer these
questions.  I'm sure we will try to reorder initialization steps
between the preconfig/prelaunch states in the future, and we
shouldn't break any expectations from external users when doing
that.

> 
> BTW this is an outdated version of series and there is a newer one v5
> https://patchwork.ozlabs.org/cover/895315/
> so pleases review it.
> 
> Short diff vs 1:
>  - only limited(minimum) set of commands is available at preconfig stage for now
>  - use QAPI schema to mark commands as preconfig enabled,
>    so mgmt could see when it can use commands.
>  - added preconfig runstate state-machine instead of adding more global variables
>    to cleanly keep track of where QEMU is paused and what it's allowed to do

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:
>> On Tue, 17 Apr 2018 11:27:39 -0300
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> 
>> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:
>> > > Igor Mammedov <imammedo@redhat.com> writes:
>> > > 
>> > > [...]  
>> > > > Series allows to configure NUMA mapping at runtime using QMP
>> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
>> > > > which allows to pause QEMU before machine_init() is run and
>> > > > adds new set-numa-node QMP command which in conjunction with
>> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
>> > > >
>> > > > Later we can modify other commands to run early, for example device_add.
>> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
>> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
>> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.  
>> > > 
>> > > That instance is just stupidity / laziness, I think: we consider any
>> > > plug after machine creation a hot plug.  Real machines remain cold until
>> > > you press the power button.  Our virtual machines should remain cold
>> > > until they start running, i.e. with -S until the first "cont".
>> It probably would be too risky to change semantics of -S from hotplug to coldplug.
>> But even if we were easy it won't matter in case if dynamic configuration
>> done properly. More on it below.
>> 
>> > > I vaguely remember me asking this before, but your answer didn't make it
>> > > into this cover letter, which gives me a pretext to ask again instead of
>> > > looking it up in the archives: what exactly prevents us from keeping the
>> > > machine cold enough for numa configuration until the first "cont"?  
>> > 
>> > I also think this would be better, but it seems to be difficult
>> > in practice, see:
>> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain
>> 
>> In addition to Eduardo's reply, here is what I've answered back
>> when you've asked question the 1st time (v2 late at -S pause point reconfig):
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
>> 
>> In short:
>> I think it's wrong in general doing fixups after machine is build
>> instead of getting correct configuration before building machine.
>> That's going to be complex and fragile and might be hard to do at
>> all depending on what we are fixing up.
>
> What "building the machine" should mean, exactly, for external
> users?
>
> The main question I'd like to see answered is: why exactly we
> must "build" the machine before the first "cont" is issued when
> using -S?  Why can't we delay everything to "cont" when using -S?

Exactly.

> Is it just because it's a long and complex task?  Does that mean
> we might still do that eventually, and eliminate the
> prelaunch/preconfig distinction in the distant future?

Why would anyone want to use -S going forward?  For reasons other "we've
always used -S, and can't be bothered to change".

> Even if we follow your approach, we need to answer these
> questions.  I'm sure we will try to reorder initialization steps
> between the preconfig/prelaunch states in the future, and we
> shouldn't break any expectations from external users when doing
> that.

Moreover, the questions need to be answered in Git.  Commit message,
comments, docs/, use your judgement.

>> BTW this is an outdated version of series and there is a newer one v5
>> https://patchwork.ozlabs.org/cover/895315/

Sorry about that.  I'm drowning in a sea of two months worth of patches.

>> so pleases review it.
>> 
>> Short diff vs 1:
>>  - only limited(minimum) set of commands is available at preconfig stage for now
>>  - use QAPI schema to mark commands as preconfig enabled,
>>    so mgmt could see when it can use commands.
>>  - added preconfig runstate state-machine instead of adding more global variables
>>    to cleanly keep track of where QEMU is paused and what it's allowed to do

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Igor Mammedov]

On Wed, 18 Apr 2018 09:08:30 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:  
> >> On Tue, 17 Apr 2018 11:27:39 -0300
> >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>   
> >> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:  
> >> > > Igor Mammedov <imammedo@redhat.com> writes:
> >> > > 
> >> > > [...]    
> >> > > > Series allows to configure NUMA mapping at runtime using QMP
> >> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
> >> > > > which allows to pause QEMU before machine_init() is run and
> >> > > > adds new set-numa-node QMP command which in conjunction with
> >> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> >> > > >
> >> > > > Later we can modify other commands to run early, for example device_add.
> >> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> >> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> >> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.    
> >> > > 
> >> > > That instance is just stupidity / laziness, I think: we consider any
> >> > > plug after machine creation a hot plug.  Real machines remain cold until
> >> > > you press the power button.  Our virtual machines should remain cold
> >> > > until they start running, i.e. with -S until the first "cont".  
> >> It probably would be too risky to change semantics of -S from hotplug to coldplug.
> >> But even if we were easy it won't matter in case if dynamic configuration
> >> done properly. More on it below.
> >>   
> >> > > I vaguely remember me asking this before, but your answer didn't make it
> >> > > into this cover letter, which gives me a pretext to ask again instead of
> >> > > looking it up in the archives: what exactly prevents us from keeping the
> >> > > machine cold enough for numa configuration until the first "cont"?    
> >> > 
> >> > I also think this would be better, but it seems to be difficult
> >> > in practice, see:
> >> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain  
> >> 
> >> In addition to Eduardo's reply, here is what I've answered back
> >> when you've asked question the 1st time (v2 late at -S pause point reconfig):
> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
> >> 
> >> In short:
> >> I think it's wrong in general doing fixups after machine is build
> >> instead of getting correct configuration before building machine.
> >> That's going to be complex and fragile and might be hard to do at
> >> all depending on what we are fixing up.  
> >
> > What "building the machine" should mean, exactly, for external
> > users?
under "building machine", I've meant machine_run_board_init()
and all follow up steps to machine_done stage.

> > The main question I'd like to see answered is: why exactly we
> > must "build" the machine before the first "cont" is issued when
> > using -S?  Why can't we delay everything to "cont" when using -S?  
Nor sure what question is about,
Did you mean if it were possible to postpone machine_run_board_init()
and all later steps to -S/cont time?
 
> > Is it just because it's a long and complex task?  Does that mean
> > we might still do that eventually, and eliminate the
> > prelaunch/preconfig distinction in the distant future?  
> 
> Why would anyone want to use -S going forward?  For reasons other "we've
> always used -S, and can't be bothered to change".
We should be able to deprecate/remove -S once we can do all
initial configuration that's possible to do there at
preconfig time.
 
> > Even if we follow your approach, we need to answer these
> > questions.  I'm sure we will try to reorder initialization steps
> > between the preconfig/prelaunch states in the future, and we
> > shouldn't break any expectations from external users when doing
> > that.
As minimum I expect -preconfig to be a runtime equivalent to CLI,
with difference that it will be interactive and use QMP interface.
As long as it sits between CLI parsing and the rest of initialization
it shouldn't break that.

> Moreover, the questions need to be answered in Git.  Commit message,
> comments, docs/, use your judgement.
I've thought that commit messages/patches were describing introduced
changes sufficiently. But I've been sitting on these patches for
a long time and what's obvious to me might be not so clear to others.
I might just not see what's missing. Any suggestions to improve it
are welcome.

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Eduardo Habkost]

On Thu, Apr 19, 2018 at 10:00:04AM +0200, Igor Mammedov wrote:
> On Wed, 18 Apr 2018 09:08:30 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > 
> > > On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:  
> > >> On Tue, 17 Apr 2018 11:27:39 -0300
> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >>   
> > >> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:  
> > >> > > Igor Mammedov <imammedo@redhat.com> writes:
> > >> > > 
> > >> > > [...]    
> > >> > > > Series allows to configure NUMA mapping at runtime using QMP
> > >> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
> > >> > > > which allows to pause QEMU before machine_init() is run and
> > >> > > > adds new set-numa-node QMP command which in conjunction with
> > >> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> > >> > > >
> > >> > > > Later we can modify other commands to run early, for example device_add.
> > >> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > >> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > >> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.    
> > >> > > 
> > >> > > That instance is just stupidity / laziness, I think: we consider any
> > >> > > plug after machine creation a hot plug.  Real machines remain cold until
> > >> > > you press the power button.  Our virtual machines should remain cold
> > >> > > until they start running, i.e. with -S until the first "cont".  
> > >> It probably would be too risky to change semantics of -S from hotplug to coldplug.
> > >> But even if we were easy it won't matter in case if dynamic configuration
> > >> done properly. More on it below.
> > >>   
> > >> > > I vaguely remember me asking this before, but your answer didn't make it
> > >> > > into this cover letter, which gives me a pretext to ask again instead of
> > >> > > looking it up in the archives: what exactly prevents us from keeping the
> > >> > > machine cold enough for numa configuration until the first "cont"?    
> > >> > 
> > >> > I also think this would be better, but it seems to be difficult
> > >> > in practice, see:
> > >> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain  
> > >> 
> > >> In addition to Eduardo's reply, here is what I've answered back
> > >> when you've asked question the 1st time (v2 late at -S pause point reconfig):
> > >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
> > >> 
> > >> In short:
> > >> I think it's wrong in general doing fixups after machine is build
> > >> instead of getting correct configuration before building machine.
> > >> That's going to be complex and fragile and might be hard to do at
> > >> all depending on what we are fixing up.  
> > >
> > > What "building the machine" should mean, exactly, for external
> > > users?
> under "building machine", I've meant machine_run_board_init()
> and all follow up steps to machine_done stage.
> 
> > > The main question I'd like to see answered is: why exactly we
> > > must "build" the machine before the first "cont" is issued when
> > > using -S?  Why can't we delay everything to "cont" when using -S?  
> Nor sure what question is about,
> Did you mean if it were possible to postpone machine_run_board_init()
> and all later steps to -S/cont time?

Exactly.  In other words, what exactly must be done before the
monitor is available when using -S, and what exactly can be
postponed after "cont" when using -S?

>  
> > > Is it just because it's a long and complex task?  Does that mean
> > > we might still do that eventually, and eliminate the
> > > prelaunch/preconfig distinction in the distant future?  
> > 
> > Why would anyone want to use -S going forward?  For reasons other "we've
> > always used -S, and can't be bothered to change".
> We should be able to deprecate/remove -S once we can do all
> initial configuration that's possible to do there at
> preconfig time.

If the plan is to deprecate -S, what are the important
user-visible differences between -S and -preconfig today?  Do we
plan to eliminate all those differences before
deprecating/removing -S?

>  
> > > Even if we follow your approach, we need to answer these
> > > questions.  I'm sure we will try to reorder initialization steps
> > > between the preconfig/prelaunch states in the future, and we
> > > shouldn't break any expectations from external users when doing
> > > that.
> As minimum I expect -preconfig to be a runtime equivalent to CLI,
> with difference that it will be interactive and use QMP interface.
> As long as it sits between CLI parsing and the rest of initialization
> it shouldn't break that.

What prevents us from making -S a runtime equivalent to CLI?

> 
> > Moreover, the questions need to be answered in Git.  Commit message,
> > comments, docs/, use your judgement.
> I've thought that commit messages/patches were describing introduced
> changes sufficiently. But I've been sitting on these patches for
> a long time and what's obvious to me might be not so clear to others.
> I might just not see what's missing. Any suggestions to improve it
> are welcome.

I miss something that documents why both -S and -preconfig need
to exist, what are the differences between them today, and what
we plan to do about the differences between them in the future.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3811 bytes --]

On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> [...]
> > Series allows to configure NUMA mapping at runtime using QMP
> > interface. For that to happen it introduces a new '-preconfig' CLI option
> > which allows to pause QEMU before machine_init() is run and
> > adds new set-numa-node QMP command which in conjunction with
> > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> >
> > Later we can modify other commands to run early, for example device_add.
> > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > stage it's considered hotplug already), so SPAPR had to work around the issue.
> 
> That instance is just stupidity / laziness, I think: we consider any
> plug after machine creation a hot plug.  Real machines remain cold until
> you press the power button.  Our virtual machines should remain cold
> until they start running, i.e. with -S until the first "cont".

Makes sense to me.  As I recall, the chief problem I had here with
PAPR hotplug was that -S stopped *after* the machine_reset, which
meant the only mechanisms we really had to inform the guest of the
hardware were the hotplug mechanisms.  Sort of.  It got complicated
because of the feature negotiation system that PAPR guests have.

At present -S doesn't really operate like a "stop before hitting the
power", it instead acts more like a breakpoint on the first
instruction of the firmware.

If we were to move the -S stop to before the machine_reset, I believe
that might simplify several things in our hotplug handling code for
papr.

> I vaguely remember me asking this before, but your answer didn't make it
> into this cover letter, which gives me a pretext to ask again instead of
> looking it up in the archives: what exactly prevents us from keeping the
> machine cold enough for numa configuration until the first "cont"?
> 
> >
> > Example of configuration session:
> > $QEMU -smp 2 -preconfig ...
> >
> > QMP:
> > # get CPUs layout for current target/machine/CLI
> > -> {'execute': 'query-hotpluggable-cpus' }  
> > <- {'return': [
> >        {'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 1}, ... },
> >        {'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 0}, ... }
> >    ]}
> >
> > # configure 1st node
> > -> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 0 } }  
> > <- {'return': {}}
> > -> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',   
> >        'node-id': 0, 'core-id': 0, 'thread-id': 0, 'socket-id': 1, }
> >    }
> > <- {'return': {}}
> >
> > # configure 2nd node
> > -> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 1 } }
> > -> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',  
> >        'node-id': 1, 'core-id': 0, 'thread-id': 0, 'socket-id': 0 }
> >    }
> > <- {'return': {}}
> >
> > # [optional] verify configuration
> > -> {'execute': 'query-hotpluggable-cpus' }  
> > <- {'return': [
> >        {'props': {'core-id': 0, 'thread-id': 0, 'node-id': 0, 'socket-id': 1}, ... },
> >        {'props': {'core-id': 0, 'thread-id': 0, 'node-id': 1, 'socket-id': 0}, ... }
> >    ]}
> >
> >
> > Git tree:
> >     https://github.com/imammedo/qemu.git qmp_preconfig_v3
> >
> > Ref to v1:
> >     https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03583.html
> >     Message-Id: <1508170976-96869-1-git-send-email-imammedo@redhat.com>
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Apr 19, 2018 at 10:00:04AM +0200, Igor Mammedov wrote:
>> On Wed, 18 Apr 2018 09:08:30 +0200
>> Markus Armbruster <armbru@redhat.com> wrote:
>> 
>> > Eduardo Habkost <ehabkost@redhat.com> writes:
>> > 
>> > > On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:  
>> > >> On Tue, 17 Apr 2018 11:27:39 -0300
>> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > >>   
>> > >> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:  
>> > >> > > Igor Mammedov <imammedo@redhat.com> writes:
>> > >> > > 
>> > >> > > [...]    
>> > >> > > > Series allows to configure NUMA mapping at runtime using QMP
>> > >> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
>> > >> > > > which allows to pause QEMU before machine_init() is run and
>> > >> > > > adds new set-numa-node QMP command which in conjunction with
>> > >> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
>> > >> > > >
>> > >> > > > Later we can modify other commands to run early, for example device_add.
>> > >> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
>> > >> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
>> > >> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.    
>> > >> > > 
>> > >> > > That instance is just stupidity / laziness, I think: we consider any
>> > >> > > plug after machine creation a hot plug.  Real machines remain cold until
>> > >> > > you press the power button.  Our virtual machines should remain cold
>> > >> > > until they start running, i.e. with -S until the first "cont".  
>> > >> It probably would be too risky to change semantics of -S from hotplug to coldplug.
>> > >> But even if we were easy it won't matter in case if dynamic configuration
>> > >> done properly. More on it below.
>> > >>   
>> > >> > > I vaguely remember me asking this before, but your answer didn't make it
>> > >> > > into this cover letter, which gives me a pretext to ask again instead of
>> > >> > > looking it up in the archives: what exactly prevents us from keeping the
>> > >> > > machine cold enough for numa configuration until the first "cont"?    
>> > >> > 
>> > >> > I also think this would be better, but it seems to be difficult
>> > >> > in practice, see:
>> > >> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain  
>> > >> 
>> > >> In addition to Eduardo's reply, here is what I've answered back
>> > >> when you've asked question the 1st time (v2 late at -S pause point reconfig):
>> > >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
>> > >> 
>> > >> In short:
>> > >> I think it's wrong in general doing fixups after machine is build
>> > >> instead of getting correct configuration before building machine.
>> > >> That's going to be complex and fragile and might be hard to do at
>> > >> all depending on what we are fixing up.  
>> > >
>> > > What "building the machine" should mean, exactly, for external
>> > > users?
>> under "building machine", I've meant machine_run_board_init()
>> and all follow up steps to machine_done stage.
>> 
>> > > The main question I'd like to see answered is: why exactly we
>> > > must "build" the machine before the first "cont" is issued when
>> > > using -S?  Why can't we delay everything to "cont" when using -S?  
>> Nor sure what question is about,
>> Did you mean if it were possible to postpone machine_run_board_init()
>> and all later steps to -S/cont time?
>
> Exactly.  In other words, what exactly must be done before the
> monitor is available when using -S, and what exactly can be
> postponed after "cont" when using -S?
>
>>  
>> > > Is it just because it's a long and complex task?  Does that mean
>> > > we might still do that eventually, and eliminate the
>> > > prelaunch/preconfig distinction in the distant future?  
>> > 
>> > Why would anyone want to use -S going forward?  For reasons other "we've
>> > always used -S, and can't be bothered to change".
>> We should be able to deprecate/remove -S once we can do all
>> initial configuration that's possible to do there at
>> preconfig time.

This sounds like there are things we can do with -S but can't
--preconfig now.  Is that correct?

If yes, I got another question.  If you want to configure NUMA, you need
to use --preconfig.  If you also want to configure one of the things you
can't with --preconfig, you need to use -S.  In other words, you need to
use both.  How would that work?

> If the plan is to deprecate -S, what are the important
> user-visible differences between -S and -preconfig today?  Do we
> plan to eliminate all those differences before
> deprecating/removing -S?

Documentation (including -help) needs to provide clear guidance on what
to use when.

>> > > Even if we follow your approach, we need to answer these
>> > > questions.  I'm sure we will try to reorder initialization steps
>> > > between the preconfig/prelaunch states in the future, and we
>> > > shouldn't break any expectations from external users when doing
>> > > that.
>> As minimum I expect -preconfig to be a runtime equivalent to CLI,
>> with difference that it will be interactive and use QMP interface.
>> As long as it sits between CLI parsing and the rest of initialization
>> it shouldn't break that.
>
> What prevents us from making -S a runtime equivalent to CLI?

Good question.  Igor?

>> > Moreover, the questions need to be answered in Git.  Commit message,
>> > comments, docs/, use your judgement.
>> I've thought that commit messages/patches were describing introduced
>> changes sufficiently.

Keep in mind that your poor reviewers context-switch to your patches
from somewhere else entirely, then read (mostly) linearly, starting with
the cover letter.  If the cover letter leads to questions and confusion,
the reading likely stops.  Answers in the commit messages won't do you a
lick of good then.

It's really, really useful to state the case for the feature right in
the cover letter.  "What" and "why", not "how".  Write for your
audience!  The cover letter's audience knows nothing (first order
approximation).  Don't make them guess.  Especially not "why".  Keep it
high-level.  Pointers to (specific!) commit messages for additional
detail can make sense.

>>                       But I've been sitting on these patches for
>> a long time and what's obvious to me might be not so clear to others.

Par for the course, don't feel bad about it.

>> I might just not see what's missing. Any suggestions to improve it
>> are welcome.
>
> I miss something that documents why both -S and -preconfig need
> to exist, what are the differences between them today, and what
> we plan to do about the differences between them in the future.

Yes, please.  Additionally, guidance for users on which of them to use.

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Igor Mammedov]

On Fri, 20 Apr 2018 08:31:18 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Apr 19, 2018 at 10:00:04AM +0200, Igor Mammedov wrote:  
> >> On Wed, 18 Apr 2018 09:08:30 +0200
> >> Markus Armbruster <armbru@redhat.com> wrote:
> >>   
> >> > Eduardo Habkost <ehabkost@redhat.com> writes:
> >> >   
> >> > > On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:    
> >> > >> On Tue, 17 Apr 2018 11:27:39 -0300
> >> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > >>     
> >> > >> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:    
> >> > >> > > Igor Mammedov <imammedo@redhat.com> writes:
> >> > >> > > 
> >> > >> > > [...]      
> >> > >> > > > Series allows to configure NUMA mapping at runtime using QMP
> >> > >> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
> >> > >> > > > which allows to pause QEMU before machine_init() is run and
> >> > >> > > > adds new set-numa-node QMP command which in conjunction with
> >> > >> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> >> > >> > > >
> >> > >> > > > Later we can modify other commands to run early, for example device_add.
> >> > >> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> >> > >> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> >> > >> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.      
> >> > >> > > 
> >> > >> > > That instance is just stupidity / laziness, I think: we consider any
> >> > >> > > plug after machine creation a hot plug.  Real machines remain cold until
> >> > >> > > you press the power button.  Our virtual machines should remain cold
> >> > >> > > until they start running, i.e. with -S until the first "cont".    
> >> > >> It probably would be too risky to change semantics of -S from hotplug to coldplug.
> >> > >> But even if we were easy it won't matter in case if dynamic configuration
> >> > >> done properly. More on it below.
> >> > >>     
> >> > >> > > I vaguely remember me asking this before, but your answer didn't make it
> >> > >> > > into this cover letter, which gives me a pretext to ask again instead of
> >> > >> > > looking it up in the archives: what exactly prevents us from keeping the
> >> > >> > > machine cold enough for numa configuration until the first "cont"?      
> >> > >> > 
> >> > >> > I also think this would be better, but it seems to be difficult
> >> > >> > in practice, see:
> >> > >> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain    
> >> > >> 
> >> > >> In addition to Eduardo's reply, here is what I've answered back
> >> > >> when you've asked question the 1st time (v2 late at -S pause point reconfig):
> >> > >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
> >> > >> 
> >> > >> In short:
> >> > >> I think it's wrong in general doing fixups after machine is build
> >> > >> instead of getting correct configuration before building machine.
> >> > >> That's going to be complex and fragile and might be hard to do at
> >> > >> all depending on what we are fixing up.    
> >> > >
> >> > > What "building the machine" should mean, exactly, for external
> >> > > users?  
> >> under "building machine", I've meant machine_run_board_init()
> >> and all follow up steps to machine_done stage.
> >>   
> >> > > The main question I'd like to see answered is: why exactly we
> >> > > must "build" the machine before the first "cont" is issued when
> >> > > using -S?  Why can't we delay everything to "cont" when using -S?    
> >> Nor sure what question is about,
> >> Did you mean if it were possible to postpone machine_run_board_init()
> >> and all later steps to -S/cont time?  
(1)
As David said -S pause point is practically breakpoint on some
instruction of built/existing machine and current monitor commands
expect it to be valid. Moving -S before machine_run_board_init()
will break semantics of current -S pause point (i.e. user expectation
on existing machine) as well as most of the commands that evolved
in environment where machine already existed.

Hence a new -preconfig option and runstate to avoid breaking
exiting users and being able to cleanly handle configuration that
affects machine_run_board_init().

> > Exactly.  In other words, what exactly must be done before the
> > monitor is available when using -S,
for MUST, it should be commands that affect machine_run_board_init()
like being added set-numa-node

> > and what exactly can be postponed after "cont" when using -S?
hotplug configuration and various runtime query commands that
expect built machine. (today it's most of the commands)

wrt configuration commands we should split them into coldplug
and hotplug ones (some could be both).
   
> >> > > Is it just because it's a long and complex task?  Does that mean
> >> > > we might still do that eventually, and eliminate the
> >> > > prelaunch/preconfig distinction in the distant future?    
> >> > 
> >> > Why would anyone want to use -S going forward?  For reasons other "we've
> >> > always used -S, and can't be bothered to change".  
> >> We should be able to deprecate/remove -S once we can do all
> >> initial configuration that's possible to do there at
> >> preconfig time.  
> 
> This sounds like there are things we can do with -S but can't
> --preconfig now.  Is that correct?
yes, we can't do at --preconfig time anything that requires built machine.

> If yes, I got another question.  If you want to configure NUMA, you need
> to use --preconfig.  If you also want to configure one of the things you
> can't with --preconfig, you need to use -S.  In other words, you need to
> use both.  How would that work?
It works and described in commit message of
"[PATCH for-2.13 v5 03/11] cli: add --preconfig option"
https://patchwork.kernel.org/patch/10323879/

"
When early introspection/configuration is done, command 'cont' 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)
"
> 
> > If the plan is to deprecate -S, what are the important
> > user-visible differences between -S and -preconfig today?  Do we
> > plan to eliminate all those differences before
> > deprecating/removing -S?
we probably won't be able to deprecate -S in foreseeable future,
for that we would need to be able to do everything starting from
machine_run_board_init() to current pause point.
But we can gradually move configuration commands to -preconfig time
and gradually add CLI equivalents for that aren't possible at -S time
(like Paolo suggested picking to be used machine model at runtime)

> Documentation (including -help) needs to provide clear guidance on what
> to use when.
for -help part the same '[PATCH for-2.13 v5 03/11] cli: add --preconfig option'
As for commands that are allowed to run at --preconfig time, it is
documented in QAPI schema
'[PATCH for-2.13 v5 05/11] qapi: introduce new cmd  option "allowed-in-preconfig"'
http://patchwork.ozlabs.org/patch/895314/

> >> > > Even if we follow your approach, we need to answer these
> >> > > questions.  I'm sure we will try to reorder initialization steps
> >> > > between the preconfig/prelaunch states in the future, and we
> >> > > shouldn't break any expectations from external users when doing
> >> > > that.  
> >> As minimum I expect -preconfig to be a runtime equivalent to CLI,
> >> with difference that it will be interactive and use QMP interface.
> >> As long as it sits between CLI parsing and the rest of initialization
> >> it shouldn't break that.  
> >
> > What prevents us from making -S a runtime equivalent to CLI?  
Possibly [1] earlier answers it.

> Good question.  Igor?
> 
> >> > Moreover, the questions need to be answered in Git.  Commit message,
> >> > comments, docs/, use your judgement.  
> >> I've thought that commit messages/patches were describing introduced
> >> changes sufficiently.  
> 
> Keep in mind that your poor reviewers context-switch to your patches
> from somewhere else entirely, then read (mostly) linearly, starting with
> the cover letter.  If the cover letter leads to questions and confusion,
> the reading likely stops.  Answers in the commit messages won't do you a
> lick of good then.
> 
> It's really, really useful to state the case for the feature right in
> the cover letter.  "What" and "why", not "how".  Write for your
> audience!  The cover letter's audience knows nothing (first order
> approximation).  Don't make them guess.  Especially not "why".  Keep it
> high-level.  Pointers to (specific!) commit messages for additional
> detail can make sense.
I'll try to write better cover letter if I end up respinning v5

> >>                       But I've been sitting on these patches for
> >> a long time and what's obvious to me might be not so clear to others.  
> 
> Par for the course, don't feel bad about it.
> 
> >> I might just not see what's missing. Any suggestions to improve it
> >> are welcome.  
> >
> > I miss something that documents why both -S and -preconfig need
> > to exist, what are the differences between them today, and what
> > we plan to do about the differences between them in the future.
Where would you prefer it being documented?

> Yes, please.  Additionally, guidance for users on which of them to use.

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Eduardo Habkost]

On Mon, Apr 23, 2018 at 11:50:16AM +0200, Igor Mammedov wrote:
> On Fri, 20 Apr 2018 08:31:18 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > 
> > > On Thu, Apr 19, 2018 at 10:00:04AM +0200, Igor Mammedov wrote:  
> > >> On Wed, 18 Apr 2018 09:08:30 +0200
> > >> Markus Armbruster <armbru@redhat.com> wrote:
> > >>   
> > >> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > >> >   
> > >> > > On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:    
> > >> > >> On Tue, 17 Apr 2018 11:27:39 -0300
> > >> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >> > >>     
> > >> > >> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:    
> > >> > >> > > Igor Mammedov <imammedo@redhat.com> writes:
> > >> > >> > > 
> > >> > >> > > [...]      
> > >> > >> > > > Series allows to configure NUMA mapping at runtime using QMP
> > >> > >> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
> > >> > >> > > > which allows to pause QEMU before machine_init() is run and
> > >> > >> > > > adds new set-numa-node QMP command which in conjunction with
> > >> > >> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> > >> > >> > > >
> > >> > >> > > > Later we can modify other commands to run early, for example device_add.
> > >> > >> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > >> > >> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > >> > >> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.      
> > >> > >> > > 
> > >> > >> > > That instance is just stupidity / laziness, I think: we consider any
> > >> > >> > > plug after machine creation a hot plug.  Real machines remain cold until
> > >> > >> > > you press the power button.  Our virtual machines should remain cold
> > >> > >> > > until they start running, i.e. with -S until the first "cont".    
> > >> > >> It probably would be too risky to change semantics of -S from hotplug to coldplug.
> > >> > >> But even if we were easy it won't matter in case if dynamic configuration
> > >> > >> done properly. More on it below.
> > >> > >>     
> > >> > >> > > I vaguely remember me asking this before, but your answer didn't make it
> > >> > >> > > into this cover letter, which gives me a pretext to ask again instead of
> > >> > >> > > looking it up in the archives: what exactly prevents us from keeping the
> > >> > >> > > machine cold enough for numa configuration until the first "cont"?      
> > >> > >> > 
> > >> > >> > I also think this would be better, but it seems to be difficult
> > >> > >> > in practice, see:
> > >> > >> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain    
> > >> > >> 
> > >> > >> In addition to Eduardo's reply, here is what I've answered back
> > >> > >> when you've asked question the 1st time (v2 late at -S pause point reconfig):
> > >> > >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
> > >> > >> 
> > >> > >> In short:
> > >> > >> I think it's wrong in general doing fixups after machine is build
> > >> > >> instead of getting correct configuration before building machine.
> > >> > >> That's going to be complex and fragile and might be hard to do at
> > >> > >> all depending on what we are fixing up.    
> > >> > >
> > >> > > What "building the machine" should mean, exactly, for external
> > >> > > users?  
> > >> under "building machine", I've meant machine_run_board_init()
> > >> and all follow up steps to machine_done stage.
> > >>   
> > >> > > The main question I'd like to see answered is: why exactly we
> > >> > > must "build" the machine before the first "cont" is issued when
> > >> > > using -S?  Why can't we delay everything to "cont" when using -S?    
> > >> Nor sure what question is about,
> > >> Did you mean if it were possible to postpone machine_run_board_init()
> > >> and all later steps to -S/cont time?  
> (1)
> As David said -S pause point is practically breakpoint on some
> instruction of built/existing machine and current monitor commands
> expect it to be valid. Moving -S before machine_run_board_init()
> will break semantics of current -S pause point (i.e. user expectation
> on existing machine) as well as most of the commands that evolved
> in environment where machine already existed.

OK, so what's missing here is a clear description what the user
can expect on -S.

> 
> Hence a new -preconfig option and runstate to avoid breaking
> exiting users and being able to cleanly handle configuration that
> affects machine_run_board_init().
> 
> > > Exactly.  In other words, what exactly must be done before the
> > > monitor is available when using -S,
> for MUST, it should be commands that affect machine_run_board_init()
> like being added set-numa-node
> 
> > > and what exactly can be postponed after "cont" when using -S?
> hotplug configuration and various runtime query commands that
> expect built machine. (today it's most of the commands)
> 
> wrt configuration commands we should split them into coldplug
> and hotplug ones (some could be both).
>    
> > >> > > Is it just because it's a long and complex task?  Does that mean
> > >> > > we might still do that eventually, and eliminate the
> > >> > > prelaunch/preconfig distinction in the distant future?    
> > >> > 
> > >> > Why would anyone want to use -S going forward?  For reasons other "we've
> > >> > always used -S, and can't be bothered to change".  
> > >> We should be able to deprecate/remove -S once we can do all
> > >> initial configuration that's possible to do there at
> > >> preconfig time.  
> > 
> > This sounds like there are things we can do with -S but can't
> > --preconfig now.  Is that correct?
> yes, we can't do at --preconfig time anything that requires built machine.

"built machine" is a very broad description.  We need to specify
more clearly what "built machine" means for an external user.
Does it mean having the QOM tree available?  Does it mean having
the VCPU threads created?  Without defining what -S really must
provide, we won't be able to deprecate and replace it.


> 
[...]
> > > If the plan is to deprecate -S, what are the important
> > > user-visible differences between -S and -preconfig today?  Do we
> > > plan to eliminate all those differences before
> > > deprecating/removing -S?
> we probably won't be able to deprecate -S in foreseeable future,
> for that we would need to be able to do everything starting from
> machine_run_board_init() to current pause point.
> But we can gradually move configuration commands to -preconfig time
> and gradually add CLI equivalents for that aren't possible at -S time
> (like Paolo suggested picking to be used machine model at runtime)

This could be a good plan, if we can explain why exactly -S is
still needed.

> 
[...]
> > >>                       But I've been sitting on these patches for
> > >> a long time and what's obvious to me might be not so clear to others.  
> > 
> > Par for the course, don't feel bad about it.
> > 
> > >> I might just not see what's missing. Any suggestions to improve it
> > >> are welcome.  
> > >
> > > I miss something that documents why both -S and -preconfig need
> > > to exist, what are the differences between them today, and what
> > > we plan to do about the differences between them in the future.
> Where would you prefer it being documented?

I suggest qemu-options.hx and/or qemu-doc.texi.

BTW, "cont" is documented as "Resume guest VCPU execution", which
is not true when using preconfig.  Maybe it's better to add a
separate QMP command for "create machine and devices" instead of
overloading the semantics of "cont"?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Igor Mammedov]

On Mon, 23 Apr 2018 10:05:54 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Apr 23, 2018 at 11:50:16AM +0200, Igor Mammedov wrote:
> > On Fri, 20 Apr 2018 08:31:18 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >   
> > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > >   
> > > > On Thu, Apr 19, 2018 at 10:00:04AM +0200, Igor Mammedov wrote:    
> > > >> On Wed, 18 Apr 2018 09:08:30 +0200
> > > >> Markus Armbruster <armbru@redhat.com> wrote:
> > > >>     
> > > >> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > >> >     
> > > >> > > On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:      
> > > >> > >> On Tue, 17 Apr 2018 11:27:39 -0300
> > > >> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >> > >>       
> > > >> > >> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:      
> > > >> > >> > > Igor Mammedov <imammedo@redhat.com> writes:
> > > >> > >> > > 
> > > >> > >> > > [...]        
> > > >> > >> > > > Series allows to configure NUMA mapping at runtime using QMP
> > > >> > >> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
> > > >> > >> > > > which allows to pause QEMU before machine_init() is run and
> > > >> > >> > > > adds new set-numa-node QMP command which in conjunction with
> > > >> > >> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> > > >> > >> > > >
> > > >> > >> > > > Later we can modify other commands to run early, for example device_add.
> > > >> > >> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > > >> > >> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > > >> > >> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.        
> > > >> > >> > > 
> > > >> > >> > > That instance is just stupidity / laziness, I think: we consider any
> > > >> > >> > > plug after machine creation a hot plug.  Real machines remain cold until
> > > >> > >> > > you press the power button.  Our virtual machines should remain cold
> > > >> > >> > > until they start running, i.e. with -S until the first "cont".      
> > > >> > >> It probably would be too risky to change semantics of -S from hotplug to coldplug.
> > > >> > >> But even if we were easy it won't matter in case if dynamic configuration
> > > >> > >> done properly. More on it below.
> > > >> > >>       
> > > >> > >> > > I vaguely remember me asking this before, but your answer didn't make it
> > > >> > >> > > into this cover letter, which gives me a pretext to ask again instead of
> > > >> > >> > > looking it up in the archives: what exactly prevents us from keeping the
> > > >> > >> > > machine cold enough for numa configuration until the first "cont"?        
> > > >> > >> > 
> > > >> > >> > I also think this would be better, but it seems to be difficult
> > > >> > >> > in practice, see:
> > > >> > >> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain      
> > > >> > >> 
> > > >> > >> In addition to Eduardo's reply, here is what I've answered back
> > > >> > >> when you've asked question the 1st time (v2 late at -S pause point reconfig):
> > > >> > >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
> > > >> > >> 
> > > >> > >> In short:
> > > >> > >> I think it's wrong in general doing fixups after machine is build
> > > >> > >> instead of getting correct configuration before building machine.
> > > >> > >> That's going to be complex and fragile and might be hard to do at
> > > >> > >> all depending on what we are fixing up.      
> > > >> > >
> > > >> > > What "building the machine" should mean, exactly, for external
> > > >> > > users?    
> > > >> under "building machine", I've meant machine_run_board_init()
> > > >> and all follow up steps to machine_done stage.
> > > >>     
> > > >> > > The main question I'd like to see answered is: why exactly we
> > > >> > > must "build" the machine before the first "cont" is issued when
> > > >> > > using -S?  Why can't we delay everything to "cont" when using -S?      
> > > >> Nor sure what question is about,
> > > >> Did you mean if it were possible to postpone machine_run_board_init()
> > > >> and all later steps to -S/cont time?    
> > (1)
> > As David said -S pause point is practically breakpoint on some
> > instruction of built/existing machine and current monitor commands
> > expect it to be valid. Moving -S before machine_run_board_init()
> > will break semantics of current -S pause point (i.e. user expectation
> > on existing machine) as well as most of the commands that evolved
> > in environment where machine already existed.  
> 
> OK, so what's missing here is a clear description what the user
> can expect on -S.
Currently it's fully configured machine with all CLI options taken
in account in paused state in initial state or with state it is getting
from migration stream if -incoming were used in combination with -S.

> > Hence a new -preconfig option and runstate to avoid breaking
> > exiting users and being able to cleanly handle configuration that
> > affects machine_run_board_init().
> >   
> > > > Exactly.  In other words, what exactly must be done before the
> > > > monitor is available when using -S,  
> > for MUST, it should be commands that affect machine_run_board_init()
> > like being added set-numa-node
> >   
> > > > and what exactly can be postponed after "cont" when using -S?  
> > hotplug configuration and various runtime query commands that
> > expect built machine. (today it's most of the commands)
> > 
> > wrt configuration commands we should split them into coldplug
> > and hotplug ones (some could be both).
> >      
> > > >> > > Is it just because it's a long and complex task?  Does that mean
> > > >> > > we might still do that eventually, and eliminate the
> > > >> > > prelaunch/preconfig distinction in the distant future?      
> > > >> > 
> > > >> > Why would anyone want to use -S going forward?  For reasons other "we've
> > > >> > always used -S, and can't be bothered to change".    
> > > >> We should be able to deprecate/remove -S once we can do all
> > > >> initial configuration that's possible to do there at
> > > >> preconfig time.    
> > > 
> > > This sounds like there are things we can do with -S but can't
> > > --preconfig now.  Is that correct?  
> > yes, we can't do at --preconfig time anything that requires built machine.  
> 
> "built machine" is a very broad description.  We need to specify
> more clearly what "built machine" means for an external user.
> Does it mean having the QOM tree available?  Does it mean having
> the VCPU threads created?  Without defining what -S really must
> provide, we won't be able to deprecate and replace it.
(*2) how about s/built machine/machine ready to execute guest code/,
that's what it is now.


> > > > If the plan is to deprecate -S, what are the important
> > > > user-visible differences between -S and -preconfig today?  Do we
> > > > plan to eliminate all those differences before
> > > > deprecating/removing -S?  
> > we probably won't be able to deprecate -S in foreseeable future,
> > for that we would need to be able to do everything starting from
> > machine_run_board_init() to current pause point.
> > But we can gradually move configuration commands to -preconfig time
> > and gradually add CLI equivalents for that aren't possible at -S time
> > (like Paolo suggested picking to be used machine model at runtime)  
> 
> This could be a good plan, if we can explain why exactly -S is
> still needed.
For a while -S would be need at least for compat reasons, if we ever
get to point where at -preconfig time machine could be build up to the
point -S provides[2] then we can talk about deprecating it, for now it's
way too premature to do something about it /I mean documenting intent
which is not there yet and might never materialize as there is no real
demand to deprecate it/.

> [...]
> > > >>                       But I've been sitting on these patches for
> > > >> a long time and what's obvious to me might be not so clear to others.    
> > > 
> > > Par for the course, don't feel bad about it.
> > >   
> > > >> I might just not see what's missing. Any suggestions to improve it
> > > >> are welcome.    
> > > >
> > > > I miss something that documents why both -S and -preconfig need
> > > > to exist, what are the differences between them today, and what
> > > > we plan to do about the differences between them in the future.  
> > Where would you prefer it being documented?  
> 
> I suggest qemu-options.hx and/or qemu-doc.texi.
Regarding qemu-options.hx patch
 "[PATCH for-2.13 v5 03/11] cli: add --preconfig option" 
adds doc text describing --preconfig option with explanation of how
'cont' could be used (including in combination with -S).

I'll try to come up with a text for qemu-doc.texi, not about
deprecating -S but about when --preconfig should be used vs -S
and where to get list of commands that could be used at preconfig state.

> BTW, "cont" is documented as "Resume guest VCPU execution", which
> is not true when using preconfig.  Maybe it's better to add a
> separate QMP command for "create machine and devices" instead of
> overloading the semantics of "cont"?
My bad, I've missed it, I can fixup 'cont' description to match
its behavior with --preconfig taken in account.

I'm not so sure about adding a new command is better though, I recall
Markus being against adding new commands unless we have to,
but I don't have strong inclination both ways so it's up to you.

I'm more inclined towards reusing 'cont', it seems logical 
(/me looking from the point if I were user).

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Eduardo Habkost]

On Mon, Apr 23, 2018 at 06:55:14PM +0200, Igor Mammedov wrote:
> On Mon, 23 Apr 2018 10:05:54 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Apr 23, 2018 at 11:50:16AM +0200, Igor Mammedov wrote:
> > > On Fri, 20 Apr 2018 08:31:18 +0200
> > > Markus Armbruster <armbru@redhat.com> wrote:
> > >   
> > > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > >   
> > > > > On Thu, Apr 19, 2018 at 10:00:04AM +0200, Igor Mammedov wrote:    
> > > > >> On Wed, 18 Apr 2018 09:08:30 +0200
> > > > >> Markus Armbruster <armbru@redhat.com> wrote:
> > > > >>     
> > > > >> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > > >> >     
> > > > >> > > On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:      
> > > > >> > >> On Tue, 17 Apr 2018 11:27:39 -0300
> > > > >> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >> > >>       
> > > > >> > >> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:      
> > > > >> > >> > > Igor Mammedov <imammedo@redhat.com> writes:
> > > > >> > >> > > 
> > > > >> > >> > > [...]        
> > > > >> > >> > > > Series allows to configure NUMA mapping at runtime using QMP
> > > > >> > >> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
> > > > >> > >> > > > which allows to pause QEMU before machine_init() is run and
> > > > >> > >> > > > adds new set-numa-node QMP command which in conjunction with
> > > > >> > >> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> > > > >> > >> > > >
> > > > >> > >> > > > Later we can modify other commands to run early, for example device_add.
> > > > >> > >> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > > > >> > >> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > > > >> > >> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.        
> > > > >> > >> > > 
> > > > >> > >> > > That instance is just stupidity / laziness, I think: we consider any
> > > > >> > >> > > plug after machine creation a hot plug.  Real machines remain cold until
> > > > >> > >> > > you press the power button.  Our virtual machines should remain cold
> > > > >> > >> > > until they start running, i.e. with -S until the first "cont".      
> > > > >> > >> It probably would be too risky to change semantics of -S from hotplug to coldplug.
> > > > >> > >> But even if we were easy it won't matter in case if dynamic configuration
> > > > >> > >> done properly. More on it below.
> > > > >> > >>       
> > > > >> > >> > > I vaguely remember me asking this before, but your answer didn't make it
> > > > >> > >> > > into this cover letter, which gives me a pretext to ask again instead of
> > > > >> > >> > > looking it up in the archives: what exactly prevents us from keeping the
> > > > >> > >> > > machine cold enough for numa configuration until the first "cont"?        
> > > > >> > >> > 
> > > > >> > >> > I also think this would be better, but it seems to be difficult
> > > > >> > >> > in practice, see:
> > > > >> > >> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain      
> > > > >> > >> 
> > > > >> > >> In addition to Eduardo's reply, here is what I've answered back
> > > > >> > >> when you've asked question the 1st time (v2 late at -S pause point reconfig):
> > > > >> > >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
> > > > >> > >> 
> > > > >> > >> In short:
> > > > >> > >> I think it's wrong in general doing fixups after machine is build
> > > > >> > >> instead of getting correct configuration before building machine.
> > > > >> > >> That's going to be complex and fragile and might be hard to do at
> > > > >> > >> all depending on what we are fixing up.      
> > > > >> > >
> > > > >> > > What "building the machine" should mean, exactly, for external
> > > > >> > > users?    
> > > > >> under "building machine", I've meant machine_run_board_init()
> > > > >> and all follow up steps to machine_done stage.
> > > > >>     
> > > > >> > > The main question I'd like to see answered is: why exactly we
> > > > >> > > must "build" the machine before the first "cont" is issued when
> > > > >> > > using -S?  Why can't we delay everything to "cont" when using -S?      
> > > > >> Nor sure what question is about,
> > > > >> Did you mean if it were possible to postpone machine_run_board_init()
> > > > >> and all later steps to -S/cont time?    
> > > (1)
> > > As David said -S pause point is practically breakpoint on some
> > > instruction of built/existing machine and current monitor commands
> > > expect it to be valid. Moving -S before machine_run_board_init()
> > > will break semantics of current -S pause point (i.e. user expectation
> > > on existing machine) as well as most of the commands that evolved
> > > in environment where machine already existed.  
> > 
> > OK, so what's missing here is a clear description what the user
> > can expect on -S.
> Currently it's fully configured machine with all CLI options taken
> in account in paused state in initial state or with state it is getting
> from migration stream if -incoming were used in combination with -S.
> 
> > > Hence a new -preconfig option and runstate to avoid breaking
> > > exiting users and being able to cleanly handle configuration that
> > > affects machine_run_board_init().
> > >   
> > > > > Exactly.  In other words, what exactly must be done before the
> > > > > monitor is available when using -S,  
> > > for MUST, it should be commands that affect machine_run_board_init()
> > > like being added set-numa-node
> > >   
> > > > > and what exactly can be postponed after "cont" when using -S?  
> > > hotplug configuration and various runtime query commands that
> > > expect built machine. (today it's most of the commands)
> > > 
> > > wrt configuration commands we should split them into coldplug
> > > and hotplug ones (some could be both).
> > >      
> > > > >> > > Is it just because it's a long and complex task?  Does that mean
> > > > >> > > we might still do that eventually, and eliminate the
> > > > >> > > prelaunch/preconfig distinction in the distant future?      
> > > > >> > 
> > > > >> > Why would anyone want to use -S going forward?  For reasons other "we've
> > > > >> > always used -S, and can't be bothered to change".    
> > > > >> We should be able to deprecate/remove -S once we can do all
> > > > >> initial configuration that's possible to do there at
> > > > >> preconfig time.    
> > > > 
> > > > This sounds like there are things we can do with -S but can't
> > > > --preconfig now.  Is that correct?  
> > > yes, we can't do at --preconfig time anything that requires built machine.  
> > 
> > "built machine" is a very broad description.  We need to specify
> > more clearly what "built machine" means for an external user.
> > Does it mean having the QOM tree available?  Does it mean having
> > the VCPU threads created?  Without defining what -S really must
> > provide, we won't be able to deprecate and replace it.
> (*2) how about s/built machine/machine ready to execute guest code/,
> that's what it is now.

This is a bit better, we still need to be clear about what
"ready" means.  e.g.: can users expect the VCPU threads be
already running?

Anyway, the details don't need to be sorted out immediately.  IMO
the most important part is to describe the difference between
-preconfig and -S.

> 
> 
> > > > > If the plan is to deprecate -S, what are the important
> > > > > user-visible differences between -S and -preconfig today?  Do we
> > > > > plan to eliminate all those differences before
> > > > > deprecating/removing -S?  
> > > we probably won't be able to deprecate -S in foreseeable future,
> > > for that we would need to be able to do everything starting from
> > > machine_run_board_init() to current pause point.
> > > But we can gradually move configuration commands to -preconfig time
> > > and gradually add CLI equivalents for that aren't possible at -S time
> > > (like Paolo suggested picking to be used machine model at runtime)  
> > 
> > This could be a good plan, if we can explain why exactly -S is
> > still needed.
> For a while -S would be need at least for compat reasons, if we ever
> get to point where at -preconfig time machine could be build up to the
> point -S provides[2] then we can talk about deprecating it, for now it's
> way too premature to do something about it /I mean documenting intent
> which is not there yet and might never materialize as there is no real
> demand to deprecate it/.

Yeah, compatibility is the main reason we can't simply deprecate
or remove -S immediately.  We just need to find out what exactly
is important on -S.


> 
> > [...]
> > > > >>                       But I've been sitting on these patches for
> > > > >> a long time and what's obvious to me might be not so clear to others.    
> > > > 
> > > > Par for the course, don't feel bad about it.
> > > >   
> > > > >> I might just not see what's missing. Any suggestions to improve it
> > > > >> are welcome.    
> > > > >
> > > > > I miss something that documents why both -S and -preconfig need
> > > > > to exist, what are the differences between them today, and what
> > > > > we plan to do about the differences between them in the future.  
> > > Where would you prefer it being documented?  
> > 
> > I suggest qemu-options.hx and/or qemu-doc.texi.
> Regarding qemu-options.hx patch
>  "[PATCH for-2.13 v5 03/11] cli: add --preconfig option" 
> adds doc text describing --preconfig option with explanation of how
> 'cont' could be used (including in combination with -S).
> 
> I'll try to come up with a text for qemu-doc.texi, not about
> deprecating -S but about when --preconfig should be used vs -S
> and where to get list of commands that could be used at preconfig state.

Sounds good to me.  Thanks!


> 
> > BTW, "cont" is documented as "Resume guest VCPU execution", which
> > is not true when using preconfig.  Maybe it's better to add a
> > separate QMP command for "create machine and devices" instead of
> > overloading the semantics of "cont"?
> My bad, I've missed it, I can fixup 'cont' description to match
> its behavior with --preconfig taken in account.
> 
> I'm not so sure about adding a new command is better though, I recall
> Markus being against adding new commands unless we have to,
> but I don't have strong inclination both ways so it's up to you.
> 
> I'm more inclined towards reusing 'cont', it seems logical 
> (/me looking from the point if I were user).

'cont' seemed logical to me at first, until I read its
documentation.  Then I think it makes things very confusing,
especially if we combine -preconfig with -S and/or -incoming.

A separate command would have less room for ambiguity.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Igor Mammedov]

On Mon, 23 Apr 2018 17:45:31 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Apr 23, 2018 at 06:55:14PM +0200, Igor Mammedov wrote:
> > On Mon, 23 Apr 2018 10:05:54 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Mon, Apr 23, 2018 at 11:50:16AM +0200, Igor Mammedov wrote:  
> > > > On Fri, 20 Apr 2018 08:31:18 +0200
> > > > Markus Armbruster <armbru@redhat.com> wrote:
> > > >     
> > > > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > > >     
> > > > > > On Thu, Apr 19, 2018 at 10:00:04AM +0200, Igor Mammedov wrote:      
> > > > > >> On Wed, 18 Apr 2018 09:08:30 +0200
> > > > > >> Markus Armbruster <armbru@redhat.com> wrote:
> > > > > >>       
> > > > > >> > Eduardo Habkost <ehabkost@redhat.com> writes:
> > > > > >> >       
> > > > > >> > > On Tue, Apr 17, 2018 at 05:41:10PM +0200, Igor Mammedov wrote:        
> > > > > >> > >> On Tue, 17 Apr 2018 11:27:39 -0300
> > > > > >> > >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > >> > >>         
> > > > > >> > >> > On Tue, Apr 17, 2018 at 04:13:34PM +0200, Markus Armbruster wrote:        
> > > > > >> > >> > > Igor Mammedov <imammedo@redhat.com> writes:
> > > > > >> > >> > > 
> > > > > >> > >> > > [...]          
> > > > > >> > >> > > > Series allows to configure NUMA mapping at runtime using QMP
> > > > > >> > >> > > > interface. For that to happen it introduces a new '-preconfig' CLI option
> > > > > >> > >> > > > which allows to pause QEMU before machine_init() is run and
> > > > > >> > >> > > > adds new set-numa-node QMP command which in conjunction with
> > > > > >> > >> > > > query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
> > > > > >> > >> > > >
> > > > > >> > >> > > > Later we can modify other commands to run early, for example device_add.
> > > > > >> > >> > > > I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> > > > > >> > >> > > > paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> > > > > >> > >> > > > stage it's considered hotplug already), so SPAPR had to work around the issue.          
> > > > > >> > >> > > 
> > > > > >> > >> > > That instance is just stupidity / laziness, I think: we consider any
> > > > > >> > >> > > plug after machine creation a hot plug.  Real machines remain cold until
> > > > > >> > >> > > you press the power button.  Our virtual machines should remain cold
> > > > > >> > >> > > until they start running, i.e. with -S until the first "cont".        
> > > > > >> > >> It probably would be too risky to change semantics of -S from hotplug to coldplug.
> > > > > >> > >> But even if we were easy it won't matter in case if dynamic configuration
> > > > > >> > >> done properly. More on it below.
> > > > > >> > >>         
> > > > > >> > >> > > I vaguely remember me asking this before, but your answer didn't make it
> > > > > >> > >> > > into this cover letter, which gives me a pretext to ask again instead of
> > > > > >> > >> > > looking it up in the archives: what exactly prevents us from keeping the
> > > > > >> > >> > > machine cold enough for numa configuration until the first "cont"?          
> > > > > >> > >> > 
> > > > > >> > >> > I also think this would be better, but it seems to be difficult
> > > > > >> > >> > in practice, see:
> > > > > >> > >> > http://mid.mail-archive.com/20180323210532.GD28161@localhost.localdomain        
> > > > > >> > >> 
> > > > > >> > >> In addition to Eduardo's reply, here is what I've answered back
> > > > > >> > >> when you've asked question the 1st time (v2 late at -S pause point reconfig):
> > > > > >> > >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg504140.html
> > > > > >> > >> 
> > > > > >> > >> In short:
> > > > > >> > >> I think it's wrong in general doing fixups after machine is build
> > > > > >> > >> instead of getting correct configuration before building machine.
> > > > > >> > >> That's going to be complex and fragile and might be hard to do at
> > > > > >> > >> all depending on what we are fixing up.        
> > > > > >> > >
> > > > > >> > > What "building the machine" should mean, exactly, for external
> > > > > >> > > users?      
> > > > > >> under "building machine", I've meant machine_run_board_init()
> > > > > >> and all follow up steps to machine_done stage.
> > > > > >>       
> > > > > >> > > The main question I'd like to see answered is: why exactly we
> > > > > >> > > must "build" the machine before the first "cont" is issued when
> > > > > >> > > using -S?  Why can't we delay everything to "cont" when using -S?        
> > > > > >> Nor sure what question is about,
> > > > > >> Did you mean if it were possible to postpone machine_run_board_init()
> > > > > >> and all later steps to -S/cont time?      
> > > > (1)
> > > > As David said -S pause point is practically breakpoint on some
> > > > instruction of built/existing machine and current monitor commands
> > > > expect it to be valid. Moving -S before machine_run_board_init()
> > > > will break semantics of current -S pause point (i.e. user expectation
> > > > on existing machine) as well as most of the commands that evolved
> > > > in environment where machine already existed.    
> > > 
> > > OK, so what's missing here is a clear description what the user
> > > can expect on -S.  
> > Currently it's fully configured machine with all CLI options taken
> > in account in paused state in initial state or with state it is getting
> > from migration stream if -incoming were used in combination with -S.
> >   
> > > > Hence a new -preconfig option and runstate to avoid breaking
> > > > exiting users and being able to cleanly handle configuration that
> > > > affects machine_run_board_init().
> > > >     
> > > > > > Exactly.  In other words, what exactly must be done before the
> > > > > > monitor is available when using -S,    
> > > > for MUST, it should be commands that affect machine_run_board_init()
> > > > like being added set-numa-node
> > > >     
> > > > > > and what exactly can be postponed after "cont" when using -S?    
> > > > hotplug configuration and various runtime query commands that
> > > > expect built machine. (today it's most of the commands)
> > > > 
> > > > wrt configuration commands we should split them into coldplug
> > > > and hotplug ones (some could be both).
> > > >        
> > > > > >> > > Is it just because it's a long and complex task?  Does that mean
> > > > > >> > > we might still do that eventually, and eliminate the
> > > > > >> > > prelaunch/preconfig distinction in the distant future?        
> > > > > >> > 
> > > > > >> > Why would anyone want to use -S going forward?  For reasons other "we've
> > > > > >> > always used -S, and can't be bothered to change".      
> > > > > >> We should be able to deprecate/remove -S once we can do all
> > > > > >> initial configuration that's possible to do there at
> > > > > >> preconfig time.      
> > > > > 
> > > > > This sounds like there are things we can do with -S but can't
> > > > > --preconfig now.  Is that correct?    
> > > > yes, we can't do at --preconfig time anything that requires built machine.    
> > > 
> > > "built machine" is a very broad description.  We need to specify
> > > more clearly what "built machine" means for an external user.
> > > Does it mean having the QOM tree available?  Does it mean having
> > > the VCPU threads created?  Without defining what -S really must
> > > provide, we won't be able to deprecate and replace it.  
> > (*2) how about s/built machine/machine ready to execute guest code/,
> > that's what it is now.  
> 
> This is a bit better, we still need to be clear about what
> "ready" means.  e.g.: can users expect the VCPU threads be
> already running?
> 
> Anyway, the details don't need to be sorted out immediately.  IMO
> the most important part is to describe the difference between
> -preconfig and -S.
> 
> > 
> >   
> > > > > > If the plan is to deprecate -S, what are the important
> > > > > > user-visible differences between -S and -preconfig today?  Do we
> > > > > > plan to eliminate all those differences before
> > > > > > deprecating/removing -S?    
> > > > we probably won't be able to deprecate -S in foreseeable future,
> > > > for that we would need to be able to do everything starting from
> > > > machine_run_board_init() to current pause point.
> > > > But we can gradually move configuration commands to -preconfig time
> > > > and gradually add CLI equivalents for that aren't possible at -S time
> > > > (like Paolo suggested picking to be used machine model at runtime)    
> > > 
> > > This could be a good plan, if we can explain why exactly -S is
> > > still needed.  
> > For a while -S would be need at least for compat reasons, if we ever
> > get to point where at -preconfig time machine could be build up to the
> > point -S provides[2] then we can talk about deprecating it, for now it's
> > way too premature to do something about it /I mean documenting intent
> > which is not there yet and might never materialize as there is no real
> > demand to deprecate it/.  
> 
> Yeah, compatibility is the main reason we can't simply deprecate
> or remove -S immediately.  We just need to find out what exactly
> is important on -S.
> 
> 
> >   
> > > [...]  
> > > > > >>                       But I've been sitting on these patches for
> > > > > >> a long time and what's obvious to me might be not so clear to others.      
> > > > > 
> > > > > Par for the course, don't feel bad about it.
> > > > >     
> > > > > >> I might just not see what's missing. Any suggestions to improve it
> > > > > >> are welcome.      
> > > > > >
> > > > > > I miss something that documents why both -S and -preconfig need
> > > > > > to exist, what are the differences between them today, and what
> > > > > > we plan to do about the differences between them in the future.    
> > > > Where would you prefer it being documented?    
> > > 
> > > I suggest qemu-options.hx and/or qemu-doc.texi.  
> > Regarding qemu-options.hx patch
> >  "[PATCH for-2.13 v5 03/11] cli: add --preconfig option" 
> > adds doc text describing --preconfig option with explanation of how
> > 'cont' could be used (including in combination with -S).
> > 
> > I'll try to come up with a text for qemu-doc.texi, not about
> > deprecating -S but about when --preconfig should be used vs -S
> > and where to get list of commands that could be used at preconfig state.  
> 
> Sounds good to me.  Thanks!
how about something like this:

diff --git a/qemu-tech.texi b/qemu-tech.texi
index 52a56ae..6951258 100644
--- a/qemu-tech.texi
+++ b/qemu-tech.texi
@@ -5,6 +5,7 @@
 * CPU emulation::
 * Translator Internals::
 * QEMU compared to other emulators::
+* Managed start up options::
 * Bibliography::
 @end menu
 
@@ -314,6 +315,44 @@ VirtualBox [9], Xen [10] and KVM [11] are based on QEMU. QEMU-SystemC
 [12] uses QEMU to simulate a system where some hardware devices are
 developed in SystemC.
 
+@node Managed start up options
+@section Managed start up options
+
+In system mode emulation, it's possible to create VM in paused state using
+-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
+was started. It could be in:
+@table @asis
+@item initial state (after reset/power on state)
+@item with direct kernel loading initial state could be ammended to execute
+code loaded by QEMU in VM's RAM and with incomming migration
+@item with incomming migrartion, initial state will by ammended by the migrated
+machine state after migration completes.
+@end table
+
+This paused state is typically used by users to query machine state and/or
+additionally configure machine (hotplug devices) in runtime before allowing
+VM code to run.
+
+However at -S pause point it's impossible to configure options that affect
+initial VM creation (like: -smp/-m/-numa ...) or cold plug devices. That's
+when -preconfig command line option should be used. It allows to pause
+QEMU before initial VM creation in preconfig state, query being created
+VM at runtime and configure start up options depending on previous query
+results. In preconfig state QEMU allows to configure VM only via QMP monitor
+with a limited command set which doesn't depend on completely initialized
+machine, which includes but not limited to:
+@table @asis
+@item qmp_capabilities
+@item query-qmp-schema
+@item query-commands
+@item query-status
+@end 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.
+
 @node Bibliography
 @section Bibliography

> >   
> > > BTW, "cont" is documented as "Resume guest VCPU execution", which
> > > is not true when using preconfig.  Maybe it's better to add a
> > > separate QMP command for "create machine and devices" instead of
> > > overloading the semantics of "cont"?  
> > My bad, I've missed it, I can fixup 'cont' description to match
> > its behavior with --preconfig taken in account.
> > 
> > I'm not so sure about adding a new command is better though, I recall
> > Markus being against adding new commands unless we have to,
> > but I don't have strong inclination both ways so it's up to you.
> > 
> > I'm more inclined towards reusing 'cont', it seems logical 
> > (/me looking from the point if I were user).  
> 
> 'cont' seemed logical to me at first, until I read its
> documentation.  Then I think it makes things very confusing,
> especially if we combine -preconfig with -S and/or -incoming.
> 
> A separate command would have less room for ambiguity.
I've added following instead of reusing 'cont':

##                                                                               
# @exit-preconfig:                                                               
#                                                                                
# Exit from "preconfig" state                                                    
#                                                                                
# 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).                     
#                                                                                
# Example:                                                                       
#                                                                                
# -> { "execute": "exit-preconfig" }                                             
# <- { "return": {} }                                                            
#                                                                                
##                                                                               
{ 'command': 'exit-preconfig', 'allowed-in-preconfig': true } 

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 5492 bytes --]

On 04/26/2018 09:39 AM, Igor Mammedov wrote:

>>> I'll try to come up with a text for qemu-doc.texi, not about
>>> deprecating -S but about when --preconfig should be used vs -S
>>> and where to get list of commands that could be used at preconfig state.  
>>
>> Sounds good to me.  Thanks!
> how about something like this:
> 
> diff --git a/qemu-tech.texi b/qemu-tech.texi
> index 52a56ae..6951258 100644
> --- a/qemu-tech.texi
> +++ b/qemu-tech.texi
> @@ -5,6 +5,7 @@
>  * CPU emulation::
>  * Translator Internals::
>  * QEMU compared to other emulators::
> +* Managed start up options::
>  * Bibliography::
>  @end menu
>  
> @@ -314,6 +315,44 @@ VirtualBox [9], Xen [10] and KVM [11] are based on QEMU. QEMU-SystemC
>  [12] uses QEMU to simulate a system where some hardware devices are
>  developed in SystemC.
>  
> +@node Managed start up options
> +@section Managed start up options
> +
> +In system mode emulation, it's possible to create VM in paused state using
> +-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
> +was started. It could be in:
> +@table @asis
> +@item initial state (after reset/power on state)
> +@item with direct kernel loading initial state could be ammended to execute

s/ammended/amended/

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

s/incomming/incoming/

> +@item with incomming migrartion, initial state will by ammended by the migrated

and again, for both words
s/migrartion/migration/

> +machine state after migration completes.
> +@end table
> +
> +This paused state is typically used by users to query machine state and/or
> +additionally configure machine (hotplug devices) in runtime before allowing
> +VM code to run.
> +
> +However at -S pause point it's impossible to configure options that affect
> +initial VM creation (like: -smp/-m/-numa ...) or cold plug devices. That's
> +when -preconfig command line option should be used. It allows to pause

s/to pause/pausing/

> +QEMU before initial VM creation in preconfig state, query being created
> +VM at runtime and configure start up options depending on previous query

Reads awkwardly.  Maybe:

It allows pausing QEMU 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.

> +results. In preconfig state QEMU allows to configure VM only via QMP monitor
> +with a limited command set which doesn't depend on completely initialized
> +machine, which includes but not limited to:

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:

> +@table @asis
> +@item qmp_capabilities
> +@item query-qmp-schema
> +@item query-commands
> +@item query-status
> +@end 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.
> +
>  @node Bibliography
>  @section Bibliography
> 

>> A separate command would have less room for ambiguity.
> I've added following instead of reusing 'cont':
> 
> ##                                                                               
> # @exit-preconfig:                                                               

This should definitely be mentioned in the docs section of commands
permitted during preconfig.

> #                                                                                
> # Exit from "preconfig" state                                                    
> #                                                                                
> # 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).                     
> #                                                                                
> # Example:                                                                       
> #                                                                                
> # -> { "execute": "exit-preconfig" }                                             
> # <- { "return": {} }                                                            
> #                                                                                
> ##                                                                               
> { 'command': 'exit-preconfig', 'allowed-in-preconfig': true } 
> 
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v4 0/9] enable numa configuration before machine_init() from QMP

[Igor Mammedov]

On Thu, 26 Apr 2018 09:55:40 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 04/26/2018 09:39 AM, Igor Mammedov wrote:
> 
> >>> I'll try to come up with a text for qemu-doc.texi, not about
> >>> deprecating -S but about when --preconfig should be used vs -S
> >>> and where to get list of commands that could be used at preconfig state.    
> >>
> >> Sounds good to me.  Thanks!  
> > how about something like this:
> > 
> > diff --git a/qemu-tech.texi b/qemu-tech.texi
> > index 52a56ae..6951258 100644
> > --- a/qemu-tech.texi
> > +++ b/qemu-tech.texi
> > @@ -5,6 +5,7 @@
> >  * CPU emulation::
> >  * Translator Internals::
> >  * QEMU compared to other emulators::
> > +* Managed start up options::
> >  * Bibliography::
> >  @end menu
> >  
> > @@ -314,6 +315,44 @@ VirtualBox [9], Xen [10] and KVM [11] are based on QEMU. QEMU-SystemC
> >  [12] uses QEMU to simulate a system where some hardware devices are
> >  developed in SystemC.
> >  
> > +@node Managed start up options
> > +@section Managed start up options
> > +
> > +In system mode emulation, it's possible to create VM in paused state using
> > +-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
> > +was started. It could be in:
> > +@table @asis
> > +@item initial state (after reset/power on state)
> > +@item with direct kernel loading initial state could be ammended to execute  
> 
> s/ammended/amended/
> 
> > +code loaded by QEMU in VM's RAM and with incomming migration  
> 
> s/incomming/incoming/
> 
> > +@item with incomming migrartion, initial state will by ammended by the migrated  
> 
> and again, for both words
> s/migrartion/migration/
> 
> > +machine state after migration completes.
> > +@end table
> > +
> > +This paused state is typically used by users to query machine state and/or
> > +additionally configure machine (hotplug devices) in runtime before allowing
> > +VM code to run.
> > +
> > +However at -S pause point it's impossible to configure options that affect
> > +initial VM creation (like: -smp/-m/-numa ...) or cold plug devices. That's
> > +when -preconfig command line option should be used. It allows to pause  
> 
> s/to pause/pausing/
> 
> > +QEMU before initial VM creation in preconfig state, query being created
> > +VM at runtime and configure start up options depending on previous query  
> 
> Reads awkwardly.  Maybe:
> 
> It allows pausing QEMU 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.
> 
> > +results. In preconfig state QEMU allows to configure VM only via QMP monitor
> > +with a limited command set which doesn't depend on completely initialized
> > +machine, which includes but not limited to:  
> 
> 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:

thanks for review,
I amended patches as you suggested.
I plan to respin v6 today with these changes since exit-preconfig
affected several patches in series.

> 
> > +@table @asis
> > +@item qmp_capabilities
> > +@item query-qmp-schema
> > +@item query-commands
> > +@item query-status
> > +@end 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.
> > +
> >  @node Bibliography
> >  @section Bibliography
> >   
> 
> >> A separate command would have less room for ambiguity.  
> > I've added following instead of reusing 'cont':
> > 
> > ##                                                                               
> > # @exit-preconfig:                                                                 
> 
> This should definitely be mentioned in the docs section of commands
> permitted during preconfig.
done

> 
> > #                                                                                
> > # Exit from "preconfig" state                                                    
> > #                                                                                
> > # 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).                     
> > #                                                                                
> > # Example:                                                                       
> > #                                                                                
> > # -> { "execute": "exit-preconfig" }                                             
> > # <- { "return": {} }                                                            
> > #                                                                                
> > ##                                                                               
> > { 'command': 'exit-preconfig', 'allowed-in-preconfig': true } 
> > 
> >   
> 

Re: [Qemu-devel] [PATCH v4 3/9] cli: add -preconfig option

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Tue, Apr 03, 2018 at 03:49:07PM +0200, Igor Mammedov wrote:
> > On Thu, 29 Mar 2018 13:57:54 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > [...]
> > > > As for the future, I agree it would be much more flexible
> > > > to allow both -preconfig and -incoming at the same time,
> > > > so we could start target with empty CLI, and then feed it
> > > > options from source. It would require audit/refactoring of
> > > > INMIGRATE state and making 'all' current CLI options
> > > > available via QMP interface.
> > > > 
> > > > But for now I'd prefer to keep using old way to start target.  
> > > 
> > > Well, if management software developers tell us that -preconfig
> > > will be already useful without -incoming support, I won't object.
> > As Peter said, mgmt can/will use -preconfig even without -incoming,
> > since it lets deal with initial (source) start-up problem (i.e.
> > avoid restarting QEMU) and lets us make numa configuration work
> > in qemu/libvirt stack without guess work. (that's the end goal of
> > the series)
> > 
> > > But it would be very nice for management software if they can
> > > simply assume that -preconfig and -incoming will work together
> > > since the first version.  Can we have this as a goal for 2.13?
> > I can't promise to refactor -incoming in near future, as I'm working
> > on ARM cpu-hotplug currently and next in queue is ARM memory hotplug.
> > 
> > Peter suggested an alternative idea, to abandon -incoming and
> > enable incoming migration from QMP after all configuration is done.
> > Amount of refactoring need probably would be the same but at least
> > interface and runstate transitions wise it looks cleaner.
> 
> Also, support for the "start incoming migration" QMP command
> would be very easy to probe using existing mechanisms.  Sounds
> good to me.

You could stick to -incoming defer,   that already says that you want
to do an inward migration but doesn't say where from until later.

Note there are some other bits of code scattered around for
checking whether we're in -incoming mode, for example to stop you
do an outwards migration while waiting for an incoming one, or
to stop you doing a migrate-incoming while you're already waiting
for one.

Dave

> -- 
> Eduardo
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK