diff mbox series

[v7,6/9] ACPI: APEI: EINJ: Add einjv2 extension struct

Message ID 20250506213814.2365788-7-zaidal@os.amperecomputing.com
State New
Headers show
Series Enable EINJv2 Support | expand

Commit Message

Zaid Alali May 6, 2025, 9:38 p.m. UTC 
Add einjv2 extension struct and EINJv2 error types to prepare
the driver for EINJv2 support. ACPI specifications[1] enables
EINJv2 by extending set_error_type_with_address struct.

Link: https://github.com/tianocore/edk2/issues/9449 [1]

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Zaid Alali <zaidal@os.amperecomputing.com>
---
 drivers/acpi/apei/einj-core.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Luck, Tony May 28, 2025, 6:38 p.m. UTC | #1 
On Tue, May 06, 2025 at 02:38:10PM -0700, Zaid Alali wrote:
> Add einjv2 extension struct and EINJv2 error types to prepare
> the driver for EINJv2 support. ACPI specifications[1] enables
> EINJv2 by extending set_error_type_with_address struct.
> 
> Link: https://github.com/tianocore/edk2/issues/9449 [1]
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Zaid Alali <zaidal@os.amperecomputing.com>
> ---
>  drivers/acpi/apei/einj-core.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> index ee26df0398fc..60e4f3dc7055 100644
> --- a/drivers/acpi/apei/einj-core.c
> +++ b/drivers/acpi/apei/einj-core.c
> @@ -50,6 +50,28 @@
>   */
>  static int acpi5;
>  
> +struct syndrome_array {
> +	union {
> +		u8	acpi_id[16];
> +		u8	device_id[16];
> +		u8	pcie_sbdf[16];
> +		u8	vendor_id[16];
> +	} comp_id;
> +	union {
> +		u8	proc_synd[16];
> +		u8	mem_synd[16];
> +		u8	pcie_synd[16];
> +		u8	vendor_synd[16];
> +	} comp_synd;
> +};
> +
> +struct einjv2_extension_struct {
> +	u32 length;
> +	u16 revision;
> +	u16 component_arr_count;
> +	struct syndrome_array component_arr[] __counted_by(component_arr_count);
> +};
> +
>  struct set_error_type_with_address {
>  	u32	type;
>  	u32	vendor_extension;
> @@ -58,6 +80,7 @@ struct set_error_type_with_address {
>  	u64	memory_address;
>  	u64	memory_address_range;
>  	u32	pcie_sbdf;
> +	struct	einjv2_extension_struct einjv2_struct;

I can't make this match up with the ACPI v6.5 spec.  The spec defines
a whole new EINJV2_SET_ERROR_TYPE data structure in table 18.34 that
is NOT just a simple addition of new fields at the end of the existing
SET_ERROR_TYPE_WITH_ADDRESS data structure. E.g. the "flags" are now
in a 3-byte field at offset 5 instead of a 4-byte field at offset 8.
There is a new "length" field that descibes the total size of the
structure including the new flex array of syndrome values at the
end.

Shouldn't this look like this?

struct set_error_type_with_address_v2 {
	u32	type;
	u8	type_code;
	u8	flags[3];
	u32	length;
	u32	severity;
	u64	memory_address;
	u64	memory_address_range;
	u32	syndrome_count;
	struct syndrome_array syndrome syndromes[];
};

>  };
>  enum {
>  	SETWA_FLAGS_APICID = 1,
> -- 
> 2.43.0

-Tony
Luck, Tony May 29, 2025, 3:28 p.m. UTC | #2 
On Wed, May 28, 2025 at 11:38:54AM -0700, Luck, Tony wrote:
> On Tue, May 06, 2025 at 02:38:10PM -0700, Zaid Alali wrote:
> >  struct set_error_type_with_address {
> >  	u32	type;
> >  	u32	vendor_extension;
> > @@ -58,6 +80,7 @@ struct set_error_type_with_address {
> >  	u64	memory_address;
> >  	u64	memory_address_range;
> >  	u32	pcie_sbdf;
> > +	struct	einjv2_extension_struct einjv2_struct;
> 
> I can't make this match up with the ACPI v6.5 spec.  The spec defines
> a whole new EINJV2_SET_ERROR_TYPE data structure in table 18.34 that
> is NOT just a simple addition of new fields at the end of the existing
> SET_ERROR_TYPE_WITH_ADDRESS data structure. E.g. the "flags" are now
> in a 3-byte field at offset 5 instead of a 4-byte field at offset 8.
> There is a new "length" field that descibes the total size of the
> structure including the new flex array of syndrome values at the
> end.

Someone pointed me to the ACPI 6.5 Errata A spec: https://uefi.org/specs/ACPI/6.5_A/

This code does match with the description there.

I'll continue looking at your patches with this as the reference.

Please make sure to reference this spec directly (not buried in links
to tianocore bugzilla entries) when you post next version.

-Tony
diff mbox series

Patch

diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
index ee26df0398fc..60e4f3dc7055 100644
--- a/drivers/acpi/apei/einj-core.c
+++ b/drivers/acpi/apei/einj-core.c
@@ -50,6 +50,28 @@ 
  */
 static int acpi5;
 
+struct syndrome_array {
+	union {
+		u8	acpi_id[16];
+		u8	device_id[16];
+		u8	pcie_sbdf[16];
+		u8	vendor_id[16];
+	} comp_id;
+	union {
+		u8	proc_synd[16];
+		u8	mem_synd[16];
+		u8	pcie_synd[16];
+		u8	vendor_synd[16];
+	} comp_synd;
+};
+
+struct einjv2_extension_struct {
+	u32 length;
+	u16 revision;
+	u16 component_arr_count;
+	struct syndrome_array component_arr[] __counted_by(component_arr_count);
+};
+
 struct set_error_type_with_address {
 	u32	type;
 	u32	vendor_extension;
@@ -58,6 +80,7 @@  struct set_error_type_with_address {
 	u64	memory_address;
 	u64	memory_address_range;
 	u32	pcie_sbdf;
+	struct	einjv2_extension_struct einjv2_struct;
 };
 enum {
 	SETWA_FLAGS_APICID = 1,