All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP
@ 2018-05-04  8:37 Igor Mammedov
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 01/11] numa: postpone options post-processing till machine_run_board_init() Igor Mammedov
                   ` (12 more replies)
  0 siblings, 13 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru



v6->v7:
  * fix up wording in documentation and left-overs of 'reusing' 'cont' command in v5
  * split out preconfig runstate introduction into separate patch to make
    series more bisection friendly and put allow-preconfig patch before
    --preconfig CLI patch
  * merge qapi schema tests patches into one
  * rename allowed_in_preconfig flag to a shorter allow_preconfig
  * rename QCO_ALLOWED_IN_PRECONFIG into shorter QCO_ALLOW_PRECONFIG to match
    allow_preconfig flag
  * reuse an-oob-command for allow_preconfig flag testing and rename it to
    test-flags-command
v5->v6:
  * add exit-preconfig QMP command instead of overloading meaning of 'cont' command
  * add doc text to qemu-tech.texi about -S and --preconfig
  * add numa configuration example into commit message of 10/11
  * limit set-numa-node QMP command to preconfig mode
v4->v5:
  * rebase on top of OOB changes that's now in master
  * s/qobject_to_qdict(/qobject_to(QDict,/
  * s/-preconfig/--preconfig/
  * s/2.12/2.13/
  * s/parse_NumaOptions/set_numa_options/
  * drop if (err) guard around error_propagate()
  * move QCO_ALLOWED_IN_PRECONFIG and do_qmp_dispatch() runstate check
    from the later patch 'qapi: introduce new cmd option  "allowed-in-preconfig"'
    to here for better bissectability
  * add TODO comment to '{ RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE }' transition
  * check for incoming && preconfig outside of option parsing loop
  * add 'use QMP instead" to error message, suggesting user the right interface to use
  * allow query-command-line-options in preconfig state
  * make sure that allowed-in-preconfig could be set only to True
  * move out QCO_ALLOWED_IN_PRECONFIG check in do_qmp_dispatch() to
    earlier 'cli: add --preconfig option' patch
  * 2 extra test patches
     'tests: let qapi-schema tests detect allowed-in-preconfig'
     'tests: add allowed-in-preconfig-test for qapi-schema'
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.


Currently it's problematic to configure NUMA mapping for CPUs using "-numa cpu="
option, without restarting QEMU, first is to query board specific CPU layout
with query-hotpluggable-cpus QMP command (which is function of -M and -smp CLI
options). Mgmt side isn't happy with restarting QEMU to query configuration
parameters first and then run actual instance, so it keeps using old cpu_index
based option '-numa cpus' instead of new socket/core/thread-id based one.

Introduce a new '--preconfig' CLI option which allows pausing QEMU before
machine_init() is run (preconfig state). So it would be possible to query
possible CPUs layout, configure NUMA mapping based on query result with
set-numa-node QMP command and continue executing  without restarting QEMU.

Difference between new --preconfig pause point vs -S is that the later pauses
QEMU after machine is constructed and ready to run guest code or in process of
incoming migration (essentially machine is in some running state (with paused
VCPUs) and any action on it is considered as hotplug). At this point it's hard
to configure or reconfigure parameters that affect machine_init() and later
stages. While the new --preconfig option pauses QEMU instance before
machine_init() and would allow to configure parameters as if doing it from CLI
but in interactve manner using QMP interface, which would allow introspecting
and configuring QEMU instance without restarting it. 

Initially only limited set of commands (that are ready to work with non
initialized machine or without it) are allowed in preconfig state:

   #existing commands:
       qmp_capabilities
       query-qmp-schema
       query-commands
       query-command-line-options
       query-status
       query-hotpluggable-cpus
   #new commands
       exit-preconfig
       set-numa-node

Which commands are allowed at preconfig state, is defined by QAPI Schema
with help of optional 'allowed-in-preconfig' flag in command definition,
which allows users to get list of commands it can run with help of
query-qmp-schema command.

Once user done with configuration at preconfig state, it could be exited with
exit-preconfig command, in which case QEMU would proceed with VM intialization
and either start guest execution or pause in prelaunch state if -S CLI option
were specified along with --prelaunch on startup.

PS:
Later we can modify other commands to run early, for example device_add. So
that devices would be added in cold-plug context, instead of getting device
'hotplugged' with followed up fixups and workarounds to make it look like
cold-plugged.

Example of configuration CPU's NUMA mapping with --preconfig:
$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_v7

 
CC: eblake@redhat.com
CC: ehabkost@redhat.com
CC: pkrempa@redhat.com
CC: armbru@redhat.com


Igor Mammedov (11):
  numa: postpone options post-processing till machine_run_board_init()
  numa: split out NumaOptions parsing into set_numa_options()
  qapi: introduce preconfig runstate
  hmp: disable monitor in preconfig state
  qapi: introduce new cmd option "allowed-in-preconfig"
  tests: qapi-schema tests for allow-preconfig
  cli: add --preconfig option
  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                 |  1 +
 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                                   | 11 +++--
 numa.c                                      | 70 +++++++++++++++++++----------
 qapi/introspect.json                        |  5 ++-
 qapi/misc.json                              | 49 ++++++++++++++++++--
 qapi/qmp-dispatch.c                         |  8 ++++
 qapi/run-state.json                         |  8 +++-
 qemu-options.hx                             | 13 ++++++
 qemu-tech.texi                              | 40 +++++++++++++++++
 qmp.c                                       | 10 +++++
 scripts/qapi/commands.py                    | 11 +++--
 scripts/qapi/common.py                      | 18 +++++---
 scripts/qapi/doc.py                         |  4 +-
 scripts/qapi/introspect.py                  |  7 +--
 tests/Makefile.include                      |  1 +
 tests/libqtest.c                            |  7 +++
 tests/numa-test.c                           | 61 +++++++++++++++++++++++++
 tests/qapi-schema/allow-preconfig-test.err  |  1 +
 tests/qapi-schema/allow-preconfig-test.exit |  1 +
 tests/qapi-schema/allow-preconfig-test.json |  2 +
 tests/qapi-schema/allow-preconfig-test.out  |  0
 tests/qapi-schema/doc-good.out              |  4 +-
 tests/qapi-schema/ident-with-escape.out     |  2 +-
 tests/qapi-schema/indented-expr.out         |  4 +-
 tests/qapi-schema/qapi-schema-test.json     |  4 +-
 tests/qapi-schema/qapi-schema-test.out      | 22 ++++-----
 tests/qapi-schema/test-qapi.py              |  8 ++--
 tests/qmp-test.c                            | 37 +++++++++++++++
 tests/test-qmp-cmds.c                       |  2 +-
 vl.c                                        | 35 ++++++++++++++-
 35 files changed, 396 insertions(+), 77 deletions(-)
 create mode 100644 tests/qapi-schema/allow-preconfig-test.err
 create mode 100644 tests/qapi-schema/allow-preconfig-test.exit
 create mode 100644 tests/qapi-schema/allow-preconfig-test.json
 create mode 100644 tests/qapi-schema/allow-preconfig-test.out

-- 
2.7.4

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 01/11] numa: postpone options post-processing till machine_run_board_init()
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-16 18:12   ` Eduardo Habkost
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 02/11] numa: split out NumaOptions parsing into set_numa_options() Igor Mammedov
                   ` (11 subsequent siblings)
  12 siblings, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

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>
Reviewed-by: Eduardo Habkost <ehabkost@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 2040177..617e5f8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -737,7 +737,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;
@@ -792,7 +792,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 78a869e..d0abd7c 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

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 02/11] numa: split out NumaOptions parsing into set_numa_options()
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 01/11] numa: postpone options post-processing till machine_run_board_init() Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-16 18:12   ` Eduardo Habkost
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 03/11] qapi: introduce preconfig runstate Igor Mammedov
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v5:
  - (Eduardo Habkost <ehabkost@redhat.com>)
    * drop if (err) guard around error_propagate()
    * s/parse_NumaOptions/set_numa_options/
---
 include/sysemu/numa.h |  1 +
 numa.c                | 46 +++++++++++++++++++++++++++-------------------
 2 files changed, 28 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 d0abd7c..63e3989 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 set_numa_options(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,31 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
     }
 
 end:
+    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);
+    }
+
+    set_numa_options(ms, object, &err);
+
+end:
     qapi_free_NumaOptions(object);
     if (err) {
         error_report_err(err);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 03/11] qapi: introduce preconfig runstate
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 01/11] numa: postpone options post-processing till machine_run_board_init() Igor Mammedov
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 02/11] numa: split out NumaOptions parsing into set_numa_options() Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-11 15:17   ` Eric Blake
  2018-05-16 18:14   ` Eduardo Habkost
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state Igor Mammedov
                   ` (9 subsequent siblings)
  12 siblings, 2 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

New preconfig runstate will be used in follow up patches
related to introducing --preconfig CLI option and is
intended to replace prelaunch runstate from QEMU start
up to machine_init callback.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qapi/run-state.json | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 1c9fff3..9694a9f 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 the --preconfig CLI option is used.
+#             (Since 2.13)
 ##
 { '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:
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (2 preceding siblings ...)
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 03/11] qapi: introduce preconfig runstate Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-24 16:00   ` Markus Armbruster
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 05/11] qapi: introduce new cmd option "allowed-in-preconfig" Igor Mammedov
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
v5:
  * add 'use QMP instead" to error message, suggesting user
    the right interface to use
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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/monitor.c b/monitor.c
index 39f8ee1..0ffdf1d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3374,6 +3374,12 @@ 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, "
+                            "use QMP instead\n");
+        return;
+    }
+
     cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
     if (!cmd) {
         return;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 05/11] qapi: introduce new cmd option "allowed-in-preconfig"
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (3 preceding siblings ...)
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-11 15:26   ` Eric Blake
  2018-05-11 16:51   ` [Qemu-devel] [PATCH v8 05/11] qapi: introduce new cmd option "allow-preconfig" Igor Mammedov
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 06/11] tests: qapi-schema tests for allow-preconfig Igor Mammedov
                   ` (7 subsequent siblings)
  12 siblings, 2 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

New option will be used to allow commands, which are prepared/need
to run, during preconfig state. Other commands that should be able
to run in preconfig state, should be amended 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 permitted 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-command-line-options
   query-status
   exit-preconfig
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 patches.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v7:
  - (Eric Blake <eblake@redhat.com>)
    * s/allowed-in-preconfig/allow-preconfig/
    * s/allowed_in_preconfig/allow_preconfig/
    * move here QCO_ALLOWED_IN_PRECONFIG declaration from
       'cli: add --preconfig option'
      and put this patch before it as well
    * s/QCO_ALLOWED_IN_PRECONFIG/QCO_ALLOW_PRECONFIG/
    * wording fixes in doc
v6:
  * exclude 'cont' command from preconfig enabled, in favor of
    exit-preconfig command
  * mark exit-preconfig with allowed-in-preconfig=true
v5:
  * allow query-command-line-options in preconfig state
  * rebase on top of OOB changes that's now in master
  * spelling/wording fixups
  * make sure that allowed-in-preconfig could be set only to True
  * move out QCO_ALLOWED_IN_PRECONFIG check in do_qmp_dispatch() to
    earlier 'cli: add -preconfig option' patch
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    |  1 +
 docs/devel/qapi-code-gen.txt   | 10 +++++++++-
 monitor.c                      |  5 ++---
 qapi/introspect.json           |  5 ++++-
 qapi/misc.json                 |  9 ++++++---
 qapi/run-state.json            |  3 ++-
 scripts/qapi/commands.py       | 11 +++++++----
 scripts/qapi/common.py         | 18 +++++++++++-------
 scripts/qapi/doc.py            |  4 ++--
 scripts/qapi/introspect.py     |  7 ++++---
 tests/qapi-schema/test-qapi.py |  4 ++--
 11 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index ffb4652..b366bb4 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -23,6 +23,7 @@ typedef enum QmpCommandOptions
     QCO_NO_OPTIONS            =  0x0,
     QCO_NO_SUCCESS_RESP       =  (1U << 0),
     QCO_ALLOW_OOB             =  (1U << 1),
+    QCO_ALLOW_PRECONFIG       =  (1U << 2),
 } QmpCommandOptions;
 
 typedef struct QmpCommand
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index a569d24..0f9fbea 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -559,7 +559,7 @@ following example objects:
 Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
          '*returns': TYPE-NAME, '*boxed': true,
          '*gen': false, '*success-response': false,
-         '*allow-oob': true }
+         '*allow-oob': true, '*allow-preconfig': true }
 
 Commands are defined by using a dictionary containing several members,
 where three members are most common.  The 'command' member is a
@@ -683,6 +683,14 @@ OOB command handlers must satisfy the following conditions:
 
 If in doubt, do not implement OOB execution support.
 
+A command may use optional 'allow-preconfig' key to permit its execution
+at early runtime configuration stage (preconfig runstate).
+If not specified then a command defaults to 'allow-preconfig': false
+
+An example of declaring a command that is enabled during preconfig:
+ { 'command': 'qmp_capabilities',
+   'allow-preconfig': true }
+
 === Events ===
 
 Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
diff --git a/monitor.c b/monitor.c
index 0ffdf1d..0dc3fdb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1182,8 +1182,7 @@ static void monitor_init_qmp_commands(void)
     qmp_init_marshal(&qmp_commands);
 
     qmp_register_command(&qmp_commands, "query-qmp-schema",
-                         qmp_query_qmp_schema,
-                         QCO_NO_OPTIONS);
+                         qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
     qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
                          QCO_NO_OPTIONS);
     qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
@@ -1193,7 +1192,7 @@ static 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_ALLOW_PRECONFIG);
 }
 
 static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap)
diff --git a/qapi/introspect.json b/qapi/introspect.json
index c7f67b7..e8a833a 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -262,13 +262,16 @@
 # @allow-oob: whether the command allows out-of-band execution.
 #             (Since: 2.12)
 #
+# @allow-preconfig: command can be executed in preconfig runstate,
+#                   default: false (Since 2.13)
+#
 # TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
 #
 # Since: 2.5
 ##
 { 'struct': 'SchemaInfoCommand',
   'data': { 'arg-type': 'str', 'ret-type': 'str',
-            'allow-oob': 'bool' } }
+            'allow-oob': 'bool', 'allow-preconfig': 'bool' } }
 
 ##
 # @SchemaInfoEvent:
diff --git a/qapi/misc.json b/qapi/misc.json
index 5636f4a..5c8be28 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -35,7 +35,8 @@
 #
 ##
 { 'command': 'qmp_capabilities',
-  'data': { '*enable': [ 'QMPCapability' ] } }
+  'data': { '*enable': [ 'QMPCapability' ] },
+  'allow-preconfig': true }
 
 ##
 # @QMPCapability:
@@ -153,7 +154,8 @@
 # Note: This example has been shortened as the real response is too long.
 #
 ##
-{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
+{ 'command': 'query-commands', 'returns': ['CommandInfo'],
+  'allow-preconfig': true }
 
 ##
 # @LostTickPolicy:
@@ -2614,7 +2616,8 @@
 #
 ##
 {'command': 'query-command-line-options', 'data': { '*option': 'str' },
- 'returns': ['CommandLineOptionInfo'] }
+ 'returns': ['CommandLineOptionInfo'],
+ 'allow-preconfig': true }
 
 ##
 # @X86CPURegister32:
diff --git a/qapi/run-state.json b/qapi/run-state.json
index 9694a9f..444b252 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',
+  'allow-preconfig': true }
 
 ##
 # @SHUTDOWN:
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 0c5da3a..3b0867c 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -193,13 +193,15 @@ out:
     return ret
 
 
-def gen_register_command(name, success_response, allow_oob):
+def gen_register_command(name, success_response, allow_oob, allow_preconfig):
     options = []
 
     if not success_response:
         options += ['QCO_NO_SUCCESS_RESP']
     if allow_oob:
         options += ['QCO_ALLOW_OOB']
+    if allow_preconfig:
+        options += ['QCO_ALLOW_PRECONFIG']
 
     if not options:
         options = ['QCO_NO_OPTIONS']
@@ -275,8 +277,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
                        c_prefix=c_name(self._prefix, protect=False)))
         genc.add(gen_registry(self._regy, self._prefix))
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_preconfig):
         if not gen:
             return
         self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
@@ -285,7 +287,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, allow_oob)
+        self._regy += gen_register_command(name, success_response, allow_oob,
+                                           allow_preconfig)
 
 
 def gen_commands(schema, output_dir, prefix):
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 3e14bc4..454f9ee 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -872,7 +872,8 @@ def check_keys(expr_elem, meta, required, optional=[]):
             raise QAPISemError(info,
                                "'%s' of %s '%s' should only use false value"
                                % (key, meta, name))
-        if (key == 'boxed' or key == 'allow-oob') and value is not True:
+        if (key == 'boxed' or key == 'allow-oob' or
+            key == 'allow-preconfig') and value is not True:
             raise QAPISemError(info,
                                "'%s' of %s '%s' should only use true value"
                                % (key, meta, name))
@@ -922,7 +923,7 @@ def check_exprs(exprs):
             meta = 'command'
             check_keys(expr_elem, 'command', [],
                        ['data', 'returns', 'gen', 'success-response',
-                        'boxed', 'allow-oob'])
+                        'boxed', 'allow-oob', 'allow-preconfig'])
         elif 'event' in expr:
             meta = 'event'
             check_keys(expr_elem, 'event', [], ['data', 'boxed'])
@@ -1044,8 +1045,8 @@ class QAPISchemaVisitor(object):
     def visit_alternate_type(self, name, info, variants):
         pass
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_preconfig):
         pass
 
     def visit_event(self, name, info, arg_type, boxed):
@@ -1422,7 +1423,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
 
 class QAPISchemaCommand(QAPISchemaEntity):
     def __init__(self, name, info, doc, arg_type, ret_type,
-                 gen, success_response, boxed, allow_oob):
+                 gen, success_response, boxed, allow_oob, allow_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)
@@ -1434,6 +1435,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         self.success_response = success_response
         self.boxed = boxed
         self.allow_oob = allow_oob
+        self.allow_preconfig = allow_preconfig
 
     def check(self, schema):
         if self._arg_type_name:
@@ -1458,7 +1460,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
         visitor.visit_command(self.name, self.info,
                               self.arg_type, self.ret_type,
                               self.gen, self.success_response,
-                              self.boxed, self.allow_oob)
+                              self.boxed, self.allow_oob,
+                              self.allow_preconfig)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1678,6 +1681,7 @@ class QAPISchema(object):
         success_response = expr.get('success-response', True)
         boxed = expr.get('boxed', False)
         allow_oob = expr.get('allow-oob', False)
+        allow_preconfig = expr.get('allow-preconfig', False)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, doc, 'arg', self._make_members(data, info))
@@ -1686,7 +1690,7 @@ class QAPISchema(object):
             rets = self._make_array_type(rets[0], info)
         self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
                                            gen, success_response,
-                                           boxed, allow_oob))
+                                           boxed, allow_oob, allow_preconfig))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 9b312b2..b563084 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -226,8 +226,8 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
                                name=doc.symbol,
                                body=texi_entity(doc, 'Members')))
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_preconfig):
         doc = self.cur_doc
         if boxed:
             body = texi_body(doc)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index f9e67e8..5b6c72c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -171,14 +171,15 @@ const QLitObject %(c_name)s = %(c_string)s;
                        {'members': [{'type': self._use_type(m.type)}
                                     for m in variants.variants]})
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_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_qlit(name, 'command',
                        {'arg-type': self._use_type(arg_type),
                         'ret-type': self._use_type(ret_type),
-                        'allow-oob': allow_oob})
+                        'allow-oob': allow_oob,
+                        'allow-preconfig': allow_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 c1a144b..89b92ed 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -41,8 +41,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         print('alternate %s' % name)
         self._print_variants(variants)
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_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 oob=%s' % \
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 06/11] tests: qapi-schema tests for allow-preconfig
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (4 preceding siblings ...)
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 05/11] qapi: introduce new cmd option "allowed-in-preconfig" Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-11 15:29   ` Eric Blake
  2018-05-11 17:15   ` [Qemu-devel] [PATCH v8 " Igor Mammedov
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 07/11] cli: add --preconfig option Igor Mammedov
                   ` (6 subsequent siblings)
  12 siblings, 2 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

use new allow-preconfig parameter in tests and make sure that
the QAPISchema can parse allow-preconfig correctly

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
v7:
  * s/allowed_in_preconfig/allow_preconfig/
  * squash in "tests: add allow-preconfig-test for qapi-schema"
  * reuse an-oob-command for allow-preconfig test and rename it to test-flags-command
---
 tests/Makefile.include                      |  1 +
 tests/qapi-schema/allow-preconfig-test.err  |  1 +
 tests/qapi-schema/allow-preconfig-test.exit |  1 +
 tests/qapi-schema/allow-preconfig-test.json |  2 ++
 tests/qapi-schema/allow-preconfig-test.out  |  0
 tests/qapi-schema/doc-good.out              |  4 ++--
 tests/qapi-schema/ident-with-escape.out     |  2 +-
 tests/qapi-schema/indented-expr.out         |  4 ++--
 tests/qapi-schema/qapi-schema-test.json     |  4 ++--
 tests/qapi-schema/qapi-schema-test.out      | 22 +++++++++++-----------
 tests/qapi-schema/test-qapi.py              |  4 ++--
 tests/test-qmp-cmds.c                       |  2 +-
 12 files changed, 26 insertions(+), 21 deletions(-)
 create mode 100644 tests/qapi-schema/allow-preconfig-test.err
 create mode 100644 tests/qapi-schema/allow-preconfig-test.exit
 create mode 100644 tests/qapi-schema/allow-preconfig-test.json
 create mode 100644 tests/qapi-schema/allow-preconfig-test.out

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e3..74ed400 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -524,6 +524,7 @@ qapi-schema += missing-type.json
 qapi-schema += nested-struct-data.json
 qapi-schema += non-objects.json
 qapi-schema += oob-test.json
+qapi-schema += allow-preconfig-test.json
 qapi-schema += pragma-doc-required-crap.json
 qapi-schema += pragma-extra-junk.json
 qapi-schema += pragma-name-case-whitelist-crap.json
diff --git a/tests/qapi-schema/allow-preconfig-test.err b/tests/qapi-schema/allow-preconfig-test.err
new file mode 100644
index 0000000..700d583
--- /dev/null
+++ b/tests/qapi-schema/allow-preconfig-test.err
@@ -0,0 +1 @@
+tests/qapi-schema/allow-preconfig-test.json:2: 'allow-preconfig' of command 'allow-preconfig-test' should only use true value
diff --git a/tests/qapi-schema/allow-preconfig-test.exit b/tests/qapi-schema/allow-preconfig-test.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/allow-preconfig-test.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/allow-preconfig-test.json b/tests/qapi-schema/allow-preconfig-test.json
new file mode 100644
index 0000000..d9f0e91
--- /dev/null
+++ b/tests/qapi-schema/allow-preconfig-test.json
@@ -0,0 +1,2 @@
+# Check against allow-preconfig illegal value
+{ 'command': 'allow-preconfig-test', 'allow-preconfig': 'some-string' }
diff --git a/tests/qapi-schema/allow-preconfig-test.out b/tests/qapi-schema/allow-preconfig-test.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 63058b1..9c8a483 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -28,9 +28,9 @@ object q_obj_cmd-arg
     member arg2: str optional=True
     member arg3: bool optional=False
 command cmd q_obj_cmd-arg -> Object
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 command cmd-boxed Object -> None
-   gen=True success_response=True boxed=True oob=False
+   gen=True success_response=True boxed=True oob=False preconfig=False
 doc freeform
     body=
 = Section
diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index 82213aa..24c976f 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -5,4 +5,4 @@ module ident-with-escape.json
 object q_obj_fooA-arg
     member bar1: str optional=False
 command fooA q_obj_fooA-arg -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 862678f..bd8a486 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -3,6 +3,6 @@ enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
     prefix QTYPE
 module indented-expr.json
 command eins None -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 command zwei None -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 06e30f4..46c7282 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -139,8 +139,8 @@
 { 'command': 'boxed-struct', 'boxed': true, 'data': 'UserDefZero' }
 { 'command': 'boxed-union', 'data': 'UserDefNativeListUnion', 'boxed': true }
 
-# Smoke test on Out-Of-Band
-{ 'command': 'an-oob-command', 'allow-oob': true }
+# Smoke test on Out-Of-Band and allow-preconfig-test
+{ 'command': 'test-flags-command', 'allow-oob': true, 'allow-preconfig': true }
 
 # For testing integer range flattening in opts-visitor. The following schema
 # corresponds to the option format:
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 467577d..542a19c 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -16,7 +16,7 @@ object Empty1
 object Empty2
     base Empty1
 command user_def_cmd0 Empty2 -> Empty2
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 enum QEnumTwo ['value1', 'value2']
     prefix QENUM_TWO
 object UserDefOne
@@ -143,31 +143,31 @@ object UserDefNativeListUnion
     case sizes: q_obj_sizeList-wrapper
     case any: q_obj_anyList-wrapper
 command user_def_cmd None -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_user_def_cmd1-arg
     member ud1a: UserDefOne optional=False
 command user_def_cmd1 q_obj_user_def_cmd1-arg -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_user_def_cmd2-arg
     member ud1a: UserDefOne optional=False
     member ud1b: UserDefOne optional=True
 command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True
 command guest-get-time q_obj_guest-get-time-arg -> int
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_guest-sync-arg
     member arg: any optional=False
 command guest-sync q_obj_guest-sync-arg -> any
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 command boxed-struct UserDefZero -> None
-   gen=True success_response=True boxed=True oob=False
+   gen=True success_response=True boxed=True oob=False preconfig=False
 command boxed-union UserDefNativeListUnion -> None
-   gen=True success_response=True boxed=True oob=False
-command an-oob-command None -> None
-   gen=True success_response=True boxed=False oob=True
+   gen=True success_response=True boxed=True oob=False preconfig=False
+command test-flags-command None -> None
+   gen=True success_response=True boxed=False oob=True preconfig=True
 object UserDefOptions
     member i64: intList optional=True
     member u64: uint64List optional=True
@@ -231,4 +231,4 @@ object q_obj___org.qemu_x-command-arg
     member c: __org.qemu_x-Union2 optional=False
     member d: __org.qemu_x-Alt optional=False
 command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 89b92ed..4512a41 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -45,8 +45,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
                       success_response, boxed, allow_oob, allow_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 oob=%s' % \
-              (gen, success_response, boxed, allow_oob))
+        print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s' % \
+              (gen, success_response, boxed, allow_oob, allow_preconfig))
 
     def visit_event(self, name, info, arg_type, boxed):
         print('event %s %s' % (name, arg_type and arg_type.name))
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index db690cc..3714271 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -16,7 +16,7 @@ void qmp_user_def_cmd(Error **errp)
 {
 }
 
-void qmp_an_oob_command(Error **errp)
+void qmp_test_flags_command(Error **errp)
 {
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 07/11] cli: add --preconfig option
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (5 preceding siblings ...)
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 06/11] tests: qapi-schema tests for allow-preconfig Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-11 15:36   ` Eric Blake
  2018-05-11 17:24   ` [Qemu-devel] [PATCH v8 " Igor Mammedov
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 08/11] tests: extend qmp test with preconfig checks Igor Mammedov
                   ` (5 subsequent siblings)
  12 siblings, 2 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

This option allows pausing QEMU in the new RUN_STATE_PRECONFIG state,
allowing the configuration of QEMU from QMP before the machine jumps
into board initialization code of machine_run_board_init()

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

The new option complements -S option and could be used with or without
it. The difference is that -S pauses QEMU when the machine is completely
initialized with all devices wired up and ready to execute guest code
(QEMU needs only to unpause VCPUs to let guest execute its code),
while the "preconfig" option pauses QEMU early before board specific init
callback (machine_run_board_init) is executed and allows the configuration
of machine parameters which will be used by board init code.

When early introspection/configuration is done, command 'exit-preconfig'
should be used to exit RUN_STATE_PRECONFIG and transition to the next
requested state (i.e. if -S is used then QEMU will pause the second
time when board/device initialization is completed or start guest
execution if -S isn't provided on CLI)

PS:
Initially 'preconfig' is planned to be used for configuring numa
topology depending on board specified possible cpus layout.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v7:
  - (Eric Blake <eblake@redhat.com>)
    * spelling/wording fixes in newelly added documentation
    * s/allowed-in-preconfig/allow-preconfig/
    * s/QCO_ALLOWED_IN_PRECONFIG/QCO_ALLOW_PRECONFIG/
v6:
  - (Eduardo Habkost <ehabkost@redhat.com>)
    * add exit-preconfig QMP command instead of overloading meaning of 'cont' command
    * add doc text to qemu-tech.texi about -S and --preconfig
v5:
  - (Eric Blake <eblake@redhat.com>)
    * more spelling/wording fixes
    * s/-preconfig/--preconfig/
    * s/2.12/2.13/
  - (Eduardo Habkost <ehabkost@redhat.com>)
    * move QCO_ALLOWED_IN_PRECONFIG and do_qmp_dispatch() runstate check
      from the later patch 'qapi: introduce new cmd option  "allowed-in-preconfig"'
      to here for better bissectability
    * add TODO comment to '{ RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE }' transition
    * check for incoming && preconfig outside of option parsing loop
v4:
  * Explain more on behaviour in commit message and use suggested
    wording in message and patch (Eric Blake <eblake@redhat.com>)

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

preconfig
---
 include/sysemu/sysemu.h |  1 +
 qapi/misc.json          | 23 +++++++++++++++++++++++
 qapi/qmp-dispatch.c     |  8 ++++++++
 qemu-options.hx         | 13 +++++++++++++
 qemu-tech.texi          | 40 ++++++++++++++++++++++++++++++++++++++++
 qmp.c                   | 10 ++++++++++
 vl.c                    | 35 ++++++++++++++++++++++++++++++++++-
 7 files changed, 129 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 544ab77..e893f72 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/misc.json b/qapi/misc.json
index 5c8be28..eb7bc4c 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1211,6 +1211,29 @@
 { 'command': 'cont' }
 
 ##
+# @exit-preconfig:
+#
+# Exit from "preconfig" state
+#
+# This command makes QEMU exit the preconfig state and proceed with
+# VM initialization using configuration data provided on the command line
+# and via the QMP monitor during the preconfig state. The command is only
+# available during the preconfig state (i.e. when the --preconfig command
+# line option was in use).
+#
+# Since 2.13
+#
+# Returns: nothing
+#
+# Example:
+#
+# -> { "execute": "exit-preconfig" }
+# <- { "return": {} }
+#
+##
+{ 'command': 'exit-preconfig', 'allow-preconfig': true }
+
+##
 # @system_wakeup:
 #
 # Wakeup guest from suspend.  Does nothing in case the guest isn't suspended.
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index dd05907..a4e2a28 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -18,6 +18,7 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qbool.h"
+#include "sysemu/sysemu.h"
 
 QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
 {
@@ -101,6 +102,13 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
         return NULL;
     }
 
+    if (runstate_check(RUN_STATE_PRECONFIG) &&
+        !(cmd->options & QCO_ALLOW_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/qemu-options.hx b/qemu-options.hx
index c611766..6ba2a4f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3304,6 +3304,19 @@ STEXI
 Run the emulation in single step mode.
 ETEXI
 
+DEF("preconfig", 0, QEMU_OPTION_preconfig, \
+    "--preconfig     pause QEMU before machine is initialized\n",
+    QEMU_ARCH_ALL)
+STEXI
+@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 'exit-preconfig' to exit
+the preconfig state and move to the next state (ie. run guest if -S
+isn't used or pause the second time if -S is used).
+ETEXI
+
 DEF("S", 0, QEMU_OPTION_S, \
     "-S              freeze CPU at startup (use 'c' to start execution)\n",
     QEMU_ARCH_ALL)
diff --git a/qemu-tech.texi b/qemu-tech.texi
index 52a56ae..a2b8c6d 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,45 @@ 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 a VM in a paused state using
+the -S command line option. In this state the machine is completely initialized
+according to command line options and ready to execute VM code but VCPU threads
+are not executing any code. The VM state in this paused state depends on the way
+QEMU was started. It could be in:
+@table @asis
+@item initial state (after reset/power on state)
+@item with direct kernel loading, the initial state could be amended to execute
+code loaded by QEMU in the VM's RAM and with incoming migration
+@item with incoming migration, initial state will by amended with 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 the machine (by hotplugging devices) in runtime before
+allowing VM code to run.
+
+However, at the -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 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. 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
+@item exit-preconfig
+@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
+'allow-preconfig' set to true.
+
 @node Bibliography
 @section Bibliography
 
diff --git a/qmp.c b/qmp.c
index f722616..9aace9c 100644
--- a/qmp.c
+++ b/qmp.c
@@ -161,6 +161,16 @@ SpiceInfo *qmp_query_spice(Error **errp)
 };
 #endif
 
+void qmp_exit_preconfig(Error **errp)
+{
+    if (!runstate_check(RUN_STATE_PRECONFIG)) {
+        error_setg(errp, "The command is permitted only in '%s' state",
+                   RunState_str(RUN_STATE_PRECONFIG));
+        return;
+    }
+    qemu_exit_preconfig_request();
+}
+
 void qmp_cont(Error **errp)
 {
     BlockBackend *blk;
diff --git a/vl.c b/vl.c
index 7487535..f133470 100644
--- a/vl.c
+++ b/vl.c
@@ -595,7 +595,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;
@@ -608,6 +608,13 @@ typedef struct {
 
 static const RunStateTransition runstate_transitions_def[] = {
     /*     from      ->     to      */
+    { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
+      /* 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 },
+
     { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
     { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
     { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
@@ -1631,6 +1638,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);
@@ -1715,6 +1723,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.
  */
@@ -1888,6 +1901,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);
     }
@@ -3721,6 +3741,9 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
+            case QEMU_OPTION_preconfig:
+                preconfig_exit_requested = false;
+                break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse_noisily(olist, "accel=kvm", false);
@@ -4090,6 +4113,12 @@ int main(int argc, char **argv, char **envp)
 
     replay_configure(icount_opts);
 
+    if (incoming && !preconfig_exit_requested) {
+        error_report("'preconfig' and 'incoming' options are "
+                     "mutually exclusive");
+        exit(EXIT_FAILURE);
+    }
+
     machine_class = select_machine();
 
     set_memory_options(&ram_slots, &maxram_size, machine_class);
@@ -4621,6 +4650,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

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 08/11] tests: extend qmp test with preconfig checks
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (6 preceding siblings ...)
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 07/11] cli: add --preconfig option Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-16 18:20   ` Eduardo Habkost
  2018-05-16 22:21   ` Eric Blake
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 09/11] qmp: permit query-hotpluggable-cpus in preconfig state Igor Mammedov
                   ` (4 subsequent siblings)
  12 siblings, 2 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

