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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C5578C4345F for ; Tue, 30 Apr 2024 15:05:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=pKCsm9Dby9ZYY6E2tqvlWwU2xaIz2NxlDyaDMed2QcY=; b=LGIVHj2EDk6tg6 f4r46Qdvk0NuUyt0BMGAoyots33Du563It6zbU0Iz+loQ4EneuDlPEEmoh22DoGkswvKieAK/rP9r 6kDrykLJ58ca5gQKdN8etQ4grkuL7jqwKZGuiQ3QKq9px++HxSsvUKvwblIPB/bYs9uiATrTe95k2 X7imXv+IhYS8+WPh+R+/tQXJzxMQeNn8Vzm9HfwFAA9ee6TwfwRtciKFd24WtWkycgEDh113sfgHk tOdepBXQcOsUTIYJZHmoXytj3gOTZzUhhOZhBzUlcdVoYEDtaAUFUmHu2U/ZjxytpOuVwtTiPzm15 lNc3YeN3REqdZzSUouPg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1s1p2X-00000006yUC-1CM0; Tue, 30 Apr 2024 15:05:05 +0000 Received: from sin.source.kernel.org ([2604:1380:40e1:4800::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1s1p2U-00000006yTC-2JtK for linux-arm-kernel@lists.infradead.org; Tue, 30 Apr 2024 15:05:04 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 29575CE10C7; Tue, 30 Apr 2024 15:05:00 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 10E17C2BBFC; Tue, 30 Apr 2024 15:04:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1714489499; bh=IXTsCs1+m3zuge8TFFA6Qz3AAaG9om+XGkqrVy5RyR4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CvyMEXnqDqtCtE4JAotOogiNGJOT76uOaNEyO1een2JsRk2efFHrUMD3QzcxXFyd2 HNaX+ktCAMWHjQpEKBeKlYc9JG9MVMP8rYUcEv+0gklcJ6pu+kgafdKy8cOVF+PY+g +7idnIowqvJEvF1OymcSimiXU5QTwR3D5ICbRn9l6BgfYtgwDWIXMC39U570Gy63Yt kR7AgE9xFrV7HuY3wddsSlhFNMQcKOofGj9rKVCOH7GypLyPXGuWwWA0N9eAx3eQJC TJAtw/XmG0O2tarWeI+pmkVm41904lS1Bw6wwTmZE8Ouuqm75Qhqm+yldMnJE5onGe pdIHHBepjfASg== Date: Tue, 30 Apr 2024 16:04:53 +0100 From: Will Deacon To: Ryan Roberts Cc: Catalin Marinas , Joey Gouly , Ard Biesheuvel , Mark Rutland , Anshuman Khandual , David Hildenbrand , Peter Xu , Mike Rapoport , Shivansh Vij , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/3] arm64/mm: Refactor PMD_PRESENT_INVALID and PTE_PROT_NONE bits Message-ID: <20240430150453.GA14187@willie-the-truck> References: <20240429140208.238056-1-ryan.roberts@arm.com> <20240429140208.238056-2-ryan.roberts@arm.com> <20240430133037.GA13848@willie-the-truck> <96fc0d1a-0c5f-4ca3-ad99-a64346990536@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <96fc0d1a-0c5f-4ca3-ad99-a64346990536@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240430_080503_042511_1756B783 X-CRM114-Status: GOOD ( 45.59 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Apr 30, 2024 at 03:02:21PM +0100, Ryan Roberts wrote: > On 30/04/2024 14:30, Will Deacon wrote: > > On Mon, Apr 29, 2024 at 03:02:05PM +0100, Ryan Roberts wrote: > >> diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h > >> index dd9ee67d1d87..de62e6881154 100644 > >> --- a/arch/arm64/include/asm/pgtable-prot.h > >> +++ b/arch/arm64/include/asm/pgtable-prot.h > >> @@ -18,14 +18,7 @@ > >> #define PTE_DIRTY (_AT(pteval_t, 1) << 55) > >> #define PTE_SPECIAL (_AT(pteval_t, 1) << 56) > >> #define PTE_DEVMAP (_AT(pteval_t, 1) << 57) > >> -#define PTE_PROT_NONE (_AT(pteval_t, 1) << 58) /* only when !PTE_VALID */ > >> - > >> -/* > >> - * This bit indicates that the entry is present i.e. pmd_page() > >> - * still points to a valid huge page in memory even if the pmd > >> - * has been invalidated. > >> - */ > >> -#define PMD_PRESENT_INVALID (_AT(pteval_t, 1) << 59) /* only when !PMD_SECT_VALID */ > >> +#define PTE_INVALID (_AT(pteval_t, 1) << 59) /* only when !PTE_VALID */ > > > > So this now overlaps with AttrIndx[3] if FEAT_AIE is implemented. Although > > this shouldn't matter on the face of things because it's only used for > > invalid entries, we originally moved the PROT_NONE bit from 2 to 57 back > > in 3676f9ef5481 ("arm64: Move PTE_PROT_NONE higher up") because it was > > possible to change the memory type for PROT_NONE mappings via some > > drivers. > > I'm not sure I follow your argument. > > 1. We don't support FEAT_AIE (currently) so AttrIndx[3] is always going to be 0 > for valid ptes. Drivers are only calling our helpers (e.g. > pgprot_writecombine(), right?) and those only know how to set AttrIndx[2:0]. Sure, but we might want to use it in future and chucking that out for the sake of uffd doesn't seem like an obviously worthwhile trade-off to me. > 2. PMD_PRESENT_INVALID was already occupying bit 59. So wouldn't the same shape > of concern apply there too for PMDs that have been invalidated, where the driver > then comes along and changes the memory type? (Perhaps because > PMD_PRESENT_INVALID is only set while the PTL is held this can't happen). I was mainly thinking of the PROT_NONE case, to be honest with you. I struggle to envisage how a driver could sensibly mess with the memory type for anonymous mappings, let alone huge pages! But perhaps I just lack imagination :) > 3. I had this same vague concern about confusion due to overlapping bit 59, > which is why in the next patch, I'm moving it to the NS bit. > > Experience tells me that when I'm arguing confidently with someone who is much > more expert than me, then I'm using wrong... so what have I missed? :) > > > > > Moving the field to the NS bit (as you do later in the series) resolves > > this, but the architecture currently says that the NS bit is RES0. How > > can we guarantee that it won't be repurposed by hardware in future? > > Well it remains free for use in valid entries of course. I think that's what I'm actually questioning! RES0 doesn't mean that tomorrow's whizz-bang CPU extension isn't allowed to use it, but that's a guarantee that we need if we're going to use it for our own purposes. > So I guess you are asking how to guarantee we won't also need to be able > to modify it on the fly for PROT_NONE entries? I don't have a definite > answer, but I've been working on the assumption that the architecture > introducing a feature that is only needed in states where NS is not needed > is unlikely (so using that bit for the feature is also unlikely). And then > needing to manipulate that feature dyanically for PROT_NONE mappings is > even less likely. The architects are quite good at inventing unlikely features :) SVE blowing the sigcontext comes to mind. I think we should seek clarification that the NS bit won't be allocated in the future if we are going to use it for our own stuff. > If all else fails we could move it to nG (bit 11) to free up bit 5. But that > requires a bit more fiddling with the swap pte format. Oh, cunning, I hadn't thought of that. I think that's probably a better approach if the NS bit isn't guaranteed to be left alone by the architecture. > >> @@ -469,7 +477,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte) > >> */ > >> static inline int pte_protnone(pte_t pte) > >> { > >> - return (pte_val(pte) & (PTE_VALID | PTE_PROT_NONE)) == PTE_PROT_NONE; > >> + return pte_invalid(pte) && !pte_user(pte) && !pte_user_exec(pte); > >> } > > > > Why do we need to check pte_user_*() here? Isn't PROT_NONE the only case > > in which a pte will have PTE_INVALID set? > > I guess for *ptes* this is technically correct. But I was trying to make the > format generic and reusable for *pmds* too. (pmd_protnone() wraps > pte_protnone()). For pmds, PTE_INVALID also represents invalid-but-present PMDs > (i.e. pmds on which pmd_mkinvalid() has been called). > > The intention is that PTE_INVALID indicates "present but not valid in HW". And > (!pte_user(pte) && !pte_user_exec(pte)) indicates the PROT_NONE permission. Ok, but it does mean the compiler can't emit a nice TBNZ instruction for the pte macro. Can you either seperate out the pmd/pte versions of the macro or just add a comment along the lines of what you said above, please? Cheers, Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel