* [PATCH 0/2] KVM: Bound KVM_PRE_FAULT_MEMORY by the vCPU's dirty ring @ 2026-11-10 11:07 Fuad Tabba 2026-11-10 11:07 ` [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full Fuad Tabba 2026-11-10 11:07 ` [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring Fuad Tabba 0 siblings, 2 replies; 14+ messages in thread From: Fuad Tabba @ 2026-11-10 11:07 UTC (permalink / raw) To: Paolo Bonzini, Sean Christopherson Cc: kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Lorenzo Stoakes, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon, Fuad Tabba Hi folks, KVM_PRE_FAULT_MEMORY never checks the vCPU's dirty ring, so one call over enough populated, dirty-logged pages overflows it. The first patch stops the loop at the soft limit, as KVM_RUN does. A stopped call returns its partial progress, or, if it stopped before any, -ENOSPC rather than the retryable -EAGAIN, since no retry can succeed until the ring is harvested. The second patch adds the case to pre_fault_memory_test. On x86 a read fault normally maps a host-writable page writable and logs it at once, so pre-faulting overflows the ring. Sean's auto-prefetch series [1] keeps that behaviour for a pre-fault, so this series bounds the loop rather than have x86 write-protect a pre-faulted page. On arm64, which gained the ioctl in 7.4, pre-faulting RAM logged with the ring alone overflows it too, until Wei-Lin's fix [2] is in mainline. Pre-faulting isn't the only ioctl that can push past the ring's reserve. On x86, KVM_SET_MSRS does too when it writes the wall clock or the Hyper-V hypercall page: up to a few hundred entries in a single call, with no bound across calls. KVM_SET_MSRS can't take the same stop, since a short count from it means the MSR failed and QEMU asserts on it. What those paths should do when the ring fills is an open question for the dirty ring API, which I don't think belongs in this series. Pre-faulting a large logged range is a supported use of the ioctl, so the loop should handle a full ring rather than lose entries. Tested on x86 and arm64 under QEMU: without the first patch, the new case hits the WARN on both. Based on Linux 7.4-rc2 (7475ea8b58616). Cheers, /fuad [1] https://lore.kernel.org/all/20261028202607.1844883-1-seanjc@google.com/ [2] https://lore.kernel.org/all/20261029175238.3214788-2-weilin.chang@arm.com/ Fuad Tabba (2): KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring Documentation/virt/kvm/api.rst | 9 +- include/linux/kvm_dirty_ring.h | 6 ++ .../selftests/kvm/pre_fault_memory_test.c | 85 +++++++++++++++++++ virt/kvm/dirty_ring.c | 2 +- virt/kvm/kvm_main.c | 6 ++ 5 files changed, 105 insertions(+), 3 deletions(-) base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb -- 2.47.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full 2026-11-10 11:07 [PATCH 0/2] KVM: Bound KVM_PRE_FAULT_MEMORY by the vCPU's dirty ring Fuad Tabba @ 2026-11-10 11:07 ` Fuad Tabba 2026-11-10 14:22 ` Lorenzo Stoakes (ARM) 2026-11-10 15:48 ` Sean Christopherson 2026-11-10 11:07 ` [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring Fuad Tabba 1 sibling, 2 replies; 14+ messages in thread From: Fuad Tabba @ 2026-11-10 11:07 UTC (permalink / raw) To: Paolo Bonzini, Sean Christopherson Cc: kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Lorenzo Stoakes, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon, Fuad Tabba, stable Pre-faulting can map a dirty-logged page writable, which pushes it to the vCPU's dirty ring. KVM_RUN exits with KVM_EXIT_DIRTY_RING_FULL before the ring overflows, but the pre-fault loop never checks it, so one call over more such pages than the ring has free trips the WARN in kvm_dirty_ring_push() and overwrites entries userspace hasn't harvested. On x86: WARNING: arch/x86/kvm/../../../virt/kvm/dirty_ring.c:225 at kvm_dirty_ring_push+0x7a/0xd0 Call Trace: make_spte+0x2cc/0x450 kvm_tdp_mmu_map+0x5e1/0x7f0 kvm_tdp_page_fault+0x12c/0x1b0 kvm_mmu_do_page_fault+0x181/0x250 kvm_arch_vcpu_pre_fault_memory+0x136/0x1d0 kvm_vcpu_pre_fault_memory+0xcf/0x180 kvm_vcpu_ioctl+0x455/0x770 Stop the loop once the ring is soft full, returning the pages already processed, or -ENOSPC when there are none, since a retry can't succeed until userspace harvests the ring. kvm_dirty_ring_check_request() fills in a KVM_RUN exit, so test the ring's state directly. The stub for !HAVE_KVM_DIRTY_RING is for s390, which has the ioctl but no ring. Fixes: bc1a5cd002116 ("KVM: Add KVM_PRE_FAULT_MEMORY vcpu ioctl to pre-populate guest memory") Cc: stable@vger.kernel.org Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev> --- Documentation/virt/kvm/api.rst | 9 +++++++-- include/linux/kvm_dirty_ring.h | 6 ++++++ virt/kvm/dirty_ring.c | 2 +- virt/kvm/kvm_main.c | 6 ++++++ 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index e95c9cda085d9..89eb6be9bc6e3 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -6544,6 +6544,7 @@ Errors: ENOENT The specified `gpa` is outside defined memslots. ENOEXEC The vCPU has not been initialised (arm64). EINTR An unmasked signal is pending and no page was processed. + ENOSPC The vCPU's dirty ring is full and no page was processed. EFAULT The parameter address was invalid. EHWPOISON A poisoned host page was encountered. EOPNOTSUPP Mapping memory for a GPA is unsupported by the @@ -6588,7 +6589,9 @@ case, the ioctl can be called in parallel. When the ioctl returns, the input values are updated to point to the remaining range. If `size` > 0 on return, the caller can just issue -the ioctl again with the same `struct kvm_map_memory` argument. +the ioctl again with the same `struct kvm_pre_fault_memory` argument. +If that fails with ENOSPC, the vCPU's dirty ring has to be harvested +and reset first. On x86, shadow page tables cannot support this ioctl because they are indexed by virtual address or nested guest physical address. @@ -8957,7 +8960,9 @@ Therefore, the ioctl must be called *before* reading the content of the dirty pages. The dirty ring can get full. When it happens, the KVM_RUN of the -vcpu will return with exit reason KVM_EXIT_DIRTY_RING_FULL. +vcpu will return with exit reason KVM_EXIT_DIRTY_RING_FULL, and +KVM_PRE_FAULT_MEMORY on the vcpu will process no more pages until the +ring has been harvested and reset. The dirty ring interface has a major difference comparing to the KVM_GET_DIRTY_LOG interface in that, when reading the dirty ring from diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h index eb10d87adf7d5..88b4f2a009274 100644 --- a/include/linux/kvm_dirty_ring.h +++ b/include/linux/kvm_dirty_ring.h @@ -60,6 +60,11 @@ static inline void kvm_dirty_ring_push(struct kvm_vcpu *vcpu, { } +static inline bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring) +{ + return false; +} + static inline struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring, u32 offset) { @@ -81,6 +86,7 @@ int kvm_dirty_ring_alloc(struct kvm *kvm, struct kvm_dirty_ring *ring, int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring, int *nr_entries_reset); void kvm_dirty_ring_push(struct kvm_vcpu *vcpu, u32 slot, u64 offset); +bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring); bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu); diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c index 572b854edf740..f73f92102cb7a 100644 --- a/virt/kvm/dirty_ring.c +++ b/virt/kvm/dirty_ring.c @@ -40,7 +40,7 @@ static u32 kvm_dirty_ring_used(struct kvm_dirty_ring *ring) return READ_ONCE(ring->dirty_index) - READ_ONCE(ring->reset_index); } -static bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring) +bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring) { return kvm_dirty_ring_used(ring) >= ring->soft_limit; } diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index cf4cb586f1407..da1b76ceb65e3 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4461,6 +4461,12 @@ static int kvm_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, break; } + /* Like KVM_RUN: only the iteration crossing the soft limit uses the reserve. */ + if (vcpu->kvm->dirty_ring_size && kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { + r = -ENOSPC; + break; + } + r = kvm_arch_vcpu_pre_fault_memory(vcpu, range); if (WARN_ON_ONCE(r == 0 || r == -EIO)) break; -- 2.47.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full 2026-11-10 11:07 ` [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full Fuad Tabba @ 2026-11-10 14:22 ` Lorenzo Stoakes (ARM) 2026-11-10 15:48 ` Sean Christopherson 1 sibling, 0 replies; 14+ messages in thread From: Lorenzo Stoakes (ARM) @ 2026-11-10 14:22 UTC (permalink / raw) To: Fuad Tabba Cc: Paolo Bonzini, Sean Christopherson, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon, Fuad Tabba, stable On Tue, Nov 10, 2026 at 11:07:23AM +0000, Fuad Tabba wrote: > Pre-faulting can map a dirty-logged page writable, which pushes it to > the vCPU's dirty ring. KVM_RUN exits with KVM_EXIT_DIRTY_RING_FULL > before the ring overflows, but the pre-fault loop never checks it, so > one call over more such pages than the ring has free trips the WARN in > kvm_dirty_ring_push() and overwrites entries userspace hasn't > harvested. On x86: > > WARNING: arch/x86/kvm/../../../virt/kvm/dirty_ring.c:225 at kvm_dirty_ring_push+0x7a/0xd0 > Call Trace: > make_spte+0x2cc/0x450 > kvm_tdp_mmu_map+0x5e1/0x7f0 > kvm_tdp_page_fault+0x12c/0x1b0 > kvm_mmu_do_page_fault+0x181/0x250 > kvm_arch_vcpu_pre_fault_memory+0x136/0x1d0 > kvm_vcpu_pre_fault_memory+0xcf/0x180 > kvm_vcpu_ioctl+0x455/0x770 If you have a local repro maybe worth sending a regression test for this?... and I type that, then check the 2/2 and see that you already did :) ok haha thanks! > > Stop the loop once the ring is soft full, returning the pages already > processed, or -ENOSPC when there are none, since a retry can't succeed > until userspace harvests the ring. kvm_dirty_ring_check_request() fills > in a KVM_RUN exit, so test the ring's state directly. The stub for > !HAVE_KVM_DIRTY_RING is for s390, which has the ioctl but no ring. > > Fixes: bc1a5cd002116 ("KVM: Add KVM_PRE_FAULT_MEMORY vcpu ioctl to pre-populate guest memory") > Cc: stable@vger.kernel.org > Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev> Yikes, nasty. Thanks for addressing this! LGTM so: Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org> > --- > Documentation/virt/kvm/api.rst | 9 +++++++-- > include/linux/kvm_dirty_ring.h | 6 ++++++ > virt/kvm/dirty_ring.c | 2 +- > virt/kvm/kvm_main.c | 6 ++++++ > 4 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index e95c9cda085d9..89eb6be9bc6e3 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -6544,6 +6544,7 @@ Errors: > ENOENT The specified `gpa` is outside defined memslots. > ENOEXEC The vCPU has not been initialised (arm64). > EINTR An unmasked signal is pending and no page was processed. > + ENOSPC The vCPU's dirty ring is full and no page was processed. > EFAULT The parameter address was invalid. > EHWPOISON A poisoned host page was encountered. > EOPNOTSUPP Mapping memory for a GPA is unsupported by the > @@ -6588,7 +6589,9 @@ case, the ioctl can be called in parallel. > > When the ioctl returns, the input values are updated to point to the > remaining range. If `size` > 0 on return, the caller can just issue > -the ioctl again with the same `struct kvm_map_memory` argument. > +the ioctl again with the same `struct kvm_pre_fault_memory` argument. > +If that fails with ENOSPC, the vCPU's dirty ring has to be harvested > +and reset first. > > On x86, shadow page tables cannot support this ioctl because they > are indexed by virtual address or nested guest physical address. > @@ -8957,7 +8960,9 @@ Therefore, the ioctl must be called *before* reading the content of > the dirty pages. > > The dirty ring can get full. When it happens, the KVM_RUN of the > -vcpu will return with exit reason KVM_EXIT_DIRTY_RING_FULL. > +vcpu will return with exit reason KVM_EXIT_DIRTY_RING_FULL, and > +KVM_PRE_FAULT_MEMORY on the vcpu will process no more pages until the > +ring has been harvested and reset. > > The dirty ring interface has a major difference comparing to the > KVM_GET_DIRTY_LOG interface in that, when reading the dirty ring from > diff --git a/include/linux/kvm_dirty_ring.h b/include/linux/kvm_dirty_ring.h > index eb10d87adf7d5..88b4f2a009274 100644 > --- a/include/linux/kvm_dirty_ring.h > +++ b/include/linux/kvm_dirty_ring.h > @@ -60,6 +60,11 @@ static inline void kvm_dirty_ring_push(struct kvm_vcpu *vcpu, > { > } > > +static inline bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring) > +{ > + return false; > +} > + > static inline struct page *kvm_dirty_ring_get_page(struct kvm_dirty_ring *ring, > u32 offset) > { > @@ -81,6 +86,7 @@ int kvm_dirty_ring_alloc(struct kvm *kvm, struct kvm_dirty_ring *ring, > int kvm_dirty_ring_reset(struct kvm *kvm, struct kvm_dirty_ring *ring, > int *nr_entries_reset); > void kvm_dirty_ring_push(struct kvm_vcpu *vcpu, u32 slot, u64 offset); > +bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring); > > bool kvm_dirty_ring_check_request(struct kvm_vcpu *vcpu); > > diff --git a/virt/kvm/dirty_ring.c b/virt/kvm/dirty_ring.c > index 572b854edf740..f73f92102cb7a 100644 > --- a/virt/kvm/dirty_ring.c > +++ b/virt/kvm/dirty_ring.c > @@ -40,7 +40,7 @@ static u32 kvm_dirty_ring_used(struct kvm_dirty_ring *ring) > return READ_ONCE(ring->dirty_index) - READ_ONCE(ring->reset_index); > } > > -static bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring) > +bool kvm_dirty_ring_soft_full(struct kvm_dirty_ring *ring) > { > return kvm_dirty_ring_used(ring) >= ring->soft_limit; > } > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index cf4cb586f1407..da1b76ceb65e3 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4461,6 +4461,12 @@ static int kvm_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, > break; > } > > + /* Like KVM_RUN: only the iteration crossing the soft limit uses the reserve. */ > + if (vcpu->kvm->dirty_ring_size && kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { > + r = -ENOSPC; > + break; > + } > + > r = kvm_arch_vcpu_pre_fault_memory(vcpu, range); > if (WARN_ON_ONCE(r == 0 || r == -EIO)) > break; > -- > 2.47.3 > -- Cheers, Lorenzo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full 2026-11-10 11:07 ` [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full Fuad Tabba 2026-11-10 14:22 ` Lorenzo Stoakes (ARM) @ 2026-11-10 15:48 ` Sean Christopherson 2026-11-10 17:18 ` Fuad Tabba 1 sibling, 1 reply; 14+ messages in thread From: Sean Christopherson @ 2026-11-10 15:48 UTC (permalink / raw) To: Fuad Tabba Cc: Paolo Bonzini, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Lorenzo Stoakes, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon, Fuad Tabba, stable On Tue, Nov 10, 2026, Fuad Tabba wrote: > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index cf4cb586f1407..da1b76ceb65e3 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -4461,6 +4461,12 @@ static int kvm_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, > break; > } > > + /* Like KVM_RUN: only the iteration crossing the soft limit uses the reserve. */ Please don't include AI comments verbatim. > + if (vcpu->kvm->dirty_ring_size && kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { Eh, I vote to just use kvm_dirty_ring_check_request(), I don't see any harm in clobbering vcpu->run->exit_reason. Side topic, kvm_dirty_ring_check_request() is rather confusing (clears and then re-sets the requests). AFAICT, it can be this instead: lockdep_assert_held(vcpu->mutex); /* * The vCPU isn't runnable when the dirty ring becomes soft full. Keep * the requests set until the dirty pages are harvested and the dirty * ring is reset by userspace. Note, checking if the ring is soft-full * can get a false positive, but not a false negative, because only the * vCPU can add entries to the ring, and its mutex is held. */ if (kvm_test_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu) && kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL; trace_kvm_dirty_ring_exit(vcpu); return true; } kvm_clear_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu); return false; > + r = -ENOSPC; > + break; > + } > + > r = kvm_arch_vcpu_pre_fault_memory(vcpu, range); > if (WARN_ON_ONCE(r == 0 || r == -EIO)) > break; > -- > 2.47.3 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full 2026-11-10 15:48 ` Sean Christopherson @ 2026-11-10 17:18 ` Fuad Tabba 2026-11-10 18:35 ` Sean Christopherson 0 siblings, 1 reply; 14+ messages in thread From: Fuad Tabba @ 2026-11-10 17:18 UTC (permalink / raw) To: Sean Christopherson Cc: Paolo Bonzini, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Lorenzo Stoakes, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon, stable Hi Sean, On Tue, 10 Nov 2026 15:48:37 +0000, Sean Christopherson <seanjc@google.com> wrote: > On Tue, Nov 10, 2026, Fuad Tabba wrote: > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c [...] > > @@ -4461,6 +4461,12 @@ static int kvm_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, > > break; > > } > > > > + /* Like KVM_RUN: only the iteration crossing the soft limit uses the reserve. */ > > Please don't include AI comments verbatim. The series is LLM-assisted, but the comment was actually me. > > + if (vcpu->kvm->dirty_ring_size && kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { > > Eh, I vote to just use kvm_dirty_ring_check_request(), I don't see any harm in > clobbering vcpu->run->exit_reason. Will do. > Side topic, kvm_dirty_ring_check_request() is rather confusing (clears and then > re-sets the requests). AFAICT, it can be this instead: > > lockdep_assert_held(vcpu->mutex); > > /* > * The vCPU isn't runnable when the dirty ring becomes soft full. Keep > * the requests set until the dirty pages are harvested and the dirty > * ring is reset by userspace. Note, checking if the ring is soft-full > * can get a false positive, but not a false negative, because only the > * vCPU can add entries to the ring, and its mutex is held. > */ > if (kvm_test_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu) && > kvm_dirty_ring_soft_full(&vcpu->dirty_ring)) { > vcpu->run->exit_reason = KVM_EXIT_DIRTY_RING_FULL; > trace_kvm_dirty_ring_exit(vcpu); > return true; > } > > kvm_clear_request(KVM_REQ_DIRTY_RING_SOFT_FULL, vcpu); > return false; I'll send it as its own patch. Cheers, /fuad ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full 2026-11-10 17:18 ` Fuad Tabba @ 2026-11-10 18:35 ` Sean Christopherson 0 siblings, 0 replies; 14+ messages in thread From: Sean Christopherson @ 2026-11-10 18:35 UTC (permalink / raw) To: Fuad Tabba Cc: Paolo Bonzini, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Lorenzo Stoakes, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon, stable On Tue, Nov 10, 2026, Fuad Tabba wrote: > Hi Sean, > > On Tue, 10 Nov 2026 15:48:37 +0000, Sean Christopherson > <seanjc@google.com> wrote: > > On Tue, Nov 10, 2026, Fuad Tabba wrote: > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > [...] > > > @@ -4461,6 +4461,12 @@ static int kvm_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, > > > break; > > > } > > > > > > + /* Like KVM_RUN: only the iteration crossing the soft limit uses the reserve. */ > > > > Please don't include AI comments verbatim. > > The series is LLM-assisted, but the comment was actually me. LOL, I think all of us are getting a bit corrupted. :-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring 2026-11-10 11:07 [PATCH 0/2] KVM: Bound KVM_PRE_FAULT_MEMORY by the vCPU's dirty ring Fuad Tabba 2026-11-10 11:07 ` [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full Fuad Tabba @ 2026-11-10 11:07 ` Fuad Tabba 2026-11-10 14:38 ` Lorenzo Stoakes (ARM) 1 sibling, 1 reply; 14+ messages in thread From: Fuad Tabba @ 2026-11-10 11:07 UTC (permalink / raw) To: Paolo Bonzini, Sean Christopherson Cc: kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Lorenzo Stoakes, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon, Fuad Tabba 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 access-tracked on x86 without EPT A/D bits, so skip the case if pre-faulting dirties nothing. Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev> --- .../selftests/kvm/pre_fault_memory_test.c | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/tools/testing/selftests/kvm/pre_fault_memory_test.c b/tools/testing/selftests/kvm/pre_fault_memory_test.c index 3082fe09b95fa..18fc1d69acbf0 100644 --- a/tools/testing/selftests/kvm/pre_fault_memory_test.c +++ b/tools/testing/selftests/kvm/pre_fault_memory_test.c @@ -5,6 +5,7 @@ * Author: * Isaku Yamahata <isaku.yamahata at gmail.com> */ +#include <asm/barrier.h> #include <linux/sizes.h> #include <test_util.h> @@ -17,6 +18,8 @@ /* Arbitrarily chosen values */ #define TEST_BASE_SIZE SZ_2M #define TEST_SLOT 10 +/* Large enough for the entries every architecture reserves. */ +#define TEST_RING_SIZE SZ_64K /* Storage of test info to share with guest code */ struct test_config { @@ -281,6 +284,87 @@ static void test_pre_fault_memory(unsigned long vm_type, enum vm_mem_backing_src for_each_guest_mode(__test_pre_fault_memory, &p); } +/* + * Pre-fault more dirty-logged pages than the ring holds: each call has to stop + * before the ring overflows, and no entry may be lost. + */ +static void test_pre_fault_dirty_ring(void) +{ + const u64 entries = TEST_RING_SIZE / sizeof(struct kvm_dirty_gfn); + const u64 npages = entries + entries / 4; + struct kvm_pre_fault_memory range, left; + struct kvm_dirty_gfn *gfns, *cur; + u64 harvested = 0, n, size; + struct kvm_vcpu *vcpu; + int ret, stops = 0; + struct kvm_vm *vm; + u32 fetch = 0; + gpa_t gpa; + + if (!kvm_has_cap(KVM_CAP_DIRTY_LOG_RING) && !kvm_has_cap(KVM_CAP_DIRTY_LOG_RING_ACQ_REL)) { + pr_info("Skipping the dirty ring test: no dirty ring\n"); + return; + } + + vm = vm_create(1); + vm_enable_dirty_ring(vm, TEST_RING_SIZE); + vcpu = vm_vcpu_add(vm, 0, NULL); + gfns = vcpu_map_dirty_ring(vcpu); + + size = npages * vm->page_size; + gpa = align_down(vm->max_gfn * vm->page_size - size, SZ_2M); + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, gpa, TEST_SLOT, npages, + KVM_MEM_LOG_DIRTY_PAGES); + + /* Populate the pages, so that pre-faulting can map them writable. */ + memset(addr_gpa2hva(vm, gpa), 1, size); + + range = (struct kvm_pre_fault_memory) { .gpa = gpa, .size = size }; + while (range.size) { + vcpu_ioctl(vcpu, KVM_PRE_FAULT_MEMORY, &range); + + if (range.size) { + left = range; + ret = __vcpu_ioctl(vcpu, KVM_PRE_FAULT_MEMORY, &range); + TEST_ASSERT_VM_VCPU_IOCTL(ret == -1 && errno == ENOSPC, + KVM_PRE_FAULT_MEMORY, ret, vm); + TEST_ASSERT(range.gpa == left.gpa && range.size == left.size, + "KVM_PRE_FAULT_MEMORY made progress on a full ring"); + stops++; + } + + for (n = 0; ; n++) { + cur = &gfns[fetch % entries]; + if (smp_load_acquire(&cur->flags) != KVM_DIRTY_GFN_F_DIRTY) + break; + + TEST_ASSERT(cur->slot == TEST_SLOT && cur->offset == harvested + n, + "Ring entry %u: slot %u offset 0x%llx, expected slot %u offset 0x%lx", + fetch, cur->slot, cur->offset, TEST_SLOT, harvested + n); + smp_store_release(&cur->flags, KVM_DIRTY_GFN_F_RESET); + fetch++; + } + + /* A read pre-fault can map a page read-only, or access-tracked on x86. */ + if (!n && !harvested && !range.size) { + pr_info("Skipping the dirty ring test: pre-faulting dirtied no pages\n"); + kvm_vm_free(vm); + return; + } + TEST_ASSERT(n, "KVM_PRE_FAULT_MEMORY pushed nothing to the ring"); + TEST_ASSERT(kvm_vm_reset_dirty_ring(vm) == n, + "KVM_RESET_DIRTY_RINGS didn't reset %lu entries", n); + harvested += n; + } + + TEST_ASSERT(harvested == npages, "%lu ring entries for %lu pages", harvested, npages); + TEST_ASSERT(stops, "KVM_PRE_FAULT_MEMORY never stopped for a %lu-entry ring", entries); + pr_info("Pre-faulted %lu pages through a %lu-entry ring, stopping %d times\n", + npages, entries, stops); + + kvm_vm_free(vm); +} + static void help(char *name) { puts(""); @@ -320,5 +404,6 @@ int main(int argc, char *argv[]) test_pre_fault_memory(KVM_X86_SW_PROTECTED_VM, backing, false); test_pre_fault_memory(KVM_X86_SW_PROTECTED_VM, backing, true); #endif + test_pre_fault_dirty_ring(); return 0; } -- 2.47.3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring 2026-11-10 11:07 ` [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring Fuad Tabba @ 2026-11-10 14:38 ` Lorenzo Stoakes (ARM) 2026-11-10 15:37 ` Fuad Tabba 0 siblings, 1 reply; 14+ messages in thread From: Lorenzo Stoakes (ARM) @ 2026-11-10 14:38 UTC (permalink / raw) To: Fuad Tabba Cc: Paolo Bonzini, Sean Christopherson, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon, Fuad Tabba 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? > access-tracked on x86 without EPT A/D bits, so skip the case if > pre-faulting dirties nothing. > > Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev> > --- > .../selftests/kvm/pre_fault_memory_test.c | 85 +++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/tools/testing/selftests/kvm/pre_fault_memory_test.c b/tools/testing/selftests/kvm/pre_fault_memory_test.c > index 3082fe09b95fa..18fc1d69acbf0 100644 > --- a/tools/testing/selftests/kvm/pre_fault_memory_test.c > +++ b/tools/testing/selftests/kvm/pre_fault_memory_test.c > @@ -5,6 +5,7 @@ > * Author: > * Isaku Yamahata <isaku.yamahata at gmail.com> > */ > +#include <asm/barrier.h> > #include <linux/sizes.h> > > #include <test_util.h> > @@ -17,6 +18,8 @@ > /* Arbitrarily chosen values */ > #define TEST_BASE_SIZE SZ_2M > #define TEST_SLOT 10 > +/* Large enough for the entries every architecture reserves. */ > +#define TEST_RING_SIZE SZ_64K > > /* Storage of test info to share with guest code */ > struct test_config { > @@ -281,6 +284,87 @@ static void test_pre_fault_memory(unsigned long vm_type, enum vm_mem_backing_src > for_each_guest_mode(__test_pre_fault_memory, &p); > } > > +/* > + * Pre-fault more dirty-logged pages than the ring holds: each call has to stop > + * before the ring overflows, and no entry may be lost. > + */ > +static void test_pre_fault_dirty_ring(void) > +{ > + const u64 entries = TEST_RING_SIZE / sizeof(struct kvm_dirty_gfn); > + const u64 npages = entries + entries / 4; > + struct kvm_pre_fault_memory range, left; > + struct kvm_dirty_gfn *gfns, *cur; > + u64 harvested = 0, n, size; > + struct kvm_vcpu *vcpu; > + int ret, stops = 0; > + struct kvm_vm *vm; > + u32 fetch = 0; > + gpa_t gpa; > + > + if (!kvm_has_cap(KVM_CAP_DIRTY_LOG_RING) && !kvm_has_cap(KVM_CAP_DIRTY_LOG_RING_ACQ_REL)) { > + pr_info("Skipping the dirty ring test: no dirty ring\n"); > + return; > + } > + > + vm = vm_create(1); > + vm_enable_dirty_ring(vm, TEST_RING_SIZE); > + vcpu = vm_vcpu_add(vm, 0, NULL); > + gfns = vcpu_map_dirty_ring(vcpu); > + > + size = npages * vm->page_size; > + gpa = align_down(vm->max_gfn * vm->page_size - size, SZ_2M); > + vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, gpa, TEST_SLOT, npages, > + KVM_MEM_LOG_DIRTY_PAGES); > + > + /* Populate the pages, so that pre-faulting can map them writable. */ Reviewed below, but I wonder how x86 could then read fault these? > + memset(addr_gpa2hva(vm, gpa), 1, size); > + > + range = (struct kvm_pre_fault_memory) { .gpa = gpa, .size = size }; > + while (range.size) { NIT: n could be scoped to this block instead. > + vcpu_ioctl(vcpu, KVM_PRE_FAULT_MEMORY, &range); > + > + if (range.size) { > + left = range; > + ret = __vcpu_ioctl(vcpu, KVM_PRE_FAULT_MEMORY, &range); > + TEST_ASSERT_VM_VCPU_IOCTL(ret == -1 && errno == ENOSPC, > + KVM_PRE_FAULT_MEMORY, ret, vm); > + TEST_ASSERT(range.gpa == left.gpa && range.size == left.size, > + "KVM_PRE_FAULT_MEMORY made progress on a full ring"); > + stops++; > + } > + > + 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? > + 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? > + break; > + > + TEST_ASSERT(cur->slot == TEST_SLOT && cur->offset == harvested + n, > + "Ring entry %u: slot %u offset 0x%llx, expected slot %u offset 0x%lx", > + fetch, cur->slot, cur->offset, TEST_SLOT, harvested + n); > + smp_store_release(&cur->flags, KVM_DIRTY_GFN_F_RESET); > + fetch++; > + } > + > + /* 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! > + 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? > + pr_info("Skipping the dirty ring test: pre-faulting dirtied no pages\n"); > + kvm_vm_free(vm); > + return; > + } > + TEST_ASSERT(n, "KVM_PRE_FAULT_MEMORY pushed nothing to the ring"); > + TEST_ASSERT(kvm_vm_reset_dirty_ring(vm) == n, > + "KVM_RESET_DIRTY_RINGS didn't reset %lu entries", n); > + harvested += n; > + } > + > + TEST_ASSERT(harvested == npages, "%lu ring entries for %lu pages", harvested, npages); > + TEST_ASSERT(stops, "KVM_PRE_FAULT_MEMORY never stopped for a %lu-entry ring", entries); > + pr_info("Pre-faulted %lu pages through a %lu-entry ring, stopping %d times\n", > + npages, entries, stops); > + > + kvm_vm_free(vm); > +} > + > static void help(char *name) > { > puts(""); > @@ -320,5 +404,6 @@ int main(int argc, char *argv[]) > test_pre_fault_memory(KVM_X86_SW_PROTECTED_VM, backing, false); > test_pre_fault_memory(KVM_X86_SW_PROTECTED_VM, backing, true); > #endif > + test_pre_fault_dirty_ring(); > return 0; > } > -- > 2.47.3 > -- Cheers, Lorenzo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring 2026-11-10 14:38 ` Lorenzo Stoakes (ARM) @ 2026-11-10 15:37 ` Fuad Tabba 2026-11-10 15:53 ` Sean Christopherson 2026-11-10 15:57 ` Lorenzo Stoakes (ARM) 0 siblings, 2 replies; 14+ messages in thread From: Fuad Tabba @ 2026-11-10 15:37 UTC (permalink / raw) To: Lorenzo Stoakes (ARM) Cc: Paolo Bonzini, Sean Christopherson, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon Hi Lorenzo, On Tue, 10 Nov 2026 14:38:27 +0000, "Lorenzo Stoakes (ARM)" <ljs@kernel.org> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring 2026-11-10 15:37 ` Fuad Tabba @ 2026-11-10 15:53 ` Sean Christopherson 2026-11-10 16:02 ` Lorenzo Stoakes (ARM) 2026-11-10 17:29 ` Fuad Tabba 2026-11-10 15:57 ` Lorenzo Stoakes (ARM) 1 sibling, 2 replies; 14+ messages in thread From: Sean Christopherson @ 2026-11-10 15:53 UTC (permalink / raw) To: Fuad Tabba Cc: Lorenzo Stoakes (ARM), Paolo Bonzini, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon On Tue, Nov 10, 2026, Fuad Tabba wrote: > Hi Lorenzo, > > On Tue, 10 Nov 2026 14:38:27 +0000, "Lorenzo Stoakes (ARM)" > <ljs@kernel.org> 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. Before we effectively solidify the behavior, I think we should have an explicit discussion about the desired behavior of KVM_PRE_FAULT_MEMORY when dirty logging is enabled. E.g. the only reason I kept the existing behavior in my auto-prefault series was because I couldn't think of a good reason to change it. But we never made a concious decision in the first place, i.e. we never considered what would happen if prefaulting collided with dirty logging (as evidenced by this bug). ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring 2026-11-10 15:53 ` Sean Christopherson @ 2026-11-10 16:02 ` Lorenzo Stoakes (ARM) 2026-11-10 17:29 ` Fuad Tabba 1 sibling, 0 replies; 14+ messages in thread From: Lorenzo Stoakes (ARM) @ 2026-11-10 16:02 UTC (permalink / raw) To: Sean Christopherson Cc: Fuad Tabba, Paolo Bonzini, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon On Tue, Nov 10, 2026 at 07:53:35AM -0800, Sean Christopherson wrote: > On Tue, Nov 10, 2026, Fuad Tabba wrote: > > Hi Lorenzo, > > > > On Tue, 10 Nov 2026 14:38:27 +0000, "Lorenzo Stoakes (ARM)" > > <ljs@kernel.org> 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. > > Before we effectively solidify the behavior, I think we should have an explicit > discussion about the desired behavior of KVM_PRE_FAULT_MEMORY when dirty logging > is enabled. E.g. the only reason I kept the existing behavior in my auto-prefault > series was because I couldn't think of a good reason to change it. But we never > made a concious decision in the first place, i.e. we never considered what would > happen if prefaulting collided with dirty logging (as evidenced by this bug). Yeah, I do worry about implicit assumptions (also commented re: ordering). I guess what makes sense, given the general approach for prefaulting = injecting a synthetic read fault, is for the expectation to be that it matches the behaviour of an ordinary read fault. Or do you feel that it makes sense for the pre-faulting to do something special in these cases and establish a sensible invariant? If the pre-faulting does indeed dirty the pages, you'd need the dirty logging to do its usual thing (not familiar with the code, so I'm guessing logging the page :) though right? -- Cheers, Lorenzo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring 2026-11-10 15:53 ` Sean Christopherson 2026-11-10 16:02 ` Lorenzo Stoakes (ARM) @ 2026-11-10 17:29 ` Fuad Tabba 1 sibling, 0 replies; 14+ messages in thread From: Fuad Tabba @ 2026-11-10 17:29 UTC (permalink / raw) To: Sean Christopherson Cc: Lorenzo Stoakes (ARM), Paolo Bonzini, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon On Tue, 10 Nov 2026 15:53:35 +0000, Sean Christopherson <seanjc@google.com> wrote: [...] > Before we effectively solidify the behavior, I think we should have an explicit > discussion about the desired behavior of KVM_PRE_FAULT_MEMORY when dirty logging > is enabled. E.g. the only reason I kept the existing behavior in my auto-prefault > series was because I couldn't think of a good reason to change it. But we never > made a concious decision in the first place, i.e. we never considered what would > happen if prefaulting collided with dirty logging (as evidenced by this bug). FWIW, I'd lean towards a pre-fault not dirtying the page. arm64 (with a dirty bitmap) and riscv already map a read fault on a logged page read-only. Tried it on x86 by not mapping a prefetch writable in a logged slot, so that the fast path logs the first write. That also changes async #PF completion, so I don't think it's a stable fix. The loop has to stop for as long as x86 logs, so should patch 1 stay as the stable fix? The test skips when pre-faulting dirties no pages, so it doesn't pin either behavior. Cheers, /fuad ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring 2026-11-10 15:37 ` Fuad Tabba 2026-11-10 15:53 ` Sean Christopherson @ 2026-11-10 15:57 ` Lorenzo Stoakes (ARM) 2026-11-10 17:31 ` Fuad Tabba 1 sibling, 1 reply; 14+ messages in thread From: Lorenzo Stoakes (ARM) @ 2026-11-10 15:57 UTC (permalink / raw) To: Fuad Tabba Cc: Paolo Bonzini, Sean Christopherson, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon 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@kernel.org> 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring 2026-11-10 15:57 ` Lorenzo Stoakes (ARM) @ 2026-11-10 17:31 ` Fuad Tabba 0 siblings, 0 replies; 14+ messages in thread From: Fuad Tabba @ 2026-11-10 17:31 UTC (permalink / raw) To: Lorenzo Stoakes (ARM) Cc: Paolo Bonzini, Sean Christopherson, kvm, linux-kselftest, linux-doc, linux-kernel, Jonathan Corbet, Shuah Khan, Isaku Yamahata, Rick Edgecombe, Marc Zyngier, Oliver Upton, kvmarm, Wei-Lin Chang, Will Deacon On Tue, 10 Nov 2026 15:57:37 +0000, "Lorenzo Stoakes (ARM)" <ljs@kernel.org> wrote: [...] > > 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? Sure. /fuad ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-11-10 18:35 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-11-10 11:07 [PATCH 0/2] KVM: Bound KVM_PRE_FAULT_MEMORY by the vCPU's dirty ring Fuad Tabba 2026-11-10 11:07 ` [PATCH 1/2] KVM: Stop KVM_PRE_FAULT_MEMORY when the vCPU's dirty ring is full Fuad Tabba 2026-11-10 14:22 ` Lorenzo Stoakes (ARM) 2026-11-10 15:48 ` Sean Christopherson 2026-11-10 17:18 ` Fuad Tabba 2026-11-10 18:35 ` Sean Christopherson 2026-11-10 11:07 ` [PATCH 2/2] KVM: selftests: Test KVM_PRE_FAULT_MEMORY against a full dirty ring Fuad Tabba 2026-11-10 14:38 ` Lorenzo Stoakes (ARM) 2026-11-10 15:37 ` Fuad Tabba 2026-11-10 15:53 ` Sean Christopherson 2026-11-10 16:02 ` Lorenzo Stoakes (ARM) 2026-11-10 17:29 ` Fuad Tabba 2026-11-10 15:57 ` Lorenzo Stoakes (ARM) 2026-11-10 17:31 ` Fuad Tabba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®