diff mbox series

[v2,2/6] hw/riscv: split fdt address calculation from fdt load

Message ID 20230116173420.1146808-3-dbarboza@ventanamicro.com (mailing list archive)
State New, archived
Headers show
Series riscv: fdt related cleanups | expand

Commit Message

Daniel Henrique Barboza Jan. 16, 2023, 5:34 p.m. UTC 
A common trend in other archs is to calculate the fdt address, which is
usually straightforward, and then calling a function that loads the
fdt/dtb by using that address.

riscv_load_fdt() is doing a bit too much in comparison. It's calculating
the fdt address via an elaborated heuristic to put the FDT at the bottom
of DRAM, and "bottom of DRAM" will vary across boards and
configurations, then it's actually loading the fdt, and finally it's
returning the fdt address used to the caller.

Reduce the existing complexity of riscv_load_fdt() by splitting its code
into a new function, riscv_compute_fdt_addr(), that will take care of
all fdt address logic. riscv_load_fdt() can then be a simple function
that just loads a fdt at the given fdt address.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c            | 24 ++++++++++++++++--------
 hw/riscv/microchip_pfsoc.c |  6 ++++--
 hw/riscv/sifive_u.c        |  7 ++++---
 hw/riscv/spike.c           |  6 +++---
 hw/riscv/virt.c            |  7 ++++---
 include/hw/riscv/boot.h    |  3 ++-
 6 files changed, 33 insertions(+), 20 deletions(-)

Comments

Alistair Francis Jan. 19, 2023, 2:13 a.m. UTC | #1 
On Tue, Jan 17, 2023 at 3:35 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> A common trend in other archs is to calculate the fdt address, which is
> usually straightforward, and then calling a function that loads the
> fdt/dtb by using that address.
>
> riscv_load_fdt() is doing a bit too much in comparison. It's calculating
> the fdt address via an elaborated heuristic to put the FDT at the bottom
> of DRAM, and "bottom of DRAM" will vary across boards and
> configurations, then it's actually loading the fdt, and finally it's
> returning the fdt address used to the caller.
>
> Reduce the existing complexity of riscv_load_fdt() by splitting its code
> into a new function, riscv_compute_fdt_addr(), that will take care of
> all fdt address logic. riscv_load_fdt() can then be a simple function
> that just loads a fdt at the given fdt address.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

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

Alistair

