public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ard Biesheuvel" <ardb@kernel.org>
To: Sunil V L <sunilvl@ventanamicro.com>
Cc: devel@edk2.groups.io, Ard Biesheuvel <ardb+tianocore@kernel.org>,
	 Jiewen Yao <jiewen.yao@intel.com>,
	Jordan Justen <jordan.l.justen@intel.com>,
	 Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [edk2-staging/RiscV64QemuVirt PATCH 26/29] OvmfPkg: Add generic Qemu NOR flash DXE driver
Date: Mon, 10 Oct 2022 18:16:28 +0200	[thread overview]
Message-ID: <CAMj1kXH5XDOTyMyeVL-4ZTNQ2Q45a1=x6XETcUBRQymuEGSrnQ@mail.gmail.com> (raw)
In-Reply-To: <Y0RCyI6wyByw+U3U@sunil-laptop>

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)


> > 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.

  reply	other threads:[~2022-10-10 16:16 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-10 10:11 [edk2-staging/RiscV64QemuVirt PATCH 00/29] Add support for RISC-V virt machine Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 01/29] MdePkg/Register: Add register definition header files for RISC-V Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 02/29] MdePkg: Add RISCV_EFI_BOOT_PROTOCOL related definitions Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 03/29] MdePkg/BaseLib: RISC-V: Add few more helper functions Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 04/29] MdePkg: Add BaseRiscVSbiLib Library for RISC-V Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 05/29] OvmfPkg/PlatformInitLib: Refactor to allow other architectures Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 06/29] OvmfPkg/PlatformInitLib: Add support for RISC-V Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 07/29] OvmfPkg/ResetSystemLib: Refactor to allow other architectures Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 08/29] OvmfPkg/ResetSystemLib: Add support for RISC-V Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 09/29] OvmfPkg/Sec: Refactor to allow other architectures Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 10/29] OvmfPkg/Sec: Add RISC-V support Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 11/29] OvmfPkg/PlatformPei: Refactor to allow other architectures Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 12/29] OvmfPkg/PlatformPei: Add support for RISC-V Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 13/29] UefiCpuPkg/CpuTimerLib: Refactor to allow other architectures Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 14/29] UefiCpuPkg/CpuTimerLib: Add support for RISC-V Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 15/29] UefiCpuPkg/CpuExceptionHandlerLib: Refactor to allow other architectures Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 16/29] UefiCpuPkg/CpuExceptionHandlerLib: Add support for RISC-V Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 17/29] UefiCpuPkg/CpuDxe: Refactor to allow other architectures Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 18/29] UefiCpuPkg/CpuDxe: Add support for RISC-V Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 19/29] UefiCpuPkg/CpuDxe: Add RISC-V Boot protocol support Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 20/29] UefiCpuPkg: Add CpuTimerDxe module Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 21/29] ArmVirtPkg/PlatformHasAcpiDtDxe: Move to OvmfPkg Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 22/29] ArmVirtPkg: Fix up the location of PlatformHasAcpiDtDxe Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 23/29] ArmVirtPkg/PlatformBootManagerLib: Move to OvmfPkg Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 24/29] ArmVirtPkg: Fix up the paths to PlatformBootManagerLib Sunil V L
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 25/29] OvmfPkg: Add NorFlashQemuLib library Sunil V L
2022-10-19 12:19   ` Ard Biesheuvel
2022-10-19 12:40     ` Sunil V L
2022-10-19 13:05     ` [edk2-devel] " Sami Mujawar
2022-10-19 13:14       ` Ard Biesheuvel
2022-10-19 13:19         ` Sami Mujawar
2022-10-19 13:46           ` Leif Lindholm
2022-10-10 10:11 ` [edk2-staging/RiscV64QemuVirt PATCH 26/29] OvmfPkg: Add generic Qemu NOR flash DXE driver Sunil V L
2022-10-10 10:39   ` Ard Biesheuvel
2022-10-10 15:19     ` Sunil V L
2022-10-10 15:29       ` Ard Biesheuvel
2022-10-10 16:05         ` Sunil V L
2022-10-10 16:16           ` Ard Biesheuvel [this message]
2022-10-10 17:20             ` Sunil V L
2022-10-11 11:09               ` Ard Biesheuvel
2022-10-10 10:12 ` [edk2-staging/RiscV64QemuVirt PATCH 27/29] OvmfPkg: RiscVVirt: Add Qemu Virt platform support Sunil V L
2022-10-10 10:12 ` [edk2-staging/RiscV64QemuVirt PATCH 28/29] Maintainers.txt: Add entry for OvmfPkg/RiscVVirt Sunil V L
2022-10-10 10:12 ` [edk2-staging/RiscV64QemuVirt PATCH 29/29] UefiCpuPkg/UefiCpuPkg.ci.yaml: Ignore RISC-V file Sunil V L

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMj1kXH5XDOTyMyeVL-4ZTNQ2Q45a1=x6XETcUBRQymuEGSrnQ@mail.gmail.com' \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox