Re: [PATCH v6 21/66] target/alpha: Implement alpha_cpu_record_sigsegv

[Philippe Mathieu-Daudé]

On 10/30/21 19:15, Richard Henderson wrote:
> Record trap_arg{0,1,2} for the linux-user signal frame.
> 
> Fill in the stores to trap_arg{1,2} that were missing
> from the previous user-only alpha_cpu_tlb_fill function.
> Use maperr to simplify computation of trap_arg1.
> 
> Remove the code for EXCP_MMFAULT from cpu_loop, as
> that part is now handled by cpu_loop_exit_sigsegv.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/alpha/cpu.h          | 13 +++++++++----
>  linux-user/alpha/cpu_loop.c |  8 --------
>  target/alpha/cpu.c          |  6 ++++--
>  target/alpha/helper.c       | 39 ++++++++++++++++++++++++++++++++-----
>  4 files changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h
> index 772828cc26..d49cc36d07 100644
> --- a/target/alpha/cpu.h
> +++ b/target/alpha/cpu.h
> @@ -439,9 +439,6 @@ void alpha_translate_init(void);
>  #define CPU_RESOLVING_TYPE TYPE_ALPHA_CPU
>  
>  void alpha_cpu_list(void);
> -bool alpha_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> -                        MMUAccessType access_type, int mmu_idx,
> -                        bool probe, uintptr_t retaddr);
>  void QEMU_NORETURN dynamic_excp(CPUAlphaState *, uintptr_t, int, int);
>  void QEMU_NORETURN arith_excp(CPUAlphaState *, uintptr_t, int, uint64_t);
>  
> @@ -449,7 +446,15 @@ uint64_t cpu_alpha_load_fpcr (CPUAlphaState *env);
>  void cpu_alpha_store_fpcr (CPUAlphaState *env, uint64_t val);
>  uint64_t cpu_alpha_load_gr(CPUAlphaState *env, unsigned reg);
>  void cpu_alpha_store_gr(CPUAlphaState *env, unsigned reg, uint64_t val);
> -#ifndef CONFIG_USER_ONLY
> +
> +#ifdef CONFIG_USER_ONLY
> +void alpha_cpu_record_sigsegv(CPUState *cs, vaddr address,
> +                              MMUAccessType access_type,
> +                              bool maperr, uintptr_t retaddr);
> +#else
> +bool alpha_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> +                        MMUAccessType access_type, int mmu_idx,
> +                        bool probe, uintptr_t retaddr);
>  void alpha_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
>                                       vaddr addr, unsigned size,
>                                       MMUAccessType access_type,
> diff --git a/linux-user/alpha/cpu_loop.c b/linux-user/alpha/cpu_loop.c
> index 1b00a81385..4cc8e0a55c 100644
> --- a/linux-user/alpha/cpu_loop.c
> +++ b/linux-user/alpha/cpu_loop.c
> @@ -54,14 +54,6 @@ void cpu_loop(CPUAlphaState *env)
>              fprintf(stderr, "External interrupt. Exit\n");
>              exit(EXIT_FAILURE);
>              break;
> -        case EXCP_MMFAULT:
> -            info.si_signo = TARGET_SIGSEGV;
> -            info.si_errno = 0;
> -            info.si_code = (page_get_flags(env->trap_arg0) & PAGE_VALID
> -                            ? TARGET_SEGV_ACCERR : TARGET_SEGV_MAPERR);
> -            info._sifields._sigfault._addr = env->trap_arg0;
> -            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
> -            break;
>          case EXCP_UNALIGN:
>              info.si_signo = TARGET_SIGBUS;
>              info.si_errno = 0;
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 93e16a2ffb..69f32c3078 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -218,9 +218,11 @@ static const struct SysemuCPUOps alpha_sysemu_ops = {
>  
>  static const struct TCGCPUOps alpha_tcg_ops = {
>      .initialize = alpha_translate_init,
> -    .tlb_fill = alpha_cpu_tlb_fill,
>  
> -#ifndef CONFIG_USER_ONLY
> +#ifdef CONFIG_USER_ONLY
> +    .record_sigsegv = alpha_cpu_record_sigsegv,
> +#else
> +    .tlb_fill = alpha_cpu_tlb_fill,
>      .cpu_exec_interrupt = alpha_cpu_exec_interrupt,
>      .do_interrupt = alpha_cpu_do_interrupt,
>      .do_transaction_failed = alpha_cpu_do_transaction_failed,
> diff --git a/target/alpha/helper.c b/target/alpha/helper.c
> index 81550d9e2f..b7e7f73b15 100644
> --- a/target/alpha/helper.c
> +++ b/target/alpha/helper.c
> @@ -120,15 +120,44 @@ void cpu_alpha_store_gr(CPUAlphaState *env, unsigned 
> reg, uint64_t val)
>  }
>  
>  #if defined(CONFIG_USER_ONLY)
> -bool alpha_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
> -                        MMUAccessType access_type, int mmu_idx,
> -                        bool probe, uintptr_t retaddr)
> +void alpha_cpu_record_sigsegv(CPUState *cs, vaddr address,
> +                              MMUAccessType access_type,
> +                              bool maperr, uintptr_t retaddr)
>  {
>      AlphaCPU *cpu = ALPHA_CPU(cs);
> +    target_ulong mmcsr, cause;
>  
> -    cs->exception_index = EXCP_MMFAULT;
> +    /* Assuming !maperr, infer the missing protection. */
> +    switch (access_type) {
> +    case MMU_DATA_LOAD:
> +        mmcsr = MM_K_FOR;
> +        cause = 0;
> +        break;
> +    case MMU_DATA_STORE:
> +        mmcsr = MM_K_FOW;
> +        cause = 1;
> +        break;
> +    case MMU_INST_FETCH:
> +        mmcsr = MM_K_FOE;
> +        cause = -1;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +    if (maperr) {
> +        if (address < BIT_ULL(TARGET_VIRT_ADDR_SPACE_BITS - 1)) {
> +            /* Userspace address, therefore page not mapped. */
> +            mmcsr = MM_K_TNV;
> +        } else {
> +            /* Kernel or invalid address. */
> +            mmcsr = MM_K_ACV;
> +        }
> +    }
> +
> +    /* Record the arguments that PALcode would give to the kernel. */
>      cpu->env.trap_arg0 = address;
> -    cpu_loop_exit_restore(cs, retaddr);
> +    cpu->env.trap_arg1 = mmcsr;
> +    cpu->env.trap_arg2 = cause;
>  }
>  #else
>  /* Returns the OSF/1 entMM failure indication, or -1 on success.  */
> 

I don't have experience with kernel nor alpha arch. The changes
make sense to me, so to the best of my knowledge:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>