[PATCH 0/2] Support 64-bit address of initrd

[PATCH 0/2] Support 64-bit address of initrd

[Jim Shu]

Support to load DTB after 3GB on RV64 system, so that larger initrd
doesn't be overlapped to DTB. Update initrd DT to support different
"#address-cells".

Verify the patch via running 4GB initramfs on the virt machine.

Jim Shu (2):
  hw/riscv: Support to load DTB after 3GB memory on 64-bit system.
  hw/riscv: Support different address-cells for initrd

 hw/riscv/boot.c            | 22 ++++++++++++++++++----
 hw/riscv/microchip_pfsoc.c |  4 ++--
 hw/riscv/sifive_u.c        |  4 ++--
 hw/riscv/spike.c           |  4 ++--
 hw/riscv/virt.c            |  2 +-
 include/hw/riscv/boot.h    |  2 +-
 6 files changed, 26 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 1/2] hw/riscv: Support to load DTB after 3GB memory on 64-bit system.

[Jim Shu]

Larger initrd image will overlap the DTB at 3GB address. Since 64-bit
system doesn't have 32-bit addressable issue, we just load DTB to the end
of dram in 64-bit system.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 hw/riscv/boot.c            | 8 ++++++--
 hw/riscv/microchip_pfsoc.c | 4 ++--
 hw/riscv/sifive_u.c        | 4 ++--
 hw/riscv/spike.c           | 4 ++--
 hw/riscv/virt.c            | 2 +-
 include/hw/riscv/boot.h    | 2 +-
 6 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 9115ecd91f..ad45bd7a6a 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -293,7 +293,7 @@ out:
  * The FDT is fdt_packed() during the calculation.
  */
 uint64_t riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
-                                MachineState *ms)
+                                MachineState *ms, RISCVHartArrayState *harts)
 {
     int ret = fdt_pack(ms->fdt);
     hwaddr dram_end, temp;
@@ -321,7 +321,11 @@ uint64_t riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
      * Thus, put it at an 2MB aligned address that less than fdt size from the
      * end of dram or 3GB whichever is lesser.
      */
-    temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
+    if (!riscv_is_32bit(harts)) {
+        temp = dram_end;
+    } else {
+        temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
+    }
 
     return QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
 }
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index f9a3b43d2e..ba8b0a2c26 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -519,7 +519,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
     bool kernel_as_payload = false;
     target_ulong firmware_end_addr, kernel_start_addr;
     uint64_t kernel_entry;
-    uint32_t fdt_load_addr;
+    uint64_t fdt_load_addr;
     DriveInfo *dinfo = drive_get(IF_SD, 0, 0);
 
     /* Sanity check on RAM size */
@@ -625,7 +625,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
         /* Compute the fdt load address in dram */
         fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
                                                memmap[MICROCHIP_PFSOC_DRAM_LO].size,
-                                               machine);
+                                               machine, &s->soc.u_cpus);
         riscv_load_fdt(fdt_load_addr, machine->fdt);
 
         /* Load the reset vector */
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 9b3dcf3a7a..fd974f2357 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -519,7 +519,7 @@ static void sifive_u_machine_init(MachineState *machine)
     const char *firmware_name;
     uint32_t start_addr_hi32 = 0x00000000;
     int i;
-    uint32_t fdt_load_addr;
+    uint64_t fdt_load_addr;
     uint64_t kernel_entry;
     DriveInfo *dinfo;
     BlockBackend *blk;
@@ -606,7 +606,7 @@ static void sifive_u_machine_init(MachineState *machine)
 
     fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
                                            memmap[SIFIVE_U_DEV_DRAM].size,
-                                           machine);
+                                           machine, &s->soc.u_cpus);
     riscv_load_fdt(fdt_load_addr, machine->fdt);
 
     if (!riscv_is_32bit(&s->soc.u_cpus)) {
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index fceb91d946..acd7ab1ae1 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -201,7 +201,7 @@ static void spike_board_init(MachineState *machine)
     hwaddr firmware_load_addr = memmap[SPIKE_DRAM].base;
     target_ulong kernel_start_addr;
     char *firmware_name;
-    uint32_t fdt_load_addr;
+    uint64_t fdt_load_addr;
     uint64_t kernel_entry;
     char *soc_name;
     int i, base_hartid, hart_count;
@@ -317,7 +317,7 @@ static void spike_board_init(MachineState *machine)
 
     fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
                                            memmap[SPIKE_DRAM].size,
-                                           machine);
+                                           machine, &s->soc[0]);
     riscv_load_fdt(fdt_load_addr, machine->fdt);
 
     /* load the reset vector */
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index ee3129f3b3..cfbeeaf7d5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1399,7 +1399,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
 
     fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
                                            memmap[VIRT_DRAM].size,
