Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 22 May 2023 17:06:56 -0700
In-Reply-To: <875y8jappz.fsf@nvidia.com>
Mime-Version: 1.0
References: <20230522063725.284686-1-apopple@nvidia.com> <ZGu1vsbscdO48V6h@google.com>
 <875y8jappz.fsf@nvidia.com>
Message-ID: <ZGwDoONUFAHi7XKz@google.com>
Subject: Re: [PATCH] mmu_notifiers: Notify on pte permission upgrades
From:   Sean Christopherson <seanjc@google.com>
To:     Alistair Popple <apopple@nvidia.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, robin.murphy@arm.com,
        will@kernel.org, nicolinc@nvidia.com,
        linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org,
        kvmarm@lists.cs.columbia.edu, jgg@nvidia.com,
        John Hubbard <jhubbard@nvidia.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Tue, May 23, 2023, Alistair Popple wrote:
> 
> Sean Christopherson <seanjc@google.com> writes:
> >> diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
> >> index d6c06e140277..f14d68f119d8 100644
> >> --- a/include/linux/mmu_notifier.h
> >> +++ b/include/linux/mmu_notifier.h
> >> @@ -31,6 +31,11 @@ struct mmu_interval_notifier;
> >>   * pages in the range so to mirror those changes the user must inspect the CPU
> >>   * page table (from the end callback).
> >>   *
> >> + * @MMU_NOTIFY_PROTECTION_UPGRAGE: update is due to a change from read-only to
> >> + * read-write for pages in the range. This must not be used to upgrade
> >> + * permissions on secondary PTEs, rather it should only be used to invalidate
> >> + * caches such as secondary TLBs that may cache old read-only entries.
> >
> > This is a poor fit for invalidate_range_{start,end}().  All other uses bookend
> > the primary MMU update, i.e. call start() _before_ changing PTEs.  The comments
> > are somewhat stale as they talk only about "unmapped", but the contract between
> > the primary MMU and the secondary MMU is otherwise quite clear on when the primary
> > MMU will invoke start() and end().
> >
> > 	 * invalidate_range_start() is called when all pages in the
> > 	 * range are still mapped and have at least a refcount of one.
> > 	 *
> > 	 * invalidate_range_end() is called when all pages in the
> > 	 * range have been unmapped and the pages have been freed by
> > 	 * the VM.
> >
> > I'm also confused as to how this actually fixes ARM's SMMU.  Unless I'm looking
> > at the wrong SMMU implementation, the SMMU implemenents only invalidate_range(),
> > not the start()/end() variants.
> 
> mmu_invalidate_range_end() calls the invalidate_range() callback if
> the start()/end() variants aren't set.

Ahh, thanks.

> > 	static const struct mmu_notifier_ops arm_smmu_mmu_notifier_ops = {
> > 		.invalidate_range	= arm_smmu_mm_invalidate_range,
> > 		.release		= arm_smmu_mm_release,
> > 		.free_notifier		= arm_smmu_mmu_notifier_free,
> > 	};
> >
> > Again from include/linux/mmu_notifier.h, not implementing the start()/end() hooks
> > is perfectly valid.  And AFAICT, the existing invalidate_range() hook is pretty
> > much a perfect fit for what you want to achieve.
> 
> Right, I didn't take that approach because it doesn't allow an event
> type to be passed which would allow them to be filtered on platforms
> which don't require this.
> 
> I had also assumed the invalidate_range() callbacks were allowed to
> sleep, hence couldn't be called under PTL. That's certainly true of mmu
> interval notifier callbacks, but Catalin reminded me that calls such as
> ptep_clear_flush_notify() already call invalidate_range() callback under
> PTL so I guess we already assume drivers don't sleep in their
> invalidate_range() callbacks. I will update the comments to reflect
> that.
> 
> > 	 * If invalidate_range() is used to manage a non-CPU TLB with
> > 	 * shared page-tables, it not necessary to implement the
> > 	 * invalidate_range_start()/end() notifiers, as
> > 	 * invalidate_range() already catches the points in time when an
> > 	 * external TLB range needs to be flushed. For more in depth
> > 	 * discussion on this see Documentation/mm/mmu_notifier.rst
> >
> > Even worse, this change may silently regress performance for secondary MMUs that
> > haven't yet taken advantage of the event type, e.g. KVM will zap all of KVM's PTEs
> > in response to the upgrade, instead of waiting until the guest actually tries to
> > utilize the new protections.
> 
> Yeah, I like the idea of introducing a
> ptep_set_access_flags_notify(). That way this won't regress performance
> on platforms that don't need it. Note this isn't a new feature but
> rather a bugfix. It's unclear to me why KVM on ARM hasn't already run
> into this issue, but I'm no KVM expert. Thanks for the feedback.

KVM manages its own page tables and so does its own TLB invalidations as needed,
e.g. KVM can and does change KVM's stage-2 PTEs from read-only to read/write
irrespective of mmu_notifiers.  I assume the SMMU issue arises only because the
SMMU is reusing the host kernel's (stage-1?) page tables.