[PATCH v3 0/6] Extend Last Attempt Status Usage

[PATCH v3 0/6] Extend Last Attempt Status Usage

[Michael Kubacki]

From: Michael Kubacki <michael.kubacki@microsoft.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2802

This patch series adds more granularity to Last Attempt Status
codes reported during FMP check image and set image operations
that greatly improve precision of the status codes.

The unsuccessful vendor range (0x1000 - 0x4000) was introduced
in UEFI Specification 2.8. At a high-level, two subranges are
defined within that range in this patch series:
  1. The FMP Reserved range - reserved for components implemented
     in FmpDevicePkg.
  2. The FMP Device Library Reserved range - reserved for
     FmpDeviceLib instance-specific usage.

The ranges are described in a public header file LastAttemptStatus.h
while the specific codes used within FmpDevicePkg implementation
are defined in a private header file FmpLastAttemptStatus.h.

FmpDeviceLib instances should use the range definition from the
public header file to define Last Attempt Status codes local to
their library instance.

Of note, there's multiple approaches to assigning private status
codes in the FMP Reserved range. For example, individual components
could define their last attempt status codes locally with the
range allocated to the component defined in a package-wide private
header file. However, one goal of the granularity being introduced
is to provide straightforward traceability to an error source.

For that reason, it was chosen to define a constant set of codes at
the package level in FmpLastAttemptStatus.h. For example, if a new
FmpDependencyLib instance is added, it would not be able to reassign
status code values in the pre-existing FMP Dependency range; it
would reuse codes for the same error source and be able to add new
codes onto the range for its usage. I wanted to highlight this for
any feedback.

V3 changes:
  1. Enhanced range definitions in LastAttemptStatus.h with more
     completeness providing length, min, and max values.
  2. Moved the actual Last Attempt Status code assignments to a
     private header file PrivateInclude/FmpLastAttemptStatus.h.
  3. Changed the value of
     LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
     to 0x3FFF instead of 0x4000 even though 0x4000 is defined in
     the UEFI specification. The length is 0x4000 but the max
     allowed value should be 0x3FFF. This change was made now to
     prevent implementation compatibility issues in the future.
  4. Included "DEVICE" in the following macro name to clearly
     associate it with the FmpDeviceLib library class:
     LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_xxx
  5. Included a map to help the reader better visualize the range
     definitions in LastAttemptStatus.h.
  6. Included additional documentation describing the enum in
     FmpLastAttemptStatus.h. An explicit statement stating that new
     codes should be added onto the end of ranges to preserve the
     values was added.
  7. Simplified error handling logic in FmpDxe for FmpDeviceLib
     calls that return Last Attempt Status.
  8. V2 had a single memory allocation failure code used for
     different memory allocations in CheckFmpDependency () in
     FmpDependencyLib. Each potential allocation failure was
     assigned a unique code.

V2 changes:
  1. Consolidate all previous incremental updates to 
     LastAttemptStatus.h into one patch (patch 2)
  2. Move LastAttemptStatus.h from Include to PrivateInclude
  3. Correct patch 1 subject from "FmpDevicePkg" to "MdePkg"

Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Guomin Jiang <guomin.jiang@intel.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>

Michael Kubacki (6):
  MdePkg/SystemResourceTable.h: Add vendor range values
  FmpDevicePkg: Add Last Attempt Status header files
  FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status
    capability
  FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status
    granularity
  FmpDevicePkg: Add Last Attempt Status support to dependency libs
  FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API

 FmpDevicePkg/FmpDxe/FmpDxe.c                                                     | 176 +++++++++++++++++---
 FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c               |  39 +++--
 FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c       |   9 +-
 FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c                         |  96 +++++++++--
 FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c                             |  48 ++++--
 FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c |   7 +-
 FmpDevicePkg/FmpDxe/FmpDxe.h                                                     |   4 +-
 FmpDevicePkg/Include/LastAttemptStatus.h                                         |  96 +++++++++++
 FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h                             |   8 +-
 FmpDevicePkg/Include/Library/FmpDependencyLib.h                                  |  44 +++--
 FmpDevicePkg/Include/Library/FmpDeviceLib.h                                      |  48 ++++--
 FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h                               |  80 +++++++++
 MdePkg/Include/Guid/SystemResourceTable.h                                        |  13 ++
 13 files changed, 575 insertions(+), 93 deletions(-)
 create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
 create mode 100644 FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h

-- 
2.28.0.windows.1

Re: [PATCH v3 0/6] Extend Last Attempt Status Usage

[Michael D Kinney]

Hi Michael,

A couple a couple general questions:

1) I see the entire range from 0x2000-0x3FFF is reserved for FmpDeviceLib.  If we
   every add more device/platform specific libs for FMP, there are no ranges available.
   Should we limit the FmpDeviceLib to a smaller range to support future expansion?

   Also, the style of LastAttemptStatus.h with extra defines for the length of 
   each range is hard to read, and I do not think there are any consumers of the
   length defines from this public include file.  Since there are really only 3
   defined ranges, couldn't this be simplified to min/max defines for each range
   for a total of 6 #defines.  I do not expect ranges (once defined) to change in
   length, and the most that might happen in the future is adding new ranges for
   new lib classes in the unused ranges.

2) This series makes non-backwards compatible changes to some of the lib classes.
   I agree this is the cleanest way to add support for the vendor specific 
   last attempt status.  It does mean that existing implementations will have 
   to update their lib implementations to be compatible with this new version.
   I would be slightly cleaner to introduce new APIs with support for the
   vendor specific last attempt status codes.  Then update all libs to produce
   both the existing APIs and the new APIs (The old APIs can call the new APIs).
   Then update FmpDxe to use the new APIs.  This would be 3 patch series.
   If FmpDxe never calls the old APIs, then we could (at a future date)
   delete the old APIs from the lib class and the lib implementations could
   remove the old API that calls the new API.

3) The following APIs in the Null implementations have OUT params.
   Should these OUT params be set to an expected value?

     CheckFmpDependency()
     FmpDeviceGetImage()
     FmpDeviceSetImage()

Thanks,

Mike
   

> -----Original Message-----
> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
> Sent: Tuesday, August 18, 2020 2:16 PM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Xu,
> Wei6 <wei6.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
> Subject: [PATCH v3 0/6] Extend Last Attempt Status Usage
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2802
> 
> This patch series adds more granularity to Last Attempt Status
> codes reported during FMP check image and set image operations
> that greatly improve precision of the status codes.
> 
> The unsuccessful vendor range (0x1000 - 0x4000) was introduced
> in UEFI Specification 2.8. At a high-level, two subranges are
> defined within that range in this patch series:
>   1. The FMP Reserved range - reserved for components implemented
>      in FmpDevicePkg.
>   2. The FMP Device Library Reserved range - reserved for
>      FmpDeviceLib instance-specific usage.
> 
> The ranges are described in a public header file LastAttemptStatus.h
> while the specific codes used within FmpDevicePkg implementation
> are defined in a private header file FmpLastAttemptStatus.h.
> 
> FmpDeviceLib instances should use the range definition from the
> public header file to define Last Attempt Status codes local to
> their library instance.
> 
> Of note, there's multiple approaches to assigning private status
> codes in the FMP Reserved range. For example, individual components
> could define their last attempt status codes locally with the
> range allocated to the component defined in a package-wide private
> header file. However, one goal of the granularity being introduced
> is to provide straightforward traceability to an error source.
> 
> For that reason, it was chosen to define a constant set of codes at
> the package level in FmpLastAttemptStatus.h. For example, if a new
> FmpDependencyLib instance is added, it would not be able to reassign
> status code values in the pre-existing FMP Dependency range; it
> would reuse codes for the same error source and be able to add new
> codes onto the range for its usage. I wanted to highlight this for
> any feedback.
> 
> V3 changes:
>   1. Enhanced range definitions in LastAttemptStatus.h with more
>      completeness providing length, min, and max values.
>   2. Moved the actual Last Attempt Status code assignments to a
>      private header file PrivateInclude/FmpLastAttemptStatus.h.
>   3. Changed the value of
>      LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
>      to 0x3FFF instead of 0x4000 even though 0x4000 is defined in
>      the UEFI specification. The length is 0x4000 but the max
>      allowed value should be 0x3FFF. This change was made now to
>      prevent implementation compatibility issues in the future.
>   4. Included "DEVICE" in the following macro name to clearly
>      associate it with the FmpDeviceLib library class:
>      LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_xxx
>   5. Included a map to help the reader better visualize the range
>      definitions in LastAttemptStatus.h.
>   6. Included additional documentation describing the enum in
>      FmpLastAttemptStatus.h. An explicit statement stating that new
>      codes should be added onto the end of ranges to preserve the
>      values was added.
>   7. Simplified error handling logic in FmpDxe for FmpDeviceLib
>      calls that return Last Attempt Status.
>   8. V2 had a single memory allocation failure code used for
>      different memory allocations in CheckFmpDependency () in
>      FmpDependencyLib. Each potential allocation failure was
>      assigned a unique code.
> 
> V2 changes:
>   1. Consolidate all previous incremental updates to
>      LastAttemptStatus.h into one patch (patch 2)
>   2. Move LastAttemptStatus.h from Include to PrivateInclude
>   3. Correct patch 1 subject from "FmpDevicePkg" to "MdePkg"
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Guomin Jiang <guomin.jiang@intel.com>
> Cc: Wei6 Xu <wei6.xu@intel.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Michael Kubacki (6):
>   MdePkg/SystemResourceTable.h: Add vendor range values
>   FmpDevicePkg: Add Last Attempt Status header files
>   FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status
>     capability
>   FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status
>     granularity
>   FmpDevicePkg: Add Last Attempt Status support to dependency libs
>   FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API
> 
>  FmpDevicePkg/FmpDxe/FmpDxe.c                                                     | 176 +++++++++++++++++---
>  FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c               |  39 +++--
>  FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c       |   9 +-
>  FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c                         |  96 +++++++++--
>  FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c                             |  48 ++++--
>  FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c |   7 +-
>  FmpDevicePkg/FmpDxe/FmpDxe.h                                                     |   4 +-
>  FmpDevicePkg/Include/LastAttemptStatus.h                                         |  96 +++++++++++
>  FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h                             |   8 +-
>  FmpDevicePkg/Include/Library/FmpDependencyLib.h                                  |  44 +++--
>  FmpDevicePkg/Include/Library/FmpDeviceLib.h                                      |  48 ++++--
>  FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h                               |  80 +++++++++
>  MdePkg/Include/Guid/SystemResourceTable.h                                        |  13 ++
>  13 files changed, 575 insertions(+), 93 deletions(-)
>  create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
>  create mode 100644 FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
> 
> --
> 2.28.0.windows.1

Re: [PATCH v3 0/6] Extend Last Attempt Status Usage

[Michael Kubacki]

Hi Mike,

1) Yes, we can certainly leave more of the unsuccessful vendor range 
available for future expansion. I believe we can also reduce the FMP 
reserved range. How about a length of 0x800 for both?

The ranges would then be defined as follows:

START     | END       | Usage
------------------------------------------------------------------|
0x1000    | 0x17FF    | FmpDevicePkg                              |
    0x1000 |    0x107F | FmpDxe driver                             |
    0x1080 |    0x109F | FMP dependencies (e.g. FmpDependencyLib)  |
0x1800    | 0x1FFF    | FmpDeviceLib instances implementation     |
0x2000    | 0x3FFF    | Unused. Available for future expansion.   |

Also, I don't see a problem with removing the length defines and 
directly specifying min/max defines for each range.

2.) I understand the compatibility concern but as you noted it is 
cleaner to maintain a single interface. I believe making the transition 
to the new API will only become more difficult in the future as it may 
go unnoticed resulting in implementations that don't implement support 
for this capability leading to an increasing amount of future effort to 
do work that could be done now. Perhaps we could get thoughts from 
others as well?

3.) I will update these to return back an expected value.

Thanks,
Michael

On 8/19/2020 7:57 PM, Kinney, Michael D wrote:
> Hi Michael,
> 
> A couple a couple general questions:
> 
> 1) I see the entire range from 0x2000-0x3FFF is reserved for FmpDeviceLib.  If we
>     every add more device/platform specific libs for FMP, there are no ranges available.
>     Should we limit the FmpDeviceLib to a smaller range to support future expansion?
> 
>     Also, the style of LastAttemptStatus.h with extra defines for the length of
>     each range is hard to read, and I do not think there are any consumers of the
>     length defines from this public include file.  Since there are really only 3
>     defined ranges, couldn't this be simplified to min/max defines for each range
>     for a total of 6 #defines.  I do not expect ranges (once defined) to change in
>     length, and the most that might happen in the future is adding new ranges for
>     new lib classes in the unused ranges.
> 
> 2) This series makes non-backwards compatible changes to some of the lib classes.
>     I agree this is the cleanest way to add support for the vendor specific
>     last attempt status.  It does mean that existing implementations will have
>     to update their lib implementations to be compatible with this new version.
>     I would be slightly cleaner to introduce new APIs with support for the
>     vendor specific last attempt status codes.  Then update all libs to produce
>     both the existing APIs and the new APIs (The old APIs can call the new APIs).
>     Then update FmpDxe to use the new APIs.  This would be 3 patch series.
>     If FmpDxe never calls the old APIs, then we could (at a future date)
>     delete the old APIs from the lib class and the lib implementations could
>     remove the old API that calls the new API.
> 
> 3) The following APIs in the Null implementations have OUT params.
>     Should these OUT params be set to an expected value?
> 
>       CheckFmpDependency()
>       FmpDeviceGetImage()
>       FmpDeviceSetImage()
> 
> Thanks,
> 
> Mike
>     
> 
>> -----Original Message-----
>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>> Sent: Tuesday, August 18, 2020 2:16 PM
>> To: devel@edk2.groups.io
>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Xu,
>> Wei6 <wei6.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
>> Subject: [PATCH v3 0/6] Extend Last Attempt Status Usage
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2802
>>
>> This patch series adds more granularity to Last Attempt Status
>> codes reported during FMP check image and set image operations
>> that greatly improve precision of the status codes.
>>
>> The unsuccessful vendor range (0x1000 - 0x4000) was introduced
>> in UEFI Specification 2.8. At a high-level, two subranges are
>> defined within that range in this patch series:
>>    1. The FMP Reserved range - reserved for components implemented
>>       in FmpDevicePkg.
>>    2. The FMP Device Library Reserved range - reserved for
>>       FmpDeviceLib instance-specific usage.
>>
>> The ranges are described in a public header file LastAttemptStatus.h
>> while the specific codes used within FmpDevicePkg implementation
>> are defined in a private header file FmpLastAttemptStatus.h.
>>
>> FmpDeviceLib instances should use the range definition from the
>> public header file to define Last Attempt Status codes local to
>> their library instance.
>>
>> Of note, there's multiple approaches to assigning private status
>> codes in the FMP Reserved range. For example, individual components
>> could define their last attempt status codes locally with the
>> range allocated to the component defined in a package-wide private
>> header file. However, one goal of the granularity being introduced
>> is to provide straightforward traceability to an error source.
>>
>> For that reason, it was chosen to define a constant set of codes at
>> the package level in FmpLastAttemptStatus.h. For example, if a new
>> FmpDependencyLib instance is added, it would not be able to reassign
>> status code values in the pre-existing FMP Dependency range; it
>> would reuse codes for the same error source and be able to add new
>> codes onto the range for its usage. I wanted to highlight this for
>> any feedback.
>>
>> V3 changes:
>>    1. Enhanced range definitions in LastAttemptStatus.h with more
>>       completeness providing length, min, and max values.
>>    2. Moved the actual Last Attempt Status code assignments to a
>>       private header file PrivateInclude/FmpLastAttemptStatus.h.
>>    3. Changed the value of
>>       LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
>>       to 0x3FFF instead of 0x4000 even though 0x4000 is defined in
>>       the UEFI specification. The length is 0x4000 but the max
>>       allowed value should be 0x3FFF. This change was made now to
>>       prevent implementation compatibility issues in the future.
>>    4. Included "DEVICE" in the following macro name to clearly
>>       associate it with the FmpDeviceLib library class:
>>       LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_xxx
>>    5. Included a map to help the reader better visualize the range
>>       definitions in LastAttemptStatus.h.
>>    6. Included additional documentation describing the enum in
>>       FmpLastAttemptStatus.h. An explicit statement stating that new
>>       codes should be added onto the end of ranges to preserve the
>>       values was added.
>>    7. Simplified error handling logic in FmpDxe for FmpDeviceLib
>>       calls that return Last Attempt Status.
>>    8. V2 had a single memory allocation failure code used for
>>       different memory allocations in CheckFmpDependency () in
>>       FmpDependencyLib. Each potential allocation failure was
>>       assigned a unique code.
>>
>> V2 changes:
>>    1. Consolidate all previous incremental updates to
>>       LastAttemptStatus.h into one patch (patch 2)
>>    2. Move LastAttemptStatus.h from Include to PrivateInclude
>>    3. Correct patch 1 subject from "FmpDevicePkg" to "MdePkg"
>>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Guomin Jiang <guomin.jiang@intel.com>
>> Cc: Wei6 Xu <wei6.xu@intel.com>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Michael Kubacki (6):
>>    MdePkg/SystemResourceTable.h: Add vendor range values
>>    FmpDevicePkg: Add Last Attempt Status header files
>>    FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status
>>      capability
>>    FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status
>>      granularity
>>    FmpDevicePkg: Add Last Attempt Status support to dependency libs
>>    FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API
>>
>>   FmpDevicePkg/FmpDxe/FmpDxe.c                                                     | 176 +++++++++++++++++---
>>   FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c               |  39 +++--
>>   FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c       |   9 +-
>>   FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c                         |  96 +++++++++--
>>   FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c                             |  48 ++++--
>>   FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c |   7 +-
>>   FmpDevicePkg/FmpDxe/FmpDxe.h                                                     |   4 +-
>>   FmpDevicePkg/Include/LastAttemptStatus.h                                         |  96 +++++++++++
>>   FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h                             |   8 +-
>>   FmpDevicePkg/Include/Library/FmpDependencyLib.h                                  |  44 +++--
>>   FmpDevicePkg/Include/Library/FmpDeviceLib.h                                      |  48 ++++--
>>   FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h                               |  80 +++++++++
>>   MdePkg/Include/Guid/SystemResourceTable.h                                        |  13 ++
>>   13 files changed, 575 insertions(+), 93 deletions(-)
>>   create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
>>   create mode 100644 FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
>>
>> --
>> 2.28.0.windows.1
> 

Re: [edk2-devel] [PATCH v3 0/6] Extend Last Attempt Status Usage

[Sean]

Michael,

#1
I would suggest calling out the sub-range

0x10A0 | 0x17FF  -- Free for Library Implementations of FmpDevicePkg

I also might suggest splitting FmpDependencyLib and 
FmpDependencyCheckLib ranges just to show a consistent pattern of how 
each library instance within FmpDevicePkg gets a defined range.


#2
Given that edk2 does not have any real FmpDeviceLibs (only instance is 
FmpDevicePkg\Library\FmpDeviceLibNull\FmpDeviceLibNull.inf).  Now is the 
best time to make a breaking change. It is very common for downstream 
code repositories to integrate edk2 and be forced to make changes 
associated with the new integration.  To me this is preferred.  A build 
break is easy to resolve. When functionality changes or new features are 
added but don't cause a break this is more difficult to integrate 
correctly.  Not only that, it leads to nearly everyone ignoring the 
change.  There is no need for this change to be a multi-year integration 
or cause extra development of shims and translation functions.  The API 
change is pretty easy.  The implementation can choose to to avoid the 
new ranges and just set the value to 1 (FMP unknown error).  This would 
match the behavior of today.  Obviously i believe it would better to 
take the extra time to create unique ranges for each of your libs.  Also 
note that if anyone is doing third party binary integration this is not 
a breaking change.  This only breaks for those doing a src build.

