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: Thu, 23 Nov 2017 20:45:46 +0200 Message-ID: <5A17175A.6070307@ORACLE.COM> References: <1511278211-12257-1-git-send-email-liran.alon@oracle.com> <1511278211-12257-4-git-send-email-liran.alon@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]:50427 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753195AbdKWSp4 (ORCPT ); Thu, 23 Nov 2017 13:45:56 -0500 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 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. >