Re: [Qemu-riscv] [Qemu-devel] [RFC v1 2/5] hw/riscv: Add support for loading a firmware

[Jonathan Behrens]

I was actually just writing up the same thing.  Shifting all the addresses in the ELF file by 2 or 4MB is somewhat surprising behavior, especially because this will apply to segments that are mapped even at much higher addresses. If you want a segment aligned to a 1GB superpage boundary you now need to place it slightly below so that it will be bumped up to the right place. Further, ANDing all addresses with 0x7fffffff makes it impossible to map anything beyond the first 2GB of RAM.

Jonathan

On Wed, Jun 19, 2019 at 11:16 AM Bin Meng <address@hidden> wrote:

On Wed, Jun 19, 2019 at 8:53 AM Alistair Francis
<address@hidden> wrote:
>
> Add support for loading a firmware file for the virt machine and the
> SiFive U. This can be run with the following command:
>
>     qemu-system-riscv64 -machine virt -bios fw_jump.elf -kernel vmlinux
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
>  hw/riscv/boot.c         | 41 +++++++++++++++++++++++++++++++++++++++--
>  hw/riscv/sifive_e.c     |  2 +-
>  hw/riscv/sifive_u.c     |  6 +++++-
>  hw/riscv/spike.c        |  6 +++---
>  hw/riscv/virt.c         |  7 ++++++-
>  include/hw/riscv/boot.h |  4 +++-
>  6 files changed, 57 insertions(+), 9 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 62f94aaf8a..392ca0cb2e 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -23,13 +23,50 @@
>  #include "exec/cpu-defs.h"
>  #include "hw/loader.h"
>  #include "hw/riscv/boot.h"
> +#include "hw/boards.h"
>  #include "elf.h"
>
> -target_ulong riscv_load_kernel(const char *kernel_filename)
> +#if defined(TARGET_RISCV32)
> +# define KERNEL_BOOT_ADDRESS 0x80400000
> +#else
> +# define KERNEL_BOOT_ADDRESS 0x80200000
> +#endif
> +
> +static uint64_t kernel_translate(void *opaque, uint64_t addr)
> +{
> +    MachineState *machine = opaque;
> +
> +    /*
> +     * If the user specified a firmware move the kernel to the offset
> +     * start address.
> +     */

Why?

> +    if (machine->firmware) {
> +        return (addr & 0x7fffffff) + KERNEL_BOOT_ADDRESS;

So with both "-bios" and "-kernel", the kernel address will be moved
to another address other than 0x80200000 (for 64-bit). This does not
look good to me.

> +    } else {
> +        return addr;
> +    }
> +}
> +

[snip]

Regards,
Bin