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().
Thoughts?
Daniel
[1]
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc
Regards,
Bin
