From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::236; helo=mail-io0-x236.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x236.google.com (mail-io0-x236.google.com [IPv6:2607:f8b0:4001:c06::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BE30420D7B254 for ; Thu, 31 May 2018 06:03:57 -0700 (PDT) Received: by mail-io0-x236.google.com with SMTP id t5-v6so13079466ioa.8 for ; Thu, 31 May 2018 06:03:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=qYzcOPGz/LEdHC+/vLGb1HtYWuGiBtpVl9QPGoBLFq0=; b=g+imTvBjp8qcH6RfzIDleWjJK3JOzsvrtn/fKPUUCdDn5fgtezrle/p1kPHKzU1ebl TG+YHf7oaDAeGmTJOztjssK8JsVkvJHz9ckS/+nMkG0nz4neuY0f/rynGWN49ZTncXxX r9/vfFCN4eVfYnWCfvbSl4zVzj49QcO95q9Eo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=qYzcOPGz/LEdHC+/vLGb1HtYWuGiBtpVl9QPGoBLFq0=; b=lcLLNu6cVfhvMikOiBcLrjOnBohMz+4nX4rLiSaLKsf8+huYJkfLgiwMYr84pG16KT uiu0bUZL4Q4cVSl10zOrA83kdpuAjSxABnxH4pz3vzxT2JLA+rXlXWCDnBUBfymqmFX+ NamyeQrlztpmMISLqAJ5eqNtxgFwj0F1kX3qac3VSC5e1OlrmoGD5dCpYC3ZhR1W21Hm XCt+YZKws6J4iscev1/CWk7NtwlnCcJjXQJZYBcYhm3mm4o/NzzC7/4dJArgqqGPtcbq OjMndx2X9yxDkz32ygEW/2G0F/QF15NcFAdDQs+SLVS9nX+VBr94MHIYas8dEhyFdBR9 fPmA== X-Gm-Message-State: ALKqPweKdydULoFNzvIONoECMR8JPeGcwZG/i+y5/X0srCUnRiMKnnP6 SYTV4zskj/r6S9x9hXVC7Uajh8f4+xpWIbYHaA3Tvw== X-Google-Smtp-Source: ADUXVKIYSeVJHjoOo9AGm20xqI1OdaYWxnXj5Gca3iL1g/EHpXzCJDnm8X17s3i77jvY2wKPnPeK6HpVTkJOcyoSbN8= X-Received: by 2002:a6b:268b:: with SMTP id m133-v6mr6603097iom.107.1527771832325; Thu, 31 May 2018 06:03:52 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 31 May 2018 06:03:51 -0700 (PDT) In-Reply-To: <20180529161755.9424-2-michael.d.kinney@intel.com> References: <20180529161755.9424-1-michael.d.kinney@intel.com> <20180529161755.9424-2-michael.d.kinney@intel.com> From: Ard Biesheuvel Date: Thu, 31 May 2018 15:03:51 +0200 Message-ID: To: Michael D Kinney , Leif Lindholm Cc: "edk2-devel@lists.01.org" , Jiewen Yao , Eric Dong , Star Zeng Subject: Re: [Patch v3 1/2] MdeModulePkg/DxeCapsuleLibFmp: Add progress bar support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 31 May 2018 13:03:58 -0000 Content-Type: text/plain; charset="UTF-8" On 29 May 2018 at 18:17, Michael D Kinney wrote: > From: "Kinney, Michael D" > > https://bugzilla.tianocore.org/show_bug.cgi?id=801 > > Based on content from the following branch/commits: > https://github.com/Microsoft/MS_UEFI/tree/share/MsCapsuleSupport > > * Change Update_Image_Progress() to UpdateImageProcess() > * Call DisplayUpdateProgressLib from UpdateImageProgress(). > * Split out a boot service and runtime version of > UpdateImageProgress() so the DisplayUpdateProgressLib is > not used at runtime. > * If gEdkiiFirmwareManagementProgressProtocolGuid is present, > then use its progress bar color and watchdog timer value. > * If gEdkiiFirmwareManagementProgressProtocolGuid is not present, > then use default progress bar color and 5 min watchdog timer. > * Remove Print() calls during capsule processing. Instead, > the DisplayUpdateProgressLib is used to inform the user > of progress during a capsule update. > > Cc: Star Zeng > Cc: Eric Dong > Cc: Jiewen Yao > Cc: Sean Brogan > Signed-off-by: Michael D Kinney > Contributed-under: TianoCore Contribution Agreement 1.1 Mike, I am trying to wire up this code for my platform, and I am having trouble understanding how the pieces fit together. I have merged your edk2 and edk2-platforms changes, and I have updated my PerformFlashWriteWithProgress() implementation to invoke the Progress() callback. Unfortunately, nothing happens when I run it. Looking into the code in more detail, I am having trouble understanding how this is ever supposed to work, given that: - UpdateCapsule() is implemented by CapsuleRuntimeDxe, which is a DXE_RUNTIME_DRIVER module - CapsuleRuntimeDxe incorporates DxeRuntimeCapsuleLib.inf - DxeRuntimeCapsuleLib.inf includes DxeCapsuleReportLibNull.c, and so its SetFmpImageData() routine passes an empty implementation of UpdateImageProgress() into Fmp->SetImage() It seems to me that some pieces are missing, but the code is a bit scattered so maybe I failed to incorporate a piece. Thanks, Ard. > --- > .../Library/DxeCapsuleLibFmp/DxeCapsuleLib.c | 47 +++++++++--- > .../Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf | 8 ++- > .../DxeCapsuleLibFmp/DxeCapsuleProcessLib.c | 84 ++++++++++++++++------ > .../DxeCapsuleLibFmp/DxeCapsuleProcessLibNull.c | 21 +++++- > .../DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf | 7 +- > 5 files changed, 131 insertions(+), 36 deletions(-) > > diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c > index 05fcd92deb..f0226eafa5 100644 > --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c > +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c > @@ -45,6 +45,7 @@ > #include > #include > #include > +#include > #include > > EFI_SYSTEM_RESOURCE_TABLE *mEsrtTable = NULL; > @@ -53,6 +54,8 @@ BOOLEAN mIsVirtualAddrConverted = FALSE; > BOOLEAN mDxeCapsuleLibEndOfDxe = FALSE; > EFI_EVENT mDxeCapsuleLibEndOfDxeEvent = NULL; > > +EDKII_FIRMWARE_MANAGEMENT_PROGRESS_PROTOCOL *mFmpProgress = NULL; > + > /** > Initialize capsule related variables. > **/ > @@ -101,18 +104,17 @@ RecordFmpCapsuleStatusVariable ( > Function indicate the current completion progress of the firmware > update. Platform may override with own specific progress function. > > - @param[in] Completion A value between 1 and 100 indicating the current completion progress of the firmware update > + @param[in] Completion A value between 1 and 100 indicating the current > + completion progress of the firmware update > > - @retval EFI_SUCESS Input capsule is a correct FMP capsule. > + @retval EFI_SUCESS The capsule update progress was updated. > + @retval EFI_INVALID_PARAMETER Completion is greater than 100%. > **/ > EFI_STATUS > EFIAPI > -Update_Image_Progress ( > +UpdateImageProgress ( > IN UINTN Completion > - ) > -{ > - return EFI_SUCCESS; > -} > + ); > > /** > Return if this CapsuleGuid is a FMP capsule GUID or not. > @@ -849,6 +851,19 @@ SetFmpImageData ( > return Status; > } > > + // > + // Lookup Firmware Management Progress Protocol before SetImage() is called > + // This is an optional protocol that may not be present on Handle. > + // > + Status = gBS->HandleProtocol ( > + Handle, > + &gEdkiiFirmwareManagementProgressProtocolGuid, > + (VOID **)&mFmpProgress > + ); > + if (EFI_ERROR (Status)) { > + mFmpProgress = NULL; > + } > + > if (ImageHeader->Version >= EFI_FIRMWARE_MANAGEMENT_CAPSULE_IMAGE_HEADER_INIT_VERSION) { > Image = (UINT8 *)(ImageHeader + 1); > } else { > @@ -873,21 +888,37 @@ SetFmpImageData ( > DEBUG((DEBUG_INFO, "(UpdateHardwareInstance - 0x%x)", ImageHeader->UpdateHardwareInstance)); > } > DEBUG((DEBUG_INFO, "\n")); > + > + // > + // Before calling SetImage(), reset the progress bar to 0% > + // > + UpdateImageProgress (0); > + > Status = Fmp->SetImage( > Fmp, > ImageHeader->UpdateImageIndex, // ImageIndex > Image, // Image > ImageHeader->UpdateImageSize, // ImageSize > VendorCode, // VendorCode > - Update_Image_Progress, // Progress > + UpdateImageProgress, // Progress > &AbortReason // AbortReason > ); > + // > + // Set the progress bar to 100% after returning from SetImage() > + // > + UpdateImageProgress (100); > + > DEBUG((DEBUG_INFO, "Fmp->SetImage - %r\n", Status)); > if (AbortReason != NULL) { > DEBUG ((DEBUG_ERROR, "%s\n", AbortReason)); > FreePool(AbortReason); > } > > + // > + // Clear mFmpProgress after SetImage() returns > + // > + mFmpProgress = NULL; > + > return Status; > } > > diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf > index 1d947101d3..8367264f76 100644 > --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf > +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf > @@ -52,6 +52,7 @@ [LibraryClasses] > PrintLib > HobLib > BmpSupportLib > + DisplayUpdateProgressLib > > [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleMax ## CONSUMES > @@ -66,9 +67,10 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleStatusCodeResettingSystem ## CONSUMES > > [Protocols] > - gEsrtManagementProtocolGuid ## CONSUMES > - gEfiFirmwareManagementProtocolGuid ## CONSUMES > - gEdkiiVariableLockProtocolGuid ## SOMETIMES_CONSUMES > + gEsrtManagementProtocolGuid ## CONSUMES > + gEfiFirmwareManagementProtocolGuid ## CONSUMES > + gEdkiiVariableLockProtocolGuid ## SOMETIMES_CONSUMES > + gEdkiiFirmwareManagementProgressProtocolGuid ## SOMETIMES_CONSUMES > > [Guids] > gEfiFmpCapsuleGuid ## SOMETIMES_CONSUMES ## GUID > diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c > index ba3ff90b9f..26ca4e295f 100644 > --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c > +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLib.c > @@ -9,7 +9,7 @@ > ProcessCapsules(), ProcessTheseCapsules() will receive untrusted > input and do basic validation. > > - Copyright (c) 2016, Intel Corporation. All rights reserved.
> + Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -22,6 +22,7 @@ > > #include > #include > +#include > > #include > #include > @@ -34,9 +35,12 @@ > #include > #include > #include > +#include > > #include > > +extern EDKII_FIRMWARE_MANAGEMENT_PROGRESS_PROTOCOL *mFmpProgress; > + > /** > Return if this FMP is a system FMP or a device FMP, based upon CapsuleHeader. > > @@ -101,6 +105,62 @@ VOID **mCapsulePtr; > EFI_STATUS *mCapsuleStatusArray; > UINT32 mCapsuleTotalNumber; > > +/** > + Function indicate the current completion progress of the firmware > + update. Platform may override with own specific progress function. > + > + @param[in] Completion A value between 1 and 100 indicating the current > + completion progress of the firmware update > + > + @retval EFI_SUCESS The capsule update progress was updated. > + @retval EFI_INVALID_PARAMETER Completion is greater than 100%. > +**/ > +EFI_STATUS > +EFIAPI > +UpdateImageProgress ( > + IN UINTN Completion > + ) > +{ > + EFI_STATUS Status; > + UINTN Seconds; > + EFI_GRAPHICS_OUTPUT_BLT_PIXEL_UNION *Color; > + > + DEBUG((DEBUG_INFO, "Update Progress - %d%%\n", Completion)); > + > + if (Completion > 100) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // Use a default timeout of 5 minutes if there is not FMP Progress Protocol. > + // > + Seconds = 5 * 60; > + Color = NULL; > + if (mFmpProgress != NULL) { > + Seconds = mFmpProgress->WatchdogSeconds; > + Color = &mFmpProgress->ProgressBarForegroundColor; > + } > + > + // > + // Cancel the watchdog timer > + // > + gBS->SetWatchdogTimer (0, 0x0000, 0, NULL); > + > + if (Completion != 100) { > + // > + // Arm the watchdog timer from PCD setting > + // > + if (Seconds != 0) { > + DEBUG ((DEBUG_VERBOSE, "Arm watchdog timer %d seconds\n", Seconds)); > + gBS->SetWatchdogTimer (Seconds, 0x0000, 0, NULL); > + } > + } > + > + Status = DisplayUpdateProgress (Completion, Color); > + > + return Status; > +} > + > /** > This function initializes the mCapsulePtr, mCapsuleStatusArray and mCapsuleTotalNumber. > **/ > @@ -319,7 +379,6 @@ ProcessTheseCapsules ( > EFI_STATUS Status; > EFI_CAPSULE_HEADER *CapsuleHeader; > UINT32 Index; > - BOOLEAN DisplayCapsuleExist; > ESRT_MANAGEMENT_PROTOCOL *EsrtManagement; > UINT16 EmbeddedDriverCount; > > @@ -354,12 +413,10 @@ ProcessTheseCapsules ( > // > // If Windows UX capsule exist, process it first > // > - DisplayCapsuleExist = FALSE; > 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)); > - DisplayCapsuleExist = TRUE; > DEBUG ((DEBUG_INFO, "Display logo capsule is found.\n")); > Status = ProcessCapsuleImage (CapsuleHeader); > mCapsuleStatusArray [Index] = EFI_SUCCESS; > @@ -368,12 +425,7 @@ ProcessTheseCapsules ( > } > } > > - if (!DisplayCapsuleExist) { > - // > - // Display Capsule not found. Display the default string. > - // > - Print (L"Updating the firmware ......\r\n"); > - } > + DEBUG ((DEBUG_INFO, "Updating the firmware ......\n")); > > // > // All capsules left are recognized by platform. > @@ -411,7 +463,6 @@ ProcessTheseCapsules ( > if (EFI_ERROR(Status)) { > REPORT_STATUS_CODE(EFI_ERROR_CODE, (EFI_SOFTWARE | PcdGet32(PcdStatusCodeSubClassCapsule) | PcdGet32(PcdCapsuleStatusCodeUpdateFirmwareFailed))); > DEBUG ((DEBUG_ERROR, "Capsule process failed!\n")); > - Print (L"Firmware update failed...\r\n"); > } else { > REPORT_STATUS_CODE(EFI_PROGRESS_CODE, (EFI_SOFTWARE | PcdGet32(PcdStatusCodeSubClassCapsule) | PcdGet32(PcdCapsuleStatusCodeUpdateFirmwareSuccess))); > } > @@ -447,18 +498,9 @@ DoResetSystem ( > VOID > ) > { > - UINTN Index; > - > - REPORT_STATUS_CODE(EFI_PROGRESS_CODE, (EFI_SOFTWARE | PcdGet32(PcdStatusCodeSubClassCapsule) | PcdGet32(PcdCapsuleStatusCodeResettingSystem))); > - > - Print(L"Capsule Request Cold Reboot.\n"); > DEBUG((DEBUG_INFO, "Capsule Request Cold Reboot.")); > > - for (Index = 5; Index > 0; Index--) { > - Print(L"\rResetting system in %d seconds ...", Index); > - DEBUG((DEBUG_INFO, "\rResetting system in %d seconds ...", Index)); > - gBS->Stall(1000000); > - } > + REPORT_STATUS_CODE(EFI_PROGRESS_CODE, (EFI_SOFTWARE | PcdGet32(PcdStatusCodeSubClassCapsule) | PcdGet32(PcdCapsuleStatusCodeResettingSystem))); > > gRT->ResetSystem(EfiResetCold, EFI_SUCCESS, 0, NULL); > > diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLibNull.c b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLibNull.c > index 07e9e46eae..274c1c4c1c 100644 > --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLibNull.c > +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleProcessLibNull.c > @@ -3,7 +3,7 @@ > Dummy function for runtime module, because CapsuleDxeRuntime > does not need call ProcessCapsules(). > > - Copyright (c) 2016, Intel Corporation. All rights reserved.
> + Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -17,6 +17,25 @@ > #include > #include > > +/** > + Function indicate the current completion progress of the firmware > + update. Platform may override with own specific progress function. > + > + @param[in] Completion A value between 1 and 100 indicating the current > + completion progress of the firmware update > + > + @retval EFI_SUCESS The capsule update progress was updated. > + @retval EFI_INVALID_PARAMETER Completion is greater than 100%. > +**/ > +EFI_STATUS > +EFIAPI > +UpdateImageProgress ( > + IN UINTN Completion > + ) > +{ > + return EFI_SUCCESS; > +} > + > /** > > This routine is called to process capsules. > diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf > index 1659b13ef4..342df9e99c 100644 > --- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf > +++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf > @@ -69,9 +69,10 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdCapsuleStatusCodeResettingSystem ## CONSUMES > > [Protocols] > - gEsrtManagementProtocolGuid ## CONSUMES > - gEfiFirmwareManagementProtocolGuid ## CONSUMES > - gEdkiiVariableLockProtocolGuid ## SOMETIMES_CONSUMES > + gEsrtManagementProtocolGuid ## CONSUMES > + gEfiFirmwareManagementProtocolGuid ## CONSUMES > + gEdkiiVariableLockProtocolGuid ## SOMETIMES_CONSUMES > + gEdkiiFirmwareManagementProgressProtocolGuid ## SOMETIMES_CONSUMES > > [Guids] > gEfiFmpCapsuleGuid ## SOMETIMES_CONSUMES ## GUID > -- > 2.14.2.windows.3 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel