From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f54.google.com (mail-wr1-f54.google.com [209.85.221.54]) by mx.groups.io with SMTP id smtpd.web10.1382.1689275295518264546 for ; Thu, 13 Jul 2023 12:08:15 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@ventanamicro.com header.s=google header.b=aMLHi9IY; spf=pass (domain: ventanamicro.com, ip: 209.85.221.54, mailfrom: tphan@ventanamicro.com) Received: by mail-wr1-f54.google.com with SMTP id ffacd0b85a97d-3094910b150so1227734f8f.0 for ; Thu, 13 Jul 2023 12:08:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ventanamicro.com; s=google; t=1689275294; x=1691867294; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=t2+IITgDwDiRyddlJcjtGBgqCE+BRqNa1h+mCQDCWZU=; b=aMLHi9IYLx3YY1SAWNRdBHZxN60dtNz41n51pZsc3TvWTWPirwOk8PTMh2+u541+fK pHGfT1DXb5wni8VN3+x6TWYavHkPmhpWWnSVeQO8nAZDvG7hZg871bKABA7kIcdUtpr7 SxFIE8VYvNZtiN2nl8OFBKbxHvl28a2dQCVgGpbTHIINqVYldiIhFbY0dqS3Duc90eT2 bQyfy1FO5txIoBftlomt/FYzIowv1wEc3a1b8D7wL2BkBygKKSpMGFcXbwUt7pXzckUe lsCHdR2mfHEuD3PrSL4/YQjGZapIxIJnUPhUeEkE0yEc6vFcUiUZJkFOxtVZ2DfPJp68 RWyw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689275294; x=1691867294; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=t2+IITgDwDiRyddlJcjtGBgqCE+BRqNa1h+mCQDCWZU=; b=Tfo05Cq0eSrsua7ckp/Vkoea8EC54JkVi7eqpEXMIkXqFvkc19QRdBRKNYKidYaqTE tRxA0AACcOXvey/80XaYRq0MfPSX/HOJobzA/Fw0uW5s29WzhKXYh4dA93r7Ebw/dSoX qWOi64aZWZPWXstEvXv+HgcdV6eC6cs4KQMRL+MXrwEFJkR8E+H2MTqjY16El7kMt5JG cFaeIWLqt6NOq50zdpB1k9WnnV6n6qV9TxJ2Y7Jz4i8m609teVBVfKB8tLBT6d6/DKY4 i9jJXZnprU9GCxXuv4z602PuA+erggbX/VZZzkkxUlMNSNNsgBGmGG0KYxPU9JVIUWry AByQ== X-Gm-Message-State: ABy/qLb3gJsWT7YA8xHCmYlLYA3Mek66r4NQ7ypzUg5Z6wD4b3m6Ykc9 Gft0Me09kahoPfsdQxm66WDLrVgDzov7n7Dal8dpiQ== X-Google-Smtp-Source: APBJJlHwuAsuhRHnzZ7laSYvszBhJ92PDLfxbudryc7+mUXR++hnrscQIF7xPRwa1AsfZnqaCAgOUxxQ6S9qeQfSFac= X-Received: by 2002:adf:e4c4:0:b0:314:14ea:e2de with SMTP id v4-20020adfe4c4000000b0031414eae2demr2398819wrm.0.1689275293896; Thu, 13 Jul 2023 12:08:13 -0700 (PDT) MIME-Version: 1.0 References: <20230623183934.23905-1-tphan@ventanamicro.com> <20230623183934.23905-6-tphan@ventanamicro.com> In-Reply-To: From: "Tuan Phan" Date: Thu, 13 Jul 2023 12:08:02 -0700 Message-ID: Subject: Re: [PATCH v4 5/7] OvmfPkg/RiscVVirt: Add VirtNorFlashDxe to APRIORI list To: Sunil V L Cc: andrei.warkentin@intel.com, ardb+tianocore@kernel.org, devel@edk2.groups.io, gaoliming@byosoft.com.cn, git@danielschaefer.me, michael.d.kinney@intel.com, zhiguang.liu@intel.com Content-Type: multipart/alternative; boundary="000000000000d22f1d0600630d5b" --000000000000d22f1d0600630d5b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Jul 4, 2023 at 12:01=E2=80=AFAM Sunil V L wrote: > On Mon, Jul 03, 2023 at 11:45:45PM -0700, Tuan Phan wrote: > > As i said, VirtNorFlashDxe needed to be loaded before VariableRuntimeDx= e > 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 > wrote: > > > > > On Wed, Jun 28, 2023 at 02:27:10PM -0700, Tuan Phan wrote: > > > > On Wed, Jun 28, 2023 at 9:47=E2=80=AFAM Sunil V L > > > wrote: > > > > > > > > > On Fri, Jun 23, 2023 at 11:39:32AM -0700, Tuan Phan wrote: > > > > > > Make sure VirtNorFlashDxe loaded before VariableRuntimeDxe as i= t > > > > > > is the backend flash driver. > > > > > > > > > > > > Signed-off-by: Tuan Phan > > > > > > --- > > > > > > 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 =3D TRUE > > > > > > READ_LOCK_CAP =3D TRUE > > > > > > READ_LOCK_STATUS =3D TRUE > > > > > > > > > > > > +APRIORI DXE { > > > > > > + INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf > > > > > > + INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf > > > > > > + INF > > > > > > > > > MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportStatusCode= RouterRuntimeDxe.inf > > > > > > + INF > > > > > > > > > MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRunt= imeDxe.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 loade= d > > > before > > > > VariableRuntimeDxe which doesn't depend on any modules. I don't see > any > > > > other clearer way than modifying VirNorFlashDxe as shown in the fir= st > > > > 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 afte= r > > > 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 > > > > --000000000000d22f1d0600630d5b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Jul 4, 2023 at 12:01=E2=80=AF= AM Sunil V L <sunilvl@ventan= amicro.com> wrote:
On Mon, Jul 03, 2023 at 11:45:45PM -0700, Tuan Phan wrote:
> As i said, VirtNorFlashDxe needed to be loaded before VariableRuntimeD= xe 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.
<= /blockquote>
It doesn't work as your workaround trying to make CpuD= xe depends on variable protocol which has nothing to do with it. Also, CpuD= xe is an essential module and should not depend on anything, what happens i= f the variable driver before generating the protocol tries to use CPU proto= col?
It is worse than having APRIORI workaround.=C2=A0

Thanks,
Sunil
> On Mon, Jul 3, 2023 at 10:07 PM Sunil V L <sunilvl@ventanamicro.com> wrot= e:
>
> > On Wed, Jun 28, 2023 at 02:27:10PM -0700, Tuan Phan wrote:
> > > On Wed, Jun 28, 2023 at 9:47=E2=80=AFAM Sunil V L <sunilvl@ventanamicr= o.com>
> > wrote:
> > >
> > > > On Fri, Jun 23, 2023 at 11:39:32AM -0700, Tuan Phan wro= te:
> > > > > Make sure VirtNorFlashDxe loaded before VariableRu= ntimeDxe as it
> > > > > is the backend flash driver.
> > > > >
> > > > > Signed-off-by: Tuan Phan <tphan@ventanamicro.com>
> > > > > ---
> > > > >=C2=A0 OvmfPkg/RiscVVirt/RiscVVirtQemu.fdf | 10 +++= +++++++
> > > > >=C2=A0 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=C2=A0 =C2=A0 =C2=A0= =C2=A0 =3D TRUE
> > > > >=C2=A0 READ_LOCK_CAP=C2=A0 =C2=A0 =C2=A0 =3D TRUE > > > > >=C2=A0 READ_LOCK_STATUS=C2=A0 =C2=A0=3D TRUE
> > > > >
> > > > > +APRIORI DXE {
> > > > > +=C2=A0 INF=C2=A0 MdeModulePkg/Universal/DevicePat= hDxe/DevicePathDxe.inf
> > > > > +=C2=A0 INF=C2=A0 MdeModulePkg/Universal/PCD/Dxe/P= cd.inf
> > > > > +=C2=A0 INF
> > > >
> > MdeModulePkg/Universal/ReportStatusCodeRouter/RuntimeDxe/ReportSt= atusCodeRouterRuntimeDxe.inf
> > > > > +=C2=A0 INF
> > > >
> > MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHan= dlerRuntimeDxe.inf
> > > > > +=C2=A0 INF=C2=A0 EmbeddedPkg/Drivers/FdtClientDxe= /FdtClientDxe.inf
> > > > > +=C2=A0 INF=C2=A0 UefiCpuPkg/CpuDxeRiscV64/CpuDxeR= iscV64.inf
> > > > > +=C2=A0 INF=C2=A0 OvmfPkg/VirtNorFlashDxe/VirtNorF= lashDxe.inf
> > > > > +}
> > > > > +
> > > > Hi Tuan,
> > > >
> > > > Actually, Ard had recommended not to use APRIORI and he= nce we avoided
> > > > it when we upstreamed RiscVVirt. So, I am wondering whe= ther this can be
> > > > avoided by using depex in CpuDxe on gEfiVariableArchPro= tocolGuid?
> > > >
> > > > 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 VirtNorFlashDx= e 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 dep= ends on it.
> > >
> > Hi Tuan,
> >
> > I couldn't locate old mail from Ard recommending to remove AP= RIORI in
> > RISC-V. But here is the recent mail on different context but thos= e
> > 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 lik= e I
> > suggested in previous mail should work. I still prefer this than<= br> > > introducing APRIORI unless there are other issues I am now aware = of.
> > What do you think?
> >
> > Thanks!
> > Sunil
> >
--000000000000d22f1d0600630d5b--