Received-SPF: pass (google.com: domain of linux-kernel+bounces-106721-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 18 Mar 2024 22:50:05 +0100
From: Robert Richter <rrichter@amd.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Alison Schofield <alison.schofield@intel.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-cxl@vger.kernel.org, Derick Marks <derick.w.marks@intel.com>,
	Len Brown <lenb@kernel.org>
Subject: Re: [PATCH 1/3] x86/numa: Fix SRAT lookup for CFMWS ranges with
 numa_fill_memblks()
Message-ID: <Zfi3Da5SNR1NFAGZ@rric.localdomain>
References: <20240318210904.2188120-1-rrichter@amd.com>
 <20240318210904.2188120-2-rrichter@amd.com>
 <65f8b191c0422_aa222941b@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <65f8b191c0422_aa222941b@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?YlwjK3Wabum2Slgn2f3AbbDE04dfnQNwub+mqvBIVmQmVCsAZEc5WUeK3kqD?=
 =?us-ascii?Q?YVaoRFM8OhEGW4oMfOTQwS9R/ZSueFO1FO4GscacHKzxTETEKTWRbVwlRXPj?=
 =?us-ascii?Q?dOcVj5wum87eApknfkUwckNL2DnBoh4G7JeHy6dhHMLwEJyVSgOS9V9Zy/4U?=
 =?us-ascii?Q?zo9A/Y/9qxys6egKV47rtEK2/teW5Y2aZtIiBevf9KmD+pUyMB+5TpejuMaF?=
 =?us-ascii?Q?v+926nsnJbNm7UORrY134xNj4mhWj94U0xhkty6DCjDsjrTn2x5K+Xu9Nwyi?=
 =?us-ascii?Q?2Cd6LniuLVs/1AaTvd+AJ3ayC4Q3Vm0nCOzCO9ChzcaYyodVt5JRyuqLoEkB?=
 =?us-ascii?Q?2YmpJHe4X0d3gjbKt5+FmHmpbO4V32L1zPKcJF46RmsNLAxetHRPziPosPLF?=
 =?us-ascii?Q?vva3FO1tfycRgVsf0TiMtN9g+zdL3IHCLPkO3I1k4524DZHMMQtQHJq4Xyjj?=
 =?us-ascii?Q?WM95IOhcmr1Tbtv5ktGZ0rQAvz2M5ggeaC6dDSzt9gJ5TFKFPAkZ+7Q229to?=
 =?us-ascii?Q?bvjcFAV0f6LP++jWUVOUHRYal+G3i1c0x2w/jBdJh1cAgcL7MS1Lpau+80Q0?=
 =?us-ascii?Q?F5RKDOPo5fKQiu52oDOLVZjZmtHDVYqXaZ0cS67w2n1NpDmy/I0m9goOxJPr?=
 =?us-ascii?Q?IPKwbIz7GORGuqDZPcfs4tA1zXdRFtCcg/9CHqZDQtEg7o4tA5Tl+p9oipYg?=
 =?us-ascii?Q?G7gh4Wm09tgl5vHwafzGANnJhaLv+J9woELsbHmsr4N/v5I6/pvn6KUiN9LB?=
 =?us-ascii?Q?y5vbw5pKNLFcRKNKkWv1b9syOfLnvDnI/VMEJMYWgm1YXwyIPEmyIHIs53dy?=
 =?us-ascii?Q?B5gtkKLmyNMsfUk/G5vv3v6JA+Yq8NT8qYLWB6qSBKKIjcLtqHi72o/kxlWR?=
 =?us-ascii?Q?pzXfw/pY/nbmft4IgqTokSUpZ0NBGKGZk4MbI96I6NbdUei10dxXH2m7ltkY?=
 =?us-ascii?Q?T5IiH+EEsqxOdt3JbRYZWgDQyu6EV8H9oZiqXpbC+7QVAhr1+jOOSjCvzW5E?=
 =?us-ascii?Q?ijmg+nxqobI5xUnk66iK/HxUTWUdr3hIcsokfMrB92guIeftmwIFh3pLcaS0?=
 =?us-ascii?Q?Ek8Icpes4z9+fhHt43vVyvDHFsFuswR0rkvGyYblTbDsOiojbY1AUiPtiJm7?=
 =?us-ascii?Q?9reTvjBz2vk0CklHSVdASdQdk+0l8wikbJPSj2/gLAVjjC78r1Biukg1tW5k?=
 =?us-ascii?Q?VrS3cCiqO4M5Hj3J6q90CS/CS0mlKgFvpv2gzpOER7LuXkDjncnzJmgGwAvv?=
 =?us-ascii?Q?lIBhoWLQKVriSAEJ/4wX+yeRN5MnPSXonXIJTstfLFKvSZzeOVND1V/f7VaQ?=
 =?us-ascii?Q?gMqZUZ+LcbuXOmRkkUVNgImpD1cMicXjC4McKuKblI0MNEljMcgltBAG1C/q?=
 =?us-ascii?Q?8QjRBwB/vUiBc/2wIQWqRp97QtdeUp8jttfEuWVx/cf6Q2jktqiH8APm0rbO?=
 =?us-ascii?Q?r6wJWg5vqxPE+8IYB2FAFNMFS9h0wQwK1kMlJZV6YVmBY0I5gjVBA/AEZb31?=
 =?us-ascii?Q?Phlyu/bB3WxngFQ3Qv/B0+q6rn8W+PRW6OEQNeiZWdCBJ42dwtZC5jwLnlOA?=
 =?us-ascii?Q?L+oDO46n3hzO10uqh8hshXvTnHaFNIeZ/5+a7oLy?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3f05a685-44d6-4a51-1bc5-08dc479562d0
X-MS-Exchange-CrossTenant-AuthSource: CYYPR12MB8750.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Mar 2024 21:50:11.3990
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qwQnBLW4VPhVG5c0J7G2AXih/68J6ekBc8Q3qbJzOnOET3pBHATddvliPgY3acUhRklKz6u2vVedyLECs2XB2g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB8769

Hi Dan,

thanks for the quick review.

Yes, this is the 'old' patch. But only the subject was corrected. I
will send a v2 anyway. See below.

On 18.03.24 14:26:41, Dan Williams wrote:
> Robert Richter wrote:
> > With kconfig option NUMA_KEEP_MEMINFO disabled the SRAT lookup done
> > with numa_fill_memblks() fails returning NUMA_NO_MEMBLK (-1). An
> > existing SRAT memory range cannot be found for a CFMWS address range.
> > This causes the addition of a duplicate numa_memblk with a different
> > node id and a subsequent page fault and kernel crash during boot.
> > 
> > Note that the issue was initially introduced with [1]. But since
> > phys_to_target_node() was originally used that returned the valid node
> > 0, an additional numa_memblk was not added. Though, the node id was
> > wrong too.
> > 
> > Fix this by enabling NUMA_KEEP_MEMINFO for x86 with ACPI and NUMA
> > enabled.
> > 
> > [1] fd49f99c1809 ("ACPI: NUMA: Add a node and memblk for each CFMWS not in SRAT")
> > 
> > Fixes: 8f1004679987 ("ACPI/NUMA: Apply SRAT proximity domain to entire CFMWS window")
> > Cc: Derick Marks <derick.w.marks@intel.com>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Alison Schofield <alison.schofield@intel.com>
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > ---
> >  drivers/acpi/numa/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/acpi/numa/Kconfig b/drivers/acpi/numa/Kconfig
> > index 849c2bd820b9..2f4ac6ac6768 100644
> > --- a/drivers/acpi/numa/Kconfig
> > +++ b/drivers/acpi/numa/Kconfig
> > @@ -3,6 +3,7 @@ config ACPI_NUMA
> >  	bool "NUMA support"
> >  	depends on NUMA
> >  	depends on (X86 || ARM64 || LOONGARCH)
> > +	select NUMA_KEEP_MEMINFO if X86
> >  	default y if ARM64
> 
> A fix is needed, yes, but this is the wrong one. NUMA_KEEP_MEMINFO is
> only about marking numa_meminfo data as not "__init". Since
> numa_fill_memblks() *is* an __init function, it should have no
> dependency on NUMA_KEEP_MEMINFO.

Right, the option is about just keeping it in non-init mem, but the
parsing is durint __init. Will take a look.

> 
> The fix here involves moving the definition of numa_fill_memblks() out
> of the "#ifdef CONFIG_NUMA_KEEP_MEMINFO" in
> arch/x86/include/asm/sparsemem.h so that it does not fallback to the
> default definition in include/linux/numa.h.
> 
> It should also be the case that cxl_acpi needs this:
> 
> diff --git a/drivers/cxl/Kconfig b/drivers/cxl/Kconfig
> index 67998dbd1d46..1bf25185c35b 100644
> --- a/drivers/cxl/Kconfig
> +++ b/drivers/cxl/Kconfig
> @@ -6,6 +6,7 @@ menuconfig CXL_BUS
>         select FW_UPLOAD
>         select PCI_DOE
>         select FIRMWARE_TABLE
> +       select NUMA_KEEP_MEMINFO if NUMA

Ok, will take a look here too. 

Thanks,

-Robert

>         help
>           CXL is a bus that is electrically compatible with PCI Express, but
>           layers three protocols on that signalling (CXL.io, CXL.cache, and