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

[Fuad Tabba]

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.

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

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

Will do.

[...]
> > +        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.

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

[...]
> > +        /* 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.

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

Cheers,
/fuad