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

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

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: Kubacki, Michael A <michael.a.kubacki@intel.com>
> Sent: Wednesday, January 15, 2020 12:31 PM
> 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
> 
> Since I don't have a strong opinion either, I won't make any changes to V1 at this
> time.
> 
> Thanks,
> Michael
> 
> > -----Original Message-----
> > From: Wang, Jian J <jian.j.wang@intel.com>
> > Sent: Tuesday, January 14, 2020 8:09 PM
> > To: Kubacki, Michael A <michael.a.kubacki@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
> >
> > 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
> > > mNvVariableCache->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
> > > mVariableModuleGlobal->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
> > > > >
> > > > >
> > > > > 
> > > >
> > >
> >
>