On Sat, Feb 4, 2023 at 7:01 AM Daniel Henrique Barboza <dbarb...@ventanamicro.com> wrote: > > Hey, > > On 2/3/23 07:45, Bin Meng wrote: > > 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. > > > I took a look and didn't find out why. I checked that in the ELF > specification some > file headers can indicate a sign extension. E.g. R_X86_64_32S for example is > described as > "Direct 32 bit zero extended". Note that the use of the tags are dependent on > how the > ELF was built, so we would need the exact ELF to check for that. All I can > say is that > there is a sign extension going on, in the 'lowaddr' field, and that > translate_fn() > wasn't enough to filter it out. I can't say whether this is intended or a bug. > > > But going back a little, this whole situation happened in v5 because, in the > next > patch, riscv_load_initrd() started to receive an uint64_t (kernel_entry) > instead of > receiving a target_ulong like it was doing before. This behavior change, > which was > accidental, not only revealed this sign-extension behavior but also > potentially changed > what riscv_load_initrd() is receiving from load_uimage_as() the same way it's > impacting load_elf_ram_sym(). The third load option, > load_image_targphys_as(), is > already using a target_ulong (kernel_start_addr) as return value so it's not > impacted. > > I believe Alistair suggested to clear bits instead of just doing a > target_ulong > cast for a reason (I guess we're trying to gate all 32/64 bit CPU logic using > a > direct approach such as checking the CPU directly), but I also think we should > clear bits for all 'kernel_entry' possibilities, not just the one that comes > from > load_elf_ram_sym(), to be consistent in all three cases. We might be even > avoiding > a future headache from load_uimage_as().
That seems like the approach to take. That way we aren't changing behaviour and it continues along with improving/adding RV32 support on 64-bit bit targets. If we want to change the behaviour in the future, we can add a patch that does that with a description of why. Alistair > > > > Thoughts? > > > Daniel > > > [1] > https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc > > > > > Regards, > > Bin >
