Hi Daniel, On Fri, Feb 3, 2023 at 6:31 PM Daniel Henrique Barboza <dbarb...@ventanamicro.com> wrote: > > > > On 2/3/23 02:39, Bin Meng wrote: > > On Thu, Feb 2, 2023 at 9:58 PM Daniel Henrique Barboza > > <dbarb...@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