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@xxxxxxxxxx>
> ---
> 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?)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
