[PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[Gregor Haas]

This patch series adds support for specifying OpenSBI domains on the QEMU
command line. A simple example of what this looks like is below, including
mapping the board's UART into the secondary domain:

qemu-system-riscv64 -machine virt -bios fw_jump.bin -cpu max -smp 2 -m 4G -nographic \
        -device opensbi-memregion,id=mem,base=0xBC000000,order=26,mmio=false \
        -device opensbi-memregion,id=uart,base=0x10000000,order=12,mmio=true,device0="/soc/serial@10000000" \
        -device opensbi-domain,id=domain,possible-harts=0-1,boot-hart=0x0,next-addr=0xBC000000,next-mode=1,region0=mem,perms0=0x3f,region1=uart,perms1=0x3f

As a result of the above configuration, QEMU will add the following subnodes to
the device tree:

chosen {
        opensbi-domains {
                compatible = "opensbi,domain,config";

                domain {
                        next-mode = <0x01>;
                        next-addr = <0x00 0xbc000000>;
                        boot-hart = <0x03>;
                        regions = <0x8000 0x3f 0x8002 0x3f>;
                        possible-harts = <0x03 0x01>;
                        phandle = <0x8003>;
                        compatible = "opensbi,domain,instance";
                };

                uart {
                        phandle = <0x8002>;
                        devices = <0x1800000>;
                        mmio;
                        order = <0x0c>;
                        base = <0x00 0x10000000>;
                        compatible = "opensbi,domain,memregion";
                };

                mem {
                        phandle = <0x8000>;
                        order = <0x1a>;
                        base = <0x00 0xbc000000>;
                        compatible = "opensbi,domain,memregion";
                };
        };
};

This results in OpenSBI output as below, where regions 01-03 are inherited from
the root domain and regions 00 and 04 correspond to the user specified ones:

Domain1 Name              : domain
Domain1 Boot HART         : 0
Domain1 HARTs             : 0,1
Domain1 Region00          : 0x0000000010000000-0x0000000010000fff M: (I,R,W,X) S/U: (R,W,X)
Domain1 Region01          : 0x0000000002000000-0x000000000200ffff M: (I,R,W) S/U: ()
Domain1 Region02          : 0x0000000080080000-0x000000008009ffff M: (R,W) S/U: ()
Domain1 Region03          : 0x0000000080000000-0x000000008007ffff M: (R,X) S/U: ()
Domain1 Region04          : 0x00000000bc000000-0x00000000bfffffff M: (R,W,X) S/U: (R,W,X)
Domain1 Next Address      : 0x00000000bc000000
Domain1 Next Arg1         : 0x0000000000000000
Domain1 Next Mode         : S-mode
Domain1 SysReset          : no
Domain1 SysSuspend        : no

v3:
- Addressed review comments from v2 by adding default values to new properties.
  This results in concrete errors at QEMU configuration time if a mandatory
  property (as mandated by the OpenSBI spec) is not provided.
- Changed command line encoding for the possible-harts field from a CPU bitmask
  (e.g. where bit X is set if CPU X is a possible hart) to a range format (e.g.
  the possible harts should be CPUs X-Y, where Y >= X). This does constrain the
  hart assignment to consecutive ranges of harts, but this constraint is also
  present for other QEMU subsystems (such as NUMA).
- Added create_fdt_one_device(), which is invoked when scanning the device tree
  for a memregion's devices. This function allocates a phandle for a region's
  device if one does not yet exist.

v2:
- Addressed review comments from v1. Specifically, renamed domain.{c,h} ->
  opensbi_domain.{c,h} to increase clarity of what these files do. Also, more
  consistently use g_autofree for dynamically allocated variables
- Added an "assign" flag to OpenSBIDomainState, which indicates whether to
  assign the domain's boot hart to it at domain parsing time.

Gregor Haas (1):
  Add support for generating OpenSBI domains in the device tree

 MAINTAINERS                       |   7 +
 hw/riscv/Kconfig                  |   4 +
 hw/riscv/meson.build              |   1 +
 hw/riscv/opensbi_domain.c         | 542 ++++++++++++++++++++++++++++++
 hw/riscv/virt.c                   |   3 +
 include/hw/riscv/opensbi_domain.h |  50 +++
 6 files changed, 607 insertions(+)
 create mode 100644 hw/riscv/opensbi_domain.c
 create mode 100644 include/hw/riscv/opensbi_domain.h

-- 
2.45.2

[PATCH v3 1/1] Add support for generating OpenSBI domains in the device tree

[Gregor Haas]

OpenSBI has support for domains, which are partitions of CPUs and memory into
isolated compartments. Domains can be specified in the device tree according to
a standardized format [1], which OpenSBI parses at boot time to initialize all
system domains. This patch enables simply specifying domains (and their
associated memory regions) on the QEMU command line, from which these are then
rendered into the machine's device tree.

At machine initialization time, a new create_fdt_opensbi_domains() function
walks the peripherals/peripherals-anon containers, identifies all domains and
memregions, and parses them into the relevant device tree structures.

[1] https://github.com/riscv-software-src/opensbi/blob/master/docs/domain_support.md

Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com>
---
 MAINTAINERS                       |   7 +
 hw/riscv/Kconfig                  |   4 +
 hw/riscv/meson.build              |   1 +
 hw/riscv/opensbi_domain.c         | 542 ++++++++++++++++++++++++++++++
 hw/riscv/virt.c                   |   3 +
 include/hw/riscv/opensbi_domain.h |  50 +++
 6 files changed, 607 insertions(+)
 create mode 100644 hw/riscv/opensbi_domain.c
 create mode 100644 include/hw/riscv/opensbi_domain.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 98eddf7ae1..796c023a7b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -355,6 +355,13 @@ F: target/riscv/XVentanaCondOps.decode
 F: target/riscv/insn_trans/trans_xventanacondops.c.inc
 F: disas/riscv-xventana*
 
+RISC-V OpenSBI domain support
+M: Gregor Haas <gregorhaas1997@gmail.com>
+L: qemu-riscv@nongnu.org
+S: Maintained
+F: hw/riscv/opensbi_domain.c
+F: include/hw/riscv/opensbi_domain.h
+
 RENESAS RX CPUs
 R: Yoshinori Sato <ysato@users.sourceforge.jp>
 S: Orphan
diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index a2030e3a6f..db3a4d77ad 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -1,6 +1,9 @@
 config RISCV_NUMA
     bool
 
+config RISCV_OPENSBI_DOMAIN
+    bool
+
 config IBEX
     bool
 
@@ -40,6 +43,7 @@ config RISCV_VIRT
     imply TPM_TIS_SYSBUS
     select DEVICE_TREE
     select RISCV_NUMA
+    select RISCV_OPENSBI_DOMAIN
     select GOLDFISH_RTC
     select PCI
     select PCI_EXPRESS_GENERIC_BRIDGE
diff --git a/hw/riscv/meson.build b/hw/riscv/meson.build
index f872674093..f47626c164 100644
--- a/hw/riscv/meson.build
+++ b/hw/riscv/meson.build
@@ -1,6 +1,7 @@
 riscv_ss = ss.source_set()
 riscv_ss.add(files('boot.c'))
 riscv_ss.add(when: 'CONFIG_RISCV_NUMA', if_true: files('numa.c'))
+riscv_ss.add(when: 'CONFIG_RISCV_OPENSBI_DOMAIN', if_true: files('opensbi_domain.c'))
 riscv_ss.add(files('riscv_hart.c'))
 riscv_ss.add(when: 'CONFIG_OPENTITAN', if_true: files('opentitan.c'))
 riscv_ss.add(when: 'CONFIG_RISCV_VIRT', if_true: files('virt.c'))
diff --git a/hw/riscv/opensbi_domain.c b/hw/riscv/opensbi_domain.c
new file mode 100644
index 0000000000..67a6bab538
--- /dev/null
+++ b/hw/riscv/opensbi_domain.c
@@ -0,0 +1,542 @@
+#include "qemu/osdep.h"
+#include "hw/riscv/opensbi_domain.h"
+#include "hw/boards.h"
+#include "hw/riscv/virt.h"
+#include "qapi/error.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "sysemu/device_tree.h"
+
+#include <libfdt.h>
+
+static void create_fdt_domain_possible_harts(MachineState *ms,
+                                             OpenSBIDomainState *s,
+                                             char *path) {
+    unsigned long i, cpu;
+    unsigned long num_cpus;
+
+    num_cpus = s->last_possible_hart - s->first_possible_hart + 1;
+    if (num_cpus) {
+        g_autofree uint32_t *phandles = g_malloc0_n(num_cpus, sizeof(uint32_t));
+
+        for (i = 0, cpu = s->first_possible_hart; i < num_cpus; i++, cpu++) {
+            g_autofree char *cpu_name = g_strdup_printf("/cpus/cpu@%li", cpu);
+            phandles[i] = cpu_to_fdt32(qemu_fdt_get_phandle(
+                    ms->fdt, cpu_name));
+        }
+
+        qemu_fdt_setprop(ms->fdt, path, "possible-harts",
+                         phandles, num_cpus * 4);
+    }
+}
+
+static void create_fdt_domain_regions(MachineState *ms,
+                                      OpenSBIDomainState *s,
+                                      char *path) {
+    unsigned long i;
+    int num_regions = 0;
+    DeviceState *ds;
+
+    for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
+        if (s->regions[i]) {
+            num_regions++;
+        }
+    }
+
+    if (num_regions) {
+        g_autofree uint32_t *regions =
+                 g_malloc0_n(num_regions, 2 * sizeof(uint32_t));
+        for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
+            if (s->regions[i]) {
+                ds = DEVICE(s->regions[i]);
+                g_autofree char *region_name = g_strdup_printf(
+                       "/chosen/opensbi-domains/%s", ds->id);
+                regions[2 * i] = cpu_to_fdt32(qemu_fdt_get_phandle
+                        (ms->fdt, region_name));
+                regions[2 * i + 1] = cpu_to_fdt32(s->region_perms[i]);
+            }
+        }
+
+        qemu_fdt_setprop(ms->fdt, path, "regions",
+                         regions, num_regions * 8);
+    }
+}
+
+struct DomainFDTState {
+    MachineState *ms;
+    bool regions;
+};
+
+static void create_fdt_one_domain(MachineState *ms, OpenSBIDomainState *s)
+{
+    DeviceState *ds = DEVICE(s);
+    g_autofree char *path, *cpu_name;
+
+    if (ds->id) {
+        path = g_strdup_printf("/chosen/opensbi-domains/%s",
+                               ds->id);
+    } else {
+        path = g_strdup_printf("/chosen/opensbi-domains/domain@%lx",
+                               s->next_addr);
+    }
+
+    qemu_fdt_add_subnode(ms->fdt, path);
+    qemu_fdt_setprop_string(ms->fdt, path, "compatible",
+                            "opensbi,domain,instance");
+    qemu_fdt_setprop_cells(ms->fdt, path, "phandle",
+                           qemu_fdt_alloc_phandle(ms->fdt));
+
+    create_fdt_domain_possible_harts(ms, s, path);
+    create_fdt_domain_regions(ms, s, path);
+
+    /* Assign boot hart to this domain */
+    if (s->boot_hart != -1) {
+        cpu_name = g_strdup_printf("/cpus/cpu@%i", s->boot_hart);
+        qemu_fdt_setprop_cell(ms->fdt, path, "boot-hart",
+                              qemu_fdt_get_phandle(ms->fdt, cpu_name));
+        if (s->assign) {
+            qemu_fdt_setprop_cell(ms->fdt, cpu_name, "opensbi-domain",
+                                    qemu_fdt_get_phandle(ms->fdt, path));
+        }
+    }
+
+    if (s->next_arg1 != -1) {
+        qemu_fdt_setprop_cells(ms->fdt, path, "next-arg1",
+                             (uint64_t) s->next_arg1 >> 32, s->next_arg1);
+    }
+
+    if (s->next_addr != -1) {
+        qemu_fdt_setprop_cells(ms->fdt, path, "next-addr",
+                             (uint64_t) s->next_addr >> 32, s->next_addr);
+    }
+
+    if (s->next_mode != -1) {
+        qemu_fdt_setprop_cell(ms->fdt, path, "next-mode",
+                            s->next_mode);
+    }
+
+    if (s->system_reset_allowed) {
+        qemu_fdt_setprop(ms->fdt, path, "system-reset-allowed", NULL, 0);
+    }
+
+    if (s->system_suspend_allowed) {
+        qemu_fdt_setprop(ms->fdt, path, "system-suspend-allowed", NULL, 0);
+    }
+}
+
+static uint32_t create_fdt_one_device(MachineState *ms, char *device)
+{
+    uint32_t phandle;
+    int offs = fdt_path_offset(ms->fdt, device);
+
+    if (offs < 0) {
+        error_report("%s: Could not find device %s: %s", __func__,
+                     device, fdt_strerror(offs));
+        exit(1);
+    }
+
+    phandle = fdt_get_phandle(ms->fdt, offs);
+    if (!phandle) {
+        phandle = qemu_fdt_alloc_phandle(ms->fdt);
+        qemu_fdt_setprop_cell(ms->fdt, device, "phandle", phandle);
+    }
+
+    return phandle;
+}
+
+static void create_fdt_one_memregion(MachineState *ms,
+                                     OpenSBIMemregionState *s)
+{
+    g_autofree char *path;
+    int i, dev, num_devices;
+    DeviceState *ds = DEVICE(s);
+
+    path = g_strdup_printf("/chosen/opensbi-domains/%s", ds->id);
+    qemu_fdt_add_subnode(ms->fdt, path);
+    qemu_fdt_setprop_string(ms->fdt, path, "compatible",
+                            "opensbi,domain,memregion");
+    qemu_fdt_setprop_cells(ms->fdt, path, "base",
+                           (uint64_t) s->base >> 32, s->base);
+
+    qemu_fdt_setprop_cell(ms->fdt, path, "order",
+                          (uint32_t) s->order);
+
+    if (s->mmio) {
+        qemu_fdt_setprop(ms->fdt, path, "mmio", NULL, 0);
+
+        /* Get all phandles for related devices */
+        num_devices = 0;
+        for (i = 0; i < OPENSBI_MEMREGION_DEVICES_MAX; i++) {
+            if (s->devices[i]) {
+                num_devices++;
+            }
+        }
+
+        if (num_devices) {
+            g_autofree uint32_t *devices =
+                g_malloc0_n(num_devices, sizeof(uint32_t));
+            for (i = 0, dev = 0; i < OPENSBI_MEMREGION_DEVICES_MAX &&
+                                 dev < num_devices; i++) {
+                if (s->devices[i]) {
+                    devices[dev++] = create_fdt_one_device(ms,
+                                                         s->devices[i]);
+                }
+            }
+
+            qemu_fdt_setprop(ms->fdt, path, "devices", devices,
+                             num_devices * 4);
+        }
+    }
+
+    qemu_fdt_setprop_cells(ms->fdt, path, "phandle",
+                           qemu_fdt_alloc_phandle(ms->fdt));
+}
+
+static int create_fdt_domains(Object *obj, void *opaque)
+{
+    struct DomainFDTState *dfs = opaque;
+    OpenSBIDomainState *osds;
+    OpenSBIMemregionState *osms;
+
+    osds = (OpenSBIDomainState *)
+            object_dynamic_cast(obj, TYPE_OPENSBI_DOMAIN);
+    osms = (OpenSBIMemregionState *)
+            object_dynamic_cast(obj, TYPE_OPENSBI_MEMREGION);
+
+    if (dfs->regions) {
+        if (osms) {
+            create_fdt_one_memregion(dfs->ms, osms);
+        }
+    } else {
+        if (osds) {
+            create_fdt_one_domain(dfs->ms, osds);
+        }
+    }
+
+    return 0;
+}
+
+static const char *containers[] = {
+        "/peripheral", "/peripheral-anon"
+};
+
+void create_fdt_opensbi_domains(MachineState *s)
+{
+    int i;
+    MachineState *ms = MACHINE(s);
+    Object *container;
+
+    struct DomainFDTState check = {
+            .ms = ms,
+            .regions = true
+    };
+
+    /* Make sure that top-level node exists */
+    qemu_fdt_add_subnode(ms->fdt, "/chosen/opensbi-domains");
+    qemu_fdt_setprop_string(ms->fdt, "/chosen/opensbi-domains",
+                            "compatible", "opensbi,domain,config");
+
+    /* Do a scan through regions first */
+    for (i = 0; i < ARRAY_SIZE(containers); i++) {
+        container = container_get(OBJECT(s), containers[i]);
+        object_child_foreach(container, create_fdt_domains, &check);
+    }
+
+    /* Then scan through domains */
+    check.regions = false;
+    for (i = 0; i < ARRAY_SIZE(containers); i++) {
+        container = container_get(OBJECT(s), containers[i]);
+        object_child_foreach(container, create_fdt_domains, &check);
+    }
+}
+
+/* OpenSBI Memregions */
+
+static void set_mmio(Object *obj, bool val, Error **err)
+{
+    OpenSBIMemregionState *s = OPENSBI_MEMREGION(obj);
+    s->mmio = val;
+}
+
+static void set_device(Object *obj, const char *val, Error **err)
+{
+    int i;
+    OpenSBIMemregionState *s = OPENSBI_MEMREGION(obj);
+
+    for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
+        if (!s->devices[i]) {
+            s->devices[i] = g_strdup(val);
+            break;
+        }
+    }
+}
+
+static void opensbi_memregion_instance_init(Object *obj)
+{
+    int i;
+    OpenSBIMemregionState *s = OPENSBI_MEMREGION(obj);
+
+    s->base = -1;
+    object_property_add_uint64_ptr(obj, "base", &s->base,
+                                   OBJ_PROP_FLAG_WRITE);
+    object_property_set_description(obj, "base",
+                                    "The base address of the domain memory region. If \"order\" is also specified, "
+                                    "this property should be a 2 ^ order aligned 64 bit address");
+
+    s->order = -1;
+    object_property_add_uint32_ptr(obj, "order", &s->order,
+                                   OBJ_PROP_FLAG_WRITE);
+    object_property_set_description(obj, "order",
+                                    "The order of the domain memory region. This property should have a 32 bit value "
+                                    "(i.e. one DT cell) in the range 3 <= order <= __riscv_xlen.");
+
+    s->mmio = false;
+    object_property_add_bool(obj, "mmio", NULL, set_mmio);
+    object_property_set_description(obj, "mmio",
+                                    "A boolean flag representing whether the domain memory region is a "
+                                    "memory-mapped I/O (MMIO) region.");
+
+    for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
+        g_autofree char *propname = g_strdup_printf("device%i", i);
+        object_property_add_str(obj, propname, NULL, set_device);
+
+        g_autofree char *description = g_strdup_printf(
+                "Device %i (out of %i) for this memregion. This property should be a device tree path to the device.",
+                i, OPENSBI_DOMAIN_MEMREGIONS_MAX);
+        object_property_set_description(obj, propname, description);
+    }
+}
+
+static void opensbi_memregion_realize(DeviceState *ds, Error **errp)
+{
+    #if defined(TARGET_RISCV32)
+    int xlen = 32;
+    #elif defined(TARGET_RISCV64)
+    int xlen = 64;
+    #endif
+
+    OpenSBIMemregionState *s = OPENSBI_MEMREGION(ds);
+
+    if (s->base == -1) {
+        error_setg(errp, "must specify base");
+        return;
+    }
+
+    if (s->order == -1) {
+        error_setg(errp, "must specify order");
+        return;
+    }
+
+    /* Check order bounds */
+    if (s->order < 3 || s->order > xlen) {
+        error_setg(errp, "order must be between 3 and %d", xlen);
+        return;
+    }
+
+    /* Check base alignment */
+    if (s->order < xlen && (s->base & (BIT(s->order) - 1))) {
+        error_setg(errp, "base not aligned to order");
+        return;
+    }
+}
+
+static void opensbi_memregion_class_init(ObjectClass *oc, void *opaque)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    dc->realize = opensbi_memregion_realize;
+}
+
+static const TypeInfo opensbi_memregion_info = {
+        .name = TYPE_OPENSBI_MEMREGION,
+        .parent = TYPE_DEVICE,
+        .instance_init = opensbi_memregion_instance_init,
+        .instance_size = sizeof(OpenSBIDomainState),
+        .class_init = opensbi_memregion_class_init
+};
+
+/* OpenSBI Domains */
+
+static void set_sysreset_allowed(Object *obj, bool val, Error **err)
+{
+    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
+    s->system_reset_allowed = val;
+}
+
+static void set_suspend_allowed(Object *obj, bool val, Error **err)
+{
+    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
+    s->system_suspend_allowed = val;
+}
+
+static void set_assign(Object *obj, bool val, Error **err)
+{
+    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
+    s->assign = val;
+}
+
+static void set_possible_harts(Object *obj, const char *str, Error **err)
+{
+    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
+    const char *firstcpu,  *firstcpu_end, *lastcpu;
+
+    firstcpu = str;
+    if (qemu_strtoul(firstcpu, &firstcpu_end, 0,
+                     &s->first_possible_hart) < 0) {
+        error_setg(err, "could not convert firstcpu");
+        return;
+    }
+
+    lastcpu = qemu_strchrnul(str, '-');
+    if (*lastcpu) {
+        if (lastcpu != firstcpu_end) {
+            error_setg(err, "could not separate firstcpu and lastcpu");
+            return;
+        }
+
+        lastcpu++;
+        if (qemu_strtoul(lastcpu, NULL, 0,
+                         &s->last_possible_hart) < 0) {
+            error_setg(err, "could not convert lastcpu");
+            return;
+        }
+    } else {
+        s->last_possible_hart = s->first_possible_hart;
+    }
+}
+
+static void opensbi_domain_instance_init(Object *obj)
+{
+    int i;
+    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
+
+    s->boot_hart = VIRT_CPUS_MAX;
+    object_property_add_uint32_ptr(obj, "boot-hart", &s->boot_hart,
+                                   OBJ_PROP_FLAG_WRITE);
+    object_property_set_description(obj, "boot-hart",
+                                    "The HART booting the domain instance.");
+
+    s->first_possible_hart = -1;
+    s->last_possible_hart = -1;
+    object_property_add_str(obj, "possible-harts", NULL, set_possible_harts);
+    object_property_set_description(obj, "possible-harts",
+                                    "The contiguous list of CPUs for the domain instance, specified as firstcpu[-lastcpu]");
+
+    s->next_arg1 = -1;
+    object_property_add_uint64_ptr(obj, "next-arg1", &s->next_arg1,
+                                   OBJ_PROP_FLAG_WRITE);
+    object_property_set_description(obj, "next-arg1",
+                                    "The 64 bit next booting stage arg1 for the domain instance.");
+
+    s->next_addr = -1;
+    object_property_add_uint64_ptr(obj, "next-addr", &s->next_addr,
+                                   OBJ_PROP_FLAG_WRITE);
+    object_property_set_description(obj, "next-addr",
+                                    "The 64 bit next booting stage address for the domain instance.");
+
+    s->next_mode = -1;
+    object_property_add_uint32_ptr(obj, "next-mode", &s->next_mode,
+                                   OBJ_PROP_FLAG_WRITE);
+    object_property_set_description(obj, "next-mode",
+                                    "The 32 bit next booting stage mode for the domain instance.");
+
+    s->system_reset_allowed = false;
+    object_property_add_bool(obj, "system-reset-allowed", NULL,
+                             set_sysreset_allowed);
+    object_property_set_description(obj, "system-reset-allowed",
+                                    "Whether the domain instance is allowed to do system reset.");
+
+    s->system_suspend_allowed = false;
+    object_property_add_bool(obj, "system-suspend-allowed", NULL,
+                             set_suspend_allowed);
+    object_property_set_description(obj, "system-suspend-allowed",
+                                    "Whether the domain instance is allowed to do system suspend.");
+
+    for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
+        s->regions[i] = NULL;
+        g_autofree char *reg_propname = g_strdup_printf("region%i", i);
+        object_property_add_link(obj, reg_propname, TYPE_OPENSBI_MEMREGION,
+                                 (Object **) &s->regions[i],
+                                 qdev_prop_allow_set_link_before_realize, 0);
+
+        g_autofree char *reg_description = g_strdup_printf(
+                "Region %i (out of %i) for this domain.",
+                i, OPENSBI_DOMAIN_MEMREGIONS_MAX);
+        object_property_set_description(obj, reg_propname, reg_description);
+
+        s->region_perms[i] = 0;
+        g_autofree char *perm_propname = g_strdup_printf("perms%i", i);
+        object_property_add_uint32_ptr(obj, perm_propname, &s->region_perms[i],
+                                       OBJ_PROP_FLAG_WRITE);
+
+        g_autofree char *perm_description = g_strdup_printf(
+                "Permissions for region %i for this domain.", i);
+        object_property_set_description(obj, perm_propname, perm_description);
+    }
+
+    object_property_add_bool(obj, "assign", NULL, set_assign);
+    object_property_set_description(obj, "assign",
+                                    "Whether to assign this domain to its boot hart.");
+}
+
+static void opensbi_domain_realize(DeviceState *ds, Error **errp)
+{
+    OpenSBIDomainState *s = OPENSBI_DOMAIN(ds);
+
+    if (s->boot_hart >= VIRT_CPUS_MAX) {
+        error_setg(errp, "boot hart larger than maximum number of CPUs (%d)",
+                 VIRT_CPUS_MAX);
+        return;
+    }
+
+    if (s->first_possible_hart == -1) {
+        if (s->last_possible_hart != -1) {
+            error_setg(errp,
+                     "last possible hart set when first possible hart unset");
+            return;
+        }
+    } else {
+        if (s->first_possible_hart >= VIRT_CPUS_MAX) {
+            error_setg(errp,
+                     "first possible hart larger than maximum number of CPUs (%d)",
+                     VIRT_CPUS_MAX);
+            return;
+        }
+
+        if (s->last_possible_hart != -1) {
+            if (s->last_possible_hart < s->first_possible_hart) {
+                error_setg(errp,
+                         "last possible hart larger than first possible hart");
+                return;
+            }
+
+            if (s->last_possible_hart >= VIRT_CPUS_MAX) {
+                error_setg(errp,
+                         "last possible hart larger than maximum number of CPUS (%d)",
+                         VIRT_CPUS_MAX);
+                return;
+            }
+        }
+    }
+}
+
+static void opensbi_domain_class_init(ObjectClass *oc, void *opaque)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    dc->realize = opensbi_domain_realize;
+}
+
+static const TypeInfo opensbi_domain_info = {
+        .name = TYPE_OPENSBI_DOMAIN,
+        .parent = TYPE_DEVICE,
+        .instance_init = opensbi_domain_instance_init,
+        .instance_size = sizeof(OpenSBIDomainState),
+        .class_init = opensbi_domain_class_init
+};
+
+static void opensbi_register_types(void)
+{
+    type_register_static(&opensbi_domain_info);
+    type_register_static(&opensbi_memregion_info);
+}
+
+type_init(opensbi_register_types)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 9981e0f6c9..bb4bf3ce5b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -55,6 +55,7 @@
 #include "hw/acpi/aml-build.h"
 #include "qapi/qapi-visit-common.h"
 #include "hw/virtio/virtio-iommu.h"
