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

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

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

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

[Wanpeng Li]

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

[Wanpeng Li]

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

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

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

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

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

>
>> 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, 

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

[Wanpeng Li]

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

[Wanpeng Li]

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

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

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

[Wanpeng Li]

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

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

[Wanpeng Li]

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

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

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

[Wanpeng Li]

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

[Wanpeng Li]

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

[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

[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