On Mon, Jan 16, 2023 at 8:37 PM Philippe Mathieu-Daudé <phi...@linaro.org> wrote: > > On 16/1/23 13:29, Daniel Henrique Barboza wrote: > > Recent hw/risc/boot.c changes caused a regression in an use case with > > the Xvisor hypervisor. Running a 32 bit QEMU guest with '-kernel' > > stopped working. The reason seems to be that Xvisor is using 64 bit to > > encode the 32 bit addresses from the guest, and load_elf_ram_sym() is > > sign-extending the result with '1's [1]. > > > > Use a translate_fn() callback to be called by load_elf_ram_sym() and > > return only the 32 bits address if we're running a 32 bit CPU. > > > > [1] https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg02281.html > > > > Suggested-by: Philippe Mathieu-Daudé <phi...@linaro.org> > > Suggested-by: Bin Meng <bmeng...@gmail.com> > > Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com> > > --- > > hw/riscv/boot.c | 20 +++++++++++++++++++- > > hw/riscv/microchip_pfsoc.c | 4 ++-- > > hw/riscv/opentitan.c | 3 ++- > > hw/riscv/sifive_e.c | 3 ++- > > hw/riscv/sifive_u.c | 4 ++-- > > hw/riscv/spike.c | 2 +- > > hw/riscv/virt.c | 4 ++-- > > include/hw/riscv/boot.h | 1 + > > target/riscv/cpu_bits.h | 1 + > > 9 files changed, 32 insertions(+), 10 deletions(-) > > > > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c > > index e868fb6ade..0fd39df7f3 100644 > > --- a/hw/riscv/boot.c > > +++ b/hw/riscv/boot.c > > @@ -213,7 +213,24 @@ static void riscv_load_initrd(MachineState *machine, > > uint64_t kernel_entry) > > } > > } > > > > +static uint64_t translate_kernel_address(void *opaque, uint64_t addr) > > +{ > > + RISCVHartArrayState *harts = opaque; > > + > > + /* > > + * For 32 bit CPUs, kernel_load_base is sign-extended (i.e. > > + * it can be padded with '1's) if the hypervisor is using > > + * 64 bit addresses with 32 bit guests. > > + */ > > + if (riscv_is_32bit(harts)) { > > Maybe move the comment within the if() and add " so remove the sign > extension by truncating to 32-bit". > > > + return extract64(addr, 0, RV32_KERNEL_ADDR_LEN); > > For 32-bit maybe a definition is not necessary, I asked before > you used 24-bit in the previous version. As the maintainer prefer :) > > > + } > > + > > + return addr; > > +} > > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > > index 8b0d7e20ea..8fcaeae342 100644 > > --- a/target/riscv/cpu_bits.h > > +++ b/target/riscv/cpu_bits.h > > @@ -751,6 +751,7 @@ typedef enum RISCVException { > > #define MENVCFG_STCE (1ULL << 63) > > > > /* For RV32 */ > > +#define RV32_KERNEL_ADDR_LEN 32 > > #define MENVCFGH_PBMTE BIT(30) > > #define MENVCFGH_STCE BIT(31) > > Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org> >
Isn't the problem in the ELF loader? Why does it return a 64-bit signed extended address given the 32-bit ELF image? Regards, Bin