Re: [PATCH -next v5 11/22] arm64: entry: Switch to generic IRQ entry

[Jinjie Ruan <ruanjinjie@xxxxxxxxxx>]

On 2025/2/10 20:24, Mark Rutland wrote:
> On Fri, Dec 06, 2024 at 06:17:33PM +0800, Jinjie Ruan wrote:
>> Currently, x86, Riscv, Loongarch use the generic entry. Convert arm64
>> to use the generic entry infrastructure from kernel/entry/*.
>> The generic entry makes maintainers' work easier and codes
>> more elegant.
>>
>> Switch arm64 to generic IRQ entry first, which removed duplicate 100+
>> LOC, and it will switch to generic entry completely later. Switch to
>> generic entry in two steps according to Mark's suggestion will make
>> it easier to review.
>>
>> The changes are below:
>>  - Remove *enter_from/exit_to_kernel_mode(), and wrap with generic
>>    irqentry_enter/exit(). Also remove *enter_from/exit_to_user_mode(),
>>    and wrap with generic enter_from/exit_to_user_mode() because they
>>    are exactly the same so far.
>>
>>  - Remove arm64_enter/exit_nmi() and use generic irqentry_nmi_enter/exit()
>>    because they're exactly the same, so the temporary arm64 version
>>    irqentry_state can also be removed.
>>
>>  - Remove PREEMPT_DYNAMIC code, as generic entry do the same thing
>>    if arm64 implement arch_irqentry_exit_need_resched().
>>
>> Suggested-by: Mark Rutland <mark.rutland@xxxxxxx>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
>> ---
>>  arch/arm64/Kconfig                    |   1 +
>>  arch/arm64/include/asm/entry-common.h |  64 ++++++
>>  arch/arm64/include/asm/preempt.h      |   6 -
>>  arch/arm64/kernel/entry-common.c      | 307 ++++++--------------------
>>  arch/arm64/kernel/signal.c            |   3 +-
>>  5 files changed, 129 insertions(+), 252 deletions(-)
>>  create mode 100644 arch/arm64/include/asm/entry-common.h
> 
> Superficially this looks nice, but to be clear I have *not* looked at
> this in great detail; minor comments below.
> 
> [...]
> 
>> +static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
>> +                                              unsigned long ti_work)
>> +{
>> +    local_daif_mask();
>> +}
>> +
>> +#define arch_exit_to_user_mode_prepare arch_exit_to_user_mode_prepare
> 
> I'm a little worried that this may be fragile having been hidden in the
> common code, as it's not clear exactly when this will occur during the
> return sequence, and the ordering requirements could easily be broken by
> refactoring there.
> 
> I suspect we'll want to pull this later in the arm64 exit sequence so
> that we can have it explicit in entry-common.c.

Yes, this key function is hidden in generic entry code and is not easy
to clear and see when it is executed. But placing it directly in
entry-common.c in arm64 may change the order in which lockdep_sys_exit()
and local_daif_mask() are called, it's not clear what the potential
impact is.

Before:
   exit_to_user_mode_prepare()
      ...
      -> local_daif_mask()
      -> lockdep_sys_exit()


arm64_exit_to_user_mode()
  ...
  -> exit_to_user_mode_prepare()
     -> lockdep_sys_exit()
  -> local_daif_mask()

> 
> [...]
> 
>> index 14ac6fdb872b..84b6628647c7 100644
>> --- a/arch/arm64/kernel/signal.c
>> +++ b/arch/arm64/kernel/signal.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/cache.h>
>>  #include <linux/compat.h>
>>  #include <linux/errno.h>
>> +#include <linux/irq-entry-common.h>
>>  #include <linux/kernel.h>
>>  #include <linux/signal.h>
>>  #include <linux/freezer.h>
>> @@ -1603,7 +1604,7 @@ static void handle_signal(struct ksignal *ksig, struct 
>> pt_regs *regs)
>>   * the kernel can handle, and then we build all the user-level signal 
>> handling
>>   * stack-frames in one go after that.
>>   */
>> -void do_signal(struct pt_regs *regs)
>> +void arch_do_signal_or_restart(struct pt_regs *regs)
>>  {
>>      unsigned long continue_addr = 0, restart_addr = 0;
>>      int retval = 0;
> 
> Is the expected semantic the same here, or is those more than just a
> name change?

Yes, the expected semantic is the same here, they both handle
_TIF_SIGPENDING and _TIF_NOTIFY_SIGNAL thread flags before
exit to user.

In arm64 the code call sequence is:

  exit_to_user_mode()
     -> exit_to_user_mode_prepare()
        -> do_notify_resume(regs, flags)
           -> if (thread_flags & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL))
                 do_signal(regs)

In generic entry code, the logic is the same:

  exit_to_user_mode_prepare()
      -> exit_to_user_mode_loop()
          -> if (ti_work & (_TIF_SIGPENDING | _TIF_NOTIFY_SIGNAL))
                 arch_do_signal_or_restart(regs)

> 
> Mark.
>