Thanks
Sean






On 8/20/2020 6:25 PM, Michael Kubacki wrote:
> Hi Mike,
> 
> 1) Yes, we can certainly leave more of the unsuccessful vendor range 
> available for future expansion. I believe we can also reduce the FMP 
> reserved range. How about a length of 0x800 for both?
> 
> The ranges would then be defined as follows:
> 
> START     | END       | Usage
> ------------------------------------------------------------------|
> 0x1000    | 0x17FF    | FmpDevicePkg                              |
>     0x1000 |    0x107F | FmpDxe driver                             |
>     0x1080 |    0x109F | FMP dependencies (e.g. FmpDependencyLib)  |
> 0x1800    | 0x1FFF    | FmpDeviceLib instances implementation     |
> 0x2000    | 0x3FFF    | Unused. Available for future expansion.   |
> 
> Also, I don't see a problem with removing the length defines and 
> directly specifying min/max defines for each range.
> 
> 2.) I understand the compatibility concern but as you noted it is 
> cleaner to maintain a single interface. I believe making the transition 
> to the new API will only become more difficult in the future as it may 
> go unnoticed resulting in implementations that don't implement support 
> for this capability leading to an increasing amount of future effort to 
> do work that could be done now. Perhaps we could get thoughts from 
> others as well?
> 
> 3.) I will update these to return back an expected value.
> 
> Thanks,
> Michael
> 
> On 8/19/2020 7:57 PM, Kinney, Michael D wrote:
>> Hi Michael,
>>
>> A couple a couple general questions:
>>
>> 1) I see the entire range from 0x2000-0x3FFF is reserved for 
>> FmpDeviceLib.  If we
>>     every add more device/platform specific libs for FMP, there are no 
>> ranges available.
>>     Should we limit the FmpDeviceLib to a smaller range to support 
>> future expansion?
>>
>>     Also, the style of LastAttemptStatus.h with extra defines for the 
>> length of
>>     each range is hard to read, and I do not think there are any 
>> consumers of the
>>     length defines from this public include file.  Since there are 
>> really only 3
>>     defined ranges, couldn't this be simplified to min/max defines for 
>> each range
>>     for a total of 6 #defines.  I do not expect ranges (once defined) 
>> to change in
>>     length, and the most that might happen in the future is adding new 
>> ranges for
>>     new lib classes in the unused ranges.
>>
>> 2) This series makes non-backwards compatible changes to some of the 
>> lib classes.
>>     I agree this is the cleanest way to add support for the vendor 
>> specific
>>     last attempt status.  It does mean that existing implementations 
>> will have
>>     to update their lib implementations to be compatible with this new 
>> version.
>>     I would be slightly cleaner to introduce new APIs with support for 
>> the
>>     vendor specific last attempt status codes.  Then update all libs 
>> to produce
>>     both the existing APIs and the new APIs (The old APIs can call the 
>> new APIs).
>>     Then update FmpDxe to use the new APIs.  This would be 3 patch 
>> series.
>>     If FmpDxe never calls the old APIs, then we could (at a future date)
>>     delete the old APIs from the lib class and the lib implementations 
>> could
>>     remove the old API that calls the new API.
>>
>> 3) The following APIs in the Null implementations have OUT params.
>>     Should these OUT params be set to an expected value?
>>
>>       CheckFmpDependency()
>>       FmpDeviceGetImage()
>>       FmpDeviceSetImage()
>>
>> Thanks,
>>
>> Mike
>>
>>> -----Original Message-----
>>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>>> Sent: Tuesday, August 18, 2020 2:16 PM
>>> To: devel@edk2.groups.io
>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D 
>>> <michael.d.kinney@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; 
>>> Xu,
>>> Wei6 <wei6.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
>>> Subject: [PATCH v3 0/6] Extend Last Attempt Status Usage
>>>
>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>
>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2802
>>>
>>> This patch series adds more granularity to Last Attempt Status
>>> codes reported during FMP check image and set image operations
>>> that greatly improve precision of the status codes.
>>>
>>> The unsuccessful vendor range (0x1000 - 0x4000) was introduced
>>> in UEFI Specification 2.8. At a high-level, two subranges are
>>> defined within that range in this patch series:
>>>    1. The FMP Reserved range - reserved for components implemented
>>>       in FmpDevicePkg.
>>>    2. The FMP Device Library Reserved range - reserved for
>>>       FmpDeviceLib instance-specific usage.
>>>
>>> The ranges are described in a public header file LastAttemptStatus.h
>>> while the specific codes used within FmpDevicePkg implementation
>>> are defined in a private header file FmpLastAttemptStatus.h.
>>>
>>> FmpDeviceLib instances should use the range definition from the
>>> public header file to define Last Attempt Status codes local to
>>> their library instance.
>>>
>>> Of note, there's multiple approaches to assigning private status
>>> codes in the FMP Reserved range. For example, individual components
>>> could define their last attempt status codes locally with the
>>> range allocated to the component defined in a package-wide private
>>> header file. However, one goal of the granularity being introduced
>>> is to provide straightforward traceability to an error source.
>>>
>>> For that reason, it was chosen to define a constant set of codes at
>>> the package level in FmpLastAttemptStatus.h. For example, if a new
>>> FmpDependencyLib instance is added, it would not be able to reassign
>>> status code values in the pre-existing FMP Dependency range; it
>>> would reuse codes for the same error source and be able to add new
>>> codes onto the range for its usage. I wanted to highlight this for
>>> any feedback.
>>>
>>> V3 changes:
>>>    1. Enhanced range definitions in LastAttemptStatus.h with more
>>>       completeness providing length, min, and max values.
>>>    2. Moved the actual Last Attempt Status code assignments to a
>>>       private header file PrivateInclude/FmpLastAttemptStatus.h.
>>>    3. Changed the value of
>>>       LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
>>>       to 0x3FFF instead of 0x4000 even though 0x4000 is defined in
>>>       the UEFI specification. The length is 0x4000 but the max
>>>       allowed value should be 0x3FFF. This change was made now to
>>>       prevent implementation compatibility issues in the future.
>>>    4. Included "DEVICE" in the following macro name to clearly
>>>       associate it with the FmpDeviceLib library class:
>>>       LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_xxx
>>>    5. Included a map to help the reader better visualize the range
>>>       definitions in LastAttemptStatus.h.
>>>    6. Included additional documentation describing the enum in
>>>       FmpLastAttemptStatus.h. An explicit statement stating that new
>>>       codes should be added onto the end of ranges to preserve the
>>>       values was added.
>>>    7. Simplified error handling logic in FmpDxe for FmpDeviceLib
>>>       calls that return Last Attempt Status.
>>>    8. V2 had a single memory allocation failure code used for
>>>       different memory allocations in CheckFmpDependency () in
>>>       FmpDependencyLib. Each potential allocation failure was
>>>       assigned a unique code.
>>>
>>> V2 changes:
>>>    1. Consolidate all previous incremental updates to
>>>       LastAttemptStatus.h into one patch (patch 2)
>>>    2. Move LastAttemptStatus.h from Include to PrivateInclude
>>>    3. Correct patch 1 subject from "FmpDevicePkg" to "MdePkg"
>>>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Guomin Jiang <guomin.jiang@intel.com>
>>> Cc: Wei6 Xu <wei6.xu@intel.com>
>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>
>>> Michael Kubacki (6):
>>>    MdePkg/SystemResourceTable.h: Add vendor range values
>>>    FmpDevicePkg: Add Last Attempt Status header files
>>>    FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status
>>>      capability
>>>    FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status
>>>      granularity
>>>    FmpDevicePkg: Add Last Attempt Status support to dependency libs
>>>    FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API
>>>
>>>   
>>> FmpDevicePkg/FmpDxe/FmpDxe.c                                                     
>>> | 176 +++++++++++++++++---
>>>   
>>> FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c               
>>> |  39 +++--
>>>   
>>> FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c       
>>> |   9 +-
>>>   
>>> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c                         
>>> |  96 +++++++++--
>>>   
>>> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c                             
>>> |  48 ++++--
>>>   
>>> FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c 
>>> |   7 +-
>>>   
>>> FmpDevicePkg/FmpDxe/FmpDxe.h                                                     
>>> |   4 +-
>>>   
>>> FmpDevicePkg/Include/LastAttemptStatus.h                                         
>>> |  96 +++++++++++
>>>   
>>> FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h                             
>>> |   8 +-
>>>   
>>> FmpDevicePkg/Include/Library/FmpDependencyLib.h                       
>>> |  44 +++--
>>>   
>>> FmpDevicePkg/Include/Library/FmpDeviceLib.h                           
>>> |  48 ++++--
>>>   
>>> FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h                               
>>> |  80 +++++++++
>>>   
>>> MdePkg/Include/Guid/SystemResourceTable.h                             
>>> |  13 ++
>>>   13 files changed, 575 insertions(+), 93 deletions(-)
>>>   create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
>>>   create mode 100644 FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
>>>
>>> -- 
>>> 2.28.0.windows.1
>>
> 
> 
> 

Re: [edk2-devel] [PATCH v3 0/6] Extend Last Attempt Status Usage

[Michael Kubacki]

Hi Sean,

Thanks for the feedback.

#1 - I will include both suggestions in v4.

Thanks,
Michael

On 8/20/2020 7:37 PM, Sean Brogan wrote:
> Michael,
> 
> #1
> I would suggest calling out the sub-range
> 
> 0x10A0 | 0x17FF  -- Free for Library Implementations of FmpDevicePkg
> 
> I also might suggest splitting FmpDependencyLib and 
> FmpDependencyCheckLib ranges just to show a consistent pattern of how 
> each library instance within FmpDevicePkg gets a defined range.
> 
> 
> #2
> Given that edk2 does not have any real FmpDeviceLibs (only instance is 
> FmpDevicePkg\Library\FmpDeviceLibNull\FmpDeviceLibNull.inf).  Now is the 
> best time to make a breaking change. It is very common for downstream 
> code repositories to integrate edk2 and be forced to make changes 
> associated with the new integration.  To me this is preferred.  A build 
> break is easy to resolve. When functionality changes or new features are 
> added but don't cause a break this is more difficult to integrate 
> correctly.  Not only that, it leads to nearly everyone ignoring the 
> change.  There is no need for this change to be a multi-year integration 
> or cause extra development of shims and translation functions.  The API 
> change is pretty easy.  The implementation can choose to to avoid the 
> new ranges and just set the value to 1 (FMP unknown error).  This would 
> match the behavior of today.  Obviously i believe it would better to 
> take the extra time to create unique ranges for each of your libs.  Also 
> note that if anyone is doing third party binary integration this is not 
> a breaking change.  This only breaks for those doing a src build.
> 
> Thanks
> Sean
> 
> 
> 
> 
> 
> 
> On 8/20/2020 6:25 PM, Michael Kubacki wrote:
>> Hi Mike,
>>
>> 1) Yes, we can certainly leave more of the unsuccessful vendor range 
>> available for future expansion. I believe we can also reduce the FMP 
>> reserved range. How about a length of 0x800 for both?
>>
>> The ranges would then be defined as follows:
>>
>> START     | END       | Usage
>> ------------------------------------------------------------------|
>> 0x1000    | 0x17FF    | FmpDevicePkg                              |
>>     0x1000 |    0x107F | FmpDxe driver                             |
>>     0x1080 |    0x109F | FMP dependencies (e.g. FmpDependencyLib)  |
>> 0x1800    | 0x1FFF    | FmpDeviceLib instances implementation     |
>> 0x2000    | 0x3FFF    | Unused. Available for future expansion.   |
>>
>> Also, I don't see a problem with removing the length defines and 
>> directly specifying min/max defines for each range.
>>
>> 2.) I understand the compatibility concern but as you noted it is 
>> cleaner to maintain a single interface. I believe making the 
>> transition to the new API will only become more difficult in the 
>> future as it may go unnoticed resulting in implementations that don't 
>> implement support for this capability leading to an increasing amount 
>> of future effort to do work that could be done now. Perhaps we could 
>> get thoughts from others as well?
>>
>> 3.) I will update these to return back an expected value.
>>
>> Thanks,
>> Michael
>>
>> On 8/19/2020 7:57 PM, Kinney, Michael D wrote:
>>> Hi Michael,
>>>
>>> A couple a couple general questions:
>>>
>>> 1) I see the entire range from 0x2000-0x3FFF is reserved for 
>>> FmpDeviceLib.  If we
>>>     every add more device/platform specific libs for FMP, there are 
>>> no ranges available.
>>>     Should we limit the FmpDeviceLib to a smaller range to support 
>>> future expansion?
>>>
>>>     Also, the style of LastAttemptStatus.h with extra defines for the 
>>> length of
>>>     each range is hard to read, and I do not think there are any 
>>> consumers of the
>>>     length defines from this public include file.  Since there are 
>>> really only 3
>>>     defined ranges, couldn't this be simplified to min/max defines 
>>> for each range
>>>     for a total of 6 #defines.  I do not expect ranges (once defined) 
>>> to change in
>>>     length, and the most that might happen in the future is adding 
>>> new ranges for
>>>     new lib classes in the unused ranges.
>>>
>>> 2) This series makes non-backwards compatible changes to some of the 
>>> lib classes.
>>>     I agree this is the cleanest way to add support for the vendor 
>>> specific
>>>     last attempt status.  It does mean that existing implementations 
>>> will have
>>>     to update their lib implementations to be compatible with this 
>>> new version.
>>>     I would be slightly cleaner to introduce new APIs with support 
>>> for the
>>>     vendor specific last attempt status codes.  Then update all libs 
>>> to produce
>>>     both the existing APIs and the new APIs (The old APIs can call 
>>> the new APIs).
>>>     Then update FmpDxe to use the new APIs.  This would be 3 patch 
>>> series.
>>>     If FmpDxe never calls the old APIs, then we could (at a future date)
>>>     delete the old APIs from the lib class and the lib 
>>> implementations could
>>>     remove the old API that calls the new API.
>>>
>>> 3) The following APIs in the Null implementations have OUT params.
>>>     Should these OUT params be set to an expected value?
>>>
>>>       CheckFmpDependency()
>>>       FmpDeviceGetImage()
>>>       FmpDeviceSetImage()
>>>
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>>>> Sent: Tuesday, August 18, 2020 2:16 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D 
>>>> <michael.d.kinney@intel.com>; Jiang, Guomin 
>>>> <guomin.jiang@intel.com>; Xu,
>>>> Wei6 <wei6.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
>>>> Subject: [PATCH v3 0/6] Extend Last Attempt Status Usage
>>>>
>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>
>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2802
>>>>
>>>> This patch series adds more granularity to Last Attempt Status
>>>> codes reported during FMP check image and set image operations
>>>> that greatly improve precision of the status codes.
>>>>
>>>> The unsuccessful vendor range (0x1000 - 0x4000) was introduced
>>>> in UEFI Specification 2.8. At a high-level, two subranges are
>>>> defined within that range in this patch series:
>>>>    1. The FMP Reserved range - reserved for components implemented
>>>>       in FmpDevicePkg.
>>>>    2. The FMP Device Library Reserved range - reserved for
>>>>       FmpDeviceLib instance-specific usage.
>>>>
>>>> The ranges are described in a public header file LastAttemptStatus.h
>>>> while the specific codes used within FmpDevicePkg implementation
>>>> are defined in a private header file FmpLastAttemptStatus.h.
>>>>
>>>> FmpDeviceLib instances should use the range definition from the
>>>> public header file to define Last Attempt Status codes local to
>>>> their library instance.
>>>>
>>>> Of note, there's multiple approaches to assigning private status
>>>> codes in the FMP Reserved range. For example, individual components
>>>> could define their last attempt status codes locally with the
>>>> range allocated to the component defined in a package-wide private
>>>> header file. However, one goal of the granularity being introduced
>>>> is to provide straightforward traceability to an error source.
>>>>
>>>> For that reason, it was chosen to define a constant set of codes at
>>>> the package level in FmpLastAttemptStatus.h. For example, if a new
>>>> FmpDependencyLib instance is added, it would not be able to reassign
>>>> status code values in the pre-existing FMP Dependency range; it
>>>> would reuse codes for the same error source and be able to add new
>>>> codes onto the range for its usage. I wanted to highlight this for
>>>> any feedback.
>>>>
>>>> V3 changes:
>>>>    1. Enhanced range definitions in LastAttemptStatus.h with more
>>>>       completeness providing length, min, and max values.
>>>>    2. Moved the actual Last Attempt Status code assignments to a
>>>>       private header file PrivateInclude/FmpLastAttemptStatus.h.
>>>>    3. Changed the value of
>>>>       LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
>>>>       to 0x3FFF instead of 0x4000 even though 0x4000 is defined in
>>>>       the UEFI specification. The length is 0x4000 but the max
>>>>       allowed value should be 0x3FFF. This change was made now to
>>>>       prevent implementation compatibility issues in the future.
>>>>    4. Included "DEVICE" in the following macro name to clearly
>>>>       associate it with the FmpDeviceLib library class:
>>>>       LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_xxx
>>>>    5. Included a map to help the reader better visualize the range
>>>>       definitions in LastAttemptStatus.h.
>>>>    6. Included additional documentation describing the enum in
>>>>       FmpLastAttemptStatus.h. An explicit statement stating that new
>>>>       codes should be added onto the end of ranges to preserve the
>>>>       values was added.
>>>>    7. Simplified error handling logic in FmpDxe for FmpDeviceLib
>>>>       calls that return Last Attempt Status.
>>>>    8. V2 had a single memory allocation failure code used for
>>>>       different memory allocations in CheckFmpDependency () in
>>>>       FmpDependencyLib. Each potential allocation failure was
>>>>       assigned a unique code.
>>>>
>>>> V2 changes:
>>>>    1. Consolidate all previous incremental updates to
>>>>       LastAttemptStatus.h into one patch (patch 2)
>>>>    2. Move LastAttemptStatus.h from Include to PrivateInclude
>>>>    3. Correct patch 1 subject from "FmpDevicePkg" to "MdePkg"
>>>>
>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>> Cc: Guomin Jiang <guomin.jiang@intel.com>
>>>> Cc: Wei6 Xu <wei6.xu@intel.com>
>>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>
>>>> Michael Kubacki (6):
>>>>    MdePkg/SystemResourceTable.h: Add vendor range values
>>>>    FmpDevicePkg: Add Last Attempt Status header files
>>>>    FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status
>>>>      capability
>>>>    FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status
>>>>      granularity
>>>>    FmpDevicePkg: Add Last Attempt Status support to dependency libs
>>>>    FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API
>>>>
>>>> FmpDevicePkg/FmpDxe/FmpDxe.c | 176 +++++++++++++++++---
>>>> FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c 
>>>> |  39 +++--
>>>> FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c 
>>>> |   9 +-
>>>> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c |  96 
>>>> +++++++++--
>>>> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c |  48 ++++--
>>>> FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c 
>>>> |   7 +-
>>>> FmpDevicePkg/FmpDxe/FmpDxe.h |   4 +-
>>>> FmpDevicePkg/Include/LastAttemptStatus.h |  96 +++++++++++
>>>> FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h |   8 +-
>>>> FmpDevicePkg/Include/Library/FmpDependencyLib.h |  44 +++--
>>>> FmpDevicePkg/Include/Library/FmpDeviceLib.h |  48 ++++--
>>>> FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h |  80 +++++++++
>>>> MdePkg/Include/Guid/SystemResourceTable.h |  13 ++
>>>>   13 files changed, 575 insertions(+), 93 deletions(-)
>>>>   create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
>>>>   create mode 100644 FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
>>>>
>>>> -- 
>>>> 2.28.0.windows.1
>>>
>>
>> 
>>

