[RFC PATCH 0/9] Initial support for machine creation via QMP

[RFC PATCH 0/9] Initial support for machine creation via QMP

[Mirela Grujic]

The direction for this work has been set in the discussion thread:
"About creating machines on the command line" in January/February 2021:
https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg01839.html
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01070.html

To customize a machine via QMP we need the ability to stop QEMU at a specific
machine initialization phase.

Currently, machine initialization phases are:
1) no-machine: machine does not exist yet (current_machine == NULL)
2) machine-created: machine exists, but its accelerator does not
   (current_machine->accelerator == NULL)
3) accel-created: machine's accelerator is configured
   (current_machine->accelerator != NULL), but machine class's init() has not
   been called (no properties validated, machine_init_done notifiers not
   registered, no sysbus, etc.)
4) initialized: machine class's init() has been called, thus machine properties
   are validated, machine_init_done notifiers registered, sysbus realized, etc.
   Devices added at this phase are considered to be cold-plugged.
5) ready: machine_init_done notifiers are called, then QEMU is ready to start
   CPUs. Devices added at this phase are considered to be hot-plugged.

QEMU can be stopped today using the -preconfig CLI option at phase 3
(accel-created). This option was introduced to enable the QMP configuration of
parameters that affect the machine initialization. We cannot add devices at
this point because the machine class's init() has not been called, thus sysbus
does not exist yet (a device cannot be added because there is no bus to attach
it to).

QEMU can be also stopped using the -S CLI option at the machine ready phase.
However, it is too late to add devices at this phase because the machine is
already configured, and any devices added at this point are considered to be
hot-plugged.

Since the existing -preconfig CLI option stops QEMU too early, and the -S option
stops too late, we need a way to stop QEMU in between (after the machine is
initialized and before it becomes ready).

We propose to add QMP commands to step through machine phases starting from the
initial CLI's '-preconfig' early stop point. With this addition, we may now
execute QMP commands at any machine phase.

The 'next-machine-phase' command would trigger QEMU to execute initialization
steps that are needed to enter the next phase. If it's more convenient to jump
to an initialization phase than to single-step through phases, the
'advance-machine-phase' command should be used instead of 'next-machine-phase'.
Additionally, we propose to add the command to query the current machine phase,
namely 'query-machine-phase'.

With this patch it would be possible to add devices via QMP. For example,
by running QEMU with:
$ qemu-system-riscv32 \
  -M sifive_dt \
  -qmp unix:./qmp-sock,server \
  -preconfig \
  ...

and scripts/qmp/qmp-shell as the QMP client:
$ qemu/scripts/qmp/qmp-shell ./qmp-sock
Welcome to the QMP low-level shell!
Connected to QEMU 6.0.0

(QEMU) query-machine-phase
{"return": {"phase": "accel-created"}}
(QEMU) next-machine-phase
{"return": {}}
(QEMU) query-machine-phase
{"return": {"phase": "initialized"}}
(QEMU) device_add driver=...
{"return": {}}
(QEMU) next-machine-phase
{"return": {}}
(QEMU) query-machine-phase
{"return": {"phase": "ready"}}

Note that with the introduced changes, devices can still be added via CLI, i.e.
we support a mixed configuration approach (CLI, QMP, or CLI/QMP). Any device
specified via CLI will be added before QEMU waits for the QMP configuration
in the machine 'initialized' phase.

Mirela Grujic (9):
  vl: Allow finer control in advancing machine through phases
  replace machine phase_check with machine_is_initialized/ready calls
  rename MachineInitPhase enumeration constants
  qapi: Implement 'query-machine-phase' command
  qapi: Implement 'next-machine-phase' command
  qapi: Implement 'advance-machine-phase' command
  qdev-monitor: Restructure and fix the check for command availability
  qapi: Introduce 'allow-init-config' option
  qapi: Allow some commands to be executed in machine 'initialized'
    phase

 docs/sphinx/qapidoc.py      |   2 +-
 qapi/machine.json           | 105 ++++++++++++++++++++++++++++++++++++
 qapi/qdev.json              |   3 +-
 include/hw/qdev-core.h      |  32 ++---------
 include/qapi/qmp/dispatch.h |   1 +
 include/sysemu/sysemu.h     |   3 ++
 hw/core/machine-qmp-cmds.c  |  33 +++++++++++-
 hw/core/machine.c           |   6 +--
 hw/core/qdev.c              |  17 +++++-
 hw/pci/pci.c                |   2 +-
 hw/usb/core.c               |   2 +-
 hw/virtio/virtio-iommu.c    |   2 +-
 monitor/hmp.c               |   2 +-
 monitor/misc.c              |   2 +-
 softmmu/qdev-monitor.c      |  31 +++++++----
 softmmu/vl.c                |  94 ++++++++++++++++++++------------
 ui/console.c                |   2 +-
 scripts/qapi/commands.py    |  10 ++--
 scripts/qapi/expr.py        |   5 +-
 scripts/qapi/introspect.py  |   3 +-
 scripts/qapi/schema.py      |  10 ++--
 21 files changed, 274 insertions(+), 93 deletions(-)

-- 
2.25.1

[RFC PATCH 1/9] vl: Allow finer control in advancing machine through phases

[Mirela Grujic]

In addition to the existing preconfig approach, which allows configuring
the machine via QMP before the machine is initialized, we need the ability
to configure the machine right after it's initialized. This patch will
enable doing that in a scalable fashion as follows.

The basic idea is to group machine initialization steps into chunks
that need to be executed to advance machine initialization from one
phase to the next one. Then, we add a finer control to allow executing
only a chunk, i.e. to advance machine initialization to the next phase.
Between the phases, we would configure the machine properties via QMP,
add devices, etc. (depends on the phase). Advancing the machine to the
next initialization phase would also be controllable via a QMP command.

For example, the machine configuration could look like this:
1) Run QEMU with existing -preconfig command line option (this will
   cause QEMU to wait before the machine is initialized)
Then, using QMP commands from a client:
2) Perform configuration that needs to be done before the machine
   is initialized
3) Advance machine initialization phase to the next (the
   'next-machine-phase' command to be added)
4) Perform configuration that needs to be done after the machine
   is initialized
5) 'next-machine-phase' (same as in 3), etc.

Available QMP commands will be specific to each phase, but the command
'next-machine-phase' will be common. In the implementation of the
'next-machine-phase' command, we will call qemu_machine_enter_phase
function introduced in this patch.

Function qemu_machine_enter_phase introduced here allows advancing
the machine to the target phase. For the single-stepping configuration
described above, the target phase would be the current machine
phase + 1 (assuming some sanity checks on top).
When this function is called for the PHASE_MACHINE_READY target phase
(the final phase) it implements the same functionality as
qmp_x_exit_preconfig before this patch. In other words, the body of
this function implements the same initialization steps as before, but
just groups them into chunks (if branches) and allows stepping through
machine init phases.

For now, the relevant target machine phases are only
PHASE_MACHINE_INITIALIZED and PHASE_MACHINE_READY, but the approach
allows to easily add more or split existing phases if needed.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 include/hw/qdev-core.h |  1 +
 hw/core/qdev.c         |  5 +++
 softmmu/vl.c           | 87 ++++++++++++++++++++++++++++--------------
 3 files changed, 64 insertions(+), 29 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bafc311bfa..6e52240d92 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -841,5 +841,6 @@ typedef enum MachineInitPhase {
 
 extern bool phase_check(MachineInitPhase phase);
 extern void phase_advance(MachineInitPhase phase);
+extern MachineInitPhase phase_get(void);
 
 #endif
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a..4a4a4d8c52 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1150,6 +1150,11 @@ void phase_advance(MachineInitPhase phase)
     machine_phase = phase;
 }
 
+MachineInitPhase phase_get(void)
+{
+    return machine_phase;
+}
+
 static const TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
diff --git a/softmmu/vl.c b/softmmu/vl.c
index aadb526138..cbf62abeb4 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2578,6 +2578,62 @@ static void qemu_machine_creation_done(void)
     }
 }
 
+static void qemu_machine_enter_phase(MachineInitPhase target_phase,
+                                     Error **errp)
+{
+    /* target phases before initialization are not handled here */
+    if (target_phase < PHASE_MACHINE_INITIALIZED) {
+        error_setg(errp, "Target machine phase too early to enter this way");
+        return;
+    }
+
+    /* check if machine has already passed through the target phase */
+    if (phase_check(target_phase)) {
+        error_setg(errp, "Target machine phase already entered");
+        return;
+    }
+
+    /*
+     * if machine has not yet passed 'initialized' phase and according to the
+     * target_phase it should
+     */
+    if (target_phase >= PHASE_MACHINE_INITIALIZED &&
+        phase_get() < PHASE_MACHINE_INITIALIZED) {
+        qemu_init_board();
+        qemu_create_cli_devices();
+    }
+
+    if (target_phase >= PHASE_MACHINE_READY &&
+        phase_get() < PHASE_MACHINE_READY) {
+        qemu_machine_creation_done();
+
+        if (loadvm) {
+            Error *local_err = NULL;
+            if (!load_snapshot(loadvm, NULL, false, NULL, &local_err)) {
+                error_report_err(local_err);
+                autostart = 0;
+                exit(1);
+            }
+        }
+        if (replay_mode != REPLAY_MODE_NONE) {
+            replay_vmstate_init();
+        }
+
+        if (incoming) {
+            Error *local_err = NULL;
+            if (strcmp(incoming, "defer") != 0) {
+                qmp_migrate_incoming(incoming, &local_err);
+                if (local_err) {
+                    error_reportf_err(local_err, "-incoming %s: ", incoming);
+                    exit(1);
+                }
+            }
+        } else if (autostart) {
+            qmp_cont(NULL);
+        }
+    }
+}
+
 void qmp_x_exit_preconfig(Error **errp)
 {
     if (phase_check(PHASE_MACHINE_INITIALIZED)) {
@@ -2585,34 +2641,7 @@ void qmp_x_exit_preconfig(Error **errp)
         return;
     }
 
-    qemu_init_board();
-    qemu_create_cli_devices();
-    qemu_machine_creation_done();
-
-    if (loadvm) {
-        Error *local_err = NULL;
-        if (!load_snapshot(loadvm, NULL, false, NULL, &local_err)) {
-            error_report_err(local_err);
-            autostart = 0;
-            exit(1);
-        }
-    }
-    if (replay_mode != REPLAY_MODE_NONE) {
-        replay_vmstate_init();
-    }
-
-    if (incoming) {
-        Error *local_err = NULL;
-        if (strcmp(incoming, "defer") != 0) {
-            qmp_migrate_incoming(incoming, &local_err);
-            if (local_err) {
-                error_reportf_err(local_err, "-incoming %s: ", incoming);
-                exit(1);
-            }
-        }
-    } else if (autostart) {
-        qmp_cont(NULL);
-    }
+    qemu_machine_enter_phase(PHASE_MACHINE_READY, errp);
 }
 
 void qemu_init(int argc, char **argv, char **envp)
@@ -3608,7 +3637,7 @@ void qemu_init(int argc, char **argv, char **envp)
     }
 
     if (!preconfig_requested) {
-        qmp_x_exit_preconfig(&error_fatal);
+        qemu_machine_enter_phase(PHASE_MACHINE_READY, &error_fatal);
     }
     qemu_init_displays();
     accel_setup_post(current_machine);
-- 
2.25.1

[RFC PATCH 2/9] replace machine phase_check with machine_is_initialized/ready calls

[Mirela Grujic]

Once we define MachineInitPhase in qapi, the generated enumeration
constants will be prefixed with the MACHINE_INIT_PHASE_.
We need to define the MachineInitPhase enum in qapi in order to
add the QMP command that will query current machine init phase.

Since in the existing definition of enum MachineInitPhase the
enumeration constants are prefixed with PHASE_, there will be a
massive find/replace to rename the existing enum constants.
We'll do this in 2 phases:
1) hide explicit use of PHASE_ prefixed enums by replacing
    phase_check(PHASE_MACHINE_INITIALIZED) -> machine_is_initialized()
    phase_check(PHASE_MACHINE_READY) -> machine_is_ready()
2) rename enums

Once 1) and 2) are done MachineInitPhase enum will be generated.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 include/hw/qdev-core.h     |  2 ++
 hw/core/machine-qmp-cmds.c |  2 +-
 hw/core/machine.c          |  2 +-
 hw/core/qdev.c             | 12 +++++++++++-
 hw/pci/pci.c               |  2 +-
 hw/usb/core.c              |  2 +-
 hw/virtio/virtio-iommu.c   |  2 +-
 monitor/hmp.c              |  2 +-
 softmmu/qdev-monitor.c     |  9 ++++-----
 softmmu/vl.c               |  2 +-
 ui/console.c               |  2 +-
 11 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 6e52240d92..5e3c6d4482 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -842,5 +842,7 @@ typedef enum MachineInitPhase {
 extern bool phase_check(MachineInitPhase phase);
 extern void phase_advance(MachineInitPhase phase);
 extern MachineInitPhase phase_get(void);
+extern bool machine_is_initialized(void);
+extern bool machine_is_ready(void);
 
 #endif
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 68a942595a..be286882cc 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -149,7 +149,7 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
 
 void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
 {
-    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
+    if (machine_is_initialized()) {
         error_setg(errp, "The command is permitted only before the machine has been created");
         return;
     }
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 40def78183..eba046924d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1239,7 +1239,7 @@ static NotifierList machine_init_done_notifiers =
 void qemu_add_machine_init_done_notifier(Notifier *notify)
 {
     notifier_list_add(&machine_init_done_notifiers, notify);
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (machine_is_ready()) {
         notify->notify(notify, NULL);
     }
 }
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 4a4a4d8c52..71906170f9 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -904,7 +904,7 @@ static void device_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
 
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (machine_is_ready()) {
         dev->hotplugged = 1;
         qdev_hot_added = true;
     }
@@ -1155,6 +1155,16 @@ MachineInitPhase phase_get(void)
     return machine_phase;
 }
 
+bool machine_is_initialized(void)
+{
+    return machine_phase >= PHASE_MACHINE_INITIALIZED;
+}
+
+bool machine_is_ready(void)
+{
+    return machine_phase >= PHASE_MACHINE_READY;
+}
+
 static const TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8f35e13a0c..19b584c3d1 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1071,7 +1071,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
     address_space_init(&pci_dev->bus_master_as,
                        &pci_dev->bus_master_container_region, pci_dev->name);
 
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (machine_is_ready()) {
         pci_init_bus_master(pci_dev);
     }
     pci_dev->irq_state = 0;
diff --git a/hw/usb/core.c b/hw/usb/core.c
index 975f76250a..2ec0dea6a0 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -97,7 +97,7 @@ void usb_wakeup(USBEndpoint *ep, unsigned int stream)
     USBDevice *dev = ep->dev;
     USBBus *bus = usb_bus_from_device(dev);
 
-    if (!phase_check(PHASE_MACHINE_READY)) {
+    if (!machine_is_ready()) {
         /*
          * This is machine init cold plug.  No need to wakeup anyone,
          * all devices will be reset anyway.  And trying to wakeup can
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 1b23e8e18c..8b1bcb2848 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -948,7 +948,7 @@ static int virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
      * accept it. Having a different masks is possible but the guest will use
      * sub-optimal block sizes, so warn about it.
      */
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (machine_is_ready()) {
         int new_granule = ctz64(new_mask);
         int cur_granule = ctz64(cur_mask);
 
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 6c0b33a0b1..c24511db6d 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -216,7 +216,7 @@ static bool cmd_can_preconfig(const HMPCommand *cmd)
 
 static bool cmd_available(const HMPCommand *cmd)
 {
-    return phase_check(PHASE_MACHINE_READY) || cmd_can_preconfig(cmd);
+    return machine_is_ready() || cmd_can_preconfig(cmd);
 }
 
 static void help_cmd_dump_one(Monitor *mon,
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index a9955b97a0..be8a892517 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -254,7 +254,7 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
 
     dc = DEVICE_CLASS(oc);
     if (!dc->user_creatable ||
-        (phase_check(PHASE_MACHINE_READY) && !dc->hotpluggable)) {
+        (machine_is_ready() && !dc->hotpluggable)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
                    "a pluggable device type");
         return NULL;
@@ -636,7 +636,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
         }
     }
 
-    if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus)) {
+    if (machine_is_ready() && bus && !qbus_is_hotpluggable(bus)) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
         return NULL;
     }
@@ -650,7 +650,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
     dev = qdev_new(driver);
 
     /* Check whether the hotplug is allowed by the machine */
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (machine_is_ready()) {
         if (!qdev_hotplug_allowed(dev, errp)) {
             goto err_del_dev;
         }
@@ -998,8 +998,7 @@ int qemu_global_option(const char *str)
 
 bool qmp_command_available(const QmpCommand *cmd, Error **errp)
 {
-    if (!phase_check(PHASE_MACHINE_READY) &&
-        !(cmd->options & QCO_ALLOW_PRECONFIG)) {
+    if (!machine_is_ready() && !(cmd->options & QCO_ALLOW_PRECONFIG)) {
         error_setg(errp, "The command '%s' is permitted only after machine initialization has completed",
                    cmd->name);
         return false;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index cbf62abeb4..3af9743ceb 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2636,7 +2636,7 @@ static void qemu_machine_enter_phase(MachineInitPhase target_phase,
 
 void qmp_x_exit_preconfig(Error **errp)
 {
-    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
+    if (machine_is_initialized()) {
         error_setg(errp, "The command is permitted only before machine initialization");
         return;
     }
diff --git a/ui/console.c b/ui/console.c
index 2de5f4105b..3513da6a54 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1353,7 +1353,7 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
     if (QTAILQ_EMPTY(&consoles)) {
         s->index = 0;
         QTAILQ_INSERT_TAIL(&consoles, s, next);
-    } else if (console_type != GRAPHIC_CONSOLE || phase_check(PHASE_MACHINE_READY)) {
+    } else if (console_type != GRAPHIC_CONSOLE || machine_is_ready()) {
         QemuConsole *last = QTAILQ_LAST(&consoles);
         s->index = last->index + 1;
         QTAILQ_INSERT_TAIL(&consoles, s, next);
-- 
2.25.1

Re: [RFC PATCH 2/9] replace machine phase_check with machine_is_initialized/ready calls

[Paolo Bonzini]

On 13/05/21 10:25, Mirela Grujic wrote:
> Once we define MachineInitPhase in qapi, the generated enumeration
> constants will be prefixed with the MACHINE_INIT_PHASE_.
> We need to define the MachineInitPhase enum in qapi in order to
> add the QMP command that will query current machine init phase.
> 
> Since in the existing definition of enum MachineInitPhase the
> enumeration constants are prefixed with PHASE_, there will be a
> massive find/replace to rename the existing enum constants.
> We'll do this in 2 phases:
> 1) hide explicit use of PHASE_ prefixed enums by replacing
>      phase_check(PHASE_MACHINE_INITIALIZED) -> machine_is_initialized()
>      phase_check(PHASE_MACHINE_READY) -> machine_is_ready()
> 2) rename enums
> 
> Once 1) and 2) are done MachineInitPhase enum will be generated.

Is it so much churn to just rename everything to MACHINE_INIT_PHASE_* 
and keep phase_check() as is?  Or is it because the QAPI-generated names 
are quite long?

Paolo

> Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
> ---
>   include/hw/qdev-core.h     |  2 ++
>   hw/core/machine-qmp-cmds.c |  2 +-
>   hw/core/machine.c          |  2 +-
>   hw/core/qdev.c             | 12 +++++++++++-
>   hw/pci/pci.c               |  2 +-
>   hw/usb/core.c              |  2 +-
>   hw/virtio/virtio-iommu.c   |  2 +-
>   monitor/hmp.c              |  2 +-
>   softmmu/qdev-monitor.c     |  9 ++++-----
>   softmmu/vl.c               |  2 +-
>   ui/console.c               |  2 +-
>   11 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 6e52240d92..5e3c6d4482 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -842,5 +842,7 @@ typedef enum MachineInitPhase {
>   extern bool phase_check(MachineInitPhase phase);
>   extern void phase_advance(MachineInitPhase phase);
>   extern MachineInitPhase phase_get(void);
> +extern bool machine_is_initialized(void);
> +extern bool machine_is_ready(void);
>   
>   #endif
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 68a942595a..be286882cc 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -149,7 +149,7 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
>   
>   void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
>   {
> -    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
> +    if (machine_is_initialized()) {
>           error_setg(errp, "The command is permitted only before the machine has been created");
>           return;
>       }
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 40def78183..eba046924d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1239,7 +1239,7 @@ static NotifierList machine_init_done_notifiers =
>   void qemu_add_machine_init_done_notifier(Notifier *notify)
>   {
>       notifier_list_add(&machine_init_done_notifiers, notify);
> -    if (phase_check(PHASE_MACHINE_READY)) {
> +    if (machine_is_ready()) {
>           notify->notify(notify, NULL);
>       }
>   }
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 4a4a4d8c52..71906170f9 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -904,7 +904,7 @@ static void device_initfn(Object *obj)
>   {
>       DeviceState *dev = DEVICE(obj);
>   
> -    if (phase_check(PHASE_MACHINE_READY)) {
> +    if (machine_is_ready()) {
>           dev->hotplugged = 1;
>           qdev_hot_added = true;
>       }
> @@ -1155,6 +1155,16 @@ MachineInitPhase phase_get(void)
>       return machine_phase;
>   }
>   
> +bool machine_is_initialized(void)
> +{
> +    return machine_phase >= PHASE_MACHINE_INITIALIZED;
> +}
> +
> +bool machine_is_ready(void)
> +{
> +    return machine_phase >= PHASE_MACHINE_READY;
> +}
> +
>   static const TypeInfo device_type_info = {
>       .name = TYPE_DEVICE,
>       .parent = TYPE_OBJECT,
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 8f35e13a0c..19b584c3d1 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1071,7 +1071,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
>       address_space_init(&pci_dev->bus_master_as,
>                          &pci_dev->bus_master_container_region, pci_dev->name);
>   
> -    if (phase_check(PHASE_MACHINE_READY)) {
> +    if (machine_is_ready()) {
>           pci_init_bus_master(pci_dev);
>       }
>       pci_dev->irq_state = 0;
> diff --git a/hw/usb/core.c b/hw/usb/core.c
> index 975f76250a..2ec0dea6a0 100644
> --- a/hw/usb/core.c
> +++ b/hw/usb/core.c
> @@ -97,7 +97,7 @@ void usb_wakeup(USBEndpoint *ep, unsigned int stream)
>       USBDevice *dev = ep->dev;
>       USBBus *bus = usb_bus_from_device(dev);
>   
> -    if (!phase_check(PHASE_MACHINE_READY)) {
> +    if (!machine_is_ready()) {
>           /*
>            * This is machine init cold plug.  No need to wakeup anyone,
>            * all devices will be reset anyway.  And trying to wakeup can
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 1b23e8e18c..8b1bcb2848 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -948,7 +948,7 @@ static int virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
>        * accept it. Having a different masks is possible but the guest will use
>        * sub-optimal block sizes, so warn about it.
>        */
> -    if (phase_check(PHASE_MACHINE_READY)) {
> +    if (machine_is_ready()) {
>           int new_granule = ctz64(new_mask);
>           int cur_granule = ctz64(cur_mask);
>   
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 6c0b33a0b1..c24511db6d 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -216,7 +216,7 @@ static bool cmd_can_preconfig(const HMPCommand *cmd)
>   
>   static bool cmd_available(const HMPCommand *cmd)
>   {
> -    return phase_check(PHASE_MACHINE_READY) || cmd_can_preconfig(cmd);
> +    return machine_is_ready() || cmd_can_preconfig(cmd);
>   }
>   
>   static void help_cmd_dump_one(Monitor *mon,
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index a9955b97a0..be8a892517 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -254,7 +254,7 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>   
>       dc = DEVICE_CLASS(oc);
>       if (!dc->user_creatable ||
> -        (phase_check(PHASE_MACHINE_READY) && !dc->hotpluggable)) {
> +        (machine_is_ready() && !dc->hotpluggable)) {
>           error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
>                      "a pluggable device type");
>           return NULL;
> @@ -636,7 +636,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>           }
>       }
>   
> -    if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus)) {
> +    if (machine_is_ready() && bus && !qbus_is_hotpluggable(bus)) {
>           error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>           return NULL;
>       }
> @@ -650,7 +650,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>       dev = qdev_new(driver);
>   
>       /* Check whether the hotplug is allowed by the machine */
> -    if (phase_check(PHASE_MACHINE_READY)) {
> +    if (machine_is_ready()) {
>           if (!qdev_hotplug_allowed(dev, errp)) {
>               goto err_del_dev;
>           }
> @@ -998,8 +998,7 @@ int qemu_global_option(const char *str)
>   
>   bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>   {
> -    if (!phase_check(PHASE_MACHINE_READY) &&
> -        !(cmd->options & QCO_ALLOW_PRECONFIG)) {
> +    if (!machine_is_ready() && !(cmd->options & QCO_ALLOW_PRECONFIG)) {
>           error_setg(errp, "The command '%s' is permitted only after machine initialization has completed",
>                      cmd->name);
>           return false;
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index cbf62abeb4..3af9743ceb 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2636,7 +2636,7 @@ static void qemu_machine_enter_phase(MachineInitPhase target_phase,
>   
>   void qmp_x_exit_preconfig(Error **errp)
>   {
> -    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
> +    if (machine_is_initialized()) {
>           error_setg(errp, "The command is permitted only before machine initialization");
>           return;
>       }
> diff --git a/ui/console.c b/ui/console.c
> index 2de5f4105b..3513da6a54 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1353,7 +1353,7 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
>       if (QTAILQ_EMPTY(&consoles)) {
>           s->index = 0;
>           QTAILQ_INSERT_TAIL(&consoles, s, next);
> -    } else if (console_type != GRAPHIC_CONSOLE || phase_check(PHASE_MACHINE_READY)) {
> +    } else if (console_type != GRAPHIC_CONSOLE || machine_is_ready()) {
>           QemuConsole *last = QTAILQ_LAST(&consoles);
>           s->index = last->index + 1;
>           QTAILQ_INSERT_TAIL(&consoles, s, next);
> 

Re: [RFC PATCH 2/9] replace machine phase_check with machine_is_initialized/ready calls

[Mirela Grujic]

On 5/13/21 7:46 PM, Paolo Bonzini wrote:
> On 13/05/21 10:25, Mirela Grujic wrote:
>> Once we define MachineInitPhase in qapi, the generated enumeration
>> constants will be prefixed with the MACHINE_INIT_PHASE_.
>> We need to define the MachineInitPhase enum in qapi in order to
>> add the QMP command that will query current machine init phase.
>>
>> Since in the existing definition of enum MachineInitPhase the
>> enumeration constants are prefixed with PHASE_, there will be a
>> massive find/replace to rename the existing enum constants.
>> We'll do this in 2 phases:
>> 1) hide explicit use of PHASE_ prefixed enums by replacing
>>      phase_check(PHASE_MACHINE_INITIALIZED) -> machine_is_initialized()
>>      phase_check(PHASE_MACHINE_READY) -> machine_is_ready()
>> 2) rename enums
>>
>> Once 1) and 2) are done MachineInitPhase enum will be generated.
>
> Is it so much churn to just rename everything to MACHINE_INIT_PHASE_* 
> and keep phase_check() as is?  Or is it because the QAPI-generated 
> names are quite long?
>

It's easier to just rename everything to MACHINE_INIT_PHASE_* :) I 
believed it was better to first hide enums, and thus avoid the need for 
such a massive find&replace in future. The names were also quite long, 
required additional line breaks...

If this is too much churn I can squash commits. When squashed, I don't 
see the big difference between just renaming and this approach, pretty 
much the same lines are modified. I believed it was easier to review 
without squash.


However, if you believe it should rather be just renamed I can do so.


> Paolo
>
>> Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
>> ---
>>   include/hw/qdev-core.h     |  2 ++
>>   hw/core/machine-qmp-cmds.c |  2 +-
>>   hw/core/machine.c          |  2 +-
>>   hw/core/qdev.c             | 12 +++++++++++-
>>   hw/pci/pci.c               |  2 +-
>>   hw/usb/core.c              |  2 +-
>>   hw/virtio/virtio-iommu.c   |  2 +-
>>   monitor/hmp.c              |  2 +-
>>   softmmu/qdev-monitor.c     |  9 ++++-----
>>   softmmu/vl.c               |  2 +-
>>   ui/console.c               |  2 +-
>>   11 files changed, 25 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 6e52240d92..5e3c6d4482 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -842,5 +842,7 @@ typedef enum MachineInitPhase {
>>   extern bool phase_check(MachineInitPhase phase);
>>   extern void phase_advance(MachineInitPhase phase);
>>   extern MachineInitPhase phase_get(void);
>> +extern bool machine_is_initialized(void);
>> +extern bool machine_is_ready(void);
>>     #endif
>> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
>> index 68a942595a..be286882cc 100644
>> --- a/hw/core/machine-qmp-cmds.c
>> +++ b/hw/core/machine-qmp-cmds.c
>> @@ -149,7 +149,7 @@ HotpluggableCPUList 
>> *qmp_query_hotpluggable_cpus(Error **errp)
>>     void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
>>   {
>> -    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
>> +    if (machine_is_initialized()) {
>>           error_setg(errp, "The command is permitted only before the 
>> machine has been created");
>>           return;
>>       }
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 40def78183..eba046924d 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -1239,7 +1239,7 @@ static NotifierList machine_init_done_notifiers =
>>   void qemu_add_machine_init_done_notifier(Notifier *notify)
>>   {
>>       notifier_list_add(&machine_init_done_notifiers, notify);
>> -    if (phase_check(PHASE_MACHINE_READY)) {
>> +    if (machine_is_ready()) {
>>           notify->notify(notify, NULL);
>>       }
>>   }
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index 4a4a4d8c52..71906170f9 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -904,7 +904,7 @@ static void device_initfn(Object *obj)
>>   {
>>       DeviceState *dev = DEVICE(obj);
>>   -    if (phase_check(PHASE_MACHINE_READY)) {
>> +    if (machine_is_ready()) {
>>           dev->hotplugged = 1;
>>           qdev_hot_added = true;
>>       }
>> @@ -1155,6 +1155,16 @@ MachineInitPhase phase_get(void)
>>       return machine_phase;
>>   }
>>   +bool machine_is_initialized(void)
>> +{
>> +    return machine_phase >= PHASE_MACHINE_INITIALIZED;
>> +}
>> +
>> +bool machine_is_ready(void)
>> +{
>> +    return machine_phase >= PHASE_MACHINE_READY;
>> +}
>> +
>>   static const TypeInfo device_type_info = {
>>       .name = TYPE_DEVICE,
>>       .parent = TYPE_OBJECT,
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 8f35e13a0c..19b584c3d1 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -1071,7 +1071,7 @@ static PCIDevice 
>> *do_pci_register_device(PCIDevice *pci_dev,
>>       address_space_init(&pci_dev->bus_master_as,
>> &pci_dev->bus_master_container_region, pci_dev->name);
>>   -    if (phase_check(PHASE_MACHINE_READY)) {
>> +    if (machine_is_ready()) {
>>           pci_init_bus_master(pci_dev);
>>       }
>>       pci_dev->irq_state = 0;
>> diff --git a/hw/usb/core.c b/hw/usb/core.c
>> index 975f76250a..2ec0dea6a0 100644
>> --- a/hw/usb/core.c
>> +++ b/hw/usb/core.c
>> @@ -97,7 +97,7 @@ void usb_wakeup(USBEndpoint *ep, unsigned int stream)
>>       USBDevice *dev = ep->dev;
>>       USBBus *bus = usb_bus_from_device(dev);
>>   -    if (!phase_check(PHASE_MACHINE_READY)) {
>> +    if (!machine_is_ready()) {
>>           /*
>>            * This is machine init cold plug.  No need to wakeup anyone,
>>            * all devices will be reset anyway.  And trying to wakeup can
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 1b23e8e18c..8b1bcb2848 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -948,7 +948,7 @@ static int 
>> virtio_iommu_set_page_size_mask(IOMMUMemoryRegion *mr,
>>        * accept it. Having a different masks is possible but the 
>> guest will use
>>        * sub-optimal block sizes, so warn about it.
>>        */
>> -    if (phase_check(PHASE_MACHINE_READY)) {
>> +    if (machine_is_ready()) {
>>           int new_granule = ctz64(new_mask);
>>           int cur_granule = ctz64(cur_mask);
>>   diff --git a/monitor/hmp.c b/monitor/hmp.c
>> index 6c0b33a0b1..c24511db6d 100644
>> --- a/monitor/hmp.c
>> +++ b/monitor/hmp.c
>> @@ -216,7 +216,7 @@ static bool cmd_can_preconfig(const HMPCommand *cmd)
>>     static bool cmd_available(const HMPCommand *cmd)
>>   {
>> -    return phase_check(PHASE_MACHINE_READY) || cmd_can_preconfig(cmd);
>> +    return machine_is_ready() || cmd_can_preconfig(cmd);
>>   }
>>     static void help_cmd_dump_one(Monitor *mon,
>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>> index a9955b97a0..be8a892517 100644
>> --- a/softmmu/qdev-monitor.c
>> +++ b/softmmu/qdev-monitor.c
>> @@ -254,7 +254,7 @@ static DeviceClass *qdev_get_device_class(const 
>> char **driver, Error **errp)
>>         dc = DEVICE_CLASS(oc);
>>       if (!dc->user_creatable ||
>> -        (phase_check(PHASE_MACHINE_READY) && !dc->hotpluggable)) {
>> +        (machine_is_ready() && !dc->hotpluggable)) {
>>           error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
>>                      "a pluggable device type");
>>           return NULL;
>> @@ -636,7 +636,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, 
>> Error **errp)
>>           }
>>       }
>>   -    if (phase_check(PHASE_MACHINE_READY) && bus && 
>> !qbus_is_hotpluggable(bus)) {
>> +    if (machine_is_ready() && bus && !qbus_is_hotpluggable(bus)) {
>>           error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>>           return NULL;
>>       }
>> @@ -650,7 +650,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, 
>> Error **errp)
>>       dev = qdev_new(driver);
>>         /* Check whether the hotplug is allowed by the machine */
>> -    if (phase_check(PHASE_MACHINE_READY)) {
>> +    if (machine_is_ready()) {
>>           if (!qdev_hotplug_allowed(dev, errp)) {
>>               goto err_del_dev;
>>           }
>> @@ -998,8 +998,7 @@ int qemu_global_option(const char *str)
>>     bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>>   {
>> -    if (!phase_check(PHASE_MACHINE_READY) &&
>> -        !(cmd->options & QCO_ALLOW_PRECONFIG)) {
>> +    if (!machine_is_ready() && !(cmd->options & QCO_ALLOW_PRECONFIG)) {
>>           error_setg(errp, "The command '%s' is permitted only after 
>> machine initialization has completed",
>>                      cmd->name);
>>           return false;
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index cbf62abeb4..3af9743ceb 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2636,7 +2636,7 @@ static void 
>> qemu_machine_enter_phase(MachineInitPhase target_phase,
>>     void qmp_x_exit_preconfig(Error **errp)
>>   {
>> -    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
>> +    if (machine_is_initialized()) {
>>           error_setg(errp, "The command is permitted only before 
>> machine initialization");
>>           return;
>>       }
>> diff --git a/ui/console.c b/ui/console.c
>> index 2de5f4105b..3513da6a54 100644
>> --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -1353,7 +1353,7 @@ static QemuConsole *new_console(DisplayState 
>> *ds, console_type_t console_type,
>>       if (QTAILQ_EMPTY(&consoles)) {
>>           s->index = 0;
>>           QTAILQ_INSERT_TAIL(&consoles, s, next);
>> -    } else if (console_type != GRAPHIC_CONSOLE || 
>> phase_check(PHASE_MACHINE_READY)) {
>> +    } else if (console_type != GRAPHIC_CONSOLE || machine_is_ready()) {
>>           QemuConsole *last = QTAILQ_LAST(&consoles);
>>           s->index = last->index + 1;
>>           QTAILQ_INSERT_TAIL(&consoles, s, next);
>>
>

Re: [RFC PATCH 2/9] replace machine phase_check with machine_is_initialized/ready calls

[Paolo Bonzini]

On 14/05/21 15:13, Mirela Grujic wrote:
> However, if you believe it should rather be just renamed I can do so.

I am just not sure it's such an advantage to replace phase_check with 
separate functions.  The rename is a constraint of QAPI, so we have to 
live with the long names.

Paolo

Re: [RFC PATCH 2/9] replace machine phase_check with machine_is_initialized/ready calls

[Eric Blake]

On Fri, May 14, 2021 at 11:14:23PM +0200, Paolo Bonzini wrote:
> On 14/05/21 15:13, Mirela Grujic wrote:
> > However, if you believe it should rather be just renamed I can do so.
> 
> I am just not sure it's such an advantage to replace phase_check with
> separate functions.  The rename is a constraint of QAPI, so we have to live
> with the long names.

You can also use 'prefix':'...' as part of the QAPI 'enum' declaration
to pick a shorter name for the C enum (see for example BlkdebugEvent
in block-core.json).

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

[RFC PATCH 3/9] rename MachineInitPhase enumeration constants

[Mirela Grujic]

This renaming is a second phase in getting the code ready for
defining MachineInitPhase in qapi (enumeration constants are
going to be generated and prefixed with a name derived from
the enumeration type, i.e. MACHINE_INIT_PHASE_.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 include/hw/qdev-core.h | 10 +++++-----
 hw/core/machine.c      |  4 ++--
 hw/core/qdev.c         |  4 ++--
 softmmu/vl.c           | 20 ++++++++++----------
 4 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5e3c6d4482..dc2f63478b 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -813,30 +813,30 @@ bool qdev_should_hide_device(QemuOpts *opts);
 
 typedef enum MachineInitPhase {
     /* current_machine is NULL.  */
-    PHASE_NO_MACHINE,
+    MACHINE_INIT_PHASE_NO_MACHINE,
 
     /* current_machine is not NULL, but current_machine->accel is NULL.  */
-    PHASE_MACHINE_CREATED,
+    MACHINE_INIT_PHASE_MACHINE_CREATED,
 
     /*
      * current_machine->accel is not NULL, but the machine properties have
      * not been validated and machine_class->init has not yet been called.
      */
-    PHASE_ACCEL_CREATED,
+    MACHINE_INIT_PHASE_ACCEL_CREATED,
 
     /*
      * machine_class->init has been called, thus creating any embedded
      * devices and validating machine properties.  Devices created at
      * this time are considered to be cold-plugged.
      */
-    PHASE_MACHINE_INITIALIZED,
+    MACHINE_INIT_PHASE_INITIALIZED,
 
     /*
      * QEMU is ready to start CPUs and devices created at this time
      * are considered to be hot-plugged.  The monitor is not restricted
      * to "preconfig" commands.
      */
-    PHASE_MACHINE_READY,
+    MACHINE_INIT_PHASE_READY,
 } MachineInitPhase;
 
 extern bool phase_check(MachineInitPhase phase);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index eba046924d..16ce88407c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1230,7 +1230,7 @@ void machine_run_board_init(MachineState *machine)
     }
 
     machine_class->init(machine);
-    phase_advance(PHASE_MACHINE_INITIALIZED);
+    phase_advance(MACHINE_INIT_PHASE_INITIALIZED);
 }
 
 static NotifierList machine_init_done_notifiers =
@@ -1262,7 +1262,7 @@ void qdev_machine_creation_done(void)
      * ok, initial machine setup is done, starting from now we can
      * only create hotpluggable devices
      */
-    phase_advance(PHASE_MACHINE_READY);
+    phase_advance(MACHINE_INIT_PHASE_READY);
     qdev_assert_realized_properly();
 
     /* TODO: once all bus devices are qdevified, this should be done
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 71906170f9..350f2acf74 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1157,12 +1157,12 @@ MachineInitPhase phase_get(void)
 
 bool machine_is_initialized(void)
 {
-    return machine_phase >= PHASE_MACHINE_INITIALIZED;
+    return machine_phase >= MACHINE_INIT_PHASE_INITIALIZED;
 }
 
 bool machine_is_ready(void)
 {
-    return machine_phase >= PHASE_MACHINE_READY;
+    return machine_phase >= MACHINE_INIT_PHASE_READY;
 }
 
 static const TypeInfo device_type_info = {
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 3af9743ceb..88f504aff9 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2510,7 +2510,7 @@ static void qemu_init_board(void)
     /* process plugin before CPUs are created, but once -smp has been parsed */
     qemu_plugin_load_list(&plugin_list, &error_fatal);
 
-    /* From here on we enter MACHINE_PHASE_INITIALIZED.  */
+    /* From here on we enter MACHINE_INIT_PHASE_INITIALIZED.  */
     machine_run_board_init(current_machine);
 
     drive_check_orphaned();
@@ -2582,7 +2582,7 @@ static void qemu_machine_enter_phase(MachineInitPhase target_phase,
                                      Error **errp)
 {
     /* target phases before initialization are not handled here */
-    if (target_phase < PHASE_MACHINE_INITIALIZED) {
+    if (target_phase < MACHINE_INIT_PHASE_INITIALIZED) {
         error_setg(errp, "Target machine phase too early to enter this way");
         return;
     }
@@ -2597,14 +2597,14 @@ static void qemu_machine_enter_phase(MachineInitPhase target_phase,
      * if machine has not yet passed 'initialized' phase and according to the
      * target_phase it should
      */
-    if (target_phase >= PHASE_MACHINE_INITIALIZED &&
-        phase_get() < PHASE_MACHINE_INITIALIZED) {
+    if (target_phase >= MACHINE_INIT_PHASE_INITIALIZED &&
+        phase_get() < MACHINE_INIT_PHASE_INITIALIZED) {
         qemu_init_board();
         qemu_create_cli_devices();
     }
 
-    if (target_phase >= PHASE_MACHINE_READY &&
-        phase_get() < PHASE_MACHINE_READY) {
+    if (target_phase >= MACHINE_INIT_PHASE_READY &&
+        phase_get() < MACHINE_INIT_PHASE_READY) {
         qemu_machine_creation_done();
 
         if (loadvm) {
@@ -2641,7 +2641,7 @@ void qmp_x_exit_preconfig(Error **errp)
         return;
     }
 
-    qemu_machine_enter_phase(PHASE_MACHINE_READY, errp);
+    qemu_machine_enter_phase(MACHINE_INIT_PHASE_READY, errp);
 }
 
 void qemu_init(int argc, char **argv, char **envp)
@@ -3580,14 +3580,14 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_create_early_backends();
 
     qemu_apply_machine_options();
-    phase_advance(PHASE_MACHINE_CREATED);
+    phase_advance(MACHINE_INIT_PHASE_MACHINE_CREATED);
 
     /*
      * Note: uses machine properties such as kernel-irqchip, must run
      * after machine_set_property().
      */
     configure_accelerators(argv[0]);
-    phase_advance(PHASE_ACCEL_CREATED);
+    phase_advance(MACHINE_INIT_PHASE_ACCEL_CREATED);
 
     /*
      * Beware, QOM objects created before this point miss global and
@@ -3637,7 +3637,7 @@ void qemu_init(int argc, char **argv, char **envp)
     }
 
     if (!preconfig_requested) {
-        qemu_machine_enter_phase(PHASE_MACHINE_READY, &error_fatal);
+        qemu_machine_enter_phase(MACHINE_INIT_PHASE_READY, &error_fatal);
     }
     qemu_init_displays();
     accel_setup_post(current_machine);
-- 
2.25.1

[RFC PATCH 4/9] qapi: Implement 'query-machine-phase' command

[Mirela Grujic]

The command returns current machine initialization phase.
From now on, the MachineInitPhase enum is generated.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 qapi/machine.json          | 54 ++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-core.h     | 29 +-------------------
 hw/core/machine-qmp-cmds.c |  9 +++++++
 3 files changed, 64 insertions(+), 28 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 6e90d463fc..47bdbec817 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1274,3 +1274,57 @@
 ##
 { 'event': 'MEM_UNPLUG_ERROR',
   'data': { 'device': 'str', 'msg': 'str' } }
+
+##
+# @MachineInitPhase:
+#
+# Enumeration of machine initialization phases.
+#
+# @no-machine: machine does not exist
+#
+# @machine-created: machine is created, but its accelerator is not
+#
+# @accel-created: accelerator is created, but the machine properties have not
+#                 been validated and machine initialization is not done yet
+#
+# @initialized: machine is initialized, thus creating any embedded devices and
+#               validating machine properties. Devices created at this time are
+#               considered to be cold-plugged.
+#
+# @ready: QEMU is ready to start CPUs and devices created at this time are
+#         considered to be hot-plugged. The monitor is not restricted to
+#         "preconfig" commands.
+##
+{ 'enum': 'MachineInitPhase',
+  'data': [ 'no-machine', 'machine-created', 'accel-created', 'initialized',
+            'ready' ] }
+
+##
+# @MachineInitPhaseStatus:
+#
+# Information about machine initialization phase
+#
+# @phase: the machine initialization phase
+#
+# Since:  #FIXME
+##
+{ 'struct': 'MachineInitPhaseStatus',
+  'data': { 'phase': 'MachineInitPhase' } }
+
+##
+# @query-machine-phase:
+#
+# Return machine initialization phase
+#
+# Since: #FIXME
+#
+# Returns: MachineInitPhaseStatus
+#
+# Example:
+#
+# -> { "execute": "query-machine-phase" }
+# <- { "return": { "phase": "initialized" } }
+#
+##
+{ 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
+             'allow-preconfig': true }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index dc2f63478b..ca39b77ae6 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -1,6 +1,7 @@
 #ifndef QDEV_CORE_H
 #define QDEV_CORE_H
 
+#include "qapi/qapi-types-machine.h"
 #include "qemu/queue.h"
 #include "qemu/bitmap.h"
 #include "qemu/rcu.h"
@@ -811,34 +812,6 @@ void device_listener_unregister(DeviceListener *listener);
  */
 bool qdev_should_hide_device(QemuOpts *opts);
 
-typedef enum MachineInitPhase {
-    /* current_machine is NULL.  */
-    MACHINE_INIT_PHASE_NO_MACHINE,
-
-    /* current_machine is not NULL, but current_machine->accel is NULL.  */
-    MACHINE_INIT_PHASE_MACHINE_CREATED,
-
-    /*
-     * current_machine->accel is not NULL, but the machine properties have
-     * not been validated and machine_class->init has not yet been called.
-     */
-    MACHINE_INIT_PHASE_ACCEL_CREATED,
-
-    /*
-     * machine_class->init has been called, thus creating any embedded
-     * devices and validating machine properties.  Devices created at
-     * this time are considered to be cold-plugged.
-     */
-    MACHINE_INIT_PHASE_INITIALIZED,
-
-    /*
-     * QEMU is ready to start CPUs and devices created at this time
-     * are considered to be hot-plugged.  The monitor is not restricted
-     * to "preconfig" commands.
-     */
-    MACHINE_INIT_PHASE_READY,
-} MachineInitPhase;
-
 extern bool phase_check(MachineInitPhase phase);
 extern void phase_advance(MachineInitPhase phase);
 extern MachineInitPhase phase_get(void);
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index be286882cc..23f837dadb 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -198,3 +198,12 @@ MemdevList *qmp_query_memdev(Error **errp)
     object_child_foreach(obj, query_memdev, &list);
     return list;
 }
+
+MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
+{
+    MachineInitPhaseStatus *status = g_malloc0(sizeof(*status));
+
+    status->phase = phase_get();
+
+    return status;
+}
-- 
2.25.1

Re: [RFC PATCH 4/9] qapi: Implement 'query-machine-phase' command

[Paolo Bonzini]

On 13/05/21 10:25, Mirela Grujic wrote:
> The command returns current machine initialization phase.
>  From now on, the MachineInitPhase enum is generated.
> 
> Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
> ---
>   qapi/machine.json          | 54 ++++++++++++++++++++++++++++++++++++++
>   include/hw/qdev-core.h     | 29 +-------------------
>   hw/core/machine-qmp-cmds.c |  9 +++++++
>   3 files changed, 64 insertions(+), 28 deletions(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 6e90d463fc..47bdbec817 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1274,3 +1274,57 @@
>   ##
>   { 'event': 'MEM_UNPLUG_ERROR',
>     'data': { 'device': 'str', 'msg': 'str' } }
> +
> +##
> +# @MachineInitPhase:
> +#
> +# Enumeration of machine initialization phases.
> +#
> +# @no-machine: machine does not exist
> +#
> +# @machine-created: machine is created, but its accelerator is not
> +#
> +# @accel-created: accelerator is created, but the machine properties have not
> +#                 been validated and machine initialization is not done yet
> +#
> +# @initialized: machine is initialized, thus creating any embedded devices and
> +#               validating machine properties. Devices created at this time are
> +#               considered to be cold-plugged.
> +#
> +# @ready: QEMU is ready to start CPUs and devices created at this time are
> +#         considered to be hot-plugged. The monitor is not restricted to
> +#         "preconfig" commands.
> +##
> +{ 'enum': 'MachineInitPhase',
> +  'data': [ 'no-machine', 'machine-created', 'accel-created', 'initialized',
> +            'ready' ] }
> +
> +##
> +# @MachineInitPhaseStatus:
> +#
> +# Information about machine initialization phase
> +#
> +# @phase: the machine initialization phase
> +#
> +# Since:  #FIXME
> +##
> +{ 'struct': 'MachineInitPhaseStatus',
> +  'data': { 'phase': 'MachineInitPhase' } }
> +
> +##
> +# @query-machine-phase:
> +#
> +# Return machine initialization phase
> +#
> +# Since: #FIXME
> +#
> +# Returns: MachineInitPhaseStatus
> +#
> +# Example:
> +#
> +# -> { "execute": "query-machine-phase" }
> +# <- { "return": { "phase": "initialized" } }
> +#
> +##
> +{ 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
> +             'allow-preconfig': true }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index dc2f63478b..ca39b77ae6 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -1,6 +1,7 @@
>   #ifndef QDEV_CORE_H
>   #define QDEV_CORE_H
>   
> +#include "qapi/qapi-types-machine.h"
>   #include "qemu/queue.h"
>   #include "qemu/bitmap.h"
>   #include "qemu/rcu.h"
> @@ -811,34 +812,6 @@ void device_listener_unregister(DeviceListener *listener);
>    */
>   bool qdev_should_hide_device(QemuOpts *opts);
>   
> -typedef enum MachineInitPhase {
> -    /* current_machine is NULL.  */
> -    MACHINE_INIT_PHASE_NO_MACHINE,
> -
> -    /* current_machine is not NULL, but current_machine->accel is NULL.  */
> -    MACHINE_INIT_PHASE_MACHINE_CREATED,
> -
> -    /*
> -     * current_machine->accel is not NULL, but the machine properties have
> -     * not been validated and machine_class->init has not yet been called.
> -     */
> -    MACHINE_INIT_PHASE_ACCEL_CREATED,
> -
> -    /*
> -     * machine_class->init has been called, thus creating any embedded
> -     * devices and validating machine properties.  Devices created at
> -     * this time are considered to be cold-plugged.
> -     */
> -    MACHINE_INIT_PHASE_INITIALIZED,
> -
> -    /*
> -     * QEMU is ready to start CPUs and devices created at this time
> -     * are considered to be hot-plugged.  The monitor is not restricted
> -     * to "preconfig" commands.
> -     */
> -    MACHINE_INIT_PHASE_READY,
> -} MachineInitPhase;
> -
>   extern bool phase_check(MachineInitPhase phase);
>   extern void phase_advance(MachineInitPhase phase);
>   extern MachineInitPhase phase_get(void);
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index be286882cc..23f837dadb 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -198,3 +198,12 @@ MemdevList *qmp_query_memdev(Error **errp)
>       object_child_foreach(obj, query_memdev, &list);
>       return list;
>   }
> +
> +MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
> +{
> +    MachineInitPhaseStatus *status = g_malloc0(sizeof(*status));
> +
> +    status->phase = phase_get();
> +
> +    return status;
> +}
> 

This command is a good idea, and we can in fact even include it already 
in QEMU.

Paolo

Re: [RFC PATCH 4/9] qapi: Implement 'query-machine-phase' command

[Daniel P. Berrangé]

On Thu, May 13, 2021 at 07:44:34PM +0200, Paolo Bonzini wrote:
> On 13/05/21 10:25, Mirela Grujic wrote:
> > The command returns current machine initialization phase.
> >  From now on, the MachineInitPhase enum is generated.
> > 
> > Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
> > ---
> >   qapi/machine.json          | 54 ++++++++++++++++++++++++++++++++++++++
> >   include/hw/qdev-core.h     | 29 +-------------------
> >   hw/core/machine-qmp-cmds.c |  9 +++++++
> >   3 files changed, 64 insertions(+), 28 deletions(-)
> > 
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index 6e90d463fc..47bdbec817 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1274,3 +1274,57 @@
> >   ##
> >   { 'event': 'MEM_UNPLUG_ERROR',
> >     'data': { 'device': 'str', 'msg': 'str' } }
> > +
> > +##
> > +# @MachineInitPhase:
> > +#
> > +# Enumeration of machine initialization phases.
> > +#
> > +# @no-machine: machine does not exist
> > +#
> > +# @machine-created: machine is created, but its accelerator is not
> > +#
> > +# @accel-created: accelerator is created, but the machine properties have not
> > +#                 been validated and machine initialization is not done yet
> > +#
> > +# @initialized: machine is initialized, thus creating any embedded devices and
> > +#               validating machine properties. Devices created at this time are
> > +#               considered to be cold-plugged.
> > +#
> > +# @ready: QEMU is ready to start CPUs and devices created at this time are
> > +#         considered to be hot-plugged. The monitor is not restricted to
> > +#         "preconfig" commands.
> > +##
> > +{ 'enum': 'MachineInitPhase',
> > +  'data': [ 'no-machine', 'machine-created', 'accel-created', 'initialized',
> > +            'ready' ] }
> > +
> > +##
> > +# @MachineInitPhaseStatus:
> > +#
> > +# Information about machine initialization phase
> > +#
> > +# @phase: the machine initialization phase
> > +#
> > +# Since:  #FIXME
> > +##
> > +{ 'struct': 'MachineInitPhaseStatus',
> > +  'data': { 'phase': 'MachineInitPhase' } }
> > +
> > +##
> > +# @query-machine-phase:
> > +#
> > +# Return machine initialization phase
> > +#
> > +# Since: #FIXME
> > +#
> > +# Returns: MachineInitPhaseStatus
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "query-machine-phase" }
> > +# <- { "return": { "phase": "initialized" } }
> > +#
> > +##
> > +{ 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
> > +             'allow-preconfig': true }
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index dc2f63478b..ca39b77ae6 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -1,6 +1,7 @@
> >   #ifndef QDEV_CORE_H
> >   #define QDEV_CORE_H
> > +#include "qapi/qapi-types-machine.h"
> >   #include "qemu/queue.h"
> >   #include "qemu/bitmap.h"
> >   #include "qemu/rcu.h"
> > @@ -811,34 +812,6 @@ void device_listener_unregister(DeviceListener *listener);
> >    */
> >   bool qdev_should_hide_device(QemuOpts *opts);
> > -typedef enum MachineInitPhase {
> > -    /* current_machine is NULL.  */
> > -    MACHINE_INIT_PHASE_NO_MACHINE,
> > -
> > -    /* current_machine is not NULL, but current_machine->accel is NULL.  */
> > -    MACHINE_INIT_PHASE_MACHINE_CREATED,
> > -
> > -    /*
> > -     * current_machine->accel is not NULL, but the machine properties have
> > -     * not been validated and machine_class->init has not yet been called.
> > -     */
> > -    MACHINE_INIT_PHASE_ACCEL_CREATED,
> > -
> > -    /*
> > -     * machine_class->init has been called, thus creating any embedded
> > -     * devices and validating machine properties.  Devices created at
> > -     * this time are considered to be cold-plugged.
> > -     */
> > -    MACHINE_INIT_PHASE_INITIALIZED,
> > -
> > -    /*
> > -     * QEMU is ready to start CPUs and devices created at this time
> > -     * are considered to be hot-plugged.  The monitor is not restricted
> > -     * to "preconfig" commands.
> > -     */
> > -    MACHINE_INIT_PHASE_READY,
> > -} MachineInitPhase;
> > -
> >   extern bool phase_check(MachineInitPhase phase);
> >   extern void phase_advance(MachineInitPhase phase);
> >   extern MachineInitPhase phase_get(void);
> > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> > index be286882cc..23f837dadb 100644
> > --- a/hw/core/machine-qmp-cmds.c
> > +++ b/hw/core/machine-qmp-cmds.c
> > @@ -198,3 +198,12 @@ MemdevList *qmp_query_memdev(Error **errp)
> >       object_child_foreach(obj, query_memdev, &list);
> >       return list;
> >   }
> > +
> > +MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
> > +{
> > +    MachineInitPhaseStatus *status = g_malloc0(sizeof(*status));
> > +
> > +    status->phase = phase_get();
> > +
> > +    return status;
> > +}
> > 
> 
> This command is a good idea, and we can in fact even include it already in
> QEMU.

I worry that this is exposing an internal QEMU implementation detail.
If apps rely on a particular set of phases existing, and running in
particular order, it could easily hobble our ability to refactor
QEMU's startup procedure to cope with new requirements in future. 

I don't even much like the existing "pre-config" concept we have
for this same reason, and this is adding many more phases.

I think we'd be better off trying to make it possible for the mgmt
app to just provide all the config at once and let QEMU figure our
the phases in which it then instantiates stuff, so we can freedom
to change our minds at any time.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[RFC PATCH 5/9] qapi: Implement 'next-machine-phase' command

[Mirela Grujic]

This command will be used to control via QMP the advancing of machine
through initialization phases. The feature is needed to enable the
machine configuration via QMP.

The command triggers QEMU to advance the machine to the next init phase,
i.e. to execute initialization steps required to enter the next phase.
The command is used in combination with -preconfig command line option.

Note: advancing the machine to the final phase has the same effect as
executing 'x-exit-preconfig' command.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 qapi/machine.json          | 24 ++++++++++++++++++++++++
 include/sysemu/sysemu.h    |  3 +++
 hw/core/machine-qmp-cmds.c | 12 ++++++++++++
 softmmu/vl.c               |  3 +--
 4 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 47bdbec817..968d67dd95 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1328,3 +1328,27 @@
 ##
 { 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
              'allow-preconfig': true }
+
+##
+# @next-machine-phase:
+#
+# Increment machine initialization phase
+#
+# Since: #FIXME
+#
+# Returns: If successful, nothing
+#
+# Notes: This command will trigger QEMU to execute initialization steps
+#        that are required to enter the next machine initialization phase.
+#        If by incrementing the initialization phase the machine reaches
+#        the final phase, the guest will start running immediately unless
+#        the -S option is used. The command is available only if the
+#        -preconfig command line option was passed.
+#
+# Example:
+#
+# -> { "execute": "next-machine-phase" }
+# <- { "return": {} }
+#
+##
+{ 'command': 'next-machine-phase', 'allow-preconfig': true }
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 8fae667172..0df06d095d 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -2,6 +2,7 @@
 #define SYSEMU_H
 /* Misc. things related to the system emulator.  */
 
+#include "hw/qdev-core.h"
 #include "qemu/timer.h"
 #include "qemu/notify.h"
 #include "qemu/uuid.h"
@@ -20,6 +21,8 @@ void qemu_run_machine_init_done_notifiers(void);
 void qemu_add_machine_init_done_notifier(Notifier *notify);
 void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
+void qemu_machine_enter_phase(MachineInitPhase target_phase, Error **errp);
+
 void configure_rtc(QemuOpts *opts);
 
 void qemu_init_subsystems(void);
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 23f837dadb..8aa743d59b 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -207,3 +207,15 @@ MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
 
     return status;
 }
+
+void qmp_next_machine_phase(Error **errp)
+{
+    MachineInitPhase target_phase = phase_get() + 1;
+
+    if (target_phase >= MACHINE_INIT_PHASE__MAX) {
+        error_setg(errp, "Cannot increment machine init phase any further");
+        return;
+    }
+
+    qemu_machine_enter_phase(target_phase, errp);
+}
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 88f504aff9..0f402806f5 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2578,8 +2578,7 @@ static void qemu_machine_creation_done(void)
     }
 }
 
-static void qemu_machine_enter_phase(MachineInitPhase target_phase,
-                                     Error **errp)
+void qemu_machine_enter_phase(MachineInitPhase target_phase, Error **errp)
 {
     /* target phases before initialization are not handled here */
     if (target_phase < MACHINE_INIT_PHASE_INITIALIZED) {
-- 
2.25.1

Re: [RFC PATCH 5/9] qapi: Implement 'next-machine-phase' command

[Eric Blake]

On Thu, May 13, 2021 at 10:25:45AM +0200, Mirela Grujic wrote:
> This command will be used to control via QMP the advancing of machine
> through initialization phases. The feature is needed to enable the
> machine configuration via QMP.
> 
> The command triggers QEMU to advance the machine to the next init phase,
> i.e. to execute initialization steps required to enter the next phase.
> The command is used in combination with -preconfig command line option.
> 
> Note: advancing the machine to the final phase has the same effect as
> executing 'x-exit-preconfig' command.
> 
> Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
> ---
>  qapi/machine.json          | 24 ++++++++++++++++++++++++
>  include/sysemu/sysemu.h    |  3 +++
>  hw/core/machine-qmp-cmds.c | 12 ++++++++++++
>  softmmu/vl.c               |  3 +--
>  4 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 47bdbec817..968d67dd95 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1328,3 +1328,27 @@
>  ##
>  { 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
>               'allow-preconfig': true }
> +
> +##
> +# @next-machine-phase:
> +#
> +# Increment machine initialization phase
> +#
> +# Since: #FIXME

You can put 6.1 instead of #FIXME if that's what you're aiming for
(but then you may have to adjust it to 6.2 if it misses soft
freeze...)

> +#
> +# Returns: If successful, nothing

This sentence doesn't add much.

> +#
> +# Notes: This command will trigger QEMU to execute initialization steps
> +#        that are required to enter the next machine initialization phase.
> +#        If by incrementing the initialization phase the machine reaches
> +#        the final phase, the guest will start running immediately unless
> +#        the -S option is used. The command is available only if the
> +#        -preconfig command line option was passed.
> +#
> +# Example:
> +#
> +# -> { "execute": "next-machine-phase" }
> +# <- { "return": {} }

As an API, this command seems awkward.  How do I query what phase I'm
currently in?  How many times do I have to call it?  Do we anticipate
the number of times I need to call it to change in future qemu
releases?

Would it be better to require me to pass an enum parameter stating the
phase I intend to move into (and error out if that is not actually the
next phase), where the introspection of the enum tells me how many
times to use this command?  Should this command return a struct
containing "new-state":"enum-value" telling me what state I moved to
on success?


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

Re: [RFC PATCH 5/9] qapi: Implement 'next-machine-phase' command

[Paolo Bonzini]

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

Il ven 4 giu 2021, 16:26 Eric Blake <eblake@redhat.com> ha scritto:

> As an API, this command seems awkward.  How do I query what phase I'm
> currently in?  How many times do I have to call it?  Do we anticipate
> the number of times I need to call it to change in future qemu
> releases?
>

Indeed this has been changed in the last version of the proposal. There
will be an x-machine-init command and the existing x-exit-preconfig.

Paolo


> Would it be better to require me to pass an enum parameter stating the
> phase I intend to move into (and error out if that is not actually the
> next phase), where the introspection of the enum tells me how many
> times to use this command?  Should this command return a struct
> containing "new-state":"enum-value" telling me what state I moved to
> on success?
>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>

[-- Attachment #2: Type: text/html, Size: 1751 bytes --]

[RFC PATCH 6/9] qapi: Implement 'advance-machine-phase' command

[Mirela Grujic]

The command takes the target initialization phase as the argument
and triggers QEMU to advance the machine to the target phase (i.e.
execute all initialization steps required to enter the target phase).

This command would be used as an alternative to 'next-machine-phase'
if it's more convenient to jump to a target initialization phase than
to single-step through phases.

The command is used in combination with the -preconfig CLI option.

Note: advancing the machine to the 'ready' phase has the same effect as
executing the 'x-exit-preconfig' command when the machine is in
'accel-created' phase.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 qapi/machine.json          | 26 ++++++++++++++++++++++++++
 hw/core/machine-qmp-cmds.c | 10 ++++++++++
 2 files changed, 36 insertions(+)

diff --git a/qapi/machine.json b/qapi/machine.json
index 968d67dd95..31872aae72 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1352,3 +1352,29 @@
 #
 ##
 { 'command': 'next-machine-phase', 'allow-preconfig': true }
+
+##
+# @advance-machine-phase:
+#
+# Advance machine initialization phase to the target phase
+#
+# @phase: target machine initialization phase
+#
+# Since: #FIXME
+#
+# Returns: If successful, nothing
+#
+# Notes: This command will trigger QEMU to execute initialization steps
+#        that are required to enter the target machine initialization phase.
+#        If the target phase is the final initialization phase, the guest will
+#        start running immediately unless the -S option is used. The command
+#        is available only if the -preconfig command line option was passed.
+#
+# Example:
+#
+# -> { "execute": "advance-machine-phase", "arguments": { "phase": "ready" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'advance-machine-phase', 'data' : {'phase': 'MachineInitPhase'},
+             'allow-preconfig': true }
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 8aa743d59b..6b21a3fdd5 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -219,3 +219,13 @@ void qmp_next_machine_phase(Error **errp)
 
     qemu_machine_enter_phase(target_phase, errp);
 }
+
+void qmp_advance_machine_phase(MachineInitPhase phase, Error **errp)
+{
+    if (phase_get() == phase) {
+        error_setg(errp, "Machine is already in the target phase");
+        return;
+    }
+
+    qemu_machine_enter_phase(phase, errp);
+}
-- 
2.25.1

Re: [RFC PATCH 6/9] qapi: Implement 'advance-machine-phase' command

[Kevin Wolf]

Am 13.05.2021 um 10:25 hat Mirela Grujic geschrieben:
> The command takes the target initialization phase as the argument
> and triggers QEMU to advance the machine to the target phase (i.e.
> execute all initialization steps required to enter the target phase).
> 
> This command would be used as an alternative to 'next-machine-phase'
> if it's more convenient to jump to a target initialization phase than
> to single-step through phases.
> 
> The command is used in combination with the -preconfig CLI option.
> 
> Note: advancing the machine to the 'ready' phase has the same effect as
> executing the 'x-exit-preconfig' command when the machine is in
> 'accel-created' phase.
> 
> Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>

I think this command is preferable, not just because it is more
convenient if you don't have anything to do in some phase, but also
because it is more explicit and doesn't change its behaviour depending
on the current state.

We probably need to expect that this is a command that might often be
used in quickly hacked up shell scripts, which are error prone (Did I
really count the number of 'next-machine-phase' command right? Which
phase are we switching to again in this line?) and may lack proper error
handling, so the least amount of implicit magic will make sure that
users don't get more surprises than necessary.

> diff --git a/qapi/machine.json b/qapi/machine.json
> index 968d67dd95..31872aae72 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1352,3 +1352,29 @@
>  #
>  ##
>  { 'command': 'next-machine-phase', 'allow-preconfig': true }
> +
> +##
> +# @advance-machine-phase:
> +#
> +# Advance machine initialization phase to the target phase
> +#
> +# @phase: target machine initialization phase
> +#
> +# Since: #FIXME
> +#
> +# Returns: If successful, nothing
> +#
> +# Notes: This command will trigger QEMU to execute initialization steps
> +#        that are required to enter the target machine initialization phase.
> +#        If the target phase is the final initialization phase, the guest will
> +#        start running immediately unless the -S option is used. The command
> +#        is available only if the -preconfig command line option was passed.
> +#
> +# Example:
> +#
> +# -> { "execute": "advance-machine-phase", "arguments": { "phase": "ready" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'advance-machine-phase', 'data' : {'phase': 'MachineInitPhase'},
> +             'allow-preconfig': true }
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 8aa743d59b..6b21a3fdd5 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -219,3 +219,13 @@ void qmp_next_machine_phase(Error **errp)
>  
>      qemu_machine_enter_phase(target_phase, errp);
>  }
> +
> +void qmp_advance_machine_phase(MachineInitPhase phase, Error **errp)
> +{
> +    if (phase_get() == phase) {
> +        error_setg(errp, "Machine is already in the target phase");
> +        return;
> +    }

Another option would be making it set-machine-phase, which doesn't fail
if you're setting the phase that you're already in. It would only fail
if you're trying to go backwards. But this is a minor detail.

> +    qemu_machine_enter_phase(phase, errp);
> +}

Kevin

[RFC PATCH 7/9] qdev-monitor: Restructure and fix the check for command availability

[Mirela Grujic]

The existing code had to be restructured to make room for adding
checks that are specific to the machine phases.

The fix is related to the way that commands with the 'allow-preconfig'
option are treated.

Commands labelled with the 'allow-preconfig' option were meant to be allowed
during the 'preconfig' state, i.e. before the machine is initialized.
The equivalent of 'preconfig' state (after its removal) is machine init
phase MACHINE_INIT_PHASE_ACCEL_CREATED. Therefore, commands with
'allow-preconfig' option should be allowed to run while the machine is
in MACHINE_INIT_PHASE_ACCEL_CREATED phase.
Before this patch, those commands were allowed to run if the machine is
not ready, which includes three more phases besides the accel-created
phase. Since there were no means to enter other phases via QMP, the
implementation was fine. However, with the introduction of
'next-machine-phase' and 'advance-machine-phase' commands, one could
enter machine 'initialized' phase and still have available 'preconfig'
commands, which is incorrect.

This patch makes available 'allow-preconfig' commands only when they're
needed - before the machine is initialized, in the accel-created phase.
To enable a command after the machine gets initialized and before it
becomes ready, one should use 'allow-init-config' option that will be
introduced in the following patch.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 softmmu/qdev-monitor.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index be8a892517..448f9dbb6f 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -998,10 +998,19 @@ int qemu_global_option(const char *str)
 
 bool qmp_command_available(const QmpCommand *cmd, Error **errp)
 {
-    if (!machine_is_ready() && !(cmd->options & QCO_ALLOW_PRECONFIG)) {
-        error_setg(errp, "The command '%s' is permitted only after machine initialization has completed",
-                   cmd->name);
-        return false;
+    switch (phase_get()) {
+    case MACHINE_INIT_PHASE_ACCEL_CREATED:
+        if (cmd->options & QCO_ALLOW_PRECONFIG) {
+            return true;
+        }
+        break;
+    case MACHINE_INIT_PHASE_READY:
+        /* All commands are available when the machine is ready */
+        return true;
+    default:
+        break;
     }
-    return true;
+    error_setg(errp, "The command '%s' is not permitted at this phase",
+               cmd->name);
+    return false;
 }
-- 
2.25.1

Re: [RFC PATCH 7/9] qdev-monitor: Restructure and fix the check for command availability

[Paolo Bonzini]

On 13/05/21 10:25, Mirela Grujic wrote:
> The existing code had to be restructured to make room for adding
> checks that are specific to the machine phases.
> 
> The fix is related to the way that commands with the 'allow-preconfig'
> option are treated.
> 
> Commands labelled with the 'allow-preconfig' option were meant to be allowed
> during the 'preconfig' state, i.e. before the machine is initialized.
> The equivalent of 'preconfig' state (after its removal) is machine init
> phase MACHINE_INIT_PHASE_ACCEL_CREATED. Therefore, commands with
> 'allow-preconfig' option should be allowed to run while the machine is
> in MACHINE_INIT_PHASE_ACCEL_CREATED phase.
> Before this patch, those commands were allowed to run if the machine is
> not ready, which includes three more phases besides the accel-created
> phase. Since there were no means to enter other phases via QMP, the
> implementation was fine. However, with the introduction of
> 'next-machine-phase' and 'advance-machine-phase' commands, one could
> enter machine 'initialized' phase and still have available 'preconfig'
> commands, which is incorrect.
> 
> This patch makes available 'allow-preconfig' commands only when they're
> needed - before the machine is initialized, in the accel-created phase.
> To enable a command after the machine gets initialized and before it
> becomes ready, one should use 'allow-init-config' option that will be
> introduced in the following patch.

There aren't many commands that are valid only for the accel created or 
machine initialized phase.  I think adding allow-init-config is more 
churn than keeping only allow-preconfig, and calling phase_check in the 
individual commands.  (Or even better, in the internal APIs that they 
call, so that QMP is completely oblivious to phases and just gets the 
Error* back).

In other words, allow-preconfig is there because there are many commands 
that are allowed only after the machine-ready phase, but anything in the 
middle can be handled just fine from C code.

Paolo

Re: [RFC PATCH 7/9] qdev-monitor: Restructure and fix the check for command availability

[Mirela Grujic]

On 5/13/21 7:43 PM, Paolo Bonzini wrote:
> On 13/05/21 10:25, Mirela Grujic wrote:
>> The existing code had to be restructured to make room for adding
>> checks that are specific to the machine phases.
>>
>> The fix is related to the way that commands with the 'allow-preconfig'
>> option are treated.
>>
>> Commands labelled with the 'allow-preconfig' option were meant to be 
>> allowed
>> during the 'preconfig' state, i.e. before the machine is initialized.
>> The equivalent of 'preconfig' state (after its removal) is machine init
>> phase MACHINE_INIT_PHASE_ACCEL_CREATED. Therefore, commands with
>> 'allow-preconfig' option should be allowed to run while the machine is
>> in MACHINE_INIT_PHASE_ACCEL_CREATED phase.
>> Before this patch, those commands were allowed to run if the machine is
>> not ready, which includes three more phases besides the accel-created
>> phase. Since there were no means to enter other phases via QMP, the
>> implementation was fine. However, with the introduction of
>> 'next-machine-phase' and 'advance-machine-phase' commands, one could
>> enter machine 'initialized' phase and still have available 'preconfig'
>> commands, which is incorrect.
>>
>> This patch makes available 'allow-preconfig' commands only when they're
>> needed - before the machine is initialized, in the accel-created phase.
>> To enable a command after the machine gets initialized and before it
>> becomes ready, one should use 'allow-init-config' option that will be
>> introduced in the following patch.
>
> There aren't many commands that are valid only for the accel created 
> or machine initialized phase.  I think adding allow-init-config is 
> more churn than keeping only allow-preconfig, and calling phase_check 
> in the individual commands.  (Or even better, in the internal APIs 
> that they call, so that QMP is completely oblivious to phases and just 
> gets the Error* back).
>
> In other words, allow-preconfig is there because there are many 
> commands that are allowed only after the machine-ready phase, but 
> anything in the middle can be handled just fine from C code.


To summarize, 'allow-preconfig' specifies whether a command is allowed 
to run before the machine is ready, so any command that should be 
allowed to run at phase < 'machine-ready' must have this flag set. For 
those commands, one should check the current machine phase in the 
implementation of the command to determine whether the command should 
run or not, and return an error if not. Ok, that's fine.


>
> Paolo
>

[RFC PATCH 8/9] qapi: Introduce 'allow-init-config' option

[Mirela Grujic]

This option will be used to select the commands which are allowed
to execute during the MACHINE_INIT_PHASE_INITIALIZED machine phase.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 docs/sphinx/qapidoc.py      |  2 +-
 include/qapi/qmp/dispatch.h |  1 +
 softmmu/qdev-monitor.c      |  5 +++++
 scripts/qapi/commands.py    | 10 +++++++---
 scripts/qapi/expr.py        |  5 +++--
 scripts/qapi/introspect.py  |  3 ++-
 scripts/qapi/schema.py      | 10 ++++++----
 7 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index b7a2d39c10..5432560480 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -332,7 +332,7 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
 
     def visit_command(self, name, info, ifcond, features, arg_type,
                       ret_type, gen, success_response, boxed, allow_oob,
-                      allow_preconfig, coroutine):
+                      allow_preconfig, coroutine, allow_init_config):
         doc = self._cur_doc
         self._add_doc('Command',
                       self._nodes_for_arguments(doc,
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 075203dc67..ecdfc9c444 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -27,6 +27,7 @@ typedef enum QmpCommandOptions
     QCO_ALLOW_PRECONFIG       =  (1U << 2),
     QCO_COROUTINE             =  (1U << 3),
     QCO_DEPRECATED            =  (1U << 4),
+    QCO_ALLOW_INIT_CONFIG     =  (1U << 5),
 } QmpCommandOptions;
 
 typedef struct QmpCommand
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 448f9dbb6f..92423f92db 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1004,6 +1004,11 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
             return true;
         }
         break;
+    case MACHINE_INIT_PHASE_INITIALIZED:
+        if (cmd->options & QCO_ALLOW_INIT_CONFIG) {
+            return true;
+        }
+        break;
     case MACHINE_INIT_PHASE_READY:
         /* All commands are available when the machine is ready */
         return true;
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 0e13d51054..cc8fc89384 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -214,7 +214,8 @@ def gen_register_command(name: str,
                          success_response: bool,
                          allow_oob: bool,
                          allow_preconfig: bool,
-                         coroutine: bool) -> str:
+                         coroutine: bool,
+                         allow_init_config: bool) -> str:
     options = []
 
     if 'deprecated' in [f.name for f in features]:
@@ -228,6 +229,8 @@ def gen_register_command(name: str,
         options += ['QCO_ALLOW_PRECONFIG']
     if coroutine:
         options += ['QCO_COROUTINE']
+    if allow_init_config:
+        options += ['QCO_ALLOW_INIT_CONFIG']
 
     if not options:
         options = ['QCO_NO_OPTIONS']
@@ -310,7 +313,8 @@ def visit_command(self,
                       boxed: bool,
                       allow_oob: bool,
                       allow_preconfig: bool,
-                      coroutine: bool) -> None:
+                      coroutine: bool,
+                      allow_init_config: bool) -> None:
         if not gen:
             return
         # FIXME: If T is a user-defined type, the user is responsible
@@ -331,7 +335,7 @@ def visit_command(self,
             with ifcontext(ifcond, self._genh, self._genc):
                 self._genc.add(gen_register_command(
                     name, features, success_response, allow_oob,
-                    allow_preconfig, coroutine))
+                    allow_preconfig, coroutine, allow_init_config))
 
 
 def gen_commands(schema: QAPISchema,
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 540b3982b1..45031c950c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -107,7 +107,8 @@ def check_flags(expr, info):
         if key in expr and expr[key] is not False:
             raise QAPISemError(
                 info, "flag '%s' may only use false value" % key)
-    for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:
+    for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine',
+                'allow-init-config']:
         if key in expr and expr[key] is not True:
             raise QAPISemError(
                 info, "flag '%s' may only use true value" % key)
@@ -378,7 +379,7 @@ def check_exprs(exprs):
                        ['command'],
                        ['data', 'returns', 'boxed', 'if', 'features',
                         'gen', 'success-response', 'allow-oob',
-                        'allow-preconfig', 'coroutine'])
+                        'allow-preconfig', 'coroutine', 'allow-init-config'])
             normalize_members(expr.get('data'))
             check_command(expr, info)
         elif meta == 'event':
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 9a348ca2e5..fccc67c6d4 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -353,7 +353,8 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
                       arg_type: Optional[QAPISchemaObjectType],
                       ret_type: Optional[QAPISchemaType], gen: bool,
                       success_response: bool, boxed: bool, allow_oob: bool,
-                      allow_preconfig: bool, coroutine: bool) -> None:
+                      allow_preconfig: bool, coroutine: bool,
+                      allow_init_config: bool) -> None:
         assert self._schema is not None
 
         arg_type = arg_type or self._schema.the_empty_object_type
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 703b446fd2..135f37d358 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -130,7 +130,7 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig, coroutine):
+                      allow_oob, allow_preconfig, coroutine, allow_init_config):
         pass
 
     def visit_event(self, name, info, ifcond, features, arg_type, boxed):
@@ -746,7 +746,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
     def __init__(self, name, info, doc, ifcond, features,
                  arg_type, ret_type,
                  gen, success_response, boxed, allow_oob, allow_preconfig,
-                 coroutine):
+                 coroutine, allow_init_config):
         super().__init__(name, info, doc, ifcond, features)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -760,6 +760,7 @@ def __init__(self, name, info, doc, ifcond, features,
         self.allow_oob = allow_oob
         self.allow_preconfig = allow_preconfig
         self.coroutine = coroutine
+        self.allow_init_config = allow_init_config
 
     def check(self, schema):
         super().check(schema)
@@ -803,7 +804,7 @@ def visit(self, visitor):
             self.name, self.info, self.ifcond, self.features,
             self.arg_type, self.ret_type, self.gen, self.success_response,
             self.boxed, self.allow_oob, self.allow_preconfig,
-            self.coroutine)
+            self.coroutine, self.allow_init_config)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1111,6 +1112,7 @@ def _def_command(self, expr, info, doc):
         allow_oob = expr.get('allow-oob', False)
         allow_preconfig = expr.get('allow-preconfig', False)
         coroutine = expr.get('coroutine', False)
+        allow_init_config = expr.get('allow-init-config', False)
         ifcond = expr.get('if')
         features = self._make_features(expr.get('features'), info)
         if isinstance(data, OrderedDict):
@@ -1124,7 +1126,7 @@ def _def_command(self, expr, info, doc):
                                            data, rets,
                                            gen, success_response,
                                            boxed, allow_oob, allow_preconfig,
-                                           coroutine))
+                                           coroutine, allow_init_config))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
-- 
2.25.1

[RFC PATCH 9/9] qapi: Allow some commands to be executed in machine 'initialized' phase

[Mirela Grujic]

The following commands are allowed to execute during the machine
'initialized' phase (enabled with 'allow-init-config' option):
1) query-machine-phase
2) next-machine-phase
3) advance-machine-phase
3) device_add

Note: for device_add command in qdev.json adding the 'allow-init-config'
option has no effect because the command appears to bypass QAPI (see
TODO at qapi/qdev.json:61). The option is added there solely to document
the intent.
For the same reason, the flags have to be explicitly set in
monitor_init_qmp_commands() when the device_add command is registered.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 qapi/machine.json | 7 ++++---
 qapi/qdev.json    | 3 ++-
 monitor/misc.c    | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 31872aae72..c77ea03acb 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1327,7 +1327,7 @@
 #
 ##
 { 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
-             'allow-preconfig': true }
+             'allow-preconfig': true, 'allow-init-config': true }
 
 ##
 # @next-machine-phase:
@@ -1351,7 +1351,8 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'next-machine-phase', 'allow-preconfig': true }
+{ 'command': 'next-machine-phase', 'allow-preconfig': true,
+             'allow-init-config': true }
 
 ##
 # @advance-machine-phase:
@@ -1377,4 +1378,4 @@
 #
 ##
 { 'command': 'advance-machine-phase', 'data' : {'phase': 'MachineInitPhase'},
-             'allow-preconfig': true }
+             'allow-preconfig': true, 'allow-init-config': true }
diff --git a/qapi/qdev.json b/qapi/qdev.json
index b83178220b..2706ef3efa 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -67,7 +67,8 @@
 ##
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
-  'gen': false } # so we can get the additional arguments
+  'gen': false, # so we can get the additional arguments
+  'allow-init-config': true }
 
 ##
 # @device_del:
diff --git a/monitor/misc.c b/monitor/misc.c
index 55f3744053..88a0edff0a 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -232,7 +232,7 @@ static void monitor_init_qmp_commands(void)
     qmp_init_marshal(&qmp_commands);
 
     qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
-                         QCO_NO_OPTIONS);
+                         QCO_ALLOW_INIT_CONFIG);
 
     QTAILQ_INIT(&qmp_cap_negotiation_commands);
     qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
-- 
2.25.1

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Paolo Bonzini]

Hi Mirela, this is very interesting!

It's unfortunate that I completely missed the discussions in 
January/February.  You might have noticed that in the 5.2/6.0 timeframe 
I worked on cleaning up the machine initialization phases and qemu_init. 
  The idea behind the cleanup was to identify clearly the steps from one 
phase to the next.  I am very happy that you are already benefitting 
from that work in this series and you were able to make a prototype with 
so little code.

My plan was a bit more ambitious though :) and it is laid out at 
https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence.

My plan was to have completely different binaries than the current 
qemu-system-*.  These would only have a handful of command line options 
(such as -name, -sandbox, -trace, -L) and would open a QMP connection on 
stdio.

All other command line option would be either considered legacy or 
adjusted to be part of two new QMP commands, machine-set and accel-set, 
which would advance through the phases like this:

PHASE_NO_MACHINE
    -> machine-set -> PHASE_MACHINE_CREATED ->
    -> accel-set -> PHASE_ACCEL_CREATED -> PHASE_MACHINE_INITIALIZED ->
    -> finish-machine-init -> PHASE_MACHINE_READY
    -> cont

I think this idea would work well with your plan below, because the 
preconfiguration that you need to do happens mostly at 
PHASE_MACHINE_INITIALIZED.

Of course, the disadvantage of my approach is that, in the initial 
version, a lot of capabilities of QEMU are not available (especially 
with respect to the UI, since there's no "display-add" command). 
However, the basic implementation of machine-set and accel-set should 
not really be *that* much more work, and it should be doable in parallel 
with the conversion efforts for those options.  For example, today I 
posted a series that maps -smp to -M (and then, SMP configuration would 
automatically become available in machine-set).

I have placed the skeleton of the work I was doing at 
https://gitlab.com/bonzini/qemu/ in the branch qemu-qmp-targets.  You 
can see a sample execution at 
https://asciinema.org/a/TXMX8EZh8Md0fZnuE7uhfv6cO.  I have not touched 
some of the patches in a long time, but I plan to give them a quick test 
tomorrow.  Starting from the code in that branch, it should not be hard 
to implement the machine-set and accel-set commands in the same fashion 
as QEMU 5.2's implementation of object-add.

Thanks for posting these patches, I have started a light review of them.

Paolo

On 13/05/21 10:25, Mirela Grujic wrote:
> The direction for this work has been set in the discussion thread:
> "About creating machines on the command line" in January/February 2021:
> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg01839.html
> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01070.html
> 
> To customize a machine via QMP we need the ability to stop QEMU at a specific
> machine initialization phase.
> 
> Currently, machine initialization phases are:
> 1) no-machine: machine does not exist yet (current_machine == NULL)
> 2) machine-created: machine exists, but its accelerator does not
>     (current_machine->accelerator == NULL)
> 3) accel-created: machine's accelerator is configured
>     (current_machine->accelerator != NULL), but machine class's init() has not
>     been called (no properties validated, machine_init_done notifiers not
>     registered, no sysbus, etc.)
> 4) initialized: machine class's init() has been called, thus machine properties
>     are validated, machine_init_done notifiers registered, sysbus realized, etc.
>     Devices added at this phase are considered to be cold-plugged.
> 5) ready: machine_init_done notifiers are called, then QEMU is ready to start
>     CPUs. Devices added at this phase are considered to be hot-plugged.
> 
> QEMU can be stopped today using the -preconfig CLI option at phase 3
> (accel-created). This option was introduced to enable the QMP configuration of
> parameters that affect the machine initialization. We cannot add devices at
> this point because the machine class's init() has not been called, thus sysbus
> does not exist yet (a device cannot be added because there is no bus to attach
> it to).
> 
> QEMU can be also stopped using the -S CLI option at the machine ready phase.
> However, it is too late to add devices at this phase because the machine is
> already configured, and any devices added at this point are considered to be
> hot-plugged.
> 
> Since the existing -preconfig CLI option stops QEMU too early, and the -S option
> stops too late, we need a way to stop QEMU in between (after the machine is
> initialized and before it becomes ready).

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Mirela Grujic]

