Re: [PATCH 16/46] cxl/hdm: Add 'mode' attribute to decoder objects

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

On Thu, 23 Jun 2022 19:46:59 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Recall that the Device Physical Address (DPA) space of a CXL Memory
> Expander is potentially partitioned into a volatile and persistent
> portion. A decoder maps a Host Physical Address (HPA) range to a DPA
> range and that translation depends on the value of all previous (lower
> instance number) decoders before the current one.
> 
> In preparation for allowing dynamic provisioning of regions, decoders
> need an ABI to indicate which DPA partition a decoder targets. This ABI
> needs to be prepared for the possibility that some other agent committed
> and locked a decoder that spans the partition boundary.
> 
> Add 'decoderX.Y/mode' to endpoint decoders that indicates which
> partition 'ram' / 'pmem' the decoder targets, or 'mixed' if the decoder
> currently spans the partition boundary.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

A few trivial things inline though I'm not super keen on it being
introduced RO for just 2 patches...  You could pull forwards
the outline of the store() to avoid that slight oddity, but
I'm not that bothered if it is a pain to do.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |   16 ++++++++++++++++
>  drivers/cxl/core/hdm.c                  |   10 ++++++++++
>  drivers/cxl/core/port.c                 |   20 ++++++++++++++++++++
>  drivers/cxl/cxl.h                       |    9 +++++++++
>  4 files changed, 55 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index 1fd5984b6158..091459216e11 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -164,3 +164,19 @@ Description:
>  		expander memory (type-3). The 'target_type' attribute indicates
>  		the current setting which may dynamically change based on what
>  		memory regions are activated in this decode hierarchy.
> +
> +

Single blank line used for previous entries. Note this carries to other
later patches.


> +What:		/sys/bus/cxl/devices/decoderX.Y/mode
> +Date:		May, 2022
> +KernelVersion:	v5.20
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) When a CXL decoder is of devtype "cxl_decoder_endpoint" it
> +		translates from a host physical address range, to a device local
> +		address range. Device-local address ranges are further split
> +		into a 'ram' (volatile memory) range and 'pmem' (persistent
> +		memory) range. The 'mode' attribute emits one of 'ram', 'pmem',
> +		'mixed', or 'none'. The 'mixed' indication is for error cases
> +		when a decoder straddles the volatile/persistent partition
> +		boundary, and 'none' indicates the decoder is not actively
> +		decoding, or no DPA allocation policy has been set.
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index daae6e533146..3f929231b822 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -204,6 +204,16 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  	cxled->dpa_res = res;
>  	cxled->skip = skip;
>  
> +	if (resource_contains(&cxlds->pmem_res, res))
> +		cxled->mode = CXL_DECODER_PMEM;
> +	else if (resource_contains(&cxlds->ram_res, res))
> +		cxled->mode = CXL_DECODER_RAM;
> +	else {
> +		dev_dbg(dev, "decoder%d.%d: %pr mixed\n", port->id,
> +			cxled->cxld.id, cxled->dpa_res);

Why debug for one case and not the the others?

> +		cxled->mode = CXL_DECODER_MIXED;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index b5f5fb9aa4b7..9d632c8c580b 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -171,6 +171,25 @@ static ssize_t target_list_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(target_list);
>  
> +static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct cxl_endpoint_decoder *cxled = to_cxl_endpoint_decoder(dev);
> +
> +	switch (cxled->mode) {
> +	case CXL_DECODER_RAM:
> +		return sysfs_emit(buf, "ram\n");
> +	case CXL_DECODER_PMEM:
> +		return sysfs_emit(buf, "pmem\n");
> +	case CXL_DECODER_NONE:
> +		return sysfs_emit(buf, "none\n");
> +	case CXL_DECODER_MIXED:
> +	default:
> +		return sysfs_emit(buf, "mixed\n");
> +	}
> +}
> +static DEVICE_ATTR_RO(mode);
> +
>  static struct attribute *cxl_decoder_base_attrs[] = {
>  	&dev_attr_start.attr,
>  	&dev_attr_size.attr,
> @@ -221,6 +240,7 @@ static const struct attribute_group *cxl_decoder_switch_attribute_groups[] = {
>  
>  static struct attribute *cxl_decoder_endpoint_attrs[] = {
>  	&dev_attr_target_type.attr,
> +	&dev_attr_mode.attr,
>  	NULL,
>  };
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6832d6d70548..aa223166f7ef 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -241,16 +241,25 @@ struct cxl_decoder {
>  	unsigned long flags;
>  };
>  
> +enum cxl_decoder_mode {
> +	CXL_DECODER_NONE,
> +	CXL_DECODER_RAM,
> +	CXL_DECODER_PMEM,
> +	CXL_DECODER_MIXED,
> +};
> +
>  /**
>   * struct cxl_endpoint_decoder - Endpoint  / SPA to DPA decoder
>   * @cxld: base cxl_decoder_object
>   * @dpa_res: actively claimed DPA span of this decoder
>   * @skip: offset into @dpa_res where @cxld.hpa_range maps
> + * @mode: which memory type / access-mode-partition this decoder targets
>   */
>  struct cxl_endpoint_decoder {
>  	struct cxl_decoder cxld;
>  	struct resource *dpa_res;
>  	resource_size_t skip;
> +	enum cxl_decoder_mode mode;
>  };
>  
>  /**
>