[PATCH 0/4] OpenRISC Device Tree Support

[PATCH 0/4] OpenRISC Device Tree Support

[Stafford Horne]

This series adds device tree support for the OpenRISC SIM hardware.

The simulator will generate an FDT and pass it to the kernel.

For example:
  qemu-system-or1k -cpu or1200 -M or1k-sim \
    -kernel /home/shorne/work/linux/vmlinux \
    -initrd /home/shorne/work/linux/initramfs.cpio.gz \
    -serial mon:stdio -nographic -gdb tcp::10001 -m 32

Using the linux kernel or1ksim_defconfig we can remove the built-in
dts and the kernel will boot as expected.  The real benefit here is
being able to specify an external initrd which qemu will load into
memory and the device tree will tell the kernel where to find it.

-Stafford

Stafford Horne (4):
  hw/openrisc/openrisc_sim: Create machine state for or1ksim
  hw/openrisc/openrisc_sim: Paramatarize initialization
  hw/openrisc/openrisc_sim; Add support for loading a decice tree
  hw/openrisc/openrisc_sim: Add support for initrd loading

 hw/openrisc/openrisc_sim.c | 261 +++++++++++++++++++++++++++++++++++--
 1 file changed, 247 insertions(+), 14 deletions(-)

-- 
2.31.1

[PATCH 1/4] hw/openrisc/openrisc_sim: Create machine state for or1ksim

[Stafford Horne]

This will allow us to attach machine state attributes like
the device tree fdt.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 hw/openrisc/openrisc_sim.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
index 73fe383c2d..b83cc1c191 100644
--- a/hw/openrisc/openrisc_sim.c
+++ b/hw/openrisc/openrisc_sim.c
@@ -37,6 +37,18 @@
 
 #define KERNEL_LOAD_ADDR 0x100
 
+#define TYPE_OR1KSIM_MACHINE MACHINE_TYPE_NAME("or1k-sim")
+#define OR1KSIM_MACHINE(obj) \
+    OBJECT_CHECK(Or1ksimState, (obj), TYPE_OR1KSIM_MACHINE)
+
+typedef struct Or1ksimState {
+    /*< private >*/
+    MachineState parent_obj;
+
+    /*< public >*/
+
+} Or1ksimState;
+
 static struct openrisc_boot_info {
     uint32_t bootstrap_pc;
 } boot_info;
@@ -141,6 +153,7 @@ static void openrisc_sim_init(MachineState *machine)
     ram_addr_t ram_size = machine->ram_size;
     const char *kernel_filename = machine->kernel_filename;
     OpenRISCCPU *cpus[2] = {};
+    Or1ksimState *s = OR1KSIM_MACHINE(machine);
     MemoryRegion *ram;
     qemu_irq serial_irq;
     int n;
@@ -183,8 +196,10 @@ static void openrisc_sim_init(MachineState *machine)
     openrisc_load_kernel(ram_size, kernel_filename);
 }
 
-static void openrisc_sim_machine_init(MachineClass *mc)
+static void openrisc_sim_machine_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
+
     mc->desc = "or1k simulation";
     mc->init = openrisc_sim_init;
     mc->max_cpus = 2;
@@ -192,4 +207,16 @@ static void openrisc_sim_machine_init(MachineClass *mc)
     mc->default_cpu_type = OPENRISC_CPU_TYPE_NAME("or1200");
 }
 
-DEFINE_MACHINE("or1k-sim", openrisc_sim_machine_init)
+static const TypeInfo or1ksim_machine_typeinfo = {
+    .name       = TYPE_OR1KSIM_MACHINE,
+    .parent     = TYPE_MACHINE,
+    .class_init = openrisc_sim_machine_init,
+    .instance_size = sizeof(Or1ksimState),
+};
+
+static void or1ksim_machine_init_register_types(void)
+{
+    type_register_static(&or1ksim_machine_typeinfo);
+}
+
+type_init(or1ksim_machine_init_register_types)
-- 
2.31.1

[PATCH 2/4] hw/openrisc/openrisc_sim: Paramatarize initialization

[Stafford Horne]

Move magic numbers to variables and enums. These will be
reused for upcoming fdt initialization.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 hw/openrisc/openrisc_sim.c | 42 ++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
index b83cc1c191..5a0cc4d27e 100644
--- a/hw/openrisc/openrisc_sim.c
+++ b/hw/openrisc/openrisc_sim.c
@@ -49,6 +49,29 @@ typedef struct Or1ksimState {
 
 } Or1ksimState;
 
+enum {
+    OR1KSIM_DRAM,
+    OR1KSIM_UART,
+    OR1KSIM_ETHOC,
+    OR1KSIM_OMPIC,
+};
+
+enum {
+    OR1KSIM_OMPIC_IRQ = 1,
+    OR1KSIM_UART_IRQ = 2,
+    OR1KSIM_ETHOC_IRQ = 4,
+};
+
+static const struct MemmapEntry {
+    hwaddr base;
+    hwaddr size;
+} or1ksim_memmap[] = {
+    [OR1KSIM_DRAM] =      { 0x00000000,          0 },
+    [OR1KSIM_UART] =      { 0x90000000,      0x100 },
+    [OR1KSIM_ETHOC] =     { 0x92000000,      0x800 },
+    [OR1KSIM_OMPIC] =     { 0x98000000,         16 },
+};
+
 static struct openrisc_boot_info {
     uint32_t bootstrap_pc;
 } boot_info;
@@ -177,21 +200,24 @@ static void openrisc_sim_init(MachineState *machine)
     memory_region_add_subregion(get_system_memory(), 0, ram);
 
     if (nd_table[0].used) {
-        openrisc_sim_net_init(0x92000000, 0x92000400, smp_cpus,
-                              cpus, 4, nd_table);
+        openrisc_sim_net_init(or1ksim_memmap[OR1KSIM_ETHOC].base,
+                              or1ksim_memmap[OR1KSIM_ETHOC].base + 0x400,
+                              smp_cpus, cpus,
+                              OR1KSIM_ETHOC_IRQ, nd_table);
     }
 
     if (smp_cpus > 1) {
-        openrisc_sim_ompic_init(0x98000000, smp_cpus, cpus, 1);
+        openrisc_sim_ompic_init(or1ksim_memmap[OR1KSIM_OMPIC].base, smp_cpus,
+                                cpus, OR1KSIM_OMPIC_IRQ);
 
-        serial_irq = qemu_irq_split(get_cpu_irq(cpus, 0, 2),
-                                    get_cpu_irq(cpus, 1, 2));
+        serial_irq = qemu_irq_split(get_cpu_irq(cpus, 0, OR1KSIM_UART_IRQ),
+                                    get_cpu_irq(cpus, 1, OR1KSIM_UART_IRQ));
     } else {
-        serial_irq = get_cpu_irq(cpus, 0, 2);
+        serial_irq = get_cpu_irq(cpus, 0, OR1KSIM_UART_IRQ);
     }
 
