[PATCH 0/4] acpi: Add CDAT parsing support to ACPI tables code

[PATCH 0/4] acpi: Add CDAT parsing support to ACPI tables code

[Dave Jiang]

Hi Rafael,
I've broken out the "cxl: Add support for QTG ID retrieval for CXL subsystem" [1]
series in order to make it more manageable. Here's the first part of the ACPI
changes. These changes are added to allow reuse of ACPI tables code to parse
the CDAT tables. While CDAT is not part of ACPI, the table structures are similar
to ACPI layouts that the code can be reused with some small modifications.

However, in order to be properly utilized by CXL users, the tables code needs
to be refactored out to be independent of ACPI. For example, a PPC BE host may
have CXL and does not have ACPI support. But it will have CDAT to read from
devices and switches. patch 4/4 included is not APCI, but I have included it as
a reference to this problem. Currently as you can see, I have the cdat code in
CXL as "cxl_core-$(CONFIG_ACPI) += cdat.o". That will not work for a scenario
with the PPC host mentioned above since it won't compile in ACPI support. I'm
looking for guidance and to start the discussion on how we want the table handling
code to be broken out to be independent of CONFIG_ACPI. Thank you!

The whole series is at [2] for convenience.

[1]: https://lore.kernel.org/linux-cxl/168193556660.1178687.15477509915255912089.stgit@djiang5-mobl3/T/#t                                                                                               
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-qtg

---

Dave Jiang (4):
      acpi: tables: Add CDAT table parsing support
      acpi: Add header struct in CDAT subtables
      acpi: fix misnamed define for CDAT DSMAS
      cxl: Add callback to parse the DSMAS subtables from CDAT


 drivers/acpi/tables.c     | 47 +++++++++++++++++++++++++++++++++++++--
 drivers/cxl/core/Makefile |  1 +
 drivers/cxl/core/cdat.c   | 40 +++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h         | 18 +++++++++++++++
 drivers/cxl/port.c        | 22 ++++++++++++++++++
 include/acpi/actbl1.h     | 11 ++++++++-
 include/linux/acpi.h      |  4 ++++
 7 files changed, 140 insertions(+), 3 deletions(-)
 create mode 100644 drivers/cxl/core/cdat.c

--

[PATCH 1/4] acpi: tables: Add CDAT table parsing support

[Dave Jiang]

The CDAT table is very similar to ACPI tables when it comes to sub-table
and entry structures. The helper functions can be also used to parse the
CDAT table. Add support to the helper functions to deal with an external
CDAT table, and also handle the endieness since CDAT can be processed by a
BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse
a CDAT table.

In order to minimize ACPI code changes, __force is being utilized to deal
with the case of a big endien (BE) host parsing a CDAT. All CDAT data
structure variables are being force casted to __leX as appropriate.

Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/tables.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
 include/acpi/actbl1.h |    3 +++
 include/linux/acpi.h  |    4 ++++
 3 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 7b4680da57d7..08486f6df442 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -42,6 +42,7 @@ enum acpi_subtable_type {
 	ACPI_SUBTABLE_HMAT,
 	ACPI_SUBTABLE_PRMT,
 	ACPI_SUBTABLE_CEDT,
+	ACPI_SUBTABLE_CDAT,
 };
 
 struct acpi_subtable_entry {
@@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry)
 		return 0;
 	case ACPI_SUBTABLE_CEDT:
 		return entry->hdr->cedt.type;
+	case ACPI_SUBTABLE_CDAT:
+		return entry->hdr->cdat.type;
 	}
 	return 0;
 }
@@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry)
 		return entry->hdr->prmt.length;
 	case ACPI_SUBTABLE_CEDT:
 		return entry->hdr->cedt.length;
+	case ACPI_SUBTABLE_CDAT:
+		return le16_to_cpu((__force __le16)entry->hdr->cdat.length);
 	}
 	return 0;
 }
@@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
 		return sizeof(entry->hdr->prmt);
 	case ACPI_SUBTABLE_CEDT:
 		return sizeof(entry->hdr->cedt);
+	case ACPI_SUBTABLE_CDAT:
+		return sizeof(entry->hdr->cdat);
 	}
 	return 0;
 }
@@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id)
 		return ACPI_SUBTABLE_PRMT;
 	if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
 		return ACPI_SUBTABLE_CEDT;
+	if (strncmp(id, ACPI_SIG_CDAT, 4) == 0)
+		return ACPI_SUBTABLE_CDAT;
 	return ACPI_SUBTABLE_COMMON;
 }
 
+static unsigned long __init_or_acpilib
+acpi_table_get_length(enum acpi_subtable_type type,
+		      struct acpi_table_header *hdr)
+{
+	if (type == ACPI_SUBTABLE_CDAT)
+		return le32_to_cpu(
+			(__force __le32)((struct acpi_table_cdat *)hdr)->length);
+
+	return hdr->length;
+}
+
 static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc)
 {
 	return proc->handler || proc->handler_arg;
@@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array(
 	int proc_num, unsigned int max_entries)
 {
 	struct acpi_subtable_entry entry;
+	enum acpi_subtable_type type;
 	unsigned long table_end, subtable_len, entry_len;
 	int count = 0;
 	int errs = 0;
 	int i;
 
-	table_end = (unsigned long)table_header + table_header->length;
+	type = acpi_get_subtable_type(id);
+	table_end = (unsigned long)table_header +
+		    acpi_table_get_length(type, table_header);
 
 	/* Parse all entries looking for a match. */
 
-	entry.type = acpi_get_subtable_type(id);
+	entry.type = type;
 	entry.hdr = (union acpi_subtable_headers *)
 	    ((unsigned long)table_header + table_size);
 	subtable_len = acpi_get_subtable_header_length(&entry);
@@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id,
 					    handler, max_entries);
 }
 
+int acpi_table_parse_cdat(enum acpi_cdat_type type,
+			  acpi_tbl_entry_handler_arg handler_arg, void *arg,
+			  struct acpi_table_cdat *table_header)
+{
+	struct acpi_subtable_proc proc = {
+		.id		= type,
+		.handler_arg	= handler_arg,
+		.arg		= arg,
+	};
+
+	if (!table_header)
+		return -EINVAL;
+
+	return acpi_parse_entries_array(ACPI_SIG_CDAT,
+			sizeof(struct acpi_table_cdat),
+			(struct acpi_table_header *)table_header,
+			&proc, 1, 0);
+}
+EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL);
+
 /**
  * acpi_table_parse - find table with @id, run @handler on it
  * @id: table id to find
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 81b9e794424d..3119be093cfe 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -66,6 +66,9 @@
 #define ACPI_SIG_IEIT           "IEIT"
 #endif
 
+/* External to ACPI */
+#define ACPI_SIG_CDAT		"CDAT" /* Coherent Device Attribute Table */
+
 /*
  * All tables must be byte-packed to match the ACPI specification, since
  * the tables are provided by the system BIOS.
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index efff750f326d..4c3dfe7587e9 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -135,6 +135,7 @@ union acpi_subtable_headers {
 	struct acpi_hmat_structure hmat;
 	struct acpi_prmt_module_header prmt;
 	struct acpi_cedt_header cedt;
+	struct acpi_cdat_header cdat;
 };
 
 typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
@@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id,
 
 int acpi_parse_mcfg (struct acpi_table_header *header);
 void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
+int acpi_table_parse_cdat(enum acpi_cdat_type type,
+			  acpi_tbl_entry_handler_arg handler, void *arg,
+			  struct acpi_table_cdat *table_header);
 
 /* the following numa functions are architecture-dependent */
 void acpi_numa_slit_init (struct acpi_table_slit *slit);

[PATCH 2/4] acpi: Add header struct in CDAT subtables

[Dave Jiang]

