linux-kernel - Re: [PATCH v3 2/2] KVM: selftests: Test behavior of KVM_X86_DISABLE_EXITS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aBu6XkrAelyMqrsB@google.com>
Date: Wed, 7 May 2025 12:54:06 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Jim Mattson <jmattson@...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 Wed, May 07, 2025, Jim Mattson wrote:
> On Mon, Apr 28, 2025 at 6:26 PM Sean Christopherson <seanjc@...gle.com> 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.