Re: [PATCH v5 10/11] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Alistair Francis]

On Mon, Jan 2, 2023 at 9:55 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> 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>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  hw/riscv/boot.c            | 22 +++++++++++++++++++---
>  hw/riscv/microchip_pfsoc.c | 12 ++----------
>  hw/riscv/opentitan.c       |  2 +-
>  hw/riscv/sifive_e.c        |  3 ++-
>  hw/riscv/sifive_u.c        | 12 ++----------
>  hw/riscv/spike.c           | 11 +----------
>  hw/riscv/virt.c            | 12 ++----------
>  include/hw/riscv/boot.h    |  1 +
>  8 files changed, 30 insertions(+), 45 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 2594276223..4888d5c1e0 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -175,10 +175,12 @@ target_ulong riscv_load_firmware(const char 
> *firmware_filename,
>
>  target_ulong riscv_load_kernel(MachineState *machine,
>                                 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;
> +    void *fdt = machine->fdt;
>
>      g_assert(kernel_filename != NULL);
>
> @@ -192,21 +194,35 @@ target_ulong riscv_load_kernel(MachineState *machine,
>      if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
>                           NULL, &kernel_load_base, NULL, NULL, 0,
>                           EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
> -        return kernel_load_base;
> +        kernel_entry = kernel_load_base;

This breaks 32-bit Xvisor loading. It seems that for the 32-bit guest
we get a value of 0xffffffff80000000.

Previously the top bits would be lost as we return a target_ulong from
this function, but with this change we pass the value
0xffffffff80000000 to riscv_load_initrd() which causes failures.

This diff fixes the failure for me

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 4888d5c1e0..f08ed44b97 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -194,7 +194,7 @@ target_ulong riscv_load_kernel(MachineState *machine,
    if (load_elf_ram_sym(kernel_filename, NULL, NULL, NULL,
                         NULL, &kernel_load_base, NULL, NULL, 0,
                         EM_RISCV, 1, 0, NULL, true, sym_cb) > 0) {
-        kernel_entry = kernel_load_base;
+        kernel_entry = (target_ulong) kernel_load_base;
        goto out;
    }


but I don't think that's the right fix. We should instead look at the
CPU XLEN and drop the high bits if required.

I'm going to drop this patch, do you mind looking into a proper fix?

Alistair