On Tue, Jan 17, 2023 at 3:34 AM Daniel Henrique Barboza <dbarb...@ventanamicro.com> wrote: > > All callers are using attributes from the MachineState object. Use a > pointer to it instead of passing dram_size (which is always > machine->ram_size) and fdt (always machine->fdt). > > Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com> > --- > hw/riscv/boot.c | 6 +++--- > hw/riscv/microchip_pfsoc.c | 4 ++-- > hw/riscv/sifive_u.c | 4 ++-- > hw/riscv/spike.c | 4 ++-- > hw/riscv/virt.c | 3 +-- > include/hw/riscv/boot.h | 2 +- > 6 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c > index b213a32157..508da3f5c7 100644 > --- a/hw/riscv/boot.c > +++ b/hw/riscv/boot.c > @@ -255,11 +255,11 @@ void riscv_load_initrd(MachineState *machine, uint64_t > kernel_entry) > * > * The FDT is fdt_packed() during the calculation. > */ > -uint32_t riscv_compute_fdt_addr(hwaddr dram_base, uint64_t mem_size, > - void *fdt) > +uint32_t riscv_compute_fdt_addr(MachineState *machine, hwaddr dram_base) > { > + void *fdt = machine->fdt; > uint64_t temp; > - hwaddr dram_end = dram_base + mem_size; > + hwaddr dram_end = dram_base + machine->ram_size; > int ret = fdt_pack(fdt); > int fdtsize; > > diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c > index dcdbc2cac3..a53e48e996 100644 > --- a/hw/riscv/microchip_pfsoc.c > +++ b/hw/riscv/microchip_pfsoc.c > @@ -641,8 +641,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); > + fdt_load_addr = riscv_compute_fdt_addr(machine, > + > memmap[MICROCHIP_PFSOC_DRAM_LO].base);
I don't think this is correct here. So, first up I understand we don't correctly handle this today, *but* I see this change as a step in the wrong direction. The problem here is that ram is split over two areas. So if machine->ram_size is larger then 0x40000000 it is going to overflow MICROCHIP_PFSOC_DRAM_LO and jump to MICROCHIP_PFSOC_DRAM_HI (0x1000000000). So we really want something like this /* Compute the fdt load address in dram */ if (machine->ram_size > memmap[MICROCHIP_PFSOC_DRAM_LO].size) { fdt_load_addr = riscv_load_fdt(memmap[MICROCHIP_PFSOC_DRAM_HI].base, machine->ram_size - memmap[MICROCHIP_PFSOC_DRAM_LO].size, machine->fdt); } else { fdt_load_addr = riscv_load_fdt(memmap[MICROCHIP_PFSOC_DRAM_LO].base, machine->ram_size, machine->fdt); } to handle overflowing MICROCHIP_PFSOC_DRAM_LO. While this patch is going in the wrong direction and making that more difficult Alistair > 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 626d4dc2f3..ebfddf161d 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -616,8 +616,8 @@ static void sifive_u_machine_init(MachineState *machine) > kernel_entry = 0; > } > > - fdt_load_addr = riscv_compute_fdt_addr(memmap[SIFIVE_U_DEV_DRAM].base, > - machine->ram_size, machine->fdt); > + fdt_load_addr = riscv_compute_fdt_addr(machine, > + memmap[SIFIVE_U_DEV_DRAM].base); > 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 88b9fdfc36..afd581436b 100644 > --- a/hw/riscv/spike.c > +++ b/hw/riscv/spike.c > @@ -324,8 +324,8 @@ static void spike_board_init(MachineState *machine) > kernel_entry = 0; > } > > - fdt_load_addr = riscv_compute_fdt_addr(memmap[SPIKE_DRAM].base, > - machine->ram_size, machine->fdt); > + fdt_load_addr = riscv_compute_fdt_addr(machine, > + memmap[SPIKE_DRAM].base); > 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 839dfaa125..cbba0b8930 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -1307,8 +1307,7 @@ static void virt_machine_done(Notifier *notifier, void > *data) > start_addr = virt_memmap[VIRT_FLASH].base; > } > > - fdt_load_addr = riscv_compute_fdt_addr(memmap[VIRT_DRAM].base, > - machine->ram_size, machine->fdt); > + fdt_load_addr = riscv_compute_fdt_addr(machine, memmap[VIRT_DRAM].base); > 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 9aea7b9c46..f933de88fb 100644 > --- a/include/hw/riscv/boot.h > +++ b/include/hw/riscv/boot.h > @@ -47,7 +47,7 @@ target_ulong riscv_load_kernel(MachineState *machine, > target_ulong firmware_end_addr, > symbol_fn_t sym_cb); > void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry); > -uint32_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size, void > *fdt); > +uint32_t riscv_compute_fdt_addr(MachineState *machine, hwaddr dram_start); > void riscv_load_fdt(uint32_t fdt_addr, void *fdt); > void riscv_setup_rom_reset_vec(MachineState *machine, RISCVHartArrayState > *harts, > hwaddr saddr, > -- > 2.39.0 > >