Re: [Xen-devel] [RFC PATCH v3 14/24] ARM: NUMA: DT: Parse NUMA distance information

2017-07-20 Thread Julien Grall 

Hi Vijay,

On 18/07/17 12:41, vijay.kil...@gmail.com wrote:

From: Vijaya Kumar K 

Parse distance-matrix and fetch node distance information.
Store distance information in node_distance[].

Register dt_node_distance() function pointer with
the ARM numa code. This approach can be later used for
ACPI.


After my comment on v1, I was expecting to see a link to the binding in 
the commit message...




Signed-off-by: Vijaya Kumar K 
---
v3: - Moved __node_distance() declaration to common
  header file
- Use device_tree_node_compatible() instead of
  device_tree_node_matches()
- Dropped xen/errno.h inclusion
---
 xen/arch/arm/numa/dt_numa.c | 131 
 xen/arch/arm/numa/numa.c|  22 
 xen/include/asm-arm/numa.h  |   2 +
 xen/include/asm-x86/numa.h  |   1 -
 xen/include/xen/numa.h  |   3 +
 5 files changed, 158 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/numa/dt_numa.c b/xen/arch/arm/numa/dt_numa.c
index 84030e7..46c0346 100644
--- a/xen/arch/arm/numa/dt_numa.c
+++ b/xen/arch/arm/numa/dt_numa.c
@@ -23,6 +23,48 @@
 #include 
 #include 

+static uint8_t node_distance[MAX_NUMNODES][MAX_NUMNODES];


On v1, you said that you will look at allocating node_distance on the 
fly. So why it is not done?


You can give a look at alloc_boot_pages(...).


+
+static uint8_t dt_node_distance(nodeid_t nodea, nodeid_t nodeb)
+{
+if ( nodea >= MAX_NUMNODES || nodeb >= MAX_NUMNODES )
+return nodea == nodeb ? LOCAL_DISTANCE : REMOTE_DISTANCE;


Do we really expect dt_node_distance to be called with wrong node?

Looking at the ACPI code, they don't check that... So likely this should 
be an ASSERT(...).



+
+return node_distance[nodea][nodeb];
+}
+
+static int dt_numa_set_distance(uint32_t nodea, uint32_t nodeb,


I think this should be __init.


+uint32_t distance)
+{
+   /* node_distance is uint8_t. Ensure distance is less than 255 */
+   if ( nodea >= MAX_NUMNODES || nodeb >= MAX_NUMNODES || distance > 255 )
+   return -EINVAL;
+
+   node_distance[nodea][nodeb] = distance;
+
+   return 0;
+}
+
+void init_dt_numa_distance(void)


Ditto.


