xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v2 00/16] Initial support for machine creation via QMP
@ 2021-09-22 16:13 Damien Hedde
  2021-09-22 16:13 ` [RFC PATCH v2 01/16] rename MachineInitPhase enum constants for QAPI compatibility Damien Hedde
                   ` (18 more replies)
  0 siblings, 19 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

Hi,

The goal of this work is to bring dynamic machine creation to QEMU:
we want to setup a machine without compiling a specific machine C
code. It would ease supporting highly configurable platforms (for
example resulting from an automated design flow). The requirements
for such configuration include begin able to specify the number of
cores, available peripherals, emmory mapping, IRQ mapping, etc.

This series focuses on the first step: populating a machine with
devices during its creation. We propose patches to support this
using QMP commands. This is a working set of patches and improves
over the earlier rfc (posted in May):
https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html

Although it is working and could be merged, it is tag as an RFC:
we probably need to discuss the conditions for allowing a device to
be created at an early stage. Patches 6, 10 and 13, 15 and 16 depend
on such conditions and are subject to change. Other patches are
unrelated to this point.

We address several issues in this series. They are detailed below.

## 1. Stoping QEMU to populate the machine with devices

QEMU goes through several steps (called _machine phases_) when
creating the machine: 'no-machine', 'machine-created',
'accel-created', 'initialized', and finally 'ready'. At 'ready'
phase, QEMU is ready to start (see Paolo's page
https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence for
more details).

Using the -preconfig CLI option, QEMU can be stopped today during
the 'accel-created' phase. Then the 'x-exit-preconfig' QMP command
triggers QEMU moving forwards to the completion of the machine
creation ('ready' phase).

The devices are created during the 'initialized' phase. 
In this phase the machine init() method has been executed and thus
machine properties have been handled. Although the sysbus exists and
the machine may have been populated by the init(), 
_machine_init_done_ notifiers have not been called yet. At this point
we can add more devices to a machine.

We propose to add 2 QMP commands:
+ The 'query-machine-phase' command would return the current machine
  phase.
+ The 'x-machine-init' command would advance the machine phase to
  'initialized'. 'x-exit-preconfig' could then still be used to
  advance to the last phase.

## 2. Adding devices

Right now, the user can create devices in 2 ways: using '-device' CLI
option or 'device_add' QMP command. Both are executed after the
machine is ready: such devices are hot-plugged. We propose to allow
'device_add' QMP command to be used during the 'initialized' phase.

In this series, we keep the constraint that the device must be
'user-creatable' (this is a device class flag). We do not see any
reason why a device the user can hot-plug could not be created at an
earlier stage.

This part is still RFC because, as Peter mentioned it (in this thread
https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg01933.html),
we may want additional or distinct conditions for:
+ device we can hot-plug
+ device we can add in '-preconfig' (cold-plug)
We are open to suggestions. We could for example add a
'preconfig-creatable' or 'init-creatable' flag to device class, which
can identify a set of devices we can create this way.

The main addition is how we handle the case of sysbus devices. Sysbus
devices are particular because unlike, for example, pci devices, you
have to manually handle the memory mapping and interrupts wiring. So
right now, a sysbus device is dynamically creatable (using -device
CLI option or device_add QMP command) only if:
+ it is 'user_creatable' (like any other device),
+ and it is in the current machine sysbus device allow list.

In this series, we propose to relax the second constraint during the
earlier phases of machine creation so that when using -preconfig we
can create any 'user-creatable' sysbus device. When the machine
progresses to the 'ready' phase, sysbus devices creation will come
back to the legacy behavior: it will be possible only based on the
per-machine authorization basis.

For sysbus devices, wiring interrupts is not a problem as we can use
the 'qom-set' QMP command, but memory mapping is.

## 3. Memory mapping

There is no point allowing the creation sysbus devices if we cannot
map them onto the memory bus (the 'sysbus').

As far as we know, right now, there is no way to add memory mapping
for sysbus device using QMP commands. We propose a 'x-sysbus-mmio-map'
command to do this. This command would only be allowed during the
'initialized' phase when using -preconfig.

## 4. Working example

The last patches of the series add and modify devices in order to
build a working machine starting from the 'none' machine.

We add a new sysbus device modeling a simple memory (ram or rom). We
also set 'user-creatable' flag of some sysbus devices. These are
trivial patches, but they depends on the conditions we choose to allow
creating devices with -preconfig. Therefore, there is really no need
to review them until we settled on the device conditions first.

With these devices (memory, ibex_uart, ibex_plic) we can dynamically
configure a part (we did not add the timer, but we could) the
opentitan machine very easily and run firmwares which demonstrates
interrupts and memory-mapping are working.

We use the existing qmp-shell script to issue machine devices
from a qmp commands script file which contains qmp commands listed in
a file.

The following qmp commands add some memories, an interrupt controller
and an uart with an interrupt.

cat > opentitan.qmp <<EOF
x-machine-init

# ROM 0x00008000
device_add        driver=sysbus-memory id=rom size=0x4000 readonly=true
x-sysbus-mmio-map device=rom addr=32768

# FLASH 0x20000000
device_add        driver=sysbus-memory id=flash size=0x80000 readonly=true
x-sysbus-mmio-map device=flash addr=536870912

# RAM 0x10000000
device_add        driver=sysbus-memory id=ram size=0x10000
x-sysbus-mmio-map device=ram addr=268435456

# PLIC 0x41010000
device_add        driver=ibex-plic id=plic
x-sysbus-mmio-map device=plic addr=1090584576

# UART 0x40000000
device_add        driver=ibex-uart id=uart chardev=serial0
x-sysbus-mmio-map device=uart addr=1073741824
qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2]

x-exit-preconfig
EOF

We've put the opentitan.qmp and a firmware opentitan-echo.elf here
(among some other qmp machine files we are working on):
https://github.com/GreenSocs/qemu-qmp-machines
This firmware is just a small interrupt-based program echoing back
whatever is sent in the uart.

QEMU should be run using the following command:
qemu-system-riscv32 -preconfig -qmp unix:/tmp/qmp-socket,server \
    -display none \
    -M none -cpu lowrisc-ibex \
    -serial mon:stdio \
    -device loader,addr=0x8090,cpu-num=0 \
    -device loader,file=opentitan-hello.elf \

and in other terminal to do the configuration (grep is just here to
remove comments):
grep -v -e '^#' opentitan.qmp | qmp-shell -v /tmp/qmp-socket

Alternatively we can load the firmware on the existing machine and
observe the same behavior:
qemu-system-riscv32 -display none \
     -M opentitan \
     -serial mon:stdio \
     -kernel opentitan-echo.elf

We chose this example because it is very simple and does not need a
lot of devices.

This work has still a lot of limitations. Cpus config is done the
normal way (the C machine does that): in our example we used the
'none' machine. We have work to do for handling backend
connection (for example net/nic are complicated) because the way it
is done in machine C code does not translate easily in QMP commands.
Firmware loading is also a bit tricky. We plan to work on this in
follow-up series.

The series is organized as follows:
- Patches 1 to 3 add qmp support to stop QEMU at an early phase
  to populate the machine with devices.
- Patches 4 to 6 prepare and allow issuing device_add during this phase.
- Patches 7 to 10 prepare and allow creating sysbus device during this phase.
- Patches 11 and 12 add the x-sysbus-mmio-map QMP command
- Patch 13 add the memory sysbus device to model ram and rom 
- Patch 14 adds some documentation
- Patches 15 and 16 set 'user_creatable' flag of ibex_uart and ibex_plic.

This work is supported by Greensocs, Sifive and Xilinx.

Thanks,
--
Damien

Damien Hedde (12):
  softmmu/qdev-monitor: add error handling in qdev_set_id
  qdev-monitor: prevent conflicts between qmp/device_add and cli/-device
  hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed
  qdev-monitor: Check sysbus device type before creating it
  hw/core/machine: Remove the dynamic sysbus devices type check
  qdev-monitor: allow adding any sysbus device before machine is ready
  softmmu/memory: add memory_region_try_add_subregion function
  add x-sysbus-mmio-map qmp command
  hw/mem/system-memory: add a memory sysbus device
  docs/system: add doc about the initialized machine phase and an
    example
  hw/char/ibex_uart: set user_creatable
  hw/intc/ibex_plic: set user_creatable

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 | 77 +++++++++++++++++++++++++++++
 qapi/machine.json               | 79 ++++++++++++++++++++++++++++++
 qapi/qdev.json                  | 24 ++++++++-
 include/exec/memory.h           | 22 +++++++++
 include/hw/boards.h             | 18 ++++++-
 include/hw/mem/sysbus-memory.h  | 32 ++++++++++++
 include/hw/qdev-core.h          | 30 +-----------
 include/monitor/qdev.h          | 25 +++++++++-
 hw/char/ibex_uart.c             |  1 +
 hw/core/machine-qmp-cmds.c      | 11 ++++-
 hw/core/machine.c               | 48 ++++++------------
 hw/core/qdev.c                  |  7 ++-
 hw/core/sysbus.c                | 41 ++++++++++++++++
 hw/intc/ibex_plic.c             |  1 +
 hw/mem/sysbus-memory.c          | 83 +++++++++++++++++++++++++++++++
 hw/pci/pci.c                    |  2 +-
 hw/usb/core.c                   |  2 +-
 hw/virtio/virtio-iommu.c        |  2 +-
 hw/xen/xen-legacy-backend.c     |  3 +-
 monitor/hmp.c                   |  2 +-
 monitor/misc.c                  |  2 +-
 softmmu/memory.c                | 22 ++++++---
 softmmu/qdev-monitor.c          | 86 +++++++++++++++++++++++++++------
 softmmu/vl.c                    | 23 ++++++---
 ui/console.c                    |  3 +-
 hw/mem/meson.build              |  2 +
 26 files changed, 547 insertions(+), 101 deletions(-)
 create mode 100644 include/hw/mem/sysbus-memory.h
 create mode 100644 hw/mem/sysbus-memory.c

-- 
2.33.0



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

* [RFC PATCH v2 01/16] rename MachineInitPhase enum constants for QAPI compatibility
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-09-22 16:13 ` [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command Damien Hedde
                   ` (17 subsequent siblings)
  18 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

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>
---
 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 34c8a7506a..859fd913bb 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -841,30 +841,30 @@ bool qdev_should_hide_device(QemuOpts *opts);
 
 typedef enum MachineInitPhase {
     /* current_machine is NULL.  */
-    PHASE_NO_MACHINE,
+    MACHINE_INIT_PHASE_NO_MACHINE,
 
     /* current_machine is not NULL, but current_machine->accel is NULL.  */
-    PHASE_MACHINE_CREATED,
+    MACHINE_INIT_PHASE_MACHINE_CREATED,
 
     /*
      * current_machine->accel is not NULL, but the machine properties have
      * not been validated and machine_class->init has not yet been called.
      */
-    PHASE_ACCEL_CREATED,
+    MACHINE_INIT_PHASE_ACCEL_CREATED,
 
     /*
      * machine_class->init has been called, thus creating any embedded
      * devices and validating machine properties.  Devices created at
      * this time are considered to be cold-plugged.
      */
-    PHASE_MACHINE_INITIALIZED,
+    MACHINE_INIT_PHASE_INITIALIZED,
 
     /*
      * QEMU is ready to start CPUs and devices created at this time
      * are considered to be hot-plugged.  The monitor is not restricted
      * to "preconfig" commands.
      */
-    PHASE_MACHINE_READY,
+    MACHINE_INIT_PHASE_READY,
 } MachineInitPhase;
 
 extern bool phase_check(MachineInitPhase phase);
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 216fdfaf3a..52168a3771 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -147,7 +147,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 067f42b528..9125c9aad0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1274,7 +1274,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 =
@@ -1283,7 +1283,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);
     }
 }
@@ -1306,7 +1306,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 cefc5eaa0a..c5fc704f55 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -904,7 +904,7 @@ static void device_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
 
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (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 23d2ae2ab2..5ed798b480 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 d50c3124e1..ad012b0b22 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -216,7 +216,7 @@ static bool cmd_can_preconfig(const HMPCommand *cmd)
 
 static bool cmd_available(const HMPCommand *cmd)
 {
-    return phase_check(PHASE_MACHINE_READY) || cmd_can_preconfig(cmd);
+    return 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 0705f00846..25275984bd 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -262,7 +262,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;
@@ -649,7 +649,8 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
         }
     }
 
-    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;
     }
@@ -663,7 +664,7 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
     dev = qdev_new(driver);
 
     /* Check whether the hotplug is allowed by the machine */
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (phase_check(MACHINE_INIT_PHASE_READY)) {
         if (!qdev_hotplug_allowed(dev, errp)) {
             goto err_del_dev;
         }
@@ -1011,7 +1012,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 55ab70eb97..d2552ba8ac 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2692,7 +2692,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;
     }
@@ -3665,14 +3665,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 eabbbc951c..b09b0f9760 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1353,7 +1353,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



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

* [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
  2021-09-22 16:13 ` [RFC PATCH v2 01/16] rename MachineInitPhase enum constants for QAPI compatibility Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-09-22 17:37   ` Philippe Mathieu-Daudé
  2021-10-12 22:08   ` Alistair Francis
  2021-09-22 16:13 ` [RFC PATCH v2 03/16] qapi: Implement x-machine-init " Damien Hedde
                   ` (16 subsequent siblings)
  18 siblings, 2 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

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>
---
 qapi/machine.json          | 56 ++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-core.h     | 30 ++------------------
 hw/core/machine-qmp-cmds.c |  9 ++++++
 hw/core/qdev.c             |  5 ++++
 4 files changed, 72 insertions(+), 28 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 157712f006..969d37fb03 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1312,3 +1312,59 @@
      '*cores': 'int',
      '*threads': 'int',
      '*maxcpus': 'int' } }
+
+##
+# @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: 6.2
+##
+{ 'enum': 'MachineInitPhase',
+  'data': [ 'no-machine', 'machine-created', 'accel-created', 'initialized',
+            'ready' ] }
+
+##
+# @MachineInitPhaseStatus:
+#
+# Information about machine initialization phase
+#
+# @phase: the machine initialization phase
+#
+# Since: 6.2
+##
+{ 'struct': 'MachineInitPhaseStatus',
+  'data': { 'phase': 'MachineInitPhase' } }
+
+##
+# @query-machine-phase:
+#
+# Return machine initialization phase
+#
+# Since: 6.2
+#
+# Returns: MachineInitPhaseStatus
+#
+# Example:
+#
+# -> { "execute": "query-machine-phase" }
+# <- { "return": { "phase": "initialized" } }
+#
+##
+{ 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
+             'allow-preconfig': true }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 859fd913bb..800eda8f54 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"
@@ -839,35 +840,8 @@ void device_listener_unregister(DeviceListener *listener);
  */
 bool qdev_should_hide_device(QemuOpts *opts);
 
-typedef enum MachineInitPhase {
-    /* current_machine is NULL.  */
-    MACHINE_INIT_PHASE_NO_MACHINE,
-
-    /* current_machine is not NULL, but current_machine->accel is NULL.  */
-    MACHINE_INIT_PHASE_MACHINE_CREATED,
-
-    /*
-     * current_machine->accel is not NULL, but the machine properties have
-     * not been validated and machine_class->init has not yet been called.
-     */
-    MACHINE_INIT_PHASE_ACCEL_CREATED,
-
-    /*
-     * machine_class->init has been called, thus creating any embedded
-     * devices and validating machine properties.  Devices created at
-     * this time are considered to be cold-plugged.
-     */
-    MACHINE_INIT_PHASE_INITIALIZED,
-
-    /*
-     * QEMU is ready to start CPUs and devices created at this time
-     * are considered to be hot-plugged.  The monitor is not restricted
-     * to "preconfig" commands.
-     */
-    MACHINE_INIT_PHASE_READY,
-} MachineInitPhase;
-
 extern bool phase_check(MachineInitPhase phase);
 extern void phase_advance(MachineInitPhase phase);
+extern MachineInitPhase phase_get(void);
 
 #endif
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 52168a3771..d3b9a04855 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -204,3 +204,12 @@ MemdevList *qmp_query_memdev(Error **errp)
     object_child_foreach(obj, query_memdev, &list);
     return list;
 }
+
+MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
+{
+    MachineInitPhaseStatus *status = g_malloc0(sizeof(*status));
+
+    status->phase = phase_get();
+
+    return status;
+}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c5fc704f55..d83f1c029a 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1150,6 +1150,11 @@ void phase_advance(MachineInitPhase phase)
     machine_phase = phase;
 }
 
+MachineInitPhase phase_get(void)
+{
+    return machine_phase;
+}
+
 static const TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
-- 
2.33.0



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

* [RFC PATCH v2 03/16] qapi: Implement x-machine-init QMP command
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
  2021-09-22 16:13 ` [RFC PATCH v2 01/16] rename MachineInitPhase enum constants for QAPI compatibility Damien Hedde
  2021-09-22 16:13 ` [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-10-12 22:19   ` Alistair Francis
  2021-09-22 16:13 ` [RFC PATCH v2 04/16] softmmu/qdev-monitor: add error handling in qdev_set_id Damien Hedde
                   ` (15 subsequent siblings)
  18 siblings, 1 reply; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

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 future commits, we will add the possiblity
to create devices at this point.

To exit the initialized phase use the x-exit-preconfig QMP command.

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---
 qapi/machine.json | 23 +++++++++++++++++++++++
 softmmu/vl.c      | 19 +++++++++++++++----
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/qapi/machine.json b/qapi/machine.json
index 969d37fb03..56330c0e8e 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1368,3 +1368,26 @@
 ##
 { 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
              'allow-preconfig': true }
+
+##
+# @x-machine-init:
+#
+# Enter machine initialized phase
+#
+# Since: 6.2
+#
+# Returns: If successful, 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. To exit the
+#        machine initialized phase use the x-exit-preconfig command.
+#
+# Example:
+#
+# -> { "execute": "x-machine-init" }
+# <- { "return": {} }
+#
+##
+{ 'command': 'x-machine-init', 'allow-preconfig': true }
diff --git a/softmmu/vl.c b/softmmu/vl.c
index d2552ba8ac..84c5132ad7 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"
@@ -2610,10 +2611,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_check(MACHINE_INIT_PHASE_INITIALIZED)) {
+        error_setg(errp, "The command is permitted only before "
+                         "the machine is initialized");
+        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);
@@ -2692,12 +2699,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



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

* [RFC PATCH v2 04/16] softmmu/qdev-monitor: add error handling in qdev_set_id
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (2 preceding siblings ...)
  2021-09-22 16:13 ` [RFC PATCH v2 03/16] qapi: Implement x-machine-init " Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-10-13  7:10   ` Alistair Francis
  2021-09-22 16:13 ` [RFC PATCH v2 05/16] qdev-monitor: prevent conflicts between qmp/device_add and cli/-device Damien Hedde
                   ` (14 subsequent siblings)
  18 siblings, 1 reply; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

qdev_set_id() is mostly used when the user adds a device (using
-device cli option or device_add qmp command). This commit adds
an error parameter to handle the case where the given id is
already taken.

Also document the function and add a return value in order to
be able to capture success/failure: the function now returns the
id in case of success, or NULL in case of failure.

The commit modifies the 2 calling places (qdev-monitor and
xen-legacy-backend) to add the error object parameter.

Note that the id is, right now, guaranteed to be unique because
all ids came from the "device" QemuOptsList where the id is used
as key. This addition is a preparation for a future commit which
will relax the uniqueness.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 include/monitor/qdev.h      | 25 +++++++++++++++++++++++-
 hw/xen/xen-legacy-backend.c |  3 ++-
 softmmu/qdev-monitor.c      | 38 +++++++++++++++++++++++++++----------
 3 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index eaa947d73a..23c31f5296 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,6 +9,29 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp);
 
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
-void qdev_set_id(DeviceState *dev, const char *id);
+
+/**
+ * qdev_set_id: parent the device and set its id if provided.
+ * @dev: device to handle
+ * @id: id to be given to the device, or NULL.
+ *
+ * Returns: the id of the device in case of success; otherwise NULL.
+ *
+ * @dev must be unrealized, unparented and must not have an id.
+ *
+ * If @id is non-NULL, this function tries to setup @dev qom path as
+ * "/peripheral/id". If @id is already taken, it fails. If it succeeds,
+ * the id field of @dev is set to @id (@dev now owns the given @id
+ * parameter).
+ *
+ * If @id is NULL, this function generates a unique name and setups @dev
+ * qom path as "/peripheral-anon/name". This name is not set as the id
+ * of @dev.
+ *
+ * Upon success, it returns the id/name (generated or provided). The
+ * returned string is owned by the corresponding child property and must
+ * not be freed by the caller.
+ */
+const char *qdev_set_id(DeviceState *dev, const char *id, Error **errp);
 
 #endif
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index dd8ae1452d..f541cfa0e9 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char *type, int dom,
     xendev = g_malloc0(ops->size);
     object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
     OBJECT(xendev)->free = g_free;
-    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
+    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
+                &error_fatal);
     qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal);
     object_unref(OBJECT(xendev));
 
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 25275984bd..0007698ff3 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -578,22 +578,34 @@ static BusState *qbus_find(const char *path, Error **errp)
     return bus;
 }
 
-void qdev_set_id(DeviceState *dev, const char *id)
+const char *qdev_set_id(DeviceState *dev, const char *id, Error **errp)
 {
+    ObjectProperty *prop;
+
+    assert(!dev->id && !dev->realized);
+
+    /*
+     * object_property_[try_]add_child() below will assert the device
+     * has no parent
+     */
     if (id) {
-        dev->id = id;
-    }
-
-    if (dev->id) {
-        object_property_add_child(qdev_get_peripheral(), dev->id,
-                                  OBJECT(dev));
+        prop = object_property_try_add_child(qdev_get_peripheral(), id,
+                                             OBJECT(dev), NULL);
+        if (prop) {
+            dev->id = id;
+        } else {
+            error_setg(errp, "Duplicate device ID '%s'", id);
+            return NULL;
+        }
     } else {
         static int anon_count;
         gchar *name = g_strdup_printf("device[%d]", anon_count++);
-        object_property_add_child(qdev_get_peripheral_anon(), name,
-                                  OBJECT(dev));
+        prop = object_property_add_child(qdev_get_peripheral_anon(), name,
+                                         OBJECT(dev));
         g_free(name);
     }
+
+    return prop->name;
 }
 
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
@@ -677,7 +689,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
         }
     }
 
-    qdev_set_id(dev, qemu_opts_id(opts));
+    /*
+     * set dev's parent and register its id.
+     * If it fails it means the id is already taken.
+     */
+    if (!qdev_set_id(dev, qemu_opts_id(opts), errp)) {
+        goto err_del_dev;
+    }
 
     /* set properties */
     if (qemu_opt_foreach(opts, set_property, dev, errp)) {
-- 
2.33.0



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

* [RFC PATCH v2 05/16] qdev-monitor: prevent conflicts between qmp/device_add and cli/-device
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (3 preceding siblings ...)
  2021-09-22 16:13 ` [RFC PATCH v2 04/16] softmmu/qdev-monitor: add error handling in qdev_set_id Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-09-22 16:13 ` [RFC PATCH v2 06/16] qapi: Allow device_add to execute in machine initialized phase Damien Hedde
                   ` (13 subsequent siblings)
  18 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

This commit prepares to extend device_add qmp command when using
-preconfig option.

In order to avoid conflicts with the cli -device option handling, we
need to handle some special case with the QemuOpts.
The qemu_device_opts is traversed when switching from
MACHINE_INIT_PHASE_INITIALIZED to MACHINE_INIT_PHASE_READY in order
to create any device specified by cli -device. Until now any
device_add qmp command was issued after that point so there was no
problem.

If we execute the qmp command before the MACHINE_INIT_PHASE_READY
phase we need to discard the QemuOpts from the qemu_device_opts in
order to avoid the cli -device code to try to create the device
again.

This commit preserves the opts behavior regarding the devices added
in 'ready' phase by the QMP command device_add.

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

Although we keep the original behavior for QMP commands issued when
the machine is ready (only authorized case so far), we are not sure
it is necessary: keeping the opts in the list is not needed anymore
to ensure the id uniqueness of devices but it has the 2 following
consequences:

1. the device opts stay in the QemuOptsList. Is this list needed
   after traversing the device cli options?

2. the DeviceState "opts" field is set. Do we need to keep it after
   the device is realized ?

Any information on this will be appreciated.
---
 softmmu/qdev-monitor.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 0007698ff3..834f2b56b5 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -848,6 +848,23 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
     if (!dev) {
         qemu_opts_del(opts);
         return;
+    } else if (!phase_check(MACHINE_INIT_PHASE_READY)) {
+        /*
+         * Always delete the related opts in case the device was created
+         * before handling of cli -device arguments:
+         * We do not want a device added by the qmp command to be handled
+         * again by the cli -device creation code. This does not break
+         * the ID uniqueness because it is checked in qdev_device_add().
+         *
+         * Note: We check the phase in order to keep the legacy behavior:
+         * in the machine ready phase case, the QemuOpts remains in the list
+         * (and the dev->opts field is kept).
+         * If it happens it was done only to ensure the ID uniqueness and
+         * the QemuOpts is never used after this point: then we could
+         * remove QemuOpts in any phase.
+         */
+        dev->opts = NULL;
+        qemu_opts_del(opts);
     }
     object_unref(OBJECT(dev));
 }
-- 
2.33.0



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

* [RFC PATCH v2 06/16] qapi: Allow device_add to execute in machine initialized phase
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (4 preceding siblings ...)
  2021-09-22 16:13 ` [RFC PATCH v2 05/16] qdev-monitor: prevent conflicts between qmp/device_add and cli/-device Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-10-12 22:25   ` Alistair Francis
  2021-09-22 16:13 ` [RFC PATCH v2 07/16] hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed Damien Hedde
                   ` (12 subsequent siblings)
  18 siblings, 1 reply; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

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

To configure a machine using QMP we need the device_add command to
execute at machine initialized phase.

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

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
---

The commit is fine, but we may add intermediate commits before this one
in order to add or change the condition for a device type to be accepted
in the 'initialized' state (see the cover-letter of the series).
---
 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 b83178220b..ad669ae175 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -67,7 +67,8 @@
 ##
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
-  'gen': false } # so we can get the additional arguments
+  'gen': false, # so we can get the additional arguments
+  'allow-preconfig': true }
 
 ##
 # @device_del:
diff --git a/monitor/misc.c b/monitor/misc.c
index ffe7966870..2c476de316 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -231,7 +231,7 @@ static void monitor_init_qmp_commands(void)
     qmp_init_marshal(&qmp_commands);
 
     qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
-                         QCO_NO_OPTIONS);
+                         QCO_ALLOW_PRECONFIG);
 
     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 834f2b56b5..47ccd90be8 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -824,6 +824,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



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

* [RFC PATCH v2 07/16] hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (5 preceding siblings ...)
  2021-09-22 16:13 ` [RFC PATCH v2 06/16] qapi: Allow device_add to execute in machine initialized phase Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-09-23 10:51   ` Ani Sinha
  2021-09-22 16:13 ` [RFC PATCH v2 08/16] qdev-monitor: Check sysbus device type before creating it Damien Hedde
                   ` (11 subsequent siblings)
  18 siblings, 1 reply; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

Right now the allowance check for adding a sysbus device using
-device cli option (or device_add qmp command) is done well after
the device has been created. It is done during the machine init done
notifier: machine_init_notify() in hw/core/machine.c

This new function will allow us to check if a sysbus device type is
allowed to be dynamically created by the machine during the device
creation time.

Also make device_is_dynamic_sysbus() use the new function.

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

In the context of our series, we need to be able to do the check at
device creation time to allow doing it depending on the current
MACHINE_INIT phase.
---
 include/hw/boards.h | 17 +++++++++++++++++
 hw/core/machine.c   | 15 ++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 463a5514f9..934443c1cd 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -51,6 +51,23 @@ void machine_set_cpu_numa_node(MachineState *machine,
  */
 void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
 
+/**
+ * machine_class_is_dynamic_sysbus_dev_allowed: Check if type is an allowed
+ * sysbus device type for the machine class.
+ * @mc: Machine class
+ * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE)
+ *
+ * Returns: true if @type is a type in the machine's list of
+ * dynamically pluggable sysbus devices; otherwise false.
+ *
+ * Check if the QOM type @type is in the list of allowed sysbus device
+ * types (see machine_class_allowed_dynamic_sysbus_dev()).
+ * Note that if @type is a subtype of a type which is in the list, it is
+ * allowed too.
+ */
+bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
+                                                 const char *type);
+
 /**
  * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
  * @mc: Machine class
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 9125c9aad0..1a18912dc8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -545,18 +545,27 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
 
 bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
 {
-    bool allowed = false;
-    strList *wl;
     Object *obj = OBJECT(dev);
 
     if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
         return false;
     }
 
+    return machine_class_is_dynamic_sysbus_dev_allowed(mc,
+            object_get_typename(obj));
+}
+
+bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
+                                                 const char *type)
+{
+    bool allowed = false;
+    strList *wl;
+    ObjectClass *klass = object_class_by_name(type);
+
     for (wl = mc->allowed_dynamic_sysbus_devices;
          !allowed && wl;
          wl = wl->next) {
-        allowed |= !!object_dynamic_cast(obj, wl->value);
+        allowed |= !!object_class_dynamic_cast(klass, wl->value);
     }
 
     return allowed;
-- 
2.33.0



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

* [RFC PATCH v2 08/16] qdev-monitor: Check sysbus device type before creating it
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (6 preceding siblings ...)
  2021-09-22 16:13 ` [RFC PATCH v2 07/16] hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-10-12 22:42   ` Alistair Francis
  2021-09-22 16:13 ` [RFC PATCH v2 09/16] hw/core/machine: Remove the dynamic sysbus devices type check Damien Hedde
                   ` (10 subsequent siblings)
  18 siblings, 1 reply; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

Add an early check to test if the requested sysbus device type
is allowed by the current machine before creating the device. This
impacts both -device cli option and device_add qmp command.

Before this patch, the check was done well after the device has
been created (in a machine init done notifier). We can now report
the error right away.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 softmmu/qdev-monitor.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 47ccd90be8..f1c9242855 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -40,6 +40,7 @@
 #include "qemu/cutils.h"
 #include "hw/qdev-properties.h"
 #include "hw/clock.h"
+#include "hw/boards.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -268,6 +269,16 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
         return NULL;
     }
 
+    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
+        /* sysbus devices need to be allowed by the machine */
+        MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
+        if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) {
+            error_setg(errp, "'%s' is not an allowed pluggable sysbus device "
+                             " type for the machine", *driver);
+            return NULL;
+        }
+    }
+
     return dc;
 }
 
-- 
2.33.0



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

* [RFC PATCH v2 09/16] hw/core/machine: Remove the dynamic sysbus devices type check
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (7 preceding siblings ...)
  2021-09-22 16:13 ` [RFC PATCH v2 08/16] qdev-monitor: Check sysbus device type before creating it Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-10-12 23:07   ` Alistair Francis
  2021-09-22 16:13 ` [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready Damien Hedde
                   ` (9 subsequent siblings)
  18 siblings, 1 reply; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

Now that we check sysbus device types during device creation, we
can remove the check done in the machine init done notifier.
This was the only thing done by this notifier, so we remove the
whole sysbus_notifier structure of the MachineState.

Note: This notifier was checking all /peripheral and /peripheral-anon
sysbus devices. Now we only check those added by -device cli option or
device_add qmp command when handling the command/option. So if there
are some devices added in one of these containers manually (eg in
machine C code), these will not be checked anymore.
This use case does not seem to appear apart from
hw/xen/xen-legacy-backend.c (it uses qdev_set_id() and in this case,
not for a sysbus device, so it's ok).

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 include/hw/boards.h |  1 -
 hw/core/machine.c   | 27 ---------------------------
 2 files changed, 28 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 934443c1cd..ccbc40355a 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -311,7 +311,6 @@ typedef struct CpuTopology {
 struct MachineState {
     /*< private >*/
     Object parent_obj;
-    Notifier sysbus_notifier;
 
     /*< public >*/
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1a18912dc8..521438e90a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -571,18 +571,6 @@ bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
     return allowed;
 }
 
-static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
-{
-    MachineState *machine = opaque;
-    MachineClass *mc = MACHINE_GET_CLASS(machine);
-
-    if (!device_is_dynamic_sysbus(mc, DEVICE(sbdev))) {
-        error_report("Option '-device %s' cannot be handled by this machine",
-                     object_class_get_name(object_get_class(OBJECT(sbdev))));
-        exit(1);
-    }
-}
-
 static char *machine_get_memdev(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -598,17 +586,6 @@ static void machine_set_memdev(Object *obj, const char *value, Error **errp)
     ms->ram_memdev_id = g_strdup(value);
 }
 
-static void machine_init_notify(Notifier *notifier, void *data)
-{
-    MachineState *machine = MACHINE(qdev_get_machine());
-
-    /*
-     * Loop through all dynamically created sysbus devices and check if they are
-     * all allowed.  If a device is not allowed, error out.
-     */
-    foreach_dynamic_sysbus_device(validate_sysbus_device, machine);
-}
-
 HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
 {
     int i;
@@ -1030,10 +1007,6 @@ static void machine_initfn(Object *obj)
                                         "Table (HMAT)");
     }
 
-    /* Register notifier when init is done for sysbus sanity checks */
-    ms->sysbus_notifier.notify = machine_init_notify;
-    qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
-
     /* default to mc->default_cpus */
     ms->smp.cpus = mc->default_cpus;
     ms->smp.max_cpus = mc->default_cpus;
-- 
2.33.0



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

* [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (8 preceding siblings ...)
  2021-09-22 16:13 ` [RFC PATCH v2 09/16] hw/core/machine: Remove the dynamic sysbus devices type check Damien Hedde
@ 2021-09-22 16:13 ` Damien Hedde
  2021-09-23 11:04   ` Ani Sinha
  2021-09-22 16:14 ` [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function Damien Hedde
                   ` (8 subsequent siblings)
  18 siblings, 1 reply; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:13 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

Skip the sysbus device type per-machine allow-list check before the
MACHINE_INIT_PHASE_READY phase.

This patch permits adding any sysbus device (it still needs to be
user_creatable) when using the -preconfig experimental option.

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

This commit is RFC. Depending on the condition to allow a device
to be added, it may change.
---
 softmmu/qdev-monitor.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index f1c9242855..73b991adda 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
         return NULL;
     }
 
-    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
-        /* sysbus devices need to be allowed by the machine */
+    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) &&
+        phase_check(MACHINE_INIT_PHASE_READY)) {
+        /*
+         * Sysbus devices need to be allowed by the machine.
+         * We only check that after the machine is ready in order to let
+         * us add any user_creatable sysbus device during machine creation.
+         */
         MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
         if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) {
             error_setg(errp, "'%s' is not an allowed pluggable sysbus device "
-- 
2.33.0



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

* [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (9 preceding siblings ...)
  2021-09-22 16:13 ` [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready Damien Hedde
@ 2021-09-22 16:14 ` Damien Hedde
  2021-09-22 17:42   ` Philippe Mathieu-Daudé
                     ` (2 more replies)
  2021-09-22 16:14 ` [RFC PATCH v2 12/16] add x-sysbus-mmio-map qmp command Damien Hedde
                   ` (7 subsequent siblings)
  18 siblings, 3 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

It allows to try to add a subregion to a memory region with error
handling. Like memory_region_add_subregion_overlap, it handles
priority as well.
Apart the error handling, the behavior is the same. It can be used
to do the simple memory_region_add_subregion() (with no overlap) by
setting the priority parameter to 0.

This commit is a preparation to further use this function in the
context of qmp command which needs error handling support.

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

Adding a new function is obviously not ideal. But there is ~900
occurrences of memory_region_add_subregion[_overlap] calls in the code
base. We do not really see an alternative here.
---
 include/exec/memory.h | 22 ++++++++++++++++++++++
 softmmu/memory.c      | 22 ++++++++++++++--------
 2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index c3d417d317..422e1eda67 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2162,6 +2162,28 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
                                          MemoryRegion *subregion,
                                          int priority);
 
+/**
+ * memory_region_try_add_subregion: Add a subregion to a container
+ *                                  with error handling.
+ *
+ * Behaves like memory_region_add_subregion_overlap(), but errors are
+ * reported if the subregion cannot be added.
+ *
+ * @mr: the region to contain the new subregion; must be a container
+ *      initialized with memory_region_init().
+ * @offset: the offset relative to @mr where @subregion is added.
+ * @subregion: the subregion to be added.
+ * @priority: used for resolving overlaps; highest priority wins.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Returns: True in case of success, false otherwise.
+ */
+bool memory_region_try_add_subregion(MemoryRegion *mr,
+                                     hwaddr offset,
+                                     MemoryRegion *subregion,
+                                     int priority,
+                                     Error **errp);
+
 /**
  * memory_region_get_ram_addr: Get the ram address associated with a memory
  *                             region
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4d..eac61f8236 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2513,22 +2513,28 @@ done:
     memory_region_transaction_commit();
 }
 
-static void memory_region_add_subregion_common(MemoryRegion *mr,
-                                               hwaddr offset,
-                                               MemoryRegion *subregion)
+bool memory_region_try_add_subregion(MemoryRegion *mr,
+                                     hwaddr offset,
+                                     MemoryRegion *subregion,
+                                     int priority,
+                                     Error **errp)
 {
-    assert(!subregion->container);
+    if (subregion->container) {
+        error_setg(errp, "The memory region is already in another region");
+        return false;
+    }
+    subregion->priority = priority;
     subregion->container = mr;
     subregion->addr = offset;
     memory_region_update_container_subregions(subregion);
+    return true;
 }
 
 void memory_region_add_subregion(MemoryRegion *mr,
                                  hwaddr offset,
                                  MemoryRegion *subregion)
 {
-    subregion->priority = 0;
-    memory_region_add_subregion_common(mr, offset, subregion);
+    memory_region_try_add_subregion(mr, offset, subregion, 0, &error_abort);
 }
 
 void memory_region_add_subregion_overlap(MemoryRegion *mr,
@@ -2536,8 +2542,8 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
                                          MemoryRegion *subregion,
                                          int priority)
 {
-    subregion->priority = priority;
-    memory_region_add_subregion_common(mr, offset, subregion);
+    memory_region_try_add_subregion(mr, offset, subregion, priority,
+                                    &error_abort);
 }
 
 void memory_region_del_subregion(MemoryRegion *mr,
-- 
2.33.0



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

* [RFC PATCH v2 12/16] add x-sysbus-mmio-map qmp command
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (10 preceding siblings ...)
  2021-09-22 16:14 ` [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function Damien Hedde
@ 2021-09-22 16:14 ` Damien Hedde
  2021-10-13  7:16   ` Alistair Francis
  2021-09-22 16:14 ` [RFC PATCH v2 13/16] hw/mem/system-memory: add a memory sysbus device Damien Hedde
                   ` (6 subsequent siblings)
  18 siblings, 1 reply; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

This command allows to map an mmio region of sysbus device onto
the system memory. Its behavior mimics the sysbus_mmio_map()
function apart from the automatic unmap (the C function unmaps
the region if it is already mapped).
For the qmp function we consider it is an error to try to map
an already mapped function. If unmapping is required, it is
probably better to add a sysbus-mmip-unmap function.

This command is still experimental (hence the x prefix), as it
is related to the sysbus device creation through qmp commands.

In future, we expect to have to handle the overlap/priority
parameter but also multiple mapping of one mmio. For some
devices, one mmio is mapped several times at different addresses on
the bus (which is not supported by sysbus_mmio_map() function and
requires the use of memory region aliases).

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

Note: this qmp command is required to be able to build a machine from
scratch as there is no qmp-way of doing a memory mapping today.

We've added the command into qapi/qdev.json section. It does not seem to
have any really adequate section yet. Any idea ? should we create for
example a new one: qapi/sysbus.json or qapi/memory.json ?
---
 qapi/qdev.json   | 21 +++++++++++++++++++++
 hw/core/sysbus.c | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index ad669ae175..dfc1104197 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -125,3 +125,24 @@
 ##
 { 'event': 'DEVICE_DELETED',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @x-sysbus-mmio-map:
+#
+# Map a sysbus device mmio onto the main system bus.
+#
+# @device: the device's QOM path
+#
+# @mmio: The mmio number to be mapped (defaults to 0).
+#
+# @addr: The base address for the mapping.
+#
+# Since: 6.2
+#
+# Returns: Nothing on success
+#
+##
+
+{ 'command': 'x-sysbus-mmio-map',
+  'data': {'device': 'str', '*mmio': 'uint8', 'addr': 'uint64'},
+  'allow-preconfig' : true }
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index aaae8e23cc..b0891f37b6 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -23,6 +23,7 @@
 #include "hw/sysbus.h"
 #include "monitor/monitor.h"
 #include "exec/address-spaces.h"
+#include "qapi/qapi-commands-qdev.h"
 
 static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
 static char *sysbus_get_fw_dev_path(DeviceState *dev);
@@ -154,6 +155,46 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
     }
 }
 
+void qmp_x_sysbus_mmio_map(const char *device, bool has_mmio, uint8_t mmio,
+                           uint64_t addr, Error **errp)
+{
+    Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE, NULL);
+    SysBusDevice *dev;
+
+    if (phase_get() != MACHINE_INIT_PHASE_INITIALIZED) {
+        error_setg(errp, "The command is permitted only when "
+                         "the machine is in initialized phase");
+        return;
+    }
+
+    if (obj == NULL) {
+        error_setg(errp, "Device '%s' not found", device);
+        return;
+    }
+
+    dev = SYS_BUS_DEVICE(obj);
+    if (!has_mmio) {
+        mmio = 0;
+    }
+    if (mmio >= dev->num_mmio) {
+        error_setg(errp, "MMIO index '%u' is out of range", mmio);
+        return;
+    }
+
+    if (dev->mmio[mmio].addr != (hwaddr)-1) {
+        error_setg(errp, "MMIO index '%u' is already mapped", mmio);
+        return;
+    }
+
+    if (!memory_region_try_add_subregion(get_system_memory(), addr,
+                                         dev->mmio[mmio].memory, 0,
+                                         errp)) {
+        return;
+    }
+
+    dev->mmio[mmio].addr = addr;
+}
+
 void sysbus_mmio_unmap(SysBusDevice *dev, int n)
 {
     assert(n >= 0 && n < dev->num_mmio);
-- 
2.33.0



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

* [RFC PATCH v2 13/16] hw/mem/system-memory: add a memory sysbus device
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (11 preceding siblings ...)
  2021-09-22 16:14 ` [RFC PATCH v2 12/16] add x-sysbus-mmio-map qmp command Damien Hedde
@ 2021-09-22 16:14 ` Damien Hedde
  2021-09-22 16:14 ` [RFC PATCH v2 14/16] docs/system: add doc about the initialized machine phase and an example Damien Hedde
                   ` (5 subsequent siblings)
  18 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

This device can be used to create some memories using standard
device_add qmp command.

This device has one property 'readonly' which allows
to choose between a ram or a rom.
The device holds the adequate memory region and can be put on
the sysbus.

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

Should we add a related CONFIG_ variable in the build system ?

Depending on the chosen condition to add a device, the commit may
change.
---
 include/hw/mem/sysbus-memory.h | 32 +++++++++++++
 hw/mem/sysbus-memory.c         | 83 ++++++++++++++++++++++++++++++++++
 hw/mem/meson.build             |  2 +
 3 files changed, 117 insertions(+)
 create mode 100644 include/hw/mem/sysbus-memory.h
 create mode 100644 hw/mem/sysbus-memory.c

diff --git a/include/hw/mem/sysbus-memory.h b/include/hw/mem/sysbus-memory.h
new file mode 100644
index 0000000000..3e1271dbfd
--- /dev/null
+++ b/include/hw/mem/sysbus-memory.h
@@ -0,0 +1,32 @@
+/*
+ * QEMU memory SysBusDevice
+ *
+ * Copyright (c) 2021 Greensocs
+ *
+ * Author:
+ * + Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_SYSBUS_MEMORY_H
+#define HW_SYSBUS_MEMORY_H
+
+#include "hw/sysbus.h"
+#include "qom/object.h"
+
+#define TYPE_SYSBUS_MEMORY "sysbus-memory"
+OBJECT_DECLARE_SIMPLE_TYPE(SysBusMemoryState, SYSBUS_MEMORY)
+
+struct SysBusMemoryState {
+    /* <private> */
+    SysBusDevice parent_obj;
+    uint64_t size;
+    bool readonly;
+
+    /* <public> */
+    MemoryRegion mem;
+};
+
+#endif /* HW_SYSBUS_MEMORY_H */
diff --git a/hw/mem/sysbus-memory.c b/hw/mem/sysbus-memory.c
new file mode 100644
index 0000000000..897fa154f0
--- /dev/null
+++ b/hw/mem/sysbus-memory.c
@@ -0,0 +1,83 @@
+/*
+ * QEMU memory SysBusDevice
+ *
+ * Copyright (c) 2021 Greensocs
+ *
+ * Author:
+ * + Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/mem/sysbus-memory.h"
+#include "hw/qdev-properties.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+
+static Property sysbus_memory_properties[] = {
+    DEFINE_PROP_UINT64("size", SysBusMemoryState, size, 0),
+    DEFINE_PROP_BOOL("readonly", SysBusMemoryState, readonly, false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void sysbus_memory_realize(DeviceState *dev, Error **errp)
+{
+    SysBusMemoryState *s = SYSBUS_MEMORY(dev);
+    gchar *name;
+
+    if (!s->size) {
+        error_setg(errp, "'size' must be non-zero.");
+        return;
+    }
+
+    /*
+     * We impose having an id (which is unique) because we need to generate
+     * a unique name for the memory region.
+     * memory_region_init_ram/rom() will abort() (in qemu_ram_set_idstr()
+     * function if 2 system-memory devices are created with the same name
+     * for the memory region).
+     */
+    if (!dev->id) {
+        error_setg(errp, "system-memory device must have an id.");
+        return;
+    }
+    name = g_strdup_printf("%s.region", dev->id);
+
+    if (s->readonly) {
+        memory_region_init_rom(&s->mem, OBJECT(dev), name, s->size, errp);
+    } else {
+        memory_region_init_ram(&s->mem, OBJECT(dev), name, s->size, errp);
+    }
+
+    g_free(name);
+
+    if (!*errp) {
+        sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mem);
+    }
+}
+
+static void sysbus_memory_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->user_creatable = true;
+    dc->realize = sysbus_memory_realize;
+    device_class_set_props(dc, sysbus_memory_properties);
+}
+
+static const TypeInfo sysbus_memory_info = {
+    .name          = TYPE_SYSBUS_MEMORY,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(SysBusMemoryState),
+    .class_init    = sysbus_memory_class_init,
+};
+
+static void sysbus_memory_register_types(void)
+{
+    type_register_static(&sysbus_memory_info);
+}
+
+type_init(sysbus_memory_register_types)
diff --git a/hw/mem/meson.build b/hw/mem/meson.build
index 3c8fdef9f9..81e2de1d34 100644
--- a/hw/mem/meson.build
+++ b/hw/mem/meson.build
@@ -7,3 +7,5 @@ mem_ss.add(when: 'CONFIG_NVDIMM', if_true: files('nvdimm.c'))
 softmmu_ss.add_all(when: 'CONFIG_MEM_DEVICE', if_true: mem_ss)
 
 softmmu_ss.add(when: 'CONFIG_FUZZ', if_true: files('sparse-mem.c'))
+
+softmmu_ss.add(files('sysbus-memory.c'))
-- 
2.33.0



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

* [RFC PATCH v2 14/16] docs/system: add doc about the initialized machine phase and an example
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (12 preceding siblings ...)
  2021-09-22 16:14 ` [RFC PATCH v2 13/16] hw/mem/system-memory: add a memory sysbus device Damien Hedde
@ 2021-09-22 16:14 ` Damien Hedde
  2021-09-22 16:14 ` [RFC PATCH v2 15/16] hw/char/ibex_uart: set user_creatable Damien Hedde
                   ` (4 subsequent siblings)
  18 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 docs/system/managed-startup.rst | 77 +++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/docs/system/managed-startup.rst b/docs/system/managed-startup.rst
index 9bcf98ea79..af12a10d27 100644
--- a/docs/system/managed-startup.rst
+++ b/docs/system/managed-startup.rst
@@ -32,4 +32,81 @@ machine, including but not limited to:
 - ``query-qmp-schema``
 - ``query-commands``
 - ``query-status``
+- ``x-machine-init``
 - ``x-exit-preconfig``
+
+In particular these commands allow to advance and stop qemu at different
+phases of the VM creation and finally to leave the "preconfig" state. The
+accessible phases are:
+
+- ``accel-created``
+- ``initialized``
+- ``ready``
+
+The order of the phases is enforced. It is not possible to go backwards.
+Note that other early phases exist, but they are not attainable with
+``--preconfig``. Depending on the phase, QMP commands can be issued to modify
+some part of the VM creation.
+
+accel-created phase
+-------------------
+
+Initial phase entered with ``--preconfig``.
+
+initialized phase
+-----------------
+
+``x-machine-init`` advances to ``initialized`` phase. During this phase, the
+machine is initialized and populated with buses and devices. The following QMP
+commands are available to manually populate or modify the machine:
+
+- ``device_add``
+- ``x-sysbus-mmio-map``
+- ``qom-set``
+
+ready phase
+-----------
+
+``x-exit-preconfig`` advances to the final phase. When entering this phase,
+the VM creation finishes. "preconfig" state is then done and QEMU goes to
+normal execution.
+
+Machine creation example
+------------------------
+
+The following is an example that shows how to add some devices with qmp
+commands, memory map them, and add interrupts::
+
+  x-machine-init
+
+  device_add        driver=sysbus-memory id=rom size=0x4000 readonly=true
+  x-sysbus-mmio-map device=rom addr=32768
+
+  device_add        driver=sysbus-memory id=flash size=0x80000 readonly=true
+  x-sysbus-mmio-map device=flash addr=536870912
+
+  device_add        driver=sysbus-memory id=ram size=0x10000
+  x-sysbus-mmio-map device=ram addr=268435456
+
+  device_add        driver=ibex-plic id=plic
+  x-sysbus-mmio-map device=plic addr=1090584576
+
+  device_add        driver=ibex-uart id=uart chardev=serial0
+  x-sysbus-mmio-map device=uart addr=1073741824
+  qom-set path=uart property=sysbus-irq[0] value=plic/unnamed-gpio-in[1]
+  qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2]
+  qom-set path=uart property=sysbus-irq[2] value=plic/unnamed-gpio-in[3]
+  qom-set path=uart property=sysbus-irq[3] value=plic/unnamed-gpio-in[4]
+
+  x-exit-preconfig
+
+These commands reproduce a subset of the riscv32 opentitan (hw/riscv/opentitan)
+machine. We can start qemu using::
+
+  qemu-sytem-riscv32 -preconfig -qmp unix:./qmp-sock,server \
+  -machine none -cpu lowriscv-ibex -serial mon:stdio ...
+
+Then we just have to issue the commands, for example using `qmp-shell`. If the
+previous commands were in a file named `machine.qmp`, we could do::
+
+  qmp-shell ./qmp-sock < machine.qmp
-- 
2.33.0



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

* [RFC PATCH v2 15/16] hw/char/ibex_uart: set user_creatable
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (13 preceding siblings ...)
  2021-09-22 16:14 ` [RFC PATCH v2 14/16] docs/system: add doc about the initialized machine phase and an example Damien Hedde
@ 2021-09-22 16:14 ` Damien Hedde
  2021-09-22 16:14 ` [RFC PATCH v2 16/16] hw/intc/ibex_plic: " Damien Hedde
                   ` (3 subsequent siblings)
  18 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

This patch allows to create the device using device_add
using -preconfig mode. This sysbus device still needs to
be allowed by a machine to be created after preconfig is done.

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

Depending on chosen condition for a device to be added, this commit
may change.
---
 hw/char/ibex_uart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
index 9b0a817713..b1646422c0 100644
--- a/hw/char/ibex_uart.c
+++ b/hw/char/ibex_uart.c
@@ -546,6 +546,7 @@ static void ibex_uart_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->user_creatable = true;
     dc->reset = ibex_uart_reset;
     dc->realize = ibex_uart_realize;
     dc->vmsd = &vmstate_ibex_uart;
-- 
2.33.0



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

* [RFC PATCH v2 16/16] hw/intc/ibex_plic: set user_creatable
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (14 preceding siblings ...)
  2021-09-22 16:14 ` [RFC PATCH v2 15/16] hw/char/ibex_uart: set user_creatable Damien Hedde
@ 2021-09-22 16:14 ` Damien Hedde
  2021-09-22 17:50 ` [RFC PATCH v2 00/16] Initial support for machine creation via QMP Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  18 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-22 16:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Damien Hedde, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell

This patch allows to create the device using device_add
using -preconfig mode. This sysbus device still needs to
be allowed by a machine to be created after preconfig is done.

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

Depending on chosen condition for a device to be added, this commit
may change.
---
 hw/intc/ibex_plic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/ibex_plic.c b/hw/intc/ibex_plic.c
index edf76e4f61..8abd5ee613 100644
--- a/hw/intc/ibex_plic.c
+++ b/hw/intc/ibex_plic.c
@@ -291,6 +291,7 @@ static void ibex_plic_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->user_creatable = true;
     dc->reset = ibex_plic_reset;
     device_class_set_props(dc, ibex_plic_properties);
     dc->realize = ibex_plic_realize;
-- 
2.33.0



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

* Re: [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command
  2021-09-22 16:13 ` [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command Damien Hedde
@ 2021-09-22 17:37   ` Philippe Mathieu-Daudé
  2021-09-23 12:43     ` Damien Hedde
  2021-10-12 22:08   ` Alistair Francis
  1 sibling, 1 reply; 41+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-22 17:37 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: Peter Maydell, Michael S. Tsirkin, David Hildenbrand, Peter Xu,
	mirela.grujic, Alistair Francis, Gerd Hoffmann, Ani Sinha,
	Eric Blake, Stefano Stabellini, xen-devel, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, qemu-riscv, Daniel P. Berrangé,
	mark.burton, edgari, Igor Mammedov

On 9/22/21 18:13, Damien Hedde 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>

Missing your S-o-b, otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   qapi/machine.json          | 56 ++++++++++++++++++++++++++++++++++++++
>   include/hw/qdev-core.h     | 30 ++------------------
>   hw/core/machine-qmp-cmds.c |  9 ++++++
>   hw/core/qdev.c             |  5 ++++
>   4 files changed, 72 insertions(+), 28 deletions(-)



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

* Re: [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function
  2021-09-22 16:14 ` [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function Damien Hedde
@ 2021-09-22 17:42   ` Philippe Mathieu-Daudé
  2021-09-22 17:49   ` David Hildenbrand
  2021-10-13  7:12   ` Alistair Francis
  2 siblings, 0 replies; 41+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-22 17:42 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: Peter Maydell, Michael S. Tsirkin, David Hildenbrand, Peter Xu,
	mirela.grujic, Alistair Francis, Gerd Hoffmann, Ani Sinha,
	Eric Blake, Stefano Stabellini, xen-devel, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, qemu-riscv, Daniel P. Berrangé,
	mark.burton, edgari, Igor Mammedov

On 9/22/21 18:14, Damien Hedde wrote:
> It allows to try to add a subregion to a memory region with error
> handling. Like memory_region_add_subregion_overlap, it handles
> priority as well.
> Apart the error handling, the behavior is the same. It can be used
> to do the simple memory_region_add_subregion() (with no overlap) by
> setting the priority parameter to 0.
> 
> This commit is a preparation to further use this function in the
> context of qmp command which needs error handling support.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> Adding a new function is obviously not ideal. But there is ~900
> occurrences of memory_region_add_subregion[_overlap] calls in the code
> base. We do not really see an alternative here.
> ---
>   include/exec/memory.h | 22 ++++++++++++++++++++++
>   softmmu/memory.c      | 22 ++++++++++++++--------
>   2 files changed, 36 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



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

* Re: [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function
  2021-09-22 16:14 ` [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function Damien Hedde
  2021-09-22 17:42   ` Philippe Mathieu-Daudé
@ 2021-09-22 17:49   ` David Hildenbrand
  2021-10-13  7:12   ` Alistair Francis
  2 siblings, 0 replies; 41+ messages in thread
From: David Hildenbrand @ 2021-09-22 17:49 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: Alistair Francis, Marc-André Lureau, Paolo Bonzini,
	Eduardo Habkost, Marcel Apfelbaum, Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, Dr. David Alan Gilbert, Markus Armbruster, Eric Blake,
	qemu-riscv, xen-devel, mark.burton, mirela.grujic, edgari,
	Peter Maydell

On 22.09.21 18:14, Damien Hedde wrote:
> It allows to try to add a subregion to a memory region with error
> handling. Like memory_region_add_subregion_overlap, it handles
> priority as well.
> Apart the error handling, the behavior is the same. It can be used
> to do the simple memory_region_add_subregion() (with no overlap) by
> setting the priority parameter to 0.
> 
> This commit is a preparation to further use this function in the
> context of qmp command which needs error handling support.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> Adding a new function is obviously not ideal. But there is ~900
> occurrences of memory_region_add_subregion[_overlap] calls in the code
> base. We do not really see an alternative here.
> ---
>   include/exec/memory.h | 22 ++++++++++++++++++++++
>   softmmu/memory.c      | 22 ++++++++++++++--------
>   2 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c3d417d317..422e1eda67 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2162,6 +2162,28 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                            MemoryRegion *subregion,
>                                            int priority);
>   
> +/**
> + * memory_region_try_add_subregion: Add a subregion to a container
> + *                                  with error handling.
> + *
> + * Behaves like memory_region_add_subregion_overlap(), but errors are
> + * reported if the subregion cannot be added.
> + *
> + * @mr: the region to contain the new subregion; must be a container
> + *      initialized with memory_region_init().
> + * @offset: the offset relative to @mr where @subregion is added.
> + * @subregion: the subregion to be added.
> + * @priority: used for resolving overlaps; highest priority wins.
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Returns: True in case of success, false otherwise.
> + */
> +bool memory_region_try_add_subregion(MemoryRegion *mr,
> +                                     hwaddr offset,
> +                                     MemoryRegion *subregion,
> +                                     int priority,
> +                                     Error **errp);
> +
>   /**
>    * memory_region_get_ram_addr: Get the ram address associated with a memory
>    *                             region
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4d..eac61f8236 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2513,22 +2513,28 @@ done:
>       memory_region_transaction_commit();
>   }
>   
> -static void memory_region_add_subregion_common(MemoryRegion *mr,
> -                                               hwaddr offset,
> -                                               MemoryRegion *subregion)
> +bool memory_region_try_add_subregion(MemoryRegion *mr,
> +                                     hwaddr offset,
> +                                     MemoryRegion *subregion,
> +                                     int priority,
> +                                     Error **errp)
>   {
> -    assert(!subregion->container);
> +    if (subregion->container) {
> +        error_setg(errp, "The memory region is already in another region");
> +        return false;
> +    }
> +    subregion->priority = priority;
>       subregion->container = mr;
>       subregion->addr = offset;
>       memory_region_update_container_subregions(subregion);
> +    return true;
>   }
>   
>   void memory_region_add_subregion(MemoryRegion *mr,
>                                    hwaddr offset,
>                                    MemoryRegion *subregion)
>   {
> -    subregion->priority = 0;
> -    memory_region_add_subregion_common(mr, offset, subregion);
> +    memory_region_try_add_subregion(mr, offset, subregion, 0, &error_abort);
>   }
>   
>   void memory_region_add_subregion_overlap(MemoryRegion *mr,
> @@ -2536,8 +2542,8 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                            MemoryRegion *subregion,
>                                            int priority)
>   {
> -    subregion->priority = priority;
> -    memory_region_add_subregion_common(mr, offset, subregion);
> +    memory_region_try_add_subregion(mr, offset, subregion, priority,
> +                                    &error_abort);
>   }
>   
>   void memory_region_del_subregion(MemoryRegion *mr,
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb



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

* Re: [RFC PATCH v2 00/16] Initial support for machine creation via QMP
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (15 preceding siblings ...)
  2021-09-22 16:14 ` [RFC PATCH v2 16/16] hw/intc/ibex_plic: " Damien Hedde
@ 2021-09-22 17:50 ` Philippe Mathieu-Daudé
  2021-09-23 11:08   ` Damien Hedde
  2021-10-04 15:56 ` Damien Hedde
  2021-10-12 22:16 ` Alistair Francis
  18 siblings, 1 reply; 41+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-09-22 17:50 UTC (permalink / raw)
  To: Damien Hedde, qemu-devel
  Cc: Peter Maydell, Michael S. Tsirkin, David Hildenbrand, Peter Xu,
	mirela.grujic, Alistair Francis, Gerd Hoffmann, Ani Sinha,
	Eric Blake, Stefano Stabellini, xen-devel, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, qemu-riscv, Daniel P. Berrangé,
	mark.burton, edgari, Igor Mammedov

Hi Damien,

On 9/22/21 18:13, Damien Hedde wrote:
> 
> The goal of this work is to bring dynamic machine creation to QEMU:
> we want to setup a machine without compiling a specific machine C
> code. It would ease supporting highly configurable platforms (for
> example resulting from an automated design flow). The requirements
> for such configuration include begin able to specify the number of
> cores, available peripherals, emmory mapping, IRQ mapping, etc.
> 
> This series focuses on the first step: populating a machine with
> devices during its creation. We propose patches to support this
> using QMP commands. This is a working set of patches and improves
> over the earlier rfc (posted in May):
> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html

Do you have a roadmap for the following steps? Or are you done with
this series?

Yesterday I was thinking about this, and one thing I was wondering is
if it would be possible to have DeviceClass and MachineClass implement
a populate_fdt() handler, to automatically generate custom DTB for these
custom machines.

Maybe in your case you don't need that, as your framework generating the
QEMU machine also generates the DTB, or even parse a DTB to generate the
machine... :)

Regards,

Phil.



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

* Re: [RFC PATCH v2 07/16] hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed
  2021-09-22 16:13 ` [RFC PATCH v2 07/16] hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed Damien Hedde
@ 2021-09-23 10:51   ` Ani Sinha
  2021-09-23 13:07     ` Damien Hedde
  0 siblings, 1 reply; 41+ messages in thread
From: Ani Sinha @ 2021-09-23 10:51 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell



On Wed, 22 Sep 2021, Damien Hedde wrote:

> Right now the allowance check for adding a sysbus device using
> -device cli option (or device_add qmp command) is done well after
> the device has been created. It is done during the machine init done
> notifier: machine_init_notify() in hw/core/machine.c
>
> This new function will allow us to check if a sysbus device type is
> allowed to be dynamically created by the machine during the device
> creation time.
>
> Also make device_is_dynamic_sysbus() use the new function.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> In the context of our series, we need to be able to do the check at
> device creation time to allow doing it depending on the current
> MACHINE_INIT phase.
> ---
>  include/hw/boards.h | 17 +++++++++++++++++
>  hw/core/machine.c   | 15 ++++++++++++---
>  2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 463a5514f9..934443c1cd 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -51,6 +51,23 @@ void machine_set_cpu_numa_node(MachineState *machine,
>   */
>  void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>
> +/**
> + * machine_class_is_dynamic_sysbus_dev_allowed: Check if type is an allowed
> + * sysbus device type for the machine class.
> + * @mc: Machine class
> + * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE)
> + *
> + * Returns: true if @type is a type in the machine's list of
> + * dynamically pluggable sysbus devices; otherwise false.
> + *
> + * Check if the QOM type @type is in the list of allowed sysbus device
> + * types (see machine_class_allowed_dynamic_sysbus_dev()).
> + * Note that if @type is a subtype of a type which is in the list, it is
> + * allowed too.
> + */
> +bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
> +                                                 const char *type);
> +

How about renaming this to device_type_is_allowed_dynamic_sysbus() ?

>  /**
>   * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
>   * @mc: Machine class
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 9125c9aad0..1a18912dc8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -545,18 +545,27 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>
>  bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
>  {
> -    bool allowed = false;
> -    strList *wl;
>      Object *obj = OBJECT(dev);
>
>      if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
>          return false;
>      }
>
> +    return machine_class_is_dynamic_sysbus_dev_allowed(mc,
> +            object_get_typename(obj));
> +}
> +
> +bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
> +                                                 const char *type)
> +{
> +    bool allowed = false;
> +    strList *wl;
> +    ObjectClass *klass = object_class_by_name(type);
> +
>      for (wl = mc->allowed_dynamic_sysbus_devices;
>           !allowed && wl;
>           wl = wl->next) {
> -        allowed |= !!object_dynamic_cast(obj, wl->value);
> +        allowed |= !!object_class_dynamic_cast(klass, wl->value);
>      }
>
>      return allowed;
> --
> 2.33.0
>
>


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

* Re: [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready
  2021-09-22 16:13 ` [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready Damien Hedde
@ 2021-09-23 11:04   ` Ani Sinha
  2021-09-23 11:55     ` Ani Sinha
  0 siblings, 1 reply; 41+ messages in thread
From: Ani Sinha @ 2021-09-23 11:04 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell



On Wed, 22 Sep 2021, Damien Hedde wrote:

> Skip the sysbus device type per-machine allow-list check before the
> MACHINE_INIT_PHASE_READY phase.
>
> This patch permits adding any sysbus device (it still needs to be
> user_creatable) when using the -preconfig experimental option.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> This commit is RFC. Depending on the condition to allow a device
> to be added, it may change.
> ---
>  softmmu/qdev-monitor.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index f1c9242855..73b991adda 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>          return NULL;
>      }
>
> -    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
> -        /* sysbus devices need to be allowed by the machine */
> +    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) &&
> +        phase_check(MACHINE_INIT_PHASE_READY)) {
> +        /*
> +         * Sysbus devices need to be allowed by the machine.
> +         * We only check that after the machine is ready in order to let
> +         * us add any user_creatable sysbus device during machine creation.
> +         */
>          MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
>          if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) {
>              error_setg(errp, "'%s' is not an allowed pluggable sysbus device "

Since now you are adding the state of the machine creation in the
valiation condition, the failure error message becomes misleading.
Better to do this I think :

if (object class is TYPE_SYS_BUS_DEVICE)
{
  if (!phase_check(MACHINE_INIT_PHASE_READY))
    {
      // error out here saying the state of the machine creation is too
early
    }

    // do the other check on dynamic sysbus

}



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

* Re: [RFC PATCH v2 00/16] Initial support for machine creation via QMP
  2021-09-22 17:50 ` [RFC PATCH v2 00/16] Initial support for machine creation via QMP Philippe Mathieu-Daudé
@ 2021-09-23 11:08   ` Damien Hedde
  0 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-23 11:08 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Peter Maydell, Michael S. Tsirkin, David Hildenbrand, Peter Xu,
	mirela.grujic, Alistair Francis, Gerd Hoffmann, Ani Sinha,
	Eric Blake, Stefano Stabellini, xen-devel, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, qemu-riscv, Daniel P. Berrangé,
	mark.burton, edgari, Igor Mammedov



On 9/22/21 19:50, Philippe Mathieu-Daudé wrote:
> Hi Damien,
> 
> On 9/22/21 18:13, Damien Hedde wrote:
>>
>> The goal of this work is to bring dynamic machine creation to QEMU:
>> we want to setup a machine without compiling a specific machine C
>> code. It would ease supporting highly configurable platforms (for
>> example resulting from an automated design flow). The requirements
>> for such configuration include begin able to specify the number of
>> cores, available peripherals, emmory mapping, IRQ mapping, etc.
>>
>> This series focuses on the first step: populating a machine with
>> devices during its creation. We propose patches to support this
>> using QMP commands. This is a working set of patches and improves
>> over the earlier rfc (posted in May):
>> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html
> 
> Do you have a roadmap for the following steps? Or are you done with
> this series?

Hi Philippe,

We would like to stick to this scope ("creating devices in a machine") 
for this particular series otherwise it will become very big and cover a 
huge scope.

We have some patches to "migrate" more devices to be early 
user-creatable (like the last 2 patches of this series) so that we can 
use more device when building a machine. But these are trivial and only 
depends on what is the condition to allow this. We plan to submit these 
when this series is done.

We plan to address other issues we have in others series of patches. We 
do not put a roadmap somewhere. But we can details this in a page in our 
github or in the qemu wiki if you think this is a good idea.
Here are the main remaining issues:
+ the base machine (we are using "none" here because it is "almost" 
empty and fit our needs but it has some limitations)
+ adding cpus
+ non-trivial memory mappings
+ solving backend (eg: net) connection issues

> 
> Yesterday I was thinking about this, and one thing I was wondering is
> if it would be possible to have DeviceClass and MachineClass implement
> a populate_fdt() handler, to automatically generate custom DTB for these
> custom machines.
> 
> Maybe in your case you don't need that, as your framework generating the
> QEMU machine also generates the DTB, or even parse a DTB to generate the
> machine... :)

You are right, we do not need this. We indeed use a device tree file to 
describe the platform but this is an "extended" device tree (it has more 
info than needed for linux). If it was not the case, I think it would 
still be easier to generate it from the source platform description than 
using qemu as an intermediate.

It is probably possible but it is tricky to generate the dtb: mapping in 
dtb are not standardized and really depends on the node types.

For example, to generate the interrupt-map property of a device node. 
You need to know the interrupt mapping (which interrupt line goes in 
which interrupt controller) and also have info about the interrupt 
controller's cells format (eg: how many bytes do we need to specify the 
interrupt). For example for some controller, you have specify the 
interrupt kind (rising or falling edge, high or low active level).

