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

[PATCH V2 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).

* Observable symptom: A volatile variable that was set successfully
  might return EFI_NOT_FOUND when the variable should be found.

* The issue is a regression introduced to the variable services only
  when the variable runtime cache is enabled by the following PCD
  being set to TRUE:
  gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache

* The issue was implemented in commit aab3b9b9a1 but the PCD was not
  set to TRUE by default enabling the issue until commit e07b7d024a.

Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706

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

Notes:
    V2 changes:
    * Sets the runtime cache instance pointer that was updated in V1
      using an if/else statement instead of just an if statement so
      it better aligns to the original code style.
    * Updates the commit message to include the simple symptom most
      commonly observed that this change fixes, an explicit description
      that this change is a regression to the variable services, that
      it only is present when the runtime variable cache is enabled, and
      relevant commit details.

 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 29d6aca993..b0ee5e50d0 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
@@ -2296,10 +2296,10 @@ UpdateVariable (
 
 Done:
   if (!EFI_ERROR (Status)) {
-    if (Variable->Volatile) {
-      VolatileCacheInstance = &(mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache);
-    } else {
+    if ((Variable->CurrPtr != NULL && !Variable->Volatile) || (Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
       VolatileCacheInstance = &(mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
+    } else {
+      VolatileCacheInstance = &(mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache);
     }
 
     if (VolatileCacheInstance->Store != NULL) {
-- 
2.16.2.windows.1

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

[Liming Gao]

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: Kubacki, Michael A
>Sent: Wednesday, November 13, 2019 7:41 AM
>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: [PATCH V2 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).
>
>* Observable symptom: A volatile variable that was set successfully
>  might return EFI_NOT_FOUND when the variable should be found.
>
>* The issue is a regression introduced to the variable services only
>  when the variable runtime cache is enabled by the following PCD
>  being set to TRUE:
>  gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
>
>* The issue was implemented in commit aab3b9b9a1 but the PCD was not
>  set to TRUE by default enabling the issue until commit e07b7d024a.
>
>Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
>
>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>
>---
>
>Notes:
>    V2 changes:
>    * Sets the runtime cache instance pointer that was updated in V1
>      using an if/else statement instead of just an if statement so
>      it better aligns to the original code style.
>    * Updates the commit message to include the simple symptom most
>      commonly observed that this change fixes, an explicit description
>      that this change is a regression to the variable services, that
>      it only is present when the runtime variable cache is enabled, and
>      relevant commit details.
>
> 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 29d6aca993..b0ee5e50d0 100644
>--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
>+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
>@@ -2296,10 +2296,10 @@ UpdateVariable (
>
> Done:
>   if (!EFI_ERROR (Status)) {
>-    if (Variable->Volatile) {
>-      VolatileCacheInstance = &(mVariableModuleGlobal-
>>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCach
>e);
>-    } else {
>+    if ((Variable->CurrPtr != NULL && !Variable->Volatile) || (Attributes &
>EFI_VARIABLE_NON_VOLATILE) != 0) {
>       VolatileCacheInstance = &(mVariableModuleGlobal-
>>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
>+    } else {
>+      VolatileCacheInstance = &(mVariableModuleGlobal-
>>VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCach
>e);
>     }
>
>     if (VolatileCacheInstance->Store != NULL) {
>--
>2.16.2.windows.1

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

[Laszlo Ersek]

On 11/13/19 00:40, Kubacki, Michael A wrote:
> 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).
> 
> * Observable symptom: A volatile variable that was set successfully
>   might return EFI_NOT_FOUND when the variable should be found.
> 
> * The issue is a regression introduced to the variable services only
>   when the variable runtime cache is enabled by the following PCD
>   being set to TRUE:
>   gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> 
> * The issue was implemented in commit aab3b9b9a1 but the PCD was not
>   set to TRUE by default enabling the issue until commit e07b7d024a.
> 
> Fixes: aab3b9b9a1e5e1f3fa966fb1667fc3e6c47e7706
> 
> 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>
> ---
> 
> Notes:
>     V2 changes:
>     * Sets the runtime cache instance pointer that was updated in V1
>       using an if/else statement instead of just an if statement so
>       it better aligns to the original code style.
>     * Updates the commit message to include the simple symptom most
>       commonly observed that this change fixes, an explicit description
>       that this change is a regression to the variable services, that
>       it only is present when the runtime variable cache is enabled, and
>       relevant commit details.

Splendid, thank you!

Acked-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

> 
>  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 29d6aca993..b0ee5e50d0 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -2296,10 +2296,10 @@ UpdateVariable (
>  
>  Done:
>    if (!EFI_ERROR (Status)) {
> -    if (Variable->Volatile) {
> -      VolatileCacheInstance = &(mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache);
> -    } else {
> +    if ((Variable->CurrPtr != NULL && !Variable->Volatile) || (Attributes & EFI_VARIABLE_NON_VOLATILE) != 0) {
>        VolatileCacheInstance = &(mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeNvCache);
> +    } else {
> +      VolatileCacheInstance = &(mVariableModuleGlobal->VariableGlobal.VariableRuntimeCacheContext.VariableRuntimeVolatileCache);
>      }
>  
>      if (VolatileCacheInstance->Store != NULL) {
>