[RFC PATCH 0/4] Introduce & Optimize compat-mode helpers

linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed

* [RFC PATCH 0/4] Introduce & Optimize compat-mode helpers
@ 2023-12-22  7:46 Leonardo Bras
  2023-12-22  7:46 ` [RFC PATCH 1/4] riscv: Replace direct thread flag check with is_compat_task() Leonardo Bras
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Leonardo Bras @ 2023-12-22  7:46 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Leonardo Bras, Conor Dooley, Andy Chiu,
	Greg Ungerer, Vincent Chen, Xiao Wang, Charlie Jenkins,
	Andrew Morton, Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, Guo Ren
  Cc: linux-riscv, linux-kernel, linux-mm

I just saw the opportunity of optimizing the helper is_compat_task() by
introducing a compile-time test, and it made possible to remove some
#ifdef's without any loss of performance.

I also saw the possibility of removing the direct check of task flags from
general code, and concentrated it in asm/compat.h by creating a few more
helpers, which in the end helped optimize code.

Leonardo Bras (4):
  riscv: Replace direct thread flag check with is_compat_task()
  riscv: add compile-time test into is_compat_task()
  riscv: Introduce is_compat_thread() into compat.h
  riscv: Introduce set_compat_task() in asm/compat.h

 arch/riscv/include/asm/compat.h    | 19 +++++++++++++++++++
 arch/riscv/include/asm/elf.h       | 11 ++---------
 arch/riscv/include/asm/pgtable.h   |  8 +-------
 arch/riscv/include/asm/processor.h |  4 ++--
 arch/riscv/kernel/ptrace.c         |  6 +++---
 5 files changed, 27 insertions(+), 21 deletions(-)


base-commit: 24e0d2e527a39f64caeb2e6be39ad5396fb2da5e
-- 
2.43.0



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 1/4] riscv: Replace direct thread flag check with is_compat_task()
  2023-12-22  7:46 [RFC PATCH 0/4] Introduce & Optimize compat-mode helpers Leonardo Bras
@ 2023-12-22  7:46 ` Leonardo Bras
  2023-12-22  7:46 ` [RFC PATCH 2/4] riscv: add compile-time test into is_compat_task() Leonardo Bras
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Leonardo Bras @ 2023-12-22  7:46 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Leonardo Bras, Conor Dooley, Andy Chiu,
	Greg Ungerer, Vincent Chen, Xiao Wang, Charlie Jenkins,
	Andrew Morton, Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, Guo Ren
  Cc: linux-riscv, linux-kernel, linux-mm

There is some code that detects compat mode into a task by checking the
flag directly, and other code that check using the helper is_compat_task().

Since the helper already exists, use it instead of checking the flags
directly.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 arch/riscv/include/asm/elf.h     | 2 +-
 arch/riscv/include/asm/pgtable.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index 06c236bfab53b..59a08367fddd7 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -54,7 +54,7 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
 
 #ifdef CONFIG_64BIT
 #ifdef CONFIG_COMPAT
-#define STACK_RND_MASK		(test_thread_flag(TIF_32BIT) ? \
+#define STACK_RND_MASK		(is_compat_task() ? \
 				 0x7ff >> (PAGE_SHIFT - 12) : \
 				 0x3ffff >> (PAGE_SHIFT - 12))
 #else
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index ab00235b018f8..1d472b31e0cfe 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -882,7 +882,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
 
 #ifdef CONFIG_COMPAT
 #define TASK_SIZE_32	(_AC(0x80000000, UL) - PAGE_SIZE)
-#define TASK_SIZE	(test_thread_flag(TIF_32BIT) ? \
+#define TASK_SIZE	(is_compat_task() ? \
 			 TASK_SIZE_32 : TASK_SIZE_64)
 #else
 #define TASK_SIZE	TASK_SIZE_64
-- 
2.43.0



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 2/4] riscv: add compile-time test into is_compat_task()
  2023-12-22  7:46 [RFC PATCH 0/4] Introduce & Optimize compat-mode helpers Leonardo Bras
  2023-12-22  7:46 ` [RFC PATCH 1/4] riscv: Replace direct thread flag check with is_compat_task() Leonardo Bras
@ 2023-12-22  7:46 ` Leonardo Bras
  2023-12-22  9:35   ` Guo Ren
  2023-12-22  7:46 ` [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h Leonardo Bras
  2023-12-22  7:46 ` [RFC PATCH 4/4] riscv: Introduce set_compat_task() in asm/compat.h Leonardo Bras
  3 siblings, 1 reply; 14+ messages in thread
From: Leonardo Bras @ 2023-12-22  7:46 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Leonardo Bras, Conor Dooley, Andy Chiu,
	Greg Ungerer, Vincent Chen, Xiao Wang, Charlie Jenkins,
	Andrew Morton, Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, Guo Ren
  Cc: linux-riscv, linux-kernel, linux-mm

Currently several places will test for CONFIG_COMPAT before testing
is_compat_task(), probably in order to avoid a run-time test into the task
structure.

Since is_compat_task() is an inlined function, it would be helpful to add a
compile-time test of CONFIG_COMPAT, making sure it always returns zero when
the option is not enabled during the kernel build.

With this, the compiler is able to understand in build-time that
is_compat_task() will always return 0, and optimize-out some of the extra
code introduced by the option.

This will also allow removing a lot #ifdefs that were introduced, and make
the code more clean.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 arch/riscv/include/asm/compat.h    | 3 +++
 arch/riscv/include/asm/elf.h       | 4 ----
 arch/riscv/include/asm/pgtable.h   | 6 ------
 arch/riscv/include/asm/processor.h | 4 ++--
 4 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
index 2ac955b51148f..91517b51b8e27 100644
--- a/arch/riscv/include/asm/compat.h
+++ b/arch/riscv/include/asm/compat.h
@@ -14,6 +14,9 @@
 
 static inline int is_compat_task(void)
 {
+	if (!IS_ENABLED(CONFIG_COMPAT))
+		return 0;
+
 	return test_thread_flag(TIF_32BIT);
 }
 
diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index 59a08367fddd7..2e88257cafaea 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -53,13 +53,9 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
 #define ELF_ET_DYN_BASE		((DEFAULT_MAP_WINDOW / 3) * 2)
 
 #ifdef CONFIG_64BIT
-#ifdef CONFIG_COMPAT
 #define STACK_RND_MASK		(is_compat_task() ? \
 				 0x7ff >> (PAGE_SHIFT - 12) : \
 				 0x3ffff >> (PAGE_SHIFT - 12))
