Re: [PATCH v2 2/5] cxl: Introduce to_{ram,pmem}_{res,perf}() helpers

[Dan Williams <dan.j.williams@intel.com>]

Alejandro Lucero Palau wrote:
> 
> On 1/22/25 08:59, Dan Williams wrote:
> > In preparation for consolidating all DPA partition information into an
> > array of DPA metadata, introduce helpers that hide the layout of the
> > current data. I.e. make the eventual replacement of ->ram_res,
> > ->pmem_res, ->ram_perf, and ->pmem_perf with a new DPA metadata array a
> > no-op for code paths that consume that information, and reduce the noise
> > of follow-on patches.
> >
> > The end goal is to consolidate all DPA information in 'struct
> > cxl_dev_state', but for now the helpers just make it appear that all DPA
> > metadata is relative to @cxlds.
> >
> > Note that a follow-on patch also cleans up the temporary placeholders of
> > @ram_res, and @pmem_res in the qos_class manipulation code,
> > cxl_dpa_alloc(), and cxl_mem_create_range_info().
> >
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Alejandro Lucero <alucerop@amd.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >   drivers/cxl/core/cdat.c      |   70 +++++++++++++++++++++++++-----------------
> >   drivers/cxl/core/hdm.c       |   26 ++++++++--------
> >   drivers/cxl/core/mbox.c      |   18 ++++++-----
> >   drivers/cxl/core/memdev.c    |   42 +++++++++++++------------
> >   drivers/cxl/core/region.c    |   10 ++++--
> >   drivers/cxl/cxlmem.h         |   58 ++++++++++++++++++++++++++++++-----
> >   drivers/cxl/mem.c            |    2 +
> >   tools/testing/cxl/test/cxl.c |   25 ++++++++-------
> >   8 files changed, 159 insertions(+), 92 deletions(-)
> >
> > diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> > index 8153f8d83a16..b177a488e29b 100644
> > --- a/drivers/cxl/core/cdat.c
> > +++ b/drivers/cxl/core/cdat.c
> > @@ -258,29 +258,33 @@ static void update_perf_entry(struct device *dev, struct dsmas_entry *dent,
> >   static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
> >   				     struct xarray *dsmas_xa)
[..]
> >   	xa_for_each(dsmas_xa, index, dent) {
> > -		if (resource_size(&cxlds->ram_res) &&
> > -		    range_contains(&ram_range, &dent->dpa_range))
> > -			update_perf_entry(dev, dent, &mds->ram_perf);
> > -		else if (resource_size(&cxlds->pmem_res) &&
> > -			 range_contains(&pmem_range, &dent->dpa_range))
> > -			update_perf_entry(dev, dent, &mds->pmem_perf);
> > -		else
> > -			dev_dbg(dev, "no partition for dsmas dpa: %pra\n",
> > -				&dent->dpa_range);
> > +		for (int i = 0; i < ARRAY_SIZE(partition); i++) {
> > +			const struct resource *res = partition[i];
> > +			struct range range = {
> > +				.start = res->start,
> > +				.end = res->end,
> > +			};
> > +
> > +			if (range_contains(&range, &dent->dpa_range))
> > +				update_perf_entry(dev, dent, perf[i]);
> 
> This is checking if range contains dent->dpa_range.
> 
> I think it is just the opposite.

This looks like an equivalent conversion to me, what am I missing?

> > @@ -567,6 +578,9 @@ static bool dpa_perf_contains(struct cxl_dpa_perf *perf,
> >   		.end = dpa_res->end,
> >   	};
> >   
> > +	if (!perf)
> > +		return false;
> > +
> 
> This change seems to be an improvement or hardening. Not against doing 
> it, but seizing the change, the function can be simplified using the 
> parameter without any local variable.

No, it's not pure hardening, it is actively avoiding NULL pointer
de-references introduced by the new scheme to not track empty
partitions.

I.e the new to_{ram,pmem}_perf() helpers return NULL when the partition
is zero-sized. Previously this code path would do range checks on empty
partitions. 

> >   	return range_contains(&perf->dpa_range, &dpa);
> >   }
> >   
[..]
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 548564c770c0..3502f1633ad2 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -1270,24 +1270,26 @@ static int add_dpa_res(struct device *dev, struct resource *parent,
> >   int cxl_mem_create_range_info(struct cxl_memdev_state *mds)
> >   {
> >   	struct cxl_dev_state *cxlds = &mds->cxlds;
> > +	struct resource *ram_res = to_ram_res(cxlds);
> > +	struct resource *pmem_res = to_pmem_res(cxlds);
> >   	struct device *dev = cxlds->dev;
> >   	int rc;
> >   
> >   	if (!cxlds->media_ready) {
> >   		cxlds->dpa_res = DEFINE_RES_MEM(0, 0);
> > -		cxlds->ram_res = DEFINE_RES_MEM(0, 0);
> > -		cxlds->pmem_res = DEFINE_RES_MEM(0, 0);
> > +		*ram_res = DEFINE_RES_MEM(0, 0);
> > +		*pmem_res = DEFINE_RES_MEM(0, 0);
> 
> 
> This is a good example for the discussion about the  patch hardening 
> resource_contains. The initialization seems fine but IORESOURCE_UNSET 
> not used.

To the contrary, I think these changes are an example of "updating
resource_contains() to check for zero is a band-aid that does not fix
the root problem".

> it could be argued the resource is set, but it is a zero-size resource 
> leading to problems in current CXL code.

I challenge you to find problems in current CXL code after these
partition reworks.

Passing empty ranges to resource_contains() (and don't forget
range_contains()) is either a sign of a confused caller, or a caller
that expressly wants "contains" to return true on empty matches. With
these DPA metadata changes a primary source if not all 0-sized resources
in drivers/cxl/ calls to resource_contains() are eliminated. The new
cxl_dpa_setup() arranges for any walk of cxlds->nr_partitions to never
find a zero-sized resource.