Re: [edk2-devel] [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName() cache support

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 11404 bytes --]



> 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 <mailto:hao.a.wu@intel.com>>
>> Sent: Thursday, October 3, 2019 1:05 AM
>> To: Kubacki, Michael A <michael.a.kubacki@intel.com <mailto:michael.a.kubacki@intel.com>>;
>> devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>> Cc: Bi, Dandan <dandan.bi@intel.com <mailto:dandan.bi@intel.com>>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org <mailto:ard.biesheuvel@linaro.org>>; Dong, Eric <eric.dong@intel.com <mailto:eric.dong@intel.com>>; Laszlo Ersek
>> <lersek@redhat.com <mailto:lersek@redhat.com>>; Gao, Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>; Kinney, Michael
>> D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Wang, Jian J
>> <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com <mailto: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
>> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 26546 bytes --]