Re: [PATCH v7 5/6] powerpc/pseries: Add support for FORM2 associativity

On Wed, Aug 11, 2021 at 09:39:32AM +0530, Aneesh Kumar K.V wrote:

David Gibson [off-list ref] writes:

quoted

On Mon, Aug 09, 2021 at 10:54:33AM +0530, Aneesh Kumar K.V wrote:

quoted

PAPR interface currently supports two different ways of communicating resource
grouping details to the OS. These are referred to as Form 0 and Form 1
associativity grouping. Form 0 is the older format and is now considered
deprecated. This patch adds another resource grouping named FORM2.

Signed-off-by: Daniel Henrique Barboza <redacted>
Signed-off-by: Aneesh Kumar K.V <redacted>

LGTM, with the exception of some minor nits noted below.

...

quoted

+

quoted

+	for (i = 0; i < max_numa_index; i++)
+		/* +1 skip the max_numa_index in the property */
+		numa_id_index_table[i] = of_read_number(&numa_lookup_index[i + 1], 1);
+
+
+	if (numa_dist_table_length != max_numa_index * max_numa_index) {
+

Stray extra whitespace line here.

quoted

+		WARN(1, "Wrong NUMA distance information\n");
+		/* consider everybody else just remote. */
+		for (i = 0;  i < max_numa_index; i++) {
+			for (j = 0; j < max_numa_index; j++) {
+				int nodeA = numa_id_index_table[i];
+				int nodeB = numa_id_index_table[j];
+
+				if (nodeA == nodeB)
+					numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
+				else
+					numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
+			}
+		}

I don't think it's necessarily a problem, but something to consider is
that this fallback will initialize distance for *all* node IDs,
whereas the normal path will only initialize it for nodes that are in
the index table.  Since some later error checks key off whether
certain fields in the distance table are initialized, is that the
outcome you want?

With the device tree details not correct, one of the possible way to
make progress is to consider everybody remote. With new node hotplug
support we used to check whether the distance table entry is
initialized. With the updated spec, we expect all possible numa node
distance to be available during boot.

Sure.  But my main point here is that the fallback behaviour in this
clause is different from the fallback behaviour if the table is there
and parseable, but incomplete - which is also not expected.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson