Re: [PATCH v3 03/11] KVM: selftests: Test Intel PMU architectural events on gp counters

[Jinrong Liang]

Sean Christopherson <seanjc@xxxxxxxxxx> 于2023年8月18日周五 06:54写道：
>
> On Mon, Aug 14, 2023, Jinrong Liang wrote:
> > Add test case for AMD Guest PerfMonV2. Also test Intel
> > MSR_CORE_PERF_GLOBAL_STATUS and MSR_CORE_PERF_GLOBAL_OVF_CTRL.
> >
> > Signed-off-by: Jinrong Liang <cloudliang@xxxxxxxxxxx>
> > ---
> >  .../kvm/x86_64/pmu_basic_functionality_test.c | 48 ++++++++++++++++++-
> >  1 file changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/x86_64/pmu_basic_functionality_test.c b/tools/testing/selftests/kvm/x86_64/pmu_basic_functionality_test.c
> > index cb2a7ad5c504..02bd1fe3900b 100644
> > --- a/tools/testing/selftests/kvm/x86_64/pmu_basic_functionality_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/pmu_basic_functionality_test.c
> > @@ -58,7 +58,9 @@ static uint64_t run_vcpu(struct kvm_vcpu *vcpu, uint64_t *ucall_arg)
> >
> >  static void guest_measure_loop(uint64_t event_code)
> >  {
> > +     uint64_t global_ovf_ctrl_msr, global_status_msr, global_ctrl_msr;
> >       uint8_t nr_gp_counters, pmu_version = 1;
> > +     uint8_t gp_counter_bit_width = 48;
> >       uint64_t event_sel_msr;
> >       uint32_t counter_msr;
> >       unsigned int i;
> > @@ -68,6 +70,12 @@ static void guest_measure_loop(uint64_t event_code)
> >               pmu_version = this_cpu_property(X86_PROPERTY_PMU_VERSION);
> >               event_sel_msr = MSR_P6_EVNTSEL0;
> >
> > +             if (pmu_version > 1) {
> > +                     global_ovf_ctrl_msr = MSR_CORE_PERF_GLOBAL_OVF_CTRL;
> > +                     global_status_msr = MSR_CORE_PERF_GLOBAL_STATUS;
> > +                     global_ctrl_msr = MSR_CORE_PERF_GLOBAL_CTRL;
> > +             }
> > +
> >               if (rdmsr(MSR_IA32_PERF_CAPABILITIES) & PMU_CAP_FW_WRITES)
> >                       counter_msr = MSR_IA32_PMC0;
> >               else
> > @@ -76,6 +84,17 @@ static void guest_measure_loop(uint64_t event_code)
> >               nr_gp_counters = AMD64_NR_COUNTERS;
> >               event_sel_msr = MSR_K7_EVNTSEL0;
> >               counter_msr = MSR_K7_PERFCTR0;
> > +
> > +             if (this_cpu_has(X86_FEATURE_AMD_PMU_EXT_CORE) &&
> > +                 this_cpu_has(X86_FEATURE_AMD_PERFMON_V2)) {
> > +                     nr_gp_counters = this_cpu_property(X86_PROPERTY_AMD_PMU_NR_CORE_COUNTERS);
> > +                     global_ovf_ctrl_msr = MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR;
> > +                     global_status_msr = MSR_AMD64_PERF_CNTR_GLOBAL_STATUS;
> > +                     global_ctrl_msr = MSR_AMD64_PERF_CNTR_GLOBAL_CTL;
> > +                     event_sel_msr = MSR_F15H_PERF_CTL0;
> > +                     counter_msr = MSR_F15H_PERF_CTR0;
> > +                     pmu_version = 2;
> > +             }
>
> Please use an if-else when the two things are completely exclusive, i.e. don't
> set "defaults" and then override them.
>
> >       }
> >
> >       for (i = 0; i < nr_gp_counters; i++) {
> > @@ -84,14 +103,39 @@ static void guest_measure_loop(uint64_t event_code)
> >                     ARCH_PERFMON_EVENTSEL_ENABLE | event_code);
> >
> >               if (pmu_version > 1) {
> > -                     wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, BIT_ULL(i));
> > +                     wrmsr(global_ctrl_msr, BIT_ULL(i));
> >                       __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
> > -                     wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, 0);
> > +                     wrmsr(global_ctrl_msr, 0);
> >                       GUEST_SYNC(_rdpmc(i));
> >               } else {
> >                       __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
> >                       GUEST_SYNC(_rdpmc(i));
> >               }
>
> This is extremely difficult to follow.  I think the same thing to do is to split
> this up into helpers, e.g. send pmu_version > 1 into one path, and pmu_version <= 1
> into an entirely different path.
>
> E.g. something like this?

