I understand that the original patch2 was huge. Thanks for taking care
of it (and the tips). I reviewed the diff below and they are reasonable.
-Wei
On 06/19/2015 10:14 AM, Paolo Bonzini wrote:
> While the code looks great, the splitting of patches 1 and 2
> left something to be desired. I've redone the split altogether
> and I'll place it soon in kvm/queue because I needed to do it myself
> to figure out some suggestions; and after 4 hours it would have been
> pointless for you to do the work again. :) Nevertheless, do take some
> time to review the single patches in order to understand the point of
> splitting the patches this way.
>
> Here is the new structuring of the patch series:
>
> * KVM: x86/vPMU: rename a few PMU functions
> * KVM: x86/vPMU: introduce pmu.h header
> * KVM: x86/vPMU: use the new macros to go between PMC, PMU and VCPU
> * KVM: x86/vPMU: whitespace and stylistic adjustments in PMU code
> * KVM: x86/vPMU: reorder PMU functions
> * KVM: x86/vPMU: introduce kvm_pmu_msr_idx_to_pmc
> * KVM: x86/vPMU: Define kvm_pmu_ops to support vPMU function dispatch
> * KVM: x86/vPMU: Implement AMD vPMU code for KVM
> * KVM: x86/vPMU: Enable PMU handling for AMD PERFCTRn and EVNTSELn MSRs
>
> The code changes compared to your patches are very minor, mostly
> undoing parts of patch 1:
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index b68b9ac847c4..5a2b4508be44 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -304,12 +304,6 @@ struct kvm_mmu {
> u64 pdptrs[4]; /* pae */
> };
>
> -struct msr_data {
> - bool host_initiated;
> - u32 index;
> - u64 data;
> -};
> -
> enum pmc_type {
> KVM_PMC_GP = 0,
> KVM_PMC_FIXED,
> @@ -324,12 +318,6 @@ struct kvm_pmc {
> struct kvm_vcpu *vcpu;
> };
>
> -struct kvm_event_hw_type_mapping {
> - u8 eventsel;
> - u8 unit_mask;
> - unsigned event_type;
> -};
> -
> struct kvm_pmu {
> unsigned nr_arch_gp_counters;
> unsigned nr_arch_fixed_counters;
> @@ -348,21 +336,7 @@ struct kvm_pmu {
> u64 reprogram_pmi;
> };
>
> -struct kvm_pmu_ops {
> - unsigned (*find_arch_event)(struct kvm_pmu *pmu, u8 event_select,
> - u8 unit_mask);
> - unsigned (*find_fixed_event)(int idx);
> - bool (*pmc_enabled)(struct kvm_pmc *pmc);
> - struct kvm_pmc *(*pmc_idx_to_pmc)(struct kvm_pmu *pmu, int pmc_idx);
> - struct kvm_pmc *(*msr_idx_to_pmc)(struct kvm_vcpu *vcpu, unsigned idx);
> - int (*is_valid_msr_idx)(struct kvm_vcpu *vcpu, unsigned idx);
> - bool (*is_valid_msr)(struct kvm_vcpu *vcpu, u32 msr);
> - int (*get_msr)(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
> - int (*set_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
> - void (*refresh)(struct kvm_vcpu *vcpu);
> - void (*init)(struct kvm_vcpu *vcpu);
> - void (*reset)(struct kvm_vcpu *vcpu);
> -};
> +struct kvm_pmu_ops;
>
> enum {
> KVM_DEBUGREG_BP_ENABLED = 1,
> @@ -725,6 +699,12 @@ struct kvm_vcpu_stat {
>
> struct x86_instruction_info;
>
> +struct msr_data {
> + bool host_initiated;
> + u32 index;
> + u64 data;
> +};
> +
> struct kvm_lapic_irq {
> u32 vector;
> u16 delivery_mode;
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index ebb5888e9d10..31aa2c85dc97 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -10,7 +10,9 @@
> *
> * This work is licensed under the terms of the GNU GPL, version 2. See
> * the COPYING file in the top-level directory.
> + *
> */
> +
> #include <linux/types.h>
> #include <linux/kvm_host.h>
> #include <linux/perf_event.h>
> @@ -88,7 +90,7 @@ static void kvm_perf_overflow_intr(struct perf_event *perf_event,
> * NMI context. Do it from irq work instead.
> */
> if (!kvm_is_in_guest())
> - irq_work_queue(&pmc->vcpu->arch.pmu.irq_work);
> + irq_work_queue(&pmc_to_pmu(pmc)->irq_work);
> else
> kvm_make_request(KVM_REQ_PMI, pmc->vcpu);
> }
> @@ -128,7 +130,7 @@ static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type,
> }
>
> pmc->perf_event = event;
> - clear_bit(pmc->idx, (unsigned long*)&pmc->vcpu->arch.pmu.reprogram_pmi);
> + clear_bit(pmc->idx, (unsigned long*)&pmc_to_pmu(pmc)->reprogram_pmi);
> }
>
> void reprogram_gp_counter(struct kvm_pmc *pmc, u64 eventsel)
> @@ -154,7 +156,7 @@ void reprogram_gp_counter(struct kvm_pmc *pmc, u64 eventsel)
> ARCH_PERFMON_EVENTSEL_CMASK |
> HSW_IN_TX |
> HSW_IN_TX_CHECKPOINTED))) {
> - config = kvm_x86_ops->pmu_ops->find_arch_event(&pmc->vcpu->arch.pmu,
> + config = kvm_x86_ops->pmu_ops->find_arch_event(pmc_to_pmu(pmc),
> event_select,
> unit_mask);
> if (config != PERF_COUNT_HW_MAX)
> @@ -305,4 +307,3 @@ void kvm_pmu_destroy(struct kvm_vcpu *vcpu)
> {
> kvm_pmu_reset(vcpu);
> }
> -
> diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
> index a19a0d5fdb07..f96e1f962587 100644
> --- a/arch/x86/kvm/pmu.h
> +++ b/arch/x86/kvm/pmu.h
> @@ -5,9 +5,31 @@
> #define pmu_to_vcpu(pmu) (container_of((pmu), struct kvm_vcpu, arch.pmu))
> #define pmc_to_pmu(pmc) (&(pmc)->vcpu->arch.pmu)
>
> -/* retrieve the control fields of IA32_FIXED_CTR_CTRL */
> +/* retrieve the 4 bits for EN and PMI out of IA32_FIXED_CTR_CTRL */
> #define fixed_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
>
> +struct kvm_event_hw_type_mapping {
> + u8 eventsel;
> + u8 unit_mask;
> + unsigned event_type;
> +};
> +
> +struct kvm_pmu_ops {
> + unsigned (*find_arch_event)(struct kvm_pmu *pmu, u8 event_select,
> + u8 unit_mask);
> + unsigned (*find_fixed_event)(int idx);
> + bool (*pmc_is_enabled)(struct kvm_pmc *pmc);
> + struct kvm_pmc *(*pmc_idx_to_pmc)(struct kvm_pmu *pmu, int pmc_idx);
> + struct kvm_pmc *(*msr_idx_to_pmc)(struct kvm_vcpu *vcpu, unsigned idx);
> + int (*is_valid_msr_idx)(struct kvm_vcpu *vcpu, unsigned idx);
> + bool (*is_valid_msr)(struct kvm_vcpu *vcpu, u32 msr);
> + int (*get_msr)(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
> + int (*set_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
> + void (*refresh)(struct kvm_vcpu *vcpu);
> + void (*init)(struct kvm_vcpu *vcpu);
> + void (*reset)(struct kvm_vcpu *vcpu);
> +};
> +
> static inline u64 pmc_bitmask(struct kvm_pmc *pmc)
> {
> struct kvm_pmu *pmu = pmc_to_pmu(pmc);
> @@ -48,7 +70,7 @@ static inline bool pmc_is_fixed(struct kvm_pmc *pmc)
>
> static inline bool pmc_is_enabled(struct kvm_pmc *pmc)
> {
> - return kvm_x86_ops->pmu_ops->pmc_enabled(pmc);
> + return kvm_x86_ops->pmu_ops->pmc_is_enabled(pmc);
> }
>
> /* returns general purpose PMC with the specified MSR. Note that it can be
> diff --git a/arch/x86/kvm/pmu_amd.c b/arch/x86/kvm/pmu_amd.c
> index ff5cea79a104..886aa25a7131 100644
> --- a/arch/x86/kvm/pmu_amd.c
> +++ b/arch/x86/kvm/pmu_amd.c
> @@ -57,7 +57,7 @@ static unsigned amd_find_fixed_event(int idx)
> /* check if a PMC is enabled by comparing it against global_ctrl bits. Because
> * AMD CPU doesn't have global_ctrl MSR, all PMCs are enabled (return TRUE).
> */
> -static bool amd_pmc_enabled(struct kvm_pmc *pmc)
> +static bool amd_pmc_is_enabled(struct kvm_pmc *pmc)
> {
> return true;
> }
> @@ -194,7 +194,7 @@ static void amd_pmu_reset(struct kvm_vcpu *vcpu)
> struct kvm_pmu_ops amd_pmu_ops = {
> .find_arch_event = amd_find_arch_event,
> .find_fixed_event = amd_find_fixed_event,
> - .pmc_enabled = amd_pmc_enabled,
> + .pmc_is_enabled = amd_pmc_is_enabled,
> .pmc_idx_to_pmc = amd_pmc_idx_to_pmc,
> .msr_idx_to_pmc = amd_msr_idx_to_pmc,
> .is_valid_msr_idx = amd_is_valid_msr_idx,
> diff --git a/arch/x86/kvm/pmu_intel.c b/arch/x86/kvm/pmu_intel.c
> index 51b4729493db..ab38af4f4947 100644
> --- a/arch/x86/kvm/pmu_intel.c
> +++ b/arch/x86/kvm/pmu_intel.c
> @@ -63,9 +63,8 @@ static void global_ctrl_changed(struct kvm_pmu *pmu, u64 data)
>
> pmu->global_ctrl = data;
>
> - for_each_set_bit(bit, (unsigned long *)&diff, X86_PMC_IDX_MAX) {
> + for_each_set_bit(bit, (unsigned long *)&diff, X86_PMC_IDX_MAX)
> reprogram_counter(pmu, bit);
> - }
> }
>
> static unsigned intel_find_arch_event(struct kvm_pmu *pmu,
> @@ -88,7 +87,6 @@ static unsigned intel_find_arch_event(struct kvm_pmu *pmu,
>
> static unsigned intel_find_fixed_event(int idx)
> {
> -
> if (idx >= ARRAY_SIZE(fixed_pmc_events))
> return PERF_COUNT_HW_MAX;
>
> @@ -96,7 +94,7 @@ static unsigned intel_find_fixed_event(int idx)
> }
>
> /* check if a PMC is enabled by comparising it with globl_ctrl bits. */
> -static bool intel_pmc_enabled(struct kvm_pmc *pmc)
> +static bool intel_pmc_is_enabled(struct kvm_pmc *pmc)
> {
> struct kvm_pmu *pmu = pmc_to_pmu(pmc);
>
> @@ -347,7 +345,7 @@ static void intel_pmu_reset(struct kvm_vcpu *vcpu)
> struct kvm_pmu_ops intel_pmu_ops = {
> .find_arch_event = intel_find_arch_event,
> .find_fixed_event = intel_find_fixed_event,
> - .pmc_enabled = intel_pmc_enabled,
> + .pmc_is_enabled = intel_pmc_is_enabled,
> .pmc_idx_to_pmc = intel_pmc_idx_to_pmc,
> .msr_idx_to_pmc = intel_msr_idx_to_pmc,
> .is_valid_msr_idx = intel_is_valid_msr_idx,
>
>
> Note how, by splitting patches, I was able to find:
>
> * some coding style violations (global_ctrl_changed, intel_find_fixed_event)
>
> * a function naming inconsistency (pmc_is_enabled calls pmu_ops->pmc_enabled)
>
> * some missed conversions to pmc_to_pmu
>
> * the unnecessary changes in patch 1 I already mentioned
>
> Interestingly, after the split git does not show anymore
> the "big" patch as rewriting pmu.c; the diffstat for that file
> is a pretty tame
>
> 1 file changed, 47 insertions(+), 336 deletions(-)
>
> where most of the insertions are the nice new header comment.
>
> The trick is to treat these kind of style changes/refactorings as
> a warning sign that you should be committing something soon. You
> can develop a git workflow, based for example on "git add -p" and
> possibly "git stash", that lets you commit them quickly without
> getting out of your previous line of thoughts. Just get them
> out of the way, you can later group them or otherwise reorder
> them as you see fit.
>
> For other examples of splitting patches, check out my SMM series
> and Xiao's patches in kvm/next.
>
> Paolo
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