Add the common header struct in all CDAT subtables. This change
complies with other ACPI sub-tables in the header file. The change
also eases the usage with the helper functions in tables.c.

Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 include/acpi/actbl1.h |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 3119be093cfe..166337b04306 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -350,6 +350,7 @@ enum acpi_cdat_type {
 /* Subtable 0: Device Scoped Memory Affinity Structure (DSMAS) */
 
 struct acpi_cdat_dsmas {
+	struct acpi_cdat_header header;
 	u8 dsmad_handle;
 	u8 flags;
 	u16 reserved;
@@ -364,6 +365,7 @@ struct acpi_cdat_dsmas {
 /* Subtable 1: Device scoped Latency and Bandwidth Information Structure (DSLBIS) */
 
 struct acpi_cdat_dslbis {
+	struct acpi_cdat_header header;
 	u8 handle;
 	u8 flags;		/* If Handle matches a DSMAS handle, the definition of this field matches
 				 * Flags field in HMAT System Locality Latency */
@@ -377,6 +379,7 @@ struct acpi_cdat_dslbis {
 /* Subtable 2: Device Scoped Memory Side Cache Information Structure (DSMSCIS) */
 
 struct acpi_cdat_dsmscis {
+	struct acpi_cdat_header header;
 	u8 dsmas_handle;
 	u8 reserved[3];
 	u64 side_cache_size;
@@ -386,6 +389,7 @@ struct acpi_cdat_dsmscis {
 /* Subtable 3: Device Scoped Initiator Structure (DSIS) */
 
 struct acpi_cdat_dsis {
+	struct acpi_cdat_header header;
 	u8 flags;
 	u8 handle;
 	u16 reserved;
@@ -398,6 +402,7 @@ struct acpi_cdat_dsis {
 /* Subtable 4: Device Scoped EFI Memory Type Structure (DSEMTS) */
 
 struct acpi_cdat_dsemts {
+	struct acpi_cdat_header header;
 	u8 dsmas_handle;
 	u8 memory_type;
 	u16 reserved;
@@ -408,6 +413,7 @@ struct acpi_cdat_dsemts {
 /* Subtable 5: Switch Scoped Latency and Bandwidth Information Structure (SSLBIS) */
 
 struct acpi_cdat_sslbis {
+	struct acpi_cdat_header header;
 	u8 data_type;
 	u8 reserved[3];
 	u64 entry_base_unit;

[PATCH 3/4] acpi: fix misnamed define for CDAT DSMAS

[Dave Jiang]

ACPI_CEDT_DSMAS_NON_VOLATILE should be defined as
ACPI_CDAT_DSMAS_NON_VOLATILE. Fix misspelled define.

Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 include/acpi/actbl1.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 166337b04306..8ea7e5d64bc1 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -360,7 +360,7 @@ struct acpi_cdat_dsmas {
 
 /* Flags for subtable above */
 
-#define ACPI_CEDT_DSMAS_NON_VOLATILE        (1 << 2)
+#define ACPI_CDAT_DSMAS_NON_VOLATILE        (1 << 2)
 
 /* Subtable 1: Device scoped Latency and Bandwidth Information Structure (DSLBIS) */
 

[PATCH 4/4] cxl: Add callback to parse the DSMAS subtables from CDAT

[Dave Jiang]

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@intel.com>
---
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;
+	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);
+	}
+}
+
 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;
 }
 

Re: [PATCH 1/4] acpi: tables: Add CDAT table parsing support

[Jonathan Cameron]

On Fri, 05 May 2023 10:32:56 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> The CDAT table is very similar to ACPI tables when it comes to sub-table
> and entry structures. The helper functions can be also used to parse the
> CDAT table. Add support to the helper functions to deal with an external
> CDAT table, and also handle the endieness since CDAT can be processed by a
> BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse
> a CDAT table.
> 
> In order to minimize ACPI code changes, __force is being utilized to deal
> with the case of a big endien (BE) host parsing a CDAT. All CDAT data
> structure variables are being force casted to __leX as appropriate.

Hi Dave,

This falls into the annoyance that CDAT doesn't have a standard table header.
Whilst I understand that was done deliberately it means some odd things happen
in this code.

Just how bad is the duplication if we don't do this at all, but instead roll
a version for CDAT that doesn't force things through pointers of the wrong types?

Otherwise, maybe we need some unions so that the type mashups don't happen.

> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/acpi/tables.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
>  include/acpi/actbl1.h |    3 +++
>  include/linux/acpi.h  |    4 ++++
>  3 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 7b4680da57d7..08486f6df442 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -42,6 +42,7 @@ enum acpi_subtable_type {
>  	ACPI_SUBTABLE_HMAT,
>  	ACPI_SUBTABLE_PRMT,
>  	ACPI_SUBTABLE_CEDT,
> +	ACPI_SUBTABLE_CDAT,
>  };
>  
>  struct acpi_subtable_entry {
> @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry)
>  		return 0;
>  	case ACPI_SUBTABLE_CEDT:
>  		return entry->hdr->cedt.type;
> +	case ACPI_SUBTABLE_CDAT:
> +		return entry->hdr->cdat.type;
>  	}
>  	return 0;
>  }
> @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry)
>  		return entry->hdr->prmt.length;
>  	case ACPI_SUBTABLE_CEDT:
>  		return entry->hdr->cedt.length;
> +	case ACPI_SUBTABLE_CDAT:
> +		return le16_to_cpu((__force __le16)entry->hdr->cdat.length);
>  	}
>  	return 0;
>  }
> @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
>  		return sizeof(entry->hdr->prmt);
>  	case ACPI_SUBTABLE_CEDT:
>  		return sizeof(entry->hdr->cedt);
> +	case ACPI_SUBTABLE_CDAT:
> +		return sizeof(entry->hdr->cdat);
>  	}
>  	return 0;
>  }
> @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id)
>  		return ACPI_SUBTABLE_PRMT;
>  	if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
>  		return ACPI_SUBTABLE_CEDT;
> +	if (strncmp(id, ACPI_SIG_CDAT, 4) == 0)
> +		return ACPI_SUBTABLE_CDAT;

I'm not super keen on inventing a SIG when the CDAT 'table'
doesn't actually have one.

>  	return ACPI_SUBTABLE_COMMON;
>  }
>  
> +static unsigned long __init_or_acpilib
> +acpi_table_get_length(enum acpi_subtable_type type,
> +		      struct acpi_table_header *hdr)

I don't like parsing in an acpi_table_header type here when it may not be one.
I think this length decision needs to be pushed up a level to where we can see
if we have a CDAT table or not.


> +{
> +	if (type == ACPI_SUBTABLE_CDAT)
> +		return le32_to_cpu(
> +			(__force __le32)((struct acpi_table_cdat *)hdr)->length);

Perhaps a local variable in here somewhere would make it more readable.
	__le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length;

	return le32_to_cpu(length)?


> +
> +	return hdr->length;
> +}
> +
>  static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc)
>  {
>  	return proc->handler || proc->handler_arg;
> @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array(
>  	int proc_num, unsigned int max_entries)
>  {
>  	struct acpi_subtable_entry entry;
> +	enum acpi_subtable_type type;
>  	unsigned long table_end, subtable_len, entry_len;
>  	int count = 0;
>  	int errs = 0;
>  	int i;
>  
> -	table_end = (unsigned long)table_header + table_header->length;
> +	type = acpi_get_subtable_type(id);
> +	table_end = (unsigned long)table_header +
> +		    acpi_table_get_length(type, table_header);
As above, I don't like carrying CDAT which doesn't have an acpi_table_header
section around as that type of pointer.

>  
>  	/* Parse all entries looking for a match. */
>  
> -	entry.type = acpi_get_subtable_type(id);
> +	entry.type = type;
>  	entry.hdr = (union acpi_subtable_headers *)
>  	    ((unsigned long)table_header + table_size);
>  	subtable_len = acpi_get_subtable_header_length(&entry);
> @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id,
>  					    handler, max_entries);
>  }
>  
> +int acpi_table_parse_cdat(enum acpi_cdat_type type,
> +			  acpi_tbl_entry_handler_arg handler_arg, void *arg,
> +			  struct acpi_table_cdat *table_header)
> +{
> +	struct acpi_subtable_proc proc = {
> +		.id		= type,
> +		.handler_arg	= handler_arg,
> +		.arg		= arg,
> +	};
> +
> +	if (!table_header)
> +		return -EINVAL;
> +
> +	return acpi_parse_entries_array(ACPI_SIG_CDAT,
> +			sizeof(struct acpi_table_cdat),
> +			(struct acpi_table_header *)table_header,
> +			&proc, 1, 0);
> +}
> +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL);
> +
>  /**
>   * acpi_table_parse - find table with @id, run @handler on it
>   * @id: table id to find
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index 81b9e794424d..3119be093cfe 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -66,6 +66,9 @@
>  #define ACPI_SIG_IEIT           "IEIT"
>  #endif
>  
> +/* External to ACPI */
> +#define ACPI_SIG_CDAT		"CDAT" /* Coherent Device Attribute Table */

Worse that that, fictional signature :)
It's the nameof the 'table', but it's not a signature as it's never
used as they are in ACPI and doesn't appear anywhere in the table.

> +
>  /*
>   * All tables must be byte-packed to match the ACPI specification, since
>   * the tables are provided by the system BIOS.
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index efff750f326d..4c3dfe7587e9 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -135,6 +135,7 @@ union acpi_subtable_headers {
>  	struct acpi_hmat_structure hmat;
>  	struct acpi_prmt_module_header prmt;
>  	struct acpi_cedt_header cedt;
> +	struct acpi_cdat_header cdat;
>  };
>  
>  typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
> @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id,
>  
>  int acpi_parse_mcfg (struct acpi_table_header *header);
>  void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
> +int acpi_table_parse_cdat(enum acpi_cdat_type type,
> +			  acpi_tbl_entry_handler_arg handler, void *arg,
> +			  struct acpi_table_cdat *table_header);
How did we end up with an 'acpi_' table that isn't in ACPI?
(I'm not looking as I fear I might be responsible :)
Should perhaps consider renaming all the CDAT entries so it doesn't looks like they
are.

>  
>  /* the following numa functions are architecture-dependent */
>  void acpi_numa_slit_init (struct acpi_table_slit *slit);
> 
> 
> 

