[edk2-devel] [PATCH v3 0/6] Extend Last Attempt Status Usage
Michael Kubacki
michael.kubacki at outlook.com
Fri Aug 28 19:20:49 UTC 2020
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 at outlook.com <michael.kubacki at outlook.com>
>>>>> Sent: Tuesday, August 18, 2020 2:16 PM
>>>>> To: devel at edk2.groups.io
>>>>> Cc: Gao, Liming <liming.gao at intel.com>; Kinney, Michael D
>>>>> <michael.d.kinney at intel.com>; Jiang, Guomin
>>>>> <guomin.jiang at intel.com>; Xu,
>>>>> Wei6 <wei6.xu at intel.com>; Liu, Zhiguang <zhiguang.liu at intel.com>
>>>>> Subject: [PATCH v3 0/6] Extend Last Attempt Status Usage
>>>>>
>>>>> From: Michael Kubacki <michael.kubacki at 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 at intel.com>
>>>>> Cc: Michael D Kinney <michael.d.kinney at intel.com>
>>>>> Cc: Guomin Jiang <guomin.jiang at intel.com>
>>>>> Cc: Wei6 Xu <wei6.xu at intel.com>
>>>>> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
>>>>> Signed-off-by: Michael Kubacki <michael.kubacki at 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
>>>>
>>>
>>>
>>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64778): https://edk2.groups.io/g/devel/message/64778
Mute This Topic: https://groups.io/mt/76274480/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list