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.

Reviewed-by: Warner Losh <imp@bsdimp.com>
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(-)

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

On Sat, 30 Oct 2021 at 19:11, 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.

We seem to be restricting tlb_fill to sysemu already.
Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

On 11/1/21 12:30 PM, Peter Maydell wrote:
> We seem to be restricting tlb_fill to sysemu already.

Not true:

> -#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


r~

On Mon, 1 Nov 2021 at 17:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/1/21 12:30 PM, Peter Maydell wrote:
> > We seem to be restricting tlb_fill to sysemu already.
>
> Not true:
>
> > -#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

Yes, that's restricting tlb_fill to sysemu only now (in this patch),
not "later", right ?

-- PMM

On 11/1/21 1:25 PM, Peter Maydell wrote:
> On Mon, 1 Nov 2021 at 17:09, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 11/1/21 12:30 PM, Peter Maydell wrote:
>>> We seem to be restricting tlb_fill to sysemu already.
>>
>> Not true:
>>
>>> -#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
> 
> Yes, that's restricting tlb_fill to sysemu only now (in this patch),
> not "later", right ?

Oh, I see what you mean, re

    "... and we'll be able to restrict
     the tlb_fill hook to sysemu later."

This was in reference to patch 41, "accel/tcg: Restrict TCGCPUOps::tlb_fill() to sysemu", 
where I remove the tlb_fill hook from TCGCPUOps for CONFIG_USER_ONLY.


r~