From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: hao.a.wu@intel.com) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by groups.io with SMTP; Thu, 30 May 2019 22:41:40 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 May 2019 22:41:39 -0700 X-ExtLoop1: 1 Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 30 May 2019 22:41:39 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 30 May 2019 22:41:38 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.137]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.153]) with mapi id 14.03.0415.000; Fri, 31 May 2019 13:41:36 +0800 From: "Wu, Hao A" To: "devel@edk2.groups.io" , "Gao, Zhichao" CC: Bret Barkelew , "Wang, Jian J" , "Ni, Ray" , "Zeng, Star" , "Gao, Liming" , Sean Brogan , Michael Turner Subject: Re: [edk2-devel] [PATCH V2] MdeModulePkg/CapsulePei: Optimize the CapsulePei Thread-Topic: [edk2-devel] [PATCH V2] MdeModulePkg/CapsulePei: Optimize the CapsulePei Thread-Index: AQHVF1Ls/+0TrzoPYEajSOz2aDeI+KaEstmg Date: Fri, 31 May 2019 05:41:35 +0000 Message-ID: References: <20190531014805.9676-1-zhichao.gao@intel.com> In-Reply-To: <20190531014805.9676-1-zhichao.gao@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: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Gao, Zhichao > Sent: Friday, May 31, 2019 9:48 AM > To: devel@edk2.groups.io > Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Li= ming; > Sean Brogan; Michael Turner; Gao, Zhichao > Subject: [edk2-devel] [PATCH V2] MdeModulePkg/CapsulePei: Optimize the > CapsulePei >=20 > From: Bret Barkelew >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1853 >=20 > Optimize some function in CapsulePei to make it easier > to maintian. maintian -> maintain > 1. Separate the capsule check function form GetCapsuleDescriptors > to AreCapsulesStaged. The original logic is unclear. > 2. Avoid querying the capsule variable twice. First time to count > the number of SG list and allocate a buffer to save SG list data. > Second time to save the SG list data to the buffer. Modified: > Using a template buffer to save the SG list data. After query, > we get the number of SG list, then allocate memory and copy > data form template buffer to the allocated memory. > 3. Using MemoryAllocationLib instead of memory function in Pei > services. Not directly related with this patch. Now the PEIM has a mixed usage of both the PEI service and memory allocation library to allocate memory. Maybe a later patch can unify such usage. > 4, Remain 2 byte(CHAR16) to be the null-terminate of CapsuleVarName. Sorry for missing your reply in the V1 patch. Upon successful return of UnicodeValueToStringS(), the input string buffer is guaranteed to be null-terminated. I think the origin logic is fine. One more minor comment below. >=20 > Cc: Jian J Wang > Cc: Hao A Wu > Cc: Ray Ni > Cc: Star Zeng > Cc: Liming Gao > Cc: Sean Brogan > Cc: Michael Turner > Cc: Bret Barkelew > Signed-off-by: Zhichao gao > --- >=20 > Code change from > https://github.com/microsoft/mu_basecore/blob/release/201903/MdeMod > ulePkg/Universal/CapsulePei/UefiCapsule.c#L801 >=20 > Note: > 1. Change the AreCapsulesStaged: directly return the BOOLEAN result. > 2. While the template buffer is to small, double its size through memory > function. >=20 > MdeModulePkg/Universal/CapsulePei/Capsule.h | 3 +- > .../Universal/CapsulePei/CapsulePei.inf | 3 +- > .../Universal/CapsulePei/UefiCapsule.c | 355 ++++++++++-------- > 3 files changed, 192 insertions(+), 169 deletions(-) >=20 > diff --git a/MdeModulePkg/Universal/CapsulePei/Capsule.h > b/MdeModulePkg/Universal/CapsulePei/Capsule.h > index baf40423af..fc20dd8b92 100644 > --- a/MdeModulePkg/Universal/CapsulePei/Capsule.h > +++ b/MdeModulePkg/Universal/CapsulePei/Capsule.h > @@ -1,6 +1,6 @@ > /** @file >=20 > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
>=20 > SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > @@ -30,6 +30,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include > #include > #include > +#include > #include > #include "Common/CommonHeader.h" >=20 > diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf > b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf > index 5d43df3075..9c88b3986f 100644 > --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf > +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf > @@ -6,7 +6,7 @@ > # This external input must be validated carefully to avoid security is= sue like > # buffer overflow, integer overflow. > # > -# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved. > +# Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved. > # Copyright (c) 2017, AMD Incorporated. All rights reserved.
> # > # SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -43,6 +43,7 @@ > BaseLib > HobLib > BaseMemoryLib > + MemoryAllocationLib > PeiServicesLib > PeimEntryPoint > DebugLib > diff --git a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > index e967599e96..b3014478a3 100644 > --- a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > +++ b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > @@ -1,7 +1,7 @@ > /** @file > Capsule update PEIM for UEFI2.0 >=20 > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> Copyright (c) 2017, AMD Incorporated. All rights reserved.
>=20 > SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -10,6 +10,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >=20 > #include "Capsule.h" >=20 > +#define DEFAULT_SG_LIST_HEADS (20) > + > #ifdef MDE_CPU_IA32 > // > // Global Descriptor Table (GDT) > @@ -791,30 +793,87 @@ BuildMemoryResourceDescriptor ( > } >=20 > /** > - Checks for the presence of capsule descriptors. > - Get capsule descriptors from variable CapsuleUpdateData, > CapsuleUpdateData1, CapsuleUpdateData2... > - and save to DescriptorBuffer. > + Check if the capsules are staged. >=20 > - @param DescriptorBuffer Pointer to the capsule descriptors > + @retval TRUE The capsules are staged. > + @retval FALSE The capsules are not staged. > + > +**/ > +BOOLEAN > +AreCapsulesStaged ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + UINTN Size; > + EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices; > + EFI_PHYSICAL_ADDRESS CapsuleDataPtr64; > + > + CapsuleDataPtr64 =3D 0; > + > + Status =3D PeiServicesLocatePpi ( > + &gEfiPeiReadOnlyVariable2PpiGuid, > + 0, > + NULL, > + (VOID **)&PPIVariableServices > + ); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI\n")); > + return FALSE; > + } > + > + // > + // Check for Update capsule > + // > + Size =3D sizeof (CapsuleDataPtr64); > + Status =3D PPIVariableServices->GetVariable( > + PPIVariableServices, > + EFI_CAPSULE_VARIABLE_NAME, > + &gEfiCapsuleVendorGuid, > + NULL, > + &Size, > + (VOID *)&CapsuleDataPtr64 > + ); > + > + if (!EFI_ERROR (Status)) { > + return TRUE; > + } > + > + return FALSE; > +} > + > +/** > + Check all the variables for SG list heads and get the count and addre= sses. > + > + @param ListLength A pointer would return the SG list le= ngth. > + @param HeadList A ponter to the capsule SG list. > + > + @retval EFI_SUCCESS a valid capsule is present > + @retval EFI_NOT_FOUND if a valid capsule is not present > + @retval EFI_INVALID_PARAMETER the input parameter is invalid > + @retval EFI_OUT_OF_RESOURCE fail to allocate memory >=20 > - @retval EFI_SUCCESS a valid capsule is present > - @retval EFI_NOT_FOUND if a valid capsule is not present > **/ > EFI_STATUS > -GetCapsuleDescriptors ( > - IN EFI_PHYSICAL_ADDRESS *DescriptorBuffer > +GetScatterGatherHeadEntries ( > + OUT UINTN *ListLength, > + OUT EFI_PHYSICAL_ADDRESS **HeadList > ) > { > - EFI_STATUS Status; > - UINTN Size; > - UINTN Index; > - UINTN TempIndex; > - UINTN ValidIndex; > - BOOLEAN Flag; > - CHAR16 CapsuleVarName[30]; > - CHAR16 *TempVarName; > - EFI_PHYSICAL_ADDRESS CapsuleDataPtr64; > - EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices; > + EFI_STATUS Status; > + UINTN Size; > + UINTN Index; > + UINTN TempIndex; > + UINTN ValidIndex; > + BOOLEAN Flag; > + CHAR16 CapsuleVarName[30]; > + CHAR16 *TempVarName; > + EFI_PHYSICAL_ADDRESS CapsuleDataPtr64; > + EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices; > + EFI_PHYSICAL_ADDRESS *TempList; > + EFI_PHYSICAL_ADDRESS *EnlargedTempList; > + UINTN TempListLength; >=20 > Index =3D 0; > TempVarName =3D NULL; > @@ -822,87 +881,118 @@ GetCapsuleDescriptors ( > ValidIndex =3D 0; > CapsuleDataPtr64 =3D 0; >=20 > + if ((ListLength =3D=3D NULL) || (HeadList =3D=3D NULL)) { > + DEBUG ((DEBUG_ERROR, "%a Invalid parameters. Inputs can't be > NULL\n", __FUNCTION__)); > + ASSERT (ListLength !=3D NULL); > + ASSERT (HeadList !=3D NULL); > + return EFI_INVALID_PARAMETER; > + } > + > + *ListLength =3D 0; > + *HeadList =3D NULL; > + > Status =3D PeiServicesLocatePpi ( > &gEfiPeiReadOnlyVariable2PpiGuid, > 0, > NULL, > (VOID **) &PPIVariableServices > ); > - if (Status =3D=3D EFI_SUCCESS) { > - StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16), > EFI_CAPSULE_VARIABLE_NAME); > - TempVarName =3D CapsuleVarName + StrLen (CapsuleVarName); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI: %r\n", > Status)); > + return Status; > + } > + > + // > + // Allocate memory for sg list head > + // > + TempListLength =3D DEFAULT_SG_LIST_HEADS * sizeof > (EFI_PHYSICAL_ADDRESS); > + TempList =3D AllocateZeroPool (TempListLength); > + if (TempList =3D=3D NULL) { > + DEBUG((DEBUG_ERROR, "Failed to allocate memory\n")); > + return EFI_OUT_OF_RESOURCES; > + } > + > + // > + // Setup var name buffer for update capsules > + // > + StrCpyS (CapsuleVarName, sizeof (CapsuleVarName) / sizeof (CHAR16), > EFI_CAPSULE_VARIABLE_NAME); > + TempVarName =3D CapsuleVarName + StrLen (CapsuleVarName); > + while (TRUE) { > + if (Index !=3D 0) { > + UnicodeValueToStringS ( > + TempVarName, > + (sizeof(CapsuleVarName) - ((StrLen(CapsuleVarName) + 1) * > sizeof(CHAR16))), > + 0, > + Index, > + 0 > + ); > + } > + > Size =3D sizeof (CapsuleDataPtr64); > - while (1) { > - if (Index =3D=3D 0) { > - // > - // For the first Capsule Image > - // > - Status =3D PPIVariableServices->GetVariable ( > - PPIVariableServices, > - CapsuleVarName, > - &gEfiCapsuleVendorGuid, > - NULL, > - &Size, > - (VOID *) &CapsuleDataPtr64 > - ); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_INFO, "Capsule -- capsule variable not set\n"))= ; > - return EFI_NOT_FOUND; > - } > - // > - // We have a chicken/egg situation where the memory init code n= eeds > to > - // know the boot mode prior to initializing memory. For this ca= se, our > - // validate function will fail. We can detect if this is the ca= se if blocklist > - // pointer is null. In that case, return success since we know = that the > - // variable is set. > - // > - if (DescriptorBuffer =3D=3D NULL) { > - return EFI_SUCCESS; > - } > - } else { > - UnicodeValueToStringS ( > - TempVarName, > - sizeof (CapsuleVarName) - ((UINTN)TempVarName - > (UINTN)CapsuleVarName), > - 0, > - Index, > - 0 > - ); > - Status =3D PPIVariableServices->GetVariable ( > - PPIVariableServices, > - CapsuleVarName, > - &gEfiCapsuleVendorGuid, > - NULL, > - &Size, > - (VOID *) &CapsuleDataPtr64 > - ); > - if (EFI_ERROR (Status)) { > - break; > - } > + Status =3D PPIVariableServices->GetVariable ( > + PPIVariableServices, > + CapsuleVarName, > + &gEfiCapsuleVendorGuid, > + NULL, > + &Size, > + (VOID *)&CapsuleDataPtr64 > + ); >=20 > - // > - // If this BlockList has been linked before, skip this variable > - // > - Flag =3D FALSE; > - for (TempIndex =3D 0; TempIndex < ValidIndex; TempIndex++) { > - if (DescriptorBuffer[TempIndex] =3D=3D CapsuleDataPtr64) { > - Flag =3D TRUE; > - break; > - } > - } > - if (Flag) { > - Index ++; > - continue; > - } > + if (EFI_ERROR (Status)) { > + if (Status !=3D EFI_NOT_FOUND) { > + DEBUG ((DEBUG_ERROR, "Unexpected error getting Capsule Update > variable. Status =3D %r\n")); > + } > + break; > + } > + > + // > + // If this BlockList has been linked before, skip this variable > + // > + Flag =3D FALSE; > + for (TempIndex =3D 0; TempIndex < ValidIndex; TempIndex++) { > + if (TempList[TempIndex] =3D=3D CapsuleDataPtr64) { > + Flag =3D TRUE; > + break; > } > + } > + if (Flag) { > + Index++; > + continue; > + } >=20 > - // > - // Cache BlockList which has been processed > - // > - DescriptorBuffer[ValidIndex++] =3D CapsuleDataPtr64; > - Index ++; > + // > + // The TempList is full, enlarge it > + // > + if ((ValidIndex + 1) >=3D TempListLength) { > + EnlargedTempList =3D AllocateZeroPool (TempListLength * 2); > + CopyMem (EnlargedTempList, TempList, TempListLength); > + FreePool (TempList); > + TempList =3D EnlargedTempList; > + TempListLength *=3D 2; > } > + > + // > + // Cache BlockList which has been processed > + // > + TempList[ValidIndex++] =3D CapsuleDataPtr64; > + Index++; > + } > + > + if (ValidIndex =3D=3D 0) { > + DEBUG((DEBUG_ERROR, "%a didn't find any SG lists in variables\n", > __FUNCTION__)); > + return EFI_NOT_FOUND; > } >=20 > + *HeadList =3D AllocateZeroPool ((ValidIndex + 1) * sizeof > (EFI_PHYSICAL_ADDRESS)); > + if (*HeadList =3D=3D NULL) { > + DEBUG((DEBUG_ERROR, "Failed to allocate memory\n")); > + return EFI_OUT_OF_RESOURCES; > + } > + > + CopyMem (*HeadList, TempList, (ValidIndex) * sizeof > (EFI_PHYSICAL_ADDRESS)); > + *ListLength =3D ValidIndex; Please help to remove the tailing white space. With the above comments addressed, Reviewed-by: Hao A Wu Best Regards, Hao Wu > + > return EFI_SUCCESS; > } >=20 > @@ -937,17 +1027,11 @@ CapsuleCoalesce ( > IN OUT UINTN *MemorySize > ) > { > - UINTN Index; > - UINTN Size; > - UINTN VariableCount; > - CHAR16 CapsuleVarName[30]; > - CHAR16 *TempVarName; > - EFI_PHYSICAL_ADDRESS CapsuleDataPtr64; > EFI_STATUS Status; > EFI_BOOT_MODE BootMode; > - EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices; > EFI_PHYSICAL_ADDRESS *VariableArrayAddress; > MEMORY_RESOURCE_DESCRIPTOR *MemoryResource; > + UINTN ListLength; > #ifdef MDE_CPU_IA32 > UINT16 CoalesceImageMachineType; > EFI_PHYSICAL_ADDRESS CoalesceImageEntryPoint; > @@ -955,10 +1039,8 @@ CapsuleCoalesce ( > EFI_CAPSULE_LONG_MODE_BUFFER LongModeBuffer; > #endif >=20 > - Index =3D 0; > - VariableCount =3D 0; > - CapsuleVarName[0] =3D 0; > - CapsuleDataPtr64 =3D 0; > + ListLength =3D 0; > + VariableArrayAddress =3D NULL; >=20 > // > // Someone should have already ascertained the boot mode. If it's not > @@ -972,74 +1054,11 @@ CapsuleCoalesce ( > } >=20 > // > - // User may set the same ScatterGatherList with several different var= iables, > - // so cache all ScatterGatherList for check later. > - // > - Status =3D PeiServicesLocatePpi ( > - &gEfiPeiReadOnlyVariable2PpiGuid, > - 0, > - NULL, > - (VOID **) &PPIVariableServices > - ); > - if (EFI_ERROR (Status)) { > - goto Done; > - } > - Size =3D sizeof (CapsuleDataPtr64); > - StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16), > EFI_CAPSULE_VARIABLE_NAME); > - TempVarName =3D CapsuleVarName + StrLen (CapsuleVarName); > - while (TRUE) { > - if (Index > 0) { > - UnicodeValueToStringS ( > - TempVarName, > - sizeof (CapsuleVarName) - ((UINTN)TempVarName - > (UINTN)CapsuleVarName), > - 0, > - Index, > - 0 > - ); > - } > - Status =3D PPIVariableServices->GetVariable ( > - PPIVariableServices, > - CapsuleVarName, > - &gEfiCapsuleVendorGuid, > - NULL, > - &Size, > - (VOID *) &CapsuleDataPtr64 > - ); > - if (EFI_ERROR (Status)) { > - // > - // There is no capsule variables, quit > - // > - DEBUG ((DEBUG_INFO,"Capsule variable Index =3D %d\n", Index)); > - break; > - } > - VariableCount++; > - Index++; > - } > - > - DEBUG ((DEBUG_INFO,"Capsule variable count =3D %d\n", VariableCount))= ; > - > - // > - // The last entry is the end flag. > - // > - Status =3D PeiServicesAllocatePool ( > - (VariableCount + 1) * sizeof (EFI_PHYSICAL_ADDRESS), > - (VOID **)&VariableArrayAddress > - ); > - > - if (Status !=3D EFI_SUCCESS) { > - DEBUG ((DEBUG_ERROR, "AllocatePages Failed!, Status =3D %x\n", Stat= us)); > - goto Done; > - } > - > - ZeroMem (VariableArrayAddress, (VariableCount + 1) * sizeof > (EFI_PHYSICAL_ADDRESS)); > - > - // > - // Find out if we actually have a capsule. > - // GetCapsuleDescriptors depends on variable PPI, so it should run in= 32-bit > environment. > + // Get ScatterGatherList > // > - Status =3D GetCapsuleDescriptors (VariableArrayAddress); > + Status =3D GetScatterGatherHeadEntries (&ListLength, > &VariableArrayAddress); > if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "Fail to find capsule variables.\n")); > + DEBUG ((DEBUG_ERROR, "%a failed to get Scatter Gather List Head > Entries. Status =3D %r\n", __FUNCTION__, Status)); > goto Done; > } >=20 > @@ -1117,9 +1136,11 @@ CheckCapsuleUpdate ( > IN EFI_PEI_SERVICES **PeiServices > ) > { > - EFI_STATUS Status; > - Status =3D GetCapsuleDescriptors (NULL); > - return Status; > + if (AreCapsulesStaged ()) { > + return EFI_SUCCESS; > + } else { > + return EFI_NOT_FOUND; > + } > } > /** > This function will look at a capsule and determine if it's a test pat= tern. > -- > 2.21.0.windows.1 >=20 >=20 >=20