So you'll probably need some special "get_dtb_interrupt_cell" method in 
interrupt controllers to generate these entries for you so that a device 
can ask dtb data to its controller.

Bus mappings also depend on the bus type, but since qemu devices are 
already organized on a bus-type basis, this is probably easier to solve.

You'll have similar issues with every mapping. But bus and interrupt are 
the most important ones.

Thanks,
Damien


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

* Re: [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready
  2021-09-23 11:04   ` Ani Sinha
@ 2021-09-23 11:55     ` Ani Sinha
  2021-09-23 14:04       ` Damien Hedde
  0 siblings, 1 reply; 41+ messages in thread
From: Ani Sinha @ 2021-09-23 11:55 UTC (permalink / raw)
  To: Ani Sinha
  Cc: Damien Hedde, qemu-devel, Alistair Francis,
	Marc-André Lureau, Paolo Bonzini, Eduardo Habkost,
	Marcel Apfelbaum, Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Gerd Hoffmann, Eric Auger,
	Stefano Stabellini, Anthony Perard, Paul Durrant, Peter Xu,
	David Hildenbrand, Dr. David Alan Gilbert, Markus Armbruster,
	Eric Blake, qemu-riscv, xen-devel, mark.burton, mirela.grujic,
	edgari, Peter Maydell



On Thu, 23 Sep 2021, Ani Sinha wrote:

>
>
> On Wed, 22 Sep 2021, Damien Hedde wrote:
>
> > Skip the sysbus device type per-machine allow-list check before the
> > MACHINE_INIT_PHASE_READY phase.
> >
> > This patch permits adding any sysbus device (it still needs to be
> > user_creatable) when using the -preconfig experimental option.
> >
> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > ---
> >
> > This commit is RFC. Depending on the condition to allow a device
> > to be added, it may change.
> > ---
> >  softmmu/qdev-monitor.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > index f1c9242855..73b991adda 100644
> > --- a/softmmu/qdev-monitor.c
> > +++ b/softmmu/qdev-monitor.c
> > @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
> >          return NULL;
> >      }
> >
> > -    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
> > -        /* sysbus devices need to be allowed by the machine */
> > +    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) &&
> > +        phase_check(MACHINE_INIT_PHASE_READY)) {
> > +        /*
> > +         * Sysbus devices need to be allowed by the machine.
> > +         * We only check that after the machine is ready in order to let
> > +         * us add any user_creatable sysbus device during machine creation.
> > +         */
> >          MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
> >          if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) {
> >              error_setg(errp, "'%s' is not an allowed pluggable sysbus device "
>
> Since now you are adding the state of the machine creation in the
> valiation condition, the failure error message becomes misleading.
> Better to do this I think :
>
> if (object class is TYPE_SYS_BUS_DEVICE)
> {
>   if (!phase_check(MACHINE_INIT_PHASE_READY))
>     {
>       // error out here saying the state of the machine creation is too
> early
>     }
>
>     // do the other check on dynamic sysbus
>
> }

The other thing to consider is whether we should put the machine phaze
check at a higher level, at qdev_device_add() perhaps. One might envison
that different device types may be allowed to be added at different
stages of machine creation. So this check needs be more generalized for
the future.




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

* Re: [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command
  2021-09-22 17:37   ` Philippe Mathieu-Daudé
@ 2021-09-23 12:43     ` Damien Hedde
  2021-09-23 13:46       ` Ani Sinha
  0 siblings, 1 reply; 41+ messages in thread
From: Damien Hedde @ 2021-09-23 12:43 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Peter Maydell, Michael S. Tsirkin, David Hildenbrand, Peter Xu,
	mirela.grujic, Alistair Francis, Gerd Hoffmann, Ani Sinha,
	Eric Blake, Stefano Stabellini, xen-devel, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, qemu-riscv, Daniel P. Berrangé,
	mark.burton, edgari, Igor Mammedov



On 9/22/21 19:37, Philippe Mathieu-Daudé wrote:
> On 9/22/21 18:13, Damien Hedde 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>
> 
> Missing your S-o-b, otherwise:

Sorry. I did not realize I had to add it since it was already done by 
Mirela. I'll add it for this patch and patches 1, 6 and 8.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com >
>> ---
>>   qapi/machine.json          | 56 ++++++++++++++++++++++++++++++++++++++
>>   include/hw/qdev-core.h     | 30 ++------------------
>>   hw/core/machine-qmp-cmds.c |  9 ++++++
>>   hw/core/qdev.c             |  5 ++++
>>   4 files changed, 72 insertions(+), 28 deletions(-)
> 


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

* Re: [RFC PATCH v2 07/16] hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed
  2021-09-23 10:51   ` Ani Sinha
@ 2021-09-23 13:07     ` Damien Hedde
  0 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-23 13:07 UTC (permalink / raw)
  To: Ani Sinha
  Cc: qemu-devel, Alistair Francis, Marc-André Lureau,
	Paolo Bonzini, Eduardo Habkost, Marcel Apfelbaum,
	Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Gerd Hoffmann, Eric Auger,
	Stefano Stabellini, Anthony Perard, Paul Durrant, Peter Xu,
	David Hildenbrand, Dr. David Alan Gilbert, Markus Armbruster,
	Eric Blake, qemu-riscv, xen-devel, mark.burton, mirela.grujic,
	edgari, Peter Maydell



On 9/23/21 12:51, Ani Sinha wrote:
> 
> 
> On Wed, 22 Sep 2021, Damien Hedde wrote:
> 
>> Right now the allowance check for adding a sysbus device using
>> -device cli option (or device_add qmp command) is done well after
>> the device has been created. It is done during the machine init done
>> notifier: machine_init_notify() in hw/core/machine.c
>>
>> This new function will allow us to check if a sysbus device type is
>> allowed to be dynamically created by the machine during the device
>> creation time.
>>
>> Also make device_is_dynamic_sysbus() use the new function.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>
>> In the context of our series, we need to be able to do the check at
>> device creation time to allow doing it depending on the current
>> MACHINE_INIT phase.
>> ---
>>   include/hw/boards.h | 17 +++++++++++++++++
>>   hw/core/machine.c   | 15 ++++++++++++---
>>   2 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 463a5514f9..934443c1cd 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -51,6 +51,23 @@ void machine_set_cpu_numa_node(MachineState *machine,
>>    */
>>   void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>>
>> +/**
>> + * machine_class_is_dynamic_sysbus_dev_allowed: Check if type is an allowed
>> + * sysbus device type for the machine class.
>> + * @mc: Machine class
>> + * @type: type to check (should be a subtype of TYPE_SYS_BUS_DEVICE)
>> + *
>> + * Returns: true if @type is a type in the machine's list of
>> + * dynamically pluggable sysbus devices; otherwise false.
>> + *
>> + * Check if the QOM type @type is in the list of allowed sysbus device
>> + * types (see machine_class_allowed_dynamic_sysbus_dev()).
>> + * Note that if @type is a subtype of a type which is in the list, it is
>> + * allowed too.
>> + */
>> +bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
>> +                                                 const char *type);
>> +
> 
> How about renaming this to device_type_is_allowed_dynamic_sysbus() ?

I followed the "machine_class_allow_dynamic_sysbus_dev" function name style.

But sure I'll rename it. We can even drop the "allowed" if we want
to stay closer to the device_is_dynamic_sysbus legacy function which if 
used to check this on a device instance.

Damien
--
Thanks


> 
>>   /**
>>    * device_is_dynamic_sysbus: test whether device is a dynamic sysbus device
>>    * @mc: Machine class
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 9125c9aad0..1a18912dc8 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -545,18 +545,27 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type)
>>
>>   bool device_is_dynamic_sysbus(MachineClass *mc, DeviceState *dev)
>>   {
>> -    bool allowed = false;
>> -    strList *wl;
>>       Object *obj = OBJECT(dev);
>>
>>       if (!object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE)) {
>>           return false;
>>       }
>>
>> +    return machine_class_is_dynamic_sysbus_dev_allowed(mc,
>> +            object_get_typename(obj));
>> +}
>> +
>> +bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
>> +                                                 const char *type)
>> +{
>> +    bool allowed = false;
>> +    strList *wl;
>> +    ObjectClass *klass = object_class_by_name(type);
>> +
>>       for (wl = mc->allowed_dynamic_sysbus_devices;
>>            !allowed && wl;
>>            wl = wl->next) {
>> -        allowed |= !!object_dynamic_cast(obj, wl->value);
>> +        allowed |= !!object_class_dynamic_cast(klass, wl->value);
>>       }
>>
>>       return allowed;
>> --
>> 2.33.0
>>
>>


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

* Re: [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command
  2021-09-23 12:43     ` Damien Hedde
@ 2021-09-23 13:46       ` Ani Sinha
  0 siblings, 0 replies; 41+ messages in thread
From: Ani Sinha @ 2021-09-23 13:46 UTC (permalink / raw)
  To: Damien Hedde
  Cc: Philippe Mathieu-Daudé,
	QEMU Developers, Peter Maydell, Michael S. Tsirkin,
	David Hildenbrand, Peter Xu, mirela.grujic, Alistair Francis,
	Gerd Hoffmann, Eric Blake, Stefano Stabellini, xen-devel,
	Paul Durrant, Markus Armbruster, Anthony Perard,
	Marc-André Lureau, Eduardo Habkost, Dr. David Alan Gilbert,
	Eric Auger, Paolo Bonzini, qemu-riscv, Daniel P. Berrangé,
	mark.burton, edgari, Igor Mammedov

On Thu, Sep 23, 2021 at 6:13 PM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
>
> On 9/22/21 19:37, Philippe Mathieu-Daudé wrote:
> > On 9/22/21 18:13, Damien Hedde 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>
> >
> > Missing your S-o-b, otherwise:
>
> Sorry. I did not realize I had to add it since it was already done by
> Mirela. I'll add it for this patch and patches 1, 6 and 8.

https://wiki.qemu.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line
btw, it's strictly not mandatory if you are both from the same company
and you did not write the patch or contribute in anyway to it.

>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com >
> >> ---
> >>   qapi/machine.json          | 56 ++++++++++++++++++++++++++++++++++++++
> >>   include/hw/qdev-core.h     | 30 ++------------------
> >>   hw/core/machine-qmp-cmds.c |  9 ++++++
> >>   hw/core/qdev.c             |  5 ++++
> >>   4 files changed, 72 insertions(+), 28 deletions(-)
> >


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

* Re: [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready
  2021-09-23 11:55     ` Ani Sinha
@ 2021-09-23 14:04       ` Damien Hedde
  0 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-09-23 14:04 UTC (permalink / raw)
  To: Ani Sinha
  Cc: Peter Maydell, Michael S. Tsirkin, David Hildenbrand, qemu-devel,
	Peter Xu, mirela.grujic, Alistair Francis, Gerd Hoffmann,
	Eric Blake, Stefano Stabellini, xen-devel, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, qemu-riscv, Daniel P. Berrangé,
	mark.burton, edgari, Igor Mammedov



On 9/23/21 13:55, Ani Sinha wrote:
> 
> 
> On Thu, 23 Sep 2021, Ani Sinha wrote:
> 
>>
>>
>> On Wed, 22 Sep 2021, Damien Hedde wrote:
>>
>>> Skip the sysbus device type per-machine allow-list check before the
>>> MACHINE_INIT_PHASE_READY phase.
>>>
>>> This patch permits adding any sysbus device (it still needs to be
>>> user_creatable) when using the -preconfig experimental option.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>>
>>> This commit is RFC. Depending on the condition to allow a device
>>> to be added, it may change.
>>> ---
>>>   softmmu/qdev-monitor.c | 9 +++++++--
>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>>> index f1c9242855..73b991adda 100644
>>> --- a/softmmu/qdev-monitor.c
>>> +++ b/softmmu/qdev-monitor.c
>>> @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>>>           return NULL;
>>>       }
>>>
>>> -    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
>>> -        /* sysbus devices need to be allowed by the machine */
>>> +    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) &&
>>> +        phase_check(MACHINE_INIT_PHASE_READY)) {
>>> +        /*
>>> +         * Sysbus devices need to be allowed by the machine.
>>> +         * We only check that after the machine is ready in order to let
>>> +         * us add any user_creatable sysbus device during machine creation.
>>> +         */
>>>           MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
>>>           if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) {
>>>               error_setg(errp, "'%s' is not an allowed pluggable sysbus device "
>>
>> Since now you are adding the state of the machine creation in the
>> valiation condition, the failure error message becomes misleading.
>> Better to do this I think :
>>
>> if (object class is TYPE_SYS_BUS_DEVICE)
>> {
>>    if (!phase_check(MACHINE_INIT_PHASE_READY))
>>      {
>>        // error out here saying the state of the machine creation is too
>> early
>>      }
>>
>>      // do the other check on dynamic sysbus
>>
>> }
> 
> The other thing to consider is whether we should put the machine phaze
> check at a higher level, at qdev_device_add() perhaps. One might envison
> that different device types may be allowed to be added at different
> stages of machine creation. So this check needs be more generalized for
> the future.
> 

Hi Ani,

I think moving out the allowance check from qdev_get_device_class is a 
good idea. The code will be more clear even if the check is not generalized.

Thanks,
--
Damien



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

* Re: [RFC PATCH v2 00/16] Initial support for machine creation via QMP
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (16 preceding siblings ...)
  2021-09-22 17:50 ` [RFC PATCH v2 00/16] Initial support for machine creation via QMP Philippe Mathieu-Daudé
@ 2021-10-04 15:56 ` Damien Hedde
  2021-10-12 22:16 ` Alistair Francis
  18 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-10-04 15:56 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alistair Francis, Marc-André Lureau, Paolo Bonzini,
	Eduardo Habkost, Marcel Apfelbaum, Daniel P. Berrangé,
	Michael S. Tsirkin, Igor Mammedov, Ani Sinha, Gerd Hoffmann,
	Eric Auger, Stefano Stabellini, Anthony Perard, Paul Durrant,
	Peter Xu, David Hildenbrand, Dr. David Alan Gilbert,
	Markus Armbruster, Eric Blake, qemu-riscv, xen-devel,
	mark.burton, mirela.grujic, edgari, Peter Maydell, John Snow,
	Kevin Wolf

Hi,

This is both a ping and a small update. It would be great to have some 
feedback about patches 1 and 3.

Right now the device part of this series conflicts with Kevin 's work 
about replacing the QemuOpts by a QemuDict in device_add:
https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg06136.html

So I'll have at least to rebase on top of his series, remove and rework 
some of the patches.

Maybe this series is too big and we should split it anyway ? We could 
for example target 3 smaller series:
  1. -> about stopping during the machine 'initialized' phase
  2. -> about enabling using device_add QMP commands during this phase
  3. -> about the sysbus device case (some of the patches are even 
independent)

Thanks,
Damien

On 9/22/21 18:13, Damien Hedde wrote:
> Hi,
> 
> The goal of this work is to bring dynamic machine creation to QEMU:
> we want to setup a machine without compiling a specific machine C
> code. It would ease supporting highly configurable platforms (for
> example resulting from an automated design flow). The requirements
> for such configuration include begin able to specify the number of
> cores, available peripherals, emmory mapping, IRQ mapping, etc.
> 
> This series focuses on the first step: populating a machine with
> devices during its creation. We propose patches to support this
> using QMP commands. This is a working set of patches and improves
> over the earlier rfc (posted in May):
> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html
> 
> Although it is working and could be merged, it is tag as an RFC:
> we probably need to discuss the conditions for allowing a device to
> be created at an early stage. Patches 6, 10 and 13, 15 and 16 depend
> on such conditions and are subject to change. Other patches are
> unrelated to this point.
> 
> We address several issues in this series. They are detailed below.
> 
> ## 1. Stoping QEMU to populate the machine with devices
> 
> QEMU goes through several steps (called _machine phases_) when
> creating the machine: 'no-machine', 'machine-created',
> 'accel-created', 'initialized', and finally 'ready'. At 'ready'
> phase, QEMU is ready to start (see Paolo's page
> https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence for
> more details).
> 
> Using the -preconfig CLI option, QEMU can be stopped today during
> the 'accel-created' phase. Then the 'x-exit-preconfig' QMP command
> triggers QEMU moving forwards to the completion of the machine
> creation ('ready' phase).
> 
> The devices are created during the 'initialized' phase.
> In this phase the machine init() method has been executed and thus
> machine properties have been handled. Although the sysbus exists and
> the machine may have been populated by the init(),
> _machine_init_done_ notifiers have not been called yet. At this point
> we can add more devices to a machine.
> 
> We propose to add 2 QMP commands:
> + The 'query-machine-phase' command would return the current machine
>    phase.
> + The 'x-machine-init' command would advance the machine phase to
>    'initialized'. 'x-exit-preconfig' could then still be used to
>    advance to the last phase.
> 
> ## 2. Adding devices
> 
> Right now, the user can create devices in 2 ways: using '-device' CLI
> option or 'device_add' QMP command. Both are executed after the
> machine is ready: such devices are hot-plugged. We propose to allow
> 'device_add' QMP command to be used during the 'initialized' phase.
> 
> In this series, we keep the constraint that the device must be
> 'user-creatable' (this is a device class flag). We do not see any
> reason why a device the user can hot-plug could not be created at an
> earlier stage.
> 
> This part is still RFC because, as Peter mentioned it (in this thread
> https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg01933.html),
> we may want additional or distinct conditions for:
> + device we can hot-plug
> + device we can add in '-preconfig' (cold-plug)
> We are open to suggestions. We could for example add a
> 'preconfig-creatable' or 'init-creatable' flag to device class, which
> can identify a set of devices we can create this way.
> 
> The main addition is how we handle the case of sysbus devices. Sysbus
> devices are particular because unlike, for example, pci devices, you
> have to manually handle the memory mapping and interrupts wiring. So
> right now, a sysbus device is dynamically creatable (using -device
> CLI option or device_add QMP command) only if:
> + it is 'user_creatable' (like any other device),
> + and it is in the current machine sysbus device allow list.
> 
> In this series, we propose to relax the second constraint during the
> earlier phases of machine creation so that when using -preconfig we
> can create any 'user-creatable' sysbus device. When the machine
> progresses to the 'ready' phase, sysbus devices creation will come
> back to the legacy behavior: it will be possible only based on the
> per-machine authorization basis.
> 
> For sysbus devices, wiring interrupts is not a problem as we can use
> the 'qom-set' QMP command, but memory mapping is.
> 
> ## 3. Memory mapping
> 
> There is no point allowing the creation sysbus devices if we cannot
> map them onto the memory bus (the 'sysbus').
> 
> As far as we know, right now, there is no way to add memory mapping
> for sysbus device using QMP commands. We propose a 'x-sysbus-mmio-map'
> command to do this. This command would only be allowed during the
> 'initialized' phase when using -preconfig.
> 
> ## 4. Working example
> 
> The last patches of the series add and modify devices in order to
> build a working machine starting from the 'none' machine.
> 
> We add a new sysbus device modeling a simple memory (ram or rom). We
> also set 'user-creatable' flag of some sysbus devices. These are
> trivial patches, but they depends on the conditions we choose to allow
> creating devices with -preconfig. Therefore, there is really no need
> to review them until we settled on the device conditions first.
> 
> With these devices (memory, ibex_uart, ibex_plic) we can dynamically
> configure a part (we did not add the timer, but we could) the
> opentitan machine very easily and run firmwares which demonstrates
> interrupts and memory-mapping are working.
> 
> We use the existing qmp-shell script to issue machine devices
> from a qmp commands script file which contains qmp commands listed in
> a file.
> 
> The following qmp commands add some memories, an interrupt controller
> and an uart with an interrupt.
> 
> cat > opentitan.qmp <<EOF
> x-machine-init
> 
> # ROM 0x00008000
> device_add        driver=sysbus-memory id=rom size=0x4000 readonly=true
> x-sysbus-mmio-map device=rom addr=32768
> 
> # FLASH 0x20000000
> device_add        driver=sysbus-memory id=flash size=0x80000 readonly=true
> x-sysbus-mmio-map device=flash addr=536870912
> 
> # RAM 0x10000000
> device_add        driver=sysbus-memory id=ram size=0x10000
> x-sysbus-mmio-map device=ram addr=268435456
> 
> # PLIC 0x41010000
> device_add        driver=ibex-plic id=plic
> x-sysbus-mmio-map device=plic addr=1090584576
> 
> # UART 0x40000000
> device_add        driver=ibex-uart id=uart chardev=serial0
> x-sysbus-mmio-map device=uart addr=1073741824
> qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2]
> 
> x-exit-preconfig
> EOF
> 
> We've put the opentitan.qmp and a firmware opentitan-echo.elf here
> (among some other qmp machine files we are working on):
> https://github.com/GreenSocs/qemu-qmp-machines
> This firmware is just a small interrupt-based program echoing back
> whatever is sent in the uart.
> 
> QEMU should be run using the following command:
> qemu-system-riscv32 -preconfig -qmp unix:/tmp/qmp-socket,server \
>      -display none \
>      -M none -cpu lowrisc-ibex \
>      -serial mon:stdio \
>      -device loader,addr=0x8090,cpu-num=0 \
>      -device loader,file=opentitan-hello.elf \
> 
> and in other terminal to do the configuration (grep is just here to
> remove comments):
> grep -v -e '^#' opentitan.qmp | qmp-shell -v /tmp/qmp-socket
> 
> Alternatively we can load the firmware on the existing machine and
> observe the same behavior:
> qemu-system-riscv32 -display none \
>       -M opentitan \
>       -serial mon:stdio \
>       -kernel opentitan-echo.elf
> 
> We chose this example because it is very simple and does not need a
> lot of devices.
> 
> This work has still a lot of limitations. Cpus config is done the
> normal way (the C machine does that): in our example we used the
> 'none' machine. We have work to do for handling backend
> connection (for example net/nic are complicated) because the way it
> is done in machine C code does not translate easily in QMP commands.
> Firmware loading is also a bit tricky. We plan to work on this in
> follow-up series.
> 
> The series is organized as follows:
> - Patches 1 to 3 add qmp support to stop QEMU at an early phase
>    to populate the machine with devices.
> - Patches 4 to 6 prepare and allow issuing device_add during this phase.
> - Patches 7 to 10 prepare and allow creating sysbus device during this phase.
> - Patches 11 and 12 add the x-sysbus-mmio-map QMP command
> - Patch 13 add the memory sysbus device to model ram and rom
> - Patch 14 adds some documentation
> - Patches 15 and 16 set 'user_creatable' flag of ibex_uart and ibex_plic.
> 
> This work is supported by Greensocs, Sifive and Xilinx.
> 
> Thanks,
> --
> Damien
> 
> Damien Hedde (12):
>    softmmu/qdev-monitor: add error handling in qdev_set_id
>    qdev-monitor: prevent conflicts between qmp/device_add and cli/-device
>    hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed
>    qdev-monitor: Check sysbus device type before creating it
>    hw/core/machine: Remove the dynamic sysbus devices type check
>    qdev-monitor: allow adding any sysbus device before machine is ready
>    softmmu/memory: add memory_region_try_add_subregion function
>    add x-sysbus-mmio-map qmp command
>    hw/mem/system-memory: add a memory sysbus device
>    docs/system: add doc about the initialized machine phase and an
>      example
>    hw/char/ibex_uart: set user_creatable
>    hw/intc/ibex_plic: set user_creatable
> 
> 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 | 77 +++++++++++++++++++++++++++++
>   qapi/machine.json               | 79 ++++++++++++++++++++++++++++++
>   qapi/qdev.json                  | 24 ++++++++-
>   include/exec/memory.h           | 22 +++++++++
>   include/hw/boards.h             | 18 ++++++-
>   include/hw/mem/sysbus-memory.h  | 32 ++++++++++++
>   include/hw/qdev-core.h          | 30 +-----------
>   include/monitor/qdev.h          | 25 +++++++++-
>   hw/char/ibex_uart.c             |  1 +
>   hw/core/machine-qmp-cmds.c      | 11 ++++-
>   hw/core/machine.c               | 48 ++++++------------
>   hw/core/qdev.c                  |  7 ++-
>   hw/core/sysbus.c                | 41 ++++++++++++++++
>   hw/intc/ibex_plic.c             |  1 +
>   hw/mem/sysbus-memory.c          | 83 +++++++++++++++++++++++++++++++
>   hw/pci/pci.c                    |  2 +-
>   hw/usb/core.c                   |  2 +-
>   hw/virtio/virtio-iommu.c        |  2 +-
>   hw/xen/xen-legacy-backend.c     |  3 +-
>   monitor/hmp.c                   |  2 +-
>   monitor/misc.c                  |  2 +-
>   softmmu/memory.c                | 22 ++++++---
>   softmmu/qdev-monitor.c          | 86 +++++++++++++++++++++++++++------
>   softmmu/vl.c                    | 23 ++++++---
>   ui/console.c                    |  3 +-
>   hw/mem/meson.build              |  2 +
>   26 files changed, 547 insertions(+), 101 deletions(-)
>   create mode 100644 include/hw/mem/sysbus-memory.h
>   create mode 100644 hw/mem/sysbus-memory.c
> 


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

* Re: [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command
  2021-09-22 16:13 ` [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command Damien Hedde
  2021-09-22 17:37   ` Philippe Mathieu-Daudé
@ 2021-10-12 22:08   ` Alistair Francis
  1 sibling, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2021-10-12 22:08 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov

On Thu, Sep 23, 2021 at 2:20 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>

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

Alistair

> ---
>  qapi/machine.json          | 56 ++++++++++++++++++++++++++++++++++++++
>  include/hw/qdev-core.h     | 30 ++------------------
>  hw/core/machine-qmp-cmds.c |  9 ++++++
>  hw/core/qdev.c             |  5 ++++
>  4 files changed, 72 insertions(+), 28 deletions(-)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 157712f006..969d37fb03 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1312,3 +1312,59 @@
>       '*cores': 'int',
>       '*threads': 'int',
>       '*maxcpus': 'int' } }
> +
> +##
> +# @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: 6.2
> +##
> +{ 'enum': 'MachineInitPhase',
> +  'data': [ 'no-machine', 'machine-created', 'accel-created', 'initialized',
> +            'ready' ] }
> +
> +##
> +# @MachineInitPhaseStatus:
> +#
> +# Information about machine initialization phase
> +#
> +# @phase: the machine initialization phase
> +#
> +# Since: 6.2
> +##
> +{ 'struct': 'MachineInitPhaseStatus',
> +  'data': { 'phase': 'MachineInitPhase' } }
> +
> +##
> +# @query-machine-phase:
> +#
> +# Return machine initialization phase
> +#
> +# Since: 6.2
> +#
> +# Returns: MachineInitPhaseStatus
> +#
> +# Example:
> +#
> +# -> { "execute": "query-machine-phase" }
> +# <- { "return": { "phase": "initialized" } }
> +#
> +##
> +{ 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
> +             'allow-preconfig': true }
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 859fd913bb..800eda8f54 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"
> @@ -839,35 +840,8 @@ void device_listener_unregister(DeviceListener *listener);
>   */
>  bool qdev_should_hide_device(QemuOpts *opts);
>
> -typedef enum MachineInitPhase {
> -    /* current_machine is NULL.  */
> -    MACHINE_INIT_PHASE_NO_MACHINE,
> -
> -    /* current_machine is not NULL, but current_machine->accel is NULL.  */
> -    MACHINE_INIT_PHASE_MACHINE_CREATED,
> -
> -    /*
> -     * current_machine->accel is not NULL, but the machine properties have
> -     * not been validated and machine_class->init has not yet been called.
> -     */
> -    MACHINE_INIT_PHASE_ACCEL_CREATED,
> -
> -    /*
> -     * machine_class->init has been called, thus creating any embedded
> -     * devices and validating machine properties.  Devices created at
> -     * this time are considered to be cold-plugged.
> -     */
> -    MACHINE_INIT_PHASE_INITIALIZED,
> -
> -    /*
> -     * QEMU is ready to start CPUs and devices created at this time
> -     * are considered to be hot-plugged.  The monitor is not restricted
> -     * to "preconfig" commands.
> -     */
> -    MACHINE_INIT_PHASE_READY,
> -} MachineInitPhase;
> -
>  extern bool phase_check(MachineInitPhase phase);
>  extern void phase_advance(MachineInitPhase phase);
> +extern MachineInitPhase phase_get(void);
>
>  #endif
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 52168a3771..d3b9a04855 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -204,3 +204,12 @@ MemdevList *qmp_query_memdev(Error **errp)
>      object_child_foreach(obj, query_memdev, &list);
>      return list;
>  }
> +
> +MachineInitPhaseStatus *qmp_query_machine_phase(Error **errp)
> +{
> +    MachineInitPhaseStatus *status = g_malloc0(sizeof(*status));
> +
> +    status->phase = phase_get();
> +
> +    return status;
> +}
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index c5fc704f55..d83f1c029a 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -1150,6 +1150,11 @@ void phase_advance(MachineInitPhase phase)
>      machine_phase = phase;
>  }
>
> +MachineInitPhase phase_get(void)
> +{
> +    return machine_phase;
> +}
> +
>  static const TypeInfo device_type_info = {
>      .name = TYPE_DEVICE,
>      .parent = TYPE_OBJECT,
> --
> 2.33.0
>
>


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

* Re: [RFC PATCH v2 00/16] Initial support for machine creation via QMP
  2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
                   ` (17 preceding siblings ...)
  2021-10-04 15:56 ` Damien Hedde
@ 2021-10-12 22:16 ` Alistair Francis
  2021-10-13  5:56   ` Mark Burton
  18 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2021-10-12 22:16 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov

On Thu, Sep 23, 2021 at 2:22 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Hi,
>
> The goal of this work is to bring dynamic machine creation to QEMU:
> we want to setup a machine without compiling a specific machine C
> code. It would ease supporting highly configurable platforms (for
> example resulting from an automated design flow). The requirements
> for such configuration include begin able to specify the number of
> cores, available peripherals, emmory mapping, IRQ mapping, etc.
>
> This series focuses on the first step: populating a machine with
> devices during its creation. We propose patches to support this
> using QMP commands. This is a working set of patches and improves
> over the earlier rfc (posted in May):
> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html
>
> Although it is working and could be merged, it is tag as an RFC:
> we probably need to discuss the conditions for allowing a device to
> be created at an early stage. Patches 6, 10 and 13, 15 and 16 depend
> on such conditions and are subject to change. Other patches are
> unrelated to this point.
>
> We address several issues in this series. They are detailed below.
>
> ## 1. Stoping QEMU to populate the machine with devices
>
> QEMU goes through several steps (called _machine phases_) when
> creating the machine: 'no-machine', 'machine-created',
> 'accel-created', 'initialized', and finally 'ready'. At 'ready'
> phase, QEMU is ready to start (see Paolo's page
> https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence for
> more details).
>
> Using the -preconfig CLI option, QEMU can be stopped today during
> the 'accel-created' phase. Then the 'x-exit-preconfig' QMP command
> triggers QEMU moving forwards to the completion of the machine
> creation ('ready' phase).
>
> The devices are created during the 'initialized' phase.
> In this phase the machine init() method has been executed and thus
> machine properties have been handled. Although the sysbus exists and
> the machine may have been populated by the init(),
> _machine_init_done_ notifiers have not been called yet. At this point
> we can add more devices to a machine.
>
> We propose to add 2 QMP commands:
> + The 'query-machine-phase' command would return the current machine
>   phase.
> + The 'x-machine-init' command would advance the machine phase to
>   'initialized'. 'x-exit-preconfig' could then still be used to
>   advance to the last phase.
>
> ## 2. Adding devices
>
> Right now, the user can create devices in 2 ways: using '-device' CLI
> option or 'device_add' QMP command. Both are executed after the
> machine is ready: such devices are hot-plugged. We propose to allow
> 'device_add' QMP command to be used during the 'initialized' phase.
>
> In this series, we keep the constraint that the device must be
> 'user-creatable' (this is a device class flag). We do not see any
> reason why a device the user can hot-plug could not be created at an
> earlier stage.
>
> This part is still RFC because, as Peter mentioned it (in this thread
> https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg01933.html),
> we may want additional or distinct conditions for:
> + device we can hot-plug
> + device we can add in '-preconfig' (cold-plug)
> We are open to suggestions. We could for example add a
> 'preconfig-creatable' or 'init-creatable' flag to device class, which
> can identify a set of devices we can create this way.
>
> The main addition is how we handle the case of sysbus devices. Sysbus
> devices are particular because unlike, for example, pci devices, you
> have to manually handle the memory mapping and interrupts wiring. So
> right now, a sysbus device is dynamically creatable (using -device
> CLI option or device_add QMP command) only if:
> + it is 'user_creatable' (like any other device),
> + and it is in the current machine sysbus device allow list.
>
> In this series, we propose to relax the second constraint during the
> earlier phases of machine creation so that when using -preconfig we
> can create any 'user-creatable' sysbus device. When the machine
> progresses to the 'ready' phase, sysbus devices creation will come
> back to the legacy behavior: it will be possible only based on the
> per-machine authorization basis.
>
> For sysbus devices, wiring interrupts is not a problem as we can use
> the 'qom-set' QMP command, but memory mapping is.
>
> ## 3. Memory mapping
>
> There is no point allowing the creation sysbus devices if we cannot
> map them onto the memory bus (the 'sysbus').
>
> As far as we know, right now, there is no way to add memory mapping
> for sysbus device using QMP commands. We propose a 'x-sysbus-mmio-map'
> command to do this. This command would only be allowed during the
> 'initialized' phase when using -preconfig.
>
> ## 4. Working example
>
> The last patches of the series add and modify devices in order to
> build a working machine starting from the 'none' machine.
>
> We add a new sysbus device modeling a simple memory (ram or rom). We
> also set 'user-creatable' flag of some sysbus devices. These are
> trivial patches, but they depends on the conditions we choose to allow
> creating devices with -preconfig. Therefore, there is really no need
> to review them until we settled on the device conditions first.
>
> With these devices (memory, ibex_uart, ibex_plic) we can dynamically
> configure a part (we did not add the timer, but we could) the
> opentitan machine very easily and run firmwares which demonstrates
> interrupts and memory-mapping are working.
>
> We use the existing qmp-shell script to issue machine devices
> from a qmp commands script file which contains qmp commands listed in
> a file.
>
> The following qmp commands add some memories, an interrupt controller
> and an uart with an interrupt.
>
> cat > opentitan.qmp <<EOF
> x-machine-init
>
> # ROM 0x00008000
> device_add        driver=sysbus-memory id=rom size=0x4000 readonly=true
> x-sysbus-mmio-map device=rom addr=32768
>
> # FLASH 0x20000000
> device_add        driver=sysbus-memory id=flash size=0x80000 readonly=true
> x-sysbus-mmio-map device=flash addr=536870912
>
> # RAM 0x10000000
> device_add        driver=sysbus-memory id=ram size=0x10000
> x-sysbus-mmio-map device=ram addr=268435456
>
> # PLIC 0x41010000
> device_add        driver=ibex-plic id=plic
> x-sysbus-mmio-map device=plic addr=1090584576
>
> # UART 0x40000000
> device_add        driver=ibex-uart id=uart chardev=serial0
> x-sysbus-mmio-map device=uart addr=1073741824
> qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2]
>
> x-exit-preconfig
> EOF
>
> We've put the opentitan.qmp and a firmware opentitan-echo.elf here
> (among some other qmp machine files we are working on):
> https://github.com/GreenSocs/qemu-qmp-machines

