Re: [PATCH v10 1/3] hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel()

[Bin Meng]

Hi Daniel,

On Fri, Feb 3, 2023 at 6:31 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 2/3/23 02:39, Bin Meng wrote:
> > On Thu, Feb 2, 2023 at 9:58 PM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >> 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].
> >
> > Still this commit message is inaccurate. It's not
> >
> > "a 32-bit QEMU guest happens to running in a hypervisor that are using
> > 64 bits to encode tis address"
> >
> > as a 32-bit ELF can only hold a 32-bit address, but it's the QEMU ELF
> > loader that does the sign extension and returns the address as
> > uint64_t. It has nothing to do with hypervisor too.
>
>
> Yeah I'm still focusing too much on the use case instead of the root of the
> problem (sign-extension from QEMU elf).
>
> >
> >>
> >> 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.
> >
> > Looking at the prototype of load_elf_ram_sym() it has
> >
> > ssize_t load_elf_ram_sym(const char *filename,
> >                           uint64_t (*elf_note_fn)(void *, void *, bool),
> >                           uint64_t (*translate_fn)(void *, uint64_t),
> >                           void *translate_opaque, uint64_t *pentry,
> >                           uint64_t *lowaddr, uint64_t *highaddr,
> >                           uint32_t *pflags, int big_endian, int elf_machine,
> >                           int clear_lsb, int data_swab,
> >                           AddressSpace *as, bool load_rom, symbol_fn_t 
> > sym_cb)
> >
> > So kernel_low is the "highaddr" parameter, not the 'lowaddr' value.
>
> And for some reason I thought kernel_base_addr was being used as 'pentry'. 
> kernel_base_addr
> is already 'lowaddr'. Guess I'll have to rewrite the commit message. And 
> revisit why my
> test case worked for riscv32 (I probably didn't use an ELF image ...)
>
> And the only way out seems to be filtering the bits from lowaddr. I'll do 
> that.
>

Can you check as to why QEMU ELF loader does the sign extension?

I personally don't know why. Maybe the ELF loader does something wrong.

Regards,
Bin