From: Like Xu <like.xu.linux@gmail.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
Sandipan Das <sandipan.das@amd.com>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [kvm-unit-tests PATCH 2/2] x86/pmu: Add AMD Guest PerfMonV2 testcases
Date: Wed, 19 Oct 2022 17:40:44 +0800 [thread overview]
Message-ID: <6b852e8d-39f1-4baf-74f5-f98dd9f4e371@gmail.com> (raw)
In-Reply-To: <Yz4AZZWE1/wWhXFP@google.com>
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.
prev parent reply other threads:[~2022-10-19 11:19 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-05 12:39 [PATCH 0/4] KVM: x86/svm/pmu: Add AMD Guest PerfMonV2 support Like Xu
2022-09-05 12:39 ` [PATCH 1/4] KVM: x86/svm/pmu: Limit the maximum number of supported GP counters Like Xu
2022-09-05 17:26 ` Jim Mattson
2022-09-06 12:38 ` Like Xu
2022-09-05 12:39 ` [PATCH 2/4] KVM: x86/pmu: Make part of the Intel v2 PMU MSRs handling x86 generic Like Xu
2022-09-05 12:39 ` [PATCH 3/4] KVM: x86/svm/pmu: Add AMD PerfMonV2 support Like Xu
2022-09-05 18:00 ` Jim Mattson
2022-09-06 12:45 ` Like Xu
2022-09-06 20:19 ` Jim Mattson
2022-09-07 3:50 ` Like Xu
2022-09-07 4:15 ` Jim Mattson
2022-09-05 12:39 ` [PATCH 4/4] KVM: x86/cpuid: Add AMD CPUID ExtPerfMonAndDbg leaf 0x80000022 Like Xu
2022-09-05 17:36 ` Jim Mattson
2022-09-06 12:53 ` Like Xu
2022-09-06 20:08 ` Jim Mattson
2022-09-07 3:59 ` Like Xu
2022-09-07 4:11 ` Jim Mattson
2022-09-07 5:52 ` Sandipan Das
2022-09-07 6:39 ` Like Xu
2022-09-08 6:00 ` Sandipan Das
2022-09-08 23:14 ` Jim Mattson
2022-09-05 12:39 ` [kvm-unit-tests PATCH 1/2] x86/pmu: Update rdpmc testcase to cover #GP and emulation path Like Xu
2022-10-05 21:36 ` Sean Christopherson
2022-10-19 8:50 ` Like Xu
2022-09-05 12:39 ` [kvm-unit-tests PATCH 2/2] x86/pmu: Add AMD Guest PerfMonV2 testcases Like Xu
2022-10-05 22:08 ` Sean Christopherson
2022-10-19 9:40 ` Like Xu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6b852e8d-39f1-4baf-74f5-f98dd9f4e371@gmail.com \
--to=like.xu.linux@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=sandipan.das@amd.com \
--cc=seanjc@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).