From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [PATCH] MdeModulePkg DxeCapsuleLib: Use Attr to know whether reset is required
Date: Tue, 31 Jul 2018 00:45:12 +0000 [thread overview]
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503ACD8219@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <1532940864-29952-1-git-send-email-star.zeng@intel.com>
Reviewed-by: Jiewen.yao@intel.com
> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, July 30, 2018 4:54 PM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH] MdeModulePkg DxeCapsuleLib: Use Attr to know whether
> reset is required
>
> Current DxeCapsuleLibFmp always do reset for FMP capsule.
> Actually, the code should use Attributes from FMP descriptor to know
> whether reset is required or not.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
> .../Library/DxeCapsuleLibFmp/DxeCapsuleLib.c | 134
> ++++++++++++++++-----
> .../DxeCapsuleLibFmp/DxeCapsuleProcessLib.c | 40 ++++--
> 2 files changed, 134 insertions(+), 40 deletions(-)
>
> diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> index e88dab8c7642..d00d7d88ff7d 100644
> --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> @@ -620,11 +620,14 @@ DumpAllFmpInfo (
>
> @param[in] UpdateImageTypeId Used to identify device
> firmware targeted by this update.
> @param[in] UpdateHardwareInstance The HardwareInstance to
> target with this update.
> - @param[in,out] NoHandles The number of handles
> returned in Buffer.
> - @param[out] Buffer[out] A pointer to the buffer to return
> the requested array of handles.
> -
> - @retval EFI_SUCCESS The array of handles was returned in
> Buffer, and the number of
> - handles in Buffer was returned in
> NoHandles.
> + @param[out] NoHandles The number of handles
> returned in HandleBuf.
> + @param[out] HandleBuf A pointer to the buffer to
> return the requested array of handles.
> + @param[out] ResetRequiredBuf A pointer to the buffer to
> return reset required flag for
> + the requested array of
> handles.
> +
> + @retval EFI_SUCCESS The array of handles and their reset
> required flag were returned in
> + HandleBuf and ResetRequiredBuf, and
> the number of handles in HandleBuf
> + was returned in NoHandles.
> @retval EFI_NOT_FOUND No handles match the search.
> @retval EFI_OUT_OF_RESOURCES There is not enough pool memory to
> store the matching results.
> **/
> @@ -632,14 +635,16 @@ EFI_STATUS
> GetFmpHandleBufferByType (
> IN EFI_GUID *UpdateImageTypeId,
> IN UINT64 UpdateHardwareInstance,
> - IN OUT UINTN *NoHandles,
> - OUT EFI_HANDLE **Buffer
> + OUT UINTN *NoHandles, OPTIONAL
> + OUT EFI_HANDLE **HandleBuf, OPTIONAL
> + OUT BOOLEAN **ResetRequiredBuf
> OPTIONAL
> )
> {
> EFI_STATUS Status;
> EFI_HANDLE *HandleBuffer;
> UINTN NumberOfHandles;
> EFI_HANDLE
> *MatchedHandleBuffer;
> + BOOLEAN
> *MatchedResetRequiredBuffer;
> UINTN
> MatchedNumberOfHandles;
> EFI_FIRMWARE_MANAGEMENT_PROTOCOL *Fmp;
> UINTN Index;
> @@ -653,8 +658,15 @@ GetFmpHandleBufferByType (
> UINTN Index2;
> EFI_FIRMWARE_IMAGE_DESCRIPTOR
> *TempFmpImageInfo;
>
> - *NoHandles = 0;
> - *Buffer = NULL;
> + if (NoHandles != NULL) {
> + *NoHandles = 0;
> + }
> + if (HandleBuf != NULL) {
> + *HandleBuf = NULL;
> + }
> + if (ResetRequiredBuf != NULL) {
> + *ResetRequiredBuf = NULL;
> + }
>
> Status = gBS->LocateHandleBuffer (
> ByProtocol,
> @@ -668,10 +680,26 @@ GetFmpHandleBufferByType (
> }
>
> MatchedNumberOfHandles = 0;
> - MatchedHandleBuffer = AllocateZeroPool (sizeof(EFI_HANDLE) *
> NumberOfHandles);
> - if (MatchedHandleBuffer == NULL) {
> - FreePool (HandleBuffer);
> - return EFI_OUT_OF_RESOURCES;
> +
> + MatchedHandleBuffer = NULL;
> + if (HandleBuf != NULL) {
> + MatchedHandleBuffer = AllocateZeroPool (sizeof(EFI_HANDLE) *
> NumberOfHandles);
> + if (MatchedHandleBuffer == NULL) {
> + FreePool (HandleBuffer);
> + return EFI_OUT_OF_RESOURCES;
> + }
> + }
> +
> + MatchedResetRequiredBuffer = NULL;
> + if (ResetRequiredBuf != NULL) {
> + MatchedResetRequiredBuffer = AllocateZeroPool (sizeof(BOOLEAN) *
> NumberOfHandles);
> + if (MatchedResetRequiredBuffer == NULL) {
> + if (MatchedHandleBuffer != NULL) {
> + FreePool (MatchedHandleBuffer);
> + }
> + FreePool (HandleBuffer);
> + return EFI_OUT_OF_RESOURCES;
> + }
> }
>
> for (Index = 0; Index < NumberOfHandles; Index++) {
> @@ -733,7 +761,15 @@ GetFmpHandleBufferByType (
> if ((UpdateHardwareInstance == 0) ||
> ((FmpImageInfoDescriptorVer >=
> EFI_FIRMWARE_IMAGE_DESCRIPTOR_VERSION) &&
> (UpdateHardwareInstance ==
> TempFmpImageInfo->HardwareInstance))) {
> - MatchedHandleBuffer[MatchedNumberOfHandles] =
> HandleBuffer[Index];
> + if (MatchedHandleBuffer != NULL) {
> + MatchedHandleBuffer[MatchedNumberOfHandles] =
> HandleBuffer[Index];
> + }
> + if (MatchedResetRequiredBuffer != NULL) {
> + MatchedResetRequiredBuffer[MatchedNumberOfHandles] =
> (((TempFmpImageInfo->AttributesSupported &
> +
> IMAGE_ATTRIBUTE_RESET_REQUIRED) != 0) &&
> +
> ((TempFmpImageInfo->AttributesSetting &
> +
> IMAGE_ATTRIBUTE_RESET_REQUIRED) != 0));
> + }
> MatchedNumberOfHandles++;
> break;
> }
> @@ -749,8 +785,15 @@ GetFmpHandleBufferByType (
> return EFI_NOT_FOUND;
> }
>
> - *NoHandles = MatchedNumberOfHandles;
> - *Buffer = MatchedHandleBuffer;
> + if (NoHandles != NULL) {
> + *NoHandles = MatchedNumberOfHandles;
> + }
> + if (HandleBuf != NULL) {
> + *HandleBuf = MatchedHandleBuffer;
> + }
> + if (ResetRequiredBuf != NULL) {
> + *ResetRequiredBuf = MatchedResetRequiredBuffer;
> + }
>
> return EFI_SUCCESS;
> }
> @@ -1078,7 +1121,8 @@ RecordFmpCapsuleStatus (
>
> This function need support nested FMP capsule.
>
> - @param[in] CapsuleHeader Points to a capsule header.
> + @param[in] CapsuleHeader Points to a capsule header.
> + @param[out] ResetRequired Indicates whether reset is required
> or not.
>
> @retval EFI_SUCESS Process Capsule Image successfully.
> @retval EFI_UNSUPPORTED Capsule image is not supported by the
> firmware.
> @@ -1088,7 +1132,8 @@ RecordFmpCapsuleStatus (
> **/
> EFI_STATUS
> ProcessFmpCapsuleImage (
> - IN EFI_CAPSULE_HEADER *CapsuleHeader
> + IN EFI_CAPSULE_HEADER *CapsuleHeader,
> + OUT BOOLEAN *ResetRequired OPTIONAL
> )
> {
> EFI_STATUS Status;
> @@ -1098,6 +1143,7 @@ ProcessFmpCapsuleImage (
> UINT32 ItemNum;
> UINTN Index;
> EFI_HANDLE *HandleBuffer;
> + BOOLEAN
> *ResetRequiredBuffer;
> UINTN NumberOfHandles;
> UINTN DriverLen;
> UINT64
> UpdateHardwareInstance;
> @@ -1106,7 +1152,7 @@ ProcessFmpCapsuleImage (
> BOOLEAN Abort;
>
> if (!IsFmpCapsuleGuid(&CapsuleHeader->CapsuleGuid)) {
> - return ProcessFmpCapsuleImage ((EFI_CAPSULE_HEADER
> *)((UINTN)CapsuleHeader + CapsuleHeader->HeaderSize));
> + return ProcessFmpCapsuleImage ((EFI_CAPSULE_HEADER
> *)((UINTN)CapsuleHeader + CapsuleHeader->HeaderSize), ResetRequired);
> }
>
> NotReady = FALSE;
> @@ -1176,7 +1222,8 @@ ProcessFmpCapsuleImage (
> &ImageHeader->UpdateImageTypeId,
> UpdateHardwareInstance,
> &NumberOfHandles,
> - &HandleBuffer
> + &HandleBuffer,
> + &ResetRequiredBuffer
> );
> if (EFI_ERROR(Status)) {
> NotReady = TRUE;
> @@ -1209,6 +1256,10 @@ ProcessFmpCapsuleImage (
> );
> if (Status != EFI_SUCCESS) {
> Abort = TRUE;
> + } else {
> + if (ResetRequired != NULL) {
> + *ResetRequired |= ResetRequiredBuffer[Index2];
> + }
> }
>
> RecordFmpCapsuleStatus (
> @@ -1222,6 +1273,9 @@ ProcessFmpCapsuleImage (
> if (HandleBuffer != NULL) {
> FreePool(HandleBuffer);
> }
> + if (ResetRequiredBuffer != NULL) {
> + FreePool(ResetRequiredBuffer);
> + }
> }
>
> if (NotReady) {
> @@ -1256,8 +1310,6 @@ IsNestedFmpCapsule (
> UINTN NestedCapsuleSize;
> ESRT_MANAGEMENT_PROTOCOL *EsrtProtocol;
> EFI_SYSTEM_RESOURCE_ENTRY Entry;
> - EFI_HANDLE *HandleBuffer;
> - UINTN NumberOfHandles;
>
> EsrtGuidFound = FALSE;
> if (mIsVirtualAddrConverted) {
> @@ -1286,19 +1338,16 @@ IsNestedFmpCapsule (
> // Check Firmware Management Protocols
> //
> if (!EsrtGuidFound) {
> - HandleBuffer = NULL;
> Status = GetFmpHandleBufferByType (
> &CapsuleHeader->CapsuleGuid,
> 0,
> - &NumberOfHandles,
> - &HandleBuffer
> + NULL,
> + NULL,
> + NULL
> );
> if (!EFI_ERROR(Status)) {
> EsrtGuidFound = TRUE;
> }
> - if (HandleBuffer != NULL) {
> - FreePool (HandleBuffer);
> - }
> }
> }
> if (!EsrtGuidFound) {
> @@ -1386,6 +1435,7 @@ SupportCapsuleImage (
> Caution: This function may receive untrusted input.
>
> @param[in] CapsuleHeader Points to a capsule header.
> + @param[out] ResetRequired Indicates whether reset is required
> or not.
>
> @retval EFI_SUCESS Process Capsule Image successfully.
> @retval EFI_UNSUPPORTED Capsule image is not supported by the
> firmware.
> @@ -1394,8 +1444,9 @@ SupportCapsuleImage (
> **/
> EFI_STATUS
> EFIAPI
> -ProcessCapsuleImage (
> - IN EFI_CAPSULE_HEADER *CapsuleHeader
> +ProcessThisCapsuleImage (
> + IN EFI_CAPSULE_HEADER *CapsuleHeader,
> + OUT BOOLEAN *ResetRequired OPTIONAL
> )
> {
> EFI_STATUS Status;
> @@ -1432,7 +1483,7 @@ ProcessCapsuleImage (
> // Process EFI FMP Capsule
> //
> DEBUG((DEBUG_INFO, "ProcessFmpCapsuleImage ...\n"));
> - Status = ProcessFmpCapsuleImage(CapsuleHeader);
> + Status = ProcessFmpCapsuleImage(CapsuleHeader, ResetRequired);
> DEBUG((DEBUG_INFO, "ProcessFmpCapsuleImage - %r\n", Status));
>
> return Status;
> @@ -1442,6 +1493,27 @@ ProcessCapsuleImage (
> }
>
> /**
> + The firmware implements to process the capsule image.
> +
> + Caution: This function may receive untrusted input.
> +
> + @param[in] CapsuleHeader Points to a capsule header.
> +
> + @retval EFI_SUCESS Process Capsule Image successfully.
> + @retval EFI_UNSUPPORTED Capsule image is not supported by the
> firmware.
> + @retval EFI_VOLUME_CORRUPTED FV volume in the capsule is
> corrupted.
> + @retval EFI_OUT_OF_RESOURCES Not enough memory.
> +**/
> +EFI_STATUS
> +EFIAPI
> +ProcessCapsuleImage (
> + IN EFI_CAPSULE_HEADER *CapsuleHeader
> + )
> +{
> + return ProcessThisCapsuleImage (CapsuleHeader, NULL);
> +}
> +
> +/**
> Callback function executed when the EndOfDxe event group is signaled.
>
> @param[in] Event Event whose notification function is being
> invoked.
> diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> index 26ca4e295f20..176dea196026 100644
> --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c
> @@ -99,13 +99,33 @@ IsValidCapsuleHeader (
> );
>
> extern BOOLEAN mDxeCapsuleLibEndOfDxe;
> -BOOLEAN mNeedReset;
> +BOOLEAN mNeedReset = FALSE;
>
> VOID **mCapsulePtr;
> EFI_STATUS *mCapsuleStatusArray;
> UINT32 mCapsuleTotalNumber;
>
> /**
> + The firmware implements to process the capsule image.
> +
> + Caution: This function may receive untrusted input.
> +
> + @param[in] CapsuleHeader Points to a capsule header.
> + @param[out] ResetRequired Indicates whether reset is required
> or not.
> +
> + @retval EFI_SUCESS Process Capsule Image successfully.
> + @retval EFI_UNSUPPORTED Capsule image is not supported by the
> firmware.
> + @retval EFI_VOLUME_CORRUPTED FV volume in the capsule is
> corrupted.
> + @retval EFI_OUT_OF_RESOURCES Not enough memory.
> +**/
> +EFI_STATUS
> +EFIAPI
> +ProcessThisCapsuleImage (
> + IN EFI_CAPSULE_HEADER *CapsuleHeader,
> + OUT BOOLEAN *ResetRequired OPTIONAL
> + );
> +
> +/**
> Function indicate the current completion progress of the firmware
> update. Platform may override with own specific progress function.
>
> @@ -381,6 +401,7 @@ ProcessTheseCapsules (
> UINT32 Index;
> ESRT_MANAGEMENT_PROTOCOL *EsrtManagement;
> UINT16 EmbeddedDriverCount;
> + BOOLEAN ResetRequired;
>
> REPORT_STATUS_CODE(EFI_PROGRESS_CODE, (EFI_SOFTWARE |
> PcdGet32(PcdStatusCodeSubClassCapsule) |
> PcdGet32(PcdCapsuleStatusCodeProcessCapsulesBegin)));
>
> @@ -416,11 +437,11 @@ ProcessTheseCapsules (
> for (Index = 0; Index < mCapsuleTotalNumber; Index++) {
> CapsuleHeader = (EFI_CAPSULE_HEADER*) mCapsulePtr [Index];
> if (CompareGuid (&CapsuleHeader->CapsuleGuid,
> &gWindowsUxCapsuleGuid)) {
> - DEBUG ((DEBUG_INFO, "ProcessCapsuleImage (Ux) - 0x%x\n",
> CapsuleHeader));
> + DEBUG ((DEBUG_INFO, "ProcessThisCapsuleImage (Ux) - 0x%x\n",
> CapsuleHeader));
> DEBUG ((DEBUG_INFO, "Display logo capsule is found.\n"));
> - Status = ProcessCapsuleImage (CapsuleHeader);
> + Status = ProcessThisCapsuleImage (CapsuleHeader, NULL);
> mCapsuleStatusArray [Index] = EFI_SUCCESS;
> - DEBUG((DEBUG_INFO, "ProcessCapsuleImage (Ux) - %r\n", Status));
> + DEBUG((DEBUG_INFO, "ProcessThisCapsuleImage (Ux) - %r\n",
> Status));
> break;
> }
> }
> @@ -454,10 +475,11 @@ ProcessTheseCapsules (
> }
>
> if ((!FirstRound) || (EmbeddedDriverCount == 0)) {
> - DEBUG((DEBUG_INFO, "ProcessCapsuleImage - 0x%x\n",
> CapsuleHeader));
> - Status = ProcessCapsuleImage (CapsuleHeader);
> + DEBUG((DEBUG_INFO, "ProcessThisCapsuleImage - 0x%x\n",
> CapsuleHeader));
> + ResetRequired = FALSE;
> + Status = ProcessThisCapsuleImage (CapsuleHeader, &ResetRequired);
> mCapsuleStatusArray [Index] = Status;
> - DEBUG((DEBUG_INFO, "ProcessCapsuleImage - %r\n", Status));
> + DEBUG((DEBUG_INFO, "ProcessThisCapsuleImage - %r\n", Status));
>
> if (Status != EFI_NOT_READY) {
> if (EFI_ERROR(Status)) {
> @@ -467,8 +489,8 @@ ProcessTheseCapsules (
> REPORT_STATUS_CODE(EFI_PROGRESS_CODE, (EFI_SOFTWARE
> | PcdGet32(PcdStatusCodeSubClassCapsule) |
> PcdGet32(PcdCapsuleStatusCodeUpdateFirmwareSuccess)));
> }
>
> - if ((CapsuleHeader->Flags &
> PcdGet16(PcdSystemRebootAfterCapsuleProcessFlag)) != 0 ||
> - IsFmpCapsule(CapsuleHeader)) {
> + mNeedReset |= ResetRequired;
> + if ((CapsuleHeader->Flags &
> PcdGet16(PcdSystemRebootAfterCapsuleProcessFlag)) != 0) {
> mNeedReset = TRUE;
> }
> }
> --
> 2.7.0.windows.1
prev parent reply other threads:[~2018-07-31 0:45 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-30 8:54 [PATCH] MdeModulePkg DxeCapsuleLib: Use Attr to know whether reset is required Star Zeng
2018-07-31 0:45 ` Yao, Jiewen [this message]
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=74D8A39837DF1E4DA445A8C0B3885C503ACD8219@shsmsx102.ccr.corp.intel.com \
--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