KVM: nSVM: Save/restore fixes for (Next)RIP
Subsystems (5)
v3 cover KVM: nSVM: Save/restore fixes for (Next)RIP
This is a combined v3 of series [1] and v2 of series [2], as patch 1 here is a dependency of patch 5. Without patch 1, NextRIP is not sync'd correctly to the cache, and restoring it for a guest without NRIPS is a bug.
The series fixes two classes of save/restore bugs:
- Some fields written by the CPU are not sync'd from vmcb02 to cached vmcb12 after VMRUN, so are not up-to-date in KVM_GET_NESTED_STATE payload (fixes in patches 1 & 2, tests in patches 3 & 4).
- Ordering between KVM_SET_NESTED_STATE and KVM_SET_{S}REGS could cause vmcb02 to be incorrectly initialized after save+restore (fixes in patches 5 to 7).
Patch 8 is a reproducer for the second class of bugs, it should not be merged.
v2 -> v3 (for series [1]):
- Dropped patch moving vmcb02->vmcb12 sync after completing interrupts.
v1 -> v2 (for series [2]):
- Move code updating NextRIP and soft IRQ RIP tracking from svm_prepare_switch_to_guest() to pre_svm_run().
[1]https://lore.kernel.org/kvm/[email protected]/ [2]https://lore.kernel.org/kvm/[email protected]/
Yosry Ahmed (8):
KVM: nSVM: Sync NextRIP to cached vmcb12 after VMRUN of L2
KVM: nSVM: Sync interrupt shadow to cached vmcb12 after VMRUN of L2
KVM: selftests: Extend state_test to check vGIF
KVM: selftests: Extend state_test to check next_rip
KVM: nSVM: Always use NextRIP as vmcb02's NextRIP after first L2 VMRUN
KVM: nSVM: Delay stuffing L2's current RIP into NextRIP until vCPU run
KVM: nSVM: Delay setting soft IRQ RIP tracking fields until vCPU run DO NOT MERGE: KVM: selftests: Reproduce nested RIP restore bug
arch/x86/kvm/svm/nested.c | 36 +++++---- arch/x86/kvm/svm/svm.c | 37 ++++++++++ .../testing/selftests/kvm/lib/x86/processor.c | 8 +- tools/testing/selftests/kvm/x86/state_test.c | 35 +++++++++ .../kvm/x86/svm_nested_soft_inject_test.c | 74 +++++++++++++++---- 5 files changed, 154 insertions(+), 36 deletions(-)
base-commit: 183bb0ce8c77b0fd1fb25874112bc8751a461e49
Signature · 1 line
2.53.0.414.gf7e9f6c205-goog
PATCH v3 1/8 KVM: nSVM: Sync NextRIP to cached vmcb12 after VMRUN of L2
After VMRUN in guest mode, nested_sync_control_from_vmcb02() syncs fields written by the CPU from vmcb02 to the cached vmcb12. This is because the cached vmcb12 is used as the authoritative copy of some of the controls, and is the payload when saving/restoring nested state.
NextRIP is also written by the CPU (in some cases) after VMRUN, but is not sync'd to the cached vmcb12. As a result, it is corrupted after save/restore (replaced by the original value written by L1 on nested VMRUN). This could cause problems for both KVM (e.g. when injecting a soft IRQ) or L1 (e.g. when using NextRIP to advance RIP after emulating an instruction).
Fix this by sync'ing NextRIP to the cache after VMRUN of L2, but only after completing interrupts (not in nested_sync_control_from_vmcb02()), as KVM may update NextRIP (e.g. when re-injecting a soft IRQ).
--- arch/x86/kvm/svm/svm.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
arch/x86/kvm/svm/svm.c +10 -0
@@ -4435,6 +4435,16 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)svm_complete_interrupts (vcpu); +/* +* Update the cache after completing interrupts to get an accurate +* NextRIP, e.g. when re-injecting a soft interrupt. +* +* FIXME: Rework svm_get_nested_state() to not pull data from the +* cache (except for maybe int_ctl). +*/ +if (is_guest_mode (vcpu)) + svm->nested.ctl.next_rip = svm->vmcb->control.next_rip; +return svm_exit_handlers_fastpath (vcpu); }
Signature · 1 line
2.53.0.414.gf7e9f6c205-goog
PATCH v3 2/8 KVM: nSVM: Sync interrupt shadow to cached vmcb12 after VMRUN of L2
In reply to Yosry AhmedAfter VMRUN in guest mode, nested_sync_control_from_vmcb02() syncs fields written by the CPU from vmcb02 to the cached vmcb12. This is because the cached vmcb12 is used as the authoritative copy of some of the controls, and is the payload when saving/restoring nested state.
int_state is also written by the CPU, specifically bit 0 (i.e. SVM_INTERRUPT_SHADOW_MASK) for nested VMs, but it is not sync'd to cached vmcb12. This does not cause a problem if KVM_SET_NESTED_STATE preceeds KVM_SET_VCPU_EVENTS in the restore path, as an interrupt shadow would be correctly restored to vmcb02 (KVM_SET_VCPU_EVENTS overwrites what KVM_SET_NESTED_STATE restored in int_state).
However, if KVM_SET_VCPU_EVENTS preceeds KVM_SET_NESTED_STATE, an interrupt shadow would be restored into vmcb01 instead of vmcb02. This would mostly be benign for L1 (delays an interrupt), but not for L2. For L2, the vCPU could hang (e.g. if a wakeup interrupt is delivered before a HLT that should have been in an interrupt shadow).
Sync int_state to the cached vmcb12 in nested_sync_control_from_vmcb02() to avoid this problem. With that, KVM_SET_NESTED_STATE restores the correct interrupt shadow state, and if KVM_SET_VCPU_EVENTS follows it would overwrite it with the same value.
--- arch/x86/kvm/svm/nested.c | 1 + 1 file changed, 1 insertion(+)
arch/x86/kvm/svm/nested.c +1 -0
@@ -521,6 +521,7 @@ void nested_sync_control_from_vmcb02(struct vcpu_svm *svm)u32 mask; svm->nested.ctl.event_inj = svm->vmcb->control.event_inj; svm->nested.ctl.event_inj_err = svm->vmcb->control.event_inj_err; + svm->nested.ctl.int_state = svm->vmcb->control.int_state;/* Only a few fields of int_ctl are written by the processor. */ mask =V_IRQ_MASK |V_TPR_MASK ;
Signature · 1 line
2.53.0.414.gf7e9f6c205-goog
Re: [PATCH v3 2/8] KVM: nSVM: Sync interrupt shadow to cached vmcb12 after VMRUN of L2
arch/x86/kvm/svm/nested.c void nested_sync_control_from_vmcb02(struct vcpu_svm *svm) · line 524
Quoted 9 lines
arch/x86/kvm/svm/nested.c +1
-0@@ -521,6 +521,7 @@ void nested_sync_control_from_vmcb02(struct vcpu_svm *svm)u32 mask; svm->nested.ctl.event_inj = svm->vmcb->control.event_inj; svm->nested.ctl.event_inj_err = svm->vmcb->control.event_inj_err; + svm->nested.ctl.int_state = svm->vmcb->control.int_state;
FWIW, this is an incomplete fix. KVM might update the interrupt shadow after this point through __svm_skip_emulated_instruction(), and that won't be captured in svm->nested.ctl.int_state.
I think it's not worth fixing that case too, and any further effort should go toward teaching KVM_GET_NESTED_STATE to pull state from the correct place as discussed earlier.
Sean Christopherson
maintainerRe: [PATCH v3 2/8] KVM: nSVM: Sync interrupt shadow to cached vmcb12 after VMRUN of L2
Quoted 14 lines · Yosry Ahmed
arch/x86/kvm/svm/nested.c void nested_sync_control_from_vmcb02(struct vcpu_svm *svm) · line 524
Quoted 9 lines
arch/x86/kvm/svm/nested.c +1
-0@@ -521,6 +521,7 @@ void nested_sync_control_from_vmcb02(struct vcpu_svm *svm)u32 mask; svm->nested.ctl.event_inj = svm->vmcb->control.event_inj; svm->nested.ctl.event_inj_err = svm->vmcb->control.event_inj_err; + svm->nested.ctl.int_state = svm->vmcb->control.int_state;FWIW, this is an incomplete fix. KVM might update the interrupt shadow after this point through
__svm_skip_emulated_instruction(), and that won't be captured insvm->nested.ctl.int_state.
Quoted 3 lines
I think it's not worth fixing that case too, and any further effort should go toward teaching
KVM_GET_NESTED_STATEto pull state from the correct place as discussed earlier.
+1. FWIW, AMD doesn't have a MOV/POP SS shadow, so practically speaking the only impact is that an STI shadow could get extended for one extra instruction. Unless the guest is doing e.g. "sti; hlt; cli", that's a non-issue.
PATCH v3 3/8 KVM: selftests: Extend state_test to check vGIF
In reply to Yosry AhmedV_GIF_MASK is one of the fields written by the CPU after VMRUN, and sync'd by KVM from vmcb02 to cached vmcb12 after running L2. Part of the reason is to make sure V_GIF_MASK is saved/restored correctly, as the cached vmcb12 is the payload of nested state.
Verify that V_GIF_MASK is saved/restored correctly in state_test by enabling vGIF in vmcb12, toggling GIF in L2 at different GUEST_SYNC() points, and verifying that V_GIF_MASK is correctly propagated to the nested state.
--- tools/testing/selftests/kvm/x86/state_test.c | 24 ++++++++++++++++++++ 1 file changed, 24 insertions(+)
tools/testing/selftests/kvm/x86/state_test.c +24 -0
@@ -26,7 +26,9 @@ void svm_l2_guest_code(void)GUEST_SYNC (4 );/* Exit to L1 */ vmcall (); +clgi ();GUEST_SYNC (6 ); +stgi ();/* Done, exit to L1 and never come back. */ vmcall (); }
@@ -41,6 +43,8 @@ static void svm_l1_guest_code(struct svm_test_data *svm)generic_svm_setup (svm, svm_l2_guest_code, &l2_guest_stack[L2_GUEST_STACK_SIZE ]); + vmcb->control.int_ctl |= (V_GIF_ENABLE_MASK |V_GIF_MASK ); +GUEST_SYNC (3 );run_guest (vmcb, svm->vmcb_gpa);GUEST_ASSERT (vmcb->control.exit_code ==SVM_EXIT_VMMCALL );
@@ -222,6 +226,24 @@ static void __attribute__((__flatten__)) guest_code(void *arg)GUEST_DONE (); } +void svm_check_nested_state (int stage,struct kvm_x86_state *state) +{ +struct vmcb *vmcb = (struct vmcb *)state->nested.data.svm; + +if (kvm_cpu_has (X86_FEATURE_VGIF )) { +if (stage ==4 ) +TEST_ASSERT_EQ (!!(vmcb->control.int_ctl &V_GIF_MASK ),1 ); +if (stage ==6 ) +TEST_ASSERT_EQ (!!(vmcb->control.int_ctl &V_GIF_MASK ),0 ); + } +} + +void check_nested_state (int stage,struct kvm_x86_state *state) +{ +if (kvm_has_cap (KVM_CAP_NESTED_STATE ) &&kvm_cpu_has (X86_FEATURE_SVM )) +svm_check_nested_state (stage, state); +} +int main (int argc,char *argv[]) {uint64_t *xstate_bv, saved_xstate_bv;
@@ -278,6 +300,8 @@ int main(int argc, char *argv[])kvm_vm_release (vm); +check_nested_state (stage, state); +/* Restore state in a new VM. */ vcpu =vm_recreate_with_one_vcpu (vm);vcpu_load_state (vcpu, state);
Signature · 1 line
2.53.0.414.gf7e9f6c205-goog
PATCH v3 4/8 KVM: selftests: Extend state_test to check next_rip
In reply to Yosry AhmedSimilar to vGIF, extend state_test to make sure that next_rip is saved correctly in nested state. GUEST_SYNC() in L2 causes IO emulation by KVM, which advances the RIP to the value of next_rip. Hence, if next_rip is saved correctly, its value should match the saved RIP value.
--- tools/testing/selftests/kvm/x86/state_test.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
tools/testing/selftests/kvm/x86/state_test.c +11 -0
@@ -236,6 +236,17 @@ void svm_check_nested_state(int stage, struct kvm_x86_state *state)if (stage ==6 )TEST_ASSERT_EQ (!!(vmcb->control.int_ctl &V_GIF_MASK ),0 ); } + +if (kvm_cpu_has (X86_FEATURE_NRIPS )) { +/* +* GUEST_SYNC() causes IO emulation in KVM, in which case the +* RIP is advanced before exiting to userspace. Hence, the RIP +* in the saved state should be the same as nRIP saved by the +* CPU in the VMCB. +*/ +if (stage ==6 ) +TEST_ASSERT_EQ (vmcb->control.next_rip, state->regs.rip); + } }void check_nested_state (int stage,struct kvm_x86_state *state)
Signature · 1 line
2.53.0.414.gf7e9f6c205-goog
PATCH v3 5/8 KVM: nSVM: Always use NextRIP as vmcb02's NextRIP after first L2 VMRUN
In reply to Yosry AhmedFor guests with NRIPS disabled, L1 does not provide NextRIP when running an L2 with an injected soft interrupt, instead it advances the current RIP before running it. KVM uses the current RIP as the NextRIP in vmcb02 to emulate a CPU without NRIPS.
However, after L2 runs the first time, NextRIP will be updated by the CPU and/or KVM, and the current RIP is no longer the correct value to use in vmcb02. Hence, after save/restore, use the current RIP if and only if a nested run is pending, otherwise use NextRIP.
--- arch/x86/kvm/svm/nested.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
arch/x86/kvm/svm/nested.c +16 -9
@@ -845,17 +845,24 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, vmcb02->control.event_inj_err = svm->nested.ctl.event_inj_err;/* -* next_rip is consumed on VMRUN as the return address pushed on the +* NextRIP is consumed on VMRUN as the return address pushed on the * stack for injected soft exceptions/interrupts. If nrips is exposed -* to L1, take it verbatim from vmcb12. 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). +* to L1, take it verbatim from vmcb12. +* +* 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). This is +* only the case for the first L2 run after VMRUN. After that (e.g. +* during save/restore), NextRIP is updated by the CPU and/or KVM, and +* the value of the L2 RIP from vmcb12 should not be used. */ -if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS )) - vmcb02->control.next_rip = svm->nested.ctl.next_rip; -else if (boot_cpu_has (X86_FEATURE_NRIPS )) - vmcb02->control.next_rip = vmcb12_rip; +if (boot_cpu_has (X86_FEATURE_NRIPS )) { +if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS ) || + !svm->nested.nested_run_pending) + vmcb02->control.next_rip = svm->nested.ctl.next_rip; +else + vmcb02->control.next_rip = vmcb12_rip; + } svm->nmi_l1_to_l2 =is_evtinj_nmi (vmcb02->control.event_inj);if (is_evtinj_soft (vmcb02->control.event_inj)) {
Signature · 1 line
2.53.0.414.gf7e9f6c205-goog
Re: [PATCH v3 5/8] KVM: nSVM: Always use NextRIP as vmcb02's NextRIP after first L2 VMRUN
Quoted 43 lines · Yosry Ahmed
For guests with NRIPS disabled, L1 does not provide NextRIP when running an L2 with an injected soft interrupt, instead it advances the current RIP before running it. KVM uses the current RIP as the NextRIP in vmcb02 to emulate a CPU without NRIPS.
However, after L2 runs the first time, NextRIP will be updated by the CPU and/or KVM, and the current RIP is no longer the correct value to use in vmcb02. Hence, after save/restore, use the current RIP if and only if a nested run is pending, otherwise use NextRIP.
Fixes cc440cdad5b7 ("KVM: nSVM: implement KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE")Signed-off-by Yosry Ahmed <[email protected]>--- arch/x86/kvm/svm/nested.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)arch/x86/kvm/svm/nested.c +9
-6@@ -845,17 +845,24 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, vmcb02->control.event_inj_err = svm->nested.ctl.event_inj_err;/* -* next_rip is consumed on VMRUN as the return address pushed on the +* NextRIP is consumed on VMRUN as the return address pushed on the * stack for injected soft exceptions/interrupts. If nrips is exposed -* to L1, take it verbatim from vmcb12. 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). +* to L1, take it verbatim from vmcb12. +* +* 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). This is +* only the case for the first L2 run after VMRUN. After that (e.g. +* during save/restore), NextRIP is updated by the CPU and/or KVM, and +* the value of the L2 RIP from vmcb12 should not be used. */ -if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS ))
arch/x86/kvm/svm/nested.c static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, · line 865
Quoted 10 lines
arch/x86/kvm/svm/nested.c +7
-3- vmcb02->control.next_rip = svm->nested.ctl.next_rip; - else if (boot_cpu_has(X86_FEATURE_NRIPS)) - vmcb02->control.next_rip = vmcb12_rip; +if (boot_cpu_has (X86_FEATURE_NRIPS )) { +if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS ) || + !svm->nested.nested_run_pending) + vmcb02->control.next_rip = svm->nested.ctl.next_rip; +else + vmcb02->control.next_rip = vmcb12_rip; + }
This should probably also apply to soft_int_next_rip below the context lines. Otherwise after patch 7 we keep it uninitialized if the guest doesn't have NRIPs and !nested_run_pending.
Quoted 6 lines
arch/x86/kvm/svm/nested.c +0
-1svm->nmi_l1_to_l2 =is_evtinj_nmi (vmcb02->control.event_inj);if (is_evtinj_soft (vmcb02->control.event_inj)) { --2.53.0.414.gf7e9f6c205-goog
Sean Christopherson
maintainerRe: [PATCH v3 5/8] KVM: nSVM: Always use NextRIP as vmcb02's NextRIP after first L2 VMRUN
Quoted 50 lines · Yosry Ahmed
Quoted 49 lines · Yosry Ahmed
For guests with NRIPS disabled, L1 does not provide NextRIP when running an L2 with an injected soft interrupt, instead it advances the current RIP before running it. KVM uses the current RIP as the NextRIP in vmcb02 to emulate a CPU without NRIPS.
However, after L2 runs the first time, NextRIP will be updated by the CPU and/or KVM, and the current RIP is no longer the correct value to use in vmcb02. Hence, after save/restore, use the current RIP if and only if a nested run is pending, otherwise use NextRIP.
Fixes cc440cdad5b7 ("KVM: nSVM: implement KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE")Signed-off-by Yosry Ahmed <[email protected]>--- arch/x86/kvm/svm/nested.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)arch/x86/kvm/svm/nested.c +12
-9@@ -845,17 +845,24 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, vmcb02->control.event_inj_err = svm->nested.ctl.event_inj_err;/* -* next_rip is consumed on VMRUN as the return address pushed on the +* NextRIP is consumed on VMRUN as the return address pushed on the * stack for injected soft exceptions/interrupts. If nrips is exposed -* to L1, take it verbatim from vmcb12. 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). +* to L1, take it verbatim from vmcb12. +* +* 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). This is +* only the case for the first L2 run after VMRUN. After that (e.g. +* during save/restore), NextRIP is updated by the CPU and/or KVM, and +* the value of the L2 RIP from vmcb12 should not be used. */ -if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS )) - vmcb02->control.next_rip = svm->nested.ctl.next_rip; -else if (boot_cpu_has (X86_FEATURE_NRIPS )) - vmcb02->control.next_rip = vmcb12_rip; +if (boot_cpu_has (X86_FEATURE_NRIPS )) { +if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS ) || + !svm->nested.nested_run_pending)
Quoted 8 lines
arch/x86/kvm/svm/nested.c static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, · line 865
Quoted 4 lines
arch/x86/kvm/svm/nested.c +4
-0+ vmcb02->control.next_rip = svm->nested.ctl.next_rip; + else + vmcb02->control.next_rip = vmcb12_rip; + }This should probably also apply to
soft_int_next_ripbelow the context lines. Otherwise after patch 7 we keep it uninitialized if the guest doesn't have NRIPs and !nested_run_pending.
That's fine though, isn't it? Because in that case, doesn't the soft int have to comein through svm_update_soft_interrupt_rip()? Ugh, no because nSVM migrates control.event_inj.
IIUC, we want to end up with this?
arch/x86/kvm/svm/nested.c +2 -1
@@ -925,7 +925,8 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)*/ if (is_evtinj_soft (vmcb02->control.event_inj)) { svm->soft_int_injected = true; -if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS )) +if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS ) || + !svm->nested.nested_run_pending) svm->soft_int_next_rip = vmcb12_ctrl->next_rip; }
Re: [PATCH v3 5/8] KVM: nSVM: Always use NextRIP as vmcb02's NextRIP after first L2 VMRUN
[..]
Quoted 36 lines
Quoted 27 lines
Quoted 27 lines
(quoted diff) +12
-9@@ -845,17 +845,24 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, vmcb02->control.event_inj_err = svm->nested.ctl.event_inj_err; /* - * next_rip is consumed on VMRUN as the return address pushed on the + * NextRIP is consumed on VMRUN as the return address pushed on the * stack for injected soft exceptions/interrupts. If nrips is exposed - * to L1, take it verbatim from vmcb12. 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). + * to L1, take it verbatim from vmcb12. + * + * 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). This is + * only the case for the first L2 run after VMRUN. After that (e.g. + * during save/restore), NextRIP is updated by the CPU and/or KVM, and + * the value of the L2 RIP from vmcb12 should not be used. */ - if (guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS)) - vmcb02->control.next_rip = svm->nested.ctl.next_rip; - else if (boot_cpu_has(X86_FEATURE_NRIPS)) - vmcb02->control.next_rip = vmcb12_rip; + if (boot_cpu_has(X86_FEATURE_NRIPS)) { + if (guest_cpu_cap_has(vcpu, X86_FEATURE_NRIPS) || + !svm->nested.nested_run_pending)Quoted 8 lines
Review static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, · line 865
Quoted 4 lines
(quoted diff) +4
-0+ vmcb02->control.next_rip = svm->nested.ctl.next_rip; + else + vmcb02->control.next_rip = vmcb12_rip; + }This should probably also apply to
soft_int_next_ripbelow the context lines. Otherwise after patch 7 we keep it uninitialized if the guest doesn't have NRIPs and !nested_run_pending.
Quoted 5 lines
That's fine though, isn't it? Because in that case, doesn't the soft int have to comein through
svm_update_soft_interrupt_rip()? Ugh, no because nSVM migrates control.event_inj.IIUC, we want to end up with this?
Yes.
Quoted 14 lines
arch/x86/kvm/svm/nested.c +2
-1@@ -925,7 +925,8 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)*/ if (is_evtinj_soft (vmcb02->control.event_inj)) { svm->soft_int_injected = true; -if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS )) +if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS ) || + !svm->nested.nested_run_pending) svm->soft_int_next_rip = vmcb12_ctrl->next_rip; }
PATCH v3 6/8 KVM: nSVM: Delay stuffing L2's current RIP into NextRIP until vCPU run
In reply to Yosry AhmedFor guests with NRIPS disabled, L1 does not provide NextRIP when running an L2 with an injected soft interrupt, instead it advances L2's RIP before running it. KVM uses L2's current RIP as the NextRIP in vmcb02 to emulate a CPU without NRIPS.
However, in svm_set_nested_state(), the value used for L2's current RIP comes from vmcb02, which is just whatever the vCPU had in vmcb02 before restoring nested state (zero on a freshly created vCPU). Passing the cached RIP value instead (i.e. kvm_rip_read()) would only fix the issue if registers are restored before nested state.
Instead, split the logic of setting NextRIP in vmcb02. Handle the 'normal' case of initializing vmcb02's NextRIP using NextRIP from vmcb12 (or KVM_GET_NESTED_STATE's payload) in nested_vmcb02_prepare_control(). Delay the special case of stuffing L2's current RIP into vmcb02's NextRIP until shortly before the vCPU is run, to make sure the most up-to-date value of RIP is used regardless of KVM_SET_REGS and KVM_SET_NESTED_STATE's relative ordering.
--- arch/x86/kvm/svm/nested.c | 25 ++++++++----------------- arch/x86/kvm/svm/svm.c | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 17 deletions(-)
arch/x86/kvm/svm/nested.c +8 -17
@@ -845,24 +845,15 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, vmcb02->control.event_inj_err = svm->nested.ctl.event_inj_err;/* -* NextRIP is consumed on VMRUN as the return address pushed on the -* stack for injected soft exceptions/interrupts. If nrips is exposed -* to L1, take it verbatim from vmcb12. -* -* 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). This is -* only the case for the first L2 run after VMRUN. After that (e.g. -* during save/restore), NextRIP is updated by the CPU and/or KVM, and -* the value of the L2 RIP from vmcb12 should not be used. +* If nrips is exposed to L1, take NextRIP as-is. Otherwise, L1 +* advances L2's RIP before VMRUN instead of using NextRIP. KVM will +* stuff the current RIP as vmcb02's NextRIP before L2 is run. After +* the first run of L2 (e.g. after save+restore), NextRIP is updated by +* the CPU and/or KVM and should be used regardless of L1's support. */ -if (boot_cpu_has (X86_FEATURE_NRIPS )) { -if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS ) || - !svm->nested.nested_run_pending) - vmcb02->control.next_rip = svm->nested.ctl.next_rip; -else - vmcb02->control.next_rip = vmcb12_rip; - } +if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS ) || + !svm->nested.nested_run_pending) + vmcb02->control.next_rip = svm->nested.ctl.next_rip; svm->nmi_l1_to_l2 =is_evtinj_nmi (vmcb02->control.event_inj);if (is_evtinj_soft (vmcb02->control.event_inj)) {
arch/x86/kvm/svm/svm.c +17 -0
@@ -3660,6 +3660,23 @@ static int pre_svm_run(struct kvm_vcpu *vcpu)if (svm->current_vmcb->asid_generation != sd->asid_generation)new_asid (svm, sd); +/* +* 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. +*/ +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); + } +return 0 ; }
Signature · 1 line
2.53.0.414.gf7e9f6c205-goog
PATCH v3 7/8 KVM: nSVM: Delay setting soft IRQ RIP tracking fields until vCPU run
In reply to Yosry AhmedIn the save+restore path, when restoring nested state, the values of RIP and CS base passed into nested_vmcb02_prepare_control() are mostly incorrect. They are both pulled from the vmcb02. For CS base, the value is only correct if system regs are restored before nested state. The value of RIP is whatever the vCPU had in vmcb02 before restoring nested state (zero on a freshly created vCPU).
Instead, take a similar approach to NextRIP, and delay initializing the RIP tracking fields until shortly before the vCPU is run, to make sure the most up-to-date values of RIP and CS base are used regardless of KVM_SET_SREGS, KVM_SET_REGS, and KVM_SET_NESTED_STATE's relative ordering.
--- arch/x86/kvm/svm/nested.c | 17 ++++++++--------- arch/x86/kvm/svm/svm.c | 10 ++++++++++ 2 files changed, 18 insertions(+), 9 deletions(-)
arch/x86/kvm/svm/nested.c +8 -9
@@ -742,9 +742,7 @@ static bool is_evtinj_nmi(u32 evtinj)return type ==SVM_EVTINJ_TYPE_NMI ; } -static void nested_vmcb02_prepare_control (struct vcpu_svm *svm, -unsigned long vmcb12_rip, -unsigned long vmcb12_csbase) +static void nested_vmcb02_prepare_control (struct vcpu_svm *svm) {u32 int_ctl_vmcb01_bits =V_INTR_MASKING_MASK ;u32 int_ctl_vmcb12_bits =V_TPR_MASK |V_IRQ_INJECTION_BITS_MASK ;
@@ -856,14 +854,15 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, vmcb02->control.next_rip = svm->nested.ctl.next_rip; svm->nmi_l1_to_l2 =is_evtinj_nmi (vmcb02->control.event_inj); + +/* +* soft_int_csbase, soft_int_old_rip, and soft_int_next_rip (if L1 +* doesn't have NRIPS) are initialized later, before the vCPU is run. +*/ if (is_evtinj_soft (vmcb02->control.event_inj)) { svm->soft_int_injected = true; - svm->soft_int_csbase = vmcb12_csbase; - svm->soft_int_old_rip = vmcb12_rip;if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS )) svm->soft_int_next_rip = svm->nested.ctl.next_rip; -else - svm->soft_int_next_rip = vmcb12_rip; }/* LBR_CTL_ENABLE_MASK is controlled by svm_update_lbrv() */
@@ -961,7 +960,7 @@ int enter_svm_guest_mode(struct kvm_vcpu *vcpu, u64 vmcb12_gpa,nested_svm_copy_common_state (svm->vmcb01.ptr, svm->nested.vmcb02.ptr);svm_switch_vmcb (svm, &svm->nested.vmcb02); -nested_vmcb02_prepare_control (svm, vmcb12->save.rip, vmcb12->save.cs.base); +nested_vmcb02_prepare_control (svm);nested_vmcb02_prepare_save (svm, vmcb12); ret = nested_svm_load_cr3(&svm->vcpu, svm->nested.save.cr3,
@@ -1906,7 +1905,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,nested_copy_vmcb_control_to_cache (svm, ctl);svm_switch_vmcb (svm, &svm->nested.vmcb02); -nested_vmcb02_prepare_control (svm, svm->vmcb->save.rip, svm->vmcb->save.cs.base); +nested_vmcb02_prepare_control (svm);/* * While the nested guest CR3 is already checked and set by
arch/x86/kvm/svm/svm.c +10 -0
@@ -3670,11 +3670,21 @@ static int pre_svm_run(struct kvm_vcpu *vcpu)* 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) {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); + } }return 0 ;
Signature · 1 line
2.53.0.414.gf7e9f6c205-goog
Re: [PATCH v3 7/8] KVM: nSVM: Delay setting soft IRQ RIP tracking fields until vCPU run
Quoted 89 lines · Yosry Ahmed
In the save+restore path, when restoring nested state, the values of RIP and CS base passed into
nested_vmcb02_prepare_control()are mostly incorrect. They are both pulled from the vmcb02. For CS base, the value is only correct if system regs are restored before nested state. The value of RIP is whatever the vCPU had in vmcb02 before restoring nested state (zero on a freshly created vCPU).Instead, take a similar approach to NextRIP, and delay initializing the RIP tracking fields until shortly before the vCPU is run, to make sure the most up-to-date values of RIP and CS base are used regardless of
KVM_SET_SREGS,KVM_SET_REGS, and KVM_SET_NESTED_STATE's relative ordering.Fixes cc440cdad5b7 ("KVM: nSVM: implement KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE")Suggested-by Sean Christopherson <[email protected]>Signed-off-by Yosry Ahmed <[email protected]>--- arch/x86/kvm/svm/nested.c | 17 ++++++++--------- arch/x86/kvm/svm/svm.c | 10 ++++++++++ 2 files changed, 18 insertions(+), 9 deletions(-)arch/x86/kvm/svm/nested.c +8
-9@@ -742,9 +742,7 @@ static bool is_evtinj_nmi(u32 evtinj)return type ==SVM_EVTINJ_TYPE_NMI ; } -static void nested_vmcb02_prepare_control (struct vcpu_svm *svm, -unsigned long vmcb12_rip, -unsigned long vmcb12_csbase) +static void nested_vmcb02_prepare_control (struct vcpu_svm *svm) {u32 int_ctl_vmcb01_bits =V_INTR_MASKING_MASK ;u32 int_ctl_vmcb12_bits =V_TPR_MASK |V_IRQ_INJECTION_BITS_MASK ;@@ -856,14 +854,15 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm, vmcb02->control.next_rip = svm->nested.ctl.next_rip; svm->nmi_l1_to_l2 =is_evtinj_nmi (vmcb02->control.event_inj); + +/* +* soft_int_csbase, soft_int_old_rip, and soft_int_next_rip (if L1 +* doesn't have NRIPS) are initialized later, before the vCPU is run. +*/ if (is_evtinj_soft (vmcb02->control.event_inj)) { svm->soft_int_injected = true; - svm->soft_int_csbase = vmcb12_csbase; - svm->soft_int_old_rip = vmcb12_rip;if (guest_cpu_cap_has (vcpu,X86_FEATURE_NRIPS )) svm->soft_int_next_rip = svm->nested.ctl.next_rip; -else - svm->soft_int_next_rip = vmcb12_rip; }/* LBR_CTL_ENABLE_MASK is controlled by svm_update_lbrv() */ @@ -961,7 +960,7 @@ int enter_svm_guest_mode(struct kvm_vcpu *vcpu, u64 vmcb12_gpa,nested_svm_copy_common_state (svm->vmcb01.ptr, svm->nested.vmcb02.ptr);svm_switch_vmcb (svm, &svm->nested.vmcb02); -nested_vmcb02_prepare_control (svm, vmcb12->save.rip, vmcb12->save.cs.base); +nested_vmcb02_prepare_control (svm);nested_vmcb02_prepare_save (svm, vmcb12); ret = nested_svm_load_cr3(&svm->vcpu, svm->nested.save.cr3,@@ -1906,7 +1905,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,nested_copy_vmcb_control_to_cache (svm, ctl);svm_switch_vmcb (svm, &svm->nested.vmcb02); -nested_vmcb02_prepare_control (svm, svm->vmcb->save.rip, svm->vmcb->save.cs.base); +nested_vmcb02_prepare_control (svm);/* * While the nested guest CR3 is already checked and set by arch/x86/kvm/svm/svm.c +3
-0@@ -3670,11 +3670,21 @@ static int pre_svm_run(struct kvm_vcpu *vcpu)* 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) {
arch/x86/kvm/svm/svm.c static int pre_svm_run(struct kvm_vcpu *vcpu) · line 3687
Quoted 10 lines
arch/x86/kvm/svm/svm.c +7
-0if (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); + }
AI found a bug here. These fields will be left uninitialized if we cancel injection before pre_svm_run() (e.g. due to kvm_vcpu_exit_request()). I was going to suggest moving this to pre-run, but this leaves a larger gap where RIP can be updated from under us. Sean has a better fixup in progress.
Sean Christopherson
maintainerRe: [PATCH v3 7/8] KVM: nSVM: Delay setting soft IRQ RIP tracking fields until vCPU run
Quoted 22 lines · Yosry Ahmed
Quoted 21 lines · Yosry Ahmed
arch/x86/kvm/svm/svm.c +8
-0@@ -3670,11 +3670,21 @@ static int pre_svm_run(struct kvm_vcpu *vcpu)* 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) {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 ))
Quoted 8 lines
arch/x86/kvm/svm/svm.c static int pre_svm_run(struct kvm_vcpu *vcpu) · line 3687
Quoted 2 lines
arch/x86/kvm/svm/svm.c +2
-0+ svm->soft_int_next_rip =kvm_rip_read (vcpu); + }AI found a bug here. These fields will be left uninitialized if we cancel injection before
pre_svm_run()(e.g. due tokvm_vcpu_exit_request()). I was going to suggest moving this to pre-run, but this leaves a larger gap where RIP can be updated from under us. Sean has a better fixup in progress.
With comments to explain the madness, this should work as fixup. It's gross and brittle, but the only alternative I see is to add a flag to differentiate the save/restore case from the VMRUN case. Which isn't terrible, but IMO most of the brittleness comes from the disaster that is the architecture.
Given that the soft int reinjection code will be inherently brittle, and that the save/restore scenario will be _extremely_ rare, I think it's worth the extra bit of nastiness so that _if_ there's a bug, it's at least slightly more likely we'll find it via the normal VMRUN path.
arch/x86/kvm/svm/svm.c +15 -6
@@ -3755,6 +3755,16 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)return svm_invoke_exit_handler (vcpu, svm->vmcb->control.exit_code); } +static void svm_set_nested_run_soft_int_state (struct kvm_vcpu *vcpu) +{ +struct vcpu_svm *svm =to_svm (vcpu); + + 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); +} +static int pre_svm_run (struct kvm_vcpu *vcpu) {struct svm_cpu_data *sd =per_cpu_ptr (&svm_data, vcpu->cpu);
@@ -3797,12 +3807,8 @@ static int pre_svm_run(struct kvm_vcpu *vcpu) !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); - } +if (svm->soft_int_injected) +svm_set_nested_run_soft_int_state (vcpu); }return 0 ;
@@ -4250,6 +4256,9 @@ static void svm_complete_soft_interrupt(struct kvm_vcpu *vcpu, u8 vector,bool is_soft = (type ==SVM_EXITINTINFO_TYPE_SOFT );struct vcpu_svm *svm =to_svm (vcpu); +if (is_guest_mode (vcpu) && svm->nested.nested_run_pending) +svm_set_nested_run_soft_int_state (vcpu); +/* * If NRIPS is enabled, KVM must snapshot the pre-VMRUN next_rip that's * associated with the original soft exception/interrupt. next_rip is
Re: [PATCH v3 7/8] KVM: nSVM: Delay setting soft IRQ RIP tracking fields until vCPU run
Quoted 11 lines
Quoted 5 lines
AI found a bug here. These fields will be left uninitialized if we cancel injection before
pre_svm_run()(e.g. due tokvm_vcpu_exit_request()). I was going to suggest moving this to pre-run, but this leaves a larger gap where RIP can be updated from under us. Sean has a better fixup in progress.With comments to explain the madness, this should work as fixup. It's gross and brittle, but the only alternative I see is to add a flag to differentiate the save/restore case from the VMRUN case. Which isn't terrible, but IMO most of the brittleness comes from the disaster that is the architecture.
Quoted 4 lines
Given that the soft int reinjection code will be inherently brittle, and that the save/restore scenario will be _extremely_ rare, I think it's worth the extra bit of nastiness so that _if_ there's a bug, it's at least slightly more likely we'll find it via the normal VMRUN path.
Agreed, the fixup looks good to me as long as we add comments to explain WTF is going on here lol.
Quoted 47 lines
arch/x86/kvm/svm/svm.c +15
-6@@ -3755,6 +3755,16 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)return svm_invoke_exit_handler (vcpu, svm->vmcb->control.exit_code); } +static void svm_set_nested_run_soft_int_state (struct kvm_vcpu *vcpu) +{ +struct vcpu_svm *svm =to_svm (vcpu); + + 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); +} +static int pre_svm_run (struct kvm_vcpu *vcpu) {struct svm_cpu_data *sd =per_cpu_ptr (&svm_data, vcpu->cpu);@@ -3797,12 +3807,8 @@ static int pre_svm_run(struct kvm_vcpu *vcpu) !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); - } +if (svm->soft_int_injected) +svm_set_nested_run_soft_int_state (vcpu); }return 0 ;@@ -4250,6 +4256,9 @@ static void svm_complete_soft_interrupt(struct kvm_vcpu *vcpu, u8 vector,bool is_soft = (type ==SVM_EXITINTINFO_TYPE_SOFT );struct vcpu_svm *svm =to_svm (vcpu); +if (is_guest_mode (vcpu) && svm->nested.nested_run_pending) +svm_set_nested_run_soft_int_state (vcpu); +/* * If NRIPS is enabled, KVM must snapshot the pre-VMRUN next_rip that's * associated with the original soft exception/interrupt. next_rip is
PATCH v3 8/8 DO NOT MERGE: KVM: selftests: Reproduce nested RIP restore bug
In reply to Yosry AhmedUpdate svm_nested_soft_inject_test such that L1 syncs to userspace before running L2. The test then enables single-stepping and steps through guest code until VMRUN is execute, and saves/restores the VM immediately after (before L2 runs).
This reproduces a bug in save/restore where L2's RIP is not used correctly to construct the vmcb02 at the destination.
--- .../testing/selftests/kvm/lib/x86/processor.c | 8 +- .../kvm/x86/svm_nested_soft_inject_test.c | 74 +++++++++++++++---- 2 files changed, 65 insertions(+), 17 deletions(-)
tools/testing/selftests/kvm/lib/x86/processor.c +7 -1
@@ -1275,6 +1275,8 @@ struct kvm_x86_state *vcpu_save_state(struct kvm_vcpu *vcpu)return state; } +#define LOAD_REGS_BEFORE_NESTED 1 +void vcpu_load_state (struct kvm_vcpu *vcpu,struct kvm_x86_state *state) {vcpu_sregs_set (vcpu, &state->sregs);
@@ -1287,10 +1289,14 @@ void vcpu_load_state(struct kvm_vcpu *vcpu, struct kvm_x86_state *state)vcpu_events_set (vcpu, &state->events);vcpu_mp_state_set (vcpu, &state->mp_state);vcpu_debugregs_set (vcpu, &state->debugregs); -vcpu_regs_set (vcpu, &state->regs); +if (LOAD_REGS_BEFORE_NESTED ) +vcpu_regs_set (vcpu, &state->regs);if (state->nested.size)vcpu_nested_state_set (vcpu, &state->nested); + +if (!LOAD_REGS_BEFORE_NESTED ) +vcpu_regs_set (vcpu, &state->regs); }void kvm_x86_state_cleanup (struct kvm_x86_state *state)
tools/testing/selftests/kvm/x86/svm_nested_soft_inject_test.c +58 -16
98 lines not on this page · raw message
Signature · 1 line
2.53.0.414.gf7e9f6c205-goog
Quoted 13 lines · Yosry Ahmed
This is a combined v3 of series [1] and v2 of series [2], as patch 1 here is a dependency of patch 5. Without patch 1, NextRIP is not sync'd correctly to the cache, and restoring it for a guest without NRIPS is a bug.
The series fixes two classes of save/restore bugs:
- Some fields written by the CPU are not sync'd from vmcb02 to cached vmcb12 after VMRUN, so are not up-to-date inKVM_GET_NESTED_STATEpayload (fixes in patches 1 & 2, tests in patches 3 & 4).
- Ordering betweenKVM_SET_NESTED_STATEand KVM_SET_{S}REGS could cause vmcb02 to be incorrectly initialized after save+restore (fixes in patches 5 to 7).
Quoted 1 line
[...]
Applied to kvm-x86 nested, with the mess of fixups. Thanks!
[1/8] KVM: nSVM: Sync NextRIP to cached vmcb12 after VMRUN of L2
https://github.com/kvm-x86/linux/commit/778d8c1b2a6f [2/8] KVM: nSVM: Sync interrupt shadow to cached vmcb12 after VMRUN of L2 https://github.com/kvm-x86/linux/commit/03bee264f8eb [3/8] KVM: selftests: Extend state_test to check vGIF https://github.com/kvm-x86/linux/commit/2303ca26fbb0 [4/8] KVM: selftests: Extend state_test to check next_rip https://github.com/kvm-x86/linux/commit/e5cdd34b5f74 [5/8] KVM: nSVM: Always use NextRIP as vmcb02's NextRIP after first L2 VMRUN https://github.com/kvm-x86/linux/commit/8d397582f6b5 [6/8] KVM: nSVM: Delay stuffing L2's current RIP into NextRIP until vCPU run https://github.com/kvm-x86/linux/commit/a0592461f39c [7/8] KVM: nSVM: Delay setting soft IRQ RIP tracking fields until vCPU run https://github.com/kvm-x86/linux/commit/c64bc6ed1764
[8/8] DO NOT MERGE: KVM: selftests: Reproduce nested RIP restore bug
(DROP - your wish is my command)