KVM: nSVM: Fix RIP usage in the control area after restore

arch/x86/kvm/svm/svm.c +8 -0

index 8f8bc863e2143..e084b9688f556 100644
@@ -1413,6 +1413,24 @@ static void svm_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
                sd->bp_spec_reduce_set = true;
                msr_set_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
        }
+
+       /*
+        * If nrips is supported in hardware but not exposed to L1, stuff the
+        * actual L2 RIP to emulate what a nrips=0 CPU would do (L1 is
+        * responsible for advancing RIP prior to injecting the event). Once L2
+        * runs after L1 executes VMRUN, NextRIP is updated by the CPU and/or
+        * KVM, and this is no longer needed.
+        *

Quoted 16 lines

arch/x86/kvm/svm/svm.c +8 -0

index 8f8bc863e2143..e084b9688f556 100644
@@ -1413,6 +1413,24 @@ static void svm_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
                sd->bp_spec_reduce_set = true;
                msr_set_bit(MSR_ZEN4_BP_CFG, MSR_ZEN4_BP_CFG_BP_SPEC_REDUCE_BIT);
        }
+
+       /*
+        * If nrips is supported in hardware but not exposed to L1, stuff the
+        * actual L2 RIP to emulate what a nrips=0 CPU would do (L1 is
+        * responsible for advancing RIP prior to injecting the event). Once L2
+        * runs after L1 executes VMRUN, NextRIP is updated by the CPU and/or
+        * KVM, and this is no longer needed.
+        *

arch/x86/kvm/svm/svm.c static void svm_prepare_switch_to_guest(struct kvm_vcpu *vcpu) · line 1433

Quoted 10 lines

arch/x86/kvm/svm/svm.c +10 -0

+        * This is done here (as opposed to when preparing vmcb02) to use the
+        * most up-to-date value of RIP regardless of the order of restoring
+        * registers and nested state in the vCPU save+restore path.
+        */
+       if (is_guest_mode(vcpu) && svm->nested.nested_run_pending) {
+               if (boot_cpu_has(X86_FEATURE_NRIPS) &&
+                   !guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS))
+                       svm->vmcb->control.next_rip = kvm_rip_read(vcpu);
+       }
+

Doing this in svm_prepare_switch_to_guest() is wrong, or at least after the svm->guest_state_loaded check. It's possible to emulate the nested VMRUN without doing a vcpu_put(), which means svm->guest_state_loaded will remain true and this code will be skipped.

In fact, this breaks the svm_nested_soft_inject_test test. Funny enough, I was only running it with my repro changes, which papered over the bug because it forced an exit to userspace after VMRUN due to single-stepping, so svm->guest_state_loaded got cleared and the code was executed on the next KVM_RUN, before L2 runs.

I can move it above the svm->guest_state_loaded check, but I think I will just put it in pre_svm_run() instead.

Quoted 12 lines

Doing this in svm_prepare_switch_to_guest() is wrong, or at least after the svm->guest_state_loaded check. It's possible to emulate the nested VMRUN without doing a vcpu_put(), which means svm->guest_state_loaded will remain true and this code will be skipped.

In fact, this breaks the svm_nested_soft_inject_test test. Funny enough, I was only running it with my repro changes, which papered over the bug because it forced an exit to userspace after VMRUN due to single-stepping, so svm->guest_state_loaded got cleared and the code was executed on the next KVM_RUN, before L2 runs.

Quoted 2 lines

I can move it above the svm->guest_state_loaded check, but I think I will just put it in pre_svm_run() instead.

I would rather not expand pre_svm_run(), and instead just open code it in svm_vcpu_run(). pre_svm_run() probably should never have been added, because it's far from a generic "pre run" API. E.g. if we want to keep the helper around, it should probably be named something something ASID.

Quoted 15 lines

Quoted 14 lines

Doing this in svm_prepare_switch_to_guest() is wrong, or at least after the svm->guest_state_loaded check. It's possible to emulate the nested VMRUN without doing a vcpu_put(), which means svm->guest_state_loaded will remain true and this code will be skipped.

In fact, this breaks the svm_nested_soft_inject_test test. Funny enough, I was only running it with my repro changes, which papered over the bug because it forced an exit to userspace after VMRUN due to single-stepping, so svm->guest_state_loaded got cleared and the code was executed on the next KVM_RUN, before L2 runs.

I can move it above the svm->guest_state_loaded check, but I think I will just put it in pre_svm_run() instead.

Quoted 4 lines

I would rather not expand pre_svm_run(), and instead just open code it in svm_vcpu_run(). pre_svm_run() probably should never have been added, because it's far from a generic "pre run" API. E.g. if we want to keep the helper around, it should probably be named something something ASID.

I sent a new version before I saw your response.. sorry.

How strongly do you feel about this? :P

