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

On Tue, Aug 04, 2026 at 01:07:48AM +0100, Jonathan Cameron wrote:
> On Fri, 31 Jul 2026 09:02:49 +0000
> [email protected] wrote:
> 
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> > - [Medium] The commit message claims to extract and store the 'read only' attribute from DSMAS tables, but this logic is completely missing from the code.
> 
> Given it will make a lot of difference to a user if they think they have writeable
> memory that isn't - I think we probably do want to have readonly here.
> 
I did some digging: it seems in v8, the read only flag was gathered and
exposed in sysfs, but dropped during the region -> partition rework.

v8 of this commit used to do:
    dent->read_only = dsmas->flags & ACPI_CDAT_DSMAS_READ_ONLY;
    ...
    mds->dc_region[i].read_only = dent->read_only;

    https://lore.kernel.org/all/[email protected]/

and a following patch exposed it, with a documented ABI:

  https://lore.kernel.org/all/[email protected]/

    /sys/bus/cxl/devices/memX/dcY/size
    /sys/bus/cxl/devices/memX/dcY/read_only
    /sys/bus/cxl/devices/memX/dcY/shareable
    /sys/bus/cxl/devices/memX/dcY/qos_class

In v9 dynamic capacity moved from its own dc_region[] array and dcY/
sysfs directory to the generic partition model:

  https://lore.kernel.org/all/[email protected]/

size and qos_class survived that move because ram and pmem already have
them. read_only and shareable did not, because the generic partition
model had nowhere to put attributes that only dynamic capacity has, but
the commit message survived the rework.

More specifically, read_only was dropped from both the sysfs commit and
this commit. shareable was dropped only from the sysfs commit, but still
read from the DSMAS entry in v9 of this commit. Not sure why though; I
didn't see any consumers of the attribute. Oh well... We use it now
since shared extents are supported to check if an extent marked shared
is actually in a shared partition.

So TLDR; I've removed the "read only" part from the commit message. But
let me know if you think it should be re-introduced?

I agree with what you said about it being important for a user to know
whether a partition is read only. Some things to consider:

- The partition model has no precedent for a DC only attribute.
  dynamic_ram_1/ currently exposes size and qos_class, the same two that
  ram/ and pmem/ expose. Adding read_only there would be the first
  attribute that exists for one partition mode and not the others.

- shareable is in exactly the same position. It is gathered and used
  internally but not exposed anywhere, so if we are adding one of these
  back we should probably decide about both.

- I don't think the DSMAS Flags bit 6 (read only) is specific to DCD?
  It describes any device scoped memory range, so a read only ram or pmem
  partition is expressible in CDAT today and the driver ignores it.
  Maybe it should be exposed in sysfs for every partition
  rather than only on dynamic_ram_1? Which probably makes it a generic CDAT/partition
  patch rather than something for this series.

> > [ ... ]
> > > @@ -266,15 +268,26 @@ static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
> > >  		bool found = false;
> > >  
> > >  		for (int i = 0; i < cxlds->nr_partitions; i++) {
> > > +			enum cxl_partition_mode mode = cxlds->part[i].mode;
> > >  			struct resource *res = &cxlds->part[i].res;
> > > +			u8 handle = cxlds->part[i].handle;
> > >  			struct range range = {
> > >  				.start = res->start,
> > >  				.end = res->end,
> > >  			};
> > >  
> > >  			if (range_contains(&range, &dent->dpa_range)) {
> > > +				if (mode == CXL_PARTMODE_DYNAMIC_RAM_1 &&
> > > +				    dent->handle != handle) {
> > > +					dev_warn(dev,
> > > +						 "Dynamic RAM perf mismatch; %pra (%u) vs %pra (%u)\n",
> > > +						 &range, handle, &dent->dpa_range,
> > > +						 dent->handle);
> > > +					continue;
> > > +				}  
> > 
> > [Severity: Medium]
> > Could this trigger spurious warnings on valid hardware configurations?
> > 
> > The CDAT specification allows overlapping DSMAS ranges with different handles
> > to support multiple memory types or sharing characteristics for the same DPA
> > space. 
> 
> Huh?  Where did it get that from.  I'm 99% sure you can't have overlapping
> DSMAS entries for the same DPA space.
> 
Sigh... sometimes it does that. I double-checked the CDAT spec (r1.04)
to confirm:

The DSMADHandle of a DSMAS entry is described as "The handle used to refer to
this DSMAD. Each instance of DSMAS shall be associated with a unique
DSMADHandle value."

So each DSMAS describes exactly 1 DSMAD (DPA range).

Additionally DSMAD is defined as:
	 Device Scoped Memory Affinity Domain (DSMAD) is a contiguous
	 DPA range *** with unique memory attributes*** ...

Treating it as a false positive.

Thanks,
Anisa
> > 
> > If a compliant device provides multiple valid DSMAS entries that overlap
> > the partition's range but have different handles, won't iterating over them 
> > incorrectly flag the unselected ones as a perf mismatch?
> >