Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h

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

#1: In v3, I'm going to split it such that the defines are in the public 
header and the enum specifying the internal driver and dependency ranges 
are in a private header to FmpDevicePkg.

Here's the current set of v3 changes to agree upon before sending a series:
1. Move the defines for the ranges to a public header
2. Move the enum to a private instance file
3. Rename LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MIN_ERROR_CODE to 
LAST_ATTEMPT_STATUS_DEVICE_LIBRARY_ERROR_MIN_ERROR_CODE
4. Include a comment to explicitly state new codes within a given range 
must be added at the end of the range

Please let me know if there's any further feedback.

Thanks,
Michael

On 8/10/2020 5:31 PM, Desimone, Nathaniel L wrote:
> My feedback:
> 
> #1: Why is LastAttemptStatus.h in PrivateInclude? Seems like something you would want to have as a public header.
> 
> #2: If someone inserts a new enum value in the middle of LAST_ATTEMPT_STATUS_EXPANDED_ERROR_LIST it will make it difficult to decode error codes in the future. Either put a comment that new error code should go on the bottom. Or add some space between each entry using something like this:
> 
> enum LAST_ATTEMPT_STATUS_EXPANDED_ERROR_LIST
> {
>    LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN,
>    LAST_ATTEMPT_STATUS_DRIVER_ERROR_PROGRESS_CALLBACK_ERROR = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 10,
>    LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_POWER_API = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 20,
>    LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_THERMAL_API = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 30,
>    LAST_ATTEMPT_STATUS_DRIVER_ERROR_THERMAL = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 40,
>    LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_ENV_API = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 50,
>    LAST_ATTEMPT_STATUS_DRIVER_ERROR_SYSTEM_ENV = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + 60,
> 
> Then you can insert something in the middle by adding +5.
> 
> Thanks,
> Nate
> 
> On 8/10/20, 1:28 PM, "devel@edk2.groups.io on behalf of Michael Kubacki" <devel@edk2.groups.io on behalf of michael.kubacki@outlook.com> wrote:
> 
>      From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
>      Introduces a header file to contain Last Attempt Status codes that
>      define granular FmpDevicePkg usage of the UEFI Specification
>      defined vendor range. The vendor range is described in UEFI
>      Specification 2.8A section 23.4.
> 
>      With this change, FmpDevicePkg currently defines three subranges of
>      the Last Attempt Status vendor range:
>        1. Driver - Codes returned from operations performed by the
>           FmpDxe driver.
>        2. Dependency - Codes returned from FMP dependency related
>           functionality (e.g. FmpDependencyLib).
>        3. Library - Codes returned from FmpDeviceLib instances.
> 
>      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>
>      Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>      ---
>       FmpDevicePkg/PrivateInclude/LastAttemptStatus.h | 81 ++++++++++++++++++++
>       1 file changed, 81 insertions(+)
> 
>      diff --git a/FmpDevicePkg/PrivateInclude/LastAttemptStatus.h b/FmpDevicePkg/PrivateInclude/LastAttemptStatus.h
>      new file mode 100644
>      index 000000000000..01e96b23edad
>      --- /dev/null
>      +++ b/FmpDevicePkg/PrivateInclude/LastAttemptStatus.h
>      @@ -0,0 +1,81 @@
>      +/** @file
>      +  Defines last attempt status codes used in FmpDevicePkg.
>      +
>      +  Copyright (c) Microsoft Corporation.<BR>
>      +
>      +  SPDX-License-Identifier: BSD-2-Clause-Patent
>      +
>      +**/
>      +
>      +#ifndef __FMP_LAST_ATTEMPT_STATUS_H__
>      +#define __FMP_LAST_ATTEMPT_STATUS_H__
>      +
>      +//
>      +// Size of the error code range for FMP driver-specific errors
>      +//
>      +#define LAST_ATTEMPT_STATUS_DRIVER_ERROR_COUNT              0x80
>      +
>      +//
>      +// Size of the error code range for FMP dependency related errors
>      +//
>      +#define LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_COUNT          0x20
>      +
>      +#define LAST_ATTEMPT_STATUS_DRIVER_MAX_ERROR_CODE_VALUE     LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN + \
>      +                                                            LAST_ATTEMPT_STATUS_DRIVER_ERROR_COUNT - 1
>      +
>      +#define LAST_ATTEMPT_STATUS_DEPENDENCY_MAX_ERROR_CODE_VALUE LAST_ATTEMPT_STATUS_DRIVER_MAX_ERROR_CODE_VALUE + \
>      +                                                            LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_COUNT
>      +
>      +#define LAST_ATTEMPT_STATUS_LIBRARY_MAX_ERROR_CODE_VALUE    LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX - 1
>      +
>      +//
>      +// Last attempt status codes defined for additional granularity in the FMP stack.
>      +//
>      +// These codes are defined within the higher-level UEFI specification defined UNSUCCESSFUL_VENDOR_RANGE.
>      +//
>      +// The following last attempt status code ranges are defined for the following corresponding component:
>      +//   * LAST_ATTEMPT_STATUS_DRIVER - FMP driver
>      +//   * LAST_ATTEMPT_STATUS_DEPENDENCY - FMP dependency functionality
>      +//   * LAST_ATTEMPT_STATUS_LIBRARY - FMP device library instances
>      +//
>      +enum LAST_ATTEMPT_STATUS_EXPANDED_ERROR_LIST
>      +{
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER                 = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_PROGRESS_CALLBACK_ERROR        ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_POWER_API                ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_THERMAL_API          ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_THERMAL                        ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_ENV_API              ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_SYSTEM_ENV                     ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER_SIZE            ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_ALL_HEADER_SIZE            ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER_VERSION         ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_NOT_PROVIDED             ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_NOT_UPDATABLE            ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_CERTIFICATE            ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_IMAGE_INDEX            ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_KEY_LENGTH             ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_KEY_LENGTH_VALUE       ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_VERSION_TOO_LOW                ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_DEVICE_LOCKED                  ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_AUTH_FAILURE             ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_PROTOCOL_ARG_MISSING           ,
>      +  LAST_ATTEMPT_STATUS_DRIVER_ERROR_MAX_ERROR_CODE                 = LAST_ATTEMPT_STATUS_DRIVER_MAX_ERROR_CODE_VALUE,
>      +
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_GET_DEPEX_FAILURE          ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_NO_END_OPCODE              ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_UNKNOWN_OPCODE             ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_MEMORY_ALLOCATION_FAILED   ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_GUID_BEYOND_DEPEX          ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_VERSION_BEYOND_DEPEX       ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_VERSION_STR_BEYOND_DEPEX   ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_FMP_NOT_FOUND              ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE               ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE                ,
>      +  LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_MAX_ERROR_CODE             = LAST_ATTEMPT_STATUS_DEPENDENCY_MAX_ERROR_CODE_VALUE,
>      +
>      +  LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MIN_ERROR_CODE                ,
>      +  LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MAX_ERROR_CODE                = LAST_ATTEMPT_STATUS_LIBRARY_MAX_ERROR_CODE_VALUE
>      +};
>      +
>      +#endif
>      --
>      2.28.0.windows.1
> 
> 
>
> 
>