On Fri, Oct 25, 2024 at 06:06:43PM +0800, Jinjie Ruan wrote:
> These changes refactor the entry and exit routines for the exceptions
> from EL1. They store the RCU and lockdep state in a struct
> irqentry_state variable on the stack, rather than recording them
> in the fields of pt_regs, since it is safe enough for these context.
In general, please descirbe *why* we want to make the change first, e.g.
| The generic entry code uses irqentry_state_t to track lockdep and RCU
| state across exception entry and return. For historical reasons, arm64
| embeds similar fields within its pt_regs structure.
|
| In preparation for moving arm64 over to the generic entry code, pull
| these fields out of arm64's pt_regs, and use a seperate structure,
| matching the style of the generic entry code.
> Before:
> struct pt_regs {
> ...
> u64 lockdep_hardirqs;
> u64 exit_rcu;
> }
>
> enter_from_kernel_mode(regs);
> ...
> exit_to_kernel_mode(regs);
>
> After:
> typedef struct irqentry_state {
> union {
> bool exit_rcu;
> bool lockdep;
> };
> } irqentry_state_t;
>
> irqentry_state_t state = enter_from_kernel_mode(regs);
> ...
> exit_to_kernel_mode(regs, state);
I don't think this part is necessary.
>
> No functional changes.
>
> Suggested-by: Mark Rutland <mark.rutl...@arm.com>
> Signed-off-by: Jinjie Ruan <ruanjin...@huawei.com>
> ---
> arch/arm64/include/asm/ptrace.h | 11 ++-
> arch/arm64/kernel/entry-common.c | 129 +++++++++++++++++++------------
> 2 files changed, 85 insertions(+), 55 deletions(-)
>
> diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
> index 3e5372a98da4..5156c0d5fa20 100644
> --- a/arch/arm64/include/asm/ptrace.h
> +++ b/arch/arm64/include/asm/ptrace.h
> @@ -149,6 +149,13 @@ static inline unsigned long pstate_to_compat_psr(const
> unsigned long pstate)
> return psr;
> }
>
> +typedef struct irqentry_state {
> + union {
> + bool exit_rcu;
> + bool lockdep;
> + };
> +} irqentry_state_t;
AFAICT this can be moved directly into arch/arm64/kernel/entry-common.c.
> +
> /*
> * This struct defines the way the registers are stored on the stack during
> an
> * exception. struct user_pt_regs must form a prefix of struct pt_regs.
> @@ -169,10 +176,6 @@ struct pt_regs {
>
> u64 sdei_ttbr1;
> struct frame_record_meta stackframe;
> -
> - /* Only valid for some EL1 exceptions. */
> - u64 lockdep_hardirqs;
> - u64 exit_rcu;
> };
>
> /* For correct stack alignment, pt_regs has to be a multiple of 16 bytes. */
> diff --git a/arch/arm64/kernel/entry-common.c
> b/arch/arm64/kernel/entry-common.c
> index c547e70428d3..68a9aecacdb9 100644
> --- a/arch/arm64/kernel/entry-common.c
> +++ b/arch/arm64/kernel/entry-common.c
> @@ -36,29 +36,36 @@
> * This is intended to match the logic in irqentry_enter(), handling the
> kernel
> * mode transitions only.
> */
> -static __always_inline void __enter_from_kernel_mode(struct pt_regs *regs)
> +static __always_inline irqentry_state_t __enter_from_kernel_mode(struct
> pt_regs *regs)
> {
> - regs->exit_rcu = false;
> + irqentry_state_t ret = {
> + .exit_rcu = false,
> + };
I realise that the generic entry code calls this 'ret' in
irqentry_enter() and similar, but could we please use 'state'
consistently in the arm64 code?
[...]
> /*
> @@ -190,9 +199,11 @@ asmlinkage void noinstr asm_exit_to_user_mode(struct
> pt_regs *regs)
> * mode. Before this function is called it is not safe to call regular kernel
> * code, instrumentable code, or any code which may trigger an exception.
> */
> -static void noinstr arm64_enter_nmi(struct pt_regs *regs)
> +static noinstr irqentry_state_t arm64_enter_nmi(struct pt_regs *regs)
> {
> - regs->lockdep_hardirqs = lockdep_hardirqs_enabled();
> + irqentry_state_t irq_state;
Likewise, please use 'state' rather than 'irq_state'.
In future we should probably have a separate structure for the NMI
paths, and get rid of the union, which would avoid the possiblity of
using mismatched helpers.
Mark.
