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

[Jim Mattson]

On Fri, Jul 21, 2017 at 1:39 AM, Wanpeng Li <kernellwp@xxxxxxxxx> wrote:
> 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.

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