Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix VarErrorFlag RT cache offset calculation

["Wang, Jian J" <jian.j.wang@intel.com>]

Mike,

Thanks for explaining. You're right that the error is rare case and it won't
cause big problem, and NonVolatileLastVariableOffset will be approaching
the whole FV size after some time. I don't have strong opinion. Both work
for me.

Regards,
Jian

> -----Original Message-----
> From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> Sent: Wednesday, January 15, 2020 11:53 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Michael Turner
> <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> VarErrorFlag RT cache offset calculation
> 
> Hi Jian,
> 
> I considered that but these are the reasons I settled on the approach in patch V1.
> 
> 1. With the variable store filled, the length of mVariableModuleGlobal-
> >NonVolatileLastVariableOffset will only marginally be a smaller value than
> mNvVariableCache->Size (since variable writes grow the store for SPI flash wear
> leveling). In this case, it will be ~CommonRuntimeVariableSpace which is usually
> a major portion of the variable store size anyway.
> 2. Since mVariableModuleGlobal->NonVolatileLastVariableOffset is a global
> moving value that is more frequently manipulated than the fixed variable store
> size, depending upon it increases the likelihood it will be set to an invalid value
> somewhere else.
> 3. This is a relatively rare case (an error condition) and the memory copy is
> within DRAM for variable stores that are typically ~128KB - ~512KB.
> 
> To reduce the copy size, the Offset parameter can be "(UINTN) VarErrFlag -
> (UINTN) mNvVariableCache" (just remove the unnecessary addition of (UINTN)
> mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase) with size
> "sizeof (TempFlag)". How about this in a V2?
> 
> Thanks,
> Michael
> 
> > -----Original Message-----
> > From: Wang, Jian J <jian.j.wang@intel.com>
> > Sent: Monday, January 13, 2020 10:43 PM
> > To: devel@edk2.groups.io; Kubacki, Michael A
> > <michael.a.kubacki@intel.com>
> > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Michael Turner
> > <michael.turner@microsoft.com>; Wu, Hao A <hao.a.wu@intel.com>
> > Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > VarErrorFlag RT cache offset calculation
> >
> > Michael,
> >
> > I'm not sure sync-ing whole variable cache memory is an efficient operation.
> > What about using mVariableModuleGlobal->NonVolatileLastVariableOffset
> > as Length parameter?
> >
> >        Status =  SynchronizeRuntimeVariableCache (
> >                    &mVariableModuleGlobal-
> > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> >                    0,
> >                    mVariableModuleGlobal->NonVolatileLastVariableOffset
> >                    );
> >
> > Regards,
> > Jian
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > > Kubacki, Michael A
> > > Sent: Tuesday, January 14, 2020 7:19 AM
> > > To: devel@edk2.groups.io
> > > Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Michael Turner
> > > <michael.turner@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>;
> > > Wu, Hao A <hao.a.wu@intel.com>
> > > Subject: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> > > VarErrorFlag RT cache offset calculation
> > >
> > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2457
> > >
> > > This commit fixes an offset calculation that is used to write the
> > > VarErrorFlag UEFI variable to the UEFI variable runtime cache.
> > >
> > > Currently a physical address is used instead of an offset. This commit
> > > changes the offset to zero with a length of the entire non-volatile
> > > variable store so the entire non-volatile variable store buffer in
> > > SMRAM (with the variable update modification) is copied to the runtime
> > > variable cache. This follows the same pattern used in other
> > > SynchronizeRuntimeVariableCache () calls for consistency.
> > >
> > > * Observable symptom: An exception in SMM will most likely occur
> > >   due to the invalid memory reference when the VarErrorFlag variable
> > >   is written. The variable is most commonly written when the UEFI
> > >   variable store is full.
> > >
> > > * The issue only occurs when the variable runtime cache is enabled
> > >   by the following PCD being set to TRUE:
> > >   gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> > >
> > > Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
> > >
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Michael Turner <michael.turner@microsoft.com>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
> > > ---
> > >  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > index b0ee5e50d0..d23aea4bc7 100644
> > > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > > @@ -16,7 +16,7 @@
> > >    VariableServiceSetVariable() should also check authenticate data to
> > > avoid buffer overflow,
> > >    integer overflow. It should also check attribute to avoid authentication
> > bypass.
> > >
> > > -Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > > reserved.<BR>
> > > +Copyright (c) 2006 - 2020, Intel Corporation. All rights
> > > +reserved.<BR>
> > >  (C) Copyright 2015-2018 Hewlett Packard Enterprise Development LP<BR>
> > >  SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > > @@ -335,8 +335,8 @@ RecordVarErrorFlag (
> > >        *VarErrFlag = TempFlag;
> > >        Status =  SynchronizeRuntimeVariableCache (
> > >                    &mVariableModuleGlobal-
> > > >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache,
> > > -                  (UINTN) VarErrFlag - (UINTN) mNvVariableCache + (UINTN)
> > > mVariableModuleGlobal->VariableGlobal.NonVolatileVariableBase,
> > > -                  sizeof (TempFlag)
> > > +                  0,
> > > +                  mNvVariableCache->Size
> > >                    );
> > >        ASSERT_EFI_ERROR (Status);
> > >      }
> > > --
> > > 2.16.2.windows.1
> > >
> > >
> > > 
> >
>