[PATCH v2 1/5] RAS: Report all ARM processor CPER information to userspace

[PATCH v2 1/5] RAS: Report all ARM processor CPER information to userspace

[Mauro Carvalho Chehab]

From: Daniel Ferguson <danielf@os.amperecomputing.com>

The ARM processor CPER record was added at UEFI 2.6, and hasn't
any changes up to UEFI 2.10 on its struct.

Yet, the original arm_event trace code added on changeset
e9279e83ad1f ("trace, ras: add ARM processor error trace event") is
incomplete, as it only traces some fields of UAPI 2.6 table N.16,
not exporting at all any information from tables N.17 to N.29 of
the record.

This is not enough for user to take appropriate action or to log
what exactly happened.

According to UEFI_2_9 specification chapter N2.4.4, the ARM processor
error section includes:

- several (ERR_INFO_NUM) ARM processor error information structures
  (Tables N.17 to N.20);
- several (CONTEXT_INFO_NUM) ARM processor context information
  structures (Tables N.21 to N.29);
- several vendor specific error information structures. The
  size is given by Section Length minus the size of the other
  fields.

In addition to those data, it also exports two fields that are
parsed by the GHES driver when firmware reports it, e. g.:

- error severity
- cpu logical index

Report all of these information to userspace via trace uAPI, So that
userspace can properly record the error and take decisions related
to cpu core isolation according to error severity and other info.

After this patch, all the data from ARM Processor record from table
N.16 are directly or indirectly visible on userspace:

======================================	=============================
UEFI field on table N.16		ARM Processor trace fields
======================================	=============================
Validation				handled when filling data for
					affinity MPIDR and running
					state.
ERR_INFO_NUM				pei_len
CONTEXT_INFO_NUM			ctx_len
Section Length				indirectly reported by
					pei_len, ctx_len and oem_len
Error affinity level			affinity
MPIDR_EL1				mpidr
MIDR_EL1				midr
Running State				running_state
PSCI State				psci_state
Processor Error Information Structure	pei_err - count at pei_len
Processor Context			ctx_err- count at ctx_len
Vendor Specific Error Info		oem - count at oem_len
======================================	=============================

It should be noticed that decoding of tables N.17 to N.29, if needed,
will be handled on userspace. That gives more flexibility, as there
won't be any need to flood the Kernel with micro-architecture specific
error decoding).
Also, decoding the other fields require a complex logic, and should
be done for each of the several values inside the record field.
So, let userspace daemons like rasdaemon decode them, parsing such
tables and having vendor-specific micro-architecture-specific decoders.

[mchehab: modified patch description, solve merge conflicts and fix coding style]
Fixes: e9279e83ad1f ("trace, ras: add ARM processor error trace event")
Signed-off-by: Shengwei Luo <luoshengwei@huawei.com>
Signed-off-by: Jason Tian <jason@os.amperecomputing.com>
Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com>
Tested-by: Shiju Jose <shiju.jose@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-section
---
 drivers/acpi/apei/ghes.c | 11 ++++-----
 drivers/ras/ras.c        | 45 +++++++++++++++++++++++++++++++++++--
 include/linux/ras.h      | 16 +++++++++++---
 include/ras/ras_event.h  | 48 +++++++++++++++++++++++++++++++++++-----
 4 files changed, 103 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 623cc0cb4a65..06d9351a9abc 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -529,7 +529,7 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
 }
 
 static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
-				       int sev, bool sync)
+				     int sev, bool sync)
 {
 	struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
 	int flags = sync ? MF_ACTION_REQUIRED : 0;
@@ -537,9 +537,8 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
 	int sec_sev, i;
 	char *p;
 
-	log_arm_hw_error(err);
-
 	sec_sev = ghes_severity(gdata->error_severity);
+	log_arm_hw_error(err, sec_sev);
 	if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
 		return false;
 
@@ -773,11 +772,9 @@ static bool ghes_do_proc(struct ghes *ghes,
 
 			arch_apei_report_mem_error(sev, mem_err);
 			queued = ghes_handle_memory_failure(gdata, sev, sync);
-		}
-		else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
+		} else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
 			ghes_handle_aer(gdata);
-		}
-		else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
+		} else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
 			queued = ghes_handle_arm_hw_error(gdata, sev, sync);
 		} else if (guid_equal(sec_type, &CPER_SEC_CXL_GEN_MEDIA_GUID)) {
 			struct cxl_cper_event_rec *rec = acpi_hest_get_payload(gdata);
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index a6e4792a1b2e..359bb163aee0 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -52,9 +52,50 @@ void log_non_standard_event(const guid_t *sec_type, const guid_t *fru_id,
 	trace_non_standard_event(sec_type, fru_id, fru_text, sev, err, len);
 }
 
-void log_arm_hw_error(struct cper_sec_proc_arm *err)
+void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev)
 {
-	trace_arm_event(err);
+	struct cper_arm_err_info *err_info;
+	struct cper_arm_ctx_info *ctx_info;
+	u8 *ven_err_data;
+	u32 ctx_len = 0;
+	int n, sz, cpu;
+	s32 vsei_len;
+	u32 pei_len;
+	u8 *pei_err;
+	u8 *ctx_err;
+
+	pei_len = sizeof(struct cper_arm_err_info) * err->err_info_num;
+	pei_err = (u8 *)err + sizeof(struct cper_sec_proc_arm);
+
+	err_info = (struct cper_arm_err_info *)(err + 1);
+	ctx_info = (struct cper_arm_ctx_info *)(err_info + err->err_info_num);
+	ctx_err = (u8 *)ctx_info;
+	for (n = 0; n < err->context_info_num; n++) {
+		sz = sizeof(struct cper_arm_ctx_info) + ctx_info->size;
+		ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + sz);
+		ctx_len += sz;
+	}
+
+	vsei_len = err->section_length - (sizeof(struct cper_sec_proc_arm) +
+					  pei_len + ctx_len);
+	if (vsei_len < 0) {
+		pr_warn(FW_BUG
+			"section length: %d\n", err->section_length);
+		pr_warn(FW_BUG
+			"section length is too small\n");
+		pr_warn(FW_BUG
+			"firmware-generated error record is incorrect\n");
+		vsei_len = 0;
+	}
+	ven_err_data = (u8 *)ctx_info;
+
+	cpu = GET_LOGICAL_INDEX(err->mpidr);
+	/* when return value is invalid, set cpu index to -1 */
+	if (cpu < 0)
+		cpu = -1;
+
+	trace_arm_event(err, pei_err, pei_len, ctx_err, ctx_len,
+			ven_err_data, (u32)vsei_len, sev, cpu);
 }
 
 static int __init ras_init(void)
