[2/5] cxl/pci: Discover and cache pointer to RCD dport's PCIe AER capability

  CXL downport PCIe AER information needs to be logged during error handling.
The RCD downport/upport does not have a BDF and is not PCI enumerable. As a
result the CXL PCIe driver is not aware of the AER in 'PCI Express'
capability located in the RCRB downport/upport. Logic must be introduced to
use the downport/upport AER information.

Update the CXL driver to find the downport's PCIe AER capability and cache
a pointer for using later. First, find the RCRB to provide the
downport/upport memory region. The downport/upport are mapped as MMIO not
PCI config space. Use readl/writel/readq/writeq as required by the CXL spec
to find and operate on the AER registers.[1]

Also, add function to detect if the device is a CXL1.1 RCD device.

[1] CXL3.0, 8.2 'Memory Mapped Registers'

Signed-off-by: Terry Bowman <terry.bowman@amd.com>
---
 drivers/cxl/acpi.c      |  56 ++++++++++++++
 drivers/cxl/core/regs.c |   1 +
 drivers/cxl/cxl.h       |   9 +++
 drivers/cxl/cxlmem.h    |   2 +
 drivers/cxl/mem.c       |   2 +
 drivers/cxl/pci.c       | 158 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 228 insertions(+)
  

Terry Bowman wrote:
> CXL downport PCIe AER information needs to be logged during error handling.
> The RCD downport/upport does not have a BDF and is not PCI enumerable. As a
> result the CXL PCIe driver is not aware of the AER in 'PCI Express'
> capability located in the RCRB downport/upport. Logic must be introduced to
> use the downport/upport AER information.

Nice, I am happy to see this work get started.

> 
> Update the CXL driver to find the downport's PCIe AER capability and cache
> a pointer for using later. First, find the RCRB to provide the
> downport/upport memory region. The downport/upport are mapped as MMIO not
> PCI config space. Use readl/writel/readq/writeq as required by the CXL spec
> to find and operate on the AER registers.[1]
> 
> Also, add function to detect if the device is a CXL1.1 RCD device.
> 
> [1] CXL3.0, 8.2 'Memory Mapped Registers'
> 
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> ---
>  drivers/cxl/acpi.c      |  56 ++++++++++++++
>  drivers/cxl/core/regs.c |   1 +
>  drivers/cxl/cxl.h       |   9 +++
>  drivers/cxl/cxlmem.h    |   2 +
>  drivers/cxl/mem.c       |   2 +
>  drivers/cxl/pci.c       | 158 ++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 228 insertions(+)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index bf251a27e460..5d543c789e8d 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -232,6 +232,7 @@ struct cxl_chbs_context {
>  	struct device *dev;
>  	unsigned long long uid;
>  	struct acpi_cedt_chbs chbs;
> +	resource_size_t chbcr;
>  };
>  
>  static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
> @@ -417,6 +418,61 @@ static void remove_cxl_resources(void *data)
>  	}
>  }
>  
> +static const struct acpi_device_id cxl_host_ids[] = {
> +	{ "ACPI0016", 0 },
> +	{ "PNP0A08", 0 },
> +	{ },
> +};
> +
> +static int __cxl_get_rcrb(union acpi_subtable_headers *header, void *arg,
> +			  const unsigned long end)
> +{
> +	struct cxl_chbs_context *ctx = arg;
> +	struct acpi_cedt_chbs *chbs;
> +
> +	if (ctx->chbcr)
> +		return 0;
> +
> +	chbs = (struct acpi_cedt_chbs *)header;
> +
> +	if (ctx->uid != chbs->uid)
> +		return 0;
> +
> +	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
> +		return 0;
> +
> +	if (chbs->length != SZ_8K)
> +		return 0;
> +
> +	ctx->chbcr = chbs->base;
> +
> +	return 0;
> +}

This seems redundant with component register enumeration that was
already added in Robert's patches.

> +
> +resource_size_t cxl_get_rcrb(struct cxl_memdev *cxlmd)
> +{
> +	struct pci_host_bridge *host = NULL;
> +	struct cxl_chbs_context ctx = {0};
> +	struct cxl_dport *dport;
> +	struct cxl_port *port;
> +
> +	port = cxl_mem_find_port(cxlmd, NULL);
> +	if (!port)
> +		return 0;
> +
> +	dport = port->parent_dport;
> +	ctx.uid = dport ? dport->port_id : 0;
> +	if (!dport)
> +		return 0;
> +
> +	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, __cxl_get_rcrb, &ctx);
> +
> +	dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)ctx.chbcr);
> +
> +	return ctx.chbcr;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_get_rcrb, CXL);

While CXL to date has been tied ACPI platforms there is no requirement
that CXL be ACPI based. Given that other coherent bus specifications
that were deployed on PowerPC have now joined the CXL consortium there
is an increasing chance of CXL appearing on an Open Firmware based
platforms. Even if that does not come to pass, the discipline of clear
separation between platform code and PCI/CXL mechanisms leads to cleaner
code organization.

All that to say, no, cxl_acpi cannot export functions for other cxl
modules to depend upon. Instead it needs to publish these details in the
CXL objects that it registers.

In this case my expectation is that cxl_acpi registers a dport decorated
with the extra RCH information. Something like:

diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index f680450f0b16..b42f4759743b 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -499,12 +499,19 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
  * @port_id: unique hardware identifier for dport in decoder target list
  * @component_reg_phys: downstream port component registers
  * @port: reference to cxl_port that contains this downstream port
