Re: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix volatile variable RT cache update logic

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

In the case a UEFI variable is deleted, the Attributes can be zero (Attributes are marked OPTIONAL in the API).
So in that case, the Attributes parameter alone does not accurately reflect the type of variable. The function
handles this fairly cryptically. The condition you specified is used in the function, for example on the following line:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c#L2024
but I believe that is only valid because the deletion case was handled earlier in the function followed by a
goto Done so this code would be skipped over in the case of a pre-existing variable which would have the
->Volatile field set properly because it would have been set depending upon which variable store buffer 
the variable was found in. At this point in the flow (after the Done label), I believe this logic is required to
generically determine the variable volatility status.

Thanks,
Michael

> -----Original Message-----
> From: Gao, Liming <liming.gao@intel.com>
> Sent: Monday, November 11, 2019 4:25 PM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> volatile variable RT cache update logic
> 
> Michael:
>   Can the logic be simplified to only check this condition 'Attributes &
> EFI_VARIABLE_NON_VOLATILE) != 0'?
> 
> Thanks
> Liming
> >-----Original Message-----
> >From: Kubacki, Michael A
> >Sent: Monday, November 11, 2019 2:11 PM
> >To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io
> >Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Wang, Jian J
> ><jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>
> >Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> >volatile variable RT cache update logic
> >
> >After a new volatile variable is written successfully, Runtime Services
> >GetVariable () may return EFI_NOT_FOUND for that variable.
> >
> >Thanks,
> >Michael
> >
> >> -----Original Message-----
> >> From: Gao, Liming <liming.gao@intel.com>
> >> Sent: Sunday, November 10, 2019 10:08 PM
> >> To: devel@edk2.groups.io; Kubacki, Michael A
> >> <michael.a.kubacki@intel.com>
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Wang, Jian J
> >> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Gao, Liming
> >> <liming.gao@intel.com>
> >> Subject: RE: [edk2-devel] [PATCH V1 1/1] MdeModulePkg/Variable: Fix
> >> volatile variable RT cache update logic
> >>
> >> Michael:
> >>   What real issue is caused by this issue?
> >>
> >> Thanks
> >> Liming
> >> >-----Original Message-----
> >> >From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> >> >Of Kubacki, Michael A
> >> >Sent: Monday, November 11, 2019 1:16 PM
> >> >To: devel@edk2.groups.io
> >> >Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> >> ><michael.d.kinney@intel.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
> >> >volatile variable RT cache update logic
> >> >
> >> >REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2333
> >> >
> >> >During a SetVariable () invocation, UpdateVariable () is called.
> >> >UpdateVariable () contains logic to determine whether a volatile or
> >> >non-volatile UEFI variable was set so the corresponding runtime
> >> >cache can be updated to reflect the change. The current logic simply
> >> >evaluates Variable->Volatile to determine which runtime cache should
> >> >be updated.
> >> >
> >> >The problem is Variable->Volatile does not always reflect whether a
> >> >volatile variable is being set. Variable->Volatile is set to TRUE
> >> >only in the case a pre-existing variable is found in the volatile
> >> >variable store. Therefore, the value is FALSE when a new volatile
> >> >variable is written.
> >> >
> >> >This change updates the logic to take this into account. If a new
> >> >variable is written successfully, the Attributes will accurately
> >> >reflect whether the variable is non-volatile. If a pre-existing
> >> >variable is modified, the Volatile field will reflect the type of
> >> >variable (Attributes are not reliable; e.g. 0x0 indicates deletion).
> >> >
> >> >Cc: Liming Gao <liming.gao@intel.com>
> >> >Cc: Michael D Kinney <michael.d.kinney@intel.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 | 5 ++---
> >> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >> >
> >> >diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> >> >b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> >> >index 29d6aca993..75d33ff724 100644
> >> >--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> >> >+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> >> >@@ -2296,9 +2296,8 @@ UpdateVariable (
> >> >
> >> > Done:
> >> >   if (!EFI_ERROR (Status)) {
> >> >-    if (Variable->Volatile) {
> >> >-      VolatileCacheInstance = &(mVariableModuleGlobal-
> >>
> >>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileC
> >> >>a
> >c
> >> h
> >> >e);
> >> >-    } else {
> >> >+    VolatileCacheInstance = &(mVariableModuleGlobal-
> >>
> >>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileC
> >> >>a
> >c
> >> h
> >> >e);
> >> >+    if ((Variable->CurrPtr != NULL && !Variable->Volatile) ||
> >> >+ (Attributes &
> >> >EFI_VARIABLE_NON_VOLATILE) != 0) {
> >> >       VolatileCacheInstance = &(mVariableModuleGlobal-
> >>
> >>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
> >> >     }
> >> >
> >> >--
> >> >2.16.2.windows.1
> >> >
> >> >
> >> >
> >>
> >
>