Re: [PATCH 07/10] hw/riscv: simplify riscv_load_fdt()

[Philippe Mathieu-Daudé]

On 11/1/23 18:09, Daniel Henrique Barboza wrote:
> All callers of riscv_load_fdt() are using machine->ram_size as
> 'mem_size' and the fdt is always retrievable via machine->fdt.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   hw/riscv/boot.c            | 4 +++-
>   hw/riscv/microchip_pfsoc.c | 4 ++--
>   hw/riscv/sifive_u.c        | 3 +--
>   hw/riscv/spike.c           | 3 +--
>   hw/riscv/virt.c            | 3 +--
>   include/hw/riscv/boot.h    | 2 +-
>   6 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index e868fb6ade..21dea7eac2 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -265,10 +265,12 @@ out:
>       return kernel_entry;
>   }
>   
> -uint64_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> +uint64_t riscv_load_fdt(MachineState *ms, hwaddr dram_base)
>   {
>       uint64_t temp, fdt_addr;
> +    uint64_t mem_size = ms->ram_size;
>       hwaddr dram_end = dram_base + mem_size;

What about sparse memory ...?

>       if (fdtsize <= 0) {
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index c45023a2b1..6bb08f66bd 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -633,8 +633,8 @@ static void microchip_icicle_kit_machine_init(MachineState 
> *machine)
>                                            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_load_fdt(machine,
> +                                       memmap[MICROCHIP_PFSOC_DRAM_LO].base);

... i.e:

hw/riscv/microchip_pfsoc.c:    [MICROCHIP_PFSOC_DRAM_LO] =         { 
0x80000000,   0x40000000 },
hw/riscv/microchip_pfsoc.c:    [MICROCHIP_PFSOC_DRAM_HI] =       { 
0x1000000000,          0x0 },

IIUC FDT is always loaded into a contiguous region, so maybe simply
'dram_base' is a bad name for '[fdt_]load_address'?

'mem_size' is used to calculate 'dram_end'... This function seems buggy.

We should pass either start_addr/end_addr or base/size, but the range
passed must be contiguous.