Hi Jim, 2017-07-21 3:16 GMT+08:00 Jim Mattson <[email protected]>: > On Wed, Jul 19, 2017 at 7:31 PM, Wanpeng Li <[email protected]> wrote: >> Hi Jim, >> 2017-07-19 2:47 GMT+08:00 Jim Mattson <[email protected]>: >>> 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 <[email protected]> wrote: >>>> 2017-06-05 20:07 GMT+08:00 Paolo Bonzini <[email protected]>: >>>>> >>>>> >>>>> 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