-    serial_mm_init(get_system_memory(), 0x90000000, 0, serial_irq,
-                   115200, serial_hd(0), DEVICE_NATIVE_ENDIAN);
+    serial_mm_init(get_system_memory(), or1ksim_memmap[OR1KSIM_UART].base, 0,
+                   serial_irq, 115200, serial_hd(0), DEVICE_NATIVE_ENDIAN);
 
     openrisc_load_kernel(ram_size, kernel_filename);
 }
-- 
2.31.1

[PATCH 3/4] hw/openrisc/openrisc_sim; Add support for loading a decice tree

[Stafford Horne]

Using the device tree means that qemu can now directly tell
the kernel what hardware is configured rather than use having
to maintain and update a separate device tree file.

This patch adds device tree support for the OpenRISC simulator.
A device tree is built up based on the state of the configure
openrisc simulator.

This is then dumpt to memory and the load address is passed to the
kernel in register r3.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 hw/openrisc/openrisc_sim.c | 158 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 154 insertions(+), 4 deletions(-)

diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
index 5a0cc4d27e..d7c26af82c 100644
--- a/hw/openrisc/openrisc_sim.c
+++ b/hw/openrisc/openrisc_sim.c
@@ -29,14 +29,20 @@
 #include "net/net.h"
 #include "hw/loader.h"
 #include "hw/qdev-properties.h"
+#include "exec/address-spaces.h"
+#include "sysemu/device_tree.h"
 #include "sysemu/sysemu.h"
 #include "hw/sysbus.h"
 #include "sysemu/qtest.h"
 #include "sysemu/reset.h"
 #include "hw/core/split-irq.h"
 
+#include <libfdt.h>
+
 #define KERNEL_LOAD_ADDR 0x100
 
+#define OR1KSIM_CLK_MHZ 20000000
+
 #define TYPE_OR1KSIM_MACHINE MACHINE_TYPE_NAME("or1k-sim")
 #define OR1KSIM_MACHINE(obj) \
     OBJECT_CHECK(Or1ksimState, (obj), TYPE_OR1KSIM_MACHINE)
@@ -46,6 +52,8 @@ typedef struct Or1ksimState {
     MachineState parent_obj;
 
     /*< public >*/
+    void *fdt;
+    int fdt_size;
 
 } Or1ksimState;
 
@@ -74,6 +82,7 @@ static const struct MemmapEntry {
 
 static struct openrisc_boot_info {
     uint32_t bootstrap_pc;
+    uint32_t fdt_addr;
 } boot_info;
 
 static void main_cpu_reset(void *opaque)
@@ -84,6 +93,7 @@ static void main_cpu_reset(void *opaque)
     cpu_reset(CPU(cpu));
 
     cpu_set_pc(cs, boot_info.bootstrap_pc);
+    cpu_set_gpr(&cpu->env, 3, boot_info.fdt_addr);
 }
 
 static qemu_irq get_cpu_irq(OpenRISCCPU *cpus[], int cpunum, int irq_pin)
@@ -137,26 +147,29 @@ static void openrisc_sim_ompic_init(hwaddr base, int num_cpus,
     sysbus_mmio_map(s, 0, base);
 }
 
-static void openrisc_load_kernel(ram_addr_t ram_size,
+static hwaddr openrisc_load_kernel(ram_addr_t ram_size,
                                  const char *kernel_filename)
 {
     long kernel_size;
     uint64_t elf_entry;
+    uint64_t high_addr;
     hwaddr entry;
 
     if (kernel_filename && !qtest_enabled()) {
         kernel_size = load_elf(kernel_filename, NULL, NULL, NULL,
-                               &elf_entry, NULL, NULL, NULL, 1, EM_OPENRISC,
-                               1, 0);
+                               &elf_entry, NULL, &high_addr, NULL, 1,
+                               EM_OPENRISC, 1, 0);
         entry = elf_entry;
         if (kernel_size < 0) {
             kernel_size = load_uimage(kernel_filename,
                                       &entry, NULL, NULL, NULL, NULL);
+            high_addr = entry + kernel_size;
         }
         if (kernel_size < 0) {
             kernel_size = load_image_targphys(kernel_filename,
                                               KERNEL_LOAD_ADDR,
                                               ram_size - KERNEL_LOAD_ADDR);
+            high_addr = KERNEL_LOAD_ADDR + kernel_size;
         }
 
         if (entry <= 0) {
@@ -168,7 +181,139 @@ static void openrisc_load_kernel(ram_addr_t ram_size,
             exit(1);
         }
         boot_info.bootstrap_pc = entry;
+
+        return high_addr;
+    }
+    return 0;
+}
+
+static uint32_t openrisc_load_fdt(Or1ksimState *s, hwaddr load_start,
+    uint64_t mem_size)
+{
+    uint32_t fdt_addr;
+    int fdtsize = fdt_totalsize(s->fdt);
+
+    if (fdtsize <= 0) {
+        error_report("invalid device-tree");
+        exit(1);
+    }
+
+    /* We should put fdt right after the kernel */
+    fdt_addr = ROUND_UP(load_start, 4);
+
+    fdt_pack(s->fdt);
+    /* copy in the device tree */
+    qemu_fdt_dumpdtb(s->fdt, fdtsize);
+
+    rom_add_blob_fixed_as("fdt", s->fdt, fdtsize, fdt_addr,
+                          &address_space_memory);
+
+    return fdt_addr;
+}
+
+static void openrisc_create_fdt(Or1ksimState *s,
+    const struct MemmapEntry *memmap, int num_cpus, uint64_t mem_size,
+    const char *cmdline)
+{
+    void *fdt;
+    int cpu;
+    char *nodename;
+    int pic_ph;
+
+    fdt = s->fdt = create_device_tree(&s->fdt_size);
+    if (!fdt) {
+        error_report("create_device_tree() failed");
+        exit(1);
+    }
+
+    qemu_fdt_setprop_string(fdt, "/", "compatible", "opencores,or1ksim");
+    qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x1);
+    qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x1);
+
+    nodename = g_strdup_printf("/memory@%lx",
+                               (long)memmap[OR1KSIM_DRAM].base);
+    qemu_fdt_add_subnode(fdt, nodename);
+    qemu_fdt_setprop_cells(fdt, nodename, "reg",
+                           memmap[OR1KSIM_DRAM].base, mem_size);
+    qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
+    g_free(nodename);
+
+    qemu_fdt_add_subnode(fdt, "/cpus");
+    qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
+    qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
+
+    for (cpu = 0; cpu < num_cpus; cpu++) {
+        nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
+        qemu_fdt_add_subnode(fdt, nodename);
+        qemu_fdt_setprop_string(fdt, nodename, "compatible",
+                                "opencores,or1200-rtlsvn481");
+        qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
+        qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency",
+                              OR1KSIM_CLK_MHZ);
+        g_free(nodename);
+    }
+
+    if (num_cpus > 0) {
+        nodename = g_strdup_printf("/ompic@%lx",
+                                   (long)memmap[OR1KSIM_OMPIC].base);
+        qemu_fdt_add_subnode(fdt, nodename);
+        qemu_fdt_setprop_string(fdt, nodename, "compatible", "openrisc,ompic");
+        qemu_fdt_setprop_cells(fdt, nodename, "reg",
+                               memmap[OR1KSIM_OMPIC].base,
+                               memmap[OR1KSIM_OMPIC].size);
+        qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0);
+        qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0);
+        qemu_fdt_setprop_cell(fdt, nodename, "interrupts", OR1KSIM_OMPIC_IRQ);
+        g_free(nodename);
     }
