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

[Sean Christopherson <seanjc@google.com>]

On Fri, Jul 24, 2026, Yosry Ahmed wrote:
> On Fri, Jul 24, 2026 at 9:45 AM Sean Christopherson <seanjc@google.com> 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?

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:

$ ./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@mail.gmail.com/).

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.

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.

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