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@xxxxxxxxxxxx>
> >> Cc: Kees Cook <kees@xxxxxxxxxx>
> >> Cc: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
> >> Cc: linux-hardening@xxxxxxxxxxxxxxx
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >> Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> >>
> >> ---
> >> 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.