Re: [PATCH 2/4] acpi: Add header struct in CDAT subtables

[Jonathan Cameron]

On Fri, 05 May 2023 10:33:02 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add the common header struct in all CDAT subtables. This change
> complies with other ACPI sub-tables in the header file. The change
> also eases the usage with the helper functions in tables.c.
> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Other than the naming question from previous patch (should these have acpi
in their names at all?), this looks sensible to me.

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

> ---
>  include/acpi/actbl1.h |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index 3119be093cfe..166337b04306 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -350,6 +350,7 @@ enum acpi_cdat_type {
>  /* Subtable 0: Device Scoped Memory Affinity Structure (DSMAS) */
>  
>  struct acpi_cdat_dsmas {
> +	struct acpi_cdat_header header;
>  	u8 dsmad_handle;
>  	u8 flags;
>  	u16 reserved;
> @@ -364,6 +365,7 @@ struct acpi_cdat_dsmas {
>  /* Subtable 1: Device scoped Latency and Bandwidth Information Structure (DSLBIS) */
>  
>  struct acpi_cdat_dslbis {
> +	struct acpi_cdat_header header;
>  	u8 handle;
>  	u8 flags;		/* If Handle matches a DSMAS handle, the definition of this field matches
>  				 * Flags field in HMAT System Locality Latency */
> @@ -377,6 +379,7 @@ struct acpi_cdat_dslbis {
>  /* Subtable 2: Device Scoped Memory Side Cache Information Structure (DSMSCIS) */
>  
>  struct acpi_cdat_dsmscis {
> +	struct acpi_cdat_header header;
>  	u8 dsmas_handle;
>  	u8 reserved[3];
>  	u64 side_cache_size;
> @@ -386,6 +389,7 @@ struct acpi_cdat_dsmscis {
>  /* Subtable 3: Device Scoped Initiator Structure (DSIS) */
>  
>  struct acpi_cdat_dsis {
> +	struct acpi_cdat_header header;
>  	u8 flags;
>  	u8 handle;
>  	u16 reserved;
> @@ -398,6 +402,7 @@ struct acpi_cdat_dsis {
>  /* Subtable 4: Device Scoped EFI Memory Type Structure (DSEMTS) */
>  
>  struct acpi_cdat_dsemts {
> +	struct acpi_cdat_header header;
>  	u8 dsmas_handle;
>  	u8 memory_type;
>  	u16 reserved;
> @@ -408,6 +413,7 @@ struct acpi_cdat_dsemts {
>  /* Subtable 5: Switch Scoped Latency and Bandwidth Information Structure (SSLBIS) */
>  
>  struct acpi_cdat_sslbis {
> +	struct acpi_cdat_header header;
>  	u8 data_type;
>  	u8 reserved[3];
>  	u64 entry_base_unit;
> 
> 
> 

Re: [PATCH 3/4] acpi: fix misnamed define for CDAT DSMAS

[Jonathan Cameron]

On Fri, 05 May 2023 10:33:08 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> ACPI_CEDT_DSMAS_NON_VOLATILE should be defined as
> ACPI_CDAT_DSMAS_NON_VOLATILE. Fix misspelled define.
> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Len Brown <lenb@kernel.org>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

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

> ---
>  include/acpi/actbl1.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index 166337b04306..8ea7e5d64bc1 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -360,7 +360,7 @@ struct acpi_cdat_dsmas {
>  
>  /* Flags for subtable above */
>  
> -#define ACPI_CEDT_DSMAS_NON_VOLATILE        (1 << 2)
> +#define ACPI_CDAT_DSMAS_NON_VOLATILE        (1 << 2)
>  
>  /* Subtable 1: Device scoped Latency and Bandwidth Information Structure (DSLBIS) */
>  
> 
> 
> 

Re: [PATCH 4/4] cxl: Add callback to parse the DSMAS subtables from CDAT

[Jonathan Cameron]

On Fri, 05 May 2023 10:33:14 -0700
Dave Jiang <dave.jiang@intel.com> 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@intel.com>
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;
>  }
>  
> 
> 
> 

Re: [PATCH 1/4] acpi: tables: Add CDAT table parsing support

[Dave Jiang]

On 5/12/23 4:58 AM, Jonathan Cameron wrote:
> On Fri, 05 May 2023 10:32:56 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> The CDAT table is very similar to ACPI tables when it comes to sub-table
>> and entry structures. The helper functions can be also used to parse the
>> CDAT table. Add support to the helper functions to deal with an external
>> CDAT table, and also handle the endieness since CDAT can be processed by a
>> BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse
>> a CDAT table.
>>
>> In order to minimize ACPI code changes, __force is being utilized to deal
>> with the case of a big endien (BE) host parsing a CDAT. All CDAT data
>> structure variables are being force casted to __leX as appropriate.
> 
> Hi Dave,
> 
> This falls into the annoyance that CDAT doesn't have a standard table header.
> Whilst I understand that was done deliberately it means some odd things happen
> in this code.
> 
> Just how bad is the duplication if we don't do this at all, but instead roll
> a version for CDAT that doesn't force things through pointers of the wrong types?

130 lines I believe.
https://lore.kernel.org/linux-cxl/168193568543.1178687.3067575213689202382.stgit@djiang5-mobl3/

Dan, can we please make a decision on which way we want to go with this?

> 
> Otherwise, maybe we need some unions so that the type mashups don't happen.
> 
>>
>> Cc: Rafael J. Wysocki <rafael@kernel.org>
>> Cc: Len Brown <lenb@kernel.org>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/acpi/tables.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
>>   include/acpi/actbl1.h |    3 +++
>>   include/linux/acpi.h  |    4 ++++
>>   3 files changed, 52 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 7b4680da57d7..08486f6df442 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -42,6 +42,7 @@ enum acpi_subtable_type {
>>   	ACPI_SUBTABLE_HMAT,
>>   	ACPI_SUBTABLE_PRMT,
>>   	ACPI_SUBTABLE_CEDT,
>> +	ACPI_SUBTABLE_CDAT,
>>   };
>>   
>>   struct acpi_subtable_entry {
>> @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry)
>>   		return 0;
>>   	case ACPI_SUBTABLE_CEDT:
>>   		return entry->hdr->cedt.type;
>> +	case ACPI_SUBTABLE_CDAT:
>> +		return entry->hdr->cdat.type;
>>   	}
>>   	return 0;
>>   }
>> @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry)
>>   		return entry->hdr->prmt.length;
>>   	case ACPI_SUBTABLE_CEDT:
>>   		return entry->hdr->cedt.length;
>> +	case ACPI_SUBTABLE_CDAT:
>> +		return le16_to_cpu((__force __le16)entry->hdr->cdat.length);
>>   	}
>>   	return 0;
>>   }
>> @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
>>   		return sizeof(entry->hdr->prmt);
>>   	case ACPI_SUBTABLE_CEDT:
>>   		return sizeof(entry->hdr->cedt);
>> +	case ACPI_SUBTABLE_CDAT:
>> +		return sizeof(entry->hdr->cdat);
>>   	}
>>   	return 0;
>>   }
>> @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id)
>>   		return ACPI_SUBTABLE_PRMT;
>>   	if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
>>   		return ACPI_SUBTABLE_CEDT;
>> +	if (strncmp(id, ACPI_SIG_CDAT, 4) == 0)
>> +		return ACPI_SUBTABLE_CDAT;
> 
> I'm not super keen on inventing a SIG when the CDAT 'table'
> doesn't actually have one.

I wasn't sure how else to deal with this. Additional parameter to 
indicate it's CDAT?

> 
>>   	return ACPI_SUBTABLE_COMMON;
>>   }
>>   
>> +static unsigned long __init_or_acpilib
>> +acpi_table_get_length(enum acpi_subtable_type type,
>> +		      struct acpi_table_header *hdr)
> 
> I don't like parsing in an acpi_table_header type here when it may not be one.
> I think this length decision needs to be pushed up a level to where we can see
> if we have a CDAT table or not.

You mean have the caller pass in the table size directly?

> 
> 
>> +{
>> +	if (type == ACPI_SUBTABLE_CDAT)
>> +		return le32_to_cpu(
>> +			(__force __le32)((struct acpi_table_cdat *)hdr)->length);
> 
> Perhaps a local variable in here somewhere would make it more readable.
> 	__le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length;
> 
> 	return le32_to_cpu(length)?
> 
> 
>> +
>> +	return hdr->length;
>> +}
>> +
>>   static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc)
>>   {
>>   	return proc->handler || proc->handler_arg;
>> @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array(
>>   	int proc_num, unsigned int max_entries)
>>   {
>>   	struct acpi_subtable_entry entry;
>> +	enum acpi_subtable_type type;
>>   	unsigned long table_end, subtable_len, entry_len;
>>   	int count = 0;
>>   	int errs = 0;
>>   	int i;
>>   
>> -	table_end = (unsigned long)table_header + table_header->length;
>> +	type = acpi_get_subtable_type(id);
>> +	table_end = (unsigned long)table_header +
>> +		    acpi_table_get_length(type, table_header);
> As above, I don't like carrying CDAT which doesn't have an acpi_table_header
> section around as that type of pointer.
> 
>>   
>>   	/* Parse all entries looking for a match. */
>>   
>> -	entry.type = acpi_get_subtable_type(id);
>> +	entry.type = type;
>>   	entry.hdr = (union acpi_subtable_headers *)
>>   	    ((unsigned long)table_header + table_size);
>>   	subtable_len = acpi_get_subtable_header_length(&entry);
>> @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id,
>>   					    handler, max_entries);
>>   }
>>   
>> +int acpi_table_parse_cdat(enum acpi_cdat_type type,
>> +			  acpi_tbl_entry_handler_arg handler_arg, void *arg,
>> +			  struct acpi_table_cdat *table_header)
>> +{
>> +	struct acpi_subtable_proc proc = {
>> +		.id		= type,
>> +		.handler_arg	= handler_arg,
>> +		.arg		= arg,
>> +	};
>> +
>> +	if (!table_header)
>> +		return -EINVAL;
>> +
>> +	return acpi_parse_entries_array(ACPI_SIG_CDAT,
>> +			sizeof(struct acpi_table_cdat),
>> +			(struct acpi_table_header *)table_header,
>> +			&proc, 1, 0);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL);
>> +
>>   /**
>>    * acpi_table_parse - find table with @id, run @handler on it
>>    * @id: table id to find
>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
>> index 81b9e794424d..3119be093cfe 100644
>> --- a/include/acpi/actbl1.h
>> +++ b/include/acpi/actbl1.h
>> @@ -66,6 +66,9 @@
>>   #define ACPI_SIG_IEIT           "IEIT"
>>   #endif
>>   
>> +/* External to ACPI */
>> +#define ACPI_SIG_CDAT		"CDAT" /* Coherent Device Attribute Table */
> 
> Worse that that, fictional signature :)
> It's the nameof the 'table', but it's not a signature as it's never
> used as they are in ACPI and doesn't appear anywhere in the table.
> 
>> +
>>   /*
>>    * All tables must be byte-packed to match the ACPI specification, since
>>    * the tables are provided by the system BIOS.
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index efff750f326d..4c3dfe7587e9 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -135,6 +135,7 @@ union acpi_subtable_headers {
>>   	struct acpi_hmat_structure hmat;
>>   	struct acpi_prmt_module_header prmt;
>>   	struct acpi_cedt_header cedt;
>> +	struct acpi_cdat_header cdat;
>>   };
>>   
>>   typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
>> @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id,
>>   
>>   int acpi_parse_mcfg (struct acpi_table_header *header);
>>   void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
>> +int acpi_table_parse_cdat(enum acpi_cdat_type type,
>> +			  acpi_tbl_entry_handler_arg handler, void *arg,
>> +			  struct acpi_table_cdat *table_header);
> How did we end up with an 'acpi_' table that isn't in ACPI?
> (I'm not looking as I fear I might be responsible :)

git blame says Bob. You are off the hook. :)

> Should perhaps consider renaming all the CDAT entries so it doesn't looks like they
> are.

Sure I can do that.

> 
>>   
>>   /* the following numa functions are architecture-dependent */
>>   void acpi_numa_slit_init (struct acpi_table_slit *slit);
>>
>>
>>
> 

Re: [PATCH 1/4] acpi: tables: Add CDAT table parsing support

[Dan Williams]

Jonathan Cameron wrote:
> On Fri, 05 May 2023 10:32:56 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
> > The CDAT table is very similar to ACPI tables when it comes to sub-table
> > and entry structures. The helper functions can be also used to parse the
> > CDAT table. Add support to the helper functions to deal with an external
> > CDAT table, and also handle the endieness since CDAT can be processed by a
> > BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse
> > a CDAT table.
> > 
> > In order to minimize ACPI code changes, __force is being utilized to deal
> > with the case of a big endien (BE) host parsing a CDAT. All CDAT data
> > structure variables are being force casted to __leX as appropriate.
> 
> Hi Dave,
> 
> This falls into the annoyance that CDAT doesn't have a standard table header.
> Whilst I understand that was done deliberately it means some odd things happen
> in this code.
> 
> Just how bad is the duplication if we don't do this at all, but instead roll
> a version for CDAT that doesn't force things through pointers of the wrong types?

Yes, this was the question before sending this out. The savings is on
the order of ~100 lines which is not amazing, but was enough for me to
say lets keep going with this idea.

The other observation is that the ACPICA project is doing something
similar for offering disassembly of CDAT buffers within the existing
ACPICA tooling vs building independent infrastructure. So that was
another weight on the scale with proceeding with the code reuse for me.

The only thing I don't like about the result is still seeing acpi_/ACPI_
prefixes. I think these entry points and symbol names should be
cdat_/CDAT_ where possible, more below.

...and as I read to the end of the feedback on this one it seems you
have the same reaction.

> 
> Otherwise, maybe we need some unions so that the type mashups don't happen.
> 
> > 
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > Cc: Len Brown <lenb@kernel.org>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> >  drivers/acpi/tables.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
> >  include/acpi/actbl1.h |    3 +++
> >  include/linux/acpi.h  |    4 ++++
> >  3 files changed, 52 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> > index 7b4680da57d7..08486f6df442 100644
> > --- a/drivers/acpi/tables.c
> > +++ b/drivers/acpi/tables.c
> > @@ -42,6 +42,7 @@ enum acpi_subtable_type {
> >  	ACPI_SUBTABLE_HMAT,
> >  	ACPI_SUBTABLE_PRMT,
> >  	ACPI_SUBTABLE_CEDT,
> > +	ACPI_SUBTABLE_CDAT,

To your point about ACPI_SIG_CDAT I also think this should be named
differently, like CDAT_SUBTABLE, just to make it clear that this is a
special case and not another ACPI table.

> >  };
> >  
> >  struct acpi_subtable_entry {
> > @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry)
> >  		return 0;
> >  	case ACPI_SUBTABLE_CEDT:
> >  		return entry->hdr->cedt.type;
> > +	case ACPI_SUBTABLE_CDAT:
> > +		return entry->hdr->cdat.type;
> >  	}
> >  	return 0;
> >  }
> > @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry)
> >  		return entry->hdr->prmt.length;
> >  	case ACPI_SUBTABLE_CEDT:
> >  		return entry->hdr->cedt.length;
> > +	case ACPI_SUBTABLE_CDAT:
> > +		return le16_to_cpu((__force __le16)entry->hdr->cdat.length);
> >  	}
> >  	return 0;
> >  }
> > @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
> >  		return sizeof(entry->hdr->prmt);
> >  	case ACPI_SUBTABLE_CEDT:
> >  		return sizeof(entry->hdr->cedt);
> > +	case ACPI_SUBTABLE_CDAT:
> > +		return sizeof(entry->hdr->cdat);
> >  	}
> >  	return 0;
> >  }
> > @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id)
> >  		return ACPI_SUBTABLE_PRMT;
> >  	if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
> >  		return ACPI_SUBTABLE_CEDT;
> > +	if (strncmp(id, ACPI_SIG_CDAT, 4) == 0)
> > +		return ACPI_SUBTABLE_CDAT;
> 
> I'm not super keen on inventing a SIG when the CDAT 'table'
> doesn't actually have one.

