Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <26a42f0c-db3b-9330-de5c-68bf44976201@loongson.cn>
Date:   Wed, 26 Oct 2022 21:24:10 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.2
Subject: Re: [PATCH v2 RESEND] LoongArch: Remove unused kernel stack padding
To:     WANG Xuerui <kernel@xen0n.name>,
        Huacai Chen <chenhuacai@kernel.org>,
        Oleg Nesterov <oleg@redhat.com>
Cc:     Xi Ruoyao <xry111@xry111.site>, loongarch@lists.linux.dev,
        linux-kernel@vger.kernel.org
References: <20221026035501.11986-1-hejinyang@loongson.cn>
 <43126fe8-9040-a574-a149-273844d3073f@xen0n.name>
From:   Jinyang He <hejinyang@loongson.cn>
In-Reply-To: <43126fe8-9040-a574-a149-273844d3073f@xen0n.name>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@loongson.cn/
>>
>
> I think this blank line between the "Link" and S-o-b tags could be 
> removed.

Ok.


>
>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>> ---
>> 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@xen0n.name>


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