回复: [edk2-devel] [PATCH v1 1/1] FmpDevicePkg/FmpDxe: Update FmpDeviceCheckImageWithStatus() handling

["gaoliming" <gaoliming@byosoft.com.cn>]

Create PR https://github.com/tianocore/edk2/pull/2427 for it. 

Thanks
Liming
> -----邮件原件-----
> 发件人: Michael Kubacki <mikuback@linux.microsoft.com>
> 发送时间: 2022年1月20日 3:46
> 收件人: devel@edk2.groups.io; guomin.jiang@intel.com
> 抄送: Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Xu, Wei6 <wei6.xu@intel.com>
> 主题: Re: [edk2-devel] [PATCH v1 1/1] FmpDevicePkg/FmpDxe: Update
> FmpDeviceCheckImageWithStatus() handling
> 
> Hi Guomin,
> 
> It has been a couple of weeks since your review and I have not seen the
> patch merged yet.
> 
> Do you know when it will be merged?
> 
> Thanks,
> Michael
> 
> On 1/6/2022 10:45 PM, Guomin Jiang wrote:
> > Reviewed-by: Guomin Jiang <guomin.jiang@intel.com>
> >
> > Guomin
> >
> >> -----Original Message-----
> >> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> >> Sent: Wednesday, January 5, 2022 4:38 AM
> >> To: devel@edk2.groups.io
> >> Cc: Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; Jiang, Guomin
> <Guomin.Jiang@intel.com>;
> >> Xu, Wei6 <wei6.xu@intel.com>
> >> Subject: [PATCH v1 1/1] FmpDevicePkg/FmpDxe: Update
> >> FmpDeviceCheckImageWithStatus() handling
> >>
> >> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>
> >> Update the logic handling last attempt status codes from
> >> FmpDeviceCheckImageWithStatus() implementations to account for cases
> >> when the function return status code is EFI_SUCCESS (since the image was
> >> checked successfully) but the ImageUpdatable value is not valid.
> >>
> >> In addition the following sentence is removed from the LastAttemptStatus
> >> parameter definition for
> >> FmpDeviceCheckImageWithStatus() since it can lead to confusion.
> >> The expected status code value range is sufficient to implement the library
> >> API.
> >>
> >>    "This value will only be checked when this
> >>     function returns an error."
> >>
> >> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Cc: Guomin Jiang <guomin.jiang@intel.com>
> >> Cc: Wei6 Xu <wei6.xu@intel.com>
> >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >> ---
> >>   FmpDevicePkg/FmpDxe/FmpDxe.c                         | 23
> +++++++++++++++-----
> >>   FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c |  3 +--
> >>   FmpDevicePkg/Include/Library/FmpDeviceLib.h          |  3 +--
> >>   3 files changed, 19 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c
> >> b/FmpDevicePkg/FmpDxe/FmpDxe.c index 197df28c8dd6..1e7ec4a09e16
> >> 100644
> >> --- a/FmpDevicePkg/FmpDxe/FmpDxe.c
> >> +++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
> >> @@ -1040,8 +1040,19 @@ CheckTheImageInternal (
> >>     //
> >>     Status = FmpDeviceCheckImageWithStatus ((((UINT8 *)Image) +
> >> AllHeaderSize), RawSize, ImageUpdatable, LastAttemptStatus);
> >>     if (EFI_ERROR (Status)) {
> >> +    // The image cannot be valid if an error occurred checking the image
> >> +    if (*ImageUpdatable == IMAGE_UPDATABLE_VALID) {
> >> +      *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
> >> +    }
> >> +
> >>       DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImage() -
> FmpDeviceLib
> >> CheckImage failed. Status = %r\n", mImageIdName, Status));
> >> +  }
> >>
> >> +  //
> >> +  // Only validate the library last attempt status code if the image is not
> >> updatable.
> >> +  // This specifically avoids converting LAST_ATTEMPT_STATUS_SUCCESS
> if it
> >> set for an updatable image.
> >> +  //
> >> +  if (*ImageUpdatable != IMAGE_UPDATABLE_VALID) {
> >>       //
> >>       // LastAttemptStatus returned from the device library should fall
> within
> >> the designated error range
> >>       //
> [LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE,
> >> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE]
> >> @@ -1049,12 +1060,12 @@ CheckTheImageInternal (
> >>       if ((*LastAttemptStatus <
> >> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE)
> ||
> >>           (*LastAttemptStatus >
> >> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE))
> >>       {
> >> -      DEBUG (
> >> -        (DEBUG_ERROR,
> >> -         "FmpDxe(%s): CheckTheImage() - LastAttemptStatus %d from
> >> FmpDeviceCheckImageWithStatus() is invalid.\n",
> >> -         mImageIdName,
> >> -         *LastAttemptStatus)
> >> -        );
> >> +      DEBUG ((
> >> +        DEBUG_ERROR,
> >> +        "FmpDxe(%s): CheckTheImage() - LastAttemptStatus %d from
> >> FmpDeviceCheckImageWithStatus() is invalid.\n",
> >> +        mImageIdName,
> >> +        *LastAttemptStatus
> >> +        ));
> >>         *LastAttemptStatus =
> LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
> >>       }
> >>     }
> >> diff --git a/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> >> b/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> >> index 2e5c17b2b0f9..82219e87a430 100644
> >> --- a/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> >> +++ b/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> >> @@ -434,8 +434,7 @@ FmpDeviceCheckImage (
> >>
> IMAGE_UPDATABLE_VALID_WITH_VENDOR_CODE
> >>     @param[out] LastAttemptStatus   A pointer to a UINT32 that holds
> the last
> >> attempt
> >>                                     status to report back to the
> ESRT table in case
> >> -                                  of error. This value will only be
> checked when this
> >> -                                  function returns an error.
> >> +                                  of error.
> >>
> >>                                     The return status code must
> fall in the range of
> >>
> >> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> diff --git a/FmpDevicePkg/Include/Library/FmpDeviceLib.h
> >> b/FmpDevicePkg/Include/Library/FmpDeviceLib.h
> >> index a14406abe8b5..f82ef64503fa 100644
> >> --- a/FmpDevicePkg/Include/Library/FmpDeviceLib.h
> >> +++ b/FmpDevicePkg/Include/Library/FmpDeviceLib.h
> >> @@ -421,8 +421,7 @@ FmpDeviceCheckImage (
> >>
> IMAGE_UPDATABLE_VALID_WITH_VENDOR_CODE
> >>     @param[out] LastAttemptStatus   A pointer to a UINT32 that holds
> the last
> >> attempt
> >>                                     status to report back to the
> ESRT table in case
> >> -                                  of error. This value will only be
> checked when this
> >> -                                  function returns an error.
> >> +                                  of error.
> >>
> >>                                     The return status code must
> fall in the range of
> >>
> >> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> --
> >> 2.28.0.windows.1
> >
> >
> >
> > 
> >