[PATCH 0/4] acpi: numa: add target support for generic port to HMAT parsing

[PATCH 0/4] acpi: numa: add target support for generic port to HMAT parsing

[Dave Jiang]

Hi Rafael,
This is the second set of ACPI changes that's part of the "cxl: Add support for
QTG ID retrieval for CXL subsystem" [1]. It has dependency on the patch [2]
that was sent to you and GregKH. But it's independent of the first set of
ACPI changes [3]. The series adds support to store the device handle from the
SRAT Generic Port Affinity Structure in correlation with a proximity domain. And
later on the HMAT is parsed and the performance data is stored associated with
the proximity domain. A helper function is added to allow the CXL driver to
retrieve the performance data that matches with the device handle later on. The
full series of CXL code is at this git repo [4] for usage reference. Please
review and consider for v6.5 inclusion if acceptable. Thank you!


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

---

Dave Jiang (4):
      acpi: numa: Create enum for memory_target access coordinates indexing
      acpi: numa: Add genport target allocation to the HMAT parsing
      acpi: numa: Add setting of generic port system locality attributes
      acpi: numa: Add helper function to retrieve the performance attributes


 drivers/acpi/numa/hmat.c | 130 ++++++++++++++++++++++++++++++++++-----
 include/acpi/actbl3.h    |   4 +-
 include/linux/acpi.h     |   9 +++
 3 files changed, 128 insertions(+), 15 deletions(-)

--

[PATCH 1/4] acpi: numa: Create enum for memory_target access coordinates indexing

[Dave Jiang]

Create enums to provide named indexing for the access coordinate array.
This is in preparation for adding generic port support which will add a
third index in the array to keep the generic port attributes separate from
the memory attributes.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c |   35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index f9ff992038fa..abed728bf09d 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -57,12 +57,18 @@ struct target_cache {
 	struct node_cache_attrs cache_attrs;
 };
 
+enum {
+	NODE_ACCESS_CLASS_0 = 0,
+	NODE_ACCESS_CLASS_1,
+	NODE_ACCESS_CLASS_MAX,
+};
+
 struct memory_target {
 	struct list_head node;
 	unsigned int memory_pxm;
 	unsigned int processor_pxm;
 	struct resource memregions;
-	struct access_coordinate coord[2];
+	struct access_coordinate coord[NODE_ACCESS_CLASS_MAX];
 	struct list_head caches;
 	struct node_cache_attrs cache_attrs;
 	bool registered;
@@ -338,10 +344,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
 			if (mem_hier == ACPI_HMAT_MEMORY) {
 				target = find_mem_target(targs[targ]);
 				if (target && target->processor_pxm == inits[init]) {
-					hmat_update_target_access(target, type, value, 0);
+					hmat_update_target_access(target, type, value,
+								  NODE_ACCESS_CLASS_0);
 					/* If the node has a CPU, update access 1 */
 					if (node_state(pxm_to_node(inits[init]), N_CPU))
-						hmat_update_target_access(target, type, value, 1);
+						hmat_update_target_access(target, type, value,
+									  NODE_ACCESS_CLASS_1);
 				}
 			}
 		}
@@ -600,10 +608,12 @@ static void hmat_register_target_initiators(struct memory_target *target)
 	 */
 	if (target->processor_pxm != PXM_INVAL) {
 		cpu_nid = pxm_to_node(target->processor_pxm);
-		register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
+		register_memory_node_under_compute_node(mem_nid, cpu_nid,
+							NODE_ACCESS_CLASS_0);
 		access0done = true;
 		if (node_state(cpu_nid, N_CPU)) {
-			register_memory_node_under_compute_node(mem_nid, cpu_nid, 1);
+			register_memory_node_under_compute_node(mem_nid, cpu_nid,
+								NODE_ACCESS_CLASS_1);
 			return;
 		}
 	}
@@ -644,12 +654,13 @@ static void hmat_register_target_initiators(struct memory_target *target)
 			}
 			if (best)
 				hmat_update_target_access(target, loc->hmat_loc->data_type,
-							  best, 0);
+							  best, NODE_ACCESS_CLASS_0);
 		}
 
 		for_each_set_bit(i, p_nodes, MAX_NUMNODES) {
 			cpu_nid = pxm_to_node(i);
-			register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
+			register_memory_node_under_compute_node(mem_nid, cpu_nid,
+								NODE_ACCESS_CLASS_0);
 		}
 	}
 
@@ -681,11 +692,13 @@ static void hmat_register_target_initiators(struct memory_target *target)
 				clear_bit(initiator->processor_pxm, p_nodes);
 		}
 		if (best)
-			hmat_update_target_access(target, loc->hmat_loc->data_type, best, 1);
+			hmat_update_target_access(target, loc->hmat_loc->data_type, best,
+						  NODE_ACCESS_CLASS_1);
 	}
 	for_each_set_bit(i, p_nodes, MAX_NUMNODES) {
 		cpu_nid = pxm_to_node(i);
-		register_memory_node_under_compute_node(mem_nid, cpu_nid, 1);
+		register_memory_node_under_compute_node(mem_nid, cpu_nid,
+							NODE_ACCESS_CLASS_1);
 	}
 }
 
@@ -746,8 +759,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, 0);
-		hmat_register_target_perf(target, 1);
+		hmat_register_target_perf(target, NODE_ACCESS_CLASS_0);
+		hmat_register_target_perf(target, NODE_ACCESS_CLASS_1);
 		target->registered = true;
 	}
 	mutex_unlock(&target_lock);

[PATCH 2/4] acpi: numa: Add genport target allocation to the HMAT parsing

[Dave Jiang]

Add SRAT parsing for the HMAT init in order to collect the device handle
from the Generic Port Affinity Structure. The devie handle will serve as
the key to search for target data.

Consoliate the common code with alloc_memory_target() in a helper function
alloc_target().

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c |   53 +++++++++++++++++++++++++++++++++++++++++++---
 include/acpi/actbl3.h    |    4 +++
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index abed728bf09d..e2ab1cce0add 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -71,6 +71,7 @@ struct memory_target {
 	struct access_coordinate coord[NODE_ACCESS_CLASS_MAX];
 	struct list_head caches;
 	struct node_cache_attrs cache_attrs;
+	u8 device_handle[ACPI_SRAT_DEVICE_HANDLE_SIZE];
 	bool registered;
 };
 
@@ -125,8 +126,7 @@ static __init void alloc_memory_initiator(unsigned int cpu_pxm)
 	list_add_tail(&initiator->node, &initiators);
 }
 