+#include "hw/riscv/opensbi_domain.h"
 
 /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
 static bool virt_use_kvm_aia(RISCVVirtState *s)
@@ -1051,6 +1052,8 @@ static void finalize_fdt(RISCVVirtState *s)
     create_fdt_uart(s, virt_memmap, irq_mmio_phandle);
 
     create_fdt_rtc(s, virt_memmap, irq_mmio_phandle);
+
+    create_fdt_opensbi_domains(MACHINE(s));
 }
 
 static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
diff --git a/include/hw/riscv/opensbi_domain.h b/include/hw/riscv/opensbi_domain.h
new file mode 100644
index 0000000000..bcce16a609
--- /dev/null
+++ b/include/hw/riscv/opensbi_domain.h
@@ -0,0 +1,50 @@
+
+#ifndef RISCV_DOMAIN_H
+#define RISCV_DOMAIN_H
+
+#include "hw/sysbus.h"
+#include "qom/object.h"
+#include "cpu.h"
+
+#define TYPE_OPENSBI_MEMREGION "opensbi-memregion"
+OBJECT_DECLARE_SIMPLE_TYPE(OpenSBIMemregionState, OPENSBI_MEMREGION)
+
+#define OPENSBI_MEMREGION_DEVICES_MAX   16
+
+struct OpenSBIMemregionState {
+    /* public */
+    DeviceState parent_obj;
+
+    /* private */
+    uint64_t base;
+    uint32_t order;
+    bool mmio;
+    char *devices[OPENSBI_MEMREGION_DEVICES_MAX];
+};
+
+#define TYPE_OPENSBI_DOMAIN "opensbi-domain"
+OBJECT_DECLARE_SIMPLE_TYPE(OpenSBIDomainState, OPENSBI_DOMAIN)
+
+#define OPENSBI_DOMAIN_MEMREGIONS_MAX   16
+
+struct OpenSBIDomainState {
+    /* public */
+    DeviceState parent_obj;
+
+    /* private */
+    OpenSBIMemregionState *regions[OPENSBI_DOMAIN_MEMREGIONS_MAX];
+    unsigned int region_perms[OPENSBI_DOMAIN_MEMREGIONS_MAX];
+    unsigned long first_possible_hart, last_possible_hart;
+    unsigned int boot_hart;
+    uint64_t next_arg1;
+    uint64_t next_addr;
+    uint32_t next_mode;
+    bool system_reset_allowed;
+    bool system_suspend_allowed;
+
+    bool assign;
+};
+
+void create_fdt_opensbi_domains(MachineState *s);
+
+#endif /* RISCV_DOMAIN_H */
-- 
2.45.2

