Re: [PATCH 2/3] KVM: x86: Simplify kvm_vcpu_ioctl_x86_get_debugregs()

[Sean Christopherson <seanjc@google.com>]

On Tue, Feb 06, 2024, Mathias Krause wrote:
> On 05.02.24 20:53, Sean Christopherson wrote:
> > On Sat, Feb 03, 2024, Mathias Krause wrote:
> >> Take 'dr6' from the arch part directly as already done for 'dr7'.
> >> There's no need to take the clunky route via kvm_get_dr().
> >>
> >> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> >> ---
> >>  arch/x86/kvm/x86.c | 5 +----
> >>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >> index 13ec948f3241..0f958dcf8458 100644
> >> --- a/arch/x86/kvm/x86.c
> >> +++ b/arch/x86/kvm/x86.c
> >> @@ -5504,12 +5504,9 @@ 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;
> >> -
> >>  	memset(dbgregs, 0, sizeof(*dbgregs));
> >>  	memcpy(dbgregs->db, vcpu->arch.db, sizeof(vcpu->arch.db));
> >> -	kvm_get_dr(vcpu, 6, &val);
> >> -	dbgregs->dr6 = val;
> >> +	dbgregs->dr6 = vcpu->arch.dr6;
> > 
> > Blech, kvm_get_dr() is so dumb, it takes an out parameter despite have a void
> > return.
> 
> Jepp, that's why I tried to get rid of it.
> 
> >          I would rather fix that wart and go the other direction, i.e. make dr7
> > go through kvm_get_dr().  This obviously isn't a fast path, so the extra CALL+RET
> > is a non-issue.
> 
> Okay. I thought, as this is an indirect call which is slightly more
> expensive under RETPOLINE, I'd go the other way and simply open-code the
> access, as done a few lines below in kvm_vcpu_ioctl_x86_set_debugregs().

It's not an indirect call.  It's not even strictly guaranteed to be a function
call, e.g. within x86.c, kvm_get_dr() is in scope of kvm_vcpu_ioctl_x86_get_debugregs()
and so a very smart compiler could fully inline and optimize it to just
"xxx = vcpu->arch.dr6" through dead code elimination (I doubt gcc or clang actually
does this, but it's possible).