Re: [PATCH v15 3/8] cxl/cdat: Gather DSMAS data for DCD partitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] cxl: Ineffective DCD disablement leaves partially configured partitions exposed
- [Medium] cxl: QoS class 0 incorrectly overwrites CXL_QOS_CLASS_INVALID
- [Low] cxl: Lockless modification of DPA partition attributes exposes torn reads
--

commit 6722908eab4fa5c8dd9820d34f1ec3beb729c8f0
Author: Ira Weiny <iweiny@kernel.org>

cxl/cdat: Gather DSMAS data for DCD partitions

Additional DCD partition (AKA region) information is contained in the
DSMAS CDAT tables, including performance, shareable and read-only
attributes. Match DCD partitions with DSMAS tables and store the meta
data.

> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 5c9f072625135..07aedff657d9e 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
[ ... ]
> @@ -219,6 +226,13 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  	xa_for_each(dsmas_xa, index, dent) {
>  		int qos_class;
>  
> +		/* No DSLBIS for this DSMAS; nothing to hand the QTG _DSM */
> +		if (!dent->cdat_coord[ACCESS_COORDINATE_CPU].read_bandwidth &&
> +		    !dent->cdat_coord[ACCESS_COORDINATE_CPU].write_bandwidth &&
> +		    !dent->cdat_coord[ACCESS_COORDINATE_CPU].read_latency &&
> +		    !dent->cdat_coord[ACCESS_COORDINATE_CPU].write_latency)
> +			continue;

[Severity: Medium]
When this continue statement is taken, dent->qos_class remains 0 (from
kzalloc_obj zero-initialization).

If at least one other entry succeeds, cxl_memdev_apply_dsmas() will be called
with CXL_DSMAS_APPLY_FLAGS_PERF, which unconditionally calls update_perf_entry()
and overwrites part->perf.qos_class (previously CXL_QOS_CLASS_INVALID) with 0.

Since 0 is a valid QoS class in CXL, does this falsely advertise a QoS class of
0 to userspace for partitions lacking DSLBIS performance coordinates?

> +
>  		cxl_coordinates_combine(dent->coord, dent->cdat_coord, ep_c);
>  		dent->entries = 1;
>  		rc = cxl_root->ops.qos_class(cxl_root,
[ ... ]
> @@ -255,35 +269,90 @@ static void update_perf_entry(struct device *dev, struct dsmas_entry *dent,
>  		dent->coord[ACCESS_COORDINATE_CPU].write_latency);
>  }
>  
> -static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
> -				     struct xarray *dsmas_xa)
> +/*
> + * A DCD shall describe its DC partition in a DSMAS entry (CXL r4.0 9.13.3).
> + * Without one the partition's attributes are unknown, so disable DCD.
> + */
> +static void cxl_dcd_dsmas_missing(struct cxl_dev_state *cxlds, const char *why)
> +{
> +	struct cxl_memdev_state *mds;
> +
> +	if (cxlds->type != CXL_DEVTYPE_CLASSMEM)
> +		return;
> +
> +	mds = to_cxl_memdev_state(cxlds);
> +	if (!cxl_dcd_supported(mds) ||
> +	    !cxl_part_size(cxlds, CXL_PARTMODE_DYNAMIC_RAM))
> +		return;
> +
> +	dev_warn(cxlds->dev, "DC partition has no valid DSMAS entry (%s), disabling DCD\n",
> +		 why);
> +	cxl_disable_dcd(mds);

[Severity: High]
Does clearing the mds->dcd_supported flag effectively disable the partition?

Since cxl_configure_dcd() and cxl_dpa_setup() have already executed during
cxl_pci_probe(), the DCD partition is permanently added to cxlds->part and its
DPA resource is registered.

Because mds->dcd_supported is never evaluated again during region creation or
sysfs access, does the partition remain fully exposed and usable by userspace
without valid DSMAS attributes?

> +}
> +
> +enum cxl_dsmas_apply {
> +	CXL_DSMAS_APPLY_FLAGS,		/* shareable etc.; no QTG data */
> +	CXL_DSMAS_APPLY_FLAGS_PERF,	/* perf coordinates and qos_class */
> +};
> +
> +/*
> + * DSMAS flags such as shareable are properties of the CDAT entry and are
> + * applied regardless; perf data is applied only if the QTG lookup produced
> + * recommendations.
> + */
> +static void cxl_memdev_apply_dsmas(struct cxl_dev_state *cxlds,
> +				   struct xarray *dsmas_xa,
> +				   enum cxl_dsmas_apply attrs)
>  {
>  	struct device *dev = cxlds->dev;
>  	struct dsmas_entry *dent;
> +	bool dc_matched = false;
>  	unsigned long index;
>  
>  	xa_for_each(dsmas_xa, index, dent) {
>  		bool found = false;
>  
>  		for (int i = 0; i < cxlds->nr_partitions; i++) {
> -			struct resource *res = &cxlds->part[i].res;
> +			struct cxl_dpa_partition *part = &cxlds->part[i];
[ ... ]
> +			if (attrs == CXL_DSMAS_APPLY_FLAGS_PERF)
> +				update_perf_entry(dev, dent, &part->perf);
> +			if (part->mode == CXL_PARTMODE_DYNAMIC_RAM) {
> +				part->shareable = dent->shareable;
> +				part->read_only = dent->read_only;
> +				dc_matched = true;
> +			}

[Severity: Low]
Are these partition attributes being modified locklessly?

The CXL memdev and its sysfs attributes are registered and visible during
cxl_pci_probe(), while this CDAT parsing happens later in
cxl_endpoint_port_probe().

If part->perf, part->shareable, and part->read_only are updated without holding
the cxl_rwsem.dpa lock, can concurrent userspace sysfs readers observe
intermediate, torn, or partially updated performance states?

> +			break;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007201828.952-1-anisa.su@samsung.com?part=3