[PATCH v4 00/14] Initial support for machine creation via QMP

[PATCH v4 00/14] Initial support for machine creation via QMP

[Damien Hedde]

Hi,

This series adds initial support to build a machine using QMP/QAPI
commands. With this series, one can start from the 'none' machine,
create cpus, sysbus devices, memory map them and wire interrupts.

Sorry for the huge cc list on this cover-letter. Apart from people
who attended the kvm call about this topic, I've cc'ed you only
according to MAINTAINERS file.

The series is divided in 4 parts which are independent of each other,
but we need the 4 parts to be able to use this mechanism:
+ Patches 1 to 6 allow to use the qapi command device_add to cold
  plug devices (like CLI -device do)
+ Patches 7 to 10 modify the 'none' machine which serves as base
  machine.
+ Patches 11 to 13 handle memory mapping and memory creation
+ Patches 14 allows dynamic cold plug of opentitan/sifive_e machine
  to build some example. This last patch is based on a cleanup
  series: it probably works without it, but some config errors are
  not handled (see based-on below).

Only patch 11 is reviewed-by.

v4:
+ cold plugging approach changed in order not to conflict with
  startup. I do not add additional command to handle this so that
  we can change everything easily.
+ device_add in cold plug context is also now equivalent to -device
  CLI regarding -fw_cfg. I also added patches to modify the 'none'
  machine.
+ reworked most of the none machine part
+ updated the sybus-mmio-map command patch

Note that there are still lot of limitations (for example if you try
to create more cpus than the _max_cpus_, tcg will abort()).
Basically all tasks done by machine init reading some parameters are
really tricky: for example, loading complex firmware. But we have to
start by something and all this is not accessible unless the user
asked for none machine and -preconfig.

I can maintain the code introduced here. I'm not sure what's the
process. Is there something else to do than propose a patch to
MAINTAINERS ?
If there is a global agreement on moving on with these feature, it
would be great to have a login on qemu wiki so I can document
limitations and the work being done to solve them.

A simple test can be done with the following scenario which build
a machine subset of the opentitan.

$ cat commands.qmp
// RAM 0x10000000
device_add driver=sysbus-memory id=ram size=0x4000 readonly=false
sysbus-mmio-map device=ram addr=268435456
// CPUS
device_add driver=riscv.hart_array id=cpus cpu-type=lowrisc-ibex-riscv-cpu num-harts=1 resetvec=0x8080
// ROM 0x00008000
device_add driver=sysbus-memory id=rom size=0x4000 readonly=true
sysbus-mmio-map device=rom addr=32768
// PLIC 0x48000000
device_add driver=riscv.sifive.plic id=plic hart-config=M hartid-base=0 num-sources=180 num-priorities=3 priority-base=0x0 pending-base=0x1000 enable-base=0x2000 enable-stride=32 context-base=0x200000 context-stride=8 aperture-size=0x4005000
sysbus-mmio-map device=plic addr=1207959552
qom-set path=plic property=unnamed-gpio-out[1] value=cpus/harts[0]/unnamed-gpio-in[11]
// UART 0x40000000
device_add driver=ibex-uart id=uart chardev=serial0
sysbus-mmio-map device=uart addr=1073741824
qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2]
// FIRMWARE
device_add driver=loader cpu-num=0 file=/path/to/firmware.elf
x-exit-preconfig

$ qemu-system-riscv32 -display none -M none -preconfig -serial stdio -qmp unix:/tmp/qmp-sock,server

In another terminal, you'll need to send the commands with, for example:
$ grep -v '^//' commands.qmp | qmp-shell /tmp/qmp-sock -v

It is the same as running
$ qemu-system-riscv32 -display none -M opentitan -serial stdio -kernel path/to/firmware.elf

If you need a firmware, you can pick this one
https://github.com/GreenSocs/qemu-qmp-machines/blob/master/opentitan-echo.elf
This firmware is just a small interrupt-based bare-metal program
echoing back whatever is sent in the uart.

This repo contains also sifive_e machine example.

Based-on: <20220218164646.132112-1-damien.hedde@greensocs.com>
"RiscV cleanups for user-related life cycles"

Thanks for your comments,
--
Damien

Damien Hedde (13):
  machine: add phase_get() and document phase_check()/advance()
  machine&vl: introduce phase_until() to handle phase transitions
  vl: support machine-initialized target in phase_until()
  qapi/device_add: compute is_hotplug flag
  qapi/device_add: handle the rom_order_override when cold-plugging
  none-machine: add the NoneMachineState structure
  none-machine: add 'ram-addr' property
  none-machine: allow cold plugging sysbus devices
  none-machine: allow several cpus
  softmmu/memory: add memory_region_try_add_subregion function
  add sysbus-mmio-map qapi command
  hw/mem/system-memory: add a memory sysbus device
  hw: set user_creatable on opentitan/sifive_e devices

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

 qapi/qdev.json                 | 34 +++++++++++-
 include/exec/memory.h          | 22 ++++++++
 include/hw/mem/sysbus-memory.h | 28 ++++++++++
 include/hw/qdev-core.h         | 33 ++++++++++++
 hw/char/ibex_uart.c            |  1 +
 hw/char/sifive_uart.c          |  1 +
 hw/core/null-machine.c         | 68 ++++++++++++++++++++++--
 hw/core/qdev.c                 |  5 ++
 hw/core/sysbus.c               | 49 ++++++++++++++++++
 hw/gpio/sifive_gpio.c          |  1 +
 hw/intc/riscv_aclint.c         |  2 +
 hw/intc/sifive_plic.c          |  1 +
 hw/mem/sysbus-memory.c         | 80 +++++++++++++++++++++++++++++
 hw/misc/sifive_e_prci.c        |  8 +++
 hw/misc/unimp.c                |  1 +
 hw/riscv/riscv_hart.c          |  1 +
 hw/timer/ibex_timer.c          |  1 +
 monitor/misc.c                 |  2 +-
 softmmu/memory.c               | 23 ++++++---
 softmmu/qdev-monitor.c         | 20 +++++++-
 softmmu/vl.c                   | 94 ++++++++++++++++++++++++++--------
 hmp-commands.hx                |  1 +
 hw/mem/meson.build             |  2 +
 23 files changed, 439 insertions(+), 39 deletions(-)
 create mode 100644 include/hw/mem/sysbus-memory.h
 create mode 100644 hw/mem/sysbus-memory.c

-- 
2.35.1

[PATCH v4 01/14] machine: add phase_get() and document phase_check()/advance()

[Damien Hedde]

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

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

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

[PATCH v4 02/14] machine&vl: introduce phase_until() to handle phase transitions

[Damien Hedde]

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

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

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

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 include/hw/qdev-core.h | 14 ++++++++
 softmmu/vl.c           | 78 ++++++++++++++++++++++++++++++++----------
 2 files changed, 74 insertions(+), 18 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index e29c705b74..5f73d06408 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -909,4 +909,18 @@ extern bool phase_check(MachineInitPhase phase);
  */
 extern void phase_advance(MachineInitPhase phase);
 
+/**
+ * @phase_until:
+ * @phase: the target phase
+ * @errp: error report
+ *
+ * Make the machine init progress until the target phase is reached.
+ *
+ * Its is a no-op is the target phase is the current or an earlier
+ * phase.
+ *
+ * Returns true in case of success.
+ */
+extern bool phase_until(MachineInitPhase phase, Error **errp);
+
 #endif
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5e1b35ba48..5689d0be88 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2741,30 +2741,72 @@ void qmp_x_exit_preconfig(Error **errp)
         error_setg(errp, "The command is permitted only before machine initialization");
         return;
     }
+    phase_until(PHASE_MACHINE_READY, errp);
+}
 
-    qemu_init_board();
-    qemu_create_cli_devices();
-    qemu_machine_creation_done();
-
-    if (loadvm) {
-        load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
-    }
-    if (replay_mode != REPLAY_MODE_NONE) {
-        replay_vmstate_init();
+bool phase_until(MachineInitPhase phase, Error **errp)
+{
+    if (!phase_check(PHASE_ACCEL_CREATED)) {
+        error_setg(errp, "Phase transition is not supported until accelerator"
+                   " is created");
+        return false;
     }
 
-    if (incoming) {
-        Error *local_err = NULL;
-        if (strcmp(incoming, "defer") != 0) {
-            qmp_migrate_incoming(incoming, &local_err);
-            if (local_err) {
-                error_reportf_err(local_err, "-incoming %s: ", incoming);
-                exit(1);
+    while (!phase_check(phase)) {
+        MachineInitPhase cur_phase = phase_get();
+
+        switch (cur_phase) {
+        case PHASE_ACCEL_CREATED:
+            qemu_init_board();
+            /* We are now in PHASE_MACHINE_INITIALIZED. */
+            qemu_create_cli_devices();
+            /*
+             * At this point all CLI options are handled apart:
+             * + -S (autostart)
+             * + -incoming
+             */
+            qemu_machine_creation_done();
+            /* We are now in PHASE_MACHINE_READY. */
+
+            if (loadvm) {
+                load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
             }
+            if (replay_mode != REPLAY_MODE_NONE) {
+                replay_vmstate_init();
+            }
+
+            if (incoming) {
+                Error *local_err = NULL;
+                if (strcmp(incoming, "defer") != 0) {
+                    qmp_migrate_incoming(incoming, &local_err);
+                    if (local_err) {
+                        error_reportf_err(local_err, "-incoming %s: ",
+                                          incoming);
+                        exit(1);
+                    }
+                }
+            } else if (autostart) {
+                qmp_cont(NULL);
+            }
+            break;
+
+        default:
+            /*
+             * If we end up here, it is because we miss a case above.
+             */
+            error_setg(&error_abort, "Requested phase transition is not"
+                       " implemented");
+            return false;
         }
-    } else if (autostart) {
-        qmp_cont(NULL);
+
+        /*
+         * Ensure we made some progress.
+         * With the default case above, it should be enough to prevent
+         * any infinite loop.
+         */
+        assert(cur_phase < phase_get());
     }
+    return true;
 }
 
 void qemu_init(int argc, char **argv, char **envp)
-- 
2.35.1

[PATCH v4 03/14] vl: support machine-initialized target in phase_until()

[Damien Hedde]

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

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

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

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 softmmu/vl.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5689d0be88..50337d68b9 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2737,8 +2737,8 @@ static void qemu_machine_creation_done(void)
 
 void qmp_x_exit_preconfig(Error **errp)
 {
-    if (phase_check(PHASE_MACHINE_INITIALIZED)) {
-        error_setg(errp, "The command is permitted only before machine initialization");
+    if (phase_check(PHASE_MACHINE_READY)) {
+        error_setg(errp, "The command is permitted only before machine is ready");
         return;
     }
     phase_until(PHASE_MACHINE_READY, errp);
@@ -2759,7 +2759,17 @@ bool phase_until(MachineInitPhase phase, Error **errp)
         case PHASE_ACCEL_CREATED:
             qemu_init_board();
             /* We are now in PHASE_MACHINE_INITIALIZED. */
+            /*
+             * Handle CLI devices now in order leave the function in a state
+             * where we can cold plug devices with QMP. The following call
+             * handles the CLI options:
+             * + -fw_cfg (has side effects on device cold plug)
+             * + -device
+             */
             qemu_create_cli_devices();
+            break;
+
+        case PHASE_MACHINE_INITIALIZED:
             /*
              * At this point all CLI options are handled apart:
              * + -S (autostart)
-- 
2.35.1

[PATCH v4 04/14] qapi/device_add: compute is_hotplug flag

[Damien Hedde]

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

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

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db5..47a89aee20 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -617,6 +617,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     char *id;
     DeviceState *dev = NULL;
     BusState *bus = NULL;
+    bool is_hotplug = phase_check(PHASE_MACHINE_READY);
 
     driver = qdict_get_try_str(opts, "driver");
     if (!driver) {
@@ -660,7 +661,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
         return NULL;
     }
 
-    if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus)) {
+    if (is_hotplug && bus && !qbus_is_hotpluggable(bus)) {
         error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
         return NULL;
     }
@@ -674,7 +675,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     dev = qdev_new(driver);
 
     /* Check whether the hotplug is allowed by the machine */
-    if (phase_check(PHASE_MACHINE_READY)) {
+    if (is_hotplug) {
         if (!qdev_hotplug_allowed(dev, errp)) {
             goto err_del_dev;
         }
-- 
2.35.1

[PATCH v4 05/14] qapi/device_add: handle the rom_order_override when cold-plugging

[Damien Hedde]

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

Unfortunately this won't work with qapi commands.

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

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

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

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 47a89aee20..9ec3e0ebff 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -43,6 +43,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/clock.h"
 #include "hw/boards.h"
+#include "hw/loader.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -671,6 +672,10 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
         return NULL;
     }
 
+    if (!is_hotplug) {
+        rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
+    }
+
     /* create device */
     dev = qdev_new(driver);
 
@@ -712,6 +717,9 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
     if (!qdev_realize(DEVICE(dev), bus, errp)) {
         goto err_del_dev;
     }
+    if (!is_hotplug) {
+        rom_reset_order_override();
+    }
     return dev;
 
 err_del_dev:
@@ -719,6 +727,9 @@ err_del_dev:
         object_unparent(OBJECT(dev));
         object_unref(OBJECT(dev));
     }
+    if (!is_hotplug) {
+        rom_reset_order_override();
+    }
     return NULL;
 }
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 50337d68b9..b91ae1b8ae 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2680,7 +2680,6 @@ static void qemu_create_cli_devices(void)
     }
 
     /* init generic devices */
-    rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
     qemu_opts_foreach(qemu_find_opts("device"),
                       device_init_func, NULL, &error_fatal);
     QTAILQ_FOREACH(opt, &device_opts, next) {
@@ -2697,7 +2696,6 @@ static void qemu_create_cli_devices(void)
         object_unref(OBJECT(dev));
         loc_pop(&opt->loc);
     }
-    rom_reset_order_override();
 }
 
 static void qemu_machine_creation_done(void)
-- 
2.35.1

[PATCH v4 06/14] qapi/device_add: Allow execution in machine initialized phase

[Damien Hedde]

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

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

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

Signed-off-by: Mirela Grujic <mirela.grujic@greensocs.com>
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---

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

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 26cd10106b..2e2de41499 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -77,7 +77,8 @@
 { 'command': 'device_add',
   'data': {'driver': 'str', '*bus': 'str', '*id': 'str'},
   'gen': false, # so we can get the additional arguments
-  'features': ['json-cli', 'json-cli-hotplug'] }
+  'features': ['json-cli', 'json-cli-hotplug'],
+  'allow-preconfig': true }
 
 ##
 # @device_del:
diff --git a/monitor/misc.c b/monitor/misc.c
index a3a6e47844..fd56dd8f08 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -233,7 +233,7 @@ static void monitor_init_qmp_commands(void)
     qmp_init_marshal(&qmp_commands);
 
     qmp_register_command(&qmp_commands, "device_add",
-                         qmp_device_add, 0, 0);
+                         qmp_device_add, QCO_ALLOW_PRECONFIG, 0);
 
     QTAILQ_INIT(&qmp_cap_negotiation_commands);
     qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 9ec3e0ebff..ec0eddaf72 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -853,6 +853,10 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
     QemuOpts *opts;
     DeviceState *dev;
 
+    if (!phase_until(PHASE_MACHINE_INITIALIZED, errp)) {
+        return;
+    }
+
     opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, errp);
     if (!opts) {
         return;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 70a9136ac2..3237085e22 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -668,6 +668,7 @@ ERST
         .help       = "add device, like -device on the command line",
         .cmd        = hmp_device_add,
         .command_completion = device_add_completion,
+        .flags      = "p",
     },
 
 SRST
-- 
2.35.1

[PATCH v4 07/14] none-machine: add the NoneMachineState structure

[Damien Hedde]

The none machine was using the parent state structure.
We'll need a custom state to add a field in the following commit.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/null-machine.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
index f586a4bef5..7eb258af07 100644
--- a/hw/core/null-machine.c
+++ b/hw/core/null-machine.c
@@ -17,6 +17,13 @@
 #include "exec/address-spaces.h"
 #include "hw/core/cpu.h"
 
+struct NoneMachineState {
+    MachineState parent;
+};
+
+#define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none")
+OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState, NONE_MACHINE)
+
 static void machine_none_init(MachineState *mch)
 {
     CPUState *cpu = NULL;
@@ -42,8 +49,10 @@ static void machine_none_init(MachineState *mch)
     }
 }
 
-static void machine_none_machine_init(MachineClass *mc)
+static void machine_none_class_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
+
     mc->desc = "empty machine";
     mc->init = machine_none_init;
     mc->max_cpus = 1;
@@ -56,4 +65,15 @@ static void machine_none_machine_init(MachineClass *mc)
     mc->no_sdcard = 1;
 }
 
-DEFINE_MACHINE("none", machine_none_machine_init)
+static const TypeInfo none_machine_info = {
+    .name          = TYPE_NONE_MACHINE,
+    .parent        = TYPE_MACHINE,
+    .instance_size = sizeof(NoneMachineState),
+    .class_init    = machine_none_class_init,
+};
+
+static void none_machine_register_types(void)
+{
+    type_register_static(&none_machine_info);
+}
+type_init(none_machine_register_types);
-- 
2.35.1

[PATCH v4 08/14] none-machine: add 'ram-addr' property

[Damien Hedde]

Add the property to configure a the base address of the ram.
The default value remains zero.

This commit is needed to use the 'none' machine as a base, and
subsequently to dynamically populate it using qapi commands. Having
a non null 'ram' is really hard to workaround because of the actual
constraints on the generic loader: it prevents loading binaries
bigger than ram_size (with a null ram, we cannot load anything).
For now we need to be able to use the existing ram creation
feature of the none machine with a configurable base address.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/null-machine.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
index 7eb258af07..5fd1cc0218 100644
--- a/hw/core/null-machine.c
+++ b/hw/core/null-machine.c
@@ -16,9 +16,11 @@
 #include "hw/boards.h"
 #include "exec/address-spaces.h"
 #include "hw/core/cpu.h"
+#include "qapi/visitor.h"
 
 struct NoneMachineState {
     MachineState parent;
+    uint64_t ram_addr;
 };
 
 #define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none")
@@ -26,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState, NONE_MACHINE)
 
 static void machine_none_init(MachineState *mch)
 {
+    NoneMachineState *nms = NONE_MACHINE(mch);
     CPUState *cpu = NULL;
 
     /* Initialize CPU (if user asked for it) */
@@ -37,9 +40,13 @@ static void machine_none_init(MachineState *mch)
         }
     }
 
-    /* RAM at address zero */
+    /* RAM at configured address (default: 0) */
     if (mch->ram) {
-        memory_region_add_subregion(get_system_memory(), 0, mch->ram);
+        memory_region_add_subregion(get_system_memory(), nms->ram_addr,
+                                    mch->ram);
+    } else if (nms->ram_addr) {
+        error_report("'ram-addr' has been specified but the size is zero");
+        exit(1);
     }
 
     if (mch->kernel_filename) {
@@ -49,6 +56,22 @@ static void machine_none_init(MachineState *mch)
     }
 }
 
+static void machine_none_get_ram_addr(Object *obj, Visitor *v, const char *name,
+                                      void *opaque, Error **errp)
+{
+    NoneMachineState *nms = NONE_MACHINE(obj);
+
+    visit_type_uint64(v, name, &nms->ram_addr, errp);
+}
+
+static void machine_none_set_ram_addr(Object *obj, Visitor *v, const char *name,
+                                      void *opaque, Error **errp)
+{
+    NoneMachineState *nms = NONE_MACHINE(obj);
+
+    visit_type_uint64(v, name, &nms->ram_addr, errp);
+}
+
 static void machine_none_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -63,6 +86,13 @@ static void machine_none_class_init(ObjectClass *oc, void *data)
     mc->no_floppy = 1;
     mc->no_cdrom = 1;
     mc->no_sdcard = 1;
+
+    object_class_property_add(oc, "ram-addr", "int",
+        machine_none_get_ram_addr,
+        machine_none_set_ram_addr,
+        NULL, NULL);
+    object_class_property_set_description(oc, "ram-addr",
+        "Base address of the RAM (default is 0)");
 }
 
 static const TypeInfo none_machine_info = {
-- 
2.35.1

[PATCH v4 09/14] none-machine: allow cold plugging sysbus devices

[Damien Hedde]

Allow plugging any sysbus device on this machine (the sysbus
devices still need to be 'user-creatable').

This commit is needed to use the 'none' machine as a base, and
subsequently to dynamically populate it with sysbus devices using
qapi commands.

Note that this only concern cold-plug: sysbus devices cann't be hot
plugged because the sysbus bus does not support it.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/null-machine.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
index 5fd1cc0218..702b56232d 100644
--- a/hw/core/null-machine.c
+++ b/hw/core/null-machine.c
@@ -17,6 +17,7 @@
 #include "exec/address-spaces.h"
 #include "hw/core/cpu.h"
 #include "qapi/visitor.h"
+#include "hw/sysbus.h"
 
 struct NoneMachineState {
     MachineState parent;
@@ -93,6 +94,9 @@ static void machine_none_class_init(ObjectClass *oc, void *data)
         NULL, NULL);
     object_class_property_set_description(oc, "ram-addr",
         "Base address of the RAM (default is 0)");
+
+    /* allow cold plugging any any "user-creatable" sysbus device */
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_SYS_BUS_DEVICE);
 }
 
 static const TypeInfo none_machine_info = {
-- 
2.35.1

[PATCH v4 10/14] none-machine: allow several cpus

[Damien Hedde]

In order to dynamically create (cold-plugging) cpus, we need to
increase _maxcpus_. This value is indeed used to initialize the
accelerator during qemu startup.
Allowing 16 seems a good starting point.

Add a check to prevent a user to request more than 1 cpu if he
specifies the cpu type on the CLI so that the legacy use case
of the none machine is preserved.

This commit is needed to use the 'none' machine as a base, and
subsequently to dynamically populate it wth cpus usign qapi
commands.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 hw/core/null-machine.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
index 702b56232d..64f9931857 100644
--- a/hw/core/null-machine.c
+++ b/hw/core/null-machine.c
@@ -34,6 +34,10 @@ static void machine_none_init(MachineState *mch)
 
     /* Initialize CPU (if user asked for it) */
     if (mch->cpu_type) {
+        if (mch->smp.cpus > 1) {
+            error_report("Cannot initialize more than 1 CPU");
+            exit(1);
+        }
         cpu = cpu_create(mch->cpu_type);
         if (!cpu) {
             error_report("Unable to initialize CPU");
@@ -79,7 +83,7 @@ static void machine_none_class_init(ObjectClass *oc, void *data)
 
     mc->desc = "empty machine";
     mc->init = machine_none_init;
-    mc->max_cpus = 1;
+    mc->max_cpus = 16;
     mc->default_ram_size = 0;
     mc->default_ram_id = "ram";
     mc->no_serial = 1;
-- 
2.35.1

[PATCH v4 11/14] softmmu/memory: add memory_region_try_add_subregion function

[Damien Hedde]

It allows adding a subregion to a memory region with error handling.
Like memory_region_add_subregion_overlap(), it handles priority as
well. Apart from 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 of this function in the
context of qapi command which needs error handling support.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 include/exec/memory.h | 22 ++++++++++++++++++++++
 softmmu/memory.c      | 23 +++++++++++++++--------
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 4d5997e6bb..070dcb5255 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2215,6 +2215,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 678dc62f06..6bc76bf6da 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2541,27 +2541,34 @@ 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)
 {
     MemoryRegion *alias;
 
-    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;
     for (alias = subregion->alias; alias; alias = alias->alias) {
         alias->mapped_via_alias++;
     }
     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,
@@ -2569,8 +2576,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.35.1

[PATCH v4 12/14] add sysbus-mmio-map qapi command

[Damien Hedde]

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 qapi 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 command.

This command is still experimental (hence the 'unstable' feature),
as it is related to the sysbus device creation through qapi commands.

This command is required to be able to dynamically build a machine
from scratch as there is no qapi-way of doing a memory mapping.

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

v4:
 + integrate priority parameter
 + use 'unstable' feature flag instead of 'x-' prefix
 + bump version to 7.0
 + dropped Alistair's reviewed-by as a consequence
---
 qapi/qdev.json   | 31 ++++++++++++++++++++++++++++++
 hw/core/sysbus.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 2e2de41499..4830e87a90 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -160,3 +160,34 @@
 ##
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @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.
+#
+# @priority: The priority of the mapping (defaults to 0).
+#
+# Features:
+# @unstable: Command is meant to map sysbus devices
+#            while in preconfig mode.
+#
+# Since: 7.0
+#
+# Returns: Nothing on success
+#
+##
+
+{ 'command': 'sysbus-mmio-map',
+  'data': { 'device': 'str',
+            '*mmio': 'uint8',
+            'addr': 'uint64',
+            '*priority': 'int32' },
+  'features': ['unstable'],
+  'allow-preconfig' : true }
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 05c1da3d31..df1f1f43a5 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,54 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
     }
 }
 
+void qmp_sysbus_mmio_map(const char *device,
+                         bool has_mmio, uint8_t mmio,
+                         uint64_t addr,
+                         bool has_priority, int32_t priority,
+                         Error **errp)
+{
+    Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE, NULL);
+    SysBusDevice *dev;
+
+    if (phase_get() != PHASE_MACHINE_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 (!has_priority) {
+        priority = 0;
+    }
+
+    if (mmio >= dev->num_mmio) {
+        error_setg(errp, "MMIO index '%u' does not exist in '%s'",
+                   mmio, device);
+        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, priority,
+                                         errp)) {
+        return;
+    }
+
+    dev->mmio[mmio].addr = addr;
+}
+
 void sysbus_mmio_unmap(SysBusDevice *dev, int n)
 {
     assert(n >= 0 && n < dev->num_mmio);
-- 
2.35.1

[PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Damien Hedde]

This device can be used to create a memory wrapped into a
sysbus device.
This device has one property 'readonly' which allows
to choose between a ram or a rom.

The purpose for this device is to be used with qapi command
device_add.

Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
 include/hw/mem/sysbus-memory.h | 28 ++++++++++++
 hw/mem/sysbus-memory.c         | 80 ++++++++++++++++++++++++++++++++++
 hw/mem/meson.build             |  2 +
 3 files changed, 110 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..5c596f8b4f
--- /dev/null
+++ b/include/hw/mem/sysbus-memory.h
@@ -0,0 +1,28 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * SysBusDevice Memory
+ *
+ * Copyright (c) 2021 Greensocs
+ */
+
+#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..f1ad7ba7ec
--- /dev/null
+++ b/hw/mem/sysbus-memory.c
@@ -0,0 +1,80 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * SysBusDevice Memory
+ *
+ * Copyright (c) 2021 Greensocs
+ */
+
+#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) {
+        return;
+    }
+
+    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 82f86d117e..04c74e12f2 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_SPARSE_MEM', if_true: files('sparse-mem.c'))
+
+softmmu_ss.add(files('sysbus-memory.c'))
-- 
2.35.1

[PATCH v4 14/14] hw: set user_creatable on opentitan/sifive_e devices

[Damien Hedde]

The devices are:
+ ibex-timer
+ ibex-uart
+ riscv.aclint.swi
+ riscv.aclint.mtimer
+ riscv.hart_array
+ riscv.sifive.e.prci
+ riscv.sifive.plic
+ riscv.sifive.uart
+ sifive_soc.gpio
+ unimplemented-device

These devices are clean regarding error handling in realize.

They are all sysbus devices, so setting user-creatable will only
enable cold-plugging them on machine having explicitely allowed them
(only _none_ machine does that).

Note that this commit include the ricv_array which embeds cpus. There
are some deep internal constraints about them: you cannot create more
cpus than the machine's maxcpus. TCG accelerator's code will for example
assert if a user try to create too many cpus.

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

I can also split this patch if you think it's better.
But it is mostly a one-line fix per file.

This patch requires first some cleanups in order to fix error errors
and some more memory leaks that could happend in legit user-related
life cycles: a miss-configuration should not be a fatal error anymore.
https://lore.kernel.org/qemu-devel/20220218164646.132112-1-damien.hedde@greensocs.com
---
 hw/char/ibex_uart.c     | 1 +
 hw/char/sifive_uart.c   | 1 +
 hw/gpio/sifive_gpio.c   | 1 +
 hw/intc/riscv_aclint.c  | 2 ++
 hw/intc/sifive_plic.c   | 1 +
 hw/misc/sifive_e_prci.c | 8 ++++++++
 hw/misc/unimp.c         | 1 +
 hw/riscv/riscv_hart.c   | 1 +
 hw/timer/ibex_timer.c   | 1 +
 9 files changed, 17 insertions(+)

diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
index e58181fcf4..0b6d45f2e7 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;
diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
index 1c75f792b3..6e88778c03 100644
--- a/hw/char/sifive_uart.c
+++ b/hw/char/sifive_uart.c
@@ -243,6 +243,7 @@ static void sifive_uart_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     ResettableClass *rc = RESETTABLE_CLASS(oc);
 
