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

[Alistair Francis]

On Wed, Jun 19, 2019 at 8:30 AM Bin Meng <address@hidden> wrote:
>
> Hi,
>
> On Wed, Jun 19, 2019 at 11:26 PM Jonathan Behrens <address@hidden> wrote:
> >
> > 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.
> >
>
> Yes, current kernel_translate() logic is tightly coupled to the kernel
> entry VA, and if we link kernel at some other address it will just
> fail.

I thought this was required but it looks like it isn't. I have remove
the kernel_translate() function.

>
> > 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?

Removed.

> >>
> >> > +    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.
> >>
>
> So why not simply return KERNEL_BOOT_ADDRESS in kernel_translate()?

That's what I am doing now.

Alistair

>
> Regards,
> Bin