Message-ID: <678837fcc0ed_20f3294fb@dwillia2-xfh.jf.intel.com.notmuch>
Date: Wed, 15 Jan 2025 14:34:36 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Ira Weiny <ira.weiny@...el.com>, Dan Williams <dan.j.williams@...el.com>,
	Dave Jiang <dave.jiang@...el.com>, Fan Ni <fan.ni@...sung.com>, "Jonathan
 Cameron" <Jonathan.Cameron@...wei.com>, Jonathan Corbet <corbet@....net>,
	Andrew Morton <akpm@...ux-foundation.org>, Kees Cook <kees@...nel.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>
CC: Dan Williams <dan.j.williams@...el.com>, Davidlohr Bueso
	<dave@...olabs.net>, Alison Schofield <alison.schofield@...el.com>, "Vishal
 Verma" <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
	<linux-cxl@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<nvdimm@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
	<linux-hardening@...r.kernel.org>, Li Ming <ming.li@...omail.com>
Subject: Re: [PATCH v8 02/21] cxl/mem: Read dynamic capacity configuration
 from the device

Ira Weiny wrote:
> Dan Williams wrote:
> > Ira Weiny wrote:
> 
> [snip]
> 
> > > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > > index e8907c403edbd83c8a36b8d013c6bc3391207ee6..05a0718aea73b3b2a02c608bae198eac7c462523 100644
> > > --- a/drivers/cxl/cxlmem.h
> > > +++ b/drivers/cxl/cxlmem.h
> > > @@ -403,6 +403,7 @@ enum cxl_devtype {
> > >  	CXL_DEVTYPE_CLASSMEM,
> > >  };
> > >  
> > > +#define CXL_MAX_DC_REGION 8
> > 
> > Please no, lets not sign up to have the "which cxl 'region' concept are
> > you referring to?" debate in perpetuity. "DPA partition", "DPA
> > resource", "DPA capacity" anything but "region".
> > 
> 
> I'm inclined to agree with Alejandro on this one.  I've walked this
> tightrope quite a bit with this series.  But there are other places where
> we have chosen to change the verbiage from the spec and it has made it
> difficult for new comers to correlate the spec with the code.
> 
> So I like Alejandro's idea of adding "HW" to the name to indicate that we
> are talking about a spec or hardware defined thing.

See below, the only people that could potentially be bothered by the
lack of spec terminology matching are the very same people that are
sophisticated enough to have read the spec to know its a problem.

> 
> That said I am open to changing some names where it is clear it is a
> software structure.  I'll audit the series for that.
> 
> > >  	u64 serial;
> > >  	enum cxl_devtype type;
> > >  	struct cxl_mailbox cxl_mbox;
> > >  };
> > >  
> > > +#define CXL_DC_REGION_STRLEN 8
> > > +struct cxl_dc_region_info {
> > > +	u64 base;
> > > +	u64 decode_len;
> > > +	u64 len;
> > 
> > Duplicating partition information in multiple places, like
> > mds->dc_region[X].base and cxlds->dc_res[X].start, feels like an
> > RFC-quality decision for expediency that needs to reconciled on the way
> > to upstream.
> 
> I think this was done to follow a pattern of the mds being passed around
> rather than creating resources right when partitions are read.
> 
> Furthermore this stands to hold this information in CPU endianess rather
> than holding an array of region info coming from the hardware.

Yes, the ask is translate all of this into common information that lives
at the cxl_dev_state level.

> 
> Let see how other changes fall out before I go hacking this though.
> 
> > 
> > > +	u64 blk_size;
> > > +	u32 dsmad_handle;
> > > +	u8 flags;
> > > +	u8 name[CXL_DC_REGION_STRLEN];
> > 
> > No, lets not entertain:
> > 
> >     printk("%s\n", mds->dc_region[index].name);
> > 
> > ...when:
> > 
> >     printk("dc%d\n", index);
> > 
> > ...will do.
> 
> Actually these buffers provide a buffer for the (struct
> resource)dc_res[x].name pointers to point to.

I missed that specific detail, but I still challenge whether this
precision is needed especially since it makes the data structure
messier. Given these names are for debug only and multi-partition DCD
devices seem unlikely to ever exist, just use a static shared name for
adding to ->dpa_res.

> 
> > 
> > DCD introduces the concept of "decode size vs usable capacity" into the
> > partition information, but I see no reason to conceptually tie that to
> > only DCD.  Fabio's memory hole patches show that there is already a
> > memory-hole concept in the CXL arena. DCD is just saying "be prepared for
> > the concept of DPA partitions with memory holes at the end".
> 
> I'm not clear how this relates.  ram and pmem partitions can already have
> holes at the end if not mapped.

The distinction is "can this DPA capacity be allocated to a region" the
new holes introduced by DCD are cases where the partition size is
greater than the allocatable size. Contrast to ram and pmem the
allocatable size is always identical to the partition size.

> > > +
> > >  	struct cxl_event_state event;
> > >  	struct cxl_poison_state poison;
> > >  	struct cxl_security_state security;
> > > @@ -708,6 +732,32 @@ struct cxl_mbox_set_partition_info {
> > >  
> > >  #define  CXL_SET_PARTITION_IMMEDIATE_FLAG	BIT(0)
> > >  
> > > +/* See CXL 3.1 Table 8-163 get dynamic capacity config Input Payload */
> > > +struct cxl_mbox_get_dc_config_in {
> > > +	u8 region_count;
> > > +	u8 start_region_index;
> > > +} __packed;
> > > +
> > > +/* See CXL 3.1 Table 8-164 get dynamic capacity config Output Payload */
> > > +struct cxl_mbox_get_dc_config_out {
> > > +	u8 avail_region_count;
> > > +	u8 regions_returned;
> > > +	u8 rsvd[6];
> > > +	/* See CXL 3.1 Table 8-165 */
> > > +	struct cxl_dc_region_config {
> > > +		__le64 region_base;
> > > +		__le64 region_decode_length;
> > > +		__le64 region_length;
> > > +		__le64 region_block_size;
> > > +		__le32 region_dsmad_handle;
> > > +		u8 flags;
> > > +		u8 rsvd[3];
> > > +	} __packed region[] __counted_by(regions_returned);
> > 
> > Yes, the spec unfortunately uses "region" for this partition info
> > payload. This would be a good place to say "CXL spec calls this 'region'
> > but Linux calls it 'partition' not to be confused with the Linux 'struct
> > cxl_region' or all the other usages of 'region' in the specification".
> 
> In this case I totally disagree.  This is a structure being filled in by
> the hardware and is directly related to the spec.  I think I would rather
> change 
> 
> s/cxl_dc_region_info/cxl_dc_partition_info/
> 
> And leave this.  Which draws a more distinct line between what is
> specified in hardware vs a software construct.
> 
> > 
> > Linux is not obligated to follow the questionable naming decisions of
> > specifications.
> 
> We are not.  But as Alejandro says it can be confusing if we don't make
> some association to the spec.
> 
> What do you think about the HW/SW line I propose above?

Rename to cxl_dc_partition_info and drop the region_ prefixes, sure.

Otherwise, for this init-time only concern I would much rather deal with
the confusion of:

"why does Linux call this partition when the spec calls it region?":
which only trips up people that already know the difference because they read the
spec. In that case the comment will answer their confusion.

...versus:

"why are there multiple region concepts in the CXL subsystem": which
trips up everyone that greps through the CXL subsystem especially those
that have no intention of ever reading the spec.

Powered by blists - more mailing lists