[PATCH v5 0/6] QAPI support for device cold-plug

[PATCH v5 0/6] QAPI support for device cold-plug

[Damien Hedde]

Hi all,

As of now dynamic cold plug of device is only possible using the CLI
"-device" option. This series add support for device cold-plug using QAPI.

Patches 2, 5 and 6 are not reviewed yet.

It relies on the use of the "preconfig" mode (only way to stop QEMU
early enough) and requires more control on the machine phase than we had
before.

This work is part of our work towards to build a machine from scratch
using QAPI (see v4 or [1]).
But this is an independent part which can already be used to add
devices on any machine using QAPI instead of having to use the CLI to
pass some options.

For example, in this command the network interface could be added using qapi:
> $ qemu-system-aarch64 -display none -M virt -cpu cortex-a53 \
>     -drive file=./images/rootfs.ext4,if=none,format=raw,id=hd0 \
>     -device virtio-blk-device,drive=hd0 \
>     -kernel ./images/Image -append "rootwait root=/dev/vda console=ttyAMA0" \
>     -netdev user,id=eth0 -device virtio-net-device,netdev=eth0 \
>     -serial stdio

By using the following command line:
> $ qemu-system-aarch64 -display none -M virt -cpu cortex-a53 \
>    -drive file=./images/rootfs.ext4,if=none,format=raw,id=hd0 \
>    -device virtio-blk-device,drive=hd0 \
>    -kernel ./images/Image -append "rootwait root=/dev/vda console=ttyAMA0" \
>    -serial stdio -preconfig -qmp socket,path=./qmpsocket,server
and then qmp-shell (or any other qmp tool) to add the network interface and
device:
> $ qmp-shell ./qmpsocket
> (QEMU) netdev_add type=user id=eth0
> {"return": {}}
> (QEMU) device_add driver=virtio-net-device netdev=eth0
> {"return": {}}
> (QEMU) x-exit-preconfig
> {"return": {}}

Thanks,
--
Damien

v5:
 + refactor patch 2 to avoid indentation changes

v4: https://lore.kernel.org/qemu-devel/20220223090706.4888-1-damien.hedde@greensocs.com/

[1]: https://github.com/GreenSocs/qemu-qmp-machines

Damien Hedde (5):
  machine: add phase_get() and document phase_check()/advance()
  machine&vl: introduce phase_until() to handle phase transitions
  vl: support machine-initialized target in phase_until()
  qapi/device_add: compute is_hotplug flag
  RFC qapi/device_add: handle the rom_order_override when cold-plugging

Mirela Grujic (1):
  qapi/device_add: Allow execution in machine initialized phase

 qapi/qdev.json         |  3 +-
 include/hw/qdev-core.h | 33 +++++++++++++++++++
 hw/core/qdev.c         |  5 +++
 monitor/misc.c         |  2 +-
 softmmu/qdev-monitor.c | 20 ++++++++++--
 softmmu/vl.c           | 72 ++++++++++++++++++++++++++++++++++++++----
 hmp-commands.hx        |  1 +
 7 files changed, 126 insertions(+), 10 deletions(-)

-- 
2.36.1

[PATCH v5 1/6] machine: add phase_get() and document phase_check()/advance()

[Damien Hedde]

phase_get() returns the current phase, we'll use it in next
commit.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/qdev-core.h | 19 +++++++++++++++++++
 hw/core/qdev.c         |  5 +++++
 2 files changed, 24 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 92c3d65208..e29c705b74 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -887,7 +887,26 @@ typedef enum MachineInitPhase {
     PHASE_MACHINE_READY,
 } MachineInitPhase;
 
+/*
+ * phase_get:
+ * Returns the current phase
+ */
+MachineInitPhase phase_get(void);
+
+/**
+ * phase_check:
+ * Test if current phase is at least @phase.
+ *
+ * Returns true if this is the case.
+ */
 extern bool phase_check(MachineInitPhase phase);
+
+/**
+ * @phase_advance:
+ * Update the current phase to @phase.
+ *
+ * Must only be used to make a single phase step.
+ */
 extern void phase_advance(MachineInitPhase phase);
 
 #endif
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 84f3019440..632dc0a4be 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -910,6 +910,11 @@ Object *qdev_get_machine(void)
 
 static MachineInitPhase machine_phase;
 
