Re: [PATCH v2 3/8] KVM: x86: set/get_events ioctl should consider only injected exceptions

[Liran Alon <LIRAN.ALON@ORACLE.COM>]

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 <LIRAN.ALON@oracle.com> wrote:
>>
>>
>> On 22/11/17 23:00, Jim Mattson wrote:
>>>
>>> On Tue, Nov 21, 2017 at 7:30 AM, Liran Alon <liran.alon@oracle.com> 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.
>>>
>>