Re: [PATCH 1/5] roms/opensbi: Update to support building bios images for generic platform

[Bin Meng]

Hi Anup,

On Mon, May 4, 2020 at 3:52 PM Anup Patel <address@hidden> wrote:
>
> On Mon, May 4, 2020 at 12:46 PM Bin Meng <address@hidden> wrote:
> >
> > Hi Anup,
> >
> > On Sun, May 3, 2020 at 12:38 PM Anup Patel <address@hidden> wrote:
> > >
> > > On Fri, May 1, 2020 at 9:26 PM Bin Meng <address@hidden> wrote:
> > > >
> > > > From: Bin Meng <address@hidden>
> > > >
> > > > The RISC-V generic platform is a flattened device tree (FDT) based
> > > > platform where all platform specific functionality is provided based
> > > > on FDT passed by previous booting stage. The support was added in
> > > > upstream opensbi recently.
> > > >
> > > > Bring the QEMU roms/opensbi submodule to the upstream opensbi commit:
> > > > commit 4f18c6e55049 ("platform: generic: Add Sifive FU540 TLB flush 
> > > > range limit override")
> > > > with the following changes since v0.7 release:
> > > >
> > > >   1bb00ab lib: No need to provide default PMP region using platform 
> > > > callbacks
> > > >   a9eac67 include: sbi_platform: Combine reboot and shutdown into one 
> > > > callback
> > > >   6585fab lib: utils: Add SiFive test device
> > > >   4781545 platform: Add Nuclei UX600 platform
> > > >   3a326af scripts: adapt binary archive script for Nuclei UX600
> > > >   5bdf022 firmware: fw_base: Remove CSR_MTVEC update check
> > > >   e6c1345 lib: utils/serial: Skip baudrate config if input frequency is 
> > > > zero
> > > >   01a8c8e lib: utils: Improve fdt_parse_uart8250() API
> > > >   0a0093b lib: utils: Add fdt_parse_uart8250_node() function
> > > >   243b0d0 lib: utils: Remove redundant clint_ipi_sync() declaration
> > > >   e3ad7c1 lib: utils: Rename fdt_parse_clint() to 
> > > > fdt_parse_compat_addr()
> > > >   a39cd6f lib: utils: Add FDT match table based node lookup
> > > >   dd33b9e lib: utils: Make fdt_get_node_addr_size() public function
> > > >   66185b3 lib: utils: Add fdt_parse_sifive_uart_node() function
> > > >   19e966b lib: utils: Add fdt_parse_hart_id() function
> > > >   44dd7be lib: utils: Add fdt_parse_max_hart_id() API
> > > >   f0eb503 lib: utils: Add fdt_parse_plic_node() function
> > > >   1ac794c include: Add array_size() macro
> > > >   8ff2b94 lib: utils: Add simple FDT timer framework
> > > >   76f0f81 lib: utils: Add simple FDT ipi framework
> > > >   75322a6 lib: utils: Add simple FDT irqchip framework
> > > >   76a8940 lib: utils: Add simple FDT serial framework
> > > >   7cc6fa4 lib: utils: Add simple FDT reset framework
> > > >   4d06353 firmware: fw_base: Introduce optional fw_platform_init()
> > > >   f1aa9e5 platform: Add generic FDT based platform support
> > > >   1f21b99 lib: sbi: Print platform hart count at boot time
> > > >   2ba7087 scripts: Add generic platform to create-binary-archive.sh
> > > >   4f18c6e platform: generic: Add Sifive FU540 TLB flush range limit 
> > > > override
> > > >
> > > > Update our Makefile to build the generic platform instead of building
> > > > virt and sifive_u separately.
> > > >
> > > > Signed-off-by: Bin Meng <address@hidden>
> > > > ---
> > > >
> > > >  roms/Makefile | 30 ++++++++----------------------
> > > >  roms/opensbi  |  2 +-
> > > >  2 files changed, 9 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/roms/Makefile b/roms/Makefile
> > > > index f9acf39..cb00628 100644
> > > > --- a/roms/Makefile
> > > > +++ b/roms/Makefile
> > > > @@ -64,10 +64,8 @@ default help:
> > > >         @echo "  u-boot.e500        -- update u-boot.e500"
> > > >         @echo "  u-boot.sam460      -- update u-boot.sam460"
> > > >         @echo "  efi                -- update UEFI (edk2) platform 
> > > > firmware"
> > > > -       @echo "  opensbi32-virt     -- update OpenSBI for 32-bit virt 
> > > > machine"
> > > > -       @echo "  opensbi64-virt     -- update OpenSBI for 64-bit virt 
> > > > machine"
> > > > -       @echo "  opensbi32-sifive_u -- update OpenSBI for 32-bit 
> > > > sifive_u machine"
> > > > -       @echo "  opensbi64-sifive_u -- update OpenSBI for 64-bit 
> > > > sifive_u machine"
> > > > +       @echo "  opensbi32-generic  -- update OpenSBI for 32-bit 
> > > > generic machine"
> > > > +       @echo "  opensbi64-generic  -- update OpenSBI for 64-bit 
> > > > generic machine"
> > > >         @echo "  bios-microvm       -- update bios-microvm.bin (qboot)"
> > > >         @echo "  clean              -- delete the files generated by 
> > > > the previous" \
> > > >                                       "build targets"
> > > > @@ -170,29 +168,17 @@ skiboot:
> > > >  efi: edk2-basetools
> > > >         $(MAKE) -f Makefile.edk2
> > > >
> > > > -opensbi32-virt:
> > > > +opensbi32-generic:
> > > >         $(MAKE) -C opensbi \
> > > >                 CROSS_COMPILE=$(riscv32_cross_prefix) \
> > > > -               PLATFORM="qemu/virt"
> > > > -       cp opensbi/build/platform/qemu/virt/firmware/fw_jump.bin 
> > > > ../pc-bios/opensbi-riscv32-virt-fw_jump.bin
> > > > +               PLATFORM="generic"
> > > > +       cp opensbi/build/platform/generic/firmware/fw_jump.bin 
> > > > ../pc-bios/opensbi-riscv32-generic-fw_jump.bin
> > >
> > > I think you should copy fw_jump.elf as well because QEMU Spike
> > > platform needs it.
> > >
> >
> > I believe we intended only to ship default bios images for virt and
> > sifive_u. Spike bios image was not shipped in previous QEMU version
> > too.
>
> Is there a specific reason for not shipping bios image for Spike machine ?
>

That's only my guess.  Based on what I see from git history, adding
"-bios" support was added via:

commit 5b8a986350a9 ("hw/riscv/spike: Allow loading firmware
separately using -bios option")

with bios image as opensbi-riscv{32,64}-spike-fw_jump.elf, but the
images were not added to QEMU repo.

> There were issues booting Linux on QEMU spike machine which are
> now fixed and available in QEMU master. I think we should certainly
> ship fw_jump.elf for Spike machine. This way we have OpenSBI generic
> firmware available as a bios image for three QEMU machines(virt, sifive_u,
> and spike).
>

If everyone thinks shipping the ELF image is OK, I can do that in v2.

Regards,
Bin