+    dc->user_creatable = true;
     dc->realize = sifive_uart_realize;
     dc->vmsd = &vmstate_sifive_uart;
     rc->phases.enter = sifive_uart_reset_enter;
diff --git a/hw/gpio/sifive_gpio.c b/hw/gpio/sifive_gpio.c
index 78bf29e996..8443befa20 100644
--- a/hw/gpio/sifive_gpio.c
+++ b/hw/gpio/sifive_gpio.c
@@ -380,6 +380,7 @@ static void sifive_gpio_class_init(ObjectClass *klass, void *data)
     dc->realize = sifive_gpio_realize;
     dc->reset = sifive_gpio_reset;
     dc->desc = "SiFive GPIO";
+    dc->user_creatable = true;
 }
 
 static const TypeInfo sifive_gpio_info = {
diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
index bef2e1988b..fbf63a52b7 100644
--- a/hw/intc/riscv_aclint.c
+++ b/hw/intc/riscv_aclint.c
@@ -283,6 +283,7 @@ static void riscv_aclint_mtimer_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     dc->realize = riscv_aclint_mtimer_realize;
     device_class_set_props(dc, riscv_aclint_mtimer_properties);
+    dc->user_creatable = true;
 }
 
 static const TypeInfo riscv_aclint_mtimer_info = {
@@ -466,6 +467,7 @@ static void riscv_aclint_swi_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     dc->realize = riscv_aclint_swi_realize;
     device_class_set_props(dc, riscv_aclint_swi_properties);
+    dc->user_creatable = true;
 }
 
 static const TypeInfo riscv_aclint_swi_info = {
diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index 8692ea6725..4e5831b487 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -434,6 +434,7 @@ static void sifive_plic_class_init(ObjectClass *klass, void *data)
 
     dc->reset = sifive_plic_reset;
     device_class_set_props(dc, sifive_plic_properties);
+    dc->user_creatable = true;
     dc->realize = sifive_plic_realize;
     dc->vmsd = &vmstate_sifive_plic;
 }
diff --git a/hw/misc/sifive_e_prci.c b/hw/misc/sifive_e_prci.c
index a8702c6a5d..7341823e43 100644
--- a/hw/misc/sifive_e_prci.c
+++ b/hw/misc/sifive_e_prci.c
@@ -97,11 +97,19 @@ static void sifive_e_prci_init(Object *obj)
     s->plloutdiv = SIFIVE_E_PRCI_PLLOUTDIV_DIV1;
 }
 
+static void sifive_e_prci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->user_creatable = true;
+}
+
 static const TypeInfo sifive_e_prci_info = {
     .name          = TYPE_SIFIVE_E_PRCI,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(SiFiveEPRCIState),
     .instance_init = sifive_e_prci_init,
+    .class_init    = sifive_e_prci_class_init,
 };
 
 static void sifive_e_prci_register_types(void)
diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c
index 6cfc5727f0..fb2a0b23dd 100644
--- a/hw/misc/unimp.c
+++ b/hw/misc/unimp.c
@@ -80,6 +80,7 @@ static void unimp_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->user_creatable = true;
     dc->realize = unimp_realize;
     device_class_set_props(dc, unimp_properties);
 }
diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index 4aed6c2a59..85fae44048 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -77,6 +77,7 @@ static void riscv_harts_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->user_creatable = true;
     device_class_set_props(dc, riscv_harts_props);
     dc->realize = riscv_harts_realize;
 }
diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c
index 8c2ca364da..d1cc337416 100644
--- a/hw/timer/ibex_timer.c
+++ b/hw/timer/ibex_timer.c
@@ -295,6 +295,7 @@ static void ibex_timer_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->user_creatable = true;
     dc->reset = ibex_timer_reset;
     dc->vmsd = &vmstate_ibex_timer;
     dc->realize = ibex_timer_realize;
-- 
2.35.1

[PATCH v4 14/14] hw: set user_creatable on opentitan/sifive_e devices

[Damien Hedde]

The devices are:
+ ibex-timer
+ ibex-uart
+ riscv.aclint.swi
+ riscv.aclint.mtimer
+ riscv.hart_array
+ riscv.sifive.e.prci
+ riscv.sifive.plic
+ riscv.sifive.uart
+ sifive_soc.gpio
+ unimplemented-device

These devices are clean regarding error handling in realize.

They are all sysbus devices, so setting user-creatable will only
enable cold-plugging them on machine having explicitely allowed them
(only _none_ machine does that).

Note that this commit include the ricv_array which embeds cpus. There
are some deep internal constraints about them: you cannot create more
cpus than the machine's maxcpus. TCG accelerator's code will for example
assert if a user try to create too many cpus.

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

I can also split this patch if you think it's better.
But it is mostly a one-line fix per file.

This patch requires first some cleanups in order to fix error errors
and some more memory leaks that could happend in legit user-related
life cycles: a miss-configuration should not be a fatal error anymore.
https://lore.kernel.org/qemu-devel/20220218164646.132112-1-damien.hedde@greensocs.com
---
 hw/char/ibex_uart.c     | 1 +
 hw/char/sifive_uart.c   | 1 +
 hw/gpio/sifive_gpio.c   | 1 +
 hw/intc/riscv_aclint.c  | 2 ++
 hw/intc/sifive_plic.c   | 1 +
 hw/misc/sifive_e_prci.c | 8 ++++++++
 hw/misc/unimp.c         | 1 +
 hw/riscv/riscv_hart.c   | 1 +
 hw/timer/ibex_timer.c   | 1 +
 9 files changed, 17 insertions(+)

diff --git a/hw/char/ibex_uart.c b/hw/char/ibex_uart.c
index e58181fcf4..0b6d45f2e7 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;
diff --git a/hw/char/sifive_uart.c b/hw/char/sifive_uart.c
index 1c75f792b3..6e88778c03 100644
--- a/hw/char/sifive_uart.c
+++ b/hw/char/sifive_uart.c
@@ -243,6 +243,7 @@ static void sifive_uart_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     ResettableClass *rc = RESETTABLE_CLASS(oc);
 
+    dc->user_creatable = true;
     dc->realize = sifive_uart_realize;
     dc->vmsd = &vmstate_sifive_uart;
     rc->phases.enter = sifive_uart_reset_enter;
diff --git a/hw/gpio/sifive_gpio.c b/hw/gpio/sifive_gpio.c
index 78bf29e996..8443befa20 100644
--- a/hw/gpio/sifive_gpio.c
+++ b/hw/gpio/sifive_gpio.c
@@ -380,6 +380,7 @@ static void sifive_gpio_class_init(ObjectClass *klass, void *data)
     dc->realize = sifive_gpio_realize;
     dc->reset = sifive_gpio_reset;
     dc->desc = "SiFive GPIO";
+    dc->user_creatable = true;
 }
 
 static const TypeInfo sifive_gpio_info = {
diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
index bef2e1988b..fbf63a52b7 100644
--- a/hw/intc/riscv_aclint.c
+++ b/hw/intc/riscv_aclint.c
@@ -283,6 +283,7 @@ static void riscv_aclint_mtimer_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     dc->realize = riscv_aclint_mtimer_realize;
     device_class_set_props(dc, riscv_aclint_mtimer_properties);
+    dc->user_creatable = true;
 }
 
 static const TypeInfo riscv_aclint_mtimer_info = {
@@ -466,6 +467,7 @@ static void riscv_aclint_swi_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     dc->realize = riscv_aclint_swi_realize;
     device_class_set_props(dc, riscv_aclint_swi_properties);
+    dc->user_creatable = true;
 }
 
 static const TypeInfo riscv_aclint_swi_info = {
diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index 8692ea6725..4e5831b487 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -434,6 +434,7 @@ static void sifive_plic_class_init(ObjectClass *klass, void *data)
 
     dc->reset = sifive_plic_reset;
     device_class_set_props(dc, sifive_plic_properties);
+    dc->user_creatable = true;
     dc->realize = sifive_plic_realize;
     dc->vmsd = &vmstate_sifive_plic;
 }
diff --git a/hw/misc/sifive_e_prci.c b/hw/misc/sifive_e_prci.c
index a8702c6a5d..7341823e43 100644
--- a/hw/misc/sifive_e_prci.c
+++ b/hw/misc/sifive_e_prci.c
@@ -97,11 +97,19 @@ static void sifive_e_prci_init(Object *obj)
     s->plloutdiv = SIFIVE_E_PRCI_PLLOUTDIV_DIV1;
 }
 
+static void sifive_e_prci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->user_creatable = true;
+}
+
 static const TypeInfo sifive_e_prci_info = {
     .name          = TYPE_SIFIVE_E_PRCI,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(SiFiveEPRCIState),
     .instance_init = sifive_e_prci_init,
+    .class_init    = sifive_e_prci_class_init,
 };
 
 static void sifive_e_prci_register_types(void)
diff --git a/hw/misc/unimp.c b/hw/misc/unimp.c
index 6cfc5727f0..fb2a0b23dd 100644
--- a/hw/misc/unimp.c
+++ b/hw/misc/unimp.c
@@ -80,6 +80,7 @@ static void unimp_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->user_creatable = true;
     dc->realize = unimp_realize;
     device_class_set_props(dc, unimp_properties);
 }
diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index 4aed6c2a59..85fae44048 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -77,6 +77,7 @@ static void riscv_harts_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->user_creatable = true;
     device_class_set_props(dc, riscv_harts_props);
     dc->realize = riscv_harts_realize;
 }
diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c
index 8c2ca364da..d1cc337416 100644
--- a/hw/timer/ibex_timer.c
+++ b/hw/timer/ibex_timer.c
@@ -295,6 +295,7 @@ static void ibex_timer_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->user_creatable = true;
     dc->reset = ibex_timer_reset;
     dc->vmsd = &vmstate_ibex_timer;
     dc->realize = ibex_timer_realize;
-- 
2.35.1

Re: [PATCH v4 11/14] softmmu/memory: add memory_region_try_add_subregion function

[Damien Hedde]

Hi Philippe,

I suppose it is ok if I change your mail in the reviewed by ?

Thanks,
Damien

On 2/23/22 10:07, Damien Hedde wrote:
> It allows adding a subregion to a memory region with error handling.
> Like memory_region_add_subregion_overlap(), it handles priority as
> well. Apart from 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 of this function in the
> context of qapi command which needs error handling support.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>   include/exec/memory.h | 22 ++++++++++++++++++++++
>   softmmu/memory.c      | 23 +++++++++++++++--------
>   2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 4d5997e6bb..070dcb5255 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2215,6 +2215,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 678dc62f06..6bc76bf6da 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -2541,27 +2541,34 @@ 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)
>   {
>       MemoryRegion *alias;
>   
> -    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;
>       for (alias = subregion->alias; alias; alias = alias->alias) {
>           alias->mapped_via_alias++;
>       }
>       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,
> @@ -2569,8 +2576,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,

Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Igor Mammedov]

On Wed, 23 Feb 2022 10:07:05 +0100
Damien Hedde <damien.hedde@greensocs.com> wrote:

> This device can be used to create a memory wrapped into a
> sysbus device.
> This device has one property 'readonly' which allows
> to choose between a ram or a rom.
> 

> The purpose for this device is to be used with qapi command
> device_add.
that's the way to add a device to QEMU but a don't actual
purpose described here, i.e. how board will use this
device/actual usecase and how it will be wired to board
and why it does have to be a sysbus device.

> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  include/hw/mem/sysbus-memory.h | 28 ++++++++++++
>  hw/mem/sysbus-memory.c         | 80 ++++++++++++++++++++++++++++++++++
>  hw/mem/meson.build             |  2 +
>  3 files changed, 110 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..5c596f8b4f
> --- /dev/null
> +++ b/include/hw/mem/sysbus-memory.h
> @@ -0,0 +1,28 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SysBusDevice Memory
> + *
> + * Copyright (c) 2021 Greensocs
> + */
> +
> +#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..f1ad7ba7ec
> --- /dev/null
> +++ b/hw/mem/sysbus-memory.c
> @@ -0,0 +1,80 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SysBusDevice Memory
> + *
> + * Copyright (c) 2021 Greensocs
> + */
> +
> +#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) {
> +        return;
> +    }
> +
> +    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 82f86d117e..04c74e12f2 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_SPARSE_MEM', if_true: files('sparse-mem.c'))
> +
> +softmmu_ss.add(files('sysbus-memory.c'))

Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Damien Hedde]

On 2/23/22 10:44, Igor Mammedov wrote:
> On Wed, 23 Feb 2022 10:07:05 +0100
> Damien Hedde <damien.hedde@greensocs.com> wrote:
> 
>> This device can be used to create a memory wrapped into a
>> sysbus device.
>> This device has one property 'readonly' which allows
>> to choose between a ram or a rom.
>>
> 
>> The purpose for this device is to be used with qapi command
>> device_add.
> that's the way to add a device to QEMU but a don't actual
> purpose described here, i.e. how board will use this
> device/actual usecase and how it will be wired to board
> and why it does have to be a sysbus device.
> 
Sorry, this was unclear.

It is a sysbus device in order to use it like any other sysbus device. 
The memory region it contains is exposed as a sysbus mmio.

I can replace the commit message by the following paragraph:

Boards instantiate memories by creating memory region objects which is 
not possible using QAPI commands.
To create a memory, the user can instantiate and map this device by 
issuing the following commands:
device_add driver=sysbus-memory size=0x1000 id=someram
sysbus-mmio-map device=someram addr=0

>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>   include/hw/mem/sysbus-memory.h | 28 ++++++++++++
>>   hw/mem/sysbus-memory.c         | 80 ++++++++++++++++++++++++++++++++++
>>   hw/mem/meson.build             |  2 +
>>   3 files changed, 110 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..5c596f8b4f
>> --- /dev/null
>> +++ b/include/hw/mem/sysbus-memory.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * SysBusDevice Memory
>> + *
>> + * Copyright (c) 2021 Greensocs
>> + */
>> +
>> +#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..f1ad7ba7ec
>> --- /dev/null
>> +++ b/hw/mem/sysbus-memory.c
>> @@ -0,0 +1,80 @@
>> +/*
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + *
>> + * SysBusDevice Memory
>> + *
>> + * Copyright (c) 2021 Greensocs
>> + */
>> +
>> +#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) {
>> +        return;
>> +    }
>> +
>> +    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 82f86d117e..04c74e12f2 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_SPARSE_MEM', if_true: files('sparse-mem.c'))
>> +
>> +softmmu_ss.add(files('sysbus-memory.c'))
> 

Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Igor Mammedov]

On Wed, 23 Feb 2022 11:19:49 +0100
Damien Hedde <damien.hedde@greensocs.com> wrote:

