| Message ID | 20200507115011.494562-10-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM_SET_GUEST_DEBUG tests and fixes, DR accessors cleanups | expand |
On Thu, May 07, 2020 at 07:50:11AM -0400, Paolo Bonzini wrote: > When KVM_EXIT_DEBUG is raised for the disabled-breakpoints case (DR7.GD), > DR6 was incorrectly copied from the value in the VM. Instead, > DR6.BD should be set in order to catch this case. > > On AMD this does not need any special code because the processor triggers > a #DB exception that is intercepted. However, the testcase would fail > without the previous patch because both DR6.BS and DR6.BD would be set. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/vmx/vmx.c | 2 +- > .../testing/selftests/kvm/x86_64/debug_regs.c | 24 ++++++++++++++++++- > 2 files changed, 24 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index e2b71b0cdfce..e45cf89c5821 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -4927,7 +4927,7 @@ static int handle_dr(struct kvm_vcpu *vcpu) > * guest debugging itself. > */ > if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) { > - vcpu->run->debug.arch.dr6 = vcpu->arch.dr6; > + vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1; After a second thought I'm thinking whether it would be okay to have BS set in that test case. I just remembered there's a test case in the kvm-unit-test that checks explicitly against BS leftover as long as dr6 is not cleared explicitly by the guest code, while the spec seems to have no explicit description on this case. Intead of above, I'm thinking whether we should allow the userspace to also change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6. Or to make it simple, maybe we can just check BD bit only? Thanks, > vcpu->run->debug.arch.dr7 = dr7; > vcpu->run->debug.arch.pc = kvm_get_linear_rip(vcpu); > vcpu->run->debug.arch.exception = DB_VECTOR;
On 07/05/20 18:18, Peter Xu wrote: >> if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) { >> - vcpu->run->debug.arch.dr6 = vcpu->arch.dr6; >> + vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1; > After a second thought I'm thinking whether it would be okay to have BS set in > that test case. I just remembered there's a test case in the kvm-unit-test > that checks explicitly against BS leftover as long as dr6 is not cleared > explicitly by the guest code, while the spec seems to have no explicit > description on this case. Yes, I noticed that test as well. But I don't like having different behavior for Intel and AMD, and the Intel behavior is more sensible. Also... > Intead of above, I'm thinking whether we should allow the userspace to also > change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now > iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6. > Or to make it simple, maybe we can just check BD bit only? ... I'm afraid that this would be a backwards-incompatible change, and it would require changes in userspace. If you look at v2, emulating the Intel behavior in AMD turns out to be self-contained and relatively elegant (will be better when we finish cleaning up nested SVM). Paolo
On Thu, May 07, 2020 at 06:21:18PM +0200, Paolo Bonzini wrote: > On 07/05/20 18:18, Peter Xu wrote: > >> if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) { > >> - vcpu->run->debug.arch.dr6 = vcpu->arch.dr6; > >> + vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1; > > After a second thought I'm thinking whether it would be okay to have BS set in > > that test case. I just remembered there's a test case in the kvm-unit-test > > that checks explicitly against BS leftover as long as dr6 is not cleared > > explicitly by the guest code, while the spec seems to have no explicit > > description on this case. > > Yes, I noticed that test as well. But I don't like having different > behavior for Intel and AMD, and the Intel behavior is more sensible. > Also... Do you mean the AMD behavior is more sensible instead? :) > > > Intead of above, I'm thinking whether we should allow the userspace to also > > change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now > > iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6. > > Or to make it simple, maybe we can just check BD bit only? > > ... I'm afraid that this would be a backwards-incompatible change, and > it would require changes in userspace. If you look at v2, emulating the > Intel behavior in AMD turns out to be self-contained and relatively > elegant (will be better when we finish cleaning up nested SVM). I'm still trying to read the other patches (I need some more digest because I'm even less familiar with nested...). I agree that it would be good to keep the same behavior across Intel/AMD. Actually that also does not violate Intel spec because the AMD one is stricter. However I guess then we might also want to fixup the kvm-unit-test too to aligh with the behaviors on leftover set bits. Thanks,
On 07/05/20 18:38, Peter Xu wrote: > On Thu, May 07, 2020 at 06:21:18PM +0200, Paolo Bonzini wrote: >> On 07/05/20 18:18, Peter Xu wrote: >>>> if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) { >>>> - vcpu->run->debug.arch.dr6 = vcpu->arch.dr6; >>>> + vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1; >>> After a second thought I'm thinking whether it would be okay to have BS set in >>> that test case. I just remembered there's a test case in the kvm-unit-test >>> that checks explicitly against BS leftover as long as dr6 is not cleared >>> explicitly by the guest code, while the spec seems to have no explicit >>> description on this case. >> >> Yes, I noticed that test as well. But I don't like having different >> behavior for Intel and AMD, and the Intel behavior is more sensible. >> Also... > > Do you mean the AMD behavior is more sensible instead? :) No, I mean within the context of KVM_EXIT_DEBUG: the Intel behavior is to only include the latest debug exception in kvm_run's DR6 field, while the AMD behavior would be to include all of them. This was an implementation detail (it happens because Intel sets kvm_run's DR6 from the exit qualification of #DB), but it's more sensible too. In addition: * AMD was completely broken until this week, so the behavior of KVM_EXIT_DEBUG is defined de facto by kvm_intel.ko. Userspace has not been required to set DR6 with KVM_SET_GUEST_DEBUG, and since we can emulate that on AMD, we should. * we have to fix anyway the fact that on AMD a KVM_EXIT_DEBUG is clobbering the contents of the guest's DR6 >>> Intead of above, I'm thinking whether we should allow the userspace to also >>> change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now >>> iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6. >>> Or to make it simple, maybe we can just check BD bit only? >> >> ... I'm afraid that this would be a backwards-incompatible change, and >> it would require changes in userspace. If you look at v2, emulating the >> Intel behavior in AMD turns out to be self-contained and relatively >> elegant (will be better when we finish cleaning up nested SVM). > > I'm still trying to read the other patches (I need some more digest because I'm > even less familiar with nested...). I agree that it would be good to keep the > same behavior across Intel/AMD. Actually that also does not violate Intel spec > because the AMD one is stricter. Again, careful---we're talking about KVM_EXIT_DEBUG, not the #DB exception. Thanks, Paolo > However I guess then we might also want to > fixup the kvm-unit-test too to aligh with the behaviors on leftover set bits.
On Thu, May 07, 2020 at 07:42:25PM +0200, Paolo Bonzini wrote: > On 07/05/20 18:38, Peter Xu wrote: > > On Thu, May 07, 2020 at 06:21:18PM +0200, Paolo Bonzini wrote: > >> On 07/05/20 18:18, Peter Xu wrote: > >>>> if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) { > >>>> - vcpu->run->debug.arch.dr6 = vcpu->arch.dr6; > >>>> + vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1; > >>> After a second thought I'm thinking whether it would be okay to have BS set in > >>> that test case. I just remembered there's a test case in the kvm-unit-test > >>> that checks explicitly against BS leftover as long as dr6 is not cleared > >>> explicitly by the guest code, while the spec seems to have no explicit > >>> description on this case. > >> > >> Yes, I noticed that test as well. But I don't like having different > >> behavior for Intel and AMD, and the Intel behavior is more sensible. > >> Also... > > > > Do you mean the AMD behavior is more sensible instead? :) > > No, I mean within the context of KVM_EXIT_DEBUG: the Intel behavior is > to only include the latest debug exception in kvm_run's DR6 field, while > the AMD behavior would be to include all of them. This was an > implementation detail (it happens because Intel sets kvm_run's DR6 from > the exit qualification of #DB), but it's more sensible too. > > In addition: > > * AMD was completely broken until this week, so the behavior of > KVM_EXIT_DEBUG is defined de facto by kvm_intel.ko. Userspace has not > been required to set DR6 with KVM_SET_GUEST_DEBUG, and since we can > emulate that on AMD, we should. > > * we have to fix anyway the fact that on AMD a KVM_EXIT_DEBUG is > clobbering the contents of the guest's DR6 > > >>> Intead of above, I'm thinking whether we should allow the userspace to also > >>> change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now > >>> iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6. > >>> Or to make it simple, maybe we can just check BD bit only? > >> > >> ... I'm afraid that this would be a backwards-incompatible change, and > >> it would require changes in userspace. If you look at v2, emulating the > >> Intel behavior in AMD turns out to be self-contained and relatively > >> elegant (will be better when we finish cleaning up nested SVM). > > > > I'm still trying to read the other patches (I need some more digest because I'm > > even less familiar with nested...). I agree that it would be good to keep the > > same behavior across Intel/AMD. Actually that also does not violate Intel spec > > because the AMD one is stricter. > > Again, careful---we're talking about KVM_EXIT_DEBUG, not the #DB exception. OK I get your point now. Thanks,
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index e2b71b0cdfce..e45cf89c5821 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -4927,7 +4927,7 @@ static int handle_dr(struct kvm_vcpu *vcpu) * guest debugging itself. */ if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) { - vcpu->run->debug.arch.dr6 = vcpu->arch.dr6; + vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1; vcpu->run->debug.arch.dr7 = dr7; vcpu->run->debug.arch.pc = kvm_get_linear_rip(vcpu); vcpu->run->debug.arch.exception = DB_VECTOR; diff --git a/tools/testing/selftests/kvm/x86_64/debug_regs.c b/tools/testing/selftests/kvm/x86_64/debug_regs.c index 077f25d61d1a..8162c58a1234 100644 --- a/tools/testing/selftests/kvm/x86_64/debug_regs.c +++ b/tools/testing/selftests/kvm/x86_64/debug_regs.c @@ -11,10 +11,13 @@ #define VCPU_ID 0 +#define DR6_BD (1 << 13) +#define DR7_GD (1 << 13) + /* For testing data access debug BP */ uint32_t guest_value; -extern unsigned char sw_bp, hw_bp, write_data, ss_start; +extern unsigned char sw_bp, hw_bp, write_data, ss_start, bd_start; static void guest_code(void) { @@ -43,6 +46,8 @@ static void guest_code(void) "rdmsr\n\t" : : : "rax", "ecx"); + /* DR6.BD test */ + asm volatile("bd_start: mov %%dr0, %%rax" : : : "rax"); GUEST_DONE(); } @@ -165,6 +170,23 @@ int main(void) target_dr6); } + /* Finally test global disable */ + CLEAR_DEBUG(); + debug.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP; + debug.arch.debugreg[7] = 0x400 | DR7_GD; + APPLY_DEBUG(); + vcpu_run(vm, VCPU_ID); + target_dr6 = 0xffff0ff0 | DR6_BD; + TEST_ASSERT(run->exit_reason == KVM_EXIT_DEBUG && + run->debug.arch.exception == DB_VECTOR && + run->debug.arch.pc == CAST_TO_RIP(bd_start) && + run->debug.arch.dr6 == target_dr6, + "DR7.GD: exit %d exception %d rip 0x%llx " + "(should be 0x%llx) dr6 0x%llx (should be 0x%llx)", + run->exit_reason, run->debug.arch.exception, + run->debug.arch.pc, target_rip, run->debug.arch.dr6, + target_dr6); + /* Disable all debug controls, run to the end */ CLEAR_DEBUG(); APPLY_DEBUG();
When KVM_EXIT_DEBUG is raised for the disabled-breakpoints case (DR7.GD), DR6 was incorrectly copied from the value in the VM. Instead, DR6.BD should be set in order to catch this case. On AMD this does not need any special code because the processor triggers a #DB exception that is intercepted. However, the testcase would fail without the previous patch because both DR6.BS and DR6.BD would be set. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/vmx/vmx.c | 2 +- .../testing/selftests/kvm/x86_64/debug_regs.c | 24 ++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-)