Re: [PATCH 34/46] cxl/region: Add region creation support

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

Jonathan Cameron wrote:
> On Thu, 23 Jun 2022 21:19:38 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > From: Ben Widawsky <bwidawsk@kernel.org>
> > 
> > CXL 2.0 allows for dynamic provisioning of new memory regions (system
> > physical address resources like "System RAM" and "Persistent Memory").
> > Whereas DDR and PMEM resources are conveyed statically at boot, CXL
> > allows for assembling and instantiating new regions from the available
> > capacity of CXL memory expanders in the system.
> > 
> > Sysfs with an "echo $region_name > $create_region_attribute" interface
> > is chosen as the mechanism to initiate the provisioning process. This
> > was chosen over ioctl() and netlink() to keep the configuration
> > interface entirely in a pseudo-fs interface, and it was chosen over
> > configfs since, aside from this one creation event, the interface is
> > read-mostly. I.e. configfs supports cases where an object is designed to
> > be provisioned each boot, like an iSCSI storage target, and CXL region
> > creation is mostly for PMEM regions which are created usually once
> > per-lifetime of a server instance.
> > 
> > Recall that the major change that CXL brings over previous
> > persistent memory architectures is the ability to dynamically define new
> > regions.  Compare that to drivers like 'nfit' where the region
> > configuration is statically defined by platform firmware.
> > 
> > Regions are created as a child of a root decoder that encompasses an
> > address space with constraints. When created through sysfs, the root
> > decoder is explicit. When created from an LSA's region structure a root
> > decoder will possibly need to be inferred by the driver.
> > 
> > Upon region creation through sysfs, a vacant region is created with a
> > unique name. Regions have a number of attributes that must be configured
> > before the region can be bound to the driver where HDM decoder program
> > is completed.
> > 
> > An example of creating a new region:
> > 
> > - Allocate a new region name:
> > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_pmem_region)
> > 
> > - Create a new region by name:
> > while
> > region=$(cat /sys/bus/cxl/devices/decoder0.0/create_pmem_region)
> 
> Perhaps it is worth calling out the region ID allocator is shared
> with nvdimms and other usecases.  I'm not really sure what the advantage
> in doing that is, but it doesn't do any real harm.

The rationale is that there are several producers of memory regions
nvdimm, device-dax (hmem), and now cxl. Of those cases cxl can pass
regoins to nvdimm and nvdimm can pass regions to device-dax (pmem). If
each of those cases allocated their own region-id it would just
complicate debug for no benefit. I can add this a note to remind why
memregion_alloc() was introduced in the first instance.

> 
> > ! echo $region > /sys/bus/cxl/devices/decoder0.0/create_pmem_region
> > do true; done

I recall you also asked to clarify the rationale of this complexity. It
is related to the potential proliferation of disaparate region ids, but
also a lesson learned from nvdimm which itself learned lessons from
md-raid. The lesson from md-raid in short is do not use ioctl for object
creation. After "not ioctl" the choice is configfs or a small bit of
sysfs hackery. Configfs is overkill when there is already a sysfs
hierarchy that just needs one new object injected.

Namespace creation in nvdimm pre-created "seed" devices which let the
kernel control the naming, but confused end users that wondered about
vestigial devices. This "read to learn next object name" + "write to
atomically claim and instantiate that id" cleans up that vestigial
device problem while also constraining object naming to follow memregion
id expectations.

