[PATCH v5 0/12] cxl: Add support to report region access coordinates to numa nodes

[PATCH v5 0/12] cxl: Add support to report region access coordinates to numa nodes

[Dave Jiang]

Hi Rafael,
Please review patches 1-4,10,11 and ack if they look ok to you. Thank you!

Hi Greg,
Please review patch 2 and 11 and ack the numa node bits if they look ok to you. Thank you!

v5:
- Fix various 0-day issues
- Remove EXPORT_SYMBOL for cxl_coords_combine() (Dan)
- Rebased against fixes series for qos_class [1].

v4:
- Introduce access class 0 and 1 for CXL access coordinates.
- See individual patches for detailed change log if applicable.

v3:
- Make attributes not visible if no data. (Jonathan)
- Fix documentation verbiage. (Jonathan)
- Check against read bandwidth instead of write bandwidth due to future RO devices. (Jonathan)
- Export node_set_perf_attrs() to all namespaces. (Jonathan)
- Remove setting of coordinate access level 1. (Jonathan)

v2:
- Move calculation function to core/cdat.c due to QTG series changes
- Make cxlr->coord static (Dan)
- Move calculation to cxl_region_attach to be under cxl_dpa_rwsem (Dan)
- Normalize perf latency numbers to nanoseconds (Brice)
- Update documentation with units and initiator details (Brice, Dan)
- Fix notifier return values (Dan)
- Use devm_add_action_or_reset() to unregister memory notifier (Dan)

This series adds support for computing the performance data of a CXL region
and also updates the performance data to the NUMA node. This series depends
on the CXL QOS class series that's pending 6.8 pull request.

CXL memory devices already attached before boot are enumerated by the BIOS.
The SRAT and HMAT tables are properly setup to including memory regions
enumerated from those CXL memory devices. For regions not programmed or a
hot-plugged CXL memory device, the BIOS does not have the relevant
information and the performance data has to be caluclated by the driver
post region assembly.

According to numaperf documentation [2] there are 2 access classes defined
for performance between an initiator node and a memory target node. Access
class "0" describes attributes between a memory target and the highest
performing initator local to the target. In this case the initiator can be
a CPU or an I/O initiator such as a GPU or NIC. Access class "1" describes
attributes between a memory target and the nearest CPU node. Both access
classes are calculated for the CXL memory target and updated for NUMA nodes
through HMAT_REPORTING code or directly depending on if the NUMA node is
described by the ACPI SRAT table.

Recall from qos_class series (v6.8) that the performance data for the ranges
of a CXL memory device is computed and cached. A CXL memory region can be
backed by one or more devices. Thus the performance data would be the
aggregated bandwidth of all devices that back a region and the worst
latency out of all devices backing the region.

See git branch [3] for convenience.

[1]: https://lore.kernel.org/linux-cxl/20240206190431.1810289-1-dave.jiang@intel.com/T/#t 
[2]: https://www.kernel.org/doc/Documentation/admin-guide/mm/numaperf.rst
[3]: https://git.kernel.org/pub/scm/linux/kernel/git/djiang/linux.git/log/?h=cxl-hmem-report

[PATCH v5 01/12] ACPI: HMAT: Remove register of memory node for generic target

[Dave Jiang]

For generic targets, there's no reason to call
register_memory_node_under_compute_node() with the access levels that are
only visible to HMAT handling code. Only update the attributes and rename
hmat_register_generic_target_initiators() to hmat_update_generic_target().

Fixes: a3a3e341f169 ("acpi: numa: Add setting of generic port system locality attributes")
Cc: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index d6b85f0f6082..a26e7793ec4e 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -770,12 +770,12 @@ static void __hmat_register_target_initiators(struct memory_target *target,
 	}
 }
 
-static void hmat_register_generic_target_initiators(struct memory_target *target)
+static void hmat_update_generic_target(struct memory_target *target)
 {
 	static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
 
-	__hmat_register_target_initiators(target, p_nodes,
-					  NODE_ACCESS_CLASS_GENPORT_SINK);
+	hmat_update_target_attrs(target, p_nodes,
+				 NODE_ACCESS_CLASS_GENPORT_SINK);
 }
 
 static void hmat_register_target_initiators(struct memory_target *target)
@@ -835,7 +835,7 @@ static void hmat_register_target(struct memory_target *target)
 	 */
 	mutex_lock(&target_lock);
 	if (*(u16 *)target->gen_port_device_handle) {
-		hmat_register_generic_target_initiators(target);
+		hmat_update_generic_target(target);
 		target->registered = true;
 	}
 	mutex_unlock(&target_lock);
-- 
2.43.0

[PATCH v5 02/12] base/node / ACPI: Enumerate node access class for 'struct access_coordinate'

[Dave Jiang]

Both generic node and HMAT handling code have been using magic numbers to
indicate access classes for 'struct access_coordinate'. Introduce enums to
enumerate the access0 and access1 classes shared by the two subsystems.
Update the function parameters and callers as appropriate to utilize the
new enum.

Access0 is named to ACCESS_COORDINATE_LOCAL in order to indicate that the
access class is for 'struct access_coordinate' between a target node and
the nearest initiator node.

Access1 is named to ACCESS_COORDINATE_CPU in order to indicate that the
access class is for 'struct access_coordinate' between a target node and
the nearest CPU node.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c | 26 ++++++++++++++------------
 drivers/base/node.c      |  6 +++---
 include/linux/node.h     | 18 +++++++++++++++---
 3 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index a26e7793ec4e..e0144cfbf1f3 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -59,9 +59,7 @@ struct target_cache {
 };
 
 enum {
-	NODE_ACCESS_CLASS_0 = 0,
-	NODE_ACCESS_CLASS_1,
-	NODE_ACCESS_CLASS_GENPORT_SINK,
+	NODE_ACCESS_CLASS_GENPORT_SINK = ACCESS_COORDINATE_MAX,
 	NODE_ACCESS_CLASS_MAX,
 };
 
@@ -374,11 +372,11 @@ static __init void hmat_update_target(unsigned int tgt_pxm, unsigned int init_px
 
 	if (target && target->processor_pxm == init_pxm) {
 		hmat_update_target_access(target, type, value,
-					  NODE_ACCESS_CLASS_0);
+					  ACCESS_COORDINATE_LOCAL);
 		/* If the node has a CPU, update access 1 */
 		if (node_state(pxm_to_node(init_pxm), N_CPU))
 			hmat_update_target_access(target, type, value,
-						  NODE_ACCESS_CLASS_1);
+						  ACCESS_COORDINATE_CPU);
 	}
 }
 
@@ -709,7 +707,8 @@ static void hmat_update_target_attrs(struct memory_target *target,
 	 */
 	if (target->processor_pxm != PXM_INVAL) {
 		cpu_nid = pxm_to_node(target->processor_pxm);
-		if (access == 0 || node_state(cpu_nid, N_CPU)) {
+		if (access == ACCESS_COORDINATE_LOCAL ||
+		    node_state(cpu_nid, N_CPU)) {
 			set_bit(target->processor_pxm, p_nodes);
 			return;
 		}
@@ -737,7 +736,8 @@ static void hmat_update_target_attrs(struct memory_target *target,
 		list_for_each_entry(initiator, &initiators, node) {
 			u32 value;
 
-			if (access == 1 && !initiator->has_cpu) {
+			if (access == ACCESS_COORDINATE_CPU &&
+			    !initiator->has_cpu) {
 				clear_bit(initiator->processor_pxm, p_nodes);
 				continue;
 			}
@@ -782,8 +782,10 @@ static void hmat_register_target_initiators(struct memory_target *target)
 {
 	static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
 
-	__hmat_register_target_initiators(target, p_nodes, 0);
-	__hmat_register_target_initiators(target, p_nodes, 1);
+	__hmat_register_target_initiators(target, p_nodes,
+					  ACCESS_COORDINATE_LOCAL);
+	__hmat_register_target_initiators(target, p_nodes,
+					  ACCESS_COORDINATE_CPU);
 }
 
 static void hmat_register_target_cache(struct memory_target *target)
@@ -854,8 +856,8 @@ static void hmat_register_target(struct memory_target *target)
 	if (!target->registered) {
 		hmat_register_target_initiators(target);
 		hmat_register_target_cache(target);
-		hmat_register_target_perf(target, NODE_ACCESS_CLASS_0);
-		hmat_register_target_perf(target, NODE_ACCESS_CLASS_1);
+		hmat_register_target_perf(target, ACCESS_COORDINATE_LOCAL);
+		hmat_register_target_perf(target, ACCESS_COORDINATE_CPU);
 		target->registered = true;
 	}
 	mutex_unlock(&target_lock);
@@ -927,7 +929,7 @@ static int hmat_calculate_adistance(struct notifier_block *self,
 		return NOTIFY_OK;
 
 	mutex_lock(&target_lock);
-	hmat_update_target_attrs(target, p_nodes, 1);
+	hmat_update_target_attrs(target, p_nodes, ACCESS_COORDINATE_CPU);
 	mutex_unlock(&target_lock);
 
 	perf = &target->coord[1];
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 1c05640461dd..a73b0c9a401a 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -126,7 +126,7 @@ static void node_access_release(struct device *dev)
 }
 
 static struct node_access_nodes *node_init_node_access(struct node *node,
-						       unsigned int access)
+						       enum access_coordinate_class access)
 {
 	struct node_access_nodes *access_node;
 	struct device *dev;
@@ -191,7 +191,7 @@ static struct attribute *access_attrs[] = {
  * @access: The access class the for the given attributes
  */
 void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord,
-			 unsigned int access)
+			 enum access_coordinate_class access)
 {
 	struct node_access_nodes *c;
 	struct node *node;
@@ -689,7 +689,7 @@ int register_cpu_under_node(unsigned int cpu, unsigned int nid)
  */
 int register_memory_node_under_compute_node(unsigned int mem_nid,
 					    unsigned int cpu_nid,
-					    unsigned int access)
+					    enum access_coordinate_class access)
 {
 	struct node *init_node, *targ_node;
 	struct node_access_nodes *initiator, *target;
diff --git a/include/linux/node.h b/include/linux/node.h
index 25b66d705ee2..dfc004e4bee7 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -34,6 +34,18 @@ struct access_coordinate {
 	unsigned int write_latency;
 };
 
+/*
+ * ACCESS_COORDINATE_LOCAL correlates to ACCESS CLASS 0
+ *	- access_coordinate between target node and nearest initiator node
+ * ACCESS_COORDINATE_CPU correlates to ACCESS CLASS 1
+ *	- access_coordinate between target node and nearest CPU node
+ */
+enum access_coordinate_class {
+	ACCESS_COORDINATE_LOCAL,
+	ACCESS_COORDINATE_CPU,
+	ACCESS_COORDINATE_MAX
+};
+
 enum cache_indexing {
 	NODE_CACHE_DIRECT_MAP,
 	NODE_CACHE_INDEXED,
@@ -66,7 +78,7 @@ struct node_cache_attrs {
 #ifdef CONFIG_HMEM_REPORTING
 void node_add_cache(unsigned int nid, struct node_cache_attrs *cache_attrs);
 void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord,
-			 unsigned access);
+			 enum access_coordinate_class access);
 #else
 static inline void node_add_cache(unsigned int nid,
 				  struct node_cache_attrs *cache_attrs)
@@ -75,7 +87,7 @@ static inline void node_add_cache(unsigned int nid,
 
 static inline void node_set_perf_attrs(unsigned int nid,
 				       struct access_coordinate *coord,
-				       unsigned access)
+				       enum access_coordinate_class access)
 {
 }
 #endif
@@ -137,7 +149,7 @@ extern void unregister_memory_block_under_nodes(struct memory_block *mem_blk);
 
 extern int register_memory_node_under_compute_node(unsigned int mem_nid,
 						   unsigned int cpu_nid,
-						   unsigned access);
+						   enum access_coordinate_class access);
 #else
 static inline void node_dev_init(void)
 {
-- 
2.43.0

[PATCH v5 03/12] ACPI: HMAT: Introduce 2 levels of generic port access class

[Dave Jiang]

In order to compute access0 and access1 classes for CXL memory, 2 levels
of generic port information must be stored. Access0 will indicate the
generic port access coordinates to the closest initiator and access1
will indicate the generic port access coordinates to the cloest CPU.

Cc: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index e0144cfbf1f3..8dbb0e366059 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -59,7 +59,8 @@ struct target_cache {
 };
 
 enum {
-	NODE_ACCESS_CLASS_GENPORT_SINK = ACCESS_COORDINATE_MAX,
+	NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL = ACCESS_COORDINATE_MAX,
+	NODE_ACCESS_CLASS_GENPORT_SINK_CPU,
 	NODE_ACCESS_CLASS_MAX,
 };
 
@@ -141,7 +142,7 @@ int acpi_get_genport_coordinates(u32 uid,
 	if (!target)
 		return -ENOENT;
 
-	*coord = target->coord[NODE_ACCESS_CLASS_GENPORT_SINK];
+	*coord = target->coord[NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL];
 
 	return 0;
 }
@@ -695,7 +696,8 @@ static void hmat_update_target_attrs(struct memory_target *target,
 	int i;
 
 	/* Don't update for generic port if there's no device handle */
-	if (access == NODE_ACCESS_CLASS_GENPORT_SINK &&
+	if ((access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL ||
+	     access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&
 	    !(*(u16 *)target->gen_port_device_handle))
 		return;
 
@@ -707,7 +709,8 @@ static void hmat_update_target_attrs(struct memory_target *target,
 	 */
 	if (target->processor_pxm != PXM_INVAL) {
 		cpu_nid = pxm_to_node(target->processor_pxm);
-		if (access == ACCESS_COORDINATE_LOCAL ||
+		if ((access == ACCESS_COORDINATE_LOCAL ||
+		     access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL) &&
 		    node_state(cpu_nid, N_CPU)) {
 			set_bit(target->processor_pxm, p_nodes);
 			return;
@@ -736,7 +739,8 @@ static void hmat_update_target_attrs(struct memory_target *target,
 		list_for_each_entry(initiator, &initiators, node) {
 			u32 value;
 
-			if (access == ACCESS_COORDINATE_CPU &&
+			if ((access == ACCESS_COORDINATE_CPU &&
+			     access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&
 			    !initiator->has_cpu) {
 				clear_bit(initiator->processor_pxm, p_nodes);
 				continue;
@@ -775,7 +779,9 @@ static void hmat_update_generic_target(struct memory_target *target)
 	static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
 
 	hmat_update_target_attrs(target, p_nodes,
-				 NODE_ACCESS_CLASS_GENPORT_SINK);
+				 NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL);
+	hmat_update_target_attrs(target, p_nodes,
+				 NODE_ACCESS_CLASS_GENPORT_SINK_CPU);
 }
 
 static void hmat_register_target_initiators(struct memory_target *target)
-- 
2.43.0

[PATCH v5 04/12] ACPI: HMAT / cxl: Add retrieval of generic port coordinates for both access classes

[Dave Jiang]

Update acpi_get_genport_coordinates() to allow retrieval of both access
classes of the 'struct access_coordinate' for a generic target. The update
will allow CXL code to compute access coordinates for both access class.

Cc: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c | 8 ++++++--
 drivers/cxl/acpi.c       | 8 +++++---
 drivers/cxl/core/port.c  | 2 +-
 drivers/cxl/cxl.h        | 2 +-
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 8dbb0e366059..5be79896bd08 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -126,7 +126,8 @@ static struct memory_target *acpi_find_genport_target(u32 uid)
 /**
  * acpi_get_genport_coordinates - Retrieve the access coordinates for a generic port
  * @uid: ACPI unique id
- * @coord: The access coordinates written back out for the generic port
+ * @coord: The access coordinates written back out for the generic port.
+ *	   Expect 2 levels array.
  *
  * Return: 0 on success. Errno on failure.
  *
@@ -142,7 +143,10 @@ int acpi_get_genport_coordinates(u32 uid,
 	if (!target)
 		return -ENOENT;
 
-	*coord = target->coord[NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL];
+	coord[ACCESS_COORDINATE_LOCAL] =
+		target->coord[NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL];
+	coord[ACCESS_COORDINATE_CPU] =
+		target->coord[NODE_ACCESS_CLASS_GENPORT_SINK_CPU];
 
 	return 0;
 }
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index dcf2b39e1048..2bf543221c6f 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -520,13 +520,15 @@ static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport)
 	if (kstrtou32(acpi_device_uid(hb), 0, &uid))
 		return -EINVAL;
 
-	rc = acpi_get_genport_coordinates(uid, &dport->hb_coord);
+	rc = acpi_get_genport_coordinates(uid, dport->hb_coord);
 	if (rc < 0)
 		return rc;
 
 	/* Adjust back to picoseconds from nanoseconds */
-	dport->hb_coord.read_latency *= 1000;
-	dport->hb_coord.write_latency *= 1000;
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		dport->hb_coord[i].read_latency *= 1000;
+		dport->hb_coord[i].write_latency *= 1000;
+	}
 
 	return 0;
 }
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index e59d9d37aa65..612bf7e1e847 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2152,7 +2152,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 	}
 
 	/* Augment with the generic port (host bridge) perf data */
-	combine_coordinates(&c, &dport->hb_coord);
+	combine_coordinates(&c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
 
 	/* Get the calculated PCI paths bandwidth */
 	pdev = to_pci_dev(port->uport_dev->parent);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 003feebab79b..fe7448f2745e 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -671,7 +671,7 @@ struct cxl_dport {
 	struct cxl_port *port;
 	struct cxl_regs regs;
 	struct access_coordinate sw_coord;
-	struct access_coordinate hb_coord;
+	struct access_coordinate hb_coord[ACCESS_COORDINATE_MAX];
 	long link_latency;
 };
 
-- 
2.43.0

[PATCH v5 05/12] cxl: Split out combine_coordinates() for common shared usage

[Dave Jiang]

Refactor the common code of combining coordinates in order to reduce code.
Create a new function cxl_cooordinates_combine() it combine two 'struct
access_coordinate'.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v5:
- Fix comment header (0-day)
- Remove EXPORT_SYMBOL (Dan)
---
 drivers/cxl/core/cdat.c | 32 +++++++++++++++++++++++---------
 drivers/cxl/core/port.c | 18 ++----------------
 drivers/cxl/cxl.h       |  4 ++++
 3 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 08fd0baea7a0..096320390ad9 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -185,15 +185,7 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 	xa_for_each(dsmas_xa, index, dent) {
 		int qos_class;
 
-		dent->coord.read_latency = dent->coord.read_latency +
-					   c.read_latency;
-		dent->coord.write_latency = dent->coord.write_latency +
-					    c.write_latency;
-		dent->coord.read_bandwidth = min_t(int, c.read_bandwidth,
-						   dent->coord.read_bandwidth);
-		dent->coord.write_bandwidth = min_t(int, c.write_bandwidth,
-						    dent->coord.write_bandwidth);
-
+		cxl_coordinates_combine(&dent->coord, &dent->coord, &c);
 		dent->entries = 1;
 		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
 					      &qos_class);
@@ -484,4 +476,26 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
 }
 EXPORT_SYMBOL_NS_GPL(cxl_switch_parse_cdat, CXL);
 
+/**
+ * cxl_coordinates_combine - Combine the two input coordinates into the first
+ *
+ * @out: Output coordinate of c1 and c2 combined
+ * @c1: first coordinate, to be written to
+ * @c2: second coordinate
+ */
+void cxl_coordinates_combine(struct access_coordinate *out,
+			     struct access_coordinate *c1,
+			     struct access_coordinate *c2)
+{
+		if (c2->write_bandwidth)
+			out->write_bandwidth = min(c1->write_bandwidth,
+						   c2->write_bandwidth);
+		out->write_latency = c1->write_latency + c2->write_latency;
+
+		if (c2->read_bandwidth)
+			out->read_bandwidth = min(c1->read_bandwidth,
+						  c2->read_bandwidth);
+		out->read_latency = c1->read_latency + c2->read_latency;
+}
+
 MODULE_IMPORT_NS(CXL);
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index 612bf7e1e847..af9458b2678c 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2096,20 +2096,6 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
 }
 EXPORT_SYMBOL_NS_GPL(schedule_cxl_memdev_detach, CXL);
 
-static void combine_coordinates(struct access_coordinate *c1,
-				struct access_coordinate *c2)
-{
-		if (c2->write_bandwidth)
-			c1->write_bandwidth = min(c1->write_bandwidth,
-						  c2->write_bandwidth);
-		c1->write_latency += c2->write_latency;
-
-		if (c2->read_bandwidth)
-			c1->read_bandwidth = min(c1->read_bandwidth,
-						 c2->read_bandwidth);
-		c1->read_latency += c2->read_latency;
-}
-
 /**
  * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
  *				   of CXL path
@@ -2143,7 +2129,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 	 * nothing to gather.
 	 */
 	while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
-		combine_coordinates(&c, &dport->sw_coord);
+		cxl_coordinates_combine(&c, &c, &dport->sw_coord);
 		c.write_latency += dport->link_latency;
 		c.read_latency += dport->link_latency;
 
@@ -2152,7 +2138,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 	}
 
 	/* Augment with the generic port (host bridge) perf data */
-	combine_coordinates(&c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
+	cxl_coordinates_combine(&c, &c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
 
 	/* Get the calculated PCI paths bandwidth */
 	pdev = to_pci_dev(port->uport_dev->parent);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index fe7448f2745e..fab2da4b1f04 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -882,6 +882,10 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 
 void cxl_memdev_update_perf(struct cxl_memdev *cxlmd);
 
+void cxl_coordinates_combine(struct access_coordinate *out,
+			     struct access_coordinate *c1,
+			     struct access_coordinate *c2);
+
 /*
  * Unit test builds overrides this to __weak, find the 'strong' version
  * of these symbols in tools/testing/cxl/.
-- 
2.43.0

[PATCH v5 06/12] cxl: Split out host bridge access coordinates

[Dave Jiang]

The difference between access class 0 and access class 1 for 'struct
access_coordinate', if any, is that class 0 is for the distance from
the target to the closest initiator and that class 1 is for the distance
from the target to the cloest CPU. For CXL memory, the nearest initiator
may not necessarily be a CPU node. The performance path from the CXL
endpoint to the host bridge should remain the same. However, the numbers
extracted and stored from HMAT is the difference for the two access
classes. Split out the performance numbers for the host bridge (generic
target) from the calculation of the entire path in order to allow
calculation of both access classes for a CXL region.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/cdat.c | 28 ++++++++++++++++++++++------
 drivers/cxl/core/port.c | 34 +++++++++++++++++++++++++++++++---
 drivers/cxl/cxl.h       |  2 ++
 3 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 096320390ad9..79844874a34b 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -162,15 +162,22 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
 static int cxl_port_perf_data_calculate(struct cxl_port *port,
 					struct xarray *dsmas_xa)
 {
-	struct access_coordinate c;
+	struct access_coordinate ep_c;
+	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
 	struct dsmas_entry *dent;
 	int valid_entries = 0;
 	unsigned long index;
 	int rc;
 
-	rc = cxl_endpoint_get_perf_coordinates(port, &c);
+	rc = cxl_endpoint_get_perf_coordinates(port, &ep_c);
 	if (rc) {
-		dev_dbg(&port->dev, "Failed to retrieve perf coordinates.\n");
+		dev_dbg(&port->dev, "Failed to retrieve ep perf coordinates.\n");
+		return rc;
+	}
+
+	rc = cxl_hb_get_perf_coordinates(port, coord);
+	if (rc)  {
+		dev_dbg(&port->dev, "Failed to retrieve hb perf coordinates.\n");
 		return rc;
 	}
 
@@ -185,10 +192,19 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 	xa_for_each(dsmas_xa, index, dent) {
 		int qos_class;
 
-		cxl_coordinates_combine(&dent->coord, &dent->coord, &c);
+		cxl_coordinates_combine(&dent->coord, &dent->coord, &ep_c);
+		/*
+		 * Keeping the host bridge coordinates separate from the dsmas
+		 * coordinates in order to allow calculation of access class
+		 * 0 and 1 for region later.
+		 */
+		cxl_coordinates_combine(&coord[ACCESS_COORDINATE_LOCAL],
+					&coord[ACCESS_COORDINATE_LOCAL],
+					&dent->coord);
 		dent->entries = 1;
-		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
-					      &qos_class);
+		rc = cxl_root->ops->qos_class(cxl_root,
+					      &coord[ACCESS_COORDINATE_LOCAL],
+					      1, &qos_class);
 		if (rc != 1)
 			continue;
 
diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index af9458b2678c..e8029170b8c6 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -2096,6 +2096,37 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
 }
 EXPORT_SYMBOL_NS_GPL(schedule_cxl_memdev_detach, CXL);
 
+/**
+ * cxl_hb_get_perf_coordinates - Retrieve performance numbers from host bridge
+ *
+ * @port: endpoint cxl_port
+ * @coord: output access coordinates
+ *
+ * Return: errno on failure, 0 on success.
+ */
+int cxl_hb_get_perf_coordinates(struct cxl_port *port,
+				struct access_coordinate *coord)
+{
+	struct cxl_port *iter = port;
+	struct cxl_dport *dport;
+
+	if (!is_cxl_endpoint(port))
+		return -EINVAL;
+
+	dport = iter->parent_dport;
+	while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
+		iter = to_cxl_port(iter->dev.parent);
+		dport = iter->parent_dport;
+	}
+
+	coord[ACCESS_COORDINATE_LOCAL] =
+		dport->hb_coord[ACCESS_COORDINATE_LOCAL];
+	coord[ACCESS_COORDINATE_CPU] =
+		dport->hb_coord[ACCESS_COORDINATE_CPU];
+
+	return 0;
+}
+
 /**
  * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
  *				   of CXL path
@@ -2137,9 +2168,6 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 		dport = iter->parent_dport;
 	}
 
-	/* Augment with the generic port (host bridge) perf data */
-	cxl_coordinates_combine(&c, &c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
-
 	/* Get the calculated PCI paths bandwidth */
 	pdev = to_pci_dev(port->uport_dev->parent);
 	bw = pcie_bandwidth_available(pdev, NULL, NULL, NULL);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index fab2da4b1f04..de477eb7f5d5 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -879,6 +879,8 @@ void cxl_switch_parse_cdat(struct cxl_port *port);
 
 int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 				      struct access_coordinate *coord);
+int cxl_hb_get_perf_coordinates(struct cxl_port *port,
+				struct access_coordinate *coord);
 
 void cxl_memdev_update_perf(struct cxl_memdev *cxlmd);
 
-- 
2.43.0

[PATCH v5 07/12] cxl: Move QoS class to be calculated from the nearest CPU

[Dave Jiang]

Retrieve the qos_class (QTG ID) using the access coordinates from the
nearest CPU rather than the nearst initiator that may not be a CPU.
This may be the more appropriate number that applications care about.

Link: https://lore.kernel.org/linux-cxl/20240112113023.00006c50@Huawei.com/
Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/cdat.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 79844874a34b..bd0ff3cebb8c 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -198,12 +198,12 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
 		 * coordinates in order to allow calculation of access class
 		 * 0 and 1 for region later.
 		 */
-		cxl_coordinates_combine(&coord[ACCESS_COORDINATE_LOCAL],
-					&coord[ACCESS_COORDINATE_LOCAL],
+		cxl_coordinates_combine(&coord[ACCESS_COORDINATE_CPU],
+					&coord[ACCESS_COORDINATE_CPU],
 					&dent->coord);
 		dent->entries = 1;
 		rc = cxl_root->ops->qos_class(cxl_root,
-					      &coord[ACCESS_COORDINATE_LOCAL],
+					      &coord[ACCESS_COORDINATE_CPU],
 					      1, &qos_class);
 		if (rc != 1)
 			continue;
-- 
2.43.0

[PATCH v5 08/12] cxl: Set cxlmd->endpoint before adding port device

[Dave Jiang]

Move setting of cxlmd->endpoint to before calling add_device() on the port
device. Otherwise when referencing cxlmd->endpoint in region discovery code
that is triggered by the port driver probe function, the endpoint port
pointer is not valid.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index e8029170b8c6..2f2b7af9275e 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -822,6 +822,7 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
 		 */
 		port->reg_map = cxlds->reg_map;
 		port->reg_map.host = &port->dev;
+		cxlmd->endpoint = port;
 	} else if (parent_dport) {
 		rc = dev_set_name(dev, "port%d", port->id);
 		if (rc)
@@ -1374,7 +1375,6 @@ int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)
 
 	get_device(host);
 	get_device(&endpoint->dev);
-	cxlmd->endpoint = endpoint;
 	cxlmd->depth = endpoint->depth;
 	return devm_add_action_or_reset(dev, delete_endpoint, cxlmd);
 }
-- 
2.43.0

[PATCH v5 09/12] cxl/region: Calculate performance data for a region

[Dave Jiang]

Calculate and store the performance data for a CXL region. Find the worst
read and write latency for all the included ranges from each of the devices
that attributes to the region and designate that as the latency data. Sum
all the read and write bandwidth data for each of the device region and
that is the total bandwidth for the region.

The perf list is expected to be constructed before the endpoint decoders
are registered and thus there should be no early reading of the entries
from the region assemble action. The calling of the region qos calculate
function is under the protection of cxl_dpa_rwsem and will ensure that
all DPA associated work has completed.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/cdat.c   | 65 +++++++++++++++++++++++++++++++++++++++
 drivers/cxl/core/region.c |  2 ++
 drivers/cxl/cxl.h         |  4 +++
 3 files changed, 71 insertions(+)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index bd0ff3cebb8c..bf7c3c33c7ee 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -9,6 +9,7 @@
 #include "cxlmem.h"
 #include "core.h"
 #include "cxl.h"
+#include "core.h"
 
 struct dsmas_entry {
 	struct range dpa_range;
@@ -515,3 +516,67 @@ void cxl_coordinates_combine(struct access_coordinate *out,
 }
 
 MODULE_IMPORT_NS(CXL);
+
+void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
+				    struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_port *port = cxlmd->endpoint;
+	struct cxl_dev_state *cxlds = cxlmd->cxlds;
+	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
+	struct access_coordinate hb_coord[ACCESS_COORDINATE_MAX];
+	struct access_coordinate coord;
+	struct range dpa = {
+			.start = cxled->dpa_res->start,
+			.end = cxled->dpa_res->end,
+	};
+	struct cxl_dpa_perf *perf;
+	int rc;
+
+	switch (cxlr->mode) {
+	case CXL_DECODER_RAM:
+		perf = &mds->ram_perf;
+		break;
+	case CXL_DECODER_PMEM:
+		perf = &mds->pmem_perf;
+		break;
+	default:
+		return;
+	}
+
+	lockdep_assert_held(&cxl_dpa_rwsem);
+
+	if (!range_contains(&perf->dpa_range, &dpa))
+		return;
+
+	rc = cxl_hb_get_perf_coordinates(port, hb_coord);
+	if (rc)  {
+		dev_dbg(&port->dev, "Failed to retrieve hb perf coordinates.\n");
+		return;
+	}
+
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		/* Pickup the host bridge coords */
+		cxl_coordinates_combine(&coord, &hb_coord[i], &perf->coord);
+
+		/* Get total bandwidth and the worst latency for the cxl region */
+		cxlr->coord[i].read_latency = max_t(unsigned int,
+						    cxlr->coord[i].read_latency,
+						    coord.read_latency);
+		cxlr->coord[i].write_latency = max_t(unsigned int,
+						     cxlr->coord[i].write_latency,
+						     coord.write_latency);
+		cxlr->coord[i].read_bandwidth += coord.read_bandwidth;
+		cxlr->coord[i].write_bandwidth += coord.write_bandwidth;
+
+		/*
+		 * Convert latency to nanosec from picosec to be consistent
+		 * with the resulting latency coordinates computed by the
+		 * HMAT_REPORTING code.
+		 */
+		cxlr->coord[i].read_latency =
+			DIV_ROUND_UP(cxlr->coord[i].read_latency, 1000);
+		cxlr->coord[i].write_latency =
+			DIV_ROUND_UP(cxlr->coord[i].write_latency, 1000);
+	}
+}
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index ce0e2d82bb2b..0c2337b1fd41 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1722,6 +1722,8 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 		return -EINVAL;
 	}
 
+	cxl_region_perf_data_calculate(cxlr, cxled);
+
 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
 		int i;
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index de477eb7f5d5..95864ce7b394 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -517,6 +517,7 @@ struct cxl_region_params {
  * @cxlr_pmem: (for pmem regions) cached copy of the nvdimm bridge
  * @flags: Region state flags
  * @params: active + config params for the region
+ * @coord: QoS access coordinates for the region
  */
 struct cxl_region {
 	struct device dev;
@@ -527,6 +528,7 @@ struct cxl_region {
 	struct cxl_pmem_region *cxlr_pmem;
 	unsigned long flags;
 	struct cxl_region_params params;
+	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
 };
 
 struct cxl_nvdimm_bridge {
@@ -881,6 +883,8 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
 				      struct access_coordinate *coord);
 int cxl_hb_get_perf_coordinates(struct cxl_port *port,
 				struct access_coordinate *coord);
+void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
+				    struct cxl_endpoint_decoder *cxled);
 
 void cxl_memdev_update_perf(struct cxl_memdev *cxlmd);
 
-- 
2.43.0

[PATCH v5 10/12] cxl/region: Add sysfs attribute for locality attributes of CXL regions

[Dave Jiang]

Add read/write latencies and bandwidth sysfs attributes for the enabled CXL
region. The bandwidth is the aggregated bandwidth of all devices that
contribute to the CXL region. The latency is the worst latency of the
device amongst all the devices that contribute to the CXL region.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-cxl |  60 +++++++++++
 drivers/cxl/core/region.c               | 134 ++++++++++++++++++++++++
 2 files changed, 194 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
index fff2581b8033..5f8c26815399 100644
--- a/Documentation/ABI/testing/sysfs-bus-cxl
+++ b/Documentation/ABI/testing/sysfs-bus-cxl
@@ -552,3 +552,63 @@ Description:
 		attribute is only visible for devices supporting the
 		capability. The retrieved errors are logged as kernel
 		events when cxl_poison event tracing is enabled.
+
+
+What:		/sys/bus/cxl/devices/regionZ/accessY/read_bandwidth
+Date:		Jan, 2024
+KernelVersion:	v6.9
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RO) The aggregated read bandwidth of the region. The number is
+		the accumulated read bandwidth of all CXL memory devices that
+		contributes to the region in MB/s. It is identical data that
+		should appear in
+		/sys/devices/system/node/nodeX/accessY/initiators/read_bandwidth.
+		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
+		the number to the closest initiator and access1 provides the
+		number to the closest CPU.
+
+
+What:		/sys/bus/cxl/devices/regionZ/accessY/write_bandwidth
+Date:		Jan, 2024
+KernelVersion:	v6.9
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RO) The aggregated write bandwidth of the region. The number is
+		the accumulated write bandwidth of all CXL memory devices that
+		contributes to the region in MB/s. It is identical data that
+		should appear in
+		/sys/devices/system/node/nodeX/accessY/initiators/write_bandwidth.
+		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
+		the number to the closest initiator and access1 provides the
+		number to the closest CPU.
+
+
+What:		/sys/bus/cxl/devices/regionZ/accessY/read_latency
+Date:		Jan, 2024
+KernelVersion:	v6.9
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RO) The read latency of the region. The number is
+		the worst read latency of all CXL memory devices that
+		contributes to the region in nanoseconds. It is identical data
+		that should appear in
+		/sys/devices/system/node/nodeX/accessY/initiators/read_latency.
+		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
+		the number to the closest initiator and access1 provides the
+		number to the closest CPU.
+
+
+What:		/sys/bus/cxl/devices/regionZ/accessY/write_latency
+Date:		Jan, 2024
+KernelVersion:	v6.9
+Contact:	linux-cxl@vger.kernel.org
+Description:
+		(RO) The write latency of the region. The number is
+		the worst write latency of all CXL memory devices that
+		contributes to the region in nanoseconds. It is identical data
+		that should appear in
+		/sys/devices/system/node/nodeX/accessY/initiators/write_latency.
+		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
+		the number to the closest initiator and access1 provides the
+		number to the closest CPU.
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 0c2337b1fd41..6347dbc746f9 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -30,6 +30,138 @@
 
 static struct cxl_region *to_cxl_region(struct device *dev);
 
+#define __ACCESS0_ATTR_RO(_name) {				\
+	.attr	= { .name = __stringify(_name), .mode = 0444 },	\
+	.show	= _name##_access0_show,				\
+}
+
+#define ACCESS0_DEVICE_ATTR_RO(_name) \
+	struct device_attribute dev_attr_access0_##_name = __ACCESS0_ATTR_RO(_name)
+
+#define ACCESS0_ATTR(attrib)					\
+static ssize_t attrib##_access0_show(struct device *dev,	\
+			   struct device_attribute *attr,	\
+			   char *buf)				\
+{								\
+	struct cxl_region *cxlr = to_cxl_region(dev);		\
+								\
+	if (cxlr->coord[0].attrib == 0)				\
+		return -ENOENT;					\
+								\
+	return sysfs_emit(buf, "%u\n", cxlr->coord[0].attrib);	\
+}								\
+static ACCESS0_DEVICE_ATTR_RO(attrib)
+
+ACCESS0_ATTR(read_bandwidth);
+ACCESS0_ATTR(read_latency);
+ACCESS0_ATTR(write_bandwidth);
+ACCESS0_ATTR(write_latency);
+
+static struct attribute *access0_coordinate_attrs[] = {
+	&dev_attr_access0_read_bandwidth.attr,
+	&dev_attr_access0_write_bandwidth.attr,
+	&dev_attr_access0_read_latency.attr,
+	&dev_attr_access0_write_latency.attr,
+	NULL,
+};
+
+static umode_t cxl_region_access0_coordinate_visible(struct kobject *kobj,
+						     struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct cxl_region *cxlr = to_cxl_region(dev);
+
+	if (a == &dev_attr_access0_read_latency.attr &&
+	    cxlr->coord[ACCESS_COORDINATE_LOCAL].read_latency == 0)
+		return 0;
+
+	if (a == &dev_attr_access0_write_latency.attr &&
+	    cxlr->coord[ACCESS_COORDINATE_LOCAL].write_latency == 0)
+		return 0;
+
+	if (a == &dev_attr_access0_read_bandwidth.attr &&
+	    cxlr->coord[ACCESS_COORDINATE_LOCAL].read_bandwidth == 0)
+		return 0;
+
+	if (a == &dev_attr_access0_write_bandwidth.attr &&
+	    cxlr->coord[ACCESS_COORDINATE_LOCAL].write_bandwidth == 0)
+		return 0;
+
+	return a->mode;
+}
+
+#define __ACCESS1_ATTR_RO(_name) {				\
+	.attr	= { .name = __stringify(_name), .mode = 0444 },	\
+	.show	= _name##_access1_show,				\
+}
+
+#define ACCESS1_DEVICE_ATTR_RO(_name) \
+	struct device_attribute dev_attr_access1_##_name = __ACCESS1_ATTR_RO(_name)
+
+#define ACCESS1_ATTR(attrib)					\
+static ssize_t attrib##_access1_show(struct device *dev,	\
+			   struct device_attribute *attr,	\
+			   char *buf)				\
+{								\
+	struct cxl_region *cxlr = to_cxl_region(dev);		\
+								\
+	if (cxlr->coord[1].attrib == 0)				\
+		return -ENOENT;					\
+								\
+	return sysfs_emit(buf, "%u\n", cxlr->coord[1].attrib);	\
+}								\
+static ACCESS1_DEVICE_ATTR_RO(attrib)
+
+ACCESS1_ATTR(read_bandwidth);
+ACCESS1_ATTR(read_latency);
+ACCESS1_ATTR(write_bandwidth);
+ACCESS1_ATTR(write_latency);
+
+static struct attribute *access1_coordinate_attrs[] = {
+	&dev_attr_access1_read_bandwidth.attr,
+	&dev_attr_access1_write_bandwidth.attr,
+	&dev_attr_access1_read_latency.attr,
+	&dev_attr_access1_write_latency.attr,
+	NULL,
+};
+
+static umode_t cxl_region_access1_coordinate_visible(struct kobject *kobj,
+						     struct attribute *a, int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct cxl_region *cxlr = to_cxl_region(dev);
+
+	if (a == &dev_attr_access1_read_latency.attr &&
+	    cxlr->coord[ACCESS_COORDINATE_CPU].read_latency == 0)
+		return 0;
+
+	if (a == &dev_attr_access1_write_latency.attr &&
+	    cxlr->coord[ACCESS_COORDINATE_CPU].write_latency == 0)
+		return 0;
+
+	if (a == &dev_attr_access1_read_bandwidth.attr &&
+	    cxlr->coord[ACCESS_COORDINATE_CPU].read_bandwidth == 0)
+		return 0;
+
+	if (a == &dev_attr_access1_write_bandwidth.attr &&
+	    cxlr->coord[ACCESS_COORDINATE_CPU].write_bandwidth == 0)
+		return 0;
+
+	return a->mode;
+}
+
+static const struct attribute_group cxl_region_access0_coordinate_group = {
+	.name = "access0",
+	.attrs = access0_coordinate_attrs,
+	.is_visible = cxl_region_access0_coordinate_visible,
+};
+
+static const struct attribute_group cxl_region_access1_coordinate_group = {
+	.name = "access1",
+	.attrs = access1_coordinate_attrs,
+	.is_visible = cxl_region_access1_coordinate_visible,
+};
+
 static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
@@ -2039,6 +2171,8 @@ static const struct attribute_group *region_groups[] = {
 	&cxl_base_attribute_group,
 	&cxl_region_group,
 	&cxl_region_target_group,
+	&cxl_region_access0_coordinate_group,
+	&cxl_region_access1_coordinate_group,
 	NULL,
 };
 
-- 
2.43.0

[PATCH v5 11/12] cxl/region: Add memory hotplug notifier for cxl region

[Dave Jiang]

When the CXL region is formed, the driver would computed the performance
data for the region. However this data is not available at the node data
collection that has been populated by the HMAT during kernel
initialization. Add a memory hotplug notifier to update the access
coordinates to the 'struct memory_target' context kept by the
HMAT_REPORTING code.

Add CXL_CALLBACK_PRI for a memory hotplug callback priority. Set the
priority number to be called before HMAT_CALLBACK_PRI. The CXL update must
happen before hmat_callback().

A new HMAT_REPORING helper hmat_update_target_coordinates() is added in
order to allow CXL to update the memory_target access coordinates.

A new ext_updated member is added to the memory_target to indicate that
the access coordinates within the memory_target has been updated by an
external agent such as CXL. This prevents data being overwritten by the
hmat_update_target_attrs() triggered by hmat_callback().

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v5:
- Fix build for PPC (0day)
---
 drivers/acpi/numa/hmat.c  | 36 +++++++++++++++++++
 drivers/cxl/core/cdat.c   |  6 ++++
 drivers/cxl/core/core.h   |  3 ++
 drivers/cxl/core/region.c | 75 +++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h         |  3 ++
 include/linux/acpi.h      | 12 +++++++
 include/linux/memory.h    |  1 +
 7 files changed, 136 insertions(+)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 5be79896bd08..dc75a3355164 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -74,6 +74,7 @@ struct memory_target {
 	struct node_cache_attrs cache_attrs;
 	u8 gen_port_device_handle[ACPI_SRAT_DEVICE_HANDLE_SIZE];
 	bool registered;
+	bool ext_updated;	/* externally updated */
 };
 
 struct memory_initiator {
@@ -328,6 +329,35 @@ static void hmat_update_target_access(struct memory_target *target,
 	}
 }
 
+int hmat_update_target_coordinates(int nid, struct access_coordinate *coord,
+				   enum access_coordinate_class access)
+{
+	struct memory_target *target;
+	int pxm;
+
+	if (nid == NUMA_NO_NODE)
+		return -EINVAL;
+
+	pxm = node_to_pxm(nid);
+	guard(mutex)(&target_lock);
+	target = find_mem_target(pxm);
+	if (!target)
+		return -ENODEV;
+
+	hmat_update_target_access(target, ACPI_HMAT_READ_LATENCY,
+				  coord->read_latency, access);
+	hmat_update_target_access(target, ACPI_HMAT_WRITE_LATENCY,
+				  coord->write_latency, access);
+	hmat_update_target_access(target, ACPI_HMAT_READ_BANDWIDTH,
+				  coord->read_bandwidth, access);
+	hmat_update_target_access(target, ACPI_HMAT_WRITE_BANDWIDTH,
+				  coord->write_bandwidth, access);
+	target->ext_updated = true;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(hmat_update_target_coordinates);
+
 static __init void hmat_add_locality(struct acpi_hmat_locality *hmat_loc)
 {
 	struct memory_locality *loc;
@@ -699,6 +729,12 @@ static void hmat_update_target_attrs(struct memory_target *target,
 	u32 best = 0;
 	int i;
 
+	/*
+	 * Don't update if an external agent has changed the data.
+	 */
+	if (target->ext_updated)
+		return;
+
 	/* Don't update for generic port if there's no device handle */
 	if ((access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL ||
 	     access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index bf7c3c33c7ee..20fb8318dfcc 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -580,3 +580,9 @@ void cxl_region_perf_data_calculate(struct cxl_region *cxlr,
 			DIV_ROUND_UP(cxlr->coord[i].write_latency, 1000);
 	}
 }
+
+int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
+				       enum access_coordinate_class access)
+{
+	return hmat_update_target_coordinates(nid, &cxlr->coord[access], access);
+}
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index 3b64fb1b9ed0..e19800a7ce06 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -90,4 +90,7 @@ enum cxl_poison_trace_type {
 
 long cxl_pci_get_latency(struct pci_dev *pdev);
 
+int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
+				       enum access_coordinate_class access);
+
 #endif /* __CXL_CORE_H__ */
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 6347dbc746f9..19e419f18472 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -4,6 +4,7 @@
 #include <linux/genalloc.h>
 #include <linux/device.h>
 #include <linux/module.h>
+#include <linux/memory.h>
 #include <linux/slab.h>
 #include <linux/uuid.h>
 #include <linux/sort.h>
@@ -156,12 +157,22 @@ static const struct attribute_group cxl_region_access0_coordinate_group = {
 	.is_visible = cxl_region_access0_coordinate_visible,
 };
 
+static const struct attribute_group *get_cxl_region_access0_group(void)
+{
+	return &cxl_region_access0_coordinate_group;
+}
+
 static const struct attribute_group cxl_region_access1_coordinate_group = {
 	.name = "access1",
 	.attrs = access1_coordinate_attrs,
 	.is_visible = cxl_region_access1_coordinate_visible,
 };
 
+static const struct attribute_group *get_cxl_region_access1_group(void)
+{
+	return &cxl_region_access1_coordinate_group;
+}
+
 static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
@@ -3066,6 +3077,65 @@ static int is_system_ram(struct resource *res, void *arg)
 	return 1;
 }
 
+static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
+{
+	int cset = 0;
+	int rc;
+
+	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
+		if (cxlr->coord[i].read_bandwidth) {
+			rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
+			if (rc == 0)
+				cset++;
+		}
+	}
+
+	if (!cset)
+		return false;
+
+	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_access0_group());
+	if (rc)
+		dev_dbg(&cxlr->dev, "Failed to update access0 group\n");
+
+	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_access1_group());
+	if (rc)
+		dev_dbg(&cxlr->dev, "Failed to update access1 group\n");
+
+	return true;
+}
+
+static int cxl_region_perf_attrs_callback(struct notifier_block *nb,
+					  unsigned long action, void *arg)
+{
+	struct cxl_region *cxlr = container_of(nb, struct cxl_region,
+					       memory_notifier);
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_endpoint_decoder *cxled = p->targets[0];
+	struct cxl_decoder *cxld = &cxled->cxld;
+	struct memory_notify *mnb = arg;
+	int nid = mnb->status_change_nid;
+	int region_nid;
+
+	if (nid == NUMA_NO_NODE || action != MEM_ONLINE)
+		return NOTIFY_DONE;
+
+	region_nid = phys_to_target_node(cxld->hpa_range.start);
+	if (nid != region_nid)
+		return NOTIFY_DONE;
+
+	if (!cxl_region_update_coordinates(cxlr, nid))
+		return NOTIFY_DONE;
+
+	return NOTIFY_OK;
+}
+
+static void remove_coord_notifier(void *data)
+{
+	struct cxl_region *cxlr = data;
+
+	unregister_memory_notifier(&cxlr->memory_notifier);
+}
+
 static int cxl_region_probe(struct device *dev)
 {
 	struct cxl_region *cxlr = to_cxl_region(dev);
@@ -3091,6 +3161,11 @@ static int cxl_region_probe(struct device *dev)
 		goto out;
 	}
 