Quoted 20 lines · Yosry Ahmed

Quoted 19 lines

Quoted 6 lines

Doing this in svm_prepare_switch_to_guest() is wrong, or at least after the svm->guest_state_loaded check. It's possible to emulate the nested VMRUN without doing a vcpu_put(), which means svm->guest_state_loaded will remain true and this code will be skipped.

Quoted 8 lines

In fact, this breaks the svm_nested_soft_inject_test test. Funny enough, I was only running it with my repro changes, which papered over the bug because it forced an exit to userspace after VMRUN due to single-stepping, so svm->guest_state_loaded got cleared and the code was executed on the next KVM_RUN, before L2 runs.

I can move it above the svm->guest_state_loaded check, but I think I will just put it in pre_svm_run() instead.

I would rather not expand pre_svm_run(), and instead just open code it in svm_vcpu_run(). pre_svm_run() probably should never have been added, because it's far from a generic "pre run" API. E.g. if we want to keep the helper around, it should probably be named something something ASID.

Quoted 3 lines

I sent a new version before I saw your response.. sorry.

How strongly do you feel about this? :P

Strong enough that I'll fix it up when applying, unless it's a sticking point on your end.

E.g. one thing that I don't love is that the code never runs for SEV guests. Which is fine, because in practice I can't imagine KVM ever supporting nested SVM for SEV, but it adds unnecessary cognitive load, because readers need to reason through why the code only applies to !SEV guests.

Quoted 26 lines · Sean Christopherson

Quoted 23 lines · Yosry Ahmed

Quoted 12 lines

Quoted 12 lines

Doing this in svm_prepare_switch_to_guest() is wrong, or at least after the svm->guest_state_loaded check. It's possible to emulate the nested VMRUN without doing a vcpu_put(), which means svm->guest_state_loaded will remain true and this code will be skipped.

In fact, this breaks the svm_nested_soft_inject_test test. Funny enough, I was only running it with my repro changes, which papered over the bug because it forced an exit to userspace after VMRUN due to single-stepping, so svm->guest_state_loaded got cleared and the code was executed on the next KVM_RUN, before L2 runs.

Quoted 7 lines

Quoted 2 lines

I can move it above the svm->guest_state_loaded check, but I think I will just put it in pre_svm_run() instead.

I would rather not expand pre_svm_run(), and instead just open code it in svm_vcpu_run(). pre_svm_run() probably should never have been added, because it's far from a generic "pre run" API. E.g. if we want to keep the helper around, it should probably be named something something ASID.

I sent a new version before I saw your response.. sorry.

How strongly do you feel about this? :P

Quoted 2 lines

Strong enough that I'll fix it up when applying, unless it's a sticking point on your end.

It's just that 99% of the time someone is reading svm_vcpu_run(), they won't care about this code, and it's also cognitive load to filter it out. We can add a helper for this code (and the soft IRQ inject), something like svm_fixup_nested_rips() or sth.

We discussed a helper before and you didn't like it, but that was in a different context (a helper that combined normal and special cases). WDYT?

Quoted 3 lines

We discussed a helper before and you didn't like it, but that was in a different context (a helper that combined normal and special cases). WDYT?

A helper would work. svm_fixup_nested_rips() is good, the only flaw is the CS.base chunk, but I'm not sure I care enough about 32-bit to reject the name just because of that :-)

That would make it easier to reduce indentation, e.g.

static void svm_fixup_nested_rips(struct kvm_vcpu *vcpu)

{
        struct vcpu_svm *svm = to_svm(vcpu);

        /*
         * If nrips is supported in hardware but not exposed to L1, stuff the
         * actual L2 RIP to emulate what a nrips=0 CPU would do (L1 is
         * responsible for advancing RIP prior to injecting the event). Once L2
         * runs after L1 executes VMRUN, NextRIP is updated by the CPU and/or
         * KVM, and this is no longer needed.
         *
         * This is done here (as opposed to when preparing vmcb02) to use the
         * most up-to-date value of RIP regardless of the order of restoring
         * registers and nested state in the vCPU save+restore path.
         *
         * Simiarly, initialize svm->soft_int_* fields here to use the most
         * up-to-date values of RIP and CS base, regardless of restore order.
         */
        if (!is_guest_mode(vcpu) || !svm->nested.nested_run_pending)
                return;

        if (boot_cpu_has(X86_FEATURE_NRIPS) &&
            !guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS))
                svm->vmcb->control.next_rip = kvm_rip_read(vcpu);

        if (svm->soft_int_injected) {
                svm->soft_int_csbase = svm->vmcb->save.cs.base;
                svm->soft_int_old_rip = kvm_rip_read(vcpu);
                if (!guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS))
                        svm->soft_int_next_rip = kvm_rip_read(vcpu);
        }
}