> On 2/23/22 10:44, Igor Mammedov wrote:
> > On Wed, 23 Feb 2022 10:07:05 +0100
> > Damien Hedde <damien.hedde@greensocs.com> wrote:
> >   
> >> This device can be used to create a memory wrapped into a
> >> sysbus device.
> >> This device has one property 'readonly' which allows
> >> to choose between a ram or a rom.
> >>  
> >   
> >> The purpose for this device is to be used with qapi command
> >> device_add.  
> > that's the way to add a device to QEMU but a don't actual
> > purpose described here, i.e. how board will use this
> > device/actual usecase and how it will be wired to board
> > and why it does have to be a sysbus device.
> >   
> Sorry, this was unclear.
> 
> It is a sysbus device in order to use it like any other sysbus device. 
> The memory region it contains is exposed as a sysbus mmio.

aside of that sysbus is legacy fictional bus (albeit widely used),
it doesn't scale to non sysbus devices (me thinking about buss-less
pc-dimm & co) since eventually we would like to create mainstream
machine types via QMP as well.

> I can replace the commit message by the following paragraph:
> 
> Boards instantiate memories by creating memory region objects which is 
> not possible using QAPI commands.

That's not entirely true, you can use object-add with hostmem backends
which do provide a means to allocate memory_region.
(there is no rom hostmem backend probably (i.e. one that return rom memory region)
but that could be added).
Another benefit of approach is that one can replace backing
memory any other backend (file/memfd/pmem...) without affecting
device model.

> To create a memory, the user can instantiate and map this device by 
> issuing the following commands:
> device_add driver=sysbus-memory size=0x1000 id=someram
> sysbus-mmio-map device=someram addr=0

I'd imagine more generic approach would look like:

object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
device_add memdevice_frontend,memdev=mem1,addr=0x0

where [pre]plug hooks in machine can map device to
an appropriate address space/place at device realize time.
(see memory_device_[pre_]plug() for starters).


> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> >> ---
> >>   include/hw/mem/sysbus-memory.h | 28 ++++++++++++
> >>   hw/mem/sysbus-memory.c         | 80 ++++++++++++++++++++++++++++++++++
> >>   hw/mem/meson.build             |  2 +
> >>   3 files changed, 110 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..5c596f8b4f
> >> --- /dev/null
> >> +++ b/include/hw/mem/sysbus-memory.h
> >> @@ -0,0 +1,28 @@
> >> +/*
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + *
> >> + * SysBusDevice Memory
> >> + *
> >> + * Copyright (c) 2021 Greensocs
> >> + */
> >> +
> >> +#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..f1ad7ba7ec
> >> --- /dev/null
> >> +++ b/hw/mem/sysbus-memory.c
> >> @@ -0,0 +1,80 @@
> >> +/*
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + *
> >> + * SysBusDevice Memory
> >> + *
> >> + * Copyright (c) 2021 Greensocs
> >> + */
> >> +
> >> +#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) {
> >> +        return;
> >> +    }
> >> +
> >> +    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 82f86d117e..04c74e12f2 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_SPARSE_MEM', if_true: files('sparse-mem.c'))
> >> +
> >> +softmmu_ss.add(files('sysbus-memory.c'))  
> >   
> 

Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Damien Hedde]

On 2/24/22 10:55, Igor Mammedov wrote:
> On Wed, 23 Feb 2022 11:19:49 +0100
> Damien Hedde <damien.hedde@greensocs.com> wrote:
> 
>> On 2/23/22 10:44, Igor Mammedov wrote:
>>> On Wed, 23 Feb 2022 10:07:05 +0100
>>> Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>    
>>>> This device can be used to create a memory wrapped into a
>>>> sysbus device.
>>>> This device has one property 'readonly' which allows
>>>> to choose between a ram or a rom.
>>>>   
>>>    
>>>> The purpose for this device is to be used with qapi command
>>>> device_add.
>>> that's the way to add a device to QEMU but a don't actual
>>> purpose described here, i.e. how board will use this
>>> device/actual usecase and how it will be wired to board
>>> and why it does have to be a sysbus device.
>>>    
>> Sorry, this was unclear.
>>
>> It is a sysbus device in order to use it like any other sysbus device.
>> The memory region it contains is exposed as a sysbus mmio.
> 
> aside of that sysbus is legacy fictional bus (albeit widely used),
> it doesn't scale to non sysbus devices (me thinking about buss-less
> pc-dimm & co) since eventually we would like to create mainstream
> machine types via QMP as well.
> 
>> I can replace the commit message by the following paragraph:
>>
>> Boards instantiate memories by creating memory region objects which is
>> not possible using QAPI commands.
> 
> That's not entirely true, you can use object-add with hostmem backends
> which do provide a means to allocate memory_region.
> (there is no rom hostmem backend probably (i.e. one that return rom memory region)
> but that could be added).
> Another benefit of approach is that one can replace backing
> memory any other backend (file/memfd/pmem...) without affecting
> device model.

I'm not familiar with memory backends. I need to take a look at them.
> 
>> To create a memory, the user can instantiate and map this device by
>> issuing the following commands:
>> device_add driver=sysbus-memory size=0x1000 id=someram
>> sysbus-mmio-map device=someram addr=0
> 
> I'd imagine more generic approach would look like:
> 
> object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
> device_add memdevice_frontend,memdev=mem1,addr=0x0
> 
> where [pre]plug hooks in machine can map device to
> an appropriate address space/place at device realize time.
> (see memory_device_[pre_]plug() for starters).
> 

We cannot rely on hooks the machine would define, because we start
from an empty machine. So anything must come from qapi and we would
need to do something like that I suppose:
object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe
sysbus-mmio-map device=memdev_fe addr=0

Thanks,
--
Damien

Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Igor Mammedov]

On Thu, 24 Feb 2022 12:43:21 +0100
Damien Hedde <damien.hedde@greensocs.com> wrote:

> On 2/24/22 10:55, Igor Mammedov wrote:
> > On Wed, 23 Feb 2022 11:19:49 +0100
> > Damien Hedde <damien.hedde@greensocs.com> wrote:
> >   
> >> On 2/23/22 10:44, Igor Mammedov wrote:  
> >>> On Wed, 23 Feb 2022 10:07:05 +0100
> >>> Damien Hedde <damien.hedde@greensocs.com> wrote:
> >>>      
> >>>> This device can be used to create a memory wrapped into a
> >>>> sysbus device.
> >>>> This device has one property 'readonly' which allows
> >>>> to choose between a ram or a rom.
> >>>>     
> >>>      
> >>>> The purpose for this device is to be used with qapi command
> >>>> device_add.  
> >>> that's the way to add a device to QEMU but a don't actual
> >>> purpose described here, i.e. how board will use this
> >>> device/actual usecase and how it will be wired to board
> >>> and why it does have to be a sysbus device.
> >>>      
> >> Sorry, this was unclear.
> >>
> >> It is a sysbus device in order to use it like any other sysbus device.
> >> The memory region it contains is exposed as a sysbus mmio.  
> > 
> > aside of that sysbus is legacy fictional bus (albeit widely used),
> > it doesn't scale to non sysbus devices (me thinking about buss-less
> > pc-dimm & co) since eventually we would like to create mainstream
> > machine types via QMP as well.
> >   
> >> I can replace the commit message by the following paragraph:
> >>
> >> Boards instantiate memories by creating memory region objects which is
> >> not possible using QAPI commands.  
> > 
> > That's not entirely true, you can use object-add with hostmem backends
> > which do provide a means to allocate memory_region.
> > (there is no rom hostmem backend probably (i.e. one that return rom memory region)
> > but that could be added).
> > Another benefit of approach is that one can replace backing
> > memory any other backend (file/memfd/pmem...) without affecting
> > device model.  
> 
> I'm not familiar with memory backends. I need to take a look at them.
> >   
> >> To create a memory, the user can instantiate and map this device by
> >> issuing the following commands:
> >> device_add driver=sysbus-memory size=0x1000 id=someram
> >> sysbus-mmio-map device=someram addr=0  
> > 
> > I'd imagine more generic approach would look like:
> > 
> > object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
> > device_add memdevice_frontend,memdev=mem1,addr=0x0
> > 
> > where [pre]plug hooks in machine can map device to
> > an appropriate address space/place at device realize time.
> > (see memory_device_[pre_]plug() for starters).
> >   
> 
> We cannot rely on hooks the machine would define, because we start
> from an empty machine. So anything must come from qapi and we would
> need to do something like that I suppose:
> object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
> device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe
> sysbus-mmio-map device=memdev_fe addr=0

As I pointed out using legacy sysbus doesn't scale, also I'd avoid
spawning more new device based on it if it can be helped.

with bus-less design, machine is still empty, in advance prepared
plug callbacks, is practically meta-data saying which device class
map into which address space which is quite generic. It helps
to avoid having extra QMP command for mapping.
However if prebuilt mapping is problematic, maybe have an alternative
QMP command that would do mapping, just not limited to sysbus,
something like 

   map_at_as device=1 as={parent_mr_name,system,io} [addr=x overlap=y prio=z]

which should give you full control where and how to map device.
 
> Thanks,
> --
> Damien
> 
> 

Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Damien Hedde]

On 2/25/22 12:38, Igor Mammedov wrote:
> On Thu, 24 Feb 2022 12:43:21 +0100
> Damien Hedde <damien.hedde@greensocs.com> wrote:
> 
>> On 2/24/22 10:55, Igor Mammedov wrote:
>>> On Wed, 23 Feb 2022 11:19:49 +0100
>>> Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>    
>>>> On 2/23/22 10:44, Igor Mammedov wrote:
>>>>> On Wed, 23 Feb 2022 10:07:05 +0100
>>>>> Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>>>       
>>>>>> This device can be used to create a memory wrapped into a
>>>>>> sysbus device.
>>>>>> This device has one property 'readonly' which allows
>>>>>> to choose between a ram or a rom.
>>>>>>      
>>>>>       
>>>>>> The purpose for this device is to be used with qapi command
>>>>>> device_add.
>>>>> that's the way to add a device to QEMU but a don't actual
>>>>> purpose described here, i.e. how board will use this
>>>>> device/actual usecase and how it will be wired to board
>>>>> and why it does have to be a sysbus device.
>>>>>       
>>>> Sorry, this was unclear.
>>>>
>>>> It is a sysbus device in order to use it like any other sysbus device.
>>>> The memory region it contains is exposed as a sysbus mmio.
>>>
>>> aside of that sysbus is legacy fictional bus (albeit widely used),
>>> it doesn't scale to non sysbus devices (me thinking about buss-less
>>> pc-dimm & co) since eventually we would like to create mainstream
>>> machine types via QMP as well.
>>>    
>>>> I can replace the commit message by the following paragraph:
>>>>
>>>> Boards instantiate memories by creating memory region objects which is
>>>> not possible using QAPI commands.
>>>
>>> That's not entirely true, you can use object-add with hostmem backends
>>> which do provide a means to allocate memory_region.
>>> (there is no rom hostmem backend probably (i.e. one that return rom memory region)
>>> but that could be added).
>>> Another benefit of approach is that one can replace backing
>>> memory any other backend (file/memfd/pmem...) without affecting
>>> device model.
>>
>> I'm not familiar with memory backends. I need to take a look at them.
>>>    
>>>> To create a memory, the user can instantiate and map this device by
>>>> issuing the following commands:
>>>> device_add driver=sysbus-memory size=0x1000 id=someram
>>>> sysbus-mmio-map device=someram addr=0
>>>
>>> I'd imagine more generic approach would look like:
>>>
>>> object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
>>> device_add memdevice_frontend,memdev=mem1,addr=0x0
>>>
>>> where [pre]plug hooks in machine can map device to
>>> an appropriate address space/place at device realize time.
>>> (see memory_device_[pre_]plug() for starters).
>>>    
>>
>> We cannot rely on hooks the machine would define, because we start
>> from an empty machine. So anything must come from qapi and we would
>> need to do something like that I suppose:
>> object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
>> device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe
>> sysbus-mmio-map device=memdev_fe addr=0
> 
> As I pointed out using legacy sysbus doesn't scale, also I'd avoid
> spawning more new device based on it if it can be helped.

I'm not sure to get the issue with adding sysbus devices, there is a lot 
of them. And for most them, they'll never be put on anything else than a 
simple memory bus. This one is trivial.
Right now there is a sysbus and the whole buses tree starts from it, it 
propagates reset. Everything is based on it.

> 
> with bus-less design, machine is still empty, in advance prepared
> plug callbacks, is practically meta-data saying which device class
> map into which address space which is quite generic. It helps
> to avoid having extra QMP command for mapping.

AFAIK the sysbus is the only bus type, on which we cannot specify the 
mapping/addresses with some device_add command parameter (this is 
probably doable, but hard). That's why I proposed to add sysbus-mmio-map 
several months ago. I didn't look again since, it's probably easier now 
with the modification done to device_add.

> However if prebuilt mapping is problematic, maybe have an alternative
> QMP command that would do mapping, just not limited to sysbus,
> something like
> 
>     map_at_as device=1 as={parent_mr_name,system,io} [addr=x overlap=y prio=z]
> 
> which should give you full control where and how to map device.

sysbus-mmio-map is not introduced by this series just for this memory 
device. It is here to solve mapping of any existing sysbus devices.

By bus-less. You mean mapping a sysbus device on another address space 
than the main one exposed by the sysbus ? We can support this by adding 
an 'as' parameter to the mapping function.

You mentioned non sysbus devices above. I don't think we need to try to 
do super-commands to solve all use cases.
I think there is a need to map a sysbus device on a sysbus.
Maybe there is also a need to map a non-sysbus device (a memory region 
then ?) to an address space.

--
Damien

Re: [PATCH v4 00/14] Initial support for machine creation via QMP

[Damien Hedde]

Ping !

It would be good to have some feedback on 1st and 2nd part.

Thanks,
Damien

On 2/23/22 10:06, Damien Hedde wrote:
> Hi,
> 
> This series adds initial support to build a machine using QMP/QAPI
> commands. With this series, one can start from the 'none' machine,
> create cpus, sysbus devices, memory map them and wire interrupts.
> 
> Sorry for the huge cc list on this cover-letter. Apart from people
> who attended the kvm call about this topic, I've cc'ed you only
> according to MAINTAINERS file.
> 
> The series is divided in 4 parts which are independent of each other,
> but we need the 4 parts to be able to use this mechanism:
> + Patches 1 to 6 allow to use the qapi command device_add to cold
>    plug devices (like CLI -device do)
> + Patches 7 to 10 modify the 'none' machine which serves as base
>    machine.
> + Patches 11 to 13 handle memory mapping and memory creation
> + Patches 14 allows dynamic cold plug of opentitan/sifive_e machine
>    to build some example. This last patch is based on a cleanup
>    series: it probably works without it, but some config errors are
>    not handled (see based-on below).
> 
> Only patch 11 is reviewed-by.
> 
> v4:
> + cold plugging approach changed in order not to conflict with
>    startup. I do not add additional command to handle this so that
>    we can change everything easily.
> + device_add in cold plug context is also now equivalent to -device
>    CLI regarding -fw_cfg. I also added patches to modify the 'none'
>    machine.
> + reworked most of the none machine part
> + updated the sybus-mmio-map command patch
> 
> Note that there are still lot of limitations (for example if you try
> to create more cpus than the _max_cpus_, tcg will abort()).
> Basically all tasks done by machine init reading some parameters are
> really tricky: for example, loading complex firmware. But we have to
> start by something and all this is not accessible unless the user
> asked for none machine and -preconfig.
> 
> I can maintain the code introduced here. I'm not sure what's the
> process. Is there something else to do than propose a patch to
> MAINTAINERS ?
> If there is a global agreement on moving on with these feature, it
> would be great to have a login on qemu wiki so I can document
> limitations and the work being done to solve them.
> 
> A simple test can be done with the following scenario which build
> a machine subset of the opentitan.
> 
> $ cat commands.qmp
> // RAM 0x10000000
> device_add driver=sysbus-memory id=ram size=0x4000 readonly=false
> sysbus-mmio-map device=ram addr=268435456
> // CPUS
> device_add driver=riscv.hart_array id=cpus cpu-type=lowrisc-ibex-riscv-cpu num-harts=1 resetvec=0x8080
> // ROM 0x00008000
> device_add driver=sysbus-memory id=rom size=0x4000 readonly=true
> sysbus-mmio-map device=rom addr=32768
> // PLIC 0x48000000
> device_add driver=riscv.sifive.plic id=plic hart-config=M hartid-base=0 num-sources=180 num-priorities=3 priority-base=0x0 pending-base=0x1000 enable-base=0x2000 enable-stride=32 context-base=0x200000 context-stride=8 aperture-size=0x4005000
> sysbus-mmio-map device=plic addr=1207959552
> qom-set path=plic property=unnamed-gpio-out[1] value=cpus/harts[0]/unnamed-gpio-in[11]
> // UART 0x40000000
> device_add driver=ibex-uart id=uart chardev=serial0
> sysbus-mmio-map device=uart addr=1073741824
> qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2]
> // FIRMWARE
> device_add driver=loader cpu-num=0 file=/path/to/firmware.elf
> x-exit-preconfig
> 
> $ qemu-system-riscv32 -display none -M none -preconfig -serial stdio -qmp unix:/tmp/qmp-sock,server
> 
> In another terminal, you'll need to send the commands with, for example:
> $ grep -v '^//' commands.qmp | qmp-shell /tmp/qmp-sock -v
> 
> It is the same as running
> $ qemu-system-riscv32 -display none -M opentitan -serial stdio -kernel path/to/firmware.elf
> 
> If you need a firmware, you can pick this one
> https://github.com/GreenSocs/qemu-qmp-machines/blob/master/opentitan-echo.elf
> This firmware is just a small interrupt-based bare-metal program
> echoing back whatever is sent in the uart.
> 
> This repo contains also sifive_e machine example.
> 
> Based-on: <20220218164646.132112-1-damien.hedde@greensocs.com>
> "RiscV cleanups for user-related life cycles"
> 
> Thanks for your comments,
> --
> Damien
> 
> Damien Hedde (13):
>    machine: add phase_get() and document phase_check()/advance()
>    machine&vl: introduce phase_until() to handle phase transitions
>    vl: support machine-initialized target in phase_until()
>    qapi/device_add: compute is_hotplug flag
>    qapi/device_add: handle the rom_order_override when cold-plugging
>    none-machine: add the NoneMachineState structure
>    none-machine: add 'ram-addr' property
>    none-machine: allow cold plugging sysbus devices
>    none-machine: allow several cpus
>    softmmu/memory: add memory_region_try_add_subregion function
>    add sysbus-mmio-map qapi command
>    hw/mem/system-memory: add a memory sysbus device
>    hw: set user_creatable on opentitan/sifive_e devices
> 
> Mirela Grujic (1):
>    qapi/device_add: Allow execution in machine initialized phase
> 
>   qapi/qdev.json                 | 34 +++++++++++-
>   include/exec/memory.h          | 22 ++++++++
>   include/hw/mem/sysbus-memory.h | 28 ++++++++++
>   include/hw/qdev-core.h         | 33 ++++++++++++
>   hw/char/ibex_uart.c            |  1 +
>   hw/char/sifive_uart.c          |  1 +
>   hw/core/null-machine.c         | 68 ++++++++++++++++++++++--
>   hw/core/qdev.c                 |  5 ++
>   hw/core/sysbus.c               | 49 ++++++++++++++++++
>   hw/gpio/sifive_gpio.c          |  1 +
>   hw/intc/riscv_aclint.c         |  2 +
>   hw/intc/sifive_plic.c          |  1 +
>   hw/mem/sysbus-memory.c         | 80 +++++++++++++++++++++++++++++
>   hw/misc/sifive_e_prci.c        |  8 +++
>   hw/misc/unimp.c                |  1 +
>   hw/riscv/riscv_hart.c          |  1 +
>   hw/timer/ibex_timer.c          |  1 +
>   monitor/misc.c                 |  2 +-
>   softmmu/memory.c               | 23 ++++++---
>   softmmu/qdev-monitor.c         | 20 +++++++-
>   softmmu/vl.c                   | 94 ++++++++++++++++++++++++++--------
>   hmp-commands.hx                |  1 +
>   hw/mem/meson.build             |  2 +
>   23 files changed, 439 insertions(+), 39 deletions(-)
>   create mode 100644 include/hw/mem/sysbus-memory.h
>   create mode 100644 hw/mem/sysbus-memory.c
> 

Re: [PATCH v4 11/14] softmmu/memory: add memory_region_try_add_subregion function

[Philippe Mathieu-Daudé]

On 23/2/22 10:12, Damien Hedde wrote:
> Hi Philippe,
> 
> I suppose it is ok if I change your mail in the reviewed by ?

No, the email is fine (git tools should take care of using the
correct email via the .mailmap entry, see commit 90f285fd83).

> Thanks,
> Damien
> 
> On 2/23/22 10:07, Damien Hedde wrote:
>> It allows adding a subregion to a memory region with error handling.
>> Like memory_region_add_subregion_overlap(), it handles priority as
>> well. Apart from 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 of this function in the
>> context of qapi command which needs error handling support.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>   include/exec/memory.h | 22 ++++++++++++++++++++++
>>   softmmu/memory.c      | 23 +++++++++++++++--------
>>   2 files changed, 37 insertions(+), 8 deletions(-)

Re: [PATCH v4 07/14] none-machine: add the NoneMachineState structure

[Philippe Mathieu-Daudé]

On 23/2/22 10:06, Damien Hedde wrote:
> The none machine was using the parent state structure.
> We'll need a custom state to add a field in the following commit.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   hw/core/null-machine.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 08/14] none-machine: add 'ram-addr' property

[Philippe Mathieu-Daudé]

On 23/2/22 10:07, Damien Hedde wrote:
> Add the property to configure a the base address of the ram.
> The default value remains zero.
> 
> This commit is needed to use the 'none' machine as a base, and
> subsequently to dynamically populate it using qapi commands. Having
> a non null 'ram' is really hard to workaround because of the actual
> constraints on the generic loader: it prevents loading binaries
> bigger than ram_size (with a null ram, we cannot load anything).
> For now we need to be able to use the existing ram creation
> feature of the none machine with a configurable base address.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   hw/core/null-machine.c | 34 ++++++++++++++++++++++++++++++++--
>   1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> index 7eb258af07..5fd1cc0218 100644
> --- a/hw/core/null-machine.c
> +++ b/hw/core/null-machine.c
> @@ -16,9 +16,11 @@
>   #include "hw/boards.h"
>   #include "exec/address-spaces.h"
>   #include "hw/core/cpu.h"
> +#include "qapi/visitor.h"
>   
>   struct NoneMachineState {
>       MachineState parent;
> +    uint64_t ram_addr;
>   };
>   
>   #define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none")
> @@ -26,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState, NONE_MACHINE)
>   
>   static void machine_none_init(MachineState *mch)
>   {
> +    NoneMachineState *nms = NONE_MACHINE(mch);
>       CPUState *cpu = NULL;
>   
>       /* Initialize CPU (if user asked for it) */
> @@ -37,9 +40,13 @@ static void machine_none_init(MachineState *mch)
>           }
>       }
>   
> -    /* RAM at address zero */
> +    /* RAM at configured address (default: 0) */
>       if (mch->ram) {
> -        memory_region_add_subregion(get_system_memory(), 0, mch->ram);
> +        memory_region_add_subregion(get_system_memory(), nms->ram_addr,
> +                                    mch->ram);
> +    } else if (nms->ram_addr) {
> +        error_report("'ram-addr' has been specified but the size is zero");

I'm not sure about this error message, IIUC we can get here if no ram
backend is provided, not if we have one zero-sized. Otherwise LGTM.

> +        exit(1);
>       }

Re: [PATCH v4 09/14] none-machine: allow cold plugging sysbus devices

[Philippe Mathieu-Daudé]

On 23/2/22 10:07, Damien Hedde wrote:
> Allow plugging any sysbus device on this machine (the sysbus
> devices still need to be 'user-creatable').
> 
> This commit is needed to use the 'none' machine as a base, and
> subsequently to dynamically populate it with sysbus devices using
> qapi commands.
> 
> Note that this only concern cold-plug: sysbus devices cann't be hot

"can not" is easier to understand for non-native / not good level of
English speakers IMHO.

> plugged because the sysbus bus does not support it.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   hw/core/null-machine.c | 4 ++++
>   1 file changed, 4 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 12/14] add sysbus-mmio-map qapi command

[Philippe Mathieu-Daudé]

On 23/2/22 10:07, Damien Hedde 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 qapi 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 command.

"sysbus-mmio-unmap" typo I presume.

> This command is still experimental (hence the 'unstable' feature),
> as it is related to the sysbus device creation through qapi commands.
> 
> This command is required to be able to dynamically build a machine
> from scratch as there is no qapi-way of doing a memory mapping.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> Cc: Alistair Francis <alistair.francis@wdc.com>
> 
> v4:
>   + integrate priority parameter
>   + use 'unstable' feature flag instead of 'x-' prefix
>   + bump version to 7.0
>   + dropped Alistair's reviewed-by as a consequence
> ---
>   qapi/qdev.json   | 31 ++++++++++++++++++++++++++++++
>   hw/core/sysbus.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 80 insertions(+)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2e2de41499..4830e87a90 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -160,3 +160,34 @@
>   ##
>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>     'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @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.
> +#
> +# @priority: The priority of the mapping (defaults to 0).
> +#
> +# Features:
> +# @unstable: Command is meant to map sysbus devices
> +#            while in preconfig mode.
> +#
> +# Since: 7.0
> +#
> +# Returns: Nothing on success
> +#
> +##
> +
> +{ 'command': 'sysbus-mmio-map',
> +  'data': { 'device': 'str',
> +            '*mmio': 'uint8',
> +            'addr': 'uint64',
> +            '*priority': 'int32' },

I wonder if not providing the explicit parent container now could
be problematic later, and if we shouldn't start with a QOM MR path
(default to 'system_memory'). Anyway, sysbus are currently
restricted to system_memory so as you described, this mimics well
sysbus_mmio_map().

> +  'features': ['unstable'],
> +  'allow-preconfig' : true }
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 05c1da3d31..df1f1f43a5 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,54 @@ static void sysbus_mmio_map_common(SysBusDevice *dev, int n, hwaddr addr,
>       }
>   }
>   
> +void qmp_sysbus_mmio_map(const char *device,
> +                         bool has_mmio, uint8_t mmio,
> +                         uint64_t addr,
> +                         bool has_priority, int32_t priority,
> +                         Error **errp)
> +{
> +    Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE, NULL);
> +    SysBusDevice *dev;
> +
> +    if (phase_get() != PHASE_MACHINE_INITIALIZED) {
> +        error_setg(errp, "The command is permitted only when "
> +                         "the machine is in initialized phase");

"command only permitted during the " #PHASE_MACHINE_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 (!has_priority) {
> +        priority = 0;
> +    }
> +
> +    if (mmio >= dev->num_mmio) {
> +        error_setg(errp, "MMIO index '%u' does not exist in '%s'",
> +                   mmio, device);
> +        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, priority,
> +                                         errp)) {
> +        return;
> +    }
> +
> +    dev->mmio[mmio].addr = addr;
> +}

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 01/14] machine: add phase_get() and document phase_check()/advance()

[Philippe Mathieu-Daudé]

On 23/2/22 10:06, Damien Hedde wrote:
> phase_get() returns the current phase, we'll use it in next
> commit.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   include/hw/qdev-core.h | 19 +++++++++++++++++++
>   hw/core/qdev.c         |  5 +++++
>   2 files changed, 24 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 03/14] vl: support machine-initialized target in phase_until()

[Philippe Mathieu-Daudé]

On 23/2/22 10:06, Damien Hedde wrote:
> phase_until() now supports the following transitions:
> + accel-created -> machine-initialized
> + machine-initialized -> machine-ready
> 
> As a consequence we can now support the use of qmp_exit_preconfig()
> from phases _accel-created_ and _machine-initialized_.
> 
> This commit is a preparation to support cold plugging a device
> using qapi (which will be introduced in a following commit). For this
> we need fine grain control of the phase.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   softmmu/vl.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 04/14] qapi/device_add: compute is_hotplug flag

[Philippe Mathieu-Daudé]

On 23/2/22 10:06, Damien Hedde wrote:
> Instead of checking the phase everytime, just store the result
> in a flag. We will use more of it in the following commit.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   softmmu/qdev-monitor.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Philippe Mathieu-Daudé]

+Mark / Daniel / Markus / Alex for design.

