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

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