> ---
>  hw/riscv/boot.c            | 24 ++++++++++++++++--------
>  hw/riscv/microchip_pfsoc.c |  6 ++++--
>  hw/riscv/sifive_u.c        |  7 ++++---
>  hw/riscv/spike.c           |  6 +++---
>  hw/riscv/virt.c            |  7 ++++---
>  include/hw/riscv/boot.h    |  3 ++-
>  6 files changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index dc14d8cd14..b213a32157 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -249,9 +249,16 @@ void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>      }
>  }
>
> -uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> +/*
> + * The FDT should be put at the farthest point possible to
> + * avoid overwriting it with the kernel/initrd.
> + *
> + * The FDT is fdt_packed() during the calculation.
> + */
> +uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
> +                                void *fdt)
>  {
> -    uint64_t temp, fdt_addr;
> +    uint64_t temp;
>      hwaddr dram_end = dram_base + mem_size;
>      int ret = fdt_pack(fdt);
>      int fdtsize;
> @@ -272,11 +279,14 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>       * end of dram or 3GB whichever is lesser.
>       */
>      temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> -    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
>
> -    ret = fdt_pack(fdt);
> -    /* Should only fail if we've built a corrupted tree */
> -    g_assert(ret == 0);
> +    return QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
> +}
> +
> +void riscv_load_fdt(uint32_t fdt_addr, void *fdt)
> +{
> +    uint32_t fdtsize = fdt_totalsize(fdt);
> +
>      /* copy in the device tree */
>      qemu_fdt_dumpdtb(fdt, fdtsize);
>
> @@ -284,8 +294,6 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>                            &address_space_memory);
>      qemu_register_reset_nosnapshotload(qemu_fdt_randomize_seeds,
>                          rom_ptr_for_as(&address_space_memory, fdt_addr, fdtsize));
> -
> -    return fdt_addr;
>  }
>
>  void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 82ae5e7023..dcdbc2cac3 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -641,8 +641,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>          }
>
>          /* Compute the fdt load address in dram */
> -        fdt_load_addr = riscv_load_fdt(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> -                                       machine->ram_size, machine->fdt);
> +        fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> +                                              machine->ram_size, machine->fdt);
> +        riscv_load_fdt(fdt_load_addr, machine->fdt);
> +
>          /* Load the reset vector */
>          riscv_setup_rom_reset_vec(machine, &s->soc.u_cpus, firmware_load_addr,
>                                    memmap[MICROCHIP_PFSOC_ENVM_DATA].base,
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 2fb6ee231f..626d4dc2f3 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -616,9 +616,10 @@ static void sifive_u_machine_init(MachineState *machine)
>          kernel_entry = 0;
>      }
>
> -    /* Compute the fdt load address in dram */
> -    fdt_load_addr = riscv_load_fdt(memmap[SIFIVE_U_DEV_DRAM].base,
> -                                   machine->ram_size, machine->fdt);
> +    fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
> +                                           machine->ram_size, machine->fdt);
> +    riscv_load_fdt(fdt_load_addr, machine->fdt);
> +
>      if (!riscv_is_32bit(&s->soc.u_cpus)) {
>          start_addr_hi32 = (uint64_t)start_addr >> 32;
>      }
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index badc11ec43..88b9fdfc36 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -324,9 +324,9 @@ static void spike_board_init(MachineState *machine)
>          kernel_entry = 0;
>      }
>
> -    /* Compute the fdt load address in dram */
> -    fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base,
> -                                   machine->ram_size, machine->fdt);
> +    fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
> +                                           machine->ram_size, machine->fdt);
> +    riscv_load_fdt(fdt_load_addr, machine->fdt);
>
>      /* load the reset vector */
>      riscv_setup_rom_reset_vec(machine, &s->soc[0], memmap[SPIKE_DRAM].base,
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index e6d4f06e8d..839dfaa125 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1307,9 +1307,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
>          start_addr = virt_memmap[VIRT_FLASH].base;
>      }
>
> -    /* Compute the fdt load address in dram */
> -    fdt_load_addr = riscv_load_fdt(memmap[VIRT_DRAM].base,
> -                                   machine->ram_size, machine->fdt);
> +    fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
> +                                           machine->ram_size, machine->fdt);
> +    riscv_load_fdt(fdt_load_addr, machine->fdt);
> +
>      /* load the reset vector */
>      riscv_setup_rom_reset_vec(machine, &s->soc[0], start_addr,
>                                virt_memmap[VIRT_MROM].base,
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index f94653a09b..9aea7b9c46 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -47,7 +47,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong firmware_end_addr,
>                                 symbol_fn_t sym_cb);
>  void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
> -uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
> +uint32_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size, void *fdt);
> +void riscv_load_fdt(uint32_t fdt_addr, void *fdt);
>  void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>                                 hwaddr saddr,
>                                 hwaddr rom_base, hwaddr rom_size,
> --
> 2.39.0
>
>
diff mbox series

Patch

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index dc14d8cd14..b213a32157 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -249,9 +249,16 @@  void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
     }
 }
 
-uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
+/*
+ * The FDT should be put at the farthest point possible to
+ * avoid overwriting it with the kernel/initrd.
+ *
+ * The FDT is fdt_packed() during the calculation.
+ */
+uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
+                                void *fdt)
 {
-    uint64_t temp, fdt_addr;
+    uint64_t temp;
     hwaddr dram_end = dram_base + mem_size;
     int ret = fdt_pack(fdt);
     int fdtsize;
@@ -272,11 +279,14 @@  uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
      * end of dram or 3GB whichever is lesser.
      */
     temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
-    fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
 
-    ret = fdt_pack(fdt);
-    /* Should only fail if we've built a corrupted tree */
-    g_assert(ret == 0);
+    return QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
+}
+
+void riscv_load_fdt(uint32_t fdt_addr, void *fdt)
+{
+    uint32_t fdtsize = fdt_totalsize(fdt);
+
     /* copy in the device tree */
     qemu_fdt_dumpdtb(fdt, fdtsize);
 
@@ -284,8 +294,6 @@  uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
                           &address_space_memory);
     qemu_register_reset_nosnapshotload(qemu_fdt_randomize_seeds,
                         rom_ptr_for_as(&address_space_memory, fdt_addr, fdtsize));
-
-    return fdt_addr;
 }
 
 void riscv_rom_copy_firmware_info(MachineState *machine, hwaddr rom_base,
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 82ae5e7023..dcdbc2cac3 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -641,8 +641,10 @@  static void microchip_icicle_kit_machine_init(MachineState *machine)
         }
 
         /* Compute the fdt load address in dram */
-        fdt_load_addr = riscv_load_fdt(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
-                                       machine->ram_size, machine->fdt);
+        fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
+                                              machine->ram_size, machine->fdt);
+        riscv_load_fdt(fdt_load_addr, machine->fdt);
+
         /* Load the reset vector */
         riscv_setup_rom_reset_vec(machine, &s->soc.u_cpus, firmware_load_addr,
                                   memmap[MICROCHIP_PFSOC_ENVM_DATA].base,
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 2fb6ee231f..626d4dc2f3 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -616,9 +616,10 @@  static void sifive_u_machine_init(MachineState *machine)
         kernel_entry = 0;
     }
 
-    /* Compute the fdt load address in dram */
-    fdt_load_addr = riscv_load_fdt(memmap[SIFIVE_U_DEV_DRAM].base,
-                                   machine->ram_size, machine->fdt);
+    fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
+                                           machine->ram_size, machine->fdt);
+    riscv_load_fdt(fdt_load_addr, machine->fdt);
+
     if (!riscv_is_32bit(&s->soc.u_cpus)) {
         start_addr_hi32 = (uint64_t)start_addr >> 32;
     }
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index badc11ec43..88b9fdfc36 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -324,9 +324,9 @@  static void spike_board_init(MachineState *machine)
         kernel_entry = 0;
     }
 
-    /* Compute the fdt load address in dram */
-    fdt_load_addr = riscv_load_fdt(memmap[SPIKE_DRAM].base,
-                                   machine->ram_size, machine->fdt);
+    fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
+                                           machine->ram_size, machine->fdt);
+    riscv_load_fdt(fdt_load_addr, machine->fdt);
 
     /* load the reset vector */
     riscv_setup_rom_reset_vec(machine, &s->soc[0], memmap[SPIKE_DRAM].base,
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index e6d4f06e8d..839dfaa125 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1307,9 +1307,10 @@  static void virt_machine_done(Notifier *notifier, void *data)
         start_addr = virt_memmap[VIRT_FLASH].base;
     }
 
-    /* Compute the fdt load address in dram */
-    fdt_load_addr = riscv_load_fdt(memmap[VIRT_DRAM].base,
-                                   machine->ram_size, machine->fdt);
+    fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
+                                           machine->ram_size, machine->fdt);
+    riscv_load_fdt(fdt_load_addr, machine->fdt);
+
     /* load the reset vector */
     riscv_setup_rom_reset_vec(machine, &s->soc[0], start_addr,
                               virt_memmap[VIRT_MROM].base,
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index f94653a09b..9aea7b9c46 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -47,7 +47,8 @@  target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong firmware_end_addr,
                                symbol_fn_t sym_cb);
 void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);
-uint64_t riscv_load_fdt(hwaddr dram_start, uint64_t dram_size, void *fdt);
+uint32_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size, void *fdt);
+void riscv_load_fdt(uint32_t fdt_addr, void *fdt);
 void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
                                hwaddr saddr,
                                hwaddr rom_base, hwaddr rom_size,