[PATCH V2 0/4] riscv: mm: Fixup & Optimize COMPAT code

[PATCH V2 0/4] riscv: mm: Fixup & Optimize COMPAT code

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

When the task is in COMPAT mode, the TASK_SIZE should be 2GB, so
STACK_TOP_MAX and arch_get_mmap_end must be limited to 2 GB. This series
fixes the problem made by commit: add2cc6b6515 ("RISC-V: mm: Restrict
address space for sv39,sv48,sv57") and optimizes the related coding
convention of TASK_SIZE.

Changelog:
v2:
 - Separate rename from fixup
 - Add STACK_TOP_MAX fixup for compat
 - Add Cleanup & rename patches

v1:
https://lore.kernel.org/linux-riscv/20231219111701.1886903-1-guoren@kernel.org/

Guo Ren (4):
  riscv: mm: Fixup compat mode boot failure
  riscv: mm: Fixup compat arch_get_mmap_end
  riscv: mm: Remove unused TASK_SIZE_MIN
  riscv: mm: Optimize TASK_SIZE definition

 arch/riscv/include/asm/pgtable.h   | 9 ++++-----
 arch/riscv/include/asm/processor.h | 6 ++----
 2 files changed, 6 insertions(+), 9 deletions(-)

-- 
2.40.1

[PATCH V2 0/4] riscv: mm: Fixup & Optimize COMPAT code

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

When the task is in COMPAT mode, the TASK_SIZE should be 2GB, so
STACK_TOP_MAX and arch_get_mmap_end must be limited to 2 GB. This series
fixes the problem made by commit: add2cc6b6515 ("RISC-V: mm: Restrict
address space for sv39,sv48,sv57") and optimizes the related coding
convention of TASK_SIZE.

Changelog:
v2:
 - Separate rename from fixup
 - Add STACK_TOP_MAX fixup for compat
 - Add Cleanup & rename patches

v1:
https://lore.kernel.org/linux-riscv/20231219111701.1886903-1-guoren@kernel.org/

Guo Ren (4):
  riscv: mm: Fixup compat mode boot failure
  riscv: mm: Fixup compat arch_get_mmap_end
  riscv: mm: Remove unused TASK_SIZE_MIN
  riscv: mm: Optimize TASK_SIZE definition

 arch/riscv/include/asm/pgtable.h   | 9 ++++-----
 arch/riscv/include/asm/processor.h | 6 ++----
 2 files changed, 6 insertions(+), 9 deletions(-)

-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
segment fault. Sometimes, it would cause boot failure when the whole
rootfs is rv32.

Freeing unused kernel image (initmem) memory: 2236K
Run /sbin/init as init process
Starting init: /sbin/init exists but couldn't execute it (error -14)
Run /etc/init as init process
...

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/pgtable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index ab00235b018f..74ffb2178f54 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
 #define TASK_SIZE_MIN	(PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
 
 #ifdef CONFIG_COMPAT
-#define TASK_SIZE_32	(_AC(0x80000000, UL) - PAGE_SIZE)
+#define TASK_SIZE_32	(_AC(0x80000000, UL))
 #define TASK_SIZE	(test_thread_flag(TIF_32BIT) ? \
 			 TASK_SIZE_32 : TASK_SIZE_64)
 #else
-- 
2.40.1

[PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
segment fault. Sometimes, it would cause boot failure when the whole
rootfs is rv32.

Freeing unused kernel image (initmem) memory: 2236K
Run /sbin/init as init process
Starting init: /sbin/init exists but couldn't execute it (error -14)
Run /etc/init as init process
...

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/pgtable.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index ab00235b018f..74ffb2178f54 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
 #define TASK_SIZE_MIN	(PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
 
 #ifdef CONFIG_COMPAT
-#define TASK_SIZE_32	(_AC(0x80000000, UL) - PAGE_SIZE)
+#define TASK_SIZE_32	(_AC(0x80000000, UL))
 #define TASK_SIZE	(test_thread_flag(TIF_32BIT) ? \
 			 TASK_SIZE_32 : TASK_SIZE_64)
 #else
-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[guoren]

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(-)

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;			\
-- 
2.40.1

[PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[guoren]

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(-)

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;			\
-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH V2 3/4] riscv: mm: Remove unused TASK_SIZE_MIN

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

Remove TASK_SIZE_MIN because it's not used anymore.

Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
 arch/riscv/include/asm/pgtable.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 74ffb2178f54..e415582276ec 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -878,7 +878,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
  */
 #ifdef CONFIG_64BIT
 #define TASK_SIZE_64	(PGDIR_SIZE * PTRS_PER_PGD / 2)
-#define TASK_SIZE_MIN	(PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
 
 #ifdef CONFIG_COMPAT
 #define TASK_SIZE_32	(_AC(0x80000000, UL))
@@ -890,7 +889,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
 
 #else
 #define TASK_SIZE	FIXADDR_START
-#define TASK_SIZE_MIN	TASK_SIZE
 #endif
 
 #else /* CONFIG_MMU */
-- 
2.40.1

[PATCH V2 3/4] riscv: mm: Remove unused TASK_SIZE_MIN

[guoren]

From: Guo Ren <guoren@linux.alibaba.com>

Remove TASK_SIZE_MIN because it's not used anymore.

Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
 arch/riscv/include/asm/pgtable.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 74ffb2178f54..e415582276ec 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -878,7 +878,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
  */
 #ifdef CONFIG_64BIT
 #define TASK_SIZE_64	(PGDIR_SIZE * PTRS_PER_PGD / 2)
-#define TASK_SIZE_MIN	(PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
 
 #ifdef CONFIG_COMPAT
 #define TASK_SIZE_32	(_AC(0x80000000, UL))
@@ -890,7 +889,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
 
 #else
 #define TASK_SIZE	FIXADDR_START
-#define TASK_SIZE_MIN	TASK_SIZE
 #endif
 
 #else /* CONFIG_MMU */
-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[guoren]

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))
 
 #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
-- 
2.40.1

[PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[guoren]

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))
 
 #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
-- 
2.40.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Leonardo Bras]

Hello Guo Ren,

On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> segment fault. Sometimes, it would cause boot failure when the whole
> rootfs is rv32.

Checking if I get the scenario:

In pgtable.h:
#ifdef CONFIG_64BIT
#define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
#define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)

#ifdef CONFIG_COMPAT
#define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
#define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
                         TASK_SIZE_32 : TASK_SIZE_64)
#else
[...]

Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in 
compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.

from processor.h:
#ifdef CONFIG_64BIT
#define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
#define STACK_TOP_MAX           TASK_SIZE_64
[...]
#define STACK_TOP               DEFAULT_MAP_WINDOW


where:
#define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
with MMAP_VA_BITS_64 being either 48 or 37.

In compat mode,
STACK_TOP = 1 << (32 - 1) 	-> 0x80000000
TASK_SIZE = 0x8000000 - 4k 	-> 0x7ffff000

IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.

Then why not:
#ifdef CONFIG_COMPAT
#define TASK_SIZE_32    STACK_TOP

With some comments explaining why there is no need to reserve a PAGE_SIZE 
in the TASK_SIZE_32.

