Re: [PATCH v3 07/10] KVM: selftests: Add basic stress test for save+restore and #PF handling

[Yosry Ahmed]

On Fri, Jul 24, 2026 at 11:12 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Fri, Jul 24, 2026, Yosry Ahmed wrote:
> > On Fri, Jul 24, 2026 at 9:45 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > > --- /dev/null
> > > > +++ b/tools/testing/selftests/kvm/x86/stress_save_restore_pf_test.c
> > > > @@ -0,0 +1,182 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +#include <stdio.h>
> > > > +#include <stdlib.h>
> > > > +#include <string.h>
> > > > +#include <errno.h>
> > > > +#include <sys/types.h>
> > > > +#include <time.h>
> > > > +#include <unistd.h>
> > > > +
> > > > +#include "test_util.h"
> > > > +#include "kvm_util.h"
> > > > +#include "processor.h"
> > > > +
> > > > +#define NR_ITERATIONS                500
> > > > +
> > > > +#define GOTO_PREV_LINE               "\033[A\r"
> > > > +#define PRINT_ITER(s, x)                                     \
> > > > +do {                                                         \
> > > > +     if (x == 1)                                             \
> > > > +             printf(s "%d\n", x);                            \
> > > > +     else                                                    \
> > > > +             printf(GOTO_PREV_LINE s "%d\n", x);             \
> > >
> > > Oh c'mon.  I had to Google the escape sequence you're (heh, "you"), using to
> > > understand this.
> >
> > I named it GOTO_PREV_LINE specifically so that people don't have to
> > Google it. AI wanted to call it something like ESC_SEQ, this is a big
> > improvement :P
> >
> > > Use a little creativity, and then the only magic escape sequence
> > > is the much more familiar carriage return.
> > >
> > >
> > >                 pr_info("\rSave+restore iterations: %d", count);
> > >         }
> > >         pr_info("\n");
> >
> > I had something like this initially and I changed it, but I can't
> > remember exactly why. I *think* when there is a test failure the log
> > is messed up because we didn't print a line break after the last
> > "Save+restore.." message.
>
> Ah, if an assert fires before the loop terminates.  Hrm.  This feels like
> something that should be a generic utility, but using any helper like this would
> require extreme care (the caller needs to know that *nothing* got printed between
> iterations).
>
> What if we simply add a newline before printing an assertion?

I like that. I will add a separate patch to do this (which will help
other tests using carriage returns), then just use a carriage return
here. Although..

>
> diff --git tools/testing/selftests/kvm/lib/assert.c tools/testing/selftests/kvm/lib/assert.c
> index 1d72dcdfce3b..3e353ac39eeb 100644
> --- tools/testing/selftests/kvm/lib/assert.c
> +++ tools/testing/selftests/kvm/lib/assert.c
> @@ -74,7 +74,7 @@ test_assert(bool exp, const char *exp_str,
>         if (!(exp)) {
>                 va_start(ap, fmt);
>
> -               fprintf(stderr, "==== Test Assertion Failure ====\n"
> +               fprintf(stderr, "\n==== Test Assertion Failure ====\n"
>                         "  %s:%u: %s\n"
>                         "  pid=%d tid=%d errno=%d - %s\n",
>                         file, line, exp_str, getpid(), kvm_gettid(),
>
>
> IMO, that's desirable even when there's a preceding newline, e.g. I find this
> ever so slightly easier to parse:

..in this very specific case, you still won't get the extra newline
because of the carriage return :P

>
> $ ./x86/stress_save_restore_pf_test
> Random seed: 0x65f90564
> Save+restore iterations: 500
>
> ==== Test Assertion Failure ====
>   x86/stress_save_restore_pf_test.c:310: guest_faults == 0
>   pid=4077 tid=4077 errno=25 - Inappropriate ioctl for device
>      1  0x0000000000403de6: main at stress_save_restore_pf_test.c:310
>      2  0x00007f8ead829d8f: ?? ??:0
>      3  0x00007f8ead829e3f: ?? ??:0
>      4  0x0000000000403fd4: _start at ??:?
>   No guest page faults triggered
>
>
> versus:
>
> $ ./x86/stress_save_restore_pf_test
> Random seed: 0xbc3c1a3
> Save+restore iterations: 500
> ==== Test Assertion Failure ====
>   x86/stress_save_restore_pf_test.c:310: guest_faults == 0
>   pid=6077 tid=6077 errno=25 - Inappropriate ioctl for device
>      1  0x0000000000403de6: main at stress_save_restore_pf_test.c:310
>      2  0x00007f4750829d8f: ?? ??:0
>      3  0x00007f4750829e3f: ?? ??:0
>      4  0x0000000000403fd4: _start at ??:?
>   No guest page faults triggered
>
>
>
> > > Hmm, I think we should put this helper in tools/testing/selftests/kvm/lib/x86/processor.c,
> > > and then provide a "struct kvm_mmu *guest_mmu;" that is automatically synchronized
> > > to the guest during kvm_arch_vm_post_create().  I think vm->mmu is fully populated
> > > at that point?
> >
> > Hmm yes, It is initialized in virt_pgd_alloc(), which is called in
> > ____vm_alloc() on the first allocation, which I think will be in
> > kvm_vm_elf_load(). We can probably add an assertion to make sure that
> > holds true.
> >
> > However, I think we can't really provide a full kvm_mmu. We can
> > synchronize the PTE masks, which I assume is what you mean here, but
> > then provide a 'pgd' which isn't really usable because guest PTEs
> > created by virt_map() (and friends) are not automatically sync'd to
> > the guest. I thought about doing that, but it isn't straightforward
> > because we cannot use direct mappings (see
>
> Nit, s/direct/identity.  That's a fairly important distinction for this code.
>
> > https://lore.kernel.org/all/CAO9r8zNqYA6CACeGTVLcyj4bJ2XPAzEXL+y9tLuCU5Y-F=D-WA@xxxxxxxxxxxxxx/).
>
> Wait, what?  Me confused.  The PTEs themselves must be "sync'd" to the guest,
> otherwise installing a new mapping would be useless.
>
> IIUC, the problem is that the guest isn't guaranteed to have valid *mappings*
> to its own PTEs, which is not the same as the actual PTEs being stale.

Yes, sorry, bad wording.

> IMO, that's totally fine, we just need to document the caveats.  The guest can
> get at its PGD via MOV CR3 anyways.  I.e. all we'd be doing is loading the gun
> for the guest to make it easier to put a hole in its foot.

Right, which is why I think writing pgd=NULL (or pgd=0 because it's a
GPA) makes it easier (I think) to see the gun?

>
> > > Then this test should be able to use the PTE macros from x86/processor.h, and
> > > future tests don't need to reinvent the wheel.
> >
> > Maybe provide a kvm_mmu with pgd=NULL just a placeholder to use the PTE masks?
>
> pgd is a GPA.
>
> Oooh, and this test relies on the page tables being identity mapped.

They are actually *not* identity mapped, they couldn't (see above).
They are mapped at a magic VA (i.e. TEST_PGTABLE_GVA_OFFSET).

> In that
> case, it's probably best to keep guest_get_pte() in this test until we can
> broadly guarantee gPTEs are reachable.  But I still think it makes sense to
> sync the MMU to the guest.

We can make the PTEs GVA offset a parameter to guest_get_pte(), but I
think that's not a good API. At least we shouldn't proactively expose
it before we have other users. So yes, I think we should keep
guest_get_pte() in the test, and just expose the placeholder MMU for
the pte masks.