[RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[Alex Bennée]

Hi,

This series adds the ability to append FDT data for blobs loaded by
the generic loader. My principle use-case was to be able to directly
boot Xen with a kernel image which avoided having to:

  - get the kernel image into my system image
  - deal with bugs in FDT munging by -bios firmware and/or grub

as such this currently a developer hack that makes *my* life easy and
is thus presented as an RFC for discussion. While I've tested it with
Xen I'm sure the approach would be applicable to other hypervisors or
firmware which expect to consume FDT data pointing at various blobs.

An example command line that launches this is (magic from -kernel):

  ./aarch64-softmmu/qemu-system-aarch64 -cpu cortex-a57 \
    -machine type=virt,virtualization=on -display none \
    -serial mon:stdio \
    -netdev user,id=unet,hostfwd=tcp::2222-:22 \
    -device virtio-net-pci,netdev=unet,id=virt-net,disable-legacy=on \
    -device virtio-scsi-pci,id=virt-scsi,disable-legacy=on \
    -blockdev driver=raw,node-name=hd,discard=unmap,file.driver=host_device,file.filename=/dev/zen-disk/debian-buster-arm64 \
    -device scsi-hd,drive=hd,id=virt-scsi-hd \
    -smp 4 -m 4096 \
    -kernel ~/lsrc/xen.git/xen/xen \
    -append "dom0_mem=1G,max:1G loglvl=all guest_loglvl=all" \
    -device loader,addr=0x47000000,file=$HOME/lsrc/linux.git/builds/arm64/arch/arm64/boot/Image,len-fdt-compat=2,fdt-compat[0]='multiboot,,module',fdt-compat[1]='multiboot,,kernel',fdt-bootargs="root=/dev/sda2 ro console=hvc0 earlyprintk=xen"

So a couple of choices I've made doing this:

Promoting *fdt to MachineState

This seemed the simplest approach to making the fdt available to the
global state, especially as MachineState already has a *dtb pointer.
I've converted the ARM virt machine and a RISCV machine but if this is
acceptable I can convert the others.

"Polluting" the generic loader arguments

This was mainly out of convenience as the loader already knows the
size of the blob being loaded. However you could certainly argue it
makes sense to have a more generic "FDT expander" virtual device that
could for example query the QOM model somehow to find the details it
needs.

FDT isn't the only way of passing system information up the boot
chain. We could reasonably want to do a similar thing with ACPI which
is what should be being used on SBSA like devices to communicate with
the hypervisor.

Also relying on ,, in the QemuOpt parser is ugly. It might be worth
having better quoting support if I press on with this.

So what do people think? Something worth developing?


Alex Bennée (4):
  hw/board: promote fdt from ARM VirtMachineState to MachineState
  hw/riscv: migrate fdt field to generic MachineState
  device_tree: add qemu_fdt_setprop_string_array helper
  generic_loader: allow the insertion of /chosen/module stanzas

 include/hw/arm/virt.h            |   1 -
 include/hw/boards.h              |   1 +
 include/hw/core/generic-loader.h |   4 +
 include/hw/riscv/virt.h          |   1 -
 include/sysemu/device_tree.h     |  17 ++
 device_tree.c                    |  26 +++
 hw/arm/virt.c                    | 322 ++++++++++++++++---------------
 hw/core/generic-loader.c         |  42 ++++
 hw/riscv/virt.c                  |  18 +-
 9 files changed, 268 insertions(+), 164 deletions(-)

-- 
2.20.1

[RFC PATCH 1/4] hw/board: promote fdt from ARM VirtMachineState to MachineState

[Alex Bennée]

The use of FDT's is quite common across our various platforms. To
allow the generic loader to tweak it we need to make it available in
the generic state. This creates the field and migrates the initial
user to use the generic field. Other boards will be updated in later
patches.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/arm/virt.h |   1 -
 include/hw/boards.h   |   1 +
 hw/arm/virt.c         | 322 ++++++++++++++++++++++--------------------
 3 files changed, 170 insertions(+), 154 deletions(-)

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index aad6d69841..648e5d6791 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -152,7 +152,6 @@ struct VirtMachineState {
     char *pciehb_nodename;
     const int *irqmap;
     int smp_cpus;
-    void *fdt;
     int fdt_size;
     uint32_t clock_phandle;
     uint32_t gic_phandle;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index bf53e8a16e..4da71decf2 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -257,6 +257,7 @@ struct MachineState {
 
     /*< public >*/
 
+    void *fdt;
     char *dtb;
     char *dumpdtb;
     int phandle_start;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e465a988d6..1ca04c597e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -216,14 +216,14 @@ static bool cpu_type_valid(const char *cpu)
     return false;
 }
 
-static void create_kaslr_seed(VirtMachineState *vms, const char *node)
+static void create_kaslr_seed(MachineState *ms, const char *node)
 {
     uint64_t seed;
 
     if (qemu_guest_getrandom(&seed, sizeof(seed), NULL)) {
         return;
     }
-    qemu_fdt_setprop_u64(vms->fdt, node, "kaslr-seed", seed);
+    qemu_fdt_setprop_u64(ms->fdt, node, "kaslr-seed", seed);
 }
 
 static void create_fdt(VirtMachineState *vms)
@@ -237,7 +237,7 @@ static void create_fdt(VirtMachineState *vms)
         exit(1);
     }
 
-    vms->fdt = fdt;
+    ms->fdt = fdt;
 
     /* Header */
     qemu_fdt_setprop_string(fdt, "/", "compatible", "linux,dummy-virt");
@@ -246,11 +246,11 @@ static void create_fdt(VirtMachineState *vms)
 
     /* /chosen must exist for load_dtb to fill in necessary properties later */
     qemu_fdt_add_subnode(fdt, "/chosen");
-    create_kaslr_seed(vms, "/chosen");
+    create_kaslr_seed(ms, "/chosen");
 
     if (vms->secure) {
         qemu_fdt_add_subnode(fdt, "/secure-chosen");
-        create_kaslr_seed(vms, "/secure-chosen");
+        create_kaslr_seed(ms, "/secure-chosen");
     }
 
     /* Clock node, for the benefit of the UART. The kernel device tree
@@ -314,6 +314,7 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
     ARMCPU *armcpu;
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
     uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
+    MachineState *ms = MACHINE(vms);
 
     if (vmc->claim_edge_triggered_timers) {
         irqflags = GIC_FDT_IRQ_FLAGS_EDGE_LO_HI;
@@ -325,19 +326,19 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
                              (1 << vms->smp_cpus) - 1);
     }
 
-    qemu_fdt_add_subnode(vms->fdt, "/timer");
+    qemu_fdt_add_subnode(ms->fdt, "/timer");
 
     armcpu = ARM_CPU(qemu_get_cpu(0));
     if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
         const char compat[] = "arm,armv8-timer\0arm,armv7-timer";
-        qemu_fdt_setprop(vms->fdt, "/timer", "compatible",
+        qemu_fdt_setprop(ms->fdt, "/timer", "compatible",
                          compat, sizeof(compat));
     } else {
-        qemu_fdt_setprop_string(vms->fdt, "/timer", "compatible",
+        qemu_fdt_setprop_string(ms->fdt, "/timer", "compatible",
                                 "arm,armv7-timer");
     }
-    qemu_fdt_setprop(vms->fdt, "/timer", "always-on", NULL, 0);
-    qemu_fdt_setprop_cells(vms->fdt, "/timer", "interrupts",
+    qemu_fdt_setprop(ms->fdt, "/timer", "always-on", NULL, 0);
+    qemu_fdt_setprop_cells(ms->fdt, "/timer", "interrupts",
                        GIC_FDT_IRQ_TYPE_PPI, ARCH_TIMER_S_EL1_IRQ, irqflags,
                        GIC_FDT_IRQ_TYPE_PPI, ARCH_TIMER_NS_EL1_IRQ, irqflags,
                        GIC_FDT_IRQ_TYPE_PPI, ARCH_TIMER_VIRT_IRQ, irqflags,
@@ -372,36 +373,36 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
         }
     }
 
-    qemu_fdt_add_subnode(vms->fdt, "/cpus");
-    qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#address-cells", addr_cells);
-    qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#size-cells", 0x0);
+    qemu_fdt_add_subnode(ms->fdt, "/cpus");
+    qemu_fdt_setprop_cell(ms->fdt, "/cpus", "#address-cells", addr_cells);
+    qemu_fdt_setprop_cell(ms->fdt, "/cpus", "#size-cells", 0x0);
 
     for (cpu = vms->smp_cpus - 1; cpu >= 0; cpu--) {
         char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
         ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
         CPUState *cs = CPU(armcpu);
 
-        qemu_fdt_add_subnode(vms->fdt, nodename);
-        qemu_fdt_setprop_string(vms->fdt, nodename, "device_type", "cpu");
-        qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
+        qemu_fdt_add_subnode(ms->fdt, nodename);
+        qemu_fdt_setprop_string(ms->fdt, nodename, "device_type", "cpu");
+        qemu_fdt_setprop_string(ms->fdt, nodename, "compatible",
                                     armcpu->dtb_compatible);
 
         if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED
             && vms->smp_cpus > 1) {
-            qemu_fdt_setprop_string(vms->fdt, nodename,
+            qemu_fdt_setprop_string(ms->fdt, nodename,
                                         "enable-method", "psci");
         }
 
         if (addr_cells == 2) {
-            qemu_fdt_setprop_u64(vms->fdt, nodename, "reg",
+            qemu_fdt_setprop_u64(ms->fdt, nodename, "reg",
                                  armcpu->mp_affinity);
         } else {
-            qemu_fdt_setprop_cell(vms->fdt, nodename, "reg",
+            qemu_fdt_setprop_cell(ms->fdt, nodename, "reg",
                                   armcpu->mp_affinity);
         }
 
         if (ms->possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
-            qemu_fdt_setprop_cell(vms->fdt, nodename, "numa-node-id",
+            qemu_fdt_setprop_cell(ms->fdt, nodename, "numa-node-id",
                 ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
         }
 
@@ -412,71 +413,74 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
 static void fdt_add_its_gic_node(VirtMachineState *vms)
 {
     char *nodename;
+    MachineState *ms = MACHINE(vms);
 
-    vms->msi_phandle = qemu_fdt_alloc_phandle(vms->fdt);
+    vms->msi_phandle = qemu_fdt_alloc_phandle(ms->fdt);
     nodename = g_strdup_printf("/intc/its@%" PRIx64,
                                vms->memmap[VIRT_GIC_ITS].base);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
-    qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
+    qemu_fdt_add_subnode(ms->fdt, nodename);
+    qemu_fdt_setprop_string(ms->fdt, nodename, "compatible",
                             "arm,gic-v3-its");
-    qemu_fdt_setprop(vms->fdt, nodename, "msi-controller", NULL, 0);
-    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+    qemu_fdt_setprop(ms->fdt, nodename, "msi-controller", NULL, 0);
+    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, vms->memmap[VIRT_GIC_ITS].base,
                                  2, vms->memmap[VIRT_GIC_ITS].size);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", vms->msi_phandle);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle", vms->msi_phandle);
     g_free(nodename);
 }
 
 static void fdt_add_v2m_gic_node(VirtMachineState *vms)
 {
+    MachineState *ms = MACHINE(vms);
     char *nodename;
 
     nodename = g_strdup_printf("/intc/v2m@%" PRIx64,
                                vms->memmap[VIRT_GIC_V2M].base);
-    vms->msi_phandle = qemu_fdt_alloc_phandle(vms->fdt);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
-    qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
+    vms->msi_phandle = qemu_fdt_alloc_phandle(ms->fdt);
+    qemu_fdt_add_subnode(ms->fdt, nodename);
+    qemu_fdt_setprop_string(ms->fdt, nodename, "compatible",
                             "arm,gic-v2m-frame");
-    qemu_fdt_setprop(vms->fdt, nodename, "msi-controller", NULL, 0);
-    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+    qemu_fdt_setprop(ms->fdt, nodename, "msi-controller", NULL, 0);
+    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, vms->memmap[VIRT_GIC_V2M].base,
                                  2, vms->memmap[VIRT_GIC_V2M].size);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", vms->msi_phandle);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle", vms->msi_phandle);
     g_free(nodename);
 }
 
 static void fdt_add_gic_node(VirtMachineState *vms)
 {
+    MachineState *ms = MACHINE(vms);
     char *nodename;
 
-    vms->gic_phandle = qemu_fdt_alloc_phandle(vms->fdt);
-    qemu_fdt_setprop_cell(vms->fdt, "/", "interrupt-parent", vms->gic_phandle);
+    vms->gic_phandle = qemu_fdt_alloc_phandle(ms->fdt);
+    qemu_fdt_setprop_cell(ms->fdt, "/", "interrupt-parent", vms->gic_phandle);
 
     nodename = g_strdup_printf("/intc@%" PRIx64,
                                vms->memmap[VIRT_GIC_DIST].base);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "#interrupt-cells", 3);
-    qemu_fdt_setprop(vms->fdt, nodename, "interrupt-controller", NULL, 0);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 0x2);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 0x2);
-    qemu_fdt_setprop(vms->fdt, nodename, "ranges", NULL, 0);
+    qemu_fdt_add_subnode(ms->fdt, nodename);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#interrupt-cells", 3);
+    qemu_fdt_setprop(ms->fdt, nodename, "interrupt-controller", NULL, 0);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#address-cells", 0x2);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#size-cells", 0x2);
+    qemu_fdt_setprop(ms->fdt, nodename, "ranges", NULL, 0);
     if (vms->gic_version == VIRT_GIC_VERSION_3) {
         int nb_redist_regions = virt_gicv3_redist_region_count(vms);
 
-        qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
+        qemu_fdt_setprop_string(ms->fdt, nodename, "compatible",
                                 "arm,gic-v3");
 
-        qemu_fdt_setprop_cell(vms->fdt, nodename,
+        qemu_fdt_setprop_cell(ms->fdt, nodename,
                               "#redistributor-regions", nb_redist_regions);
 
         if (nb_redist_regions == 1) {
-            qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+            qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                          2, vms->memmap[VIRT_GIC_DIST].base,
                                          2, vms->memmap[VIRT_GIC_DIST].size,
                                          2, vms->memmap[VIRT_GIC_REDIST].base,
                                          2, vms->memmap[VIRT_GIC_REDIST].size);
         } else {
-            qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+            qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, vms->memmap[VIRT_GIC_DIST].base,
                                  2, vms->memmap[VIRT_GIC_DIST].size,
                                  2, vms->memmap[VIRT_GIC_REDIST].base,
@@ -486,22 +490,22 @@ static void fdt_add_gic_node(VirtMachineState *vms)
         }
 
         if (vms->virt) {
-            qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
+            qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
                                    GIC_FDT_IRQ_TYPE_PPI, ARCH_GIC_MAINT_IRQ,
                                    GIC_FDT_IRQ_FLAGS_LEVEL_HI);
         }
     } else {
         /* 'cortex-a15-gic' means 'GIC v2' */
-        qemu_fdt_setprop_string(vms->fdt, nodename, "compatible",
+        qemu_fdt_setprop_string(ms->fdt, nodename, "compatible",
                                 "arm,cortex-a15-gic");
         if (!vms->virt) {
-            qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+            qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                          2, vms->memmap[VIRT_GIC_DIST].base,
                                          2, vms->memmap[VIRT_GIC_DIST].size,
                                          2, vms->memmap[VIRT_GIC_CPU].base,
                                          2, vms->memmap[VIRT_GIC_CPU].size);
         } else {
-            qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+            qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                          2, vms->memmap[VIRT_GIC_DIST].base,
                                          2, vms->memmap[VIRT_GIC_DIST].size,
                                          2, vms->memmap[VIRT_GIC_CPU].base,
@@ -510,13 +514,13 @@ static void fdt_add_gic_node(VirtMachineState *vms)
                                          2, vms->memmap[VIRT_GIC_HYP].size,
                                          2, vms->memmap[VIRT_GIC_VCPU].base,
                                          2, vms->memmap[VIRT_GIC_VCPU].size);
-            qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
+            qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
                                    GIC_FDT_IRQ_TYPE_PPI, ARCH_GIC_MAINT_IRQ,
                                    GIC_FDT_IRQ_FLAGS_LEVEL_HI);
         }
     }
 
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", vms->gic_phandle);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle", vms->gic_phandle);
     g_free(nodename);
 }
 
@@ -524,6 +528,7 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
 {
     ARMCPU *armcpu = ARM_CPU(first_cpu);
     uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
+    MachineState *ms = MACHINE(vms);
 
     if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
         assert(!object_property_get_bool(OBJECT(armcpu), "pmu", NULL));
@@ -536,12 +541,12 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
                              (1 << vms->smp_cpus) - 1);
     }
 
-    qemu_fdt_add_subnode(vms->fdt, "/pmu");
+    qemu_fdt_add_subnode(ms->fdt, "/pmu");
     if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
         const char compat[] = "arm,armv8-pmuv3";
-        qemu_fdt_setprop(vms->fdt, "/pmu", "compatible",
+        qemu_fdt_setprop(ms->fdt, "/pmu", "compatible",
                          compat, sizeof(compat));
-        qemu_fdt_setprop_cells(vms->fdt, "/pmu", "interrupts",
+        qemu_fdt_setprop_cells(ms->fdt, "/pmu", "interrupts",
                                GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_PMU_IRQ, irqflags);
     }
 }
@@ -747,6 +752,7 @@ static void create_uart(const VirtMachineState *vms, int uart,
     const char clocknames[] = "uartclk\0apb_pclk";
     DeviceState *dev = qdev_new(TYPE_PL011);
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
+    MachineState *ms = MACHINE(vms);
 
     qdev_prop_set_chr(dev, "chardev", chr);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
@@ -755,28 +761,28 @@ static void create_uart(const VirtMachineState *vms, int uart,
     sysbus_connect_irq(s, 0, qdev_get_gpio_in(vms->gic, irq));
 
     nodename = g_strdup_printf("/pl011@%" PRIx64, base);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
+    qemu_fdt_add_subnode(ms->fdt, nodename);
     /* Note that we can't use setprop_string because of the embedded NUL */
-    qemu_fdt_setprop(vms->fdt, nodename, "compatible",
+    qemu_fdt_setprop(ms->fdt, nodename, "compatible",
                          compat, sizeof(compat));
-    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                      2, base, 2, size);
-    qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
+    qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
                                GIC_FDT_IRQ_TYPE_SPI, irq,
                                GIC_FDT_IRQ_FLAGS_LEVEL_HI);
-    qemu_fdt_setprop_cells(vms->fdt, nodename, "clocks",
+    qemu_fdt_setprop_cells(ms->fdt, nodename, "clocks",
                                vms->clock_phandle, vms->clock_phandle);
-    qemu_fdt_setprop(vms->fdt, nodename, "clock-names",
+    qemu_fdt_setprop(ms->fdt, nodename, "clock-names",
                          clocknames, sizeof(clocknames));
 
     if (uart == VIRT_UART) {
-        qemu_fdt_setprop_string(vms->fdt, "/chosen", "stdout-path", nodename);
+        qemu_fdt_setprop_string(ms->fdt, "/chosen", "stdout-path", nodename);
     } else {
         /* Mark as not usable by the normal world */
-        qemu_fdt_setprop_string(vms->fdt, nodename, "status", "disabled");
-        qemu_fdt_setprop_string(vms->fdt, nodename, "secure-status", "okay");
+        qemu_fdt_setprop_string(ms->fdt, nodename, "status", "disabled");
+        qemu_fdt_setprop_string(ms->fdt, nodename, "secure-status", "okay");
 
-        qemu_fdt_setprop_string(vms->fdt, "/secure-chosen", "stdout-path",
+        qemu_fdt_setprop_string(ms->fdt, "/secure-chosen", "stdout-path",
                                 nodename);
     }
 
@@ -790,19 +796,20 @@ static void create_rtc(const VirtMachineState *vms)
     hwaddr size = vms->memmap[VIRT_RTC].size;
     int irq = vms->irqmap[VIRT_RTC];
     const char compat[] = "arm,pl031\0arm,primecell";
+    MachineState *ms = MACHINE(vms);
 
     sysbus_create_simple("pl031", base, qdev_get_gpio_in(vms->gic, irq));
 
     nodename = g_strdup_printf("/pl031@%" PRIx64, base);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
-    qemu_fdt_setprop(vms->fdt, nodename, "compatible", compat, sizeof(compat));
-    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+    qemu_fdt_add_subnode(ms->fdt, nodename);
+    qemu_fdt_setprop(ms->fdt, nodename, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, base, 2, size);
-    qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
+    qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
                            GIC_FDT_IRQ_TYPE_SPI, irq,
                            GIC_FDT_IRQ_FLAGS_LEVEL_HI);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "clocks", vms->clock_phandle);
-    qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "clocks", vms->clock_phandle);
+    qemu_fdt_setprop_string(ms->fdt, nodename, "clock-names", "apb_pclk");
     g_free(nodename);
 }
 
@@ -827,38 +834,39 @@ static void create_gpio(const VirtMachineState *vms)
     hwaddr size = vms->memmap[VIRT_GPIO].size;
     int irq = vms->irqmap[VIRT_GPIO];
     const char compat[] = "arm,pl061\0arm,primecell";
+    MachineState *ms = MACHINE(vms);
 
     pl061_dev = sysbus_create_simple("pl061", base,
                                      qdev_get_gpio_in(vms->gic, irq));
 
-    uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
+    uint32_t phandle = qemu_fdt_alloc_phandle(ms->fdt);
     nodename = g_strdup_printf("/pl061@%" PRIx64, base);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
-    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+    qemu_fdt_add_subnode(ms->fdt, nodename);
+    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, base, 2, size);
-    qemu_fdt_setprop(vms->fdt, nodename, "compatible", compat, sizeof(compat));
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "#gpio-cells", 2);
-    qemu_fdt_setprop(vms->fdt, nodename, "gpio-controller", NULL, 0);
-    qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
+    qemu_fdt_setprop(ms->fdt, nodename, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#gpio-cells", 2);
+    qemu_fdt_setprop(ms->fdt, nodename, "gpio-controller", NULL, 0);
+    qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
                            GIC_FDT_IRQ_TYPE_SPI, irq,
                            GIC_FDT_IRQ_FLAGS_LEVEL_HI);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "clocks", vms->clock_phandle);
-    qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", phandle);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "clocks", vms->clock_phandle);
+    qemu_fdt_setprop_string(ms->fdt, nodename, "clock-names", "apb_pclk");
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "phandle", phandle);
 
     gpio_key_dev = sysbus_create_simple("gpio-key", -1,
                                         qdev_get_gpio_in(pl061_dev, 3));
-    qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
-    qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible", "gpio-keys");
-    qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
-    qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#address-cells", 1);
+    qemu_fdt_add_subnode(ms->fdt, "/gpio-keys");
+    qemu_fdt_setprop_string(ms->fdt, "/gpio-keys", "compatible", "gpio-keys");
+    qemu_fdt_setprop_cell(ms->fdt, "/gpio-keys", "#size-cells", 0);
+    qemu_fdt_setprop_cell(ms->fdt, "/gpio-keys", "#address-cells", 1);
 
-    qemu_fdt_add_subnode(vms->fdt, "/gpio-keys/poweroff");
-    qemu_fdt_setprop_string(vms->fdt, "/gpio-keys/poweroff",
+    qemu_fdt_add_subnode(ms->fdt, "/gpio-keys/poweroff");
+    qemu_fdt_setprop_string(ms->fdt, "/gpio-keys/poweroff",
                             "label", "GPIO Key Poweroff");
-    qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys/poweroff", "linux,code",
+    qemu_fdt_setprop_cell(ms->fdt, "/gpio-keys/poweroff", "linux,code",
                           KEY_POWER);
-    qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
+    qemu_fdt_setprop_cells(ms->fdt, "/gpio-keys/poweroff",
                            "gpios", phandle, 3, 0);
     g_free(nodename);
 }
@@ -867,6 +875,7 @@ static void create_virtio_devices(const VirtMachineState *vms)
 {
     int i;
     hwaddr size = vms->memmap[VIRT_MMIO].size;
+    MachineState *ms = MACHINE(vms);
 
     /* We create the transports in forwards order. Since qbus_realize()
      * prepends (not appends) new child buses, the incrementing loop below will
@@ -916,15 +925,15 @@ static void create_virtio_devices(const VirtMachineState *vms)
         hwaddr base = vms->memmap[VIRT_MMIO].base + i * size;
 
         nodename = g_strdup_printf("/virtio_mmio@%" PRIx64, base);
-        qemu_fdt_add_subnode(vms->fdt, nodename);
-        qemu_fdt_setprop_string(vms->fdt, nodename,
+        qemu_fdt_add_subnode(ms->fdt, nodename);
+        qemu_fdt_setprop_string(ms->fdt, nodename,
                                 "compatible", "virtio,mmio");
-        qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+        qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                      2, base, 2, size);
-        qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
+        qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
                                GIC_FDT_IRQ_TYPE_SPI, irq,
                                GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
-        qemu_fdt_setprop(vms->fdt, nodename, "dma-coherent", NULL, 0);
+        qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
         g_free(nodename);
     }
 }
@@ -1005,17 +1014,18 @@ static void virt_flash_fdt(VirtMachineState *vms,
 {
     hwaddr flashsize = vms->memmap[VIRT_FLASH].size / 2;
     hwaddr flashbase = vms->memmap[VIRT_FLASH].base;
+    MachineState *ms = MACHINE(vms);
     char *nodename;
 
     if (sysmem == secure_sysmem) {
         /* Report both flash devices as a single node in the DT */
         nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
-        qemu_fdt_add_subnode(vms->fdt, nodename);
-        qemu_fdt_setprop_string(vms->fdt, nodename, "compatible", "cfi-flash");
-        qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+        qemu_fdt_add_subnode(ms->fdt, nodename);
+        qemu_fdt_setprop_string(ms->fdt, nodename, "compatible", "cfi-flash");
+        qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                      2, flashbase, 2, flashsize,
                                      2, flashbase + flashsize, 2, flashsize);
-        qemu_fdt_setprop_cell(vms->fdt, nodename, "bank-width", 4);
+        qemu_fdt_setprop_cell(ms->fdt, nodename, "bank-width", 4);
         g_free(nodename);
     } else {
         /*
@@ -1023,21 +1033,21 @@ static void virt_flash_fdt(VirtMachineState *vms,
          * only visible to the secure world.
          */
         nodename = g_strdup_printf("/secflash@%" PRIx64, flashbase);
-        qemu_fdt_add_subnode(vms->fdt, nodename);
-        qemu_fdt_setprop_string(vms->fdt, nodename, "compatible", "cfi-flash");
-        qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+        qemu_fdt_add_subnode(ms->fdt, nodename);
+        qemu_fdt_setprop_string(ms->fdt, nodename, "compatible", "cfi-flash");
+        qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                      2, flashbase, 2, flashsize);
-        qemu_fdt_setprop_cell(vms->fdt, nodename, "bank-width", 4);
-        qemu_fdt_setprop_string(vms->fdt, nodename, "status", "disabled");
-        qemu_fdt_setprop_string(vms->fdt, nodename, "secure-status", "okay");
+        qemu_fdt_setprop_cell(ms->fdt, nodename, "bank-width", 4);
+        qemu_fdt_setprop_string(ms->fdt, nodename, "status", "disabled");
+        qemu_fdt_setprop_string(ms->fdt, nodename, "secure-status", "okay");
         g_free(nodename);
 
         nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
-        qemu_fdt_add_subnode(vms->fdt, nodename);
-        qemu_fdt_setprop_string(vms->fdt, nodename, "compatible", "cfi-flash");
-        qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+        qemu_fdt_add_subnode(ms->fdt, nodename);
+        qemu_fdt_setprop_string(ms->fdt, nodename, "compatible", "cfi-flash");
+        qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                      2, flashbase + flashsize, 2, flashsize);
-        qemu_fdt_setprop_cell(vms->fdt, nodename, "bank-width", 4);
+        qemu_fdt_setprop_cell(ms->fdt, nodename, "bank-width", 4);
         g_free(nodename);
     }
 }
@@ -1102,17 +1112,17 @@ static FWCfgState *create_fw_cfg(const VirtMachineState *vms, AddressSpace *as)
     fw_cfg_add_i16(fw_cfg, FW_CFG_NB_CPUS, (uint16_t)ms->smp.cpus);
 
     nodename = g_strdup_printf("/fw-cfg@%" PRIx64, base);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
-    qemu_fdt_setprop_string(vms->fdt, nodename,
+    qemu_fdt_add_subnode(ms->fdt, nodename);
+    qemu_fdt_setprop_string(ms->fdt, nodename,
                             "compatible", "qemu,fw-cfg-mmio");
-    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, base, 2, size);
-    qemu_fdt_setprop(vms->fdt, nodename, "dma-coherent", NULL, 0);
+    qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
     g_free(nodename);
     return fw_cfg;
 }
 
-static void create_pcie_irq_map(const VirtMachineState *vms,
+static void create_pcie_irq_map(const MachineState *ms,
                                 uint32_t gic_phandle,
                                 int first_irq, const char *nodename)
 {
@@ -1140,10 +1150,10 @@ static void create_pcie_irq_map(const VirtMachineState *vms,
         }
     }
 
-    qemu_fdt_setprop(vms->fdt, nodename, "interrupt-map",
+    qemu_fdt_setprop(ms->fdt, nodename, "interrupt-map",
                      full_irq_map, sizeof(full_irq_map));
 
-    qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupt-map-mask",
+    qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupt-map-mask",
                            0x1800, 0, 0, /* devfn (PCI_SLOT(3)) */
                            0x7           /* PCI irq */);
 }
@@ -1159,6 +1169,7 @@ static void create_smmu(const VirtMachineState *vms,
     hwaddr size = vms->memmap[VIRT_SMMU].size;
     const char irq_names[] = "eventq\0priq\0cmdq-sync\0gerror";
     DeviceState *dev;
+    MachineState *ms = MACHINE(vms);
 
     if (vms->iommu != VIRT_IOMMU_SMMUV3 || !vms->iommu_phandle) {
         return;
@@ -1176,26 +1187,26 @@ static void create_smmu(const VirtMachineState *vms,
     }
 
     node = g_strdup_printf("/smmuv3@%" PRIx64, base);
-    qemu_fdt_add_subnode(vms->fdt, node);
-    qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat));
-    qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg", 2, base, 2, size);
+    qemu_fdt_add_subnode(ms->fdt, node);
+    qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg", 2, base, 2, size);
 
-    qemu_fdt_setprop_cells(vms->fdt, node, "interrupts",
+    qemu_fdt_setprop_cells(ms->fdt, node, "interrupts",
             GIC_FDT_IRQ_TYPE_SPI, irq    , GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
             GIC_FDT_IRQ_TYPE_SPI, irq + 1, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
             GIC_FDT_IRQ_TYPE_SPI, irq + 2, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI,
             GIC_FDT_IRQ_TYPE_SPI, irq + 3, GIC_FDT_IRQ_FLAGS_EDGE_LO_HI);
 
-    qemu_fdt_setprop(vms->fdt, node, "interrupt-names", irq_names,
+    qemu_fdt_setprop(ms->fdt, node, "interrupt-names", irq_names,
                      sizeof(irq_names));
 
-    qemu_fdt_setprop_cell(vms->fdt, node, "clocks", vms->clock_phandle);
-    qemu_fdt_setprop_string(vms->fdt, node, "clock-names", "apb_pclk");
-    qemu_fdt_setprop(vms->fdt, node, "dma-coherent", NULL, 0);
+    qemu_fdt_setprop_cell(ms->fdt, node, "clocks", vms->clock_phandle);
+    qemu_fdt_setprop_string(ms->fdt, node, "clock-names", "apb_pclk");
+    qemu_fdt_setprop(ms->fdt, node, "dma-coherent", NULL, 0);
 
-    qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1);
+    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
 
-    qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms->iommu_phandle);
+    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms->iommu_phandle);
     g_free(node);
 }
 
@@ -1203,22 +1214,23 @@ static void create_virtio_iommu_dt_bindings(VirtMachineState *vms)
 {
     const char compat[] = "virtio,pci-iommu";
     uint16_t bdf = vms->virtio_iommu_bdf;
+    MachineState *ms = MACHINE(vms);
     char *node;
 
-    vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt);
+    vms->iommu_phandle = qemu_fdt_alloc_phandle(ms->fdt);
 
     node = g_strdup_printf("%s/virtio_iommu@%d", vms->pciehb_nodename, bdf);
-    qemu_fdt_add_subnode(vms->fdt, node);
-    qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat));
-    qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg",
+    qemu_fdt_add_subnode(ms->fdt, node);
+    qemu_fdt_setprop(ms->fdt, node, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_sized_cells(ms->fdt, node, "reg",
                                  1, bdf << 8, 1, 0, 1, 0,
                                  1, 0, 1, 0);
 
-    qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1);
-    qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms->iommu_phandle);
+    qemu_fdt_setprop_cell(ms->fdt, node, "#iommu-cells", 1);
+    qemu_fdt_setprop_cell(ms->fdt, node, "phandle", vms->iommu_phandle);
     g_free(node);
 
-    qemu_fdt_setprop_cells(vms->fdt, vms->pciehb_nodename, "iommu-map",
+    qemu_fdt_setprop_cells(ms->fdt, vms->pciehb_nodename, "iommu-map",
                            0x0, vms->iommu_phandle, 0x0, bdf,
                            bdf + 1, vms->iommu_phandle, bdf + 1, 0xffff - bdf);
 }
@@ -1243,6 +1255,7 @@ static void create_pcie(VirtMachineState *vms)
     char *nodename;
     int i, ecam_id;
     PCIHostState *pci;
+    MachineState *ms = MACHINE(vms);
 
     dev = qdev_new(TYPE_GPEX_HOST);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
@@ -1302,27 +1315,27 @@ static void create_pcie(VirtMachineState *vms)
     }
 
     nodename = vms->pciehb_nodename = g_strdup_printf("/pcie@%" PRIx64, base);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
