* [PATCH v2 1/6] MdePkg/SystemResourceTable.h: Add vendor range values
[not found] <20200810202753.1318-1-michael.kubacki@outlook.com>
@ 2020-08-10 20:27 ` Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h Michael Kubacki
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Michael Kubacki @ 2020-08-10 20:27 UTC (permalink / raw)
To: devel; +Cc: Liming Gao, Michael D Kinney, Guomin Jiang, Wei6 Xu, Zhiguang Liu
From: Michael Kubacki <michael.kubacki@microsoft.com>
Adds the following macros to define the unsuccessful vendor range
min and max (defined in UEFI Specification 2.8):
1. LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN
2. LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX
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>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
MdePkg/Include/Guid/SystemResourceTable.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/MdePkg/Include/Guid/SystemResourceTable.h b/MdePkg/Include/Guid/SystemResourceTable.h
index 418b8c8d055a..e242e389df00 100644
--- a/MdePkg/Include/Guid/SystemResourceTable.h
+++ b/MdePkg/Include/Guid/SystemResourceTable.h
@@ -1,6 +1,7 @@
/** @file
Guid & data structure used for EFI System Resource Table (ESRT)
+ Copyright (c) Microsoft Corporation.<BR>
Copyright (c) 2015 - 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -44,6 +45,9 @@
#define LAST_ATTEMPT_STATUS_ERROR_PWR_EVT_BATT 0x00000007
#define LAST_ATTEMPT_STATUS_ERROR_UNSATISFIED_DEPENDENCIES 0x00000008
+#define LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN 0x00001000
+#define LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MAX 0x00004000
+
typedef struct {
///
/// The firmware class field contains a GUID that identifies a firmware component
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
[not found] <20200810202753.1318-1-michael.kubacki@outlook.com>
2020-08-10 20:27 ` [PATCH v2 1/6] MdePkg/SystemResourceTable.h: Add vendor range values Michael Kubacki
@ 2020-08-10 20:27 ` Michael Kubacki
2020-08-11 0:31 ` [edk2-devel] " Nate DeSimone
2020-08-10 20:27 ` [PATCH v2 3/6] FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status capability Michael Kubacki
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Michael Kubacki @ 2020-08-10 20:27 UTC (permalink / raw)
To: devel; +Cc: Liming Gao, Michael D Kinney, Guomin Jiang, Wei6 Xu
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
2020-08-10 20:27 ` [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h Michael Kubacki
@ 2020-08-11 0:31 ` Nate DeSimone
2020-08-11 17:46 ` Michael Kubacki
0 siblings, 1 reply; 14+ messages in thread
From: Nate DeSimone @ 2020-08-11 0:31 UTC (permalink / raw)
To: devel@edk2.groups.io, michael.kubacki@outlook.com
Cc: Gao, Liming, Kinney, Michael D, Jiang, Guomin, Xu, Wei6
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
2020-08-11 0:31 ` [edk2-devel] " Nate DeSimone
@ 2020-08-11 17:46 ` Michael Kubacki
2020-08-11 18:57 ` Michael Kubacki
0 siblings, 1 reply; 14+ messages in thread
From: Michael Kubacki @ 2020-08-11 17:46 UTC (permalink / raw)
To: Desimone, Nathaniel L, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Jiang, Guomin, Xu, Wei6
#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
>
>
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
2020-08-11 17:46 ` Michael Kubacki
@ 2020-08-11 18:57 ` Michael Kubacki
2020-08-21 5:33 ` Nate DeSimone
0 siblings, 1 reply; 14+ messages in thread
From: Michael Kubacki @ 2020-08-11 18:57 UTC (permalink / raw)
To: Desimone, Nathaniel L, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Jiang, Guomin, Xu, Wei6
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
>>
>>
>>
>>
>>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
2020-08-11 18:57 ` Michael Kubacki
@ 2020-08-21 5:33 ` Nate DeSimone
2020-08-21 21:23 ` Michael Kubacki
0 siblings, 1 reply; 14+ messages in thread
From: Nate DeSimone @ 2020-08-21 5:33 UTC (permalink / raw)
To: devel@edk2.groups.io, michael.kubacki@outlook.com
Cc: Gao, Liming, Kinney, Michael D, Jiang, Guomin, Xu, Wei6
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
>>
>>
>>
>>
>>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
2020-08-21 5:33 ` Nate DeSimone
@ 2020-08-21 21:23 ` Michael Kubacki
2020-08-24 17:22 ` Nate DeSimone
0 siblings, 1 reply; 14+ messages in thread
From: Michael Kubacki @ 2020-08-21 21:23 UTC (permalink / raw)
To: Desimone, Nathaniel L, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Jiang, Guomin, Xu, Wei6
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@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
>>>
>>>
>>>
>>>
>>>
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
2020-08-21 21:23 ` Michael Kubacki
@ 2020-08-24 17:22 ` Nate DeSimone
2020-08-25 0:30 ` Michael Kubacki
[not found] ` <957bfe53-d369-2f06-441f-c5c1bb74aeb2@outlook.com>
0 siblings, 2 replies; 14+ messages in thread
From: Nate DeSimone @ 2020-08-24 17:22 UTC (permalink / raw)
To: devel@edk2.groups.io, michael.kubacki@outlook.com
Cc: Gao, Liming, Kinney, Michael D, Jiang, Guomin, Xu, Wei6
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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Friday, August 21, 2020 2:24 PM
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
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@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
>>>
>>>
>>>
>>>
>>>
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
2020-08-24 17:22 ` Nate DeSimone
@ 2020-08-25 0:30 ` Michael Kubacki
[not found] ` <957bfe53-d369-2f06-441f-c5c1bb74aeb2@outlook.com>
1 sibling, 0 replies; 14+ messages in thread
From: Michael Kubacki @ 2020-08-25 0:30 UTC (permalink / raw)
To: Desimone, Nathaniel L, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Jiang, Guomin, Xu, Wei6
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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Friday, August 21, 2020 2:24 PM
> 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
>
> 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@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
>>>>
>>>>
>>>>
>>>>
>>>>
>>
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread[parent not found: <957bfe53-d369-2f06-441f-c5c1bb74aeb2@outlook.com>]
* Re: [edk2-devel] [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h
[not found] ` <957bfe53-d369-2f06-441f-c5c1bb74aeb2@outlook.com>
@ 2020-08-28 17:46 ` Michael Kubacki
0 siblings, 0 replies; 14+ messages in thread
From: Michael Kubacki @ 2020-08-28 17:46 UTC (permalink / raw)
To: Desimone, Nathaniel L, devel@edk2.groups.io
Cc: Gao, Liming, Kinney, Michael D, Jiang, Guomin, Xu, Wei6
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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
>> Kubacki
>> Sent: Friday, August 21, 2020 2:24 PM
>> 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
>>
>> 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@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
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>>
>>
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/6] FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status capability
[not found] <20200810202753.1318-1-michael.kubacki@outlook.com>
2020-08-10 20:27 ` [PATCH v2 1/6] MdePkg/SystemResourceTable.h: Add vendor range values Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 2/6] FmpDevicePkg: Add LastAttemptStatus.h Michael Kubacki
@ 2020-08-10 20:27 ` Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 4/6] FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status granularity Michael Kubacki
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Michael Kubacki @ 2020-08-10 20:27 UTC (permalink / raw)
To: devel; +Cc: Liming Gao, Michael D Kinney, Guomin Jiang, Wei6 Xu
From: Michael Kubacki <michael.kubacki@microsoft.com>
CheckTheImage() is used to provide the CheckImage() implementation
for the EFI_FIRMWARE_MANAGEMENT_PROTOCOL instance produced by
FmpDxe in addition to being called internally in the SetImage()
path.
Since CheckTheImage() plays a major role in determining the
validity of a given firmware image, an internal version of the
function is introduced - CheckTheImageInternal() that is capable
of returning a Last Attempt Status code to internal callers such
as SetTheImage().
The CheckImage() API as defined in the UEFI Specification for
EFI_FIRMWARE_MANAGEMENT_PROTOCOL is not impacted by this change.
CheckTheImageInternal() contains unique Last Attempt Status codes
during error paths in the function so it is easier to identify
the issue with a particular image through the Last Attempt Status
code value.
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/FmpDxe/FmpDxe.c | 102 +++++++++++++++++---
FmpDevicePkg/FmpDxe/FmpDxe.h | 3 +-
2 files changed, 93 insertions(+), 12 deletions(-)
diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
index 854feec0a162..dc714f7e9cd4 100644
--- a/FmpDevicePkg/FmpDxe/FmpDxe.c
+++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
@@ -721,6 +721,24 @@ GetAllHeaderSize (
@param[in] ImageSize Size of the new image in bytes.
@param[out] ImageUpdatable Indicates if the new image is valid for update. It also provides,
if available, additional information if the image is invalid.
+ @param[out] LastAttemptStatus A pointer to a UINT32 that holds the last attempt
+ status to report back to the ESRT table in case
+ of error.
+
+ This function will return error codes that occur within this function
+ implementation within a driver range of last attempt error codes from
+ LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN
+ to LAST_ATTEMPT_STATUS_DRIVER_MAX_ERROR_CODE_VALUE.
+
+ This function will return error codes that occur within FMP dependency
+ related functionality used by this function within a dependency range of
+ last attempt error codes from LAST_ATTEMPT_STATUS_DRIVER_MAX_ERROR_CODE_VALUE
+ to LAST_ATTEMPT_STATUS_DEPENDENCY_MAX_ERROR_CODE_VALUE.
+
+ This function will additionally return error codes that are returned
+ from a device-specific CheckImage () implementation in the range of
+ LAST_ATTEMPT_STATUS_DEPENDENCY_MAX_ERROR_CODE_VALUE to
+ LAST_ATTEMPT_STATUS_LIBRARY_MAX_ERROR_CODE_VALUE.
@retval EFI_SUCCESS The image was successfully checked.
@retval EFI_ABORTED The operation is aborted.
@@ -731,15 +749,17 @@ GetAllHeaderSize (
**/
EFI_STATUS
EFIAPI
-CheckTheImage (
+CheckTheImageInternal (
IN EFI_FIRMWARE_MANAGEMENT_PROTOCOL *This,
IN UINT8 ImageIndex,
IN CONST VOID *Image,
IN UINTN ImageSize,
- OUT UINT32 *ImageUpdatable
+ OUT UINT32 *ImageUpdatable,
+ OUT UINT32 *LastAttemptStatus
)
{
EFI_STATUS Status;
+ UINT32 LocalLastAttemptStatus;
FIRMWARE_MANAGEMENT_PRIVATE_DATA *Private;
UINTN RawSize;
VOID *FmpPayloadHeader;
@@ -755,23 +775,31 @@ CheckTheImage (
EFI_FIRMWARE_IMAGE_DEP *Dependencies;
UINT32 DependenciesSize;
- Status = EFI_SUCCESS;
- RawSize = 0;
- FmpPayloadHeader = NULL;
- FmpPayloadSize = 0;
- Version = 0;
- FmpHeaderSize = 0;
- AllHeaderSize = 0;
- Dependencies = NULL;
- DependenciesSize = 0;
+ Status = EFI_SUCCESS;
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
+ RawSize = 0;
+ FmpPayloadHeader = NULL;
+ FmpPayloadSize = 0;
+ Version = 0;
+ FmpHeaderSize = 0;
+ AllHeaderSize = 0;
+ Dependencies = NULL;
+ DependenciesSize = 0;
if (!FeaturePcdGet (PcdFmpDeviceStorageAccessEnable)) {
return EFI_UNSUPPORTED;
}
+ if (LastAttemptStatus == NULL) {
+ DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImageInternal() - LastAttemptStatus is NULL.\n", mImageIdName));
+ Status = EFI_INVALID_PARAMETER;
+ goto cleanup;
+ }
+
if (This == NULL) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - This is NULL.\n", mImageIdName));
Status = EFI_INVALID_PARAMETER;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_PROTOCOL_ARG_MISSING;
goto cleanup;
}
@@ -789,6 +817,7 @@ CheckTheImage (
if (ImageUpdatable == NULL) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - ImageUpdatable Pointer Parameter is NULL.\n", mImageIdName));
Status = EFI_INVALID_PARAMETER;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_NOT_UPDATABLE;
goto cleanup;
}
@@ -808,6 +837,7 @@ CheckTheImage (
// not sure if this is needed
//
*ImageUpdatable = IMAGE_UPDATABLE_INVALID;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_NOT_PROVIDED;
return EFI_INVALID_PARAMETER;
}
@@ -817,6 +847,7 @@ CheckTheImage (
if (PublicKeyDataXdr == NULL || (PublicKeyDataXdr == PublicKeyDataXdrEnd)) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Invalid certificate, skipping it.\n", mImageIdName));
Status = EFI_ABORTED;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_CERTIFICATE;
} else {
//
// Try each key from PcdFmpDevicePkcs7CertBufferXdr
@@ -839,6 +870,7 @@ CheckTheImage (
//
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Certificate size extends beyond end of PCD, skipping it.\n", mImageIdName));
Status = EFI_ABORTED;
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_KEY_LENGTH_VALUE;
break;
}
//
@@ -855,6 +887,7 @@ CheckTheImage (
//
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): Certificate extends beyond end of PCD, skipping it.\n", mImageIdName));
Status = EFI_ABORTED;
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_KEY_LENGTH;
break;
}
PublicKeyData = PublicKeyDataXdr;
@@ -874,6 +907,11 @@ CheckTheImage (
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImage() - Authentication Failed %r.\n", mImageIdName, Status));
+ if (LocalLastAttemptStatus != LAST_ATTEMPT_STATUS_SUCCESS) {
+ *LastAttemptStatus = LocalLastAttemptStatus;
+ } else {
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_IMAGE_AUTH_FAILURE;
+ }
goto cleanup;
}
@@ -884,6 +922,7 @@ CheckTheImage (
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckImage() - Image Index Invalid.\n", mImageIdName));
*ImageUpdatable = IMAGE_UPDATABLE_INVALID_TYPE;
Status = EFI_INVALID_PARAMETER;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_INVALID_IMAGE_INDEX;
goto cleanup;
}
@@ -899,6 +938,7 @@ CheckTheImage (
if (FmpPayloadHeader == NULL) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImage() - GetFmpHeader failed.\n", mImageIdName));
Status = EFI_ABORTED;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER;
goto cleanup;
}
Status = GetFmpPayloadHeaderVersion (FmpPayloadHeader, FmpPayloadSize, &Version);
@@ -906,6 +946,7 @@ CheckTheImage (
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImage() - GetFmpPayloadHeaderVersion failed %r.\n", mImageIdName, Status));
*ImageUpdatable = IMAGE_UPDATABLE_INVALID;
Status = EFI_SUCCESS;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER_VERSION;
goto cleanup;
}
@@ -920,6 +961,7 @@ CheckTheImage (
);
*ImageUpdatable = IMAGE_UPDATABLE_INVALID_OLD;
Status = EFI_SUCCESS;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_VERSION_TOO_LOW;
goto cleanup;
}
@@ -942,6 +984,7 @@ CheckTheImage (
DEBUG ((DEBUG_ERROR, "FmpDxe: CheckTheImage() - GetFmpPayloadHeaderSize failed %r.\n", Status));
*ImageUpdatable = IMAGE_UPDATABLE_INVALID;
Status = EFI_SUCCESS;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER_SIZE;
goto cleanup;
}
@@ -953,6 +996,7 @@ CheckTheImage (
if (AllHeaderSize == 0) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImage() - GetAllHeaderSize failed.\n", mImageIdName));
Status = EFI_ABORTED;
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_ALL_HEADER_SIZE;
goto cleanup;
}
RawSize = ImageSize - AllHeaderSize;
@@ -969,6 +1013,42 @@ CheckTheImage (
return Status;
}
+/**
+ Checks if the firmware image is valid for the device.
+
+ This function allows firmware update application to validate the firmware image without
+ invoking the SetImage() first.
+
+ @param[in] This A pointer to the EFI_FIRMWARE_MANAGEMENT_PROTOCOL instance.
+ @param[in] ImageIndex A unique number identifying the firmware image(s) within the device.
+ The number is between 1 and DescriptorCount.
+ @param[in] Image Points to the new image.
+ @param[in] ImageSize Size of the new image in bytes.
+ @param[out] ImageUpdatable Indicates if the new image is valid for update. It also provides,
+ if available, additional information if the image is invalid.
+
+ @retval EFI_SUCCESS The image was successfully checked.
+ @retval EFI_ABORTED The operation is aborted.
+ @retval EFI_INVALID_PARAMETER The Image was NULL.
+ @retval EFI_UNSUPPORTED The operation is not supported.
+ @retval EFI_SECURITY_VIOLATION The operation could not be performed due to an authentication failure.
+
+**/
+EFI_STATUS
+EFIAPI
+CheckTheImage (
+ IN EFI_FIRMWARE_MANAGEMENT_PROTOCOL *This,
+ IN UINT8 ImageIndex,
+ IN CONST VOID *Image,
+ IN UINTN ImageSize,
+ OUT UINT32 *ImageUpdatable
+ )
+{
+ UINT32 LastAttemptStatus;
+
+ return CheckTheImageInternal (This, ImageIndex, Image, ImageSize, ImageUpdatable, &LastAttemptStatus);
+}
+
/**
Updates the firmware image of the device.
diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.h b/FmpDevicePkg/FmpDxe/FmpDxe.h
index 30754dea495e..91e9e51f1484 100644
--- a/FmpDevicePkg/FmpDxe/FmpDxe.h
+++ b/FmpDevicePkg/FmpDxe/FmpDxe.h
@@ -3,7 +3,7 @@
image stored in a firmware device with platform and firmware device specific
information provided through PCDs and libraries.
- Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -36,6 +36,7 @@
#include <Protocol/VariableLock.h>
#include <Guid/SystemResourceTable.h>
#include <Guid/EventGroup.h>
+#include <LastAttemptStatus.h>
#define VERSION_STRING_NOT_SUPPORTED L"VERSION STRING NOT SUPPORTED"
#define VERSION_STRING_NOT_AVAILABLE L"VERSION STRING NOT AVAILABLE"
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 4/6] FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status granularity
[not found] <20200810202753.1318-1-michael.kubacki@outlook.com>
` (2 preceding siblings ...)
2020-08-10 20:27 ` [PATCH v2 3/6] FmpDevicePkg/FmpDxe: Add check image path Last Attempt Status capability Michael Kubacki
@ 2020-08-10 20:27 ` Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 5/6] FmpDevicePkg: Add Last Attempt Status support to dependency libs Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 6/6] FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API Michael Kubacki
5 siblings, 0 replies; 14+ messages in thread
From: Michael Kubacki @ 2020-08-10 20:27 UTC (permalink / raw)
To: devel; +Cc: Liming Gao, Michael D Kinney, Guomin Jiang, Wei6 Xu
From: Michael Kubacki <michael.kubacki@microsoft.com>
Increases the level of granularity for Last Attempt Status codes
returned from SetTheImage() in FmpDxe. This allows better
identification of the error that occurred in the set image
operation using Last Attempt Status codes.
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/FmpDxe/FmpDxe.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
index dc714f7e9cd4..aeb888c86bc3 100644
--- a/FmpDevicePkg/FmpDxe/FmpDxe.c
+++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
@@ -1141,6 +1141,7 @@ SetTheImage (
if (This == NULL) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - This is NULL.\n", mImageIdName));
Status = EFI_INVALID_PARAMETER;
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_PROTOCOL_ARG_MISSING;
goto cleanup;
}
@@ -1166,6 +1167,7 @@ SetTheImage (
//
if (Private->FmpDeviceLocked) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - Device is already locked. Can't update.\n", mImageIdName));
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_DEVICE_LOCKED;
Status = EFI_UNSUPPORTED;
goto cleanup;
}
@@ -1173,12 +1175,9 @@ SetTheImage (
//
// Call check image to verify the image
//
- Status = CheckTheImage (This, ImageIndex, Image, ImageSize, &Updateable);
+ Status = CheckTheImageInternal (This, ImageIndex, Image, ImageSize, &Updateable, &LastAttemptStatus);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - Check The Image failed with %r.\n", mImageIdName, Status));
- if (Status == EFI_SECURITY_VIOLATION) {
- LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_AUTH_ERROR;
- }
goto cleanup;
}
@@ -1194,6 +1193,7 @@ SetTheImage (
FmpHeader = GetFmpHeader ( (EFI_FIRMWARE_IMAGE_AUTHENTICATION *)Image, ImageSize, DependenciesSize, &FmpPayloadSize );
if (FmpHeader == NULL) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - GetFmpHeader failed.\n", mImageIdName));
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER;
Status = EFI_ABORTED;
goto cleanup;
}
@@ -1221,6 +1221,7 @@ SetTheImage (
if (Progress == NULL) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - Invalid progress callback\n", mImageIdName));
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_PROGRESS_CALLBACK_ERROR;
Status = EFI_INVALID_PARAMETER;
goto cleanup;
}
@@ -1241,6 +1242,7 @@ SetTheImage (
Status = CheckSystemPower (&BooleanValue);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - CheckSystemPower - API call failed %r.\n", mImageIdName, Status));
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_POWER_API;
goto cleanup;
}
if (!BooleanValue) {
@@ -1261,10 +1263,12 @@ SetTheImage (
Status = CheckSystemThermal (&BooleanValue);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - CheckSystemThermal - API call failed %r.\n", mImageIdName, Status));
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_THERMAL_API;
goto cleanup;
}
if (!BooleanValue) {
Status = EFI_ABORTED;
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_THERMAL;
DEBUG (
(DEBUG_ERROR,
"FmpDxe(%s): SetTheImage() - CheckSystemThermal - returned False. Update not allowed due to System Thermal.\n", mImageIdName)
@@ -1280,10 +1284,12 @@ SetTheImage (
Status = CheckSystemEnvironment (&BooleanValue);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - CheckSystemEnvironment - API call failed %r.\n", mImageIdName, Status));
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_CHECK_SYS_ENV_API;
goto cleanup;
}
if (!BooleanValue) {
Status = EFI_ABORTED;
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_SYSTEM_ENV;
DEBUG (
(DEBUG_ERROR,
"FmpDxe(%s): SetTheImage() - CheckSystemEnvironment - returned False. Update not allowed due to System Environment.\n", mImageIdName)
@@ -1305,12 +1311,14 @@ SetTheImage (
Status = GetFmpPayloadHeaderSize (FmpHeader, FmpPayloadSize, &FmpHeaderSize);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - GetFmpPayloadHeaderSize failed %r.\n", mImageIdName, Status));
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_FMP_HEADER_SIZE;
goto cleanup;
}
AllHeaderSize = GetAllHeaderSize ((EFI_FIRMWARE_IMAGE_AUTHENTICATION *)Image, FmpHeaderSize + DependenciesSize);
if (AllHeaderSize == 0) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - GetAllHeaderSize failed.\n", mImageIdName));
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_DRIVER_ERROR_GET_ALL_HEADER_SIZE;
Status = EFI_ABORTED;
goto cleanup;
}
@@ -1373,6 +1381,7 @@ SetTheImage (
cleanup:
mProgressFunc = NULL;
+ DEBUG ((DEBUG_INFO, "FmpDxe(%s): SetTheImage() LastAttemptStatus: %u.\n", mImageIdName, LastAttemptStatus));
SetLastAttemptStatusInVariable (Private, LastAttemptStatus);
if (Progress != NULL) {
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 5/6] FmpDevicePkg: Add Last Attempt Status support to dependency libs
[not found] <20200810202753.1318-1-michael.kubacki@outlook.com>
` (3 preceding siblings ...)
2020-08-10 20:27 ` [PATCH v2 4/6] FmpDevicePkg/FmpDxe: Improve set image path Last Attempt Status granularity Michael Kubacki
@ 2020-08-10 20:27 ` Michael Kubacki
2020-08-10 20:27 ` [PATCH v2 6/6] FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API Michael Kubacki
5 siblings, 0 replies; 14+ messages in thread
From: Michael Kubacki @ 2020-08-10 20:27 UTC (permalink / raw)
To: devel; +Cc: Liming Gao, Michael D Kinney, Guomin Jiang, Wei6 Xu
From: Michael Kubacki <michael.kubacki@microsoft.com>
The FMP dependency libraries are leveraged during firmware update
to check for dependencies required to update the image.
This change adds granular Last Attempt Status code support to these
services so failures can be more easily observed during the firmware
update process via Last Attempt Status codes.
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/FmpDxe/FmpDxe.c | 25 +++++-
FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c | 38 +++++---
FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c | 9 +-
FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c | 95 +++++++++++++++++---
FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c | 7 +-
FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h | 8 +-
FmpDevicePkg/Include/Library/FmpDependencyLib.h | 44 +++++----
7 files changed, 179 insertions(+), 47 deletions(-)
diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
index aeb888c86bc3..84a8e15cfc17 100644
--- a/FmpDevicePkg/FmpDxe/FmpDxe.c
+++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
@@ -929,7 +929,17 @@ CheckTheImageInternal (
//
// Get the dependency from Image.
//
- Dependencies = GetImageDependency ((EFI_FIRMWARE_IMAGE_AUTHENTICATION *)Image, ImageSize, &DependenciesSize);
+ Dependencies = GetImageDependency (
+ (EFI_FIRMWARE_IMAGE_AUTHENTICATION *) Image,
+ ImageSize,
+ &DependenciesSize,
+ &LocalLastAttemptStatus
+ );
+ if (LocalLastAttemptStatus != LAST_ATTEMPT_STATUS_SUCCESS) {
+ Status = EFI_ABORTED;
+ *LastAttemptStatus = LocalLastAttemptStatus;
+ goto cleanup;
+ }
//
// Check the FmpPayloadHeader
@@ -968,11 +978,20 @@ CheckTheImageInternal (
//
// Evaluate dependency expression
//
- Private->DependenciesSatisfied = CheckFmpDependency (Private->Descriptor.ImageTypeId, Version, Dependencies, DependenciesSize);
+ Private->DependenciesSatisfied = CheckFmpDependency (
+ Private->Descriptor.ImageTypeId,
+ Version,
+ Dependencies,
+ DependenciesSize,
+ &LocalLastAttemptStatus
+ );
if (!Private->DependenciesSatisfied) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImage() - Dependency check failed.\n", mImageIdName));
*ImageUpdatable = IMAGE_UPDATABLE_INVALID;
Status = EFI_SUCCESS;
+ if (LocalLastAttemptStatus != LAST_ATTEMPT_STATUS_SUCCESS) {
+ *LastAttemptStatus = LocalLastAttemptStatus;
+ }
goto cleanup;
}
@@ -1184,7 +1203,7 @@ SetTheImage (
//
// Get the dependency from Image.
//
- Dependencies = GetImageDependency ((EFI_FIRMWARE_IMAGE_AUTHENTICATION *)Image, ImageSize, &DependenciesSize);
+ Dependencies = GetImageDependency ((EFI_FIRMWARE_IMAGE_AUTHENTICATION *)Image, ImageSize, &DependenciesSize, &LastAttemptStatus);
//
// No functional error in CheckTheImage. Attempt to get the Version to
diff --git a/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c b/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
index 02ed600e0e95..d1e3b6b93bc2 100644
--- a/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
+++ b/FmpDevicePkg/Library/FmpDependencyCheckLib/FmpDependencyCheckLib.c
@@ -17,6 +17,8 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/UefiLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Guid/SystemResourceTable.h>
+#include <LastAttemptStatus.h>
/**
Check dependency for firmware update.
@@ -25,6 +27,10 @@
@param[in] Version New version.
@param[in] Dependencies Fmp dependency.
@param[in] DependenciesSize Size, in bytes, of the Fmp dependency.
+ @param[out] LastAttemptStatus An optional pointer to a UINT32 that holds the
+ last attempt status to report back to the caller.
+ A function error code may not always be accompanied
+ by a last attempt status code.
@retval TRUE Dependencies are satisfied.
@retval FALSE Dependencies are unsatisfied or dependency check fails.
@@ -36,7 +42,8 @@ CheckFmpDependency (
IN EFI_GUID ImageTypeId,
IN UINT32 Version,
IN EFI_FIRMWARE_IMAGE_DEP *Dependencies, OPTIONAL
- IN UINT32 DependenciesSize
+ IN UINT32 DependenciesSize,
+ OUT UINT32 *LastAttemptStatus OPTIONAL
)
{
EFI_STATUS Status;
@@ -44,6 +51,7 @@ CheckFmpDependency (
UINTN Index;
EFI_FIRMWARE_MANAGEMENT_PROTOCOL *Fmp;
UINTN ImageInfoSize;
+ UINT32 LocalLastAttemptStatus;
UINT32 *DescriptorVer;
UINT8 FmpImageInfoCount;
UINTN *DescriptorSize;
@@ -55,14 +63,15 @@ CheckFmpDependency (
UINTN FmpVersionsCount;
BOOLEAN IsSatisfied;
- FmpImageInfoBuf = NULL;
- DescriptorVer = NULL;
- DescriptorSize = NULL;
- NumberOfFmpInstance = 0;
- FmpVersions = NULL;
- FmpVersionsCount = 0;
- IsSatisfied = TRUE;
- PackageVersionName = NULL;
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
+ FmpImageInfoBuf = NULL;
+ DescriptorVer = NULL;
+ DescriptorSize = NULL;
+ NumberOfFmpInstance = 0;
+ FmpVersions = NULL;
+ FmpVersionsCount = 0;
+ IsSatisfied = TRUE;
+ PackageVersionName = NULL;
//
// Get ImageDescriptors of all FMP instances, and archive them for dependency evaluation.
@@ -77,30 +86,35 @@ CheckFmpDependency (
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "CheckFmpDependency: Get Firmware Management Protocol failed. (%r)", Status));
IsSatisfied = FALSE;
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_MEMORY_ALLOCATION_FAILED;
goto cleanup;
}
FmpImageInfoBuf = AllocateZeroPool (sizeof(EFI_FIRMWARE_IMAGE_DESCRIPTOR *) * NumberOfFmpInstance);
if (FmpImageInfoBuf == NULL) {
IsSatisfied = FALSE;
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_MEMORY_ALLOCATION_FAILED;
goto cleanup;
}
DescriptorVer = AllocateZeroPool (sizeof(UINT32) * NumberOfFmpInstance);
if (DescriptorVer == NULL ) {
IsSatisfied = FALSE;
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_MEMORY_ALLOCATION_FAILED;
goto cleanup;
}
DescriptorSize = AllocateZeroPool (sizeof(UINTN) * NumberOfFmpInstance);
if (DescriptorSize == NULL ) {
IsSatisfied = FALSE;
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_MEMORY_ALLOCATION_FAILED;
goto cleanup;
}
FmpVersions = AllocateZeroPool (sizeof(FMP_DEPEX_CHECK_VERSION_DATA) * NumberOfFmpInstance);
if (FmpVersions == NULL) {
IsSatisfied = FALSE;
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_MEMORY_ALLOCATION_FAILED;
goto cleanup;
}
@@ -164,7 +178,7 @@ CheckFmpDependency (
// Evaluate firmware image's depex, against the version of other Fmp instances.
//
if (Dependencies != NULL) {
- IsSatisfied = EvaluateDependency (Dependencies, DependenciesSize, FmpVersions, FmpVersionsCount);
+ IsSatisfied = EvaluateDependency (Dependencies, DependenciesSize, FmpVersions, FmpVersionsCount, &LocalLastAttemptStatus);
}
if (!IsSatisfied) {
@@ -194,5 +208,9 @@ CheckFmpDependency (
FreePool (FmpVersions);
}
+ if (LastAttemptStatus != NULL && LocalLastAttemptStatus != LAST_ATTEMPT_STATUS_SUCCESS) {
+ *LastAttemptStatus = LocalLastAttemptStatus;
+ }
+
return IsSatisfied;
}
diff --git a/FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c b/FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c
index 55e9af22909d..8937fb56d919 100644
--- a/FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c
+++ b/FmpDevicePkg/Library/FmpDependencyCheckLibNull/FmpDependencyCheckLibNull.c
@@ -1,6 +1,7 @@
/** @file
Null instance of FmpDependencyCheckLib.
+ Copyright (c) Microsoft Corporation.<BR>
Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -16,7 +17,10 @@
@param[in] Version New version.
@param[in] Dependencies Fmp dependency.
@param[in] DependenciesSize Size, in bytes, of the Fmp dependency.
-
+ @param[out] LastAttemptStatus An optional pointer to a UINT32 that holds the
+ last attempt status to report back to the caller.
+ A function error code may not always be accompanied
+ by a last attempt status code.
@retval TRUE Dependencies are satisfied.
@retval FALSE Dependencies are unsatisfied or dependency check fails.
@@ -27,7 +31,8 @@ CheckFmpDependency (
IN EFI_GUID ImageTypeId,
IN UINT32 Version,
IN EFI_FIRMWARE_IMAGE_DEP *Dependencies, OPTIONAL
- IN UINT32 DependenciesSize
+ IN UINT32 DependenciesSize,
+ OUT UINT32 *LastAttemptStatus OPTIONAL
)
{
return TRUE;
diff --git a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
index 5ef25d2415cf..2045570b0628 100644
--- a/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
+++ b/FmpDevicePkg/Library/FmpDependencyLib/FmpDependencyLib.c
@@ -13,6 +13,8 @@
#include <Library/DebugLib.h>
#include <Library/FmpDependencyLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Guid/SystemResourceTable.h>
+#include <LastAttemptStatus.h>
//
// Define the initial size of the dependency expression evaluation stack
@@ -203,6 +205,10 @@ Pop (
parameter is optional and can be set to NULL.
@param[in] FmpVersionsCount Element count of the array. When FmpVersions
is NULL, FmpVersionsCount must be 0.
+ @param[out] LastAttemptStatus An optional pointer to a UINT32 that holds the
+ last attempt status to report back to the caller.
+ A function error code may not always be accompanied
+ by a last attempt status code.
@retval TRUE Dependency expressions evaluate to TRUE.
@retval FALSE Dependency expressions evaluate to FALSE.
@@ -211,10 +217,11 @@ Pop (
BOOLEAN
EFIAPI
EvaluateDependency (
- IN EFI_FIRMWARE_IMAGE_DEP *Dependencies,
- IN UINTN DependenciesSize,
- IN FMP_DEPEX_CHECK_VERSION_DATA *FmpVersions OPTIONAL,
- IN UINTN FmpVersionsCount
+ IN EFI_FIRMWARE_IMAGE_DEP *Dependencies,
+ IN UINTN DependenciesSize,
+ IN FMP_DEPEX_CHECK_VERSION_DATA *FmpVersions, OPTIONAL
+ IN UINTN FmpVersionsCount,
+ OUT UINT32 *LastAttemptStatus OPTIONAL
)
{
EFI_STATUS Status;
@@ -224,6 +231,9 @@ EvaluateDependency (
DEPEX_ELEMENT Element2;
GUID ImageTypeId;
UINT32 Version;
+ UINT32 LocalLastAttemptStatus;
+
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
//
// Check if parameter is valid.
@@ -249,6 +259,7 @@ EvaluateDependency (
case EFI_FMP_DEP_PUSH_GUID:
if (Iterator + sizeof (EFI_GUID) >= (UINT8 *) Dependencies->Dependencies + DependenciesSize) {
DEBUG ((DEBUG_ERROR, "EvaluateDependency: GUID extends beyond end of dependency expression!\n"));
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_GUID_BEYOND_DEPEX;
goto Error;
}
@@ -259,6 +270,7 @@ EvaluateDependency (
if(CompareGuid (&FmpVersions[Index].ImageTypeId, &ImageTypeId)){
Status = Push (FmpVersions[Index].Version, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
@@ -266,18 +278,21 @@ EvaluateDependency (
}
if (Index == FmpVersionsCount) {
DEBUG ((DEBUG_ERROR, "EvaluateDependency: %g is not found!\n", &ImageTypeId));
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_FMP_NOT_FOUND;
goto Error;
}
break;
case EFI_FMP_DEP_PUSH_VERSION:
if (Iterator + sizeof (UINT32) >= (UINT8 *) Dependencies->Dependencies + DependenciesSize ) {
DEBUG ((DEBUG_ERROR, "EvaluateDependency: VERSION extends beyond end of dependency expression!\n"));
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_VERSION_BEYOND_DEPEX;
goto Error;
}
Version = *(UINT32 *) (Iterator + 1);
Status = Push (Version, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
Iterator = Iterator + sizeof (UINT32);
@@ -286,154 +301,191 @@ EvaluateDependency (
Iterator += AsciiStrnLenS ((CHAR8 *) Iterator, DependenciesSize - (Iterator - Dependencies->Dependencies));
if (Iterator == (UINT8 *) Dependencies->Dependencies + DependenciesSize) {
DEBUG ((DEBUG_ERROR, "EvaluateDependency: STRING extends beyond end of dependency expression!\n"));
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_VERSION_STR_BEYOND_DEPEX;
goto Error;
}
break;
case EFI_FMP_DEP_AND:
Status = Pop (&Element1, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Pop (&Element2, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Push (Element1.Value.Boolean & Element2.Value.Boolean, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_OR:
Status = Pop (&Element1, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Pop(&Element2, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Push (Element1.Value.Boolean | Element2.Value.Boolean, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_NOT:
Status = Pop (&Element1, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Push (!(Element1.Value.Boolean), BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_TRUE:
Status = Push (TRUE, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_FALSE:
Status = Push (FALSE, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_EQ:
Status = Pop (&Element1, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Pop (&Element2, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = (Element1.Value.Version == Element2.Value.Version) ? Push (TRUE, BooleanType) : Push (FALSE, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_GT:
Status = Pop (&Element1, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Pop (&Element2, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = (Element1.Value.Version > Element2.Value.Version) ? Push (TRUE, BooleanType) : Push (FALSE, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_GTE:
Status = Pop (&Element1, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Pop (&Element2, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = (Element1.Value.Version >= Element2.Value.Version) ? Push (TRUE, BooleanType) : Push (FALSE, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_LT:
Status = Pop (&Element1, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Pop (&Element2, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus= LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = (Element1.Value.Version < Element2.Value.Version) ? Push (TRUE, BooleanType) : Push (FALSE, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_LTE:
Status = Pop (&Element1, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = Pop (&Element2, VersionType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
Status = (Element1.Value.Version <= Element2.Value.Version) ? Push (TRUE, BooleanType) : Push (FALSE, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_PUSH_FAILURE;
goto Error;
}
break;
case EFI_FMP_DEP_END:
Status = Pop (&Element1, BooleanType);
if (EFI_ERROR (Status)) {
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_POP_FAILURE;
goto Error;
}
return Element1.Value.Boolean;
default:
DEBUG ((DEBUG_ERROR, "EvaluateDependency: Unknown Opcode - %02x!\n", *Iterator));
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_UNKNOWN_OPCODE;
goto Error;
}
Iterator++;
}
DEBUG ((DEBUG_ERROR, "EvaluateDependency: No EFI_FMP_DEP_END Opcode in expression!\n"));
+ LocalLastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_NO_END_OPCODE;
Error:
+ if (LastAttemptStatus != NULL && LocalLastAttemptStatus != LAST_ATTEMPT_STATUS_SUCCESS) {
+ *LastAttemptStatus = LocalLastAttemptStatus;
+ }
+
return FALSE;
}
/**
Validate the dependency expression and output its size.
- @param[in] Dependencies Pointer to the EFI_FIRMWARE_IMAGE_DEP.
- @param[in] MaxDepexSize Max size of the dependency.
- @param[out] DepexSize Size of dependency.
+ @param[in] Dependencies Pointer to the EFI_FIRMWARE_IMAGE_DEP.
+ @param[in] MaxDepexSize Max size of the dependency.
+ @param[out] DepexSize Size of dependency.
+ @param[out] LastAttemptStatus An optional pointer to a UINT32 that holds the
+ last attempt status to report back to the caller.
+ A function error code may not always be accompanied
+ by a last attempt status code.
@retval TRUE The dependency expression is valid.
@retval FALSE The dependency expression is invalid.
@@ -444,7 +496,8 @@ EFIAPI
ValidateDependency (
IN EFI_FIRMWARE_IMAGE_DEP *Dependencies,
IN UINTN MaxDepexSize,
- OUT UINT32 *DepexSize
+ OUT UINT32 *DepexSize,
+ OUT UINT32 *LastAttemptStatus OPTIONAL
)
{
UINT8 *Depex;
@@ -489,20 +542,30 @@ ValidateDependency (
}
return TRUE;
default:
+ if (LastAttemptStatus != NULL) {
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_UNKNOWN_OPCODE;
+ }
return FALSE;
}
}
+ if (LastAttemptStatus != NULL) {
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_NO_END_OPCODE;
+ }
+
return FALSE;
}
/**
Get dependency from firmware image.
- @param[in] Image Points to the firmware image.
- @param[in] ImageSize Size, in bytes, of the firmware image.
- @param[out] DepexSize Size, in bytes, of the dependency.
-
+ @param[in] Image Points to the firmware image.
+ @param[in] ImageSize Size, in bytes, of the firmware image.
+ @param[out] DepexSize Size, in bytes, of the dependency.
+ @param[out] LastAttemptStatus An optional pointer to a UINT32 that holds the
+ last attempt status to report back to the caller.
+ A function error code may not always be accompanied
+ by a last attempt status code.
@retval The pointer to dependency.
@retval Null
@@ -512,7 +575,8 @@ EFIAPI
GetImageDependency (
IN EFI_FIRMWARE_IMAGE_AUTHENTICATION *Image,
IN UINTN ImageSize,
- OUT UINT32 *DepexSize
+ OUT UINT32 *DepexSize,
+ OUT UINT32 *LastAttemptStatus OPTIONAL
)
{
EFI_FIRMWARE_IMAGE_DEP *Depex;
@@ -530,6 +594,9 @@ GetImageDependency (
//
// Pointer overflow. Invalid image.
//
+ if (LastAttemptStatus != NULL) {
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_DEPENDENCY_ERROR_GET_DEPEX_FAILURE;
+ }
return NULL;
}
@@ -539,7 +606,7 @@ GetImageDependency (
//
// Validate the dependency and get the size of dependency
//
- if (ValidateDependency (Depex, MaxDepexSize, DepexSize)) {
+ if (ValidateDependency (Depex, MaxDepexSize, DepexSize, LastAttemptStatus)) {
return Depex;
}
diff --git a/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c b/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
index f8ccdd906f29..92a1ac815d51 100644
--- a/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
+++ b/FmpDevicePkg/Test/UnitTest/Library/FmpDependencyLib/EvaluateDependencyUnitTest.c
@@ -1,6 +1,7 @@
/** @file
Unit tests of EvaluateDependency API in FmpDependencyLib.
+ Copyright (c) Microsoft Corporation.<BR>
Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -162,6 +163,7 @@ EvaluateDependencyTest (
{
BASIC_TEST_CONTEXT *TestContext;
BOOLEAN EvaluationResult;
+ UINT32 LastAttemptStatus;
TestContext = (BASIC_TEST_CONTEXT *)Context;
@@ -169,8 +171,9 @@ EvaluateDependencyTest (
(EFI_FIRMWARE_IMAGE_DEP *)TestContext->Dependencies,
TestContext->DependenciesSize,
mFmpVersions,
- sizeof(mFmpVersions)/sizeof(FMP_DEPEX_CHECK_VERSION_DATA)
- );
+ sizeof(mFmpVersions)/sizeof(FMP_DEPEX_CHECK_VERSION_DATA),
+ &LastAttemptStatus
+ );
UT_ASSERT_EQUAL (EvaluationResult, TestContext->ExpectedResult);
diff --git a/FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h b/FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h
index ec380c4947bd..708cbb06ba74 100644
--- a/FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h
+++ b/FmpDevicePkg/Include/Library/FmpDependencyCheckLib.h
@@ -2,6 +2,7 @@
Fmp Capsule Dependency check functions for Firmware Management Protocol based
firmware updates.
+ Copyright (c) Microsoft Corporation.<BR>
Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -21,6 +22,10 @@
@param[in] Version New version.
@param[in] Dependencies Fmp dependency.
@param[in] DependenciesSize Size, in bytes, of the Fmp dependency.
+ @param[out] LastAttemptStatus An optional pointer to a UINT32 that holds the
+ last attempt status to report back to the caller.
+ A function error code may not always be accompanied
+ by a last attempt status code.
@retval TRUE Dependencies are satisfied.
@retval FALSE Dependencies are unsatisfied or dependency check fails.
@@ -32,7 +37,8 @@ CheckFmpDependency (
IN EFI_GUID ImageTypeId,
IN UINT32 Version,
IN EFI_FIRMWARE_IMAGE_DEP *Dependencies, OPTIONAL
- IN UINT32 DependenciesSize
+ IN UINT32 DependenciesSize,
+ OUT UINT32 *LastAttemptStatus OPTIONAL
);
#endif
diff --git a/FmpDevicePkg/Include/Library/FmpDependencyLib.h b/FmpDevicePkg/Include/Library/FmpDependencyLib.h
index c732903425b4..b8e9c4341165 100644
--- a/FmpDevicePkg/Include/Library/FmpDependencyLib.h
+++ b/FmpDevicePkg/Include/Library/FmpDependencyLib.h
@@ -26,9 +26,13 @@ typedef struct {
/**
Validate the dependency expression and output its size.
- @param[in] Dependencies Pointer to the EFI_FIRMWARE_IMAGE_DEP.
- @param[in] MaxDepexSize Max size of the dependency.
- @param[out] DepexSize Size of dependency.
+ @param[in] Dependencies Pointer to the EFI_FIRMWARE_IMAGE_DEP.
+ @param[in] MaxDepexSize Max size of the dependency.
+ @param[out] DepexSize Size of dependency.
+ @param[out] LastAttemptStatus An optional pointer to a UINT32 that holds the
+ last attempt status to report back to the caller.
+ A function error code may not always be accompanied
+ by a last attempt status code.
@retval TRUE The dependency expression is valid.
@retval FALSE The dependency expression is invalid.
@@ -39,16 +43,20 @@ EFIAPI
ValidateDependency (
IN EFI_FIRMWARE_IMAGE_DEP *Dependencies,
IN UINTN MaxDepexSize,
- OUT UINT32 *DepexSize
+ OUT UINT32 *DepexSize,
+ OUT UINT32 *LastAttemptStatus OPTIONAL
);
/**
Get dependency from firmware image.
- @param[in] Image Points to the firmware image.
- @param[in] ImageSize Size, in bytes, of the firmware image.
- @param[out] DepexSize Size, in bytes, of the dependency.
-
+ @param[in] Image Points to the firmware image.
+ @param[in] ImageSize Size, in bytes, of the firmware image.
+ @param[out] DepexSize Size, in bytes, of the dependency.
+ @param[out] LastAttemptStatus An optional pointer to a UINT32 that holds the
+ last attempt status to report back to the caller.
+ A function error code may not always be accompanied
+ by a last attempt status code.
@retval The pointer to dependency.
@retval Null
@@ -56,9 +64,10 @@ ValidateDependency (
EFI_FIRMWARE_IMAGE_DEP*
EFIAPI
GetImageDependency (
- IN EFI_FIRMWARE_IMAGE_AUTHENTICATION *Image,
- IN UINTN ImageSize,
- OUT UINT32 *DepexSize
+ IN EFI_FIRMWARE_IMAGE_AUTHENTICATION *Image,
+ IN UINTN ImageSize,
+ OUT UINT32 *DepexSize,
+ OUT UINT32 *LastAttemptStatus OPTIONAL
);
/**
@@ -73,6 +82,10 @@ GetImageDependency (
parameter is optional and can be set to NULL.
@param[in] FmpVersionsCount Element count of the array. When FmpVersions
is NULL, FmpVersionsCount must be 0.
+ @param[out] LastAttemptStatus An optional pointer to a UINT32 that holds the
+ last attempt status to report back to the caller.
+ A function error code may not always be accompanied
+ by a last attempt status code.
@retval TRUE Dependency expressions evaluate to TRUE.
@retval FALSE Dependency expressions evaluate to FALSE.
@@ -81,10 +94,11 @@ GetImageDependency (
BOOLEAN
EFIAPI
EvaluateDependency (
- IN EFI_FIRMWARE_IMAGE_DEP *Dependencies,
- IN UINTN DependenciesSize,
- IN FMP_DEPEX_CHECK_VERSION_DATA *FmpVersions OPTIONAL,
- IN UINTN FmpVersionsCount
+ IN EFI_FIRMWARE_IMAGE_DEP *Dependencies,
+ IN UINTN DependenciesSize,
+ IN FMP_DEPEX_CHECK_VERSION_DATA *FmpVersions, OPTIONAL
+ IN UINTN FmpVersionsCount,
+ OUT UINT32 *LastAttemptStatus OPTIONAL
);
#endif
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 6/6] FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API
[not found] <20200810202753.1318-1-michael.kubacki@outlook.com>
` (4 preceding siblings ...)
2020-08-10 20:27 ` [PATCH v2 5/6] FmpDevicePkg: Add Last Attempt Status support to dependency libs Michael Kubacki
@ 2020-08-10 20:27 ` Michael Kubacki
5 siblings, 0 replies; 14+ messages in thread
From: Michael Kubacki @ 2020-08-10 20:27 UTC (permalink / raw)
To: devel; +Cc: Liming Gao, Michael D Kinney, Guomin Jiang, Wei6 Xu
From: Michael Kubacki <michael.kubacki@microsoft.com>
Provides the ability for a given FMP device library instance to
return a Last Attempt Status code during FmpDeviceCheckImage()
and FmpDeviceSetImage().
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/FmpDxe/FmpDxe.c | 36 +++++++++++++++-
FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c | 42 ++++++++++++-------
FmpDevicePkg/Include/Library/FmpDeviceLib.h | 44 +++++++++++++-------
3 files changed, 92 insertions(+), 30 deletions(-)
diff --git a/FmpDevicePkg/FmpDxe/FmpDxe.c b/FmpDevicePkg/FmpDxe/FmpDxe.c
index 84a8e15cfc17..9f55e6ba5f5d 100644
--- a/FmpDevicePkg/FmpDxe/FmpDxe.c
+++ b/FmpDevicePkg/FmpDxe/FmpDxe.c
@@ -1020,14 +1020,32 @@ CheckTheImageInternal (
}
RawSize = ImageSize - AllHeaderSize;
+ //
+ // Set LastAttemptStatus to successful prior to getting any potential error codes from FmpDeviceLib
+ //
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_SUCCESS;
+
//
// FmpDeviceLib CheckImage function to do any specific checks
//
- Status = FmpDeviceCheckImage ((((UINT8 *)Image) + AllHeaderSize), RawSize, ImageUpdatable);
+ Status = FmpDeviceCheckImage ((((UINT8 *)Image) + AllHeaderSize), RawSize, ImageUpdatable, LastAttemptStatus);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImage() - FmpDeviceLib CheckImage failed. Status = %r\n", mImageIdName, Status));
}
+ //
+ // LastAttemptStatus returned from the device library should either be a generic UEFI Specification defined value or
+ // fall within the designated LAST_ATTEMPT_STATUS_LIBRARY_ERROR range
+ //
+ if (
+ (*LastAttemptStatus < LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MIN_ERROR_CODE &&
+ *LastAttemptStatus >= LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN) ||
+ (*LastAttemptStatus > LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MAX_ERROR_CODE)) {
+ DEBUG ((DEBUG_ERROR, "FmpDxe(%s): CheckTheImage() - LastAttemptStatus from FmpDeviceCheckImage is invalid.\n", mImageIdName));
+ ASSERT (FALSE);
+ *LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
+ }
+
cleanup:
return Status;
}
@@ -1356,8 +1374,22 @@ SetTheImage (
VendorCode,
FmpDxeProgress,
IncomingFwVersion,
- AbortReason
+ AbortReason,
+ &LastAttemptStatus
);
+ //
+ // LastAttemptStatus returned from the device library should either be a generic UEFI Specification defined value or
+ // fall within the designated LAST_ATTEMPT_STATUS_LIBRARY_ERROR range
+ //
+ if (
+ (LastAttemptStatus < LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MIN_ERROR_CODE &&
+ LastAttemptStatus >= LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL_VENDOR_RANGE_MIN) ||
+ (LastAttemptStatus > LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MAX_ERROR_CODE)) {
+ DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() - LastAttemptStatus from FmpDeviceSetImage is invalid.\n", mImageIdName));
+ ASSERT (FALSE);
+ LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL;
+ }
+
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "FmpDxe(%s): SetTheImage() SetImage from FmpDeviceLib failed. Status = %r.\n", mImageIdName, Status));
goto cleanup;
diff --git a/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c b/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
index 316de12e910c..d08b5b82d42f 100644
--- a/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
+++ b/FmpDevicePkg/Library/FmpDeviceLibNull/FmpDeviceLib.c
@@ -2,7 +2,7 @@
Provides firmware device specific services to support updates of a firmware
image stored in a firmware device.
- Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -380,17 +380,22 @@ FmpDeviceGetImage (
function allows firmware update operation to validate the firmware image
before FmpDeviceSetImage() is called.
- @param[in] Image Points to a new firmware image.
- @param[in] ImageSize Size, in bytes, of a new firmware image.
- @param[out] ImageUpdatable Indicates if a new firmware image is valid for
- a firmware update to the firmware device. The
- following values from the Firmware Management
- Protocol are supported:
- IMAGE_UPDATABLE_VALID
- IMAGE_UPDATABLE_INVALID
- IMAGE_UPDATABLE_INVALID_TYPE
- IMAGE_UPDATABLE_INVALID_OLD
- IMAGE_UPDATABLE_VALID_WITH_VENDOR_CODE
+ @param[in] Image Points to a new firmware image.
+ @param[in] ImageSize Size, in bytes, of a new firmware image.
+ @param[out] ImageUpdatable Indicates if a new firmware image is valid for
+ a firmware update to the firmware device. The
+ following values from the Firmware Management
+ Protocol are supported:
+ IMAGE_UPDATABLE_VALID
+ IMAGE_UPDATABLE_INVALID
+ IMAGE_UPDATABLE_INVALID_TYPE
+ IMAGE_UPDATABLE_INVALID_OLD
+ IMAGE_UPDATABLE_VALID_WITH_VENDOR_CODE
+ @param[out] LastAttemptStatus A pointer to a UINT32 that holds the last attempt
+ status to report back to the ESRT table in case
+ of error. The return status code must fall in the range of
+ LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MIN_ERROR_CODE to
+ LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MAX_ERROR_CODE.
@retval EFI_SUCCESS The image was successfully checked. Additional
status information is returned in
@@ -404,7 +409,8 @@ EFIAPI
FmpDeviceCheckImage (
IN CONST VOID *Image,
IN UINTN ImageSize,
- OUT UINT32 *ImageUpdatable
+ OUT UINT32 *ImageUpdatable,
+ OUT UINT32 *LastAttemptStatus
)
{
return EFI_SUCCESS;
@@ -453,6 +459,13 @@ FmpDeviceCheckImage (
EFI_BOOT_SERVICES.AllocatePool(). It is the
caller's responsibility to free this buffer with
EFI_BOOT_SERVICES.FreePool().
+ @param[out] LastAttemptStatus A pointer to a UINT32 that holds the last attempt
+ status to report back to the ESRT table in case
+ of error. Will only be checked when this funtions
+ returns error. Returned status code falls outside of
+ LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MIN_ERROR_CODE and
+ LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MAX_ERROR_CODE
+ will be converted to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL
@retval EFI_SUCCESS The firmware device was successfully updated
with the new firmware image.
@@ -470,7 +483,8 @@ FmpDeviceSetImage (
IN CONST VOID *VendorCode, OPTIONAL
IN EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS Progress, OPTIONAL
IN UINT32 CapsuleFwVersion,
- OUT CHAR16 **AbortReason
+ OUT CHAR16 **AbortReason,
+ OUT UINT32 *LastAttemptStatus
)
{
return EFI_UNSUPPORTED;
diff --git a/FmpDevicePkg/Include/Library/FmpDeviceLib.h b/FmpDevicePkg/Include/Library/FmpDeviceLib.h
index 9a89f5c2eec5..163714654d60 100644
--- a/FmpDevicePkg/Include/Library/FmpDeviceLib.h
+++ b/FmpDevicePkg/Include/Library/FmpDeviceLib.h
@@ -2,7 +2,7 @@
Provides firmware device specific services to support updates of a firmware
image stored in a firmware device.
- Copyright (c) 2016, Microsoft Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
Copyright (c) 2018 - 2019, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -12,6 +12,8 @@
#ifndef __FMP_DEVICE_LIB__
#define __FMP_DEVICE_LIB__
+#include <Guid/SystemResourceTable.h>
+#include <LastAttemptStatus.h>
#include <Protocol/FirmwareManagement.h>
/**
@@ -376,17 +378,22 @@ FmpDeviceGetImage (
function allows firmware update operation to validate the firmware image
before FmpDeviceSetImage() is called.
- @param[in] Image Points to a new firmware image.
- @param[in] ImageSize Size, in bytes, of a new firmware image.
- @param[out] ImageUpdatable Indicates if a new firmware image is valid for
- a firmware update to the firmware device. The
- following values from the Firmware Management
- Protocol are supported:
- IMAGE_UPDATABLE_VALID
- IMAGE_UPDATABLE_INVALID
- IMAGE_UPDATABLE_INVALID_TYPE
- IMAGE_UPDATABLE_INVALID_OLD
- IMAGE_UPDATABLE_VALID_WITH_VENDOR_CODE
+ @param[in] Image Points to a new firmware image.
+ @param[in] ImageSize Size, in bytes, of a new firmware image.
+ @param[out] ImageUpdatable Indicates if a new firmware image is valid for
+ a firmware update to the firmware device. The
+ following values from the Firmware Management
+ Protocol are supported:
+ IMAGE_UPDATABLE_VALID
+ IMAGE_UPDATABLE_INVALID
+ IMAGE_UPDATABLE_INVALID_TYPE
+ IMAGE_UPDATABLE_INVALID_OLD
+ IMAGE_UPDATABLE_VALID_WITH_VENDOR_CODE
+ @param[out] LastAttemptStatus A pointer to a UINT32 that holds the last attempt
+ status to report back to the ESRT table in case
+ of error. The return status code must fall in the range of
+ LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MIN_ERROR_CODE to
+ LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MAX_ERROR_CODE.
@retval EFI_SUCCESS The image was successfully checked. Additional
status information is returned in
@@ -400,7 +407,8 @@ EFIAPI
FmpDeviceCheckImage (
IN CONST VOID *Image,
IN UINTN ImageSize,
- OUT UINT32 *ImageUpdatable
+ OUT UINT32 *ImageUpdatable,
+ OUT UINT32 *LastAttemptStatus
);
/**
@@ -446,6 +454,13 @@ FmpDeviceCheckImage (
EFI_BOOT_SERVICES.AllocatePool(). It is the
caller's responsibility to free this buffer with
EFI_BOOT_SERVICES.FreePool().
+ @param[out] LastAttemptStatus A pointer to a UINT32 that holds the last attempt
+ status to report back to the ESRT table in case
+ of error. Will only be checked when this funtions
+ returns error. Returned status code falls outside of
+ LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MIN_ERROR_CODE and
+ LAST_ATTEMPT_STATUS_LIBRARY_ERROR_MAX_ERROR_CODE
+ will be converted to LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL
@retval EFI_SUCCESS The firmware device was successfully updated
with the new firmware image.
@@ -463,7 +478,8 @@ FmpDeviceSetImage (
IN CONST VOID *VendorCode, OPTIONAL
IN EFI_FIRMWARE_MANAGEMENT_UPDATE_IMAGE_PROGRESS Progress, OPTIONAL
IN UINT32 CapsuleFwVersion,
- OUT CHAR16 **AbortReason
+ OUT CHAR16 **AbortReason,
+ OUT UINT32 *LastAttemptStatus
);
/**
--
2.28.0.windows.1
^ permalink raw reply related [flat|nested] 14+ messages in thread