Re: [PATCH 2/2] x86/sev: Add per-node RMPOPT windows for >2TB systems

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] x86/sev: rmpopt_gb_to_node() misclassifies gb 0 and shifts
  Node 0 window
- [Medium] x86/sev: rmpopt_init() leaves rmpopt_node_work uninitialized
  for offline nodes
--

commit 48452fd940a44051bb4eac5ae9acc4e9431f65e6
Author: Ashish Kalra <ashish.kalra@amd.com>

x86/sev: Add per-node RMPOPT windows for >2TB systems

This commit selects a per-NUMA-node 2 TB RMPOPT_BASE window using a
sliding-window search and warms each node's window in parallel on systems
with more than 2 TB of RAM.

> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
> index a2756b18d5f7a..1ed1fba3b62dd 100644
> --- a/arch/x86/virt/svm/sev.c
> +++ b/arch/x86/virt/svm/sev.c
[ ... ]
> @@ -632,19 +638,125 @@ static void rmpopt_scan_range(void *arg)
>  		rmpopt(pa);
>  }
>  
> +/* NUMA node backing the 1 GB region at index @gb, or NUMA_NO_NODE if empty. */
> +static int rmpopt_gb_to_node(u64 gb)
> +{
> +	u64 pfn = (gb * SZ_1G) >> PAGE_SHIFT;
> +	int nid;
> +
> +	for_each_online_node(nid) {
> +		if (pfn >= node_start_pfn(nid) && pfn < node_end_pfn(nid))
> +			return nid;
> +	}
> +
> +	return NUMA_NO_NODE;
> +}

[Severity: Medium]
Does rmpopt_gb_to_node(0) always return NUMA_NO_NODE on x86?

trim_bios_range() reserves page 0 as E820_TYPE_RESERVED before memblock
setup, so min_low_pfn and node_start_pfn(0) are at least 1, while gb = 0
has pfn = 0.  On systems with more than 2 TB of RAM, gb = 0 is treated as
unpopulated with distance RMPOPT_EMPTY_DISTANCE (255), so when
rmpopt_find_best_window(0) slides base_gb from 0 to 1 it drops gb = 0
without reducing own and lowers dist, shifting Node 0's RMPOPT_BASE to at
least 1 GB and leaving [0, 1 GB) unoptimized.  Checking for range overlap
between [pfn, pfn + (SZ_1G >> PAGE_SHIFT)) and [node_start_pfn(nid),
node_end_pfn(nid)) would also handle other nodes whose start PFN is not
1 GB-aligned.

[ ... ]

> @@ -654,6 +766,14 @@ static int __init rmpopt_init(void)
>  		return -ENOMEM;
>  	}
>  
> +	rmpopt_node_work = kcalloc(nr_node_ids, sizeof(*rmpopt_node_work), GFP_KERNEL);
> +	if (!rmpopt_node_work) {
> +		destroy_workqueue(rmpopt_wq);
> +		return -ENOMEM;
> +	}
> +	for_each_online_node(nid)
> +		INIT_WORK(&rmpopt_node_work[nid], rmpopt_warmup_fn);

[Severity: Medium]
What happens if a NUMA node is offline during rmpopt_init() and is brought
online later (for example via CXL memory or CPU hotplug)?

rmpopt_init() only calls INIT_WORK() for nodes already online at init time,
leaving rmpopt_node_work[nid] zero-initialized for offline nodes.  When
do_rmpopt_work() later iterates for_each_online_node(nid) on a >2TB system,
flush_work(&rmpopt_node_work[nid]) hits WARN_ON(!work->func) in
__flush_work(), and schedule_work_on() queues an uninitialized work_struct
if the node also has online CPUs.  Initializing across
for_each_possible_node(nid) avoids this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793921823.git.ashish.kalra@amd.com?part=2