-                                           machine);
+                                           machine, &s->soc[0]);
     riscv_load_fdt(fdt_load_addr, machine->fdt);
 
     /* load the reset vector */
diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
index 18bfe9f7bf..7ccbe5f62a 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -49,7 +49,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
                                bool load_initrd,
                                symbol_fn_t sym_cb);
 uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
-                                MachineState *ms);
+                                MachineState *ms, RISCVHartArrayState *harts);
 void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
 void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
                                hwaddr saddr,
-- 
2.17.1

[PATCH 2/2] hw/riscv: Support different address-cells for initrd

[Jim Shu]

The cells of 'initrd-start/end' should follow the '#address-cell'.
QEMU API could support 1 and 2 cells.

Signed-off-by: Jim Shu <jim.shu@sifive.com>
---
 hw/riscv/boot.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index ad45bd7a6a..76b099c696 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -182,6 +182,7 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
     void *fdt = machine->fdt;
     hwaddr start, end;
     ssize_t size;
+    uint32_t acells;
 
     g_assert(filename != NULL);
 
@@ -209,9 +210,18 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
 
     /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
     if (fdt) {
+        acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
+                                       NULL, NULL);
+        if (acells == 0) {
+            error_report("dtb file invalid (#address-cells 0)");
+            exit(1);
+        }
+
         end = start + size;
-        qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start", start);
-        qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end", end);
+        qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
+                                     acells, start);
+        qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
+                                     acells, end);
     }
 }
 
-- 
2.17.1

Re: [PATCH 1/2] hw/riscv: Support to load DTB after 3GB memory on 64-bit system.

[Daniel Henrique Barboza]

