Message-ID: <9ac12446-816c-4f99-b4de-24d51d457185@amd.com>
Date: Wed, 5 Feb 2025 08:22:40 -0600
From: "Bowman, Terry" <terry.bowman@....com>
To: Li Ming <ming.li@...omail.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 2/5/2025 7:58 AM, Li Ming wrote:
> On 2/5/2025 11:46 AM, Bowman, Terry wrote:
>> 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@....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."
>>>
>> 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?
> One problem I realized is if using cxl_handle_error() instead of pci_aer_handle_error() for the above case I described(a CXL RP is working on PCIe mode because it connected to a PCIe device), the CXL RP will miss pcie_do_recovery() invoked in pci_aer_handle_error() when the error is an UCE, and it will also miss pcie error handler implemented in pcie port driver. 
>
> It means that AER handling logic is different between CXL RP working on PCIe mode and PCIe RP. I am not sure whether it is OK.
>
>
> Although cxl_handle_error() includes cxl_do_recovery() implemented in patch #7, cxl_do_recovery() seems like only for CXL cases(CXL RP working on CXL mode), is it suitable for pcie port recovery(CXL RP working on PCIe mode)?
>
> Please correct me if I am wrong.
>
>
> Ming

Hi Ming,

Yes, the plan is to handle CXL protocol errors in a separate CXL path than PCIe protocol errors.

You stated this is a problem. Can you elaborate on the issue ?

Regards,
Terry

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

Powered by blists - more mailing lists