[PATCH v4 0/3] riscv_load_fdt() semantics change

[PATCH v4 0/3] riscv_load_fdt() semantics change

[Daniel Henrique Barboza]

Hi,

After discussions in the previous version, where we ended up discovering
the details of why the current riscv_load_fdt() works with the Microchip
Icicle Kit board almost by accident, I decided to change how
riscv_compute_fdt_addr() (the FDT address calculation from
riscv_load_fdt()) operates. 

Instead of relying on premises that the Icicle Kit board can't hold
right from start, since dram_base + mem_size will never be contained in
a contiguous RAM area, change the FDT address calculation to also
receive the bondaries of the DRAM block that the board guarantees that
it's not sparse. With this extra information we're able to make a more
consistent FDT address calculation that will cover all existing cases we
have today.


Changes from v3:
- patch 3:
  - function to handle Icicle Kit FDT separately: discarded
  - change riscv_compute_fdt_addr() to clearly handle cases like the
    Icicle Kit board where not all RAM is contiguous
- v3 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04464.html

Daniel Henrique Barboza (3):
  hw/riscv/boot.c: calculate fdt size after fdt_pack()
  hw/riscv: split fdt address calculation from fdt load
  hw/riscv: change riscv_compute_fdt_addr() semantics

 hw/riscv/boot.c            | 56 +++++++++++++++++++++++++++++++-------
 hw/riscv/microchip_pfsoc.c |  7 +++--
 hw/riscv/sifive_u.c        |  8 ++++--
 hw/riscv/spike.c           |  7 +++--
 hw/riscv/virt.c            |  8 ++++--
 include/hw/riscv/boot.h    |  4 ++-
 6 files changed, 68 insertions(+), 22 deletions(-)

-- 
2.39.1

[PATCH v4 1/3] hw/riscv/boot.c: calculate fdt size after fdt_pack()

[Daniel Henrique Barboza]

fdt_pack() can change the fdt size, meaning that fdt_totalsize() can
contain a now deprecated (bigger) value.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 3172a76220..a563b7482a 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -287,8 +287,13 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
 {
     uint64_t temp, fdt_addr;
     hwaddr dram_end = dram_base + mem_size;
-    int ret, fdtsize = fdt_totalsize(fdt);
+    int ret = fdt_pack(fdt);
+    int fdtsize;
 
+    /* Should only fail if we've built a corrupted tree */
+    g_assert(ret == 0);
+
+    fdtsize = fdt_totalsize(fdt);
     if (fdtsize <= 0) {
         error_report("invalid device-tree");
         exit(1);
-- 
2.39.1

[PATCH v4 2/3] hw/riscv: split fdt address calculation from fdt load

[Daniel Henrique Barboza]

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.

We're also taken the opportunity to clarify the intentions and
assumptions made by these functions. riscv_load_fdt() is now receiving a
hwaddr as fdt_addr because there is no restriction of having to load the
fdt in higher addresses that doesn't fit in an uint32_t.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 hw/riscv/boot.c            | 33 +++++++++++++++++++++++++--------
 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    |  4 +++-
 6 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index a563b7482a..a6f7b8ae8e 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -283,9 +283,21 @@ out:
     return 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.
+ *
+ * This function makes an assumption that the DRAM is
+ * contiguous. It also cares about 32-bit systems and
+ * will limit fdt_addr to be addressable by them even for
+ * 64-bit CPUs.
+ *
+ * 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;
@@ -306,11 +318,18 @@ 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);
+}
+
+/*
+ * 'fdt_addr' is received as hwaddr because boards might put
+ * the FDT beyond 32-bit addressing boundary.
+ */
+void riscv_load_fdt(hwaddr fdt_addr, void *fdt)
+{
+    uint32_t fdtsize = fdt_totalsize(fdt);
+
     /* copy in the device tree */
     qemu_fdt_dumpdtb(fdt, fdtsize);
 
@@ -318,8 +337,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 b7e171b605..a30203db85 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -633,8 +633,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
                                          kernel_start_addr, true, NULL);
 
         /* 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 b0b3e6f03a..6bbdbe5fb7 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -608,9 +608,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 483581e05f..ceebe34c5f 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -316,9 +316,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 48326406fd..43fca597f0 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1292,9 +1292,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 bc9faed397..7babd669c7 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -48,7 +48,9 @@ target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong firmware_end_addr,
                                bool load_initrd,
                                symbol_fn_t sym_cb);
-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(hwaddr fdt_addr, void *fdt);
 void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
                                hwaddr saddr,
                                hwaddr rom_base, hwaddr rom_size,
-- 
2.39.1

[PATCH v4 3/3] hw/riscv: change riscv_compute_fdt_addr() semantics

[Daniel Henrique Barboza]

As it is now, riscv_compute_fdt_addr() is receiving a dram_base, a
mem_size (which is defaulted to MachineState::ram_size in all boards)
and the FDT pointer. And it makes a very important assumption: the DRAM
interval dram_base + mem_size is contiguous. This is indeed the case for
most boards that uses a FDT.

The Icicle Kit board works with 2 distinct RAM banks that are separated
by a gap. We have a lower bank with 1GiB size, a gap follows, then at
64GiB the high memory starts. MachineClass::default_ram_size for this
board is set to 1.5Gb, and machine_init() is enforcing it as minimal RAM
size, meaning that there we'll always have at least 512 MiB in the Hi
RAM area.

Using riscv_compute_fdt_addr() in this board is weird because not only
the board has sparse RAM, and it's calling it using the base address of
the Lo RAM area, but it's also using a mem_size that we have guarantees
that it will go up to the Hi RAM. All the function assumptions doesn't
work for this board.

In fact, what makes the function works at all in this case is a
coincidence.  Commit 1a475d39ef54 introduced a 3GB boundary for the FDT,
down from 4Gb, that is enforced if dram_base is lower than 3072 MiB. For
the Icicle Kit board, memmap[MICROCHIP_PFSOC_DRAM_LO].base is 0x80000000
(2 Gb) and it has a 1Gb size, so it will fall in the conditions to put
the FDT under a 3Gb address, which happens to be exactly at the end of
DRAM_LO. If the base address of the Lo area started later than 3Gb this
function would be unusable by the board. Changing any assumptions inside
riscv_compute_fdt_addr() can also break it by accident as well.

Let's change riscv_compute_fdt_addr() semantics to be appropriate to the
Icicle Kit board and for future boards that might have sparse RAM
topologies to worry about:

- relieve the condition that the dram_base + mem_size area is contiguous,
since this is already not the case today;

- receive an extra 'dram_size' size attribute that refers to a contiguous
RAM block that the board wants the FDT to reside on.

Together with 'mem_size' and 'fdt', which are now now being consumed by a
MachineState pointer, we're able to make clear assumptions based on the
DRAM block and total mem_size available to ensure that the FDT will be put
in a valid RAM address.

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

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index a6f7b8ae8e..8f4991480b 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -284,33 +284,47 @@ out:
 }
 
 /*
- * The FDT should be put at the farthest point possible to
- * avoid overwriting it with the kernel/initrd.
+ * This function makes an assumption that the DRAM interval
+ * 'dram_base' + 'dram_size' is contiguous.
  *
- * This function makes an assumption that the DRAM is
- * contiguous. It also cares about 32-bit systems and
- * will limit fdt_addr to be addressable by them even for
- * 64-bit CPUs.
+ * Considering that 'dram_end' is the lowest value between
+ * the end of the DRAM block and MachineState->ram_size, the
+ * FDT location will vary according to 'dram_base':
+ *
+ * - if 'dram_base' is less that 3072 MiB, the FDT will be
+ * put at the lowest value between 3072 MiB and 'dram_end';
+ *
+ * - if 'dram_base' is higher than 3072 MiB, the FDT will be
+ * put at 'dram_end'.
  *
  * The FDT is fdt_packed() during the calculation.
  */
-uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
-                                void *fdt)
+hwaddr riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
+                              MachineState *ms)
 {
-    uint64_t temp;
-    hwaddr dram_end = dram_base + mem_size;
-    int ret = fdt_pack(fdt);
+    int ret = fdt_pack(ms->fdt);
+    hwaddr dram_end, temp;
     int fdtsize;
 
     /* Should only fail if we've built a corrupted tree */
     g_assert(ret == 0);
 
-    fdtsize = fdt_totalsize(fdt);
+    fdtsize = fdt_totalsize(ms->fdt);
     if (fdtsize <= 0) {
         error_report("invalid device-tree");
         exit(1);
     }
 
+    /*
+     * A dram_size == 0, usually from a MemMapEntry[].size element,
+     * means that the DRAM block goes all the way to ms->ram_size.
+     */
+    if (dram_size == 0x0) {
+        dram_end = dram_base + ms->ram_size;
+    } else {
+        dram_end = dram_base + MIN(ms->ram_size, dram_size);
+    }
+
     /*
      * We should put fdt as far as possible to avoid kernel/initrd overwriting
      * its content. But it should be addressable by 32 bit system as well.
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index a30203db85..e81bbd12df 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -634,7 +634,8 @@ 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,
-                                               machine->ram_size, machine->fdt);
+                                               memmap[MICROCHIP_PFSOC_DRAM_LO].size,
+                                               machine);
         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 6bbdbe5fb7..ad3bb35b34 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -609,7 +609,8 @@ static void sifive_u_machine_init(MachineState *machine)
     }
 
     fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
-                                           machine->ram_size, machine->fdt);
+                                           memmap[SIFIVE_U_DEV_DRAM].size,
+                                           machine);
     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 ceebe34c5f..b5979eddd6 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -317,7 +317,8 @@ static void spike_board_init(MachineState *machine)
     }
 
     fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
-                                           machine->ram_size, machine->fdt);
+                                           memmap[SPIKE_DRAM].size,
+                                           machine);
     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 43fca597f0..f079a30b60 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1293,7 +1293,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
     }
 
     fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
-                                           machine->ram_size, machine->fdt);
+                                           memmap[VIRT_DRAM].size,
+                                           machine);
     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 7babd669c7..a6099c2dc6 100644
--- a/include/hw/riscv/boot.h
+++ b/include/hw/riscv/boot.h
@@ -48,8 +48,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
                                target_ulong firmware_end_addr,
                                bool load_initrd,
                                symbol_fn_t sym_cb);
-uint32_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
-                                void *fdt);
+hwaddr riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
+                              MachineState *ms);
 void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
 void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
                                hwaddr saddr,
-- 
2.39.1

Re: [PATCH v4 0/3] riscv_load_fdt() semantics change

[Conor Dooley]

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

On Thu, Jan 26, 2023 at 10:52:16AM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> After discussions in the previous version, where we ended up discovering
> the details of why the current riscv_load_fdt() works with the Microchip
> Icicle Kit board almost by accident, I decided to change how
> riscv_compute_fdt_addr() (the FDT address calculation from
> riscv_load_fdt()) operates. 
> 
> Instead of relying on premises that the Icicle Kit board can't hold
> right from start, since dram_base + mem_size will never be contained in
> a contiguous RAM area, change the FDT address calculation to also
> receive the bondaries of the DRAM block that the board guarantees that
> it's not sparse. With this extra information we're able to make a more
> consistent FDT address calculation that will cover all existing cases we
> have today.

The "test" case that fail before, is now back passing again. Thanks
Daniel!

Tested-by: Conor Dooley <conor.dooley@microchip.com>


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

Re: [PATCH v4 1/3] hw/riscv/boot.c: calculate fdt size after fdt_pack()

[Bin Meng]

Hi Daniel,

On Thu, Jan 26, 2023 at 9:53 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> fdt_pack() can change the fdt size, meaning that fdt_totalsize() can
> contain a now deprecated (bigger) value.

The commit message is a bit confusing.

The original code in this patch does not call fdt_pack(). So not sure
where the issue of "deprecated (bigger) value" happens?

>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 3172a76220..a563b7482a 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -287,8 +287,13 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>  {
>      uint64_t temp, fdt_addr;
>      hwaddr dram_end = dram_base + mem_size;
> -    int ret, fdtsize = fdt_totalsize(fdt);
> +    int ret = fdt_pack(fdt);
> +    int fdtsize;
>
> +    /* Should only fail if we've built a corrupted tree */
> +    g_assert(ret == 0);
> +
> +    fdtsize = fdt_totalsize(fdt);
>      if (fdtsize <= 0) {
>          error_report("invalid device-tree");
>          exit(1);

Regards,
Bin

Re: [PATCH v4 1/3] hw/riscv/boot.c: calculate fdt size after fdt_pack()

[Bin Meng]

On Sun, Jan 29, 2023 at 10:20 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Daniel,
>
> On Thu, Jan 26, 2023 at 9:53 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> > fdt_pack() can change the fdt size, meaning that fdt_totalsize() can
> > contain a now deprecated (bigger) value.
>
> The commit message is a bit confusing.
>
> The original code in this patch does not call fdt_pack(). So not sure
> where the issue of "deprecated (bigger) value" happens?

I see where the call to fdt_pack() happens.

I think you should move the following changes in patch#2 of this
series to this commit.

-    ret = fdt_pack(fdt);
-    /* Should only fail if we've built a corrupted tree */
-    g_assert(ret == 0);

After that, your commit message makes sense, as it describes the
problem and how your patch fixes the problem.

>
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >  hw/riscv/boot.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 3172a76220..a563b7482a 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -287,8 +287,13 @@ uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> >  {
> >      uint64_t temp, fdt_addr;
> >      hwaddr dram_end = dram_base + mem_size;
> > -    int ret, fdtsize = fdt_totalsize(fdt);
> > +    int ret = fdt_pack(fdt);
> > +    int fdtsize;
> >
> > +    /* Should only fail if we've built a corrupted tree */
> > +    g_assert(ret == 0);
> > +
> > +    fdtsize = fdt_totalsize(fdt);
> >      if (fdtsize <= 0) {
> >          error_report("invalid device-tree");
> >          exit(1);
>

Regards,
Bin

Re: [PATCH v4 2/3] hw/riscv: split fdt address calculation from fdt load

[Bin Meng]

On Thu, Jan 26, 2023 at 9:53 PM 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.
>
> We're also taken the opportunity to clarify the intentions and
> assumptions made by these functions. riscv_load_fdt() is now receiving a
> hwaddr as fdt_addr because there is no restriction of having to load the
> fdt in higher addresses that doesn't fit in an uint32_t.
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c            | 33 +++++++++++++++++++++++++--------
>  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    |  4 +++-
>  6 files changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index a563b7482a..a6f7b8ae8e 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -283,9 +283,21 @@ out:
>      return 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.
> + *
> + * This function makes an assumption that the DRAM is
> + * contiguous. It also cares about 32-bit systems and
> + * will limit fdt_addr to be addressable by them even for
> + * 64-bit CPUs.
> + *
> + * The FDT is fdt_packed() during the calculation.
> + */
> +uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
> +                                void *fdt)

The original code returns a uint64_t for fdt_addr but now this is uint32_t?

>  {
> -    uint64_t temp, fdt_addr;
> +    uint64_t temp;
>      hwaddr dram_end = dram_base + mem_size;
>      int ret = fdt_pack(fdt);
>      int fdtsize;
> @@ -306,11 +318,18 @@ 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);
> +}
> +
> +/*
> + * 'fdt_addr' is received as hwaddr because boards might put
> + * the FDT beyond 32-bit addressing boundary.
> + */
> +void riscv_load_fdt(hwaddr fdt_addr, void *fdt)
> +{
> +    uint32_t fdtsize = fdt_totalsize(fdt);
> +
>      /* copy in the device tree */
>      qemu_fdt_dumpdtb(fdt, fdtsize);
>
> @@ -318,8 +337,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 b7e171b605..a30203db85 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -633,8 +633,10 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
>                                           kernel_start_addr, true, NULL);
>
>          /* 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 b0b3e6f03a..6bbdbe5fb7 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -608,9 +608,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 483581e05f..ceebe34c5f 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -316,9 +316,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 48326406fd..43fca597f0 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1292,9 +1292,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 bc9faed397..7babd669c7 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -48,7 +48,9 @@ target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong firmware_end_addr,
>                                 bool load_initrd,
>                                 symbol_fn_t sym_cb);
> -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(hwaddr fdt_addr, void *fdt);
>  void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>                                 hwaddr saddr,
>                                 hwaddr rom_base, hwaddr rom_size,
> --

Regards,
Bin

Re: [PATCH v4 3/3] hw/riscv: change riscv_compute_fdt_addr() semantics

[Bin Meng]

On Thu, Jan 26, 2023 at 9:54 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> As it is now, riscv_compute_fdt_addr() is receiving a dram_base, a
> mem_size (which is defaulted to MachineState::ram_size in all boards)
> and the FDT pointer. And it makes a very important assumption: the DRAM
> interval dram_base + mem_size is contiguous. This is indeed the case for
> most boards that uses a FDT.

s/uses/use

>
> The Icicle Kit board works with 2 distinct RAM banks that are separated
> by a gap. We have a lower bank with 1GiB size, a gap follows, then at
> 64GiB the high memory starts. MachineClass::default_ram_size for this
> board is set to 1.5Gb, and machine_init() is enforcing it as minimal RAM
> size, meaning that there we'll always have at least 512 MiB in the Hi
> RAM area.
>
> Using riscv_compute_fdt_addr() in this board is weird because not only
> the board has sparse RAM, and it's calling it using the base address of
> the Lo RAM area, but it's also using a mem_size that we have guarantees
> that it will go up to the Hi RAM. All the function assumptions doesn't
> work for this board.
>
> In fact, what makes the function works at all in this case is a
> coincidence.  Commit 1a475d39ef54 introduced a 3GB boundary for the FDT,
> down from 4Gb, that is enforced if dram_base is lower than 3072 MiB. For
> the Icicle Kit board, memmap[MICROCHIP_PFSOC_DRAM_LO].base is 0x80000000
> (2 Gb) and it has a 1Gb size, so it will fall in the conditions to put
> the FDT under a 3Gb address, which happens to be exactly at the end of
> DRAM_LO. If the base address of the Lo area started later than 3Gb this
> function would be unusable by the board. Changing any assumptions inside
> riscv_compute_fdt_addr() can also break it by accident as well.
>
> Let's change riscv_compute_fdt_addr() semantics to be appropriate to the
> Icicle Kit board and for future boards that might have sparse RAM
> topologies to worry about:
>
> - relieve the condition that the dram_base + mem_size area is contiguous,
> since this is already not the case today;
>
> - receive an extra 'dram_size' size attribute that refers to a contiguous
> RAM block that the board wants the FDT to reside on.
>
> Together with 'mem_size' and 'fdt', which are now now being consumed by a
> MachineState pointer, we're able to make clear assumptions based on the
> DRAM block and total mem_size available to ensure that the FDT will be put
> in a valid RAM address.
>

Well written commit message. Thanks!

> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c            | 38 ++++++++++++++++++++++++++------------
>  hw/riscv/microchip_pfsoc.c |  3 ++-
>  hw/riscv/sifive_u.c        |  3 ++-
>  hw/riscv/spike.c           |  3 ++-
>  hw/riscv/virt.c            |  3 ++-
>  include/hw/riscv/boot.h    |  4 ++--
>  6 files changed, 36 insertions(+), 18 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index a6f7b8ae8e..8f4991480b 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -284,33 +284,47 @@ out:
>  }
>
>  /*
> - * The FDT should be put at the farthest point possible to
> - * avoid overwriting it with the kernel/initrd.
> + * This function makes an assumption that the DRAM interval
> + * 'dram_base' + 'dram_size' is contiguous.
>   *
> - * This function makes an assumption that the DRAM is
> - * contiguous. It also cares about 32-bit systems and
> - * will limit fdt_addr to be addressable by them even for
> - * 64-bit CPUs.
> + * Considering that 'dram_end' is the lowest value between
> + * the end of the DRAM block and MachineState->ram_size, the
> + * FDT location will vary according to 'dram_base':
> + *
> + * - if 'dram_base' is less that 3072 MiB, the FDT will be
> + * put at the lowest value between 3072 MiB and 'dram_end';
> + *
> + * - if 'dram_base' is higher than 3072 MiB, the FDT will be
> + * put at 'dram_end'.
>   *
>   * The FDT is fdt_packed() during the calculation.
>   */
> -uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
> -                                void *fdt)
> +hwaddr riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,

Using hwaddr to represent a size looks weird. Although technically
they are the same ... I would leave this as it is.

> +                              MachineState *ms)
>  {
> -    uint64_t temp;
> -    hwaddr dram_end = dram_base + mem_size;
> -    int ret = fdt_pack(fdt);
> +    int ret = fdt_pack(ms->fdt);
> +    hwaddr dram_end, temp;
>      int fdtsize;
>
>      /* Should only fail if we've built a corrupted tree */
>      g_assert(ret == 0);
>
> -    fdtsize = fdt_totalsize(fdt);
> +    fdtsize = fdt_totalsize(ms->fdt);
>      if (fdtsize <= 0) {
>          error_report("invalid device-tree");
>          exit(1);
>      }
>
> +    /*
> +     * A dram_size == 0, usually from a MemMapEntry[].size element,
> +     * means that the DRAM block goes all the way to ms->ram_size.
> +     */
> +    if (dram_size == 0x0) {
> +        dram_end = dram_base + ms->ram_size;
> +    } else {
> +        dram_end = dram_base + MIN(ms->ram_size, dram_size);
> +    }

How about:

g_assert(dram_size < ms->ram_size);
dram_end = dram_base + (dram_size ? dram_size : ms->ram_size);

> +
>      /*
>       * We should put fdt as far as possible to avoid kernel/initrd overwriting
>       * its content. But it should be addressable by 32 bit system as well.
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index a30203db85..e81bbd12df 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -634,7 +634,8 @@ 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,
> -                                               machine->ram_size, machine->fdt);
> +                                               memmap[MICROCHIP_PFSOC_DRAM_LO].size,
> +                                               machine);
>          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 6bbdbe5fb7..ad3bb35b34 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -609,7 +609,8 @@ static void sifive_u_machine_init(MachineState *machine)
>      }
>
>      fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
> -                                           machine->ram_size, machine->fdt);
> +                                           memmap[SIFIVE_U_DEV_DRAM].size,
> +                                           machine);
>      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 ceebe34c5f..b5979eddd6 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -317,7 +317,8 @@ static void spike_board_init(MachineState *machine)
>      }
>
>      fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
> -                                           machine->ram_size, machine->fdt);
> +                                           memmap[SPIKE_DRAM].size,
> +                                           machine);
>      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 43fca597f0..f079a30b60 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1293,7 +1293,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
>      }
>
>      fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
> -                                           machine->ram_size, machine->fdt);
> +                                           memmap[VIRT_DRAM].size,
> +                                           machine);
>      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 7babd669c7..a6099c2dc6 100644
> --- a/include/hw/riscv/boot.h
> +++ b/include/hw/riscv/boot.h
> @@ -48,8 +48,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>                                 target_ulong firmware_end_addr,
>                                 bool load_initrd,
>                                 symbol_fn_t sym_cb);
> -uint32_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
> -                                void *fdt);
> +hwaddr riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
> +                              MachineState *ms);
>  void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
>  void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>                                 hwaddr saddr,
> --

Regards,
Bin

Re: [PATCH v4 3/3] hw/riscv: change riscv_compute_fdt_addr() semantics

[Daniel Henrique Barboza]

On 1/29/23 02:45, Bin Meng wrote:
> On Thu, Jan 26, 2023 at 9:54 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>> As it is now, riscv_compute_fdt_addr() is receiving a dram_base, a
>> mem_size (which is defaulted to MachineState::ram_size in all boards)
>> and the FDT pointer. And it makes a very important assumption: the DRAM
>> interval dram_base + mem_size is contiguous. This is indeed the case for
>> most boards that uses a FDT.
> 
> s/uses/use
> 
>>
>> The Icicle Kit board works with 2 distinct RAM banks that are separated
>> by a gap. We have a lower bank with 1GiB size, a gap follows, then at
>> 64GiB the high memory starts. MachineClass::default_ram_size for this
>> board is set to 1.5Gb, and machine_init() is enforcing it as minimal RAM
>> size, meaning that there we'll always have at least 512 MiB in the Hi
>> RAM area.
>>
>> Using riscv_compute_fdt_addr() in this board is weird because not only
>> the board has sparse RAM, and it's calling it using the base address of
>> the Lo RAM area, but it's also using a mem_size that we have guarantees
>> that it will go up to the Hi RAM. All the function assumptions doesn't
>> work for this board.
>>
>> In fact, what makes the function works at all in this case is a
>> coincidence.  Commit 1a475d39ef54 introduced a 3GB boundary for the FDT,
>> down from 4Gb, that is enforced if dram_base is lower than 3072 MiB. For
>> the Icicle Kit board, memmap[MICROCHIP_PFSOC_DRAM_LO].base is 0x80000000
>> (2 Gb) and it has a 1Gb size, so it will fall in the conditions to put
>> the FDT under a 3Gb address, which happens to be exactly at the end of
>> DRAM_LO. If the base address of the Lo area started later than 3Gb this
>> function would be unusable by the board. Changing any assumptions inside
>> riscv_compute_fdt_addr() can also break it by accident as well.
>>
>> Let's change riscv_compute_fdt_addr() semantics to be appropriate to the
>> Icicle Kit board and for future boards that might have sparse RAM
>> topologies to worry about:
>>
>> - relieve the condition that the dram_base + mem_size area is contiguous,
>> since this is already not the case today;
>>
>> - receive an extra 'dram_size' size attribute that refers to a contiguous
>> RAM block that the board wants the FDT to reside on.
>>
>> Together with 'mem_size' and 'fdt', which are now now being consumed by a
>> MachineState pointer, we're able to make clear assumptions based on the
>> DRAM block and total mem_size available to ensure that the FDT will be put
>> in a valid RAM address.
>>
> 
> Well written commit message. Thanks!
> 
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   hw/riscv/boot.c            | 38 ++++++++++++++++++++++++++------------
>>   hw/riscv/microchip_pfsoc.c |  3 ++-
>>   hw/riscv/sifive_u.c        |  3 ++-
>>   hw/riscv/spike.c           |  3 ++-
>>   hw/riscv/virt.c            |  3 ++-
>>   include/hw/riscv/boot.h    |  4 ++--
>>   6 files changed, 36 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>> index a6f7b8ae8e..8f4991480b 100644
>> --- a/hw/riscv/boot.c
>> +++ b/hw/riscv/boot.c
>> @@ -284,33 +284,47 @@ out:
>>   }
>>
>>   /*
>> - * The FDT should be put at the farthest point possible to
>> - * avoid overwriting it with the kernel/initrd.
>> + * This function makes an assumption that the DRAM interval
>> + * 'dram_base' + 'dram_size' is contiguous.
>>    *
>> - * This function makes an assumption that the DRAM is
>> - * contiguous. It also cares about 32-bit systems and
>> - * will limit fdt_addr to be addressable by them even for
>> - * 64-bit CPUs.
>> + * Considering that 'dram_end' is the lowest value between
>> + * the end of the DRAM block and MachineState->ram_size, the
>> + * FDT location will vary according to 'dram_base':
>> + *
>> + * - if 'dram_base' is less that 3072 MiB, the FDT will be
>> + * put at the lowest value between 3072 MiB and 'dram_end';
>> + *
>> + * - if 'dram_base' is higher than 3072 MiB, the FDT will be
>> + * put at 'dram_end'.
>>    *
>>    * The FDT is fdt_packed() during the calculation.
>>    */
>> -uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
>> -                                void *fdt)
>> +hwaddr riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
> 
> Using hwaddr to represent a size looks weird. Although technically
> they are the same ... I would leave this as it is.

I'll leave it as it was back in patch 2 (uint64_t).

> 
>> +                              MachineState *ms)
>>   {
>> -    uint64_t temp;
>> -    hwaddr dram_end = dram_base + mem_size;
>> -    int ret = fdt_pack(fdt);
>> +    int ret = fdt_pack(ms->fdt);
>> +    hwaddr dram_end, temp;
>>       int fdtsize;
>>
>>       /* Should only fail if we've built a corrupted tree */
>>       g_assert(ret == 0);
>>
>> -    fdtsize = fdt_totalsize(fdt);
>> +    fdtsize = fdt_totalsize(ms->fdt);
>>       if (fdtsize <= 0) {
>>           error_report("invalid device-tree");
>>           exit(1);
>>       }
>>
>> +    /*
>> +     * A dram_size == 0, usually from a MemMapEntry[].size element,
>> +     * means that the DRAM block goes all the way to ms->ram_size.
>> +     */
>> +    if (dram_size == 0x0) {
>> +        dram_end = dram_base + ms->ram_size;
>> +    } else {
>> +        dram_end = dram_base + MIN(ms->ram_size, dram_size);
>> +    }
> 
> How about:
> 
> g_assert(dram_size < ms->ram_size);

I don't believe that dram_size > ms->ram_size should be an error. A board can
have a declared MemMapEntry.size that is larger than its current setting of
ms->ram_size.
  

> dram_end = dram_base + (dram_size ? dram_size : ms->ram_size);

I can change the if/else statement up there for a ternary:

dram_end = dram_base + (dram_size ? ms->ram_size : MIN(ms->ram_size, dram_size))


Thanks,

Daniel

> 
>> +
>>       /*
>>        * We should put fdt as far as possible to avoid kernel/initrd overwriting
>>        * its content. But it should be addressable by 32 bit system as well.
>> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
>> index a30203db85..e81bbd12df 100644
>> --- a/hw/riscv/microchip_pfsoc.c
>> +++ b/hw/riscv/microchip_pfsoc.c
>> @@ -634,7 +634,8 @@ 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,
>> -                                               machine->ram_size, machine->fdt);
>> +                                               memmap[MICROCHIP_PFSOC_DRAM_LO].size,
>> +                                               machine);
>>           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 6bbdbe5fb7..ad3bb35b34 100644
>> --- a/hw/riscv/sifive_u.c
>> +++ b/hw/riscv/sifive_u.c
>> @@ -609,7 +609,8 @@ static void sifive_u_machine_init(MachineState *machine)
>>       }
>>
>>       fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
>> -                                           machine->ram_size, machine->fdt);
>> +                                           memmap[SIFIVE_U_DEV_DRAM].size,
>> +                                           machine);
>>       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 ceebe34c5f..b5979eddd6 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -317,7 +317,8 @@ static void spike_board_init(MachineState *machine)
>>       }
>>
>>       fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
>> -                                           machine->ram_size, machine->fdt);
>> +                                           memmap[SPIKE_DRAM].size,
>> +                                           machine);
>>       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 43fca597f0..f079a30b60 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -1293,7 +1293,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
>>       }
>>
>>       fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
>> -                                           machine->ram_size, machine->fdt);
>> +                                           memmap[VIRT_DRAM].size,
>> +                                           machine);
>>       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 7babd669c7..a6099c2dc6 100644
>> --- a/include/hw/riscv/boot.h
>> +++ b/include/hw/riscv/boot.h
>> @@ -48,8 +48,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>                                  target_ulong firmware_end_addr,
>>                                  bool load_initrd,
>>                                  symbol_fn_t sym_cb);
>> -uint32_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>> -                                void *fdt);
>> +hwaddr riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>> +                              MachineState *ms);
>>   void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
>>   void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>>                                  hwaddr saddr,
>> --
> 
> Regards,
> Bin

Re: [PATCH v4 3/3] hw/riscv: change riscv_compute_fdt_addr() semantics

[Daniel Henrique Barboza]

On 1/30/23 14:16, Daniel Henrique Barboza wrote:
> 
> 
> On 1/29/23 02:45, Bin Meng wrote:
>> On Thu, Jan 26, 2023 at 9:54 PM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>> As it is now, riscv_compute_fdt_addr() is receiving a dram_base, a
>>> mem_size (which is defaulted to MachineState::ram_size in all boards)
>>> and the FDT pointer. And it makes a very important assumption: the DRAM
>>> interval dram_base + mem_size is contiguous. This is indeed the case for
>>> most boards that uses a FDT.
>>
>> s/uses/use
>>
>>>
>>> The Icicle Kit board works with 2 distinct RAM banks that are separated
>>> by a gap. We have a lower bank with 1GiB size, a gap follows, then at
>>> 64GiB the high memory starts. MachineClass::default_ram_size for this
>>> board is set to 1.5Gb, and machine_init() is enforcing it as minimal RAM
>>> size, meaning that there we'll always have at least 512 MiB in the Hi
>>> RAM area.
>>>
>>> Using riscv_compute_fdt_addr() in this board is weird because not only
>>> the board has sparse RAM, and it's calling it using the base address of
>>> the Lo RAM area, but it's also using a mem_size that we have guarantees
>>> that it will go up to the Hi RAM. All the function assumptions doesn't
>>> work for this board.
>>>
>>> In fact, what makes the function works at all in this case is a
>>> coincidence.  Commit 1a475d39ef54 introduced a 3GB boundary for the FDT,
>>> down from 4Gb, that is enforced if dram_base is lower than 3072 MiB. For
>>> the Icicle Kit board, memmap[MICROCHIP_PFSOC_DRAM_LO].base is 0x80000000
>>> (2 Gb) and it has a 1Gb size, so it will fall in the conditions to put
>>> the FDT under a 3Gb address, which happens to be exactly at the end of
>>> DRAM_LO. If the base address of the Lo area started later than 3Gb this
>>> function would be unusable by the board. Changing any assumptions inside
>>> riscv_compute_fdt_addr() can also break it by accident as well.
>>>
>>> Let's change riscv_compute_fdt_addr() semantics to be appropriate to the
>>> Icicle Kit board and for future boards that might have sparse RAM
>>> topologies to worry about:
>>>
>>> - relieve the condition that the dram_base + mem_size area is contiguous,
>>> since this is already not the case today;
>>>
>>> - receive an extra 'dram_size' size attribute that refers to a contiguous
>>> RAM block that the board wants the FDT to reside on.
>>>
>>> Together with 'mem_size' and 'fdt', which are now now being consumed by a
>>> MachineState pointer, we're able to make clear assumptions based on the
>>> DRAM block and total mem_size available to ensure that the FDT will be put
>>> in a valid RAM address.
>>>
>>
>> Well written commit message. Thanks!
>>
>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   hw/riscv/boot.c            | 38 ++++++++++++++++++++++++++------------
>>>   hw/riscv/microchip_pfsoc.c |  3 ++-
>>>   hw/riscv/sifive_u.c        |  3 ++-
>>>   hw/riscv/spike.c           |  3 ++-
>>>   hw/riscv/virt.c            |  3 ++-
>>>   include/hw/riscv/boot.h    |  4 ++--
>>>   6 files changed, 36 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>>> index a6f7b8ae8e..8f4991480b 100644
>>> --- a/hw/riscv/boot.c
>>> +++ b/hw/riscv/boot.c
>>> @@ -284,33 +284,47 @@ out:
>>>   }
>>>
>>>   /*
>>> - * The FDT should be put at the farthest point possible to
>>> - * avoid overwriting it with the kernel/initrd.
>>> + * This function makes an assumption that the DRAM interval
>>> + * 'dram_base' + 'dram_size' is contiguous.
>>>    *
>>> - * This function makes an assumption that the DRAM is
>>> - * contiguous. It also cares about 32-bit systems and
>>> - * will limit fdt_addr to be addressable by them even for
>>> - * 64-bit CPUs.
>>> + * Considering that 'dram_end' is the lowest value between
>>> + * the end of the DRAM block and MachineState->ram_size, the
>>> + * FDT location will vary according to 'dram_base':
>>> + *
>>> + * - if 'dram_base' is less that 3072 MiB, the FDT will be
>>> + * put at the lowest value between 3072 MiB and 'dram_end';
>>> + *
>>> + * - if 'dram_base' is higher than 3072 MiB, the FDT will be
>>> + * put at 'dram_end'.
>>>    *
>>>    * The FDT is fdt_packed() during the calculation.
>>>    */
>>> -uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
>>> -                                void *fdt)
>>> +hwaddr riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
>>
>> Using hwaddr to represent a size looks weird. Although technically
>> they are the same ... I would leave this as it is.
> 
> I'll leave it as it was back in patch 2 (uint64_t).
> 
>>
>>> +                              MachineState *ms)
>>>   {
>>> -    uint64_t temp;
>>> -    hwaddr dram_end = dram_base + mem_size;
>>> -    int ret = fdt_pack(fdt);
>>> +    int ret = fdt_pack(ms->fdt);
>>> +    hwaddr dram_end, temp;
>>>       int fdtsize;
>>>
>>>       /* Should only fail if we've built a corrupted tree */
>>>       g_assert(ret == 0);
>>>
>>> -    fdtsize = fdt_totalsize(fdt);
>>> +    fdtsize = fdt_totalsize(ms->fdt);
>>>       if (fdtsize <= 0) {
>>>           error_report("invalid device-tree");
>>>           exit(1);
>>>       }
>>>
>>> +    /*
>>> +     * A dram_size == 0, usually from a MemMapEntry[].size element,
>>> +     * means that the DRAM block goes all the way to ms->ram_size.
>>> +     */
>>> +    if (dram_size == 0x0) {
>>> +        dram_end = dram_base + ms->ram_size;
>>> +    } else {
>>> +        dram_end = dram_base + MIN(ms->ram_size, dram_size);
>>> +    }
>>
>> How about:
>>
>> g_assert(dram_size < ms->ram_size);
> 
> I don't believe that dram_size > ms->ram_size should be an error. A board can
> have a declared MemMapEntry.size that is larger than its current setting of
> ms->ram_size.
> 
> 
>> dram_end = dram_base + (dram_size ? dram_size : ms->ram_size);
> 
> I can change the if/else statement up there for a ternary:
> 
> dram_end = dram_base + (dram_size ? ms->ram_size : MIN(ms->ram_size, dram_size))

This is an ooopsie. This ternary is doing the opposite of what it should do :/

I would do something like this, breaking in two lines to avoid 80+ chars in the
same line:

     dram_end = dram_base;
     dram_end += dram_size ? MIN(ms->ram_size, dram_size) : ms->ram_size;


Daniel

> 
> 
> Thanks,
> 
> Daniel
> 
>>
>>> +
>>>       /*
>>>        * We should put fdt as far as possible to avoid kernel/initrd overwriting
>>>        * its content. But it should be addressable by 32 bit system as well.
>>> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
>>> index a30203db85..e81bbd12df 100644
>>> --- a/hw/riscv/microchip_pfsoc.c
>>> +++ b/hw/riscv/microchip_pfsoc.c
>>> @@ -634,7 +634,8 @@ 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,
>>> -                                               machine->ram_size, machine->fdt);
>>> +                                               memmap[MICROCHIP_PFSOC_DRAM_LO].size,
>>> +                                               machine);
>>>           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 6bbdbe5fb7..ad3bb35b34 100644
>>> --- a/hw/riscv/sifive_u.c
>>> +++ b/hw/riscv/sifive_u.c
>>> @@ -609,7 +609,8 @@ static void sifive_u_machine_init(MachineState *machine)
>>>       }
>>>
>>>       fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base,
>>> -                                           machine->ram_size, machine->fdt);
>>> +                                           memmap[SIFIVE_U_DEV_DRAM].size,
>>> +                                           machine);
>>>       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 ceebe34c5f..b5979eddd6 100644
>>> --- a/hw/riscv/spike.c
>>> +++ b/hw/riscv/spike.c
>>> @@ -317,7 +317,8 @@ static void spike_board_init(MachineState *machine)
>>>       }
>>>
>>>       fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base,
>>> -                                           machine->ram_size, machine->fdt);
>>> +                                           memmap[SPIKE_DRAM].size,
>>> +                                           machine);
>>>       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 43fca597f0..f079a30b60 100644
>>> --- a/hw/riscv/virt.c
>>> +++ b/hw/riscv/virt.c
>>> @@ -1293,7 +1293,8 @@ static void virt_machine_done(Notifier *notifier, void *data)
>>>       }
>>>
>>>       fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base,
>>> -                                           machine->ram_size, machine->fdt);
>>> +                                           memmap[VIRT_DRAM].size,
>>> +                                           machine);
>>>       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 7babd669c7..a6099c2dc6 100644
>>> --- a/include/hw/riscv/boot.h
>>> +++ b/include/hw/riscv/boot.h
>>> @@ -48,8 +48,8 @@ target_ulong riscv_load_kernel(MachineState *machine,
>>>                                  target_ulong firmware_end_addr,
>>>                                  bool load_initrd,
>>>                                  symbol_fn_t sym_cb);
>>> -uint32_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>>> -                                void *fdt);
>>> +hwaddr riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,
>>> +                              MachineState *ms);
>>>   void riscv_load_fdt(hwaddr fdt_addr, void *fdt);
>>>   void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState *harts,
>>>                                  hwaddr saddr,
>>> -- 
>>
>> Regards,
>> Bin

Re: [PATCH v4 3/3] hw/riscv: change riscv_compute_fdt_addr() semantics

[Bin Meng]

On Tue, Jan 31, 2023 at 1:16 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 1/29/23 02:45, Bin Meng wrote:
> > On Thu, Jan 26, 2023 at 9:54 PM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> As it is now, riscv_compute_fdt_addr() is receiving a dram_base, a
> >> mem_size (which is defaulted to MachineState::ram_size in all boards)
> >> and the FDT pointer. And it makes a very important assumption: the DRAM
> >> interval dram_base + mem_size is contiguous. This is indeed the case for
> >> most boards that uses a FDT.
> >
> > s/uses/use
> >
> >>
> >> The Icicle Kit board works with 2 distinct RAM banks that are separated
> >> by a gap. We have a lower bank with 1GiB size, a gap follows, then at
> >> 64GiB the high memory starts. MachineClass::default_ram_size for this
> >> board is set to 1.5Gb, and machine_init() is enforcing it as minimal RAM
> >> size, meaning that there we'll always have at least 512 MiB in the Hi
> >> RAM area.
> >>
> >> Using riscv_compute_fdt_addr() in this board is weird because not only
> >> the board has sparse RAM, and it's calling it using the base address of
> >> the Lo RAM area, but it's also using a mem_size that we have guarantees
> >> that it will go up to the Hi RAM. All the function assumptions doesn't
> >> work for this board.
> >>
> >> In fact, what makes the function works at all in this case is a
> >> coincidence.  Commit 1a475d39ef54 introduced a 3GB boundary for the FDT,
> >> down from 4Gb, that is enforced if dram_base is lower than 3072 MiB. For
> >> the Icicle Kit board, memmap[MICROCHIP_PFSOC_DRAM_LO].base is 0x80000000
> >> (2 Gb) and it has a 1Gb size, so it will fall in the conditions to put
> >> the FDT under a 3Gb address, which happens to be exactly at the end of
> >> DRAM_LO. If the base address of the Lo area started later than 3Gb this
> >> function would be unusable by the board. Changing any assumptions inside
> >> riscv_compute_fdt_addr() can also break it by accident as well.
> >>
> >> Let's change riscv_compute_fdt_addr() semantics to be appropriate to the
> >> Icicle Kit board and for future boards that might have sparse RAM
> >> topologies to worry about:
> >>
> >> - relieve the condition that the dram_base + mem_size area is contiguous,
> >> since this is already not the case today;
> >>
> >> - receive an extra 'dram_size' size attribute that refers to a contiguous
> >> RAM block that the board wants the FDT to reside on.
> >>
> >> Together with 'mem_size' and 'fdt', which are now now being consumed by a
> >> MachineState pointer, we're able to make clear assumptions based on the
> >> DRAM block and total mem_size available to ensure that the FDT will be put
> >> in a valid RAM address.
> >>
> >
> > Well written commit message. Thanks!
> >
> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >> ---
> >>   hw/riscv/boot.c            | 38 ++++++++++++++++++++++++++------------
> >>   hw/riscv/microchip_pfsoc.c |  3 ++-
> >>   hw/riscv/sifive_u.c        |  3 ++-
> >>   hw/riscv/spike.c           |  3 ++-
> >>   hw/riscv/virt.c            |  3 ++-
> >>   include/hw/riscv/boot.h    |  4 ++--
> >>   6 files changed, 36 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> >> index a6f7b8ae8e..8f4991480b 100644
> >> --- a/hw/riscv/boot.c
> >> +++ b/hw/riscv/boot.c
> >> @@ -284,33 +284,47 @@ out:
> >>   }
> >>
> >>   /*
> >> - * The FDT should be put at the farthest point possible to
> >> - * avoid overwriting it with the kernel/initrd.
> >> + * This function makes an assumption that the DRAM interval
> >> + * 'dram_base' + 'dram_size' is contiguous.
> >>    *
> >> - * This function makes an assumption that the DRAM is
> >> - * contiguous. It also cares about 32-bit systems and
> >> - * will limit fdt_addr to be addressable by them even for
> >> - * 64-bit CPUs.
> >> + * Considering that 'dram_end' is the lowest value between
> >> + * the end of the DRAM block and MachineState->ram_size, the
> >> + * FDT location will vary according to 'dram_base':
> >> + *
> >> + * - if 'dram_base' is less that 3072 MiB, the FDT will be
> >> + * put at the lowest value between 3072 MiB and 'dram_end';
> >> + *
> >> + * - if 'dram_base' is higher than 3072 MiB, the FDT will be
> >> + * put at 'dram_end'.
> >>    *
> >>    * The FDT is fdt_packed() during the calculation.
> >>    */
> >> -uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
> >> -                                void *fdt)
> >> +hwaddr riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
> >
> > Using hwaddr to represent a size looks weird. Although technically
> > they are the same ... I would leave this as it is.
>
> I'll leave it as it was back in patch 2 (uint64_t).
>
> >
> >> +                              MachineState *ms)
> >>   {
> >> -    uint64_t temp;
> >> -    hwaddr dram_end = dram_base + mem_size;
> >> -    int ret = fdt_pack(fdt);
> >> +    int ret = fdt_pack(ms->fdt);
> >> +    hwaddr dram_end, temp;
> >>       int fdtsize;
> >>
> >>       /* Should only fail if we've built a corrupted tree */
> >>       g_assert(ret == 0);
> >>
> >> -    fdtsize = fdt_totalsize(fdt);
> >> +    fdtsize = fdt_totalsize(ms->fdt);
> >>       if (fdtsize <= 0) {
> >>           error_report("invalid device-tree");
> >>           exit(1);
> >>       }
> >>
> >> +    /*
> >> +     * A dram_size == 0, usually from a MemMapEntry[].size element,
> >> +     * means that the DRAM block goes all the way to ms->ram_size.
> >> +     */
> >> +    if (dram_size == 0x0) {
> >> +        dram_end = dram_base + ms->ram_size;
> >> +    } else {
> >> +        dram_end = dram_base + MIN(ms->ram_size, dram_size);
> >> +    }
> >
> > How about:
> >
> > g_assert(dram_size < ms->ram_size);
>
> I don't believe that dram_size > ms->ram_size should be an error. A board can
> have a declared MemMapEntry.size that is larger than its current setting of
> ms->ram_size.

What use case is that? This updated function now has the assumption that:

1. dram_size being 0 meaning contiguous system RAM region from dram_base
2. otherwise dram_size being the *first* contiguous system RAM region
from dram_base

We can use g_assert(dram_size < ms->ram_size) to catch either case.

>
>
> > dram_end = dram_base + (dram_size ? dram_size : ms->ram_size);
>
> I can change the if/else statement up there for a ternary:
>
> dram_end = dram_base + (dram_size ? ms->ram_size : MIN(ms->ram_size, dram_size))
>

Regards,
Bin

Re: [PATCH v4 3/3] hw/riscv: change riscv_compute_fdt_addr() semantics

[Daniel Henrique Barboza]

On 1/30/23 22:00, Bin Meng wrote:
> On Tue, Jan 31, 2023 at 1:16 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 1/29/23 02:45, Bin Meng wrote:
>>> On Thu, Jan 26, 2023 at 9:54 PM Daniel Henrique Barboza
>>> <dbarboza@ventanamicro.com> wrote:
>>>>
>>>> As it is now, riscv_compute_fdt_addr() is receiving a dram_base, a
>>>> mem_size (which is defaulted to MachineState::ram_size in all boards)
>>>> and the FDT pointer. And it makes a very important assumption: the DRAM
>>>> interval dram_base + mem_size is contiguous. This is indeed the case for
>>>> most boards that uses a FDT.
>>>
>>> s/uses/use
>>>
>>>>
>>>> The Icicle Kit board works with 2 distinct RAM banks that are separated
>>>> by a gap. We have a lower bank with 1GiB size, a gap follows, then at
>>>> 64GiB the high memory starts. MachineClass::default_ram_size for this
>>>> board is set to 1.5Gb, and machine_init() is enforcing it as minimal RAM
>>>> size, meaning that there we'll always have at least 512 MiB in the Hi
>>>> RAM area.
>>>>
>>>> Using riscv_compute_fdt_addr() in this board is weird because not only
>>>> the board has sparse RAM, and it's calling it using the base address of
>>>> the Lo RAM area, but it's also using a mem_size that we have guarantees
>>>> that it will go up to the Hi RAM. All the function assumptions doesn't
>>>> work for this board.
>>>>
>>>> In fact, what makes the function works at all in this case is a
>>>> coincidence.  Commit 1a475d39ef54 introduced a 3GB boundary for the FDT,
>>>> down from 4Gb, that is enforced if dram_base is lower than 3072 MiB. For
>>>> the Icicle Kit board, memmap[MICROCHIP_PFSOC_DRAM_LO].base is 0x80000000
>>>> (2 Gb) and it has a 1Gb size, so it will fall in the conditions to put
>>>> the FDT under a 3Gb address, which happens to be exactly at the end of
>>>> DRAM_LO. If the base address of the Lo area started later than 3Gb this
>>>> function would be unusable by the board. Changing any assumptions inside
>>>> riscv_compute_fdt_addr() can also break it by accident as well.
>>>>
>>>> Let's change riscv_compute_fdt_addr() semantics to be appropriate to the
>>>> Icicle Kit board and for future boards that might have sparse RAM
>>>> topologies to worry about:
>>>>
>>>> - relieve the condition that the dram_base + mem_size area is contiguous,
>>>> since this is already not the case today;
>>>>
>>>> - receive an extra 'dram_size' size attribute that refers to a contiguous
>>>> RAM block that the board wants the FDT to reside on.
>>>>
>>>> Together with 'mem_size' and 'fdt', which are now now being consumed by a
>>>> MachineState pointer, we're able to make clear assumptions based on the
>>>> DRAM block and total mem_size available to ensure that the FDT will be put
>>>> in a valid RAM address.
>>>>
>>>
>>> Well written commit message. Thanks!
>>>
>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>> ---
>>>>    hw/riscv/boot.c            | 38 ++++++++++++++++++++++++++------------
>>>>    hw/riscv/microchip_pfsoc.c |  3 ++-
>>>>    hw/riscv/sifive_u.c        |  3 ++-
>>>>    hw/riscv/spike.c           |  3 ++-
>>>>    hw/riscv/virt.c            |  3 ++-
>>>>    include/hw/riscv/boot.h    |  4 ++--
>>>>    6 files changed, 36 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
>>>> index a6f7b8ae8e..8f4991480b 100644
>>>> --- a/hw/riscv/boot.c
>>>> +++ b/hw/riscv/boot.c
>>>> @@ -284,33 +284,47 @@ out:
>>>>    }
>>>>
>>>>    /*
>>>> - * The FDT should be put at the farthest point possible to
>>>> - * avoid overwriting it with the kernel/initrd.
>>>> + * This function makes an assumption that the DRAM interval
>>>> + * 'dram_base' + 'dram_size' is contiguous.
>>>>     *
>>>> - * This function makes an assumption that the DRAM is
>>>> - * contiguous. It also cares about 32-bit systems and
>>>> - * will limit fdt_addr to be addressable by them even for
>>>> - * 64-bit CPUs.
>>>> + * Considering that 'dram_end' is the lowest value between
>>>> + * the end of the DRAM block and MachineState->ram_size, the
>>>> + * FDT location will vary according to 'dram_base':
>>>> + *
>>>> + * - if 'dram_base' is less that 3072 MiB, the FDT will be
>>>> + * put at the lowest value between 3072 MiB and 'dram_end';
>>>> + *
>>>> + * - if 'dram_base' is higher than 3072 MiB, the FDT will be
>>>> + * put at 'dram_end'.
>>>>     *
>>>>     * The FDT is fdt_packed() during the calculation.
>>>>     */
>>>> -uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
>>>> -                                void *fdt)
>>>> +hwaddr riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
>>>
>>> Using hwaddr to represent a size looks weird. Although technically
>>> they are the same ... I would leave this as it is.
>>
>> I'll leave it as it was back in patch 2 (uint64_t).
>>
>>>
>>>> +                              MachineState *ms)
>>>>    {
>>>> -    uint64_t temp;
>>>> -    hwaddr dram_end = dram_base + mem_size;
>>>> -    int ret = fdt_pack(fdt);
>>>> +    int ret = fdt_pack(ms->fdt);
>>>> +    hwaddr dram_end, temp;
>>>>        int fdtsize;
>>>>
>>>>        /* Should only fail if we've built a corrupted tree */
>>>>        g_assert(ret == 0);
>>>>
>>>> -    fdtsize = fdt_totalsize(fdt);
>>>> +    fdtsize = fdt_totalsize(ms->fdt);
>>>>        if (fdtsize <= 0) {
>>>>            error_report("invalid device-tree");
>>>>            exit(1);
>>>>        }
>>>>
>>>> +    /*
>>>> +     * A dram_size == 0, usually from a MemMapEntry[].size element,
>>>> +     * means that the DRAM block goes all the way to ms->ram_size.
>>>> +     */
>>>> +    if (dram_size == 0x0) {
>>>> +        dram_end = dram_base + ms->ram_size;
>>>> +    } else {
>>>> +        dram_end = dram_base + MIN(ms->ram_size, dram_size);
>>>> +    }
>>>
>>> How about:
>>>
>>> g_assert(dram_size < ms->ram_size);
>>
>> I don't believe that dram_size > ms->ram_size should be an error. A board can
>> have a declared MemMapEntry.size that is larger than its current setting of
>> ms->ram_size.
> 
> What use case is that? This updated function now has the assumption that:
> 
> 1. dram_size being 0 meaning contiguous system RAM region from dram_base
> 2. otherwise dram_size being the *first* contiguous system RAM region
> from dram_base

Yes, but that doesn't mean that dram_size is necessarily smaller than ms->ram_size.

We don't have any board where this happens today but let's pretend that the Icicle
Kit board didn't have the 1.5Gb minimal RAM restriction. Its first region has
dram_size 1Gb:

[MICROCHIP_PFSOC_DRAM_LO] =         { 0x80000000,   0x40000000 },

So, if I start the board with 512M, ms->ram_size = 512M and this assert will trigger
because dram_size = 1Gb.


Thanks,


Daniel


> 
> We can use g_assert(dram_size < ms->ram_size) to catch either case.
> 
>>
>>
>>> dram_end = dram_base + (dram_size ? dram_size : ms->ram_size);
>>
>> I can change the if/else statement up there for a ternary:
>>
>> dram_end = dram_base + (dram_size ? ms->ram_size : MIN(ms->ram_size, dram_size))
>>
> 
> Regards,
> Bin

Re: [PATCH v4 3/3] hw/riscv: change riscv_compute_fdt_addr() semantics

[Bin Meng]

On Tue, Jan 31, 2023 at 5:57 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 1/30/23 22:00, Bin Meng wrote:
> > On Tue, Jan 31, 2023 at 1:16 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >>
> >>
> >> On 1/29/23 02:45, Bin Meng wrote:
> >>> On Thu, Jan 26, 2023 at 9:54 PM Daniel Henrique Barboza
> >>> <dbarboza@ventanamicro.com> wrote:
> >>>>
> >>>> As it is now, riscv_compute_fdt_addr() is receiving a dram_base, a
> >>>> mem_size (which is defaulted to MachineState::ram_size in all boards)
> >>>> and the FDT pointer. And it makes a very important assumption: the DRAM
> >>>> interval dram_base + mem_size is contiguous. This is indeed the case for
> >>>> most boards that uses a FDT.
> >>>
> >>> s/uses/use
> >>>
> >>>>
> >>>> The Icicle Kit board works with 2 distinct RAM banks that are separated
> >>>> by a gap. We have a lower bank with 1GiB size, a gap follows, then at
> >>>> 64GiB the high memory starts. MachineClass::default_ram_size for this
> >>>> board is set to 1.5Gb, and machine_init() is enforcing it as minimal RAM
> >>>> size, meaning that there we'll always have at least 512 MiB in the Hi
> >>>> RAM area.
> >>>>
> >>>> Using riscv_compute_fdt_addr() in this board is weird because not only
> >>>> the board has sparse RAM, and it's calling it using the base address of
> >>>> the Lo RAM area, but it's also using a mem_size that we have guarantees
> >>>> that it will go up to the Hi RAM. All the function assumptions doesn't
> >>>> work for this board.
> >>>>
> >>>> In fact, what makes the function works at all in this case is a
> >>>> coincidence.  Commit 1a475d39ef54 introduced a 3GB boundary for the FDT,
> >>>> down from 4Gb, that is enforced if dram_base is lower than 3072 MiB. For
> >>>> the Icicle Kit board, memmap[MICROCHIP_PFSOC_DRAM_LO].base is 0x80000000
> >>>> (2 Gb) and it has a 1Gb size, so it will fall in the conditions to put
> >>>> the FDT under a 3Gb address, which happens to be exactly at the end of
> >>>> DRAM_LO. If the base address of the Lo area started later than 3Gb this
> >>>> function would be unusable by the board. Changing any assumptions inside
> >>>> riscv_compute_fdt_addr() can also break it by accident as well.
> >>>>
> >>>> Let's change riscv_compute_fdt_addr() semantics to be appropriate to the
> >>>> Icicle Kit board and for future boards that might have sparse RAM
> >>>> topologies to worry about:
> >>>>
> >>>> - relieve the condition that the dram_base + mem_size area is contiguous,
> >>>> since this is already not the case today;
> >>>>
> >>>> - receive an extra 'dram_size' size attribute that refers to a contiguous
> >>>> RAM block that the board wants the FDT to reside on.
> >>>>
> >>>> Together with 'mem_size' and 'fdt', which are now now being consumed by a
> >>>> MachineState pointer, we're able to make clear assumptions based on the
> >>>> DRAM block and total mem_size available to ensure that the FDT will be put
> >>>> in a valid RAM address.
> >>>>
> >>>
> >>> Well written commit message. Thanks!
> >>>
> >>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >>>> ---
> >>>>    hw/riscv/boot.c            | 38 ++++++++++++++++++++++++++------------
> >>>>    hw/riscv/microchip_pfsoc.c |  3 ++-
> >>>>    hw/riscv/sifive_u.c        |  3 ++-
> >>>>    hw/riscv/spike.c           |  3 ++-
> >>>>    hw/riscv/virt.c            |  3 ++-
> >>>>    include/hw/riscv/boot.h    |  4 ++--
> >>>>    6 files changed, 36 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> >>>> index a6f7b8ae8e..8f4991480b 100644
> >>>> --- a/hw/riscv/boot.c
> >>>> +++ b/hw/riscv/boot.c
> >>>> @@ -284,33 +284,47 @@ out:
> >>>>    }
> >>>>
> >>>>    /*
> >>>> - * The FDT should be put at the farthest point possible to
> >>>> - * avoid overwriting it with the kernel/initrd.
> >>>> + * This function makes an assumption that the DRAM interval
> >>>> + * 'dram_base' + 'dram_size' is contiguous.
> >>>>     *
> >>>> - * This function makes an assumption that the DRAM is
> >>>> - * contiguous. It also cares about 32-bit systems and
> >>>> - * will limit fdt_addr to be addressable by them even for
> >>>> - * 64-bit CPUs.
> >>>> + * Considering that 'dram_end' is the lowest value between
> >>>> + * the end of the DRAM block and MachineState->ram_size, the
> >>>> + * FDT location will vary according to 'dram_base':
> >>>> + *
> >>>> + * - if 'dram_base' is less that 3072 MiB, the FDT will be
> >>>> + * put at the lowest value between 3072 MiB and 'dram_end';
> >>>> + *
> >>>> + * - if 'dram_base' is higher than 3072 MiB, the FDT will be
> >>>> + * put at 'dram_end'.
> >>>>     *
> >>>>     * The FDT is fdt_packed() during the calculation.
> >>>>     */
> >>>> -uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size,
> >>>> -                                void *fdt)
> >>>> +hwaddr riscv_compute_fdt_addr(hwaddr dram_base, hwaddr dram_size,
> >>>
> >>> Using hwaddr to represent a size looks weird. Although technically
> >>> they are the same ... I would leave this as it is.
> >>
> >> I'll leave it as it was back in patch 2 (uint64_t).
> >>
> >>>
> >>>> +                              MachineState *ms)
> >>>>    {
> >>>> -    uint64_t temp;
> >>>> -    hwaddr dram_end = dram_base + mem_size;
> >>>> -    int ret = fdt_pack(fdt);
> >>>> +    int ret = fdt_pack(ms->fdt);
> >>>> +    hwaddr dram_end, temp;
> >>>>        int fdtsize;
> >>>>
> >>>>        /* Should only fail if we've built a corrupted tree */
> >>>>        g_assert(ret == 0);
> >>>>
> >>>> -    fdtsize = fdt_totalsize(fdt);
> >>>> +    fdtsize = fdt_totalsize(ms->fdt);
> >>>>        if (fdtsize <= 0) {
> >>>>            error_report("invalid device-tree");
> >>>>            exit(1);
> >>>>        }
> >>>>
> >>>> +    /*
> >>>> +     * A dram_size == 0, usually from a MemMapEntry[].size element,
> >>>> +     * means that the DRAM block goes all the way to ms->ram_size.
> >>>> +     */
> >>>> +    if (dram_size == 0x0) {
> >>>> +        dram_end = dram_base + ms->ram_size;
> >>>> +    } else {
> >>>> +        dram_end = dram_base + MIN(ms->ram_size, dram_size);
> >>>> +    }
> >>>
> >>> How about:
> >>>
> >>> g_assert(dram_size < ms->ram_size);
> >>
> >> I don't believe that dram_size > ms->ram_size should be an error. A board can
> >> have a declared MemMapEntry.size that is larger than its current setting of
> >> ms->ram_size.
> >
> > What use case is that? This updated function now has the assumption that:
> >
> > 1. dram_size being 0 meaning contiguous system RAM region from dram_base
> > 2. otherwise dram_size being the *first* contiguous system RAM region
> > from dram_base
>
> Yes, but that doesn't mean that dram_size is necessarily smaller than ms->ram_size.
>
> We don't have any board where this happens today but let's pretend that the Icicle
> Kit board didn't have the 1.5Gb minimal RAM restriction. Its first region has
> dram_size 1Gb:
>
> [MICROCHIP_PFSOC_DRAM_LO] =         { 0x80000000,   0x40000000 },
>
> So, if I start the board with 512M, ms->ram_size = 512M and this assert will trigger
> because dram_size = 1Gb.
>

Ah, I see. So for a machine whose memory size can be dynamically
configured, yes that makes sense.

Regards,
Bin