[PATCH] KVM: nVMX: Fix exception injection
From: Wanpeng Li WARNING: CPU: 3 PID: 2840 at arch/x86/kvm/vmx.c:10966 nested_vmx_vmexit+0xdcd/0xde0 [kvm_intel] CPU: 3 PID: 2840 Comm: qemu-system-x86 Tainted: G OE 4.12.0-rc3+ #23 RIP: 0010:nested_vmx_vmexit+0xdcd/0xde0 [kvm_intel] Call Trace: ? kvm_check_async_pf_completion+0xef/0x120 [kvm] ? rcu_read_lock_sched_held+0x79/0x80 vmx_queue_exception+0x104/0x160 [kvm_intel] ? vmx_queue_exception+0x104/0x160 [kvm_intel] kvm_arch_vcpu_ioctl_run+0x1171/0x1ce0 [kvm] ? kvm_arch_vcpu_load+0x47/0x240 [kvm] ? kvm_arch_vcpu_load+0x62/0x240 [kvm] kvm_vcpu_ioctl+0x384/0x7b0 [kvm] ? kvm_vcpu_ioctl+0x384/0x7b0 [kvm] ? __fget+0xf3/0x210 do_vfs_ioctl+0xa4/0x700 ? __fget+0x114/0x210 SyS_ioctl+0x79/0x90 do_syscall_64+0x81/0x220 entry_SYSCALL64_slow_path+0x25/0x25 This is triggered occasionally by running both win7 and win2016 in L2, in addition, EPT is disabled on both L1 and L2. It can't be reproduced easily. Commit 0b6ac343fc (KVM: nVMX: Correct handling of exception injection) mentioned that "KVM wants to inject page-faults which it got to the guest. This function assumes it is called with the exit reason in vmcs02 being a #PF exception". Commit e011c663 (KVM: nVMX: Check all exceptions for intercept during delivery to L2) allows to check all exceptions for intercept during delivery to L2. However, there is no guarantee the exit reason is exception currently, when there is an external interrupt occurred on host, maybe a time interrupt for host which should not be injected to guest, and somewhere queues an exception, then the function nested_vmx_check_exception() will be called and the vmexit emulation codes will try to emulate the "Acknowledge interrupt on exit" behavior, the warning is triggered. This patch fixes it by confirming to inject exception to the guest when the exit reason in vmcs02 is exception. Cc: Paolo Bonzini Cc: Radim Krčmář Signed-off-by: Wanpeng Li --- arch/x86/kvm/vmx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 9b4b5d6..778a8f3 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2422,7 +2422,8 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned nr) { struct vmcs12 *vmcs12 = get_vmcs12(vcpu); - if (!(vmcs12->exception_bitmap & (1u << nr))) + if (to_vmx(vcpu)->exit_reason != EXIT_REASON_EXCEPTION_NMI || + !(vmcs12->exception_bitmap & (1u << nr))) return 0; nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, -- 2.7.4
Re: [PATCH] KVM: nVMX: Fix exception injection
On 03/06/2017 05:21, Wanpeng Li wrote: > Commit 0b6ac343fc (KVM: nVMX: Correct handling of exception injection) > mentioned > that "KVM wants to inject page-faults which it got to the guest. This > function > assumes it is called with the exit reason in vmcs02 being a #PF exception". > Commit e011c663 (KVM: nVMX: Check all exceptions for intercept during > delivery to > L2) allows to check all exceptions for intercept during delivery to L2. > However, > there is no guarantee the exit reason is exception currently, when there is > an > external interrupt occurred on host, maybe a time interrupt for host which > should > not be injected to guest, and somewhere queues an exception, then the > function > nested_vmx_check_exception() will be called and the vmexit emulation codes > will > try to emulate the "Acknowledge interrupt on exit" behavior, the warning is > triggered. > > This patch fixes it by confirming to inject exception to the guest when the > exit > reason in vmcs02 is exception. I am confused. On one hand, the comment originally "this is the only case in which KVM injects a #PF when L2 is running", but I'm not sure it's true. For example, KVM could emulate a movs while running L2. If the source is MMIO and the destination is a missing page, the original failure could be an EPT misconfig, but the access to the destination would cause a #PF in the guest (could be a nice testcase for kvm-unit-tests, BTW :)). On the other hand, why would you reuse to_vmx(vcpu)->exit_reason in nested_vmx_check_exception? Would the following fix the bug: diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 9b4b5d6dcd34..ca5d2b93385c 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2425,7 +2425,7 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned nr) if (!(vmcs12->exception_bitmap & (1u << nr))) return 0; - nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, + nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI, vmcs_read32(VM_EXIT_INTR_INFO), vmcs_readl(EXIT_QUALIFICATION)); return 1; ? Thanks, Paolo
Re: [PATCH] KVM: nVMX: Fix exception injection
2017-06-05 20:07 GMT+08:00 Paolo Bonzini : > > > On 03/06/2017 05:21, Wanpeng Li wrote: >> Commit 0b6ac343fc (KVM: nVMX: Correct handling of exception injection) >> mentioned >> that "KVM wants to inject page-faults which it got to the guest. This >> function >> assumes it is called with the exit reason in vmcs02 being a #PF exception". >> Commit e011c663 (KVM: nVMX: Check all exceptions for intercept during >> delivery to >> L2) allows to check all exceptions for intercept during delivery to L2. >> However, >> there is no guarantee the exit reason is exception currently, when there is >> an >> external interrupt occurred on host, maybe a time interrupt for host which >> should >> not be injected to guest, and somewhere queues an exception, then the >> function >> nested_vmx_check_exception() will be called and the vmexit emulation codes >> will >> try to emulate the "Acknowledge interrupt on exit" behavior, the warning is >> triggered. >> >> This patch fixes it by confirming to inject exception to the guest when the >> exit >> reason in vmcs02 is exception. > > I am confused. On one hand, the comment originally "this is the only > case in which KVM injects a #PF when L2 is running", but I'm not sure > it's true. For example, KVM could emulate a movs while running L2. If > the source is MMIO and the destination is a missing page, the original > failure could be an EPT misconfig, but the access to the destination > would cause a #PF in the guest (could be a nice testcase for > kvm-unit-tests, BTW :)). > > On the other hand, why would you reuse to_vmx(vcpu)->exit_reason in > nested_vmx_check_exception? Would the following fix the bug: > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 9b4b5d6dcd34..ca5d2b93385c 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -2425,7 +2425,7 @@ static int nested_vmx_check_exception(struct > kvm_vcpu *vcpu, unsigned nr) > if (!(vmcs12->exception_bitmap & (1u << nr))) > return 0; > > - nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, > + nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI, > vmcs_read32(VM_EXIT_INTR_INFO), > vmcs_readl(EXIT_QUALIFICATION)); > return 1; > You are right. Regards, Wanpeng Li
Re: [PATCH] KVM: nVMX: Fix exception injection
On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li wrote: > Hi Jim, > 2017-07-19 2:47 GMT+08:00 Jim Mattson : >> Why do we expect the VM_EXIT_INTR_INFO and EXIT_QUALIFICATION fields >> of the VMCS to have the correct values for the injected exception? > > Good point, I think we should synthesize VM_EXIT_INTR_INFO and > EXIT_QUALIFICATION manually, I will post a patch for it. Btw, how > about setting EXIT_QULIFICATION to vcpu->arch.cr2 for the page fault > exception and 0 for other exceptions? >From the SDM, section 27.1: If an event causes a VM exit directly, it does not update architectural state as it would have if it had it not caused the VM exit: - A debug exception does not update DR6, DR7.GD, or IA32_DEBUGCTL.LBR. (Information about the nature of the debug exception is saved in the exit qualification field.) - A page fault does not update CR2. (The linear address causing the page fault is saved in the exit-qualification field.) This means that vcpu->arch.cr2 should not be set at this point for a #PF injection (and vcpu->arch.dr6 should not be set at this point for a #DB injection). For all other exceptions, yes, the exit qualification should be cleared. > > Regards, > Wanpeng Li > >> >> On Mon, Jun 5, 2017 at 5:19 AM, Wanpeng Li wrote: >>> 2017-06-05 20:07 GMT+08:00 Paolo Bonzini : On 03/06/2017 05:21, Wanpeng Li wrote: > Commit 0b6ac343fc (KVM: nVMX: Correct handling of exception injection) > mentioned > that "KVM wants to inject page-faults which it got to the guest. This > function > assumes it is called with the exit reason in vmcs02 being a #PF > exception". > Commit e011c663 (KVM: nVMX: Check all exceptions for intercept during > delivery to > L2) allows to check all exceptions for intercept during delivery to L2. > However, > there is no guarantee the exit reason is exception currently, when there > is an > external interrupt occurred on host, maybe a time interrupt for host > which should > not be injected to guest, and somewhere queues an exception, then the > function > nested_vmx_check_exception() will be called and the vmexit emulation > codes will > try to emulate the "Acknowledge interrupt on exit" behavior, the warning > is > triggered. > > This patch fixes it by confirming to inject exception to the guest when > the exit > reason in vmcs02 is exception. I am confused. On one hand, the comment originally "this is the only case in which KVM injects a #PF when L2 is running", but I'm not sure it's true. For example, KVM could emulate a movs while running L2. If the source is MMIO and the destination is a missing page, the original failure could be an EPT misconfig, but the access to the destination would cause a #PF in the guest (could be a nice testcase for kvm-unit-tests, BTW :)). On the other hand, why would you reuse to_vmx(vcpu)->exit_reason in nested_vmx_check_exception? Would the following fix the bug: diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 9b4b5d6dcd34..ca5d2b93385c 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -2425,7 +2425,7 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned nr) if (!(vmcs12->exception_bitmap & (1u << nr))) return 0; Here, we should consult vmcs12->page_fault_error_code_mask and vmcs12->page_fault_error_code_match when nr==PF_VECTOR, as in nested_vmx_is_page_fault_vmexit(). - nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, + nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI, vmcs_read32(VM_EXIT_INTR_INFO), vmcs_readl(EXIT_QUALIFICATION)); return 1; >>> >>> You are right. >>> >>> Regards, >>> Wanpeng Li
Re: [PATCH] KVM: nVMX: Fix exception injection
Hi Jim, 2017-07-21 3:16 GMT+08:00 Jim Mattson : > On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li wrote: >> Hi Jim, >> 2017-07-19 2:47 GMT+08:00 Jim Mattson : >>> Why do we expect the VM_EXIT_INTR_INFO and EXIT_QUALIFICATION fields >>> of the VMCS to have the correct values for the injected exception? >> >> Good point, I think we should synthesize VM_EXIT_INTR_INFO and >> EXIT_QUALIFICATION manually, I will post a patch for it. Btw, how >> about setting EXIT_QULIFICATION to vcpu->arch.cr2 for the page fault >> exception and 0 for other exceptions? > > From the SDM, section 27.1: > > If an event causes a VM exit directly, it does not update I mentioned this in the patch description: > However, there is no guarantee the exit reason is exception currently, when > there is an external interrupt occurred on host, maybe a time interrupt for > host which should not be injected to guest, and somewhere queues an > exception, then the function nested_vmx_check_exception() will be called and > the vmexit emulation codes will try to emulate the "Acknowledge interrupt on > exit" behavior, the warning is triggered. If you think the scenario is correct, then it should be an event causes a VM exit indirectly. So if both the scenario which I mentioned and "This function assumes it is called with the exit reason in vmcs02 being a #PF exception" can happen, then maybe we should figure out how to fix both scenarios suitable. > architectural state as it would have if it had it not caused the VM > exit: > - A debug exception does not update DR6, DR7.GD, or > IA32_DEBUGCTL.LBR. (Information about the nature of the debug > exception is saved in the exit qualification field.) > - A page fault does not update CR2. (The linear address causing > the page fault is saved in the exit-qualification field.) > > This means that vcpu->arch.cr2 should not be set at this point for a > #PF injection (and vcpu->arch.dr6 should not be set at this point for > a #DB injection). For all other exceptions, yes, the exit > qualification should be cleared. > >> >> Regards, >> Wanpeng Li >> >>> >>> On Mon, Jun 5, 2017 at 5:19 AM, Wanpeng Li wrote: 2017-06-05 20:07 GMT+08:00 Paolo Bonzini : > > > On 03/06/2017 05:21, Wanpeng Li wrote: >> Commit 0b6ac343fc (KVM: nVMX: Correct handling of exception injection) >> mentioned >> that "KVM wants to inject page-faults which it got to the guest. This >> function >> assumes it is called with the exit reason in vmcs02 being a #PF >> exception". >> Commit e011c663 (KVM: nVMX: Check all exceptions for intercept during >> delivery to >> L2) allows to check all exceptions for intercept during delivery to L2. >> However, >> there is no guarantee the exit reason is exception currently, when there >> is an >> external interrupt occurred on host, maybe a time interrupt for host >> which should >> not be injected to guest, and somewhere queues an exception, then the >> function >> nested_vmx_check_exception() will be called and the vmexit emulation >> codes will >> try to emulate the "Acknowledge interrupt on exit" behavior, the warning >> is >> triggered. >> >> This patch fixes it by confirming to inject exception to the guest when >> the exit >> reason in vmcs02 is exception. > > I am confused. On one hand, the comment originally "this is the only > case in which KVM injects a #PF when L2 is running", but I'm not sure > it's true. For example, KVM could emulate a movs while running L2. If > the source is MMIO and the destination is a missing page, the original > failure could be an EPT misconfig, but the access to the destination > would cause a #PF in the guest (could be a nice testcase for > kvm-unit-tests, BTW :)). > > On the other hand, why would you reuse to_vmx(vcpu)->exit_reason in > nested_vmx_check_exception? Would the following fix the bug: > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 9b4b5d6dcd34..ca5d2b93385c 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -2425,7 +2425,7 @@ static int nested_vmx_check_exception(struct > kvm_vcpu *vcpu, unsigned nr) > if (!(vmcs12->exception_bitmap & (1u << nr))) > return 0; > > Here, we should consult vmcs12->page_fault_error_code_mask and > vmcs12->page_fault_error_code_match when nr==PF_VECTOR, as in > nested_vmx_is_page_fault_vmexit(). Yeah, it should be done for "This function assumes it is called with the exit reason in vmcs02 being a #PF exception". Regards, Wanpeng Li
Re: [PATCH] KVM: nVMX: Fix exception injection
On Fri, Jul 21, 2017 at 1:39 AM, Wanpeng Li wrote: > Hi Jim, > 2017-07-21 3:16 GMT+08:00 Jim Mattson : >> On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li wrote: >>> Hi Jim, >>> 2017-07-19 2:47 GMT+08:00 Jim Mattson : Why do we expect the VM_EXIT_INTR_INFO and EXIT_QUALIFICATION fields of the VMCS to have the correct values for the injected exception? >>> >>> Good point, I think we should synthesize VM_EXIT_INTR_INFO and >>> EXIT_QUALIFICATION manually, I will post a patch for it. Btw, how >>> about setting EXIT_QULIFICATION to vcpu->arch.cr2 for the page fault >>> exception and 0 for other exceptions? >> >> From the SDM, section 27.1: >> >> If an event causes a VM exit directly, it does not update > > I mentioned this in the patch description: > >> However, there is no guarantee the exit reason is exception currently, when >> there is an external interrupt occurred on host, maybe a time interrupt for >> host which should not be injected to guest, and somewhere queues an >> exception, then the function nested_vmx_check_exception() will be called and >> the vmexit emulation codes will try to emulate the "Acknowledge interrupt on >> exit" behavior, the warning is triggered. > > If you think the scenario is correct, then it should be an event > causes a VM exit indirectly. So if both the scenario which I mentioned > and "This function > assumes it is called with the exit reason in vmcs02 being a #PF > exception" can happen, then maybe we should figure out how to fix both > scenarios suitable. In the situation you describe, the #PF causes a synthesized VM-exit from L2 to L1 directly, not indirectly. From the SDM: An exception causes a VM exit directly if the bit corresponding to that exception is set in the exception bitmap. Hence, CR2 should not be set yet. > >> architectural state as it would have if it had it not caused the VM >> exit: >> - A debug exception does not update DR6, DR7.GD, or >> IA32_DEBUGCTL.LBR. (Information about the nature of the debug >> exception is saved in the exit qualification field.) >> - A page fault does not update CR2. (The linear address causing >> the page fault is saved in the exit-qualification field.) >> >> This means that vcpu->arch.cr2 should not be set at this point for a >> #PF injection (and vcpu->arch.dr6 should not be set at this point for >> a #DB injection). For all other exceptions, yes, the exit >> qualification should be cleared. >> >>> >>> Regards, >>> Wanpeng Li >>> On Mon, Jun 5, 2017 at 5:19 AM, Wanpeng Li wrote: > 2017-06-05 20:07 GMT+08:00 Paolo Bonzini : >> >> >> On 03/06/2017 05:21, Wanpeng Li wrote: >>> Commit 0b6ac343fc (KVM: nVMX: Correct handling of exception injection) >>> mentioned >>> that "KVM wants to inject page-faults which it got to the guest. This >>> function >>> assumes it is called with the exit reason in vmcs02 being a #PF >>> exception". >>> Commit e011c663 (KVM: nVMX: Check all exceptions for intercept during >>> delivery to >>> L2) allows to check all exceptions for intercept during delivery to L2. >>> However, >>> there is no guarantee the exit reason is exception currently, when >>> there is an >>> external interrupt occurred on host, maybe a time interrupt for host >>> which should >>> not be injected to guest, and somewhere queues an exception, then the >>> function >>> nested_vmx_check_exception() will be called and the vmexit emulation >>> codes will >>> try to emulate the "Acknowledge interrupt on exit" behavior, the >>> warning is >>> triggered. >>> >>> This patch fixes it by confirming to inject exception to the guest when >>> the exit >>> reason in vmcs02 is exception. >> >> I am confused. On one hand, the comment originally "this is the only >> case in which KVM injects a #PF when L2 is running", but I'm not sure >> it's true. For example, KVM could emulate a movs while running L2. If >> the source is MMIO and the destination is a missing page, the original >> failure could be an EPT misconfig, but the access to the destination >> would cause a #PF in the guest (could be a nice testcase for >> kvm-unit-tests, BTW :)). >> >> On the other hand, why would you reuse to_vmx(vcpu)->exit_reason in >> nested_vmx_check_exception? Would the following fix the bug: >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 9b4b5d6dcd34..ca5d2b93385c 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -2425,7 +2425,7 @@ static int nested_vmx_check_exception(struct >> kvm_vcpu *vcpu, unsigned nr) >> if (!(vmcs12->exception_bitmap & (1u << nr))) >> return 0; >> >> Here, we should consult vmcs12->page_fault_error_code_mask and >> vmcs12->page_fault_error_code_match when nr==PF_VECTOR, as in >> nested_vmx_is_page_fault_vmexit(). > > Yeah, i
Re: [PATCH] KVM: nVMX: Fix exception injection
2017-07-22 22:25 GMT+08:00 Jim Mattson : > On Fri, Jul 21, 2017 at 1:39 AM, Wanpeng Li wrote: >> Hi Jim, >> 2017-07-21 3:16 GMT+08:00 Jim Mattson : >>> On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li wrote: Hi Jim, 2017-07-19 2:47 GMT+08:00 Jim Mattson : > Why do we expect the VM_EXIT_INTR_INFO and EXIT_QUALIFICATION fields > of the VMCS to have the correct values for the injected exception? Good point, I think we should synthesize VM_EXIT_INTR_INFO and EXIT_QUALIFICATION manually, I will post a patch for it. Btw, how about setting EXIT_QULIFICATION to vcpu->arch.cr2 for the page fault exception and 0 for other exceptions? >>> >>> From the SDM, section 27.1: >>> >>> If an event causes a VM exit directly, it does not update >> >> I mentioned this in the patch description: >> >>> However, there is no guarantee the exit reason is exception currently, when >>> there is an external interrupt occurred on host, maybe a time interrupt for >>> host which should not be injected to guest, and somewhere queues an >>> exception, then the function nested_vmx_check_exception() will be called >>> and the vmexit emulation codes will try to emulate the "Acknowledge >>> interrupt on exit" behavior, the warning is triggered. >> >> If you think the scenario is correct, then it should be an event >> causes a VM exit indirectly. So if both the scenario which I mentioned >> and "This function >> assumes it is called with the exit reason in vmcs02 being a #PF >> exception" can happen, then maybe we should figure out how to fix both >> scenarios suitable. > > In the situation you describe, the #PF causes a synthesized VM-exit > from L2 to L1 directly, not indirectly. From the SDM: > >An exception causes a VM exit directly if the bit corresponding to > that exception is set in the exception bitmap. > > Hence, CR2 should not be set yet. Any idea how to synthesize exit qualification for page fault and debug exception? Regards, Wanpeng Li > >> >>> architectural state as it would have if it had it not caused the VM >>> exit: >>> - A debug exception does not update DR6, DR7.GD, or >>> IA32_DEBUGCTL.LBR. (Information about the nature of the debug >>> exception is saved in the exit qualification field.) >>> - A page fault does not update CR2. (The linear address causing >>> the page fault is saved in the exit-qualification field.) >>> >>> This means that vcpu->arch.cr2 should not be set at this point for a >>> #PF injection (and vcpu->arch.dr6 should not be set at this point for >>> a #DB injection). For all other exceptions, yes, the exit >>> qualification should be cleared. >>>
Re: [PATCH] KVM: nVMX: Fix exception injection
I think the ancillary data for #DB and #PF should be added to kvm_queued_exception and plumbed through to where it's needed. Vector number and error code are not sufficient to describe a #DB or #PF. On Sat, Jul 22, 2017 at 5:29 PM, Wanpeng Li wrote: > 2017-07-22 22:25 GMT+08:00 Jim Mattson : >> On Fri, Jul 21, 2017 at 1:39 AM, Wanpeng Li wrote: >>> Hi Jim, >>> 2017-07-21 3:16 GMT+08:00 Jim Mattson : On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li wrote: > Hi Jim, > 2017-07-19 2:47 GMT+08:00 Jim Mattson : >> Why do we expect the VM_EXIT_INTR_INFO and EXIT_QUALIFICATION fields >> of the VMCS to have the correct values for the injected exception? > > Good point, I think we should synthesize VM_EXIT_INTR_INFO and > EXIT_QUALIFICATION manually, I will post a patch for it. Btw, how > about setting EXIT_QULIFICATION to vcpu->arch.cr2 for the page fault > exception and 0 for other exceptions? From the SDM, section 27.1: If an event causes a VM exit directly, it does not update >>> >>> I mentioned this in the patch description: >>> However, there is no guarantee the exit reason is exception currently, when there is an external interrupt occurred on host, maybe a time interrupt for host which should not be injected to guest, and somewhere queues an exception, then the function nested_vmx_check_exception() will be called and the vmexit emulation codes will try to emulate the "Acknowledge interrupt on exit" behavior, the warning is triggered. >>> >>> If you think the scenario is correct, then it should be an event >>> causes a VM exit indirectly. So if both the scenario which I mentioned >>> and "This function >>> assumes it is called with the exit reason in vmcs02 being a #PF >>> exception" can happen, then maybe we should figure out how to fix both >>> scenarios suitable. >> >> In the situation you describe, the #PF causes a synthesized VM-exit >> from L2 to L1 directly, not indirectly. From the SDM: >> >>An exception causes a VM exit directly if the bit corresponding to >> that exception is set in the exception bitmap. >> >> Hence, CR2 should not be set yet. > > Any idea how to synthesize exit qualification for page fault and debug > exception? > > Regards, > Wanpeng Li > >> >>> architectural state as it would have if it had it not caused the VM exit: - A debug exception does not update DR6, DR7.GD, or IA32_DEBUGCTL.LBR. (Information about the nature of the debug exception is saved in the exit qualification field.) - A page fault does not update CR2. (The linear address causing the page fault is saved in the exit-qualification field.) This means that vcpu->arch.cr2 should not be set at this point for a #PF injection (and vcpu->arch.dr6 should not be set at this point for a #DB injection). For all other exceptions, yes, the exit qualification should be cleared.
Re: [PATCH] KVM: nVMX: Fix exception injection
On 23/07/2017 03:11, Jim Mattson wrote: >>> In the situation you describe, the #PF causes a synthesized VM-exit >>> from L2 to L1 directly, not indirectly. From the SDM: >>> >>>An exception causes a VM exit directly if the bit corresponding to >>> that exception is set in the exception bitmap. >>> >>> Hence, CR2 should not be set yet. >> >> Any idea how to synthesize exit qualification for page fault and debug >> exception? > > I think the ancillary data for #DB and #PF should be added to > kvm_queued_exception and plumbed through to where it's needed. Vector > number and error code are not sufficient to describe a #DB or #PF. It's more complicated than that, because you'd have to copy it out to userspace in KVM_GET_VCPU_EVENTS. But I agree it's the way to go: CR2 and DR6 should be only by inject_pending_event. Paolo
Re: [PATCH] KVM: nVMX: Fix exception injection
Why do we expect the VM_EXIT_INTR_INFO and EXIT_QUALIFICATION fields of the VMCS to have the correct values for the injected exception? On Mon, Jun 5, 2017 at 5:19 AM, Wanpeng Li wrote: > 2017-06-05 20:07 GMT+08:00 Paolo Bonzini : >> >> >> On 03/06/2017 05:21, Wanpeng Li wrote: >>> Commit 0b6ac343fc (KVM: nVMX: Correct handling of exception injection) >>> mentioned >>> that "KVM wants to inject page-faults which it got to the guest. This >>> function >>> assumes it is called with the exit reason in vmcs02 being a #PF exception". >>> Commit e011c663 (KVM: nVMX: Check all exceptions for intercept during >>> delivery to >>> L2) allows to check all exceptions for intercept during delivery to L2. >>> However, >>> there is no guarantee the exit reason is exception currently, when there is >>> an >>> external interrupt occurred on host, maybe a time interrupt for host which >>> should >>> not be injected to guest, and somewhere queues an exception, then the >>> function >>> nested_vmx_check_exception() will be called and the vmexit emulation codes >>> will >>> try to emulate the "Acknowledge interrupt on exit" behavior, the warning is >>> triggered. >>> >>> This patch fixes it by confirming to inject exception to the guest when the >>> exit >>> reason in vmcs02 is exception. >> >> I am confused. On one hand, the comment originally "this is the only >> case in which KVM injects a #PF when L2 is running", but I'm not sure >> it's true. For example, KVM could emulate a movs while running L2. If >> the source is MMIO and the destination is a missing page, the original >> failure could be an EPT misconfig, but the access to the destination >> would cause a #PF in the guest (could be a nice testcase for >> kvm-unit-tests, BTW :)). >> >> On the other hand, why would you reuse to_vmx(vcpu)->exit_reason in >> nested_vmx_check_exception? Would the following fix the bug: >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 9b4b5d6dcd34..ca5d2b93385c 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -2425,7 +2425,7 @@ static int nested_vmx_check_exception(struct >> kvm_vcpu *vcpu, unsigned nr) >> if (!(vmcs12->exception_bitmap & (1u << nr))) >> return 0; >> >> - nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, >> + nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI, >> vmcs_read32(VM_EXIT_INTR_INFO), >> vmcs_readl(EXIT_QUALIFICATION)); >> return 1; >> > > You are right. > > Regards, > Wanpeng Li
Re: [PATCH] KVM: nVMX: Fix exception injection
Hi Jim, 2017-07-19 2:47 GMT+08:00 Jim Mattson : > Why do we expect the VM_EXIT_INTR_INFO and EXIT_QUALIFICATION fields > of the VMCS to have the correct values for the injected exception? Good point, I think we should synthesize VM_EXIT_INTR_INFO and EXIT_QUALIFICATION manually, I will post a patch for it. Btw, how about setting EXIT_QULIFICATION to vcpu->arch.cr2 for the page fault exception and 0 for other exceptions? Regards, Wanpeng Li > > On Mon, Jun 5, 2017 at 5:19 AM, Wanpeng Li wrote: >> 2017-06-05 20:07 GMT+08:00 Paolo Bonzini : >>> >>> >>> On 03/06/2017 05:21, Wanpeng Li wrote: Commit 0b6ac343fc (KVM: nVMX: Correct handling of exception injection) mentioned that "KVM wants to inject page-faults which it got to the guest. This function assumes it is called with the exit reason in vmcs02 being a #PF exception". Commit e011c663 (KVM: nVMX: Check all exceptions for intercept during delivery to L2) allows to check all exceptions for intercept during delivery to L2. However, there is no guarantee the exit reason is exception currently, when there is an external interrupt occurred on host, maybe a time interrupt for host which should not be injected to guest, and somewhere queues an exception, then the function nested_vmx_check_exception() will be called and the vmexit emulation codes will try to emulate the "Acknowledge interrupt on exit" behavior, the warning is triggered. This patch fixes it by confirming to inject exception to the guest when the exit reason in vmcs02 is exception. >>> >>> I am confused. On one hand, the comment originally "this is the only >>> case in which KVM injects a #PF when L2 is running", but I'm not sure >>> it's true. For example, KVM could emulate a movs while running L2. If >>> the source is MMIO and the destination is a missing page, the original >>> failure could be an EPT misconfig, but the access to the destination >>> would cause a #PF in the guest (could be a nice testcase for >>> kvm-unit-tests, BTW :)). >>> >>> On the other hand, why would you reuse to_vmx(vcpu)->exit_reason in >>> nested_vmx_check_exception? Would the following fix the bug: >>> >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>> index 9b4b5d6dcd34..ca5d2b93385c 100644 >>> --- a/arch/x86/kvm/vmx.c >>> +++ b/arch/x86/kvm/vmx.c >>> @@ -2425,7 +2425,7 @@ static int nested_vmx_check_exception(struct >>> kvm_vcpu *vcpu, unsigned nr) >>> if (!(vmcs12->exception_bitmap & (1u << nr))) >>> return 0; >>> >>> - nested_vmx_vmexit(vcpu, to_vmx(vcpu)->exit_reason, >>> + nested_vmx_vmexit(vcpu, EXIT_REASON_EXCEPTION_NMI, >>> vmcs_read32(VM_EXIT_INTR_INFO), >>> vmcs_readl(EXIT_QUALIFICATION)); >>> return 1; >>> >> >> You are right. >> >> Regards, >> Wanpeng Li