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:     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>
From:   Jinyang He <hejinyang@loongson.cn>
Message-ID: <8eaea09e-67b0-5e51-4632-2c31a4c56a3e@loongson.cn>
Date:   Tue, 27 Dec 2022 17:30:19 +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: <CAAhV-H6VX=d1oymY0rh-mwOPMuBsYd7C9RzQat8BmmnrJ7apng@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


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.


>   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?


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
>>