Re: [edk2-staging/RiscV64QemuVirt PATCH 26/29] OvmfPkg: Add generic Qemu NOR flash DXE driver

["Sunil V L" <sunilvl@ventanamicro.com>]

On Mon, Oct 10, 2022 at 06:16:28PM +0200, Ard Biesheuvel wrote:
> On Mon, 10 Oct 2022 at 18:05, Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > On Mon, Oct 10, 2022 at 05:29:15PM +0200, Ard Biesheuvel wrote:
> > > On Mon, 10 Oct 2022 at 17:19, Sunil V L <sunilvl@ventanamicro.com> wrote:
> > > >
> > > > On Mon, Oct 10, 2022 at 12:39:21PM +0200, Ard Biesheuvel wrote:
> > > > > On Mon, 10 Oct 2022 at 12:13, Sunil V L <sunilvl@ventanamicro.com> wrote:
> > > > > >
> > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4076
> > > > > >
> > > > > > RISC-V needs NorFlashDxe driver for qemu virt machine. The
> > > > > > ArmPlatformPkg has this driver but migrating it to generic
> > > > > > package like MdeModulePkg introduces circular dependencies.
> > > > > > So, add a simplified version of the NorFlashDxe driver in
> > > > > > OvmfPkg.
> > > > > >
> > > > >
> > > > > So what is the difference between this simplified version and the old
> > > > > version? And it is backed in QEMU by the same NOR flash emulation,
> > > > > shouldn't we use this driver for ArmVirtQemu as well?
> > > >
> > > > I agree. If we can break the dependency on EmbeddedPkg due to
> > > > NvVarStoreFormattedLib, then we can migrate to MdeModulePkg and all
> > > > consumers can use the same driver which would be the best solution IMO.
> > > >
> > > > Could you please let me know  why NvVarStoreFormattedLib is added
> > > > in EmbeddedPkg instead of MdePkg or MdeModulePkg? Is it only for
> > > > non-server class platforms? I don't see it doing much so not sure
> > > > its use case.
> > > >
> > >
> > > I think that library as well as the definition of
> > > gEdkiiNvVarStoreFormattedGuid should be moved to MdeModulePkg.
> > >
> > > Then, we can look at moving NorFlashDxe in there as well.
> >
> > Great. Let me rework these patches then.
> > >
> > > But you haven't answered my question regarding how your version was simplified.
> > >
> >
> > Oh sorry. I forgot to modify the commit message but what I really meant
> > was removing the code which depends upon PcdNorFlashCheckBlockLocked for
> > virtual platforms. I thought it is useful only for real platforms and
> > qemu doesn't emulate it. Let me know if I am wrong.
> > Since the driver is copied for OVMF, I didn't want to add the PCD unnecessarily.
> >
> 
> What I would like to do is the following:
> 
> - move NvVarstoreFormattedLib into MdeModulePkg (as you proposed already)
> - create a new NorFlashDxe in Ovmf that is cleaned up, drops the
> handling of locked blocks (as you suggest) and minimizes the number of
> transitions between array mode and programming mode (i have some
> patches i can share as a starting point)
> - move ArmPlatformPkg's NorFlashDxe into its only remaining user,
> which is is Platform/ARM in edk2-platforms (there are two more users
> in edk2-platforms, but both are QEMU based on so they can switch to
> the OVMF one)

Interesting. In that case, we can continue with current approach of copy in Ovmf
and add the optimization. I was assuming you want the same driver for
real and virtual platforms in MdeModulePkg :-).

Sure, please share those optimization changes. I will create patches as
you have outlined here.

> 
> 
> > > Note that there is some room for improvement in that driver in
> > > relation to execution under KVM: switching between programming mode
> > > and array mode involves setting up/tearing down the KVM memslot, and
> > > currently, the driver is far from optimized when it comes to
> > > minimizing the number of transitions between read mode and write mode.
> >
> > I was not looking at optimizing it at this point of time.
> >
> 
> I understand. And yet you are already proposing a different version of
> NorFlashDxe, and I don't want to clone the existing NorFlashDxe
> without cleaning it up first.

Understood. Thanks for your help.

Thanks
Sunil