[PATCH 0/8] Decode IA32/X64 CPER

[PATCH 0/8] Decode IA32/X64 CPER

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

This series adds decoding for the IA32/X64 Common Platform Error Record.

Patch 1 fixes the IA32/X64 Processor Error Section definition to match
the UEFI spec.

Patches 2-8 add the new decoding. The patches incrementally add the
decoding starting from the top-level "Error Section". Hopefully, this
will make reviewing a bit easier compared to one large patch.

The formatting of the field names and options is taken from the UEFI
spec. I tried to keep everything the same to make searching easier.

The patches were written to the UEFI 2.7 spec though the definition of
the IA32/X64 CPER seems to be the same as when it was introduced in
the UEFI 2.1 spec.

Without basic decoding, users will be confused about what these
"Hardware Errors" mean. So I'm requesting this set to be applied to the
stable branches. This set applies to the v4.16. However, patch 2 will
have a conflict on older branches, so I'll send this set again with the
conflict fixed.

Thanks,
Yazen

Yazen Ghannam (8):
  efi: Fix IA32/X64 Processor Error Record definition
  efi: Decode IA32/X64 Processor Error Section
  efi: Decode IA32/X64 Processor Error Info Structure
  efi: Decode UEFI-defined IA32/X64 Error Structure GUIDs
  efi: Decode IA32/X64 Cache, TLB, and Bus Check structures
  efi: Decode additional IA32/X64 Bus Check fields
  efi: Decode IA32/X64 MS Check structure
  efi: Decode IA32/X64 Context Info structure

 drivers/firmware/efi/Kconfig    |   5 +
 drivers/firmware/efi/Makefile   |   2 +
 drivers/firmware/efi/cper-x86.c | 371 ++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/cper.c     |  10 ++
 include/linux/cper.h            |   4 +-
 5 files changed, 391 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/efi/cper-x86.c

-- 
2.14.1

[PATCH 1/8] efi: Fix IA32/X64 Processor Error Record definition

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Based on UEFI 2.7 Table 255. Processor Error Record, the "Local APIC_ID"
field is 8 bytes but Linux defines this field as 1 byte.

Fix this in the struct cper_sec_proc_ia definition.

Cc: <stable@vger.kernel.org> # 4.16.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 include/linux/cper.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/cper.h b/include/linux/cper.h
index d14ef4e77c8a..4b5f8459b403 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -381,7 +381,7 @@ struct cper_sec_proc_generic {
 /* IA32/X64 Processor Error Section */
 struct cper_sec_proc_ia {
 	__u64	validation_bits;
-	__u8	lapic_id;
+	__u64	lapic_id;
 	__u8	cpuid[48];
 };
 
-- 
2.14.1

[PATCH 2/8] efi: Decode IA32/X64 Processor Error Section

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Recognize the IA32/X64 Processor Error Section.

Do the section decoding in a new "cper-x86.c" file and add this to the
Makefile depending on a new "UEFI_CPER_X86" config option.

Print the Local APIC ID and CPUID info from the Processor Error Record.

The "Processor Error Info" and "Processor Context" fields will be
decoded in following patches.

Based on UEFI 2.7 Table 255. Processor Error Record.

Cc: <stable@vger.kernel.org> # 4.16.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/firmware/efi/Kconfig    |  5 +++++
 drivers/firmware/efi/Makefile   |  2 ++
 drivers/firmware/efi/cper-x86.c | 26 ++++++++++++++++++++++++++
 drivers/firmware/efi/cper.c     | 10 ++++++++++
 include/linux/cper.h            |  2 ++
 5 files changed, 45 insertions(+)
 create mode 100644 drivers/firmware/efi/cper-x86.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 3098410abad8..8b684147835d 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -174,6 +174,11 @@ config UEFI_CPER_ARM
 	depends on UEFI_CPER && ( ARM || ARM64 )
 	default y
 
+config UEFI_CPER_X86
+	bool
+	depends on UEFI_CPER && ( X86_32 || X86_64 )
+	default y
+
 config EFI_DEV_PATH_PARSER
 	bool
 	depends on ACPI
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index cb805374f4bc..7cf8d75ebe51 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -31,3 +31,5 @@ obj-$(CONFIG_ARM)			+= $(arm-obj-y)
 obj-$(CONFIG_ARM64)			+= $(arm-obj-y)
 obj-$(CONFIG_EFI_CAPSULE_LOADER)	+= capsule-loader.o
 obj-$(CONFIG_UEFI_CPER_ARM)		+= cper-arm.o
+
+obj-$(CONFIG_UEFI_CPER_X86)		+= cper-x86.o
diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
new file mode 100644
index 000000000000..b50ee3cdf637
--- /dev/null
+++ b/drivers/firmware/efi/cper-x86.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018, Advanced Micro Devices, Inc.
+// Copyright (C) 2018, Yazen Ghannam <yazen.ghannam@amd.com>
+
+#include <linux/cper.h>
+
+/*
+ * We don't need a "CPER_IA" prefix since these are all locally defined.
+ * This will save us a lot of line space.
+ */
+#define VALID_LAPIC_ID			BIT_ULL(0)
+#define VALID_CPUID_INFO		BIT_ULL(1)
+
+void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
+{
+	printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
+
+	if (proc->validation_bits & VALID_LAPIC_ID)
+		printk("%sLocal APIC_ID: 0x%llx\n", pfx, proc->lapic_id);
+
+	if (proc->validation_bits & VALID_CPUID_INFO) {
+		printk("%sCPUID Info:\n", pfx);
+		print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc->cpuid,
+			       sizeof(proc->cpuid), 0);
+	}
+}
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index c165933ebf38..5a59b582c9aa 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -469,6 +469,16 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata
 			cper_print_proc_arm(newpfx, arm_err);
 		else
 			goto err_section_too_small;
+#endif
+#if defined(CONFIG_UEFI_CPER_X86)
+	} else if (guid_equal(sec_type, &CPER_SEC_PROC_IA)) {
+		struct cper_sec_proc_ia *ia_err = acpi_hest_get_payload(gdata);
+
+		printk("%ssection_type: IA32/X64 processor error\n", newpfx);
+		if (gdata->error_data_length >= sizeof(*ia_err))
+			cper_print_proc_ia(newpfx, ia_err);
+		else
+			goto err_section_too_small;
 #endif
 	} else {
 		const void *err = acpi_hest_get_payload(gdata);
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 4b5f8459b403..9c703a0abe6e 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -551,5 +551,7 @@ const char *cper_mem_err_unpack(struct trace_seq *,
 				struct cper_mem_err_compact *);
 void cper_print_proc_arm(const char *pfx,
 			 const struct cper_sec_proc_arm *proc);
+void cper_print_proc_ia(const char *pfx,
+			const struct cper_sec_proc_ia *proc);
 
 #endif
-- 
2.14.1

[PATCH 3/8] efi: Decode IA32/X64 Processor Error Info Structure

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Print the fields in the IA32/X64 Processor Error Info Structure.

Based on UEFI 2.7 Table 256. IA32/X64 Processor Error Information
Structure.

Cc: <stable@vger.kernel.org> # 4.16.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/firmware/efi/cper-x86.c | 53 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
index b50ee3cdf637..9d608f742c98 100644
--- a/drivers/firmware/efi/cper-x86.c
+++ b/drivers/firmware/efi/cper-x86.c
@@ -4,15 +4,28 @@
 
 #include <linux/cper.h>
 
+#define INDENT_SP	" "
+
 /*
  * We don't need a "CPER_IA" prefix since these are all locally defined.
  * This will save us a lot of line space.
  */
 #define VALID_LAPIC_ID			BIT_ULL(0)
 #define VALID_CPUID_INFO		BIT_ULL(1)
+#define VALID_PROC_ERR_INFO_NUM(bits)	((bits & GENMASK_ULL(7, 2)) >> 2)
+
+#define INFO_VALID_CHECK_INFO		BIT_ULL(0)
+#define INFO_VALID_TARGET_ID		BIT_ULL(1)
+#define INFO_VALID_REQUESTOR_ID		BIT_ULL(2)
+#define INFO_VALID_RESPONDER_ID		BIT_ULL(3)
+#define INFO_VALID_IP			BIT_ULL(4)
 
 void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
 {
+	int i;
+	struct cper_ia_err_info *err_info;
+	char newpfx[64];
+
 	printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
 
 	if (proc->validation_bits & VALID_LAPIC_ID)
@@ -23,4 +36,44 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
 		print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc->cpuid,
 			       sizeof(proc->cpuid), 0);
 	}
+
+	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
+
+	err_info = (struct cper_ia_err_info *)(proc + 1);
+	for (i = 0; i < VALID_PROC_ERR_INFO_NUM(proc->validation_bits); i++) {
+		printk("%sError Information Structure %d:\n", pfx, i);
+
+		printk("%sError Structure Type: %pUl\n", newpfx,
+			 &err_info->err_type);
+
+		printk("%sValidation Bits: 0x%016llx\n",
+			 newpfx, err_info->validation_bits);
+
+		if (err_info->validation_bits & INFO_VALID_CHECK_INFO) {
+			printk("%sCheck Information: 0x%016llx\n", newpfx,
+				 err_info->check_info);
+		}
+
+		if (err_info->validation_bits & INFO_VALID_TARGET_ID) {
+			printk("%sTarget Identifier: 0x%016llx\n",
+				 newpfx, err_info->target_id);
+		}
+
+		if (err_info->validation_bits & INFO_VALID_REQUESTOR_ID) {
+			printk("%sRequestor Identifier: 0x%016llx\n",
+				 newpfx, err_info->requestor_id);
+		}
+
+		if (err_info->validation_bits & INFO_VALID_RESPONDER_ID) {
+			printk("%sResponder Identifier: 0x%016llx\n",
+				 newpfx, err_info->responder_id);
+		}
+
+		if (err_info->validation_bits & INFO_VALID_IP) {
+			printk("%sInstruction Pointer: 0x%016llx\n",
+				 newpfx, err_info->ip);
+		}
+
+		err_info++;
+	}
 }
-- 
2.14.1

[PATCH 4/8] efi: Decode UEFI-defined IA32/X64 Error Structure GUIDs

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

For easier handling, match the known IA32/X64 error structure GUIDs to
enums.

Also, print out the name of the matching Error Structure Type.

GUIDs taken from UEFI 2.7 section N.2.4.2.1 IA32/X64 Processor Error
Information Structure.

Cc: <stable@vger.kernel.org> # 4.16.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/firmware/efi/cper-x86.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
index 9d608f742c98..3b86223bdb67 100644
--- a/drivers/firmware/efi/cper-x86.c
+++ b/drivers/firmware/efi/cper-x86.c
@@ -14,17 +14,53 @@
 #define VALID_CPUID_INFO		BIT_ULL(1)
 #define VALID_PROC_ERR_INFO_NUM(bits)	((bits & GENMASK_ULL(7, 2)) >> 2)
 