Does that make sense?

Thanks!
Leo

> 
> Freeing unused kernel image (initmem) memory: 2236K
> Run /sbin/init as init process
> Starting init: /sbin/init exists but couldn't execute it (error -14)
> Run /etc/init as init process
> ...
> 
> 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/pgtable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index ab00235b018f..74ffb2178f54 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
>  #define TASK_SIZE_MIN	(PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
>  
>  #ifdef CONFIG_COMPAT
> -#define TASK_SIZE_32	(_AC(0x80000000, UL) - PAGE_SIZE)
> +#define TASK_SIZE_32	(_AC(0x80000000, UL))




>  #define TASK_SIZE	(test_thread_flag(TIF_32BIT) ? \
>  			 TASK_SIZE_32 : TASK_SIZE_64)
>  #else
> -- 
> 2.40.1
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Leonardo Bras]

Hello Guo Ren,

On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> segment fault. Sometimes, it would cause boot failure when the whole
> rootfs is rv32.

Checking if I get the scenario:

In pgtable.h:
#ifdef CONFIG_64BIT
#define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
#define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)

#ifdef CONFIG_COMPAT
#define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
#define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
                         TASK_SIZE_32 : TASK_SIZE_64)
#else
[...]

Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in 
compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.

from processor.h:
#ifdef CONFIG_64BIT
#define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
#define STACK_TOP_MAX           TASK_SIZE_64
[...]
#define STACK_TOP               DEFAULT_MAP_WINDOW


where:
#define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
with MMAP_VA_BITS_64 being either 48 or 37.

In compat mode,
STACK_TOP = 1 << (32 - 1) 	-> 0x80000000
TASK_SIZE = 0x8000000 - 4k 	-> 0x7ffff000

IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.

Then why not:
#ifdef CONFIG_COMPAT
#define TASK_SIZE_32    STACK_TOP

With some comments explaining why there is no need to reserve a PAGE_SIZE 
in the TASK_SIZE_32.

Does that make sense?

Thanks!
Leo

> 
> Freeing unused kernel image (initmem) memory: 2236K
> Run /sbin/init as init process
> Starting init: /sbin/init exists but couldn't execute it (error -14)
> Run /etc/init as init process
> ...
> 
> 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/pgtable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index ab00235b018f..74ffb2178f54 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
>  #define TASK_SIZE_MIN	(PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
>  
>  #ifdef CONFIG_COMPAT
> -#define TASK_SIZE_32	(_AC(0x80000000, UL) - PAGE_SIZE)
> +#define TASK_SIZE_32	(_AC(0x80000000, UL))




>  #define TASK_SIZE	(test_thread_flag(TIF_32BIT) ? \
>  			 TASK_SIZE_32 : TASK_SIZE_64)
>  #else
> -- 
> 2.40.1
> 

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Guo Ren]

On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
>
> Hello Guo Ren,
>
> On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > segment fault. Sometimes, it would cause boot failure when the whole
> > rootfs is rv32.
>
> Checking if I get the scenario:
>
> In pgtable.h:
> #ifdef CONFIG_64BIT
> #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
>
> #ifdef CONFIG_COMPAT
> #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
>                          TASK_SIZE_32 : TASK_SIZE_64)
> #else
> [...]
>
> Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
>
> from processor.h:
> #ifdef CONFIG_64BIT
> #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> #define STACK_TOP_MAX           TASK_SIZE_64
> [...]
> #define STACK_TOP               DEFAULT_MAP_WINDOW
>
>
> where:
> #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> with MMAP_VA_BITS_64 being either 48 or 37.
>
> In compat mode,
> STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
>
> IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
Yes, it causes the problem, which causes the boot to fail.

>
> Then why not:
> #ifdef CONFIG_COMPAT
> #define TASK_SIZE_32    STACK_TOP
Yes, it's the solution that I think at first. But I didn't find any
problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
unify it with Sv39 and Sv48.

>
> With some comments explaining why there is no need to reserve a PAGE_SIZE
> in the TASK_SIZE_32.
At first, I wanted to put a invalid page between the user & kernel
space, but it seems useless.

>
> Does that make sense?
>
> Thanks!
> Leo
>
> >
> > Freeing unused kernel image (initmem) memory: 2236K
> > Run /sbin/init as init process
> > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > Run /etc/init as init process
> > ...
> >
> > 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/pgtable.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index ab00235b018f..74ffb2178f54 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> >
> >  #ifdef CONFIG_COMPAT
> > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
>
>
>
>
> >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> >                        TASK_SIZE_32 : TASK_SIZE_64)
> >  #else
> > --
> > 2.40.1
> >
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Guo Ren]

On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
>
> Hello Guo Ren,
>
> On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > segment fault. Sometimes, it would cause boot failure when the whole
> > rootfs is rv32.
>
> Checking if I get the scenario:
>
> In pgtable.h:
> #ifdef CONFIG_64BIT
> #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
>
> #ifdef CONFIG_COMPAT
> #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
>                          TASK_SIZE_32 : TASK_SIZE_64)
> #else
> [...]
>
> Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
>
> from processor.h:
> #ifdef CONFIG_64BIT
> #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> #define STACK_TOP_MAX           TASK_SIZE_64
> [...]
> #define STACK_TOP               DEFAULT_MAP_WINDOW
>
>
> where:
> #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> with MMAP_VA_BITS_64 being either 48 or 37.
>
> In compat mode,
> STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
>
> IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
Yes, it causes the problem, which causes the boot to fail.

>
> Then why not:
> #ifdef CONFIG_COMPAT
> #define TASK_SIZE_32    STACK_TOP
Yes, it's the solution that I think at first. But I didn't find any
problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
unify it with Sv39 and Sv48.

>
> With some comments explaining why there is no need to reserve a PAGE_SIZE
> in the TASK_SIZE_32.
At first, I wanted to put a invalid page between the user & kernel
space, but it seems useless.

>
> Does that make sense?
>
> Thanks!
> Leo
>
> >
> > Freeing unused kernel image (initmem) memory: 2236K
> > Run /sbin/init as init process
> > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > Run /etc/init as init process
> > ...
> >
> > 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/pgtable.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index ab00235b018f..74ffb2178f54 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> >
> >  #ifdef CONFIG_COMPAT
> > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
>
>
>
>
> >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> >                        TASK_SIZE_32 : TASK_SIZE_64)
> >  #else
> > --
> > 2.40.1
> >
>


-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> 

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Charlie Jenkins]

On Fri, Dec 22, 2023 at 10:57:16AM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
> >
> > Hello Guo Ren,
> >
> > On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > > segment fault. Sometimes, it would cause boot failure when the whole
> > > rootfs is rv32.
> >
> > Checking if I get the scenario:
> >
> > In pgtable.h:
> > #ifdef CONFIG_64BIT
> > #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> >
> > #ifdef CONFIG_COMPAT
> > #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> > #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
> >                          TASK_SIZE_32 : TASK_SIZE_64)
> > #else
> > [...]
> >
> > Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> > compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
> >
> > from processor.h:
> > #ifdef CONFIG_64BIT
> > #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> > #define STACK_TOP_MAX           TASK_SIZE_64
> > [...]
> > #define STACK_TOP               DEFAULT_MAP_WINDOW
> >
> >
> > where:
> > #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> > with MMAP_VA_BITS_64 being either 48 or 37.
> >
> > In compat mode,
> > STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> > TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
> >
> > IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
> Yes, it causes the problem, which causes the boot to fail.

I think what Leonardo is getting at is that it is odd that it would
cause boot issues if TASK_SIZE is not equal STACK_TOP. This seems
indicative of a different problem. While this may fix the issue, it
should be valid for TASK_SIZE to be less than STACK_TOP.

- Charlie

> 
> >
> > Then why not:
> > #ifdef CONFIG_COMPAT
> > #define TASK_SIZE_32    STACK_TOP
> Yes, it's the solution that I think at first. But I didn't find any
> problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
> unify it with Sv39 and Sv48.
> 
> >
> > With some comments explaining why there is no need to reserve a PAGE_SIZE
> > in the TASK_SIZE_32.
> At first, I wanted to put a invalid page between the user & kernel
> space, but it seems useless.
> 
> >
> > Does that make sense?
> >
> > Thanks!
> > Leo
> >
> > >
> > > Freeing unused kernel image (initmem) memory: 2236K
> > > Run /sbin/init as init process
> > > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > > Run /etc/init as init process
> > > ...
> > >
> > > 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/pgtable.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > index ab00235b018f..74ffb2178f54 100644
> > > --- a/arch/riscv/include/asm/pgtable.h
> > > +++ b/arch/riscv/include/asm/pgtable.h
> > > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > >
> > >  #ifdef CONFIG_COMPAT
> > > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
> >
> >
> >
> >
> > >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> > >                        TASK_SIZE_32 : TASK_SIZE_64)
> > >  #else
> > > --
> > > 2.40.1
> > >
> >
> 
> 
> -- 
> Best Regards
>  Guo Ren

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Charlie Jenkins]

On Fri, Dec 22, 2023 at 10:57:16AM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
> >
> > Hello Guo Ren,
> >
> > On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > > segment fault. Sometimes, it would cause boot failure when the whole
> > > rootfs is rv32.
> >
> > Checking if I get the scenario:
> >
> > In pgtable.h:
> > #ifdef CONFIG_64BIT
> > #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> >
> > #ifdef CONFIG_COMPAT
> > #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> > #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
> >                          TASK_SIZE_32 : TASK_SIZE_64)
> > #else
> > [...]
> >
> > Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> > compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
> >
> > from processor.h:
> > #ifdef CONFIG_64BIT
> > #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> > #define STACK_TOP_MAX           TASK_SIZE_64
> > [...]
> > #define STACK_TOP               DEFAULT_MAP_WINDOW
> >
> >
> > where:
> > #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> > with MMAP_VA_BITS_64 being either 48 or 37.
> >
> > In compat mode,
> > STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> > TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
> >
> > IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
> Yes, it causes the problem, which causes the boot to fail.

I think what Leonardo is getting at is that it is odd that it would
cause boot issues if TASK_SIZE is not equal STACK_TOP. This seems
indicative of a different problem. While this may fix the issue, it
should be valid for TASK_SIZE to be less than STACK_TOP.

- Charlie

> 
> >
> > Then why not:
> > #ifdef CONFIG_COMPAT
> > #define TASK_SIZE_32    STACK_TOP
> Yes, it's the solution that I think at first. But I didn't find any
> problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
> unify it with Sv39 and Sv48.
> 
> >
> > With some comments explaining why there is no need to reserve a PAGE_SIZE
> > in the TASK_SIZE_32.
> At first, I wanted to put a invalid page between the user & kernel
> space, but it seems useless.
> 
> >
> > Does that make sense?
> >
> > Thanks!
> > Leo
> >
> > >
> > > Freeing unused kernel image (initmem) memory: 2236K
> > > Run /sbin/init as init process
> > > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > > Run /etc/init as init process
> > > ...
> > >
> > > 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/pgtable.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > index ab00235b018f..74ffb2178f54 100644
> > > --- a/arch/riscv/include/asm/pgtable.h
> > > +++ b/arch/riscv/include/asm/pgtable.h
> > > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > >
> > >  #ifdef CONFIG_COMPAT
> > > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
> >
> >
> >
> >
> > >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> > >                        TASK_SIZE_32 : TASK_SIZE_64)
> > >  #else
> > > --
> > > 2.40.1
> > >
> >
> 
> 
> -- 
> Best Regards
>  Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Charlie Jenkins]

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
> > 
> 

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Charlie Jenkins]

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
> > 
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> > > 
> > 
> 

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> > > 
> > 
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Guo Ren]

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
> >
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Guo Ren]

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
> >
>


-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Leonardo Bras]

On Thu, Dec 21, 2023 at 07:50:27PM -0800, Charlie Jenkins wrote:
> On Fri, Dec 22, 2023 at 10:57:16AM +0800, Guo Ren wrote:
> > On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
> > >
> > > Hello Guo Ren,
> > >
> > > On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > >
> > > > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > > > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > > > segment fault. Sometimes, it would cause boot failure when the whole
> > > > rootfs is rv32.
> > >
> > > Checking if I get the scenario:
> > >
> > > In pgtable.h:
> > > #ifdef CONFIG_64BIT
> > > #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > > #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > >
> > > #ifdef CONFIG_COMPAT
> > > #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> > > #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
> > >                          TASK_SIZE_32 : TASK_SIZE_64)
> > > #else
> > > [...]
> > >
> > > Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> > > compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
> > >
> > > from processor.h:
> > > #ifdef CONFIG_64BIT
> > > #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> > > #define STACK_TOP_MAX           TASK_SIZE_64
> > > [...]
> > > #define STACK_TOP               DEFAULT_MAP_WINDOW
> > >
> > >
> > > where:
> > > #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> > > with MMAP_VA_BITS_64 being either 48 or 37.
> > >
> > > In compat mode,
> > > STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> > > TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
> > >
> > > IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
> > Yes, it causes the problem, which causes the boot to fail.
> 
> I think what Leonardo is getting at is that it is odd that it would
> cause boot issues if TASK_SIZE is not equal STACK_TOP. This seems
> indicative of a different problem. While this may fix the issue, it
> should be valid for TASK_SIZE to be less than STACK_TOP.
> 
> - Charlie
> 

That is also a good point, but I am not that acquainted to this to 
actually propose this. 

I was thinking more on these questions:
Is TASK_SIZE and STACK_TOP related somehow?
If so, would not be better to describe one in terms of the other, like
#define TASK_SIZE (STACK_TOP - PAGE_SIZE)

Or the other way around.

I mean, if they have any relation it would be much easier to represent them 
that way, and it would avoid having two magical numbers.

Thanks!
Leo

> > 
> > >
> > > Then why not:
> > > #ifdef CONFIG_COMPAT
> > > #define TASK_SIZE_32    STACK_TOP
> > Yes, it's the solution that I think at first. But I didn't find any
> > problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
> > unify it with Sv39 and Sv48.
> > 
> > >
> > > With some comments explaining why there is no need to reserve a PAGE_SIZE
> > > in the TASK_SIZE_32.
> > At first, I wanted to put a invalid page between the user & kernel
> > space, but it seems useless.
> > 
> > >
> > > Does that make sense?
> > >
> > > Thanks!
> > > Leo
> > >
> > > >
> > > > Freeing unused kernel image (initmem) memory: 2236K
> > > > Run /sbin/init as init process
> > > > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > > > Run /etc/init as init process
> > > > ...
> > > >
> > > > 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/pgtable.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > > index ab00235b018f..74ffb2178f54 100644
> > > > --- a/arch/riscv/include/asm/pgtable.h
> > > > +++ b/arch/riscv/include/asm/pgtable.h
> > > > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > > >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > > >
> > > >  #ifdef CONFIG_COMPAT
> > > > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > > > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
> > >
> > >
> > >
> > >
> > > >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> > > >                        TASK_SIZE_32 : TASK_SIZE_64)
> > > >  #else
> > > > --
> > > > 2.40.1
> > > >
> > >
> > 
> > 
> > -- 
> > Best Regards
> >  Guo Ren
> 

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Leonardo Bras]

On Thu, Dec 21, 2023 at 07:50:27PM -0800, Charlie Jenkins wrote:
> On Fri, Dec 22, 2023 at 10:57:16AM +0800, Guo Ren wrote:
> > On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
> > >
> > > Hello Guo Ren,
> > >
> > > On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > >
> > > > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > > > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > > > segment fault. Sometimes, it would cause boot failure when the whole
> > > > rootfs is rv32.
> > >
> > > Checking if I get the scenario:
> > >
> > > In pgtable.h:
> > > #ifdef CONFIG_64BIT
> > > #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > > #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > >
> > > #ifdef CONFIG_COMPAT
> > > #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> > > #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
> > >                          TASK_SIZE_32 : TASK_SIZE_64)
> > > #else
> > > [...]
> > >
> > > Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> > > compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
> > >
> > > from processor.h:
> > > #ifdef CONFIG_64BIT
> > > #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> > > #define STACK_TOP_MAX           TASK_SIZE_64
> > > [...]
> > > #define STACK_TOP               DEFAULT_MAP_WINDOW
> > >
> > >
> > > where:
> > > #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> > > with MMAP_VA_BITS_64 being either 48 or 37.
> > >
> > > In compat mode,
> > > STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> > > TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
> > >
> > > IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
> > Yes, it causes the problem, which causes the boot to fail.
> 
> I think what Leonardo is getting at is that it is odd that it would
> cause boot issues if TASK_SIZE is not equal STACK_TOP. This seems
> indicative of a different problem. While this may fix the issue, it
> should be valid for TASK_SIZE to be less than STACK_TOP.
> 
> - Charlie
> 

That is also a good point, but I am not that acquainted to this to 
actually propose this. 

I was thinking more on these questions:
Is TASK_SIZE and STACK_TOP related somehow?
If so, would not be better to describe one in terms of the other, like
#define TASK_SIZE (STACK_TOP - PAGE_SIZE)

Or the other way around.

I mean, if they have any relation it would be much easier to represent them 
that way, and it would avoid having two magical numbers.

Thanks!
Leo

> > 
> > >
> > > Then why not:
> > > #ifdef CONFIG_COMPAT
> > > #define TASK_SIZE_32    STACK_TOP
> > Yes, it's the solution that I think at first. But I didn't find any
> > problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
> > unify it with Sv39 and Sv48.
> > 
> > >
> > > With some comments explaining why there is no need to reserve a PAGE_SIZE
> > > in the TASK_SIZE_32.
> > At first, I wanted to put a invalid page between the user & kernel
> > space, but it seems useless.
> > 
> > >
> > > Does that make sense?
> > >
> > > Thanks!
> > > Leo
> > >
> > > >
> > > > Freeing unused kernel image (initmem) memory: 2236K
> > > > Run /sbin/init as init process
> > > > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > > > Run /etc/init as init process
> > > > ...
> > > >
> > > > 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/pgtable.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > > index ab00235b018f..74ffb2178f54 100644
> > > > --- a/arch/riscv/include/asm/pgtable.h
> > > > +++ b/arch/riscv/include/asm/pgtable.h
> > > > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > > >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > > >
> > > >  #ifdef CONFIG_COMPAT
> > > > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > > > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
> > >
> > >
> > >
> > >
> > > >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> > > >                        TASK_SIZE_32 : TASK_SIZE_64)
> > > >  #else
> > > > --
> > > > 2.40.1
> > > >
> > >
> > 
> > 
> > -- 
> > Best Regards
> >  Guo Ren
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Guo Ren]

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
> > >
> >



-- 
Best Regards
 Guo Ren

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Guo Ren]

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
> > >
> >



-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> 

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 3/4] riscv: mm: Remove unused TASK_SIZE_MIN

[Leonardo Bras]

On Thu, Dec 21, 2023 at 10:47:00AM -0500, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> Remove TASK_SIZE_MIN because it's not used anymore.
> 
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
>  arch/riscv/include/asm/pgtable.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index 74ffb2178f54..e415582276ec 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -878,7 +878,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
>   */
>  #ifdef CONFIG_64BIT
>  #define TASK_SIZE_64	(PGDIR_SIZE * PTRS_PER_PGD / 2)
> -#define TASK_SIZE_MIN	(PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
>  
>  #ifdef CONFIG_COMPAT
>  #define TASK_SIZE_32	(_AC(0x80000000, UL))
> @@ -890,7 +889,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
>  
>  #else
>  #define TASK_SIZE	FIXADDR_START
> -#define TASK_SIZE_MIN	TASK_SIZE
>  #endif
>  
>  #else /* CONFIG_MMU */
> -- 
> 2.40.1
> 

On torvalds/master:

$git grep TASK_SIZE_MIN
arch/loongarch/include/asm/processor.h:23:#define TASK_SIZE_MIN TASK_SIZE
arch/loongarch/include/asm/processor.h:36:#define TASK_SIZE_MIN TASK_SIZE32
arch/riscv/include/asm/pgtable.h:881:#define TASK_SIZE_MIN      (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
arch/riscv/include/asm/pgtable.h:893:#define TASK_SIZE_MIN      TASK_SIZE

I can only see definitions, without any usage, so agreed on removing them.

FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>

I would also send a patch for loongarch, since they are in the same boat :)

Thanks!
Leo

Re: [PATCH V2 3/4] riscv: mm: Remove unused TASK_SIZE_MIN

[Leonardo Bras]

On Thu, Dec 21, 2023 at 10:47:00AM -0500, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
> 
> Remove TASK_SIZE_MIN because it's not used anymore.
> 
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Signed-off-by: Guo Ren <guoren@kernel.org>
> ---
>  arch/riscv/include/asm/pgtable.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index 74ffb2178f54..e415582276ec 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -878,7 +878,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
>   */
>  #ifdef CONFIG_64BIT
>  #define TASK_SIZE_64	(PGDIR_SIZE * PTRS_PER_PGD / 2)
> -#define TASK_SIZE_MIN	(PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
>  
>  #ifdef CONFIG_COMPAT
>  #define TASK_SIZE_32	(_AC(0x80000000, UL))
> @@ -890,7 +889,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
>  
>  #else
>  #define TASK_SIZE	FIXADDR_START
> -#define TASK_SIZE_MIN	TASK_SIZE
>  #endif
>  
>  #else /* CONFIG_MMU */
> -- 
> 2.40.1
> 

On torvalds/master:

$git grep TASK_SIZE_MIN
arch/loongarch/include/asm/processor.h:23:#define TASK_SIZE_MIN TASK_SIZE
arch/loongarch/include/asm/processor.h:36:#define TASK_SIZE_MIN TASK_SIZE32
arch/riscv/include/asm/pgtable.h:881:#define TASK_SIZE_MIN      (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
arch/riscv/include/asm/pgtable.h:893:#define TASK_SIZE_MIN      TASK_SIZE

I can only see definitions, without any usage, so agreed on removing them.

FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>

I would also send a patch for loongarch, since they are in the same boat :)

Thanks!
Leo


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[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
> >
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[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
> >
>


-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Leonardo Bras]

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>

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Leonardo Bras]

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>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> 

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Charlie Jenkins]

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
> > > > 
> > > 
> > 
> 

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Charlie Jenkins]

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
> > > > 
> > > 
> > 
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 3/4] riscv: mm: Remove unused TASK_SIZE_MIN

[Guo Ren]

On Fri, Dec 22, 2023 at 12:49 PM Leonardo Bras <leobras@redhat.com> wrote:
>
> On Thu, Dec 21, 2023 at 10:47:00AM -0500, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Remove TASK_SIZE_MIN because it's not used anymore.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > ---
> >  arch/riscv/include/asm/pgtable.h | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index 74ffb2178f54..e415582276ec 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -878,7 +878,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> >   */
> >  #ifdef CONFIG_64BIT
> >  #define TASK_SIZE_64 (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > -#define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> >
> >  #ifdef CONFIG_COMPAT
> >  #define TASK_SIZE_32 (_AC(0x80000000, UL))
> > @@ -890,7 +889,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> >
> >  #else
> >  #define TASK_SIZE    FIXADDR_START
> > -#define TASK_SIZE_MIN        TASK_SIZE
> >  #endif
> >
> >  #else /* CONFIG_MMU */
> > --
> > 2.40.1
> >
>
> On torvalds/master:
>
> $git grep TASK_SIZE_MIN
> arch/loongarch/include/asm/processor.h:23:#define TASK_SIZE_MIN TASK_SIZE
> arch/loongarch/include/asm/processor.h:36:#define TASK_SIZE_MIN TASK_SIZE32
> arch/riscv/include/asm/pgtable.h:881:#define TASK_SIZE_MIN      (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> arch/riscv/include/asm/pgtable.h:893:#define TASK_SIZE_MIN      TASK_SIZE
>
> I can only see definitions, without any usage, so agreed on removing them.
>
> FWIW:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>
Thx

>
> I would also send a patch for loongarch, since they are in the same boat :)
Eh... I've sent one yesterday together.

https://lore.kernel.org/loongarch/20231221054624.2208019-1-guoren@kernel.org/

>
> Thanks!
> Leo
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V2 3/4] riscv: mm: Remove unused TASK_SIZE_MIN

[Guo Ren]

On Fri, Dec 22, 2023 at 12:49 PM Leonardo Bras <leobras@redhat.com> wrote:
>
> On Thu, Dec 21, 2023 at 10:47:00AM -0500, guoren@kernel.org wrote:
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > Remove TASK_SIZE_MIN because it's not used anymore.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > ---
> >  arch/riscv/include/asm/pgtable.h | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index 74ffb2178f54..e415582276ec 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -878,7 +878,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> >   */
> >  #ifdef CONFIG_64BIT
> >  #define TASK_SIZE_64 (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > -#define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> >
> >  #ifdef CONFIG_COMPAT
> >  #define TASK_SIZE_32 (_AC(0x80000000, UL))
> > @@ -890,7 +889,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> >
> >  #else
> >  #define TASK_SIZE    FIXADDR_START
> > -#define TASK_SIZE_MIN        TASK_SIZE
> >  #endif
> >
> >  #else /* CONFIG_MMU */
> > --
> > 2.40.1
> >
>
> On torvalds/master:
>
> $git grep TASK_SIZE_MIN
> arch/loongarch/include/asm/processor.h:23:#define TASK_SIZE_MIN TASK_SIZE
> arch/loongarch/include/asm/processor.h:36:#define TASK_SIZE_MIN TASK_SIZE32
> arch/riscv/include/asm/pgtable.h:881:#define TASK_SIZE_MIN      (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> arch/riscv/include/asm/pgtable.h:893:#define TASK_SIZE_MIN      TASK_SIZE
>
> I can only see definitions, without any usage, so agreed on removing them.
>
> FWIW:
> Reviewed-by: Leonardo Bras <leobras@redhat.com>
Thx

>
> I would also send a patch for loongarch, since they are in the same boat :)
Eh... I've sent one yesterday together.

https://lore.kernel.org/loongarch/20231221054624.2208019-1-guoren@kernel.org/

>
> Thanks!
> Leo
>


-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Guo Ren]

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
> >
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Guo Ren]

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
> >
>


-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 3/4] riscv: mm: Remove unused TASK_SIZE_MIN

[Leonardo Bras]

On Fri, Dec 22, 2023 at 03:16:45PM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 12:49 PM Leonardo Bras <leobras@redhat.com> wrote:
> >
> > On Thu, Dec 21, 2023 at 10:47:00AM -0500, guoren@kernel.org wrote:
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > Remove TASK_SIZE_MIN because it's not used anymore.
> > >
> > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > ---
> > >  arch/riscv/include/asm/pgtable.h | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > index 74ffb2178f54..e415582276ec 100644
> > > --- a/arch/riscv/include/asm/pgtable.h
> > > +++ b/arch/riscv/include/asm/pgtable.h
> > > @@ -878,7 +878,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > >   */
> > >  #ifdef CONFIG_64BIT
> > >  #define TASK_SIZE_64 (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > > -#define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > >
> > >  #ifdef CONFIG_COMPAT
> > >  #define TASK_SIZE_32 (_AC(0x80000000, UL))
> > > @@ -890,7 +889,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > >
> > >  #else
> > >  #define TASK_SIZE    FIXADDR_START
> > > -#define TASK_SIZE_MIN        TASK_SIZE
> > >  #endif
> > >
> > >  #else /* CONFIG_MMU */
> > > --
> > > 2.40.1
> > >
> >
> > On torvalds/master:
> >
> > $git grep TASK_SIZE_MIN
> > arch/loongarch/include/asm/processor.h:23:#define TASK_SIZE_MIN TASK_SIZE
> > arch/loongarch/include/asm/processor.h:36:#define TASK_SIZE_MIN TASK_SIZE32
> > arch/riscv/include/asm/pgtable.h:881:#define TASK_SIZE_MIN      (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > arch/riscv/include/asm/pgtable.h:893:#define TASK_SIZE_MIN      TASK_SIZE
> >
> > I can only see definitions, without any usage, so agreed on removing them.
> >
> > FWIW:
> > Reviewed-by: Leonardo Bras <leobras@redhat.com>
> Thx
> 
> >
> > I would also send a patch for loongarch, since they are in the same boat :)
> Eh... I've sent one yesterday together.
> 
> https://lore.kernel.org/loongarch/20231221054624.2208019-1-guoren@kernel.org/

Awesome! :)

> 
> >
> > Thanks!
> > Leo
> >
> 
> 
> -- 
> Best Regards
>  Guo Ren
> 

Re: [PATCH V2 3/4] riscv: mm: Remove unused TASK_SIZE_MIN

[Leonardo Bras]

On Fri, Dec 22, 2023 at 03:16:45PM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 12:49 PM Leonardo Bras <leobras@redhat.com> wrote:
> >
> > On Thu, Dec 21, 2023 at 10:47:00AM -0500, guoren@kernel.org wrote:
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > Remove TASK_SIZE_MIN because it's not used anymore.
> > >
> > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > ---
> > >  arch/riscv/include/asm/pgtable.h | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > index 74ffb2178f54..e415582276ec 100644
> > > --- a/arch/riscv/include/asm/pgtable.h
> > > +++ b/arch/riscv/include/asm/pgtable.h
> > > @@ -878,7 +878,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > >   */
> > >  #ifdef CONFIG_64BIT
> > >  #define TASK_SIZE_64 (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > > -#define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > >
> > >  #ifdef CONFIG_COMPAT
> > >  #define TASK_SIZE_32 (_AC(0x80000000, UL))
> > > @@ -890,7 +889,6 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > >
> > >  #else
> > >  #define TASK_SIZE    FIXADDR_START
> > > -#define TASK_SIZE_MIN        TASK_SIZE
> > >  #endif
> > >
> > >  #else /* CONFIG_MMU */
> > > --
> > > 2.40.1
> > >
> >
> > On torvalds/master:
> >
> > $git grep TASK_SIZE_MIN
> > arch/loongarch/include/asm/processor.h:23:#define TASK_SIZE_MIN TASK_SIZE
> > arch/loongarch/include/asm/processor.h:36:#define TASK_SIZE_MIN TASK_SIZE32
> > arch/riscv/include/asm/pgtable.h:881:#define TASK_SIZE_MIN      (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > arch/riscv/include/asm/pgtable.h:893:#define TASK_SIZE_MIN      TASK_SIZE
> >
> > I can only see definitions, without any usage, so agreed on removing them.
> >
> > FWIW:
> > Reviewed-by: Leonardo Bras <leobras@redhat.com>
> Thx
> 
> >
> > I would also send a patch for loongarch, since they are in the same boat :)
> Eh... I've sent one yesterday together.
> 
> https://lore.kernel.org/loongarch/20231221054624.2208019-1-guoren@kernel.org/

Awesome! :)

> 
> >
> > Thanks!
> > Leo
> >
> 
> 
> -- 
> Best Regards
>  Guo Ren
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Guo Ren]

On Fri, Dec 22, 2023 at 12:33 PM Leonardo Bras <leobras@redhat.com> wrote:
>
> On Thu, Dec 21, 2023 at 07:50:27PM -0800, Charlie Jenkins wrote:
> > On Fri, Dec 22, 2023 at 10:57:16AM +0800, Guo Ren wrote:
> > > On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
> > > >
> > > > Hello Guo Ren,
> > > >
> > > > On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > > >
> > > > > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > > > > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > > > > segment fault. Sometimes, it would cause boot failure when the whole
> > > > > rootfs is rv32.
> > > >
> > > > Checking if I get the scenario:
> > > >
> > > > In pgtable.h:
> > > > #ifdef CONFIG_64BIT
> > > > #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > > > #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > > >
> > > > #ifdef CONFIG_COMPAT
> > > > #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> > > > #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
> > > >                          TASK_SIZE_32 : TASK_SIZE_64)
> > > > #else
> > > > [...]
> > > >
> > > > Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> > > > compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
> > > >
> > > > from processor.h:
> > > > #ifdef CONFIG_64BIT
> > > > #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> > > > #define STACK_TOP_MAX           TASK_SIZE_64
> > > > [...]
> > > > #define STACK_TOP               DEFAULT_MAP_WINDOW
> > > >
> > > >
> > > > where:
> > > > #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> > > > with MMAP_VA_BITS_64 being either 48 or 37.
> > > >
> > > > In compat mode,
> > > > STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> > > > TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
> > > >
> > > > IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
> > > Yes, it causes the problem, which causes the boot to fail.
> >
> > I think what Leonardo is getting at is that it is odd that it would
> > cause boot issues if TASK_SIZE is not equal STACK_TOP. This seems
> > indicative of a different problem. While this may fix the issue, it
> > should be valid for TASK_SIZE to be less than STACK_TOP.
> >
> > - Charlie
> >
>
> That is also a good point, but I am not that acquainted to this to
> actually propose this.
>
> I was thinking more on these questions:
> Is TASK_SIZE and STACK_TOP related somehow?
> If so, would not be better to describe one in terms of the other, like
> #define TASK_SIZE (STACK_TOP - PAGE_SIZE)
TASK_SIZE means the maximum user address space, so it's the limitation
to any kind of mmap or stack ...
So STACK_TOP <= TASK_SIZE

Follow your idea. The question is:
#define TASK_SIZE ((UL(1) << (VA_BITS - 1)) - PAGE_SIZE)

Do we need to reserve one page between userspace & kernel?


>
> Or the other way around.
>
> I mean, if they have any relation it would be much easier to represent them
> that way, and it would avoid having two magical numbers.
>
> Thanks!
> Leo
>
> > >
> > > >
> > > > Then why not:
> > > > #ifdef CONFIG_COMPAT
> > > > #define TASK_SIZE_32    STACK_TOP
> > > Yes, it's the solution that I think at first. But I didn't find any
> > > problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
> > > unify it with Sv39 and Sv48.
> > >
> > > >
> > > > With some comments explaining why there is no need to reserve a PAGE_SIZE
> > > > in the TASK_SIZE_32.
> > > At first, I wanted to put a invalid page between the user & kernel
> > > space, but it seems useless.
> > >
> > > >
> > > > Does that make sense?
> > > >
> > > > Thanks!
> > > > Leo
> > > >
> > > > >
> > > > > Freeing unused kernel image (initmem) memory: 2236K
> > > > > Run /sbin/init as init process
> > > > > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > > > > Run /etc/init as init process
> > > > > ...
> > > > >
> > > > > 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/pgtable.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > > > index ab00235b018f..74ffb2178f54 100644
> > > > > --- a/arch/riscv/include/asm/pgtable.h
> > > > > +++ b/arch/riscv/include/asm/pgtable.h
> > > > > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > > > >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > > > >
> > > > >  #ifdef CONFIG_COMPAT
> > > > > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > > > > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
> > > >
> > > >
> > > >
> > > >
> > > > >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> > > > >                        TASK_SIZE_32 : TASK_SIZE_64)
> > > > >  #else
> > > > > --
> > > > > 2.40.1
> > > > >
> > > >
> > >
> > >
> > > --
> > > Best Regards
> > >  Guo Ren
> >
>