+MachineInitPhase phase_get(void)
+{
+    return machine_phase;
+}
+
 bool phase_check(MachineInitPhase phase)
 {
     return machine_phase >= phase;
-- 
2.36.1

[PATCH v5 2/6] machine&vl: introduce phase_until() to handle phase transitions

[Damien Hedde]

phase_until() is implemented in vl.c and is meant to be used
to make startup progress up to a specified phase being reached().
At this point, no behavior change is introduced: phase_until()
only supports a single double transition corresponding
to the functionality of qmp_exit_preconfig():
+ accel-created -> machine-initialized -> machine-ready

As a result qmp_exit_preconfig() now uses phase_until().

This commit is a preparation to support cold plugging a device
using qapi command (which will be introduced in a following commit).
For this we need fine grain control of the phase.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

v5:
  + refactor to avoid indentation change
---
 include/hw/qdev-core.h | 14 +++++++++++++
 softmmu/vl.c           | 46 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e29c705b74..5f73d06408 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -909,4 +909,18 @@ extern bool phase_check(MachineInitPhase phase);
  */
 extern void phase_advance(MachineInitPhase phase);
 
+/**
+ * @phase_until:
+ * @phase: the target phase
+ * @errp: error report
+ *
+ * Make the machine init progress until the target phase is reached.
+ *
+ * Its is a no-op is the target phase is the current or an earlier
+ * phase.
+ *
+ * Returns true in case of success.
+ */
+extern bool phase_until(MachineInitPhase phase, Error **errp);
+
 #endif
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 84a31eba76..7f8d15b5b8 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2702,11 +2702,17 @@ void qmp_x_exit_preconfig(Error **errp)
         error_setg(errp, "The command is permitted only before machine initialization");
         return;
     }
+    phase_until(PHASE_MACHINE_READY, errp);
+}
 
+static void qemu_phase_ready(Error **errp)
+{
     qemu_init_board();
+    /* phase is now PHASE_MACHINE_INITIALIZED. */
     qemu_create_cli_devices();
     cxl_fixed_memory_window_link_targets(errp);
     qemu_machine_creation_done();
+    /* Phase is now PHASE_MACHINE_READY. */
 
     if (loadvm) {
         load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
@@ -2729,6 +2735,46 @@ void qmp_x_exit_preconfig(Error **errp)
     }
 }
 
+bool phase_until(MachineInitPhase phase, Error **errp)
+{
+    ERRP_GUARD();
+    if (!phase_check(PHASE_ACCEL_CREATED)) {
+        error_setg(errp, "Phase transition is not supported until accelerator"
+                   " is created");
+        return false;
+    }
+
+    while (!phase_check(phase)) {
+        MachineInitPhase cur_phase = phase_get();
+
+        switch (cur_phase) {
+        case PHASE_ACCEL_CREATED:
+            qemu_phase_ready(errp);
+            break;
+
+        default:
+            /*
+             * If we end up here, it is because we miss a case above.
+             */
+            error_setg(&error_abort, "Requested phase transition is not"
+                       " implemented");
+            return false;
+        }
+
+        if (*errp) {
+            return false;
+        }
+
+        /*
+         * Ensure we made some progress.
+         * With the default case above, it should be enough to prevent
+         * any infinite loop.
+         */
+        assert(cur_phase < phase_get());
+    }
+    return true;
+}
+
 void qemu_init(int argc, char **argv, char **envp)
 {
     QemuOpts *opts;
-- 
2.36.1

[PATCH v5 3/6] vl: support machine-initialized target in phase_until()

[Damien Hedde]

phase_until() now supports the following transitions:
+ accel-created -> machine-initialized
+ machine-initialized -> machine-ready

As a consequence we can now support the use of qmp_exit_preconfig()
from phases _accel-created_ and _machine-initialized_.

This commit is a preparation to support cold plugging a device
using qapi (which will be introduced in a following commit). For this
we need fine grain control of the phase.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

v5: update due to refactor of previous commit
---
 softmmu/vl.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 7f8d15b5b8..ea15e37973 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2698,8 +2698,9 @@ static void qemu_machine_creation_done(void)
 
 void qmp_x_exit_preconfig(Error **errp)
 {
-    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
-        error_setg(errp, "The command is permitted only before machine initialization");
+    if (phase_check(PHASE_MACHINE_READY)) {
+        error_setg(errp, "The command is permitted only before"
+                         " machine is ready");
         return;
     }
     phase_until(PHASE_MACHINE_READY, errp);
@@ -2707,9 +2708,6 @@ void qmp_x_exit_preconfig(Error **errp)
 
 static void qemu_phase_ready(Error **errp)
 {
-    qemu_init_board();
-    /* phase is now PHASE_MACHINE_INITIALIZED. */
-    qemu_create_cli_devices();
     cxl_fixed_memory_window_link_targets(errp);
     qemu_machine_creation_done();
     /* Phase is now PHASE_MACHINE_READY. */
@@ -2749,6 +2747,24 @@ bool phase_until(MachineInitPhase phase, Error **errp)
 
         switch (cur_phase) {
         case PHASE_ACCEL_CREATED:
+            qemu_init_board();
+            /* Phase is now PHASE_MACHINE_INITIALIZED. */
+            /*
+             * Handle CLI devices now in order leave this case in a state
+             * where we can cold plug devices with QMP. The following call
+             * handles the CLI options:
+             * + -fw_cfg (has side effects on device cold plug)
+             * + -device
+             */
+            qemu_create_cli_devices();
+            /*
+             * At this point all CLI options are handled apart:
+             * + -S (autostart)
+             * + -incoming
+             */
+            break;
+
+        case PHASE_MACHINE_INITIALIZED:
             qemu_phase_ready(errp);
             break;
 
-- 
2.36.1

[PATCH v5 4/6] qapi/device_add: compute is_hotplug flag

[Damien Hedde]

Instead of checking the phase everytime, just store the result
in a flag. We will use more of it in the following commit.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 softmmu/qdev-monitor.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 12fe60c467..d68ef883b5 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -619,6 +619,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     char *id;
     DeviceState *dev = NULL;
     BusState *bus = NULL;
+    bool is_hotplug = phase_check(PHASE_MACHINE_READY);
 
     driver = qdict_get_try_str(opts, "driver");
     if (!driver) {
@@ -662,7 +663,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
         return NULL;
     }
 
-    if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus)) {
+    if (is_hotplug && bus && !qbus_is_hotpluggable(bus)) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
         return NULL;
     }
@@ -676,7 +677,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 (is_hotplug) {
         if (!qdev_hotplug_allowed(dev, errp)) {
             goto err_del_dev;
         }
-- 
2.36.1

[PATCH v5 5/6] RFC qapi/device_add: handle the rom_order_override when cold-plugging

[Damien Hedde]

rom_set_order_override() and rom_reset_order_override() were called
in qemu_create_cli_devices() to set the rom_order_override value
once and for all when creating the devices added on CLI.

Unfortunately this won't work with qapi commands.

Move the calls inside device_add so that it will be done in every
case:
+ CLI option: -device
+ QAPI command: device_add

rom_[set|reset]_order_override() are implemented in hw/core/loader.c
They either do nothing or call fw_cfg_[set|reset]_order_override().
The later functions are implemented in hw/nvram/fw_cfg.c and only
change an integer value of a "global" variable.
In consequence, there are no complex side effects involved and we can
safely move them from outside the -device option loop to the inner
function.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

I see 2 other ways to handle this:

1. Adding a new option to device_add.

We could add a new boolean (_rom_order_override_ for example) option
tot he qapi command. This flag would then be forced to "true" when
handling "-device" parameter from CLI.
The flag default could be:
- always false
- false on hot-plug, true on cold-plug.

2. Adding a new qapi command
We could add one or two commands to do the
rom_[set|reset]_order_override() operation.
---
 softmmu/qdev-monitor.c | 11 +++++++++++
 softmmu/vl.c           |  2 --
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index d68ef883b5..7cbee2b0d8 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -43,6 +43,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/clock.h"
 #include "hw/boards.h"
+#include "hw/loader.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -673,6 +674,10 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
         return NULL;
     }
 
+    if (!is_hotplug) {
+        rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
+    }
+
     /* create device */
     dev = qdev_new(driver);
 
@@ -714,6 +719,9 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     if (!qdev_realize(DEVICE(dev), bus, errp)) {
         goto err_del_dev;
     }
+    if (!is_hotplug) {
+        rom_reset_order_override();
+    }
     return dev;
 
 err_del_dev:
@@ -721,6 +729,9 @@ err_del_dev:
         object_unparent(OBJECT(dev));
         object_unref(OBJECT(dev));
     }
+    if (!is_hotplug) {
+        rom_reset_order_override();
+    }
     return NULL;
 }
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ea15e37973..5a0d54b595 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2635,7 +2635,6 @@ static void qemu_create_cli_devices(void)
     }
 
     /* init generic devices */
