[PATCH v2 0/6] ACPI: Only create NUMA nodes from entries in SRAT or SRAT emulation.

[PATCH v2 0/6] ACPI: Only create NUMA nodes from entries in SRAT or SRAT emulation.

[Jonathan Cameron]

Here, I will use the term Proximity Domains for the ACPI description and
NUMA Nodes for the in kernel representation.

ACPI 6.3 included a clarification that only Static Resource Allocation
Structures in SRAT may define the existence of proximity domains
(sec 5.2.16). This clarification closed a possible interpretation that
other parts of ACPI (e.g. DSDT _PXM, NFIT etc) could define new proximity
domains that were not also mentioned in SRAT structures.

In practice the kernel has never allowed this alternative interpretation as
such nodes are only partially initialized. This is architecture specific
but to take an example, on x86 alloc_node_data has not been called.
Any use of them for node specific allocation, will result in a crash as the
infrastructure to fallback to a node with memory is not setup.

We ran into a problem when enabling _PXM handling for PCI devices and found
there were boards out there advertising devices in proximity domains that
didn't exist [2].

The fix suggested in this series is to replace instances that should not
'create' new nodes with pxm_to_node.  This function needs a some additional
hardening against invalid inputs to make sure it is safe for use in these
new callers.

Patch 1 Hardens pxm_to_node() against numa_off, and pxm entry being too large.

Patch 2-4 change the various callers not related to SRAT entries so that they
set this parameter to false, so do not attempt to initialize a new NUMA node
if the relevant one does not already exist.

Patch 5 is a function rename to reflect change in functionality of
acpi_map_pxm_to_online_node() as it no longer creates a new map, but just does a
lookup of existing maps.

Patch 6 covers the one place we do not allow the full flexibility defined
in the ACPI spec.  For SRAT GIC Interrupt Translation Service (ITS) Affinity
Structures, on ARM64, the driver currently makes an additional pass of SRAT
later in the boot than the one used to identify NUMA domains.
Note, this currently means that an ITS placed in a proximity domain that is
not defined by another SRAT structure will result in the a crash.

To avoid this crash with minimal changes we do not create new NUMA nodes based
on this particular entry type.  Any current platform trying to do this will not
boot, so this is an improvement, if perhaps not a perfect solution.

[1] Note in ACPI Specification 6.3 5.2.16 System Resource Affinity Table (SRAT)
[2] https://patchwork.kernel.org/patch/10597777/

Thanks to Bjorn Helgaas for review of v1 and Barry Song for internal reviews that
lead to a slightly different approach for this v2.

Changes since v1.
* Use pxm_to_node for what was previously the path using acpi_map_pxm_to_node
  with create==false. (Barry)
* Broke patch up into an initial noop stage followed by patches (Bjorn)
  to update each type of case in which partial creation of NUMA nodes is prevented.
* Added patch 5 to rename function to reflect change of functionality.
* Updated descriptions (now mostly in individual patches) inline with Bjorn's comments.

Jonathan Cameron (6):
  ACPI: Add out of bounds and numa_off protections to pxm_to_node
  ACPI: Do not create new NUMA domains from ACPI static tables that are
    not SRAT
  ACPI: Remove side effect of partly creating a node in
    acpi_map_pxm_to_online_node
  ACPI: rename acpi_map_pxm_to_online_node to pxm_to_online_node
  ACPI: Remove side effect of partly creating a node in acpi_get_node
  irq-chip/gic-v3-its: Fix crash if ITS is in a proximity domain without
    processor or memory

 drivers/acpi/arm64/iort.c        |  2 +-
 drivers/acpi/nfit/core.c         |  6 ++----
 drivers/acpi/numa/hmat.c         |  4 ++--
 drivers/acpi/numa/srat.c         |  4 ++--
 drivers/iommu/intel/dmar.c       |  2 +-
 drivers/irqchip/irq-gic-v3-its.c |  7 ++++++-
 include/linux/acpi.h             | 15 +++++++--------
 7 files changed, 21 insertions(+), 19 deletions(-)

-- 
2.19.1

[PATCH v2 1/6] ACPI: Add out of bounds and numa_off protections to pxm_to_node

[Jonathan Cameron]

The function should check the validity of the pxm value before using
it to index the pxm_to_node_map array.