+{
+int i, j;
+
+for ( i = 0; i < MAX_NUMNODES; i++ )
+{
+for ( j = 0; j < MAX_NUMNODES; j++ )
+{
+/*
+ * Initialize distance 10 for local distance and
+ * 20 for remote distance.
+ */
+if ( i  == j )
+node_distance[i][j] = LOCAL_DISTANCE;
+else
+node_distance[i][j] = REMOTE_DISTANCE;
+}
+}
+}
+
 /*
  * Even though we connect cpus to numa domains later in SMP
  * init, we need to know the node ids now for all cpus.
@@ -58,6 +100,76 @@ static int __init dt_numa_process_cpu_node(const void *fdt)
 return 0;
 }

+static int __init dt_numa_parse_distance_map(const void *fdt, int node,
+ const char *name,
+ uint32_t address_cells,
+ uint32_t size_cells)
+{
+const struct fdt_property *prop;
+const __be32 *matrix;
+int entry_count, len, i;
+
+printk(XENLOG_INFO "NUMA: parsing numa-distance-map\n");
+
+prop = fdt_get_property(fdt, node, "distance-matrix", );
+if ( !prop )
+{
+printk(XENLOG_WARNING


s/XENLOG_WARNING/XENLOG_INFO/ because numa-distance-map is not mandatory.


+   "NUMA: No distance-matrix property in distance-map\n");
+
+return -EINVAL;


If I am reading correctly the binding, the distance-matrix is not 
mandatory. If it is not present, you should use a default matrix. But 
here you will disable NUMA completely.



+}
+
+if ( len % sizeof(uint32_t) != 0 )
+{
+ printk(XENLOG_WARNING
+"distance-matrix in node is not a multiple of u32\n");
+
+return -EINVAL;
+}
+
+entry_count = len / sizeof(uint32_t);
+if ( entry_count <= 0 )
+{
+printk(XENLOG_WARNING "NUMA: Invalid distance-matrix\n");
+
+return -EINVAL;
+}
+
+matrix = (const __be32 *)prop->data;
+for ( i = 0; i + 2 < entry_count; i += 3 )


It would be easier to read if entry_count is the number of triplet. E.g

entry_count = (len / sizeof(uint32_t)) / 3;

for ( i = 0; i < entry_count; i++ )


+{
+uint32_t nodea, nodeb, distance;
+
+nodea = dt_read_number(matrix, 1);
+matrix++;


nodea = dt_next_cell(1, ) will do the increment for you.


+nodeb = dt_read_number(matrix, 1);
+matrix++;


Ditto.


+distance = dt_read_number(matrix, 1);
+matrix++;


Ditto.


+
+if ( dt_numa_set_distance(nodea, nodeb, distance) )
+{
+printk(XENLOG_WARNING
+   "NUMA: node-id out of range in distance matrix for [node%d -> 
node%d]\n",


s/%d/%u/


+

[Xen-devel] [RFC PATCH v3 14/24] ARM: NUMA: DT: Parse NUMA distance information

2017-07-18 Thread vijay . kilari 
From: Vijaya Kumar K 

Parse distance-matrix and fetch node distance information.
Store distance information in node_distance[].

Register dt_node_distance() function pointer with
the ARM numa code. This approach can be later used for
ACPI.

Signed-off-by: Vijaya Kumar K 
---
v3: - Moved __node_distance() declaration to common
  header file
- Use device_tree_node_compatible() instead of
  device_tree_node_matches()
- Dropped xen/errno.h inclusion
---
 xen/arch/arm/numa/dt_numa.c | 131 
 xen/arch/arm/numa/numa.c|  22 
 xen/include/asm-arm/numa.h  |   2 +
 xen/include/asm-x86/numa.h  |   1 -
 xen/include/xen/numa.h  |   3 +
 5 files changed, 158 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/numa/dt_numa.c b/xen/arch/arm/numa/dt_numa.c
index 84030e7..46c0346 100644
--- a/xen/arch/arm/numa/dt_numa.c
+++ b/xen/arch/arm/numa/dt_numa.c
@@ -23,6 +23,48 @@
 #include 
 #include 
 
+static uint8_t node_distance[MAX_NUMNODES][MAX_NUMNODES];
+
+static uint8_t dt_node_distance(nodeid_t nodea, nodeid_t nodeb)
+{
+if ( nodea >= MAX_NUMNODES || nodeb >= MAX_NUMNODES )
+return nodea == nodeb ? LOCAL_DISTANCE : REMOTE_DISTANCE;
+
+return node_distance[nodea][nodeb];
+}
+
+static int dt_numa_set_distance(uint32_t nodea, uint32_t nodeb,
+uint32_t distance)
+{
+   /* node_distance is uint8_t. Ensure distance is less than 255 */
+   if ( nodea >= MAX_NUMNODES || nodeb >= MAX_NUMNODES || distance > 255 )
+   return -EINVAL;
+
+   node_distance[nodea][nodeb] = distance;
+
+   return 0;
+}
+
+void init_dt_numa_distance(void)
+{
+int i, j;
+
+for ( i = 0; i < MAX_NUMNODES; i++ )
+{
+for ( j = 0; j < MAX_NUMNODES; j++ )
+{
+/*
+ * Initialize distance 10 for local distance and
+ * 20 for remote distance.
+ */
+if ( i  == j )
+node_distance[i][j] = LOCAL_DISTANCE;
+else
+node_distance[i][j] = REMOTE_DISTANCE;
+}
+}
+}
+
 /*
  * Even though we connect cpus to numa domains later in SMP
  * init, we need to know the node ids now for all cpus.
@@ -58,6 +100,76 @@ static int __init dt_numa_process_cpu_node(const void *fdt)
 return 0;
 }
 
+static int __init dt_numa_parse_distance_map(const void *fdt, int node,
+ const char *name,
+ uint32_t address_cells,
+ uint32_t size_cells)
+{
+const struct fdt_property *prop;
+const __be32 *matrix;
+int entry_count, len, i;
+
+printk(XENLOG_INFO "NUMA: parsing numa-distance-map\n");
+
+prop = fdt_get_property(fdt, node, "distance-matrix", );
+if ( !prop )
+{
+printk(XENLOG_WARNING
+   "NUMA: No distance-matrix property in distance-map\n");
+
+return -EINVAL;
+}
+
+if ( len % sizeof(uint32_t) != 0 )
+{
+ printk(XENLOG_WARNING
+"distance-matrix in node is not a multiple of u32\n");
+
+return -EINVAL;
+}
+
+entry_count = len / sizeof(uint32_t);
+if ( entry_count <= 0 )
+{
+printk(XENLOG_WARNING "NUMA: Invalid distance-matrix\n");
+
+return -EINVAL;
+}
+
+matrix = (const __be32 *)prop->data;
+for ( i = 0; i + 2 < entry_count; i += 3 )
+{
+uint32_t nodea, nodeb, distance;
+
+nodea = dt_read_number(matrix, 1);
+matrix++;
+nodeb = dt_read_number(matrix, 1);
+matrix++;
+distance = dt_read_number(matrix, 1);
+matrix++;
+
+if ( dt_numa_set_distance(nodea, nodeb, distance) )
+{
+printk(XENLOG_WARNING
+   "NUMA: node-id out of range in distance matrix for [node%d 
-> node%d]\n",
+   nodea, nodeb);
+return -EINVAL;
+
+}
+printk(XENLOG_INFO "NUMA: distance[node%d -> node%d] = %d\n",
+   nodea, nodeb, distance);
+
+/*
+ * Set default distance of node B->A same as A->B.
+ * No need to check for return value of numa_set_distance.
+ */
+if ( nodeb > nodea )
+dt_numa_set_distance(nodeb, nodea, distance);
+}
+
+return 0;
+}
+
 void __init dt_numa_process_memory_node(uint32_t nid, paddr_t start,
paddr_t size)
 {
@@ -90,11 +202,30 @@ void __init dt_numa_process_memory_node(uint32_t nid, 
paddr_t start,
 return;
 }
 
+static int __init dt_numa_scan_distance_node(const void *fdt, int node,
+ const char *name, int depth,
+ uint32_t address_cells,
+ uint32_t size_cells, void *data)
+{
+if ( device_tree_node_compatible(fdt, node,