-    rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
     qemu_opts_foreach(qemu_find_opts("device"),
                       device_init_func, NULL, &error_fatal);
     QTAILQ_FOREACH(opt, &device_opts, next) {
@@ -2652,7 +2651,6 @@ static void qemu_create_cli_devices(void)
         object_unref(OBJECT(dev));
         loc_pop(&opt->loc);
     }
-    rom_reset_order_override();
 }
 
 static void qemu_machine_creation_done(void)
-- 
2.36.1

[PATCH v5 6/6] qapi/device_add: Allow execution 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>
---

v4:
 + use phase_until()
 + add missing flag in hmp-commands.hx
---
 qapi/qdev.json         | 3 ++-
 monitor/misc.c         | 2 +-
 softmmu/qdev-monitor.c | 4 ++++
 hmp-commands.hx        | 1 +
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 26cd10106b..2e2de41499 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -77,7 +77,8 @@
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
   'gen': false, # so we can get the additional arguments
-  'features': ['json-cli', 'json-cli-hotplug'] }
+  'features': ['json-cli', 'json-cli-hotplug'],
+  'allow-preconfig': true }
 
 ##
 # @device_del:
diff --git a/monitor/misc.c b/monitor/misc.c
index 6c5bb82d3b..d3d413d70c 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 7cbee2b0d8..c53f62be51 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -855,6 +855,10 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
     QemuOpts *opts;
     DeviceState *dev;
 
+    if (!phase_until(PHASE_MACHINE_INITIALIZED, errp)) {
+        return;
+    }
+
     opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
     if (!opts) {
         return;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 03e6a73d1f..0091b8e2dd 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -672,6 +672,7 @@ ERST
         .help       = "add device, like -device on the command line",
         .cmd        = hmp_device_add,
         .command_completion = device_add_completion,
+        .flags      = "p",
     },
 
 SRST
-- 
2.36.1

Re: [PATCH v5 1/6] machine: add phase_get() and document phase_check()/advance()

[Jim Shu]

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

Tested-by: Jim Shu <jim.shu@sifive.com>

On Thu, May 19, 2022 at 11:41 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

