Re: [PATCH v5 23/67] target/arm: Implement arm_cpu_record_sigsegv

[Warner Losh <imp@bsdimp.com>]

[-- Attachment #1: Type: text/plain, Size: 5186 bytes --]

On Thu, Oct 14, 2021 at 10:11 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> Because of the complexity of setting ESR, continue to use
> arm_deliver_fault.  This means we cannot remove the code
> within cpu_loop that decodes EXCP_DATA_ABORT and
> EXCP_PREFETCH_ABORT.
>
> But using the new hook means that we don't have to do the
> page_get_flags check manually, and we'll be able to restrict
> the tlb_fill hook to sysemu later.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/internals.h  |  6 ++++++
>  target/arm/cpu.c        |  6 ++++--
>  target/arm/cpu_tcg.c    |  6 ++++--
>  target/arm/tlb_helper.c | 36 +++++++++++++++++++-----------------
>  4 files changed, 33 insertions(+), 21 deletions(-)
>

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



> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 3612107ab2..5a7aaf0f51 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -544,9 +544,15 @@ static inline bool arm_extabort_type(MemTxResult
> result)
>      return result != MEMTX_DECODE_ERROR;
>  }
>
> +#ifdef CONFIG_USER_ONLY
> +void arm_cpu_record_sigsegv(CPUState *cpu, vaddr addr,
> +                            MMUAccessType access_type,
> +                            bool maperr, uintptr_t ra);
> +#else
>  bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                        MMUAccessType access_type, int mmu_idx,
>                        bool probe, uintptr_t retaddr);
> +#endif
>
>  static inline int arm_to_core_mmu_idx(ARMMMUIdx mmu_idx)
>  {
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 641a8c2d3d..7a18a58ca0 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2031,10 +2031,12 @@ static const struct SysemuCPUOps arm_sysemu_ops = {
>  static const struct TCGCPUOps arm_tcg_ops = {
>      .initialize = arm_translate_init,
>      .synchronize_from_tb = arm_cpu_synchronize_from_tb,
> -    .tlb_fill = arm_cpu_tlb_fill,
>      .debug_excp_handler = arm_debug_excp_handler,
>
> -#if !defined(CONFIG_USER_ONLY)
> +#ifdef CONFIG_USER_ONLY
> +    .record_sigsegv = arm_cpu_record_sigsegv,
> +#else
> +    .tlb_fill = arm_cpu_tlb_fill,
>      .cpu_exec_interrupt = arm_cpu_exec_interrupt,
>      .do_interrupt = arm_cpu_do_interrupt,
>      .do_transaction_failed = arm_cpu_do_transaction_failed,
> diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
> index 0d5adccf1a..7b3bea2fbb 100644
> --- a/target/arm/cpu_tcg.c
> +++ b/target/arm/cpu_tcg.c
> @@ -898,10 +898,12 @@ static void pxa270c5_initfn(Object *obj)
>  static const struct TCGCPUOps arm_v7m_tcg_ops = {
>      .initialize = arm_translate_init,
>      .synchronize_from_tb = arm_cpu_synchronize_from_tb,
> -    .tlb_fill = arm_cpu_tlb_fill,
>      .debug_excp_handler = arm_debug_excp_handler,
>
> -#if !defined(CONFIG_USER_ONLY)
> +#ifdef CONFIG_USER_ONLY
> +    .record_sigsegv = arm_cpu_record_sigsegv,
> +#else
> +    .tlb_fill = arm_cpu_tlb_fill,
>      .cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt,
>      .do_interrupt = arm_v7m_cpu_do_interrupt,
>      .do_transaction_failed = arm_cpu_do_transaction_failed,
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index 3107f9823e..dc5860180f 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -147,28 +147,12 @@ void arm_cpu_do_transaction_failed(CPUState *cs,
> hwaddr physaddr,
>      arm_deliver_fault(cpu, addr, access_type, mmu_idx, &fi);
>  }
>
> -#endif /* !defined(CONFIG_USER_ONLY) */
> -
>  bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                        MMUAccessType access_type, int mmu_idx,
>                        bool probe, uintptr_t retaddr)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      ARMMMUFaultInfo fi = {};
> -
> -#ifdef CONFIG_USER_ONLY
> -    int flags = page_get_flags(useronly_clean_ptr(address));
> -    if (flags & PAGE_VALID) {
> -        fi.type = ARMFault_Permission;
> -    } else {
> -        fi.type = ARMFault_Translation;
> -    }
> -    fi.level = 3;
> -
> -    /* now we have a real cpu fault */
> -    cpu_restore_state(cs, retaddr, true);
> -    arm_deliver_fault(cpu, address, access_type, mmu_idx, &fi);
> -#else
>      hwaddr phys_addr;
>      target_ulong page_size;
>      int prot, ret;
> @@ -210,5 +194,23 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
>          cpu_restore_state(cs, retaddr, true);
>          arm_deliver_fault(cpu, address, access_type, mmu_idx, &fi);
>      }
> -#endif
>  }
> +#else
> +void arm_cpu_record_sigsegv(CPUState *cs, vaddr addr,
> +                            MMUAccessType access_type,
> +                            bool maperr, uintptr_t ra)
> +{
> +    ARMMMUFaultInfo fi = {
> +        .type = maperr ? ARMFault_Translation : ARMFault_Permission,
> +        .level = 3,
> +    };
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    /*
> +     * We report both ESR and FAR to signal handlers.
> +     * For now, it's easiest to deliver the fault normally.
> +     */
> +    cpu_restore_state(cs, ra, true);
> +    arm_deliver_fault(cpu, addr, access_type, MMU_USER_IDX, &fi);
> +}
> +#endif /* !defined(CONFIG_USER_ONLY) */
> --
> 2.25.1
>
>

[-- Attachment #2: Type: text/html, Size: 6538 bytes --]