Re: [PATCH v3 4/6] cxl/pci: Add RCH downstream port error logging

[Terry Bowman <Terry.Bowman@amd.com>]

Hi Jonathan,

I added responses inline below.

On 4/13/23 11:50, Jonathan Cameron wrote:
> On Tue, 11 Apr 2023 13:03:00 -0500
> Terry Bowman <terry.bowman@amd.com> wrote:
> 
>> RCH downstream port error logging is missing in the current CXL driver. The
>> missing AER and RAS error logging is needed for communicating driver error
>> details to userspace. Update the driver to include PCIe AER and CXL RAS
>> error logging.
>>
>> Add RCH downstream port error handling into the existing RCiEP handler.
>> The downstream port error handler is added to the RCiEP error handler
>> because the downstream port is implemented in a RCRB, is not PCI
>> enumerable, and as a result is not directly accessible to the PCI AER
>> root port driver. The AER root port driver calls the RCiEP handler for
>> handling RCD errors and RCH downstream port protocol errors.
>>
>> Update mem.c to include RAS and AER setup. This includes AER and RAS
>> capability discovery and mapping for later use in the error handler.
>>
>> Disable RCH downstream port's root port cmd interrupts.[1]
>>
>> Update existing RCiEP correctable and uncorrectable handlers to also call
>> the RCH handler. The RCH handler will read the RCH AER registers, check for
>> error severity, and if an error exists will log using an existing kernel
>> AER trace routine. The RCH handler will also log downstream port RAS errors
>> if they exist.
>>
>> [1] CXL 3.0 Spec, 12.2.1.1 - RCH Downstream Port Detected Errors
>>
>> Co-developed-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Robert Richter <rrichter@amd.com>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Some minor stuff inline. Looks fine to me otherwise.
> 
> I do find it a little confusing how often we go into an RCD or RCH specific
> function then drop out directly for 2.0+ case, but you do seem to be consistent
> with existing code so fair enough.
> 
> Jonathan
> 

This was to simplify the code from the caller(s) perspective while also trying to 
generalize the logic. 

>> ---
>>  drivers/cxl/core/pci.c  | 126 ++++++++++++++++++++++++++++++++++++----
>>  drivers/cxl/core/regs.c |   1 +
>>  drivers/cxl/cxl.h       |  13 +++++
>>  drivers/cxl/mem.c       |  73 +++++++++++++++++++++++
>>  4 files changed, 201 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 523d5b9fd7fc..d435ed2ff8b6 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
> 
> 
>> +/*
>> + * Copy the AER capability registers to a buffer. This is necessary
>> + * because RCRB AER capability is MMIO mapped. Clear the status
>> + * after copying.
>> + *
>> + * @aer_base: base address of AER capability block in RCRB
>> + * @aer_regs: destination for copying AER capability
>> + */
>> +static bool cxl_rch_get_aer_info(void __iomem *aer_base,
>> +				 struct aer_capability_regs *aer_regs)
>> +{
>> +	int read_cnt = PCI_AER_CAPABILITY_LENGTH / sizeof(u32);
>> +	u32 *aer_regs_buf = (u32 *)aer_regs;
>> +	int n;
>> +
>> +	if (!aer_base)
>> +		return false;
>> +
>> +	for (n = 0; n < read_cnt; n++)
>> +		aer_regs_buf[n] = readl(aer_base + n * sizeof(u32));
> 
> Maybe add a comment here on why memcpy_fromio() doesn't work for us.
> I'm assuming we need these to definitely be 32bit reads.
> Otherwise someone will 'optimize' it in future.
> 

Correct, this was to enforce 32-bit accesses. I will add a comment.

>> +
>> +	writel(aer_regs->uncor_status, aer_base + PCI_ERR_UNCOR_STATUS);
>> +	writel(aer_regs->cor_status, aer_base + PCI_ERR_COR_STATUS);
>> +
>> +	return true;
>> +}
> =
>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
>> index bde1fffab09e..dfa6fcfc428a 100644
>> --- a/drivers/cxl/core/regs.c
>> +++ b/drivers/cxl/core/regs.c
>> @@ -198,6 +198,7 @@ void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
>>  
>>  	return ret_val;
>>  }
>> +EXPORT_SYMBOL_NS_GPL(devm_cxl_iomap_block, CXL);
>>  
>>  int cxl_map_component_regs(struct device *dev, struct cxl_component_regs *regs,
>>  			   struct cxl_register_map *map, unsigned long map_mask)
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index df64c402e6e6..dae3f141ffcb 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -66,6 +66,8 @@
>>  #define CXL_DECODER_MIN_GRANULARITY 256
>>  #define CXL_DECODER_MAX_ENCODED_IG 6
>>  
>> +#define PCI_AER_CAPABILITY_LENGTH 56
> 
> Odd place to find a PCI specific define. Also a spec reference is
> always good for these.  What's the the length of? PCI r6.0 has
> cap going up to address 0x5c  so length 0x60.  This seems to be igoring
> the header log register.
>

