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

From: Peter Maydell <peter.maydell@linaro.org>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: Stafford Horne <shorne@gmail.com>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Laurent Vivier <laurent@vivier.eu>
Subject: Re: [PATCH v2 24/30] linux-user/openrisc: Use force_sig_fault, force_sigsegv_for_addr
Date: Tue, 24 Aug 2021 18:17:31 +0100	[thread overview]
Message-ID: <CAFEAcA-C_xwxZo2fGsk5Bc36DyFPwOKH_pJgLt8OkSyV=iYZhg@mail.gmail.com> (raw)
In-Reply-To: <20210822035537.283193-25-richard.henderson@linaro.org>

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