[v4,08/23] cxl: Add support for _DSM Function for retrieving QTG ID

CXL spec v3.0 9.17.3 CXL Root Device Specific Methods (_DSM)

Add support to retrieve QTG ID via ACPI _DSM call. The _DSM call requires
an input of an ACPI package with 4 dwords (read latency, write latency,
read bandwidth, write bandwidth). The call returns a package with 1 WORD
that provides the max supported QTG ID and a package that may contain 0 or
more WORDs as the recommended QTG IDs in the recommended order.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>

---
v2:
- Reorder var declaration and use C99 style. (Jonathan)
- Allow >2 ACPI objects in package for future expansion. (Jonathan)
- Check QTG IDs against MAX QTG ID provided by output package. (Jonathan)
---
 drivers/cxl/core/Makefile |    1 
 drivers/cxl/core/acpi.c   |  116 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h         |   16 ++++++
 3 files changed, 133 insertions(+)
 create mode 100644 drivers/cxl/core/acpi.c

On Wed, 19 Apr 2023 13:21:49 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> CXL spec v3.0 9.17.3 CXL Root Device Specific Methods (_DSM)
> 
> Add support to retrieve QTG ID via ACPI _DSM call. The _DSM call requires
> an input of an ACPI package with 4 dwords (read latency, write latency,
> read bandwidth, write bandwidth). The call returns a package with 1 WORD
> that provides the max supported QTG ID and a package that may contain 0 or
> more WORDs as the recommended QTG IDs in the recommended order.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 

A few minor comments inline. 


> +/**
> + * cxl_acpi_evaluate_qtg_dsm - Retrieve QTG ids via ACPI _DSM
> + * @handle: ACPI handle
> + * @input: bandwidth and latency data
> + *
> + * Issue QTG _DSM with accompanied bandwidth and latency data in order to get
> + * the QTG IDs that falls within the performance data.
Falls within is a little vague.  Perhaps something like

the QTG IDs that are suitable for the performance point in order of most suitable
to least suitable.

> + */
> +struct qtg_dsm_output *cxl_acpi_evaluate_qtg_dsm(acpi_handle handle,
> +						 struct qtg_dsm_input *input)
> +{
> +	union acpi_object *out_obj, *out_buf, *pkg;
> +	union acpi_object in_buf = {
> +		.buffer = {
> +			.type = ACPI_TYPE_BUFFER,
> +			.pointer = (u8 *)input,
> +			.length = sizeof(u32) * 4,

sizeof(*input)?

Also, ACPI structures are always little endian. Do we need to be careful of that
here?

> +		},
> +	};
> +	union acpi_object in_obj = {
> +		.package = {
> +			.type = ACPI_TYPE_PACKAGE,
> +			.count = 1,
> +			.elements = &in_buf
> +		},
> +	};
> +	struct qtg_dsm_output *output = NULL;
> +	int len, rc, i;
> +	u16 *max_qtg;
> +
> +	out_obj = acpi_evaluate_dsm(handle, &acpi_cxl_qtg_id_guid, 1, 1, &in_obj);
> +	if (!out_obj)
> +		return ERR_PTR(-ENXIO);
> +
> +	if (out_obj->type != ACPI_TYPE_PACKAGE) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +
> +	/* Check Max QTG ID */
> +	pkg = &out_obj->package.elements[0];
> +	if (pkg->type != ACPI_TYPE_BUFFER) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +
> +	if (pkg->buffer.length != sizeof(u16)) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +	max_qtg = (u16 *)pkg->buffer.pointer;
> +
> +	/* Retrieve QTG IDs package */
> +	pkg = &out_obj->package.elements[1];
> +	if (pkg->type != ACPI_TYPE_PACKAGE) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +
> +	out_buf = &pkg->package.elements[0];
> +	if (out_buf->type != ACPI_TYPE_BUFFER) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +
> +	len = out_buf->buffer.length;
> +
> +	/* It's legal to have 0 QTG entries */
> +	if (len == 0)
> +		goto out;
> +
> +	/* Malformed package, not multiple of WORD size */
> +	if (len % sizeof(u16)) {
> +		rc = -ENXIO;
> +		goto out;
> +	}
> +
> +	output = kmalloc(len + sizeof(*output), GFP_KERNEL);
> +	if (!output) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +
> +	output->nr = len / sizeof(u16);
> +	memcpy(output->qtg_ids, out_buf->buffer.pointer, len);
> +
> +	for (i = 0; i < output->nr; i++) {
> +		if (output->qtg_ids[i] > *max_qtg)
> +			pr_warn("QTG ID %u greater than MAX %u\n",
> +				output->qtg_ids[i], *max_qtg);
> +	}
> +
> +out:
> +	ACPI_FREE(out_obj);
> +	return output;
> +
> +err:
> +	ACPI_FREE(out_obj);
> +	return ERR_PTR(rc);

Why not combine these with something like

	return IS_ERR(rc) ? ERR_PTR(rc) : output;

I'm fine with leaving as it is, if this is common style for these
sorts of ACPI functions.

> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_acpi_evaluate_qtg_dsm, CXL);

On 4/20/23 5:00 AM, Jonathan Cameron wrote:
> On Wed, 19 Apr 2023 13:21:49 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> CXL spec v3.0 9.17.3 CXL Root Device Specific Methods (_DSM)
>>
>> Add support to retrieve QTG ID via ACPI _DSM call. The _DSM call requires
>> an input of an ACPI package with 4 dwords (read latency, write latency,
>> read bandwidth, write bandwidth). The call returns a package with 1 WORD
>> that provides the max supported QTG ID and a package that may contain 0 or
>> more WORDs as the recommended QTG IDs in the recommended order.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
> 
> A few minor comments inline.
> 
> 
>> +/**
>> + * cxl_acpi_evaluate_qtg_dsm - Retrieve QTG ids via ACPI _DSM
>> + * @handle: ACPI handle
>> + * @input: bandwidth and latency data
>> + *
>> + * Issue QTG _DSM with accompanied bandwidth and latency data in order to get
>> + * the QTG IDs that falls within the performance data.
> Falls within is a little vague.  Perhaps something like
> 
> the QTG IDs that are suitable for the performance point in order of most suitable
> to least suitable.

Thanks. I will update with your suggestion.

> 
>> + */
>> +struct qtg_dsm_output *cxl_acpi_evaluate_qtg_dsm(acpi_handle handle,
>> +						 struct qtg_dsm_input *input)
>> +{
>> +	union acpi_object *out_obj, *out_buf, *pkg;
>> +	union acpi_object in_buf = {
>> +		.buffer = {
>> +			.type = ACPI_TYPE_BUFFER,
>> +			.pointer = (u8 *)input,
>> +			.length = sizeof(u32) * 4,
> 
> sizeof(*input)?

ok

> 
> Also, ACPI structures are always little endian. Do we need to be careful of that
> here?

sigh yes. I will add in endieness handling for the patch.

> 
>> +		},
>> +	};
>> +	union acpi_object in_obj = {
>> +		.package = {
>> +			.type = ACPI_TYPE_PACKAGE,
>> +			.count = 1,
>> +			.elements = &in_buf
>> +		},
>> +	};
>> +	struct qtg_dsm_output *output = NULL;
>> +	int len, rc, i;
>> +	u16 *max_qtg;
>> +
>> +	out_obj = acpi_evaluate_dsm(handle, &acpi_cxl_qtg_id_guid, 1, 1, &in_obj);
>> +	if (!out_obj)
>> +		return ERR_PTR(-ENXIO);
>> +
>> +	if (out_obj->type != ACPI_TYPE_PACKAGE) {
>> +		rc = -ENXIO;
>> +		goto err;
>> +	}
>> +
>> +	/* Check Max QTG ID */
>> +	pkg = &out_obj->package.elements[0];
>> +	if (pkg->type != ACPI_TYPE_BUFFER) {
>> +		rc = -ENXIO;
>> +		goto err;
>> +	}
>> +
>> +	if (pkg->buffer.length != sizeof(u16)) {
>> +		rc = -ENXIO;
>> +		goto err;
>> +	}
>> +	max_qtg = (u16 *)pkg->buffer.pointer;
>> +
>> +	/* Retrieve QTG IDs package */
>> +	pkg = &out_obj->package.elements[1];
>> +	if (pkg->type != ACPI_TYPE_PACKAGE) {
>> +		rc = -ENXIO;
>> +		goto err;
>> +	}
>> +
>> +	out_buf = &pkg->package.elements[0];
>> +	if (out_buf->type != ACPI_TYPE_BUFFER) {
>> +		rc = -ENXIO;
>> +		goto err;
>> +	}
>> +
>> +	len = out_buf->buffer.length;
>> +
>> +	/* It's legal to have 0 QTG entries */
>> +	if (len == 0)
>> +		goto out;
>> +
>> +	/* Malformed package, not multiple of WORD size */
>> +	if (len % sizeof(u16)) {
>> +		rc = -ENXIO;
>> +		goto out;
>> +	}
>> +
>> +	output = kmalloc(len + sizeof(*output), GFP_KERNEL);
>> +	if (!output) {
>> +		rc = -ENOMEM;
>> +		goto err;
>> +	}
>> +
>> +	output->nr = len / sizeof(u16);
>> +	memcpy(output->qtg_ids, out_buf->buffer.pointer, len);
>> +
>> +	for (i = 0; i < output->nr; i++) {
>> +		if (output->qtg_ids[i] > *max_qtg)
>> +			pr_warn("QTG ID %u greater than MAX %u\n",
>> +				output->qtg_ids[i], *max_qtg);
>> +	}
>> +
>> +out:
>> +	ACPI_FREE(out_obj);
>> +	return output;
>> +
>> +err:
>> +	ACPI_FREE(out_obj);
>> +	return ERR_PTR(rc);
> 
> Why not combine these with something like
> 
> 	return IS_ERR(rc) ? ERR_PTR(rc) : output;
> 
> I'm fine with leaving as it is, if this is common style for these
> sorts of ACPI functions.

I'll combine it. But if I just set output to ERR_PTR(errno) for all the 
error cases then we can just return output directly without checking?

> 
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_acpi_evaluate_qtg_dsm, CXL);
>

