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;
>
