[edk2-devel] [PATCH v3 0/6] Extend Last Attempt Status Usage
Michael D Kinney
michael.d.kinney at intel.com
Thu Aug 20 02:57:22 UTC 2020
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 (#64478): https://edk2.groups.io/g/devel/message/64478
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