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; Tue, 28 May 2019 23:54:57 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 May 2019 23:54:56 -0700 X-ExtLoop1: 1 Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga006.jf.intel.com with ESMTP; 28 May 2019 23:54:56 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 28 May 2019 23:54:55 -0700 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 28 May 2019 23:54:53 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.33]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.136]) with mapi id 14.03.0415.000; Wed, 29 May 2019 14:54:51 +0800 From: "Wu, Hao A" To: "Gao, Zhichao" , "devel@edk2.groups.io" CC: Bret Barkelew , "Wang, Jian J" , "Ni, Ray" , "Zeng, Star" , "Gao, Liming" , Sean Brogan , Michael Turner Subject: Re: [PATCH] MdeModulePkg/CapsulePei: Optimize the CapsulePei Thread-Topic: [PATCH] MdeModulePkg/CapsulePei: Optimize the CapsulePei Thread-Index: AQHVFbfnYNPany5s2k2d3KexyDFyn6aBotQw Date: Wed, 29 May 2019 06:54:51 +0000 Message-ID: References: <20190529004555.45364-1-zhichao.gao@intel.com> In-Reply-To: <20190529004555.45364-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: Gao, Zhichao > Sent: Wednesday, May 29, 2019 8:46 AM > To: devel@edk2.groups.io > Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Lim= ing; > Sean Brogan; Michael Turner; Gao, Zhichao > Subject: [PATCH] MdeModulePkg/CapsulePei: Optimize the CapsulePei >=20 > From: Bret Barkelew >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1853 >=20 > Sperate the capsule check function from GetCapsuleDescriptors Sperate -> Separate > and name it to AreCapsulesStaged. > Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries. > And optimize its to remove the duplicated code. >=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 > --- > MdeModulePkg/Universal/CapsulePei/Capsule.h | 3 +- > .../Universal/CapsulePei/CapsulePei.inf | 3 +- > .../Universal/CapsulePei/UefiCapsule.c | 357 ++++++++++-------- > 3 files changed, 194 insertions(+), 169 deletions(-) I am a bit confused for the purpose of this patch. My understanding is that this patch will refine this driver to remove duplicated code by abstract common codes into a new function. And there will be no functional impact. However, after the change, the line of codes of this driver increased by 20+ lines. Did I miss something for the purpose of this patch? Some additional comments below. >=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 iss= ue 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..2d003369ca 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,89 @@ 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 > +EFIAPI > +AreCapsulesStaged ( Keyword 'EFIAPI' seems not needed here. AreCapsulesStaged() is an internal function here. > + 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 addres= ses. > + > + @param ListLength A pointer would return the SG list len= gth. > + @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 > +EFIAPI > +GetScatterGatherHeadEntries ( Keyword 'EFIAPI' seems not needed here. GetScatterGatherHeadEntries() is an internal function here. > + 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 +883,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))), Compared with the origin code: ''' sizeof (CapsuleVarName) - ((UINTN)TempVarName - (UINTN)CapsuleVarName), ''' The size of buffer passed into function UnicodeValueToStringS() is 2 bytes smaller for the modified code. Is there a reason for such change? Best Regards, Hao Wu > + 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 ne= eds > to > - // know the boot mode prior to initializing memory. For this cas= e, our > - // validate function will fail. We can detect if this is the cas= e if blocklist > - // pointer is null. In that case, return success since we know t= hat 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; > + } >=20 > - // > - // Cache BlockList which has been processed > - // > - DescriptorBuffer[ValidIndex++] =3D CapsuleDataPtr64; > - Index ++; > + // > + // 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; > + } > + > + // > + // 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; > + > return EFI_SUCCESS; > } >=20 > @@ -937,17 +1029,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 +1041,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 +1056,11 @@ CapsuleCoalesce ( > } >=20 > // > - // User may set the same ScatterGatherList with several different vari= ables, > - // so cache all ScatterGatherList for check later. > + // Get ScatterGatherList > // > - Status =3D PeiServicesLocatePpi ( > - &gEfiPeiReadOnlyVariable2PpiGuid, > - 0, > - NULL, > - (VOID **) &PPIVariableServices > - ); > + Status =3D GetScatterGatherHeadEntries (&ListLength, > &VariableArrayAddress); > 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", Statu= s)); > - 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. > - // > - Status =3D GetCapsuleDescriptors (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 +1138,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 patt= ern. > -- > 2.21.0.windows.1