Re: [PATCH v3 6/6] KVM: x86/vPMU: Add lazy mechanism to release perf_event per vPMC

[Like Xu]

Hi Paolo,
On 2019/10/22 18:47, Paolo Bonzini wrote:
On 21/10/19 18:06, Like Xu wrote:
  
+		__set_bit(INTEL_PMC_IDX_FIXED + i, pmu->pmc_in_use);
                reprogram_fixed_counter(pmc, new_ctrl, i);
        }
  
@@ -329,6 +330,11 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
            (boot_cpu_has(X86_FEATURE_HLE) || boot_cpu_has(X86_FEATURE_RTM)) &&
            (entry->ebx & (X86_FEATURE_HLE|X86_FEATURE_RTM)))
                pmu->reserved_bits ^= HSW_IN_TX|HSW_IN_TX_CHECKPOINTED;
+
+       bitmap_set(pmu->all_valid_pmc_idx,
+               0, pmu->nr_arch_gp_counters);
+       bitmap_set(pmu->all_valid_pmc_idx,
+               INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);

The offset needs to be INTEL_PMC_IDX_FIXED for GP counters, and 0 for
fixed counters, otherwise pmc_in_use and all_valid_pmc_idx are not in sync.


First, the bitmap_set is declared as:

        static __always_inline void bitmap_set(unsigned long *map,
        unsigned int start, unsigned int nbits)

Second, the structure of pmu->pmc_in_use is in the following format:

  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

Then let me translate your suggestion to the following code:

        bitmap_set(pmu->all_valid_pmc_idx, 0,
                   pmu->nr_arch_fixed_counters);
        bitmap_set(pmu->all_valid_pmc_idx, INTEL_PMC_IDX_FIXED,
                   pmu->nr_arch_gp_counters);

and the above code doesn't pass the following verification patch:

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index a8793f965941..0a73bc8c587d 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -469,6 +469,7 @@ void kvm_pmu_cleanup(struct kvm_vcpu *vcpu)

        /* release events for unmarked vPMCs in the last sched time 
slice */
        for_each_set_bit(i, bitmask, X86_PMC_IDX_MAX) {
+               pr_info("%s, do cleanup check for i = %d", __func__, i);
                pmc = kvm_x86_ops->pmu_ops->pmc_idx_to_pmc(pmu, i);

                if (pmc && pmc->perf_event && !pmc_speculative_in_use(pmc))

The print message would never stop after the guest user finishes the
perf command and it's checking the invalid idx for i = 35 unexpectedly.

However, my code does work just as you suggest.

By the way, how about other kvm patches?

Paolo