Re: [PATCH 3/7] KVM: x86: hyper-v: Always use vcpu_to_hv_vcpu() accessor to get to 'struct kvm_vcpu_hv'

[Sean Christopherson <seanjc@google.com>]

On Wed, Jan 20, 2021, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Wed, Jan 13, 2021, Vitaly Kuznetsov wrote:
> >> As a preparation to allocating Hyper-V context dynamically, make it clear
> >> who's the user of the said context.
> >> 
> >> No functional change intended.
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> ---
> >>  arch/x86/kvm/hyperv.c  | 14 ++++++++------
> >>  arch/x86/kvm/hyperv.h  |  4 +++-
> >>  arch/x86/kvm/lapic.h   |  6 +++++-
> >>  arch/x86/kvm/vmx/vmx.c |  9 ++++++---
> >>  arch/x86/kvm/x86.c     |  4 +++-
> >>  5 files changed, 25 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> >> index 922c69dcca4d..82f51346118f 100644
> >> --- a/arch/x86/kvm/hyperv.c
> >> +++ b/arch/x86/kvm/hyperv.c
> >> @@ -190,7 +190,7 @@ static void kvm_hv_notify_acked_sint(struct kvm_vcpu *vcpu, u32 sint)
> >>  static void synic_exit(struct kvm_vcpu_hv_synic *synic, u32 msr)
> >>  {
> >>  	struct kvm_vcpu *vcpu = synic_to_vcpu(synic);
> >> -	struct kvm_vcpu_hv *hv_vcpu = &vcpu->arch.hyperv;
> >> +	struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
> >
> > Tangentially related...
> >
> > What say you about aligning Hyper-V to VMX and SVM terminology?  E.g. I like
> > that VMX and VXM omit the "vcpu_" part and just call it "to_vmx/svm()", and the
> > VM-scoped variables have a "kvm_" prefix but the vCPU-scoped variables do not.
> > I'd probably even vote to do s/vcpu_to_pi_desc/to_pi_desc, but for whatever
> > reason that one doesn't annoy as much, probably because it's less pervasive than
> > the Hyper-V code.
> 
> Gererally I have nothing against the idea, will try to prepare a series.

Thanks!  My hope is that cleaning up the Hyper-V code will make it easier for
you to get reviews for Hyper-V patches in the future.

> > It would also help if the code were more consistent with itself.  It's all a bit
> > haphazard when it comes to variable names, using helpers (or not), etc...
> >
> > Long term, it might also be worthwhile to refactor the various flows to always
> > pass @vcpu instead of constantly converting to/from various objects.  Some of
> > the conversions appear to be necessary, e.g. for timer callbacks, but AFAICT a
> > lot of the shenanigans are entirely self-inflicted.
> >
> > E.g. stimer_set_count() has one caller, which already has @vcpu, but
> > stimer_set_count() takes @stimer instead of @vcpu and then does several
> > conversions in as many lines.  None of the conversions are super expensive, but
> > it seems like every little helper in Hyper-V is doing multiple conversions to
> > and from kvm_vcpu, and half the generated code is getting the right pointer. :-)
> 
> I *think* the idea was that everything synic-related takes a 'synic',
> everything stimer-related takes an 'stimer' and so on. While this looks
> cleaner from 'api' perspective, it indeed makes the code longer in some
> cases so I'd also agree with 'optimization'.

Makes sense.  Perhaps the middle ground is to take both @vcpu and @stimer/etc.,
to keep the APIs clean-ish.