+
+    nodename = (char *)"/pic";
+    qemu_fdt_add_subnode(fdt, nodename);
+    pic_ph = qemu_fdt_alloc_phandle(fdt);
+    qemu_fdt_setprop_string(fdt, nodename, "compatible",
+                            "opencores,or1k-pic-level");
+    qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 1);
+    qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0);
+    qemu_fdt_setprop_cell(fdt, nodename, "phandle", pic_ph);
+
+    qemu_fdt_setprop_cell(fdt, "/", "interrupt-parent", pic_ph);
+
+    if (nd_table[0].used) {
+        nodename = g_strdup_printf("/ethoc@%lx",
+                                   (long)memmap[OR1KSIM_ETHOC].base);
+        qemu_fdt_add_subnode(fdt, nodename);
+        qemu_fdt_setprop_string(fdt, nodename, "compatible", "opencores,ethoc");
+        qemu_fdt_setprop_cells(fdt, nodename, "reg",
+                               memmap[OR1KSIM_ETHOC].base,
+                               memmap[OR1KSIM_ETHOC].size);
+        qemu_fdt_setprop_cell(fdt, nodename, "interrupts", OR1KSIM_ETHOC_IRQ);
+        qemu_fdt_setprop(fdt, nodename, "big-endian", NULL, 0);
+
+        qemu_fdt_add_subnode(fdt, "/aliases");
+        qemu_fdt_setprop_string(fdt, "/aliases", "enet0", nodename);
+        g_free(nodename);
+    }
+
+    nodename = g_strdup_printf("/serial@%lx",
+                               (long)memmap[OR1KSIM_UART].base);
+    qemu_fdt_add_subnode(fdt, nodename);
+    qemu_fdt_setprop_string(fdt, nodename, "compatible", "ns16550a");
+    qemu_fdt_setprop_cells(fdt, nodename, "reg",
+                           memmap[OR1KSIM_UART].base,
+                           memmap[OR1KSIM_UART].size);
+    qemu_fdt_setprop_cell(fdt, nodename, "interrupts", OR1KSIM_UART_IRQ);
+    qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency", OR1KSIM_CLK_MHZ);
+    qemu_fdt_setprop(fdt, nodename, "big-endian", NULL, 0);
+
+    qemu_fdt_add_subnode(fdt, "/chosen");
+    qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
+    if (cmdline) {
+        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
+    }
+
+    qemu_fdt_setprop_string(fdt, "/aliases", "uart0", nodename);
+
+    g_free(nodename);
 }
 
 static void openrisc_sim_init(MachineState *machine)
@@ -178,6 +323,7 @@ static void openrisc_sim_init(MachineState *machine)
     OpenRISCCPU *cpus[2] = {};
     Or1ksimState *s = OR1KSIM_MACHINE(machine);
     MemoryRegion *ram;
+    hwaddr load_addr;
     qemu_irq serial_irq;
     int n;
     unsigned int smp_cpus = machine->smp.cpus;
@@ -219,7 +365,11 @@ static void openrisc_sim_init(MachineState *machine)
     serial_mm_init(get_system_memory(), or1ksim_memmap[OR1KSIM_UART].base, 0,
                    serial_irq, 115200, serial_hd(0), DEVICE_NATIVE_ENDIAN);
 
-    openrisc_load_kernel(ram_size, kernel_filename);
+    openrisc_create_fdt(s, or1ksim_memmap, smp_cpus, machine->ram_size,
+                        machine->kernel_cmdline);
+
+    load_addr = openrisc_load_kernel(ram_size, kernel_filename);
+    boot_info.fdt_addr = openrisc_load_fdt(s, load_addr, machine->ram_size);
 }
 
 static void openrisc_sim_machine_init(ObjectClass *oc, void *data)
-- 
2.31.1

[PATCH 4/4] hw/openrisc/openrisc_sim: Add support for initrd loading

[Stafford Horne]

The loaded initrd is loaded into memory.  It's location and size is then
added to the device tree so the kernel knows where to find it.

Signed-off-by: Stafford Horne <shorne@gmail.com>
---
 hw/openrisc/openrisc_sim.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
index d7c26af82c..5354797e20 100644
--- a/hw/openrisc/openrisc_sim.c
+++ b/hw/openrisc/openrisc_sim.c
@@ -187,6 +187,32 @@ static hwaddr openrisc_load_kernel(ram_addr_t ram_size,
     return 0;
 }
 
