Re: [PATCH 13/17] cxl/core: Add cache id verification

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Tue, 11 Nov 2025 15:40:28 -0600
Ben Cheatham <Benjamin.Cheatham@amd.com> wrote:

> The CXL cache id capability (CXL 3.2 8.2.4.28/29) is an optional
This should probably be more specific given there are several
capability structures and none have that name.

> capability that allows for multiple CXL.cache devices to be enabled
> under a single virtual hierarchy (VH). The cache id capability is
> required for having multiple CXL.cache devices in the same VH.
> 
> It's possible for the platform to enable and set up the cache id for a
> CXL.cache device. Add code to cxl_cache to check whether the device's
> cache id is programmed and correct. If it is programmed, allocate the
> cache id to prevent reuse.
> 
> Checking the correctness of the id requires knowing if the endpoint
> device is a type 2 device using HDM-D flows. Add a requirement for type
> 2 driver to specify if the device is using HDM-D flows before calling
> devm_cxl_add_cachedev().

Probably support this with some spec references.


> 
> Programming of cache ids will come in a later commit.
> 
> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> ---
>  drivers/cxl/core/cachedev.c |  20 ++++
>  drivers/cxl/core/pci.c      |   4 +-
>  drivers/cxl/core/port.c     | 229 ++++++++++++++++++++++++++++++++++++
>  drivers/cxl/core/regs.c     |  20 ++++
>  drivers/cxl/cxl.h           |  44 +++++++
>  drivers/cxl/cxlcache.h      |   3 +-
>  drivers/cxl/port.c          |  28 ++++-
>  7 files changed, 344 insertions(+), 4 deletions(-)

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 70666a059d1f..1504631ae620 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -750,6 +750,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>  		dev->parent = uport_dev;
>  
>  	ida_init(&port->decoder_ida);
> +	ida_init(&port->cache_ida);
>  	port->hdm_end = -1;
>  	port->commit_end = -1;
>  	xa_init(&port->dports);
> @@ -2414,6 +2415,234 @@ int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_decoder_autoremove, "CXL");
>  
> +static bool cache_decoder_committed(struct cxl_dport *dport)
> +{
> +	u32 cap, stat;
> +
> +	cap = readl(dport->regs.cidd + CXL_CACHE_IDD_CAP_OFFSET);
> +	if (!(cap & CXL_CACHE_IDD_CAP_COMMIT_REQUIRED))
> +		return true;
> +
> +	stat = readl(dport->regs.cidd + CXL_CACHE_IDD_STAT_OFFSET);
> +	return (stat & CXL_CACHE_IDD_STAT_COMMITTED);

Brackets not adding much so I'd drop them.

> +}
> +
> +static bool cache_decoder_valid(struct cxl_dport *dport, int id, bool endpoint)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dport->dport_dev);
> +	bool flit_256b = cxl_pci_flit_256(pdev);
> +	u32 ctrl;
> +
> +	if (id && !flit_256b)
> +		return false;
> +
> +	ctrl = readl(dport->regs.cidd + CXL_CACHE_IDD_CTRL_OFFSET);
> +	if ((endpoint || !flit_256b) &&
> +	    !(ctrl & CXL_CACHE_IDD_CTRL_ASGN_ID))
> +		return false;
> +	else if (!(ctrl & CXL_CACHE_IDD_CTRL_FWD_ID))

returned so no point in else.

> +		return false;
> +
> +	return true;
> +}

> +int cxl_endpoint_map_cache_id_regs(struct cxl_port *port)
> +{
> +	struct cxl_dport *parent_dport = port->parent_dport;
> +	int rc;
> +
> +	if (!is_cxl_cachedev(port->uport_dev))
> +		return -EINVAL;
> +
> +	port = parent_port_of(port);
> +	while (port) {
> +		if (!port->reg_map.component_map.cidrt.valid)
> +			return -ENXIO;
> +
> +		scoped_guard(device, &port->dev) {
> +			if (!port->regs.cidrt) {
> +				rc = cxl_map_component_regs(
> +					&port->reg_map, &port->regs,
> +					BIT(CXL_CM_CAP_CAP_ID_CIDRT));
> +				if (rc)
> +					return rc;
> +			}
> +		}
> +
> +		/*
> +		 * Parent dports of host bridges are cxl root (ACPI0017) dports
> +		 * and don't have cache id decoders.
> +		 */
> +		if (is_cxl_root(parent_dport->port))
> +			break;
> +
> +		if (!parent_dport->reg_map.component_map.cidd.valid)
> +			return -ENXIO;
> +
> +		scoped_guard(device, &parent_dport->port->dev) {
> +			if (!parent_dport->regs.cidd) {
> +				rc = cxl_map_component_regs(
> +					&parent_dport->reg_map,
> +					&parent_dport->regs.component,
> +					BIT(CXL_CM_CAP_CAP_ID_CIDD));
> +				if (rc)
> +					return rc;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_endpoint_map_cache_id_regs, "CXL");
> +
> +/**
> + * cxl_endpoint_allocate_cache_id - Allocate a cache id @id on the endpoint's
> + * host bridge.
> + * @endpoint: Endpoint port representing a CXL.cache device
> + * @id: Cache id to attempt to allocate
> + *
> + * Returns rc < 0 if id allocation fails. Returns allocated id otherwise.
> + */
> +int cxl_endpoint_allocate_cache_id(struct cxl_port *endpoint, int id)
> +{
> +	struct cxl_cachedev *cxlcd;
> +	struct cxl_port *hb;
> +	int nr_hdmd;
> +	u32 cap;
> +
> +	if (!is_cxl_cachedev(endpoint->uport_dev) || id < 0)
> +		return -EINVAL;
> +	cxlcd = to_cxl_cachedev(endpoint->uport_dev);
> +
> +	hb = parent_port_of(endpoint);
> +	while (!is_cxl_host_bridge(&hb->dev))
> +		hb = parent_port_of(hb);
> +
> +	if (cxl_cachedev_is_type2(cxlcd) && cxlcd->cxlds->hdmd) {
> +		cap = readl(hb->regs.cidrt + CXL_CACHE_IDRT_CAP_OFFSET);
> +		nr_hdmd = FIELD_GET(CXL_CACHE_IDRT_CAP_TYPE2_CNT_MASK, cap);
> +
> +		guard(device)(&hb->dev);
> +		if (hb->nr_hdmd + 1 >= nr_hdmd)
> +			return -EINVAL;
> +
> +		hb->nr_hdmd++;
> +	}
> +
> +	return ida_alloc_range(&hb->cache_ida, id, id, GFP_KERNEL);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_endpoint_allocate_cache_id, "CXL");
> +
> +void cxl_endpoint_free_cache_id(struct cxl_port *endpoint, int id)
> +{
> +	struct cxl_cachedev *cxlcd;
> +	struct cxl_port *hb;
> +
> +	if (!is_cxl_cachedev(endpoint->uport_dev))
> +		return;
> +	cxlcd = to_cxl_cachedev(endpoint->uport_dev);
> +
> +	hb = endpoint;
> +	while (!is_cxl_host_bridge(&hb->dev))
> +		hb = parent_port_of(hb);
> +
> +	if (cxl_cachedev_is_type2(cxlcd) && cxlcd->cxlds->hdmd) {
> +		guard(device)(&hb->dev);
> +		hb->nr_hdmd--;
> +	}
> +
> +	ida_free(&hb->cache_ida, id);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_endpoint_free_cache_id, "CXL");

> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index de29ffc3d74f..f4dc912d67ed 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -42,6 +42,8 @@ extern const struct nvdimm_security_ops *cxl_security_ops;
>  #define   CXL_CM_CAP_CAP_ID_RAS 0x2
>  #define   CXL_CM_CAP_CAP_ID_HDM 0x5
>  #define   CXL_CM_CAP_CAP_ID_SNOOP 0x8
> +#define   CXL_CM_CAP_CAP_ID_CIDRT 0xD
> +#define   CXL_CM_CAP_CAP_ID_CIDD 0xE
>  #define   CXL_CM_CAP_CAP_HDM_VERSION 1
>  
>  /* HDM decoders CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure */
> @@ -159,6 +161,30 @@ static inline int ways_to_eiw(unsigned int ways, u8 *eiw)
>  #define CXL_SNOOP_FILTER_SIZE_OFFSET 0x4
>  #define CXL_SNOOP_CAPABILITY_LENGTH 0x8
>  
> +/* CXL 3.2 8.2.4.28 CXL Cache ID Route Table Capability Structure */
> +#define CXL_CACHE_IDRT_CAP_OFFSET 0x0
> +#define   CXL_CACHE_IDRT_CAP_CNT_MASK GENMASK(4, 0)
> +#define   CXL_CACHE_IDRT_CAP_TYPE2_CNT_MASK GENMASK(11, 8)

As below. the -D bit should be in this name I think.

> +#define   CXL_CACHE_IDRT_CAP_COMMIT_REQUIRED BIT(16)
> +#define CXL_CACHE_IDRT_STAT_OFFSET 0x8
> +#define   CXL_CACHE_IDRT_STAT_COMMITTED BIT(0)
> +#define CXL_CACHE_IDRT_TARGETN_OFFSET(n) (0x10 + (2 * (n)))
> +#define   CXL_CACHE_IDRT_TARGETN_VALID BIT(0)
> +#define   CXL_CACHE_IDRT_TARGETN_PORTN GENMASK(15, 8)
> +
> +/* CXL 3.2 8.2.4.29 CXL Cache ID Decoder Capability Structure */
> +#define CXL_CACHE_IDD_CAP_OFFSET 0x0
> +#define   CXL_CACHE_IDD_CAP_COMMIT_REQUIRED BIT(0)
> +#define CXL_CACHE_IDD_CTRL_OFFSET 0x4
> +#define   CXL_CACHE_IDD_CTRL_FWD_ID BIT(0)
> +#define   CXL_CACHE_IDD_CTRL_ASGN_ID BIT(1)
> +#define   CXL_CACHE_IDD_CTRL_TYPE2 BIT(2)

Need to include the HDM-D bit of this name or it's very confusing as
nothing stops HDM-DB devices being type 2 ones.


> +#define   CXL_CACHE_IDD_CTRL_TYPE2_ID_MASK GENMASK(11, 8)
As above, the HDM-D part of this matters. Obviously it's a long
name but I'm not sure we can avoid that as it's a very specific
thing!