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

[Shivansh Dhiman]

On 13-08-26 04:57, Sean Christopherson wrote:
> +Nikunj
> 
> On Mon, Aug 10, 2026, Shivansh Dhiman wrote:
>> 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@xxxxxxx>
>>>> ---
>>>>  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?
> 
> Take a hard dependency on an upcoming APM update that states the control area
> is valid after shutdown, and rework KVM to not clobber control fields on shutdown
> interception.

Sure, Sean. I'm thinking something like the diff below. It contains three
parts:

1. Clear the save area and offsets 60h, 61h, and 68h of the control area,
   as per the APM.

2. Zero the intercepts as well. The APM doesn't require it, but it seemed
   worthwhile so that the new intercepts are computed on a clean slate
   rather than on top of whatever survived the triple fault. Is that worth
   doing?

3. For nested, same fields are copied to vmcb12 when a shutdown is intercepted
   in L2. Clear them while copying so L1 gets a deterministic value
   rather than whatever the hardware left behind. Does that seem like right
   thing to do?


diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 83de3456df708..38843b29b21ec 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1149,6 +1149,11 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
        vmcb12->control.event_inj         = svm->nested.ctl.event_inj;
        vmcb12->control.event_inj_err     = svm->nested.ctl.event_inj_err;

+       if (vmcb02->control.exit_code == SVM_EXIT_SHUTDOWN) {
+               vmcb12->control.int_ctl &= ~GENMASK(15, 0);
+               vmcb12->control.int_state = 0;
+       }
+
        if (!kvm_pause_in_guest(vcpu->kvm)) {
                vmcb01->control.pause_filter_count = vmcb02->control.pause_filter_count;
                vmcb_mark_dirty(vmcb01, VMCB_INTERCEPTS);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 519249e75420d..62b4feb9c3909 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2090,7 +2090,11 @@ static int shutdown_interception(struct kvm_vcpu *vcpu)
         * cannot be reinitialized, i.e. synthesizing INIT is futile.
         */
        if (!sev_es_guest(vcpu->kvm)) {
-               clear_page(svm->vmcb);
+               struct vmcb_control_area *control = &svm->vmcb->control;
+               memset(&svm->vmcb->save, 0, sizeof(svm->vmcb->save));
+               memset(control->intercepts, 0, sizeof(control->intercepts));
+               control->int_ctl &= ~GENMASK(15, 0);
+               control->int_state = 0;
 #ifdef CONFIG_KVM_SMM
                if (is_smm(vcpu))
                        kvm_smm_changed(vcpu, false);


Cheers,
Shivansh