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

["Cheatham, Benjamin" <benjamin.cheatham@amd.com>]

On 12/22/2025 7:47 AM, Jonathan Cameron wrote:
> 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.

Sure, I'll update it.

> 
>> 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.

I'll add some.

> 
> 
>>
>> 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.

Sure

> 
>> +}
>> +
>> +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.

Good point, I'll remove it.

> 
>> +		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.

Good point, I'll expand it.> 
> 
>> +#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!

I agree. So much for trying to keep the names "short" XD.

Thanks,
Ben
> 
> 
>