Kernel KVM virtualization development
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Yosry Ahmed <yosry@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 10/10] KVM: selftests: Trigger L2->L1 exits stress save+restore and #PF test
Date: Fri, 24 Jul 2026 11:25:04 -0700	[thread overview]
Message-ID: <amOuALTiqI9u4tFh@google.com> (raw)
In-Reply-To: <CAO9r8zOy3qyW11ByE4o+VWn92rZJN_kbF3XJTt7qf3TnqXXuWg@mail.gmail.com>

On Fri, Jul 24, 2026, Yosry Ahmed wrote:
> On Fri, Jul 24, 2026 at 10:50 AM Sean Christopherson <seanjc@google.com> wrote:
> > >  static bool parse_args_nested(int argc, char *argv[])
> > >  {
> > >       bool nested = false;
> > > @@ -192,10 +218,13 @@ int main(int argc, char *argv[])
> > >       gva_t gva;
> > >       u64 pte;
> > >
> > > +     TEST_REQUIRE(kvm_has_cap(KVM_CAP_EXCEPTION_PAYLOAD));
> >
> > But KVM_CAP_EXCEPTION_PAYLOAD _isn't_ required, it's an optional feature.  Actually,
> > this is ridiculous.  The test is injecting a #UD, it doesn't have a payload.
> > Bad AI, bad.
> 
> KVM_CAP_EXCEPTION_PAYLOAD is required to inject a *pending* exception,
> which is needed as KVM won't check for interception on already
> injected exceptions. I will add a comment.

Drop the TEST_REQUIRE(), and instead only inject the pending #UD if the CAP
is supported.

> > > +
> > >       nested = parse_args_nested(argc, argv);
> > >
> > >       vm = vm_create_with_one_vcpu(&vcpu, nested ? l1_guest_code : guest_access_memory);
> > >       vm_install_exception_handler(vm, PF_VECTOR, guest_pf_handler);
> > > +     vm_enable_cap(vm, KVM_CAP_EXCEPTION_PAYLOAD, -2ul);
> >
> > -2ul?
> 
> That was actually me, not the AI. Apparently the other selftests also
> write -2ul for some reason. I was too lazy to go change them and/or
> figure out why -2ul, it didn't make any sense to me. I chose the lazy
> option and just used the same thing here.
> 
> Someone named Sean Christopherson wrote the other two, maybe we can ask him? :P

Nah, that guy's an asshole, not worth your time.

> > >       if (nested) {
> > >               TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_SVM) || kvm_cpu_has(X86_FEATURE_VMX));
> > > @@ -270,8 +299,16 @@ int main(int argc, char *argv[])
> > >
> > >               state = vcpu_save_state(vcpu);
> > >
> > > +             /*
> > > +              * If the vCPU is in guest mode, inject a #UD to trigger an
> > > +              * L2->L1 VM-Exit every other iteration.
> > > +              */
> > > +             if (nested && vcpu_state_is_guest_mode(state) && count % 2 == 0)
> >
> > Checking "nested" here is unnecessary.
> 
> It is, actually. Sashiko pointed out [1] that
> vcpu_state_is_guest_mode() will read uninitialized memory otherwise.
> 
> [1]https://sashiko.dev/#/patchset/20260518202514.2037078-1-yosry%40kernel.org?part=8

Oof.  That's subtle.  At the risk of true evil, what if we did:

	int inject_ud;

	/*
	 * Big comment here, explaining both the CAP dependency and the need to
	 * check for nested before querying guest state.
	 */
	inject_ud = nested && kvm_has_cap(KVM_CAP_EXCEPTION_PAYLOAD);


	...

	if ((i & inject_ud) && kvm_x86_state_is_guest_mode(&state)
		kvm_x86_state_inject_ud(&state);

> > > +                     vcpu_state_inject_ud(state);
> >
> > Honestly, I'd rather open code this whole thing, because this doesn't actually
> > inject a #UD.  It _prepares_ state, but doesn't send that into KVM.  E.g.
> 
> It injects a #UD into the state. The function name and parameter
> should make it clear. I prefer the helper, but I won't die on this
> hill.

I'm ok with a helper, it's the "vcpu" part that I find misleading, because the
"vcpu" namespace in selftest typically means "send this command into a vCPU ioctl".

kvm_x86_state_xxx() appears to be the standard namespace, let's go with that?


  reply	other threads:[~2026-07-24 18:25 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-29 18:37 [PATCH v3 00/10] KVM: selftests: Stress save+restore and #PF (ft. nested) Yosry Ahmed
2026-06-29 18:37 ` [PATCH v3 01/10] KVM: selftests: Move STR() and XSTR() definitions to test_util.h Yosry Ahmed
2026-07-24 15:26   ` Sean Christopherson
2026-06-29 18:37 ` [PATCH v3 02/10] KVM: selftests: Fix RAX and RFLAGS VMCB offsets when running L2 Yosry Ahmed
2026-06-29 18:37 ` [PATCH v3 03/10] KVM: selftests: Use an array for guest_regs (and fix offsets) Yosry Ahmed
2026-07-24 15:33   ` Sean Christopherson
2026-07-24 16:18     ` Yosry Ahmed
2026-07-24 17:03       ` Sean Christopherson
2026-07-24 17:44         ` Yosry Ahmed
2026-06-29 18:37 ` [PATCH v3 04/10] KVM: selftests: Move GPR load/save definitions outside of nSVM code Yosry Ahmed
2026-06-29 18:37 ` [PATCH v3 05/10] KVM: selftests: Reuse GPR switching logic for nVMX Yosry Ahmed
2026-06-29 18:49   ` sashiko-bot
2026-06-29 20:26     ` Yosry Ahmed
2026-06-29 18:37 ` [PATCH v3 06/10] KVM: selftests: Drop HORRIFIC_L2_UCALL_CLOBBER_HACK Yosry Ahmed
2026-07-24 15:37   ` Sean Christopherson
2026-07-24 16:20     ` Yosry Ahmed
2026-06-29 18:37 ` [PATCH v3 07/10] KVM: selftests: Add basic stress test for save+restore and #PF handling Yosry Ahmed
2026-07-24 16:45   ` Sean Christopherson
2026-07-24 17:38     ` Yosry Ahmed
2026-07-24 18:12       ` Sean Christopherson
2026-07-24 18:25         ` Yosry Ahmed
2026-06-29 18:37 ` [PATCH v3 08/10] KVM: selftests: Trigger save+restore randomly in the #PF stress test Yosry Ahmed
2026-06-29 18:48   ` sashiko-bot
2026-06-29 20:29     ` Yosry Ahmed
2026-06-29 18:37 ` [PATCH v3 09/10] KVM: selftests: Support running stress save+restore and #PF test in L2 Yosry Ahmed
2026-07-24 17:37   ` Sean Christopherson
2026-07-24 17:40     ` Yosry Ahmed
2026-06-29 18:37 ` [PATCH v3 10/10] KVM: selftests: Trigger L2->L1 exits stress save+restore and #PF test Yosry Ahmed
2026-07-24 17:50   ` Sean Christopherson
2026-07-24 18:15     ` Yosry Ahmed
2026-07-24 18:25       ` Sean Christopherson [this message]
2026-07-24 18:35         ` Yosry Ahmed
2026-07-24 20:32           ` Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=amOuALTiqI9u4tFh@google.com \
    --to=seanjc@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=yosry@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox