Re: [PATCH v4 2/3] ACPI, APEI, EINJ: Add CXL 1.1 EINJ error type support

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

On 9/12/23 9:00 AM, Jonathan Cameron wrote:
> On Thu, 7 Sep 2023 14:19:55 -0500
> Ben Cheatham <Benjamin.Cheatham@amd.com> wrote:
> 
>> Add support for CXL EINJ error types for CXL 1.1 hosts added in ACPI
>> v6.5. Because these error types target memory-mapped CXL 1.1 compliant
>> downstream ports and not physical (normal/persistent) memory, these
>> error types are not currently  allowed through the memory range
>> validation done by the EINJ driver.
>>
>> The MMIO address of a CXL 1.1 downstream port can be found in the
>> cxl_rcrb_addr file in the corresponding dport directory under
>> /sys/bus/cxl/devices/portX. CXL 1.1 error types follow the same
>> procedure as a memory error type, but with param1 set to the
>> downstream port MMIO address.
>>
>> Example usage:
>> $ cd /sys/kernel/debug/apei/einj
>> $ cat available_error_type
>>     0x00000008      Memory Correctable
>>     0x00000010      Memory Uncorrectable non-fatal
>>     0x00000020      Memory Uncorrectable fatal
>>     0x00000040      PCI Express Correctable
>>     0x00000080      PCI Express Uncorrectable non-fatal
>>     0x00000100      PCI Express Uncorrectable fatal
>>     0x00008000      CXL.mem Protocol Correctable
>>     0x00020000      CXL.mem Protocol Uncorrectable fatal
>> $ echo 0x8000 > error_type
>> $ echo 0xfffffffffffff000 > param2
>> $ echo 0x3 > flags
> 
> From einj.rst (and the ACPI spec) bit 0 here is
> Processor APIC field valid.  Why is that relevant here?
> If it were you'd be writing param3 I think.  So probably
> harmless, but I think this should be 0x2 > flags
> 

I don't think it matters in this case, but I agree.
I'll change it to use 0x2 instead.

>> $ cat /sys/bus/cxl/devices/portX/dportY/cxl_rcrb_addr
>> 0xb2f00000
>> $ echo 0xb2f00000 > param1
>> $ echo 1 > error_inject
>>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> Hi Ben,
> 
> Why bother with the config dependent build?
> It doesn't save that much in code built and right now there
> are no non ACPI CXL systems so in reality it's always built
> anyway.
> 

Ok, I'll remove the config dependencies. It'll make my life a
lot easier :).

> Otherwise this LGTM
> 
> 
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 001ab8742e21..f8f300496140 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -1122,6 +1122,25 @@ struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>>  }
>>  EXPORT_SYMBOL_NS_GPL(devm_cxl_add_rch_dport, CXL);
>>  
>> +#if IS_ENABLED(CONFIG_CXL_ACPI)
> I'm not a particular fan processor magic down in c files.
> 
> Do we really care about the saving of not having this in builds where
> CONFIG_CXL_ACPI isn't set?  I'm thinking those don't really exist.
> 

I don't like it much either, I did it as a quick fix for the build errors
from the kernel test bot.

Thanks,
Ben

>> +struct cxl_dport *cxl_find_rch_dport_by_rcrb(resource_size_t rcrb_base)
>> +{
>> +	struct cxl_dport *dport;
>> +	unsigned long index;
>> +
>> +	if (!cxl_root)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	xa_for_each(&cxl_root->dports, index, dport)
>> +		if ((dport->rch && dport->rcrb.base != CXL_RESOURCE_NONE)
>> +		    && dport->rcrb.base == rcrb_base)
>> +			return dport;
>> +
>> +	return NULL;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_find_rch_dport_by_rcrb, CXL);
>> +#endif
>> +
>>  static int add_ep(struct cxl_ep *new)
>>  {
>>  	struct cxl_port *port = new->dport->port;
>