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? > + > 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/[email protected]?part=3
