On 06/05/20 20:15, Peter Xu wrote: > 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?
It's the GD test. >> + /* 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. You're right, I need to cover both cases that trigger #DB. > 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. No, the latest guest register is always in vcpu->arch.dr6. It's only because of KVM_DEBUGREG_WONT_EXIT that kvm_update_dr6() needs to pass vcpu->arch.dr6 to kvm_x86_ops.set_dr6. Actually this patch could even check KVM_DEBUGREG_WONT_EXIT instead of vcpu->guest_debug. I'll take a look tomorrow. > 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. Sort of, userspace can try to juggle host and guest debugging (this is why you have KVM_GUESTDBG_INJECT_DB and KVM_GUESTDBG_INJECT_BP). > 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? The patch is based on kvm/master, and indeed that line is from a bugfix that I've posted yesterday ("KVM: SVM: fill in kvm_run->debug.arch.dr[67]"). I had pushed that one right away, because it was quite obviously suitable for 5.7. Paolo