Hi Paolo,


Thanks for the feedback!


On 5/13/21 7:52 PM, Paolo Bonzini wrote:
> Hi Mirela, this is very interesting!
>
> It's unfortunate that I completely missed the discussions in 
> January/February.  You might have noticed that in the 5.2/6.0 
> timeframe I worked on cleaning up the machine initialization phases 
> and qemu_init.  The idea behind the cleanup was to identify clearly 
> the steps from one phase to the next.  I am very happy that you are 
> already benefitting from that work in this series and you were able to 
> make a prototype with so little code.


I was really happy to see your changes in 6.0, they simplified the 
implementation a lot. It looked like you're up to something bigger, and 
I'm glad that we can sync up now.


>
> My plan was a bit more ambitious though :) and it is laid out at 
> https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence.
>
> My plan was to have completely different binaries than the current 
> qemu-system-*.  These would only have a handful of command line 
> options (such as -name, -sandbox, -trace, -L) and would open a QMP 
> connection on stdio.
>
> All other command line option would be either considered legacy or 
> adjusted to be part of two new QMP commands, machine-set and 
> accel-set, which would advance through the phases like this:
>
> PHASE_NO_MACHINE
>    -> machine-set -> PHASE_MACHINE_CREATED ->
>    -> accel-set -> PHASE_ACCEL_CREATED -> PHASE_MACHINE_INITIALIZED ->
>    -> finish-machine-init -> PHASE_MACHINE_READY
>    -> cont
>
> I think this idea would work well with your plan below, because the 
> preconfiguration that you need to do happens mostly at 
> PHASE_MACHINE_INITIALIZED.
>
> Of course, the disadvantage of my approach is that, in the initial 
> version, a lot of capabilities of QEMU are not available (especially 
> with respect to the UI, since there's no "display-add" command). 
> However, the basic implementation of machine-set and accel-set should 
> not really be *that* much more work, and it should be doable in 
> parallel with the conversion efforts for those options.  For example, 
> today I posted a series that maps -smp to -M (and then, SMP 
> configuration would automatically become available in machine-set).


With our approach, transitioning to the QMP configuration suppose to 
happen gradually, i.e. we still specify some configuration options via 
command line. For your approach to be applicable to our use case we 
would at least need a QMP equivalent for the following:

qemu-system-riscv64 \
     -M sifive_dt \
     -cpu 
rv64,i=true,g=false,m=true,a=true,f=true,d=true,c=true,s=false,u=false,x-b=true,pmp=true,mmu=false,num-pmp-regions=8 
\
     -smp 1 \
     -device ...

AFAIU from the materials you shared, we would need to add -cpu and 
-device, but I don't see any reason why we wouldn't do this.


>
> I have placed the skeleton of the work I was doing at 
> https://gitlab.com/bonzini/qemu/ in the branch qemu-qmp-targets. You 
> can see a sample execution at 
> https://asciinema.org/a/TXMX8EZh8Md0fZnuE7uhfv6cO.  I have not touched 
> some of the patches in a long time, but I plan to give them a quick 
> test tomorrow.  Starting from the code in that branch, it should not 
> be hard to implement the machine-set and accel-set commands in the 
> same fashion as QEMU 5.2's implementation of object-add.
>

Ok, please let me know once you test, then I would run your code and 
play with it to better understand what needs to be done. Then I might 
come back with a couple of questions, so that we align on the TODOs. Is 
that ok with you?

Btw, when (in which version) did you plan to integrate the qemu-qmp-* 
support? I guess once machine-set/accel-set is implemented, but maybe 
I'm wrong...


> Thanks for posting these patches, I have started a light review of them.
>

If we would add the support for our use case to your approach, then this 
series would likely be split into a couple of patches that are 
applicable and the rest that is obsolete.


In summary, we believe it would be great to join efforts, please let us 
know how can we help.


Thanks,

Mirela


> Paolo
>
> On 13/05/21 10:25, Mirela Grujic wrote:
>> The direction for this work has been set in the discussion thread:
>> "About creating machines on the command line" in January/February 2021:
>> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg01839.html
>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01070.html
>>
>> To customize a machine via QMP we need the ability to stop QEMU at a 
>> specific
>> machine initialization phase.
>>
>> Currently, machine initialization phases are:
>> 1) no-machine: machine does not exist yet (current_machine == NULL)
>> 2) machine-created: machine exists, but its accelerator does not
>>     (current_machine->accelerator == NULL)
>> 3) accel-created: machine's accelerator is configured
>>     (current_machine->accelerator != NULL), but machine class's 
>> init() has not
>>     been called (no properties validated, machine_init_done notifiers 
>> not
>>     registered, no sysbus, etc.)
>> 4) initialized: machine class's init() has been called, thus machine 
>> properties
>>     are validated, machine_init_done notifiers registered, sysbus 
>> realized, etc.
>>     Devices added at this phase are considered to be cold-plugged.
>> 5) ready: machine_init_done notifiers are called, then QEMU is ready 
>> to start
>>     CPUs. Devices added at this phase are considered to be hot-plugged.
>>
>> QEMU can be stopped today using the -preconfig CLI option at phase 3
>> (accel-created). This option was introduced to enable the QMP 
>> configuration of
>> parameters that affect the machine initialization. We cannot add 
>> devices at
>> this point because the machine class's init() has not been called, 
>> thus sysbus
>> does not exist yet (a device cannot be added because there is no bus 
>> to attach
>> it to).
>>
>> QEMU can be also stopped using the -S CLI option at the machine ready 
>> phase.
>> However, it is too late to add devices at this phase because the 
>> machine is
>> already configured, and any devices added at this point are 
>> considered to be
>> hot-plugged.
>>
>> Since the existing -preconfig CLI option stops QEMU too early, and 
>> the -S option
>> stops too late, we need a way to stop QEMU in between (after the 
>> machine is
>> initialized and before it becomes ready).
>

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Paolo Bonzini]

