Re: [PATCH v8 02/21] cxl/mem: Read dynamic capacity configuration from the device

[Ira Weiny <ira.weiny@intel.com>]

Dan Williams wrote:
> 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.

Honestly at this point I think the code has deviated enough from the spec
that it is just not worth me saying any more.  I'll change everything to
partition and field the questions later as they come.

> 
> > 
> > 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.

yea.  And build on what you have in the DPA rework.

> 
> > 
> > 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.

Using a static name is good.

> 
> > 
> > > 
> > > 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.

I still don't quite get what you are saying.  The user can always allocate
a region of the full DCD partition size.  It is just that the memory
within that region may not be backed yet (no extents).

> 
> > > > +
> > > >  	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?":

But this is not the question I will get.  The question will be.

"Where is DCD region processed in the code?  I grepped for region and
found nothing."

Or

"I'm searching the spec PDF for DCD partition and can't find that.  Where
is DCD partition specified?"

> 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.

Ok I've already said more than I intended.  I will change everything to
partition.

Ira