Re: [PATCH 2/3] KVM: SVM: Add host support for Enhanced SMT Protection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] MSR_AMD64_IDLE_WAKEUP_ICR is only programmed for online CPUs during module load without CPU hotplug locks, breaking ESMTP on hotplugged CPUs.
- [High] Immediate retry on ESMTP_ILLSIB and ESMTP_TIMEOUT exits with cond_resched() causes a kernel livelock and violates SRCU locking invariants.
- [Medium] Missing teardown of MSR_AMD64_IDLE_WAKEUP_ICR on KVM module unload leads to permanent spurious IPIs.
--

commit 6679aa4cd87f7de484f6cabf547ea6d6220bfceb
Author: Pratik R. Sampat <prsampat@amd.com>

KVM: SVM: Add host support for Enhanced SMT Protection

This commit introduces support for Enhanced SMT Protection (ESMTP) to protect
SEV-SNP guests from SMT side channels. It adds logic to program the host's
wakeup IPIs and handles three new non-fatal VMRUN exits (ESMTP_ILLSIB,
ESMTP_TIMEOUT, ESMTP_RETRY) by yielding the CPU.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 944aaea6501f2..9bafb671a8b7f 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
[ ... ]
> @@ -3045,6 +3059,30 @@ static const char * __init sev_str_feature_state(bool is_supported, bool is_usable)
>  	return is_supported ? is_usable ? "enabled" : "unusable" : "disabled";
>  }
>  
> +static bool sev_esmtp_setup_wakeup_icr(void)
> +{
> +	unsigned int cpu, sibling;
> +	bool wrmsr_success = true;
> +
> +	for_each_online_cpu(cpu) {
> +		for_each_cpu(sibling, topology_sibling_cpumask(cpu)) {

[Severity: High]
Could this break ESMTP on hotplugged CPUs?

This loop only touches currently online CPUs during module load and does so
without holding cpus_read_lock(). Because this is a one-time invocation
during module init, any CPU brought online after the KVM module is loaded
will lack the required MSR programming.

Hardware will then fail to wake the sibling upon CC6 idle, causing VMRUN
to indefinitely stall and trigger ESMTP_TIMEOUT exits on those hotplugged
CPUs.

> +			u64 icr;
> +			int ret;
> +
> +			if (sibling == cpu)
> +				continue;
> +
> +			icr = (u64)per_cpu(x86_cpu_to_apicid, sibling) << 32;
> +			icr |= LOCAL_TIMER_VECTOR & 0xff;
> +			ret = wrmsrq_safe_on_cpu(cpu, MSR_AMD64_IDLE_WAKEUP_ICR, icr);
> +			if (ret)
> +				wrmsr_success = false;
> +		}
> +	}
> +
> +	return wrmsr_success;
> +}
> +
>  void __init sev_hardware_setup(void)
>  {
>  	unsigned int eax, ebx, ecx, edx, sev_asid_count, sev_es_asid_count;
[ ... ]
> @@ -3221,6 +3259,10 @@ void __init sev_hardware_setup(void)
>  
>  	if (sev_snp_enabled && tsc_khz && cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
>  		sev_supported_vmsa_features |= SVM_SEV_FEAT_SECURE_TSC;
> +
> +	if (sev_snp_enabled && cpu_feature_enabled(X86_FEATURE_AMD_ESMTP) &&
> +	    sev_esmtp_setup_wakeup_icr())
> +		sev_supported_vmsa_features |= SVM_SEV_FEAT_ESMTP;

[Severity: Medium]
Will this lead to permanent spurious IPIs when the KVM module is unloaded?

MSR_AMD64_IDLE_WAKEUP_ICR is programmed globally on all online CPUs to send a
local timer IPI when a CPU enters CC6 idle. However, I don't see a teardown
path (like in sev_hardware_unsetup) restoring this MSR to zero. 

After the KVM module is unloaded, the CPUs will permanently continue sending
spurious local timer IPIs to their siblings upon idling, which could cause
host power regressions and prevent deep sleep.

>  }
>  
>  void sev_hardware_unsetup(void)
[ ... ]
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index e2be2cd86c320..0b128f7911348 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
[ ... ]
> @@ -3732,6 +3732,13 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
>  		return 0;
>  	}
>  
> +	if (svm->vmcb->control.exit_code == SVM_EXIT_ESMTP_RETRY ||
> +	    svm->vmcb->control.exit_code == SVM_EXIT_ESMTP_ILLSIB ||
> +	    svm->vmcb->control.exit_code == SVM_EXIT_ESMTP_TIMEOUT) {
> +		cond_resched();
> +		return 1;
> +	}

[Severity: High]
Does this immediate retry create a kernel livelock and violate SRCU invariants?

If the runqueue has no other tasks (TIF_NEED_RESCHED is false), cond_resched()
does nothing. The vCPU immediately attempts VMRUN again, hits the exact same
hardware conflict, and exits again. This creates a tight spin loop that can
peg the host CPU at 100% utilization.

Additionally, if a task is pending, cond_resched() yields the CPU while
holding KVM's SRCU read lock, because svm_handle_exit() is invoked within
the vcpu_enter_guest() SRCU critical section. This would stall
synchronize_srcu() indefinitely and bypass KVM's safe
xfer_to_guest_mode_handle_work() protocol.

> +
>  	if (exit_fastpath != EXIT_FASTPATH_NONE)
>  		return 1;
>

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