Fix CPER issues related to UEFI 2.9A Errata
v4 cover Fix CPER issues related to UEFI 2.9A Errata
The UEFI 2.9A errata makes clear how ARM processor type encoding should be done: it is meant to be equal to Generic processor, using a bitmask.
The current code assumes, for both generic and ARM processor types that this is an integer, which is an incorrect assumption.
Fix it. While here, also fix a compilation issue when using W=1.
After the change, Kernel will properly decode receiving two errors at the same message, as defined at UEFI spec:
[ 75.282430] Memory failure: 0x5cdfd: recovery action for free buddy page: Recovered
[ 94.973081] {2}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1
[ 94.973770] {2}[Hardware Error]: event severity: recoverable
[ 94.974334] {2}[Hardware Error]: Error 0, type: recoverable
[ 94.974962] {2}[Hardware Error]: section_type: ARM processor error
[ 94.975586] {2}[Hardware Error]: MIDR: 0x000000000000cd24
[ 94.976202] {2}[Hardware Error]: Multiprocessor Affinity Register (MPIDR): 0x000000000000ab12
[ 94.977011] {2}[Hardware Error]: error affinity level: 2
[ 94.977593] {2}[Hardware Error]: running state: 0x1
[ 94.978135] {2}[Hardware Error]: Power State Coordination Interface state: 4660
[ 94.978884] {2}[Hardware Error]: Error info structure 0:
[ 94.979463] {2}[Hardware Error]: num errors: 3
[ 94.979971] {2}[Hardware Error]: first error captured
[ 94.980523] {2}[Hardware Error]: propagated error captured
[ 94.981110] {2}[Hardware Error]: overflow occurred, error info is incomplete
[ 94.981893] {2}[Hardware Error]: error_type: 0x0006: cache error|TLB error
[ 94.982606] {2}[Hardware Error]: error_info: 0x000000000091000f
[ 94.983249] {2}[Hardware Error]: transaction type: Data Access
[ 94.983891] {2}[Hardware Error]: cache error, operation type: Data write
[ 94.984559] {2}[Hardware Error]: TLB error, operation type: Data write
[ 94.985215] {2}[Hardware Error]: cache level: 2
[ 94.985749] {2}[Hardware Error]: TLB level: 2
[ 94.986277] {2}[Hardware Error]: processor context not corruptedAnd the error code is properly decoded according with table N.17 from UEFI 2.10 spec:
[ 94.981893] {2}[Hardware Error]: error_type: 0x0006: cache error|TLB errorMauro Carvalho Chehab (3):
efi/cper: Adjust infopfx size to accept an extra space efi/cper: Add a new helper function to print bitmasks efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs
drivers/acpi/apei/ghes.c | 10 +++--- drivers/firmware/efi/cper-arm.c | 48 ++++++++++++--------------- drivers/firmware/efi/cper.c | 59 +++++++++++++++++++++++++++++++++ include/linux/cper.h | 13 +++++--- 4 files changed, 94 insertions(+), 36 deletions(-)
Signature · 1 line
2.45.2
PATCH v4 1/3 efi/cper: Adjust infopfx size to accept an extra space
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.
--- drivers/firmware/efi/cper-arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/cper-arm.c +1 -1
@@ -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[65 ];printk ("%sMIDR: 0x%016llx\n" , pfx, proc->midr);
Signature · 1 line
2.45.2
Re: [PATCH v4 1/3] efi/cper: Adjust infopfx size to accept an extra space
Quoted 7 lines · On Thu, 20 Jun 2024 20:01:44 +0200 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
Quoted 7 lines
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 <[email protected]>
Trivial suggestion inline. Either way LGTM
Quoted 9 lines
--- drivers/firmware/efi/cper-arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/firmware/efi/cper-arm.c +0
-0
drivers/firmware/efi/cper-arm.c void cper_print_proc_arm(const char *pfx, · line 243
Quoted 5 lines
drivers/firmware/efi/cper-arm.c +1
-1@@ -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[65 ];
Maybe make it explicit so we don't wonder if it was a typo in future. Something like?
char newpfx[64]; char infofx[ARRAY_SIZE(newpfx) + 1];
Quoted 3 lines
drivers/firmware/efi/cper-arm.c +0
-0printk ("%sMIDR: 0x%016llx\n" , pfx, proc->midr);
PATCH v4 2/3 efi/cper: Add a new helper function to print bitmasks
In reply to Mauro Carvalho ChehabSometimes it is desired to produce a single log line for errors. Add a new helper function for such purpose.
--- drivers/firmware/efi/cper.c | 59 +++++++++++++++++++++++++++++++++++++ include/linux/cper.h | 3 ++ 2 files changed, 62 insertions(+)
drivers/firmware/efi/cper.c +59 -0
@@ -106,6 +106,65 @@ 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 in @strs to @buf. +*/ +char *cper_bits_to_str (char *buf,int buf_size,unsigned int bits, +const char *const strs[],unsigned int strs_size, +unsigned int mask) +{ +int i, size, first_bit; +int len = buf_size; +const char *start; +char *str = buf; + +if (strs_size <16 ) + size =snprintf (str, len,"0x%02x: " , bits); +if (strs_size <32 ) + size =snprintf (str, len,"0x%04x: " , bits); + + len -= size; + str += size; + + start = str; + +if (mask) { + first_bit =ffs (mask) -1 ; +if (bits & ~mask) { + size =strscpy (str,"reserved bit(s)" , len); + len -= size; + str += size; + } + }else { + first_bit =0 ; + } + +for (i =0 ; i < strs_size; i++) { +if (!(bits & (1U << (i + first_bit)))) +continue ; + +if (*start && len >0 ) { + *str ='|' ; + len--; + str++; + } + + size =strscpy (str, strs[i], len); + len -= size; + str += size; + } +return buf; +} +EXPORT_SYMBOL_GPL (cper_bits_to_str); +static const char *const proc_type_strs[] = {"IA32/X64" ,"IA64" ,
include/linux/cper.h +3 -0
@@ -584,6 +584,9 @@ 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); +char *cper_bits_to_str (char *buf,int buf_size,unsigned int bits, +const char *const strs[],unsigned int strs_size, +unsigned int mask);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 *,
Signature · 1 line
2.45.2
Luck, Tony
maintainerRE: [PATCH v4 2/3] efi/cper: Add a new helper function to print bitmasks
+
+ size = strscpy(str, strs[i], len);
Check for error return from strscpy()
+ len -= size;
+ str += size;
Re: [PATCH v4 2/3] efi/cper: Add a new helper function to print bitmasks
In reply to Mauro Carvalho ChehabQuoted 14 lines · On Thu, 20 Jun 2024 20:01:45 +0200 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 <[email protected]>--- drivers/firmware/efi/cper.c | 59 +++++++++++++++++++++++++++++++++++++ include/linux/cper.h | 3 ++ 2 files changed, 62 insertions(+)drivers/firmware/efi/cper.c +0
-0
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 109
Quoted 4 lines
drivers/firmware/efi/cper.c +1
-0@@ -106,6 +106,65 @@ void cper_print_bits(const char *pfx, unsigned int bits,printk ("%s\n" , buf); } +/*
It's exported and in a header used by other code, so why not make this kernel-doc? /**
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 115
Quoted 6 lines
drivers/firmware/efi/cper.c +6
-0+* 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
If it had been kernel doc, W=1 would have told you mask is missing.
Passing a 0 for mask seems probably not worth while. If all bits of the unsigned int are set then people can pass ~0
Or make this cper_bits_to_str_masked() and have cper_bits_to_str() that doesn't take a mask.
If you do that, some simplifications can be easily made.
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 120
Quoted 5 lines
drivers/firmware/efi/cper.c +5
-0+* +* add to @buf the bitmask in hexadecimal. Then, for each set bit in @bits, +* add the corresponding string in @strs to @buf. +*/ +char *cper_bits_to_str(char *buf, int buf_size, unsigned int bits,
Perhaps make bits an unsigned long as then you can use the for_each_set_bit() etc.
Quoted 18 lines
drivers/firmware/efi/cper.c +18
-0+ const char * const strs[], unsigned int strs_size, + unsigned int mask) +{ + int i, size, first_bit; +int len = buf_size; +const char *start; +char *str = buf; + +if (strs_size <16 ) + size =snprintf (str, len,"0x%02x: " , bits); +if (strs_size <32 ) + size =snprintf (str, len,"0x%04x: " , bits); + + len -= size; + str += size; + + start = str; +
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 148
Quoted 10 lines
drivers/firmware/efi/cper.c +10
-0+if (mask) { + first_bit =ffs (mask) -1 ; +if (bits & ~mask) { + size =strscpy (str,"reserved bit(s)" , len); + len -= size; + str += size; + } + }else { + first_bit =0 ; + }
Might be worth
bits = bits & mask;
Obviously setting bits that aren't in the mask is odd though so maybe a warning print if that happens?
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 149
Quoted 1 line
drivers/firmware/efi/cper.c +1
-0+
for_each_bit_set(i, &bits, strs_size) {
...}
Quoted 35 lines
drivers/firmware/efi/cper.c +18
-0+for (i =0 ; i < strs_size; i++) { +if (!(bits & (1U << (i + first_bit)))) +continue ; + +if (*start && len >0 ) { + *str ='|' ; + len--; + str++; + } + + size =strscpy (str, strs[i], len); + len -= size; + str += size; + } +return buf; +} +EXPORT_SYMBOL_GPL (cper_bits_to_str); +static const char *const proc_type_strs[] = {"IA32/X64" ,"IA64" ,include/linux/cper.h +3
-0@@ -584,6 +584,9 @@ 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); +char *cper_bits_to_str (char *buf,int buf_size,unsigned int bits, +const char *const strs[],unsigned int strs_size, +unsigned int mask);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 v4 2/3] efi/cper: Add a new helper function to print bitmasks
Quoted 32 lines · On Fri, 21 Jun 2024 10:20:36 +0100 Jonathan Cameron…
Quoted 14 lines · On Thu, 20 Jun 2024 20:01:45 +0200 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 <[email protected]>--- drivers/firmware/efi/cper.c | 59 +++++++++++++++++++++++++++++++++++++ include/linux/cper.h | 3 ++ 2 files changed, 62 insertions(+)drivers/firmware/efi/cper.c +0
-0drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 109
Quoted 4 lines
drivers/firmware/efi/cper.c +1
-0@@ -106,6 +106,65 @@ void cper_print_bits(const char *pfx, unsigned int bits,printk ("%s\n" , buf); } +/* It's exported and in a header used by other code, so why not make this kernel-doc? /**
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 115
Quoted 6 lines
drivers/firmware/efi/cper.c +6
-0+* 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
Quoted 9 lines
If it had been kernel doc, W=1 would have told you mask is missing.
Passing a 0 for mask seems probably not worth while. If all bits of the unsigned int are set then people can pass ~0
Or make this
cper_bits_to_str_masked()and havecper_bits_to_str()that doesn't take a mask.
Mask definitely needs docs as I misunderstood it :( Also needs to be contiguous I think which is also a bit unusual.
Quoted 43 lines
If you do that, some simplifications can be easily made.
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 120
Quoted 5 lines
drivers/firmware/efi/cper.c +5
-0+* +* add to @buf the bitmask in hexadecimal. Then, for each set bit in @bits, +* add the corresponding string in @strs to @buf. +*/ +char *cper_bits_to_str(char *buf, int buf_size, unsigned int bits,Perhaps make bits an unsigned long as then you can use the
for_each_set_bit()etc.Quoted 18 lines
drivers/firmware/efi/cper.c +18
-0+ const char * const strs[], unsigned int strs_size, + unsigned int mask) +{ + int i, size, first_bit; +int len = buf_size; +const char *start; +char *str = buf; + +if (strs_size <16 ) + size =snprintf (str, len,"0x%02x: " , bits); +if (strs_size <32 ) + size =snprintf (str, len,"0x%04x: " , bits); + + len -= size; + str += size; + + start = str; +drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 148
Quoted 10 lines
drivers/firmware/efi/cper.c +10
-0+if (mask) { + first_bit =ffs (mask) -1 ; +if (bits & ~mask) { + size =strscpy (str,"reserved bit(s)" , len); + len -= size; + str += size; + } + }else { + first_bit =0 ; + }Might be worth
Quoted 8 lines
bits = bits & mask;Obviously setting bits that aren't in the mask is odd though so maybe a warning print if that happens?
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 149
Quoted 1 line
drivers/firmware/efi/cper.c +1
-0+
for_each_bit_set(i, &bits, strs_size){
Ah. I'd missed the offset and gotten function name wrong.
i = first_bitfor_each_set_bit_from(i, &bits, strs_size + first_bit)
and look up based on i - first_bit
Quoted 40 lines
...}
Quoted 35 lines
drivers/firmware/efi/cper.c +18
-0+for (i =0 ; i < strs_size; i++) { +if (!(bits & (1U << (i + first_bit)))) +continue ; + +if (*start && len >0 ) { + *str ='|' ; + len--; + str++; + } + + size =strscpy (str, strs[i], len); + len -= size; + str += size; + } +return buf; +} +EXPORT_SYMBOL_GPL (cper_bits_to_str); +static const char *const proc_type_strs[] = {"IA32/X64" ,"IA64" ,include/linux/cper.h +3
-0@@ -584,6 +584,9 @@ 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); +char *cper_bits_to_str (char *buf,int buf_size,unsigned int bits, +const char *const strs[],unsigned int strs_size, +unsigned int mask);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 v4 2/3] efi/cper: Add a new helper function to print bitmasks
In reply to Jonathan CameronEm Fri, 21 Jun 2024 10:20:36 +0100
Jonathan Cameron <[email protected]> escreveu:
Quoted 20 lines
Quoted 17 lines · On Thu, 20 Jun 2024 20:01:45 +0200 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 <[email protected]>--- drivers/firmware/efi/cper.c | 59 +++++++++++++++++++++++++++++++++++++ include/linux/cper.h | 3 ++ 2 files changed, 62 insertions(+)drivers/firmware/efi/cper.c +0
-0@@ -106,6 +106,65 @@ void cper_print_bits(const char *pfx, unsigned int bits,printk ("%s\n" , buf); }
Quoted 4 lines
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 109
Quoted 1 line
drivers/firmware/efi/cper.c +1
-0+/* It's exported and in a header used by other code, so why not make this kernel-doc? /**
I tried to preserve the original non-kernel-doc way, as I'm not sure why other comments on this file are not marked as kernel-doc stuff.
The code there at cper.c also has other coding style issues - for instance it uses printk() without an error level.
Anyway, I intend to submit later on a separate patch series converting the existing function documentation stuff to kernel-doc (and adding to Documentation if not there already), and maybe addressing some other coding style issues.
Yet, I would prefer to have such changes out of this fix patch series.
Quoted 9 lines
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 115
Quoted 6 lines
drivers/firmware/efi/cper.c +6
-0+* 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 If it had been kernel doc, W=1 would have told you mask is missing.
Yeah, I saw that just after hitting send :-) I'll add mask at v5.
Quoted 2 lines
Passing a 0 for mask seems probably not worth while. If all bits of the unsigned int are set then people can pass ~0
Makes sense.
Quoted 8 lines
Or make this
cper_bits_to_str_masked()and havecper_bits_to_str()that doesn't take a mask.If you do that, some simplifications can be easily made.
Quoted 8 lines
drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 120
Quoted 5 lines
drivers/firmware/efi/cper.c +5
-0+* +* add to @buf the bitmask in hexadecimal. Then, for each set bit in @bits, +* add the corresponding string in @strs to @buf. +*/ +char *cper_bits_to_str(char *buf, int buf_size, unsigned int bits,Perhaps make bits an unsigned long as then you can use the
for_each_set_bit()etc.
Ok.
Quoted 31 lines
Quoted 19 lines
drivers/firmware/efi/cper.c +18
-0+ const char * const strs[], unsigned int strs_size, + unsigned int mask) +{ + int i, size, first_bit; +int len = buf_size; +const char *start; +char *str = buf; + +if (strs_size <16 ) + size =snprintf (str, len,"0x%02x: " , bits); +if (strs_size <32 ) + size =snprintf (str, len,"0x%04x: " , bits); + + len -= size; + str += size; + + start = str; +drivers/firmware/efi/cper.c void cper_print_bits(const char *pfx, unsigned int bits, · line 148
Quoted 10 lines
drivers/firmware/efi/cper.c +10
-0+if (mask) { + first_bit =ffs (mask) -1 ; +if (bits & ~mask) { + size =strscpy (str,"reserved bit(s)" , len); + len -= size; + str += size; + } + }else { + first_bit =0 ; + }Might be worth
Quoted 1 line
bits = bits & mask;
No need to to that if we use for_each_set_bit().
Quoted 3 lines
Obviously setting bits that aren't in the mask is odd though so maybe a warning print if that happens?
The code already warns about that printing:
"reserved bit(s)"
at the output buffer.
Regards,
Mauro
PATCH v4 3/3 efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs
In reply to Mauro Carvalho ChehabUp to UEFI spec, 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.
--- drivers/acpi/apei/ghes.c | 10 ++++--- drivers/firmware/efi/cper-arm.c | 46 ++++++++++++++------------------- include/linux/cper.h | 10 +++---- 3 files changed, 31 insertions(+), 35 deletions(-)
drivers/acpi/apei/ghes.c +6 -4
@@ -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;
@@ -546,9 +547,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
@@ -562,8 +562,10 @@ 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), err_info->type, + cper_proc_error_type_strs, +ARRAY_SIZE (cper_proc_error_type_strs), +CPER_ARM_ERR_TYPE_MASK ); pr_warn_ratelimited(FW_WARN GHES_PFX "Unhandled processor error type: %s\n",
drivers/firmware/efi/cper-arm.c +20 -26
93 lines not on this page · raw message
include/linux/cper.h +5 -5
@@ -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)
Signature · 1 line
2.45.2
Re: [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs
Quoted 24 lines · On Thu, 20 Jun 2024 20:01:46 +0200 Mauro Carvalho Chehab…
Up to UEFI spec, 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 reservedYet, 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 reservedThat actually aligns with the values already defined on older versions at N.2.4.1. Generic Processor Error Section.
Quoted 8 lines
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.
Signed-off-by Mauro Carvalho Chehab <[email protected]>
I think you can avoid complexity of your masking solution. Cost is we don't have that function print that there were reserved bits set, but that could be easily handled at the caller including notifying on bits above the defined range which might be helpful.
Quoted 4 lines
drivers/firmware/efi/cper-arm.c +0
-0
...
Quoted 10 lines
drivers/firmware/efi/cper-arm.c +1
-0if (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[65 ]; +char error_type[120 ];printk ("%sMIDR: 0x%016llx\n" , pfx, proc->midr);
drivers/firmware/efi/cper-arm.c void cper_print_proc_arm(const char *pfx, · line 287
Quoted 10 lines
drivers/firmware/efi/cper-arm.c +4
-3@@ -289,9 +281,11 @@ 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), err_info->type, + cper_proc_error_type_strs, +ARRAY_SIZE (cper_proc_error_type_strs), +CPER_ARM_ERR_TYPE_MASK );
Maybe drop this mask complexity and just use FIELD_GET() to extract the relevant field with no shift from 0.
Quoted 4 lines
drivers/firmware/efi/cper-arm.c +1
-0+printk ("%serror_type: %s\n" , newpfx, error_type);if (err_info->validation_bits &CPER_ARM_INFO_VALID_ERR_INFO ) {printk ("%serror_info: 0x%016llx\n" , newpfx, err_info->error_info);
Re: [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs
Em Fri, 21 Jun 2024 10:30:50 +0100
Jonathan Cameron <[email protected]> escreveu:
Quoted 60 lines
Quoted 24 lines · On Thu, 20 Jun 2024 20:01:46 +0200 Mauro Carvalho Chehab…
Up to UEFI spec, 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 reservedYet, 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 reservedThat actually aligns with the values already defined on older versions at N.2.4.1. Generic Processor Error Section.
Quoted 8 lines
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.
Signed-off-by Mauro Carvalho Chehab <[email protected]>I think you can avoid complexity of your masking solution. Cost is we don't have that function print that there were reserved bits set, but that could be easily handled at the caller including notifying on bits above the defined range which might be helpful.
Quoted 4 lines
drivers/firmware/efi/cper-arm.c +0
-0...
Quoted 13 lines
drivers/firmware/efi/cper-arm.c +1
-0if (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[65 ]; +char error_type[120 ];printk ("%sMIDR: 0x%016llx\n" , pfx, proc->midr);@@ -289,9 +281,11 @@ void cper_print_proc_arm(const char *pfx, newpfx); }
Quoted 10 lines
drivers/firmware/efi/cper-arm.c void cper_print_proc_arm(const char *pfx, · line 287
Quoted 7 lines
drivers/firmware/efi/cper-arm.c +4
-3-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), err_info->type, + cper_proc_error_type_strs, +ARRAY_SIZE (cper_proc_error_type_strs), +CPER_ARM_ERR_TYPE_MASK );Maybe drop this mask complexity and just use
FIELD_GET()to extract the relevant field with no shift from 0.
IMO not using the function will make the code here more complex, as the same code needs to be duplicated on two places: here and at ghes, where the error bits are printed using pr_warn_ratelimited():
cper_bits_to_str(error_type, sizeof(error_type), err_info->type,
cper_proc_error_type_strs,
ARRAY_SIZE(cper_proc_error_type_strs),
CPER_ARM_ERR_TYPE_MASK);
pr_warn_ratelimited(FW_WARN GHES_PFX
"Unhandled processor error type: %s\n",Also, other parts of CPER uses cper_bits_print() for the same reason:
to have the common print code handled inside a function instead of repeating the same print pattern everywhere.
Quoted 6 lines
drivers/firmware/efi/cper-arm.c void cper_print_proc_arm(const char *pfx, · line 291
Quoted 4 lines
drivers/firmware/efi/cper-arm.c +1
-0+printk ("%serror_type: %s\n" , newpfx, error_type);if (err_info->validation_bits &CPER_ARM_INFO_VALID_ERR_INFO ) {printk ("%serror_info: 0x%016llx\n" , newpfx, err_info->error_info);
Regards,
Mauro
Ard Biesheuvel
maintainerQuoted 39 lines · Mauro Carvalho Chehab
The UEFI 2.9A errata makes clear how ARM processor type encoding should be done: it is meant to be equal to Generic processor, using a bitmask.
The current code assumes, for both generic and ARM processor types that this is an integer, which is an incorrect assumption.
Fix it. While here, also fix a compilation issue when using W=1.
After the change, Kernel will properly decode receiving two errors at the same message, as defined at UEFI spec:
[ 75.282430] Memory failure: 0x5cdfd: recovery action for free buddy page: Recovered [ 94.973081] {2}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 [ 94.973770] {2}[Hardware Error]: event severity: recoverable [ 94.974334] {2}[Hardware Error]: Error 0, type: recoverable [ 94.974962] {2}[Hardware Error]: section_type: ARM processor error [ 94.975586] {2}[Hardware Error]: MIDR: 0x000000000000cd24 [ 94.976202] {2}[Hardware Error]: Multiprocessor Affinity Register (MPIDR): 0x000000000000ab12 [ 94.977011] {2}[Hardware Error]: error affinity level: 2 [ 94.977593] {2}[Hardware Error]: running state: 0x1 [ 94.978135] {2}[Hardware Error]: Power State Coordination Interface state: 4660 [ 94.978884] {2}[Hardware Error]: Error info structure 0: [ 94.979463] {2}[Hardware Error]: num errors: 3 [ 94.979971] {2}[Hardware Error]: first error captured [ 94.980523] {2}[Hardware Error]: propagated error captured [ 94.981110] {2}[Hardware Error]: overflow occurred, error info is incomplete [ 94.981893] {2}[Hardware Error]: error_type: 0x0006: cache error|TLB error [ 94.982606] {2}[Hardware Error]: error_info: 0x000000000091000f [ 94.983249] {2}[Hardware Error]: transaction type: Data Access [ 94.983891] {2}[Hardware Error]: cache error, operation type: Data write [ 94.984559] {2}[Hardware Error]: TLB error, operation type: Data write [ 94.985215] {2}[Hardware Error]: cache level: 2 [ 94.985749] {2}[Hardware Error]: TLB level: 2 [ 94.986277] {2}[Hardware Error]: processor context not corruptedAnd the error code is properly decoded according with table N.17 from UEFI 2.10 spec:
Quoted 7 lines
[ 94.981893] {2}[Hardware Error]: error_type: 0x0006: cache error|TLB errorMauro Carvalho Chehab (3):
efi/cper: Adjust infopfx size to accept an extra space efi/cper: Add a new helper function to print bitmasks efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs
Hello Mauro,
How this is v4 different from the preceding 3 revisions that you sent over the past 2 days?
I would expect an experienced maintainer like yourself to be familiar with the common practice here: please leave some time between sending revisions so people can take a look. And if there is a pressing need to deviate from this rule, at least put an explanation in the commit log of how the series differs from the preceding one.
Thanks,
Ard.
Hi Ard,
Em Fri, 21 Jun 2024 09:45:16 +0200
Ard Biesheuvel <[email protected]> escreveu:
Quoted 51 lines
Quoted 39 lines · Mauro Carvalho Chehab
The UEFI 2.9A errata makes clear how ARM processor type encoding should be done: it is meant to be equal to Generic processor, using a bitmask.
The current code assumes, for both generic and ARM processor types that this is an integer, which is an incorrect assumption.
Fix it. While here, also fix a compilation issue when using W=1.
After the change, Kernel will properly decode receiving two errors at the same message, as defined at UEFI spec:
[ 75.282430] Memory failure: 0x5cdfd: recovery action for free buddy page: Recovered [ 94.973081] {2}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1 [ 94.973770] {2}[Hardware Error]: event severity: recoverable [ 94.974334] {2}[Hardware Error]: Error 0, type: recoverable [ 94.974962] {2}[Hardware Error]: section_type: ARM processor error [ 94.975586] {2}[Hardware Error]: MIDR: 0x000000000000cd24 [ 94.976202] {2}[Hardware Error]: Multiprocessor Affinity Register (MPIDR): 0x000000000000ab12 [ 94.977011] {2}[Hardware Error]: error affinity level: 2 [ 94.977593] {2}[Hardware Error]: running state: 0x1 [ 94.978135] {2}[Hardware Error]: Power State Coordination Interface state: 4660 [ 94.978884] {2}[Hardware Error]: Error info structure 0: [ 94.979463] {2}[Hardware Error]: num errors: 3 [ 94.979971] {2}[Hardware Error]: first error captured [ 94.980523] {2}[Hardware Error]: propagated error captured [ 94.981110] {2}[Hardware Error]: overflow occurred, error info is incomplete [ 94.981893] {2}[Hardware Error]: error_type: 0x0006: cache error|TLB error [ 94.982606] {2}[Hardware Error]: error_info: 0x000000000091000f [ 94.983249] {2}[Hardware Error]: transaction type: Data Access [ 94.983891] {2}[Hardware Error]: cache error, operation type: Data write [ 94.984559] {2}[Hardware Error]: TLB error, operation type: Data write [ 94.985215] {2}[Hardware Error]: cache level: 2 [ 94.985749] {2}[Hardware Error]: TLB level: 2 [ 94.986277] {2}[Hardware Error]: processor context not corruptedAnd the error code is properly decoded according with table N.17 from UEFI 2.10 spec:
Quoted 7 lines
[ 94.981893] {2}[Hardware Error]: error_type: 0x0006: cache error|TLB errorMauro Carvalho Chehab (3):
efi/cper: Adjust infopfx size to accept an extra space efi/cper: Add a new helper function to print bitmasks efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specsHello Mauro,
Quoted 8 lines
How this is v4 different from the preceding 3 revisions that you sent over the past 2 days?
I would expect an experienced maintainer like yourself to be familiar with the common practice here: please leave some time between sending revisions so people can take a look. And if there is a pressing need to deviate from this rule, at least put an explanation in the commit log of how the series differs from the preceding one.
Sorry, I'll add a version review on that. Basically I was missing a test environment to do error injection. When I got it enabled, and fixed to cope with UEFI 2.9A/2.10 expected behavior, I was able to discover some issues and to do some code improvements.
v1:
- (tagged as RFC) was mostly to give a heads up that the current implementation is not following the spec. It also touches only cper code.
v2:
- It fixes the way printks are handled on both cper_arm and ghes drivers;
v3:
- It adds a helper function to produce a buffer describing the error bits at cper's printk and ghes pr_warn_bitrated. It also fixes a W=1 error while building cper;
v4:
- The print function had some bugs on it, which was discovered with the help of an error injection tool I'm now using.
I have already another version ready to send. It does some code cleanup and address the issues pointed by Tony and Jonathan. If you prefer, I can hold it until Monday to give you some time to look at it.
Thanks,
Mauro