On 10/21/24 1:09 AM, Jim Shu wrote:
> Larger initrd image will overlap the DTB at 3GB address. Since 64-bit
> system doesn't have 32-bit addressable issue, we just load DTB to the end
> of dram in 64-bit system.
> 
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> ---
>   hw/riscv/boot.c            | 8 ++++++--
>   hw/riscv/microchip_pfsoc.c | 4 ++--
>   hw/riscv/sifive_u.c        | 4 ++--
>   hw/riscv/spike.c           | 4 ++--
>   hw/riscv/virt.c            | 2 +-
>   include/hw/riscv/boot.h    | 2 +-
>   6 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 9115ecd91f..ad45bd7a6a 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -293,7 +293,7 @@ out:
>    * The FDT is fdt_packed() during the calculation.
>    */
>   uint64_t riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
> -                                MachineState *ms)
> +                                MachineState *ms, RISCVHartArrayState *harts)
>   {
>       int ret = fdt_pack(ms->fdt);
>       hwaddr dram_end, temp;
> @@ -321,7 +321,11 @@ uint64_t riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
>        * Thus, put it at an 2MB aligned address that less than fdt size from the
>        * end of dram or 3GB whichever is lesser.
>        */
> -    temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> +    if (!riscv_is_32bit(harts)) {
> +        temp = dram_end;
> +    } else {
> +        temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> +    }

I think it's a fine idea to do a different computation if we're running a 64 bit
CPU but this alone won't fix the issue you're reporting.

Running a 64-bit CPU does not guarantee that 'dram_end' will be enough to fit
kernel+initrd and the fdt. There's no correlation between running 64 bits and having
enough RAM to fit everything, i.e. you can run a 64-bit CPU with short RAM and end
up with the same problem. There's also the board mem topology to account for: the
Microchip board will always use a 1GB ram bank ('dram_end' will be always 1Gb max)
running 32 or 64 bit CPUs.

It seems that we also need to consider the end of kernel+initrd to properly tune
where to put the fdt, erroring out if we don't have enough size for everything.
This would make the logic work regardless of the word size of the CPU.

This information is calculated in riscv_load_kernel(), which is currently
returning only the kernel start addr.  We have the information we need written in
the FDT as "/chosen/linux,initrd-end" but that isn't enough because this attribute
is written only if we have an initrd. We would hit the same problem again if we
have a big enough kernel in an low enough RAM env.

First thing that comes to mind is to use an abstraction like

typedef struct RISCVBootProps {
     uint64_t kernel_start;
     uint64_t kernel_end;
     bool is_64bit;
} RISCVKernelProps;

And use it to allow riscv_load_kernel() to write both kernel_start (the current
return value) and kernel_end (that can be either kernel_start + kernel_size or,
if we have an initrd, the value written in linux,initrd-end). riscv_compute_fdt_addr()
would then use this as an extra arg and then we have the 'kernel_end' value to
calculate the fdt_addr too.



In fact, I think this can be further extended to fully encapsulate the boot process
of all boards, that are kind of doing the same thing with all these helpers, into a
single 'super function' that would receive a struct like the one from above (with
more properties), setting all the boot parameters the board needs, pass it to a single
function that does everything and just use the result as each board wants. This
is something that we might play around with but it'll be something for the next
release.


Thanks,

Daniel
   
  

>   
>       return QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
>   }
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index f9a3b43d2e..ba8b0a2c26 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -519,7 +519,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>       bool kernel_as_payload = false;
>       target_ulong firmware_end_addr, kernel_start_addr;
>       uint64_t kernel_entry;
> -    uint32_t fdt_load_addr;
> +    uint64_t fdt_load_addr;
>       DriveInfo *dinfo = drive_get(IF_SD, 0, 0);
>   
>       /* Sanity check on RAM size */
> @@ -625,7 +625,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>           /* Compute the fdt load address in dram */
>           fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>                                                  memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> -                                               machine);
> +                                               machine, &s->soc.u_cpus);
>           riscv_load_fdt(fdt_load_addr, machine->fdt);
>   
>           /* Load the reset vector */
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 9b3dcf3a7a..fd974f2357 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -519,7 +519,7 @@ static void sifive_u_machine_init(MachineState *machine)
>       const char *firmware_name;
>       uint32_t start_addr_hi32 = 0x00000000;
>       int i;
> -    uint32_t fdt_load_addr;
> +    uint64_t fdt_load_addr;
>       uint64_t kernel_entry;
>       DriveInfo *dinfo;
>       BlockBackend *blk;
> @@ -606,7 +606,7 @@ static void sifive_u_machine_init(MachineState *machine)
>   
>       fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
>                                              memmap[SIFIVE_U_DEV_DRAM].size,
> -                                           machine);
> +                                           machine, &s->soc.u_cpus);
>       riscv_load_fdt(fdt_load_addr, machine->fdt);
>   
>       if (!riscv_is_32bit(&s->soc.u_cpus)) {
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index fceb91d946..acd7ab1ae1 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -201,7 +201,7 @@ static void spike_board_init(MachineState *machine)
>       hwaddr firmware_load_addr = memmap[SPIKE_DRAM].base;
>       target_ulong kernel_start_addr;
>       char *firmware_name;
> -    uint32_t fdt_load_addr;
> +    uint64_t fdt_load_addr;
>       uint64_t kernel_entry;
>       char *soc_name;
>       int i, base_hartid, hart_count;
> @@ -317,7 +317,7 @@ static void spike_board_init(MachineState *machine)
>   
>       fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
>                                              memmap[SPIKE_DRAM].size,
> -                                           machine);
> +                                           machine, &s->soc[0]);
>       riscv_load_fdt(fdt_load_addr, machine->fdt);
>   
>       /* load the reset vector */
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index ee3129f3b3..cfbeeaf7d5 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1399,7 +1399,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
>   
>       fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
>                                              memmap[VIRT_DRAM].size,
> -                                           machine);
> +                                           machine, &s->soc[0]);
>       riscv_load_fdt(fdt_load_addr, machine->fdt);
>   
>       /* load the reset vector */
> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> index 18bfe9f7bf..7ccbe5f62a 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -49,7 +49,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
>                                  bool load_initrd,
>                                  symbol_fn_t sym_cb);
>   uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
> -                                MachineState *ms);
> +                                MachineState *ms, RISCVHartArrayState *harts);
>   void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
>   void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>                                  hwaddr saddr,

Re: [PATCH 2/2] hw/riscv: Support different address-cells for initrd

[Daniel Henrique Barboza]

On 10/21/24 1:09 AM, Jim Shu wrote:
> The cells of 'initrd-start/end' should follow the '#address-cell'.
> QEMU API could support 1 and 2 cells.
> 
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> ---
>   hw/riscv/boot.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index ad45bd7a6a..76b099c696 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -182,6 +182,7 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>       void *fdt = machine->fdt;
>       hwaddr start, end;
>       ssize_t size;
> +    uint32_t acells;
>   
>       g_assert(filename != NULL);
>   
> @@ -209,9 +210,18 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
>   
>       /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
>       if (fdt) {
> +        acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
> +                                       NULL, NULL);
> +        if (acells == 0) {
> +            error_report("dtb file invalid (#address-cells 0)");
> +            exit(1);
> +        }
> +
>           end = start + size;
> -        qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start", start);
> -        qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end", end);
> +        qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
> +                                     acells, start);
> +        qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
> +                                     acells, end);
>       }

Is this a legal format for linux,initrd-start and linux,initrd-end?

This link:

https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt

Defines both attributes as:

"These properties hold the physical start and end address of an initrd that's
loaded by the bootloader."

So I'm not sure if this format you're using here is valid.


Conor, care to weight in? Thanks,

Daniel

>   }
>   

Re: [PATCH 1/2] hw/riscv: Support to load DTB after 3GB memory on 64-bit system.

[Jim Shu]

On Mon, Oct 21, 2024 at 9:42 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 10/21/24 1:09 AM, Jim Shu wrote:
> > Larger initrd image will overlap the DTB at 3GB address. Since 64-bit
> > system doesn't have 32-bit addressable issue, we just load DTB to the end
> > of dram in 64-bit system.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >   hw/riscv/boot.c            | 8 ++++++--
> >   hw/riscv/microchip_pfsoc.c | 4 ++--
> >   hw/riscv/sifive_u.c        | 4 ++--
> >   hw/riscv/spike.c           | 4 ++--
> >   hw/riscv/virt.c            | 2 +-
> >   include/hw/riscv/boot.h    | 2 +-
> >   6 files changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 9115ecd91f..ad45bd7a6a 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -293,7 +293,7 @@ out:
> >    * The FDT is fdt_packed() during the calculation.
> >    */
> >   uint64_t riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
> > -                                MachineState *ms)
> > +                                MachineState *ms, RISCVHartArrayState *harts)
> >   {
> >       int ret = fdt_pack(ms->fdt);
> >       hwaddr dram_end, temp;
> > @@ -321,7 +321,11 @@ uint64_t riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
> >        * Thus, put it at an 2MB aligned address that less than fdt size from the
> >        * end of dram or 3GB whichever is lesser.
> >        */
> > -    temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> > +    if (!riscv_is_32bit(harts)) {
> > +        temp = dram_end;
> > +    } else {
> > +        temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
> > +    }
>
> I think it's a fine idea to do a different computation if we're running a 64 bit
> CPU but this alone won't fix the issue you're reporting.
>
> Running a 64-bit CPU does not guarantee that 'dram_end' will be enough to fit
> kernel+initrd and the fdt. There's no correlation between running 64 bits and having
> enough RAM to fit everything, i.e. you can run a 64-bit CPU with short RAM and end
> up with the same problem. There's also the board mem topology to account for: the
> Microchip board will always use a 1GB ram bank ('dram_end' will be always 1Gb max)
> running 32 or 64 bit CPUs.
>
> It seems that we also need to consider the end of kernel+initrd to properly tune
> where to put the fdt, erroring out if we don't have enough size for everything.
> This would make the logic work regardless of the word size of the CPU.
>
> This information is calculated in riscv_load_kernel(), which is currently
> returning only the kernel start addr.  We have the information we need written in
> the FDT as "/chosen/linux,initrd-end" but that isn't enough because this attribute
> is written only if we have an initrd. We would hit the same problem again if we
> have a big enough kernel in an low enough RAM env.
>
> First thing that comes to mind is to use an abstraction like
>
> typedef struct RISCVBootProps {
>      uint64_t kernel_start;
>      uint64_t kernel_end;
>      bool is_64bit;
> } RISCVKernelProps;
>
> And use it to allow riscv_load_kernel() to write both kernel_start (the current
> return value) and kernel_end (that can be either kernel_start + kernel_size or,
> if we have an initrd, the value written in linux,initrd-end). riscv_compute_fdt_addr()
> would then use this as an extra arg and then we have the 'kernel_end' value to
> calculate the fdt_addr too.
>
>
>
> In fact, I think this can be further extended to fully encapsulate the boot process
> of all boards, that are kind of doing the same thing with all these helpers, into a
> single 'super function' that would receive a struct like the one from above (with
> more properties), setting all the boot parameters the board needs, pass it to a single
> function that does everything and just use the result as each board wants. This
> is something that we might play around with but it'll be something for the next
> release.
>
>
> Thanks,
>
> Daniel

Thanks for the feedback. I can try to add the checking of overlapping
between initrd and dtb in the V2 patch.
I think it may change some boot APIs in "hw/riscv/boot.c"

ARM has an abstraction of the most info in `struct arm_boot_info`, so
it can add checkings in the functions like
`arm_setup_direct_kernel_boot()`.
We may do the similar work but be simpler at first.


Thanks,
Jim Shu

>
>
>
> >
> >       return QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
> >   }
> > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> > index f9a3b43d2e..ba8b0a2c26 100644
> > --- a/hw/riscv/microchip_pfsoc.c
> > +++ b/hw/riscv/microchip_pfsoc.c
> > @@ -519,7 +519,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> >       bool kernel_as_payload = false;
> >       target_ulong firmware_end_addr, kernel_start_addr;
> >       uint64_t kernel_entry;
> > -    uint32_t fdt_load_addr;
> > +    uint64_t fdt_load_addr;
> >       DriveInfo *dinfo = drive_get(IF_SD, 0, 0);
> >
> >       /* Sanity check on RAM size */
> > @@ -625,7 +625,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
> >           /* Compute the fdt load address in dram */
> >           fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
> >                                                  memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> > -                                               machine);
> > +                                               machine, &s->soc.u_cpus);
> >           riscv_load_fdt(fdt_load_addr, machine->fdt);
> >
> >           /* Load the reset vector */
> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> > index 9b3dcf3a7a..fd974f2357 100644
> > --- a/hw/riscv/sifive_u.c
> > +++ b/hw/riscv/sifive_u.c
> > @@ -519,7 +519,7 @@ static void sifive_u_machine_init(MachineState *machine)
> >       const char *firmware_name;
> >       uint32_t start_addr_hi32 = 0x00000000;
> >       int i;
> > -    uint32_t fdt_load_addr;
> > +    uint64_t fdt_load_addr;
> >       uint64_t kernel_entry;
> >       DriveInfo *dinfo;
> >       BlockBackend *blk;
> > @@ -606,7 +606,7 @@ static void sifive_u_machine_init(MachineState *machine)
> >
> >       fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
> >                                              memmap[SIFIVE_U_DEV_DRAM].size,
> > -                                           machine);
> > +                                           machine, &s->soc.u_cpus);
> >       riscv_load_fdt(fdt_load_addr, machine->fdt);
> >
> >       if (!riscv_is_32bit(&s->soc.u_cpus)) {
> > diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> > index fceb91d946..acd7ab1ae1 100644
> > --- a/hw/riscv/spike.c
> > +++ b/hw/riscv/spike.c
> > @@ -201,7 +201,7 @@ static void spike_board_init(MachineState *machine)
> >       hwaddr firmware_load_addr = memmap[SPIKE_DRAM].base;
> >       target_ulong kernel_start_addr;
> >       char *firmware_name;
> > -    uint32_t fdt_load_addr;
> > +    uint64_t fdt_load_addr;
> >       uint64_t kernel_entry;
> >       char *soc_name;
> >       int i, base_hartid, hart_count;
> > @@ -317,7 +317,7 @@ static void spike_board_init(MachineState *machine)
> >
> >       fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
> >                                              memmap[SPIKE_DRAM].size,
> > -                                           machine);
> > +                                           machine, &s->soc[0]);
> >       riscv_load_fdt(fdt_load_addr, machine->fdt);
> >
> >       /* load the reset vector */
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index ee3129f3b3..cfbeeaf7d5 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -1399,7 +1399,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
> >
> >       fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
> >                                              memmap[VIRT_DRAM].size,
> > -                                           machine);
> > +                                           machine, &s->soc[0]);
> >       riscv_load_fdt(fdt_load_addr, machine->fdt);
> >
> >       /* load the reset vector */
> > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
> > index 18bfe9f7bf..7ccbe5f62a 100644
> > --- a/include/hw/riscv/boot.h
> > +++ b/include/hw/riscv/boot.h
> > @@ -49,7 +49,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
> >                                  bool load_initrd,
> >                                  symbol_fn_t sym_cb);
> >   uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
> > -                                MachineState *ms);
> > +                                MachineState *ms, RISCVHartArrayState *harts);
> >   void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
> >   void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
> >                                  hwaddr saddr,