-#else
-#define STACK_RND_MASK		(0x3ffff >> (PAGE_SHIFT - 12))
-#endif
 #endif
 
 /*
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 1d472b31e0cfe..ea5b269be223a 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -127,16 +127,10 @@
 #define VA_USER_SV48 (UL(1) << (VA_BITS_SV48 - 1))
 #define VA_USER_SV57 (UL(1) << (VA_BITS_SV57 - 1))
 
-#ifdef CONFIG_COMPAT
 #define MMAP_VA_BITS_64 ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
 #define MMAP_MIN_VA_BITS_64 (VA_BITS_SV39)
 #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
 #define MMAP_MIN_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_MIN_VA_BITS_64)
-#else
-#define MMAP_VA_BITS ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
-#define MMAP_MIN_VA_BITS (VA_BITS_SV39)
-#endif /* CONFIG_COMPAT */
-
 #else
 #include <asm/pgtable-32.h>
 #endif /* CONFIG_64BIT */
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index f19f861cda549..ed32e53e55999 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -22,7 +22,7 @@
 ({								\
 	unsigned long mmap_end;					\
 	typeof(addr) _addr = (addr);				\
-	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
+	if ((_addr) == 0 || is_compat_task())			\
 		mmap_end = STACK_TOP_MAX;			\
 	else if ((_addr) >= VA_USER_SV57)			\
 		mmap_end = STACK_TOP_MAX;			\
@@ -39,7 +39,7 @@
 	typeof(addr) _addr = (addr);				\
 	typeof(base) _base = (base);				\
 	unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);	\
-	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
+	if ((_addr) == 0 || is_compat_task())			\
 		mmap_base = (_base);				\
 	else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
 		mmap_base = VA_USER_SV57 - rnd_gap;		\
-- 
2.43.0



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/4] riscv: add compile-time test into is_compat_task()
  2023-12-22  7:46 ` [RFC PATCH 2/4] riscv: add compile-time test into is_compat_task() Leonardo Bras
@ 2023-12-22  9:35   ` Guo Ren
  2023-12-22 17:12     ` Leonardo Bras
  0 siblings, 1 reply; 14+ messages in thread
From: Guo Ren @ 2023-12-22  9:35 UTC (permalink / raw)
  To: Leonardo Bras
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Conor Dooley, Andy Chiu, Greg Ungerer,
	Vincent Chen, Xiao Wang, Charlie Jenkins, Andrew Morton,
	Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, linux-riscv, linux-kernel, linux-mm

On Fri, Dec 22, 2023 at 5:02 PM Leonardo Bras <leobras@redhat.com> wrote:
>
> Currently several places will test for CONFIG_COMPAT before testing
> is_compat_task(), probably in order to avoid a run-time test into the task
> structure.
>
> Since is_compat_task() is an inlined function, it would be helpful to add a
> compile-time test of CONFIG_COMPAT, making sure it always returns zero when
> the option is not enabled during the kernel build.
>
> With this, the compiler is able to understand in build-time that
> is_compat_task() will always return 0, and optimize-out some of the extra
> code introduced by the option.
>
> This will also allow removing a lot #ifdefs that were introduced, and make
> the code more clean.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  arch/riscv/include/asm/compat.h    | 3 +++
>  arch/riscv/include/asm/elf.h       | 4 ----
>  arch/riscv/include/asm/pgtable.h   | 6 ------
>  arch/riscv/include/asm/processor.h | 4 ++--
>  4 files changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
> index 2ac955b51148f..91517b51b8e27 100644
> --- a/arch/riscv/include/asm/compat.h
> +++ b/arch/riscv/include/asm/compat.h
> @@ -14,6 +14,9 @@
>
>  static inline int is_compat_task(void)
>  {
> +       if (!IS_ENABLED(CONFIG_COMPAT))
> +               return 0;
> +
>         return test_thread_flag(TIF_32BIT);
>  }
>
> diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
> index 59a08367fddd7..2e88257cafaea 100644
> --- a/arch/riscv/include/asm/elf.h
> +++ b/arch/riscv/include/asm/elf.h
> @@ -53,13 +53,9 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
>  #define ELF_ET_DYN_BASE                ((DEFAULT_MAP_WINDOW / 3) * 2)
>
>  #ifdef CONFIG_64BIT
> -#ifdef CONFIG_COMPAT
>  #define STACK_RND_MASK         (is_compat_task() ? \
>                                  0x7ff >> (PAGE_SHIFT - 12) : \
>                                  0x3ffff >> (PAGE_SHIFT - 12))
> -#else
> -#define STACK_RND_MASK         (0x3ffff >> (PAGE_SHIFT - 12))
> -#endif
>  #endif
>
>  /*
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index 1d472b31e0cfe..ea5b269be223a 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -127,16 +127,10 @@
>  #define VA_USER_SV48 (UL(1) << (VA_BITS_SV48 - 1))
>  #define VA_USER_SV57 (UL(1) << (VA_BITS_SV57 - 1))
>
> -#ifdef CONFIG_COMPAT
>  #define MMAP_VA_BITS_64 ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
>  #define MMAP_MIN_VA_BITS_64 (VA_BITS_SV39)
>  #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
>  #define MMAP_MIN_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_MIN_VA_BITS_64)
> -#else
> -#define MMAP_VA_BITS ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
> -#define MMAP_MIN_VA_BITS (VA_BITS_SV39)
> -#endif /* CONFIG_COMPAT */
> -
>  #else
>  #include <asm/pgtable-32.h>
>  #endif /* CONFIG_64BIT */
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index f19f861cda549..ed32e53e55999 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -22,7 +22,7 @@
>  ({                                                             \
>         unsigned long mmap_end;                                 \
>         typeof(addr) _addr = (addr);                            \
> -       if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> +       if ((_addr) == 0 || is_compat_task())                   \
>                 mmap_end = STACK_TOP_MAX;                       \
>         else if ((_addr) >= VA_USER_SV57)                       \
>                 mmap_end = STACK_TOP_MAX;                       \
> @@ -39,7 +39,7 @@
>         typeof(addr) _addr = (addr);                            \
>         typeof(base) _base = (base);                            \
>         unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);   \
> -       if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> +       if ((_addr) == 0 || is_compat_task())                   \
>                 mmap_base = (_base);                            \
>         else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
>                 mmap_base = VA_USER_SV57 - rnd_gap;             \
> --
> 2.43.0
>
Reviewed-by: Guo Ren <guoren@kernel.org>

-- 
Best Regards
 Guo Ren


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/4] riscv: add compile-time test into is_compat_task()
  2023-12-22  9:35   ` Guo Ren
@ 2023-12-22 17:12     ` Leonardo Bras
  2023-12-23  4:15       ` Andy Chiu
  0 siblings, 1 reply; 14+ messages in thread
From: Leonardo Bras @ 2023-12-22 17:12 UTC (permalink / raw)
  To: Guo Ren
  Cc: Leonardo Bras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Eric Biederman, Kees Cook, Oleg Nesterov, Conor Dooley,
	Andy Chiu, Greg Ungerer, Vincent Chen, Xiao Wang,
	Charlie Jenkins, Andrew Morton, Alexandre Ghiti, Kemeng Shi,
	David Hildenbrand, Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, linux-riscv, linux-kernel, linux-mm

On Fri, Dec 22, 2023 at 05:35:20PM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 5:02 PM Leonardo Bras <leobras@redhat.com> wrote:
> >
> > Currently several places will test for CONFIG_COMPAT before testing
> > is_compat_task(), probably in order to avoid a run-time test into the task
> > structure.
> >
> > Since is_compat_task() is an inlined function, it would be helpful to add a
> > compile-time test of CONFIG_COMPAT, making sure it always returns zero when
> > the option is not enabled during the kernel build.
> >
> > With this, the compiler is able to understand in build-time that
> > is_compat_task() will always return 0, and optimize-out some of the extra
> > code introduced by the option.
> >
> > This will also allow removing a lot #ifdefs that were introduced, and make
> > the code more clean.
> >
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > ---
> >  arch/riscv/include/asm/compat.h    | 3 +++
> >  arch/riscv/include/asm/elf.h       | 4 ----
> >  arch/riscv/include/asm/pgtable.h   | 6 ------
> >  arch/riscv/include/asm/processor.h | 4 ++--
> >  4 files changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
> > index 2ac955b51148f..91517b51b8e27 100644
> > --- a/arch/riscv/include/asm/compat.h
> > +++ b/arch/riscv/include/asm/compat.h
> > @@ -14,6 +14,9 @@
> >
> >  static inline int is_compat_task(void)
> >  {
> > +       if (!IS_ENABLED(CONFIG_COMPAT))
> > +               return 0;
> > +
> >         return test_thread_flag(TIF_32BIT);
> >  }
> >
> > diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
> > index 59a08367fddd7..2e88257cafaea 100644
> > --- a/arch/riscv/include/asm/elf.h
> > +++ b/arch/riscv/include/asm/elf.h
> > @@ -53,13 +53,9 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
> >  #define ELF_ET_DYN_BASE                ((DEFAULT_MAP_WINDOW / 3) * 2)
> >
> >  #ifdef CONFIG_64BIT
> > -#ifdef CONFIG_COMPAT
> >  #define STACK_RND_MASK         (is_compat_task() ? \
> >                                  0x7ff >> (PAGE_SHIFT - 12) : \
> >                                  0x3ffff >> (PAGE_SHIFT - 12))
> > -#else
> > -#define STACK_RND_MASK         (0x3ffff >> (PAGE_SHIFT - 12))
> > -#endif
> >  #endif
> >
> >  /*
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index 1d472b31e0cfe..ea5b269be223a 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -127,16 +127,10 @@
> >  #define VA_USER_SV48 (UL(1) << (VA_BITS_SV48 - 1))
> >  #define VA_USER_SV57 (UL(1) << (VA_BITS_SV57 - 1))
> >
> > -#ifdef CONFIG_COMPAT
> >  #define MMAP_VA_BITS_64 ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
> >  #define MMAP_MIN_VA_BITS_64 (VA_BITS_SV39)
> >  #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> >  #define MMAP_MIN_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_MIN_VA_BITS_64)
> > -#else
> > -#define MMAP_VA_BITS ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
> > -#define MMAP_MIN_VA_BITS (VA_BITS_SV39)
> > -#endif /* CONFIG_COMPAT */
> > -
> >  #else
> >  #include <asm/pgtable-32.h>
> >  #endif /* CONFIG_64BIT */
> > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> > index f19f861cda549..ed32e53e55999 100644
> > --- a/arch/riscv/include/asm/processor.h
> > +++ b/arch/riscv/include/asm/processor.h
> > @@ -22,7 +22,7 @@
> >  ({                                                             \
> >         unsigned long mmap_end;                                 \
> >         typeof(addr) _addr = (addr);                            \
> > -       if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> > +       if ((_addr) == 0 || is_compat_task())                   \
> >                 mmap_end = STACK_TOP_MAX;                       \
> >         else if ((_addr) >= VA_USER_SV57)                       \
> >                 mmap_end = STACK_TOP_MAX;                       \
> > @@ -39,7 +39,7 @@
> >         typeof(addr) _addr = (addr);                            \
> >         typeof(base) _base = (base);                            \
> >         unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);   \
> > -       if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> > +       if ((_addr) == 0 || is_compat_task())                   \
> >                 mmap_base = (_base);                            \
> >         else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
> >                 mmap_base = VA_USER_SV57 - rnd_gap;             \
> > --
> > 2.43.0
> >
> Reviewed-by: Guo Ren <guoren@kernel.org>

Thanks!
Leo

> 
> -- 
> Best Regards
>  Guo Ren
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 2/4] riscv: add compile-time test into is_compat_task()
  2023-12-22 17:12     ` Leonardo Bras
@ 2023-12-23  4:15       ` Andy Chiu
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Chiu @ 2023-12-23  4:15 UTC (permalink / raw)
  To: Leonardo Bras
  Cc: Guo Ren, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Eric Biederman, Kees Cook, Oleg Nesterov, Conor Dooley,
	Greg Ungerer, Vincent Chen, Xiao Wang, Charlie Jenkins,
	Andrew Morton, Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, linux-riscv, linux-kernel, linux-mm

On Sat, Dec 23, 2023 at 1:12 AM Leonardo Bras <leobras@redhat.com> wrote:
>
> On Fri, Dec 22, 2023 at 05:35:20PM +0800, Guo Ren wrote:
> > On Fri, Dec 22, 2023 at 5:02 PM Leonardo Bras <leobras@redhat.com> wrote:
> > >
> > > Currently several places will test for CONFIG_COMPAT before testing
> > > is_compat_task(), probably in order to avoid a run-time test into the task
> > > structure.
> > >
> > > Since is_compat_task() is an inlined function, it would be helpful to add a
> > > compile-time test of CONFIG_COMPAT, making sure it always returns zero when
> > > the option is not enabled during the kernel build.
> > >
> > > With this, the compiler is able to understand in build-time that
> > > is_compat_task() will always return 0, and optimize-out some of the extra
> > > code introduced by the option.
> > >
> > > This will also allow removing a lot #ifdefs that were introduced, and make
> > > the code more clean.
> > >
> > > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > > ---
> > >  arch/riscv/include/asm/compat.h    | 3 +++
> > >  arch/riscv/include/asm/elf.h       | 4 ----
> > >  arch/riscv/include/asm/pgtable.h   | 6 ------
> > >  arch/riscv/include/asm/processor.h | 4 ++--
> > >  4 files changed, 5 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
> > > index 2ac955b51148f..91517b51b8e27 100644
> > > --- a/arch/riscv/include/asm/compat.h
> > > +++ b/arch/riscv/include/asm/compat.h
> > > @@ -14,6 +14,9 @@
> > >
> > >  static inline int is_compat_task(void)
> > >  {
> > > +       if (!IS_ENABLED(CONFIG_COMPAT))
> > > +               return 0;
> > > +
> > >         return test_thread_flag(TIF_32BIT);
> > >  }
> > >
> > > diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
> > > index 59a08367fddd7..2e88257cafaea 100644
> > > --- a/arch/riscv/include/asm/elf.h
> > > +++ b/arch/riscv/include/asm/elf.h
> > > @@ -53,13 +53,9 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
> > >  #define ELF_ET_DYN_BASE                ((DEFAULT_MAP_WINDOW / 3) * 2)
> > >
> > >  #ifdef CONFIG_64BIT
> > > -#ifdef CONFIG_COMPAT
> > >  #define STACK_RND_MASK         (is_compat_task() ? \
> > >                                  0x7ff >> (PAGE_SHIFT - 12) : \
> > >                                  0x3ffff >> (PAGE_SHIFT - 12))
> > > -#else
> > > -#define STACK_RND_MASK         (0x3ffff >> (PAGE_SHIFT - 12))
> > > -#endif
> > >  #endif
> > >
> > >  /*
> > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > index 1d472b31e0cfe..ea5b269be223a 100644
> > > --- a/arch/riscv/include/asm/pgtable.h
> > > +++ b/arch/riscv/include/asm/pgtable.h
> > > @@ -127,16 +127,10 @@
> > >  #define VA_USER_SV48 (UL(1) << (VA_BITS_SV48 - 1))
> > >  #define VA_USER_SV57 (UL(1) << (VA_BITS_SV57 - 1))
> > >
> > > -#ifdef CONFIG_COMPAT
> > >  #define MMAP_VA_BITS_64 ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
> > >  #define MMAP_MIN_VA_BITS_64 (VA_BITS_SV39)
> > >  #define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
> > >  #define MMAP_MIN_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_MIN_VA_BITS_64)
> > > -#else
> > > -#define MMAP_VA_BITS ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
> > > -#define MMAP_MIN_VA_BITS (VA_BITS_SV39)
> > > -#endif /* CONFIG_COMPAT */
> > > -
> > >  #else
> > >  #include <asm/pgtable-32.h>
> > >  #endif /* CONFIG_64BIT */
> > > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> > > index f19f861cda549..ed32e53e55999 100644
> > > --- a/arch/riscv/include/asm/processor.h
> > > +++ b/arch/riscv/include/asm/processor.h
> > > @@ -22,7 +22,7 @@
> > >  ({                                                             \
> > >         unsigned long mmap_end;                                 \
> > >         typeof(addr) _addr = (addr);                            \
> > > -       if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> > > +       if ((_addr) == 0 || is_compat_task())                   \
> > >                 mmap_end = STACK_TOP_MAX;                       \
> > >         else if ((_addr) >= VA_USER_SV57)                       \
> > >                 mmap_end = STACK_TOP_MAX;                       \
> > > @@ -39,7 +39,7 @@
> > >         typeof(addr) _addr = (addr);                            \
> > >         typeof(base) _base = (base);                            \
> > >         unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);   \
> > > -       if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
> > > +       if ((_addr) == 0 || is_compat_task())                   \
> > >                 mmap_base = (_base);                            \
> > >         else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
> > >                 mmap_base = VA_USER_SV57 - rnd_gap;             \
> > > --
> > > 2.43.0
> > >
> > Reviewed-by: Guo Ren <guoren@kernel.org>
>
> Thanks!
> Leo
>
> >
> > --
> > Best Regards
> >  Guo Ren
> >
>

Reviewed-by: Andy Chiu <andy.chiu@sifive.com>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h
  2023-12-22  7:46 [RFC PATCH 0/4] Introduce & Optimize compat-mode helpers Leonardo Bras
  2023-12-22  7:46 ` [RFC PATCH 1/4] riscv: Replace direct thread flag check with is_compat_task() Leonardo Bras
  2023-12-22  7:46 ` [RFC PATCH 2/4] riscv: add compile-time test into is_compat_task() Leonardo Bras
@ 2023-12-22  7:46 ` Leonardo Bras
  2023-12-22 11:20   ` Guo Ren
  2023-12-23  4:25   ` Andy Chiu
  2023-12-22  7:46 ` [RFC PATCH 4/4] riscv: Introduce set_compat_task() in asm/compat.h Leonardo Bras
  3 siblings, 2 replies; 14+ messages in thread
From: Leonardo Bras @ 2023-12-22  7:46 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Leonardo Bras, Conor Dooley, Andy Chiu,
	Greg Ungerer, Vincent Chen, Xiao Wang, Charlie Jenkins,
	Andrew Morton, Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, Guo Ren
  Cc: linux-riscv, linux-kernel, linux-mm

task_user_regset_view() makes use of a function very similar to
is_compat_task(), but pointing to a any thread.

In arm64 asm/compat.h there is a function very similar to that:
is_compat_thread(struct thread_info *thread)

Copy this function to riscv asm/compat.h and make use of it into
task_user_regset_view().

Also, introduce a compile-time test for CONFIG_COMPAT and simplify the
function code by removing the #ifdef.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 arch/riscv/include/asm/compat.h | 8 ++++++++
 arch/riscv/kernel/ptrace.c      | 6 +++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
index 91517b51b8e27..da4b28cd01a95 100644
--- a/arch/riscv/include/asm/compat.h
+++ b/arch/riscv/include/asm/compat.h
@@ -20,6 +20,14 @@ static inline int is_compat_task(void)
 	return test_thread_flag(TIF_32BIT);
 }
 
+static inline int is_compat_thread(struct thread_info *thread)
+{
+	if (!IS_ENABLED(CONFIG_COMPAT))
+		return 0;
+
+	return test_ti_thread_flag(thread, TIF_32BIT);
+}
+
 struct compat_user_regs_struct {
 	compat_ulong_t pc;
 	compat_ulong_t ra;
diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
index 2afe460de16a6..f362832123616 100644
--- a/arch/riscv/kernel/ptrace.c
+++ b/arch/riscv/kernel/ptrace.c
@@ -374,14 +374,14 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 
 	return ret;
 }
+#else
+static const struct user_regset_view compat_riscv_user_native_view = {};
 #endif /* CONFIG_COMPAT */
 
 const struct user_regset_view *task_user_regset_view(struct task_struct *task)
 {
-#ifdef CONFIG_COMPAT
-	if (test_tsk_thread_flag(task, TIF_32BIT))
+	if (is_compat_thread(&task->thread_info))
 		return &compat_riscv_user_native_view;
 	else
-#endif
 		return &riscv_user_native_view;
 }
-- 
2.43.0



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h
  2023-12-22  7:46 ` [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h Leonardo Bras
@ 2023-12-22 11:20   ` Guo Ren
  2023-12-22 17:14     ` Leonardo Bras
  2023-12-23  4:25   ` Andy Chiu
  1 sibling, 1 reply; 14+ messages in thread
From: Guo Ren @ 2023-12-22 11:20 UTC (permalink / raw)
  To: Leonardo Bras
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Conor Dooley, Andy Chiu, Greg Ungerer,
	Vincent Chen, Xiao Wang, Charlie Jenkins, Andrew Morton,
	Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, linux-riscv, linux-kernel, linux-mm

On Fri, Dec 22, 2023 at 3:47 PM Leonardo Bras <leobras@redhat.com> wrote:
>
> task_user_regset_view() makes use of a function very similar to
> is_compat_task(), but pointing to a any thread.
>
> In arm64 asm/compat.h there is a function very similar to that:
> is_compat_thread(struct thread_info *thread)
>
> Copy this function to riscv asm/compat.h and make use of it into
> task_user_regset_view().
>
> Also, introduce a compile-time test for CONFIG_COMPAT and simplify the
> function code by removing the #ifdef.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  arch/riscv/include/asm/compat.h | 8 ++++++++
>  arch/riscv/kernel/ptrace.c      | 6 +++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
> index 91517b51b8e27..da4b28cd01a95 100644
> --- a/arch/riscv/include/asm/compat.h
> +++ b/arch/riscv/include/asm/compat.h
> @@ -20,6 +20,14 @@ static inline int is_compat_task(void)
>         return test_thread_flag(TIF_32BIT);
>  }
>
> +static inline int is_compat_thread(struct thread_info *thread)
> +{
> +       if (!IS_ENABLED(CONFIG_COMPAT))
> +               return 0;
We also could put this into is_compat_task().

> +
> +       return test_ti_thread_flag(thread, TIF_32BIT);
> +}
> +
>  struct compat_user_regs_struct {
>         compat_ulong_t pc;
>         compat_ulong_t ra;
> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> index 2afe460de16a6..f362832123616 100644
> --- a/arch/riscv/kernel/ptrace.c
> +++ b/arch/riscv/kernel/ptrace.c
> @@ -374,14 +374,14 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
>
>         return ret;
>  }
> +#else
> +static const struct user_regset_view compat_riscv_user_native_view = {};
>  #endif /* CONFIG_COMPAT */
>
>  const struct user_regset_view *task_user_regset_view(struct task_struct *task)
>  {
> -#ifdef CONFIG_COMPAT
> -       if (test_tsk_thread_flag(task, TIF_32BIT))
> +       if (is_compat_thread(&task->thread_info))
>                 return &compat_riscv_user_native_view;
>         else
> -#endif
>                 return &riscv_user_native_view;
>  }
> --
> 2.43.0
>
LGTM

Reviewed-by: Guo Ren <guoren@kernel.org>

-- 
Best Regards
 Guo Ren


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h
  2023-12-22 11:20   ` Guo Ren
@ 2023-12-22 17:14     ` Leonardo Bras
  0 siblings, 0 replies; 14+ messages in thread
From: Leonardo Bras @ 2023-12-22 17:14 UTC (permalink / raw)
  To: Guo Ren
  Cc: Leonardo Bras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Eric Biederman, Kees Cook, Oleg Nesterov, Conor Dooley,
	Andy Chiu, Greg Ungerer, Vincent Chen, Xiao Wang,
	Charlie Jenkins, Andrew Morton, Alexandre Ghiti, Kemeng Shi,
	David Hildenbrand, Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, linux-riscv, linux-kernel, linux-mm

On Fri, Dec 22, 2023 at 07:20:36PM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 3:47 PM Leonardo Bras <leobras@redhat.com> wrote:
> >
> > task_user_regset_view() makes use of a function very similar to
> > is_compat_task(), but pointing to a any thread.
> >
> > In arm64 asm/compat.h there is a function very similar to that:
> > is_compat_thread(struct thread_info *thread)
> >
> > Copy this function to riscv asm/compat.h and make use of it into
> > task_user_regset_view().
> >
> > Also, introduce a compile-time test for CONFIG_COMPAT and simplify the
> > function code by removing the #ifdef.
> >
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > ---
> >  arch/riscv/include/asm/compat.h | 8 ++++++++
> >  arch/riscv/kernel/ptrace.c      | 6 +++---
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
> > index 91517b51b8e27..da4b28cd01a95 100644
> > --- a/arch/riscv/include/asm/compat.h
> > +++ b/arch/riscv/include/asm/compat.h
> > @@ -20,6 +20,14 @@ static inline int is_compat_task(void)
> >         return test_thread_flag(TIF_32BIT);
> >  }
> >
> > +static inline int is_compat_thread(struct thread_info *thread)
> > +{
> > +       if (!IS_ENABLED(CONFIG_COMPAT))
> > +               return 0;
> We also could put this into is_compat_task().
> 

Yeah! it's done in a previous patch. :)

> > +
> > +       return test_ti_thread_flag(thread, TIF_32BIT);
> > +}
> > +
> >  struct compat_user_regs_struct {
> >         compat_ulong_t pc;
> >         compat_ulong_t ra;
> > diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> > index 2afe460de16a6..f362832123616 100644
> > --- a/arch/riscv/kernel/ptrace.c
> > +++ b/arch/riscv/kernel/ptrace.c
> > @@ -374,14 +374,14 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> >
> >         return ret;
> >  }
> > +#else
> > +static const struct user_regset_view compat_riscv_user_native_view = {};
> >  #endif /* CONFIG_COMPAT */
> >
> >  const struct user_regset_view *task_user_regset_view(struct task_struct *task)
> >  {
> > -#ifdef CONFIG_COMPAT
> > -       if (test_tsk_thread_flag(task, TIF_32BIT))
> > +       if (is_compat_thread(&task->thread_info))
> >                 return &compat_riscv_user_native_view;
> >         else
> > -#endif
> >                 return &riscv_user_native_view;
> >  }
> > --
> > 2.43.0
> >
> LGTM
> 
> Reviewed-by: Guo Ren <guoren@kernel.org>

Thanks!
Leo

> 
> -- 
> Best Regards
>  Guo Ren
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h
  2023-12-22  7:46 ` [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h Leonardo Bras
  2023-12-22 11:20   ` Guo Ren
@ 2023-12-23  4:25   ` Andy Chiu
       [not found]     ` <CAJ6HWG4Pe0co91-tRpV-gKoVUJkobB=uz8J-pjTvyX=Ed49u_w@mail.gmail.com>
  1 sibling, 1 reply; 14+ messages in thread
From: Andy Chiu @ 2023-12-23  4:25 UTC (permalink / raw)
  To: Leonardo Bras
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Conor Dooley, Greg Ungerer,
	Vincent Chen, Xiao Wang, Charlie Jenkins, Andrew Morton,
	Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, Guo Ren, linux-riscv, linux-kernel,
	linux-mm

On Fri, Dec 22, 2023 at 3:46 PM Leonardo Bras <leobras@redhat.com> wrote:
>
> task_user_regset_view() makes use of a function very similar to
> is_compat_task(), but pointing to a any thread.
>
> In arm64 asm/compat.h there is a function very similar to that:
> is_compat_thread(struct thread_info *thread)
>
> Copy this function to riscv asm/compat.h and make use of it into
> task_user_regset_view().
>
> Also, introduce a compile-time test for CONFIG_COMPAT and simplify the
> function code by removing the #ifdef.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  arch/riscv/include/asm/compat.h | 8 ++++++++
>  arch/riscv/kernel/ptrace.c      | 6 +++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
> index 91517b51b8e27..da4b28cd01a95 100644
> --- a/arch/riscv/include/asm/compat.h
> +++ b/arch/riscv/include/asm/compat.h
> @@ -20,6 +20,14 @@ static inline int is_compat_task(void)
>         return test_thread_flag(TIF_32BIT);
>  }
>
> +static inline int is_compat_thread(struct thread_info *thread)
> +{
> +       if (!IS_ENABLED(CONFIG_COMPAT))
> +               return 0;
> +
> +       return test_ti_thread_flag(thread, TIF_32BIT);
> +}
> +

Does it make sense to use a #ifdef CONFIG_COMPAT clause to group
is_compat_thread() and is_compat_flag()? For example,

#ifdef CONFIG_COMPAT
static inline int is_compat_thread(struct thread_info *thread)
{
 return test_ti_thread_flag(thread, TIF_32BIT);
}
static inline int is_compat_task(void)
{
 return is_compat_thread(current);
}
#else
static inline int is_compat_thread(struct thread_info *thread) { return 0; }
static inline int is_compat_task(void) { return 0; }
#endif

>  struct compat_user_regs_struct {
>         compat_ulong_t pc;
>         compat_ulong_t ra;
> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> index 2afe460de16a6..f362832123616 100644
> --- a/arch/riscv/kernel/ptrace.c
> +++ b/arch/riscv/kernel/ptrace.c
> @@ -374,14 +374,14 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
>
>         return ret;
>  }
> +#else
> +static const struct user_regset_view compat_riscv_user_native_view = {};
>  #endif /* CONFIG_COMPAT */
>
>  const struct user_regset_view *task_user_regset_view(struct task_struct *task)
>  {
> -#ifdef CONFIG_COMPAT
> -       if (test_tsk_thread_flag(task, TIF_32BIT))
> +       if (is_compat_thread(&task->thread_info))
>                 return &compat_riscv_user_native_view;
>         else
> -#endif
>                 return &riscv_user_native_view;
>  }
> --
> 2.43.0
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

[parent not found: <CAJ6HWG4Pe0co91-tRpV-gKoVUJkobB=uz8J-pjTvyX=Ed49u_w@mail.gmail.com>]

* Re: [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h
       [not found]     ` <CAJ6HWG4Pe0co91-tRpV-gKoVUJkobB=uz8J-pjTvyX=Ed49u_w@mail.gmail.com>
@ 2023-12-28  3:10       ` Andy Chiu
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Chiu @ 2023-12-28  3:10 UTC (permalink / raw)
  To: Leonardo Bras Soares Passos
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Conor Dooley, Greg Ungerer,
	Vincent Chen, Xiao Wang, Charlie Jenkins, Andrew Morton,
	Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, Guo Ren, linux-riscv, linux-kernel,
	linux-mm

On Wed, Dec 27, 2023 at 3:25 AM Leonardo Bras Soares Passos
<leobras@redhat.com> wrote:
>
> On Sat, Dec 23, 2023 at 1:26 AM Andy Chiu <andy.chiu@sifive.com> wrote:
> >
> > On Fri, Dec 22, 2023 at 3:46 PM Leonardo Bras <leobras@redhat.com> wrote:
> > >
> > > task_user_regset_view() makes use of a function very similar to
> > > is_compat_task(), but pointing to a any thread.
> > >
> > > In arm64 asm/compat.h there is a function very similar to that:
> > > is_compat_thread(struct thread_info *thread)
> > >
> > > Copy this function to riscv asm/compat.h and make use of it into
> > > task_user_regset_view().
> > >
> > > Also, introduce a compile-time test for CONFIG_COMPAT and simplify the
> > > function code by removing the #ifdef.
> > >
> > > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > > ---
> > >  arch/riscv/include/asm/compat.h | 8 ++++++++
> > >  arch/riscv/kernel/ptrace.c      | 6 +++---
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
> > > index 91517b51b8e27..da4b28cd01a95 100644
> > > --- a/arch/riscv/include/asm/compat.h
> > > +++ b/arch/riscv/include/asm/compat.h
> > > @@ -20,6 +20,14 @@ static inline int is_compat_task(void)
> > >         return test_thread_flag(TIF_32BIT);
> > >  }
> > >
> > > +static inline int is_compat_thread(struct thread_info *thread)
> > > +{
> > > +       if (!IS_ENABLED(CONFIG_COMPAT))
> > > +               return 0;
> > > +
> > > +       return test_ti_thread_flag(thread, TIF_32BIT);
> > > +}
> > > +
> >
> > Does it make sense to use a #ifdef CONFIG_COMPAT clause to group
> > is_compat_thread() and is_compat_flag()? For example,
>
> Hello Andy,
>
> Sure, it does make sense.
>
> But I honestly think that using IS_ENABLED() instead of #ifdef +
> multiple same-named functions works better for code reading, at least
> for small functions such as these.
>
> Does this make sense?
>
> Thanks for reviewing!
> Leo

