Re: [Patch v2 2/4] KVM: SVM: Add host support for Enhanced SMT Protection

[Tom Lendacky <thomas.lendacky@amd.com>]

On 10/9/26 14:40, Pratik R. Sampat wrote:
> Hi Tom,
> 
> On 10/9/26 3:05 PM, Tom Lendacky wrote:
>> On 10/6/26 11:54, Pratik R. Sampat wrote:
>>> Enhanced SMT Protection (ESMTP) protects an SEV-SNP guest from SMT side
>>> channels by requiring that, while a vCPU is in guest mode, its SMT sibling
>>> thread is either idle in host mode or executing a vCPU the guest has
>>> declared to be a legal sibling. A legal sibling is another ESMTP vCPU of
>>> the same guest that carries the same VCPU_SIBLING_MASK and agrees on
>>> VCPU_ID outside that mask.
>>>
>>> Hardware enforces this at VMRUN, which no longer enters guest mode right
>>> away but stalls until the condition holds. Three new exits report that it
>>> could not be met, all of them non-fatal:
>>>
>>> ESMTP_ILLSIB: a sibling thread is running a vCPU that is not a legal
>>> sibling of this one.
>>>
>>> ESMTP_TIMEOUT: the stall exceeded the timeout programmed in the VMCB. The
>>> timer bounds how long VMRUN waits for the core to settle. A module
>>> parameter exposes an interface to program this timer.
>>>
>>> ESMTP_RETRY: If DBREQ is detected, VMRUN exits. ESMTP_RETRY indicates an
>>> internal event and the hypervisor should execute a VMRUN.
>>>
>>> Refer to the AMD64 APM Vol 2, section "AMD64 Enhanced SMT Protection".
>>>
>>> Acked-by: Borislav Petkov (AMD) <bp@alien8.de> # non-virtualization changes
>>> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
>>> ---
>>> v1..v2
>>> * Move idle wakeup ICR programming to svm_enable_virtualization_cpu()
>>>   instead of sev_hardware_setup()
>>> * Remove cond_sched() in ESMTP exit paths
>>> * Program the IDLE_WAKEUP_ICR for all present CPU siblings
>>> * Add documentation for ESMTP timeout
>>> ---
>>>  .../admin-guide/kernel-parameters.txt         | 19 ++++++++
>>>  arch/x86/include/asm/cpufeatures.h            |  1 +
>>>  arch/x86/include/asm/msr-index.h              |  1 +
>>>  arch/x86/include/asm/svm.h                    | 12 ++++-
>>>  arch/x86/include/uapi/asm/svm.h               |  9 +++-
>>>  arch/x86/kernel/cpu/scattered.c               |  1 +
>>>  arch/x86/kvm/svm/sev.c                        | 48 ++++++++++++++++++-
>>>  arch/x86/kvm/svm/svm.c                        |  8 ++++
>>>  arch/x86/kvm/svm/svm.h                        |  2 +
>>>  9 files changed, 97 insertions(+), 4 deletions(-)
>>>

>>> --- a/arch/x86/kvm/svm/sev.c
>>> +++ b/arch/x86/kvm/svm/sev.c
>>> @@ -67,6 +67,11 @@ module_param_named(sev_snp, sev_snp_enabled, bool, 0444);
>>>  static unsigned int __ro_after_init nr_ciphertext_hiding_asids;
>>>  module_param_named(ciphertext_hiding_asids, nr_ciphertext_hiding_asids, uint, 0444);
>>>  
>>> +static unsigned long esmtp_timeout_ns;
>>> +module_param(esmtp_timeout_ns, ulong, 0644);
>>> +MODULE_PARM_DESC(esmtp_timeout_ns,
>>> +		 "ESMTP VMRUN sibling-wait timeout in nanoseconds (0 disables the timer)");
>>
>> "ESMTP VMRUN sibling-wait timeout, in nanoseconds. 0 (default) disables
>> the timer."
>>
>> Not sure if that reads any better, main point is just to highlight the
>> default is 0, timer disabled.
>>
> 
> Sure, your example reads fine. As an alternative:
> "ESMTP VMRUN sibling-wait timeout in nanoseconds (default: 0, timer disabled)"

Ack

> 
>>> +

>>> +void sev_esmtp_enable_wakeup_icr(void)
>>> +{
>>> +	unsigned int cpu, sibling, shift;
>>> +	u32 core;
>>> +
>>> +	if (!(sev_supported_vmsa_features & SVM_SEV_FEAT_ESMTP))
>>> +		return;
>>> +
>>> +	cpu = raw_smp_processor_id();
>>> +	shift = topology_get_domain_shift(TOPO_CORE_DOMAIN);
>>> +	core = per_cpu(x86_cpu_to_apicid, cpu) >> shift;
>>> +
>>> +	for_each_present_cpu(sibling) {
>>> +		u32 apicid = per_cpu(x86_cpu_to_apicid, sibling);
>>> +		u64 icr;
>>> +
>>> +		if (sibling == cpu || (apicid >> shift) != core)
>>> +			continue;
>>> +
>>> +		icr = (u64)apicid << 32;
>>> +		icr |= LOCAL_TIMER_VECTOR;
>>> +		WARN_ON_ONCE(wrmsrq_safe(MSR_AMD64_IDLE_WAKEUP_ICR, icr));
>>> +	}
>>
>> Can you use cpu_sibling_map here instead of going through every present cpu?
>>
> 
> I think cpu_sibling_map only covers the list of online CPUs. If SMT was say
> turned off and then on again the CPU that stayed online would never be
> reprogrammed with its new sibling's APIC ID. Walking the present CPUs avoids
> that, because x86_cpu_to_apicid is already valid for offline siblings.

Makes sense. I know we disable hotplug when SNP is enabled, but this runs
before that occurs.

Thanks,
Tom

> 
> I had used topology_sibling_cpumask() in v1 when Sashiko pointed out this case
> to me.
> 
> Thank you!
> Pratik
>