diff mbox series

[v2] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS

Message ID 20231223001044.1401226-1-alison.schofield@intel.com
State New
Headers show
Series [v2] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS | expand

Commit Message

Alison Schofield Dec. 23, 2023, 12:10 a.m. UTC 
From: Alison Schofield <alison.schofield@intel.com>

When the BIOS only partially describes a CFMWS Window in the SRAT
the acpi driver uses numa_fill_memblks() to extend existing memblk(s)
to fill the entire CFMWS Window, thereby applying the proximity domain
to the entire CFMWS.

The calculation of the memblks to fill has an off-by-one error, that
causes numa_init to fail when it sees the overlap:

[] ACPI: SRAT: Node 0 PXM 0 [mem 0x00000000-0x7fffffff]
[] ACPI: SRAT: Node 0 PXM 0 [mem 0x100000000-0xffffffffff]
[] ACPI: SRAT: Node 1 PXM 1 [mem 0x10000000000-0x1ffffffffff]
[] node 0 [mem 0x100000000-0xffffffffff] overlaps with node 1 [mem 0x100000000-0x1ffffffffff]

Fix by making the 'end' parameter to numa_fill_memblks() exclusive.

Fixes: 8f1004679987 ("ACPI/NUMA: Apply SRAT proximity domain to entire CFMWS window")
Reported-by: "Huang, Ying" <ying.huang@intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---

Changes in v2:
- Send to ACPI maintainer, reviewer, and mailing list.


 drivers/acpi/numa/srat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86

Comments