-    qemu_fdt_setprop_string(vms->fdt, nodename,
+    qemu_fdt_add_subnode(ms->fdt, nodename);
+    qemu_fdt_setprop_string(ms->fdt, nodename,
                             "compatible", "pci-host-ecam-generic");
-    qemu_fdt_setprop_string(vms->fdt, nodename, "device_type", "pci");
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "#address-cells", 3);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "#size-cells", 2);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "linux,pci-domain", 0);
-    qemu_fdt_setprop_cells(vms->fdt, nodename, "bus-range", 0,
+    qemu_fdt_setprop_string(ms->fdt, nodename, "device_type", "pci");
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#address-cells", 3);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#size-cells", 2);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "linux,pci-domain", 0);
+    qemu_fdt_setprop_cells(ms->fdt, nodename, "bus-range", 0,
                            nr_pcie_buses - 1);
-    qemu_fdt_setprop(vms->fdt, nodename, "dma-coherent", NULL, 0);
+    qemu_fdt_setprop(ms->fdt, nodename, "dma-coherent", NULL, 0);
 
     if (vms->msi_phandle) {
-        qemu_fdt_setprop_cells(vms->fdt, nodename, "msi-parent",
+        qemu_fdt_setprop_cells(ms->fdt, nodename, "msi-parent",
                                vms->msi_phandle);
     }
 
-    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
+    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
                                  2, base_ecam, 2, size_ecam);
 
     if (vms->highmem) {
-        qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "ranges",
+        qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "ranges",
                                      1, FDT_PCI_RANGE_IOPORT, 2, 0,
                                      2, base_pio, 2, size_pio,
                                      1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
@@ -1331,23 +1344,23 @@ static void create_pcie(VirtMachineState *vms)
                                      2, base_mmio_high,
                                      2, base_mmio_high, 2, size_mmio_high);
     } else {
-        qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "ranges",
+        qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "ranges",
                                      1, FDT_PCI_RANGE_IOPORT, 2, 0,
                                      2, base_pio, 2, size_pio,
                                      1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
                                      2, base_mmio, 2, size_mmio);
     }
 
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "#interrupt-cells", 1);
-    create_pcie_irq_map(vms, vms->gic_phandle, irq, nodename);
+    qemu_fdt_setprop_cell(ms->fdt, nodename, "#interrupt-cells", 1);
+    create_pcie_irq_map(ms, vms->gic_phandle, irq, nodename);
 
     if (vms->iommu) {
-        vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt);
+        vms->iommu_phandle = qemu_fdt_alloc_phandle(ms->fdt);
 
         switch (vms->iommu) {
         case VIRT_IOMMU_SMMUV3:
             create_smmu(vms, pci->bus);
-            qemu_fdt_setprop_cells(vms->fdt, nodename, "iommu-map",
+            qemu_fdt_setprop_cells(ms->fdt, nodename, "iommu-map",
                                    0x0, vms->iommu_phandle, 0x0, 0x10000);
             break;
         default:
@@ -1399,17 +1412,18 @@ static void create_secure_ram(VirtMachineState *vms,
     char *nodename;
     hwaddr base = vms->memmap[VIRT_SECURE_MEM].base;
     hwaddr size = vms->memmap[VIRT_SECURE_MEM].size;
+    MachineState *ms = MACHINE(vms);
 
     memory_region_init_ram(secram, NULL, "virt.secure-ram", size,
                            &error_fatal);
     memory_region_add_subregion(secure_sysmem, base, secram);
 
     nodename = g_strdup_printf("/secram@%" PRIx64, base);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
-    qemu_fdt_setprop_string(vms->fdt, nodename, "device_type", "memory");
-    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg", 2, base, 2, size);
-    qemu_fdt_setprop_string(vms->fdt, nodename, "status", "disabled");
-    qemu_fdt_setprop_string(vms->fdt, nodename, "secure-status", "okay");
+    qemu_fdt_add_subnode(ms->fdt, nodename);
+    qemu_fdt_setprop_string(ms->fdt, nodename, "device_type", "memory");
+    qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", 2, base, 2, size);
+    qemu_fdt_setprop_string(ms->fdt, nodename, "status", "disabled");
+    qemu_fdt_setprop_string(ms->fdt, nodename, "secure-status", "okay");
 
     if (secure_tag_sysmem) {
         create_tag_ram(secure_tag_sysmem, base, size, "mach-virt.secure-tag");
@@ -1422,9 +1436,11 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
 {
     const VirtMachineState *board = container_of(binfo, VirtMachineState,
                                                  bootinfo);
+    MachineState *ms = MACHINE(board);
+
 
     *fdt_size = board->fdt_size;
-    return board->fdt;
+    return ms->fdt;
 }
 
 static void virt_build_smbios(VirtMachineState *vms)
@@ -1472,7 +1488,7 @@ void virt_machine_done(Notifier *notifier, void *data)
      * while qemu takes charge of the qom stuff.
      */
     if (info->dtb_filename == NULL) {
-        platform_bus_add_all_fdt_nodes(vms->fdt, "/intc",
+        platform_bus_add_all_fdt_nodes(ms->fdt, "/intc",
                                        vms->memmap[VIRT_PLATFORM_BUS].base,
                                        vms->memmap[VIRT_PLATFORM_BUS].size,
                                        vms->irqmap[VIRT_PLATFORM_BUS]);
-- 
2.20.1

[RFC PATCH 2/4] hw/riscv: migrate fdt field to generic MachineState

[Alex Bennée]

This is a mechanical change to make the fdt available through
MachineState.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/riscv/virt.h |  1 -
 hw/riscv/virt.c         | 18 +++++++++---------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index b4ed9a32eb..6505ae8d23 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -41,7 +41,6 @@ struct RISCVVirtState {
     DeviceState *plic[VIRT_SOCKETS_MAX];
     PFlashCFI01 *flash[2];
 
-    void *fdt;
     int fdt_size;
 };
 
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 41bd2f38ba..17c0706156 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -194,7 +194,7 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
     hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
 
-    fdt = s->fdt = create_device_tree(&s->fdt_size);
+    fdt = mc->fdt = create_device_tree(&s->fdt_size);
     if (!fdt) {
         error_report("create_device_tree() failed");
         exit(1);
@@ -434,12 +434,12 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     g_free(name);
 
     name = g_strdup_printf("/soc/flash@%" PRIx64, flashbase);
-    qemu_fdt_add_subnode(s->fdt, name);
-    qemu_fdt_setprop_string(s->fdt, name, "compatible", "cfi-flash");
-    qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
+    qemu_fdt_add_subnode(mc->fdt, name);
+    qemu_fdt_setprop_string(mc->fdt, name, "compatible", "cfi-flash");
+    qemu_fdt_setprop_sized_cells(mc->fdt, name, "reg",
                                  2, flashbase, 2, flashsize,
                                  2, flashbase + flashsize, 2, flashsize);
-    qemu_fdt_setprop_cell(s->fdt, name, "bank-width", 4);
+    qemu_fdt_setprop_cell(mc->fdt, name, "bank-width", 4);
     g_free(name);
 }
 
@@ -613,9 +613,9 @@ static void virt_machine_init(MachineState *machine)
             hwaddr end = riscv_load_initrd(machine->initrd_filename,
                                            machine->ram_size, kernel_entry,
                                            &start);
-            qemu_fdt_setprop_cell(s->fdt, "/chosen",
+            qemu_fdt_setprop_cell(machine->fdt, "/chosen",
                                   "linux,initrd-start", start);
-            qemu_fdt_setprop_cell(s->fdt, "/chosen", "linux,initrd-end",
+            qemu_fdt_setprop_cell(machine->fdt, "/chosen", "linux,initrd-end",
                                   end);
         }
     } else {
@@ -636,11 +636,11 @@ static void virt_machine_init(MachineState *machine)
 
     /* Compute the fdt load address in dram */
     fdt_load_addr = riscv_load_fdt(memmap[VIRT_DRAM].base,
-                                   machine->ram_size, s->fdt);
+                                   machine->ram_size, machine->fdt);
     /* load the reset vector */
     riscv_setup_rom_reset_vec(start_addr, virt_memmap[VIRT_MROM].base,
                               virt_memmap[VIRT_MROM].size, kernel_entry,
-                              fdt_load_addr, s->fdt);
+                              fdt_load_addr, machine->fdt);
 
     /* SiFive Test MMIO device */
     sifive_test_create(memmap[VIRT_TEST].base);
-- 
2.20.1

[RFC PATCH 3/4] device_tree: add qemu_fdt_setprop_string_array helper

[Alex Bennée]

A string array in device tree is simply a series of \0 terminated
strings next to each other. As libfdt doesn't support that directly
we need to build it ourselves.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/sysemu/device_tree.h | 17 +++++++++++++++++
 device_tree.c                | 26 ++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 982c89345f..8a2fe55622 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -70,6 +70,23 @@ int qemu_fdt_setprop_u64(void *fdt, const char *node_path,
                          const char *property, uint64_t val);
 int qemu_fdt_setprop_string(void *fdt, const char *node_path,
                             const char *property, const char *string);
+
+/**
+ * qemu_fdt_setprop_string_array: set a string array property
+ *
+ * @fdt: pointer to the dt blob
+ * @name: node name
+ * @prop: property array
+ * @array: pointer to an array of string pointers
+ * @len: length of array
+ *
+ * assigns a string array to a property. This function converts and
+ * array of strings to a sequential string with \0 separators before
+ * setting the property.
+ */
+int qemu_fdt_setprop_string_array(void *fdt, const char *node_path,
+                                  const char *prop, char **array, int len);
+
 int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
                              const char *property,
                              const char *target_node_path);
diff --git a/device_tree.c b/device_tree.c
index b335dae707..a19873316a 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -21,6 +21,7 @@
 #include "qemu/error-report.h"
 #include "qemu/option.h"
 #include "qemu/bswap.h"
+#include "qemu/cutils.h"
 #include "sysemu/device_tree.h"
 #include "sysemu/sysemu.h"
 #include "hw/loader.h"
@@ -397,6 +398,31 @@ int qemu_fdt_setprop_string(void *fdt, const char *node_path,
     return r;
 }
 
+/*
+ * libfdt doesn't allow us to add string arrays directly but they are
+ * test a series of null terminated strings with a length. We build
+ * the string up here so we can calculate the final length.
+ */
+int qemu_fdt_setprop_string_array(void *fdt, const char *node_path, const char *prop,
+                                  char **array, int len)
+{
+    int ret, i, total_len = 0;
+    char *str, *p;
+    for (i = 0; i < len; i++) {
+        total_len += strlen(array[i]) + 1;
+    }
+    p = str = g_malloc0(total_len);
+    for (i = 0; i < len; i++) {
+        int len = strlen(array[i]) + 1;
+        pstrcpy(p, len, array[i]);
+        p += len;
+    }
+
+    ret = qemu_fdt_setprop(fdt, node_path, prop, str, total_len);
+    g_free(str);
+    return ret;
+}
+
 const void *qemu_fdt_getprop(void *fdt, const char *node_path,
                              const char *property, int *lenp, Error **errp)
 {
-- 
2.20.1

[RFC PATCH 4/4] generic_loader: allow the insertion of /chosen/module stanzas

[Alex Bennée]

The /chosen FDT node is how the firmware indicates information about
the kernel to the loader code. In a full boot chain this would come
from something like a boot loader. However if we use the generic
loader to load blobs into RAM before launching a hypervisor for
example we can boot directly:

  $QEMU $ARGS  -kernel ~/xen.git/xen/xen \
    -append "dom0_mem=1G,max:1G loglvl=all guest_loglvl=all" \
    -device loader,addr=0x47000000,\
    file=Image,\
    len-fdt-compat=2,\
    fdt-compat[0]='multiboot,,module',\
    fdt-compat[1]='multiboot,,kernel',\
    fdt-bootargs="root=/dev/mapper/vg0-root ro console=hvc0 earlyprintk=xen"

Note the ,, escapes required for the command line parser.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/core/generic-loader.h |  4 +++
 hw/core/generic-loader.c         | 42 ++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/include/hw/core/generic-loader.h b/include/hw/core/generic-loader.h
index 19d87b39c8..98b0452430 100644
--- a/include/hw/core/generic-loader.h
+++ b/include/hw/core/generic-loader.h
@@ -39,6 +39,10 @@ struct GenericLoaderState {
     bool force_raw;
     bool data_be;
     bool set_pc;
+
+    char **fdt_compat;
+    uint32_t fdt_compat_count;
+    char *fdt_bootargs;
 };
 
 #define TYPE_GENERIC_LOADER "loader"
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index a242c076f6..8bd8a33e80 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -40,6 +40,8 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "hw/core/generic-loader.h"
+#include "sysemu/device_tree.h"
+#include "hw/boards.h"
 
 #define CPU_NONE 0xFFFFFFFF
 
@@ -61,6 +63,39 @@ static void generic_loader_reset(void *opaque)
     }
 }
 
+/*
+ * Insert some FDT nodes for the loaded blob.
+ */
+static void loader_insert_fdt(GenericLoaderState *s, int size, Error **errp)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+    void *fdt = machine->fdt;
+    g_autofree char *node = g_strdup_printf("/chosen/module@%#08lx", s->addr);
+    uint64_t reg_attr[2] = {cpu_to_be64(s->addr), cpu_to_be64(size)};
+
+    if (!fdt) {
+        error_setg(errp, "Cannot modify FDT fields if the machine has none");
+        return;
+    }
+
+    qemu_fdt_add_subnode(fdt, node);
+    qemu_fdt_setprop(fdt, node, "reg", &reg_attr, sizeof(reg_attr));
+
+    if (s->fdt_compat) {
+        if (qemu_fdt_setprop_string_array
+            (fdt, node, "compatible", s->fdt_compat, s->fdt_compat_count) < 0) {
+            error_setg(errp, "couldn't set %s/compatible", node);
+            return;
+        }
+    }
+
+    if (s->fdt_bootargs) {
+        if (qemu_fdt_setprop_string(fdt, node, "bootargs", s->fdt_bootargs) < 0) {
+            error_setg(errp, "couldn't set %s/bootargs", node);
+        }
+    }
+}
+
 static void generic_loader_realize(DeviceState *dev, Error **errp)
 {
     GenericLoaderState *s = GENERIC_LOADER(dev);
@@ -171,6 +206,10 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
     } else {
         s->data = cpu_to_le64(s->data);
     }
+
+    if (s->fdt_compat || s->fdt_bootargs) {
+        loader_insert_fdt(s, size, errp);
+    }
 }
 
 static void generic_loader_unrealize(DeviceState *dev)
@@ -186,6 +225,9 @@ static Property generic_loader_props[] = {
     DEFINE_PROP_UINT32("cpu-num", GenericLoaderState, cpu_num, CPU_NONE),
     DEFINE_PROP_BOOL("force-raw", GenericLoaderState, force_raw, false),
     DEFINE_PROP_STRING("file", GenericLoaderState, file),
+    DEFINE_PROP_ARRAY("fdt-compat", GenericLoaderState, fdt_compat_count,
+                      fdt_compat, qdev_prop_string, char *),
+    DEFINE_PROP_STRING("fdt-bootargs", GenericLoaderState, fdt_bootargs),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.20.1

Re: [RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201009170742.23695-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201009170742.23695-1-alex.bennee@linaro.org
Subject: [RFC PATCH  0/4] generic loader FDT support (for direct Xen boot)

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20201009170742.23695-1-alex.bennee@linaro.org -> patchew/20201009170742.23695-1-alex.bennee@linaro.org
Switched to a new branch 'test'
8fbccbe generic_loader: allow the insertion of /chosen/module stanzas
2baf07a device_tree: add qemu_fdt_setprop_string_array helper
dd6eea3 hw/riscv: migrate fdt field to generic MachineState
dd8ee86 hw/board: promote fdt from ARM VirtMachineState to MachineState

=== OUTPUT BEGIN ===
1/4 Checking commit dd8ee86f5cdf (hw/board: promote fdt from ARM VirtMachineState to MachineState)
2/4 Checking commit dd6eea3e6e34 (hw/riscv: migrate fdt field to generic MachineState)
3/4 Checking commit 2baf07a7abfc (device_tree: add qemu_fdt_setprop_string_array helper)
WARNING: line over 80 characters
#35: FILE: device_tree.c:406:
+int qemu_fdt_setprop_string_array(void *fdt, const char *node_path, const char *prop,

total: 0 errors, 1 warnings, 61 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/4 Checking commit 8fbccbec9628 (generic_loader: allow the insertion of /chosen/module stanzas)
ERROR: Don't use '#' flag of printf format ('%#') in format strings, use '0x' prefix instead
#51: FILE: hw/core/generic-loader.c:73:
+    g_autofree char *node = g_strdup_printf("/chosen/module@%#08lx", s->addr);

WARNING: line over 80 characters
#71: FILE: hw/core/generic-loader.c:93:
+        if (qemu_fdt_setprop_string(fdt, node, "bootargs", s->fdt_bootargs) < 0) {

total: 1 errors, 1 warnings, 76 lines checked

Patch 4/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201009170742.23695-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[Alistair Francis]

On Fri, Oct 9, 2020 at 10:07 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Hi,
>
> This series adds the ability to append FDT data for blobs loaded by
> the generic loader. My principle use-case was to be able to directly
> boot Xen with a kernel image which avoided having to:
>
>   - get the kernel image into my system image
>   - deal with bugs in FDT munging by -bios firmware and/or grub
>
> as such this currently a developer hack that makes *my* life easy and
> is thus presented as an RFC for discussion. While I've tested it with
> Xen I'm sure the approach would be applicable to other hypervisors or
> firmware which expect to consume FDT data pointing at various blobs.

An interesting idea. I think this comes up enough that it's worth
thinking about.

>
> An example command line that launches this is (magic from -kernel):
>
>   ./aarch64-softmmu/qemu-system-aarch64 -cpu cortex-a57 \
>     -machine type=virt,virtualization=on -display none \
>     -serial mon:stdio \
>     -netdev user,id=unet,hostfwd=tcp::2222-:22 \
>     -device virtio-net-pci,netdev=unet,id=virt-net,disable-legacy=on \
>     -device virtio-scsi-pci,id=virt-scsi,disable-legacy=on \
>     -blockdev driver=raw,node-name=hd,discard=unmap,file.driver=host_device,file.filename=/dev/zen-disk/debian-buster-arm64 \
>     -device scsi-hd,drive=hd,id=virt-scsi-hd \
>     -smp 4 -m 4096 \
>     -kernel ~/lsrc/xen.git/xen/xen \
>     -append "dom0_mem=1G,max:1G loglvl=all guest_loglvl=all" \
>     -device loader,addr=0x47000000,file=$HOME/lsrc/linux.git/builds/arm64/arch/arm64/boot/Image,len-fdt-compat=2,fdt-compat[0]='multiboot,,module',fdt-compat[1]='multiboot,,kernel',fdt-bootargs="root=/dev/sda2 ro console=hvc0 earlyprintk=xen"

You are loading the kernel `Image` file, but changing the device tree
that was generated by QEMU and is being loaded in memory. As a user
that is really confusing.

>
> So a couple of choices I've made doing this:
>
> Promoting *fdt to MachineState
>
> This seemed the simplest approach to making the fdt available to the
> global state, especially as MachineState already has a *dtb pointer.
> I've converted the ARM virt machine and a RISCV machine but if this is
> acceptable I can convert the others.

This seems fine to me.

>
> "Polluting" the generic loader arguments
>
> This was mainly out of convenience as the loader already knows the
> size of the blob being loaded. However you could certainly argue it
> makes sense to have a more generic "FDT expander" virtual device that
> could for example query the QOM model somehow to find the details it
> needs.

That seems like a better option. Why not have a generic way to modify
the device tree with a specific argument? It could either be -device
loader,file=fdt,... or -fdt ...

Alistair

>
> FDT isn't the only way of passing system information up the boot
> chain. We could reasonably want to do a similar thing with ACPI which
> is what should be being used on SBSA like devices to communicate with
> the hypervisor.
>
> Also relying on ,, in the QemuOpt parser is ugly. It might be worth
> having better quoting support if I press on with this.
>
> So what do people think? Something worth developing?
>
>
> Alex Bennée (4):
>   hw/board: promote fdt from ARM VirtMachineState to MachineState
>   hw/riscv: migrate fdt field to generic MachineState
>   device_tree: add qemu_fdt_setprop_string_array helper
>   generic_loader: allow the insertion of /chosen/module stanzas
>
>  include/hw/arm/virt.h            |   1 -
>  include/hw/boards.h              |   1 +
>  include/hw/core/generic-loader.h |   4 +
>  include/hw/riscv/virt.h          |   1 -
>  include/sysemu/device_tree.h     |  17 ++
>  device_tree.c                    |  26 +++
>  hw/arm/virt.c                    | 322 ++++++++++++++++---------------
>  hw/core/generic-loader.c         |  42 ++++
>  hw/riscv/virt.c                  |  18 +-
>  9 files changed, 268 insertions(+), 164 deletions(-)
>
> --
> 2.20.1
>
>

Re: [RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[Alex Bennée]

Alistair Francis <alistair23@gmail.com> writes:

> On Fri, Oct 9, 2020 at 10:07 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Hi,
>>
>> This series adds the ability to append FDT data for blobs loaded by
>> the generic loader. My principle use-case was to be able to directly
>> boot Xen with a kernel image which avoided having to:
>>
>>   - get the kernel image into my system image
>>   - deal with bugs in FDT munging by -bios firmware and/or grub
>>
>> as such this currently a developer hack that makes *my* life easy and
>> is thus presented as an RFC for discussion. While I've tested it with
>> Xen I'm sure the approach would be applicable to other hypervisors or
>> firmware which expect to consume FDT data pointing at various blobs.
>
> An interesting idea. I think this comes up enough that it's worth
> thinking about.
>
>>
>> An example command line that launches this is (magic from -kernel):
>>
>>   ./aarch64-softmmu/qemu-system-aarch64 -cpu cortex-a57 \
>>     -machine type=virt,virtualization=on -display none \
>>     -serial mon:stdio \
>>     -netdev user,id=unet,hostfwd=tcp::2222-:22 \
>>     -device virtio-net-pci,netdev=unet,id=virt-net,disable-legacy=on \
>>     -device virtio-scsi-pci,id=virt-scsi,disable-legacy=on \
>>     -blockdev driver=raw,node-name=hd,discard=unmap,file.driver=host_device,file.filename=/dev/zen-disk/debian-buster-arm64 \
>>     -device scsi-hd,drive=hd,id=virt-scsi-hd \
>>     -smp 4 -m 4096 \
>>     -kernel ~/lsrc/xen.git/xen/xen \
>>     -append "dom0_mem=1G,max:1G loglvl=all guest_loglvl=all" \
>>     -device loader,addr=0x47000000,file=$HOME/lsrc/linux.git/builds/arm64/arch/arm64/boot/Image,len-fdt-compat=2,fdt-compat[0]='multiboot,,module',fdt-compat[1]='multiboot,,kernel',fdt-bootargs="root=/dev/sda2 ro console=hvc0 earlyprintk=xen"
>
> You are loading the kernel `Image` file, but changing the device tree
> that was generated by QEMU and is being loaded in memory. As a user
> that is really confusing.

Well in this case the "kernel" is Xen which helpfully comes with a
kernel compatible header that the kernel loaded understand. The blob
could be any Dom0 kernel - it just happens to be a Linux kernel in this
case.

>
>>
>> So a couple of choices I've made doing this:
>>
>> Promoting *fdt to MachineState
>>
>> This seemed the simplest approach to making the fdt available to the
>> global state, especially as MachineState already has a *dtb pointer.
>> I've converted the ARM virt machine and a RISCV machine but if this is
>> acceptable I can convert the others.
>
> This seems fine to me.
>
>>
>> "Polluting" the generic loader arguments
>>
>> This was mainly out of convenience as the loader already knows the
>> size of the blob being loaded. However you could certainly argue it
>> makes sense to have a more generic "FDT expander" virtual device that
>> could for example query the QOM model somehow to find the details it
>> needs.
>
> That seems like a better option. Why not have a generic way to modify
> the device tree with a specific argument? It could either be -device
> loader,file=fdt,... or -fdt ...

Well it comes down to how much of a special case this is? Pretty much
all FDT (and ACPI for the matter) is basically down to the board level
models - and not all of them just the funky configurable ones. For other
board models we just expect the user to pass the FDT they got with their
kernel blob.

For modern VirtIO systems the only thing you really need to expose is
the root PCI bus because the rest of the devices are discover-able
there.

So the real question is are there any other -devices that we want to be
able to graft FDT entries on or is the generic loader enough of a
special case that we keep all the logic in there?

>
> Alistair
>
>>
>> FDT isn't the only way of passing system information up the boot
>> chain. We could reasonably want to do a similar thing with ACPI which
>> is what should be being used on SBSA like devices to communicate with
>> the hypervisor.
>>
>> Also relying on ,, in the QemuOpt parser is ugly. It might be worth
>> having better quoting support if I press on with this.
>>
>> So what do people think? Something worth developing?
>>
>>
>> Alex Bennée (4):
>>   hw/board: promote fdt from ARM VirtMachineState to MachineState
>>   hw/riscv: migrate fdt field to generic MachineState
>>   device_tree: add qemu_fdt_setprop_string_array helper
>>   generic_loader: allow the insertion of /chosen/module stanzas
>>
>>  include/hw/arm/virt.h            |   1 -
>>  include/hw/boards.h              |   1 +
>>  include/hw/core/generic-loader.h |   4 +
>>  include/hw/riscv/virt.h          |   1 -
>>  include/sysemu/device_tree.h     |  17 ++
>>  device_tree.c                    |  26 +++
>>  hw/arm/virt.c                    | 322 ++++++++++++++++---------------
>>  hw/core/generic-loader.c         |  42 ++++
>>  hw/riscv/virt.c                  |  18 +-
>>  9 files changed, 268 insertions(+), 164 deletions(-)
>>
>> --
>> 2.20.1
>>
>>


-- 
Alex Bennée

Re: [RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[Peter Maydell]

On Mon, 12 Oct 2020 at 17:19, Alex Bennée <alex.bennee@linaro.org> wrote:
> So the real question is are there any other -devices that we want to be
> able to graft FDT entries on or is the generic loader enough of a
> special case that we keep all the logic in there?

To my mind the point of the generic loader is exactly that
it is not a special case. It works exactly the same way
for every board, for every architecture. It shouldn't have
special case "this makes things work for the thing I want
to load on these boards that all have FDT and want a
particular change to it". We already have a loader that
has that kind of "do what I mean" behaviour (--kernel),
and I very much do not want the generic loader device to
go in that direction. Its whole advantage is that it is
not that, but is just a "load the file, nothing else,
no magic" operation.

thanks
-- PMM

Re: [RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 12 Oct 2020 at 17:19, Alex Bennée <alex.bennee@linaro.org> wrote:
>> So the real question is are there any other -devices that we want to be
>> able to graft FDT entries on or is the generic loader enough of a
>> special case that we keep all the logic in there?
>
> To my mind the point of the generic loader is exactly that
> it is not a special case. It works exactly the same way
> for every board, for every architecture. It shouldn't have
> special case "this makes things work for the thing I want
> to load on these boards that all have FDT and want a
> particular change to it". We already have a loader that
> has that kind of "do what I mean" behaviour (--kernel),
> and I very much do not want the generic loader device to
> go in that direction. Its whole advantage is that it is
> not that, but is just a "load the file, nothing else,
> no magic" operation.

So should we introduce a sub-classed -device guest-loader which behaves
like generic loader except that it is also aware of platform data
issues?

The other option would be to add the logic to each board that supports
dynamic platform data. It would keep the problem bounded but also lead
to a fair amount of duplicated boiler-plate.

>
> thanks
> -- PMM


-- 
Alex Bennée

Re: [RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[Edgar E. Iglesias]

On Mon, Oct 12, 2020 at 05:02:57PM +0100, Alex Bennée wrote:
> 
> Alistair Francis <alistair23@gmail.com> writes:
> 
> > On Fri, Oct 9, 2020 at 10:07 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> Hi,
> >>
> >> This series adds the ability to append FDT data for blobs loaded by
> >> the generic loader. My principle use-case was to be able to directly
> >> boot Xen with a kernel image which avoided having to:
> >>
> >>   - get the kernel image into my system image
> >>   - deal with bugs in FDT munging by -bios firmware and/or grub
> >>
> >> as such this currently a developer hack that makes *my* life easy and
> >> is thus presented as an RFC for discussion. While I've tested it with
> >> Xen I'm sure the approach would be applicable to other hypervisors or
> >> firmware which expect to consume FDT data pointing at various blobs.
> >
> > An interesting idea. I think this comes up enough that it's worth
> > thinking about.
> >
> >>
> >> An example command line that launches this is (magic from -kernel):
> >>
> >>   ./aarch64-softmmu/qemu-system-aarch64 -cpu cortex-a57 \
> >>     -machine type=virt,virtualization=on -display none \
> >>     -serial mon:stdio \
> >>     -netdev user,id=unet,hostfwd=tcp::2222-:22 \
> >>     -device virtio-net-pci,netdev=unet,id=virt-net,disable-legacy=on \
> >>     -device virtio-scsi-pci,id=virt-scsi,disable-legacy=on \
> >>     -blockdev driver=raw,node-name=hd,discard=unmap,file.driver=host_device,file.filename=/dev/zen-disk/debian-buster-arm64 \
> >>     -device scsi-hd,drive=hd,id=virt-scsi-hd \
> >>     -smp 4 -m 4096 \
> >>     -kernel ~/lsrc/xen.git/xen/xen \
> >>     -append "dom0_mem=1G,max:1G loglvl=all guest_loglvl=all" \
> >>     -device loader,addr=0x47000000,file=$HOME/lsrc/linux.git/builds/arm64/arch/arm64/boot/Image,len-fdt-compat=2,fdt-compat[0]='multiboot,,module',fdt-compat[1]='multiboot,,kernel',fdt-bootargs="root=/dev/sda2 ro console=hvc0 earlyprintk=xen"
> >
> > You are loading the kernel `Image` file, but changing the device tree
> > that was generated by QEMU and is being loaded in memory. As a user
> > that is really confusing.
> 
> Well in this case the "kernel" is Xen which helpfully comes with a
> kernel compatible header that the kernel loaded understand. The blob
> could be any Dom0 kernel - it just happens to be a Linux kernel in this
> case.
> 
> >
> >>
> >> So a couple of choices I've made doing this:
> >>
> >> Promoting *fdt to MachineState
> >>
> >> This seemed the simplest approach to making the fdt available to the
> >> global state, especially as MachineState already has a *dtb pointer.
> >> I've converted the ARM virt machine and a RISCV machine but if this is
> >> acceptable I can convert the others.
> >
> > This seems fine to me.
> >
> >>
> >> "Polluting" the generic loader arguments
> >>
> >> This was mainly out of convenience as the loader already knows the
> >> size of the blob being loaded. However you could certainly argue it
> >> makes sense to have a more generic "FDT expander" virtual device that
> >> could for example query the QOM model somehow to find the details it
> >> needs.
> >
> > That seems like a better option. Why not have a generic way to modify
> > the device tree with a specific argument? It could either be -device
> > loader,file=fdt,... or -fdt ...
> 
> Well it comes down to how much of a special case this is? Pretty much
> all FDT (and ACPI for the matter) is basically down to the board level
> models - and not all of them just the funky configurable ones. For other
> board models we just expect the user to pass the FDT they got with their
> kernel blob.
> 
> For modern VirtIO systems the only thing you really need to expose is
> the root PCI bus because the rest of the devices are discover-able
> there.
> 
> So the real question is are there any other -devices that we want to be
> able to graft FDT entries on or is the generic loader enough of a
> special case that we keep all the logic in there?
>

Hi,

Another option is to allow the user to pass along a DTB overlay with the
generic loader option (or with a separate option as Alistair suggested).
With overlways we wouldn't need all the command-line options to enable
construction of dtb fragments, it would be more or less transparent to
QEMU. There may be limitations with the overlay flows that I'm not aware
of though...

Cheers,
Edgar

Re: [RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[Alex Bennée]

Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:

> On Mon, Oct 12, 2020 at 05:02:57PM +0100, Alex Bennée wrote:
>> 
>> Alistair Francis <alistair23@gmail.com> writes:
>> 
>> > On Fri, Oct 9, 2020 at 10:07 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>> >>
>> >> Hi,
>> >>
>> >> This series adds the ability to append FDT data for blobs loaded by
>> >> the generic loader. My principle use-case was to be able to directly
>> >> boot Xen with a kernel image which avoided having to:
>> >>
>> >>   - get the kernel image into my system image
>> >>   - deal with bugs in FDT munging by -bios firmware and/or grub
<snip>
>> >> "Polluting" the generic loader arguments
>> >>
>> >> This was mainly out of convenience as the loader already knows the
>> >> size of the blob being loaded. However you could certainly argue it
>> >> makes sense to have a more generic "FDT expander" virtual device that
>> >> could for example query the QOM model somehow to find the details it
>> >> needs.
>> >
>> > That seems like a better option. Why not have a generic way to modify
>> > the device tree with a specific argument? It could either be -device
>> > loader,file=fdt,... or -fdt ...
>> 
>> Well it comes down to how much of a special case this is? Pretty much
>> all FDT (and ACPI for the matter) is basically down to the board level
>> models - and not all of them just the funky configurable ones. For other
>> board models we just expect the user to pass the FDT they got with their
>> kernel blob.
>> 
>> For modern VirtIO systems the only thing you really need to expose is
>> the root PCI bus because the rest of the devices are discover-able
>> there.
>> 
>> So the real question is are there any other -devices that we want to be
>> able to graft FDT entries on or is the generic loader enough of a
>> special case that we keep all the logic in there?
>>
>
> Hi,
>
> Another option is to allow the user to pass along a DTB overlay with the
> generic loader option (or with a separate option as Alistair suggested).
> With overlways we wouldn't need all the command-line options to enable
> construction of dtb fragments, it would be more or less transparent to
> QEMU. There may be limitations with the overlay flows that I'm not aware
> of though...

So the problem of adding DTB overlays is it's not that much easier than
the current options of using -machine dumpdtb and then hand hacking the
magic values and rebuilding, for example:

  https://medium.com/@denisobrezkov/xen-on-arm-and-qemu-1654f24dea75

Unless we come up with some sort of template support that allows QEMU to
insert numbers like address and size while processing the template. But
that seems a little too over engineered and likely introduces complexity
into the system.

Given the generic-loader is so simple I'm leaning towards another
approach of just c&p'ing generic-loader into a new "magic" device (maybe
guest-loader) and stripping out the bits we don't need (data, data-len,
be etc) and making the options more tuned what we are trying to achieve.
For example:

  -device guest-loader,kernel=path/to/Image,args="command line",addr=0x47000000,hyp=xen
  -device guest-loader,initrd=path/to/initrd,addr=0x42000000,hyp=xen

And then we can embed the smarts in the loader to set either DTB or ACPI
entries as required and if we need additional magic to support different
hypervisors (which hopefully you don't but...) you can modulate the
hyp=FOO variable.

There may be an argument for having a -hypervisor as a sugar alias for
-kernel (which maps to machine.kernel) but currently I see no practical
differences you need to launch it except maybe forcing the
virtualisation property to true if it exists - but that seems a little
ARM focused.

-- 
Alex Bennée

Re: [RFC PATCH 0/4] generic loader FDT support (for direct Xen boot)

[Alistair Francis]

On Tue, Oct 13, 2020 at 3:11 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Edgar E. Iglesias <edgar.iglesias@gmail.com> writes:
>
> > On Mon, Oct 12, 2020 at 05:02:57PM +0100, Alex Bennée wrote:
> >>
> >> Alistair Francis <alistair23@gmail.com> writes:
> >>
> >> > On Fri, Oct 9, 2020 at 10:07 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> >> >>
> >> >> Hi,
> >> >>
> >> >> This series adds the ability to append FDT data for blobs loaded by
> >> >> the generic loader. My principle use-case was to be able to directly
> >> >> boot Xen with a kernel image which avoided having to:
> >> >>
> >> >>   - get the kernel image into my system image
> >> >>   - deal with bugs in FDT munging by -bios firmware and/or grub
> <snip>
> >> >> "Polluting" the generic loader arguments
> >> >>
> >> >> This was mainly out of convenience as the loader already knows the
> >> >> size of the blob being loaded. However you could certainly argue it
> >> >> makes sense to have a more generic "FDT expander" virtual device that
> >> >> could for example query the QOM model somehow to find the details it
> >> >> needs.
> >> >
> >> > That seems like a better option. Why not have a generic way to modify
> >> > the device tree with a specific argument? It could either be -device
> >> > loader,file=fdt,... or -fdt ...
> >>
> >> Well it comes down to how much of a special case this is? Pretty much
> >> all FDT (and ACPI for the matter) is basically down to the board level
> >> models - and not all of them just the funky configurable ones. For other
> >> board models we just expect the user to pass the FDT they got with their
> >> kernel blob.
> >>
> >> For modern VirtIO systems the only thing you really need to expose is
> >> the root PCI bus because the rest of the devices are discover-able
> >> there.
> >>
> >> So the real question is are there any other -devices that we want to be
> >> able to graft FDT entries on or is the generic loader enough of a
> >> special case that we keep all the logic in there?
> >>
> >
> > Hi,
> >
> > Another option is to allow the user to pass along a DTB overlay with the
> > generic loader option (or with a separate option as Alistair suggested).
> > With overlways we wouldn't need all the command-line options to enable
> > construction of dtb fragments, it would be more or less transparent to
> > QEMU. There may be limitations with the overlay flows that I'm not aware
> > of though...
>
> So the problem of adding DTB overlays is it's not that much easier than
> the current options of using -machine dumpdtb and then hand hacking the
> magic values and rebuilding, for example:

I agree with this. If a user is changing a DTB from a command line
they probably only want small changes and are unlikely to need the
full power of an overlay.

>
>   https://medium.com/@denisobrezkov/xen-on-arm-and-qemu-1654f24dea75
>
> Unless we come up with some sort of template support that allows QEMU to
> insert numbers like address and size while processing the template. But
> that seems a little too over engineered and likely introduces complexity
> into the system.

This though sounds interesting :)

>
> Given the generic-loader is so simple I'm leaning towards another
> approach of just c&p'ing generic-loader into a new "magic" device (maybe
> guest-loader) and stripping out the bits we don't need (data, data-len,
> be etc) and making the options more tuned what we are trying to achieve.
> For example:
>
>   -device guest-loader,kernel=path/to/Image,args="command line",addr=0x47000000,hyp=xen
>   -device guest-loader,initrd=path/to/initrd,addr=0x42000000,hyp=xen

At first I'm not thrilled of adding a new loader. In saying that,
there are lots of times where -kernel and friends doesn't do what I
want it to do and I have to fall back to the generic loader and code
changes to QEMU so maybe this is a good idea for image loading.

I'm guessing this would be the same for every platform which would be
a nice change from -kernel.

Alistair

>
> And then we can embed the smarts in the loader to set either DTB or ACPI
> entries as required and if we need additional magic to support different
> hypervisors (which hopefully you don't but...) you can modulate the
> hyp=FOO variable.
>
> There may be an argument for having a -hypervisor as a sugar alias for
> -kernel (which maps to machine.kernel) but currently I see no practical
> differences you need to launch it except maybe forcing the
> virtualisation property to true if it exists - but that seems a little
> ARM focused.
>
> --
> Alex Bennée