I am unable to access this repo, maybe it's not public?

Alistair


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

* Re: [RFC PATCH v2 03/16] qapi: Implement x-machine-init QMP command
  2021-09-22 16:13 ` [RFC PATCH v2 03/16] qapi: Implement x-machine-init " Damien Hedde
@ 2021-10-12 22:19   ` Alistair Francis
  0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2021-10-12 22:19 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov

On Thu, Sep 23, 2021 at 2:17 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> 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 future commits, we will add the possiblity
> to create devices at this point.
>
> To exit the initialized phase use the x-exit-preconfig QMP command.
>
> Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>

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

Alistair

> ---
>  qapi/machine.json | 23 +++++++++++++++++++++++
>  softmmu/vl.c      | 19 +++++++++++++++----
>  2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 969d37fb03..56330c0e8e 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1368,3 +1368,26 @@
>  ##
>  { 'command': 'query-machine-phase', 'returns': 'MachineInitPhaseStatus',
>               'allow-preconfig': true }
> +
> +##
> +# @x-machine-init:
> +#
> +# Enter machine initialized phase
> +#
> +# Since: 6.2
> +#
> +# Returns: If successful, 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. To exit the
> +#        machine initialized phase use the x-exit-preconfig command.
> +#
> +# Example:
> +#
> +# -> { "execute": "x-machine-init" }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'x-machine-init', 'allow-preconfig': true }
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index d2552ba8ac..84c5132ad7 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"
> @@ -2610,10 +2611,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_check(MACHINE_INIT_PHASE_INITIALIZED)) {
> +        error_setg(errp, "The command is permitted only before "
> +                         "the machine is initialized");
> +        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);
> @@ -2692,12 +2699,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
>
>


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

