public inbox for linux-acpi@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata
@ 2024-06-20 18:01 Mauro Carvalho Chehab
  2024-06-20 18:01 ` [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs Mauro Carvalho Chehab
  2024-06-21  7:45 ` [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata Ard Biesheuvel
  0 siblings, 2 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2024-06-20 18:01 UTC (permalink / raw)
  Cc: Mauro Carvalho Chehab, Borislav Petkov, Tony Luck, James Morse,
	Jonathan Cameron, Shiju Jose, linux-efi, linux-kernel, linux-edac,
	Ard Biesheuvel, Len Brown, linux-acpi

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 corrupted

And 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 error

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

-- 
2.45.2



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs
  2024-06-20 18:01 [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata Mauro Carvalho Chehab
@ 2024-06-20 18:01 ` Mauro Carvalho Chehab
  2024-06-21  9:30   ` Jonathan Cameron
  2024-06-21  7:45 ` [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata Ard Biesheuvel
  1 sibling, 1 reply; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2024-06-20 18:01 UTC (permalink / raw)
  Cc: Mauro Carvalho Chehab, Borislav Petkov, James Morse,
	Jonathan Cameron, Rafael J. Wysocki, Shiju Jose, Tony Luck,
	Uwe Kleine-König, Alison Schofield, Ard Biesheuvel,
	Dan Williams, Dave Jiang, Ira Weiny, Len Brown, Shuai Xue,
	linux-acpi, linux-edac, linux-efi, linux-kernel

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

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

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 <mchehab+huawei@kernel.org>
---
 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(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 623cc0cb4a65..093a2d0e49e7 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;
@@ -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",
diff --git a/drivers/firmware/efi/cper-arm.c b/drivers/firmware/efi/cper-arm.c
index d9bbcea0adf4..4c101a09fd80 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[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);
 		}
 
-		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);
+		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);
diff --git a/include/linux/cper.h b/include/linux/cper.h
index 856e8f00a7fb..4fef462944d6 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata
  2024-06-20 18:01 [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata Mauro Carvalho Chehab
  2024-06-20 18:01 ` [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs Mauro Carvalho Chehab
@ 2024-06-21  7:45 ` Ard Biesheuvel
  2024-06-21 15:26   ` Mauro Carvalho Chehab
  1 sibling, 1 reply; 6+ messages in thread
From: Ard Biesheuvel @ 2024-06-21  7:45 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Borislav Petkov, Tony Luck, James Morse, Jonathan Cameron,
	Shiju Jose, linux-efi, linux-kernel, linux-edac, Len Brown,
	linux-acpi

On Thu, 20 Jun 2024 at 20:01, Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> 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 corrupted
>
> And 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 error
>
> Mauro 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs
  2024-06-20 18:01 ` [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs Mauro Carvalho Chehab
@ 2024-06-21  9:30   ` Jonathan Cameron
  2024-06-21  9:47     ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2024-06-21  9:30 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Borislav Petkov, James Morse, Rafael J. Wysocki, Shiju Jose,
	Tony Luck, Uwe Kleine-König, Alison Schofield,
	Ard Biesheuvel, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
	Shuai Xue, linux-acpi, linux-edac, linux-efi, linux-kernel

On Thu, 20 Jun 2024 20:01:46 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> 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 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
> 
> See: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-information
> 
> 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 <mchehab+huawei@kernel.org>

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.

> diff --git a/drivers/firmware/efi/cper-arm.c b/drivers/firmware/efi/cper-arm.c
> index d9bbcea0adf4..4c101a09fd80 100644
> --- a/drivers/firmware/efi/cper-arm.c
> +++ b/drivers/firmware/efi/cper-arm.c
...

>  	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[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);
>  		}
>  
> -		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.


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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs
  2024-06-21  9:30   ` Jonathan Cameron
@ 2024-06-21  9:47     ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2024-06-21  9:47 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Borislav Petkov, James Morse, Rafael J. Wysocki, Shiju Jose,
	Tony Luck, Uwe Kleine-König, Alison Schofield,
	Ard Biesheuvel, Dan Williams, Dave Jiang, Ira Weiny, Len Brown,
	Shuai Xue, linux-acpi, linux-edac, linux-efi, linux-kernel

Em Fri, 21 Jun 2024 10:30:50 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> On Thu, 20 Jun 2024 20:01:46 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > 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 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
> > 
> > See: https://uefi.org/specs/UEFI/2.10/Apx_N_Common_Platform_Error_Record.html#arm-processor-error-information
> > 
> > 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 <mchehab+huawei@kernel.org>  
> 
> 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.
> 
> > diff --git a/drivers/firmware/efi/cper-arm.c b/drivers/firmware/efi/cper-arm.c
> > index d9bbcea0adf4..4c101a09fd80 100644
> > --- a/drivers/firmware/efi/cper-arm.c
> > +++ b/drivers/firmware/efi/cper-arm.c  
> ...
> 
> >  	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[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);
> >  		}
> >  
> > -		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.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata
  2024-06-21  7:45 ` [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata Ard Biesheuvel
@ 2024-06-21 15:26   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 6+ messages in thread
From: Mauro Carvalho Chehab @ 2024-06-21 15:26 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Borislav Petkov, Tony Luck, James Morse, Jonathan Cameron,
	Shiju Jose, linux-efi, linux-kernel, linux-edac, Len Brown,
	linux-acpi

Hi Ard,

Em Fri, 21 Jun 2024 09:45:16 +0200
Ard Biesheuvel <ardb@kernel.org> escreveu:

> On Thu, 20 Jun 2024 at 20:01, Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > 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 corrupted
> >
> > And 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 error
> >
> > Mauro 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.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-06-21 15:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-20 18:01 [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata Mauro Carvalho Chehab
2024-06-20 18:01 ` [PATCH v4 3/3] efi/cper: align ARM CPER type with UEFI 2.9A/2.10 specs Mauro Carvalho Chehab
2024-06-21  9:30   ` Jonathan Cameron
2024-06-21  9:47     ` Mauro Carvalho Chehab
2024-06-21  7:45 ` [PATCH v4 0/3] Fix CPER issues related to UEFI 2.9A Errata Ard Biesheuvel
2024-06-21 15:26   ` Mauro Carvalho Chehab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox