[PATCH 0/4] CXL Address Translation

[PATCH 0/4] CXL Address Translation

[alison.schofield]

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

Introducing CXL DPA->HPA address translations. User space requires
HPA addresses when CXL devices, participating in a region, report
errors. For example, a patchset[1] adding trace events for media errors 
is in review on this list, yet it only provides userspace with the DPA.
By adding this translation, HPA’s can be added to those media error
trace events.

Patch 1 performs the translation and Patch 2 gives it exercise by
translating a small sample of addresses at every pmem region creation,
in a debug kernel.

Patches 1 & 2 deliver what is required for address translation, and
do not depend on Patches 3 & 4.

Patches 3 & 4 seem like a good idea, however, the value of that idea
is not clear. The idea is to add another level of validation to address
translations for regions using XOR arithmetic (as opposed to modulo).

Patch 4 runs the newly constructed HPA through the existing XORMAP filter
to see if it maps to the expected dport. While a valid check, it's not
clear that it offers anything more than the range checking already done
in the generic translation. That is, if an HPA translation passes the
range check, how can a match on dport test ever fail? I don't know.
I’m looking for help in proving or disproving the usefulness of 3 & 4.

Using cxl_test, existing unit tests that create regions exercise
the address translation and the results can be viewed like this:

# meson test -C build --suite=cxl
ninja: Entering directory `/root/ndctl/build'
[109/109] Linking target ndctl/ndctl
1/5 ndctl:cxl / cxl-topology.sh             OK               4.19s
2/5 ndctl:cxl / cxl-region-sysfs.sh         OK               2.69s
3/5 ndctl:cxl / cxl-labels.sh               OK               5.85s
4/5 ndctl:cxl / cxl-create-region.sh        OK               5.35s
5/5 ndctl:cxl / cxl-xor-region.sh           OK               1.62s

# dmesg | grep -m1 "Address translation"
cxl_core:devm_cxl_add_pmem_region:2006: cxl_region region6: Address translation check: Pass

# dmesg | grep Addr | grep Pass | wc -l
46
That '46', is how many regions are created and their addresses successfully
translated in the existing cxl unit tests.  (No Fails today ;))

TODO in ndctl: If the check on region creation gets merged, add watchers for
it to cxl unit tests that create regions.

Alison Schofield (4):
  cxl/region: Add a DPA to HPA translation helper
  cxl/region: Check addr trans at pmem region create (debug only)
  cxl/acpi: Move the target entry(n) calc to its own function
  cxl/acpi: Add a match on dport check for XOR addr translation

 drivers/cxl/acpi.c        |  91 +++++++++++++++++++++++-------
 drivers/cxl/core/port.c   |   5 +-
 drivers/cxl/core/region.c | 116 +++++++++++++++++++++++++++++++++++++-
 drivers/cxl/cxl.h         |  11 +++-
 4 files changed, 199 insertions(+), 24 deletions(-)


base-commit: f0c4d9fc9cc9462659728d168387191387e903cc

This is built upon 6.1-rc4 plus CXL XOR Interleave Arithmetic [2]
If Patches 3 & 4 are discarded, then there is no dependency on [2]

[1] https://lore.kernel.org/linux-cxl/cover.1668115235.git.alison.schofield@intel.com/
[2] https://lore.kernel.org/linux-cxl/cover.1669153633.git.alison.schofield@intel.com/

Gobble Gobble
-- 
2.37.3

[PATCH 1/4] cxl/region: Add a DPA to HPA translation helper

[alison.schofield]

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

CXL devices may report errors and events using their DPA (device
physical address). When a CXL device contributes capacity to a
CXL region, the device's physical addresses are mapped to HPA's.
(host physical addresses)

Provide a helper to calculate the HPA when given a DPA, a region,
and the devices position in the region interleave.

Verify that the HPA is within the expected ranges that this device
contributes to the region interleave set.

The initial use case is translating the DPAs that CXL devices
report in media error records.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/core.h   |  3 ++
 drivers/cxl/core/region.c | 80 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 1d8f87be283f..72b58e53c394 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -67,6 +67,9 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
 	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
 }
 
+u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
+		   struct cxl_endpoint_decoder *cxled);
+
 int cxl_memdev_init(void);
 void cxl_memdev_exit(void);
 void cxl_mbox_init(void);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index f9ae5ad284ff..c847517e766c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1865,6 +1865,86 @@ static void cxlr_pmem_unregister(void *dev)
 	device_unregister(dev);
 }
 
+static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	int gran = p->interleave_granularity;
+	int ways = p->interleave_ways;
+	u64 stride;
+
+	/* Is the hpa within this region at all */
+	if (hpa < p->res->start || hpa > p->res->end) {
+		dev_dbg(&cxlr->dev,
+			"Addr trans fail: hpa 0x%llx not in region\n", hpa);
+		return false;
+	}
+	/* Is the hpa in an expected stride for its pos(-ition) */
+	stride = p->res->start + pos * gran;
+	do {
+		if (hpa >= stride && hpa <= stride + gran - 1)
+			return true;
+
+		stride = stride + ways * gran;
+	} while (stride < p->res->end);
+
+	dev_dbg(&cxlr->dev,
+		"Addr trans fail: hpa 0x%llx not in any stride\n", hpa);
+
+	return false;
+}
+
+u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
+		   struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	u64 dpa_offset, hpa_offset, hpa;
+	int rc, pos = cxled->pos;
+	u16 eig;
+	u8 eiw;
+
+	rc = ways_to_cxl(p->interleave_ways, &eiw);
+	if (rc)
+		return rc;
+	rc = granularity_to_cxl(p->interleave_granularity, &eig);
+	if (rc)
+		return rc;
+
+	/*
+	 * Reverse the HPA->DPA decode logic defined
+	 * in the CXL Spec 3.0 Section 8.2.4.19.13
+	 * Implementation Note: Device Decode Logic
+	 *
+	 * The device position in the region interleave
+	 * set was removed in the HPA->DPA translation.
+	 * Put it back to reconstruct the HPA.
+	 */
+
+	/* Remove the dpa base */
+	dpa_offset = dpa - cxl_dpa_resource_start(cxled);
+
+	/* Restore the position */
+	if (eiw <= 8) {
+		hpa_offset = (dpa_offset & GENMASK_ULL(51, eig + 8)) << eiw;
+		hpa_offset |= GENMASK_ULL(eig + 8 + eiw, eig + 8)
+			      & (pos << (eig + 8));
+	}
+	if (eiw == 9)
+		hpa_offset |= BIT(eig + eiw) & (pos & 0x01);
+	if (eiw == 10)
+		hpa_offset |= GENMASK_ULL(eig + eiw, eig + 8) & (pos & 0x03);
+
+	/* The lower bits remain unchanged */
+	hpa_offset |= dpa_offset & GENMASK_ULL(eig + 7, 0);
+
+	/* Apply the hpa_offset to region base address */
+	hpa = hpa_offset + p->res->start;
+
+	if (!cxl_is_hpa_in_range(hpa, cxlr, cxled->pos))
+		return 0;
+
+	return hpa;
+}
+
 /**
  * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
  * @cxlr: parent CXL region for this pmem region bridge device
-- 
2.37.3

[PATCH 2/4] cxl/region: Check addr trans at pmem region create (debug only)

[alison.schofield]

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

At the time of region creation, device physical addresses (DPA)
are mapped to host physical addresses (HPA). The CXL driver
translates DPA's to HPA's for user space consumption. In order
to prove and exercise that translation functionality, perform
a small sample of DPA to HPA translations whenever a pmem region
is created in a debug kernel.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index c847517e766c..32216b5fe450 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1945,6 +1945,36 @@ u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
 	return hpa;
 }
 
+static bool cxl_check_addrtrans(struct cxl_region *cxlr)
+{
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_endpoint_decoder *cxled;
+	u64 start, end, dpa;
+
+	/*
+	 * Translate a few DPAs (start,mid,end) to HPAs
+	 * for each contributing endpoint decoder.
+	 */
+	for (int i = 0; i <  p->nr_targets; i++) {
+		cxled = p->targets[i];
+		start = cxl_dpa_resource_start(cxled);
+		end = start + cxl_dpa_size(cxled) - 1;
+
+		dpa = start;
+		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
+			return false;
+
+		dpa = start + cxl_dpa_size(cxled) / 2;
+		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
+			return false;
+
+		dpa = end;
+		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
+			return false;
+	}
+	return true;
+}
+
 /**
  * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
  * @cxlr: parent CXL region for this pmem region bridge device
@@ -1973,8 +2003,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
 	dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
 		dev_name(dev));
 
+	dev_dbg(&cxlr->dev, "Address translation check: %s\n",
+		cxl_check_addrtrans(cxlr) ? "Pass" : "Fail");
+
 	return devm_add_action_or_reset(&cxlr->dev, cxlr_pmem_unregister, dev);
-
 err:
 	put_device(dev);
 	return rc;
-- 
2.37.3

[PATCH 3/4] cxl/acpi: Move the target entry(n) calc to its own function

[alison.schofield]

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

In preparation for reusing the calculation of a targets
entry (n) in the host bridge interleave set, move it to
a separate function.

Make it accept an HPA parameter so that it is useful for
validating DPA to HPA address translations.

No functional change.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/acpi.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 98c84942ed37..38b5f77164b0 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -19,6 +19,25 @@ struct cxl_cxims_data {
  * Find a targets entry (n) in the host bridge interleave list.
  * CXL Specfication 3.0 Table 9-22
  */
+static int cxl_xor_calc_n(u64 hpa, struct cxl_cxims_data *cximsd, int iw,
+			  int ig)
+{
+	int eiw, i = 0, n = 0;
+
+	/* IW: 2,4,6,8,12,16 begin building 'n' using xormaps */
+	if (iw != 3) {
+		for (i = 0; i < cximsd->nr_maps; i++)
+			n |= (hweight64(hpa & cximsd->xormaps[i]) & 1) << i;
+	}
+	/* 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 n;
+}
+
 static struct cxl_dport *cxl_hb_xor(struct cxl_root_decoder *cxlrd, int pos)
 {
 	struct cxl_cxims_data *cximsd = cxlrd->platform_data;
@@ -26,7 +45,7 @@ static struct cxl_dport *cxl_hb_xor(struct cxl_root_decoder *cxlrd, int pos)
 	struct cxl_decoder *cxld = &cxlsd->cxld;
 	int ig = cxld->interleave_granularity;
 	int iw = cxld->interleave_ways;
-	int eiw, i = 0, n = 0;
+	int n = 0;
 	u64 hpa;
 
 	if (dev_WARN_ONCE(&cxld->dev,
@@ -34,26 +53,12 @@ static struct cxl_dport *cxl_hb_xor(struct cxl_root_decoder *cxlrd, int pos)
 			  "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;
+	/* Entry (n) is 0 for no interleave (iw == 1) */
+	if (iw != 1)
+		n = cxl_xor_calc_n(hpa, cximsd, iw, ig);
 
-	/* 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];
 }
 
-- 
2.37.3

[PATCH 4/4] cxl/acpi: Add a match on dport check for XOR addr translation

[alison.schofield]

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

Address translations from DPA to HPA are validated by checking
that the resulting HPA is within the expected region resource.

When the host bridge is using XOR arithmetic, an additional
check can be performed by passing the HPA through an XOR
function that finds its index in the host bridge interleave
list. An HPA passes this check if the index derived matches
the known dport of the endpoint decoder.

Since this is a check that applies only to host bridges using
XOR arithmetic, layer it on top of the existing cxl_dpa_to_hpa()
by adding a new call back type: cxl_calc_hpa_fn() to the
cxl_root_decoder.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/acpi.c        | 50 ++++++++++++++++++++++++++++++++++++---
 drivers/cxl/core/core.h   |  3 ---
 drivers/cxl/core/port.c   |  5 +++-
 drivers/cxl/core/region.c |  8 ++++---
 drivers/cxl/cxl.h         | 11 ++++++++-
 5 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 38b5f77164b0..424469d73549 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -8,6 +8,7 @@
 #include <linux/pci.h>
 #include <asm/div64.h>
 #include "cxlpci.h"
+#include "cxlmem.h"
 #include "cxl.h"
 
 struct cxl_cxims_data {
@@ -38,6 +39,44 @@ static int cxl_xor_calc_n(u64 hpa, struct cxl_cxims_data *cximsd, int iw,
 	return n;
 }
 
+static bool cxl_xor_hpa_to_dport(u64 hpa, struct cxl_root_decoder *cxlrd,
+				 struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_cxims_data *cximsd = cxlrd->platform_data;
+	int ig = cxled->cxld.interleave_granularity;
+	int iw = cxled->cxld.interleave_ways;
+	struct cxl_dport *match_dport;
+	int n = 0;
+
+	if (iw != 1)
+		n = cxl_xor_calc_n(hpa, cximsd, iw, ig);
+
+	match_dport = cxl_find_dport_by_dev(cxlrd_to_port(cxlrd),
+					    cxled_to_port(cxled)->host_bridge);
+	if (cxlrd->cxlsd.target[n] != match_dport)
+		return false;
+
+	return true;
+}
+
+static u64 cxl_dpa_to_hpa_xor(u64 dpa, struct cxl_region *cxlr,
+			      struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+	u64 hpa;
+
+	hpa = cxl_dpa_to_hpa(dpa, cxlr, cxled);
+	if (!hpa)
+		return 0;
+
+	if (!cxl_xor_hpa_to_dport(hpa, cxlrd, cxled)) {
+		dev_dbg(&cxlr->dev,
+			"Addr trans fail: hpa:0x%llx dport mismatch\n", hpa);
+		return 0;
+	}
+	return hpa;
+}
+
 static struct cxl_dport *cxl_hb_xor(struct cxl_root_decoder *cxlrd, int pos)
 {
 	struct cxl_cxims_data *cximsd = cxlrd->platform_data;
@@ -193,6 +232,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 	struct cxl_root_decoder *cxlrd;
 	struct device *dev = ctx->dev;
 	struct acpi_cedt_cfmws *cfmws;
+	cxl_calc_hpa_fn cxl_calc_hpa;
 	cxl_calc_hb_fn cxl_calc_hb;
 	struct cxl_decoder *cxld;
 	unsigned int ways, i, ig;
@@ -235,12 +275,16 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 	if (rc)
 		goto err_insert;
 
-	if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_MODULO)
+	if (cfmws->interleave_arithmetic == ACPI_CEDT_CFMWS_ARITHMETIC_MODULO) {
 		cxl_calc_hb = cxl_hb_modulo;
-	else
+		cxl_calc_hpa = cxl_dpa_to_hpa;
+	} else {
 		cxl_calc_hb = cxl_hb_xor;
+		cxl_calc_hpa = cxl_dpa_to_hpa_xor;
+	}
 
-	cxlrd = cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb);
+	cxlrd = cxl_root_decoder_alloc(root_port, ways, cxl_calc_hb,
+				       cxl_calc_hpa);
 	if (IS_ERR(cxlrd))
 		return 0;
 
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 72b58e53c394..1d8f87be283f 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -67,9 +67,6 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
 	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
 }
 
-u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
-		   struct cxl_endpoint_decoder *cxled);
-
 int cxl_memdev_init(void);
 void cxl_memdev_exit(void);
 void cxl_mbox_init(void);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 42cdf224a85d..0f1e691ed02f 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1504,6 +1504,7 @@ static int cxl_switch_decoder_init(struct cxl_port *port,
  * @port: owning CXL root of this decoder
  * @nr_targets: static number of downstream targets
  * @calc_hb: which host bridge covers the n'th position by granularity
+ * @calc_hpa: dpa to hpa address translation function
  *
  * Return: A new cxl decoder to be registered by cxl_decoder_add(). A
  * 'CXL root' decoder is one that decodes from a top-level / static platform
@@ -1512,7 +1513,8 @@ static int cxl_switch_decoder_init(struct cxl_port *port,
  */
 struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
 						unsigned int nr_targets,
-						cxl_calc_hb_fn calc_hb)
+						cxl_calc_hb_fn calc_hb,
+						cxl_calc_hpa_fn calc_hpa)
 {
 	struct cxl_root_decoder *cxlrd;
 	struct cxl_switch_decoder *cxlsd;
@@ -1535,6 +1537,7 @@ struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
 	}
 
 	cxlrd->calc_hb = calc_hb;
+	cxlrd->calc_hpa = calc_hpa;
 
 	cxld = &cxlsd->cxld;
 	cxld->dev.type = &cxl_decoder_root_type;
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 32216b5fe450..c14d098d557b 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1944,9 +1944,11 @@ u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
 
 	return hpa;
 }
+EXPORT_SYMBOL_NS_GPL(cxl_dpa_to_hpa, CXL);
 
 static bool cxl_check_addrtrans(struct cxl_region *cxlr)
 {
+	struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_endpoint_decoder *cxled;
 	u64 start, end, dpa;
@@ -1961,15 +1963,15 @@ static bool cxl_check_addrtrans(struct cxl_region *cxlr)
 		end = start + cxl_dpa_size(cxled) - 1;
 
 		dpa = start;
-		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
+		if (!cxlrd->calc_hpa(dpa, cxlr, cxled))
 			return false;
 
 		dpa = start + cxl_dpa_size(cxled) / 2;
-		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
+		if (!cxlrd->calc_hpa(dpa, cxlr, cxled))
 			return false;
 
 		dpa = end;
-		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
+		if (!cxlrd->calc_hpa(dpa, cxlr, cxled))
 			return false;
 	}
 	return true;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index d03aa1776fc8..a14a1defa14f 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -328,12 +328,15 @@ struct cxl_root_decoder;
 struct cxl_endpoint_decoder;
 typedef struct cxl_dport *(*cxl_calc_hb_fn)(struct cxl_root_decoder *cxlrd,
 					    int pos);
+typedef u64 (*cxl_calc_hpa_fn)(u64 dpa, struct cxl_region *cxlr,
+			       struct cxl_endpoint_decoder *cxled);
 
 /**
  * struct cxl_root_decoder - Static platform CXL address 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
+ * @calc_hpa: dpa to hpa address translation function
  * @platform_data: platform specific configuration data
  * @cxlsd: base cxl switch decoder
  */
@@ -341,6 +344,7 @@ struct cxl_root_decoder {
 	struct resource *res;
 	atomic_t region_id;
 	cxl_calc_hb_fn calc_hb;
+	cxl_calc_hpa_fn calc_hpa;
 	void *platform_data;
 	struct cxl_switch_decoder cxlsd;
 };
@@ -589,7 +593,10 @@ bool is_endpoint_decoder(struct device *dev);
 
 struct cxl_root_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
 						unsigned int nr_targets,
-						cxl_calc_hb_fn calc_hb);
+						cxl_calc_hb_fn calc_hb,
+						cxl_calc_hpa_fn calc_hpa);
+
+/* TODO should cxl_hb_module be of type 'cxl_calc_hb_fn */
 struct cxl_dport *cxl_hb_modulo(struct cxl_root_decoder *cxlrd, int pos);
 struct cxl_switch_decoder *cxl_switch_decoder_alloc(struct cxl_port *port,
 						    unsigned int nr_targets);
@@ -598,6 +605,8 @@ struct cxl_endpoint_decoder *cxl_endpoint_decoder_alloc(struct cxl_port *port);
 int cxl_decoder_add_locked(struct cxl_decoder *cxld, int *target_map);
 int cxl_decoder_autoremove(struct device *host, struct cxl_decoder *cxld);
 int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint);
+u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
+		   struct cxl_endpoint_decoder *cxled);
 
 struct cxl_hdm;
 struct cxl_hdm *devm_cxl_setup_hdm(struct cxl_port *port);
-- 
2.37.3

Re: [PATCH 1/4] cxl/region: Add a DPA to HPA translation helper

[Jonathan Cameron]

On Tue, 22 Nov 2022 15:07:48 -0800
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> CXL devices may report errors and events using their DPA (device
> physical address). When a CXL device contributes capacity to a
> CXL region, the device's physical addresses are mapped to HPA's.
> (host physical addresses)
> 
> Provide a helper to calculate the HPA when given a DPA, a region,
> and the devices position in the region interleave.

device's 

> 
> Verify that the HPA is within the expected ranges that this device
> contributes to the region interleave set.
> 
> The initial use case is translating the DPAs that CXL devices
> report in media error records.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

I'm not following the maths for the 3/6/12 way interleave cases.

> ---
>  drivers/cxl/core/core.h   |  3 ++
>  drivers/cxl/core/region.c | 80 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 1d8f87be283f..72b58e53c394 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -67,6 +67,9 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>  	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
>  }
>  
> +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> +		   struct cxl_endpoint_decoder *cxled);
> +
>  int cxl_memdev_init(void);
>  void cxl_memdev_exit(void);
>  void cxl_mbox_init(void);
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index f9ae5ad284ff..c847517e766c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1865,6 +1865,86 @@ static void cxlr_pmem_unregister(void *dev)
>  	device_unregister(dev);
>  }
>  
> +static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	int gran = p->interleave_granularity;
> +	int ways = p->interleave_ways;
> +	u64 stride;
> +
> +	/* Is the hpa within this region at all */
> +	if (hpa < p->res->start || hpa > p->res->end) {
> +		dev_dbg(&cxlr->dev,
> +			"Addr trans fail: hpa 0x%llx not in region\n", hpa);
> +		return false;
> +	}
> +	/* Is the hpa in an expected stride for its pos(-ition) */
> +	stride = p->res->start + pos * gran;
> +	do {
> +		if (hpa >= stride && hpa <= stride + gran - 1)
	< stride + gran seems simpler.

> +			return true;
> +
> +		stride = stride + ways * gran;
> +	} while (stride < p->res->end);

That's going to take a 'while' with a large region.  Can we do something quicker
along the lines of...

Something like (untested)
	u64 offset = hpa - p->res_start;

	/* Offset within the right 'stride' */
	offset = offset % (gran * ways);

	if (offset >= pos * gran && offset < (pos + 1) * gran)
		return true;

..
> +
> +	dev_dbg(&cxlr->dev,
> +		"Addr trans fail: hpa 0x%llx not in any stride\n", hpa);
> +
> +	return false;
> +}
> +
> +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> +		   struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	u64 dpa_offset, hpa_offset, hpa;
> +	int rc, pos = cxled->pos;
> +	u16 eig;
> +	u8 eiw;
> +
> +	rc = ways_to_cxl(p->interleave_ways, &eiw);
I lost track a bit, but I think we ended up with einw?
> +	if (rc)
> +		return rc;
> +	rc = granularity_to_cxl(p->interleave_granularity, &eig);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * Reverse the HPA->DPA decode logic defined
> +	 * in the CXL Spec 3.0 Section 8.2.4.19.13
> +	 * Implementation Note: Device Decode Logic
Short line wrap. Probably want to go nearer 80 chars

> +	 *
> +	 * The device position in the region interleave
> +	 * set was removed in the HPA->DPA translation.
> +	 * Put it back to reconstruct the HPA.
> +	 */
> +
> +	/* Remove the dpa base */
> +	dpa_offset = dpa - cxl_dpa_resource_start(cxled);
> +
> +	/* Restore the position */
> +	if (eiw <= 8) {

This looks to be the power of 2 version, so should that be eiw < 8?

> +		hpa_offset = (dpa_offset & GENMASK_ULL(51, eig + 8)) << eiw;
> +		hpa_offset |= GENMASK_ULL(eig + 8 + eiw, eig + 8)
> +			      & (pos << (eig + 8));

Why is the masking needed here?  I think pos should always fit in the iw bits.

> +	}
> +	if (eiw == 9)

else if would show clearly that only one of these ifs is true.
 
> +		hpa_offset |= BIT(eig + eiw) & (pos & 0x01);

I don't follow this logic. hpa_offset hasn't been set to anything before
this point + the right hand side of the above will always be 0 (I think...)


> +	if (eiw == 10)
else if
> +		hpa_offset |= GENMASK_ULL(eig + eiw, eig + 8) & (pos & 0x03);
> +
> +	/* The lower bits remain unchanged */
> +	hpa_offset |= dpa_offset & GENMASK_ULL(eig + 7, 0);
> +
> +	/* Apply the hpa_offset to region base address */
> +	hpa = hpa_offset + p->res->start;
> +
> +	if (!cxl_is_hpa_in_range(hpa, cxlr, cxled->pos))
> +		return 0;
> +
> +	return hpa;
> +}
> +
>  /**
>   * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
>   * @cxlr: parent CXL region for this pmem region bridge device

Re: [PATCH 1/4] cxl/region: Add a DPA to HPA translation helper

[Jonathan Cameron]

On Tue, 22 Nov 2022 15:07:48 -0800
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> CXL devices may report errors and events using their DPA (device
> physical address). When a CXL device contributes capacity to a
> CXL region, the device's physical addresses are mapped to HPA's.
> (host physical addresses)
> 
> Provide a helper to calculate the HPA when given a DPA, a region,
> and the devices position in the region interleave.
> 
> Verify that the HPA is within the expected ranges that this device
> contributes to the region interleave set.
> 
> The initial use case is translating the DPAs that CXL devices
> report in media error records.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/core.h   |  3 ++
>  drivers/cxl/core/region.c | 80 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> index 1d8f87be283f..72b58e53c394 100644
> --- a/drivers/cxl/core/core.h
> +++ b/drivers/cxl/core/core.h
> @@ -67,6 +67,9 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>  	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
>  }
>  
> +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> +		   struct cxl_endpoint_decoder *cxled);
> +
>  int cxl_memdev_init(void);
>  void cxl_memdev_exit(void);
>  void cxl_mbox_init(void);
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index f9ae5ad284ff..c847517e766c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1865,6 +1865,86 @@ static void cxlr_pmem_unregister(void *dev)
>  	device_unregister(dev);
>  }
>  
> +static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	int gran = p->interleave_granularity;
> +	int ways = p->interleave_ways;
> +	u64 stride;
> +
> +	/* Is the hpa within this region at all */
> +	if (hpa < p->res->start || hpa > p->res->end) {
> +		dev_dbg(&cxlr->dev,
> +			"Addr trans fail: hpa 0x%llx not in region\n", hpa);
> +		return false;
> +	}
> +	/* Is the hpa in an expected stride for its pos(-ition) */
> +	stride = p->res->start + pos * gran;
> +	do {
> +		if (hpa >= stride && hpa <= stride + gran - 1)
> +			return true;
> +
> +		stride = stride + ways * gran;
> +	} while (stride < p->res->end);
> +
> +	dev_dbg(&cxlr->dev,
> +		"Addr trans fail: hpa 0x%llx not in any stride\n", hpa);
> +
> +	return false;
> +}
> +
> +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> +		   struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	u64 dpa_offset, hpa_offset, hpa;
> +	int rc, pos = cxled->pos;
> +	u16 eig;
> +	u8 eiw;
> +
> +	rc = ways_to_cxl(p->interleave_ways, &eiw);
> +	if (rc)
> +		return rc;
> +	rc = granularity_to_cxl(p->interleave_granularity, &eig);
> +	if (rc)
> +		return rc;
returning integer as u64, so it won't end up as a useful error code.

> +
> +	/*
> +	 * Reverse the HPA->DPA decode logic defined
> +	 * in the CXL Spec 3.0 Section 8.2.4.19.13
> +	 * Implementation Note: Device Decode Logic
> +	 *
> +	 * The device position in the region interleave
> +	 * set was removed in the HPA->DPA translation.
> +	 * Put it back to reconstruct the HPA.
> +	 */
> +
> +	/* Remove the dpa base */
> +	dpa_offset = dpa - cxl_dpa_resource_start(cxled);
> +
> +	/* Restore the position */
> +	if (eiw <= 8) {
> +		hpa_offset = (dpa_offset & GENMASK_ULL(51, eig + 8)) << eiw;
> +		hpa_offset |= GENMASK_ULL(eig + 8 + eiw, eig + 8)
> +			      & (pos << (eig + 8));
> +	}
> +	if (eiw == 9)
> +		hpa_offset |= BIT(eig + eiw) & (pos & 0x01);
> +	if (eiw == 10)
> +		hpa_offset |= GENMASK_ULL(eig + eiw, eig + 8) & (pos & 0x03);
> +
> +	/* The lower bits remain unchanged */
> +	hpa_offset |= dpa_offset & GENMASK_ULL(eig + 7, 0);
> +
> +	/* Apply the hpa_offset to region base address */
> +	hpa = hpa_offset + p->res->start;
> +
> +	if (!cxl_is_hpa_in_range(hpa, cxlr, cxled->pos))
> +		return 0;
> +
> +	return hpa;
> +}
> +
>  /**
>   * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
>   * @cxlr: parent CXL region for this pmem region bridge device

Re: [PATCH 2/4] cxl/region: Check addr trans at pmem region create (debug only)

[Jonathan Cameron]

On Tue, 22 Nov 2022 15:07:49 -0800
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> At the time of region creation, device physical addresses (DPA)
> are mapped to host physical addresses (HPA). The CXL driver
> translates DPA's to HPA's for user space consumption. In order
> to prove and exercise that translation functionality, perform
> a small sample of DPA to HPA translations whenever a pmem region
> is created in a debug kernel.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
I'd drop this patch - it isn't doing a particularly useful check
as it only checks the resulting HPA is in range, not that it's
actually correct.


> ---
>  drivers/cxl/core/region.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c847517e766c..32216b5fe450 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1945,6 +1945,36 @@ u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
>  	return hpa;
>  }
>  
> +static bool cxl_check_addrtrans(struct cxl_region *cxlr)
> +{
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_endpoint_decoder *cxled;
> +	u64 start, end, dpa;
> +
> +	/*
> +	 * Translate a few DPAs (start,mid,end) to HPAs
> +	 * for each contributing endpoint decoder.
> +	 */
> +	for (int i = 0; i <  p->nr_targets; i++) {
> +		cxled = p->targets[i];
> +		start = cxl_dpa_resource_start(cxled);
> +		end = start + cxl_dpa_size(cxled) - 1;
> +
> +		dpa = start;
> +		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
> +			return false;
Hmm. This verifies they are in range, but not that the
address is right.  Not sure that is particularly helpful.
Also - in theory, hpa address 0 is valid value, or does something
stop people putting a CFMWS at HPA 0 onwards?
Sure it's unlikely anyone will do so, but not impossible...

> +
> +		dpa = start + cxl_dpa_size(cxled) / 2;
> +		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
> +			return false;
> +
> +		dpa = end;
> +		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
> +			return false;
Maybe more useful to check 1 higher and trip the 'out of range'
return?
> +	}
> +	return true;
> +}
> +
>  /**
>   * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
>   * @cxlr: parent CXL region for this pmem region bridge device
> @@ -1973,8 +2003,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>  	dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
>  		dev_name(dev));
>  
> +	dev_dbg(&cxlr->dev, "Address translation check: %s\n",
> +		cxl_check_addrtrans(cxlr) ? "Pass" : "Fail");
> +
>  	return devm_add_action_or_reset(&cxlr->dev, cxlr_pmem_unregister, dev);
> -

stray change

>  err:
>  	put_device(dev);
>  	return rc;

Re: [PATCH 2/4] cxl/region: Check addr trans at pmem region create (debug only)

[Alison Schofield]

On Wed, Nov 30, 2022 at 04:42:35PM +0000, Jonathan Cameron wrote:
> On Tue, 22 Nov 2022 15:07:49 -0800
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > At the time of region creation, device physical addresses (DPA)
> > are mapped to host physical addresses (HPA). The CXL driver
> > translates DPA's to HPA's for user space consumption. In order
> > to prove and exercise that translation functionality, perform
> > a small sample of DPA to HPA translations whenever a pmem region
> > is created in a debug kernel.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> I'd drop this patch - it isn't doing a particularly useful check
> as it only checks the resulting HPA is in range, not that it's
> actually correct.

Hi Jonathan,
Belated thanks on this review. I've put this set aside and implemented
the DPA -> HPA translation for the specific use case of adding an 
HPA field to the cxl_poison TRACE EVENT. I'm about to post that
patch, but want to respond to your feedback, which I did take
into the new patch.

more...


> 
> 
> > ---
> >  drivers/cxl/core/region.c | 34 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index c847517e766c..32216b5fe450 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1945,6 +1945,36 @@ u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> >  	return hpa;
> >  }
> >  
> > +static bool cxl_check_addrtrans(struct cxl_region *cxlr)
> > +{
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	struct cxl_endpoint_decoder *cxled;
> > +	u64 start, end, dpa;
> > +
> > +	/*
> > +	 * Translate a few DPAs (start,mid,end) to HPAs
> > +	 * for each contributing endpoint decoder.
> > +	 */
> > +	for (int i = 0; i <  p->nr_targets; i++) {
> > +		cxled = p->targets[i];
> > +		start = cxl_dpa_resource_start(cxled);
> > +		end = start + cxl_dpa_size(cxled) - 1;
> > +
> > +		dpa = start;
> > +		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
> > +			return false;
> Hmm. This verifies they are in range, but not that the
> address is right.  Not sure that is particularly helpful.
> Also - in theory, hpa address 0 is valid value, or does something
> stop people putting a CFMWS at HPA 0 onwards?
> Sure it's unlikely anyone will do so, but not impossible...
> 

Understand about the HPA 0 now, and use ULLONG_MAX as failure
on non-translateable - in future patch.


> > +
> > +		dpa = start + cxl_dpa_size(cxled) / 2;
> > +		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
> > +			return false;
> > +
> > +		dpa = end;
> > +		if (!cxl_dpa_to_hpa(dpa, cxlr, cxled))
> > +			return false;
> Maybe more useful to check 1 higher and trip the 'out of range'
> return?

So, I've dropped this check on region create work entirely for now.

> > +	}
> > +	return true;
> > +}
> > +
> >  /**
> >   * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
> >   * @cxlr: parent CXL region for this pmem region bridge device
> > @@ -1973,8 +2003,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
> >  	dev_dbg(&cxlr->dev, "%s: register %s\n", dev_name(dev->parent),
> >  		dev_name(dev));
> >  
> > +	dev_dbg(&cxlr->dev, "Address translation check: %s\n",
> > +		cxl_check_addrtrans(cxlr) ? "Pass" : "Fail");
> > +
> >  	return devm_add_action_or_reset(&cxlr->dev, cxlr_pmem_unregister, dev);
> > -
> 
> stray change

Thanks

> 
> >  err:
> >  	put_device(dev);
> >  	return rc;
> 

Re: [PATCH 1/4] cxl/region: Add a DPA to HPA translation helper

[Alison Schofield]

On Wed, Nov 30, 2022 at 04:38:09PM +0000, Jonathan Cameron wrote:
> On Tue, 22 Nov 2022 15:07:48 -0800
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > CXL devices may report errors and events using their DPA (device
> > physical address). When a CXL device contributes capacity to a
> > CXL region, the device's physical addresses are mapped to HPA's.
> > (host physical addresses)
> > 
> > Provide a helper to calculate the HPA when given a DPA, a region,
> > and the devices position in the region interleave.
> > 
> > Verify that the HPA is within the expected ranges that this device
> > contributes to the region interleave set.
> > 
> > The initial use case is translating the DPAs that CXL devices
> > report in media error records.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/core.h   |  3 ++
> >  drivers/cxl/core/region.c | 80 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 83 insertions(+)
> > 
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index 1d8f87be283f..72b58e53c394 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -67,6 +67,9 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
> >  	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
> >  }
> >  
> > +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> > +		   struct cxl_endpoint_decoder *cxled);
> > +
> >  int cxl_memdev_init(void);
> >  void cxl_memdev_exit(void);
> >  void cxl_mbox_init(void);
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index f9ae5ad284ff..c847517e766c 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1865,6 +1865,86 @@ static void cxlr_pmem_unregister(void *dev)
> >  	device_unregister(dev);
> >  }
> >  
> > +static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
> > +{
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	int gran = p->interleave_granularity;
> > +	int ways = p->interleave_ways;
> > +	u64 stride;
> > +
> > +	/* Is the hpa within this region at all */
> > +	if (hpa < p->res->start || hpa > p->res->end) {
> > +		dev_dbg(&cxlr->dev,
> > +			"Addr trans fail: hpa 0x%llx not in region\n", hpa);
> > +		return false;
> > +	}
> > +	/* Is the hpa in an expected stride for its pos(-ition) */
> > +	stride = p->res->start + pos * gran;
> > +	do {
> > +		if (hpa >= stride && hpa <= stride + gran - 1)
> > +			return true;
> > +
> > +		stride = stride + ways * gran;
> > +	} while (stride < p->res->end);
> > +
> > +	dev_dbg(&cxlr->dev,
> > +		"Addr trans fail: hpa 0x%llx not in any stride\n", hpa);
> > +
> > +	return false;
> > +}
> > +
> > +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> > +		   struct cxl_endpoint_decoder *cxled)
> > +{
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	u64 dpa_offset, hpa_offset, hpa;
> > +	int rc, pos = cxled->pos;
> > +	u16 eig;
> > +	u8 eiw;
> > +
> > +	rc = ways_to_cxl(p->interleave_ways, &eiw);
> > +	if (rc)
> > +		return rc;
> > +	rc = granularity_to_cxl(p->interleave_granularity, &eig);
> > +	if (rc)
> > +		return rc;
> returning integer as u64, so it won't end up as a useful error code.
> 

You'll see use of ULLONG_MAX as the error rc in the next patch.
Alas, I also decided NOT to keep checking the rc on these functions,
as the validity was checked at the time of storage of ways and gran.

Ducks a bit,
Alison

> > +
snip

Re: [PATCH 1/4] cxl/region: Add a DPA to HPA translation helper

[Alison Schofield]

On Wed, Nov 30, 2022 at 04:27:36PM +0000, Jonathan Cameron wrote:
> On Tue, 22 Nov 2022 15:07:48 -0800
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > CXL devices may report errors and events using their DPA (device
> > physical address). When a CXL device contributes capacity to a
> > CXL region, the device's physical addresses are mapped to HPA's.
> > (host physical addresses)
> > 
> > Provide a helper to calculate the HPA when given a DPA, a region,
> > and the devices position in the region interleave.
> 
> device's 
> 
> > 
> > Verify that the HPA is within the expected ranges that this device
> > contributes to the region interleave set.
> > 
> > The initial use case is translating the DPAs that CXL devices
> > report in media error records.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> I'm not following the maths for the 3/6/12 way interleave cases.
> 
Yeah, they were off. I reworked this to strictly follow the 
spec.

> > ---
> >  drivers/cxl/core/core.h   |  3 ++
> >  drivers/cxl/core/region.c | 80 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 83 insertions(+)
> > 
> > diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
> > index 1d8f87be283f..72b58e53c394 100644
> > --- a/drivers/cxl/core/core.h
> > +++ b/drivers/cxl/core/core.h
> > @@ -67,6 +67,9 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
> >  	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
> >  }
> >  
> > +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> > +		   struct cxl_endpoint_decoder *cxled);
> > +
> >  int cxl_memdev_init(void);
> >  void cxl_memdev_exit(void);
> >  void cxl_mbox_init(void);
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index f9ae5ad284ff..c847517e766c 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1865,6 +1865,86 @@ static void cxlr_pmem_unregister(void *dev)
> >  	device_unregister(dev);
> >  }
> >  
> > +static bool cxl_is_hpa_in_range(u64 hpa, struct cxl_region *cxlr, int pos)
> > +{
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	int gran = p->interleave_granularity;
> > +	int ways = p->interleave_ways;
> > +	u64 stride;
> > +
> > +	/* Is the hpa within this region at all */
> > +	if (hpa < p->res->start || hpa > p->res->end) {
> > +		dev_dbg(&cxlr->dev,
> > +			"Addr trans fail: hpa 0x%llx not in region\n", hpa);
> > +		return false;
> > +	}
> > +	/* Is the hpa in an expected stride for its pos(-ition) */
> > +	stride = p->res->start + pos * gran;
> > +	do {
> > +		if (hpa >= stride && hpa <= stride + gran - 1)
> 	< stride + gran seems simpler.
> 

Got it.

> > +			return true;
> > +
> > +		stride = stride + ways * gran;
> > +	} while (stride < p->res->end);
> 
> That's going to take a 'while' with a large region.  Can we do something quicker
> along the lines of...
> 
> Something like (untested)
> 	u64 offset = hpa - p->res_start;
> 
> 	/* Offset within the right 'stride' */
> 	offset = offset % (gran * ways);
> 
> 	if (offset >= pos * gran && offset < (pos + 1) * gran)
> 		return true;
> 
> ..

I did adopt this calc you offered up here.

I kept this range check function in the new patch, so you'll see
it again.

> > +
> > +	dev_dbg(&cxlr->dev,
> > +		"Addr trans fail: hpa 0x%llx not in any stride\n", hpa);
> > +
> > +	return false;
> > +}
> > +
> > +u64 cxl_dpa_to_hpa(u64 dpa, struct cxl_region *cxlr,
> > +		   struct cxl_endpoint_decoder *cxled)
> > +{
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	u64 dpa_offset, hpa_offset, hpa;
> > +	int rc, pos = cxled->pos;
> > +	u16 eig;
> > +	u8 eiw;
> > +
> > +	rc = ways_to_cxl(p->interleave_ways, &eiw);
> I lost track a bit, but I think we ended up with einw?

Yes. Above and below are updated w latest names:
ways_to_eiw() and granularity_to_eig()

> > +	if (rc)
> > +		return rc;
> > +	rc = granularity_to_cxl(p->interleave_granularity, &eig);
> > +	if (rc)
> > +		return rc;
> > +
> > +	/*
> > +	 * Reverse the HPA->DPA decode logic defined
> > +	 * in the CXL Spec 3.0 Section 8.2.4.19.13
> > +	 * Implementation Note: Device Decode Logic
> Short line wrap. Probably want to go nearer 80 chars
> 
> > +	 *
> > +	 * The device position in the region interleave
> > +	 * set was removed in the HPA->DPA translation.
> > +	 * Put it back to reconstruct the HPA.
> > +	 */
> > +
> > +	/* Remove the dpa base */
> > +	dpa_offset = dpa - cxl_dpa_resource_start(cxled);
> > +
> > +	/* Restore the position */
> > +	if (eiw <= 8) {
> 
> This looks to be the power of 2 version, so should that be eiw < 8?
> 

This calc section got a rewrite w 2 cases eiw < 8 and 'all others'.

> > +		hpa_offset = (dpa_offset & GENMASK_ULL(51, eig + 8)) << eiw;
> > +		hpa_offset |= GENMASK_ULL(eig + 8 + eiw, eig + 8)
> > +			      & (pos << (eig + 8));
> 
> Why is the masking needed here?  I think pos should always fit in the iw bits.
Not needed.

> 
> > +	}
> > +	if (eiw == 9)
> 
> else if would show clearly that only one of these ifs is true.
>  
> > +		hpa_offset |= BIT(eig + eiw) & (pos & 0x01);
> 
> I don't follow this logic. hpa_offset hasn't been set to anything before
> this point + the right hand side of the above will always be 0 (I think...)
> 
> 
> > +	if (eiw == 10)
> else if
> > +		hpa_offset |= GENMASK_ULL(eig + eiw, eig + 8) & (pos & 0x03);

Rework has 2 cases (eiw < 8), else all others.
I hope you can pick up the review in the new patch.

Thanks Jonathan!
Alison

> > +
> > +	/* The lower bits remain unchanged */
> > +	hpa_offset |= dpa_offset & GENMASK_ULL(eig + 7, 0);
> > +
> > +	/* Apply the hpa_offset to region base address */
> > +	hpa = hpa_offset + p->res->start;
> > +
> > +	if (!cxl_is_hpa_in_range(hpa, cxlr, cxled->pos))
> > +		return 0;
> > +
> > +	return hpa;
> > +}
> > +
> >  /**
> >   * devm_cxl_add_pmem_region() - add a cxl_region-to-nd_region bridge
> >   * @cxlr: parent CXL region for this pmem region bridge device
>