Re: [PATCH 2/2] hw/riscv: Support different address-cells for initrd

[Jim Shu]

Compare Linux code to handle "linux,initrd-start" [1] and
"linux,usable-memory-range" [2], I think the initrd code doesn't check
root #address-cells.
Using the origin code of "u64" is okay. I can remove this commit in
the next version.

[1] https://elixir.bootlin.com/linux/v6.11.4/source/drivers/of/fdt.c#L785
[2] https://elixir.bootlin.com/linux/v6.11.4/source/drivers/of/fdt.c#L857

Thanks,
Jim Shu

On Tue, Oct 22, 2024 at 3:30 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 10/21/24 1:09 AM, Jim Shu wrote:
> > The cells of 'initrd-start/end' should follow the '#address-cell'.
> > QEMU API could support 1 and 2 cells.
> >
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >   hw/riscv/boot.c | 14 ++++++++++++--
> >   1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index ad45bd7a6a..76b099c696 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -182,6 +182,7 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> >       void *fdt = machine->fdt;
> >       hwaddr start, end;
> >       ssize_t size;
> > +    uint32_t acells;
> >
> >       g_assert(filename != NULL);
> >
> > @@ -209,9 +210,18 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> >
> >       /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
> >       if (fdt) {
> > +        acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
> > +                                       NULL, NULL);
> > +        if (acells == 0) {
> > +            error_report("dtb file invalid (#address-cells 0)");
> > +            exit(1);
> > +        }
> > +
> >           end = start + size;
> > -        qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start", start);
> > -        qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end", end);
> > +        qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
> > +                                     acells, start);
> > +        qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
> > +                                     acells, end);
> >       }
>
> Is this a legal format for linux,initrd-start and linux,initrd-end?
>
> This link:
>
> https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt
>
> Defines both attributes as:
>
> "These properties hold the physical start and end address of an initrd that's
> loaded by the bootloader."
>
> So I'm not sure if this format you're using here is valid.
>
>
> Conor, care to weight in? Thanks,
>
> Daniel
>
> >   }
> >

Re: [PATCH 1/2] hw/riscv: Support to load DTB after 3GB memory on 64-bit system.

[Daniel Henrique Barboza]

On 10/23/24 7:19 AM, Jim Shu wrote:
> On Mon, Oct 21, 2024 at 9:42 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 10/21/24 1:09 AM, Jim Shu wrote:
>>> Larger initrd image will overlap the DTB at 3GB address. Since 64-bit
>>> system doesn't have 32-bit addressable issue, we just load DTB to the end
>>> of dram in 64-bit system.
>>>
>>> Signed-off-by: Jim Shu <jim.shu@sifive.com>
>>> ---
>>>    hw/riscv/boot.c            | 8 ++++++--
>>>    hw/riscv/microchip_pfsoc.c | 4 ++--
>>>    hw/riscv/sifive_u.c        | 4 ++--
>>>    hw/riscv/spike.c           | 4 ++--
>>>    hw/riscv/virt.c            | 2 +-
>>>    include/hw/riscv/boot.h    | 2 +-
>>>    6 files changed, 14 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>>> index 9115ecd91f..ad45bd7a6a 100644
>>> --- a/hw/riscv/boot.c
>>> +++ b/hw/riscv/boot.c
>>> @@ -293,7 +293,7 @@ out:
>>>     * The FDT is fdt_packed() during the calculation.
>>>     */
>>>    uint64_t riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
>>> -                                MachineState *ms)
>>> +                                MachineState *ms, RISCVHartArrayState *harts)
>>>    {
>>>        int ret = fdt_pack(ms->fdt);
>>>        hwaddr dram_end, temp;
>>> @@ -321,7 +321,11 @@ uint64_t riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
>>>         * Thus, put it at an 2MB aligned address that less than fdt size from the
>>>         * end of dram or 3GB whichever is lesser.
>>>         */
>>> -    temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
>>> +    if (!riscv_is_32bit(harts)) {
>>> +        temp = dram_end;
>>> +    } else {
>>> +        temp = (dram_base < 3072 * MiB) ? MIN(dram_end, 3072 * MiB) : dram_end;
>>> +    }
>>
>> I think it's a fine idea to do a different computation if we're running a 64 bit
>> CPU but this alone won't fix the issue you're reporting.
>>
>> Running a 64-bit CPU does not guarantee that 'dram_end' will be enough to fit
>> kernel+initrd and the fdt. There's no correlation between running 64 bits and having
>> enough RAM to fit everything, i.e. you can run a 64-bit CPU with short RAM and end
>> up with the same problem. There's also the board mem topology to account for: the
>> Microchip board will always use a 1GB ram bank ('dram_end' will be always 1Gb max)
>> running 32 or 64 bit CPUs.
>>
>> It seems that we also need to consider the end of kernel+initrd to properly tune
>> where to put the fdt, erroring out if we don't have enough size for everything.
>> This would make the logic work regardless of the word size of the CPU.
>>
>> This information is calculated in riscv_load_kernel(), which is currently
>> returning only the kernel start addr.  We have the information we need written in
>> the FDT as "/chosen/linux,initrd-end" but that isn't enough because this attribute
>> is written only if we have an initrd. We would hit the same problem again if we
>> have a big enough kernel in an low enough RAM env.
>>
>> First thing that comes to mind is to use an abstraction like
>>
>> typedef struct RISCVBootProps {
>>       uint64_t kernel_start;
>>       uint64_t kernel_end;
>>       bool is_64bit;
>> } RISCVKernelProps;
>>
>> And use it to allow riscv_load_kernel() to write both kernel_start (the current
>> return value) and kernel_end (that can be either kernel_start + kernel_size or,
>> if we have an initrd, the value written in linux,initrd-end). riscv_compute_fdt_addr()
>> would then use this as an extra arg and then we have the 'kernel_end' value to
>> calculate the fdt_addr too.
>>
>>
>>
>> In fact, I think this can be further extended to fully encapsulate the boot process
>> of all boards, that are kind of doing the same thing with all these helpers, into a
>> single 'super function' that would receive a struct like the one from above (with
>> more properties), setting all the boot parameters the board needs, pass it to a single
>> function that does everything and just use the result as each board wants. This
>> is something that we might play around with but it'll be something for the next
>> release.
>>
>>
>> Thanks,
>>
>> Daniel
> 
> Thanks for the feedback. I can try to add the checking of overlapping
> between initrd and dtb in the V2 patch.
> I think it may change some boot APIs in "hw/riscv/boot.c"
> 
> ARM has an abstraction of the most info in `struct arm_boot_info`, so
> it can add checkings in the functions like
> `arm_setup_direct_kernel_boot()`.
> We may do the similar work but be simpler at first.

Don't need to go too deep right now. You can focus on fixing the problem you
have at hand first, adding a 'struct riscv_boot_info' if you want to take an
inspiration from ARM, and passing just the kernel_start and kernel_end attrs
around. In a second step we can use the same struct, extend it and refactor
hw/riscv/boot.c (and its callers) accordingly.

If we could make the board populate a riscv_boot_info struct and then just do
a single call to a riscv_machine_boot() function that would do everything,
returning the updated struct to the board, that would be great.


ps: all function names I'm using up there are tentative. Feel free to name the
functions whatever you think fits best. Thanks,


Daniel

> 
> 
> Thanks,
> Jim Shu
> 
>>
>>
>>
>>>
>>>        return QEMU_ALIGN_DOWN(temp - fdtsize, 2 * MiB);
>>>    }
>>> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
>>> index f9a3b43d2e..ba8b0a2c26 100644
>>> --- a/hw/riscv/microchip_pfsoc.c
>>> +++ b/hw/riscv/microchip_pfsoc.c
>>> @@ -519,7 +519,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>>>        bool kernel_as_payload = false;
>>>        target_ulong firmware_end_addr, kernel_start_addr;
>>>        uint64_t kernel_entry;
>>> -    uint32_t fdt_load_addr;
>>> +    uint64_t fdt_load_addr;
>>>        DriveInfo *dinfo = drive_get(IF_SD, 0, 0);
>>>
>>>        /* Sanity check on RAM size */
>>> @@ -625,7 +625,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>>>            /* Compute the fdt load address in dram */
>>>            fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,
>>>                                                   memmap[MICROCHIP_PFSOC_DRAM_LO].size,
>>> -                                               machine);
>>> +                                               machine, &s->soc.u_cpus);
>>>            riscv_load_fdt(fdt_load_addr, machine->fdt);
>>>
>>>            /* Load the reset vector */
>>> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
>>> index 9b3dcf3a7a..fd974f2357 100644
>>> --- a/hw/riscv/sifive_u.c
>>> +++ b/hw/riscv/sifive_u.c
>>> @@ -519,7 +519,7 @@ static void sifive_u_machine_init(MachineState *machine)
>>>        const char *firmware_name;
>>>        uint32_t start_addr_hi32 = 0x00000000;
>>>        int i;
>>> -    uint32_t fdt_load_addr;
>>> +    uint64_t fdt_load_addr;
>>>        uint64_t kernel_entry;
>>>        DriveInfo *dinfo;
>>>        BlockBackend *blk;
>>> @@ -606,7 +606,7 @@ static void sifive_u_machine_init(MachineState *machine)
>>>
>>>        fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
>>>                                               memmap[SIFIVE_U_DEV_DRAM].size,
>>> -                                           machine);
>>> +                                           machine, &s->soc.u_cpus);
>>>        riscv_load_fdt(fdt_load_addr, machine->fdt);
>>>
>>>        if (!riscv_is_32bit(&s->soc.u_cpus)) {
>>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>>> index fceb91d946..acd7ab1ae1 100644
>>> --- a/hw/riscv/spike.c
>>> +++ b/hw/riscv/spike.c
>>> @@ -201,7 +201,7 @@ static void spike_board_init(MachineState *machine)
>>>        hwaddr firmware_load_addr = memmap[SPIKE_DRAM].base;
>>>        target_ulong kernel_start_addr;
>>>        char *firmware_name;
>>> -    uint32_t fdt_load_addr;
>>> +    uint64_t fdt_load_addr;
>>>        uint64_t kernel_entry;
>>>        char *soc_name;
>>>        int i, base_hartid, hart_count;
>>> @@ -317,7 +317,7 @@ static void spike_board_init(MachineState *machine)
>>>
>>>        fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
>>>                                               memmap[SPIKE_DRAM].size,
>>> -                                           machine);
>>> +                                           machine, &s->soc[0]);
>>>        riscv_load_fdt(fdt_load_addr, machine->fdt);
>>>
>>>        /* load the reset vector */
>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>> index ee3129f3b3..cfbeeaf7d5 100644
>>> --- a/hw/riscv/virt.c
>>> +++ b/hw/riscv/virt.c
>>> @@ -1399,7 +1399,7 @@ static void virt_machine_done(Notifier *notifier, void *data)
>>>
>>>        fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
>>>                                               memmap[VIRT_DRAM].size,
>>> -                                           machine);
>>> +                                           machine, &s->soc[0]);
>>>        riscv_load_fdt(fdt_load_addr, machine->fdt);
>>>
>>>        /* load the reset vector */
>>> diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h
>>> index 18bfe9f7bf..7ccbe5f62a 100644
>>> --- a/include/hw/riscv/boot.h
>>> +++ b/include/hw/riscv/boot.h
>>> @@ -49,7 +49,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>>                                   bool load_initrd,
>>>                                   symbol_fn_t sym_cb);
>>>    uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>>> -                                MachineState *ms);
>>> +                                MachineState *ms, RISCVHartArrayState *harts);
>>>    void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
>>>    void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>>>                                   hwaddr saddr,

Re: [PATCH 2/2] hw/riscv: Support different address-cells for initrd

[Conor Dooley]

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

On Mon, Oct 21, 2024 at 04:30:11PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 10/21/24 1:09 AM, Jim Shu wrote:
> > The cells of 'initrd-start/end' should follow the '#address-cell'.
> > QEMU API could support 1 and 2 cells.
> > 
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > ---
> >   hw/riscv/boot.c | 14 ++++++++++++--
> >   1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index ad45bd7a6a..76b099c696 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -182,6 +182,7 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> >       void *fdt = machine->fdt;
> >       hwaddr start, end;
> >       ssize_t size;
> > +    uint32_t acells;
> >       g_assert(filename != NULL);
> > @@ -209,9 +210,18 @@ static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)
> >       /* Some RISC-V machines (e.g. opentitan) don't have a fdt. */
> >       if (fdt) {
> > +        acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
> > +                                       NULL, NULL);
> > +        if (acells == 0) {
> > +            error_report("dtb file invalid (#address-cells 0)");
> > +            exit(1);
> > +        }
> > +
> >           end = start + size;
> > -        qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-start", start);
> > -        qemu_fdt_setprop_u64(fdt, "/chosen", "linux,initrd-end", end);
> > +        qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-start",
> > +                                     acells, start);
> > +        qemu_fdt_setprop_sized_cells(fdt, "/chosen", "linux,initrd-end",
> > +                                     acells, end);
> >       }
> 
> Is this a legal format for linux,initrd-start and linux,initrd-end?
> 
> This link:
> 
> https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt
> 
> Defines both attributes as:
> 
> "These properties hold the physical start and end address of an initrd that's
> loaded by the bootloader."
> 
> So I'm not sure if this format you're using here is valid.

Looks like my input isn't really required here anymore, but I think
these two are actually identical in how they appear in the blob. Don't
see much reason to change it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]