Whilst hardening this code may be good in general, the main intent
here is to enable following patches that use this function to replace
acpi_map_pxm_to_node for non SRAT usecases which should return
NO_NUMA_NODE for PXM entries not matching with those in SRAT.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/acpi/numa/srat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index 5be5a977da1b..8ef44ee0d76b 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -31,7 +31,7 @@ int acpi_numa __initdata;
 
 int pxm_to_node(int pxm)
 {
-	if (pxm < 0)
+	if (pxm < 0 || pxm >= MAX_PXM_DOMAINS || numa_off)
 		return NUMA_NO_NODE;
 	return pxm_to_node_map[pxm];
 }
-- 
2.19.1

[PATCH v2 2/6] ACPI: Do not create new NUMA domains from ACPI static tables that are not SRAT

[Jonathan Cameron]

Several ACPI static tables contain references to proximity domains.
ACPI 6.3 has clarified that only entries in SRAT may define a new domain
(sec 5.2.16).

Those tables described in the ACPI spec have additional clarifying text.

NFIT: Table 5-132,

"Integer that represents the proximity domain to which the memory belongs.
 This number must match with corresponding entry in the SRAT table."

HMAT: Table 5-145,

"... This number must match with the corresponding entry in the SRAT
 table's processor affinity structure ... if the initiator is a processor,
 or the Generic Initiator Affinity Structure if the initiator is a generic
 initiator".

IORT and DMAR are defined by external specifications.

Intel Virtualization Technology for Directed I/O Rev 3.1 does not make any
explicit statements, but the general SRAT statement above will still apply.
https://software.intel.com/sites/default/files/managed/c5/15/vt-directed-io-spec.pdf

IO Remapping Table, Platform Design Document rev D, also makes not explicit
statement, but refers to ACPI SRAT table for more information and again the
generic SRAT statement above applies.
https://developer.arm.com/documentation/den0049/d/

