Re: [PATCH v3 02/11] KVM: selftests: Add pmu.h for PMU events and common masks

[Sean Christopherson <seanjc@google.com>]

On Mon, Aug 14, 2023, Jinrong Liang wrote:
> +static const uint64_t intel_arch_events[] = {

Hmm, rather than have a bunch of static arrays, I think it makes sense to go ahead
and add lib/pmu.c.  Hmm, and this should probably be further namespaced, e.g.
intel_pmu_arch_events

> +	[INTEL_ARCH_CPU_CYCLES]			= ARCH_EVENT(0x3c, 0x0),
> +	[INTEL_ARCH_INSTRUCTIONS_RETIRED]	= ARCH_EVENT(0xc0, 0x0),
> +	[INTEL_ARCH_REFERENCE_CYCLES]		= ARCH_EVENT(0x3c, 0x1),
> +	[INTEL_ARCH_LLC_REFERENCES]		= ARCH_EVENT(0x2e, 0x4f),
> +	[INTEL_ARCH_LLC_MISSES]			= ARCH_EVENT(0x2e, 0x41),
> +	[INTEL_ARCH_BRANCHES_RETIRED]		= ARCH_EVENT(0xc4, 0x0),
> +	[INTEL_ARCH_BRANCHES_MISPREDICTED]	= ARCH_EVENT(0xc5, 0x0),
> +	[PSEUDO_ARCH_REFERENCE_CYCLES]		= ARCH_EVENT(0xa4, 0x1),
> +};
> +
> +/* mapping between fixed pmc index and intel_arch_events array */
> +static const int fixed_pmc_events[] = {

Please be consistent with names (even if KVM itself may be anything but).  KVM
gets away with sloppiness because the arrays are only visible to pmu_intel.c,
but that's not the case here.

intel_pmu_fixed_pmc_events?

> +	[0] = INTEL_ARCH_INSTRUCTIONS_RETIRED,
> +	[1] = INTEL_ARCH_CPU_CYCLES,
> +	[2] = PSEUDO_ARCH_REFERENCE_CYCLES,
> +};
> +
> +enum amd_pmu_k7_events {
> +	AMD_ZEN_CORE_CYCLES,
> +	AMD_ZEN_INSTRUCTIONS,
> +	AMD_ZEN_BRANCHES,
> +	AMD_ZEN_BRANCH_MISSES,
> +};
> +
> +static const uint64_t amd_arch_events[] = {

And then amd_pmu_arch_events.

> +	[AMD_ZEN_CORE_CYCLES]			= ARCH_EVENT(0x76, 0x00),
> +	[AMD_ZEN_INSTRUCTIONS]			= ARCH_EVENT(0xc0, 0x00),
> +	[AMD_ZEN_BRANCHES]			= ARCH_EVENT(0xc2, 0x00),
> +	[AMD_ZEN_BRANCH_MISSES]			= ARCH_EVENT(0xc3, 0x00),
> +};
> +
> +static inline bool arch_event_is_supported(struct kvm_vcpu *vcpu,
> +					   uint8_t arch_event)

Same namespace problem.  And I'd put the "is" earlier so that it's clearly a
question and not a command, e.g. "is this arch event supported?" versus "this
arch event is supported".

So pmu_is_arch_event_supported()?  Actually, no, you're reinventing the wheel.
You can query all of the Intel arch events from within the guest via this_pmu_has(),
e.g. see X86_PMU_FEATURE_BRANCH_INSNS_RETIRED.  You just need a helper (or array)
to get from an arbitrary index to its associated feature.

And now that GUEST_ASSERT_EQ() prints values, you can just do

			counter = _rdpmc(i);
			GUEST_ASSERT_EQ(this_pmu_has(...), !!counter);

in guest_measure_loop() instead of funneling the counter into ucall and back to
the host.

> +{
> +	struct kvm_cpuid_entry2 *entry;
> +
> +	entry = vcpu_get_cpuid_entry(vcpu, 0xa);
> +
> +	return !(entry->ebx & BIT_ULL(arch_event)) &&
> +		(kvm_cpuid_property(vcpu->cpuid,
> +		 X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH) > arch_event);
> +}
> +
> +static inline bool fixed_counter_is_supported(struct kvm_vcpu *vcpu,

More namespace problems.  I don't love pmu_is_fixed_counter_supported(), because
that glosses over this operating on the vCPU, e.g. not KVM and not guest CPUID
(from within the guest).

And with a bit of massaging to the "anti-feature" framework, this_pmu_has() and
kvm_pmu_has() can be extended (unless I'm missing something).

diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index 07b980b8bec2..21f0c45c2ac6 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -287,12 +287,12 @@ struct kvm_x86_cpu_property {
  * architectural event is supported.
  */
 struct kvm_x86_pmu_feature {
-       struct kvm_x86_cpu_feature anti_feature;
+       struct kvm_x86_cpu_feature pmu_feature;
 };
 #define        KVM_X86_PMU_FEATURE(name, __bit)                                        \
 ({                                                                             \
        struct kvm_x86_pmu_feature feature = {                                  \
-               .anti_feature = KVM_X86_CPU_FEATURE(0xa, 0, EBX, __bit),        \
+               .pmu_feature = KVM_X86_CPU_FEATURE(0xa, 0, EBX, __bit), \
        };                                                                      \
                                                                                \
        feature;                                                                \
@@ -690,10 +690,19 @@ static __always_inline bool this_cpu_has_p(struct kvm_x86_cpu_property property)
 
 static inline bool this_pmu_has(struct kvm_x86_pmu_feature feature)
 {
-       uint32_t nr_bits = this_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
+       uint32_t nr_bits;
 
-       return nr_bits > feature.anti_feature.bit &&
-              !this_cpu_has(feature.anti_feature);
+       if (feature.pmu_feature.reg == KVM_CPUID_EBX) {
+               nr_bits = this_cpu_property(X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH);
+               return nr_bits > feature.pmu_feature.bit &&
+                      !this_cpu_has(feature.pmu_feature);
+       } else if (feature.pmu_feature.reg == KVM_CPUID_ECX) {
+               nr_bits = this_cpu_property(X86_PROPERTY_PMU_NR_FIXED_COUNTERS);
+               return nr_bits > feature.pmu_feature.bit ||
+                      this_cpu_has(feature.pmu_feature);
+       } else {
+               TEST_FAIL(...);
+       }
 }
 
 static __always_inline uint64_t this_cpu_supported_xcr0(void)


That doesn't give you a direct path to replacing fixed_counter_is_supported(),
but the usage in intel_test_oob_fixed_ctr() is bizarre and looks wrong, e.g. if
it's not supported, the test does nothing.