linux-kernel - Re: [PATCH 2/3] KVM: x86: Introduce definitions to support static calls for kvm_pmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YYVSW4Jr75oJ6MhC@google.com>
Date:   Fri, 5 Nov 2021 15:48:43 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Like Xu <like.xu.linux@...il.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] KVM: x86: Introduce definitions to support static
 calls for kvm_pmu_ops

On Wed, Nov 03, 2021, Like Xu wrote:
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 0db1887137d9..b6f08c719125 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -50,6 +50,13 @@
>  struct kvm_pmu_ops kvm_pmu_ops __read_mostly;
>  EXPORT_SYMBOL_GPL(kvm_pmu_ops);
>  
> +#define	KVM_X86_PMU_OP(func)	\
> +	DEFINE_STATIC_CALL_NULL(kvm_x86_pmu_##func,	\
> +				*(((struct kvm_pmu_ops *)0)->func))
> +#define	KVM_X86_PMU_OP_NULL	KVM_X86_PMU_OP

More of a question for the existing code, what's the point of KVM_X86_OP_NULL?
AFAICT, it always resolves to KVM_X86_OP.  Unless there's some magic I'm missing,
I vote we remove KVM_X86_OP_NULL and then not introduce KVM_X86_PMU_OP_NULL.
And I'm pretty sure it's useless, e.g. get_cs_db_l_bits is defined with the NULL
variant, but it's never NULL and its calls aren't guarded with anything.  And if
KVM_X86_OP_NULL is intended to aid in documenting behavior, it's doing a pretty
miserable job of that :-)

> +#include <asm/kvm-x86-pmu-ops.h>
> +EXPORT_STATIC_CALL_GPL(kvm_x86_pmu_is_valid_msr);

I'll double down on my nVMX suggestion so that this export can be avoided.

>  static void kvm_pmi_trigger_fn(struct irq_work *irq_work)
>  {
>  	struct kvm_pmu *pmu = container_of(irq_work, struct kvm_pmu, irq_work);
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index b2fe135d395a..e5550d4acf14 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -3,6 +3,8 @@
>  #define __KVM_X86_PMU_H
>  
>  #include <linux/nospec.h>
> +#include <linux/static_call_types.h>
> +#include <linux/static_call.h>
>  
>  #define vcpu_to_pmu(vcpu) (&(vcpu)->arch.pmu)
>  #define pmu_to_vcpu(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
> @@ -45,6 +47,19 @@ struct kvm_pmu_ops {
>  	void (*cleanup)(struct kvm_vcpu *vcpu);
>  };
>  
> +#define	KVM_X86_PMU_OP(func)	\
> +	DECLARE_STATIC_CALL(kvm_x86_pmu_##func, *(((struct kvm_pmu_ops *)0)->func))
> +#define	KVM_X86_PMU_OP_NULL	KVM_X86_PMU_OP
> +#include <asm/kvm-x86-pmu-ops.h>
> +
> +static inline void kvm_pmu_ops_static_call_update(void)
> +{
> +#define	KVM_X86_PMU_OP(func)	\
> +	static_call_update(kvm_x86_pmu_##func, kvm_pmu_ops.func)
> +#define	KVM_X86_PMU_OP_NULL	KVM_X86_PMU_OP
> +#include <asm/kvm-x86-pmu-ops.h>
> +}

As alluded to in patch 01, I'd prefer these go in kvm_ops_static_call_update()
to keep the static call magic somewhat contained.

> +
>  static inline u64 pmc_bitmask(struct kvm_pmc *pmc)
>  {
>  	struct kvm_pmu *pmu = pmc_to_pmu(pmc);
> -- 
> 2.33.0
>