|
| From: | Daniel Henrique Barboza |
| Subject: | Re: [PATCH v2 3/6] hw/riscv: simplify riscv_compute_fdt_addr() |
| Date: | Thu, 19 Jan 2023 13:47:58 -0300 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 |
On 1/18/23 23:23, Alistair Francis wrote:
On Tue, Jan 17, 2023 at 3:34 AM Daniel Henrique Barboza <dbarboza@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 <dbarboza@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); }
Checking the code from microchip-icicle-kit I see that the minimal ram_size is 1.5Gb. In fact the machine would not allow anything less: $ ./qemu-system-riscv64 -M microchip-icicle-kit -m 512M qemu-system-riscv64: Invalid RAM size, should be bigger than 1.5 GiB The reasoning is in its machine_class_init(): ==== * Map 513 MiB high memory, the mimimum required high memory size, because * HSS will do memory test against the high memory address range regardless * of physical memory installed. ==== This also means that this machine is putting the FDT in the wrong spot every time, since 1.5Gb is going to hit in the gap between the low and hi memory every time and the start of the hi area is 64Gb away. I believe this is yet another reason to create a specific helper to deal with the FDT of this machine. I'll re-send with this change. Daniel
to handle overflowing MICROCHIP_PFSOC_DRAM_LO. While this patch is going in the wrong direction and making that more difficult Alistairriscv_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
| [Prev in Thread] | Current Thread | [Next in Thread] |