> On 8 Oct 2018, at 21:29, Jim Mattson <jmattson@xxxxxxxxxx> wrote:
>
> When exception payloads are enabled by userspace (which is not yet
> possible) and a #DB is raised in L2, defer the setting of DR6 until
> later. Under VMX, this allows the L1 hypervisor to intercept the fault
> before DR6 is modified. Under SVM, DR6 is modified before L1 can
> intercept the fault (as has always been the case with DR7).
>
> Note that the payload associated with a #DB exception includes only
> the "new DR6 bits." When the payload is delievered, DR6.B0-B3 will be
> cleared and DR6.RTM will be set prior to merging in the new DR6 bits.
>
> Also note that bit 16 in 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. Though the reverse of DR6.RTM, this makes the #DB payload
> field compatible with both the pending debug exceptions field under
> VMX and the exit qualification for #DB exceptions under VMX.
>
> 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>
> ---
> arch/x86/kvm/vmx.c | 18 ++++++------------
> arch/x86/kvm/x86.c | 47 ++++++++++++++++++++++++++++++++++------------
> 2 files changed, 41 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index fc3f2d27b580..45a346acc40b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3288,18 +3288,12 @@ static int nested_vmx_check_exception(struct kvm_vcpu *vcpu, unsigned long *exit
> *exit_qual = has_payload ? payload : vcpu->arch.cr2;
> return 1;
> }
> - } else {
> - /*
> - * FIXME: we must not write DR6 when L1 intercepts an
> - * L2 #DB exception.
> - */
> - if (vmcs12->exception_bitmap & (1u << nr)) {
> - if (nr == DB_VECTOR)
> - *exit_qual = vcpu->arch.dr6;
> - else
> - *exit_qual = 0;
> - return 1;
> - }
> + } else if (vmcs12->exception_bitmap & BIT(nr)) {
I dislike the fact that you also changed (1u << nr) to BIT(nr) on this patch.
All such use-cases currently in vmx.c are of the form (1u << X) and there is no use of BIT() macro.
I am not against it but I prefer for such a change to happen on a separate patch and modify other similar places as-well.
> + if (nr == DB_VECTOR)
> + *exit_qual = has_payload ? payload : vcpu->arch.dr6;
> + else
> + *exit_qual = 0;
> + return 1;
> }
>
> return 0;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 974f0784ac99..33e171e6d067 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -410,6 +410,28 @@ void kvm_deliver_exception_payload(struct kvm_vcpu *vcpu)
> return;
>
> switch (nr) {
> + case DB_VECTOR:
> + /*
> + * "Certain debug exceptions may clear bit 0-3. The
> + * remaining contents of the DR6 register are never
> + * cleared by the processor".
> + */
> + vcpu->arch.dr6 &= ~DR_TRAP_BITS;
> + /*
> + * DR6.RTM is set by all #DB exceptions that don't clear it.
> + */
> + vcpu->arch.dr6 |= DR6_RTM;
> + vcpu->arch.dr6 |= payload;
> + /*
> + * Bit 16 should be set in the payload whenever the #DB
> + * exception should clear DR6.RTM. This makes the payload
> + * compatible with the pending debug exceptions under VMX.
> + * Though not currently documented in the SDM, this also
> + * makes the payload compatible with the exit qualification
> + * for #DB exceptions under VMX.
> + */
> + vcpu->arch.dr6 ^= payload & DR6_RTM;
> + break;
> case PF_VECTOR:
> vcpu->arch.cr2 = payload;
> break;
> @@ -501,6 +523,12 @@ void kvm_requeue_exception(struct kvm_vcpu *vcpu, unsigned nr)
> }
> EXPORT_SYMBOL_GPL(kvm_requeue_exception);
>
> +static void kvm_queue_exception_p(struct kvm_vcpu *vcpu, unsigned nr,
> + unsigned long payload)
> +{
> + kvm_multiple_exception(vcpu, nr, false, 0, true, payload, false);
> +}
> +
> static void kvm_queue_exception_e_p(struct kvm_vcpu *vcpu, unsigned nr,
> u32 error_code, unsigned long payload)
> {
> @@ -6101,14 +6129,7 @@ static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
> kvm_run->exit_reason = KVM_EXIT_DEBUG;
> *r = EMULATE_USER_EXIT;
> } else {
> - /*
> - * "Certain debug exceptions may clear bit 0-3. The
> - * remaining contents of the DR6 register are never
> - * cleared by the processor".
> - */
> - vcpu->arch.dr6 &= ~15;
> - vcpu->arch.dr6 |= DR6_BS | DR6_RTM;
> - kvm_queue_exception(vcpu, DB_VECTOR);
> + kvm_queue_exception_p(vcpu, DB_VECTOR, DR6_BS);
> }
> }
>
> @@ -7047,10 +7068,12 @@ static int inject_pending_event(struct kvm_vcpu *vcpu, bool req_int_win)
> __kvm_set_rflags(vcpu, kvm_get_rflags(vcpu) |
> X86_EFLAGS_RF);
>
> - if (vcpu->arch.exception.nr == DB_VECTOR &&
> - (vcpu->arch.dr7 & DR7_GD)) {
> - vcpu->arch.dr7 &= ~DR7_GD;
> - kvm_update_dr7(vcpu);
> + if (vcpu->arch.exception.nr == DB_VECTOR) {
> + kvm_deliver_exception_payload(vcpu);
I would add a comment here that one should note that once we will modify nSVM to use
check_nested_events() framework, the call here for kvm_deliver_exception_payload()
should be moved to svm_check_nested_events().
> + if (vcpu->arch.dr7 & DR7_GD) {
> + vcpu->arch.dr7 &= ~DR7_GD;
> + kvm_update_dr7(vcpu);
> + }
> }
>
> kvm_x86_ops->queue_exception(vcpu);
> --
> 2.19.0.605.g01d371f741-goog
>
Looks good.
Reviewed-by: Liran Alon <liran.alon@xxxxxxxxxx>
