public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
       [not found] <1639FD37CA1FD9A2.6392@groups.io>
@ 2020-10-15 21:27 ` Michael Kubacki
       [not found] ` <bf090dba-75d3-520e-7414-bcdb8314562b@outlook.com>
  1 sibling, 0 replies; 5+ messages in thread
From: Michael Kubacki @ 2020-10-15 21:27 UTC (permalink / raw)
  To: devel; +Cc: Michael D Kinney, Yi Qian, Zailiang Sun

Hi,

It's been two weeks with no feedback.

A review would be appreciated.

Thanks,
Michael

On 10/1/2020 2:58 PM, Michael Kubacki wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Makes the changes necessary for these library instances of
> FmpDeviceLib to be compatible with new functions added recently
> to FmpDeviceLib.
> 
> Two new functions were introduced in FmpDeviceLib to allow a
> library instance to return a Last Attempt Status code during
> check image and set image operations:
>    1. FmpDeviceCheckImageWithStatus ( )
>    2. FmpDeviceSetImageWithStatus ( )
> 
> FmpDxe (in FmpDevicePkg) will begin calling these new functions
> instead of the previous functions. Therefore, this change:
>    1. Adds these functions to Vlv2TbltDevicePkg implementations
>    2. Moves the main functionality to these new functions
>    3. Updates the old functions to call the new functions
>       (for backward compatibility)
> 
> Note: As of this commit, the Vlv2TbltDevicePkg build is broken
> due to:
>    1. A required RngLib library instance not defined by the platform
>    2. Other FMP libraries not being defined by the platform
>       (e.g. FmpDependencyLib, FmpDependencyCheckLib, etc.)
> 
> Those changes were fixed locally to test the changes in this commit
> but maintainers should make the proper changes for those issues.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Yi Qian <yi.qian@intel.com>
> Cc: Zailiang Sun <zailiang.sun@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> 
> Notes:
>      Only build was checked, I do not have access to a
>      VLV2 device for testing.
> 
>   Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c       | 299 +++++++++++++++-----
>   Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c | 284 ++++++++++++++-----
>   2 files changed, 446 insertions(+), 137 deletions(-)
> 
> diff --git a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> index d8c9036012ad..df8a36d9854c 100644
> --- a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> +++ b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> @@ -1,6 +1,6 @@
>   /**
>   
> -Copyright (c) 2016, Microsoft Corporation.  All rights reserved.
> +Copyright (c) Microsoft Corporation.<BR>
>   Copyright (c) 2019, Intel Corporation.  All rights reserved.
>   
>   SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -8,7 +8,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   **/
>   
>   #include <PiDxe.h>
> -
> +#include <LastAttemptStatus.h>
> +#include <Guid/SystemResourceTable.h>
>   #include <Library/FmpDeviceLib.h>
>   
>   #include <Library/DebugLib.h>
> @@ -444,20 +445,17 @@ FmpDeviceGetImage (
>   }
>   
>   /**
> -  Updates the firmware image of the device.
> -
> -  This function updates the hardware with the new firmware image.  This function
> -  returns EFI_UNSUPPORTED if the firmware image is not updatable.  If the
> -  firmware image is updatable, the function should perform the following minimal
> -  validations before proceeding to do the firmware image update.
> -    - Validate the image is a supported image for this device.  The function
> -      returns EFI_ABORTED if the image is unsupported.  The function can
> -      optionally provide more detailed information on why the image is not a
> -      supported image.
> -    - Validate the data from VendorCode if not null.  Image validation must be
> -      performed before VendorCode data validation.  VendorCode data is ignored
> -      or considered invalid if image validation failed.  The function returns
> -      EFI_ABORTED if the data is invalid.
> +  Updates a firmware device with a new firmware image.  This function returns
> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the firmware image
> +  is updatable, the function should perform the following minimal validations
> +  before proceeding to do the firmware image update.
> +    - Validate that the image is a supported image for this firmware device.
> +      Return EFI_ABORTED if the image is not supported.  Additional details
> +      on why the image is not a supported image may be returned in AbortReason.
> +    - Validate the data from VendorCode if is not NULL.  Firmware image
> +      validation must be performed before VendorCode data validation.
> +      VendorCode data is ignored or considered invalid if image validation
> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
>   
>     VendorCode enables vendor to implement vendor-specific firmware image update
>     policy.  Null if the caller did not specify the policy or use the default
> @@ -470,38 +468,56 @@ FmpDeviceGetImage (
>     have the option to provide a more detailed description of the abort reason to
>     the caller.
>   
> -  @param[in]  Image             Points to the new image.
> -  @param[in]  ImageSize         Size of the new image in bytes.
> +  @param[in]  Image             Points to the new firmware image.
> +  @param[in]  ImageSize         Size, in bytes, of the new firmware image.
>     @param[in]  VendorCode        This enables vendor to implement vendor-specific
> -                                firmware image update policy. Null indicates the
> -                                caller did not specify the policy or use the
> +                                firmware image update policy.  NULL indicates
> +                                the caller did not specify the policy or use the
>                                   default policy.
> -  @param[in]  Progress          A function used by the driver to report the
> -                                progress of the firmware update.
> -  @param[in]  CapsuleFwVersion  FMP Payload Header version of the image.
> -  @param[out] AbortReason       A pointer to a pointer to a null-terminated
> -                                string providing more details for the aborted
> -                                operation. The buffer is allocated by this
> -                                function with AllocatePool(), and it is the
> -                                caller's responsibility to free it with a call
> -                                to FreePool().
> +  @param[in]  Progress          A function used to report the progress of
> +                                updating the firmware device with the new
> +                                firmware image.
> +  @param[in]  CapsuleFwVersion  The version of the new firmware image from the
> +                                update capsule that provided the new firmware
> +                                image.
> +  @param[out] AbortReason       A pointer to a pointer to a Null-terminated
> +                                Unicode string providing more details on an
> +                                aborted operation. The buffer is allocated by
> +                                this function with
> +                                EFI_BOOT_SERVICES.AllocatePool().  It is the
> +                                caller's responsibility to free this buffer with
> +                                EFI_BOOT_SERVICES.FreePool().
> +  @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.
>   
> -  @retval EFI_SUCCESS            The device was successfully updated with the
> -                                 new image.
> -  @retval EFI_ABORTED            The operation is aborted.
> +                                The return status code must fall in the range of
> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> +
> +                                If the value falls outside this range, it will be converted
> +                                to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> +
> +  @retval EFI_SUCCESS            The firmware device was successfully updated
> +                                 with the new firmware image.
> +  @retval EFI_ABORTED            The operation is aborted.  Additional details
> +                                 are provided in AbortReason.
>     @retval EFI_INVALID_PARAMETER  The Image was NULL.
> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
>     @retval EFI_UNSUPPORTED        The operation is not supported.
>   
>   **/
>   EFI_STATUS
>   EFIAPI
> -FmpDeviceSetImage (
> +FmpDeviceSetImageWithStatus (
>     IN  CONST VOID                                     *Image,
>     IN  UINTN                                          ImageSize,
> -  IN  CONST VOID                                     *VendorCode,
> -  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
> +  IN  CONST VOID                                     *VendorCode,       OPTIONAL
> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,          OPTIONAL
>     IN  UINT32                                         CapsuleFwVersion,
> -  OUT CHAR16                                         **AbortReason
> +  OUT CHAR16                                         **AbortReason,
> +  OUT UINT32                                         *LastAttemptStatus
>     )
>   {
>     EFI_STATUS          Status;
> @@ -513,25 +529,27 @@ FmpDeviceSetImage (
>     UINTN               BytesWritten;
>   
>     Updateable = 0;
> -  Status = FmpDeviceCheckImage (Image, ImageSize, &Updateable);
> +  Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, &Updateable, LastAttemptStatus);
>     if (EFI_ERROR (Status)) {
> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image failed with %r.\n", Status));
> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check Image failed with %r.\n", Status));
>       return Status;
>     }
>   
>     if (Updateable != IMAGE_UPDATABLE_VALID) {
> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image returned that the Image was not valid for update.  Updatable value = 0x%X.\n", Updateable));
> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check Image returned that the Image was not valid for update.  Updatable value = 0x%X.\n", Updateable));
> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>       return EFI_ABORTED;
>     }
>   
>     if (Progress == NULL) {
> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Invalid progress callback\n"));
> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Invalid progress callback\n"));
> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>       return EFI_INVALID_PARAMETER;
>     }
>   
>     Status = Progress (15);
>     if (EFI_ERROR (Status)) {
> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed with Status %r.\n", Status));
> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress Callback failed with Status %r.\n", Status));
>     }
>   
>     //
> @@ -539,7 +557,7 @@ FmpDeviceSetImage (
>     //
>     Progress (20);
>     if (EFI_ERROR (Status)) {
> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed with Status %r.\n", Status));
> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress Callback failed with Status %r.\n", Status));
>     }
>   
>     //
> @@ -553,11 +571,11 @@ FmpDeviceSetImage (
>   
>   //    Progress (Percentage);
>   //    if (EFI_ERROR (Status)) {
> -//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed with Status %r.\n", Status));
> +//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress Callback failed with Status %r.\n", Status));
>   //    }
>     }
>   
> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %d Images ...\n", ARRAY_SIZE (mUpdateConfigData)));
> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %d Images ...\n", ARRAY_SIZE (mUpdateConfigData)));
>   
>     if (ARRAY_SIZE (mUpdateConfigData) == 0) {
>       DEBUG((DEBUG_INFO, "PlatformUpdate: BaseAddress - 0x%lx ImageOffset - 0x%x Length - 0x%x\n", 0, 0, ImageSize));
> @@ -605,11 +623,173 @@ FmpDeviceSetImage (
>       BytesWritten += ConfigData->Length;
>     }
>   
> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %r\n", Status));
> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %r\n", Status));
> +
> +  if (EFI_ERROR (Status)) {
> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> +  }
>   
>     return Status;
>   }
>   
> +/**
> +  Updates the firmware image of the device.
> +
> +  This function updates the hardware with the new firmware image.  This function
> +  returns EFI_UNSUPPORTED if the firmware image is not updatable.  If the
> +  firmware image is updatable, the function should perform the following minimal
> +  validations before proceeding to do the firmware image update.
> +    - Validate the image is a supported image for this device.  The function
> +      returns EFI_ABORTED if the image is unsupported.  The function can
> +      optionally provide more detailed information on why the image is not a
> +      supported image.
> +    - Validate the data from VendorCode if not null.  Image validation must be
> +      performed before VendorCode data validation.  VendorCode data is ignored
> +      or considered invalid if image validation failed.  The function returns
> +      EFI_ABORTED if the data is invalid.
> +
> +  VendorCode enables vendor to implement vendor-specific firmware image update
> +  policy.  Null if the caller did not specify the policy or use the default
> +  policy.  As an example, vendor can implement a policy to allow an option to
> +  force a firmware image update when the abort reason is due to the new firmware
> +  image version is older than the current firmware image version or bad image
> +  checksum.  Sensitive operations such as those wiping the entire firmware image
> +  and render the device to be non-functional should be encoded in the image
> +  itself rather than passed with the VendorCode.  AbortReason enables vendor to
> +  have the option to provide a more detailed description of the abort reason to
> +  the caller.
> +
> +  @param[in]  Image             Points to the new image.
> +  @param[in]  ImageSize         Size of the new image in bytes.
> +  @param[in]  VendorCode        This enables vendor to implement vendor-specific
> +                                firmware image update policy. Null indicates the
> +                                caller did not specify the policy or use the
> +                                default policy.
> +  @param[in]  Progress          A function used by the driver to report the
> +                                progress of the firmware update.
> +  @param[in]  CapsuleFwVersion  FMP Payload Header version of the image.
> +  @param[out] AbortReason       A pointer to a pointer to a null-terminated
> +                                string providing more details for the aborted
> +                                operation. The buffer is allocated by this
> +                                function with AllocatePool(), and it is the
> +                                caller's responsibility to free it with a call
> +                                to FreePool().
> +
> +  @retval EFI_SUCCESS            The device was successfully updated with the
> +                                 new image.
> +  @retval EFI_ABORTED            The operation is aborted.
> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> +  @retval EFI_UNSUPPORTED        The operation is not supported.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +FmpDeviceSetImage (
> +  IN  CONST VOID                                     *Image,
> +  IN  UINTN                                          ImageSize,
> +  IN  CONST VOID                                     *VendorCode,
> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
> +  IN  UINT32                                         CapsuleFwVersion,
> +  OUT CHAR16                                         **AbortReason
> +  )
> +{
> +  UINT32  LastAttemptStatus;
> +
> +  return  FmpDeviceSetImageWithStatus (
> +            Image,
> +            ImageSize,
> +            VendorCode,
> +            Progress,
> +            CapsuleFwVersion,
> +            AbortReason,
> +            &LastAttemptStatus
> +            );
> +}
> +
> +/**
> +  Checks if a new firmware image is valid for the firmware device.  This
> +  function allows firmware update operation to validate the firmware image
> +  before FmpDeviceSetImage() is called.
> +
> +  @param[in]  Image               Points to a new firmware image.
> +  @param[in]  ImageSize           Size, in bytes, of a new firmware image.
> +  @param[out] ImageUpdatable      Indicates if a new firmware image is valid for
> +                                  a firmware update to the firmware device.  The
> +                                  following values from the Firmware Management
> +                                  Protocol are supported:
> +                                    IMAGE_UPDATABLE_VALID
> +                                    IMAGE_UPDATABLE_INVALID
> +                                    IMAGE_UPDATABLE_INVALID_TYPE
> +                                    IMAGE_UPDATABLE_INVALID_OLD
> +                                    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.
> +
> +                                  The return status code must fall in the range of
> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> +
> +                                  If the value falls outside this range, it will be converted
> +                                  to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> +
> +  @retval EFI_SUCCESS            The image was successfully checked.  Additional
> +                                 status information is returned in
> +                                 ImageUpdatable.
> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +FmpDeviceCheckImageWithStatus (
> +  IN  CONST VOID  *Image,
> +  IN  UINTN       ImageSize,
> +  OUT UINT32      *ImageUpdatable,
> +  OUT UINT32      *LastAttemptStatus
> +  )
> +{
> +  if (LastAttemptStatus == NULL) {
> +    DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - LastAttemptStatus Pointer Parameter is NULL.\n"));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
> +
> +  if (ImageUpdatable == NULL) {
> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - ImageUpdatable Pointer Parameter is NULL.\n"));
> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  //Set to valid and then if any tests fail it will update this flag.
> +  //
> +  *ImageUpdatable = IMAGE_UPDATABLE_VALID;
> +
> +  if (Image == NULL) {
> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer Parameter is NULL.\n"));
> +    //
> +    // Not sure if this is needed
> +    //
> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Make sure the image size is correct
> +  //
> +  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
>   /**
>   Checks if the firmware image is valid for the device.
>   
> @@ -633,34 +813,9 @@ FmpDeviceCheckImage (
>     OUT UINT32      *ImageUpdateable
>     )
>   {
> -  if (ImageUpdateable == NULL) {
> -    DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer Parameter is NULL.\n"));
> -    return EFI_INVALID_PARAMETER;
> -  }
> +  UINT32  LastAttemptStatus;
>   
> -  //
> -  //Set to valid and then if any tests fail it will update this flag.
> -  //
> -  *ImageUpdateable = IMAGE_UPDATABLE_VALID;
> -
> -  if (Image == NULL) {
> -    DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter is NULL.\n"));
> -    //
> -    // Not sure if this is needed
> -    //
> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
> -  //
> -  // Make sure the image size is correct
> -  //
> -  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
> -    return EFI_INVALID_PARAMETER;
> -  }
> -
> -  return EFI_SUCCESS;
> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, ImageUpdateable, &LastAttemptStatus);
>   }
>   
>   /**
> diff --git a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> index db0f238ea534..132b60844ad4 100644
> --- a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> +++ b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> @@ -1,6 +1,6 @@
>   /**
>   
> -Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>   
>   SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   
>   #include <PiDxe.h>
> +#include <LastAttemptStatus.h>
> +#include <Guid/SystemResourceTable.h>
>   #include <Library/DebugLib.h>
>   #include <Protocol/FirmwareManagement.h>
>   #include <Library/BaseLib.h>
> @@ -345,6 +347,131 @@ Return Value:
>   }//GetImage()
>   
>   
> +/**
> +  Updates a firmware device with a new firmware image.  This function returns
> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the firmware image
> +  is updatable, the function should perform the following minimal validations
> +  before proceeding to do the firmware image update.
> +    - Validate that the image is a supported image for this firmware device.
> +      Return EFI_ABORTED if the image is not supported.  Additional details
> +      on why the image is not a supported image may be returned in AbortReason.
> +    - Validate the data from VendorCode if is not NULL.  Firmware image
> +      validation must be performed before VendorCode data validation.
> +      VendorCode data is ignored or considered invalid if image validation
> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
> +
> +  VendorCode enables vendor to implement vendor-specific firmware image update
> +  policy.  Null if the caller did not specify the policy or use the default
> +  policy.  As an example, vendor can implement a policy to allow an option to
> +  force a firmware image update when the abort reason is due to the new firmware
> +  image version is older than the current firmware image version or bad image
> +  checksum.  Sensitive operations such as those wiping the entire firmware image
> +  and render the device to be non-functional should be encoded in the image
> +  itself rather than passed with the VendorCode.  AbortReason enables vendor to
> +  have the option to provide a more detailed description of the abort reason to
> +  the caller.
> +
> +  @param[in]  Image             Points to the new firmware image.
> +  @param[in]  ImageSize         Size, in bytes, of the new firmware image.
> +  @param[in]  VendorCode        This enables vendor to implement vendor-specific
> +                                firmware image update policy.  NULL indicates
> +                                the caller did not specify the policy or use the
> +                                default policy.
> +  @param[in]  Progress          A function used to report the progress of
> +                                updating the firmware device with the new
> +                                firmware image.
> +  @param[in]  CapsuleFwVersion  The version of the new firmware image from the
> +                                update capsule that provided the new firmware
> +                                image.
> +  @param[out] AbortReason       A pointer to a pointer to a Null-terminated
> +                                Unicode string providing more details on an
> +                                aborted operation. The buffer is allocated by
> +                                this function with
> +                                EFI_BOOT_SERVICES.AllocatePool().  It is the
> +                                caller's responsibility to free this buffer with
> +                                EFI_BOOT_SERVICES.FreePool().
> +  @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.
> +
> +                                The return status code must fall in the range of
> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> +
> +                                If the value falls outside this range, it will be converted
> +                                to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> +
> +  @retval EFI_SUCCESS            The firmware device was successfully updated
> +                                 with the new firmware image.
> +  @retval EFI_ABORTED            The operation is aborted.  Additional details
> +                                 are provided in AbortReason.
> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
> +  @retval EFI_UNSUPPORTED        The operation is not supported.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +FmpDeviceSetImageWithStatus (
> +  IN  CONST VOID                                     *Image,
> +  IN  UINTN                                          ImageSize,
> +  IN  CONST VOID                                     *VendorCode,       OPTIONAL
> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,          OPTIONAL
> +  IN  UINT32                                         CapsuleFwVersion,
> +  OUT CHAR16                                         **AbortReason,
> +  OUT UINT32                                         *LastAttemptStatus
> +  )
> +{
> +    EFI_STATUS Status                         = EFI_SUCCESS;
> +    UINT32 Updateable                         = 0;
> +
> +    Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, &Updateable, LastAttemptStatus);
> +    if (EFI_ERROR(Status))
> +    {
> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image failed with %r.\n", Status));
> +        goto cleanup;
> +    }
> +
> +    if (Updateable != IMAGE_UPDATABLE_VALID)
> +    {
> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image returned that the Image was not valid for update.  Updatable value = 0x%X.\n", Updateable));
> +        Status = EFI_ABORTED;
> +        goto cleanup;
> +    }
> +
> +    if (Progress == NULL)
> +    {
> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Invalid progress callback\n"));
> +        Status = EFI_INVALID_PARAMETER;
> +        goto cleanup;
> +    }
> +
> +    Status = Progress(15);
> +    if (EFI_ERROR(Status))
> +    {
> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Progress Callback failed with Status %r.\n", Status));
> +    }
> +
> +    {
> +      UINTN  p;
> +
> +      for (p = 20; p < 100; p++) {
> +        gBS->Stall (100000);  //us  = 0.1 seconds
> +        Progress (p);
> +      }
> +    }
> +
> +    //TODO: add support for VendorCode, and AbortReason
> +cleanup:
> +    if (EFI_ERROR (Status)) {
> +      *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> +    }
> +
> +    return Status;
> +}// SetImageWithStatus()
> +
> +
>   /**
>   Updates the firmware image of the device.
>   
> @@ -396,51 +523,98 @@ IN  UINT32                                           CapsuleFwVersion,
>   OUT CHAR16                                           **AbortReason
>   )
>   {
> -    EFI_STATUS Status                         = EFI_SUCCESS;
> -    UINT32 Updateable                         = 0;
> -
> -    Status = FmpDeviceCheckImage(Image, ImageSize, &Updateable);
> -    if (EFI_ERROR(Status))
> -    {
> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image failed with %r.\n", Status));
> -        goto cleanup;
> -    }
> -
> -    if (Updateable != IMAGE_UPDATABLE_VALID)
> -    {
> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image returned that the Image was not valid for update.  Updatable value = 0x%X.\n", Updateable));
> -        Status = EFI_ABORTED;
> -        goto cleanup;
> -    }
> -
> -    if (Progress == NULL)
> -    {
> -        DEBUG((DEBUG_ERROR, "SetImage - Invalid progress callback\n"));
> -        Status = EFI_INVALID_PARAMETER;
> -        goto cleanup;
> -    }
> -
> -    Status = Progress(15);
> -    if (EFI_ERROR(Status))
> -    {
> -        DEBUG((DEBUG_ERROR, "SetImage - Progress Callback failed with Status %r.\n", Status));
> -    }
> -
> -    {
> -      UINTN  p;
> -
> -      for (p = 20; p < 100; p++) {
> -        gBS->Stall (100000);  //us  = 0.1 seconds
> -        Progress (p);
> -      }
> -    }
> -
> -    //TODO: add support for VendorCode, and AbortReason
> -cleanup:
> -    return Status;
> +  UINT32  LastAttemptStatus;
> +
> +  return  FmpDeviceSetImageWithStatus (
> +            Image,
> +            ImageSize,
> +            VendorCode,
> +            Progress,
> +            CapsuleFwVersion,
> +            AbortReason,
> +            &LastAttemptStatus
> +            );
>   }// SetImage()
>   
>   
> +/**
> +  Checks if a new firmware image is valid for the firmware device.  This
> +  function allows firmware update operation to validate the firmware image
> +  before FmpDeviceSetImage() is called.
> +
> +  @param[in]  Image               Points to a new firmware image.
> +  @param[in]  ImageSize           Size, in bytes, of a new firmware image.
> +  @param[out] ImageUpdatable      Indicates if a new firmware image is valid for
> +                                  a firmware update to the firmware device.  The
> +                                  following values from the Firmware Management
> +                                  Protocol are supported:
> +                                    IMAGE_UPDATABLE_VALID
> +                                    IMAGE_UPDATABLE_INVALID
> +                                    IMAGE_UPDATABLE_INVALID_TYPE
> +                                    IMAGE_UPDATABLE_INVALID_OLD
> +                                    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.
> +
> +                                  The return status code must fall in the range of
> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> +
> +                                  If the value falls outside this range, it will be converted
> +                                  to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> +
> +  @retval EFI_SUCCESS            The image was successfully checked.  Additional
> +                                 status information is returned in
> +                                 ImageUpdatable.
> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +FmpDeviceCheckImageWithStatus (
> +  IN  CONST VOID  *Image,
> +  IN  UINTN       ImageSize,
> +  OUT UINT32      *ImageUpdatable,
> +  OUT UINT32      *LastAttemptStatus
> +  )
> +{
> +    EFI_STATUS status = EFI_SUCCESS;
> +
> +    if (LastAttemptStatus == NULL) {
> +      DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - LastAttemptStatus Pointer Parameter is NULL.\n"));
> +      return EFI_INVALID_PARAMETER;
> +    }
> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
> +
> +    if (ImageUpdatable == NULL)
> +    {
> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - ImageUpdatable Pointer Parameter is NULL.\n"));
> +        *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> +        status = EFI_INVALID_PARAMETER;
> +        goto cleanup;
> +    }
> +
> +    //
> +    //Set to valid and then if any tests fail it will update this flag.
> +    //
> +    *ImageUpdatable = IMAGE_UPDATABLE_VALID;
> +
> +    if (Image == NULL)
> +    {
> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer Parameter is NULL.\n"));
> +        *ImageUpdatable = IMAGE_UPDATABLE_INVALID; //not sure if this is needed
> +        *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> +        return EFI_INVALID_PARAMETER;
> +    }
> +
> +cleanup:
> +    return status;
> +}// CheckImageWithStatus()
> +
>   
>   /**
>   Checks if the firmware image is valid for the device.
> @@ -465,29 +639,9 @@ IN  UINTN                             ImageSize,
>   OUT UINT32                            *ImageUpdateable
>   )
>   {
> -    EFI_STATUS status = EFI_SUCCESS;
> +  UINT32  LastAttemptStatus;
>   
> -    if (ImageUpdateable == NULL)
> -    {
> -        DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer Parameter is NULL.\n"));
> -        status = EFI_INVALID_PARAMETER;
> -        goto cleanup;
> -    }
> -
> -    //
> -    //Set to valid and then if any tests fail it will update this flag.
> -    //
> -    *ImageUpdateable = IMAGE_UPDATABLE_VALID;
> -
> -    if (Image == NULL)
> -    {
> -        DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter is NULL.\n"));
> -        *ImageUpdateable = IMAGE_UPDATABLE_INVALID; //not sure if this is needed
> -        return EFI_INVALID_PARAMETER;
> -    }
> -
> -cleanup:
> -    return status;
> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, ImageUpdateable, &LastAttemptStatus);
>   }// CheckImage()
>   
>   /**
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
       [not found] ` <bf090dba-75d3-520e-7414-bcdb8314562b@outlook.com>
@ 2020-10-27  0:50   ` Michael Kubacki
  0 siblings, 0 replies; 5+ messages in thread
From: Michael Kubacki @ 2020-10-27  0:50 UTC (permalink / raw)
  To: devel; +Cc: Michael D Kinney, Yi Qian, Zailiang Sun

I still haven't seen any feedback on this patch.

Can you please review it?

Thanks,
Michael

On 10/15/2020 2:27 PM, Michael Kubacki wrote:
> Hi,
> 
> It's been two weeks with no feedback.
> 
> A review would be appreciated.
> 
> Thanks,
> Michael
> 
> On 10/1/2020 2:58 PM, Michael Kubacki wrote:
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Makes the changes necessary for these library instances of
>> FmpDeviceLib to be compatible with new functions added recently
>> to FmpDeviceLib.
>>
>> Two new functions were introduced in FmpDeviceLib to allow a
>> library instance to return a Last Attempt Status code during
>> check image and set image operations:
>>    1. FmpDeviceCheckImageWithStatus ( )
>>    2. FmpDeviceSetImageWithStatus ( )
>>
>> FmpDxe (in FmpDevicePkg) will begin calling these new functions
>> instead of the previous functions. Therefore, this change:
>>    1. Adds these functions to Vlv2TbltDevicePkg implementations
>>    2. Moves the main functionality to these new functions
>>    3. Updates the old functions to call the new functions
>>       (for backward compatibility)
>>
>> Note: As of this commit, the Vlv2TbltDevicePkg build is broken
>> due to:
>>    1. A required RngLib library instance not defined by the platform
>>    2. Other FMP libraries not being defined by the platform
>>       (e.g. FmpDependencyLib, FmpDependencyCheckLib, etc.)
>>
>> Those changes were fixed locally to test the changes in this commit
>> but maintainers should make the proper changes for those issues.
>>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Yi Qian <yi.qian@intel.com>
>> Cc: Zailiang Sun <zailiang.sun@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>>
>> Notes:
>>      Only build was checked, I do not have access to a
>>      VLV2 device for testing.
>>
>>   
>> Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c       
>> | 299 +++++++++++++++-----
>>   
>> Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c 
>> | 284 ++++++++++++++-----
>>   2 files changed, 446 insertions(+), 137 deletions(-)
>>
>> diff --git 
>> a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c 
>> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c 
>>
>> index d8c9036012ad..df8a36d9854c 100644
>> --- 
>> a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c 
>>
>> +++ 
>> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c 
>>
>> @@ -1,6 +1,6 @@
>>   /**
>> -Copyright (c) 2016, Microsoft Corporation.  All rights reserved.
>> +Copyright (c) Microsoft Corporation.<BR>
>>   Copyright (c) 2019, Intel Corporation.  All rights reserved.
>>   SPDX-License-Identifier: BSD-2-Clause-Patent
>> @@ -8,7 +8,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>   **/
>>   #include <PiDxe.h>
>> -
>> +#include <LastAttemptStatus.h>
>> +#include <Guid/SystemResourceTable.h>
>>   #include <Library/FmpDeviceLib.h>
>>   #include <Library/DebugLib.h>
>> @@ -444,20 +445,17 @@ FmpDeviceGetImage (
>>   }
>>   /**
>> -  Updates the firmware image of the device.
>> -
>> -  This function updates the hardware with the new firmware image.  
>> This function
>> -  returns EFI_UNSUPPORTED if the firmware image is not updatable.  If 
>> the
>> -  firmware image is updatable, the function should perform the 
>> following minimal
>> -  validations before proceeding to do the firmware image update.
>> -    - Validate the image is a supported image for this device.  The 
>> function
>> -      returns EFI_ABORTED if the image is unsupported.  The function can
>> -      optionally provide more detailed information on why the image 
>> is not a
>> -      supported image.
>> -    - Validate the data from VendorCode if not null.  Image 
>> validation must be
>> -      performed before VendorCode data validation.  VendorCode data 
>> is ignored
>> -      or considered invalid if image validation failed.  The function 
>> returns
>> -      EFI_ABORTED if the data is invalid.
>> +  Updates a firmware device with a new firmware image.  This function 
>> returns
>> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the 
>> firmware image
>> +  is updatable, the function should perform the following minimal 
>> validations
>> +  before proceeding to do the firmware image update.
>> +    - Validate that the image is a supported image for this firmware 
>> device.
>> +      Return EFI_ABORTED if the image is not supported.  Additional 
>> details
>> +      on why the image is not a supported image may be returned in 
>> AbortReason.
>> +    - Validate the data from VendorCode if is not NULL.  Firmware image
>> +      validation must be performed before VendorCode data validation.
>> +      VendorCode data is ignored or considered invalid if image 
>> validation
>> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
>>     VendorCode enables vendor to implement vendor-specific firmware 
>> image update
>>     policy.  Null if the caller did not specify the policy or use the 
>> default
>> @@ -470,38 +468,56 @@ FmpDeviceGetImage (
>>     have the option to provide a more detailed description of the 
>> abort reason to
>>     the caller.
>> -  @param[in]  Image             Points to the new image.
>> -  @param[in]  ImageSize         Size of the new image in bytes.
>> +  @param[in]  Image             Points to the new firmware image.
>> +  @param[in]  ImageSize         Size, in bytes, of the new firmware 
>> image.
>>     @param[in]  VendorCode        This enables vendor to implement 
>> vendor-specific
>> -                                firmware image update policy. Null 
>> indicates the
>> -                                caller did not specify the policy or 
>> use the
>> +                                firmware image update policy.  NULL 
>> indicates
>> +                                the caller did not specify the policy 
>> or use the
>>                                   default policy.
>> -  @param[in]  Progress          A function used by the driver to 
>> report the
>> -                                progress of the firmware update.
>> -  @param[in]  CapsuleFwVersion  FMP Payload Header version of the image.
>> -  @param[out] AbortReason       A pointer to a pointer to a 
>> null-terminated
>> -                                string providing more details for the 
>> aborted
>> -                                operation. The buffer is allocated by 
>> this
>> -                                function with AllocatePool(), and it 
>> is the
>> -                                caller's responsibility to free it 
>> with a call
>> -                                to FreePool().
>> +  @param[in]  Progress          A function used to report the 
>> progress of
>> +                                updating the firmware device with the 
>> new
>> +                                firmware image.
>> +  @param[in]  CapsuleFwVersion  The version of the new firmware image 
>> from the
>> +                                update capsule that provided the new 
>> firmware
>> +                                image.
>> +  @param[out] AbortReason       A pointer to a pointer to a 
>> Null-terminated
>> +                                Unicode string providing more details 
>> on an
>> +                                aborted operation. The buffer is 
>> allocated by
>> +                                this function with
>> +                                EFI_BOOT_SERVICES.AllocatePool().  It 
>> is the
>> +                                caller's responsibility to free this 
>> buffer with
>> +                                EFI_BOOT_SERVICES.FreePool().
>> +  @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.
>> -  @retval EFI_SUCCESS            The device was successfully updated 
>> with the
>> -                                 new image.
>> -  @retval EFI_ABORTED            The operation is aborted.
>> +                                The return status code must fall in 
>> the range of
>> +                                
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
>> +                                
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
>> +
>> +                                If the value falls outside this 
>> range, it will be converted
>> +                                to 
>> LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
>> +
>> +  @retval EFI_SUCCESS            The firmware device was successfully 
>> updated
>> +                                 with the new firmware image.
>> +  @retval EFI_ABORTED            The operation is aborted.  
>> Additional details
>> +                                 are provided in AbortReason.
>>     @retval EFI_INVALID_PARAMETER  The Image was NULL.
>> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
>>     @retval EFI_UNSUPPORTED        The operation is not supported.
>>   **/
>>   EFI_STATUS
>>   EFIAPI
>> -FmpDeviceSetImage (
>> +FmpDeviceSetImageWithStatus (
>>     IN  CONST VOID                                     *Image,
>>     IN  UINTN                                          ImageSize,
>> -  IN  CONST VOID                                     *VendorCode,
>> -  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
>> +  IN  CONST VOID                                     
>> *VendorCode,       OPTIONAL
>> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  
>> Progress,          OPTIONAL
>>     IN  UINT32                                         CapsuleFwVersion,
>> -  OUT CHAR16                                         **AbortReason
>> +  OUT CHAR16                                         **AbortReason,
>> +  OUT UINT32                                         *LastAttemptStatus
>>     )
>>   {
>>     EFI_STATUS          Status;
>> @@ -513,25 +529,27 @@ FmpDeviceSetImage (
>>     UINTN               BytesWritten;
>>     Updateable = 0;
>> -  Status = FmpDeviceCheckImage (Image, ImageSize, &Updateable);
>> +  Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, 
>> &Updateable, LastAttemptStatus);
>>     if (EFI_ERROR (Status)) {
>> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image failed with 
>> %r.\n", Status));
>> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check Image 
>> failed with %r.\n", Status));
>>       return Status;
>>     }
>>     if (Updateable != IMAGE_UPDATABLE_VALID) {
>> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image returned 
>> that the Image was not valid for update.  Updatable value = 0x%X.\n", 
>> Updateable));
>> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check Image 
>> returned that the Image was not valid for update.  Updatable value = 
>> 0x%X.\n", Updateable));
>> +    *LastAttemptStatus = 
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>>       return EFI_ABORTED;
>>     }
>>     if (Progress == NULL) {
>> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Invalid progress 
>> callback\n"));
>> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Invalid 
>> progress callback\n"));
>> +    *LastAttemptStatus = 
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>>       return EFI_INVALID_PARAMETER;
>>     }
>>     Status = Progress (15);
>>     if (EFI_ERROR (Status)) {
>> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed 
>> with Status %r.\n", Status));
>> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress 
>> Callback failed with Status %r.\n", Status));
>>     }
>>     //
>> @@ -539,7 +557,7 @@ FmpDeviceSetImage (
>>     //
>>     Progress (20);
>>     if (EFI_ERROR (Status)) {
>> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed 
>> with Status %r.\n", Status));
>> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress 
>> Callback failed with Status %r.\n", Status));
>>     }
>>     //
>> @@ -553,11 +571,11 @@ FmpDeviceSetImage (
>>   //    Progress (Percentage);
>>   //    if (EFI_ERROR (Status)) {
>> -//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback 
>> failed with Status %r.\n", Status));
>> +//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress 
>> Callback failed with Status %r.\n", Status));
>>   //    }
>>     }
>> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %d Images ...\n", 
>> ARRAY_SIZE (mUpdateConfigData)));
>> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %d Images 
>> ...\n", ARRAY_SIZE (mUpdateConfigData)));
>>     if (ARRAY_SIZE (mUpdateConfigData) == 0) {
>>       DEBUG((DEBUG_INFO, "PlatformUpdate: BaseAddress - 0x%lx 
>> ImageOffset - 0x%x Length - 0x%x\n", 0, 0, ImageSize));
>> @@ -605,11 +623,173 @@ FmpDeviceSetImage (
>>       BytesWritten += ConfigData->Length;
>>     }
>> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %r\n", Status));
>> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %r\n", Status));
>> +
>> +  if (EFI_ERROR (Status)) {
>> +    *LastAttemptStatus = 
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>> +  }
>>     return Status;
>>   }
>> +/**
>> +  Updates the firmware image of the device.
>> +
>> +  This function updates the hardware with the new firmware image.  
>> This function
>> +  returns EFI_UNSUPPORTED if the firmware image is not updatable.  If 
>> the
>> +  firmware image is updatable, the function should perform the 
>> following minimal
>> +  validations before proceeding to do the firmware image update.
>> +    - Validate the image is a supported image for this device.  The 
>> function
>> +      returns EFI_ABORTED if the image is unsupported.  The function can
>> +      optionally provide more detailed information on why the image 
>> is not a
>> +      supported image.
>> +    - Validate the data from VendorCode if not null.  Image 
>> validation must be
>> +      performed before VendorCode data validation.  VendorCode data 
>> is ignored
>> +      or considered invalid if image validation failed.  The function 
>> returns
>> +      EFI_ABORTED if the data is invalid.
>> +
>> +  VendorCode enables vendor to implement vendor-specific firmware 
>> image update
>> +  policy.  Null if the caller did not specify the policy or use the 
>> default
>> +  policy.  As an example, vendor can implement a policy to allow an 
>> option to
>> +  force a firmware image update when the abort reason is due to the 
>> new firmware
>> +  image version is older than the current firmware image version or 
>> bad image
>> +  checksum.  Sensitive operations such as those wiping the entire 
>> firmware image
>> +  and render the device to be non-functional should be encoded in the 
>> image
>> +  itself rather than passed with the VendorCode.  AbortReason enables 
>> vendor to
>> +  have the option to provide a more detailed description of the abort 
>> reason to
>> +  the caller.
>> +
>> +  @param[in]  Image             Points to the new image.
>> +  @param[in]  ImageSize         Size of the new image in bytes.
>> +  @param[in]  VendorCode        This enables vendor to implement 
>> vendor-specific
>> +                                firmware image update policy. Null 
>> indicates the
>> +                                caller did not specify the policy or 
>> use the
>> +                                default policy.
>> +  @param[in]  Progress          A function used by the driver to 
>> report the
>> +                                progress of the firmware update.
>> +  @param[in]  CapsuleFwVersion  FMP Payload Header version of the image.
>> +  @param[out] AbortReason       A pointer to a pointer to a 
>> null-terminated
>> +                                string providing more details for the 
>> aborted
>> +                                operation. The buffer is allocated by 
>> this
>> +                                function with AllocatePool(), and it 
>> is the
>> +                                caller's responsibility to free it 
>> with a call
>> +                                to FreePool().
>> +
>> +  @retval EFI_SUCCESS            The device was successfully updated 
>> with the
>> +                                 new image.
>> +  @retval EFI_ABORTED            The operation is aborted.
>> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>> +  @retval EFI_UNSUPPORTED        The operation is not supported.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +FmpDeviceSetImage (
>> +  IN  CONST VOID                                     *Image,
>> +  IN  UINTN                                          ImageSize,
>> +  IN  CONST VOID                                     *VendorCode,
>> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
>> +  IN  UINT32                                         CapsuleFwVersion,
>> +  OUT CHAR16                                         **AbortReason
>> +  )
>> +{
>> +  UINT32  LastAttemptStatus;
>> +
>> +  return  FmpDeviceSetImageWithStatus (
>> +            Image,
>> +            ImageSize,
>> +            VendorCode,
>> +            Progress,
>> +            CapsuleFwVersion,
>> +            AbortReason,
>> +            &LastAttemptStatus
>> +            );
>> +}
>> +
>> +/**
>> +  Checks if a new firmware image is valid for the firmware device.  This
>> +  function allows firmware update operation to validate the firmware 
>> image
>> +  before FmpDeviceSetImage() is called.
>> +
>> +  @param[in]  Image               Points to a new firmware image.
>> +  @param[in]  ImageSize           Size, in bytes, of a new firmware 
>> image.
>> +  @param[out] ImageUpdatable      Indicates if a new firmware image 
>> is valid for
>> +                                  a firmware update to the firmware 
>> device.  The
>> +                                  following values from the Firmware 
>> Management
>> +                                  Protocol are supported:
>> +                                    IMAGE_UPDATABLE_VALID
>> +                                    IMAGE_UPDATABLE_INVALID
>> +                                    IMAGE_UPDATABLE_INVALID_TYPE
>> +                                    IMAGE_UPDATABLE_INVALID_OLD
>> +                                    
>> 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.
>> +
>> +                                  The return status code must fall in 
>> the range of
>> +                                  
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
>> +                                  
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
>> +
>> +                                  If the value falls outside this 
>> range, it will be converted
>> +                                  to 
>> LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
>> +
>> +  @retval EFI_SUCCESS            The image was successfully checked.  
>> Additional
>> +                                 status information is returned in
>> +                                 ImageUpdatable.
>> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
>> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
>> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +FmpDeviceCheckImageWithStatus (
>> +  IN  CONST VOID  *Image,
>> +  IN  UINTN       ImageSize,
>> +  OUT UINT32      *ImageUpdatable,
>> +  OUT UINT32      *LastAttemptStatus
>> +  )
>> +{
>> +  if (LastAttemptStatus == NULL) {
>> +    DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - LastAttemptStatus 
>> Pointer Parameter is NULL.\n"));
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +  *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
>> +
>> +  if (ImageUpdatable == NULL) {
>> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - ImageUpdatable 
>> Pointer Parameter is NULL.\n"));
>> +    *LastAttemptStatus = 
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  //
>> +  //Set to valid and then if any tests fail it will update this flag.
>> +  //
>> +  *ImageUpdatable = IMAGE_UPDATABLE_VALID;
>> +
>> +  if (Image == NULL) {
>> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer 
>> Parameter is NULL.\n"));
>> +    //
>> +    // Not sure if this is needed
>> +    //
>> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
>> +    *LastAttemptStatus = 
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  //
>> +  // Make sure the image size is correct
>> +  //
>> +  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
>> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
>> +    *LastAttemptStatus = 
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>>   /**
>>   Checks if the firmware image is valid for the device.
>> @@ -633,34 +813,9 @@ FmpDeviceCheckImage (
>>     OUT UINT32      *ImageUpdateable
>>     )
>>   {
>> -  if (ImageUpdateable == NULL) {
>> -    DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer 
>> Parameter is NULL.\n"));
>> -    return EFI_INVALID_PARAMETER;
>> -  }
>> +  UINT32  LastAttemptStatus;
>> -  //
>> -  //Set to valid and then if any tests fail it will update this flag.
>> -  //
>> -  *ImageUpdateable = IMAGE_UPDATABLE_VALID;
>> -
>> -  if (Image == NULL) {
>> -    DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter is 
>> NULL.\n"));
>> -    //
>> -    // Not sure if this is needed
>> -    //
>> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
>> -    return EFI_INVALID_PARAMETER;
>> -  }
>> -
>> -  //
>> -  // Make sure the image size is correct
>> -  //
>> -  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
>> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
>> -    return EFI_INVALID_PARAMETER;
>> -  }
>> -
>> -  return EFI_SUCCESS;
>> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, 
>> ImageUpdateable, &LastAttemptStatus);
>>   }
>>   /**
>> diff --git 
>> a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c 
>> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c 
>>
>> index db0f238ea534..132b60844ad4 100644
>> --- 
>> a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c 
>>
>> +++ 
>> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c 
>>
>> @@ -1,6 +1,6 @@
>>   /**
>> -Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
>> +Copyright (c) Microsoft Corporation.<BR>
>>   Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>>   SPDX-License-Identifier: BSD-2-Clause-Patent
>> @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>   #include <PiDxe.h>
>> +#include <LastAttemptStatus.h>
>> +#include <Guid/SystemResourceTable.h>
>>   #include <Library/DebugLib.h>
>>   #include <Protocol/FirmwareManagement.h>
>>   #include <Library/BaseLib.h>
>> @@ -345,6 +347,131 @@ Return Value:
>>   }//GetImage()
>> +/**
>> +  Updates a firmware device with a new firmware image.  This function 
>> returns
>> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the 
>> firmware image
>> +  is updatable, the function should perform the following minimal 
>> validations
>> +  before proceeding to do the firmware image update.
>> +    - Validate that the image is a supported image for this firmware 
>> device.
>> +      Return EFI_ABORTED if the image is not supported.  Additional 
>> details
>> +      on why the image is not a supported image may be returned in 
>> AbortReason.
>> +    - Validate the data from VendorCode if is not NULL.  Firmware image
>> +      validation must be performed before VendorCode data validation.
>> +      VendorCode data is ignored or considered invalid if image 
>> validation
>> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
>> +
>> +  VendorCode enables vendor to implement vendor-specific firmware 
>> image update
>> +  policy.  Null if the caller did not specify the policy or use the 
>> default
>> +  policy.  As an example, vendor can implement a policy to allow an 
>> option to
>> +  force a firmware image update when the abort reason is due to the 
>> new firmware
>> +  image version is older than the current firmware image version or 
>> bad image
>> +  checksum.  Sensitive operations such as those wiping the entire 
>> firmware image
>> +  and render the device to be non-functional should be encoded in the 
>> image
>> +  itself rather than passed with the VendorCode.  AbortReason enables 
>> vendor to
>> +  have the option to provide a more detailed description of the abort 
>> reason to
>> +  the caller.
>> +
>> +  @param[in]  Image             Points to the new firmware image.
>> +  @param[in]  ImageSize         Size, in bytes, of the new firmware 
>> image.
>> +  @param[in]  VendorCode        This enables vendor to implement 
>> vendor-specific
>> +                                firmware image update policy.  NULL 
>> indicates
>> +                                the caller did not specify the policy 
>> or use the
>> +                                default policy.
>> +  @param[in]  Progress          A function used to report the 
>> progress of
>> +                                updating the firmware device with the 
>> new
>> +                                firmware image.
>> +  @param[in]  CapsuleFwVersion  The version of the new firmware image 
>> from the
>> +                                update capsule that provided the new 
>> firmware
>> +                                image.
>> +  @param[out] AbortReason       A pointer to a pointer to a 
>> Null-terminated
>> +                                Unicode string providing more details 
>> on an
>> +                                aborted operation. The buffer is 
>> allocated by
>> +                                this function with
>> +                                EFI_BOOT_SERVICES.AllocatePool().  It 
>> is the
>> +                                caller's responsibility to free this 
>> buffer with
>> +                                EFI_BOOT_SERVICES.FreePool().
>> +  @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.
>> +
>> +                                The return status code must fall in 
>> the range of
>> +                                
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
>> +                                
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
>> +
>> +                                If the value falls outside this 
>> range, it will be converted
>> +                                to 
>> LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
>> +
>> +  @retval EFI_SUCCESS            The firmware device was successfully 
>> updated
>> +                                 with the new firmware image.
>> +  @retval EFI_ABORTED            The operation is aborted.  
>> Additional details
>> +                                 are provided in AbortReason.
>> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
>> +  @retval EFI_UNSUPPORTED        The operation is not supported.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +FmpDeviceSetImageWithStatus (
>> +  IN  CONST VOID                                     *Image,
>> +  IN  UINTN                                          ImageSize,
>> +  IN  CONST VOID                                     
>> *VendorCode,       OPTIONAL
>> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  
>> Progress,          OPTIONAL
>> +  IN  UINT32                                         CapsuleFwVersion,
>> +  OUT CHAR16                                         **AbortReason,
>> +  OUT UINT32                                         *LastAttemptStatus
>> +  )
>> +{
>> +    EFI_STATUS Status                         = EFI_SUCCESS;
>> +    UINT32 Updateable                         = 0;
>> +
>> +    Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, 
>> &Updateable, LastAttemptStatus);
>> +    if (EFI_ERROR(Status))
>> +    {
>> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image failed 
>> with %r.\n", Status));
>> +        goto cleanup;
>> +    }
>> +
>> +    if (Updateable != IMAGE_UPDATABLE_VALID)
>> +    {
>> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image 
>> returned that the Image was not valid for update.  Updatable value = 
>> 0x%X.\n", Updateable));
>> +        Status = EFI_ABORTED;
>> +        goto cleanup;
>> +    }
>> +
>> +    if (Progress == NULL)
>> +    {
>> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Invalid progress 
>> callback\n"));
>> +        Status = EFI_INVALID_PARAMETER;
>> +        goto cleanup;
>> +    }
>> +
>> +    Status = Progress(15);
>> +    if (EFI_ERROR(Status))
>> +    {
>> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Progress Callback 
>> failed with Status %r.\n", Status));
>> +    }
>> +
>> +    {
>> +      UINTN  p;
>> +
>> +      for (p = 20; p < 100; p++) {
>> +        gBS->Stall (100000);  //us  = 0.1 seconds
>> +        Progress (p);
>> +      }
>> +    }
>> +
>> +    //TODO: add support for VendorCode, and AbortReason
>> +cleanup:
>> +    if (EFI_ERROR (Status)) {
>> +      *LastAttemptStatus = 
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>> +    }
>> +
>> +    return Status;
>> +}// SetImageWithStatus()
>> +
>> +
>>   /**
>>   Updates the firmware image of the device.
>> @@ -396,51 +523,98 @@ IN  
>> UINT32                                           CapsuleFwVersion,
>>   OUT CHAR16                                           **AbortReason
>>   )
>>   {
>> -    EFI_STATUS Status                         = EFI_SUCCESS;
>> -    UINT32 Updateable                         = 0;
>> -
>> -    Status = FmpDeviceCheckImage(Image, ImageSize, &Updateable);
>> -    if (EFI_ERROR(Status))
>> -    {
>> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image failed with 
>> %r.\n", Status));
>> -        goto cleanup;
>> -    }
>> -
>> -    if (Updateable != IMAGE_UPDATABLE_VALID)
>> -    {
>> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image returned that the 
>> Image was not valid for update.  Updatable value = 0x%X.\n", 
>> Updateable));
>> -        Status = EFI_ABORTED;
>> -        goto cleanup;
>> -    }
>> -
>> -    if (Progress == NULL)
>> -    {
>> -        DEBUG((DEBUG_ERROR, "SetImage - Invalid progress callback\n"));
>> -        Status = EFI_INVALID_PARAMETER;
>> -        goto cleanup;
>> -    }
>> -
>> -    Status = Progress(15);
>> -    if (EFI_ERROR(Status))
>> -    {
>> -        DEBUG((DEBUG_ERROR, "SetImage - Progress Callback failed with 
>> Status %r.\n", Status));
>> -    }
>> -
>> -    {
>> -      UINTN  p;
>> -
>> -      for (p = 20; p < 100; p++) {
>> -        gBS->Stall (100000);  //us  = 0.1 seconds
>> -        Progress (p);
>> -      }
>> -    }
>> -
>> -    //TODO: add support for VendorCode, and AbortReason
>> -cleanup:
>> -    return Status;
>> +  UINT32  LastAttemptStatus;
>> +
>> +  return  FmpDeviceSetImageWithStatus (
>> +            Image,
>> +            ImageSize,
>> +            VendorCode,
>> +            Progress,
>> +            CapsuleFwVersion,
>> +            AbortReason,
>> +            &LastAttemptStatus
>> +            );
>>   }// SetImage()
>> +/**
>> +  Checks if a new firmware image is valid for the firmware device.  This
>> +  function allows firmware update operation to validate the firmware 
>> image
>> +  before FmpDeviceSetImage() is called.
>> +
>> +  @param[in]  Image               Points to a new firmware image.
>> +  @param[in]  ImageSize           Size, in bytes, of a new firmware 
>> image.
>> +  @param[out] ImageUpdatable      Indicates if a new firmware image 
>> is valid for
>> +                                  a firmware update to the firmware 
>> device.  The
>> +                                  following values from the Firmware 
>> Management
>> +                                  Protocol are supported:
>> +                                    IMAGE_UPDATABLE_VALID
>> +                                    IMAGE_UPDATABLE_INVALID
>> +                                    IMAGE_UPDATABLE_INVALID_TYPE
>> +                                    IMAGE_UPDATABLE_INVALID_OLD
>> +                                    
>> 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.
>> +
>> +                                  The return status code must fall in 
>> the range of
>> +                                  
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
>> +                                  
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
>> +
>> +                                  If the value falls outside this 
>> range, it will be converted
>> +                                  to 
>> LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
>> +
>> +  @retval EFI_SUCCESS            The image was successfully checked.  
>> Additional
>> +                                 status information is returned in
>> +                                 ImageUpdatable.
>> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
>> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
>> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +FmpDeviceCheckImageWithStatus (
>> +  IN  CONST VOID  *Image,
>> +  IN  UINTN       ImageSize,
>> +  OUT UINT32      *ImageUpdatable,
>> +  OUT UINT32      *LastAttemptStatus
>> +  )
>> +{
>> +    EFI_STATUS status = EFI_SUCCESS;
>> +
>> +    if (LastAttemptStatus == NULL) {
>> +      DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - LastAttemptStatus 
>> Pointer Parameter is NULL.\n"));
>> +      return EFI_INVALID_PARAMETER;
>> +    }
>> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
>> +
>> +    if (ImageUpdatable == NULL)
>> +    {
>> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - ImageUpdatable 
>> Pointer Parameter is NULL.\n"));
>> +        *LastAttemptStatus = 
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>> +        status = EFI_INVALID_PARAMETER;
>> +        goto cleanup;
>> +    }
>> +
>> +    //
>> +    //Set to valid and then if any tests fail it will update this flag.
>> +    //
>> +    *ImageUpdatable = IMAGE_UPDATABLE_VALID;
>> +
>> +    if (Image == NULL)
>> +    {
>> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer 
>> Parameter is NULL.\n"));
>> +        *ImageUpdatable = IMAGE_UPDATABLE_INVALID; //not sure if this 
>> is needed
>> +        *LastAttemptStatus = 
>> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>> +        return EFI_INVALID_PARAMETER;
>> +    }
>> +
>> +cleanup:
>> +    return status;
>> +}// CheckImageWithStatus()
>> +
>>   /**
>>   Checks if the firmware image is valid for the device.
>> @@ -465,29 +639,9 @@ IN  UINTN                             ImageSize,
>>   OUT UINT32                            *ImageUpdateable
>>   )
>>   {
>> -    EFI_STATUS status = EFI_SUCCESS;
>> +  UINT32  LastAttemptStatus;
>> -    if (ImageUpdateable == NULL)
>> -    {
>> -        DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer 
>> Parameter is NULL.\n"));
>> -        status = EFI_INVALID_PARAMETER;
>> -        goto cleanup;
>> -    }
>> -
>> -    //
>> -    //Set to valid and then if any tests fail it will update this flag.
>> -    //
>> -    *ImageUpdateable = IMAGE_UPDATABLE_VALID;
>> -
>> -    if (Image == NULL)
>> -    {
>> -        DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter is 
>> NULL.\n"));
>> -        *ImageUpdateable = IMAGE_UPDATABLE_INVALID; //not sure if 
>> this is needed
>> -        return EFI_INVALID_PARAMETER;
>> -    }
>> -
>> -cleanup:
>> -    return status;
>> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, 
>> ImageUpdateable, &LastAttemptStatus);
>>   }// CheckImage()
>>   /**
>>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
  2020-11-11  3:03     ` Michael D Kinney
@ 2020-11-11  4:16       ` Kilian Kegel
  2020-11-11  5:17         ` Michael Kubacki
  2020-11-11 17:17         ` Michael D Kinney
  0 siblings, 2 replies; 5+ messages in thread
From: Kilian Kegel @ 2020-11-11  4:16 UTC (permalink / raw)
  To: devel@edk2.groups.io, michael.d.kinney@intel.com, Michael Kubacki
  Cc: Qian, Yi, Sun, Zailiang

[-- Attachment #1: Type: text/plain, Size: 40358 bytes --]

Hi all,

if this patch also affects https://bugzilla.tianocore.org/show_bug.cgi?id=2983, please close it.

Thanks,
Kilian

Von: Michael D Kinney<mailto:michael.d.kinney@intel.com>
Gesendet: Wednesday, November 11, 2020 4:03 AM
An: Michael Kubacki<mailto:michael.kubacki@outlook.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>
Cc: Qian, Yi<mailto:yi.qian@intel.com>; Sun, Zailiang<mailto:zailiang.sun@intel.com>
Betreff: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility

Hi Michael,

This patch has been merged.

Thanks,

Mike

> -----Original Message-----
> From: Michael Kubacki <michael.kubacki@outlook.com>
> Sent: Thursday, November 5, 2020 12:36 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
> Cc: Qian, Yi <yi.qian@intel.com>; Sun, Zailiang <zailiang.sun@intel.com>
> Subject: Re: [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
>
> Hi Mike,
>
> I saw other Vlv2TbltDevicePkg patches were merged yesterday. Do you know
> when this patch will be merged?
>
> Thanks,
> Michael
>
> On 10/28/2020 6:12 PM, Kinney, Michael D wrote:
> > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
> >
> > I also verified build and boot and FMP based capsule updates work as expected.
> >
> > Tested-by: Michael D Kinney <Michael.d.kinney@intel.com>
> >
> > Thanks,
> >
> > Mike
> >
> >
> >> -----Original Message-----
> >> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
> >> Sent: Thursday, October 1, 2020 2:59 PM
> >> To: devel@edk2.groups.io
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Qian, Yi <yi.qian@intel.com>; Sun, Zailiang
> <zailiang.sun@intel.com>
> >> Subject: [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
> >>
> >> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>
> >> Makes the changes necessary for these library instances of
> >> FmpDeviceLib to be compatible with new functions added recently
> >> to FmpDeviceLib.
> >>
> >> Two new functions were introduced in FmpDeviceLib to allow a
> >> library instance to return a Last Attempt Status code during
> >> check image and set image operations:
> >>    1. FmpDeviceCheckImageWithStatus ( )
> >>    2. FmpDeviceSetImageWithStatus ( )
> >>
> >> FmpDxe (in FmpDevicePkg) will begin calling these new functions
> >> instead of the previous functions. Therefore, this change:
> >>    1. Adds these functions to Vlv2TbltDevicePkg implementations
> >>    2. Moves the main functionality to these new functions
> >>    3. Updates the old functions to call the new functions
> >>       (for backward compatibility)
> >>
> >> Note: As of this commit, the Vlv2TbltDevicePkg build is broken
> >> due to:
> >>    1. A required RngLib library instance not defined by the platform
> >>    2. Other FMP libraries not being defined by the platform
> >>       (e.g. FmpDependencyLib, FmpDependencyCheckLib, etc.)
> >>
> >> Those changes were fixed locally to test the changes in this commit
> >> but maintainers should make the proper changes for those issues.
> >>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >> Cc: Yi Qian <yi.qian@intel.com>
> >> Cc: Zailiang Sun <zailiang.sun@intel.com>
> >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >> ---
> >>
> >> Notes:
> >>      Only build was checked, I do not have access to a
> >>      VLV2 device for testing.
> >>
> >>   Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c       | 299 +++++++++++++++-----
> >>   Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c | 284 ++++++++++++++-----
> >>   2 files changed, 446 insertions(+), 137 deletions(-)
> >>
> >> diff --git a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> >> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> >> index d8c9036012ad..df8a36d9854c 100644
> >> --- a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> >> +++ b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> >> @@ -1,6 +1,6 @@
> >>   /**
> >>
> >> -Copyright (c) 2016, Microsoft Corporation.  All rights reserved.
> >> +Copyright (c) Microsoft Corporation.<BR>
> >>   Copyright (c) 2019, Intel Corporation.  All rights reserved.
> >>
> >>   SPDX-License-Identifier: BSD-2-Clause-Patent
> >> @@ -8,7 +8,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>   **/
> >>
> >>   #include <PiDxe.h>
> >> -
> >> +#include <LastAttemptStatus.h>
> >> +#include <Guid/SystemResourceTable.h>
> >>   #include <Library/FmpDeviceLib.h>
> >>
> >>   #include <Library/DebugLib.h>
> >> @@ -444,20 +445,17 @@ FmpDeviceGetImage (
> >>   }
> >>
> >>   /**
> >> -  Updates the firmware image of the device.
> >> -
> >> -  This function updates the hardware with the new firmware image.  This function
> >> -  returns EFI_UNSUPPORTED if the firmware image is not updatable.  If the
> >> -  firmware image is updatable, the function should perform the following minimal
> >> -  validations before proceeding to do the firmware image update.
> >> -    - Validate the image is a supported image for this device.  The function
> >> -      returns EFI_ABORTED if the image is unsupported.  The function can
> >> -      optionally provide more detailed information on why the image is not a
> >> -      supported image.
> >> -    - Validate the data from VendorCode if not null.  Image validation must be
> >> -      performed before VendorCode data validation.  VendorCode data is ignored
> >> -      or considered invalid if image validation failed.  The function returns
> >> -      EFI_ABORTED if the data is invalid.
> >> +  Updates a firmware device with a new firmware image.  This function returns
> >> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the firmware image
> >> +  is updatable, the function should perform the following minimal validations
> >> +  before proceeding to do the firmware image update.
> >> +    - Validate that the image is a supported image for this firmware device.
> >> +      Return EFI_ABORTED if the image is not supported.  Additional details
> >> +      on why the image is not a supported image may be returned in AbortReason.
> >> +    - Validate the data from VendorCode if is not NULL.  Firmware image
> >> +      validation must be performed before VendorCode data validation.
> >> +      VendorCode data is ignored or considered invalid if image validation
> >> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
> >>
> >>     VendorCode enables vendor to implement vendor-specific firmware image update
> >>     policy.  Null if the caller did not specify the policy or use the default
> >> @@ -470,38 +468,56 @@ FmpDeviceGetImage (
> >>     have the option to provide a more detailed description of the abort reason to
> >>     the caller.
> >>
> >> -  @param[in]  Image             Points to the new image.
> >> -  @param[in]  ImageSize         Size of the new image in bytes.
> >> +  @param[in]  Image             Points to the new firmware image.
> >> +  @param[in]  ImageSize         Size, in bytes, of the new firmware image.
> >>     @param[in]  VendorCode        This enables vendor to implement vendor-specific
> >> -                                firmware image update policy. Null indicates the
> >> -                                caller did not specify the policy or use the
> >> +                                firmware image update policy.  NULL indicates
> >> +                                the caller did not specify the policy or use the
> >>                                   default policy.
> >> -  @param[in]  Progress          A function used by the driver to report the
> >> -                                progress of the firmware update.
> >> -  @param[in]  CapsuleFwVersion  FMP Payload Header version of the image.
> >> -  @param[out] AbortReason       A pointer to a pointer to a null-terminated
> >> -                                string providing more details for the aborted
> >> -                                operation. The buffer is allocated by this
> >> -                                function with AllocatePool(), and it is the
> >> -                                caller's responsibility to free it with a call
> >> -                                to FreePool().
> >> +  @param[in]  Progress          A function used to report the progress of
> >> +                                updating the firmware device with the new
> >> +                                firmware image.
> >> +  @param[in]  CapsuleFwVersion  The version of the new firmware image from the
> >> +                                update capsule that provided the new firmware
> >> +                                image.
> >> +  @param[out] AbortReason       A pointer to a pointer to a Null-terminated
> >> +                                Unicode string providing more details on an
> >> +                                aborted operation. The buffer is allocated by
> >> +                                this function with
> >> +                                EFI_BOOT_SERVICES.AllocatePool().  It is the
> >> +                                caller's responsibility to free this buffer with
> >> +                                EFI_BOOT_SERVICES.FreePool().
> >> +  @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.
> >>
> >> -  @retval EFI_SUCCESS            The device was successfully updated with the
> >> -                                 new image.
> >> -  @retval EFI_ABORTED            The operation is aborted.
> >> +                                The return status code must fall in the range of
> >> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> >> +
> >> +                                If the value falls outside this range, it will be converted
> >> +                                to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> >> +
> >> +  @retval EFI_SUCCESS            The firmware device was successfully updated
> >> +                                 with the new firmware image.
> >> +  @retval EFI_ABORTED            The operation is aborted.  Additional details
> >> +                                 are provided in AbortReason.
> >>     @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
> >>     @retval EFI_UNSUPPORTED        The operation is not supported.
> >>
> >>   **/
> >>   EFI_STATUS
> >>   EFIAPI
> >> -FmpDeviceSetImage (
> >> +FmpDeviceSetImageWithStatus (
> >>     IN  CONST VOID                                     *Image,
> >>     IN  UINTN                                          ImageSize,
> >> -  IN  CONST VOID                                     *VendorCode,
> >> -  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
> >> +  IN  CONST VOID                                     *VendorCode,       OPTIONAL
> >> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,          OPTIONAL
> >>     IN  UINT32                                         CapsuleFwVersion,
> >> -  OUT CHAR16                                         **AbortReason
> >> +  OUT CHAR16                                         **AbortReason,
> >> +  OUT UINT32                                         *LastAttemptStatus
> >>     )
> >>   {
> >>     EFI_STATUS          Status;
> >> @@ -513,25 +529,27 @@ FmpDeviceSetImage (
> >>     UINTN               BytesWritten;
> >>
> >>     Updateable = 0;
> >> -  Status = FmpDeviceCheckImage (Image, ImageSize, &Updateable);
> >> +  Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, &Updateable, LastAttemptStatus);
> >>     if (EFI_ERROR (Status)) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image failed with %r.\n", Status));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check Image failed with %r.\n", Status));
> >>       return Status;
> >>     }
> >>
> >>     if (Updateable != IMAGE_UPDATABLE_VALID) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image returned that the Image was not valid for update.  Updatable
> >> value = 0x%X.\n", Updateable));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check Image returned that the Image was not valid for update.
> >> Updatable value = 0x%X.\n", Updateable));
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >>       return EFI_ABORTED;
> >>     }
> >>
> >>     if (Progress == NULL) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Invalid progress callback\n"));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Invalid progress callback\n"));
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >>       return EFI_INVALID_PARAMETER;
> >>     }
> >>
> >>     Status = Progress (15);
> >>     if (EFI_ERROR (Status)) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed with Status %r.\n", Status));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress Callback failed with Status %r.\n", Status));
> >>     }
> >>
> >>     //
> >> @@ -539,7 +557,7 @@ FmpDeviceSetImage (
> >>     //
> >>     Progress (20);
> >>     if (EFI_ERROR (Status)) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed with Status %r.\n", Status));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress Callback failed with Status %r.\n", Status));
> >>     }
> >>
> >>     //
> >> @@ -553,11 +571,11 @@ FmpDeviceSetImage (
> >>
> >>   //    Progress (Percentage);
> >>   //    if (EFI_ERROR (Status)) {
> >> -//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed with Status %r.\n", Status));
> >> +//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress Callback failed with Status %r.\n", Status));
> >>   //    }
> >>     }
> >>
> >> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %d Images ...\n", ARRAY_SIZE (mUpdateConfigData)));
> >> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %d Images ...\n", ARRAY_SIZE (mUpdateConfigData)));
> >>
> >>     if (ARRAY_SIZE (mUpdateConfigData) == 0) {
> >>       DEBUG((DEBUG_INFO, "PlatformUpdate: BaseAddress - 0x%lx ImageOffset - 0x%x Length - 0x%x\n", 0, 0, ImageSize));
> >> @@ -605,11 +623,173 @@ FmpDeviceSetImage (
> >>       BytesWritten += ConfigData->Length;
> >>     }
> >>
> >> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %r\n", Status));
> >> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %r\n", Status));
> >> +
> >> +  if (EFI_ERROR (Status)) {
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +  }
> >>
> >>     return Status;
> >>   }
> >>
> >> +/**
> >> +  Updates the firmware image of the device.
> >> +
> >> +  This function updates the hardware with the new firmware image.  This function
> >> +  returns EFI_UNSUPPORTED if the firmware image is not updatable.  If the
> >> +  firmware image is updatable, the function should perform the following minimal
> >> +  validations before proceeding to do the firmware image update.
> >> +    - Validate the image is a supported image for this device.  The function
> >> +      returns EFI_ABORTED if the image is unsupported.  The function can
> >> +      optionally provide more detailed information on why the image is not a
> >> +      supported image.
> >> +    - Validate the data from VendorCode if not null.  Image validation must be
> >> +      performed before VendorCode data validation.  VendorCode data is ignored
> >> +      or considered invalid if image validation failed.  The function returns
> >> +      EFI_ABORTED if the data is invalid.
> >> +
> >> +  VendorCode enables vendor to implement vendor-specific firmware image update
> >> +  policy.  Null if the caller did not specify the policy or use the default
> >> +  policy.  As an example, vendor can implement a policy to allow an option to
> >> +  force a firmware image update when the abort reason is due to the new firmware
> >> +  image version is older than the current firmware image version or bad image
> >> +  checksum.  Sensitive operations such as those wiping the entire firmware image
> >> +  and render the device to be non-functional should be encoded in the image
> >> +  itself rather than passed with the VendorCode.  AbortReason enables vendor to
> >> +  have the option to provide a more detailed description of the abort reason to
> >> +  the caller.
> >> +
> >> +  @param[in]  Image             Points to the new image.
> >> +  @param[in]  ImageSize         Size of the new image in bytes.
> >> +  @param[in]  VendorCode        This enables vendor to implement vendor-specific
> >> +                                firmware image update policy. Null indicates the
> >> +                                caller did not specify the policy or use the
> >> +                                default policy.
> >> +  @param[in]  Progress          A function used by the driver to report the
> >> +                                progress of the firmware update.
> >> +  @param[in]  CapsuleFwVersion  FMP Payload Header version of the image.
> >> +  @param[out] AbortReason       A pointer to a pointer to a null-terminated
> >> +                                string providing more details for the aborted
> >> +                                operation. The buffer is allocated by this
> >> +                                function with AllocatePool(), and it is the
> >> +                                caller's responsibility to free it with a call
> >> +                                to FreePool().
> >> +
> >> +  @retval EFI_SUCCESS            The device was successfully updated with the
> >> +                                 new image.
> >> +  @retval EFI_ABORTED            The operation is aborted.
> >> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_UNSUPPORTED        The operation is not supported.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +FmpDeviceSetImage (
> >> +  IN  CONST VOID                                     *Image,
> >> +  IN  UINTN                                          ImageSize,
> >> +  IN  CONST VOID                                     *VendorCode,
> >> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
> >> +  IN  UINT32                                         CapsuleFwVersion,
> >> +  OUT CHAR16                                         **AbortReason
> >> +  )
> >> +{
> >> +  UINT32  LastAttemptStatus;
> >> +
> >> +  return  FmpDeviceSetImageWithStatus (
> >> +            Image,
> >> +            ImageSize,
> >> +            VendorCode,
> >> +            Progress,
> >> +            CapsuleFwVersion,
> >> +            AbortReason,
> >> +            &LastAttemptStatus
> >> +            );
> >> +}
> >> +
> >> +/**
> >> +  Checks if a new firmware image is valid for the firmware device.  This
> >> +  function allows firmware update operation to validate the firmware image
> >> +  before FmpDeviceSetImage() is called.
> >> +
> >> +  @param[in]  Image               Points to a new firmware image.
> >> +  @param[in]  ImageSize           Size, in bytes, of a new firmware image.
> >> +  @param[out] ImageUpdatable      Indicates if a new firmware image is valid for
> >> +                                  a firmware update to the firmware device.  The
> >> +                                  following values from the Firmware Management
> >> +                                  Protocol are supported:
> >> +                                    IMAGE_UPDATABLE_VALID
> >> +                                    IMAGE_UPDATABLE_INVALID
> >> +                                    IMAGE_UPDATABLE_INVALID_TYPE
> >> +                                    IMAGE_UPDATABLE_INVALID_OLD
> >> +                                    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.
> >> +
> >> +                                  The return status code must fall in the range of
> >> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> >> +
> >> +                                  If the value falls outside this range, it will be converted
> >> +                                  to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> >> +
> >> +  @retval EFI_SUCCESS            The image was successfully checked.  Additional
> >> +                                 status information is returned in
> >> +                                 ImageUpdatable.
> >> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
> >> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
> >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +FmpDeviceCheckImageWithStatus (
> >> +  IN  CONST VOID  *Image,
> >> +  IN  UINTN       ImageSize,
> >> +  OUT UINT32      *ImageUpdatable,
> >> +  OUT UINT32      *LastAttemptStatus
> >> +  )
> >> +{
> >> +  if (LastAttemptStatus == NULL) {
> >> +    DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - LastAttemptStatus Pointer Parameter is NULL.\n"));
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +  *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
> >> +
> >> +  if (ImageUpdatable == NULL) {
> >> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - ImageUpdatable Pointer Parameter is NULL.\n"));
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +
> >> +  //
> >> +  //Set to valid and then if any tests fail it will update this flag.
> >> +  //
> >> +  *ImageUpdatable = IMAGE_UPDATABLE_VALID;
> >> +
> >> +  if (Image == NULL) {
> >> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer Parameter is NULL.\n"));
> >> +    //
> >> +    // Not sure if this is needed
> >> +    //
> >> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +
> >> +  //
> >> +  // Make sure the image size is correct
> >> +  //
> >> +  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
> >> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >>   /**
> >>   Checks if the firmware image is valid for the device.
> >>
> >> @@ -633,34 +813,9 @@ FmpDeviceCheckImage (
> >>     OUT UINT32      *ImageUpdateable
> >>     )
> >>   {
> >> -  if (ImageUpdateable == NULL) {
> >> -    DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer Parameter is NULL.\n"));
> >> -    return EFI_INVALID_PARAMETER;
> >> -  }
> >> +  UINT32  LastAttemptStatus;
> >>
> >> -  //
> >> -  //Set to valid and then if any tests fail it will update this flag.
> >> -  //
> >> -  *ImageUpdateable = IMAGE_UPDATABLE_VALID;
> >> -
> >> -  if (Image == NULL) {
> >> -    DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter is NULL.\n"));
> >> -    //
> >> -    // Not sure if this is needed
> >> -    //
> >> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
> >> -    return EFI_INVALID_PARAMETER;
> >> -  }
> >> -
> >> -  //
> >> -  // Make sure the image size is correct
> >> -  //
> >> -  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
> >> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
> >> -    return EFI_INVALID_PARAMETER;
> >> -  }
> >> -
> >> -  return EFI_SUCCESS;
> >> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, ImageUpdateable, &LastAttemptStatus);
> >>   }
> >>
> >>   /**
> >> diff --git a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> >> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> >> index db0f238ea534..132b60844ad4 100644
> >> --- a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> >> +++ b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> >> @@ -1,6 +1,6 @@
> >>   /**
> >>
> >> -Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
> >> +Copyright (c) Microsoft Corporation.<BR>
> >>   Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> >>
> >>   SPDX-License-Identifier: BSD-2-Clause-Patent
> >> @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>
> >>
> >>   #include <PiDxe.h>
> >> +#include <LastAttemptStatus.h>
> >> +#include <Guid/SystemResourceTable.h>
> >>   #include <Library/DebugLib.h>
> >>   #include <Protocol/FirmwareManagement.h>
> >>   #include <Library/BaseLib.h>
> >> @@ -345,6 +347,131 @@ Return Value:
> >>   }//GetImage()
> >>
> >>
> >> +/**
> >> +  Updates a firmware device with a new firmware image.  This function returns
> >> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the firmware image
> >> +  is updatable, the function should perform the following minimal validations
> >> +  before proceeding to do the firmware image update.
> >> +    - Validate that the image is a supported image for this firmware device.
> >> +      Return EFI_ABORTED if the image is not supported.  Additional details
> >> +      on why the image is not a supported image may be returned in AbortReason.
> >> +    - Validate the data from VendorCode if is not NULL.  Firmware image
> >> +      validation must be performed before VendorCode data validation.
> >> +      VendorCode data is ignored or considered invalid if image validation
> >> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
> >> +
> >> +  VendorCode enables vendor to implement vendor-specific firmware image update
> >> +  policy.  Null if the caller did not specify the policy or use the default
> >> +  policy.  As an example, vendor can implement a policy to allow an option to
> >> +  force a firmware image update when the abort reason is due to the new firmware
> >> +  image version is older than the current firmware image version or bad image
> >> +  checksum.  Sensitive operations such as those wiping the entire firmware image
> >> +  and render the device to be non-functional should be encoded in the image
> >> +  itself rather than passed with the VendorCode.  AbortReason enables vendor to
> >> +  have the option to provide a more detailed description of the abort reason to
> >> +  the caller.
> >> +
> >> +  @param[in]  Image             Points to the new firmware image.
> >> +  @param[in]  ImageSize         Size, in bytes, of the new firmware image.
> >> +  @param[in]  VendorCode        This enables vendor to implement vendor-specific
> >> +                                firmware image update policy.  NULL indicates
> >> +                                the caller did not specify the policy or use the
> >> +                                default policy.
> >> +  @param[in]  Progress          A function used to report the progress of
> >> +                                updating the firmware device with the new
> >> +                                firmware image.
> >> +  @param[in]  CapsuleFwVersion  The version of the new firmware image from the
> >> +                                update capsule that provided the new firmware
> >> +                                image.
> >> +  @param[out] AbortReason       A pointer to a pointer to a Null-terminated
> >> +                                Unicode string providing more details on an
> >> +                                aborted operation. The buffer is allocated by
> >> +                                this function with
> >> +                                EFI_BOOT_SERVICES.AllocatePool().  It is the
> >> +                                caller's responsibility to free this buffer with
> >> +                                EFI_BOOT_SERVICES.FreePool().
> >> +  @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.
> >> +
> >> +                                The return status code must fall in the range of
> >> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> >> +
> >> +                                If the value falls outside this range, it will be converted
> >> +                                to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> >> +
> >> +  @retval EFI_SUCCESS            The firmware device was successfully updated
> >> +                                 with the new firmware image.
> >> +  @retval EFI_ABORTED            The operation is aborted.  Additional details
> >> +                                 are provided in AbortReason.
> >> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
> >> +  @retval EFI_UNSUPPORTED        The operation is not supported.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +FmpDeviceSetImageWithStatus (
> >> +  IN  CONST VOID                                     *Image,
> >> +  IN  UINTN                                          ImageSize,
> >> +  IN  CONST VOID                                     *VendorCode,       OPTIONAL
> >> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,          OPTIONAL
> >> +  IN  UINT32                                         CapsuleFwVersion,
> >> +  OUT CHAR16                                         **AbortReason,
> >> +  OUT UINT32                                         *LastAttemptStatus
> >> +  )
> >> +{
> >> +    EFI_STATUS Status                         = EFI_SUCCESS;
> >> +    UINT32 Updateable                         = 0;
> >> +
> >> +    Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, &Updateable, LastAttemptStatus);
> >> +    if (EFI_ERROR(Status))
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image failed with %r.\n", Status));
> >> +        goto cleanup;
> >> +    }
> >> +
> >> +    if (Updateable != IMAGE_UPDATABLE_VALID)
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image returned that the Image was not valid for update.
> >> Updatable value = 0x%X.\n", Updateable));
> >> +        Status = EFI_ABORTED;
> >> +        goto cleanup;
> >> +    }
> >> +
> >> +    if (Progress == NULL)
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Invalid progress callback\n"));
> >> +        Status = EFI_INVALID_PARAMETER;
> >> +        goto cleanup;
> >> +    }
> >> +
> >> +    Status = Progress(15);
> >> +    if (EFI_ERROR(Status))
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Progress Callback failed with Status %r.\n", Status));
> >> +    }
> >> +
> >> +    {
> >> +      UINTN  p;
> >> +
> >> +      for (p = 20; p < 100; p++) {
> >> +        gBS->Stall (100000);  //us  = 0.1 seconds
> >> +        Progress (p);
> >> +      }
> >> +    }
> >> +
> >> +    //TODO: add support for VendorCode, and AbortReason
> >> +cleanup:
> >> +    if (EFI_ERROR (Status)) {
> >> +      *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +    }
> >> +
> >> +    return Status;
> >> +}// SetImageWithStatus()
> >> +
> >> +
> >>   /**
> >>   Updates the firmware image of the device.
> >>
> >> @@ -396,51 +523,98 @@ IN  UINT32                                           CapsuleFwVersion,
> >>   OUT CHAR16                                           **AbortReason
> >>   )
> >>   {
> >> -    EFI_STATUS Status                         = EFI_SUCCESS;
> >> -    UINT32 Updateable                         = 0;
> >> -
> >> -    Status = FmpDeviceCheckImage(Image, ImageSize, &Updateable);
> >> -    if (EFI_ERROR(Status))
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image failed with %r.\n", Status));
> >> -        goto cleanup;
> >> -    }
> >> -
> >> -    if (Updateable != IMAGE_UPDATABLE_VALID)
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image returned that the Image was not valid for update.  Updatable value
> =
> >> 0x%X.\n", Updateable));
> >> -        Status = EFI_ABORTED;
> >> -        goto cleanup;
> >> -    }
> >> -
> >> -    if (Progress == NULL)
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "SetImage - Invalid progress callback\n"));
> >> -        Status = EFI_INVALID_PARAMETER;
> >> -        goto cleanup;
> >> -    }
> >> -
> >> -    Status = Progress(15);
> >> -    if (EFI_ERROR(Status))
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "SetImage - Progress Callback failed with Status %r.\n", Status));
> >> -    }
> >> -
> >> -    {
> >> -      UINTN  p;
> >> -
> >> -      for (p = 20; p < 100; p++) {
> >> -        gBS->Stall (100000);  //us  = 0.1 seconds
> >> -        Progress (p);
> >> -      }
> >> -    }
> >> -
> >> -    //TODO: add support for VendorCode, and AbortReason
> >> -cleanup:
> >> -    return Status;
> >> +  UINT32  LastAttemptStatus;
> >> +
> >> +  return  FmpDeviceSetImageWithStatus (
> >> +            Image,
> >> +            ImageSize,
> >> +            VendorCode,
> >> +            Progress,
> >> +            CapsuleFwVersion,
> >> +            AbortReason,
> >> +            &LastAttemptStatus
> >> +            );
> >>   }// SetImage()
> >>
> >>
> >> +/**
> >> +  Checks if a new firmware image is valid for the firmware device.  This
> >> +  function allows firmware update operation to validate the firmware image
> >> +  before FmpDeviceSetImage() is called.
> >> +
> >> +  @param[in]  Image               Points to a new firmware image.
> >> +  @param[in]  ImageSize           Size, in bytes, of a new firmware image.
> >> +  @param[out] ImageUpdatable      Indicates if a new firmware image is valid for
> >> +                                  a firmware update to the firmware device.  The
> >> +                                  following values from the Firmware Management
> >> +                                  Protocol are supported:
> >> +                                    IMAGE_UPDATABLE_VALID
> >> +                                    IMAGE_UPDATABLE_INVALID
> >> +                                    IMAGE_UPDATABLE_INVALID_TYPE
> >> +                                    IMAGE_UPDATABLE_INVALID_OLD
> >> +                                    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.
> >> +
> >> +                                  The return status code must fall in the range of
> >> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> >> +
> >> +                                  If the value falls outside this range, it will be converted
> >> +                                  to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> >> +
> >> +  @retval EFI_SUCCESS            The image was successfully checked.  Additional
> >> +                                 status information is returned in
> >> +                                 ImageUpdatable.
> >> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
> >> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
> >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +FmpDeviceCheckImageWithStatus (
> >> +  IN  CONST VOID  *Image,
> >> +  IN  UINTN       ImageSize,
> >> +  OUT UINT32      *ImageUpdatable,
> >> +  OUT UINT32      *LastAttemptStatus
> >> +  )
> >> +{
> >> +    EFI_STATUS status = EFI_SUCCESS;
> >> +
> >> +    if (LastAttemptStatus == NULL) {
> >> +      DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - LastAttemptStatus Pointer Parameter is NULL.\n"));
> >> +      return EFI_INVALID_PARAMETER;
> >> +    }
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
> >> +
> >> +    if (ImageUpdatable == NULL)
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - ImageUpdatable Pointer Parameter is NULL.\n"));
> >> +        *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +        status = EFI_INVALID_PARAMETER;
> >> +        goto cleanup;
> >> +    }
> >> +
> >> +    //
> >> +    //Set to valid and then if any tests fail it will update this flag.
> >> +    //
> >> +    *ImageUpdatable = IMAGE_UPDATABLE_VALID;
> >> +
> >> +    if (Image == NULL)
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer Parameter is NULL.\n"));
> >> +        *ImageUpdatable = IMAGE_UPDATABLE_INVALID; //not sure if this is needed
> >> +        *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +        return EFI_INVALID_PARAMETER;
> >> +    }
> >> +
> >> +cleanup:
> >> +    return status;
> >> +}// CheckImageWithStatus()
> >> +
> >>
> >>   /**
> >>   Checks if the firmware image is valid for the device.
> >> @@ -465,29 +639,9 @@ IN  UINTN                             ImageSize,
> >>   OUT UINT32                            *ImageUpdateable
> >>   )
> >>   {
> >> -    EFI_STATUS status = EFI_SUCCESS;
> >> +  UINT32  LastAttemptStatus;
> >>
> >> -    if (ImageUpdateable == NULL)
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer Parameter is NULL.\n"));
> >> -        status = EFI_INVALID_PARAMETER;
> >> -        goto cleanup;
> >> -    }
> >> -
> >> -    //
> >> -    //Set to valid and then if any tests fail it will update this flag.
> >> -    //
> >> -    *ImageUpdateable = IMAGE_UPDATABLE_VALID;
> >> -
> >> -    if (Image == NULL)
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter is NULL.\n"));
> >> -        *ImageUpdateable = IMAGE_UPDATABLE_INVALID; //not sure if this is needed
> >> -        return EFI_INVALID_PARAMETER;
> >> -    }
> >> -
> >> -cleanup:
> >> -    return status;
> >> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, ImageUpdateable, &LastAttemptStatus);
> >>   }// CheckImage()
> >>
> >>   /**
> >> --
> >> 2.28.0.windows.1
> >






[-- Attachment #2: Type: text/html, Size: 84322 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
  2020-11-11  4:16       ` [edk2-devel] " Kilian Kegel
@ 2020-11-11  5:17         ` Michael Kubacki
  2020-11-11 17:17         ` Michael D Kinney
  1 sibling, 0 replies; 5+ messages in thread
From: Michael Kubacki @ 2020-11-11  5:17 UTC (permalink / raw)
  To: Kilian Kegel, devel@edk2.groups.io, michael.d.kinney@intel.com
  Cc: Qian, Yi, Sun, Zailiang

Hi Kilian,

That was broken due to FmpDevicePkg changes unrelated to these Last 
Attempt Status changes. I ran into the same isues as you when attempting 
to test this.


I believe Mike made those fixes in the following two commits:
1. 2fcfe4763022c1665be8e6a11f80aa69f0a58dce
2. b0de06c7d8494d05b315fb4a2574664f151e108d

Thanks,
Michael

On 11/10/2020 8:16 PM, Kilian Kegel wrote:
> Hi all,
> 
> if this patch also affects 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2983 
> <https://bugzilla.tianocore.org/show_bug.cgi?id=2983>, please close it.
> 
> Thanks,
> 
> Kilian
> 
> *Von: *Michael D Kinney <mailto:michael.d.kinney@intel.com>
> *Gesendet: *Wednesday, November 11, 2020 4:03 AM
> *An: *Michael Kubacki <mailto:michael.kubacki@outlook.com>; 
> devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Kinney, Michael D 
> <mailto:michael.d.kinney@intel.com>
> *Cc: *Qian, Yi <mailto:yi.qian@intel.com>; Sun, Zailiang 
> <mailto:zailiang.sun@intel.com>
> *Betreff: *Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] 
> Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
> 
> Hi Michael,
> 
> This patch has been merged.
> 
> Thanks,
> 
> Mike
> 
>  > -----Original Message-----
>  > From: Michael Kubacki <michael.kubacki@outlook.com>
>  > Sent: Thursday, November 5, 2020 12:36 PM
>  > To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
>  > Cc: Qian, Yi <yi.qian@intel.com>; Sun, Zailiang <zailiang.sun@intel.com>
>  > Subject: Re: [edk2-platforms][PATCH v1 1/1] 
> Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
>  >
>  > Hi Mike,
>  >
>  > I saw other Vlv2TbltDevicePkg patches were merged yesterday. Do you know
>  > when this patch will be merged?
>  >
>  > Thanks,
>  > Michael
>  >
>  > On 10/28/2020 6:12 PM, Kinney, Michael D wrote:
>  > > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
>  > >
>  > > I also verified build and boot and FMP based capsule updates work 
> as expected.
>  > >
>  > > Tested-by: Michael D Kinney <Michael.d.kinney@intel.com>
>  > >
>  > > Thanks,
>  > >
>  > > Mike
>  > >
>  > >
>  > >> -----Original Message-----
>  > >> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>  > >> Sent: Thursday, October 1, 2020 2:59 PM
>  > >> To: devel@edk2.groups.io
>  > >> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Qian, Yi 
> <yi.qian@intel.com>; Sun, Zailiang
>  > <zailiang.sun@intel.com>
>  > >> Subject: [edk2-platforms][PATCH v1 1/1] 
> Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
>  > >>
>  > >> From: Michael Kubacki <michael.kubacki@microsoft.com>
>  > >>
>  > >> Makes the changes necessary for these library instances of
>  > >> FmpDeviceLib to be compatible with new functions added recently
>  > >> to FmpDeviceLib.
>  > >>
>  > >> Two new functions were introduced in FmpDeviceLib to allow a
>  > >> library instance to return a Last Attempt Status code during
>  > >> check image and set image operations:
>  > >>    1. FmpDeviceCheckImageWithStatus ( )
>  > >>    2. FmpDeviceSetImageWithStatus ( )
>  > >>
>  > >> FmpDxe (in FmpDevicePkg) will begin calling these new functions
>  > >> instead of the previous functions. Therefore, this change:
>  > >>    1. Adds these functions to Vlv2TbltDevicePkg implementations
>  > >>    2. Moves the main functionality to these new functions
>  > >>    3. Updates the old functions to call the new functions
>  > >>       (for backward compatibility)
>  > >>
>  > >> Note: As of this commit, the Vlv2TbltDevicePkg build is broken
>  > >> due to:
>  > >>    1. A required RngLib library instance not defined by the platform
>  > >>    2. Other FMP libraries not being defined by the platform
>  > >>       (e.g. FmpDependencyLib, FmpDependencyCheckLib, etc.)
>  > >>
>  > >> Those changes were fixed locally to test the changes in this commit
>  > >> but maintainers should make the proper changes for those issues.
>  > >>
>  > >> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>  > >> Cc: Yi Qian <yi.qian@intel.com>
>  > >> Cc: Zailiang Sun <zailiang.sun@intel.com>
>  > >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>  > >> ---
>  > >>
>  > >> Notes:
>  > >>      Only build was checked, I do not have access to a
>  > >>      VLV2 device for testing.
>  > >>
>  > >>   
> Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c       
> | 299 +++++++++++++++-----
>  > >>   
> Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c 
> | 284 ++++++++++++++-----
>  > >>   2 files changed, 446 insertions(+), 137 deletions(-)
>  > >>
>  > >> diff --git 
> a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
>  > >> 
> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
>  > >> index d8c9036012ad..df8a36d9854c 100644
>  > >> --- 
> a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
>  > >> +++ 
> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
>  > >> @@ -1,6 +1,6 @@
>  > >>   /**
>  > >>
>  > >> -Copyright (c) 2016, Microsoft Corporation.  All rights reserved.
>  > >> +Copyright (c) Microsoft Corporation.<BR>
>  > >>   Copyright (c) 2019, Intel Corporation.  All rights reserved.
>  > >>
>  > >>   SPDX-License-Identifier: BSD-2-Clause-Patent
>  > >> @@ -8,7 +8,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  > >>   **/
>  > >>
>  > >>   #include <PiDxe.h>
>  > >> -
>  > >> +#include <LastAttemptStatus.h>
>  > >> +#include <Guid/SystemResourceTable.h>
>  > >>   #include <Library/FmpDeviceLib.h>
>  > >>
>  > >>   #include <Library/DebugLib.h>
>  > >> @@ -444,20 +445,17 @@ FmpDeviceGetImage (
>  > >>   }
>  > >>
>  > >>   /**
>  > >> -  Updates the firmware image of the device.
>  > >> -
>  > >> -  This function updates the hardware with the new firmware 
> image.  This function
>  > >> -  returns EFI_UNSUPPORTED if the firmware image is not 
> updatable.  If the
>  > >> -  firmware image is updatable, the function should perform the 
> following minimal
>  > >> -  validations before proceeding to do the firmware image update.
>  > >> -    - Validate the image is a supported image for this device.
> The function
>  > >> -      returns EFI_ABORTED if the image is unsupported.  The 
> function can
>  > >> -      optionally provide more detailed information on why the 
> image is not a
>  > >> -      supported image.
>  > >> -    - Validate the data from VendorCode if not null.  Image 
> validation must be
>  > >> -      performed before VendorCode data validation.  VendorCode 
> data is ignored
>  > >> -      or considered invalid if image validation failed.  The 
> function returns
>  > >> -      EFI_ABORTED if the data is invalid.
>  > >> +  Updates a firmware device with a new firmware image.  This 
> function returns
>  > >> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the 
> firmware image
>  > >> +  is updatable, the function should perform the following minimal 
> validations
>  > >> +  before proceeding to do the firmware image update.
>  > >> +    - Validate that the image is a supported image for this 
> firmware device.
>  > >> +      Return EFI_ABORTED if the image is not supported.
> Additional details
>  > >> +      on why the image is not a supported image may be returned 
> in AbortReason.
>  > >> +    - Validate the data from VendorCode if is not NULL.  Firmware 
> image
>  > >> +      validation must be performed before VendorCode data validation.
>  > >> +      VendorCode data is ignored or considered invalid if image 
> validation
>  > >> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
>  > >>
>  > >>     VendorCode enables vendor to implement vendor-specific 
> firmware image update
>  > >>     policy.  Null if the caller did not specify the policy or use 
> the default
>  > >> @@ -470,38 +468,56 @@ FmpDeviceGetImage (
>  > >>     have the option to provide a more detailed description of the 
> abort reason to
>  > >>     the caller.
>  > >>
>  > >> -  @param[in]  Image             Points to the new image.
>  > >> -  @param[in]  ImageSize         Size of the new image in bytes.
>  > >> +  @param[in]  Image             Points to the new firmware image.
>  > >> +  @param[in]  ImageSize         Size, in bytes, of the new 
> firmware image.
>  > >>     @param[in]  VendorCode        This enables vendor to implement 
> vendor-specific
>  > >> -                                firmware image update policy. 
> Null indicates the
>  > >> -                                caller did not specify the policy 
> or use the
>  > >> +                                firmware image update policy.
> NULL indicates
>  > >> +                                the caller did not specify the 
> policy or use the
>  > >>                                   default policy.
>  > >> -  @param[in]  Progress          A function used by the driver to 
> report the
>  > >> -                                progress of the firmware update.
>  > >> -  @param[in]  CapsuleFwVersion  FMP Payload Header version of the 
> image.
>  > >> -  @param[out] AbortReason       A pointer to a pointer to a 
> null-terminated
>  > >> -                                string providing more details for 
> the aborted
>  > >> -                                operation. The buffer is 
> allocated by this
>  > >> -                                function with AllocatePool(), and 
> it is the
>  > >> -                                caller's responsibility to free 
> it with a call
>  > >> -                                to FreePool().
>  > >> +  @param[in]  Progress          A function used to report the 
> progress of
>  > >> +                                updating the firmware device with 
> the new
>  > >> +                                firmware image.
>  > >> +  @param[in]  CapsuleFwVersion  The version of the new firmware 
> image from the
>  > >> +                                update capsule that provided the 
> new firmware
>  > >> +                                image.
>  > >> +  @param[out] AbortReason       A pointer to a pointer to a 
> Null-terminated
>  > >> +                                Unicode string providing more 
> details on an
>  > >> +                                aborted operation. The buffer is 
> allocated by
>  > >> +                                this function with
>  > >> +
> EFI_BOOT_SERVICES.AllocatePool().  It is the
>  > >> +                                caller's responsibility to free 
> this buffer with
>  > >> +                                EFI_BOOT_SERVICES.FreePool().
>  > >> +  @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.
>  > >>
>  > >> -  @retval EFI_SUCCESS            The device was successfully 
> updated with the
>  > >> -                                 new image.
>  > >> -  @retval EFI_ABORTED            The operation is aborted.
>  > >> +                                The return status code must fall 
> in the range of
>  > >> +
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
>  > >> +
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
>  > >> +
>  > >> +                                If the value falls outside this 
> range, it will be converted
>  > >> +                                to 
> LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
>  > >> +
>  > >> +  @retval EFI_SUCCESS            The firmware device was 
> successfully updated
>  > >> +                                 with the new firmware image.
>  > >> +  @retval EFI_ABORTED            The operation is aborted.
> Additional details
>  > >> +                                 are provided in AbortReason.
>  > >>     @retval EFI_INVALID_PARAMETER  The Image was NULL.
>  > >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
>  > >>     @retval EFI_UNSUPPORTED        The operation is not supported.
>  > >>
>  > >>   **/
>  > >>   EFI_STATUS
>  > >>   EFIAPI
>  > >> -FmpDeviceSetImage (
>  > >> +FmpDeviceSetImageWithStatus (
>  > >>     IN  CONST VOID                                     *Image,
>  > >>     IN  UINTN                                          ImageSize,
>  > >> -  IN  CONST VOID                                     *VendorCode,
>  > >> -  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
>  > >> +  IN  CONST VOID                                     
> *VendorCode,       OPTIONAL
>  > >> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS
> Progress,          OPTIONAL
>  > >>     IN  UINT32                                         
> CapsuleFwVersion,
>  > >> -  OUT CHAR16                                         **AbortReason
>  > >> +  OUT CHAR16                                         **AbortReason,
>  > >> +  OUT UINT32                                         
> *LastAttemptStatus
>  > >>     )
>  > >>   {
>  > >>     EFI_STATUS          Status;
>  > >> @@ -513,25 +529,27 @@ FmpDeviceSetImage (
>  > >>     UINTN               BytesWritten;
>  > >>
>  > >>     Updateable = 0;
>  > >> -  Status = FmpDeviceCheckImage (Image, ImageSize, &Updateable);
>  > >> +  Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, 
> &Updateable, LastAttemptStatus);
>  > >>     if (EFI_ERROR (Status)) {
>  > >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image failed 
> with %r.\n", Status));
>  > >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check 
> Image failed with %r.\n", Status));
>  > >>       return Status;
>  > >>     }
>  > >>
>  > >>     if (Updateable != IMAGE_UPDATABLE_VALID) {
>  > >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image returned 
> that the Image was not valid for update.  Updatable
>  > >> value = 0x%X.\n", Updateable));
>  > >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check 
> Image returned that the Image was not valid for update.
>  > >> Updatable value = 0x%X.\n", Updateable));
>  > >> +    *LastAttemptStatus = 
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>  > >>       return EFI_ABORTED;
>  > >>     }
>  > >>
>  > >>     if (Progress == NULL) {
>  > >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Invalid progress 
> callback\n"));
>  > >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Invalid 
> progress callback\n"));
>  > >> +    *LastAttemptStatus = 
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>  > >>       return EFI_INVALID_PARAMETER;
>  > >>     }
>  > >>
>  > >>     Status = Progress (15);
>  > >>     if (EFI_ERROR (Status)) {
>  > >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback 
> failed with Status %r.\n", Status));
>  > >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress 
> Callback failed with Status %r.\n", Status));
>  > >>     }
>  > >>
>  > >>     //
>  > >> @@ -539,7 +557,7 @@ FmpDeviceSetImage (
>  > >>     //
>  > >>     Progress (20);
>  > >>     if (EFI_ERROR (Status)) {
>  > >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback 
> failed with Status %r.\n", Status));
>  > >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress 
> Callback failed with Status %r.\n", Status));
>  > >>     }
>  > >>
>  > >>     //
>  > >> @@ -553,11 +571,11 @@ FmpDeviceSetImage (
>  > >>
>  > >>   //    Progress (Percentage);
>  > >>   //    if (EFI_ERROR (Status)) {
>  > >> -//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress 
> Callback failed with Status %r.\n", Status));
>  > >> +//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - 
> Progress Callback failed with Status %r.\n", Status));
>  > >>   //    }
>  > >>     }
>  > >>
>  > >> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %d Images ...\n", 
> ARRAY_SIZE (mUpdateConfigData)));
>  > >> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %d Images 
> ...\n", ARRAY_SIZE (mUpdateConfigData)));
>  > >>
>  > >>     if (ARRAY_SIZE (mUpdateConfigData) == 0) {
>  > >>       DEBUG((DEBUG_INFO, "PlatformUpdate: BaseAddress - 0x%lx 
> ImageOffset - 0x%x Length - 0x%x\n", 0, 0, ImageSize));
>  > >> @@ -605,11 +623,173 @@ FmpDeviceSetImage (
>  > >>       BytesWritten += ConfigData->Length;
>  > >>     }
>  > >>
>  > >> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %r\n", Status));
>  > >> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %r\n", Status));
>  > >> +
>  > >> +  if (EFI_ERROR (Status)) {
>  > >> +    *LastAttemptStatus = 
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>  > >> +  }
>  > >>
>  > >>     return Status;
>  > >>   }
>  > >>
>  > >> +/**
>  > >> +  Updates the firmware image of the device.
>  > >> +
>  > >> +  This function updates the hardware with the new firmware 
> image.  This function
>  > >> +  returns EFI_UNSUPPORTED if the firmware image is not 
> updatable.  If the
>  > >> +  firmware image is updatable, the function should perform the 
> following minimal
>  > >> +  validations before proceeding to do the firmware image update.
>  > >> +    - Validate the image is a supported image for this device.
> The function
>  > >> +      returns EFI_ABORTED if the image is unsupported.  The 
> function can
>  > >> +      optionally provide more detailed information on why the 
> image is not a
>  > >> +      supported image.
>  > >> +    - Validate the data from VendorCode if not null.  Image 
> validation must be
>  > >> +      performed before VendorCode data validation.  VendorCode 
> data is ignored
>  > >> +      or considered invalid if image validation failed.  The 
> function returns
>  > >> +      EFI_ABORTED if the data is invalid.
>  > >> +
>  > >> +  VendorCode enables vendor to implement vendor-specific firmware 
> image update
>  > >> +  policy.  Null if the caller did not specify the policy or use 
> the default
>  > >> +  policy.  As an example, vendor can implement a policy to allow 
> an option to
>  > >> +  force a firmware image update when the abort reason is due to 
> the new firmware
>  > >> +  image version is older than the current firmware image version 
> or bad image
>  > >> +  checksum.  Sensitive operations such as those wiping the entire 
> firmware image
>  > >> +  and render the device to be non-functional should be encoded in 
> the image
>  > >> +  itself rather than passed with the VendorCode.  AbortReason 
> enables vendor to
>  > >> +  have the option to provide a more detailed description of the 
> abort reason to
>  > >> +  the caller.
>  > >> +
>  > >> +  @param[in]  Image             Points to the new image.
>  > >> +  @param[in]  ImageSize         Size of the new image in bytes.
>  > >> +  @param[in]  VendorCode        This enables vendor to implement 
> vendor-specific
>  > >> +                                firmware image update policy. 
> Null indicates the
>  > >> +                                caller did not specify the policy 
> or use the
>  > >> +                                default policy.
>  > >> +  @param[in]  Progress          A function used by the driver to 
> report the
>  > >> +                                progress of the firmware update.
>  > >> +  @param[in]  CapsuleFwVersion  FMP Payload Header version of the 
> image.
>  > >> +  @param[out] AbortReason       A pointer to a pointer to a 
> null-terminated
>  > >> +                                string providing more details for 
> the aborted
>  > >> +                                operation. The buffer is 
> allocated by this
>  > >> +                                function with AllocatePool(), and 
> it is the
>  > >> +                                caller's responsibility to free 
> it with a call
>  > >> +                                to FreePool().
>  > >> +
>  > >> +  @retval EFI_SUCCESS            The device was successfully 
> updated with the
>  > >> +                                 new image.
>  > >> +  @retval EFI_ABORTED            The operation is aborted.
>  > >> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>  > >> +  @retval EFI_UNSUPPORTED        The operation is not supported.
>  > >> +
>  > >> +**/
>  > >> +EFI_STATUS
>  > >> +EFIAPI
>  > >> +FmpDeviceSetImage (
>  > >> +  IN  CONST VOID                                     *Image,
>  > >> +  IN  UINTN                                          ImageSize,
>  > >> +  IN  CONST VOID                                     *VendorCode,
>  > >> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
>  > >> +  IN  UINT32                                         
> CapsuleFwVersion,
>  > >> +  OUT CHAR16                                         **AbortReason
>  > >> +  )
>  > >> +{
>  > >> +  UINT32  LastAttemptStatus;
>  > >> +
>  > >> +  return  FmpDeviceSetImageWithStatus (
>  > >> +            Image,
>  > >> +            ImageSize,
>  > >> +            VendorCode,
>  > >> +            Progress,
>  > >> +            CapsuleFwVersion,
>  > >> +            AbortReason,
>  > >> +            &LastAttemptStatus
>  > >> +            );
>  > >> +}
>  > >> +
>  > >> +/**
>  > >> +  Checks if a new firmware image is valid for the firmware 
> device.  This
>  > >> +  function allows firmware update operation to validate the 
> firmware image
>  > >> +  before FmpDeviceSetImage() is called.
>  > >> +
>  > >> +  @param[in]  Image               Points to a new firmware image.
>  > >> +  @param[in]  ImageSize           Size, in bytes, of a new 
> firmware image.
>  > >> +  @param[out] ImageUpdatable      Indicates if a new firmware 
> image is valid for
>  > >> +                                  a firmware update to the 
> firmware device.  The
>  > >> +                                  following values from the 
> Firmware Management
>  > >> +                                  Protocol are supported:
>  > >> +                                    IMAGE_UPDATABLE_VALID
>  > >> +                                    IMAGE_UPDATABLE_INVALID
>  > >> +                                    IMAGE_UPDATABLE_INVALID_TYPE
>  > >> +                                    IMAGE_UPDATABLE_INVALID_OLD
>  > >> +
> 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.
>  > >> +
>  > >> +                                  The return status code must 
> fall in the range of
>  > >> +
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
>  > >> +
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
>  > >> +
>  > >> +                                  If the value falls outside this 
> range, it will be converted
>  > >> +                                  to 
> LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
>  > >> +
>  > >> +  @retval EFI_SUCCESS            The image was successfully 
> checked.  Additional
>  > >> +                                 status information is returned in
>  > >> +                                 ImageUpdatable.
>  > >> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
>  > >> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
>  > >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
>  > >> +
>  > >> +**/
>  > >> +EFI_STATUS
>  > >> +EFIAPI
>  > >> +FmpDeviceCheckImageWithStatus (
>  > >> +  IN  CONST VOID  *Image,
>  > >> +  IN  UINTN       ImageSize,
>  > >> +  OUT UINT32      *ImageUpdatable,
>  > >> +  OUT UINT32      *LastAttemptStatus
>  > >> +  )
>  > >> +{
>  > >> +  if (LastAttemptStatus == NULL) {
>  > >> +    DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - 
> LastAttemptStatus Pointer Parameter is NULL.\n"));
>  > >> +    return EFI_INVALID_PARAMETER;
>  > >> +  }
>  > >> +  *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
>  > >> +
>  > >> +  if (ImageUpdatable == NULL) {
>  > >> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - ImageUpdatable 
> Pointer Parameter is NULL.\n"));
>  > >> +    *LastAttemptStatus = 
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>  > >> +    return EFI_INVALID_PARAMETER;
>  > >> +  }
>  > >> +
>  > >> +  //
>  > >> +  //Set to valid and then if any tests fail it will update this flag.
>  > >> +  //
>  > >> +  *ImageUpdatable = IMAGE_UPDATABLE_VALID;
>  > >> +
>  > >> +  if (Image == NULL) {
>  > >> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer 
> Parameter is NULL.\n"));
>  > >> +    //
>  > >> +    // Not sure if this is needed
>  > >> +    //
>  > >> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
>  > >> +    *LastAttemptStatus = 
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>  > >> +    return EFI_INVALID_PARAMETER;
>  > >> +  }
>  > >> +
>  > >> +  //
>  > >> +  // Make sure the image size is correct
>  > >> +  //
>  > >> +  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
>  > >> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
>  > >> +    *LastAttemptStatus = 
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>  > >> +    return EFI_INVALID_PARAMETER;
>  > >> +  }
>  > >> +
>  > >> +  return EFI_SUCCESS;
>  > >> +}
>  > >> +
>  > >>   /**
>  > >>   Checks if the firmware image is valid for the device.
>  > >>
>  > >> @@ -633,34 +813,9 @@ FmpDeviceCheckImage (
>  > >>     OUT UINT32      *ImageUpdateable
>  > >>     )
>  > >>   {
>  > >> -  if (ImageUpdateable == NULL) {
>  > >> -    DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer 
> Parameter is NULL.\n"));
>  > >> -    return EFI_INVALID_PARAMETER;
>  > >> -  }
>  > >> +  UINT32  LastAttemptStatus;
>  > >>
>  > >> -  //
>  > >> -  //Set to valid and then if any tests fail it will update this flag.
>  > >> -  //
>  > >> -  *ImageUpdateable = IMAGE_UPDATABLE_VALID;
>  > >> -
>  > >> -  if (Image == NULL) {
>  > >> -    DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter is 
> NULL.\n"));
>  > >> -    //
>  > >> -    // Not sure if this is needed
>  > >> -    //
>  > >> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
>  > >> -    return EFI_INVALID_PARAMETER;
>  > >> -  }
>  > >> -
>  > >> -  //
>  > >> -  // Make sure the image size is correct
>  > >> -  //
>  > >> -  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
>  > >> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
>  > >> -    return EFI_INVALID_PARAMETER;
>  > >> -  }
>  > >> -
>  > >> -  return EFI_SUCCESS;
>  > >> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, 
> ImageUpdateable, &LastAttemptStatus);
>  > >>   }
>  > >>
>  > >>   /**
>  > >> diff --git 
> a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
>  > >> 
> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
>  > >> index db0f238ea534..132b60844ad4 100644
>  > >> --- 
> a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
>  > >> +++ 
> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
>  > >> @@ -1,6 +1,6 @@
>  > >>   /**
>  > >>
>  > >> -Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
>  > >> +Copyright (c) Microsoft Corporation.<BR>
>  > >>   Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>  > >>
>  > >>   SPDX-License-Identifier: BSD-2-Clause-Patent
>  > >> @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  > >>
>  > >>
>  > >>   #include <PiDxe.h>
>  > >> +#include <LastAttemptStatus.h>
>  > >> +#include <Guid/SystemResourceTable.h>
>  > >>   #include <Library/DebugLib.h>
>  > >>   #include <Protocol/FirmwareManagement.h>
>  > >>   #include <Library/BaseLib.h>
>  > >> @@ -345,6 +347,131 @@ Return Value:
>  > >>   }//GetImage()
>  > >>
>  > >>
>  > >> +/**
>  > >> +  Updates a firmware device with a new firmware image.  This 
> function returns
>  > >> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the 
> firmware image
>  > >> +  is updatable, the function should perform the following minimal 
> validations
>  > >> +  before proceeding to do the firmware image update.
>  > >> +    - Validate that the image is a supported image for this 
> firmware device.
>  > >> +      Return EFI_ABORTED if the image is not supported.
> Additional details
>  > >> +      on why the image is not a supported image may be returned 
> in AbortReason.
>  > >> +    - Validate the data from VendorCode if is not NULL.  Firmware 
> image
>  > >> +      validation must be performed before VendorCode data validation.
>  > >> +      VendorCode data is ignored or considered invalid if image 
> validation
>  > >> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
>  > >> +
>  > >> +  VendorCode enables vendor to implement vendor-specific firmware 
> image update
>  > >> +  policy.  Null if the caller did not specify the policy or use 
> the default
>  > >> +  policy.  As an example, vendor can implement a policy to allow 
> an option to
>  > >> +  force a firmware image update when the abort reason is due to 
> the new firmware
>  > >> +  image version is older than the current firmware image version 
> or bad image
>  > >> +  checksum.  Sensitive operations such as those wiping the entire 
> firmware image
>  > >> +  and render the device to be non-functional should be encoded in 
> the image
>  > >> +  itself rather than passed with the VendorCode.  AbortReason 
> enables vendor to
>  > >> +  have the option to provide a more detailed description of the 
> abort reason to
>  > >> +  the caller.
>  > >> +
>  > >> +  @param[in]  Image             Points to the new firmware image.
>  > >> +  @param[in]  ImageSize         Size, in bytes, of the new 
> firmware image.
>  > >> +  @param[in]  VendorCode        This enables vendor to implement 
> vendor-specific
>  > >> +                                firmware image update policy.
> NULL indicates
>  > >> +                                the caller did not specify the 
> policy or use the
>  > >> +                                default policy.
>  > >> +  @param[in]  Progress          A function used to report the 
> progress of
>  > >> +                                updating the firmware device with 
> the new
>  > >> +                                firmware image.
>  > >> +  @param[in]  CapsuleFwVersion  The version of the new firmware 
> image from the
>  > >> +                                update capsule that provided the 
> new firmware
>  > >> +                                image.
>  > >> +  @param[out] AbortReason       A pointer to a pointer to a 
> Null-terminated
>  > >> +                                Unicode string providing more 
> details on an
>  > >> +                                aborted operation. The buffer is 
> allocated by
>  > >> +                                this function with
>  > >> +
> EFI_BOOT_SERVICES.AllocatePool().  It is the
>  > >> +                                caller's responsibility to free 
> this buffer with
>  > >> +                                EFI_BOOT_SERVICES.FreePool().
>  > >> +  @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.
>  > >> +
>  > >> +                                The return status code must fall 
> in the range of
>  > >> +
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
>  > >> +
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
>  > >> +
>  > >> +                                If the value falls outside this 
> range, it will be converted
>  > >> +                                to 
> LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
>  > >> +
>  > >> +  @retval EFI_SUCCESS            The firmware device was 
> successfully updated
>  > >> +                                 with the new firmware image.
>  > >> +  @retval EFI_ABORTED            The operation is aborted.
> Additional details
>  > >> +                                 are provided in AbortReason.
>  > >> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
>  > >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
>  > >> +  @retval EFI_UNSUPPORTED        The operation is not supported.
>  > >> +
>  > >> +**/
>  > >> +EFI_STATUS
>  > >> +EFIAPI
>  > >> +FmpDeviceSetImageWithStatus (
>  > >> +  IN  CONST VOID                                     *Image,
>  > >> +  IN  UINTN                                          ImageSize,
>  > >> +  IN  CONST VOID                                     
> *VendorCode,       OPTIONAL
>  > >> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS
> Progress,          OPTIONAL
>  > >> +  IN  UINT32                                         
> CapsuleFwVersion,
>  > >> +  OUT CHAR16                                         **AbortReason,
>  > >> +  OUT UINT32                                         
> *LastAttemptStatus
>  > >> +  )
>  > >> +{
>  > >> +    EFI_STATUS Status                         = EFI_SUCCESS;
>  > >> +    UINT32 Updateable                         = 0;
>  > >> +
>  > >> +    Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, 
> &Updateable, LastAttemptStatus);
>  > >> +    if (EFI_ERROR(Status))
>  > >> +    {
>  > >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image 
> failed with %r.\n", Status));
>  > >> +        goto cleanup;
>  > >> +    }
>  > >> +
>  > >> +    if (Updateable != IMAGE_UPDATABLE_VALID)
>  > >> +    {
>  > >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image 
> returned that the Image was not valid for update.
>  > >> Updatable value = 0x%X.\n", Updateable));
>  > >> +        Status = EFI_ABORTED;
>  > >> +        goto cleanup;
>  > >> +    }
>  > >> +
>  > >> +    if (Progress == NULL)
>  > >> +    {
>  > >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Invalid 
> progress callback\n"));
>  > >> +        Status = EFI_INVALID_PARAMETER;
>  > >> +        goto cleanup;
>  > >> +    }
>  > >> +
>  > >> +    Status = Progress(15);
>  > >> +    if (EFI_ERROR(Status))
>  > >> +    {
>  > >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Progress 
> Callback failed with Status %r.\n", Status));
>  > >> +    }
>  > >> +
>  > >> +    {
>  > >> +      UINTN  p;
>  > >> +
>  > >> +      for (p = 20; p < 100; p++) {
>  > >> +        gBS->Stall (100000);  //us  = 0.1 seconds
>  > >> +        Progress (p);
>  > >> +      }
>  > >> +    }
>  > >> +
>  > >> +    //TODO: add support for VendorCode, and AbortReason
>  > >> +cleanup:
>  > >> +    if (EFI_ERROR (Status)) {
>  > >> +      *LastAttemptStatus = 
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>  > >> +    }
>  > >> +
>  > >> +    return Status;
>  > >> +}// SetImageWithStatus()
>  > >> +
>  > >> +
>  > >>   /**
>  > >>   Updates the firmware image of the device.
>  > >>
>  > >> @@ -396,51 +523,98 @@ IN
> UINT32                                           CapsuleFwVersion,
>  > >>   OUT CHAR16                                           **AbortReason
>  > >>   )
>  > >>   {
>  > >> -    EFI_STATUS Status                         = EFI_SUCCESS;
>  > >> -    UINT32 Updateable                         = 0;
>  > >> -
>  > >> -    Status = FmpDeviceCheckImage(Image, ImageSize, &Updateable);
>  > >> -    if (EFI_ERROR(Status))
>  > >> -    {
>  > >> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image failed with 
> %r.\n", Status));
>  > >> -        goto cleanup;
>  > >> -    }
>  > >> -
>  > >> -    if (Updateable != IMAGE_UPDATABLE_VALID)
>  > >> -    {
>  > >> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image returned that 
> the Image was not valid for update.  Updatable value
>  > =
>  > >> 0x%X.\n", Updateable));
>  > >> -        Status = EFI_ABORTED;
>  > >> -        goto cleanup;
>  > >> -    }
>  > >> -
>  > >> -    if (Progress == NULL)
>  > >> -    {
>  > >> -        DEBUG((DEBUG_ERROR, "SetImage - Invalid progress 
> callback\n"));
>  > >> -        Status = EFI_INVALID_PARAMETER;
>  > >> -        goto cleanup;
>  > >> -    }
>  > >> -
>  > >> -    Status = Progress(15);
>  > >> -    if (EFI_ERROR(Status))
>  > >> -    {
>  > >> -        DEBUG((DEBUG_ERROR, "SetImage - Progress Callback failed 
> with Status %r.\n", Status));
>  > >> -    }
>  > >> -
>  > >> -    {
>  > >> -      UINTN  p;
>  > >> -
>  > >> -      for (p = 20; p < 100; p++) {
>  > >> -        gBS->Stall (100000);  //us  = 0.1 seconds
>  > >> -        Progress (p);
>  > >> -      }
>  > >> -    }
>  > >> -
>  > >> -    //TODO: add support for VendorCode, and AbortReason
>  > >> -cleanup:
>  > >> -    return Status;
>  > >> +  UINT32  LastAttemptStatus;
>  > >> +
>  > >> +  return  FmpDeviceSetImageWithStatus (
>  > >> +            Image,
>  > >> +            ImageSize,
>  > >> +            VendorCode,
>  > >> +            Progress,
>  > >> +            CapsuleFwVersion,
>  > >> +            AbortReason,
>  > >> +            &LastAttemptStatus
>  > >> +            );
>  > >>   }// SetImage()
>  > >>
>  > >>
>  > >> +/**
>  > >> +  Checks if a new firmware image is valid for the firmware 
> device.  This
>  > >> +  function allows firmware update operation to validate the 
> firmware image
>  > >> +  before FmpDeviceSetImage() is called.
>  > >> +
>  > >> +  @param[in]  Image               Points to a new firmware image.
>  > >> +  @param[in]  ImageSize           Size, in bytes, of a new 
> firmware image.
>  > >> +  @param[out] ImageUpdatable      Indicates if a new firmware 
> image is valid for
>  > >> +                                  a firmware update to the 
> firmware device.  The
>  > >> +                                  following values from the 
> Firmware Management
>  > >> +                                  Protocol are supported:
>  > >> +                                    IMAGE_UPDATABLE_VALID
>  > >> +                                    IMAGE_UPDATABLE_INVALID
>  > >> +                                    IMAGE_UPDATABLE_INVALID_TYPE
>  > >> +                                    IMAGE_UPDATABLE_INVALID_OLD
>  > >> +
> 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.
>  > >> +
>  > >> +                                  The return status code must 
> fall in the range of
>  > >> +
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
>  > >> +
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
>  > >> +
>  > >> +                                  If the value falls outside this 
> range, it will be converted
>  > >> +                                  to 
> LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
>  > >> +
>  > >> +  @retval EFI_SUCCESS            The image was successfully 
> checked.  Additional
>  > >> +                                 status information is returned in
>  > >> +                                 ImageUpdatable.
>  > >> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
>  > >> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
>  > >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
>  > >> +
>  > >> +**/
>  > >> +EFI_STATUS
>  > >> +EFIAPI
>  > >> +FmpDeviceCheckImageWithStatus (
>  > >> +  IN  CONST VOID  *Image,
>  > >> +  IN  UINTN       ImageSize,
>  > >> +  OUT UINT32      *ImageUpdatable,
>  > >> +  OUT UINT32      *LastAttemptStatus
>  > >> +  )
>  > >> +{
>  > >> +    EFI_STATUS status = EFI_SUCCESS;
>  > >> +
>  > >> +    if (LastAttemptStatus == NULL) {
>  > >> +      DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - 
> LastAttemptStatus Pointer Parameter is NULL.\n"));
>  > >> +      return EFI_INVALID_PARAMETER;
>  > >> +    }
>  > >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
>  > >> +
>  > >> +    if (ImageUpdatable == NULL)
>  > >> +    {
>  > >> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - 
> ImageUpdatable Pointer Parameter is NULL.\n"));
>  > >> +        *LastAttemptStatus = 
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>  > >> +        status = EFI_INVALID_PARAMETER;
>  > >> +        goto cleanup;
>  > >> +    }
>  > >> +
>  > >> +    //
>  > >> +    //Set to valid and then if any tests fail it will update this 
> flag.
>  > >> +    //
>  > >> +    *ImageUpdatable = IMAGE_UPDATABLE_VALID;
>  > >> +
>  > >> +    if (Image == NULL)
>  > >> +    {
>  > >> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer 
> Parameter is NULL.\n"));
>  > >> +        *ImageUpdatable = IMAGE_UPDATABLE_INVALID; //not sure if 
> this is needed
>  > >> +        *LastAttemptStatus = 
> LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
>  > >> +        return EFI_INVALID_PARAMETER;
>  > >> +    }
>  > >> +
>  > >> +cleanup:
>  > >> +    return status;
>  > >> +}// CheckImageWithStatus()
>  > >> +
>  > >>
>  > >>   /**
>  > >>   Checks if the firmware image is valid for the device.
>  > >> @@ -465,29 +639,9 @@ IN  UINTN                             ImageSize,
>  > >>   OUT UINT32                            *ImageUpdateable
>  > >>   )
>  > >>   {
>  > >> -    EFI_STATUS status = EFI_SUCCESS;
>  > >> +  UINT32  LastAttemptStatus;
>  > >>
>  > >> -    if (ImageUpdateable == NULL)
>  > >> -    {
>  > >> -        DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer 
> Parameter is NULL.\n"));
>  > >> -        status = EFI_INVALID_PARAMETER;
>  > >> -        goto cleanup;
>  > >> -    }
>  > >> -
>  > >> -    //
>  > >> -    //Set to valid and then if any tests fail it will update this 
> flag.
>  > >> -    //
>  > >> -    *ImageUpdateable = IMAGE_UPDATABLE_VALID;
>  > >> -
>  > >> -    if (Image == NULL)
>  > >> -    {
>  > >> -        DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter 
> is NULL.\n"));
>  > >> -        *ImageUpdateable = IMAGE_UPDATABLE_INVALID; //not sure if 
> this is needed
>  > >> -        return EFI_INVALID_PARAMETER;
>  > >> -    }
>  > >> -
>  > >> -cleanup:
>  > >> -    return status;
>  > >> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, 
> ImageUpdateable, &LastAttemptStatus);
>  > >>   }// CheckImage()
>  > >>
>  > >>   /**
>  > >> --
>  > >> 2.28.0.windows.1
>  > >
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
  2020-11-11  4:16       ` [edk2-devel] " Kilian Kegel
  2020-11-11  5:17         ` Michael Kubacki
@ 2020-11-11 17:17         ` Michael D Kinney
  1 sibling, 0 replies; 5+ messages in thread
From: Michael D Kinney @ 2020-11-11 17:17 UTC (permalink / raw)
  To: devel@edk2.groups.io, KILIAN_KEGEL@OUTLOOK.COM, Michael Kubacki,
	Kinney, Michael D
  Cc: Qian, Yi, Sun, Zailiang

[-- Attachment #1: Type: text/plain, Size: 41535 bytes --]

Hi Kilian,

2983 was resolved by a previous commit.  Thank you for the reminder to update the BZ with the details.  It is closed now.

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kilian Kegel
Sent: Tuesday, November 10, 2020 8:17 PM
To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Michael Kubacki <michael.kubacki@outlook.com>
Cc: Qian, Yi <yi.qian@intel.com>; Sun, Zailiang <zailiang.sun@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility

Hi all,

if this patch also affects https://bugzilla.tianocore.org/show_bug.cgi?id=2983, please close it.

Thanks,
Kilian

Von: Michael D Kinney<mailto:michael.d.kinney@intel.com>
Gesendet: Wednesday, November 11, 2020 4:03 AM
An: Michael Kubacki<mailto:michael.kubacki@outlook.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>
Cc: Qian, Yi<mailto:yi.qian@intel.com>; Sun, Zailiang<mailto:zailiang.sun@intel.com>
Betreff: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility

Hi Michael,

This patch has been merged.

Thanks,

Mike

> -----Original Message-----
> From: Michael Kubacki <michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>>
> Sent: Thursday, November 5, 2020 12:36 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Qian, Yi <yi.qian@intel.com<mailto:yi.qian@intel.com>>; Sun, Zailiang <zailiang.sun@intel.com<mailto:zailiang.sun@intel.com>>
> Subject: Re: [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
>
> Hi Mike,
>
> I saw other Vlv2TbltDevicePkg patches were merged yesterday. Do you know
> when this patch will be merged?
>
> Thanks,
> Michael
>
> On 10/28/2020 6:12 PM, Kinney, Michael D wrote:
> > Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> >
> > I also verified build and boot and FMP based capsule updates work as expected.
> >
> > Tested-by: Michael D Kinney <Michael.d.kinney@intel.com<mailto:Michael.d.kinney@intel.com>>
> >
> > Thanks,
> >
> > Mike
> >
> >
> >> -----Original Message-----
> >> From: michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com> <michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>>
> >> Sent: Thursday, October 1, 2020 2:59 PM
> >> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> >> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Qian, Yi <yi.qian@intel.com<mailto:yi.qian@intel.com>>; Sun, Zailiang
> <zailiang.sun@intel.com<mailto:zailiang.sun@intel.com>>
> >> Subject: [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility
> >>
> >> From: Michael Kubacki <michael.kubacki@microsoft.com<mailto:michael.kubacki@microsoft.com>>
> >>
> >> Makes the changes necessary for these library instances of
> >> FmpDeviceLib to be compatible with new functions added recently
> >> to FmpDeviceLib.
> >>
> >> Two new functions were introduced in FmpDeviceLib to allow a
> >> library instance to return a Last Attempt Status code during
> >> check image and set image operations:
> >>    1. FmpDeviceCheckImageWithStatus ( )
> >>    2. FmpDeviceSetImageWithStatus ( )
> >>
> >> FmpDxe (in FmpDevicePkg) will begin calling these new functions
> >> instead of the previous functions. Therefore, this change:
> >>    1. Adds these functions to Vlv2TbltDevicePkg implementations
> >>    2. Moves the main functionality to these new functions
> >>    3. Updates the old functions to call the new functions
> >>       (for backward compatibility)
> >>
> >> Note: As of this commit, the Vlv2TbltDevicePkg build is broken
> >> due to:
> >>    1. A required RngLib library instance not defined by the platform
> >>    2. Other FMP libraries not being defined by the platform
> >>       (e.g. FmpDependencyLib, FmpDependencyCheckLib, etc.)
> >>
> >> Those changes were fixed locally to test the changes in this commit
> >> but maintainers should make the proper changes for those issues.
> >>
> >> Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> >> Cc: Yi Qian <yi.qian@intel.com<mailto:yi.qian@intel.com>>
> >> Cc: Zailiang Sun <zailiang.sun@intel.com<mailto:zailiang.sun@intel.com>>
> >> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com<mailto:michael.kubacki@microsoft.com>>
> >> ---
> >>
> >> Notes:
> >>      Only build was checked, I do not have access to a
> >>      VLV2 device for testing.
> >>
> >>   Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c       | 299 +++++++++++++++-----
> >>   Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c | 284 ++++++++++++++-----
> >>   2 files changed, 446 insertions(+), 137 deletions(-)
> >>
> >> diff --git a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> >> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> >> index d8c9036012ad..df8a36d9854c 100644
> >> --- a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> >> +++ b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLib/FmpDeviceLib.c
> >> @@ -1,6 +1,6 @@
> >>   /**
> >>
> >> -Copyright (c) 2016, Microsoft Corporation.  All rights reserved.
> >> +Copyright (c) Microsoft Corporation.<BR>
> >>   Copyright (c) 2019, Intel Corporation.  All rights reserved.
> >>
> >>   SPDX-License-Identifier: BSD-2-Clause-Patent
> >> @@ -8,7 +8,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>   **/
> >>
> >>   #include <PiDxe.h>
> >> -
> >> +#include <LastAttemptStatus.h>
> >> +#include <Guid/SystemResourceTable.h>
> >>   #include <Library/FmpDeviceLib.h>
> >>
> >>   #include <Library/DebugLib.h>
> >> @@ -444,20 +445,17 @@ FmpDeviceGetImage (
> >>   }
> >>
> >>   /**
> >> -  Updates the firmware image of the device.
> >> -
> >> -  This function updates the hardware with the new firmware image.  This function
> >> -  returns EFI_UNSUPPORTED if the firmware image is not updatable.  If the
> >> -  firmware image is updatable, the function should perform the following minimal
> >> -  validations before proceeding to do the firmware image update.
> >> -    - Validate the image is a supported image for this device.  The function
> >> -      returns EFI_ABORTED if the image is unsupported.  The function can
> >> -      optionally provide more detailed information on why the image is not a
> >> -      supported image.
> >> -    - Validate the data from VendorCode if not null.  Image validation must be
> >> -      performed before VendorCode data validation.  VendorCode data is ignored
> >> -      or considered invalid if image validation failed.  The function returns
> >> -      EFI_ABORTED if the data is invalid.
> >> +  Updates a firmware device with a new firmware image.  This function returns
> >> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the firmware image
> >> +  is updatable, the function should perform the following minimal validations
> >> +  before proceeding to do the firmware image update.
> >> +    - Validate that the image is a supported image for this firmware device.
> >> +      Return EFI_ABORTED if the image is not supported.  Additional details
> >> +      on why the image is not a supported image may be returned in AbortReason.
> >> +    - Validate the data from VendorCode if is not NULL.  Firmware image
> >> +      validation must be performed before VendorCode data validation.
> >> +      VendorCode data is ignored or considered invalid if image validation
> >> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
> >>
> >>     VendorCode enables vendor to implement vendor-specific firmware image update
> >>     policy.  Null if the caller did not specify the policy or use the default
> >> @@ -470,38 +468,56 @@ FmpDeviceGetImage (
> >>     have the option to provide a more detailed description of the abort reason to
> >>     the caller.
> >>
> >> -  @param[in]  Image             Points to the new image.
> >> -  @param[in]  ImageSize         Size of the new image in bytes.
> >> +  @param[in]  Image             Points to the new firmware image.
> >> +  @param[in]  ImageSize         Size, in bytes, of the new firmware image.
> >>     @param[in]  VendorCode        This enables vendor to implement vendor-specific
> >> -                                firmware image update policy. Null indicates the
> >> -                                caller did not specify the policy or use the
> >> +                                firmware image update policy.  NULL indicates
> >> +                                the caller did not specify the policy or use the
> >>                                   default policy.
> >> -  @param[in]  Progress          A function used by the driver to report the
> >> -                                progress of the firmware update.
> >> -  @param[in]  CapsuleFwVersion  FMP Payload Header version of the image.
> >> -  @param[out] AbortReason       A pointer to a pointer to a null-terminated
> >> -                                string providing more details for the aborted
> >> -                                operation. The buffer is allocated by this
> >> -                                function with AllocatePool(), and it is the
> >> -                                caller's responsibility to free it with a call
> >> -                                to FreePool().
> >> +  @param[in]  Progress          A function used to report the progress of
> >> +                                updating the firmware device with the new
> >> +                                firmware image.
> >> +  @param[in]  CapsuleFwVersion  The version of the new firmware image from the
> >> +                                update capsule that provided the new firmware
> >> +                                image.
> >> +  @param[out] AbortReason       A pointer to a pointer to a Null-terminated
> >> +                                Unicode string providing more details on an
> >> +                                aborted operation. The buffer is allocated by
> >> +                                this function with
> >> +                                EFI_BOOT_SERVICES.AllocatePool().  It is the
> >> +                                caller's responsibility to free this buffer with
> >> +                                EFI_BOOT_SERVICES.FreePool().
> >> +  @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.
> >>
> >> -  @retval EFI_SUCCESS            The device was successfully updated with the
> >> -                                 new image.
> >> -  @retval EFI_ABORTED            The operation is aborted.
> >> +                                The return status code must fall in the range of
> >> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> >> +
> >> +                                If the value falls outside this range, it will be converted
> >> +                                to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> >> +
> >> +  @retval EFI_SUCCESS            The firmware device was successfully updated
> >> +                                 with the new firmware image.
> >> +  @retval EFI_ABORTED            The operation is aborted.  Additional details
> >> +                                 are provided in AbortReason.
> >>     @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
> >>     @retval EFI_UNSUPPORTED        The operation is not supported.
> >>
> >>   **/
> >>   EFI_STATUS
> >>   EFIAPI
> >> -FmpDeviceSetImage (
> >> +FmpDeviceSetImageWithStatus (
> >>     IN  CONST VOID                                     *Image,
> >>     IN  UINTN                                          ImageSize,
> >> -  IN  CONST VOID                                     *VendorCode,
> >> -  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
> >> +  IN  CONST VOID                                     *VendorCode,       OPTIONAL
> >> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,          OPTIONAL
> >>     IN  UINT32                                         CapsuleFwVersion,
> >> -  OUT CHAR16                                         **AbortReason
> >> +  OUT CHAR16                                         **AbortReason,
> >> +  OUT UINT32                                         *LastAttemptStatus
> >>     )
> >>   {
> >>     EFI_STATUS          Status;
> >> @@ -513,25 +529,27 @@ FmpDeviceSetImage (
> >>     UINTN               BytesWritten;
> >>
> >>     Updateable = 0;
> >> -  Status = FmpDeviceCheckImage (Image, ImageSize, &Updateable);
> >> +  Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, &Updateable, LastAttemptStatus);
> >>     if (EFI_ERROR (Status)) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image failed with %r.\n", Status));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check Image failed with %r.\n", Status));
> >>       return Status;
> >>     }
> >>
> >>     if (Updateable != IMAGE_UPDATABLE_VALID) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Check Image returned that the Image was not valid for update.  Updatable
> >> value = 0x%X.\n", Updateable));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Check Image returned that the Image was not valid for update.
> >> Updatable value = 0x%X.\n", Updateable));
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >>       return EFI_ABORTED;
> >>     }
> >>
> >>     if (Progress == NULL) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Invalid progress callback\n"));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Invalid progress callback\n"));
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >>       return EFI_INVALID_PARAMETER;
> >>     }
> >>
> >>     Status = Progress (15);
> >>     if (EFI_ERROR (Status)) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed with Status %r.\n", Status));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress Callback failed with Status %r.\n", Status));
> >>     }
> >>
> >>     //
> >> @@ -539,7 +557,7 @@ FmpDeviceSetImage (
> >>     //
> >>     Progress (20);
> >>     if (EFI_ERROR (Status)) {
> >> -    DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed with Status %r.\n", Status));
> >> +    DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress Callback failed with Status %r.\n", Status));
> >>     }
> >>
> >>     //
> >> @@ -553,11 +571,11 @@ FmpDeviceSetImage (
> >>
> >>   //    Progress (Percentage);
> >>   //    if (EFI_ERROR (Status)) {
> >> -//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImage - Progress Callback failed with Status %r.\n", Status));
> >> +//      DEBUG((DEBUG_ERROR, "FmpDeviceSetImageWithStatus - Progress Callback failed with Status %r.\n", Status));
> >>   //    }
> >>     }
> >>
> >> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %d Images ...\n", ARRAY_SIZE (mUpdateConfigData)));
> >> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %d Images ...\n", ARRAY_SIZE (mUpdateConfigData)));
> >>
> >>     if (ARRAY_SIZE (mUpdateConfigData) == 0) {
> >>       DEBUG((DEBUG_INFO, "PlatformUpdate: BaseAddress - 0x%lx ImageOffset - 0x%x Length - 0x%x\n", 0, 0, ImageSize));
> >> @@ -605,11 +623,173 @@ FmpDeviceSetImage (
> >>       BytesWritten += ConfigData->Length;
> >>     }
> >>
> >> -  DEBUG ((DEBUG_INFO, "FmpDeviceSetImage - %r\n", Status));
> >> +  DEBUG ((DEBUG_INFO, "FmpDeviceSetImageWithStatus - %r\n", Status));
> >> +
> >> +  if (EFI_ERROR (Status)) {
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +  }
> >>
> >>     return Status;
> >>   }
> >>
> >> +/**
> >> +  Updates the firmware image of the device.
> >> +
> >> +  This function updates the hardware with the new firmware image.  This function
> >> +  returns EFI_UNSUPPORTED if the firmware image is not updatable.  If the
> >> +  firmware image is updatable, the function should perform the following minimal
> >> +  validations before proceeding to do the firmware image update.
> >> +    - Validate the image is a supported image for this device.  The function
> >> +      returns EFI_ABORTED if the image is unsupported.  The function can
> >> +      optionally provide more detailed information on why the image is not a
> >> +      supported image.
> >> +    - Validate the data from VendorCode if not null.  Image validation must be
> >> +      performed before VendorCode data validation.  VendorCode data is ignored
> >> +      or considered invalid if image validation failed.  The function returns
> >> +      EFI_ABORTED if the data is invalid.
> >> +
> >> +  VendorCode enables vendor to implement vendor-specific firmware image update
> >> +  policy.  Null if the caller did not specify the policy or use the default
> >> +  policy.  As an example, vendor can implement a policy to allow an option to
> >> +  force a firmware image update when the abort reason is due to the new firmware
> >> +  image version is older than the current firmware image version or bad image
> >> +  checksum.  Sensitive operations such as those wiping the entire firmware image
> >> +  and render the device to be non-functional should be encoded in the image
> >> +  itself rather than passed with the VendorCode.  AbortReason enables vendor to
> >> +  have the option to provide a more detailed description of the abort reason to
> >> +  the caller.
> >> +
> >> +  @param[in]  Image             Points to the new image.
> >> +  @param[in]  ImageSize         Size of the new image in bytes.
> >> +  @param[in]  VendorCode        This enables vendor to implement vendor-specific
> >> +                                firmware image update policy. Null indicates the
> >> +                                caller did not specify the policy or use the
> >> +                                default policy.
> >> +  @param[in]  Progress          A function used by the driver to report the
> >> +                                progress of the firmware update.
> >> +  @param[in]  CapsuleFwVersion  FMP Payload Header version of the image.
> >> +  @param[out] AbortReason       A pointer to a pointer to a null-terminated
> >> +                                string providing more details for the aborted
> >> +                                operation. The buffer is allocated by this
> >> +                                function with AllocatePool(), and it is the
> >> +                                caller's responsibility to free it with a call
> >> +                                to FreePool().
> >> +
> >> +  @retval EFI_SUCCESS            The device was successfully updated with the
> >> +                                 new image.
> >> +  @retval EFI_ABORTED            The operation is aborted.
> >> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_UNSUPPORTED        The operation is not supported.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +FmpDeviceSetImage (
> >> +  IN  CONST VOID                                     *Image,
> >> +  IN  UINTN                                          ImageSize,
> >> +  IN  CONST VOID                                     *VendorCode,
> >> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,
> >> +  IN  UINT32                                         CapsuleFwVersion,
> >> +  OUT CHAR16                                         **AbortReason
> >> +  )
> >> +{
> >> +  UINT32  LastAttemptStatus;
> >> +
> >> +  return  FmpDeviceSetImageWithStatus (
> >> +            Image,
> >> +            ImageSize,
> >> +            VendorCode,
> >> +            Progress,
> >> +            CapsuleFwVersion,
> >> +            AbortReason,
> >> +            &LastAttemptStatus
> >> +            );
> >> +}
> >> +
> >> +/**
> >> +  Checks if a new firmware image is valid for the firmware device.  This
> >> +  function allows firmware update operation to validate the firmware image
> >> +  before FmpDeviceSetImage() is called.
> >> +
> >> +  @param[in]  Image               Points to a new firmware image.
> >> +  @param[in]  ImageSize           Size, in bytes, of a new firmware image.
> >> +  @param[out] ImageUpdatable      Indicates if a new firmware image is valid for
> >> +                                  a firmware update to the firmware device.  The
> >> +                                  following values from the Firmware Management
> >> +                                  Protocol are supported:
> >> +                                    IMAGE_UPDATABLE_VALID
> >> +                                    IMAGE_UPDATABLE_INVALID
> >> +                                    IMAGE_UPDATABLE_INVALID_TYPE
> >> +                                    IMAGE_UPDATABLE_INVALID_OLD
> >> +                                    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.
> >> +
> >> +                                  The return status code must fall in the range of
> >> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> >> +
> >> +                                  If the value falls outside this range, it will be converted
> >> +                                  to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> >> +
> >> +  @retval EFI_SUCCESS            The image was successfully checked.  Additional
> >> +                                 status information is returned in
> >> +                                 ImageUpdatable.
> >> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
> >> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
> >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +FmpDeviceCheckImageWithStatus (
> >> +  IN  CONST VOID  *Image,
> >> +  IN  UINTN       ImageSize,
> >> +  OUT UINT32      *ImageUpdatable,
> >> +  OUT UINT32      *LastAttemptStatus
> >> +  )
> >> +{
> >> +  if (LastAttemptStatus == NULL) {
> >> +    DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - LastAttemptStatus Pointer Parameter is NULL.\n"));
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +  *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
> >> +
> >> +  if (ImageUpdatable == NULL) {
> >> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - ImageUpdatable Pointer Parameter is NULL.\n"));
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +
> >> +  //
> >> +  //Set to valid and then if any tests fail it will update this flag.
> >> +  //
> >> +  *ImageUpdatable = IMAGE_UPDATABLE_VALID;
> >> +
> >> +  if (Image == NULL) {
> >> +    DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer Parameter is NULL.\n"));
> >> +    //
> >> +    // Not sure if this is needed
> >> +    //
> >> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +
> >> +  //
> >> +  // Make sure the image size is correct
> >> +  //
> >> +  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
> >> +    *ImageUpdatable = IMAGE_UPDATABLE_INVALID;
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +    return EFI_INVALID_PARAMETER;
> >> +  }
> >> +
> >> +  return EFI_SUCCESS;
> >> +}
> >> +
> >>   /**
> >>   Checks if the firmware image is valid for the device.
> >>
> >> @@ -633,34 +813,9 @@ FmpDeviceCheckImage (
> >>     OUT UINT32      *ImageUpdateable
> >>     )
> >>   {
> >> -  if (ImageUpdateable == NULL) {
> >> -    DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer Parameter is NULL.\n"));
> >> -    return EFI_INVALID_PARAMETER;
> >> -  }
> >> +  UINT32  LastAttemptStatus;
> >>
> >> -  //
> >> -  //Set to valid and then if any tests fail it will update this flag.
> >> -  //
> >> -  *ImageUpdateable = IMAGE_UPDATABLE_VALID;
> >> -
> >> -  if (Image == NULL) {
> >> -    DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter is NULL.\n"));
> >> -    //
> >> -    // Not sure if this is needed
> >> -    //
> >> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
> >> -    return EFI_INVALID_PARAMETER;
> >> -  }
> >> -
> >> -  //
> >> -  // Make sure the image size is correct
> >> -  //
> >> -  if (ImageSize != PcdGet32 (PcdBiosRomSize)) {
> >> -    *ImageUpdateable = IMAGE_UPDATABLE_INVALID;
> >> -    return EFI_INVALID_PARAMETER;
> >> -  }
> >> -
> >> -  return EFI_SUCCESS;
> >> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, ImageUpdateable, &LastAttemptStatus);
> >>   }
> >>
> >>   /**
> >> diff --git a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> >> b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> >> index db0f238ea534..132b60844ad4 100644
> >> --- a/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> >> +++ b/Platform/Intel/Vlv2TbltDevicePkg/Feature/Capsule/Library/FmpDeviceLibSample/FmpDeviceLib.c
> >> @@ -1,6 +1,6 @@
> >>   /**
> >>
> >> -Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
> >> +Copyright (c) Microsoft Corporation.<BR>
> >>   Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> >>
> >>   SPDX-License-Identifier: BSD-2-Clause-Patent
> >> @@ -9,6 +9,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>
> >>
> >>   #include <PiDxe.h>
> >> +#include <LastAttemptStatus.h>
> >> +#include <Guid/SystemResourceTable.h>
> >>   #include <Library/DebugLib.h>
> >>   #include <Protocol/FirmwareManagement.h>
> >>   #include <Library/BaseLib.h>
> >> @@ -345,6 +347,131 @@ Return Value:
> >>   }//GetImage()
> >>
> >>
> >> +/**
> >> +  Updates a firmware device with a new firmware image.  This function returns
> >> +  EFI_UNSUPPORTED if the firmware image is not updatable.  If the firmware image
> >> +  is updatable, the function should perform the following minimal validations
> >> +  before proceeding to do the firmware image update.
> >> +    - Validate that the image is a supported image for this firmware device.
> >> +      Return EFI_ABORTED if the image is not supported.  Additional details
> >> +      on why the image is not a supported image may be returned in AbortReason.
> >> +    - Validate the data from VendorCode if is not NULL.  Firmware image
> >> +      validation must be performed before VendorCode data validation.
> >> +      VendorCode data is ignored or considered invalid if image validation
> >> +      fails.  Return EFI_ABORTED if the VendorCode data is invalid.
> >> +
> >> +  VendorCode enables vendor to implement vendor-specific firmware image update
> >> +  policy.  Null if the caller did not specify the policy or use the default
> >> +  policy.  As an example, vendor can implement a policy to allow an option to
> >> +  force a firmware image update when the abort reason is due to the new firmware
> >> +  image version is older than the current firmware image version or bad image
> >> +  checksum.  Sensitive operations such as those wiping the entire firmware image
> >> +  and render the device to be non-functional should be encoded in the image
> >> +  itself rather than passed with the VendorCode.  AbortReason enables vendor to
> >> +  have the option to provide a more detailed description of the abort reason to
> >> +  the caller.
> >> +
> >> +  @param[in]  Image             Points to the new firmware image.
> >> +  @param[in]  ImageSize         Size, in bytes, of the new firmware image.
> >> +  @param[in]  VendorCode        This enables vendor to implement vendor-specific
> >> +                                firmware image update policy.  NULL indicates
> >> +                                the caller did not specify the policy or use the
> >> +                                default policy.
> >> +  @param[in]  Progress          A function used to report the progress of
> >> +                                updating the firmware device with the new
> >> +                                firmware image.
> >> +  @param[in]  CapsuleFwVersion  The version of the new firmware image from the
> >> +                                update capsule that provided the new firmware
> >> +                                image.
> >> +  @param[out] AbortReason       A pointer to a pointer to a Null-terminated
> >> +                                Unicode string providing more details on an
> >> +                                aborted operation. The buffer is allocated by
> >> +                                this function with
> >> +                                EFI_BOOT_SERVICES.AllocatePool().  It is the
> >> +                                caller's responsibility to free this buffer with
> >> +                                EFI_BOOT_SERVICES.FreePool().
> >> +  @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.
> >> +
> >> +                                The return status code must fall in the range of
> >> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> +                                LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> >> +
> >> +                                If the value falls outside this range, it will be converted
> >> +                                to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> >> +
> >> +  @retval EFI_SUCCESS            The firmware device was successfully updated
> >> +                                 with the new firmware image.
> >> +  @retval EFI_ABORTED            The operation is aborted.  Additional details
> >> +                                 are provided in AbortReason.
> >> +  @retval EFI_INVALID_PARAMETER  The Image was NULL.
> >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus was NULL.
> >> +  @retval EFI_UNSUPPORTED        The operation is not supported.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +FmpDeviceSetImageWithStatus (
> >> +  IN  CONST VOID                                     *Image,
> >> +  IN  UINTN                                          ImageSize,
> >> +  IN  CONST VOID                                     *VendorCode,       OPTIONAL
> >> +  IN  EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS  Progress,          OPTIONAL
> >> +  IN  UINT32                                         CapsuleFwVersion,
> >> +  OUT CHAR16                                         **AbortReason,
> >> +  OUT UINT32                                         *LastAttemptStatus
> >> +  )
> >> +{
> >> +    EFI_STATUS Status                         = EFI_SUCCESS;
> >> +    UINT32 Updateable                         = 0;
> >> +
> >> +    Status = FmpDeviceCheckImageWithStatus (Image, ImageSize, &Updateable, LastAttemptStatus);
> >> +    if (EFI_ERROR(Status))
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image failed with %r.\n", Status));
> >> +        goto cleanup;
> >> +    }
> >> +
> >> +    if (Updateable != IMAGE_UPDATABLE_VALID)
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Check Image returned that the Image was not valid for update.
> >> Updatable value = 0x%X.\n", Updateable));
> >> +        Status = EFI_ABORTED;
> >> +        goto cleanup;
> >> +    }
> >> +
> >> +    if (Progress == NULL)
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Invalid progress callback\n"));
> >> +        Status = EFI_INVALID_PARAMETER;
> >> +        goto cleanup;
> >> +    }
> >> +
> >> +    Status = Progress(15);
> >> +    if (EFI_ERROR(Status))
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "SetImageWithStatus  - Progress Callback failed with Status %r.\n", Status));
> >> +    }
> >> +
> >> +    {
> >> +      UINTN  p;
> >> +
> >> +      for (p = 20; p < 100; p++) {
> >> +        gBS->Stall (100000);  //us  = 0.1 seconds
> >> +        Progress (p);
> >> +      }
> >> +    }
> >> +
> >> +    //TODO: add support for VendorCode, and AbortReason
> >> +cleanup:
> >> +    if (EFI_ERROR (Status)) {
> >> +      *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +    }
> >> +
> >> +    return Status;
> >> +}// SetImageWithStatus()
> >> +
> >> +
> >>   /**
> >>   Updates the firmware image of the device.
> >>
> >> @@ -396,51 +523,98 @@ IN  UINT32                                           CapsuleFwVersion,
> >>   OUT CHAR16                                           **AbortReason
> >>   )
> >>   {
> >> -    EFI_STATUS Status                         = EFI_SUCCESS;
> >> -    UINT32 Updateable                         = 0;
> >> -
> >> -    Status = FmpDeviceCheckImage(Image, ImageSize, &Updateable);
> >> -    if (EFI_ERROR(Status))
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image failed with %r.\n", Status));
> >> -        goto cleanup;
> >> -    }
> >> -
> >> -    if (Updateable != IMAGE_UPDATABLE_VALID)
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "SetImage - Check Image returned that the Image was not valid for update.  Updatable value
> =
> >> 0x%X.\n", Updateable));
> >> -        Status = EFI_ABORTED;
> >> -        goto cleanup;
> >> -    }
> >> -
> >> -    if (Progress == NULL)
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "SetImage - Invalid progress callback\n"));
> >> -        Status = EFI_INVALID_PARAMETER;
> >> -        goto cleanup;
> >> -    }
> >> -
> >> -    Status = Progress(15);
> >> -    if (EFI_ERROR(Status))
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "SetImage - Progress Callback failed with Status %r.\n", Status));
> >> -    }
> >> -
> >> -    {
> >> -      UINTN  p;
> >> -
> >> -      for (p = 20; p < 100; p++) {
> >> -        gBS->Stall (100000);  //us  = 0.1 seconds
> >> -        Progress (p);
> >> -      }
> >> -    }
> >> -
> >> -    //TODO: add support for VendorCode, and AbortReason
> >> -cleanup:
> >> -    return Status;
> >> +  UINT32  LastAttemptStatus;
> >> +
> >> +  return  FmpDeviceSetImageWithStatus (
> >> +            Image,
> >> +            ImageSize,
> >> +            VendorCode,
> >> +            Progress,
> >> +            CapsuleFwVersion,
> >> +            AbortReason,
> >> +            &LastAttemptStatus
> >> +            );
> >>   }// SetImage()
> >>
> >>
> >> +/**
> >> +  Checks if a new firmware image is valid for the firmware device.  This
> >> +  function allows firmware update operation to validate the firmware image
> >> +  before FmpDeviceSetImage() is called.
> >> +
> >> +  @param[in]  Image               Points to a new firmware image.
> >> +  @param[in]  ImageSize           Size, in bytes, of a new firmware image.
> >> +  @param[out] ImageUpdatable      Indicates if a new firmware image is valid for
> >> +                                  a firmware update to the firmware device.  The
> >> +                                  following values from the Firmware Management
> >> +                                  Protocol are supported:
> >> +                                    IMAGE_UPDATABLE_VALID
> >> +                                    IMAGE_UPDATABLE_INVALID
> >> +                                    IMAGE_UPDATABLE_INVALID_TYPE
> >> +                                    IMAGE_UPDATABLE_INVALID_OLD
> >> +                                    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.
> >> +
> >> +                                  The return status code must fall in the range of
> >> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE to
> >> +                                  LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MAX_ERROR_CODE_VALUE.
> >> +
> >> +                                  If the value falls outside this range, it will be converted
> >> +                                  to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL.
> >> +
> >> +  @retval EFI_SUCCESS            The image was successfully checked.  Additional
> >> +                                 status information is returned in
> >> +                                 ImageUpdatable.
> >> +  @retval EFI_INVALID_PARAMETER  Image is NULL.
> >> +  @retval EFI_INVALID_PARAMETER  ImageUpdatable is NULL.
> >> +  @retval EFI_INVALID_PARAMETER  LastAttemptStatus is NULL.
> >> +
> >> +**/
> >> +EFI_STATUS
> >> +EFIAPI
> >> +FmpDeviceCheckImageWithStatus (
> >> +  IN  CONST VOID  *Image,
> >> +  IN  UINTN       ImageSize,
> >> +  OUT UINT32      *ImageUpdatable,
> >> +  OUT UINT32      *LastAttemptStatus
> >> +  )
> >> +{
> >> +    EFI_STATUS status = EFI_SUCCESS;
> >> +
> >> +    if (LastAttemptStatus == NULL) {
> >> +      DEBUG ((DEBUG_ERROR, "CheckImageWithStatus - LastAttemptStatus Pointer Parameter is NULL.\n"));
> >> +      return EFI_INVALID_PARAMETER;
> >> +    }
> >> +    *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
> >> +
> >> +    if (ImageUpdatable == NULL)
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - ImageUpdatable Pointer Parameter is NULL.\n"));
> >> +        *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +        status = EFI_INVALID_PARAMETER;
> >> +        goto cleanup;
> >> +    }
> >> +
> >> +    //
> >> +    //Set to valid and then if any tests fail it will update this flag.
> >> +    //
> >> +    *ImageUpdatable = IMAGE_UPDATABLE_VALID;
> >> +
> >> +    if (Image == NULL)
> >> +    {
> >> +        DEBUG((DEBUG_ERROR, "CheckImageWithStatus - Image Pointer Parameter is NULL.\n"));
> >> +        *ImageUpdatable = IMAGE_UPDATABLE_INVALID; //not sure if this is needed
> >> +        *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_MIN_ERROR_CODE_VALUE;
> >> +        return EFI_INVALID_PARAMETER;
> >> +    }
> >> +
> >> +cleanup:
> >> +    return status;
> >> +}// CheckImageWithStatus()
> >> +
> >>
> >>   /**
> >>   Checks if the firmware image is valid for the device.
> >> @@ -465,29 +639,9 @@ IN  UINTN                             ImageSize,
> >>   OUT UINT32                            *ImageUpdateable
> >>   )
> >>   {
> >> -    EFI_STATUS status = EFI_SUCCESS;
> >> +  UINT32  LastAttemptStatus;
> >>
> >> -    if (ImageUpdateable == NULL)
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "CheckImage - ImageUpdateable Pointer Parameter is NULL.\n"));
> >> -        status = EFI_INVALID_PARAMETER;
> >> -        goto cleanup;
> >> -    }
> >> -
> >> -    //
> >> -    //Set to valid and then if any tests fail it will update this flag.
> >> -    //
> >> -    *ImageUpdateable = IMAGE_UPDATABLE_VALID;
> >> -
> >> -    if (Image == NULL)
> >> -    {
> >> -        DEBUG((DEBUG_ERROR, "CheckImage - Image Pointer Parameter is NULL.\n"));
> >> -        *ImageUpdateable = IMAGE_UPDATABLE_INVALID; //not sure if this is needed
> >> -        return EFI_INVALID_PARAMETER;
> >> -    }
> >> -
> >> -cleanup:
> >> -    return status;
> >> +  return FmpDeviceCheckImageWithStatus (Image, ImageSize, ImageUpdateable, &LastAttemptStatus);
> >>   }// CheckImage()
> >>
> >>   /**
> >> --
> >> 2.28.0.windows.1
> >






[-- Attachment #2: Type: text/html, Size: 123724 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-11 17:17 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1639FD37CA1FD9A2.6392@groups.io>
2020-10-15 21:27 ` [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility Michael Kubacki
     [not found] ` <bf090dba-75d3-520e-7414-bcdb8314562b@outlook.com>
2020-10-27  0:50   ` Michael Kubacki
2020-10-01 21:58 Michael Kubacki
2020-10-29  1:12 ` Michael D Kinney
2020-11-05 20:36   ` Michael Kubacki
2020-11-11  3:03     ` Michael D Kinney
2020-11-11  4:16       ` [edk2-devel] " Kilian Kegel
2020-11-11  5:17         ` Michael Kubacki
2020-11-11 17:17         ` Michael D Kinney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox