| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z9PhAKEFj5ugVuow@phytium.com.cn>
Date: Fri, 14 Mar 2025 15:55:44 +0800
From: Yuquan Wang <wangyuquan1236@...tium.com.cn>
To: Alison Schofield <alison.schofield@...el.com>
Cc: rafael@...nel.org, lenb@...nel.org, dave@...olabs.net,
jonathan.cameron@...wei.com, dave.jiang@...el.com,
vishal.l.verma@...el.com, ira.weiny@...el.com,
dan.j.williams@...el.com, rrichter@....com, bfaccini@...dia.com,
rppt@...nel.org, haibo1.xu@...el.com, chenbaozi@...tium.com.cn,
linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-cxl@...r.kernel.org
Subject: Re: [PATCH v2] ACPI: NUMA: debug invalid unused PXM value for CFMWs
On Thu, Mar 13, 2025 at 09:28:46AM -0700, Alison Schofield wrote:
> On Thu, Mar 13, 2025 at 02:09:07PM +0800, Yuquan Wang wrote:
> > The absence of SRAT would cause the fake_pxm to be -1 and increment
> > to 0, then send to acpi_parse_cfmws(). If there exists CXL memory
> > ranges that are defined in the CFMWS and not already defined in the
> > SRAT, the new node (node0) for the CXL memory would be invalid, as
> > node0 is already in "used".
> >
> > This utilizes disable_srat() & srat_disabled() to fail CXL init.
>
> Seems like this fixup has drifted from adjusting the fake_pxm to
> shutting down CXL parsing. More below -
>
> >
> > Signed-off-by: Yuquan Wang <wangyuquan1236@...tium.com.cn>
> > ---
> >
> > Changes in v2:
> > - Add disable_srat() when fake_pxm is invalid
> > - Add srat_disabled() check in cxl_acpi_probe() and acpi_parse_cfmws()
> >
> >
> > drivers/acpi/numa/srat.c | 10 ++++++++++
> > drivers/cxl/acpi.c | 4 ++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> > index 00ac0d7bb8c9..2dac25c9258a 100644
> > --- a/drivers/acpi/numa/srat.c
> > +++ b/drivers/acpi/numa/srat.c
> > @@ -441,6 +441,11 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
> > start = cfmws->base_hpa;
> > end = cfmws->base_hpa + cfmws->window_size;
> >
> > + if (srat_disabled()) {
> > + pr_err("SRAT is missing or bad while processing CFMWS.\n");
> > + return -EINVAL;
> > + }
> > +
>
> This goes too far by shutting down cfmws parsing for lack of SRAT.
>
Actually, I thought there need another patch to fix the follow problem
that the fake node bring when no SRAT.
Detailed description below.
> > /*
> > * The SRAT may have already described NUMA details for all,
> > * or a portion of, this CFMWS HPA range. Extend the memblks
> > @@ -646,6 +651,11 @@ int __init acpi_numa_init(void)
> > if (node_to_pxm_map[i] > fake_pxm)
> > fake_pxm = node_to_pxm_map[i];
> > }
> > +
> > + /* Make sure CFMWs fake nodes start at node[1] */
> > + if (fake_pxm < 0)
> > + disable_srat();
> > +
>
> How does the code above make sure fake node starts at node[1]?
> Would an explicit adjustment like this work?
Thanks for your correction :) Yes, the way I used here is too implicit.
>
> - last_real_pxm = fake_pxm;
> - fake_pxm++;
> + fake_pxm = max(fake_pxm, 1);
> + last_real_pxm = fake_pxm--;
I tried the adjustment below:
fake_pxm = max(fake_pxm, 0); // 0 because it will increment to 1
last_real_pxm = fake_pxm++;
This works but it might only control the parameter sent to acpi_parse_cfmws().
According to acpi_map_pxm_to_node(), altough the input fake_pxm is 1 when no
SRAT, the returned node would still be 0 and the following nodes are aslo
incorrect.
Hence, I tried add a new line below:
fake_pxm = max(fake_pxm, 0);
last_real_pxm = fake_pxm++;
node_set(0, nodes_found_map);
As no matter what situation, node[0] would be found and set. With this
setting, acpi_map_pxm_to_node() could return the expected node value
even if no SRAT. :(
Unfortunately, when we use "cxl create-region" to enable our cxl memory,
it would still be assigned to node[0], because the "numa_add_memblk()"
can only add numa_memblk to numa_meminfo list.
If our SRAT is OK, the numa_memblks_init() would then utilize
numa_move_tail_memblk() to move the numa_memblk from numa_meminfo to
numa_reserved_meminfo in CFMWs fake node situation. If SRAT is missing
or bad, the numa_memblks_init() would fail since init_func() would fail.
And it causes that no numa_memblk in numa_reserved_meminfo list and the
following dax&memory_hotplug drivers could not online the expected fake
node.
Based on the above problem, I have a new patch idea that introduce a new
function in mm/numa_memblks.c: numa_add_reserved_memblk(). It could add
one numa_memblk to nuam_reserved_meminfo directly. Maybe we could call
it in acpi_parse_cfmws() if srat is missing.
In mm/numa_memblks.c:
int __init numa_add_reserved_memblk(int nid, u64 start, u64 end)
{
return numa_add_memblk_to(nid, start, end, &numa_reserved_meminfo);
}
In drivers/acpi/numa/srat.c:
if (srat_disabled()) {
if (numa_add_reserved_memblk(node, start, end) < 0) {
pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
node, start, end);
}
}
:( But..., the dax_kmem driver will fail because something wrong in
memory_group_register_static(). The good result is our cxl memory would
not be assigned to node[0] anymore!
BTW, as papering these things looks like not easily, I chose to aggressively
fail the acpi_parse_cfmws() in srat.c since it mainly works for building
cxl fake nodes and also fail the CXL init in cxl_acpi_probe per Jonathan.
Link: https://lists.nongnu.org/archive/html/qemu-devel/2025-03/msg03668.html
Hopes more comments to guide me! I'm a really rookie in kernel community :P
> > last_real_pxm = fake_pxm;
> > fake_pxm++;
> > acpi_table_parse_cedt(ACPI_CEDT_TYPE_CFMWS, acpi_parse_cfmws,
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index cb14829bb9be..e75a8ead99f6 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -829,6 +829,10 @@ static int cxl_acpi_probe(struct platform_device *pdev)
> > if (rc)
> > return rc;
> >
> > + /* CXL must be in a NUMA system */
> > + if (srat_disabled())
> > + return -EINVAL;
> > +
> > cxl_res = devm_kzalloc(host, sizeof(*cxl_res), GFP_KERNEL);
> > if (!cxl_res)
> > return -ENOMEM;
> > --
> > 2.34.1
> >
Powered by blists - more mailing lists