Re: [PATCH 3/4] KVM: x86/pmu: Reuse find_perf_hw_id() and drop find_fixed_event()

From: Like Xu
Date: Fri Nov 19 2021 - 02:17:14 EST


On 18/11/2021 11:00 pm, Paolo Bonzini wrote:
On 11/16/21 13:20, Like Xu wrote:
+static inline unsigned int intel_find_fixed_event(int idx)
+{
+    u32 event;
+    size_t size = ARRAY_SIZE(fixed_pmc_events);
+
+    if (idx >= size)
+        return PERF_COUNT_HW_MAX;
+
+    event = fixed_pmc_events[array_index_nospec(idx, size)];
+    return intel_arch_events[event].event_type;
+}
+
+
  static unsigned int intel_find_perf_hw_id(struct kvm_pmc *pmc)
  {
      struct kvm_pmu *pmu = pmc_to_pmu(pmc);
@@ -75,6 +88,9 @@ static unsigned int intel_find_perf_hw_id(struct kvm_pmc *pmc)
      u8 unit_mask = (pmc->eventsel & ARCH_PERFMON_EVENTSEL_UMASK) >> 8;
      int i;
+    if (pmc_is_fixed(pmc))
+        return intel_find_fixed_event(pmc->idx - INTEL_PMC_IDX_FIXED);

Is intel_find_fixed_event needed at all?  As you point out in the commit
message, eventsel/unit_mask are valid so you can do

@@ -88,13 +75,11 @@ static unsigned int intel_pmc_perf_hw_id(struct kvm_pmc *pmc)
     u8 unit_mask = (pmc->eventsel & ARCH_PERFMON_EVENTSEL_UMASK) >> 8;
     int i;

-    if (pmc_is_fixed(pmc))
-        return intel_find_fixed_event(pmc->idx - INTEL_PMC_IDX_FIXED);
-
     for (i = 0; i < ARRAY_SIZE(intel_arch_events); i++)
         if (intel_arch_events[i].eventsel == event_select
             && intel_arch_events[i].unit_mask == unit_mask
-            && (pmu->available_event_types & (1 << i)))
+            && (pmc_is_fixed(pmc) ||
+            pmu->available_event_types & (1 << i)))

It's a good move while the tricky thing I've found recently is:

the events masked in pmu->available_event_types are just *Intel CPUID* events
(they're not a subset or superset of the *kernel generic* hw events),
some of which can be programmed and enabled with generic or fixed counter.

According Intel SDM, when an Intel CPUID event (e.g. "instructions retirement")
is not masked in pmu->available_event_types (configured via Intel CPUID.0A.EBX),
the guest should not use generic or fixed counter to count/sample this event.

This issue is detailed in another patch set [1] and comments need to be collected.

It's proposed to get [V2] merged and continue to review the fixes from [1] seamlessly,
and then further unify all fixed/gp stuff including intel_find_fixed_event() as a follow up.

[1] https://lore.kernel.org/kvm/20211112095139.21775-1-likexu@xxxxxxxxxxx/
[V2] https://lore.kernel.org/kvm/20211119064856.77948-1-likexu@xxxxxxxxxxx/

             break;

     if (i == ARRAY_SIZE(intel_arch_events))

What do you think?  It's less efficient but makes fixed/gp more similar.

Can you please resubmit the series based on the review feedback?

Thanks,