Re: [PATCH V5 0/4] Consolidated KVM vPMU support for x86

[Wei Huang <wei@redhat.com>]

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