This was to avoid including the TLP log at 0x38+.

I can use sizeof(struct aer_capability_regs) or sizeof(*aer_regs) instead. 
It's the same 38h(56) and will allow me to remove this #define in the 
patchset revision.
 
>> +
>>  static inline int cxl_hdm_decoder_count(u32 cap_hdr)
>>  {
>>  	int val = FIELD_GET(CXL_HDM_DECODER_COUNT_MASK, cap_hdr);
>> @@ -209,6 +211,15 @@ struct cxl_regs {
>>  	struct_group_tagged(cxl_device_regs, device_regs,
>>  		void __iomem *status, *mbox, *memdev;
>>  	);
>> +
>> +	/*
>> +	 * Pointer to RCH cxl_dport AER. (only for RCH/RCD mode)
>> +	 * @dport_aer: CXL 2.0 12.2.11 RCH Downstream Port-detected Errors
> 
> As with other cases, I'd like full comments, so something for @aer as well.
> 
>> +	 */
>> +	struct_group_tagged(cxl_rch_regs, rch_regs,
>> +		void __iomem *aer;
>> +		void __iomem *dport_ras;
>> +	);
>>  };
>>  
>>  struct cxl_reg_map {
>> @@ -249,6 +260,8 @@ struct cxl_register_map {
>>  	};
>>  };
>>  
>> +void __iomem *devm_cxl_iomap_block(struct device *dev, resource_size_t addr,
>> +				   resource_size_t length);
>>  void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>>  			      struct cxl_component_reg_map *map);
>>  void cxl_probe_device_regs(struct device *dev, void __iomem *base,
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 014295ab6bc6..dd5ae0a4560c 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -4,6 +4,7 @@
>>  #include <linux/device.h>
>>  #include <linux/module.h>
>>  #include <linux/pci.h>
>> +#include <linux/aer.h>
>>  
>>  #include "cxlmem.h"
>>  #include "cxlpci.h"
>> @@ -45,6 +46,71 @@ static int cxl_mem_dpa_show(struct seq_file *file, void *data)
>>  	return 0;
>>  }
>>  
>> +static void rch_disable_root_ints(void __iomem *aer_base)
>> +{
>> +	u32 aer_cmd_mask, aer_cmd;
>> +
>> +	/*
>> +	 * Disable RCH root port command interrupts.
>> +	 * CXL3.0 12.2.1.1 - RCH Downstream Port-detected Errors
>> +	 */
>> +	aer_cmd_mask = (PCI_ERR_ROOT_CMD_COR_EN |
>> +			PCI_ERR_ROOT_CMD_NONFATAL_EN |
>> +			PCI_ERR_ROOT_CMD_FATAL_EN);
>> +	aer_cmd = readl(aer_base + PCI_ERR_ROOT_COMMAND);
>> +	aer_cmd &= ~aer_cmd_mask;
>> +	writel(aer_cmd, aer_base + PCI_ERR_ROOT_COMMAND);
> 
> Should we be touching these if firmware hasn't granted control to
> the OS?  Description in the spec refers to 'software'. Is that
> the kernel? No idea.  I guess this is safe even if it has already
> been done. Perhaps a comment to say it should already be in this state?
> 
> 

These need to be disabled because the RCH shouldn't behave as a root 
port/RCEC generating interrupts as a result of correctable, fatal, or non-fatal
AER errors. I added this per the CXL3.0 spec but, as you mentioned, isn't 
likely necessary because they are disabled by default per PCI6.0.[1][2]

This would be the case for OS/native and HW/FW error reporting.

I'll add a comment stating it is already in this state. 

[1] CXL3.0 - 12.2.1.1 RCH Downstream Port-detected Errors
[2] PCI 6.0 - 7.8.4.9 Root Error Command Register (Offset 2Ch)

>> +}
>> +
>> +static int cxl_rch_map_ras(struct cxl_dev_state *cxlds,
>> +			   struct cxl_dport *parent_dport)
>> +{
>> +	struct device *dev = parent_dport->dport;
>> +	resource_size_t aer_phys, ras_phys;
>> +	void __iomem *aer, *dport_ras;
>> +
>> +	if (!parent_dport->rch)
>> +		return 0;
>> +
>> +	if (!parent_dport->aer_cap || !parent_dport->ras_cap ||
>> +	    parent_dport->component_reg_phys == CXL_RESOURCE_NONE)
>> +		return -ENODEV;
>> +
>> +	aer_phys = parent_dport->aer_cap + parent_dport->rcrb;
>> +	aer = devm_cxl_iomap_block(dev, aer_phys,
>> +				   PCI_AER_CAPABILITY_LENGTH);
>> +
>> +	if (!aer)
>> +		return -ENOMEM;
>> +
>> +	ras_phys = parent_dport->ras_cap + parent_dport->component_reg_phys;
>> +	dport_ras = devm_cxl_iomap_block(dev, ras_phys,
>> +					 CXL_RAS_CAPABILITY_LENGTH);
>> +
>> +	if (!dport_ras)
>> +		return -ENOMEM;
>> +
>> +	cxlds->regs.aer = aer;
>> +	cxlds->regs.dport_ras = dport_ras;
>> +
>> +	return 0;
>> +}
>> +
>> +static int cxl_setup_ras(struct cxl_dev_state *cxlds,
>> +			 struct cxl_dport *parent_dport)
>> +{
>> +	int rc;
>> +
>> +	rc = cxl_rch_map_ras(cxlds, parent_dport);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (cxlds->rcd)
>> +		rch_disable_root_ints(cxlds->regs.aer);
>> +
>> +	return rc;
>> +}
>> +
>>  static void cxl_setup_rcrb(struct cxl_dev_state *cxlds,
>>  			   struct cxl_dport *parent_dport)
>>  {
>> @@ -91,6 +157,13 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>>  
>>  	cxl_setup_rcrb(cxlds, parent_dport);
>>  
>> +	rc = cxl_setup_ras(cxlds, parent_dport);
>> +	/* Continue with RAS setup errors */
>> +	if (rc)
>> +		dev_warn(&cxlmd->dev, "CXL RAS setup failed: %d\n", rc);
>> +	else
>> +		dev_info(&cxlmd->dev, "CXL error handling enabled\n");
> 
> This feels a little noisy as something to add given we didn't shout about it for
> non RCD cases (I think).  Maybe a dev_dbg()?
> 

Ok.

Regards,
Terry

>> +
>>  	endpoint = devm_cxl_add_port(host, &cxlmd->dev, cxlds->component_reg_phys,
>>  				     parent_dport);
>>  	if (IS_ERR(endpoint))
>