+#define INFO_ERR_STRUCT_TYPE_CACHE					\
+	GUID_INIT(0xA55701F5, 0xE3EF, 0x43DE, 0xAC, 0x72, 0x24, 0x9B,	\
+		  0x57, 0x3F, 0xAD, 0x2C)
+#define INFO_ERR_STRUCT_TYPE_TLB					\
+	GUID_INIT(0xFC06B535, 0x5E1F, 0x4562, 0x9F, 0x25, 0x0A, 0x3B,	\
+		  0x9A, 0xDB, 0x63, 0xC3)
+#define INFO_ERR_STRUCT_TYPE_BUS					\
+	GUID_INIT(0x1CF3F8B3, 0xC5B1, 0x49a2, 0xAA, 0x59, 0x5E, 0xEF,	\
+		  0x92, 0xFF, 0xA6, 0x3C)
+#define INFO_ERR_STRUCT_TYPE_MS						\
+	GUID_INIT(0x48AB7F57, 0xDC34, 0x4f6c, 0xA7, 0xD3, 0xB0, 0xB5,	\
+		  0xB0, 0xA7, 0x43, 0x14)
+
 #define INFO_VALID_CHECK_INFO		BIT_ULL(0)
 #define INFO_VALID_TARGET_ID		BIT_ULL(1)
 #define INFO_VALID_REQUESTOR_ID		BIT_ULL(2)
 #define INFO_VALID_RESPONDER_ID		BIT_ULL(3)
 #define INFO_VALID_IP			BIT_ULL(4)
 
