Re: [PATCH v2 27/41] target/i386: Implement x86_cpu_record_sigsegv

[Warner Losh]

> On Sep 18, 2021, at 12:45 PM, Richard Henderson 
> <richard.henderson@linaro.org> wrote:
> 
> Record cr2, error_code, and exception_index.  That last means
> that we must exit to cpu_loop ourselves, instead of letting
> exception_index being overwritten.
> 
> Use the maperr parameter to properly set PG_ERROR_P_MASK.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/i386/tcg/helper-tcg.h       |  6 ++++++
> target/i386/tcg/tcg-cpu.c          |  3 ++-
> target/i386/tcg/user/excp_helper.c | 23 +++++++++++++++++------
> 3 files changed, 25 insertions(+), 7 deletions(-)


Reviewed by: Warner Losh <imp@bsdimp.com>

I think this encoding is fine, but haven’t thought though how I’d implement
this in bsd-user yet…  I have the signal code queued up, but not ready to send
off yet.

> diff --git a/target/i386/tcg/helper-tcg.h b/target/i386/tcg/helper-tcg.h
> index 60ca09e95e..0a4401e917 100644
> --- a/target/i386/tcg/helper-tcg.h
> +++ b/target/i386/tcg/helper-tcg.h
> @@ -43,9 +43,15 @@ bool x86_cpu_exec_interrupt(CPUState *cpu, int int_req);
> #endif
> 
> /* helper.c */
> +#ifdef CONFIG_USER_ONLY
> +void x86_cpu_record_sigsegv(CPUState *cs, vaddr addr,
> +                            MMUAccessType access_type,
> +                            bool maperr, uintptr_t ra);
> +#else
> bool x86_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                       MMUAccessType access_type, int mmu_idx,
>                       bool probe, uintptr_t retaddr);
> +#endif
> 
> void breakpoint_handler(CPUState *cs);
> 
> diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
> index aef050d089..3fab3676b1 100644
> --- a/target/i386/tcg/tcg-cpu.c
> +++ b/target/i386/tcg/tcg-cpu.c
> @@ -77,11 +77,12 @@ static const struct TCGCPUOps x86_tcg_ops = {
>     .synchronize_from_tb = x86_cpu_synchronize_from_tb,
>     .cpu_exec_enter = x86_cpu_exec_enter,
>     .cpu_exec_exit = x86_cpu_exec_exit,
> -    .tlb_fill = x86_cpu_tlb_fill,
> #ifdef CONFIG_USER_ONLY
>     .fake_user_interrupt = x86_cpu_do_interrupt,
> +    .record_sigsegv = x86_cpu_record_sigsegv,
> #else
>     .has_work = x86_cpu_has_work,
> +    .tlb_fill = x86_cpu_tlb_fill,
>     .do_interrupt = x86_cpu_do_interrupt,
>     .cpu_exec_interrupt = x86_cpu_exec_interrupt,
>     .debug_excp_handler = breakpoint_handler,
> diff --git a/target/i386/tcg/user/excp_helper.c 
> b/target/i386/tcg/user/excp_helper.c
> index a89b5228fd..cd507e2a1b 100644
> --- a/target/i386/tcg/user/excp_helper.c
> +++ b/target/i386/tcg/user/excp_helper.c
> @@ -22,18 +22,29 @@
> #include "exec/exec-all.h"
> #include "tcg/helper-tcg.h"
> 
> -bool x86_cpu_tlb_fill(CPUState *cs, vaddr addr, int size,
> -                      MMUAccessType access_type, int mmu_idx,
> -                      bool probe, uintptr_t retaddr)
> +void x86_cpu_record_sigsegv(CPUState *cs, vaddr addr,
> +                            MMUAccessType access_type,
> +                            bool maperr, uintptr_t ra)
> {
>     X86CPU *cpu = X86_CPU(cs);
>     CPUX86State *env = &cpu->env;
> 
> +    /*
> +     * The error_code that hw reports as part of the exception frame
> +     * is copied to linux sigcontext.err.  The exception_index is
> +     * copied to linux sigcontext.trapno.  Short of inventing a new
> +     * place to store the trapno, we cannot let our caller raise the
> +     * signal and set exception_index to EXCP_INTERRUPT.
> +     */
>     env->cr[2] = addr;
> -    env->error_code = (access_type == MMU_DATA_STORE) << PG_ERROR_W_BIT;
> -    env->error_code |= PG_ERROR_U_MASK;
> +    env->error_code = ((access_type == MMU_DATA_STORE) << PG_ERROR_W_BIT)
> +                    | (maperr ? 0 : PG_ERROR_P_MASK)
> +                    | PG_ERROR_U_MASK;
>     cs->exception_index = EXCP0E_PAGE;
> +
> +    /* Disable do_interrupt_user. */
>     env->exception_is_int = 0;
>     env->exception_next_eip = -1;
> -    cpu_loop_exit_restore(cs, retaddr);
> +
> +    cpu_loop_exit_restore(cs, ra);
> }
> -- 
> 2.25.1
> 
>