+	cxlr->memory_notifier.notifier_call = cxl_region_perf_attrs_callback;
+	cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
+	register_memory_notifier(&cxlr->memory_notifier);
+	rc = devm_add_action_or_reset(&cxlr->dev, remove_coord_notifier, cxlr);
+
 	/*
 	 * From this point on any path that changes the region's state away from
 	 * CXL_CONFIG_COMMIT is also responsible for releasing the driver.
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 95864ce7b394..534e25e2f0a4 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -6,6 +6,7 @@
 
 #include <linux/libnvdimm.h>
 #include <linux/bitfield.h>
+#include <linux/notifier.h>
 #include <linux/bitops.h>
 #include <linux/log2.h>
 #include <linux/node.h>
@@ -518,6 +519,7 @@ struct cxl_region_params {
  * @flags: Region state flags
  * @params: active + config params for the region
  * @coord: QoS access coordinates for the region
+ * @memory_notifier: notifier for setting the access coordinates to node
  */
 struct cxl_region {
 	struct device dev;
@@ -529,6 +531,7 @@ struct cxl_region {
 	unsigned long flags;
 	struct cxl_region_params params;
 	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
+	struct notifier_block memory_notifier;
 };
 
 struct cxl_nvdimm_bridge {
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index b7165e52b3c6..c84c2f34b8ee 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1547,4 +1547,16 @@ static inline void acpi_use_parent_companion(struct device *dev)
 	ACPI_COMPANION_SET(dev, ACPI_COMPANION(dev->parent));
 }
 
+#ifdef CONFIG_ACPI_HMAT
+int hmat_update_target_coordinates(int nid, struct access_coordinate *coord,
+				   enum access_coordinate_class access);
+#else
+static inline int hmat_update_target_coordinates(int nid,
+						 struct access_coordinate *coord,
+						 enum access_coordinate_class access)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+
 #endif	/*_LINUX_ACPI_H*/
diff --git a/include/linux/memory.h b/include/linux/memory.h
index f53cfdaaaa41..d8588256578a 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -114,6 +114,7 @@ struct mem_section;
 #define DEFAULT_CALLBACK_PRI	0
 #define SLAB_CALLBACK_PRI	1
 #define HMAT_CALLBACK_PRI	2
+#define CXL_CALLBACK_PRI	5
 #define MM_COMPUTE_BATCH_PRI	10
 #define CPUSET_CALLBACK_PRI	10
 #define MEMTIER_HOTPLUG_PRI	100
-- 
2.43.0

[PATCH v5 12/12] cxl/region: Deal with numa nodes not enumarated by SRAT

[Dave Jiang]

For the numa nodes that are not created by SRAT, no memory_target is
allocated and is not managed by the HMAT_REPORTING code. Therefore
hmat_callback() memory hotplug notifier will exit early on those NUMA
nodes. The CXL memory hotplug notifier will need to call
node_set_perf_attrs() directly in order to setup the access sysfs
attributes.

In acpi_numa_init(), the last proximity domain (pxm) id created by SRAT is
stored. Add a helper function acpi_node_backed_by_real_pxm() in order to
check if a NUMA node id is defined by SRAT or created by CFMWS.

node_set_perf_attrs() symbol is exported to allow update of perf attribs
for a node. The sysfs path of
/sys/devices/system/node/nodeX/access0/initiators/* is created by
ndoe_set_perf_attrs() for the various attributes where nodeX is matched
to the NUMA node of the CXL region.

Cc: Rafael J. Wysocki <rafael@kernel.org>
Reviewed-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v5:
- Fix ARM compile of missing prototype (0day)
---
 drivers/acpi/numa/srat.c  | 11 +++++++++++
 drivers/base/node.c       |  1 +
 drivers/cxl/core/cdat.c   |  5 +++++
 drivers/cxl/core/core.h   |  1 +
 drivers/cxl/core/region.c |  7 ++++++-
 include/linux/acpi.h      |  9 +++++++++
 6 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index 0214518fc582..e45e64993c50 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -29,6 +29,8 @@ static int node_to_pxm_map[MAX_NUMNODES]
 unsigned char acpi_srat_revision __initdata;
 static int acpi_numa __initdata;
 
+static int last_real_pxm;
+
 void __init disable_srat(void)
 {
 	acpi_numa = -1;
@@ -536,6 +538,7 @@ int __init acpi_numa_init(void)
 		if (node_to_pxm_map[i] > fake_pxm)
 			fake_pxm = node_to_pxm_map[i];
 	}
+	last_real_pxm = fake_pxm;
 	fake_pxm++;
 	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, acpi_parse_cfmws,
 			      &fake_pxm);
@@ -547,6 +550,14 @@ int __init acpi_numa_init(void)
 	return 0;
 }
 
+bool acpi_node_backed_by_real_pxm(int nid)
+{
+	int pxm = node_to_pxm(nid);
+
+	return pxm <= last_real_pxm;
+}
+EXPORT_SYMBOL_GPL(acpi_node_backed_by_real_pxm);
+
 static int acpi_get_pxm(acpi_handle h)
 {
 	unsigned long long pxm;
diff --git a/drivers/base/node.c b/drivers/base/node.c
index a73b0c9a401a..eb72580288e6 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -215,6 +215,7 @@ void node_set_perf_attrs(unsigned int nid, struct access_coordinate *coord,
 		}
 	}
 }
+EXPORT_SYMBOL_GPL(node_set_perf_attrs);
 
 /**
  * struct node_cache_info - Internal tracking for memory node caches
diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index 20fb8318dfcc..c012c7304164 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -586,3 +586,8 @@ int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
 {
 	return hmat_update_target_coordinates(nid, &cxlr->coord[access], access);
 }
+
+bool cxl_need_node_perf_attrs_update(int nid)
+{
+	return !acpi_node_backed_by_real_pxm(nid);
+}
diff --git a/drivers/cxl/core/core.h b/drivers/cxl/core/core.h
index e19800a7ce06..bc5a95665aa0 100644
--- a/drivers/cxl/core/core.h
+++ b/drivers/cxl/core/core.h
@@ -92,5 +92,6 @@ long cxl_pci_get_latency(struct pci_dev *pdev);
 
 int cxl_update_hmat_access_coordinates(int nid, struct cxl_region *cxlr,
 				       enum access_coordinate_class access);
+bool cxl_need_node_perf_attrs_update(int nid);
 
 #endif /* __CXL_CORE_H__ */
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 19e419f18472..db51e35cb44f 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3084,7 +3084,12 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
 
 	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
 		if (cxlr->coord[i].read_bandwidth) {
-			rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
+			rc = 0;
+			if (cxl_need_node_perf_attrs_update(nid))
+				node_set_perf_attrs(nid, &cxlr->coord[i], i);
+			else
+				rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
+
 			if (rc == 0)
 				cset++;
 		}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index c84c2f34b8ee..2a7c4b90d589 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1559,4 +1559,13 @@ static inline int hmat_update_target_coordinates(int nid,
 }
 #endif
 
+#ifdef CONFIG_ACPI_NUMA
+bool acpi_node_backed_by_real_pxm(int nid);
+#else
+static inline bool acpi_node_backed_by_real_pxm(int nid)
+{
+	return false;
+}
+#endif
+
 #endif	/*_LINUX_ACPI_H*/
-- 
2.43.0

Re: [PATCH v5 01/12] ACPI: HMAT: Remove register of memory node for generic target

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:29 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> For generic targets, there's no reason to call
> register_memory_node_under_compute_node() with the access levels that are
> only visible to HMAT handling code. Only update the attributes and rename
> hmat_register_generic_target_initiators() to hmat_update_generic_target().

What goes wrong as a result of this bug?

Looks like a cleanup at first glance.

With some more description if it is a fix.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> Fixes: a3a3e341f169 ("acpi: numa: Add setting of generic port system locality attributes")
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/acpi/numa/hmat.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index d6b85f0f6082..a26e7793ec4e 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -770,12 +770,12 @@ static void __hmat_register_target_initiators(struct memory_target *target,
>  	}
>  }
>  
> -static void hmat_register_generic_target_initiators(struct memory_target *target)
> +static void hmat_update_generic_target(struct memory_target *target)
>  {
>  	static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
>  
> -	__hmat_register_target_initiators(target, p_nodes,
> -					  NODE_ACCESS_CLASS_GENPORT_SINK);
> +	hmat_update_target_attrs(target, p_nodes,
> +				 NODE_ACCESS_CLASS_GENPORT_SINK);
>  }
>  
>  static void hmat_register_target_initiators(struct memory_target *target)
> @@ -835,7 +835,7 @@ static void hmat_register_target(struct memory_target *target)
>  	 */
>  	mutex_lock(&target_lock);
>  	if (*(u16 *)target->gen_port_device_handle) {
> -		hmat_register_generic_target_initiators(target);
> +		hmat_update_generic_target(target);
>  		target->registered = true;
>  	}
>  	mutex_unlock(&target_lock);

