Re: [PATCH 2/5] cxl/region: Add dynamic capacity cxl region support.

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

Dave Jiang wrote:
> 
> 
> On 6/14/23 12:16, ira.weiny@intel.com wrote:
> > From: Navneet Singh <navneet.singh@intel.com>
> > 
> > CXL devices optionally support dynamic capacity. CXL Regions must be
> > created to access this capacity.
> > 
> > Add sysfs entries to create dynamic capacity cxl regions. Provide a new
> > Dynamic Capacity decoder mode which targets dynamic capacity on devices
> > which are added to that region.
> > 
> > Below are the steps to create and delete dynamic capacity region0
> > (example).
> > 
> >      region=$(cat /sys/bus/cxl/devices/decoder0.0/create_dc_region)
> >      echo $region> /sys/bus/cxl/devices/decoder0.0/create_dc_region
> >      echo 256 > /sys/bus/cxl/devices/$region/interleave_granularity
> >      echo 1 > /sys/bus/cxl/devices/$region/interleave_ways
> > 
> >      echo "dc0" >/sys/bus/cxl/devices/decoder1.0/mode
> >      echo 0x400000000 >/sys/bus/cxl/devices/decoder1.0/dpa_size
> > 
> >      echo 0x400000000 > /sys/bus/cxl/devices/$region/size
> >      echo  "decoder1.0" > /sys/bus/cxl/devices/$region/target0
> >      echo 1 > /sys/bus/cxl/devices/$region/commit
> >      echo $region > /sys/bus/cxl/drivers/cxl_region/bind
> > 
> >      echo $region> /sys/bus/cxl/devices/decoder0.0/delete_region
> > 
> > Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> > 
> > ---
> > [iweiny: fixups]
> > [iweiny: remove unused CXL_DC_REGION_MODE macro]
> > [iweiny: Make dc_mode_to_region_index static]
> > [iweiny: simplify <sysfs>/create_dc_region]
> > [iweiny: introduce decoder_mode_is_dc]
> > [djbw: fixups, no sign-off: preview only]
> > ---
> >   drivers/cxl/Kconfig       |  11 +++
> >   drivers/cxl/core/core.h   |   7 ++
> >   drivers/cxl/core/hdm.c    | 234 ++++++++++++++++++++++++++++++++++++++++++----
> >   drivers/cxl/core/port.c   |  18 ++++
> >   drivers/cxl/core/region.c | 135 ++++++++++++++++++++++++--
> >   drivers/cxl/cxl.h         |  28 ++++++
> >   drivers/dax/cxl.c         |   4 +
> >   7 files changed, 409 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> > index ff4e78117b31..df034889d053 100644
> > --- a/drivers/cxl/Kconfig
> > +++ b/drivers/cxl/Kconfig
> > @@ -121,6 +121,17 @@ config CXL_REGION
> >   
> >   	  If unsure say 'y'
> >   
> > +config CXL_DCD
> > +	bool "CXL: DCD Support"
> > +	default CXL_BUS
> > +	depends on CXL_REGION
> > +	help
> > +	  Enable the CXL core to provision CXL DCD regions.
> > +	  CXL devices optionally support dynamic capacity and DCD region
> > +	  maps the dynamic capacity regions DPA's into Host HPA ranges.
> > +
> > +	  If unsure say 'y'
> > +
> >   config CXL_REGION_INVALIDATION_TEST
> >   	bool "CXL: Region Cache Management Bypass (TEST)"
> >   	depends on CXL_REGION
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index 27f0968449de..725700ab5973 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -9,6 +9,13 @@ extern const struct device_type cxl_nvdimm_type;
> >   
> >   extern struct attribute_group cxl_base_attribute_group;
> >   
> > +#ifdef CONFIG_CXL_DCD
> > +extern struct device_attribute dev_attr_create_dc_region;
> > +#define SET_CXL_DC_REGION_ATTR(x) (&dev_attr_##x.attr),
> > +#else
> > +#define SET_CXL_DC_REGION_ATTR(x)
> > +#endif
> > +
> >   #ifdef CONFIG_CXL_REGION
> >   extern struct device_attribute dev_attr_create_pmem_region;
> >   extern struct device_attribute dev_attr_create_ram_region;
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 514d30131d92..29649b47d177 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -233,14 +233,23 @@ static void __cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
> >   	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> >   	struct resource *res = cxled->dpa_res;
> >   	resource_size_t skip_start;
> > +	resource_size_t skipped = cxled->skip;
> >   
> >   	lockdep_assert_held_write(&cxl_dpa_rwsem);
> >   
> >   	/* save @skip_start, before @res is released */
> > -	skip_start = res->start - cxled->skip;
> > +	skip_start = res->start - skipped;
> >   	__release_region(&cxlds->dpa_res, res->start, resource_size(res));
> > -	if (cxled->skip)
> > -		__release_region(&cxlds->dpa_res, skip_start, cxled->skip);
> > +	if (cxled->skip != 0) {
> > +		while (skipped != 0) {
> > +			res = xa_load(&cxled->skip_res, skip_start);
> > +			__release_region(&cxlds->dpa_res, skip_start,
> > +							resource_size(res));
> > +			xa_erase(&cxled->skip_res, skip_start);
> > +			skip_start += resource_size(res);
> > +			skipped -= resource_size(res);
> > +			}
> > +	}
> >   	cxled->skip = 0;
> >   	cxled->dpa_res = NULL;
> >   	put_device(&cxled->cxld.dev);
> > @@ -267,6 +276,19 @@ static void devm_cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
> >   	__cxl_dpa_release(cxled);
> >   }
> >   
> > +static int dc_mode_to_region_index(enum cxl_decoder_mode mode)
> > +{
> > +	int index = 0;
> > +
> > +	for (int i = CXL_DECODER_DC0; i <= CXL_DECODER_DC7; i++) {
> > +		if (mode == i)
> > +			return index;
> > +		index++;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >   static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> >   			     resource_size_t base, resource_size_t len,
> >   			     resource_size_t skipped)
> > @@ -275,7 +297,11 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> >   	struct cxl_port *port = cxled_to_port(cxled);
> >   	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> >   	struct device *dev = &port->dev;
> > +	struct device *ed_dev = &cxled->cxld.dev;
> > +	struct resource *dpa_res = &cxlds->dpa_res;
> > +	resource_size_t skip_len = 0;
> >   	struct resource *res;
> > +	int rc, index;
> >   
> >   	lockdep_assert_held_write(&cxl_dpa_rwsem);
> >   
> > @@ -304,28 +330,119 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> >   	}
> >   
> >   	if (skipped) {
> > -		res = __request_region(&cxlds->dpa_res, base - skipped, skipped,
> > -				       dev_name(&cxled->cxld.dev), 0);
> > -		if (!res) {
> > -			dev_dbg(dev,
> > -				"decoder%d.%d: failed to reserve skipped space\n",
> > -				port->id, cxled->cxld.id);
> > -			return -EBUSY;
> > +		resource_size_t skip_base = base - skipped;
> > +
> > +		if (decoder_mode_is_dc(cxled->mode)) {
> 
> Maybe move this entire block to a helper function to reduce the size of 
> the current function and reduce indent levels and improve readability?

:-/

I'll work on breaking it out more.  The logic here is getting kind of
crazy.

> 
> > +			if (resource_size(&cxlds->ram_res) &&
> > +					skip_base <= cxlds->ram_res.end) {
> > +				skip_len = cxlds->ram_res.end - skip_base + 1;
> > +				res = __request_region(dpa_res, skip_base,
> > +						skip_len, dev_name(ed_dev), 0);
> > +				if (!res)
> > +					goto error;
> > +
> > +				rc = xa_insert(&cxled->skip_res, skip_base, res,
> > +								GFP_KERNEL);
> > +				skip_base += skip_len;
> > +			}
> > +
> > +			if (resource_size(&cxlds->ram_res) &&
                                                  ^^^^^^^
						  pmem_res?

> > +					skip_base <= cxlds->pmem_res.end) {

The 2 if statements here are almost exactly the same.  To the point I
wonder if there is a bug.

Navneet,

Why does the code check ram_res the second time but go on to use pmem_res
in the block?

> > +				skip_len = cxlds->pmem_res.end - skip_base + 1;
> > +				res = __request_region(dpa_res, skip_base,
> > +						skip_len, dev_name(ed_dev), 0);
> > +				if (!res)
> > +					goto error;
> > +
> > +				rc = xa_insert(&cxled->skip_res, skip_base, res,
> > +								GFP_KERNEL);
> > +				skip_base += skip_len;
> > +			}
> > +
> > +			index = dc_mode_to_region_index(cxled->mode);
> > +			for (int i = 0; i <= index; i++) {
> > +				struct resource *dcr = &cxlds->dc_res[i];
> > +
> > +				if (skip_base < dcr->start) {
> > +					skip_len = dcr->start - skip_base;
> > +					res = __request_region(dpa_res,
> > +							skip_base, skip_len,
> > +							dev_name(ed_dev), 0);
> > +					if (!res)
> > +						goto error;
> > +
> > +					rc = xa_insert(&cxled->skip_res, skip_base,
> > +							res, GFP_KERNEL);
> > +					skip_base += skip_len;
> > +				}
> > +
> > +				if (skip_base == base) {
> > +					dev_dbg(dev, "skip done!\n");
> > +					break;
> > +				}
> > +
> > +				if (resource_size(dcr) &&
> > +						skip_base <= dcr->end) {
> > +					if (skip_base > base)
> > +						dev_err(dev, "Skip error\n");
> > +
> > +					skip_len = dcr->end - skip_base + 1;
> > +					res = __request_region(dpa_res, skip_base,
> > +							skip_len,
> > +							dev_name(ed_dev), 0);
> > +					if (!res)
> > +						goto error;
> > +
> > +					rc = xa_insert(&cxled->skip_res, skip_base,
> > +							res, GFP_KERNEL);
> > +					skip_base += skip_len;
> > +				}
> > +			}
> > +		} else	{
> > +			res = __request_region(dpa_res, base - skipped, skipped,
> > +							dev_name(ed_dev), 0);
> > +			if (!res)
> > +				goto error;
> > +
> > +			rc = xa_insert(&cxled->skip_res, skip_base, res,
> > +								GFP_KERNEL);
> >   		}
> >   	}
> > -	res = __request_region(&cxlds->dpa_res, base, len,
> > -			       dev_name(&cxled->cxld.dev), 0);
> > +
> > +	res = __request_region(dpa_res, base, len, dev_name(ed_dev), 0);
> >   	if (!res) {
> >   		dev_dbg(dev, "decoder%d.%d: failed to reserve allocation\n",
> > -			port->id, cxled->cxld.id);
> > -		if (skipped)
> > -			__release_region(&cxlds->dpa_res, base - skipped,
> > -					 skipped);
> > +				port->id, cxled->cxld.id);
> > +		if (skipped) {
> > +			resource_size_t skip_base = base - skipped;
> > +
> > +			while (skipped != 0) {
> > +				if (skip_base > base)
> > +					dev_err(dev, "Skip error\n");
> > +
> > +				res = xa_load(&cxled->skip_res, skip_base);
> > +				__release_region(dpa_res, skip_base,
> > +							resource_size(res));
> > +				xa_erase(&cxled->skip_res, skip_base);
> > +				skip_base += resource_size(res);
> > +				skipped -= resource_size(res);
> > +			}
> > +		}
> >   		return -EBUSY;
> >   	}
> >   	cxled->dpa_res = res;
> >   	cxled->skip = skipped;
> >   
> > +	for (int mode = CXL_DECODER_DC0; mode <= CXL_DECODER_DC7; mode++) {
> > +		int index = dc_mode_to_region_index(mode);
> > +
> > +		if (resource_contains(&cxlds->dc_res[index], res)) {
> > +			cxled->mode = mode;
> > +			dev_dbg(dev, "decoder%d.%d: %pr mode: %d\n", port->id,
> > +				cxled->cxld.id, cxled->dpa_res, cxled->mode);
> > +			goto success > +		}
> > +	}
> 
> This block should only happen if decoder_mode_is_dc() right? If that's 
> the case, you might be able to refactor it so the 'goto success' isn't 
> necessary.

I'll check.  I looked through this code a couple of times in my review
before posting because I'm not 100% sure I want to see 8 different modes
DC decoders and regions.

I think the 'mode' should be 'DC' with an index in the endpoint decoder to
map DC region that decoder is mapping.  But that change was much bigger to
Navneets code and I wanted to see how others felt about having DC0 - DC7
modes.  My compromise was creating decoder_mode_is_dc().

> 
> >   	if (resource_contains(&cxlds->pmem_res, res))
> >   		cxled->mode = CXL_DECODER_PMEM;
> >   	else if (resource_contains(&cxlds->ram_res, res))
> > @@ -336,9 +453,16 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> >   		cxled->mode = CXL_DECODER_MIXED;
> >   	}
> >   
> > +success:
> >   	port->hdm_end++;
> >   	get_device(&cxled->cxld.dev);
> >   	return 0;
> > +
> > +error:
> > +	dev_dbg(dev, "decoder%d.%d: failed to reserve skipped space\n",
> > +			port->id, cxled->cxld.id);
> > +	return -EBUSY;
> > +
> >   }
> >   
> >   int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> > @@ -429,6 +553,14 @@ int cxl_dpa_set_mode(struct cxl_endpoint_decoder *cxled,
> >   	switch (mode) {
> >   	case CXL_DECODER_RAM:
> >   	case CXL_DECODER_PMEM:
> > +	case CXL_DECODER_DC0:
> > +	case CXL_DECODER_DC1:
> > +	case CXL_DECODER_DC2:
> > +	case CXL_DECODER_DC3:
> > +	case CXL_DECODER_DC4:
> > +	case CXL_DECODER_DC5:
> > +	case CXL_DECODER_DC6:
> > +	case CXL_DECODER_DC7:

For example this seems very hacky...

[snip]

> >   
> > +/*
> > + * The region can not be manged by CXL if any portion of
> > + * it is already online as 'System RAM'
> > + */
> > +static bool region_is_system_ram(struct cxl_region *cxlr,
> > +				 struct cxl_region_params *p)
> > +{
> > +	return (walk_iomem_res_desc(IORES_DESC_NONE,
> > +				    IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
> > +				    p->res->start, p->res->end, cxlr,
> > +				    is_system_ram) > 0);
> > +}
> > +
> >   static int cxl_region_probe(struct device *dev)
> >   {
> >   	struct cxl_region *cxlr = to_cxl_region(dev);
> > @@ -3174,14 +3287,7 @@ static int cxl_region_probe(struct device *dev)
> >   	case CXL_DECODER_PMEM:
> >   		return devm_cxl_add_pmem_region(cxlr);
> >   	case CXL_DECODER_RAM:
> > -		/*
> > -		 * The region can not be manged by CXL if any portion of
> > -		 * it is already online as 'System RAM'
> > -		 */
> > -		if (walk_iomem_res_desc(IORES_DESC_NONE,
> > -					IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
> > -					p->res->start, p->res->end, cxlr,
> > -					is_system_ram) > 0)
> > +		if (region_is_system_ram(cxlr, p))
> 
> Maybe split this change out as a prep patch before the current patch.

That seems reasonable.  But the patch is not so large and the
justification for creating a helper is that we need this same check for DC
regions.  So it seemed ok to leave it like this.  Let me see about
splitting it out.

[snip]

> > diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c
> > index ccdf8de85bd5..eb5eb81bfbd7 100644
> > --- a/drivers/dax/cxl.c
> > +++ b/drivers/dax/cxl.c
> > @@ -23,11 +23,15 @@ static int cxl_dax_region_probe(struct device *dev)
> >   	if (!dax_region)
> >   		return -ENOMEM;
> >   
> > +	if (decoder_mode_is_dc(cxlr->mode))
> > +		return 0;
> > +
> >   	data = (struct dev_dax_data) {
> >   		.dax_region = dax_region,
> >   		.id = -1,
> >   		.size = range_len(&cxlr_dax->hpa_range),
> >   	};
> > +
> 
> Stray blank line?

Opps!  Fixed!

Ira