MdeModulePkg/Universal/CapsulePei/Capsule.h | 3 +- .../Universal/CapsulePei/CapsulePei.inf | 3 +- .../Universal/CapsulePei/UefiCapsule.c | 357 ++++++++++-------- 3 files changed, 194 insertions(+), 169 deletions(-)
From: Bret Barkelew <Bret.Barkelew@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853
Sperate the capsule check function from GetCapsuleDescriptors
and name it to AreCapsulesStaged.
Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries.
And optimize its to remove the duplicated code.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Cc: Michael Turner <Michael.Turner@microsoft.com>
Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Signed-off-by: Zhichao gao <zhichao.gao@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(-)
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 (
+ 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 (
+ 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))),
+ 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 (#41553): https://edk2.groups.io/g/devel/message/41553
Mute This Topic: https://groups.io/mt/31828852/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Wed, May 29, 2019 at 08:45:55AM +0800, Gao, Zhichao wrote:
> From: Bret Barkelew <Bret.Barkelew@microsoft.com>
If this code is from Microsoft...
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853
>
> Sperate the capsule check function from GetCapsuleDescriptors
> and name it to AreCapsulesStaged.
> Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries.
> And optimize its to remove the duplicated code.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Signed-off-by: Zhichao gao <zhichao.gao@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(-)
>
> 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>
...why does Intel get the copyright?
/
Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#41599): https://edk2.groups.io/g/devel/message/41599
Mute This Topic: https://groups.io/mt/31828852/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
I just update the date of copyright. And the code in Mu project didn't add its own copyright. If it is required, I would add it for them. And I also make some minor changes on it. Thanks, Zhichao > -----Original Message----- > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > Sent: Wednesday, May 29, 2019 7:12 PM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > Michael Turner <Michael.Turner@microsoft.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize the > CapsulePei > > On Wed, May 29, 2019 at 08:45:55AM +0800, Gao, Zhichao wrote: > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > If this code is from Microsoft... > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853 > > > > Sperate the capsule check function from GetCapsuleDescriptors and name > > it to AreCapsulesStaged. > > Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries. > > And optimize its to remove the duplicated code. > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > Cc: Hao A Wu <hao.a.wu@intel.com> > > Cc: Ray Ni <ray.ni@intel.com> > > Cc: Star Zeng <star.zeng@intel.com> > > Cc: Liming Gao <liming.gao@intel.com> > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > Signed-off-by: Zhichao gao <zhichao.gao@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(-) > > > > 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> > > ...why does Intel get the copyright? > > / > Leif -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41621): https://edk2.groups.io/g/devel/message/41621 Mute This Topic: https://groups.io/mt/31828852/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
No. Please don't touch copyright if you doesn't change the file. > -----Original Message----- > From: Gao, Zhichao > Sent: Wednesday, May 29, 2019 11:01 PM > To: Leif Lindholm <leif.lindholm@linaro.org>; devel@edk2.groups.io > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > Michael Turner <Michael.Turner@microsoft.com> > Subject: RE: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize the CapsulePei > > I just update the date of copyright. And the code in Mu project didn't add its own copyright. > If it is required, I would add it for them. > And I also make some minor changes on it. > > Thanks, > Zhichao > > > -----Original Message----- > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > > Sent: Wednesday, May 29, 2019 7:12 PM > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > Michael Turner <Michael.Turner@microsoft.com> > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize the > > CapsulePei > > > > On Wed, May 29, 2019 at 08:45:55AM +0800, Gao, Zhichao wrote: > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > If this code is from Microsoft... > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853 > > > > > > Sperate the capsule check function from GetCapsuleDescriptors and name > > > it to AreCapsulesStaged. > > > Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries. > > > And optimize its to remove the duplicated code. > > > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > Cc: Hao A Wu <hao.a.wu@intel.com> > > > Cc: Ray Ni <ray.ni@intel.com> > > > Cc: Star Zeng <star.zeng@intel.com> > > > Cc: Liming Gao <liming.gao@intel.com> > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > Signed-off-by: Zhichao gao <zhichao.gao@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(-) > > > > > > 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> > > > > ...why does Intel get the copyright? > > > > / > > Leif -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41622): https://edk2.groups.io/g/devel/message/41622 Mute This Topic: https://groups.io/mt/31828852/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Wed, May 29, 2019 at 03:01:12PM +0000, Gao, Zhichao wrote: > I just update the date of copyright. And the code in Mu project didn't add its own copyright. > If it is required, I would add it for them. Well, hopefully Microsoft will add their own copyright to the original :) Although it would certainly be better to add it here as well anyway. So what modifications were made to the code on the way from the project Mu repository? That would be useful to mention in the commit message. Regards, Leif > And I also make some minor changes on it. > > Thanks, > Zhichao > > > -----Original Message----- > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > > Sent: Wednesday, May 29, 2019 7:12 PM > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > Michael Turner <Michael.Turner@microsoft.com> > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize the > > CapsulePei > > > > On Wed, May 29, 2019 at 08:45:55AM +0800, Gao, Zhichao wrote: > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > If this code is from Microsoft... > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853 > > > > > > Sperate the capsule check function from GetCapsuleDescriptors and name > > > it to AreCapsulesStaged. > > > Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries. > > > And optimize its to remove the duplicated code. > > > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > Cc: Hao A Wu <hao.a.wu@intel.com> > > > Cc: Ray Ni <ray.ni@intel.com> > > > Cc: Star Zeng <star.zeng@intel.com> > > > Cc: Liming Gao <liming.gao@intel.com> > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > Signed-off-by: Zhichao gao <zhichao.gao@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(-) > > > > > > 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> > > > > ...why does Intel get the copyright? > > > > / > > Leif > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41623): https://edk2.groups.io/g/devel/message/41623 Mute This Topic: https://groups.io/mt/31828852/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message----- > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > Sent: Wednesday, May 29, 2019 11:09 PM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > Michael Turner <Michael.Turner@microsoft.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize the > CapsulePei > > On Wed, May 29, 2019 at 03:01:12PM +0000, Gao, Zhichao wrote: > > I just update the date of copyright. And the code in Mu project didn't add > its own copyright. > > If it is required, I would add it for them. > > Well, hopefully Microsoft will add their own copyright to the original > :) > > Although it would certainly be better to add it here as well anyway. I think it is better to let MS to add the copyright by themselves. > > So what modifications were made to the code on the way from the project > Mu repository? That would be useful to mention in the commit message. I would add this info blow commit message(not in commit message). It is helpful for review. But it may not be useful to add them in the commit message. On my opinion, the commit message should contain the summary and impact of the changes. Thanks, Zhichao > > Regards, > > Leif > > > And I also make some minor changes on it. > > > > Thanks, > > Zhichao > > > > > -----Original Message----- > > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > > > Sent: Wednesday, May 29, 2019 7:12 PM > > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > > > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > > Michael Turner <Michael.Turner@microsoft.com> > > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize > > > the CapsulePei > > > > > > On Wed, May 29, 2019 at 08:45:55AM +0800, Gao, Zhichao wrote: > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > If this code is from Microsoft... > > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853 > > > > > > > > Sperate the capsule check function from GetCapsuleDescriptors and > > > > name it to AreCapsulesStaged. > > > > Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries. > > > > And optimize its to remove the duplicated code. > > > > > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > > Cc: Hao A Wu <hao.a.wu@intel.com> > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > Cc: Star Zeng <star.zeng@intel.com> > > > > Cc: Liming Gao <liming.gao@intel.com> > > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > Signed-off-by: Zhichao gao <zhichao.gao@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(-) > > > > > > > > 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> > > > > > > ...why does Intel get the copyright? > > > > > > / > > > Leif > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41701): https://edk2.groups.io/g/devel/message/41701 Mute This Topic: https://groups.io/mt/31828852/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Fri, May 31, 2019 at 01:46:14AM +0000, Gao, Zhichao wrote: > > So what modifications were made to the code on the way from the project > > Mu repository? That would be useful to mention in the commit message. > > I would add this info blow commit message(not in commit message). It > is helpful for review. But it may not be useful to add them in the > commit message. > On my opinion, the commit message should contain the summary and impact of the changes. You are importing a file from a different repository, produced by a different company. As part of that import, you are claiming Intel copyright for 2019 for the code provided in the patch. This means that you are making a legal claim to the intellectual property provided by the patch on behalf of Intel. Either: - you have modified the code compared to the original, at which point the commit mesage *must* reflect this - it is no longer the contribution that the original message describes. For an example, see how Laszlo reflected his changes to 94e0dd1afe53. - the Intel copyright addition is a mistake (and must be dropped). Regards, Leif > > Thanks, > Zhichao > > > > > Regards, > > > > Leif > > > > > And I also make some minor changes on it. > > > > > > Thanks, > > > Zhichao > > > > > > > -----Original Message----- > > > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > > > > Sent: Wednesday, May 29, 2019 7:12 PM > > > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > > > > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > > > Michael Turner <Michael.Turner@microsoft.com> > > > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize > > > > the CapsulePei > > > > > > > > On Wed, May 29, 2019 at 08:45:55AM +0800, Gao, Zhichao wrote: > > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > > > If this code is from Microsoft... > > > > > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853 > > > > > > > > > > Sperate the capsule check function from GetCapsuleDescriptors and > > > > > name it to AreCapsulesStaged. > > > > > Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries. > > > > > And optimize its to remove the duplicated code. > > > > > > > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > > > Cc: Hao A Wu <hao.a.wu@intel.com> > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > Cc: Star Zeng <star.zeng@intel.com> > > > > > Cc: Liming Gao <liming.gao@intel.com> > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > Signed-off-by: Zhichao gao <zhichao.gao@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(-) > > > > > > > > > > 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> > > > > > > > > ...why does Intel get the copyright? > > > > > > > > / > > > > Leif > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41753): https://edk2.groups.io/g/devel/message/41753 Mute This Topic: https://groups.io/mt/31828852/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Leif Lindholm > Sent: Saturday, June 1, 2019 12:44 AM > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > Michael Turner <Michael.Turner@microsoft.com> > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize the > CapsulePei > > On Fri, May 31, 2019 at 01:46:14AM +0000, Gao, Zhichao wrote: > > > So what modifications were made to the code on the way from the > > > project Mu repository? That would be useful to mention in the commit > message. > > > > I would add this info blow commit message(not in commit message). It > > is helpful for review. But it may not be useful to add them in the > > commit message. > > On my opinion, the commit message should contain the summary and > impact of the changes. > > You are importing a file from a different repository, produced by a different > company. As part of that import, you are claiming Intel copyright for 2019 for > the code provided in the patch. > > This means that you are making a legal claim to the intellectual property > provided by the patch on behalf of Intel. > Either: > - you have modified the code compared to the original, at which > point the commit mesage *must* reflect this - it is no longer the > contribution that the original message describes. > For an example, see how Laszlo reflected his changes to 94e0dd1afe53. Sorry. I can't understand the example. But maybe I got your point. I would update the commit message with the MU link and mention what changes I made. Then I would update the copyright of Intel. Is that the correct flow? By the way, I have done that in V2 but the link and change info didn't include to the commit message. I would put them into the commit message in next patch. Thanks, Zhichao > - the Intel copyright addition is a mistake (and must be dropped). > > Regards, > > Leif > > > > > Thanks, > > Zhichao > > > > > > > > Regards, > > > > > > Leif > > > > > > > And I also make some minor changes on it. > > > > > > > > Thanks, > > > > Zhichao > > > > > > > > > -----Original Message----- > > > > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > > > > > Sent: Wednesday, May 29, 2019 7:12 PM > > > > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > > > > > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > > > Liming <liming.gao@intel.com>; Sean Brogan > > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > > <Michael.Turner@microsoft.com> > > > > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: > > > > > Optimize the CapsulePei > > > > > > > > > > On Wed, May 29, 2019 at 08:45:55AM +0800, Gao, Zhichao wrote: > > > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > > > > > If this code is from Microsoft... > > > > > > > > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853 > > > > > > > > > > > > Sperate the capsule check function from GetCapsuleDescriptors > > > > > > and name it to AreCapsulesStaged. > > > > > > Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries. > > > > > > And optimize its to remove the duplicated code. > > > > > > > > > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > > > > Cc: Hao A Wu <hao.a.wu@intel.com> > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > > Cc: Star Zeng <star.zeng@intel.com> > > > > > > Cc: Liming Gao <liming.gao@intel.com> > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > Signed-off-by: Zhichao gao <zhichao.gao@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(-) > > > > > > > > > > > > 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> > > > > > > > > > > ...why does Intel get the copyright? > > > > > > > > > > / > > > > > Leif > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41802): https://edk2.groups.io/g/devel/message/41802 Mute This Topic: https://groups.io/mt/31828852/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, Jun 03, 2019 at 08:18:03AM +0000, Gao, Zhichao wrote: > > > > -----Original Message----- > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > > Leif Lindholm > > Sent: Saturday, June 1, 2019 12:44 AM > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming > > <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; > > Michael Turner <Michael.Turner@microsoft.com> > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: Optimize the > > CapsulePei > > > > On Fri, May 31, 2019 at 01:46:14AM +0000, Gao, Zhichao wrote: > > > > So what modifications were made to the code on the way from the > > > > project Mu repository? That would be useful to mention in the commit > > message. > > > > > > I would add this info blow commit message(not in commit message). It > > > is helpful for review. But it may not be useful to add them in the > > > commit message. > > > On my opinion, the commit message should contain the summary and > > impact of the changes. > > > > You are importing a file from a different repository, produced by a different > > company. As part of that import, you are claiming Intel copyright for 2019 for > > the code provided in the patch. > > > > This means that you are making a legal claim to the intellectual property > > provided by the patch on behalf of Intel. > > Either: > > - you have modified the code compared to the original, at which > > point the commit mesage *must* reflect this - it is no longer the > > contribution that the original message describes. > > For an example, see how Laszlo reflected his changes to 94e0dd1afe53. > > Sorry. I can't understand the example. But maybe I got your point. I > would update the commit message with the MU link and mention what > changes I made. Then I would update the copyright of Intel. Is that > the correct flow? Yes, this is correct. Me, Ard and Laszlo tend to follow the format set out in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v5.2-rc3#n468 but there is no official form for this in edk2. The importance is that the information is kept in the commit message, though. > By the way, I have done that in V2 but the link and change info > didn't include to the commit message. I would put them into the > commit message in next patch. Excellent, thank you. Best Regards, Leif > Thanks, > Zhichao > > > - the Intel copyright addition is a mistake (and must be dropped). > > > > Regards, > > > > Leif > > > > > > > > Thanks, > > > Zhichao > > > > > > > > > > > Regards, > > > > > > > > Leif > > > > > > > > > And I also make some minor changes on it. > > > > > > > > > > Thanks, > > > > > Zhichao > > > > > > > > > > > -----Original Message----- > > > > > > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > > > > > > Sent: Wednesday, May 29, 2019 7:12 PM > > > > > > To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com> > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J > > > > > > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray > > > > > > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, > > > > > > Liming <liming.gao@intel.com>; Sean Brogan > > > > > > <sean.brogan@microsoft.com>; Michael Turner > > > > > > <Michael.Turner@microsoft.com> > > > > > > Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/CapsulePei: > > > > > > Optimize the CapsulePei > > > > > > > > > > > > On Wed, May 29, 2019 at 08:45:55AM +0800, Gao, Zhichao wrote: > > > > > > > From: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > > > > > > > If this code is from Microsoft... > > > > > > > > > > > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853 > > > > > > > > > > > > > > Sperate the capsule check function from GetCapsuleDescriptors > > > > > > > and name it to AreCapsulesStaged. > > > > > > > Rename GetCapsuleDescriptors to GetScatterGatherHeadEntries. > > > > > > > And optimize its to remove the duplicated code. > > > > > > > > > > > > > > Cc: Jian J Wang <jian.j.wang@intel.com> > > > > > > > Cc: Hao A Wu <hao.a.wu@intel.com> > > > > > > > Cc: Ray Ni <ray.ni@intel.com> > > > > > > > Cc: Star Zeng <star.zeng@intel.com> > > > > > > > Cc: Liming Gao <liming.gao@intel.com> > > > > > > > Cc: Sean Brogan <sean.brogan@microsoft.com> > > > > > > > Cc: Michael Turner <Michael.Turner@microsoft.com> > > > > > > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com> > > > > > > > Signed-off-by: Zhichao gao <zhichao.gao@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(-) > > > > > > > > > > > > > > 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> > > > > > > > > > > > > ...why does Intel get the copyright? > > > > > > > > > > > > / > > > > > > Leif > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41805): https://edk2.groups.io/g/devel/message/41805 Mute This Topic: https://groups.io/mt/31828852/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----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, Liming;
> Sean Brogan; Michael Turner; Gao, Zhichao
> Subject: [PATCH] MdeModulePkg/CapsulePei: Optimize the CapsulePei
>
> From: Bret Barkelew <Bret.Barkelew@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@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Signed-off-by: Zhichao gao <zhichao.gao@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?
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?
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 (#41576): https://edk2.groups.io/g/devel/message/41576
Mute This Topic: https://groups.io/mt/31828852/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, May 29, 2019 2:55 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean Brogan
> <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@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@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@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@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Michael Turner <Michael.Turner@microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Signed-off-by: Zhichao gao <zhichao.gao@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> -----Original Message-----
> From: Gao, Zhichao
> Sent: Thursday, May 30, 2019 11:21 AM
> To: Wu, Hao A; 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
>
>
>
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Wednesday, May 29, 2019 2:55 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> > <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean Brogan
> > <sean.brogan@microsoft.com>; Michael Turner
> > <Michael.Turner@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@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@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@intel.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Star Zeng <star.zeng@intel.com>
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > Cc: Michael Turner <Michael.Turner@microsoft.com>
> > > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > Signed-off-by: Zhichao gao <zhichao.gao@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.
Understood, thanks for the clarification.
> 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.
Agree.
Best Regards,
Hao Wu
>
> 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 (#41661): https://edk2.groups.io/g/devel/message/41661
Mute This Topic: https://groups.io/mt/31828852/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.