Re: [edk2-devel] [PATCH V3 9/9] OvmfPkg: Disable variable runtime cache

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

You have a good point with the proposed order for enabling the PCD. I will make that
update in a V4 later today.

Thanks,
Michael

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Monday, October 14, 2019 11:50 AM
> To: devel@edk2.groups.io; Kubacki, Michael A
> <michael.a.kubacki@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming
> <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Wu,
> Hao A <hao.a.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH V3 9/9] OvmfPkg: Disable variable runtime
> cache
> 
> On 10/14/19 20:08, Kubacki, Michael A wrote:
> > Sets gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> > to FALSE in OvmfPkgIa32.dsc, OvmfPkgIa32X64.dsc, and OvmfPkgX64.dsc
> so
> > that when SMM_REQUIRE is TRUE, the SMM variable driver will not use
> > the runtime variable cache.
> >
> > This is done for OvmfPkg because it currently depends upon a SMM
> > variable GetVariable ()implementation as a simple method to exercise
> > the SMM driver stack. This allows the following commands to be used
> > for variables such as Boot####, BootOrder, and BootNext to test SMM
> > timing and stability differences on the BSP (e.g. CPU#0) vs an AP
> > (e.g. CPU#1).
> >  # taskset -c 0 efibootmgr
> >  # taskset -c 1 efibootmgr
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> > ---
> >  OvmfPkg/OvmfPkgIa32.dsc    | 1 +
> >  OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
> >  OvmfPkg/OvmfPkgX64.dsc     | 1 +
> >  3 files changed, 3 insertions(+)
> >
> > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index
> > 66e944436a..4861e2342b 100644
> > --- a/OvmfPkg/OvmfPkgIa32.dsc
> > +++ b/OvmfPkg/OvmfPkgIa32.dsc
> > @@ -428,6 +428,7 @@
> >  !if $(SMM_REQUIRE) == TRUE
> >    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> > +
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALS
> E
> >  !endif
> >
> >  [PcdsFixedAtBuild]
> > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc
> b/OvmfPkg/OvmfPkgIa32X64.dsc
> > index 51c2bfb44f..f316d97d6a 100644
> > --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> > @@ -433,6 +433,7 @@
> >  !if $(SMM_REQUIRE) == TRUE
> >    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> > +
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALS
> E
> >  !endif
> >
> >  [PcdsFixedAtBuild]
> > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index
> > ba7a758844..546649f0a2 100644
> > --- a/OvmfPkg/OvmfPkgX64.dsc
> > +++ b/OvmfPkg/OvmfPkgX64.dsc
> > @@ -433,6 +433,7 @@
> >  !if $(SMM_REQUIRE) == TRUE
> >    gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> > +
> gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache|FALS
> E
> >  !endif
> >
> >  [PcdsFixedAtBuild]
> >
> 
> For this patch:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> And many thanks for accommodating this use case!
> 
> However, I'd prefer the following order of changes:
> 
> - patch 7: introduce the PCD with default FALSE value
> - patch 9: set the PCD explicitly to FALSE in OvmfPkg
> - patch 10 (new patch): flip the PCD default value to TRUE
> 
> Because with v3, if someone were to bisect an issue, using OVMF, and the
> bisection landed them at patch 7 or at patch 8, they'd see a
> (transitory) change in variable caching behavior.
> 
> Thanks,
> Laszlo
> 
>