From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=uI4CLeW3; spf=pass (domain: linaro.org, ip: 209.85.128.65, mailfrom: leif.lindholm@linaro.org) Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by groups.io with SMTP; Thu, 06 Jun 2019 04:18:16 -0700 Received: by mail-wm1-f65.google.com with SMTP id t5so2002372wmh.3 for ; Thu, 06 Jun 2019 04:18:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=qWZZMPySswVndinp42Jx9k1jb2vhfRNwv4MQGt4pUSg=; b=uI4CLeW3LT4QKM3p0ziXk8avTWt3zCjQBH10+xj8/kGquuSKP0wxPPlJJ0UpU3kKY5 tuOnz4G3ZGnXgcDpX5j1LYyHbc2mp7XvlO9d2LShSyocQ7XpcwUfDxog25owhrsuaN5o O1YilsXQuiW94x2P6O88ZiapmLv/Wl+gNYXg2NQyHXv+HvywkQirmDNrAYyq9o2bHH+b ADYy8QgAMhvLuLgf4BBEMx9JogOkIuuVfm4u4fyYIM4MGcrOQTGfBgZ+4XYUP3/6sjB3 pIrtVsRmgsqQNVN6w4zzQEhF3ktdxcsVfawQpbKqdUn8FyEA1ZbHwdLQ/caSb3mTU9MT Pp7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=qWZZMPySswVndinp42Jx9k1jb2vhfRNwv4MQGt4pUSg=; b=b61seO0W4vHNMSS2vBAGirOAJQtSnmsV+DuKAtfy+LeAO+glH1Kj4tNxUrF8kzelPM Dj3XBjZuYMQXkVc4/WNlCY0wumXdFU40FdM45Jx3SyrvUQFzYX5NwKOGMzjQwqub/m4u D1c3jxB6qoP1R4xBqIefIOFyVRpKHR1/YBioyqnrDv2gGryTt4MbwLVP8JIW2dF/TQ1U aHAsxK8DMkL5BC8KmR7vM8Y0UxYMicUaM+iEXWQ5TkfleTzv4Ex+Ol7CICInr7JcdKLz MexECWwX6fS2kr8rpusZZSa7SwT7DirTX7dqDXZ5Ftj4ywO3+cpTDtA7yu+8UHRiYnvA JOcA== X-Gm-Message-State: APjAAAUvX+pZsrTzooeK8Lusz9yRohVE2gyK+/qg6ldXO/p1nDkYD6TX C30bfPyk5B1ix8P1nMrADB+KaQ== X-Google-Smtp-Source: APXvYqyZgfE8ePlomvBPwXwWl8AWKmm6wPptDo0oP9zdr6sE7d3hYtN8qlXDm4CthmpxiSpFmTtwgg== X-Received: by 2002:a1c:ddd6:: with SMTP id u205mr13576149wmg.54.1559819894922; Thu, 06 Jun 2019 04:18:14 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id v2sm1505621wrn.30.2019.06.06.04.18.13 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 06 Jun 2019 04:18:14 -0700 (PDT) Date: Thu, 6 Jun 2019 12:18:12 +0100 From: "Leif Lindholm" To: Zhichao Gao Cc: devel@edk2.groups.io, Jian J Wang , Hao A Wu , Ray Ni , Star Zeng , Liming Gao , Sean Brogan , Michael Turner , Bret Barkelew Subject: Re: [PATCH v4 2/2] MdeMoudlePkg/CapsulePei: Substantial change on UefiCapsule.c Message-ID: <20190606111812.ihe7jagwxunj25kl@bivouac.eciton.net> References: <20190605011545.18724-1-zhichao.gao@intel.com> <20190605011545.18724-3-zhichao.gao@intel.com> MIME-Version: 1.0 In-Reply-To: <20190605011545.18724-3-zhichao.gao@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > 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 = 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 >