[PATCH] x86: Fix an issue with invalid ACPI numa config

[PATCH] x86: Fix an issue with invalid ACPI numa config

[Jonathan Cameron]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="UTF-8", Size: 2012 bytes --]

The addition of support to read the numa node for a PCI
card specified by _PXM resulted in Martin's system not
booting.   Looking at the ACPI tables it seems that there
are PXM entries for the root ports, but no SRAT table.

The absence of SRAT table results in dummy_numa_init being
called.  However, unlike on arm64, this doesn't then result
in numa_off being set.  When the PCI code later comes along
and calls acpi_get_node for any PCI card below the root port,
it navigates up the ACPI tree until it finds the PXM value in
the root port. This value is then passed to
acpi_map_pxm_to_node.  If numa_off is set this returns,
NUMA_NO_NODE (as it does on arm64), on x86 it instead tries
to allocate a numa node from the unused set without setting
up all the infrastructure that would normally accompany such
a call.  We have not identified exactly which driver is
causing the subsequent hang for Martin.

It is invalid under the ACPI spec to specify new
numa nodes using PXM if they have no presence in SRAT.
Thus the simplest fix is to set numa_off when it is off due
to an invalid SRAT (here not present at all).

I do not have easy access to appropriate x86 numa systems so
would appreciate some testing of this one!

Known problem boards setups:

AMD Ryzen Threadripper 2950X on ASROCK X399 TAICHI
MSI X399 SLI PLUS (probably - not confirmed yet)

The PCI patch has been reverted, so this fix is not critical.

Reported-by: Martin Hundebøll <martin@geanix.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Fixes: bad7dcd94f39 ("ACPI/PCI: Pay attention to device-specific _PXM node values")

---
 arch/x86/mm/numa.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
index 1308f5408bf7..ce1182f953ff 100644
--- a/arch/x86/mm/numa.c
+++ b/arch/x86/mm/numa.c
@@ -695,6 +695,8 @@ static int __init dummy_numa_init(void)
 	node_set(0, numa_nodes_parsed);
 	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
 
+	numa_off = true;
+
 	return 0;
 }
 
-- 
2.18.0

Re: [PATCH] x86: Fix an issue with invalid ACPI numa config

[Jonathan Cameron]

On Thu, 15 Nov 2018 11:06:17 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> The addition of support to read the numa node for a PCI
> card specified by _PXM resulted in Martin's system not
> booting.   Looking at the ACPI tables it seems that there
> are PXM entries for the root ports, but no SRAT table.
> 
> The absence of SRAT table results in dummy_numa_init being
> called.  However, unlike on arm64, this doesn't then result
> in numa_off being set.  When the PCI code later comes along
> and calls acpi_get_node for any PCI card below the root port,
> it navigates up the ACPI tree until it finds the PXM value in
> the root port. This value is then passed to
> acpi_map_pxm_to_node.  If numa_off is set this returns,
> NUMA_NO_NODE (as it does on arm64), on x86 it instead tries
> to allocate a numa node from the unused set without setting
> up all the infrastructure that would normally accompany such
> a call.  We have not identified exactly which driver is
> causing the subsequent hang for Martin.
> 
> It is invalid under the ACPI spec to specify new
> numa nodes using PXM if they have no presence in SRAT.
> Thus the simplest fix is to set numa_off when it is off due
> to an invalid SRAT (here not present at all).
> 
> I do not have easy access to appropriate x86 numa systems so
> would appreciate some testing of this one!
> 
> Known problem boards setups:
> 
> AMD Ryzen Threadripper 2950X on ASROCK X399 TAICHI
> MSI X399 SLI PLUS (probably - not confirmed yet)
> 
> The PCI patch has been reverted, so this fix is not critical.
> 
> Reported-by: Martin Hundeb�ll <martin@geanix.com>
Sorry, seems I had a wrong encoding somewhere down the line and it messed
up Martin's name.  Could we fix that if applying!

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Fixes: bad7dcd94f39 ("ACPI/PCI: Pay attention to device-specific _PXM node values")
> 
> ---
>  arch/x86/mm/numa.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index 1308f5408bf7..ce1182f953ff 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -695,6 +695,8 @@ static int __init dummy_numa_init(void)
>  	node_set(0, numa_nodes_parsed);
>  	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
>  
> +	numa_off = true;
> +
>  	return 0;
>  }
>  