diff --git a/include/linux/ras.h b/include/linux/ras.h
index a64182bc72ad..df444492b434 100644
--- a/include/linux/ras.h
+++ b/include/linux/ras.h
@@ -24,8 +24,7 @@ int __init parse_cec_param(char *str);
 void log_non_standard_event(const guid_t *sec_type,
 			    const guid_t *fru_id, const char *fru_text,
 			    const u8 sev, const u8 *err, const u32 len);
-void log_arm_hw_error(struct cper_sec_proc_arm *err);
-
+void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev);
 #else
 static inline void
 log_non_standard_event(const guid_t *sec_type,
@@ -33,7 +32,7 @@ log_non_standard_event(const guid_t *sec_type,
 		       const u8 sev, const u8 *err, const u32 len)
 { return; }
 static inline void
-log_arm_hw_error(struct cper_sec_proc_arm *err) { return; }
+log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev) { return; }
 #endif
 
 struct atl_err {
@@ -53,4 +52,15 @@ static inline unsigned long
 amd_convert_umc_mca_addr_to_sys_addr(struct atl_err *err) { return -EINVAL; }
 #endif /* CONFIG_AMD_ATL */
 
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+#include <asm/smp_plat.h>
+/*
+ * Include ARM specific SMP header which provides a function mapping mpidr to
+ * cpu logical index.
+ */
+#define GET_LOGICAL_INDEX(mpidr) get_logical_index(mpidr & MPIDR_HWID_BITMASK)
+#else
+#define GET_LOGICAL_INDEX(mpidr) -EINVAL
+#endif /* CONFIG_ARM || CONFIG_ARM64 */
+
 #endif /* __RAS_H__ */
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 7c47151d5c72..7d818763934f 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -168,11 +168,24 @@ TRACE_EVENT(mc_event,
  * This event is generated when hardware detects an ARM processor error
  * has occurred. UEFI 2.6 spec section N.2.4.4.
  */
+#define APEIL "ARM Processor Err Info data len"
+#define APEID "ARM Processor Err Info raw data"
+#define APECIL "ARM Processor Err Context Info data len"
+#define APECID "ARM Processor Err Context Info raw data"
+#define VSEIL "Vendor Specific Err Info data len"
+#define VSEID "Vendor Specific Err Info raw data"
 TRACE_EVENT(arm_event,
 
-	TP_PROTO(const struct cper_sec_proc_arm *proc),
+	TP_PROTO(const struct cper_sec_proc_arm *proc, const u8 *pei_err,
+			const u32 pei_len,
+			const u8 *ctx_err,
+			const u32 ctx_len,
+			const u8 *oem,
+			const u32 oem_len,
+			u8 sev,
+			int cpu),
 
-	TP_ARGS(proc),
+	TP_ARGS(proc, pei_err, pei_len, ctx_err, ctx_len, oem, oem_len, sev, cpu),
 
 	TP_STRUCT__entry(
 		__field(u64, mpidr)
@@ -180,6 +193,14 @@ TRACE_EVENT(arm_event,
 		__field(u32, running_state)
 		__field(u32, psci_state)
 		__field(u8, affinity)
+		__field(u32, pei_len)
+		__dynamic_array(u8, pei_buf, pei_len)
+		__field(u32, ctx_len)
+		__dynamic_array(u8, ctx_buf, ctx_len)
+		__field(u32, oem_len)
+		__dynamic_array(u8, oem_buf, oem_len)
+		__field(u8, sev)
+		__field(int, cpu)
 	),
 
 	TP_fast_assign(
@@ -199,12 +220,29 @@ TRACE_EVENT(arm_event,
 			__entry->running_state = ~0;
 			__entry->psci_state = ~0;
 		}
+		__entry->pei_len = pei_len;
+		memcpy(__get_dynamic_array(pei_buf), pei_err, pei_len);
+		__entry->ctx_len = ctx_len;
+		memcpy(__get_dynamic_array(ctx_buf), ctx_err, ctx_len);
+		__entry->oem_len = oem_len;
+		memcpy(__get_dynamic_array(oem_buf), oem, oem_len);
+		__entry->sev = sev;
+		__entry->cpu = cpu;
 	),
 
-	TP_printk("affinity level: %d; MPIDR: %016llx; MIDR: %016llx; "
-		  "running state: %d; PSCI state: %d",
+	TP_printk("cpu: %d; error: %d; affinity level: %d; MPIDR: %016llx; MIDR: %016llx; "
+		  "running state: %d; PSCI state: %d; "
+		  "%s: %d; %s: %s; %s: %d; %s: %s; %s: %d; %s: %s",
+		  __entry->cpu,
+		  __entry->sev,
 		  __entry->affinity, __entry->mpidr, __entry->midr,
-		  __entry->running_state, __entry->psci_state)
+		  __entry->running_state, __entry->psci_state,
+		  APEIL, __entry->pei_len, APEID,
+		  __print_hex(__get_dynamic_array(pei_buf), __entry->pei_len),
+		  APECIL, __entry->ctx_len, APECID,
+		  __print_hex(__get_dynamic_array(ctx_buf), __entry->ctx_len),
+		  VSEIL, __entry->oem_len, VSEID,
+		  __print_hex(__get_dynamic_array(oem_buf), __entry->oem_len))
 );
 
 /*
-- 
2.45.2

[PATCH v2 2/5] efi/cper: Adjust infopfx size to accept an extra space

[Mauro Carvalho Chehab]

Compiling with W=1 with werror enabled produces an error:

drivers/firmware/efi/cper-arm.c: In function ‘cper_print_proc_arm’:
drivers/firmware/efi/cper-arm.c:298:64: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
  298 |                         snprintf(infopfx, sizeof(infopfx), "%s ", newpfx);
      |                                                                ^
drivers/firmware/efi/cper-arm.c:298:25: note: ‘snprintf’ output between 2 and 65 bytes into a destination of size 64
  298 |                         snprintf(infopfx, sizeof(infopfx), "%s ", newpfx);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

As the logic there adds an space at the end of infopx buffer.
Add an extra space to avoid such warning.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/efi/cper-arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/cper-arm.c b/drivers/firmware/efi/cper-arm.c
index fa9c1c3bf168..eb7ee6af55f2 100644
--- a/drivers/firmware/efi/cper-arm.c
+++ b/drivers/firmware/efi/cper-arm.c
@@ -240,7 +240,7 @@ void cper_print_proc_arm(const char *pfx,
 	int i, len, max_ctx_type;
 	struct cper_arm_err_info *err_info;
 	struct cper_arm_ctx_info *ctx_info;
-	char newpfx[64], infopfx[64];
+	char newpfx[64], infopfx[ARRAY_SIZE(newpfx) + 1];
 
 	printk("%sMIDR: 0x%016llx\n", pfx, proc->midr);
 
-- 
2.45.2

[PATCH v2 3/5] efi/cper: Add a new helper function to print bitmasks

[Mauro Carvalho Chehab]

Sometimes it is desired to produce a single log line for errors.
Add a new helper function for such purpose.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
 include/linux/cper.h        |  2 ++
 2 files changed, 45 insertions(+)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 7d2cdd9e2227..462d739e8dd1 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -106,6 +106,49 @@ void cper_print_bits(const char *pfx, unsigned int bits,
 		printk("%s\n", buf);
 }
 
+/**
+ * cper_bits_to_str - return a string for set bits
+ * @buf: buffer to store the output string
+ * @buf_size: size of the output string buffer
+ * @bits: bit mask
+ * @strs: string array, indexed by bit position
+ * @strs_size: size of the string array: @strs
+ *
+ * Add to @buf the bitmask in hexadecimal. Then, for each set bit in @bits,
+ * add the corresponding string describing the bit in @strs to @buf.
+ *
+ * Return: number of bytes stored or an error code if lower than zero.
+ */
+int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
+		     const char * const strs[], unsigned int strs_size)
+{
+	int len = buf_size;
+	char *str = buf;
+	int i, size;
+
+	*buf = '\0';
+
+	for_each_set_bit(i, &bits, strs_size) {
+		if (!(bits & (1U << (i))))
+			continue;
+
+		if (*buf && len > 0) {
+			*str = '|';
+			len--;
+			str++;
+		}
+
+		size = strscpy(str, strs[i], len);
+		if (size < 0)
+			return size;
+
+		len -= size;
+		str += size;
+	}
+	return len - buf_size;
+}
+EXPORT_SYMBOL_GPL(cper_bits_to_str);
+
 static const char * const proc_type_strs[] = {
 	"IA32/X64",
 	"IA64",
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 265b0f8fc0b3..25858a7608b7 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -584,6 +584,8 @@ const char *cper_mem_err_type_str(unsigned int);
 const char *cper_mem_err_status_str(u64 status);
 void cper_print_bits(const char *prefix, unsigned int bits,
 		     const char * const strs[], unsigned int strs_size);
+int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
+		     const char * const strs[], unsigned int strs_size);
 void cper_mem_err_pack(const struct cper_sec_mem_err *,
 		       struct cper_mem_err_compact *);
 const char *cper_mem_err_unpack(struct trace_seq *,
-- 
2.45.2

[PATCH v2 4/5] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs

[Mauro Carvalho Chehab]

Up to UEFI spec 2.9, the type byte of CPER struct for ARM processor
was defined simply as:

Type at byte offset 4:

	- Cache error
	- TLB Error
	- Bus Error
	- Micro-architectural Error
	All other values are reserved

Yet, there was no information about how this would be encoded.

Spec 2.9A errata corrected it by defining:

	- Bit 1 - Cache Error
	- Bit 2 - TLB Error
	- Bit 3 - Bus Error
	- Bit 4 - Micro-architectural Error
	All other values are reserved

That actually aligns with the values already defined on older
versions at N.2.4.1. Generic Processor Error Section.

Spec 2.10 also preserve the same encoding as 2.9A.

Adjust CPER and GHES handling code for both generic and ARM
processors to properly handle UEFI 2.9A and 2.10 encoding.

Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-information
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/acpi/apei/ghes.c        | 15 ++++++----
 drivers/firmware/efi/cper-arm.c | 50 ++++++++++++++++-----------------
 include/linux/cper.h            | 10 +++----
 3 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 06d9351a9abc..d7d78d546f68 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -533,6 +533,7 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
 {
 	struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
 	int flags = sync ? MF_ACTION_REQUIRED : 0;
+	char error_type[120];
 	bool queued = false;
 	int sec_sev, i;
 	char *p;
@@ -545,9 +546,8 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
 	p = (char *)(err + 1);
 	for (i = 0; i < err->err_info_num; i++) {
 		struct cper_arm_err_info *err_info = (struct cper_arm_err_info *)p;
-		bool is_cache = (err_info->type == CPER_ARM_CACHE_ERROR);
+		bool is_cache = err_info->type & CPER_ARM_CACHE_ERROR;
 		bool has_pa = (err_info->validation_bits & CPER_ARM_INFO_VALID_PHYSICAL_ADDR);
-		const char *error_type = "unknown error";
 
 		/*
 		 * The field (err_info->error_info & BIT(26)) is fixed to set to
@@ -561,12 +561,15 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
 			continue;
 		}
 
-		if (err_info->type < ARRAY_SIZE(cper_proc_error_type_strs))
-			error_type = cper_proc_error_type_strs[err_info->type];
+		cper_bits_to_str(error_type, sizeof(error_type),
+				 FIELD_GET(CPER_ARM_ERR_TYPE_MASK, err_info->type),
+				 cper_proc_error_type_strs,
+				 ARRAY_SIZE(cper_proc_error_type_strs));
 
 		pr_warn_ratelimited(FW_WARN GHES_PFX
-				    "Unhandled processor error type: %s\n",
-				    error_type);
+				    "Unhandled processor error type 0x%02x: %s%s\n",
+				    err_info->type, error_type,
+				    (err_info->type & ~CPER_ARM_ERR_TYPE_MASK) ? " with reserved bit(s)" : "");
 		p += err_info->length;
 	}
 
diff --git a/drivers/firmware/efi/cper-arm.c b/drivers/firmware/efi/cper-arm.c
index eb7ee6af55f2..52d18490b59e 100644
--- a/drivers/firmware/efi/cper-arm.c
+++ b/drivers/firmware/efi/cper-arm.c
@@ -93,15 +93,11 @@ static void cper_print_arm_err_info(const char *pfx, u32 type,
 	bool proc_context_corrupt, corrected, precise_pc, restartable_pc;
 	bool time_out, access_mode;
 
-	/* If the type is unknown, bail. */
-	if (type > CPER_ARM_MAX_TYPE)
-		return;
-
 	/*
 	 * Vendor type errors have error information values that are vendor
 	 * specific.
 	 */
-	if (type == CPER_ARM_VENDOR_ERROR)
+	if (type & CPER_ARM_VENDOR_ERROR)
 		return;
 
 	if (error_info & CPER_ARM_ERR_VALID_TRANSACTION_TYPE) {
@@ -116,43 +112,38 @@ static void cper_print_arm_err_info(const char *pfx, u32 type,
 	if (error_info & CPER_ARM_ERR_VALID_OPERATION_TYPE) {
 		op_type = ((error_info >> CPER_ARM_ERR_OPERATION_SHIFT)
 			   & CPER_ARM_ERR_OPERATION_MASK);
-		switch (type) {
-		case CPER_ARM_CACHE_ERROR:
+		if (type & CPER_ARM_CACHE_ERROR) {
 			if (op_type < ARRAY_SIZE(arm_cache_err_op_strs)) {
-				printk("%soperation type: %s\n", pfx,
+				printk("%scache error, operation type: %s\n", pfx,
 				       arm_cache_err_op_strs[op_type]);
 			}
-			break;
-		case CPER_ARM_TLB_ERROR:
+		}
+		if (type & CPER_ARM_TLB_ERROR) {
 			if (op_type < ARRAY_SIZE(arm_tlb_err_op_strs)) {
-				printk("%soperation type: %s\n", pfx,
+				printk("%sTLB error, operation type: %s\n", pfx,
 				       arm_tlb_err_op_strs[op_type]);
 			}
-			break;
-		case CPER_ARM_BUS_ERROR:
+		}
+		if (type & CPER_ARM_BUS_ERROR) {
 			if (op_type < ARRAY_SIZE(arm_bus_err_op_strs)) {
-				printk("%soperation type: %s\n", pfx,
+				printk("%sbus error, operation type: %s\n", pfx,
 				       arm_bus_err_op_strs[op_type]);
 			}
-			break;
 		}
 	}
 
 	if (error_info & CPER_ARM_ERR_VALID_LEVEL) {
 		level = ((error_info >> CPER_ARM_ERR_LEVEL_SHIFT)
 			 & CPER_ARM_ERR_LEVEL_MASK);
-		switch (type) {
-		case CPER_ARM_CACHE_ERROR:
+		if (type & CPER_ARM_CACHE_ERROR)
 			printk("%scache level: %d\n", pfx, level);
-			break;
-		case CPER_ARM_TLB_ERROR:
+
+		if (type & CPER_ARM_TLB_ERROR)
 			printk("%sTLB level: %d\n", pfx, level);
-			break;
-		case CPER_ARM_BUS_ERROR:
+
+		if (type & CPER_ARM_BUS_ERROR)
 			printk("%saffinity level at which the bus error occurred: %d\n",
 			       pfx, level);
-			break;
-		}
 	}
 
 	if (error_info & CPER_ARM_ERR_VALID_PROC_CONTEXT_CORRUPT) {
@@ -241,6 +232,7 @@ void cper_print_proc_arm(const char *pfx,
 	struct cper_arm_err_info *err_info;
 	struct cper_arm_ctx_info *ctx_info;
 	char newpfx[64], infopfx[ARRAY_SIZE(newpfx) + 1];
+	char error_type[120];
 
 	printk("%sMIDR: 0x%016llx\n", pfx, proc->midr);
 
@@ -289,9 +281,15 @@ void cper_print_proc_arm(const char *pfx,
 				       newpfx);
 		}
 
-		printk("%serror_type: %d, %s\n", newpfx, err_info->type,
-			err_info->type < ARRAY_SIZE(cper_proc_error_type_strs) ?
-			cper_proc_error_type_strs[err_info->type] : "unknown");
+		cper_bits_to_str(error_type, sizeof(error_type),
+				 FIELD_GET(CPER_ARM_ERR_TYPE_MASK, err_info->type),
+				 cper_proc_error_type_strs,
+				 ARRAY_SIZE(cper_proc_error_type_strs));
+
+		printk("%serror_type: 0x%02x: %s%s\n", newpfx, err_info->type,
+		       error_type,
+		       (err_info->type & ~CPER_ARM_ERR_TYPE_MASK) ? " with reserved bit(s)" : "");
+
 		if (err_info->validation_bits & CPER_ARM_INFO_VALID_ERR_INFO) {
 			printk("%serror_info: 0x%016llx\n", newpfx,
 			       err_info->error_info);
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 25858a7608b7..3670b866ac11 100644
--- a/include/linux/cper.h
+++ b/include/linux/cper.h
@@ -293,11 +293,11 @@ enum {
 #define CPER_ARM_INFO_FLAGS_PROPAGATED		BIT(2)
 #define CPER_ARM_INFO_FLAGS_OVERFLOW		BIT(3)
 
-#define CPER_ARM_CACHE_ERROR			0
-#define CPER_ARM_TLB_ERROR			1
-#define CPER_ARM_BUS_ERROR			2
-#define CPER_ARM_VENDOR_ERROR			3
-#define CPER_ARM_MAX_TYPE			CPER_ARM_VENDOR_ERROR
+#define CPER_ARM_ERR_TYPE_MASK			GENMASK(4,1)
+#define CPER_ARM_CACHE_ERROR			BIT(1)
+#define CPER_ARM_TLB_ERROR			BIT(2)
+#define CPER_ARM_BUS_ERROR			BIT(3)
+#define CPER_ARM_VENDOR_ERROR			BIT(4)
 
 #define CPER_ARM_ERR_VALID_TRANSACTION_TYPE	BIT(0)
 #define CPER_ARM_ERR_VALID_OPERATION_TYPE	BIT(1)
-- 
2.45.2

[PATCH v2 5/5] docs: efi: add CPER functions to driver-api

[Mauro Carvalho Chehab]

There are two kernel-doc like descriptions at cper, which is used
by other parts of cper and on ghes driver. They both have kernel-doc
like descriptions.

Change the tags for them to be actual kernel-doc tags and add them
to the driver-api documentaion at the UEFI section.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 Documentation/driver-api/firmware/efi/index.rst | 11 ++++++++---
 drivers/firmware/efi/cper.c                     |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/firmware/efi/index.rst b/Documentation/driver-api/firmware/efi/index.rst
index 4fe8abba9fc6..5a6b6229592c 100644
--- a/Documentation/driver-api/firmware/efi/index.rst
+++ b/Documentation/driver-api/firmware/efi/index.rst
@@ -1,11 +1,16 @@
 .. SPDX-License-Identifier: GPL-2.0
 
-============
-UEFI Support
-============
+====================================================
+Unified Extensible Firmware Interface (UEFI) Support
+====================================================
 
 UEFI stub library functions
 ===========================
 
 .. kernel-doc:: drivers/firmware/efi/libstub/mem.c
    :internal:
+
+UEFI Common Platform Error Record (CPER) functions
+==================================================
+
+.. kernel-doc:: drivers/firmware/efi/cper.c
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 462d739e8dd1..295f6f339575 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -69,7 +69,7 @@ const char *cper_severity_str(unsigned int severity)
 }
 EXPORT_SYMBOL_GPL(cper_severity_str);
 
-/*
+/**
  * cper_print_bits - print strings for set bits
  * @pfx: prefix for each line, including log level and prefix string
  * @bits: bit mask
-- 
2.45.2

Re: [PATCH v2 3/5] efi/cper: Add a new helper function to print bitmasks

[Jonathan Cameron]

On Thu, 11 Jul 2024 08:28:54 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Sometimes it is desired to produce a single log line for errors.
> Add a new helper function for such purpose.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
LGTM

Reviewed-by; Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
>  include/linux/cper.h        |  2 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 7d2cdd9e2227..462d739e8dd1 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -106,6 +106,49 @@ void cper_print_bits(const char *pfx, unsigned int bits,
>  		printk("%s\n", buf);
>  }
>  
> +/**
> + * cper_bits_to_str - return a string for set bits
> + * @buf: buffer to store the output string
> + * @buf_size: size of the output string buffer
> + * @bits: bit mask
> + * @strs: string array, indexed by bit position
> + * @strs_size: size of the string array: @strs
> + *
> + * Add to @buf the bitmask in hexadecimal. Then, for each set bit in @bits,
> + * add the corresponding string describing the bit in @strs to @buf.
> + *
> + * Return: number of bytes stored or an error code if lower than zero.
> + */
> +int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
> +		     const char * const strs[], unsigned int strs_size)
> +{
> +	int len = buf_size;
> +	char *str = buf;
> +	int i, size;
> +
> +	*buf = '\0';
> +
> +	for_each_set_bit(i, &bits, strs_size) {
> +		if (!(bits & (1U << (i))))
> +			continue;
> +
> +		if (*buf && len > 0) {
> +			*str = '|';
> +			len--;
> +			str++;
> +		}
> +
> +		size = strscpy(str, strs[i], len);
> +		if (size < 0)
> +			return size;
> +
> +		len -= size;
> +		str += size;
> +	}
> +	return len - buf_size;
> +}
> +EXPORT_SYMBOL_GPL(cper_bits_to_str);
> +
>  static const char * const proc_type_strs[] = {
>  	"IA32/X64",
>  	"IA64",
> diff --git a/include/linux/cper.h b/include/linux/cper.h
> index 265b0f8fc0b3..25858a7608b7 100644
> --- a/include/linux/cper.h
> +++ b/include/linux/cper.h
> @@ -584,6 +584,8 @@ const char *cper_mem_err_type_str(unsigned int);
>  const char *cper_mem_err_status_str(u64 status);
>  void cper_print_bits(const char *prefix, unsigned int bits,
>  		     const char * const strs[], unsigned int strs_size);
> +int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
> +		     const char * const strs[], unsigned int strs_size);
>  void cper_mem_err_pack(const struct cper_sec_mem_err *,
>  		       struct cper_mem_err_compact *);
>  const char *cper_mem_err_unpack(struct trace_seq *,

Re: [PATCH v2 1/5] RAS: Report all ARM processor CPER information to userspace

[Borislav Petkov]

On Thu, Jul 11, 2024 at 08:28:52AM +0200, Mauro Carvalho Chehab wrote:
> In addition to those data, it also exports two fields that are
> parsed by the GHES driver when firmware reports it, e. g.:
> 
> - error severity
> - cpu logical index

s/cpu/CPU/g

check your whole set pls.

> Report all of these information to userspace via trace uAPI, So that
> userspace can properly record the error and take decisions related
> to cpu core isolation according to error severity and other info.
> 
> After this patch, all the data from ARM Processor record from table

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

...

> [mchehab: modified patch description, solve merge conflicts and fix coding style]
> Fixes: e9279e83ad1f ("trace, ras: add ARM processor error trace event")
> Signed-off-by: Shengwei Luo <luoshengwei@huawei.com>
> Signed-off-by: Jason Tian <jason@os.amperecomputing.com>
> Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com>

What is this SOB chain trying to tell me?

All those folks handled the patch?

> Tested-by: Shiju Jose <shiju.jose@huawei.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-section
> ---
>  drivers/acpi/apei/ghes.c | 11 ++++-----
>  drivers/ras/ras.c        | 45 +++++++++++++++++++++++++++++++++++--
>  include/linux/ras.h      | 16 +++++++++++---
>  include/ras/ras_event.h  | 48 +++++++++++++++++++++++++++++++++++-----
>  4 files changed, 103 insertions(+), 17 deletions(-)

...

> -void log_arm_hw_error(struct cper_sec_proc_arm *err)
> +void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev)
>  {
> -	trace_arm_event(err);
> +	struct cper_arm_err_info *err_info;
> +	struct cper_arm_ctx_info *ctx_info;
> +	u8 *ven_err_data;
> +	u32 ctx_len = 0;
> +	int n, sz, cpu;
> +	s32 vsei_len;
> +	u32 pei_len;
> +	u8 *pei_err;
> +	u8 *ctx_err;
> +
> +	pei_len = sizeof(struct cper_arm_err_info) * err->err_info_num;
> +	pei_err = (u8 *)err + sizeof(struct cper_sec_proc_arm);
> +
> +	err_info = (struct cper_arm_err_info *)(err + 1);
> +	ctx_info = (struct cper_arm_ctx_info *)(err_info + err->err_info_num);
> +	ctx_err = (u8 *)ctx_info;
> +	for (n = 0; n < err->context_info_num; n++) {
> +		sz = sizeof(struct cper_arm_ctx_info) + ctx_info->size;
> +		ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + sz);
> +		ctx_len += sz;
> +	}
> +
> +	vsei_len = err->section_length - (sizeof(struct cper_sec_proc_arm) +
> +					  pei_len + ctx_len);
> +	if (vsei_len < 0) {
> +		pr_warn(FW_BUG
> +			"section length: %d\n", err->section_length);
> +		pr_warn(FW_BUG
> +			"section length is too small\n");
> +		pr_warn(FW_BUG
> +			"firmware-generated error record is incorrect\n");

No need to break those lines.

> +		vsei_len = 0;
> +	}
> +	ven_err_data = (u8 *)ctx_info;
> +
> +	cpu = GET_LOGICAL_INDEX(err->mpidr);
> +	/* when return value is invalid, set cpu index to -1 */

Obvious comment - no need for it.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 1/5] RAS: Report all ARM processor CPER information to userspace

[Mauro Carvalho Chehab]

Hi Boris,

Em Thu, 29 Aug 2024 16:38:11 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Thu, Jul 11, 2024 at 08:28:52AM +0200, Mauro Carvalho Chehab wrote:
> > In addition to those data, it also exports two fields that are
> > parsed by the GHES driver when firmware reports it, e. g.:
> > 
> > - error severity
> > - cpu logical index  
> 
> s/cpu/CPU/g
> 
> check your whole set pls.

Ok. Will address those at the hole series, sending you later today
a new version. Except for those, are patches 2-5 ok?

Regards,
Mauro

> > Report all of these information to userspace via trace uAPI, So that
> > userspace can properly record the error and take decisions related
> > to cpu core isolation according to error severity and other info.
> > 
> > After this patch, all the data from ARM Processor record from table  
> 
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
> 
> Also, do
> 
> $ git grep 'This patch' Documentation/process
> 
> for more details.

Usually, I don't use "this patch". In this specific case, I wanted
to bold the new fields that were added to the ARM trace event, making
clear that before the changeset, none of such fields exist; they were
added on such change. On other words, the keyword here is not patch,
but instead "After". Maybe I can replace it with "now", e. g.:

	The updated ARM trace event now contains the following fields:

	======================================	=============================
	UEFI field on table N.16		ARM Processor trace fields
	======================================	=============================
	Validation				handled when filling data for
						affinity MPIDR and running
						state.
	ERR_INFO_NUM				pei_len
	CONTEXT_INFO_NUM			ctx_len
	Section Length				indirectly reported by
						pei_len, ctx_len and oem_len
	Error affinity level			affinity
	MPIDR_EL1				mpidr
	MIDR_EL1				midr
	Running State				running_state
	PSCI State				psci_state
	Processor Error Information Structure	pei_err - count at pei_len
	Processor Context			ctx_err- count at ctx_len
	Vendor Specific Error Info		oem - count at oem_len
	======================================	=============================


> 
> ...
> 
> > [mchehab: modified patch description, solve merge conflicts and fix coding style]
> > Fixes: e9279e83ad1f ("trace, ras: add ARM processor error trace event")
> > Signed-off-by: Shengwei Luo <luoshengwei@huawei.com>
> > Signed-off-by: Jason Tian <jason@os.amperecomputing.com>
> > Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com>  
> 
> What is this SOB chain trying to tell me?
> 
> All those folks handled the patch?

They reflect what happened with past attempts of upstreaming this
change at the EDAC mailing list.

See, originally this seems to come from Jason Tian in 2021:
	https://lore.kernel.org/linux-edac/20210205022229.313030-1-jason@os.amperecomputing.com/
	https://lore.kernel.org/linux-edac/20210422084944.3718-1-jason@os.amperecomputing.com/
	https://lore.kernel.org/linux-edac/20210802135929.5283-1-shijie@os.amperecomputing.com/

In 2022, it came a new version from Shengwei Luo:
	https://lore.kernel.org/lkml/20220126030906.56765-1-lostway@zju.edu.cn/
	https://lore.kernel.org/linux-edac/20220214030813.135766-1-lostway@zju.edu.cn/

A new version from Daniel Ferguson arrived in 2023:
	https://lore.kernel.org/linux-edac/20231214232330.306526-2-danielf@os.amperecomputing.com/

Hard to reconstruct the entire history of this changeset, as there were
several attempts to fix it, and patches got renamed on some of such
attempts.

Anyway, it sounds that the custody chan can better be written as:

	Co-authored-by: Jason Tian <jason@os.amperecomputing.com>
	Co-authored-by: Signed-off-by: Shengwei Luo <luoshengwei@huawei.com>
	Co-authored-by: Daniel Ferguson <danielf@os.amperecomputing.com>  
	Signed-off-by: Jason Tian <jason@os.amperecomputing.com>
	Signed-off-by: Shengwei Luo <luoshengwei@huawei.com>
	Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com>  

It probably makes sense to also indicate the original author of
it by change the "From" metadata to:

	From: Jason Tian <jason@os.amperecomputing.com>

> 
> > Tested-by: Shiju Jose <shiju.jose@huawei.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-section
> > ---
> >  drivers/acpi/apei/ghes.c | 11 ++++-----
> >  drivers/ras/ras.c        | 45 +++++++++++++++++++++++++++++++++++--
> >  include/linux/ras.h      | 16 +++++++++++---
> >  include/ras/ras_event.h  | 48 +++++++++++++++++++++++++++++++++++-----
> >  4 files changed, 103 insertions(+), 17 deletions(-)  
> 
> ...
> 
> > -void log_arm_hw_error(struct cper_sec_proc_arm *err)
> > +void log_arm_hw_error(struct cper_sec_proc_arm *err, const u8 sev)
> >  {
> > -	trace_arm_event(err);
> > +	struct cper_arm_err_info *err_info;
> > +	struct cper_arm_ctx_info *ctx_info;
> > +	u8 *ven_err_data;
> > +	u32 ctx_len = 0;
> > +	int n, sz, cpu;
> > +	s32 vsei_len;
> > +	u32 pei_len;
> > +	u8 *pei_err;
> > +	u8 *ctx_err;
> > +
> > +	pei_len = sizeof(struct cper_arm_err_info) * err->err_info_num;
> > +	pei_err = (u8 *)err + sizeof(struct cper_sec_proc_arm);
> > +
> > +	err_info = (struct cper_arm_err_info *)(err + 1);
> > +	ctx_info = (struct cper_arm_ctx_info *)(err_info + err->err_info_num);
> > +	ctx_err = (u8 *)ctx_info;
> > +	for (n = 0; n < err->context_info_num; n++) {
> > +		sz = sizeof(struct cper_arm_ctx_info) + ctx_info->size;
> > +		ctx_info = (struct cper_arm_ctx_info *)((long)ctx_info + sz);
> > +		ctx_len += sz;
> > +	}
> > +
> > +	vsei_len = err->section_length - (sizeof(struct cper_sec_proc_arm) +
> > +					  pei_len + ctx_len);
> > +	if (vsei_len < 0) {
> > +		pr_warn(FW_BUG
> > +			"section length: %d\n", err->section_length);
> > +		pr_warn(FW_BUG
> > +			"section length is too small\n");
> > +		pr_warn(FW_BUG
> > +			"firmware-generated error record is incorrect\n");  
> 
> No need to break those lines.
> 
> > +		vsei_len = 0;
> > +	}
> > +	ven_err_data = (u8 *)ctx_info;
> > +
> > +	cpu = GET_LOGICAL_INDEX(err->mpidr);
> > +	/* when return value is invalid, set cpu index to -1 */  
> 
> Obvious comment - no need for it.
> 

Will address at the next review.

Thanks,
Mauro

Re: [PATCH v2 1/5] RAS: Report all ARM processor CPER information to userspace

[Borislav Petkov]

On Mon, Sep 02, 2024 at 06:12:36AM +0200, Mauro Carvalho Chehab wrote:
> Ok. Will address those at the hole series, sending you later today
> a new version. Except for those, are patches 2-5 ok?

Haven't looked at them yet.

> Usually, I don't use "this patch". In this specific case, I wanted
> to bold the new fields that were added to the ARM trace event, making
> clear that before the changeset, none of such fields exist; they were
> added on such change. On other words, the keyword here is not patch,
> but instead "After". Maybe I can replace it with "now", e. g.:

Yes, and you can see what you're doing in the patch itself.

> Anyway, it sounds that the custody chan can better be written as:
> 
> 	Co-authored-by: Jason Tian <jason@os.amperecomputing.com>
> 	Co-authored-by: Signed-off-by: Shengwei Luo <luoshengwei@huawei.com>
> 	Co-authored-by: Daniel Ferguson <danielf@os.amperecomputing.com>  
> 	Signed-off-by: Jason Tian <jason@os.amperecomputing.com>
> 	Signed-off-by: Shengwei Luo <luoshengwei@huawei.com>
> 	Signed-off-by: Daniel Ferguson <danielf@os.amperecomputing.com>  

The tag's name is Co-developed-by: and yes, I think it makes more sense here.

Note:

"Since Co-developed-by: denotes authorship, every Co-developed-by: must be
immediately followed by a Signed-off-by: of the associated co-author.
Standard sign-off procedure applies, i.e. the ordering of Signed-off-by: tags
should reflect the chronological history of the patch insofar as possible,
regardless of whether the author is attributed via From: or Co-developed-by:.
Notably, the last Signed-off-by: must always be that of the developer
submitting the patch."

It is all documented:

Documentation/process/submitting-patches.rst

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 3/5] efi/cper: Add a new helper function to print bitmasks

[Borislav Petkov]

On Thu, Jul 11, 2024 at 08:28:54AM +0200, Mauro Carvalho Chehab wrote:
> Sometimes it is desired to produce a single log line for errors.
> Add a new helper function for such purpose.

How does this have anything to do with the below function?

Example?

Why isn't anything in lib/bitmap-str.c not useful for this?

> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
>  include/linux/cper.h        |  2 ++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 7d2cdd9e2227..462d739e8dd1 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -106,6 +106,49 @@ void cper_print_bits(const char *pfx, unsigned int bits,
>  		printk("%s\n", buf);
>  }
>  
> +/**
> + * cper_bits_to_str - return a string for set bits
> + * @buf: buffer to store the output string
> + * @buf_size: size of the output string buffer
> + * @bits: bit mask
> + * @strs: string array, indexed by bit position
> + * @strs_size: size of the string array: @strs
> + *
> + * Add to @buf the bitmask in hexadecimal. 

Where does it do that?

> Then, for each set bit in @bits,
> + * add the corresponding string describing the bit in @strs to @buf.
> + *
> + * Return: number of bytes stored or an error code if lower than zero.
> + */
> +int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
> +		     const char * const strs[], unsigned int strs_size)
> +{
> +	int len = buf_size;
> +	char *str = buf;
> +	int i, size;
> +
> +	*buf = '\0';
> +
> +	for_each_set_bit(i, &bits, strs_size) {
> +		if (!(bits & (1U << (i))))

BIT_UL()

> +			continue;
> +
> +		if (*buf && len > 0) {

Uff, this is testing the first char in buf and it gets copied in below in
strscpy() through the str pointer.

So this function converts a set of set bits to their corresponding *names*
from strs[].

This name doesn't even begin to explain what this function does - it converts
a bitmap to the corresponding names of the bits in @strs. If anything, the
above comment needs an example and the function needs to be named properly.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 4/5] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs

[Borislav Petkov]

On Thu, Jul 11, 2024 at 08:28:55AM +0200, Mauro Carvalho Chehab wrote:
> Up to UEFI spec 2.9, the type byte of CPER struct for ARM processor
> was defined simply as:
> 
> Type at byte offset 4:
> 
> 	- Cache error
> 	- TLB Error
> 	- Bus Error
> 	- Micro-architectural Error
> 	All other values are reserved
> 
> Yet, there was no information about how this would be encoded.
> 
> Spec 2.9A errata corrected it by defining:
> 
> 	- Bit 1 - Cache Error
> 	- Bit 2 - TLB Error
> 	- Bit 3 - Bus Error
> 	- Bit 4 - Micro-architectural Error
> 	All other values are reserved
> 
> That actually aligns with the values already defined on older
> versions at N.2.4.1. Generic Processor Error Section.
> 
> Spec 2.10 also preserve the same encoding as 2.9A.
> 
> Adjust CPER and GHES handling code for both generic and ARM
> processors to properly handle UEFI 2.9A and 2.10 encoding.
> 
> Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-information
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/acpi/apei/ghes.c        | 15 ++++++----
>  drivers/firmware/efi/cper-arm.c | 50 ++++++++++++++++-----------------
>  include/linux/cper.h            | 10 +++----
>  3 files changed, 38 insertions(+), 37 deletions(-)

How was this thing ever tested?!?!

Geez.

drivers/acpi/apei/ghes.c: In function ‘ghes_handle_arm_hw_error’:
drivers/acpi/apei/ghes.c:565:34: error: implicit declaration of function ‘FIELD_GET’ [-Werror=implicit-function-declaration]
  565 |                                  FIELD_GET(CPER_ARM_ERR_TYPE_MASK, err_info->type),
      |                                  ^~~~~~~~~
cc1: some warnings being treated as errors
make[5]: *** [scripts/Makefile.build:244: drivers/acpi/apei/ghes.o] Error 1
make[4]: *** [scripts/Makefile.build:485: drivers/acpi/apei] Error 2
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/acpi] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/mnt/kernel/kernel/2nd/linux/Makefile:1925: .] Error 2
make: *** [Makefile:224: __sub-make] Error 2

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 4/5] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs

[Mauro Carvalho Chehab]

Em Mon, 2 Sep 2024 17:27:55 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Thu, Jul 11, 2024 at 08:28:55AM +0200, Mauro Carvalho Chehab wrote:
> > Up to UEFI spec 2.9, the type byte of CPER struct for ARM processor
> > was defined simply as:
> > 
> > Type at byte offset 4:
> > 
> > 	- Cache error
> > 	- TLB Error
> > 	- Bus Error
> > 	- Micro-architectural Error
> > 	All other values are reserved
> > 
> > Yet, there was no information about how this would be encoded.
> > 
> > Spec 2.9A errata corrected it by defining:
> > 
> > 	- Bit 1 - Cache Error
> > 	- Bit 2 - TLB Error
> > 	- Bit 3 - Bus Error
> > 	- Bit 4 - Micro-architectural Error
> > 	All other values are reserved
> > 
> > That actually aligns with the values already defined on older
> > versions at N.2.4.1. Generic Processor Error Section.
> > 
> > Spec 2.10 also preserve the same encoding as 2.9A.
> > 
> > Adjust CPER and GHES handling code for both generic and ARM
> > processors to properly handle UEFI 2.9A and 2.10 encoding.
> > 
> > Link: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-information
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/acpi/apei/ghes.c        | 15 ++++++----
> >  drivers/firmware/efi/cper-arm.c | 50 ++++++++++++++++-----------------
> >  include/linux/cper.h            | 10 +++----
> >  3 files changed, 38 insertions(+), 37 deletions(-)  
> 
> How was this thing ever tested?!?!

It was tested on ARM, together with QEMU error injection code I'm working
there. Currently, only GPIO and SEA notification types are supported, so
no x86 support yet (I'm trying to add SCI too, allowing to test on x86
as well[1]).

It sounds that bitfield.h is indirectly included from
arch/arm64/include/asm/sysreg.h when compiled on such arch.

Anyway, I'll add an include for it there.

[1] https://gitlab.com/mchehab_kernel/qemu/-/commits/TODO_x86_ghes?ref_type=heads
    code is not working yet. I'm stuck with an issue related with EDK2.
    on x86_64, it is failing to create a new fw_cfg file needed to
    store HEST error structures.

Regards,
Mauro

Re: [PATCH v2 3/5] efi/cper: Add a new helper function to print bitmasks

[Mauro Carvalho Chehab]

Em Mon, 2 Sep 2024 13:24:29 +0200
Borislav Petkov <bp@alien8.de> escreveu:

> On Thu, Jul 11, 2024 at 08:28:54AM +0200, Mauro Carvalho Chehab wrote:
> > Sometimes it is desired to produce a single log line for errors.
> > Add a new helper function for such purpose.  
> 
> How does this have anything to do with the below function?

There is a variant at cper.c that creates a multi-line output
for bitmaps.

> Example?
> 
> Why isn't anything in lib/bitmap-str.c not useful for this?

I took a look there. I wasn't able to find anything remotely
close to convert a bitmap into their correspondent bit names.

See, the intended usage for this function is to convert ACPI
bitmasks into the field names. On ARM Processor Error, this is
used to properly parse the type field, as described at:

	https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-information

The definition for this specific bitmask starts on bit 1,
so the logic to parse it uses a FIELD_GET to apply the
proper bitmask. This is how it is used (see patch 4/5):

	const char * const cper_proc_error_type_strs[] = {
		"cache error",
		"TLB error",
		"bus error",
		"micro-architectural error",
	};

	#define CPER_ARM_ERR_TYPE_MASK                     GENMASK(4,1)

	char error_type[120];

	cper_bits_to_str(error_type, sizeof(error_type),
			 FIELD_GET(CPER_ARM_ERR_TYPE_MASK, err_info->type),
			 cper_proc_error_type_strs,
			 ARRAY_SIZE(cper_proc_error_type_strs));

I'll add an example similar to it to kernel-doc comment.

> 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
> >  include/linux/cper.h        |  2 ++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> > index 7d2cdd9e2227..462d739e8dd1 100644
> > --- a/drivers/firmware/efi/cper.c
> > +++ b/drivers/firmware/efi/cper.c
> > @@ -106,6 +106,49 @@ void cper_print_bits(const char *pfx, unsigned int bits,
> >  		printk("%s\n", buf);
> >  }
> >  
> > +/**
> > + * cper_bits_to_str - return a string for set bits
> > + * @buf: buffer to store the output string
> > + * @buf_size: size of the output string buffer
> > + * @bits: bit mask
> > + * @strs: string array, indexed by bit position
> > + * @strs_size: size of the string array: @strs
> > + *

> > + * Add to @buf the bitmask in hexadecimal.   
> 
> Where does it do that?

Legacy comment from the past version. Will drop it.

> > Then, for each set bit in @bits,
> > + * add the corresponding string describing the bit in @strs to @buf.
> > + *
> > + * Return: number of bytes stored or an error code if lower than zero.
> > + */
> > +int cper_bits_to_str(char *buf, int buf_size, unsigned long bits,
> > +		     const char * const strs[], unsigned int strs_size)
> > +{
> > +	int len = buf_size;
> > +	char *str = buf;
> > +	int i, size;
> > +
> > +	*buf = '\0';
> > +
> > +	for_each_set_bit(i, &bits, strs_size) {
> > +		if (!(bits & (1U << (i))))  
> 
> BIT_UL()
> 
> > +			continue;
> > +
> > +		if (*buf && len > 0) {  
> 
> Uff, this is testing the first char in buf and it gets copied in below in
> strscpy() through the str pointer.
> 
> So this function converts a set of set bits to their corresponding *names*
> from strs[].

Yes.

> This name doesn't even begin to explain what this function does - it converts
> a bitmap to the corresponding names of the bits in @strs. If anything, the
> above comment needs an example and the function needs to be named properly.

I'll add an example.

Thanks,
Mauro

Re: [PATCH v2 4/5] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs

[Borislav Petkov]

On Wed, Sep 04, 2024 at 06:45:49AM +0200, Mauro Carvalho Chehab wrote:
> It was tested on ARM, together with QEMU error injection code I'm working
> there. Currently, only GPIO and SEA notification types are supported, so
> no x86 support yet (I'm trying to add SCI too, allowing to test on x86
> as well[1]).
> 
> It sounds that bitfield.h is indirectly included from
> arch/arm64/include/asm/sysreg.h when compiled on such arch.

Before you send patches, make sure you *at* *least* build-test them on the
affected architectures. I don't have to tell you this.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette