Re: [PATCH v5 05/16] PCI/AER: Add CXL PCIe Port correctable error support in AER service driver

["Bowman, Terry" <terry.bowman@amd.com>]

On 1/15/2025 9:15 PM, Li Ming wrote:
> On 1/15/2025 10:39 PM, Bowman, Terry wrote:
>>
>> On 1/14/2025 7:18 PM, Li Ming wrote:
>>> On 1/15/2025 3:29 AM, Bowman, Terry wrote:
>>>> On 1/14/2025 12:54 AM, Li Ming wrote:
>>>>> On 1/7/2025 10:38 PM, Terry Bowman wrote:
>>>>>> The AER service driver supports handling Downstream Port Protocol Errors in
>>>>>> Restricted CXL host (RCH) mode also known as CXL1.1. It needs the same
>>>>>> functionality for CXL PCIe Ports operating in Virtual Hierarchy (VH)
>>>>>> mode.[1]
>>>>>>
>>>>>> CXL and PCIe Protocol Error handling have different requirements that
>>>>>> necessitate a separate handling path. The AER service driver may try to
>>>>>> recover PCIe uncorrectable non-fatal errors (UCE). The same recovery is not
>>>>>> suitable for CXL PCIe Port devices because of potential for system memory
>>>>>> corruption. Instead, CXL Protocol Error handling must use a kernel panic
>>>>>> in the case of a fatal or non-fatal UCE. The AER driver's PCIe Protocol
>>>>>> Error handling does not panic the kernel in response to a UCE.
>>>>>>
>>>>>> Introduce a separate path for CXL Protocol Error handling in the AER
>>>>>> service driver. This will allow CXL Protocol Errors to use CXL specific
>>>>>> handling instead of PCIe handling. Add the CXL specific changes without
>>>>>> affecting or adding functionality in the PCIe handling.
>>>>>>
>>>>>> Make this update alongside the existing Downstream Port RCH error handling
>>>>>> logic, extending support to CXL PCIe Ports in VH mode.
>>>>>>
>>>>>> is_internal_error() is currently limited by CONFIG_PCIEAER_CXL kernel
>>>>>> config. Update is_internal_error()'s function declaration such that it is
>>>>>> always available regardless if CONFIG_PCIEAER_CXL kernel config is enabled
>>>>>> or disabled.
>>>>>>
>>>>>> The uncorrectable error (UCE) handling will be added in a future patch.
>>>>>>
>>>>>> [1] CXL 3.1 Spec, 12.2.2 CXL Root Ports, Downstream Switch Ports, and
>>>>>> Upstream Switch Ports
>>>>>>
>>>>>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>>>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>>>>>> ---
>>>>>>  drivers/pci/pcie/aer.c | 61 +++++++++++++++++++++++++++---------------
>>>>>>  1 file changed, 40 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>>>>> index f8b3350fcbb4..62be599e3bee 100644
>>>>>> --- a/drivers/pci/pcie/aer.c
>>>>>> +++ b/drivers/pci/pcie/aer.c
>>>>>> @@ -942,8 +942,15 @@ static bool find_source_device(struct pci_dev *parent,
>>>>>>  	return true;
>>>>>>  }
>>>>>>  
>>>>>> -#ifdef CONFIG_PCIEAER_CXL
>>>>>> +static bool is_internal_error(struct aer_err_info *info)
>>>>>> +{
>>>>>> +	if (info->severity == AER_CORRECTABLE)
>>>>>> +		return info->status & PCI_ERR_COR_INTERNAL;
>>>>>>  
>>>>>> +	return info->status & PCI_ERR_UNC_INTN;
>>>>>> +}
>>>>>> +
>>>>>> +#ifdef CONFIG_PCIEAER_CXL
>>>>>>  /**
>>>>>>   * pci_aer_unmask_internal_errors - unmask internal errors
>>>>>>   * @dev: pointer to the pcie_dev data structure
>>>>>> @@ -995,14 +1002,6 @@ static bool cxl_error_is_native(struct pci_dev *dev)
>>>>>>  	return (pcie_ports_native || host->native_aer);
>>>>>>  }
>>>>>>  
>>>>>> -static bool is_internal_error(struct aer_err_info *info)
>>>>>> -{
>>>>>> -	if (info->severity == AER_CORRECTABLE)
>>>>>> -		return info->status & PCI_ERR_COR_INTERNAL;
>>>>>> -
>>>>>> -	return info->status & PCI_ERR_UNC_INTN;
>>>>>> -}
>>>>>> -
>>>>>>  static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
>>>>>>  {
>>>>>>  	struct aer_err_info *info = (struct aer_err_info *)data;
>>>>>> @@ -1034,14 +1033,23 @@ static int cxl_rch_handle_error_iter(struct pci_dev *dev, void *data)
>>>>>>  
>>>>>>  static void cxl_handle_error(struct pci_dev *dev, struct aer_err_info *info)
>>>>>>  {
>>>>>> -	/*
>>>>>> -	 * Internal errors of an RCEC indicate an AER error in an
>>>>>> -	 * RCH's downstream port. Check and handle them in the CXL.mem
>>>>>> -	 * device driver.
>>>>>> -	 */
>>>>>> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC &&
>>>>>> -	    is_internal_error(info))
>>>>>> -		pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info);
>>>>>> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
>>>>>> +		return pcie_walk_rcec(dev, cxl_rch_handle_error_iter, info);
>>>>>> +
>>>>>> +	if (info->severity == AER_CORRECTABLE) {
>>>>>> +		struct pci_driver *pdrv = dev->driver;
>>>>>> +		int aer = dev->aer_cap;
>>>>>> +
>>>>>> +		if (aer)
>>>>>> +			pci_write_config_dword(dev, aer + PCI_ERR_COR_STATUS,
>>>>>> +					       info->status);
>>>>>> +
>>>>>> +		if (pdrv && pdrv->cxl_err_handler &&
>>>>>> +		    pdrv->cxl_err_handler->cor_error_detected)
>>>>>> +			pdrv->cxl_err_handler->cor_error_detected(dev);
>>>>>>
>>>>>> +		pcie_clear_device_status(dev);
>>>>>> +	}
>>>>>>  }
>>>>>>  
>>>>>>  static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
>>>>>> @@ -1059,9 +1067,13 @@ static bool handles_cxl_errors(struct pci_dev *dev)
>>>>>>  {
>>>>>>  	bool handles_cxl = false;
>>>>>>  
>>>>>> -	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC &&
>>>>>> -	    pcie_aer_is_native(dev))
>>>>>> +	if (!pcie_aer_is_native(dev))
>>>>>> +		return false;
>>>>>> +
>>>>>> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC)
>>>>>>  		pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl);
>>>>>> +	else
>>>>>> +		handles_cxl = pcie_is_cxl_port(dev);
>>>>> My understanding is if a cxl RP/USP/DSP is working on PCIe mode, they are also possible to expose a DVSEC ID 3(CXL r3.1 section 9.12.3). In such case, the AER handler should be pci_aer_handle_error() rather than cxl_handle_error().
>>>>>
>>>>> pcie_is_cxl_port() only checks if there is a DVSEC ID 3, but I think it should also check if the cxl port is working on CXL mode, does it make more sense?
>>>>>
>>>>>
>>>>> Ming
>>>> Hi Ming and Jonathan,
>>>>
>>>> RCH AER & RCH RAS are currently logged by the CXL driver's RCH handlers.
>>>>
>>>> If the recommended change is made then RCH RAS will not be logged and the
>>>> user would miss CXL details about the alternate protocol training failure.
>>>> Also, AER is not CXL required and as a result in some cases you would only
>>>> have the RCEC forwarded UIE/CIE message logged by the AER driver without
>>>> any other logging.
>>>>
>>>> Is there value in *not* logging CXL RAS for errors on an untrained RCH
>>>> link? Isn't it more informative to log PCIe AER and CXL RAS in this case?
>>>>
>>>> Regards,
>>>> Terry
>>> Hi Terry,
>>>
>>>
>>> I don't understand why the recommended change will influence RCH RAS handling, would you mind giving more details?
>>>
>>> My understanding is that above 'pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl)' is used for RCH case.
>>>
>>> And the 'else' block is used for VH case, so just check if the cxl port is working on CXL mode in pcie_is_cxl_port() or adding an extra function to check it in the 'else' block. I think it will not change RCH AER & RAS handling, is it right? or do I miss other details?
>>>
>>>
>>> Ming
>> Hi Ming,
>>
>> You're recommending this example case is handled by pci_aer_handle_error() rather than cxl_handle_error(). Correct me if I misunderstood. And, I believe this should continue to be handled by cxl_handle_error(). There are 2 issues with the recommended approach that deserve to be mentioned.
> I guess that what you thought is the recommended change using pci_aer_handle_error() to handle CXL RAS issues? If yes, it is not what I meant.
>
> handles_cxl_errors() is used to distinguish if the errors is a CXL error or a PCIe error. if the returned value of handles_cxl_errors() is 'true', that means the error is a CXL error. Then invoking either cxl_handle_error() or pcie_aer_handle_error() depending on the returned value. I think no problem in this part.
>
> handles_cxl_errors() is using pcie_is_cxl_port() to distinguish CXL errors for VH cases. the implementation of pcie_is_cxl_port() is only checking if there is a DVSEC ID 3 exposed on the CXL RP/DSP/USP. I think it is not enough.
>
> For example, If a CXL device connected to a CXL RP, there is no problem, because the return value of handles_cxl_errors() will be 'true' then cxl_handle_error() will be invoked to handle the errors.
>
> If a PCIe device connected to a CXL RP, the CXL RP is working on PCIe mode, the CXL RP is possible to expose a DVSEC ID 3[1]. If the CXL RP has a DVSEC ID 3 in the case, the return value of handles_cxl_errors() is also 'true' and also invoking cxl_handle_error() to handle the error, I thinks it is not right, the CXL RP is working on PCIe mode, the error should be a PCIe error, and it should be handled by pcie_aer_handle_error(). So my suggestion is about checking if the CXL RP/DSP/USP is working on CXL mode in pcie_is_cxl_port() for VH cases.
>
>
> [1] CXL r3.1 - 9.12.3 Enumerating CXL RPs and DSPs
>
>    "CXL root port or DSP connected to a PCIe device/switch may or may not expose theCXL DVSEC ID 3 and the CXL DVSEC ID 7 capability structures."
>

Hi Ming,

I apologize for the delayed response. Thanks for the patience in explaining.

In your example using a RP with downstream non-CXL device, the RP AER will log the
RP's CE/UCE and RAS status for a protocol error. It's not helpful in this case
because it's a non-CXL device but it is failing alternate prootcol training that can
also happen with a CXL endpoint. I expect the RAS registers contain details about
the failed CXL training in the endpoint case.

I believe we should give the user as much error details within reason. And for CXL using
AER CE/UCE errors, this should include the RAS logging. If we rely on the PCIe handling path,
this information will not be logged.

Also, CE/UCE AER is logged in the CXL handling path. The AER driver logs AER status before
calling the CE/UCE CXL handlers.

Are there any other use cases or reasons why to use PCIe handling if alt. protocol training
fails? Is there anything lost by using CXL handling?

Terry
>> First, the RCH Downstream Port (DP) is implemented as an RCRB and does not have a
>> SBDF.[1] The RCH AER error is reported with the RCEC SBDF in the AER SRC_ID register.[2] The
>> RCEC is used to find the RCH's handlers using a CXL unique procedure (see cxl_handle_error()).
>>
>> The logic in pci_aer_handle_error() operates on a 'struct pci_dev' type and pci_aer_handle_error() is not plumbed to support searching for the RCH handlers.
>>
>> Using pci_aer_handle_error would require significant changes to support a CXL RCH
>> in addition to a PCIe device. These changes are already in cxl_handle_error().
>>  
>> Another issue to note is the CXL RAS information will (should) not be logged with this
>> recommended change. pci_aer_handle_error is PCIe specific and is not aware of CXL RAS. As a result,pci_aer_handle_error() is not suited to log the CXL RAS.
>>
>> The example scenario was the RCH DP failed training. The user needs to know why training
>> failed and these details are stored in the CXL RAS registers. Again, CXL RAS needs to be logged
>> as well but CXL specific awareness shouldn't be added to pci_aer_handle_error().
> For these two issues, handles_cxl_errors() is always using "pcie_walk_rcec(dev, handles_cxl_error_iter, &handles_cxl)" for RCH cases. I believe no change on this part, the return value of handles_cxl_errors() will be 'true' as expected in the cases you mentioned, cxl_handle_error() will help to handle these errors.
>
>
> Ming
>
>> Terry
>>
>> [1] CXL r3.1 - 8.2 Memory Mapped Registers
>> [2] CXL r3.1 - 12.2.1.1 RCH Downstream Port-detected Errors+
>>>>>>  
>>>>>>  	return handles_cxl;
>>>>>>  }
>>>>>> @@ -1079,6 +1091,10 @@ static void cxl_enable_internal_errors(struct pci_dev *dev)
>>>>>>  static inline void cxl_enable_internal_errors(struct pci_dev *dev) { }
>>>>>>  static inline void cxl_handle_error(struct pci_dev *dev,
>>>>>>  				    struct aer_err_info *info) { }
>>>>>> +static bool handles_cxl_errors(struct pci_dev *dev)
>>>>>> +{
>>>>>> +	return false;
>>>>>> +}
>>>>>>  #endif
>>>>>>  
>>>>>>  /**
>>>>>> @@ -1116,8 +1132,11 @@ static void pci_aer_handle_error(struct pci_dev *dev, struct aer_err_info *info)
>>>>>>  
>>>>>>  static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info)
>>>>>>  {
>>>>>> -	cxl_handle_error(dev, info);
>>>>>> -	pci_aer_handle_error(dev, info);
>>>>>> +	if (is_internal_error(info) && handles_cxl_errors(dev))
>>>>>> +		cxl_handle_error(dev, info);
>>>>>> +	else
>>>>>> +		pci_aer_handle_error(dev, info);
>>>>>> +
>>>>>>  	pci_dev_put(dev);
>>>>>>  }
>>>>>>  
>