Re: [PATCH] x86: Fix an issue with invalid ACPI numa config

[Peter Zijlstra]

On Thu, Nov 15, 2018 at 11:06:17AM +0000, Jonathan Cameron wrote:
> The addition of support to read the numa node for a PCI
> card specified by _PXM resulted in Martin's system not
> booting.   Looking at the ACPI tables it seems that there
> are PXM entries for the root ports, but no SRAT table.
> 
> The absence of SRAT table results in dummy_numa_init being
> called.  However, unlike on arm64, this doesn't then result
> in numa_off being set.  When the PCI code later comes along
> and calls acpi_get_node for any PCI card below the root port,
> it navigates up the ACPI tree until it finds the PXM value in
> the root port. This value is then passed to
> acpi_map_pxm_to_node.  If numa_off is set this returns,
> NUMA_NO_NODE (as it does on arm64), on x86 it instead tries
> to allocate a numa node from the unused set without setting
> up all the infrastructure that would normally accompany such
> a call.  We have not identified exactly which driver is
> causing the subsequent hang for Martin.
> 
> It is invalid under the ACPI spec to specify new
> numa nodes using PXM if they have no presence in SRAT.
> Thus the simplest fix is to set numa_off when it is off due
> to an invalid SRAT (here not present at all).
> 
> I do not have easy access to appropriate x86 numa systems so
> would appreciate some testing of this one!
> 
> Known problem boards setups:
> 
> AMD Ryzen Threadripper 2950X on ASROCK X399 TAICHI
> MSI X399 SLI PLUS (probably - not confirmed yet)
> 
> The PCI patch has been reverted, so this fix is not critical.
> 
> Reported-by: Martin Hundeb?ll <martin@geanix.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Fixes: bad7dcd94f39 ("ACPI/PCI: Pay attention to device-specific _PXM node values")
> 
> ---
>  arch/x86/mm/numa.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index 1308f5408bf7..ce1182f953ff 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -695,6 +695,8 @@ static int __init dummy_numa_init(void)
>  	node_set(0, numa_nodes_parsed);
>  	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
>  
> +	numa_off = true;

Should we not:

	pr_err(FW_BUG "Invalid SRAT table.\n");

or something along those lines?

We should take every possibility to call out broken and non-compliant
firmware.

Re: [PATCH] x86: Fix an issue with invalid ACPI numa config

[Jonathan Cameron]

On Tue, 20 Nov 2018 13:01:05 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Nov 15, 2018 at 11:06:17AM +0000, Jonathan Cameron wrote:
> > The addition of support to read the numa node for a PCI
> > card specified by _PXM resulted in Martin's system not
> > booting.   Looking at the ACPI tables it seems that there
> > are PXM entries for the root ports, but no SRAT table.
> > 
> > The absence of SRAT table results in dummy_numa_init being
> > called.  However, unlike on arm64, this doesn't then result
> > in numa_off being set.  When the PCI code later comes along
> > and calls acpi_get_node for any PCI card below the root port,
> > it navigates up the ACPI tree until it finds the PXM value in
> > the root port. This value is then passed to
> > acpi_map_pxm_to_node.  If numa_off is set this returns,
> > NUMA_NO_NODE (as it does on arm64), on x86 it instead tries
> > to allocate a numa node from the unused set without setting
> > up all the infrastructure that would normally accompany such
> > a call.  We have not identified exactly which driver is
> > causing the subsequent hang for Martin.
> > 
> > It is invalid under the ACPI spec to specify new
> > numa nodes using PXM if they have no presence in SRAT.
> > Thus the simplest fix is to set numa_off when it is off due
> > to an invalid SRAT (here not present at all).
> > 
> > I do not have easy access to appropriate x86 numa systems so
> > would appreciate some testing of this one!
> > 
> > Known problem boards setups:
> > 
> > AMD Ryzen Threadripper 2950X on ASROCK X399 TAICHI
> > MSI X399 SLI PLUS (probably - not confirmed yet)
> > 
> > The PCI patch has been reverted, so this fix is not critical.
> > 
> > Reported-by: Martin Hundeb?ll <martin@geanix.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Fixes: bad7dcd94f39 ("ACPI/PCI: Pay attention to device-specific _PXM node values")
> > 
> > ---
> >  arch/x86/mm/numa.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> > index 1308f5408bf7..ce1182f953ff 100644
> > --- a/arch/x86/mm/numa.c
> > +++ b/arch/x86/mm/numa.c
> > @@ -695,6 +695,8 @@ static int __init dummy_numa_init(void)
> >  	node_set(0, numa_nodes_parsed);
> >  	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
> >  
> > +	numa_off = true;  
> 
> Should we not:
> 
> 	pr_err(FW_BUG "Invalid SRAT table.\n");
> 
> or something along those lines?
> 
> We should take every possibility to call out broken and non-compliant
> firmware.

While I agree we should definitely be calling this out nice and loud,
not having an SRAT isn't indicating a broken firmware as SRAT is optional
in ACPI.  The breakage only occurs much later when the DSDT contains a
PXM entry that doesn't correspond to any entries in SRAT.

To report at that point, we would need some background info stashed
on why numa_off was set.  It could be off because we set it so via
the kernel command line.

If people are happy with this general direction I'm happy to spin a
patch to do that state tracking and pr_err as a follow up.

Thanks,

Jonathan

Re: [PATCH] x86: Fix an issue with invalid ACPI numa config

[Jonathan Cameron]

On Tue, 20 Nov 2018 13:19:35 +0000
Jonathan Cameron <jonathan.cameron@huawei.com> wrote:

> On Tue, 20 Nov 2018 13:01:05 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Thu, Nov 15, 2018 at 11:06:17AM +0000, Jonathan Cameron wrote:  
> > > The addition of support to read the numa node for a PCI
> > > card specified by _PXM resulted in Martin's system not
> > > booting.   Looking at the ACPI tables it seems that there
> > > are PXM entries for the root ports, but no SRAT table.
> > > 
> > > The absence of SRAT table results in dummy_numa_init being
> > > called.  However, unlike on arm64, this doesn't then result
> > > in numa_off being set.  When the PCI code later comes along
> > > and calls acpi_get_node for any PCI card below the root port,
> > > it navigates up the ACPI tree until it finds the PXM value in
> > > the root port. This value is then passed to
> > > acpi_map_pxm_to_node.  If numa_off is set this returns,
> > > NUMA_NO_NODE (as it does on arm64), on x86 it instead tries
> > > to allocate a numa node from the unused set without setting
> > > up all the infrastructure that would normally accompany such
> > > a call.  We have not identified exactly which driver is
> > > causing the subsequent hang for Martin.
> > > 
> > > It is invalid under the ACPI spec to specify new
> > > numa nodes using PXM if they have no presence in SRAT.
> > > Thus the simplest fix is to set numa_off when it is off due
> > > to an invalid SRAT (here not present at all).
> > > 
> > > I do not have easy access to appropriate x86 numa systems so
> > > would appreciate some testing of this one!
> > > 
> > > Known problem boards setups:
> > > 
> > > AMD Ryzen Threadripper 2950X on ASROCK X399 TAICHI
> > > MSI X399 SLI PLUS (probably - not confirmed yet)
> > > 
> > > The PCI patch has been reverted, so this fix is not critical.
> > > 
> > > Reported-by: Martin Hundeb?ll <martin@geanix.com>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > Fixes: bad7dcd94f39 ("ACPI/PCI: Pay attention to device-specific _PXM node values")
> > > 
> > > ---
> > >  arch/x86/mm/numa.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> > > index 1308f5408bf7..ce1182f953ff 100644
> > > --- a/arch/x86/mm/numa.c
> > > +++ b/arch/x86/mm/numa.c
> > > @@ -695,6 +695,8 @@ static int __init dummy_numa_init(void)
> > >  	node_set(0, numa_nodes_parsed);
> > >  	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
> > >  
> > > +	numa_off = true;    
> > 
> > Should we not:
> > 
> > 	pr_err(FW_BUG "Invalid SRAT table.\n");
> > 
> > or something along those lines?
> > 
> > We should take every possibility to call out broken and non-compliant
> > firmware.  
> 
> While I agree we should definitely be calling this out nice and loud,
> not having an SRAT isn't indicating a broken firmware as SRAT is optional
> in ACPI.  The breakage only occurs much later when the DSDT contains a
> PXM entry that doesn't correspond to any entries in SRAT.
> 
> To report at that point, we would need some background info stashed
> on why numa_off was set.  It could be off because we set it so via
> the kernel command line.
> 
> If people are happy with this general direction I'm happy to spin a
> patch to do that state tracking and pr_err as a follow up.
> 

Just a quick *bump* on this one as I'd really like to get the PCIe patch
that caused the issue back in for the next merge window if possible.
There is some related new ACPI spec stuff coming shortly that will be
making more use of this.

Martin tested the basic principle (if not necessarily exactly this patch)
with success.

Ingo, did you have a chance to take a look at this for your test system?

To my mind it seems like a very straight forward fix, with a small potential
that someone is dong something 'interesting' that would stop working due to
us blocking the domain lookup when SRAT isn't present.

Thanks,

Jonathan

Re: [PATCH] x86: Fix an issue with invalid ACPI numa config

[Bjorn Helgaas]

I don't see this in linux-next so I assume nobody has picked it up yet?
numa.c isn't my file, but the fix is for something that went in via
PCI, so I can take this patch if nobody else has already.

The changelog could all be wrapped to use more of the line.  Assume 80
char width and allow for the 4 spaces added by git log.

On Thu, Nov 15, 2018 at 11:06:17AM +0000, Jonathan Cameron wrote:
> The addition of support to read the numa node for a PCI

s/numa/NUMA/

> card specified by _PXM resulted in Martin's system not
> booting.   Looking at the ACPI tables it seems that there
> are PXM entries for the root ports, but no SRAT table.

s/PXM/_PXM/ (and below)

> The absence of SRAT table results in dummy_numa_init being

dummy_numa_init() and similar for other functions below.

> called.  However, unlike on arm64, this doesn't then result
> in numa_off being set.  When the PCI code later comes along
> and calls acpi_get_node for any PCI card below the root port,
> it navigates up the ACPI tree until it finds the PXM value in
> the root port. This value is then passed to
> acpi_map_pxm_to_node.  If numa_off is set this returns,
> NUMA_NO_NODE (as it does on arm64), on x86 it instead tries

This doesn't parse quite right.

> to allocate a numa node from the unused set without setting
> up all the infrastructure that would normally accompany such
> a call.  We have not identified exactly which driver is
> causing the subsequent hang for Martin.

It's not clear to me why dummy_numa_init() must be different between
arm64 and x86.  This patch makes x86 more like arm64, which is good.
Maybe a subsequent patch could go even further?

> It is invalid under the ACPI spec to specify new
> numa nodes using PXM if they have no presence in SRAT.
> Thus the simplest fix is to set numa_off when it is off due
> to an invalid SRAT (here not present at all).
> 
> I do not have easy access to appropriate x86 numa systems so
> would appreciate some testing of this one!
> 
> Known problem boards setups:
> 
> AMD Ryzen Threadripper 2950X on ASROCK X399 TAICHI
> MSI X399 SLI PLUS (probably - not confirmed yet)
> 
> The PCI patch has been reverted, so this fix is not critical.
> 
> Reported-by: Martin Hundeb?ll <martin@geanix.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Fixes: bad7dcd94f39 ("ACPI/PCI: Pay attention to device-specific _PXM node values")
> 
> ---
>  arch/x86/mm/numa.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index 1308f5408bf7..ce1182f953ff 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -695,6 +695,8 @@ static int __init dummy_numa_init(void)
>  	node_set(0, numa_nodes_parsed);
>  	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
>  
> +	numa_off = true;
> +
>  	return 0;
>  }
>  
> -- 
> 2.18.0
> 
> 

