Message ID | 20231221154702.2267684-5-guoren@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-8706-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2483:b0:fb:cd0c:d3e with SMTP id q3csp504507dyi; Thu, 21 Dec 2023 07:56:38 -0800 (PST) X-Google-Smtp-Source: AGHT+IFQprSxFTrB5Y4+0J6YJzp28yaol8fN9OroXhJXDSNQImNJw77RKAWlXIdtRJM1gPdeffGt X-Received: by 2002:a2e:b0db:0:b0:2cc:a596:836c with SMTP id g27-20020a2eb0db000000b002cca596836cmr227272ljl.48.1703174197846; Thu, 21 Dec 2023 07:56:37 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1703174197; cv=none; d=google.com; s=arc-20160816; b=LhwWmwRB8Ap6vKb6N171txdWJMCwkD3xm1yL3Suw0usnBloUGR+r4fPoyaSLFcNUSr GJsJt5LPxVJSnTjSSGWbQbJ3BUkeCEazqUWvpbdVz+utreahtlajtqbBEzPEFS3N8O/p tKVFnRpYSqYbjsxsD0U5WLhyMrP/j15Ll4c5H05RGv3U8uTEVAEiC+jMvNxP1CTo7L3w Vo04eowhP3yJWJQpM3KUAq/0jtr50sJy9wizvnNdxhSKVaoOp4u4Y5anBCkDq28l8QqI Zs2T3KwRqhfEIgOCu8FY51TA5afQi7qkqblOO6WMp7t1RlH6OolXlxzJNFpT51uxqdtO 5g+w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=AajFo/iT7WYVtZCobe2uu6pbNS/ImqF+Ccbr1QTAATk=; fh=+2B64yVuborNRhR0hRJ4G06jDIODc2br0dk92JgFIMw=; b=NQdYerMsXut4bK6rIlfmQgAvjAW0NXiMJ4hiEKxLD7N38gVCIoMsftfHQwZRkrTUni QYs8CSREtOJIIGLPR5qbRm1O17NnCOVxOQFoPnesKpYgSEe9QmveLJrq/eqkvjnWlQaH hKASjmY1/5pQzcN5I6VjEsQaKY+xQcX2H/IZS8/4FlGm2x6p3IStxzD+lMyNefJt74ho zSY22Dq3T8WZzel3Se+OSZCivSbqoq3P9MGeKiYEFwuZ8FPgc884s+YX3hZDcXi4Qviz J3ebOUl2S9CuJgEmfpKKFJwi0jKq2QKJ4667CijW4sXcd6PUWOUZX5cT9xSxy9sVpaJ6 /Tew== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=uhWq7HNl; spf=pass (google.com: domain of linux-kernel+bounces-8706-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-8706-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id cn20-20020a0564020cb400b0055386ca9e0bsi969410edb.286.2023.12.21.07.56.37 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Dec 2023 07:56:37 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-8706-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=uhWq7HNl; spf=pass (google.com: domain of linux-kernel+bounces-8706-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-8706-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id E74761F29B96 for <ouuuleilei@gmail.com>; Thu, 21 Dec 2023 15:48:21 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 752BC5991D; Thu, 21 Dec 2023 15:47:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="uhWq7HNl" X-Original-To: linux-kernel@vger.kernel.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EBD9458215 for <linux-kernel@vger.kernel.org>; Thu, 21 Dec 2023 15:47:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3FAB8C433CC; Thu, 21 Dec 2023 15:47:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1703173645; bh=AsvVmmiuGWBFu5stNq3XoYXyaKfmZAWOgztQ+Ek0sqY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=uhWq7HNl8+B0YhiTGUMxZjDlYDFYfy7vtI+h7lvAxn2hltTLuvJqzyS9lA/OSTMQ0 j/bRnzscfA31jLF0OIf5dpyilmyauPu/t8bsZprZPn/fTJSo8BvWwOh+dZXoX5KH7U 3Q2TrfEKJzGctJ3WbHBq/y/ehYU4EvRzS0xyLxbDLJO8CwhMQZhtsL7VBEVK1b6HjW QRFIwyuTmIHOEffqdhjB93JeIiBsMZFxYWtGK0x4+Sxi/pfczeoY2E6FJdQUNlfPN9 7X6ptv0pxxlOjUC0t5dE/n9RDR+UOaYXjMozP+dC1l2Tb1OToPPtbQaU9CGN/MjHpd 6446AbwEvSzUw== From: guoren@kernel.org To: linux-kernel@vger.kernel.org, paul.walmsley@sifive.com, palmer@dabbelt.com, alexghiti@rivosinc.com, charlie@rivosinc.com, xiao.w.wang@intel.com, guoren@kernel.org, david@redhat.com, panqinglin2020@iscas.ac.cn, rick.p.edgecombe@intel.com, willy@infradead.org, bjorn@rivosinc.com, conor.dooley@microchip.com, cleger@rivosinc.com, leobras@redhat.com Cc: linux-riscv@lists.infradead.org, Guo Ren <guoren@linux.alibaba.com> Subject: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition Date: Thu, 21 Dec 2023 10:47:01 -0500 Message-Id: <20231221154702.2267684-5-guoren@kernel.org> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20231221154702.2267684-1-guoren@kernel.org> References: <20231221154702.2267684-1-guoren@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785907588011552624 X-GMAIL-MSGID: 1785907588011552624 |
Series |
riscv: mm: Fixup & Optimize COMPAT code
|
|
Commit Message
Guo Ren
Dec. 21, 2023, 3:47 p.m. UTC
From: Guo Ren <guoren@linux.alibaba.com> Unify the TASK_SIZE definition with VA_BITS for better readability. Add COMPAT mode user address space info in the comment. Signed-off-by: Guo Ren <guoren@linux.alibaba.com> Signed-off-by: Guo Ren <guoren@kernel.org> --- arch/riscv/include/asm/pgtable.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Comments
On Thu, Dec 21, 2023 at 10:47:01AM -0500, guoren@kernel.org wrote: > From: Guo Ren <guoren@linux.alibaba.com> > > Unify the TASK_SIZE definition with VA_BITS for better readability. > Add COMPAT mode user address space info in the comment. > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/include/asm/pgtable.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h > index e415582276ec..d165ddae3b42 100644 > --- a/arch/riscv/include/asm/pgtable.h > +++ b/arch/riscv/include/asm/pgtable.h > @@ -866,6 +866,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte) > * Note that PGDIR_SIZE must evenly divide TASK_SIZE. > * Task size is: > * - 0x9fc00000 (~2.5GB) for RV32. > + * - 0x80000000 ( 2GB) for RV64 compat mode > * - 0x4000000000 ( 256GB) for RV64 using SV39 mmu > * - 0x800000000000 ( 128TB) for RV64 using SV48 mmu > * - 0x100000000000000 ( 64PB) for RV64 using SV57 mmu > @@ -877,11 +878,11 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte) > * Similarly for SV57, bits 63–57 must be equal to bit 56. > */ > #ifdef CONFIG_64BIT > -#define TASK_SIZE_64 (PGDIR_SIZE * PTRS_PER_PGD / 2) > +#define TASK_SIZE_64 (UL(1) << (VA_BITS - 1)) Checked for l5, l4 and l3, and it seems a correct replacement. > > #ifdef CONFIG_COMPAT > -#define TASK_SIZE_32 (_AC(0x80000000, UL)) > -#define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \ > +#define TASK_SIZE_32 (UL(1) << (VA_BITS_SV32 - 1)) Oh, much better. Thanks for removing the magic number :) > +#define TASK_SIZE (is_compat_task() ? \ > TASK_SIZE_32 : TASK_SIZE_64) > #else > #define TASK_SIZE TASK_SIZE_64 > -- > 2.40.1 > That's much more readable IMO now. Thanks! FWIW: Reviewed-by: Leonardo Bras <leobras@redhat.com>
On Fri, Dec 22, 2023 at 1:09 PM Leonardo Bras <leobras@redhat.com> wrote: > > On Thu, Dec 21, 2023 at 10:47:01AM -0500, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > Unify the TASK_SIZE definition with VA_BITS for better readability. > > Add COMPAT mode user address space info in the comment. > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Signed-off-by: Guo Ren <guoren@kernel.org> > > --- > > arch/riscv/include/asm/pgtable.h | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h > > index e415582276ec..d165ddae3b42 100644 > > --- a/arch/riscv/include/asm/pgtable.h > > +++ b/arch/riscv/include/asm/pgtable.h > > @@ -866,6 +866,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte) > > * Note that PGDIR_SIZE must evenly divide TASK_SIZE. > > * Task size is: > > * - 0x9fc00000 (~2.5GB) for RV32. > > + * - 0x80000000 ( 2GB) for RV64 compat mode > > * - 0x4000000000 ( 256GB) for RV64 using SV39 mmu > > * - 0x800000000000 ( 128TB) for RV64 using SV48 mmu > > * - 0x100000000000000 ( 64PB) for RV64 using SV57 mmu > > @@ -877,11 +878,11 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte) > > * Similarly for SV57, bits 63–57 must be equal to bit 56. > > */ > > #ifdef CONFIG_64BIT > > -#define TASK_SIZE_64 (PGDIR_SIZE * PTRS_PER_PGD / 2) > > +#define TASK_SIZE_64 (UL(1) << (VA_BITS - 1)) > > Checked for l5, l4 and l3, and it seems a correct replacement. > > > > > #ifdef CONFIG_COMPAT > > -#define TASK_SIZE_32 (_AC(0x80000000, UL)) > > -#define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \ > > +#define TASK_SIZE_32 (UL(1) << (VA_BITS_SV32 - 1)) > > Oh, much better. Thanks for removing the magic number :) > > > +#define TASK_SIZE (is_compat_task() ? \ > > TASK_SIZE_32 : TASK_SIZE_64) I would remove is_compat_task() in the next version because your patch contains that. > > #else > > #define TASK_SIZE TASK_SIZE_64 > > -- > > 2.40.1 > > > > That's much more readable IMO now. Thanks! > > FWIW: > Reviewed-by: Leonardo Bras <leobras@redhat.com> >
From: Guo Ren > Sent: 22 December 2023 11:25 ... > > > +#define TASK_SIZE (is_compat_task() ? \ > > > TASK_SIZE_32 : TASK_SIZE_64) > I would remove is_compat_task() in the next version because your patch > contains that. Does TASK_SIZE get used in access_ok() ? If so the repeated expansion of that 'mess' will slow things down. OTOH access_ok(ptr, len) can just check (ptr | (ptr + len)) < 0) and rely on the page faults for everything else. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David, On Fri, Dec 22, 2023 at 7:52 PM David Laight <David.Laight@aculab.com> wrote: > > From: Guo Ren > > Sent: 22 December 2023 11:25 > ... > > > > +#define TASK_SIZE (is_compat_task() ? \ > > > > TASK_SIZE_32 : TASK_SIZE_64) > > I would remove is_compat_task() in the next version because your patch > > contains that. > > Does TASK_SIZE get used in access_ok() ? > If so the repeated expansion of that 'mess' will slow things down. > > OTOH access_ok(ptr, len) can just check (ptr | (ptr + len)) < 0) > and rely on the page faults for everything else. I mean, I would remove is_compat_task() optimization. test_thread_flag(TIF_32BIT) -> (is_compat_task() ? Sorry for the bad wording. Leonardo's new patch series contains the optimization on is_compat_task(), so I canceled mine. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
On Fri, Dec 22, 2023 at 7:52 PM David Laight <David.Laight@aculab.com> wrote: > > From: Guo Ren > > Sent: 22 December 2023 11:25 > ... > > > > +#define TASK_SIZE (is_compat_task() ? \ > > > > TASK_SIZE_32 : TASK_SIZE_64) > > I would remove is_compat_task() in the next version because your patch > > contains that. > > Does TASK_SIZE get used in access_ok() ? > If so the repeated expansion of that 'mess' will slow things down. > > OTOH access_ok(ptr, len) can just check (ptr | (ptr + len)) < 0) > and rely on the page faults for everything else. Or do you want to discuss the bad side effect of is_compat_task()? Yes, test_thread_flag(TIF_32BIT) would slow down access_ok(). But if we use TASK_SIZE_MAX, VA_BITS still needs pgtable_l5_enabled, pgtable_l4_enabled detectation for riscv. It's not only for compat mode, but also Sv39, Sv48, Sv57. All treat TASK_SIZE_MAX as 0x8000000000000000, right? Then: access_ok(ptr, len) can just check (ptr | (ptr + len)) < 0) It's another feature and does not relate to compat mode. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
From: Guo Ren > Sent: 23 December 2023 02:53 > > On Fri, Dec 22, 2023 at 7:52 PM David Laight <David.Laight@aculab.com> wrote: > > > > From: Guo Ren > > > Sent: 22 December 2023 11:25 > > ... > > > > > +#define TASK_SIZE (is_compat_task() ? \ > > > > > TASK_SIZE_32 : TASK_SIZE_64) > > > I would remove is_compat_task() in the next version because your patch > > > contains that. > > > > Does TASK_SIZE get used in access_ok() ? > > If so the repeated expansion of that 'mess' will slow things down. > > > > OTOH access_ok(ptr, len) can just check (ptr | (ptr + len)) < 0) > > and rely on the page faults for everything else. > Or do you want to discuss the bad side effect of is_compat_task()? > > Yes, test_thread_flag(TIF_32BIT) would slow down access_ok(). But if > we use TASK_SIZE_MAX, VA_BITS still needs pgtable_l5_enabled, > pgtable_l4_enabled detectation for riscv. > > It's not only for compat mode, but also Sv39, Sv48, Sv57. All treat > TASK_SIZE_MAX as 0x8000000000000000, right? Then: > access_ok(ptr, len) can just check (ptr | (ptr + len)) < 0) > > It's another feature and does not relate to compat mode. Compat mode just makes it worse... One possibility would be to save the task's max user address in the task structure itself - that would save all the conditionals at a cost of an extra value in the task structure. There is also the question of whether a normally 64-bit task can actually make the compat mmap() system call? On x86 that is certainly possible (IIRC wine does it), x86 userspace can flip between 32bit and 63bit mode without a system call. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sat, Dec 23, 2023 at 6:31 PM David Laight <David.Laight@aculab.com> wrote: > > From: Guo Ren > > Sent: 23 December 2023 02:53 > > > > On Fri, Dec 22, 2023 at 7:52 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > From: Guo Ren > > > > Sent: 22 December 2023 11:25 > > > ... > > > > > > +#define TASK_SIZE (is_compat_task() ? \ > > > > > > TASK_SIZE_32 : TASK_SIZE_64) > > > > I would remove is_compat_task() in the next version because your patch > > > > contains that. > > > > > > Does TASK_SIZE get used in access_ok() ? > > > If so the repeated expansion of that 'mess' will slow things down. > > > > > > OTOH access_ok(ptr, len) can just check (ptr | (ptr + len)) < 0) > > > and rely on the page faults for everything else. > > Or do you want to discuss the bad side effect of is_compat_task()? > > > > Yes, test_thread_flag(TIF_32BIT) would slow down access_ok(). But if > > we use TASK_SIZE_MAX, VA_BITS still needs pgtable_l5_enabled, > > pgtable_l4_enabled detectation for riscv. > > > > It's not only for compat mode, but also Sv39, Sv48, Sv57. All treat > > TASK_SIZE_MAX as 0x8000000000000000, right? Then: > > access_ok(ptr, len) can just check (ptr | (ptr + len)) < 0) > > > > It's another feature and does not relate to compat mode. > > Compat mode just makes it worse... It's hard to observe. > > One possibility would be to save the task's max user address > in the task structure itself - that would save all the conditionals > at a cost of an extra value in the task structure. It would still cause memory load operation, although it is $tp->xxx. If we want to gain observability benefits, "just check (ptr | (ptr + len)) < 0)" is better. > > There is also the question of whether a normally 64-bit task > can actually make the compat mmap() system call? No. > On x86 that is certainly possible (IIRC wine does it), x86 > userspace can flip between 32bit and 63bit mode without a > system call. RISC-V can't do that because it needs sstatux.uxl=32/64, which can only be modified in S-mode. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
From: Guo Ren > Sent: 24 December 2023 01:24 ... > > One possibility would be to save the task's max user address > > in the task structure itself - that would save all the conditionals > > at a cost of an extra value in the task structure. > It would still cause memory load operation, although it is $tp->xxx. All the (mispredicted) branches are likely to cause more of a problem than a load from the current task structure. > If we want to gain observability benefits, "just check (ptr | (ptr + > len)) < 0)" is better. If you can guarantee a faulting page between user and kernel addresses and assume (check) that the accesses are 'reasonably sequential' then you only need to check the base address. That is likely hard for 32bit but easier for 64bit (except arm64) because A63 and A62 have to match. Unless you have some hardware address masking which makes it much more likely that 'random values' will be valid addresses. (Someone remind me why that is a good idea unless the high bits are validated by the hardware.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h index e415582276ec..d165ddae3b42 100644 --- a/arch/riscv/include/asm/pgtable.h +++ b/arch/riscv/include/asm/pgtable.h @@ -866,6 +866,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte) * Note that PGDIR_SIZE must evenly divide TASK_SIZE. * Task size is: * - 0x9fc00000 (~2.5GB) for RV32. + * - 0x80000000 ( 2GB) for RV64 compat mode * - 0x4000000000 ( 256GB) for RV64 using SV39 mmu * - 0x800000000000 ( 128TB) for RV64 using SV48 mmu * - 0x100000000000000 ( 64PB) for RV64 using SV57 mmu @@ -877,11 +878,11 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte) * Similarly for SV57, bits 63–57 must be equal to bit 56. */ #ifdef CONFIG_64BIT -#define TASK_SIZE_64 (PGDIR_SIZE * PTRS_PER_PGD / 2) +#define TASK_SIZE_64 (UL(1) << (VA_BITS - 1)) #ifdef CONFIG_COMPAT -#define TASK_SIZE_32 (_AC(0x80000000, UL)) -#define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \ +#define TASK_SIZE_32 (UL(1) << (VA_BITS_SV32 - 1)) +#define TASK_SIZE (is_compat_task() ? \ TASK_SIZE_32 : TASK_SIZE_64) #else #define TASK_SIZE TASK_SIZE_64