Message-ID: <6788a82d2455a_20fa294b2@dwillia2-xfh.jf.intel.com.notmuch>
Date: Wed, 15 Jan 2025 22:33:17 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Alejandro Lucero Palau <alucerop@....com>, Dan Williams
	<dan.j.williams@...el.com>, Ira Weiny <ira.weiny@...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: Davidlohr Bueso <dave@...olabs.net>, Alison Schofield
	<alison.schofield@...el.com>, Vishal Verma <vishal.l.verma@...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

Alejandro Lucero Palau wrote:
> 
> On 1/15/25 02:35, Dan Williams wrote:
> > Ira Weiny wrote:
> >> Devices which optionally support Dynamic Capacity (DC) are configured
> >> via mailbox commands.  CXL 3.1 requires the host to issue the Get DC
> >> Configuration command in order to properly configure DCDs.  Without the
> >> Get DC Configuration command DCD can't be supported.
> >>
> >> Implement the DC mailbox commands as specified in CXL 3.1 section
> >> 8.2.9.9.9 (opcodes 48XXh) to read and store the DCD configuration
> >> information.  Disable DCD if DCD is not supported.  Leverage the Get DC
> >> Configuration command supported bit to indicate if DCD is supported.
> >>
> >> Linux has no use for the trailing fields of the Get Dynamic Capacity
> >> Configuration Output Payload (Total number of supported extents, number
> >> of available extents, total number of supported tags, and number of
> >> available tags).  Avoid defining those fields to use the more useful
> >> dynamic C array.
> >>
> >> Based on an original patch by Navneet Singh.
> >>
> >> Cc: Li Ming <ming.li@...omail.com>
> >> Cc: Kees Cook <kees@...nel.org>
> >> Cc: Gustavo A. R. Silva <gustavoars@...nel.org>
> >> Cc: linux-hardening@...r.kernel.org
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> >> Signed-off-by: Ira Weiny <ira.weiny@...el.com>
> >>
> >> ---
> >> Changes:
> >> [iweiny: fix EXPORT_SYMBOL_NS_GPL(cxl_dev_dynamic_capacity_identify)]
> >> [iweiny: limit variable scope in cxl_dev_dynamic_capacity_identify]
> >> ---
> >>   drivers/cxl/core/mbox.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++-
> >>   drivers/cxl/cxlmem.h    |  64 ++++++++++++++++++-
> >>   drivers/cxl/pci.c       |   4 ++
> >>   3 files changed, 232 insertions(+), 2 deletions(-)
> >>
> > [snipping the C code to do a data structure review first]
> >
> >> 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".
> >
> >
> 
> This next comment is not my main point to discuss in this email 
> (resources initialization is), but I seize it for giving my view in this 
> one.
> 
> Dan, you say later we (Linux) are not obligated to use "questionable 
> naming decisions of specifications", but we should not confuse people 
> either.
> 
> Maybe CXL_MAX_DC_HW_REGION would help here, for differentiating it from 
> the kernel software cxl region construct. I think we will need a CXL 
> kernel dictionary sooner or later ...

I addressed this on the reply to Ira, and yes one of the first entries
in a Linux CXL terminology document is that "regions" are mapped memory
and partitions are DPA capacity.

> >>   /**
> >>    * struct cxl_dpa_perf - DPA performance property entry
> >>    * @dpa_range: range for DPA address
> >> @@ -434,6 +435,8 @@ struct cxl_dpa_perf {
> >>    * @dpa_res: Overall DPA resource tree for the device
> >>    * @pmem_res: Active Persistent memory capacity configuration
> >>    * @ram_res: Active Volatile memory capacity configuration
> >> + * @dc_res: Active Dynamic Capacity memory configuration for each possible
> >> + *          region
> >>    * @serial: PCIe Device Serial Number
> >>    * @type: Generic Memory Class device or Vendor Specific Memory device
> >>    * @cxl_mbox: CXL mailbox context
> >> @@ -449,11 +452,23 @@ struct cxl_dev_state {
> >>   	struct resource dpa_res;
> >>   	struct resource pmem_res;
> >>   	struct resource ram_res;
> >> +	struct resource dc_res[CXL_MAX_DC_REGION];
> > This is throwing off cargo-cult alarms. The named pmem_res and ram_res
> > served us well up until the point where DPA partitions grew past 2 types
> > at well defined locations. I like the array of resources idea, but that
> > begs the question why not put all partition information into an array?
> >
> > This would also head off complications later on in this series where the
> > DPA capacity reservation and allocation flows have "dc" sidecars bolted
> > on rather than general semantics like "allocating from partition index N
> > means that all partitions indices less than N need to be skipped and
> > marked reserved".
> 
> 
> I guess this is likely how you want to change the type2 resource 
> initialization issue and where I'm afraid these two patchsets are going 
> to collide at.
> 
> If that is the case, both are going to miss the next kernel cycle since 
> it means major changes, but let's discuss it without further delays for 
> the sake of implementing the accepted changes as soon as possible, and I 
> guess with a close sync between Ira and I.

Type-2, as far as I can see, is a priority because it is in support of a
real device that end users can get their hands on today, right?

DCD, as far as I know, has no known product intercepts, just QEMU
emulation. So there is no rush there. If someone has information to the
contrary please share, if able.

The Type-2 series can also be prioritized because it is something we can get
done without cross-subsystem entanglements. So, no I do not think the
door is closed on Type-2 for v6.14, but it is certainly close which is
why I am throwing out code suggestions along with the review.

Otherwise, when 2 patch series trip over the same design wart (i.e. the
no longer suitable explicit ->ram_res and ->pmem_res members of 'struct
cxl_dev_state'), the cleanup needs to come first.

> BTW, in the case of the Type2, there are more things to discuss which I 
> do there.

Yes, hopefully it goes smoother after this point.

Powered by blists - more mailing lists