Re: [PATCH v3 1/1] Add support for generating OpenSBI domains in the device tree

[Daniel Henrique Barboza]

On 8/5/24 6:04 PM, Gregor Haas wrote:
> OpenSBI has support for domains, which are partitions of CPUs and memory into
> isolated compartments. Domains can be specified in the device tree according to
> a standardized format [1], which OpenSBI parses at boot time to initialize all
> system domains. This patch enables simply specifying domains (and their
> associated memory regions) on the QEMU command line, from which these are then
> rendered into the machine's device tree.
> 
> At machine initialization time, a new create_fdt_opensbi_domains() function
> walks the peripherals/peripherals-anon containers, identifies all domains and
> memregions, and parses them into the relevant device tree structures.
> 
> [1] https://github.com/riscv-software-src/opensbi/blob/master/docs/domain_support.md
> 
> Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   MAINTAINERS                       |   7 +
>   hw/riscv/Kconfig                  |   4 +
>   hw/riscv/meson.build              |   1 +
>   hw/riscv/opensbi_domain.c         | 542 ++++++++++++++++++++++++++++++
>   hw/riscv/virt.c                   |   3 +
>   include/hw/riscv/opensbi_domain.h |  50 +++
>   6 files changed, 607 insertions(+)
>   create mode 100644 hw/riscv/opensbi_domain.c
>   create mode 100644 include/hw/riscv/opensbi_domain.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 98eddf7ae1..796c023a7b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -355,6 +355,13 @@ F: target/riscv/XVentanaCondOps.decode
>   F: target/riscv/insn_trans/trans_xventanacondops.c.inc
>   F: disas/riscv-xventana*
>   
> +RISC-V OpenSBI domain support
> +M: Gregor Haas <gregorhaas1997@gmail.com>
> +L: qemu-riscv@nongnu.org
> +S: Maintained
> +F: hw/riscv/opensbi_domain.c
> +F: include/hw/riscv/opensbi_domain.h
> +
>   RENESAS RX CPUs
>   R: Yoshinori Sato <ysato@users.sourceforge.jp>
>   S: Orphan
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index a2030e3a6f..db3a4d77ad 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -1,6 +1,9 @@
>   config RISCV_NUMA
>       bool
>   
> +config RISCV_OPENSBI_DOMAIN
> +    bool
> +
>   config IBEX
>       bool
>   
> @@ -40,6 +43,7 @@ config RISCV_VIRT
>       imply TPM_TIS_SYSBUS
>       select DEVICE_TREE
>       select RISCV_NUMA
> +    select RISCV_OPENSBI_DOMAIN
>       select GOLDFISH_RTC
>       select PCI
>       select PCI_EXPRESS_GENERIC_BRIDGE
> diff --git a/hw/riscv/meson.build b/hw/riscv/meson.build
> index f872674093..f47626c164 100644
> --- a/hw/riscv/meson.build
> +++ b/hw/riscv/meson.build
> @@ -1,6 +1,7 @@
>   riscv_ss = ss.source_set()
>   riscv_ss.add(files('boot.c'))
>   riscv_ss.add(when: 'CONFIG_RISCV_NUMA', if_true: files('numa.c'))
> +riscv_ss.add(when: 'CONFIG_RISCV_OPENSBI_DOMAIN', if_true: files('opensbi_domain.c'))
>   riscv_ss.add(files('riscv_hart.c'))
>   riscv_ss.add(when: 'CONFIG_OPENTITAN', if_true: files('opentitan.c'))
>   riscv_ss.add(when: 'CONFIG_RISCV_VIRT', if_true: files('virt.c'))
> diff --git a/hw/riscv/opensbi_domain.c b/hw/riscv/opensbi_domain.c
> new file mode 100644
> index 0000000000..67a6bab538
> --- /dev/null
> +++ b/hw/riscv/opensbi_domain.c
> @@ -0,0 +1,542 @@
> +#include "qemu/osdep.h"
> +#include "hw/riscv/opensbi_domain.h"
> +#include "hw/boards.h"
> +#include "hw/riscv/virt.h"
> +#include "qapi/error.h"
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/device_tree.h"
> +
> +#include <libfdt.h>
> +
> +static void create_fdt_domain_possible_harts(MachineState *ms,
> +                                             OpenSBIDomainState *s,
> +                                             char *path) {
> +    unsigned long i, cpu;
> +    unsigned long num_cpus;
> +
> +    num_cpus = s->last_possible_hart - s->first_possible_hart + 1;
> +    if (num_cpus) {
> +        g_autofree uint32_t *phandles = g_malloc0_n(num_cpus, sizeof(uint32_t));
> +
> +        for (i = 0, cpu = s->first_possible_hart; i < num_cpus; i++, cpu++) {
> +            g_autofree char *cpu_name = g_strdup_printf("/cpus/cpu@%li", cpu);
> +            phandles[i] = cpu_to_fdt32(qemu_fdt_get_phandle(
> +                    ms->fdt, cpu_name));
> +        }
> +
> +        qemu_fdt_setprop(ms->fdt, path, "possible-harts",
> +                         phandles, num_cpus * 4);
> +    }
> +}
> +
> +static void create_fdt_domain_regions(MachineState *ms,
> +                                      OpenSBIDomainState *s,
> +                                      char *path) {
> +    unsigned long i;
> +    int num_regions = 0;
> +    DeviceState *ds;
> +
> +    for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
> +        if (s->regions[i]) {
> +            num_regions++;
> +        }
> +    }
> +
> +    if (num_regions) {
> +        g_autofree uint32_t *regions =
> +                 g_malloc0_n(num_regions, 2 * sizeof(uint32_t));
> +        for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
> +            if (s->regions[i]) {
> +                ds = DEVICE(s->regions[i]);
> +                g_autofree char *region_name = g_strdup_printf(
> +                       "/chosen/opensbi-domains/%s", ds->id);
> +                regions[2 * i] = cpu_to_fdt32(qemu_fdt_get_phandle
> +                        (ms->fdt, region_name));
> +                regions[2 * i + 1] = cpu_to_fdt32(s->region_perms[i]);
> +            }
> +        }
> +
> +        qemu_fdt_setprop(ms->fdt, path, "regions",
> +                         regions, num_regions * 8);
> +    }
> +}
> +
> +struct DomainFDTState {
> +    MachineState *ms;
> +    bool regions;
> +};
> +
> +static void create_fdt_one_domain(MachineState *ms, OpenSBIDomainState *s)
> +{
> +    DeviceState *ds = DEVICE(s);
> +    g_autofree char *path, *cpu_name;
> +
> +    if (ds->id) {
> +        path = g_strdup_printf("/chosen/opensbi-domains/%s",
> +                               ds->id);
> +    } else {
> +        path = g_strdup_printf("/chosen/opensbi-domains/domain@%lx",
> +                               s->next_addr);
> +    }
> +
> +    qemu_fdt_add_subnode(ms->fdt, path);
> +    qemu_fdt_setprop_string(ms->fdt, path, "compatible",
> +                            "opensbi,domain,instance");
> +    qemu_fdt_setprop_cells(ms->fdt, path, "phandle",
> +                           qemu_fdt_alloc_phandle(ms->fdt));
> +
> +    create_fdt_domain_possible_harts(ms, s, path);
> +    create_fdt_domain_regions(ms, s, path);
> +
> +    /* Assign boot hart to this domain */
> +    if (s->boot_hart != -1) {
> +        cpu_name = g_strdup_printf("/cpus/cpu@%i", s->boot_hart);
> +        qemu_fdt_setprop_cell(ms->fdt, path, "boot-hart",
> +                              qemu_fdt_get_phandle(ms->fdt, cpu_name));
> +        if (s->assign) {
> +            qemu_fdt_setprop_cell(ms->fdt, cpu_name, "opensbi-domain",
> +                                    qemu_fdt_get_phandle(ms->fdt, path));
> +        }
> +    }
> +
> +    if (s->next_arg1 != -1) {
> +        qemu_fdt_setprop_cells(ms->fdt, path, "next-arg1",
> +                             (uint64_t) s->next_arg1 >> 32, s->next_arg1);
> +    }
> +
> +    if (s->next_addr != -1) {
> +        qemu_fdt_setprop_cells(ms->fdt, path, "next-addr",
> +                             (uint64_t) s->next_addr >> 32, s->next_addr);
> +    }
> +
> +    if (s->next_mode != -1) {
> +        qemu_fdt_setprop_cell(ms->fdt, path, "next-mode",
> +                            s->next_mode);
> +    }
> +
> +    if (s->system_reset_allowed) {
> +        qemu_fdt_setprop(ms->fdt, path, "system-reset-allowed", NULL, 0);
> +    }
> +
> +    if (s->system_suspend_allowed) {
> +        qemu_fdt_setprop(ms->fdt, path, "system-suspend-allowed", NULL, 0);
> +    }
> +}
> +
> +static uint32_t create_fdt_one_device(MachineState *ms, char *device)
> +{
> +    uint32_t phandle;
> +    int offs = fdt_path_offset(ms->fdt, device);
> +
> +    if (offs < 0) {
> +        error_report("%s: Could not find device %s: %s", __func__,
> +                     device, fdt_strerror(offs));
> +        exit(1);
> +    }
> +
> +    phandle = fdt_get_phandle(ms->fdt, offs);
> +    if (!phandle) {
> +        phandle = qemu_fdt_alloc_phandle(ms->fdt);
> +        qemu_fdt_setprop_cell(ms->fdt, device, "phandle", phandle);
> +    }
> +
> +    return phandle;
> +}
> +
> +static void create_fdt_one_memregion(MachineState *ms,
> +                                     OpenSBIMemregionState *s)
> +{
> +    g_autofree char *path;
> +    int i, dev, num_devices;
> +    DeviceState *ds = DEVICE(s);
> +
> +    path = g_strdup_printf("/chosen/opensbi-domains/%s", ds->id);
> +    qemu_fdt_add_subnode(ms->fdt, path);
> +    qemu_fdt_setprop_string(ms->fdt, path, "compatible",
> +                            "opensbi,domain,memregion");
> +    qemu_fdt_setprop_cells(ms->fdt, path, "base",
> +                           (uint64_t) s->base >> 32, s->base);
> +
> +    qemu_fdt_setprop_cell(ms->fdt, path, "order",
> +                          (uint32_t) s->order);
> +
> +    if (s->mmio) {
> +        qemu_fdt_setprop(ms->fdt, path, "mmio", NULL, 0);
> +
> +        /* Get all phandles for related devices */
> +        num_devices = 0;
> +        for (i = 0; i < OPENSBI_MEMREGION_DEVICES_MAX; i++) {
> +            if (s->devices[i]) {
> +                num_devices++;
> +            }
> +        }
> +
> +        if (num_devices) {
> +            g_autofree uint32_t *devices =
> +                g_malloc0_n(num_devices, sizeof(uint32_t));
> +            for (i = 0, dev = 0; i < OPENSBI_MEMREGION_DEVICES_MAX &&
> +                                 dev < num_devices; i++) {
> +                if (s->devices[i]) {
> +                    devices[dev++] = create_fdt_one_device(ms,
> +                                                         s->devices[i]);
> +                }
> +            }
> +
> +            qemu_fdt_setprop(ms->fdt, path, "devices", devices,
> +                             num_devices * 4);
> +        }
> +    }
> +
> +    qemu_fdt_setprop_cells(ms->fdt, path, "phandle",
> +                           qemu_fdt_alloc_phandle(ms->fdt));
> +}
> +
> +static int create_fdt_domains(Object *obj, void *opaque)
> +{
> +    struct DomainFDTState *dfs = opaque;
> +    OpenSBIDomainState *osds;
> +    OpenSBIMemregionState *osms;
> +
> +    osds = (OpenSBIDomainState *)
> +            object_dynamic_cast(obj, TYPE_OPENSBI_DOMAIN);
> +    osms = (OpenSBIMemregionState *)
> +            object_dynamic_cast(obj, TYPE_OPENSBI_MEMREGION);
> +
> +    if (dfs->regions) {
> +        if (osms) {
> +            create_fdt_one_memregion(dfs->ms, osms);
> +        }
> +    } else {
> +        if (osds) {
> +            create_fdt_one_domain(dfs->ms, osds);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static const char *containers[] = {
> +        "/peripheral", "/peripheral-anon"
> +};
> +
> +void create_fdt_opensbi_domains(MachineState *s)
> +{
> +    int i;
> +    MachineState *ms = MACHINE(s);
> +    Object *container;
> +
> +    struct DomainFDTState check = {
> +            .ms = ms,
> +            .regions = true
> +    };
> +
> +    /* Make sure that top-level node exists */
> +    qemu_fdt_add_subnode(ms->fdt, "/chosen/opensbi-domains");
> +    qemu_fdt_setprop_string(ms->fdt, "/chosen/opensbi-domains",
> +                            "compatible", "opensbi,domain,config");
> +
> +    /* Do a scan through regions first */
> +    for (i = 0; i < ARRAY_SIZE(containers); i++) {
> +        container = container_get(OBJECT(s), containers[i]);
> +        object_child_foreach(container, create_fdt_domains, &check);
> +    }
> +
> +    /* Then scan through domains */
> +    check.regions = false;
> +    for (i = 0; i < ARRAY_SIZE(containers); i++) {
> +        container = container_get(OBJECT(s), containers[i]);
> +        object_child_foreach(container, create_fdt_domains, &check);
> +    }
> +}
> +
> +/* OpenSBI Memregions */
> +
> +static void set_mmio(Object *obj, bool val, Error **err)
> +{
> +    OpenSBIMemregionState *s = OPENSBI_MEMREGION(obj);
> +    s->mmio = val;
> +}
> +
> +static void set_device(Object *obj, const char *val, Error **err)
> +{
> +    int i;
> +    OpenSBIMemregionState *s = OPENSBI_MEMREGION(obj);
> +
> +    for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
> +        if (!s->devices[i]) {
> +            s->devices[i] = g_strdup(val);
> +            break;
> +        }
> +    }
> +}
> +
> +static void opensbi_memregion_instance_init(Object *obj)
> +{
> +    int i;
> +    OpenSBIMemregionState *s = OPENSBI_MEMREGION(obj);
> +
> +    s->base = -1;
> +    object_property_add_uint64_ptr(obj, "base", &s->base,
> +                                   OBJ_PROP_FLAG_WRITE);
> +    object_property_set_description(obj, "base",
> +                                    "The base address of the domain memory region. If \"order\" is also specified, "
> +                                    "this property should be a 2 ^ order aligned 64 bit address");
> +
> +    s->order = -1;
> +    object_property_add_uint32_ptr(obj, "order", &s->order,
> +                                   OBJ_PROP_FLAG_WRITE);
> +    object_property_set_description(obj, "order",
> +                                    "The order of the domain memory region. This property should have a 32 bit value "
> +                                    "(i.e. one DT cell) in the range 3 <= order <= __riscv_xlen.");
> +
> +    s->mmio = false;
> +    object_property_add_bool(obj, "mmio", NULL, set_mmio);
> +    object_property_set_description(obj, "mmio",
> +                                    "A boolean flag representing whether the domain memory region is a "
> +                                    "memory-mapped I/O (MMIO) region.");
> +
> +    for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
> +        g_autofree char *propname = g_strdup_printf("device%i", i);
> +        object_property_add_str(obj, propname, NULL, set_device);
> +
> +        g_autofree char *description = g_strdup_printf(
> +                "Device %i (out of %i) for this memregion. This property should be a device tree path to the device.",
> +                i, OPENSBI_DOMAIN_MEMREGIONS_MAX);
> +        object_property_set_description(obj, propname, description);
> +    }
> +}
> +
> +static void opensbi_memregion_realize(DeviceState *ds, Error **errp)
> +{
> +    #if defined(TARGET_RISCV32)
> +    int xlen = 32;
> +    #elif defined(TARGET_RISCV64)
> +    int xlen = 64;
> +    #endif
> +
> +    OpenSBIMemregionState *s = OPENSBI_MEMREGION(ds);
> +
> +    if (s->base == -1) {
> +        error_setg(errp, "must specify base");
> +        return;
> +    }
> +
> +    if (s->order == -1) {
> +        error_setg(errp, "must specify order");
> +        return;
> +    }
> +
> +    /* Check order bounds */
> +    if (s->order < 3 || s->order > xlen) {
> +        error_setg(errp, "order must be between 3 and %d", xlen);
> +        return;
> +    }
> +
> +    /* Check base alignment */
> +    if (s->order < xlen && (s->base & (BIT(s->order) - 1))) {
> +        error_setg(errp, "base not aligned to order");
> +        return;
> +    }
> +}
> +
> +static void opensbi_memregion_class_init(ObjectClass *oc, void *opaque)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    dc->realize = opensbi_memregion_realize;
> +}
> +
> +static const TypeInfo opensbi_memregion_info = {
> +        .name = TYPE_OPENSBI_MEMREGION,
> +        .parent = TYPE_DEVICE,
> +        .instance_init = opensbi_memregion_instance_init,
> +        .instance_size = sizeof(OpenSBIDomainState),
> +        .class_init = opensbi_memregion_class_init
> +};
> +
> +/* OpenSBI Domains */
> +
> +static void set_sysreset_allowed(Object *obj, bool val, Error **err)
> +{
> +    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
> +    s->system_reset_allowed = val;
> +}
> +
> +static void set_suspend_allowed(Object *obj, bool val, Error **err)
> +{
> +    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
> +    s->system_suspend_allowed = val;
> +}
> +
> +static void set_assign(Object *obj, bool val, Error **err)
> +{
> +    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
> +    s->assign = val;
> +}
> +
> +static void set_possible_harts(Object *obj, const char *str, Error **err)
> +{
> +    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
> +    const char *firstcpu,  *firstcpu_end, *lastcpu;
> +
> +    firstcpu = str;
> +    if (qemu_strtoul(firstcpu, &firstcpu_end, 0,
> +                     &s->first_possible_hart) < 0) {
> +        error_setg(err, "could not convert firstcpu");
> +        return;
> +    }
> +
> +    lastcpu = qemu_strchrnul(str, '-');
> +    if (*lastcpu) {
> +        if (lastcpu != firstcpu_end) {
> +            error_setg(err, "could not separate firstcpu and lastcpu");
> +            return;
> +        }
> +
> +        lastcpu++;
> +        if (qemu_strtoul(lastcpu, NULL, 0,
> +                         &s->last_possible_hart) < 0) {
> +            error_setg(err, "could not convert lastcpu");
> +            return;
> +        }
> +    } else {
> +        s->last_possible_hart = s->first_possible_hart;
> +    }
> +}
> +
> +static void opensbi_domain_instance_init(Object *obj)
> +{
> +    int i;
> +    OpenSBIDomainState *s = OPENSBI_DOMAIN(obj);
> +
> +    s->boot_hart = VIRT_CPUS_MAX;
> +    object_property_add_uint32_ptr(obj, "boot-hart", &s->boot_hart,
> +                                   OBJ_PROP_FLAG_WRITE);
> +    object_property_set_description(obj, "boot-hart",
> +                                    "The HART booting the domain instance.");
> +
> +    s->first_possible_hart = -1;
> +    s->last_possible_hart = -1;
> +    object_property_add_str(obj, "possible-harts", NULL, set_possible_harts);
> +    object_property_set_description(obj, "possible-harts",
> +                                    "The contiguous list of CPUs for the domain instance, specified as firstcpu[-lastcpu]");
> +
> +    s->next_arg1 = -1;
> +    object_property_add_uint64_ptr(obj, "next-arg1", &s->next_arg1,
> +                                   OBJ_PROP_FLAG_WRITE);
> +    object_property_set_description(obj, "next-arg1",
> +                                    "The 64 bit next booting stage arg1 for the domain instance.");
> +
> +    s->next_addr = -1;
> +    object_property_add_uint64_ptr(obj, "next-addr", &s->next_addr,
> +                                   OBJ_PROP_FLAG_WRITE);
> +    object_property_set_description(obj, "next-addr",
> +                                    "The 64 bit next booting stage address for the domain instance.");
> +
> +    s->next_mode = -1;
> +    object_property_add_uint32_ptr(obj, "next-mode", &s->next_mode,
> +                                   OBJ_PROP_FLAG_WRITE);
> +    object_property_set_description(obj, "next-mode",
> +                                    "The 32 bit next booting stage mode for the domain instance.");
> +
> +    s->system_reset_allowed = false;
> +    object_property_add_bool(obj, "system-reset-allowed", NULL,
> +                             set_sysreset_allowed);
> +    object_property_set_description(obj, "system-reset-allowed",
> +                                    "Whether the domain instance is allowed to do system reset.");
> +
> +    s->system_suspend_allowed = false;
> +    object_property_add_bool(obj, "system-suspend-allowed", NULL,
> +                             set_suspend_allowed);
> +    object_property_set_description(obj, "system-suspend-allowed",
> +                                    "Whether the domain instance is allowed to do system suspend.");
> +
> +    for (i = 0; i < OPENSBI_DOMAIN_MEMREGIONS_MAX; i++) {
> +        s->regions[i] = NULL;
> +        g_autofree char *reg_propname = g_strdup_printf("region%i", i);
> +        object_property_add_link(obj, reg_propname, TYPE_OPENSBI_MEMREGION,
> +                                 (Object **) &s->regions[i],
> +                                 qdev_prop_allow_set_link_before_realize, 0);
> +
> +        g_autofree char *reg_description = g_strdup_printf(
> +                "Region %i (out of %i) for this domain.",
> +                i, OPENSBI_DOMAIN_MEMREGIONS_MAX);
> +        object_property_set_description(obj, reg_propname, reg_description);
> +
> +        s->region_perms[i] = 0;
> +        g_autofree char *perm_propname = g_strdup_printf("perms%i", i);
> +        object_property_add_uint32_ptr(obj, perm_propname, &s->region_perms[i],
> +                                       OBJ_PROP_FLAG_WRITE);
> +
> +        g_autofree char *perm_description = g_strdup_printf(
> +                "Permissions for region %i for this domain.", i);
> +        object_property_set_description(obj, perm_propname, perm_description);
> +    }
> +
> +    object_property_add_bool(obj, "assign", NULL, set_assign);
> +    object_property_set_description(obj, "assign",
> +                                    "Whether to assign this domain to its boot hart.");
> +}
> +
> +static void opensbi_domain_realize(DeviceState *ds, Error **errp)
> +{
> +    OpenSBIDomainState *s = OPENSBI_DOMAIN(ds);
> +
> +    if (s->boot_hart >= VIRT_CPUS_MAX) {
> +        error_setg(errp, "boot hart larger than maximum number of CPUs (%d)",
> +                 VIRT_CPUS_MAX);
> +        return;
> +    }
> +
> +    if (s->first_possible_hart == -1) {
> +        if (s->last_possible_hart != -1) {
> +            error_setg(errp,
> +                     "last possible hart set when first possible hart unset");
> +            return;
> +        }
> +    } else {
> +        if (s->first_possible_hart >= VIRT_CPUS_MAX) {
> +            error_setg(errp,
> +                     "first possible hart larger than maximum number of CPUs (%d)",
> +                     VIRT_CPUS_MAX);
> +            return;
> +        }
> +
> +        if (s->last_possible_hart != -1) {
> +            if (s->last_possible_hart < s->first_possible_hart) {
> +                error_setg(errp,
> +                         "last possible hart larger than first possible hart");
> +                return;
> +            }
> +
> +            if (s->last_possible_hart >= VIRT_CPUS_MAX) {
> +                error_setg(errp,
> +                         "last possible hart larger than maximum number of CPUS (%d)",
> +                         VIRT_CPUS_MAX);
> +                return;
> +            }
> +        }
> +    }
> +}
> +
> +static void opensbi_domain_class_init(ObjectClass *oc, void *opaque)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    dc->realize = opensbi_domain_realize;
> +}
> +
> +static const TypeInfo opensbi_domain_info = {
> +        .name = TYPE_OPENSBI_DOMAIN,
> +        .parent = TYPE_DEVICE,
> +        .instance_init = opensbi_domain_instance_init,
> +        .instance_size = sizeof(OpenSBIDomainState),
> +        .class_init = opensbi_domain_class_init
> +};
> +
> +static void opensbi_register_types(void)
> +{
> +    type_register_static(&opensbi_domain_info);
> +    type_register_static(&opensbi_memregion_info);
> +}
> +
> +type_init(opensbi_register_types)
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 9981e0f6c9..bb4bf3ce5b 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -55,6 +55,7 @@
>   #include "hw/acpi/aml-build.h"
>   #include "qapi/qapi-visit-common.h"
>   #include "hw/virtio/virtio-iommu.h"
> +#include "hw/riscv/opensbi_domain.h"
>   
>   /* KVM AIA only supports APLIC MSI. APLIC Wired is always emulated by QEMU. */
>   static bool virt_use_kvm_aia(RISCVVirtState *s)
> @@ -1051,6 +1052,8 @@ static void finalize_fdt(RISCVVirtState *s)
>       create_fdt_uart(s, virt_memmap, irq_mmio_phandle);
>   
>       create_fdt_rtc(s, virt_memmap, irq_mmio_phandle);
> +
> +    create_fdt_opensbi_domains(MACHINE(s));
>   }
>   
>   static void create_fdt(RISCVVirtState *s, const MemMapEntry *memmap)
> diff --git a/include/hw/riscv/opensbi_domain.h b/include/hw/riscv/opensbi_domain.h
> new file mode 100644
> index 0000000000..bcce16a609
> --- /dev/null
> +++ b/include/hw/riscv/opensbi_domain.h
> @@ -0,0 +1,50 @@
> +
> +#ifndef RISCV_DOMAIN_H
> +#define RISCV_DOMAIN_H
> +
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
> +#include "cpu.h"
> +
> +#define TYPE_OPENSBI_MEMREGION "opensbi-memregion"
> +OBJECT_DECLARE_SIMPLE_TYPE(OpenSBIMemregionState, OPENSBI_MEMREGION)
> +
> +#define OPENSBI_MEMREGION_DEVICES_MAX   16
> +
> +struct OpenSBIMemregionState {
> +    /* public */
> +    DeviceState parent_obj;
> +
> +    /* private */
> +    uint64_t base;
> +    uint32_t order;
> +    bool mmio;
> +    char *devices[OPENSBI_MEMREGION_DEVICES_MAX];
> +};
> +
> +#define TYPE_OPENSBI_DOMAIN "opensbi-domain"
> +OBJECT_DECLARE_SIMPLE_TYPE(OpenSBIDomainState, OPENSBI_DOMAIN)
> +
> +#define OPENSBI_DOMAIN_MEMREGIONS_MAX   16
> +
> +struct OpenSBIDomainState {
> +    /* public */
> +    DeviceState parent_obj;
> +
> +    /* private */
> +    OpenSBIMemregionState *regions[OPENSBI_DOMAIN_MEMREGIONS_MAX];
> +    unsigned int region_perms[OPENSBI_DOMAIN_MEMREGIONS_MAX];
> +    unsigned long first_possible_hart, last_possible_hart;
> +    unsigned int boot_hart;
> +    uint64_t next_arg1;
> +    uint64_t next_addr;
> +    uint32_t next_mode;
> +    bool system_reset_allowed;
> +    bool system_suspend_allowed;
> +
> +    bool assign;
> +};
> +
> +void create_fdt_opensbi_domains(MachineState *s);
> +
> +#endif /* RISCV_DOMAIN_H */

