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

["Pratik R. Sampat" <prsampat@amd.com>]

On 9/14/26 1:13 PM, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> 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.


Having is in the svm_enable_virtualization_cpu() probably makes sense.

> 
>> +                     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.
> 

Described this way is likely not possible. If the KVM module is unloaded, an
ESMTP enabled guest cannot be spawned. The ICR only fires when a VMEXIT routine
of a guest with ESMTP enabled, when a sibling thread is still executing guest
code or still trying to complete a VMRUN.

I could clear the MSR in svm_disable_virtualization_cpu() but I don't think
that is needed.

>>  }
>>
>>  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.
> 

Right, the retry can spin, but mainly only for ILLSIB. This also seems more
likely to occur when vCPUs are pinned. RETRY and TIMEOUT are transient by
construction.

Having said that, I was toying with the idea of having bounded number of
consecutive retries and that would fix it entirely.

> 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.
> 

Yeah, cond_resched() seems like the wrong thing to do here. Although probably
for a different reason. It consumes NEED_RESCHED with kvm->srcu held, so the 
outer unlock-then-schedule() path is skipped.  It is a worse preemption point 
than the one KVM already has. I will drop it and plain return 1 instead.


>> +
>>       if (exit_fastpath != EXIT_FASTPATH_NONE)
>>               return 1;
>>
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789399214.git.prsampat@amd.com?part=2