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

[Wanpeng Li]

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