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.
>
> 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'.
>
>> hv_vcpu->exit.type = KVM_EXIT_HYPERV_SYNIC;
>> hv_vcpu->exit.u.synic.msr = msr;
>> @@ -294,7 +294,7 @@ static int kvm_hv_syndbg_complete_userspace(struct kvm_vcpu *vcpu)
>> static void syndbg_exit(struct kvm_vcpu *vcpu, u32 msr)
>> {
>> struct kvm_hv_syndbg *syndbg = vcpu_to_hv_syndbg(vcpu);
>> - struct kvm_vcpu_hv *hv_vcpu = &vcpu->arch.hyperv;
>> + struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
>>
>> hv_vcpu->exit.type = KVM_EXIT_HYPERV_SYNDBG;
>> hv_vcpu->exit.u.syndbg.msr = msr;
>> @@ -840,7 +840,9 @@ void kvm_hv_vcpu_uninit(struct kvm_vcpu *vcpu)
>>
>> bool kvm_hv_assist_page_enabled(struct kvm_vcpu *vcpu)
>> {
>> - if (!(vcpu->arch.hyperv.hv_vapic & HV_X64_MSR_VP_ASSIST_PAGE_ENABLE))
>> + struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
>> +
>> + if (!(hv_vcpu->hv_vapic & HV_X64_MSR_VP_ASSIST_PAGE_ENABLE))
>> return false;
>> return vcpu->arch.pv_eoi.msr_val & KVM_MSR_ENABLED;
>> }
>> @@ -1216,7 +1218,7 @@ static u64 current_task_runtime_100ns(void)
>>
>> static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host)
>> {
>> - struct kvm_vcpu_hv *hv_vcpu = &vcpu->arch.hyperv;
>> + struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
>>
>> switch (msr) {
>> case HV_X64_MSR_VP_INDEX: {
>> @@ -1379,7 +1381,7 @@ static int kvm_hv_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata,
>> bool host)
>> {
>> u64 data = 0;
>> - struct kvm_vcpu_hv *hv_vcpu = &vcpu->arch.hyperv;
>> + struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
>>
>> switch (msr) {
>> case HV_X64_MSR_VP_INDEX:
>> @@ -1494,7 +1496,7 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *current_vcpu, u64 ingpa,
>> u16 rep_cnt, bool ex)
>> {
>> struct kvm *kvm = current_vcpu->kvm;
>
> Ugh, "current_vcpu". That's really, really nasty, as it's silently shadowing a
> global per-cpu variable. E.g. this compiles without so much as a warning:
>
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 922c69dcca4d..142fe9c12957 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -1490,7 +1490,7 @@ static __always_inline unsigned long *sparse_set_to_vcpu_mask(
> return vcpu_bitmap;
> }
>
> -static u64 kvm_hv_flush_tlb(struct kvm_vcpu *current_vcpu, u64 ingpa,
> +static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, u64 ingpa,
> u16 rep_cnt, bool ex)
> {
> struct kvm *kvm = current_vcpu->kvm;
> @@ -1592,7 +1592,7 @@ static void kvm_send_ipi_to_many(struct kvm *kvm, u32 vector,
> }
> }
>
> -static u64 kvm_hv_send_ipi(struct kvm_vcpu *current_vcpu, u64 ingpa, u64 outgpa,
> +static u64 kvm_hv_send_ipi(struct kvm_vcpu *vcpu, u64 ingpa, u64 outgpa,
> bool ex, bool fast)
> {
> struct kvm *kvm = current_vcpu->kvm;
>
My memory tells me both these functions had local 'vcpu' variable to
iterate over all vCPUs but it's not there now, I'll send a patch to drop
'current_vcpu'.
>> - struct kvm_vcpu_hv *hv_vcpu = ¤t_vcpu->arch.hyperv;
>> + struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(current_vcpu);
>> struct hv_tlb_flush_ex flush_ex;
>> struct hv_tlb_flush flush;
>> u64 vp_bitmap[KVM_HV_MAX_SPARSE_VCPU_SET_BITS];
>> diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
>> index 6d7def2b0aad..6300038e7a52 100644
>> --- a/arch/x86/kvm/hyperv.h
>> +++ b/arch/x86/kvm/hyperv.h
>> @@ -114,7 +114,9 @@ static inline struct kvm_vcpu *stimer_to_vcpu(struct kvm_vcpu_hv_stimer *stimer)
>>
>> static inline bool kvm_hv_has_stimer_pending(struct kvm_vcpu *vcpu)
>> {
>> - return !bitmap_empty(vcpu->arch.hyperv.stimer_pending_bitmap,
>> + struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
>> +
>> + return !bitmap_empty(hv_vcpu->stimer_pending_bitmap,
>> HV_SYNIC_STIMER_COUNT);
>> }
>>
>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>> index 4fb86e3a9dd3..dec7356f2fcd 100644
>> --- a/arch/x86/kvm/lapic.h
>> +++ b/arch/x86/kvm/lapic.h
>> @@ -6,6 +6,8 @@
>>
>> #include <linux/kvm_host.h>
>>
>> +#include "hyperv.h"
>> +
>> #define KVM_APIC_INIT 0
>> #define KVM_APIC_SIPI 1
>> #define KVM_APIC_LVT_NUM 6
>> @@ -127,7 +129,9 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
>>
>> static inline bool kvm_hv_vapic_assist_page_enabled(struct kvm_vcpu *vcpu)
>> {
>> - return vcpu->arch.hyperv.hv_vapic & HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
>> + struct kvm_vcpu_hv *hv_vcpu = vcpu_to_hv_vcpu(vcpu);
>> +
>> + return hv_vcpu->hv_vapic & HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
>
> A short to_hyperv() would be nice here, e.g.
>
> return to_hyperv(vcpu)->hv_vapic & HV_X64_MSR_VP_ASSIST_PAGE_ENABLE;
>
>
> LOL, actually, kvm_hv_vapic_assist_page_enabled() doesn't have any callers and
> can be dropped. Looks likes it's supplanted by kvm_hv_assist_page_enabled().
>
:-)
>> }
>>
>> int kvm_lapic_enable_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len);
>
--
Vitaly
