[RFC PATCH v3 0/5] QMP support for cold-plugging devices

[RFC PATCH v3 0/5] QMP support for cold-plugging devices

[Damien Hedde]

Hi all,

This series adds support for cold-plugging devices using QMP
commands. It is a step towards machine configuration using QMP, but
it does not allow the user to add more devices than he could do with
the CLI options before.

Right now we can add a device using 2 ways:
+ giving "-device" CLI option. In that case the device is
  cold-plugged: it is created before the machine becomes ready.
+ issuing device_add QMP command. In that case the device is
  hot-plugged (the command can not be issued before the machine is
  ready).

This series allows to issue device_add QMP to cold-plug a device
like we do with "-device" CLI option. All added QMP commands are
marked as 'unstable' in qapi as they are part of preconfig.
We achieve this by introducing a new 'x-machine-init' command to
stop the VM creation at a point where we can cold-plug devices.

We are aware of the ongoing discussion about preconfig future (see
[1]). This patchset includes no major modifications from the v2 (but
the scope is reduced) and "x-machine-init" simply stops the
configuration between qemu_board_init() and qemu_create_cli_devices()
function calls.

As a consequence, in the current state, the timeline is:
+ "x-machine-init" command
+ "device_add" cold-plug commands (no fw_cfg legacy order support)
+ "x-exit-preconfig" command will then trigger the following
+ "-soundhw" CLI options
+ "-fw_cfg" CLI options
+ usb devices creation
+ "-device" CLI options (with fw_cfg legacy order support)
+ some other devices creation (with fw_cfg legacy order support)

We don't know if the differences between -device/device_add are
acceptable or not. To reduce/remove them we could move the
"x-machine-init" stopping point. What do you think ?

Patches 1, 3 and 5 miss a review.

The series is organized as follow:

+ Patches 1 and 2 converts the MachinePhase enum to a qapi definition
  and add the 'query-machine-phase'. It allows to introspect the
  current machine phase during preconfig as we will now be able to
  reach several machine phases using QMP.
+ Patch 3 adds the 'x-machine-init' QMP command to stop QEMU at
  machine-initialized phase during preconfig.
+ Patch 4 allows issuing device_add QMP command during the
  machine-initialized phase.
+ Patch 5 improves the doc about preconfig in consequence. 

[1]: https://lore.kernel.org/qemu-devel/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mprivozn@redhat.com

Thanks for your feedback.

---
Damien

v3:
 + extracted patches related to cold-plugging devices from the v2 rfc
 + updated for rebase (qapi 'unstable' feature addition and 7.0 version bump)
 + fix check for x-machine-init command (and drop Alistair's ack-by on
   patch 4)
 + extracted only a bit of the doc patch
 + drop qdev_set_id patch because it was merged in Kevin's
   "qdev: Add JSON -device" series which did a lot of cleanups in
   device_add related functions:
   https://lore.kernel.org/qemu-devel/20211008133442.141332-1-kwolf@redhat.com

v2 was part of this rfc:
https://lore.kernel.org/qemu-devel/20210922161405.140018-1-damien.hedde@greensocs.com

Damien Hedde (1):
  docs/system: improve doc about preconfig

Mirela Grujic (4):
  rename MachineInitPhase enum constants for QAPI compatibility
  qapi: Implement query-machine-phase QMP command
  qapi: Implement x-machine-init QMP command
  qapi: Allow device_add to execute in machine initialized phase

 docs/system/managed-startup.rst | 20 +++++++-
 qapi/machine.json               | 87 +++++++++++++++++++++++++++++++++
 qapi/qdev.json                  |  3 +-
 include/hw/qdev-core.h          | 30 +-----------
 hw/core/machine-qmp-cmds.c      | 11 ++++-
 hw/core/machine.c               |  6 +--
 hw/core/qdev.c                  |  7 ++-
 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          | 15 ++++--
 softmmu/vl.c                    | 23 ++++++---
 ui/console.c                    |  3 +-
 15 files changed, 164 insertions(+), 51 deletions(-)

-- 
2.33.0

[RFC PATCH v3 1/5] rename MachineInitPhase enum constants for QAPI compatibility

[Damien Hedde]

From: Mirela Grujic <mirela.grujic@greensocs.com>

This commit is a preparation to switch to a QAPI definition
of the MachineInitPhase enum.

QAPI will generate enumeration constants prefixed with the
MACHINE_INIT_PHASE_, so rename values accordingly.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 include/hw/qdev-core.h     | 10 +++++-----
 hw/core/machine-qmp-cmds.c |  2 +-
 hw/core/machine.c          |  6 +++---
 hw/core/qdev.c             |  2 +-
 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               |  6 +++---
 ui/console.c               |  3 ++-
 11 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 20d3066595..ef2d612d39 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -849,30 +849,30 @@ bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
 
 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-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 4f4ab30f8c..ddbdc5212f 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -148,7 +148,7 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
 
 void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
 {
-    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
+    if (phase_check(MACHINE_INIT_PHASE_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 26ec54e726..8560bb4c42 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1178,7 +1178,7 @@ void machine_run_board_init(MachineState *machine)
 
     accel_init_interfaces(ACCEL_GET_CLASS(machine->accelerator));
     machine_class->init(machine);
-    phase_advance(PHASE_MACHINE_INITIALIZED);
+    phase_advance(MACHINE_INIT_PHASE_INITIALIZED);
 }
 
 static NotifierList machine_init_done_notifiers =
@@ -1187,7 +1187,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 (phase_check(MACHINE_INIT_PHASE_READY)) {
         notify->notify(notify, NULL);
     }
 }
@@ -1210,7 +1210,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 84f3019440..ccfd6f0dc4 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -674,7 +674,7 @@ static void device_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
 
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (phase_check(MACHINE_INIT_PHASE_READY)) {
         dev->hotplugged = 1;
         qdev_hot_added = true;
     }
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e5993c1ef5..f77d9e8d57 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1102,7 +1102,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 (phase_check(MACHINE_INIT_PHASE_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..7a9a81c4fe 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 (!phase_check(MACHINE_INIT_PHASE_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..b777a80be2 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 (phase_check(MACHINE_INIT_PHASE_READY)) {
         int new_granule = ctz64(new_mask);
         int cur_granule = ctz64(cur_mask);
 
diff --git a/monitor/hmp.c b/monitor/hmp.c
index b20737e63c..3275e7aeed 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -217,7 +217,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 phase_check(MACHINE_INIT_PHASE_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 01f3834db5..1d6a1c4716 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -250,7 +250,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)) {
+        (phase_check(MACHINE_INIT_PHASE_READY) && !dc->hotpluggable)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
                    "a pluggable device type");
         return NULL;
@@ -660,7 +660,8 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
         return NULL;
     }
 
-    if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus)) {
+    if (phase_check(MACHINE_INIT_PHASE_READY) && bus &&
+        !qbus_is_hotpluggable(bus)) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
         return NULL;
     }
@@ -674,7 +675,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     dev = qdev_new(driver);
 
     /* Check whether the hotplug is allowed by the machine */
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (phase_check(MACHINE_INIT_PHASE_READY)) {
         if (!qdev_hotplug_allowed(dev, errp)) {
             goto err_del_dev;
         }
@@ -1040,7 +1041,7 @@ int qemu_global_option(const char *str)
 
 bool qmp_command_available(const QmpCommand *cmd, Error **errp)
 {
-    if (!phase_check(PHASE_MACHINE_READY) &&
+    if (!phase_check(MACHINE_INIT_PHASE_READY) &&
         !(cmd->options & QCO_ALLOW_PRECONFIG)) {
         error_setg(errp, "The command '%s' is permitted only after machine initialization has completed",
                    cmd->name);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 1159a64bce..df19b911e6 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2732,7 +2732,7 @@ static void qemu_machine_creation_done(void)
 
 void qmp_x_exit_preconfig(Error **errp)
 {
-    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
+    if (phase_check(MACHINE_INIT_PHASE_INITIALIZED)) {
         error_setg(errp, "The command is permitted only before machine initialization");
         return;
     }
@@ -3715,14 +3715,14 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_apply_legacy_machine_options(machine_opts_dict);
     qemu_apply_machine_options(machine_opts_dict);
     qobject_unref(machine_opts_dict);
-    phase_advance(PHASE_MACHINE_CREATED);
+    phase_advance(MACHINE_INIT_PHASE_MACHINE_CREATED);
 
     /*
      * Note: uses machine properties such as kernel-irqchip, must run
      * after qemu_apply_machine_options.
      */
     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
diff --git a/ui/console.c b/ui/console.c
index 29a3e3f0f5..df66536a79 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1295,7 +1295,8 @@ 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 ||
+               phase_check(MACHINE_INIT_PHASE_READY)) {
         QemuConsole *last = QTAILQ_LAST(&consoles);
         s->index = last->index + 1;
         QTAILQ_INSERT_TAIL(&consoles, s, next);
-- 
2.33.0

Re: [RFC PATCH v3 1/5] rename MachineInitPhase enum constants for QAPI compatibility

[Alistair Francis]

On Thu, Nov 18, 2021 at 12:49 AM Damien Hedde
<damien.hedde@greensocs.com> wrote:
>
> From: Mirela Grujic <mirela.grujic@greensocs.com>
>
> This commit is a preparation to switch to a QAPI definition
> of the MachineInitPhase enum.
>
> QAPI will generate enumeration constants prefixed with the
> MACHINE_INIT_PHASE_, so rename values accordingly.
>
> Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/qdev-core.h     | 10 +++++-----
>  hw/core/machine-qmp-cmds.c |  2 +-
>  hw/core/machine.c          |  6 +++---
>  hw/core/qdev.c             |  2 +-
>  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               |  6 +++---
>  ui/console.c               |  3 ++-
>  11 files changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 20d3066595..ef2d612d39 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -849,30 +849,30 @@ bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
>
>  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-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 4f4ab30f8c..ddbdc5212f 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -148,7 +148,7 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error **errp)
>
>  void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
>  {
> -    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
> +    if (phase_check(MACHINE_INIT_PHASE_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 26ec54e726..8560bb4c42 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1178,7 +1178,7 @@ void machine_run_board_init(MachineState *machine)
>
>      accel_init_interfaces(ACCEL_GET_CLASS(machine->accelerator));
>      machine_class->init(machine);
> -    phase_advance(PHASE_MACHINE_INITIALIZED);
> +    phase_advance(MACHINE_INIT_PHASE_INITIALIZED);
>  }
>
>  static NotifierList machine_init_done_notifiers =
> @@ -1187,7 +1187,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 (phase_check(MACHINE_INIT_PHASE_READY)) {
>          notify->notify(notify, NULL);
>      }
>  }
> @@ -1210,7 +1210,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 84f3019440..ccfd6f0dc4 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -674,7 +674,7 @@ static void device_initfn(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
>
> -    if (phase_check(PHASE_MACHINE_READY)) {
> +    if (phase_check(MACHINE_INIT_PHASE_READY)) {
>          dev->hotplugged = 1;
>          qdev_hot_added = true;
>      }
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e5993c1ef5..f77d9e8d57 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1102,7 +1102,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 (phase_check(MACHINE_INIT_PHASE_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..7a9a81c4fe 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 (!phase_check(MACHINE_INIT_PHASE_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..b777a80be2 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 (phase_check(MACHINE_INIT_PHASE_READY)) {
>          int new_granule = ctz64(new_mask);
>          int cur_granule = ctz64(cur_mask);
>
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index b20737e63c..3275e7aeed 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -217,7 +217,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 phase_check(MACHINE_INIT_PHASE_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 01f3834db5..1d6a1c4716 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -250,7 +250,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)) {
> +        (phase_check(MACHINE_INIT_PHASE_READY) && !dc->hotpluggable)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
>                     "a pluggable device type");
>          return NULL;
> @@ -660,7 +660,8 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>          return NULL;
>      }
>
> -    if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus)) {
> +    if (phase_check(MACHINE_INIT_PHASE_READY) && bus &&
> +        !qbus_is_hotpluggable(bus)) {
>          error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>          return NULL;
>      }
> @@ -674,7 +675,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>      dev = qdev_new(driver);
>
>      /* Check whether the hotplug is allowed by the machine */
> -    if (phase_check(PHASE_MACHINE_READY)) {
> +    if (phase_check(MACHINE_INIT_PHASE_READY)) {
>          if (!qdev_hotplug_allowed(dev, errp)) {
>              goto err_del_dev;
>          }
> @@ -1040,7 +1041,7 @@ int qemu_global_option(const char *str)
>
>  bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>  {
> -    if (!phase_check(PHASE_MACHINE_READY) &&
> +    if (!phase_check(MACHINE_INIT_PHASE_READY) &&
>          !(cmd->options & QCO_ALLOW_PRECONFIG)) {
>          error_setg(errp, "The command '%s' is permitted only after machine initialization has completed",
>                     cmd->name);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 1159a64bce..df19b911e6 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2732,7 +2732,7 @@ static void qemu_machine_creation_done(void)
>
>  void qmp_x_exit_preconfig(Error **errp)
>  {
> -    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
> +    if (phase_check(MACHINE_INIT_PHASE_INITIALIZED)) {
>          error_setg(errp, "The command is permitted only before machine initialization");
>          return;
>      }
> @@ -3715,14 +3715,14 @@ void qemu_init(int argc, char **argv, char **envp)
>      qemu_apply_legacy_machine_options(machine_opts_dict);
>      qemu_apply_machine_options(machine_opts_dict);
>      qobject_unref(machine_opts_dict);
> -    phase_advance(PHASE_MACHINE_CREATED);
> +    phase_advance(MACHINE_INIT_PHASE_MACHINE_CREATED);
>
>      /*
>       * Note: uses machine properties such as kernel-irqchip, must run
>       * after qemu_apply_machine_options.
>       */
>      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
> diff --git a/ui/console.c b/ui/console.c
> index 29a3e3f0f5..df66536a79 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1295,7 +1295,8 @@ 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 ||
> +               phase_check(MACHINE_INIT_PHASE_READY)) {
>          QemuConsole *last = QTAILQ_LAST(&consoles);
>          s->index = last->index + 1;
>          QTAILQ_INSERT_TAIL(&consoles, s, next);
> --
> 2.33.0
>
>

[RFC PATCH v3 2/5] qapi: Implement query-machine-phase QMP command

[Damien Hedde]

From: Mirela Grujic <mirela.grujic@greensocs.com>

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

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---

v3:
 + add 'unstable' feature
 + bump to version 7.0
---
 qapi/machine.json          | 60 ++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-core.h     | 30 ++-----------------
 hw/core/machine-qmp-cmds.c |  9 ++++++
 hw/core/qdev.c             |  5 ++++
 4 files changed, 76 insertions(+), 28 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 067e3f5378..8e9a8afb1d 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1557,3 +1557,63 @@
 { 'command': 'x-query-usb',
   'returns': 'HumanReadableText',
   'features': [ 'unstable' ] }
+
+##
+# @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.
+#
+# Since: 7.0
+##
+{ 'enum': 'MachineInitPhase',
+  'data': [ 'no-machine', 'machine-created', 'accel-created', 'initialized',
+            'ready' ] }
+
+##
+# @MachineInitPhaseStatus:
+#
+# Information about machine initialization phase
+#
+# @phase: the machine initialization phase
+#
+# Since: 7.0
+##
+{ 'struct': 'MachineInitPhaseStatus',
+  'data': { 'phase': 'MachineInitPhase' } }
+
+##
+# @query-machine-phase:
+#
+# Return machine initialization phase
+#
+# Features:
+# @unstable: This command is part of the experimental preconfig mode.
+#
+# Since: 7.0
+#
+# Returns: MachineInitPhaseStatus
+#
+# Example:
+#
+# -> { "execute": "query-machine-phase" }
+# <- { "return": { "phase": "initialized" } }
+#
+##
+{ 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
+  'features' : ['unstable'],
+  'allow-preconfig': true }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index ef2d612d39..ac856821ab 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"
@@ -847,35 +848,8 @@ void device_listener_unregister(DeviceListener *listener);
  */
 bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
 
-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);
 
 #endif
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index ddbdc5212f..19f9da4c2d 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -244,3 +244,12 @@ HumanReadableText *qmp_x_query_numa(Error **errp)
  done:
     return human_readable_text_from_str(buf);
 }
+
+MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
+{
+    MachineInitPhaseStatus *status = g_malloc0(sizeof(*status));
+
+    status->phase = phase_get();
+
+    return status;
+}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index ccfd6f0dc4..f22c050c89 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -921,6 +921,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,
-- 
2.33.0

Re: [RFC PATCH v3 2/5] qapi: Implement query-machine-phase QMP command

[Alistair Francis]

On Thu, Nov 18, 2021 at 12:53 AM Damien Hedde
<damien.hedde@greensocs.com> wrote:
>
> From: Mirela Grujic <mirela.grujic@greensocs.com>
>
> The command returns current machine initialization phase.
> From now on, the MachineInitPhase enum is generated from the
> QAPI schema.
>
> Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
> v3:
>  + add 'unstable' feature
>  + bump to version 7.0
> ---
>  qapi/machine.json          | 60 ++++++++++++++++++++++++++++++++++++++
>  include/hw/qdev-core.h     | 30 ++-----------------
>  hw/core/machine-qmp-cmds.c |  9 ++++++
>  hw/core/qdev.c             |  5 ++++
>  4 files changed, 76 insertions(+), 28 deletions(-)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 067e3f5378..8e9a8afb1d 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1557,3 +1557,63 @@
>  { 'command': 'x-query-usb',
>    'returns': 'HumanReadableText',
>    'features': [ 'unstable' ] }
> +
> +##
> +# @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.
> +#
> +# Since: 7.0
> +##
> +{ 'enum': 'MachineInitPhase',
> +  'data': [ 'no-machine', 'machine-created', 'accel-created', 'initialized',
> +            'ready' ] }
> +
> +##
> +# @MachineInitPhaseStatus:
> +#
> +# Information about machine initialization phase
> +#
> +# @phase: the machine initialization phase
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'MachineInitPhaseStatus',
> +  'data': { 'phase': 'MachineInitPhase' } }
> +
> +##
> +# @query-machine-phase:
> +#
> +# Return machine initialization phase
> +#
> +# Features:
> +# @unstable: This command is part of the experimental preconfig mode.
> +#
> +# Since: 7.0
> +#
> +# Returns: MachineInitPhaseStatus
> +#
> +# Example:
> +#
> +# -> { "execute": "query-machine-phase" }
> +# <- { "return": { "phase": "initialized" } }
> +#
> +##
> +{ 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
> +  'features' : ['unstable'],
> +  'allow-preconfig': true }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index ef2d612d39..ac856821ab 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"
> @@ -847,35 +848,8 @@ void device_listener_unregister(DeviceListener *listener);
>   */
>  bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
>
> -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);
>
>  #endif
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index ddbdc5212f..19f9da4c2d 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -244,3 +244,12 @@ HumanReadableText *qmp_x_query_numa(Error **errp)
>   done:
>      return human_readable_text_from_str(buf);
>  }
> +
> +MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
> +{
> +    MachineInitPhaseStatus *status = g_malloc0(sizeof(*status));
> +
> +    status->phase = phase_get();
> +
> +    return status;
> +}
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index ccfd6f0dc4..f22c050c89 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -921,6 +921,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,
> --
> 2.33.0
>
>

[RFC PATCH v3 3/5] qapi: Implement x-machine-init QMP command

[Damien Hedde]

From: Mirela Grujic <mirela.grujic@greensocs.com>

The x-machine-init QMP command is available only if the -preconfig
option is used and the current machine initialization phase is
accel-created.

The command triggers QEMU to enter machine initialized phase and wait
for the QMP configuration. In the next commit, we will add the
possibility to create devices at this point. To exit the initialized
phase, use the existing x-exit-preconfig QMP command.

As part of preconfig mode, the command has the 'unstable' feature.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
Cc: Alistair Francis <alistair.francis@wdc.com>

v3:
 + add 'unstable' feature
 + bump the target version to 7.0
 + fix the entrance check (and drop alistair ack-by). In previous
   version we were only checking we were not too early, we now check
   we are not too late too.
---
 qapi/machine.json | 27 +++++++++++++++++++++++++++
 softmmu/vl.c      | 19 +++++++++++++++----
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 8e9a8afb1d..39c2397629 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1617,3 +1617,30 @@
 { 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
   'features' : ['unstable'],
   'allow-preconfig': true }
+
+##
+# @x-machine-init:
+#
+# Enter machine initialized phase
+#
+# Features:
+# @unstable: This command is part of the experimental preconfig mode.
+#
+# Since: 7.0
+#
+# Returns: nothing
+#
+# Notes: This command will trigger QEMU to execute initialization steps
+#        that are required to enter the machine initialized phase. The command
+#        is available only if the -preconfig command line option was passed and
+#        if the machine is currently in the accel-created phase.
+#
+# Example:
+#
+# -> { "execute": "x-machine-init" }
+# <- { "return": {} }
+#
+##
+{ 'command': 'x-machine-init',
+  'features' : ['unstable'],
+  'allow-preconfig': true }
diff --git a/softmmu/vl.c b/softmmu/vl.c
index df19b911e6..a3bbe7b249 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -123,6 +123,7 @@
 #include "qapi/qapi-visit-qom.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/qapi-commands-machine.h"
 #include "qapi/qmp/qerror.h"
 #include "sysemu/iothread.h"
 #include "qemu/guest-random.h"
@@ -2636,10 +2637,16 @@ static void qemu_init_displays(void)
     }
 }
 
-static void qemu_init_board(void)
+void qmp_x_machine_init(Error **errp)
 {
     MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
 
+    if (phase_get() != MACHINE_INIT_PHASE_ACCEL_CREATED) {
+        error_setg(errp, "The command is permitted only when "
+                         "the machine is in accel-created phase");
+        return;
+    }
+
     if (machine_class->default_ram_id && current_machine->ram_size &&
         numa_uses_legacy_mem() && !current_machine->ram_memdev_id) {
         create_default_memdev(current_machine, mem_path);
@@ -2732,12 +2739,16 @@ static void qemu_machine_creation_done(void)
 
 void qmp_x_exit_preconfig(Error **errp)
 {
-    if (phase_check(MACHINE_INIT_PHASE_INITIALIZED)) {
-        error_setg(errp, "The command is permitted only before machine initialization");
+    if (phase_check(MACHINE_INIT_PHASE_READY)) {
+        error_setg(errp, "The command is permitted only before "
+                         "the machine is ready");
         return;
     }
 
-    qemu_init_board();
+    if (!phase_check(MACHINE_INIT_PHASE_INITIALIZED)) {
+        qmp_x_machine_init(errp);
+    }
+
     qemu_create_cli_devices();
     qemu_machine_creation_done();
 
-- 
2.33.0

[RFC PATCH v3 4/5] qapi: Allow device_add to execute in machine initialized phase

[Damien Hedde]

From: Mirela Grujic <mirela.grujic@greensocs.com>

This commit allows to use the QMP command to add a cold-plugged
device like we can do with the CLI option -device.

Note: for device_add command in qdev.json adding the 'allow-preconfig'
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>
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 qapi/qdev.json         | 3 ++-
 monitor/misc.c         | 2 +-
 softmmu/qdev-monitor.c | 6 ++++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 69656b14df..51c5b2fbcf 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -74,7 +74,8 @@
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
   'gen': false, # so we can get the additional arguments
-  'features': ['json-cli'] }
+  'features': ['json-cli'],
+  'allow-preconfig': true }
 
 ##
 # @device_del:
diff --git a/monitor/misc.c b/monitor/misc.c
index a3a6e47844..fd56dd8f08 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -233,7 +233,7 @@ static void monitor_init_qmp_commands(void)
     qmp_init_marshal(&qmp_commands);
 
     qmp_register_command(&qmp_commands, "device_add",
-                         qmp_device_add, 0, 0);
+                         qmp_device_add, QCO_ALLOW_PRECONFIG, 0);
 
     QTAILQ_INIT(&qmp_cap_negotiation_commands);
     qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 1d6a1c4716..06729145b7 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -842,6 +842,12 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
     QemuOpts *opts;
     DeviceState *dev;
 
+    if (!phase_check(MACHINE_INIT_PHASE_INITIALIZED)) {
+        error_setg(errp, "The command is permitted only after "
+                         "the machine is initialized");
+        return;
+    }
+
     opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
     if (!opts) {
         return;
-- 
2.33.0

[RFC PATCH v3 5/5] docs/system: improve doc about preconfig

[Damien Hedde]

Separate -S / -preconfig sections and improve a bit the preconfig part.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 docs/system/managed-startup.rst | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/docs/system/managed-startup.rst b/docs/system/managed-startup.rst
index 9bcf98ea79..e1cdbb79b1 100644
--- a/docs/system/managed-startup.rst
+++ b/docs/system/managed-startup.rst
@@ -1,6 +1,9 @@
 Managed start up options
 ========================
 
+CPU Frezee start
+----------------
+
 In system mode emulation, it's possible to create a VM in a paused
 state using the ``-S`` command line option. In this state the machine
 is completely initialized according to command line options and ready
@@ -20,7 +23,12 @@ allowing VM code to run.
 
 However, at the ``-S`` pause point, it's impossible to configure options
 that affect initial VM creation (like: ``-smp``/``-m``/``-numa`` ...) or
-cold plug devices. The experimental ``--preconfig`` command line option
+cold plug devices.
+
+Preconfig (experimental)
+------------------------
+
+The experimental ``--preconfig`` command line option
 allows pausing QEMU before the initial VM creation, in a "preconfig" state,
 where additional queries and configuration can be performed via QMP
 before moving on to the resulting configuration startup. In the
@@ -32,4 +40,14 @@ machine, including but not limited to:
 - ``query-qmp-schema``
 - ``query-commands``
 - ``query-status``
+- ``query-machine-phase``
+- ``x-machine-init``
 - ``x-exit-preconfig``
+
+Some commands make QEMU to progress along the VM creation procedure:
+
+- ``x-machine-init`` initializes the machine. Devices can be added only after
+  this command has been issued.
+
+- ``x-exit-preconfig`` leaves the preconfig state. It can be issued at any time
+  during preconfig and it will finish the VM creation.
-- 
2.33.0

Re: [RFC PATCH v3 0/5] QMP support for cold-plugging devices

[Markus Armbruster]

Damien Hedde <damien.hedde@greensocs.com> writes:

> Hi all,
>
> This series adds support for cold-plugging devices using QMP
> commands. It is a step towards machine configuration using QMP, but
> it does not allow the user to add more devices than he could do with
> the CLI options before.
>
> Right now we can add a device using 2 ways:
> + giving "-device" CLI option. In that case the device is
>   cold-plugged: it is created before the machine becomes ready.
> + issuing device_add QMP command. In that case the device is
>   hot-plugged (the command can not be issued before the machine is
>   ready).
>
> This series allows to issue device_add QMP to cold-plug a device
> like we do with "-device" CLI option. All added QMP commands are
> marked as 'unstable' in qapi as they are part of preconfig.
> We achieve this by introducing a new 'x-machine-init' command to
> stop the VM creation at a point where we can cold-plug devices.
>
> We are aware of the ongoing discussion about preconfig future (see
> [1]). This patchset includes no major modifications from the v2 (but
> the scope is reduced) and "x-machine-init" simply stops the
> configuration between qemu_board_init() and qemu_create_cli_devices()
> function calls.
>
> As a consequence, in the current state, the timeline is:

"current state" = with this series applied?

> + "x-machine-init" command
> + "device_add" cold-plug commands (no fw_cfg legacy order support)
> + "x-exit-preconfig" command will then trigger the following
> + "-soundhw" CLI options
> + "-fw_cfg" CLI options
> + usb devices creation
> + "-device" CLI options (with fw_cfg legacy order support)
> + some other devices creation (with fw_cfg legacy order support)
>
> We don't know if the differences between -device/device_add are
> acceptable or not. To reduce/remove them we could move the
> "x-machine-init" stopping point. What do you think ?

I'm not sure I understand this paragraph.

I understand the difference between -device and device_add in master:
cold vs. hot plug.

Your patch series makes "cold" device_add possible, i.e. it reduces
(eliminates?) the difference between -device and device_add when the
latter is "cold".

What difference remains that moving 'the "x-machine-init" stopping
point' would 'reduce/remove'?

> Patches 1, 3 and 5 miss a review.
>
> The series is organized as follow:
>
> + Patches 1 and 2 converts the MachinePhase enum to a qapi definition
>   and add the 'query-machine-phase'. It allows to introspect the
>   current machine phase during preconfig as we will now be able to
>   reach several machine phases using QMP.

If we fold MachinePhase into RunState, we can reuse query-status.

Having two state machines run one after the other feels like one too
many.

> + Patch 3 adds the 'x-machine-init' QMP command to stop QEMU at
>   machine-initialized phase during preconfig.
> + Patch 4 allows issuing device_add QMP command during the
>   machine-initialized phase.
> + Patch 5 improves the doc about preconfig in consequence. 

I understand you want to make progress towards machine configuration
with QMP.  However, QEMU startup is (in my educated opinion) a hole, and
we should be wary of digging deeper.

The "timeline" you gave above illustrates this.  It's a complicated
shuffling of command line options and QMP commands that basically nobody
can keep in working memory.  We have reshuffled it / made it more
complicated quite a few times already to support new features.  Based on
your cover letter, I figure you're making it more complicated once more.

At some point, we need to stop digging us deeper into the hole.  This is
not an objection to merging your work.  It's a call to stop and think.

Let me quote the sketch I posted to the "Stabilize preconfig" thread:

1. Start event loop

2. Feed it CLI left to right.  Each option runs a handler just like each
   QMP command does.

   Options that read a configuration file inject the file into the feed.

   Options that create a monitor create it suspended.

   Options may advance the phase / run state, and they may require
   certain phase(s).

3. When we're done with CLI, resume any monitors we created.

4. Monitors now feed commands to the event loop.  Commands may advance
   the phase / run state, and they may require certain phase(s).

Users can do as much or as little with the CLI as they want.  You'd
probably want to set up a QMP monitor and no more.

device_add becomes possible at a certain state of the phase / run state
machine.  It changes from cold to hot plug at a certain later state.

> [1]: https://lore.kernel.org/qemu-devel/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mprivozn@redhat.com
>
> Thanks for your feedback.

Re: [RFC PATCH v3 0/5] QMP support for cold-plugging devices

[Damien Hedde]

On 11/20/21 10:00, Markus Armbruster wrote:
> Damien Hedde <damien.hedde@greensocs.com> writes:
> 
>> Hi all,
>>
>> This series adds support for cold-plugging devices using QMP
>> commands. It is a step towards machine configuration using QMP, but
>> it does not allow the user to add more devices than he could do with
>> the CLI options before.
>>
>> Right now we can add a device using 2 ways:
>> + giving "-device" CLI option. In that case the device is
>>    cold-plugged: it is created before the machine becomes ready.
>> + issuing device_add QMP command. In that case the device is
>>    hot-plugged (the command can not be issued before the machine is
>>    ready).
>>
>> This series allows to issue device_add QMP to cold-plug a device
>> like we do with "-device" CLI option. All added QMP commands are
>> marked as 'unstable' in qapi as they are part of preconfig.
>> We achieve this by introducing a new 'x-machine-init' command to
>> stop the VM creation at a point where we can cold-plug devices.
>>
>> We are aware of the ongoing discussion about preconfig future (see
>> [1]). This patchset includes no major modifications from the v2 (but
>> the scope is reduced) and "x-machine-init" simply stops the
>> configuration between qemu_board_init() and qemu_create_cli_devices()
>> function calls.
>>
>> As a consequence, in the current state, the timeline is:
> 
> "current state" = with this series applied?

yes. this patchset adds the first two steps.

> 
>> + "x-machine-init" command
>> + "device_add" cold-plug commands (no fw_cfg legacy order support)
>> + "x-exit-preconfig" command will then trigger the following
>> + "-soundhw" CLI options
>> + "-fw_cfg" CLI options
>> + usb devices creation
>> + "-device" CLI options (with fw_cfg legacy order support)
>> + some other devices creation (with fw_cfg legacy order support)
>>
>> We don't know if the differences between -device/device_add are
>> acceptable or not. To reduce/remove them we could move the
>> "x-machine-init" stopping point. What do you think ?
> 
> I'm not sure I understand this paragraph.
> 
> I understand the difference between -device and device_add in master:
> cold vs. hot plug.
> 
> Your patch series makes "cold" device_add possible, i.e. it reduces
> (eliminates?) the difference between -device and device_add when the
> latter is "cold".

Yes.
Apart, before this patchset cold-plugging with device_add was not 
possible at all.

So, any difference between -device and a cold device_add is added here. 
(no bad intention, the patch did not move since v1 and this part of the 
code is just really tricky to understand...)

> 
> What difference remains that moving 'the "x-machine-init" stopping
> point' would 'reduce/remove'?

To answer this, let's take a look at qemu_create_cli_devices() (I 
removed some comments).

| 1 static void qemu_create_cli_devices(void)
| 2 {
| 3     DeviceOption *opt;
| 4
| 5     soundhw_init();
| 6
| 7     qemu_opts_foreach(qemu_find_opts("fw_cfg"),
| 8                       parse_fw_cfg, fw_cfg_find(), &error_fatal);
| 9
|10     /* init USB devices */
|11     if (machine_usb(current_machine)) {
|12         if (foreach_device_config(DEV_USB, usb_parse) < 0)
|13             exit(1);
|14     }
|15
|16     /* init generic devices */
|17     rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
|18     qemu_opts_foreach(qemu_find_opts("device"),
|19                       device_init_func, NULL, &error_fatal);
|20     QTAILQ_FOREACH(opt, &device_opts, next) {
|21         loc_push_restore(&opt->loc);
|22         qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
|23         loc_pop(&opt->loc);
|24     }
|25     rom_reset_order_override();
|26 }

The configuration timeline is:
Line 3    : handle "-soundhw" (deprecated).
Line 7-8  : handle "-fw_cfg"
Line 10-14: related to USB devices
Line 18-19: handle "-device" CLI options (legacy cli format)
Line 20-24: handle "-device" CLI options (json format)

With this patchset implementation, we pause just before calling this 
function (it seemed logical to stop here, given the machine phase). But 
the above timeline happens after we paused and issued device_add to cold 
plug devices. As a consequence there is a difference between (1) giving 
a -device option and (2) issuing a device_add at this pause point.

The biggest difference is the fw_cfg option I think: it is related with 
the rom_set_order_override()/rom_reset_order_override() (line 17 and 
25). There is also the usb devices parts in between. I lack the 
knowledge about fw_cfg/usb to tell if it is important or not.

What I wanted to say is I don't know if the difference is acceptable. If 
we want device_add to support all -device use cases, it is not. In that 
case we need to stop either in the middle of this function (line 15) or 
at the end (better with your sketch in mind).

Note that rom_set_order_override()/rom_reset_order_override() only 
set/reset a switch variable that changes how fw_cfg files are sorted. It 
could be integrated into device_add code (and removed from the above 
function) without changing the behavior.

> 
>> Patches 1, 3 and 5 miss a review.
>>
>> The series is organized as follow:
>>
>> + Patches 1 and 2 converts the MachinePhase enum to a qapi definition
>>    and add the 'query-machine-phase'. It allows to introspect the
>>    current machine phase during preconfig as we will now be able to
>>    reach several machine phases using QMP.
> 
> If we fold MachinePhase into RunState, we can reuse query-status.
> 
> Having two state machines run one after the other feels like one too
> many.
> 
>> + Patch 3 adds the 'x-machine-init' QMP command to stop QEMU at
>>    machine-initialized phase during preconfig.
>> + Patch 4 allows issuing device_add QMP command during the
>>    machine-initialized phase.
>> + Patch 5 improves the doc about preconfig in consequence.
> 
> I understand you want to make progress towards machine configuration
> with QMP.  However, QEMU startup is (in my educated opinion) a hole, and
> we should be wary of digging deeper.
> 
> The "timeline" you gave above illustrates this.  It's a complicated
> shuffling of command line options and QMP commands that basically nobody
> can keep in working memory.  We have reshuffled it / made it more
> complicated quite a few times already to support new features.  Based on
> your cover letter, I figure you're making it more complicated once more.
> 
> At some point, we need to stop digging us deeper into the hole.  This is
> not an objection to merging your work.  It's a call to stop and think.

That's why we re-posted this as RFC. Reading the preconfig thread, I had 
the feeling what we've initially proposed 6 months ago was not going 
into the direction discussed in the thread. We don't want to put more 
effort in a dead-end but we are committed into fixing it so that it fits 
into the good direction.

Do you mean we should wait for "stabilize preconfig" thread to arrive to 
some conclusion before we continue to work on this ?

Thanks,
Damien

> 
> Let me quote the sketch I posted to the "Stabilize preconfig" thread:
> 
> 1. Start event loop
> 
> 2. Feed it CLI left to right.  Each option runs a handler just like each
>     QMP command does.
> 
>     Options that read a configuration file inject the file into the feed.
> 
>     Options that create a monitor create it suspended.
> 
>     Options may advance the phase / run state, and they may require
>     certain phase(s).
> 
> 3. When we're done with CLI, resume any monitors we created.
> 
> 4. Monitors now feed commands to the event loop.  Commands may advance
>     the phase / run state, and they may require certain phase(s).
> 
> Users can do as much or as little with the CLI as they want.  You'd
> probably want to set up a QMP monitor and no more.
> 
> device_add becomes possible at a certain state of the phase / run state
> machine.  It changes from cold to hot plug at a certain later state.
> 
>> [1]: https://lore.kernel.org/qemu-devel/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mprivozn@redhat.com
>>
>> Thanks for your feedback.
> 

Re: [RFC PATCH v3 0/5] QMP support for cold-plugging devices

[Markus Armbruster]

Damien Hedde <damien.hedde@greensocs.com> writes:

> On 11/20/21 10:00, Markus Armbruster wrote:
>> Damien Hedde <damien.hedde@greensocs.com> writes:
>> 
>>> Hi all,
>>>
>>> This series adds support for cold-plugging devices using QMP
>>> commands. It is a step towards machine configuration using QMP, but
>>> it does not allow the user to add more devices than he could do with
>>> the CLI options before.
>>>
>>> Right now we can add a device using 2 ways:
>>> + giving "-device" CLI option. In that case the device is
>>>    cold-plugged: it is created before the machine becomes ready.
>>> + issuing device_add QMP command. In that case the device is
>>>    hot-plugged (the command can not be issued before the machine is
>>>    ready).
>>>
>>> This series allows to issue device_add QMP to cold-plug a device
>>> like we do with "-device" CLI option. All added QMP commands are
>>> marked as 'unstable' in qapi as they are part of preconfig.
>>> We achieve this by introducing a new 'x-machine-init' command to
>>> stop the VM creation at a point where we can cold-plug devices.
>>>
>>> We are aware of the ongoing discussion about preconfig future (see
>>> [1]). This patchset includes no major modifications from the v2 (but
>>> the scope is reduced) and "x-machine-init" simply stops the
>>> configuration between qemu_board_init() and qemu_create_cli_devices()
>>> function calls.
>>>
>>> As a consequence, in the current state, the timeline is:
>> 
>> "current state" = with this series applied?
>
> yes. this patchset adds the first two steps.
>
>>> + "x-machine-init" command
>>> + "device_add" cold-plug commands (no fw_cfg legacy order support)
>>> + "x-exit-preconfig" command will then trigger the following
>>> + "-soundhw" CLI options
>>> + "-fw_cfg" CLI options
>>> + usb devices creation
>>> + "-device" CLI options (with fw_cfg legacy order support)
>>> + some other devices creation (with fw_cfg legacy order support)
>>>
>>> We don't know if the differences between -device/device_add are
>>> acceptable or not. To reduce/remove them we could move the
>>> "x-machine-init" stopping point. What do you think ?
>> 
>> I'm not sure I understand this paragraph.
>> I understand the difference between -device and device_add in master:
>> cold vs. hot plug.
>> 
>> Your patch series makes "cold" device_add possible, i.e. it reduces
>> (eliminates?) the difference between -device and device_add when the
>> latter is "cold".
>
> Yes.
> Apart, before this patchset cold-plugging with device_add was not
> possible at all.
>
> So, any difference between -device and a cold device_add is added
> here. (no bad intention, the patch did not move since v1 and this part
> of the code is just really tricky to understand...)
>
>> What difference remains that moving 'the "x-machine-init" stopping
>> point' would 'reduce/remove'?
>
> To answer this, let's take a look at qemu_create_cli_devices() (I
> removed some comments).
>
> | 1 static void qemu_create_cli_devices(void)
> | 2 {
> | 3     DeviceOption *opt;
> | 4
> | 5     soundhw_init();
> | 6
> | 7     qemu_opts_foreach(qemu_find_opts("fw_cfg"),
> | 8                       parse_fw_cfg, fw_cfg_find(), &error_fatal);
> | 9
> |10     /* init USB devices */
> |11     if (machine_usb(current_machine)) {
> |12         if (foreach_device_config(DEV_USB, usb_parse) < 0)
> |13             exit(1);
> |14     }
> |15
> |16     /* init generic devices */
> |17     rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
> |18     qemu_opts_foreach(qemu_find_opts("device"),
> |19                       device_init_func, NULL, &error_fatal);
> |20     QTAILQ_FOREACH(opt, &device_opts, next) {
> |21         loc_push_restore(&opt->loc);
> |22         qdev_device_add_from_qdict(opt->opts, true, &error_fatal);
> |23         loc_pop(&opt->loc);
> |24     }
> |25     rom_reset_order_override();
> |26 }
>
> The configuration timeline is:
> Line 3    : handle "-soundhw" (deprecated).
> Line 7-8  : handle "-fw_cfg"
> Line 10-14: related to USB devices
> Line 18-19: handle "-device" CLI options (legacy cli format)
> Line 20-24: handle "-device" CLI options (json format)
>
> With this patchset implementation, we pause just before calling this
> function (it seemed logical to stop here, given the machine
> phase). But the above timeline happens after we paused and issued
> device_add to cold plug devices. As a consequence there is a
> difference between (1) giving a -device option and (2) issuing a
> device_add at this pause point.

I see.

> The biggest difference is the fw_cfg option I think: it is related
> with the rom_set_order_override()/rom_reset_order_override() (line 17
> and 25). There is also the usb devices parts in between. I lack the 
> knowledge about fw_cfg/usb to tell if it is important or not.
>
> What I wanted to say is I don't know if the difference is
> acceptable. If we want device_add to support all -device use cases, it
> is not. In that case we need to stop either in the middle of this
> function (line 15) or at the end (better with your sketch in mind).
>
> Note that rom_set_order_override()/rom_reset_order_override() only
> set/reset a switch variable that changes how fw_cfg files are
> sorted. It could be integrated into device_add code (and removed from
> the above function) without changing the behavior.

For me, the part that puts me off is interleaving CLI and QMP.

We process the CLI in an order few people understand, and only while
staring at the code.  That's bad.

Injecting QMP at certain points in that sequence can only make it worse.

If I understand your proposal correctly, we need special QMP commands to
opt into / manage QMP command injection, not least to avoid incompatible
change.

For instance, this needs to keep working:

1. Plug a SCSI HBA with CLI, say -device virtio-scsi,id=scsi-hba0

2. Plug a SCSI device with QMP, say {"execute": "device_add",
   "arguments": {"driver": "scsi-cd"}}

>>> Patches 1, 3 and 5 miss a review.
>>>
>>> The series is organized as follow:
>>>
>>> + Patches 1 and 2 converts the MachinePhase enum to a qapi definition
>>>    and add the 'query-machine-phase'. It allows to introspect the
>>>    current machine phase during preconfig as we will now be able to
>>>    reach several machine phases using QMP.
>> 
>> If we fold MachinePhase into RunState, we can reuse query-status.
>> 
>> Having two state machines run one after the other feels like one too
>> many.
>> 
>>> + Patch 3 adds the 'x-machine-init' QMP command to stop QEMU at
>>>    machine-initialized phase during preconfig.
>>> + Patch 4 allows issuing device_add QMP command during the
>>>    machine-initialized phase.
>>> + Patch 5 improves the doc about preconfig in consequence.
>> 
>> I understand you want to make progress towards machine configuration
>> with QMP.  However, QEMU startup is (in my educated opinion) a hole, and
>> we should be wary of digging deeper.
>> 
>> The "timeline" you gave above illustrates this.  It's a complicated
>> shuffling of command line options and QMP commands that basically nobody
>> can keep in working memory.  We have reshuffled it / made it more
>> complicated quite a few times already to support new features.  Based on
>> your cover letter, I figure you're making it more complicated once more.
>> 
>> At some point, we need to stop digging us deeper into the hole.  This is
>> not an objection to merging your work.  It's a call to stop and think.
>
> That's why we re-posted this as RFC. Reading the preconfig thread, I
> had the feeling what we've initially proposed 6 months ago was not
> going into the direction discussed in the thread. We don't want to put
> more effort in a dead-end but we are committed into fixing it so that
> it fits into the good direction.

Appreciated!

> Do you mean we should wait for "stabilize preconfig" thread to arrive
> to some conclusion before we continue to work on this ?

"Wait for" feels dangerously passive.  "Push for"?

> Thanks,
> Damien
>
>> Let me quote the sketch I posted to the "Stabilize preconfig" thread:
>> 
>> 1. Start event loop
>> 
>> 2. Feed it CLI left to right.  Each option runs a handler just like each
>>     QMP command does.
>> 
>>     Options that read a configuration file inject the file into the feed.
>> 
>>     Options that create a monitor create it suspended.
>> 
>>     Options may advance the phase / run state, and they may require
>>     certain phase(s).
>> 
>> 3. When we're done with CLI, resume any monitors we created.
>> 
>> 4. Monitors now feed commands to the event loop.  Commands may advance
>>     the phase / run state, and they may require certain phase(s).
>> 
>> Users can do as much or as little with the CLI as they want.  You'd
>> probably want to set up a QMP monitor and no more.
>> 
>> device_add becomes possible at a certain state of the phase / run state
>> machine.  It changes from cold to hot plug at a certain later state.
>> 
>>> [1]: https://lore.kernel.org/qemu-devel/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mprivozn@redhat.com
>>>
>>> Thanks for your feedback.
>> 

Re: [RFC PATCH v3 0/5] QMP support for cold-plugging devices

[Daniel P. Berrangé]

On Wed, Nov 24, 2021 at 02:50:11PM +0100, Markus Armbruster wrote:
> Damien Hedde <damien.hedde@greensocs.com> writes:
> 
> > The biggest difference is the fw_cfg option I think: it is related
> > with the rom_set_order_override()/rom_reset_order_override() (line 17
> > and 25). There is also the usb devices parts in between. I lack the 
> > knowledge about fw_cfg/usb to tell if it is important or not.
> >
> > What I wanted to say is I don't know if the difference is
> > acceptable. If we want device_add to support all -device use cases, it
> > is not. In that case we need to stop either in the middle of this
> > function (line 15) or at the end (better with your sketch in mind).
> >
> > Note that rom_set_order_override()/rom_reset_order_override() only
> > set/reset a switch variable that changes how fw_cfg files are
> > sorted. It could be integrated into device_add code (and removed from
> > the above function) without changing the behavior.
> 
> For me, the part that puts me off is interleaving CLI and QMP.
> 
> We process the CLI in an order few people understand, and only while
> staring at the code.  That's bad.
> 
> Injecting QMP at certain points in that sequence can only make it worse.

Yep, I share your unease here.. especially wrt this quoted text
from later:

  > >> Users can do as much or as little with the CLI as they want.  You'd
  > >> probably want to set up a QMP monitor and no more.

I would say that is a case of overkill. It can only make our
lives harder as maintainers in the long term, if we have to
worry about such arbitrary mixing of QMP and CLI. This is
also why I'm pretty uneasy about the 'preconfig' stuff as
implemented today in general.

It is a half-way house that doesn't really give mgmt apps
what they want, which is a 100% QAPI-only config. If mgmt
apps start using preconfig, it won't make life any better
for them and will also lock QEMU maintainers into supporting
this half-way house.

We have a bit of a track record with QEMU of introducing
partial solutions and never quite finishing the job. There's
little strong incentive to ever finish it, if you can freely
mix both old and new style forever, and thus maintainers are
burdened forever with both.

IMHO, we should only try to support the non-mixed scenarios

  - 100% of hardware configured via CLI args
  - 100% of hardware configured via QAPI (whether live in
    QMP, or fed in via a QAPI based JSON/YAML config file)

so that we only have two clear cases we need to worry about
dealing with.

Focus our efforts 100% of the 100% QAPI scenario and don't
divert energy into short term hybrid solutions.


> >> Let me quote the sketch I posted to the "Stabilize preconfig" thread:
> >> 
> >> 1. Start event loop
> >> 
> >> 2. Feed it CLI left to right.  Each option runs a handler just like each
> >>     QMP command does.
> >> 
> >>     Options that read a configuration file inject the file into the feed.
> >> 
> >>     Options that create a monitor create it suspended.
> >> 
> >>     Options may advance the phase / run state, and they may require
> >>     certain phase(s).
> >> 
> >> 3. When we're done with CLI, resume any monitors we created.
> >> 
> >> 4. Monitors now feed commands to the event loop.  Commands may advance
> >>     the phase / run state, and they may require certain phase(s).
> >> 
> >> Users can do as much or as little with the CLI as they want.  You'd
> >> probably want to set up a QMP monitor and no more.
> >> 
> >> device_add becomes possible at a certain state of the phase / run state
> >> machine.  It changes from cold to hot plug at a certain later state.
> >> 
> >>> [1]: https://lore.kernel.org/qemu-devel/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mprivozn@redhat.com
> >>>
> >>> Thanks for your feedback.
> >> 
> 

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 v3 0/5] QMP support for cold-plugging devices

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Nov 24, 2021 at 02:50:11PM +0100, Markus Armbruster wrote:
>> Damien Hedde <damien.hedde@greensocs.com> writes:
>> 
>> > The biggest difference is the fw_cfg option I think: it is related
>> > with the rom_set_order_override()/rom_reset_order_override() (line 17
>> > and 25). There is also the usb devices parts in between. I lack the 
>> > knowledge about fw_cfg/usb to tell if it is important or not.
>> >
>> > What I wanted to say is I don't know if the difference is
>> > acceptable. If we want device_add to support all -device use cases, it
>> > is not. In that case we need to stop either in the middle of this
>> > function (line 15) or at the end (better with your sketch in mind).
>> >
>> > Note that rom_set_order_override()/rom_reset_order_override() only
>> > set/reset a switch variable that changes how fw_cfg files are
>> > sorted. It could be integrated into device_add code (and removed from
>> > the above function) without changing the behavior.
>> 
>> For me, the part that puts me off is interleaving CLI and QMP.
>> 
>> We process the CLI in an order few people understand, and only while
>> staring at the code.  That's bad.
>> 
>> Injecting QMP at certain points in that sequence can only make it worse.
>
> Yep, I share your unease here.. especially wrt this quoted text
> from later:
>
>   > >> Users can do as much or as little with the CLI as they want.  You'd
>   > >> probably want to set up a QMP monitor and no more.
>
> I would say that is a case of overkill. It can only make our
> lives harder as maintainers in the long term, if we have to
> worry about such arbitrary mixing of QMP and CLI. This is
> also why I'm pretty uneasy about the 'preconfig' stuff as
> implemented today in general.
>
> It is a half-way house that doesn't really give mgmt apps
> what they want, which is a 100% QAPI-only config. If mgmt
> apps start using preconfig, it won't make life any better
> for them and will also lock QEMU maintainers into supporting
> this half-way house.

Misunderstanding?  The paragraph you quoted is about this design:

    1. Start event loop
    
    2. Feed it CLI left to right.  Each option runs a handler just like each
        QMP command does.
    
        Options that read a configuration file inject the file into the feed.
    
        Options that create a monitor create it suspended.
    
        Options may advance the phase / run state, and they may require
        certain phase(s).
    
    3. When we're done with CLI, resume any monitors we created.
    
    4. Monitors now feed commands to the event loop.  Commands may advance
        the phase / run state, and they may require certain phase(s).
    
    Users can do as much or as little with the CLI as they want.  You'd
    probably want to set up a QMP monitor and no more.
    
    device_add becomes possible at a certain state of the phase / run state
    machine.  It changes from cold to hot plug at a certain later state.

Certainly enables 100% QAPI-only config.  It just doesn't *force* you to
100%.  Feature.

> We have a bit of a track record with QEMU of introducing
> partial solutions and never quite finishing the job. There's
> little strong incentive to ever finish it, if you can freely
> mix both old and new style forever, and thus maintainers are
> burdened forever with both.
>
> IMHO, we should only try to support the non-mixed scenarios
>
>   - 100% of hardware configured via CLI args
>   - 100% of hardware configured via QAPI (whether live in
>     QMP, or fed in via a QAPI based JSON/YAML config file)
>
> so that we only have two clear cases we need to worry about
> dealing with.
>
> Focus our efforts 100% of the 100% QAPI scenario and don't
> divert energy into short term hybrid solutions.

The design above pretty much requires 100% QAPI.

It's based on the notion that there's no real difference between a CLI
option and a QMP command that doesn't return a value.  So treat the CLI
more like a monitor.

For sanity's sake, make it not race with the other monitors by starting
them suspended.

This design is arguably *less* hybrid than one that treats a (severely
dumbed down) CLI unlike a monitor.

Re: [RFC PATCH v3 0/5] QMP support for cold-plugging devices

[Damien Hedde]

On 11/24/21 15:51, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Wed, Nov 24, 2021 at 02:50:11PM +0100, Markus Armbruster wrote:
>>> Damien Hedde <damien.hedde@greensocs.com> writes:
>>>
>>>> The biggest difference is the fw_cfg option I think: it is related
>>>> with the rom_set_order_override()/rom_reset_order_override() (line 17
>>>> and 25). There is also the usb devices parts in between. I lack the
>>>> knowledge about fw_cfg/usb to tell if it is important or not.
>>>>
>>>> What I wanted to say is I don't know if the difference is
>>>> acceptable. If we want device_add to support all -device use cases, it
>>>> is not. In that case we need to stop either in the middle of this
>>>> function (line 15) or at the end (better with your sketch in mind).
>>>>
>>>> Note that rom_set_order_override()/rom_reset_order_override() only
>>>> set/reset a switch variable that changes how fw_cfg files are
>>>> sorted. It could be integrated into device_add code (and removed from
>>>> the above function) without changing the behavior.
>>>
>>> For me, the part that puts me off is interleaving CLI and QMP.
>>>
>>> We process the CLI in an order few people understand, and only while
>>> staring at the code.  That's bad.
>>>
>>> Injecting QMP at certain points in that sequence can only make it worse.
>>
>> Yep, I share your unease here.. especially wrt this quoted text
>> from later:
>>
>>    > >> Users can do as much or as little with the CLI as they want.  You'd
>>    > >> probably want to set up a QMP monitor and no more.
>>
>> I would say that is a case of overkill. It can only make our
>> lives harder as maintainers in the long term, if we have to
>> worry about such arbitrary mixing of QMP and CLI. This is
>> also why I'm pretty uneasy about the 'preconfig' stuff as
>> implemented today in general.
>>
>> It is a half-way house that doesn't really give mgmt apps
>> what they want, which is a 100% QAPI-only config. If mgmt
>> apps start using preconfig, it won't make life any better
>> for them and will also lock QEMU maintainers into supporting
>> this half-way house.
> 
> Misunderstanding?  The paragraph you quoted is about this design:
> 
>      1. Start event loop
>      
>      2. Feed it CLI left to right.  Each option runs a handler just like each
>          QMP command does.
>      
>          Options that read a configuration file inject the file into the feed.
>      
>          Options that create a monitor create it suspended.
>      
>          Options may advance the phase / run state, and they may require
>          certain phase(s).
>      
>      3. When we're done with CLI, resume any monitors we created.
>      
>      4. Monitors now feed commands to the event loop.  Commands may advance
>          the phase / run state, and they may require certain phase(s).
>      
>      Users can do as much or as little with the CLI as they want.  You'd
>      probably want to set up a QMP monitor and no more.
>      
>      device_add becomes possible at a certain state of the phase / run state
>      machine.  It changes from cold to hot plug at a certain later state.
> 
> Certainly enables 100% QAPI-only config.  It just doesn't *force* you to
> 100%.  Feature.
> 
>> We have a bit of a track record with QEMU of introducing
>> partial solutions and never quite finishing the job. There's
>> little strong incentive to ever finish it, if you can freely
>> mix both old and new style forever, and thus maintainers are
>> burdened forever with both.
>>
>> IMHO, we should only try to support the non-mixed scenarios
>>
>>    - 100% of hardware configured via CLI args
>>    - 100% of hardware configured via QAPI (whether live in
>>      QMP, or fed in via a QAPI based JSON/YAML config file)
>>
>> so that we only have two clear cases we need to worry about
>> dealing with.
>>
>> Focus our efforts 100% of the 100% QAPI scenario and don't
>> divert energy into short term hybrid solutions.
> 
> The design above pretty much requires 100% QAPI.
> 
> It's based on the notion that there's no real difference between a CLI
> option and a QMP command that doesn't return a value.  So treat the CLI
> more like a monitor.
> 
> For sanity's sake, make it not race with the other monitors by starting
> them suspended.
> 
> This design is arguably *less* hybrid than one that treats a (severely
> dumbed down) CLI unlike a monitor.
>

It seems there is a big gap from where we are now to a full QAPI startup 
support.
Could we adopt a plan which would allow us to progress from where we are 
to full QAPI support in small steps ?

For example, the following:

  1. CLI/QMP interleaving seems to be big issue right now. We could 
solve this by making -preconfig stop only after all CLI options are 
"consumed".
For example if you give -preconfig and some -device, qemu won't stop 
before having created the devices.

Meaning you would do
$qemu [out-of-order CLI with -preconfig] then jump into the monitors.

Depending on your use case, you would have to give a few CLI options so 
that -preconfig stops early enough.

  2. Then we can enable QMP commands one by one corresponding to 
unsupported and needed/cleaned up CLI options. They will check and/or 
advance the phase/runstate.

Basically this would mean we have to first convert CLI options that are 
used last in the startup procedure.
Along the road we will be able to make -preconfig stop earlier and earlier.

You could do a ~0% CLI startup at any point during the development
$qemu -monitor... --preconfig
but you would have a reduced set of configuration possibilities due the 
lack of QAPI support.

In addition, optionally, if we want to go with the left-to-right CLI 
parsing and reading a CLI script file like Markus proposed, we could 
have something like:
$qemu [out-of-order CLI] --preconfig [in-order CLI]
with [in-order CLI] being a 1:1 equivalent of QMP commands. [in-order 
CLI] will still be parsed and executed before jumping in the monitors.

Damien

Re: [RFC PATCH v3 0/5] QMP support for cold-plugging devices

[Daniel P. Berrangé]

On Wed, Nov 24, 2021 at 03:51:23PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, Nov 24, 2021 at 02:50:11PM +0100, Markus Armbruster wrote:
> >> Damien Hedde <damien.hedde@greensocs.com> writes:
> >> 
> >> > The biggest difference is the fw_cfg option I think: it is related
> >> > with the rom_set_order_override()/rom_reset_order_override() (line 17
> >> > and 25). There is also the usb devices parts in between. I lack the 
> >> > knowledge about fw_cfg/usb to tell if it is important or not.
> >> >
> >> > What I wanted to say is I don't know if the difference is
> >> > acceptable. If we want device_add to support all -device use cases, it
> >> > is not. In that case we need to stop either in the middle of this
> >> > function (line 15) or at the end (better with your sketch in mind).
> >> >
> >> > Note that rom_set_order_override()/rom_reset_order_override() only
> >> > set/reset a switch variable that changes how fw_cfg files are
> >> > sorted. It could be integrated into device_add code (and removed from
> >> > the above function) without changing the behavior.
> >> 
> >> For me, the part that puts me off is interleaving CLI and QMP.
> >> 
> >> We process the CLI in an order few people understand, and only while
> >> staring at the code.  That's bad.
> >> 
> >> Injecting QMP at certain points in that sequence can only make it worse.
> >
> > Yep, I share your unease here.. especially wrt this quoted text
> > from later:
> >
> >   > >> Users can do as much or as little with the CLI as they want.  You'd
> >   > >> probably want to set up a QMP monitor and no more.
> >
> > I would say that is a case of overkill. It can only make our
> > lives harder as maintainers in the long term, if we have to
> > worry about such arbitrary mixing of QMP and CLI. This is
> > also why I'm pretty uneasy about the 'preconfig' stuff as
> > implemented today in general.
> >
> > It is a half-way house that doesn't really give mgmt apps
> > what they want, which is a 100% QAPI-only config. If mgmt
> > apps start using preconfig, it won't make life any better
> > for them and will also lock QEMU maintainers into supporting
> > this half-way house.
> 
> Misunderstanding?  The paragraph you quoted is about this design:
> 
>     1. Start event loop
>     
>     2. Feed it CLI left to right.  Each option runs a handler just like each
>         QMP command does.
>     
>         Options that read a configuration file inject the file into the feed.
>     
>         Options that create a monitor create it suspended.
>     
>         Options may advance the phase / run state, and they may require
>         certain phase(s).
>     
>     3. When we're done with CLI, resume any monitors we created.
>     
>     4. Monitors now feed commands to the event loop.  Commands may advance
>         the phase / run state, and they may require certain phase(s).
>     
>     Users can do as much or as little with the CLI as they want.  You'd
>     probably want to set up a QMP monitor and no more.
>     
>     device_add becomes possible at a certain state of the phase / run state
>     machine.  It changes from cold to hot plug at a certain later state.
> 
> Certainly enables 100% QAPI-only config.  It just doesn't *force* you to
> 100%.  Feature.

This is far away from how our CLI handling works today, since we don't
require left-to-right args. Converting existing binaries to this
approach is going to be hard with a high risk of regressions. This
is especiall true if we try todo an incremental conversion, of
different pieces of the CLI and allow arbitrary mixing of CLI and
QMP throughout.

IMHO a pre-requisite for changing CLI arg processing ordering, is
a fresh binary that leaves QemuOpts behind for its CLI, so any
usage is consistent with QAPI. 

> > We have a bit of a track record with QEMU of introducing
> > partial solutions and never quite finishing the job. There's
> > little strong incentive to ever finish it, if you can freely
> > mix both old and new style forever, and thus maintainers are
> > burdened forever with both.
> >
> > IMHO, we should only try to support the non-mixed scenarios
> >
> >   - 100% of hardware configured via CLI args
> >   - 100% of hardware configured via QAPI (whether live in
> >     QMP, or fed in via a QAPI based JSON/YAML config file)
> >
> > so that we only have two clear cases we need to worry about
> > dealing with.
> >
> > Focus our efforts 100% of the 100% QAPI scenario and don't
> > divert energy into short term hybrid solutions.
> 
> The design above pretty much requires 100% QAPI.
> 
> It's based on the notion that there's no real difference between a CLI
> option and a QMP command that doesn't return a value.  So treat the CLI
> more like a monitor.
> 
> For sanity's sake, make it not race with the other monitors by starting
> them suspended.
> 
> This design is arguably *less* hybrid than one that treats a (severely
> dumbed down) CLI unlike a monitor.

Yes, my concern is more about how that gets introduced into the code
for the existing binaries, vs having a clean break where we're 100%
QAPI and no back compat CLI options exist.

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 v3 0/5] QMP support for cold-plugging devices

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Damien Hedde <damien.hedde@greensocs.com> writes:

> > Patches 1, 3 and 5 miss a review.
> >
> > The series is organized as follow:
> >
> > + Patches 1 and 2 converts the MachinePhase enum to a qapi definition
> >   and add the 'query-machine-phase'. It allows to introspect the
> >   current machine phase during preconfig as we will now be able to
> >   reach several machine phases using QMP.
> 
> If we fold MachinePhase into RunState, we can reuse query-status.
> 
> Having two state machines run one after the other feels like one too
> many.

Be careful, the RunState is API and things watch for events on it, so
any changes to it are delicate.

Dave

> > + Patch 3 adds the 'x-machine-init' QMP command to stop QEMU at
> >   machine-initialized phase during preconfig.
> > + Patch 4 allows issuing device_add QMP command during the
> >   machine-initialized phase.
> > + Patch 5 improves the doc about preconfig in consequence. 
> 
> I understand you want to make progress towards machine configuration
> with QMP.  However, QEMU startup is (in my educated opinion) a hole, and
> we should be wary of digging deeper.
> 
> The "timeline" you gave above illustrates this.  It's a complicated
> shuffling of command line options and QMP commands that basically nobody
> can keep in working memory.  We have reshuffled it / made it more
> complicated quite a few times already to support new features.  Based on
> your cover letter, I figure you're making it more complicated once more.
> 
> At some point, we need to stop digging us deeper into the hole.  This is
> not an objection to merging your work.  It's a call to stop and think.
> 
> Let me quote the sketch I posted to the "Stabilize preconfig" thread:
> 
> 1. Start event loop
> 
> 2. Feed it CLI left to right.  Each option runs a handler just like each
>    QMP command does.
> 
>    Options that read a configuration file inject the file into the feed.
> 
>    Options that create a monitor create it suspended.
> 
>    Options may advance the phase / run state, and they may require
>    certain phase(s).
> 
> 3. When we're done with CLI, resume any monitors we created.
> 
> 4. Monitors now feed commands to the event loop.  Commands may advance
>    the phase / run state, and they may require certain phase(s).
> 
> Users can do as much or as little with the CLI as they want.  You'd
> probably want to set up a QMP monitor and no more.
> 
> device_add becomes possible at a certain state of the phase / run state
> machine.  It changes from cold to hot plug at a certain later state.
> 
> > [1]: https://lore.kernel.org/qemu-devel/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mprivozn@redhat.com
> >
> > Thanks for your feedback.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK