[PATCH] riscv: vector: only enable interrupts in the first-use trap

[Guo Ren]

On Wed, Jul 5, 2023 at 11:39 AM Andy Chiu <andy.chiu at sifive.com> wrote:
>
> Hi Guo,
>
> On Thu, Jun 29, 2023 at 2:12 PM Guo Ren <guoren at kernel.org> wrote:
> >
> > Another one:
> > diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> > index f9c8e19ab301..764b7c098165 100644
> > --- a/arch/riscv/kernel/vector.c
> > +++ b/arch/riscv/kernel/vector.c
> > @@ -84,7 +84,7 @@ static int riscv_v_thread_zalloc(void)
> >  {
> >         void *datap;
> >
> >         datap = kzalloc(riscv_v_vsize, GFP_KERNEL);
> >         if (!datap)
> >                 return -ENOMEM;
> >
> > @@ -162,10 +162,9 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
> >          * context where VS has been off. So, try to allocate the user's V
> >          * context and resume execution.
> >          */
> > -       if (riscv_v_thread_zalloc()) {
> > -               force_sig(SIGBUS);
> > -               return true;
> > -       }
> > +       if (riscv_v_thread_zalloc())
> > +               return false;
> > +
> >         riscv_v_vstate_on(regs);
> >         return true;
> >  }
> > --------
> >
> > Your force_sig throws the debug info away, and the standard one is
> > enough for us.
>
> Could you help me to understand more on how this throws debug info
> away? Do you mean the pr_info() print in the do_trap() function? If
Yes, you code just send a sig, but no debug info.

> we'd want to use the standard one then we should instead return a
> signal number in riscv_v_first_use_handler(). Because we want to send
> SIGBUS instead of SIGILL.

Do we really need a SIGBUS here?
When we can't handle this instruction, treat it as a invalid one.
SIGILL  - invalid program image, such as invalid instruction
SIGBUS - abbreviation for “Bus Error”.


>
> >
> > On Thu, Jun 29, 2023 at 2:04 AM Guo Ren <guoren at kernel.org> wrote:
> > >
> > > On Sun, Jun 25, 2023 at 11:54 AM Andy Chiu <andy.chiu at sifive.com> wrote:
> > > >
> > > > The function irqentry_exit_to_user_mode() must be called with interrupt
> > > > disabled. The caller of do_trap_insn_illegal() also assumes running
> > > > without interrupts. So, we should turn off interrupts after
> > > > riscv_v_first_use_handler() returns.
> > > >
> > > > Fixes: cd054837243b ("riscv: Allocate user's vector context in the first-use trap")
> > > > Signed-off-by: Andy Chiu <andy.chiu at sifive.com>
> > > > ---
> > > >  arch/riscv/kernel/traps.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> > > > index 05ffdcd1424e..1595e246bda1 100644
> > > > --- a/arch/riscv/kernel/traps.c
> > > > +++ b/arch/riscv/kernel/traps.c
> > > > @@ -149,12 +149,18 @@ DO_ERROR_INFO(do_trap_insn_fault,
> > > >
> > > >  asmlinkage __visible __trap_section void do_trap_insn_illegal(struct pt_regs *regs)
> > > >  {
> > > > +       bool handled;
> > > > +
> > > >         if (user_mode(regs)) {
> > > >                 irqentry_enter_from_user_mode(regs);
> > > >
> > > >                 local_irq_enable();
> > > >
> > > > -               if (!riscv_v_first_use_handler(regs))
> > > > +               handled = riscv_v_first_use_handler(regs);
> > > How about making riscv_v_first_use_handler irq_disable safe?
> > > --------
> > > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> > > index 5158961ea977..545295045705 100644
> > > --- a/arch/riscv/kernel/traps.c
> > > +++ b/arch/riscv/kernel/traps.c
> > > @@ -153,8 +153,6 @@ asmlinkage __visible __trap_section void
> > > do_trap_insn_illegal(struct pt_regs *re
> > >         if (user_mode(regs)) {
> > >                 irqentry_enter_from_user_mode(regs);
> > >
> > > -               local_irq_enable();
> > > -
> > >                 if (!riscv_v_first_use_handler(regs))
> > >                         do_trap_error(regs, SIGILL, ILL_ILLOPC, regs->epc,
> > >                                       "Oops - illegal instruction");
> > > diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
> > > index f9c8e19ab301..7616c027ee64 100644
> > > --- a/arch/riscv/kernel/vector.c
> > > +++ b/arch/riscv/kernel/vector.c
> > > @@ -84,7 +84,7 @@ static int riscv_v_thread_zalloc(void)
> > >  {
> > >         void *datap;
> > >
> > > -       datap = kzalloc(riscv_v_vsize, GFP_KERNEL);
> > > +       datap = kzalloc(riscv_v_vsize, GFP_KERNEL | GFP_ATOMIC);
> > >         if (!datap)
> > >                 return -ENOMEM;
> > > ---------
> > >
> > > > +
> > > > +               local_irq_disable();
> > > > +
> > > > +               if (!handled)
> > > >                         do_trap_error(regs, SIGILL, ILL_ILLOPC, regs->epc,
> > > >                                       "Oops - illegal instruction");
> > > >
> > > > --
> > > > 2.17.1
> > > >
> > >
> > >
> > > --
> > > Best Regards
> > >  Guo Ren
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren
>
> Thanks,
> Andy



--
Best Regards
 Guo Ren