* Re: [RFC PATCH v2 06/16] qapi: Allow device_add to execute in machine initialized phase
  2021-09-22 16:13 ` [RFC PATCH v2 06/16] qapi: Allow device_add to execute in machine initialized phase Damien Hedde
@ 2021-10-12 22:25   ` Alistair Francis
  0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2021-10-12 22:25 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov

On Thu, Sep 23, 2021 at 2:26 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> From: Mirela Grujic <mirela.grujic@greensocs.com>
>
> To configure a machine using QMP we need the device_add command to
> execute at machine initialized phase.
>
> Note: for device_add command in qdev.json adding the 'allow-init-config'
> option has no effect because the command appears to bypass QAPI (see
> TODO at qapi/qdev.json:61). The option is added there solely to document
> the intent.
> For the same reason, the flags have to be explicitly set in
> monitor_init_qmp_commands() when the device_add command is registered.
>
> Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>

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

Alistair

> ---
>
> The commit is fine, but we may add intermediate commits before this one
> in order to add or change the condition for a device type to be accepted
> in the 'initialized' state (see the cover-letter of the series).
> ---
>  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 b83178220b..ad669ae175 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -67,7 +67,8 @@
>  ##
>  { 'command': 'device_add',
>    'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
> -  'gen': false } # so we can get the additional arguments
> +  'gen': false, # so we can get the additional arguments
> +  'allow-preconfig': true }
>
>  ##
>  # @device_del:
> diff --git a/monitor/misc.c b/monitor/misc.c
> index ffe7966870..2c476de316 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -231,7 +231,7 @@ static void monitor_init_qmp_commands(void)
>      qmp_init_marshal(&qmp_commands);
>
>      qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
> -                         QCO_NO_OPTIONS);
> +                         QCO_ALLOW_PRECONFIG);
>
>      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 834f2b56b5..47ccd90be8 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -824,6 +824,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
>
>


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

* Re: [RFC PATCH v2 08/16] qdev-monitor: Check sysbus device type before creating it
  2021-09-22 16:13 ` [RFC PATCH v2 08/16] qdev-monitor: Check sysbus device type before creating it Damien Hedde
@ 2021-10-12 22:42   ` Alistair Francis
  0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2021-10-12 22:42 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov

On Thu, Sep 23, 2021 at 2:53 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Add an early check to test if the requested sysbus device type
> is allowed by the current machine before creating the device. This
> impacts both -device cli option and device_add qmp command.
>
> Before this patch, the check was done well after the device has
> been created (in a machine init done notifier). We can now report
> the error right away.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

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

Alistair

> ---
>  softmmu/qdev-monitor.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 47ccd90be8..f1c9242855 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -40,6 +40,7 @@
>  #include "qemu/cutils.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/clock.h"
> +#include "hw/boards.h"
>
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> @@ -268,6 +269,16 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>          return NULL;
>      }
>
> +    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
> +        /* sysbus devices need to be allowed by the machine */
> +        MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
> +        if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) {
> +            error_setg(errp, "'%s' is not an allowed pluggable sysbus device "
> +                             " type for the machine", *driver);
> +            return NULL;
> +        }
> +    }
> +
>      return dc;
>  }
>
> --
> 2.33.0
>
>


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

* Re: [RFC PATCH v2 09/16] hw/core/machine: Remove the dynamic sysbus devices type check
  2021-09-22 16:13 ` [RFC PATCH v2 09/16] hw/core/machine: Remove the dynamic sysbus devices type check Damien Hedde
@ 2021-10-12 23:07   ` Alistair Francis
  0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2021-10-12 23:07 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov

On Thu, Sep 23, 2021 at 2:23 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> Now that we check sysbus device types during device creation, we
> can remove the check done in the machine init done notifier.
> This was the only thing done by this notifier, so we remove the
> whole sysbus_notifier structure of the MachineState.
>
> Note: This notifier was checking all /peripheral and /peripheral-anon
> sysbus devices. Now we only check those added by -device cli option or
> device_add qmp command when handling the command/option. So if there
> are some devices added in one of these containers manually (eg in
> machine C code), these will not be checked anymore.
> This use case does not seem to appear apart from
> hw/xen/xen-legacy-backend.c (it uses qdev_set_id() and in this case,
> not for a sysbus device, so it's ok).
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

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

Alistair

> ---
>  include/hw/boards.h |  1 -
>  hw/core/machine.c   | 27 ---------------------------
>  2 files changed, 28 deletions(-)
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 934443c1cd..ccbc40355a 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -311,7 +311,6 @@ typedef struct CpuTopology {
>  struct MachineState {
>      /*< private >*/
>      Object parent_obj;
> -    Notifier sysbus_notifier;
>
>      /*< public >*/
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1a18912dc8..521438e90a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -571,18 +571,6 @@ bool machine_class_is_dynamic_sysbus_dev_allowed(MachineClass *mc,
>      return allowed;
>  }
>
> -static void validate_sysbus_device(SysBusDevice *sbdev, void *opaque)
> -{
> -    MachineState *machine = opaque;
> -    MachineClass *mc = MACHINE_GET_CLASS(machine);
> -
> -    if (!device_is_dynamic_sysbus(mc, DEVICE(sbdev))) {
> -        error_report("Option '-device %s' cannot be handled by this machine",
> -                     object_class_get_name(object_get_class(OBJECT(sbdev))));
> -        exit(1);
> -    }
> -}
> -
>  static char *machine_get_memdev(Object *obj, Error **errp)
>  {
>      MachineState *ms = MACHINE(obj);
> @@ -598,17 +586,6 @@ static void machine_set_memdev(Object *obj, const char *value, Error **errp)
>      ms->ram_memdev_id = g_strdup(value);
>  }
>
> -static void machine_init_notify(Notifier *notifier, void *data)
> -{
> -    MachineState *machine = MACHINE(qdev_get_machine());
> -
> -    /*
> -     * Loop through all dynamically created sysbus devices and check if they are
> -     * all allowed.  If a device is not allowed, error out.
> -     */
> -    foreach_dynamic_sysbus_device(validate_sysbus_device, machine);
> -}
> -
>  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine)
>  {
>      int i;
> @@ -1030,10 +1007,6 @@ static void machine_initfn(Object *obj)
>                                          "Table (HMAT)");
>      }
>
> -    /* Register notifier when init is done for sysbus sanity checks */
> -    ms->sysbus_notifier.notify = machine_init_notify;
> -    qemu_add_machine_init_done_notifier(&ms->sysbus_notifier);
> -
>      /* default to mc->default_cpus */
>      ms->smp.cpus = mc->default_cpus;
>      ms->smp.max_cpus = mc->default_cpus;
> --
> 2.33.0
>
>


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

* Re: [RFC PATCH v2 00/16] Initial support for machine creation via QMP
  2021-10-12 22:16 ` Alistair Francis
@ 2021-10-13  5:56   ` Mark Burton
  0 siblings, 0 replies; 41+ messages in thread
From: Mark Burton @ 2021-10-13  5:56 UTC (permalink / raw)
  To: Alistair Francis
  Cc: Damien Hedde, qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V,
	"Daniel P. Berrangé",
	Edgar Iglesias, Igor Mammedov

Fixed
Cheers
Mark.


> On 13 Oct 2021, at 00:16, Alistair Francis <alistair23@gmail.com> wrote:
> 
> On Thu, Sep 23, 2021 at 2:22 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>> 
>> Hi,
>> 
>> The goal of this work is to bring dynamic machine creation to QEMU:
>> we want to setup a machine without compiling a specific machine C
>> code. It would ease supporting highly configurable platforms (for
>> example resulting from an automated design flow). The requirements
>> for such configuration include begin able to specify the number of
>> cores, available peripherals, emmory mapping, IRQ mapping, etc.
>> 
>> This series focuses on the first step: populating a machine with
>> devices during its creation. We propose patches to support this
>> using QMP commands. This is a working set of patches and improves
>> over the earlier rfc (posted in May):
>> https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg03706.html
>> 
>> Although it is working and could be merged, it is tag as an RFC:
>> we probably need to discuss the conditions for allowing a device to
>> be created at an early stage. Patches 6, 10 and 13, 15 and 16 depend
>> on such conditions and are subject to change. Other patches are
>> unrelated to this point.
>> 
>> We address several issues in this series. They are detailed below.
>> 
>> ## 1. Stoping QEMU to populate the machine with devices
>> 
>> QEMU goes through several steps (called _machine phases_) when
>> creating the machine: 'no-machine', 'machine-created',
>> 'accel-created', 'initialized', and finally 'ready'. At 'ready'
>> phase, QEMU is ready to start (see Paolo's page
>> https://wiki.qemu.org/User:Paolo_Bonzini/Machine_init_sequence for
>> more details).
>> 
>> Using the -preconfig CLI option, QEMU can be stopped today during
>> the 'accel-created' phase. Then the 'x-exit-preconfig' QMP command
>> triggers QEMU moving forwards to the completion of the machine
>> creation ('ready' phase).
>> 
>> The devices are created during the 'initialized' phase.
>> In this phase the machine init() method has been executed and thus
>> machine properties have been handled. Although the sysbus exists and
>> the machine may have been populated by the init(),
>> _machine_init_done_ notifiers have not been called yet. At this point
>> we can add more devices to a machine.
>> 
>> We propose to add 2 QMP commands:
>> + The 'query-machine-phase' command would return the current machine
>>  phase.
>> + The 'x-machine-init' command would advance the machine phase to
>>  'initialized'. 'x-exit-preconfig' could then still be used to
>>  advance to the last phase.
>> 
>> ## 2. Adding devices
>> 
>> Right now, the user can create devices in 2 ways: using '-device' CLI
>> option or 'device_add' QMP command. Both are executed after the
>> machine is ready: such devices are hot-plugged. We propose to allow
>> 'device_add' QMP command to be used during the 'initialized' phase.
>> 
>> In this series, we keep the constraint that the device must be
>> 'user-creatable' (this is a device class flag). We do not see any
>> reason why a device the user can hot-plug could not be created at an
>> earlier stage.
>> 
>> This part is still RFC because, as Peter mentioned it (in this thread
>> https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg01933.html),
>> we may want additional or distinct conditions for:
>> + device we can hot-plug
>> + device we can add in '-preconfig' (cold-plug)
>> We are open to suggestions. We could for example add a
>> 'preconfig-creatable' or 'init-creatable' flag to device class, which
>> can identify a set of devices we can create this way.
>> 
>> The main addition is how we handle the case of sysbus devices. Sysbus
>> devices are particular because unlike, for example, pci devices, you
>> have to manually handle the memory mapping and interrupts wiring. So
>> right now, a sysbus device is dynamically creatable (using -device
>> CLI option or device_add QMP command) only if:
>> + it is 'user_creatable' (like any other device),
>> + and it is in the current machine sysbus device allow list.
>> 
>> In this series, we propose to relax the second constraint during the
>> earlier phases of machine creation so that when using -preconfig we
>> can create any 'user-creatable' sysbus device. When the machine
>> progresses to the 'ready' phase, sysbus devices creation will come
>> back to the legacy behavior: it will be possible only based on the
>> per-machine authorization basis.
>> 
>> For sysbus devices, wiring interrupts is not a problem as we can use
>> the 'qom-set' QMP command, but memory mapping is.
>> 
>> ## 3. Memory mapping
>> 
>> There is no point allowing the creation sysbus devices if we cannot
>> map them onto the memory bus (the 'sysbus').
>> 
>> As far as we know, right now, there is no way to add memory mapping
>> for sysbus device using QMP commands. We propose a 'x-sysbus-mmio-map'
>> command to do this. This command would only be allowed during the
>> 'initialized' phase when using -preconfig.
>> 
>> ## 4. Working example
>> 
>> The last patches of the series add and modify devices in order to
>> build a working machine starting from the 'none' machine.
>> 
>> We add a new sysbus device modeling a simple memory (ram or rom). We
>> also set 'user-creatable' flag of some sysbus devices. These are
>> trivial patches, but they depends on the conditions we choose to allow
>> creating devices with -preconfig. Therefore, there is really no need
>> to review them until we settled on the device conditions first.
>> 
>> With these devices (memory, ibex_uart, ibex_plic) we can dynamically
>> configure a part (we did not add the timer, but we could) the
>> opentitan machine very easily and run firmwares which demonstrates
>> interrupts and memory-mapping are working.
>> 
>> We use the existing qmp-shell script to issue machine devices
>> from a qmp commands script file which contains qmp commands listed in
>> a file.
>> 
>> The following qmp commands add some memories, an interrupt controller
>> and an uart with an interrupt.
>> 
>> cat > opentitan.qmp <<EOF
>> x-machine-init
>> 
>> # ROM 0x00008000
>> device_add        driver=sysbus-memory id=rom size=0x4000 readonly=true
>> x-sysbus-mmio-map device=rom addr=32768
>> 
>> # FLASH 0x20000000
>> device_add        driver=sysbus-memory id=flash size=0x80000 readonly=true
>> x-sysbus-mmio-map device=flash addr=536870912
>> 
>> # RAM 0x10000000
>> device_add        driver=sysbus-memory id=ram size=0x10000
>> x-sysbus-mmio-map device=ram addr=268435456
>> 
>> # PLIC 0x41010000
>> device_add        driver=ibex-plic id=plic
>> x-sysbus-mmio-map device=plic addr=1090584576
>> 
>> # UART 0x40000000
>> device_add        driver=ibex-uart id=uart chardev=serial0
>> x-sysbus-mmio-map device=uart addr=1073741824
>> qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2]
>> 
>> x-exit-preconfig
>> EOF
>> 
>> We've put the opentitan.qmp and a firmware opentitan-echo.elf here
>> (among some other qmp machine files we are working on):
>> https://github.com/GreenSocs/qemu-qmp-machines
> 
> I am unable to access this repo, maybe it's not public?
> 
> Alistair



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

* Re: [RFC PATCH v2 04/16] softmmu/qdev-monitor: add error handling in qdev_set_id
  2021-09-22 16:13 ` [RFC PATCH v2 04/16] softmmu/qdev-monitor: add error handling in qdev_set_id Damien Hedde
@ 2021-10-13  7:10   ` Alistair Francis
  2021-10-13 14:30     ` Damien Hedde
  0 siblings, 1 reply; 41+ messages in thread
From: Alistair Francis @ 2021-10-13  7:10 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov

On Thu, Sep 23, 2021 at 2:29 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> qdev_set_id() is mostly used when the user adds a device (using
> -device cli option or device_add qmp command). This commit adds
> an error parameter to handle the case where the given id is
> already taken.
>
> Also document the function and add a return value in order to
> be able to capture success/failure: the function now returns the
> id in case of success, or NULL in case of failure.
>
> The commit modifies the 2 calling places (qdev-monitor and
> xen-legacy-backend) to add the error object parameter.
>
> Note that the id is, right now, guaranteed to be unique because
> all ids came from the "device" QemuOptsList where the id is used
> as key. This addition is a preparation for a future commit which
> will relax the uniqueness.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

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

Alistair

> ---
>  include/monitor/qdev.h      | 25 +++++++++++++++++++++++-
>  hw/xen/xen-legacy-backend.c |  3 ++-
>  softmmu/qdev-monitor.c      | 38 +++++++++++++++++++++++++++----------
>  3 files changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index eaa947d73a..23c31f5296 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -9,6 +9,29 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp);
>
>  int qdev_device_help(QemuOpts *opts);
>  DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
> -void qdev_set_id(DeviceState *dev, const char *id);
> +
> +/**
> + * qdev_set_id: parent the device and set its id if provided.
> + * @dev: device to handle
> + * @id: id to be given to the device, or NULL.
> + *
> + * Returns: the id of the device in case of success; otherwise NULL.
> + *
> + * @dev must be unrealized, unparented and must not have an id.
> + *
> + * If @id is non-NULL, this function tries to setup @dev qom path as
> + * "/peripheral/id". If @id is already taken, it fails. If it succeeds,
> + * the id field of @dev is set to @id (@dev now owns the given @id
> + * parameter).
> + *
> + * If @id is NULL, this function generates a unique name and setups @dev
> + * qom path as "/peripheral-anon/name". This name is not set as the id
> + * of @dev.
> + *
> + * Upon success, it returns the id/name (generated or provided). The
> + * returned string is owned by the corresponding child property and must
> + * not be freed by the caller.
> + */
> +const char *qdev_set_id(DeviceState *dev, const char *id, Error **errp);
>
>  #endif
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index dd8ae1452d..f541cfa0e9 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -276,7 +276,8 @@ static struct XenLegacyDevice *xen_be_get_xendev(const char *type, int dom,
>      xendev = g_malloc0(ops->size);
>      object_initialize(&xendev->qdev, ops->size, TYPE_XENBACKEND);
>      OBJECT(xendev)->free = g_free;
> -    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev));
> +    qdev_set_id(DEVICE(xendev), g_strdup_printf("xen-%s-%d", type, dev),
> +                &error_fatal);
>      qdev_realize(DEVICE(xendev), xen_sysbus, &error_fatal);
>      object_unref(OBJECT(xendev));
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 25275984bd..0007698ff3 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -578,22 +578,34 @@ static BusState *qbus_find(const char *path, Error **errp)
>      return bus;
>  }
>
> -void qdev_set_id(DeviceState *dev, const char *id)
> +const char *qdev_set_id(DeviceState *dev, const char *id, Error **errp)
>  {
> +    ObjectProperty *prop;
> +
> +    assert(!dev->id && !dev->realized);
> +
> +    /*
> +     * object_property_[try_]add_child() below will assert the device
> +     * has no parent
> +     */
>      if (id) {
> -        dev->id = id;
> -    }
> -
> -    if (dev->id) {
> -        object_property_add_child(qdev_get_peripheral(), dev->id,
> -                                  OBJECT(dev));
> +        prop = object_property_try_add_child(qdev_get_peripheral(), id,
> +                                             OBJECT(dev), NULL);
> +        if (prop) {
> +            dev->id = id;
> +        } else {
> +            error_setg(errp, "Duplicate device ID '%s'", id);
> +            return NULL;
> +        }
>      } else {
>          static int anon_count;
>          gchar *name = g_strdup_printf("device[%d]", anon_count++);
> -        object_property_add_child(qdev_get_peripheral_anon(), name,
> -                                  OBJECT(dev));
> +        prop = object_property_add_child(qdev_get_peripheral_anon(), name,
> +                                         OBJECT(dev));
>          g_free(name);
>      }
> +
> +    return prop->name;
>  }
>
>  DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
> @@ -677,7 +689,13 @@ DeviceState *qdev_device_add(QemuOpts *opts, Error **errp)
>          }
>      }
>
> -    qdev_set_id(dev, qemu_opts_id(opts));
> +    /*
> +     * set dev's parent and register its id.
> +     * If it fails it means the id is already taken.
> +     */
> +    if (!qdev_set_id(dev, qemu_opts_id(opts), errp)) {
> +        goto err_del_dev;
> +    }
>
>      /* set properties */
>      if (qemu_opt_foreach(opts, set_property, dev, errp)) {
> --
> 2.33.0
>
>


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

* Re: [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function
  2021-09-22 16:14 ` [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function Damien Hedde
  2021-09-22 17:42   ` Philippe Mathieu-Daudé
  2021-09-22 17:49   ` David Hildenbrand
@ 2021-10-13  7:12   ` Alistair Francis
  2 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2021-10-13  7:12 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov

On Thu, Sep 23, 2021 at 2:29 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> It allows to try to add a subregion to a memory region with error
> handling. Like memory_region_add_subregion_overlap, it handles
> priority as well.
> Apart the error handling, the behavior is the same. It can be used
> to do the simple memory_region_add_subregion() (with no overlap) by
> setting the priority parameter to 0.
>
> This commit is a preparation to further use this function in the
> context of qmp command which needs error handling support.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>

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

Alistair

> ---
>
> Adding a new function is obviously not ideal. But there is ~900
> occurrences of memory_region_add_subregion[_overlap] calls in the code
> base. We do not really see an alternative here.
> ---
>  include/exec/memory.h | 22 ++++++++++++++++++++++
>  softmmu/memory.c      | 22 ++++++++++++++--------
>  2 files changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c3d417d317..422e1eda67 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2162,6 +2162,28 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                           MemoryRegion *subregion,
>                                           int priority);
>
> +/**
> + * memory_region_try_add_subregion: Add a subregion to a container
> + *                                  with error handling.
> + *
> + * Behaves like memory_region_add_subregion_overlap(), but errors are
> + * reported if the subregion cannot be added.
> + *
> + * @mr: the region to contain the new subregion; must be a container
> + *      initialized with memory_region_init().
> + * @offset: the offset relative to @mr where @subregion is added.
> + * @subregion: the subregion to be added.
> + * @priority: used for resolving overlaps; highest priority wins.
> + * @errp: pointer to Error*, to store an error if it happens.
> + *
> + * Returns: True in case of success, false otherwise.
> + */
> +bool memory_region_try_add_subregion(MemoryRegion *mr,
> +                                     hwaddr offset,
> +                                     MemoryRegion *subregion,
> +                                     int priority,
> +                                     Error **errp);
> +
>  /**
>   * memory_region_get_ram_addr: Get the ram address associated with a memory
>   *                             region
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4d..eac61f8236 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2513,22 +2513,28 @@ done:
>      memory_region_transaction_commit();
>  }
>
> -static void memory_region_add_subregion_common(MemoryRegion *mr,
> -                                               hwaddr offset,
> -                                               MemoryRegion *subregion)
> +bool memory_region_try_add_subregion(MemoryRegion *mr,
> +                                     hwaddr offset,
> +                                     MemoryRegion *subregion,
> +                                     int priority,
> +                                     Error **errp)
>  {
> -    assert(!subregion->container);
> +    if (subregion->container) {
> +        error_setg(errp, "The memory region is already in another region");
> +        return false;
> +    }
> +    subregion->priority = priority;
>      subregion->container = mr;
>      subregion->addr = offset;
>      memory_region_update_container_subregions(subregion);
> +    return true;
>  }
>
>  void memory_region_add_subregion(MemoryRegion *mr,
>                                   hwaddr offset,
>                                   MemoryRegion *subregion)
>  {
> -    subregion->priority = 0;
> -    memory_region_add_subregion_common(mr, offset, subregion);
> +    memory_region_try_add_subregion(mr, offset, subregion, 0, &error_abort);
>  }
>
>  void memory_region_add_subregion_overlap(MemoryRegion *mr,
> @@ -2536,8 +2542,8 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr,
>                                           MemoryRegion *subregion,
>                                           int priority)
>  {
> -    subregion->priority = priority;
> -    memory_region_add_subregion_common(mr, offset, subregion);
> +    memory_region_try_add_subregion(mr, offset, subregion, priority,
> +                                    &error_abort);
>  }
>
>  void memory_region_del_subregion(MemoryRegion *mr,
> --
> 2.33.0
>
>


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

* Re: [RFC PATCH v2 12/16] add x-sysbus-mmio-map qmp command
  2021-09-22 16:14 ` [RFC PATCH v2 12/16] add x-sysbus-mmio-map qmp command Damien Hedde
@ 2021-10-13  7:16   ` Alistair Francis
  0 siblings, 0 replies; 41+ messages in thread
From: Alistair Francis @ 2021-10-13  7:16 UTC (permalink / raw)
  To: Damien Hedde
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov

On Thu, Sep 23, 2021 at 2:26 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> This command allows to map an mmio region of sysbus device onto
> the system memory. Its behavior mimics the sysbus_mmio_map()
> function apart from the automatic unmap (the C function unmaps
> the region if it is already mapped).
> For the qmp function we consider it is an error to try to map
> an already mapped function. If unmapping is required, it is
> probably better to add a sysbus-mmip-unmap function.
>
> This command is still experimental (hence the x prefix), as it
> is related to the sysbus device creation through qmp commands.
>
> In future, we expect to have to handle the overlap/priority
> parameter but also multiple mapping of one mmio. For some
> devices, one mmio is mapped several times at different addresses on
> the bus (which is not supported by sysbus_mmio_map() function and
> requires the use of memory region aliases).

I think as is this is a good start. This is a useful feature!

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

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

> ---
>
> Note: this qmp command is required to be able to build a machine from
> scratch as there is no qmp-way of doing a memory mapping today.
>
> We've added the command into qapi/qdev.json section. It does not seem to
> have any really adequate section yet. Any idea ? should we create for
> example a new one: qapi/sysbus.json or qapi/memory.json ?

I would say leave it in qdev. We don't really want many more sysbus
commands, so qapi/sysbus.json doesn't need it's own file.

Alistair

> ---
>  qapi/qdev.json   | 21 +++++++++++++++++++++
>  hw/core/sysbus.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index ad669ae175..dfc1104197 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -125,3 +125,24 @@
>  ##
>  { 'event': 'DEVICE_DELETED',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @x-sysbus-mmio-map:
> +#
> +# Map a sysbus device mmio onto the main system bus.
> +#
> +# @device: the device's QOM path
> +#
> +# @mmio: The mmio number to be mapped (defaults to 0).
> +#
> +# @addr: The base address for the mapping.
> +#
> +# Since: 6.2
> +#
> +# Returns: Nothing on success
> +#
> +##
> +
> +{ 'command': 'x-sysbus-mmio-map',
> +  'data': {'device': 'str', '*mmio': 'uint8', 'addr': 'uint64'},
> +  'allow-preconfig' : true }
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index aaae8e23cc..b0891f37b6 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -23,6 +23,7 @@
>  #include "hw/sysbus.h"
>  #include "monitor/monitor.h"
>  #include "exec/address-spaces.h"
> +#include "qapi/qapi-commands-qdev.h"
>
>  static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent);
>  static char *sysbus_get_fw_dev_path(DeviceState *dev);
> @@ -154,6 +155,46 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
>      }
>  }
>
> +void qmp_x_sysbus_mmio_map(const char *device, bool has_mmio, uint8_t mmio,
> +                           uint64_t addr, Error **errp)
> +{
> +    Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE, NULL);
> +    SysBusDevice *dev;
> +
> +    if (phase_get() != MACHINE_INIT_PHASE_INITIALIZED) {
> +        error_setg(errp, "The command is permitted only when "
> +                         "the machine is in initialized phase");
> +        return;
> +    }
> +
> +    if (obj == NULL) {
> +        error_setg(errp, "Device '%s' not found", device);
> +        return;
> +    }
> +
> +    dev = SYS_BUS_DEVICE(obj);
> +    if (!has_mmio) {
> +        mmio = 0;
> +    }
> +    if (mmio >= dev->num_mmio) {
> +        error_setg(errp, "MMIO index '%u' is out of range", mmio);
> +        return;
> +    }
> +
> +    if (dev->mmio[mmio].addr != (hwaddr)-1) {
> +        error_setg(errp, "MMIO index '%u' is already mapped", mmio);
> +        return;
> +    }
> +
> +    if (!memory_region_try_add_subregion(get_system_memory(), addr,
> +                                         dev->mmio[mmio].memory, 0,
> +                                         errp)) {
> +        return;
> +    }
> +
> +    dev->mmio[mmio].addr = addr;
> +}
> +
>  void sysbus_mmio_unmap(SysBusDevice *dev, int n)
>  {
>      assert(n >= 0 && n < dev->num_mmio);
> --
> 2.33.0
>
>


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

* Re: [RFC PATCH v2 04/16] softmmu/qdev-monitor: add error handling in qdev_set_id
  2021-10-13  7:10   ` Alistair Francis
@ 2021-10-13 14:30     ` Damien Hedde
  0 siblings, 0 replies; 41+ messages in thread
From: Damien Hedde @ 2021-10-13 14:30 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-devel@nongnu.org Developers, Peter Maydell,
	Michael S. Tsirkin, David Hildenbrand, Peter Xu, mirela.grujic,
	Alistair Francis, Gerd Hoffmann, Ani Sinha, Eric Blake,
	Stefano Stabellini, open list:X86, Paul Durrant,
	Markus Armbruster, Anthony Perard, Marc-André Lureau,
	Eduardo Habkost, Dr. David Alan Gilbert, Eric Auger,
	Paolo Bonzini, open list:RISC-V, Daniel P. Berrangé,
	Mark Burton, Edgar Iglesias, Igor Mammedov, Kevin Wolf



On 10/13/21 09:10, Alistair Francis wrote:
> On Thu, Sep 23, 2021 at 2:29 AM Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> qdev_set_id() is mostly used when the user adds a device (using
>> -device cli option or device_add qmp command). This commit adds
>> an error parameter to handle the case where the given id is
>> already taken.
>>
>> Also document the function and add a return value in order to
>> be able to capture success/failure: the function now returns the
>> id in case of success, or NULL in case of failure.
>>
>> The commit modifies the 2 calling places (qdev-monitor and
>> xen-legacy-backend) to add the error object parameter.
>>
>> Note that the id is, right now, guaranteed to be unique because
>> all ids came from the "device" QemuOptsList where the id is used
>> as key. This addition is a preparation for a future commit which
>> will relax the uniqueness.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> 
> Alistair
> 

CC'ing Kevin who integrated this patch into his series about
"qdev: Add JSON -device"
https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg01826.html

Thanks,
--
Damien


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

end of thread, other threads:[~2021-10-13 14:30 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 16:13 [RFC PATCH v2 00/16] Initial support for machine creation via QMP Damien Hedde
2021-09-22 16:13 ` [RFC PATCH v2 01/16] rename MachineInitPhase enum constants for QAPI compatibility Damien Hedde
2021-09-22 16:13 ` [RFC PATCH v2 02/16] qapi: Implement query-machine-phase QMP command Damien Hedde
2021-09-22 17:37   ` Philippe Mathieu-Daudé
2021-09-23 12:43     ` Damien Hedde
2021-09-23 13:46       ` Ani Sinha
2021-10-12 22:08   ` Alistair Francis
2021-09-22 16:13 ` [RFC PATCH v2 03/16] qapi: Implement x-machine-init " Damien Hedde
2021-10-12 22:19   ` Alistair Francis
2021-09-22 16:13 ` [RFC PATCH v2 04/16] softmmu/qdev-monitor: add error handling in qdev_set_id Damien Hedde
2021-10-13  7:10   ` Alistair Francis
2021-10-13 14:30     ` Damien Hedde
2021-09-22 16:13 ` [RFC PATCH v2 05/16] qdev-monitor: prevent conflicts between qmp/device_add and cli/-device Damien Hedde
2021-09-22 16:13 ` [RFC PATCH v2 06/16] qapi: Allow device_add to execute in machine initialized phase Damien Hedde
2021-10-12 22:25   ` Alistair Francis
2021-09-22 16:13 ` [RFC PATCH v2 07/16] hw/core/machine: add machine_class_is_dynamic_sysbus_dev_allowed Damien Hedde
2021-09-23 10:51   ` Ani Sinha
2021-09-23 13:07     ` Damien Hedde
2021-09-22 16:13 ` [RFC PATCH v2 08/16] qdev-monitor: Check sysbus device type before creating it Damien Hedde
2021-10-12 22:42   ` Alistair Francis
2021-09-22 16:13 ` [RFC PATCH v2 09/16] hw/core/machine: Remove the dynamic sysbus devices type check Damien Hedde
2021-10-12 23:07   ` Alistair Francis
2021-09-22 16:13 ` [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready Damien Hedde
2021-09-23 11:04   ` Ani Sinha
2021-09-23 11:55     ` Ani Sinha
2021-09-23 14:04       ` Damien Hedde
2021-09-22 16:14 ` [RFC PATCH v2 11/16] softmmu/memory: add memory_region_try_add_subregion function Damien Hedde
2021-09-22 17:42   ` Philippe Mathieu-Daudé
2021-09-22 17:49   ` David Hildenbrand
2021-10-13  7:12   ` Alistair Francis
2021-09-22 16:14 ` [RFC PATCH v2 12/16] add x-sysbus-mmio-map qmp command Damien Hedde
2021-10-13  7:16   ` Alistair Francis
2021-09-22 16:14 ` [RFC PATCH v2 13/16] hw/mem/system-memory: add a memory sysbus device Damien Hedde
2021-09-22 16:14 ` [RFC PATCH v2 14/16] docs/system: add doc about the initialized machine phase and an example Damien Hedde
2021-09-22 16:14 ` [RFC PATCH v2 15/16] hw/char/ibex_uart: set user_creatable Damien Hedde
2021-09-22 16:14 ` [RFC PATCH v2 16/16] hw/intc/ibex_plic: " Damien Hedde
2021-09-22 17:50 ` [RFC PATCH v2 00/16] Initial support for machine creation via QMP Philippe Mathieu-Daudé
2021-09-23 11:08   ` Damien Hedde
2021-10-04 15:56 ` Damien Hedde
2021-10-12 22:16 ` Alistair Francis
2021-10-13  5:56   ` Mark Burton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).