* RE: [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS
2023-12-23 0:00 [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS alison.schofield
@ 2023-12-23 0:12 ` Dan Williams
2023-12-23 0:49 ` Dave Jiang
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Dan Williams @ 2023-12-23 0:12 UTC (permalink / raw)
To: alison.schofield, Davidlohr Bueso, Jonathan Cameron, Dave Jiang,
Vishal Verma, Ira Weiny, Dan Williams
Cc: linux-cxl, Huang, Ying, linux-acpi, rafael
[ add Rafael and linux-acpi for their awareness ]
Rafael, since the issue came in through cxl development I will take the fix
through the cxl tree unless you have an objection.
alison.schofield@ 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>
> ---
> 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
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS
2023-12-23 0:00 [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS alison.schofield
2023-12-23 0:12 ` Dan Williams
@ 2023-12-23 0:49 ` Dave Jiang
2023-12-23 19:27 ` fan
2023-12-25 1:39 ` Huang, Ying
3 siblings, 0 replies; 7+ messages in thread
From: Dave Jiang @ 2023-12-23 0:49 UTC (permalink / raw)
To: alison.schofield, Davidlohr Bueso, Jonathan Cameron, Vishal Verma,
Ira Weiny, Dan Williams
Cc: linux-cxl, Huang, Ying
On 12/22/23 17:00, 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>
Reviewed-by: Dave Jiang <dave.jiang@intel.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 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS
2023-12-23 0:00 [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS alison.schofield
2023-12-23 0:12 ` Dan Williams
2023-12-23 0:49 ` Dave Jiang
@ 2023-12-23 19:27 ` fan
2023-12-25 1:39 ` Huang, Ying
3 siblings, 0 replies; 7+ messages in thread
From: fan @ 2023-12-23 19:27 UTC (permalink / raw)
To: alison.schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl, Huang, Ying
On Fri, Dec 22, 2023 at 04:00:25PM -0800, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
>
Reviewed-by: Fan Ni <fan.ni@samsung.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>
> ---
> 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
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS
2023-12-23 0:00 [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS alison.schofield
` (2 preceding siblings ...)
2023-12-23 19:27 ` fan
@ 2023-12-25 1:39 ` Huang, Ying
2023-12-27 22:44 ` Alison Schofield
2024-01-02 19:27 ` Dan Williams
3 siblings, 2 replies; 7+ messages in thread
From: Huang, Ying @ 2023-12-25 1:39 UTC (permalink / raw)
To: alison.schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
<alison.schofield@intel.com> writes:
> 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>
> ---
> 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;
IIUC, this makes end inclusive instead of exclusive. That is, from
"[start, end)" to "[start, end]". Am I right?
If so, later in the function, "numa_add_memblk(node, start, end)" is
called, where "end" is expected to be exclusive. And I think that it's
better to make the parameter "end" of numa_fill_memblks() consistent
with other numa_memblk family functions, that is, exclusive.
> /*
> * The SRAT may have already described NUMA details for all,
>
> base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
--
Best Regards,
Huang, Ying
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS
2023-12-25 1:39 ` Huang, Ying
@ 2023-12-27 22:44 ` Alison Schofield
2024-01-02 19:27 ` Dan Williams
1 sibling, 0 replies; 7+ messages in thread
From: Alison Schofield @ 2023-12-27 22:44 UTC (permalink / raw)
To: Huang, Ying
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
On Mon, Dec 25, 2023 at 09:39:09AM +0800, Huang, Ying wrote:
> <alison.schofield@intel.com> writes:
>
> > 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>
> > ---
> > 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;
>
> IIUC, this makes end inclusive instead of exclusive. That is, from
> "[start, end)" to "[start, end]". Am I right?
You are right.
numa_fill_memblks() is confusing. It's kernel doc comments says
end is inclusive, then in-code comments that end is exclusive,
and then it fails to handle the exclusive case correctly.
I think this change in numa_fill_memblks() that you suggested
(off-list) is the right way to go:
>> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
>> index c01c5506fd4a..3850ba23fa90 100644
>> --- a/arch/x86/mm/numa.c
>> +++ b/arch/x86/mm/numa.c
>> @@ -1007,7 +1007,7 @@ int __init numa_fill_memblks(u64 start, u64 end)
>> for (int i = 0; i < mi->nr_blks; i++) {
>> struct numa_memblk *bi = &mi->blk[i];
>>
>> - if (start < bi->end && end >= bi->start) {
>> + if (start < bi->end && end > bi->start) {
>> blk[count] = &mi->blk[i];
>> count++;
>> }
>>
I'll also clean up the in code & kernel doc comments.
Thanks for the catch!
Alison
>
> If so, later in the function, "numa_add_memblk(node, start, end)" is
> called, where "end" is expected to be exclusive. And I think that it's
> better to make the parameter "end" of numa_fill_memblks() consistent
> with other numa_memblk family functions, that is, exclusive.
>
Agree.
> > /*
> > * The SRAT may have already described NUMA details for all,
> >
> > base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86
>
> --
> Best Regards,
> Huang, Ying
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ACPI: NUMA: Fix overlap when extending memblks to fill CFMWS
2023-12-25 1:39 ` Huang, Ying
2023-12-27 22:44 ` Alison Schofield
@ 2024-01-02 19:27 ` Dan Williams
1 sibling, 0 replies; 7+ messages in thread
From: Dan Williams @ 2024-01-02 19:27 UTC (permalink / raw)
To: Huang, Ying, alison.schofield
Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
Ira Weiny, Dan Williams, linux-cxl
Huang, Ying wrote:
> <alison.schofield@intel.com> writes:
>
> > 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>
> > ---
> > 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;
>
> IIUC, this makes end inclusive instead of exclusive. That is, from
> "[start, end)" to "[start, end]". Am I right?
"Exlcusive" => Exclude the last value ("cfmws->base_hpa +
cfmws->window_size") from the range:
[cfmws->base_hpa, cfmws->window_size)
> If so, later in the function, "numa_add_memblk(node, start, end)" is
> called, where "end" is expected to be exclusive.
Good eye, yes.
> And I think that it's better to make the parameter "end" of
> numa_fill_memblks() consistent with other numa_memblk family
> functions, that is, exclusive.
s/exclusive/inclusive/, but yes for the immediate fix. It is unfortunate
that "end" here is different than the typical meaning of end as in
'struct resource' and 'struct range'. I would love to see a conversion
along the lines of:
diff --git a/arch/x86/mm/numa_internal.h b/arch/x86/mm/numa_internal.h
index 86860f279662..8a0eeac9d64b 100644
--- a/arch/x86/mm/numa_internal.h
+++ b/arch/x86/mm/numa_internal.h
@@ -6,8 +6,7 @@
#include <asm/numa.h>
struct numa_memblk {
- u64 start;
- u64 end;
+ struct range r;
int nid;
};
...and then intoduce helpers in include/linux/range.h to remove the
error-prone open-coded versions of all the range comparison math.
^ permalink raw reply related [flat|nested] 7+ messages in thread