> > 
> > - Region now exists in sysfs:
> > stat -t /sys/bus/cxl/devices/decoder0.0/$region
> > 
> > - Delete the region, and name:
> > echo $region > /sys/bus/cxl/devices/decoder0.0/delete_region
> > 
> > Signed-off-by: Ben Widawsky <bwidawsk@kernel.org>
> > [djbw: simplify locking, reword changelog]
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cxl       |  25 +++
> >  .../driver-api/cxl/memory-devices.rst         |  11 +
> >  drivers/cxl/Kconfig                           |   5 +
> >  drivers/cxl/core/Makefile                     |   1 +
> >  drivers/cxl/core/core.h                       |  12 ++
> >  drivers/cxl/core/port.c                       |  39 +++-
> >  drivers/cxl/core/region.c                     | 199 ++++++++++++++++++
> >  drivers/cxl/cxl.h                             |  18 ++
> >  tools/testing/cxl/Kbuild                      |   1 +
> >  9 files changed, 308 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/cxl/core/region.c
> > 
> 
> ...
> 
> 
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index 472ec9cb1018..ebe6197fb9b8 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -9,6 +9,18 @@ extern const struct device_type cxl_nvdimm_type;
> >  
> >  extern struct attribute_group cxl_base_attribute_group;
> >  
> > +#ifdef CONFIG_CXL_REGION
> > +extern struct device_attribute dev_attr_create_pmem_region;
> > +extern struct device_attribute dev_attr_delete_region;
> > +/*
> > + * Note must be used at the end of an attribute list, since it
> > + * terminates the list in the CONFIG_CXL_REGION=n case.
> 
> That's rather ugly.  Maybe just push the ifdef down into the c file
> where we will be shortening the list and it should be obvious what is
> going on without needing the comment?  Much as I don't like ifdef
> magic in the c files, it sometimes ends up cleaner.

No, I think ifdef in C is definitely uglier, but I also notice that
helpers like SET_SYSTEM_SLEEP_PM_OPS() are defined to be used in any
place in the list. So, I'll just duplicate that approach.

> 
> > + */
> > +#define CXL_REGION_ATTR(x) (&dev_attr_##x.attr)
> > +#else
> > +#define CXL_REGION_ATTR(x) NULL
> > +#endif
> > +
> >  struct cxl_send_command;
> >  struct cxl_mem_query_commands;
> >  int cxl_query_cmd(struct cxl_memdev *cxlmd,
> > diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> > index 2e56903399c2..c9207ebc3f32 100644
> > --- a/drivers/cxl/core/port.c
> > +++ b/drivers/cxl/core/port.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> >  /* Copyright(c) 2020 Intel Corporation. All rights reserved. */
> >  #include <linux/io-64-nonatomic-lo-hi.h>
> > +#include <linux/memregion.h>
> >  #include <linux/workqueue.h>
> >  #include <linux/debugfs.h>
> >  #include <linux/device.h>
> > @@ -300,11 +301,35 @@ static struct attribute *cxl_decoder_root_attrs[] = {
> >  	&dev_attr_cap_type2.attr,
> >  	&dev_attr_cap_type3.attr,
> >  	&dev_attr_target_list.attr,
> > +	CXL_REGION_ATTR(create_pmem_region),
> > +	CXL_REGION_ATTR(delete_region),
> >  	NULL,
> >  };
> 
> >  
> >  static const struct attribute_group *cxl_decoder_root_attribute_groups[] = {
> > @@ -387,6 +412,7 @@ static void cxl_root_decoder_release(struct device *dev)
> >  {
> >  	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev);
> >  
> > +	memregion_free(atomic_read(&cxlrd->region_id));
> >  	__cxl_decoder_release(&cxlrd->cxlsd.cxld);
> >  	kfree(cxlrd);
> >  }
> > @@ -1415,6 +1441,7 @@ static struct lock_class_key cxl_decoder_key;
> >  static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> >  					     unsigned int nr_targets)
> >  {
> > +	struct cxl_root_decoder *cxlrd = NULL;
> >  	struct cxl_decoder *cxld;
> >  	struct device *dev;
> >  	void *alloc;
> > @@ -1425,16 +1452,20 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> >  
> >  	if (nr_targets) {
> >  		struct cxl_switch_decoder *cxlsd;
> > -		struct cxl_root_decoder *cxlrd;
> >  
> >  		if (is_cxl_root(port)) {
> >  			alloc = kzalloc(struct_size(cxlrd, cxlsd.target,
> >  						    nr_targets),
> >  					GFP_KERNEL);
> >  			cxlrd = alloc;
> > -			if (cxlrd)
> > +			if (cxlrd) {
> >  				cxlsd = &cxlrd->cxlsd;
> > -			else
> > +				atomic_set(&cxlrd->region_id, -1);
> > +				rc = memregion_alloc(GFP_KERNEL);
> > +				if (rc < 0)
> > +					goto err;
> 
> Leaving region_id set to -1 seems interesting for ever
> recovering from this error.  Perhaps a comment on how the magic
> value is used.

Comment added.

> 
> > +				atomic_set(&cxlrd->region_id, rc);
> > +			} else
> >  				cxlsd = NULL;
> >  		} else {
> >  			alloc = kzalloc(struct_size(cxlsd, target, nr_targets),
> > @@ -1490,6 +1521,8 @@ static struct cxl_decoder *cxl_decoder_alloc(struct cxl_port *port,
> >  
> >  	return cxld;
> >  err:
> > +	if (cxlrd && atomic_read(&cxlrd->region_id) >= 0)
> > +		memregion_free(atomic_read(&cxlrd->region_id));
> >  	kfree(alloc);
> >  	return ERR_PTR(rc);
> >  }
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > new file mode 100644
> > index 000000000000..f2a0ead20ca7
> > --- /dev/null
> > +++ b/drivers/cxl/core/region.c
> > @@ -0,0 +1,199 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> > +#include <linux/memregion.h>
> > +#include <linux/genalloc.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/idr.h>
> > +#include <cxl.h>
> > +#include "core.h"
> > +
> > +/**
> > + * DOC: cxl core region
> > + *
> > + * CXL Regions represent mapped memory capacity in system physical address
> > + * space. Whereas the CXL Root Decoders identify the bounds of potential CXL
> > + * Memory ranges, Regions represent the active mapped capacity by the HDM
> > + * Decoder Capability structures throughout the Host Bridges, Switches, and
> > + * Endpoints in the topology.
> > + */
> > +
> > +static struct cxl_region *to_cxl_region(struct device *dev);
> > +
> > +static void cxl_region_release(struct device *dev)
> > +{
> > +	struct cxl_region *cxlr = to_cxl_region(dev);
> > +
> > +	memregion_free(cxlr->id);
> > +	kfree(cxlr);
> > +}
> > +
> > +static const struct device_type cxl_region_type = {
> > +	.name = "cxl_region",
> > +	.release = cxl_region_release,
> > +};
> > +
> > +bool is_cxl_region(struct device *dev)
> > +{
> > +	return dev->type == &cxl_region_type;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(is_cxl_region, CXL);
> > +
> > +static struct cxl_region *to_cxl_region(struct device *dev)
> > +{
> > +	if (dev_WARN_ONCE(dev, dev->type != &cxl_region_type,
> > +			  "not a cxl_region device\n"))
> > +		return NULL;
> > +
> > +	return container_of(dev, struct cxl_region, dev);
> > +}
> > +
> > +static void unregister_region(void *dev)
> > +{
> > +	device_unregister(dev);
> > +}
> > +
> > +static struct lock_class_key cxl_region_key;
> > +
> > +static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int id)
> > +{
> > +	struct cxl_region *cxlr;
> > +	struct device *dev;
> > +
> > +	cxlr = kzalloc(sizeof(*cxlr), GFP_KERNEL);
> > +	if (!cxlr) {
> > +		memregion_free(id);
> 
> That's a bit nasty as it gives the function side effects. Perhaps some
> comments in the callers of this to highlight that memregion will either be freed
> in here or handled over to the device.

Added to the devm_cxl_add_region() kdoc that the memregion id is freed
on failure.

> 
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	dev = &cxlr->dev;
> > +	device_initialize(dev);
> > +	lockdep_set_class(&dev->mutex, &cxl_region_key);
> > +	dev->parent = &cxlrd->cxlsd.cxld.dev;
> > +	device_set_pm_not_required(dev);
> > +	dev->bus = &cxl_bus_type;
> > +	dev->type = &cxl_region_type;
> > +	cxlr->id = id;
> > +
> > +	return cxlr;
> > +}
> > +
> > +/**
> > + * devm_cxl_add_region - Adds a region to a decoder
> > + * @cxlrd: root decoder
> > + * @id: memregion id to create
> > + * @mode: mode for the endpoint decoders of this region
> 
> Missing docs for type

Added.

> 
> > + *
> > + * This is the second step of region initialization. Regions exist within an
> > + * address space which is mapped by a @cxlrd.
> > + *
> > + * Return: 0 if the region was added to the @cxlrd, else returns negative error
> > + * code. The region will be named "regionZ" where Z is the unique region number.
> > + */
> > +static struct cxl_region *devm_cxl_add_region(struct cxl_root_decoder *cxlrd,
> > +					      int id,
> > +					      enum cxl_decoder_mode mode,
> > +					      enum cxl_decoder_type type)
> > +{
> > +	struct cxl_port *port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
> > +	struct cxl_region *cxlr;
> > +	struct device *dev;
> > +	int rc;
> > +
> > +	cxlr = cxl_region_alloc(cxlrd, id);
> > +	if (IS_ERR(cxlr))
> > +		return cxlr;
> > +	cxlr->mode = mode;
> > +	cxlr->type = type;
> > +
> > +	dev = &cxlr->dev;
> > +	rc = dev_set_name(dev, "region%d", id);
> > +	if (rc)
> > +		goto err;
> > +
> > +	rc = device_add(dev);
> > +	if (rc)
> > +		goto err;
> > +
> > +	rc = devm_add_action_or_reset(port->uport, unregister_region, cxlr);
> > +	if (rc)
> > +		return ERR_PTR(rc);
> > +
> > +	dev_dbg(port->uport, "%s: created %s\n",
> > +		dev_name(&cxlrd->cxlsd.cxld.dev), dev_name(dev));
> > +	return cxlr;
> > +
> > +err:
> > +	put_device(dev);
> > +	return ERR_PTR(rc);
> > +}
> > +
> 
> > +static ssize_t create_pmem_region_store(struct device *dev,
> > +					struct device_attribute *attr,
> > +					const char *buf, size_t len)
> > +{
> > +	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev);
> > +	struct cxl_region *cxlr;
> > +	unsigned int id, rc;
> > +
> > +	rc = sscanf(buf, "region%u\n", &id);
> > +	if (rc != 1)
> > +		return -EINVAL;
> > +
> > +	rc = memregion_alloc(GFP_KERNEL);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
> > +		memregion_free(rc);
> > +		return -EBUSY;
> > +	}
> > +
> > +	cxlr = devm_cxl_add_region(cxlrd, id, CXL_DECODER_PMEM,
> > +				   CXL_DECODER_EXPANDER);
> > +	if (IS_ERR(cxlr))
> > +		return PTR_ERR(cxlr);
> > +
> > +	return len;
> > +}
> > +DEVICE_ATTR_RW(create_pmem_region);
> > +
> > +static struct cxl_region *cxl_find_region_by_name(struct cxl_decoder *cxld,
> 
> Perhaps rename cxld here to make it clear it's a root decoder only.

Yes, in fact it should just be a 'struct cxl_root_decoder' type
argument.

> 
> > +						  const char *name)
> > +{
> > +	struct device *region_dev;
> > +
> > +	region_dev = device_find_child_by_name(&cxld->dev, name);
> > +	if (!region_dev)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	return to_cxl_region(region_dev);
> > +}
> > +
> > +static ssize_t delete_region_store(struct device *dev,
> > +				   struct device_attribute *attr,
> > +				   const char *buf, size_t len)
> > +{
> > +	struct cxl_port *port = to_cxl_port(dev->parent);
> > +	struct cxl_decoder *cxld = to_cxl_decoder(dev);
> As above, given it's the root decoder can we name it to make that
> obvious?

Right, this is now:

struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev);