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

[Bin Meng]

Hi Alistair,

On Thu, Jan 12, 2023 at 8:36 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> 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.

Shouldn't the bug be the 32-bit Xvisor image? How can a 32-bit image
return an address 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?
>

Regards,
Bin