[PATCH v6 0/3] CXL XOR Interleave Arithmetic

[PATCH v6 0/3] CXL XOR Interleave Arithmetic

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Changes in v6:
- Rebased on 6.1-rc4, merging with Dan's latest cxl_test work.
- ACPI patch is now the 'official' linuxized version, not yet merged.

Changes in v5:
- Add to 'n' for 6 & 12 way. (v3->v4 broke it)
- Clean up x3 index init. (Dan)
- Remove unneeded HB's from cxl_test topology.
- Remove dependency on stale patch in cxl_test.

Changes in v4:
- Use GENMASK_ULL to fix i386 arch build (0-day)
- Use do_div to fix ARM arch build (0-day)
- Update comments in ACPICA patch to reflect new state of the
  ACPICA patch - pending again in github.

Changes in v3:
- Fix the 3, 6, 12 way interleave (again).
- Do not look for a CXIMS when not needed for x1 & x3 interleaves
- New cxl_test patch: Add cxl_test module support for this feature
- In a separate ndctl patch, cxl test: cxl_xor_region is added

Changes in v2:
- Use ilog2() of the decoded interleave ways to determine number
  of xormaps, instead of using encoded ways directly. This fixes
  3, 6, and 12 way interleaves. (Dan)

Add support for the new 'XOR' Interleave Arithmetic as defined
in the CXL 3.0 Specification:
https://www.computeexpresslink.org/download-the-specification

Alison Schofield (3):
  ACPICA commit 2d8dc0383d3c908389053afbdc329bbd52f009ce
  cxl/acpi: Support CXL XOR Interleave Math (CXIMS)
  tools/testing/cxl: Add XOR Math support to cxl_test

 drivers/cxl/acpi.c           | 129 +++++++++++++++++++++++++++++++++--
 drivers/cxl/cxl.h            |   2 +
 include/acpi/actbl1.h        |  35 +++++++++-
 tools/testing/cxl/test/cxl.c | 118 +++++++++++++++++++++++++++++++-
 4 files changed, 275 insertions(+), 9 deletions(-)


base-commit: f0c4d9fc9cc9462659728d168387191387e903cc
-- 
2.37.3

[PATCH v6 1/3] ACPICA commit 2d8dc0383d3c908389053afbdc329bbd52f009ce

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

The CXL 3.0 Specification [1] adds two new structures to
the CXL Early Discovery Table (CEDT). The CEDT may include
zero or more entries of these types:

CXIMS: CXL XOR Interleave Math Structure
       Enables the host to find a targets position in an
       Interleave Target List when XOR Math is used.

RDPAS: RCEC Downstream Post Association Structure
       Enables the host to locate the Downstream Port(s)
       that report errors to a given Root Complex Event
       Collector (RCEC).

Link: https://www.computeexpresslink.org/spec-landing # [1]
Link: https://github.com/acpica/acpica/commit/2d8dc038
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Bob Moore <robert.moore@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/acpi/actbl1.h | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index 15c78678c5d3..2aba6f516e70 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -329,7 +329,9 @@ struct acpi_cedt_header {
 enum acpi_cedt_type {
 	ACPI_CEDT_TYPE_CHBS = 0,
 	ACPI_CEDT_TYPE_CFMWS = 1,
-	ACPI_CEDT_TYPE_RESERVED = 2,
+	ACPI_CEDT_TYPE_CXIMS = 2,
+	ACPI_CEDT_TYPE_RDPAS = 3,
+	ACPI_CEDT_TYPE_RESERVED = 4,
 };
 
 /* Values for version field above */
@@ -380,6 +382,7 @@ struct acpi_cedt_cfmws_target_element {
 /* Values for Interleave Arithmetic field above */
 
 #define ACPI_CEDT_CFMWS_ARITHMETIC_MODULO   (0)
+#define ACPI_CEDT_CFMWS_ARITHMETIC_XOR      (1)
 
 /* Values for Restrictions field above */
 
@@ -389,6 +392,36 @@ struct acpi_cedt_cfmws_target_element {
 #define ACPI_CEDT_CFMWS_RESTRICT_PMEM       (1<<3)
 #define ACPI_CEDT_CFMWS_RESTRICT_FIXED      (1<<4)
 
+/* 2: CXL XOR Interleave Math Structure */
+
+struct acpi_cedt_cxims {
+	struct acpi_cedt_header header;
+	u16 reserved1;
+	u8 hbig;
+	u8 nr_xormaps;
+	u64 xormap_list[];
+};
+
+/* 3: CXL RCEC Downstream Port Association Structure */
+
+struct acpi_cedt_rdpas {
+	struct acpi_cedt_header header;
+	u8 reserved1;
+	u16 length;
+	u16 segment;
+	u16 bdf;
+	u8 protocol;
+	u64 address;
+};
+
+/* Masks for bdf field above */
+#define ACPI_CEDT_RDPAS_BUS_MASK            0xff00
+#define ACPI_CEDT_RDPAS_DEVICE_MASK         0x00f8
+#define ACPI_CEDT_RDPAS_FUNCTION_MASK       0x0007
+
+#define ACPI_CEDT_RDPAS_PROTOCOL_IO        (0)
+#define ACPI_CEDT_RDPAS_PROTOCOL_CACHEMEM  (1)
+
 /*******************************************************************************
  *
  * CPEP - Corrected Platform Error Polling table (ACPI 4.0)
-- 
2.37.3

[PATCH v6 2/3] cxl/acpi: Support CXL XOR Interleave Math (CXIMS)

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

When the CFMWS is using XOR math, parse the corresponding
CXIMS structure and store the xormaps in the root decoder
structure. Use the xormaps in a new lookup, cxl_hb_xor(),
to find a targets entry in the host bridge interleave
target list.

Defined in CXL Specfication 3.0 Section: 9.17.1

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/acpi.c | 129 +++++++++++++++++++++++++++++++++++++++++++--
 drivers/cxl/cxl.h  |   2 +
 2 files changed, 126 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index fb649683dd3a..1211c31c29d2 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -6,9 +6,106 @@
 #include <linux/kernel.h>
 #include <linux/acpi.h>
 #include <linux/pci.h>
+#include <asm/div64.h>
 #include "cxlpci.h"
 #include "cxl.h"
 
+struct cxims_data {
+	int nr_maps;
+	u64 xormaps[];
+};
+
+/*
+ * Find a targets entry (n) in the host bridge interleave list.
+ * CXL Specfication 3.0 Table 9-22
+ */
+static struct cxl_dport *cxl_hb_xor(struct cxl_root_decoder *cxlrd, int pos)
+{
+	struct cxl_switch_decoder *cxlsd = &cxlrd->cxlsd;
+	struct cxims_data *cximsd = cxlrd->platform_data;
+	struct cxl_decoder *cxld = &cxlsd->cxld;
+	int ig = cxld->interleave_granularity;
+	int iw = cxld->interleave_ways;
+	int eiw, i = 0, n = 0;
+	u64 hpa;
+
+	if (dev_WARN_ONCE(&cxld->dev,
+			  cxld->interleave_ways != cxlsd->nr_targets,
+			  "misconfigured root decoder\n"))
+		return NULL;
+
+	if (iw == 1)
+		/* Entry is always 0 for no interleave */
+		return cxlrd->cxlsd.target[0];
+
+	hpa = cxlrd->res->start + pos * ig;
+
+	if (iw == 3)
+		goto no_map;
+
+	/* IW: 2,4,6,8,12,16 begin building 'n' using xormaps */
+	for (i = 0; i < cximsd->nr_maps; i++)
+		n |= (hweight64(hpa & cximsd->xormaps[i]) & 1) << i;
+
+no_map:
+	/* IW: 3,6,12 add a modulo calculation to 'n' */
+	if (!is_power_of_2(iw)) {
+		eiw = ilog2(iw / 3) + 8;
+		hpa &= GENMASK_ULL(51, eiw + ig);
+		n |= do_div(hpa, 3) << i;
+	}
+	return cxlrd->cxlsd.target[n];
+}
+
+struct cxl_cxims_context {
+	struct device *dev;
+	struct cxl_root_decoder *cxlrd;
+};
+
+static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg,
+			   const unsigned long end)
+{
+	struct acpi_cedt_cxims *cxims = (struct acpi_cedt_cxims *)header;
+	struct cxl_cxims_context *ctx = arg;
+	struct cxl_root_decoder *cxlrd = ctx->cxlrd;
+	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
+	struct device *dev = ctx->dev;
+	struct cxims_data *cximsd;
+	unsigned int hbig, nr_maps;
+	int rc;
+
+	rc = cxl_to_granularity(cxims->hbig, &hbig);
+	if (rc)
+		return rc;
+
+	if (hbig == cxld->interleave_granularity) {
+		/* IW 1,3 do not use xormaps and skip this parsing entirely */
+
+		if (is_power_of_2(cxld->interleave_ways))
+			/* 2, 4, 8, 16 way */
+			nr_maps = ilog2(cxld->interleave_ways);
+		else
+			/* 6, 12 way */
+			nr_maps = ilog2(cxld->interleave_ways / 3);
+
+		if (cxims->nr_xormaps < nr_maps) {
+			dev_dbg(dev, "CXIMS nr_xormaps[%d] expected[%d]\n",
+				cxims->nr_xormaps, nr_maps);
+			return -ENXIO;
+		}
+
+		cximsd = devm_kzalloc(dev,
+				      struct_size(cximsd, xormaps, nr_maps),
+				      GFP_KERNEL);
+		memcpy(cximsd->xormaps, cxims->xormap_list,
+		       nr_maps * sizeof(*cximsd->xormaps));
+		cximsd->nr_maps = nr_maps;
+		cxlrd->platform_data = cximsd;
+		cxlrd->calc_hb = cxl_hb_xor;
+	}
+	return 0;
+}
+
 static unsigned long cfmws_to_decoder_flags(int restrictions)
 {
 	unsigned long flags = CXL_DECODER_F_ENABLE;
@@ -33,11 +130,6 @@ static int cxl_acpi_cfmws_verify(struct device *dev,
 	int rc, expected_len;
 	unsigned int ways;
 
-	if (cfmws->interleave_arithmetic != ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) {
-		dev_err(dev, "CFMWS Unsupported Interleave Arithmetic\n");
-		return -EINVAL;
-	}
-
 	if (!IS_ALIGNED(cfmws->base_hpa, SZ_256M)) {
 		dev_err(dev, "CFMWS Base HPA not 256MB aligned\n");
 		return -EINVAL;
@@ -84,6 +176,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 	struct cxl_cfmws_context *ctx = arg;
 	struct cxl_port *root_port = ctx->root_port;
 	struct resource *cxl_res = ctx->cxl_res;
+	struct cxl_cxims_context cxims_ctx;
 	struct cxl_root_decoder *cxlrd;
 	struct device *dev = ctx->dev;
 	struct acpi_cedt_cfmws *cfmws;
@@ -148,7 +241,33 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 		ig = CXL_DECODER_MIN_GRANULARITY;
 	cxld->interleave_granularity = ig;
 
+	if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_XOR) {
+		if (ways == 1 || ways == 3)	{
+			/* Skip CXIMS parsing for IW 1 or 3. No xormaps used */
+			cxlrd->calc_hb = cxl_hb_xor;
+			goto decoder_add;
+		}
+
+		cxims_ctx = (struct cxl_cxims_context) {
+			.dev = dev,
+			.cxlrd = cxlrd,
+		};
+
+		rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CXIMS,
+					   cxl_parse_cxims, &cxims_ctx);
+		if (rc < 0)
+			goto err_xormap;
+
+		if (cxlrd->calc_hb != cxl_hb_xor) {
+			rc = -ENXIO;
+			goto err_xormap;
+		}
+	}
+
+decoder_add:
 	rc = cxl_decoder_add(cxld, target_map);
+
+err_xormap:
 	if (rc)
 		put_device(&cxld->dev);
 	else
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index ac75554b5d76..3f97a5e01c12 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -330,12 +330,14 @@ struct cxl_switch_decoder {
  * @res: host / parent resource for region allocations
  * @region_id: region id for next region provisioning event
  * @calc_hb: which host bridge covers the n'th position by granularity
+ * @platform_data: platform specific configuration data
  * @cxlsd: base cxl switch decoder
  */
 struct cxl_root_decoder {
 	struct resource *res;
 	atomic_t region_id;
 	struct cxl_dport *(*calc_hb)(struct cxl_root_decoder *cxlrd, int pos);
+	void *platform_data;
 	struct cxl_switch_decoder cxlsd;
 };
 
-- 
2.37.3

[PATCH v6 3/3] tools/testing/cxl: Add XOR Math support to cxl_test

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Expand the cxl_test topology to include CFMWS's that use XOR math
for interleave arithmetic, as defined in the CXL Specification 3.0.

With this expanded topology, cxl_test is useful for testing:
x1,x2,x4 ways with XOR interleave arithmetic.

Define the additional XOR CFMWS entries to appear only with the
module parameter interleave_arithmetic=1. The cxl_test default
continues to be modulo math.

modprobe cxl_test interleave_arithmetic=1

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 tools/testing/cxl/test/cxl.c | 118 ++++++++++++++++++++++++++++++++++-
 1 file changed, 115 insertions(+), 3 deletions(-)

diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
index 7edce12fd2ce..97849e9753f5 100644
--- a/tools/testing/cxl/test/cxl.c
+++ b/tools/testing/cxl/test/cxl.c
@@ -11,6 +11,8 @@
 #include <cxlmem.h>
 #include "mock.h"
 
+static int interleave_arithmetic;
+
 #define NR_CXL_HOST_BRIDGES 2
 #define NR_CXL_SINGLE_HOST 1
 #define NR_CXL_ROOT_PORTS 2
@@ -122,6 +124,22 @@ static struct {
 		struct acpi_cedt_cfmws cfmws;
 		u32 target[1];
 	} cfmws4;
+	struct {
+		struct acpi_cedt_cfmws cfmws;
+		u32 target[1];
+	} cfmws5;
+	struct {
+		struct acpi_cedt_cfmws cfmws;
+		u32 target[2];
+	} cfmws6;
+	struct {
+		struct acpi_cedt_cfmws cfmws;
+		u32 target[4];
+	} cfmws7;
+	struct {
+		struct acpi_cedt_cxims cxims;
+		u64 xormap_list[2];
+	} cxims0;
 } __packed mock_cedt = {
 	.cedt = {
 		.header = {
@@ -229,14 +247,89 @@ static struct {
 		},
 		.target = { 2 },
 	},
+	/* .cfmws5,6,7 use XOR Math (interleave_arithmetic == 1) */
+	.cfmws5 = {
+		.cfmws = {
+			.header = {
+				.type = ACPI_CEDT_TYPE_CFMWS,
+				.length = sizeof(mock_cedt.cfmws5),
+			},
+			.interleave_arithmetic = 1,
+			.interleave_ways = 0,
+			.granularity = 4,
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+					ACPI_CEDT_CFMWS_RESTRICT_PMEM,
+			.qtg_id = 0,
+			.window_size = SZ_256M * 8UL,
+		},
+		.target = { 0 },
+	},
+	.cfmws6 = {
+		.cfmws = {
+			.header = {
+				.type = ACPI_CEDT_TYPE_CFMWS,
+				.length = sizeof(mock_cedt.cfmws6),
+			},
+			.interleave_arithmetic = 1,
+			.interleave_ways = 1,
+			.granularity = 0,
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+					ACPI_CEDT_CFMWS_RESTRICT_PMEM,
+			.qtg_id = 1,
+			.window_size = SZ_256M * 8UL,
+		},
+		.target = { 0, 1, },
+	},
+	.cfmws7 = {
+		.cfmws = {
+			.header = {
+				.type = ACPI_CEDT_TYPE_CFMWS,
+				.length = sizeof(mock_cedt.cfmws7),
+			},
+			.interleave_arithmetic = 1,
+			.interleave_ways = 2,
+			.granularity = 0,
+			.restrictions = ACPI_CEDT_CFMWS_RESTRICT_TYPE3 |
+					ACPI_CEDT_CFMWS_RESTRICT_PMEM,
+			.qtg_id = 0,
+			.window_size = SZ_256M * 16UL,
+		},
+		.target = { 0, 1, 0, 1, },
+	},
+	.cxims0 = {
+		.cxims = {
+			.header = {
+				.type = ACPI_CEDT_TYPE_CXIMS,
+				.length = sizeof(mock_cedt.cxims0),
+			},
+			.hbig = 0,
+			.nr_xormaps = 2,
+		},
+		.xormap_list = { 0x404100, 0x808200 },
+	},
 };
 
-struct acpi_cedt_cfmws *mock_cfmws[] = {
+struct acpi_cedt_cfmws *mock_cfmws[8] = {
 	[0] = &mock_cedt.cfmws0.cfmws,
 	[1] = &mock_cedt.cfmws1.cfmws,
 	[2] = &mock_cedt.cfmws2.cfmws,
 	[3] = &mock_cedt.cfmws3.cfmws,
 	[4] = &mock_cedt.cfmws4.cfmws,
+	/* Modulo Math above, XOR Math below */
+	[5] = &mock_cedt.cfmws5.cfmws,
+	[6] = &mock_cedt.cfmws6.cfmws,
+	[7] = &mock_cedt.cfmws7.cfmws,
+};
+
+static int cfmws_start;
+static int cfmws_end;
+#define CFMWS_MOD_ARRAY_START 0
+#define CFMWS_MOD_ARRAY_END   4
+#define CFMWS_XOR_ARRAY_START 5
+#define CFMWS_XOR_ARRAY_END   7
+
+struct acpi_cedt_cxims *mock_cxims[1] = {
+	[0] = &mock_cedt.cxims0.cxims,
 };
 
 struct cxl_mock_res {
@@ -308,7 +401,7 @@ static int populate_cedt(void)
 		chbs->length = size;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(mock_cfmws); i++) {
+	for (i = cfmws_start; i <= cfmws_end; i++) {
 		struct acpi_cedt_cfmws *window = mock_cfmws[i];
 
 		res = alloc_mock_res(window->window_size);
@@ -351,12 +444,19 @@ static int mock_acpi_table_parse_cedt(enum acpi_cedt_type id,
 		}
 
 	if (id == ACPI_CEDT_TYPE_CFMWS)
-		for (i = 0; i < ARRAY_SIZE(mock_cfmws); i++) {
+		for (i = cfmws_start; i <= cfmws_end; i++) {
 			h = (union acpi_subtable_headers *) mock_cfmws[i];
 			end = (unsigned long) h + mock_cfmws[i]->header.length;
 			handler_arg(h, arg, end);
 		}
 
+	if (id == ACPI_CEDT_TYPE_CXIMS)
+		for (i = 0; i < ARRAY_SIZE(mock_cxims); i++) {
+			h = (union acpi_subtable_headers *)mock_cxims[i];
+			end = (unsigned long)h + mock_cxims[i]->header.length;
+			handler_arg(h, arg, end);
+		}
+
 	return 0;
 }
 
@@ -897,6 +997,16 @@ static __init int cxl_test_init(void)
 	if (rc)
 		goto err_gen_pool_add;
 
+	if (interleave_arithmetic == 1) {
+		cfmws_start = CFMWS_XOR_ARRAY_START;
+		cfmws_end = CFMWS_XOR_ARRAY_END;
+		dev_dbg(NULL, "cxl_test loading xor math option\n");
+	} else {
+		cfmws_start = CFMWS_MOD_ARRAY_START;
+		cfmws_end = CFMWS_MOD_ARRAY_END;
+		dev_dbg(NULL, "cxl_test loading modulo math option\n");
+	}
+
 	rc = populate_cedt();
 	if (rc)
 		goto err_populate;
@@ -1073,6 +1183,8 @@ static __exit void cxl_test_exit(void)
 	unregister_cxl_mock_ops(&cxl_mock_ops);
 }
 
+module_param(interleave_arithmetic, int, 0000);
+MODULE_PARM_DESC(interleave_arithmetic, "Modulo:0, XOR:1");
 module_init(cxl_test_init);
 module_exit(cxl_test_exit);
 MODULE_LICENSE("GPL v2");
-- 
2.37.3

Re: [PATCH v6 2/3] cxl/acpi: Support CXL XOR Interleave Math (CXIMS)

[Davidlohr Bueso]

On Fri, 11 Nov 2022, alison.schofield@intel.com wrote:

>+static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg,
>+			   const unsigned long end)
>+{
>+	struct acpi_cedt_cxims *cxims = (struct acpi_cedt_cxims *)header;
>+	struct cxl_cxims_context *ctx = arg;
>+	struct cxl_root_decoder *cxlrd = ctx->cxlrd;
>+	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
>+	struct device *dev = ctx->dev;
>+	struct cxims_data *cximsd;
>+	unsigned int hbig, nr_maps;
>+	int rc;
>+
>+	rc = cxl_to_granularity(cxims->hbig, &hbig);
>+	if (rc)
>+		return rc;
>+
>+	if (hbig == cxld->interleave_granularity) {
>+		/* IW 1,3 do not use xormaps and skip this parsing entirely */
>+
>+		if (is_power_of_2(cxld->interleave_ways))
>+			/* 2, 4, 8, 16 way */
>+			nr_maps = ilog2(cxld->interleave_ways);
>+		else
>+			/* 6, 12 way */
>+			nr_maps = ilog2(cxld->interleave_ways / 3);
>+
>+		if (cxims->nr_xormaps < nr_maps) {
>+			dev_dbg(dev, "CXIMS nr_xormaps[%d] expected[%d]\n",
>+				cxims->nr_xormaps, nr_maps);
>+			return -ENXIO;
>+		}
>+
>+		cximsd = devm_kzalloc(dev,
>+				      struct_size(cximsd, xormaps, nr_maps),
>+				      GFP_KERNEL);

Need to check for failed kzalloc.

>+		memcpy(cximsd->xormaps, cxims->xormap_list,
>+		       nr_maps * sizeof(*cximsd->xormaps));
>+		cximsd->nr_maps = nr_maps;
>+		cxlrd->platform_data = cximsd;
>+		cxlrd->calc_hb = cxl_hb_xor;
>+	}
>+	return 0;
>+}
>+
> static unsigned long cfmws_to_decoder_flags(int restrictions)
> {
>	unsigned long flags = CXL_DECODER_F_ENABLE;
>@@ -33,11 +130,6 @@ static int cxl_acpi_cfmws_verify(struct device *dev,
>	int rc, expected_len;
>	unsigned int ways;
>
>-	if (cfmws->interleave_arithmetic != ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) {
>-		dev_err(dev, "CFMWS Unsupported Interleave Arithmetic\n");
>-		return -EINVAL;
>-	}

It seems more robust to keep the check and just add the ACPI_CEDT_CFMWS_ARITHMETIC_XOR case.

>-
>	if (!IS_ALIGNED(cfmws->base_hpa, SZ_256M)) {
>		dev_err(dev, "CFMWS Base HPA not 256MB aligned\n");
>		return -EINVAL;
>@@ -84,6 +176,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>	struct cxl_cfmws_context *ctx = arg;
>	struct cxl_port *root_port = ctx->root_port;
>	struct resource *cxl_res = ctx->cxl_res;
>+	struct cxl_cxims_context cxims_ctx;
>	struct cxl_root_decoder *cxlrd;
>	struct device *dev = ctx->dev;
>	struct acpi_cedt_cfmws *cfmws;
>@@ -148,7 +241,33 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>		ig = CXL_DECODER_MIN_GRANULARITY;
>	cxld->interleave_granularity = ig;
>
>+	if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_XOR) {
>+		if (ways == 1 || ways == 3)	{
>+			/* Skip CXIMS parsing for IW 1 or 3. No xormaps used */
>+			cxlrd->calc_hb = cxl_hb_xor;
>+			goto decoder_add;
>+		}
>+
>+		cxims_ctx = (struct cxl_cxims_context) {
>+			.dev = dev,
>+			.cxlrd = cxlrd,
>+		};
>+
>+		rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CXIMS,
>+					   cxl_parse_cxims, &cxims_ctx);
>+		if (rc < 0)
>+			goto err_xormap;
>+
>+		if (cxlrd->calc_hb != cxl_hb_xor) {
>+			rc = -ENXIO;
>+			goto err_xormap;
>+		}
>+	}
>+

I'm not crazy about now having different layers setting the ->calc_hb callback. Normally
this is done when cxl_root_decoder_alloc(), and now it gets overwritten and reassigned in
both parser calls (cfmws and cxims). How about just moving it out of the cxlrd allocation
call and doing it all here in cxl_parse_cfmws()? You can also avoid the decoder_add goto.

	if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_XOR) {
		if (ways != 1 && ways != 3) {
			// arm cxims_ctx
			rc = acpi_table_parse_cedt();
			if (rc < 0)
				goto err_xormap;
		}

		cxlrd->calc_hb = cxl_hb_xor;
	} else
		cxlrd->calc_hb = cxl_hb_modulo;

>+decoder_add:
>	rc = cxl_decoder_add(cxld, target_map);
>+
>+err_xormap:
>	if (rc)
>		put_device(&cxld->dev);
>	else

Re: [PATCH v6 2/3] cxl/acpi: Support CXL XOR Interleave Math (CXIMS)

[Alison Schofield]

On Fri, Nov 11, 2022 at 10:28:23PM -0800, Davidlohr Bueso wrote:

Thanks for the review David!

> On Fri, 11 Nov 2022, alison.schofield@intel.com wrote:
> 
> > +static int cxl_parse_cxims(union acpi_subtable_headers *header, void *arg,
> > +			   const unsigned long end)
> > +{
> > +	struct acpi_cedt_cxims *cxims = (struct acpi_cedt_cxims *)header;
> > +	struct cxl_cxims_context *ctx = arg;
> > +	struct cxl_root_decoder *cxlrd = ctx->cxlrd;
> > +	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> > +	struct device *dev = ctx->dev;
> > +	struct cxims_data *cximsd;
> > +	unsigned int hbig, nr_maps;
> > +	int rc;
> > +
> > +	rc = cxl_to_granularity(cxims->hbig, &hbig);
> > +	if (rc)
> > +		return rc;
> > +
> > +	if (hbig == cxld->interleave_granularity) {
> > +		/* IW 1,3 do not use xormaps and skip this parsing entirely */
> > +
> > +		if (is_power_of_2(cxld->interleave_ways))
> > +			/* 2, 4, 8, 16 way */
> > +			nr_maps = ilog2(cxld->interleave_ways);
> > +		else
> > +			/* 6, 12 way */
> > +			nr_maps = ilog2(cxld->interleave_ways / 3);
> > +
> > +		if (cxims->nr_xormaps < nr_maps) {
> > +			dev_dbg(dev, "CXIMS nr_xormaps[%d] expected[%d]\n",
> > +				cxims->nr_xormaps, nr_maps);
> > +			return -ENXIO;
> > +		}
> > +
> > +		cximsd = devm_kzalloc(dev,
> > +				      struct_size(cximsd, xormaps, nr_maps),
> > +				      GFP_KERNEL);
> 
> Need to check for failed kzalloc.
> 

Got it, thanks!

> > +		memcpy(cximsd->xormaps, cxims->xormap_list,
> > +		       nr_maps * sizeof(*cximsd->xormaps));
> > +		cximsd->nr_maps = nr_maps;
> > +		cxlrd->platform_data = cximsd;
> > +		cxlrd->calc_hb = cxl_hb_xor;
> > +	}
> > +	return 0;
> > +}
> > +
> > static unsigned long cfmws_to_decoder_flags(int restrictions)
> > {
> > 	unsigned long flags = CXL_DECODER_F_ENABLE;
> > @@ -33,11 +130,6 @@ static int cxl_acpi_cfmws_verify(struct device *dev,
> > 	int rc, expected_len;
> > 	unsigned int ways;
> > 
> > -	if (cfmws->interleave_arithmetic != ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) {
> > -		dev_err(dev, "CFMWS Unsupported Interleave Arithmetic\n");
> > -		return -EINVAL;
> > -	}
> 
> It seems more robust to keep the check and just add the ACPI_CEDT_CFMWS_ARITHMETIC_XOR case.
> 

The reasoning behind removing the check, versus expanding it, is that the
check shouldn't have been there in the first place. The ACPI driver is
responsible for parsing the CFMWS's but it is the responsibility of the
region driver to decide which CFMWS's it can use. 

If you believe that premise, then we let all interleave arithmetics
'get through' to the region driver.

More below on same topic...

> > -
> > 	if (!IS_ALIGNED(cfmws->base_hpa, SZ_256M)) {
> > 		dev_err(dev, "CFMWS Base HPA not 256MB aligned\n");
> > 		return -EINVAL;
> > @@ -84,6 +176,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
> > 	struct cxl_cfmws_context *ctx = arg;
> > 	struct cxl_port *root_port = ctx->root_port;
> > 	struct resource *cxl_res = ctx->cxl_res;
> > +	struct cxl_cxims_context cxims_ctx;
> > 	struct cxl_root_decoder *cxlrd;
> > 	struct device *dev = ctx->dev;
> > 	struct acpi_cedt_cfmws *cfmws;
> > @@ -148,7 +241,33 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
> > 		ig = CXL_DECODER_MIN_GRANULARITY;
> > 	cxld->interleave_granularity = ig;
> > 
> > +	if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_XOR) {
> > +		if (ways == 1 || ways == 3)	{
> > +			/* Skip CXIMS parsing for IW 1 or 3. No xormaps used */
> > +			cxlrd->calc_hb = cxl_hb_xor;
> > +			goto decoder_add;
> > +		}
> > +
> > +		cxims_ctx = (struct cxl_cxims_context) {
> > +			.dev = dev,
> > +			.cxlrd = cxlrd,
> > +		};
> > +
> > +		rc = acpi_table_parse_cedt(ACPI_CEDT_TYPE_CXIMS,
> > +					   cxl_parse_cxims, &cxims_ctx);
> > +		if (rc < 0)
> > +			goto err_xormap;
> > +
> > +		if (cxlrd->calc_hb != cxl_hb_xor) {
> > +			rc = -ENXIO;
> > +			goto err_xormap;
> > +		}
> > +	}
> > +
> 
> I'm not crazy about now having different layers setting the ->calc_hb callback. Normally
> this is done when cxl_root_decoder_alloc(), and now it gets overwritten and reassigned in
> both parser calls (cfmws and cxims). How about just moving it out of the cxlrd allocation
> call and doing it all here in cxl_parse_cfmws()? You can also avoid the decoder_add goto.

Following on the theme of maintaining the separation of ACPI and REGION
driver responsibilities, the decision on the calc_hb callback doesn't
belong to the ACPI driver. Polluting the ACPI driver w knowledge of
cxl_hb_xor() is not a clean soln either, as you point out.

How about this as a clean(er) separation:

ACPI Driver
-  adds cxlrd->interleave arithmetic field
-  adds fields: .interleave_arithemetic and .callback to the new
   platform data field (ie. cxims_data)

Region Driver
- uses the cxlrd->interleave_arithmetic field to assign callback
- If that interleave_arithmetic is not 'known' to the region driver,
  it can lookup the callback in the cxlrd platform data 
  
I will try the above flow out, and await further discussion on
the separation or ACPI & REGION driver responsibilites.

Thanks for bringing it up David,
Alison

> 
snip
> 

Re: [PATCH v6 2/3] cxl/acpi: Support CXL XOR Interleave Math (CXIMS)

[Davidlohr Bueso]

On Mon, 14 Nov 2022, Alison Schofield wrote:

>The reasoning behind removing the check, versus expanding it, is that the
>check shouldn't have been there in the first place. The ACPI driver is
>responsible for parsing the CFMWS's but it is the responsibility of the
>region driver to decide which CFMWS's it can use.
>
>If you believe that premise, then we let all interleave arithmetics
>'get through' to the region driver.

The way I see it, acpi should be setting the callback because it is a _direct_
consequence of parsing the cfmws, and that's really the same logic as we have
now, in that it sets it when allocating the rp decoder. And that doesn't take
away from the region driver responsibility of choosing the actual host bridge.

Thanks,
Davidlohr

Re: [PATCH v6 2/3] cxl/acpi: Support CXL XOR Interleave Math (CXIMS)

[Alison Schofield]

On Mon, Nov 14, 2022 at 10:57:29AM -0800, Davidlohr Bueso wrote:
> On Mon, 14 Nov 2022, Alison Schofield wrote:
> 
> > The reasoning behind removing the check, versus expanding it, is that the
> > check shouldn't have been there in the first place. The ACPI driver is
> > responsible for parsing the CFMWS's but it is the responsibility of the
> > region driver to decide which CFMWS's it can use.
> > 
> > If you believe that premise, then we let all interleave arithmetics
> > 'get through' to the region driver.
> 
> The way I see it, acpi should be setting the callback because it is a _direct_
> consequence of parsing the cfmws, and that's really the same logic as we have
> now, in that it sets it when allocating the rp decoder. And that doesn't take
> away from the region driver responsibility of choosing the actual host bridge.

With a little help from my friends ;) I'm hopeful the next version
meets your expectation by setting the calc_hb only once when allocating
that root port decoder.

> 
> Thanks,
> Davidlohr