-static __init void alloc_memory_target(unsigned int mem_pxm,
-		resource_size_t start, resource_size_t len)
+static __init struct memory_target *alloc_target(unsigned int mem_pxm)
 {
 	struct memory_target *target;
 
@@ -134,7 +134,7 @@ static __init void alloc_memory_target(unsigned int mem_pxm,
 	if (!target) {
 		target = kzalloc(sizeof(*target), GFP_KERNEL);
 		if (!target)
-			return;
+			return NULL;
 		target->memory_pxm = mem_pxm;
 		target->processor_pxm = PXM_INVAL;
 		target->memregions = (struct resource) {
@@ -147,6 +147,19 @@ static __init void alloc_memory_target(unsigned int mem_pxm,
 		INIT_LIST_HEAD(&target->caches);
 	}
 
+	return target;
+}
+
+static __init void alloc_memory_target(unsigned int mem_pxm,
+				       resource_size_t start,
+				       resource_size_t len)
+{
+	struct memory_target *target;
+
+	target = alloc_target(mem_pxm);
+	if (!target)
+		return;
+
 	/*
 	 * There are potentially multiple ranges per PXM, so record each
 	 * in the per-target memregions resource tree.
@@ -157,6 +170,17 @@ static __init void alloc_memory_target(unsigned int mem_pxm,
 				start, start + len, mem_pxm);
 }
 
+static __init void alloc_genport_target(unsigned int mem_pxm, u8 *handle)
+{
+	struct memory_target *target;
+
+	target = alloc_target(mem_pxm);
+	if (!target)
+		return;
+
+	memcpy(target->device_handle, handle, ACPI_SRAT_DEVICE_HANDLE_SIZE);
+}
+
 static __init const char *hmat_data_type(u8 type)
 {
 	switch (type) {
@@ -498,6 +522,22 @@ static __init int srat_parse_mem_affinity(union acpi_subtable_headers *header,
 	return 0;
 }
 
+static __init int srat_parse_genport_affinity(union acpi_subtable_headers *header,
+					      const unsigned long end)
+{
+	struct acpi_srat_generic_affinity *ga = (void *)header;
+
+	if (!ga)
+		return -EINVAL;
+
+	if (!(ga->flags & ACPI_SRAT_GENERIC_AFFINITY_ENABLED))
+		return 0;
+
+	alloc_genport_target(ga->proximity_domain, (u8 *)ga->device_handle);
+
+	return 0;
+}
+
 static u32 hmat_initiator_perf(struct memory_target *target,
 			       struct memory_initiator *initiator,
 			       struct acpi_hmat_locality *hmat_loc)
@@ -848,6 +888,13 @@ static __init int hmat_init(void)
 				ACPI_SRAT_TYPE_MEMORY_AFFINITY,
 				srat_parse_mem_affinity, 0) < 0)
 		goto out_put;
+
+	if (acpi_table_parse_entries(ACPI_SIG_SRAT,
+				     sizeof(struct acpi_table_srat),
+				     ACPI_SRAT_TYPE_GENERIC_PORT_AFFINITY,
+				     srat_parse_genport_affinity, 0) < 0)
+		goto out_put;
+
 	acpi_put_table(tbl);
 
 	status = acpi_get_table(ACPI_SIG_HMAT, 0, &tbl);
diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
index 832c6464f063..ed293d041211 100644
--- a/include/acpi/actbl3.h
+++ b/include/acpi/actbl3.h
@@ -279,12 +279,14 @@ struct acpi_srat_gic_its_affinity {
  * 6: ACPI_SRAT_TYPE_GENERIC_PORT_AFFINITY
  */
 
+#define ACPI_SRAT_DEVICE_HANDLE_SIZE		16
+
 struct acpi_srat_generic_affinity {
 	struct acpi_subtable_header header;
 	u8 reserved;
 	u8 device_handle_type;
 	u32 proximity_domain;
-	u8 device_handle[16];
+	u8 device_handle[ACPI_SRAT_DEVICE_HANDLE_SIZE];
 	u32 flags;
 	u32 reserved1;
 };

[PATCH 3/4] acpi: numa: Add setting of generic port system locality attributes

[Dave Jiang]

Add generic port support for the parsing of HMAT system locality sub-table.
The attributes will be added to the third array member of the access
coordinates in order to not mix with the existing memory attributes it only
provides the system locality attributes from initator to the generic port
targets and is missing the rest of the data to the actual memory device.

The complete attributes will be updated when a memory device is
attached and the system locality information is calculated end to end.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index e2ab1cce0add..951579e903cf 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -60,6 +60,7 @@ struct target_cache {
 enum {
 	NODE_ACCESS_CLASS_0 = 0,
 	NODE_ACCESS_CLASS_1,
+	NODE_ACCESS_CLASS_GENPORT,
 	NODE_ACCESS_CLASS_MAX,
 };
 
@@ -368,6 +369,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
 			if (mem_hier == ACPI_HMAT_MEMORY) {
 				target = find_mem_target(targs[targ]);
 				if (target && target->processor_pxm == inits[init]) {
+					if (*target->device_handle) {
+						hmat_update_target_access(target, type, value,
+								NODE_ACCESS_CLASS_GENPORT);
+						continue;
+					}
+
 					hmat_update_target_access(target, type, value,
 								  NODE_ACCESS_CLASS_0);
 					/* If the node has a CPU, update access 1 */

[PATCH 4/4] acpi: numa: Add helper function to retrieve the performance attributes

[Dave Jiang]

Add helper to retrieve the performance attributes based on the device
handle.  The helper function is exported so the CXL driver can use that
to acquire the performance data between the CPU and the CXL host bridge.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/acpi/numa/hmat.c |   35 +++++++++++++++++++++++++++++++++++
 include/linux/acpi.h     |    9 +++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 951579e903cf..73d716e6096e 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -107,6 +107,41 @@ static struct memory_target *find_mem_target(unsigned int mem_pxm)
 	return NULL;
 }
 
+static struct memory_target *acpi_find_genport_target(u8 *device_handle)
+{
+	struct memory_target *target;
+
+	list_for_each_entry(target, &targets, node) {
+		if (!strncmp(target->device_handle, device_handle,
+			     ACPI_SRAT_DEVICE_HANDLE_SIZE))
+			return target;
+	}
+
+	return NULL;
+}
+
+/**
+ * acpi_get_genport_coordinates - Retrieve the access coordinates for a generic port
+ * @device_handle: Device handle string (ACPI or PCI) to match up to the gen port
+ * @coord: The access coordinates written back out for the generic port
+ *
+ * Return: 0 on success. Errno on failure.
+ */
+int acpi_get_genport_coordinates(u8 *device_handle,
+				 struct access_coordinate *coord)
+{
+	struct memory_target *target;
+
+	target = acpi_find_genport_target(device_handle);
+	if (!target)
+		return -ENOENT;
+
+	*coord = target->coord[NODE_ACCESS_CLASS_GENPORT];
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(acpi_get_genport_coordinates, CXL);
+
 static __init void alloc_memory_initiator(unsigned int cpu_pxm)
 {
 	struct memory_initiator *initiator;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 4c3dfe7587e9..d6a99fa430dd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/property.h>
 #include <linux/uuid.h>
+#include <linux/node.h>
 
 #ifndef _LINUX
 #define _LINUX
@@ -455,6 +456,8 @@ extern bool acpi_osi_is_win8(void);
 #ifdef CONFIG_ACPI_NUMA
 int acpi_map_pxm_to_node(int pxm);
 int acpi_get_node(acpi_handle handle);
+int acpi_get_genport_coordinates(u8 *device_handle,
+				 struct access_coordinate *coord);
 
 /**
  * pxm_to_online_node - Map proximity ID to online node
@@ -489,6 +492,12 @@ static inline int acpi_get_node(acpi_handle handle)
 {
 	return 0;
 }
+
+static inline int acpi_get_genport_coordinates(u8 *device_handle,
+					       struct access_coordinate *coord)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 extern int acpi_paddr_to_node(u64 start_addr, u64 size);
 

Re: [PATCH 1/4] acpi: numa: Create enum for memory_target access coordinates indexing

[Jonathan Cameron]

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

> Create enums to provide named indexing for the access coordinate array.
> This is in preparation for adding generic port support which will add a
> third index in the array to keep the generic port attributes separate from
> the memory attributes.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

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



> ---
>  drivers/acpi/numa/hmat.c |   35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index f9ff992038fa..abed728bf09d 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -57,12 +57,18 @@ struct target_cache {
>  	struct node_cache_attrs cache_attrs;
>  };
>  
> +enum {
> +	NODE_ACCESS_CLASS_0 = 0,
> +	NODE_ACCESS_CLASS_1,
> +	NODE_ACCESS_CLASS_MAX,
> +};
> +
>  struct memory_target {
>  	struct list_head node;
>  	unsigned int memory_pxm;
>  	unsigned int processor_pxm;
>  	struct resource memregions;
> -	struct access_coordinate coord[2];
> +	struct access_coordinate coord[NODE_ACCESS_CLASS_MAX];
>  	struct list_head caches;
>  	struct node_cache_attrs cache_attrs;
>  	bool registered;
> @@ -338,10 +344,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
>  			if (mem_hier == ACPI_HMAT_MEMORY) {
>  				target = find_mem_target(targs[targ]);
>  				if (target && target->processor_pxm == inits[init]) {
> -					hmat_update_target_access(target, type, value, 0);
> +					hmat_update_target_access(target, type, value,
> +								  NODE_ACCESS_CLASS_0);
>  					/* If the node has a CPU, update access 1 */
>  					if (node_state(pxm_to_node(inits[init]), N_CPU))
> -						hmat_update_target_access(target, type, value, 1);
> +						hmat_update_target_access(target, type, value,
> +									  NODE_ACCESS_CLASS_1);
>  				}
>  			}
>  		}
> @@ -600,10 +608,12 @@ static void hmat_register_target_initiators(struct memory_target *target)
>  	 */
>  	if (target->processor_pxm != PXM_INVAL) {
>  		cpu_nid = pxm_to_node(target->processor_pxm);
> -		register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
> +		register_memory_node_under_compute_node(mem_nid, cpu_nid,
> +							NODE_ACCESS_CLASS_0);
>  		access0done = true;
>  		if (node_state(cpu_nid, N_CPU)) {
> -			register_memory_node_under_compute_node(mem_nid, cpu_nid, 1);
> +			register_memory_node_under_compute_node(mem_nid, cpu_nid,
> +								NODE_ACCESS_CLASS_1);
>  			return;
>  		}
>  	}
> @@ -644,12 +654,13 @@ static void hmat_register_target_initiators(struct memory_target *target)
>  			}
>  			if (best)
>  				hmat_update_target_access(target, loc->hmat_loc->data_type,
> -							  best, 0);
> +							  best, NODE_ACCESS_CLASS_0);
>  		}
>  
>  		for_each_set_bit(i, p_nodes, MAX_NUMNODES) {
>  			cpu_nid = pxm_to_node(i);
> -			register_memory_node_under_compute_node(mem_nid, cpu_nid, 0);
> +			register_memory_node_under_compute_node(mem_nid, cpu_nid,
> +								NODE_ACCESS_CLASS_0);
>  		}
>  	}
>  
> @@ -681,11 +692,13 @@ static void hmat_register_target_initiators(struct memory_target *target)
>  				clear_bit(initiator->processor_pxm, p_nodes);
>  		}
>  		if (best)
> -			hmat_update_target_access(target, loc->hmat_loc->data_type, best, 1);
> +			hmat_update_target_access(target, loc->hmat_loc->data_type, best,
> +						  NODE_ACCESS_CLASS_1);
>  	}
>  	for_each_set_bit(i, p_nodes, MAX_NUMNODES) {
>  		cpu_nid = pxm_to_node(i);
> -		register_memory_node_under_compute_node(mem_nid, cpu_nid, 1);
> +		register_memory_node_under_compute_node(mem_nid, cpu_nid,
> +							NODE_ACCESS_CLASS_1);
>  	}
>  }
>  
> @@ -746,8 +759,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, 0);
> -		hmat_register_target_perf(target, 1);
> +		hmat_register_target_perf(target, NODE_ACCESS_CLASS_0);
> +		hmat_register_target_perf(target, NODE_ACCESS_CLASS_1);
>  		target->registered = true;
>  	}
>  	mutex_unlock(&target_lock);
> 
> 

Re: [PATCH 2/4] acpi: numa: Add genport target allocation to the HMAT parsing

[Jonathan Cameron]

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

> Add SRAT parsing for the HMAT init in order to collect the device handle
> from the Generic Port Affinity Structure. The devie handle will serve as

device

> the key to search for target data.
> 
> Consoliate the common code with alloc_memory_target() in a helper function
> alloc_target().
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

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

Re: [PATCH 3/4] acpi: numa: Add setting of generic port system locality attributes

[Jonathan Cameron]

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

> Add generic port support for the parsing of HMAT system locality sub-table.
> The attributes will be added to the third array member of the access
> coordinates in order to not mix with the existing memory attributes it only

attributes. It only ?
Otherwise I'm having trouble following that sentence.

> provides the system locality attributes from initator to the generic port
> targets and is missing the rest of the data to the actual memory device.
> 
> The complete attributes will be updated when a memory device is
> attached and the system locality information is calculated end to end.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/acpi/numa/hmat.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index e2ab1cce0add..951579e903cf 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -60,6 +60,7 @@ struct target_cache {
>  enum {
>  	NODE_ACCESS_CLASS_0 = 0,
>  	NODE_ACCESS_CLASS_1,
> +	NODE_ACCESS_CLASS_GENPORT,
>  	NODE_ACCESS_CLASS_MAX,
>  };
>  
> @@ -368,6 +369,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
>  			if (mem_hier == ACPI_HMAT_MEMORY) {
>  				target = find_mem_target(targs[targ]);
>  				if (target && target->processor_pxm == inits[init]) {
> +					if (*target->device_handle) {
> +						hmat_update_target_access(target, type, value,
> +								NODE_ACCESS_CLASS_GENPORT);
> +						continue;

There isn't anything that I know of that says a generic port has to be in it's own PXM.

In theory at least they could share one with CPUs or Memory.
Sure the access info will be inaccurate, but meh, it's a firmware table and if the spec
doesn't exclude it then someone might build it.

So rather than continuing here I think you need to add a check on there being memory
or not before deciding not to update the other access classes.

Jonathan


> +					}
> +
>  					hmat_update_target_access(target, type, value,
>  								  NODE_ACCESS_CLASS_0);
>  					/* If the node has a CPU, update access 1 */
> 
> 

Re: [PATCH 3/4] acpi: numa: Add setting of generic port system locality attributes

[Jonathan Cameron]

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

> Add generic port support for the parsing of HMAT system locality sub-table.
> The attributes will be added to the third array member of the access
> coordinates in order to not mix with the existing memory attributes it only
> provides the system locality attributes from initator to the generic port
> targets and is missing the rest of the data to the actual memory device.
> 
> The complete attributes will be updated when a memory device is
> attached and the system locality information is calculated end to end.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Strange question for you.  For now we have it easy as we only have Type 3
CXL devices using this.  Later when we have accelerators, then the generic
port becomes both an initiator and a target (well proxy of one in both cases).
We'll probably want to map the linux view of a generic initiator (CPU less
node) on top of the accelerator using the access characteristics from HMAT +
other parts.

Any thoughts on how to extend this approach to that case?

I can see it might need another access class so that normal memory for example
can be the target of a generic port.  

Maybe it's worth predicting that and renaming CLASS_GENPORT to CLASS_GENPORT_SINK
or something along those lines?

Or just leave it until someone cares?  I'm fine with this answer if you agree ;)

Jonathan

> ---
>  drivers/acpi/numa/hmat.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index e2ab1cce0add..951579e903cf 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -60,6 +60,7 @@ struct target_cache {
>  enum {
>  	NODE_ACCESS_CLASS_0 = 0,
>  	NODE_ACCESS_CLASS_1,
> +	NODE_ACCESS_CLASS_GENPORT,
>  	NODE_ACCESS_CLASS_MAX,
>  };
>  
> @@ -368,6 +369,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
>  			if (mem_hier == ACPI_HMAT_MEMORY) {
>  				target = find_mem_target(targs[targ]);
>  				if (target && target->processor_pxm == inits[init]) {
> +					if (*target->device_handle) {
> +						hmat_update_target_access(target, type, value,
> +								NODE_ACCESS_CLASS_GENPORT);
> +						continue;
> +					}
> +
>  					hmat_update_target_access(target, type, value,
>  								  NODE_ACCESS_CLASS_0);
>  					/* If the node has a CPU, update access 1 */
> 
> 

Re: [PATCH 3/4] acpi: numa: Add setting of generic port system locality attributes

[Dave Jiang]

On 5/12/23 9:16 AM, Jonathan Cameron wrote:
> On Fri, 05 May 2023 17:05:28 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add generic port support for the parsing of HMAT system locality sub-table.
>> The attributes will be added to the third array member of the access
>> coordinates in order to not mix with the existing memory attributes it only
>> provides the system locality attributes from initator to the generic port
>> targets and is missing the rest of the data to the actual memory device.
>>
>> The complete attributes will be updated when a memory device is
>> attached and the system locality information is calculated end to end.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> Strange question for you.  For now we have it easy as we only have Type 3
> CXL devices using this.  Later when we have accelerators, then the generic
> port becomes both an initiator and a target (well proxy of one in both cases).
> We'll probably want to map the linux view of a generic initiator (CPU less
> node) on top of the accelerator using the access characteristics from HMAT +
> other parts.
> 
> Any thoughts on how to extend this approach to that case?

I have not given much thought on it. But I'll think on it.

> 
> I can see it might need another access class so that normal memory for example
> can be the target of a generic port.
> 
> Maybe it's worth predicting that and renaming CLASS_GENPORT to CLASS_GENPORT_SINK
> or something along those lines?

Sure. We can name it to something more specific with an eye on the 
future. But we can deal with the rest when the time comes.

> 
> Or just leave it until someone cares?  I'm fine with this answer if you agree ;)
> 
> Jonathan
> 
>> ---
>>   drivers/acpi/numa/hmat.c |    7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
>> index e2ab1cce0add..951579e903cf 100644
>> --- a/drivers/acpi/numa/hmat.c
>> +++ b/drivers/acpi/numa/hmat.c
>> @@ -60,6 +60,7 @@ struct target_cache {
>>   enum {
>>   	NODE_ACCESS_CLASS_0 = 0,
>>   	NODE_ACCESS_CLASS_1,
>> +	NODE_ACCESS_CLASS_GENPORT,
>>   	NODE_ACCESS_CLASS_MAX,
>>   };
>>   
>> @@ -368,6 +369,12 @@ static __init int hmat_parse_locality(union acpi_subtable_headers *header,
>>   			if (mem_hier == ACPI_HMAT_MEMORY) {
>>   				target = find_mem_target(targs[targ]);
>>   				if (target && target->processor_pxm == inits[init]) {
>> +					if (*target->device_handle) {
>> +						hmat_update_target_access(target, type, value,
>> +								NODE_ACCESS_CLASS_GENPORT);
>> +						continue;
>> +					}
>> +
>>   					hmat_update_target_access(target, type, value,
>>   								  NODE_ACCESS_CLASS_0);
>>   					/* If the node has a CPU, update access 1 */
>>
>>
> 

Re: [PATCH 4/4] acpi: numa: Add helper function to retrieve the performance attributes

[Jonathan Cameron]

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

> Add helper to retrieve the performance attributes based on the device
> handle.  The helper function is exported so the CXL driver can use that
> to acquire the performance data between the CPU and the CXL host bridge.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Trivial comment inline. Otherwise LGTM

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

> ---
>  drivers/acpi/numa/hmat.c |   35 +++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h     |    9 +++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index 951579e903cf..73d716e6096e 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -107,6 +107,41 @@ static struct memory_target *find_mem_target(unsigned int mem_pxm)
>  	return NULL;
>  }
>  
> +static struct memory_target *acpi_find_genport_target(u8 *device_handle)
> +{
> +	struct memory_target *target;
> +
> +	list_for_each_entry(target, &targets, node) {
> +		if (!strncmp(target->device_handle, device_handle,
> +			     ACPI_SRAT_DEVICE_HANDLE_SIZE))

Using this for something that isn't a string?
memcmp() ?

Mind you I'm not sure what the nameless author this code was doing
in packing the device handle as a u8 array rather than a union of the two
types it can contain.  They probably had a reason lost to the mists of time...

> +			return target;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * acpi_get_genport_coordinates - Retrieve the access coordinates for a generic port
> + * @device_handle: Device handle string (ACPI or PCI) to match up to the gen port
> + * @coord: The access coordinates written back out for the generic port
> + *
> + * Return: 0 on success. Errno on failure.
> + */
> +int acpi_get_genport_coordinates(u8 *device_handle,
> +				 struct access_coordinate *coord)
> +{
> +	struct memory_target *target;
> +
> +	target = acpi_find_genport_target(device_handle);
> +	if (!target)
> +		return -ENOENT;
> +
> +	*coord = target->coord[NODE_ACCESS_CLASS_GENPORT];
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(acpi_get_genport_coordinates, CXL);
> +
>  static __init void alloc_memory_initiator(unsigned int cpu_pxm)
>  {
>  	struct memory_initiator *initiator;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 4c3dfe7587e9..d6a99fa430dd 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -15,6 +15,7 @@
>  #include <linux/device.h>
>  #include <linux/property.h>
>  #include <linux/uuid.h>
> +#include <linux/node.h>
>  
>  #ifndef _LINUX
>  #define _LINUX
> @@ -455,6 +456,8 @@ extern bool acpi_osi_is_win8(void);
>  #ifdef CONFIG_ACPI_NUMA
>  int acpi_map_pxm_to_node(int pxm);
>  int acpi_get_node(acpi_handle handle);
> +int acpi_get_genport_coordinates(u8 *device_handle,
> +				 struct access_coordinate *coord);
>  
>  /**
>   * pxm_to_online_node - Map proximity ID to online node
> @@ -489,6 +492,12 @@ static inline int acpi_get_node(acpi_handle handle)
>  {
>  	return 0;
>  }
> +
> +static inline int acpi_get_genport_coordinates(u8 *device_handle,
> +					       struct access_coordinate *coord)
> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif
>  extern int acpi_paddr_to_node(u64 start_addr, u64 size);
>  
> 
>