Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] Vlv2TbltDevicePkg/FmpDeviceLib: Add LastAttemptStatus compatibility

["Michael Kubacki" <michael.kubacki@outlook.com>]

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()
>>   /**
>>