Agree, I think CDAT_SIG makes it clearer that CDAT is not in the
traditional ACPI namespace.

> 
> >  	return ACPI_SUBTABLE_COMMON;
> >  }
> >  
> > +static unsigned long __init_or_acpilib
> > +acpi_table_get_length(enum acpi_subtable_type type,
> > +		      struct acpi_table_header *hdr)
> 
> I don't like parsing in an acpi_table_header type here when it may not be one.
> I think this length decision needs to be pushed up a level to where we can see
> if we have a CDAT table or not.
> 
> 
> > +{
> > +	if (type == ACPI_SUBTABLE_CDAT)
> > +		return le32_to_cpu(
> > +			(__force __le32)((struct acpi_table_cdat *)hdr)->length);
> 
> Perhaps a local variable in here somewhere would make it more readable.
> 	__le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length;
> 
> 	return le32_to_cpu(length)?
> 
> 
> > +
> > +	return hdr->length;
> > +}
> > +
> >  static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc)
> >  {
> >  	return proc->handler || proc->handler_arg;
> > @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array(
> >  	int proc_num, unsigned int max_entries)
> >  {
> >  	struct acpi_subtable_entry entry;
> > +	enum acpi_subtable_type type;
> >  	unsigned long table_end, subtable_len, entry_len;
> >  	int count = 0;
> >  	int errs = 0;
> >  	int i;
> >  
> > -	table_end = (unsigned long)table_header + table_header->length;
> > +	type = acpi_get_subtable_type(id);
> > +	table_end = (unsigned long)table_header +
> > +		    acpi_table_get_length(type, table_header);
> As above, I don't like carrying CDAT which doesn't have an acpi_table_header
> section around as that type of pointer.
> 
> >  
> >  	/* Parse all entries looking for a match. */
> >  
> > -	entry.type = acpi_get_subtable_type(id);
> > +	entry.type = type;
> >  	entry.hdr = (union acpi_subtable_headers *)
> >  	    ((unsigned long)table_header + table_size);
> >  	subtable_len = acpi_get_subtable_header_length(&entry);
> > @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id,
> >  					    handler, max_entries);
> >  }
> >  
> > +int acpi_table_parse_cdat(enum acpi_cdat_type type,
> > +			  acpi_tbl_entry_handler_arg handler_arg, void *arg,
> > +			  struct acpi_table_cdat *table_header)
> > +{
> > +	struct acpi_subtable_proc proc = {
> > +		.id		= type,
> > +		.handler_arg	= handler_arg,
> > +		.arg		= arg,
> > +	};
> > +
> > +	if (!table_header)
> > +		return -EINVAL;
> > +
> > +	return acpi_parse_entries_array(ACPI_SIG_CDAT,
> > +			sizeof(struct acpi_table_cdat),
> > +			(struct acpi_table_header *)table_header,
> > +			&proc, 1, 0);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL);
> > +
> >  /**
> >   * acpi_table_parse - find table with @id, run @handler on it
> >   * @id: table id to find
> > diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> > index 81b9e794424d..3119be093cfe 100644
> > --- a/include/acpi/actbl1.h
> > +++ b/include/acpi/actbl1.h
> > @@ -66,6 +66,9 @@
> >  #define ACPI_SIG_IEIT           "IEIT"
> >  #endif
> >  
> > +/* External to ACPI */
> > +#define ACPI_SIG_CDAT		"CDAT" /* Coherent Device Attribute Table */
> 
> Worse that that, fictional signature :)
> It's the nameof the 'table', but it's not a signature as it's never
> used as they are in ACPI and doesn't appear anywhere in the table.
> 
> > +
> >  /*
> >   * All tables must be byte-packed to match the ACPI specification, since
> >   * the tables are provided by the system BIOS.
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index efff750f326d..4c3dfe7587e9 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -135,6 +135,7 @@ union acpi_subtable_headers {
> >  	struct acpi_hmat_structure hmat;
> >  	struct acpi_prmt_module_header prmt;
> >  	struct acpi_cedt_header cedt;
> > +	struct acpi_cdat_header cdat;
> >  };
> >  
> >  typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
> > @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id,
> >  
> >  int acpi_parse_mcfg (struct acpi_table_header *header);
> >  void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
> > +int acpi_table_parse_cdat(enum acpi_cdat_type type,
> > +			  acpi_tbl_entry_handler_arg handler, void *arg,
> > +			  struct acpi_table_cdat *table_header);
> How did we end up with an 'acpi_' table that isn't in ACPI?
> (I'm not looking as I fear I might be responsible :)
> Should perhaps consider renaming all the CDAT entries so it doesn't looks like they
> are.
> 
> >  
> >  /* the following numa functions are architecture-dependent */
> >  void acpi_numa_slit_init (struct acpi_table_slit *slit);
> > 
> > 
> > 
> 