Add permission checks for commands at 'preconfig' stage.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v6:
  * replace 'cont' with 'exit-preconfig' command
v5:
  * s/-preconfig/--preconfig/
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 772058f..c49837a 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -392,6 +392,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': 'exit-preconfig' }")));
+    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;
@@ -403,6 +446,7 @@ int main(int argc, char *argv[])
     qtest_add_func("qmp/oob", test_qmp_oob);
     qmp_schema_init(&schema);
     add_query_tests(&schema);
+    qtest_add_func("qmp/preconfig", test_qmp_preconfig);
 
     ret = g_test_run();
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 09/11] qmp: permit query-hotpluggable-cpus in preconfig state
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (7 preceding siblings ...)
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 08/11] tests: extend qmp test with preconfig checks Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-16 18:24   ` Eduardo Habkost
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 10/11] qmp: add set-numa-node command Igor Mammedov
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

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
---
v7:
  * s/allowed-in-preconfig/allow-preconfig/
---
 qapi/misc.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index eb7bc4c..12ae310 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3251,7 +3251,8 @@
 #    ]}
 #
 ##
-{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
+{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'],
+             'allow-preconfig': true }
 
 ##
 # @GuidInfo:
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 10/11] qmp: add set-numa-node command
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (8 preceding siblings ...)
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 09/11] qmp: permit query-hotpluggable-cpus in preconfig state Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-16 18:26   ` Eduardo Habkost
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node Igor Mammedov
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

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.

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

QMP:
-> {'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}, ... }
   ]}

-> {'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': {}}

-> {'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': {}}

-> {'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}, ... }
   ]}

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v7:
 * s/allowed-in-preconfig/allow-preconfig/
v6:
 * limit command to preconfig state only
 * add example QMP command sequence
---
 numa.c         | 11 +++++++++++
 qapi/misc.json | 14 ++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/numa.c b/numa.c
index 63e3989..760aef0 100644
--- a/numa.c
+++ b/numa.c
@@ -444,6 +444,17 @@ void parse_numa_opts(MachineState *ms)
     }
 }
 
+void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
+{
+    if (!runstate_check(RUN_STATE_PRECONFIG)) {
+        error_setg(errp, "The command is permitted only in '%s' state",
+                   RunState_str(RUN_STATE_PRECONFIG));
+         return;
+    }
+
+    set_numa_options(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 12ae310..79dfd39 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -3476,3 +3476,17 @@
 ##
 { 'command': 'x-oob-test', 'data' : { 'lock': 'bool' },
   'allow-oob': true }
+
+##
+# @set-numa-node:
+#
+# Runtime equivalent of '-numa' CLI option, available at
+# preconfigure stage to configure numa mapping before initializing
+# machine.
+#
+# Since 2.13
+##
+{ 'command': 'set-numa-node', 'boxed': true,
+  'data': 'NumaOptions',
+  'allow-preconfig': true
+}
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (9 preceding siblings ...)
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 10/11] qmp: add set-numa-node command Igor Mammedov
@ 2018-05-04  8:37 ` Igor Mammedov
  2018-05-16 18:27   ` Eduardo Habkost
                     ` (2 more replies)
  2018-05-16 19:02 ` [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Eduardo Habkost
  2018-05-30 10:47 ` Igor Mammedov
  12 siblings, 3 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-04  8:37 UTC (permalink / raw)
  To: qemu-devel; +Cc: eblake, ehabkost, pkrempa, armbru

 * 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>
---
v6:
  * replace 'cont' with 'exit-preconfig' command
v5:
  * s/qobject_to_qdict(/qobject_to(QDict,/
  * s/-preconfig/--preconfig/
v4:
  * drop duplicate is_err() and reuse qmp_rsp_is_err() wich is moved
    to generic file libqtest.c. (Eric Blake <eblake@redhat.com>)

FIXUP! tests: functional tests for QMP command  set-numa-node
---
 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 cbe8df4..ac52872 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -972,4 +972,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 6f33a37..33426d5 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -1098,3 +1098,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 0f861d8..0135a0c 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': 'exit-preconfig' }")));
+    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 c49837a..4d5f198 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -392,13 +392,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

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 03/11] qapi: introduce preconfig runstate
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 03/11] qapi: introduce preconfig runstate Igor Mammedov
@ 2018-05-11 15:17   ` Eric Blake
  2018-05-16 18:14   ` Eduardo Habkost
  1 sibling, 0 replies; 48+ messages in thread
From: Eric Blake @ 2018-05-11 15:17 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel; +Cc: ehabkost, pkrempa, armbru

On 05/04/2018 03:37 AM, Igor Mammedov wrote:
> New preconfig runstate will be used in follow up patches
> related to introducing --preconfig CLI option and is
> intended to replace prelaunch runstate from QEMU start
> up to machine_init callback.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   qapi/run-state.json | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 1c9fff3..9694a9f 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 the --preconfig CLI option is used.
> +#             (Since 2.13)
>   ##
>   { '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:
> 

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 05/11] qapi: introduce new cmd option "allowed-in-preconfig"
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 05/11] qapi: introduce new cmd option "allowed-in-preconfig" Igor Mammedov
@ 2018-05-11 15:26   ` Eric Blake
  2018-05-11 16:56     ` Igor Mammedov
  2018-05-11 16:51   ` [Qemu-devel] [PATCH v8 05/11] qapi: introduce new cmd option "allow-preconfig" Igor Mammedov
  1 sibling, 1 reply; 48+ messages in thread
From: Eric Blake @ 2018-05-11 15:26 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel; +Cc: ehabkost, pkrempa, armbru

On 05/04/2018 03:37 AM, Igor Mammedov wrote:

Subject line is stale, needs to be updated to match new spelling...

> New option will be used to allow commands, which are prepared/need
> to run, during preconfig state. Other commands that should be able
> to run in preconfig state, should be amended 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 permitted to run in

another stale comment...

> 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-command-line-options
>     query-status
>     exit-preconfig
> 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 patches.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v7:
>    - (Eric Blake <eblake@redhat.com>)
>      * s/allowed-in-preconfig/allow-preconfig/

...used in the rest of the patch.

>      * s/allowed_in_preconfig/allow_preconfig/
>      * move here QCO_ALLOWED_IN_PRECONFIG declaration from
>         'cli: add --preconfig option'
>        and put this patch before it as well
>      * s/QCO_ALLOWED_IN_PRECONFIG/QCO_ALLOW_PRECONFIG/
>      * wording fixes in doc

> +++ b/include/qapi/qmp/dispatch.h
> @@ -23,6 +23,7 @@ typedef enum QmpCommandOptions
>       QCO_NO_OPTIONS            =  0x0,
>       QCO_NO_SUCCESS_RESP       =  (1U << 0),
>       QCO_ALLOW_OOB             =  (1U << 1),
> +    QCO_ALLOW_PRECONFIG       =  (1U << 2),
>   } QmpCommandOptions;
>   
>   typedef struct QmpCommand
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index a569d24..0f9fbea 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -559,7 +559,7 @@ following example objects:
>   Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
>            '*returns': TYPE-NAME, '*boxed': true,
>            '*gen': false, '*success-response': false,
> -         '*allow-oob': true }
> +         '*allow-oob': true, '*allow-preconfig': true }

Thanks for taking on the bikeshed renaming; it does look better now that 
the two flags have similar spellings.

>   
>   Commands are defined by using a dictionary containing several members,
>   where three members are most common.  The 'command' member is a
> @@ -683,6 +683,14 @@ OOB command handlers must satisfy the following conditions:
>   
>   If in doubt, do not implement OOB execution support.
>   
> +A command may use optional 'allow-preconfig' key to permit its execution

s/use/use the/

> +at early runtime configuration stage (preconfig runstate).
> +If not specified then a command defaults to 'allow-preconfig': false

trailing '.'

> +
> +An example of declaring a command that is enabled during preconfig:
> + { 'command': 'qmp_capabilities',
> +   'allow-preconfig': true }

It might be worth having this example match our current schema, as in:

{ 'command': 'qmp_capabilities',
   'allow-preconfig': true,
   'data': { '*enable': [ 'QMPCapability' ] } }

> +++ b/qapi/misc.json
> @@ -35,7 +35,8 @@
>   #
>   ##
>   { 'command': 'qmp_capabilities',
> -  'data': { '*enable': [ 'QMPCapability' ] } }
> +  'data': { '*enable': [ 'QMPCapability' ] },
> +  'allow-preconfig': true }

Or the way you actually wrote it ;)

Otherwise looks pretty good.

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 06/11] tests: qapi-schema tests for allow-preconfig
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 06/11] tests: qapi-schema tests for allow-preconfig Igor Mammedov
@ 2018-05-11 15:29   ` Eric Blake
  2018-05-16 18:16     ` Eduardo Habkost
  2018-05-11 17:15   ` [Qemu-devel] [PATCH v8 " Igor Mammedov
  1 sibling, 1 reply; 48+ messages in thread
From: Eric Blake @ 2018-05-11 15:29 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel; +Cc: ehabkost, pkrempa, armbru

On 05/04/2018 03:37 AM, Igor Mammedov wrote:
> use new allow-preconfig parameter in tests and make sure that
> the QAPISchema can parse allow-preconfig correctly
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> v7:

Missing --- separator to stop 'git am' from including the changelog.

>    * s/allowed_in_preconfig/allow_preconfig/
>    * squash in "tests: add allow-preconfig-test for qapi-schema"
>    * reuse an-oob-command for allow-preconfig test and rename it to test-flags-command

Reviewed-by: Eric Blake <eblake@redhat.com>

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 07/11] cli: add --preconfig option
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 07/11] cli: add --preconfig option Igor Mammedov
@ 2018-05-11 15:36   ` Eric Blake
  2018-05-11 17:24   ` [Qemu-devel] [PATCH v8 " Igor Mammedov
  1 sibling, 0 replies; 48+ messages in thread
From: Eric Blake @ 2018-05-11 15:36 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel; +Cc: ehabkost, pkrempa, armbru

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

> +This paused state is typically used by users to query machine state and/or
> +additionally configure the machine (by hotplugging devices) in runtime before
> +allowing VM code to run.
> +
> +However, at the -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 pausing QEMU

s/when/when the/

> +before the initial VM creation, in a new preconfig state, where additional
> +queries and configuration can be performed via QMP before moving on to
> +the resulting configuration startup. In the preconfig state, QEMU only allows
> +a limited set of commands over the QMP monitor, where the commands do not
> +depend on an initialized machine, including but not limited to:

Reviewed-by: Eric Blake <eblake@redhat.com>

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v8 05/11] qapi: introduce new cmd option "allow-preconfig"
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 05/11] qapi: introduce new cmd option "allowed-in-preconfig" Igor Mammedov
  2018-05-11 15:26   ` Eric Blake
@ 2018-05-11 16:51   ` Igor Mammedov
  2018-05-11 17:16     ` Eric Blake
  1 sibling, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-11 16:51 UTC (permalink / raw)
  To: qemu-devel; +Cc: ehabkost, armbru, eblake

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

For compatibility reasons, commands that don't use new flag
'allow-preconfig' explicitly are not permitted 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-command-line-options
   query-status
   exit-preconfig
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 patches.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v8:
  - (Eric Blake <eblake@redhat.com>)
    * replace allowed-in-preconfig on allow-preconfig in commit msg
    * more wording fixes
    * make example in doc match QMP schema
v7:
  - (Eric Blake <eblake@redhat.com>)
    * s/allowed-in-preconfig/allow-preconfig/
    * s/allowed_in_preconfig/allow_preconfig/
    * move here QCO_ALLOWED_IN_PRECONFIG declaration from
       'cli: add --preconfig option'
      and put this patch before it as well
    * s/QCO_ALLOWED_IN_PRECONFIG/QCO_ALLOW_PRECONFIG/
    * wording fixes in doc