Re: [PATCH v5 02/12] base/node / ACPI: Enumerate node access class for 'struct access_coordinate'

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:30 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Both generic node and HMAT handling code have been using magic numbers to
> indicate access classes for 'struct access_coordinate'. Introduce enums to
> enumerate the access0 and access1 classes shared by the two subsystems.
> Update the function parameters and callers as appropriate to utilize the
> new enum.
> 
> Access0 is named to ACCESS_COORDINATE_LOCAL in order to indicate that the
> access class is for 'struct access_coordinate' between a target node and
> the nearest initiator node.
> 
> Access1 is named to ACCESS_COORDINATE_CPU in order to indicate that the
> access class is for 'struct access_coordinate' between a target node and
> the nearest CPU node.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

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

I resisted bikeshedding on names.  These are clear enough for me.

Re: [PATCH v5 03/12] ACPI: HMAT: Introduce 2 levels of generic port access class

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:31 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> In order to compute access0 and access1 classes for CXL memory, 2 levels
> of generic port information must be stored. Access0 will indicate the
> generic port access coordinates to the closest initiator and access1
> will indicate the generic port access coordinates to the cloest CPU.
> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Grumble.  I never liked Memory Proximity Domain Attributes Structure.
Adds little value and for ports I don't think you should consider it
(because it's about GI or Processor to Memory connections and
 Generic Ports aren't allowed in either field).

Other than dropping that short cut, LGTM.

> ---
>  drivers/acpi/numa/hmat.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index e0144cfbf1f3..8dbb0e366059 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -59,7 +59,8 @@ struct target_cache {
>  };
>  
>  enum {
> -	NODE_ACCESS_CLASS_GENPORT_SINK = ACCESS_COORDINATE_MAX,
> +	NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL = ACCESS_COORDINATE_MAX,
> +	NODE_ACCESS_CLASS_GENPORT_SINK_CPU,
>  	NODE_ACCESS_CLASS_MAX,
>  };
>  
> @@ -141,7 +142,7 @@ int acpi_get_genport_coordinates(u32 uid,
>  	if (!target)
>  		return -ENOENT;
>  
> -	*coord = target->coord[NODE_ACCESS_CLASS_GENPORT_SINK];
> +	*coord = target->coord[NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL];
>  
>  	return 0;
>  }
> @@ -695,7 +696,8 @@ static void hmat_update_target_attrs(struct memory_target *target,
>  	int i;
>  
>  	/* Don't update for generic port if there's no device handle */
> -	if (access == NODE_ACCESS_CLASS_GENPORT_SINK &&
> +	if ((access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL ||
> +	     access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&
>  	    !(*(u16 *)target->gen_port_device_handle))
>  		return;
>  
> @@ -707,7 +709,8 @@ static void hmat_update_target_attrs(struct memory_target *target,
>  	 */
>  	if (target->processor_pxm != PXM_INVAL) {
>  		cpu_nid = pxm_to_node(target->processor_pxm);
> -		if (access == ACCESS_COORDINATE_LOCAL ||
> +		if ((access == ACCESS_COORDINATE_LOCAL ||
> +		     access == NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL) &&
The comment above this says:
	/*
	 * If the Address Range Structure provides a local processor pxm, set
	 * only that one. Otherwise, find the best performance attributes and
	 * collect all initiators that match.
	 */

Assuming that is correct, under what circumstances is it relevant to a
generic port?  I'm hoping no one builds systems with RAM that is local to
a port?

Note that the comment requires some archaeology - the Address Range structure
has been renamed as Memory Proximity Domain Attributes Structure. (see ACPI 6.2)
And the 'local processor PXM was probably originally Processor Proximity Domain.
That has now become "Proximity Domain for the Attached Initiator." It's
used only basically override the HMAT distances and say:
Memory domain X is directly attached to Processor Y
Here we don't have a memory domain (generic port) and so I don't think that
case is relevant.

So leave this block alone.

>  		    node_state(cpu_nid, N_CPU)) {
>  			set_bit(target->processor_pxm, p_nodes);
>  			return;
> @@ -736,7 +739,8 @@ static void hmat_update_target_attrs(struct memory_target *target,
>  		list_for_each_entry(initiator, &initiators, node) {
>  			u32 value;
>  
> -			if (access == ACCESS_COORDINATE_CPU &&
> +			if ((access == ACCESS_COORDINATE_CPU &&
> +			     access == NODE_ACCESS_CLASS_GENPORT_SINK_CPU) &&

This one looks to be correct.

>  			    !initiator->has_cpu) {
>  				clear_bit(initiator->processor_pxm, p_nodes);
>  				continue;
> @@ -775,7 +779,9 @@ static void hmat_update_generic_target(struct memory_target *target)
>  	static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
>  
>  	hmat_update_target_attrs(target, p_nodes,
> -				 NODE_ACCESS_CLASS_GENPORT_SINK);
> +				 NODE_ACCESS_CLASS_GENPORT_SINK_LOCAL);
> +	hmat_update_target_attrs(target, p_nodes,
> +				 NODE_ACCESS_CLASS_GENPORT_SINK_CPU);
>  }
>  
>  static void hmat_register_target_initiators(struct memory_target *target)

Re: [PATCH v5 04/12] ACPI: HMAT / cxl: Add retrieval of generic port coordinates for both access classes

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:32 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Update acpi_get_genport_coordinates() to allow retrieval of both access
> classes of the 'struct access_coordinate' for a generic target. The update
> will allow CXL code to compute access coordinates for both access class.
> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v5 05/12] cxl: Split out combine_coordinates() for common shared usage

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:33 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Refactor the common code of combining coordinates in order to reduce code.
> Create a new function cxl_cooordinates_combine() it combine two 'struct
> access_coordinate'.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v5:
> - Fix comment header (0-day)
> - Remove EXPORT_SYMBOL (Dan)
> ---
>  drivers/cxl/core/cdat.c | 32 +++++++++++++++++++++++---------
>  drivers/cxl/core/port.c | 18 ++----------------
>  drivers/cxl/cxl.h       |  4 ++++
>  3 files changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 08fd0baea7a0..096320390ad9 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -185,15 +185,7 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  	xa_for_each(dsmas_xa, index, dent) {
>  		int qos_class;
>  
> -		dent->coord.read_latency = dent->coord.read_latency +
> -					   c.read_latency;
> -		dent->coord.write_latency = dent->coord.write_latency +
> -					    c.write_latency;
> -		dent->coord.read_bandwidth = min_t(int, c.read_bandwidth,
> -						   dent->coord.read_bandwidth);
> -		dent->coord.write_bandwidth = min_t(int, c.write_bandwidth,
> -						    dent->coord.write_bandwidth);
> -
> +		cxl_coordinates_combine(&dent->coord, &dent->coord, &c);
>  		dent->entries = 1;
>  		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
>  					      &qos_class);
> @@ -484,4 +476,26 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_switch_parse_cdat, CXL);
>  
> +/**
> + * cxl_coordinates_combine - Combine the two input coordinates into the first
> + *
> + * @out: Output coordinate of c1 and c2 combined
> + * @c1: first coordinate, to be written to

When you say to be written to, what do you mean?
Left over from adding out?

No obvious reason for this to have any idea that c1 and c2 are
ordered.  

> + * @c2: second coordinate
> + */
> +void cxl_coordinates_combine(struct access_coordinate *out,
> +			     struct access_coordinate *c1,
> +			     struct access_coordinate *c2)
> +{
> +		if (c2->write_bandwidth)
> +			out->write_bandwidth = min(c1->write_bandwidth,
> +						   c2->write_bandwidth);

Why check c2->write_bandwidth?
Code already does it, but I'm not sure why + I don't think you should
treat c1 and c2 differently.

> +		out->write_latency = c1->write_latency + c2->write_latency;
> +
> +		if (c2->read_bandwidth)
> +			out->read_bandwidth = min(c1->read_bandwidth,
> +						  c2->read_bandwidth);
> +		out->read_latency = c1->read_latency + c2->read_latency;
> +}
> +
>  MODULE_IMPORT_NS(CXL);
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 612bf7e1e847..af9458b2678c 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2096,20 +2096,6 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
>  }
>  EXPORT_SYMBOL_NS_GPL(schedule_cxl_memdev_detach, CXL);
>  
> -static void combine_coordinates(struct access_coordinate *c1,
> -				struct access_coordinate *c2)
> -{
> -		if (c2->write_bandwidth)
> -			c1->write_bandwidth = min(c1->write_bandwidth,
> -						  c2->write_bandwidth);
> -		c1->write_latency += c2->write_latency;
> -
> -		if (c2->read_bandwidth)
> -			c1->read_bandwidth = min(c1->read_bandwidth,
> -						 c2->read_bandwidth);
> -		c1->read_latency += c2->read_latency;
> -}
> -
>  /**
>   * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
>   *				   of CXL path
> @@ -2143,7 +2129,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  	 * nothing to gather.
>  	 */
>  	while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
> -		combine_coordinates(&c, &dport->sw_coord);
> +		cxl_coordinates_combine(&c, &c, &dport->sw_coord);
>  		c.write_latency += dport->link_latency;
>  		c.read_latency += dport->link_latency;
>  
> @@ -2152,7 +2138,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  	}
>  
>  	/* Augment with the generic port (host bridge) perf data */
> -	combine_coordinates(&c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
> +	cxl_coordinates_combine(&c, &c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
>  
>  	/* Get the calculated PCI paths bandwidth */
>  	pdev = to_pci_dev(port->uport_dev->parent);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index fe7448f2745e..fab2da4b1f04 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -882,6 +882,10 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  
>  void cxl_memdev_update_perf(struct cxl_memdev *cxlmd);
>  
> +void cxl_coordinates_combine(struct access_coordinate *out,
> +			     struct access_coordinate *c1,
> +			     struct access_coordinate *c2);
> +
>  /*
>   * Unit test builds overrides this to __weak, find the 'strong' version
>   * of these symbols in tools/testing/cxl/.

Re: [PATCH v5 06/12] cxl: Split out host bridge access coordinates

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:34 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> The difference between access class 0 and access class 1 for 'struct
> access_coordinate', if any, is that class 0 is for the distance from
> the target to the closest initiator and that class 1 is for the distance
> from the target to the cloest CPU. For CXL memory, the nearest initiator

spell check.

> may not necessarily be a CPU node. The performance path from the CXL
> endpoint to the host bridge should remain the same. However, the numbers
> extracted and stored from HMAT is the difference for the two access
> classes. Split out the performance numbers for the host bridge (generic
> target) from the calculation of the entire path in order to allow
> calculation of both access classes for a CXL region.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

one suggestion on function comment.

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

> ---
>  drivers/cxl/core/cdat.c | 28 ++++++++++++++++++++++------
>  drivers/cxl/core/port.c | 34 +++++++++++++++++++++++++++++++---
>  drivers/cxl/cxl.h       |  2 ++
>  3 files changed, 55 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 096320390ad9..79844874a34b 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -162,15 +162,22 @@ static int cxl_cdat_endpoint_process(struct cxl_port *port,
>  static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  					struct xarray *dsmas_xa)
>  {
> -	struct access_coordinate c;
> +	struct access_coordinate ep_c;
> +	struct access_coordinate coord[ACCESS_COORDINATE_MAX];
>  	struct dsmas_entry *dent;
>  	int valid_entries = 0;
>  	unsigned long index;
>  	int rc;
>  
> -	rc = cxl_endpoint_get_perf_coordinates(port, &c);
> +	rc = cxl_endpoint_get_perf_coordinates(port, &ep_c);
>  	if (rc) {
> -		dev_dbg(&port->dev, "Failed to retrieve perf coordinates.\n");
> +		dev_dbg(&port->dev, "Failed to retrieve ep perf coordinates.\n");
> +		return rc;
> +	}
> +
> +	rc = cxl_hb_get_perf_coordinates(port, coord);
> +	if (rc)  {
> +		dev_dbg(&port->dev, "Failed to retrieve hb perf coordinates.\n");
>  		return rc;
>  	}
>  
> @@ -185,10 +192,19 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  	xa_for_each(dsmas_xa, index, dent) {
>  		int qos_class;
>  
> -		cxl_coordinates_combine(&dent->coord, &dent->coord, &c);
> +		cxl_coordinates_combine(&dent->coord, &dent->coord, &ep_c);
> +		/*
> +		 * Keeping the host bridge coordinates separate from the dsmas
> +		 * coordinates in order to allow calculation of access class
> +		 * 0 and 1 for region later.
> +		 */
> +		cxl_coordinates_combine(&coord[ACCESS_COORDINATE_LOCAL],
> +					&coord[ACCESS_COORDINATE_LOCAL],
> +					&dent->coord);
>  		dent->entries = 1;
> -		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
> -					      &qos_class);
> +		rc = cxl_root->ops->qos_class(cxl_root,
> +					      &coord[ACCESS_COORDINATE_LOCAL],
> +					      1, &qos_class);
>  		if (rc != 1)
>  			continue;
>  
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index af9458b2678c..e8029170b8c6 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -2096,6 +2096,37 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
>  }
>  EXPORT_SYMBOL_NS_GPL(schedule_cxl_memdev_detach, CXL);
>  
> +/**
> + * cxl_hb_get_perf_coordinates - Retrieve performance numbers from host bridge

This description confused me.  What numbers does the host bridge have?

between initiator and host bridge
perhaps?

> + *
> + * @port: endpoint cxl_port
> + * @coord: output access coordinates
> + *
> + * Return: errno on failure, 0 on success.
> + */
> +int cxl_hb_get_perf_coordinates(struct cxl_port *port,
> +				struct access_coordinate *coord)
> +{
> +	struct cxl_port *iter = port;
> +	struct cxl_dport *dport;
> +
> +	if (!is_cxl_endpoint(port))
> +		return -EINVAL;
> +
> +	dport = iter->parent_dport;
> +	while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
> +		iter = to_cxl_port(iter->dev.parent);
> +		dport = iter->parent_dport;
> +	}
> +
> +	coord[ACCESS_COORDINATE_LOCAL] =
> +		dport->hb_coord[ACCESS_COORDINATE_LOCAL];
> +	coord[ACCESS_COORDINATE_CPU] =
> +		dport->hb_coord[ACCESS_COORDINATE_CPU];
> +
> +	return 0;
> +}
> +
>  /**
>   * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
>   *				   of CXL path
> @@ -2137,9 +2168,6 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  		dport = iter->parent_dport;
>  	}
>  
> -	/* Augment with the generic port (host bridge) perf data */
> -	cxl_coordinates_combine(&c, &c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
> -
>  	/* Get the calculated PCI paths bandwidth */
>  	pdev = to_pci_dev(port->uport_dev->parent);
>  	bw = pcie_bandwidth_available(pdev, NULL, NULL, NULL);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index fab2da4b1f04..de477eb7f5d5 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -879,6 +879,8 @@ void cxl_switch_parse_cdat(struct cxl_port *port);
>  
>  int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>  				      struct access_coordinate *coord);
> +int cxl_hb_get_perf_coordinates(struct cxl_port *port,
> +				struct access_coordinate *coord);
>  
>  void cxl_memdev_update_perf(struct cxl_memdev *cxlmd);
>  

Re: [PATCH v5 07/12] cxl: Move QoS class to be calculated from the nearest CPU

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:35 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Retrieve the qos_class (QTG ID) using the access coordinates from the
> nearest CPU rather than the nearst initiator that may not be a CPU.
> This may be the more appropriate number that applications care about.
> 
> Link: https://lore.kernel.org/linux-cxl/20240112113023.00006c50@Huawei.com/
> Suggested-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Might be worth calling out that in most cases they are the same so
that no one bothers to backport this.

> ---
>  drivers/cxl/core/cdat.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 79844874a34b..bd0ff3cebb8c 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
> @@ -198,12 +198,12 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>  		 * coordinates in order to allow calculation of access class
>  		 * 0 and 1 for region later.
>  		 */
> -		cxl_coordinates_combine(&coord[ACCESS_COORDINATE_LOCAL],
> -					&coord[ACCESS_COORDINATE_LOCAL],
> +		cxl_coordinates_combine(&coord[ACCESS_COORDINATE_CPU],
> +					&coord[ACCESS_COORDINATE_CPU],
>  					&dent->coord);
>  		dent->entries = 1;
>  		rc = cxl_root->ops->qos_class(cxl_root,
> -					      &coord[ACCESS_COORDINATE_LOCAL],
> +					      &coord[ACCESS_COORDINATE_CPU],
>  					      1, &qos_class);
>  		if (rc != 1)
>  			continue;

Re: [PATCH v5 08/12] cxl: Set cxlmd->endpoint before adding port device

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:36 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Move setting of cxlmd->endpoint to before calling add_device() on the port
> device. Otherwise when referencing cxlmd->endpoint in region discovery code
> that is triggered by the port driver probe function, the endpoint port
> pointer is not valid.

Maybe make it clear this only matters (I assume) when we start doing
such a look up?  Otherwise description sounds like a fix.


> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Change looks fine

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

> ---
>  drivers/cxl/core/port.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index e8029170b8c6..2f2b7af9275e 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -822,6 +822,7 @@ static struct cxl_port *__devm_cxl_add_port(struct device *host,
>  		 */
>  		port->reg_map = cxlds->reg_map;
>  		port->reg_map.host = &port->dev;
> +		cxlmd->endpoint = port;
>  	} else if (parent_dport) {
>  		rc = dev_set_name(dev, "port%d", port->id);
>  		if (rc)
> @@ -1374,7 +1375,6 @@ int cxl_endpoint_autoremove(struct cxl_memdev *cxlmd, struct cxl_port *endpoint)
>  
>  	get_device(host);
>  	get_device(&endpoint->dev);
> -	cxlmd->endpoint = endpoint;
>  	cxlmd->depth = endpoint->depth;
>  	return devm_add_action_or_reset(dev, delete_endpoint, cxlmd);
>  }

Re: [PATCH v5 10/12] cxl/region: Add sysfs attribute for locality attributes of CXL regions

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:38 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add read/write latencies and bandwidth sysfs attributes for the enabled CXL
> region. The bandwidth is the aggregated bandwidth of all devices that
> contribute to the CXL region. The latency is the worst latency of the
> device amongst all the devices that contribute to the CXL region.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
A few comments inline.  Mostly about reducing duplication.

> ---
>  Documentation/ABI/testing/sysfs-bus-cxl |  60 +++++++++++
>  drivers/cxl/core/region.c               | 134 ++++++++++++++++++++++++
>  2 files changed, 194 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl b/Documentation/ABI/testing/sysfs-bus-cxl
> index fff2581b8033..5f8c26815399 100644
> --- a/Documentation/ABI/testing/sysfs-bus-cxl
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -552,3 +552,63 @@ Description:
>  		attribute is only visible for devices supporting the
>  		capability. The retrieved errors are logged as kernel
>  		events when cxl_poison event tracing is enabled.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/accessY/read_bandwidth

Up to you, but it's fairly common to have multiple what lines in these
files and you could easily combine at least the read/write descriptions.

> +Date:		Jan, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) The aggregated read bandwidth of the region. The number is
> +		the accumulated read bandwidth of all CXL memory devices that
> +		contributes to the region in MB/s. It is identical data that
> +		should appear in
> +		/sys/devices/system/node/nodeX/accessY/initiators/read_bandwidth.
> +		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
> +		the number to the closest initiator and access1 provides the
> +		number to the closest CPU.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/accessY/write_bandwidth
> +Date:		Jan, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) The aggregated write bandwidth of the region. The number is
> +		the accumulated write bandwidth of all CXL memory devices that
> +		contributes to the region in MB/s. It is identical data that
> +		should appear in
> +		/sys/devices/system/node/nodeX/accessY/initiators/write_bandwidth.
> +		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
> +		the number to the closest initiator and access1 provides the
> +		number to the closest CPU.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/accessY/read_latency
> +Date:		Jan, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) The read latency of the region. The number is
> +		the worst read latency of all CXL memory devices that
> +		contributes to the region in nanoseconds. It is identical data
> +		that should appear in
> +		/sys/devices/system/node/nodeX/accessY/initiators/read_latency.
> +		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
> +		the number to the closest initiator and access1 provides the
> +		number to the closest CPU.
> +
> +
> +What:		/sys/bus/cxl/devices/regionZ/accessY/write_latency
> +Date:		Jan, 2024
> +KernelVersion:	v6.9
> +Contact:	linux-cxl@vger.kernel.org
> +Description:
> +		(RO) The write latency of the region. The number is
> +		the worst write latency of all CXL memory devices that
> +		contributes to the region in nanoseconds. It is identical data
> +		that should appear in
> +		/sys/devices/system/node/nodeX/accessY/initiators/write_latency.
> +		See Documentation/ABI/stable/sysfs-devices-node. access0 provides
> +		the number to the closest initiator and access1 provides the
> +		number to the closest CPU.
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 0c2337b1fd41..6347dbc746f9 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -30,6 +30,138 @@
>  
>  static struct cxl_region *to_cxl_region(struct device *dev);
>  
> +#define __ACCESS0_ATTR_RO(_name) {				\
> +	.attr	= { .name = __stringify(_name), .mode = 0444 },	\
> +	.show	= _name##_access0_show,				\
> +}
> +
> +#define ACCESS0_DEVICE_ATTR_RO(_name) \
> +	struct device_attribute dev_attr_access0_##_name = __ACCESS0_ATTR_RO(_name)
> +
> +#define ACCESS0_ATTR(attrib)					\
> +static ssize_t attrib##_access0_show(struct device *dev,	\
> +			   struct device_attribute *attr,	\
> +			   char *buf)				\
> +{								\
> +	struct cxl_region *cxlr = to_cxl_region(dev);		\
> +								\
> +	if (cxlr->coord[0].attrib == 0)				\
> +		return -ENOENT;					\
> +								\
> +	return sysfs_emit(buf, "%u\n", cxlr->coord[0].attrib);	\
> +}								\
> +static ACCESS0_DEVICE_ATTR_RO(attrib)
> +
> +ACCESS0_ATTR(read_bandwidth);
> +ACCESS0_ATTR(read_latency);
> +ACCESS0_ATTR(write_bandwidth);
> +ACCESS0_ATTR(write_latency);
> +
> +static struct attribute *access0_coordinate_attrs[] = {
> +	&dev_attr_access0_read_bandwidth.attr,
> +	&dev_attr_access0_write_bandwidth.attr,
> +	&dev_attr_access0_read_latency.attr,
> +	&dev_attr_access0_write_latency.attr,
> +	NULL,
> +};
> +
> +static umode_t cxl_region_access0_coordinate_visible(struct kobject *kobj,
> +						     struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +
> +	if (a == &dev_attr_access0_read_latency.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_LOCAL].read_latency == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access0_write_latency.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_LOCAL].write_latency == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access0_read_bandwidth.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_LOCAL].read_bandwidth == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access0_write_bandwidth.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_LOCAL].write_bandwidth == 0)
> +		return 0;
> +
> +	return a->mode;
> +}
> +
> +#define __ACCESS1_ATTR_RO(_name) {				\
> +	.attr	= { .name = __stringify(_name), .mode = 0444 },	\
> +	.show	= _name##_access1_show,				\
> +}

Maybe define __ACCESSX_ATTR_RO(_name, _access)
etc to avoid some duplicaton.

> +
> +#define ACCESS1_DEVICE_ATTR_RO(_name) \
> +	struct device_attribute dev_attr_access1_##_name = __ACCESS1_ATTR_RO(_name)
> +
> +#define ACCESS1_ATTR(attrib)					\
> +static ssize_t attrib##_access1_show(struct device *dev,	\
> +			   struct device_attribute *attr,	\
> +			   char *buf)				\
> +{								\
> +	struct cxl_region *cxlr = to_cxl_region(dev);		\
> +								\
> +	if (cxlr->coord[1].attrib == 0)				\
> +		return -ENOENT;					\
> +								\
> +	return sysfs_emit(buf, "%u\n", cxlr->coord[1].attrib);	\
> +}								\
> +static ACCESS1_DEVICE_ATTR_RO(attrib)
> +
> +ACCESS1_ATTR(read_bandwidth);
> +ACCESS1_ATTR(read_latency);
> +ACCESS1_ATTR(write_bandwidth);
> +ACCESS1_ATTR(write_latency);
> +
> +static struct attribute *access1_coordinate_attrs[] = {
> +	&dev_attr_access1_read_bandwidth.attr,
> +	&dev_attr_access1_write_bandwidth.attr,
> +	&dev_attr_access1_read_latency.attr,
> +	&dev_attr_access1_write_latency.attr,
> +	NULL,
> +};
> +
> +static umode_t cxl_region_access1_coordinate_visible(struct kobject *kobj,
> +						     struct attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cxl_region *cxlr = to_cxl_region(dev);
> +
> +	if (a == &dev_attr_access1_read_latency.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_CPU].read_latency == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access1_write_latency.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_CPU].write_latency == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access1_read_bandwidth.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_CPU].read_bandwidth == 0)
> +		return 0;
> +
> +	if (a == &dev_attr_access1_write_bandwidth.attr &&
> +	    cxlr->coord[ACCESS_COORDINATE_CPU].write_bandwidth == 0)
> +		return 0;
> +
> +	return a->mode;
> +}
> +
> +static const struct attribute_group cxl_region_access0_coordinate_group = {
> +	.name = "access0",
> +	.attrs = access0_coordinate_attrs,
> +	.is_visible = cxl_region_access0_coordinate_visible,
> +};
> +
> +static const struct attribute_group cxl_region_access1_coordinate_group = {
> +	.name = "access1",
> +	.attrs = access1_coordinate_attrs,
> +	.is_visible = cxl_region_access1_coordinate_visible,
> +};
> +
>  static ssize_t uuid_show(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> @@ -2039,6 +2171,8 @@ static const struct attribute_group *region_groups[] = {
>  	&cxl_base_attribute_group,
>  	&cxl_region_group,
>  	&cxl_region_target_group,
> +	&cxl_region_access0_coordinate_group,
> +	&cxl_region_access1_coordinate_group,
>  	NULL,
>  };
>  

Re: [PATCH v5 11/12] cxl/region: Add memory hotplug notifier for cxl region

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:39 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> When the CXL region is formed, the driver would computed the performance
> data for the region. However this data is not available at the node data
> collection that has been populated by the HMAT during kernel
> initialization. Add a memory hotplug notifier to update the access
> coordinates to the 'struct memory_target' context kept by the
> HMAT_REPORTING code.
> 
> Add CXL_CALLBACK_PRI for a memory hotplug callback priority. Set the
> priority number to be called before HMAT_CALLBACK_PRI. The CXL update must
> happen before hmat_callback().
> 
> A new HMAT_REPORING helper hmat_update_target_coordinates() is added in
> order to allow CXL to update the memory_target access coordinates.
> 
> A new ext_updated member is added to the memory_target to indicate that
> the access coordinates within the memory_target has been updated by an
> external agent such as CXL. This prevents data being overwritten by the
> hmat_update_target_attrs() triggered by hmat_callback().
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

A few trivial things inline.

>  static __init void hmat_add_locality(struct acpi_hmat_locality *hmat_loc)
>  {
>  	struct memory_locality *loc;
> @@ -699,6 +729,12 @@ static void hmat_update_target_attrs(struct memory_target *target,
>  	u32 best = 0;
>  	int i;
>  
> +	/*
> +	 * Don't update if an external agent has changed the data.

Single line comment fine here I think.

> +	 */
> +	if (target->ext_updated)
> +		return;
> +



> +
> +static void remove_coord_notifier(void *data)
> +{
> +	struct cxl_region *cxlr = data;
> +
> +	unregister_memory_notifier(&cxlr->memory_notifier);

Simpler to pass in the notifier not the region.
More obvious that a remove notifier would take a notifier
and less code.

> +}
> +
>  static int cxl_region_probe(struct device *dev)
>  {
>  	struct cxl_region *cxlr = to_cxl_region(dev);
> @@ -3091,6 +3161,11 @@ static int cxl_region_probe(struct device *dev)
>  		goto out;
>  	}
>  
> +	cxlr->memory_notifier.notifier_call = cxl_region_perf_attrs_callback;
> +	cxlr->memory_notifier.priority = CXL_CALLBACK_PRI;
> +	register_memory_notifier(&cxlr->memory_notifier);
> +	rc = devm_add_action_or_reset(&cxlr->dev, remove_coord_notifier, cxlr);
> +
>  	/*
>  	 * From this point on any path that changes the region's state away from
>  	 * CXL_CONFIG_COMMIT is also responsible for releasing the driver.

Re: [PATCH v5 12/12] cxl/region: Deal with numa nodes not enumarated by SRAT

[Jonathan Cameron]

On Tue, 6 Feb 2024 15:28:40 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> For the numa nodes that are not created by SRAT, no memory_target is
> allocated and is not managed by the HMAT_REPORTING code. Therefore
> hmat_callback() memory hotplug notifier will exit early on those NUMA
> nodes. The CXL memory hotplug notifier will need to call
> node_set_perf_attrs() directly in order to setup the access sysfs
> attributes.
> 
> In acpi_numa_init(), the last proximity domain (pxm) id created by SRAT is
> stored. Add a helper function acpi_node_backed_by_real_pxm() in order to
> check if a NUMA node id is defined by SRAT or created by CFMWS.
> 
> node_set_perf_attrs() symbol is exported to allow update of perf attribs
> for a node. The sysfs path of
> /sys/devices/system/node/nodeX/access0/initiators/* is created by
> ndoe_set_perf_attrs() for the various attributes where nodeX is matched
node

> to the NUMA node of the CXL region.
> 
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Trivial comment inline.
Otherwise LGTM

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

> ---
> v5:
> - Fix ARM compile of missing prototype (0day)
> ---



> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 19e419f18472..db51e35cb44f 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3084,7 +3084,12 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>  
>  	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
>  		if (cxlr->coord[i].read_bandwidth) {
> -			rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
> +			rc = 0;
> +			if (cxl_need_node_perf_attrs_update(nid))
> +				node_set_perf_attrs(nid, &cxlr->coord[i], i);
maybe
continue;
here
> +			else
> +				rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
> +

Then no need to indent the above

>  			if (rc == 0)
>  				cset++;
>  		}

Re: [PATCH v5 05/12] cxl: Split out combine_coordinates() for common shared usage

[Dave Jiang]

On 2/15/24 9:51 AM, Jonathan Cameron wrote:
> On Tue, 6 Feb 2024 15:28:33 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Refactor the common code of combining coordinates in order to reduce code.
>> Create a new function cxl_cooordinates_combine() it combine two 'struct
>> access_coordinate'.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>> v5:
>> - Fix comment header (0-day)
>> - Remove EXPORT_SYMBOL (Dan)
>> ---
>>  drivers/cxl/core/cdat.c | 32 +++++++++++++++++++++++---------
>>  drivers/cxl/core/port.c | 18 ++----------------
>>  drivers/cxl/cxl.h       |  4 ++++
>>  3 files changed, 29 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
>> index 08fd0baea7a0..096320390ad9 100644
>> --- a/drivers/cxl/core/cdat.c
>> +++ b/drivers/cxl/core/cdat.c
>> @@ -185,15 +185,7 @@ static int cxl_port_perf_data_calculate(struct cxl_port *port,
>>  	xa_for_each(dsmas_xa, index, dent) {
>>  		int qos_class;
>>  
>> -		dent->coord.read_latency = dent->coord.read_latency +
>> -					   c.read_latency;
>> -		dent->coord.write_latency = dent->coord.write_latency +
>> -					    c.write_latency;
>> -		dent->coord.read_bandwidth = min_t(int, c.read_bandwidth,
>> -						   dent->coord.read_bandwidth);
>> -		dent->coord.write_bandwidth = min_t(int, c.write_bandwidth,
>> -						    dent->coord.write_bandwidth);
>> -
>> +		cxl_coordinates_combine(&dent->coord, &dent->coord, &c);
>>  		dent->entries = 1;
>>  		rc = cxl_root->ops->qos_class(cxl_root, &dent->coord, 1,
>>  					      &qos_class);
>> @@ -484,4 +476,26 @@ void cxl_switch_parse_cdat(struct cxl_port *port)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(cxl_switch_parse_cdat, CXL);
>>  
>> +/**
>> + * cxl_coordinates_combine - Combine the two input coordinates
>> + *
>> + * @out: Output coordinate of c1 and c2 combined
>> + * @c1: first coordinate, to be written to
> 
> When you say to be written to, what do you mean?
> Left over from adding out?

Yeah I'm not sure why it says that. Seems like half a sentence I left dangling. Going to change the block to below:

/**
 * cxl_coordinates_combine - Combine the two input coordinates
 *
 * @out: Output coordinate of c1 and c2 combined
 * @c1: input coordinates
 * @c2: input coordinates
 */


> 
> No obvious reason for this to have any idea that c1 and c2 are
> ordered.  
> 
>> + * @c2: second coordinate
>> + */
>> +void cxl_coordinates_combine(struct access_coordinate *out,
>> +			     struct access_coordinate *c1,
>> +			     struct access_coordinate *c2)
>> +{
>> +		if (c2->write_bandwidth)
>> +			out->write_bandwidth = min(c1->write_bandwidth,
>> +						   c2->write_bandwidth);
> 
> Why check c2->write_bandwidth?
> Code already does it, but I'm not sure why + I don't think you should
> treat c1 and c2 differently.

I think I need to check both and make sure that neither are 0 since we are taking the min of the two and both need to be valid.

> 
>> +		out->write_latency = c1->write_latency + c2->write_latency;
>> +
>> +		if (c2->read_bandwidth)
>> +			out->read_bandwidth = min(c1->read_bandwidth,
>> +						  c2->read_bandwidth);
>> +		out->read_latency = c1->read_latency + c2->read_latency;
>> +}
>> +
>>  MODULE_IMPORT_NS(CXL);
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 612bf7e1e847..af9458b2678c 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -2096,20 +2096,6 @@ bool schedule_cxl_memdev_detach(struct cxl_memdev *cxlmd)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(schedule_cxl_memdev_detach, CXL);
>>  
>> -static void combine_coordinates(struct access_coordinate *c1,
>> -				struct access_coordinate *c2)
>> -{
>> -		if (c2->write_bandwidth)
>> -			c1->write_bandwidth = min(c1->write_bandwidth,
>> -						  c2->write_bandwidth);
>> -		c1->write_latency += c2->write_latency;
>> -
>> -		if (c2->read_bandwidth)
>> -			c1->read_bandwidth = min(c1->read_bandwidth,
>> -						 c2->read_bandwidth);
>> -		c1->read_latency += c2->read_latency;
>> -}
>> -
>>  /**
>>   * cxl_endpoint_get_perf_coordinates - Retrieve performance numbers stored in dports
>>   *				   of CXL path
>> @@ -2143,7 +2129,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>>  	 * nothing to gather.
>>  	 */
>>  	while (iter && !is_cxl_root(to_cxl_port(iter->dev.parent))) {
>> -		combine_coordinates(&c, &dport->sw_coord);
>> +		cxl_coordinates_combine(&c, &c, &dport->sw_coord);
>>  		c.write_latency += dport->link_latency;
>>  		c.read_latency += dport->link_latency;
>>  
>> @@ -2152,7 +2138,7 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>>  	}
>>  
>>  	/* Augment with the generic port (host bridge) perf data */
>> -	combine_coordinates(&c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
>> +	cxl_coordinates_combine(&c, &c, &dport->hb_coord[ACCESS_COORDINATE_LOCAL]);
>>  
>>  	/* Get the calculated PCI paths bandwidth */
>>  	pdev = to_pci_dev(port->uport_dev->parent);
>> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
>> index fe7448f2745e..fab2da4b1f04 100644
>> --- a/drivers/cxl/cxl.h
>> +++ b/drivers/cxl/cxl.h
>> @@ -882,6 +882,10 @@ int cxl_endpoint_get_perf_coordinates(struct cxl_port *port,
>>  
>>  void cxl_memdev_update_perf(struct cxl_memdev *cxlmd);
>>  
>> +void cxl_coordinates_combine(struct access_coordinate *out,
>> +			     struct access_coordinate *c1,
>> +			     struct access_coordinate *c2);
>> +
>>  /*
>>   * Unit test builds overrides this to __weak, find the 'strong' version
>>   * of these symbols in tools/testing/cxl/.
> 

Re: [PATCH v5 12/12] cxl/region: Deal with numa nodes not enumarated by SRAT

[Dave Jiang]

On 2/15/24 10:24 AM, Jonathan Cameron wrote:
> On Tue, 6 Feb 2024 15:28:40 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> For the numa nodes that are not created by SRAT, no memory_target is
>> allocated and is not managed by the HMAT_REPORTING code. Therefore
>> hmat_callback() memory hotplug notifier will exit early on those NUMA
>> nodes. The CXL memory hotplug notifier will need to call
>> node_set_perf_attrs() directly in order to setup the access sysfs
>> attributes.
>>
>> In acpi_numa_init(), the last proximity domain (pxm) id created by SRAT is
>> stored. Add a helper function acpi_node_backed_by_real_pxm() in order to
>> check if a NUMA node id is defined by SRAT or created by CFMWS.
>>
>> node_set_perf_attrs() symbol is exported to allow update of perf attribs
>> for a node. The sysfs path of
>> /sys/devices/system/node/nodeX/access0/initiators/* is created by
>> ndoe_set_perf_attrs() for the various attributes where nodeX is matched
> node
> 
>> to the NUMA node of the CXL region.
>>
>> Cc: Rafael J. Wysocki <rafael@kernel.org>
>> Reviewed-by: Alison Schofield <alison.schofield@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Trivial comment inline.
> Otherwise LGTM
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>> v5:
>> - Fix ARM compile of missing prototype (0day)
>> ---
> 
> 
> 
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index 19e419f18472..db51e35cb44f 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -3084,7 +3084,12 @@ static bool cxl_region_update_coordinates(struct cxl_region *cxlr, int nid)
>>  
>>  	for (int i = 0; i < ACCESS_COORDINATE_MAX; i++) {
>>  		if (cxlr->coord[i].read_bandwidth) {
>> -			rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
>> +			rc = 0;
>> +			if (cxl_need_node_perf_attrs_update(nid))
>> +				node_set_perf_attrs(nid, &cxlr->coord[i], i);
> maybe
> continue;
> here

Whether we are updating the node attrs or the hmat perf attrs, we want to refresh the CXL sysfs attrs to make sure is_visible() has changed.

DJ
 
>> +			else
>> +				rc = cxl_update_hmat_access_coordinates(nid, cxlr, i);
>> +
> 
> Then no need to indent the above
> 
>>  			if (rc == 0)
>>  				cset++;
>>  		}
>