I agree with all the proposed code changes you have provided. Your
comments have been incredibly helpful in making the necessary
improvements to the code. I will diligently follow your suggestions
and modify the code accordingly.

>
> static void guest_measure_loop(uint64_t event_code)
> {
>         uint64_t global_ovf_ctrl_msr, global_status_msr, global_ctrl_msr;
>         uint8_t nr_gp_counters, pmu_version;
>         uint8_t gp_counter_bit_width;
>         uint64_t event_sel_msr;
>         uint32_t counter_msr;
>         unsigned int i;
>
>         if (host_cpu_is_intel)
>                 pmu_version = this_cpu_property(X86_PROPERTY_PMU_VERSION);
>         else if (this_cpu_has(X86_FEATURE_PERFCTR_CORE) &&
>                  this_cpu_has(X86_FEATURE_PERFMON_V2)) {
>                 pmu_version = 2;
>         } else {
>                 pmu_version = 1;
>         }
>
>         if (pmu_version <= 1) {
>                 guest_measure_pmu_legacy(...);
>                 return;
>         }
>
>         if (host_cpu_is_intel) {
>                 nr_gp_counters = this_cpu_property(X86_PROPERTY_PMU_NR_GP_COUNTERS);
>                 global_ovf_ctrl_msr = MSR_CORE_PERF_GLOBAL_OVF_CTRL;
>                 global_status_msr = MSR_CORE_PERF_GLOBAL_STATUS;
>                 global_ctrl_msr = MSR_CORE_PERF_GLOBAL_CTRL;
>                 gp_counter_bit_width = this_cpu_property(X86_PROPERTY_PMU_GP_COUNTERS_BIT_WIDTH);
>
>                 if (rdmsr(MSR_IA32_PERF_CAPABILITIES) & PMU_CAP_FW_WRITES)
>                         counter_msr = MSR_IA32_PMC0;
>                 else
>                         counter_msr = MSR_IA32_PERFCTR0;
>         } else {
>                 nr_gp_counters = this_cpu_property(X86_PROPERTY_AMD_PMU_NR_CORE_COUNTERS);
>                 global_ovf_ctrl_msr = MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR;
>                 global_status_msr = MSR_AMD64_PERF_CNTR_GLOBAL_STATUS;
>                 global_ctrl_msr = MSR_AMD64_PERF_CNTR_GLOBAL_CTL;
>                 event_sel_msr = MSR_F15H_PERF_CTL0;
>                 counter_msr = MSR_F15H_PERF_CTR0;
>                 gp_counter_bit_width = 48;
>         }
>
>         for (i = 0; i < nr_gp_counters; i++) {
>                 wrmsr(counter_msr + i, 0);
>                 wrmsr(event_sel_msr + i, ARCH_PERFMON_EVENTSEL_OS |
>                       ARCH_PERFMON_EVENTSEL_ENABLE | event_code);
>
>                 wrmsr(global_ctrl_msr, BIT_ULL(i));
>                 __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
>                 wrmsr(global_ctrl_msr, 0);
>                 counter = _rdpmc(i);
>                 GUEST_ASSERT_EQ(this_pmu_has(...), !!counter);
>
>                 if ( _rdpmc(i)) {
>                         wrmsr(global_ctrl_msr, 0);
>                         wrmsr(counter_msr + i, 0);
>                         __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
>                         GUEST_ASSERT(!_rdpmc(i));
>
>                         wrmsr(global_ctrl_msr, BIT_ULL(i));
>                         __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
>                         GUEST_ASSERT(_rdpmc(i));
>
>                         wrmsr(global_ctrl_msr, 0);
>                         wrmsr(counter_msr + i, (1ULL << gp_counter_bit_width) - 2);
>                         wrmsr(global_ctrl_msr, BIT_ULL(i));
>                         __asm__ __volatile__("loop ." : "+c"((int){NUM_BRANCHES}));
>                         GUEST_ASSERT(rdmsr(global_status_msr) & BIT_ULL(i));
>
>                         wrmsr(global_ctrl_msr, 0);
>                         wrmsr(global_ovf_ctrl_msr, BIT_ULL(i));
>                         GUEST_ASSERT(!(rdmsr(global_status_msr) & BIT_ULL(i)));
>                 }
>         }
>

I truly appreciate your time and effort in reviewing the code and
providing such valuable feedback. Please feel free to share any
further suggestions or ideas in the future.