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.
