| Message ID | e718da8df07915765217dece609255b6ad196955.1560904640.git.alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: Add firmware loading support and default | expand |
On Wed, Jun 19, 2019 at 8:53 AM Alistair Francis <alistair.francis@wdc.com> 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 <alistair.francis@wdc.com> > --- > 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
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 <bmeng.cn@gmail.com> wrote: > On Wed, Jun 19, 2019 at 8:53 AM Alistair Francis > <alistair.francis@wdc.com> 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 <alistair.francis@wdc.com> > > --- > > 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 > >
Hi, On Wed, Jun 19, 2019 at 11:26 PM Jonathan Behrens <fintelia@gmail.com> 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. > Jonathan > > On Wed, Jun 19, 2019 at 11:16 AM Bin Meng <bmeng.cn@gmail.com> wrote: >> >> On Wed, Jun 19, 2019 at 8:53 AM Alistair Francis >> <alistair.francis@wdc.com> 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 <alistair.francis@wdc.com> >> > --- >> > 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. >> So why not simply return KERNEL_BOOT_ADDRESS in kernel_translate()? Regards, Bin
On Wed, Jun 19, 2019 at 8:30 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi, > > On Wed, Jun 19, 2019 at 11:26 PM Jonathan Behrens <fintelia@gmail.com> 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 <bmeng.cn@gmail.com> wrote: > >> > >> On Wed, Jun 19, 2019 at 8:53 AM Alistair Francis > >> <alistair.francis@wdc.com> 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 <alistair.francis@wdc.com> > >> > --- > >> > 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
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. + */ + if (machine->firmware) { + return (addr & 0x7fffffff) + KERNEL_BOOT_ADDRESS; + } else { + return addr; + } +} + +target_ulong riscv_load_firmware(const char *firmware_filename) +{ + uint64_t firmware_entry, firmware_start, firmware_end; + + if (load_elf(firmware_filename, NULL, NULL, NULL, + &firmware_entry, &firmware_start, &firmware_end, + 0, EM_RISCV, 1, 0) < 0) { + error_report("could not load firmware '%s'", firmware_filename); + exit(1); + } + + return firmware_entry; +} + +target_ulong riscv_load_kernel(MachineState *machine, + const char *kernel_filename) { uint64_t kernel_entry, kernel_high; - if (load_elf(kernel_filename, NULL, NULL, NULL, + if (load_elf(kernel_filename, NULL, kernel_translate, machine, &kernel_entry, NULL, &kernel_high, 0, EM_RISCV, 1, 0) < 0) { error_report("could not load kernel '%s'", kernel_filename); diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c index 9d58ae362b..3695c686be 100644 --- a/hw/riscv/sifive_e.c +++ b/hw/riscv/sifive_e.c @@ -118,7 +118,7 @@ static void riscv_sifive_e_init(MachineState *machine) memmap[SIFIVE_E_MROM].base, &address_space_memory); if (machine->kernel_filename) { - riscv_load_kernel(machine->kernel_filename); + riscv_load_kernel(machine, machine->kernel_filename); } } diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 1b9281bd4a..03a6c64d04 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -266,8 +266,12 @@ static void riscv_sifive_u_init(MachineState *machine) /* create device tree */ create_fdt(s, memmap, machine->ram_size, machine->kernel_cmdline); + if (machine->firmware) { + riscv_load_firmware(machine->firmware); + } + if (machine->kernel_filename) { - riscv_load_kernel(machine->kernel_filename); + riscv_load_kernel(machine, machine->kernel_filename); } /* reset vector */ diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c index e68be00a5f..81cef0dcea 100644 --- a/hw/riscv/spike.c +++ b/hw/riscv/spike.c @@ -186,7 +186,7 @@ static void spike_board_init(MachineState *machine) mask_rom); if (machine->kernel_filename) { - riscv_load_kernel(machine->kernel_filename); + riscv_load_kernel(machine, machine->kernel_filename); } /* reset vector */ @@ -274,7 +274,7 @@ static void spike_v1_10_0_board_init(MachineState *machine) mask_rom); if (machine->kernel_filename) { - riscv_load_kernel(machine->kernel_filename); + riscv_load_kernel(machine, machine->kernel_filename); } /* reset vector */ @@ -359,7 +359,7 @@ static void spike_v1_09_1_board_init(MachineState *machine) mask_rom); if (machine->kernel_filename) { - riscv_load_kernel(machine->kernel_filename); + riscv_load_kernel(machine, machine->kernel_filename); } /* reset vector */ diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index 5f8c11471b..d3670b5a7c 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -380,8 +380,13 @@ static void riscv_virt_board_init(MachineState *machine) memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base, mask_rom); + if (machine->firmware) { + riscv_load_firmware(machine->firmware); + } + if (machine->kernel_filename) { - uint64_t kernel_entry = riscv_load_kernel(machine->kernel_filename); + uint64_t kernel_entry = riscv_load_kernel(machine, + machine->kernel_filename); if (machine->initrd_filename) { hwaddr start; diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h index f84fd6c2df..6f586939c7 100644 --- a/include/hw/riscv/boot.h +++ b/include/hw/riscv/boot.h @@ -20,7 +20,9 @@ #ifndef RISCV_BOOT_H #define RISCV_BOOT_H -target_ulong riscv_load_kernel(const char *kernel_filename); +target_ulong riscv_load_firmware(const char *firmware_filename); +target_ulong riscv_load_kernel(MachineState *machine, + const char *kernel_filename); hwaddr riscv_load_initrd(const char *filename, uint64_t mem_size, uint64_t kernel_entry, hwaddr *start);
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 <alistair.francis@wdc.com> --- 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(-)