v6:
  * exclude 'cont' command from preconfig enabled, in favor of
    exit-preconfig command
  * mark exit-preconfig with allowed-in-preconfig=true
v5:
  * allow query-command-line-options in preconfig state
  * rebase on top of OOB changes that's now in master
  * spelling/wording fixups
  * make sure that allowed-in-preconfig could be set only to True
  * move out QCO_ALLOWED_IN_PRECONFIG check in do_qmp_dispatch() to
    earlier 'cli: add -preconfig option' patch
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    |  1 +
 docs/devel/qapi-code-gen.txt   | 11 ++++++++++-
 monitor.c                      |  5 ++---
 qapi/introspect.json           |  5 ++++-
 qapi/misc.json                 |  9 ++++++---
 qapi/run-state.json            |  3 ++-
 scripts/qapi/commands.py       | 11 +++++++----
 scripts/qapi/common.py         | 18 +++++++++++-------
 scripts/qapi/doc.py            |  4 ++--
 scripts/qapi/introspect.py     |  7 ++++---
 tests/qapi-schema/test-qapi.py |  4 ++--
 11 files changed, 51 insertions(+), 27 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index ffb4652..b366bb4 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -23,6 +23,7 @@ typedef enum QmpCommandOptions
     QCO_NO_OPTIONS            =  0x0,
     QCO_NO_SUCCESS_RESP       =  (1U << 0),
     QCO_ALLOW_OOB             =  (1U << 1),
+    QCO_ALLOW_PRECONFIG       =  (1U << 2),
 } QmpCommandOptions;
 
 typedef struct QmpCommand
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index b9b6eab..1366228 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -559,7 +559,7 @@ following example objects:
 Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
          '*returns': TYPE-NAME, '*boxed': true,
          '*gen': false, '*success-response': false,
-         '*allow-oob': true }
+         '*allow-oob': true, '*allow-preconfig': true }
 
 Commands are defined by using a dictionary containing several members,
 where three members are most common.  The 'command' member is a
@@ -683,6 +683,15 @@ OOB command handlers must satisfy the following conditions:
 
 If in doubt, do not implement OOB execution support.
 
+A command may use the optional 'allow-preconfig' key to permit its execution
+at early runtime configuration stage (preconfig runstate).
+If not specified then a command defaults to 'allow-preconfig': false.
+
+An example of declaring a command that is enabled during preconfig:
+ { 'command': 'qmp_capabilities',
+   'data': { '*enable': [ 'QMPCapability' ] },
+   'allow-preconfig': true }
+
 === Events ===
 
 Usage: { 'event': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
diff --git a/monitor.c b/monitor.c
index 9e50418..922cfc0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1179,8 +1179,7 @@ static void monitor_init_qmp_commands(void)
     qmp_init_marshal(&qmp_commands);
 
     qmp_register_command(&qmp_commands, "query-qmp-schema",
-                         qmp_query_qmp_schema,
-                         QCO_NO_OPTIONS);
+                         qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
     qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
                          QCO_NO_OPTIONS);
     qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
@@ -1190,7 +1189,7 @@ static 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_ALLOW_PRECONFIG);
 }
 
 static bool qmp_cap_enabled(Monitor *mon, QMPCapability cap)
diff --git a/qapi/introspect.json b/qapi/introspect.json
index c7f67b7..e8a833a 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -262,13 +262,16 @@
 # @allow-oob: whether the command allows out-of-band execution.
 #             (Since: 2.12)
 #
+# @allow-preconfig: command can be executed in preconfig runstate,
+#                   default: false (Since 2.13)
+#
 # TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
 #
 # Since: 2.5
 ##
 { 'struct': 'SchemaInfoCommand',
   'data': { 'arg-type': 'str', 'ret-type': 'str',
-            'allow-oob': 'bool' } }
+            'allow-oob': 'bool', 'allow-preconfig': 'bool' } }
 
 ##
 # @SchemaInfoEvent:
diff --git a/qapi/misc.json b/qapi/misc.json
index f5988cc..a16a7ef 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -37,7 +37,8 @@
 #
 ##
 { 'command': 'qmp_capabilities',
-  'data': { '*enable': [ 'QMPCapability' ] } }
+  'data': { '*enable': [ 'QMPCapability' ] },
+  'allow-preconfig': true }
 
 ##
 # @QMPCapability:
@@ -155,7 +156,8 @@
 # Note: This example has been shortened as the real response is too long.
 #
 ##
-{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
+{ 'command': 'query-commands', 'returns': ['CommandInfo'],
+  'allow-preconfig': true }
 
 ##
 # @LostTickPolicy:
@@ -2648,7 +2650,8 @@
 #
 ##
 {'command': 'query-command-line-options', 'data': { '*option': 'str' },
- 'returns': ['CommandLineOptionInfo'] }
+ 'returns': ['CommandLineOptionInfo'],
+ 'allow-preconfig': true }
 
 ##
 # @X86CPURegister32:
diff --git a/qapi/run-state.json b/qapi/run-state.json
index 9694a9f..444b252 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',
+  'allow-preconfig': true }
 
 ##
 # @SHUTDOWN:
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 0c5da3a..3b0867c 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -193,13 +193,15 @@ out:
     return ret
 
 
-def gen_register_command(name, success_response, allow_oob):
+def gen_register_command(name, success_response, allow_oob, allow_preconfig):
     options = []
 
     if not success_response:
         options += ['QCO_NO_SUCCESS_RESP']
     if allow_oob:
         options += ['QCO_ALLOW_OOB']
+    if allow_preconfig:
+        options += ['QCO_ALLOW_PRECONFIG']
 
     if not options:
         options = ['QCO_NO_OPTIONS']
@@ -275,8 +277,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
                        c_prefix=c_name(self._prefix, protect=False)))
         genc.add(gen_registry(self._regy, self._prefix))
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_preconfig):
         if not gen:
             return
         self._genh.add(gen_command_decl(name, arg_type, boxed, ret_type))
@@ -285,7 +287,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, allow_oob)
+        self._regy += gen_register_command(name, success_response, allow_oob,
+                                           allow_preconfig)
 
 
 def gen_commands(schema, output_dir, prefix):
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index a032cec..e82990f 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -872,7 +872,8 @@ def check_keys(expr_elem, meta, required, optional=[]):
             raise QAPISemError(info,
                                "'%s' of %s '%s' should only use false value"
                                % (key, meta, name))
-        if (key == 'boxed' or key == 'allow-oob') and value is not True:
+        if (key == 'boxed' or key == 'allow-oob' or
+            key == 'allow-preconfig') and value is not True:
             raise QAPISemError(info,
                                "'%s' of %s '%s' should only use true value"
                                % (key, meta, name))
@@ -922,7 +923,7 @@ def check_exprs(exprs):
             meta = 'command'
             check_keys(expr_elem, 'command', [],
                        ['data', 'returns', 'gen', 'success-response',
-                        'boxed', 'allow-oob'])
+                        'boxed', 'allow-oob', 'allow-preconfig'])
         elif 'event' in expr:
             meta = 'event'
             check_keys(expr_elem, 'event', [], ['data', 'boxed'])
@@ -1044,8 +1045,8 @@ class QAPISchemaVisitor(object):
     def visit_alternate_type(self, name, info, variants):
         pass
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_preconfig):
         pass
 
     def visit_event(self, name, info, arg_type, boxed):
@@ -1422,7 +1423,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
 
 class QAPISchemaCommand(QAPISchemaEntity):
     def __init__(self, name, info, doc, arg_type, ret_type,
-                 gen, success_response, boxed, allow_oob):
+                 gen, success_response, boxed, allow_oob, allow_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)
@@ -1434,6 +1435,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         self.success_response = success_response
         self.boxed = boxed
         self.allow_oob = allow_oob
+        self.allow_preconfig = allow_preconfig
 
     def check(self, schema):
         if self._arg_type_name:
@@ -1458,7 +1460,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
         visitor.visit_command(self.name, self.info,
                               self.arg_type, self.ret_type,
                               self.gen, self.success_response,
-                              self.boxed, self.allow_oob)
+                              self.boxed, self.allow_oob,
+                              self.allow_preconfig)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1678,6 +1681,7 @@ class QAPISchema(object):
         success_response = expr.get('success-response', True)
         boxed = expr.get('boxed', False)
         allow_oob = expr.get('allow-oob', False)
+        allow_preconfig = expr.get('allow-preconfig', False)
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, doc, 'arg', self._make_members(data, info))
@@ -1686,7 +1690,7 @@ class QAPISchema(object):
             rets = self._make_array_type(rets[0], info)
         self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
                                            gen, success_response,
-                                           boxed, allow_oob))
+                                           boxed, allow_oob, allow_preconfig))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 9b312b2..b563084 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -226,8 +226,8 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
                                name=doc.symbol,
                                body=texi_entity(doc, 'Members')))
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_preconfig):
         doc = self.cur_doc
         if boxed:
             body = texi_body(doc)
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index f9e67e8..5b6c72c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -171,14 +171,15 @@ const QLitObject %(c_name)s = %(c_string)s;
                        {'members': [{'type': self._use_type(m.type)}
                                     for m in variants.variants]})
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_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_qlit(name, 'command',
                        {'arg-type': self._use_type(arg_type),
                         'ret-type': self._use_type(ret_type),
-                        'allow-oob': allow_oob})
+                        'allow-oob': allow_oob,
+                        'allow-preconfig': allow_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 c1a144b..89b92ed 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -41,8 +41,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         print('alternate %s' % name)
         self._print_variants(variants)
 
-    def visit_command(self, name, info, arg_type, ret_type,
-                      gen, success_response, boxed, allow_oob):
+    def visit_command(self, name, info, arg_type, ret_type, gen,
+                      success_response, boxed, allow_oob, allow_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 oob=%s' % \
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 05/11] qapi: introduce new cmd option "allowed-in-preconfig"
  2018-05-11 15:26   ` Eric Blake
@ 2018-05-11 16:56     ` Igor Mammedov
  0 siblings, 0 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-11 16:56 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, ehabkost, pkrempa, armbru

On Fri, 11 May 2018 10:26:45 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 05/04/2018 03:37 AM, Igor Mammedov wrote:
> 
> Subject line is stale, needs to be updated to match new spelling...
> 
> > New option will be used to allow commands, which are prepared/need
> > to run, during preconfig state. Other commands that should be able
> > to run in preconfig state, should be amended 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 permitted to run in  
> 
> another stale comment...
> 
> > 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-command-line-options
> >     query-status
> >     exit-preconfig
> > 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 patches.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v7:
> >    - (Eric Blake <eblake@redhat.com>)
> >      * s/allowed-in-preconfig/allow-preconfig/  
> 
> ...used in the rest of the patch.
> 
> >      * s/allowed_in_preconfig/allow_preconfig/
> >      * move here QCO_ALLOWED_IN_PRECONFIG declaration from
> >         'cli: add --preconfig option'
> >        and put this patch before it as well
> >      * s/QCO_ALLOWED_IN_PRECONFIG/QCO_ALLOW_PRECONFIG/
> >      * wording fixes in doc  
> 
> > +++ b/include/qapi/qmp/dispatch.h
> > @@ -23,6 +23,7 @@ typedef enum QmpCommandOptions
> >       QCO_NO_OPTIONS            =  0x0,
> >       QCO_NO_SUCCESS_RESP       =  (1U << 0),
> >       QCO_ALLOW_OOB             =  (1U << 1),
> > +    QCO_ALLOW_PRECONFIG       =  (1U << 2),
> >   } QmpCommandOptions;
> >   
> >   typedef struct QmpCommand
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index a569d24..0f9fbea 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -559,7 +559,7 @@ following example objects:
> >   Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
> >            '*returns': TYPE-NAME, '*boxed': true,
> >            '*gen': false, '*success-response': false,
> > -         '*allow-oob': true }
> > +         '*allow-oob': true, '*allow-preconfig': true }  
> 
> Thanks for taking on the bikeshed renaming; it does look better now that 
> the two flags have similar spellings.

There was no reason to refuse good suggestion.


I've just posted fixed up v8 of this patch as reply here,
since changes don't affect other patches.

> 
> >   
> >   Commands are defined by using a dictionary containing several members,
> >   where three members are most common.  The 'command' member is a
> > @@ -683,6 +683,14 @@ OOB command handlers must satisfy the following conditions:
> >   
> >   If in doubt, do not implement OOB execution support.
> >   
> > +A command may use optional 'allow-preconfig' key to permit its execution  
> 
> s/use/use the/
> 
> > +at early runtime configuration stage (preconfig runstate).
> > +If not specified then a command defaults to 'allow-preconfig': false  
> 
> trailing '.'
> 
> > +
> > +An example of declaring a command that is enabled during preconfig:
> > + { 'command': 'qmp_capabilities',
> > +   'allow-preconfig': true }  
> 
> It might be worth having this example match our current schema, as in:
> 
> { 'command': 'qmp_capabilities',
>    'allow-preconfig': true,
>    'data': { '*enable': [ 'QMPCapability' ] } }
> 
> > +++ b/qapi/misc.json
> > @@ -35,7 +35,8 @@
> >   #
> >   ##
> >   { 'command': 'qmp_capabilities',
> > -  'data': { '*enable': [ 'QMPCapability' ] } }
> > +  'data': { '*enable': [ 'QMPCapability' ] },
> > +  'allow-preconfig': true }  
> 
> Or the way you actually wrote it ;)
> 
> Otherwise looks pretty good.
> 
Thanks for reviewing!

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v8 06/11] tests: qapi-schema tests for allow-preconfig
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 06/11] tests: qapi-schema tests for allow-preconfig Igor Mammedov
  2018-05-11 15:29   ` Eric Blake
@ 2018-05-11 17:15   ` Igor Mammedov
  1 sibling, 0 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-11 17:15 UTC (permalink / raw)
  To: qemu-devel; +Cc: ehabkost, armbru, eblake

use new allow-preconfig parameter in tests and make sure that
the QAPISchema can parse allow-preconfig correctly

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
v7:
  * s/allowed_in_preconfig/allow_preconfig/
  * squash in "tests: add allow-preconfig-test for qapi-schema"
  * reuse an-oob-command for allow-preconfig test and rename it to test-flags-command
---
 tests/Makefile.include                      |  1 +
 tests/qapi-schema/allow-preconfig-test.err  |  1 +
 tests/qapi-schema/allow-preconfig-test.exit |  1 +
 tests/qapi-schema/allow-preconfig-test.json |  2 ++
 tests/qapi-schema/allow-preconfig-test.out  |  0
 tests/qapi-schema/doc-good.out              |  4 ++--
 tests/qapi-schema/ident-with-escape.out     |  2 +-
 tests/qapi-schema/indented-expr.out         |  4 ++--
 tests/qapi-schema/qapi-schema-test.json     |  4 ++--
 tests/qapi-schema/qapi-schema-test.out      | 22 +++++++++++-----------
 tests/qapi-schema/test-qapi.py              |  4 ++--
 tests/test-qmp-cmds.c                       |  2 +-
 12 files changed, 26 insertions(+), 21 deletions(-)
 create mode 100644 tests/qapi-schema/allow-preconfig-test.err
 create mode 100644 tests/qapi-schema/allow-preconfig-test.exit
 create mode 100644 tests/qapi-schema/allow-preconfig-test.json
 create mode 100644 tests/qapi-schema/allow-preconfig-test.out

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e3..74ed400 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -524,6 +524,7 @@ qapi-schema += missing-type.json
 qapi-schema += nested-struct-data.json
 qapi-schema += non-objects.json
 qapi-schema += oob-test.json