Re: [PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[Daniel Henrique Barboza]

On 8/5/24 6:04 PM, Gregor Haas wrote:
> This patch series adds support for specifying OpenSBI domains on the QEMU
> command line. A simple example of what this looks like is below, including
> mapping the board's UART into the secondary domain:
> 
> qemu-system-riscv64 -machine virt -bios fw_jump.bin -cpu max -smp 2 -m 4G -nographic \
>          -device opensbi-memregion,id=mem,base=0xBC000000,order=26,mmio=false \
>          -device opensbi-memregion,id=uart,base=0x10000000,order=12,mmio=true,device0="/soc/serial@10000000" \
>          -device opensbi-domain,id=domain,possible-harts=0-1,boot-hart=0x0,next-addr=0xBC000000,next-mode=1,region0=mem,perms0=0x3f,region1=uart,perms1=0x3f
> 
> As a result of the above configuration, QEMU will add the following subnodes to
> the device tree:
> 
> chosen {
>          opensbi-domains {
>                  compatible = "opensbi,domain,config";
> 
>                  domain {
>                          next-mode = <0x01>;
>                          next-addr = <0x00 0xbc000000>;
>                          boot-hart = <0x03>;
>                          regions = <0x8000 0x3f 0x8002 0x3f>;
>                          possible-harts = <0x03 0x01>;
>                          phandle = <0x8003>;
>                          compatible = "opensbi,domain,instance";
>                  };
> 
>                  uart {
>                          phandle = <0x8002>;
>                          devices = <0x1800000>;
>                          mmio;
>                          order = <0x0c>;
>                          base = <0x00 0x10000000>;
>                          compatible = "opensbi,domain,memregion";
>                  };
> 
>                  mem {
>                          phandle = <0x8000>;
>                          order = <0x1a>;
>                          base = <0x00 0xbc000000>;
>                          compatible = "opensbi,domain,memregion";
>                  };
>          };
> };
> 
> This results in OpenSBI output as below, where regions 01-03 are inherited from
> the root domain and regions 00 and 04 correspond to the user specified ones:
> 
> Domain1 Name              : domain
> Domain1 Boot HART         : 0
> Domain1 HARTs             : 0,1
> Domain1 Region00          : 0x0000000010000000-0x0000000010000fff M: (I,R,W,X) S/U: (R,W,X)
> Domain1 Region01          : 0x0000000002000000-0x000000000200ffff M: (I,R,W) S/U: ()
> Domain1 Region02          : 0x0000000080080000-0x000000008009ffff M: (R,W) S/U: ()
> Domain1 Region03          : 0x0000000080000000-0x000000008007ffff M: (R,X) S/U: ()
> Domain1 Region04          : 0x00000000bc000000-0x00000000bfffffff M: (R,W,X) S/U: (R,W,X)
> Domain1 Next Address      : 0x00000000bc000000
> Domain1 Next Arg1         : 0x0000000000000000
> Domain1 Next Mode         : S-mode
> Domain1 SysReset          : no
> Domain1 SysSuspend        : no

I believe we need OpenSBI patches for this output, don't we? If I try this example using stock
OpenSBI 1.5.1 from QEMU this happens:


OpenSBI v1.5.1
    ____                    _____ ____ _____
   / __ \                  / ____|  _ \_   _|
  | |  | |_ __   ___ _ __ | (___ | |_) || |
  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
  | |__| | |_) |  __/ | | |____) | |_) || |_
   \____/| .__/ \___|_| |_|_____/|____/_____|
         | |
         |_|

