On 12/7/23 6:03 PM, Dan Williams wrote:
> Ben Cheatham wrote:
>> Add functions to the EINJ module to be used in the CXL module for CXL
>> protocol error injection. The callbacks implement the einj_types and
>> einj_inject files under /sys/kernel/debug/cxl/portX/dportY. These two
>> files work in the same way as the available_error_types and error_inject
>> files under /sys/kernel/debug/apei/einj, but only for CXL error types.
>> If the dport is enumerated in RCH (CXL 1.1) mode, a CXL 1.1 error is
>> injected into the dport; a CXL 2.0 error is injected otherwise.
>>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@xxxxxxx>
>> ---
>> drivers/acpi/apei/Kconfig | 3 ++
>> drivers/acpi/apei/einj.c | 105 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 108 insertions(+)
>>
>> diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
>> index 6b18f8bc7be3..100c27beb581 100644
>> --- a/drivers/acpi/apei/Kconfig
>> +++ b/drivers/acpi/apei/Kconfig
>> @@ -11,6 +11,9 @@ config ACPI_APEI
>> select PSTORE
>> select UEFI_CPER
>> depends on HAVE_ACPI_APEI
>> + imply CXL_BUS
>> + imply CXL_ACPI
>> + imply CXL_PORT
>
> This goes away with the CONFIG_CXL_EINJ scheme proposed on patch1.
>
>> help
>> APEI allows to report errors (for example from the chipset)
>> to the operating system. This improves NMI handling
>> diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
>> index 330329ac2f1f..98d5e6d60a02 100644
>> --- a/drivers/acpi/apei/einj.c
>> +++ b/drivers/acpi/apei/einj.c
>> @@ -21,9 +21,11 @@
>> #include <linux/nmi.h>
>> #include <linux/delay.h>
>> #include <linux/mm.h>
>> +#include <linux/pci.h>
>> #include <asm/unaligned.h>
>>
>> #include "apei-internal.h"
>> +#include "../../cxl/cxl.h"
>
> EINJ has no business knowing all of the details in cxl.h. In fact all
> EINJ cares about is dport->dport_dev and dport->rch. I think just add
> those to the calling convention, see below:
>
>> #undef pr_fmt
>> #define pr_fmt(fmt) "EINJ: " fmt
>> @@ -627,6 +629,25 @@ static int available_error_type_show(struct seq_file *m, void *v)
>>
>> DEFINE_SHOW_ATTRIBUTE(available_error_type);
>>
>> +static int cxl_einj_available_error_type(struct seq_file *m, void *v)
>> +{
>> + int cxl_err, rc;
>> + u32 available_error_type = 0;
>> +
>> + 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_puts(m, einj_cxl_error_type_string[pos]);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int validate_error_type(u64 type)
>> {
>> u32 tval, vendor, available_error_type = 0;
>> @@ -657,6 +678,68 @@ static int validate_error_type(u64 type)
>> return 0;
>> }
>>
>> +static int cxl_dport_get_sbdf(struct cxl_dport *dport, u64 *sbdf)
>> +{
>> + struct pci_dev *pdev;
>> + struct pci_bus *pbus;
>> + struct pci_host_bridge *bridge;
>> + u64 seg = 0, bus;
>> +
>> + if (!dev_is_pci(dport->dport_dev))
>> + return -EINVAL;
>> +
>> + pdev = to_pci_dev(dport->dport_dev);
>> + pbus = pdev->bus;
>> + bridge = pci_find_host_bridge(pbus);
>> +
>> + if (!bridge)
>> + return -ENODEV;
>> +
>> + if (bridge->domain_nr != PCI_DOMAIN_NR_NOT_SET)
>> + seg = bridge->domain_nr << 24;
>> +
>> + bus = pbus->number << 16;
>> + *sbdf = seg | bus | pdev->devfn;
>> +
>> + return 0;
>> +}
>> +
>> +static int cxl_einj_inject_error(struct cxl_dport *dport, u64 type)
>
> If you split this into cxl_einj_inject_error() and
> cxl_einj_inject_rch_error() with following prototypes:
>
> static int cxl_einj_inject_error(struct pci_dev *dport_dev, u64 type)
> static int cxl_einj_inject_rch_error(u64 rcrb, u64 type)
>
> ...then you don't need any of the cxl.h definitions. The dev_is_pci()
> and rch determination details can remain private to CXL.
>
Good suggestion. Will do!
Thanks,
Ben
>> +{
>> + u64 param1 = 0, param2 = 0, param4 = 0;
>> + u32 flags;
>> + int rc;
>> +
>> + /* Only CXL error types can be specified */
>> + if (type & ~CXL_ERROR_MASK || (type & ACPI5_VENDOR_BIT))
>> + return -EINVAL;
>> +
>> + rc = validate_error_type(type);
>> + if (rc)
>> + return rc;
>> +
>> + /*
>> + * If dport is in restricted mode, inject a CXL 1.1 error,
>> + * otherwise inject a CXL 2.0 error
>> + */
>> + if (dport->rch) {
>> + if (dport->rcrb.base == CXL_RESOURCE_NONE)
>> + return -EINVAL;
>> +
>> + param1 = (u64) dport->rcrb.base;
>> + param2 = 0xfffffffffffff000;
>> + flags = 0x2;
>> + } else {
>> + rc = cxl_dport_get_sbdf(dport, ¶m4);
>> + if (rc)
>> + return rc;
>> +
>> + flags = 0x4;
>> + }
>> +
>> + return einj_error_inject(type, flags, param1, param2, 0, param4);
>> +}
>> +
>> static int error_type_get(void *data, u64 *val)
>> {
>> *val = error_type;
>> @@ -668,6 +751,7 @@ static int error_type_set(void *data, u64 val)
>> {
>> int rc;
>>
>> + /* CXL error types have to be injected from cxl debugfs */
>> if (val & CXL_ERROR_MASK && !(val & ACPI5_VENDOR_BIT))
>> return -EINVAL;
>>
>> @@ -714,6 +798,7 @@ static int __init einj_init(void)
>> {
>> int rc;
>> acpi_status status;
>> + struct cxl_einj_ops cxl_ops;
>> struct apei_exec_context ctx;
>>
>> if (acpi_disabled) {
>> @@ -793,6 +878,15 @@ static int __init einj_init(void)
>> einj_debug_dir, &vendor_flags);
>> }
>>
>> + if (IS_REACHABLE(CONFIG_CXL_ACPI) || IS_REACHABLE(CONFIG_CXL_PORT)) {
>> + cxl_ops = (struct cxl_einj_ops) {
>> + .einj_type = cxl_einj_available_error_type,
>> + .einj_inject = cxl_einj_inject_error,
>> + };
>> +
>> + cxl_einj_set_ops_cbs(&cxl_ops);
>> + }
>
> This goes away with the new Kconfig dependency scheme.
>
>> +
>> pr_info("Error INJection is initialized.\n");
>>
>> return 0;
>> @@ -810,8 +904,18 @@ static int __init einj_init(void)
>>
>> static void __exit einj_exit(void)
>> {
>> + struct cxl_einj_ops cxl_ops;
>> struct apei_exec_context ctx;
>>
>> + if (IS_REACHABLE(CONFIG_CXL_ACPI) || IS_REACHABLE(CONFIG_CXL_PORT)) {
>> + cxl_ops = (struct cxl_einj_ops) {
>> + .einj_type = NULL,
>> + .einj_inject = NULL,
>> + };
>> +
>> + cxl_einj_set_ops_cbs(&cxl_ops);
>> + }
>> +
>> if (einj_param) {
>> acpi_size size = (acpi5) ?
>> sizeof(struct set_error_type_with_address) :
>> @@ -832,4 +936,5 @@ module_exit(einj_exit);
>>
>> MODULE_AUTHOR("Huang Ying");
>> MODULE_DESCRIPTION("APEI Error INJection support");
>> +MODULE_IMPORT_NS(CXL);
>
> EINJ never references a CXL symbol in the new proposed scheme.
