On Wed, May 28, 2025 at 4:25 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Wed, May 28, 2025, James Houghton wrote:
> > On Wed, May 28, 2025 at 1:30 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > index c5d21bcfa3ed4..f1db3f7742b28 100644
> > --- a/arch/arm64/kvm/mmu.c
> > +++ b/arch/arm64/kvm/mmu.c
> > @@ -2127,15 +2131,23 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
> > const struct kvm_memory_slot *new,
> > enum kvm_mr_change change)
> > {
> > - bool log_dirty_pages = new && new->flags & KVM_MEM_LOG_DIRTY_PAGES;
> > + u32 old_flags = old ? old->flags : 0;
> > + u32 new_flags = new ? new->flags : 0;
> > +
> > + /*
> > + * If only changing flags, nothing to do if not toggling
> > + * dirty logging.
> > + */
> > + if (change == KVM_MR_FLAGS_ONLY &&
> > + !((old_flags ^ new_flags) & KVM_MEM_LOG_DIRTY_PAGES))
> > + return;
> >
> > /*
> > * At this point memslot has been committed and there is an
> > * allocated dirty_bitmap[], dirty pages will be tracked while the
> > * memory slot is write protected.
> > */
> > - if (log_dirty_pages) {
> > -
> > + if (new_flags & KVM_MEM_LOG_DIRTY_PAGES) {
> > if (change == KVM_MR_DELETE)
> > return;
> >
> >
> > So we need to bail out early if we are enabling KVM_MEM_USERFAULT but
> > KVM_MEM_LOG_DIRTY_PAGES is already enabled, otherwise we'll be
> > write-protecting a bunch of PTEs that we don't need or want to WP.
> >
> > When *disabling* KVM_MEM_USERFAULT, we definitely don't want to WP
> > things, as we aren't going to get the unmap afterwards anyway.
> >
> > So the check we started with handles this:
> > > > > > + u32 old_flags = old ? old->flags : 0;
> > > > > > + u32 new_flags = new ? new->flags : 0;
> > > > > > +
> > > > > > + /* Nothing to do if not toggling dirty logging. */
> > > > > > + if (!((old_flags ^ new_flags) & KVM_MEM_LOG_DIRTY_PAGES))
> > > > > > + return;
> >
> > So why also check for `change == KVM_MR_FLAGS_ONLY` as well? Everything I just
> > said doesn't really apply when the memslot is being created, moved, or
> > destroyed. Otherwise, consider the case where we never enable dirty logging:
> >
> > - Memslot deletion would be totally broken; we'll see that
> > KVM_MEM_LOG_DIRTY_PAGES is not getting toggled and then bail out, skipping
> > some freeing.
>
> No, because @new and thus new_flags will be 0. If dirty logging wasn't enabled,
> then there's nothing to be done.
>
> > - Memslot creation would be broken in a similar way; we'll skip a bunch of
> > setup work.
>
> No, because @old and thus old_flags will be 0. If dirty logging isn't being
> enabled, then there's nothing to be done.
>
> > - For memslot moving, the only case that we could possibly be leaving
> > KVM_MEM_LOG_DIRTY_PAGES set without the change being KVM_MR_FLAGS_ONLY,
> > I think we still need to do the split and WP stuff.
>
> No, because KVM invokes kvm_arch_flush_shadow_memslot() on the memslot and marks
> it invalid prior to installing the new, moved memslot. See kvm_invalidate_memslot().
>
> So I'm still not seeing what's buggy.
Sorry, I didn't see your reply, Sean. :(
You're right, I was confusing the KVM_MEM_USERFAULT and
KVM_MEM_LOG_DIRTY_PAGES. I'll undo the little change I said I was
going to make.
Thank you!
