From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: zhichao.gao@intel.com) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by groups.io with SMTP; Sun, 09 Jun 2019 19:09:57 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jun 2019 19:09:55 -0700 X-ExtLoop1: 1 Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga006.jf.intel.com with ESMTP; 09 Jun 2019 19:09:55 -0700 Received: from fmsmsx153.amr.corp.intel.com (10.18.125.6) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 9 Jun 2019 19:09:40 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX153.amr.corp.intel.com (10.18.125.6) with Microsoft SMTP Server (TLS) id 14.3.408.0; Sun, 9 Jun 2019 19:09:39 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.10]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.6]) with mapi id 14.03.0415.000; Mon, 10 Jun 2019 10:09:38 +0800 From: "Gao, Zhichao" To: Leif Lindholm CC: "devel@edk2.groups.io" , "Wang, Jian J" , "Wu, Hao A" , "Ni, Ray" , "Zeng, Star" , "Gao, Liming" , Sean Brogan , "Michael Turner" , Bret Barkelew Subject: Re: [PATCH v4 2/2] MdeMoudlePkg/CapsulePei: Substantial change on UefiCapsule.c Thread-Topic: [PATCH v4 2/2] MdeMoudlePkg/CapsulePei: Substantial change on UefiCapsule.c Thread-Index: AQHVGzxAazPltMIuoE2IKpb7rtn62aaN9nkAgAY1Q+A= Date: Mon, 10 Jun 2019 02:09:37 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B7E50AC@SHSMSX101.ccr.corp.intel.com> References: <20190605011545.18724-1-zhichao.gao@intel.com> <20190605011545.18724-3-zhichao.gao@intel.com> <20190606111812.ihe7jagwxunj25kl@bivouac.eciton.net> In-Reply-To: <20190606111812.ihe7jagwxunj25kl@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: zhichao.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Leif Lindholm [mailto:leif.lindholm@linaro.org] > Sent: Thursday, June 6, 2019 7:18 PM > To: Gao, Zhichao > Cc: devel@edk2.groups.io; Wang, Jian J ; Wu, Hao A > ; Ni, Ray ; Zeng, Star > ; Gao, Liming ; Sean Brogan > ; Michael Turner > ; Bret Barkelew > > Subject: Re: [PATCH v4 2/2] MdeMoudlePkg/CapsulePei: Substantial change > on UefiCapsule.c >=20 > Zhichao, >=20 > Thank you for splitting the patches up. This makes for much better histor= y. > However, >=20 > On Wed, Jun 05, 2019 at 09:15:45AM +0800, Zhichao Gao wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1853 > > > > AreCapsulesStaged do not need to return the status, only boolean > > result is useful. So directly return a boolean value. > > Cannot initialize the variable at its definition. > > > > GetScatterGatherHeadEntries: use allocated buffer instead of fixed > > array to handle the condition which the SG list is larger then the > > array size. > > > > Remove API specifier AreCapsulesStaged and > GetScatterGatherHeadEntries > > because they are internal used. > > > > Fix some coding style issues. >=20 > The above are three or four (is the EFIAPI change a coding style issue or= not?) > unrelated changes. Could you please break this up further? EFIAPI is mentioned in CCS_2_1 spec. So I think it is a coding style issue = too. The other coding style issue is lacking space between the function nam= e and bracket. I would separate 2/2 base on the purpose to make the history clear. Thanks, Zhichao >=20 > Best Regards, >=20 > Leif >=20 > > Cc: Jian J Wang > > Cc: Hao A Wu > > Cc: Ray Ni > > Cc: Star Zeng > > Cc: Liming Gao > > Cc: Sean Brogan > > Cc: Michael Turner > > Cc: Bret Barkelew > > Cc: Leif Lindholm > > Signed-off-by: Zhichao gao > > --- > > .../Universal/CapsulePei/UefiCapsule.c | 103 +++++++++--------- > > 1 file changed, 54 insertions(+), 49 deletions(-) > > > > diff --git a/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > > b/MdeModulePkg/Universal/CapsulePei/UefiCapsule.c > > index 7c8c7a0f45..fabf30926c 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.
> > +Copyright (c) 2006 - 2019, Intel Corporation. All rights > > +reserved.
> > Copyright (c) 2017, AMD Incorporated. All rights reserved.
> > > > 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) > > @@ -793,30 +795,21 @@ BuildMemoryResourceDescriptor ( > > /** > > Check if the capsules are staged. > > > > - @param UpdateCapsules A pointer to return the check result. > > - > > - @retval EFI_INVALID_PARAMETER The parameter is null. > > - @retval EFI_SUCCESS The Capsules are staged. > > + @retval TRUE The capsules are staged. > > + @retval FALSE The capsules are not staged. > > > > **/ > > -EFI_STATUS > > -EFIAPI > > -AreCapsulesStaged( > > - OUT BOOLEAN *UpdateCapsules > > +BOOLEAN > > +AreCapsulesStaged ( > > + VOID > > ) > > { > > EFI_STATUS Status; > > UINTN Size; > > EFI_PEI_READ_ONLY_VARIABLE2_PPI *PPIVariableServices; > > - EFI_PHYSICAL_ADDRESS CapsuleDataPtr64 =3D 0; > > - > > - if (UpdateCapsules =3D=3D NULL) { > > - DEBUG ((DEBUG_ERROR, "%a Invalid parameters. Inputs can't be > NULL\n", __FUNCTION__)); > > - ASSERT (UpdateCapsules !=3D NULL); > > - return EFI_INVALID_PARAMETER; > > - } > > + EFI_PHYSICAL_ADDRESS CapsuleDataPtr64; > > > > - *UpdateCapsules =3D FALSE; > > + CapsuleDataPtr64 =3D 0; > > > > Status =3D PeiServicesLocatePpi( > > &gEfiPeiReadOnlyVariable2PpiGuid, @@ -827,7 +820,7 @@ > > AreCapsulesStaged( > > > > if (EFI_ERROR (Status)) { > > DEBUG ((DEBUG_ERROR, "Failed to find ReadOnlyVariable2PPI\n")); > > - return Status; > > + return FALSE; > > } > > > > // > > @@ -844,14 +837,12 @@ AreCapsulesStaged( > > ); > > > > if (!EFI_ERROR (Status)) { > > - *UpdateCapsules =3D TRUE; > > + return TRUE; > > } > > > > - return EFI_SUCCESS; > > + return FALSE; > > } > > > > -#define MAX_SG_LIST_HEADS (20) > > - > > /** > > Check all the variables for SG list heads and get the count and addr= esses. > > > > @@ -865,23 +856,24 @@ AreCapsulesStaged( > > > > **/ > > EFI_STATUS > > -EFIAPI > > -GetScatterGatherHeadEntries( > > +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_PHYSICAL_ADDRESS TempList[MAX_SG_LIST_HEADS]; > > + 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 =3D 0; > > TempVarName =3D NULL; > > @@ -911,16 +903,26 @@ GetScatterGatherHeadEntries( > > return Status; > > } > > > > + // > > + // Allocate memory for sg list head // TempListLength =3D > > + DEFAULT_SG_LIST_HEADS * sizeof (EFI_PHYSICAL_ADDRESS); TempList =3D > > + AllocateZeroPool (TempListLength); if (TempList =3D=3D NULL) { > > + DEBUG((DEBUG_ERROR, "Failed to allocate memory\n")); > > + return EFI_OUT_OF_RESOURCES; > > + } > > + > > // > > // setup var name buffer for update capsules > > // > > StrCpyS (CapsuleVarName, sizeof (CapsuleVarName) / sizeof (CHAR16), > EFI_CAPSULE_VARIABLE_NAME); > > TempVarName =3D CapsuleVarName + StrLen (CapsuleVarName); > > - while (ValidIndex < MAX_SG_LIST_HEADS) { > > + while (TRUE) { > > if (Index !=3D 0) { > > UnicodeValueToStringS ( > > TempVarName, > > - (sizeof (CapsuleVarName) - ((StrLen (CapsuleVarName) + 1) * si= zeof > (CHAR16))), > > + (sizeof(CapsuleVarName) - ((UINTN)TempVarName - > > + (UINTN)CapsuleVarName)), > > 0, > > Index, > > 0 > > @@ -958,6 +960,17 @@ GetScatterGatherHeadEntries( > > continue; > > } > > > > + // > > + // The TempList is full, enlarge it > > + // > > + if ((ValidIndex + 1) >=3D TempListLength) { > > + EnlargedTempList =3D AllocateZeroPool (TempListLength * 2); > > + CopyMem (EnlargedTempList, TempList, TempListLength); > > + FreePool (TempList); > > + TempList =3D EnlargedTempList; > > + TempListLength *=3D 2; > > + } > > + > > // > > // add it to the cached list > > // > > @@ -1122,19 +1135,11 @@ CheckCapsuleUpdate ( > > IN EFI_PEI_SERVICES **PeiServices > > ) > > { > > - EFI_STATUS Status; > > - BOOLEAN Update; > > - > > - Status =3D AreCapsulesStaged (&Update); > > - > > - if (!EFI_ERROR (Status)) { > > - if (Update) { > > - Status =3D EFI_SUCCESS; > > - } else { > > - Status =3D EFI_NOT_FOUND; > > - } > > + if (AreCapsulesStaged ()) { > > + return EFI_SUCCESS; > > + } else { > > + return EFI_NOT_FOUND; > > } > > - return Status; > > } > > /** > > This function will look at a capsule and determine if it's a test pa= ttern. > > -- > > 2.21.0.windows.1 > >