+qapi-schema += allow-preconfig-test.json
 qapi-schema += pragma-doc-required-crap.json
 qapi-schema += pragma-extra-junk.json
 qapi-schema += pragma-name-case-whitelist-crap.json
diff --git a/tests/qapi-schema/allow-preconfig-test.err b/tests/qapi-schema/allow-preconfig-test.err
new file mode 100644
index 0000000..700d583
--- /dev/null
+++ b/tests/qapi-schema/allow-preconfig-test.err
@@ -0,0 +1 @@
+tests/qapi-schema/allow-preconfig-test.json:2: 'allow-preconfig' of command 'allow-preconfig-test' should only use true value
diff --git a/tests/qapi-schema/allow-preconfig-test.exit b/tests/qapi-schema/allow-preconfig-test.exit
new file mode 100644
index 0000000..d00491f
--- /dev/null
+++ b/tests/qapi-schema/allow-preconfig-test.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/allow-preconfig-test.json b/tests/qapi-schema/allow-preconfig-test.json
new file mode 100644
index 0000000..d9f0e91
--- /dev/null
+++ b/tests/qapi-schema/allow-preconfig-test.json
@@ -0,0 +1,2 @@
+# Check against allow-preconfig illegal value
+{ 'command': 'allow-preconfig-test', 'allow-preconfig': 'some-string' }
diff --git a/tests/qapi-schema/allow-preconfig-test.out b/tests/qapi-schema/allow-preconfig-test.out
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 63058b1..9c8a483 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -28,9 +28,9 @@ object q_obj_cmd-arg
     member arg2: str optional=True
     member arg3: bool optional=False
 command cmd q_obj_cmd-arg -> Object
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 command cmd-boxed Object -> None
-   gen=True success_response=True boxed=True oob=False
+   gen=True success_response=True boxed=True oob=False preconfig=False
 doc freeform
     body=
 = Section
diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index 82213aa..24c976f 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -5,4 +5,4 @@ module ident-with-escape.json
 object q_obj_fooA-arg
     member bar1: str optional=False
 command fooA q_obj_fooA-arg -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 862678f..bd8a486 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -3,6 +3,6 @@ enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
     prefix QTYPE
 module indented-expr.json
 command eins None -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 command zwei None -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 06e30f4..46c7282 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -139,8 +139,8 @@
 { 'command': 'boxed-struct', 'boxed': true, 'data': 'UserDefZero' }
 { 'command': 'boxed-union', 'data': 'UserDefNativeListUnion', 'boxed': true }
 
-# Smoke test on Out-Of-Band
-{ 'command': 'an-oob-command', 'allow-oob': true }
+# Smoke test on Out-Of-Band and allow-preconfig-test
+{ 'command': 'test-flags-command', 'allow-oob': true, 'allow-preconfig': true }
 
 # For testing integer range flattening in opts-visitor. The following schema
 # corresponds to the option format:
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 467577d..542a19c 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -16,7 +16,7 @@ object Empty1
 object Empty2
     base Empty1
 command user_def_cmd0 Empty2 -> Empty2
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 enum QEnumTwo ['value1', 'value2']
     prefix QENUM_TWO
 object UserDefOne
@@ -143,31 +143,31 @@ object UserDefNativeListUnion
     case sizes: q_obj_sizeList-wrapper
     case any: q_obj_anyList-wrapper
 command user_def_cmd None -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_user_def_cmd1-arg
     member ud1a: UserDefOne optional=False
 command user_def_cmd1 q_obj_user_def_cmd1-arg -> None
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_user_def_cmd2-arg
     member ud1a: UserDefOne optional=False
     member ud1b: UserDefOne optional=True
 command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True
 command guest-get-time q_obj_guest-get-time-arg -> int
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 object q_obj_guest-sync-arg
     member arg: any optional=False
 command guest-sync q_obj_guest-sync-arg -> any
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
 command boxed-struct UserDefZero -> None
-   gen=True success_response=True boxed=True oob=False
+   gen=True success_response=True boxed=True oob=False preconfig=False
 command boxed-union UserDefNativeListUnion -> None
-   gen=True success_response=True boxed=True oob=False
-command an-oob-command None -> None
-   gen=True success_response=True boxed=False oob=True
+   gen=True success_response=True boxed=True oob=False preconfig=False
+command test-flags-command None -> None
+   gen=True success_response=True boxed=False oob=True preconfig=True
 object UserDefOptions
     member i64: intList optional=True
     member u64: uint64List optional=True
@@ -231,4 +231,4 @@ object q_obj___org.qemu_x-command-arg
     member c: __org.qemu_x-Union2 optional=False
     member d: __org.qemu_x-Alt optional=False
 command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1
-   gen=True success_response=True boxed=False oob=False
+   gen=True success_response=True boxed=False oob=False preconfig=False
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 89b92ed..4512a41 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -45,8 +45,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
                       success_response, boxed, allow_oob, allow_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 oob=%s' % \
-              (gen, success_response, boxed, allow_oob))
+        print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s' % \
+              (gen, success_response, boxed, allow_oob, allow_preconfig))
 
     def visit_event(self, name, info, arg_type, boxed):
         print('event %s %s' % (name, arg_type and arg_type.name))
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index e0ed461..491b0c4 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -16,7 +16,7 @@ void qmp_user_def_cmd(Error **errp)
 {
 }
 
-void qmp_an_oob_command(Error **errp)
+void qmp_test_flags_command(Error **errp)
 {
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v8 05/11] qapi: introduce new cmd option "allow-preconfig"
  2018-05-11 16:51   ` [Qemu-devel] [PATCH v8 05/11] qapi: introduce new cmd option "allow-preconfig" Igor Mammedov
@ 2018-05-11 17:16     ` Eric Blake
  0 siblings, 0 replies; 48+ messages in thread
From: Eric Blake @ 2018-05-11 17:16 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel; +Cc: ehabkost, armbru

On 05/11/2018 11:51 AM, Igor Mammedov wrote:
> New option will be used to allow commands, which are prepared/need
> to run, during preconfig state. Other commands that should be able
> to run in preconfig state, should be amended to not expect machine
> in initialized state or deal with it.
> 
> For compatibility reasons, commands that don't use new flag
> 'allow-preconfig' explicitly are not permitted 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-command-line-options
>     query-status
>     exit-preconfig
> 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 patches.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v8:
>    - (Eric Blake <eblake@redhat.com>)
>      * replace allowed-in-preconfig on allow-preconfig in commit msg
>      * more wording fixes
>      * make example in doc match QMP schema

Reviewed-by: Eric Blake <eblake@redhat.com>

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v8 07/11] cli: add --preconfig option
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 07/11] cli: add --preconfig option Igor Mammedov
  2018-05-11 15:36   ` Eric Blake
@ 2018-05-11 17:24   ` Igor Mammedov
  1 sibling, 0 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-11 17:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: ehabkost, armbru, eblake

This option allows pausing QEMU in the new RUN_STATE_PRECONFIG state,
allowing the configuration of QEMU from QMP before the machine jumps
into board initialization code of machine_run_board_init()

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

The new option complements -S option and could be used with or without
it. The difference is that -S pauses QEMU when the machine is completely
initialized with all devices wired up and ready to execute guest code
(QEMU needs only to unpause VCPUs to let guest execute its code),
while the "preconfig" option pauses QEMU early before board specific init
callback (machine_run_board_init) is executed and allows the configuration
of machine parameters which will be used by board init code.

When early introspection/configuration is done, command 'exit-preconfig'
should be used to exit RUN_STATE_PRECONFIG and transition to the next
requested state (i.e. if -S is used then QEMU will pause the second
time when board/device initialization is completed or start guest
execution if -S isn't provided on CLI)

PS:
Initially 'preconfig' is planned to be used for configuring numa
topology depending on board specified possible cpus layout.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
v8:
  - doc gramar fixes (Eric Blake <eblake@redhat.com>)
v7:
  - (Eric Blake <eblake@redhat.com>)
    * spelling/wording fixes in newelly added documentation
    * s/allowed-in-preconfig/allow-preconfig/
    * s/QCO_ALLOWED_IN_PRECONFIG/QCO_ALLOW_PRECONFIG/
v6:
  - (Eduardo Habkost <ehabkost@redhat.com>)
    * add exit-preconfig QMP command instead of overloading meaning of 'cont' command
    * add doc text to qemu-tech.texi about -S and --preconfig
v5:
  - (Eric Blake <eblake@redhat.com>)
    * more spelling/wording fixes
    * s/-preconfig/--preconfig/
    * s/2.12/2.13/
  - (Eduardo Habkost <ehabkost@redhat.com>)
    * move QCO_ALLOWED_IN_PRECONFIG and do_qmp_dispatch() runstate check
      from the later patch 'qapi: introduce new cmd option  "allowed-in-preconfig"'
      to here for better bissectability
    * add TODO comment to '{ RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE }' transition
    * check for incoming && preconfig outside of option parsing loop
v4:
  * Explain more on behaviour in commit message and use suggested
    wording in message and patch (Eric Blake <eblake@redhat.com>)

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

preconfig
---
 include/sysemu/sysemu.h |  1 +
 qapi/misc.json          | 23 +++++++++++++++++++++++
 qapi/qmp-dispatch.c     |  8 ++++++++
 qemu-options.hx         | 13 +++++++++++++
 qemu-tech.texi          | 40 ++++++++++++++++++++++++++++++++++++++++
 qmp.c                   | 10 ++++++++++
 vl.c                    | 35 ++++++++++++++++++++++++++++++++++-
 7 files changed, 129 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 544ab77..e893f72 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/misc.json b/qapi/misc.json
index a16a7ef..502d5cf 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1245,6 +1245,29 @@
 { 'command': 'cont' }
 
 ##
+# @exit-preconfig:
+#
+# Exit from "preconfig" state
+#
+# This command makes QEMU exit the preconfig state and proceed with
+# VM initialization using configuration data provided on the command line
+# and via the QMP monitor during the preconfig state. The command is only
+# available during the preconfig state (i.e. when the --preconfig command
+# line option was in use).
+#
+# Since 2.13
+#
+# Returns: nothing
+#
+# Example:
+#
+# -> { "execute": "exit-preconfig" }
+# <- { "return": {} }
+#
+##
+{ 'command': 'exit-preconfig', 'allow-preconfig': true }
+
+##
 # @system_wakeup:
 #
 # Wakeup guest from suspend.  Does nothing in case the guest isn't suspended.
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index f9377b2..935f9e1 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -18,6 +18,7 @@
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qjson.h"
 #include "qapi/qmp/qbool.h"
+#include "sysemu/sysemu.h"
 
 QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
 {
@@ -101,6 +102,13 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
         return NULL;
     }
 
+    if (runstate_check(RUN_STATE_PRECONFIG) &&
+        !(cmd->options & QCO_ALLOW_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/qemu-options.hx b/qemu-options.hx
index c611766..6ba2a4f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3304,6 +3304,19 @@ STEXI
 Run the emulation in single step mode.
 ETEXI
 
+DEF("preconfig", 0, QEMU_OPTION_preconfig, \
+    "--preconfig     pause QEMU before machine is initialized\n",
+    QEMU_ARCH_ALL)
+STEXI
+@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 'exit-preconfig' to exit
+the preconfig state and move to the next state (ie. run guest if -S
+isn't used or pause the second time if -S is used).
+ETEXI
+
 DEF("S", 0, QEMU_OPTION_S, \
     "-S              freeze CPU at startup (use 'c' to start execution)\n",
     QEMU_ARCH_ALL)
diff --git a/qemu-tech.texi b/qemu-tech.texi
index 52a56ae..dcecba8 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,45 @@ 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 a VM in a paused state using
+the -S command line option. In this state the machine is completely initialized
+according to command line options and ready to execute VM code but VCPU threads
+are not executing any code. The VM state in this paused state depends on the way
+QEMU was started. It could be in:
+@table @asis
+@item initial state (after reset/power on state)
+@item with direct kernel loading, the initial state could be amended to execute
+code loaded by QEMU in the VM's RAM and with incoming migration
+@item with incoming migration, initial state will by amended with 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 the machine (by hotplugging devices) in runtime before
+allowing VM code to run.
+
+However, at the -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 the --preconfig command line option should be used. 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. 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
+@item exit-preconfig
+@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
+'allow-preconfig' set to true.
+
 @node Bibliography
 @section Bibliography
 
diff --git a/qmp.c b/qmp.c
index 25fdc9a..73e46d7 100644
--- a/qmp.c
+++ b/qmp.c
@@ -161,6 +161,16 @@ SpiceInfo *qmp_query_spice(Error **errp)
 };
 #endif
 
+void qmp_exit_preconfig(Error **errp)
+{
+    if (!runstate_check(RUN_STATE_PRECONFIG)) {
+        error_setg(errp, "The command is permitted only in '%s' state",
+                   RunState_str(RUN_STATE_PRECONFIG));
+        return;
+    }
+    qemu_exit_preconfig_request();
+}
+
 void qmp_cont(Error **errp)
 {
     BlockBackend *blk;
diff --git a/vl.c b/vl.c
index 12e31d1..3480b30 100644
--- a/vl.c
+++ b/vl.c
@@ -592,7 +592,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;
@@ -605,6 +605,13 @@ typedef struct {
 
 static const RunStateTransition runstate_transitions_def[] = {
     /*     from      ->     to      */
+    { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
+      /* 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 },
+
     { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
     { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
     { RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
@@ -1628,6 +1635,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);
@@ -1712,6 +1720,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.
  */
@@ -1885,6 +1898,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);
     }
@@ -3674,6 +3694,9 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
                 }
                 break;
+            case QEMU_OPTION_preconfig:
+                preconfig_exit_requested = false;
+                break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
                 qemu_opts_parse_noisily(olist, "accel=kvm", false);
@@ -4043,6 +4066,12 @@ int main(int argc, char **argv, char **envp)
 
     replay_configure(icount_opts);
 
+    if (incoming && !preconfig_exit_requested) {
+        error_report("'preconfig' and 'incoming' options are "
+                     "mutually exclusive");
+        exit(EXIT_FAILURE);
+    }
+
     machine_class = select_machine();
 
     set_memory_options(&ram_slots, &maxram_size, machine_class);
@@ -4560,6 +4589,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

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 01/11] numa: postpone options post-processing till machine_run_board_init()
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 01/11] numa: postpone options post-processing till machine_run_board_init() Igor Mammedov
@ 2018-05-16 18:12   ` Eduardo Habkost
  0 siblings, 0 replies; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 18:12 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, armbru

On Fri, May 04, 2018 at 10:37:39AM +0200, 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>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Queued, thanks.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 02/11] numa: split out NumaOptions parsing into set_numa_options()
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 02/11] numa: split out NumaOptions parsing into set_numa_options() Igor Mammedov
@ 2018-05-16 18:12   ` Eduardo Habkost
  0 siblings, 0 replies; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 18:12 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, armbru

On Fri, May 04, 2018 at 10:37:40AM +0200, Igor Mammedov wrote:
> it will allow to reuse set_numa_options() for parsing
> configuration commands received via QMP interface
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Queued, thanks.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 03/11] qapi: introduce preconfig runstate
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 03/11] qapi: introduce preconfig runstate Igor Mammedov
  2018-05-11 15:17   ` Eric Blake
@ 2018-05-16 18:14   ` Eduardo Habkost
  1 sibling, 0 replies; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 18:14 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, armbru

