Re: [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring

[Lorenzo Stoakes (ARM)]

On Tue, Nov 10, 2026 at 03:37:05PM +0000, Fuad Tabba wrote:
> Hi Lorenzo,
>
> On Tue, 10 Nov 2026 14:38:27 +0000, "Lorenzo Stoakes (ARM)"
> <ljs@xxxxxxxxxx> wrote:
> > On Tue, Nov 10, 2026 at 11:07:24AM +0000, Fuad Tabba wrote:
> > > Pre-fault more populated, dirty-logged pages than the vCPU's dirty ring
> > > holds, and check that each call stops before the ring overflows, that a
> > > retry returns ENOSPC without progress, and that the harvested entries
> > > are the pages in order. A read pre-fault can map a page read-only, or
> >
> > Are we asserting an invariant that prefaulting happens in address order
> > here?  Is that explicitly presumed in the prefault API, or just how it
> > happens to pan out?
> >
> > I wonder if we might decide for whatever reason in future to do things
> > differently there. Bit nitty I know, but wonder if it's asserting an
> > implementation detail?
>
> Yes, the API doesn't guarantee an order. I'll assert that every page
> is reported exactly once instead.

THanks!

>
> [...]
> > > diff --git a/tools/testing/selftests/kvm/pre_fault_memory_test.c b/tools/testing/selftests/kvm/pre_fault_memory_test.c
> [...]
> > > +static void test_pre_fault_dirty_ring(void)
> [...]
> > > +    /* Populate the pages, so that pre-faulting can map them writable. */
> >
> > Reviewed below, but I wonder how x86 could then read fault these?
>
> KVM_PRE_FAULT_MEMORY is defined as a read fault, but x86 maps the page
> writable when the host page is, and marks it dirty, unless it falls
> back to access tracking (below). Populating the pages makes sure they
> aren't backed by the zero page.

Ack.

>
> [...]
> > > +    while (range.size) {
> >
> > NIT: n could be scoped to this block instead.
>
> Will do.

Thanks!

>
> [...]
> > > +        for (n = 0; ; n++) {
> >
> > Could this loop forever if cur->flags always == KVM_DIRTY_GFN_F_DIRTY? Or
> > is that guaranteed not to be the case?
>
> It can't. Every harvested entry is set to KVM_DIRTY_GFN_F_RESET and
> stays that way until KVM_RESET_DIRTY_RINGS, which runs after the loop,
> so one pass stops after at most a full ring.

Ack OK.

>
> > > +            cur = &gfns[fetch % entries];
> > > +            if (smp_load_acquire(&cur->flags) != KVM_DIRTY_GFN_F_DIRTY)
> >
> > Is this and the below release paired with equivalent stuff in the dirty ring code?
>
> Yes, the acquire pairs with the smp_wmb() in kvm_dirty_ring_push()
> ahead of the flag store, and the release with the smp_load_acquire()
> in kvm_dirty_gfn_harvested(). dirty_log_test does the same.

I know it's test code, but general kernel practice is to add a comment saying
what memory barriers pair with so it's easier to track, could you add those for
these?

>
> [...]
> > > +        /* A read pre-fault can map a page read-only, or access-tracked on x86. */
> >
> > Hmm under what circumstances would that happen? Is there no way to ensure pages
> > are dirtied by prefaulting? I imagine not though!
>
> No, it's up to the architecture. arm64 maps a read fault read-only
> while the slot is logged with a bitmap, and x86 with EPT but no A/D
> bits maps it access-tracked. Neither pushes anything to the ring.

Right, OK, makes sense.

>
> > > +        if (!n && !harvested && !range.size) {
> >
> > Hmm, so !n && (harvested || range_size) will pass this, but then you assert
> > n immediately below, so it seems like n is an unconditional requirement.
> >
> > So I wonder whether this should just be !n in this conditional or maybe the
> > assert afterwards should be dropped?
>
> The skip is for when pre-faulting dirties nothing at all: the first
> call maps the whole range without pushing a single entry, so there's
> nothing to test. Any other empty harvest is a bug, which the assert
> reports, so I'd rather keep both as they are.

OK I see. Really the assert below is !n && (harvested || range.size), maybe
clearer to be explicit but it's also a bit noisy so not a big deal.

--
Cheers, Lorenzo