Messages in this thread | | | Date | Wed, 19 Oct 2022 17:40:44 +0800 | Subject | Re: [kvm-unit-tests PATCH 2/2] x86/pmu: Add AMD Guest PerfMonV2 testcases | From | Like Xu <> |
| |
All applied, thanks.
On 6/10/2022 6:08 am, Sean Christopherson wrote: > Can you provide a single series for all of the KVM-Unit-Tests PMU work (separate > from the KVM patches)? Ya, it'll be big and is a blatant violation of "do one > thing", but trying to manually handle the dependencies on the review side is time > consuming.
Thanks for your time. PMU test cases will be combined, if no new ideas emerge.
> > One thought to help keep track of dependencies between KVM and KUT would be to > add dummy commits between each sub-series, with the dummy commit containing a lore > link to the relevant KVM patches/series. That would allow throwing everything into > one series without losing track of things. Hopefully.
Sure, adding a lore link to a cover letter is always helpful. It seems that the ageing KVM project has moved to a test-driven approach to development, and any new developer should be aware of this rule.
> > On Mon, Sep 05, 2022, Like Xu wrote: >> diff --git a/lib/x86/processor.h b/lib/x86/processor.h >> index 9c490d9..b9592c4 100644 >> --- a/lib/x86/processor.h >> +++ b/lib/x86/processor.h >> @@ -796,8 +796,12 @@ static inline void flush_tlb(void) >> >> static inline u8 pmu_version(void) >> { >> - if (!is_intel()) >> + if (!is_intel()) { >> + /* Performance Monitoring Version 2 Supported */ >> + if (cpuid(0x80000022).a & 0x1) > > Add an X86_FEATURE_*, that way this is self-documenting. > >> + return 2; >> return 0; >> + } >> >> return cpuid(10).a & 0xff; >> } >> @@ -824,6 +828,9 @@ static inline u8 pmu_nr_gp_counters(void) >> { >> if (is_intel()) { >> return (cpuid(10).a >> 8) & 0xff; >> + } else if (this_cpu_has_perf_global_ctrl()) { > > Eww. Took me too long to connect the dots to understand how this guarantees that > leaf 0x80000022 is available. With an X86_FEATURE_* this can simply be: > > } else if (this_cpu_has(X86_FEATURE_AMD_PMU_V2) { > > or whatever name is appropriate. > >> + /* Number of Core Performance Counters. */ >> + return cpuid(0x80000022).b & 0xf; >> } else if (!has_amd_perfctr_core()) { >> return AMD64_NUM_COUNTERS; >> } >> diff --git a/x86/pmu.c b/x86/pmu.c >> index 11607c0..6d5363b 100644 >> --- a/x86/pmu.c >> +++ b/x86/pmu.c >> @@ -72,6 +72,9 @@ struct pmu_event { >> #define PMU_CAP_FW_WRITES (1ULL << 13) >> static u32 gp_counter_base; >> static u32 gp_select_base; >> +static u32 global_status_msr; >> +static u32 global_ctl_msr; >> +static u32 global_status_clr_msr; > > What do you think about naming these like MSR #defines? E.g. > > MSR_PERF_GLOBAL_CTRL > MSR_PERF_GLOBAL_STATUS > MSR_PERF_GLOBAL_STATUS_CLR > > There's a little risk of causing confusing, but I think it would make the code > easier to read.
Lowercase variable names are applied.
> >> static unsigned int gp_events_size; >> static unsigned int nr_gp_counters; >> >> @@ -150,8 +153,7 @@ static void global_enable(pmu_counter_t *cnt) >> return; >> >> cnt->idx = event_to_global_idx(cnt); >> - wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, rdmsr(MSR_CORE_PERF_GLOBAL_CTRL) | >> - (1ull << cnt->idx)); >> + wrmsr(global_ctl_msr, rdmsr(global_ctl_msr) | (1ull << cnt->idx)); > > Opportunistically use BIT_ULL(). > >> } >> >> static void global_disable(pmu_counter_t *cnt) >> @@ -159,8 +161,7 @@ static void global_disable(pmu_counter_t *cnt) >> if (pmu_version() < 2) >> return; >> >> - wrmsr(MSR_CORE_PERF_GLOBAL_CTRL, rdmsr(MSR_CORE_PERF_GLOBAL_CTRL) & >> - ~(1ull << cnt->idx)); >> + wrmsr(global_ctl_msr, rdmsr(global_ctl_msr) & ~(1ull << cnt->idx)); > > BIT_ULL() > >> } >> >> static inline uint32_t get_gp_counter_msr(unsigned int i) >> @@ -326,6 +327,23 @@ static void check_counters_many(void) >> report(i == n, "all counters"); >> } >> >> +static bool is_the_count_reproducible(pmu_counter_t *cnt) >> +{ >> + unsigned int i; >> + uint64_t count; >> + >> + __measure(cnt, 0); >> + count = cnt->count; >> + >> + for (i = 0; i < 10; i++) { >> + __measure(cnt, 0); >> + if (count != cnt->count) >> + return false; >> + } >> + >> + return true; >> +} >> + >> static void check_counter_overflow(void) >> { >> uint64_t count; >> @@ -334,13 +352,14 @@ static void check_counter_overflow(void) >> .ctr = gp_counter_base, >> .config = EVNTSEL_OS | EVNTSEL_USR | (*gp_events)[1].unit_sel /* instructions */, >> }; >> + bool precise_event = is_the_count_reproducible(&cnt); >> + >> __measure(&cnt, 0); >> count = cnt.count; >> >> /* clear status before test */ >> if (pmu_version() > 1) { > > Please provide helper(s) to replace the myriad open coded "pmu_version() > ???" > checks. E.g. this one appears to be: > > if (this_cpu_has_perf_global_status_clr()) > > I obviously don't care about the verbosity, it's that people like me might not > know what the PMU version has to do with an MSR being accessible.
| |