> phase_get() returns the current phase, we'll use it in next
> commit.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/qdev-core.h | 19 +++++++++++++++++++
>  hw/core/qdev.c         |  5 +++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..e29c705b74 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -887,7 +887,26 @@ typedef enum MachineInitPhase {
>      PHASE_MACHINE_READY,
>  } MachineInitPhase;
>
> +/*
> + * phase_get:
> + * Returns the current phase
> + */
> +MachineInitPhase phase_get(void);
> +
> +/**
> + * phase_check:
> + * Test if current phase is at least @phase.
> + *
> + * Returns true if this is the case.
> + */
>  extern bool phase_check(MachineInitPhase phase);
> +
> +/**
> + * @phase_advance:
> + * Update the current phase to @phase.
> + *
> + * Must only be used to make a single phase step.
> + */
>  extern void phase_advance(MachineInitPhase phase);
>
>  #endif
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 84f3019440..632dc0a4be 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -910,6 +910,11 @@ Object *qdev_get_machine(void)
>
>  static MachineInitPhase machine_phase;
>
> +MachineInitPhase phase_get(void)
> +{
> +    return machine_phase;
> +}
> +
>  bool phase_check(MachineInitPhase phase)
>  {
>      return machine_phase >= phase;
> --
> 2.36.1
>
>
>

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

Re: [PATCH v5 2/6] machine&vl: introduce phase_until() to handle phase transitions

[Jim Shu]

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

Tested-by: Jim Shu <jim.shu@sifive.com>

On Thu, May 19, 2022 at 11:41 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

> phase_until() is implemented in vl.c and is meant to be used
> to make startup progress up to a specified phase being reached().
> At this point, no behavior change is introduced: phase_until()
> only supports a single double transition corresponding
> to the functionality of qmp_exit_preconfig():
> + accel-created -> machine-initialized -> machine-ready
>
> As a result qmp_exit_preconfig() now uses phase_until().
>
> This commit is a preparation to support cold plugging a device
> using qapi command (which will be introduced in a following commit).
> For this we need fine grain control of the phase.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> v5:
>   + refactor to avoid indentation change
> ---
>  include/hw/qdev-core.h | 14 +++++++++++++
>  softmmu/vl.c           | 46 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index e29c705b74..5f73d06408 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -909,4 +909,18 @@ extern bool phase_check(MachineInitPhase phase);
>   */
>  extern void phase_advance(MachineInitPhase phase);
>
> +/**
> + * @phase_until:
> + * @phase: the target phase
> + * @errp: error report
> + *
> + * Make the machine init progress until the target phase is reached.
> + *
> + * Its is a no-op is the target phase is the current or an earlier
> + * phase.
> + *
> + * Returns true in case of success.
> + */
> +extern bool phase_until(MachineInitPhase phase, Error **errp);
> +
>  #endif
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 84a31eba76..7f8d15b5b8 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2702,11 +2702,17 @@ void qmp_x_exit_preconfig(Error **errp)
>          error_setg(errp, "The command is permitted only before machine
> initialization");
>          return;
>      }
> +    phase_until(PHASE_MACHINE_READY, errp);
> +}
>
> +static void qemu_phase_ready(Error **errp)
> +{
>      qemu_init_board();
> +    /* phase is now PHASE_MACHINE_INITIALIZED. */
>      qemu_create_cli_devices();
>      cxl_fixed_memory_window_link_targets(errp);
>      qemu_machine_creation_done();
> +    /* Phase is now PHASE_MACHINE_READY. */
>
>      if (loadvm) {
>          load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
> @@ -2729,6 +2735,46 @@ void qmp_x_exit_preconfig(Error **errp)
>      }
>  }
>
> +bool phase_until(MachineInitPhase phase, Error **errp)
> +{
> +    ERRP_GUARD();
> +    if (!phase_check(PHASE_ACCEL_CREATED)) {
> +        error_setg(errp, "Phase transition is not supported until
> accelerator"
> +                   " is created");
> +        return false;
> +    }
> +
> +    while (!phase_check(phase)) {
> +        MachineInitPhase cur_phase = phase_get();
> +
> +        switch (cur_phase) {
> +        case PHASE_ACCEL_CREATED:
> +            qemu_phase_ready(errp);
> +            break;
> +
> +        default:
> +            /*
> +             * If we end up here, it is because we miss a case above.
> +             */
> +            error_setg(&error_abort, "Requested phase transition is not"
> +                       " implemented");
> +            return false;
> +        }
> +
> +        if (*errp) {
> +            return false;
> +        }
> +
> +        /*
> +         * Ensure we made some progress.
> +         * With the default case above, it should be enough to prevent
> +         * any infinite loop.
> +         */
> +        assert(cur_phase < phase_get());
> +    }
> +    return true;
> +}
> +
>  void qemu_init(int argc, char **argv, char **envp)
>  {
>      QemuOpts *opts;
> --
> 2.36.1
>
>
>

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

Re: [PATCH v5 3/6] vl: support machine-initialized target in phase_until()

[Jim Shu]

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

Tested-by: Jim Shu <jim.shu@sifive.com>

On Thu, May 19, 2022 at 11:36 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

> phase_until() now supports the following transitions:
> + accel-created -> machine-initialized
> + machine-initialized -> machine-ready
>
> As a consequence we can now support the use of qmp_exit_preconfig()
> from phases _accel-created_ and _machine-initialized_.
>
> This commit is a preparation to support cold plugging a device
> using qapi (which will be introduced in a following commit). For this
> we need fine grain control of the phase.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>
> v5: update due to refactor of previous commit
> ---
>  softmmu/vl.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 7f8d15b5b8..ea15e37973 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2698,8 +2698,9 @@ static void qemu_machine_creation_done(void)
>
>  void qmp_x_exit_preconfig(Error **errp)
>  {
> -    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
> -        error_setg(errp, "The command is permitted only before machine
> initialization");
> +    if (phase_check(PHASE_MACHINE_READY)) {
> +        error_setg(errp, "The command is permitted only before"
> +                         " machine is ready");
>          return;
>      }
>      phase_until(PHASE_MACHINE_READY, errp);
> @@ -2707,9 +2708,6 @@ void qmp_x_exit_preconfig(Error **errp)
>
>  static void qemu_phase_ready(Error **errp)
>  {
> -    qemu_init_board();
> -    /* phase is now PHASE_MACHINE_INITIALIZED. */
> -    qemu_create_cli_devices();
>      cxl_fixed_memory_window_link_targets(errp);
>      qemu_machine_creation_done();
>      /* Phase is now PHASE_MACHINE_READY. */
> @@ -2749,6 +2747,24 @@ bool phase_until(MachineInitPhase phase, Error
> **errp)
>
>          switch (cur_phase) {
>          case PHASE_ACCEL_CREATED:
> +            qemu_init_board();
> +            /* Phase is now PHASE_MACHINE_INITIALIZED. */
> +            /*
> +             * Handle CLI devices now in order leave this case in a state
> +             * where we can cold plug devices with QMP. The following call
> +             * handles the CLI options:
> +             * + -fw_cfg (has side effects on device cold plug)
> +             * + -device
> +             */
> +            qemu_create_cli_devices();
> +            /*
> +             * At this point all CLI options are handled apart:
> +             * + -S (autostart)
> +             * + -incoming
> +             */
> +            break;
> +
> +        case PHASE_MACHINE_INITIALIZED:
>              qemu_phase_ready(errp);
>              break;
>
> --
> 2.36.1
>
>
>

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

Re: [PATCH v5 4/6] qapi/device_add: compute is_hotplug flag

[Jim Shu]

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

Tested-by: Jim Shu <jim.shu@sifive.com>

On Thu, May 19, 2022 at 11:37 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

> Instead of checking the phase everytime, just store the result
> in a flag. We will use more of it in the following commit.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  softmmu/qdev-monitor.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 12fe60c467..d68ef883b5 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -619,6 +619,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
>      char *id;
>      DeviceState *dev = NULL;
>      BusState *bus = NULL;
> +    bool is_hotplug = phase_check(PHASE_MACHINE_READY);
>
>      driver = qdict_get_try_str(opts, "driver");
>      if (!driver) {
> @@ -662,7 +663,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
>          return NULL;
>      }
>
> -    if (phase_check(PHASE_MACHINE_READY) && bus &&
> !qbus_is_hotpluggable(bus)) {
> +    if (is_hotplug && bus && !qbus_is_hotpluggable(bus)) {
>          error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
>          return NULL;
>      }
> @@ -676,7 +677,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 (is_hotplug) {
>          if (!qdev_hotplug_allowed(dev, errp)) {
>              goto err_del_dev;
>          }
> --
> 2.36.1
>
>
>

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

Re: [PATCH v5 6/6] qapi/device_add: Allow execution in machine initialized phase

[Jim Shu]

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

Tested-by: Jim Shu <jim.shu@sifive.com>

On Thu, May 19, 2022 at 11:37 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

> 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>
> ---
>
> v4:
>  + use phase_until()
>  + add missing flag in hmp-commands.hx
> ---
>  qapi/qdev.json         | 3 ++-
>  monitor/misc.c         | 2 +-
>  softmmu/qdev-monitor.c | 4 ++++
>  hmp-commands.hx        | 1 +
>  4 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 26cd10106b..2e2de41499 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -77,7 +77,8 @@
>  { 'command': 'device_add',
>    'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
>    'gen': false, # so we can get the additional arguments
> -  'features': ['json-cli', 'json-cli-hotplug'] }
> +  'features': ['json-cli', 'json-cli-hotplug'],
> +  'allow-preconfig': true }
>
>  ##
>  # @device_del:
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 6c5bb82d3b..d3d413d70c 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 7cbee2b0d8..c53f62be51 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -855,6 +855,10 @@ void qmp_device_add(QDict *qdict, QObject **ret_data,
> Error **errp)
>      QemuOpts *opts;
>      DeviceState *dev;
>
> +    if (!phase_until(PHASE_MACHINE_INITIALIZED, errp)) {
> +        return;
> +    }
> +
>      opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
>      if (!opts) {
>          return;
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 03e6a73d1f..0091b8e2dd 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -672,6 +672,7 @@ ERST
>          .help       = "add device, like -device on the command line",
>          .cmd        = hmp_device_add,
>          .command_completion = device_add_completion,
> +        .flags      = "p",
>      },
>
>  SRST
> --
> 2.36.1
>
>
>

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