Re: [RFC PATCH 4/4] cxl/region: Introduce concept of region configuration

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 10 Jun 2021 11:57:25 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> The region creation APIs leave a region unconfigured. Configuring the
> region will work in the same way as similar subsystems such as devdax.
> Sysfs attrs will be provided to allow userspace to configure the region.
> Finally once all configuration is complete, userspace may "commit" the
> config. What the kernel decides to do after a config is committed is out
> of scope at this point.
> 
> Introduced here are the most basic attributes needed to configure a
> region.
> 
> A x1 interleave example is provided below:
> 
> decoder1.0
> ├── create_region
> ├── delete_region
> ├── devtype
> ├── locked
> ├── region1.0:0
> │   ├── offset
> │   ├── size
> │   ├── subsystem -> ../../../../../../../bus/cxl
> │   ├── target0
> │   ├── uevent
> │   ├── uuid
> │   └── verify
> ├── size
> ├── start
> ├── subsystem -> ../../../../../../bus/cxl
> ├── target_list
> ├── target_type
> └── uevent
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |  27 +++
>  drivers/cxl/mem.h                       |   2 +
>  drivers/cxl/region.c                    | 227 +++++++++++++++++++++++-
>  3 files changed, 255 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 699c8514fd7b..d7174a84f70d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -159,3 +159,30 @@ Description:
>  		integer is returned describing the first error found in the
>  		configuration. A verified region can still fail binding due to
>  		lack of resources.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/{offset,size}

I missed this before, but why do we need X.Y in the region naming given it's
always inside decoderX.Y.  Seems like RegionZ would work.

> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		A region resides within an address space that is claimed by a
> +		decoder. The region will be of some size within the address
> +		space and at some offset that must also reside within the
> +		address space. The size and position of the region is specified
> +		by these attributes.
Could perhaps reword this.  Something like.

		The region defined by size and offset must be fully contained
		within the address space.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/uuid
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		The unique identifier for the region.
> +
> +What:		/sys/bus/cxl/devices/decoderX.Y/regionX.Y:Z/target[0-15]
> +Date:		June, 2021
> +KernelVersion:	v5.14
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		Memory devices are the backing storage for a region. Each target
> +		must be populated with a memdev in order for the region to be
> +		eligible to be activated.

How do you do that?  What is written to this file?

> diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> index 9795aa924035..059fbf084fa1 100644
> --- a/drivers/cxl/mem.h
> +++ b/drivers/cxl/mem.h
> @@ -58,6 +58,7 @@ static inline struct cxl_memdev *to_cxl_memdev(struct device *dev)
>   * @dev: This region's device.
>   * @id: This regions id. Id is globally unique across all regions.
>   * @res: Address space consumed by this region.
> + * @uuid: The UUID for this region.
>   * @list: Node in decoders region list.
>   * @targets: The memory devices comprising the region.
>   */
> @@ -65,6 +66,7 @@ struct cxl_region {
>  	struct device dev;
>  	int id;
>  	struct resource res;
> +	uuid_t uuid;
>  	struct list_head list;
>  	struct cxl_memdev *targets[];
>  };
> diff --git a/drivers/cxl/region.c b/drivers/cxl/region.c
> index ea1ac848c713..a69ee00514cb 100644
> --- a/drivers/cxl/region.c
> +++ b/drivers/cxl/region.c
> @@ -3,7 +3,9 @@
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
> +#include <linux/sizes.h>
>  #include <linux/slab.h>
> +#include <linux/uuid.h>
>  #include <linux/idr.h>
>  #include "cxl.h"
>  #include "mem.h"
> @@ -20,15 +22,130 @@
>   * relationship between decoder and region when the region is interleaved.
>   */
>  
> +static struct cxl_region *to_cxl_region(struct device *dev);
> +
> +#define cxl_region_ways(region)                                                \
> +	to_cxl_decoder((region)->dev.parent)->interleave_ways
> +
>  static ssize_t verify_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> +	struct cxl_decoder *cxld = to_cxl_decoder(dev->parent);
> +	struct cxl_region *region = to_cxl_region(dev);
> +	struct resource decode_res;
> +	int i;
> +
> +	decode_res = (struct resource)DEFINE_RES_MEM(cxld->range.start,
> +						     range_len(&cxld->range));
> +
> +	/* Invalid region size */
> +	if (!resource_contains(&decode_res, &region->res))
> +		return sysfs_emit(buf, "size");

