| Message ID | 20240424154846.2152750-7-rrichter@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SRAT/CEDT fixes and updates | expand |
Robert Richter wrote: > Adding a pr_info() when successfully adding a CFMWS memory range. > > Suggested-by: Alison Schofield <alison.schofield@intel.com> > Signed-off-by: Robert Richter <rrichter@amd.com> > --- > drivers/acpi/numa/srat.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c > index e3f26e71637a..c62e4636e472 100644 > --- a/drivers/acpi/numa/srat.c > +++ b/drivers/acpi/numa/srat.c > @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, > * found for any portion of the window to cover the entire > * window. > */ > - if (!numa_fill_memblks(start, end)) > + if (!numa_fill_memblks(start, end)) { > + pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n", > + (unsigned long long) start, (unsigned long long) end - 1); This looks like pr_debug() material to me. > return 0; > + } > > /* No SRAT description. Create a new node. */ > node = acpi_map_pxm_to_node(*fake_pxm); > @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, > pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n", > node, start, end); > } > + > node_set(node, numa_nodes_parsed); > > + pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n", > + node, *fake_pxm, > + (unsigned long long) start, (unsigned long long) end - 1); > + This makes sense to mirror the SRAT pr_info().
On 24.04.24 10:54:22, Dan Williams wrote: > Robert Richter wrote: > > Adding a pr_info() when successfully adding a CFMWS memory range. > > > > Suggested-by: Alison Schofield <alison.schofield@intel.com> > > Signed-off-by: Robert Richter <rrichter@amd.com> > > --- > > drivers/acpi/numa/srat.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c > > index e3f26e71637a..c62e4636e472 100644 > > --- a/drivers/acpi/numa/srat.c > > +++ b/drivers/acpi/numa/srat.c > > @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, > > * found for any portion of the window to cover the entire > > * window. > > */ > > - if (!numa_fill_memblks(start, end)) > > + if (!numa_fill_memblks(start, end)) { > > + pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n", > > + (unsigned long long) start, (unsigned long long) end - 1); > > This looks like pr_debug() material to me. This should have the same log level as the message below and/or its corresponding SRAT message. CEDT mem blocks wouldn't be reported otherwise only because a smaller (overlapping) entry was registered before. That is why I used pr_info here. > > > return 0; > > + } > > > > /* No SRAT description. Create a new node. */ > > node = acpi_map_pxm_to_node(*fake_pxm); > > @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, > > pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n", > > node, start, end); > > } > > + > > node_set(node, numa_nodes_parsed); > > > > + pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n", > > + node, *fake_pxm, > > + (unsigned long long) start, (unsigned long long) end - 1); > > + > > This makes sense to mirror the SRAT pr_info(). I evaluated this. SRAT shows this: pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n", node, pxm, (unsigned long long) start, (unsigned long long) end - 1, hotpluggable ? " hotplug" : "", ma->flags & ACPI_SRAT_MEM_NON_VOLATILE ? " non-volatile" : ""); There is no direct mapping of SRAT memory affinity flags (acpi-6.5 spec, table 5.59) and something in the CFMWS entry (cxl-3.1, table 9-22). There is no "hotplug" flag and the "non-volatile" part would be ambiguous, as some logic must be defined to handle the "volatile" and "persistent" Window Restrictions. Since the CFMWS restrictions are not used at all by the kernel my conclusion was to just dropped the flag for the CEDT info. Note there is a mapping defined for CDAT DSMAS and SRAT entries, see CDAT 1.03 spec, Table 4. -Robert
On Thu, Apr 25, 2024 at 09:30:15AM +0200, Robert Richter wrote: > On 24.04.24 10:54:22, Dan Williams wrote: > > Robert Richter wrote: > > > Adding a pr_info() when successfully adding a CFMWS memory range. > > > > > > Suggested-by: Alison Schofield <alison.schofield@intel.com> > > > Signed-off-by: Robert Richter <rrichter@amd.com> > > > --- > > > drivers/acpi/numa/srat.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c > > > index e3f26e71637a..c62e4636e472 100644 > > > --- a/drivers/acpi/numa/srat.c > > > +++ b/drivers/acpi/numa/srat.c > > > @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, > > > * found for any portion of the window to cover the entire > > > * window. > > > */ > > > - if (!numa_fill_memblks(start, end)) > > > + if (!numa_fill_memblks(start, end)) { > > > + pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n", > > > + (unsigned long long) start, (unsigned long long) end - 1); > > > > This looks like pr_debug() material to me. > > This should have the same log level as the message below and/or its > corresponding SRAT message. CEDT mem blocks wouldn't be reported > otherwise only because a smaller (overlapping) entry was registered > before. That is why I used pr_info here. It does feel like this message belongs but maybe it should also mimic the SRAT define message and emit what node maps this range if memblocks were extended. But...seems this will emit a message for every CFMWS range, even those where no memblk needed to be extended - ie the range was fully described in the SRAT. Sadly, numa_fill_memblks() return of 'success' has double meaning. It can mean memblks were extended, or that (start, end) was found fully described. I don't see an good place to insert the message in numa_fill_memblks(). Sorry, just stirring the pot here, with no clear suggestion on how to emit info. > > > > > > return 0; > > > + } > > > > > > /* No SRAT description. Create a new node. */ > > > node = acpi_map_pxm_to_node(*fake_pxm); > > > @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, > > > pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n", > > > node, start, end); > > > } > > > + > > > node_set(node, numa_nodes_parsed); > > > > > > + pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n", > > > + node, *fake_pxm, > > > + (unsigned long long) start, (unsigned long long) end - 1); > > > + > > > > This makes sense to mirror the SRAT pr_info(). > > I evaluated this. > I read Dan's comment as simple acceptance. Like, yeah this one is good because it mimics the SRAT pr_info. > SRAT shows this: > > pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n", > node, pxm, > (unsigned long long) start, (unsigned long long) end - 1, > hotpluggable ? " hotplug" : "", > ma->flags & ACPI_SRAT_MEM_NON_VOLATILE ? " non-volatile" : ""); > > There is no direct mapping of SRAT memory affinity flags (acpi-6.5 > spec, table 5.59) and something in the CFMWS entry (cxl-3.1, table > 9-22). There is no "hotplug" flag and the "non-volatile" part would be > ambiguous, as some logic must be defined to handle the "volatile" and > "persistent" Window Restrictions. Since the CFMWS restrictions are not > used at all by the kernel my conclusion was to just dropped the flag > for the CEDT info. > > Note there is a mapping defined for CDAT DSMAS and SRAT entries, see > CDAT 1.03 spec, Table 4. > > -Robert
On 25.04.24 11:56:44, Alison Schofield wrote: > On Thu, Apr 25, 2024 at 09:30:15AM +0200, Robert Richter wrote: > > On 24.04.24 10:54:22, Dan Williams wrote: > > > Robert Richter wrote: > > > > Adding a pr_info() when successfully adding a CFMWS memory range. > > > > > > > > Suggested-by: Alison Schofield <alison.schofield@intel.com> > > > > Signed-off-by: Robert Richter <rrichter@amd.com> > > > > --- > > > > drivers/acpi/numa/srat.c | 10 +++++++++- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c > > > > index e3f26e71637a..c62e4636e472 100644 > > > > --- a/drivers/acpi/numa/srat.c > > > > +++ b/drivers/acpi/numa/srat.c > > > > @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, > > > > * found for any portion of the window to cover the entire > > > > * window. > > > > */ > > > > - if (!numa_fill_memblks(start, end)) > > > > + if (!numa_fill_memblks(start, end)) { > > > > + pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n", > > > > + (unsigned long long) start, (unsigned long long) end - 1); > > > > > > This looks like pr_debug() material to me. > > > > This should have the same log level as the message below and/or its > > corresponding SRAT message. CEDT mem blocks wouldn't be reported > > otherwise only because a smaller (overlapping) entry was registered > > before. That is why I used pr_info here. > > It does feel like this message belongs but maybe it should also > mimic the SRAT define message and emit what node maps this range > if memblocks were extended. > > But...seems this will emit a message for every CFMWS range, even those > where no memblk needed to be extended - ie the range was fully described > in the SRAT. > > Sadly, numa_fill_memblks() return of 'success' has double meaning. > It can mean memblks were extended, or that (start, end) was found fully > described. I don't see an good place to insert the message in > numa_fill_memblks(). > > Sorry, just stirring the pot here, with no clear suggestion on how > to emit info. Ok, I have changed numa_fill_memblks() to also return if memblks have been modified. That information is used to print the message. > > > > > > > > > > return 0; > > > > + } > > > > > > > > /* No SRAT description. Create a new node. */ > > > > node = acpi_map_pxm_to_node(*fake_pxm); > > > > @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, > > > > pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n", > > > > node, start, end); > > > > } > > > > + > > > > node_set(node, numa_nodes_parsed); > > > > > > > > + pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n", > > > > + node, *fake_pxm, > > > > + (unsigned long long) start, (unsigned long long) end - 1); > > > > + > > > > > > This makes sense to mirror the SRAT pr_info(). > > > > I evaluated this. > > > > I read Dan's comment as simple acceptance. Like, yeah this one is good > because it mimics the SRAT pr_info. Ah, I misread this, thanks for clarification. -Robert
diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c index e3f26e71637a..c62e4636e472 100644 --- a/drivers/acpi/numa/srat.c +++ b/drivers/acpi/numa/srat.c @@ -338,8 +338,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, * found for any portion of the window to cover the entire * window. */ - if (!numa_fill_memblks(start, end)) + if (!numa_fill_memblks(start, end)) { + pr_info("CEDT: memblk extended [mem %#010Lx-%#010Lx]\n", + (unsigned long long) start, (unsigned long long) end - 1); return 0; + } /* No SRAT description. Create a new node. */ node = acpi_map_pxm_to_node(*fake_pxm); @@ -354,8 +357,13 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header, pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n", node, start, end); } + node_set(node, numa_nodes_parsed); + pr_info("CEDT: Node %u PXM %u [mem %#010Lx-%#010Lx]\n", + node, *fake_pxm, + (unsigned long long) start, (unsigned long long) end - 1); + /* Set the next available fake_pxm value */ (*fake_pxm)++; return 0;
Adding a pr_info() when successfully adding a CFMWS memory range. Suggested-by: Alison Schofield <alison.schofield@intel.com> Signed-off-by: Robert Richter <rrichter@amd.com> --- drivers/acpi/numa/srat.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)