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;
Subject: Re: [PATCH] LoongArch: Fix irq enable in exception handlers
To:     Qi Hu <huqi@loongson.cn>, Huacai Chen <chenhuacai@kernel.org>
Cc:     WANG Xuerui <kernel@xen0n.name>,
        Tiezhu Yang <yangtiezhu@loongson.cn>,
        loongarch@lists.linux.dev, linux-kernel@vger.kernel.org
References: <20221221074238.6699-1-hejinyang@loongson.cn>
 <CAAhV-H6VX=d1oymY0rh-mwOPMuBsYd7C9RzQat8BmmnrJ7apng@mail.gmail.com>
 <8eaea09e-67b0-5e51-4632-2c31a4c56a3e@loongson.cn>
 <CAAhV-H7maW1YWhfvCzYVix=aCMMFbx0KkGodaBjka4ThunBO5Q@mail.gmail.com>
 <1c578efd-ae8a-2d80-e505-a09b1a2f7eaa@loongson.cn>
 <b267cab1-3df4-6263-9c83-0061f20dd4a4@loongson.cn>
From:   Jinyang He <hejinyang@loongson.cn>
Message-ID: <2364400d-bb1e-cbe2-8a65-adbe87186938@loongson.cn>
Date:   Thu, 29 Dec 2022 14:13:51 +0800
User-Agent: Mozilla/5.0 (X11; Linux loongarch64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <b267cab1-3df4-6263-9c83-0061f20dd4a4@loongson.cn>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 2022-12-29 00:51, Qi Hu wrote:

>
> On 2022/12/27 18:10, Jinyang He wrote:
>> On 2022-12-27 17:52, Huacai Chen wrote:
>>
>>> On Tue, Dec 27, 2022 at 5:30 PM Jinyang He <hejinyang@loongson.cn> 
>>> wrote:
>>>>
>>>> On 2022-12-27 15:37, Huacai Chen wrote:
>>>>> Hi, Jinyang,
>>>>>
>>>>> Move die_if_kernel to irq disabled context to solve what?
>>>> For more strict logical. If the code flow go to die in 
>>>> die_if_kernel(),
>>>> its interrupt state is enable, that means it may cause schedule.
>>>> So I think it is better to call die_if_kernel() firstly.
>>> die_if_kernel is called with irq enabled in old kernels for several
>>> years, and has no problems.
>>
>>
>> I think because it never call die() in die_if_kernel(). What I do
>> emphasize is that there needs to be more strict logic here than
>> it worked well in the past. I bet if die_if_kernel() was removed,
>> it will still work well in the future.
>>
>>
>>>
>>>>
>>>>>    And LBT is
>>>>> surely allowed to be triggered in kernel context.
>>>> I'm not familar with lbt, I just not see any lbt codes in kernel. Plz,
>>>> how lbt exception triggered, and how kernel trigger lbt exception?
>>> You can ask Huqi for more details, and this was discussed publicly 
>>> last week.
>>
>> To: Qi Hu
>>
>>
>> Hi,
>>
>>
>> We really need some help. Can you give us some ideas?
>>
>>
>> Thanks,
>>
>> Jinyang
>>
> Huacai is correct. The LBT disable exception (BTD) can be triggered in 
> kernel context.
>
> If the CSR.ENEU.BTE == 0 [^1], the LBT instructions (these [^2] will 
> be used in the kernel) will trigger the exception.
>
> Unfortunately, when you want to do some fpu_{save, restore}, you need 
> to use some LBT instructions [^3] [^4]. So if FPD is triggered, LBT 
> might still not be enabled, and the 'do_lbt' will be called in the 
> kernel context.
>
> Hope the information can help. Thanks.
>
>
> [1] 
> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#extended-component-unit-enable
>
> [2] 
> https://github.com/loongson/linux/pull/4/files#diff-381d03cf86e2796d280e2fc82c005409d5e44b4bbbf90dd0dc17f5f0fa5553f1R140-R184
>
> [3] 
> https://github.com/loongson/linux/pull/4/files#diff-381d03cf86e2796d280e2fc82c005409d5e44b4bbbf90dd0dc17f5f0fa5553f1R218-R230
>
> [4] 
> https://github.com/loongson/linux/pull/4/files#diff-381d03cf86e2796d280e2fc82c005409d5e44b4bbbf90dd0dc17f5f0fa5553f1R236-R263
>
>
Hi,


That's helpful. Thanks!


But I still wonder if SXD or ASXD have the same possibility of being 
triggered in the kernel mode by sc_save_{lsx, lasx} or other place. Do 
we need remove these die_if_kernel codes in do_lasx() and do_lsx()?


Jinyang


> Qi
>
>>
>>> Huacai
>>>>
>>>> Thanks,
>>>>
>>>> Jinyang
>>>>
>>>>
>>>>> Huacai
>>>>>
>>>>> On Wed, Dec 21, 2022 at 3:43 PM Jinyang He <hejinyang@loongson.cn> 
>>>>> wrote:
>>>>>> The interrupt state can be got by regs->csr_prmd. Once previous
>>>>>> interrupt state is disable, we shouldn't enable interrupt if we
>>>>>> triggered exception which can be triggered in kernel mode. So
>>>>>> conditionally enable interrupt. For those do_\exception which
>>>>>> can not triggered in kernel mode but need enable interrupt, call
>>>>>> die_if_kernel() firstly. And for do_lsx, do_lasx and do_lbt cannot
>>>>>> triggered in kernel mode, too.
>>>>>>
>>>>>> Signed-off-by: Jinyang He <hejinyang@loongson.cn>
>>>>>> ---
>>>>>>    arch/loongarch/kernel/traps.c | 19 ++++++++++---------
>>>>>>    1 file changed, 10 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/loongarch/kernel/traps.c 
>>>>>> b/arch/loongarch/kernel/traps.c
>>>>>> index 1ea14f6c18d3..3ac7b32d1e15 100644
>>>>>> --- a/arch/loongarch/kernel/traps.c
>>>>>> +++ b/arch/loongarch/kernel/traps.c
>>>>>> @@ -340,9 +340,9 @@ asmlinkage void noinstr do_fpe(struct pt_regs 
>>>>>> *regs, unsigned long fcsr)
>>>>>>
>>>>>>           /* Clear FCSR.Cause before enabling interrupts */
>>>>>>           write_fcsr(LOONGARCH_FCSR0, fcsr & ~mask_fcsr_x(fcsr));
>>>>>> -       local_irq_enable();
>>>>>>
>>>>>>           die_if_kernel("FP exception in kernel code", regs);
>>>>>> +       local_irq_enable();
>>>>>>
>>>>>>           sig = SIGFPE;
>>>>>>           fault_addr = (void __user *) regs->csr_era;
>>>>>> @@ -432,7 +432,8 @@ asmlinkage void noinstr do_bp(struct pt_regs 
>>>>>> *regs)
>>>>>>           unsigned long era = exception_era(regs);
>>>>>>           irqentry_state_t state = irqentry_enter(regs);
>>>>>>
>>>>>> -       local_irq_enable();
>>>>>> +       if (regs->csr_prmd & CSR_PRMD_PIE)
>>>>>> +               local_irq_enable();
>>>>>>           current->thread.trap_nr = read_csr_excode();
>>>>>>           if (__get_inst(&opcode, (u32 *)era, user))
>>>>>>                   goto out_sigsegv;
>>>>>> @@ -514,7 +515,8 @@ asmlinkage void noinstr do_ri(struct pt_regs 
>>>>>> *regs)
>>>>>>           unsigned int __user *era = (unsigned int __user 
>>>>>> *)exception_era(regs);
>>>>>>           irqentry_state_t state = irqentry_enter(regs);
>>>>>>
>>>>>> -       local_irq_enable();
>>>>>> +       if (regs->csr_prmd & CSR_PRMD_PIE)
>>>>>> +               local_irq_enable();
>>>>>>           current->thread.trap_nr = read_csr_excode();
>>>>>>
>>>>>>           if (notify_die(DIE_RI, "RI Fault", regs, 0, 
>>>>>> current->thread.trap_nr,
>>>>>> @@ -606,8 +608,8 @@ asmlinkage void noinstr do_fpu(struct pt_regs 
>>>>>> *regs)
>>>>>>    {
>>>>>>           irqentry_state_t state = irqentry_enter(regs);
>>>>>>
>>>>>> -       local_irq_enable();
>>>>>>           die_if_kernel("do_fpu invoked from kernel context!", 
>>>>>> regs);
>>>>>> +       local_irq_enable();
>>>>>>           BUG_ON(is_lsx_enabled());
>>>>>>           BUG_ON(is_lasx_enabled());
>>>>>>
>>>>>> @@ -623,13 +625,13 @@ asmlinkage void noinstr do_lsx(struct 
>>>>>> pt_regs *regs)
>>>>>>    {
>>>>>>           irqentry_state_t state = irqentry_enter(regs);
>>>>>>
>>>>>> +       die_if_kernel("do_lsx invoked from kernel context!", regs);
>>>>>>           local_irq_enable();
>>>>>>           if (!cpu_has_lsx) {
>>>>>>                   force_sig(SIGILL);
>>>>>>                   goto out;
>>>>>>           }
>>>>>>
>>>>>> -       die_if_kernel("do_lsx invoked from kernel context!", regs);
>>>>>>           BUG_ON(is_lasx_enabled());
>>>>>>
>>>>>>           preempt_disable();
>>>>>> @@ -645,14 +647,13 @@ asmlinkage void noinstr do_lasx(struct 
>>>>>> pt_regs *regs)
>>>>>>    {
>>>>>>           irqentry_state_t state = irqentry_enter(regs);
>>>>>>
>>>>>> +       die_if_kernel("do_lasx invoked from kernel context!", regs);
>>>>>>           local_irq_enable();
>>>>>>           if (!cpu_has_lasx) {
>>>>>>                   force_sig(SIGILL);
>>>>>>                   goto out;
>>>>>>           }
>>>>>>
>>>>>> -       die_if_kernel("do_lasx invoked from kernel context!", regs);
>>>>>> -
>>>>>>           preempt_disable();
>>>>>>           init_restore_lasx();
>>>>>>           preempt_enable();
>>>>>> @@ -666,6 +667,7 @@ asmlinkage void noinstr do_lbt(struct pt_regs 
>>>>>> *regs)
>>>>>>    {
>>>>>>           irqentry_state_t state = irqentry_enter(regs);
>>>>>>
>>>>>> +       die_if_kernel("do_lbt invoked from kernel context!", regs);
>>>>>>           local_irq_enable();
>>>>>>           force_sig(SIGILL);
>>>>>>           local_irq_disable();
>>>>>> @@ -677,7 +679,6 @@ asmlinkage void noinstr do_reserved(struct 
>>>>>> pt_regs *regs)
>>>>>>    {
>>>>>>           irqentry_state_t state = irqentry_enter(regs);
>>>>>>
>>>>>> -       local_irq_enable();
>>>>>>           /*
>>>>>>            * Game over - no way to handle this if it ever occurs. 
>>>>>> Most probably
>>>>>>            * caused by a fatal error after another 
>>>>>> hardware/software error.
>>>>>> @@ -685,8 +686,8 @@ asmlinkage void noinstr do_reserved(struct 
>>>>>> pt_regs *regs)
>>>>>>           pr_err("Caught reserved exception %u on pid:%d [%s] - 
>>>>>> should not happen\n",
>>>>>>                   read_csr_excode(), current->pid, current->comm);
>>>>>>           die_if_kernel("do_reserved exception", regs);
>>>>>> +       local_irq_enable();
>>>>>>           force_sig(SIGUNUSED);
>>>>>> -
>>>>>>           local_irq_disable();
>>>>>>
>>>>>>           irqentry_exit(regs, state);
>>>>>> -- 
>>>>>> 2.34.3
>>>>>>
>>>>
>>
>