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

[Ben Cheatham <benjamin.cheatham@amd.com>]

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