On 14/05/21 14:48, Mirela Grujic wrote:
> 
> With our approach, transitioning to the QMP configuration suppose to 
> happen gradually, i.e. we still specify some configuration options via 
> command line. For your approach to be applicable to our use case we 
> would at least need a QMP equivalent for the following:
> 
> qemu-system-riscv64 \
>      -M sifive_dt \
>      -cpu 
> rv64,i=true,g=false,m=true,a=true,f=true,d=true,c=true,s=false,u=false,x-b=true,pmp=true,mmu=false,num-pmp-regions=8 
> \
>      -smp 1 \
>      -device ...
> 
> AFAIU from the materials you shared, we would need to add -cpu and 
> -device, but I don't see any reason why we wouldn't do this.

-cpu is indeed the big one that I had not looked at so far, while 
-device should be mostly covered by the existing device_add command.

One possibility for -cpu is to make it an argument of machine-set too. 
For example the above would be

{ 'execute': 'machine-set', arguments: {
     'type': 'sifive_dt',
     'smp': { 'cpus': 1 },
     'cpu': { 'model': 'rv64', 'i': true, 'g': false, ... }
}

> Ok, please let me know once you test, then I would run your code and
> play with it to better understand what needs to be done. Then I might
> come back with a couple of questions, so that we align on the TODOs. Is
> that ok with you?

Yes, of course.  I pushed something that at least compiles and passes a 
basic smoke test.

> Btw, when (in which version) did you plan to integrate the
> qemu-qmp-* support? I guess once machine-set/accel-set is implemented,  > but maybe I'm wrong...

Well, the right answer is "when somebody needs it".  The things that I 
was mostly interested in (e.g. compound properties for machines, such as 
smp in the example above) were all enablers for qemu-qmp-* but I was not 
really interested in the new binaries.  I did the qemu-qmp-* patches 
mostly to validate that the 5.2/6.0 refactoring of preconfig was going 
in the right direction.

However, if there is indeed somebody that needs it I'll contribute where 
our interests overlap.  In particular I can take care of converting the 
command line options to properties.

Paolo

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Daniel P. Berrangé]

