Re: [PATCH 1/2] KVM: x86/mmu: Don't rely on page-track mechanism to flush on memslot change

[Yan Zhao]

On Thu, Nov 10, 2022 at 05:08:30PM +0000, Sean Christopherson wrote:
> On Thu, Nov 10, 2022, Yan Zhao wrote:
> > On Thu, Nov 10, 2022 at 01:48:20AM +0000, Sean Christopherson wrote:
> > > Call kvm_mmu_zap_all_fast() directly when flushing a memslot instead of
> > > bounding through the page-track mechanism.  KVM (unfortunately) needs to
> > > zap and flush all page tables on memslot DELETE/MOVE irrespective of
> > > whether KVM is shadowing guest page tables.
> > > 
> > > This will allow changing KVM to register a page-track notifier on the
> > > first shadow root allocation, and will also allow deleting the misguided
> > > kvm_page_track_flush_slot() hook itself once KVM-GT also moves to a
> > > different method for reacting to memslot changes.
> > >
> > <...>
> > > @@ -6021,7 +6014,6 @@ int kvm_mmu_init_vm(struct kvm *kvm)
> > >  		return r;
> > >  
> > >  	node->track_write = kvm_mmu_pte_write;
> > > -	node->track_flush_slot = kvm_mmu_invalidate_zap_pages_in_memslot;
> > >  	kvm_page_track_register_notifier(kvm, node);
> > >  
> > >  	kvm->arch.split_page_header_cache.kmem_cache = mmu_page_header_cache;
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index e46e458c5b08..5da86fe3c113 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -12550,6 +12550,8 @@ void kvm_arch_flush_shadow_all(struct kvm *kvm)
> > >  void kvm_arch_flush_shadow_memslot(struct kvm *kvm,
> > >  				   struct kvm_memory_slot *slot)
> > >  {
> > > +	kvm_mmu_zap_all_fast(kvm);
> > > +
> > >  	kvm_page_track_flush_slot(kvm, slot);
> > Could we move this kvm_page_track_flush_slot() to right before
> > kvm_commit_memory_region()?
> 
> More or less.  The page-track stuff is x86-specific, just move it into x86's
> kvm_arch_commit_memory_region().
> 
> > As KVM now does not need track_flush_slot any more and kvmgt is the only user
> > to track_flush_slot, we can rename it to track_slot_changed to notify
> > the new/deleted/moved slot.
> > Do you think it's good?
> 
> Given that KVM/KVM-GT have never propery supported the MOVE case, and (IIUC) that
> there's no danger to the kernel if KVM-GT fails to write-protect a moved memslot,
> I would say just change the hook to ->remove_memslot().  I.e. even if the memslot
> is being moved, simply notify KVM-GT that the old memslot is being removed.
>
I think it should be good.
We can refine the support of MOVE later after it really happens.
Will do it by following your suggestions and based on this series :)

Thanks
Yan

> E.g.
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5a2821ca03b8..437e3832e377 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -12566,6 +12566,9 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>                                 const struct kvm_memory_slot *new,
>                                 enum kvm_mr_change change)
>  {
> +       if (change == KVM_MR_DELETE || change == KVM_MR_MOVE)
> +               kvm_page_track_remove_memslot(kvm, old);
> +
>         if (!kvm->arch.n_requested_mmu_pages &&
>             (change == KVM_MR_CREATE || change == KVM_MR_DELETE)) {
>                 unsigned long nr_mmu_pages;