Re: [PATCH v4 16/29] KVM: x86: Reject memslot MOVE operations if KVMGT is attached

[Sean Christopherson <seanjc@google.com>]

On Thu, Aug 31, 2023, Like Xu wrote:
> On 31/8/2023 4:50 am, Sean Christopherson wrote:
> > On Wed, Aug 30, 2023, Like Xu wrote:
> > > On 2023/7/29 09:35, Sean Christopherson wrote:
> > > > Disallow moving memslots if the VM has external page-track users, i.e. if
> > > > KVMGT is being used to expose a virtual GPU to the guest, as KVMGT doesn't
> > > > correctly handle moving memory regions.
> > > > 
> > > > Note, this is potential ABI breakage!  E.g. userspace could move regions
> > > > that aren't shadowed by KVMGT without harming the guest.  However, the
> > > > only known user of KVMGT is QEMU, and QEMU doesn't move generic memory
> > > 
> > > This change breaks two kvm selftests:
> > > 
> > > - set_memory_region_test;
> > > - memslot_perf_test;
> > 
> > It shoudn't.  As of this patch, KVM doesn't register itself as a page-track user,
> > i.e. KVMGT is the only remaining caller to kvm_page_track_register_notifier().
> > Unless I messed up, the only way kvm_page_track_has_external_user() can return
> > true is if KVMGT is attached to the VM.  The selftests most definitely don't do
> > anything with KVMGT, so I don't see how they can fail.
> > 
> > Are you seeing actually failures?
> 
> $ set_memory_region_test

...

> At one point I wondered if some of the less common kconfig's were enabled,
> but the above two test failures could be easily fixed with the following diff:

Argh, none of the configs I actually ran selftests on selected
CONFIG_KVM_EXTERNAL_WRITE_TRACKING=y. 

> diff --git a/arch/x86/kvm/mmu/page_track.h b/arch/x86/kvm/mmu/page_track.h
> index 62f98c6c5af3..d4d72ed999b1 100644
> --- a/arch/x86/kvm/mmu/page_track.h
> +++ b/arch/x86/kvm/mmu/page_track.h
> @@ -32,7 +32,7 @@ void kvm_page_track_delete_slot(struct kvm *kvm, struct
> kvm_memory_slot *slot);
> 
>  static inline bool kvm_page_track_has_external_user(struct kvm *kvm)
>  {
> -	return hlist_empty(&kvm->arch.track_notifier_head.track_notifier_list);
> +	return !hlist_empty(&kvm->arch.track_notifier_head.track_notifier_list);
>  }
>  #else
>  static inline int kvm_page_track_init(struct kvm *kvm) { return 0; }
> 
> , so I guess it's pretty obvious what's going on here.

Yes.  I'll rerun tests today and get the above posted on your behalf (unless you
don't want me to do that).

Sorry for yet another screw up, and thanks for testing!