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

Wei Huang
2015-04-09T20:03:51Z

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.

>