Received-SPF: pass (google.com: domain of linux-kernel+bounces-149544-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f0b85a37-ead0-4954-b7e0-dd09cbd9ab09@intel.com>
Date: Thu, 18 Apr 2024 13:53:50 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 3/6] PCI/AER: Enable RCEC to report internal error for
 CXL root port
To: Terry Bowman <Terry.Bowman@amd.com>, <dan.j.williams@intel.com>,
	<rrichter@amd.com>
CC: <linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240313083602.239201-1-ming4.li@intel.com>
 <20240313083602.239201-4-ming4.li@intel.com>
 <7172ec2f-7cba-460b-a6c6-9fff45ba938b@amd.com>
 <b4a721c2-567d-4ab3-8a85-963e3f323e61@intel.com>
 <91ddd182-cc71-480f-a1b2-e7c31b29a549@amd.com>
Content-Language: en-US
From: "Li, Ming" <ming4.li@intel.com>
Organization: Intel
In-Reply-To: <91ddd182-cc71-480f-a1b2-e7c31b29a549@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?djVJenUzUjh5REtXL2hxWWFObm1pcmx5WHpZeGxwTXBJRWU0NkNVN25pNzh2?=
 =?utf-8?B?SEk1c0VJUkFEdGJxaEdTSmdBenJJN3NPNjR0OWdyc0l1RzdGNWdDb1pSOGs1?=
 =?utf-8?B?UnlEMzlVd052RTlrOXRDbVRTdXp3aTJZN3M4UGdVZTQvNlpnMXEzSFlYM2wy?=
 =?utf-8?B?YUVZc1k3UkNHVS9hbmhPMENrdk9EYmplajE4S0xwbVRlUjNwaHNzaXV3Q0F2?=
 =?utf-8?B?SDUvMHlDSUpnWkRVNWI5SVF0OTJSREtwalhGdnYzNEM4dEtybjA2TS9peHRz?=
 =?utf-8?B?WHR6TmtoSFZ5QS8waEJSUzJPRlg0QU1IT0ZmZWp1ZHluRDgvT0VYTWN3dXJT?=
 =?utf-8?B?MktXNFFhbXF1a0tWSVVsZEhvaDdRYWRMNE1mZ1JZV0lmeForTlhGazMwTVhZ?=
 =?utf-8?B?ME5DVVBNSGdSbkxkNzQxN2NVMk92SWY4V0hMR3NpUjdCNmFRaklETEd6S1NX?=
 =?utf-8?B?R2d4YVhLUjNIc0tJVURVekhWbzgvQnBYeHRaWGN6TU05c3kvSzRFMC9sRWxh?=
 =?utf-8?B?UXZJRUhzd09VWm8xZG1oWWZpLzZ0ZWtaWHJSbmlrYkJUektsbTF3U3NDWGtF?=
 =?utf-8?B?ODNnUGZjRkRSL1pXVDNuc1crSHBXbUxkTk84T25HdUhGUWR1MUlQSWcxUGJ4?=
 =?utf-8?B?M0VXUjNNaDduMU5Zd1lTRW5Ibm1vN2ZtNndCUUs1SzBGczU5NnJuQXRTQjUy?=
 =?utf-8?B?WXlWcGZmWkt1UkMxSFN6REg5ZGxqTzl0NS9uMi91RmpOeDV0MHU0RzJjcTVP?=
 =?utf-8?B?bmY1c21DekJ0VkJYbVFhOGRJTTFiS3o0WkZOZ2VvRG4vNVJ6cHk4dVJ2WVBF?=
 =?utf-8?B?SVd1OWxwajl3Q0JTWFFtVkp3a1FoYmVCajBLcUhrNGxUblZvaFFSZnZZeGwv?=
 =?utf-8?B?RkpRKzUwb0lKRTZ0SzQrY3BsbkVIQ3BUb2hqN3Y1MWxtZEIrdEMyYmF1QTJ1?=
 =?utf-8?B?L0dBWGU2Y0JkQTZIUEpqc2JqcWkrT1hiOVd0Skk1Ris2V2NlS1p6Z3ZabG9k?=
 =?utf-8?B?QU4zVFVIekMxSXNtRUVXQXh6bUNGVkM5L05pa0NGMXRvVjh6UnRPNmxHL28r?=
 =?utf-8?B?NEZpVHlSTXFNenNqT2hrM3A2NFFJd0szL2g3S0Vnc3I3RldydmdjaXlqL0Jt?=
 =?utf-8?B?a1hqbjV2UHhCdjdSNDNsT3VDc2xzR0RZRUZrdi9ldDZNbmp3aXp3Nks1ckFn?=
 =?utf-8?B?azMycmg1Y1hSRnMrOUcrZnpqWFVmUGVGbEhFenE4OUlZQVFvNHlLWEI2L0Nx?=
 =?utf-8?B?YzZWTzRkaVlYL3VYR2lKc2tlcnZibXNWRnFSUFZDazl6cG1SSmxRMW5qVWkz?=
 =?utf-8?B?d2FFVVc3QWthcHpWZDdiOTJET0tiMFEyMlV1MFMwMFkxLytMWUljSUZRSHFJ?=
 =?utf-8?B?OXk1V3UveWprVkQ5MmpNWG9aQ09DQWRjN2dEaEJYNjZZMkFGRjNSN2NRbjIy?=
 =?utf-8?B?RFBUWW1LRXNHb0dFZFpXaTNWK2RzNlNYZkxUVTNXLzVLVDFGM3ovb01YcStm?=
 =?utf-8?B?eWtQZjhFN2J6K0hvVlhNMHNTWnpiK05WRjExTEhZcE9rNmdlQ0ZQSmhtaHN2?=
 =?utf-8?B?aldjTmlqblZlNmhWYXh0cnZIa3o4QW9lejVQVDczRmZlSC94cWpOc0w2Vnl6?=
 =?utf-8?B?VGplcEk4eGdBRWV3UkhtZnBzZnpuS243ek1MOXRud1dUdWpqQVVLN1FTRXBO?=
 =?utf-8?B?Tm13VkFpUWNSZmdNVElPaSt3MjV3UlFmWm1pU1FrVXEwdTh0WjZPWndhdWRS?=
 =?utf-8?B?N3M4RDVVODJscGpOTWphOGgydDBteDhqNWNrVloyZ2FhVk5EZ3dEb3VBWGky?=
 =?utf-8?B?Ti9tcTdiMXp1OGtzS1l3Z1RDZER5MHpmdHVieFRUSHlaeUNwRTNFZEpGd1ZM?=
 =?utf-8?B?ZlZyVENNUVhzNHpWRjJjREZPdXBvaE4xcDdGUjBJeU4weDBvLzdrVnZFVDVX?=
 =?utf-8?B?RFFKRXdLVEpiN3VLMHYyWEFMeERRTGJyNEZwQ2QrdHhSUkI3NEVaRnNPcWZM?=
 =?utf-8?B?ZXdGWEtSTnJFOTBLME5JTU9SNnB0cXo5STRWVEsvSUNGTmM5dG9DSFM1S3ky?=
 =?utf-8?B?S3Q1aUZxRFlaRk56U0FBYmZDVUwwYnk5STcwY20xcGRDNDNTcFdTY1FkTEdy?=
 =?utf-8?Q?8AH3TlL6P2E70WN6JD+aEafTO?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a63749e8-6c9b-447a-8e98-08dc5f6bf0d4