sbi_domain_finalize: platform domains_init() failed (error -3)
init_coldboot: domain finalize failed (error -3)
(--- hangs ---)

It's not a big deal or a blocker to have this merged in QEMU regardless, but it would be nice
to have this documented somewhere (perhaps a new doc file). This would prevent users from trying
to use the device without the proper support.

This can be done after this patch is queued though. Thanks,


Daniel


> 
> v3:
> - Addressed review comments from v2 by adding default values to new properties.
>    This results in concrete errors at QEMU configuration time if a mandatory
>    property (as mandated by the OpenSBI spec) is not provided.
> - Changed command line encoding for the possible-harts field from a CPU bitmask
>    (e.g. where bit X is set if CPU X is a possible hart) to a range format (e.g.
>    the possible harts should be CPUs X-Y, where Y >= X). This does constrain the
>    hart assignment to consecutive ranges of harts, but this constraint is also
>    present for other QEMU subsystems (such as NUMA).
> - Added create_fdt_one_device(), which is invoked when scanning the device tree
>    for a memregion's devices. This function allocates a phandle for a region's
>    device if one does not yet exist.
> 
> v2:
> - Addressed review comments from v1. Specifically, renamed domain.{c,h} ->
>    opensbi_domain.{c,h} to increase clarity of what these files do. Also, more
>    consistently use g_autofree for dynamically allocated variables
> - Added an "assign" flag to OpenSBIDomainState, which indicates whether to
>    assign the domain's boot hart to it at domain parsing time.
> 
> Gregor Haas (1):
>    Add support for generating OpenSBI domains in the device tree
> 
>   MAINTAINERS                       |   7 +
>   hw/riscv/Kconfig                  |   4 +
>   hw/riscv/meson.build              |   1 +
>   hw/riscv/opensbi_domain.c         | 542 ++++++++++++++++++++++++++++++
>   hw/riscv/virt.c                   |   3 +
>   include/hw/riscv/opensbi_domain.h |  50 +++
>   6 files changed, 607 insertions(+)
>   create mode 100644 hw/riscv/opensbi_domain.c
>   create mode 100644 include/hw/riscv/opensbi_domain.h
> 

