* [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP
@ 2018-02-16 12:37 Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 1/9] numa: postpone options post-processing till machine_run_board_init() Igor Mammedov
` (9 more replies)
0 siblings, 10 replies; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
v1->v3:
* introduce PRECONFIG runstate with -preconfig option.
it's cleaner to manage transitions and do checks
than reusing existing PRELAUNCH state.
* extend QAPI schema commands with 'runstates' keyword,
so that it would be possible to specify in command definition
when it is valid to execute.
(python changes a bit hackery, since I have little to
no idea how it should work)
* add preconfig QMP and set-numa-node tests
* make mutually exclusive -preconfig and -incoming options,
for simplicity sake. Shouldn't be problem as target can
be starter with pure CLI, since mapping on source is
already known.
* Drop HMP part and leave only QMP in preconfig state.
Series allows to configure NUMA mapping at runtime using QMP
interface. For that to happen it introduces a new '-preconfig' CLI option
which allows to pause QEMU before machine_init() is run and
adds new set-numa-node QMP command which in conjunction with
query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
Later we can modify other commands to run early, for example device_add.
I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
stage it's considered hotplug already), so SPAPR had to work around the issue.
Example of configuration session:
$QEMU -smp 2 -preconfig ...
QMP:
# get CPUs layout for current target/machine/CLI
-> {'execute': 'query-hotpluggable-cpus' }
<- {'return': [
{'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 1}, ... },
{'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 0}, ... }
]}
# configure 1st node
-> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 0 } }
<- {'return': {}}
-> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',
'node-id': 0, 'core-id': 0, 'thread-id': 0, 'socket-id': 1, }
}
<- {'return': {}}
# configure 2nd node
-> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 1 } }
-> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',
'node-id': 1, 'core-id': 0, 'thread-id': 0, 'socket-id': 0 }
}
<- {'return': {}}
# [optional] verify configuration
-> {'execute': 'query-hotpluggable-cpus' }
<- {'return': [
{'props': {'core-id': 0, 'thread-id': 0, 'node-id': 0, 'socket-id': 1}, ... },
{'props': {'core-id': 0, 'thread-id': 0, 'node-id': 1, 'socket-id': 0}, ... }
]}
Git tree:
https://github.com/imammedo/qemu.git qmp_preconfig_v3
Ref to v1:
https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03583.html
Message-Id: <1508170976-96869-1-git-send-email-imammedo@redhat.com>
CC: eblake@redhat.com
CC: armbru@redhat.com
CC: ehabkost@redhat.com
CC: pkrempa@redhat.com
CC: david@gibson.dropbear.id.au
CC: peter.maydell@linaro.org
CC: pbonzini@redhat.com
CC: cohuck@redhat.com
Igor Mammedov (9):
numa: postpone options post-processing till machine_run_board_init()
numa: split out NumaOptions parsing into parse_NumaOptions()
CLI: add -preconfig option
HMP: disable monitor in preconfig state
QAPI: allow to specify valid runstates per command
tests: extend qmp test with pereconfig 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 | 5 ++-
include/sysemu/numa.h | 2 +
include/sysemu/sysemu.h | 1 +
hw/core/machine.c | 5 ++-
monitor.c | 32 +++++++++++++--
numa.c | 66 +++++++++++++++++++-----------
qapi-schema.json | 32 +++++++++++++--
qapi/qmp-dispatch.c | 39 ++++++++++++++++++
qapi/qmp-registry.c | 4 +-
qapi/run-state.json | 9 ++++-
qemu-options.hx | 11 +++++
qmp.c | 5 +++
scripts/qapi-commands.py | 46 ++++++++++++++++-----
scripts/qapi-introspect.py | 2 +-
scripts/qapi.py | 15 ++++---
scripts/qapi2texi.py | 2 +-
tests/numa-test.c | 71 +++++++++++++++++++++++++++++++++
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.out | 18 ++++-----
tests/qapi-schema/test-qapi.py | 6 +--
tests/qmp-test.c | 56 ++++++++++++++++++++++++++
vl.c | 35 +++++++++++++++-
24 files changed, 401 insertions(+), 71 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v3 1/9] numa: postpone options post-processing till machine_run_board_init()
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
@ 2018-02-16 12:37 ` Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 2/9] numa: split out NumaOptions parsing into parse_NumaOptions() Igor Mammedov
` (8 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
in preparation for numa options to being handled via QMP before
machine_run_board_init(), move final numa configuration checks
and processing to machine_run_board_init() so it could take into
account both CLI (via parse_numa_opts()) and QMP input
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
- remove duplicate qemu_opts_foreach() in numa_complete_configuration()
that was causing non explicitly IDed node "-numa node" parsed twice.
---
include/sysemu/numa.h | 1 +
hw/core/machine.c | 5 +++--
numa.c | 13 ++++++++-----
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index d99e547..21713b7 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -23,6 +23,7 @@ struct NumaNodeMem {
extern NodeInfo numa_info[MAX_NODES];
void parse_numa_opts(MachineState *ms);
+void numa_complete_configuration(MachineState *ms);
void query_numa_node_mem(NumaNodeMem node_mem[]);
extern QemuOptsList qemu_numa_opts;
void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 5d44583..7d014b0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -715,7 +715,7 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
return g_string_free(s, false);
}
-static void machine_numa_finish_init(MachineState *machine)
+static void machine_numa_finish_cpu_init(MachineState *machine)
{
int i;
bool default_mapping;
@@ -770,7 +770,8 @@ void machine_run_board_init(MachineState *machine)
MachineClass *machine_class = MACHINE_GET_CLASS(machine);
if (nb_numa_nodes) {
- machine_numa_finish_init(machine);
+ numa_complete_configuration(machine);
+ machine_numa_finish_cpu_init(machine);
}
/* If the machine supports the valid_cpu_types check and the user
diff --git a/numa.c b/numa.c
index 7e0e789..1b2820a 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] 23+ messages in thread
* [Qemu-devel] [PATCH v3 2/9] numa: split out NumaOptions parsing into parse_NumaOptions()
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 1/9] numa: postpone options post-processing till machine_run_board_init() Igor Mammedov
@ 2018-02-16 12:37 ` Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 3/9] CLI: add -preconfig option Igor Mammedov
` (7 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
it will allow to reuse parse_NumaOptions() for parsing
configuration commands received via QMP interface
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/sysemu/numa.h | 1 +
numa.c | 48 +++++++++++++++++++++++++++++-------------------
2 files changed, 30 insertions(+), 19 deletions(-)
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 21713b7..7a0ae75 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -22,6 +22,7 @@ struct NumaNodeMem {
};
extern NodeInfo numa_info[MAX_NODES];
+int parse_numa(void *opaque, QemuOpts *opts, Error **errp);
void parse_numa_opts(MachineState *ms);
void numa_complete_configuration(MachineState *ms);
void query_numa_node_mem(NumaNodeMem node_mem[]);
diff --git a/numa.c b/numa.c
index 1b2820a..04e34eb 100644
--- a/numa.c
+++ b/numa.c
@@ -169,28 +169,11 @@ static void parse_numa_distance(NumaDistOptions *dist, Error **errp)
have_numa_distance = true;
}
-static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
+static
+void parse_NumaOptions(MachineState *ms, NumaOptions *object, Error **errp)
{
- NumaOptions *object = NULL;
- MachineState *ms = opaque;
Error *err = NULL;
- {
- Visitor *v = opts_visitor_new(opts);
- visit_type_NumaOptions(v, NULL, &object, &err);
- visit_free(v);
- }
-
- if (err) {
- goto end;
- }
-
- /* Fix up legacy suffix-less format */
- if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
- const char *mem_str = qemu_opt_get(opts, "mem");
- qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
- }
-
switch (object->type) {
case NUMA_OPTIONS_TYPE_NODE:
parse_numa_node(ms, &object->u.node, &err);
@@ -224,6 +207,33 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
}
end:
+ if (err) {
+ error_propagate(errp, err);
+ }
+}
+
+int parse_numa(void *opaque, QemuOpts *opts, Error **errp)
+{
+ NumaOptions *object = NULL;
+ MachineState *ms = MACHINE(opaque);
+ Error *err = NULL;
+ Visitor *v = opts_visitor_new(opts);
+
+ visit_type_NumaOptions(v, NULL, &object, &err);
+ visit_free(v);
+ if (err) {
+ goto end;
+ }
+
+ /* Fix up legacy suffix-less format */
+ if ((object->type == NUMA_OPTIONS_TYPE_NODE) && object->u.node.has_mem) {
+ const char *mem_str = qemu_opt_get(opts, "mem");
+ qemu_strtosz_MiB(mem_str, NULL, &object->u.node.mem);
+ }
+
+ parse_NumaOptions(ms, object, &err);
+
+end:
qapi_free_NumaOptions(object);
if (err) {
error_report_err(err);
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v3 3/9] CLI: add -preconfig option
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 1/9] numa: postpone options post-processing till machine_run_board_init() Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 2/9] numa: split out NumaOptions parsing into parse_NumaOptions() Igor Mammedov
@ 2018-02-16 12:37 ` Igor Mammedov
2018-02-27 20:39 ` Eric Blake
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 4/9] HMP: disable monitor in preconfig state Igor Mammedov
` (6 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
Option allows to pause QEMU at new RUN_STATE_PRECONFIG time,
which would allow to configure QEMU from QMP before machine
jumps into board initialization code machine_run_board_init().
Intent is to allow management to query machine state and
additionally configure it using previous query results
within one QEMU instance (i.e. eliminate need to start QEMU
twice, 1st to query board specific parameters and 2nd for
for actual VM start using query result for additional
parameters).
Initially it's planned to be used for configuring numa
topology depending on cpu layout.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/sysemu/sysemu.h | 1 +
qapi/run-state.json | 3 ++-
qemu-options.hx | 11 +++++++++++
qmp.c | 5 +++++
vl.c | 35 ++++++++++++++++++++++++++++++++++-
5 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 77bb3da..99d1aad 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -64,6 +64,7 @@ typedef enum WakeupReason {
QEMU_WAKEUP_REASON_OTHER,
} WakeupReason;
+void qemu_exit_preconfig_request(void);
void qemu_system_reset_request(ShutdownCause reason);
void qemu_system_suspend_request(void);
void qemu_register_suspend_notifier(Notifier *notifier);
diff --git a/qapi/run-state.json b/qapi/run-state.json
index bca46a8..d24a4e8 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -49,12 +49,13 @@
# @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 machine is created.
##
{ 'enum': 'RunState',
'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
- 'guest-panicked', 'colo' ] }
+ 'guest-panicked', 'colo', 'preconfig' ] }
##
# @StatusInfo:
diff --git a/qemu-options.hx b/qemu-options.hx
index 5050a49..ed0b4b7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3283,6 +3283,17 @@ 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 machine is created,
+which allows to query and configure properties affecting machine
+initialization. Use QMP command 'cont' to exit paused state.
+ETEXI
+
DEF("S", 0, QEMU_OPTION_S, \
"-S freeze CPU at startup (use 'c' to start execution)\n",
QEMU_ARCH_ALL)
diff --git a/qmp.c b/qmp.c
index 793f6f3..2c6e147 100644
--- a/qmp.c
+++ b/qmp.c
@@ -164,6 +164,11 @@ void qmp_cont(Error **errp)
BlockBackend *blk;
Error *local_err = NULL;
+ if (runstate_check(RUN_STATE_PRECONFIG)) {
+ qemu_exit_preconfig_request();
+ return;
+ }
+
/* if there is a dump in background, we should wait until the dump
* finished */
if (dump_in_progress()) {
diff --git a/vl.c b/vl.c
index 7a5554b..7fc133d 100644
--- a/vl.c
+++ b/vl.c
@@ -594,7 +594,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;
@@ -607,6 +607,9 @@ typedef struct {
static const RunStateTransition runstate_transitions_def[] = {
/* from -> to */
+ { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
+ { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },
+
{ RUN_STATE_DEBUG, RUN_STATE_RUNNING },
{ RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
{ RUN_STATE_DEBUG, RUN_STATE_PRELAUNCH },
@@ -1630,6 +1633,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);
@@ -1714,6 +1718,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.
*/
@@ -1880,6 +1889,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);
}
@@ -3701,6 +3717,14 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
break;
+ case QEMU_OPTION_preconfig:
+ if (runstate_check(RUN_STATE_INMIGRATE)) {
+ error_report("option can not be used with "
+ "-incoming option");
+ exit(EXIT_FAILURE);
+ }
+ preconfig_exit_requested = false;
+ break;
case QEMU_OPTION_enable_kvm:
olist = qemu_find_opts("machine");
qemu_opts_parse_noisily(olist, "accel=kvm", false);
@@ -3908,6 +3932,11 @@ int main(int argc, char **argv, char **envp)
}
break;
case QEMU_OPTION_incoming:
+ if (!preconfig_exit_requested) {
+ error_report("option can not be used with "
+ "-preconfig option");
+ exit(EXIT_FAILURE);
+ }
if (!incoming) {
runstate_set(RUN_STATE_INMIGRATE);
}
@@ -4607,6 +4636,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] 23+ messages in thread
* [Qemu-devel] [PATCH v3 4/9] HMP: disable monitor in preconfig state
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
` (2 preceding siblings ...)
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 3/9] CLI: add -preconfig option Igor Mammedov
@ 2018-02-16 12:37 ` Igor Mammedov
2018-03-07 14:01 ` Dr. David Alan Gilbert
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command Igor Mammedov
` (5 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
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>
---
---
monitor.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/monitor.c b/monitor.c
index f499250..fcb5386 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3097,6 +3097,10 @@ 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 precofig state\n");
+ }
+
cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
if (!cmd) {
return;
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
` (3 preceding siblings ...)
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 4/9] HMP: disable monitor in preconfig state Igor Mammedov
@ 2018-02-16 12:37 ` Igor Mammedov
2018-02-27 22:10 ` Eric Blake
2018-03-07 14:16 ` Dr. David Alan Gilbert
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 6/9] tests: extend qmp test with pereconfig checks Igor Mammedov
` (4 subsequent siblings)
9 siblings, 2 replies; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
Add optional 'runstates' parameter in QAPI command definition,
which will permit to specify RunState variations in which
a command could be exectuted via QMP monitor.
For compatibility reasons, commands, that don't use
'runstates' explicitly, are not permited to run in
preconfig state but allowed in all other states.
New option will be used to allow commands, which are
prepared/need to run this early, to run in preconfig state.
It will include query-hotpluggable-cpus and new set-numa-node
commands. Other commands that should be able to run in
preconfig state, should be ammeded to not expect machine
in initialized state.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/qapi/qmp/dispatch.h | 5 +++-
monitor.c | 28 +++++++++++++++++---
qapi-schema.json | 12 +++++++--
qapi/qmp-dispatch.c | 39 ++++++++++++++++++++++++++++
qapi/qmp-registry.c | 4 ++-
qapi/run-state.json | 6 ++++-
scripts/qapi-commands.py | 46 ++++++++++++++++++++++++++-------
scripts/qapi-introspect.py | 2 +-
scripts/qapi.py | 15 +++++++----
scripts/qapi2texi.py | 2 +-
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.out | 18 ++++++-------
tests/qapi-schema/test-qapi.py | 6 ++---
15 files changed, 151 insertions(+), 42 deletions(-)
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1e694b5..f821781 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -15,6 +15,7 @@
#define QAPI_QMP_DISPATCH_H
#include "qemu/queue.h"
+#include "qapi-types.h"
typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
@@ -31,12 +32,14 @@ typedef struct QmpCommand
QmpCommandOptions options;
QTAILQ_ENTRY(QmpCommand) node;
bool enabled;
+ const RunState *valid_runstates;
} QmpCommand;
typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
void qmp_register_command(QmpCommandList *cmds, const char *name,
- QmpCommandFunc *fn, QmpCommandOptions options);
+ QmpCommandFunc *fn, QmpCommandOptions options,
+ const RunState valid_runstates[]);
void qmp_unregister_command(QmpCommandList *cmds, const char *name);
QmpCommand *qmp_find_command(QmpCommandList *cmds, const char *name);
QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request);
diff --git a/monitor.c b/monitor.c
index fcb5386..2edc96c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -224,6 +224,25 @@ static mon_cmd_t mon_cmds[];
static mon_cmd_t info_cmds[];
QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
+const RunState cap_negotiation_valid_runstates[] = {
+ RUN_STATE_DEBUG,
+ RUN_STATE_INMIGRATE,
+ RUN_STATE_INTERNAL_ERROR,
+ RUN_STATE_IO_ERROR,
+ RUN_STATE_PAUSED,
+ RUN_STATE_POSTMIGRATE,
+ RUN_STATE_PRELAUNCH,
+ RUN_STATE_FINISH_MIGRATE,
+ RUN_STATE_RESTORE_VM,
+ RUN_STATE_RUNNING,
+ RUN_STATE_SAVE_VM,
+ RUN_STATE_SHUTDOWN,
+ RUN_STATE_SUSPENDED,
+ RUN_STATE_WATCHDOG,
+ RUN_STATE_GUEST_PANICKED,
+ RUN_STATE_COLO,
+ RUN_STATE_PRECONFIG,
+};
Monitor *cur_mon;
@@ -1016,17 +1035,18 @@ void monitor_init_qmp_commands(void)
qmp_register_command(&qmp_commands, "query-qmp-schema",
qmp_query_qmp_schema,
- QCO_NO_OPTIONS);
+ QCO_NO_OPTIONS, NULL);
qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
- QCO_NO_OPTIONS);
+ QCO_NO_OPTIONS, NULL);
qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
- QCO_NO_OPTIONS);
+ QCO_NO_OPTIONS, NULL);
qmp_unregister_commands_hack();
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_NO_OPTIONS,
+ cap_negotiation_valid_runstates);
}
void qmp_qmp_capabilities(Error **errp)
diff --git a/qapi-schema.json b/qapi-schema.json
index 0262b9f..ee1053d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -219,7 +219,11 @@
# Note: This example has been shortened as the real response is too long.
#
##
-{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
+{ 'command': 'query-commands', 'returns': ['CommandInfo'],
+ 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
+ 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
+ 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
+ 'guest-panicked', 'colo', 'preconfig' ] }
##
# @LostTickPolicy:
@@ -1146,7 +1150,11 @@
# <- { "return": {} }
#
##
-{ 'command': 'cont' }
+{ 'command': 'cont',
+ 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
+ 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
+ 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
+ 'guest-panicked', 'colo', 'preconfig' ] }
##
# @system_wakeup:
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index e31ac4b..26cba19 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -17,6 +17,7 @@
#include "qapi/qmp/json-parser.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qjson.h"
+#include "sysemu/sysemu.h"
static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
{
@@ -65,6 +66,40 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
return dict;
}
+static bool is_cmd_permited(const QmpCommand *cmd, Error **errp)
+{
+ int i;
+ char *list = NULL;
+
+ /* Old commands that don't have explicit runstate in schema
+ * are permited to run except of at PRECONFIG stage
+ */
+ if (!cmd->valid_runstates) {
+ if (runstate_check(RUN_STATE_PRECONFIG)) {
+ const char *state = RunState_str(RUN_STATE_PRECONFIG);
+ error_setg(errp, "The command '%s' isn't valid in '%s'",
+ cmd->name, state);
+ return false;
+ }
+ return true;
+ }
+
+ for (i = 0; cmd->valid_runstates[i] != RUN_STATE__MAX; i++) {
+ if (runstate_check(cmd->valid_runstates[i])) {
+ return true;
+ }
+ }
+
+ for (i = 0; cmd->valid_runstates[i] != RUN_STATE__MAX; i++) {
+ const char *state = RunState_str(cmd->valid_runstates[i]);
+ list = g_strjoin(", ", state, list, NULL);
+ }
+ error_setg(errp, "The command '%s' is valid only in '%s'", cmd->name, list);
+ g_free(list);
+
+ return false;
+}
+
static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
Error **errp)
{
@@ -92,6 +127,10 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
return NULL;
}
+ if (!is_cmd_permited(cmd, errp)) {
+ return NULL;
+ }
+
if (!qdict_haskey(dict, "arguments")) {
args = qdict_new();
} else {
diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
index 5af484c..59a5b85 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -16,7 +16,8 @@
#include "qapi/qmp/dispatch.h"
void qmp_register_command(QmpCommandList *cmds, const char *name,
- QmpCommandFunc *fn, QmpCommandOptions options)
+ QmpCommandFunc *fn, QmpCommandOptions options,
+ const RunState valid_runstates[])
{
QmpCommand *cmd = g_malloc0(sizeof(*cmd));
@@ -24,6 +25,7 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
cmd->fn = fn;
cmd->enabled = true;
cmd->options = options;
+ cmd->valid_runstates = valid_runstates;
QTAILQ_INSERT_TAIL(cmds, cmd, node);
}
diff --git a/qapi/run-state.json b/qapi/run-state.json
index d24a4e8..1c1c9b8 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -92,7 +92,11 @@
# "status": "running" } }
#
##
-{ 'command': 'query-status', 'returns': 'StatusInfo' }
+{ 'command': 'query-status', 'returns': 'StatusInfo',
+ 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
+ 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
+ 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
+ 'guest-panicked', 'colo', 'preconfig' ] }
##
# @SHUTDOWN:
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index f89d748..659e167 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -192,17 +192,45 @@ out:
return ret
-def gen_register_command(name, success_response):
+def gen_register_command(name, success_response, runstates):
options = 'QCO_NO_OPTIONS'
if not success_response:
options = 'QCO_NO_SUCCESS_RESP'
- ret = mcgen('''
- qmp_register_command(cmds, "%(name)s",
- qmp_marshal_%(c_name)s, %(opts)s);
-''',
- name=name, c_name=c_name(name),
- opts=options)
+ if runstates is None:
+ ret = mcgen('''
+ qmp_register_command(cmds, "%(name)s",
+ qmp_marshal_%(c_name)s, %(opts)s,
+ NULL);
+ ''',
+ name=name, c_name=c_name(name),
+ opts=options)
+ else:
+ ret = mcgen('''
+ static const RunState qmp_valid_states_%(c_name)s[] = {
+'''
+ , c_name=c_name(name))
+
+ for value in runstates:
+ ret += mcgen('''
+ %(c_enum)s,
+'''
+ , c_enum=c_enum_const('RunState', value))
+
+ ret += mcgen('''
+ %(c_enum)s,
+ };
+'''
+ , c_enum=c_enum_const('RunState', "_MAX"))
+
+ ret += mcgen('''
+ qmp_register_command(cmds, "%(name)s",
+ qmp_marshal_%(c_name)s, %(opts)s,
+ qmp_valid_states_%(c_name)s);
+ ''',
+ name=name, c_name=c_name(name),
+ opts=options)
+
return ret
@@ -241,7 +269,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
self._visited_ret_types = None
def visit_command(self, name, info, arg_type, ret_type,
- gen, success_response, boxed):
+ gen, success_response, boxed, runstates):
if not gen:
return
self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
@@ -250,7 +278,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
self.defn += gen_marshal_output(ret_type)
self.decl += gen_marshal_decl(name)
self.defn += gen_marshal(name, arg_type, boxed, ret_type)
- self._regy += gen_register_command(name, success_response)
+ self._regy += gen_register_command(name, success_response, runstates)
(input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line()
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index 032bcea..ede86ac 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -154,7 +154,7 @@ const char %(c_name)s[] = %(c_string)s;
for m in variants.variants]})
def visit_command(self, name, info, arg_type, ret_type,
- gen, success_response, boxed):
+ gen, success_response, boxed, runstates):
arg_type = arg_type or self._schema.the_empty_object_type
ret_type = ret_type or self._schema.the_empty_object_type
self._gen_json(name, 'command',
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 58f995b..5c5037e 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -919,7 +919,8 @@ def check_exprs(exprs):
elif 'command' in expr:
meta = 'command'
check_keys(expr_elem, 'command', [],
- ['data', 'returns', 'gen', 'success-response', 'boxed'])
+ ['data', 'returns', 'gen', 'success-response', 'boxed',
+ 'runstates'])
elif 'event' in expr:
meta = 'event'
check_keys(expr_elem, 'event', [], ['data', 'boxed'])
@@ -1030,7 +1031,7 @@ class QAPISchemaVisitor(object):
pass
def visit_command(self, name, info, arg_type, ret_type,
- gen, success_response, boxed):
+ gen, success_response, boxed, runstates):
pass
def visit_event(self, name, info, arg_type, boxed):
@@ -1397,7 +1398,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
class QAPISchemaCommand(QAPISchemaEntity):
def __init__(self, name, info, doc, arg_type, ret_type,
- gen, success_response, boxed):
+ gen, success_response, boxed, runstates):
QAPISchemaEntity.__init__(self, name, info, doc)
assert not arg_type or isinstance(arg_type, str)
assert not ret_type or isinstance(ret_type, str)
@@ -1408,6 +1409,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
self.gen = gen
self.success_response = success_response
self.boxed = boxed
+ self.runstates = runstates
def check(self, schema):
if self._arg_type_name:
@@ -1431,7 +1433,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
def visit(self, visitor):
visitor.visit_command(self.name, self.info,
self.arg_type, self.ret_type,
- self.gen, self.success_response, self.boxed)
+ self.gen, self.success_response, self.boxed,
+ self.runstates)
class QAPISchemaEvent(QAPISchemaEntity):
@@ -1639,6 +1642,7 @@ class QAPISchema(object):
gen = expr.get('gen', True)
success_response = expr.get('success-response', True)
boxed = expr.get('boxed', False)
+ runstates = expr.get('runstates')
if isinstance(data, OrderedDict):
data = self._make_implicit_object_type(
name, info, doc, 'arg', self._make_members(data, info))
@@ -1646,7 +1650,8 @@ class QAPISchema(object):
assert len(rets) == 1
rets = self._make_array_type(rets[0], info)
self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
- gen, success_response, boxed))
+ gen, success_response, boxed,
+ runstates))
def _def_event(self, expr, info, doc):
name = expr['event']
diff --git a/scripts/qapi2texi.py b/scripts/qapi2texi.py
index bf1c57b..d28594c 100755
--- a/scripts/qapi2texi.py
+++ b/scripts/qapi2texi.py
@@ -227,7 +227,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor):
body=texi_entity(doc, 'Members'))
def visit_command(self, name, info, arg_type, ret_type,
- gen, success_response, boxed):
+ gen, success_response, boxed, runstates):
doc = self.cur_doc
if boxed:
body = texi_body(doc)
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 1d2c250..a47d6f6 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -18,9 +18,9 @@ object Variant1
member var1: str optional=False
object Variant2
command cmd q_obj_cmd-arg -> Object
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
command cmd-boxed Object -> None
- gen=True success_response=True boxed=True
+ gen=True success_response=True boxed=True, runstates=None
object q_empty
object q_obj_Variant1-wrapper
member data: Variant1 optional=False
diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
index b5637cb..d9a272b 100644
--- a/tests/qapi-schema/ident-with-escape.out
+++ b/tests/qapi-schema/ident-with-escape.out
@@ -1,7 +1,7 @@
enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
prefix QTYPE
command fooA q_obj_fooA-arg -> None
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
object q_empty
object q_obj_fooA-arg
member bar1: str optional=False
diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
index 586795f..4b128d5 100644
--- a/tests/qapi-schema/indented-expr.out
+++ b/tests/qapi-schema/indented-expr.out
@@ -1,7 +1,7 @@
enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
prefix QTYPE
command eins None -> None
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
object q_empty
command zwei None -> None
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 3b1e908..787c228 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -153,15 +153,15 @@ object __org.qemu_x-Union2
tag __org.qemu_x-member1
case __org.qemu_x-value: __org.qemu_x-Struct2
command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
command boxed-struct UserDefZero -> None
- gen=True success_response=True boxed=True
+ gen=True success_response=True boxed=True, runstates=None
command boxed-union UserDefNativeListUnion -> None
- gen=True success_response=True boxed=True
+ gen=True success_response=True boxed=True, runstates=None
command guest-get-time q_obj_guest-get-time-arg -> int
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
command guest-sync q_obj_guest-sync-arg -> any
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
object q_empty
object q_obj_EVENT_C-arg
member a: int optional=True
@@ -222,10 +222,10 @@ object q_obj_user_def_cmd2-arg
member ud1a: UserDefOne optional=False
member ud1b: UserDefOne optional=True
command user_def_cmd None -> None
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
command user_def_cmd0 Empty2 -> Empty2
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
command user_def_cmd1 q_obj_user_def_cmd1-arg -> None
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
- gen=True success_response=True boxed=False
+ gen=True success_response=True boxed=False, runstates=None
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index ac43d34..958576d 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -37,11 +37,11 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
self._print_variants(variants)
def visit_command(self, name, info, arg_type, ret_type,
- gen, success_response, boxed):
+ gen, success_response, boxed, runstates):
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' % \
- (gen, success_response, boxed))
+ print(' gen=%s success_response=%s boxed=%s, runstates=%s' % \
+ (gen, success_response, boxed, runstates))
def visit_event(self, name, info, arg_type, boxed):
print('event %s %s' % (name, arg_type and arg_type.name))
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v3 6/9] tests: extend qmp test with pereconfig checks
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
` (4 preceding siblings ...)
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command Igor Mammedov
@ 2018-02-16 12:37 ` Igor Mammedov
2018-02-27 22:13 ` Eric Blake
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 7/9] QMP: permit query-hotpluggable-cpus in preconfig state Igor Mammedov
` (3 subsequent siblings)
9 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
Add permission checks for commands in 'preconfig' state.
It should work for all targets, but won't work with
machine 'none' as it's limited to -smp 1 only.
So use PC machine for testing preconfig and 'runstate'
parameter.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
tests/qmp-test.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 5808483..3adc485 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -304,8 +304,54 @@ static void add_query_tests(QmpSchema *schema)
}
}
+static bool is_err(QDict *rsp)
+{
+ const char *desc = NULL;
+ QDict *error = qdict_get_qdict(rsp, "error");
+ if (error) {
+ desc = qdict_get_try_str(error, "desc");
+ }
+ QDECREF(rsp);
+ return !!desc;
+}
+
+static void test_qmp_preconfig(void)
+{
+ QDict *rsp, *ret;
+ QTestState *qs = qtest_startf("-nodefaults -preconfig -smp 2");
+
+ /* preconfig state */
+ /* enabled commands, no error expected */
+ g_assert(!is_err(qtest_qmp(qs, "{ 'execute': 'query-commands' }")));
+
+ /* forbidden commands, expected error */
+ g_assert(is_err(qtest_qmp(qs, "{ 'execute': 'query-cpus' }")));
+
+ 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(!is_err(qtest_qmp(qs, "{ 'execute': 'cont' }")));
+ qtest_qmp_eventwait(qs, "RESUME");
+
+ 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(!is_err(qtest_qmp(qs, "{ 'execute': 'query-cpus' }")));
+
+ qtest_quit(qs);
+}
+
int main(int argc, char *argv[])
{
+ const char *arch = qtest_get_arch();
QmpSchema schema;
int ret;
@@ -314,6 +360,9 @@ int main(int argc, char *argv[])
qtest_add_func("qmp/protocol", test_qmp_protocol);
qmp_schema_init(&schema);
add_query_tests(&schema);
+ if (!strcmp(arch, "i386") || !strcmp(arch, "x86_64")) {
+ qtest_add_func("qmp/preconfig", test_qmp_preconfig);
+ }
ret = g_test_run();
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v3 7/9] QMP: permit query-hotpluggable-cpus in preconfig state
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
` (5 preceding siblings ...)
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 6/9] tests: extend qmp test with pereconfig checks Igor Mammedov
@ 2018-02-16 12:37 ` Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 8/9] QMP: add set-numa-node command Igor Mammedov
` (2 subsequent siblings)
9 siblings, 0 replies; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
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.
*) 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
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
qapi-schema.json | 6 +++++-
tests/qmp-test.c | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/qapi-schema.json b/qapi-schema.json
index ee1053d..4365dfe 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3167,7 +3167,11 @@
# ]}
#
##
-{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
+{ 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'],
+ 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
+ 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
+ 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
+ 'guest-panicked', 'colo', 'preconfig' ] }
##
# @GuidInfo:
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 3adc485..4ab0b7c 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -323,6 +323,7 @@ static void test_qmp_preconfig(void)
/* preconfig state */
/* enabled commands, no error expected */
g_assert(!is_err(qtest_qmp(qs, "{ 'execute': 'query-commands' }")));
+ g_assert(!is_err(qtest_qmp(qs, "{ 'execute': 'query-hotpluggable-cpus'}")));
/* forbidden commands, expected error */
g_assert(is_err(qtest_qmp(qs, "{ 'execute': 'query-cpus' }")));
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v3 8/9] QMP: add set-numa-node command
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
` (6 preceding siblings ...)
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 7/9] QMP: permit query-hotpluggable-cpus in preconfig state Igor Mammedov
@ 2018-02-16 12:37 ` Igor Mammedov
2018-02-27 22:17 ` Eric Blake
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 9/9] tests: functional tests for QMP command set-numa-node Igor Mammedov
2018-02-27 16:36 ` [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
9 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
Command is allowed to run only in preconfig stage and
will allow to configure numa mapping for CPUs depending
on possible CPUs layout (query-hotpluggable-cpus) for
given machine instance.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
numa.c | 5 +++++
qapi-schema.json | 14 ++++++++++++++
tests/qmp-test.c | 6 ++++++
3 files changed, 25 insertions(+)
diff --git a/numa.c b/numa.c
index 04e34eb..e3b7f15 100644
--- a/numa.c
+++ b/numa.c
@@ -446,6 +446,11 @@ void parse_numa_opts(MachineState *ms)
}
}
+void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
+{
+ parse_NumaOptions(MACHINE(qdev_get_machine()), cmd, errp);
+}
+
void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
{
int node_id = object_property_get_int(OBJECT(dev), "node-id", &error_abort);
diff --git a/qapi-schema.json b/qapi-schema.json
index 4365dfe..a96c31f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3201,3 +3201,17 @@
# Since: 2.11
##
{ 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
+
+##
+# @set-numa-node:
+#
+# Runtime equivalent of '-numa' CLI option, available at
+# preconfigure stage to configure numa mapping before initializing
+# machine.
+#
+# Since 2.12
+##
+{ 'command': 'set-numa-node', 'boxed': true,
+ 'data': 'NumaOptions',
+ 'runstates': [ 'preconfig' ]
+}
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 4ab0b7c..b0de2b1 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -324,6 +324,8 @@ static void test_qmp_preconfig(void)
/* enabled commands, no error expected */
g_assert(!is_err(qtest_qmp(qs, "{ 'execute': 'query-commands' }")));
g_assert(!is_err(qtest_qmp(qs, "{ 'execute': 'query-hotpluggable-cpus'}")));
+ g_assert(!is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+ " 'arguments': { 'type': 'node', 'nodeid': 0 } }")));
/* forbidden commands, expected error */
g_assert(is_err(qtest_qmp(qs, "{ 'execute': 'query-cpus' }")));
@@ -347,6 +349,10 @@ static void test_qmp_preconfig(void)
/* enabled commands, no error expected */
g_assert(!is_err(qtest_qmp(qs, "{ 'execute': 'query-cpus' }")));
+ /* forbidden commands, expected error */
+ g_assert(is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+ " 'arguments': { 'type': 'node', 'nodeid': 1 } }")));
+
qtest_quit(qs);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v3 9/9] tests: functional tests for QMP command set-numa-node
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
` (7 preceding siblings ...)
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 8/9] QMP: add set-numa-node command Igor Mammedov
@ 2018-02-16 12:37 ` Igor Mammedov
2018-02-27 22:19 ` Eric Blake
2018-02-27 16:36 ` [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
9 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2018-02-16 12:37 UTC (permalink / raw)
To: qemu-devel
Cc: eblake, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
* 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>
---
tests/numa-test.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 71 insertions(+)
diff --git a/tests/numa-test.c b/tests/numa-test.c
index 68aca9c..11c2842 100644
--- a/tests/numa-test.c
+++ b/tests/numa-test.c
@@ -260,6 +260,76 @@ static void aarch64_numa_cpu(const void *data)
g_free(cli);
}
+static bool is_err(QDict *response)
+{
+ const char *desc = NULL;
+ QDict *error = qdict_get_qdict(response, "error");
+ if (error) {
+ desc = qdict_get_try_str(error, "desc");
+ }
+ QDECREF(response);
+ return !!desc;
+}
+
+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(!is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+ " 'arguments': { 'type': 'node', 'nodeid': 0 } }")));
+ g_assert(!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(!is_err(qtest_qmp(qs, "{ 'execute': 'set-numa-node',"
+ " 'arguments': { 'type': 'cpu', 'node-id': 0, 'socket-id': 1 } }")));
+ g_assert(!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(!is_err(qtest_qmp(qs, "{ 'execute': 'cont' }")));
+ qtest_qmp_eventwait(qs, "RESUME");
+
+ /* check that CPUs are mapped as expected */
+ resp = qtest_qmp(qs, "{ 'execute': 'query-hotpluggable-cpus'}");
+ g_assert(qdict_haskey(resp, "return"));
+ cpus = qdict_get_qlist(resp, "return");
+ g_assert(cpus);
+ while ((e = qlist_pop(cpus))) {
+ const QDict *cpu, *props;
+ int64_t socket, node;
+
+ cpu = qobject_to_qdict(e);
+ g_assert(qdict_haskey(cpu, "props"));
+ props = qdict_get_qdict(cpu, "props");
+
+ g_assert(qdict_haskey(props, "node-id"));
+ node = qdict_get_int(props, "node-id");
+ g_assert(qdict_haskey(props, "socket-id"));
+ socket = qdict_get_int(props, "socket-id");
+
+ if (socket == 0) {
+ g_assert_cmpint(node, ==, 1);
+ } else if (socket == 1) {
+ g_assert_cmpint(node, ==, 0);
+ } else {
+ g_assert(false);
+ }
+ qobject_decref(e);
+ }
+
+ qtest_quit(qs);
+}
+
int main(int argc, char **argv)
{
const char *args = NULL;
@@ -278,6 +348,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")) {
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
` (8 preceding siblings ...)
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 9/9] tests: functional tests for QMP command set-numa-node Igor Mammedov
@ 2018-02-27 16:36 ` Igor Mammedov
2018-02-27 20:29 ` Eric Blake
9 siblings, 1 reply; 23+ messages in thread
From: Igor Mammedov @ 2018-02-27 16:36 UTC (permalink / raw)
To: qemu-devel
On Fri, 16 Feb 2018 13:37:12 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
Eric,
Adding you to CC list (git send-mail somehow haven't noticed you in cover letter).
Could you please look at QAPI/QMP parts of this series.
> v1->v3:
> * introduce PRECONFIG runstate with -preconfig option.
> it's cleaner to manage transitions and do checks
> than reusing existing PRELAUNCH state.
> * extend QAPI schema commands with 'runstates' keyword,
> so that it would be possible to specify in command definition
> when it is valid to execute.
> (python changes a bit hackery, since I have little to
> no idea how it should work)
> * add preconfig QMP and set-numa-node tests
> * make mutually exclusive -preconfig and -incoming options,
> for simplicity sake. Shouldn't be problem as target can
> be starter with pure CLI, since mapping on source is
> already known.
> * Drop HMP part and leave only QMP in preconfig state.
>
>
> Series allows to configure NUMA mapping at runtime using QMP
> interface. For that to happen it introduces a new '-preconfig' CLI option
> which allows to pause QEMU before machine_init() is run and
> adds new set-numa-node QMP command which in conjunction with
> query-hotpluggable-cpus allows to configure NUMA mapping for cpus.
>
> Later we can modify other commands to run early, for example device_add.
> I recall SPAPR had problem when libvirt started QEMU with -S and, while it's
> paused, added CPUs with device_add. Intent was to coldplug CPUs (but at that
> stage it's considered hotplug already), so SPAPR had to work around the issue.
>
> Example of configuration session:
> $QEMU -smp 2 -preconfig ...
>
> QMP:
> # get CPUs layout for current target/machine/CLI
> -> {'execute': 'query-hotpluggable-cpus' }
> <- {'return': [
> {'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 1}, ... },
> {'props': {'core-id': 0, 'thread-id': 0, 'socket-id': 0}, ... }
> ]}
>
> # configure 1st node
> -> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 0 } }
> <- {'return': {}}
> -> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',
> 'node-id': 0, 'core-id': 0, 'thread-id': 0, 'socket-id': 1, }
> }
> <- {'return': {}}
>
> # configure 2nd node
> -> {'execute': 'set-numa-node', 'arguments': { 'type': 'node', 'nodeid': 1 } }
> -> {'execute': 'set-numa-node', 'arguments': { 'type': 'cpu',
> 'node-id': 1, 'core-id': 0, 'thread-id': 0, 'socket-id': 0 }
> }
> <- {'return': {}}
>
> # [optional] verify configuration
> -> {'execute': 'query-hotpluggable-cpus' }
> <- {'return': [
> {'props': {'core-id': 0, 'thread-id': 0, 'node-id': 0, 'socket-id': 1}, ... },
> {'props': {'core-id': 0, 'thread-id': 0, 'node-id': 1, 'socket-id': 0}, ... }
> ]}
>
>
> Git tree:
> https://github.com/imammedo/qemu.git qmp_preconfig_v3
>
> Ref to v1:
> https://lists.gnu.org/archive/html/qemu-devel/2017-10/msg03583.html
> Message-Id: <1508170976-96869-1-git-send-email-imammedo@redhat.com>
>
> CC: eblake@redhat.com
> CC: armbru@redhat.com
> CC: ehabkost@redhat.com
> CC: pkrempa@redhat.com
> CC: david@gibson.dropbear.id.au
> CC: peter.maydell@linaro.org
> CC: pbonzini@redhat.com
> CC: cohuck@redhat.com
>
>
> Igor Mammedov (9):
> numa: postpone options post-processing till machine_run_board_init()
> numa: split out NumaOptions parsing into parse_NumaOptions()
> CLI: add -preconfig option
> HMP: disable monitor in preconfig state
> QAPI: allow to specify valid runstates per command
> tests: extend qmp test with pereconfig 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 | 5 ++-
> include/sysemu/numa.h | 2 +
> include/sysemu/sysemu.h | 1 +
> hw/core/machine.c | 5 ++-
> monitor.c | 32 +++++++++++++--
> numa.c | 66 +++++++++++++++++++-----------
> qapi-schema.json | 32 +++++++++++++--
> qapi/qmp-dispatch.c | 39 ++++++++++++++++++
> qapi/qmp-registry.c | 4 +-
> qapi/run-state.json | 9 ++++-
> qemu-options.hx | 11 +++++
> qmp.c | 5 +++
> scripts/qapi-commands.py | 46 ++++++++++++++++-----
> scripts/qapi-introspect.py | 2 +-
> scripts/qapi.py | 15 ++++---
> scripts/qapi2texi.py | 2 +-
> tests/numa-test.c | 71 +++++++++++++++++++++++++++++++++
> 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.out | 18 ++++-----
> tests/qapi-schema/test-qapi.py | 6 +--
> tests/qmp-test.c | 56 ++++++++++++++++++++++++++
> vl.c | 35 +++++++++++++++-
> 24 files changed, 401 insertions(+), 71 deletions(-)
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP
2018-02-27 16:36 ` [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
@ 2018-02-27 20:29 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2018-02-27 20:29 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel
On 02/27/2018 10:36 AM, Igor Mammedov wrote:
> On Fri, 16 Feb 2018 13:37:12 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
>
> Eric,
>
> Adding you to CC list (git send-mail somehow haven't noticed you in cover letter).
Actually, I did get CC'd on the original cover letter, but you are
reading the copy that hit the list, which means we are suffering from
mailman's bug that it rewrites emails sent through the list to drop cc
of readers that requested no duplicates (I really detest that mailman 2
corrupts cc's as a side effect of what is otherwise a potentially useful
knob, and can only hope that mailman 3 has split its behavior into two
separate knobs).
>
> Could you please look at QAPI/QMP parts of this series.
Yes, starting a read-through now.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/9] CLI: add -preconfig option
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 3/9] CLI: add -preconfig option Igor Mammedov
@ 2018-02-27 20:39 ` Eric Blake
2018-02-28 15:54 ` Igor Mammedov
0 siblings, 1 reply; 23+ messages in thread
From: Eric Blake @ 2018-02-27 20:39 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel
Cc: armbru, ehabkost, pkrempa, david, peter.maydell, pbonzini, cohuck
On 02/16/2018 06:37 AM, Igor Mammedov wrote:
> Option allows to pause QEMU at new RUN_STATE_PRECONFIG time,
> which would allow to configure QEMU from QMP before machine
> jumps into board initialization code machine_run_board_init().
Grammar suggestion:
This option allows pausing QEMU in the new RUN_STATE_PRECONFIG state,
allowing the configuration of QEMU from QMP before the machine jumps
into board initialization code of machine_run_board_init().
>
> Intent is to allow management to query machine state and
> additionally configure it using previous query results
> within one QEMU instance (i.e. eliminate need to start QEMU
> twice, 1st to query board specific parameters and 2nd for
> for actual VM start using query result for additional
> parameters).
>
> Initially it's planned to be used for configuring numa
> topology depending on cpu layout.
It may be worth mentioning in the commit message how this differs from
-S, and what the QMP client must do to get the guest started in this
mode to enter the normal lifecycle that it used to have when using -S.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> include/sysemu/sysemu.h | 1 +
> qapi/run-state.json | 3 ++-
> qemu-options.hx | 11 +++++++++++
> qmp.c | 5 +++++
> vl.c | 35 ++++++++++++++++++++++++++++++++++-
> 5 files changed, 53 insertions(+), 2 deletions(-)
>
> +++ b/qapi/run-state.json
> @@ -49,12 +49,13 @@
> # @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 machine is created.
Needs a '(since 2.12)' tag. Probably also be worth mentioning that this
state is only visible for clients that pass the new CLI option.
> +++ b/qemu-options.hx
> @@ -3283,6 +3283,17 @@ 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 machine is created,
> +which allows to query and configure properties affecting machine
> +initialization. Use QMP command 'cont' to exit paused state.
Pause QEMU for interactive configuration before the machine is created,
which allows querying and configuring properties that will affect
machine initialization. Use the QMP command 'cont' to exit the
preconfig state.
Hmm - can you also transition from preconfig to the normal paused state
via 'stop', which you would do to emit other commands that you used to
issue between the older 'qemu -S' and the 'cont'?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command Igor Mammedov
@ 2018-02-27 22:10 ` Eric Blake
2018-02-28 16:17 ` Igor Mammedov
2018-03-07 14:16 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 23+ messages in thread
From: Eric Blake @ 2018-02-27 22:10 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel
Cc: armbru, ehabkost, pkrempa, david, peter.maydell, pbonzini, cohuck
On 02/16/2018 06:37 AM, Igor Mammedov wrote:
> Add optional 'runstates' parameter in QAPI command definition,
> which will permit to specify RunState variations in which
> a command could be exectuted via QMP monitor.
s/exectuted/executed/
>
> For compatibility reasons, commands, that don't use
s/commands,/commands/
> 'runstates' explicitly, are not permited to run in
s/explicitly,/explicitly/
s/permited/permitted/
> preconfig state but allowed in all other states.
>
> New option will be used to allow commands, which are
> prepared/need to run this early, to run in preconfig state.
> It will include query-hotpluggable-cpus and new set-numa-node
> commands. Other commands that should be able to run in
> preconfig state, should be ammeded to not expect machine
s/ammeded/amended/
> in initialized state.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> include/qapi/qmp/dispatch.h | 5 +++-
> monitor.c | 28 +++++++++++++++++---
> qapi-schema.json | 12 +++++++--
> qapi/qmp-dispatch.c | 39 ++++++++++++++++++++++++++++
> qapi/qmp-registry.c | 4 ++-
> qapi/run-state.json | 6 ++++-
> scripts/qapi-commands.py | 46 ++++++++++++++++++++++++++-------
> scripts/qapi-introspect.py | 2 +-
> scripts/qapi.py | 15 +++++++----
> scripts/qapi2texi.py | 2 +-
> 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.out | 18 ++++++-------
> tests/qapi-schema/test-qapi.py | 6 ++---
> 15 files changed, 151 insertions(+), 42 deletions(-)
Missing mention in docs/; among other things, see how the OOB series
adds a similar per-command witness during QMP on whether the command can
be used in certain contexts:
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg05789.html
including edits to docs/devel/qapi-code-gen.txt (definitely needed here)
and docs/interop/qmp-spec.txt (may or may not be needed here).
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1e694b5..f821781 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -15,6 +15,7 @@
> #define QAPI_QMP_DISPATCH_H
>
> #include "qemu/queue.h"
> +#include "qapi-types.h"
Probably conflict with the pending work from Markus to reorganize the
QAPI header files to be more modular.
> +++ b/qapi-schema.json
> @@ -219,7 +219,11 @@
> # Note: This example has been shortened as the real response is too long.
> #
> ##
> -{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
> +{ 'command': 'query-commands', 'returns': ['CommandInfo'],
> + 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> + 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> + 'guest-panicked', 'colo', 'preconfig' ] }
Wow, that's going to be a lot of states to list for every command that
is interested in the non-default state. Would a simple bool flag be any
easier than a list of states, since it looks like preconfig is the only
special state?
>
> ##
> # @LostTickPolicy:
> @@ -1146,7 +1150,11 @@
> # <- { "return": {} }
> #
> ##
> -{ 'command': 'cont' }
> +{ 'command': 'cont',
> + 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> + 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> + 'guest-panicked', 'colo', 'preconfig' ] }
Should 'stop' also be permitted in the preconfig state, to get to the
state that used to be provided by 'qemu -S'?
> @@ -65,6 +66,40 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
> return dict;
> }
>
> +static bool is_cmd_permited(const QmpCommand *cmd, Error **errp)
s/permited/permitted/g
> +{
> + int i;
> + char *list = NULL;
> +
> + /* Old commands that don't have explicit runstate in schema
> + * are permited to run except of at PRECONFIG stage
including in the comments
> + */
> + if (!cmd->valid_runstates) {
> + if (runstate_check(RUN_STATE_PRECONFIG)) {
> + const char *state = RunState_str(RUN_STATE_PRECONFIG);
> + error_setg(errp, "The command '%s' isn't valid in '%s'",
> + cmd->name, state);
> + return false;
> + }
> + return true;
> + }
> +
> + for (i = 0; cmd->valid_runstates[i] != RUN_STATE__MAX; i++) {
> + if (runstate_check(cmd->valid_runstates[i])) {
> + return true;
> + }
> + }
> +
> + for (i = 0; cmd->valid_runstates[i] != RUN_STATE__MAX; i++) {
> + const char *state = RunState_str(cmd->valid_runstates[i]);
> + list = g_strjoin(", ", state, list, NULL);
This is rather complex compared to a simple bool of whether a command
can be run in preconfig; do we anticipate ever making any other commands
fine-grained by state where the length of this message is worthwhile?
> +++ b/scripts/qapi-commands.py
> @@ -192,17 +192,45 @@ out:
> return ret
>
>
> -def gen_register_command(name, success_response):
> +def gen_register_command(name, success_response, runstates):
> options = 'QCO_NO_OPTIONS'
> if not success_response:
> options = 'QCO_NO_SUCCESS_RESP'
>
> - ret = mcgen('''
> - qmp_register_command(cmds, "%(name)s",
> - qmp_marshal_%(c_name)s, %(opts)s);
> -''',
> - name=name, c_name=c_name(name),
> - opts=options)
> + if runstates is None:
> + ret = mcgen('''
> + qmp_register_command(cmds, "%(name)s",
This is changing the indentation of generated output; everything within
the mcgen() should be indented according to the output level, not the
current Python nesting of the source file.
> + qmp_marshal_%(c_name)s, %(opts)s,
> + NULL);
> + ''',
> + name=name, c_name=c_name(name),
> + opts=options)
> + else:
> + ret = mcgen('''
> + static const RunState qmp_valid_states_%(c_name)s[] = {
> +'''
> + , c_name=c_name(name))
Unusual placement of the , between args to mcgen().
> +
> + for value in runstates:
> + ret += mcgen('''
> + %(c_enum)s,
> +'''
> + , c_enum=c_enum_const('RunState', value))
> +
> + ret += mcgen('''
> + %(c_enum)s,
> + };
> +'''
> + , c_enum=c_enum_const('RunState', "_MAX"))
> +
> + ret += mcgen('''
> + qmp_register_command(cmds, "%(name)s",
> + qmp_marshal_%(c_name)s, %(opts)s,
> + qmp_valid_states_%(c_name)s);
> + ''',
> + name=name, c_name=c_name(name),
> + opts=options)
> +
> return ret
>
>
> @@ -241,7 +269,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
> self._visited_ret_types = None
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> + gen, success_response, boxed, runstates):
> if not gen:
> return
> self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
> @@ -250,7 +278,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
> self.defn += gen_marshal_output(ret_type)
> self.decl += gen_marshal_decl(name)
> self.defn += gen_marshal(name, arg_type, boxed, ret_type)
> - self._regy += gen_register_command(name, success_response)
> + self._regy += gen_register_command(name, success_response, runstates)
Yeah, we'll definitely need to rebase this on top of Markus' work to
modularize QAPI output files.
> +++ b/scripts/qapi.py
> @@ -919,7 +919,8 @@ def check_exprs(exprs):
> elif 'command' in expr:
> meta = 'command'
> check_keys(expr_elem, 'command', [],
> - ['data', 'returns', 'gen', 'success-response', 'boxed'])
> + ['data', 'returns', 'gen', 'success-response', 'boxed',
> + 'runstates'])
'runstates' is specific to QMP, and doesn't really apply to QGA, right?
That makes me wonder if it is really the best interface to be exposing
to the QAPI generator. Certainly, a single bool that states whether a
command is allowed in preconfig is a bit more extensible to both QGA and
QMP, when compared to a list of QMP-specific states.
> @@ -1639,6 +1642,7 @@ class QAPISchema(object):
> gen = expr.get('gen', True)
> success_response = expr.get('success-response', True)
> boxed = expr.get('boxed', False)
> + runstates = expr.get('runstates')
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(
> name, info, doc, 'arg', self._make_members(data, info))
> @@ -1646,7 +1650,8 @@ class QAPISchema(object):
> assert len(rets) == 1
> rets = self._make_array_type(rets[0], info)
> self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
> - gen, success_response, boxed))
> + gen, success_response, boxed,
> + runstates))
Where do we validate that the list of states passed in the QAPI file
actually makes sense (and that the user didn't typo anything)? That's
another argument for a bool flag rather than an array of states, as it
is easier to validate that a bool was set correctly rather than that a
list doesn't introduce bogus states.
> +++ b/tests/qapi-schema/doc-good.out
> @@ -18,9 +18,9 @@ object Variant1
> member var1: str optional=False
> object Variant2
> command cmd q_obj_cmd-arg -> Object
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
Inconsistent on whether output arguments are separated by comma...
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -37,11 +37,11 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
> self._print_variants(variants)
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> + gen, success_response, boxed, runstates):
> 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' % \
> - (gen, success_response, boxed))
> + print(' gen=%s success_response=%s boxed=%s, runstates=%s' % \
...here
Also, while you did add coverage of the new information in the
successful tests, you didn't add any negative tests to check diagnosis
messages emitted when the field is present in the QAPI schema but
doesn't make sense, and you didn't modify any of the test-only QAPI
commands to use non-default states (which means only the live QMP
commands test the new feature). I would have expected at least a change
in tests/qapi-schema/qapi-schema-test.json.
Overall, this is adding a lot of complexity to QMP; are we sure this is
the interface libvirt wants to use for early NUMA configuration? Can we
simplify it to just a new bool, similar to 'allow-oob'?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 6/9] tests: extend qmp test with pereconfig checks
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 6/9] tests: extend qmp test with pereconfig checks Igor Mammedov
@ 2018-02-27 22:13 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2018-02-27 22:13 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel
Cc: armbru, ehabkost, pkrempa, david, peter.maydell, pbonzini, cohuck
On 02/16/2018 06:37 AM, Igor Mammedov wrote:
In the subject line: s/pereconfig/preconfig/
> Add permission checks for commands in 'preconfig' state.
>
> It should work for all targets, but won't work with
> machine 'none' as it's limited to -smp 1 only.
> So use PC machine for testing preconfig and 'runstate'
> parameter.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> tests/qmp-test.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
>
> +static bool is_err(QDict *rsp)
> +{
> + const char *desc = NULL;
> + QDict *error = qdict_get_qdict(rsp, "error");
> + if (error) {
> + desc = qdict_get_try_str(error, "desc");
> + }
> + QDECREF(rsp);
> + return !!desc;
Wait, so this returns false if this was an error but without a valid desc?
> +}
> +
> +static void test_qmp_preconfig(void)
> +{
> + QDict *rsp, *ret;
> + QTestState *qs = qtest_startf("-nodefaults -preconfig -smp 2");
> +
> + /* preconfig state */
> + /* enabled commands, no error expected */
> + g_assert(!is_err(qtest_qmp(qs, "{ 'execute': 'query-commands' }")));
> +
> + /* forbidden commands, expected error */
> + g_assert(is_err(qtest_qmp(qs, "{ 'execute': 'query-cpus' }")));
Does introspection show which commands are valid in preconfig state?
That may be useful information for a client to know.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 8/9] QMP: add set-numa-node command
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 8/9] QMP: add set-numa-node command Igor Mammedov
@ 2018-02-27 22:17 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2018-02-27 22:17 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel
Cc: armbru, ehabkost, pkrempa, david, peter.maydell, pbonzini, cohuck
On 02/16/2018 06:37 AM, 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.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> numa.c | 5 +++++
> qapi-schema.json | 14 ++++++++++++++
> tests/qmp-test.c | 6 ++++++
> 3 files changed, 25 insertions(+)
>
> +++ b/qapi-schema.json
> @@ -3201,3 +3201,17 @@
> # Since: 2.11
> ##
> { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
> +
> +##
> +# @set-numa-node:
> +#
> +# Runtime equivalent of '-numa' CLI option, available at
> +# preconfigure stage to configure numa mapping before initializing
> +# machine.
> +#
> +# Since 2.12
> +##
> +{ 'command': 'set-numa-node', 'boxed': true,
> + 'data': 'NumaOptions',
> + 'runstates': [ 'preconfig' ]
> +}
Oh, so you ARE trying to do fine-grained control of which commands are
valid in which states. Still, would that be easier through a
three-state enum (or pair of bools) instead of making every client
enumerate an array of 'all states', 'all but preconfig', and 'preconfig
only'?
Also, while preconfig is special (not every command can be made to run
during preconfig, so having the state rejection logic centralized makes
some sense), there are a lot fewer commands that are preconfig-only -
could those commands (just set-numa-node at the moment) be made to
perform state checks themselves rather than relying on centralized
logic, and then you still only need a single bool in the QAPI schema
(safe for preconfig, unsafe for preconfig in central logic; unsafe in
other states in a per-command code).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 9/9] tests: functional tests for QMP command set-numa-node
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 9/9] tests: functional tests for QMP command set-numa-node Igor Mammedov
@ 2018-02-27 22:19 ` Eric Blake
0 siblings, 0 replies; 23+ messages in thread
From: Eric Blake @ 2018-02-27 22:19 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel
Cc: armbru, ehabkost, pkrempa, david, peter.maydell, pbonzini, cohuck
On 02/16/2018 06:37 AM, 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>
> ---
> tests/numa-test.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 71 insertions(+)
>
> diff --git a/tests/numa-test.c b/tests/numa-test.c
> index 68aca9c..11c2842 100644
> --- a/tests/numa-test.c
> +++ b/tests/numa-test.c
> @@ -260,6 +260,76 @@ static void aarch64_numa_cpu(const void *data)
> g_free(cli);
> }
>
> +static bool is_err(QDict *response)
> +{
> + const char *desc = NULL;
> + QDict *error = qdict_get_qdict(response, "error");
> + if (error) {
> + desc = qdict_get_try_str(error, "desc");
> + }
> + QDECREF(response);
> + return !!desc;
Why are we duplicating this helper in more than one .c file? If it is a
common task, it should be in a common file for code reuse. And as
before, why are you returning false if the reply is an error but merely
lacked 'desc'?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/9] CLI: add -preconfig option
2018-02-27 20:39 ` Eric Blake
@ 2018-02-28 15:54 ` Igor Mammedov
0 siblings, 0 replies; 23+ messages in thread
From: Igor Mammedov @ 2018-02-28 15:54 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, peter.maydell, pkrempa, ehabkost, cohuck, armbru,
pbonzini, david
On Tue, 27 Feb 2018 14:39:22 -0600
Eric Blake <eblake@redhat.com> wrote:
> On 02/16/2018 06:37 AM, Igor Mammedov wrote:
> > Option allows to pause QEMU at new RUN_STATE_PRECONFIG time,
> > which would allow to configure QEMU from QMP before machine
> > jumps into board initialization code machine_run_board_init().
>
> Grammar suggestion:
>
> This option allows pausing QEMU in the new RUN_STATE_PRECONFIG state,
> allowing the configuration of QEMU from QMP before the machine jumps
> into board initialization code of machine_run_board_init().
>
> >
> > Intent is to allow management to query machine state and
> > additionally configure it using previous query results
> > within one QEMU instance (i.e. eliminate need to start QEMU
> > twice, 1st to query board specific parameters and 2nd for
> > for actual VM start using query result for additional
> > parameters).
> >
> > Initially it's planned to be used for configuring numa
> > topology depending on cpu layout.
>
> It may be worth mentioning in the commit message how this differs from
> -S, and what the QMP client must do to get the guest started in this
> mode to enter the normal lifecycle that it used to have when using -S.
>
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > include/sysemu/sysemu.h | 1 +
> > qapi/run-state.json | 3 ++-
> > qemu-options.hx | 11 +++++++++++
> > qmp.c | 5 +++++
> > vl.c | 35 ++++++++++++++++++++++++++++++++++-
> > 5 files changed, 53 insertions(+), 2 deletions(-)
> >
>
> > +++ b/qapi/run-state.json
> > @@ -49,12 +49,13 @@
> > # @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 machine is created.
>
> Needs a '(since 2.12)' tag. Probably also be worth mentioning that this
> state is only visible for clients that pass the new CLI option.
>
> > +++ b/qemu-options.hx
> > @@ -3283,6 +3283,17 @@ 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 machine is created,
> > +which allows to query and configure properties affecting machine
> > +initialization. Use QMP command 'cont' to exit paused state.
>
> Pause QEMU for interactive configuration before the machine is created,
> which allows querying and configuring properties that will affect
> machine initialization. Use the QMP command 'cont' to exit the
> preconfig state.
Thanks,
I'll fix all of above on respin.
> Hmm - can you also transition from preconfig to the normal paused state
> via 'stop', which you would do to emit other commands that you used to
> issue between the older 'qemu -S' and the 'cont'?
Both -preconfig and -S could be used together on CLI to
achieve above effect, where 'cont' would transition state
to the next requested state (i.e. if -S specified QEMU will do
second pause where it used to do and if there is no -S on
CLI machine will start to execute guest code).
I'm not sure about suggested 'stop' behavior though,
wouldn't it be confusing since it would actually do
something beside pausing and from already paused state
at that?
I've opted for adding as little as possible of new behavior,
and 'cont' looked like a reasonable choice.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
2018-02-27 22:10 ` Eric Blake
@ 2018-02-28 16:17 ` Igor Mammedov
0 siblings, 0 replies; 23+ messages in thread
From: Igor Mammedov @ 2018-02-28 16:17 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, armbru, ehabkost, pkrempa, david, peter.maydell,
pbonzini, cohuck
On Tue, 27 Feb 2018 16:10:31 -0600
Eric Blake <eblake@redhat.com> wrote:
> On 02/16/2018 06:37 AM, Igor Mammedov wrote:
> > Add optional 'runstates' parameter in QAPI command definition,
> > which will permit to specify RunState variations in which
> > a command could be exectuted via QMP monitor.
>
> s/exectuted/executed/
>
> >
> > For compatibility reasons, commands, that don't use
>
> s/commands,/commands/
>
> > 'runstates' explicitly, are not permited to run in
>
> s/explicitly,/explicitly/
> s/permited/permitted/
>
> > preconfig state but allowed in all other states.
> >
> > New option will be used to allow commands, which are
> > prepared/need to run this early, to run in preconfig state.
> > It will include query-hotpluggable-cpus and new set-numa-node
> > commands. Other commands that should be able to run in
> > preconfig state, should be ammeded to not expect machine
>
> s/ammeded/amended/
>
> > in initialized state.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > include/qapi/qmp/dispatch.h | 5 +++-
> > monitor.c | 28 +++++++++++++++++---
> > qapi-schema.json | 12 +++++++--
> > qapi/qmp-dispatch.c | 39 ++++++++++++++++++++++++++++
> > qapi/qmp-registry.c | 4 ++-
> > qapi/run-state.json | 6 ++++-
> > scripts/qapi-commands.py | 46 ++++++++++++++++++++++++++-------
> > scripts/qapi-introspect.py | 2 +-
> > scripts/qapi.py | 15 +++++++----
> > scripts/qapi2texi.py | 2 +-
> > 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.out | 18 ++++++-------
> > tests/qapi-schema/test-qapi.py | 6 ++---
> > 15 files changed, 151 insertions(+), 42 deletions(-)
>
[...]
> Overall, this is adding a lot of complexity to QMP; are we sure this is
> the interface libvirt wants to use for early NUMA configuration? Can we
> simplify it to just a new bool, similar to 'allow-oob'?
Being unsure if preconfig only specific knob would fly, I was trying
to introduce a generic mechanism that could be used to limit any command
to certain runstates.
I'd gladly to scrape it off and implement something like 'allow-oob'
if that's acceptable. It could be "preconfig-enabled" which defaults
to false and necessary commands would be explicitly marked as preconfig
enabled. Then I could just build into 'set-numa-node' C callback
an extra check to limit it only to preconfig state, like we typically
do in other handlers instead of going for more complex generic approach.
That would significantly simplify series.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/9] HMP: disable monitor in preconfig state
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 4/9] HMP: disable monitor in preconfig state Igor Mammedov
@ 2018-03-07 14:01 ` Dr. David Alan Gilbert
2018-03-08 15:47 ` Igor Mammedov
0 siblings, 1 reply; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2018-03-07 14:01 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, peter.maydell, pkrempa, ehabkost, cohuck, armbru,
pbonzini, david
* Igor Mammedov (imammedo@redhat.com) wrote:
> Ban it for now, if someone would need it to work early,
> one would have to implement checks if HMP command is valid
> at preconfig state.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> ---
> monitor.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index f499250..fcb5386 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3097,6 +3097,10 @@ 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 precofig state\n");
> + }
But you've not returned, so the command will still get parsed?
Dave
> cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
> if (!cmd) {
> return;
> --
> 2.7.4
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command Igor Mammedov
2018-02-27 22:10 ` Eric Blake
@ 2018-03-07 14:16 ` Dr. David Alan Gilbert
2018-03-08 15:55 ` Igor Mammedov
1 sibling, 1 reply; 23+ messages in thread
From: Dr. David Alan Gilbert @ 2018-03-07 14:16 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, peter.maydell, pkrempa, ehabkost, cohuck, armbru,
pbonzini, david
* Igor Mammedov (imammedo@redhat.com) wrote:
> Add optional 'runstates' parameter in QAPI command definition,
> which will permit to specify RunState variations in which
> a command could be exectuted via QMP monitor.
>
> For compatibility reasons, commands, that don't use
> 'runstates' explicitly, are not permited to run in
> preconfig state but allowed in all other states.
>
> New option will be used to allow commands, which are
> prepared/need to run this early, to run in preconfig state.
> It will include query-hotpluggable-cpus and new set-numa-node
> commands. Other commands that should be able to run in
> preconfig state, should be ammeded to not expect machine
> in initialized state.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> include/qapi/qmp/dispatch.h | 5 +++-
> monitor.c | 28 +++++++++++++++++---
> qapi-schema.json | 12 +++++++--
> qapi/qmp-dispatch.c | 39 ++++++++++++++++++++++++++++
> qapi/qmp-registry.c | 4 ++-
> qapi/run-state.json | 6 ++++-
> scripts/qapi-commands.py | 46 ++++++++++++++++++++++++++-------
> scripts/qapi-introspect.py | 2 +-
> scripts/qapi.py | 15 +++++++----
> scripts/qapi2texi.py | 2 +-
> 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.out | 18 ++++++-------
> tests/qapi-schema/test-qapi.py | 6 ++---
> 15 files changed, 151 insertions(+), 42 deletions(-)
>
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1e694b5..f821781 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.h
> @@ -15,6 +15,7 @@
> #define QAPI_QMP_DISPATCH_H
>
> #include "qemu/queue.h"
> +#include "qapi-types.h"
>
> typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);
>
> @@ -31,12 +32,14 @@ typedef struct QmpCommand
> QmpCommandOptions options;
> QTAILQ_ENTRY(QmpCommand) node;
> bool enabled;
> + const RunState *valid_runstates;
> } QmpCommand;
>
> typedef QTAILQ_HEAD(QmpCommandList, QmpCommand) QmpCommandList;
>
> void qmp_register_command(QmpCommandList *cmds, const char *name,
> - QmpCommandFunc *fn, QmpCommandOptions options);
> + QmpCommandFunc *fn, QmpCommandOptions options,
> + const RunState valid_runstates[]);
> void qmp_unregister_command(QmpCommandList *cmds, const char *name);
> QmpCommand *qmp_find_command(QmpCommandList *cmds, const char *name);
> QObject *qmp_dispatch(QmpCommandList *cmds, QObject *request);
> diff --git a/monitor.c b/monitor.c
> index fcb5386..2edc96c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -224,6 +224,25 @@ static mon_cmd_t mon_cmds[];
> static mon_cmd_t info_cmds[];
>
> QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> +const RunState cap_negotiation_valid_runstates[] = {
> + RUN_STATE_DEBUG,
> + RUN_STATE_INMIGRATE,
> + RUN_STATE_INTERNAL_ERROR,
> + RUN_STATE_IO_ERROR,
> + RUN_STATE_PAUSED,
> + RUN_STATE_POSTMIGRATE,
> + RUN_STATE_PRELAUNCH,
> + RUN_STATE_FINISH_MIGRATE,
> + RUN_STATE_RESTORE_VM,
> + RUN_STATE_RUNNING,
> + RUN_STATE_SAVE_VM,
> + RUN_STATE_SHUTDOWN,
> + RUN_STATE_SUSPENDED,
> + RUN_STATE_WATCHDOG,
> + RUN_STATE_GUEST_PANICKED,
> + RUN_STATE_COLO,
> + RUN_STATE_PRECONFIG,
> +};
It's a shame this is such a manual copy.
While you're taking a big hammer to HMP in the preconfig case,
it's worth noting this more specific code is only protecting QMP
commands.
It's a shame in all the uses below we can't be working with bitmasks
of run-state's; it would feel a lot easier to have a default and mask
out or or in extra states.
Dave
> Monitor *cur_mon;
>
> @@ -1016,17 +1035,18 @@ void monitor_init_qmp_commands(void)
>
> qmp_register_command(&qmp_commands, "query-qmp-schema",
> qmp_query_qmp_schema,
> - QCO_NO_OPTIONS);
> + QCO_NO_OPTIONS, NULL);
> qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
> - QCO_NO_OPTIONS);
> + QCO_NO_OPTIONS, NULL);
> qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
> - QCO_NO_OPTIONS);
> + QCO_NO_OPTIONS, NULL);
>
> qmp_unregister_commands_hack();
>
> 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_NO_OPTIONS,
> + cap_negotiation_valid_runstates);
> }
>
> void qmp_qmp_capabilities(Error **errp)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 0262b9f..ee1053d 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -219,7 +219,11 @@
> # Note: This example has been shortened as the real response is too long.
> #
> ##
> -{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
> +{ 'command': 'query-commands', 'returns': ['CommandInfo'],
> + 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> + 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> + 'guest-panicked', 'colo', 'preconfig' ] }
That's going to get to be a pain to update as we add more states.
> ##
> # @LostTickPolicy:
> @@ -1146,7 +1150,11 @@
> # <- { "return": {} }
> #
> ##
> -{ 'command': 'cont' }
> +{ 'command': 'cont',
> + 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> + 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> + 'guest-panicked', 'colo', 'preconfig' ] }
>
> ##
> # @system_wakeup:
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index e31ac4b..26cba19 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -17,6 +17,7 @@
> #include "qapi/qmp/json-parser.h"
> #include "qapi/qmp/qdict.h"
> #include "qapi/qmp/qjson.h"
> +#include "sysemu/sysemu.h"
>
> static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
> {
> @@ -65,6 +66,40 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp)
> return dict;
> }
>
> +static bool is_cmd_permited(const QmpCommand *cmd, Error **errp)
> +{
> + int i;
> + char *list = NULL;
> +
> + /* Old commands that don't have explicit runstate in schema
> + * are permited to run except of at PRECONFIG stage
> + */
> + if (!cmd->valid_runstates) {
> + if (runstate_check(RUN_STATE_PRECONFIG)) {
> + const char *state = RunState_str(RUN_STATE_PRECONFIG);
> + error_setg(errp, "The command '%s' isn't valid in '%s'",
> + cmd->name, state);
> + return false;
> + }
> + return true;
> + }
> +
> + for (i = 0; cmd->valid_runstates[i] != RUN_STATE__MAX; i++) {
> + if (runstate_check(cmd->valid_runstates[i])) {
> + return true;
> + }
> + }
> +
> + for (i = 0; cmd->valid_runstates[i] != RUN_STATE__MAX; i++) {
> + const char *state = RunState_str(cmd->valid_runstates[i]);
> + list = g_strjoin(", ", state, list, NULL);
> + }
> + error_setg(errp, "The command '%s' is valid only in '%s'", cmd->name, list);
> + g_free(list);
> +
> + return false;
> +}
> +
> static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
> Error **errp)
> {
> @@ -92,6 +127,10 @@ static QObject *do_qmp_dispatch(QmpCommandList *cmds, QObject *request,
> return NULL;
> }
>
> + if (!is_cmd_permited(cmd, errp)) {
> + return NULL;
> + }
> +
> if (!qdict_haskey(dict, "arguments")) {
> args = qdict_new();
> } else {
> diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
> index 5af484c..59a5b85 100644
> --- a/qapi/qmp-registry.c
> +++ b/qapi/qmp-registry.c
> @@ -16,7 +16,8 @@
> #include "qapi/qmp/dispatch.h"
>
> void qmp_register_command(QmpCommandList *cmds, const char *name,
> - QmpCommandFunc *fn, QmpCommandOptions options)
> + QmpCommandFunc *fn, QmpCommandOptions options,
> + const RunState valid_runstates[])
> {
> QmpCommand *cmd = g_malloc0(sizeof(*cmd));
>
> @@ -24,6 +25,7 @@ void qmp_register_command(QmpCommandList *cmds, const char *name,
> cmd->fn = fn;
> cmd->enabled = true;
> cmd->options = options;
> + cmd->valid_runstates = valid_runstates;
> QTAILQ_INSERT_TAIL(cmds, cmd, node);
> }
>
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index d24a4e8..1c1c9b8 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -92,7 +92,11 @@
> # "status": "running" } }
> #
> ##
> -{ 'command': 'query-status', 'returns': 'StatusInfo' }
> +{ 'command': 'query-status', 'returns': 'StatusInfo',
> + 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> + 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> + 'guest-panicked', 'colo', 'preconfig' ] }
>
> ##
> # @SHUTDOWN:
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index f89d748..659e167 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -192,17 +192,45 @@ out:
> return ret
>
>
> -def gen_register_command(name, success_response):
> +def gen_register_command(name, success_response, runstates):
> options = 'QCO_NO_OPTIONS'
> if not success_response:
> options = 'QCO_NO_SUCCESS_RESP'
>
> - ret = mcgen('''
> - qmp_register_command(cmds, "%(name)s",
> - qmp_marshal_%(c_name)s, %(opts)s);
> -''',
> - name=name, c_name=c_name(name),
> - opts=options)
> + if runstates is None:
> + ret = mcgen('''
> + qmp_register_command(cmds, "%(name)s",
> + qmp_marshal_%(c_name)s, %(opts)s,
> + NULL);
> + ''',
> + name=name, c_name=c_name(name),
> + opts=options)
> + else:
> + ret = mcgen('''
> + static const RunState qmp_valid_states_%(c_name)s[] = {
> +'''
> + , c_name=c_name(name))
> +
> + for value in runstates:
> + ret += mcgen('''
> + %(c_enum)s,
> +'''
> + , c_enum=c_enum_const('RunState', value))
> +
> + ret += mcgen('''
> + %(c_enum)s,
> + };
> +'''
> + , c_enum=c_enum_const('RunState', "_MAX"))
> +
> + ret += mcgen('''
> + qmp_register_command(cmds, "%(name)s",
> + qmp_marshal_%(c_name)s, %(opts)s,
> + qmp_valid_states_%(c_name)s);
> + ''',
> + name=name, c_name=c_name(name),
> + opts=options)
> +
> return ret
>
>
> @@ -241,7 +269,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
> self._visited_ret_types = None
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> + gen, success_response, boxed, runstates):
> if not gen:
> return
> self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
> @@ -250,7 +278,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
> self.defn += gen_marshal_output(ret_type)
> self.decl += gen_marshal_decl(name)
> self.defn += gen_marshal(name, arg_type, boxed, ret_type)
> - self._regy += gen_register_command(name, success_response)
> + self._regy += gen_register_command(name, success_response, runstates)
>
>
> (input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line()
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 032bcea..ede86ac 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -154,7 +154,7 @@ const char %(c_name)s[] = %(c_string)s;
> for m in variants.variants]})
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> + gen, success_response, boxed, runstates):
> arg_type = arg_type or self._schema.the_empty_object_type
> ret_type = ret_type or self._schema.the_empty_object_type
> self._gen_json(name, 'command',
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 58f995b..5c5037e 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -919,7 +919,8 @@ def check_exprs(exprs):
> elif 'command' in expr:
> meta = 'command'
> check_keys(expr_elem, 'command', [],
> - ['data', 'returns', 'gen', 'success-response', 'boxed'])
> + ['data', 'returns', 'gen', 'success-response', 'boxed',
> + 'runstates'])
> elif 'event' in expr:
> meta = 'event'
> check_keys(expr_elem, 'event', [], ['data', 'boxed'])
> @@ -1030,7 +1031,7 @@ class QAPISchemaVisitor(object):
> pass
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> + gen, success_response, boxed, runstates):
> pass
>
> def visit_event(self, name, info, arg_type, boxed):
> @@ -1397,7 +1398,7 @@ class QAPISchemaAlternateType(QAPISchemaType):
>
> class QAPISchemaCommand(QAPISchemaEntity):
> def __init__(self, name, info, doc, arg_type, ret_type,
> - gen, success_response, boxed):
> + gen, success_response, boxed, runstates):
> QAPISchemaEntity.__init__(self, name, info, doc)
> assert not arg_type or isinstance(arg_type, str)
> assert not ret_type or isinstance(ret_type, str)
> @@ -1408,6 +1409,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
> self.gen = gen
> self.success_response = success_response
> self.boxed = boxed
> + self.runstates = runstates
>
> def check(self, schema):
> if self._arg_type_name:
> @@ -1431,7 +1433,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
> def visit(self, visitor):
> visitor.visit_command(self.name, self.info,
> self.arg_type, self.ret_type,
> - self.gen, self.success_response, self.boxed)
> + self.gen, self.success_response, self.boxed,
> + self.runstates)
>
>
> class QAPISchemaEvent(QAPISchemaEntity):
> @@ -1639,6 +1642,7 @@ class QAPISchema(object):
> gen = expr.get('gen', True)
> success_response = expr.get('success-response', True)
> boxed = expr.get('boxed', False)
> + runstates = expr.get('runstates')
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(
> name, info, doc, 'arg', self._make_members(data, info))
> @@ -1646,7 +1650,8 @@ class QAPISchema(object):
> assert len(rets) == 1
> rets = self._make_array_type(rets[0], info)
> self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
> - gen, success_response, boxed))
> + gen, success_response, boxed,
> + runstates))
>
> def _def_event(self, expr, info, doc):
> name = expr['event']
> diff --git a/scripts/qapi2texi.py b/scripts/qapi2texi.py
> index bf1c57b..d28594c 100755
> --- a/scripts/qapi2texi.py
> +++ b/scripts/qapi2texi.py
> @@ -227,7 +227,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor):
> body=texi_entity(doc, 'Members'))
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> + gen, success_response, boxed, runstates):
> doc = self.cur_doc
> if boxed:
> body = texi_body(doc)
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 1d2c250..a47d6f6 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -18,9 +18,9 @@ object Variant1
> member var1: str optional=False
> object Variant2
> command cmd q_obj_cmd-arg -> Object
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> command cmd-boxed Object -> None
> - gen=True success_response=True boxed=True
> + gen=True success_response=True boxed=True, runstates=None
> object q_empty
> object q_obj_Variant1-wrapper
> member data: Variant1 optional=False
> diff --git a/tests/qapi-schema/ident-with-escape.out b/tests/qapi-schema/ident-with-escape.out
> index b5637cb..d9a272b 100644
> --- a/tests/qapi-schema/ident-with-escape.out
> +++ b/tests/qapi-schema/ident-with-escape.out
> @@ -1,7 +1,7 @@
> enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
> prefix QTYPE
> command fooA q_obj_fooA-arg -> None
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> object q_empty
> object q_obj_fooA-arg
> member bar1: str optional=False
> diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out
> index 586795f..4b128d5 100644
> --- a/tests/qapi-schema/indented-expr.out
> +++ b/tests/qapi-schema/indented-expr.out
> @@ -1,7 +1,7 @@
> enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool']
> prefix QTYPE
> command eins None -> None
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> object q_empty
> command zwei None -> None
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
> index 3b1e908..787c228 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -153,15 +153,15 @@ object __org.qemu_x-Union2
> tag __org.qemu_x-member1
> case __org.qemu_x-value: __org.qemu_x-Struct2
> command __org.qemu_x-command q_obj___org.qemu_x-command-arg -> __org.qemu_x-Union1
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> command boxed-struct UserDefZero -> None
> - gen=True success_response=True boxed=True
> + gen=True success_response=True boxed=True, runstates=None
> command boxed-union UserDefNativeListUnion -> None
> - gen=True success_response=True boxed=True
> + gen=True success_response=True boxed=True, runstates=None
> command guest-get-time q_obj_guest-get-time-arg -> int
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> command guest-sync q_obj_guest-sync-arg -> any
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> object q_empty
> object q_obj_EVENT_C-arg
> member a: int optional=True
> @@ -222,10 +222,10 @@ object q_obj_user_def_cmd2-arg
> member ud1a: UserDefOne optional=False
> member ud1b: UserDefOne optional=True
> command user_def_cmd None -> None
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> command user_def_cmd0 Empty2 -> Empty2
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> command user_def_cmd1 q_obj_user_def_cmd1-arg -> None
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
> - gen=True success_response=True boxed=False
> + gen=True success_response=True boxed=False, runstates=None
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index ac43d34..958576d 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -37,11 +37,11 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
> self._print_variants(variants)
>
> def visit_command(self, name, info, arg_type, ret_type,
> - gen, success_response, boxed):
> + gen, success_response, boxed, runstates):
> 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' % \
> - (gen, success_response, boxed))
> + print(' gen=%s success_response=%s boxed=%s, runstates=%s' % \
> + (gen, success_response, boxed, runstates))
>
> def visit_event(self, name, info, arg_type, boxed):
> print('event %s %s' % (name, arg_type and arg_type.name))
> --
> 2.7.4
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/9] HMP: disable monitor in preconfig state
2018-03-07 14:01 ` Dr. David Alan Gilbert
@ 2018-03-08 15:47 ` Igor Mammedov
0 siblings, 0 replies; 23+ messages in thread
From: Igor Mammedov @ 2018-03-08 15:47 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, peter.maydell, pkrempa, ehabkost, cohuck, armbru,
pbonzini, david
On Wed, 7 Mar 2018 14:01:27 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Igor Mammedov (imammedo@redhat.com) wrote:
> > Ban it for now, if someone would need it to work early,
> > one would have to implement checks if HMP command is valid
> > at preconfig state.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > ---
> > monitor.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/monitor.c b/monitor.c
> > index f499250..fcb5386 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3097,6 +3097,10 @@ 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 precofig state\n");
> > + }
>
> But you've not returned, so the command will still get parsed?
yep, it's bug. I've already fixed in v4.
Plan to post simplified v4 this Friday if nothing goes south.
>
> Dave
>
> > cmd = monitor_parse_command(mon, cmdline, &cmdline, mon->cmd_table);
> > if (!cmd) {
> > return;
> > --
> > 2.7.4
> >
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command
2018-03-07 14:16 ` Dr. David Alan Gilbert
@ 2018-03-08 15:55 ` Igor Mammedov
0 siblings, 0 replies; 23+ messages in thread
From: Igor Mammedov @ 2018-03-08 15:55 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, peter.maydell, pkrempa, ehabkost, cohuck, armbru,
pbonzini, david
On Wed, 7 Mar 2018 14:16:50 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Igor Mammedov (imammedo@redhat.com) wrote:
> > Add optional 'runstates' parameter in QAPI command definition,
> > which will permit to specify RunState variations in which
> > a command could be exectuted via QMP monitor.
> >
> > For compatibility reasons, commands, that don't use
> > 'runstates' explicitly, are not permited to run in
> > preconfig state but allowed in all other states.
> >
> > New option will be used to allow commands, which are
> > prepared/need to run this early, to run in preconfig state.
> > It will include query-hotpluggable-cpus and new set-numa-node
> > commands. Other commands that should be able to run in
> > preconfig state, should be ammeded to not expect machine
> > in initialized state.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
[...]
> > @@ -224,6 +224,25 @@ static mon_cmd_t mon_cmds[];
> > static mon_cmd_t info_cmds[];
> >
> > QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
> > +const RunState cap_negotiation_valid_runstates[] = {
> > + RUN_STATE_DEBUG,
> > + RUN_STATE_INMIGRATE,
> > + RUN_STATE_INTERNAL_ERROR,
> > + RUN_STATE_IO_ERROR,
> > + RUN_STATE_PAUSED,
> > + RUN_STATE_POSTMIGRATE,
> > + RUN_STATE_PRELAUNCH,
> > + RUN_STATE_FINISH_MIGRATE,
> > + RUN_STATE_RESTORE_VM,
> > + RUN_STATE_RUNNING,
> > + RUN_STATE_SAVE_VM,
> > + RUN_STATE_SHUTDOWN,
> > + RUN_STATE_SUSPENDED,
> > + RUN_STATE_WATCHDOG,
> > + RUN_STATE_GUEST_PANICKED,
> > + RUN_STATE_COLO,
> > + RUN_STATE_PRECONFIG,
> > +};
>
> It's a shame this is such a manual copy.
>
> While you're taking a big hammer to HMP in the preconfig case,
> it's worth noting this more specific code is only protecting QMP
> commands.
>
>
> It's a shame in all the uses below we can't be working with bitmasks
> of run-state's; it would feel a lot easier to have a default and mask
> out or or in extra states.
>
> Dave
[...]
> > @@ -219,7 +219,11 @@
> > # Note: This example has been shortened as the real response is too long.
> > #
> > ##
> > -{ 'command': 'query-commands', 'returns': ['CommandInfo'] }
> > +{ 'command': 'query-commands', 'returns': ['CommandInfo'],
> > + 'runstates': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> > + 'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> > + 'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> > + 'guest-panicked', 'colo', 'preconfig' ] }
>
> That's going to get to be a pain to update as we add more states.
[...]
Yep it would be a pain so,
In v4 this approach/patch is replaced by a simpler one that Eric's suggested.
It will provide preconfig specific flag in command, similar to what allow-oob
patches on list do. So it would look like following:
------------------------ docs/devel/qapi-code-gen.txt -------------------------
index 25b7180..170f15f 100644
@@ -556,7 +556,8 @@ following example objects:
Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT,
'*returns': TYPE-NAME, '*boxed': true,
- '*gen': false, '*success-response': false }
+ '*gen': false, '*success-response': false,
+ '*allowed-in-preconfig': true }
Commands are defined by using a dictionary containing several members,
where three members are most common. The 'command' member is a
@@ -636,6 +637,13 @@ possible, the command expression should include the optional key
'success-response' with boolean value false. So far, only QGA makes
use of this member.
+A command may use optional 'allowed-in-preconfig' key to permit
+its execution at early runtime configuration stage (preconfig runstate).
+If not specified then a command defaults to 'allowed-in-preconfig: false'.
+
+An example of declaring preconfig enabled command:
+ { 'command': 'qmp_capabilities',
+ 'allowed-in-preconfig': true }
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-03-08 15:55 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-16 12:37 [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 1/9] numa: postpone options post-processing till machine_run_board_init() Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 2/9] numa: split out NumaOptions parsing into parse_NumaOptions() Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 3/9] CLI: add -preconfig option Igor Mammedov
2018-02-27 20:39 ` Eric Blake
2018-02-28 15:54 ` Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 4/9] HMP: disable monitor in preconfig state Igor Mammedov
2018-03-07 14:01 ` Dr. David Alan Gilbert
2018-03-08 15:47 ` Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 5/9] QAPI: allow to specify valid runstates per command Igor Mammedov
2018-02-27 22:10 ` Eric Blake
2018-02-28 16:17 ` Igor Mammedov
2018-03-07 14:16 ` Dr. David Alan Gilbert
2018-03-08 15:55 ` Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 6/9] tests: extend qmp test with pereconfig checks Igor Mammedov
2018-02-27 22:13 ` Eric Blake
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 7/9] QMP: permit query-hotpluggable-cpus in preconfig state Igor Mammedov
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 8/9] QMP: add set-numa-node command Igor Mammedov
2018-02-27 22:17 ` Eric Blake
2018-02-16 12:37 ` [Qemu-devel] [PATCH v3 9/9] tests: functional tests for QMP command set-numa-node Igor Mammedov
2018-02-27 22:19 ` Eric Blake
2018-02-27 16:36 ` [Qemu-devel] [PATCH v3 0/9] enable numa configuration before machine_init() from QMP Igor Mammedov
2018-02-27 20:29 ` Eric Blake
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.