2015-04-18 02:23-0400, Wei Huang:
> This patch splits existing vPMU code into a common vPMU interface (pmc.c)
> and Intel specific vPMU code (pmu_intel.c) using the following steps:
>
> - Part of arechitectural vPMU code is extracted and moved to pmu_intel.c
> file. They are hooked up with the newly-defined intel_pmu_ops, which will
> be called from pmu interface;
> - Create a dummy pmu_amd.c file for AMD SVM with empty functions;
>
> All architectural vPMU functions are now called via PMU function dispatcher
> (kvm_pmu_ops). This function dispatcher is initialized by calling
> kvm_x86_ops->get_pmu_ops() at the beginning. Also note that Intel and AMD
> modules are now generated by combinig their corresponding arch files
> (vmx.c/svm.c) and pmu files (pmu_intel.c/pmu_amd.c).
>
> Signed-off-by: Wei Huang <wei@xxxxxxxxxx>
> ---
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> @@ -19,83 +18,41 @@
> +/* NOTE:
> + * - Each perf counter is defined as "struct kvm_pmc";
> + * - There are two types of perf counters: general purpose (gp) and fixed.
> + * gp counters are stored in gp_counters[] and fixed counters are stored
> + * in fixed_counters[] respectively. Both of them are part of "struct
> + * kvm_pmu";
> + * - pmu.c understands the difference between gp counters and fixed counters.
> + * However AMD doesn't support fixed-counters;
> + * - There are three types of index to access perf counters (PMC):
> + * 1. MSR (named msr): For example Intel has MSR_IA32_PERFCTRn and AMD
> + * has MSR_K7_PERFCTRn.
> + * 2. MSR Index (named idx):
Unless it's named msr :(
> + This normally is used by RDPMC instruction.
> + * For instance AMD RDPMC instruction uses 0000_0003h in ECX to access
> + * C001_0007h (MSR_K7_PERCTR3). Intel has a similar mechanism, except
> + * that it also supports fixed counters. idx can be used to as index to
> + * gp and fixed counters.
> + * 3. Global PMC Index (named pmc_idx): pmc_idx is an index specific to PMU
> + * code. Each pmc_idx, stored in kvm_pmc.idx field, is unique across
> + * all perf counters (both gp and fixed). The mapping relationship
> + * between pmc_idx and perf counters is as the following:
> + * * Intel: [0 .. INTEL_PMC_MAX_GENERIC-1] <=> gp counters
> + * [INTEL_PMC_IDX_FIXED .. INTEL_PMC_IDX_FIXED + 2] <=> fixed
> + * * AMD: [0 .. AMD64_NUM_COUNTERS-1] <=> gp counters
> + */
The declaration from [1/4] will hopefully help to show what I dislike:
struct kvm_pmu_ops {
int (*check_msr)(struct kvm_vcpu *vcpu, unsigned msr);
struct kvm_pmc *(*msr_to_pmc)(struct kvm_vcpu *vcpu, unsigned idx);
bool (*is_pmu_msr)(struct kvm_vcpu *vcpu, u32 msr);
int (*get_msr)(struct kvm_vcpu *vcpu, u32 index, u64 *data);
int (*set_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
};
This makes you think how to use it those two similar checks and what you
gain by converting to pmc ...
There are actually two groups of meaning for msr
1) check_msr, msr_to_pmc (msr = PMC identifier)
2) is_pmu_msr, get_msr, set_msr (msr = MSR identifier)
And even after you know there are two meanings, only the position in
declaration really helps to distinguish them, which is far from what I'd
call good naming for API.
(I think that 'check_msr' goes well with 'get_msr' and 'set_msr', and
wrappers just prepend 'kvm_pmu_'.)
Any different names (ideally not very similar) would work better.
--
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