--
Best Regards
 Guo Ren

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Guo Ren]

On Fri, Dec 22, 2023 at 12:33 PM Leonardo Bras <leobras@redhat.com> wrote:
>
> On Thu, Dec 21, 2023 at 07:50:27PM -0800, Charlie Jenkins wrote:
> > On Fri, Dec 22, 2023 at 10:57:16AM +0800, Guo Ren wrote:
> > > On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
> > > >
> > > > Hello Guo Ren,
> > > >
> > > > On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > > >
> > > > > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > > > > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > > > > segment fault. Sometimes, it would cause boot failure when the whole
> > > > > rootfs is rv32.
> > > >
> > > > Checking if I get the scenario:
> > > >
> > > > In pgtable.h:
> > > > #ifdef CONFIG_64BIT
> > > > #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > > > #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > > >
> > > > #ifdef CONFIG_COMPAT
> > > > #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> > > > #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
> > > >                          TASK_SIZE_32 : TASK_SIZE_64)
> > > > #else
> > > > [...]
> > > >
> > > > Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> > > > compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
> > > >
> > > > from processor.h:
> > > > #ifdef CONFIG_64BIT
> > > > #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> > > > #define STACK_TOP_MAX           TASK_SIZE_64
> > > > [...]
> > > > #define STACK_TOP               DEFAULT_MAP_WINDOW
> > > >
> > > >
> > > > where:
> > > > #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> > > > with MMAP_VA_BITS_64 being either 48 or 37.
> > > >
> > > > In compat mode,
> > > > STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> > > > TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
> > > >
> > > > IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
> > > Yes, it causes the problem, which causes the boot to fail.
> >
> > I think what Leonardo is getting at is that it is odd that it would
> > cause boot issues if TASK_SIZE is not equal STACK_TOP. This seems
> > indicative of a different problem. While this may fix the issue, it
> > should be valid for TASK_SIZE to be less than STACK_TOP.
> >
> > - Charlie
> >
>
> That is also a good point, but I am not that acquainted to this to
> actually propose this.
>
> I was thinking more on these questions:
> Is TASK_SIZE and STACK_TOP related somehow?
> If so, would not be better to describe one in terms of the other, like
> #define TASK_SIZE (STACK_TOP - PAGE_SIZE)
TASK_SIZE means the maximum user address space, so it's the limitation
to any kind of mmap or stack ...
So STACK_TOP <= TASK_SIZE

Follow your idea. The question is:
#define TASK_SIZE ((UL(1) << (VA_BITS - 1)) - PAGE_SIZE)

Do we need to reserve one page between userspace & kernel?


>
> Or the other way around.
>
> I mean, if they have any relation it would be much easier to represent them
> that way, and it would avoid having two magical numbers.
>
> Thanks!
> Leo
>
> > >
> > > >
> > > > Then why not:
> > > > #ifdef CONFIG_COMPAT
> > > > #define TASK_SIZE_32    STACK_TOP
> > > Yes, it's the solution that I think at first. But I didn't find any
> > > problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
> > > unify it with Sv39 and Sv48.
> > >
> > > >
> > > > With some comments explaining why there is no need to reserve a PAGE_SIZE
> > > > in the TASK_SIZE_32.
> > > At first, I wanted to put a invalid page between the user & kernel
> > > space, but it seems useless.
> > >
> > > >
> > > > Does that make sense?
> > > >
> > > > Thanks!
> > > > Leo
> > > >
> > > > >
> > > > > Freeing unused kernel image (initmem) memory: 2236K
> > > > > Run /sbin/init as init process
> > > > > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > > > > Run /etc/init as init process
> > > > > ...
> > > > >
> > > > > 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/pgtable.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > > > index ab00235b018f..74ffb2178f54 100644
> > > > > --- a/arch/riscv/include/asm/pgtable.h
> > > > > +++ b/arch/riscv/include/asm/pgtable.h
> > > > > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > > > >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > > > >
> > > > >  #ifdef CONFIG_COMPAT
> > > > > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > > > > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
> > > >
> > > >
> > > >
> > > >
> > > > >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> > > > >                        TASK_SIZE_32 : TASK_SIZE_64)
> > > > >  #else
> > > > > --
> > > > > 2.40.1
> > > > >
> > > >
> > >
> > >
> > > --
> > > Best Regards
> > >  Guo Ren
> >
>


--
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> 

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> > > > > 
> > > > 
> > > 
> > 
> 

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Leonardo Bras]

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
> > > > > 
> > > > 
> > > 
> > 
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

RE: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[David Laight]

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)

RE: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[David Laight]

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)


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Guo Ren]

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)
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V2 2/4] riscv: mm: Fixup compat arch_get_mmap_end

[Guo Ren]

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)
>


-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Guo Ren]

Hello Charlie,

On Fri, Dec 22, 2023 at 11:50 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Fri, Dec 22, 2023 at 10:57:16AM +0800, Guo Ren wrote:
> > On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
> > >
> > > Hello Guo Ren,
> > >
> > > On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > >
> > > > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > > > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > > > segment fault. Sometimes, it would cause boot failure when the whole
> > > > rootfs is rv32.
> > >
> > > Checking if I get the scenario:
> > >
> > > In pgtable.h:
> > > #ifdef CONFIG_64BIT
> > > #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > > #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > >
> > > #ifdef CONFIG_COMPAT
> > > #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> > > #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
> > >                          TASK_SIZE_32 : TASK_SIZE_64)
> > > #else
> > > [...]
> > >
> > > Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> > > compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
> > >
> > > from processor.h:
> > > #ifdef CONFIG_64BIT
> > > #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> > > #define STACK_TOP_MAX           TASK_SIZE_64
> > > [...]
> > > #define STACK_TOP               DEFAULT_MAP_WINDOW
> > >
> > >
> > > where:
> > > #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> > > with MMAP_VA_BITS_64 being either 48 or 37.
> > >
> > > In compat mode,
> > > STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> > > TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
> > >
> > > IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
> > Yes, it causes the problem, which causes the boot to fail.
>
> I think what Leonardo is getting at is that it is odd that it would
> cause boot issues if TASK_SIZE is not equal STACK_TOP. This seems
> indicative of a different problem. While this may fix the issue, it
> should be valid for TASK_SIZE to be less than STACK_TOP.
Sorry, I don't make sense here. Why do you think STACK_TOP could > TASK_SIZE?
TASK_SIZE is the highest priority of the address limitation for
user-space address, right?

Do you mean:
STACK_TOP could > MMAP_END?

>
> - Charlie
>
> >
> > >
> > > Then why not:
> > > #ifdef CONFIG_COMPAT
> > > #define TASK_SIZE_32    STACK_TOP
> > Yes, it's the solution that I think at first. But I didn't find any
> > problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
> > unify it with Sv39 and Sv48.
> >
> > >
> > > With some comments explaining why there is no need to reserve a PAGE_SIZE
> > > in the TASK_SIZE_32.
> > At first, I wanted to put a invalid page between the user & kernel
> > space, but it seems useless.
> >
> > >
> > > Does that make sense?
> > >
> > > Thanks!
> > > Leo
> > >
> > > >
> > > > Freeing unused kernel image (initmem) memory: 2236K
> > > > Run /sbin/init as init process
> > > > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > > > Run /etc/init as init process
> > > > ...
> > > >
> > > > 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/pgtable.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > > index ab00235b018f..74ffb2178f54 100644
> > > > --- a/arch/riscv/include/asm/pgtable.h
> > > > +++ b/arch/riscv/include/asm/pgtable.h
> > > > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > > >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > > >
> > > >  #ifdef CONFIG_COMPAT
> > > > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > > > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
> > >
> > >
> > >
> > >
> > > >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> > > >                        TASK_SIZE_32 : TASK_SIZE_64)
> > > >  #else
> > > > --
> > > > 2.40.1
> > > >
> > >
> >
> >
> > --
> > Best Regards
> >  Guo Ren



-- 
Best Regards
 Guo Ren

Re: [PATCH V2 1/4] riscv: mm: Fixup compat mode boot failure

[Guo Ren]

Hello Charlie,

On Fri, Dec 22, 2023 at 11:50 AM Charlie Jenkins <charlie@rivosinc.com> wrote:
>
> On Fri, Dec 22, 2023 at 10:57:16AM +0800, Guo Ren wrote:
> > On Fri, Dec 22, 2023 at 9:51 AM Leonardo Bras <leobras@redhat.com> wrote:
> > >
> > > Hello Guo Ren,
> > >
> > > On Thu, Dec 21, 2023 at 10:46:58AM -0500, guoren@kernel.org wrote:
> > > > From: Guo Ren <guoren@linux.alibaba.com>
> > > >
> > > > In COMPAT mode, the STACK_TOP is 0x80000000, but the TASK_SIZE is
> > > > 0x7fff000. When the user stack is upon 0x7fff000, it will cause a user
> > > > segment fault. Sometimes, it would cause boot failure when the whole
> > > > rootfs is rv32.
> > >
> > > Checking if I get the scenario:
> > >
> > > In pgtable.h:
> > > #ifdef CONFIG_64BIT
> > > #define TASK_SIZE_64    (PGDIR_SIZE * PTRS_PER_PGD / 2)
> > > #define TASK_SIZE_MIN   (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > >
> > > #ifdef CONFIG_COMPAT
> > > #define TASK_SIZE_32    (_AC(0x80000000, UL) - PAGE_SIZE)
> > > #define TASK_SIZE       (test_thread_flag(TIF_32BIT) ? \
> > >                          TASK_SIZE_32 : TASK_SIZE_64)
> > > #else
> > > [...]
> > >
> > > Meaning CONFIG_COMPAT is only available in CONFIG_64BIT, and TASK_SIZE in
> > > compat mode is either TASK_SIZE_32 or TASK_SIZE_64 depending on the thread_flag.
> > >
> > > from processor.h:
> > > #ifdef CONFIG_64BIT
> > > #define DEFAULT_MAP_WINDOW      (UL(1) << (MMAP_VA_BITS - 1))
> > > #define STACK_TOP_MAX           TASK_SIZE_64
> > > [...]
> > > #define STACK_TOP               DEFAULT_MAP_WINDOW
> > >
> > >
> > > where:
> > > #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> > > with MMAP_VA_BITS_64 being either 48 or 37.
> > >
> > > In compat mode,
> > > STACK_TOP = 1 << (32 - 1)       -> 0x80000000
> > > TASK_SIZE = 0x8000000 - 4k      -> 0x7ffff000
> > >
> > > IIUC, your suggestion is to make TASK_SIZE = STACK_TOP in compat mode only.
> > Yes, it causes the problem, which causes the boot to fail.
>
> I think what Leonardo is getting at is that it is odd that it would
> cause boot issues if TASK_SIZE is not equal STACK_TOP. This seems
> indicative of a different problem. While this may fix the issue, it
> should be valid for TASK_SIZE to be less than STACK_TOP.
Sorry, I don't make sense here. Why do you think STACK_TOP could > TASK_SIZE?
TASK_SIZE is the highest priority of the address limitation for
user-space address, right?

Do you mean:
STACK_TOP could > MMAP_END?

>
> - Charlie
>
> >
> > >
> > > Then why not:
> > > #ifdef CONFIG_COMPAT
> > > #define TASK_SIZE_32    STACK_TOP
> > Yes, it's the solution that I think at first. But I didn't find any
> > problem with 0x7ffff000 ~ 0x80000000, and then I removed this gap to
> > unify it with Sv39 and Sv48.
> >
> > >
> > > With some comments explaining why there is no need to reserve a PAGE_SIZE
> > > in the TASK_SIZE_32.
> > At first, I wanted to put a invalid page between the user & kernel
> > space, but it seems useless.
> >
> > >
> > > Does that make sense?
> > >
> > > Thanks!
> > > Leo
> > >
> > > >
> > > > Freeing unused kernel image (initmem) memory: 2236K
> > > > Run /sbin/init as init process
> > > > Starting init: /sbin/init exists but couldn't execute it (error -14)
> > > > Run /etc/init as init process
> > > > ...
> > > >
> > > > 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/pgtable.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > > index ab00235b018f..74ffb2178f54 100644
> > > > --- a/arch/riscv/include/asm/pgtable.h
> > > > +++ b/arch/riscv/include/asm/pgtable.h
> > > > @@ -881,7 +881,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
> > > >  #define TASK_SIZE_MIN        (PGDIR_SIZE_L3 * PTRS_PER_PGD / 2)
> > > >
> > > >  #ifdef CONFIG_COMPAT
> > > > -#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
> > > > +#define TASK_SIZE_32 (_AC(0x80000000, UL))
> > >
> > >
> > >
> > >
> > > >  #define TASK_SIZE    (test_thread_flag(TIF_32BIT) ? \
> > > >                        TASK_SIZE_32 : TASK_SIZE_64)
> > > >  #else
> > > > --
> > > > 2.40.1
> > > >
> > >
> >
> >
> > --
> > Best Regards
> >  Guo Ren



-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Guo Ren]

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>
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Guo Ren]

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>
>


-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

RE: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[David Laight]

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)

RE: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[David Laight]

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)
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Guo Ren]

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)



-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Guo Ren]

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)



-- 
Best Regards
 Guo Ren

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Guo Ren]

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)



-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Guo Ren]

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)



-- 
Best Regards
 Guo Ren

RE: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[David Laight]

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)

RE: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[David Laight]

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)
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Guo Ren]

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)



-- 
Best Regards
 Guo Ren

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[Guo Ren]

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)



-- 
Best Regards
 Guo Ren

RE: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[David Laight]

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)

RE: [PATCH V2 4/4] riscv: mm: Optimize TASK_SIZE definition

[David Laight]

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)
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv