efi/cper: Adjust infopfx size to accept an extra space

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

[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 <[email protected]>

Signed-off-by Jason Tian <[email protected]>

Signed-off-by Daniel Ferguson <[email protected]>

Tested-by Shiju Jose <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

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

drivers/ras/ras.c +22 -2

-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;
+	}

Quoted 5 lines

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.

Quoted 5 lines

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.

...

Quoted 5 lines

[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 <[email protected]>

Signed-off-by Jason Tian <[email protected]>

Signed-off-by Daniel Ferguson <[email protected]>

What is this SOB chain trying to tell me?

All those folks handled the patch?

Quoted 12 lines

Tested-by Shiju Jose <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

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

...

Quoted 25 lines

drivers/ras/ras.c +22 -2

-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;
+	}

drivers/ras/ras.c void log_non_standard_event(const guid_t *sec_type, const guid_t *fru_id, · line 87

Quoted 10 lines

drivers/ras/ras.c +10 -0

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

drivers/ras/ras.c void log_non_standard_event(const guid_t *sec_type, const guid_t *fru_id, · line 93

Quoted 6 lines

drivers/ras/ras.c +6 -0

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

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

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

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

Co-authored-by Jason Tian <[email protected]>

Co-authored-by Signed-off-by: Shengwei Luo <[email protected]>

Co-authored-by Daniel Ferguson <[email protected]>

Signed-off-by Jason Tian <[email protected]>

Signed-off-by Shengwei Luo <[email protected]>

Signed-off-by Daniel Ferguson <[email protected]>

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 <[email protected]>

---
 drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
 include/linux/cper.h        |  2 ++
 2 files changed, 45 insertions(+)

drivers/firmware/efi/cper.c +43 -0

index 7d2cdd9e2227..462d739e8dd1 100644
@@ -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",

include/linux/cper.h +2 -0

index 265b0f8fc0b3..25858a7608b7 100644
@@ -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 *,

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 <[email protected]>

---
 drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
 include/linux/cper.h        |  2 ++
 2 files changed, 45 insertions(+)

drivers/firmware/efi/cper.c +0 -0

index 7d2cdd9e2227..462d739e8dd1 100644
@@ -106,6 +106,49 @@ void cper_print_bits(const char *pfx, unsigned int bits,
 		printk("%s\n", buf);
 }

drivers/firmware/efi/cper.c +5 -0

 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,

Quoted 2 lines

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?

Quoted 23 lines

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---
 drivers/firmware/efi/cper.c | 43 +++++++++++++++++++++++++++++++++++++
 include/linux/cper.h        |  2 ++
 2 files changed, 45 insertions(+)

drivers/firmware/efi/cper.c +8 -0

index 7d2cdd9e2227..462d739e8dd1 100644
@@ -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
+ *

drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 117

Quoted 1 line

drivers/firmware/efi/cper.c +1 -0

+ * Add to @buf the bitmask in hexadecimal.   

Where does it do that?

Quoted 6 lines

drivers/firmware/efi/cper.c +5 -0

 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,

drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 132

Quoted 10 lines

drivers/firmware/efi/cper.c +10 -0

+		     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()

drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 135

Quoted 3 lines

drivers/firmware/efi/cper.c +3 -0

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

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 <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

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

Quoted 29 lines

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.

Quoted 8 lines

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 <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

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