* [PATCH 0/2] KVM: x86: Cleanup kvm_get_dr() usage
@ 2024-02-09 22:07 Sean Christopherson
2024-02-09 22:07 ` [PATCH 1/2] KVM: x86: Make kvm_get_dr() return a value, not use an out parameter Sean Christopherson
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Sean Christopherson @ 2024-02-09 22:07 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Mathias Krause
Make kvm_get_dr() use an actual return instead of a void return with an
output, which led to a _lot_ of ugly code, and then open code all direct
reads to DR6 and DR7, as KVM has a goofy mix of some flows open coding
reads and some flows bouncing through kvm_get_dr().
Sean Christopherson (2):
KVM: x86: Make kvm_get_dr() return a value, not use an out parameter
KVM: x86: Open code all direct reads to guest DR6 and DR7
arch/x86/include/asm/kvm_host.h | 2 +-
arch/x86/kvm/emulate.c | 17 ++++-------------
arch/x86/kvm/kvm_emulate.h | 2 +-
arch/x86/kvm/smm.c | 15 ++++-----------
arch/x86/kvm/svm/svm.c | 7 ++-----
arch/x86/kvm/vmx/nested.c | 2 +-
arch/x86/kvm/vmx/vmx.c | 5 +----
arch/x86/kvm/x86.c | 20 +++++++-------------
8 files changed, 21 insertions(+), 49 deletions(-)
base-commit: 873eef46b33c86be414d60bd00390e64fc0f006f
--
2.43.0.687.g38aa6559b0-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] KVM: x86: Make kvm_get_dr() return a value, not use an out parameter 2024-02-09 22:07 [PATCH 0/2] KVM: x86: Cleanup kvm_get_dr() usage Sean Christopherson @ 2024-02-09 22:07 ` Sean Christopherson 2024-02-09 22:29 ` Mathias Krause 2024-02-09 22:07 ` [PATCH 2/2] KVM: x86: Open code all direct reads to guest DR6 and DR7 Sean Christopherson 2024-02-23 1:35 ` [PATCH 0/2] KVM: x86: Cleanup kvm_get_dr() usage Sean Christopherson 2 siblings, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2024-02-09 22:07 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Mathias Krause Convert kvm_get_dr()'s output parameter to a return value, and clean up most of the mess that was created by forcing callers to provide a pointer. No functional change intended. Acked-by: Mathias Krause <minipli@grsecurity.net> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/kvm_host.h | 2 +- arch/x86/kvm/emulate.c | 17 ++++------------- arch/x86/kvm/kvm_emulate.h | 2 +- arch/x86/kvm/smm.c | 15 ++++----------- arch/x86/kvm/svm/svm.c | 7 ++----- arch/x86/kvm/vmx/nested.c | 2 +- arch/x86/kvm/vmx/vmx.c | 5 +---- arch/x86/kvm/x86.c | 20 +++++++------------- 8 files changed, 21 insertions(+), 49 deletions(-) diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index ad5319a503f0..464fa2197748 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -2046,7 +2046,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3); int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4); int kvm_set_cr8(struct kvm_vcpu *vcpu, unsigned long cr8); int kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val); -void kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val); +unsigned long kvm_get_dr(struct kvm_vcpu *vcpu, int dr); unsigned long kvm_get_cr8(struct kvm_vcpu *vcpu); void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw); int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu); diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 695ab5b6055c..33444627fcf4 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -3011,7 +3011,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, ret = em_push(ctxt); } - ops->get_dr(ctxt, 7, &dr7); + dr7 = ops->get_dr(ctxt, 7); ops->set_dr(ctxt, 7, dr7 & ~(DR_LOCAL_ENABLE_MASK | DR_LOCAL_SLOWDOWN)); return ret; @@ -3866,15 +3866,6 @@ static int check_cr_access(struct x86_emulate_ctxt *ctxt) return X86EMUL_CONTINUE; } -static int check_dr7_gd(struct x86_emulate_ctxt *ctxt) -{ - unsigned long dr7; - - ctxt->ops->get_dr(ctxt, 7, &dr7); - - return dr7 & DR7_GD; -} - static int check_dr_read(struct x86_emulate_ctxt *ctxt) { int dr = ctxt->modrm_reg; @@ -3887,10 +3878,10 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt) if ((cr4 & X86_CR4_DE) && (dr == 4 || dr == 5)) return emulate_ud(ctxt); - if (check_dr7_gd(ctxt)) { + if (ctxt->ops->get_dr(ctxt, 7) & DR7_GD) { ulong dr6; - ctxt->ops->get_dr(ctxt, 6, &dr6); + dr6 = ctxt->ops->get_dr(ctxt, 6); dr6 &= ~DR_TRAP_BITS; dr6 |= DR6_BD | DR6_ACTIVE_LOW; ctxt->ops->set_dr(ctxt, 6, dr6); @@ -5449,7 +5440,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt) ctxt->dst.val = ops->get_cr(ctxt, ctxt->modrm_reg); break; case 0x21: /* mov from dr to reg */ - ops->get_dr(ctxt, ctxt->modrm_reg, &ctxt->dst.val); + ctxt->dst.val = ops->get_dr(ctxt, ctxt->modrm_reg); break; case 0x40 ... 0x4f: /* cmov */ if (test_cc(ctxt->b, ctxt->eflags)) diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h index 4351149484fb..5382646162a3 100644 --- a/arch/x86/kvm/kvm_emulate.h +++ b/arch/x86/kvm/kvm_emulate.h @@ -203,7 +203,7 @@ struct x86_emulate_ops { ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr); int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val); int (*cpl)(struct x86_emulate_ctxt *ctxt); - void (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest); + ulong (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr); int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value); int (*set_msr_with_filter)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 data); int (*get_msr_with_filter)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 *pdata); diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c index dc3d95fdca7d..19a7a0a31953 100644 --- a/arch/x86/kvm/smm.c +++ b/arch/x86/kvm/smm.c @@ -184,7 +184,6 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, struct kvm_smram_state_32 *smram) { struct desc_ptr dt; - unsigned long val; int i; smram->cr0 = kvm_read_cr0(vcpu); @@ -195,10 +194,8 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, for (i = 0; i < 8; i++) smram->gprs[i] = kvm_register_read_raw(vcpu, i); - kvm_get_dr(vcpu, 6, &val); - smram->dr6 = (u32)val; - kvm_get_dr(vcpu, 7, &val); - smram->dr7 = (u32)val; + smram->dr6 = (u32)kvm_get_dr(vcpu, 6); + smram->dr7 = (u32)kvm_get_dr(vcpu, 7); enter_smm_save_seg_32(vcpu, &smram->tr, &smram->tr_sel, VCPU_SREG_TR); enter_smm_save_seg_32(vcpu, &smram->ldtr, &smram->ldtr_sel, VCPU_SREG_LDTR); @@ -231,7 +228,6 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu, struct kvm_smram_state_64 *smram) { struct desc_ptr dt; - unsigned long val; int i; for (i = 0; i < 16; i++) @@ -240,11 +236,8 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu, smram->rip = kvm_rip_read(vcpu); smram->rflags = kvm_get_rflags(vcpu); - - kvm_get_dr(vcpu, 6, &val); - smram->dr6 = val; - kvm_get_dr(vcpu, 7, &val); - smram->dr7 = val; + smram->dr6 = kvm_get_dr(vcpu, 6); + smram->dr7 = kvm_get_dr(vcpu, 7); smram->cr0 = kvm_read_cr0(vcpu); smram->cr3 = kvm_read_cr3(vcpu); diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index e90b429c84f1..dda91f7cd71b 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -2735,7 +2735,6 @@ static int dr_interception(struct kvm_vcpu *vcpu) { struct vcpu_svm *svm = to_svm(vcpu); int reg, dr; - unsigned long val; int err = 0; /* @@ -2763,11 +2762,9 @@ static int dr_interception(struct kvm_vcpu *vcpu) dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0; if (dr >= 16) { /* mov to DRn */ dr -= 16; - val = kvm_register_read(vcpu, reg); - err = kvm_set_dr(vcpu, dr, val); + err = kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg)); } else { - kvm_get_dr(vcpu, dr, &val); - kvm_register_write(vcpu, reg, val); + kvm_register_write(vcpu, reg, kvm_get_dr(vcpu, dr)); } return kvm_complete_insn_gp(vcpu, err); diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 994e014f8a50..28d1088a1770 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4433,7 +4433,7 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) (vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE); if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_DEBUG_CONTROLS) - kvm_get_dr(vcpu, 7, (unsigned long *)&vmcs12->guest_dr7); + vmcs12->guest_dr7 = kvm_get_dr(vcpu, 7); if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER) vmcs12->guest_ia32_efer = vcpu->arch.efer; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index e262bc2ba4e5..aa47433d0c9b 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -5566,10 +5566,7 @@ static int handle_dr(struct kvm_vcpu *vcpu) reg = DEBUG_REG_ACCESS_REG(exit_qualification); if (exit_qualification & TYPE_MOV_FROM_DR) { - unsigned long val; - - kvm_get_dr(vcpu, dr, &val); - kvm_register_write(vcpu, reg, val); + kvm_register_write(vcpu, reg, kvm_get_dr(vcpu, dr)); err = 0; } else { err = kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg)); diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index b66c45e7f6f8..bfffc13f91e6 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1399,22 +1399,19 @@ int kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) } EXPORT_SYMBOL_GPL(kvm_set_dr); -void kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val) +unsigned long kvm_get_dr(struct kvm_vcpu *vcpu, int dr) { size_t size = ARRAY_SIZE(vcpu->arch.db); switch (dr) { case 0 ... 3: - *val = vcpu->arch.db[array_index_nospec(dr, size)]; - break; + return vcpu->arch.db[array_index_nospec(dr, size)]; case 4: case 6: - *val = vcpu->arch.dr6; - break; + return vcpu->arch.dr6; case 5: default: /* 7 */ - *val = vcpu->arch.dr7; - break; + return vcpu->arch.dr7; } } EXPORT_SYMBOL_GPL(kvm_get_dr); @@ -5505,7 +5502,6 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu, struct kvm_debugregs *dbgregs) { - unsigned long val; unsigned int i; memset(dbgregs, 0, sizeof(*dbgregs)); @@ -5514,8 +5510,7 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu, for (i = 0; i < ARRAY_SIZE(vcpu->arch.db); i++) dbgregs->db[i] = vcpu->arch.db[i]; - kvm_get_dr(vcpu, 6, &val); - dbgregs->dr6 = val; + dbgregs->dr6 = kvm_get_dr(vcpu, 6); dbgregs->dr7 = vcpu->arch.dr7; } @@ -8169,10 +8164,9 @@ static void emulator_wbinvd(struct x86_emulate_ctxt *ctxt) kvm_emulate_wbinvd_noskip(emul_to_vcpu(ctxt)); } -static void emulator_get_dr(struct x86_emulate_ctxt *ctxt, int dr, - unsigned long *dest) +static unsigned long emulator_get_dr(struct x86_emulate_ctxt *ctxt, int dr) { - kvm_get_dr(emul_to_vcpu(ctxt), dr, dest); + return kvm_get_dr(emul_to_vcpu(ctxt), dr); } static int emulator_set_dr(struct x86_emulate_ctxt *ctxt, int dr, -- 2.43.0.687.g38aa6559b0-goog ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] KVM: x86: Make kvm_get_dr() return a value, not use an out parameter 2024-02-09 22:07 ` [PATCH 1/2] KVM: x86: Make kvm_get_dr() return a value, not use an out parameter Sean Christopherson @ 2024-02-09 22:29 ` Mathias Krause 0 siblings, 0 replies; 6+ messages in thread From: Mathias Krause @ 2024-02-09 22:29 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel On 09.02.24 23:07, Sean Christopherson wrote: > Convert kvm_get_dr()'s output parameter to a return value, and clean up > most of the mess that was created by forcing callers to provide a pointer. > > No functional change intended. > > Acked-by: Mathias Krause <minipli@grsecurity.net> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_host.h | 2 +- > arch/x86/kvm/emulate.c | 17 ++++------------- > arch/x86/kvm/kvm_emulate.h | 2 +- > arch/x86/kvm/smm.c | 15 ++++----------- > arch/x86/kvm/svm/svm.c | 7 ++----- > arch/x86/kvm/vmx/nested.c | 2 +- > arch/x86/kvm/vmx/vmx.c | 5 +---- > arch/x86/kvm/x86.c | 20 +++++++------------- > 8 files changed, 21 insertions(+), 49 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index ad5319a503f0..464fa2197748 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -2046,7 +2046,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3); > int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4); > int kvm_set_cr8(struct kvm_vcpu *vcpu, unsigned long cr8); > int kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val); > -void kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val); > +unsigned long kvm_get_dr(struct kvm_vcpu *vcpu, int dr); > unsigned long kvm_get_cr8(struct kvm_vcpu *vcpu); > void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw); > int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu); > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 695ab5b6055c..33444627fcf4 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -3011,7 +3011,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt, > ret = em_push(ctxt); > } > > - ops->get_dr(ctxt, 7, &dr7); > + dr7 = ops->get_dr(ctxt, 7); > ops->set_dr(ctxt, 7, dr7 & ~(DR_LOCAL_ENABLE_MASK | DR_LOCAL_SLOWDOWN)); > > return ret; > @@ -3866,15 +3866,6 @@ static int check_cr_access(struct x86_emulate_ctxt *ctxt) > return X86EMUL_CONTINUE; > } > > -static int check_dr7_gd(struct x86_emulate_ctxt *ctxt) > -{ > - unsigned long dr7; > - > - ctxt->ops->get_dr(ctxt, 7, &dr7); > - > - return dr7 & DR7_GD; > -} > - > static int check_dr_read(struct x86_emulate_ctxt *ctxt) > { > int dr = ctxt->modrm_reg; > @@ -3887,10 +3878,10 @@ static int check_dr_read(struct x86_emulate_ctxt *ctxt) > if ((cr4 & X86_CR4_DE) && (dr == 4 || dr == 5)) > return emulate_ud(ctxt); > > - if (check_dr7_gd(ctxt)) { > + if (ctxt->ops->get_dr(ctxt, 7) & DR7_GD) { > ulong dr6; > > - ctxt->ops->get_dr(ctxt, 6, &dr6); > + dr6 = ctxt->ops->get_dr(ctxt, 6); > dr6 &= ~DR_TRAP_BITS; > dr6 |= DR6_BD | DR6_ACTIVE_LOW; > ctxt->ops->set_dr(ctxt, 6, dr6); > @@ -5449,7 +5440,7 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt) > ctxt->dst.val = ops->get_cr(ctxt, ctxt->modrm_reg); > break; > case 0x21: /* mov from dr to reg */ > - ops->get_dr(ctxt, ctxt->modrm_reg, &ctxt->dst.val); > + ctxt->dst.val = ops->get_dr(ctxt, ctxt->modrm_reg); > break; > case 0x40 ... 0x4f: /* cmov */ > if (test_cc(ctxt->b, ctxt->eflags)) > diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h > index 4351149484fb..5382646162a3 100644 > --- a/arch/x86/kvm/kvm_emulate.h > +++ b/arch/x86/kvm/kvm_emulate.h > @@ -203,7 +203,7 @@ struct x86_emulate_ops { > ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr); > int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val); > int (*cpl)(struct x86_emulate_ctxt *ctxt); > - void (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest); > + ulong (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr); > int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value); > int (*set_msr_with_filter)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 data); > int (*get_msr_with_filter)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 *pdata); > diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c > index dc3d95fdca7d..19a7a0a31953 100644 > --- a/arch/x86/kvm/smm.c > +++ b/arch/x86/kvm/smm.c > @@ -184,7 +184,6 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, > struct kvm_smram_state_32 *smram) > { > struct desc_ptr dt; > - unsigned long val; > int i; > > smram->cr0 = kvm_read_cr0(vcpu); > @@ -195,10 +194,8 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, > for (i = 0; i < 8; i++) > smram->gprs[i] = kvm_register_read_raw(vcpu, i); > > - kvm_get_dr(vcpu, 6, &val); > - smram->dr6 = (u32)val; > - kvm_get_dr(vcpu, 7, &val); > - smram->dr7 = (u32)val; > + smram->dr6 = (u32)kvm_get_dr(vcpu, 6); > + smram->dr7 = (u32)kvm_get_dr(vcpu, 7); > > enter_smm_save_seg_32(vcpu, &smram->tr, &smram->tr_sel, VCPU_SREG_TR); > enter_smm_save_seg_32(vcpu, &smram->ldtr, &smram->ldtr_sel, VCPU_SREG_LDTR); > @@ -231,7 +228,6 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu, > struct kvm_smram_state_64 *smram) > { > struct desc_ptr dt; > - unsigned long val; > int i; > > for (i = 0; i < 16; i++) > @@ -240,11 +236,8 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu, > smram->rip = kvm_rip_read(vcpu); > smram->rflags = kvm_get_rflags(vcpu); > > - > - kvm_get_dr(vcpu, 6, &val); > - smram->dr6 = val; > - kvm_get_dr(vcpu, 7, &val); > - smram->dr7 = val; > + smram->dr6 = kvm_get_dr(vcpu, 6); > + smram->dr7 = kvm_get_dr(vcpu, 7); > > smram->cr0 = kvm_read_cr0(vcpu); > smram->cr3 = kvm_read_cr3(vcpu); > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index e90b429c84f1..dda91f7cd71b 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -2735,7 +2735,6 @@ static int dr_interception(struct kvm_vcpu *vcpu) > { > struct vcpu_svm *svm = to_svm(vcpu); > int reg, dr; > - unsigned long val; > int err = 0; > > /* > @@ -2763,11 +2762,9 @@ static int dr_interception(struct kvm_vcpu *vcpu) > dr = svm->vmcb->control.exit_code - SVM_EXIT_READ_DR0; > if (dr >= 16) { /* mov to DRn */ > dr -= 16; > - val = kvm_register_read(vcpu, reg); > - err = kvm_set_dr(vcpu, dr, val); > + err = kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg)); > } else { > - kvm_get_dr(vcpu, dr, &val); > - kvm_register_write(vcpu, reg, val); > + kvm_register_write(vcpu, reg, kvm_get_dr(vcpu, dr)); > } > > return kvm_complete_insn_gp(vcpu, err); > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 994e014f8a50..28d1088a1770 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4433,7 +4433,7 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > (vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE); > > if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_DEBUG_CONTROLS) > - kvm_get_dr(vcpu, 7, (unsigned long *)&vmcs12->guest_dr7); > + vmcs12->guest_dr7 = kvm_get_dr(vcpu, 7); > > if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER) > vmcs12->guest_ia32_efer = vcpu->arch.efer; > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index e262bc2ba4e5..aa47433d0c9b 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -5566,10 +5566,7 @@ static int handle_dr(struct kvm_vcpu *vcpu) > > reg = DEBUG_REG_ACCESS_REG(exit_qualification); > if (exit_qualification & TYPE_MOV_FROM_DR) { > - unsigned long val; > - > - kvm_get_dr(vcpu, dr, &val); > - kvm_register_write(vcpu, reg, val); > + kvm_register_write(vcpu, reg, kvm_get_dr(vcpu, dr)); > err = 0; > } else { > err = kvm_set_dr(vcpu, dr, kvm_register_read(vcpu, reg)); > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index b66c45e7f6f8..bfffc13f91e6 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1399,22 +1399,19 @@ int kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val) > } > EXPORT_SYMBOL_GPL(kvm_set_dr); > > -void kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val) > +unsigned long kvm_get_dr(struct kvm_vcpu *vcpu, int dr) > { > size_t size = ARRAY_SIZE(vcpu->arch.db); > > switch (dr) { > case 0 ... 3: > - *val = vcpu->arch.db[array_index_nospec(dr, size)]; > - break; > + return vcpu->arch.db[array_index_nospec(dr, size)]; > case 4: > case 6: > - *val = vcpu->arch.dr6; > - break; > + return vcpu->arch.dr6; > case 5: > default: /* 7 */ > - *val = vcpu->arch.dr7; > - break; > + return vcpu->arch.dr7; > } > } > EXPORT_SYMBOL_GPL(kvm_get_dr); > @@ -5505,7 +5502,6 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, > static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu, > struct kvm_debugregs *dbgregs) > { > - unsigned long val; > unsigned int i; > > memset(dbgregs, 0, sizeof(*dbgregs)); > @@ -5514,8 +5510,7 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu, > for (i = 0; i < ARRAY_SIZE(vcpu->arch.db); i++) > dbgregs->db[i] = vcpu->arch.db[i]; > > - kvm_get_dr(vcpu, 6, &val); > - dbgregs->dr6 = val; > + dbgregs->dr6 = kvm_get_dr(vcpu, 6); > dbgregs->dr7 = vcpu->arch.dr7; > } > > @@ -8169,10 +8164,9 @@ static void emulator_wbinvd(struct x86_emulate_ctxt *ctxt) > kvm_emulate_wbinvd_noskip(emul_to_vcpu(ctxt)); > } > > -static void emulator_get_dr(struct x86_emulate_ctxt *ctxt, int dr, > - unsigned long *dest) > +static unsigned long emulator_get_dr(struct x86_emulate_ctxt *ctxt, int dr) > { > - kvm_get_dr(emul_to_vcpu(ctxt), dr, dest); > + return kvm_get_dr(emul_to_vcpu(ctxt), dr); > } > > static int emulator_set_dr(struct x86_emulate_ctxt *ctxt, int dr, Reviewed-by: Mathias Krause <minipli@grsecurity.net> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] KVM: x86: Open code all direct reads to guest DR6 and DR7 2024-02-09 22:07 [PATCH 0/2] KVM: x86: Cleanup kvm_get_dr() usage Sean Christopherson 2024-02-09 22:07 ` [PATCH 1/2] KVM: x86: Make kvm_get_dr() return a value, not use an out parameter Sean Christopherson @ 2024-02-09 22:07 ` Sean Christopherson 2024-02-09 22:30 ` Mathias Krause 2024-02-23 1:35 ` [PATCH 0/2] KVM: x86: Cleanup kvm_get_dr() usage Sean Christopherson 2 siblings, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2024-02-09 22:07 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Mathias Krause Bite the bullet, and open code all direct reads of DR6 and DR7. KVM currently has a mix of open coded accesses and calls to kvm_get_dr(), which is confusing and ugly because there's no rhyme or reason as to why any particular chunk of code uses kvm_get_dr(). The obvious alternative is to force all accesses through kvm_get_dr(), but it's not at all clear that doing so would be a net positive, e.g. even if KVM ends up wanting/needing to force all reads through a common helper, e.g. to play caching games, the cost of reverting this change is likely lower than the ongoing cost of maintaining weird, arbitrary code. No functional change intended. Cc: Mathias Krause <minipli@grsecurity.net> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/smm.c | 8 ++++---- arch/x86/kvm/vmx/nested.c | 2 +- arch/x86/kvm/x86.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c index 19a7a0a31953..d06d43d8d2aa 100644 --- a/arch/x86/kvm/smm.c +++ b/arch/x86/kvm/smm.c @@ -194,8 +194,8 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, for (i = 0; i < 8; i++) smram->gprs[i] = kvm_register_read_raw(vcpu, i); - smram->dr6 = (u32)kvm_get_dr(vcpu, 6); - smram->dr7 = (u32)kvm_get_dr(vcpu, 7); + smram->dr6 = (u32)vcpu->arch.dr6; + smram->dr7 = (u32)vcpu->arch.dr7; enter_smm_save_seg_32(vcpu, &smram->tr, &smram->tr_sel, VCPU_SREG_TR); enter_smm_save_seg_32(vcpu, &smram->ldtr, &smram->ldtr_sel, VCPU_SREG_LDTR); @@ -236,8 +236,8 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu, smram->rip = kvm_rip_read(vcpu); smram->rflags = kvm_get_rflags(vcpu); - smram->dr6 = kvm_get_dr(vcpu, 6); - smram->dr7 = kvm_get_dr(vcpu, 7); + smram->dr6 = vcpu->arch.dr6; + smram->dr7 = vcpu->arch.dr7; smram->cr0 = kvm_read_cr0(vcpu); smram->cr3 = kvm_read_cr3(vcpu); diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 28d1088a1770..d05ddf751491 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -4433,7 +4433,7 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) (vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE); if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_DEBUG_CONTROLS) - vmcs12->guest_dr7 = kvm_get_dr(vcpu, 7); + vmcs12->guest_dr7 = vcpu->arch.dr7; if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER) vmcs12->guest_ia32_efer = vcpu->arch.efer; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index bfffc13f91e6..5a08d895bde6 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -5510,7 +5510,7 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu, for (i = 0; i < ARRAY_SIZE(vcpu->arch.db); i++) dbgregs->db[i] = vcpu->arch.db[i]; - dbgregs->dr6 = kvm_get_dr(vcpu, 6); + dbgregs->dr6 = vcpu->arch.dr6; dbgregs->dr7 = vcpu->arch.dr7; } -- 2.43.0.687.g38aa6559b0-goog ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] KVM: x86: Open code all direct reads to guest DR6 and DR7 2024-02-09 22:07 ` [PATCH 2/2] KVM: x86: Open code all direct reads to guest DR6 and DR7 Sean Christopherson @ 2024-02-09 22:30 ` Mathias Krause 0 siblings, 0 replies; 6+ messages in thread From: Mathias Krause @ 2024-02-09 22:30 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel On 09.02.24 23:07, Sean Christopherson wrote: > Bite the bullet, and open code all direct reads of DR6 and DR7. KVM > currently has a mix of open coded accesses and calls to kvm_get_dr(), > which is confusing and ugly because there's no rhyme or reason as to why > any particular chunk of code uses kvm_get_dr(). > > The obvious alternative is to force all accesses through kvm_get_dr(), > but it's not at all clear that doing so would be a net positive, e.g. even > if KVM ends up wanting/needing to force all reads through a common helper, > e.g. to play caching games, the cost of reverting this change is likely > lower than the ongoing cost of maintaining weird, arbitrary code. > > No functional change intended. > > Cc: Mathias Krause <minipli@grsecurity.net> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/smm.c | 8 ++++---- > arch/x86/kvm/vmx/nested.c | 2 +- > arch/x86/kvm/x86.c | 2 +- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/smm.c b/arch/x86/kvm/smm.c > index 19a7a0a31953..d06d43d8d2aa 100644 > --- a/arch/x86/kvm/smm.c > +++ b/arch/x86/kvm/smm.c > @@ -194,8 +194,8 @@ static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, > for (i = 0; i < 8; i++) > smram->gprs[i] = kvm_register_read_raw(vcpu, i); > > - smram->dr6 = (u32)kvm_get_dr(vcpu, 6); > - smram->dr7 = (u32)kvm_get_dr(vcpu, 7); > + smram->dr6 = (u32)vcpu->arch.dr6; > + smram->dr7 = (u32)vcpu->arch.dr7; > > enter_smm_save_seg_32(vcpu, &smram->tr, &smram->tr_sel, VCPU_SREG_TR); > enter_smm_save_seg_32(vcpu, &smram->ldtr, &smram->ldtr_sel, VCPU_SREG_LDTR); > @@ -236,8 +236,8 @@ static void enter_smm_save_state_64(struct kvm_vcpu *vcpu, > smram->rip = kvm_rip_read(vcpu); > smram->rflags = kvm_get_rflags(vcpu); > > - smram->dr6 = kvm_get_dr(vcpu, 6); > - smram->dr7 = kvm_get_dr(vcpu, 7); > + smram->dr6 = vcpu->arch.dr6; > + smram->dr7 = vcpu->arch.dr7; > > smram->cr0 = kvm_read_cr0(vcpu); > smram->cr3 = kvm_read_cr3(vcpu); > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 28d1088a1770..d05ddf751491 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -4433,7 +4433,7 @@ static void sync_vmcs02_to_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > (vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE); > > if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_DEBUG_CONTROLS) > - vmcs12->guest_dr7 = kvm_get_dr(vcpu, 7); > + vmcs12->guest_dr7 = vcpu->arch.dr7; > > if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_IA32_EFER) > vmcs12->guest_ia32_efer = vcpu->arch.efer; > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index bfffc13f91e6..5a08d895bde6 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -5510,7 +5510,7 @@ static void kvm_vcpu_ioctl_x86_get_debugregs(struct kvm_vcpu *vcpu, > for (i = 0; i < ARRAY_SIZE(vcpu->arch.db); i++) > dbgregs->db[i] = vcpu->arch.db[i]; > > - dbgregs->dr6 = kvm_get_dr(vcpu, 6); > + dbgregs->dr6 = vcpu->arch.dr6; > dbgregs->dr7 = vcpu->arch.dr7; > } > Reviewed-by: Mathias Krause <minipli@grsecurity.net> Nice cleanup. Thanks a lot, Sean! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] KVM: x86: Cleanup kvm_get_dr() usage 2024-02-09 22:07 [PATCH 0/2] KVM: x86: Cleanup kvm_get_dr() usage Sean Christopherson 2024-02-09 22:07 ` [PATCH 1/2] KVM: x86: Make kvm_get_dr() return a value, not use an out parameter Sean Christopherson 2024-02-09 22:07 ` [PATCH 2/2] KVM: x86: Open code all direct reads to guest DR6 and DR7 Sean Christopherson @ 2024-02-23 1:35 ` Sean Christopherson 2 siblings, 0 replies; 6+ messages in thread From: Sean Christopherson @ 2024-02-23 1:35 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Mathias Krause On Fri, 09 Feb 2024 14:07:50 -0800, Sean Christopherson wrote: > Make kvm_get_dr() use an actual return instead of a void return with an > output, which led to a _lot_ of ugly code, and then open code all direct > reads to DR6 and DR7, as KVM has a goofy mix of some flows open coding > reads and some flows bouncing through kvm_get_dr(). > > Sean Christopherson (2): > KVM: x86: Make kvm_get_dr() return a value, not use an out parameter > KVM: x86: Open code all direct reads to guest DR6 and DR7 > > [...] Applied to kvm-x86 misc, thanks! [1/2] KVM: x86: Make kvm_get_dr() return a value, not use an out parameter https://github.com/kvm-x86/linux/commit/fc5375dd8c06 [2/2] KVM: x86: Open code all direct reads to guest DR6 and DR7 https://github.com/kvm-x86/linux/commit/2a5f091ce1c9 -- https://github.com/kvm-x86/linux/tree/next ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-23 1:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-02-09 22:07 [PATCH 0/2] KVM: x86: Cleanup kvm_get_dr() usage Sean Christopherson 2024-02-09 22:07 ` [PATCH 1/2] KVM: x86: Make kvm_get_dr() return a value, not use an out parameter Sean Christopherson 2024-02-09 22:29 ` Mathias Krause 2024-02-09 22:07 ` [PATCH 2/2] KVM: x86: Open code all direct reads to guest DR6 and DR7 Sean Christopherson 2024-02-09 22:30 ` Mathias Krause 2024-02-23 1:35 ` [PATCH 0/2] KVM: x86: Cleanup kvm_get_dr() usage Sean Christopherson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®