Re: [PATCH v4 15/41] linux-user/host/riscv: Populate host_signal.h

[Alistair Francis]

On Thu, Oct 7, 2021 at 3:31 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Split host_signal_pc and host_signal_write out of user-exec.c.
>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  linux-user/host/riscv/host-signal.h |  85 +++++++++++++++++-
>  accel/tcg/user-exec.c               | 134 ----------------------------
>  2 files changed, 84 insertions(+), 135 deletions(-)
>
> diff --git a/linux-user/host/riscv/host-signal.h 
> b/linux-user/host/riscv/host-signal.h
> index f4b4d65031..5860dce7d7 100644
> --- a/linux-user/host/riscv/host-signal.h
> +++ b/linux-user/host/riscv/host-signal.h
> @@ -1 +1,84 @@
> -#define HOST_SIGNAL_PLACEHOLDER
> +/*
> + * host-signal.h: signal info dependent on the host architecture
> + *
> + * Copyright (C) 2021 Linaro Limited
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef RISCV_HOST_SIGNAL_H
> +#define RISCV_HOST_SIGNAL_H
> +
> +static inline uintptr_t host_signal_pc(ucontext_t *uc)
> +{
> +    return uc->uc_mcontext.__gregs[REG_PC];
> +}
> +
> +static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
> +{
> +    uint32_t insn = *(uint32_t *)host_signal_pc(uc);
> +
> +    /*
> +     * Detect store by reading the instruction at the program
> +     * counter. Note: we currently only generate 32-bit
> +     * instructions so we thus only detect 32-bit stores
> +     */
> +    switch (((insn >> 0) & 0b11)) {
> +    case 3:
> +        switch (((insn >> 2) & 0b11111)) {
> +        case 8:
> +            switch (((insn >> 12) & 0b111)) {
> +            case 0: /* sb */
> +            case 1: /* sh */
> +            case 2: /* sw */
> +            case 3: /* sd */
> +            case 4: /* sq */
> +                return true;
> +            default:
> +                break;
> +            }
> +            break;
> +        case 9:
> +            switch (((insn >> 12) & 0b111)) {
> +            case 2: /* fsw */
> +            case 3: /* fsd */
> +            case 4: /* fsq */
> +                return true;
> +            default:
> +                break;
> +            }
> +            break;
> +        default:
> +            break;
> +        }
> +    }
> +
> +    /* Check for compressed instructions */
> +    switch (((insn >> 13) & 0b111)) {
> +    case 7:
> +        switch (insn & 0b11) {
> +        case 0: /*c.sd */
> +        case 2: /* c.sdsp */
> +            return true;
> +        default:
> +            break;
> +        }
> +        break;
> +    case 6:
> +        switch (insn & 0b11) {
> +        case 0: /* c.sw */
> +        case 3: /* c.swsp */
> +            return true;
> +        default:
> +            break;
> +        }
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    return false;
> +}
> +
> +#endif
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index fe3a3ce6e2..de8e106b68 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -139,64 +139,6 @@ bool handle_sigsegv_accerr_write(CPUState *cpu, sigset_t 
> *old_set,
>      }
>  }
>
> -/*
> - * 'pc' is the host PC at which the exception was raised.
> - * 'address' is the effective address of the memory exception.
> - * 'is_write' is 1 if a write caused the exception and otherwise 0.
> - * 'old_set' is the signal set which should be restored.
> - */
> -static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
> -                                    int is_write, sigset_t *old_set)
> -{
> -    CPUState *cpu = current_cpu;
> -    CPUClass *cc;
> -    unsigned long host_addr = (unsigned long)info->si_addr;
> -    MMUAccessType access_type = adjust_signal_pc(&pc, is_write);
> -    abi_ptr guest_addr;
> -
> -    /* For synchronous signals we expect to be coming from the vCPU
> -     * thread (so current_cpu should be valid) and either from running
> -     * code or during translation which can fault as we cross pages.
> -     *
> -     * If neither is true then something has gone wrong and we should
> -     * abort rather than try and restart the vCPU execution.
> -     */
> -    if (!cpu || !cpu->running) {
> -        printf("qemu:%s received signal outside vCPU context @ pc=0x%"
> -               PRIxPTR "\n",  __func__, pc);
> -        abort();
> -    }
> -
> -#if defined(DEBUG_SIGNAL)
> -    printf("qemu: SIGSEGV pc=0x%08lx address=%08lx w=%d oldset=0x%08lx\n",
> -           pc, host_addr, is_write, *(unsigned long *)old_set);
> -#endif
> -
> -    /* Convert forcefully to guest address space, invalid addresses
> -       are still valid segv ones */
> -    guest_addr = h2g_nocheck(host_addr);
> -
> -    /* XXX: locking issue */
> -    if (is_write &&
> -        info->si_signo == SIGSEGV &&
> -        info->si_code == SEGV_ACCERR &&
> -        h2g_valid(host_addr) &&
> -        handle_sigsegv_accerr_write(cpu, old_set, pc, guest_addr)) {
> -        return 1;
> -    }
> -
> -    /*
> -     * There is no way the target can handle this other than raising
> -     * an exception.  Undo signal and retaddr state prior to longjmp.
> -     */
> -    sigprocmask(SIG_SETMASK, old_set, NULL);
> -
> -    cc = CPU_GET_CLASS(cpu);
> -    cc->tcg_ops->tlb_fill(cpu, guest_addr, 0, access_type,
> -                          MMU_USER_IDX, false, pc);
> -    g_assert_not_reached();
> -}
> -
>  static int probe_access_internal(CPUArchState *env, target_ulong addr,
>                                   int fault_size, MMUAccessType access_type,
>                                   bool nonfault, uintptr_t ra)
> @@ -255,82 +197,6 @@ void *probe_access(CPUArchState *env, target_ulong addr, 
> int size,
>      return size ? g2h(env_cpu(env), addr) : NULL;
>  }
>
> -#if defined(__riscv)
> -
> -int cpu_signal_handler(int host_signum, void *pinfo,
> -                       void *puc)
> -{
> -    siginfo_t *info = pinfo;
> -    ucontext_t *uc = puc;
> -    greg_t pc = uc->uc_mcontext.__gregs[REG_PC];
> -    uint32_t insn = *(uint32_t *)pc;
> -    int is_write = 0;
> -
> -    /* Detect store by reading the instruction at the program
> -       counter. Note: we currently only generate 32-bit
> -       instructions so we thus only detect 32-bit stores */
> -    switch (((insn >> 0) & 0b11)) {
> -    case 3:
> -        switch (((insn >> 2) & 0b11111)) {
> -        case 8:
> -            switch (((insn >> 12) & 0b111)) {
> -            case 0: /* sb */
> -            case 1: /* sh */
> -            case 2: /* sw */
> -            case 3: /* sd */
> -            case 4: /* sq */
> -                is_write = 1;
> -                break;
> -            default:
> -                break;
> -            }
> -            break;
> -        case 9:
> -            switch (((insn >> 12) & 0b111)) {
> -            case 2: /* fsw */
> -            case 3: /* fsd */
> -            case 4: /* fsq */
> -                is_write = 1;
> -                break;
> -            default:
> -                break;
> -            }
> -            break;
> -        default:
> -            break;
> -        }
> -    }
> -
> -    /* Check for compressed instructions */
> -    switch (((insn >> 13) & 0b111)) {
> -    case 7:
> -        switch (insn & 0b11) {
> -        case 0: /*c.sd */
> -        case 2: /* c.sdsp */
> -            is_write = 1;
> -            break;
> -        default:
> -            break;
> -        }
> -        break;
> -    case 6:
> -        switch (insn & 0b11) {
> -        case 0: /* c.sw */
> -        case 3: /* c.swsp */
> -            is_write = 1;
> -            break;
> -        default:
> -            break;
> -        }
> -        break;
> -    default:
> -        break;
> -    }
> -
> -    return handle_cpu_signal(pc, info, is_write, &uc->uc_sigmask);
> -}
> -#endif
> -
>  /* The softmmu versions of these helpers are in cputlb.c.  */
>
>  uint32_t cpu_ldub_data(CPUArchState *env, abi_ptr ptr)
> --
> 2.25.1
>
>