+static hwaddr openrisc_load_initrd(Or1ksimState *s, const char *filename,
+    hwaddr load_start, uint64_t mem_size)
+{
+    int size;
+    hwaddr start;
+
+    /* We put the initrd right after the kernel; page aligned. */
+    start = TARGET_PAGE_ALIGN(load_start);
+
+    size = load_ramdisk(filename, start, mem_size - start);
+    if (size < 0) {
+        size = load_image_targphys(filename, start, mem_size - start);
+        if (size < 0) {
+            error_report("could not load ramdisk '%s'", filename);
+            exit(1);
+        }
+    }
+
+    qemu_fdt_setprop_cell(s->fdt, "/chosen",
+                          "linux,initrd-start", start);
+    qemu_fdt_setprop_cell(s->fdt, "/chosen",
+                          "linux,initrd-end", start + size);
+
+    return start + size;
+}
+
 static uint32_t openrisc_load_fdt(Or1ksimState *s, hwaddr load_start,
     uint64_t mem_size)
 {
@@ -198,7 +224,7 @@ static uint32_t openrisc_load_fdt(Or1ksimState *s, hwaddr load_start,
         exit(1);
     }
 
-    /* We should put fdt right after the kernel */
+    /* We put fdt right after the kernel and/or initrd. */
     fdt_addr = ROUND_UP(load_start, 4);
 
     fdt_pack(s->fdt);
@@ -369,6 +395,10 @@ static void openrisc_sim_init(MachineState *machine)
                         machine->kernel_cmdline);
 
     load_addr = openrisc_load_kernel(ram_size, kernel_filename);
+    if (machine->initrd_filename) {
+        load_addr = openrisc_load_initrd(s, machine->initrd_filename,
+                                         load_addr, machine->ram_size);
+    }
     boot_info.fdt_addr = openrisc_load_fdt(s, load_addr, machine->ram_size);
 }
 
-- 
2.31.1

Re: [PATCH 1/4] hw/openrisc/openrisc_sim: Create machine state for or1ksim

[Philippe Mathieu-Daudé via]

On 10/2/22 07:30, Stafford Horne wrote:
> This will allow us to attach machine state attributes like
> the device tree fdt.
> 
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>   hw/openrisc/openrisc_sim.c | 31 +++++++++++++++++++++++++++++--
>   1 file changed, 29 insertions(+), 2 deletions(-)

> @@ -141,6 +153,7 @@ static void openrisc_sim_init(MachineState *machine)
>       ram_addr_t ram_size = machine->ram_size;
>       const char *kernel_filename = machine->kernel_filename;
>       OpenRISCCPU *cpus[2] = {};
> +    Or1ksimState *s = OR1KSIM_MACHINE(machine);

This change belong to patch #3.

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

>       MemoryRegion *ram;
>       qemu_irq serial_irq;
>       int n;
> @@ -183,8 +196,10 @@ static void openrisc_sim_init(MachineState *machine)
>       openrisc_load_kernel(ram_size, kernel_filename);
>   }

Re: [PATCH 2/4] hw/openrisc/openrisc_sim: Paramatarize initialization

[Philippe Mathieu-Daudé via]

On 10/2/22 07:30, Stafford Horne wrote:
> Move magic numbers to variables and enums. These will be
> reused for upcoming fdt initialization.
> 
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>   hw/openrisc/openrisc_sim.c | 42 ++++++++++++++++++++++++++++++--------
>   1 file changed, 34 insertions(+), 8 deletions(-)

Typo "Parameterize" in subject.

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

Re: [PATCH 3/4] hw/openrisc/openrisc_sim; Add support for loading a decice tree

[Philippe Mathieu-Daudé via]

Typo "device" in subject.

On 10/2/22 07:30, Stafford Horne wrote:
> Using the device tree means that qemu can now directly tell
> the kernel what hardware is configured rather than use having
> to maintain and update a separate device tree file.
> 
> This patch adds device tree support for the OpenRISC simulator.
> A device tree is built up based on the state of the configure
> openrisc simulator.
> 
> This is then dumpt to memory and the load address is passed to the

"dumped"?

> kernel in register r3.
> 
> Signed-off-by: Stafford Horne <shorne@gmail.com>
> ---
>   hw/openrisc/openrisc_sim.c | 158 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 154 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
> index 5a0cc4d27e..d7c26af82c 100644
> --- a/hw/openrisc/openrisc_sim.c
> +++ b/hw/openrisc/openrisc_sim.c
> @@ -29,14 +29,20 @@
>   #include "net/net.h"
>   #include "hw/loader.h"
>   #include "hw/qdev-properties.h"
> +#include "exec/address-spaces.h"
> +#include "sysemu/device_tree.h"
>   #include "sysemu/sysemu.h"
>   #include "hw/sysbus.h"
>   #include "sysemu/qtest.h"
>   #include "sysemu/reset.h"
>   #include "hw/core/split-irq.h"
>   
> +#include <libfdt.h>

Watch out, you now need to add TARGET_NEED_FDT=y
to configs/targets/or1k-softmmu.mak.

Re: [PATCH 1/4] hw/openrisc/openrisc_sim: Create machine state for or1ksim

[Stafford Horne]

On Thu, Feb 10, 2022 at 12:05:22PM +0100, Philippe Mathieu-Daudé wrote:
> On 10/2/22 07:30, Stafford Horne wrote:
> > This will allow us to attach machine state attributes like
> > the device tree fdt.
> > 
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >   hw/openrisc/openrisc_sim.c | 31 +++++++++++++++++++++++++++++--
> >   1 file changed, 29 insertions(+), 2 deletions(-)
> 
> > @@ -141,6 +153,7 @@ static void openrisc_sim_init(MachineState *machine)
> >       ram_addr_t ram_size = machine->ram_size;
> >       const char *kernel_filename = machine->kernel_filename;
> >       OpenRISCCPU *cpus[2] = {};
> > +    Or1ksimState *s = OR1KSIM_MACHINE(machine);
> 
> This change belong to patch #3.

Yes, when I was splitting this patch out I left it here because I was
"preparing".  But it is not being used, so fair enough.

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

Thanks

> >       MemoryRegion *ram;
> >       qemu_irq serial_irq;
> >       int n;
> > @@ -183,8 +196,10 @@ static void openrisc_sim_init(MachineState *machine)
> >       openrisc_load_kernel(ram_size, kernel_filename);
> >   }

Re: [PATCH 2/4] hw/openrisc/openrisc_sim: Paramatarize initialization

[Stafford Horne]

On Thu, Feb 10, 2022 at 12:07:02PM +0100, Philippe Mathieu-Daudé wrote:
> On 10/2/22 07:30, Stafford Horne wrote:
> > Move magic numbers to variables and enums. These will be
> > reused for upcoming fdt initialization.
> > 
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >   hw/openrisc/openrisc_sim.c | 42 ++++++++++++++++++++++++++++++--------
> >   1 file changed, 34 insertions(+), 8 deletions(-)
> 
> Typo "Parameterize" in subject.

Yes.

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

Thank you.

Re: [PATCH 3/4] hw/openrisc/openrisc_sim; Add support for loading a decice tree

[Stafford Horne]

On Thu, Feb 10, 2022 at 12:10:54PM +0100, Philippe Mathieu-Daudé wrote:
> Typo "device" in subject.

OK.

> On 10/2/22 07:30, Stafford Horne wrote:
> > Using the device tree means that qemu can now directly tell
> > the kernel what hardware is configured rather than use having
> > to maintain and update a separate device tree file.
> > 
> > This patch adds device tree support for the OpenRISC simulator.
> > A device tree is built up based on the state of the configure
> > openrisc simulator.
> > 
> > This is then dumpt to memory and the load address is passed to the
> 
> "dumped"?

Yes.

> > kernel in register r3.
> > 
> > Signed-off-by: Stafford Horne <shorne@gmail.com>
> > ---
> >   hw/openrisc/openrisc_sim.c | 158 ++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 154 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/openrisc/openrisc_sim.c b/hw/openrisc/openrisc_sim.c
> > index 5a0cc4d27e..d7c26af82c 100644
> > --- a/hw/openrisc/openrisc_sim.c
> > +++ b/hw/openrisc/openrisc_sim.c
> > @@ -29,14 +29,20 @@
> >   #include "net/net.h"
> >   #include "hw/loader.h"
> >   #include "hw/qdev-properties.h"
> > +#include "exec/address-spaces.h"
> > +#include "sysemu/device_tree.h"
> >   #include "sysemu/sysemu.h"
> >   #include "hw/sysbus.h"
> >   #include "sysemu/qtest.h"
> >   #include "sysemu/reset.h"
> >   #include "hw/core/split-irq.h"
> > +#include <libfdt.h>
> 
> Watch out, you now need to add TARGET_NEED_FDT=y
> to configs/targets/or1k-softmmu.mak.

OK, I will add it in this patch.  I missed that part.

Re: [PATCH 3/4] hw/openrisc/openrisc_sim; Add support for loading a decice tree

[Peter Maydell]

On Thu, 10 Feb 2022 at 06:46, Stafford Horne <shorne@gmail.com> wrote:
>
> Using the device tree means that qemu can now directly tell
> the kernel what hardware is configured rather than use having
> to maintain and update a separate device tree file.
>
> This patch adds device tree support for the OpenRISC simulator.
> A device tree is built up based on the state of the configure
> openrisc simulator.

This sounds like it's support for creating a device
tree? Support for loading a device tree would be "the
user passes us a filename of a dtb file". (This is mostly a
quibble about commit message wording.)

> -static void openrisc_load_kernel(ram_addr_t ram_size,
> +static hwaddr openrisc_load_kernel(ram_addr_t ram_size,
>                                   const char *kernel_filename)

Indentation looks off now ?

>  {
>      long kernel_size;
>      uint64_t elf_entry;
> +    uint64_t high_addr;
>      hwaddr entry;
>
>      if (kernel_filename && !qtest_enabled()) {
>          kernel_size = load_elf(kernel_filename, NULL, NULL, NULL,
> -                               &elf_entry, NULL, NULL, NULL, 1, EM_OPENRISC,
> -                               1, 0);
> +                               &elf_entry, NULL, &high_addr, NULL, 1,
> +                               EM_OPENRISC, 1, 0);
>          entry = elf_entry;
>          if (kernel_size < 0) {
>              kernel_size = load_uimage(kernel_filename,
>                                        &entry, NULL, NULL, NULL, NULL);
> +            high_addr = entry + kernel_size;
>          }
>          if (kernel_size < 0) {
>              kernel_size = load_image_targphys(kernel_filename,
>                                                KERNEL_LOAD_ADDR,
>                                                ram_size - KERNEL_LOAD_ADDR);
> +            high_addr = KERNEL_LOAD_ADDR + kernel_size;
>          }
>
>          if (entry <= 0) {
> @@ -168,7 +181,139 @@ static void openrisc_load_kernel(ram_addr_t ram_size,
>              exit(1);
>          }
>          boot_info.bootstrap_pc = entry;
> +
> +        return high_addr;
> +    }
> +    return 0;
> +}
> +
> +static uint32_t openrisc_load_fdt(Or1ksimState *s, hwaddr load_start,
> +    uint64_t mem_size)

Indentation again.

> +{
> +    uint32_t fdt_addr;
> +    int fdtsize = fdt_totalsize(s->fdt);
> +
> +    if (fdtsize <= 0) {
> +        error_report("invalid device-tree");
> +        exit(1);
> +    }
> +
> +    /* We should put fdt right after the kernel */

You change this comment in patch 4 -- I think you might as well
just use that text in this patch to start with.

> +    fdt_addr = ROUND_UP(load_start, 4);
> +
> +    fdt_pack(s->fdt);

fdt_pack() returns an error code -- you should check it.

> +    /* copy in the device tree */
> +    qemu_fdt_dumpdtb(s->fdt, fdtsize);
> +
> +    rom_add_blob_fixed_as("fdt", s->fdt, fdtsize, fdt_addr,
> +                          &address_space_memory);
> +
> +    return fdt_addr;
> +}
> +
> +static void openrisc_create_fdt(Or1ksimState *s,
> +    const struct MemmapEntry *memmap, int num_cpus, uint64_t mem_size,
> +    const char *cmdline)

Indentation.

> +{
> +    void *fdt;
> +    int cpu;
> +    char *nodename;
> +    int pic_ph;
> +
> +    fdt = s->fdt = create_device_tree(&s->fdt_size);
> +    if (!fdt) {
> +        error_report("create_device_tree() failed");
> +        exit(1);
> +    }
> +
> +    qemu_fdt_setprop_string(fdt, "/", "compatible", "opencores,or1ksim");
> +    qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x1);
> +    qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x1);
> +
> +    nodename = g_strdup_printf("/memory@%lx",
> +                               (long)memmap[OR1KSIM_DRAM].base);

Use the appropriate format string macro for the type, rather than
casting to long (here and below).