Re: [PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[Gregor Haas]

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

Hi Daniel,

That's correct -- I believe this [1] patch on the OpenSBI mailing list
addresses
this issue. I am currently waiting for it to be reviewed.

Thanks,
Gregor

[1] http://lists.infradead.org/pipermail/opensbi/2024-August/007240.html

On Thu, Aug 22, 2024 at 2:49 PM Daniel Henrique Barboza <
dbarboza@ventanamicro.com> wrote:

>
>
> On 8/5/24 6:04 PM, Gregor Haas wrote:
> > This patch series adds support for specifying OpenSBI domains on the QEMU
> > command line. A simple example of what this looks like is below,
> including
> > mapping the board's UART into the secondary domain:
> >
> > qemu-system-riscv64 -machine virt -bios fw_jump.bin -cpu max -smp 2 -m
> 4G -nographic \
> >          -device
> opensbi-memregion,id=mem,base=0xBC000000,order=26,mmio=false \
> >          -device
> opensbi-memregion,id=uart,base=0x10000000,order=12,mmio=true,device0="/soc/serial@10000000"
> \
> >          -device
> opensbi-domain,id=domain,possible-harts=0-1,boot-hart=0x0,next-addr=0xBC000000,next-mode=1,region0=mem,perms0=0x3f,region1=uart,perms1=0x3f
> >
> > As a result of the above configuration, QEMU will add the following
> subnodes to
> > the device tree:
> >
> > chosen {
> >          opensbi-domains {
> >                  compatible = "opensbi,domain,config";
> >
> >                  domain {
> >                          next-mode = <0x01>;
> >                          next-addr = <0x00 0xbc000000>;
> >                          boot-hart = <0x03>;
> >                          regions = <0x8000 0x3f 0x8002 0x3f>;
> >                          possible-harts = <0x03 0x01>;
> >                          phandle = <0x8003>;
> >                          compatible = "opensbi,domain,instance";
> >                  };
> >
> >                  uart {
> >                          phandle = <0x8002>;
> >                          devices = <0x1800000>;
> >                          mmio;
> >                          order = <0x0c>;
> >                          base = <0x00 0x10000000>;
> >                          compatible = "opensbi,domain,memregion";
> >                  };
> >
> >                  mem {
> >                          phandle = <0x8000>;
> >                          order = <0x1a>;
> >                          base = <0x00 0xbc000000>;
> >                          compatible = "opensbi,domain,memregion";
> >                  };
> >          };
> > };
> >
> > This results in OpenSBI output as below, where regions 01-03 are
> inherited from
> > the root domain and regions 00 and 04 correspond to the user specified
> ones:
> >
> > Domain1 Name              : domain
> > Domain1 Boot HART         : 0
> > Domain1 HARTs             : 0,1
> > Domain1 Region00          : 0x0000000010000000-0x0000000010000fff M:
> (I,R,W,X) S/U: (R,W,X)
> > Domain1 Region01          : 0x0000000002000000-0x000000000200ffff M:
> (I,R,W) S/U: ()
> > Domain1 Region02          : 0x0000000080080000-0x000000008009ffff M:
> (R,W) S/U: ()
> > Domain1 Region03          : 0x0000000080000000-0x000000008007ffff M:
> (R,X) S/U: ()
> > Domain1 Region04          : 0x00000000bc000000-0x00000000bfffffff M:
> (R,W,X) S/U: (R,W,X)
> > Domain1 Next Address      : 0x00000000bc000000
> > Domain1 Next Arg1         : 0x0000000000000000
> > Domain1 Next Mode         : S-mode
> > Domain1 SysReset          : no
> > Domain1 SysSuspend        : no
>
> I believe we need OpenSBI patches for this output, don't we? If I try this
> example using stock
> OpenSBI 1.5.1 from QEMU this happens:
>
>
> OpenSBI v1.5.1
>     ____                    _____ ____ _____
>    / __ \                  / ____|  _ \_   _|
>   | |  | |_ __   ___ _ __ | (___ | |_) || |
>   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>   | |__| | |_) |  __/ | | |____) | |_) || |_
>    \____/| .__/ \___|_| |_|_____/|____/_____|
>          | |
>          |_|
>
> sbi_domain_finalize: platform domains_init() failed (error -3)
> init_coldboot: domain finalize failed (error -3)
> (--- hangs ---)
>
> It's not a big deal or a blocker to have this merged in QEMU regardless,
> but it would be nice
> to have this documented somewhere (perhaps a new doc file). This would
> prevent users from trying
> to use the device without the proper support.
>
> This can be done after this patch is queued though. Thanks,
>
>
> Daniel
>
>
> >
> > v3:
> > - Addressed review comments from v2 by adding default values to new
> properties.
> >    This results in concrete errors at QEMU configuration time if a
> mandatory
> >    property (as mandated by the OpenSBI spec) is not provided.
> > - Changed command line encoding for the possible-harts field from a CPU
> bitmask
> >    (e.g. where bit X is set if CPU X is a possible hart) to a range
> format (e.g.
> >    the possible harts should be CPUs X-Y, where Y >= X). This does
> constrain the
> >    hart assignment to consecutive ranges of harts, but this constraint
> is also
> >    present for other QEMU subsystems (such as NUMA).
> > - Added create_fdt_one_device(), which is invoked when scanning the
> device tree
> >    for a memregion's devices. This function allocates a phandle for a
> region's
> >    device if one does not yet exist.
> >
> > v2:
> > - Addressed review comments from v1. Specifically, renamed domain.{c,h}
> ->
> >    opensbi_domain.{c,h} to increase clarity of what these files do.
> Also, more
> >    consistently use g_autofree for dynamically allocated variables
> > - Added an "assign" flag to OpenSBIDomainState, which indicates whether
> to
> >    assign the domain's boot hart to it at domain parsing time.
> >
> > Gregor Haas (1):
> >    Add support for generating OpenSBI domains in the device tree
> >
> >   MAINTAINERS                       |   7 +
> >   hw/riscv/Kconfig                  |   4 +
> >   hw/riscv/meson.build              |   1 +
> >   hw/riscv/opensbi_domain.c         | 542 ++++++++++++++++++++++++++++++
> >   hw/riscv/virt.c                   |   3 +
> >   include/hw/riscv/opensbi_domain.h |  50 +++
> >   6 files changed, 607 insertions(+)
> >   create mode 100644 hw/riscv/opensbi_domain.c
> >   create mode 100644 include/hw/riscv/opensbi_domain.h
> >
>

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

