Re: [PATCH 7/7] KVM: SVM: Enable save/restore of FRED MSRs

[Shivansh Dhiman <shivansh.dhiman@amd.com>]

On 07-03-26 07:44, Sean Christopherson wrote:
> On Thu, Jan 29, 2026, Shivansh Dhiman wrote:
>> Set the FRED_VIRT_ENABLE bit (bit 4) in the VIRT_EXT field of VMCB to enable
>> FRED Virtualization for the guest. This enables automatic save/restore of
>> FRED MSRs. Also toggle this bit when setting CPUIDs, to support booting of
>> secure guests.
>>
>> Signed-off-by: Shivansh Dhiman <shivansh.dhiman@amd.com>
>> ---
>>  arch/x86/kvm/svm/svm.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 954df4eae90e..24579c149937 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -1144,6 +1144,9 @@ static void init_vmcb(struct kvm_vcpu *vcpu, bool init_event)
>>  	save->fred_ssp3 = 0;
>>  	save->fred_config = 0;
>>  
>> +	if (guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
>> +		svm->vmcb->control.virt_ext |= FRED_VIRT_ENABLE_MASK;
> 
> This is completely unnecessary, no?  CPUID is empty at vCPU creation and so FRED
> _can't_ be enabled before going through svm_vcpu_after_set_cpuid().

Hi Sean,

You're right for vCPU creation, CPUID is empty there and the hunk does
nothing, so I dropped it in v2. While preparing v3 I hit a case that does
need it though, a triple fault:

 1. The guest enumerates FRED, so the FRED MSRs are passed through and
    FRED_VIRT_ENABLE is set in virt_ext.

 2. The guest triple faults and KVM intercepts SHUTDOWN.

 3. shutdown_interception() does clear_page(svm->vmcb), which wipes
    virt_ext along with the rest of the VMCB, and then INITs the vCPU.

 4. init_vmcb() runs, but svm_vcpu_after_set_cpuid() does not.

 5. CPUID still enumerates FRED, so the intercept recalc puts the FRED
    MSRs back into passthrough.

 6. Nothing restores FRED_VIRT_ENABLE, as only svm_vcpu_after_set_cpuid()
    ever sets it.


So the guest ends up with direct access to the FRED MSRs while hardware is
no longer context switching them, i.e. it can clobber the host's FRED state.

Setting the bit in init_vmcb() is the smallest fix I came up with, so I'd
like to add the hunk back in v3.

Would you prefer it handled in svm_recalc_fred_msr_intercepts() instead
while setting intercepts? Or is there a better way to deal with this?

Regards,
Shivansh

> 
>>  	init_sys_seg(&save->ldtr, SEG_TYPE_LDT);
>>  	init_sys_seg(&save->tr, SEG_TYPE_BUSY_TSS16);
>>  
>> @@ -4529,6 +4532,9 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>  	if (guest_cpuid_is_intel_compatible(vcpu))
>>  		guest_cpu_cap_clear(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD);
>>  
>> +	if (guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
>> +		svm->vmcb->control.virt_ext |= FRED_VIRT_ENABLE_MASK;
> 
> The flag needs to be cleared if FRED isn't supported, because KVM's wonderful
> ABI allows userspace to modify CPUID however many times it wants before running
> the vCPU.
> 
>> +
>>  	if (sev_guest(vcpu->kvm))
>>  		sev_vcpu_after_set_cpuid(svm);
>>  }
>> -- 
>> 2.43.0
>>