On Wed, Jan 20, 2021, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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.
