Re: [PATCH 03/17] cxl/core: Add function for getting CXL cache info

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

On 12/17/2025 10:09 AM, Jonathan Cameron wrote:
> On Tue, 11 Nov 2025 15:40:18 -0600
> Ben Cheatham <Benjamin.Cheatham@amd.com> wrote:
> 
>> Add a function for getting common CXL.cache information. This
>> information will be stored in the struct cxl_cache_state member
>> (cstate) of struct cxl_dev_state for easy access by endpoint drivers.
>>
>> Signed-off-by: Ben Cheatham <Benjamin.Cheatham@amd.com>
> Hi Ben,
> 
> I'm only having a first read through so probably only superficial feedback
> at this point.
> 
> We have so many major series floating around I suspect this will be a cycle
> or two anyway :(
> 

Don't worry about it. I figured this won't be ready for a few cycles regardless :).

> Anyhow, comments inline.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>  drivers/cxl/core/pci.c | 53 ++++++++++++++++++++++++++++++++++++++++++
>>  drivers/cxl/cxl.h      | 13 +++++++++++
>>  drivers/cxl/cxlcache.h |  7 ++++++
>>  drivers/cxl/cxlpci.h   |  4 ++++
>>  4 files changed, 77 insertions(+)
>>  create mode 100644 drivers/cxl/cxlcache.h
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 18825e1505d6..5b1cace8fc0f 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -7,6 +7,7 @@
>>  #include <linux/pci.h>
>>  #include <linux/pci-doe.h>
>>  #include <linux/aer.h>
>> +#include <cxlcache.h>
>>  #include <cxlpci.h>
>>  #include <cxlmem.h>
>>  #include <cxl.h>
>> @@ -1258,3 +1259,55 @@ int cxl_port_get_possible_dports(struct cxl_port *port)
>>  
>>  	return ctx.count;
>>  }
>> +EXPORT_SYMBOL_NS_GPL(cxl_port_get_possible_dports, "CXL");
> 
> Stray change?
> 

Yep, I'll remove it.

>> +
>> +/**
>> + * cxl_accel_read_cache_info - Get the CXL cache information of a CXL cache device
>> + * @cxlds: CXL device state associated with cache device
>> + *
>> + * Returns 0 and populates the struct cxl_cache_state member of @cxlds on
>> + * success, error otherwise.
>> + */
>> +int cxl_accel_read_cache_info(struct cxl_dev_state *cxlds)
>> +{
>> +	struct cxl_cache_state *cstate = &cxlds->cstate;
>> +	struct pci_dev *pdev;
>> +	int dvsec, rc;
>> +	u16 cap, cap2;
>> +
>> +	if (!dev_is_pci(cxlds->dev))
>> +		return -EINVAL;
>> +	pdev = to_pci_dev(cxlds->dev);
>> +
>> +	dvsec = cxlds->cxl_dvsec;
>> +
>> +	rc = pci_read_config_word(pdev, dvsec + CXL_DVSEC_CAP_OFFSET, &cap);
>> +	if (rc)
>> +		return rc;
>> +
>> +	if (!(cap & CXL_DVSEC_CACHE_CAPABLE))
>> +		return -ENXIO;
>> +
>> +	rc = pci_read_config_word(pdev, dvsec + CXL_DVSEC_CAP2_OFFSET, &cap2);
>> +	if (rc)
>> +		return rc;
>> +
>> +	/* CXL 3.2 8.1.3.7 DVSEC CXL Capability2 for encoding */
> 
> Given old versions of CXL spec tend to become hard to get, probably
> shift to using 4.0 references for this series (this probably raced
> with that being published!)

I wasn't aware that it was published, so that's the reason it's still 3.2 XD.
I'll update them for v1.

> 
>> +	switch (FIELD_GET(CXL_DVSEC_CACHE_UNIT_MASK, cap2)) {
>> +	case 1:
>> +		cstate->unit = 64 * SZ_1K;
> 
> SZ_64K

Will change.

> 
> I'll comment on it in the sysfs abi patch but I'm not seeing unit
> as necessarily something we need to keep around once we have
> used it to interpret the size.  So probably local variable is fine.
> 

I'll address this in the sysfs patch.
>> +		break;
>> +	case 2:
>> +		cstate->unit = SZ_1M;
>> +		break;
>> +	default:
> 
> I wonder if there are devices out there that simply don't tell us
> (so case 0)?

I heard of such a device yesterday as a matter of fact, so I need to update this.

> 
>> +		return -ENXIO;
>> +	}
>> +
>> +	cstate->size = FIELD_GET(CXL_DVSEC_CACHE_SIZE_MASK, cap2) * cstate->unit;
>> +	if (!cstate->size)
>> +		return -ENXIO;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_read_cache_info, "CXL");
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index 1950cf3d5399..259d806fb3e3 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -759,6 +759,17 @@ struct cxl_dpa_info {
>>  	int nr_partitions;
>>  };
>>  
>> +
>> +/**
>> + * struct cxl_cache_state - State of a device's CXL cache
>> + * @size: Size of cache in bytes
>> + * @unit: Unit size of cache in bytes
>> + */
>> +struct cxl_cache_state {
>> +	u64 size;
>> +	u32 unit;
>> +};
>