[edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize the CapsulePei
Gao, Zhichao
zhichao.gao at intel.com
Thu May 30 03:20:54 UTC 2019
> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, May 29, 2019 2:55 PM
> To: Gao, Zhichao <zhichao.gao at intel.com>; devel at edk2.groups.io
> Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>; Wang, Jian J
> <jian.j.wang at intel.com>; Ni, Ray <ray.ni at intel.com>; Zeng, Star
> <star.zeng at intel.com>; Gao, Liming <liming.gao at intel.com>; Sean Brogan
> <sean.brogan at microsoft.com>; Michael Turner
> <Michael.Turner at microsoft.com>
> Subject: RE: [PATCH] MdeModulePkg/CapsulePei: Optimize the CapsulePei
>
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Wednesday, May 29, 2019 8:46 AM
> > To: devel at edk2.groups.io
> > Cc: Bret Barkelew; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao,
> > Liming; Sean Brogan; Michael Turner; Gao, Zhichao
> > Subject: [PATCH] MdeModulePkg/CapsulePei: Optimize the CapsulePei
> >
> > From: Bret Barkelew <Bret.Barkelew at microsoft.com>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853
> >
> > 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.
> >
> > Cc: Jian J Wang <jian.j.wang at intel.com>
> > Cc: Hao A Wu <hao.a.wu at intel.com>
> > Cc: Ray Ni <ray.ni at intel.com>
> > Cc: Star Zeng <star.zeng at intel.com>
> > Cc: Liming Gao <liming.gao at intel.com>
> > Cc: Sean Brogan <sean.brogan at microsoft.com>
> > Cc: Michael Turner <Michael.Turner at microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>
> > Signed-off-by: Zhichao gao <zhichao.gao at intel.com>
> > ---
> > 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?
The commit message should be update:
I view the code change again, here is the purpose of this patch:
1. separate the check function from GetCapsuleDescriptors. The original logic GetCapsuleDescriptors in is unclear.
2. avoid calling query capsule variable twice, first time to get the SG list number and allocate buffer to save it, second time to copy the SG list to the buffer.
After the patch it would put the SG list data into a template buffer and count the number. Then allocate the memory and copy data.
I would update the above info to next patch. And remove the incorrect description.
>
> Some additional comments below.
>
> >
> > 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
> >
> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> >
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -30,6 +30,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > #include <Library/PcdLib.h> #include <Library/ReportStatusCodeLib.h>
> > #include <Library/DebugAgentLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > #include <IndustryStandard/PeImage.h> #include
> > "Common/CommonHeader.h"
> >
> > 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
> > issue like # buffer overflow, integer overflow.
> > #
> > -# Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +# Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> > # Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> #
> > # 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
> >
> > -Copyright (c) 2006 - 2018, Intel Corporation. All rights
> > reserved.<BR>
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights
> > +reserved.<BR>
> > Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> >
> > SPDX-License-Identifier: BSD-2-Clause-Patent @@ -10,6 +10,8 @@
> > SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > #include "Capsule.h"
> >
> > +#define DEFAULT_SG_LIST_HEADS (20)
> > +
> > #ifdef MDE_CPU_IA32
> > //
> > // Global Descriptor Table (GDT)
> > @@ -791,30 +793,89 @@ BuildMemoryResourceDescriptor ( }
> >
> > /**
> > - 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.
> >
> > - @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 = 0;
> > +
> > + Status = 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 = sizeof (CapsuleDataPtr64);
> > + Status = 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 addresses.
> > +
> > + @param ListLength A pointer would return the SG list length.
> > + @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
> >
> > - @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;
> >
> > Index = 0;
> > TempVarName = NULL;
> > @@ -822,87 +883,118 @@ GetCapsuleDescriptors (
> > ValidIndex = 0;
> > CapsuleDataPtr64 = 0;
> >
> > + if ((ListLength == NULL) || (HeadList == NULL)) {
> > + DEBUG ((DEBUG_ERROR, "%a Invalid parameters. Inputs can't be
> > NULL\n", __FUNCTION__));
> > + ASSERT (ListLength != NULL);
> > + ASSERT (HeadList != NULL);
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + *ListLength = 0;
> > + *HeadList = NULL;
> > +
> > Status = PeiServicesLocatePpi (
> > &gEfiPeiReadOnlyVariable2PpiGuid,
> > 0,
> > NULL,
> > (VOID **) &PPIVariableServices
> > );
> > - if (Status == EFI_SUCCESS) {
> > - StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16),
> > EFI_CAPSULE_VARIABLE_NAME);
> > - TempVarName = 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 =
> > + DEFAULT_SG_LIST_HEADS * sizeof
> > (EFI_PHYSICAL_ADDRESS);
> > + TempList = AllocateZeroPool (TempListLength); if (TempList ==
> > + 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 = CapsuleVarName + StrLen (CapsuleVarName); while
> > + (TRUE) {
> > + if (Index != 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?
Just my thought. This can remain two byte to be zero as the null-terminate of the string. The original one may override the last two byte and the buffer can not be regard as a legal string.
Thanks,
Zhichao
>
> Best Regards,
> Hao Wu
>
> > + 0,
> > + Index,
> > + 0
> > + );
> > + }
> > +
> > Size = sizeof (CapsuleDataPtr64);
> > - while (1) {
> > - if (Index == 0) {
> > - //
> > - // For the first Capsule Image
> > - //
> > - Status = 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 needs
> > to
> > - // know the boot mode prior to initializing memory. For this case, our
> > - // validate function will fail. We can detect if this is the case if blocklist
> > - // pointer is null. In that case, return success since we know that the
> > - // variable is set.
> > - //
> > - if (DescriptorBuffer == NULL) {
> > - return EFI_SUCCESS;
> > - }
> > - } else {
> > - UnicodeValueToStringS (
> > - TempVarName,
> > - sizeof (CapsuleVarName) - ((UINTN)TempVarName -
> > (UINTN)CapsuleVarName),
> > - 0,
> > - Index,
> > - 0
> > - );
> > - Status = PPIVariableServices->GetVariable (
> > - PPIVariableServices,
> > - CapsuleVarName,
> > - &gEfiCapsuleVendorGuid,
> > - NULL,
> > - &Size,
> > - (VOID *) &CapsuleDataPtr64
> > - );
> > - if (EFI_ERROR (Status)) {
> > - break;
> > - }
> > + Status = PPIVariableServices->GetVariable (
> > + PPIVariableServices,
> > + CapsuleVarName,
> > + &gEfiCapsuleVendorGuid,
> > + NULL,
> > + &Size,
> > + (VOID *)&CapsuleDataPtr64
> > + );
> >
> > - //
> > - // If this BlockList has been linked before, skip this variable
> > - //
> > - Flag = FALSE;
> > - for (TempIndex = 0; TempIndex < ValidIndex; TempIndex++) {
> > - if (DescriptorBuffer[TempIndex] == CapsuleDataPtr64) {
> > - Flag = TRUE;
> > - break;
> > - }
> > - }
> > - if (Flag) {
> > - Index ++;
> > - continue;
> > - }
> > + if (EFI_ERROR (Status)) {
> > + if (Status != EFI_NOT_FOUND) {
> > + DEBUG ((DEBUG_ERROR, "Unexpected error getting Capsule Update
> > variable. Status = %r\n"));
> > }
> > + break;
> > + }
> >
> > - //
> > - // Cache BlockList which has been processed
> > - //
> > - DescriptorBuffer[ValidIndex++] = CapsuleDataPtr64;
> > - Index ++;
> > + //
> > + // If this BlockList has been linked before, skip this variable
> > + //
> > + Flag = FALSE;
> > + for (TempIndex = 0; TempIndex < ValidIndex; TempIndex++) {
> > + if (TempList[TempIndex] == CapsuleDataPtr64) {
> > + Flag = TRUE;
> > + break;
> > + }
> > }
> > + if (Flag) {
> > + Index++;
> > + continue;
> > + }
> > +
> > + //
> > + // The TempList is full, enlarge it
> > + //
> > + if ((ValidIndex + 1) >= TempListLength) {
> > + EnlargedTempList = AllocateZeroPool (TempListLength * 2);
> > + CopyMem (EnlargedTempList, TempList, TempListLength);
> > + FreePool (TempList);
> > + TempList = EnlargedTempList;
> > + TempListLength *= 2;
> > + }
> > +
> > + //
> > + // Cache BlockList which has been processed
> > + //
> > + TempList[ValidIndex++] = CapsuleDataPtr64;
> > + Index++;
> > + }
> > +
> > + if (ValidIndex == 0) {
> > + DEBUG((DEBUG_ERROR, "%a didn't find any SG lists in variables\n",
> > __FUNCTION__));
> > + return EFI_NOT_FOUND;
> > }
> >
> > + *HeadList = AllocateZeroPool ((ValidIndex + 1) * sizeof
> > (EFI_PHYSICAL_ADDRESS));
> > + if (*HeadList == NULL) {
> > + DEBUG((DEBUG_ERROR, "Failed to allocate memory\n"));
> > + return EFI_OUT_OF_RESOURCES;
> > + }
> > +
> > + CopyMem (*HeadList, TempList, (ValidIndex) * sizeof
> > (EFI_PHYSICAL_ADDRESS));
> > + *ListLength = ValidIndex;
> > +
> > return EFI_SUCCESS;
> > }
> >
> > @@ -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
> >
> > - Index = 0;
> > - VariableCount = 0;
> > - CapsuleVarName[0] = 0;
> > - CapsuleDataPtr64 = 0;
> > + ListLength = 0;
> > + VariableArrayAddress = NULL;
> >
> > //
> > // Someone should have already ascertained the boot mode. If it's
> > not @@ -972,74 +1056,11 @@ CapsuleCoalesce (
> > }
> >
> > //
> > - // User may set the same ScatterGatherList with several different
> > variables,
> > - // so cache all ScatterGatherList for check later.
> > + // Get ScatterGatherList
> > //
> > - Status = PeiServicesLocatePpi (
> > - &gEfiPeiReadOnlyVariable2PpiGuid,
> > - 0,
> > - NULL,
> > - (VOID **) &PPIVariableServices
> > - );
> > + Status = GetScatterGatherHeadEntries (&ListLength,
> > &VariableArrayAddress);
> > if (EFI_ERROR (Status)) {
> > - goto Done;
> > - }
> > - Size = sizeof (CapsuleDataPtr64);
> > - StrCpyS (CapsuleVarName, sizeof(CapsuleVarName)/sizeof(CHAR16),
> > EFI_CAPSULE_VARIABLE_NAME);
> > - TempVarName = CapsuleVarName + StrLen (CapsuleVarName);
> > - while (TRUE) {
> > - if (Index > 0) {
> > - UnicodeValueToStringS (
> > - TempVarName,
> > - sizeof (CapsuleVarName) - ((UINTN)TempVarName -
> > (UINTN)CapsuleVarName),
> > - 0,
> > - Index,
> > - 0
> > - );
> > - }
> > - Status = 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 = %d\n", Index));
> > - break;
> > - }
> > - VariableCount++;
> > - Index++;
> > - }
> > -
> > - DEBUG ((DEBUG_INFO,"Capsule variable count = %d\n",
> > VariableCount));
> > -
> > - //
> > - // The last entry is the end flag.
> > - //
> > - Status = PeiServicesAllocatePool (
> > - (VariableCount + 1) * sizeof (EFI_PHYSICAL_ADDRESS),
> > - (VOID **)&VariableArrayAddress
> > - );
> > -
> > - if (Status != EFI_SUCCESS) {
> > - DEBUG ((DEBUG_ERROR, "AllocatePages Failed!, Status = %x\n",
> Status));
> > - 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 = 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 = %r\n", __FUNCTION__, Status));
> > goto Done;
> > }
> >
> > @@ -1117,9 +1138,11 @@ CheckCapsuleUpdate (
> > IN EFI_PEI_SERVICES **PeiServices
> > )
> > {
> > - EFI_STATUS Status;
> > - Status = 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 pattern.
> > --
> > 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41657): https://edk2.groups.io/g/devel/message/41657
Mute This Topic: https://groups.io/mt/31828852/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list