+ * @is_rch: enable RCH vs VH enumeration (see CXL 3.0 9.11.8)
  */
 struct cxl_dport {
        struct device *dport;
        int port_id;
        resource_size_t component_reg_phys;
        struct cxl_port *port;
+       bool is_rch;
+};
+
+struct cxl_rch_dport {
+       struct cxl_dport dport;
+       resource_size_t rcrb_phys;
 };
 
 /**

Then, when cxl_mem notices that the memdev is being produced by an
RCIEP, it can skip devm_cxl_enumerate_ports() and jump straight to
cxl_mem_find_port(). That will return this dport with the rcrb base
where cxl_mem can arrange the AER handling. Likely we will need some
notification mechanism to route Root Complex AER events to cxl_acpi,
cxl_pci, and/or cxl_mem to optionally add the CXL RAS data to the log.

> +
>  /**
>   * add_cxl_resources() - reflect CXL fixed memory windows in iomem_resource
>   * @cxl_res: A standalone resource tree where each CXL window is a sibling
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index ec178e69b18f..0d4f633e5c01 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -184,6 +184,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 pci_dev *pdev,
>  			   struct cxl_component_regs *regs,
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index ac8998b627b5..7d507ab80a78 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -204,6 +204,14 @@ struct cxl_register_map {
>  	};
>  };
>  
> +struct cxl_memdev;
> +int cxl_pci_probe_dport(struct cxl_memdev *cxlmd);
> +
> +void cxl_pci_aer_init(struct cxl_memdev *cxlmd);
> +
> +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,
> @@ -549,6 +557,7 @@ static inline bool is_cxl_root(struct cxl_port *port)
>  	return port->uport == port->dev.parent;
>  }
>  
> +resource_size_t cxl_get_rcrb(struct cxl_memdev *cxlmd);
>  bool is_cxl_port(struct device *dev);
>  struct cxl_port *to_cxl_port(struct device *dev);
>  struct pci_bus;
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 88e3a8e54b6a..079db2e15acc 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -242,6 +242,8 @@ struct cxl_dev_state {
>  	u64 next_volatile_bytes;
>  	u64 next_persistent_bytes;
>  
> +	struct cxl_register_map aer_map;
> +
>  	resource_size_t component_reg_phys;
>  	u64 serial;
>  
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 64ccf053d32c..d1e663be43c2 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -74,6 +74,8 @@ static int cxl_mem_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> +	cxl_pci_aer_init(cxlmd);
> +
>  	parent_port = cxl_mem_find_port(cxlmd, &dport);
>  	if (!parent_port) {
>  		dev_err(dev, "CXL port topology not found\n");
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index faeb5d9d7a7a..2287b5225862 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -35,6 +35,15 @@
>  	(readl((cxlds)->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET) &                  \
>  	 CXLDEV_MBOX_CTRL_DOORBELL)
>  
> +/* PCI 5.0 - 7.8.4 Advanced Error Reporting Extended Capability */
> +#define PCI_AER_CAP_SIZE 0x48
> +
> +/* CXL 3.0 - 8.2.1.3.3, Offset to DVSEC Port Status */
> +#define CXL_DVSEC_PORT_STATUS_OFF 0xE
> +
> +/* CXL 3.0 - 8.2.1.3.3 */
> +#define CXL_DVSEC_VH_SUPPORT 0x20
> +
>  /* CXL 2.0 - 8.2.8.4 */
>  #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
>  
> @@ -428,6 +437,155 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
>  	}
>  }
>  
> +static resource_size_t cxl_get_dport_ext_cap(struct cxl_memdev *cxlmd, u32 cap_id)
> +{
> +	resource_size_t rcrb, offset;
> +	void *rcrb_mapped;
> +	u32 cap_hdr;
> +
> +	rcrb = cxl_get_rcrb(cxlmd);
> +	if (!rcrb)
> +		return 0;
> +
> +	rcrb_mapped = ioremap(rcrb, SZ_4K);
> +	if (!rcrb_mapped)
> +		return 0;
> +
> +	offset = PCI_CFG_SPACE_SIZE;
> +	cap_hdr = readl(rcrb_mapped + offset);
> +
> +	while (PCI_EXT_CAP_ID(cap_hdr)) {
> +		if (PCI_EXT_CAP_ID(cap_hdr) == cap_id)
> +			break;
> +
> +		offset = PCI_EXT_CAP_NEXT(cap_hdr);
> +		if (offset == 0)
> +			break;
> +
> +		cap_hdr = readl(rcrb_mapped + offset);
> +	}
> +	iounmap((void *)rcrb_mapped);

The memdev owns the upstream port RAS capability, why is it mapping the
downstream port ras capability?

> +
> +	if (PCI_EXT_CAP_ID(cap_hdr) != cap_id)
> +		return 0;
> +
> +	pr_debug("Found capability %X @ %llX (%X)\n",
> +		 cap_id, rcrb + offset, cap_hdr);
> +
> +	return rcrb + offset;
> +}
> +
> +bool is_rcd(struct cxl_memdev *cxlmd)
> +{
> +	struct pci_dev *pdev;
> +	resource_size_t dvsec;
> +	void *dvsec_mapped;
> +	u32 dvsec_data;
> +
> +	if (!dev_is_pci(cxlmd->cxlds->dev))
> +		return false;

Just use cxlmd->dev.parent, no need to route through cxlds for this.

> +
> +	pdev = to_pci_dev(cxlmd->cxlds->dev);
> +
> +	/*
> +	 * 'CXL devices operating in this mode always set the Device/Port
> +	 * Type field in the PCI Express Capabilities register to RCiEP.'
> +	 * - CXL3.0 9.11.1 'RCD Mode'
> +	 */
> +	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
> +		return false;
> +
> +	/*
> +	 * Check if VH is enabled
> +	 * - CXL3.0 8.2.1.3.1 'DVSEC Flex Bus Port Capability'
> +	 */
> +	dvsec = cxl_get_dport_ext_cap(cxlmd, PCI_EXT_CAP_ID_DVSEC);
> +	if (!dvsec)
> +		return false;
> +
> +	dvsec_mapped = ioremap(dvsec, SZ_4K);

No ioremap error handling?

> +	dvsec_data = readl(dvsec_mapped + CXL_DVSEC_PORT_STATUS_OFF);
> +	iounmap(dvsec_mapped);
> +	if (dvsec_data & CXL_DVSEC_VH_SUPPORT)
> +		return false;

There's no such thing as a root-complex-integrated endpoint in CXL VH
mode, right? Is this not redundant?

> +
> +	return true;
> +}
> +
> +#define PCI_CAP_ID(header)	(header & 0x000000ff)
> +#define PCI_CAP_NEXT(header)	((header >> 8) & 0xff)
> +
> +static resource_size_t cxl_get_dport_cap(struct cxl_memdev *cxlmd, int cap_id)
> +{
> +	resource_size_t offset, rcrb;
> +	void *rcrb_mapped;
> +	u32 cap_hdr;
> +
> +	rcrb = cxl_get_rcrb(cxlmd);
> +	if (!rcrb)
> +		return 0;
> +
> +	rcrb_mapped = ioremap(rcrb, SZ_4K);
> +	if (!rcrb_mapped)
> +		return 0;
> +
> +	offset = readl(rcrb_mapped + PCI_CAPABILITY_LIST);
> +	cap_hdr = readl(rcrb_mapped + offset);
> +
> +	while (PCI_CAP_ID(cap_hdr)) {
> +		if (PCI_CAP_ID(cap_hdr) == cap_id)
> +			break;
> +
> +		offset = PCI_CAP_NEXT(cap_hdr);
> +		if (offset == 0)
> +			break;
> +
> +		cap_hdr = readl(rcrb_mapped + offset);
> +	}
> +	iounmap((void *)rcrb_mapped);

All of this mapping and unmapping of the RCRB needs to be centralized in
one place and owned by one driver for the downstream portion and one
driver for the upstream portion. That *feels* like cxl_acpi for the
downstream side and cxl_port for the upstream side (when it drives the
endpoint port registered by cxl_mem). It should also be protected by
request_region() to make sure multiple agents are not stepping on each
other's toes.

> +
> +	if (PCI_CAP_ID(cap_hdr) != cap_id)
> +		return 0;
> +
> +	pr_debug("Found capability %X @ %llX (%X)\n",
> +		 cap_id, rcrb + offset, cap_hdr);
> +
> +	return rcrb + offset;
> +}
> +
> +static int cxl_setup_dport_aer(struct cxl_memdev *cxlmd, resource_size_t cap_base)
> +{
> +	struct cxl_register_map *map = &cxlmd->cxlds->aer_map;
> +	struct pci_dev *pdev = to_pci_dev(&cxlmd->dev);
> +
> +	if (!cap_base)
> +		return  -ENODEV;
> +
> +	map->base = devm_cxl_iomap_block(&pdev->dev, cap_base,
> +					 PCI_CAP_EXP_RC_ENDPOINT_SIZEOF_V1);
> +	if (!map->base)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +void cxl_pci_aer_init(struct cxl_memdev *cxlmd)
> +{
> +	resource_size_t cap_base;
> +
> +	/* CXL2.0 is enumerable and will use AER attached to `struct pci_dev` */
> +	if (!is_rcd(cxlmd))
> +		return;
> +
> +	/*
> +	 * Read base address of the PCI express cap. Cache the cap's
> +	 * PCI_EXP_DEVCTL and PCI_EXP_DEVSTA for AER control and status.
> +	 */
> +	cap_base = cxl_get_dport_cap(cxlmd, PCI_CAP_ID_EXP);
> +	cxl_setup_dport_aer(cxlmd, cap_base);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_pci_aer_init, CXL);
> +
>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	struct cxl_register_map map;
> -- 
> 2.34.1
>
  

On 10/22/22 16:45, Dan Williams wrote:
> Terry Bowman wrote:
>> CXL downport PCIe AER information needs to be logged during error handling.
>> The RCD downport/upport does not have a BDF and is not PCI enumerable. As a
>> result the CXL PCIe driver is not aware of the AER in 'PCI Express'
>> capability located in the RCRB downport/upport. Logic must be introduced to
>> use the downport/upport AER information.
> 
> Nice, I am happy to see this work get started.
> 
>>
>> Update the CXL driver to find the downport's PCIe AER capability and cache
>> a pointer for using later. First, find the RCRB to provide the
>> downport/upport memory region. The downport/upport are mapped as MMIO not
>> PCI config space. Use readl/writel/readq/writeq as required by the CXL spec
>> to find and operate on the AER registers.[1]
>>
>> Also, add function to detect if the device is a CXL1.1 RCD device.
>>
>> [1] CXL3.0, 8.2 'Memory Mapped Registers'
>>
>> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
>> ---
>>  drivers/cxl/acpi.c      |  56 ++++++++++++++
>>  drivers/cxl/core/regs.c |   1 +
>>  drivers/cxl/cxl.h       |   9 +++
>>  drivers/cxl/cxlmem.h    |   2 +
>>  drivers/cxl/mem.c       |   2 +
>>  drivers/cxl/pci.c       | 158 ++++++++++++++++++++++++++++++++++++++++
>>  6 files changed, 228 insertions(+)
>>
>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
>> index bf251a27e460..5d543c789e8d 100644
>> --- a/drivers/cxl/acpi.c
>> +++ b/drivers/cxl/acpi.c
>> @@ -232,6 +232,7 @@ struct cxl_chbs_context {
>>  	struct device *dev;
>>  	unsigned long long uid;
>>  	struct acpi_cedt_chbs chbs;
>> +	resource_size_t chbcr;
>>  };
>>  
>>  static int cxl_get_chbs(union acpi_subtable_headers *header, void *arg,
>> @@ -417,6 +418,61 @@ static void remove_cxl_resources(void *data)
>>  	}
>>  }
>>  
>> +static const struct acpi_device_id cxl_host_ids[] = {
>> +	{ "ACPI0016", 0 },
>> +	{ "PNP0A08", 0 },
>> +	{ },
>> +};
>> +
>> +static int __cxl_get_rcrb(union acpi_subtable_headers *header, void *arg,
>> +			  const unsigned long end)
>> +{
>> +	struct cxl_chbs_context *ctx = arg;
>> +	struct acpi_cedt_chbs *chbs;
>> +
>> +	if (ctx->chbcr)
>> +		return 0;
>> +
>> +	chbs = (struct acpi_cedt_chbs *)header;
>> +
>> +	if (ctx->uid != chbs->uid)
>> +		return 0;
>> +
>> +	if (chbs->cxl_version != ACPI_CEDT_CHBS_VERSION_CXL11)
>> +		return 0;
>> +
>> +	if (chbs->length != SZ_8K)
>> +		return 0;
>> +
>> +	ctx->chbcr = chbs->base;
>> +
>> +	return 0;
>> +}
> 
> This seems redundant with component register enumeration that was
> already added in Robert's patches.
> 

