On Wed, May 06, 2020 at 07:10:33AM -0400, Paolo Bonzini wrote:
> On Intel, #DB exceptions transmit the DR6 value via the exit qualification
> field of the VMCS, and the exit qualification only contains the description
> of the precise event that caused a vmexit.
>
> On AMD, instead the DR6 field of the VMCB is filled in as if the #DB exception
> was to be injected into the guest. This has two effects when guest debugging
> is in use:
>
> * the guest DR6 is clobbered
>
> * the kvm_run->debug.arch.dr6 field can accumulate more debug events, rather
> than just the last one that happened.
>
> Fortunately, if guest debugging is in use we debug register reads and writes
> are always intercepted. Now that the guest DR6 is always synchronized with
> vcpu->arch.dr6, we can just run the guest with an all-zero DR6 while guest
> debugging is enabled, and restore the guest value when it is disabled. This
> fixes both problems.
>
> A testcase for the second issue is added in the next patch.
Is there supposed to be another test after this one, or the GD test?
>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> arch/x86/kvm/svm/svm.c | 2 ++
> arch/x86/kvm/x86.c | 12 ++++++++----
> arch/x86/kvm/x86.h | 2 ++
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index f03bffafd9e6..29dc7311dbb1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1750,6 +1750,8 @@ static int db_interception(struct vcpu_svm *svm)
> kvm_run->exit_reason = KVM_EXIT_DEBUG;
> kvm_run->debug.arch.dr6 = svm->vmcb->save.dr6;
> kvm_run->debug.arch.dr7 = svm->vmcb->save.dr7;
[1]
> + /* This restores DR6 to all zeros. */
> + kvm_update_dr6(vcpu);
I feel like it won't work as expected for KVM_GUESTDBG_SINGLESTEP, because at
[2] below it'll go to the "else" instead so dr6 seems won't be cleared in that
case.
Another concern I have is that, I mostly read kvm_update_dr6() as "apply the
dr6 memory cache --> VMCB". I'm worried this might confuse people (at least I
used quite a few minutes to digest...) here because latest data should already
be in the VMCB.
Also, IMHO it would be fine to have invalid dr6 values during
KVM_SET_GUEST_DEBUG. I'm not sure whether my understanding is correct, but I
see KVM_SET_GUEST_DEBUG needs to override the in-guest debug completely. If we
worry about dr6 being incorrect after KVM_SET_GUEST_DEBUG is disabled, IMHO we
can reset dr6 in kvm_arch_vcpu_ioctl_set_guest_debug() properly before we
return the debug registers to the guest.
PS. I cannot see above lines [1] in my local tree (which seems to be really a
bugfix...). I tried to use kvm/queue just in case I missed some patches, but I
still didn't see them. So am I reading the wrong tree here?
Thanks,
> kvm_run->debug.arch.pc =
> svm->vmcb->save.cs.base + svm->vmcb->save.rip;
> kvm_run->debug.arch.exception = DB_VECTOR;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f4254d716b10..1b5e0fc346bb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -104,7 +104,6 @@ static u64 __read_mostly cr4_reserved_bits = CR4_RESERVED_BITS;
> KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)
>
> static void update_cr8_intercept(struct kvm_vcpu *vcpu);
> -static void kvm_update_dr6(struct kvm_vcpu *vcpu);
> static void process_nmi(struct kvm_vcpu *vcpu);
> static void enter_smm(struct kvm_vcpu *vcpu);
> static void __kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
> @@ -1048,10 +1047,14 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu)
> }
> }
>
> -static void kvm_update_dr6(struct kvm_vcpu *vcpu)
> +void kvm_update_dr6(struct kvm_vcpu *vcpu)
> {
> - if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) &&
> - kvm_x86_ops.set_dr6)
> + if (!kvm_x86_ops.set_dr6)
> + return;
> +
> + if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)
[2]
> + kvm_x86_ops.set_dr6(vcpu, DR6_FIXED_1 | DR6_RTM);
> + else
> kvm_x86_ops.set_dr6(vcpu, vcpu->arch.dr6);
> }
>
> @@ -9154,6 +9157,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> for (i = 0; i < KVM_NR_DB_REGS; i++)
> vcpu->arch.eff_db[i] = vcpu->arch.db[i];
> }
> + kvm_update_dr6(vcpu);
> kvm_update_dr7(vcpu);
>
> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index b968acc0516f..a4c950ad4d60 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -240,6 +240,8 @@ static inline bool kvm_vcpu_latch_init(struct kvm_vcpu *vcpu)
> return is_smm(vcpu) || kvm_x86_ops.apic_init_signal_blocked(vcpu);
> }
>
> +void kvm_update_dr6(struct kvm_vcpu *vcpu);
> +
> void kvm_set_pending_timer(struct kvm_vcpu *vcpu);
> void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
>
> --
> 2.18.2
>
>
--
Peter Xu
