On 2025/8/5 23:07, Ada Couprie Diaz wrote:
> Hi Jinjie,
>
> The code changes look good to me, just a small missing clean up I believe.
>
> On 29/07/2025 02:54, 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. The next patch serise 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.
>
> I think the commit message could be clearer, especially since now this
> series
> only moves arm64 to generic IRQ entry and not the complete generic entry.
>
> What do you think of something like below ? It repeats a bit less and I
> think
> it helps understanding what is going on in this specific commit, as you
> already
> have details on the larger plans in the cover.
>
> Currently, x86, Riscv and Loongarch use the generic entry code,
> which makes
> maintainer's work easier and code more elegant.
> Start converting arm64 to use the generic entry infrastructure
> from kernel/entry/* by switching it to generic IRQ entry, which
> removes 100+
> lines of duplicate code.
> arm64 will completely switch to generic entry in a later series.
>
Yes, this is more concise and accurate, and make the motivation more
clearer.
>> 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.
> Nit : "so far" can be removed
>> - 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().
> This feels unrelated, given that the part that needs
> `arch_irqentry_exit_need_resched()`
> is called whether or not PREEMPT_DYNAMIC is enabled ?
Yes, the language here needs to be reorganized in conjunction with your
comments from the fifth patch.
>
> Given my comments on patch 5, I feel that the commit message should mention
> explicitly the implementation of `arch_irqentry_exit_need_resched()` and
> why,
> even though it was already mentioned in patch 5.
> (This is what I was referencing in patch 5 : as I feel it's useful to
> mention again
> the reasons when implementing it, it doesn't feel too out of place to
> introduce
> the generic part at the same time. But again, I might be wrong here.)
>
> Then you can have another point explaining that
> `raw_irqentry_exit_cond_resched()`
> and the PREEMPT_DYNAMIC code is removed because they are identical to the
> generic entry code, similarly to your other points.
>> Tested ok with following test cases on Qemu virt platform:
>> - Perf tests.
>> - Different `dynamic preempt` mode switch.
>> - Pseudo NMI tests.
>> - Stress-ng CPU stress test.
>> - MTE test case in
>> Documentation/arch/arm64/memory-tagging-extension.rst
>> and all test cases in tools/testing/selftests/arm64/mte/*.
> Nit : I'm not sure if the commit message is the best place for this,
> given you
> already gave some details in the cover ?
> But I don't have much experience here, so I'll leave it up to you and
> others !
Yes, this can be removed as the cover letter already has it.
>> Suggested-by: Mark Rutland <mark.rutl...@arm.com>
>> Signed-off-by: Jinjie Ruan <ruanjin...@huawei.com>
>> ---
>> [...]
>> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
>> index db3f972f8cd9..1110eeb21f57 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>
>> @@ -1576,7 +1577,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)
> Given that `do_signal(struct pt_regs *regs)` is defined in
> `arch/arm64/include/asm/exception.h`,
> and that there remains no users of `do_signal()`, I think it should be
> removed there.
Good catch! I'll remove it.
>
> Thanks,
> Ada
>
