Message ID | 20231221154702.2267684-3-guoren@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-8704-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2483:b0:fb:cd0c:d3e with SMTP id q3csp499762dyi; Thu, 21 Dec 2023 07:47:54 -0800 (PST) X-Google-Smtp-Source: AGHT+IEN51GcHpkm52XzDrxaVfv7ft8lstSSGTaGQPpQwdT0yTr/tsWrtnW8rYHqCxwzT6QXdHXv X-Received: by 2002:a05:6359:4c01:b0:170:d724:b7a9 with SMTP id kj1-20020a0563594c0100b00170d724b7a9mr1534961rwc.39.1703173673967; Thu, 21 Dec 2023 07:47:53 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1703173673; cv=none; d=google.com; s=arc-20160816; b=qEuuaL5IX6FnYDZiqE0WUS3cd72McDUum4j0r9FnYPogehd63ioiepmxBG8H8mXXlK KOH1IL9wkjsQ5qTpnMqd0hZjzLgSxRCpDMl6DtTatthPhChadhWGf2hiWqvD8E35wYmi IkA/F4HwsHxID4mXBJBwpnBF07Zo1JkD6fy/uP2G9mqylMFeiZsYa0JvtmxLaTq7jMXx kA8Kv9zwFlhErRUzqeqUwVpSjBIPm7BGEwc0vs51hPbe/cs5Rg5a4clgWfY1AF8pLiVS R0QkxQEebrz8yLt4dSA8eDajD+08zMGtA1gCjMQTxM5hJDqbThsYbTEVBOV6o1+UpMQ3 FnLw== 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=P5fSU2y1bQrGuo1xDnVoB9ZBkV6O9GH5rhy3PHTHV0M=; fh=3YQLS1cPsLhMKuGGotL7Ux7rpkwBoh7ZT2nN27VXc5c=; b=MQFCw4WJIJCJFoRkBl7j95di6HavWy4mCeDOw7crKjPZ7LKZEHPmHXi5j/i4tyWPmV GpAEmA3xHd24AZWiDgiQxrII0lNPIT3bNXwXbcvX+IwIdPZSRCxU7Ox2YuWv25Y0t16v IN1YfaI6eOD3lt4mPZ+DasdrYj3eABOidCkwCw7nWXCPwApEsCU9WPrnP+MmWHFvfpIR IHl9tbIQTHVeBXQKiXP4z5HhrBww10ZewMqCnkcuio+UCPKVWOaU4bAq5J5/eyHyS7as 3/fiVLfKp0075qWnoB4qyxb9kivLM2oagJPCNxBJiVM9w5V7IW1WXFhtbFX89tl9iMX1 YAgg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=cDlxg7WA; spf=pass (google.com: domain of linux-kernel+bounces-8704-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-8704-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id c34-20020a630d22000000b005b8a295f016si1812142pgl.64.2023.12.21.07.47.53 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Dec 2023 07:47:53 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-8704-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=cDlxg7WA; spf=pass (google.com: domain of linux-kernel+bounces-8704-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-8704-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 sv.mirrors.kernel.org (Postfix) with ESMTPS id AA34D2893E1 for <ouuuleilei@gmail.com>; Thu, 21 Dec 2023 15:47:53 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 4012653A17; Thu, 21 Dec 2023 15:47:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="cDlxg7WA" 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 A625455E41; Thu, 21 Dec 2023 15:47:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C8972C433C8; Thu, 21 Dec 2023 15:47:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1703173638; bh=dOGyrLopRgWASgaIZLAx6CNRKKRLi1o/fzWOzw5hikY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=cDlxg7WA1eBIwKV7bwZmyHXskjpiO8H0zm7M2Ghzhr/xJdhhL6Xl5kRI6BWyYA5lD VBxAhJBQbB+9gO3Ag/gZJDFb+yKcYSbIwQNpD0ma517/zmjRtZImkNo7sEdHNxRaMo 1gXtByNw8LvPKTENtE7Eyo0fI2WALLGGdtPJwJF36c/Jju3NFsv75uhnr92pS3Mkxz 9QYAV4nD600gK2ZN6ikxvxtty6vjKnY3kXSgadf2i6U5AbJDy8ZsKxPN1iBVipFQBV fplnF8mxIeIWVezxJQwQjGSA/khdXnqsTghUjHSyjZpLYbroFFW3LkjSldkXwKqcCY 8N6jkGpF6/EHQ== 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>, stable@vger.kernel.org Subject: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end Date: Thu, 21 Dec 2023 10:46:59 -0500 Message-Id: <20231221154702.2267684-3-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-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785907038050821313 X-GMAIL-MSGID: 1785907038050821313 |
Series |
riscv: mm: Fixup & Optimize COMPAT code
|
|
Commit Message
Guo Ren
Dec. 21, 2023, 3:46 p.m. UTC
From: Guo Ren <guoren@linux.alibaba.com> When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() detection, so change the definition of STACK_TOP_MAX to TASK_SIZE directly. Cc: stable@vger.kernel.org Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") Signed-off-by: Guo Ren <guoren@linux.alibaba.com> Signed-off-by: Guo Ren <guoren@kernel.org> --- arch/riscv/include/asm/processor.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Comments
On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > From: Guo Ren <guoren@linux.alibaba.com> > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > directly. ok > > Cc: stable@vger.kernel.org > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > Signed-off-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/include/asm/processor.h | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > index f19f861cda54..1f538fc4448d 100644 > --- a/arch/riscv/include/asm/processor.h > +++ b/arch/riscv/include/asm/processor.h > @@ -16,15 +16,13 @@ > > #ifdef CONFIG_64BIT > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > -#define STACK_TOP_MAX TASK_SIZE_64 > +#define STACK_TOP_MAX TASK_SIZE It means STACK_TOP_MAX will be in 64BIT: - TASK_SIZE_32 if compat_mode=y - TASK_SIZE_64 if compat_mode=n Makes sense for me. > > #define arch_get_mmap_end(addr, len, flags) \ > ({ \ > unsigned long mmap_end; \ > typeof(addr) _addr = (addr); \ > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > - mmap_end = STACK_TOP_MAX; \ > - else if ((_addr) >= VA_USER_SV57) \ > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > mmap_end = STACK_TOP_MAX; \ > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > mmap_end = VA_USER_SV48; \ I don't think I got this change, or how it's connected to the commit msg. Before: - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX - 2^48 < addr < 2^57: mmap_end = 2^48 - 0 < addr < 2^48 : mmap_end = 2^39 Now: - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX - 2^48 < addr < 2^57: mmap_end = 2^48 - 0 < addr < 2^48 : mmap_end = 2^39 IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 if addr != 0. Is that desireable? (if not, above change is unneeded) Also, unrelated to the change: - 2^48 < addr < 2^57: mmap_end = 2^48 Is the above correct? It looks like it should be 2^57 instead, and a new if clause for 2^32 < addr < 2^48 should have mmap_end = 2^48. Do I get it wrong? (I will send an RFC 'fixing' the code the way I am whinking it should look like) Thanks, Leo > -- > 2.40.1 >
On Fri, Dec 22, 2023 at 12:34:56AM -0300, Leonardo Bras wrote: > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > directly. > > ok > > > > > Cc: stable@vger.kernel.org > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Signed-off-by: Guo Ren <guoren@kernel.org> > > --- > > arch/riscv/include/asm/processor.h | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > index f19f861cda54..1f538fc4448d 100644 > > --- a/arch/riscv/include/asm/processor.h > > +++ b/arch/riscv/include/asm/processor.h > > @@ -16,15 +16,13 @@ > > > > #ifdef CONFIG_64BIT > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > -#define STACK_TOP_MAX TASK_SIZE_64 > > +#define STACK_TOP_MAX TASK_SIZE > > It means STACK_TOP_MAX will be in 64BIT: > - TASK_SIZE_32 if compat_mode=y > - TASK_SIZE_64 if compat_mode=n > > Makes sense for me. > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > ({ \ > > unsigned long mmap_end; \ > > typeof(addr) _addr = (addr); \ > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > - mmap_end = STACK_TOP_MAX; \ > > - else if ((_addr) >= VA_USER_SV57) \ > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > mmap_end = STACK_TOP_MAX; \ > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > mmap_end = VA_USER_SV48; \ > > > I don't think I got this change, or how it's connected to the commit msg. > > Before: > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > - 2^48 < addr < 2^57: mmap_end = 2^48 > - 0 < addr < 2^48 : mmap_end = 2^39 > > Now: > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > - 2^48 < addr < 2^57: mmap_end = 2^48 > - 0 < addr < 2^48 : mmap_end = 2^39 > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > if addr != 0. Is that desireable? > (if not, above change is unneeded) I agree, this change does not make sense for compat mode. Compat mode should never return an address that is greater than 2^32, but this change allows that. > > Also, unrelated to the change: > - 2^48 < addr < 2^57: mmap_end = 2^48 > Is the above correct? > It looks like it should be 2^57 instead, and a new if clause for > 2^32 < addr < 2^48 should have mmap_end = 2^48. That is not the case. I documented this behavior and reasoning in Documentation/arch/riscv/vm-layout.rst in the "Userspace VAs" section. I can reiterate here though. The hint address to mmap (defined here as "addr") is the maximum userspace address that mmap should provide. What you are describing is a minimum. The purpose of this change was to allow applications that are not compatible with a larger virtual address (such as applications like Java that use the upper bits of the VA to store data) to have a consistent way of specifying how many bits they would like to be left free in the VA. This requires to take the next lowest address space to guaruntee that all of the most-significant bits left clear in hint address do not end up populated in the virtual address returned by mmap. - Charlie > > Do I get it wrong? > > (I will send an RFC 'fixing' the code the way I am whinking it should look > like) > > Thanks, > Leo > > > > > > > -- > > 2.40.1 > > >
On Thu, Dec 21, 2023 at 08:04:43PM -0800, Charlie Jenkins wrote: > On Fri, Dec 22, 2023 at 12:34:56AM -0300, Leonardo Bras wrote: > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > > directly. > > > > ok > > > > > > > > Cc: stable@vger.kernel.org > > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > --- > > > arch/riscv/include/asm/processor.h | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > index f19f861cda54..1f538fc4448d 100644 > > > --- a/arch/riscv/include/asm/processor.h > > > +++ b/arch/riscv/include/asm/processor.h > > > @@ -16,15 +16,13 @@ > > > > > > #ifdef CONFIG_64BIT > > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > > -#define STACK_TOP_MAX TASK_SIZE_64 > > > +#define STACK_TOP_MAX TASK_SIZE > > > > It means STACK_TOP_MAX will be in 64BIT: > > - TASK_SIZE_32 if compat_mode=y > > - TASK_SIZE_64 if compat_mode=n > > > > Makes sense for me. > > > > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > > ({ \ > > > unsigned long mmap_end; \ > > > typeof(addr) _addr = (addr); \ > > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > - mmap_end = STACK_TOP_MAX; \ > > > - else if ((_addr) >= VA_USER_SV57) \ > > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > mmap_end = STACK_TOP_MAX; \ > > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > > mmap_end = VA_USER_SV48; \ > > > > > > I don't think I got this change, or how it's connected to the commit msg. > > > > Before: > > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > Now: > > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > > if addr != 0. Is that desireable? > > (if not, above change is unneeded) > > I agree, this change does not make sense for compat mode. Compat mode > should never return an address that is greater than 2^32, but this > change allows that. > > > > > Also, unrelated to the change: > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > Is the above correct? > > It looks like it should be 2^57 instead, and a new if clause for > > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > That is not the case. I documented this behavior and reasoning in > Documentation/arch/riscv/vm-layout.rst in the "Userspace VAs" section. > > I can reiterate here though. The hint address to mmap (defined here as > "addr") is the maximum userspace address that mmap should provide. What > you are describing is a minimum. The purpose of this change was to allow > applications that are not compatible with a larger virtual address (such > as applications like Java that use the upper bits of the VA to store > data) to have a consistent way of specifying how many bits they would > like to be left free in the VA. This requires to take the next lowest > address space to guaruntee that all of the most-significant bits left > clear in hint address do not end up populated in the virtual address > returned by mmap. > > - Charlie Hello Charlie, thank you for helping me understand! Ok, that does make sense now! The addr value hints "don't allocate > addr" and thus: - 0 < addr < 2^48 : mmap_end = 2^39 - 2^48 < addr < 2^57: mmap_end = 2^48 Ok, but then - addr > 2^57: mmap_end = 2^57 right? I mean, probably STACK_TOP_MAX in non-compat mode means 2^57 already, but having it explicitly like: else if ((_addr) >= VA_USER_SV57) \ mmap_end = VA_USER_SV57; \ would not be better for a future full 64-bit addressing? (since it's already on a different if clause) I could add comment on top of the macro with a short version on your addr hint description above. Would that be ok? Thanks! Leo > > > > > Do I get it wrong? > > > > (I will send an RFC 'fixing' the code the way I am whinking it should look > > like) > > > > Thanks, > > Leo > > > > > > > > > > > > > -- > > > 2.40.1 > > > > > >
On Fri, Dec 22, 2023 at 11:35 AM Leonardo Bras <leobras@redhat.com> wrote: > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > From: Guo Ren <guoren@linux.alibaba.com> > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > directly. > > ok > > > > > Cc: stable@vger.kernel.org > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > Signed-off-by: Guo Ren <guoren@kernel.org> > > --- > > arch/riscv/include/asm/processor.h | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > index f19f861cda54..1f538fc4448d 100644 > > --- a/arch/riscv/include/asm/processor.h > > +++ b/arch/riscv/include/asm/processor.h > > @@ -16,15 +16,13 @@ > > > > #ifdef CONFIG_64BIT > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > -#define STACK_TOP_MAX TASK_SIZE_64 > > +#define STACK_TOP_MAX TASK_SIZE > > It means STACK_TOP_MAX will be in 64BIT: > - TASK_SIZE_32 if compat_mode=y > - TASK_SIZE_64 if compat_mode=n > > Makes sense for me. > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > ({ \ > > unsigned long mmap_end; \ > > typeof(addr) _addr = (addr); \ > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > - mmap_end = STACK_TOP_MAX; \ > > - else if ((_addr) >= VA_USER_SV57) \ > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > mmap_end = STACK_TOP_MAX; \ > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > mmap_end = VA_USER_SV48; \ > > > I don't think I got this change, or how it's connected to the commit msg. The above is just code simplification; if STACK_TOP_MAX is TASK_SIZE, then if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ mmap_end = STACK_TOP_MAX; \ else if ((_addr) >= VA_USER_SV57) \ is equal to: if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > Before: > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > - 2^48 < addr < 2^57: mmap_end = 2^48 > - 0 < addr < 2^48 : mmap_end = 2^39 > > Now: > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > - 2^48 < addr < 2^57: mmap_end = 2^48 > - 0 < addr < 2^48 : mmap_end = 2^39 > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > if addr != 0. Is that desireable? > (if not, above change is unneeded) > > Also, unrelated to the change: > - 2^48 < addr < 2^57: mmap_end = 2^48 > Is the above correct? > It looks like it should be 2^57 instead, and a new if clause for > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > Do I get it wrong? Maybe I should move this into the optimization part. > > (I will send an RFC 'fixing' the code the way I am whinking it should look > like) > > Thanks, > Leo > > > > > > > -- > > 2.40.1 > > >
On Fri, Dec 22, 2023 at 12:04 PM Charlie Jenkins <charlie@rivosinc.com> wrote: > > On Fri, Dec 22, 2023 at 12:34:56AM -0300, Leonardo Bras wrote: > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > > directly. > > > > ok > > > > > > > > Cc: stable@vger.kernel.org > > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > --- > > > arch/riscv/include/asm/processor.h | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > index f19f861cda54..1f538fc4448d 100644 > > > --- a/arch/riscv/include/asm/processor.h > > > +++ b/arch/riscv/include/asm/processor.h > > > @@ -16,15 +16,13 @@ > > > > > > #ifdef CONFIG_64BIT > > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > > -#define STACK_TOP_MAX TASK_SIZE_64 > > > +#define STACK_TOP_MAX TASK_SIZE > > > > It means STACK_TOP_MAX will be in 64BIT: > > - TASK_SIZE_32 if compat_mode=y > > - TASK_SIZE_64 if compat_mode=n > > > > Makes sense for me. > > > > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > > ({ \ > > > unsigned long mmap_end; \ > > > typeof(addr) _addr = (addr); \ > > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > - mmap_end = STACK_TOP_MAX; \ > > > - else if ((_addr) >= VA_USER_SV57) \ > > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > mmap_end = STACK_TOP_MAX; \ > > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > > mmap_end = VA_USER_SV48; \ > > > > > > I don't think I got this change, or how it's connected to the commit msg. > > > > Before: > > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > Now: > > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > > if addr != 0. Is that desireable? > > (if not, above change is unneeded) > > I agree, this change does not make sense for compat mode. Compat mode > should never return an address that is greater than 2^32, but this > change allows that. #define STACK_TOP_MAX TASK_SIZE #define TASK_SIZE (is_compat_task() ? TASK_SIZE_32 : TASK_SIZE_64) So, this change limits an address in 2^32 for compat mode, and your patch broke the rule. So that is why we need this patch to fix up. > > > > > Also, unrelated to the change: > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > Is the above correct? > > It looks like it should be 2^57 instead, and a new if clause for > > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > That is not the case. I documented this behavior and reasoning in > Documentation/arch/riscv/vm-layout.rst in the "Userspace VAs" section. > > I can reiterate here though. The hint address to mmap (defined here as > "addr") is the maximum userspace address that mmap should provide. What > you are describing is a minimum. The purpose of this change was to allow > applications that are not compatible with a larger virtual address (such > as applications like Java that use the upper bits of the VA to store > data) to have a consistent way of specifying how many bits they would Yes, I agree with this change and use Zjpm with PLEN=48 to optimize them in the future. > like to be left free in the VA. This requires to take the next lowest > address space to guaruntee that all of the most-significant bits left > clear in hint address do not end up populated in the virtual address > returned by mmap. > > - Charlie > > > > > Do I get it wrong? > > > > (I will send an RFC 'fixing' the code the way I am whinking it should look > > like) > > > > Thanks, > > Leo > > > > > > > > > > > > > -- > > > 2.40.1 > > > > >
On Fri, Dec 22, 2023 at 12:26:19PM +0800, Guo Ren wrote: > On Fri, Dec 22, 2023 at 11:35 AM Leonardo Bras <leobras@redhat.com> wrote: > > > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > > directly. > > > > ok > > > > > > > > Cc: stable@vger.kernel.org > > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > --- > > > arch/riscv/include/asm/processor.h | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > index f19f861cda54..1f538fc4448d 100644 > > > --- a/arch/riscv/include/asm/processor.h > > > +++ b/arch/riscv/include/asm/processor.h > > > @@ -16,15 +16,13 @@ > > > > > > #ifdef CONFIG_64BIT > > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > > -#define STACK_TOP_MAX TASK_SIZE_64 > > > +#define STACK_TOP_MAX TASK_SIZE > > > > It means STACK_TOP_MAX will be in 64BIT: > > - TASK_SIZE_32 if compat_mode=y > > - TASK_SIZE_64 if compat_mode=n > > > > Makes sense for me. > > > > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > > ({ \ > > > unsigned long mmap_end; \ > > > typeof(addr) _addr = (addr); \ > > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > - mmap_end = STACK_TOP_MAX; \ > > > - else if ((_addr) >= VA_USER_SV57) \ > > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > mmap_end = STACK_TOP_MAX; \ > > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > > mmap_end = VA_USER_SV48; \ > > > > > > I don't think I got this change, or how it's connected to the commit msg. > The above is just code simplification; if STACK_TOP_MAX is TASK_SIZE, then > > if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > mmap_end = STACK_TOP_MAX; \ > else if ((_addr) >= VA_USER_SV57) \ > > is equal to: > > if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ I am failing to understand exactly how are they equal. I mean, what in your STACK_TOP_MAX change made them equal? See below, the behavior changed: > > > > > Before: > > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > Now: > > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > > if addr != 0. Is that desireable? > > (if not, above change is unneeded) > > ^ With your change on STACK_TOP_MAX only (not changing arch_get_mmap_end), you would have: - compat_mode & (0 < addr < 2^32) -> mmap_end = 2^32 - non-compat, addr == 0, or addr > 2^57 -> mmap_end = TASK_SIZE_64 - non-compat, (2^48 < addr < 2^57) -> mmap_end = 2^48 - non-compat, (0 < addr < 2^48) -> mmap_end = 2^39 Which seems more likely, based on Charlie comments. Thanks, Leo > > Also, unrelated to the change: > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > Is the above correct? > > It looks like it should be 2^57 instead, and a new if clause for > > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > > > Do I get it wrong? > Maybe I should move this into the optimization part. > > > > > (I will send an RFC 'fixing' the code the way I am whinking it should look > > like) > > > > Thanks, > > Leo > > > > > > > > > > > > > -- > > > 2.40.1 > > > > > > > > -- > Best Regards > Guo Ren >
On Fri, Dec 22, 2023 at 12:43 PM Leonardo Bras <leobras@redhat.com> wrote: > > On Fri, Dec 22, 2023 at 12:26:19PM +0800, Guo Ren wrote: > > On Fri, Dec 22, 2023 at 11:35 AM Leonardo Bras <leobras@redhat.com> wrote: > > > > > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > > > directly. > > > > > > ok > > > > > > > > > > > Cc: stable@vger.kernel.org > > > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > > --- > > > > arch/riscv/include/asm/processor.h | 6 ++---- > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > > index f19f861cda54..1f538fc4448d 100644 > > > > --- a/arch/riscv/include/asm/processor.h > > > > +++ b/arch/riscv/include/asm/processor.h > > > > @@ -16,15 +16,13 @@ > > > > > > > > #ifdef CONFIG_64BIT > > > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > > > -#define STACK_TOP_MAX TASK_SIZE_64 > > > > +#define STACK_TOP_MAX TASK_SIZE > > > > > > It means STACK_TOP_MAX will be in 64BIT: > > > - TASK_SIZE_32 if compat_mode=y > > > - TASK_SIZE_64 if compat_mode=n > > > > > > Makes sense for me. > > > > > > > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > > > ({ \ > > > > unsigned long mmap_end; \ > > > > typeof(addr) _addr = (addr); \ > > > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > > - mmap_end = STACK_TOP_MAX; \ > > > > - else if ((_addr) >= VA_USER_SV57) \ > > > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > > mmap_end = STACK_TOP_MAX; \ > > > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > > > mmap_end = VA_USER_SV48; \ > > > > > > > > > I don't think I got this change, or how it's connected to the commit msg. > > The above is just code simplification; if STACK_TOP_MAX is TASK_SIZE, then > > > > if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > mmap_end = STACK_TOP_MAX; \ > > else if ((_addr) >= VA_USER_SV57) \ > > > > is equal to: > > > > if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > I am failing to understand exactly how are they equal. > I mean, what in your STACK_TOP_MAX change made them equal? #define STACK_TOP_MAX TASK_SIZE #define TASK_SIZE (is_compat_task() ? TASK_SIZE_32 : TASK_SIZE_64) > > See below, the behavior changed: > > > > > > > > Before: > > > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > Now: > > > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > > > if addr != 0. Is that desireable? > > > (if not, above change is unneeded) > > > > > ^ > > With your change on STACK_TOP_MAX only (not changing arch_get_mmap_end), > you would have: > > - compat_mode & (0 < addr < 2^32) -> mmap_end = 2^32 compat_mode -> mmap_end = 2^32 > - non-compat, addr == 0, or addr > 2^57 -> mmap_end = TASK_SIZE_64 > - non-compat, (2^48 < addr < 2^57) -> mmap_end = 2^48 > - non-compat, (0 < addr < 2^48) -> mmap_end = 2^39 > > Which seems more likely, based on Charlie comments. > > Thanks, > Leo > > > > Also, unrelated to the change: > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > Is the above correct? > > > It looks like it should be 2^57 instead, and a new if clause for > > > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > > > > > Do I get it wrong? > > Maybe I should move this into the optimization part. > > > > > > > > (I will send an RFC 'fixing' the code the way I am whinking it should look > > > like) > > > > > > Thanks, > > > Leo > > > > > > > > > > > > > > > > > > > -- > > > > 2.40.1 > > > > > > > > > > > > > -- > > Best Regards > > Guo Ren > > >
On Fri, Dec 22, 2023 at 12:50:44PM +0800, Guo Ren wrote: > On Fri, Dec 22, 2023 at 12:43 PM Leonardo Bras <leobras@redhat.com> wrote: > > > > On Fri, Dec 22, 2023 at 12:26:19PM +0800, Guo Ren wrote: > > > On Fri, Dec 22, 2023 at 11:35 AM Leonardo Bras <leobras@redhat.com> wrote: > > > > > > > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > > > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > > > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > > > > directly. > > > > > > > > ok > > > > > > > > > > > > > > Cc: stable@vger.kernel.org > > > > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > > > --- > > > > > arch/riscv/include/asm/processor.h | 6 ++---- > > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > > > index f19f861cda54..1f538fc4448d 100644 > > > > > --- a/arch/riscv/include/asm/processor.h > > > > > +++ b/arch/riscv/include/asm/processor.h > > > > > @@ -16,15 +16,13 @@ > > > > > > > > > > #ifdef CONFIG_64BIT > > > > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > > > > -#define STACK_TOP_MAX TASK_SIZE_64 > > > > > +#define STACK_TOP_MAX TASK_SIZE > > > > > > > > It means STACK_TOP_MAX will be in 64BIT: > > > > - TASK_SIZE_32 if compat_mode=y > > > > - TASK_SIZE_64 if compat_mode=n > > > > > > > > Makes sense for me. > > > > > > > > > > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > > > > ({ \ > > > > > unsigned long mmap_end; \ > > > > > typeof(addr) _addr = (addr); \ > > > > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > > > - mmap_end = STACK_TOP_MAX; \ > > > > > - else if ((_addr) >= VA_USER_SV57) \ > > > > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > > > mmap_end = STACK_TOP_MAX; \ > > > > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > > > > mmap_end = VA_USER_SV48; \ > > > > > > > > > > > > I don't think I got this change, or how it's connected to the commit msg. > > > The above is just code simplification; if STACK_TOP_MAX is TASK_SIZE, then > > > > > > if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > mmap_end = STACK_TOP_MAX; \ > > > else if ((_addr) >= VA_USER_SV57) \ > > > > > > is equal to: > > > > > > if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > > I am failing to understand exactly how are they equal. > > I mean, what in your STACK_TOP_MAX change made them equal? > #define STACK_TOP_MAX TASK_SIZE > #define TASK_SIZE (is_compat_task() ? TASK_SIZE_32 : TASK_SIZE_64) > yes, I am aware. Let's do a simple test with the new code and addr = 2^27 (random 32-bit addr) and compat mode. if ((_addr) == 0 || (_addr) >= VA_USER_SV57) // Evaluates to false: 2^27 != 0, and is < 2^57 else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) // Evaluates to false: 2^27 < 2^48 else mmap_end = VA_USER_SV39; mmap_end = VA_USER_SV39, even in compat_mode. We need the extra is_compat_task() if we want to return 2^32. Thanks! Leo > > > > See below, the behavior changed: > > > > > > > > > > > Before: > > > > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > > > Now: > > > > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > > > > if addr != 0. Is that desireable? > > > > (if not, above change is unneeded) > > > > > > > > ^ > > > > With your change on STACK_TOP_MAX only (not changing arch_get_mmap_end), > > you would have: > > > > - compat_mode & (0 < addr < 2^32) -> mmap_end = 2^32 > compat_mode -> mmap_end = 2^32 > This is correct! Yeah, since you changed STACK_TOP_MAX to be 2^32 in compat mode, any addr value < 2^32 with compat value will return 2^32. (without the change in arch_get_mmap_end(), that is.) > > - non-compat, addr == 0, or addr > 2^57 -> mmap_end = TASK_SIZE_64 > > - non-compat, (2^48 < addr < 2^57) -> mmap_end = 2^48 > > - non-compat, (0 < addr < 2^48) -> mmap_end = 2^39 > > > > Which seems more likely, based on Charlie comments. > > > > Thanks, > > Leo > > > > > > Also, unrelated to the change: > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > Is the above correct? > > > > It looks like it should be 2^57 instead, and a new if clause for > > > > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > > > > > > > Do I get it wrong? > > > Maybe I should move this into the optimization part. > > > > > > > > > > > (I will send an RFC 'fixing' the code the way I am whinking it should look > > > > like) > > > > > > > > Thanks, > > > > Leo > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > 2.40.1 > > > > > > > > > > > > > > > > > > -- > > > Best Regards > > > Guo Ren > > > > > > > > -- > Best Regards > Guo Ren >
On Fri, Dec 22, 2023 at 01:23:29AM -0300, Leonardo Bras wrote: > On Thu, Dec 21, 2023 at 08:04:43PM -0800, Charlie Jenkins wrote: > > On Fri, Dec 22, 2023 at 12:34:56AM -0300, Leonardo Bras wrote: > > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > > > directly. > > > > > > ok > > > > > > > > > > > Cc: stable@vger.kernel.org > > > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > > --- > > > > arch/riscv/include/asm/processor.h | 6 ++---- > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > > index f19f861cda54..1f538fc4448d 100644 > > > > --- a/arch/riscv/include/asm/processor.h > > > > +++ b/arch/riscv/include/asm/processor.h > > > > @@ -16,15 +16,13 @@ > > > > > > > > #ifdef CONFIG_64BIT > > > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > > > -#define STACK_TOP_MAX TASK_SIZE_64 > > > > +#define STACK_TOP_MAX TASK_SIZE > > > > > > It means STACK_TOP_MAX will be in 64BIT: > > > - TASK_SIZE_32 if compat_mode=y > > > - TASK_SIZE_64 if compat_mode=n > > > > > > Makes sense for me. > > > > > > > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > > > ({ \ > > > > unsigned long mmap_end; \ > > > > typeof(addr) _addr = (addr); \ > > > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > > - mmap_end = STACK_TOP_MAX; \ > > > > - else if ((_addr) >= VA_USER_SV57) \ > > > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > > mmap_end = STACK_TOP_MAX; \ > > > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > > > mmap_end = VA_USER_SV48; \ > > > > > > > > > I don't think I got this change, or how it's connected to the commit msg. > > > > > > Before: > > > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > Now: > > > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > > > if addr != 0. Is that desireable? > > > (if not, above change is unneeded) > > > > I agree, this change does not make sense for compat mode. Compat mode > > should never return an address that is greater than 2^32, but this > > change allows that. > > > > > > > > Also, unrelated to the change: > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > Is the above correct? > > > It looks like it should be 2^57 instead, and a new if clause for > > > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > > > That is not the case. I documented this behavior and reasoning in > > Documentation/arch/riscv/vm-layout.rst in the "Userspace VAs" section. > > > > I can reiterate here though. The hint address to mmap (defined here as > > "addr") is the maximum userspace address that mmap should provide. What > > you are describing is a minimum. The purpose of this change was to allow > > applications that are not compatible with a larger virtual address (such > > as applications like Java that use the upper bits of the VA to store > > data) to have a consistent way of specifying how many bits they would > > like to be left free in the VA. This requires to take the next lowest > > address space to guaruntee that all of the most-significant bits left > > clear in hint address do not end up populated in the virtual address > > returned by mmap. > > > > - Charlie > > Hello Charlie, thank you for helping me understand! > > Ok, that does make sense now! The addr value hints "don't allocate > addr" > and thus: > > - 0 < addr < 2^48 : mmap_end = 2^39 > - 2^48 < addr < 2^57: mmap_end = 2^48 > > Ok, but then > - addr > 2^57: mmap_end = 2^57 > right? > > I mean, probably STACK_TOP_MAX in non-compat mode means 2^57 already, but > having it explicitly like: > > else if ((_addr) >= VA_USER_SV57) \ > mmap_end = VA_USER_SV57; \ > > would not be better for a future full 64-bit addressing? > (since it's already on a different if clause) I agree, that does make more sense. > > I could add comment on top of the macro with a short version on your addr > hint description above. Would that be ok? Sure :) - Charlie > > Thanks! > Leo > > > > > > > > > > > > > Do I get it wrong? > > > > > > (I will send an RFC 'fixing' the code the way I am whinking it should look > > > like) > > > > > > Thanks, > > > Leo > > > > > > > > > > > > > > > > > > > -- > > > > 2.40.1 > > > > > > > > > >
On Fri, Dec 22, 2023 at 1:28 PM Leonardo Bras <leobras@redhat.com> wrote: > > On Fri, Dec 22, 2023 at 12:50:44PM +0800, Guo Ren wrote: > > On Fri, Dec 22, 2023 at 12:43 PM Leonardo Bras <leobras@redhat.com> wrote: > > > > > > On Fri, Dec 22, 2023 at 12:26:19PM +0800, Guo Ren wrote: > > > > On Fri, Dec 22, 2023 at 11:35 AM Leonardo Bras <leobras@redhat.com> wrote: > > > > > > > > > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > > > > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > > > > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > > > > > directly. > > > > > > > > > > ok > > > > > > > > > > > > > > > > > Cc: stable@vger.kernel.org > > > > > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > > > > --- > > > > > > arch/riscv/include/asm/processor.h | 6 ++---- > > > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > > > > index f19f861cda54..1f538fc4448d 100644 > > > > > > --- a/arch/riscv/include/asm/processor.h > > > > > > +++ b/arch/riscv/include/asm/processor.h > > > > > > @@ -16,15 +16,13 @@ > > > > > > > > > > > > #ifdef CONFIG_64BIT > > > > > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > > > > > -#define STACK_TOP_MAX TASK_SIZE_64 > > > > > > +#define STACK_TOP_MAX TASK_SIZE > > > > > > > > > > It means STACK_TOP_MAX will be in 64BIT: > > > > > - TASK_SIZE_32 if compat_mode=y > > > > > - TASK_SIZE_64 if compat_mode=n > > > > > > > > > > Makes sense for me. > > > > > > > > > > > > > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > > > > > ({ \ > > > > > > unsigned long mmap_end; \ > > > > > > typeof(addr) _addr = (addr); \ > > > > > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > > > > - mmap_end = STACK_TOP_MAX; \ > > > > > > - else if ((_addr) >= VA_USER_SV57) \ > > > > > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > > > > mmap_end = STACK_TOP_MAX; \ > > > > > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > > > > > mmap_end = VA_USER_SV48; \ > > > > > > > > > > > > > > > I don't think I got this change, or how it's connected to the commit msg. > > > > The above is just code simplification; if STACK_TOP_MAX is TASK_SIZE, then > > > > > > > > if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > > mmap_end = STACK_TOP_MAX; \ > > > > else if ((_addr) >= VA_USER_SV57) \ > > > > > > > > is equal to: > > > > > > > > if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > > > > I am failing to understand exactly how are they equal. > > > I mean, what in your STACK_TOP_MAX change made them equal? > > #define STACK_TOP_MAX TASK_SIZE > > #define TASK_SIZE (is_compat_task() ? TASK_SIZE_32 : TASK_SIZE_64) > > > > yes, I am aware. Let's do a simple test with the new code and > addr = 2^27 (random 32-bit addr) and compat mode. > > if ((_addr) == 0 || (_addr) >= VA_USER_SV57) > // Evaluates to false: 2^27 != 0, and is < 2^57 > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) > // Evaluates to false: 2^27 < 2^48 > else > mmap_end = VA_USER_SV39; > > mmap_end = VA_USER_SV39, even in compat_mode. > > We need the extra is_compat_task() if we want to return 2^32. Yes, my stupid, I fell into the wrong logic. Sorry for the noisy part, which should be removed. > > Thanks! > Leo > > > > > > > > See below, the behavior changed: > > > > > > > > > > > > > > Before: > > > > > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > > > > > Now: > > > > > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > > > > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > > > > > if addr != 0. Is that desireable? > > > > > (if not, above change is unneeded) > > > > > > > > > > > ^ > > > > > > With your change on STACK_TOP_MAX only (not changing arch_get_mmap_end), > > > you would have: > > > > > > - compat_mode & (0 < addr < 2^32) -> mmap_end = 2^32 > > compat_mode -> mmap_end = 2^32 > > > > This is correct! > Yeah, since you changed STACK_TOP_MAX to be 2^32 in compat mode, > any addr value < 2^32 with compat value will return 2^32. > (without the change in arch_get_mmap_end(), that is.) > > > > - non-compat, addr == 0, or addr > 2^57 -> mmap_end = TASK_SIZE_64 > > > - non-compat, (2^48 < addr < 2^57) -> mmap_end = 2^48 > > > - non-compat, (0 < addr < 2^48) -> mmap_end = 2^39 > > > > > > Which seems more likely, based on Charlie comments. > > > > > > Thanks, > > > Leo > > > > > > > > Also, unrelated to the change: > > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > > Is the above correct? > > > > > It looks like it should be 2^57 instead, and a new if clause for > > > > > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > > > > > > > > > Do I get it wrong? > > > > Maybe I should move this into the optimization part. > > > > > > > > > > > > > > (I will send an RFC 'fixing' the code the way I am whinking it should look > > > > > like) > > > > > > > > > > Thanks, > > > > > Leo > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > 2.40.1 > > > > > > > > > > > > > > > > > > > > > > > -- > > > > Best Regards > > > > Guo Ren > > > > > > > > > > > > > -- > > Best Regards > > Guo Ren > > >
On Fri, Dec 22, 2023 at 03:20:15PM +0800, Guo Ren wrote: > On Fri, Dec 22, 2023 at 1:28 PM Leonardo Bras <leobras@redhat.com> wrote: > > > > On Fri, Dec 22, 2023 at 12:50:44PM +0800, Guo Ren wrote: > > > On Fri, Dec 22, 2023 at 12:43 PM Leonardo Bras <leobras@redhat.com> wrote: > > > > > > > > On Fri, Dec 22, 2023 at 12:26:19PM +0800, Guo Ren wrote: > > > > > On Fri, Dec 22, 2023 at 11:35 AM Leonardo Bras <leobras@redhat.com> wrote: > > > > > > > > > > > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > > > > > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > > > > > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > > > > > > directly. > > > > > > > > > > > > ok > > > > > > > > > > > > > > > > > > > > Cc: stable@vger.kernel.org > > > > > > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > > > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > > > > > --- > > > > > > > arch/riscv/include/asm/processor.h | 6 ++---- > > > > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > > > > > index f19f861cda54..1f538fc4448d 100644 > > > > > > > --- a/arch/riscv/include/asm/processor.h > > > > > > > +++ b/arch/riscv/include/asm/processor.h > > > > > > > @@ -16,15 +16,13 @@ > > > > > > > > > > > > > > #ifdef CONFIG_64BIT > > > > > > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > > > > > > -#define STACK_TOP_MAX TASK_SIZE_64 > > > > > > > +#define STACK_TOP_MAX TASK_SIZE > > > > > > > > > > > > It means STACK_TOP_MAX will be in 64BIT: > > > > > > - TASK_SIZE_32 if compat_mode=y > > > > > > - TASK_SIZE_64 if compat_mode=n > > > > > > > > > > > > Makes sense for me. > > > > > > > > > > > > > > > > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > > > > > > ({ \ > > > > > > > unsigned long mmap_end; \ > > > > > > > typeof(addr) _addr = (addr); \ > > > > > > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > > > > > - mmap_end = STACK_TOP_MAX; \ > > > > > > > - else if ((_addr) >= VA_USER_SV57) \ > > > > > > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > > > > > mmap_end = STACK_TOP_MAX; \ > > > > > > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > > > > > > mmap_end = VA_USER_SV48; \ > > > > > > > > > > > > > > > > > > I don't think I got this change, or how it's connected to the commit msg. > > > > > The above is just code simplification; if STACK_TOP_MAX is TASK_SIZE, then > > > > > > > > > > if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > > > mmap_end = STACK_TOP_MAX; \ > > > > > else if ((_addr) >= VA_USER_SV57) \ > > > > > > > > > > is equal to: > > > > > > > > > > if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > > > > > > I am failing to understand exactly how are they equal. > > > > I mean, what in your STACK_TOP_MAX change made them equal? > > > #define STACK_TOP_MAX TASK_SIZE > > > #define TASK_SIZE (is_compat_task() ? TASK_SIZE_32 : TASK_SIZE_64) > > > > > > > yes, I am aware. Let's do a simple test with the new code and > > addr = 2^27 (random 32-bit addr) and compat mode. > > > > if ((_addr) == 0 || (_addr) >= VA_USER_SV57) > > // Evaluates to false: 2^27 != 0, and is < 2^57 > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) > > // Evaluates to false: 2^27 < 2^48 > > else > > mmap_end = VA_USER_SV39; > > > > mmap_end = VA_USER_SV39, even in compat_mode. > > > > We need the extra is_compat_task() if we want to return 2^32. > Yes, my stupid, I fell into the wrong logic. Sorry for the noisy part, > which should be removed. Don't worry, I also do stuff like this when I am too focused in the issue :) Thanks! Leo > > > > > Thanks! > > Leo > > > > > > > > > > > > See below, the behavior changed: > > > > > > > > > > > > > > > > > Before: > > > > > > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > > > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > > > > > > > Now: > > > > > > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > > > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > > > > > > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > > > > > > if addr != 0. Is that desireable? > > > > > > (if not, above change is unneeded) > > > > > > > > > > > > > > ^ > > > > > > > > With your change on STACK_TOP_MAX only (not changing arch_get_mmap_end), > > > > you would have: > > > > > > > > - compat_mode & (0 < addr < 2^32) -> mmap_end = 2^32 > > > compat_mode -> mmap_end = 2^32 > > > > > > > This is correct! > > Yeah, since you changed STACK_TOP_MAX to be 2^32 in compat mode, > > any addr value < 2^32 with compat value will return 2^32. > > (without the change in arch_get_mmap_end(), that is.) > > > > > > - non-compat, addr == 0, or addr > 2^57 -> mmap_end = TASK_SIZE_64 > > > > - non-compat, (2^48 < addr < 2^57) -> mmap_end = 2^48 > > > > - non-compat, (0 < addr < 2^48) -> mmap_end = 2^39 > > > > > > > > Which seems more likely, based on Charlie comments. > > > > > > > > Thanks, > > > > Leo > > > > > > > > > > Also, unrelated to the change: > > > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > > > Is the above correct? > > > > > > It looks like it should be 2^57 instead, and a new if clause for > > > > > > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > > > > > > > > > > > Do I get it wrong? > > > > > Maybe I should move this into the optimization part. > > > > > > > > > > > > > > > > > (I will send an RFC 'fixing' the code the way I am whinking it should look > > > > > > like) > > > > > > > > > > > > Thanks, > > > > > > Leo > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > 2.40.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best Regards > > > > > Guo Ren > > > > > > > > > > > > > > > > > > -- > > > Best Regards > > > Guo Ren > > > > > > > > -- > Best Regards > Guo Ren >
On Thu, Dec 21, 2023 at 09:42:05PM -0800, Charlie Jenkins wrote: > On Fri, Dec 22, 2023 at 01:23:29AM -0300, Leonardo Bras wrote: > > On Thu, Dec 21, 2023 at 08:04:43PM -0800, Charlie Jenkins wrote: > > > On Fri, Dec 22, 2023 at 12:34:56AM -0300, Leonardo Bras wrote: > > > > On Thu, Dec 21, 2023 at 10:46:59AM -0500, guoren@kernel.org wrote: > > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > > > > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > > > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > > > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > > > > directly. > > > > > > > > ok > > > > > > > > > > > > > > Cc: stable@vger.kernel.org > > > > > Fixes: add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") > > > > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com> > > > > > Signed-off-by: Guo Ren <guoren@kernel.org> > > > > > --- > > > > > arch/riscv/include/asm/processor.h | 6 ++---- > > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h > > > > > index f19f861cda54..1f538fc4448d 100644 > > > > > --- a/arch/riscv/include/asm/processor.h > > > > > +++ b/arch/riscv/include/asm/processor.h > > > > > @@ -16,15 +16,13 @@ > > > > > > > > > > #ifdef CONFIG_64BIT > > > > > #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) > > > > > -#define STACK_TOP_MAX TASK_SIZE_64 > > > > > +#define STACK_TOP_MAX TASK_SIZE > > > > > > > > It means STACK_TOP_MAX will be in 64BIT: > > > > - TASK_SIZE_32 if compat_mode=y > > > > - TASK_SIZE_64 if compat_mode=n > > > > > > > > Makes sense for me. > > > > > > > > > > > > > > #define arch_get_mmap_end(addr, len, flags) \ > > > > > ({ \ > > > > > unsigned long mmap_end; \ > > > > > typeof(addr) _addr = (addr); \ > > > > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ > > > > > - mmap_end = STACK_TOP_MAX; \ > > > > > - else if ((_addr) >= VA_USER_SV57) \ > > > > > + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ > > > > > mmap_end = STACK_TOP_MAX; \ > > > > > else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ > > > > > mmap_end = VA_USER_SV48; \ > > > > > > > > > > > > I don't think I got this change, or how it's connected to the commit msg. > > > > > > > > Before: > > > > - addr == 0, or addr > 2^57, or compat: mmap_end = STACK_TOP_MAX > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > > > Now: > > > > - addr == 0, or addr > 2^57: mmap_end = STACK_TOP_MAX > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > > > > > > > IIUC compat mode addr will be < 2^32, so will always have mmap_end = 2^39 > > > > if addr != 0. Is that desireable? > > > > (if not, above change is unneeded) > > > > > > I agree, this change does not make sense for compat mode. Compat mode > > > should never return an address that is greater than 2^32, but this > > > change allows that. > > > > > > > > > > > Also, unrelated to the change: > > > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > Is the above correct? > > > > It looks like it should be 2^57 instead, and a new if clause for > > > > 2^32 < addr < 2^48 should have mmap_end = 2^48. > > > > > > That is not the case. I documented this behavior and reasoning in > > > Documentation/arch/riscv/vm-layout.rst in the "Userspace VAs" section. > > > > > > I can reiterate here though. The hint address to mmap (defined here as > > > "addr") is the maximum userspace address that mmap should provide. What > > > you are describing is a minimum. The purpose of this change was to allow > > > applications that are not compatible with a larger virtual address (such > > > as applications like Java that use the upper bits of the VA to store > > > data) to have a consistent way of specifying how many bits they would > > > like to be left free in the VA. This requires to take the next lowest > > > address space to guaruntee that all of the most-significant bits left > > > clear in hint address do not end up populated in the virtual address > > > returned by mmap. > > > > > > - Charlie > > > > Hello Charlie, thank you for helping me understand! > > > > Ok, that does make sense now! The addr value hints "don't allocate > addr" > > and thus: > > > > - 0 < addr < 2^48 : mmap_end = 2^39 > > - 2^48 < addr < 2^57: mmap_end = 2^48 > > > > Ok, but then > > - addr > 2^57: mmap_end = 2^57 > > right? > > > > I mean, probably STACK_TOP_MAX in non-compat mode means 2^57 already, but > > having it explicitly like: > > > > else if ((_addr) >= VA_USER_SV57) \ > > mmap_end = VA_USER_SV57; \ > > > > would not be better for a future full 64-bit addressing? > > (since it's already on a different if clause) > > I agree, that does make more sense. > > > > > I could add comment on top of the macro with a short version on your addr > > hint description above. Would that be ok? > > Sure :) Sent, thanks! Leo > > - Charlie > > > > > Thanks! > > Leo > > > > > > > > > > > > > > > > > > > > > Do I get it wrong? > > > > > > > > (I will send an RFC 'fixing' the code the way I am whinking it should look > > > > like) > > > > > > > > Thanks, > > > > Leo > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > 2.40.1 > > > > > > > > > > > > > > >
From: guoren@kernel.org <guoren@kernel.org> > Sent: 21 December 2023 15:47 > > From: Guo Ren <guoren@linux.alibaba.com> > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > directly. Why 2G ? IIRC for 32-bit native x86 the limit is 3G, but in compat mode it is (just under) 4G. There is a special mmap option (for programs like wine) to limit mmap() to 2G. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Dec 22, 2023 at 5:00 PM David Laight <David.Laight@aculab.com> wrote: > > From: guoren@kernel.org <guoren@kernel.org> > > Sent: 21 December 2023 15:47 > > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > When the task is in COMPAT mode, the arch_get_mmap_end should be 2GB, > > not TASK_SIZE_64. The TASK_SIZE has contained is_compat_mode() > > detection, so change the definition of STACK_TOP_MAX to TASK_SIZE > > directly. > > Why 2G ? > > IIRC for 32-bit native x86 the limit is 3G, but in compat mode > it is (just under) 4G. > > There is a special mmap option (for programs like wine) to > limit mmap() to 2G. The 2G address space seems enough for a small memory scenario, and I agree the compat mode could support 4G, but it should be another feature. We limited our rv32 applications to under 2GB because we want to leave more address space for the kernel side (Our s64ilp32 kernel needs vmmap stack, kasan ...). > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h index f19f861cda54..1f538fc4448d 100644 --- a/arch/riscv/include/asm/processor.h +++ b/arch/riscv/include/asm/processor.h @@ -16,15 +16,13 @@ #ifdef CONFIG_64BIT #define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1)) -#define STACK_TOP_MAX TASK_SIZE_64 +#define STACK_TOP_MAX TASK_SIZE #define arch_get_mmap_end(addr, len, flags) \ ({ \ unsigned long mmap_end; \ typeof(addr) _addr = (addr); \ - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ - mmap_end = STACK_TOP_MAX; \ - else if ((_addr) >= VA_USER_SV57) \ + if ((_addr) == 0 || (_addr) >= VA_USER_SV57) \ mmap_end = STACK_TOP_MAX; \ else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >= VA_BITS_SV48)) \ mmap_end = VA_USER_SV48; \