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@xxxxxxxxxxxxxx>
> >> ---
> >> 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).
