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

["Xu, Wei6" <wei6.xu@intel.com>]

Hi Michael,

Thanks for the updates. The patch set is good to me.
Reviewed-by: Wei6 Xu <wei6.xu@intel.com>

BR,
Wei
-----Original Message-----
From: Michael Kubacki <michael.kubacki@outlook.com> 
Sent: Saturday, October 17, 2020 3:46 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io
Cc: Liming Gao <gaoliming@byosoft.com.cn>; Jiang, Guomin <guomin.jiang@intel.com>; Xu, Wei6 <wei6.xu@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>
Subject: Re: [PATCH v4 0/6] Extend Last Attempt Status Usage

I can definitely make those changes.

I haven't heard much from others (Liming, Wei, Zhiguang). If you could please indicate your status on the patch series (use v5) with a reply of either acceptance or suggestions that would be great before I send out
v6 with these updates.

Thanks,
Michael

On 10/16/2020 8:33 AM, Kinney, Michael D wrote:
> Hi Michael,
> 
> Thank you for the updates.  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 <michael.kubacki@outlook.com>
>> Sent: Thursday, October 15, 2020 2:20 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: Re: [PATCH v4 0/6] Extend Last Attempt Status Usage
>>
>> Hi all,
>>
>> It's been about two weeks since this email and I still haven't seen 
>> any feedback on v4.
>>
>> I made a very small update that resulted in a v5 series today:
>> https://edk2.groups.io/g/devel/message/66287
>>
>> If you could please provide timely feedback on v5 it would be appreciated.
>>
>> Also, there's an issue building FmpDevicePkg at the moment that 
>> you'll need this patch to fix:
>> https://edk2.groups.io/g/devel/message/66286
>>
>> That patch needs review as well.
>>
>> Thanks,
>> Michael
>>
>> On 10/2/2020 9:26 AM, Michael Kubacki wrote:
>>> It is going on a week now and I haven't seen a response to this 
>>> patch series yet. Please review it when possible.
>>>
>>> On a somewhat related note, I made the changes that should be 
>>> necessary in edk2-platforms for backward compatibility in this patch:
>>> https://edk2.groups.io/g/devel/message/65821
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 9/25/2020 7:19 PM, michael.kubacki@outlook.com wrote:
>>>> 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.
>>>>
>>>> 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/FmpDependencyCheckLi
>>>> bNull.c
>>>> |  14 +-
>>>>
>>>> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
>>>> |  93 +++++++++++--
>>>>
>>>> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
>>>> | 132 +++++++++++++++++-
>>>>
>>>> FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDepende
>>>> ncyUnitTest.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
>>>>