Noted. Plese see my next response below.

>> +
>> +resource_size_t cxl_get_rcrb(struct cxl_memdev *cxlmd)
>> +{
>> +	struct pci_host_bridge *host = NULL;
>> +	struct cxl_chbs_context ctx = {0};
>> +	struct cxl_dport *dport;
>> +	struct cxl_port *port;
>> +
>> +	port = cxl_mem_find_port(cxlmd, NULL);
>> +	if (!port)
>> +		return 0;
>> +
>> +	dport = port->parent_dport;
>> +	ctx.uid = dport ? dport->port_id : 0;
>> +	if (!dport)
>> +		return 0;
>> +
>> +	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, __cxl_get_rcrb, &ctx);
>> +
>> +	dev_dbg(&host->dev, "RCRB found: 0x%08llx\n", (u64)ctx.chbcr);
>> +
>> +	return ctx.chbcr;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_get_rcrb, CXL);
> 
> While CXL to date has been tied ACPI platforms there is no requirement
> that CXL be ACPI based. Given that other coherent bus specifications
> that were deployed on PowerPC have now joined the CXL consortium there
> is an increasing chance of CXL appearing on an Open Firmware based
> platforms. Even if that does not come to pass, the discipline of clear
> separation between platform code and PCI/CXL mechanisms leads to cleaner
> code organization.
> 
> All that to say, no, cxl_acpi cannot export functions for other cxl
> modules to depend upon. Instead it needs to publish these details in the
> CXL objects that it registers.
> 

Ok. Ill rework such that ACPI functions are not exported. Adding  the recommended 
caching 'rcrb_phys' will help recfactoring out the exported function. I'll
cache the RCRB to 'rcrb_phys' during enumeration instead of calling a helper 
function for every query.

> In this case my expectation is that cxl_acpi registers a dport decorated
> with the extra RCH information. Something like:
> 
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index f680450f0b16..b42f4759743b 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -499,12 +499,19 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>   * @port_id: unique hardware identifier for dport in decoder target list
>   * @component_reg_phys: downstream port component registers
>   * @port: reference to cxl_port that contains this downstream port
> + * @is_rch: enable RCH vs VH enumeration (see CXL 3.0 9.11.8)
>   */
>  struct cxl_dport {
>         struct device *dport;
>         int port_id;
>         resource_size_t component_reg_phys;
>         struct cxl_port *port;
> +       bool is_rch;
> +};
> +
> +struct cxl_rch_dport {
> +       struct cxl_dport dport;
> +       resource_size_t rcrb_phys;
>  };
>  

The same is needed for uport as well, correct ?

>  /**
> 
> Then, when cxl_mem notices that the memdev is being produced by an
> RCIEP, it can skip devm_cxl_enumerate_ports() and jump straight to
> cxl_mem_find_port(). That will return this dport with the rcrb base
> where cxl_mem can arrange the AER handling. Likely we will need some
> notification mechanism to route Root Complex AER events to cxl_acpi,
> cxl_pci, and/or cxl_mem to optionally add the CXL RAS data to the log.
> 
Isn't the notification mechanism through the AER interrupt processing? 
I will have more related comments in patch 3/5.

>> +
>>  /**
>>   * add_cxl_resources() - reflect CXL fixed memory windows in iomem_resource
>>   * @cxl_res: A standalone resource tree where each CXL window is a sibling
>> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
>> index ec178e69b18f..0d4f633e5c01 100644
>> --- a/drivers/cxl/core/regs.c
>> +++ b/drivers/cxl/core/regs.c
>> @@ -184,6 +184,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 pci_dev *pdev,
>>  			   struct cxl_component_regs *regs,
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index ac8998b627b5..7d507ab80a78 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -204,6 +204,14 @@ struct cxl_register_map {
>>  	};
>>  };
>>  
>> +struct cxl_memdev;
>> +int cxl_pci_probe_dport(struct cxl_memdev *cxlmd);
>> +
>> +void cxl_pci_aer_init(struct cxl_memdev *cxlmd);
>> +
>> +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,
>> @@ -549,6 +557,7 @@ static inline bool is_cxl_root(struct cxl_port *port)
>>  	return port->uport == port->dev.parent;
>>  }
>>  
>> +resource_size_t cxl_get_rcrb(struct cxl_memdev *cxlmd);
>>  bool is_cxl_port(struct device *dev);
>>  struct cxl_port *to_cxl_port(struct device *dev);
>>  struct pci_bus;
>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
>> index 88e3a8e54b6a..079db2e15acc 100644
>> --- a/drivers/cxl/cxlmem.h
>> +++ b/drivers/cxl/cxlmem.h
>> @@ -242,6 +242,8 @@ struct cxl_dev_state {
>>  	u64 next_volatile_bytes;
>>  	u64 next_persistent_bytes;
>>  
>> +	struct cxl_register_map aer_map;
>> +
>>  	resource_size_t component_reg_phys;
>>  	u64 serial;
>>  
>> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>> index 64ccf053d32c..d1e663be43c2 100644
>> --- a/drivers/cxl/mem.c
>> +++ b/drivers/cxl/mem.c
>> @@ -74,6 +74,8 @@ static int cxl_mem_probe(struct device *dev)
>>  	if (rc)
>>  		return rc;
>>  
>> +	cxl_pci_aer_init(cxlmd);
>> +
>>  	parent_port = cxl_mem_find_port(cxlmd, &dport);
>>  	if (!parent_port) {
>>  		dev_err(dev, "CXL port topology not found\n");
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index faeb5d9d7a7a..2287b5225862 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -35,6 +35,15 @@
>>  	(readl((cxlds)->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET) &                  \
>>  	 CXLDEV_MBOX_CTRL_DOORBELL)
>>  
>> +/* PCI 5.0 - 7.8.4 Advanced Error Reporting Extended Capability */
>> +#define PCI_AER_CAP_SIZE 0x48
>> +
>> +/* CXL 3.0 - 8.2.1.3.3, Offset to DVSEC Port Status */
>> +#define CXL_DVSEC_PORT_STATUS_OFF 0xE
>> +
>> +/* CXL 3.0 - 8.2.1.3.3 */
>> +#define CXL_DVSEC_VH_SUPPORT 0x20
>> +
>>  /* CXL 2.0 - 8.2.8.4 */
>>  #define CXL_MAILBOX_TIMEOUT_MS (2 * HZ)
>>  
>> @@ -428,6 +437,155 @@ static void devm_cxl_pci_create_doe(struct cxl_dev_state *cxlds)
>>  	}
>>  }
>>  
>> +static resource_size_t cxl_get_dport_ext_cap(struct cxl_memdev *cxlmd, u32 cap_id)
>> +{
>> +	resource_size_t rcrb, offset;
>> +	void *rcrb_mapped;
>> +	u32 cap_hdr;
>> +
>> +	rcrb = cxl_get_rcrb(cxlmd);
>> +	if (!rcrb)
>> +		return 0;
>> +
>> +	rcrb_mapped = ioremap(rcrb, SZ_4K);
>> +	if (!rcrb_mapped)
>> +		return 0;
>> +
>> +	offset = PCI_CFG_SPACE_SIZE;
>> +	cap_hdr = readl(rcrb_mapped + offset);
>> +
>> +	while (PCI_EXT_CAP_ID(cap_hdr)) {
>> +		if (PCI_EXT_CAP_ID(cap_hdr) == cap_id)
>> +			break;
>> +
>> +		offset = PCI_EXT_CAP_NEXT(cap_hdr);
>> +		if (offset == 0)
>> +			break;
>> +
>> +		cap_hdr = readl(rcrb_mapped + offset);
>> +	}
>> +	iounmap((void *)rcrb_mapped);
> 
> The memdev owns the upstream port RAS capability, why is it mapping the
> downstream port ras capability?
> 

You're right, this needs to be changed to read the upport's extended 
capabilities. 

>> +
>> +	if (PCI_EXT_CAP_ID(cap_hdr) != cap_id)
>> +		return 0;
>> +
>> +	pr_debug("Found capability %X @ %llX (%X)\n",
>> +		 cap_id, rcrb + offset, cap_hdr);
>> +
>> +	return rcrb + offset;
>> +}
>> +
>> +bool is_rcd(struct cxl_memdev *cxlmd)
>> +{
>> +	struct pci_dev *pdev;
>> +	resource_size_t dvsec;
>> +	void *dvsec_mapped;
>> +	u32 dvsec_data;
>> +
>> +	if (!dev_is_pci(cxlmd->cxlds->dev))
>> +		return false;
> 
> Just use cxlmd->dev.parent, no need to route through cxlds for this.
> 

Ok

>> +
>> +	pdev = to_pci_dev(cxlmd->cxlds->dev);
>> +
>> +	/*
>> +	 * 'CXL devices operating in this mode always set the Device/Port
>> +	 * Type field in the PCI Express Capabilities register to RCiEP.'
>> +	 * - CXL3.0 9.11.1 'RCD Mode'
>> +	 */
>> +	if (pci_pcie_type(pdev) != PCI_EXP_TYPE_RC_END)
>> +		return false;
>> +
>> +	/*
>> +	 * Check if VH is enabled
>> +	 * - CXL3.0 8.2.1.3.1 'DVSEC Flex Bus Port Capability'
>> +	 */
>> +	dvsec = cxl_get_dport_ext_cap(cxlmd, PCI_EXT_CAP_ID_DVSEC);
>> +	if (!dvsec)
>> +		return false;
>> +
>> +	dvsec_mapped = ioremap(dvsec, SZ_4K);
> 
> No ioremap error handling?
> 

I'll add.

>> +	dvsec_data = readl(dvsec_mapped + CXL_DVSEC_PORT_STATUS_OFF);
>> +	iounmap(dvsec_mapped);
>> +	if (dvsec_data & CXL_DVSEC_VH_SUPPORT)
>> +		return false;
> 
> There's no such thing as a root-complex-integrated endpoint in CXL VH
> mode, right? Is this not redundant?
> 

Youre correct.

'CXL Root Ports may be directly connected to a CXL device that is not an eRCD, or a CXL
Switch. These Root Ports spawn a CXL Virtual Hierarchy (VH). Enumeration within a
CXL VH is described below. These CXL devices appear as a standard PCIe Endpoints with a Type 0 Header.'
-cxl3.0 9.12.2 'CXL Virtual Hierarchy'

I will remove the DVSEC check.

>> +
>> +	return true;
>> +}
>> +
>> +#define PCI_CAP_ID(header)	(header & 0x000000ff)
>> +#define PCI_CAP_NEXT(header)	((header >> 8) & 0xff)
>> +
>> +static resource_size_t cxl_get_dport_cap(struct cxl_memdev *cxlmd, int cap_id)
>> +{
>> +	resource_size_t offset, rcrb;
>> +	void *rcrb_mapped;
>> +	u32 cap_hdr;
>> +
>> +	rcrb = cxl_get_rcrb(cxlmd);
>> +	if (!rcrb)
>> +		return 0;
>> +
>> +	rcrb_mapped = ioremap(rcrb, SZ_4K);
>> +	if (!rcrb_mapped)
>> +		return 0;
>> +
>> +	offset = readl(rcrb_mapped + PCI_CAPABILITY_LIST);
>> +	cap_hdr = readl(rcrb_mapped + offset);
>> +
>> +	while (PCI_CAP_ID(cap_hdr)) {
>> +		if (PCI_CAP_ID(cap_hdr) == cap_id)
>> +			break;
>> +
>> +		offset = PCI_CAP_NEXT(cap_hdr);
>> +		if (offset == 0)
>> +			break;
>> +
>> +		cap_hdr = readl(rcrb_mapped + offset);
>> +	}
>> +	iounmap((void *)rcrb_mapped);
> 
> All of this mapping and unmapping of the RCRB needs to be centralized in
> one place and owned by one driver for the downstream portion and one
> driver for the upstream portion. That *feels* like cxl_acpi for the
> downstream side and cxl_port for the upstream side (when it drives the
> endpoint port registered by cxl_mem). It should also be protected by
> request_region() to make sure multiple agents are not stepping on each
> other's toes.
> 

Ok. I'll look into this and make the change.

Thank you for this review.

Regards,
Terry Bowman

>> +
>> +	if (PCI_CAP_ID(cap_hdr) != cap_id)
>> +		return 0;
>> +
>> +	pr_debug("Found capability %X @ %llX (%X)\n",
>> +		 cap_id, rcrb + offset, cap_hdr);
>> +
>> +	return rcrb + offset;
>> +}
>> +
>> +static int cxl_setup_dport_aer(struct cxl_memdev *cxlmd, resource_size_t cap_base)
>> +{
>> +	struct cxl_register_map *map = &cxlmd->cxlds->aer_map;
>> +	struct pci_dev *pdev = to_pci_dev(&cxlmd->dev);
>> +
>> +	if (!cap_base)
>> +		return  -ENODEV;
>> +
>> +	map->base = devm_cxl_iomap_block(&pdev->dev, cap_base,
>> +					 PCI_CAP_EXP_RC_ENDPOINT_SIZEOF_V1);
>> +	if (!map->base)
>> +		return -ENOMEM;
>> +
>> +	return 0;
>> +}
>> +
>> +void cxl_pci_aer_init(struct cxl_memdev *cxlmd)
>> +{
>> +	resource_size_t cap_base;
>> +
>> +	/* CXL2.0 is enumerable and will use AER attached to `struct pci_dev` */
>> +	if (!is_rcd(cxlmd))
>> +		return;
>> +
>> +	/*
>> +	 * Read base address of the PCI express cap. Cache the cap's
>> +	 * PCI_EXP_DEVCTL and PCI_EXP_DEVSTA for AER control and status.
>> +	 */
>> +	cap_base = cxl_get_dport_cap(cxlmd, PCI_CAP_ID_EXP);
>> +	cxl_setup_dport_aer(cxlmd, cap_base);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_aer_init, CXL);
>> +
>>  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>  {
>>  	struct cxl_register_map map;
>> -- 
>> 2.34.1
>>
> 
>
  

Terry Bowman wrote:
[..]
> > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> > index f680450f0b16..b42f4759743b 100644
> > --- a/drivers/cxl/cxl.h
> > +++ b/drivers/cxl/cxl.h
> > @@ -499,12 +499,19 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
> >   * @port_id: unique hardware identifier for dport in decoder target list
> >   * @component_reg_phys: downstream port component registers
> >   * @port: reference to cxl_port that contains this downstream port
> > + * @is_rch: enable RCH vs VH enumeration (see CXL 3.0 9.11.8)
> >   */
> >  struct cxl_dport {
> >         struct device *dport;
> >         int port_id;
> >         resource_size_t component_reg_phys;
> >         struct cxl_port *port;
> > +       bool is_rch;
> > +};
> > +
> > +struct cxl_rch_dport {
> > +       struct cxl_dport dport;
> > +       resource_size_t rcrb_phys;
> >  };
> >  
> 
> The same is needed for uport as well, correct ?

Hmm, I don't think there are any 'struct cxl_port' instances that need
an RCH flag in the Linux CXL topology model.  That was the feedback /
realization that Dave and I came to while reviewing Robert's RCH series.
In the RCH case the ACPI0016 host-bridge houses a
root-complex-integrated endpoint that is a peer of the
downstream-PCI-root-ports in the bridge. The topology ends up looking
like this:

# cxl list -BEMPTu
{
  "bus":"root0",
  "provider":"ACPI.CXL",
  "nr_dports":1,
  "dports":[
    {
      "dport":"pci0000:38",
      "id":"0x31"
    }
  ],
  "endpoints:root0":[
    {
      "endpoint":"endpoint1",
      "host":"mem0",
      "depth":1,
      "memdev":{
        "memdev":"mem0",
        "pmem_size":0,
        "ram_size":"16.00 GiB (17.18 GB)",
        "serial":"0",
        "numa_node":0,
        "host":"0000:38:00.0"
      }
    }
  ]
}

Notice that the logical ACPI0017 (CXL root) device lists the ACPI0016
device (associated with pci0000:38) as a 'struct cxl_dport'. The
endpoint is then connected to that dport. There are no intervening
'struct cxl_port' instances between the root0 and endpoint1 like there
would be in the CXL VH case.

Now, the problem is that there is no viable object that can drive access
to the upstream port component registers. It may be the case that when
the cxl_port driver attaches to an endpoint port that the endpoint port
driver maps both the upstream and downstream hardware-port registers for
the purposes of conveying RAS information.

> >  /**
> > 
> > Then, when cxl_mem notices that the memdev is being produced by an
> > RCIEP, it can skip devm_cxl_enumerate_ports() and jump straight to
> > cxl_mem_find_port(). That will return this dport with the rcrb base
> > where cxl_mem can arrange the AER handling. Likely we will need some
> > notification mechanism to route Root Complex AER events to cxl_acpi,
> > cxl_pci, and/or cxl_mem to optionally add the CXL RAS data to the log.
> > 
> Isn't the notification mechanism through the AER interrupt processing? 
> I will have more related comments in patch 3/5.

In this case I was talking about notifying the cxl_mem driver and/or the
cxl_port driver that it needs to decorate an AER event with more
object-local information.
  

On Fri, Oct 21, 2022 at 01:56:12PM -0500, Terry Bowman wrote:
> CXL downport PCIe AER information needs to be logged during error handling.
> The RCD downport/upport does not have a BDF and is not PCI enumerable. As a
> result the CXL PCIe driver is not aware of the AER in 'PCI Express'
> capability located in the RCRB downport/upport. Logic must be introduced to
> use the downport/upport AER information.

I assume "downport" is the same as "dport" in "cxl_dport" and means
"Downstream Port".  Might be nice to reduce the number of variations
if feasible.

> +static resource_size_t cxl_get_dport_cap(struct cxl_memdev *cxlmd, int cap_id)
> +{
> +	resource_size_t offset, rcrb;
> +	void *rcrb_mapped;
> +	u32 cap_hdr;
> +
> +	rcrb = cxl_get_rcrb(cxlmd);
> +	if (!rcrb)
> +		return 0;
> +
> +	rcrb_mapped = ioremap(rcrb, SZ_4K);
> +	if (!rcrb_mapped)
> +		return 0;
> +
> +	offset = readl(rcrb_mapped + PCI_CAPABILITY_LIST);
> +	cap_hdr = readl(rcrb_mapped + offset);
> +
> +	while (PCI_CAP_ID(cap_hdr)) {
> +		if (PCI_CAP_ID(cap_hdr) == cap_id)
> +			break;
> +
> +		offset = PCI_CAP_NEXT(cap_hdr);
> +		if (offset == 0)
> +			break;
> +
> +		cap_hdr = readl(rcrb_mapped + offset);
> +	}
> +	iounmap((void *)rcrb_mapped);
> +
> +	if (PCI_CAP_ID(cap_hdr) != cap_id)
> +		return 0;
> +
> +	pr_debug("Found capability %X @ %llX (%X)\n",
> +		 cap_id, rcrb + offset, cap_hdr);

Would be nice to use dev_dbg() if possible here.

Is "%X" (upper-case hex) the convention in CXL?  Most places in Linux
seem to use "%x".  Also consider "%#x" (or "%#X") so it's obvious
these are hex.

> +void cxl_pci_aer_init(struct cxl_memdev *cxlmd)
> +{
> +	resource_size_t cap_base;
> +
> +	/* CXL2.0 is enumerable and will use AER attached to `struct pci_dev` */
> +	if (!is_rcd(cxlmd))
> +		return;
> +
> +	/*
> +	 * Read base address of the PCI express cap. Cache the cap's
> +	 * PCI_EXP_DEVCTL and PCI_EXP_DEVSTA for AER control and status.
> +	 */
> +	cap_base = cxl_get_dport_cap(cxlmd, PCI_CAP_ID_EXP);
> +	cxl_setup_dport_aer(cxlmd, cap_base);

I don't see anything about PCI_EXP_DEVCTL and PCI_EXP_DEVSTA in
cxl_get_dport_cap() or cxl_setup_dport_aer().  And I don't see any
caching, except for setting map->base in cxl_setup_dport_aer().

Caching those registers, especially PCI_EXP_DEVSTA, doesn't seem like
it would make much sense anyway since bits there are set by hardware
when things happen.
  

Hi Bjorn,


On 10/27/22 09:52, Bjorn Helgaas wrote:
> On Fri, Oct 21, 2022 at 01:56:12PM -0500, Terry Bowman wrote:
>> CXL downport PCIe AER information needs to be logged during error handling.
>> The RCD downport/upport does not have a BDF and is not PCI enumerable. As a
>> result the CXL PCIe driver is not aware of the AER in 'PCI Express'
>> capability located in the RCRB downport/upport. Logic must be introduced to
>> use the downport/upport AER information.
> 
> I assume "downport" is the same as "dport" in "cxl_dport" and means
> "Downstream Port".  Might be nice to reduce the number of variations
> if feasible.
> 

Yes, I'll update the terminology throughout to use cxl_port, cxl_dport,
and cxl_uport.

>> +static resource_size_t cxl_get_dport_cap(struct cxl_memdev *cxlmd, int cap_id)
>> +{
>> +	resource_size_t offset, rcrb;
>> +	void *rcrb_mapped;
>> +	u32 cap_hdr;
>> +
>> +	rcrb = cxl_get_rcrb(cxlmd);
>> +	if (!rcrb)
>> +		return 0;
>> +
>> +	rcrb_mapped = ioremap(rcrb, SZ_4K);
>> +	if (!rcrb_mapped)
>> +		return 0;
>> +
>> +	offset = readl(rcrb_mapped + PCI_CAPABILITY_LIST);
>> +	cap_hdr = readl(rcrb_mapped + offset);
>> +
>> +	while (PCI_CAP_ID(cap_hdr)) {
>> +		if (PCI_CAP_ID(cap_hdr) == cap_id)
>> +			break;
>> +
>> +		offset = PCI_CAP_NEXT(cap_hdr);
>> +		if (offset == 0)
>> +			break;
>> +
>> +		cap_hdr = readl(rcrb_mapped + offset);
>> +	}
>> +	iounmap((void *)rcrb_mapped);
>> +
>> +	if (PCI_CAP_ID(cap_hdr) != cap_id)
>> +		return 0;
>> +
>> +	pr_debug("Found capability %X @ %llX (%X)\n",
>> +		 cap_id, rcrb + offset, cap_hdr);
> 
> Would be nice to use dev_dbg() if possible here.
> 
> Is "%X" (upper-case hex) the convention in CXL?  Most places in Linux
> seem to use "%x".  Also consider "%#x" (or "%#X") so it's obvious
> these are hex.
> 

Ok, I will make the change.

>> +void cxl_pci_aer_init(struct cxl_memdev *cxlmd)
>> +{
>> +	resource_size_t cap_base;
>> +
>> +	/* CXL2.0 is enumerable and will use AER attached to `struct pci_dev` */
>> +	if (!is_rcd(cxlmd))
>> +		return;
>> +
>> +	/*
>> +	 * Read base address of the PCI express cap. Cache the cap's
>> +	 * PCI_EXP_DEVCTL and PCI_EXP_DEVSTA for AER control and status.
>> +	 */
>> +	cap_base = cxl_get_dport_cap(cxlmd, PCI_CAP_ID_EXP);
>> +	cxl_setup_dport_aer(cxlmd, cap_base);
> 
> I don't see anything about PCI_EXP_DEVCTL and PCI_EXP_DEVSTA in
> cxl_get_dport_cap() or cxl_setup_dport_aer().  And I don't see any
> caching, except for setting map->base in cxl_setup_dport_aer().
> 
> Caching those registers, especially PCI_EXP_DEVSTA, doesn't seem like
> it would make much sense anyway since bits there are set by hardware
> when things happen.

This comment needs to be moved to later patch enabling AER.