Re: [PATCH v3 10/10] KVM: selftests: Trigger L2->L1 exits stress save+restore and #PF test

[Yosry Ahmed]

On Fri, Jul 24, 2026 at 11:25 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Fri, Jul 24, 2026, Yosry Ahmed wrote:
> > On Fri, Jul 24, 2026 at 10:50 AM Sean Christopherson <seanjc@xxxxxxxxxx> 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.

I prefer to keep the TEST_REQUIRE() as I don't like the test coverage
silently changing based on the available caps tbh. Does it matter in
practice? IIUC it's only possible for KVM_CAP_EXCEPTION_PAYLOAD to not
exist if the test is run on an older kernel, right?

> > > > +
> > > >       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:

As I mentioned above, I prefer taking a dependency on
KVM_CAP_EXCEPTION_PAYLOAD rather than varying coverage. At least for
the nested case, I think I can move the TEST_REQUIRE() into the
'nested' case, but then it's probably better to leave it as an arg to
allow for meaningfully running the test in L0.

Actually, what if we just skip the nested case completely if
KVM_CAP_EXCEPTION_PAYLOAD isn't available?

>
>         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);

Either way yeah I think we should document why checking 'nested' is needed.

Or.. what if we just check state->nested.size inside
kvm_x86_state_is_guest_mode()? or explicitly clear state->nested in
vcpu_save_state() if nested_size == 0?

>
>
>         ...
>
>         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?

Sounds good.