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

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

Hi Mike,

These changes will be made on top of v5.

Thanks,
Michael

On 10/16/2020 3:57 PM, Michael D Kinney wrote:
> Hi Michael,
> 
> Thank you for the updates.  This feedback was meant for V5, and not V4.
> 
> A couple minor comments:
> 
> 1) FmpDevicePkg/Library/FmpDeviceLibNull
> 
>     In order to demonstrate the preferred implementation I think we should
>     have FmpDeviceCheckImage() call FmpDeviceCheckImageWithStatus() and
>     FmpDeviceSetImage() call FmpDeviceSetImageWithStatus().  This way, it
>     will be clear to FmpDeviceLib developers that they only need to
>     implement the *WithStatus() version.
> 
> 2) FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
> 
>     FmpDependencyCheckLib comment block should use /// instead of // to be
>     consistent with the rest of the include file.
> 
> Thanks,
> 
> Mike
> 
>> -----Original Message-----
>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>> Sent: Thursday, October 15, 2020 2:11 PM
>> To: devel@edk2.groups.io
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>; 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 v5 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.
>>
>> V5 changes:
>>    1. Fixed an issue where
>>       LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_CERTIFICATE is changed
>>       to LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_AUTH_FAILURE in the
>>       logic to return the last attempt status code in
>>       CheckTheImageInternal().
>>
>> V4 changes:
>>    1. Simplified range value definitions in LastAttemptStatus.h.
>>       Directly assign the values in the macro definition instead
>>       of using calculations.
>>    2. Adjusted range sizes to leave more room for future expansion.
>>
>>       OLD:
>>       START     | END       | Usage
>>       ------------------------------------------------|
>>       0x1000    | 0x1FFF    | FmpDevicePkg            |
>>          0x1000 |    0x107F | FmpDxe driver           |
>>          0x1080 |    0x109F | FMP dependency Libs     |
>>       0x2000    | 0x3FFF    | FmpDeviceLib instances  |
>>
>>       NEW:
>>       START     | END       | Usage
>>       ----------------------------------------------------------------|
>>       0x1000    | 0x17FF    | FmpDevicePkg                            |
>>          0x1000 |    0x107F | FmpDxe driver                           |
>>          0x1080 |    0x109F | FmpDependencyLib                        |
>>          0x10A0 |    0x10BF | FmpDependencyCheckLib                   |
>>          0x10C0 |    0x17FF | Unused. Available for future expansion. |
>>       0x1800    | 0x1FFF    | FmpDeviceLib instances implementation   |
>>       0x2000    | 0x3FFF    | Unused. Available for future expansion. |
>>
>>    3. Broke the single range in v3 for FMP Dependency libraries into
>>       separate ranges.
>>    4. Clarified LastAttemptStatus return values in each function
>>       description.
>>    5. Returned an expected LastAttemptStatus value for some functions
>>       that previously did not return a value.
>>    6. Reverted changes in FmpDxe to call the new FmpDeviceLib APIs
>>       for FmpDeviceCheckImage () and FmpDeviceSetImage (). These will
>>       be added in a future series after impacted platforms in
>>       edk2-platforms are updated to use the new APIs.
>>    7. Instead of directly changing the pre-existing APIs in
>>       FmpDeviceLib to add a LastAttemptStatus parameter, the new
>>       functions were added to the library interface:
>>         * FmpDeviceCheckImageWithStatus ()
>>         * FmpDeviceSetImageWithStatus ()
>>
>> 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 <gaoliming@byosoft.com.cn>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Guomin Jiang <guomin.jiang@intel.com>
>> Cc: Wei6 Xu <wei6.xu@intel.com>
>> 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                                                     | 146 +++++++++++++++++---
>>   FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c               |  39 ++++--
>>   FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c       |  14 +-
>>   FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c                         |  93 +++++++++++--
>>   FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c                             | 132 +++++++++++++++++-
>>   FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c |   7 +-
>>   FmpDevicePkg/FmpDxe/FmpDxe.h                                                     |   4 +-
>>   FmpDevicePkg/Include/LastAttemptStatus.h                                         |  81 +++++++++++
>>   FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h                             |   8 +-
>>   FmpDevicePkg/Include/Library/FmpDependencyLib.h                                  |  44 ++++--
>>   FmpDevicePkg/Include/Library/FmpDeviceLib.h                                      | 121 +++++++++++++++-
>>   FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h                               |  81 +++++++++++
>>   MdePkg/Include/Guid/SystemResourceTable.h                                        |  13 ++
>>   13 files changed, 718 insertions(+), 65 deletions(-)
>>   create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
>>   create mode 100644 FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
>>
>> --
>> 2.28.0.windows.1
> 
> 
> 
> 
> 
>