Hi Alistair,
On Tue, May 5, 2020 at 12:00 AM Alistair Francis <alistai...@gmail.com> wrote:
>
> On Mon, May 4, 2020 at 1:05 AM Bin Meng <bmeng...@gmail.com> wrote:
> >
> > On Mon, May 4, 2020 at 4:00 PM Bin Meng <bmeng...@gmail.com> wrote:
> > >
> > > Hi Anup,
> > >
> > > On Mon, May 4, 2020 at 3:52 PM Anup Patel <a...@brainfault.org> wrote:
> > > >
> > > > On Mon, May 4, 2020 at 12:46 PM Bin Meng <bmeng...@gmail.com> wrote:
> > > > >
> > > > > Hi Anup,
> > > > >
> > > > > On Sun, May 3, 2020 at 12:38 PM Anup Patel <a...@brainfault.org>
> > > > > wrote:
> > > > > >
> > > > > > On Fri, May 1, 2020 at 9:26 PM Bin Meng <bmeng...@gmail.com> wrote:
> > > > > > >
> > > > > > > From: Bin Meng <bin.m...@windriver.com>
> > > > > > >
> > > > > > > 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.
>
> Hey Bin,
>
> Thanks for the patch!
>
> I don't think we can take this update though, as we should stick with
> OpenSBI releases.
Sure, will delay this series once OpenSBI v0.8 release is out.
>
> > > > > > >
> > > > > > > Signed-off-by: Bin Meng <bin.m...@windriver.com>
> > > > > > > ---
> > > > > > >
> > > > > > > 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.
>
> I don't really mind either way.
>
> On one hand it is nice to have all the boards "just work" with
> OpenSBI. On the other hand Spike isn't used very often from what I can
> tell and it's a larger maintenance burden to update another image.
>
> >
> > One additional note, that's why patch 5 in this series for. Without
> > the default bios image being shipped in QEMU, QEMU testing will
> > complain.
> >
> > So in the future, when we have more QEMU RISC-V machines added, if
> > they are not using the generic firmware, do we want to ship all of
> > these different firmware images in QEMU?
>
> That's a good point.
>
> I don't really want to be maintaining 20 different OpenSBI binaries.
>
> I suspect the plan will be that we supply OpenSBI binaries for the
> "key" boards. Which is the Virt board and sifive_u. If other boards
> can use the same OpenSBI binary then that's great. If not we won't
> ship a binary. The main reason to ship the binary is to allow people
> to try RISC-V easily. The virt and sifive_u machines do that, we don't
> need them all to do that.
>
> So on that note, I think we should include the generic elf which will
> support Spike and any future boards that need the elf as well.
>
> So to summarise my ramblings:
> - We will swap to shipping generic ELF and BIN OpenSBI files
> - All boards that can use those should
> - Other boards won't have pre-built OpenSBI support
> - For future updates we just update the 4 files (32-bit and 64-bit)
> on each release.
Sounds good to me. Thanks for providing the guideline from the
maintainer's view!
Regards,
Bin
