Message-ID: <a8950b91-3485-4dd5-9d84-7d3a3031b752@zohomail.com>
Date: Thu, 16 Jan 2025 11:15:52 +0800
From: Li Ming <ming.li@...omail.com>
To: "Bowman, Terry" <terry.bowman@....com>, linux-cxl@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
 nifan.cxl@...il.com, dave@...olabs.net, jonathan.cameron@...wei.com,
 dave.jiang@...el.com, alison.schofield@...el.com, vishal.l.verma@...el.com,
 dan.j.williams@...el.com, bhelgaas@...gle.com, mahesh@...ux.ibm.com,
 ira.weiny@...el.com, oohall@...il.com, Benjamin.Cheatham@....com,
 rrichter@....com, nathan.fontenot@....com,
 Smita.KoralahalliChannabasappa@....com, lukas@...ner.de,
 PradeepVineshReddy.Kodamati@....com, alucerop@....com
Subject: Re: [PATCH v5 05/16] PCI/AER: Add CXL PCIe Port correctable error
 support in AER service driver

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@....com>
>>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>>>>> Reviewed-by: Dave Jiang <dave.jiang@...el.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."

>
> 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);
>>>>>  }
>>>>>  

Powered by blists - more mailing lists