| Message ID | 20230625155416.18629-1-andy.chiu@sifive.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 26c38cd802c947401cfbcc285b7d841256b5f17f |
| Headers | show |
| Series | riscv: vector: only enable interrupts in the first-use trap | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Single patches do not need cover letters |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD 4681dacadeef |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 6 and now 6 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 8 this patch: 8 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 8 this patch: 8 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 20 this patch: 20 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 19 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | Fixes tag looks correct |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
Andy Chiu <andy.chiu@sifive.com> writes: > 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@sifive.com> Good catch, Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
On Sun, Jun 25, 2023 at 11:54 AM Andy Chiu <andy.chiu@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@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 >
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.
On Thu, Jun 29, 2023 at 2:04 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Sun, Jun 25, 2023 at 11:54 AM Andy Chiu <andy.chiu@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@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
On Sun, 25 Jun 2023 15:54:15 +0000, Andy Chiu 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. > > Applied, thanks! [1/1] riscv: vector: only enable interrupts in the first-use trap https://git.kernel.org/palmer/c/26c38cd802c9 Best regards,
Hello: This patch was applied to riscv/linux.git (for-next) by Palmer Dabbelt <palmer@rivosinc.com>: On Sun, 25 Jun 2023 15:54:15 +0000 you 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@sifive.com> > > [...] Here is the summary with links: - riscv: vector: only enable interrupts in the first-use trap https://git.kernel.org/riscv/c/26c38cd802c9 You are awesome, thank you!
Hi Guo, On Thu, Jun 29, 2023 at 2:12 PM Guo Ren <guoren@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@kernel.org> wrote: > > > > On Sun, Jun 25, 2023 at 11:54 AM Andy Chiu <andy.chiu@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@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
On Wed, Jul 5, 2023 at 11:39 AM Andy Chiu <andy.chiu@sifive.com> wrote: > > Hi Guo, > > On Thu, Jun 29, 2023 at 2:12 PM Guo Ren <guoren@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@kernel.org> wrote: > > > > > > On Sun, Jun 25, 2023 at 11:54 AM Andy Chiu <andy.chiu@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@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
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); + + local_irq_disable(); + + if (!handled) do_trap_error(regs, SIGILL, ILL_ILLOPC, regs->epc, "Oops - illegal instruction");
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@sifive.com> --- arch/riscv/kernel/traps.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)