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

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

[Kubacki, Michael A]

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.VariableRuntimeVolatileCache);
-    } else {
+    VolatileCacheInstance = &(mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache);
+    if ((Variable->CurrPtr != NULL && !Variable->Volatile) || (Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
       VolatileCacheInstance = &(mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
     }
 
-- 
2.16.2.windows.1

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

[Liming Gao]

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.VariableRuntimeVolatileCach
>e);
>-    } else {
>+    VolatileCacheInstance = &(mVariableModuleGlobal-
>>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCach
>e);
>+    if ((Variable->CurrPtr != NULL && !Variable->Volatile) || (Attributes &
>EFI_VARIABLE_NON_VOLATILE) != 0) {
>       VolatileCacheInstance = &(mVariableModuleGlobal-
>>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
>     }
>
>--
>2.16.2.windows.1
>
>
>

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

[Kubacki, Michael A]

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.VariableRuntimeVolatileCac
> h
> >e);
> >-    } else {
> >+    VolatileCacheInstance = &(mVariableModuleGlobal-
> >>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCac
> h
> >e);
> >+    if ((Variable->CurrPtr != NULL && !Variable->Volatile) ||
> >+ (Attributes &
> >EFI_VARIABLE_NON_VOLATILE) != 0) {
> >       VolatileCacheInstance = &(mVariableModuleGlobal-
> >>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
> >     }
> >
> >--
> >2.16.2.windows.1
> >
> >
> >
> 

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

[Liming Gao]

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.VariableRuntimeVolatileCa
>c
>> h
>> >e);
>> >-    } else {
>> >+    VolatileCacheInstance = &(mVariableModuleGlobal-
>> >>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCa
>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
>> >
>> >
>> >
>>
>

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

[Kubacki, Michael A]

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
> >> >
> >> >
> >> >
> >>
> >
> 

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

[Laszlo Ersek]

Hello Michael,

On 11/11/19 07:11, Kubacki, Michael A wrote:
> After a new volatile variable is written successfully, Runtime Services GetVariable () may return
> EFI_NOT_FOUND for that variable.

I've read <https://bugzilla.tianocore.org/show_bug.cgi?id=2333>, up to
comment #1.

Can you please update the commit message, with the following information:

- the symptom that you describe above (in response to Liming's
  question),

- whether this issue is a regression introduced by the earlier work for
  TianoCore#2220,

  - if so, then: whether the issue affects a platform even if the
    platform sets "PcdEnableVariableRuntimeCache" to false

- alternatively, whether this issue is a preexistent one, which has been
  *exposed* only, by the work for TianoCore#2220,

- in case this is a regression genuinely introduced by the work for
  TianoCore#2220, can you please add a "Fixes: <commit hash>" line near
  the end of the commit message, to identify the specific commit that
  introduced the regression?

Regarding the Bugzilla ticket (TianoCore#2333): if the issue is a
regression, then:

- please set the "regression" keyword, in the Keywords field,

- furthermore, please add "2220" to the See Also field.

Thanks!
Laszlo


>> -----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.VariableRuntimeVolatileCac
>> h
>>> e);
>>> -    } else {
>>> +    VolatileCacheInstance = &(mVariableModuleGlobal-
>>>> VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCac
>> h
>>> e);
>>> +    if ((Variable->CurrPtr != NULL && !Variable->Volatile) ||
>>> + (Attributes &
>>> EFI_VARIABLE_NON_VOLATILE) != 0) {
>>>       VolatileCacheInstance = &(mVariableModuleGlobal-
>>>> VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
>>>     }
>>>
>>> --
>>> 2.16.2.windows.1
>>>
>>>
>>>
>>
> 
> 
> 
> 

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

[Wang, Jian J]

I reproduced this issue in secure boot. The patch fixed the problem.

One minor issue in the patch: you changed the code from if...else... to ...if...,
which confused me a little bit. I thought you wanted to use 
'VolatileCacheInstance' in the 'if' expression but you didn't. So I suggest to
keep original style, i.e. put the first fetch of 'VolatileCacheInstance' in the 'else'
statement (it'd be better to go to the if statement to keep the same order as
original code, if it won't cause too complex if expression). From the reviewer
perspective, this way make it clearer to know you just want to update the
condition to determine the non-volatile variable update.

With this addressed (no v2 needed),

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

Regards,
Jian

> -----Original Message-----
> From: devel@edk2.groups.io <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.VariableRuntimeVolatileCache);
> -    } else {
> +    VolatileCacheInstance = &(mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache);
> +    if ((Variable->CurrPtr != NULL && !Variable->Volatile) || (Attributes &
> EFI_VARIABLE_NON_VOLATILE) != 0) {
>        VolatileCacheInstance = &(mVariableModuleGlobal-
> >VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
>      }
> 
> --
> 2.16.2.windows.1
> 
> 
>