[PATCH v13 0/4] cxl, EINJ: Update EINJ for CXL error types

[PATCH v13 0/4] cxl, EINJ: Update EINJ for CXL error types

[Ben Cheatham]

v13 Changes:
	- Create new einj-cxl module for EINJ CXL error type functionality
	- Rename CONFIG_CXL_EINJ to CONFIG_ACPI_APEI_EINJ_CXL
	- Move CONFIG_ACPI_APEI_CXL to be under CONFIG_ACPI_APEI_EINJ (due to
	  new CONFIG_CXL_BUS dependency)
	- Add an optional dependency to CONFIG_CXL_BUS on
	  CONFIG_ACPI_APEI_EINJ_CXL 
	- Change pr_warn("EINJ table not found.") to a pr_info() for when/if
	  EINJ probe fails (Tony)
	- Add a clarification that a CXL port needs to be present for CXL 
	  EINJ error types to einj.rst (Davidlohr)

v12 Changes:
	- Rebase onto v6.8-rc4
	- Squash Kconfig patch into patch 2/3 (Jonathan)
	- Change CONFIG_CXL_EINJ from "depends on ACPI_APEI_EINJ >= CXL_BUS"
	  to "depends on ACPI_APEI_EINJ = CXL_BUS"
	- Drop "ACPI, APEI" part of commit message title and use just EINJ
	instead (Dan)
	- Add protocol error types to "einj_types" documentation (Jonathan)
	- Change 0xffff... constants to use GENMASK()
	- Drop param* variables and use constants instead in cxl error
	  inject functions (Jonathan)
	- Add is_cxl_error_type() helper function in einj.c (Jonathan)
	- Remove a stray function declaration in einj-cxl.h (Jonathan)
	- Comment #else/#endifs with corresponding #if/#ifdef in
	einj-cxl.h (Jonathan)

The new CXL error types will use the Memory Address field in the
SET_ERROR_TYPE_WITH_ADDRESS structure in order to target a CXL 1.1
compliant memory-mapped downstream port. The value of the memory address
will be in the port's MMIO range, and it will not represent physical
(normal or persistent) memory.

Add the functionality for injecting CXL 1.1 errors to the EINJ module,
but not through the EINJ legacy interface under /sys/kernel/debug/apei/einj.
Instead, make the error types available under /sys/kernel/debug/cxl.
This allows for validating the MMIO address for a CXL 1.1 error type
while also not making the user responsible for finding it.

Ben Cheatham (4):
  EINJ: Migrate to a platform driver
  EINJ: Add CXL error type support
  cxl/core: Add CXL EINJ debugfs files
  EINJ, Documentation: Update EINJ kernel doc

 Documentation/ABI/testing/debugfs-cxl         |  30 +++++
 .../firmware-guide/acpi/apei/einj.rst         |  21 +++
 MAINTAINERS                                   |   1 +
 drivers/acpi/apei/Kconfig                     |  12 ++
 drivers/acpi/apei/Makefile                    |   1 +
 drivers/acpi/apei/apei-internal.h             |  17 +++
 drivers/acpi/apei/einj-cxl.c                  | 121 +++++++++++++++++
 drivers/acpi/apei/einj.c                      | 127 ++++++++++++++----
 drivers/cxl/Kconfig                           |   1 +
 drivers/cxl/core/port.c                       |  41 ++++++
 include/linux/einj-cxl.h                      |  40 ++++++
 11 files changed, 385 insertions(+), 27 deletions(-)
 create mode 100644 drivers/acpi/apei/einj-cxl.c
 create mode 100644 include/linux/einj-cxl.h

-- 
2.34.1

[PATCH v13 1/4] EINJ: Migrate to a platform driver

[Ben Cheatham]

Change the EINJ module to install a platform device/driver on module
init and move the module init() and exit() functions to driver probe and
remove. This change allows the EINJ module to load regardless of whether
setting up EINJ succeeds, which allows dependent modules to still load
(i.e. the CXL core).

Since EINJ may no longer be initialized when the module loads, any
functions that are called from dependent/external modules should check
the einj_initialized variable before calling any EINJ functions.

Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
---
 drivers/acpi/apei/einj.c | 46 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 89fb9331c611..6ea323b9d8ef 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -21,6 +21,7 @@
 #include <linux/nmi.h>
 #include <linux/delay.h>
 #include <linux/mm.h>
+#include <linux/platform_device.h>
 #include <asm/unaligned.h>
 
 #include "apei-internal.h"
@@ -137,6 +138,11 @@ static struct apei_exec_ins_type einj_ins_type[] = {
  */
 static DEFINE_MUTEX(einj_mutex);
 
+/*
+ * Exported APIs use this flag to exit early if einj_probe() failed.
+ */
+static bool einj_initialized __ro_after_init;
+
 static void *einj_param;
 
 static void einj_exec_ctx_init(struct apei_exec_context *ctx)
@@ -703,7 +709,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab)
 	return 0;
 }
 
