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.