Re: [PATCH 1/4] acpi: tables: Add CDAT table parsing support

[Jonathan Cameron]

On Fri, 12 May 2023 08:24:21 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 5/12/23 4:58 AM, Jonathan Cameron wrote:
> > On Fri, 05 May 2023 10:32:56 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> >> The CDAT table is very similar to ACPI tables when it comes to sub-table
> >> and entry structures. The helper functions can be also used to parse the
> >> CDAT table. Add support to the helper functions to deal with an external
> >> CDAT table, and also handle the endieness since CDAT can be processed by a
> >> BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse
> >> a CDAT table.
> >>
> >> In order to minimize ACPI code changes, __force is being utilized to deal
> >> with the case of a big endien (BE) host parsing a CDAT. All CDAT data
> >> structure variables are being force casted to __leX as appropriate.  
> > 
> > Hi Dave,
> > 
> > This falls into the annoyance that CDAT doesn't have a standard table header.
> > Whilst I understand that was done deliberately it means some odd things happen
> > in this code.
> > 
> > Just how bad is the duplication if we don't do this at all, but instead roll
> > a version for CDAT that doesn't force things through pointers of the wrong types?  
> 
> 130 lines I believe.
> https://lore.kernel.org/linux-cxl/168193568543.1178687.3067575213689202382.stgit@djiang5-mobl3/
> 
> Dan, can we please make a decision on which way we want to go with this?
> 
> > 
> > Otherwise, maybe we need some unions so that the type mashups don't happen.
> >   
> >>
> >> Cc: Rafael J. Wysocki <rafael@kernel.org>
> >> Cc: Len Brown <lenb@kernel.org>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> >> ---
> >>   drivers/acpi/tables.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
> >>   include/acpi/actbl1.h |    3 +++
> >>   include/linux/acpi.h  |    4 ++++
> >>   3 files changed, 52 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> >> index 7b4680da57d7..08486f6df442 100644
> >> --- a/drivers/acpi/tables.c
> >> +++ b/drivers/acpi/tables.c
> >> @@ -42,6 +42,7 @@ enum acpi_subtable_type {
> >>   	ACPI_SUBTABLE_HMAT,
> >>   	ACPI_SUBTABLE_PRMT,
> >>   	ACPI_SUBTABLE_CEDT,
> >> +	ACPI_SUBTABLE_CDAT,
> >>   };
> >>   
> >>   struct acpi_subtable_entry {
> >> @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry)
> >>   		return 0;
> >>   	case ACPI_SUBTABLE_CEDT:
> >>   		return entry->hdr->cedt.type;
> >> +	case ACPI_SUBTABLE_CDAT:
> >> +		return entry->hdr->cdat.type;
> >>   	}
> >>   	return 0;
> >>   }
> >> @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry)
> >>   		return entry->hdr->prmt.length;
> >>   	case ACPI_SUBTABLE_CEDT:
> >>   		return entry->hdr->cedt.length;
> >> +	case ACPI_SUBTABLE_CDAT:
> >> +		return le16_to_cpu((__force __le16)entry->hdr->cdat.length);
> >>   	}
> >>   	return 0;
> >>   }
> >> @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
> >>   		return sizeof(entry->hdr->prmt);
> >>   	case ACPI_SUBTABLE_CEDT:
> >>   		return sizeof(entry->hdr->cedt);
> >> +	case ACPI_SUBTABLE_CDAT:
> >> +		return sizeof(entry->hdr->cdat);
> >>   	}
> >>   	return 0;
> >>   }
> >> @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id)
> >>   		return ACPI_SUBTABLE_PRMT;
> >>   	if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
> >>   		return ACPI_SUBTABLE_CEDT;
> >> +	if (strncmp(id, ACPI_SIG_CDAT, 4) == 0)
> >> +		return ACPI_SUBTABLE_CDAT;  
> > 
> > I'm not super keen on inventing a SIG when the CDAT 'table'
> > doesn't actually have one.  
> 
> I wasn't sure how else to deal with this. Additional parameter to 
> indicate it's CDAT?

No idea. I poke holes, not provide solutions (well not on Friday at
going home time anyway) :)

> 
> >   
> >>   	return ACPI_SUBTABLE_COMMON;
> >>   }
> >>   
> >> +static unsigned long __init_or_acpilib
> >> +acpi_table_get_length(enum acpi_subtable_type type,
> >> +		      struct acpi_table_header *hdr)  
> > 
> > I don't like parsing in an acpi_table_header type here when it may not be one.
> > I think this length decision needs to be pushed up a level to where we can see
> > if we have a CDAT table or not.  
> 
> You mean have the caller pass in the table size directly?
> 
Ah, I wrongly thought the acpi_table_header was assigned one function up
but nope, its' many layers above. 

union maybe?

