[edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
Michael Kubacki
michael.kubacki at outlook.com
Fri Aug 28 17:46:08 UTC 2020
Hi Nate,
Bumping this mail in case you missed it.
Thanks,
Michael
On 8/24/2020 5:30 PM, Michael Kubacki wrote:
> Can you provide an example of how you expect the namespace
> identifier/error code token to be used?
>
> Thanks,
> Michael
>
> On 8/24/2020 10:22 AM, Desimone, Nathaniel L wrote:
>> Ah interesting. So you are more concerned about the namespace that the
>> error code comes from as opposed to the actual meaning of the error
>> code itself.
>>
>> That brings another piece of feedback to mind, generally in UEFI
>> namespaces are established using GUIDs... would it be more appropriate
>> to decompose this into a GUID namespace identifier plus an error code
>> integer token? That would eliminate the need for any knowledge of the
>> integer values of the error codes outside of the producing module and
>> seems to follow the "Single Responsibility Principle"
>> (https://en.wikipedia.org/wiki/Single-responsibility_principle) more
>> closely.
>>
>> Thanks,
>> Nate
>>
>> -----Original Message-----
>> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael
>> Kubacki
>> Sent: Friday, August 21, 2020 2:24 PM
>> To: Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>;
>> 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>
>> Subject: Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add
>> LastAttemptStatus.h
>>
>> Hi Nate,
>>
>> Yes, these are individual codes used within FmpDevicePkg. The specific
>> error codes in the enum in v2 are not intended to be used outside
>> FmpDevicePkg. I refactored this in v3.
>>
>> What is desired is a way to consistently map error codes to specific
>> error sources during the update flow. The codes might come from common
>> FmpDevicePkg source code (like FmpDxe) or platform authored source
>> code (like FmpDeviceLib). The exact codes used in FmpDevicePkg
>> implementation do not necessarily need to be known to the library (it
>> doesn't receive those as input) and the library is free to define
>> codes for its own library instance implementation to use as output.
>> For example, there might be cases where the FmpDxe driver and a
>> FmpDeviceLib instance both define a similar error code (e.g. memory
>> allocation failed) but the specific value leads to a particular error
>> condition in either component.
>>
>> At the moment, FmpDevicePkg implementation and FmpDeviceLib instances
>> are the two high-level pieces involved in producing error codes so
>> within the overall 0x1000 - 0x3FFF range available, they're each be
>> assigned an overall range in the public header of length 0x800 (in v4)
>> leaving 0x2000 for future expansion. In V3, this led to the ranges
>> being defined in a public header but the specific error codes in the
>> enum being defined in a private header.
>>
>> Thanks,
>> Michael
>>
>> On 8/20/2020 10:33 PM, Desimone, Nathaniel L wrote:
>>> 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael
>>> Kubacki
>>> Sent: Tuesday, August 11, 2020 11:58 AM
>>> To: Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>;
>>> 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>
>>> 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 at edk2.groups.io on behalf of Michael
>>>>> Kubacki" <devel at edk2.groups.io on behalf of
>>>>> michael.kubacki at outlook.com> wrote:
>>>>>
>>>>> From: Michael Kubacki <michael.kubacki at 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 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>
>>>>> Signed-off-by: Michael Kubacki <michael.kubacki at 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>>
>>
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64764): https://edk2.groups.io/g/devel/message/64764
Mute This Topic: https://groups.io/mt/76113211/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