On Fri, May 04, 2018 at 10:37:41AM +0200, Igor Mammedov wrote:
> New preconfig runstate will be used in follow up patches
> related to introducing --preconfig CLI option and is
> intended to replace prelaunch runstate from QEMU start
> up to machine_init callback.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Queued, thanks.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 06/11] tests: qapi-schema tests for allow-preconfig
  2018-05-11 15:29   ` Eric Blake
@ 2018-05-16 18:16     ` Eduardo Habkost
  0 siblings, 0 replies; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 18:16 UTC (permalink / raw)
  To: Eric Blake; +Cc: Igor Mammedov, qemu-devel, pkrempa, armbru

On Fri, May 11, 2018 at 10:29:00AM -0500, Eric Blake wrote:
> On 05/04/2018 03:37 AM, Igor Mammedov wrote:
> > use new allow-preconfig parameter in tests and make sure that
> > the QAPISchema can parse allow-preconfig correctly
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > v7:
> 
> Missing --- separator to stop 'git am' from including the changelog.

Fixed while applying.

> 
> >    * s/allowed_in_preconfig/allow_preconfig/
> >    * squash in "tests: add allow-preconfig-test for qapi-schema"
> >    * reuse an-oob-command for allow-preconfig test and rename it to test-flags-command
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Queued, thanks.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 08/11] tests: extend qmp test with preconfig checks
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 08/11] tests: extend qmp test with preconfig checks Igor Mammedov
@ 2018-05-16 18:20   ` Eduardo Habkost
  2018-05-16 22:21   ` Eric Blake
  1 sibling, 0 replies; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 18:20 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, armbru

On Fri, May 04, 2018 at 10:37:46AM +0200, Igor Mammedov wrote:
> Add permission checks for commands at 'preconfig' stage.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

Queued, thanks.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 09/11] qmp: permit query-hotpluggable-cpus in preconfig state
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 09/11] qmp: permit query-hotpluggable-cpus in preconfig state Igor Mammedov
@ 2018-05-16 18:24   ` Eduardo Habkost
  0 siblings, 0 replies; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 18:24 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, armbru

On Fri, May 04, 2018 at 10:37:47AM +0200, Igor Mammedov wrote:
> 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>

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

I suggest a follow-up patch adding a test for
query-hotpluggable-cpus on test_qmp_preconfig().

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 10/11] qmp: add set-numa-node command
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 10/11] qmp: add set-numa-node command Igor Mammedov
@ 2018-05-16 18:26   ` Eduardo Habkost
  0 siblings, 0 replies; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 18:26 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, armbru

On Fri, May 04, 2018 at 10:37:48AM +0200, Igor Mammedov wrote:
> 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.
> 
> Example of configuration session:
> $QEMU -smp 2 --preconfig ...
> 
> QMP:
> -> {'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}, ... }
>    ]}
> 
> -> {'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': {}}
> 
> -> {'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': {}}
> 
> -> {'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}, ... }
>    ]}
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

I suggest sending a test case as a follow-up patch.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node Igor Mammedov
@ 2018-05-16 18:27   ` Eduardo Habkost
  2018-05-16 22:12   ` Eduardo Habkost
  2018-05-17 11:30   ` [Qemu-devel] [PATCH v8 " Igor Mammedov
  2 siblings, 0 replies; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 18:27 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, armbru

On Fri, May 04, 2018 at 10:37:49AM +0200, Igor Mammedov wrote:
>  * 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>

Nice, this is the test case I asked for in previous patches.  :)

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

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (10 preceding siblings ...)
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node Igor Mammedov
@ 2018-05-16 19:02 ` Eduardo Habkost
  2018-05-24 15:42   ` Markus Armbruster
  2018-05-30 10:47 ` Igor Mammedov
  12 siblings, 1 reply; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 19:02 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, armbru, Daniel P. Berrange


I'm queueing this version (including v8 of patches 5-7) on
numa-next.

Markus, Daniel: you were the people I remember expressing some
concerns about the new preconfig mechanism.  Do you have any
objections to this version?