> > 
> >   
> >> +{
> >> +	if (type == ACPI_SUBTABLE_CDAT)
> >> +		return le32_to_cpu(
> >> +			(__force __le32)((struct acpi_table_cdat *)hdr)->length);  
> > 
> > Perhaps a local variable in here somewhere would make it more readable.
> > 	__le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length;
> > 
> > 	return le32_to_cpu(length)?
> > 
> >   
> >> +
> >> +	return hdr->length;
> >> +}
> >> +
> >>   static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc)
> >>   {
> >>   	return proc->handler || proc->handler_arg;
> >> @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array(
> >>   	int proc_num, unsigned int max_entries)
> >>   {
> >>   	struct acpi_subtable_entry entry;
> >> +	enum acpi_subtable_type type;
> >>   	unsigned long table_end, subtable_len, entry_len;
> >>   	int count = 0;
> >>   	int errs = 0;
> >>   	int i;
> >>   
> >> -	table_end = (unsigned long)table_header + table_header->length;
> >> +	type = acpi_get_subtable_type(id);
> >> +	table_end = (unsigned long)table_header +
> >> +		    acpi_table_get_length(type, table_header);  
> > As above, I don't like carrying CDAT which doesn't have an acpi_table_header
> > section around as that type of pointer.
> >   
> >>   
> >>   	/* Parse all entries looking for a match. */
> >>   
> >> -	entry.type = acpi_get_subtable_type(id);
> >> +	entry.type = type;
> >>   	entry.hdr = (union acpi_subtable_headers *)
> >>   	    ((unsigned long)table_header + table_size);
> >>   	subtable_len = acpi_get_subtable_header_length(&entry);
> >> @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id,
> >>   					    handler, max_entries);
> >>   }
> >>   
> >> +int acpi_table_parse_cdat(enum acpi_cdat_type type,
> >> +			  acpi_tbl_entry_handler_arg handler_arg, void *arg,
> >> +			  struct acpi_table_cdat *table_header)
> >> +{
> >> +	struct acpi_subtable_proc proc = {
> >> +		.id		= type,
> >> +		.handler_arg	= handler_arg,
> >> +		.arg		= arg,
> >> +	};
> >> +
> >> +	if (!table_header)
> >> +		return -EINVAL;
> >> +
> >> +	return acpi_parse_entries_array(ACPI_SIG_CDAT,
> >> +			sizeof(struct acpi_table_cdat),
> >> +			(struct acpi_table_header *)table_header,
> >> +			&proc, 1, 0);
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL);
> >> +

Re: [PATCH 1/4] acpi: tables: Add CDAT table parsing support

[Jonathan Cameron]

On Fri, 12 May 2023 09:14:15 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Jonathan Cameron wrote:
> > On Fri, 05 May 2023 10:32:56 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> > > The CDAT table is very similar to ACPI tables when it comes to sub-table
> > > and entry structures. The helper functions can be also used to parse the
> > > CDAT table. Add support to the helper functions to deal with an external
> > > CDAT table, and also handle the endieness since CDAT can be processed by a
> > > BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse
> > > a CDAT table.
> > > 
> > > In order to minimize ACPI code changes, __force is being utilized to deal
> > > with the case of a big endien (BE) host parsing a CDAT. All CDAT data
> > > structure variables are being force casted to __leX as appropriate.  
> > 
> > Hi Dave,
> > 
> > This falls into the annoyance that CDAT doesn't have a standard table header.
> > Whilst I understand that was done deliberately it means some odd things happen
> > in this code.
> > 
> > Just how bad is the duplication if we don't do this at all, but instead roll
> > a version for CDAT that doesn't force things through pointers of the wrong types?  
> 
> Yes, this was the question before sending this out. The savings is on
> the order of ~100 lines which is not amazing, but was enough for me to
> say lets keep going with this idea.
> 
> The other observation is that the ACPICA project is doing something
> similar for offering disassembly of CDAT buffers within the existing
> ACPICA tooling vs building independent infrastructure. So that was
> another weight on the scale with proceeding with the code reuse for me.

Great. I'd missed that.  I took a look at that ages ago and decided it
was too hard to solve and ran away / wrote CDAT tables in a hex editor instead
for a bit.

> 
> The only thing I don't like about the result is still seeing acpi_/ACPI_
> prefixes. I think these entry points and symbol names should be
> cdat_/CDAT_ where possible, more below.
> 
> ...and as I read to the end of the feedback on this one it seems you
> have the same reaction.
> 
> > 
> > Otherwise, maybe we need some unions so that the type mashups don't happen.

A union or two would get rid of the type confusion - or at least make it
deliberate at the cost of annoyingly noisy patch or maybe some wrappers.

> >   
> > > 
> > > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > > Cc: Len Brown <lenb@kernel.org>
> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > ---
> > >  drivers/acpi/tables.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
> > >  include/acpi/actbl1.h |    3 +++
> > >  include/linux/acpi.h  |    4 ++++
> > >  3 files changed, 52 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> > > index 7b4680da57d7..08486f6df442 100644
> > > --- a/drivers/acpi/tables.c
> > > +++ b/drivers/acpi/tables.c
> > > @@ -42,6 +42,7 @@ enum acpi_subtable_type {
> > >  	ACPI_SUBTABLE_HMAT,
> > >  	ACPI_SUBTABLE_PRMT,
> > >  	ACPI_SUBTABLE_CEDT,
> > > +	ACPI_SUBTABLE_CDAT,  
> 
> To your point about ACPI_SIG_CDAT I also think this should be named
> differently, like CDAT_SUBTABLE, just to make it clear that this is a
> special case and not another ACPI table.

That works for me.

> 
> > >  };
> > >  
> > >  struct acpi_subtable_entry {
> > > @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry)
> > >  		return 0;
> > >  	case ACPI_SUBTABLE_CEDT:
> > >  		return entry->hdr->cedt.type;
> > > +	case ACPI_SUBTABLE_CDAT:
> > > +		return entry->hdr->cdat.type;
> > >  	}
> > >  	return 0;
> > >  }
> > > @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry)
> > >  		return entry->hdr->prmt.length;
> > >  	case ACPI_SUBTABLE_CEDT:
> > >  		return entry->hdr->cedt.length;
> > > +	case ACPI_SUBTABLE_CDAT:
> > > +		return le16_to_cpu((__force __le16)entry->hdr->cdat.length);
> > >  	}
> > >  	return 0;
> > >  }
> > > @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
> > >  		return sizeof(entry->hdr->prmt);
> > >  	case ACPI_SUBTABLE_CEDT:
> > >  		return sizeof(entry->hdr->cedt);
> > > +	case ACPI_SUBTABLE_CDAT:
> > > +		return sizeof(entry->hdr->cdat);
> > >  	}
> > >  	return 0;
> > >  }
> > > @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id)
> > >  		return ACPI_SUBTABLE_PRMT;
> > >  	if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
> > >  		return ACPI_SUBTABLE_CEDT;
> > > +	if (strncmp(id, ACPI_SIG_CDAT, 4) == 0)
> > > +		return ACPI_SUBTABLE_CDAT;  
> > 
> > I'm not super keen on inventing a SIG when the CDAT 'table'
> > doesn't actually have one.  
> 
> Agree, I think CDAT_SIG makes it clearer that CDAT is not in the
> traditional ACPI namespace.

Agreed. IF it shouts different I'm fine with this.

> 
> >   
> > >  	return ACPI_SUBTABLE_COMMON;
> > >  }
> > >  
> > > +static unsigned long __init_or_acpilib
> > > +acpi_table_get_length(enum acpi_subtable_type type,
> > > +		      struct acpi_table_header *hdr)  
> > 
> > I don't like parsing in an acpi_table_header type here when it may not be one.
> > I think this length decision needs to be pushed up a level to where we can see
> > if we have a CDAT table or not.
This comment was (as Dave pointed out) not very helpful.

Switch struct acpi_table_header for a union of that and
struct acpi_table_cdat (renamed)  and it would make me less
uncomfortable with this



> 

Re: [PATCH 1/4] acpi: tables: Add CDAT table parsing support

[Dave Jiang]

On 5/12/23 9:14 AM, Dan Williams wrote:
> Jonathan Cameron wrote:
>> On Fri, 05 May 2023 10:32:56 -0700
>> Dave Jiang <dave.jiang@intel.com> wrote:
>>
>>> The CDAT table is very similar to ACPI tables when it comes to sub-table
>>> and entry structures. The helper functions can be also used to parse the
>>> CDAT table. Add support to the helper functions to deal with an external
>>> CDAT table, and also handle the endieness since CDAT can be processed by a
>>> BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse
>>> a CDAT table.
>>>
>>> In order to minimize ACPI code changes, __force is being utilized to deal
>>> with the case of a big endien (BE) host parsing a CDAT. All CDAT data
>>> structure variables are being force casted to __leX as appropriate.
>>
>> Hi Dave,
>>
>> This falls into the annoyance that CDAT doesn't have a standard table header.
>> Whilst I understand that was done deliberately it means some odd things happen
>> in this code.
>>
>> Just how bad is the duplication if we don't do this at all, but instead roll
>> a version for CDAT that doesn't force things through pointers of the wrong types?
> 
> Yes, this was the question before sending this out. The savings is on
> the order of ~100 lines which is not amazing, but was enough for me to
> say lets keep going with this idea.
> 
> The other observation is that the ACPICA project is doing something
> similar for offering disassembly of CDAT buffers within the existing
> ACPICA tooling vs building independent infrastructure. So that was
> another weight on the scale with proceeding with the code reuse for me.
> 
> The only thing I don't like about the result is still seeing acpi_/ACPI_
> prefixes. I think these entry points and symbol names should be
> cdat_/CDAT_ where possible, more below.