Perhaps "outside region"?  Size might be fine, but not the offset.
Also, docs say a negative integer is returned for this attribute.
Those docs need to call out the full list of things that might be returned
so that userspace can know what to expect.

> +
> +	if (resource_size(&region->res) % (SZ_256M * cxld->interleave_ways))
> +		return sysfs_emit(buf, "alignment");
> +
> +	/* Missing target memory device */
> +	for (i = 0; i < cxld->interleave_ways; i++)
> +		if (!region->targets[i])
> +			return sysfs_emit(buf, "memdev");
> +
>  	return sysfs_emit(buf, "0");
>  }
>  
>  static DEVICE_ATTR_RO(verify);
>  
> +static bool is_region_active(struct cxl_region *region)
> +{
> +	/* TODO: Regions can't be activated yet. */
> +	return false;
> +}
> +
> +static ssize_t offset_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct cxl_region *region = to_cxl_region(dev);
> +
> +	return sysfs_emit(buf, "%#llx\n", region->res.start);
> +}
> +
> +static ssize_t offset_store(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t len)
> +{
> +	struct cxl_region *region = to_cxl_region(dev);
> +	unsigned long long val;
> +	ssize_t rc;
> +
> +	rc = kstrtoull(buf, 0, &val);
> +	if (rc)
> +		return rc;
> +
> +	if (is_region_active(region)) {
> +		/* TODO: */
> +	} else {
> +		region->res.start = val;
> +	}
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR_RW(offset);
> +
> +static ssize_t size_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct cxl_region *region = to_cxl_region(dev);
> +
> +	return sysfs_emit(buf, "%llu\n", resource_size(&region->res));
> +}
> +
> +static ssize_t size_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t len)
> +{
> +	struct cxl_region *region = to_cxl_region(dev);
> +	unsigned long long val;
> +	ssize_t rc;
> +
> +	rc = kstrtoull(buf, 0, &val);
> +	if (rc)
> +		return rc;
> +
> +	if (is_region_active(region)) {
> +		/* TODO: */
> +	} else {
> +		region->res.end = region->res.start + val - 1;
> +	}
> +
> +	return len;
> +}
> +static DEVICE_ATTR_RW(size);
> +
> +static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct cxl_region *region = to_cxl_region(dev);
> +
> +	return sysfs_emit(buf, "%pUb\n", &region->uuid);
> +}
> +static ssize_t uuid_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t len)
> +{
> +	struct cxl_region *region = to_cxl_region(dev);
> +	ssize_t rc;
> +
> +	if (len != UUID_STRING_LEN + 1)
> +		return -EINVAL;
> +
> +	rc = uuid_parse(buf, &region->uuid);
> +
> +	return rc ? rc : len;
> +}
> +static DEVICE_ATTR_RW(uuid);
> +
>  static struct attribute *region_attrs[] = {
>  	&dev_attr_verify.attr,
> +	&dev_attr_offset.attr,
> +	&dev_attr_size.attr,
> +	&dev_attr_uuid.attr,
>  	NULL,
>  };
>  
> @@ -36,8 +153,111 @@ static const struct attribute_group region_group = {
>  	.attrs = region_attrs,
>  };
>  
> +static size_t show_targetN(struct cxl_region *region, char *buf, int n)
> +{
> +	if (region->targets[n])
> +		return sysfs_emit(buf, "%s\n", dev_name(&region->targets[n]->dev));
> +	else
> +		return sysfs_emit(buf, "nil\n");

This needs documenting in the ABI docs. I'd I guessed it would return an empty
string.

> +}
> +
> +static size_t set_targetN(struct cxl_region *region, const char *buf, int n, size_t len)
> +{
> +	struct device *memdev_dev;
> +	struct cxl_memdev *cxlmd;
> +	ssize_t rc;
> +	int val;
> +
> +	rc = kstrtoint(buf, 0, &val);
> +	if (!rc && val == 0) {
> +		cxlmd = region->targets[n] = cxlmd;
> +		if (cxlmd)
> +			put_device(&cxlmd->dev);
> +		region->targets[n] = NULL;
> +		return len;
> +	}
> +
> +	memdev_dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf);
> +	if (!memdev_dev)
> +		return -ENOENT;
> +
> +	cxlmd = to_cxl_memdev(memdev_dev);
> +	get_device(&cxlmd->dev);
> +	region->targets[n] = cxlmd;
> +
> +	return len;
> +}
> +
> +#define TARGET_ATTR_RW(n)                                                      \
> +	static ssize_t target##n##_show(                                       \
> +		struct device *dev, struct device_attribute *attr, char *buf)  \
> +	{                                                                      \
> +		return show_targetN(to_cxl_region(dev), buf, n);               \
> +	}                                                                      \
> +	static ssize_t target##n##_store(struct device *dev,                   \
> +					 struct device_attribute *attr,        \
> +					 const char *buf, size_t len)          \
> +	{                                                                      \
> +		return set_targetN(to_cxl_region(dev), buf, n, len);           \
> +	}                                                                      \
> +	static DEVICE_ATTR_RW(target##n)
> +
> +TARGET_ATTR_RW(0);
> +TARGET_ATTR_RW(1);
> +TARGET_ATTR_RW(2);
> +TARGET_ATTR_RW(3);
> +TARGET_ATTR_RW(4);
> +TARGET_ATTR_RW(5);
> +TARGET_ATTR_RW(6);
> +TARGET_ATTR_RW(7);
> +TARGET_ATTR_RW(8);
> +TARGET_ATTR_RW(9);
> +TARGET_ATTR_RW(10);
> +TARGET_ATTR_RW(11);
> +TARGET_ATTR_RW(12);
> +TARGET_ATTR_RW(13);
> +TARGET_ATTR_RW(14);
> +TARGET_ATTR_RW(15);
> +
> +static struct attribute *interleave_attrs[] = {
> +	&dev_attr_target0.attr,
> +	&dev_attr_target1.attr,
> +	&dev_attr_target2.attr,
> +	&dev_attr_target3.attr,
> +	&dev_attr_target4.attr,
> +	&dev_attr_target5.attr,
> +	&dev_attr_target6.attr,
> +	&dev_attr_target7.attr,
> +	&dev_attr_target8.attr,
> +	&dev_attr_target9.attr,
> +	&dev_attr_target10.attr,
> +	&dev_attr_target11.attr,
> +	&dev_attr_target12.attr,
> +	&dev_attr_target13.attr,
> +	&dev_attr_target14.attr,
> +	&dev_attr_target15.attr,
> +	NULL,
> +};
> +
> +static umode_t visible_targets(struct kobject *kobj, struct attribute *a, int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct cxl_region *region = to_cxl_region(dev);
> +
> +	if (n < cxl_region_ways(region))
> +		return a->mode;
> +	return 0;
> +}
> +
> +static const struct attribute_group region_interleave_group = {
> +	.attrs = interleave_attrs,
> +	.is_visible = visible_targets,
> +};
> +
>  static const struct attribute_group *region_groups[] = {
>  	&region_group,
> +	&region_interleave_group,
> +	NULL,
>  };
>  
>  static void cxl_region_release(struct device *dev);
> @@ -58,8 +278,13 @@ static struct cxl_region *to_cxl_region(struct device *dev)
>  
>  void cxl_free_region(struct cxl_decoder *cxld, struct cxl_region *region)
>  {
> +	int i;
> +
>  	ida_free(&cxld->region_ida, region->id);
> -	kfree(region->targets);

This line looks like a bug in earlier patch as targets is allocated
as part of the allocation of region.

> +	for (i = 0; i < cxld->interleave_ways; i++) {
> +		if (region->targets[i])
> +			put_device(&region->targets[i]->dev);
> +	}
>  	kfree(region);
>  }
>