-static int __init einj_init(void)
+static int __init einj_probe(struct platform_device *pdev)
 {
 	int rc;
 	acpi_status status;
@@ -717,7 +723,7 @@ static int __init einj_init(void)
 	status = acpi_get_table(ACPI_SIG_EINJ, 0,
 				(struct acpi_table_header **)&einj_tab);
 	if (status == AE_NOT_FOUND) {
-		pr_warn("EINJ table not found.\n");
+		pr_info("EINJ table not found.\n");
 		return -ENODEV;
 	} else if (ACPI_FAILURE(status)) {
 		pr_err("Failed to get EINJ table: %s\n",
@@ -805,7 +811,7 @@ static int __init einj_init(void)
 	return rc;
 }
 
-static void __exit einj_exit(void)
+static void __exit einj_remove(struct platform_device *pdev)
 {
 	struct apei_exec_context ctx;
 
@@ -826,6 +832,40 @@ static void __exit einj_exit(void)
 	acpi_put_table((struct acpi_table_header *)einj_tab);
 }
 
+static struct platform_device *einj_dev;
+struct platform_driver einj_driver = {
+	.remove_new = einj_remove,
+	.driver = {
+		.name = "acpi-einj",
+	},
+};
+
+static int __init einj_init(void)
+{
+	struct platform_device_info einj_dev_info = {
+		.name = "acpi-einj",
+		.id = -1,
+	};
+	int rc;
+
+	einj_dev = platform_device_register_full(&einj_dev_info);
+	if (IS_ERR_OR_NULL(einj_dev))
+		return PTR_ERR(einj_dev);
+
+	rc = platform_driver_probe(&einj_driver, einj_probe);
+	einj_initialized = rc == 0;
+
+	return 0;
+}
+
+static void __exit einj_exit(void)
+{
+	if (einj_initialized)
+		platform_driver_unregister(&einj_driver);
+
+	platform_device_del(einj_dev);
+}
+
 module_init(einj_init);
 module_exit(einj_exit);
 
-- 
2.34.1

RE: [PATCH v13 1/4] EINJ: Migrate to a platform driver

[Dan Williams]

Ben Cheatham wrote:
> Change the EINJ module to install a platform device/driver on module
> init and move the module init() and exit() functions to driver probe and
> remove. This change allows the EINJ module to load regardless of whether
> setting up EINJ succeeds, which allows dependent modules to still load
> (i.e. the CXL core).
> 
> Since EINJ may no longer be initialized when the module loads, any
> functions that are called from dependent/external modules should check
> the einj_initialized variable before calling any EINJ functions.
> 
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> ---
>  drivers/acpi/apei/einj.c | 46 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index 89fb9331c611..6ea323b9d8ef 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -21,6 +21,7 @@
>  #include <linux/nmi.h>
>  #include <linux/delay.h>
>  #include <linux/mm.h>
> +#include <linux/platform_device.h>
>  #include <asm/unaligned.h>
>  
>  #include "apei-internal.h"
> @@ -137,6 +138,11 @@ static struct apei_exec_ins_type einj_ins_type[] = {
>   */
>  static DEFINE_MUTEX(einj_mutex);
>  
> +/*
> + * Exported APIs use this flag to exit early if einj_probe() failed.
> + */
> +static bool einj_initialized __ro_after_init;
> +
>  static void *einj_param;
>  
>  static void einj_exec_ctx_init(struct apei_exec_context *ctx)
> @@ -703,7 +709,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab)
>  	return 0;
>  }
>  
> -static int __init einj_init(void)
> +static int __init einj_probe(struct platform_device *pdev)
>  {
>  	int rc;
>  	acpi_status status;
> @@ -717,7 +723,7 @@ static int __init einj_init(void)
>  	status = acpi_get_table(ACPI_SIG_EINJ, 0,
>  				(struct acpi_table_header **)&einj_tab);
>  	if (status == AE_NOT_FOUND) {
> -		pr_warn("EINJ table not found.\n");
> +		pr_info("EINJ table not found.\n");

Per comment on cover letter this should be pr_debug() to hide it given
that this module is no longer only loaded manually.

>  		return -ENODEV;
>  	} else if (ACPI_FAILURE(status)) {
>  		pr_err("Failed to get EINJ table: %s\n",
> @@ -805,7 +811,7 @@ static int __init einj_init(void)
>  	return rc;
>  }
>  
> -static void __exit einj_exit(void)
> +static void __exit einj_remove(struct platform_device *pdev)
>  {
>  	struct apei_exec_context ctx;
>  
> @@ -826,6 +832,40 @@ static void __exit einj_exit(void)
>  	acpi_put_table((struct acpi_table_header *)einj_tab);
>  }
>  
> +static struct platform_device *einj_dev;
> +struct platform_driver einj_driver = {

This can be static.

> +	.remove_new = einj_remove,
> +	.driver = {
> +		.name = "acpi-einj",
> +	},
> +};
> +
> +static int __init einj_init(void)
> +{
> +	struct platform_device_info einj_dev_info = {
> +		.name = "acpi-einj",
> +		.id = -1,
> +	};
> +	int rc;
> +
> +	einj_dev = platform_device_register_full(&einj_dev_info);
> +	if (IS_ERR_OR_NULL(einj_dev))

Given that platform_device_register_full() never returns NULL, this
should be IS_ERR().

> +		return PTR_ERR(einj_dev);
> +
> +	rc = platform_driver_probe(&einj_driver, einj_probe);
> +	einj_initialized = rc == 0;
> +
> +	return 0;
> +}
> +
> +static void __exit einj_exit(void)
> +{
> +	if (einj_initialized)
> +		platform_driver_unregister(&einj_driver);
> +
> +	platform_device_del(einj_dev);
> +}
> +
>  module_init(einj_init);
>  module_exit(einj_exit);
>  
> -- 
> 2.34.1
> 

Re: [PATCH v13 1/4] EINJ: Migrate to a platform driver

[Ben Cheatham]

Thanks for taking a look Dan, responses inline.

On 2/21/24 12:18 AM, Dan Williams wrote:
> Ben Cheatham wrote:
>> Change the EINJ module to install a platform device/driver on module
>> init and move the module init() and exit() functions to driver probe and
>> remove. This change allows the EINJ module to load regardless of whether
>> setting up EINJ succeeds, which allows dependent modules to still load
>> (i.e. the CXL core).
>>
>> Since EINJ may no longer be initialized when the module loads, any
>> functions that are called from dependent/external modules should check
>> the einj_initialized variable before calling any EINJ functions.
>>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
>> ---
>>  drivers/acpi/apei/einj.c | 46 +++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
>> index 89fb9331c611..6ea323b9d8ef 100644
>> --- a/drivers/acpi/apei/einj.c
>> +++ b/drivers/acpi/apei/einj.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/nmi.h>
>>  #include <linux/delay.h>
>>  #include <linux/mm.h>
>> +#include <linux/platform_device.h>
>>  #include <asm/unaligned.h>
>>  
>>  #include "apei-internal.h"
>> @@ -137,6 +138,11 @@ static struct apei_exec_ins_type einj_ins_type[] = {
>>   */
>>  static DEFINE_MUTEX(einj_mutex);
>>  
>> +/*
>> + * Exported APIs use this flag to exit early if einj_probe() failed.
>> + */
>> +static bool einj_initialized __ro_after_init;
>> +
>>  static void *einj_param;
>>  
>>  static void einj_exec_ctx_init(struct apei_exec_context *ctx)
>> @@ -703,7 +709,7 @@ static int einj_check_table(struct acpi_table_einj *einj_tab)
>>  	return 0;
>>  }
>>  
>> -static int __init einj_init(void)
>> +static int __init einj_probe(struct platform_device *pdev)
>>  {
>>  	int rc;
>>  	acpi_status status;
>> @@ -717,7 +723,7 @@ static int __init einj_init(void)
>>  	status = acpi_get_table(ACPI_SIG_EINJ, 0,
>>  				(struct acpi_table_header **)&einj_tab);
>>  	if (status == AE_NOT_FOUND) {
>> -		pr_warn("EINJ table not found.\n");
>> +		pr_info("EINJ table not found.\n");
> 
> Per comment on cover letter this should be pr_debug() to hide it given
> that this module is no longer only loaded manually.
> 

Alright sounds good.

>>  		return -ENODEV;
>>  	} else if (ACPI_FAILURE(status)) {
>>  		pr_err("Failed to get EINJ table: %s\n",
>> @@ -805,7 +811,7 @@ static int __init einj_init(void)
>>  	return rc;
>>  }
>>  
>> -static void __exit einj_exit(void)
>> +static void __exit einj_remove(struct platform_device *pdev)
>>  {
>>  	struct apei_exec_context ctx;
>>  
>> @@ -826,6 +832,40 @@ static void __exit einj_exit(void)
>>  	acpi_put_table((struct acpi_table_header *)einj_tab);
>>  }
>>  
>> +static struct platform_device *einj_dev;
>> +struct platform_driver einj_driver = {
> 
> This can be static.
> 

Will change.

>> +	.remove_new = einj_remove,
>> +	.driver = {
>> +		.name = "acpi-einj",
>> +	},
>> +};
>> +
>> +static int __init einj_init(void)
>> +{
>> +	struct platform_device_info einj_dev_info = {
>> +		.name = "acpi-einj",
>> +		.id = -1,
>> +	};
>> +	int rc;
>> +
>> +	einj_dev = platform_device_register_full(&einj_dev_info);
>> +	if (IS_ERR_OR_NULL(einj_dev))
> 
> Given that platform_device_register_full() never returns NULL, this
> should be IS_ERR().
> 

Sure thing.

>> +		return PTR_ERR(einj_dev);
>> +
>> +	rc = platform_driver_probe(&einj_driver, einj_probe);
>> +	einj_initialized = rc == 0;
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit einj_exit(void)
>> +{
>> +	if (einj_initialized)
>> +		platform_driver_unregister(&einj_driver);
>> +
>> +	platform_device_del(einj_dev);
>> +}
>> +
>>  module_init(einj_init);
>>  module_exit(einj_exit);
>>  
>> -- 
>> 2.34.1
>>
> 
> 

[PATCH v13 2/4] EINJ: Add CXL error type support

[Ben Cheatham]

Remove CXL protocol error types from the EINJ module and move them to
a new einj_cxl module. The einj_cxl module implements the necessary
handling for CXL protocol error injection and exposes an API for the
CXL core to use said functionality. Because the CXL error types
require special handling, only allow them to be injected through the
einj_cxl module and return an error when attempting to inject through
"regular" EINJ.

Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
---
 MAINTAINERS                       |   1 +
 drivers/acpi/apei/Kconfig         |  12 +++
 drivers/acpi/apei/Makefile        |   1 +
 drivers/acpi/apei/apei-internal.h |  17 +++++
 drivers/acpi/apei/einj-cxl.c      | 121 ++++++++++++++++++++++++++++++
 drivers/acpi/apei/einj.c          |  81 ++++++++++++++------
 include/linux/einj-cxl.h          |  40 ++++++++++
 7 files changed, 249 insertions(+), 24 deletions(-)
 create mode 100644 drivers/acpi/apei/einj-cxl.c
 create mode 100644 include/linux/einj-cxl.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 73d898383e51..51f9a0da57d7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5289,6 +5289,7 @@ M:	Dan Williams <dan.j.williams@intel.com>
 L:	linux-cxl@vger.kernel.org
 S:	Maintained
 F:	drivers/cxl/
+F:	include/linux/cxl-einj.h
 F:	include/linux/cxl-event.h
 F:	include/uapi/linux/cxl_mem.h
 F:	tools/testing/cxl/
diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
index 6b18f8bc7be3..040a9b2de235 100644
--- a/drivers/acpi/apei/Kconfig
+++ b/drivers/acpi/apei/Kconfig
@@ -60,6 +60,18 @@ config ACPI_APEI_EINJ
 	  mainly used for debugging and testing the other parts of
 	  APEI and some other RAS features.
 
+config ACPI_APEI_EINJ_CXL
+	tristate "CXL Error INJection Support"
+	default ACPI_APEI_EINJ
+	depends on ACPI_APEI_EINJ
+	help
+	  Support for CXL protocol Error INJection through debugfs/cxl.
+	  Availability and which errors are supported is dependent on
+	  the host platform. Look to ACPI v6.5 section 18.6.4 and kernel
+	  EINJ documentation for more information.
+
+	  If unsure say 'n'
+
 config ACPI_APEI_ERST_DEBUG
 	tristate "APEI Error Record Serialization Table (ERST) Debug Support"
 	depends on ACPI_APEI
diff --git a/drivers/acpi/apei/Makefile b/drivers/acpi/apei/Makefile
index 4dfac2128737..c18e96d342b2 100644
--- a/drivers/acpi/apei/Makefile
+++ b/drivers/acpi/apei/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_ACPI_APEI)		+= apei.o
 obj-$(CONFIG_ACPI_APEI_GHES)	+= ghes.o
 obj-$(CONFIG_ACPI_APEI_EINJ)	+= einj.o
+obj-$(CONFIG_ACPI_APEI_EINJ_CXL) += einj-cxl.o
 obj-$(CONFIG_ACPI_APEI_ERST_DEBUG) += erst-dbg.o
 
 apei-y := apei-base.o hest.o erst.o bert.o
diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
index 67c2c3b959e1..336408f4f293 100644
--- a/drivers/acpi/apei/apei-internal.h
+++ b/drivers/acpi/apei/apei-internal.h
@@ -130,4 +130,21 @@ static inline u32 cper_estatus_len(struct acpi_hest_generic_status *estatus)
 }
 
 int apei_osc_setup(void);
+
+int einj_get_available_error_type(u32 *type);
+int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
+		      u64 param4);
+bool einj_is_initialized(void);
+bool einj_is_cxl_error_type(u64 type);
+int einj_validate_error_type(u64 type);
+
+#ifndef ACPI_EINJ_CXL_CACHE_CORRECTABLE
+#define ACPI_EINJ_CXL_CACHE_CORRECTABLE     BIT(12)
+#define ACPI_EINJ_CXL_CACHE_UNCORRECTABLE   BIT(13)
+#define ACPI_EINJ_CXL_CACHE_FATAL           BIT(14)
+#define ACPI_EINJ_CXL_MEM_CORRECTABLE       BIT(15)
+#define ACPI_EINJ_CXL_MEM_UNCORRECTABLE     BIT(16)
+#define ACPI_EINJ_CXL_MEM_FATAL             BIT(17)
+#endif
+
 #endif
diff --git a/drivers/acpi/apei/einj-cxl.c b/drivers/acpi/apei/einj-cxl.c
new file mode 100644
index 000000000000..607d4f6adb98
--- /dev/null
+++ b/drivers/acpi/apei/einj-cxl.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * CXL Error INJection support. Used by CXL core to inject
+ * protocol errors into CXL ports.
+ *
+ * Copyright (C) 2023 Advanced Micro Devices, Inc.
+ *
+ * Author: Ben Cheatham <benjamin.cheatham@amd.com>
+ */
+#include <linux/einj-cxl.h>
+#include <linux/debugfs.h>
+
+#include "apei-internal.h"
+
+static struct { u32 mask; const char *str; } const einj_cxl_error_type_string[] = {
+	{ BIT(12), "CXL.cache Protocol Correctable" },
+	{ BIT(13), "CXL.cache Protocol Uncorrectable non-fatal" },
+	{ BIT(14), "CXL.cache Protocol Uncorrectable fatal" },
+	{ BIT(15), "CXL.mem Protocol Correctable" },
+	{ BIT(16), "CXL.mem Protocol Uncorrectable non-fatal" },
+	{ BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
+};
+
+int einj_cxl_available_error_type_show(struct seq_file *m, void *v)
+{
+	int cxl_err, rc;
+	u32 available_error_type = 0;
+
+	if (!einj_is_initialized())
+		return -ENXIO;
+
+	rc = einj_get_available_error_type(&available_error_type);
+	if (rc)
+		return rc;
+
+	for (int pos = 0; pos < ARRAY_SIZE(einj_cxl_error_type_string); pos++) {
+		cxl_err = ACPI_EINJ_CXL_CACHE_CORRECTABLE << pos;
+
+		if (available_error_type & cxl_err)
+			seq_printf(m, "0x%08x\t%s\n",
+				   einj_cxl_error_type_string[pos].mask,
+				   einj_cxl_error_type_string[pos].str);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(einj_cxl_available_error_type_show, CXL);
+
+static int cxl_dport_get_sbdf(struct pci_dev *dport_dev, u64 *sbdf)
+{
+	struct pci_bus *pbus;
+	struct pci_host_bridge *bridge;
+	u64 seg = 0, bus;
+
+	pbus = dport_dev->bus;
+	bridge = pci_find_host_bridge(pbus);
+
+	if (!bridge)
+		return -ENODEV;
+
+	if (bridge->domain_nr != PCI_DOMAIN_NR_NOT_SET)
+		seg = bridge->domain_nr;
+
+	bus = pbus->number;
+	*sbdf = (seg << 24) | (bus << 16) | dport_dev->devfn;
+
+	return 0;
+}
+
+int einj_cxl_inject_rch_error(u64 rcrb, u64 type)
+{
+	int rc;
+
+	if (!einj_is_initialized())
+		return -ENXIO;
+
+	/* Only CXL error types can be specified */
+	if (!einj_is_cxl_error_type(type))
+		return -EINVAL;
+
+	rc = einj_validate_error_type(type);
+	if (rc)
+		return rc;
+
+	return einj_error_inject(type, 0x2, rcrb, GENMASK_ULL(63, 12), 0, 0);
+}
+EXPORT_SYMBOL_NS_GPL(einj_cxl_inject_rch_error, CXL);
+
+int einj_cxl_inject_error(struct pci_dev *dport, u64 type)
+{
+	u64 param4 = 0;
+	int rc;
+
+	if (!einj_is_initialized())
+		return -ENXIO;
+
+	/* Only CXL error types can be specified */
+	if (!einj_is_cxl_error_type(type))
+		return -EINVAL;
+
+	rc = einj_validate_error_type(type);
+	if (rc)
+		return rc;
+
+	rc = cxl_dport_get_sbdf(dport, &param4);
+	if (rc)
+		return rc;
+
+	return einj_error_inject(type, 0x4, 0, 0, 0, param4);
+}
+EXPORT_SYMBOL_NS_GPL(einj_cxl_inject_error, CXL);
+
+bool einj_cxl_is_initialized(void)
+{
+	return einj_is_initialized();
+}
+EXPORT_SYMBOL_NS_GPL(einj_cxl_is_initialized, CXL);
+
+MODULE_AUTHOR("Ben Cheatham");
+MODULE_DESCRIPTION("CXL Error INJection support");
+MODULE_LICENSE("GPL");
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 6ea323b9d8ef..e76e64df97a7 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -37,6 +37,12 @@
 #define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
 				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
 				ACPI_EINJ_MEMORY_FATAL)
+#define CXL_ERROR_MASK		(ACPI_EINJ_CXL_CACHE_CORRECTABLE | \
+				ACPI_EINJ_CXL_CACHE_UNCORRECTABLE | \
+				ACPI_EINJ_CXL_CACHE_FATAL | \
+				ACPI_EINJ_CXL_MEM_CORRECTABLE | \
+				ACPI_EINJ_CXL_MEM_UNCORRECTABLE | \
+				ACPI_EINJ_CXL_MEM_FATAL)
 
 /*
  * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
@@ -166,7 +172,7 @@ static int __einj_get_available_error_type(u32 *type)
 }
 
 /* Get error injection capabilities of the platform */
-static int einj_get_available_error_type(u32 *type)
+int einj_get_available_error_type(u32 *type)
 {
 	int rc;
 
@@ -176,6 +182,7 @@ static int einj_get_available_error_type(u32 *type)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(einj_get_available_error_type);
 
 static int einj_timedout(u64 *t)
 {
@@ -536,8 +543,8 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
 }
 
 /* Inject the specified hardware error */
-static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
-			     u64 param3, u64 param4)
+int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
+		      u64 param4)
 {
 	int rc;
 	u64 base_addr, size;
@@ -560,8 +567,11 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
 	if (type & ACPI5_VENDOR_BIT) {
 		if (vendor_flags != SETWA_FLAGS_MEM)
 			goto inject;
-	} else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM))
+	} else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM)) {
 		goto inject;
+	} else if ((type & CXL_ERROR_MASK) && (flags & SETWA_FLAGS_MEM)) {
+		goto inject;
+	}
 
 	/*
 	 * Disallow crazy address masks that give BIOS leeway to pick
@@ -592,6 +602,7 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(einj_error_inject);
 
 static u32 error_type;
 static u32 error_flags;
@@ -613,12 +624,6 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
 	{ BIT(9), "Platform Correctable" },
 	{ BIT(10), "Platform Uncorrectable non-fatal" },
 	{ BIT(11), "Platform Uncorrectable fatal"},
-	{ BIT(12), "CXL.cache Protocol Correctable" },
-	{ BIT(13), "CXL.cache Protocol Uncorrectable non-fatal" },
-	{ BIT(14), "CXL.cache Protocol Uncorrectable fatal" },
-	{ BIT(15), "CXL.mem Protocol Correctable" },
-	{ BIT(16), "CXL.mem Protocol Uncorrectable non-fatal" },
-	{ BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
 	{ BIT(31), "Vendor Defined Error Types" },
 };
 
@@ -640,29 +645,21 @@ static int available_error_type_show(struct seq_file *m, void *v)
 
 DEFINE_SHOW_ATTRIBUTE(available_error_type);
 
-static int error_type_get(void *data, u64 *val)
-{
-	*val = error_type;
-
-	return 0;
-}
-
-static int error_type_set(void *data, u64 val)
+int einj_validate_error_type(u64 type)
 {
+	u32 tval, vendor, available_error_type = 0;
 	int rc;
-	u32 available_error_type = 0;
-	u32 tval, vendor;
 
 	/* Only low 32 bits for error type are valid */
-	if (val & GENMASK_ULL(63, 32))
+	if (type & GENMASK_ULL(63, 32))
 		return -EINVAL;
 
 	/*
 	 * Vendor defined types have 0x80000000 bit set, and
 	 * are not enumerated by ACPI_EINJ_GET_ERROR_TYPE
 	 */
-	vendor = val & ACPI5_VENDOR_BIT;
-	tval = val & 0x7fffffff;
+	vendor = type & ACPI5_VENDOR_BIT;
+	tval = type & GENMASK(30, 0);
 
 	/* Only one error type can be specified */
 	if (tval & (tval - 1))
@@ -671,9 +668,39 @@ static int error_type_set(void *data, u64 val)
 		rc = einj_get_available_error_type(&available_error_type);
 		if (rc)
 			return rc;
-		if (!(val & available_error_type))
+		if (!(type & available_error_type))
 			return -EINVAL;
 	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(einj_validate_error_type);
+
+bool einj_is_cxl_error_type(u64 type)
+{
+	return (type & CXL_ERROR_MASK) && (!(type & ACPI5_VENDOR_BIT));
+}
+EXPORT_SYMBOL_GPL(einj_is_cxl_error_type);
+
+static int error_type_get(void *data, u64 *val)
+{
+	*val = error_type;
+
+	return 0;
+}
+
+static int error_type_set(void *data, u64 val)
+{
+	int rc;
+
+	/* CXL error types have to be injected from cxl debugfs */
+	if (einj_is_cxl_error_type(val))
+		return -EINVAL;
+
+	rc = einj_validate_error_type(val);
+	if (rc)
+		return rc;
+
 	error_type = val;
 
 	return 0;
@@ -709,6 +736,12 @@ static int einj_check_table(struct acpi_table_einj *einj_tab)
 	return 0;
 }
 
+bool einj_is_initialized(void)
+{
+	return einj_initialized;
+}
+EXPORT_SYMBOL_GPL(einj_is_initialized);
+
 static int __init einj_probe(struct platform_device *pdev)
 {
 	int rc;
diff --git a/include/linux/einj-cxl.h b/include/linux/einj-cxl.h
new file mode 100644
index 000000000000..4a1f4600539a
--- /dev/null
+++ b/include/linux/einj-cxl.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * CXL protocol Error INJection support.
+ *
+ * Copyright (c) 2023 Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Author: Ben Cheatham <benjamin.cheatham@amd.com>
+ */
+#ifndef EINJ_CXL_H
+#define EINJ_CXL_H
+
+#include <linux/pci.h>
+
+#if IS_ENABLED(CONFIG_ACPI_APEI_EINJ_CXL)
+int einj_cxl_available_error_type_show(struct seq_file *m, void *v);
+int einj_cxl_inject_error(struct pci_dev *dport_dev, u64 type);
+int einj_cxl_inject_rch_error(u64 rcrb, u64 type);
+bool einj_cxl_is_initialized(void);
+#else /* !IS_ENABLED(CONFIG_ACPI_APEI_EINJ_CXL) */
+static inline int einj_cxl_available_error_type_show(struct seq_file *m,
+						     void *v)
+{
+	return -ENXIO;
+}
+
+static inline int einj_cxl_inject_error(struct pci_dev *dport_dev, u64 type)
+{
+	return -ENXIO;
+}
+
+static inline int einj_cxl_inject_rch_error(u64 rcrb, u64 type)
+{
+	return -ENXIO;
+}
+
+static inline bool einj_cxl_is_initialized(void) { return false; }
+#endif /* CONFIG_ACPI_APEI_EINJ_CXL */
+
+#endif /* EINJ_CXL_H */
-- 
2.34.1

RE: [PATCH v13 2/4] EINJ: Add CXL error type support

[Dan Williams]

Ben Cheatham wrote:
> Remove CXL protocol error types from the EINJ module and move them to
> a new einj_cxl module. The einj_cxl module implements the necessary
> handling for CXL protocol error injection and exposes an API for the
> CXL core to use said functionality. Because the CXL error types
> require special handling, only allow them to be injected through the
> einj_cxl module and return an error when attempting to inject through
> "regular" EINJ.

So Robustness Principle says be conservative in what you send and
liberal in what you accept. So cleaning up the reporting of CXL
capabilities over to the new interface is consistent with that
principle, but not removing the ability to inject via the legacy
interface. Especially since that has been the status quo for a few
kernel cycles is there a good reason to actively prevent usage of that
path?

> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> ---
>  MAINTAINERS                       |   1 +
>  drivers/acpi/apei/Kconfig         |  12 +++
>  drivers/acpi/apei/Makefile        |   1 +
>  drivers/acpi/apei/apei-internal.h |  17 +++++
>  drivers/acpi/apei/einj-cxl.c      | 121 ++++++++++++++++++++++++++++++
>  drivers/acpi/apei/einj.c          |  81 ++++++++++++++------
>  include/linux/einj-cxl.h          |  40 ++++++++++
>  7 files changed, 249 insertions(+), 24 deletions(-)
>  create mode 100644 drivers/acpi/apei/einj-cxl.c
>  create mode 100644 include/linux/einj-cxl.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 73d898383e51..51f9a0da57d7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5289,6 +5289,7 @@ M:	Dan Williams <dan.j.williams@intel.com>
>  L:	linux-cxl@vger.kernel.org
>  S:	Maintained
>  F:	drivers/cxl/
> +F:	include/linux/cxl-einj.h
>  F:	include/linux/cxl-event.h
>  F:	include/uapi/linux/cxl_mem.h
>  F:	tools/testing/cxl/
> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
> index 6b18f8bc7be3..040a9b2de235 100644
> --- a/drivers/acpi/apei/Kconfig
> +++ b/drivers/acpi/apei/Kconfig
> @@ -60,6 +60,18 @@ config ACPI_APEI_EINJ
>  	  mainly used for debugging and testing the other parts of
>  	  APEI and some other RAS features.
>  
> +config ACPI_APEI_EINJ_CXL
> +	tristate "CXL Error INJection Support"

This should still be a boolean because it is add-on functionality to the
cxl_core.ko module which has its own tristate configuration.

> +	default ACPI_APEI_EINJ
> +	depends on ACPI_APEI_EINJ

The dependency still needs to be:

    depends on ACPI_APEI_EINJ && CXL_BUS >= ACPI_APEI_EINJ

...because CXL_BUS can not tolerate being built-in when ACPI_APEI_EINJ
is not.

> +	help
> +	  Support for CXL protocol Error INJection through debugfs/cxl.
> +	  Availability and which errors are supported is dependent on
> +	  the host platform. Look to ACPI v6.5 section 18.6.4 and kernel
> +	  EINJ documentation for more information.
> +
> +	  If unsure say 'n'
> +
>  config ACPI_APEI_ERST_DEBUG
>  	tristate "APEI Error Record Serialization Table (ERST) Debug Support"
>  	depends on ACPI_APEI
> diff --git a/drivers/acpi/apei/Makefile b/drivers/acpi/apei/Makefile
> index 4dfac2128737..c18e96d342b2 100644
> --- a/drivers/acpi/apei/Makefile
> +++ b/drivers/acpi/apei/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_ACPI_APEI)		+= apei.o
>  obj-$(CONFIG_ACPI_APEI_GHES)	+= ghes.o
>  obj-$(CONFIG_ACPI_APEI_EINJ)	+= einj.o
> +obj-$(CONFIG_ACPI_APEI_EINJ_CXL) += einj-cxl.o

No new module needed. It only needs another compilation unit optionally
added to einj.ko. Something like this:

diff --git a/drivers/acpi/apei/Makefile b/drivers/acpi/apei/Makefile
index 4dfac2128737..2c474e6477e1 100644
--- a/drivers/acpi/apei/Makefile
+++ b/drivers/acpi/apei/Makefile
@@ -2,6 +2,8 @@
 obj-$(CONFIG_ACPI_APEI)                += apei.o
 obj-$(CONFIG_ACPI_APEI_GHES)   += ghes.o
 obj-$(CONFIG_ACPI_APEI_EINJ)   += einj.o
+einj-y                         := einj-core.o
+einj-$(CONFIG_ACPI_APEI_EINJ_CXL) += einj-cxl.o
 obj-$(CONFIG_ACPI_APEI_ERST_DEBUG) += erst-dbg.o
 
 apei-y := apei-base.o hest.o erst.o bert.o
diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj-core.c
similarity index 100%
rename from drivers/acpi/apei/einj.c
rename to drivers/acpi/apei/einj-core.c

>  obj-$(CONFIG_ACPI_APEI_ERST_DEBUG) += erst-dbg.o
>  
>  apei-y := apei-base.o hest.o erst.o bert.o
> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
> index 67c2c3b959e1..336408f4f293 100644
> --- a/drivers/acpi/apei/apei-internal.h
> +++ b/drivers/acpi/apei/apei-internal.h
> @@ -130,4 +130,21 @@ static inline u32 cper_estatus_len(struct acpi_hest_generic_status *estatus)
>  }
>  
>  int apei_osc_setup(void);
> +
> +int einj_get_available_error_type(u32 *type);
> +int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
> +		      u64 param4);
> +bool einj_is_initialized(void);
> +bool einj_is_cxl_error_type(u64 type);
> +int einj_validate_error_type(u64 type);
> +
> +#ifndef ACPI_EINJ_CXL_CACHE_CORRECTABLE
> +#define ACPI_EINJ_CXL_CACHE_CORRECTABLE     BIT(12)
> +#define ACPI_EINJ_CXL_CACHE_UNCORRECTABLE   BIT(13)
> +#define ACPI_EINJ_CXL_CACHE_FATAL           BIT(14)
> +#define ACPI_EINJ_CXL_MEM_CORRECTABLE       BIT(15)
> +#define ACPI_EINJ_CXL_MEM_UNCORRECTABLE     BIT(16)
> +#define ACPI_EINJ_CXL_MEM_FATAL             BIT(17)
> +#endif
> +
>  #endif
> diff --git a/drivers/acpi/apei/einj-cxl.c b/drivers/acpi/apei/einj-cxl.c
> new file mode 100644
> index 000000000000..607d4f6adb98
> --- /dev/null
> +++ b/drivers/acpi/apei/einj-cxl.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * CXL Error INJection support. Used by CXL core to inject
> + * protocol errors into CXL ports.
> + *
> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
> + *
> + * Author: Ben Cheatham <benjamin.cheatham@amd.com>
> + */
> +#include <linux/einj-cxl.h>
> +#include <linux/debugfs.h>
> +
> +#include "apei-internal.h"
> +
> +static struct { u32 mask; const char *str; } const einj_cxl_error_type_string[] = {
> +	{ BIT(12), "CXL.cache Protocol Correctable" },
> +	{ BIT(13), "CXL.cache Protocol Uncorrectable non-fatal" },
> +	{ BIT(14), "CXL.cache Protocol Uncorrectable fatal" },
> +	{ BIT(15), "CXL.mem Protocol Correctable" },
> +	{ BIT(16), "CXL.mem Protocol Uncorrectable non-fatal" },
> +	{ BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
> +};
> +
> +int einj_cxl_available_error_type_show(struct seq_file *m, void *v)
> +{
> +	int cxl_err, rc;
> +	u32 available_error_type = 0;
> +
> +	if (!einj_is_initialized())
> +		return -ENXIO;
> +
> +	rc = einj_get_available_error_type(&available_error_type);
> +	if (rc)
> +		return rc;
> +
> +	for (int pos = 0; pos < ARRAY_SIZE(einj_cxl_error_type_string); pos++) {
> +		cxl_err = ACPI_EINJ_CXL_CACHE_CORRECTABLE << pos;
> +
> +		if (available_error_type & cxl_err)
> +			seq_printf(m, "0x%08x\t%s\n",
> +				   einj_cxl_error_type_string[pos].mask,
> +				   einj_cxl_error_type_string[pos].str);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(einj_cxl_available_error_type_show, CXL);
> +
> +static int cxl_dport_get_sbdf(struct pci_dev *dport_dev, u64 *sbdf)
> +{
> +	struct pci_bus *pbus;
> +	struct pci_host_bridge *bridge;
> +	u64 seg = 0, bus;
> +
> +	pbus = dport_dev->bus;
> +	bridge = pci_find_host_bridge(pbus);
> +
> +	if (!bridge)
> +		return -ENODEV;
> +
> +	if (bridge->domain_nr != PCI_DOMAIN_NR_NOT_SET)
> +		seg = bridge->domain_nr;
> +
> +	bus = pbus->number;
> +	*sbdf = (seg << 24) | (bus << 16) | dport_dev->devfn;
> +
> +	return 0;
> +}
> +
> +int einj_cxl_inject_rch_error(u64 rcrb, u64 type)
> +{
> +	int rc;
> +
> +	if (!einj_is_initialized())
> +		return -ENXIO;
> +
> +	/* Only CXL error types can be specified */
> +	if (!einj_is_cxl_error_type(type))
> +		return -EINVAL;
> +
> +	rc = einj_validate_error_type(type);
> +	if (rc)
> +		return rc;
> +
> +	return einj_error_inject(type, 0x2, rcrb, GENMASK_ULL(63, 12), 0, 0);
> +}
> +EXPORT_SYMBOL_NS_GPL(einj_cxl_inject_rch_error, CXL);
> +
> +int einj_cxl_inject_error(struct pci_dev *dport, u64 type)
> +{
> +	u64 param4 = 0;
> +	int rc;
> +
> +	if (!einj_is_initialized())
> +		return -ENXIO;
> +
> +	/* Only CXL error types can be specified */
> +	if (!einj_is_cxl_error_type(type))
> +		return -EINVAL;
> +
> +	rc = einj_validate_error_type(type);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_dport_get_sbdf(dport, &param4);
> +	if (rc)
> +		return rc;
> +
> +	return einj_error_inject(type, 0x4, 0, 0, 0, param4);
> +}
> +EXPORT_SYMBOL_NS_GPL(einj_cxl_inject_error, CXL);
> +
> +bool einj_cxl_is_initialized(void)
> +{
> +	return einj_is_initialized();
> +}
> +EXPORT_SYMBOL_NS_GPL(einj_cxl_is_initialized, CXL);
> +
> +MODULE_AUTHOR("Ben Cheatham");
> +MODULE_DESCRIPTION("CXL Error INJection support");
> +MODULE_LICENSE("GPL");

These go away when cxl-einj.ko is no longer its own module.

> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> index 6ea323b9d8ef..e76e64df97a7 100644
> --- a/drivers/acpi/apei/einj.c
> +++ b/drivers/acpi/apei/einj.c
> @@ -37,6 +37,12 @@
>  #define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
>  				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
>  				ACPI_EINJ_MEMORY_FATAL)
> +#define CXL_ERROR_MASK		(ACPI_EINJ_CXL_CACHE_CORRECTABLE | \
> +				ACPI_EINJ_CXL_CACHE_UNCORRECTABLE | \
> +				ACPI_EINJ_CXL_CACHE_FATAL | \
> +				ACPI_EINJ_CXL_MEM_CORRECTABLE | \
> +				ACPI_EINJ_CXL_MEM_UNCORRECTABLE | \
> +				ACPI_EINJ_CXL_MEM_FATAL)
>  
>  /*
>   * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
> @@ -166,7 +172,7 @@ static int __einj_get_available_error_type(u32 *type)
>  }
>  
>  /* Get error injection capabilities of the platform */
> -static int einj_get_available_error_type(u32 *type)
> +int einj_get_available_error_type(u32 *type)
>  {
>  	int rc;
>  
> @@ -176,6 +182,7 @@ static int einj_get_available_error_type(u32 *type)
>  
>  	return rc;
>  }
> +EXPORT_SYMBOL_GPL(einj_get_available_error_type);

There should not be any need for new exports from the legacy einj.c.

>  
>  static int einj_timedout(u64 *t)
>  {
> @@ -536,8 +543,8 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>  }
>  
>  /* Inject the specified hardware error */
> -static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
> -			     u64 param3, u64 param4)
> +int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
> +		      u64 param4)
>  {
>  	int rc;
>  	u64 base_addr, size;
> @@ -560,8 +567,11 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>  	if (type & ACPI5_VENDOR_BIT) {
>  		if (vendor_flags != SETWA_FLAGS_MEM)
>  			goto inject;
> -	} else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM))
> +	} else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM)) {
>  		goto inject;
> +	} else if ((type & CXL_ERROR_MASK) && (flags & SETWA_FLAGS_MEM)) {
> +		goto inject;
> +	}
>  
>  	/*
>  	 * Disallow crazy address masks that give BIOS leeway to pick
> @@ -592,6 +602,7 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>  
>  	return rc;
>  }
> +EXPORT_SYMBOL_GPL(einj_error_inject);
>  
>  static u32 error_type;
>  static u32 error_flags;
> @@ -613,12 +624,6 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>  	{ BIT(9), "Platform Correctable" },
>  	{ BIT(10), "Platform Uncorrectable non-fatal" },
>  	{ BIT(11), "Platform Uncorrectable fatal"},
> -	{ BIT(12), "CXL.cache Protocol Correctable" },
> -	{ BIT(13), "CXL.cache Protocol Uncorrectable non-fatal" },
> -	{ BIT(14), "CXL.cache Protocol Uncorrectable fatal" },
> -	{ BIT(15), "CXL.mem Protocol Correctable" },
> -	{ BIT(16), "CXL.mem Protocol Uncorrectable non-fatal" },
> -	{ BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
>  	{ BIT(31), "Vendor Defined Error Types" },
>  };
>  
> @@ -640,29 +645,21 @@ static int available_error_type_show(struct seq_file *m, void *v)
>  
>  DEFINE_SHOW_ATTRIBUTE(available_error_type);
>  
> -static int error_type_get(void *data, u64 *val)
> -{
> -	*val = error_type;
> -
> -	return 0;
> -}
> -
> -static int error_type_set(void *data, u64 val)
> +int einj_validate_error_type(u64 type)
>  {
> +	u32 tval, vendor, available_error_type = 0;
>  	int rc;
> -	u32 available_error_type = 0;
> -	u32 tval, vendor;
>  
>  	/* Only low 32 bits for error type are valid */
> -	if (val & GENMASK_ULL(63, 32))
> +	if (type & GENMASK_ULL(63, 32))
>  		return -EINVAL;
>  
>  	/*
>  	 * Vendor defined types have 0x80000000 bit set, and
>  	 * are not enumerated by ACPI_EINJ_GET_ERROR_TYPE
>  	 */
> -	vendor = val & ACPI5_VENDOR_BIT;
> -	tval = val & 0x7fffffff;
> +	vendor = type & ACPI5_VENDOR_BIT;
> +	tval = type & GENMASK(30, 0);
>  
>  	/* Only one error type can be specified */
>  	if (tval & (tval - 1))
> @@ -671,9 +668,39 @@ static int error_type_set(void *data, u64 val)
>  		rc = einj_get_available_error_type(&available_error_type);
>  		if (rc)
>  			return rc;
> -		if (!(val & available_error_type))
> +		if (!(type & available_error_type))
>  			return -EINVAL;
>  	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(einj_validate_error_type);
> +
> +bool einj_is_cxl_error_type(u64 type)
> +{
> +	return (type & CXL_ERROR_MASK) && (!(type & ACPI5_VENDOR_BIT));
> +}
> +EXPORT_SYMBOL_GPL(einj_is_cxl_error_type);
> +
> +static int error_type_get(void *data, u64 *val)
> +{
> +	*val = error_type;
> +
> +	return 0;
> +}
> +
> +static int error_type_set(void *data, u64 val)
> +{
> +	int rc;
> +
> +	/* CXL error types have to be injected from cxl debugfs */
> +	if (einj_is_cxl_error_type(val))
> +		return -EINVAL;
> +
> +	rc = einj_validate_error_type(val);
> +	if (rc)
> +		return rc;
> +
>  	error_type = val;
>  
>  	return 0;
> @@ -709,6 +736,12 @@ static int einj_check_table(struct acpi_table_einj *einj_tab)
>  	return 0;
>  }
>  
> +bool einj_is_initialized(void)
> +{
> +	return einj_initialized;
> +}
> +EXPORT_SYMBOL_GPL(einj_is_initialized);