On Fri, May 04, 2018 at 10:37:38AM +0200, Igor Mammedov wrote:
> 
> 
> v6->v7:
>   * fix up wording in documentation and left-overs of 'reusing' 'cont' command in v5
>   * split out preconfig runstate introduction into separate patch to make
>     series more bisection friendly and put allow-preconfig patch before
>     --preconfig CLI patch
>   * merge qapi schema tests patches into one
>   * rename allowed_in_preconfig flag to a shorter allow_preconfig
>   * rename QCO_ALLOWED_IN_PRECONFIG into shorter QCO_ALLOW_PRECONFIG to match
>     allow_preconfig flag
>   * reuse an-oob-command for allow_preconfig flag testing and rename it to
>     test-flags-command
> v5->v6:
>   * add exit-preconfig QMP command instead of overloading meaning of 'cont' command
>   * add doc text to qemu-tech.texi about -S and --preconfig
>   * add numa configuration example into commit message of 10/11
>   * limit set-numa-node QMP command to preconfig mode
> v4->v5:
>   * rebase on top of OOB changes that's now in master
>   * s/qobject_to_qdict(/qobject_to(QDict,/
>   * s/-preconfig/--preconfig/
>   * s/2.12/2.13/
>   * s/parse_NumaOptions/set_numa_options/
>   * drop if (err) guard around error_propagate()
>   * move QCO_ALLOWED_IN_PRECONFIG and do_qmp_dispatch() runstate check
>     from the later patch 'qapi: introduce new cmd option  "allowed-in-preconfig"'
>     to here for better bissectability
>   * add TODO comment to '{ RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE }' transition
>   * check for incoming && preconfig outside of option parsing loop
>   * add 'use QMP instead" to error message, suggesting user the right interface to use
>   * allow query-command-line-options in preconfig state
>   * make sure that allowed-in-preconfig could be set only to True
>   * move out QCO_ALLOWED_IN_PRECONFIG check in do_qmp_dispatch() to
>     earlier 'cli: add --preconfig option' patch
>   * 2 extra test patches
>      'tests: let qapi-schema tests detect allowed-in-preconfig'
>      'tests: add allowed-in-preconfig-test for qapi-schema'
> 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.
> 
> 
> Currently it's problematic to configure NUMA mapping for CPUs using "-numa cpu="
> option, without restarting QEMU, first is to query board specific CPU layout
> with query-hotpluggable-cpus QMP command (which is function of -M and -smp CLI
> options). Mgmt side isn't happy with restarting QEMU to query configuration
> parameters first and then run actual instance, so it keeps using old cpu_index
> based option '-numa cpus' instead of new socket/core/thread-id based one.
> 
> Introduce a new '--preconfig' CLI option which allows pausing QEMU before
> machine_init() is run (preconfig state). So it would be possible to query
> possible CPUs layout, configure NUMA mapping based on query result with
> set-numa-node QMP command and continue executing  without restarting QEMU.
> 
> Difference between new --preconfig pause point vs -S is that the later pauses
> QEMU after machine is constructed and ready to run guest code or in process of
> incoming migration (essentially machine is in some running state (with paused
> VCPUs) and any action on it is considered as hotplug). At this point it's hard
> to configure or reconfigure parameters that affect machine_init() and later
> stages. While the new --preconfig option pauses QEMU instance before
> machine_init() and would allow to configure parameters as if doing it from CLI
> but in interactve manner using QMP interface, which would allow introspecting
> and configuring QEMU instance without restarting it. 
> 
> Initially only limited set of commands (that are ready to work with non
> initialized machine or without it) are allowed in preconfig state:
> 
>    #existing commands:
>        qmp_capabilities
>        query-qmp-schema
>        query-commands
>        query-command-line-options
>        query-status
>        query-hotpluggable-cpus
>    #new commands
>        exit-preconfig
>        set-numa-node
> 
> Which commands are allowed at preconfig state, is defined by QAPI Schema
> with help of optional 'allowed-in-preconfig' flag in command definition,
> which allows users to get list of commands it can run with help of
> query-qmp-schema command.
> 
> Once user done with configuration at preconfig state, it could be exited with
> exit-preconfig command, in which case QEMU would proceed with VM intialization
> and either start guest execution or pause in prelaunch state if -S CLI option
> were specified along with --prelaunch on startup.
> 
> PS:
> Later we can modify other commands to run early, for example device_add. So
> that devices would be added in cold-plug context, instead of getting device
> 'hotplugged' with followed up fixups and workarounds to make it look like
> cold-plugged.
> 
> Example of configuration CPU's NUMA mapping with --preconfig:
> $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_v7
> 
>  
> CC: eblake@redhat.com
> CC: ehabkost@redhat.com
> CC: pkrempa@redhat.com
> CC: armbru@redhat.com
> 
> 
> Igor Mammedov (11):
>   numa: postpone options post-processing till machine_run_board_init()
>   numa: split out NumaOptions parsing into set_numa_options()
>   qapi: introduce preconfig runstate
>   hmp: disable monitor in preconfig state
>   qapi: introduce new cmd option "allowed-in-preconfig"
>   tests: qapi-schema tests for allow-preconfig
>   cli: add --preconfig option
>   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                 |  1 +
>  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                                   | 11 +++--
>  numa.c                                      | 70 +++++++++++++++++++----------
>  qapi/introspect.json                        |  5 ++-
>  qapi/misc.json                              | 49 ++++++++++++++++++--
>  qapi/qmp-dispatch.c                         |  8 ++++
>  qapi/run-state.json                         |  8 +++-
>  qemu-options.hx                             | 13 ++++++
>  qemu-tech.texi                              | 40 +++++++++++++++++
>  qmp.c                                       | 10 +++++
>  scripts/qapi/commands.py                    | 11 +++--
>  scripts/qapi/common.py                      | 18 +++++---
>  scripts/qapi/doc.py                         |  4 +-
>  scripts/qapi/introspect.py                  |  7 +--
>  tests/Makefile.include                      |  1 +
>  tests/libqtest.c                            |  7 +++
>  tests/numa-test.c                           | 61 +++++++++++++++++++++++++
>  tests/qapi-schema/allow-preconfig-test.err  |  1 +
>  tests/qapi-schema/allow-preconfig-test.exit |  1 +
>  tests/qapi-schema/allow-preconfig-test.json |  2 +
>  tests/qapi-schema/allow-preconfig-test.out  |  0
>  tests/qapi-schema/doc-good.out              |  4 +-
>  tests/qapi-schema/ident-with-escape.out     |  2 +-
>  tests/qapi-schema/indented-expr.out         |  4 +-
>  tests/qapi-schema/qapi-schema-test.json     |  4 +-
>  tests/qapi-schema/qapi-schema-test.out      | 22 ++++-----
>  tests/qapi-schema/test-qapi.py              |  8 ++--
>  tests/qmp-test.c                            | 37 +++++++++++++++
>  tests/test-qmp-cmds.c                       |  2 +-
>  vl.c                                        | 35 ++++++++++++++-
>  35 files changed, 396 insertions(+), 77 deletions(-)
>  create mode 100644 tests/qapi-schema/allow-preconfig-test.err
>  create mode 100644 tests/qapi-schema/allow-preconfig-test.exit
>  create mode 100644 tests/qapi-schema/allow-preconfig-test.json
>  create mode 100644 tests/qapi-schema/allow-preconfig-test.out
> 
> -- 
> 2.7.4
> 
> 

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node Igor Mammedov
  2018-05-16 18:27   ` Eduardo Habkost
@ 2018-05-16 22:12   ` Eduardo Habkost
  2018-05-17  8:01     ` Igor Mammedov
  2018-05-17 11:30   ` [Qemu-devel] [PATCH v8 " Igor Mammedov
  2 siblings, 1 reply; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-16 22:12 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, armbru

On Fri, May 04, 2018 at 10:37:49AM +0200, Igor Mammedov wrote:
>  * 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>
> ---
> v6:
>   * replace 'cont' with 'exit-preconfig' command
> v5:
>   * s/qobject_to_qdict(/qobject_to(QDict,/
>   * s/-preconfig/--preconfig/
> v4:
>   * drop duplicate is_err() and reuse qmp_rsp_is_err() wich is moved
>     to generic file libqtest.c. (Eric Blake <eblake@redhat.com>)
> 
> FIXUP! tests: functional tests for QMP command  set-numa-node
> ---
>  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 cbe8df4..ac52872 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -972,4 +972,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 6f33a37..33426d5 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -1098,3 +1098,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);


Oops:

  tests/libqtest.c: In function ‘qmp_rsp_is_err’:
  tests/libqtest.c:1105:5: error: implicit declaration of function ‘QDECREF’ [-Werror=implicit-function-declaration]
       QDECREF(rsp);
       ^
  tests/libqtest.c:1105:5: error: nested extern declaration of ‘QDECREF’ [-Werror=nested-externs]

I've fixed this on numa-next, replaced QDECREF with object_unref.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 08/11] tests: extend qmp test with preconfig checks
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 08/11] tests: extend qmp test with preconfig checks Igor Mammedov
  2018-05-16 18:20   ` Eduardo Habkost
@ 2018-05-16 22:21   ` Eric Blake
  2018-05-17 11:28     ` [Qemu-devel] [PATCH v8 " Igor Mammedov
  1 sibling, 1 reply; 48+ messages in thread
From: Eric Blake @ 2018-05-16 22:21 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel; +Cc: ehabkost, pkrempa, armbru

On 05/04/2018 03:37 AM, Igor Mammedov wrote:
> Add permission checks for commands at 'preconfig' stage.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

> +++ b/tests/qmp-test.c
> @@ -392,6 +392,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;

At first glance, I was worried that this was a use-after-free; but as 
you are not actually dereferencing error, but merely checking whether 
the pointer was non-NULL (which works even if the pointer has gone stale 
in the meantime due to the QDECREF), you are okay.

> +}
> +
> +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': 'exit-preconfig' }")));
> +    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' }")));

It would also be worth testing that a second exit-preconfig fails, now 
that you are no longer in preconfig state.

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node
  2018-05-16 22:12   ` Eduardo Habkost
@ 2018-05-17  8:01     ` Igor Mammedov
  2018-05-17 11:27       ` Igor Mammedov
  0 siblings, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-17  8:01 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: qemu-devel, pkrempa, armbru

On Wed, 16 May 2018 19:12:30 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, May 04, 2018 at 10:37:49AM +0200, Igor Mammedov wrote:
> >  * 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>
> > ---
> > v6:
> >   * replace 'cont' with 'exit-preconfig' command
> > v5:
> >   * s/qobject_to_qdict(/qobject_to(QDict,/
> >   * s/-preconfig/--preconfig/
> > v4:
> >   * drop duplicate is_err() and reuse qmp_rsp_is_err() wich is moved
> >     to generic file libqtest.c. (Eric Blake <eblake@redhat.com>)
> > 
> > FIXUP! tests: functional tests for QMP command  set-numa-node
> > ---
> >  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 cbe8df4..ac52872 100644
> > --- a/tests/libqtest.h
> > +++ b/tests/libqtest.h
> > @@ -972,4 +972,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 6f33a37..33426d5 100644
> > --- a/tests/libqtest.c
> > +++ b/tests/libqtest.c
> > @@ -1098,3 +1098,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);  
> 
> 
> Oops:
> 
>   tests/libqtest.c: In function ‘qmp_rsp_is_err’:
>   tests/libqtest.c:1105:5: error: implicit declaration of function ‘QDECREF’ [-Werror=implicit-function-declaration]
>        QDECREF(rsp);
>        ^
>   tests/libqtest.c:1105:5: error: nested extern declaration of ‘QDECREF’ [-Werror=nested-externs]
> 
> I've fixed this on numa-next, replaced QDECREF with object_unref.
I guess QDECREF was removed while patch were sitting on the list.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node
  2018-05-17  8:01     ` Igor Mammedov
@ 2018-05-17 11:27       ` Igor Mammedov
  0 siblings, 0 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-17 11:27 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: pkrempa, qemu-devel, armbru

On Thu, 17 May 2018 10:01:34 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Wed, 16 May 2018 19:12:30 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, May 04, 2018 at 10:37:49AM +0200, Igor Mammedov wrote:  
> > >  * 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>
> > > ---
> > > v6:
> > >   * replace 'cont' with 'exit-preconfig' command
> > > v5:
> > >   * s/qobject_to_qdict(/qobject_to(QDict,/
> > >   * s/-preconfig/--preconfig/
> > > v4:
> > >   * drop duplicate is_err() and reuse qmp_rsp_is_err() wich is moved
> > >     to generic file libqtest.c. (Eric Blake <eblake@redhat.com>)
> > > 
> > > FIXUP! tests: functional tests for QMP command  set-numa-node
> > > ---
> > >  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 cbe8df4..ac52872 100644
> > > --- a/tests/libqtest.h
> > > +++ b/tests/libqtest.h
> > > @@ -972,4 +972,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 6f33a37..33426d5 100644
> > > --- a/tests/libqtest.c
> > > +++ b/tests/libqtest.c
> > > @@ -1098,3 +1098,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);    
> > 
> > 
> > Oops:
> > 
> >   tests/libqtest.c: In function ‘qmp_rsp_is_err’:
> >   tests/libqtest.c:1105:5: error: implicit declaration of function ‘QDECREF’ [-Werror=implicit-function-declaration]
> >        QDECREF(rsp);
> >        ^
> >   tests/libqtest.c:1105:5: error: nested extern declaration of ‘QDECREF’ [-Werror=nested-externs]
> > 
> > I've fixed this on numa-next, replaced QDECREF with object_unref.  
> I guess QDECREF was removed while patch were sitting on the list.
change affected 8th and 11th patches make the last depended on 8th.
I'll post rebased v8 8,11th patches here so it would still bisectable
and include into 8th Eric's request for negative exit-preconfig testcase (+3LOC)

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v8 08/11] tests: extend qmp test with preconfig checks
  2018-05-16 22:21   ` Eric Blake
@ 2018-05-17 11:28     ` Igor Mammedov
  2018-05-17 13:43       ` Eric Blake
  0 siblings, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-17 11:28 UTC (permalink / raw)
  To: qemu-devel; +Cc: pkrempa, ehabkost, armbru, eblake

Add permission checks for commands at 'preconfig' stage.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v8:
  * there isn't QDECREF anymore use qobject_unref instead
  * add negative test for exit-preconfig
v6:
  * replace 'cont' with 'exit-preconfig' command
v5:
  * s/-preconfig/--preconfig/
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 | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 88f867f..2ee441c 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -392,6 +392,52 @@ static void add_query_tests(QmpSchema *schema)
     }
 }
 
+static bool qmp_rsp_is_err(QDict *rsp)
+{
+    QDict *error = qdict_get_qdict(rsp, "error");
+    qobject_unref(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");
+    qobject_unref(rsp);
+
+    /* exit preconfig state */
+    g_assert(!qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'exit-preconfig' }")));
+    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");
+    qobject_unref(rsp);
+
+    /* check that exit-preconfig returns error after exiting preconfig */
+    g_assert(qmp_rsp_is_err(qtest_qmp(qs, "{ 'execute': 'exit-preconfig' }")));
+
+    /* 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;
@@ -403,6 +449,7 @@ int main(int argc, char *argv[])
     qtest_add_func("qmp/oob", test_qmp_oob);
     qmp_schema_init(&schema);
     add_query_tests(&schema);
+    qtest_add_func("qmp/preconfig", test_qmp_preconfig);
 
     ret = g_test_run();
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [Qemu-devel] [PATCH v8 11/11] tests: functional tests for QMP command set-numa-node
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node Igor Mammedov
  2018-05-16 18:27   ` Eduardo Habkost
  2018-05-16 22:12   ` Eduardo Habkost
@ 2018-05-17 11:30   ` Igor Mammedov
  2 siblings, 0 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-17 11:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: pkrempa, ehabkost, armbru, eblake

 * 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>
---
v8:
  * there isn't QDECREF/qobject_decref anymore use qobject_unref instead
---
 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 cbe8df4..ac52872 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -972,4 +972,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 43fb97e..e0ca19d 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -1098,3 +1098,10 @@ void qtest_qmp_device_del(const char *id)
     qobject_unref(response1);
     qobject_unref(response2);
 }
+
+bool qmp_rsp_is_err(QDict *rsp)
+{
+    QDict *error = qdict_get_qdict(rsp, "error");
+    qobject_unref(rsp);
+    return !!error;
+}
diff --git a/tests/numa-test.c b/tests/numa-test.c
index 169213f..b7a6ef8 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': 'exit-preconfig' }")));
+    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_unref(e);
+    }
+    qobject_unref(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 2ee441c..a49cbc6 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -392,13 +392,6 @@ static void add_query_tests(QmpSchema *schema)
     }
 }
 
-static bool qmp_rsp_is_err(QDict *rsp)
-{
-    QDict *error = qdict_get_qdict(rsp, "error");
-    qobject_unref(rsp);
-    return !!error;
-}
-
 static void test_qmp_preconfig(void)
 {
     QDict *rsp, *ret;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v8 08/11] tests: extend qmp test with preconfig checks
  2018-05-17 11:28     ` [Qemu-devel] [PATCH v8 " Igor Mammedov
@ 2018-05-17 13:43       ` Eric Blake
  0 siblings, 0 replies; 48+ messages in thread
From: Eric Blake @ 2018-05-17 13:43 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel; +Cc: pkrempa, ehabkost, armbru

On 05/17/2018 06:28 AM, Igor Mammedov wrote:
> Add permission checks for commands at 'preconfig' stage.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v8:
>    * there isn't QDECREF anymore use qobject_unref instead
>    * add negative test for exit-preconfig

Reviewed-by: Eric Blake <eblake@redhat.com>

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP
  2018-05-16 19:02 ` [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Eduardo Habkost
@ 2018-05-24 15:42   ` Markus Armbruster
  2018-05-28  9:40     ` Igor Mammedov
  0 siblings, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2018-05-24 15:42 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Igor Mammedov, pkrempa, qemu-devel

Eduardo Habkost <ehabkost@redhat.com> writes:

> I'm queueing this version (including v8 of patches 5-7) on
> numa-next.
>
> Markus, Daniel: you were the people I remember expressing some
> concerns about the new preconfig mechanism.  Do you have any
> objections to this version?

My gut's reaction to preconfig is as nauseous as ever.  It adds
complexity we wouldn't want or need with sanely structured program
startup.  It might still be the best we can do, and something we need to
do.  You have to take on technical debt sometimes.  To know for sure
this is the case here, we'd have to explore alternatives more seriously.
Like repaying some of the existing technical debt around there.  Sadly,
I lack the time to take on such a project.

Since I lack the money to put where my mouth is, I'm going to shut up
and get out of your way.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state
  2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state Igor Mammedov
@ 2018-05-24 16:00   ` Markus Armbruster
  2018-05-24 18:16     ` Markus Armbruster
  0 siblings, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2018-05-24 16:00 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, ehabkost

Igor Mammedov <imammedo@redhat.com> writes:

> 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> v5:
>   * add 'use QMP instead" to error message, suggesting user
>     the right interface to use
> 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index 39f8ee1..0ffdf1d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3374,6 +3374,12 @@ 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, "
> +                            "use QMP instead\n");
> +        return;
> +    }
> +
>      cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
>      if (!cmd) {
>          return;

So we offer the user an HMP monitor, but we summarily fail all commands.
I'm sorry, but that's... searching for polite word... embarrassing.  We
should accept HMP output only when we're ready to accept it.  Yes, that
would involve a bit more surgery rather than this cheap hack.  The whole
preconfig thing smells like a cheap hack to me, but let's not overdo it.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state
  2018-05-24 16:00   ` Markus Armbruster
@ 2018-05-24 18:16     ` Markus Armbruster
  2018-05-24 18:26       ` Eduardo Habkost
  0 siblings, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2018-05-24 18:16 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel, pkrempa, ehabkost

Markus Armbruster <armbru@redhat.com> writes:

> Igor Mammedov <imammedo@redhat.com> writes:
>
>> 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>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>> v5:
>>   * add 'use QMP instead" to error message, suggesting user
>>     the right interface to use
>> 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 | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 39f8ee1..0ffdf1d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -3374,6 +3374,12 @@ 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, "
>> +                            "use QMP instead\n");
>> +        return;
>> +    }
>> +
>>      cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
>>      if (!cmd) {
>>          return;
>
> So we offer the user an HMP monitor, but we summarily fail all commands.
> I'm sorry, but that's... searching for polite word... embarrassing.  We
> should accept HMP output only when we're ready to accept it.  Yes, that
> would involve a bit more surgery rather than this cheap hack.  The whole
> preconfig thing smells like a cheap hack to me, but let's not overdo it.

Clarification: I don't think we need to hold the series because of
this.  I do think you should investigate delaying HMP until it can work.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state
  2018-05-24 18:16     ` Markus Armbruster
@ 2018-05-24 18:26       ` Eduardo Habkost
  2018-05-25  6:05         ` Markus Armbruster
  0 siblings, 1 reply; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-24 18:26 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Igor Mammedov, qemu-devel, pkrempa

On Thu, May 24, 2018 at 08:16:20PM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Igor Mammedov <imammedo@redhat.com> writes:
> >
> >> 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>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> ---
> >> v5:
> >>   * add 'use QMP instead" to error message, suggesting user
> >>     the right interface to use
> >> 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 | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 39f8ee1..0ffdf1d 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -3374,6 +3374,12 @@ 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, "
> >> +                            "use QMP instead\n");
> >> +        return;
> >> +    }
> >> +
> >>      cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
> >>      if (!cmd) {
> >>          return;
> >
> > So we offer the user an HMP monitor, but we summarily fail all commands.
> > I'm sorry, but that's... searching for polite word... embarrassing.  We
> > should accept HMP output only when we're ready to accept it.  Yes, that
> > would involve a bit more surgery rather than this cheap hack.  The whole
> > preconfig thing smells like a cheap hack to me, but let's not overdo it.
> 
> Clarification: I don't think we need to hold the series because of
> this.  I do think you should investigate delaying HMP until it can work.

What would it mean to delay HMP?  Not creating the socket?
Creating the socket but not accepting clients?  Accepting clients
but not consuming any input from the socket until we are out of
preconfig?

I'm not sure if any of those options would be better.  If a human
is already trying to talk on the other side, it seems better to
show QEMU is alive (but not ready to hold a conversation yet)
than staying silent.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state
  2018-05-24 18:26       ` Eduardo Habkost
@ 2018-05-25  6:05         ` Markus Armbruster
  2018-05-25 19:39           ` Eduardo Habkost
  0 siblings, 1 reply; 48+ messages in thread
From: Markus Armbruster @ 2018-05-25  6:05 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Igor Mammedov, pkrempa, qemu-devel

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, May 24, 2018 at 08:16:20PM +0200, Markus Armbruster wrote:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Igor Mammedov <imammedo@redhat.com> writes:
>> >
>> >> 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>
>> >> Reviewed-by: Eric Blake <eblake@redhat.com>
>> >> ---
>> >> v5:
>> >>   * add 'use QMP instead" to error message, suggesting user
>> >>     the right interface to use
>> >> 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 | 6 ++++++
>> >>  1 file changed, 6 insertions(+)
>> >>
>> >> diff --git a/monitor.c b/monitor.c
>> >> index 39f8ee1..0ffdf1d 100644
>> >> --- a/monitor.c
>> >> +++ b/monitor.c
>> >> @@ -3374,6 +3374,12 @@ 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, "
>> >> +                            "use QMP instead\n");
>> >> +        return;
>> >> +    }
>> >> +
>> >>      cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
>> >>      if (!cmd) {
>> >>          return;
>> >
>> > So we offer the user an HMP monitor, but we summarily fail all commands.
>> > I'm sorry, but that's... searching for polite word... embarrassing.  We
>> > should accept HMP output only when we're ready to accept it.  Yes, that
>> > would involve a bit more surgery rather than this cheap hack.  The whole
>> > preconfig thing smells like a cheap hack to me, but let's not overdo it.
>> 
>> Clarification: I don't think we need to hold the series because of
>> this.  I do think you should investigate delaying HMP until it can work.
>
> What would it mean to delay HMP?  Not creating the socket?
> Creating the socket but not accepting clients?  Accepting clients
> but not consuming any input from the socket until we are out of
> preconfig?
>
> I'm not sure if any of those options would be better.  If a human
> is already trying to talk on the other side, it seems better to
> show QEMU is alive (but not ready to hold a conversation yet)
> than staying silent.

If this

    QEMU 2.12.50 monitor - type 'help' for more information
    (qemu) help
    HMP not available in preconfig state, use QMP instead
    (qemu) quit
    HMP not available in preconfig state, use QMP instead
    (qemu) let me out dammit
    HMP not available in preconfig state, use QMP instead
    (qemu) 

is better than the alternatives, then I wonder how much more
entertainment the alternatives could provide!

We *can* do better.  Start like this:

    QEMU 2.12.50 monitor is not ready with -preconfig until you complete
    configuration with QMP

and when we exit preconfig state, add:

    QEMU 2.12.50 monitor - type 'help' for more information
    (qemu) 

Note that this is upfront about the monitor not being ready, avoids
misleading the user about "help", talks to the user in the user's terms
(-preconfig) instead of internal terms (preconfig state), and is more
specific on how to ready the monitor.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state
  2018-05-25  6:05         ` Markus Armbruster
@ 2018-05-25 19:39           ` Eduardo Habkost
  2018-05-28  9:31             ` Igor Mammedov
  0 siblings, 1 reply; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-25 19:39 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Igor Mammedov, pkrempa, qemu-devel

On Fri, May 25, 2018 at 08:05:30AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, May 24, 2018 at 08:16:20PM +0200, Markus Armbruster wrote:
> >> Markus Armbruster <armbru@redhat.com> writes:
> >> 
> >> > Igor Mammedov <imammedo@redhat.com> writes:
> >> >
> >> >> 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>
> >> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> >> ---
> >> >> v5:
> >> >>   * add 'use QMP instead" to error message, suggesting user
> >> >>     the right interface to use
> >> >> 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 | 6 ++++++
> >> >>  1 file changed, 6 insertions(+)
> >> >>
> >> >> diff --git a/monitor.c b/monitor.c
> >> >> index 39f8ee1..0ffdf1d 100644
> >> >> --- a/monitor.c
> >> >> +++ b/monitor.c
> >> >> @@ -3374,6 +3374,12 @@ 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, "
> >> >> +                            "use QMP instead\n");
> >> >> +        return;
> >> >> +    }
> >> >> +
> >> >>      cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
> >> >>      if (!cmd) {
> >> >>          return;
> >> >
> >> > So we offer the user an HMP monitor, but we summarily fail all commands.
> >> > I'm sorry, but that's... searching for polite word... embarrassing.  We
> >> > should accept HMP output only when we're ready to accept it.  Yes, that
> >> > would involve a bit more surgery rather than this cheap hack.  The whole
> >> > preconfig thing smells like a cheap hack to me, but let's not overdo it.
> >> 
> >> Clarification: I don't think we need to hold the series because of
> >> this.  I do think you should investigate delaying HMP until it can work.
> >
> > What would it mean to delay HMP?  Not creating the socket?
> > Creating the socket but not accepting clients?  Accepting clients
> > but not consuming any input from the socket until we are out of
> > preconfig?
> >
> > I'm not sure if any of those options would be better.  If a human
> > is already trying to talk on the other side, it seems better to
> > show QEMU is alive (but not ready to hold a conversation yet)
> > than staying silent.
> 
> If this
> 
>     QEMU 2.12.50 monitor - type 'help' for more information
>     (qemu) help
>     HMP not available in preconfig state, use QMP instead
>     (qemu) quit
>     HMP not available in preconfig state, use QMP instead
>     (qemu) let me out dammit
>     HMP not available in preconfig state, use QMP instead
>     (qemu) 
> 
> is better than the alternatives, then I wonder how much more
> entertainment the alternatives could provide!
> 
> We *can* do better.  Start like this:
> 
>     QEMU 2.12.50 monitor is not ready with -preconfig until you complete
>     configuration with QMP
> 
> and when we exit preconfig state, add:
> 
>     QEMU 2.12.50 monitor - type 'help' for more information
>     (qemu) 
> 
> Note that this is upfront about the monitor not being ready, avoids
> misleading the user about "help", talks to the user in the user's terms
> (-preconfig) instead of internal terms (preconfig state), and is more
> specific on how to ready the monitor.

Yes, this sounds better than any of the options I have
considered.

Making at least 'help', 'quit', and 'exit-preconfig' work might
be even better, though.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state
  2018-05-25 19:39           ` Eduardo Habkost
@ 2018-05-28  9:31             ` Igor Mammedov
  2018-06-01  8:59               ` Dr. David Alan Gilbert
  0 siblings, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-28  9:31 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Markus Armbruster, pkrempa, qemu-devel

On Fri, 25 May 2018 16:39:34 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, May 25, 2018 at 08:05:30AM +0200, Markus Armbruster wrote:
> > Eduardo Habkost <ehabkost@redhat.com> writes:
> >   
> > > On Thu, May 24, 2018 at 08:16:20PM +0200, Markus Armbruster wrote:  
> > >> Markus Armbruster <armbru@redhat.com> writes:
> > >>   
> > >> > Igor Mammedov <imammedo@redhat.com> writes:
> > >> >  
> > >> >> 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>
> > >> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> > >> >> ---
> > >> >> v5:
> > >> >>   * add 'use QMP instead" to error message, suggesting user
> > >> >>     the right interface to use
> > >> >> 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 | 6 ++++++
> > >> >>  1 file changed, 6 insertions(+)
> > >> >>
> > >> >> diff --git a/monitor.c b/monitor.c
> > >> >> index 39f8ee1..0ffdf1d 100644
> > >> >> --- a/monitor.c
> > >> >> +++ b/monitor.c
> > >> >> @@ -3374,6 +3374,12 @@ 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, "
> > >> >> +                            "use QMP instead\n");
> > >> >> +        return;
> > >> >> +    }
> > >> >> +
> > >> >>      cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
> > >> >>      if (!cmd) {
> > >> >>          return;  
> > >> >
> > >> > So we offer the user an HMP monitor, but we summarily fail all commands.
> > >> > I'm sorry, but that's... searching for polite word... embarrassing.  We
> > >> > should accept HMP output only when we're ready to accept it.  Yes, that
> > >> > would involve a bit more surgery rather than this cheap hack.  The whole
> > >> > preconfig thing smells like a cheap hack to me, but let's not overdo it.  
> > >> 
> > >> Clarification: I don't think we need to hold the series because of
> > >> this.  I do think you should investigate delaying HMP until it can work.  
> > >
> > > What would it mean to delay HMP?  Not creating the socket?
> > > Creating the socket but not accepting clients?  Accepting clients
> > > but not consuming any input from the socket until we are out of
> > > preconfig?
> > >
> > > I'm not sure if any of those options would be better.  If a human
> > > is already trying to talk on the other side, it seems better to
> > > show QEMU is alive (but not ready to hold a conversation yet)
> > > than staying silent.  
> > 
> > If this
> > 
> >     QEMU 2.12.50 monitor - type 'help' for more information
> >     (qemu) help
> >     HMP not available in preconfig state, use QMP instead
> >     (qemu) quit
> >     HMP not available in preconfig state, use QMP instead
> >     (qemu) let me out dammit
> >     HMP not available in preconfig state, use QMP instead
> >     (qemu) 
> > 
> > is better than the alternatives, then I wonder how much more
> > entertainment the alternatives could provide!
> > 
> > We *can* do better.  Start like this:
> > 
> >     QEMU 2.12.50 monitor is not ready with -preconfig until you complete
> >     configuration with QMP
> > 
> > and when we exit preconfig state, add:
> > 
> >     QEMU 2.12.50 monitor - type 'help' for more information
> >     (qemu) 
> > 
> > Note that this is upfront about the monitor not being ready, avoids
> > misleading the user about "help", talks to the user in the user's terms
> > (-preconfig) instead of internal terms (preconfig state), and is more
> > specific on how to ready the monitor.  
> 
> Yes, this sounds better than any of the options I have
> considered.
> 
> Making at least 'help', 'quit', and 'exit-preconfig' work might
> be even better, though.
I'll look into both options and try to come up a patch to make it better.

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP
  2018-05-24 15:42   ` Markus Armbruster
@ 2018-05-28  9:40     ` Igor Mammedov
  0 siblings, 0 replies; 48+ messages in thread
From: Igor Mammedov @ 2018-05-28  9:40 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Eduardo Habkost, pkrempa, qemu-devel

On Thu, 24 May 2018 17:42:03 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > I'm queueing this version (including v8 of patches 5-7) on
> > numa-next.
> >
> > Markus, Daniel: you were the people I remember expressing some
> > concerns about the new preconfig mechanism.  Do you have any
> > objections to this version?  
> 
> My gut's reaction to preconfig is as nauseous as ever.  It adds
> complexity we wouldn't want or need with sanely structured program
> startup.  It might still be the best we can do, and something we need to
> do.  You have to take on technical debt sometimes.  To know for sure
> this is the case here, we'd have to explore alternatives more seriously.
> Like repaying some of the existing technical debt around there.  Sadly,
> I lack the time to take on such a project.
Usually I do quite a bit of re-factoring to reduce our technical debt,
so while working on enabling VM start with libvirt using only --preconfig
(without -S) I most likely will have to improve code around VM creation.

[...]

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP
  2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
                   ` (11 preceding siblings ...)
  2018-05-16 19:02 ` [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Eduardo Habkost
@ 2018-05-30 10:47 ` Igor Mammedov
  2018-05-30 16:22   ` Eduardo Habkost
  12 siblings, 1 reply; 48+ messages in thread
From: Igor Mammedov @ 2018-05-30 10:47 UTC (permalink / raw)
  To: qemu-devel; +Cc: ehabkost

Eduardo,

I've rebased series on top of current master
the only change in several patches was s/2.13/3.0/
otherwise there weren't any other conflicts.
You can find rebased version at

https://github.com/imammedo/qemu.git qmp_preconfig_v9

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP
  2018-05-30 10:47 ` Igor Mammedov
@ 2018-05-30 16:22   ` Eduardo Habkost
  0 siblings, 0 replies; 48+ messages in thread
From: Eduardo Habkost @ 2018-05-30 16:22 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: qemu-devel

On Wed, May 30, 2018 at 12:47:17PM +0200, Igor Mammedov wrote:
> Eduardo,
> 
> I've rebased series on top of current master
> the only change in several patches was s/2.13/3.0/
> otherwise there weren't any other conflicts.
> You can find rebased version at
> 
> https://github.com/imammedo/qemu.git qmp_preconfig_v9

Thanks!  I've rebased my branch on top of it and noticed that the
only differences are the "since 3.0" lines.  I've fixed them and
pushed a new numa-next branch.

Once the travis-ci job finishes, I will send a pull request.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state
  2018-05-28  9:31             ` Igor Mammedov
@ 2018-06-01  8:59               ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 48+ messages in thread
From: Dr. David Alan Gilbert @ 2018-06-01  8:59 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: Eduardo Habkost, pkrempa, Markus Armbruster, qemu-devel

* Igor Mammedov (imammedo@redhat.com) wrote:
> On Fri, 25 May 2018 16:39:34 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, May 25, 2018 at 08:05:30AM +0200, Markus Armbruster wrote:
> > > Eduardo Habkost <ehabkost@redhat.com> writes:
> > >   
> > > > On Thu, May 24, 2018 at 08:16:20PM +0200, Markus Armbruster wrote:  
> > > >> Markus Armbruster <armbru@redhat.com> writes:
> > > >>   
> > > >> > Igor Mammedov <imammedo@redhat.com> writes:
> > > >> >  
> > > >> >> 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>
> > > >> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> > > >> >> ---
> > > >> >> v5:
> > > >> >>   * add 'use QMP instead" to error message, suggesting user
> > > >> >>     the right interface to use
> > > >> >> 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 | 6 ++++++
> > > >> >>  1 file changed, 6 insertions(+)
> > > >> >>
> > > >> >> diff --git a/monitor.c b/monitor.c
> > > >> >> index 39f8ee1..0ffdf1d 100644
> > > >> >> --- a/monitor.c
> > > >> >> +++ b/monitor.c
> > > >> >> @@ -3374,6 +3374,12 @@ 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, "
> > > >> >> +                            "use QMP instead\n");
> > > >> >> +        return;
> > > >> >> +    }
> > > >> >> +
> > > >> >>      cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
> > > >> >>      if (!cmd) {
> > > >> >>          return;  
> > > >> >
> > > >> > So we offer the user an HMP monitor, but we summarily fail all commands.
> > > >> > I'm sorry, but that's... searching for polite word... embarrassing.  We
> > > >> > should accept HMP output only when we're ready to accept it.  Yes, that
> > > >> > would involve a bit more surgery rather than this cheap hack.  The whole
> > > >> > preconfig thing smells like a cheap hack to me, but let's not overdo it.  
> > > >> 
> > > >> Clarification: I don't think we need to hold the series because of
> > > >> this.  I do think you should investigate delaying HMP until it can work.  
> > > >
> > > > What would it mean to delay HMP?  Not creating the socket?
> > > > Creating the socket but not accepting clients?  Accepting clients
> > > > but not consuming any input from the socket until we are out of
> > > > preconfig?
> > > >
> > > > I'm not sure if any of those options would be better.  If a human
> > > > is already trying to talk on the other side, it seems better to
> > > > show QEMU is alive (but not ready to hold a conversation yet)
> > > > than staying silent.  
> > > 
> > > If this
> > > 
> > >     QEMU 2.12.50 monitor - type 'help' for more information
> > >     (qemu) help
> > >     HMP not available in preconfig state, use QMP instead
> > >     (qemu) quit
> > >     HMP not available in preconfig state, use QMP instead
> > >     (qemu) let me out dammit
> > >     HMP not available in preconfig state, use QMP instead
> > >     (qemu) 
> > > 
> > > is better than the alternatives, then I wonder how much more
> > > entertainment the alternatives could provide!
> > > 
> > > We *can* do better.  Start like this:
> > > 
> > >     QEMU 2.12.50 monitor is not ready with -preconfig until you complete
> > >     configuration with QMP
> > > 
> > > and when we exit preconfig state, add:
> > > 
> > >     QEMU 2.12.50 monitor - type 'help' for more information
> > >     (qemu) 
> > > 
> > > Note that this is upfront about the monitor not being ready, avoids
> > > misleading the user about "help", talks to the user in the user's terms
> > > (-preconfig) instead of internal terms (preconfig state), and is more
> > > specific on how to ready the monitor.  
> > 
> > Yes, this sounds better than any of the options I have
> > considered.
> > 
> > Making at least 'help', 'quit', and 'exit-preconfig' work might
> > be even better, though.
> I'll look into both options and try to come up a patch to make it better.

Lets keep whatever we do here simple.
As I understand it, the only reason to deny HMP in preconfig is
because we've not got a per-command flag to say which commands
are allowed in preconfig state.  If you're going to allow
'help', 'quit' etc then you just end up adding that flag
(which should be easy) and then we've got the flag and
we can go back and enable other HMP commands in preconfig as well.

Dave

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

^ permalink raw reply	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2018-06-01  8:59 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-04  8:37 [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Igor Mammedov
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 01/11] numa: postpone options post-processing till machine_run_board_init() Igor Mammedov
2018-05-16 18:12   ` Eduardo Habkost
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 02/11] numa: split out NumaOptions parsing into set_numa_options() Igor Mammedov
2018-05-16 18:12   ` Eduardo Habkost
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 03/11] qapi: introduce preconfig runstate Igor Mammedov
2018-05-11 15:17   ` Eric Blake
2018-05-16 18:14   ` Eduardo Habkost
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state Igor Mammedov
2018-05-24 16:00   ` Markus Armbruster
2018-05-24 18:16     ` Markus Armbruster
2018-05-24 18:26       ` Eduardo Habkost
2018-05-25  6:05         ` Markus Armbruster
2018-05-25 19:39           ` Eduardo Habkost
2018-05-28  9:31             ` Igor Mammedov
2018-06-01  8:59               ` Dr. David Alan Gilbert
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 05/11] qapi: introduce new cmd option "allowed-in-preconfig" Igor Mammedov
2018-05-11 15:26   ` Eric Blake
2018-05-11 16:56     ` Igor Mammedov
2018-05-11 16:51   ` [Qemu-devel] [PATCH v8 05/11] qapi: introduce new cmd option "allow-preconfig" Igor Mammedov
2018-05-11 17:16     ` Eric Blake
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 06/11] tests: qapi-schema tests for allow-preconfig Igor Mammedov
2018-05-11 15:29   ` Eric Blake
2018-05-16 18:16     ` Eduardo Habkost
2018-05-11 17:15   ` [Qemu-devel] [PATCH v8 " Igor Mammedov
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 07/11] cli: add --preconfig option Igor Mammedov
2018-05-11 15:36   ` Eric Blake
2018-05-11 17:24   ` [Qemu-devel] [PATCH v8 " Igor Mammedov
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 08/11] tests: extend qmp test with preconfig checks Igor Mammedov
2018-05-16 18:20   ` Eduardo Habkost
2018-05-16 22:21   ` Eric Blake
2018-05-17 11:28     ` [Qemu-devel] [PATCH v8 " Igor Mammedov
2018-05-17 13:43       ` Eric Blake
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 09/11] qmp: permit query-hotpluggable-cpus in preconfig state Igor Mammedov
2018-05-16 18:24   ` Eduardo Habkost
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 10/11] qmp: add set-numa-node command Igor Mammedov
2018-05-16 18:26   ` Eduardo Habkost
2018-05-04  8:37 ` [Qemu-devel] [PATCH v7 11/11] tests: functional tests for QMP command set-numa-node Igor Mammedov
2018-05-16 18:27   ` Eduardo Habkost
2018-05-16 22:12   ` Eduardo Habkost
2018-05-17  8:01     ` Igor Mammedov
2018-05-17 11:27       ` Igor Mammedov
2018-05-17 11:30   ` [Qemu-devel] [PATCH v8 " Igor Mammedov
2018-05-16 19:02 ` [Qemu-devel] [PATCH v7 00/11] enable numa configuration before machine_init() from QMP Eduardo Habkost
2018-05-24 15:42   ` Markus Armbruster
2018-05-28  9:40     ` Igor Mammedov
2018-05-30 10:47 ` Igor Mammedov
2018-05-30 16:22   ` Eduardo Habkost

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.