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

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

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/.
> 
>