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

[Wei Huang <wei@redhat.com>]

On 4/9/15 14:43, Radim Krčmář wrote:
> 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.)
Some of them were carried from original pmu.c file. I will purge them in V3.

>
> | [...]
>> +EXPORT_SYMBOL_GPL(reprogram_counter);
>> +
>
> (double line)
>
OK

>> +
>> +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.)
OK

>
>> +{
>> +	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.)
>
OK, I will move them to pmu.h header as inline.

>> +{
>> +	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.)
OK

>
>> +}
> | [...]
>> +/* 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 ^^.
>
Ahh, will fix it.

>> +
>> +	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.)
>
OK, will make it to lower case.

>> +/* 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.)
>
OK, agreed that such changes can be separated. I will extract them to 
make your life easier.

>> -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?)
Original kvm_pmu_read_pmc() was only used for RDPMC instruction. Such 
change make it easier to be correlated with RDPMC directly; plus pmc is 
a confusing term in vPMU code (see "Design Note" in pmu.c file), which 
makes me to think kvm_pmu_read_pmc() should read from "struct kvm_pmc" 
directly.

>
>>   	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".)
>
OK

>> @@ -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?)
See "Design Note" in pmu.c for a better explanation. I tried to use msr 
as real x86 MSR; and msr_idx refers to MSR offset.

>