Hi, yes!

It makes sense to me.

Reviewed-by: Andy Chiu <andy.chiu@sifive.com>

Thanks,
Andy

>
>
> >
> > #ifdef CONFIG_COMPAT
> > static inline int is_compat_thread(struct thread_info *thread)
> > {
> >  return test_ti_thread_flag(thread, TIF_32BIT);
> > }
> > static inline int is_compat_task(void)
> > {
> >  return is_compat_thread(current);
> > }
> > #else
> > static inline int is_compat_thread(struct thread_info *thread) { return 0; }
> > static inline int is_compat_task(void) { return 0; }
> > #endif
> >
> > >  struct compat_user_regs_struct {
> > >         compat_ulong_t pc;
> > >         compat_ulong_t ra;
> > > diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> > > index 2afe460de16a6..f362832123616 100644
> > > --- a/arch/riscv/kernel/ptrace.c
> > > +++ b/arch/riscv/kernel/ptrace.c
> > > @@ -374,14 +374,14 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
> > >
> > >         return ret;
> > >  }
> > > +#else
> > > +static const struct user_regset_view compat_riscv_user_native_view = {};
> > >  #endif /* CONFIG_COMPAT */
> > >
> > >  const struct user_regset_view *task_user_regset_view(struct task_struct *task)
> > >  {
> > > -#ifdef CONFIG_COMPAT
> > > -       if (test_tsk_thread_flag(task, TIF_32BIT))
> > > +       if (is_compat_thread(&task->thread_info))
> > >                 return &compat_riscv_user_native_view;
> > >         else
> > > -#endif
> > >                 return &riscv_user_native_view;
> > >  }
> > > --
> > > 2.43.0
> > >
> >
>


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH 4/4] riscv: Introduce set_compat_task() in asm/compat.h
  2023-12-22  7:46 [RFC PATCH 0/4] Introduce & Optimize compat-mode helpers Leonardo Bras
                   ` (2 preceding siblings ...)
  2023-12-22  7:46 ` [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h Leonardo Bras
@ 2023-12-22  7:46 ` Leonardo Bras
  2023-12-22  8:11   ` Guo Ren
  3 siblings, 1 reply; 14+ messages in thread
From: Leonardo Bras @ 2023-12-22  7:46 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Leonardo Bras, Conor Dooley, Andy Chiu,
	Greg Ungerer, Vincent Chen, Xiao Wang, Charlie Jenkins,
	Andrew Morton, Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, Guo Ren
  Cc: linux-riscv, linux-kernel, linux-mm

In order to have all task compat bit access directly in compat.h, introduce
set_compat_task() to set/reset those when needed.

Also, since it's only used on an if/else scenario, simplify the macro using
it.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 arch/riscv/include/asm/compat.h | 8 ++++++++
 arch/riscv/include/asm/elf.h    | 5 +----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
index da4b28cd01a95..aa103530a5c83 100644
--- a/arch/riscv/include/asm/compat.h
+++ b/arch/riscv/include/asm/compat.h
@@ -28,6 +28,14 @@ static inline int is_compat_thread(struct thread_info *thread)
 	return test_ti_thread_flag(thread, TIF_32BIT);
 }
 
+static inline void set_compat_task(bool is_compat)
+{
+	if (is_compat)
+		set_thread_flag(TIF_32BIT);
+	else
+		clear_thread_flag(TIF_32BIT);
+}
+
 struct compat_user_regs_struct {
 	compat_ulong_t pc;
 	compat_ulong_t ra;
diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index 2e88257cafaea..c7aea7886d22a 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -135,10 +135,7 @@ do {							\
 #ifdef CONFIG_COMPAT
 
 #define SET_PERSONALITY(ex)					\
-do {    if ((ex).e_ident[EI_CLASS] == ELFCLASS32)		\
-		set_thread_flag(TIF_32BIT);			\
-	else							\
-		clear_thread_flag(TIF_32BIT);			\
+do {	set_compat_task((ex).e_ident[EI_CLASS] == ELFCLASS32);	\
 	if (personality(current->personality) != PER_LINUX32)	\
 		set_personality(PER_LINUX |			\
 			(current->personality & (~PER_MASK)));	\
-- 
2.43.0



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 4/4] riscv: Introduce set_compat_task() in asm/compat.h
  2023-12-22  7:46 ` [RFC PATCH 4/4] riscv: Introduce set_compat_task() in asm/compat.h Leonardo Bras
@ 2023-12-22  8:11   ` Guo Ren
  2023-12-22  8:28     ` Leonardo Bras
  0 siblings, 1 reply; 14+ messages in thread
From: Guo Ren @ 2023-12-22  8:11 UTC (permalink / raw)
  To: Leonardo Bras
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Eric Biederman,
	Kees Cook, Oleg Nesterov, Conor Dooley, Andy Chiu, Greg Ungerer,
	Vincent Chen, Xiao Wang, Charlie Jenkins, Andrew Morton,
	Alexandre Ghiti, Kemeng Shi, David Hildenbrand,
	Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, linux-riscv, linux-kernel, linux-mm

On Fri, Dec 22, 2023 at 3:47 PM Leonardo Bras <leobras@redhat.com> wrote:
>
> In order to have all task compat bit access directly in compat.h, introduce
> set_compat_task() to set/reset those when needed.
>
> Also, since it's only used on an if/else scenario, simplify the macro using
> it.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  arch/riscv/include/asm/compat.h | 8 ++++++++
>  arch/riscv/include/asm/elf.h    | 5 +----
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
> index da4b28cd01a95..aa103530a5c83 100644
> --- a/arch/riscv/include/asm/compat.h
> +++ b/arch/riscv/include/asm/compat.h
> @@ -28,6 +28,14 @@ static inline int is_compat_thread(struct thread_info *thread)
>         return test_ti_thread_flag(thread, TIF_32BIT);
>  }
>
> +static inline void set_compat_task(bool is_compat)
> +{
> +       if (is_compat)
> +               set_thread_flag(TIF_32BIT);
> +       else
> +               clear_thread_flag(TIF_32BIT);
> +}
> +
>  struct compat_user_regs_struct {
>         compat_ulong_t pc;
>         compat_ulong_t ra;
> diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
> index 2e88257cafaea..c7aea7886d22a 100644
> --- a/arch/riscv/include/asm/elf.h
> +++ b/arch/riscv/include/asm/elf.h
> @@ -135,10 +135,7 @@ do {                                                       \
>  #ifdef CONFIG_COMPAT
>
>  #define SET_PERSONALITY(ex)                                    \
> -do {    if ((ex).e_ident[EI_CLASS] == ELFCLASS32)              \
> -               set_thread_flag(TIF_32BIT);                     \
> -       else                                                    \
> -               clear_thread_flag(TIF_32BIT);                   \
> +do {   set_compat_task((ex).e_ident[EI_CLASS] == ELFCLASS32);  \
>         if (personality(current->personality) != PER_LINUX32)   \
>                 set_personality(PER_LINUX |                     \
>                         (current->personality & (~PER_MASK)));  \
> --
> 2.43.0
>
LGTM

Reviewed-by: Guo Ren <guoren@kernel.org>

-- 
Best Regards
 Guo Ren


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH 4/4] riscv: Introduce set_compat_task() in asm/compat.h
  2023-12-22  8:11   ` Guo Ren
@ 2023-12-22  8:28     ` Leonardo Bras
  0 siblings, 0 replies; 14+ messages in thread
From: Leonardo Bras @ 2023-12-22  8:28 UTC (permalink / raw)
  To: Guo Ren
  Cc: Leonardo Bras, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Eric Biederman, Kees Cook, Oleg Nesterov, Conor Dooley,
	Andy Chiu, Greg Ungerer, Vincent Chen, Xiao Wang,
	Charlie Jenkins, Andrew Morton, Alexandre Ghiti, Kemeng Shi,
	David Hildenbrand, Matthew Wilcox (Oracle),
	Qinglin Pan, Greentime Hu, Björn Töpel,
	Clément Léger, linux-riscv, linux-kernel, linux-mm

On Fri, Dec 22, 2023 at 04:11:10PM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 3:47 PM Leonardo Bras <leobras@redhat.com> wrote:
> >
> > In order to have all task compat bit access directly in compat.h, introduce
> > set_compat_task() to set/reset those when needed.
> >
> > Also, since it's only used on an if/else scenario, simplify the macro using
> > it.
> >
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > ---
> >  arch/riscv/include/asm/compat.h | 8 ++++++++
> >  arch/riscv/include/asm/elf.h    | 5 +----
> >  2 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
> > index da4b28cd01a95..aa103530a5c83 100644
> > --- a/arch/riscv/include/asm/compat.h
> > +++ b/arch/riscv/include/asm/compat.h
> > @@ -28,6 +28,14 @@ static inline int is_compat_thread(struct thread_info *thread)
> >         return test_ti_thread_flag(thread, TIF_32BIT);
> >  }
> >
> > +static inline void set_compat_task(bool is_compat)
> > +{
> > +       if (is_compat)
> > +               set_thread_flag(TIF_32BIT);
> > +       else
> > +               clear_thread_flag(TIF_32BIT);
> > +}
> > +
> >  struct compat_user_regs_struct {
> >         compat_ulong_t pc;
> >         compat_ulong_t ra;
> > diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
> > index 2e88257cafaea..c7aea7886d22a 100644
> > --- a/arch/riscv/include/asm/elf.h
> > +++ b/arch/riscv/include/asm/elf.h
> > @@ -135,10 +135,7 @@ do {                                                       \
> >  #ifdef CONFIG_COMPAT
> >
> >  #define SET_PERSONALITY(ex)                                    \
> > -do {    if ((ex).e_ident[EI_CLASS] == ELFCLASS32)              \
> > -               set_thread_flag(TIF_32BIT);                     \
> > -       else                                                    \
> > -               clear_thread_flag(TIF_32BIT);                   \
> > +do {   set_compat_task((ex).e_ident[EI_CLASS] == ELFCLASS32);  \
> >         if (personality(current->personality) != PER_LINUX32)   \
> >                 set_personality(PER_LINUX |                     \
> >                         (current->personality & (~PER_MASK)));  \
> > --
> > 2.43.0
> >
> LGTM
> 
> Reviewed-by: Guo Ren <guoren@kernel.org>

Thanks!
Leo

> 
> -- 
> Best Regards
>  Guo Ren
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2023-12-28  3:11 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-22  7:46 [RFC PATCH 0/4] Introduce & Optimize compat-mode helpers Leonardo Bras
2023-12-22  7:46 ` [RFC PATCH 1/4] riscv: Replace direct thread flag check with is_compat_task() Leonardo Bras
2023-12-22  7:46 ` [RFC PATCH 2/4] riscv: add compile-time test into is_compat_task() Leonardo Bras
2023-12-22  9:35   ` Guo Ren
2023-12-22 17:12     ` Leonardo Bras
2023-12-23  4:15       ` Andy Chiu
2023-12-22  7:46 ` [RFC PATCH 3/4] riscv: Introduce is_compat_thread() into compat.h Leonardo Bras
2023-12-22 11:20   ` Guo Ren
2023-12-22 17:14     ` Leonardo Bras
2023-12-23  4:25   ` Andy Chiu
     [not found]     ` <CAJ6HWG4Pe0co91-tRpV-gKoVUJkobB=uz8J-pjTvyX=Ed49u_w@mail.gmail.com>
2023-12-28  3:10       ` Andy Chiu
2023-12-22  7:46 ` [RFC PATCH 4/4] riscv: Introduce set_compat_task() in asm/compat.h Leonardo Bras
2023-12-22  8:11   ` Guo Ren
2023-12-22  8:28     ` Leonardo Bras

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox