On Fri, 05 May 2023 10:33:14 -0700
Dave Jiang <dave.jiang@xxxxxxxxx> wrote:
> Provide a callback function to the CDAT parser in order to parse the Device
> Scoped Memory Affinity Structure (DSMAS). Each DSMAS structure contains the
> DPA range and its associated attributes in each entry. See the CDAT
> specification for details. The device handle and the DPA range is saved and
> to be associated with the DSLBIS locality data when the DSLBIS entries are
> parsed. The list is a local list. When the total path performance data is
> calculated and storred this list can be discarded.
>
> Coherent Device Attribute Table 1.03 2.1 Device Scoped memory Affinity
> Structure (DSMAS)
>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
A few trivials inline.
I've pretty much forgotten this since I last looked, so it's more than
possible I disagree with what I may have said for previous versions.
Jonathan
> ---
> v5:
> - Update commit log to indicate what list is used for. (Jonathan, Dan)
> - Use acpi_table_parse_cdat()
> - Isolate cdat code behind CONFIG_ACPI
> v3:
> - Add spec section number. (Alison)
> - Remove cast from void *. (Alison)
> - Refactor cxl_port_probe() block. (Alison)
> - Move CDAT parse to cxl_endpoint_port_probe()
>
> v2:
> - Add DSMAS table size check. (Lukas)
> - Use local DSMAS header for LE handling.
> - Remove dsmas lock. (Jonathan)
> - Fix handle size (Jonathan)
> - Add LE to host conversion for DSMAS address and length.
> - Make dsmas_list local
> ---
> drivers/cxl/core/Makefile | 1 +
> drivers/cxl/core/cdat.c | 40 ++++++++++++++++++++++++++++++++++++++++
> drivers/cxl/cxl.h | 18 ++++++++++++++++++
> drivers/cxl/port.c | 22 ++++++++++++++++++++++
> 4 files changed, 81 insertions(+)
> create mode 100644 drivers/cxl/core/cdat.c
>
> diff --git a/drivers/cxl/core/Makefile b/drivers/cxl/core/Makefile
> index ca4ae31d8f57..98ddfd110f9b 100644
> --- a/drivers/cxl/core/Makefile
> +++ b/drivers/cxl/core/Makefile
> @@ -12,5 +12,6 @@ cxl_core-y += memdev.o
> cxl_core-y += mbox.o
> cxl_core-y += pci.o
> cxl_core-y += hdm.o
> +cxl_core-$(CONFIG_ACPI) += cdat.o
> cxl_core-$(CONFIG_TRACING) += trace.o
> cxl_core-$(CONFIG_CXL_REGION) += region.o
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> new file mode 100644
> index 000000000000..61979f0789aa
> --- /dev/null
> +++ b/drivers/cxl/core/cdat.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
> +#include <linux/acpi.h>
> +#include "cxlpci.h"
> +#include "cxl.h"
> +
> +static int cdat_dsmas_handler(union acpi_subtable_headers *header, void *arg,
> + const unsigned long end)
> +{
> + struct acpi_cdat_dsmas *dsmas = (struct acpi_cdat_dsmas *)header;
No functional change, but I'd rather see the &header->cdat
cast than the union. Makes it more obvious why this is fine.
> + struct list_head *dsmas_list = arg;
> + struct dsmas_entry *dent;
> + u16 len;
> +
> + len = le16_to_cpu((__force __le16)dsmas->header.length);
> + if (len != sizeof(*dsmas) || (unsigned long)header + len > end) {
> + pr_warn("Malformed DSMAS table length: (%lu:%u)\n",
> + (unsigned long)sizeof(*dsmas), len);
> + return -EINVAL;
> + }
> +
> + dent = kzalloc(sizeof(*dent), GFP_KERNEL);
> + if (!dent)
> + return -ENOMEM;
> +
> + dent->handle = dsmas->dsmad_handle;
> + dent->dpa_range.start = le64_to_cpu((__force __le64)dsmas->dpa_base_address);
> + dent->dpa_range.end = le64_to_cpu((__force __le64)dsmas->dpa_base_address) +
> + le64_to_cpu((__force __le64)dsmas->dpa_length) - 1;
> + list_add_tail(&dent->list, dsmas_list);
> +
> + return 0;
> +}
> +
> +int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list)
> +{
> + return acpi_table_parse_cdat(ACPI_CDAT_TYPE_DSMAS, cdat_dsmas_handler,
> + list, port->cdat.table);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_cdat_endpoint_process, CXL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 4577d808ac6d..dda7238b47f5 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/list.h>
> #include <linux/log2.h>
> #include <linux/io.h>
>
> @@ -791,6 +792,23 @@ static inline struct cxl_dax_region *to_cxl_dax_region(struct device *dev)
> }
> #endif
>
> +/* CDAT related bits */
> +struct dsmas_entry {
> + struct list_head list;
> + struct range dpa_range;
> + u8 handle;
> +};
> +
> +#ifdef CONFIG_ACPI
> +int cxl_cdat_endpoint_process(struct cxl_port *port, struct list_head *list);
> +#else
> +static inline int cxl_cdat_endpoint_process(struct cxl_port *port,
> + struct list_head *list)
> +{
> + return -EOPNOTSUPP;
> +}
> +#endif
> +
> /*
> * Unit test builds overrides this to __weak, find the 'strong' version
> * of these symbols in tools/testing/cxl/.
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index a49f5eb149f1..da023feaa6e2 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -57,6 +57,16 @@ static int discover_region(struct device *dev, void *root)
> return 0;
> }
>
> +static void dsmas_list_destroy(struct list_head *dsmas_list)
> +{
> + struct dsmas_entry *dentry, *n;
> +
> + list_for_each_entry_safe(dentry, n, dsmas_list, list) {
> + list_del(&dentry->list);
> + kfree(dentry);
> + }
> +}
I'd rather see this alongside the code that created it in core/cdat.c
than in here.
> +
> static int cxl_switch_port_probe(struct cxl_port *port)
> {
> struct cxl_hdm *cxlhdm;
> @@ -131,6 +141,18 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
> device_for_each_child(&port->dev, root, discover_region);
> put_device(&root->dev);
>
> + if (port->cdat.table) {
> + LIST_HEAD(dsmas_list);
> +
> + rc = cxl_cdat_endpoint_process(port, &dsmas_list);
> + if (rc < 0)
> + dev_dbg(&port->dev, "Failed to parse CDAT: %d\n", rc);
> +
> + /* Performance data processing */
> +
> + dsmas_list_destroy(&dsmas_list);
> + }
> +
> return 0;
> }
>
>
>
>
