Re: [PATCH v2] hw/riscv: virt: Assume M-mode FW in pflash0 only when "-bios none"

[Sunil V L]

On Wed, May 17, 2023 at 02:57:12PM +1000, Alistair Francis wrote:
> On Mon, May 8, 2023 at 9:45 PM Andrea Bolognani <abologna@redhat.com> wrote:
> >
> > On Mon, May 08, 2023 at 04:53:46PM +0530, Sunil V L wrote:
> > > On Mon, May 08, 2023 at 03:00:02AM -0700, Andrea Bolognani wrote:
> > > > I think that it's more important to align with other architectures.
> 
> That's true, ideally we want to match what people are already doing.
> 
> > > >
> > > > The number of people currently running edk2 on RISC-V is probably
> > > > vanishingly small, and in my opinion requiring them to tweak their
> > > > command lines a bit is a fair price to pay to avoid having to carry a
> > > > subtle difference between architectures for years to come.
> > >
> > > It is not just tweaking the command line. The current EDK2 will not work
> > > anymore if code is moved to plfash 0 since EDK2 assumed its entry point
> > > is in pflash1. I agree there may not be too many users but if we have
> > > to align with other archs, there will be combinations of qemu and
> > > edk2 versions which won't work.
> >
> > Right.
> >
> > > > With that in mind, my preference would be to go back to v1.
> > >
> > > Thanks!. If this is the preference,  we can request people to use proper
> > > versions of EDK2 with different qemu versions.
> >
> > Yeah, in the (not so) long run this will just not matter, as the
> > versions of edk2 and QEMU available to people will all implement the
> > new behavior. Better to optimize for the long future ahead of us
> > rather than causing ongoing pain for the sake of the few users of a
> > work-in-progress board.
> >
> > > > Taking a step back, what is even the use case for having M-mode code
> > > > in pflash0? If you want to use an M-mode firmware, can't you just use
> > > > -bios instead? In other words, can we change the behavior so that
> > > > pflash being present always mean loading S-mode firmware off it?
> 
> It was originally added to support Oreboot (the Rust version of
> Coreboot). The idea was that Oreboot (ROM) would be in flash and then
> go from there.
> 
> It also applies to other ROM code that a user might want to test that
> runs before OpenSBI.
> 
> > >
> > > TBH, I don't know. I am sure Alistair would know since it was added in
> > > https://github.com/qemu/qemu/commit/1c20d3ff6004b600336c52cbef9f134fad3ccd94
> > > I don't think opensbi can be launched from pflash. So, it may be some
> > > other use case which I am now aware of.
> > >
> > > I will be happy if this can be avoided by using -bios.
> >
> > The actual commit would be [1], from late 2019. Things might have
> > changed in the intervening ~3.5 years. Let's wait to hear from
> > Alistair :)
> 
> Overall for this patch I don't feel strongly about following what ARM
> does or continuing with what we already have. I would prefer to match
> other archs if we can though.
> 
> Also, either way we should update the documentation in
> docs/system/riscv/virt.rst to describe what happens.
> 
Thanks! Alistair. My reminder mail was sent just before seeing this
response. Sorry about that.

Let me go back to v1 and also update the virt.rst and send v3.

Thanks!
Sunil