Re: [PATCH 2/3] KVM: x86: Introduce definitions to support static calls for kvm_pmu_ops

[Like Xu <like.xu.linux@gmail.com>]

On 5/11/2021 11:48 pm, Sean Christopherson wrote:
> 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?

The comment says:

  * KVM_X86_OP_NULL() can leave a NULL definition for the
  * case where there is no definition or a function name that
  * doesn't match the typical naming convention is supplied.

Does it help ?

> 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

This transitions will not be included in the next version. Open to you.

> 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.

Fine to me.

> 
>>   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.

Thank and applied.

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