+enum err_types {
+	ERR_TYPE_CACHE = 0,
+	ERR_TYPE_TLB,
+	ERR_TYPE_BUS,
+	ERR_TYPE_MS,
+	N_ERR_TYPES
+};
+
+static enum err_types cper_get_err_type(const guid_t *err_type)
+{
+	if (guid_equal(err_type, &INFO_ERR_STRUCT_TYPE_CACHE))
+		return ERR_TYPE_CACHE;
+	else if (guid_equal(err_type, &INFO_ERR_STRUCT_TYPE_TLB))
+		return ERR_TYPE_TLB;
+	else if (guid_equal(err_type, &INFO_ERR_STRUCT_TYPE_BUS))
+		return ERR_TYPE_BUS;
+	else if (guid_equal(err_type, &INFO_ERR_STRUCT_TYPE_MS))
+		return ERR_TYPE_MS;
+	else
+		return N_ERR_TYPES;
+}
+
 void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
 {
 	int i;
 	struct cper_ia_err_info *err_info;
 	char newpfx[64];
+	enum err_types err_type;
 
 	printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
 
@@ -46,6 +82,11 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
 		printk("%sError Structure Type: %pUl\n", newpfx,
 			 &err_info->err_type);
 
+		err_type = cper_get_err_type(&err_info->err_type);
+		printk("%sError Structure Type: %s\n", newpfx,
+			 err_type < ARRAY_SIZE(cper_proc_error_type_strs) ?
+			 cper_proc_error_type_strs[err_type] : "unknown");
+
 		printk("%sValidation Bits: 0x%016llx\n",
 			 newpfx, err_info->validation_bits);
 
-- 
2.14.1

[PATCH 5/8] efi: Decode IA32/X64 Cache, TLB, and Bus Check structures

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Print the common fields of the Cache, TLB, and Bus check structures.The
fields of these three check types are the same except for a few more
fields in the Bus check structure. The remaining Bus check structure
fields will be decoded in a following patch.

Based on UEFI 2.7,
Table 257. IA32/X64 Cache Check Structure
Table 258. IA32/X64 TLB Check Structure
Table 259. IA32/X64 Bus Check Structure

Cc: <stable@vger.kernel.org> # 4.16.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/firmware/efi/cper-x86.c | 101 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
index 3b86223bdb67..75f664043076 100644
--- a/drivers/firmware/efi/cper-x86.c
+++ b/drivers/firmware/efi/cper-x86.c
@@ -33,6 +33,25 @@
 #define INFO_VALID_RESPONDER_ID		BIT_ULL(3)
 #define INFO_VALID_IP			BIT_ULL(4)
 
+#define CHECK_VALID_TRANS_TYPE		BIT_ULL(0)
+#define CHECK_VALID_OPERATION		BIT_ULL(1)
+#define CHECK_VALID_LEVEL		BIT_ULL(2)
+#define CHECK_VALID_PCC			BIT_ULL(3)
+#define CHECK_VALID_UNCORRECTED		BIT_ULL(4)
+#define CHECK_VALID_PRECISE_IP		BIT_ULL(5)
+#define CHECK_VALID_RESTARTABLE_IP	BIT_ULL(6)
+#define CHECK_VALID_OVERFLOW		BIT_ULL(7)
+
+#define CHECK_VALID_BITS(check)		((check & GENMASK_ULL(15, 0)))
+#define CHECK_TRANS_TYPE(check)		((check & GENMASK_ULL(17, 16)) >> 16)
+#define CHECK_OPERATION(check)		((check & GENMASK_ULL(21, 18)) >> 18)
+#define CHECK_LEVEL(check)		((check & GENMASK_ULL(24, 22)) >> 22)
+#define CHECK_PCC			BIT_ULL(25)
+#define CHECK_UNCORRECTED		BIT_ULL(26)
+#define CHECK_PRECISE_IP		BIT_ULL(27)
+#define CHECK_RESTARTABLE_IP		BIT_ULL(28)
+#define CHECK_OVERFLOW			BIT_ULL(29)
+
 enum err_types {
 	ERR_TYPE_CACHE = 0,
 	ERR_TYPE_TLB,
@@ -55,11 +74,83 @@ static enum err_types cper_get_err_type(const guid_t *err_type)
 		return N_ERR_TYPES;
 }
 
+static const char * const ia_check_trans_type_strs[] = {
+	"Instruction",
+	"Data Access",
+	"Generic",
+};
+
+static const char * const ia_check_op_strs[] = {
+	"generic error",
+	"generic read",
+	"generic write",
+	"data read",
+	"data write",
+	"instruction fetch",
+	"prefetch",
+	"eviction",
+	"snoop",
+};
+
+static inline void print_bool(char *str, const char *pfx, u64 check, u64 bit)
+{
+	printk("%s%s: %s\n", pfx, str, (check & bit) ? "true" : "false");
+}
+
+static void print_err_info(const char *pfx, enum err_types err_type, u64 check)
+{
+	u16 validation_bits = CHECK_VALID_BITS(check);
+
+	printk("%sValidation Bits: 0x%04x\n", pfx, validation_bits);
+
+	if (err_type == ERR_TYPE_MS)
+		return;
+
+	if (validation_bits & CHECK_VALID_TRANS_TYPE) {
+		u8 trans_type = CHECK_TRANS_TYPE(check);
+
+		printk("%sTransaction Type: %u, %s\n", pfx, trans_type,
+			 trans_type < ARRAY_SIZE(ia_check_trans_type_strs) ?
+			 ia_check_trans_type_strs[trans_type] : "unknown");
+	}
+
+	if (validation_bits & CHECK_VALID_OPERATION) {
+		u8 op = CHECK_OPERATION(check);
+
+		/*
+		 * CACHE has more operation types than TLB or BUS, though the
+		 * name and the order are the same.
+		 */
+		u8 max_ops = (err_type == ERR_TYPE_CACHE) ? 9 : 7;
+
+		printk("%sOperation: %u, %s\n", pfx, op,
+			 op < max_ops ? ia_check_op_strs[op] : "unknown");
+	}
+
+	if (validation_bits & CHECK_VALID_LEVEL)
+		printk("%sLevel: %llu\n", pfx, CHECK_LEVEL(check));
+
+	if (validation_bits & CHECK_VALID_PCC)
+		print_bool("Processor Context Corrupt", pfx, check, CHECK_PCC);
+
+	if (validation_bits & CHECK_VALID_UNCORRECTED)
+		print_bool("Uncorrected", pfx, check, CHECK_UNCORRECTED);
+
+	if (validation_bits & CHECK_VALID_PRECISE_IP)
+		print_bool("Precise IP", pfx, check, CHECK_PRECISE_IP);
+
+	if (validation_bits & CHECK_VALID_RESTARTABLE_IP)
+		print_bool("Restartable IP", pfx, check, CHECK_RESTARTABLE_IP);
+
+	if (validation_bits & CHECK_VALID_OVERFLOW)
+		print_bool("Overflow", pfx, check, CHECK_OVERFLOW);
+}
+
 void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
 {
 	int i;
 	struct cper_ia_err_info *err_info;
-	char newpfx[64];
+	char newpfx[64], infopfx[64];
 	enum err_types err_type;
 
 	printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
@@ -93,6 +184,14 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
 		if (err_info->validation_bits & INFO_VALID_CHECK_INFO) {
 			printk("%sCheck Information: 0x%016llx\n", newpfx,
 				 err_info->check_info);
+
+			if (err_type < N_ERR_TYPES) {
+				snprintf(infopfx, sizeof(infopfx), "%s%s",
+					 newpfx, INDENT_SP);
+
+				print_err_info(infopfx, err_type,
+					       err_info->check_info);
+			}
 		}
 
 		if (err_info->validation_bits & INFO_VALID_TARGET_ID) {
-- 
2.14.1

[PATCH 6/8] efi: Decode additional IA32/X64 Bus Check fields

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

The "Participation Type", "Time Out", and "Address Space" fields are
unique to the IA32/X64 Bus Check structure. Print these fields.

Based on UEFI 2.7 Table 259. IA32/X64 Bus Check Structure

Cc: <stable@vger.kernel.org> # 4.16.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/firmware/efi/cper-x86.c | 44 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
index 75f664043076..df4cf6221d8e 100644
--- a/drivers/firmware/efi/cper-x86.c
+++ b/drivers/firmware/efi/cper-x86.c
@@ -42,6 +42,10 @@
 #define CHECK_VALID_RESTARTABLE_IP	BIT_ULL(6)
 #define CHECK_VALID_OVERFLOW		BIT_ULL(7)
 
+#define CHECK_VALID_BUS_PART_TYPE	BIT_ULL(8)
+#define CHECK_VALID_BUS_TIME_OUT	BIT_ULL(9)
+#define CHECK_VALID_BUS_ADDR_SPACE	BIT_ULL(10)
+
 #define CHECK_VALID_BITS(check)		((check & GENMASK_ULL(15, 0)))
 #define CHECK_TRANS_TYPE(check)		((check & GENMASK_ULL(17, 16)) >> 16)
 #define CHECK_OPERATION(check)		((check & GENMASK_ULL(21, 18)) >> 18)
@@ -52,6 +56,10 @@
 #define CHECK_RESTARTABLE_IP		BIT_ULL(28)
 #define CHECK_OVERFLOW			BIT_ULL(29)
 
+#define CHECK_BUS_PART_TYPE(check)	((check & GENMASK_ULL(31, 30)) >> 30)
+#define CHECK_BUS_TIME_OUT		BIT_ULL(32)
+#define CHECK_BUS_ADDR_SPACE(check)	((check & GENMASK_ULL(34, 33)) >> 33)
+
 enum err_types {
 	ERR_TYPE_CACHE = 0,
 	ERR_TYPE_TLB,
@@ -92,6 +100,20 @@ static const char * const ia_check_op_strs[] = {
 	"snoop",
 };
 
+static const char * const ia_check_bus_part_type_strs[] = {
+	"Local Processor originated request",
+	"Local Processor responded to request",
+	"Local Processor observed",
+	"Generic",
+};
+
+static const char * const ia_check_bus_addr_space_strs[] = {
+	"Memory Access",
+	"Reserved",
+	"I/O",
+	"Other Transaction",
+};
+
 static inline void print_bool(char *str, const char *pfx, u64 check, u64 bit)
 {
 	printk("%s%s: %s\n", pfx, str, (check & bit) ? "true" : "false");
@@ -144,6 +166,28 @@ static void print_err_info(const char *pfx, enum err_types err_type, u64 check)
 
 	if (validation_bits & CHECK_VALID_OVERFLOW)
 		print_bool("Overflow", pfx, check, CHECK_OVERFLOW);
+
+	if (err_type != ERR_TYPE_BUS)
+		return;
+
+	if (validation_bits & CHECK_VALID_BUS_PART_TYPE) {
+		u8 part_type = CHECK_BUS_PART_TYPE(check);
+
+		printk("%sParticipation Type: %u, %s\n", pfx, part_type,
+			 part_type < ARRAY_SIZE(ia_check_bus_part_type_strs) ?
+			 ia_check_bus_part_type_strs[part_type] : "unknown");
+	}
+
+	if (validation_bits & CHECK_VALID_BUS_TIME_OUT)
+		print_bool("Time Out", pfx, check, CHECK_BUS_TIME_OUT);
+
+	if (validation_bits & CHECK_VALID_BUS_ADDR_SPACE) {
+		u8 addr_space = CHECK_BUS_ADDR_SPACE(check);
+
+		printk("%sAddress Space: %u, %s\n", pfx, addr_space,
+			 addr_space < ARRAY_SIZE(ia_check_bus_addr_space_strs) ?
+			 ia_check_bus_addr_space_strs[addr_space] : "unknown");
+	}
 }
 
 void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
-- 
2.14.1

[PATCH 7/8] efi: Decode IA32/X64 MS Check structure

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

The IA32/X64 MS Check structure varies from the other Check structures
in the the bit positions of its fields, and it includes an additional
"Error Type" field.

Decode the MS Check structure in a separate function.

Based on UEFI 2.7 Table 260. IA32/X64 MS Check Field Description.

Cc: <stable@vger.kernel.org> # 4.16.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/firmware/efi/cper-x86.c | 55 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
index df4cf6221d8e..02b1b424f537 100644
--- a/drivers/firmware/efi/cper-x86.c
+++ b/drivers/firmware/efi/cper-x86.c
@@ -60,6 +60,20 @@
 #define CHECK_BUS_TIME_OUT		BIT_ULL(32)
 #define CHECK_BUS_ADDR_SPACE(check)	((check & GENMASK_ULL(34, 33)) >> 33)
 
+#define CHECK_VALID_MS_ERR_TYPE		BIT_ULL(0)
+#define CHECK_VALID_MS_PCC		BIT_ULL(1)
+#define CHECK_VALID_MS_UNCORRECTED	BIT_ULL(2)
+#define CHECK_VALID_MS_PRECISE_IP	BIT_ULL(3)
+#define CHECK_VALID_MS_RESTARTABLE_IP	BIT_ULL(4)
+#define CHECK_VALID_MS_OVERFLOW		BIT_ULL(5)
+
+#define CHECK_MS_ERR_TYPE(check)	((check & GENMASK_ULL(18, 16)) >> 16)
+#define CHECK_MS_PCC			BIT_ULL(19)
+#define CHECK_MS_UNCORRECTED		BIT_ULL(20)
+#define CHECK_MS_PRECISE_IP		BIT_ULL(21)
+#define CHECK_MS_RESTARTABLE_IP		BIT_ULL(22)
+#define CHECK_MS_OVERFLOW		BIT_ULL(23)
+
 enum err_types {
 	ERR_TYPE_CACHE = 0,
 	ERR_TYPE_TLB,
@@ -114,19 +128,58 @@ static const char * const ia_check_bus_addr_space_strs[] = {
 	"Other Transaction",
 };
 
+static const char * const ia_check_ms_error_type_strs[] = {
+	"No Error",
+	"Unclassified",
+	"Microcode ROM Parity Error",
+	"External Error",
+	"FRC Error",
+	"Internal Unclassified",
+};
+
 static inline void print_bool(char *str, const char *pfx, u64 check, u64 bit)
 {
 	printk("%s%s: %s\n", pfx, str, (check & bit) ? "true" : "false");
 }
 
+static void print_err_info_ms(const char *pfx, u16 validation_bits, u64 check)
+{
+	if (validation_bits & CHECK_VALID_MS_ERR_TYPE) {
+		u8 err_type = CHECK_MS_ERR_TYPE(check);
+
+		printk("%sError Type: %u, %s\n", pfx, err_type,
+			 err_type < ARRAY_SIZE(ia_check_ms_error_type_strs) ?
+			 ia_check_ms_error_type_strs[err_type] : "unknown");
+	}
+
+	if (validation_bits & CHECK_VALID_MS_PCC)
+		print_bool("Processor Context Corrupt", pfx, check, CHECK_MS_PCC);
+
+	if (validation_bits & CHECK_VALID_MS_UNCORRECTED)
+		print_bool("Uncorrected", pfx, check, CHECK_MS_UNCORRECTED);
+
+	if (validation_bits & CHECK_VALID_MS_PRECISE_IP)
+		print_bool("Precise IP", pfx, check, CHECK_MS_PRECISE_IP);
+
+	if (validation_bits & CHECK_VALID_MS_RESTARTABLE_IP)
+		print_bool("Restartable IP", pfx, check, CHECK_MS_RESTARTABLE_IP);
+
+	if (validation_bits & CHECK_VALID_MS_OVERFLOW)
+		print_bool("Overflow", pfx, check, CHECK_MS_OVERFLOW);
+}
+
 static void print_err_info(const char *pfx, enum err_types err_type, u64 check)
 {
 	u16 validation_bits = CHECK_VALID_BITS(check);
 
 	printk("%sValidation Bits: 0x%04x\n", pfx, validation_bits);
 
+	/*
+	 * The MS Check structure varies a lot from the others, so use a
+	 * separate function for decoding.
+	 */
 	if (err_type == ERR_TYPE_MS)
-		return;
+		return print_err_info_ms(pfx, validation_bits, check);
 
 	if (validation_bits & CHECK_VALID_TRANS_TYPE) {
 		u8 trans_type = CHECK_TRANS_TYPE(check);
-- 
2.14.1

[PATCH 8/8] efi: Decode IA32/X64 Context Info structure

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Print the fields of the IA32/X64 Context Information structure.

Print the "Register Array" as raw values. Some context types are defined
in the UEFI spec, so more detailed decoded may be added in the future.

Based on UEFI 2.7 section N.2.4.2.2 IA32/X64 Processor Context
Information Structure.

Cc: <stable@vger.kernel.org> # 4.16.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 drivers/firmware/efi/cper-x86.c | 55 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
index 02b1b424f537..bb6cef0b5e53 100644
--- a/drivers/firmware/efi/cper-x86.c
+++ b/drivers/firmware/efi/cper-x86.c
@@ -13,6 +13,7 @@
 #define VALID_LAPIC_ID			BIT_ULL(0)
 #define VALID_CPUID_INFO		BIT_ULL(1)
 #define VALID_PROC_ERR_INFO_NUM(bits)	((bits & GENMASK_ULL(7, 2)) >> 2)
+#define VALID_PROC_CNXT_INFO_NUM(bits)	((bits & GENMASK_ULL(13, 8)) >> 8)
 
 #define INFO_ERR_STRUCT_TYPE_CACHE					\
 	GUID_INIT(0xA55701F5, 0xE3EF, 0x43DE, 0xAC, 0x72, 0x24, 0x9B,	\
@@ -74,6 +75,9 @@
 #define CHECK_MS_RESTARTABLE_IP		BIT_ULL(22)
 #define CHECK_MS_OVERFLOW		BIT_ULL(23)
 
+#define CTX_TYPE_MSR			1
+#define CTX_TYPE_MMREG			7
+
 enum err_types {
 	ERR_TYPE_CACHE = 0,
 	ERR_TYPE_TLB,
@@ -137,6 +141,17 @@ static const char * const ia_check_ms_error_type_strs[] = {
 	"Internal Unclassified",
 };
 
+static const char * const ia_reg_ctx_strs[] = {
+	"Unclassified Data",
+	"MSR Registers (Machine Check and other MSRs)",
+	"32-bit Mode Execution Context",
+	"64-bit Mode Execution Context",
+	"FXSAVE Context",
+	"32-bit Mode Debug Registers (DR0-DR7)",
+	"64-bit Mode Debug Registers (DR0-DR7)",
+	"Memory Mapped Registers",
+};
+
 static inline void print_bool(char *str, const char *pfx, u64 check, u64 bit)
 {
 	printk("%s%s: %s\n", pfx, str, (check & bit) ? "true" : "false");
@@ -247,8 +262,10 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
 {
 	int i;
 	struct cper_ia_err_info *err_info;
+	struct cper_ia_proc_ctx *ctx_info;
 	char newpfx[64], infopfx[64];
 	enum err_types err_type;
+	unsigned int max_ctx_type = ARRAY_SIZE(ia_reg_ctx_strs) - 1;
 
 	printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
 
@@ -313,4 +330,42 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
 
 		err_info++;
 	}
+
+	ctx_info = (struct cper_ia_proc_ctx *)err_info;
+	for (i = 0; i < VALID_PROC_CNXT_INFO_NUM(proc->validation_bits); i++) {
+		int size = sizeof(*ctx_info) + ctx_info->reg_arr_size;
+		int groupsize = 4;
+
+		printk("%sContext Information Structure %d:\n", pfx, i);
+
+		if (ctx_info->reg_ctx_type > max_ctx_type) {
+			printk("%sInvalid Register Context Type: %d (max: %d)\n",
+				 newpfx, ctx_info->reg_ctx_type, max_ctx_type);
+			goto next_ctx;
+		}
+
+		printk("%sRegister Context Type: %s\n", newpfx,
+			 ia_reg_ctx_strs[ctx_info->reg_ctx_type]);
+
+		printk("%sRegister Array Size: 0x%04x\n", newpfx,
+			 ctx_info->reg_arr_size);
+
+		if (ctx_info->reg_ctx_type == CTX_TYPE_MSR) {
+			groupsize = 8; /* MSRs are 8 bytes wide. */
+			printk("%sMSR Address: 0x%08x\n", newpfx,
+				 ctx_info->msr_addr);
+		}
+
+		if (ctx_info->reg_ctx_type == CTX_TYPE_MMREG) {
+			printk("%sMM Register Address: 0x%016llx\n", newpfx,
+				 ctx_info->mm_reg_addr);
+		}
+
+		printk("%sRegister Array:\n", newpfx);
+		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, groupsize,
+			       (ctx_info + 1), ctx_info->reg_arr_size, 0);
+
+next_ctx:
+		ctx_info = (struct cper_ia_proc_ctx *)((long)ctx_info + size);
+	}
 }
-- 
2.14.1

Re: [PATCH 2/8] efi: Decode IA32/X64 Processor Error Section

[Ard Biesheuvel]

On 23 February 2018 at 20:03, Yazen Ghannam <Yazen.Ghannam@amd.com> wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> Recognize the IA32/X64 Processor Error Section.
>
> Do the section decoding in a new "cper-x86.c" file and add this to the
> Makefile depending on a new "UEFI_CPER_X86" config option.
>
> Print the Local APIC ID and CPUID info from the Processor Error Record.
>
> The "Processor Error Info" and "Processor Context" fields will be
> decoded in following patches.
>
> Based on UEFI 2.7 Table 255. Processor Error Record.
>
> Cc: <stable@vger.kernel.org> # 4.16.x
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  drivers/firmware/efi/Kconfig    |  5 +++++
>  drivers/firmware/efi/Makefile   |  2 ++
>  drivers/firmware/efi/cper-x86.c | 26 ++++++++++++++++++++++++++
>  drivers/firmware/efi/cper.c     | 10 ++++++++++
>  include/linux/cper.h            |  2 ++
>  5 files changed, 45 insertions(+)
>  create mode 100644 drivers/firmware/efi/cper-x86.c
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 3098410abad8..8b684147835d 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -174,6 +174,11 @@ config UEFI_CPER_ARM
>         depends on UEFI_CPER && ( ARM || ARM64 )
>         default y
>
> +config UEFI_CPER_X86
> +       bool
> +       depends on UEFI_CPER && ( X86_32 || X86_64 )

Just 'UEFI_CPER && X86' should be sufficient, no?

> +       default y
> +
>  config EFI_DEV_PATH_PARSER
>         bool
>         depends on ACPI
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index cb805374f4bc..7cf8d75ebe51 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -31,3 +31,5 @@ obj-$(CONFIG_ARM)                     += $(arm-obj-y)
>  obj-$(CONFIG_ARM64)                    += $(arm-obj-y)
>  obj-$(CONFIG_EFI_CAPSULE_LOADER)       += capsule-loader.o
>  obj-$(CONFIG_UEFI_CPER_ARM)            += cper-arm.o
> +

Spurious newline

> +obj-$(CONFIG_UEFI_CPER_X86)            += cper-x86.o
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> new file mode 100644
> index 000000000000..b50ee3cdf637
> --- /dev/null
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018, Advanced Micro Devices, Inc.
> +// Copyright (C) 2018, Yazen Ghannam <yazen.ghannam@amd.com>
> +

Do you really own the copyright for code that you write on behalf of
your employer?

> +#include <linux/cper.h>
> +
> +/*
> + * We don't need a "CPER_IA" prefix since these are all locally defined.
> + * This will save us a lot of line space.
> + */
> +#define VALID_LAPIC_ID                 BIT_ULL(0)
> +#define VALID_CPUID_INFO               BIT_ULL(1)
> +
> +void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
> +{
> +       printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
> +
> +       if (proc->validation_bits & VALID_LAPIC_ID)
> +               printk("%sLocal APIC_ID: 0x%llx\n", pfx, proc->lapic_id);
> +
> +       if (proc->validation_bits & VALID_CPUID_INFO) {
> +               printk("%sCPUID Info:\n", pfx);
> +               print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc->cpuid,
> +                              sizeof(proc->cpuid), 0);
> +       }
> +}
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index c165933ebf38..5a59b582c9aa 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -469,6 +469,16 @@ cper_estatus_print_section(const char *pfx, struct acpi_hest_generic_data *gdata
>                         cper_print_proc_arm(newpfx, arm_err);
>                 else
>                         goto err_section_too_small;
> +#endif
> +#if defined(CONFIG_UEFI_CPER_X86)
> +       } else if (guid_equal(sec_type, &CPER_SEC_PROC_IA)) {
> +               struct cper_sec_proc_ia *ia_err = acpi_hest_get_payload(gdata);
> +
> +               printk("%ssection_type: IA32/X64 processor error\n", newpfx);
> +               if (gdata->error_data_length >= sizeof(*ia_err))
> +                       cper_print_proc_ia(newpfx, ia_err);
> +               else
> +                       goto err_section_too_small;
>  #endif
>         } else {
>                 const void *err = acpi_hest_get_payload(gdata);
> diff --git a/include/linux/cper.h b/include/linux/cper.h
> index 4b5f8459b403..9c703a0abe6e 100644
> --- a/include/linux/cper.h
> +++ b/include/linux/cper.h
> @@ -551,5 +551,7 @@ const char *cper_mem_err_unpack(struct trace_seq *,
>                                 struct cper_mem_err_compact *);
>  void cper_print_proc_arm(const char *pfx,
>                          const struct cper_sec_proc_arm *proc);
> +void cper_print_proc_ia(const char *pfx,
> +                       const struct cper_sec_proc_ia *proc);
>
>  #endif
> --
> 2.14.1
>

Re: [PATCH 3/8] efi: Decode IA32/X64 Processor Error Info Structure

[Ard Biesheuvel]

On 23 February 2018 at 20:03, Yazen Ghannam <Yazen.Ghannam@amd.com> wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> Print the fields in the IA32/X64 Processor Error Info Structure.
>
> Based on UEFI 2.7 Table 256. IA32/X64 Processor Error Information
> Structure.
>
> Cc: <stable@vger.kernel.org> # 4.16.x
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  drivers/firmware/efi/cper-x86.c | 53 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> index b50ee3cdf637..9d608f742c98 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -4,15 +4,28 @@
>
>  #include <linux/cper.h>
>
> +#define INDENT_SP      " "
> +
>  /*
>   * We don't need a "CPER_IA" prefix since these are all locally defined.
>   * This will save us a lot of line space.
>   */
>  #define VALID_LAPIC_ID                 BIT_ULL(0)
>  #define VALID_CPUID_INFO               BIT_ULL(1)
> +#define VALID_PROC_ERR_INFO_NUM(bits)  ((bits & GENMASK_ULL(7, 2)) >> 2)
> +

Parens around 'bits' please

> +#define INFO_VALID_CHECK_INFO          BIT_ULL(0)
> +#define INFO_VALID_TARGET_ID           BIT_ULL(1)
> +#define INFO_VALID_REQUESTOR_ID                BIT_ULL(2)
> +#define INFO_VALID_RESPONDER_ID                BIT_ULL(3)
> +#define INFO_VALID_IP                  BIT_ULL(4)
>
>  void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
>  {
> +       int i;
> +       struct cper_ia_err_info *err_info;
> +       char newpfx[64];
> +
>         printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
>
>         if (proc->validation_bits & VALID_LAPIC_ID)
> @@ -23,4 +36,44 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
>                 print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc->cpuid,
>                                sizeof(proc->cpuid), 0);
>         }
> +
> +       snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> +
> +       err_info = (struct cper_ia_err_info *)(proc + 1);
> +       for (i = 0; i < VALID_PROC_ERR_INFO_NUM(proc->validation_bits); i++) {
> +               printk("%sError Information Structure %d:\n", pfx, i);
> +
> +               printk("%sError Structure Type: %pUl\n", newpfx,
> +                        &err_info->err_type);
> +

The indentation is a bit awkward here. Could you please align followup
lines with the character following the ( on the first line?

> +               printk("%sValidation Bits: 0x%016llx\n",
> +                        newpfx, err_info->validation_bits);
> +
> +               if (err_info->validation_bits & INFO_VALID_CHECK_INFO) {
> +                       printk("%sCheck Information: 0x%016llx\n", newpfx,
> +                                err_info->check_info);
> +               }
> +
> +               if (err_info->validation_bits & INFO_VALID_TARGET_ID) {
> +                       printk("%sTarget Identifier: 0x%016llx\n",
> +                                newpfx, err_info->target_id);
> +               }
> +
> +               if (err_info->validation_bits & INFO_VALID_REQUESTOR_ID) {
> +                       printk("%sRequestor Identifier: 0x%016llx\n",
> +                                newpfx, err_info->requestor_id);
> +               }
> +
> +               if (err_info->validation_bits & INFO_VALID_RESPONDER_ID) {
> +                       printk("%sResponder Identifier: 0x%016llx\n",
> +                                newpfx, err_info->responder_id);
> +               }
> +
> +               if (err_info->validation_bits & INFO_VALID_IP) {
> +                       printk("%sInstruction Pointer: 0x%016llx\n",
> +                                newpfx, err_info->ip);
> +               }
> +
> +               err_info++;
> +       }
>  }
> --
> 2.14.1
>

Re: [PATCH 4/8] efi: Decode UEFI-defined IA32/X64 Error Structure GUIDs

[Ard Biesheuvel]

On 23 February 2018 at 20:03, Yazen Ghannam <Yazen.Ghannam@amd.com> wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> For easier handling, match the known IA32/X64 error structure GUIDs to
> enums.
>
> Also, print out the name of the matching Error Structure Type.
>
> GUIDs taken from UEFI 2.7 section N.2.4.2.1 IA32/X64 Processor Error
> Information Structure.
>
> Cc: <stable@vger.kernel.org> # 4.16.x
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  drivers/firmware/efi/cper-x86.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> index 9d608f742c98..3b86223bdb67 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -14,17 +14,53 @@
>  #define VALID_CPUID_INFO               BIT_ULL(1)
>  #define VALID_PROC_ERR_INFO_NUM(bits)  ((bits & GENMASK_ULL(7, 2)) >> 2)
>
> +#define INFO_ERR_STRUCT_TYPE_CACHE                                     \
> +       GUID_INIT(0xA55701F5, 0xE3EF, 0x43DE, 0xAC, 0x72, 0x24, 0x9B,   \
> +                 0x57, 0x3F, 0xAD, 0x2C)
> +#define INFO_ERR_STRUCT_TYPE_TLB                                       \
> +       GUID_INIT(0xFC06B535, 0x5E1F, 0x4562, 0x9F, 0x25, 0x0A, 0x3B,   \
> +                 0x9A, 0xDB, 0x63, 0xC3)
> +#define INFO_ERR_STRUCT_TYPE_BUS                                       \
> +       GUID_INIT(0x1CF3F8B3, 0xC5B1, 0x49a2, 0xAA, 0x59, 0x5E, 0xEF,   \
> +                 0x92, 0xFF, 0xA6, 0x3C)
> +#define INFO_ERR_STRUCT_TYPE_MS                                                \
> +       GUID_INIT(0x48AB7F57, 0xDC34, 0x4f6c, 0xA7, 0xD3, 0xB0, 0xB5,   \
> +                 0xB0, 0xA7, 0x43, 0x14)
> +
>  #define INFO_VALID_CHECK_INFO          BIT_ULL(0)
>  #define INFO_VALID_TARGET_ID           BIT_ULL(1)
>  #define INFO_VALID_REQUESTOR_ID                BIT_ULL(2)
>  #define INFO_VALID_RESPONDER_ID                BIT_ULL(3)
>  #define INFO_VALID_IP                  BIT_ULL(4)
>
> +enum err_types {
> +       ERR_TYPE_CACHE = 0,
> +       ERR_TYPE_TLB,
> +       ERR_TYPE_BUS,
> +       ERR_TYPE_MS,
> +       N_ERR_TYPES
> +};
> +
> +static enum err_types cper_get_err_type(const guid_t *err_type)
> +{
> +       if (guid_equal(err_type, &INFO_ERR_STRUCT_TYPE_CACHE))
> +               return ERR_TYPE_CACHE;
> +       else if (guid_equal(err_type, &INFO_ERR_STRUCT_TYPE_TLB))
> +               return ERR_TYPE_TLB;
> +       else if (guid_equal(err_type, &INFO_ERR_STRUCT_TYPE_BUS))
> +               return ERR_TYPE_BUS;
> +       else if (guid_equal(err_type, &INFO_ERR_STRUCT_TYPE_MS))
> +               return ERR_TYPE_MS;
> +       else
> +               return N_ERR_TYPES;
> +}
> +
>  void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
>  {
>         int i;
>         struct cper_ia_err_info *err_info;
>         char newpfx[64];
> +       enum err_types err_type;
>

Can you make this an u8 please? The signedness of an enum is not well
defined, and you only check the upper bound below.

>         printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
>
> @@ -46,6 +82,11 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
>                 printk("%sError Structure Type: %pUl\n", newpfx,
>                          &err_info->err_type);
>
> +               err_type = cper_get_err_type(&err_info->err_type);
> +               printk("%sError Structure Type: %s\n", newpfx,
> +                        err_type < ARRAY_SIZE(cper_proc_error_type_strs) ?
> +                        cper_proc_error_type_strs[err_type] : "unknown");
> +
>                 printk("%sValidation Bits: 0x%016llx\n",
>                          newpfx, err_info->validation_bits);
>
> --
> 2.14.1
>

Re: [PATCH 5/8] efi: Decode IA32/X64 Cache, TLB, and Bus Check structures

[Ard Biesheuvel]

On 23 February 2018 at 20:03, Yazen Ghannam <Yazen.Ghannam@amd.com> wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> Print the common fields of the Cache, TLB, and Bus check structures.The
> fields of these three check types are the same except for a few more
> fields in the Bus check structure. The remaining Bus check structure
> fields will be decoded in a following patch.
>
> Based on UEFI 2.7,
> Table 257. IA32/X64 Cache Check Structure
> Table 258. IA32/X64 TLB Check Structure
> Table 259. IA32/X64 Bus Check Structure
>
> Cc: <stable@vger.kernel.org> # 4.16.x
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  drivers/firmware/efi/cper-x86.c | 101 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 100 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> index 3b86223bdb67..75f664043076 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -33,6 +33,25 @@
>  #define INFO_VALID_RESPONDER_ID                BIT_ULL(3)
>  #define INFO_VALID_IP                  BIT_ULL(4)
>
> +#define CHECK_VALID_TRANS_TYPE         BIT_ULL(0)
> +#define CHECK_VALID_OPERATION          BIT_ULL(1)
> +#define CHECK_VALID_LEVEL              BIT_ULL(2)
> +#define CHECK_VALID_PCC                        BIT_ULL(3)
> +#define CHECK_VALID_UNCORRECTED                BIT_ULL(4)
> +#define CHECK_VALID_PRECISE_IP         BIT_ULL(5)
> +#define CHECK_VALID_RESTARTABLE_IP     BIT_ULL(6)
> +#define CHECK_VALID_OVERFLOW           BIT_ULL(7)
> +
> +#define CHECK_VALID_BITS(check)                ((check & GENMASK_ULL(15, 0)))
> +#define CHECK_TRANS_TYPE(check)                ((check & GENMASK_ULL(17, 16)) >> 16)
> +#define CHECK_OPERATION(check)         ((check & GENMASK_ULL(21, 18)) >> 18)
> +#define CHECK_LEVEL(check)             ((check & GENMASK_ULL(24, 22)) >> 22)

Parens around 'check' please

> +#define CHECK_PCC                      BIT_ULL(25)
> +#define CHECK_UNCORRECTED              BIT_ULL(26)
> +#define CHECK_PRECISE_IP               BIT_ULL(27)
> +#define CHECK_RESTARTABLE_IP           BIT_ULL(28)
> +#define CHECK_OVERFLOW                 BIT_ULL(29)
> +
>  enum err_types {
>         ERR_TYPE_CACHE = 0,
>         ERR_TYPE_TLB,
> @@ -55,11 +74,83 @@ static enum err_types cper_get_err_type(const guid_t *err_type)
>                 return N_ERR_TYPES;
>  }
>
> +static const char * const ia_check_trans_type_strs[] = {
> +       "Instruction",
> +       "Data Access",
> +       "Generic",
> +};
> +
> +static const char * const ia_check_op_strs[] = {
> +       "generic error",
> +       "generic read",
> +       "generic write",
> +       "data read",
> +       "data write",
> +       "instruction fetch",
> +       "prefetch",
> +       "eviction",
> +       "snoop",
> +};
> +
> +static inline void print_bool(char *str, const char *pfx, u64 check, u64 bit)
> +{
> +       printk("%s%s: %s\n", pfx, str, (check & bit) ? "true" : "false");
> +}
> +
> +static void print_err_info(const char *pfx, enum err_types err_type, u64 check)

u8 for err_type please

> +{
> +       u16 validation_bits = CHECK_VALID_BITS(check);
> +
> +       printk("%sValidation Bits: 0x%04x\n", pfx, validation_bits);
> +
> +       if (err_type == ERR_TYPE_MS)
> +               return;
> +
> +       if (validation_bits & CHECK_VALID_TRANS_TYPE) {
> +               u8 trans_type = CHECK_TRANS_TYPE(check);
> +
> +               printk("%sTransaction Type: %u, %s\n", pfx, trans_type,
> +                        trans_type < ARRAY_SIZE(ia_check_trans_type_strs) ?
> +                        ia_check_trans_type_strs[trans_type] : "unknown");
> +       }
> +
> +       if (validation_bits & CHECK_VALID_OPERATION) {
> +               u8 op = CHECK_OPERATION(check);
> +
> +               /*
> +                * CACHE has more operation types than TLB or BUS, though the
> +                * name and the order are the same.
> +                */
> +               u8 max_ops = (err_type == ERR_TYPE_CACHE) ? 9 : 7;
> +
> +               printk("%sOperation: %u, %s\n", pfx, op,
> +                        op < max_ops ? ia_check_op_strs[op] : "unknown");
> +       }
> +
> +       if (validation_bits & CHECK_VALID_LEVEL)
> +               printk("%sLevel: %llu\n", pfx, CHECK_LEVEL(check));
> +
> +       if (validation_bits & CHECK_VALID_PCC)
> +               print_bool("Processor Context Corrupt", pfx, check, CHECK_PCC);
> +
> +       if (validation_bits & CHECK_VALID_UNCORRECTED)
> +               print_bool("Uncorrected", pfx, check, CHECK_UNCORRECTED);
> +
> +       if (validation_bits & CHECK_VALID_PRECISE_IP)
> +               print_bool("Precise IP", pfx, check, CHECK_PRECISE_IP);
> +
> +       if (validation_bits & CHECK_VALID_RESTARTABLE_IP)
> +               print_bool("Restartable IP", pfx, check, CHECK_RESTARTABLE_IP);
> +
> +       if (validation_bits & CHECK_VALID_OVERFLOW)
> +               print_bool("Overflow", pfx, check, CHECK_OVERFLOW);
> +}
> +
>  void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
>  {
>         int i;
>         struct cper_ia_err_info *err_info;
> -       char newpfx[64];
> +       char newpfx[64], infopfx[64];
>         enum err_types err_type;
>
>         printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
> @@ -93,6 +184,14 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
>                 if (err_info->validation_bits & INFO_VALID_CHECK_INFO) {
>                         printk("%sCheck Information: 0x%016llx\n", newpfx,
>                                  err_info->check_info);
> +
> +                       if (err_type < N_ERR_TYPES) {
> +                               snprintf(infopfx, sizeof(infopfx), "%s%s",
> +                                        newpfx, INDENT_SP);
> +
> +                               print_err_info(infopfx, err_type,
> +                                              err_info->check_info);
> +                       }
>                 }
>
>                 if (err_info->validation_bits & INFO_VALID_TARGET_ID) {
> --
> 2.14.1
>

Re: [PATCH 6/8] efi: Decode additional IA32/X64 Bus Check fields

[Ard Biesheuvel]

On 23 February 2018 at 20:03, Yazen Ghannam <Yazen.Ghannam@amd.com> wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> The "Participation Type", "Time Out", and "Address Space" fields are
> unique to the IA32/X64 Bus Check structure. Print these fields.
>
> Based on UEFI 2.7 Table 259. IA32/X64 Bus Check Structure
>
> Cc: <stable@vger.kernel.org> # 4.16.x
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  drivers/firmware/efi/cper-x86.c | 44 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> index 75f664043076..df4cf6221d8e 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -42,6 +42,10 @@
>  #define CHECK_VALID_RESTARTABLE_IP     BIT_ULL(6)
>  #define CHECK_VALID_OVERFLOW           BIT_ULL(7)
>
> +#define CHECK_VALID_BUS_PART_TYPE      BIT_ULL(8)
> +#define CHECK_VALID_BUS_TIME_OUT       BIT_ULL(9)
> +#define CHECK_VALID_BUS_ADDR_SPACE     BIT_ULL(10)
> +
>  #define CHECK_VALID_BITS(check)                ((check & GENMASK_ULL(15, 0)))
>  #define CHECK_TRANS_TYPE(check)                ((check & GENMASK_ULL(17, 16)) >> 16)
>  #define CHECK_OPERATION(check)         ((check & GENMASK_ULL(21, 18)) >> 18)
> @@ -52,6 +56,10 @@
>  #define CHECK_RESTARTABLE_IP           BIT_ULL(28)
>  #define CHECK_OVERFLOW                 BIT_ULL(29)
>
> +#define CHECK_BUS_PART_TYPE(check)     ((check & GENMASK_ULL(31, 30)) >> 30)
> +#define CHECK_BUS_TIME_OUT             BIT_ULL(32)
> +#define CHECK_BUS_ADDR_SPACE(check)    ((check & GENMASK_ULL(34, 33)) >> 33)
> +

Parens around 'check' please

>  enum err_types {
>         ERR_TYPE_CACHE = 0,
>         ERR_TYPE_TLB,
> @@ -92,6 +100,20 @@ static const char * const ia_check_op_strs[] = {
>         "snoop",
>  };
>
> +static const char * const ia_check_bus_part_type_strs[] = {
> +       "Local Processor originated request",
> +       "Local Processor responded to request",
> +       "Local Processor observed",
> +       "Generic",
> +};
> +
> +static const char * const ia_check_bus_addr_space_strs[] = {
> +       "Memory Access",
> +       "Reserved",
> +       "I/O",
> +       "Other Transaction",
> +};
> +
>  static inline void print_bool(char *str, const char *pfx, u64 check, u64 bit)
>  {
>         printk("%s%s: %s\n", pfx, str, (check & bit) ? "true" : "false");
> @@ -144,6 +166,28 @@ static void print_err_info(const char *pfx, enum err_types err_type, u64 check)
>
>         if (validation_bits & CHECK_VALID_OVERFLOW)
>                 print_bool("Overflow", pfx, check, CHECK_OVERFLOW);
> +
> +       if (err_type != ERR_TYPE_BUS)
> +               return;
> +
> +       if (validation_bits & CHECK_VALID_BUS_PART_TYPE) {
> +               u8 part_type = CHECK_BUS_PART_TYPE(check);
> +
> +               printk("%sParticipation Type: %u, %s\n", pfx, part_type,
> +                        part_type < ARRAY_SIZE(ia_check_bus_part_type_strs) ?
> +                        ia_check_bus_part_type_strs[part_type] : "unknown");
> +       }
> +
> +       if (validation_bits & CHECK_VALID_BUS_TIME_OUT)
> +               print_bool("Time Out", pfx, check, CHECK_BUS_TIME_OUT);
> +
> +       if (validation_bits & CHECK_VALID_BUS_ADDR_SPACE) {
> +               u8 addr_space = CHECK_BUS_ADDR_SPACE(check);
> +
> +               printk("%sAddress Space: %u, %s\n", pfx, addr_space,
> +                        addr_space < ARRAY_SIZE(ia_check_bus_addr_space_strs) ?
> +                        ia_check_bus_addr_space_strs[addr_space] : "unknown");
> +       }
>  }
>
>  void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
> --
> 2.14.1
>

Re: [PATCH 7/8] efi: Decode IA32/X64 MS Check structure

[Ard Biesheuvel]

On 23 February 2018 at 20:03, Yazen Ghannam <Yazen.Ghannam@amd.com> wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> The IA32/X64 MS Check structure varies from the other Check structures
> in the the bit positions of its fields, and it includes an additional
> "Error Type" field.
>
> Decode the MS Check structure in a separate function.
>
> Based on UEFI 2.7 Table 260. IA32/X64 MS Check Field Description.
>
> Cc: <stable@vger.kernel.org> # 4.16.x
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  drivers/firmware/efi/cper-x86.c | 55 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> index df4cf6221d8e..02b1b424f537 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -60,6 +60,20 @@
>  #define CHECK_BUS_TIME_OUT             BIT_ULL(32)
>  #define CHECK_BUS_ADDR_SPACE(check)    ((check & GENMASK_ULL(34, 33)) >> 33)
>
> +#define CHECK_VALID_MS_ERR_TYPE                BIT_ULL(0)
> +#define CHECK_VALID_MS_PCC             BIT_ULL(1)
> +#define CHECK_VALID_MS_UNCORRECTED     BIT_ULL(2)
> +#define CHECK_VALID_MS_PRECISE_IP      BIT_ULL(3)
> +#define CHECK_VALID_MS_RESTARTABLE_IP  BIT_ULL(4)
> +#define CHECK_VALID_MS_OVERFLOW                BIT_ULL(5)
> +
> +#define CHECK_MS_ERR_TYPE(check)       ((check & GENMASK_ULL(18, 16)) >> 16)

Parens

> +#define CHECK_MS_PCC                   BIT_ULL(19)
> +#define CHECK_MS_UNCORRECTED           BIT_ULL(20)
> +#define CHECK_MS_PRECISE_IP            BIT_ULL(21)
> +#define CHECK_MS_RESTARTABLE_IP                BIT_ULL(22)
> +#define CHECK_MS_OVERFLOW              BIT_ULL(23)
> +
>  enum err_types {
>         ERR_TYPE_CACHE = 0,
>         ERR_TYPE_TLB,
> @@ -114,19 +128,58 @@ static const char * const ia_check_bus_addr_space_strs[] = {
>         "Other Transaction",
>  };
>
> +static const char * const ia_check_ms_error_type_strs[] = {
> +       "No Error",
> +       "Unclassified",
> +       "Microcode ROM Parity Error",
> +       "External Error",
> +       "FRC Error",
> +       "Internal Unclassified",
> +};
> +
>  static inline void print_bool(char *str, const char *pfx, u64 check, u64 bit)
>  {
>         printk("%s%s: %s\n", pfx, str, (check & bit) ? "true" : "false");
>  }
>
> +static void print_err_info_ms(const char *pfx, u16 validation_bits, u64 check)
> +{
> +       if (validation_bits & CHECK_VALID_MS_ERR_TYPE) {
> +               u8 err_type = CHECK_MS_ERR_TYPE(check);
> +
> +               printk("%sError Type: %u, %s\n", pfx, err_type,
> +                        err_type < ARRAY_SIZE(ia_check_ms_error_type_strs) ?
> +                        ia_check_ms_error_type_strs[err_type] : "unknown");
> +       }
> +
> +       if (validation_bits & CHECK_VALID_MS_PCC)
> +               print_bool("Processor Context Corrupt", pfx, check, CHECK_MS_PCC);
> +
> +       if (validation_bits & CHECK_VALID_MS_UNCORRECTED)
> +               print_bool("Uncorrected", pfx, check, CHECK_MS_UNCORRECTED);
> +
> +       if (validation_bits & CHECK_VALID_MS_PRECISE_IP)
> +               print_bool("Precise IP", pfx, check, CHECK_MS_PRECISE_IP);
> +
> +       if (validation_bits & CHECK_VALID_MS_RESTARTABLE_IP)
> +               print_bool("Restartable IP", pfx, check, CHECK_MS_RESTARTABLE_IP);
> +
> +       if (validation_bits & CHECK_VALID_MS_OVERFLOW)
> +               print_bool("Overflow", pfx, check, CHECK_MS_OVERFLOW);
> +}
> +
>  static void print_err_info(const char *pfx, enum err_types err_type, u64 check)
>  {
>         u16 validation_bits = CHECK_VALID_BITS(check);
>
>         printk("%sValidation Bits: 0x%04x\n", pfx, validation_bits);
>
> +       /*
> +        * The MS Check structure varies a lot from the others, so use a
> +        * separate function for decoding.
> +        */
>         if (err_type == ERR_TYPE_MS)
> -               return;
> +               return print_err_info_ms(pfx, validation_bits, check);
>
>         if (validation_bits & CHECK_VALID_TRANS_TYPE) {
>                 u8 trans_type = CHECK_TRANS_TYPE(check);
> --
> 2.14.1
>

Re: [PATCH 8/8] efi: Decode IA32/X64 Context Info structure

[Ard Biesheuvel]

On 23 February 2018 at 20:03, Yazen Ghannam <Yazen.Ghannam@amd.com> wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> Print the fields of the IA32/X64 Context Information structure.
>
> Print the "Register Array" as raw values. Some context types are defined
> in the UEFI spec, so more detailed decoded may be added in the future.
>
> Based on UEFI 2.7 section N.2.4.2.2 IA32/X64 Processor Context
> Information Structure.
>
> Cc: <stable@vger.kernel.org> # 4.16.x
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  drivers/firmware/efi/cper-x86.c | 55 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> index 02b1b424f537..bb6cef0b5e53 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -13,6 +13,7 @@
>  #define VALID_LAPIC_ID                 BIT_ULL(0)
>  #define VALID_CPUID_INFO               BIT_ULL(1)
>  #define VALID_PROC_ERR_INFO_NUM(bits)  ((bits & GENMASK_ULL(7, 2)) >> 2)
> +#define VALID_PROC_CNXT_INFO_NUM(bits) ((bits & GENMASK_ULL(13, 8)) >> 8)

Parens

Also, CNXT isn't really idiomatic when abbreviating 'context' (and you
use CTX below as well)

>
>  #define INFO_ERR_STRUCT_TYPE_CACHE                                     \
>         GUID_INIT(0xA55701F5, 0xE3EF, 0x43DE, 0xAC, 0x72, 0x24, 0x9B,   \
> @@ -74,6 +75,9 @@
>  #define CHECK_MS_RESTARTABLE_IP                BIT_ULL(22)
>  #define CHECK_MS_OVERFLOW              BIT_ULL(23)
>
> +#define CTX_TYPE_MSR                   1
> +#define CTX_TYPE_MMREG                 7
> +
>  enum err_types {
>         ERR_TYPE_CACHE = 0,
>         ERR_TYPE_TLB,
> @@ -137,6 +141,17 @@ static const char * const ia_check_ms_error_type_strs[] = {
>         "Internal Unclassified",
>  };
>
> +static const char * const ia_reg_ctx_strs[] = {
> +       "Unclassified Data",
> +       "MSR Registers (Machine Check and other MSRs)",
> +       "32-bit Mode Execution Context",
> +       "64-bit Mode Execution Context",
> +       "FXSAVE Context",
> +       "32-bit Mode Debug Registers (DR0-DR7)",
> +       "64-bit Mode Debug Registers (DR0-DR7)",
> +       "Memory Mapped Registers",
> +};
> +
>  static inline void print_bool(char *str, const char *pfx, u64 check, u64 bit)
>  {
>         printk("%s%s: %s\n", pfx, str, (check & bit) ? "true" : "false");
> @@ -247,8 +262,10 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
>  {
>         int i;
>         struct cper_ia_err_info *err_info;
> +       struct cper_ia_proc_ctx *ctx_info;
>         char newpfx[64], infopfx[64];
>         enum err_types err_type;
> +       unsigned int max_ctx_type = ARRAY_SIZE(ia_reg_ctx_strs) - 1;
>
>         printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
>
> @@ -313,4 +330,42 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
>
>                 err_info++;
>         }
> +
> +       ctx_info = (struct cper_ia_proc_ctx *)err_info;
> +       for (i = 0; i < VALID_PROC_CNXT_INFO_NUM(proc->validation_bits); i++) {
> +               int size = sizeof(*ctx_info) + ctx_info->reg_arr_size;
> +               int groupsize = 4;
> +
> +               printk("%sContext Information Structure %d:\n", pfx, i);
> +
> +               if (ctx_info->reg_ctx_type > max_ctx_type) {
> +                       printk("%sInvalid Register Context Type: %d (max: %d)\n",
> +                                newpfx, ctx_info->reg_ctx_type, max_ctx_type);
> +                       goto next_ctx;
> +               }
> +
> +               printk("%sRegister Context Type: %s\n", newpfx,
> +                        ia_reg_ctx_strs[ctx_info->reg_ctx_type]);
> +
> +               printk("%sRegister Array Size: 0x%04x\n", newpfx,
> +                        ctx_info->reg_arr_size);
> +
> +               if (ctx_info->reg_ctx_type == CTX_TYPE_MSR) {
> +                       groupsize = 8; /* MSRs are 8 bytes wide. */
> +                       printk("%sMSR Address: 0x%08x\n", newpfx,
> +                                ctx_info->msr_addr);
> +               }
> +
> +               if (ctx_info->reg_ctx_type == CTX_TYPE_MMREG) {
> +                       printk("%sMM Register Address: 0x%016llx\n", newpfx,
> +                                ctx_info->mm_reg_addr);
> +               }
> +
> +               printk("%sRegister Array:\n", newpfx);
> +               print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, groupsize,
> +                              (ctx_info + 1), ctx_info->reg_arr_size, 0);
> +
> +next_ctx:
> +               ctx_info = (struct cper_ia_proc_ctx *)((long)ctx_info + size);
> +       }
>  }
> --
> 2.14.1
>

Re: [PATCH 0/8] Decode IA32/X64 CPER

[Ard Biesheuvel]

Hi Yazen,

On 23 February 2018 at 20:03, Yazen Ghannam <Yazen.Ghannam@amd.com> wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> This series adds decoding for the IA32/X64 Common Platform Error Record.
>
> Patch 1 fixes the IA32/X64 Processor Error Section definition to match
> the UEFI spec.
>
> Patches 2-8 add the new decoding. The patches incrementally add the
> decoding starting from the top-level "Error Section". Hopefully, this
> will make reviewing a bit easier compared to one large patch.
>
> The formatting of the field names and options is taken from the UEFI
> spec. I tried to keep everything the same to make searching easier.
>
> The patches were written to the UEFI 2.7 spec though the definition of
> the IA32/X64 CPER seems to be the same as when it was introduced in
> the UEFI 2.1 spec.
>
> Without basic decoding, users will be confused about what these
> "Hardware Errors" mean. So I'm requesting this set to be applied to the
> stable branches. This set applies to the v4.16. However, patch 2 will
> have a conflict on older branches, so I'll send this set again with the
> conflict fixed.
>

These patches look mostly fine to me, with the exception of some minor nits.

I can queue this for v4.17 if you respin it, but I am not sending 400
lines of brand new error record parsing code to Greg for inclusion in
-stable, so please drop the cc stable tags

Thanks,
Ard.

RE: [PATCH 0/8] Decode IA32/X64 CPER

[Ghannam, Yazen]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Saturday, February 24, 2018 11:47 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-efi@vger.kernel.org; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Borislav Petkov <bp@suse.de>; the arch/x86
> maintainers <x86@kernel.org>
> Subject: Re: [PATCH 0/8] Decode IA32/X64 CPER
> 
> Hi Yazen,
> 
> On 23 February 2018 at 20:03, Yazen Ghannam
> <Yazen.Ghannam@amd.com> wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > This series adds decoding for the IA32/X64 Common Platform Error Record.
> >
> > Patch 1 fixes the IA32/X64 Processor Error Section definition to match
> > the UEFI spec.
> >
> > Patches 2-8 add the new decoding. The patches incrementally add the
> > decoding starting from the top-level "Error Section". Hopefully, this
> > will make reviewing a bit easier compared to one large patch.
> >
> > The formatting of the field names and options is taken from the UEFI
> > spec. I tried to keep everything the same to make searching easier.
> >
> > The patches were written to the UEFI 2.7 spec though the definition of
> > the IA32/X64 CPER seems to be the same as when it was introduced in
> > the UEFI 2.1 spec.
> >
> > Without basic decoding, users will be confused about what these
> > "Hardware Errors" mean. So I'm requesting this set to be applied to the
> > stable branches. This set applies to the v4.16. However, patch 2 will
> > have a conflict on older branches, so I'll send this set again with the
> > conflict fixed.
> >
> 
> These patches look mostly fine to me, with the exception of some minor nits.
> 
> I can queue this for v4.17 if you respin it, but I am not sending 400
> lines of brand new error record parsing code to Greg for inclusion in
> -stable, so please drop the cc stable tags
> 

Okay, will do.

Thanks,
Yazen

RE: [PATCH 2/8] efi: Decode IA32/X64 Processor Error Section

[Ghannam, Yazen]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Ard Biesheuvel
> Sent: Saturday, February 24, 2018 11:39 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-efi@vger.kernel.org; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Borislav Petkov <bp@suse.de>; the arch/x86
> maintainers <x86@kernel.org>
> Subject: Re: [PATCH 2/8] efi: Decode IA32/X64 Processor Error Section
> 
...
> >
> > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > index 3098410abad8..8b684147835d 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -174,6 +174,11 @@ config UEFI_CPER_ARM
> >         depends on UEFI_CPER && ( ARM || ARM64 )
> >         default y
> >
> > +config UEFI_CPER_X86
> > +       bool
> > +       depends on UEFI_CPER && ( X86_32 || X86_64 )
> 
> Just 'UEFI_CPER && X86' should be sufficient, no?
> 

Yes.

> > +       default y
> > +
> >  config EFI_DEV_PATH_PARSER
> >         bool
> >         depends on ACPI
> > diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> > index cb805374f4bc..7cf8d75ebe51 100644
> > --- a/drivers/firmware/efi/Makefile
> > +++ b/drivers/firmware/efi/Makefile
> > @@ -31,3 +31,5 @@ obj-$(CONFIG_ARM)                     += $(arm-obj-y)
> >  obj-$(CONFIG_ARM64)                    += $(arm-obj-y)
> >  obj-$(CONFIG_EFI_CAPSULE_LOADER)       += capsule-loader.o
> >  obj-$(CONFIG_UEFI_CPER_ARM)            += cper-arm.o
> > +
> 
> Spurious newline
> 

Okay.

> > +obj-$(CONFIG_UEFI_CPER_X86)            += cper-x86.o
> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-
> x86.c
> > new file mode 100644
> > index 000000000000..b50ee3cdf637
> > --- /dev/null
> > +++ b/drivers/firmware/efi/cper-x86.c
> > @@ -0,0 +1,26 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018, Advanced Micro Devices, Inc.
> > +// Copyright (C) 2018, Yazen Ghannam <yazen.ghannam@amd.com>
> > +
> 
> Do you really own the copyright for code that you write on behalf of
> your employer?
> 

I'll drop that line.

> > +#include <linux/cper.h>
> > +
> > +/*
> > + * We don't need a "CPER_IA" prefix since these are all locally defined.
> > + * This will save us a lot of line space.
> > + */
> > +#define VALID_LAPIC_ID                 BIT_ULL(0)
> > +#define VALID_CPUID_INFO               BIT_ULL(1)
> > +
> > +void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia
> *proc)
> > +{
> > +       printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
> > +
> > +       if (proc->validation_bits & VALID_LAPIC_ID)
> > +               printk("%sLocal APIC_ID: 0x%llx\n", pfx, proc->lapic_id);
> > +
> > +       if (proc->validation_bits & VALID_CPUID_INFO) {
> > +               printk("%sCPUID Info:\n", pfx);
> > +               print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc-
> >cpuid,
> > +                              sizeof(proc->cpuid), 0);
> > +       }
> > +}
> > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> > index c165933ebf38..5a59b582c9aa 100644
> > --- a/drivers/firmware/efi/cper.c
> > +++ b/drivers/firmware/efi/cper.c
> > @@ -469,6 +469,16 @@ cper_estatus_print_section(const char *pfx, struct
> acpi_hest_generic_data *gdata
> >                         cper_print_proc_arm(newpfx, arm_err);
> >                 else
> >                         goto err_section_too_small;
> > +#endif
> > +#if defined(CONFIG_UEFI_CPER_X86)
> > +       } else if (guid_equal(sec_type, &CPER_SEC_PROC_IA)) {
> > +               struct cper_sec_proc_ia *ia_err = acpi_hest_get_payload(gdata);
> > +
> > +               printk("%ssection_type: IA32/X64 processor error\n", newpfx);
> > +               if (gdata->error_data_length >= sizeof(*ia_err))
> > +                       cper_print_proc_ia(newpfx, ia_err);
> > +               else
> > +                       goto err_section_too_small;
> >  #endif
> >         } else {
> >                 const void *err = acpi_hest_get_payload(gdata);
> > diff --git a/include/linux/cper.h b/include/linux/cper.h
> > index 4b5f8459b403..9c703a0abe6e 100644
> > --- a/include/linux/cper.h
> > +++ b/include/linux/cper.h
> > @@ -551,5 +551,7 @@ const char *cper_mem_err_unpack(struct
> trace_seq *,
> >                                 struct cper_mem_err_compact *);
> >  void cper_print_proc_arm(const char *pfx,
> >                          const struct cper_sec_proc_arm *proc);
> > +void cper_print_proc_ia(const char *pfx,
> > +                       const struct cper_sec_proc_ia *proc);
> >
> >  #endif
> > --
> > 2.14.1
> >

RE: [PATCH 3/8] efi: Decode IA32/X64 Processor Error Info Structure

[Ghannam, Yazen]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Saturday, February 24, 2018 11:40 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-efi@vger.kernel.org; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Borislav Petkov <bp@suse.de>; the arch/x86
> maintainers <x86@kernel.org>
> Subject: Re: [PATCH 3/8] efi: Decode IA32/X64 Processor Error Info Structure
> 
...
> >
> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-
> x86.c
> > index b50ee3cdf637..9d608f742c98 100644
> > --- a/drivers/firmware/efi/cper-x86.c
> > +++ b/drivers/firmware/efi/cper-x86.c
> > @@ -4,15 +4,28 @@
> >
> >  #include <linux/cper.h>
> >
> > +#define INDENT_SP      " "
> > +
> >  /*
> >   * We don't need a "CPER_IA" prefix since these are all locally defined.
> >   * This will save us a lot of line space.
> >   */
> >  #define VALID_LAPIC_ID                 BIT_ULL(0)
> >  #define VALID_CPUID_INFO               BIT_ULL(1)
> > +#define VALID_PROC_ERR_INFO_NUM(bits)  ((bits & GENMASK_ULL(7, 2))
> >> 2)
> > +
> 
> Parens around 'bits' please
> 

Like this?

#define VALID_PROC_ERR_INFO_NUM(bits)  (((bits) & GENMASK_ULL(7, 2)) >> 2)

I'll do the same for the others.

> > +#define INFO_VALID_CHECK_INFO          BIT_ULL(0)
> > +#define INFO_VALID_TARGET_ID           BIT_ULL(1)
> > +#define INFO_VALID_REQUESTOR_ID                BIT_ULL(2)
> > +#define INFO_VALID_RESPONDER_ID                BIT_ULL(3)
> > +#define INFO_VALID_IP                  BIT_ULL(4)
> >
> >  void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia
> *proc)
> >  {
> > +       int i;
> > +       struct cper_ia_err_info *err_info;
> > +       char newpfx[64];
> > +
> >         printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
> >
> >         if (proc->validation_bits & VALID_LAPIC_ID)
> > @@ -23,4 +36,44 @@ void cper_print_proc_ia(const char *pfx, const struct
> cper_sec_proc_ia *proc)
> >                 print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc-
> >cpuid,
> >                                sizeof(proc->cpuid), 0);
> >         }
> > +
> > +       snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> > +
> > +       err_info = (struct cper_ia_err_info *)(proc + 1);
> > +       for (i = 0; i < VALID_PROC_ERR_INFO_NUM(proc->validation_bits); i++)
> {
> > +               printk("%sError Information Structure %d:\n", pfx, i);
> > +
> > +               printk("%sError Structure Type: %pUl\n", newpfx,
> > +                        &err_info->err_type);
> > +
> 
> The indentation is a bit awkward here. Could you please align followup
> lines with the character following the ( on the first line?
> 

Yes, will do.

> > +               printk("%sValidation Bits: 0x%016llx\n",
> > +                        newpfx, err_info->validation_bits);
> > +
> > +               if (err_info->validation_bits & INFO_VALID_CHECK_INFO) {
> > +                       printk("%sCheck Information: 0x%016llx\n", newpfx,
> > +                                err_info->check_info);
> > +               }
> > +
> > +               if (err_info->validation_bits & INFO_VALID_TARGET_ID) {
> > +                       printk("%sTarget Identifier: 0x%016llx\n",
> > +                                newpfx, err_info->target_id);
> > +               }
> > +
> > +               if (err_info->validation_bits & INFO_VALID_REQUESTOR_ID) {
> > +                       printk("%sRequestor Identifier: 0x%016llx\n",
> > +                                newpfx, err_info->requestor_id);
> > +               }
> > +
> > +               if (err_info->validation_bits & INFO_VALID_RESPONDER_ID) {
> > +                       printk("%sResponder Identifier: 0x%016llx\n",
> > +                                newpfx, err_info->responder_id);
> > +               }
> > +
> > +               if (err_info->validation_bits & INFO_VALID_IP) {
> > +                       printk("%sInstruction Pointer: 0x%016llx\n",
> > +                                newpfx, err_info->ip);
> > +               }
> > +
> > +               err_info++;
> > +       }
> >  }
> > --
> > 2.14.1
> >

RE: [PATCH 4/8] efi: Decode UEFI-defined IA32/X64 Error Structure GUIDs

[Ghannam, Yazen]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Saturday, February 24, 2018 11:41 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-efi@vger.kernel.org; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Borislav Petkov <bp@suse.de>; the arch/x86
> maintainers <x86@kernel.org>
> Subject: Re: [PATCH 4/8] efi: Decode UEFI-defined IA32/X64 Error Structure
> GUIDs
> 
> On 23 February 2018 at 20:03, Yazen Ghannam
> <Yazen.Ghannam@amd.com> wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > For easier handling, match the known IA32/X64 error structure GUIDs to
> > enums.
> >
> > Also, print out the name of the matching Error Structure Type.
> >
> > GUIDs taken from UEFI 2.7 section N.2.4.2.1 IA32/X64 Processor Error
> > Information Structure.
> >
> > Cc: <stable@vger.kernel.org> # 4.16.x
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> >  drivers/firmware/efi/cper-x86.c | 41
> +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-
> x86.c
> > index 9d608f742c98..3b86223bdb67 100644
> > --- a/drivers/firmware/efi/cper-x86.c
> > +++ b/drivers/firmware/efi/cper-x86.c
> > @@ -14,17 +14,53 @@
...
> >  void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia
> *proc)
> >  {
> >         int i;
> >         struct cper_ia_err_info *err_info;
> >         char newpfx[64];
> > +       enum err_types err_type;
> >
> 
> Can you make this an u8 please? The signedness of an enum is not well
> defined, and you only check the upper bound below.
> 

Yes, will do.

Thanks,
Yazen

Re: [PATCH 3/8] efi: Decode IA32/X64 Processor Error Info Structure

[Ard Biesheuvel]

On 26 February 2018 at 16:00, Ghannam, Yazen <Yazen.Ghannam@amd.com> wrote:
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Saturday, February 24, 2018 11:40 AM
>> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
>> Cc: linux-efi@vger.kernel.org; Linux Kernel Mailing List <linux-
>> kernel@vger.kernel.org>; Borislav Petkov <bp@suse.de>; the arch/x86
>> maintainers <x86@kernel.org>
>> Subject: Re: [PATCH 3/8] efi: Decode IA32/X64 Processor Error Info Structure
>>
> ...
>> >
>> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-
>> x86.c
>> > index b50ee3cdf637..9d608f742c98 100644
>> > --- a/drivers/firmware/efi/cper-x86.c
>> > +++ b/drivers/firmware/efi/cper-x86.c
>> > @@ -4,15 +4,28 @@
>> >
>> >  #include <linux/cper.h>
>> >
>> > +#define INDENT_SP      " "
>> > +
>> >  /*
>> >   * We don't need a "CPER_IA" prefix since these are all locally defined.
>> >   * This will save us a lot of line space.
>> >   */
>> >  #define VALID_LAPIC_ID                 BIT_ULL(0)
>> >  #define VALID_CPUID_INFO               BIT_ULL(1)
>> > +#define VALID_PROC_ERR_INFO_NUM(bits)  ((bits & GENMASK_ULL(7, 2))
>> >> 2)
>> > +
>>
>> Parens around 'bits' please
>>
>
> Like this?
>
> #define VALID_PROC_ERR_INFO_NUM(bits)  (((bits) & GENMASK_ULL(7, 2)) >> 2)
>

Yes. Your code currently does not pass expressions into these, but it
is good form to use parens here to make them future proof

> I'll do the same for the others.
>

Cheers

>> > +#define INFO_VALID_CHECK_INFO          BIT_ULL(0)
>> > +#define INFO_VALID_TARGET_ID           BIT_ULL(1)
>> > +#define INFO_VALID_REQUESTOR_ID                BIT_ULL(2)
>> > +#define INFO_VALID_RESPONDER_ID                BIT_ULL(3)
>> > +#define INFO_VALID_IP                  BIT_ULL(4)
>> >
>> >  void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia
>> *proc)
>> >  {
>> > +       int i;
>> > +       struct cper_ia_err_info *err_info;
>> > +       char newpfx[64];
>> > +
>> >         printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
>> >
>> >         if (proc->validation_bits & VALID_LAPIC_ID)
>> > @@ -23,4 +36,44 @@ void cper_print_proc_ia(const char *pfx, const struct
>> cper_sec_proc_ia *proc)
>> >                 print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc-
>> >cpuid,
>> >                                sizeof(proc->cpuid), 0);
>> >         }
>> > +
>> > +       snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
>> > +
>> > +       err_info = (struct cper_ia_err_info *)(proc + 1);
>> > +       for (i = 0; i < VALID_PROC_ERR_INFO_NUM(proc->validation_bits); i++)
>> {
>> > +               printk("%sError Information Structure %d:\n", pfx, i);
>> > +
>> > +               printk("%sError Structure Type: %pUl\n", newpfx,
>> > +                        &err_info->err_type);
>> > +
>>
>> The indentation is a bit awkward here. Could you please align followup
>> lines with the character following the ( on the first line?
>>
>
> Yes, will do.
>
>> > +               printk("%sValidation Bits: 0x%016llx\n",
>> > +                        newpfx, err_info->validation_bits);
>> > +
>> > +               if (err_info->validation_bits & INFO_VALID_CHECK_INFO) {
>> > +                       printk("%sCheck Information: 0x%016llx\n", newpfx,
>> > +                                err_info->check_info);
>> > +               }
>> > +
>> > +               if (err_info->validation_bits & INFO_VALID_TARGET_ID) {
>> > +                       printk("%sTarget Identifier: 0x%016llx\n",
>> > +                                newpfx, err_info->target_id);
>> > +               }
>> > +
>> > +               if (err_info->validation_bits & INFO_VALID_REQUESTOR_ID) {
>> > +                       printk("%sRequestor Identifier: 0x%016llx\n",
>> > +                                newpfx, err_info->requestor_id);
>> > +               }
>> > +
>> > +               if (err_info->validation_bits & INFO_VALID_RESPONDER_ID) {
>> > +                       printk("%sResponder Identifier: 0x%016llx\n",
>> > +                                newpfx, err_info->responder_id);
>> > +               }
>> > +
>> > +               if (err_info->validation_bits & INFO_VALID_IP) {
>> > +                       printk("%sInstruction Pointer: 0x%016llx\n",
>> > +                                newpfx, err_info->ip);
>> > +               }
>> > +
>> > +               err_info++;
>> > +       }
>> >  }
>> > --
>> > 2.14.1
>> >

RE: [PATCH 8/8] efi: Decode IA32/X64 Context Info structure

[Ghannam, Yazen]

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Saturday, February 24, 2018 11:46 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-efi@vger.kernel.org; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; Borislav Petkov <bp@suse.de>; the arch/x86
> maintainers <x86@kernel.org>
> Subject: Re: [PATCH 8/8] efi: Decode IA32/X64 Context Info structure
> 
> On 23 February 2018 at 20:03, Yazen Ghannam
> <Yazen.Ghannam@amd.com> wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > Print the fields of the IA32/X64 Context Information structure.
> >
> > Print the "Register Array" as raw values. Some context types are defined
> > in the UEFI spec, so more detailed decoded may be added in the future.
> >
> > Based on UEFI 2.7 section N.2.4.2.2 IA32/X64 Processor Context
> > Information Structure.
> >
> > Cc: <stable@vger.kernel.org> # 4.16.x
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> >  drivers/firmware/efi/cper-x86.c | 55
> +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-
> x86.c
> > index 02b1b424f537..bb6cef0b5e53 100644
> > --- a/drivers/firmware/efi/cper-x86.c
> > +++ b/drivers/firmware/efi/cper-x86.c
> > @@ -13,6 +13,7 @@
> >  #define VALID_LAPIC_ID                 BIT_ULL(0)
> >  #define VALID_CPUID_INFO               BIT_ULL(1)
> >  #define VALID_PROC_ERR_INFO_NUM(bits)  ((bits & GENMASK_ULL(7, 2))
> >> 2)
> > +#define VALID_PROC_CNXT_INFO_NUM(bits) ((bits & GENMASK_ULL(13,
> 8)) >> 8)
> 
> Parens
> 
> Also, CNXT isn't really idiomatic when abbreviating 'context' (and you
> use CTX below as well)
> 

The UEFI spec uses PROC_CONTEXT_INFO_NUM. 

Would you prefer 1 or 2 below?
1) VALID_PROC_CONTEXT_INFO_NUM
2) VALID_PROC_CTX_INFO_NUM

Thanks,
Yazen

Re: [PATCH 8/8] efi: Decode IA32/X64 Context Info structure

[Ard Biesheuvel]

On 26 February 2018 at 16:05, Ghannam, Yazen <Yazen.Ghannam@amd.com> wrote:
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: Saturday, February 24, 2018 11:46 AM
>> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
>> Cc: linux-efi@vger.kernel.org; Linux Kernel Mailing List <linux-
>> kernel@vger.kernel.org>; Borislav Petkov <bp@suse.de>; the arch/x86
>> maintainers <x86@kernel.org>
>> Subject: Re: [PATCH 8/8] efi: Decode IA32/X64 Context Info structure
>>
>> On 23 February 2018 at 20:03, Yazen Ghannam
>> <Yazen.Ghannam@amd.com> wrote:
>> > From: Yazen Ghannam <yazen.ghannam@amd.com>
>> >
>> > Print the fields of the IA32/X64 Context Information structure.
>> >
>> > Print the "Register Array" as raw values. Some context types are defined
>> > in the UEFI spec, so more detailed decoded may be added in the future.
>> >
>> > Based on UEFI 2.7 section N.2.4.2.2 IA32/X64 Processor Context
>> > Information Structure.
>> >
>> > Cc: <stable@vger.kernel.org> # 4.16.x
>> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
>> > ---
>> >  drivers/firmware/efi/cper-x86.c | 55
>> +++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 55 insertions(+)
>> >
>> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-
>> x86.c
>> > index 02b1b424f537..bb6cef0b5e53 100644
>> > --- a/drivers/firmware/efi/cper-x86.c
>> > +++ b/drivers/firmware/efi/cper-x86.c
>> > @@ -13,6 +13,7 @@
>> >  #define VALID_LAPIC_ID                 BIT_ULL(0)
>> >  #define VALID_CPUID_INFO               BIT_ULL(1)
>> >  #define VALID_PROC_ERR_INFO_NUM(bits)  ((bits & GENMASK_ULL(7, 2))
>> >> 2)
>> > +#define VALID_PROC_CNXT_INFO_NUM(bits) ((bits & GENMASK_ULL(13,
>> 8)) >> 8)
>>
>> Parens
>>
>> Also, CNXT isn't really idiomatic when abbreviating 'context' (and you
>> use CTX below as well)
>>
>
> The UEFI spec uses PROC_CONTEXT_INFO_NUM.
>
> Would you prefer 1 or 2 below?
> 1) VALID_PROC_CONTEXT_INFO_NUM
> 2) VALID_PROC_CTX_INFO_NUM
>

Let's go with 2) since you're already using CTX in numerous places.