| Message ID | 20200506111034.11756-8-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM_SET_GUEST_DEBUG tests and fixes, DR accessors cleanups | expand |
On Wed, May 06, 2020 at 07:10:32AM -0400, Paolo Bonzini wrote: > kvm_x86_ops.set_dr6 is only ever called with vcpu->arch.dr6 as the > second argument, and for both SVM and VMX the VMCB value is kept > synchronized with vcpu->arch.dr6 on #DB; we can therefore remove the > read accessor. > > For the write accessor we can avoid the retpoline penalty on Intel > by accepting a NULL value and just skipping the call in that case. > > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> (I think this patch and the previous one seem to be the same as the previous version. Anyway...) Reviewed-by: Peter Xu <peterx@redhat.com>
On 06/05/20 18:06, Peter Xu wrote: > On Wed, May 06, 2020 at 07:10:32AM -0400, Paolo Bonzini wrote: >> kvm_x86_ops.set_dr6 is only ever called with vcpu->arch.dr6 as the >> second argument, and for both SVM and VMX the VMCB value is kept >> synchronized with vcpu->arch.dr6 on #DB; we can therefore remove the >> read accessor. >> >> For the write accessor we can avoid the retpoline penalty on Intel >> by accepting a NULL value and just skipping the call in that case. >> >> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > (I think this patch and the previous one seem to be the same as the previous > version. Anyway...) Yes, I placed them here because they are needed to solve the SVM bugs in patch 8. Sorry for not adding your Reviewed-by. Paolo > Reviewed-by: Peter Xu <peterx@redhat.com>
On Wed, May 06, 2020 at 06:09:23PM +0200, Paolo Bonzini wrote: > On 06/05/20 18:06, Peter Xu wrote: > > On Wed, May 06, 2020 at 07:10:32AM -0400, Paolo Bonzini wrote: > >> kvm_x86_ops.set_dr6 is only ever called with vcpu->arch.dr6 as the > >> second argument, and for both SVM and VMX the VMCB value is kept > >> synchronized with vcpu->arch.dr6 on #DB; we can therefore remove the > >> read accessor. > >> > >> For the write accessor we can avoid the retpoline penalty on Intel > >> by accepting a NULL value and just skipping the call in that case. > >> > >> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > (I think this patch and the previous one seem to be the same as the previous > > version. Anyway...) > > Yes, I placed them here because they are needed to solve the SVM bugs in > patch 8. Sorry for not adding your Reviewed-by. That's not a problem to me. :) Instead I'm more afraid of not noticing some trivial difference in the patch comparing to the last one. Thanks,
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 8c247bcb037e..93f6f696d059 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1093,7 +1093,6 @@ struct kvm_x86_ops { void (*set_idt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt); void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt); void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt); - u64 (*get_dr6)(struct kvm_vcpu *vcpu); void (*set_dr6)(struct kvm_vcpu *vcpu, unsigned long value); void (*sync_dirty_debug_regs)(struct kvm_vcpu *vcpu); void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value); diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 5627004e077e..f03bffafd9e6 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -1672,11 +1672,6 @@ static void new_asid(struct vcpu_svm *svm, struct svm_cpu_data *sd) mark_dirty(svm->vmcb, VMCB_ASID); } -static u64 svm_get_dr6(struct kvm_vcpu *vcpu) -{ - return vcpu->arch.dr6; -} - static void svm_set_dr6(struct kvm_vcpu *vcpu, unsigned long value) { struct vcpu_svm *svm = to_svm(vcpu); @@ -3932,7 +3927,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = { .set_idt = svm_set_idt, .get_gdt = svm_get_gdt, .set_gdt = svm_set_gdt, - .get_dr6 = svm_get_dr6, .set_dr6 = svm_set_dr6, .set_dr7 = svm_set_dr7, .sync_dirty_debug_regs = svm_sync_dirty_debug_regs, diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 2384a2dbec44..e2b71b0cdfce 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -4965,15 +4965,6 @@ static int handle_dr(struct kvm_vcpu *vcpu) return kvm_skip_emulated_instruction(vcpu); } -static u64 vmx_get_dr6(struct kvm_vcpu *vcpu) -{ - return vcpu->arch.dr6; -} - -static void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val) -{ -} - static void vmx_sync_dirty_debug_regs(struct kvm_vcpu *vcpu) { get_debugreg(vcpu->arch.db[0], 0); @@ -7736,8 +7727,6 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = { .set_idt = vmx_set_idt, .get_gdt = vmx_get_gdt, .set_gdt = vmx_set_gdt, - .get_dr6 = vmx_get_dr6, - .set_dr6 = vmx_set_dr6, .set_dr7 = vmx_set_dr7, .sync_dirty_debug_regs = vmx_sync_dirty_debug_regs, .cache_reg = vmx_cache_reg, diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index f7628555f036..f4254d716b10 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1050,7 +1050,8 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu) static void kvm_update_dr6(struct kvm_vcpu *vcpu) { - if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) + if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) && + kvm_x86_ops.set_dr6) kvm_x86_ops.set_dr6(vcpu, vcpu->arch.dr6); } @@ -1129,10 +1130,7 @@ int kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val) case 4: /* fall through */ case 6: - if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) - *val = vcpu->arch.dr6; - else - *val = kvm_x86_ops.get_dr6(vcpu); + *val = vcpu->arch.dr6; break; case 5: /* fall through */
kvm_x86_ops.set_dr6 is only ever called with vcpu->arch.dr6 as the second argument, and for both SVM and VMX the VMCB value is kept synchronized with vcpu->arch.dr6 on #DB; we can therefore remove the read accessor. For the write accessor we can avoid the retpoline penalty on Intel by accepting a NULL value and just skipping the call in that case. Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/include/asm/kvm_host.h | 1 - arch/x86/kvm/svm/svm.c | 6 ------ arch/x86/kvm/vmx/vmx.c | 11 ----------- arch/x86/kvm/x86.c | 8 +++----- 4 files changed, 3 insertions(+), 23 deletions(-)