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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 23 Sep 2026 21:29:58 -0700
Anisa Su <anisa.su887@gmail.com> wrote:

> On Tue, Sep 22, 2026 at 12:34:02AM +0100, Jonathan Cameron wrote:
> > On Wed, 5 Aug 2026 01:30:16 -0700
> > Anisa Su <anisa.su887@gmail.com> wrote:
> >   
> > > On Tue, Aug 04, 2026 at 01:07:48AM +0100, Jonathan Cameron wrote:  
> > > > On Fri, 31 Jul 2026 09:02:49 +0000
> > > > sashiko-bot@kernel.org 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/20241210-dcd-type2-upstream-v8-6-812852504400@intel.com/
> > > 
> > > and a following patch exposed it, with a documented ABI:
> > > 
> > >   https://lore.kernel.org/all/20241210-dcd-type2-upstream-v8-7-812852504400@intel.com/
> > > 
> > >     /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/20250413-dcd-type2-upstream-v9-3-1d4911a0b365@intel.com/
> > > 
> > > 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 vaguely recall the discussion on this ending up with we'd call the
> > partition something different to reflect the extra property.  
> > 
> > dynamic_readonly_1 / dynamic_sharedram_1 maybe?  Argument being
> > that these properties are as different from normal RAM as persistent
> > memory is.
> >   
> Yeah I think we discussed making it its own partition mode but I don't think there
> was a strong consensus, just that we agreed it should be exposed somewhere,
> somehow...
> 
> I am leaning towards just putting it under dynamic_ram_1 even though
> these attributes would only exist for dynamic_ram mode:
> 
> /sys/bus/cxl/devices/memX/dynamic_ram_1/shareable
> /sys/bus/cxl/devices/memX/dynamic_ram_1/read_only
> 
> Mainly because if we go up to 8 partitions, we would have
> (dynamic_ram, dynamic_shared, dynamic_readonly, dynamic_shared_readonly) * 8.
> And NDCTL would have to know how to handle creating each type.
> 
> I also don't see anything in the code that makes it terrible:
> - each partition mode has its own attribute array and visibility callback
>   function, so it doesn't affect ram/pmem modes
> 
> I brought it up because I wasn't sure if there was a specific reason why
> shareable and read_only weren't carried over after the partition
> rework. From a code standpoint, it seems fine to me?
> I assumed it was so all partitions would share common sub-attributes
> but I guess I shouldn't have assumed...

I think this needs another discussion. I agree there were differences
of opinion but thought we'd ended up with the different names rather
than attributes for each one.

> 
> > > 
> > > - I don't think the DSMAS Flags bit 6 (read only) is specific to DCD?  
> > 
> > In theory yes, in practice there isn't a path for non DCD to actually
> > get the data in place and transtion to read only so maybe an expensive
> > source of zeros?  So yes allowed but not much use to anyone so we
> > won't see it.  
> >   
> Mmm very true. You might just clarifying this point here but just to make
> sure we're on the same page: I dropped the R_O part of the series and deferred
> it for the future because it includes implementing write rejection for
> r_o partitions which IMO can be done after the core functionality is in.

Why do we need to reject writes?  The device just drops them. Maybe
it is a nice to have to prevent the mapping path setting up writeable mappings
and so fault earlier but I'm not sure it is necessary.

> 
> Thanks,
> Anisa
> > >   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.
> > >   
> > Jonathan
> >