KVM: VMX: Fix invalid guest state detection after task-switch emulation
PATCH 1/1 KVM: VMX: Fix invalid guest state detection after task-switch emulation
From: Wanpeng Li <[email protected]>
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reason TASK_SWITCH rip 0x0 info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reason TASK_SWITCH rip 0x0 info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)
......................
It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing vmx->emulation_required to be set, when in fact invalid guest state emulation is not needed.
This patch fixes it by updating vmx->emulation_required after the rflags (and vm86 flag) is updated in task-switch emulation.
--- arch/x86/kvm/vmx.c | 1 + 1 file changed, 1 insertion(+)
arch/x86/kvm/vmx.c +1 -0
@@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)* TODO: What about debug traps on tss switch? * Are we supposed to inject them and update dr6? */ + vmx->emulation_required =emulation_required (vcpu);return 1 ; }
Signature · 1 line
2.7.4
2017-07-11 00:13-0700, Wanpeng Li:
Quoted 38 lines
From: Wanpeng Li <[email protected]>
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)
......................It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated in task-switch emulation.Suggested-by Nadav Amit <[email protected]>Cc Paolo Bonzini <[email protected]>Cc Radim Krčmář <[email protected]>Cc Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>--- arch/x86/kvm/vmx.c | 1 + 1 file changed, 1 insertion(+)arch/x86/kvm/vmx.c +0
-0
arch/x86/kvm/vmx.c static int handle_task_switch(struct kvm_vcpu *vcpu) · line 6258
Quoted 4 lines
arch/x86/kvm/vmx.c +1
-0@@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)* TODO: What about debug traps on tss switch? * Are we supposed to inject them and update dr6? */ + vmx->emulation_required =emulation_required (vcpu);
Hm, so the problem happened because changes to rflags can flip the value of emulation_required(). I would add this line to vmx_set_rflags() to make sure that we fixed everything, thanks.
Paolo Bonzini
maintainerQuoted 34 lines · Radim Krčmář
2017-07-11 00:13-0700, Wanpeng Li:
Quoted 33 lines
From: Wanpeng Li <[email protected]>
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)
......................It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated in task-switch emulation.Suggested-by Nadav Amit <[email protected]>Cc Paolo Bonzini <[email protected]>Cc Radim Krčmář <[email protected]>Cc Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>--- arch/x86/kvm/vmx.c | 1 + 1 file changed, 1 insertion(+)
Quoted 13 lines
arch/x86/kvm/vmx.c static int handle_task_switch(struct kvm_vcpu *vcpu) · line 6258
Quoted 9 lines
arch/x86/kvm/vmx.c +1
-0@@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)* TODO: What about debug traps on tss switch? * Are we supposed to inject them and update dr6? */ + vmx->emulation_required =emulation_required (vcpu);Hm, so the problem happened because changes to rflags can flip the value of
emulation_required(). I would add this line tovmx_set_rflags()to make sure that we fixed everything, thanks.
Note that there is some extra complication, because emulation_required is expensive and you'll want to run it only when EFLAGS.VM changes.
Paolo
2017-07-12 0:09 GMT+08:00 Paolo Bonzini <[email protected]>:
Quoted 49 lines · Radim Krčmář
Quoted 34 lines
2017-07-11 00:13-0700, Wanpeng Li:
Quoted 33 lines
From: Wanpeng Li <[email protected]>
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)
......................It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated in task-switch emulation.Suggested-by Nadav Amit <[email protected]>Cc Paolo Bonzini <[email protected]>Cc Radim Krčmář <[email protected]>Cc Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>--- arch/x86/kvm/vmx.c | 1 + 1 file changed, 1 insertion(+)Quoted 13 lines
arch/x86/kvm/vmx.c static int handle_task_switch(struct kvm_vcpu *vcpu) · line 6258
Quoted 9 lines
arch/x86/kvm/vmx.c +1
-0@@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)* TODO: What about debug traps on tss switch? * Are we supposed to inject them and update dr6? */ + vmx->emulation_required =emulation_required (vcpu);Hm, so the problem happened because changes to rflags can flip the value of
emulation_required(). I would add this line tovmx_set_rflags()to make sure that we fixed everything, thanks.
Quoted 2 lines
Note that there is some extra complication, because
emulation_requiredis expensive and you'll want to run it only when EFLAGS.VM changes.
Agreed.
Regards,
Wanpeng Li
Ping, :)
2017-07-11 15:13 GMT+08:00 Wanpeng Li <[email protected]>:
Quoted 48 lines
From: Wanpeng Li <[email protected]>
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)
......................It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated in task-switch emulation.Suggested-by Nadav Amit <[email protected]>Cc Paolo Bonzini <[email protected]>Cc Radim Krčmář <[email protected]>Cc Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>--- arch/x86/kvm/vmx.c | 1 + 1 file changed, 1 insertion(+)arch/x86/kvm/vmx.c +1
-1@@ -6255,6 +6255,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)* TODO: What about debug traps on tss switch? * Are we supposed to inject them and update dr6? */ + vmx->emulation_required =emulation_required (vcpu);return 1 ; } --2.7.4
PATCH 1/1 KVM: VMX: Fix invalid guest state detection after task-switch emulation
2017-07-19 13:34+0800, Wanpeng Li:
Quoted 1 line
Ping, :)
I misunderstood where the ball is ... is the patch below ok?
---8<---
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reason TASK_SWITCH rip 0x0 info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reason TASK_SWITCH rip 0x0 info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)
It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing vmx->emulation_required to be set, when in fact invalid guest state emulation is not needed.
This patch fixes it by updating vmx->emulation_required after the rflags (and vm86 flag) is updated.
[Wanpeng wrote the commit message with initial patch and Radim moved the
update to vmx_set_rflags and added Paolo's suggestion for the check.]
--- arch/x86/kvm/vmx.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
arch/x86/kvm/vmx.c +10 -5
@@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)__vmx_load_host_state (to_vmx (vcpu)); } +static bool emulation_required (struct kvm_vcpu *vcpu) +{ +return emulate_invalid_guest_state && !guest_state_valid (vcpu); +} +static void vmx_decache_cr0_guest_bits (struct kvm_vcpu *vcpu);/*
@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =to_vmx (vcpu)->rflags; +__set_bit (VCPU_EXREG_RFLAGS , (ulong *)&vcpu->arch.regs_avail);to_vmx (vcpu)->rflags = rflags;if (to_vmx (vcpu)->rmode.vm86_active) {
@@ -2370,6 +2377,9 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) rflags |=X86_EFLAGS_IOPL |X86_EFLAGS_VM ; }vmcs_writel (GUEST_RFLAGS , rflags); + +if ((old_rflags ^ rflags) &X86_EFLAGS_VM ) +to_vmx (vcpu)->emulation_required =emulation_required (vcpu); }static u32 vmx_get_pkru (struct kvm_vcpu *vcpu)
@@ -3857,11 +3867,6 @@ static __init int alloc_kvm_area(void)return 0 ; } -static bool emulation_required (struct kvm_vcpu *vcpu) -{ -return emulate_invalid_guest_state && !guest_state_valid (vcpu); -} -static void fix_pmode_seg (struct kvm_vcpu *vcpu,int seg,struct kvm_segment *save) {
Signature · 1 line
2.13.2
Quoted 53 lines · Radim Krčmář
2017-07-19 13:34+0800, Wanpeng Li:
Quoted 1 line
Ping, :)
I misunderstood where the ball is ... is the patch below ok?
---8<---
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated.Suggested-by Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>[Wanpeng wrote the commit message with initial patch and Radim moved the update to
vmx_set_rflagsand added Paolo's suggestion for the check.]Signed-off-by Radim Krčmář <[email protected]>---
arch/x86/kvm/vmx.c| 15 ++++++++++-----1 file changed, 10 insertions(+), 5 deletions(-)arch/x86/kvm/vmx.c +5
-0@@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)__vmx_load_host_state (to_vmx (vcpu)); } +static bool emulation_required (struct kvm_vcpu *vcpu) +{ +return emulate_invalid_guest_state && !guest_state_valid (vcpu); +} +static void vmx_decache_cr0_guest_bits (struct kvm_vcpu *vcpu);/*
arch/x86/kvm/vmx.c static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) · line 2371
Quoted 4 lines
arch/x86/kvm/vmx.c +1
-0@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =to_vmx (vcpu)->rflags;
It assumes rflags was decached from the VMCS before. Probably it is true, but…
PATCH v2 1/1 KVM: VMX: Fix invalid guest state detection after task-switch emulation
2017-07-19 08:14-0700, Nadav Amit:
Quoted 8 lines
Review static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) · line 2371
Quoted 5 lines · Radim Krčmář
(quoted diff) +1
-0@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) { + unsigned long old_rflags = to_vmx(vcpu)->rflags;It assumes rflags was decached from the VMCS before. Probably it is true, but…
Right, it's better to use accessors everywhere, thanks. The line should read:
+ unsigned long old_rflags = vmx_get_rflags(vcpu);
---8<---
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reason TASK_SWITCH rip 0x0 info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reason TASK_SWITCH rip 0x0 info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)
It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing vmx->emulation_required to be set, when in fact invalid guest state emulation is not needed.
This patch fixes it by updating vmx->emulation_required after the rflags (and vm86 flag) is updated.
[Wanpeng wrote the commit message with initial patch and Radim moved the
update to vmx_set_rflags and added Paolo's suggestion for the check.]
--- arch/x86/kvm/vmx.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
arch/x86/kvm/vmx.c +10 -5
@@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)__vmx_load_host_state (to_vmx (vcpu)); } +static bool emulation_required (struct kvm_vcpu *vcpu) +{ +return emulate_invalid_guest_state && !guest_state_valid (vcpu); +} +static void vmx_decache_cr0_guest_bits (struct kvm_vcpu *vcpu);/*
@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =vmx_get_rflags (vcpu); +__set_bit (VCPU_EXREG_RFLAGS , (ulong *)&vcpu->arch.regs_avail);to_vmx (vcpu)->rflags = rflags;if (to_vmx (vcpu)->rmode.vm86_active) {
@@ -2370,6 +2377,9 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) rflags |=X86_EFLAGS_IOPL |X86_EFLAGS_VM ; }vmcs_writel (GUEST_RFLAGS , rflags); + +if ((old_rflags ^ rflags) &X86_EFLAGS_VM ) +to_vmx (vcpu)->emulation_required =emulation_required (vcpu); }static u32 vmx_get_pkru (struct kvm_vcpu *vcpu)
@@ -3857,11 +3867,6 @@ static __init int alloc_kvm_area(void)return 0 ; } -static bool emulation_required (struct kvm_vcpu *vcpu) -{ -return emulate_invalid_guest_state && !guest_state_valid (vcpu); -} -static void fix_pmode_seg (struct kvm_vcpu *vcpu,int seg,struct kvm_segment *save) {
Signature · 1 line
2.13.2
Quoted 72 lines · Radim Krčmář
2017-07-19 08:14-0700, Nadav Amit:
Quoted 8 lines
arch/x86/kvm/vmx.c static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) · line 2371
Quoted 5 lines · Radim Krčmář
arch/x86/kvm/vmx.c +1
-0@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =to_vmx (vcpu)->rflags;It assumes rflags was decached from the VMCS before. Probably it is true, but…
Right, it's better to use accessors everywhere, thanks. The line should read:
(quoted diff) +1
-1+ unsigned long old_rflags = vmx_get_rflags(vcpu); ---8<---This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated.Suggested-by Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>[Wanpeng wrote the commit message with initial patch and Radim moved the update to
vmx_set_rflagsand added Paolo's suggestion for the check.]Signed-off-by Radim Krčmář <[email protected]>---
arch/x86/kvm/vmx.c| 15 ++++++++++-----1 file changed, 10 insertions(+), 5 deletions(-)arch/x86/kvm/vmx.c +7
-0@@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)__vmx_load_host_state (to_vmx (vcpu)); } +static bool emulation_required (struct kvm_vcpu *vcpu) +{ +return emulate_invalid_guest_state && !guest_state_valid (vcpu); +} +static void vmx_decache_cr0_guest_bits (struct kvm_vcpu *vcpu);/* @@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =vmx_get_rflags (vcpu); +__set_bit (VCPU_EXREG_RFLAGS , (ulong *)&vcpu->arch.regs_avail);to_vmx (vcpu)->rflags = rflags;if (to_vmx (vcpu)->rmode.vm86_active) {
arch/x86/kvm/vmx.c static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) · line 2382
Quoted 6 lines
arch/x86/kvm/vmx.c +3
-0@@ -2370,6 +2377,9 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) rflags |=X86_EFLAGS_IOPL |X86_EFLAGS_VM ; }vmcs_writel (GUEST_RFLAGS , rflags); + +if ((old_rflags ^ rflags) &X86_EFLAGS_VM ) +to_vmx (vcpu)->emulation_required =emulation_required (vcpu);
Sorry for not pointing it before, but here you compare the old_rflags with the new rflags but after you already “massaged” it. So the value you compare with is not what the guest “sees”.
2017-07-20 0:25 GMT+08:00 Nadav Amit <[email protected]>:
Quoted 76 lines
Quoted 74 lines · Radim Krčmář
2017-07-19 08:14-0700, Nadav Amit:
Quoted 8 lines
arch/x86/kvm/vmx.c static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) · line 2371
Quoted 5 lines · Radim Krčmář
arch/x86/kvm/vmx.c +1
-0@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =to_vmx (vcpu)->rflags;It assumes rflags was decached from the VMCS before. Probably it is true, but…
Right, it's better to use accessors everywhere, thanks. The line should read:
+ unsigned long
old_rflags=vmx_get_rflags(vcpu);---8<---
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated.Suggested-by Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>[Wanpeng wrote the commit message with initial patch and Radim moved the update to
vmx_set_rflagsand added Paolo's suggestion for the check.]Signed-off-by Radim Krčmář <[email protected]>---
arch/x86/kvm/vmx.c| 15 ++++++++++-----1 file changed, 10 insertions(+), 5 deletions(-)arch/x86/kvm/vmx.c +7
-0@@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)__vmx_load_host_state (to_vmx (vcpu)); } +static bool emulation_required (struct kvm_vcpu *vcpu) +{ +return emulate_invalid_guest_state && !guest_state_valid (vcpu); +} +static void vmx_decache_cr0_guest_bits (struct kvm_vcpu *vcpu);/* @@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =vmx_get_rflags (vcpu); +__set_bit (VCPU_EXREG_RFLAGS , (ulong *)&vcpu->arch.regs_avail);to_vmx (vcpu)->rflags = rflags;if (to_vmx (vcpu)->rmode.vm86_active) {@@ -2370,6 +2377,9 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) rflags |=X86_EFLAGS_IOPL |X86_EFLAGS_VM ; }
Quoted 8 lines
arch/x86/kvm/vmx.c static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) · line 2382
Quoted 4 lines
arch/x86/kvm/vmx.c +3
-0vmcs_writel (GUEST_RFLAGS , rflags); + +if ((old_rflags ^ rflags) &X86_EFLAGS_VM ) +to_vmx (vcpu)->emulation_required =emulation_required (vcpu);Sorry for not pointing it before, but here you compare the
old_rflagswith the new rflags but after you already “massaged” it. So the value you compare with is not what the guest “sees”.
So you mean we should use unsigned long old_rflags = vmcs_readl(GUEST_RFLAGS); right?
Regards,
Wanpeng Li
Quoted 86 lines · Wanpeng Li
2017-07-20 0:25 GMT+08:00 Nadav Amit <[email protected]>:
Quoted 76 lines
Quoted 74 lines · Radim Krčmář
2017-07-19 08:14-0700, Nadav Amit:
Quoted 8 lines
arch/x86/kvm/vmx.c static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) · line 2371
Quoted 5 lines · Radim Krčmář
arch/x86/kvm/vmx.c +1
-0@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =to_vmx (vcpu)->rflags;It assumes rflags was decached from the VMCS before. Probably it is true, but…
Right, it's better to use accessors everywhere, thanks. The line should read:
+ unsigned long
old_rflags=vmx_get_rflags(vcpu);---8<---
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated.Suggested-by Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>[Wanpeng wrote the commit message with initial patch and Radim moved the update to
vmx_set_rflagsand added Paolo's suggestion for the check.]Signed-off-by Radim Krčmář <[email protected]>---
arch/x86/kvm/vmx.c| 15 ++++++++++-----1 file changed, 10 insertions(+), 5 deletions(-)arch/x86/kvm/vmx.c +7
-0@@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)__vmx_load_host_state (to_vmx (vcpu)); } +static bool emulation_required (struct kvm_vcpu *vcpu) +{ +return emulate_invalid_guest_state && !guest_state_valid (vcpu); +} +static void vmx_decache_cr0_guest_bits (struct kvm_vcpu *vcpu);/* @@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =vmx_get_rflags (vcpu); +__set_bit (VCPU_EXREG_RFLAGS , (ulong *)&vcpu->arch.regs_avail);to_vmx (vcpu)->rflags = rflags;if (to_vmx (vcpu)->rmode.vm86_active) {@@ -2370,6 +2377,9 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) rflags |=X86_EFLAGS_IOPL |X86_EFLAGS_VM ; }Quoted 8 lines
arch/x86/kvm/vmx.c static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) · line 2382
Quoted 4 lines
arch/x86/kvm/vmx.c +3
-0vmcs_writel (GUEST_RFLAGS , rflags); + +if ((old_rflags ^ rflags) &X86_EFLAGS_VM ) +to_vmx (vcpu)->emulation_required =emulation_required (vcpu);Sorry for not pointing it before, but here you compare the
old_rflagswith the new rflags but after you already “massaged” it. So the value you compare with is not what the guest “sees”.
Quoted 2 lines
So you mean we should use unsigned long
old_rflags=vmcs_readl(GUEST_RFLAGS); right?
No. The problem is not with old_rflags now, but with rflags. If vm86_active, then rflags is changed and you don’t compare the guest-visible rflags anymore.
2017-07-20 6:53 GMT+08:00 Nadav Amit <[email protected]>:
Quoted 88 lines
Quoted 86 lines · Wanpeng Li
2017-07-20 0:25 GMT+08:00 Nadav Amit <[email protected]>:
Quoted 84 lines
Quoted 72 lines · Radim Krčmář
2017-07-19 08:14-0700, Nadav Amit:
Quoted 8 lines
arch/x86/kvm/vmx.c static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) · line 2371
Quoted 5 lines · Radim Krčmář
arch/x86/kvm/vmx.c +1
-0@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =to_vmx (vcpu)->rflags;It assumes rflags was decached from the VMCS before. Probably it is true, but…
Right, it's better to use accessors everywhere, thanks. The line should read:
+ unsigned long
old_rflags=vmx_get_rflags(vcpu);---8<---
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated.Suggested-by Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>[Wanpeng wrote the commit message with initial patch and Radim moved the update to
vmx_set_rflagsand added Paolo's suggestion for the check.]Signed-off-by Radim Krčmář <[email protected]>---
arch/x86/kvm/vmx.c| 15 ++++++++++-----1 file changed, 10 insertions(+), 5 deletions(-)arch/x86/kvm/vmx.c +7
-0@@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)__vmx_load_host_state (to_vmx (vcpu)); } +static bool emulation_required (struct kvm_vcpu *vcpu) +{ +return emulate_invalid_guest_state && !guest_state_valid (vcpu); +} +static void vmx_decache_cr0_guest_bits (struct kvm_vcpu *vcpu);/* @@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =vmx_get_rflags (vcpu); +__set_bit (VCPU_EXREG_RFLAGS , (ulong *)&vcpu->arch.regs_avail);to_vmx (vcpu)->rflags = rflags;if (to_vmx (vcpu)->rmode.vm86_active) {arch/x86/kvm/vmx.c static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) · line 2382
Quoted 6 lines
arch/x86/kvm/vmx.c +3
-0@@ -2370,6 +2377,9 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) rflags |=X86_EFLAGS_IOPL |X86_EFLAGS_VM ; }vmcs_writel (GUEST_RFLAGS , rflags); + +if ((old_rflags ^ rflags) &X86_EFLAGS_VM ) +to_vmx (vcpu)->emulation_required =emulation_required (vcpu);Sorry for not pointing it before, but here you compare the
old_rflagswith the new rflags but after you already “massaged” it. So the value you compare with is not what the guest “sees”.
Quoted 6 lines
Quoted 2 lines
So you mean we should use unsigned long
old_rflags=vmcs_readl(GUEST_RFLAGS); right?No. The problem is not with
old_rflagsnow, but with rflags. Ifvm86_active, then rflags is changed and you don’t compare the guest-visible rflags anymore.
Ah, I see. So we should compare the old_flags with the rmode->save_rflags(guest-visible rflags) instead of the rflags (shadow rflags), right?
Regards,
Wanpeng Li
Quoted 92 lines · Wanpeng Li
2017-07-20 6:53 GMT+08:00 Nadav Amit <[email protected]>:
Quoted 91 lines
Quoted 88 lines · Wanpeng Li
2017-07-20 0:25 GMT+08:00 Nadav Amit <[email protected]>:
Quoted 76 lines
Quoted 74 lines · Radim Krčmář
2017-07-19 08:14-0700, Nadav Amit:
Quoted 8 lines
arch/x86/kvm/vmx.c static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) · line 2371
Quoted 5 lines · Radim Krčmář
arch/x86/kvm/vmx.c +1
-0@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =to_vmx (vcpu)->rflags;It assumes rflags was decached from the VMCS before. Probably it is true, but…
Right, it's better to use accessors everywhere, thanks. The line should read:
+ unsigned long
old_rflags=vmx_get_rflags(vcpu);---8<---
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated.Suggested-by Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>[Wanpeng wrote the commit message with initial patch and Radim moved the update to
vmx_set_rflagsand added Paolo's suggestion for the check.]Signed-off-by Radim Krčmář <[email protected]>---
arch/x86/kvm/vmx.c| 15 ++++++++++-----1 file changed, 10 insertions(+), 5 deletions(-)arch/x86/kvm/vmx.c +7
-0@@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)__vmx_load_host_state (to_vmx (vcpu)); } +static bool emulation_required (struct kvm_vcpu *vcpu) +{ +return emulate_invalid_guest_state && !guest_state_valid (vcpu); +} +static void vmx_decache_cr0_guest_bits (struct kvm_vcpu *vcpu);/* @@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =vmx_get_rflags (vcpu); +__set_bit (VCPU_EXREG_RFLAGS , (ulong *)&vcpu->arch.regs_avail);to_vmx (vcpu)->rflags = rflags;if (to_vmx (vcpu)->rmode.vm86_active) {@@ -2370,6 +2377,9 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) rflags |=X86_EFLAGS_IOPL |X86_EFLAGS_VM ; }Quoted 8 lines
arch/x86/kvm/vmx.c static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) · line 2382
Quoted 4 lines
arch/x86/kvm/vmx.c +3
-0vmcs_writel (GUEST_RFLAGS , rflags); + +if ((old_rflags ^ rflags) &X86_EFLAGS_VM ) +to_vmx (vcpu)->emulation_required =emulation_required (vcpu);Sorry for not pointing it before, but here you compare the
old_rflagswith the new rflags but after you already “massaged” it. So the value you compare with is not what the guest “sees”.So you mean we should use unsigned long
old_rflags=vmcs_readl(GUEST_RFLAGS); right?
Quoted 7 lines
Quoted 3 lines
No. The problem is not with
old_rflagsnow, but with rflags. Ifvm86_active, then rflags is changed and you don’t compare the guest-visible rflags anymore.Ah, I see. So we should compare the
old_flagswith thermode->save_rflags(guest-visible rflags)instead of the rflags (shadow rflags), right?
Not exactly, since rmode->save_rflags are invalid if !vm86_active. Instead, I think you should have a save_rflags variable on the stack that would hold the rflags before “massaging” and use it instead.
2017-07-20 7:06 GMT+08:00 Nadav Amit <[email protected]>:
Quoted 98 lines
Quoted 96 lines · Wanpeng Li
2017-07-20 6:53 GMT+08:00 Nadav Amit <[email protected]>:
Quoted 94 lines
Quoted 86 lines · Wanpeng Li
2017-07-20 0:25 GMT+08:00 Nadav Amit <[email protected]>:
Quoted 76 lines
Quoted 74 lines · Radim Krčmář
2017-07-19 08:14-0700, Nadav Amit:
Quoted 8 lines
arch/x86/kvm/vmx.c static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu) · line 2371
Quoted 5 lines · Radim Krčmář
arch/x86/kvm/vmx.c +1
-0@@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =to_vmx (vcpu)->rflags;It assumes rflags was decached from the VMCS before. Probably it is true, but…
Right, it's better to use accessors everywhere, thanks. The line should read:
+ unsigned long
old_rflags=vmx_get_rflags(vcpu);---8<---
This can be reproduced by EPT=1, unrestricted_guest=N, emulate_invalid_state=Y or EPT=0, the trace of kvm-unit-tests/taskswitch2.flat is like below, it tries to emulate invalid guest state task-switch:
kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)kvm_entry: vcpu 0kvm_exit: reasonTASK_SWITCHrip0x0info 40000058 0kvm_emulate_insn: 42000:0:0f 0b (0x2)kvm_emulate_insn: 42000:0:0f 0b (0x2) failedkvm_inj_exception: #UD (0x0)It appears that the task-switch emulation updates rflags (and vm86 flag) only after the segments are loaded, causing
vmx->emulation_requiredto be set, when in fact invalid guest state emulation is not needed.This patch fixes it by updating
vmx->emulation_requiredafter the rflags (and vm86 flag) is updated.Suggested-by Nadav Amit <[email protected]>Signed-off-by Wanpeng Li <[email protected]>[Wanpeng wrote the commit message with initial patch and Radim moved the update to
vmx_set_rflagsand added Paolo's suggestion for the check.]Signed-off-by Radim Krčmář <[email protected]>---
arch/x86/kvm/vmx.c| 15 ++++++++++-----1 file changed, 10 insertions(+), 5 deletions(-)arch/x86/kvm/vmx.c +7
-0@@ -2326,6 +2326,11 @@ static void vmx_vcpu_put(struct kvm_vcpu *vcpu)__vmx_load_host_state (to_vmx (vcpu)); } +static bool emulation_required (struct kvm_vcpu *vcpu) +{ +return emulate_invalid_guest_state && !guest_state_valid (vcpu); +} +static void vmx_decache_cr0_guest_bits (struct kvm_vcpu *vcpu);/* @@ -2363,6 +2368,8 @@ static unsigned long vmx_get_rflags(struct kvm_vcpu *vcpu)static void vmx_set_rflags (struct kvm_vcpu *vcpu,unsigned long rflags) { +unsigned long old_rflags =vmx_get_rflags (vcpu); +__set_bit (VCPU_EXREG_RFLAGS , (ulong *)&vcpu->arch.regs_avail);to_vmx (vcpu)->rflags = rflags;if (to_vmx (vcpu)->rmode.vm86_active) {@@ -2370,6 +2377,9 @@ static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) rflags |=X86_EFLAGS_IOPL |X86_EFLAGS_VM ; }Quoted 8 lines
arch/x86/kvm/vmx.c static void vmx_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags) · line 2382
Quoted 4 lines
arch/x86/kvm/vmx.c +3
-0vmcs_writel (GUEST_RFLAGS , rflags); + +if ((old_rflags ^ rflags) &X86_EFLAGS_VM ) +to_vmx (vcpu)->emulation_required =emulation_required (vcpu);Sorry for not pointing it before, but here you compare the
old_rflagswith the new rflags but after you already “massaged” it. So the value you compare with is not what the guest “sees”.Quoted 2 lines
So you mean we should use unsigned long
old_rflags=vmcs_readl(GUEST_RFLAGS); right?No. The problem is not with
old_rflagsnow, but with rflags. Ifvm86_active, then rflags is changed and you don’t compare the guest-visible rflags anymore.
Quoted 7 lines
Quoted 3 lines
Ah, I see. So we should compare the
old_flagswith thermode->save_rflags(guest-visible rflags)instead of the rflags (shadow rflags), right?Not exactly, since
rmode->save_rflagsare invalid if !vm86_active. Instead, I think you should have asave_rflagsvariable on the stack that would hold the rflags before “massaging” and use it instead.
Thanks for pointing out :) I will send out a new version, in addition, thanks Radim's help for these two versions. :)
Regards,
Wanpeng Li