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 X-Spam-Level: X-Spam-Status: No, score=-19.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AC50FC433B4 for ; Tue, 18 May 2021 15:35:47 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 39A15611CE for ; Tue, 18 May 2021 15:35:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 39A15611CE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; 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=LqbmGXBHUGGNLOFvffEgAwcRsEFKqYlZ+I7aVKQJwTg=; b=lD+NX+3Bkv+XcoZzEuuj9NF1S l3O+ma9MqnPxZLCfpDCEDN7FnMoOHlev9ILPN3epAjhgYVbsSPj5AlFmkEAH15p/Z5H5cT2tFWrON 5+J+mNcJn+WXnR43zhC2lGQlrlsemL9OSAMpzSW7XpZOBFbI7VmhEVZClpsqzJV1uUbOwOOoTod+2 7QZCfazSNA5iTYa8yCyzBVCm2d1Fu+Ja5tle0ikEwVfrQ51uI9DY8O2Ets1mSiN1dayRwQ7fVF8Kk 5fNsc8ae+jJsV2c1baXNV7U8NYzBPiVtnjAzq+YjF4jeu+bmrkzyzsjhiPsusCyDh5l1jWQdUXlTM BU4GmHh2g==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lj1jU-001Bwa-Ql; Tue, 18 May 2021 15:34:08 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lj1jP-001BwQ-AY for linux-arm-kernel@desiato.infradead.org; Tue, 18 May 2021 15:34:03 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=2BNIeHVlwtAdDadmBGRd1TlySL8j/UT0DfmPH9r7Z+M=; b=B+1SRpPjiatVpqMkiqFimL04SJ AZR5f3zYs4dqHko5YGZk0TZlJliZq9nmqyPwsMKR/OY7qGgcO0ytHnfe76VppP0acivv7UvN0uRnw +Orm5uim/ZzQJKXD2Ymh0+4mhLulqZ5ZdN0y4/EgBaW1/71G4ylptzFoX/l6SJ/hTpVFrSaEp+1bj BljIn2MgV/Cfn8jAQmLdg9eoHQkXGJ48Y9yvUucTh4/JzdjBWhNpCDlUul3j0h5ZtdSSgCdHQ6yKL KDEt0qHvhcGrPP5G4jciqtO60KKkISQ+Gw3FnSW+svGnJyfzVXQFlTD18jeTOMPDpjF4lrxYWkWyq ga9I4gHw==; Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lj1jL-00ElkW-Sl for linux-arm-kernel@lists.infradead.org; Tue, 18 May 2021 15:34:02 +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 C2585ED1; Tue, 18 May 2021 08:33:56 -0700 (PDT) Received: from C02TD0UTHF1T.local (unknown [10.57.6.226]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7AE0B3F73B; Tue, 18 May 2021 08:33:54 -0700 (PDT) Date: Tue, 18 May 2021 16:33:51 +0100 From: Mark Rutland To: Fuad Tabba Cc: linux-arm-kernel@lists.infradead.org, will@kernel.org, catalin.marinas@arm.com, maz@kernel.org, ardb@kernel.org, james.morse@arm.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, robin.murphy@arm.com Subject: Re: [PATCH v2 02/16] arm64: Do not enable uaccess for flush_icache_range Message-ID: <20210518153351.GJ82842@C02TD0UTHF1T.local> References: <20210517075124.152151-1-tabba@google.com> <20210517075124.152151-3-tabba@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210517075124.152151-3-tabba@google.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210518_083400_058577_DF58AE0D X-CRM114-Status: GOOD ( 33.35 ) 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 Hi Fuad, This is great! I had a play with the series locally, and I have a few suggestions below for how to make this a bit clearer. On Mon, May 17, 2021 at 08:51:10AM +0100, Fuad Tabba wrote: > __flush_icache_range works on the kernel linear map, and doesn't > need uaccess. The existing code is a side-effect of its current > implementation with __flush_cache_user_range fallthrough. > > Instead of fallthrough to share the code, use a common macro for > the two where the caller can specify whether user-space access is > needed. > > No functional change intended. > Possible performance impact due to the reduced number of > instructions. This looks correct, but I'm not too keen on all the duplication we have to do w.r.t. `needs_uaccess`, and I think it would be much clearer to put the TTBR maintenance directly in `__flush_cache_user_range` immediately, rather than doing that later in the series. > Reported-by: Catalin Marinas > Reported-by: Will Deacon > Link: https://lore.kernel.org/linux-arch/20200511110014.lb9PEahJ4hVOYrbwIb_qUHXyNy9KQzNFdb_I3YlzY6A@z/ > Signed-off-by: Fuad Tabba > --- > arch/arm64/include/asm/assembler.h | 13 ++++-- > arch/arm64/mm/cache.S | 64 +++++++++++++++++++++--------- > 2 files changed, 54 insertions(+), 23 deletions(-) > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > index 8418c1bd8f04..6ff7a3a3b238 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -426,16 +426,21 @@ alternative_endif > * Macro to perform an instruction cache maintenance for the interval > * [start, end) > * > - * start, end: virtual addresses describing the region > - * label: A label to branch to on user fault. > - * Corrupts: tmp1, tmp2 > + * start, end: virtual addresses describing the region > + * needs_uaccess: might access user space memory > + * label: label to branch to on user fault (if needs_uaccess) > + * Corrupts: tmp1, tmp2 > */ I'm not too keen on the separate `needs_uaccess` and `label` arguments. We should be able to collapse those into a single argument by checking with .ifnc, e.g. .macro op arg, fixup .ifnc fixup, do_thing_with \fixup .endif .endm ... which I think would make things clearer overall. > - .macro invalidate_icache_by_line start, end, tmp1, tmp2, label > + .macro invalidate_icache_by_line start, end, tmp1, tmp2, needs_uaccess, label > icache_line_size \tmp1, \tmp2 > sub \tmp2, \tmp1, #1 > bic \tmp2, \start, \tmp2 > 9997: > + .if \needs_uaccess > USER(\label, ic ivau, \tmp2) // invalidate I line PoU > + .else > + ic ivau, \tmp2 > + .endif > add \tmp2, \tmp2, \tmp1 > cmp \tmp2, \end > b.lo 9997b I'm also not keen on duplicating the instruction here. I reckon what we should do is add a conditional extable macro: .macro _cond_extable insn, fixup .ifnc \fixup, _asm_extable \insn, \fixup .endif .endm ... which'd allow us to do: .macro invalidate_icache_by_line start, end, tmp1, tmp2, fixup icache_line_size \tmp1, \tmp2 sub \tmp2, \tmp1, #1 bic \tmp2, \start, \tmp2 .Licache_op\@: ic ivau, \tmp2 // invalidate I line PoU add \tmp2, \tmp2, \tmp1 cmp \tmp2, \end b.lo .Licache_op\@ dsb ish isb _cond_extable .Licache_op\@, \fixup .endm ... which I think is clearer. We could do likewise in dcache_by_line_op, and with some refactoring we could remove the logic that we have to currently duplicate. I pushed a couple of prearatory patches for that to: https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/cleanups/cache git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/cleanups/cache ... in case you felt like taking those as-is. > diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S > index 2d881f34dd9d..092f73acdf9a 100644 > --- a/arch/arm64/mm/cache.S > +++ b/arch/arm64/mm/cache.S > @@ -15,30 +15,20 @@ > #include > > /* > - * flush_icache_range(start,end) > + * __flush_cache_range(start,end) [needs_uaccess] > * > * Ensure that the I and D caches are coherent within specified region. > * This is typically used when code has been written to a memory region, > * and will be executed. > * > - * - start - virtual start address of region > - * - end - virtual end address of region > + * - start - virtual start address of region > + * - end - virtual end address of region > + * - needs_uaccess - (macro parameter) might access user space memory > */ > -SYM_FUNC_START(__flush_icache_range) > - /* FALLTHROUGH */ > - > -/* > - * __flush_cache_user_range(start,end) > - * > - * Ensure that the I and D caches are coherent within specified region. > - * This is typically used when code has been written to a memory region, > - * and will be executed. > - * > - * - start - virtual start address of region > - * - end - virtual end address of region > - */ > -SYM_FUNC_START(__flush_cache_user_range) > +.macro __flush_cache_range, needs_uaccess > + .if \needs_uaccess > uaccess_ttbr0_enable x2, x3, x4 > + .endif > alternative_if ARM64_HAS_CACHE_IDC > dsb ishst > b 7f > @@ -47,7 +37,11 @@ alternative_else_nop_endif > sub x3, x2, #1 > bic x4, x0, x3 > 1: > + .if \needs_uaccess > user_alt 9f, "dc cvau, x4", "dc civac, x4", ARM64_WORKAROUND_CLEAN_CACHE > + .else > +alternative_insn "dc cvau, x4", "dc civac, x4", ARM64_WORKAROUND_CLEAN_CACHE > + .endif > add x4, x4, x2 > cmp x4, x1 > b.lo 1b > @@ -58,15 +52,47 @@ alternative_if ARM64_HAS_CACHE_DIC > isb > b 8f > alternative_else_nop_endif > - invalidate_icache_by_line x0, x1, x2, x3, 9f > + invalidate_icache_by_line x0, x1, x2, x3, \needs_uaccess, 9f > 8: mov x0, #0 > 1: > + .if \needs_uaccess > uaccess_ttbr0_disable x1, x2 > + .endif > ret > + > + .if \needs_uaccess > 9: > mov x0, #-EFAULT > b 1b > + .endif > +.endm As above, I think we should reduce this to the core logic, moving the ttbr manipulation and fixup handler inline in __flush_cache_user_range. For clarity, I'd also like to leave the RETs out of the macro, since that's required for the fixup handling anyway, and it generally amkes the control flow clearer at the function definition. > +/* > + * flush_icache_range(start,end) > + * > + * Ensure that the I and D caches are coherent within specified region. > + * This is typically used when code has been written to a memory region, > + * and will be executed. > + * > + * - start - virtual start address of region > + * - end - virtual end address of region > + */ > +SYM_FUNC_START(__flush_icache_range) > + __flush_cache_range needs_uaccess=0 > SYM_FUNC_END(__flush_icache_range) ...so with the suggestions above, this could be: SYM_FUNC_START(__flush_icache_range) __flush_cache_range ret SYM_FUNC_END(__flush_icache_range) > +/* > + * __flush_cache_user_range(start,end) > + * > + * Ensure that the I and D caches are coherent within specified region. > + * This is typically used when code has been written to a memory region, > + * and will be executed. > + * > + * - start - virtual start address of region > + * - end - virtual end address of region > + */ > +SYM_FUNC_START(__flush_cache_user_range) > + __flush_cache_range needs_uaccess=1 > SYM_FUNC_END(__flush_cache_user_range) ... this could be: SYM_FUNC_START(__flush_cache_user_range) uaccess_ttbr0_enable x2, x3, x4 __flush_cache_range 2f 1: uaccess_ttbr0_disable x1, x2 ret 2: mov x0, #-EFAULT b 1b SYM_FUNC_END(__flush_cache_user_range) > /* > @@ -86,7 +112,7 @@ alternative_else_nop_endif > > uaccess_ttbr0_enable x2, x3, x4 > > - invalidate_icache_by_line x0, x1, x2, x3, 2f > + invalidate_icache_by_line x0, x1, x2, x3, 1, 2f ... and this wouldn't need to change. Thanks, Mark. > mov x0, xzr > 1: > uaccess_ttbr0_disable x1, x2 > -- > 2.31.1.751.gd2f1c929bd-goog > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel