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

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

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