Re: [PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[Alistair Francis]

On Tue, Aug 6, 2024 at 7:05 AM Gregor Haas <gregorhaas1997@gmail.com> wrote:
>
> This patch series adds support for specifying OpenSBI domains on the QEMU
> command line. A simple example of what this looks like is below, including
> mapping the board's UART into the secondary domain:

Thanks for the patch, sorry it took me so long to look into this

>
> qemu-system-riscv64 -machine virt -bios fw_jump.bin -cpu max -smp 2 -m 4G -nographic \
>         -device opensbi-memregion,id=mem,base=0xBC000000,order=26,mmio=false \
>         -device opensbi-memregion,id=uart,base=0x10000000,order=12,mmio=true,device0="/soc/serial@10000000" \
>         -device opensbi-domain,id=domain,possible-harts=0-1,boot-hart=0x0,next-addr=0xBC000000,next-mode=1,region0=mem,perms0=0x3f,region1=uart,perms1=0x3f

This will need documentation added under docs (probably under
docs/system/riscv) of how this should be used.

I'm not convinced this is something we want though. A user can dump
the QEMU DTB and edit it to support OpenSBI domains if they want.

My worry is that the command line method is complex for users to get
right and will be fragile and prone to breakage as parts of QEMU's DTB
changes.

Also, how are users supposed to know what options to use? Maybe some
documentation will help clear up how this should be used by users

>
> As a result of the above configuration, QEMU will add the following subnodes to
> the device tree:
>
> chosen {
>         opensbi-domains {
>                 compatible = "opensbi,domain,config";
>
>                 domain {
>                         next-mode = <0x01>;
>                         next-addr = <0x00 0xbc000000>;
>                         boot-hart = <0x03>;
>                         regions = <0x8000 0x3f 0x8002 0x3f>;
>                         possible-harts = <0x03 0x01>;
>                         phandle = <0x8003>;
>                         compatible = "opensbi,domain,instance";
>                 };
>
>                 uart {
>                         phandle = <0x8002>;
>                         devices = <0x1800000>;
>                         mmio;
>                         order = <0x0c>;
>                         base = <0x00 0x10000000>;
>                         compatible = "opensbi,domain,memregion";
>                 };
>
>                 mem {
>                         phandle = <0x8000>;
>                         order = <0x1a>;
>                         base = <0x00 0xbc000000>;
>                         compatible = "opensbi,domain,memregion";
>                 };
>         };
> };
>
> This results in OpenSBI output as below, where regions 01-03 are inherited from
> the root domain and regions 00 and 04 correspond to the user specified ones:
>
> Domain1 Name              : domain
> Domain1 Boot HART         : 0
> Domain1 HARTs             : 0,1
> Domain1 Region00          : 0x0000000010000000-0x0000000010000fff M: (I,R,W,X) S/U: (R,W,X)
> Domain1 Region01          : 0x0000000002000000-0x000000000200ffff M: (I,R,W) S/U: ()
> Domain1 Region02          : 0x0000000080080000-0x000000008009ffff M: (R,W) S/U: ()
> Domain1 Region03          : 0x0000000080000000-0x000000008007ffff M: (R,X) S/U: ()
> Domain1 Region04          : 0x00000000bc000000-0x00000000bfffffff M: (R,W,X) S/U: (R,W,X)
> Domain1 Next Address      : 0x00000000bc000000
> Domain1 Next Arg1         : 0x0000000000000000
> Domain1 Next Mode         : S-mode
> Domain1 SysReset          : no
> Domain1 SysSuspend        : no
>
> v3:
> - Addressed review comments from v2 by adding default values to new properties.
>   This results in concrete errors at QEMU configuration time if a mandatory
>   property (as mandated by the OpenSBI spec) is not provided.
> - Changed command line encoding for the possible-harts field from a CPU bitmask
>   (e.g. where bit X is set if CPU X is a possible hart) to a range format (e.g.
>   the possible harts should be CPUs X-Y, where Y >= X). This does constrain the
>   hart assignment to consecutive ranges of harts, but this constraint is also
>   present for other QEMU subsystems (such as NUMA).
> - Added create_fdt_one_device(), which is invoked when scanning the device tree
>   for a memregion's devices. This function allocates a phandle for a region's
>   device if one does not yet exist.
>
> v2:
> - Addressed review comments from v1. Specifically, renamed domain.{c,h} ->
>   opensbi_domain.{c,h} to increase clarity of what these files do. Also, more
>   consistently use g_autofree for dynamically allocated variables
> - Added an "assign" flag to OpenSBIDomainState, which indicates whether to
>   assign the domain's boot hart to it at domain parsing time.
>
> Gregor Haas (1):
>   Add support for generating OpenSBI domains in the device tree
>
>  MAINTAINERS                       |   7 +
>  hw/riscv/Kconfig                  |   4 +
>  hw/riscv/meson.build              |   1 +
>  hw/riscv/opensbi_domain.c         | 542 ++++++++++++++++++++++++++++++

There should be a license comment at the start of new files. Have a
look at other QEMU files for examples

Alistair

>  hw/riscv/virt.c                   |   3 +
>  include/hw/riscv/opensbi_domain.h |  50 +++
>  6 files changed, 607 insertions(+)
>  create mode 100644 hw/riscv/opensbi_domain.c
>  create mode 100644 include/hw/riscv/opensbi_domain.h
>
> --
> 2.45.2
>
>

Re: [PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[Gregor Haas]

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

Hi Alistair!

On Sun, Sep 8, 2024 at 8:27 PM Alistair Francis <alistair23@gmail.com>
wrote:

> On Tue, Aug 6, 2024 at 7:05 AM Gregor Haas <gregorhaas1997@gmail.com>
> wrote:
> >
> > This patch series adds support for specifying OpenSBI domains on the QEMU
> > command line. A simple example of what this looks like is below,
> including
> > mapping the board's UART into the secondary domain:
>
> Thanks for the patch, sorry it took me so long to look into this
>

No worries -- thanks for your review!


>
> >
> > qemu-system-riscv64 -machine virt -bios fw_jump.bin -cpu max -smp 2 -m
> 4G -nographic \
> >         -device
> opensbi-memregion,id=mem,base=0xBC000000,order=26,mmio=false \
> >         -device
> opensbi-memregion,id=uart,base=0x10000000,order=12,mmio=true,device0="/soc/serial@10000000"
> \
> >         -device
> opensbi-domain,id=domain,possible-harts=0-1,boot-hart=0x0,next-addr=0xBC000000,next-mode=1,region0=mem,perms0=0x3f,region1=uart,perms1=0x3f
>
> This will need documentation added under docs (probably under
> docs/system/riscv) of how this should be used.
>

I've just sent a v4 patch series which includes documentation! Please let
me know what you think.


> I'm not convinced this is something we want though. A user can dump
> the QEMU DTB and edit it to support OpenSBI domains if they want.
>
> My worry is that the command line method is complex for users to get
> right and will be fragile and prone to breakage as parts of QEMU's DTB
> changes.
>

I've found this patch series really useful for programmatically generating
test
fixtures in an isolation system I'm working on, which is built on top of
OpenSBI
domains. In that sense, I've found generating the correct flags is easier
rather
than manually editing or generating several dozen device tree files for each
test configuration.

I take your point that these flags are hard to get right, and there may be
more
user-friendly ways to do this. FWIW, I that this will only rarely break if
QEMU's
DTB changes -- the only part that really depends on QEMU's DTB (rather than
just adding new information to it) is the device-linking part for
memregions, and
it gives a loud, direct error if it cannot find the user-specified device.


> Also, how are users supposed to know what options to use? Maybe some
> documentation will help clear up how this should be used by users
>
> >
> > As a result of the above configuration, QEMU will add the following
> subnodes to
> > the device tree:
> >
> > chosen {
> >         opensbi-domains {
> >                 compatible = "opensbi,domain,config";
> >
> >                 domain {
> >                         next-mode = <0x01>;
> >                         next-addr = <0x00 0xbc000000>;
> >                         boot-hart = <0x03>;
> >                         regions = <0x8000 0x3f 0x8002 0x3f>;
> >                         possible-harts = <0x03 0x01>;
> >                         phandle = <0x8003>;
> >                         compatible = "opensbi,domain,instance";
> >                 };
> >
> >                 uart {
> >                         phandle = <0x8002>;
> >                         devices = <0x1800000>;
> >                         mmio;
> >                         order = <0x0c>;
> >                         base = <0x00 0x10000000>;
> >                         compatible = "opensbi,domain,memregion";
> >                 };
> >
> >                 mem {
> >                         phandle = <0x8000>;
> >                         order = <0x1a>;
> >                         base = <0x00 0xbc000000>;
> >                         compatible = "opensbi,domain,memregion";
> >                 };
> >         };
> > };
> >
> > This results in OpenSBI output as below, where regions 01-03 are
> inherited from
> > the root domain and regions 00 and 04 correspond to the user specified
> ones:
> >
> > Domain1 Name              : domain
> > Domain1 Boot HART         : 0
> > Domain1 HARTs             : 0,1
> > Domain1 Region00          : 0x0000000010000000-0x0000000010000fff M:
> (I,R,W,X) S/U: (R,W,X)
> > Domain1 Region01          : 0x0000000002000000-0x000000000200ffff M:
> (I,R,W) S/U: ()
> > Domain1 Region02          : 0x0000000080080000-0x000000008009ffff M:
> (R,W) S/U: ()
> > Domain1 Region03          : 0x0000000080000000-0x000000008007ffff M:
> (R,X) S/U: ()
> > Domain1 Region04          : 0x00000000bc000000-0x00000000bfffffff M:
> (R,W,X) S/U: (R,W,X)
> > Domain1 Next Address      : 0x00000000bc000000
> > Domain1 Next Arg1         : 0x0000000000000000
> > Domain1 Next Mode         : S-mode
> > Domain1 SysReset          : no
> > Domain1 SysSuspend        : no
> >
> > v3:
> > - Addressed review comments from v2 by adding default values to new
> properties.
> >   This results in concrete errors at QEMU configuration time if a
> mandatory
> >   property (as mandated by the OpenSBI spec) is not provided.
> > - Changed command line encoding for the possible-harts field from a CPU
> bitmask
> >   (e.g. where bit X is set if CPU X is a possible hart) to a range
> format (e.g.
> >   the possible harts should be CPUs X-Y, where Y >= X). This does
> constrain the
> >   hart assignment to consecutive ranges of harts, but this constraint is
> also
> >   present for other QEMU subsystems (such as NUMA).
> > - Added create_fdt_one_device(), which is invoked when scanning the
> device tree
> >   for a memregion's devices. This function allocates a phandle for a
> region's
> >   device if one does not yet exist.
> >
> > v2:
> > - Addressed review comments from v1. Specifically, renamed domain.{c,h}
> ->
> >   opensbi_domain.{c,h} to increase clarity of what these files do. Also,
> more
> >   consistently use g_autofree for dynamically allocated variables
> > - Added an "assign" flag to OpenSBIDomainState, which indicates whether
> to
> >   assign the domain's boot hart to it at domain parsing time.
> >
> > Gregor Haas (1):
> >   Add support for generating OpenSBI domains in the device tree
> >
> >  MAINTAINERS                       |   7 +
> >  hw/riscv/Kconfig                  |   4 +
> >  hw/riscv/meson.build              |   1 +
> >  hw/riscv/opensbi_domain.c         | 542 ++++++++++++++++++++++++++++++
>
> There should be a license comment at the start of new files. Have a
> look at other QEMU files for examples
>

I've included this in my v4 patch series as well!


> Alistair
>
> >  hw/riscv/virt.c                   |   3 +
> >  include/hw/riscv/opensbi_domain.h |  50 +++
> >  6 files changed, 607 insertions(+)
> >  create mode 100644 hw/riscv/opensbi_domain.c
> >  create mode 100644 include/hw/riscv/opensbi_domain.h
> >
> > --
> > 2.45.2
> >
> >
>

Thanks,
Gregor

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

Re: [PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[Alistair Francis]

On Wed, Sep 11, 2024 at 7:08 AM Gregor Haas <gregorhaas1997@gmail.com> wrote:
>
> Hi Alistair!
>
> On Sun, Sep 8, 2024 at 8:27 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, Aug 6, 2024 at 7:05 AM Gregor Haas <gregorhaas1997@gmail.com> wrote:
>> >
>> > This patch series adds support for specifying OpenSBI domains on the QEMU
>> > command line. A simple example of what this looks like is below, including
>> > mapping the board's UART into the secondary domain:
>>
>> Thanks for the patch, sorry it took me so long to look into this
>
>
> No worries -- thanks for your review!
>
>>
>>
>> >
>> > qemu-system-riscv64 -machine virt -bios fw_jump.bin -cpu max -smp 2 -m 4G -nographic \
>> >         -device opensbi-memregion,id=mem,base=0xBC000000,order=26,mmio=false \
>> >         -device opensbi-memregion,id=uart,base=0x10000000,order=12,mmio=true,device0="/soc/serial@10000000" \
>> >         -device opensbi-domain,id=domain,possible-harts=0-1,boot-hart=0x0,next-addr=0xBC000000,next-mode=1,region0=mem,perms0=0x3f,region1=uart,perms1=0x3f
>>
>> This will need documentation added under docs (probably under
>> docs/system/riscv) of how this should be used.
>
>
> I've just sent a v4 patch series which includes documentation! Please let
> me know what you think.
>
>>
>> I'm not convinced this is something we want though. A user can dump
>> the QEMU DTB and edit it to support OpenSBI domains if they want.
>>
>> My worry is that the command line method is complex for users to get
>> right and will be fragile and prone to breakage as parts of QEMU's DTB
>> changes.
>
>
> I've found this patch series really useful for programmatically generating test
> fixtures in an isolation system I'm working on, which is built on top of OpenSBI
> domains. In that sense, I've found generating the correct flags is easier rather
> than manually editing or generating several dozen device tree files for each
> test configuration.

That's fair

>
> I take your point that these flags are hard to get right, and there may be more
> user-friendly ways to do this. FWIW, I that this will only rarely break if QEMU's
> DTB changes -- the only part that really depends on QEMU's DTB (rather than
> just adding new information to it) is the device-linking part for memregions, and
> it gives a loud, direct error if it cannot find the user-specified device.

Maybe some unit tests would help here as well, to make sure we catch
the error when the change happens.

Alistair

Re: [PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[Andrew Jones]

On Mon, Sep 09, 2024 at 01:27:05PM GMT, Alistair Francis wrote:
> On Tue, Aug 6, 2024 at 7:05 AM Gregor Haas <gregorhaas1997@gmail.com> wrote:
> >
> > This patch series adds support for specifying OpenSBI domains on the QEMU
> > command line. A simple example of what this looks like is below, including
> > mapping the board's UART into the secondary domain:
> 
> Thanks for the patch, sorry it took me so long to look into this
> 
> >
> > qemu-system-riscv64 -machine virt -bios fw_jump.bin -cpu max -smp 2 -m 4G -nographic \
> >         -device opensbi-memregion,id=mem,base=0xBC000000,order=26,mmio=false \
> >         -device opensbi-memregion,id=uart,base=0x10000000,order=12,mmio=true,device0="/soc/serial@10000000" \
> >         -device opensbi-domain,id=domain,possible-harts=0-1,boot-hart=0x0,next-addr=0xBC000000,next-mode=1,region0=mem,perms0=0x3f,region1=uart,perms1=0x3f
> 
> This will need documentation added under docs (probably under
> docs/system/riscv) of how this should be used.
> 
> I'm not convinced this is something we want though. A user can dump
> the QEMU DTB and edit it to support OpenSBI domains if they want.
>

I also feel like this is just pushing the population of device tree
nodes from an editor of a .dts file to the QEMU command line. If some
generation is needed, then maybe we need a script, possibly one which
has the same command line inputs as proposed here. afaik, we haven't
typically taken patches which help overlay the generated devicetree
with additional nodes. For example, see [1] for one such proposal
and rejection.

[1] https://lore.kernel.org/all/20210926183410.256484-1-sjg@chromium.org/

Thanks,
drew

Re: [PATCH v3 0/1] Add support for generating OpenSBI domains in the device tree

[Gregor Haas]

Hi Drew,

On Tue, Sep 17, 2024 at 5:45 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Sep 09, 2024 at 01:27:05PM GMT, Alistair Francis wrote:
> > On Tue, Aug 6, 2024 at 7:05 AM Gregor Haas <gregorhaas1997@gmail.com> wrote:
> > >
> > > This patch series adds support for specifying OpenSBI domains on the QEMU
> > > command line. A simple example of what this looks like is below, including
> > > mapping the board's UART into the secondary domain:
> >
> > Thanks for the patch, sorry it took me so long to look into this
> >
> > >
> > > qemu-system-riscv64 -machine virt -bios fw_jump.bin -cpu max -smp 2 -m 4G -nographic \
> > >         -device opensbi-memregion,id=mem,base=0xBC000000,order=26,mmio=false \
> > >         -device opensbi-memregion,id=uart,base=0x10000000,order=12,mmio=true,device0="/soc/serial@10000000" \
> > >         -device opensbi-domain,id=domain,possible-harts=0-1,boot-hart=0x0,next-addr=0xBC000000,next-mode=1,region0=mem,perms0=0x3f,region1=uart,perms1=0x3f
> >
> > This will need documentation added under docs (probably under
> > docs/system/riscv) of how this should be used.
> >
> > I'm not convinced this is something we want though. A user can dump
> > the QEMU DTB and edit it to support OpenSBI domains if they want.
> >
>
> I also feel like this is just pushing the population of device tree
> nodes from an editor of a .dts file to the QEMU command line. If some
> generation is needed, then maybe we need a script, possibly one which
> has the same command line inputs as proposed here. afaik, we haven't
> typically taken patches which help overlay the generated devicetree
> with additional nodes. For example, see [1] for one such proposal
> and rejection.
>
> [1] https://lore.kernel.org/all/20210926183410.256484-1-sjg@chromium.org/
> Thanks,
> drew

Thanks for the link! After reading through that chain, I can see that there is
typically high resistance to guest-specific device tree patches. As such, I'll
probably abandon this patch series rather than doing more work to clean it up.
I do wonder why there is so much resistance to these types of changes when the
need for them arises so often in various boot firmwares.

Thanks,
Gregor