Re: [PATCH] KVM: Use precise range-based flush in mmu_notifier hooks when possible

[Sean Christopherson <seanjc@google.com>]

On Fri, Aug 23, 2024, Will Deacon wrote:
> On Tue, Aug 20, 2024 at 10:06:00AM -0700, Sean Christopherson wrote:
> > On Tue, Aug 20, 2024, Will Deacon wrote:
> > > On Tue, Aug 20, 2024 at 09:07:22AM -0700, Sean Christopherson wrote:
> > > > On Tue, Aug 20, 2024, Will Deacon wrote:
> > > > > handler could do the invalidation as part of its page-table walk (for
> > > > > example, it could use information about the page-table structure such
> > > > > as the level of the leaves to optimise the invalidation further), but
> > > > > this does at least avoid zapping the whole VMID on CPUs with range
> > > > > support.
> > > > > 
> > > > > My only slight concern is that, should clear_flush_young() be extended
> > > > > to operate on more than a single page-at-a-time in future, this will
> > > > > silently end up invalidating the entire VMID for each memslot unless we
> > > > > teach kvm_arch_flush_remote_tlbs_range() to return !0 in that case.
> > > > 
> > > > I'm not sure I follow the "entire VMID for each memslot" concern.  Are you
> > > > worried about kvm_arch_flush_remote_tlbs_range() failing and triggering a VM-wide
> > > > flush?
> > > 
> > > The arm64 implementation of kvm_arch_flush_remote_tlbs_range()
> > > unconditionally returns 0, so we could end up over-invalidating pretty
> > > badly if that doesn't change. It should be straightforward to fix, but
> > > I just wanted to point it out because it would be easy to miss too!
> > 
> > Sorry, I'm still not following.  0==success, and gfn_range.{start,end} is scoped
> > precisely to the overlap between the memslot and hva range.  Regardless of the
> > number of pages that are passed into clear_flush_young(), KVM should naturally
> > flush only the exact range being aged.  The only hiccup would be if the hva range
> > straddles multiple memslots, but if userspace creates multiple memslots for a
> > single vma, then that's a userspace problem.
> 
> Fair enough, but it's not a lot of effort to fix this (untested diff
> below) and if the code were to change in future so that
> __kvm_handle_hva_range() was more commonly used to span multiple
> memslots we probably wouldn't otherwise notice the silent
> over-invalidation for a while.
> 
> Will
> 
> --->8
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 6981b1bc0946..1e34127f79b0 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -175,6 +175,9 @@ int kvm_arch_flush_remote_tlbs(struct kvm *kvm)
>  int kvm_arch_flush_remote_tlbs_range(struct kvm *kvm,
>                                       gfn_t gfn, u64 nr_pages)
>  {
> +       if (!system_supports_tlb_range())
> +               return -EOPNOTSUPP;

Oooh, now your comments make a lot more sense.  I didn't catch on that range-based
flushing wasn't universally supported.

Agreed, not doing the above would be asinine.

> +
>         kvm_tlb_flush_vmid_range(&kvm->arch.mmu,
>                                 gfn << PAGE_SHIFT, nr_pages << PAGE_SHIFT);
>         return 0;
>