Rafael J. Wysocki Dec. 29, 2023, 5:43 p.m. UTC | #1 
On Sat, Dec 23, 2023 at 1:10 AM <alison.schofield@intel.com> wrote:
>
> From: Alison Schofield <alison.schofield@intel.com>
>
> When the BIOS only partially describes a CFMWS Window in the SRAT
> the acpi driver uses numa_fill_memblks() to extend existing memblk(s)
> to fill the entire CFMWS Window, thereby applying the proximity domain
> to the entire CFMWS.
>
> The calculation of the memblks to fill has an off-by-one error, that
> causes numa_init to fail when it sees the overlap:
>
> [] ACPI: SRAT: Node 0 PXM 0 [mem 0x00000000-0x7fffffff]
> [] ACPI: SRAT: Node 0 PXM 0 [mem 0x100000000-0xffffffffff]
> [] ACPI: SRAT: Node 1 PXM 1 [mem 0x10000000000-0x1ffffffffff]
> [] node 0 [mem 0x100000000-0xffffffffff] overlaps with node 1 [mem 0x100000000-0x1ffffffffff]
>
> Fix by making the 'end' parameter to numa_fill_memblks() exclusive.
>
> Fixes: 8f1004679987 ("ACPI/NUMA: Apply SRAT proximity domain to entire CFMWS window")
> Reported-by: "Huang, Ying" <ying.huang@intel.com>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>
> Changes in v2:
> - Send to ACPI maintainer, reviewer, and mailing list.
>
>
>  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 12f330b0eac0..b99062f7c412 100644
> --- a/drivers/acpi/numa/srat.c
> +++ b/drivers/acpi/numa/srat.c
> @@ -308,7 +308,7 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
>
>         cfmws = (struct acpi_cedt_cfmws *)header;
>         start = cfmws->base_hpa;
> -       end = cfmws->base_hpa + cfmws->window_size;
> +       end = cfmws->base_hpa + cfmws->window_size - 1;
>
>         /*
>          * The SRAT may have already described NUMA details for all,
>
> base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
> --

Applied as 6.8 material, thanks!
Alison Schofield Dec. 29, 2023, 8:04 p.m. UTC | #2 
On Fri, Dec 29, 2023 at 06:43:26PM +0100, Rafael J. Wysocki wrote:
> On Sat, Dec 23, 2023 at 1:10 AM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > When the BIOS only partially describes a CFMWS Window in the SRAT
> > the acpi driver uses numa_fill_memblks() to extend existing memblk(s)
> > to fill the entire CFMWS Window, thereby applying the proximity domain
> > to the entire CFMWS.
> >
> > The calculation of the memblks to fill has an off-by-one error, that
> > causes numa_init to fail when it sees the overlap:
> >
> > [] ACPI: SRAT: Node 0 PXM 0 [mem 0x00000000-0x7fffffff]
> > [] ACPI: SRAT: Node 0 PXM 0 [mem 0x100000000-0xffffffffff]
> > [] ACPI: SRAT: Node 1 PXM 1 [mem 0x10000000000-0x1ffffffffff]
> > [] node 0 [mem 0x100000000-0xffffffffff] overlaps with node 1 [mem 0x100000000-0x1ffffffffff]
> >
> > Fix by making the 'end' parameter to numa_fill_memblks() exclusive.
> >
> > Fixes: 8f1004679987 ("ACPI/NUMA: Apply SRAT proximity domain to entire CFMWS window")
> > Reported-by: "Huang, Ying" <ying.huang@intel.com>
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >
> > Changes in v2:
> > - Send to ACPI maintainer, reviewer, and mailing list.
> >
> >
> >  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 12f330b0eac0..b99062f7c412 100644
> > --- a/drivers/acpi/numa/srat.c
> > +++ b/drivers/acpi/numa/srat.c
> > @@ -308,7 +308,7 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> >
> >         cfmws = (struct acpi_cedt_cfmws *)header;
> >         start = cfmws->base_hpa;
> > -       end = cfmws->base_hpa + cfmws->window_size;
> > +       end = cfmws->base_hpa + cfmws->window_size - 1;
> >
> >         /*
> >          * The SRAT may have already described NUMA details for all,
> >
> > base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
> > --
> 
> Applied as 6.8 material, thanks!

Sorry Rafael - this patch is bad.  Can you back it out?

I got some feedback here that showed this breaks a subsequent call
to numa_add_memblks().
https://lore.kernel.org/linux-cxl/ZYyozZcK9g4JE11B@aschofie-mobl2/T/#m16f13cafc186c7ebbf6037b3ae0e3ae572a83f77

ATM I expect this will lead to a fix in x86/mm/numa, not ACPI.

My apologies for messing up the mailing list and causing this confusion.

Alison
Rafael J. Wysocki Jan. 1, 2024, 3:08 p.m. UTC | #3 
On Fri, Dec 29, 2023 at 9:04 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
> On Fri, Dec 29, 2023 at 06:43:26PM +0100, Rafael J. Wysocki wrote:
> > On Sat, Dec 23, 2023 at 1:10 AM <alison.schofield@intel.com> wrote:
> > >
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > When the BIOS only partially describes a CFMWS Window in the SRAT
> > > the acpi driver uses numa_fill_memblks() to extend existing memblk(s)
> > > to fill the entire CFMWS Window, thereby applying the proximity domain
> > > to the entire CFMWS.
> > >
> > > The calculation of the memblks to fill has an off-by-one error, that
> > > causes numa_init to fail when it sees the overlap:
> > >
> > > [] ACPI: SRAT: Node 0 PXM 0 [mem 0x00000000-0x7fffffff]
> > > [] ACPI: SRAT: Node 0 PXM 0 [mem 0x100000000-0xffffffffff]
> > > [] ACPI: SRAT: Node 1 PXM 1 [mem 0x10000000000-0x1ffffffffff]
> > > [] node 0 [mem 0x100000000-0xffffffffff] overlaps with node 1 [mem 0x100000000-0x1ffffffffff]
> > >
> > > Fix by making the 'end' parameter to numa_fill_memblks() exclusive.
> > >
> > > Fixes: 8f1004679987 ("ACPI/NUMA: Apply SRAT proximity domain to entire CFMWS window")
> > > Reported-by: "Huang, Ying" <ying.huang@intel.com>
> > > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > > ---
> > >
> > > Changes in v2:
> > > - Send to ACPI maintainer, reviewer, and mailing list.
> > >
> > >
> > >  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 12f330b0eac0..b99062f7c412 100644
> > > --- a/drivers/acpi/numa/srat.c
> > > +++ b/drivers/acpi/numa/srat.c
> > > @@ -308,7 +308,7 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> > >
> > >         cfmws = (struct acpi_cedt_cfmws *)header;
> > >         start = cfmws->base_hpa;
> > > -       end = cfmws->base_hpa + cfmws->window_size;
> > > +       end = cfmws->base_hpa + cfmws->window_size - 1;
> > >
> > >         /*
> > >          * The SRAT may have already described NUMA details for all,
> > >
> > > base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
> > > --
> >
> > Applied as 6.8 material, thanks!
>
> Sorry Rafael - this patch is bad.  Can you back it out?
>
> I got some feedback here that showed this breaks a subsequent call
> to numa_add_memblks().
> https://lore.kernel.org/linux-cxl/ZYyozZcK9g4JE11B@aschofie-mobl2/T/#m16f13cafc186c7ebbf6037b3ae0e3ae572a83f77
>
> ATM I expect this will lead to a fix in x86/mm/numa, not ACPI.
>
> My apologies for messing up the mailing list and causing this confusion.

No worries, thanks for letting me know.

Dropped now.
diff mbox series

Patch

diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
index 12f330b0eac0..b99062f7c412 100644
--- a/drivers/acpi/numa/srat.c
+++ b/drivers/acpi/numa/srat.c
@@ -308,7 +308,7 @@  static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
 
 	cfmws = (struct acpi_cedt_cfmws *)header;
 	start = cfmws->base_hpa;
-	end = cfmws->base_hpa + cfmws->window_size;
+	end = cfmws->base_hpa + cfmws->window_size - 1;
 
 	/*
 	 * The SRAT may have already described NUMA details for all,