On Wed, Oct 07, 2026 at 08:33:12PM +0000, [email protected] wrote:
> 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 <[email protected]>
>
> 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?
>
Fixed. dsams_entry.qos_class is initialized to CXL_QOS_CLASS_INVALID in
cdat_dsmas_handler().
> > +
> > 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?
>
Nothing is exposed in sysfs by this series. The rest of the series that does
adds those capabilities will hide the memdev dynamic_ram group and refuse the
decoder mode if dcd is not supported.
> > +}
> > +
> > +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?
>
update_perf_entry() is the function that writes to part->perf and not
touched by this series.
shareable and read-only are bools, so stores are atomic.
> > + break;
> > }
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=3
