From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mx.groups.io with SMTP id smtpd.web10.2846.1571212625692096865 for ; Wed, 16 Oct 2019 00:57:05 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Oct 2019 00:57:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,303,1566889200"; d="scan'208";a="208359724" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga001.fm.intel.com with ESMTP; 16 Oct 2019 00:57:00 -0700 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 16 Oct 2019 00:56:59 -0700 Received: from shsmsx106.ccr.corp.intel.com (10.239.4.159) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 16 Oct 2019 00:56:59 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.166]) by SHSMSX106.ccr.corp.intel.com ([169.254.10.119]) with mapi id 14.03.0439.000; Wed, 16 Oct 2019 15:56:57 +0800 From: "Wu, Hao A" 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 V4 08/10] MdeModulePkg/Variable: Add RT GetNextVariableName() cache support Thread-Topic: [PATCH V4 08/10] MdeModulePkg/Variable: Add RT GetNextVariableName() cache support Thread-Index: AQHVgudlIyxsUU2gR0mOL8w0g3aLBqdc5g8g Date: Wed, 16 Oct 2019 07:56:56 +0000 Message-ID: References: <20191014233001.33024-1-michael.a.kubacki@intel.com> <20191014233001.33024-9-michael.a.kubacki@intel.com> In-Reply-To: <20191014233001.33024-9-michael.a.kubacki@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Kubacki, Michael A > Sent: Tuesday, October 15, 2019 7:30 AM > To: devel@edk2.groups.io > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming; Ki= nney, > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen > Subject: [PATCH V4 08/10] MdeModulePkg/Variable: Add RT > GetNextVariableName() cache support >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D2220 >=20 > 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 > when gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache > is set to TRUE (default value). I think from the perspective of this patch, the default value for the featu= re PCD is still FALSE, and the last patch of the series will actually switch the default value to TRUE, right? If my take is correct, I think we can drop the mention of "(default value)" here. Other than this, the patch looks good to me: Reviewed-by: Hao A Wu Best Regards, Hao Wu >=20 > 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. >=20 > 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 > --- >=20 > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe. > c | 137 ++++++++++++++++++-- > 1 file changed, 128 insertions(+), 9 deletions(-) >=20 > diff --git > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > e.c > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > e.c > index e236ddff33..a795b9fcab 100644 > --- > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > e.c > +++ > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx > e.c > @@ -823,7 +823,7 @@ RuntimeServiceGetVariable ( > } >=20 > /** > - This code Finds the Next available variable. > + Finds the next available variable in a runtime cache variable store. >=20 > @param[in, out] VariableNameSize Size of the variable name. > @param[in, out] VariableName Pointer to variable name. > @@ -836,8 +836,81 @@ RuntimeServiceGetVariable ( >=20 > **/ > EFI_STATUS > -EFIAPI > -RuntimeServiceGetNextVariableName ( > +GetNextVariableNameInRuntimeCache ( > + IN OUT UINTN *VariableNameSize, > + IN OUT CHAR16 *VariableName, > + IN OUT EFI_GUID *VendorGuid > + ) > +{ > + EFI_STATUS Status; > + UINTN VarNameSize; > + VARIABLE_HEADER *VariablePtr; > + VARIABLE_STORE_HEADER > *VariableStoreHeader[VariableStoreTypeMax]; > + > + Status =3D EFI_NOT_FOUND; > + > + // > + // The UEFI specification restricts Runtime Services callers from invo= king > the same or certain other Runtime Service > + // functions prior to completion and return from a previous Runtime > Service call. These restrictions prevent > + // a GetVariable () or GetNextVariable () call from being issued until= a prior > call has returned. The runtime > + // cache read lock should always be free when entering this function. > + // > + ASSERT (!mVariableRuntimeCacheReadLock); > + > + CheckForRuntimeCacheSync (); > + > + mVariableRuntimeCacheReadLock =3D 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] =3D > mVariableRuntimeVolatileCacheBuffer; > + VariableStoreHeader[VariableStoreTypeHob] =3D > mVariableRuntimeHobCacheBuffer; > + VariableStoreHeader[VariableStoreTypeNv] =3D > mVariableRuntimeNvCacheBuffer; > + > + Status =3D VariableServiceGetNextVariableInternal ( > + VariableName, > + VendorGuid, > + VariableStoreHeader, > + &VariablePtr, > + mVariableAuthFormat > + ); > + if (!EFI_ERROR (Status)) { > + VarNameSize =3D NameSizeOfVariable (VariablePtr, > mVariableAuthFormat); > + ASSERT (VarNameSize !=3D 0); > + if (VarNameSize <=3D *VariableNameSize) { > + CopyMem (VariableName, GetVariableNamePtr (VariablePtr, > mVariableAuthFormat), VarNameSize); > + CopyMem (VendorGuid, GetVendorGuidPtr (VariablePtr, > mVariableAuthFormat), sizeof (EFI_GUID)); > + Status =3D EFI_SUCCESS; > + } else { > + Status =3D EFI_BUFFER_TOO_SMALL; > + } > + > + *VariableNameSize =3D VarNameSize; > + } > + } > + mVariableRuntimeCacheReadLock =3D FALSE; > + > + return Status; > +} > + > +/** > + Finds the next available variable in a SMM variable store. > + > + @param[in, out] VariableNameSize Size of the variable name. > + @param[in, out] VariableName Pointer to variable name. > + @param[in, out] VendorGuid Variable Vendor Guid. > + > + @retval EFI_INVALID_PARAMETER Invalid parameter. > + @retval EFI_SUCCESS Find the specified variable. > + @retval EFI_NOT_FOUND Not found. > + @retval EFI_BUFFER_TO_SMALL DataSize is too small for the resul= t. > + > +**/ > +EFI_STATUS > +GetNextVariableNameInSmm ( > IN OUT UINTN *VariableNameSize, > IN OUT CHAR16 *VariableName, > IN OUT EFI_GUID *VendorGuid > @@ -849,10 +922,6 @@ RuntimeServiceGetNextVariableName ( > UINTN OutVariableNameSize; > UINTN InVariableNameSize; >=20 > - if (VariableNameSize =3D=3D NULL || VariableName =3D=3D NULL || Vendor= Guid =3D=3D > NULL) { > - return EFI_INVALID_PARAMETER; > - } > - > OutVariableNameSize =3D *VariableNameSize; > InVariableNameSize =3D StrSize (VariableName); > SmmGetNextVariableName =3D NULL; > @@ -864,8 +933,6 @@ RuntimeServiceGetNextVariableName ( > return EFI_INVALID_PARAMETER; > } >=20 > - AcquireLockOnlyAtBootTime(&mVariableServicesLock); > - > // > // Init the communicate buffer. The buffer data size is: > // SMM_COMMUNICATE_HEADER_SIZE + > SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize. > @@ -924,7 +991,59 @@ RuntimeServiceGetNextVariableName ( > CopyMem (VariableName, SmmGetNextVariableName->Name, > SmmGetNextVariableName->NameSize); >=20 > Done: > + return Status; > +} > + > +/** > + This code Finds the Next available variable. > + > + @param[in, out] VariableNameSize Size of the variable name. > + @param[in, out] VariableName Pointer to variable name. > + @param[in, out] VendorGuid Variable Vendor Guid. > + > + @retval EFI_INVALID_PARAMETER Invalid parameter. > + @retval EFI_SUCCESS Find the specified variable. > + @retval EFI_NOT_FOUND Not found. > + @retval EFI_BUFFER_TO_SMALL DataSize is too small for the resul= t. > + > +**/ > +EFI_STATUS > +EFIAPI > +RuntimeServiceGetNextVariableName ( > + IN OUT UINTN *VariableNameSize, > + IN OUT CHAR16 *VariableName, > + IN OUT EFI_GUID *VendorGuid > + ) > +{ > + EFI_STATUS Status; > + UINTN MaxLen; > + > + Status =3D EFI_NOT_FOUND; > + > + if (VariableNameSize =3D=3D NULL || VariableName =3D=3D NULL || Vendor= Guid =3D=3D > NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // Calculate the possible maximum length of name string, including the= Null > terminator. > + // > + MaxLen =3D *VariableNameSize / sizeof (CHAR16); > + if ((MaxLen =3D=3D 0) || (StrnLenS (VariableName, MaxLen) =3D=3D MaxLe= n)) { > + // > + // 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); > + if (FeaturePcdGet (PcdEnableVariableRuntimeCache)) { > + Status =3D GetNextVariableNameInRuntimeCache (VariableNameSize, > VariableName, VendorGuid); > + } else { > + Status =3D GetNextVariableNameInSmm (VariableNameSize, VariableName, > VendorGuid); > + } > ReleaseLockOnlyAtBootTime (&mVariableServicesLock); > + > return Status; > } >=20 > -- > 2.16.2.windows.1