On Fri, May 14, 2021 at 06:00:37PM +0200, Paolo Bonzini wrote:
> On 14/05/21 14:48, Mirela Grujic wrote:
> > 
> > With our approach, transitioning to the QMP configuration suppose to
> > happen gradually, i.e. we still specify some configuration options via
> > command line. For your approach to be applicable to our use case we
> > would at least need a QMP equivalent for the following:
> > 
> > qemu-system-riscv64 \
> >      -M sifive_dt \
> >      -cpu rv64,i=true,g=false,m=true,a=true,f=true,d=true,c=true,s=false,u=false,x-b=true,pmp=true,mmu=false,num-pmp-regions=8
> > \
> >      -smp 1 \
> >      -device ...
> > 
> > AFAIU from the materials you shared, we would need to add -cpu and
> > -device, but I don't see any reason why we wouldn't do this.
> 
> -cpu is indeed the big one that I had not looked at so far, while -device
> should be mostly covered by the existing device_add command.
> 
> One possibility for -cpu is to make it an argument of machine-set too. For
> example the above would be
> 
> { 'execute': 'machine-set', arguments: {
>     'type': 'sifive_dt',
>     'smp': { 'cpus': 1 },
>     'cpu': { 'model': 'rv64', 'i': true, 'g': false, ... }
> }

CPUs are a little complex because they have association with both machine
types and accelerator backends.  You have 'accel-set' being issued after
'machine-set', but the 'host' CPU model is only valid if you have set
the KVM accelerator, not TCG.

It is desirable that you get an error report about bad CPU at the time
you specify the CPU config, rather than have that error delayed to when
a later command runs and invalidates your CPU choice.

This is a long winded way of saying either 'accel-set' should be
run before 'machine-set', or 'cpu' would have to be set as its own
command or as part of 'accel-set'.

My gut feeling though is accel-set would be more logical being done
first, as that also influences the set of features available in other
areas of QEMU configuration. Was there a reason you listed it after
machine-set ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Paolo Bonzini]

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

Il ven 14 mag 2021, 18:20 Daniel P. Berrangé <berrange@redhat.com> ha
scritto:

> My gut feeling though is accel-set would be more logical being done
> first, as that also influences the set of features available in other
> areas of QEMU configuration. Was there a reason you listed it after
> machine-set ?
>

That was also my initial gut feeling, but actually right now the machine
influences the accelerator more than the other way round. For example the
initialization of the accelerator takes a machine so that for example on
x86 the per-architecture KVM knows whether to set up SMM. Also different
machines could use different modes for KVM (HV vs PR for ppc), and some
machines may not be virtualizable at all so they require TCG.

The host CPU these days is really a virtualization-only synonym for -cpu
max, which works for TCG as well. But you're right that x86 CPU flags are
dictated by the accelerator rather than the machine, so specifying it in
machine-set would be clumsy. On the other hand on ARM it's a bit of both:
for KVM it's basically always -cpu host so the accelerator is important;
but some machines may have an M profile CPU and some may have an A.

I don't have the sources at hand to check in which phase CPUs are created,
but it's definitely after ACCEL_CREATED. Adding a third command
cpu-model-set is probably the easiest way to proceed.

Paolo


> Regards,
> Daniel
> --
> |: https://berrange.com      -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-
> https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-
> https://www.instagram.com/dberrange :|
>
>

[-- Attachment #2: Type: text/html, Size: 2947 bytes --]

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Igor Mammedov]

On Fri, 14 May 2021 20:32:22 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il ven 14 mag 2021, 18:20 Daniel P. Berrangé <berrange@redhat.com> ha
> scritto:
> 
> > My gut feeling though is accel-set would be more logical being done
> > first, as that also influences the set of features available in other
> > areas of QEMU configuration. Was there a reason you listed it after
> > machine-set ?
> >  
> 
> That was also my initial gut feeling, but actually right now the machine
> influences the accelerator more than the other way round. For example the
> initialization of the accelerator takes a machine so that for example on
> x86 the per-architecture KVM knows whether to set up SMM. Also different
> machines could use different modes for KVM (HV vs PR for ppc), and some
> machines may not be virtualizable at all so they require TCG.
> 
> The host CPU these days is really a virtualization-only synonym for -cpu
> max, which works for TCG as well. But you're right that x86 CPU flags are
> dictated by the accelerator rather than the machine, so specifying it in
> machine-set would be clumsy. On the other hand on ARM it's a bit of both:
> for KVM it's basically always -cpu host so the accelerator is important;
> but some machines may have an M profile CPU and some may have an A.
and some do not support -cpu/-smp/or rather ignore it and go ahead with hardcoded one/


> I don't have the sources at hand to check in which phase CPUs are created,
> but it's definitely after ACCEL_CREATED. Adding a third command
> cpu-model-set is probably the easiest way to proceed.


a naive question,
why not ditch -cpu completely and instantiate CPUs with

device_add cpu-foo,flagX=on/off

the former is just a kluge for making CLI based -smp initial_nr_cpu/-cpu foo, work.
I'd move that logic to CLI compat wrapper translating that to a series of device_add calls
for QMP based QEMU.

Also I maybe wrong, but I thought that -cpu nowadays does nothing except of translating
legacy cpu model name to cpu-type and flags to a bunch of '-global', which are applied
automatically when CPUa are created at board_init() time or later on when device_add is used.

> Paolo
> 
> 
> > Regards,
> > Daniel
> > --
> > |: https://berrange.com      -o-
> > https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org         -o-
> > https://fstop138.berrange.com :|
> > |: https://entangle-photo.org    -o-
> > https://www.instagram.com/dberrange :|
> >
> >  

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Igor Mammedov]

On Mon, 24 May 2021 19:20:37 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Fri, 14 May 2021 20:32:22 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > Il ven 14 mag 2021, 18:20 Daniel P. Berrangé <berrange@redhat.com> ha
> > scritto:
> >   
> > > My gut feeling though is accel-set would be more logical being done
> > > first, as that also influences the set of features available in other
> > > areas of QEMU configuration. Was there a reason you listed it after
> > > machine-set ?
> > >    
> > 
> > That was also my initial gut feeling, but actually right now the machine
> > influences the accelerator more than the other way round. For example the
> > initialization of the accelerator takes a machine so that for example on
> > x86 the per-architecture KVM knows whether to set up SMM. Also different
> > machines could use different modes for KVM (HV vs PR for ppc), and some
> > machines may not be virtualizable at all so they require TCG.
> > 
> > The host CPU these days is really a virtualization-only synonym for -cpu
> > max, which works for TCG as well. But you're right that x86 CPU flags are
> > dictated by the accelerator rather than the machine, so specifying it in
> > machine-set would be clumsy. On the other hand on ARM it's a bit of both:
> > for KVM it's basically always -cpu host so the accelerator is important;
> > but some machines may have an M profile CPU and some may have an A.  
> and some do not support -cpu/-smp/or rather ignore it and go ahead with hardcoded one/
> 
> 
> > I don't have the sources at hand to check in which phase CPUs are created,
> > but it's definitely after ACCEL_CREATED. Adding a third command
> > cpu-model-set is probably the easiest way to proceed.  
> 
> 
> a naive question,
> why not ditch -cpu completely and instantiate CPUs with
> 
> device_add cpu-foo,flagX=on/off
while writing my another reply to this thread, I realized we might need cpu type
at least for 3 reasons:
 1. It would be hard/impossible to convert every board to create CPUs with
    device_add after board_init(), many boards depend on CPUs being available
    at that time to wire up another devices created at the same time.
 2. If we can postpone cpu creation after board_init() time, user would need
    query-hotpluggable-cpus command to get which CPU type and topology properties
    to use with device_add. (that's what libvirt uses curently)
 3. Recent work on ECPY cpu topology issues, showed that the topology, that board
    generates from -smp might depends on CPU type (we got rid of that eventually but
    I wouldn't rule out possibility in the future).
    
What I would remove from -cpu behavior is resolving cpu-model to CPU type.
QMP command would use only cpu-type so it would be consistent with device_add
which already uses cpu-type, which user gets from query-hotpluggble-cpus.

All legacy cpu-model conversion can be pushed out into CLI compat binary or
up the stack to keep existing deployments running, and new VMs can use cpu types
directly.

Question is how user can query available (for given target/machine/accel) CPU types
using QMP, is there such command already?

> the former is just a kluge for making CLI based -smp initial_nr_cpu/-cpu foo, work.
> I'd move that logic to CLI compat wrapper translating that to a series of device_add calls
> for QMP based QEMU.
> 
> Also I maybe wrong, but I thought that -cpu nowadays does nothing except of translating
> legacy cpu model name to cpu-type and flags to a bunch of '-global', which are applied
> automatically when CPUa are created at board_init() time or later on when device_add is used.
> 
> > Paolo
> > 
> >   
> > > Regards,
> > > Daniel
> > > --
> > > |: https://berrange.com      -o-
> > > https://www.flickr.com/photos/dberrange :|
> > > |: https://libvirt.org         -o-
> > > https://fstop138.berrange.com :|
> > > |: https://entangle-photo.org    -o-
> > > https://www.instagram.com/dberrange :|
> > >
> > >    
> 
> 

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Hi Mirela, this is very interesting!
>
> It's unfortunate that I completely missed the discussions in
> January/February.  You might have noticed that in the 5.2/6.0
> timeframe I worked on cleaning up the machine initialization phases
> and qemu_init.   The idea behind the cleanup was to identify clearly
> the steps from one phase to the next.  I am very happy that you are
> already benefitting from that work in this series and you were able to
> make a prototype with so little code.
>
> My plan was a bit more ambitious though :) and it is laid out at
> https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence.
>
> My plan was to have completely different binaries than the current
> qemu-system-*.

Now you have 2x problems :)

>                 These would only have a handful of command line
> options (such as -name, -sandbox, -trace, -L) and would open a QMP
> connection on stdio.
>
> All other command line option would be either considered legacy or
> adjusted to be part of two new QMP commands, machine-set and
> accel-set, which would advance through the phases like this:
>
> PHASE_NO_MACHINE
>    -> machine-set -> PHASE_MACHINE_CREATED ->
>    -> accel-set -> PHASE_ACCEL_CREATED -> PHASE_MACHINE_INITIALIZED ->
>    -> finish-machine-init -> PHASE_MACHINE_READY
>    -> cont

Is machine-set one big command, or a sequence of commands, where each
command configures just one thing?

Same for accel-set.

Permit me to go off on a tangent: how much and what kind of magic do we
want in the initialization sequence?

The existing order is a confusing mess grown out of a half-hearted
attempt to make things just work.  We've talked about it a few times,
e.g. here:

    Message-ID: <87poomg77m.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg00163.html

Are you aiming for "leave ordering to the user", or for "do the right
thing" (the user's order doesn't matter)", or for "neither; confusing
messes are fun"?

Another tangent:

1. A command line option is like a QMP command that returns nothing.
Generating CLI options from a QAPI schema is no harder than generating
QMP commands, it's just work.

2. A configuration file is like a command line, only easier to work with
when configuration becomes non-trivial.  Generating configuration file
language from a QAPI schema is no harder than generating QMP commands /
CLI options, it's just work.

3. QMP is strictly more powerful than CLI or comfiguration file.  It's
also less convenient for ad hoc use by humans: compare -readconfig
vm123.cfg to feeding the equivalent QMP commands with socat.

4. We'll see whether the convenience for humans can motivate us to put
in the work.

> I think this idea would work well with your plan below, because the
> preconfiguration that you need to do happens mostly at 
> PHASE_MACHINE_INITIALIZED.
>
> Of course, the disadvantage of my approach is that, in the initial
> version, a lot of capabilities of QEMU are not available (especially 
> with respect to the UI, since there's no "display-add"
> command). However, the basic implementation of machine-set and
> accel-set should not really be *that* much more work, and it should be
> doable in parallel with the conversion efforts for those options.  For
> example, today I posted a series that maps -smp to -M (and then, SMP
> configuration would automatically become available in machine-set).
>
> I have placed the skeleton of the work I was doing at
> https://gitlab.com/bonzini/qemu/ in the branch qemu-qmp-targets.  You 
> can see a sample execution at
> https://asciinema.org/a/TXMX8EZh8Md0fZnuE7uhfv6cO.  I have not touched 
> some of the patches in a long time, but I plan to give them a quick
> test tomorrow.  Starting from the code in that branch, it should not
> be hard to implement the machine-set and accel-set commands in the
> same fashion as QEMU 5.2's implementation of object-add.
>
> Thanks for posting these patches, I have started a light review of them.

Please cc: on future work in this area.

I'm about to drop off for two weeks of vacation, though :)

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Paolo Bonzini]

On 21/05/21 13:32, Markus Armbruster wrote:
> PHASE_NO_MACHINE
>    -> machine-set -> PHASE_MACHINE_CREATED ->
>    -> accel-set -> PHASE_ACCEL_CREATED -> PHASE_MACHINE_INITIALIZED ->
>    -> finish-machine-init -> PHASE_MACHINE_READY
>    -> cont
>
> Is machine-set one big command, or a sequence of commands, where each
> command configures just one thing?
> 
> Same for accel-set.

They would be almost 1:1 mappings with -M and -accel.  If we add a third 
command for the CPU model, machine-set and accel-set would be basically 
as big as device_add or object-add.

So the full flow would be

  PHASE_NO_MACHINE
     -> machine-set -> PHASE_MACHINE_CREATED ->
     -> accel-set -> PHASE_ACCEL_CREATED ->
     -> cpu-model-set -> PHASE_MACHINE_INITIALIZED ->
     -> device_add...
     -> finish-machine-init -> PHASE_MACHINE_READY
     -> cont

> Permit me to go off on a tangent: how much and what kind of magic do we
> want in the initialization sequence?

No magic at all, because the QMP configuration would be entirely 
-nodefaults.  Default devices, for boards that need them, can be created 
by setting properties such as serial0, netdev0 in machine-set (and in no 
other way).

Paolo

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Mirela Grujic]

I cc-ed Igor because he worked on preconfig support.


On 5/13/21 7:52 PM, Paolo Bonzini wrote:
> Hi Mirela, this is very interesting!
>
> It's unfortunate that I completely missed the discussions in 
> January/February.  You might have noticed that in the 5.2/6.0 
> timeframe I worked on cleaning up the machine initialization phases 
> and qemu_init.  The idea behind the cleanup was to identify clearly 
> the steps from one phase to the next.  I am very happy that you are 
> already benefitting from that work in this series and you were able to 
> make a prototype with so little code.
>
> My plan was a bit more ambitious though :) and it is laid out at 
> https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence.
>
> My plan was to have completely different binaries than the current 
> qemu-system-*.  These would only have a handful of command line 
> options (such as -name, -sandbox, -trace, -L) and would open a QMP 
> connection on stdio.
>
> All other command line option would be either considered legacy or 
> adjusted to be part of two new QMP commands, machine-set and 
> accel-set, which would advance through the phases like this:
>
> PHASE_NO_MACHINE
>    -> machine-set -> PHASE_MACHINE_CREATED ->
>    -> accel-set -> PHASE_ACCEL_CREATED -> PHASE_MACHINE_INITIALIZED ->


My understanding is that an equivalent of previously supported 
'preconfig' state is PHASE_ACCEL_CREATED, from the perspective of the 
QMP configuration that Igor implemented. In other words, I believe that 
when -preconfig CLI option was passed, QEMU was waiting for the QMP 
configuration in PHASE_ACCEL_CREATED phase. Now, if accel-set advances 
the machine directly to PHASE_MACHINE_INITIALIZED, there will be no 
chance to configure what Igor did with -preconfig.

Is this something you don't want to support anymore, or it can be 
configured in another way? Or is this something that we haven't thought 
of yet, but we should?


Another point I'm trying to make here - sooner or later I believe we 
will run again into the issue similar to the one described above. I 
truly believe that the commands you proposed (machine-set and accel-set) 
should be kept as is, except that they wouldn't automatically advance 
the machine initialization phase. I think the best approach would be to 
combine your proposal with mine: the machine-set and accel-set commands 
would configure stuff as you proposed, but there will be separate 
commands to advance machine phases (next-machine-phase and 
target/set-machine-phase). I also believe such a separation would be 
extremely important for troubleshooting configurations.


Btw, we already have such a separation because we will reuse device_add 
command, that only does the configuration. I believe it will be much 
cleaner and flexible for future development to separate QMP commands 
into 1) those that configure stuff, and 2) those that control advancing 
of the machine init phases.

Moreover, once it turns out that a command receives too many arguments 
and should be split into few, it will be easier to do so if a single 
command did not include both configuration and control-flow. Similar 
holds for adding new commands into the flow.


Please let me know what you think about the proposal.


Thanks,

Mirela


> -> finish-machine-init -> PHASE_MACHINE_READY
>    -> cont
>
> I think this idea would work well with your plan below, because the 
> preconfiguration that you need to do happens mostly at 
> PHASE_MACHINE_INITIALIZED.
>
> Of course, the disadvantage of my approach is that, in the initial 
> version, a lot of capabilities of QEMU are not available (especially 
> with respect to the UI, since there's no "display-add" command). 
> However, the basic implementation of machine-set and accel-set should 
> not really be *that* much more work, and it should be doable in 
> parallel with the conversion efforts for those options.  For example, 
> today I posted a series that maps -smp to -M (and then, SMP 
> configuration would automatically become available in machine-set).
>
> I have placed the skeleton of the work I was doing at 
> https://gitlab.com/bonzini/qemu/ in the branch qemu-qmp-targets. You 
> can see a sample execution at 
> https://asciinema.org/a/TXMX8EZh8Md0fZnuE7uhfv6cO.  I have not touched 
> some of the patches in a long time, but I plan to give them a quick 
> test tomorrow.  Starting from the code in that branch, it should not 
> be hard to implement the machine-set and accel-set commands in the 
> same fashion as QEMU 5.2's implementation of object-add.
>
> Thanks for posting these patches, I have started a light review of them.
>
> Paolo
>
> On 13/05/21 10:25, Mirela Grujic wrote:
>> The direction for this work has been set in the discussion thread:
>> "About creating machines on the command line" in January/February 2021:
>> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg01839.html
>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01070.html
>>
>> To customize a machine via QMP we need the ability to stop QEMU at a 
>> specific
>> machine initialization phase.
>>
>> Currently, machine initialization phases are:
>> 1) no-machine: machine does not exist yet (current_machine == NULL)
>> 2) machine-created: machine exists, but its accelerator does not
>>     (current_machine->accelerator == NULL)
>> 3) accel-created: machine's accelerator is configured
>>     (current_machine->accelerator != NULL), but machine class's 
>> init() has not
>>     been called (no properties validated, machine_init_done notifiers 
>> not
>>     registered, no sysbus, etc.)
>> 4) initialized: machine class's init() has been called, thus machine 
>> properties
>>     are validated, machine_init_done notifiers registered, sysbus 
>> realized, etc.
>>     Devices added at this phase are considered to be cold-plugged.
>> 5) ready: machine_init_done notifiers are called, then QEMU is ready 
>> to start
>>     CPUs. Devices added at this phase are considered to be hot-plugged.
>>
>> QEMU can be stopped today using the -preconfig CLI option at phase 3
>> (accel-created). This option was introduced to enable the QMP 
>> configuration of
>> parameters that affect the machine initialization. We cannot add 
>> devices at
>> this point because the machine class's init() has not been called, 
>> thus sysbus
>> does not exist yet (a device cannot be added because there is no bus 
>> to attach
>> it to).
>>
>> QEMU can be also stopped using the -S CLI option at the machine ready 
>> phase.
>> However, it is too late to add devices at this phase because the 
>> machine is
>> already configured, and any devices added at this point are 
>> considered to be
>> hot-plugged.
>>
>> Since the existing -preconfig CLI option stops QEMU too early, and 
>> the -S option
>> stops too late, we need a way to stop QEMU in between (after the 
>> machine is
>> initialized and before it becomes ready).
>

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Paolo Bonzini]

On 21/05/21 16:06, Mirela Grujic wrote:
>>
>> PHASE_NO_MACHINE
>>    -> machine-set -> PHASE_MACHINE_CREATED ->
>>    -> accel-set -> PHASE_ACCEL_CREATED -> PHASE_MACHINE_INITIALIZED ->
> 
> 
> My understanding is that an equivalent of previously supported 
> 'preconfig' state is PHASE_ACCEL_CREATED, from the perspective of the 
> QMP configuration that Igor implemented. In other words, I believe that 
> when -preconfig CLI option was passed, QEMU was waiting for the QMP 
> configuration in PHASE_ACCEL_CREATED phase. Now, if accel-set advances 
> the machine directly to PHASE_MACHINE_INITIALIZED, there will be no 
> chance to configure what Igor did with -preconfig.

Right, that was only NUMA.  I have to check, but I think it can be moved 
to PHASE_MACHINE_CREATED.

Apart from that, if we add a third command for the CPU model, that third 
command would run from PHASE_ACCEL_CREATED so the pre-existing preconfig 
state would be accessible.

Paolo

> Is this something you don't want to support anymore, or it can be 
> configured in another way? Or is this something that we haven't thought 
> of yet, but we should?

Re: [RFC PATCH 0/9] Initial support for machine creation via QMP

[Igor Mammedov]

On Fri, 21 May 2021 18:57:36 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 21/05/21 16:06, Mirela Grujic wrote:
> >>
> >> PHASE_NO_MACHINE
> >>    -> machine-set -> PHASE_MACHINE_CREATED ->
> >>    -> accel-set -> PHASE_ACCEL_CREATED -> PHASE_MACHINE_INITIALIZED ->  
> > 
> > 
> > My understanding is that an equivalent of previously supported 
> > 'preconfig' state is PHASE_ACCEL_CREATED, from the perspective of the 
> > QMP configuration that Igor implemented. In other words, I believe that 
> > when -preconfig CLI option was passed, QEMU was waiting for the QMP 
> > configuration in PHASE_ACCEL_CREATED phase. Now, if accel-set advances 
> > the machine directly to PHASE_MACHINE_INITIALIZED, there will be no 
> > chance to configure what Igor did with -preconfig.  
> 
> Right, that was only NUMA.  I have to check, but I think it can be moved 
> to PHASE_MACHINE_CREATED.
Dependency for NUMA were:
  1: -smp/-cpu being parsed before set_numa_options QMP command is called
        it's necessary to for machine being able to provide topology for
        given -smp combination.

        -cpu is not must have dependency (currently), it was just conveniently
        available when building topology in possible_cpu_arch_ids(), setting
        cpu-type there could be deffered to the later time (actual user for
        CPU type is QMP command query-hotpluggable-cpus, so that user could know
        what cpu type and what properties to use with device_add at hot-add time).

  2: memory backends depended on accel (TCG)
     (I think, Paolo has removed that dependency)


> Apart from that, if we add a third command for the CPU model, that third 
> command would run from PHASE_ACCEL_CREATED so the pre-existing preconfig 
> state would be accessible.
> 
> Paolo
> 
> > Is this something you don't want to support anymore, or it can be 
> > configured in another way? Or is this something that we haven't thought 
> > of yet, but we should?  
> 
>