> On 9 Oct 2018, at 17:01, Jim Mattson <jmattson@xxxxxxxxxx> wrote:
>
> I'm happy to do the kvm-unit-tests for (1) and (2). The subtlety of
> exception.pending and exception.injected is lost on me. We do need to
> handle pending debug exceptions in a MOV-SS shadow, but I don't think
> that's what you're talking about. Can you explain?
Today, KVM_GET_VCPU_EVENTS IOCTL returns in events->exception.injected
the value of (vcpu->arch.exception.pending || vcpu->arch.exception.injected).
Which means userspace have no way of knowing if exception was re-injected
and thus cannot be intercepted by L1 or it is still pending and therefore can be intercepted.
Similarly, KVM_SET_VCPU_EVENTS IOCTL sets cpu->arch.exception.pending
to the value of events->exception.injected. Which means userspace only have the
ability to inject exceptions which cannot be intercepted by a L1 guest.
Before this series, userspace must have assumed that exception cannot be intercepted
as exception side-effects (payload) is assumed to already been done by userspace.
Now however, it is possible for userspace to set a pending exception via IOCTL together
with it’s pending exception such that it can be correctly intercepted by L1.
-Liran
>
> On Tue, Oct 9, 2018 at 5:33 AM, Liran Alon <liran.alon@xxxxxxxxxx> wrote:
>>
>>
>>> On 8 Oct 2018, at 21:29, Jim Mattson <jmattson@xxxxxxxxxx> wrote:
>>>
>>> This is a per-VM capability which can be enabled by userspace so that
>>> the faulting linear address will be included with the information
>>> about a pending #PF in L2, and the "new DR6 bits" will be included
>>> with the information about a pending #DB in L2. With this capability
>>> enabled, the L1 hypervisor can now intercept #PF before CR2 is
>>> modified. Under VMX, the L1 hypervisor can now intercept #DB before
>>> DR6 and DR7 are modified.
>>>
>>> When userspace has enabled KVM_CAP_EXCEPTION_PAYLOAD, it should
>>> generally provide an appropriate payload when injecting a #PF or #DB
>>> exception via KVM_SET_VCPU_EVENTS. However, to support restoring old
>>> checkpoints, this payload is not required.
>>>
>>> Note that bit 16 of the "new DR6 bits" is set to indicate that a debug
>>> exception (#DB) or a breakpoint exception (#BP) occurred inside an RTM
>>> region while advanced debugging of RTM transactional regions was
>>> enabled. This is the reverse of DR6.RTM, which is cleared in this
>>> scenario.
>>>
>>> Reported-by: Jim Mattson <jmattson@xxxxxxxxxx>
>>> Suggested-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>>> Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
>>> Reviewed-by: Peter Shier <pshier@xxxxxxxxxx>
>>
>> Patch itself looks fine:
>> Reviewed-by: Liran Alon <liran.alon@xxxxxxxxxx>
>>
>> A couple of general notes regarding series:
>> 1) I saw that kvm-unit-test 414bd9d5ebd7 ("x86: nVMX: Basic test of #DB intercept in L1”)
>> verifies that indeed intercept on #DB is delivered before DR6 is modified.
>> It would be nice to also have a kvm-unit-test that similarly verifies that intercept on #PF is delivered before CR2 is modified.
>> 2) Similar kvm-unit-tests should also be written for nSVM.
>> 3) I think now we have the needed framework to also fix
>> kvm_vcpu_ioctl_x86_get_vcpu_events() and kvm_vcpu_ioctl_x86_set_vcpu_events()
>> to pass exception.pending and exception.injected separately.
>> Do you think this work should be done at the end of this patch series or a separate one once this is applied?
>>
>> -Liran
>>
>>
