Re: [RFC PATCH 7/7] linux-user: Reorg cpu_signal_handler

[Philippe Mathieu-Daudé]

Hi Richard,

On 9/14/21 12:05 AM, Richard Henderson wrote:
> Split out two functions into linux-user/host/arch/host-signal.h.
> Since linux-user requires a linux host, drop all of the BSD and
> Solaris ifdefs.  These should be recreated under bsd-user/ when
> the current blanks there are filled.
> 
> Fold the remnants of handle_cpu_signal into host_signal_handler.
> 
> Drop the call to cc->tcg_ops->tlb_fill.  This was simply an indirect
> method to raise SIGSEGV; it is far easier to pass the host siginfo_t
> along to the guest.  This fixes all of the guest cpu_loop code that
> currently fails to properly fill in SEGV_MAPERR vs SEGV_ACCERR.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/exec-all.h               |  12 -
>  linux-user/host/aarch64/host-signal.h |  73 +++
>  linux-user/host/alpha/host-signal.h   |  41 ++
>  linux-user/host/arm/host-signal.h     |  30 ++
>  linux-user/host/i386/host-signal.h    |  24 +
>  linux-user/host/mips/host-signal.h    |  61 +++
>  linux-user/host/ppc/host-signal.h     |  24 +
>  linux-user/host/ppc64/host-signal.h   |   1 +
>  linux-user/host/riscv32/host-signal.h |  57 +++
>  linux-user/host/riscv64/host-signal.h |   1 +
>  linux-user/host/s390/host-signal.h    |  92 ++++
>  linux-user/host/s390x/host-signal.h   |   1 +
>  linux-user/host/sparc/host-signal.h   |  53 ++
>  linux-user/host/sparc64/host-signal.h |   1 +
>  linux-user/host/x86_64/host-signal.h  |  24 +
>  accel/tcg/user-exec.c                 | 712 --------------------------
>  linux-user/signal.c                   |  93 ++--
>  17 files changed, 543 insertions(+), 757 deletions(-)
>  create mode 100644 linux-user/host/aarch64/host-signal.h
>  create mode 100644 linux-user/host/alpha/host-signal.h
>  create mode 100644 linux-user/host/arm/host-signal.h
>  create mode 100644 linux-user/host/i386/host-signal.h
>  create mode 100644 linux-user/host/mips/host-signal.h
>  create mode 100644 linux-user/host/ppc/host-signal.h
>  create mode 100644 linux-user/host/ppc64/host-signal.h
>  create mode 100644 linux-user/host/riscv32/host-signal.h
>  create mode 100644 linux-user/host/riscv64/host-signal.h
>  create mode 100644 linux-user/host/s390/host-signal.h
>  create mode 100644 linux-user/host/s390x/host-signal.h
>  create mode 100644 linux-user/host/sparc/host-signal.h
>  create mode 100644 linux-user/host/sparc64/host-signal.h
>  create mode 100644 linux-user/host/x86_64/host-signal.h

> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 73c0f9066b..509dad7850 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c

> -static void host_signal_handler(int host_signum, siginfo_t *info,
> -                                void *puc)
> +static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
>  {
>      CPUArchState *env = thread_cpu->env_ptr;
>      CPUState *cpu = env_cpu(env);
>      TaskState *ts = cpu->opaque;
> -
> -    int sig;
> +    bool sync_sig = false;
>      target_siginfo_t tinfo;
>      ucontext_t *uc = puc;
>      struct emulated_sigtable *k;
> +    uintptr_t pc = 0;
> +    int guest_sig;
>  
> -    /* the CPU emulator uses some host signals to detect exceptions,
> -       we forward to it some signals */
> -    if ((host_signum == SIGSEGV || host_signum == SIGBUS)
> -        && info->si_code > 0) {
> -        if (cpu_signal_handler(host_signum, info, puc))
> +    /*
> +     * Non-spoofed SIGSEGV and SIGBUS are synchronous, and need special
> +     * handling wrt signal blocking and unwinding.  SIGSEGV may need to
> +     * remove write-protection and restart the instruction.
> +     */
> +    if ((host_sig == SIGSEGV || host_sig == SIGBUS) && info->si_code > 0) {
> +        pc = adjust_signal_pc(host_signal_pc(uc));
> +        if (host_sig == SIGSEGV &&
> +            info->si_code == SEGV_ACCERR &&
> +            host_sigsegv_write(info, uc) &&
> +            handle_sigsegv_accerr_write(cpu, &uc->uc_sigmask, pc,
> +                                        (uintptr_t)info->si_addr)) {
>              return;
> +        }
> +        sync_sig = true;
> +    } else {
> +        rewind_if_in_safe_syscall(puc);
> +
> +        /*
> +         * Block host signals until target signal handler entered.
> +         * We can't block SIGSEGV or SIGBUS while we're executing
> +         * guest code in case the guest code provokes one in the
> +         * window between now and it getting out to the main loop.
> +         * Signals will be unblocked again in process_pending_signals().
> +         *
> +         * WARNING: we cannot use sigfillset() here because the uc_sigmask
> +         * field is a kernel sigset_t, which is much smaller than the
> +         * libc sigset_t which sigfillset() operates on. Using sigfillset()
> +         * would write 0xff bytes off the end of the structure and trash
> +         * data on the struct.
> +         * We can't use sizeof(uc->uc_sigmask) either, because the libc
> +         * headers define the struct field with the wrong (too large) type.
> +         */
> +        memset(&uc->uc_sigmask, 0xff, SIGSET_T_SIZE);
> +        sigdelset(&uc->uc_sigmask, SIGSEGV);
> +        sigdelset(&uc->uc_sigmask, SIGBUS);
>      }
>  
>      /* get target signal number */
> -    sig = host_to_target_signal(host_signum);
> -    if (sig < 1 || sig > TARGET_NSIG)
> +    guest_sig = host_to_target_signal(host_sig);
> +    if (guest_sig < 1 || guest_sig > TARGET_NSIG) {
>          return;
> -    trace_user_host_signal(env, host_signum, sig);
> -
> -    rewind_if_in_safe_syscall(puc);
> +    }
> +    trace_user_host_signal(env, host_sig, guest_sig);
>  
>      host_to_target_siginfo_noswap(&tinfo, info);
> -    k = &ts->sigtab[sig - 1];
> +    k = &ts->sigtab[guest_sig - 1];
>      k->info = tinfo;
> -    k->pending = sig;
> +    k->pending = guest_sig;
>      ts->signal_pending = 1;
>  
> -    /* Block host signals until target signal handler entered. We
> -     * can't block SIGSEGV or SIGBUS while we're executing guest
> -     * code in case the guest code provokes one in the window between
> -     * now and it getting out to the main loop. Signals will be
> -     * unblocked again in process_pending_signals().
> -     *
> -     * WARNING: we cannot use sigfillset() here because the uc_sigmask
> -     * field is a kernel sigset_t, which is much smaller than the
> -     * libc sigset_t which sigfillset() operates on. Using sigfillset()
> -     * would write 0xff bytes off the end of the structure and trash
> -     * data on the struct.
> -     * We can't use sizeof(uc->uc_sigmask) either, because the libc
> -     * headers define the struct field with the wrong (too large) type.
> +    /*
> +     * For synchronous signals, unwind the cpu state to the faulting
> +     * insn and then exit back to the main loop so that the signal
> +     * is delivered immediately.
>       */
> -    memset(&uc->uc_sigmask, 0xff, SIGSET_T_SIZE);
> -    sigdelset(&uc->uc_sigmask, SIGSEGV);
> -    sigdelset(&uc->uc_sigmask, SIGBUS);
> +    if (sync_sig) {
> +        clear_helper_retaddr();
> +        sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
> +        cpu->exception_index = EXCP_INTERRUPT;
> +        cpu_loop_exit_restore(cpu, pc);
> +    }
>  
> -    /* interrupt the virtual CPU as soon as possible */
> +    /*
> +     * Interrupt the virtual CPU as soon as possible, but for now
> +     * return to continue with the current TB.
> +     */
>      cpu_exit(thread_cpu);
>  }

Is it possible to split this patch per architectures,
doing for the first arch:

  #if ARCH1

  new host_signal_handler() {}

  #else

  old unmodified host_signal_handler() {}

  #endif

Then for the second:

  #if ARCH1 || ARCH2

  new host_signal_handler() {}

  #else

  old unmodified host_signal_handler() {}

  #endif

Last patch being cleaning the transition:

- #if ARCH1 || ARCH2 || ...

  new host_signal_handler() {}

- #else
-
- old unmodified host_signal_handler() {}
-
- #endif

?