Re: [PATCH V2 3/5] KVM: x86/vPMU: Create vPMU interface for VMX and SVM

["Radim Krčmář" <rkrcmar@redhat.com>]

2015-04-08 12:18-0400, Wei Huang:
> This patch converts existing Intel specific vPMU code into a common vPMU
> interface with the following steps:
> 
> - A large portion of Intel vPMU code is now moved to pmu.c file. The rest
>   is re-arranged and hooked up with the newly-defined intel_pmu_ops.
> - Create a corresponding pmu_amd.c file with empty functions for AMD SVM
> - The PMU function pointer, kvm_pmu_ops, is initialized by calling
>   kvm_x86_ops->get_pmu_ops().
> - To reduce the code size, Intel and AMD modules are now generated
>   from their corrponding arch and PMU files; In the meanwhile, due
>   to this arrangement several functions are exposed as public ones
>   to allow calling from PMU code.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
| [...]
> +static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type, 
> +				  unsigned config, bool exclude_user, 
> +				  bool exclude_kernel, bool intr, 

This patch introduces a lot of trailing whitespaces, please remove them.
(`git am` says 15.)

| [...]
> +EXPORT_SYMBOL_GPL(reprogram_counter);
> +

(double line)

> +
> +void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
| [...]
> +/* check if msr_idx is a valid index to access PMU */
> +inline int kvm_pmu_check_msr_idx(struct kvm_vcpu *vcpu, unsigned msr_idx)

If we really want it inline, it's better done in header.
(I think GCC would inline this in-module anyway, but other modules still
 have to call it.)

> +{
> +	return kvm_pmu_ops->check_msr_idx(vcpu, msr_idx);
> +}
> +
| [...]
> +bool kvm_pmu_is_msr(struct kvm_vcpu *vcpu, u32 msr)

(Might make sense to inline these trivial wrappers.)

> +{
> +	return kvm_pmu_ops->is_pmu_msr(vcpu, msr);
> +}
> +
> +int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
> +{
> +	return kvm_pmu_ops->get_msr(vcpu, msr, data);
> +}
> +
> +int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> +{
> +	return kvm_pmu_ops->set_msr(vcpu, msr_info);
> +	

(Technically also a trailing newline.)

> +}
| [...]
> +/* Called before using any PMU functions above */
> +int kvm_pmu_arch_init(struct kvm_x86_ops *x86_ops)
> +{
> +	kvm_pmu_ops = x86_ops->get_pmu_ops();

I guess you forgot this line ^^.

> +
> +	if (x86_ops && (kvm_pmu_ops = x86_ops->get_pmu_ops()) != NULL)
> +		return 0;
> +	else
> +		return -EINVAL;
> +}
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> @@ -0,0 +1,98 @@
> +#ifndef __KVM_X86_PMU_H
> +#define __KVM_X86_PMU_H
> +
> +#define VCPU_TO_PMU(vcpu) (&(vcpu)->arch.pmu)
> +#define PMU_TO_VCPU(pmu)  (container_of((pmu), struct kvm_vcpu, arch.pmu))
> +#define PMC_TO_PMU(pmc)   (&(pmc)->vcpu->arch.pmu)

(We are replacing inline functions with these macros and they wouldn't
 have been in caps, so I would use lower case; looks better too, IMO.)

> +/* retrieve the control fields of IA32_FIXED_CTR_CTRL */
> +#define fixed_ctr_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
| [...]
> diff --git a/arch/x86/kvm/pmu_intel.c b/arch/x86/kvm/pmu_intel.c
> @@ -328,20 +156,21 @@ bool kvm_pmu_msr(struct kvm_vcpu *vcpu, u32 msr)
>  		ret = pmu->version > 1;
>  		break;
>  	default:
> -		ret = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)
> -			|| get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0)
> -			|| get_fixed_pmc(pmu, msr);
> +		ret = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) ||
> +			get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0) ||
> +			get_fixed_pmc(pmu, msr);
>  		break;
>  	}
> +
>  	return ret;
>  }

(This hunk and the following hunks where you mostly change index -> msr
 could have been in a separate cleanup patch.)

> -int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data)
> +static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
| [...]
> -int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> +static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
| [...]
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 32bf19e..4d9e7de 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -28,6 +28,7 @@
>  #include "x86.h"
>  #include "cpuid.h"
>  #include "assigned-dev.h"
> +#include "pmu.h"
>  
>  #include <linux/clocksource.h>
>  #include <linux/interrupt.h>
> @@ -899,7 +900,7 @@ bool kvm_rdpmc(struct kvm_vcpu *vcpu)
>  	u64 data;
>  	int err;
>  
> -	err = kvm_pmu_read_pmc(vcpu, ecx, &data);
> +	err = kvm_pmu_rdpmc(vcpu, ecx, &data);

(What was the reason behind the new name?)

>  	if (err)
>  		return err;
>  	kvm_register_write(vcpu, VCPU_REGS_RAX, (u32)data);
> @@ -2279,7 +2280,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		pr = true;
>  	case MSR_P6_EVNTSEL0:
>  	case MSR_P6_EVNTSEL1:
> -		if (kvm_pmu_msr(vcpu, msr))
> +		if (kvm_pmu_is_msr(vcpu, msr))

(I liked kvm_pmu_msr better.  The 'is' is in a wrong spot ... we know it
 is "a MSR", we want to know if it is "PMU MSR".)

> @@ -4918,13 +4919,13 @@ static int emulator_set_msr(struct x86_emulate_ctxt *ctxt,
>  static int emulator_check_pmc(struct x86_emulate_ctxt *ctxt,
>  			      u32 pmc)
>  {
> -	return kvm_pmu_check_pmc(emul_to_vcpu(ctxt), pmc);
> +	return kvm_pmu_check_msr_idx(emul_to_vcpu(ctxt), pmc);

(Why not pmc?)