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 78AF5EB64DC for ; Thu, 15 Jun 2023 09:27:28 +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=JiF46gx7eg/wO7w85rGqLJztSNPTF94mnV6+BufKzwA=; b=rk9Mk0g8AfWnL3 +J1SBDfUzYZUEb3Dmt0gsjsHiqiv6vC51NnF1dIJ+hP2e8NeZJ1ryiLY6w4LZckYolLo/c5FaBiaI e0ichiTAxXu/JltiXBgMP2NbMRIYAqB7fTdUNZOBzVOuNH9IwS6YlBQcON5bt+7MXnEILT/xWKDsV ACmnh0JyHLV2D14ird2hOFcF+cdfFL9lSA+aUHA65XTp2e9L0o5qg48szYBo5ybwEJqnQFHr2Ku3f hLowjsdRhQmrLYu+nObYNL/xtUogvAzeaAmna3R//IG3AjE7bDtGTgNf9EHK3KchLaC4xVajfLGjv 5v4og1m/br9i7Fg2y32g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q9jG2-00EIC7-1h; Thu, 15 Jun 2023 09:27:10 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q9jFz-00EIBW-2m for linux-arm-kernel@lists.infradead.org; Thu, 15 Jun 2023 09:27:09 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0F8181FB; Thu, 15 Jun 2023 02:27:46 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.27.237]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EFBB93F71E; Thu, 15 Jun 2023 02:27:00 -0700 (PDT) Date: Thu, 15 Jun 2023 10:26:55 +0100 From: Mark Rutland To: Jamie Iles , Catalin Marinas Cc: linux-arm-kernel@lists.infradead.org, Will Deacon Subject: Re: [PATCH] arm64/mm: remove now-superfluous ISBs from TTBR writes Message-ID: References: <20230613141959.92697-1-quic_jiles@quicinc.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230613141959.92697-1-quic_jiles@quicinc.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230615_022708_011730_BCE62C53 X-CRM114-Status: GOOD ( 29.98 ) 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, Jun 13, 2023 at 03:19:59PM +0100, Jamie Iles wrote: > At the time of authoring 7655abb95386 ("arm64: mm: Move ASID from TTBR0 > to TTBR1"), the Arm ARM did not specify any ordering guarantees for > direct writes to TTBR0_ELx and TTBR1_ELx and so an ISB was required > after each write to ensure TLBs would only be populated from the > expected (or reserved tables). > > In a recent update to the Arm ARM, the requirements have been relaxed to > reflect the implementation of current CPUs and required implementation > of future CPUs to read (RDYDPX in D8.2.3 Translation table base address > register): > > Direct writes to TTBR0_ELx and TTBR1_ELx occur in program order > relative to one another, without the need for explicit > synchronization. For any one translation, all indirect reads of > TTBR0_ELx and TTBR1_ELx that are made as part of the translation > observe only one point in that order of direct writes. > > Remove the superfluous ISBs to optimize uaccess helpers and context > switch. > > Cc: Catalin Marinas > Cc: Will Deacon > Cc: Mark Rutland > Signed-off-by: Jamie Iles This matches my understanding, and the changes look correct to me. I have a couple of minor comments below; with those handled: Reviewed-by: Mark Rutland Catalin, are you hapy to fix those up when applying? Thanks, Mark. > --- > arch/arm64/include/asm/asm-uaccess.h | 2 -- > arch/arm64/include/asm/mmu_context.h | 9 +++++++-- > arch/arm64/include/asm/uaccess.h | 2 -- > arch/arm64/mm/context.c | 1 - > 4 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h > index 75b211c98dea..5b6efe8abeeb 100644 > --- a/arch/arm64/include/asm/asm-uaccess.h > +++ b/arch/arm64/include/asm/asm-uaccess.h > @@ -18,7 +18,6 @@ > bic \tmp1, \tmp1, #TTBR_ASID_MASK > sub \tmp1, \tmp1, #RESERVED_SWAPPER_OFFSET // reserved_pg_dir > msr ttbr0_el1, \tmp1 // set reserved TTBR0_EL1 > - isb > add \tmp1, \tmp1, #RESERVED_SWAPPER_OFFSET > msr ttbr1_el1, \tmp1 // set reserved ASID > isb > @@ -31,7 +30,6 @@ > extr \tmp2, \tmp2, \tmp1, #48 > ror \tmp2, \tmp2, #16 > msr ttbr1_el1, \tmp2 // set the active ASID > - isb > msr ttbr0_el1, \tmp1 // set the non-PAN TTBR0_EL1 > isb > .endm > diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h > index 56911691bef0..a80285defe81 100644 > --- a/arch/arm64/include/asm/mmu_context.h > +++ b/arch/arm64/include/asm/mmu_context.h > @@ -39,11 +39,16 @@ static inline void contextidr_thread_switch(struct task_struct *next) > /* > * Set TTBR0 to reserved_pg_dir. No translations will be possible via TTBR0. > */ > -static inline void cpu_set_reserved_ttbr0(void) > +static inline void __cpu_set_reserved_ttbr0(void) > { > unsigned long ttbr = phys_to_ttbr(__pa_symbol(reserved_pg_dir)); > > write_sysreg(ttbr, ttbr0_el1); > +} Could we please call this cpu_set_reserved_ttbr0_nosync() ? I think that's a little clearer than the underscores. > + > +static inline void cpu_set_reserved_ttbr0(void) > +{ > + __cpu_set_reserved_ttbr0(); > isb(); > } > > @@ -52,7 +57,7 @@ void cpu_do_switch_mm(phys_addr_t pgd_phys, struct mm_struct *mm); > static inline void cpu_switch_mm(pgd_t *pgd, struct mm_struct *mm) > { > BUG_ON(pgd == swapper_pg_dir); > - cpu_set_reserved_ttbr0(); > + __cpu_set_reserved_ttbr0(); > cpu_do_switch_mm(virt_to_phys(pgd),mm); Could we please move the __cpu_set_reserved_ttbr0() into cpu_do_switch_mm(), just before the first write to TTBR1? That would make it clearer that we have the required barriers and ordering, as we'd have a back-to-back sequence: cpu_set_reserved_ttbr0_nosync(); write_sysreg(ttbr1, ttbr1_el1); write_sysreg(ttbr0, ttbr0_el1); isb(); ... and it'd be less likely we'd accidentally break that in future. We didn't do that originally as back when we added this in commit: 7655abb953860485 ("arm64: mm: Move ASID from TTBR0 to TTBR1") ... cpu_do_switch_mm() was still written in asm and cpu_set_reserved_ttbr0() was written in C. We eventually moved cpu_do_switch_mm() to C in commit: 25b92693a1b67a47 ("arm64: mm: convert cpu_do_switch_mm() to C") ... but didn't think to move the call to cpu_set_reserved_ttbr0(). Thanks, Mark. > } > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index 05f4fc265428..14be5000c5a0 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -65,7 +65,6 @@ static inline void __uaccess_ttbr0_disable(void) > ttbr &= ~TTBR_ASID_MASK; > /* reserved_pg_dir placed before swapper_pg_dir */ > write_sysreg(ttbr - RESERVED_SWAPPER_OFFSET, ttbr0_el1); > - isb(); > /* Set reserved ASID */ > write_sysreg(ttbr, ttbr1_el1); > isb(); > @@ -89,7 +88,6 @@ static inline void __uaccess_ttbr0_enable(void) > ttbr1 &= ~TTBR_ASID_MASK; /* safety measure */ > ttbr1 |= ttbr0 & TTBR_ASID_MASK; > write_sysreg(ttbr1, ttbr1_el1); > - isb(); > > /* Restore user page table */ > write_sysreg(ttbr0, ttbr0_el1); > diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c > index e1e0dca01839..cf4ba575342e 100644 > --- a/arch/arm64/mm/context.c > +++ b/arch/arm64/mm/context.c > @@ -365,7 +365,6 @@ void cpu_do_switch_mm(phys_addr_t pgd_phys, struct mm_struct *mm) > ttbr1 |= FIELD_PREP(TTBR_ASID_MASK, asid); > > write_sysreg(ttbr1, ttbr1_el1); > - isb(); > write_sysreg(ttbr0, ttbr0_el1); > isb(); > post_ttbr_update_workaround(); > -- > 2.25.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel