Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c5cfa5b8b33ed4ef8d1aff16ff0212e4d0ae30cd.camel@redhat.com>
Subject: Re: [PATCH v4 32/32] KVM: x86: Track required APICv inhibits with
 variable, not callback
From:   Maxim Levitsky <mlevitsk@redhat.com>
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Alejandro Jimenez <alejandro.j.jimenez@oracle.com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        Li RongQing <lirongqing@baidu.com>
Date:   Fri, 09 Dec 2022 00:03:35 +0200
In-Reply-To: <20221001005915.2041642-33-seanjc@google.com>
References: <20221001005915.2041642-1-seanjc@google.com>
         <20221001005915.2041642-33-seanjc@google.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5 (3.36.5-2.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote:
> Track the per-vendor required APICv inhibits with a variable instead of
> calling into vendor code every time KVM wants to query the set of
> required inhibits.  The required inhibits are a property of the vendor's
> virtualization architecture, i.e. are 100% static.
> 
> Using a variable allows the compiler to inline the check, e.g. generate
> a single-uop TEST+Jcc, and thus eliminates any desire to avoid checking
> inhibits for performance reasons.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/kvm-x86-ops.h |  1 -
>  arch/x86/include/asm/kvm_host.h    |  2 +-
>  arch/x86/kvm/svm/avic.c            | 20 --------------------
>  arch/x86/kvm/svm/svm.c             |  2 +-
>  arch/x86/kvm/svm/svm.h             | 17 ++++++++++++++++-
>  arch/x86/kvm/vmx/vmx.c             | 24 +++++++++++-------------
>  arch/x86/kvm/x86.c                 |  9 +++++++--
>  7 files changed, 36 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 82ba4a564e58..533d0e804e5a 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -76,7 +76,6 @@ KVM_X86_OP(set_nmi_mask)
>  KVM_X86_OP(enable_nmi_window)
>  KVM_X86_OP(enable_irq_window)
>  KVM_X86_OP_OPTIONAL(update_cr8_intercept)
> -KVM_X86_OP(check_apicv_inhibit_reasons)
>  KVM_X86_OP(refresh_apicv_exec_ctrl)
>  KVM_X86_OP_OPTIONAL(hwapic_irr_update)
>  KVM_X86_OP_OPTIONAL(hwapic_isr_update)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 4fd06965c773..41d1c684a1e1 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1566,7 +1566,7 @@ struct kvm_x86_ops {
>  	void (*enable_nmi_window)(struct kvm_vcpu *vcpu);
>  	void (*enable_irq_window)(struct kvm_vcpu *vcpu);
>  	void (*update_cr8_intercept)(struct kvm_vcpu *vcpu, int tpr, int irr);
> -	bool (*check_apicv_inhibit_reasons)(enum kvm_apicv_inhibit reason);
> +	const unsigned long required_apicv_inhibits;
>  	void (*refresh_apicv_exec_ctrl)(struct kvm_vcpu *vcpu);
>  	void (*hwapic_irr_update)(struct kvm_vcpu *vcpu, int max_irr);
>  	void (*hwapic_isr_update)(int isr);
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 35b0ef877e53..5b25944274b5 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -966,26 +966,6 @@ int avic_pi_update_irte(struct kvm *kvm, unsigned int host_irq,
>  	return ret;
>  }
>  
> -bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason)
> -{
> -	ulong supported = BIT(APICV_INHIBIT_REASON_DISABLE) |
> -			  BIT(APICV_INHIBIT_REASON_ABSENT) |
> -			  BIT(APICV_INHIBIT_REASON_HYPERV) |
> -			  BIT(APICV_INHIBIT_REASON_NESTED) |
> -			  BIT(APICV_INHIBIT_REASON_IRQWIN) |
> -			  BIT(APICV_INHIBIT_REASON_PIT_REINJ) |
> -			  BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |
> -			  BIT(APICV_INHIBIT_REASON_SEV)      |
> -			  BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |
> -			  BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |
> -			  BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |
> -			  BIT(APICV_INHIBIT_REASON_X2APIC) |
> -			  BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED);
> -
> -	return supported & BIT(reason);
> -}
> -
> -
>  static inline int
>  avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
>  {
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 37fe7bcf8496..6bca3f9f39e6 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4800,8 +4800,8 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>  	.update_cr8_intercept = svm_update_cr8_intercept,
>  	.set_virtual_apic_mode = avic_refresh_virtual_apic_mode,
>  	.refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
> -	.check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
>  	.apicv_post_state_restore = avic_apicv_post_state_restore,
> +	.required_apicv_inhibits = AVIC_REQUIRED_APICV_INHIBITS,
>  
>  	.get_exit_info = svm_get_exit_info,
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 29c334a932c3..dfde85782da5 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -619,6 +619,22 @@ void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb);
>  extern struct kvm_x86_nested_ops svm_nested_ops;
>  
>  /* avic.c */
> +#define AVIC_REQUIRED_APICV_INHIBITS			\
> +(							\
> +	BIT(APICV_INHIBIT_REASON_DISABLE) |		\
> +	BIT(APICV_INHIBIT_REASON_ABSENT) |		\
> +	BIT(APICV_INHIBIT_REASON_HYPERV) |		\
> +	BIT(APICV_INHIBIT_REASON_NESTED) |		\
> +	BIT(APICV_INHIBIT_REASON_IRQWIN) |		\
> +	BIT(APICV_INHIBIT_REASON_PIT_REINJ) |		\
> +	BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |		\
> +	BIT(APICV_INHIBIT_REASON_SEV)      |		\
> +	BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |	\
> +	BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |	\
> +	BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED) |	\
> +	BIT(APICV_INHIBIT_REASON_X2APIC) |		\
> +	BIT(APICV_INHIBIT_REASON_LOGICAL_ID_ALIASED)	\
> +)
>  
>  bool avic_hardware_setup(struct kvm_x86_ops *ops);
>  int avic_ga_log_notifier(u32 ga_tag);
> @@ -632,7 +648,6 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
>  void avic_vcpu_put(struct kvm_vcpu *vcpu);
>  void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu);
>  void avic_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu);
> -bool avic_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason);
>  int avic_pi_update_irte(struct kvm *kvm, unsigned int host_irq,
>  			uint32_t guest_irq, bool set);
>  void avic_vcpu_blocking(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 5920166d7260..54bc447ba87e 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7949,18 +7949,16 @@ static void vmx_hardware_unsetup(void)
>  	free_kvm_area();
>  }
>  
> -static bool vmx_check_apicv_inhibit_reasons(enum kvm_apicv_inhibit reason)
> -{
> -	ulong supported = BIT(APICV_INHIBIT_REASON_DISABLE) |
> -			  BIT(APICV_INHIBIT_REASON_ABSENT) |
> -			  BIT(APICV_INHIBIT_REASON_HYPERV) |
> -			  BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |
> -			  BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |
> -			  BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |
> -			  BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED);
> -
> -	return supported & BIT(reason);
> -}
> +#define VMX_REQUIRED_APICV_INHIBITS			\
> +(							\
> +	BIT(APICV_INHIBIT_REASON_DISABLE)|		\
> +	BIT(APICV_INHIBIT_REASON_ABSENT) |		\
> +	BIT(APICV_INHIBIT_REASON_HYPERV) |		\
> +	BIT(APICV_INHIBIT_REASON_BLOCKIRQ) |		\
> +	BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_ALIASED) |	\
> +	BIT(APICV_INHIBIT_REASON_APIC_ID_MODIFIED) |	\
> +	BIT(APICV_INHIBIT_REASON_APIC_BASE_MODIFIED)	\
> +)
>  
>  static void vmx_vm_destroy(struct kvm *kvm)
>  {
> @@ -8044,7 +8042,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
>  	.refresh_apicv_exec_ctrl = vmx_refresh_apicv_exec_ctrl,
>  	.load_eoi_exitmap = vmx_load_eoi_exitmap,
>  	.apicv_post_state_restore = vmx_apicv_post_state_restore,
> -	.check_apicv_inhibit_reasons = vmx_check_apicv_inhibit_reasons,
> +	.required_apicv_inhibits = VMX_REQUIRED_APICV_INHIBITS,
>  	.hwapic_irr_update = vmx_hwapic_irr_update,
>  	.hwapic_isr_update = vmx_hwapic_isr_update,
>  	.guest_apic_has_interrupt = vmx_guest_apic_has_interrupt,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a20002924eb4..b307637d9200 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10251,6 +10251,11 @@ void kvm_make_scan_ioapic_request(struct kvm *kvm)
>  	kvm_make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
>  }
>  
> +static bool kvm_is_required_apicv_inhibit(enum kvm_apicv_inhibit reason)
> +{
> +	return kvm_x86_ops.required_apicv_inhibits & BIT(reason);
> +}
> +
>  void __kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
> @@ -10294,7 +10299,7 @@ static void kvm_vcpu_update_apicv(struct kvm_vcpu *vcpu)
>  		return;
>  
>  	if (apic_x2apic_mode(vcpu->arch.apic) &&
> -	    static_call(kvm_x86_check_apicv_inhibit_reasons)(APICV_INHIBIT_REASON_X2APIC))
> +	    kvm_is_required_apicv_inhibit(APICV_INHIBIT_REASON_X2APIC))
>  		kvm_inhibit_apic_access_page(vcpu);
>  
>  	__kvm_vcpu_update_apicv(vcpu);
> @@ -10307,7 +10312,7 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm,
>  
>  	lockdep_assert_held_write(&kvm->arch.apicv_update_lock);
>  
> -	if (!static_call(kvm_x86_check_apicv_inhibit_reasons)(reason))
> +	if (!kvm_is_required_apicv_inhibit(reason))
>  		return;
>  
>  	old = new = kvm->arch.apicv_inhibit_reasons;


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky