Thanks Andrew, I had not seen that before. From: 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 Cc: Wu, Hao A ; Bi, Dandan ; Ard Biesheuvel ; Dong, Eric ; Laszlo Ersek ; Gao, Liming ; Kinney, Michael D ; Ni, Ray ; Wang, Jian J ; Yao, Jiewen 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 > 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 > Sent: Thursday, October 3, 2019 1:05 AM To: Kubacki, Michael A >; devel@edk2.groups.io Cc: Bi, Dandan >; Ard Biesheuvel >; Dong, Eric >; Laszlo Ersek >; Gao, Liming >; Kinney, Michael D >; Ni, Ray >; Wang, Jian J >; Yao, Jiewen > 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 > Cc: Ard Biesheuvel > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Liming Gao > Cc: Michael D Kinney > Cc: Ray Ni > Cc: Jian J Wang > Cc: Hao A Wu > Cc: Jiewen Yao > Signed-off-by: Michael Kubacki > --- 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