Message-ID: <CALMp9eS5hqD-F8k=4YOGFedOWjgc=rDvqP+98gOrn9ne68NNpA@mail.gmail.com>
Date: Wed, 7 May 2025 11:19:15 -0700
From: Jim Mattson <jmattson@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org, 
	Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v3 2/2] KVM: selftests: Test behavior of KVM_X86_DISABLE_EXITS_APERFMPERF

On Mon, Apr 28, 2025 at 6:26 PM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Fri, Mar 21, 2025, Jim Mattson wrote:
> > +#include <fcntl.h>
> > +#include <limits.h>
> > +#include <pthread.h>
> > +#include <sched.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <stdint.h>
> > +#include <unistd.h>
> > +#include <asm/msr-index.h>
> > +
> > +#include "kvm_util.h"
> > +#include "processor.h"
> > +#include "test_util.h"
> > +
> > +#define NUM_ITERATIONS 100
> > +
> > +static void pin_thread(int cpu)
> > +{
> > +     cpu_set_t cpuset;
> > +     int rc;
> > +
> > +     CPU_ZERO(&cpuset);
> > +     CPU_SET(cpu, &cpuset);
> > +
> > +     rc = pthread_setaffinity_np(pthread_self(), sizeof(cpuset), &cpuset);
> > +     TEST_ASSERT(rc == 0, "%s: Can't set thread affinity", __func__);
>
> Heh, you copy-pasted this from hardware_disable_test.c, didn't you?  :-)

Probably.

> Would it make sense to turn this into a generic API that takes care of the entire
> sched_getcpu() => pthread_setaffinity_np()?  E.g. kvm_pin_task_to_current_cpu().
> I suspect there are other (potential) tests that don't care about what CPU they
> run on, so long as the test is pinned.

Sure.

> > +}
> > +
> > +static int open_dev_msr(int cpu)
> > +{
> > +     char path[PATH_MAX];
> > +     int msr_fd;
> > +
> > +     snprintf(path, sizeof(path), "/dev/cpu/%d/msr", cpu);
> > +     msr_fd = open(path, O_RDONLY);
> > +     __TEST_REQUIRE(msr_fd >= 0, "Can't open %s for read", path);
>
> Please use open_path_or_exit().

TIL.

> Hmm, and I'm planning on posting a small series to add a variant that takes an
> ENOENT message, and spits out a (hopefully) helpful message for the EACCES case.
> It would be nice to have this one spit out something like "Is msk.ko loaded?",
> but I would say don't worry about trying to coordinate anything.  Worst case
> scenario we can add a help message when the dust settles.
>
> > +     return msr_fd;
> > +}
> > +
> > +static uint64_t read_dev_msr(int msr_fd, uint32_t msr)
> > +{
> > +     uint64_t data;
> > +     ssize_t rc;
> > +
> > +     rc = pread(msr_fd, &data, sizeof(data), msr);
> > +     TEST_ASSERT(rc == sizeof(data), "Read of MSR 0x%x failed", msr);
> > +
> > +     return data;
> > +}
> > +
> > +static void guest_code(void)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < NUM_ITERATIONS; i++) {
> > +             uint64_t aperf = rdmsr(MSR_IA32_APERF);
> > +             uint64_t mperf = rdmsr(MSR_IA32_MPERF);
> > +
> > +             GUEST_SYNC2(aperf, mperf);
>
> Does the test generate multiple RDMSR per MSR if you do:
>
>                 GUEST_SYNC2(rdmsr(MSR_IA32_APERF), rdmsr(MSR_IA32_MPERF));
>
> If the code generation comes out

I'll have to check.

>
> > +     }
> > +
> > +     GUEST_DONE();
> > +}
> > +
> > +static bool kvm_can_disable_aperfmperf_exits(struct kvm_vm *vm)
> > +{
> > +     int flags = vm_check_cap(vm, KVM_CAP_X86_DISABLE_EXITS);
> > +
> > +     return flags & KVM_X86_DISABLE_EXITS_APERFMPERF;
> > +}
>
> Please don't add one-off helpers like this, especially when they're the condition
> for TEST_REQUIRE().  I *want* the gory details if the test is skipped, so that I
> don't have to go look at the source code to figure out what's missing.
>
> And it's literally more code.

