Re: [PATCH v3 3/7] KVM: x86/pmu: Avoid using PEBS perf_events for normal counters

[Sean Christopherson]

On Wed, Aug 31, 2022, Like Xu wrote:
> To address this issue, the reuse check has been revamped and KVM will

State what the patch does as a command, don't describe the effects in the past
tense.

> go back to do reprogram_counter() when any bit of guest PEBS_ENABLE
> msr has changed, which is similar to what global_ctrl_changed() does.

This managed to confuse the heck out of me.  I misread reprogram_counter() as
reprogram_counters() because that's what I see in the diff, and then got all
turned around because this patch also stealthily renames global_ctrl_changed() to
reprogram_counters().

This is a very good example of when splitting a refactor with a bug fix can help
tremendously, e.g. with the refactor moved to a separate patch, this fix becomes:

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 390d697efde1..d9b9a0f0db17 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -237,8 +237,8 @@ static bool pmc_resume_counter(struct kvm_pmc *pmc)
                              get_sample_period(pmc, pmc->counter)))
                return false;
 
-       if (!test_bit(pmc->idx, (unsigned long *)&pmc_to_pmu(pmc)->pebs_enable) &&
-           pmc->perf_event->attr.precise_ip)
+       if (test_bit(pmc->idx, (unsigned long *)&pmc_to_pmu(pmc)->pebs_enable) !=
+           (!!pmc->perf_event->attr.precise_ip))
                return false;
 
        /* reuse perf_event to serve as pmc_reprogram_counter() does*/
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 5592b1259e1b..25b70a85bef5 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -431,7 +431,9 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                if (pmu->pebs_enable == data)
                        return 0;
                if (!(data & pmu->pebs_enable_mask)) {
+                       diff = pmu->pebs_enable ^ data;
                        pmu->pebs_enable = data;
+                       reprogram_counters(pmu, diff);
                        return 0;
                }
                break;

which is much, much easier to describe and to follow.

TL;DR: I split this into two patches.

> Fixes: 79f3e3b58386 ("KVM: x86/pmu: Reprogram PEBS event to emulate guest PEBS counter")
> Signed-off-by: Like Xu <likexu@xxxxxxxxxxx>
> ---

...

> -/* function is called when global control register has been updated. */
> -static void global_ctrl_changed(struct kvm_pmu *pmu, u64 data)
> +static void reprogram_counters(struct kvm_pmu *pmu, u64 diff)