Re: [edk2-devel] [PATCH v3 0/6] Extend Last Attempt Status Usage

[Michael Kubacki]

Hi Mike,

We've stated a few reasons we prefer to change the library API now. Do 
you find that acceptable for v4?

Thanks,
Michael

On 8/21/2020 2:25 PM, Michael Kubacki wrote:
> Hi Sean,
> 
> Thanks for the feedback.
> 
> #1 - I will include both suggestions in v4.
> 
> Thanks,
> Michael
> 
> On 8/20/2020 7:37 PM, Sean Brogan wrote:
>> Michael,
>>
>> #1
>> I would suggest calling out the sub-range
>>
>> 0x10A0 | 0x17FF  -- Free for Library Implementations of FmpDevicePkg
>>
>> I also might suggest splitting FmpDependencyLib and 
>> FmpDependencyCheckLib ranges just to show a consistent pattern of how 
>> each library instance within FmpDevicePkg gets a defined range.
>>
>>
>> #2
>> Given that edk2 does not have any real FmpDeviceLibs (only instance is 
>> FmpDevicePkg\Library\FmpDeviceLibNull\FmpDeviceLibNull.inf).  Now is 
>> the best time to make a breaking change. It is very common for 
>> downstream code repositories to integrate edk2 and be forced to make 
>> changes associated with the new integration.  To me this is 
>> preferred.  A build break is easy to resolve. When functionality 
>> changes or new features are added but don't cause a break this is more 
>> difficult to integrate correctly.  Not only that, it leads to nearly 
>> everyone ignoring the change.  There is no need for this change to be 
>> a multi-year integration or cause extra development of shims and 
>> translation functions.  The API change is pretty easy.  The 
>> implementation can choose to to avoid the new ranges and just set the 
>> value to 1 (FMP unknown error).  This would match the behavior of 
>> today.  Obviously i believe it would better to take the extra time to 
>> create unique ranges for each of your libs.  Also note that if anyone 
>> is doing third party binary integration this is not a breaking 
>> change.  This only breaks for those doing a src build.
>>
>> Thanks
>> Sean
>>
>>
>>
>>
>>
>>
>> On 8/20/2020 6:25 PM, Michael Kubacki wrote:
>>> Hi Mike,
>>>
>>> 1) Yes, we can certainly leave more of the unsuccessful vendor range 
>>> available for future expansion. I believe we can also reduce the FMP 
>>> reserved range. How about a length of 0x800 for both?
>>>
>>> The ranges would then be defined as follows:
>>>
>>> START     | END       | Usage
>>> ------------------------------------------------------------------|
>>> 0x1000    | 0x17FF    | FmpDevicePkg                              |
>>>     0x1000 |    0x107F | FmpDxe driver                             |
>>>     0x1080 |    0x109F | FMP dependencies (e.g. FmpDependencyLib)  |
>>> 0x1800    | 0x1FFF    | FmpDeviceLib instances implementation     |
>>> 0x2000    | 0x3FFF    | Unused. Available for future expansion.   |
>>>
>>> Also, I don't see a problem with removing the length defines and 
>>> directly specifying min/max defines for each range.
>>>
>>> 2.) I understand the compatibility concern but as you noted it is 
>>> cleaner to maintain a single interface. I believe making the 
>>> transition to the new API will only become more difficult in the 
>>> future as it may go unnoticed resulting in implementations that don't 
>>> implement support for this capability leading to an increasing amount 
>>> of future effort to do work that could be done now. Perhaps we could 
>>> get thoughts from others as well?
>>>
>>> 3.) I will update these to return back an expected value.
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 8/19/2020 7:57 PM, Kinney, Michael D wrote:
>>>> Hi Michael,
>>>>
>>>> A couple a couple general questions:
>>>>
>>>> 1) I see the entire range from 0x2000-0x3FFF is reserved for 
>>>> FmpDeviceLib.  If we
>>>>     every add more device/platform specific libs for FMP, there are 
>>>> no ranges available.
>>>>     Should we limit the FmpDeviceLib to a smaller range to support 
>>>> future expansion?
>>>>
>>>>     Also, the style of LastAttemptStatus.h with extra defines for 
>>>> the length of
>>>>     each range is hard to read, and I do not think there are any 
>>>> consumers of the
>>>>     length defines from this public include file.  Since there are 
>>>> really only 3
>>>>     defined ranges, couldn't this be simplified to min/max defines 
>>>> for each range
>>>>     for a total of 6 #defines.  I do not expect ranges (once 
>>>> defined) to change in
>>>>     length, and the most that might happen in the future is adding 
>>>> new ranges for
>>>>     new lib classes in the unused ranges.
>>>>
>>>> 2) This series makes non-backwards compatible changes to some of the 
>>>> lib classes.
>>>>     I agree this is the cleanest way to add support for the vendor 
>>>> specific
>>>>     last attempt status.  It does mean that existing implementations 
>>>> will have
>>>>     to update their lib implementations to be compatible with this 
>>>> new version.
>>>>     I would be slightly cleaner to introduce new APIs with support 
>>>> for the
>>>>     vendor specific last attempt status codes.  Then update all libs 
>>>> to produce
>>>>     both the existing APIs and the new APIs (The old APIs can call 
>>>> the new APIs).
>>>>     Then update FmpDxe to use the new APIs.  This would be 3 patch 
>>>> series.
>>>>     If FmpDxe never calls the old APIs, then we could (at a future 
>>>> date)
>>>>     delete the old APIs from the lib class and the lib 
>>>> implementations could
>>>>     remove the old API that calls the new API.
>>>>
>>>> 3) The following APIs in the Null implementations have OUT params.
>>>>     Should these OUT params be set to an expected value?
>>>>
>>>>       CheckFmpDependency()
>>>>       FmpDeviceGetImage()
>>>>       FmpDeviceSetImage()
>>>>
>>>> Thanks,
>>>>
>>>> Mike
>>>>
>>>>> -----Original Message-----
>>>>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>>>>> Sent: Tuesday, August 18, 2020 2:16 PM
>>>>> To: devel@edk2.groups.io
>>>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D 
>>>>> <michael.d.kinney@intel.com>; Jiang, Guomin 
>>>>> <guomin.jiang@intel.com>; Xu,
>>>>> Wei6 <wei6.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
>>>>> Subject: [PATCH v3 0/6] Extend Last Attempt Status Usage
>>>>>
>>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>
>>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2802
>>>>>
>>>>> This patch series adds more granularity to Last Attempt Status
>>>>> codes reported during FMP check image and set image operations
>>>>> that greatly improve precision of the status codes.
>>>>>
>>>>> The unsuccessful vendor range (0x1000 - 0x4000) was introduced
>>>>> in UEFI Specification 2.8. At a high-level, two subranges are
>>>>> defined within that range in this patch series:
>>>>>    1. The FMP Reserved range - reserved for components implemented
>>>>>       in FmpDevicePkg.
>>>>>    2. The FMP Device Library Reserved range - reserved for
>>>>>       FmpDeviceLib instance-specific usage.
>>>>>
>>>>> The ranges are described in a public header file LastAttemptStatus.h
>>>>> while the specific codes used within FmpDevicePkg implementation
>>>>> are defined in a private header file FmpLastAttemptStatus.h.
>>>>>
>>>>> FmpDeviceLib instances should use the range definition from the
>>>>> public header file to define Last Attempt Status codes local to
>>>>> their library instance.
>>>>>
>>>>> Of note, there's multiple approaches to assigning private status
>>>>> codes in the FMP Reserved range. For example, individual components
>>>>> could define their last attempt status codes locally with the
>>>>> range allocated to the component defined in a package-wide private
>>>>> header file. However, one goal of the granularity being introduced
>>>>> is to provide straightforward traceability to an error source.
>>>>>
>>>>> For that reason, it was chosen to define a constant set of codes at
>>>>> the package level in FmpLastAttemptStatus.h. For example, if a new
>>>>> FmpDependencyLib instance is added, it would not be able to reassign
>>>>> status code values in the pre-existing FMP Dependency range; it
>>>>> would reuse codes for the same error source and be able to add new
>>>>> codes onto the range for its usage. I wanted to highlight this for
>>>>> any feedback.
>>>>>
>>>>> V3 changes:
>>>>>    1. Enhanced range definitions in LastAttemptStatus.h with more
>>>>>       completeness providing length, min, and max values.
>>>>>    2. Moved the actual Last Attempt Status code assignments to a
>>>>>       private header file PrivateInclude/FmpLastAttemptStatus.h.
>>>>>    3. Changed the value of
>>>>>       LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
>>>>>       to 0x3FFF instead of 0x4000 even though 0x4000 is defined in
>>>>>       the UEFI specification. The length is 0x4000 but the max
>>>>>       allowed value should be 0x3FFF. This change was made now to
>>>>>       prevent implementation compatibility issues in the future.
>>>>>    4. Included "DEVICE" in the following macro name to clearly
>>>>>       associate it with the FmpDeviceLib library class:
>>>>>       LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_xxx
>>>>>    5. Included a map to help the reader better visualize the range
>>>>>       definitions in LastAttemptStatus.h.
>>>>>    6. Included additional documentation describing the enum in
>>>>>       FmpLastAttemptStatus.h. An explicit statement stating that new
>>>>>       codes should be added onto the end of ranges to preserve the
>>>>>       values was added.
>>>>>    7. Simplified error handling logic in FmpDxe for FmpDeviceLib
>>>>>       calls that return Last Attempt Status.
>>>>>    8. V2 had a single memory allocation failure code used for
>>>>>       different memory allocations in CheckFmpDependency () in
>>>>>       FmpDependencyLib. Each potential allocation failure was
>>>>>       assigned a unique code.
>>>>>
>>>>> V2 changes:
>>>>>    1. Consolidate all previous incremental updates to
>>>>>       LastAttemptStatus.h into one patch (patch 2)
>>>>>    2. Move LastAttemptStatus.h from Include to PrivateInclude
>>>>>    3. Correct patch 1 subject from "FmpDevicePkg" to "MdePkg"
>>>>>
>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>> Cc: Guomin Jiang <guomin.jiang@intel.com>
>>>>> Cc: Wei6 Xu <wei6.xu@intel.com>
>>>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>
>>>>> Michael Kubacki (6):
>>>>>    MdePkg/SystemResourceTable.h: Add vendor range values
>>>>>    FmpDevicePkg: Add Last Attempt Status header files
>>>>>    FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status
>>>>>      capability
>>>>>    FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status
>>>>>      granularity
>>>>>    FmpDevicePkg: Add Last Attempt Status support to dependency libs
>>>>>    FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API
>>>>>
>>>>> FmpDevicePkg/FmpDxe/FmpDxe.c | 176 +++++++++++++++++---
>>>>> FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c 
>>>>> |  39 +++--
>>>>> FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c 
>>>>> |   9 +-
>>>>> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c |  96 
>>>>> +++++++++--
>>>>> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c |  48 ++++--
>>>>> FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c 
>>>>> |   7 +-
>>>>> FmpDevicePkg/FmpDxe/FmpDxe.h |   4 +-
>>>>> FmpDevicePkg/Include/LastAttemptStatus.h |  96 +++++++++++
>>>>> FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h |   8 +-
>>>>> FmpDevicePkg/Include/Library/FmpDependencyLib.h |  44 +++--
>>>>> FmpDevicePkg/Include/Library/FmpDeviceLib.h |  48 ++++--
>>>>> FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h |  80 +++++++++
>>>>> MdePkg/Include/Guid/SystemResourceTable.h |  13 ++
>>>>>   13 files changed, 575 insertions(+), 93 deletions(-)
>>>>>   create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
>>>>>   create mode 100644 
>>>>> FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
>>>>>
>>>>> -- 
>>>>> 2.28.0.windows.1
>>>>
>>>
>>> 
>>>

