Re: [PATCH v3 1/6] cxl/pci: Add RCH downstream port AER and RAS register discovery

[Robert Richter <rrichter@amd.com>]

Hi Dan,

see comment to your patch below.

On 17.04.23 16:00:47, Dan Williams wrote:
> Terry Bowman wrote:

> > +	/*
> > +	 * The component registers for an RCD might come from the
> > +	 * host-bridge RCRB if they are not already mapped via the
> > +	 * typical register locator mechanism.
> > +	 */
> > +	if (cxlds->component_reg_phys == CXL_RESOURCE_NONE)
> > +		cxlds->component_reg_phys = cxl_rcrb_to_component(
> > +			&cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
> > +
> > +	parent_dport->aer_cap = cxl_rcrb_to_aer(parent_dport->dport,
> > +						parent_dport->rcrb);
> 
> Hmm, how about just retrieve this as part of cxl_rcrb_to_component()
> (renamed to cxl_probe_rcrb()), and make an rch dport its own distinct
> object? Otherwise it feels odd to be retrieving downstream port
> properties this late at upstream port component register detection time.
> It also feels awkward to keep adding more RCH dport specific details to
> the common 'struct cxl_dport'. So, I'm thinking something like the
> following (compiled and cxl_test regression passed):
> 
> -- >8 --
> From 18fbc72f98655d10301c7a35f614b6152f46c44b Mon Sep 17 00:00:00 2001
> From: Dan Williams <dan.j.williams@intel.com>
> Date: Mon, 17 Apr 2023 15:45:50 -0700
> Subject: [PATCH] cxl/rch: Prepare for caching the MMIO mapped PCIe AER
>  capability
> 
> Prepare cxl_probe_rcrb() for retrieving more than just the component
> register block. The RCH AER handling code wants to get back to the AER
> capability that happens to be MMIO mapped rather then configuration
> cycles.
> 
> Move rcrb specific dport data, like the RCRB base and the AER capability
> offset, into its own data structure ('struct cxl_rcrb_info') for
> cxl_probe_rcrb() to fill.  Introduce 'struct cxl_rch_dport' to wrap a
> 'struct cxl_dport' with a 'struct cxl_rcrb_info' attribute.
> 
> This centralizes all RCRB scanning in one routine.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/acpi.c            | 16 ++++++++--------
>  drivers/cxl/core/port.c       | 33 +++++++++++++++++++++------------
>  drivers/cxl/core/regs.c       | 12 ++++++++----
>  drivers/cxl/cxl.h             | 21 +++++++++++++++------
>  drivers/cxl/mem.c             | 15 ++++++++++-----
>  tools/testing/cxl/Kbuild      |  2 +-
>  tools/testing/cxl/test/cxl.c  | 10 ++++++----
>  tools/testing/cxl/test/mock.c | 12 ++++++------
>  tools/testing/cxl/test/mock.h |  7 ++++---
>  9 files changed, 79 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 4e66483f1fd3..2647eb04fcdb 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -375,7 +375,7 @@ static int add_host_bridge_uport(struct device *match, void *arg)
>  struct cxl_chbs_context {
>  	struct device *dev;
>  	unsigned long long uid;
> -	resource_size_t rcrb;
> +	struct cxl_rcrb_info rcrb;
>  	resource_size_t chbcr;
>  	u32 cxl_version;
>  };
> @@ -395,7 +395,7 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
>  		return 0;
>  
>  	ctx->cxl_version = chbs->cxl_version;
> -	ctx->rcrb = CXL_RESOURCE_NONE;
> +	ctx->rcrb.base = CXL_RESOURCE_NONE;
>  	ctx->chbcr = CXL_RESOURCE_NONE;
>  
>  	if (!chbs->base)
> @@ -409,9 +409,8 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg,
>  	if (chbs->length != CXL_RCRB_SIZE)
>  		return 0;
>  
> -	ctx->rcrb = chbs->base;
> -	ctx->chbcr = cxl_rcrb_to_component(ctx->dev, chbs->base,
> -					   CXL_RCRB_DOWNSTREAM);
> +	ctx->chbcr = cxl_probe_rcrb(ctx->dev, chbs->base, &ctx->rcrb,
> +				    CXL_RCRB_DOWNSTREAM);

Let's just extract the rcrb base here and do the probe later in
__devm_cxl_add_dport(). Which means chbcr will be extracted there and
we completely remove the cxl_rcrb_to_component() here. The code here
becomes much simpler and the ACPI table parser no longer contains mmio
mapping calls etc.

>  
>  	return 0;
>  }
> @@ -451,8 +450,9 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  		return 0;
>  	}
>  
> -	if (ctx.rcrb != CXL_RESOURCE_NONE)
> -		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.rcrb);
> +	if (ctx.rcrb.base != CXL_RESOURCE_NONE)
> +		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid,
> +			&ctx.rcrb.base);
>  
>  	if (ctx.chbcr == CXL_RESOURCE_NONE) {
>  		dev_warn(match, "CHBCR invalid for Host Bridge (UID %lld)\n",
> @@ -466,7 +466,7 @@ static int add_host_bridge_dport(struct device *match, void *arg)
>  	bridge = pci_root->bus->bridge;
>  	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
>  		dport = devm_cxl_add_rch_dport(root_port, bridge, uid,
> -					       ctx.chbcr, ctx.rcrb);
> +					       ctx.chbcr, &ctx.rcrb);
>  	else
>  		dport = devm_cxl_add_dport(root_port, bridge, uid,
>  					   ctx.chbcr);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 4003f445320c..d194f48259ff 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -920,7 +920,7 @@ static void cxl_dport_unlink(void *data)
>  static struct cxl_dport *
>  __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  		     int port_id, resource_size_t component_reg_phys,
> -		     resource_size_t rcrb)
> +		     struct cxl_rcrb_info *ri)
>  {
>  	char link_name[CXL_TARGET_STRLEN];
>  	struct cxl_dport *dport;
> @@ -942,17 +942,26 @@ __devm_cxl_add_dport(struct cxl_port *port, struct device *dport_dev,
>  	    CXL_TARGET_STRLEN)
>  		return ERR_PTR(-EINVAL);
>  
> -	dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> -	if (!dport)
> -		return ERR_PTR(-ENOMEM);
> +	if (ri && ri->base != CXL_RESOURCE_NONE) {
> +		struct cxl_rch_dport *rdport;
> +
> +		rdport = devm_kzalloc(host, sizeof(*rdport), GFP_KERNEL);
> +		if (!rdport)
> +			return ERR_PTR(-ENOMEM);
> +
> +		rdport->rcrb.base = ri->base;
> +		dport = &rdport->dport;
> +		dport->rch = true;
> +	} else {
> +		dport = devm_kzalloc(host, sizeof(*dport), GFP_KERNEL);
> +		if (!dport)
> +			return ERR_PTR(-ENOMEM);

I think we can simlify the allocation part if we just move the struct
into 'struct cxl_dport', see below.

> +	}
>  
>  	dport->dport = dport_dev;
>  	dport->port_id = port_id;
>  	dport->component_reg_phys = component_reg_phys;
>  	dport->port = port;
> -	if (rcrb != CXL_RESOURCE_NONE)
> -		dport->rch = true;
> -	dport->rcrb = rcrb;
>  
>  	cond_cxl_root_lock(port);
>  	rc = add_dport(port, dport);
> @@ -994,7 +1003,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>  	struct cxl_dport *dport;
>  
>  	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> -				     component_reg_phys, CXL_RESOURCE_NONE);
> +				     component_reg_phys, NULL);
>  	if (IS_ERR(dport)) {
>  		dev_dbg(dport_dev, "failed to add dport to %s: %ld\n",
>  			dev_name(&port->dev), PTR_ERR(dport));
> @@ -1013,24 +1022,24 @@ EXPORT_SYMBOL_NS_GPL(devm_cxl_add_dport, CXL);
>   * @dport_dev: firmware or PCI device representing the dport
>   * @port_id: identifier for this dport in a decoder's target list
>   * @component_reg_phys: optional location of CXL component registers
> - * @rcrb: mandatory location of a Root Complex Register Block
> + * @ri: mandatory data about the Root Complex Register Block layout
>   *
>   * See CXL 3.0 9.11.8 CXL Devices Attached to an RCH
>   */
>  struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>  					 struct device *dport_dev, int port_id,
>  					 resource_size_t component_reg_phys,
> -					 resource_size_t rcrb)
> +					 struct cxl_rcrb_info *ri)
>  {
>  	struct cxl_dport *dport;
>  
> -	if (rcrb == CXL_RESOURCE_NONE) {
> +	if (!ri || ri->base == CXL_RESOURCE_NONE) {
>  		dev_dbg(&port->dev, "failed to add RCH dport, missing RCRB\n");
>  		return ERR_PTR(-EINVAL);
>  	}
>  
>  	dport = __devm_cxl_add_dport(port, dport_dev, port_id,
> -				     component_reg_phys, rcrb);
> +				     component_reg_phys, ri);
>  	if (IS_ERR(dport)) {
>  		dev_dbg(dport_dev, "failed to add RCH dport to %s: %ld\n",
>  			dev_name(&port->dev), PTR_ERR(dport));
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index 52d1dbeda527..b1c0db898a50 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -332,9 +332,8 @@ int cxl_find_regblock(struct pci_dev *pdev, enum cxl_regloc_type type,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_find_regblock, CXL);
>  
> -resource_size_t cxl_rcrb_to_component(struct device *dev,
> -				      resource_size_t rcrb,
> -				      enum cxl_rcrb which)
> +resource_size_t cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> +			       struct cxl_rcrb_info *ri, enum cxl_rcrb which)
>  {
>  	resource_size_t component_reg_phys;
>  	void __iomem *addr;
> @@ -344,6 +343,8 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>  
>  	if (which == CXL_RCRB_UPSTREAM)
>  		rcrb += SZ_4K;
> +	else
> +		ri->base = rcrb;

For upstream ports nothing is written to ri, allow NULL pointer for ri
then but check for NULL here.

>  
>  	/*
>  	 * RCRB's BAR[0..1] point to component block containing CXL
> @@ -364,6 +365,9 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>  	cmd = readw(addr + PCI_COMMAND);
>  	bar0 = readl(addr + PCI_BASE_ADDRESS_0);
>  	bar1 = readl(addr + PCI_BASE_ADDRESS_1);
> +
> +	/* TODO: retrieve rcrb->aer_cap here */
> +

Yes, very good. The aer cap of the RCRB would be very early available
then and independent of of other drivers than cxl_acpi, esp. the pci
subsystem.

>  	iounmap(addr);
>  	release_mem_region(rcrb, SZ_4K);
>  
> @@ -395,4 +399,4 @@ resource_size_t cxl_rcrb_to_component(struct device *dev,
>  
>  	return component_reg_phys;
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_rcrb_to_component, CXL);
> +EXPORT_SYMBOL_NS_GPL(cxl_probe_rcrb, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 1503ccec9a84..b0807f54e9fd 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -267,9 +267,9 @@ enum cxl_rcrb {
>  	CXL_RCRB_DOWNSTREAM,
>  	CXL_RCRB_UPSTREAM,
>  };
> -resource_size_t cxl_rcrb_to_component(struct device *dev,
> -				      resource_size_t rcrb,
> -				      enum cxl_rcrb which);
> +struct cxl_rcrb_info;
> +resource_size_t cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> +			       struct cxl_rcrb_info *ri, enum cxl_rcrb which);
>  
>  #define CXL_RESOURCE_NONE ((resource_size_t) -1)
>  #define CXL_TARGET_STRLEN 20
> @@ -589,12 +589,12 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev)
>  	return xa_load(&port->dports, (unsigned long)dport_dev);
>  }
>  
> +

We will drop that.

>  /**
>   * struct cxl_dport - CXL downstream port
>   * @dport: PCI bridge or firmware device representing the downstream link
>   * @port_id: unique hardware identifier for dport in decoder target list
>   * @component_reg_phys: downstream port component registers
> - * @rcrb: base address for the Root Complex Register Block
>   * @rch: Indicate whether this dport was enumerated in RCH or VH mode
>   * @port: reference to cxl_port that contains this downstream port
>   */
> @@ -602,11 +602,20 @@ struct cxl_dport {
>  	struct device *dport;
>  	int port_id;
>  	resource_size_t component_reg_phys;
> -	resource_size_t rcrb;
>  	bool rch;
>  	struct cxl_port *port;
>  };
>  
> +struct cxl_rcrb_info {
> +	resource_size_t base;
> +	u16 aer_cap;
> +};
> +
> +struct cxl_rch_dport {
> +	struct cxl_dport dport;
> +	struct cxl_rcrb_info rcrb;
> +};

How about including cxl_rcrb_info directly in cxl_dport? This
simplifies dport allocation and allows direct access in cxl_dport to
the cxl_rcrb_info without a container_of() call:

struct cxl_dport {
	struct device *dport;
	struct cxl_port *port;
	int port_id;
	resource_size_t component_reg_phys;
	bool rch;
	struct cxl_rcrb_info rcrb;
};

I know you were complaining about to many RCH dport specific details,
but all this is kept in cxl_rcrb_info and the struct itself is not too
big. The flat structure allows quick access, like:

	if (dport->rch)
		component_reg_phys = cxl_probe_rcrb(..., dport->rcrb.base, ...)

> +
>  /**
>   * struct cxl_ep - track an endpoint's interest in a port
>   * @ep: device that hosts a generic CXL endpoint (expander or accelerator)
> @@ -674,7 +683,7 @@ struct cxl_dport *devm_cxl_add_dport(struct cxl_port *port,
>  struct cxl_dport *devm_cxl_add_rch_dport(struct cxl_port *port,
>  					 struct device *dport_dev, int port_id,
>  					 resource_size_t component_reg_phys,
> -					 resource_size_t rcrb);
> +					 struct cxl_rcrb_info *ri);
>  
>  struct cxl_decoder *to_cxl_decoder(struct device *dev);
>  struct cxl_root_decoder *to_cxl_root_decoder(struct device *dev);
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 097d86dd2a8e..7da6135e0b17 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -71,10 +71,15 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>  	 * host-bridge RCRB if they are not already mapped via the
>  	 * typical register locator mechanism.
>  	 */
> -	if (parent_dport->rch && cxlds->component_reg_phys == CXL_RESOURCE_NONE)
> -		component_reg_phys = cxl_rcrb_to_component(
> -			&cxlmd->dev, parent_dport->rcrb, CXL_RCRB_UPSTREAM);
> -	else
> +	if (parent_dport->rch &&
> +	    cxlds->component_reg_phys == CXL_RESOURCE_NONE) {
> +		struct cxl_rch_dport *rdport =
> +			container_of(parent_dport, typeof(*rdport), dport);
> +
> +		component_reg_phys =
> +			cxl_probe_rcrb(&cxlmd->dev, rdport->rcrb.base,
> +				       &rdport->rcrb, CXL_RCRB_UPSTREAM);

This could overwrite the dport's contents with the upstream port info.
But since we only need the info and write to it in case of a
downstream port, let's set that to null here (plus adding a check in
cxl_probe_rcrb()).

Similar to the host case (cxl_acpi driver) where the rcrb is probed
early, this code should be moved to cxl_pci. But since RAS does not
use the upstream port's RCRB it is subject of a separate patch not
part of this series.

> +	} else
>  		component_reg_phys = cxlds->component_reg_phys;
>  	endpoint = devm_cxl_add_port(host, &cxlmd->dev, component_reg_phys,
>  				     parent_dport);
> @@ -92,7 +97,7 @@ static int devm_cxl_add_endpoint(struct device *host, struct cxl_memdev *cxlmd,
>  	}
>  
>  	return 0;
> -}
> +	}

Dropping that change.

>  
>  static int cxl_mem_probe(struct device *dev)
>  {
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index fba7bec96acd..bef1bc3bd912 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -11,7 +11,7 @@ ldflags-y += --wrap=devm_cxl_enumerate_decoders
>  ldflags-y += --wrap=cxl_await_media_ready
>  ldflags-y += --wrap=cxl_hdm_decode_init
>  ldflags-y += --wrap=cxl_dvsec_rr_decode
> -ldflags-y += --wrap=cxl_rcrb_to_component
> +ldflags-y += --wrap=cxl_probe_rcrb
>  
>  DRIVERS := ../../../drivers
>  CXL_SRC := $(DRIVERS)/cxl
> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 385cdeeab22c..805c79491485 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c
> @@ -983,12 +983,14 @@ static int mock_cxl_port_enumerate_dports(struct cxl_port *port)
>  	return 0;
>  }
>  
> -resource_size_t mock_cxl_rcrb_to_component(struct device *dev,
> -					   resource_size_t rcrb,
> -					   enum cxl_rcrb which)
> +resource_size_t mock_cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> +				    struct cxl_rcrb_info *ri, enum cxl_rcrb which)
>  {
>  	dev_dbg(dev, "rcrb: %pa which: %d\n", &rcrb, which);
>  
> +	if (which == CXL_RCRB_DOWNSTREAM)
> +		ri->base = rcrb;
> +
>  	return (resource_size_t) which + 1;
>  }
>  
> @@ -1000,7 +1002,7 @@ static struct cxl_mock_ops cxl_mock_ops = {
>  	.is_mock_dev = is_mock_dev,
>  	.acpi_table_parse_cedt = mock_acpi_table_parse_cedt,
>  	.acpi_evaluate_integer = mock_acpi_evaluate_integer,
> -	.cxl_rcrb_to_component = mock_cxl_rcrb_to_component,
> +	.cxl_probe_rcrb = mock_cxl_probe_rcrb,
>  	.acpi_pci_find_root = mock_acpi_pci_find_root,
>  	.devm_cxl_port_enumerate_dports = mock_cxl_port_enumerate_dports,
>  	.devm_cxl_setup_hdm = mock_cxl_setup_hdm,
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index c4e53f22e421..148bd4f184f5 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -244,9 +244,9 @@ int __wrap_cxl_dvsec_rr_decode(struct device *dev, int dvsec,
>  }
>  EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dvsec_rr_decode, CXL);
>  
> -resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
> -					     resource_size_t rcrb,
> -					     enum cxl_rcrb which)
> +resource_size_t __wrap_cxl_probe_rcrb(struct device *dev, resource_size_t rcrb,
> +				      struct cxl_rcrb_info *ri,
> +				      enum cxl_rcrb which)
>  {
>  	int index;
>  	resource_size_t component_reg_phys;
> @@ -254,14 +254,14 @@ resource_size_t __wrap_cxl_rcrb_to_component(struct device *dev,
>  
>  	if (ops && ops->is_mock_port(dev))
>  		component_reg_phys =
> -			ops->cxl_rcrb_to_component(dev, rcrb, which);
> +			ops->cxl_probe_rcrb(dev, rcrb, ri, which);
>  	else
> -		component_reg_phys = cxl_rcrb_to_component(dev, rcrb, which);
> +		component_reg_phys = cxl_probe_rcrb(dev, rcrb, ri, which);
>  	put_cxl_mock_ops(index);
>  
>  	return component_reg_phys;
>  }
> -EXPORT_SYMBOL_NS_GPL(__wrap_cxl_rcrb_to_component, CXL);
> +EXPORT_SYMBOL_NS_GPL(__wrap_cxl_probe_rcrb, CXL);
>  
>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(ACPI);
> diff --git a/tools/testing/cxl/test/mock.h b/tools/testing/cxl/test/mock.h
> index bef8817b01f2..7ef21356d052 100644
> --- a/tools/testing/cxl/test/mock.h
> +++ b/tools/testing/cxl/test/mock.h
> @@ -15,9 +15,10 @@ struct cxl_mock_ops {
>  					     acpi_string pathname,
>  					     struct acpi_object_list *arguments,
>  					     unsigned long long *data);
> -	resource_size_t (*cxl_rcrb_to_component)(struct device *dev,
> -						 resource_size_t rcrb,
> -						 enum cxl_rcrb which);
> +	resource_size_t (*cxl_probe_rcrb)(struct device *dev,
> +					  resource_size_t rcrb,
> +					  struct cxl_rcrb_info *ri,
> +					  enum cxl_rcrb which);
>  	struct acpi_pci_root *(*acpi_pci_find_root)(acpi_handle handle);
>  	bool (*is_mock_bus)(struct pci_bus *bus);
>  	bool (*is_mock_port)(struct device *dev);
> -- 
> 2.39.2
> -- >8 --
> 
> 
> > +
> > +	parent_dport->ras_cap = cxl_component_to_ras(parent_dport->dport,
> > +						     parent_dport->component_reg_phys);
> 
> Since this is component register offset based can it not be shared with
> the VH case? I have been expecting that RCH RAS capability and VH RAS
> capability scanning would need to be unified in the cxl_port driver.

I have a modified version of your patch with following changes:

 * cxl_probe_rcrb():

   * Moved cxl_probe_rcrb() out of ACPI CEDT parse to
     __devm_cxl_add_dport() (separate patch).

   * Set cxl_rcrb_info pointer to NULL for upstream ports including
     NULL pointer check.

 * Integrated 'struct cxl_rcrb_info' in 'struct cxl_dport'.

 * Adressed comments above.

 * Dropped unrelated newlines and whitespaces.

We will include the patches in v4.

Thanks,

-Robert