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: [email protected] <[email protected]>
Sent: Tuesday, August 18, 2020 2:16 PM
To: [email protected]
Cc: Gao, Liming <[email protected]>; Kinney, Michael D
<[email protected]>; Jiang, Guomin <[email protected]>;
Xu,
Wei6 <[email protected]>; Liu, Zhiguang <[email protected]>
Subject: [PATCH v3 0/6] Extend Last Attempt Status Usage
From: Michael Kubacki <[email protected]>
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 <[email protected]>
Cc: Michael D Kinney <[email protected]>
Cc: Guomin Jiang <[email protected]>
Cc: Wei6 Xu <[email protected]>
Cc: Zhiguang Liu <[email protected]>
Signed-off-by: Michael Kubacki <[email protected]>
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 (#64518): https://edk2.groups.io/g/devel/message/64518
Mute This Topic: https://groups.io/mt/76274480/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