On 25/2/22 16:31, Damien Hedde wrote:
> On 2/25/22 12:38, Igor Mammedov wrote:
>> On Thu, 24 Feb 2022 12:43:21 +0100
>> Damien Hedde <damien.hedde@greensocs.com> wrote:
>>> On 2/24/22 10:55, Igor Mammedov wrote:
>>>> On Wed, 23 Feb 2022 11:19:49 +0100
>>>> Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>>> On 2/23/22 10:44, Igor Mammedov wrote:
>>>>>> On Wed, 23 Feb 2022 10:07:05 +0100
>>>>>> Damien Hedde <damien.hedde@greensocs.com> wrote:
>>>>>>> This device can be used to create a memory wrapped into a
>>>>>>> sysbus device.
>>>>>>> This device has one property 'readonly' which allows
>>>>>>> to choose between a ram or a rom.
>>>>>>> The purpose for this device is to be used with qapi command
>>>>>>> device_add.
>>>>>> that's the way to add a device to QEMU but a don't actual
>>>>>> purpose described here, i.e. how board will use this
>>>>>> device/actual usecase and how it will be wired to board
>>>>>> and why it does have to be a sysbus device.
>>>>> Sorry, this was unclear.
>>>>>
>>>>> It is a sysbus device in order to use it like any other sysbus device.
>>>>> The memory region it contains is exposed as a sysbus mmio.
>>>>
>>>> aside of that sysbus is legacy fictional bus (albeit widely used),
>>>> it doesn't scale to non sysbus devices (me thinking about buss-less
>>>> pc-dimm & co) since eventually we would like to create mainstream
>>>> machine types via QMP as well.
>>>>> I can replace the commit message by the following paragraph:
>>>>>
>>>>> Boards instantiate memories by creating memory region objects which is
>>>>> not possible using QAPI commands.
>>>>
>>>> That's not entirely true, you can use object-add with hostmem backends
>>>> which do provide a means to allocate memory_region.
>>>> (there is no rom hostmem backend probably (i.e. one that return rom 
>>>> memory region)
>>>> but that could be added).
>>>> Another benefit of approach is that one can replace backing
>>>> memory any other backend (file/memfd/pmem...) without affecting
>>>> device model.
>>>
>>> I'm not familiar with memory backends. I need to take a look at them.
>>>>> To create a memory, the user can instantiate and map this device by
>>>>> issuing the following commands:
>>>>> device_add driver=sysbus-memory size=0x1000 id=someram
>>>>> sysbus-mmio-map device=someram addr=0
>>>>
>>>> I'd imagine more generic approach would look like:
>>>>
>>>> object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
>>>> device_add memdevice_frontend,memdev=mem1,addr=0x0
>>>>
>>>> where [pre]plug hooks in machine can map device to
>>>> an appropriate address space/place at device realize time.
>>>> (see memory_device_[pre_]plug() for starters).
>>>
>>> We cannot rely on hooks the machine would define, because we start
>>> from an empty machine. So anything must come from qapi and we would
>>> need to do something like that I suppose:
>>> object-add memory-backend-ram,id=mem1,size=0x1000,other_backend_twiks
>>> device_add sysbus-memory-frontend,memdev=mem1,id=memdev_fe
>>> sysbus-mmio-map device=memdev_fe addr=0
>>
>> As I pointed out using legacy sysbus doesn't scale, also I'd avoid
>> spawning more new device based on it if it can be helped.
> 
> I'm not sure to get the issue with adding sysbus devices, there is a lot 
> of them. And for most them, they'll never be put on anything else than a 
> simple memory bus. This one is trivial.
> Right now there is a sysbus and the whole buses tree starts from it, it 
> propagates reset. Everything is based on it.
> 
>>
>> with bus-less design, machine is still empty, in advance prepared
>> plug callbacks, is practically meta-data saying which device class
>> map into which address space which is quite generic. It helps
>> to avoid having extra QMP command for mapping.
> 
> AFAIK the sysbus is the only bus type, on which we cannot specify the 
> mapping/addresses with some device_add command parameter (this is 
> probably doable, but hard). That's why I proposed to add sysbus-mmio-map 
> several months ago. I didn't look again since, it's probably easier now 
> with the modification done to device_add.
> 
>> However if prebuilt mapping is problematic, maybe have an alternative
>> QMP command that would do mapping, just not limited to sysbus,
>> something like
>>
>>     map_at_as device=1 as={parent_mr_name,system,io} [addr=x overlap=y 
>> prio=z]
>>
>> which should give you full control where and how to map device.

I agree with Igor (this is what I mentioned I was worried about when
reviewing patch #12 "add sysbus-mmio-map qapi command").

Sysbus devices are limited to a single main bus, and it is limiting us.
Instead of growing the sysbus API, we should add such feature to the
qdev API, so it could benefit any device (sysbus devices being a corner
case of qdev devices).

> sysbus-mmio-map is not introduced by this series just for this memory 
> device. It is here to solve mapping of any existing sysbus devices.
> 
> By bus-less. You mean mapping a sysbus device on another address space 
> than the main one exposed by the sysbus ? We can support this by adding 
> an 'as' parameter to the mapping function.

Yes.

> You mentioned non sysbus devices above. I don't think we need to try to 
> do super-commands to solve all use cases.

I tend to disagree, we want to build any machine, being able to use all
of our devices, not only the sysbus ones. How do you plan to create a
Clock object for example? IIRC some machines don't use sysbus at all;
I don't think the raspi3b do for example.

> I think there is a need to map a sysbus device on a sysbus.
> Maybe there is also a need to map a non-sysbus device (a memory region 
> then ?) to an address space.

Certainly.

Re: [PATCH v4 08/14] none-machine: add 'ram-addr' property

[Damien Hedde]

On 3/3/22 15:41, Philippe Mathieu-Daudé wrote:
> On 23/2/22 10:07, Damien Hedde wrote:
>> Add the property to configure a the base address of the ram.
>> The default value remains zero.
>>
>> This commit is needed to use the 'none' machine as a base, and
>> subsequently to dynamically populate it using qapi commands. Having
>> a non null 'ram' is really hard to workaround because of the actual
>> constraints on the generic loader: it prevents loading binaries
>> bigger than ram_size (with a null ram, we cannot load anything).
>> For now we need to be able to use the existing ram creation
>> feature of the none machine with a configurable base address.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>   hw/core/null-machine.c | 34 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
>> index 7eb258af07..5fd1cc0218 100644
>> --- a/hw/core/null-machine.c
>> +++ b/hw/core/null-machine.c
>> @@ -16,9 +16,11 @@
>>   #include "hw/boards.h"
>>   #include "exec/address-spaces.h"
>>   #include "hw/core/cpu.h"
>> +#include "qapi/visitor.h"
>>   struct NoneMachineState {
>>       MachineState parent;
>> +    uint64_t ram_addr;
>>   };
>>   #define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none")
>> @@ -26,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState, 
>> NONE_MACHINE)
>>   static void machine_none_init(MachineState *mch)
>>   {
>> +    NoneMachineState *nms = NONE_MACHINE(mch);
>>       CPUState *cpu = NULL;
>>       /* Initialize CPU (if user asked for it) */
>> @@ -37,9 +40,13 @@ static void machine_none_init(MachineState *mch)
>>           }
>>       }
>> -    /* RAM at address zero */
>> +    /* RAM at configured address (default: 0) */
>>       if (mch->ram) {
>> -        memory_region_add_subregion(get_system_memory(), 0, mch->ram);
>> +        memory_region_add_subregion(get_system_memory(), nms->ram_addr,
>> +                                    mch->ram);
>> +    } else if (nms->ram_addr) {
>> +        error_report("'ram-addr' has been specified but the size is 
>> zero");
> 
> I'm not sure about this error message, IIUC we can get here if no ram
> backend is provided, not if we have one zero-sized. Otherwise LGTM.

You're most probably right. Keeping the ram_size to 0 is just one way of 
getting here. I can replace the message by a more generic formulation
"'ram-addr' has been specified but the machine has no ram"

Re: [PATCH v4 12/14] add sysbus-mmio-map qapi command

[Damien Hedde]

On 3/3/22 15:59, Philippe Mathieu-Daudé wrote:
> On 23/2/22 10:07, Damien Hedde 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 qapi 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 command.
> 
> "sysbus-mmio-unmap" typo I presume.
> 
>> This command is still experimental (hence the 'unstable' feature),
>> as it is related to the sysbus device creation through qapi commands.
>>
>> This command is required to be able to dynamically build a machine
>> from scratch as there is no qapi-way of doing a memory mapping.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>> Cc: Alistair Francis <alistair.francis@wdc.com>
>>
>> v4:
>>   + integrate priority parameter
>>   + use 'unstable' feature flag instead of 'x-' prefix
>>   + bump version to 7.0
>>   + dropped Alistair's reviewed-by as a consequence
>> ---
>>   qapi/qdev.json   | 31 ++++++++++++++++++++++++++++++
>>   hw/core/sysbus.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 80 insertions(+)
>>
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 2e2de41499..4830e87a90 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -160,3 +160,34 @@
>>   ##
>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>     'data': { '*device': 'str', 'path': 'str' } }
>> +
>> +##
>> +# @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.
>> +#
>> +# @priority: The priority of the mapping (defaults to 0).
>> +#
>> +# Features:
>> +# @unstable: Command is meant to map sysbus devices
>> +#            while in preconfig mode.
>> +#
>> +# Since: 7.0
>> +#
>> +# Returns: Nothing on success
>> +#
>> +##
>> +
>> +{ 'command': 'sysbus-mmio-map',
>> +  'data': { 'device': 'str',
>> +            '*mmio': 'uint8',
>> +            'addr': 'uint64',
>> +            '*priority': 'int32' },
> 
> I wonder if not providing the explicit parent container now could
> be problematic later, and if we shouldn't start with a QOM MR path
> (default to 'system_memory'). Anyway, sysbus are currently
> restricted to system_memory so as you described, this mimics well
> sysbus_mmio_map().

I think we ended-up adding such a parameter to handle complex xilinx 
machines having several cpu clusters. I wanted to stay simple in this 
series here as there are probably several way to address this issue. (we 
could also add a bus parameter, and create more sysbus).
We can still add the parameter later, with an appropriate default value 
(or even make the parameter mandatory).

If everybody agree to go for the bus-less approach. I can add the MR 
parameter right now.

CCing Igor

>> +void qmp_sysbus_mmio_map(const char *device,
>> +                         bool has_mmio, uint8_t mmio,
>> +                         uint64_t addr,
>> +                         bool has_priority, int32_t priority,
>> +                         Error **errp)
>> +{
>> +    Object *obj = object_resolve_path_type(device, 
>> TYPE_SYS_BUS_DEVICE, NULL);
>> +    SysBusDevice *dev;
>> +
>> +    if (phase_get() != PHASE_MACHINE_INITIALIZED) {
>> +        error_setg(errp, "The command is permitted only when "
>> +                         "the machine is in initialized phase");
> 
> "command only permitted during the " #PHASE_MACHINE_INITIALIZED "phase"?

What do you mean by '#', this is not a macro parameter. 
PHASE_MACHINE_INITIALIZED is just an enum value and there is no 
human/qapi exposed name.
"when the machine is initialized/initializing" ?
I think all the machine phase error message are bit like that (not 
mentioning the phase).

Re: [PATCH v4 11/14] softmmu/memory: add memory_region_try_add_subregion function

[Damien Hedde]

On 3/3/22 14:32, Philippe Mathieu-Daudé wrote:
> On 23/2/22 10:12, Damien Hedde wrote:
>> Hi Philippe,
>>
>> I suppose it is ok if I change your mail in the reviewed by ?
> 
> No, the email is fine (git tools should take care of using the
> correct email via the .mailmap entry, see commit 90f285fd83).
> 
>> Thanks,
>> Damien
>>

ok.

Looks like git keeps as-is the "*-by:" entries untouched when cc-ing them.

--
Damien

Re: [PATCH v4 14/14] hw: set user_creatable on opentitan/sifive_e devices

[Philippe Mathieu-Daudé]

On 23/2/22 10:07, Damien Hedde wrote:
> The devices are:
> + ibex-timer
> + ibex-uart
> + riscv.aclint.swi
> + riscv.aclint.mtimer
> + riscv.hart_array
> + riscv.sifive.e.prci
> + riscv.sifive.plic
> + riscv.sifive.uart
> + sifive_soc.gpio
> + unimplemented-device
> 
> These devices are clean regarding error handling in realize.
> 
> They are all sysbus devices, so setting user-creatable will only
> enable cold-plugging them on machine having explicitely allowed them
> (only _none_ machine does that).
> 
> Note that this commit include the ricv_array which embeds cpus. There

Typo "includes" I guess.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> are some deep internal constraints about them: you cannot create more
> cpus than the machine's maxcpus. TCG accelerator's code will for example
> assert if a user try to create too many cpus.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> I can also split this patch if you think it's better.
> But it is mostly a one-line fix per file.
> 
> This patch requires first some cleanups in order to fix error errors
> and some more memory leaks that could happend in legit user-related
> life cycles: a miss-configuration should not be a fatal error anymore.
> https://lore.kernel.org/qemu-devel/20220218164646.132112-1-damien.hedde@greensocs.com
> ---
>   hw/char/ibex_uart.c     | 1 +
>   hw/char/sifive_uart.c   | 1 +
>   hw/gpio/sifive_gpio.c   | 1 +
>   hw/intc/riscv_aclint.c  | 2 ++
>   hw/intc/sifive_plic.c   | 1 +
>   hw/misc/sifive_e_prci.c | 8 ++++++++
>   hw/misc/unimp.c         | 1 +
>   hw/riscv/riscv_hart.c   | 1 +
>   hw/timer/ibex_timer.c   | 1 +
>   9 files changed, 17 insertions(+)

Re: [PATCH v4 14/14] hw: set user_creatable on opentitan/sifive_e devices

[Philippe Mathieu-Daudé]

On 23/2/22 10:07, Damien Hedde wrote:
> The devices are:
> + ibex-timer
> + ibex-uart
> + riscv.aclint.swi
> + riscv.aclint.mtimer
> + riscv.hart_array
> + riscv.sifive.e.prci
> + riscv.sifive.plic
> + riscv.sifive.uart
> + sifive_soc.gpio
> + unimplemented-device
> 
> These devices are clean regarding error handling in realize.
> 
> They are all sysbus devices, so setting user-creatable will only
> enable cold-plugging them on machine having explicitely allowed them
> (only _none_ machine does that).
> 
> Note that this commit include the ricv_array which embeds cpus. There

Typo "includes" I guess.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> are some deep internal constraints about them: you cannot create more
> cpus than the machine's maxcpus. TCG accelerator's code will for example
> assert if a user try to create too many cpus.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> 
> I can also split this patch if you think it's better.
> But it is mostly a one-line fix per file.
> 
> This patch requires first some cleanups in order to fix error errors
> and some more memory leaks that could happend in legit user-related
> life cycles: a miss-configuration should not be a fatal error anymore.
> https://lore.kernel.org/qemu-devel/20220218164646.132112-1-damien.hedde@greensocs.com
> ---
>   hw/char/ibex_uart.c     | 1 +
>   hw/char/sifive_uart.c   | 1 +
>   hw/gpio/sifive_gpio.c   | 1 +
>   hw/intc/riscv_aclint.c  | 2 ++
>   hw/intc/sifive_plic.c   | 1 +
>   hw/misc/sifive_e_prci.c | 8 ++++++++
>   hw/misc/unimp.c         | 1 +
>   hw/riscv/riscv_hart.c   | 1 +
>   hw/timer/ibex_timer.c   | 1 +
>   9 files changed, 17 insertions(+)

Re: [PATCH v4 02/14] machine&vl: introduce phase_until() to handle phase transitions

[Damien Hedde]

Hi !

It would nice to have some feedback about this solution.
Basically it just introduces a new function 'qemu_until' which implement
the startup fsm.
It's bit like what Markus proposed in december (hence the name), only it 
is is only internal so we can change it if we find out it should be done 
otherwise regarding startup.

In practice it is just some code move from qmp_exit_preconfig() to 
phase_until() with more indentation (not sure if there is a way to make 
that easier to read).

In the following patches I use it in device_add() to ensure the startup 
is advanced.

Thanks,
--
Damien

On 2/23/22 10:06, Damien Hedde wrote:
> phase_until() is implemented in vl.c and is meant to be used
> to make startup progress up to a specified phase being reached().
> At this point, no behavior change is introduced: phase_until()
> only supports a single double transition corresponding
> to the functionality of qmp_exit_preconfig():
> + accel-created -> machine-initialized -> machine-ready
> 
> As a result qmp_exit_preconfig() now uses phase_until().
> 
> This commit is a preparation to support cold plugging a device
> using qapi command (which will be introduced in a following commit).
> For this we need fine grain control of the phase.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>   include/hw/qdev-core.h | 14 ++++++++
>   softmmu/vl.c           | 78 ++++++++++++++++++++++++++++++++----------
>   2 files changed, 74 insertions(+), 18 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index e29c705b74..5f73d06408 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -909,4 +909,18 @@ extern bool phase_check(MachineInitPhase phase);
>    */
>   extern void phase_advance(MachineInitPhase phase);
>   
> +/**
> + * @phase_until:
> + * @phase: the target phase
> + * @errp: error report
> + *
> + * Make the machine init progress until the target phase is reached.
> + *
> + * Its is a no-op is the target phase is the current or an earlier
> + * phase.
> + *
> + * Returns true in case of success.
> + */
> +extern bool phase_until(MachineInitPhase phase, Error **errp);
> +
>   #endif
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 5e1b35ba48..5689d0be88 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2741,30 +2741,72 @@ void qmp_x_exit_preconfig(Error **errp)
>           error_setg(errp, "The command is permitted only before machine initialization");
>           return;
>       }
> +    phase_until(PHASE_MACHINE_READY, errp);
> +}
>   
> -    qemu_init_board();
> -    qemu_create_cli_devices();
> -    qemu_machine_creation_done();
> -
> -    if (loadvm) {
> -        load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
> -    }
> -    if (replay_mode != REPLAY_MODE_NONE) {
> -        replay_vmstate_init();
> +bool phase_until(MachineInitPhase phase, Error **errp)
> +{
> +    if (!phase_check(PHASE_ACCEL_CREATED)) {
> +        error_setg(errp, "Phase transition is not supported until accelerator"
> +                   " is created");
> +        return false;
>       }
>   
> -    if (incoming) {
> -        Error *local_err = NULL;
> -        if (strcmp(incoming, "defer") != 0) {
> -            qmp_migrate_incoming(incoming, &local_err);
> -            if (local_err) {
> -                error_reportf_err(local_err, "-incoming %s: ", incoming);
> -                exit(1);
> +    while (!phase_check(phase)) {
> +        MachineInitPhase cur_phase = phase_get();
> +
> +        switch (cur_phase) {
> +        case PHASE_ACCEL_CREATED:
> +            qemu_init_board();
> +            /* We are now in PHASE_MACHINE_INITIALIZED. */
> +            qemu_create_cli_devices();
> +            /*
> +             * At this point all CLI options are handled apart:
> +             * + -S (autostart)
> +             * + -incoming
> +             */
> +            qemu_machine_creation_done();
> +            /* We are now in PHASE_MACHINE_READY. */
> +
> +            if (loadvm) {
> +                load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
>               }
> +            if (replay_mode != REPLAY_MODE_NONE) {
> +                replay_vmstate_init();
> +            }
> +
> +            if (incoming) {
> +                Error *local_err = NULL;
> +                if (strcmp(incoming, "defer") != 0) {
> +                    qmp_migrate_incoming(incoming, &local_err);
> +                    if (local_err) {
> +                        error_reportf_err(local_err, "-incoming %s: ",
> +                                          incoming);
> +                        exit(1);
> +                    }
> +                }
> +            } else if (autostart) {
> +                qmp_cont(NULL);
> +            }
> +            break;
> +
> +        default:
> +            /*
> +             * If we end up here, it is because we miss a case above.
> +             */
> +            error_setg(&error_abort, "Requested phase transition is not"
> +                       " implemented");
> +            return false;
>           }
> -    } else if (autostart) {
> -        qmp_cont(NULL);
> +
> +        /*
> +         * Ensure we made some progress.
> +         * With the default case above, it should be enough to prevent
> +         * any infinite loop.
> +         */
> +        assert(cur_phase < phase_get());
>       }
> +    return true;
>   }
>   
>   void qemu_init(int argc, char **argv, char **envp)

Re: [PATCH v4 00/14] Initial support for machine creation via QMP

[Jim Shu]

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

Hi all,

Thanks for the work!

I'm from SiFive and we are very interested in this feature.
QMP/QAPI configurable QEMU machine is a useful feature in our use case.
With this feature, we can both model our versatile FPGA-based platforms
more easily and model a new platform without modification of source code.
It is helpful for early software development of SoC prototyping.
We think this feature is also helpful to the QEMU community.

Also, I have tested this patchset (v4) and newer v5 patchset [1] with
Damien's firmware [2] and it works correctly.

p.s. QMP option "-qmp socket,path=./qmpsocket,server" in v5 patchset
instruction may not work?
I use the option "-qmp unix:./qmpsocket,server" instead.

[1] [PATCH v5 0/6] QAPI support for device cold-plug
https://lore.kernel.org/qemu-devel/20220519153402.41540-1-damien.hedde@greensocs.com/

[2] Test firmware for patchset
v5: https://github.com/GreenSocs/qemu-qmp-machines/tree/master/arm-virt
v4:
https://github.com/GreenSocs/qemu-qmp-machines/tree/eba16dab8b587e624d65c5c302aeef424bece3a0

On Thu, Mar 3, 2022 at 7:02 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

> Ping !
>
> It would be good to have some feedback on 1st and 2nd part.
>
> Thanks,
> Damien
>
> On 2/23/22 10:06, Damien Hedde wrote:
> > Hi,
> >
> > This series adds initial support to build a machine using QMP/QAPI
> > commands. With this series, one can start from the 'none' machine,
> > create cpus, sysbus devices, memory map them and wire interrupts.
> >
> > Sorry for the huge cc list on this cover-letter. Apart from people
> > who attended the kvm call about this topic, I've cc'ed you only
> > according to MAINTAINERS file.
> >
> > The series is divided in 4 parts which are independent of each other,
> > but we need the 4 parts to be able to use this mechanism:
> > + Patches 1 to 6 allow to use the qapi command device_add to cold
> >    plug devices (like CLI -device do)
> > + Patches 7 to 10 modify the 'none' machine which serves as base
> >    machine.
> > + Patches 11 to 13 handle memory mapping and memory creation
> > + Patches 14 allows dynamic cold plug of opentitan/sifive_e machine
> >    to build some example. This last patch is based on a cleanup
> >    series: it probably works without it, but some config errors are
> >    not handled (see based-on below).
> >
> > Only patch 11 is reviewed-by.
> >
> > v4:
> > + cold plugging approach changed in order not to conflict with
> >    startup. I do not add additional command to handle this so that
> >    we can change everything easily.
> > + device_add in cold plug context is also now equivalent to -device
> >    CLI regarding -fw_cfg. I also added patches to modify the 'none'
> >    machine.
> > + reworked most of the none machine part
> > + updated the sybus-mmio-map command patch
> >
> > Note that there are still lot of limitations (for example if you try
> > to create more cpus than the _max_cpus_, tcg will abort()).
> > Basically all tasks done by machine init reading some parameters are
> > really tricky: for example, loading complex firmware. But we have to
> > start by something and all this is not accessible unless the user
> > asked for none machine and -preconfig.
> >
> > I can maintain the code introduced here. I'm not sure what's the
> > process. Is there something else to do than propose a patch to
> > MAINTAINERS ?
> > If there is a global agreement on moving on with these feature, it
> > would be great to have a login on qemu wiki so I can document
> > limitations and the work being done to solve them.
> >
> > A simple test can be done with the following scenario which build
> > a machine subset of the opentitan.
> >
> > $ cat commands.qmp
> > // RAM 0x10000000
> > device_add driver=sysbus-memory id=ram size=0x4000 readonly=false
> > sysbus-mmio-map device=ram addr=268435456
> > // CPUS
> > device_add driver=riscv.hart_array id=cpus
> cpu-type=lowrisc-ibex-riscv-cpu num-harts=1 resetvec=0x8080
> > // ROM 0x00008000
> > device_add driver=sysbus-memory id=rom size=0x4000 readonly=true
> > sysbus-mmio-map device=rom addr=32768
> > // PLIC 0x48000000
> > device_add driver=riscv.sifive.plic id=plic hart-config=M hartid-base=0
> num-sources=180 num-priorities=3 priority-base=0x0 pending-base=0x1000
> enable-base=0x2000 enable-stride=32 context-base=0x200000 context-stride=8
> aperture-size=0x4005000
> > sysbus-mmio-map device=plic addr=1207959552
> > qom-set path=plic property=unnamed-gpio-out[1]
> value=cpus/harts[0]/unnamed-gpio-in[11]
> > // UART 0x40000000
> > device_add driver=ibex-uart id=uart chardev=serial0
> > sysbus-mmio-map device=uart addr=1073741824
> > qom-set path=uart property=sysbus-irq[1] value=plic/unnamed-gpio-in[2]
> > // FIRMWARE
> > device_add driver=loader cpu-num=0 file=/path/to/firmware.elf
> > x-exit-preconfig
> >
> > $ qemu-system-riscv32 -display none -M none -preconfig -serial stdio
> -qmp unix:/tmp/qmp-sock,server
> >
> > In another terminal, you'll need to send the commands with, for example:
> > $ grep -v '^//' commands.qmp | qmp-shell /tmp/qmp-sock -v
> >
> > It is the same as running
> > $ qemu-system-riscv32 -display none -M opentitan -serial stdio -kernel
> path/to/firmware.elf
> >
> > If you need a firmware, you can pick this one
> >
> https://github.com/GreenSocs/qemu-qmp-machines/blob/master/opentitan-echo.elf
> > This firmware is just a small interrupt-based bare-metal program
> > echoing back whatever is sent in the uart.
> >
> > This repo contains also sifive_e machine example.
> >
> > Based-on: <20220218164646.132112-1-damien.hedde@greensocs.com>
> > "RiscV cleanups for user-related life cycles"
> >
> > Thanks for your comments,
> > --
> > Damien
> >
> > Damien Hedde (13):
> >    machine: add phase_get() and document phase_check()/advance()
> >    machine&vl: introduce phase_until() to handle phase transitions
> >    vl: support machine-initialized target in phase_until()
> >    qapi/device_add: compute is_hotplug flag
> >    qapi/device_add: handle the rom_order_override when cold-plugging
> >    none-machine: add the NoneMachineState structure
> >    none-machine: add 'ram-addr' property
> >    none-machine: allow cold plugging sysbus devices
> >    none-machine: allow several cpus
> >    softmmu/memory: add memory_region_try_add_subregion function
> >    add sysbus-mmio-map qapi command
> >    hw/mem/system-memory: add a memory sysbus device
> >    hw: set user_creatable on opentitan/sifive_e devices
> >
> > Mirela Grujic (1):
> >    qapi/device_add: Allow execution in machine initialized phase
> >
> >   qapi/qdev.json                 | 34 +++++++++++-
> >   include/exec/memory.h          | 22 ++++++++
> >   include/hw/mem/sysbus-memory.h | 28 ++++++++++
> >   include/hw/qdev-core.h         | 33 ++++++++++++
> >   hw/char/ibex_uart.c            |  1 +
> >   hw/char/sifive_uart.c          |  1 +
> >   hw/core/null-machine.c         | 68 ++++++++++++++++++++++--
> >   hw/core/qdev.c                 |  5 ++
> >   hw/core/sysbus.c               | 49 ++++++++++++++++++
> >   hw/gpio/sifive_gpio.c          |  1 +
> >   hw/intc/riscv_aclint.c         |  2 +
> >   hw/intc/sifive_plic.c          |  1 +
> >   hw/mem/sysbus-memory.c         | 80 +++++++++++++++++++++++++++++
> >   hw/misc/sifive_e_prci.c        |  8 +++
> >   hw/misc/unimp.c                |  1 +
> >   hw/riscv/riscv_hart.c          |  1 +
> >   hw/timer/ibex_timer.c          |  1 +
> >   monitor/misc.c                 |  2 +-
> >   softmmu/memory.c               | 23 ++++++---
> >   softmmu/qdev-monitor.c         | 20 +++++++-
> >   softmmu/vl.c                   | 94 ++++++++++++++++++++++++++--------
> >   hmp-commands.hx                |  1 +
> >   hw/mem/meson.build             |  2 +
> >   23 files changed, 439 insertions(+), 39 deletions(-)
> >   create mode 100644 include/hw/mem/sysbus-memory.h
> >   create mode 100644 hw/mem/sysbus-memory.c
> >
>
>

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

Re: [PATCH v4 05/14] qapi/device_add: handle the rom_order_override when cold-plugging

[Jim Shu]

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

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

On Wed, Feb 23, 2022 at 5:18 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

> rom_set_order_override() and rom_reset_order_override() were called
> in qemu_create_cli_devices() to set the rom_order_override value
> once and for all when creating the devices added on CLI.
>
> Unfortunately this won't work with qapi commands.
>
> Move the calls inside device_add so that it will be done in every
> case:
> + CLI option: -device
> + QAPI command: device_add
>
> rom_[set|reset]_order_override() are implemented in hw/core/loader.c
> They either do nothing or call fw_cfg_[set|reset]_order_override().
> The later functions are implemented in hw/nvram/fw_cfg.c and only
> change an integer value of a "global" variable.
> In consequence, there are no complex side effects involved and we can
> safely move them from outside the -device option loop to the inner
> function.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  softmmu/qdev-monitor.c | 11 +++++++++++
>  softmmu/vl.c           |  2 --
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 47a89aee20..9ec3e0ebff 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -43,6 +43,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/clock.h"
>  #include "hw/boards.h"
> +#include "hw/loader.h"
>
>  /*
>   * Aliases were a bad idea from the start.  Let's keep them
> @@ -671,6 +672,10 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
>          return NULL;
>      }
>
> +    if (!is_hotplug) {
> +        rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
> +    }
> +
>      /* create device */
>      dev = qdev_new(driver);
>
> @@ -712,6 +717,9 @@ DeviceState *qdev_device_add_from_qdict(const QDict
> *opts,
>      if (!qdev_realize(DEVICE(dev), bus, errp)) {
>          goto err_del_dev;
>      }
> +    if (!is_hotplug) {
> +        rom_reset_order_override();
> +    }
>      return dev;
>
>  err_del_dev:
> @@ -719,6 +727,9 @@ err_del_dev:
>          object_unparent(OBJECT(dev));
>          object_unref(OBJECT(dev));
>      }
> +    if (!is_hotplug) {
> +        rom_reset_order_override();
> +    }
>      return NULL;
>  }
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 50337d68b9..b91ae1b8ae 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2680,7 +2680,6 @@ static void qemu_create_cli_devices(void)
>      }
>
>      /* init generic devices */
> -    rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE);
>      qemu_opts_foreach(qemu_find_opts("device"),
>                        device_init_func, NULL, &error_fatal);
>      QTAILQ_FOREACH(opt, &device_opts, next) {
> @@ -2697,7 +2696,6 @@ static void qemu_create_cli_devices(void)
>          object_unref(OBJECT(dev));
>          loc_pop(&opt->loc);
>      }
> -    rom_reset_order_override();
>  }
>
>  static void qemu_machine_creation_done(void)
> --
> 2.35.1
>
>
>

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

Re: [PATCH v4 07/14] none-machine: add the NoneMachineState structure

[Jim Shu]

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

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

On Wed, Feb 23, 2022 at 5:59 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

> The none machine was using the parent state structure.
> We'll need a custom state to add a field in the following commit.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  hw/core/null-machine.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> index f586a4bef5..7eb258af07 100644
> --- a/hw/core/null-machine.c
> +++ b/hw/core/null-machine.c
> @@ -17,6 +17,13 @@
>  #include "exec/address-spaces.h"
>  #include "hw/core/cpu.h"
>
> +struct NoneMachineState {
> +    MachineState parent;
> +};
> +
> +#define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none")
> +OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState, NONE_MACHINE)
> +
>  static void machine_none_init(MachineState *mch)
>  {
>      CPUState *cpu = NULL;
> @@ -42,8 +49,10 @@ static void machine_none_init(MachineState *mch)
>      }
>  }
>
> -static void machine_none_machine_init(MachineClass *mc)
> +static void machine_none_class_init(ObjectClass *oc, void *data)
>  {
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +
>      mc->desc = "empty machine";
>      mc->init = machine_none_init;
>      mc->max_cpus = 1;
> @@ -56,4 +65,15 @@ static void machine_none_machine_init(MachineClass *mc)
>      mc->no_sdcard = 1;
>  }
>
> -DEFINE_MACHINE("none", machine_none_machine_init)
> +static const TypeInfo none_machine_info = {
> +    .name          = TYPE_NONE_MACHINE,
> +    .parent        = TYPE_MACHINE,
> +    .instance_size = sizeof(NoneMachineState),
> +    .class_init    = machine_none_class_init,
> +};
> +
> +static void none_machine_register_types(void)
> +{
> +    type_register_static(&none_machine_info);
> +}
> +type_init(none_machine_register_types);
> --
> 2.35.1
>
>
>

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

Re: [PATCH v4 08/14] none-machine: add 'ram-addr' property

[Jim Shu]

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

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

On Fri, Mar 4, 2022 at 12:36 AM Damien Hedde <damien.hedde@greensocs.com>
wrote:

>
>
> On 3/3/22 15:41, Philippe Mathieu-Daudé wrote:
> > On 23/2/22 10:07, Damien Hedde wrote:
> >> Add the property to configure a the base address of the ram.
> >> The default value remains zero.
> >>
> >> This commit is needed to use the 'none' machine as a base, and
> >> subsequently to dynamically populate it using qapi commands. Having
> >> a non null 'ram' is really hard to workaround because of the actual
> >> constraints on the generic loader: it prevents loading binaries
> >> bigger than ram_size (with a null ram, we cannot load anything).
> >> For now we need to be able to use the existing ram creation
> >> feature of the none machine with a configurable base address.
> >>
> >> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> >> ---
> >>   hw/core/null-machine.c | 34 ++++++++++++++++++++++++++++++++--
> >>   1 file changed, 32 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/core/null-machine.c b/hw/core/null-machine.c
> >> index 7eb258af07..5fd1cc0218 100644
> >> --- a/hw/core/null-machine.c
> >> +++ b/hw/core/null-machine.c
> >> @@ -16,9 +16,11 @@
> >>   #include "hw/boards.h"
> >>   #include "exec/address-spaces.h"
> >>   #include "hw/core/cpu.h"
> >> +#include "qapi/visitor.h"
> >>   struct NoneMachineState {
> >>       MachineState parent;
> >> +    uint64_t ram_addr;
> >>   };
> >>   #define TYPE_NONE_MACHINE MACHINE_TYPE_NAME("none")
> >> @@ -26,6 +28,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(NoneMachineState,
> >> NONE_MACHINE)
> >>   static void machine_none_init(MachineState *mch)
> >>   {
> >> +    NoneMachineState *nms = NONE_MACHINE(mch);
> >>       CPUState *cpu = NULL;
> >>       /* Initialize CPU (if user asked for it) */
> >> @@ -37,9 +40,13 @@ static void machine_none_init(MachineState *mch)
> >>           }
> >>       }
> >> -    /* RAM at address zero */
> >> +    /* RAM at configured address (default: 0) */
> >>       if (mch->ram) {
> >> -        memory_region_add_subregion(get_system_memory(), 0, mch->ram);
> >> +        memory_region_add_subregion(get_system_memory(), nms->ram_addr,
> >> +                                    mch->ram);
> >> +    } else if (nms->ram_addr) {
> >> +        error_report("'ram-addr' has been specified but the size is
> >> zero");
> >
> > I'm not sure about this error message, IIUC we can get here if no ram
> > backend is provided, not if we have one zero-sized. Otherwise LGTM.
>
> You're most probably right. Keeping the ram_size to 0 is just one way of
> getting here. I can replace the message by a more generic formulation
> "'ram-addr' has been specified but the machine has no ram"
>
>
>

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

Re: [PATCH v4 09/14] none-machine: allow cold plugging sysbus devices

[Jim Shu]

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

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

On Thu, Mar 3, 2022 at 10:46 PM Philippe Mathieu-Daudé <
philippe.mathieu.daude@gmail.com> wrote:

> On 23/2/22 10:07, Damien Hedde wrote:
> > Allow plugging any sysbus device on this machine (the sysbus
> > devices still need to be 'user-creatable').
> >
> > This commit is needed to use the 'none' machine as a base, and
> > subsequently to dynamically populate it with sysbus devices using
> > qapi commands.
> >
> > Note that this only concern cold-plug: sysbus devices cann't be hot
>
> "can not" is easier to understand for non-native / not good level of
> English speakers IMHO.
>
> > plugged because the sysbus bus does not support it.
> >
> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > ---
> >   hw/core/null-machine.c | 4 ++++
> >   1 file changed, 4 insertions(+)
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>

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

Re: [PATCH v4 12/14] add sysbus-mmio-map qapi command

[Jim Shu]

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

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

On Wed, Feb 23, 2022 at 5:37 PM 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 qapi 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 command.
>
> This command is still experimental (hence the 'unstable' feature),
> as it is related to the sysbus device creation through qapi commands.
>
> This command is required to be able to dynamically build a machine
> from scratch as there is no qapi-way of doing a memory mapping.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
> Cc: Alistair Francis <alistair.francis@wdc.com>
>
> v4:
>  + integrate priority parameter
>  + use 'unstable' feature flag instead of 'x-' prefix
>  + bump version to 7.0
>  + dropped Alistair's reviewed-by as a consequence
> ---
>  qapi/qdev.json   | 31 ++++++++++++++++++++++++++++++
>  hw/core/sysbus.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2e2de41499..4830e87a90 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -160,3 +160,34 @@
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @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.
> +#
> +# @priority: The priority of the mapping (defaults to 0).
> +#
> +# Features:
> +# @unstable: Command is meant to map sysbus devices
> +#            while in preconfig mode.
> +#
> +# Since: 7.0
> +#
> +# Returns: Nothing on success
> +#
> +##
> +
> +{ 'command': 'sysbus-mmio-map',
> +  'data': { 'device': 'str',
> +            '*mmio': 'uint8',
> +            'addr': 'uint64',
> +            '*priority': 'int32' },
> +  'features': ['unstable'],
> +  'allow-preconfig' : true }
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 05c1da3d31..df1f1f43a5 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,54 @@ static void sysbus_mmio_map_common(SysBusDevice *dev,
> int n, hwaddr addr,
>      }
>  }
>
> +void qmp_sysbus_mmio_map(const char *device,
> +                         bool has_mmio, uint8_t mmio,
> +                         uint64_t addr,
> +                         bool has_priority, int32_t priority,
> +                         Error **errp)
> +{
> +    Object *obj = object_resolve_path_type(device, TYPE_SYS_BUS_DEVICE,
> NULL);
> +    SysBusDevice *dev;
> +
> +    if (phase_get() != PHASE_MACHINE_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 (!has_priority) {
> +        priority = 0;
> +    }
> +
> +    if (mmio >= dev->num_mmio) {
> +        error_setg(errp, "MMIO index '%u' does not exist in '%s'",
> +                   mmio, device);
> +        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, priority,
> +                                         errp)) {
> +        return;
> +    }
> +
> +    dev->mmio[mmio].addr = addr;
> +}
> +
>  void sysbus_mmio_unmap(SysBusDevice *dev, int n)
>  {
>      assert(n >= 0 && n < dev->num_mmio);
> --
> 2.35.1
>
>
>

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

Re: [PATCH v4 11/14] softmmu/memory: add memory_region_try_add_subregion function

[Jim Shu]

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

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


On Fri, Mar 4, 2022 at 7:00 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

>
>
>
> On 3/3/22 14:32, Philippe Mathieu-Daudé wrote:
> > On 23/2/22 10:12, Damien Hedde wrote:
> >> Hi Philippe,
> >>
> >> I suppose it is ok if I change your mail in the reviewed by ?
> >
> > No, the email is fine (git tools should take care of using the
> > correct email via the .mailmap entry, see commit 90f285fd83).
> >
> >> Thanks,
> >> Damien
> >>
>
> ok.
>
> Looks like git keeps as-is the "*-by:" entries untouched when cc-ing them.
>
> --
> Damien
>
>

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

Re: [PATCH v4 13/14] hw/mem/system-memory: add a memory sysbus device

[Jim Shu]

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

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

On Wed, Feb 23, 2022 at 5:14 PM Damien Hedde <damien.hedde@greensocs.com>
wrote:

> This device can be used to create a memory wrapped into a
> sysbus device.
> This device has one property 'readonly' which allows
> to choose between a ram or a rom.
>
> The purpose for this device is to be used with qapi command
> device_add.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>  include/hw/mem/sysbus-memory.h | 28 ++++++++++++
>  hw/mem/sysbus-memory.c         | 80 ++++++++++++++++++++++++++++++++++
>  hw/mem/meson.build             |  2 +
>  3 files changed, 110 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..5c596f8b4f
> --- /dev/null
> +++ b/include/hw/mem/sysbus-memory.h
> @@ -0,0 +1,28 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SysBusDevice Memory
> + *
> + * Copyright (c) 2021 Greensocs
> + */
> +
> +#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..f1ad7ba7ec
> --- /dev/null
> +++ b/hw/mem/sysbus-memory.c
> @@ -0,0 +1,80 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * SysBusDevice Memory
> + *
> + * Copyright (c) 2021 Greensocs
> + */
> +
> +#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) {
> +        return;
> +    }
> +
> +    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 82f86d117e..04c74e12f2 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_SPARSE_MEM', if_true: files('sparse-mem.c'))
> +
> +softmmu_ss.add(files('sysbus-memory.c'))
> --
> 2.35.1
>
>
>

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

Re: [PATCH v4 14/14] hw: set user_creatable on opentitan/sifive_e devices

[Jim Shu]

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

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

On Fri, Mar 4, 2022 at 11:23 PM Philippe Mathieu-Daudé <
philippe.mathieu.daude@gmail.com> wrote:

> On 23/2/22 10:07, Damien Hedde wrote:
> > The devices are:
> > + ibex-timer
> > + ibex-uart
> > + riscv.aclint.swi
> > + riscv.aclint.mtimer
> > + riscv.hart_array
> > + riscv.sifive.e.prci
> > + riscv.sifive.plic
> > + riscv.sifive.uart
> > + sifive_soc.gpio
> > + unimplemented-device
> >
> > These devices are clean regarding error handling in realize.
> >
> > They are all sysbus devices, so setting user-creatable will only
> > enable cold-plugging them on machine having explicitely allowed them
> > (only _none_ machine does that).
> >
> > Note that this commit include the ricv_array which embeds cpus. There
>
> Typo "includes" I guess.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> > are some deep internal constraints about them: you cannot create more
> > cpus than the machine's maxcpus. TCG accelerator's code will for example
> > assert if a user try to create too many cpus.
> >
> > Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> > ---
> >
> > I can also split this patch if you think it's better.
> > But it is mostly a one-line fix per file.
> >
> > This patch requires first some cleanups in order to fix error errors
> > and some more memory leaks that could happend in legit user-related
> > life cycles: a miss-configuration should not be a fatal error anymore.
> >
> https://lore.kernel.org/qemu-devel/20220218164646.132112-1-damien.hedde@greensocs.com
> > ---
> >   hw/char/ibex_uart.c     | 1 +
> >   hw/char/sifive_uart.c   | 1 +
> >   hw/gpio/sifive_gpio.c   | 1 +
> >   hw/intc/riscv_aclint.c  | 2 ++
> >   hw/intc/sifive_plic.c   | 1 +
> >   hw/misc/sifive_e_prci.c | 8 ++++++++
> >   hw/misc/unimp.c         | 1 +
> >   hw/riscv/riscv_hart.c   | 1 +
> >   hw/timer/ibex_timer.c   | 1 +
> >   9 files changed, 17 insertions(+)
>
>

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