Re: [edk2-devel] [PATCH v3 0/6] Extend Last Attempt Status Usage

[Michael D Kinney]

Hi Michael,

There are implementations of FMpDeviceLib in edk2-platforms.  So the challenge is how
to prepare a set of patch series to edk2 and edk2-platforms where there are no 
intermediate states that break builds.  Adding new APIs and later removing the 
old APIs is one way to meet this requirement.  Do you have other ideas?

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Friday, August 28, 2020 12:21 PM
> To: Sean Brogan <spbrogan@outlook.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Gao, Liming <liming.gao@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Xu, Wei6 <wei6.xu@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v3 0/6] Extend Last Attempt Status Usage
> 
> Hi Mike,
> 
> We've stated a few reasons we prefer to change the library API now. Do
> you find that acceptable for v4?
> 
> Thanks,
> Michael
> 
> On 8/21/2020 2:25 PM, Michael Kubacki wrote:
> > Hi Sean,
> >
> > Thanks for the feedback.
> >
> > #1 - I will include both suggestions in v4.
> >
> > Thanks,
> > Michael
> >
> > On 8/20/2020 7:37 PM, Sean Brogan wrote:
> >> Michael,
> >>
> >> #1
> >> I would suggest calling out the sub-range
> >>
> >> 0x10A0 | 0x17FF  -- Free for Library Implementations of FmpDevicePkg
> >>
> >> I also might suggest splitting FmpDependencyLib and
> >> FmpDependencyCheckLib ranges just to show a consistent pattern of how
> >> each library instance within FmpDevicePkg gets a defined range.
> >>
> >>
> >> #2
> >> Given that edk2 does not have any real FmpDeviceLibs (only instance is
> >> FmpDevicePkg\Library\FmpDeviceLibNull\FmpDeviceLibNull.inf).  Now is
> >> the best time to make a breaking change. It is very common for
> >> downstream code repositories to integrate edk2 and be forced to make
> >> changes associated with the new integration.  To me this is
> >> preferred.  A build break is easy to resolve. When functionality
> >> changes or new features are added but don't cause a break this is more
> >> difficult to integrate correctly.  Not only that, it leads to nearly
> >> everyone ignoring the change.  There is no need for this change to be
> >> a multi-year integration or cause extra development of shims and
> >> translation functions.  The API change is pretty easy.  The
> >> implementation can choose to to avoid the new ranges and just set the
> >> value to 1 (FMP unknown error).  This would match the behavior of
> >> today.  Obviously i believe it would better to take the extra time to
> >> create unique ranges for each of your libs.  Also note that if anyone
> >> is doing third party binary integration this is not a breaking
> >> change.  This only breaks for those doing a src build.
> >>
> >> Thanks
> >> Sean
> >>
> >>
> >>
> >>
> >>
> >>
> >> On 8/20/2020 6:25 PM, Michael Kubacki wrote:
> >>> Hi Mike,
> >>>
> >>> 1) Yes, we can certainly leave more of the unsuccessful vendor range
> >>> available for future expansion. I believe we can also reduce the FMP
> >>> reserved range. How about a length of 0x800 for both?
> >>>
> >>> The ranges would then be defined as follows:
> >>>
> >>> START     | END       | Usage
> >>> ------------------------------------------------------------------|
> >>> 0x1000    | 0x17FF    | FmpDevicePkg                              |
> >>>     0x1000 |    0x107F | FmpDxe driver                             |
> >>>     0x1080 |    0x109F | FMP dependencies (e.g. FmpDependencyLib)  |
> >>> 0x1800    | 0x1FFF    | FmpDeviceLib instances implementation     |
> >>> 0x2000    | 0x3FFF    | Unused. Available for future expansion.   |
> >>>
> >>> Also, I don't see a problem with removing the length defines and
> >>> directly specifying min/max defines for each range.
> >>>
> >>> 2.) I understand the compatibility concern but as you noted it is
> >>> cleaner to maintain a single interface. I believe making the
> >>> transition to the new API will only become more difficult in the
> >>> future as it may go unnoticed resulting in implementations that don't
> >>> implement support for this capability leading to an increasing amount
> >>> of future effort to do work that could be done now. Perhaps we could
> >>> get thoughts from others as well?
> >>>
> >>> 3.) I will update these to return back an expected value.
> >>>
> >>> Thanks,
> >>> Michael
> >>>
> >>> On 8/19/2020 7:57 PM, Kinney, Michael D wrote:
> >>>> Hi Michael,
> >>>>
> >>>> A couple a couple general questions:
> >>>>
> >>>> 1) I see the entire range from 0x2000-0x3FFF is reserved for
> >>>> FmpDeviceLib.  If we
> >>>>     every add more device/platform specific libs for FMP, there are
> >>>> no ranges available.
> >>>>     Should we limit the FmpDeviceLib to a smaller range to support
> >>>> future expansion?
> >>>>
> >>>>     Also, the style of LastAttemptStatus.h with extra defines for
> >>>> the length of
> >>>>     each range is hard to read, and I do not think there are any
> >>>> consumers of the
> >>>>     length defines from this public include file.  Since there are
> >>>> really only 3
> >>>>     defined ranges, couldn't this be simplified to min/max defines
> >>>> for each range
> >>>>     for a total of 6 #defines.  I do not expect ranges (once
> >>>> defined) to change in
> >>>>     length, and the most that might happen in the future is adding
> >>>> new ranges for
> >>>>     new lib classes in the unused ranges.
> >>>>
> >>>> 2) This series makes non-backwards compatible changes to some of the
> >>>> lib classes.
> >>>>     I agree this is the cleanest way to add support for the vendor
> >>>> specific
> >>>>     last attempt status.  It does mean that existing implementations
> >>>> will have
> >>>>     to update their lib implementations to be compatible with this
> >>>> new version.
> >>>>     I would be slightly cleaner to introduce new APIs with support
> >>>> for the
> >>>>     vendor specific last attempt status codes.  Then update all libs
> >>>> to produce
> >>>>     both the existing APIs and the new APIs (The old APIs can call
> >>>> the new APIs).
> >>>>     Then update FmpDxe to use the new APIs.  This would be 3 patch
> >>>> series.
> >>>>     If FmpDxe never calls the old APIs, then we could (at a future
> >>>> date)
> >>>>     delete the old APIs from the lib class and the lib
> >>>> implementations could
> >>>>     remove the old API that calls the new API.
> >>>>
> >>>> 3) The following APIs in the Null implementations have OUT params.
> >>>>     Should these OUT params be set to an expected value?
> >>>>
> >>>>       CheckFmpDependency()
> >>>>       FmpDeviceGetImage()
> >>>>       FmpDeviceSetImage()
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Mike
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
> >>>>> Sent: Tuesday, August 18, 2020 2:16 PM
> >>>>> To: devel@edk2.groups.io
> >>>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> >>>>> <michael.d.kinney@intel.com>; Jiang, Guomin
> >>>>> <guomin.jiang@intel.com>; Xu,
> >>>>> Wei6 <wei6.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
> >>>>> Subject: [PATCH v3 0/6] Extend Last Attempt Status Usage
> >>>>>
> >>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
> >>>>>
> >>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2802
> >>>>>
> >>>>> This patch series adds more granularity to Last Attempt Status
> >>>>> codes reported during FMP check image and set image operations
> >>>>> that greatly improve precision of the status codes.
> >>>>>
> >>>>> The unsuccessful vendor range (0x1000 - 0x4000) was introduced
> >>>>> in UEFI Specification 2.8. At a high-level, two subranges are
> >>>>> defined within that range in this patch series:
> >>>>>    1. The FMP Reserved range - reserved for components implemented
> >>>>>       in FmpDevicePkg.
> >>>>>    2. The FMP Device Library Reserved range - reserved for
> >>>>>       FmpDeviceLib instance-specific usage.
> >>>>>
> >>>>> The ranges are described in a public header file LastAttemptStatus.h
> >>>>> while the specific codes used within FmpDevicePkg implementation
> >>>>> are defined in a private header file FmpLastAttemptStatus.h.
> >>>>>
> >>>>> FmpDeviceLib instances should use the range definition from the
> >>>>> public header file to define Last Attempt Status codes local to
> >>>>> their library instance.
> >>>>>
> >>>>> Of note, there's multiple approaches to assigning private status
> >>>>> codes in the FMP Reserved range. For example, individual components
> >>>>> could define their last attempt status codes locally with the
> >>>>> range allocated to the component defined in a package-wide private
> >>>>> header file. However, one goal of the granularity being introduced
> >>>>> is to provide straightforward traceability to an error source.
> >>>>>
> >>>>> For that reason, it was chosen to define a constant set of codes at
> >>>>> the package level in FmpLastAttemptStatus.h. For example, if a new
> >>>>> FmpDependencyLib instance is added, it would not be able to reassign
> >>>>> status code values in the pre-existing FMP Dependency range; it
> >>>>> would reuse codes for the same error source and be able to add new
> >>>>> codes onto the range for its usage. I wanted to highlight this for
> >>>>> any feedback.
> >>>>>
> >>>>> V3 changes:
> >>>>>    1. Enhanced range definitions in LastAttemptStatus.h with more
> >>>>>       completeness providing length, min, and max values.
> >>>>>    2. Moved the actual Last Attempt Status code assignments to a
> >>>>>       private header file PrivateInclude/FmpLastAttemptStatus.h.
> >>>>>    3. Changed the value of
> >>>>>       LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
> >>>>>       to 0x3FFF instead of 0x4000 even though 0x4000 is defined in
> >>>>>       the UEFI specification. The length is 0x4000 but the max
> >>>>>       allowed value should be 0x3FFF. This change was made now to
> >>>>>       prevent implementation compatibility issues in the future.
> >>>>>    4. Included "DEVICE" in the following macro name to clearly
> >>>>>       associate it with the FmpDeviceLib library class:
> >>>>>       LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_xxx
> >>>>>    5. Included a map to help the reader better visualize the range
> >>>>>       definitions in LastAttemptStatus.h.
> >>>>>    6. Included additional documentation describing the enum in
> >>>>>       FmpLastAttemptStatus.h. An explicit statement stating that new
> >>>>>       codes should be added onto the end of ranges to preserve the
> >>>>>       values was added.
> >>>>>    7. Simplified error handling logic in FmpDxe for FmpDeviceLib
> >>>>>       calls that return Last Attempt Status.
> >>>>>    8. V2 had a single memory allocation failure code used for
> >>>>>       different memory allocations in CheckFmpDependency () in
> >>>>>       FmpDependencyLib. Each potential allocation failure was
> >>>>>       assigned a unique code.
> >>>>>
> >>>>> V2 changes:
> >>>>>    1. Consolidate all previous incremental updates to
> >>>>>       LastAttemptStatus.h into one patch (patch 2)
> >>>>>    2. Move LastAttemptStatus.h from Include to PrivateInclude
> >>>>>    3. Correct patch 1 subject from "FmpDevicePkg" to "MdePkg"
> >>>>>
> >>>>> Cc: Liming Gao <liming.gao@intel.com>
> >>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >>>>> Cc: Guomin Jiang <guomin.jiang@intel.com>
> >>>>> Cc: Wei6 Xu <wei6.xu@intel.com>
> >>>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> >>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> >>>>>
> >>>>> Michael Kubacki (6):
> >>>>>    MdePkg/SystemResourceTable.h: Add vendor range values
> >>>>>    FmpDevicePkg: Add Last Attempt Status header files
> >>>>>    FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status
> >>>>>      capability
> >>>>>    FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status
> >>>>>      granularity
> >>>>>    FmpDevicePkg: Add Last Attempt Status support to dependency libs
> >>>>>    FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API
> >>>>>
> >>>>> FmpDevicePkg/FmpDxe/FmpDxe.c | 176 +++++++++++++++++---
> >>>>> FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
> >>>>> |  39 +++--
> >>>>> FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c
> >>>>> |   9 +-
> >>>>> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c |  96
> >>>>> +++++++++--
> >>>>> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c |  48 ++++--
> >>>>> FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
> >>>>> |   7 +-
> >>>>> FmpDevicePkg/FmpDxe/FmpDxe.h |   4 +-
> >>>>> FmpDevicePkg/Include/LastAttemptStatus.h |  96 +++++++++++
> >>>>> FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h |   8 +-
> >>>>> FmpDevicePkg/Include/Library/FmpDependencyLib.h |  44 +++--
> >>>>> FmpDevicePkg/Include/Library/FmpDeviceLib.h |  48 ++++--
> >>>>> FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h |  80 +++++++++
> >>>>> MdePkg/Include/Guid/SystemResourceTable.h |  13 ++
> >>>>>   13 files changed, 575 insertions(+), 93 deletions(-)
> >>>>>   create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
> >>>>>   create mode 100644
> >>>>> FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
> >>>>>
> >>>>> --
> >>>>> 2.28.0.windows.1
> >>>>
> >>>
> >>>
> >>>
> 
> 

