Re: [PATCH v4 5/7] OvmfPkg/RiscVVirt: Add VirtNorFlashDxe to APRIORI list

["Tuan Phan" <tphan@ventanamicro.com>]

[-- Attachment #1: Type: text/plain, Size: 4102 bytes --]

On Tue, Jul 4, 2023 at 12:01 AM Sunil V L <sunilvl@ventanamicro.com> wrote:

> On Mon, Jul 03, 2023 at 11:45:45PM -0700, Tuan Phan wrote:
> > As i said, VirtNorFlashDxe needed to be loaded before VariableRuntimeDxe
> so
> > your suggestion will not work.
> >
> Okay, at least for me, by removing APRIORI patch and adding this depex,
> edk2 boots fine with your series. I am not sure what won't work.
>
> Hi Ard, any thoughts? If no better way, may be we have to use APRIORI.
>
It doesn't work as your workaround trying to make CpuDxe depends on
variable protocol which has nothing to do with it. Also, CpuDxe is an
essential module and should not depend on anything, what happens if the
variable driver before generating the protocol tries to use CPU protocol?
It is worse than having APRIORI workaround.

>
> Thanks,
> Sunil
> > On Mon, Jul 3, 2023 at 10:07 PM Sunil V L <sunilvl@ventanamicro.com>
> wrote:
> >
> > > On Wed, Jun 28, 2023 at 02:27:10PM -0700, Tuan Phan wrote:
> > > > On Wed, Jun 28, 2023 at 9:47 AM Sunil V L <sunilvl@ventanamicro.com>
> > > wrote:
> > > >
> > > > > On Fri, Jun 23, 2023 at 11:39:32AM -0700, Tuan Phan wrote:
> > > > > > Make sure VirtNorFlashDxe loaded before VariableRuntimeDxe as it
> > > > > > is the backend flash driver.
> > > > > >
> > > > > > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> > > > > > ---
> > > > > >  OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > >
> > > > > > diff --git a/OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf
> > > > > b/OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf
> > > > > > index 21e4ba67379f..9ab8eb3ba7d8 100644
> > > > > > --- a/OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf
> > > > > > +++ b/OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf
> > > > > > @@ -53,6 +53,16 @@ READ_STATUS        = TRUE
> > > > > >  READ_LOCK_CAP      = TRUE
> > > > > >  READ_LOCK_STATUS   = TRUE
> > > > > >
> > > > > > +APRIORI DXE {
> > > > > > +  INF  MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> > > > > > +  INF  MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
> > > > > > +  INF
> > > > >
> > >
> MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCodeRouterRuntimeDxe.inf
> > > > > > +  INF
> > > > >
> > >
> MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf
> > > > > > +  INF  EmbeddedPkg/Drivers/FdtClientDxe/FdtClientDxe.inf
> > > > > > +  INF  UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf
> > > > > > +  INF  OvmfPkg/VirtNorFlashDxe/VirtNorFlashDxe.inf
> > > > > > +}
> > > > > > +
> > > > > Hi Tuan,
> > > > >
> > > > > Actually, Ard had recommended not to use APRIORI and hence we
> avoided
> > > > > it when we upstreamed RiscVVirt. So, I am wondering whether this
> can be
> > > > > avoided by using depex in CpuDxe on gEfiVariableArchProtocolGuid?
> > > > >
> > > > > Hi Sunil,
> > > > Not sure what the reason behind avoiding APRIORI besides it is a
> > > workaround
> > > > for broken DEPEX. BTW, what we need is to put VirtNorFlashDxe loaded
> > > before
> > > > VariableRuntimeDxe which doesn't depend on any modules. I don't see
> any
> > > > other clearer way than modifying VirNorFlashDxe as shown in the first
> > > > version of this series.
> > > >
> > > > The CpuDxeRiscV64 in the aprioriy list as VirNorFlashDxe depends on
> it.
> > > >
> > > Hi Tuan,
> > >
> > > I couldn't locate old mail from Ard recommending to remove APRIORI in
> > > RISC-V. But here is the recent mail on different context but those
> > > reasons are still valid in any case.
> > > https://edk2.groups.io/g/devel/message/104543
> > >
> > > IMO, there is no dependency between VirtNorFlashDxe and
> > > VariableRuntimeDxe. I think what we need is CpuDxeRiscV64 loaded after
> > > VariableRuntimeDxe and before VirtNorFlashDxe. A simple depex like I
> > > suggested in previous mail should work. I still prefer this than
> > > introducing APRIORI unless there are other issues I am now aware of.
> > > What do you think?
> > >
> > > Thanks!
> > > Sunil
> > >
>

[-- Attachment #2: Type: text/html, Size: 5805 bytes --]