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

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

I realized there is room for misinterpretation of the macros 
LAST_ATTEMPT_STATUS_DRIVER_ERROR_COUNT and 
LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_COUNT based on name.

If there's no further feedback on the topic, I'll change them to 
LAST_ATTEMPT_STATUS_DRIVER_ERROR_RANGE_LENGTH and 
LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_RANGE_LENGTH.

Thanks,
Michael

On 8/11/2020 10:46 AM, Michael Kubacki wrote:
> #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
>>
>>
>>      
>>
>>