The name change results in significant ACPICA code changes. We probably 
want to avoid that.

DJ
> 
> ...and as I read to the end of the feedback on this one it seems you
> have the same reaction.
> 
>>
>> Otherwise, maybe we need some unions so that the type mashups don't happen.
>>
>>>
>>> Cc: Rafael J. Wysocki <rafael@kernel.org>
>>> Cc: Len Brown <lenb@kernel.org>
>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>> ---
>>>   drivers/acpi/tables.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
>>>   include/acpi/actbl1.h |    3 +++
>>>   include/linux/acpi.h  |    4 ++++
>>>   3 files changed, 52 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>>> index 7b4680da57d7..08486f6df442 100644
>>> --- a/drivers/acpi/tables.c
>>> +++ b/drivers/acpi/tables.c
>>> @@ -42,6 +42,7 @@ enum acpi_subtable_type {
>>>   	ACPI_SUBTABLE_HMAT,
>>>   	ACPI_SUBTABLE_PRMT,
>>>   	ACPI_SUBTABLE_CEDT,
>>> +	ACPI_SUBTABLE_CDAT,
> 
> To your point about ACPI_SIG_CDAT I also think this should be named
> differently, like CDAT_SUBTABLE, just to make it clear that this is a
> special case and not another ACPI table.
> 
>>>   };
>>>   
>>>   struct acpi_subtable_entry {
>>> @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry)
>>>   		return 0;
>>>   	case ACPI_SUBTABLE_CEDT:
>>>   		return entry->hdr->cedt.type;
>>> +	case ACPI_SUBTABLE_CDAT:
>>> +		return entry->hdr->cdat.type;
>>>   	}
>>>   	return 0;
>>>   }
>>> @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry)
>>>   		return entry->hdr->prmt.length;
>>>   	case ACPI_SUBTABLE_CEDT:
>>>   		return entry->hdr->cedt.length;
>>> +	case ACPI_SUBTABLE_CDAT:
>>> +		return le16_to_cpu((__force __le16)entry->hdr->cdat.length);
>>>   	}
>>>   	return 0;
>>>   }
>>> @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
>>>   		return sizeof(entry->hdr->prmt);
>>>   	case ACPI_SUBTABLE_CEDT:
>>>   		return sizeof(entry->hdr->cedt);
>>> +	case ACPI_SUBTABLE_CDAT:
>>> +		return sizeof(entry->hdr->cdat);
>>>   	}
>>>   	return 0;
>>>   }
>>> @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id)
>>>   		return ACPI_SUBTABLE_PRMT;
>>>   	if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
>>>   		return ACPI_SUBTABLE_CEDT;
>>> +	if (strncmp(id, ACPI_SIG_CDAT, 4) == 0)
>>> +		return ACPI_SUBTABLE_CDAT;
>>
>> I'm not super keen on inventing a SIG when the CDAT 'table'
>> doesn't actually have one.
> 
> Agree, I think CDAT_SIG makes it clearer that CDAT is not in the
> traditional ACPI namespace.
> 
>>
>>>   	return ACPI_SUBTABLE_COMMON;
>>>   }
>>>   
>>> +static unsigned long __init_or_acpilib
>>> +acpi_table_get_length(enum acpi_subtable_type type,
>>> +		      struct acpi_table_header *hdr)
>>
>> I don't like parsing in an acpi_table_header type here when it may not be one.
>> I think this length decision needs to be pushed up a level to where we can see
>> if we have a CDAT table or not.
>>
>>
>>> +{
>>> +	if (type == ACPI_SUBTABLE_CDAT)
>>> +		return le32_to_cpu(
>>> +			(__force __le32)((struct acpi_table_cdat *)hdr)->length);
>>
>> Perhaps a local variable in here somewhere would make it more readable.
>> 	__le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length;
>>
>> 	return le32_to_cpu(length)?
>>
>>
>>> +
>>> +	return hdr->length;
>>> +}
>>> +
>>>   static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc)
>>>   {
>>>   	return proc->handler || proc->handler_arg;
>>> @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array(
>>>   	int proc_num, unsigned int max_entries)
>>>   {
>>>   	struct acpi_subtable_entry entry;
>>> +	enum acpi_subtable_type type;
>>>   	unsigned long table_end, subtable_len, entry_len;
>>>   	int count = 0;
>>>   	int errs = 0;
>>>   	int i;
>>>   
>>> -	table_end = (unsigned long)table_header + table_header->length;
>>> +	type = acpi_get_subtable_type(id);
>>> +	table_end = (unsigned long)table_header +
>>> +		    acpi_table_get_length(type, table_header);
>> As above, I don't like carrying CDAT which doesn't have an acpi_table_header
>> section around as that type of pointer.
>>
>>>   
>>>   	/* Parse all entries looking for a match. */
>>>   
>>> -	entry.type = acpi_get_subtable_type(id);
>>> +	entry.type = type;
>>>   	entry.hdr = (union acpi_subtable_headers *)
>>>   	    ((unsigned long)table_header + table_size);
>>>   	subtable_len = acpi_get_subtable_header_length(&entry);
>>> @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id,
>>>   					    handler, max_entries);
>>>   }
>>>   
>>> +int acpi_table_parse_cdat(enum acpi_cdat_type type,
>>> +			  acpi_tbl_entry_handler_arg handler_arg, void *arg,
>>> +			  struct acpi_table_cdat *table_header)
>>> +{
>>> +	struct acpi_subtable_proc proc = {
>>> +		.id		= type,
>>> +		.handler_arg	= handler_arg,
>>> +		.arg		= arg,
>>> +	};
>>> +
>>> +	if (!table_header)
>>> +		return -EINVAL;
>>> +
>>> +	return acpi_parse_entries_array(ACPI_SIG_CDAT,
>>> +			sizeof(struct acpi_table_cdat),
>>> +			(struct acpi_table_header *)table_header,
>>> +			&proc, 1, 0);
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL);
>>> +
>>>   /**
>>>    * acpi_table_parse - find table with @id, run @handler on it
>>>    * @id: table id to find
>>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
>>> index 81b9e794424d..3119be093cfe 100644
>>> --- a/include/acpi/actbl1.h
>>> +++ b/include/acpi/actbl1.h
>>> @@ -66,6 +66,9 @@
>>>   #define ACPI_SIG_IEIT           "IEIT"
>>>   #endif
>>>   
>>> +/* External to ACPI */
>>> +#define ACPI_SIG_CDAT		"CDAT" /* Coherent Device Attribute Table */
>>
>> Worse that that, fictional signature :)
>> It's the nameof the 'table', but it's not a signature as it's never
>> used as they are in ACPI and doesn't appear anywhere in the table.
>>
>>> +
>>>   /*
>>>    * All tables must be byte-packed to match the ACPI specification, since
>>>    * the tables are provided by the system BIOS.
>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>>> index efff750f326d..4c3dfe7587e9 100644
>>> --- a/include/linux/acpi.h
>>> +++ b/include/linux/acpi.h
>>> @@ -135,6 +135,7 @@ union acpi_subtable_headers {
>>>   	struct acpi_hmat_structure hmat;
>>>   	struct acpi_prmt_module_header prmt;
>>>   	struct acpi_cedt_header cedt;
>>> +	struct acpi_cdat_header cdat;
>>>   };
>>>   
>>>   typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
>>> @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id,
>>>   
>>>   int acpi_parse_mcfg (struct acpi_table_header *header);
>>>   void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
>>> +int acpi_table_parse_cdat(enum acpi_cdat_type type,
>>> +			  acpi_tbl_entry_handler_arg handler, void *arg,
>>> +			  struct acpi_table_cdat *table_header);
>> How did we end up with an 'acpi_' table that isn't in ACPI?
>> (I'm not looking as I fear I might be responsible :)
>> Should perhaps consider renaming all the CDAT entries so it doesn't looks like they
>> are.
>>
>>>   
>>>   /* the following numa functions are architecture-dependent */
>>>   void acpi_numa_slit_init (struct acpi_table_slit *slit);
>>>
>>>
>>>
>>
> 
> 

Re: [PATCH 1/4] acpi: tables: Add CDAT table parsing support

[Dave Jiang]

