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

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

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
>>>
>>
>> 
>>