> >> +	}
> >> +
> >> +out:
> >> +	ACPI_FREE(out_obj);
> >> +	return output;
> >> +
> >> +err:
> >> +	ACPI_FREE(out_obj);
> >> +	return ERR_PTR(rc);  
> > 
> > Why not combine these with something like
> > 
> > 	return IS_ERR(rc) ? ERR_PTR(rc) : output;
> > 
> > I'm fine with leaving as it is, if this is common style for these
> > sorts of ACPI functions.  
> 
> I'll combine it. But if I just set output to ERR_PTR(errno) for all the 
> error cases then we can just return output directly without checking?

Excellent point.

> 
> >   
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_acpi_evaluate_qtg_dsm, CXL);  
> >

Dave Jiang wrote:
> CXL spec v3.0 9.17.3 CXL Root Device Specific Methods (_DSM)
> 
> Add support to retrieve QTG ID via ACPI _DSM call. The _DSM call requires
> an input of an ACPI package with 4 dwords (read latency, write latency,
> read bandwidth, write bandwidth). The call returns a package with 1 WORD
> that provides the max supported QTG ID and a package that may contain 0 or
> more WORDs as the recommended QTG IDs in the recommended order.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> v2:
> - Reorder var declaration and use C99 style. (Jonathan)
> - Allow >2 ACPI objects in package for future expansion. (Jonathan)
> - Check QTG IDs against MAX QTG ID provided by output package. (Jonathan)
> ---
>  drivers/cxl/core/Makefile |    1 
>  drivers/cxl/core/acpi.c   |  116 +++++++++++++++++++++++++++++++++++++++++++++

Why a new core file? This seems something that only drivers/cxl/acpi.c
could ever care about.

Similar to the @calc_hb callback for root decoders this is another
platform specific callback that the endpoint drivers need not care that
it is an ACPI platform or not. They just ask their 'root' cxl_port
implementation for a qos_class and whether that is ACPI or not is
hidden.

>  drivers/cxl/cxl.h         |   16 ++++++
>  3 files changed, 133 insertions(+)
>  create mode 100644 drivers/cxl/core/acpi.c
> 
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index 867a8014b462..30d61c8cae22 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -13,5 +13,6 @@ cxl_core-y += mbox.o
>  cxl_core-y += pci.o
>  cxl_core-y += hdm.o
>  cxl_core-y += cdat.o
> +cxl_core-y += acpi.o
>  cxl_core-$(CONFIG_TRACING) += trace.o
>  cxl_core-$(CONFIG_CXL_REGION) += region.o
> diff --git a/drivers/cxl/core/acpi.c b/drivers/cxl/core/acpi.c
> new file mode 100644
> index 000000000000..6eda5cad8d59
> --- /dev/null
> +++ b/drivers/cxl/core/acpi.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/acpi.h>
> +#include <linux/pci.h>
> +#include <asm/div64.h>
> +#include "cxlpci.h"
> +#include "cxl.h"
> +
> +const guid_t acpi_cxl_qtg_id_guid =

static?

> +	GUID_INIT(0xF365F9A6, 0xA7DE, 0x4071,
> +		  0xA6, 0x6A, 0xB4, 0x0C, 0x0B, 0x4F, 0x8E, 0x52);
> +
> +/**
> + * cxl_acpi_evaluate_qtg_dsm - Retrieve QTG ids via ACPI _DSM
> + * @handle: ACPI handle
> + * @input: bandwidth and latency data
> + *
> + * Issue QTG _DSM with accompanied bandwidth and latency data in order to get
> + * the QTG IDs that falls within the performance data.
> + */
> +struct qtg_dsm_output *cxl_acpi_evaluate_qtg_dsm(acpi_handle handle,
> +						 struct qtg_dsm_input *input)
> +{
> +	union acpi_object *out_obj, *out_buf, *pkg;
> +	union acpi_object in_buf = {
> +		.buffer = {
> +			.type = ACPI_TYPE_BUFFER,
> +			.pointer = (u8 *)input,
> +			.length = sizeof(u32) * 4,
> +		},
> +	};
> +	union acpi_object in_obj = {
> +		.package = {
> +			.type = ACPI_TYPE_PACKAGE,
> +			.count = 1,
> +			.elements = &in_buf
> +		},
> +	};
> +	struct qtg_dsm_output *output = NULL;
> +	int len, rc, i;
> +	u16 *max_qtg;
> +
> +	out_obj = acpi_evaluate_dsm(handle, &acpi_cxl_qtg_id_guid, 1, 1, &in_obj);
> +	if (!out_obj)
> +		return ERR_PTR(-ENXIO);
> +
> +	if (out_obj->type != ACPI_TYPE_PACKAGE) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +
> +	/* Check Max QTG ID */
> +	pkg = &out_obj->package.elements[0];
> +	if (pkg->type != ACPI_TYPE_BUFFER) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +
> +	if (pkg->buffer.length != sizeof(u16)) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +	max_qtg = (u16 *)pkg->buffer.pointer;
> +
> +	/* Retrieve QTG IDs package */
> +	pkg = &out_obj->package.elements[1];
> +	if (pkg->type != ACPI_TYPE_PACKAGE) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +
> +	out_buf = &pkg->package.elements[0];
> +	if (out_buf->type != ACPI_TYPE_BUFFER) {
> +		rc = -ENXIO;
> +		goto err;
> +	}
> +
> +	len = out_buf->buffer.length;
> +
> +	/* It's legal to have 0 QTG entries */
> +	if (len == 0)
> +		goto out;
> +
> +	/* Malformed package, not multiple of WORD size */
> +	if (len % sizeof(u16)) {
> +		rc = -ENXIO;
> +		goto out;
> +	}
> +
> +	output = kmalloc(len + sizeof(*output), GFP_KERNEL);

This feels more complicated than it needs to be. The only output from
this function that matters is a qos_class number for a given input. The
backup qtg-ids are not yet interesting without a real world example of
where selecting from the backup list vs any other id matters. In other
words the only recommendation is match or non-match. Whether a non-match
is in the backup list is still a platform-specific consideration that
Linux as of today has nothing to point to say that this distinction
matters.

That will be an end user call to their platform vendor to ask "there's
not enough capacity left in QoS class X what are the implications for
picking performance class Y", or "please increase capacity of the
window for QoS class X".

> +	if (!output) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +
> +	output->nr = len / sizeof(u16);
> +	memcpy(output->qtg_ids, out_buf->buffer.pointer, len);
> +
> +	for (i = 0; i < output->nr; i++) {
> +		if (output->qtg_ids[i] > *max_qtg)
> +			pr_warn("QTG ID %u greater than MAX %u\n",
> +				output->qtg_ids[i], *max_qtg);
> +	}
> +
> +out:
> +	ACPI_FREE(out_obj);
> +	return output;
> +
> +err:
> +	ACPI_FREE(out_obj);
> +	return ERR_PTR(rc);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_acpi_evaluate_qtg_dsm, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 318aa051f65a..6426c4c22e28 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -7,6 +7,7 @@
>  #include <linux/libnvdimm.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
> +#include <linux/acpi.h>
>  #include <linux/log2.h>
>  #include <linux/list.h>
>  #include <linux/io.h>
> @@ -793,6 +794,21 @@ static inline struct cxl_dax_region *to_cxl_dax_region(struct device *dev)
>  }
>  #endif
>  
> +struct qtg_dsm_input {
> +	u32 rd_lat;
> +	u32 wr_lat;
> +	u32 rd_bw;
> +	u32 wr_bw;
> +};
> +
> +struct qtg_dsm_output {
> +	int nr;
> +	u16 qtg_ids[];
> +};
> +
> +struct qtg_dsm_output *cxl_acpi_evaluate_qtg_dsm(acpi_handle handle,
> +						 struct qtg_dsm_input *input);
> +
>  /*
>   * Unit test builds overrides this to __weak, find the 'strong' version
>   * of these symbols in tools/testing/cxl/.
> 
>