Re: [PATCH v2 RESEND] LoongArch: Remove unused kernel stack padding

[Jinyang He]

Hi, Xuerui,


在 2022/10/26 15:27, WANG Xuerui 写道:
> On 2022/10/26 11:55, Jinyang He wrote:
> > Kernel stack padding looks like obey MIPS o32 Calling Convention, as
> > LoongArch is inspired by MIPS and keep it. Remove it avoid not clear
> > code.
>
> Just some improvement to the commit message so it's clearer:
>
> "The current LoongArch kernel stack is padded as if obeying the MIPS 
> o32 calling convention, signifying the port's MIPS lineage but no 
> longer making sense. Remove the padding for clarity."

Thanks, I'll improve it and send it later.


> > Link: 
> > https://lore.kernel.org/loongarch/1662130897-13156-1-git-send-email-hejinyang@xxxxxxxxxxx/
>
> I think this blank line between the "Link" and S-o-b tags could be 
> removed.

Ok.


> > Signed-off-by: Jinyang He <hejinyang@xxxxxxxxxxx>
> > ---
> > v2: Remove TOP_OF_KERNEL_STACK_PADDING
> >      Remove 'init stack pointer' in head.S
> >
> >   arch/loongarch/include/asm/processor.h | 2 +-
> >   arch/loongarch/include/asm/ptrace.h    | 2 +-
> >   arch/loongarch/kernel/head.S           | 3 +--
> >   arch/loongarch/kernel/process.c        | 4 ++--
> >   arch/loongarch/kernel/switch.S         | 2 +-
> >   5 files changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/loongarch/include/asm/processor.h 
> > b/arch/loongarch/include/asm/processor.h
> > index 6954dc5d24e9..7184f1dc61f2 100644
> > --- a/arch/loongarch/include/asm/processor.h
> > +++ b/arch/loongarch/include/asm/processor.h
> > @@ -191,7 +191,7 @@ static inline void flush_thread(void)
> >   unsigned long __get_wchan(struct task_struct *p);
> >     #define __KSTK_TOS(tsk) ((unsigned long)task_stack_page(tsk) + \
> > -             THREAD_SIZE - 32 - sizeof(struct pt_regs))
> > +             THREAD_SIZE - sizeof(struct pt_regs))
> >   #define task_pt_regs(tsk) ((struct pt_regs *)__KSTK_TOS(tsk))
> >   #define KSTK_EIP(tsk) (task_pt_regs(tsk)->csr_era)
> >   #define KSTK_ESP(tsk) (task_pt_regs(tsk)->regs[3])
> > diff --git a/arch/loongarch/include/asm/ptrace.h 
> > b/arch/loongarch/include/asm/ptrace.h
> > index 7437b9366c3b..59c4608de91d 100644
> > --- a/arch/loongarch/include/asm/ptrace.h
> > +++ b/arch/loongarch/include/asm/ptrace.h
> > @@ -133,7 +133,7 @@ static inline void die_if_kernel(const char *str, 
> > struct pt_regs *regs)
> >   #define current_pt_regs()                        \
> >   ({                                    \
> >       unsigned long sp = (unsigned long)__builtin_frame_address(0);    \
> > -    (struct pt_regs *)((sp | (THREAD_SIZE - 1)) + 1 - 32) - 1;    \
> > +    (struct pt_regs *)((sp | (THREAD_SIZE - 1)) + 1) - 1;        \
> >   })
> >     /* Helpers for working with the user stack pointer */
> > diff --git a/arch/loongarch/kernel/head.S b/arch/loongarch/kernel/head.S
> > index 97425779ce9f..84970e266658 100644
> > --- a/arch/loongarch/kernel/head.S
> > +++ b/arch/loongarch/kernel/head.S
> > @@ -84,10 +84,9 @@ SYM_CODE_START(kernel_entry)            # kernel 
> > entry point
> >         la.pcrel    tp, init_thread_union
> >       /* Set the SP after an empty pt_regs.  */
> > -    PTR_LI        sp, (_THREAD_SIZE - 32 - PT_SIZE)
> > +    PTR_LI        sp, (_THREAD_SIZE - PT_SIZE)
> >       PTR_ADD        sp, sp, tp
> >       set_saved_sp    sp, t0, t1
> > -    PTR_ADDI    sp, sp, -4 * SZREG    # init stack pointer
> >         bl        start_kernel
> >       ASM_BUG()
> > diff --git a/arch/loongarch/kernel/process.c 
> > b/arch/loongarch/kernel/process.c
> > index 1256e3582475..2526b68f1c0f 100644
> > --- a/arch/loongarch/kernel/process.c
> > +++ b/arch/loongarch/kernel/process.c
> > @@ -129,7 +129,7 @@ int copy_thread(struct task_struct *p, const 
> > struct kernel_clone_args *args)
> >       unsigned long clone_flags = args->flags;
> >       struct pt_regs *childregs, *regs = current_pt_regs();
> >   -    childksp = (unsigned long)task_stack_page(p) + THREAD_SIZE - 32;
> > +    childksp = (unsigned long)task_stack_page(p) + THREAD_SIZE;
> >         /* set up new TSS. */
> >       childregs = (struct pt_regs *) childksp - 1;
> > @@ -236,7 +236,7 @@ bool in_task_stack(unsigned long stack, struct 
> > task_struct *task,
> >               struct stack_info *info)
> >   {
> >       unsigned long begin = (unsigned long)task_stack_page(task);
> > -    unsigned long end = begin + THREAD_SIZE - 32;
> > +    unsigned long end = begin + THREAD_SIZE;
> >         if (stack < begin || stack >= end)
> >           return false;
> > diff --git a/arch/loongarch/kernel/switch.S 
> > b/arch/loongarch/kernel/switch.S
> > index 43ebbc3990f7..202a163cb32f 100644
> > --- a/arch/loongarch/kernel/switch.S
> > +++ b/arch/loongarch/kernel/switch.S
> > @@ -26,7 +26,7 @@ SYM_FUNC_START(__switch_to)
> >       move    tp, a2
> >       cpu_restore_nonscratch a1
> >   -    li.w        t0, _THREAD_SIZE - 32
> > +    li.w        t0, _THREAD_SIZE
> >       PTR_ADD        t0, t0, tp
> >       set_saved_sp    t0, t1, t2
>
> Otherwise LGTM, assuming you have tested it before sending (I don't 
> currently have the capacity to immediately test this for you)...
>
> Reviewed-by: WANG Xuerui <git@xxxxxxxxxx>


Emm, to be honest, I didn't do tests like LTP. I ususally do development
on CLFS-LoongArch while kernel has this patch. And it works normally.


Thanks,

Jinyang