On Wed, May 07, 2025, Jim Mattson wrote:
> On Mon, Apr 28, 2025 at 6:26 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > + /*
> > > + * 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.
Ha, that it is, both on the KVM and the selftests side.
Unless you already have something clever in hand, just keep what you have. I poked
at this a bit today, and came to the conclusion that trying to save two lives of
"manual" effort isn't worth the explosion in APIs and complexity. I was thinking
that the only additional input would be the capability to enable, but most usage
also needs to specify a payload, and this pattern is used in a few places where
a selftest does more than toggle a capability.
What I really want is the ability to provide a closure to all of the "create with
vCPUs" APIs, e.g.
vm = vm_create_with_one_vcpu(&vcpu, guest_code, magic() {
vm_enable_cap(vm, KVM_CAP_X86_DISABLE_EXITS,
KVM_X86_DISABLE_EXITS_APERFMPERF);
});
But even if we managed to make something work, I'm not sure it'd be worth the
plumbing.
One thing that would make me less annoyed would be to eliminate the @vcpu_id
param, e.g.
static inline struct kvm_vcpu *vm_vcpu_add(struct kvm_vm *vm, void *guest_code)
{
return __vm_vcpu_add(vm, vm->nr_vcpus++, guest_code);
}
so that at least this pattern doesn't have '0' hardcoded everywhere. But that's
an annoying cleanup due to __vm_vcpu_add() not being a strict superset of
vm_vcpu_add(), i.e. would require a lot of churn.
So for this series, just keep the copy+pasted pattern.
> > > + */
> > > + 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.
Nah, don't bother, pick whatever you like. I was genuinely curious if one format
or another made it easier to understand the output.
