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

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi Michael,

I guess I might not understand the exact use cases for the enum. It seems like the meaning of the error codes you only want to be known within FmpDevicePkg, but it appears to me that the error code values could traverse to drivers outside this package, is that correct?

Thanks,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Tuesday, August 11, 2020 11:58 AM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io
Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Jiang, Guomin <guomin.jiang@intel.com>; Xu, Wei6 <wei6.xu@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h

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
>>
>>
>>      
>>
>>