The variable can be referenced directly as a global symbol.

Re: [PATCH v13 2/4] EINJ: Add CXL error type support

[Ben Cheatham]

On 2/21/24 11:43 AM, Dan Williams wrote:
> Ben Cheatham wrote:
>> Remove CXL protocol error types from the EINJ module and move them to
>> a new einj_cxl module. The einj_cxl module implements the necessary
>> handling for CXL protocol error injection and exposes an API for the
>> CXL core to use said functionality. Because the CXL error types
>> require special handling, only allow them to be injected through the
>> einj_cxl module and return an error when attempting to inject through
>> "regular" EINJ.
> 
> So Robustness Principle says be conservative in what you send and
> liberal in what you accept. So cleaning up the reporting of CXL
> capabilities over to the new interface is consistent with that
> principle, but not removing the ability to inject via the legacy
> interface. Especially since that has been the status quo for a few
> kernel cycles is there a good reason to actively prevent usage of that
> path?
> 

For CXL 2.0+ ports it's fine since EINJ only expects an SBDF which is
pretty readily accessible by the user. CXL 1.1/1.0 ports however, it's a bit
of a headache. It would require the user to find the address of the RCRB
for the port and supply that to the EINJ module. I originally had this option
anyway, but I think it got shot down for being too obtuse to use (I think by
you, but it's been a while xD). If you think it's still worthwhile I can
remove the restriction for both types of ports or just the 2.0+ ports.

For CXL 1.0/1.1 ports there's also the security issue of being able to inject
to any address since the way it works is by skipping the memory address
checks, but since this is a debug module I don't think it's that big
of a deal.

>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
>> ---
>>  MAINTAINERS                       |   1 +
>>  drivers/acpi/apei/Kconfig         |  12 +++
>>  drivers/acpi/apei/Makefile        |   1 +
>>  drivers/acpi/apei/apei-internal.h |  17 +++++
>>  drivers/acpi/apei/einj-cxl.c      | 121 ++++++++++++++++++++++++++++++
>>  drivers/acpi/apei/einj.c          |  81 ++++++++++++++------
>>  include/linux/einj-cxl.h          |  40 ++++++++++
>>  7 files changed, 249 insertions(+), 24 deletions(-)
>>  create mode 100644 drivers/acpi/apei/einj-cxl.c
>>  create mode 100644 include/linux/einj-cxl.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 73d898383e51..51f9a0da57d7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -5289,6 +5289,7 @@ M:	Dan Williams <dan.j.williams@intel.com>
>>  L:	linux-cxl@vger.kernel.org
>>  S:	Maintained
>>  F:	drivers/cxl/
>> +F:	include/linux/cxl-einj.h
>>  F:	include/linux/cxl-event.h
>>  F:	include/uapi/linux/cxl_mem.h
>>  F:	tools/testing/cxl/
>> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
>> index 6b18f8bc7be3..040a9b2de235 100644
>> --- a/drivers/acpi/apei/Kconfig
>> +++ b/drivers/acpi/apei/Kconfig
>> @@ -60,6 +60,18 @@ config ACPI_APEI_EINJ
>>  	  mainly used for debugging and testing the other parts of
>>  	  APEI and some other RAS features.
>>  
>> +config ACPI_APEI_EINJ_CXL
>> +	tristate "CXL Error INJection Support"
> 
> This should still be a boolean because it is add-on functionality to the
> cxl_core.ko module which has its own tristate configuration.
> 

I tried this but was running into issues, more about this in the Makefile
portion of the patch.

>> +	default ACPI_APEI_EINJ
>> +	depends on ACPI_APEI_EINJ
> 
> The dependency still needs to be:
> 
>     depends on ACPI_APEI_EINJ && CXL_BUS >= ACPI_APEI_EINJ
> 
> ...because CXL_BUS can not tolerate being built-in when ACPI_APEI_EINJ
> is not.
> 

Will do.

>> +	help
>> +	  Support for CXL protocol Error INJection through debugfs/cxl.
>> +	  Availability and which errors are supported is dependent on
>> +	  the host platform. Look to ACPI v6.5 section 18.6.4 and kernel
>> +	  EINJ documentation for more information.
>> +
>> +	  If unsure say 'n'
>> +
>>  config ACPI_APEI_ERST_DEBUG
>>  	tristate "APEI Error Record Serialization Table (ERST) Debug Support"
>>  	depends on ACPI_APEI
>> diff --git a/drivers/acpi/apei/Makefile b/drivers/acpi/apei/Makefile
>> index 4dfac2128737..c18e96d342b2 100644
>> --- a/drivers/acpi/apei/Makefile
>> +++ b/drivers/acpi/apei/Makefile
>> @@ -2,6 +2,7 @@
>>  obj-$(CONFIG_ACPI_APEI)		+= apei.o
>>  obj-$(CONFIG_ACPI_APEI_GHES)	+= ghes.o
>>  obj-$(CONFIG_ACPI_APEI_EINJ)	+= einj.o
>> +obj-$(CONFIG_ACPI_APEI_EINJ_CXL) += einj-cxl.o
> 
> No new module needed. It only needs another compilation unit optionally
> added to einj.ko. Something like this:
> 
> diff --git a/drivers/acpi/apei/Makefile b/drivers/acpi/apei/Makefile
> index 4dfac2128737..2c474e6477e1 100644
> --- a/drivers/acpi/apei/Makefile
> +++ b/drivers/acpi/apei/Makefile
> @@ -2,6 +2,8 @@
>  obj-$(CONFIG_ACPI_APEI)                += apei.o
>  obj-$(CONFIG_ACPI_APEI_GHES)   += ghes.o
>  obj-$(CONFIG_ACPI_APEI_EINJ)   += einj.o
> +einj-y                         := einj-core.o
> +einj-$(CONFIG_ACPI_APEI_EINJ_CXL) += einj-cxl.o
>  obj-$(CONFIG_ACPI_APEI_ERST_DEBUG) += erst-dbg.o
>  
>  apei-y := apei-base.o hest.o erst.o bert.o
> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj-core.c
> similarity index 100%
> rename from drivers/acpi/apei/einj.c
> rename to drivers/acpi/apei/einj-core.c
> 

And this is what was causing my issues. I couldn't CONFIG_ACPI_APEI_EINJ_CXL work
as a boolean because I didn't put it in the right compilation unit. This should
also allow me to remove the dependency in the next patch and the exports below.

>>  obj-$(CONFIG_ACPI_APEI_ERST_DEBUG) += erst-dbg.o
>>  
>>  apei-y := apei-base.o hest.o erst.o bert.o
>> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
>> index 67c2c3b959e1..336408f4f293 100644
>> --- a/drivers/acpi/apei/apei-internal.h
>> +++ b/drivers/acpi/apei/apei-internal.h
>> @@ -130,4 +130,21 @@ static inline u32 cper_estatus_len(struct acpi_hest_generic_status *estatus)
>>  }
>>  
>>  int apei_osc_setup(void);
>> +
>> +int einj_get_available_error_type(u32 *type);
>> +int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
>> +		      u64 param4);
>> +bool einj_is_initialized(void);
>> +bool einj_is_cxl_error_type(u64 type);
>> +int einj_validate_error_type(u64 type);
>> +
>> +#ifndef ACPI_EINJ_CXL_CACHE_CORRECTABLE
>> +#define ACPI_EINJ_CXL_CACHE_CORRECTABLE     BIT(12)
>> +#define ACPI_EINJ_CXL_CACHE_UNCORRECTABLE   BIT(13)
>> +#define ACPI_EINJ_CXL_CACHE_FATAL           BIT(14)
>> +#define ACPI_EINJ_CXL_MEM_CORRECTABLE       BIT(15)
>> +#define ACPI_EINJ_CXL_MEM_UNCORRECTABLE     BIT(16)
>> +#define ACPI_EINJ_CXL_MEM_FATAL             BIT(17)
>> +#endif
>> +
>>  #endif
>> diff --git a/drivers/acpi/apei/einj-cxl.c b/drivers/acpi/apei/einj-cxl.c
>> new file mode 100644
>> index 000000000000..607d4f6adb98
>> --- /dev/null
>> +++ b/drivers/acpi/apei/einj-cxl.c
>> @@ -0,0 +1,121 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * CXL Error INJection support. Used by CXL core to inject
>> + * protocol errors into CXL ports.
>> + *
>> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
>> + *
>> + * Author: Ben Cheatham <benjamin.cheatham@amd.com>
>> + */
>> +#include <linux/einj-cxl.h>
>> +#include <linux/debugfs.h>
>> +
>> +#include "apei-internal.h"
>> +
>> +static struct { u32 mask; const char *str; } const einj_cxl_error_type_string[] = {
>> +	{ BIT(12), "CXL.cache Protocol Correctable" },
>> +	{ BIT(13), "CXL.cache Protocol Uncorrectable non-fatal" },
>> +	{ BIT(14), "CXL.cache Protocol Uncorrectable fatal" },
>> +	{ BIT(15), "CXL.mem Protocol Correctable" },
>> +	{ BIT(16), "CXL.mem Protocol Uncorrectable non-fatal" },
>> +	{ BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
>> +};
>> +
>> +int einj_cxl_available_error_type_show(struct seq_file *m, void *v)
>> +{
>> +	int cxl_err, rc;
>> +	u32 available_error_type = 0;
>> +
>> +	if (!einj_is_initialized())
>> +		return -ENXIO;
>> +
>> +	rc = einj_get_available_error_type(&available_error_type);
>> +	if (rc)
>> +		return rc;
>> +
>> +	for (int pos = 0; pos < ARRAY_SIZE(einj_cxl_error_type_string); pos++) {
>> +		cxl_err = ACPI_EINJ_CXL_CACHE_CORRECTABLE << pos;
>> +
>> +		if (available_error_type & cxl_err)
>> +			seq_printf(m, "0x%08x\t%s\n",
>> +				   einj_cxl_error_type_string[pos].mask,
>> +				   einj_cxl_error_type_string[pos].str);
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(einj_cxl_available_error_type_show, CXL);
>> +
>> +static int cxl_dport_get_sbdf(struct pci_dev *dport_dev, u64 *sbdf)
>> +{
>> +	struct pci_bus *pbus;
>> +	struct pci_host_bridge *bridge;
>> +	u64 seg = 0, bus;
>> +
>> +	pbus = dport_dev->bus;
>> +	bridge = pci_find_host_bridge(pbus);
>> +
>> +	if (!bridge)
>> +		return -ENODEV;
>> +
>> +	if (bridge->domain_nr != PCI_DOMAIN_NR_NOT_SET)
>> +		seg = bridge->domain_nr;
>> +
>> +	bus = pbus->number;
>> +	*sbdf = (seg << 24) | (bus << 16) | dport_dev->devfn;
>> +
>> +	return 0;
>> +}
>> +
>> +int einj_cxl_inject_rch_error(u64 rcrb, u64 type)
>> +{
>> +	int rc;
>> +
>> +	if (!einj_is_initialized())
>> +		return -ENXIO;
>> +
>> +	/* Only CXL error types can be specified */
>> +	if (!einj_is_cxl_error_type(type))
>> +		return -EINVAL;
>> +
>> +	rc = einj_validate_error_type(type);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return einj_error_inject(type, 0x2, rcrb, GENMASK_ULL(63, 12), 0, 0);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(einj_cxl_inject_rch_error, CXL);
>> +
>> +int einj_cxl_inject_error(struct pci_dev *dport, u64 type)
>> +{
>> +	u64 param4 = 0;
>> +	int rc;
>> +
>> +	if (!einj_is_initialized())
>> +		return -ENXIO;
>> +
>> +	/* Only CXL error types can be specified */
>> +	if (!einj_is_cxl_error_type(type))
>> +		return -EINVAL;
>> +
>> +	rc = einj_validate_error_type(type);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_dport_get_sbdf(dport, &param4);
>> +	if (rc)
>> +		return rc;
>> +
>> +	return einj_error_inject(type, 0x4, 0, 0, 0, param4);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(einj_cxl_inject_error, CXL);
>> +
>> +bool einj_cxl_is_initialized(void)
>> +{
>> +	return einj_is_initialized();
>> +}
>> +EXPORT_SYMBOL_NS_GPL(einj_cxl_is_initialized, CXL);
>> +
>> +MODULE_AUTHOR("Ben Cheatham");
>> +MODULE_DESCRIPTION("CXL Error INJection support");
>> +MODULE_LICENSE("GPL");
> 
> These go away when cxl-einj.ko is no longer its own module.
> 
>> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
>> index 6ea323b9d8ef..e76e64df97a7 100644
>> --- a/drivers/acpi/apei/einj.c
>> +++ b/drivers/acpi/apei/einj.c
>> @@ -37,6 +37,12 @@
>>  #define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
>>  				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
>>  				ACPI_EINJ_MEMORY_FATAL)
>> +#define CXL_ERROR_MASK		(ACPI_EINJ_CXL_CACHE_CORRECTABLE | \
>> +				ACPI_EINJ_CXL_CACHE_UNCORRECTABLE | \
>> +				ACPI_EINJ_CXL_CACHE_FATAL | \
>> +				ACPI_EINJ_CXL_MEM_CORRECTABLE | \
>> +				ACPI_EINJ_CXL_MEM_UNCORRECTABLE | \
>> +				ACPI_EINJ_CXL_MEM_FATAL)
>>  
>>  /*
>>   * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
>> @@ -166,7 +172,7 @@ static int __einj_get_available_error_type(u32 *type)
>>  }
>>  
>>  /* Get error injection capabilities of the platform */
>> -static int einj_get_available_error_type(u32 *type)
>> +int einj_get_available_error_type(u32 *type)
>>  {
>>  	int rc;
>>  
>> @@ -176,6 +182,7 @@ static int einj_get_available_error_type(u32 *type)
>>  
>>  	return rc;
>>  }
>> +EXPORT_SYMBOL_GPL(einj_get_available_error_type);
> 
> There should not be any need for new exports from the legacy einj.c.
> 
>>  
>>  static int einj_timedout(u64 *t)
>>  {
>> @@ -536,8 +543,8 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>>  }
>>  
>>  /* Inject the specified hardware error */
>> -static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>> -			     u64 param3, u64 param4)
>> +int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
>> +		      u64 param4)
>>  {
>>  	int rc;
>>  	u64 base_addr, size;
>> @@ -560,8 +567,11 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>>  	if (type & ACPI5_VENDOR_BIT) {
>>  		if (vendor_flags != SETWA_FLAGS_MEM)
>>  			goto inject;
>> -	} else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM))
>> +	} else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM)) {
>>  		goto inject;
>> +	} else if ((type & CXL_ERROR_MASK) && (flags & SETWA_FLAGS_MEM)) {
>> +		goto inject;
>> +	}
>>  
>>  	/*
>>  	 * Disallow crazy address masks that give BIOS leeway to pick
>> @@ -592,6 +602,7 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>>  
>>  	return rc;
>>  }
>> +EXPORT_SYMBOL_GPL(einj_error_inject);
>>  
>>  static u32 error_type;
>>  static u32 error_flags;
>> @@ -613,12 +624,6 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>>  	{ BIT(9), "Platform Correctable" },
>>  	{ BIT(10), "Platform Uncorrectable non-fatal" },
>>  	{ BIT(11), "Platform Uncorrectable fatal"},
>> -	{ BIT(12), "CXL.cache Protocol Correctable" },
>> -	{ BIT(13), "CXL.cache Protocol Uncorrectable non-fatal" },
>> -	{ BIT(14), "CXL.cache Protocol Uncorrectable fatal" },
>> -	{ BIT(15), "CXL.mem Protocol Correctable" },
>> -	{ BIT(16), "CXL.mem Protocol Uncorrectable non-fatal" },
>> -	{ BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
>>  	{ BIT(31), "Vendor Defined Error Types" },
>>  };
>>  
>> @@ -640,29 +645,21 @@ static int available_error_type_show(struct seq_file *m, void *v)
>>  
>>  DEFINE_SHOW_ATTRIBUTE(available_error_type);
>>  
>> -static int error_type_get(void *data, u64 *val)
>> -{
>> -	*val = error_type;
>> -
>> -	return 0;
>> -}
>> -
>> -static int error_type_set(void *data, u64 val)
>> +int einj_validate_error_type(u64 type)
>>  {
>> +	u32 tval, vendor, available_error_type = 0;
>>  	int rc;
>> -	u32 available_error_type = 0;
>> -	u32 tval, vendor;
>>  
>>  	/* Only low 32 bits for error type are valid */
>> -	if (val & GENMASK_ULL(63, 32))
>> +	if (type & GENMASK_ULL(63, 32))
>>  		return -EINVAL;
>>  
>>  	/*
>>  	 * Vendor defined types have 0x80000000 bit set, and
>>  	 * are not enumerated by ACPI_EINJ_GET_ERROR_TYPE
>>  	 */
>> -	vendor = val & ACPI5_VENDOR_BIT;
>> -	tval = val & 0x7fffffff;
>> +	vendor = type & ACPI5_VENDOR_BIT;
>> +	tval = type & GENMASK(30, 0);
>>  
>>  	/* Only one error type can be specified */
>>  	if (tval & (tval - 1))
>> @@ -671,9 +668,39 @@ static int error_type_set(void *data, u64 val)
>>  		rc = einj_get_available_error_type(&available_error_type);
>>  		if (rc)
>>  			return rc;
>> -		if (!(val & available_error_type))
>> +		if (!(type & available_error_type))
>>  			return -EINVAL;
>>  	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(einj_validate_error_type);
>> +
>> +bool einj_is_cxl_error_type(u64 type)
>> +{
>> +	return (type & CXL_ERROR_MASK) && (!(type & ACPI5_VENDOR_BIT));
>> +}
>> +EXPORT_SYMBOL_GPL(einj_is_cxl_error_type);
>> +
>> +static int error_type_get(void *data, u64 *val)
>> +{
>> +	*val = error_type;
>> +
>> +	return 0;
>> +}
>> +
>> +static int error_type_set(void *data, u64 val)
>> +{
>> +	int rc;
>> +
>> +	/* CXL error types have to be injected from cxl debugfs */
>> +	if (einj_is_cxl_error_type(val))
>> +		return -EINVAL;
>> +
>> +	rc = einj_validate_error_type(val);
>> +	if (rc)
>> +		return rc;
>> +
>>  	error_type = val;
>>  
>>  	return 0;
>> @@ -709,6 +736,12 @@ static int einj_check_table(struct acpi_table_einj *einj_tab)
>>  	return 0;
>>  }
>>  
>> +bool einj_is_initialized(void)
>> +{
>> +	return einj_initialized;
>> +}
>> +EXPORT_SYMBOL_GPL(einj_is_initialized);
> 
> The variable can be referenced directly as a global symbol.

Re: [PATCH v13 2/4] EINJ: Add CXL error type support

[Ben Cheatham]

On 2/21/24 2:27 PM, Ben Cheatham wrote:
> 
> 
> On 2/21/24 11:43 AM, Dan Williams wrote:
>> Ben Cheatham wrote:
>>> Remove CXL protocol error types from the EINJ module and move them to
>>> a new einj_cxl module. The einj_cxl module implements the necessary
>>> handling for CXL protocol error injection and exposes an API for the
>>> CXL core to use said functionality. Because the CXL error types
>>> require special handling, only allow them to be injected through the
>>> einj_cxl module and return an error when attempting to inject through
>>> "regular" EINJ.
>>
>> So Robustness Principle says be conservative in what you send and
>> liberal in what you accept. So cleaning up the reporting of CXL
>> capabilities over to the new interface is consistent with that
>> principle, but not removing the ability to inject via the legacy
>> interface. Especially since that has been the status quo for a few
>> kernel cycles is there a good reason to actively prevent usage of that
>> path?
>>
> 
> For CXL 2.0+ ports it's fine since EINJ only expects an SBDF which is
> pretty readily accessible by the user. CXL 1.1/1.0 ports however, it's a bit
> of a headache. It would require the user to find the address of the RCRB
> for the port and supply that to the EINJ module. I originally had this option
> anyway, but I think it got shot down for being too obtuse to use (I think by
> you, but it's been a while xD). If you think it's still worthwhile I can
> remove the restriction for both types of ports or just the 2.0+ ports.
> 
> For CXL 1.0/1.1 ports there's also the security issue of being able to inject
> to any address since the way it works is by skipping the memory address
> checks, but since this is a debug module I don't think it's that big
> of a deal.
> 
>>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
>>> ---
>>>  MAINTAINERS                       |   1 +
>>>  drivers/acpi/apei/Kconfig         |  12 +++
>>>  drivers/acpi/apei/Makefile        |   1 +
>>>  drivers/acpi/apei/apei-internal.h |  17 +++++
>>>  drivers/acpi/apei/einj-cxl.c      | 121 ++++++++++++++++++++++++++++++
>>>  drivers/acpi/apei/einj.c          |  81 ++++++++++++++------
>>>  include/linux/einj-cxl.h          |  40 ++++++++++
>>>  7 files changed, 249 insertions(+), 24 deletions(-)
>>>  create mode 100644 drivers/acpi/apei/einj-cxl.c
>>>  create mode 100644 include/linux/einj-cxl.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 73d898383e51..51f9a0da57d7 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -5289,6 +5289,7 @@ M:	Dan Williams <dan.j.williams@intel.com>
>>>  L:	linux-cxl@vger.kernel.org
>>>  S:	Maintained
>>>  F:	drivers/cxl/
>>> +F:	include/linux/cxl-einj.h
>>>  F:	include/linux/cxl-event.h
>>>  F:	include/uapi/linux/cxl_mem.h
>>>  F:	tools/testing/cxl/
>>> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
>>> index 6b18f8bc7be3..040a9b2de235 100644
>>> --- a/drivers/acpi/apei/Kconfig
>>> +++ b/drivers/acpi/apei/Kconfig
>>> @@ -60,6 +60,18 @@ config ACPI_APEI_EINJ
>>>  	  mainly used for debugging and testing the other parts of
>>>  	  APEI and some other RAS features.
>>>  
>>> +config ACPI_APEI_EINJ_CXL
>>> +	tristate "CXL Error INJection Support"
>>
>> This should still be a boolean because it is add-on functionality to the
>> cxl_core.ko module which has its own tristate configuration.
>>
> 
> I tried this but was running into issues, more about this in the Makefile
> portion of the patch.
> 
>>> +	default ACPI_APEI_EINJ
>>> +	depends on ACPI_APEI_EINJ
>>
>> The dependency still needs to be:
>>
>>     depends on ACPI_APEI_EINJ && CXL_BUS >= ACPI_APEI_EINJ
>>
>> ...because CXL_BUS can not tolerate being built-in when ACPI_APEI_EINJ
>> is not.
>>
> 
> Will do.
> 

Sorry, little clarifying question. Shouldn't this be CXL_BUS <= ACPI_APEI_EINJ instead?
The other way would allow CXL_BUS to be built-in while ACPI_APEI_EINJ is a module, right?

Thanks,
Ben

>>> +	help
>>> +	  Support for CXL protocol Error INJection through debugfs/cxl.
>>> +	  Availability and which errors are supported is dependent on
>>> +	  the host platform. Look to ACPI v6.5 section 18.6.4 and kernel
>>> +	  EINJ documentation for more information.
>>> +
>>> +	  If unsure say 'n'
>>> +
>>>  config ACPI_APEI_ERST_DEBUG
>>>  	tristate "APEI Error Record Serialization Table (ERST) Debug Support"
>>>  	depends on ACPI_APEI
>>> diff --git a/drivers/acpi/apei/Makefile b/drivers/acpi/apei/Makefile
>>> index 4dfac2128737..c18e96d342b2 100644
>>> --- a/drivers/acpi/apei/Makefile
>>> +++ b/drivers/acpi/apei/Makefile
>>> @@ -2,6 +2,7 @@
>>>  obj-$(CONFIG_ACPI_APEI)		+= apei.o
>>>  obj-$(CONFIG_ACPI_APEI_GHES)	+= ghes.o
>>>  obj-$(CONFIG_ACPI_APEI_EINJ)	+= einj.o
>>> +obj-$(CONFIG_ACPI_APEI_EINJ_CXL) += einj-cxl.o
>>
>> No new module needed. It only needs another compilation unit optionally
>> added to einj.ko. Something like this:
>>
>> diff --git a/drivers/acpi/apei/Makefile b/drivers/acpi/apei/Makefile
>> index 4dfac2128737..2c474e6477e1 100644
>> --- a/drivers/acpi/apei/Makefile
>> +++ b/drivers/acpi/apei/Makefile
>> @@ -2,6 +2,8 @@
>>  obj-$(CONFIG_ACPI_APEI)                += apei.o
>>  obj-$(CONFIG_ACPI_APEI_GHES)   += ghes.o
>>  obj-$(CONFIG_ACPI_APEI_EINJ)   += einj.o
>> +einj-y                         := einj-core.o
>> +einj-$(CONFIG_ACPI_APEI_EINJ_CXL) += einj-cxl.o
>>  obj-$(CONFIG_ACPI_APEI_ERST_DEBUG) += erst-dbg.o
>>  
>>  apei-y := apei-base.o hest.o erst.o bert.o
>> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj-core.c
>> similarity index 100%
>> rename from drivers/acpi/apei/einj.c
>> rename to drivers/acpi/apei/einj-core.c
>>
> 
> And this is what was causing my issues. I couldn't CONFIG_ACPI_APEI_EINJ_CXL work
> as a boolean because I didn't put it in the right compilation unit. This should
> also allow me to remove the dependency in the next patch and the exports below.
> 
>>>  obj-$(CONFIG_ACPI_APEI_ERST_DEBUG) += erst-dbg.o
>>>  
>>>  apei-y := apei-base.o hest.o erst.o bert.o
>>> diff --git a/drivers/acpi/apei/apei-internal.h b/drivers/acpi/apei/apei-internal.h
>>> index 67c2c3b959e1..336408f4f293 100644
>>> --- a/drivers/acpi/apei/apei-internal.h
>>> +++ b/drivers/acpi/apei/apei-internal.h
>>> @@ -130,4 +130,21 @@ static inline u32 cper_estatus_len(struct acpi_hest_generic_status *estatus)
>>>  }
>>>  
>>>  int apei_osc_setup(void);
>>> +
>>> +int einj_get_available_error_type(u32 *type);
>>> +int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
>>> +		      u64 param4);
>>> +bool einj_is_initialized(void);
>>> +bool einj_is_cxl_error_type(u64 type);
>>> +int einj_validate_error_type(u64 type);
>>> +
>>> +#ifndef ACPI_EINJ_CXL_CACHE_CORRECTABLE
>>> +#define ACPI_EINJ_CXL_CACHE_CORRECTABLE     BIT(12)
>>> +#define ACPI_EINJ_CXL_CACHE_UNCORRECTABLE   BIT(13)
>>> +#define ACPI_EINJ_CXL_CACHE_FATAL           BIT(14)
>>> +#define ACPI_EINJ_CXL_MEM_CORRECTABLE       BIT(15)
>>> +#define ACPI_EINJ_CXL_MEM_UNCORRECTABLE     BIT(16)
>>> +#define ACPI_EINJ_CXL_MEM_FATAL             BIT(17)
>>> +#endif
>>> +
>>>  #endif
>>> diff --git a/drivers/acpi/apei/einj-cxl.c b/drivers/acpi/apei/einj-cxl.c
>>> new file mode 100644
>>> index 000000000000..607d4f6adb98
>>> --- /dev/null
>>> +++ b/drivers/acpi/apei/einj-cxl.c
>>> @@ -0,0 +1,121 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * CXL Error INJection support. Used by CXL core to inject
>>> + * protocol errors into CXL ports.
>>> + *
>>> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
>>> + *
>>> + * Author: Ben Cheatham <benjamin.cheatham@amd.com>
>>> + */
>>> +#include <linux/einj-cxl.h>
>>> +#include <linux/debugfs.h>
>>> +
>>> +#include "apei-internal.h"
>>> +
>>> +static struct { u32 mask; const char *str; } const einj_cxl_error_type_string[] = {
>>> +	{ BIT(12), "CXL.cache Protocol Correctable" },
>>> +	{ BIT(13), "CXL.cache Protocol Uncorrectable non-fatal" },
>>> +	{ BIT(14), "CXL.cache Protocol Uncorrectable fatal" },
>>> +	{ BIT(15), "CXL.mem Protocol Correctable" },
>>> +	{ BIT(16), "CXL.mem Protocol Uncorrectable non-fatal" },
>>> +	{ BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
>>> +};
>>> +
>>> +int einj_cxl_available_error_type_show(struct seq_file *m, void *v)
>>> +{
>>> +	int cxl_err, rc;
>>> +	u32 available_error_type = 0;
>>> +
>>> +	if (!einj_is_initialized())
>>> +		return -ENXIO;
>>> +
>>> +	rc = einj_get_available_error_type(&available_error_type);
>>> +	if (rc)
>>> +		return rc;
>>> +
>>> +	for (int pos = 0; pos < ARRAY_SIZE(einj_cxl_error_type_string); pos++) {
>>> +		cxl_err = ACPI_EINJ_CXL_CACHE_CORRECTABLE << pos;
>>> +
>>> +		if (available_error_type & cxl_err)
>>> +			seq_printf(m, "0x%08x\t%s\n",
>>> +				   einj_cxl_error_type_string[pos].mask,
>>> +				   einj_cxl_error_type_string[pos].str);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(einj_cxl_available_error_type_show, CXL);
>>> +
>>> +static int cxl_dport_get_sbdf(struct pci_dev *dport_dev, u64 *sbdf)
>>> +{
>>> +	struct pci_bus *pbus;
>>> +	struct pci_host_bridge *bridge;
>>> +	u64 seg = 0, bus;
>>> +
>>> +	pbus = dport_dev->bus;
>>> +	bridge = pci_find_host_bridge(pbus);
>>> +
>>> +	if (!bridge)
>>> +		return -ENODEV;
>>> +
>>> +	if (bridge->domain_nr != PCI_DOMAIN_NR_NOT_SET)
>>> +		seg = bridge->domain_nr;
>>> +
>>> +	bus = pbus->number;
>>> +	*sbdf = (seg << 24) | (bus << 16) | dport_dev->devfn;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int einj_cxl_inject_rch_error(u64 rcrb, u64 type)
>>> +{
>>> +	int rc;
>>> +
>>> +	if (!einj_is_initialized())
>>> +		return -ENXIO;
>>> +
>>> +	/* Only CXL error types can be specified */
>>> +	if (!einj_is_cxl_error_type(type))
>>> +		return -EINVAL;
>>> +
>>> +	rc = einj_validate_error_type(type);
>>> +	if (rc)
>>> +		return rc;
>>> +
>>> +	return einj_error_inject(type, 0x2, rcrb, GENMASK_ULL(63, 12), 0, 0);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(einj_cxl_inject_rch_error, CXL);
>>> +
>>> +int einj_cxl_inject_error(struct pci_dev *dport, u64 type)
>>> +{
>>> +	u64 param4 = 0;
>>> +	int rc;
>>> +
>>> +	if (!einj_is_initialized())
>>> +		return -ENXIO;
>>> +
>>> +	/* Only CXL error types can be specified */
>>> +	if (!einj_is_cxl_error_type(type))
>>> +		return -EINVAL;
>>> +
>>> +	rc = einj_validate_error_type(type);
>>> +	if (rc)
>>> +		return rc;
>>> +
>>> +	rc = cxl_dport_get_sbdf(dport, &param4);
>>> +	if (rc)
>>> +		return rc;
>>> +
>>> +	return einj_error_inject(type, 0x4, 0, 0, 0, param4);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(einj_cxl_inject_error, CXL);
>>> +
>>> +bool einj_cxl_is_initialized(void)
>>> +{
>>> +	return einj_is_initialized();
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(einj_cxl_is_initialized, CXL);
>>> +
>>> +MODULE_AUTHOR("Ben Cheatham");
>>> +MODULE_DESCRIPTION("CXL Error INJection support");
>>> +MODULE_LICENSE("GPL");
>>
>> These go away when cxl-einj.ko is no longer its own module.
>>
>>> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
>>> index 6ea323b9d8ef..e76e64df97a7 100644
>>> --- a/drivers/acpi/apei/einj.c
>>> +++ b/drivers/acpi/apei/einj.c
>>> @@ -37,6 +37,12 @@
>>>  #define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
>>>  				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
>>>  				ACPI_EINJ_MEMORY_FATAL)
>>> +#define CXL_ERROR_MASK		(ACPI_EINJ_CXL_CACHE_CORRECTABLE | \
>>> +				ACPI_EINJ_CXL_CACHE_UNCORRECTABLE | \
>>> +				ACPI_EINJ_CXL_CACHE_FATAL | \
>>> +				ACPI_EINJ_CXL_MEM_CORRECTABLE | \
>>> +				ACPI_EINJ_CXL_MEM_UNCORRECTABLE | \
>>> +				ACPI_EINJ_CXL_MEM_FATAL)
>>>  
>>>  /*
>>>   * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
>>> @@ -166,7 +172,7 @@ static int __einj_get_available_error_type(u32 *type)
>>>  }
>>>  
>>>  /* Get error injection capabilities of the platform */
>>> -static int einj_get_available_error_type(u32 *type)
>>> +int einj_get_available_error_type(u32 *type)
>>>  {
>>>  	int rc;
>>>  
>>> @@ -176,6 +182,7 @@ static int einj_get_available_error_type(u32 *type)
>>>  
>>>  	return rc;
>>>  }
>>> +EXPORT_SYMBOL_GPL(einj_get_available_error_type);
>>
>> There should not be any need for new exports from the legacy einj.c.
>>
>>>  
>>>  static int einj_timedout(u64 *t)
>>>  {
>>> @@ -536,8 +543,8 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>>>  }
>>>  
>>>  /* Inject the specified hardware error */
>>> -static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>>> -			     u64 param3, u64 param4)
>>> +int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
>>> +		      u64 param4)
>>>  {
>>>  	int rc;
>>>  	u64 base_addr, size;
>>> @@ -560,8 +567,11 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>>>  	if (type & ACPI5_VENDOR_BIT) {
>>>  		if (vendor_flags != SETWA_FLAGS_MEM)
>>>  			goto inject;
>>> -	} else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM))
>>> +	} else if (!(type & MEM_ERROR_MASK) && !(flags & SETWA_FLAGS_MEM)) {
>>>  		goto inject;
>>> +	} else if ((type & CXL_ERROR_MASK) && (flags & SETWA_FLAGS_MEM)) {
>>> +		goto inject;
>>> +	}
>>>  
>>>  	/*
>>>  	 * Disallow crazy address masks that give BIOS leeway to pick
>>> @@ -592,6 +602,7 @@ static int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>>>  
>>>  	return rc;
>>>  }
>>> +EXPORT_SYMBOL_GPL(einj_error_inject);
>>>  
>>>  static u32 error_type;
>>>  static u32 error_flags;
>>> @@ -613,12 +624,6 @@ static struct { u32 mask; const char *str; } const einj_error_type_string[] = {
>>>  	{ BIT(9), "Platform Correctable" },
>>>  	{ BIT(10), "Platform Uncorrectable non-fatal" },
>>>  	{ BIT(11), "Platform Uncorrectable fatal"},
>>> -	{ BIT(12), "CXL.cache Protocol Correctable" },
>>> -	{ BIT(13), "CXL.cache Protocol Uncorrectable non-fatal" },
>>> -	{ BIT(14), "CXL.cache Protocol Uncorrectable fatal" },
>>> -	{ BIT(15), "CXL.mem Protocol Correctable" },
>>> -	{ BIT(16), "CXL.mem Protocol Uncorrectable non-fatal" },
>>> -	{ BIT(17), "CXL.mem Protocol Uncorrectable fatal" },
>>>  	{ BIT(31), "Vendor Defined Error Types" },
>>>  };
>>>  
>>> @@ -640,29 +645,21 @@ static int available_error_type_show(struct seq_file *m, void *v)
>>>  
>>>  DEFINE_SHOW_ATTRIBUTE(available_error_type);
>>>  
>>> -static int error_type_get(void *data, u64 *val)
>>> -{
>>> -	*val = error_type;
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int error_type_set(void *data, u64 val)
>>> +int einj_validate_error_type(u64 type)
>>>  {
>>> +	u32 tval, vendor, available_error_type = 0;
>>>  	int rc;
>>> -	u32 available_error_type = 0;
>>> -	u32 tval, vendor;
>>>  
>>>  	/* Only low 32 bits for error type are valid */
>>> -	if (val & GENMASK_ULL(63, 32))
>>> +	if (type & GENMASK_ULL(63, 32))
>>>  		return -EINVAL;
>>>  
>>>  	/*
>>>  	 * Vendor defined types have 0x80000000 bit set, and
>>>  	 * are not enumerated by ACPI_EINJ_GET_ERROR_TYPE
>>>  	 */
>>> -	vendor = val & ACPI5_VENDOR_BIT;
>>> -	tval = val & 0x7fffffff;
>>> +	vendor = type & ACPI5_VENDOR_BIT;
>>> +	tval = type & GENMASK(30, 0);
>>>  
>>>  	/* Only one error type can be specified */
>>>  	if (tval & (tval - 1))
>>> @@ -671,9 +668,39 @@ static int error_type_set(void *data, u64 val)
>>>  		rc = einj_get_available_error_type(&available_error_type);
>>>  		if (rc)
>>>  			return rc;
>>> -		if (!(val & available_error_type))
>>> +		if (!(type & available_error_type))
>>>  			return -EINVAL;
>>>  	}
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(einj_validate_error_type);
>>> +
>>> +bool einj_is_cxl_error_type(u64 type)
>>> +{
>>> +	return (type & CXL_ERROR_MASK) && (!(type & ACPI5_VENDOR_BIT));
>>> +}
>>> +EXPORT_SYMBOL_GPL(einj_is_cxl_error_type);
>>> +
>>> +static int error_type_get(void *data, u64 *val)
>>> +{
>>> +	*val = error_type;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int error_type_set(void *data, u64 val)
>>> +{
>>> +	int rc;
>>> +
>>> +	/* CXL error types have to be injected from cxl debugfs */
>>> +	if (einj_is_cxl_error_type(val))
>>> +		return -EINVAL;
>>> +
>>> +	rc = einj_validate_error_type(val);
>>> +	if (rc)
>>> +		return rc;
>>> +
>>>  	error_type = val;
>>>  
>>>  	return 0;
>>> @@ -709,6 +736,12 @@ static int einj_check_table(struct acpi_table_einj *einj_tab)
>>>  	return 0;
>>>  }
>>>  
>>> +bool einj_is_initialized(void)
>>> +{
>>> +	return einj_initialized;
>>> +}
>>> +EXPORT_SYMBOL_GPL(einj_is_initialized);
>>
>> The variable can be referenced directly as a global symbol.
> 

Re: [PATCH v13 2/4] EINJ: Add CXL error type support

[Dan Williams]

Ben Cheatham wrote:
> 
> 
> On 2/21/24 11:43 AM, Dan Williams wrote:
> > Ben Cheatham wrote:
> >> Remove CXL protocol error types from the EINJ module and move them to
> >> a new einj_cxl module. The einj_cxl module implements the necessary
> >> handling for CXL protocol error injection and exposes an API for the
> >> CXL core to use said functionality. Because the CXL error types
> >> require special handling, only allow them to be injected through the
> >> einj_cxl module and return an error when attempting to inject through
> >> "regular" EINJ.
> > 
> > So Robustness Principle says be conservative in what you send and
> > liberal in what you accept. So cleaning up the reporting of CXL
> > capabilities over to the new interface is consistent with that
> > principle, but not removing the ability to inject via the legacy
> > interface. Especially since that has been the status quo for a few
> > kernel cycles is there a good reason to actively prevent usage of that
> > path?
> > 
> 
> For CXL 2.0+ ports it's fine since EINJ only expects an SBDF which is
> pretty readily accessible by the user. CXL 1.1/1.0 ports however, it's a bit
> of a headache. It would require the user to find the address of the RCRB
> for the port and supply that to the EINJ module. I originally had this option
> anyway, but I think it got shot down for being too obtuse to use (I think by
> you, but it's been a while xD). If you think it's still worthwhile I can
> remove the restriction for both types of ports or just the 2.0+ ports.

So, to be clear, yes I NAKd that being the primary interface, and I am
not changing my mind on it being too obtuse to inflict on end users.
However, just because it is too obtuse to be the primary interface does
not mean that if someone runs that gauntlet, or has expert knowledge of
where RCRB is located, that they be tripped up at the last moment from
specifying that parameter via the legacy path.

So consider it an undocumented feature, and I think it only ends up
being a one line change to let that parameter through, if it can be done
safely.

That said, if there is any concern about input validation and safety
then keep the policy as you have it.

> For CXL 1.0/1.1 ports there's also the security issue of being able to inject
> to any address since the way it works is by skipping the memory address
> checks, but since this is a debug module I don't think it's that big
> of a deal.

Say more here, this seems like just the safety issue I just mentioned
that would justify forcing folks through the CXL interface. Depending on
how severe this is it might also require backporting the removal of CXL
injection from older kernels.

The main concern would be whether einj needs to disabled when kernel
lockdown is enabled.

Re: [PATCH v13 2/4] EINJ: Add CXL error type support

[Ben Cheatham]

On 2/21/24 2:41 PM, Dan Williams wrote:
> Ben Cheatham wrote:
>>
>>
>> On 2/21/24 11:43 AM, Dan Williams wrote:
>>> Ben Cheatham wrote:
>>>> Remove CXL protocol error types from the EINJ module and move them to
>>>> a new einj_cxl module. The einj_cxl module implements the necessary
>>>> handling for CXL protocol error injection and exposes an API for the
>>>> CXL core to use said functionality. Because the CXL error types
>>>> require special handling, only allow them to be injected through the
>>>> einj_cxl module and return an error when attempting to inject through
>>>> "regular" EINJ.
>>>
>>> So Robustness Principle says be conservative in what you send and
>>> liberal in what you accept. So cleaning up the reporting of CXL
>>> capabilities over to the new interface is consistent with that
>>> principle, but not removing the ability to inject via the legacy
>>> interface. Especially since that has been the status quo for a few
>>> kernel cycles is there a good reason to actively prevent usage of that
>>> path?
>>>
>>
>> For CXL 2.0+ ports it's fine since EINJ only expects an SBDF which is
>> pretty readily accessible by the user. CXL 1.1/1.0 ports however, it's a bit
>> of a headache. It would require the user to find the address of the RCRB
>> for the port and supply that to the EINJ module. I originally had this option
>> anyway, but I think it got shot down for being too obtuse to use (I think by
>> you, but it's been a while xD). If you think it's still worthwhile I can
>> remove the restriction for both types of ports or just the 2.0+ ports.
> 
> So, to be clear, yes I NAKd that being the primary interface, and I am
> not changing my mind on it being too obtuse to inflict on end users.
> However, just because it is too obtuse to be the primary interface does
> not mean that if someone runs that gauntlet, or has expert knowledge of
> where RCRB is located, that they be tripped up at the last moment from
> specifying that parameter via the legacy path.
> 
> So consider it an undocumented feature, and I think it only ends up
> being a one line change to let that parameter through, if it can be done
> safely.
> 
> That said, if there is any concern about input validation and safety
> then keep the policy as you have it.
> 
>> For CXL 1.0/1.1 ports there's also the security issue of being able to inject
>> to any address since the way it works is by skipping the memory address
>> checks, but since this is a debug module I don't think it's that big
>> of a deal.
> 
> Say more here, this seems like just the safety issue I just mentioned
> that would justify forcing folks through the CXL interface. Depending on
> how severe this is it might also require backporting the removal of CXL
> injection from older kernels.
> 
> The main concern would be whether einj needs to disabled when kernel
> lockdown is enabled.

So the way the EINJ module currently works (at least as I understand it)
is that any address supplied for memory errors is checked to make sure it's
a "normal" memory address. Looking at the comment above the memory checks:

	/*
	 * Disallow crazy address masks that give BIOS leeway to pick
	 * injection address almost anywhere. Insist on page or
	 * better granularity and that target address is normal RAM or
	 * NVDIMM.
	 */

it seems that's the case. What this means is that we can't supply the
RCRB of a CXL 1.0/1.1 port because it's an MMIO address and we have to disable
the checks to inject a CXL 1.0/1.1 error. So, there won't have to be anything
done in terms of backporting since CXL error injections will get caught by this
filter in all kernels that don't have these patches. For kernels that do have
these patches though, there won't be a check on the address you supply as long
as you specify a CXL error type.

So, it seems like a bad idea to provide legacy access for CXL 1.0/1.1 ports
due to this issue. CXL 2.0+ ports don't suffer from this issue though since
they are specified by an SBDF, not a MMIO address. And looking at the code,
it looks like EINJ error types that use an SBDF already get through the
filter. So it doesn't look like there's actually anything to be done for
CXL 2.0+ ports and the new interface is just a convenience feature for 2.0+
ports.

Thanks,
Ben

Re: [PATCH v13 2/4] EINJ: Add CXL error type support

[Dan Williams]

Ben Cheatham wrote:
[..]
> So, it seems like a bad idea to provide legacy access for CXL 1.0/1.1 ports
> due to this issue. CXL 2.0+ ports don't suffer from this issue though since
> they are specified by an SBDF, not a MMIO address. And looking at the code,
> it looks like EINJ error types that use an SBDF already get through the
> filter. So it doesn't look like there's actually anything to be done for
> CXL 2.0+ ports and the new interface is just a convenience feature for 2.0+
> ports.

Ok, I am on board. Continue the status quo of blocking CXL 1.1 error
injection, and let CXL 2.0 error injection be accomplished through
either interface.

Re: Re: [PATCH v13 2/4] EINJ: Add CXL error type support

[Davidlohr Bueso]

On Wed, 21 Feb 2024, Ben Cheatham wrote:

>So the way the EINJ module currently works (at least as I understand it)
>is that any address supplied for memory errors is checked to make sure it's
>a "normal" memory address. Looking at the comment above the memory checks:
>
>	/*
>	 * Disallow crazy address masks that give BIOS leeway to pick
>	 * injection address almost anywhere. Insist on page or
>	 * better granularity and that target address is normal RAM or
>	 * NVDIMM.
>	 */
>
>it seems that's the case. What this means is that we can't supply the
>RCRB of a CXL 1.0/1.1 port because it's an MMIO address and we have to disable
>the checks to inject a CXL 1.0/1.1 error.

Maybe worth a comment here as to why the error checking is skipped for cxl?

+	} else if ((type & CXL_ERROR_MASK) && (flags & SETWA_FLAGS_MEM)) {
+		goto inject;

Thanks,
Davidlohr

Re: [PATCH v13 2/4] EINJ: Add CXL error type support

[Ben Cheatham]

On 2/22/24 7:13 PM, Davidlohr Bueso wrote:
> On Wed, 21 Feb 2024, Ben Cheatham wrote:
> 
>> So the way the EINJ module currently works (at least as I understand it)
>> is that any address supplied for memory errors is checked to make sure it's
>> a "normal" memory address. Looking at the comment above the memory checks:
>>
>>     /*
>>      * Disallow crazy address masks that give BIOS leeway to pick
>>      * injection address almost anywhere. Insist on page or
>>      * better granularity and that target address is normal RAM or
>>      * NVDIMM.
>>      */
>>
>> it seems that's the case. What this means is that we can't supply the
>> RCRB of a CXL 1.0/1.1 port because it's an MMIO address and we have to disable
>> the checks to inject a CXL 1.0/1.1 error.
> 
> Maybe worth a comment here as to why the error checking is skipped for cxl?
> 
> +    } else if ((type & CXL_ERROR_MASK) && (flags & SETWA_FLAGS_MEM)) {
> +        goto inject;
> 

I think that's a good idea, I'll go ahead and add one in.

Thanks,
Ben

> Thanks,
> Davidlohr

Re: [PATCH v13 2/4] EINJ: Add CXL error type support

[kernel test robot]

Hi Ben,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on linus/master v6.8-rc5 next-20240221]
[cannot apply to rafael-pm/acpi-bus rafael-pm/devprop]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ben-Cheatham/EINJ-Migrate-to-a-platform-driver/20240221-061359
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240220221146.399209-3-Benjamin.Cheatham%40amd.com
patch subject: [PATCH v13 2/4] EINJ: Add CXL error type support
config: i386-buildonly-randconfig-004-20240221 (https://download.01.org/0day-ci/archive/20240222/202402221559.7ZWbaUpL-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240222/202402221559.7ZWbaUpL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402221559.7ZWbaUpL-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/acpi/apei/einj-cxl.o: in function `einj_cxl_inject_error':
>> einj-cxl.c:(.text+0x166): undefined reference to `pci_find_host_bridge'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v13 3/4] cxl/core: Add CXL EINJ debugfs files

[Ben Cheatham]

Export CXL helper functions in the einj_cxl module for getting/injecting
available CXL protocol error types to sysfs under kernel/debug/cxl.

The kernel/debug/cxl/einj_types file will print the available CXL
protocol errors in the same format as the available_error_types
file provided by the einj_cxl module. The
kernel/debug/cxl/$dport_dev/einj_inject is functionally the same as the
error_type and error_inject files provided by the EINJ module, i.e.:
writing an error type into $dport_dev/einj_inject will inject said error
type into the CXL dport represented by $dport_dev.

Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
---
 Documentation/ABI/testing/debugfs-cxl | 30 ++++++++++++++++++++
 drivers/cxl/Kconfig                   |  1 +
 drivers/cxl/core/port.c               | 41 +++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

diff --git a/Documentation/ABI/testing/debugfs-cxl b/Documentation/ABI/testing/debugfs-cxl
index fe61d372e3fa..4c0f62f881ca 100644
--- a/Documentation/ABI/testing/debugfs-cxl
+++ b/Documentation/ABI/testing/debugfs-cxl
@@ -33,3 +33,33 @@ Description:
 		device cannot clear poison from the address, -ENXIO is returned.
 		The clear_poison attribute is only visible for devices
 		supporting the capability.
+
+What:		/sys/kernel/debug/cxl/einj_types
+Date:		January, 2024
+KernelVersion:	v6.9
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RO) Prints the CXL protocol error types made available by
+		the platform in the format "0x<error number>	<error type>".
+		The possible error types are (as of ACPI v6.5):
+			0x1000	CXL.cache Protocol Correctable
+			0x2000	CXL.cache Protocol Uncorrectable non-fatal
+			0x4000	CXL.cache Protocol Uncorrectable fatal
+			0x8000	CXL.mem Protocol Correctable
+			0x10000	CXL.mem Protocol Uncorrectable non-fatal
+			0x20000	CXL.mem Protocol Uncorrectable fatal
+
+		The <error number> can be written to einj_inject to inject
+		<error type> into a chosen dport.
+
+What:		/sys/kernel/debug/cxl/$dport_dev/einj_inject
+Date:		January, 2024
+KernelVersion:	v6.9
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(WO) Writing an integer to this file injects the corresponding
+		CXL protocol error into $dport_dev ($dport_dev will be a device
+		name from /sys/bus/pci/devices). The integer to type mapping for
+		injection can be found by reading from einj_types. If the dport
+		was enumerated in RCH mode, a CXL 1.1 error is injected, otherwise
+		a CXL 2.0 error is injected.
diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
index 67998dbd1d46..c86ae4c65c03 100644
--- a/drivers/cxl/Kconfig
+++ b/drivers/cxl/Kconfig
@@ -2,6 +2,7 @@
 menuconfig CXL_BUS
 	tristate "CXL (Compute Express Link) Devices Support"
 	depends on PCI
+	depends on ACPI_APEI_EINJ_CXL || !ACPI_APEI_EINJ_CXL
 	select FW_LOADER
 	select FW_UPLOAD
 	select PCI_DOE
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index e59d9d37aa65..3b579eef4d23 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -3,6 +3,7 @@
 #include <linux/platform_device.h>
 #include <linux/memregion.h>
 #include <linux/workqueue.h>
+#include <linux/einj-cxl.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/module.h>
@@ -793,6 +794,39 @@ static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
 	return rc;
 }
 
+DEFINE_SHOW_ATTRIBUTE(einj_cxl_available_error_type);
+
+static int cxl_einj_inject(void *data, u64 type)
+{
+	struct cxl_dport *dport = data;
+
+	if (dport->rch)
+		return einj_cxl_inject_rch_error(dport->rcrb.base, type);
+
+	return einj_cxl_inject_error(to_pci_dev(dport->dport_dev), type);
+}
+DEFINE_DEBUGFS_ATTRIBUTE(cxl_einj_inject_fops, NULL, cxl_einj_inject, "%llx\n");
+
+static void cxl_debugfs_create_dport_dir(struct cxl_dport *dport)
+{
+	struct dentry *dir;
+
+	if (!einj_cxl_is_initialized())
+		return;
+
+	/*
+	 * dport_dev needs to be a PCIe port for CXL 2.0+ ports because
+	 * EINJ expects a dport SBDF to be specified for 2.0 error injection.
+	 */
+	if (!dport->rch && !dev_is_pci(dport->dport_dev))
+		return;
+
+	dir = cxl_debugfs_create_dir(dev_name(dport->dport_dev));
+
+	debugfs_create_file("einj_inject", 0200, dir, dport,
+			    &cxl_einj_inject_fops);
+}
+
 static struct cxl_port *__devm_cxl_add_port(struct device *host,
 					    struct device *uport_dev,
 					    resource_size_t component_reg_phys,
@@ -1149,6 +1183,8 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
 	if (dev_is_pci(dport_dev))
 		dport->link_latency = cxl_pci_get_latency(to_pci_dev(dport_dev));
 
+	cxl_debugfs_create_dport_dir(dport);
+
 	return dport;
 }
 
@@ -2221,6 +2257,11 @@ static __init int cxl_core_init(void)
 
 	cxl_debugfs = debugfs_create_dir("cxl", NULL);
 
+	if (einj_cxl_is_initialized()) {
+		debugfs_create_file("einj_types", 0400, cxl_debugfs, NULL,
+				    &einj_cxl_available_error_type_fops);
+	}
+
 	cxl_mbox_init();
 
 	rc = cxl_memdev_init();
-- 
2.34.1

RE: [PATCH v13 3/4] cxl/core: Add CXL EINJ debugfs files

[Dan Williams]

Ben Cheatham wrote:
> Export CXL helper functions in the einj_cxl module for getting/injecting
> available CXL protocol error types to sysfs under kernel/debug/cxl.
> 
> The kernel/debug/cxl/einj_types file will print the available CXL
> protocol errors in the same format as the available_error_types
> file provided by the einj_cxl module. The
> kernel/debug/cxl/$dport_dev/einj_inject is functionally the same as the
> error_type and error_inject files provided by the EINJ module, i.e.:
> writing an error type into $dport_dev/einj_inject will inject said error
> type into the CXL dport represented by $dport_dev.
> 
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> ---
>  Documentation/ABI/testing/debugfs-cxl | 30 ++++++++++++++++++++
>  drivers/cxl/Kconfig                   |  1 +
>  drivers/cxl/core/port.c               | 41 +++++++++++++++++++++++++++
>  3 files changed, 72 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/debugfs-cxl b/Documentation/ABI/testing/debugfs-cxl
> index fe61d372e3fa..4c0f62f881ca 100644
> --- a/Documentation/ABI/testing/debugfs-cxl
> +++ b/Documentation/ABI/testing/debugfs-cxl
> @@ -33,3 +33,33 @@ Description:
>  		device cannot clear poison from the address, -ENXIO is returned.
>  		The clear_poison attribute is only visible for devices
>  		supporting the capability.
> +
> +What:		/sys/kernel/debug/cxl/einj_types
> +Date:		January, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) Prints the CXL protocol error types made available by
> +		the platform in the format "0x<error number>	<error type>".
> +		The possible error types are (as of ACPI v6.5):
> +			0x1000	CXL.cache Protocol Correctable
> +			0x2000	CXL.cache Protocol Uncorrectable non-fatal
> +			0x4000	CXL.cache Protocol Uncorrectable fatal
> +			0x8000	CXL.mem Protocol Correctable
> +			0x10000	CXL.mem Protocol Uncorrectable non-fatal
> +			0x20000	CXL.mem Protocol Uncorrectable fatal
> +
> +		The <error number> can be written to einj_inject to inject
> +		<error type> into a chosen dport.
> +
> +What:		/sys/kernel/debug/cxl/$dport_dev/einj_inject
> +Date:		January, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(WO) Writing an integer to this file injects the corresponding
> +		CXL protocol error into $dport_dev ($dport_dev will be a device
> +		name from /sys/bus/pci/devices). The integer to type mapping for
> +		injection can be found by reading from einj_types. If the dport
> +		was enumerated in RCH mode, a CXL 1.1 error is injected, otherwise
> +		a CXL 2.0 error is injected.
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 67998dbd1d46..c86ae4c65c03 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -2,6 +2,7 @@
>  menuconfig CXL_BUS
>  	tristate "CXL (Compute Express Link) Devices Support"
>  	depends on PCI
> +	depends on ACPI_APEI_EINJ_CXL || !ACPI_APEI_EINJ_CXL

This statement is always true "x || !x"

I mentioned in the other patch that ACPI_APEI_EINJ_CXL needs to have a
dependency on CXL_BUS.

Re: [PATCH v13 3/4] cxl/core: Add CXL EINJ debugfs files

[Ben Cheatham]

On 2/21/24 11:48 AM, Dan Williams wrote:
> Ben Cheatham wrote:
>> Export CXL helper functions in the einj_cxl module for getting/injecting
>> available CXL protocol error types to sysfs under kernel/debug/cxl.
>>
>> The kernel/debug/cxl/einj_types file will print the available CXL
>> protocol errors in the same format as the available_error_types
>> file provided by the einj_cxl module. The
>> kernel/debug/cxl/$dport_dev/einj_inject is functionally the same as the
>> error_type and error_inject files provided by the EINJ module, i.e.:
>> writing an error type into $dport_dev/einj_inject will inject said error
>> type into the CXL dport represented by $dport_dev.
>>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
>> ---
>>  Documentation/ABI/testing/debugfs-cxl | 30 ++++++++++++++++++++
>>  drivers/cxl/Kconfig                   |  1 +
>>  drivers/cxl/core/port.c               | 41 +++++++++++++++++++++++++++
>>  3 files changed, 72 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/debugfs-cxl b/Documentation/ABI/testing/debugfs-cxl
>> index fe61d372e3fa..4c0f62f881ca 100644
>> --- a/Documentation/ABI/testing/debugfs-cxl
>> +++ b/Documentation/ABI/testing/debugfs-cxl
>> @@ -33,3 +33,33 @@ Description:
>>  		device cannot clear poison from the address, -ENXIO is returned.
>>  		The clear_poison attribute is only visible for devices
>>  		supporting the capability.
>> +
>> +What:		/sys/kernel/debug/cxl/einj_types
>> +Date:		January, 2024
>> +KernelVersion:	v6.9
>> +Contact:	linux-cxl@vger.kernel.org
>> +Description:
>> +		(RO) Prints the CXL protocol error types made available by
>> +		the platform in the format "0x<error number>	<error type>".
>> +		The possible error types are (as of ACPI v6.5):
>> +			0x1000	CXL.cache Protocol Correctable
>> +			0x2000	CXL.cache Protocol Uncorrectable non-fatal
>> +			0x4000	CXL.cache Protocol Uncorrectable fatal
>> +			0x8000	CXL.mem Protocol Correctable
>> +			0x10000	CXL.mem Protocol Uncorrectable non-fatal
>> +			0x20000	CXL.mem Protocol Uncorrectable fatal
>> +
>> +		The <error number> can be written to einj_inject to inject
>> +		<error type> into a chosen dport.
>> +
>> +What:		/sys/kernel/debug/cxl/$dport_dev/einj_inject
>> +Date:		January, 2024
>> +KernelVersion:	v6.9
>> +Contact:	linux-cxl@vger.kernel.org
>> +Description:
>> +		(WO) Writing an integer to this file injects the corresponding
>> +		CXL protocol error into $dport_dev ($dport_dev will be a device
>> +		name from /sys/bus/pci/devices). The integer to type mapping for
>> +		injection can be found by reading from einj_types. If the dport
>> +		was enumerated in RCH mode, a CXL 1.1 error is injected, otherwise
>> +		a CXL 2.0 error is injected.
>> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
>> index 67998dbd1d46..c86ae4c65c03 100644
>> --- a/drivers/cxl/Kconfig
>> +++ b/drivers/cxl/Kconfig
>> @@ -2,6 +2,7 @@
>>  menuconfig CXL_BUS
>>  	tristate "CXL (Compute Express Link) Devices Support"
>>  	depends on PCI
>> +	depends on ACPI_APEI_EINJ_CXL || !ACPI_APEI_EINJ_CXL
> 
> This statement is always true "x || !x"

Yeah, I lifted this from the Kconfig documentation (https://www.kernel.org/doc/html/next/kbuild/kconfig-language.html#kconfig-hints).
It allows CXL_BUS to built if ACPI_APEI_EINJ_CXL=n, but restricts CXL_BUS >= ACPI_APEI_EINJ_CXL otherwise.
Now that I think about it, it seems like this just flipped the dependency I had in v12 the other way.

> 
> I mentioned in the other patch that ACPI_APEI_EINJ_CXL needs to have a
> dependency on CXL_BUS.

Yep I agree, I was just having issues and was able to get it working this way.
I'll change this back to what it was.

Thanks,
Ben

[PATCH v13 4/4] EINJ, Documentation: Update EINJ kernel doc

[Ben Cheatham]

Update EINJ kernel document to include how to inject CXL protocol error
types, build the kernel to include CXL error types, and give an example
injection.

Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
---
 .../firmware-guide/acpi/apei/einj.rst         | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/firmware-guide/acpi/apei/einj.rst b/Documentation/firmware-guide/acpi/apei/einj.rst
index d6b61d22f525..23741ec0de75 100644
--- a/Documentation/firmware-guide/acpi/apei/einj.rst
+++ b/Documentation/firmware-guide/acpi/apei/einj.rst
@@ -181,6 +181,27 @@ You should see something like this in dmesg::
   [22715.834759] EDAC sbridge MC3: PROCESSOR 0:306e7 TIME 1422553404 SOCKET 0 APIC 0
   [22716.616173] EDAC MC3: 1 CE memory read error on CPU_SrcID#0_Channel#0_DIMM#0 (channel:0 slot:0 page:0x12345 offset:0x0 grain:32 syndrome:0x0 -  area:DRAM err_code:0001:0090 socket:0 channel_mask:1 rank:0)
 
+CXL error types are supported from ACPI 6.5 onwards (given a CXL port
+is present). These error types are not available in the legacy interface
+at /sys/kernel/debug/apei/einj, and are instead at /sys/kernel/debug/cxl/.
+There is a file under debug/cxl called "einj_type" that is analogous to
+available_error_type under debug/cxl. There is also a "einj_inject" file
+in each $dport_dev directory under debug/cxl that will inject a given error
+into the dport represented by $dport_dev.
+
+For example, to inject a CXL.mem protocol correctable error into
+$dport_dev=pci0000:0c::
+
+    # cd /sys/kernel/debug/cxl/
+    # cat einj_type                 # See which error can be injected
+	0x00008000  CXL.mem Protocol Correctable
+	0x00010000  CXL.mem Protocol Uncorrectable non-fatal
+	0x00020000  CXL.mem Protocol Uncorrectable fatal
+    # cd 0000:e0:01.1               # Navigate to dport to inject into
+    # echo 0x8000 > einj_inject     # Inject error
+
+To use CXL error types, ``CONFIG_ACPI_APEI_CXL_EINJ`` will need to be enabled.
+
 Special notes for injection into SGX enclaves:
 
 There may be a separate BIOS setup option to enable SGX injection.
-- 
2.34.1

Re: [PATCH v13 4/4] EINJ, Documentation: Update EINJ kernel doc

[Dan Williams]

Ben Cheatham wrote:
> Update EINJ kernel document to include how to inject CXL protocol error
> types, build the kernel to include CXL error types, and give an example
> injection.
> 
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> ---
>  .../firmware-guide/acpi/apei/einj.rst         | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/firmware-guide/acpi/apei/einj.rst b/Documentation/firmware-guide/acpi/apei/einj.rst
> index d6b61d22f525..23741ec0de75 100644
> --- a/Documentation/firmware-guide/acpi/apei/einj.rst
> +++ b/Documentation/firmware-guide/acpi/apei/einj.rst
> @@ -181,6 +181,27 @@ You should see something like this in dmesg::
>    [22715.834759] EDAC sbridge MC3: PROCESSOR 0:306e7 TIME 1422553404 SOCKET 0 APIC 0
>    [22716.616173] EDAC MC3: 1 CE memory read error on CPU_SrcID#0_Channel#0_DIMM#0 (channel:0 slot:0 page:0x12345 offset:0x0 grain:32 syndrome:0x0 -  area:DRAM err_code:0001:0090 socket:0 channel_mask:1 rank:0)
>  
> +CXL error types are supported from ACPI 6.5 onwards (given a CXL port
> +is present). These error types are not available in the legacy interface
> +at /sys/kernel/debug/apei/einj, and are instead at /sys/kernel/debug/cxl/.
> +There is a file under debug/cxl called "einj_type" that is analogous to
> +available_error_type under debug/cxl. There is also a "einj_inject" file
> +in each $dport_dev directory under debug/cxl that will inject a given error
> +into the dport represented by $dport_dev.

Follow the style of the current document and document the files in a
section following this one:

---
The EINJ user interface is in <debugfs mount point>/apei/einj.

The following files belong to it:
---

> +
> +For example, to inject a CXL.mem protocol correctable error into
> +$dport_dev=pci0000:0c::
> +
> +    # cd /sys/kernel/debug/cxl/
> +    # cat einj_type                 # See which error can be injected
> +	0x00008000  CXL.mem Protocol Correctable
> +	0x00010000  CXL.mem Protocol Uncorrectable non-fatal
> +	0x00020000  CXL.mem Protocol Uncorrectable fatal
> +    # cd 0000:e0:01.1               # Navigate to dport to inject into
> +    # echo 0x8000 > einj_inject     # Inject error
> +
> +To use CXL error types, ``CONFIG_ACPI_APEI_CXL_EINJ`` will need to be enabled.

Config symbols are already communicated in this list:

---
To use EINJ, make sure the following are options enabled in your kernel
configuration::

  CONFIG_DEBUG_FS
  CONFIG_ACPI_APEI
  CONFIG_ACPI_APEI_EINJ
---

...so just add something like:

---
diff --git a/Documentation/firmware-guide/acpi/apei/einj.rst b/Documentation/firmware-guide/acpi/apei/e>
index d6b61d22f525..3bfd86f50c61 100644
--- a/Documentation/firmware-guide/acpi/apei/einj.rst
+++ b/Documentation/firmware-guide/acpi/apei/einj.rst
@@ -32,6 +32,10 @@ configuration::
   CONFIG_ACPI_APEI
   CONFIG_ACPI_APEI_EINJ
 
+...and optionally to enable CXL protocol error injection set:
+
+  CONFIG_ACPI_APEI_EINJ_CXL
+
 The EINJ user interface is in <debugfs mount point>/apei/einj.
 
 The following files belong to it:
---

...just to make it look like the document was written by a single
author.

RE: [PATCH v13 0/4] cxl, EINJ: Update EINJ for CXL error types

[Dan Williams]

Ben Cheatham wrote:
> v13 Changes:
> 	- Create new einj-cxl module for EINJ CXL error type functionality
> 	- Rename CONFIG_CXL_EINJ to CONFIG_ACPI_APEI_EINJ_CXL
> 	- Move CONFIG_ACPI_APEI_CXL to be under CONFIG_ACPI_APEI_EINJ (due to
> 	  new CONFIG_CXL_BUS dependency)
> 	- Add an optional dependency to CONFIG_CXL_BUS on
> 	  CONFIG_ACPI_APEI_EINJ_CXL 
> 	- Change pr_warn("EINJ table not found.") to a pr_info() for when/if
> 	  EINJ probe fails (Tony)

pr_info() is too chatty once CXL starts triggering this module to
autoload. I will take a look for anything else that might trigger
another spin, but if that s/pr_info/pr_debug/ change is all that's
needed I can handle that on applying.

> 	- Add a clarification that a CXL port needs to be present for CXL 
> 	  EINJ error types to einj.rst (Davidlohr)
> 
> v12 Changes:
> 	- Rebase onto v6.8-rc4
> 	- Squash Kconfig patch into patch 2/3 (Jonathan)
> 	- Change CONFIG_CXL_EINJ from "depends on ACPI_APEI_EINJ >= CXL_BUS"
> 	  to "depends on ACPI_APEI_EINJ = CXL_BUS"
> 	- Drop "ACPI, APEI" part of commit message title and use just EINJ
> 	instead (Dan)
> 	- Add protocol error types to "einj_types" documentation (Jonathan)
> 	- Change 0xffff... constants to use GENMASK()
> 	- Drop param* variables and use constants instead in cxl error
> 	  inject functions (Jonathan)
> 	- Add is_cxl_error_type() helper function in einj.c (Jonathan)
> 	- Remove a stray function declaration in einj-cxl.h (Jonathan)
> 	- Comment #else/#endifs with corresponding #if/#ifdef in
> 	einj-cxl.h (Jonathan)
> 
> The new CXL error types will use the Memory Address field in the
> SET_ERROR_TYPE_WITH_ADDRESS structure in order to target a CXL 1.1
> compliant memory-mapped downstream port. The value of the memory address
> will be in the port's MMIO range, and it will not represent physical
> (normal or persistent) memory.
> 
> Add the functionality for injecting CXL 1.1 errors to the EINJ module,
> but not through the EINJ legacy interface under /sys/kernel/debug/apei/einj.
> Instead, make the error types available under /sys/kernel/debug/cxl.
> This allows for validating the MMIO address for a CXL 1.1 error type
> while also not making the user responsible for finding it.
> 
> Ben Cheatham (4):
>   EINJ: Migrate to a platform driver
>   EINJ: Add CXL error type support
>   cxl/core: Add CXL EINJ debugfs files
>   EINJ, Documentation: Update EINJ kernel doc
> 
>  Documentation/ABI/testing/debugfs-cxl         |  30 +++++
>  .../firmware-guide/acpi/apei/einj.rst         |  21 +++
>  MAINTAINERS                                   |   1 +
>  drivers/acpi/apei/Kconfig                     |  12 ++
>  drivers/acpi/apei/Makefile                    |   1 +
>  drivers/acpi/apei/apei-internal.h             |  17 +++
>  drivers/acpi/apei/einj-cxl.c                  | 121 +++++++++++++++++
>  drivers/acpi/apei/einj.c                      | 127 ++++++++++++++----
>  drivers/cxl/Kconfig                           |   1 +
>  drivers/cxl/core/port.c                       |  41 ++++++
>  include/linux/einj-cxl.h                      |  40 ++++++
>  11 files changed, 385 insertions(+), 27 deletions(-)
>  create mode 100644 drivers/acpi/apei/einj-cxl.c
>  create mode 100644 include/linux/einj-cxl.h
> 
> -- 
> 2.34.1
>