> +    qemu_fdt_add_subnode(fdt, nodename);
> +    qemu_fdt_setprop_cells(fdt, nodename, "reg",
> +                           memmap[OR1KSIM_DRAM].base, mem_size);
> +    qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
> +    g_free(nodename);
> +
> +    qemu_fdt_add_subnode(fdt, "/cpus");
> +    qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
> +    qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
> +
> +    for (cpu = 0; cpu < num_cpus; cpu++) {
> +        nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
> +        qemu_fdt_add_subnode(fdt, nodename);
> +        qemu_fdt_setprop_string(fdt, nodename, "compatible",
> +                                "opencores,or1200-rtlsvn481");
> +        qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
> +        qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency",
> +                              OR1KSIM_CLK_MHZ);
> +        g_free(nodename);
> +    }
> +
> +    if (num_cpus > 0) {
> +        nodename = g_strdup_printf("/ompic@%lx",
> +                                   (long)memmap[OR1KSIM_OMPIC].base);
> +        qemu_fdt_add_subnode(fdt, nodename);
> +        qemu_fdt_setprop_string(fdt, nodename, "compatible", "openrisc,ompic");
> +        qemu_fdt_setprop_cells(fdt, nodename, "reg",
> +                               memmap[OR1KSIM_OMPIC].base,
> +                               memmap[OR1KSIM_OMPIC].size);
> +        qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0);
> +        qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0);
> +        qemu_fdt_setprop_cell(fdt, nodename, "interrupts", OR1KSIM_OMPIC_IRQ);
> +        g_free(nodename);
>      }
> +
> +    nodename = (char *)"/pic";
> +    qemu_fdt_add_subnode(fdt, nodename);
> +    pic_ph = qemu_fdt_alloc_phandle(fdt);
> +    qemu_fdt_setprop_string(fdt, nodename, "compatible",
> +                            "opencores,or1k-pic-level");
> +    qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 1);
> +    qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0);
> +    qemu_fdt_setprop_cell(fdt, nodename, "phandle", pic_ph);
> +
> +    qemu_fdt_setprop_cell(fdt, "/", "interrupt-parent", pic_ph);
> +
> +    if (nd_table[0].used) {
> +        nodename = g_strdup_printf("/ethoc@%lx",
> +                                   (long)memmap[OR1KSIM_ETHOC].base);
> +        qemu_fdt_add_subnode(fdt, nodename);
> +        qemu_fdt_setprop_string(fdt, nodename, "compatible", "opencores,ethoc");
> +        qemu_fdt_setprop_cells(fdt, nodename, "reg",
> +                               memmap[OR1KSIM_ETHOC].base,
> +                               memmap[OR1KSIM_ETHOC].size);
> +        qemu_fdt_setprop_cell(fdt, nodename, "interrupts", OR1KSIM_ETHOC_IRQ);
> +        qemu_fdt_setprop(fdt, nodename, "big-endian", NULL, 0);
> +
> +        qemu_fdt_add_subnode(fdt, "/aliases");
> +        qemu_fdt_setprop_string(fdt, "/aliases", "enet0", nodename);
> +        g_free(nodename);
> +    }
> +
> +    nodename = g_strdup_printf("/serial@%lx",
> +                               (long)memmap[OR1KSIM_UART].base);
> +    qemu_fdt_add_subnode(fdt, nodename);
> +    qemu_fdt_setprop_string(fdt, nodename, "compatible", "ns16550a");
> +    qemu_fdt_setprop_cells(fdt, nodename, "reg",
> +                           memmap[OR1KSIM_UART].base,
> +                           memmap[OR1KSIM_UART].size);
> +    qemu_fdt_setprop_cell(fdt, nodename, "interrupts", OR1KSIM_UART_IRQ);
> +    qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency", OR1KSIM_CLK_MHZ);
> +    qemu_fdt_setprop(fdt, nodename, "big-endian", NULL, 0);
> +
> +    qemu_fdt_add_subnode(fdt, "/chosen");
> +    qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
> +    if (cmdline) {
> +        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> +    }
> +
> +    qemu_fdt_setprop_string(fdt, "/aliases", "uart0", nodename);

I think the pattern the hw/arm/virt.c code uses, where the board
code functions that create the UART, interrupt controller, etc
devices on the board also have the code to add FDT nodes for them.
Especially as the board gets new devices added over time, it's easier
to check that the FDT nodes and the devices match up, and you don't
have to awkwardly duplicate control-flow logic like "we only have
an ethernet device if nd_table[0].used". But I won't insist that
you rewrite this.

> +    g_free(nodename);
>  }
>
>  static void openrisc_sim_init(MachineState *machine)
> @@ -178,6 +323,7 @@ static void openrisc_sim_init(MachineState *machine)
>      OpenRISCCPU *cpus[2] = {};
>      Or1ksimState *s = OR1KSIM_MACHINE(machine);
>      MemoryRegion *ram;
> +    hwaddr load_addr;
>      qemu_irq serial_irq;
>      int n;
>      unsigned int smp_cpus = machine->smp.cpus;
> @@ -219,7 +365,11 @@ static void openrisc_sim_init(MachineState *machine)
>      serial_mm_init(get_system_memory(), or1ksim_memmap[OR1KSIM_UART].base, 0,
>                     serial_irq, 115200, serial_hd(0), DEVICE_NATIVE_ENDIAN);
>
> -    openrisc_load_kernel(ram_size, kernel_filename);
> +    openrisc_create_fdt(s, or1ksim_memmap, smp_cpus, machine->ram_size,
> +                        machine->kernel_cmdline);
> +
> +    load_addr = openrisc_load_kernel(ram_size, kernel_filename);
> +    boot_info.fdt_addr = openrisc_load_fdt(s, load_addr, machine->ram_size);
>  }

If the user doesn't specify a kernel file, we'll still load the FDT,
at address zero. Is that sensible/intended behaviour ?

thanks
-- PMM

Re: [PATCH 3/4] hw/openrisc/openrisc_sim; Add support for loading a decice tree

[Stafford Horne]

On Thu, Feb 17, 2022 at 06:18:58PM +0000, Peter Maydell wrote:
> On Thu, 10 Feb 2022 at 06:46, Stafford Horne <shorne@gmail.com> wrote:
> >
> > Using the device tree means that qemu can now directly tell
> > the kernel what hardware is configured rather than use having
> > to maintain and update a separate device tree file.
> >
> > This patch adds device tree support for the OpenRISC simulator.
> > A device tree is built up based on the state of the configure
> > openrisc simulator.
> 
> This sounds like it's support for creating a device
> tree? Support for loading a device tree would be "the
> user passes us a filename of a dtb file". (This is mostly a
> quibble about commit message wording.)

