Sean Christopherson <seanjc@xxxxxxxxxx> writes:
On Wed, Sep 18, 2024, Colton Lewis wrote:
Test events on core counters by iterating through every combination of
events in amd_pmu_zen_events with every core counter.
For each combination, calculate the appropriate register addresses for
the event selection/control register and the counter register. The
base addresses and layout schemes change depending on whether we have
the CoreExt feature.
To do the testing, reuse GUEST_TEST_EVENT to run a standard known
workload. Decouple it from guest_assert_event_count (now
guest_assert_intel_event_count) to generalize to AMD.
Then assert the most specific detail that can be reasonably known
about the counter result. Exact count is defined and known for some
events and for other events merely asserted to be nonzero.
Note on exact counts: AMD counts one more branch than Intel for the
same workload. Though I can't confirm a reason, the only thing it
could be is the boundary of the loop instruction being counted
differently. Presumably, when the counter reaches 0 and execution
continues to the next instruction, AMD counts this as a branch and
Intel doesn't
IIRC, VMRUN is counted as a branch instruction for the guest. Assuming
my memory
is correct, that means this test is going to be flaky as an asynchronous
exit,
e.g. due to a host IRQ, during the measurement loop will inflate the
count. I'm
not entirely sure what to do about that :-(
:-( but thanks for the explanation
+static void __guest_test_core_event(uint8_t event_idx, uint8_t
counter_idx)
+{
+ /* One fortunate area of actual compatibility! This register
/*
* This is the proper format for multi-line comments. We are not the
* crazy net/ folks.
*/
Will do. As with some other formatting comments, checkpatch didn't
complain.
+ * layout is the same for both AMD and Intel.
It's not, actually. There are differences in the layout, it just so
happens that
the differences don't throw a wrench in things.
The comments in tools/testing/selftests/kvm/include/x86_64/pmu.h document
this
fairly well, I don't see any reason to have a comment here.
Will delete the comment
+ */
+ uint64_t eventsel = ARCH_PERFMON_EVENTSEL_OS |
+ ARCH_PERFMON_EVENTSEL_ENABLE |
+ amd_pmu_zen_events[event_idx];
Align the indentation.
uint64_t eventsel = ARCH_PERFMON_EVENTSEL_OS |
ARCH_PERFMON_EVENTSEL_ENABLE |
amd_pmu_zen_events[event_idx];
Will do
+ bool core_ext = this_cpu_has(X86_FEATURE_PERF_CTR_EXT_CORE);
+ uint64_t esel_msr_base = core_ext ? MSR_F15H_PERF_CTL :
MSR_K7_EVNTSEL0;
+ uint64_t cnt_msr_base = core_ext ? MSR_F15H_PERF_CTR : MSR_K7_PERFCTR0;
+ uint64_t msr_step = core_ext ? 2 : 1;
+ uint64_t esel_msr = esel_msr_base + msr_step * counter_idx;
+ uint64_t cnt_msr = cnt_msr_base + msr_step * counter_idx;
This pattern of code is copy+pasted in three functions. Please add
macros and/or
helpers to consolidate things. These should also be uint32_t, not 64.
Will do
It's a bit evil, but one approach would be to add a macro to iterate over
all
PMU counters. Eating the VM-Exit for the CPUID to get
X86_FEATURE_PERF_CTR_EXT_CORE
each time is unfortunate, but I doubt/hope it's not problematic in
practice. If
the cost is meaningful, we could figure out a way to cache the info, e.g.
something
awful like this might work:
/* Note, this relies on guest state being recreated between each test. */
static int has_perfctr_core = -1;
if (has_perfctr_core == -1)
has_perfctr_core = this_cpu_has(X86_FEATURE_PERFCTR_CORE);
if (has_perfctr_core) {
static bool get_pmu_counter_msrs(int idx, uint32_t *eventsel, uint32_t
*pmc)
{
if (this_cpu_has(X86_FEATURE_PERFCTR_CORE)) {
*eventsel = MSR_F15H_PERF_CTL + idx * 2;
*pmc = MSR_F15H_PERF_CTR + idx * 2;
} else {
*eventsel = MSR_K7_EVNTSEL0 + idx;
*pmc = MSR_K7_PERFCTR0 + idx;
}
return true;
}
#define for_each_pmu_counter(_i, _nr_counters, _eventsel, _pmc) \
for (_i = 0; i < _nr_counters; _i++) \
if (get_pmu_counter_msrs(_i, &_eventsel, _pmc)) \
static void guest_test_core_events(void)
{
uint8_t nr_counters = guest_nr_core_counters();
uint32_t eventsel_msr, pmc_msr;
int i, j;
for (i = 0; i < NR_AMD_ZEN_EVENTS; i++) {
for_each_pmu_counter(j, nr_counters, eventsel_msr, pmc_msr) {
uint64_t eventsel = ARCH_PERFMON_EVENTSEL_OS |
ARCH_PERFMON_EVENTSEL_ENABLE |
amd_pmu_zen_events[event_idx];
GUEST_TEST_EVENT(pmc_msr, eventsel_msr, eventsel, "");
guest_assert_amd_event_count(i, j, pmc_msr);
if (!is_forced_emulation_enabled)
continue;
GUEST_TEST_EVENT(pmc_msr, eventsel_msr, eventsel, KVM_FEP);
guest_assert_amd_event_count(i, j, pmc_msr);
}
}
}
I'll experiment with this