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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, Dec 22, 2023 at 3:47 PM Leonardo Bras <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
--
Best Regards
Guo Ren
On Fri, Dec 22, 2023 at 04:11:10PM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 3:47 PM Leonardo Bras <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks!
Leo
>
> --
> Best Regards
> Guo Ren
>
On Fri, Dec 22, 2023 at 5:02 PM Leonardo Bras <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
--
Best Regards
Guo Ren
On Fri, Dec 22, 2023 at 3:47 PM Leonardo Bras <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
--
Best Regards
Guo Ren
On Fri, Dec 22, 2023 at 05:35:20PM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 5:02 PM Leonardo Bras <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks!
Leo
>
> --
> Best Regards
> Guo Ren
>
On Fri, Dec 22, 2023 at 07:20:36PM +0800, Guo Ren wrote:
> On Fri, Dec 22, 2023 at 3:47 PM Leonardo Bras <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks!
Leo
>
> --
> Best Regards
> Guo Ren
>
On Sat, Dec 23, 2023 at 1:12 AM Leonardo Bras <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> Thanks!
> Leo
>
> >
> > --
> > Best Regards
> > Guo Ren
> >
>
Reviewed-by: Andy Chiu <[email protected]>
On Fri, Dec 22, 2023 at 3:46 PM Leonardo Bras <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Sat, Dec 23, 2023 at 1:15 AM Andy Chiu <[email protected]> wrote:
>
> On Sat, Dec 23, 2023 at 1:12 AM Leonardo Bras <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> >
> > Thanks!
> > Leo
> >
> > >
> > > --
> > > Best Regards
> > > Guo Ren
> > >
> >
>
> Reviewed-by: Andy Chiu <[email protected]>
>
Thanks Andy!
On Sat, Dec 23, 2023 at 1:26 AM Andy Chiu <[email protected]> wrote:
>
> On Fri, Dec 22, 2023 at 3:46 PM Leonardo Bras <[email protected]> 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 <[email protected]>
> > ---
> > 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
>
> #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
> >
>
On Wed, Dec 27, 2023 at 3:25 AM Leonardo Bras Soares Passos
<[email protected]> wrote:
>
> On Sat, Dec 23, 2023 at 1:26 AM Andy Chiu <[email protected]> wrote:
> >
> > On Fri, Dec 22, 2023 at 3:46 PM Leonardo Bras <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
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
> > >
> >
>