Okay.

> > +
> > +int main(int argc, char *argv[])
> > +{
> > +     uint64_t host_aperf_before, host_mperf_before;
> > +     int cpu = sched_getcpu();
> > +     struct kvm_vcpu *vcpu;
> > +     struct kvm_vm *vm;
> > +     int msr_fd;
> > +     int i;
> > +
> > +     pin_thread(cpu);
> > +
> > +     msr_fd = open_dev_msr(cpu);
> > +
> > +     /*
> > +      * This test requires a non-standard VM initialization, because
> > +      * KVM_ENABLE_CAP cannot be used on a VM file descriptor after
> > +      * a VCPU has been created.
>
> Hrm, we should really sort this out.  Every test that needs to enable a capability
> is having to copy+paste this pattern.  I don't love the idea of expanding
> __vm_create_with_one_vcpu(), but there's gotta be a solution that isn't horrible,
> and anything is better than endly copy paste.

This is all your fault, I believe. But, I'll see what I can do.

> > +      */
> > +     vm = vm_create(1);
> > +
> > +     TEST_REQUIRE(kvm_can_disable_aperfmperf_exits(vm));
>
>         TEST_REQUIRE(vm_check_cap(vm, KVM_CAP_X86_DISABLE_EXITS) &
>                      KVM_X86_DISABLE_EXITS_APERFMPERF);
> > +
> > +     vm_enable_cap(vm, KVM_CAP_X86_DISABLE_EXITS,
> > +                   KVM_X86_DISABLE_EXITS_APERFMPERF);
> > +
> > +     vcpu = vm_vcpu_add(vm, 0, guest_code);
> > +
> > +     host_aperf_before = read_dev_msr(msr_fd, MSR_IA32_APERF);
> > +     host_mperf_before = read_dev_msr(msr_fd, MSR_IA32_MPERF);
> > +
> > +     for (i = 0; i < NUM_ITERATIONS; i++) {
> > +             uint64_t host_aperf_after, host_mperf_after;
> > +             uint64_t guest_aperf, guest_mperf;
> > +             struct ucall uc;
> > +
> > +             vcpu_run(vcpu);
> > +             TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
> > +
> > +             switch (get_ucall(vcpu, &uc)) {
> > +             case UCALL_DONE:
> > +                     break;
> > +             case UCALL_ABORT:
> > +                     REPORT_GUEST_ASSERT(uc);
> > +             case UCALL_SYNC:
> > +                     guest_aperf = uc.args[0];
> > +                     guest_mperf = uc.args[1];
> > +
> > +                     host_aperf_after = read_dev_msr(msr_fd, MSR_IA32_APERF);
> > +                     host_mperf_after = read_dev_msr(msr_fd, MSR_IA32_MPERF);
> > +
> > +                     TEST_ASSERT(host_aperf_before < guest_aperf,
> > +                                 "APERF: host_before (%lu) >= guest (%lu)",
> > +                                 host_aperf_before, guest_aperf);
>
> Honest question, is decimal really better than hex for these?

They are just numbers, so any base should be fine. I guess it depends
on which base you're most comfortable with. I could add a command-line
parameter.

> > +                     TEST_ASSERT(guest_aperf < host_aperf_after,
> > +                                 "APERF: guest (%lu) >= host_after (%lu)",
> > +                                 guest_aperf, host_aperf_after);
> > +                     TEST_ASSERT(host_mperf_before < guest_mperf,
> > +                                 "MPERF: host_before (%lu) >= guest (%lu)",
> > +                                 host_mperf_before, guest_mperf);
> > +                     TEST_ASSERT(guest_mperf < host_mperf_after,
> > +                                 "MPERF: guest (%lu) >= host_after (%lu)",
> > +                                 guest_mperf, host_mperf_after);
> > +
> > +                     host_aperf_before = host_aperf_after;
> > +                     host_mperf_before = host_mperf_after;
> > +
> > +                     break;
> > +             }
> > +     }
> > +
> > +     TEST_ASSERT_EQ(i, NUM_ITERATIONS);
>
> Why?

I think this was leftover from a version where it was possible to
break out of the loop early. I'll get rid of it.

V4 this week, I hope.

Powered by blists - more mailing lists