Re: [PATCH] x86: Fix an issue with invalid ACPI numa config

[Jonathan Cameron]

On Mon, 10 Dec 2018 17:56:37 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> I don't see this in linux-next so I assume nobody has picked it up yet?
> numa.c isn't my file, but the fix is for something that went in via
> PCI, so I can take this patch if nobody else has already.

Great. I've been assuming it is a safe change to make, but not gotten
a great deal of feedback from the x86 side.

> 
> The changelog could all be wrapped to use more of the line.  Assume 80
> char width and allow for the 4 spaces added by git log.
> 
> On Thu, Nov 15, 2018 at 11:06:17AM +0000, Jonathan Cameron wrote:
> > The addition of support to read the numa node for a PCI  
> 
> s/numa/NUMA/
> 
> > card specified by _PXM resulted in Martin's system not
> > booting.   Looking at the ACPI tables it seems that there
> > are PXM entries for the root ports, but no SRAT table.  
> 
> s/PXM/_PXM/ (and below)
> 
> > The absence of SRAT table results in dummy_numa_init being  
> 
> dummy_numa_init() and similar for other functions below.
> 
> > called.  However, unlike on arm64, this doesn't then result
> > in numa_off being set.  When the PCI code later comes along
> > and calls acpi_get_node for any PCI card below the root port,
> > it navigates up the ACPI tree until it finds the PXM value in
> > the root port. This value is then passed to
> > acpi_map_pxm_to_node.  If numa_off is set this returns,
> > NUMA_NO_NODE (as it does on arm64), on x86 it instead tries  
> 
> This doesn't parse quite right.
> 
> > to allocate a numa node from the unused set without setting
> > up all the infrastructure that would normally accompany such
> > a call.  We have not identified exactly which driver is
> > causing the subsequent hang for Martin.  
> 
> It's not clear to me why dummy_numa_init() must be different between
> arm64 and x86.  This patch makes x86 more like arm64, which is good.
> Maybe a subsequent patch could go even further?

I agree there may be potential here for a bit of unification
at a later stage. There is an awful lot of handling in this
area that is 'similar' between the architectures.

V2 will fix the other things you have commented on.

Thanks,

Jonathan

> 
> > It is invalid under the ACPI spec to specify new
> > numa nodes using PXM if they have no presence in SRAT.
> > Thus the simplest fix is to set numa_off when it is off due
> > to an invalid SRAT (here not present at all).
> > 
> > I do not have easy access to appropriate x86 numa systems so
> > would appreciate some testing of this one!
> > 
> > Known problem boards setups:
> > 
> > AMD Ryzen Threadripper 2950X on ASROCK X399 TAICHI
> > MSI X399 SLI PLUS (probably - not confirmed yet)
> > 
> > The PCI patch has been reverted, so this fix is not critical.
> > 
> > Reported-by: Martin Hundeb?ll <martin@geanix.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Fixes: bad7dcd94f39 ("ACPI/PCI: Pay attention to device-specific _PXM node values")
> > 
> > ---
> >  arch/x86/mm/numa.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> > index 1308f5408bf7..ce1182f953ff 100644
> > --- a/arch/x86/mm/numa.c
> > +++ b/arch/x86/mm/numa.c
> > @@ -695,6 +695,8 @@ static int __init dummy_numa_init(void)
> >  	node_set(0, numa_nodes_parsed);
> >  	numa_add_memblk(0, 0, PFN_PHYS(max_pfn));
> >  
> > +	numa_off = true;
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.18.0
> > 
> >