From: Sean Christopherson <seanjc@google.com>
To: Jim Mattson <jmattson@google.com>
Cc: Dapeng Mi <dapeng1.mi@linux.intel.com>,
Paolo Bonzini <pbonzini@redhat.com>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
Kan Liang <kan.liang@linux.intel.com>,
Jinrong Liang <cloudliang@tencent.com>,
Aaron Lewis <aaronlewis@google.com>,
Like Xu <likexu@tencent.com>
Subject: Re: [PATCH v9 10/28] KVM: x86/pmu: Explicitly check for RDPMC of unsupported Intel PMC types
Date: Tue, 12 Dec 2023 18:25:37 -0800 [thread overview]
Message-ID: <ZXkWIY6WRcBiuLMd@google.com> (raw)
In-Reply-To: <CALMp9eSZdPyZChd1PwFy+PqFAM2Eg4zQS97LG1s2DOov5e_mUQ@mail.gmail.com>
On Mon, Dec 11, 2023, Jim Mattson wrote:
> On Mon, Dec 11, 2023 at 3:43 PM Sean Christopherson <seanjc@google.com> wrote:
> > > > > @@ -82,9 +85,13 @@ static struct kvm_pmc *intel_rdpmc_ecx_to_pmc(struct kvm_vcpu *vcpu,
> > > > > /*
> > > > > * Fixed PMCs are supported on all architectural PMUs. Note, KVM only
> > > > > * emulates fixed PMCs for PMU v2+, but the flag itself is still valid,
> > > > > - * i.e. let RDPMC fail due to accessing a non-existent counter.
> > > > > + * i.e. let RDPMC fail due to accessing a non-existent counter. Reject
> > > > > + * attempts to read all other types, which are unknown/unsupported.
> > > > > */
> > > > > - idx &= ~INTEL_RDPMC_FIXED;
> > > > > + if (idx & INTEL_RDPMC_TYPE_MASK & ~INTEL_RDPMC_FIXED)
> > >
> > > You know how I hate to be pedantic (ROFL), but the SDM only says:
> > >
> > > If the processor does support architectural performance monitoring
> > > (CPUID.0AH:EAX[7:0] ≠ 0), ECX[31:16] specifies type of PMC while
> > > ECX[15:0] specifies the index of the PMC to be read within that type.
> > >
> > > It does not say that the types are bitwise-exclusive.
> > >
> > > Yes, the types defined thus far are bitwise-exclusive, but who knows
> > > what tomorrow may bring?
> >
> > The goal isn't to make the types exclusive, the goal is to reject types that
> > aren't supported by KVM. The above accomplishes that, no? I don't see how KVM
> > could get a false negative or false positive, the above allows exactly FIXED and
> > "none" types. Or are you objecting to the comment?
>
> You're right. The code is fine. My brain is not.
>
> But what's wrong with something like:
>
> type = idx & INTEL_RDPMC_TYPE_MASK;
> if (type != INTEL_RDPMC_GP && type != INTEL_RDPMC_FIXED) ...
>
> This makes it more clear what kvm accepts and what it doesn't accept,
> regardless of the actual values of the macros.
Because when I read the SDM, my reading was heavily colored by KVM's existing
implementation. And the SDM using 4000H and 2000H for the non-zero types doesn't
help (those scream "flags" to me). But rereading things, the SDM clearly states
they are explicit, distinct types. I'll massage this to have KVM treat them as
such.
next prev parent reply other threads:[~2023-12-13 2:25 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-02 0:03 [PATCH v9 00/28] KVM: x86/pmu: selftests: Fixes and new tests Sean Christopherson
2023-12-02 0:03 ` [PATCH v9 01/28] KVM: x86/pmu: Always treat Fixed counters as available when supported Sean Christopherson
2023-12-02 0:03 ` [PATCH v9 02/28] KVM: x86/pmu: Allow programming events that match unsupported arch events Sean Christopherson
2023-12-02 0:03 ` [PATCH v9 03/28] KVM: x86/pmu: Remove KVM's enumeration of Intel's architectural encodings Sean Christopherson
2023-12-02 0:03 ` [PATCH v9 04/28] KVM: x86/pmu: Setup fixed counters' eventsel during PMU initialization Sean Christopherson
2023-12-02 0:03 ` [PATCH v9 05/28] KVM: x86/pmu: Get eventsel for fixed counters from perf Sean Christopherson
2023-12-02 0:03 ` [PATCH v9 06/28] KVM: x86/pmu: Don't ignore bits 31:30 for RDPMC index on AMD Sean Christopherson
2023-12-02 0:03 ` [PATCH v9 07/28] KVM: x86/pmu: Prioritize VMX interception over #GP on RDPMC due to bad index Sean Christopherson
2023-12-02 0:03 ` [PATCH v9 08/28] KVM: x86/pmu: Apply "fast" RDPMC only to Intel PMUs Sean Christopherson
2023-12-02 0:03 ` [PATCH v9 09/28] KVM: x86/pmu: Disallow "fast" RDPMC for architectural " Sean Christopherson
2023-12-11 6:03 ` Mi, Dapeng
2023-12-02 0:03 ` [PATCH v9 10/28] KVM: x86/pmu: Explicitly check for RDPMC of unsupported Intel PMC types Sean Christopherson
2023-12-11 6:26 ` Mi, Dapeng
2023-12-11 21:33 ` Jim Mattson
2023-12-11 23:43 ` Sean Christopherson
2023-12-12 2:26 ` Jim Mattson
2023-12-13 2:25 ` Sean Christopherson [this message]
2023-12-02 0:04 ` [PATCH v9 11/28] KVM: selftests: Add vcpu_set_cpuid_property() to set properties Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 12/28] KVM: selftests: Drop the "name" param from KVM_X86_PMU_FEATURE() Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 13/28] KVM: selftests: Extend {kvm,this}_pmu_has() to support fixed counters Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 14/28] KVM: selftests: Add pmu.h and lib/pmu.c for common PMU assets Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 15/28] KVM: selftests: Test Intel PMU architectural events on gp counters Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 16/28] KVM: selftests: Test Intel PMU architectural events on fixed counters Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 17/28] KVM: selftests: Test consistency of CPUID with num of gp counters Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 18/28] KVM: selftests: Test consistency of CPUID with num of fixed counters Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 19/28] KVM: selftests: Add functional test for Intel's fixed PMU counters Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 20/28] KVM: selftests: Expand PMU counters test to verify LLC events Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 21/28] KVM: selftests: Add a helper to query if the PMU module param is enabled Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 22/28] KVM: selftests: Add helpers to read integer module params Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 23/28] KVM: selftests: Query module param to detect FEP in MSR filtering test Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 24/28] KVM: selftests: Move KVM_FEP macro into common library header Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 25/28] KVM: selftests: Test PMC virtualization with forced emulation Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 26/28] KVM: selftests: Add a forced emulation variation of KVM_ASM_SAFE() Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 27/28] KVM: selftests: Add helpers for safe and safe+forced RDMSR, RDPMC, and XGETBV Sean Christopherson
2023-12-02 0:04 ` [PATCH v9 28/28] KVM: selftests: Extend PMU counters test to validate RDPMC after WRMSR Sean Christopherson
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=ZXkWIY6WRcBiuLMd@google.com \
--to=seanjc@google.com \
--cc=aaronlewis@google.com \
--cc=cloudliang@tencent.com \
--cc=dapeng1.mi@linux.intel.com \
--cc=jmattson@google.com \
--cc=kan.liang@linux.intel.com \
--cc=kvm@vger.kernel.org \
--cc=likexu@tencent.com \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.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