From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liran Alon Subject: Re: [PATCH v2 3/8] KVM: x86: set/get_events ioctl should consider only injected exceptions Date: Mon, 27 Nov 2017 20:30:06 +0200 Message-ID: <5A1C59AE.4090503@ORACLE.COM> References: <1511278211-12257-1-git-send-email-liran.alon@oracle.com> <1511278211-12257-4-git-send-email-liran.alon@oracle.com> <5A17175A.6070307@ORACLE.COM> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Paolo Bonzini , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , kvm list , Wanpeng Li , Idan Brown , Krish Sadhukhan To: Jim Mattson Return-path: Received: from userp1040.oracle.com ([156.151.31.81]:33611 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752959AbdK0SaS (ORCPT ); Mon, 27 Nov 2017 13:30:18 -0500 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 27/11/17 19:26, Jim Mattson wrote: > I think we need some way for userspace to indicate whether or not it > can deal with pending events (with side effects recorded in > kvm_vcpu_events.reserved[0] and kvm_vcpu_events.reserved[1]) when it > issues a KVM_GET_VCPU_EVENTS ioctl. That seems to argue for a new flag > bit in kvm_vcpu_events.flags (as input to KVM_GET_VCPU_EVENTS) and a > capability indicating that the flag can be set by userspace. Yep I agree. I will do the relevant changes for the next version of this series. Thanks, -Liran > > On Thu, Nov 23, 2017 at 10:45 AM, Liran Alon wrote: >> >> >> On 22/11/17 23:00, Jim Mattson wrote: >>> >>> On Tue, Nov 21, 2017 at 7:30 AM, Liran Alon wrote: >>> >>>> Being symmetrical, injecting an exception from user-mode should >>>> consider injected exception as in "injected" state and not in >>>> "pending" state. >>> >>> >>> I disagree with this contention. Suppose, for example, that we are >>> executing in a nested context (i.e. vmcs02 is live) and usermode >>> "injects" a page-fault. The page fault may be delivered on L2's IDT or >>> it may cause an emulated VM-exit from L2 to L1, depending on the page >>> fault error code, the exception bitmap in the vmcs12, and the >>> page-fault error-code mask and match fields in the vmcs12. If the page >>> fault is delivered on L2's IDT, then the exception can be considered >>> as "injected," with the CR2 side effect already processed. However, if >>> the page fault causes an emulated VM-exit from L2 to L1, then it must >>> be considered as "pending" and the CR2 side effect must not have been >>> processed. >> >> I understand you are referring here to the FIXME comment that is present on >> nested_vmx_check_exception()? >>> >>> >>> So, where do we find the new CR2 value? Admittedly, the existing >>> KVM_SET_VCPU_EVENTS API is broken and appears to assume that the side >>> effects have already been performed for exceptions injected from >>> userspace, though this assumption isn't documented AFAICT. >>> Fortunately, there's enough padding in the kvm_vcpu_events structure >>> to fix the API (with the possible exception of injected #VE*): one bit >>> to indicate that userspace is providing the side effects in the events >>> structure, and 64 bits for the new CR2 value (#PF) or the new DR6 bits >>> (#DB). >> >> I see. Then what do you think about the following change: >> 1. Rename kvm_vcpu_events.exception.injected to >> kvm_vcpu_events.exception.raised and remain still to be the logical OR of >> "exception.pending | exception.injected" as of today (to not break backwards >> compatibility). >> 2. Add a new flag to kvm_vcpu_events.flags to indicate if >> kvm_vcpu_events.exception.raised is actually exception.pending or >> exception.injected (they are mutually exclusive). A value of 0 will be >> considered as "injected" to preserve backwards compatibility. >> 3. Use kvm_vcpu_events.reserved[0] and kvm_vcpu_events.reserved[1] for >> exception_extra_info which will be either CR2 for #PF or DR6 for #DB. >> 4. Add to kvm_queued_exception() a u64 exception_extra_info that will either >> be CR2 for #PF or DR6 for #DB. Make sure that these will be set on relevant >> places and filled to vcpu.arch.cr2/VMCS only on inject_pending_event() >> injection of a pending exception. >> >> I think that in the above proposal, we don't need to be conditional on a new >> capability because old user-space shouldn't be affected. >> (They will still get same value in kvm_vcpu_events.exception.raised and the >> rest were ignored fields). >> >> What do you think? >> >> Thanks for the excellent review! >> -Liran >> >>> >>> * One could argue that userspace should not be delivering a #VE >>> directly, but should be injecting an EPT Violation instead. >>> >>