In conclusion, any proximity domain specified in these tables, should be a
reference to a proximity domain also found in SRAT, and they should not be able
to instantiate a new domain.  Hence we switch to pxm_to_node() which will only
return existing nodes.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/acpi/arm64/iort.c  | 2 +-
 drivers/acpi/nfit/core.c   | 3 +--
 drivers/acpi/numa/hmat.c   | 2 +-
 drivers/iommu/intel/dmar.c | 2 +-
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 28a6b387e80e..eb0f158612c8 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1293,7 +1293,7 @@ static int  __init arm_smmu_v3_set_proximity(struct device *dev,
 
 	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
 	if (smmu->flags & ACPI_IORT_SMMU_V3_PXM_VALID) {
-		int dev_node = acpi_map_pxm_to_node(smmu->pxm);
+		int dev_node = pxm_to_node(smmu->pxm);
 
 		if (dev_node != NUMA_NO_NODE && !node_online(dev_node))
 			return -EINVAL;
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 7c138a4edc03..d933a4636d00 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -2947,8 +2947,7 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 	if (spa->flags & ACPI_NFIT_PROXIMITY_VALID) {
 		ndr_desc->numa_node = acpi_map_pxm_to_online_node(
 						spa->proximity_domain);
-		ndr_desc->target_node = acpi_map_pxm_to_node(
-				spa->proximity_domain);
+		ndr_desc->target_node = pxm_to_node(spa->proximity_domain);
 	} else {
 		ndr_desc->numa_node = NUMA_NO_NODE;
 		ndr_desc->target_node = NUMA_NO_NODE;
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index 2c32cfb72370..cf6df2df26cd 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -666,7 +666,7 @@ static void hmat_register_target_device(struct memory_target *target,
 
 	pdev->dev.numa_node = acpi_map_pxm_to_online_node(target->memory_pxm);
 	info = (struct memregion_info) {
-		.target_node = acpi_map_pxm_to_node(target->memory_pxm),
+		.target_node = pxm_to_node(target->memory_pxm),
 	};
 	rc = platform_device_add_data(pdev, &info, sizeof(info));
 	if (rc < 0) {
diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 683b812c5c47..b1acdaead059 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -473,7 +473,7 @@ static int dmar_parse_one_rhsa(struct acpi_dmar_header *header, void *arg)
 	rhsa = (struct acpi_dmar_rhsa *)header;
 	for_each_drhd_unit(drhd) {
 		if (drhd->reg_base_addr == rhsa->base_address) {
-			int node = acpi_map_pxm_to_node(rhsa->proximity_domain);
+			int node = pxm_to_node(rhsa->proximity_domain);
 
 			if (!node_online(node))
 				node = NUMA_NO_NODE;
-- 
2.19.1

[PATCH v2 3/6] ACPI: Remove side effect of partly creating a node in acpi_map_pxm_to_online_node

[Jonathan Cameron]

Whilst this function will only return an online node, it can have the side
effect of partially creating a new node.  The existing comments suggest this
is intentional, but the usecases of this function are related to NFIT and
HMAT parsing, neither of which should be able to define new nodes.

One route by which the existing behaviour would cause a crash is to have a
_PXM entry in ACPI DSDT attempt to place a device within this partly
created proximity domain. A subsequent call to devm_kzalloc or similar
would result in an attempt to allocate memory on a node for which zone
lists have not been set up and a null pointer dereference.

We prevent such cases by switching to pxm_to_node() within
acpi_map_pxm_to_online_node which cannot cause a new node to be partly
created. If one would previously have been created we now return
NO_NUMA_NODE.  Documentation updated to reflect this change.
We may want to think about renaming acpi_map_pxm_to_online_node to
pxm_to_online_node to reflect this change.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/linux/acpi.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d661cd0ee64d..b541a0b444fd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -430,13 +430,12 @@ int acpi_get_node(acpi_handle handle);
  * ACPI device drivers, which are called after the NUMA initialization has
  * completed in the kernel, can call this interface to obtain their device
  * NUMA topology from ACPI tables.  Such drivers do not have to deal with
- * offline nodes.  A node may be offline when a device proximity ID is
- * unique, SRAT memory entry does not exist, or NUMA is disabled, ex.
- * "numa=off" on x86.
+ * offline nodes.  A node may be offline when SRAT memory entry does not exist,
+ * or NUMA is disabled, ex. "numa=off" on x86.
  */
 static inline int acpi_map_pxm_to_online_node(int pxm)
 {
-	int node = acpi_map_pxm_to_node(pxm);
+	int node = pxm_to_node(pxm);
 
 	return numa_map_to_online_node(node);
 }
-- 
2.19.1

[PATCH v2 4/6] ACPI: Rename acpi_map_pxm_to_online_node to pxm_to_online_node

[Jonathan Cameron]

As this function is no longer allowed to create new mappings
let us rename it to reflect this.

Note all nodes should already exist before any of the users
of this function are called.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/acpi/nfit/core.c | 3 +--
 drivers/acpi/numa/hmat.c | 2 +-
 include/linux/acpi.h     | 8 ++++----
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index d933a4636d00..d4ba2eb21ce0 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -2945,8 +2945,7 @@ static int acpi_nfit_register_region(struct acpi_nfit_desc *acpi_desc,
 	ndr_desc->provider_data = nfit_spa;
 	ndr_desc->attr_groups = acpi_nfit_region_attribute_groups;
 	if (spa->flags & ACPI_NFIT_PROXIMITY_VALID) {
-		ndr_desc->numa_node = acpi_map_pxm_to_online_node(
-						spa->proximity_domain);
+		ndr_desc->numa_node = pxm_to_online_node(spa->proximity_domain);
 		ndr_desc->target_node = pxm_to_node(spa->proximity_domain);
 	} else {
 		ndr_desc->numa_node = NUMA_NO_NODE;
diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
index cf6df2df26cd..e7add2609c03 100644
--- a/drivers/acpi/numa/hmat.c
+++ b/drivers/acpi/numa/hmat.c
@@ -664,7 +664,7 @@ static void hmat_register_target_device(struct memory_target *target,
 		goto out_pdev;
 	}
 
-	pdev->dev.numa_node = acpi_map_pxm_to_online_node(target->memory_pxm);
+	pdev->dev.numa_node = pxm_to_online_node(target->memory_pxm);
 	info = (struct memregion_info) {
 		.target_node = pxm_to_node(target->memory_pxm),
 	};
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index b541a0b444fd..a56386fd98a8 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -420,10 +420,10 @@ int acpi_map_pxm_to_node(int pxm);
 int acpi_get_node(acpi_handle handle);
 
 /**
- * acpi_map_pxm_to_online_node - Map proximity ID to online node
+ * pxm_to_online_node - Map proximity ID to online node
  * @pxm: ACPI proximity ID
  *
- * This is similar to acpi_map_pxm_to_node(), but always returns an online
+ * This is similar to pxm_to_node(), but always returns an online
  * node.  When the mapped node from a given proximity ID is offline, it
  * looks up the node distance table and returns the nearest online node.
  *
@@ -433,14 +433,14 @@ int acpi_get_node(acpi_handle handle);
  * offline nodes.  A node may be offline when SRAT memory entry does not exist,
  * or NUMA is disabled, ex. "numa=off" on x86.
  */
-static inline int acpi_map_pxm_to_online_node(int pxm)
+static inline int pxm_to_online_node(int pxm)
 {
 	int node = pxm_to_node(pxm);
 
 	return numa_map_to_online_node(node);
 }
 #else
-static inline int acpi_map_pxm_to_online_node(int pxm)
+static inline int pxm_to_online_node(int pxm)
 {
 	return 0;
 }
-- 
2.19.1

[PATCH v2 5/6] ACPI: Remove side effect of partly creating a node in acpi_get_node

[Jonathan Cameron]

acpi_get_node calls acpi_get_pxm to evaluate the _PXM AML method for
entries found in DSDT/SSDT. ACPI 6.3 sec 6.2.14 states
"_PXM evaluates to an integer that identifies a device as belonging to
 a Proximity Domain defined in the System Resource Affinity Table (SRAT)."

Hence a _PXM method should not result in creation of a new NUMA node.
Before this patch, _PXM could result in partial instantiation of
NUMA node, missing elements such as zone lists.  A call to devm_kzalloc
for example results in a null pointer dereference.

This patch therefore replaces the acpi_map_pxm_to_node with a call
to pxm_to_node.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/acpi/numa/srat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index 8ef44ee0d76b..697a5c9e2eb5 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -444,6 +444,6 @@ int acpi_get_node(acpi_handle handle)
 
 	pxm = acpi_get_pxm(handle);
 
-	return acpi_map_pxm_to_node(pxm);
+	return pxm_to_node(pxm);
 }
 EXPORT_SYMBOL(acpi_get_node);
-- 
2.19.1

[PATCH v2 6/6] irq-chip/gic-v3-its: Fix crash if ITS is in a proximity domain without processor or memory

[Jonathan Cameron]

Note this crash is present before any of the patches in this series, but
as explained below it is highly unlikely anyone is shipping a firmware that
causes it. Tests were done using an overriden SRAT.

On ARM64, the gic-v3 driver directly parses SRAT to locate GIC Interrupt
Translation Service (ITS) Affinity Structures. This is done much later
in the boot than the parses of SRAT which identify proximity domains.

As a result, an ITS placed in a proximity domain that is not defined by
another SRAT structure will result in a NUMA node that is not completely
configured and a crash.

ITS [mem 0x202100000-0x20211ffff]
ITS@0x0000000202100000: Using ITS number 0
Unable to handle kernel paging request at virtual address 0000000000001a08
...

Call trace:
  __alloc_pages_nodemask+0xe8/0x338
  alloc_pages_node.constprop.0+0x34/0x40
  its_probe_one+0x2f8/0xb18
  gic_acpi_parse_madt_its+0x108/0x150
  acpi_table_parse_entries_array+0x17c/0x264
  acpi_table_parse_entries+0x48/0x6c
  acpi_table_parse_madt+0x30/0x3c
  its_init+0x1c4/0x644
  gic_init_bases+0x4b8/0x4ec
  gic_acpi_init+0x134/0x264
  acpi_match_madt+0x4c/0x84
  acpi_table_parse_entries_array+0x17c/0x264
  acpi_table_parse_entries+0x48/0x6c
  acpi_table_parse_madt+0x30/0x3c
  __acpi_probe_device_table+0x8c/0xe8
  irqchip_init+0x3c/0x48
  init_IRQ+0xcc/0x100
  start_kernel+0x33c/0x548

ACPI 6.3 allows any set of Affinity Structures in SRAT to define a proximity
domain.  However, as we do not see this crash, we can conclude that no
firmware is currently placing an ITS in a node that is separate from
those containing memory and / or processors.

We could modify the SRAT parsing behavior to identify the existence
of Proximity Domains unique to the ITS structures, and handle them as
a special case of a generic initiator (once support for those merges).

This patch avoids the complexity that would be needed to handle this corner
case, by not allowing the ITS entry parsing code to instantiate new NUMA
Nodes.  If one is encountered that does not already exist, then NO_NUMA_NODE
is assigned and a warning printed just as if the value had been greater than
allowed NUMA Nodes.

"SRAT: Invalid NUMA node -1 in ITS affinity"

Whilst this does not provide the full flexibility allowed by ACPI,
it does fix the problem.  We can revisit a more sophisticated solution if
needed by future platforms.

Change is simply to replace acpi_map_pxm_to_node with pxm_to_node reflecting
the fact a new mapping is not created.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 6a5a87fc4601..c26862a074da 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -5248,7 +5248,12 @@ static int __init gic_acpi_parse_srat_its(union acpi_subtable_headers *header,
 		return -EINVAL;
 	}
 
-	node = acpi_map_pxm_to_node(its_affinity->proximity_domain);
+	/*
+	 * Note that in theory a new proximity node could be created by this
+	 * entry as it is an SRAT resource allocation structure.
+	 * We do not currently support doing so.
+	 */
+	node = pxm_to_node(its_affinity->proximity_domain);
 
 	if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
 		pr_err("SRAT: Invalid NUMA node %d in ITS affinity\n", node);
-- 
2.19.1

RE: [PATCH v2 1/6] ACPI: Add out of bounds and numa_off protections to pxm_to_node

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron
> Sent: Saturday, July 18, 2020 6:00 AM
> To: linux-mm@kvack.org; linux-acpi@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; x86@kernel.org
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Bjorn Helgaas
> <bhelgaas@google.com>; linux-pci@vger.kernel.org; martin@geanix.com; Ingo
> Molnar <mingo@redhat.com>; linux-ia64@vger.kernel.org; Tony Luck
> <tony.luck@intel.com>; Fenghua Yu <fenghua.yu@intel.com>; Thomas
> Gleixner <tglx@linutronix.de>; Linuxarm <linuxarm@huawei.com>; Dan
> Williams <dan.j.williams@intel.com>; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>
> Subject: [PATCH v2 1/6] ACPI: Add out of bounds and numa_off protections to
> pxm_to_node
> 
> The function should check the validity of the pxm value before using
> it to index the pxm_to_node_map array.
> 
> Whilst hardening this code may be good in general, the main intent
> here is to enable following patches that use this function to replace
> acpi_map_pxm_to_node for non SRAT usecases which should return
> NO_NUMA_NODE for PXM entries not matching with those in SRAT.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

> ---
>  drivers/acpi/numa/srat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> index 5be5a977da1b..8ef44ee0d76b 100644
> --- a/drivers/acpi/numa/srat.c
> +++ b/drivers/acpi/numa/srat.c
> @@ -31,7 +31,7 @@ int acpi_numa __initdata;
> 
>  int pxm_to_node(int pxm)
>  {
> -	if (pxm < 0)
> +	if (pxm < 0 || pxm >= MAX_PXM_DOMAINS || numa_off)
>  		return NUMA_NO_NODE;
>  	return pxm_to_node_map[pxm];
>  }
> --
> 2.19.1

RE: [PATCH v2 2/6] ACPI: Do not create new NUMA domains from ACPI static tables that are not SRAT

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron
> Sent: Saturday, July 18, 2020 6:00 AM
> To: linux-mm@kvack.org; linux-acpi@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; x86@kernel.org
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Bjorn Helgaas
> <bhelgaas@google.com>; linux-pci@vger.kernel.org; martin@geanix.com; Ingo
> Molnar <mingo@redhat.com>; linux-ia64@vger.kernel.org; Tony Luck
> <tony.luck@intel.com>; Fenghua Yu <fenghua.yu@intel.com>; Thomas
> Gleixner <tglx@linutronix.de>; Linuxarm <linuxarm@huawei.com>; Dan
> Williams <dan.j.williams@intel.com>; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>
> Subject: [PATCH v2 2/6] ACPI: Do not create new NUMA domains from ACPI
> static tables that are not SRAT
> 
> Several ACPI static tables contain references to proximity domains.
> ACPI 6.3 has clarified that only entries in SRAT may define a new domain
> (sec 5.2.16).
> 
> Those tables described in the ACPI spec have additional clarifying text.
> 
> NFIT: Table 5-132,
> 
> "Integer that represents the proximity domain to which the memory belongs.
>  This number must match with corresponding entry in the SRAT table."
> 
> HMAT: Table 5-145,
> 
> "... This number must match with the corresponding entry in the SRAT
>  table's processor affinity structure ... if the initiator is a processor,
>  or the Generic Initiator Affinity Structure if the initiator is a generic
>  initiator".
> 
> IORT and DMAR are defined by external specifications.
> 
> Intel Virtualization Technology for Directed I/O Rev 3.1 does not make any
> explicit statements, but the general SRAT statement above will still apply.
> https://software.intel.com/sites/default/files/managed/c5/15/vt-directed-io-s
> pec.pdf
> 
> IO Remapping Table, Platform Design Document rev D, also makes not explicit
> statement, but refers to ACPI SRAT table for more information and again the
> generic SRAT statement above applies.
> https://developer.arm.com/documentation/den0049/d/
> 
> In conclusion, any proximity domain specified in these tables, should be a
> reference to a proximity domain also found in SRAT, and they should not be
> able
> to instantiate a new domain.  Hence we switch to pxm_to_node() which will
> only
> return existing nodes.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Much better than v1 which was using a bool parameter "false" to stop acpi_map_pxm_to_node()
from creating node.

And pxm_to_node() has been used by some other places as an API which is
serving the transition from pxm to node.

Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

> ---
>  drivers/acpi/arm64/iort.c  | 2 +-
>  drivers/acpi/nfit/core.c   | 3 +--
>  drivers/acpi/numa/hmat.c   | 2 +-
>  drivers/iommu/intel/dmar.c | 2 +-
>  4 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 28a6b387e80e..eb0f158612c8 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1293,7 +1293,7 @@ static int  __init
> arm_smmu_v3_set_proximity(struct device *dev,
> 
>  	smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>  	if (smmu->flags & ACPI_IORT_SMMU_V3_PXM_VALID) {
> -		int dev_node = acpi_map_pxm_to_node(smmu->pxm);
> +		int dev_node = pxm_to_node(smmu->pxm);
> 
>  		if (dev_node != NUMA_NO_NODE && !node_online(dev_node))
>  			return -EINVAL;
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 7c138a4edc03..d933a4636d00 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -2947,8 +2947,7 @@ static int acpi_nfit_register_region(struct
> acpi_nfit_desc *acpi_desc,
>  	if (spa->flags & ACPI_NFIT_PROXIMITY_VALID) {
>  		ndr_desc->numa_node = acpi_map_pxm_to_online_node(
>  						spa->proximity_domain);
> -		ndr_desc->target_node = acpi_map_pxm_to_node(
> -				spa->proximity_domain);
> +		ndr_desc->target_node = pxm_to_node(spa->proximity_domain);
>  	} else {
>  		ndr_desc->numa_node = NUMA_NO_NODE;
>  		ndr_desc->target_node = NUMA_NO_NODE;
> diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c
> index 2c32cfb72370..cf6df2df26cd 100644
> --- a/drivers/acpi/numa/hmat.c
> +++ b/drivers/acpi/numa/hmat.c
> @@ -666,7 +666,7 @@ static void hmat_register_target_device(struct
> memory_target *target,
> 
>  	pdev->dev.numa_node =
> acpi_map_pxm_to_online_node(target->memory_pxm);
>  	info = (struct memregion_info) {
> -		.target_node = acpi_map_pxm_to_node(target->memory_pxm),
> +		.target_node = pxm_to_node(target->memory_pxm),
>  	};
>  	rc = platform_device_add_data(pdev, &info, sizeof(info));
>  	if (rc < 0) {
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index 683b812c5c47..b1acdaead059 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -473,7 +473,7 @@ static int dmar_parse_one_rhsa(struct
> acpi_dmar_header *header, void *arg)
>  	rhsa = (struct acpi_dmar_rhsa *)header;
>  	for_each_drhd_unit(drhd) {
>  		if (drhd->reg_base_addr == rhsa->base_address) {
> -			int node = acpi_map_pxm_to_node(rhsa->proximity_domain);
> +			int node = pxm_to_node(rhsa->proximity_domain);
> 
>  			if (!node_online(node))
>  				node = NUMA_NO_NODE;
> --
> 2.19.1

Thanks
Barry

RE: [PATCH v2 5/6] ACPI: Remove side effect of partly creating a node in acpi_get_node

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Jonathan Cameron
> Sent: Saturday, July 18, 2020 6:00 AM
> To: linux-mm@kvack.org; linux-acpi@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; x86@kernel.org
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Bjorn Helgaas
> <bhelgaas@google.com>; linux-pci@vger.kernel.org; martin@geanix.com; Ingo
> Molnar <mingo@redhat.com>; linux-ia64@vger.kernel.org; Tony Luck
> <tony.luck@intel.com>; Fenghua Yu <fenghua.yu@intel.com>; Thomas
> Gleixner <tglx@linutronix.de>; Linuxarm <linuxarm@huawei.com>; Dan
> Williams <dan.j.williams@intel.com>; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>
> Subject: [PATCH v2 5/6] ACPI: Remove side effect of partly creating a node in
> acpi_get_node
> 
> acpi_get_node calls acpi_get_pxm to evaluate the _PXM AML method for
> entries found in DSDT/SSDT. ACPI 6.3 sec 6.2.14 states
> "_PXM evaluates to an integer that identifies a device as belonging to
>  a Proximity Domain defined in the System Resource Affinity Table (SRAT)."
> 
> Hence a _PXM method should not result in creation of a new NUMA node.
> Before this patch, _PXM could result in partial instantiation of
> NUMA node, missing elements such as zone lists.  A call to devm_kzalloc
> for example results in a null pointer dereference.
> 
> This patch therefore replaces the acpi_map_pxm_to_node with a call
> to pxm_to_node.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Reviewed-by: Barry Song <song.bao.hua@hisilicon.com>

> ---
>  drivers/acpi/numa/srat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> index 8ef44ee0d76b..697a5c9e2eb5 100644
> --- a/drivers/acpi/numa/srat.c
> +++ b/drivers/acpi/numa/srat.c
> @@ -444,6 +444,6 @@ int acpi_get_node(acpi_handle handle)
> 
>  	pxm = acpi_get_pxm(handle);
> 
> -	return acpi_map_pxm_to_node(pxm);
> +	return pxm_to_node(pxm);
>  }
>  EXPORT_SYMBOL(acpi_get_node);
> --
> 2.19.1

Re: [PATCH v2 0/6] ACPI: Only create NUMA nodes from entries in SRAT or SRAT emulation.

[Hanjun Guo]

On 2020/7/18 1:59, Jonathan Cameron wrote:
> Here, I will use the term Proximity Domains for the ACPI description and
> NUMA Nodes for the in kernel representation.
> 
> ACPI 6.3 included a clarification that only Static Resource Allocation
> Structures in SRAT may define the existence of proximity domains
> (sec 5.2.16). This clarification closed a possible interpretation that
> other parts of ACPI (e.g. DSDT _PXM, NFIT etc) could define new proximity
> domains that were not also mentioned in SRAT structures.
> 
> In practice the kernel has never allowed this alternative interpretation as
> such nodes are only partially initialized. This is architecture specific
> but to take an example, on x86 alloc_node_data has not been called.
> Any use of them for node specific allocation, will result in a crash as the
> infrastructure to fallback to a node with memory is not setup.
> 
> We ran into a problem when enabling _PXM handling for PCI devices and found
> there were boards out there advertising devices in proximity domains that
> didn't exist [2].
> 
> The fix suggested in this series is to replace instances that should not
> 'create' new nodes with pxm_to_node.  This function needs a some additional
> hardening against invalid inputs to make sure it is safe for use in these
> new callers.
> 
> Patch 1 Hardens pxm_to_node() against numa_off, and pxm entry being too large.
> 
> Patch 2-4 change the various callers not related to SRAT entries so that they
> set this parameter to false, so do not attempt to initialize a new NUMA node
> if the relevant one does not already exist.
> 
> Patch 5 is a function rename to reflect change in functionality of
> acpi_map_pxm_to_online_node() as it no longer creates a new map, but just does a
> lookup of existing maps.
> 
> Patch 6 covers the one place we do not allow the full flexibility defined
> in the ACPI spec.  For SRAT GIC Interrupt Translation Service (ITS) Affinity
> Structures, on ARM64, the driver currently makes an additional pass of SRAT
> later in the boot than the one used to identify NUMA domains.
> Note, this currently means that an ITS placed in a proximity domain that is
> not defined by another SRAT structure will result in the a crash.
> 
> To avoid this crash with minimal changes we do not create new NUMA nodes based
> on this particular entry type.  Any current platform trying to do this will not
> boot, so this is an improvement, if perhaps not a perfect solution.

Make sense to me,

Reviewed-by: Hanjun Guo <guohanjun@huawei.com>

Re: [PATCH v2 0/6] ACPI: Only create NUMA nodes from entries in SRAT or SRAT emulation.

[Jonathan Cameron]

On Sat, 18 Jul 2020 01:59:53 +0800
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> Here, I will use the term Proximity Domains for the ACPI description and
> NUMA Nodes for the in kernel representation.
> 
> ACPI 6.3 included a clarification that only Static Resource Allocation
> Structures in SRAT may define the existence of proximity domains
> (sec 5.2.16). This clarification closed a possible interpretation that
> other parts of ACPI (e.g. DSDT _PXM, NFIT etc) could define new proximity
> domains that were not also mentioned in SRAT structures.
> 
> In practice the kernel has never allowed this alternative interpretation as
> such nodes are only partially initialized. This is architecture specific
> but to take an example, on x86 alloc_node_data has not been called.
> Any use of them for node specific allocation, will result in a crash as the
> infrastructure to fallback to a node with memory is not setup.
> 
> We ran into a problem when enabling _PXM handling for PCI devices and found
> there were boards out there advertising devices in proximity domains that
> didn't exist [2].
> 
> The fix suggested in this series is to replace instances that should not
> 'create' new nodes with pxm_to_node.  This function needs a some additional
> hardening against invalid inputs to make sure it is safe for use in these
> new callers.
> 
> Patch 1 Hardens pxm_to_node() against numa_off, and pxm entry being too large.
> 
> Patch 2-4 change the various callers not related to SRAT entries so that they
> set this parameter to false, so do not attempt to initialize a new NUMA node
> if the relevant one does not already exist.
> 
> Patch 5 is a function rename to reflect change in functionality of
> acpi_map_pxm_to_online_node() as it no longer creates a new map, but just does a
> lookup of existing maps.
> 
> Patch 6 covers the one place we do not allow the full flexibility defined
> in the ACPI spec.  For SRAT GIC Interrupt Translation Service (ITS) Affinity
> Structures, on ARM64, the driver currently makes an additional pass of SRAT
> later in the boot than the one used to identify NUMA domains.
> Note, this currently means that an ITS placed in a proximity domain that is
> not defined by another SRAT structure will result in the a crash.
> 
> To avoid this crash with minimal changes we do not create new NUMA nodes based
> on this particular entry type.  Any current platform trying to do this will not
> boot, so this is an improvement, if perhaps not a perfect solution.
> 
> [1] Note in ACPI Specification 6.3 5.2.16 System Resource Affinity Table (SRAT)
> [2] https://patchwork.kernel.org/patch/10597777/
> 
> Thanks to Bjorn Helgaas for review of v1 and Barry Song for internal reviews that
> lead to a slightly different approach for this v2.

Thanks Barry / Hanjun,

Anyone else have time to take a look?

I'm happy to bring it back after the merge window closes, but would like to know
if people are happy with the general approach.  I'm keen to finally be able to
resolve that issue with _PXM and PCI. 

Thanks,

Jonathan

> 
> Changes since v1.
> * Use pxm_to_node for what was previously the path using acpi_map_pxm_to_node
>   with create==false. (Barry)
> * Broke patch up into an initial noop stage followed by patches (Bjorn)
>   to update each type of case in which partial creation of NUMA nodes is prevented.
> * Added patch 5 to rename function to reflect change of functionality.
> * Updated descriptions (now mostly in individual patches) inline with Bjorn's comments.
> 
> Jonathan Cameron (6):
>   ACPI: Add out of bounds and numa_off protections to pxm_to_node
>   ACPI: Do not create new NUMA domains from ACPI static tables that are
>     not SRAT
>   ACPI: Remove side effect of partly creating a node in
>     acpi_map_pxm_to_online_node
>   ACPI: rename acpi_map_pxm_to_online_node to pxm_to_online_node
>   ACPI: Remove side effect of partly creating a node in acpi_get_node
>   irq-chip/gic-v3-its: Fix crash if ITS is in a proximity domain without
>     processor or memory
> 
>  drivers/acpi/arm64/iort.c        |  2 +-
>  drivers/acpi/nfit/core.c         |  6 ++----
>  drivers/acpi/numa/hmat.c         |  4 ++--
>  drivers/acpi/numa/srat.c         |  4 ++--
>  drivers/iommu/intel/dmar.c       |  2 +-
>  drivers/irqchip/irq-gic-v3-its.c |  7 ++++++-
>  include/linux/acpi.h             | 15 +++++++--------
>  7 files changed, 21 insertions(+), 19 deletions(-)
>