From: "Leif Lindholm" <leif.lindholm@linaro.org>
To: Zhichao Gao <zhichao.gao@intel.com>
Cc: devel@edk2.groups.io, Jian J Wang <jian.j.wang@intel.com>,
Hao A Wu <hao.a.wu@intel.com>, Ray Ni <ray.ni@intel.com>,
Star Zeng <star.zeng@intel.com>,
Liming Gao <liming.gao@intel.com>,
Sean Brogan <sean.brogan@microsoft.com>,
Michael Turner <Michael.Turner@microsoft.com>,
Bret Barkelew <Bret.Barkelew@microsoft.com>
Subject: Re: [PATCH v4 2/2] MdeMoudlePkg/CapsulePei: Substantial change on UefiCapsule.c
Date: Thu, 6 Jun 2019 12:18:12 +0100 [thread overview]
Message-ID: <20190606111812.ihe7jagwxunj25kl@bivouac.eciton.net> (raw)
In-Reply-To: <20190605011545.18724-3-zhichao.gao@intel.com>
Zhichao,
Thank you for splitting the patches up. This makes for much better history.
However,
On Wed, Jun 05, 2019 at 09:15:45AM +0800, Zhichao Gao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1853
>
> 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.
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?
Best Regards,
Leif
> 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>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Signed-off-by: Zhichao gao <zhichao.gao@intel.com>
> ---
> .../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.<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)
> @@ -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 = 0;
> -
> - if (UpdateCapsules == NULL) {
> - DEBUG ((DEBUG_ERROR, "%a Invalid parameters. Inputs can't be NULL\n", __FUNCTION__));
> - ASSERT (UpdateCapsules != NULL);
> - return EFI_INVALID_PARAMETER;
> - }
> + EFI_PHYSICAL_ADDRESS CapsuleDataPtr64;
>
> - *UpdateCapsules = FALSE;
> + CapsuleDataPtr64 = 0;
>
> Status = 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 = 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 addresses.
>
> @@ -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 = 0;
> TempVarName = NULL;
> @@ -911,16 +903,26 @@ GetScatterGatherHeadEntries(
> 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 (ValidIndex < MAX_SG_LIST_HEADS) {
> + while (TRUE) {
> if (Index != 0) {
> UnicodeValueToStringS (
> TempVarName,
> - (sizeof (CapsuleVarName) - ((StrLen (CapsuleVarName) + 1) * sizeof (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) >= TempListLength) {
> + EnlargedTempList = AllocateZeroPool (TempListLength * 2);
> + CopyMem (EnlargedTempList, TempList, TempListLength);
> + FreePool (TempList);
> + TempList = EnlargedTempList;
> + TempListLength *= 2;
> + }
> +
> //
> // add it to the cached list
> //
> @@ -1122,19 +1135,11 @@ CheckCapsuleUpdate (
> IN EFI_PEI_SERVICES **PeiServices
> )
> {
> - EFI_STATUS Status;
> - BOOLEAN Update;
> -
> - Status = AreCapsulesStaged (&Update);
> -
> - if (!EFI_ERROR (Status)) {
> - if (Update) {
> - Status = EFI_SUCCESS;
> - } else {
> - Status = 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 pattern.
> --
> 2.21.0.windows.1
>
next prev parent reply other threads:[~2019-06-06 11:18 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-05 1:15 [PATCH v4 0/2] MdeModulePkg/CapsulePei: Optimize the CapsulePei Gao, Zhichao
2019-06-05 1:15 ` [PATCH v4 1/2] " Gao, Zhichao
2019-06-05 1:15 ` [PATCH v4 2/2] MdeMoudlePkg/CapsulePei: Substantial change on UefiCapsule.c Gao, Zhichao
2019-06-06 11:18 ` Leif Lindholm [this message]
2019-06-10 2:09 ` Gao, Zhichao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190606111812.ihe7jagwxunj25kl@bivouac.eciton.net \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox