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

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

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

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
>