public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Kubacki, Michael A" <michael.a.kubacki@intel.com>
To: devel@edk2.groups.io
Cc: Liming Gao <liming.gao@intel.com>,
	Michael D Kinney <michael.d.kinney@intel.com>,
	Jian J Wang <jian.j.wang@intel.com>,
	Hao A Wu <hao.a.wu@intel.com>
Subject: [PATCH V2 1/1] MdeModulePkg/Variable: Fix volatile variable RT cache update logic
Date: Tue, 12 Nov 2019 15:40:38 -0800	[thread overview]
Message-ID: <20191112234038.23092-1-michael.a.kubacki@intel.com> (raw)

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


             reply	other threads:[~2019-11-12 23:40 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-12 23:40 Kubacki, Michael A [this message]
2019-11-13  0:08 ` [PATCH V2 1/1] MdeModulePkg/Variable: Fix volatile variable RT cache update logic Liming Gao
2019-11-13  8:41 ` [edk2-devel] " Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191112234038.23092-1-michael.a.kubacki@intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox