Re: [PATCH v6 1/2] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel()

[Philippe Mathieu-Daudé]

On 13/1/23 03:47, Bin Meng wrote:
> On Fri, Jan 13, 2023 at 6:37 AM 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_initrd(), 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            | 30 +++++++++++++++++++++++++++---
> >   hw/riscv/microchip_pfsoc.c | 12 ++----------
> >   hw/riscv/opentitan.c       |  3 ++-
> >   hw/riscv/sifive_e.c        |  4 +++-
> >   hw/riscv/sifive_u.c        | 13 +++----------
> >   hw/riscv/spike.c           | 10 +---------
> >   hw/riscv/virt.c            | 13 +++----------
> >   include/hw/riscv/boot.h    |  2 ++
> >   8 files changed, 43 insertions(+), 44 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 2594276223..e8e8b8517c 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, bool is_32bits,
> >                                  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,43 @@ 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;
> > +        /*
> > +         * kernel_load_base is sign-extended for 32 bits and can
> > +         * be padded with '1's. Do an uint32_t cast to avoid the
> > +         * padding if we're running a 32 bit CPU.
> > +         */
>
> I see both kernel_load_base and kernel_entry are declared as a
> uint64_t, and load_elf_ram_sym() accepts a uint64_t parameter. Where
> does the sign-extension happen?

Likely load_elf_ram_sym()'s translate_fn() argument is missing?

 * @translate_fn: optional function to translate load addresses
 * @translate_opaque: opaque data passed to @translate_fn

Others archs provide:

$ git grep -F '(void *opaque, uint64_t'
hw/alpha/dp264.c:23:static uint64_t cpu_alpha_superpage_to_phys(void 
*opaque, uint64_t addr)
hw/cris/boot.c:62:static uint64_t translate_kernel_address(void *opaque, 
uint64_t addr)
hw/hppa/machine.c:107:static uint64_t cpu_hppa_to_phys(void *opaque, 
uint64_t addr)
hw/intc/openpic.c:857:static uint64_t openpic_src_read(void *opaque, 
uint64_t addr, unsigned len)
hw/loongarch/virt.c:390:static uint64_t cpu_loongarch_virt_to_phys(void 
*opaque, uint64_t addr)
hw/microblaze/boot.c:112:static uint64_t translate_kernel_address(void 
*opaque, uint64_t addr)
hw/nios2/boot.c:78:static uint64_t translate_kernel_address(void 
*opaque, uint64_t addr)
hw/ppc/mac_newworld.c:117:static uint64_t translate_kernel_address(void 
*opaque, uint64_t addr)
hw/ppc/mac_oldworld.c:75:static uint64_t translate_kernel_address(void 
*opaque, uint64_t addr)
hw/ppc/spapr.c:1263:static uint64_t translate_kernel_address(void 
*opaque, uint64_t addr)
hw/s390x/ipl.c:106:static uint64_t bios_translate_addr(void *opaque, 
uint64_t srcaddr)
hw/sparc/sun4m.c:217:static uint64_t translate_kernel_address(void 
*opaque, uint64_t addr)
hw/sparc/sun4m.c:673:static uint64_t translate_prom_address(void 
*opaque, uint64_t addr)
hw/sparc64/sun4u.c:412:static uint64_t translate_prom_address(void 
*opaque, uint64_t addr)
hw/ssi/xlnx-versal-ospi.c:1613:static void ospi_indac_write(void 
*opaque, uint64_t value, unsigned int size)
hw/timer/pxa2xx_timer.c:117:static void pxa2xx_timer_update(void 
*opaque, uint64_t now_qemu)
hw/timer/pxa2xx_timer.c:134:static void pxa2xx_timer_update4(void 
*opaque, uint64_t now_qemu, int n)
hw/xtensa/sim.c:40:static uint64_t translate_phys_addr(void *opaque, 
uint64_t addr)
hw/xtensa/xtfpga.c:189:static uint64_t translate_phys_addr(void *opaque, 
uint64_t addr)

> > +        if (is_32bits) {
> > +            kernel_entry = (uint32_t)kernel_load_base;
> > +        }
> > +        goto out;
> >       }