Re: [RFC PATCH 12/12] drivers: hv: hv_vsm_boot: Boot secondary processors in VTL1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The boot CPU is incorrectly included in the `cpu_online_mask` passed to VTL1.
- [High] AP VTL1 boot failures are silently ignored, leaving the system in a partially-booted secure state.
- [High] Unsafe usage of `smp_processor_id()` in preemptible context and task migration race.
- [Medium] Unprotected snapshot and iteration of `cpu_online_mask`.
--

commit 99445d1f4eda186e7be403a5b296f1c80a9cdae6
Author: Thara Gopinath <tgopinath@linux.microsoft.com>

drivers: hv: hv_vsm_boot: Boot secondary processors in VTL1

This commit adds support to boot secondary processors into VTL1 after the
primary CPU has transitioned. It passes an online CPU mask to VTL1 via a
shared page and uses a CPU-bound kthread to sequentially issue a vtlcall
for each AP.

> diff --git a/drivers/hv/hv_vsm_boot.c b/drivers/hv/hv_vsm_boot.c
> index c4f15c42df1f6..ad161b2f56653 100644
> --- a/drivers/hv/hv_vsm_boot.c
> +++ b/drivers/hv/hv_vsm_boot.c
[ ... ]
> +static int __init hv_vsm_boot_sec_vp_thread_fn(void *arg)
> +{
> +	struct hv_vsm_ap_boot_ctx *ctx = arg;
> +	struct hv_vtlcall_param args = {0};
> +	int cpu = smp_processor_id();
> +	u16 vp_enabled_vtl_set = 0;
> +	u8 active_mbec_enabled = 0;
> +	s64 sk_status;
> +	int ret = 0;
> +
> +	pr_info("cpu%d entering vtl1 boot thread\n", cpu);
> +	sk_status = hv_vsm_vtlcall(&args);
> +	if (sk_status)
> +		pr_warn("VP%d VTL1 boot returned status %lld\n", cpu, sk_status);
> +
> +	ret = hv_vsm_get_vp_status(&vp_enabled_vtl_set, &active_mbec_enabled);
> +	if (ret)
> +		goto out;
> +
> +	if (!active_mbec_enabled) {
> +		pr_err("Failed to enable MBEC for VP%d\n", cpu);
> +		hv_vsm_mbec_enabled = false;
> +	}
> +out:
> +	ctx->ret = ret;

[Severity: High]
If hv_vsm_vtlcall() fails and returns an error in sk_status, will this
error be silently ignored? The warning is printed, but ret remains 0 before
calling hv_vsm_get_vp_status(). If hv_vsm_get_vp_status() succeeds, ret
is unconditionally set to 0. Does this leave the caller believing the AP
booted successfully instead of surfacing the error?

> +	complete(&ctx->done);
> +	return 0;
> +}
[ ... ]
> +static int __init hv_vsm_boot_ap_vtl(void)
> +{
> +	struct hv_vtlcall_param args = {0};
> +	struct page *cpu_online_page;
> +	unsigned int cpu, cur_cpu = smp_processor_id();

[Severity: High]
Is it safe to use smp_processor_id() here in a preemptible context?
Fetching it here triggers a kernel BUG warning. Also, because the thread can
migrate between CPU evaluation and the sleepable operations like alloc_page()
and wait_for_completion() below, could cur_cpu become stale? This might
cause the AP loop to skip the new CPU and incorrectly attempt to boot the
original primary CPU as an AP.

> +	s64 sk_status;
> +	int ret;
> +
> +	cpu_online_page = hv_vsm_alloc_shared_page();
> +	if (IS_ERR(cpu_online_page))
> +		return PTR_ERR(cpu_online_page);
> +
> +	cpumask_copy(page_address(cpu_online_page), cpu_online_mask);

[Severity: Medium]
Should the snapshot of cpu_online_mask and subsequent iteration
be protected by cpus_read_lock()? Accessing this without hotplug
protection could allow concurrent CPU hotplug events to alter the
mask, potentially passing an inconsistent snapshot to VTL1 or binding
an AP boot kthread to a newly offlined CPU.

[Severity: High]
Does this verbatim copy of cpu_online_mask incorrectly include the
already-booted primary CPU in the mask passed to VTL1? If cur_cpu isn't
cleared from the mask first, could VTL1 mistakenly expect the primary CPU to
issue an AP boot vtlcall and hang or corrupt the primary CPU's secure
context?

> +
> +	/*
> +	 * Hand VTL1 the set of APs to expect. VTL1 copies the mask
> +	 * synchronously inside this vtlcall and does not reference the
> +	 * page after it returns, so freeing it here is safe.
> +	 */
> +	args.a0 = VSM_VTL_CALL_FUNC_ID_BOOT_APS;
> +	args.a1 = page_to_pfn(cpu_online_page);
> +	sk_status = hv_vsm_vtlcall(&args);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901165647.3160413-1-tgopinath@linux.microsoft.com?part=12