Ah, yes I will fix this to say, "adds automatic device tree generation support"
....

> > -static void openrisc_load_kernel(ram_addr_t ram_size,
> > +static hwaddr openrisc_load_kernel(ram_addr_t ram_size,
> >                                   const char *kernel_filename)
> 
> Indentation looks off now ?

Fixed now.

> >  {
> >      long kernel_size;
> >      uint64_t elf_entry;
> > +    uint64_t high_addr;
> >      hwaddr entry;
> >
> >      if (kernel_filename && !qtest_enabled()) {
> >          kernel_size = load_elf(kernel_filename, NULL, NULL, NULL,
> > -                               &elf_entry, NULL, NULL, NULL, 1, EM_OPENRISC,
> > -                               1, 0);
> > +                               &elf_entry, NULL, &high_addr, NULL, 1,
> > +                               EM_OPENRISC, 1, 0);
> >          entry = elf_entry;
> >          if (kernel_size < 0) {
> >              kernel_size = load_uimage(kernel_filename,
> >                                        &entry, NULL, NULL, NULL, NULL);
> > +            high_addr = entry + kernel_size;
> >          }
> >          if (kernel_size < 0) {
> >              kernel_size = load_image_targphys(kernel_filename,
> >                                                KERNEL_LOAD_ADDR,
> >                                                ram_size - KERNEL_LOAD_ADDR);
> > +            high_addr = KERNEL_LOAD_ADDR + kernel_size;
> >          }
> >
> >          if (entry <= 0) {
> > @@ -168,7 +181,139 @@ static void openrisc_load_kernel(ram_addr_t ram_size,
> >              exit(1);
> >          }
> >          boot_info.bootstrap_pc = entry;
> > +
> > +        return high_addr;
> > +    }
> > +    return 0;
> > +}
> > +
> > +static uint32_t openrisc_load_fdt(Or1ksimState *s, hwaddr load_start,
> > +    uint64_t mem_size)
> 
> Indentation again.

Fixed.

> > +{
> > +    uint32_t fdt_addr;
> > +    int fdtsize = fdt_totalsize(s->fdt);
> > +
> > +    if (fdtsize <= 0) {
> > +        error_report("invalid device-tree");
> > +        exit(1);
> > +    }
> > +
> > +    /* We should put fdt right after the kernel */
> 
> You change this comment in patch 4 -- I think you might as well
> just use that text in this patch to start with.

OK, I had that at first but I did this to be more techincally correct.  I will
simplify as you suggest.

> > +    fdt_addr = ROUND_UP(load_start, 4);
> > +
> > +    fdt_pack(s->fdt);
> 
> fdt_pack() returns an error code -- you should check it.

OK.

> > +    /* copy in the device tree */
> > +    qemu_fdt_dumpdtb(s->fdt, fdtsize);
> > +
> > +    rom_add_blob_fixed_as("fdt", s->fdt, fdtsize, fdt_addr,
> > +                          &address_space_memory);
> > +
> > +    return fdt_addr;
> > +}
> > +
> > +static void openrisc_create_fdt(Or1ksimState *s,
> > +    const struct MemmapEntry *memmap, int num_cpus, uint64_t mem_size,
> > +    const char *cmdline)
> 
> Indentation.

Right, fixed.

> > +{
> > +    void *fdt;
> > +    int cpu;
> > +    char *nodename;
> > +    int pic_ph;
> > +
> > +    fdt = s->fdt = create_device_tree(&s->fdt_size);
> > +    if (!fdt) {
> > +        error_report("create_device_tree() failed");
> > +        exit(1);
> > +    }
> > +
> > +    qemu_fdt_setprop_string(fdt, "/", "compatible", "opencores,or1ksim");
> > +    qemu_fdt_setprop_cell(fdt, "/", "#address-cells", 0x1);
> > +    qemu_fdt_setprop_cell(fdt, "/", "#size-cells", 0x1);
> > +
> > +    nodename = g_strdup_printf("/memory@%lx",
> > +                               (long)memmap[OR1KSIM_DRAM].base);
> 
> Use the appropriate format string macro for the type, rather than
> casting to long (here and below).

Right good point.

> > +    qemu_fdt_add_subnode(fdt, nodename);
> > +    qemu_fdt_setprop_cells(fdt, nodename, "reg",
> > +                           memmap[OR1KSIM_DRAM].base, mem_size);
> > +    qemu_fdt_setprop_string(fdt, nodename, "device_type", "memory");
> > +    g_free(nodename);
> > +
> > +    qemu_fdt_add_subnode(fdt, "/cpus");
> > +    qemu_fdt_setprop_cell(fdt, "/cpus", "#size-cells", 0x0);
> > +    qemu_fdt_setprop_cell(fdt, "/cpus", "#address-cells", 0x1);
> > +
> > +    for (cpu = 0; cpu < num_cpus; cpu++) {
> > +        nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
> > +        qemu_fdt_add_subnode(fdt, nodename);
> > +        qemu_fdt_setprop_string(fdt, nodename, "compatible",
> > +                                "opencores,or1200-rtlsvn481");
> > +        qemu_fdt_setprop_cell(fdt, nodename, "reg", cpu);
> > +        qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency",
> > +                              OR1KSIM_CLK_MHZ);
> > +        g_free(nodename);
> > +    }
> > +
> > +    if (num_cpus > 0) {
> > +        nodename = g_strdup_printf("/ompic@%lx",
> > +                                   (long)memmap[OR1KSIM_OMPIC].base);
> > +        qemu_fdt_add_subnode(fdt, nodename);
> > +        qemu_fdt_setprop_string(fdt, nodename, "compatible", "openrisc,ompic");
> > +        qemu_fdt_setprop_cells(fdt, nodename, "reg",
> > +                               memmap[OR1KSIM_OMPIC].base,
> > +                               memmap[OR1KSIM_OMPIC].size);
> > +        qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0);
> > +        qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 0);
> > +        qemu_fdt_setprop_cell(fdt, nodename, "interrupts", OR1KSIM_OMPIC_IRQ);
> > +        g_free(nodename);
> >      }
> > +
> > +    nodename = (char *)"/pic";
> > +    qemu_fdt_add_subnode(fdt, nodename);
> > +    pic_ph = qemu_fdt_alloc_phandle(fdt);
> > +    qemu_fdt_setprop_string(fdt, nodename, "compatible",
> > +                            "opencores,or1k-pic-level");
> > +    qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 1);
> > +    qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0);
> > +    qemu_fdt_setprop_cell(fdt, nodename, "phandle", pic_ph);
> > +
> > +    qemu_fdt_setprop_cell(fdt, "/", "interrupt-parent", pic_ph);
> > +
> > +    if (nd_table[0].used) {
> > +        nodename = g_strdup_printf("/ethoc@%lx",
> > +                                   (long)memmap[OR1KSIM_ETHOC].base);
> > +        qemu_fdt_add_subnode(fdt, nodename);
> > +        qemu_fdt_setprop_string(fdt, nodename, "compatible", "opencores,ethoc");
> > +        qemu_fdt_setprop_cells(fdt, nodename, "reg",
> > +                               memmap[OR1KSIM_ETHOC].base,
> > +                               memmap[OR1KSIM_ETHOC].size);
> > +        qemu_fdt_setprop_cell(fdt, nodename, "interrupts", OR1KSIM_ETHOC_IRQ);
> > +        qemu_fdt_setprop(fdt, nodename, "big-endian", NULL, 0);
> > +
> > +        qemu_fdt_add_subnode(fdt, "/aliases");
> > +        qemu_fdt_setprop_string(fdt, "/aliases", "enet0", nodename);
> > +        g_free(nodename);
> > +    }
> > +
> > +    nodename = g_strdup_printf("/serial@%lx",
> > +                               (long)memmap[OR1KSIM_UART].base);
> > +    qemu_fdt_add_subnode(fdt, nodename);
> > +    qemu_fdt_setprop_string(fdt, nodename, "compatible", "ns16550a");
> > +    qemu_fdt_setprop_cells(fdt, nodename, "reg",
> > +                           memmap[OR1KSIM_UART].base,
> > +                           memmap[OR1KSIM_UART].size);
> > +    qemu_fdt_setprop_cell(fdt, nodename, "interrupts", OR1KSIM_UART_IRQ);
> > +    qemu_fdt_setprop_cell(fdt, nodename, "clock-frequency", OR1KSIM_CLK_MHZ);
> > +    qemu_fdt_setprop(fdt, nodename, "big-endian", NULL, 0);
> > +
> > +    qemu_fdt_add_subnode(fdt, "/chosen");
> > +    qemu_fdt_setprop_string(fdt, "/chosen", "stdout-path", nodename);
> > +    if (cmdline) {
> > +        qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> > +    }
> > +
> > +    qemu_fdt_setprop_string(fdt, "/aliases", "uart0", nodename);
> 
> I think the pattern the hw/arm/virt.c code uses, where the board
> code functions that create the UART, interrupt controller, etc
> devices on the board also have the code to add FDT nodes for them.
> Especially as the board gets new devices added over time, it's easier
> to check that the FDT nodes and the devices match up, and you don't
> have to awkwardly duplicate control-flow logic like "we only have
> an ethernet device if nd_table[0].used". But I won't insist that
> you rewrite this.

