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

Jinjie Ruan
2025-02-11T11:34:54Z

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@arm.com>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>> 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.
>