Hi,

This new version removed the translate_fn() from patch 1 because it

wasn't removing the sign-extension for pentry as we thought it would.

A more detailed explanation is given in the commit msg of patch 1.

We're now retrieving the 'lowaddr' value from load_elf_ram_sym() and

using it when we're running a 32-bit CPU. This worked with 32 bit

'virt' machine booting with the -kernel option.

If this approach doesn't work for the Xvisor use case, IMO we should

just filter kernel_load_addr bits directly as we were doing a handful of

versions ago.

Patches are based on current riscv-to-apply.next.

Changes from v9:

- patch 1:

- removed the translate_fn() callback

- return 'kernel_low' when running a 32-bit CPU

- v9 link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg04509.html

Daniel Henrique Barboza (3):

hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel()

hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

hw/riscv/boot.c: make riscv_load_initrd() static

hw/riscv/boot.c | 96 +++++++++++++++++++++++---------------

hw/riscv/microchip_pfsoc.c | 12 +----

hw/riscv/opentitan.c | 4 +-

hw/riscv/sifive_e.c | 4 +-

hw/riscv/sifive_u.c | 12 +----

hw/riscv/spike.c | 14 ++----

hw/riscv/virt.c | 12 +----

include/hw/riscv/boot.h | 3 +-

8 files changed, 76 insertions(+), 81 deletions(-)

2.39.1

The microchip_icicle_kit, sifive_u, spike and virt boards are now doing

the same steps when '-kernel' is used:

- execute load_kernel()

- load init_rd()

- write kernel_cmdline

Let's fold everything inside riscv_load_kernel() to avoid code

repetition. To not change the behavior of boards that aren't calling

riscv_load_init(), add an 'load_initrd' flag to riscv_load_kernel() and

allow these boards to opt out from initrd loading.

Cc: Palmer Dabbelt <palmer@dabbelt.com>

Reviewed-by: Bin Meng <bmeng@tinylab.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

hw/riscv/boot.c | 21 ++++++++++++++++++---

hw/riscv/microchip_pfsoc.c | 11 +----------

hw/riscv/opentitan.c | 3 ++-

hw/riscv/sifive_e.c | 3 ++-

hw/riscv/sifive_u.c | 11 +----------

hw/riscv/spike.c | 11 +----------

hw/riscv/virt.c | 11 +----------

include/hw/riscv/boot.h | 1 +

8 files changed, 27 insertions(+), 45 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c

--- a/hw/riscv/boot.c

+++ b/hw/riscv/boot.c

@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,

target_ulong riscv_load_kernel(MachineState *machine,

RISCVHartArrayState *harts,

target_ulong kernel_start_addr,

+ bool load_initrd,

symbol_fn_t sym_cb)

{

const char *kernel_filename = machine->kernel_filename;

uint64_t kernel_load_base, kernel_entry, kernel_low;

+ void *fdt = machine->fdt;

g_assert(kernel_filename != NULL);

@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_kernel(MachineState *machine,

kernel_entry = kernel_low;

}

- return kernel_entry;

+ goto out;

}

if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL,

NULL, NULL, NULL) > 0) {

- return kernel_entry;

+ goto out;

}

if (load_image_targphys_as(kernel_filename, kernel_start_addr,

current_machine->ram_size, NULL) > 0) {

- return kernel_start_addr;

+ kernel_entry = kernel_start_addr;

+ goto out;

}

error_report("could not load kernel '%s'", kernel_filename);

exit(1);

+

+out:

+ if (load_initrd && machine->initrd_filename) {

+ riscv_load_initrd(machine, kernel_entry);

+

+ if (fdt && machine->kernel_cmdline && *machine->kernel_cmdline) {

+ qemu_fdt_setprop_string(fdt, "/chosen", "bootargs",

+ machine->kernel_cmdline);

+ }

+

+ return kernel_entry;

}

void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)

diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c

--- a/hw/riscv/microchip_pfsoc.c

+++ b/hw/riscv/microchip_pfsoc.c

@@ -XXX,XX +XXX,XX @@ static void microchip_icicle_kit_machine_init(MachineState *machine)

firmware_end_addr);

kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,

- kernel_start_addr, NULL);

-

- if (machine->initrd_filename) {

- riscv_load_initrd(machine, kernel_entry);

- }

-

- if (machine->kernel_cmdline && *machine->kernel_cmdline) {

- qemu_fdt_setprop_string(machine->fdt, "/chosen",

- "bootargs", machine->kernel_cmdline);

- }

+ kernel_start_addr, true, NULL);

/* Compute the fdt load address in dram */

fdt_load_addr = riscv_compute_fdt_addr(memmap[MICROCHIP_PFSOC_DRAM_LO].base,

diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/opentitan.c

+++ b/hw/riscv/opentitan.c

@@ -XXX,XX +XXX,XX @@ static void opentitan_board_init(MachineState *machine)

if (machine->kernel_filename) {

riscv_load_kernel(machine, &s->soc.cpus,

- memmap[IBEX_DEV_RAM].base, NULL);

+ memmap[IBEX_DEV_RAM].base,

+ false, NULL);

}

}

diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/sifive_e.c

+++ b/hw/riscv/sifive_e.c

@@ -XXX,XX +XXX,XX @@ static void sifive_e_machine_init(MachineState *machine)

if (machine->kernel_filename) {

riscv_load_kernel(machine, &s->soc.cpus,

- memmap[SIFIVE_E_DEV_DTIM].base, NULL);

+ memmap[SIFIVE_E_DEV_DTIM].base,

+ false, NULL);

}

}

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/sifive_u.c

+++ b/hw/riscv/sifive_u.c

@@ -XXX,XX +XXX,XX @@ static void sifive_u_machine_init(MachineState *machine)

firmware_end_addr);

kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,

- kernel_start_addr, NULL);

-

- if (machine->initrd_filename) {

- riscv_load_initrd(machine, kernel_entry);

- }

-

- if (machine->kernel_cmdline && *machine->kernel_cmdline) {

- qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",

- machine->kernel_cmdline);

- }

+ kernel_start_addr, true, NULL);

} else {

/*

* If dynamic firmware is used, it doesn't know where is the next mode

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/spike.c

+++ b/hw/riscv/spike.c

@@ -XXX,XX +XXX,XX @@ static void spike_board_init(MachineState *machine)

kernel_entry = riscv_load_kernel(machine, &s->soc[0],

kernel_start_addr,

- htif_symbol_callback);

-

- if (machine->initrd_filename) {

- riscv_load_initrd(machine, kernel_entry);

- }

-

- if (machine->kernel_cmdline && *machine->kernel_cmdline) {

- qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",

- machine->kernel_cmdline);

- }

+ true, htif_symbol_callback);

} else {

/*

* If dynamic firmware is used, it doesn't know where is the next mode

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/virt.c

+++ b/hw/riscv/virt.c

@@ -XXX,XX +XXX,XX @@ static void virt_machine_done(Notifier *notifier, void *data)

firmware_end_addr);

kernel_entry = riscv_load_kernel(machine, &s->soc[0],

- kernel_start_addr, NULL);

-

- if (machine->initrd_filename) {

- riscv_load_initrd(machine, kernel_entry);

- }

-

- if (machine->kernel_cmdline && *machine->kernel_cmdline) {

- qemu_fdt_setprop_string(machine->fdt, "/chosen", "bootargs",

- machine->kernel_cmdline);

- }

+ kernel_start_addr, true, NULL);

} else {

/*

* If dynamic firmware is used, it doesn't know where is the next mode

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h

index XXXXXXX..XXXXXXX 100644

--- a/include/hw/riscv/boot.h

+++ b/include/hw/riscv/boot.h

@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,

target_ulong riscv_load_kernel(MachineState *machine,

RISCVHartArrayState *harts,

target_ulong firmware_end_addr,

+ bool load_initrd,

symbol_fn_t sym_cb);

void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);

uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,

2.39.1

The only remaining caller is riscv_load_kernel_and_initrd() which

belongs to the same file.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Bin Meng <bmeng@tinylab.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

hw/riscv/boot.c | 80 ++++++++++++++++++++---------------------

include/hw/riscv/boot.h | 1 -

2 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c

--- a/hw/riscv/boot.c

+++ b/hw/riscv/boot.c

@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,

exit(1);

+static void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry)

+ const char *filename = machine->initrd_filename;

+ uint64_t mem_size = machine->ram_size;

+ void *fdt = machine->fdt;

+ hwaddr start, end;

+ ssize_t size;

* This function makes an assumption that the DRAM interval

* 'dram_base' + 'dram_size' is contiguous.

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h

--- a/include/hw/riscv/boot.h

+++ b/include/hw/riscv/boot.h

@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_kernel(MachineState *machine,

target_ulong firmware_end_addr,

bool load_initrd,

symbol_fn_t sym_cb);

-void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);

uint64_t riscv_compute_fdt_addr(hwaddr dram_start, uint64_t dram_size,

MachineState *ms);

void riscv_load_fdt(hwaddr fdt_addr, void *fdt);

2.39.1

load_elf_ram_sym() will sign-extend 32 bit addresses. If a 32 bit QEMU

guest happens to be running in a hypervisor that are using 64 bits to

encode its address, kernel_entry can be padded with '1's and create

problems [1].

Using a translate_fn() callback in load_elf_ram_sym() to filter the

padding from the address doesn't work. A more detailed explanation can

be found in [2]. The short version is that glue(load_elf, SZ), from

include/hw/elf_ops.h, will calculate 'pentry' (mapped into the

'kernel_load_base' var in riscv_load_Kernel()) before using

translate_fn(), and will not recalculate it after executing it. This

means that the callback does not prevent the padding from

kernel_load_base to appear.

Let's instead use a kernel_low var to capture the 'lowaddr' value from

load_elf_ram_sim(), and return it when we're dealing with 32 bit CPUs.

[1] https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02281.html

[2] https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00099.html

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

---

hw/riscv/boot.c | 15 +++++++++++----

hw/riscv/microchip_pfsoc.c | 3 ++-

hw/riscv/opentitan.c | 3 ++-

hw/riscv/sifive_e.c | 3 ++-

hw/riscv/sifive_u.c | 3 ++-

hw/riscv/spike.c | 3 ++-

hw/riscv/virt.c | 3 ++-

include/hw/riscv/boot.h | 1 +

8 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/boot.c

+++ b/hw/riscv/boot.c

@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,

}

target_ulong riscv_load_kernel(MachineState *machine,

+ RISCVHartArrayState *harts,

target_ulong kernel_start_addr,

symbol_fn_t sym_cb)

{

const char *kernel_filename = machine->kernel_filename;

- uint64_t kernel_load_base, kernel_entry;

+ uint64_t kernel_load_base, kernel_entry, kernel_low;

g_assert(kernel_filename != NULL);

@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_kernel(MachineState *machine,

* the (expected) load address load address. This allows kernels to have

* separate SBI and ELF entry points (used by FreeBSD, for example).

*/

- if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,

- NULL, &kernel_load_base, NULL, NULL, 0,

+ if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL, NULL,

+ &kernel_load_base, &kernel_low, NULL, 0,

EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {

- return kernel_load_base;

+ kernel_entry = kernel_load_base;

+

+ if (riscv_is_32bit(harts)) {

+ kernel_entry = kernel_low;

+ }

+

+ return kernel_entry;

}

if (load_uimage_as(kernel_filename, &kernel_entry, NULL, NULL,

diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/microchip_pfsoc.c

+++ b/hw/riscv/microchip_pfsoc.c

@@ -XXX,XX +XXX,XX @@ static void microchip_icicle_kit_machine_init(MachineState *machine)

kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,

firmware_end_addr);

- kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);

+ kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,

+ kernel_start_addr, NULL);

if (machine->initrd_filename) {

riscv_load_initrd(machine, kernel_entry);

diff --git a/hw/riscv/opentitan.c b/hw/riscv/opentitan.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/opentitan.c

+++ b/hw/riscv/opentitan.c

@@ -XXX,XX +XXX,XX @@ static void opentitan_board_init(MachineState *machine)

}

if (machine->kernel_filename) {

- riscv_load_kernel(machine, memmap[IBEX_DEV_RAM].base, NULL);

+ riscv_load_kernel(machine, &s->soc.cpus,

+ memmap[IBEX_DEV_RAM].base, NULL);

}

}

diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/sifive_e.c

+++ b/hw/riscv/sifive_e.c

@@ -XXX,XX +XXX,XX @@ static void sifive_e_machine_init(MachineState *machine)

memmap[SIFIVE_E_DEV_MROM].base, &address_space_memory);

if (machine->kernel_filename) {

- riscv_load_kernel(machine, memmap[SIFIVE_E_DEV_DTIM].base, NULL);

+ riscv_load_kernel(machine, &s->soc.cpus,

+ memmap[SIFIVE_E_DEV_DTIM].base, NULL);

}

}

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/sifive_u.c

+++ b/hw/riscv/sifive_u.c

@@ -XXX,XX +XXX,XX @@ static void sifive_u_machine_init(MachineState *machine)

kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc.u_cpus,

firmware_end_addr);

- kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);

+ kernel_entry = riscv_load_kernel(machine, &s->soc.u_cpus,

+ kernel_start_addr, NULL);

if (machine->initrd_filename) {

riscv_load_initrd(machine, kernel_entry);

diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/spike.c

+++ b/hw/riscv/spike.c

@@ -XXX,XX +XXX,XX @@ static void spike_board_init(MachineState *machine)

kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],

firmware_end_addr);

- kernel_entry = riscv_load_kernel(machine, kernel_start_addr,

+ kernel_entry = riscv_load_kernel(machine, &s->soc[0],

+ kernel_start_addr,

htif_symbol_callback);

if (machine->initrd_filename) {

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c

index XXXXXXX..XXXXXXX 100644

--- a/hw/riscv/virt.c

+++ b/hw/riscv/virt.c

@@ -XXX,XX +XXX,XX @@ static void virt_machine_done(Notifier *notifier, void *data)

kernel_start_addr = riscv_calc_kernel_start_addr(&s->soc[0],

firmware_end_addr);

- kernel_entry = riscv_load_kernel(machine, kernel_start_addr, NULL);

+ kernel_entry = riscv_load_kernel(machine, &s->soc[0],

+ kernel_start_addr, NULL);

if (machine->initrd_filename) {

riscv_load_initrd(machine, kernel_entry);

diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h

index XXXXXXX..XXXXXXX 100644

--- a/include/hw/riscv/boot.h

+++ b/include/hw/riscv/boot.h

@@ -XXX,XX +XXX,XX @@ target_ulong riscv_load_firmware(const char *firmware_filename,

hwaddr firmware_load_addr,

symbol_fn_t sym_cb);

target_ulong riscv_load_kernel(MachineState *machine,

+ RISCVHartArrayState *harts,

target_ulong firmware_end_addr,

symbol_fn_t sym_cb);

void riscv_load_initrd(MachineState *machine, uint64_t kernel_entry);

--

2.39.1