Right, this makes sense.  I might as well split it out.  I did it that way for
initrd.  I it may make sense to do for UART, OMPIC and Ethernet here.

> > +    g_free(nodename);
> >  }
> >
> >  static void openrisc_sim_init(MachineState *machine)
> > @@ -178,6 +323,7 @@ static void openrisc_sim_init(MachineState *machine)
> >      OpenRISCCPU *cpus[2] = {};
> >      Or1ksimState *s = OR1KSIM_MACHINE(machine);
> >      MemoryRegion *ram;
> > +    hwaddr load_addr;
> >      qemu_irq serial_irq;
> >      int n;
> >      unsigned int smp_cpus = machine->smp.cpus;
> > @@ -219,7 +365,11 @@ static void openrisc_sim_init(MachineState *machine)
> >      serial_mm_init(get_system_memory(), or1ksim_memmap[OR1KSIM_UART].base, 0,
> >                     serial_irq, 115200, serial_hd(0), DEVICE_NATIVE_ENDIAN);
> >
> > -    openrisc_load_kernel(ram_size, kernel_filename);
> > +    openrisc_create_fdt(s, or1ksim_memmap, smp_cpus, machine->ram_size,
> > +                        machine->kernel_cmdline);
> > +
> > +    load_addr = openrisc_load_kernel(ram_size, kernel_filename);
> > +    boot_info.fdt_addr = openrisc_load_fdt(s, load_addr, machine->ram_size);
> >  }
> 
> If the user doesn't specify a kernel file, we'll still load the FDT,
> at address zero. Is that sensible/intended behaviour ?

Good point,  I guess we can add some space to not override the interrupt
vectors.  The system booting with no kernel will probably not very useful
anyway.  But I imaging one could connect a debugger and load some binary into
memory and having the FDT in memory would be helpful.

So moving the fdt offset to 0 + 1-page would give enough space.  Does this sound
OK?

-Stafford

Re: [PATCH 3/4] hw/openrisc/openrisc_sim; Add support for loading a decice tree

[Peter Maydell]

On Thu, 17 Feb 2022 at 21:39, Stafford Horne <shorne@gmail.com> wrote:
>
> On Thu, Feb 17, 2022 at 06:18:58PM +0000, Peter Maydell wrote:
> > If the user doesn't specify a kernel file, we'll still load the FDT,
> > at address zero. Is that sensible/intended behaviour ?
>
> Good point,  I guess we can add some space to not override the interrupt
> vectors.  The system booting with no kernel will probably not very useful
> anyway.  But I imaging one could connect a debugger and load some binary into
> memory and having the FDT in memory would be helpful.
>
> So moving the fdt offset to 0 + 1-page would give enough space.  Does this sound
> OK?

I don't have a strong opinion -- you can not load the fdt, or you can
define a "this is probably sensible for firmware" FDT load location.
If you do define something like that you should document it, though.
Bear in mind that if you put the FDT at 0+1-page then it will prevent
users loading a bare-metal binary with eg the generic loader that starts
at address 0, though (because it will show up as two overlapping
ROM blobs). So unless you have a definite use case in mind that will
want the fdt in memory it might be better to not load it. You can always
add code to load it later if there is a concrete requirement later.

-- PMM