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

[Alistair Francis]

On Sat, Feb 4, 2023 at 7:01 AM Daniel Henrique Barboza
<dbarboza@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
> > <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.
>
>
> 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
>