X-MS-Exchange-CrossTenant-AuthSource: IA1PR11MB7200.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 05:53:58.9626
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qyp99GZUc9Vx+my5iPVlXAZUFeNHgWZv2VNQdIxVov1YqZYeTulOaCOABox24pdxcAlDOGXgU+LOlZTqFDkgbw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4763
X-OriginatorOrg: intel.com

On 4/16/2024 10:46 PM, Terry Bowman wrote:
> Hi Ming,
> 
> On 4/16/24 02:27, Li, Ming wrote:
>> On 3/26/2024 3:42 AM, Terry Bowman wrote:
>>> Hi Li, 
>>>
>>> I added comments below.
>>>
>>> On 3/13/24 03:35, Li Ming wrote:
>>>> Per CXl r3.1 section 12.2.2, CXL.cachemem protocol erros detected by CXL
>>>> root port could be logged in RCEC AER Extended Capability as
>>>> PCI_ERR_UNC_INTN or PCI_ERR_COR_INTERNAL. Unmask these errors for that
>>>> case.
>>>>
>>>> Signed-off-by: Li Ming <ming4.li@intel.com>
>>>> ---
>>>>  drivers/pci/pcie/aer.c | 24 +++++++++++++++++-------
>>>>  1 file changed, 17 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>>>> index 42a3bd35a3e1..364c74e47273 100644
>>>> --- a/drivers/pci/pcie/aer.c
>>>> +++ b/drivers/pci/pcie/aer.c
>>>> @@ -985,7 +985,7 @@ static bool cxl_error_is_native(struct pci_dev *dev)
>>>>  {
>>>>  	struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
>>>>  
>>>> -	return (pcie_ports_native || host->native_aer);
>>>> +	return (pcie_ports_native || host->native_aer) && host->is_cxl;
>>>>  }
>>>>  
>>>>  static bool is_internal_error(struct aer_err_info *info)
>>>> @@ -1041,8 +1041,13 @@ static int handles_cxl_error_iter(struct pci_dev *dev, void *data)
>>>>  {
>>>>  	bool *handles_cxl = data;
>>>>  
>>>> -	if (!*handles_cxl)
>>>> -		*handles_cxl = is_cxl_mem_dev(dev) && cxl_error_is_native(dev);
>>>> +	if (!*handles_cxl && cxl_error_is_native(dev)) {
>>>> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_RC_END &&
>>>> +		    dev->rcec && is_cxl_mem_dev(dev))
>>>> +			*handles_cxl = true;
>>>> +		if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
>>>> +			*handles_cxl = true;
>>>> +	}
>>> I understand a root port can be found under an RCEC. It's possible. But, does the downstream 
>>> root port forward AER to the upstream RCEC? My understanding is AER is handled and processed
>>> at the first root port/RCEC upstream from the device/RCH/USP/DSP.
>>>  
>>> Regards,
>>> Terry
>>>
>>
>> CXL r3.1 section 12.2.2 mentions this:
>>
>> "If the CXL.cachemem protocol errors detected by a CXL root port are logged as
>> CIEs or UIEs in an RCEC’s AER Extended Capability, it is recommended that the System
>> Firmware populate an RDPAS record (see Section 9.18.1.5) to establish the association
>> between the RCEC and the root port."
>>
>> I think it means that CXL root port is possible to forward its AER to RCEC.
>>
>> Thanks
>> Ming
>>
> 
> Thanks for pointing to spec details. 
> 
> In testing here, we used root port as agent to consume root port CXL protocol errors.
> The logic to handle the root port errors requires little to no AER driver changes.
> This results in a root port consuming VH protocol errors and RCEC consuming RCD 
> protocol errors. The RCEC and root port both use the PCIe port bus driver's AER service
> driver in separate instances for RCEC-RCD and root-port-VH.
> 
> The driver support is much simpler if RCEC does not handle VH protocol errors. Is there 
> a reason to forward root port VH mode protocol errors to an RCEC rather than consume 
> in the root port's AER driver and forward to CXL error handler? 
> 
> Regards,
> Terry

