Re: [PATCH v4 0/6] Extend Last Attempt Status Usage

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Michael,

Thank you for the updates.  A couple minor comments:

1) FmpDevicePkg/Library/FmpDeviceLibNull

   In order to demonstrate the preferred implementation I think we should
   have FmpDeviceCheckImage() call FmpDeviceCheckImageWithStatus() and
   FmpDeviceSetImage() call FmpDeviceSetImageWithStatus().  This way, it
   will be clear to FmpDeviceLib developers that they only need to 
   implement the *WithStatus() version.

2) FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h

   FmpDependencyCheckLib comment block should use /// instead of // to be
   consistent with the rest of the include file.

Thanks,

Mike


> -----Original Message-----
> From: Michael Kubacki <michael.kubacki@outlook.com>
> Sent: Thursday, October 15, 2020 2:20 PM
> To: devel@edk2.groups.io
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; 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: Re: [PATCH v4 0/6] Extend Last Attempt Status Usage
> 
> Hi all,
> 
> It's been about two weeks since this email and I still haven't seen any
> feedback on v4.
> 
> I made a very small update that resulted in a v5 series today:
> https://edk2.groups.io/g/devel/message/66287
> 
> If you could please provide timely feedback on v5 it would be appreciated.
> 
> Also, there's an issue building FmpDevicePkg at the moment that you'll
> need this patch to fix:
> https://edk2.groups.io/g/devel/message/66286
> 
> That patch needs review as well.
> 
> Thanks,
> Michael
> 
> On 10/2/2020 9:26 AM, Michael Kubacki wrote:
> > It is going on a week now and I haven't seen a response to this patch
> > series yet. Please review it when possible.
> >
> > On a somewhat related note, I made the changes that should be necessary
> > in edk2-platforms for backward compatibility in this patch:
> > https://edk2.groups.io/g/devel/message/65821
> >
> > Thanks,
> > Michael
> >
> > On 9/25/2020 7:19 PM, michael.kubacki@outlook.com wrote:
> >> 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.
> >>
> >> V4 changes:
> >>    1. Simplified range value definitions in LastAttemptStatus.h.
> >>       Directly assign the values in the macro definition instead
> >>       of using calculations.
> >>    2. Adjusted range sizes to leave more room for future expansion.
> >>
> >>       OLD:
> >>       START     | END       | Usage
> >>       ------------------------------------------------|
> >>       0x1000    | 0x1FFF    | FmpDevicePkg            |
> >>          0x1000 |    0x107F | FmpDxe driver           |
> >>          0x1080 |    0x109F | FMP dependency Libs     |
> >>       0x2000    | 0x3FFF    | FmpDeviceLib instances  |
> >>
> >>       NEW:
> >>       START     | END       | Usage
> >>       ----------------------------------------------------------------|
> >>       0x1000    | 0x17FF    | FmpDevicePkg                            |
> >>          0x1000 |    0x107F | FmpDxe driver                           |
> >>          0x1080 |    0x109F | FmpDependencyLib                        |
> >>          0x10A0 |    0x10BF | FmpDependencyCheckLib                   |
> >>          0x10C0 |    0x17FF | Unused. Available for future expansion. |
> >>       0x1800    | 0x1FFF    | FmpDeviceLib instances implementation   |
> >>       0x2000    | 0x3FFF    | Unused. Available for future expansion. |
> >>
> >>    3. Broke the single range in v3 for FMP Dependency libraries into
> >>       separate ranges.
> >>    4. Clarified LastAttemptStatus return values in each function
> >>       description.
> >>    5. Returned an expected LastAttemptStatus value for some functions
> >>       that previously did not return a value.
> >>    6. Reverted changes in FmpDxe to call the new FmpDeviceLib APIs
> >>       for FmpDeviceCheckImage () and FmpDeviceSetImage (). These will
> >>       be added in a future series after impacted platforms in
> >>       edk2-platforms are updated to use the new APIs.
> >>    7. Instead of directly changing the pre-existing APIs in
> >>       FmpDeviceLib to add a LastAttemptStatus parameter, the new
> >>       functions were added to the library interface:
> >>         * FmpDeviceCheckImageWithStatus ()
> >>         * FmpDeviceSetImageWithStatus ()
> >>
> >> 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 <gaoliming@byosoft.com.cn>
> >> 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
> >> | 146 +++++++++++++++++---
> >>
> >> FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
> >> |  39 ++++--
> >>
> >> FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c
> >> |  14 +-
> >>
> >> FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
> >> |  93 +++++++++++--
> >>
> >> FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
> >> | 132 +++++++++++++++++-
> >>
> >> FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
> >> |   7 +-
> >>
> >> FmpDevicePkg/FmpDxe/FmpDxe.h
> >> |   4 +-
> >>
> >> FmpDevicePkg/Include/LastAttemptStatus.h
> >> |  81 +++++++++++
> >>
> >> FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h
> >> |   8 +-
> >>
> >> FmpDevicePkg/Include/Library/FmpDependencyLib.h
> >> |  44 ++++--
> >>
> >> FmpDevicePkg/Include/Library/FmpDeviceLib.h
> >> | 121 +++++++++++++++-
> >>
> >> FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
> >> |  81 +++++++++++
> >>
> >> MdePkg/Include/Guid/SystemResourceTable.h
> >> |  13 ++
> >>   13 files changed, 718 insertions(+), 65 deletions(-)
> >>   create mode 100644 FmpDevicePkg/Include/LastAttemptStatus.h
> >>   create mode 100644 FmpDevicePkg/PrivateInclude/FmpLastAttemptStatus.h
> >>