On 5/12/23 4:58 AM, Jonathan Cameron wrote:
> On Fri, 05 May 2023 10:32:56 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> The CDAT table is very similar to ACPI tables when it comes to sub-table
>> and entry structures. The helper functions can be also used to parse the
>> CDAT table. Add support to the helper functions to deal with an external
>> CDAT table, and also handle the endieness since CDAT can be processed by a
>> BE host. Export a function acpi_table_parse_cdat() for CXL driver to parse
>> a CDAT table.
>>
>> In order to minimize ACPI code changes, __force is being utilized to deal
>> with the case of a big endien (BE) host parsing a CDAT. All CDAT data
>> structure variables are being force casted to __leX as appropriate.
> 
> Hi Dave,
> 
> This falls into the annoyance that CDAT doesn't have a standard table header.
> Whilst I understand that was done deliberately it means some odd things happen
> in this code.
> 
> Just how bad is the duplication if we don't do this at all, but instead roll
> a version for CDAT that doesn't force things through pointers of the wrong types?
> 
> Otherwise, maybe we need some unions so that the type mashups don't happen.
> 
>>
>> Cc: Rafael J. Wysocki <rafael@kernel.org>
>> Cc: Len Brown <lenb@kernel.org>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/acpi/tables.c |   47 +++++++++++++++++++++++++++++++++++++++++++++--
>>   include/acpi/actbl1.h |    3 +++
>>   include/linux/acpi.h  |    4 ++++
>>   3 files changed, 52 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
>> index 7b4680da57d7..08486f6df442 100644
>> --- a/drivers/acpi/tables.c
>> +++ b/drivers/acpi/tables.c
>> @@ -42,6 +42,7 @@ enum acpi_subtable_type {
>>   	ACPI_SUBTABLE_HMAT,
>>   	ACPI_SUBTABLE_PRMT,
>>   	ACPI_SUBTABLE_CEDT,
>> +	ACPI_SUBTABLE_CDAT,
>>   };
>>   
>>   struct acpi_subtable_entry {
>> @@ -239,6 +240,8 @@ acpi_get_entry_type(struct acpi_subtable_entry *entry)
>>   		return 0;
>>   	case ACPI_SUBTABLE_CEDT:
>>   		return entry->hdr->cedt.type;
>> +	case ACPI_SUBTABLE_CDAT:
>> +		return entry->hdr->cdat.type;
>>   	}
>>   	return 0;
>>   }
>> @@ -255,6 +258,8 @@ acpi_get_entry_length(struct acpi_subtable_entry *entry)
>>   		return entry->hdr->prmt.length;
>>   	case ACPI_SUBTABLE_CEDT:
>>   		return entry->hdr->cedt.length;
>> +	case ACPI_SUBTABLE_CDAT:
>> +		return le16_to_cpu((__force __le16)entry->hdr->cdat.length);
>>   	}
>>   	return 0;
>>   }
>> @@ -271,6 +276,8 @@ acpi_get_subtable_header_length(struct acpi_subtable_entry *entry)
>>   		return sizeof(entry->hdr->prmt);
>>   	case ACPI_SUBTABLE_CEDT:
>>   		return sizeof(entry->hdr->cedt);
>> +	case ACPI_SUBTABLE_CDAT:
>> +		return sizeof(entry->hdr->cdat);
>>   	}
>>   	return 0;
>>   }
>> @@ -284,9 +291,22 @@ acpi_get_subtable_type(char *id)
>>   		return ACPI_SUBTABLE_PRMT;
>>   	if (strncmp(id, ACPI_SIG_CEDT, 4) == 0)
>>   		return ACPI_SUBTABLE_CEDT;
>> +	if (strncmp(id, ACPI_SIG_CDAT, 4) == 0)
>> +		return ACPI_SUBTABLE_CDAT;
> 
> I'm not super keen on inventing a SIG when the CDAT 'table'
> doesn't actually have one.
> 
>>   	return ACPI_SUBTABLE_COMMON;
>>   }
>>   
>> +static unsigned long __init_or_acpilib
>> +acpi_table_get_length(enum acpi_subtable_type type,
>> +		      struct acpi_table_header *hdr)
> 
> I don't like parsing in an acpi_table_header type here when it may not be one.
> I think this length decision needs to be pushed up a level to where we can see
> if we have a CDAT table or not.
> 
> 
>> +{
>> +	if (type == ACPI_SUBTABLE_CDAT)
>> +		return le32_to_cpu(
>> +			(__force __le32)((struct acpi_table_cdat *)hdr)->length);
> 
> Perhaps a local variable in here somewhere would make it more readable.
> 	__le32 length = (__force__le32)((struct acpi_table_cdat *)hdr)->length;
> 
> 	return le32_to_cpu(length)?
> 
> 
>> +
>> +	return hdr->length;
>> +}
>> +
>>   static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc)
>>   {
>>   	return proc->handler || proc->handler_arg;
>> @@ -332,16 +352,19 @@ static int __init_or_acpilib acpi_parse_entries_array(
>>   	int proc_num, unsigned int max_entries)
>>   {
>>   	struct acpi_subtable_entry entry;
>> +	enum acpi_subtable_type type;
>>   	unsigned long table_end, subtable_len, entry_len;
>>   	int count = 0;
>>   	int errs = 0;
>>   	int i;
>>   
>> -	table_end = (unsigned long)table_header + table_header->length;
>> +	type = acpi_get_subtable_type(id);
>> +	table_end = (unsigned long)table_header +
>> +		    acpi_table_get_length(type, table_header);
> As above, I don't like carrying CDAT which doesn't have an acpi_table_header
> section around as that type of pointer.
> 
>>   
>>   	/* Parse all entries looking for a match. */
>>   
>> -	entry.type = acpi_get_subtable_type(id);
>> +	entry.type = type;
>>   	entry.hdr = (union acpi_subtable_headers *)
>>   	    ((unsigned long)table_header + table_size);
>>   	subtable_len = acpi_get_subtable_header_length(&entry);
>> @@ -464,6 +487,26 @@ int __init acpi_table_parse_madt(enum acpi_madt_type id,
>>   					    handler, max_entries);
>>   }
>>   
>> +int acpi_table_parse_cdat(enum acpi_cdat_type type,
>> +			  acpi_tbl_entry_handler_arg handler_arg, void *arg,
>> +			  struct acpi_table_cdat *table_header)
>> +{
>> +	struct acpi_subtable_proc proc = {
>> +		.id		= type,
>> +		.handler_arg	= handler_arg,
>> +		.arg		= arg,
>> +	};
>> +
>> +	if (!table_header)
>> +		return -EINVAL;
>> +
>> +	return acpi_parse_entries_array(ACPI_SIG_CDAT,
>> +			sizeof(struct acpi_table_cdat),
>> +			(struct acpi_table_header *)table_header,
>> +			&proc, 1, 0);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(acpi_table_parse_cdat, CXL);
>> +
>>   /**
>>    * acpi_table_parse - find table with @id, run @handler on it
>>    * @id: table id to find
>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
>> index 81b9e794424d..3119be093cfe 100644
>> --- a/include/acpi/actbl1.h
>> +++ b/include/acpi/actbl1.h
>> @@ -66,6 +66,9 @@
>>   #define ACPI_SIG_IEIT           "IEIT"
>>   #endif
>>   
>> +/* External to ACPI */
>> +#define ACPI_SIG_CDAT		"CDAT" /* Coherent Device Attribute Table */
> 
> Worse that that, fictional signature :)
> It's the nameof the 'table', but it's not a signature as it's never
> used as they are in ACPI and doesn't appear anywhere in the table.

This is not necessary as include/acpi/actbl2.h already defines it. But 
looks like we are stuck with using ACPI_SIG_CDAT. I'm really trying to 
avoid touching lots of ACPICA code if possible.

> 
>> +
>>   /*
>>    * All tables must be byte-packed to match the ACPI specification, since
>>    * the tables are provided by the system BIOS.
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index efff750f326d..4c3dfe7587e9 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -135,6 +135,7 @@ union acpi_subtable_headers {
>>   	struct acpi_hmat_structure hmat;
>>   	struct acpi_prmt_module_header prmt;
>>   	struct acpi_cedt_header cedt;
>> +	struct acpi_cdat_header cdat;
>>   };
>>   
>>   typedef int (*acpi_tbl_table_handler)(struct acpi_table_header *table);
>> @@ -266,6 +267,9 @@ acpi_table_parse_cedt(enum acpi_cedt_type id,
>>   
>>   int acpi_parse_mcfg (struct acpi_table_header *header);
>>   void acpi_table_print_madt_entry (struct acpi_subtable_header *madt);
>> +int acpi_table_parse_cdat(enum acpi_cdat_type type,
>> +			  acpi_tbl_entry_handler_arg handler, void *arg,
>> +			  struct acpi_table_cdat *table_header);
> How did we end up with an 'acpi_' table that isn't in ACPI?
> (I'm not looking as I fear I might be responsible :)
> Should perhaps consider renaming all the CDAT entries so it doesn't looks like they
> are.
> 
>>   
>>   /* the following numa functions are architecture-dependent */
>>   void acpi_numa_slit_init (struct acpi_table_slit *slit);
>>
>>
>>
>