[PATCH] KVM: nVMX: Fix exception injection

[PATCH] KVM: nVMX: Fix exception injection

[Wanpeng Li]

From: Wanpeng Li <wanpeng.li@hotmail.com>

 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 <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 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

[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;

?

Thanks,

Paolo

Re: [PATCH] KVM: nVMX: Fix exception injection

[Wanpeng Li]

2017-06-05 20:07 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> 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

[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?

On Mon, Jun 5, 2017 at 5:19 AM, Wanpeng Li <kernellwp@gmail.com> wrote:
> 2017-06-05 20:07 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>
>>
>> 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

[Wanpeng Li]

Hi Jim,
2017-07-19 2:47 GMT+08:00 Jim Mattson <jmattson@google.com>:
> 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 <kernellwp@gmail.com> wrote:
>> 2017-06-05 20:07 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>
>>>
>>> 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

[Jim Mattson]

On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
> Hi Jim,
> 2017-07-19 2:47 GMT+08:00 Jim Mattson <jmattson@google.com>:
>> 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 <kernellwp@gmail.com> wrote:
>>> 2017-06-05 20:07 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>>
>>>>
>>>> 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

[Wanpeng Li]

Hi Jim,
2017-07-21 3:16 GMT+08:00 Jim Mattson <jmattson@google.com>:
> On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>> Hi Jim,
>> 2017-07-19 2:47 GMT+08:00 Jim Mattson <jmattson@google.com>:
>>> 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 <kernellwp@gmail.com> wrote:
>>>> 2017-06-05 20:07 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>>>
>>>>>
>>>>> 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

[Jim Mattson]

On Fri, Jul 21, 2017 at 1:39 AM, Wanpeng Li <kernellwp@gmail.com> wrote:
> Hi Jim,
> 2017-07-21 3:16 GMT+08:00 Jim Mattson <jmattson@google.com>:
>> On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>>> Hi Jim,
>>> 2017-07-19 2:47 GMT+08:00 Jim Mattson <jmattson@google.com>:
>>>> 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 <kernellwp@gmail.com> wrote:
>>>>> 2017-06-05 20:07 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>>>>
>>>>>>
>>>>>> 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

[Wanpeng Li]

2017-07-22 22:25 GMT+08:00 Jim Mattson <jmattson@google.com>:
> On Fri, Jul 21, 2017 at 1:39 AM, Wanpeng Li <kernellwp@gmail.com> wrote:
>> Hi Jim,
>> 2017-07-21 3:16 GMT+08:00 Jim Mattson <jmattson@google.com>:
>>> On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>>>> Hi Jim,
>>>> 2017-07-19 2:47 GMT+08:00 Jim Mattson <jmattson@google.com>:
>>>>> 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

[Jim Mattson]

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 <kernellwp@gmail.com> wrote:
> 2017-07-22 22:25 GMT+08:00 Jim Mattson <jmattson@google.com>:
>> On Fri, Jul 21, 2017 at 1:39 AM, Wanpeng Li <kernellwp@gmail.com> wrote:
>>> Hi Jim,
>>> 2017-07-21 3:16 GMT+08:00 Jim Mattson <jmattson@google.com>:
>>>> On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li <kernellwp@gmail.com> wrote:
>>>>> Hi Jim,
>>>>> 2017-07-19 2:47 GMT+08:00 Jim Mattson <jmattson@google.com>:
>>>>>> 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

[Paolo Bonzini]

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