[v2,24/30] linux-user/openrisc: Use force_sig_fault, force_sigsegv_for_addr

Use the new functions instead of setting up a target_siginfo_t
and calling queue_signal.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 linux-user/openrisc/cpu_loop.c | 37 +++++++++-------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

-- 
2.25.1

On Sun, 22 Aug 2021 at 04:55, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Use the new functions instead of setting up a target_siginfo_t

> and calling queue_signal.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  linux-user/openrisc/cpu_loop.c | 37 +++++++++-------------------------

>  1 file changed, 10 insertions(+), 27 deletions(-)

>

> diff --git a/linux-user/openrisc/cpu_loop.c b/linux-user/openrisc/cpu_loop.c

> index b33fa77718..d2632ce6a3 100644

> --- a/linux-user/openrisc/cpu_loop.c

> +++ b/linux-user/openrisc/cpu_loop.c

> @@ -21,13 +21,14 @@

>  #include "qemu-common.h"

>  #include "qemu.h"

>  #include "cpu_loop-common.h"

> +#include "signal-common.h"

> +

>

>  void cpu_loop(CPUOpenRISCState *env)

>  {

>      CPUState *cs = env_cpu(env);

>      int trapnr;

>      abi_long ret;

> -    target_siginfo_t info;

>

>      for (;;) {

>          cpu_exec_start(cs);

> @@ -54,42 +55,24 @@ void cpu_loop(CPUOpenRISCState *env)

>              break;

>          case EXCP_DPF:

>          case EXCP_IPF:

> +            force_sigsegv_for_addr(env->eear);

> +            break;

>          case EXCP_RANGE:

> -            info.si_signo = TARGET_SIGSEGV;

> -            info.si_errno = 0;

> -            info.si_code = TARGET_SEGV_MAPERR;

> -            info._sifields._sigfault._addr = env->pc;

> -            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);

> +        case EXCP_FPE:

> +            /* ??? The kernel vectors both of these to unhandled_exception. */


I think that EXCP_RANGE should for us be unreachable in user-only
mode (because it can only happen if the relevant bits in SR are
set, and SR is writeable only in supervisor mode, and its starting
value doesn't set these bits). So we could just delete the EXCP_RANGE
handling and let it hit the default g_assert_not_reached() case.

EXCP_FPE is more tricky -- this happens for FP exceptions, where
the enabling bit is in the FPCSR, which does appear to be writeable
from user mode. So either:
 * our mtspr is wrong and should either be not allowing writes
   to FPCSR in usermode (or at least sanitizing them)
 * the Linux kernel for openrisc is wrong, because a userspace
   program that sets FPCSR.FPEE can make it run into unhandled_exception()
   and die(), and it should be doing something else, like delivering
   a suitable SIGFPE

> +            force_sig(TARGET_SIGSEGV);

>              break;

>          case EXCP_ALIGN:

> -            info.si_signo = TARGET_SIGBUS;

> -            info.si_errno = 0;

> -            info.si_code = TARGET_BUS_ADRALN;

> -            info._sifields._sigfault._addr = env->pc;

> -            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);

> +            force_sig_fault(TARGET_SIGBUS, TARGET_BUS_ADRALN, env->eear);


So we were reporting completely the wrong address previously ?

>              break;

>          case EXCP_ILLEGAL:

> -            info.si_signo = TARGET_SIGILL;

> -            info.si_errno = 0;

> -            info.si_code = TARGET_ILL_ILLOPC;

> -            info._sifields._sigfault._addr = env->pc;

> -            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);

> -            break;

> -        case EXCP_FPE:

> -            info.si_signo = TARGET_SIGFPE;

> -            info.si_errno = 0;

> -            info.si_code = 0;

> -            info._sifields._sigfault._addr = env->pc;

> -            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);

> +            force_sig_fault(TARGET_SIGILL, TARGET_ILL_ILLOPC, env->pc);

>              break;

>          case EXCP_INTERRUPT:

>              /* We processed the pending cpu work above.  */

>              break;

>          case EXCP_DEBUG:

> -            info.si_signo = TARGET_SIGTRAP;

> -            info.si_errno = 0;

> -            info.si_code = TARGET_TRAP_BRKPT;

> -            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);

> +            force_sig_fault(TARGET_SIGTRAP, TARGET_TRAP_BRKPT, env->pc);

>              break;

>          case EXCP_ATOMIC:

>              cpu_exec_step_atomic(cs);

> --

> 2.25.1


thanks
-- PMM

On 8/24/21 10:17 AM, Peter Maydell wrote:
> I think that EXCP_RANGE should for us be unreachable in user-only

> mode (because it can only happen if the relevant bits in SR are

> set, and SR is writeable only in supervisor mode, and its starting

> value doesn't set these bits). So we could just delete the EXCP_RANGE

> handling and let it hit the default g_assert_not_reached() case.


If I also disable the SR case from gdbstub.

> EXCP_FPE is more tricky -- this happens for FP exceptions, where

> the enabling bit is in the FPCSR, which does appear to be writeable

> from user mode. So either:

>   * our mtspr is wrong and should either be not allowing writes

>     to FPCSR in usermode (or at least sanitizing them)

>   * the Linux kernel for openrisc is wrong, because a userspace

>     program that sets FPCSR.FPEE can make it run into unhandled_exception()

>     and die(), and it should be doing something else, like delivering

>     a suitable SIGFPE


I believe the kernel to be buggy.  But it also point to the fact that no one has written 
fenv.h for or1k for musl, so no one has tried to use those bits.


r~

On Sun, Sep 19, 2021 at 10:49:26AM -0700, Richard Henderson wrote:
> On 8/24/21 10:17 AM, Peter Maydell wrote:

> > I think that EXCP_RANGE should for us be unreachable in user-only

> > mode (because it can only happen if the relevant bits in SR are

> > set, and SR is writeable only in supervisor mode, and its starting

> > value doesn't set these bits). So we could just delete the EXCP_RANGE

> > handling and let it hit the default g_assert_not_reached() case.

> 

> If I also disable the SR case from gdbstub.

> 

> > EXCP_FPE is more tricky -- this happens for FP exceptions, where

> > the enabling bit is in the FPCSR, which does appear to be writeable

> > from user mode. So either:

> >   * our mtspr is wrong and should either be not allowing writes

> >     to FPCSR in usermode (or at least sanitizing them)

> >   * the Linux kernel for openrisc is wrong, because a userspace

> >     program that sets FPCSR.FPEE can make it run into unhandled_exception()

> >     and die(), and it should be doing something else, like delivering

> >     a suitable SIGFPE

> 

> I believe the kernel to be buggy.  But it also point to the fact that no one

> has written fenv.h for or1k for musl, so no one has tried to use those bits.


Hi,

*On User Accessible FPCSR*

As per the spec FPCSR should not be accessible in user space. But...

I am currently working on the OpenRISC port for glibc, and at first I was
planning for FPU support but this was one thing that slowed me down.

For that reason I proposed an architecture change to allow setting fpcsr in user
space, it seems that is allowed by almost all other architectures:

 https://openrisc.io/proposals/p17-user-mode-fpcsr

I think I could also simulate it in the kernel by catching the mtspr failure and
then performing it on behalf of the user if its for MTSPR.

At the moment I am going with softfpu until I can spend time on sorting out the
FPCSR issue.

*On QEMU*

When I started to develop the glibc FPU code, I put a patch into qemu to allow
for using mtspr and mfspr in user space:

 branch:
  https://github.com/stffrdhrn/qemu/commits/or1k-glibc

 commit:
   https://github.com/stffrdhrn/qemu/commit/dfa5331bf43f71535847c585a6b3f5779a422b13

User space access it not allowed as per trans_l_mfspr, trans_l_mfspr.  I did not
post this upstream as it's not as per spec.


I hope it helps a bit.

-Stafford