[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v10 1/3] hw/riscv: handle 32 bit CPUs kernel_addr in riscv_lo
Re: [PATCH v10 1/3] hw/riscv: handle 32 bit CPUs kernel_addr in riscv_load_kernel()
Fri, 3 Feb 2023 18:45:42 +0800
On Fri, Feb 3, 2023 at 6:31 PM Daniel Henrique Barboza
> On 2/3/23 02:39, Bin Meng wrote:
> > On Thu, Feb 2, 2023 at 9:58 PM Daniel Henrique Barboza
> > <firstname.lastname@example.org> 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 .
> > 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 . 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'.
> 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
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.
[PATCH v10 2/3] hw/riscv/boot.c: consolidate all kernel init in riscv_load_kernel(), Daniel Henrique Barboza, 2023/02/02
[PATCH v10 3/3] hw/riscv/boot.c: make riscv_load_initrd() static, Daniel Henrique Barboza, 2023/02/02
Re: [PATCH v10 0/3] hw/riscv: handle kernel_entry high bits with 32bit CPUs, Conor Dooley, 2023/02/02