Re: [edk2-devel] [PATCH v3 0/6] Extend Last Attempt Status Usage

[Michael Kubacki]

Hi Mike,

I agree that's a good way to meet the requirement. Maintaining the two 
APIs would just be a transient state (i.e. on order of days) to update 
edk2-platforms users and then we could remove the old API, right?

Our main concern was maintaining the two APIs side-by-side long term.

Thanks,
Michael

On 8/28/2020 12:25 PM, Kinney, Michael D wrote:
> Hi Michael,
> 
> There are implementations of FMpDeviceLib in edk2-platforms.  So the challenge is how
> to prepare a set of patch series to edk2 and edk2-platforms where there are no
> intermediate states that break builds.  Adding new APIs and later removing the
> old APIs is one way to meet this requirement.  Do you have other ideas?
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Friday, August 28, 2020 12:21 PM
>> To: Sean Brogan <spbrogan@outlook.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>> Cc: Gao, Liming <liming.gao@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Xu, Wei6 <wei6.xu@intel.com>; Liu, Zhiguang
>> <zhiguang.liu@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v3 0/6] Extend Last Attempt Status Usage
>>
>> Hi Mike,
>>
>> We've stated a few reasons we prefer to change the library API now. Do
>> you find that acceptable for v4?
>>
>> Thanks,
>> Michael
>>
>> On 8/21/2020 2:25 PM, Michael Kubacki wrote:
>>> Hi Sean,
>>>
>>> Thanks for the feedback.
>>>
>>> #1 - I will include both suggestions in v4.
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 8/20/2020 7:37 PM, Sean Brogan wrote:
>>>> Michael,
>>>>
>>>> #1
>>>> I would suggest calling out the sub-range
>>>>
>>>> 0x10A0 | 0x17FF  -- Free for Library Implementations of FmpDevicePkg
>>>>
>>>> I also might suggest splitting FmpDependencyLib and
>>>> FmpDependencyCheckLib ranges just to show a consistent pattern of how
>>>> each library instance within FmpDevicePkg gets a defined range.
>>>>
>>>>
>>>> #2
>>>> Given that edk2 does not have any real FmpDeviceLibs (only instance is
>>>> FmpDevicePkg\Library\FmpDeviceLibNull\FmpDeviceLibNull.inf).  Now is
>>>> the best time to make a breaking change. It is very common for
>>>> downstream code repositories to integrate edk2 and be forced to make
>>>> changes associated with the new integration.  To me this is
>>>> preferred.  A build break is easy to resolve. When functionality
>>>> changes or new features are added but don't cause a break this is more
>>>> difficult to integrate correctly.  Not only that, it leads to nearly
>>>> everyone ignoring the change.  There is no need for this change to be
>>>> a multi-year integration or cause extra development of shims and
>>>> translation functions.  The API change is pretty easy.  The
>>>> implementation can choose to to avoid the new ranges and just set the
>>>> value to 1 (FMP unknown error).  This would match the behavior of
>>>> today.  Obviously i believe it would better to take the extra time to
>>>> create unique ranges for each of your libs.  Also note that if anyone
>>>> is doing third party binary integration this is not a breaking
>>>> change.  This only breaks for those doing a src build.
>>>>
>>>> Thanks
>>>> Sean
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On 8/20/2020 6:25 PM, Michael Kubacki wrote:
>>>>> Hi Mike,
>>>>>
>>>>> 1) Yes, we can certainly leave more of the unsuccessful vendor range
>>>>> available for future expansion. I believe we can also reduce the FMP
>>>>> reserved range. How about a length of 0x800 for both?
>>>>>
>>>>> The ranges would then be defined as follows:
>>>>>
>>>>> START     | END       | Usage
>>>>> ------------------------------------------------------------------|
>>>>> 0x1000    | 0x17FF    | FmpDevicePkg                              |
>>>>>      0x1000 |    0x107F | FmpDxe driver                             |
>>>>>      0x1080 |    0x109F | FMP dependencies (e.g. FmpDependencyLib)  |
>>>>> 0x1800    | 0x1FFF    | FmpDeviceLib instances implementation     |
>>>>> 0x2000    | 0x3FFF    | Unused. Available for future expansion.   |
>>>>>
>>>>> Also, I don't see a problem with removing the length defines and
>>>>> directly specifying min/max defines for each range.
>>>>>
>>>>> 2.) I understand the compatibility concern but as you noted it is
>>>>> cleaner to maintain a single interface. I believe making the
>>>>> transition to the new API will only become more difficult in the
>>>>> future as it may go unnoticed resulting in implementations that don't
>>>>> implement support for this capability leading to an increasing amount
>>>>> of future effort to do work that could be done now. Perhaps we could
>>>>> get thoughts from others as well?
>>>>>
>>>>> 3.) I will update these to return back an expected value.
>>>>>
>>>>> Thanks,
>>>>> Michael
>>>>>
>>>>> On 8/19/2020 7:57 PM, Kinney, Michael D wrote:
>>>>>> Hi Michael,
>>>>>>
>>>>>> A couple a couple general questions:
>>>>>>
>>>>>> 1) I see the entire range from 0x2000-0x3FFF is reserved for
>>>>>> FmpDeviceLib.  If we
>>>>>>      every add more device/platform specific libs for FMP, there are
>>>>>> no ranges available.
>>>>>>      Should we limit the FmpDeviceLib to a smaller range to support
>>>>>> future expansion?
>>>>>>
>>>>>>      Also, the style of LastAttemptStatus.h with extra defines for
>>>>>> the length of
>>>>>>      each range is hard to read, and I do not think there are any
>>>>>> consumers of the
>>>>>>      length defines from this public include file.  Since there are
>>>>>> really only 3
>>>>>>      defined ranges, couldn't this be simplified to min/max defines
>>>>>> for each range
>>>>>>      for a total of 6 #defines.  I do not expect ranges (once
>>>>>> defined) to change in
>>>>>>      length, and the most that might happen in the future is adding
>>>>>> new ranges for
>>>>>>      new lib classes in the unused ranges.
>>>>>>
>>>>>> 2) This series makes non-backwards compatible changes to some of the
>>>>>> lib classes.
>>>>>>      I agree this is the cleanest way to add support for the vendor
>>>>>> specific
>>>>>>      last attempt status.  It does mean that existing implementations
>>>>>> will have
>>>>>>      to update their lib implementations to be compatible with this
>>>>>> new version.
>>>>>>      I would be slightly cleaner to introduce new APIs with support
>>>>>> for the
>>>>>>      vendor specific last attempt status codes.  Then update all libs
>>>>>> to produce
>>>>>>      both the existing APIs and the new APIs (The old APIs can call
>>>>>> the new APIs).
>>>>>>      Then update FmpDxe to use the new APIs.  This would be 3 patch
>>>>>> series.
>>>>>>      If FmpDxe never calls the old APIs, then we could (at a future
>>>>>> date)
>>>>>>      delete the old APIs from the lib class and the lib
>>>>>> implementations could
>>>>>>      remove the old API that calls the new API.
>>>>>>
>>>>>> 3) The following APIs in the Null implementations have OUT params.
>>>>>>      Should these OUT params be set to an expected value?
>>>>>>
>>>>>>        CheckFmpDependency()
>>>>>>        FmpDeviceGetImage()
>>>>>>        FmpDeviceSetImage()
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Mike
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>>>>>>> Sent: Tuesday, August 18, 2020 2:16 PM
>>>>>>> To: devel@edk2.groups.io
>>>>>>> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>>>>>>> <michael.d.kinney@intel.com>; Jiang, Guomin
>>>>>>> <guomin.jiang@intel.com>; Xu,
>>>>>>> Wei6 <wei6.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
>>>>>>> Subject: [PATCH v3 0/6] Extend Last Attempt Status Usage
>>>>>>>
>>>>>>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>>>
>>>>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2802
>>>>>>>
>>>>>>> This patch series adds more granularity to Last Attempt Status
>>>>>>> codes reported during FMP check image and set image operations
>>>>>>> that greatly improve precision of the status codes.
>>>>>>>
>>>>>>> The unsuccessful vendor range (0x1000 - 0x4000) was introduced
>>>>>>> in UEFI Specification 2.8. At a high-level, two subranges are
>>>>>>> defined within that range in this patch series:
>>>>>>>     1. The FMP Reserved range - reserved for components implemented
>>>>>>>        in FmpDevicePkg.
>>>>>>>     2. The FMP Device Library Reserved range - reserved for
>>>>>>>        FmpDeviceLib instance-specific usage.
>>>>>>>
>>>>>>> The ranges are described in a public header file LastAttemptStatus.h
>>>>>>> while the specific codes used within FmpDevicePkg implementation
>>>>>>> are defined in a private header file FmpLastAttemptStatus.h.
>>>>>>>
>>>>>>> FmpDeviceLib instances should use the range definition from the
>>>>>>> public header file to define Last Attempt Status codes local to
>>>>>>> their library instance.
>>>>>>>
>>>>>>> Of note, there's multiple approaches to assigning private status
>>>>>>> codes in the FMP Reserved range. For example, individual components
>>>>>>> could define their last attempt status codes locally with the
>>>>>>> range allocated to the component defined in a package-wide private
>>>>>>> header file. However, one goal of the granularity being introduced
>>>>>>> is to provide straightforward traceability to an error source.
>>>>>>>
>>>>>>> For that reason, it was chosen to define a constant set of codes at
>>>>>>> the package level in FmpLastAttemptStatus.h. For example, if a new
>>>>>>> FmpDependencyLib instance is added, it would not be able to reassign
>>>>>>> status code values in the pre-existing FMP Dependency range; it
>>>>>>> would reuse codes for the same error source and be able to add new
>>>>>>> codes onto the range for its usage. I wanted to highlight this for
>>>>>>> any feedback.
>>>>>>>
>>>>>>> V3 changes:
>>>>>>>     1. Enhanced range definitions in LastAttemptStatus.h with more
>>>>>>>        completeness providing length, min, and max values.
>>>>>>>     2. Moved the actual Last Attempt Status code assignments to a
>>>>>>>        private header file PrivateInclude/FmpLastAttemptStatus.h.
>>>>>>>     3. Changed the value of
>>>>>>>        LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
>>>>>>>        to 0x3FFF instead of 0x4000 even though 0x4000 is defined in
>>>>>>>        the UEFI specification. The length is 0x4000 but the max
>>>>>>>        allowed value should be 0x3FFF. This change was made now to
>>>>>>>        prevent implementation compatibility issues in the future.
>>>>>>>     4. Included "DEVICE" in the following macro name to clearly
>>>>>>>        associate it with the FmpDeviceLib library class:
>>>>>>>        LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_xxx
>>>>>>>     5. Included a map to help the reader better visualize the range
>>>>>>>        definitions in LastAttemptStatus.h.
>>>>>>>     6. Included additional documentation describing the enum in
>>>>>>>        FmpLastAttemptStatus.h. An explicit statement stating that new
>>>>>>>        codes should be added onto the end of ranges to preserve the
>>>>>>>        values was added.
>>>>>>>     7. Simplified error handling logic in FmpDxe for FmpDeviceLib
>>>>>>>        calls that return Last Attempt Status.
>>>>>>>     8. V2 had a single memory allocation failure code used for
>>>>>>>        different memory allocations in CheckFmpDependency () in
>>>>>>>        FmpDependencyLib. Each potential allocation failure was
>>>>>>>        assigned a unique code.
>>>>>>>
>>>>>>> V2 changes:
>>>>>>>     1. Consolidate all previous incremental updates to
>>>>>>>        LastAttemptStatus.h into one patch (patch 2)
>>>>>>>     2. Move LastAttemptStatus.h from Include to PrivateInclude
>>>>>>>     3. Correct patch 1 subject from "FmpDevicePkg" to "MdePkg"
>>>>>>>
>>>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>>>> Cc: Guomin Jiang <guomin.jiang@intel.com>
>>>>>>> Cc: Wei6 Xu <wei6.xu@intel.com>
>>>>>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>>>>>>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>>>>>>
>>>>>>> Michael Kubacki (6):
>>>>>>>     MdePkg/SystemResourceTable.h: Add vendor range values
>>>>>>>     FmpDevicePkg: Add Last Attempt Status header files
>>>>>>>     FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status
>>>>>>>       capability
>>>>>>>     FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status
>>>>>>>       granularity
>>>>>>>     FmpDevicePkg: Add Last Attempt Status support to dependency libs
>>>>>>>     FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API
>>>>>>>
>>>>>>> FmpDevicePkg/FmpDxe/FmpDxe.c | 176 +++++++++++++++++---
>>>>>>> FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
>>>>>>> |  39 +++--
>>>>>>> FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c
>>>>>>> |   9 +-
>>>>>>> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c |  96
>>>>>>> +++++++++--
>>>>>>> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c |  48 ++++--
>>>>>>> FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
>>>>>>> |   7 +-
>>>>>>> FmpDevicePkg/FmpDxe/FmpDxe.h |   4 +-
>>>>>>> FmpDevicePkg/Include/LastAttemptStatus.h |  96 +++++++++++
>>>>>>> FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h |   8 +-
>>>>>>> FmpDevicePkg/Include/Library/FmpDependencyLib.h |  44 +++--
>>>>>>> FmpDevicePkg/Include/Library/FmpDeviceLib.h |  48 ++++--
>>>>>>> FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h |  80 +++++++++
>>>>>>> MdePkg/Include/Guid/SystemResourceTable.h |  13 ++
>>>>>>>    13 files changed, 575 insertions(+), 93 deletions(-)
>>>>>>>    create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
>>>>>>>    create mode 100644
>>>>>>> FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
>>>>>>>
>>>>>>> --
>>>>>>> 2.28.0.windows.1
>>>>>>
>>>>>
>>>>>
>>>>>
>>
>> 
>