[v2,7/9] sparc64: numa: check the node id consistently for sparc64

Commit Message

Yunsheng Lin Aug. 31, 2019, 5:58 a.m. UTC

According to Section 6.2.14 from ACPI spec 6.3 [1], the setting
of proximity domain is optional, as below:

This optional object is used to describe proximity domain
associations within a machine. _PXM evaluates to an integer
that identifies a device as belonging to a Proximity Domain
defined in the System Resource Affinity Table (SRAT).

This patch checks node id with the below case before returning
&numa_cpumask_lookup_table[node]:
1. if node_id >= nr_node_ids, return cpu_none_mask
2. if node_id < 0, return cpu_online_mask
3. Since numa_cpumask_lookup_table is not a pointer, a comment
   is added to indicate that

[1] https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf

Signed-off-by: Yunsheng Lin <[e-mail]>
---
 arch/sparc/include/asm/topology_64.h | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

David Miller Aug. 31, 2019, 6:53 a.m. UTC | #1

From: Yunsheng Lin <[e-mail]>
Date: Sat, 31 Aug 2019 13:58:21 +0800

> According to Section 6.2.14 from ACPI spec 6.3 [1], the setting
> of proximity domain is optional, as below:

What in the world does the ACPI spec have to do with sparc64 NUMA
node ID checking?

Yunsheng Lin Aug. 31, 2019, 8:57 a.m. UTC | #2

On 2019/8/31 14:53, David Miller wrote:
> From: Yunsheng Lin <[e-mail]>
> Date: Sat, 31 Aug 2019 13:58:21 +0800
> 
>> According to Section 6.2.14 from ACPI spec 6.3 [1], the setting
>> of proximity domain is optional, as below:
> 
> What in the world does the ACPI spec have to do with sparc64 NUMA
> node ID checking?

I am not sure I understand your question fully here.

Here is my issue when the bios does not implement the proximity domain
of a device because the feature is optional according to the ACPI spec,
the dev_to_node(dev) return -1, which causes out of bound access when
using the value to get the device's cpu mask by calling cpumask_of_node.

Did you mean sparc64 system does not has ACPI, the device's node id will
not specified by ACPI, so the ACPI is unrelated here?

Or did you mean the commit log is not clear enough to justify the change?

Or did you mean this problem should be fixed in somewhere else?

Any detail advice and suggestion will be very helpful, thanks.

> 
> .
>

David Miller Aug. 31, 2019, 8:02 p.m. UTC | #3

From: Yunsheng Lin <[e-mail]>
Date: Sat, 31 Aug 2019 16:57:04 +0800

> Did you mean sparc64 system does not has ACPI, the device's node id will
> not specified by ACPI, so the ACPI is unrelated here?

Yes, sparc64 never has and never will have ACPI.

This is also true for several other platforms where you have made this
change.

The assumption of your entire patch set is that the semantics of the
NUMA node ID are somehow completely defined by ACPI semantics.  Which
is not true.

Yunsheng Lin Sept. 2, 2019, 6:08 a.m. UTC | #4

On 2019/9/1 4:02, David Miller wrote:
> From: Yunsheng Lin <[e-mail]>
> Date: Sat, 31 Aug 2019 16:57:04 +0800
> 
>> Did you mean sparc64 system does not has ACPI, the device's node id will
>> not specified by ACPI, so the ACPI is unrelated here?
> 
> Yes, sparc64 never has and never will have ACPI.
> 
> This is also true for several other platforms where you have made this
> change.
> 
> The assumption of your entire patch set is that the semantics of the
> NUMA node ID are somehow completely defined by ACPI semantics.  Which
> is not true.

Thanks for pointing out.

The NUMA node id in sparc64 system is defined by DT semantics?

> 
> .
>

David Miller Sept. 2, 2019, 3:17 p.m. UTC | #5

From: Yunsheng Lin <[e-mail]>
Date: Mon, 2 Sep 2019 14:08:31 +0800

> The NUMA node id in sparc64 system is defined by DT semantics?

Sometimes, and in other cases other methods are used to determine
the NUMA node id.

Patch

diff --git a/arch/sparc/include/asm/topology_64.h b/arch/sparc/include/asm/topology_64.h
index 34c628a..66a7917 100644
--- a/arch/sparc/include/asm/topology_64.h
+++ b/arch/sparc/include/asm/topology_64.h
@@ -11,9 +11,19 @@  static inline int cpu_to_node(int cpu)
 	return numa_cpu_lookup_table[cpu];
 }
 
-#define cpumask_of_node(node) ((node) == -1 ?				\
-			       cpu_all_mask :				\
-			       &numa_cpumask_lookup_table[node])
+static inline const struct cpumask *cpumask_of_node(int node)
+{
+	if (node >= MAX_NUMNODES)
+		return cpu_none_mask;
+
+	/* numa_cpumask_lookup_table[node] is not a pointer, so
+	 * no need to check for NULL here.
+	 */
+	if (node < 0)
+		return cpu_online_mask;
+
+	return &numa_cpumask_lookup_table[node];
+}
 
 struct pci_bus;
 #ifdef CONFIG_PCI