Thanks Andrew, I had not seen that before.

 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Andrew Fish via Groups.Io
Sent: Thursday, October 3, 2019 12:00 PM
To: devel@edk2.groups.io; Kubacki, Michael A <michael.a.kubacki@intel.com>
Cc: Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName() cache support

 

 

 

On Oct 3, 2019, at 1:52 PM, Kubacki, Michael A <michael.a.kubacki@intel.com> wrote:

 

-Done:
+  mVariableRuntimeCacheReadLock = FALSE;



Similar to the previous patch (7/9),
if timeout occurs when acquiring the read lock, should this flag be set to
FALSE in such case?


Given that the runtime service can be invoked in a multi-threaded OS environment,
it is possible that one thread could be stuck with the lock while another thread times
out waiting to acquire the lock. In that case, I believe the global lock should not be
released. I can move setting the flag to FALSE within the same conditional block in
which it is set.

 

UEFI Spec sets the rules is 8.1 ...

 

All callers of Runtime Services are restricted from calling the same or certain other Runtime Service functions prior to the completion and return of a previous Runtime Service call. These restrictions apply to:

• Runtime Services that have been interrupted

• Runtime Services that are active on another processor.

Callers are prohibited from using certain other services from another processor or on the same processor following an interrupt as specified in Table 35. For this table ‘Busy’ is defined as the state when a Runtime Service has been entered and has not returned to the caller.

The consequence of a caller violating these restrictions is undefined except for certain special cases described below.

 

Table 35 variable info:

If previous call is busy in:

GetVariable() GetNextVariableName() SetVariable() QueryVariableInfo() UpdateCapsule() QueryCapsuleCapabilities() GetNextHighMonotonicCount()

 

Forbidden to call:

GetVariable(), GetNextVariableName(), SetVariable(), QueryVariableInfo(), UpdateCapsule(), QueryCapsuleCapabilities(), GetNextHighMonotonicCount()

 

Thanks,

 

Andrew Fish

 

Thanks,
Michael

-----Original Message-----
From: Wu, Hao A <hao.a.wu@intel.com>
Sent: Thursday, October 3, 2019 1:05 AM
To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
devel@edk2.groups.io
Cc: Bi, Dandan <dandan.bi@intel.com>; Ard Biesheuvel
<ard.biesheuvel@linaro.org>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
<lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael
D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J
<jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH V2 8/9] MdeModulePkg/Variable: Add RT
GetNextVariableName() cache support

-----Original Message-----
From: Kubacki, Michael A
Sent: Saturday, September 28, 2019 9:47 AM
To: devel@edk2.groups.io
Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming;
Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
Subject: [PATCH V2 8/9] MdeModulePkg/Variable: Add RT
GetNextVariableName() cache support

https://bugzilla.tianocore.org/show_bug.cgi?id=2220

This change implements the Runtime Service GetNextVariableName()

using

the Runtime Cache in VariableSmmRuntimeDxe. Runtime Service calls to
GetNextVariableName() will no longer trigger a SW SMI.

Overall system performance and stability will be improved by
eliminating an SMI for these calls as they typically result in a
relatively large number of invocations to retrieve all variable names
in all variable stores present.

Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Michael Kubacki <michael.a.kubacki@intel.com>
---

MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe

.c | 118 +++++++++-----------
1 file changed, 50 insertions(+), 68 deletions(-)

diff --git

a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD

xe.c

b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD

xe.c
index 46f69765a4..bc3b56b0ce 100644
---

a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD

xe.c
+++

b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeD

xe.c
@@ -799,87 +799,69 @@ RuntimeServiceGetNextVariableName (
  IN OUT  EFI_GUID                          *VendorGuid
  )
{
-  EFI_STATUS                                      Status;
-  UINTN                                           PayloadSize;
-  SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME
*SmmGetNextVariableName;
-  UINTN                                           OutVariableNameSize;
-  UINTN                                           InVariableNameSize;
+  EFI_STATUS              Status;
+  UINTN                   DelayIndex;
+  UINTN                   MaxLen;
+  UINTN                   VarNameSize;
+  VARIABLE_HEADER         *VariablePtr;
+  VARIABLE_STORE_HEADER
*VariableStoreHeader[VariableStoreTypeMax];
+
+  Status = EFI_NOT_FOUND;

  if (VariableNameSize == NULL || VariableName == NULL || VendorGuid
==
NULL) {
    return EFI_INVALID_PARAMETER;
  }

-  OutVariableNameSize   = *VariableNameSize;
-  InVariableNameSize    = StrSize (VariableName);
-  SmmGetNextVariableName = NULL;
-
  //
-  // If input string exceeds SMM payload limit. Return failure
+  // Calculate the possible maximum length of name string, including
+ the
Null terminator.
  //
-  if (InVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF
(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name))

{

+  MaxLen = *VariableNameSize / sizeof (CHAR16);  if ((MaxLen == 0) ||
+ (StrnLenS (VariableName, MaxLen) == MaxLen)) {
+    //
+    // Null-terminator is not found in the first VariableNameSize
+ bytes of the
input VariableName buffer,
+    // follow spec to return EFI_INVALID_PARAMETER.
+    //
    return EFI_INVALID_PARAMETER;
  }

-  AcquireLockOnlyAtBootTime(&mVariableServicesLock);
+  AcquireLockOnlyAtBootTime (&mVariableServicesLock);

-  //
-  // Init the communicate buffer. The buffer data size is:
-  // SMM_COMMUNICATE_HEADER_SIZE +
SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.
-  //
-  if (OutVariableNameSize > mVariableBufferPayloadSize - OFFSET_OF
(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name))

{

-    //
-    // If output buffer exceed SMM payload limit. Trim output buffer to

SMM

payload size
-    //
-    OutVariableNameSize = mVariableBufferPayloadSize - OFFSET_OF
(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name);
+  for (DelayIndex = 0; mVariableRuntimeCacheReadLock && DelayIndex <
VARIABLE_RT_CACHE_READ_LOCK_TIMEOUT; DelayIndex++) {
+    MicroSecondDelay (10);
  }
-  //
-  // Payload should be Guid + NameSize + MAX of Input & Output buffer
-  //
-  PayloadSize = OFFSET_OF
(SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME, Name)

+ MAX

(OutVariableNameSize, InVariableNameSize);
-
-  Status = InitCommunicateBuffer ((VOID

**)&SmmGetNextVariableName,

PayloadSize, SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME);
-  if (EFI_ERROR (Status)) {
-    goto Done;
-  }
-  ASSERT (SmmGetNextVariableName != NULL);
-
-  //
-  // SMM comm buffer->NameSize is buffer size for return string
-  //
-  SmmGetNextVariableName->NameSize = OutVariableNameSize;
-
-  CopyGuid (&SmmGetNextVariableName->Guid, VendorGuid);
-  //
-  // Copy whole string
-  //
-  CopyMem (SmmGetNextVariableName->Name, VariableName,
InVariableNameSize);
-  if (OutVariableNameSize > InVariableNameSize) {
-    ZeroMem ((UINT8 *) SmmGetNextVariableName->Name +
InVariableNameSize, OutVariableNameSize - InVariableNameSize);
-  }
-
-  //
-  // Send data to SMM
-  //
-  Status = SendCommunicateBuffer (PayloadSize);
-
-  //
-  // Get data from SMM.
-  //
-  if (Status == EFI_SUCCESS || Status == EFI_BUFFER_TOO_SMALL) {
-    //
-    // SMM CommBuffer NameSize can be a trimed value
-    // Only update VariableNameSize when needed
-    //
-    *VariableNameSize = SmmGetNextVariableName->NameSize;
-  }
-  if (EFI_ERROR (Status)) {
-    goto Done;
+  if (DelayIndex < VARIABLE_RT_CACHE_READ_LOCK_TIMEOUT) {
+    ASSERT (!mVariableRuntimeCacheReadLock);
+
+    CheckForRuntimeCacheSync ();
+    mVariableRuntimeCacheReadLock = TRUE;
+
+    if (!mVariableRuntimeCachePendingUpdate) {
+      //
+      // 0: Volatile, 1: HOB, 2: Non-Volatile.
+      // The index and attributes mapping must be kept in this order
+ as
FindVariable
+      // makes use of this mapping to implement search algorithm.
+      //
+      VariableStoreHeader[VariableStoreTypeVolatile] =
mVariableRuntimeVolatileCacheBuffer;
+      VariableStoreHeader[VariableStoreTypeHob]      =
mVariableRuntimeHobCacheBuffer;
+      VariableStoreHeader[VariableStoreTypeNv]       =
mVariableRuntimeNvCacheBuffer;
+
+      Status = GetNextVariableEx (VariableName, VendorGuid,
VariableStoreHeader, &VariablePtr);
+      if (!EFI_ERROR (Status)) {
+        VarNameSize = NameSizeOfVariable (VariablePtr);
+        ASSERT (VarNameSize != 0);
+        if (VarNameSize <= *VariableNameSize) {
+          CopyMem (VariableName, GetVariableNamePtr (VariablePtr),
VarNameSize);
+          CopyMem (VendorGuid, GetVendorGuidPtr (VariablePtr), sizeof
(EFI_GUID));
+          Status = EFI_SUCCESS;
+        } else {
+          Status = EFI_BUFFER_TOO_SMALL;
+        }
+
+        *VariableNameSize = VarNameSize;
+      }
+    }
  }
-
-  CopyGuid (VendorGuid, &SmmGetNextVariableName->Guid);
-  CopyMem (VariableName, SmmGetNextVariableName->Name,
SmmGetNextVariableName->NameSize);
-
-Done:
+  mVariableRuntimeCacheReadLock = FALSE;



Similar to the previous patch (7/9),
if timeout occurs when acquiring the read lock, should this flag be set to
FALSE in such case?

Best Regards,
Hao Wu


  ReleaseLockOnlyAtBootTime (&mVariableServicesLock);
  return Status;
}
--
2.16.2.windows.1