All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jinrong Liang <cloudliang@tencent.com>,
	Like Xu <likexu@tencent.com>
Subject: [PATCH v5 03/13] KVM: x86/pmu: Always treat Fixed counters as available when supported
Date: Mon, 23 Oct 2023 17:26:23 -0700	[thread overview]
Message-ID: <20231024002633.2540714-4-seanjc@google.com> (raw)
In-Reply-To: <20231024002633.2540714-1-seanjc@google.com>

Now that KVM hides fixed counters that can't be virtualized, treat fixed
counters as available when they are supported, i.e. don't silently ignore
an enabled fixed counter just because guest CPUID says the associated
general purpose architectural event is unavailable.

KVM originally treated fixed counters as always available, but that got
changed as part of a fix to avoid confusing REF_CPU_CYCLES, which does NOT
map to an architectural event, with the actual architectural event used
associated with bit 7, TOPDOWN_SLOTS.

The commit justified the change with:

    If the event is marked as unavailable in the Intel guest CPUID
    0AH.EBX leaf, we need to avoid any perf_event creation, whether
    it's a gp or fixed counter.

but that justification doesn't mesh with reality.  The Intel SDM uses
"architectural events" to refer to both general purpose events (the ones
with the reverse polarity mask in CPUID.0xA.EBX) and the events for fixed
counters, e.g. the SDM makes statements like:

  Each of the fixed-function PMC can count only one architectural
  performance event.

but the fact that fixed counter 2 (TSC reference cycles) doesn't have an
associated general purpose architectural makes trying to apply the mask
from CPUID.0xA.EBX impossible.  Furthermore, the SDM never explicitly
says that an architectural events that's marked unavailable in EBX affects
the fixed counters.

Note, at the time of the change, KVM didn't enforce hardware support, i.e.
didn't prevent userspace from enumerating support in guest CPUID.0xA.EBX
for architectural events that aren't supported in hardware.  I.e. silently
dropping the fixed counter didn't somehow protection against counting the
wrong event, it just enforced guest CPUID.

Arguably, userspace is creating a bogus vCPU model by advertising a fixed
counter but saying the associated general purpose architectural event is
unavailable.  But regardless of the validity of the vCPU model, letting
the guest enable a fixed counter and then not actually having it count
anything is completely nonsensical.  I.e. even if all of the above is
wrong and it's illegal for a fixed counter to exist when the architectural
event is unavailable, silently doing nothing is still the wrong behavior
and KVM should instead disallow enabling the fixed counter in the first
place.

Fixes: a21864486f7e ("KVM: x86/pmu: Fix available_event_types check for REF_CPU_CYCLES event")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/pmu_intel.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 3316fdea212a..1c0a17661781 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -138,11 +138,24 @@ static bool intel_hw_event_available(struct kvm_pmc *pmc)
 	u8 unit_mask = (pmc->eventsel & ARCH_PERFMON_EVENTSEL_UMASK) >> 8;
 	int i;
 
+	/*
+	 * Fixed counters are always available if KVM reaches this point.  If a
+	 * fixed counter is unsupported in hardware or guest CPUID, KVM doesn't
+	 * allow the counter's corresponding MSR to be written.  KVM does use
+	 * architectural events to program fixed counters, as the interface to
+	 * perf doesn't allow requesting a specific fixed counter, e.g. perf
+	 * may (sadly) back a guest fixed PMC with a general purposed counter.
+	 * But if _hardware_ doesn't support the associated event, KVM simply
+	 * doesn't enumerate support for the fixed counter.
+	 */
+	if (pmc_is_fixed(pmc))
+		return true;
+
 	BUILD_BUG_ON(ARRAY_SIZE(intel_arch_events) != NR_INTEL_ARCH_EVENTS);
 
 	/*
 	 * Disallow events reported as unavailable in guest CPUID.  Note, this
-	 * doesn't apply to pseudo-architectural events.
+	 * doesn't apply to pseudo-architectural events (see above).
 	 */
 	for (i = 0; i < NR_REAL_INTEL_ARCH_EVENTS; i++) {
 		if (intel_arch_events[i].eventsel != event_select ||
-- 
2.42.0.758.gaed0368e0e-goog


  parent reply	other threads:[~2023-10-24  0:26 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-24  0:26 [PATCH v5 00/13] KVM: x86/pmu: selftests: Fixes and new tests Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 01/13] KVM: x86/pmu: Don't allow exposing unsupported architectural events Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 02/13] KVM: x86/pmu: Don't enumerate support for fixed counters KVM can't virtualize Sean Christopherson
2023-10-24  0:26 ` Sean Christopherson [this message]
2023-10-24  0:26 ` [PATCH v5 04/13] KVM: selftests: Add vcpu_set_cpuid_property() to set properties Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 05/13] KVM: selftests: Drop the "name" param from KVM_X86_PMU_FEATURE() Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 06/13] KVM: selftests: Extend {kvm,this}_pmu_has() to support fixed counters Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 07/13] KVM: selftests: Add pmu.h and lib/pmu.c for common PMU assets Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 08/13] KVM: selftests: Test Intel PMU architectural events on gp counters Sean Christopherson
2023-10-24 19:49   ` Sean Christopherson
2023-10-24 21:00     ` Sean Christopherson
2023-10-25  3:17     ` JinrongLiang
2023-10-26 20:38   ` Mingwei Zhang
2023-10-26 20:54     ` Sean Christopherson
2023-10-26 22:10       ` Mingwei Zhang
2023-10-26 22:54         ` Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 09/13] KVM: selftests: Test Intel PMU architectural events on fixed counters Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 10/13] KVM: selftests: Test consistency of CPUID with num of gp counters Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 11/13] KVM: selftests: Test consistency of CPUID with num of fixed counters Sean Christopherson
2023-10-24 11:40   ` JinrongLiang
2023-10-24 14:22     ` Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 12/13] KVM: selftests: Add functional test for Intel's fixed PMU counters Sean Christopherson
2023-10-24  0:26 ` [PATCH v5 13/13] KVM: selftests: Extend PMU counters test to permute on vPMU version Sean Christopherson
2023-10-24 11:49   ` JinrongLiang
2023-10-24 14:23     ` 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=20231024002633.2540714-4-seanjc@google.com \
    --to=seanjc@google.com \
    --cc=cloudliang@tencent.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.