by Like Xu

[permalink] [raw]

Subject: Re: [kvm-unit-tests PATCH 2/2] x86/pmu: Add AMD Guest PerfMonV2 testcases

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.