I agree that is simpler if only root port handle VH protocol errors, but I think that software has no chance to choose if VH protocol errors reported to RCEC or root port, it depends on platform implementation. So I think we should support both cases.


Thanks
Ming

> 
>>>>  
>>>>  	/* Non-zero terminates iteration */
>>>>  	return *handles_cxl;
>>>> @@ -1054,13 +1059,18 @@ static bool handles_cxl_errors(struct pci_dev *rcec)
>>>>  
>>>>  	if (pci_pcie_type(rcec) == PCI_EXP_TYPE_RC_EC &&
>>>>  	    pcie_aer_is_native(rcec))
>>>> -		pcie_walk_rcec(rcec, handles_cxl_error_iter, &handles_cxl);
>>>> +		pcie_walk_rcec_all(rcec, handles_cxl_error_iter, &handles_cxl);
>>>>  
>>>>  	return handles_cxl;
>>>>  }
>>>>  
>>>> -static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>>>> +static void cxl_enable_rcec(struct pci_dev *rcec)
>>>>  {
>>>> +	/*
>>>> +	 * Enable RCEC's internal error report for two cases:
>>>> +	 * 1. RCiEP detected CXL.cachemem protocol errors
>>>> +	 * 2. CXL root port detected CXL.cachemem protocol errors.
>>>> +	 */
>>>>  	if (!handles_cxl_errors(rcec))
>>>>  		return;
>>>>  
>>>> @@ -1069,7 +1079,7 @@ static void cxl_rch_enable_rcec(struct pci_dev *rcec)
>>>>  }
>>>>  
>>>>  #else
>>>> -static inline void cxl_rch_enable_rcec(struct pci_dev *dev) { }
>>>> +static inline void cxl_enable_rcec(struct pci_dev *dev) { }
>>>>  static inline void cxl_rch_handle_error(struct pci_dev *dev,
>>>>  					struct aer_err_info *info) { }
>>>>  #endif
>>>> @@ -1494,7 +1504,7 @@ static int aer_probe(struct pcie_device *dev)
>>>>  		return status;
>>>>  	}
>>>>  
>>>> -	cxl_rch_enable_rcec(port);
>>>> +	cxl_enable_rcec(port);
>>>>  	aer_enable_rootport(rpc);
>>>>  	pci_info(port, "enabled with IRQ %d\n", dev->irq);
>>>>  	return 0;
>>