From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 444BEC77B75 for ; Tue, 23 May 2023 00:39:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234583AbjEWAj3 (ORCPT ); Mon, 22 May 2023 20:39:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39736 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235630AbjEWAXE (ORCPT ); Mon, 22 May 2023 20:23:04 -0400 Received: from mail-pg1-x54a.google.com (mail-pg1-x54a.google.com [IPv6:2607:f8b0:4864:20::54a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 45DA3FA for ; Mon, 22 May 2023 17:06:58 -0700 (PDT) Received: by mail-pg1-x54a.google.com with SMTP id 41be03b00d2f7-53485aa1972so2298968a12.2 for ; Mon, 22 May 2023 17:06:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1684800418; x=1687392418; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=nqpMDrC4KK3NQ6AuoTcuN/aH10d3Gs6pWZJ4L+6LYdI=; b=einRQC06NM+pLkPcWJKuFkA3JrqGZ4jSC+L44hYXq+E/rLb8tn1KKR5KGxdVgGH9fL z4wM40qWv+kR5vQgB2cbnU9DNxDeI2IC0tmcs88RbHAhC13vUSJeJwXY2E2Dld8hrPg3 MG3L4X+OVgZf3FszqPcq/K+aN6nVAet/JMgxgQXoHdcBP0my7P2nA29a9lYONLLhRYMZ s2S/qIxe2XDDMncs9mGy6cUgAaVEpju9PTe00dW7/KviwcI5VWtDKN+UuNKlHfU72jrL KU7Iq+8yaRoCE4WlHn45jJU/E5rxEradrcAdgs1h1bhJKao7Ozt805O7vAzm8fUMRQot nQfg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684800418; x=1687392418; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=nqpMDrC4KK3NQ6AuoTcuN/aH10d3Gs6pWZJ4L+6LYdI=; b=NkHX1/ny0CgDhNU9/6sa5XAdfgVmEChTfGqqejR1qyhCtZftgi4RxT5QxeXkAXiiEq GMQlRtAlDIMIIH+KkFQmWARirgV7JIKIPs4Wl/cvEvpPYCM+mpYMstx+MYvnY/vWITfh qYpzbc3/dNkc33UEaC3aRYp4dLBgHgrnaz85Aaapf3VT30Arwzmdmye0YKNj08q1Y7L2 W/uh7rwzvYfBOdUK1WwQtgJe/DkAFxY4rLjXq0MwFeIAJRBRjadYcE83Oieg34XO9XHA yiFaeWQ9OMg56IB/b4zllxjNd2+35IkuejH3oWn8n/HpdG5oiyWzwcuB1BxCH/U37/CT 2eIA== X-Gm-Message-State: AC+VfDxTJ140nns1MVPIj72Y12MfugOUypC9RCcsZcmSDapVeqiW2LSx FQIKBFqwGjCfchJsG8Ke9q8ezwSJD3U= X-Google-Smtp-Source: ACHHUZ6HjllyZzTlaaKR4BoUDyFce04HZ9UhEoY+TcxZpDQZdfHOuNAQJ4T7tsiqCTcoicnwg0ExZ7H4Ffw= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a63:5f91:0:b0:52c:b362:1894 with SMTP id t139-20020a635f91000000b0052cb3621894mr2851167pgb.3.1684800417778; Mon, 22 May 2023 17:06:57 -0700 (PDT) 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> <875y8jappz.fsf@nvidia.com> Message-ID: Subject: Re: [PATCH] mmu_notifiers: Notify on pte permission upgrades From: Sean Christopherson To: Alistair Popple Cc: Andrew Morton , 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 Content-Type: text/plain; charset="us-ascii" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 23, 2023, Alistair Popple wrote: > > Sean Christopherson 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.