Re: [PATCH 1/2] KVM: arm64: Replace memslot_is_logging() with kvm_slot_dirty_track_enabled()

[Wei-Lin Chang]

Hi Fuad,

Sorry for the late reply, I was focusing on something else.

On Wed, Oct 14, 2026 at 02:18:29PM +0100, Fuad Tabba wrote:
> Hi Wei-Lin,
> 
> On Fri, 05 Jun 2026 16:32:47 +0100, Wei-Lin Chang <weilin.chang@xxxxxxx> wrote:
> > When checking whether a memslot has dirty logging enabled, the
> > KVM_MEM_LOG_DIRTY_PAGES flag is the source of truth. Previously we were
> > using memslot_is_logging() which only tests dirty bitmap and did not
> > consider dirty ring. This was not detected because
> > KVM_CAP_DIRTY_LOG_RING_WITH_BITMAP was introduced together with KVM
> > arm64 dirty ring, and users need to enable it to ensure dirty
> > information is not lost for the case of VGIC LPI/ITS table changes.
> 
> I ran into the same bug while testing the arm64 KVM_PRE_FAULT_MEMORY
> series, and then found that you'd already fixed it.
> 
> It might be worth mentioning the impact in the commit message: with
> the ring and no bitmap, a logged memslot still gets block mappings, so
> guest writes to most of a block are never logged and a migration can
> copy stale memory. That would also make the case for a Cc: stable.

That makes sense, thanks for the suggestion!

> 
> > Note that memslot_is_logging() also treats a memslot as not logging if
> > KVM_MEM_READONLY is set, hence a memslot with both dirty logging and
> > read only would be seen as not logging for memslot_is_logging(), but
> > logging for kvm_slot_dirty_track_enabled(). This allows a read only
> > mapping of size > PAGE_SIZE to be built when memslot_is_logging() is
> > used, leading to a better read performance compared to
> > kvm_slot_dirty_track_enabled(). However memslots that have both
> > KVM_MEM_LOG_DIRTY_PAGES and KVM_MEM_READONLY set do not really make
> > sense as dirty logging is essentially nop for a read only memslot, so
> > this shouldn't affect real workloads much.
> 
> FWIW, I'd go with Leo's suggestion of keeping the KVM_MEM_READONLY
> check, so those memslots keep their block mappings.

Okay, then I'll not remove memslot_is_logging(), just use
kvm_slot_dirty_track_enabled() within it. This makes it even easier to
rebase & backport.

> 
> [...]
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> [...]
> > @@ -2084,7 +2079,7 @@ static int user_mem_abort(const struct kvm_s2_fault_desc *s2fd)
> >          * and a write fault needs to collapse a block entry into a table.
> >          */
> >         memcache = get_mmu_memcache(s2fd->vcpu);
> > -       if (!perm_fault || (memslot_is_logging(s2fd->memslot) &&
> > +       if (!perm_fault || (kvm_slot_dirty_track_enabled(s2fd->memslot) &&
> >                             kvm_is_write_fault(s2fd->vcpu))) {
> 
> FYI, this no longer applies to kvmarm/fixes or kvmarm/next, where the
> condition now ends in memslot_is_logging() ||
> is_protected_kvm_enabled().

Ack.

> 
> I tested the series on v7.3-rc7 (with that conflict resolved) in QEMU,
> with dirty_log_test changed to back the memslot with THP. In ring-only
> mode it fails on the first iteration without the series and passes
> with it. I'll send the test change later.
> 
> Reviewed-by: Fuad Tabba <fuad.tabba@xxxxxxxxx>
> Tested-by: Fuad Tabba <fuad.tabba@xxxxxxxxx>

Thanks for the tags!
Wei-Lin Chang

> 
> Cheers,
> /fuad