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

[Andy Chiu]

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

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