diff mbox series

[v3,27/42] target/arm: Use softmmu tlbs for page table walking

Message ID 20221001162318.153420-28-richard.henderson@linaro.org
State New
Headers show
Series target/arm: Implement FEAT_HAFDBS | expand

Commit Message

Richard Henderson Oct. 1, 2022, 4:23 p.m. UTC 
So far, limit the change to S1_ptw_translate, arm_ldl_ptw, and
arm_ldq_ptw.  Use probe_access_full to find the host address,
and if so use a host load.  If the probe fails, we've got our
fault info already.  On the off chance that page tables are not
in RAM, continue to use the address_space_ld* functions.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h        |   5 +
 target/arm/ptw.c        | 207 ++++++++++++++++++++++++++--------------
 target/arm/tlb_helper.c |  17 +++-
 3 files changed, 155 insertions(+), 74 deletions(-)

Comments

Peter Maydell Oct. 7, 2022, 9:01 a.m. UTC | #1 
On Sat, 1 Oct 2022 at 17:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> So far, limit the change to S1_ptw_translate, arm_ldl_ptw, and
> arm_ldq_ptw.  Use probe_access_full to find the host address,
> and if so use a host load.  If the probe fails, we've got our
> fault info already.  On the off chance that page tables are not
> in RAM, continue to use the address_space_ld* functions.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---


> -static bool ptw_attrs_are_device(uint64_t hcr, ARMCacheAttrs cacheattrs)
> +static bool S2_attrs_are_device(uint64_t hcr, uint8_t attrs)
>  {
>      /*
>       * For an S1 page table walk, the stage 1 attributes are always
> @@ -202,41 +203,72 @@ static bool ptw_attrs_are_device(uint64_t hcr, ARMCacheAttrs cacheattrs)
>       * With HCR_EL2.FWB == 1 this is when descriptor bit [4] is 0, ie
>       * when cacheattrs.attrs bit [2] is 0.
>       */
> -    assert(cacheattrs.is_s2_format);
>      if (hcr & HCR_FWB) {
> -        return (cacheattrs.attrs & 0x4) == 0;
> +        return (attrs & 0x4) == 0;
>      } else {
> -        return (cacheattrs.attrs & 0xc) == 0;
> +        return (attrs & 0xc) == 0;
>      }

The upcoming v8R support has its stage 2 attributes in the MAIR
format, so it might be a little awkward to assume the v8A-stage-2
format here rather than being able to add the "if !is_s2_format"
condition. I guess we'll deal with that when we get to it...

> +        env->tlb_fi = fi;
> +        flags = probe_access_full(env, addr, MMU_DATA_LOAD,
> +                                  arm_to_core_mmu_idx(s2_mmu_idx),
> +                                  true, hphys, &full, 0);
> +        env->tlb_fi = NULL;
> +
> +        if (unlikely(flags & TLB_INVALID_MASK)) {
> +            goto fail;
> +        }
> +        *gphys = full->phys_addr;
> +        pte_attrs = full->pte_attrs;
> +        pte_secure = full->attrs.secure;
> +    }
> +

> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -208,10 +208,21 @@ bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                        bool probe, uintptr_t retaddr)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> -    ARMMMUFaultInfo fi = {};
>      GetPhysAddrResult res = {};
> +    ARMMMUFaultInfo local_fi, *fi;
>      int ret;
>
> +    /*
> +     * Allow S1_ptw_translate to see any fault generated here.
> +     * Since this may recurse, read and clear.
> +     */
> +    fi = cpu->env.tlb_fi;
> +    if (fi) {
> +        cpu->env.tlb_fi = NULL;
> +    } else {
> +        fi = memset(&local_fi, 0, sizeof(local_fi));
> +    }

This makes two architectures now that want to do "call a probe_access
function, and get information that's known in the architecture-specific
tlb_fill function", and need to do it via this awkward "have tlb_fill
know that it should stash the info away in the CPU state struct somewhere"
trick (the other being s390 tlb_fill_exc/tlb_fill_tec). But I don't
really have a better idea, so

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
Richard Henderson Oct. 7, 2022, 3:27 p.m. UTC | #2 
On 10/7/22 02:01, Peter Maydell wrote:
> The upcoming v8R support has its stage 2 attributes in the MAIR
> format, so it might be a little awkward to assume the v8A-stage-2
> format here rather than being able to add the "if !is_s2_format"
> condition. I guess we'll deal with that when we get to it...

Ah.  I had wondered whether it would be better to convert the result here, so that we 
always have the MAIR format.  I decided against it within the scope of this patch set 
because it meant that I kept the existing s1+s2 attribute merging logic unchanged.

>> +    /*
>> +     * Allow S1_ptw_translate to see any fault generated here.
>> +     * Since this may recurse, read and clear.
>> +     */
>> +    fi = cpu->env.tlb_fi;
>> +    if (fi) {
>> +        cpu->env.tlb_fi = NULL;
>> +    } else {
>> +        fi = memset(&local_fi, 0, sizeof(local_fi));
>> +    }
> 
> This makes two architectures now that want to do "call a probe_access
> function, and get information that's known in the architecture-specific
> tlb_fill function", and need to do it via this awkward "have tlb_fill
> know that it should stash the info away in the CPU state struct somewhere"
> trick (the other being s390 tlb_fill_exc/tlb_fill_tec). But I don't
> really have a better idea...

A better idea would be most welcome, if anyone has one...  :-)


r~
Peter Maydell Oct. 7, 2022, 4:08 p.m. UTC | #3 
On Fri, 7 Oct 2022 at 16:27, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/7/22 02:01, Peter Maydell wrote:
> > The upcoming v8R support has its stage 2 attributes in the MAIR
> > format, so it might be a little awkward to assume the v8A-stage-2
> > format here rather than being able to add the "if !is_s2_format"
> > condition. I guess we'll deal with that when we get to it...
>
> Ah.  I had wondered whether it would be better to convert the result here, so that we
> always have the MAIR format.  I decided against it within the scope of this patch set
> because it meant that I kept the existing s1+s2 attribute merging logic unchanged.

Unfortunately, for A-profile you can't just convert the s2 attrs
to MAIR format, because their interpretation depends on the
s1 attr values in the FWB case. So you have to keep the s2
attrs as raw until they get to the point of combination.
(v8R doesn't have any equivalent of FWB.)

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 732c0c00ac..7108568685 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -225,6 +225,8 @@  typedef struct CPUARMTBFlags {
     target_ulong flags2;
 } CPUARMTBFlags;
 
+typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
+
 typedef struct CPUArchState {
     /* Regs for current mode.  */
     uint32_t regs[16];
@@ -715,6 +717,9 @@  typedef struct CPUArchState {
     struct CPUBreakpoint *cpu_breakpoint[16];
     struct CPUWatchpoint *cpu_watchpoint[16];
 
+    /* Optional fault info across tlb lookup. */
+    ARMMMUFaultInfo *tlb_fi;
+
     /* Fields up to this point are cleared by a CPU reset */
     struct {} end_reset_fields;
 
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 45adb9d5a9..ba496c3421 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -9,6 +9,7 @@ 
 #include "qemu/osdep.h"
 #include "qemu/log.h"
 #include "qemu/range.h"
+#include "exec/exec-all.h"
 #include "cpu.h"
 #include "internals.h"
 #include "idau.h"
@@ -191,7 +192,7 @@  static bool regime_translation_disabled(CPUARMState *env, ARMMMUIdx mmu_idx,
     return (regime_sctlr(env, mmu_idx) & SCTLR_M) == 0;
 }
 
-static bool ptw_attrs_are_device(uint64_t hcr, ARMCacheAttrs cacheattrs)
+static bool S2_attrs_are_device(uint64_t hcr, uint8_t attrs)
 {
     /*
      * For an S1 page table walk, the stage 1 attributes are always
@@ -202,41 +203,72 @@  static bool ptw_attrs_are_device(uint64_t hcr, ARMCacheAttrs cacheattrs)
      * With HCR_EL2.FWB == 1 this is when descriptor bit [4] is 0, ie
      * when cacheattrs.attrs bit [2] is 0.
      */
-    assert(cacheattrs.is_s2_format);
     if (hcr & HCR_FWB) {
-        return (cacheattrs.attrs & 0x4) == 0;
+        return (attrs & 0x4) == 0;
     } else {
-        return (cacheattrs.attrs & 0xc) == 0;
+        return (attrs & 0xc) == 0;
     }
 }
 
 /* Translate a S1 pagetable walk through S2 if needed.  */
-static hwaddr S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
-                               hwaddr addr, bool *is_secure_ptr, bool debug,
-                               ARMMMUFaultInfo *fi)
+static bool S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx, hwaddr addr,
+                             bool *is_secure_ptr, void **hphys, hwaddr *gphys,
+                             bool debug, ARMMMUFaultInfo *fi)
 {
     bool is_secure = *is_secure_ptr;
     ARMMMUIdx s2_mmu_idx = is_secure ? ARMMMUIdx_Stage2_S : ARMMMUIdx_Stage2;
+    bool s2_phys = false;
+    uint8_t pte_attrs;
+    bool pte_secure;
 
-    if (arm_mmu_idx_is_stage1_of_2(mmu_idx) &&
-        !regime_translation_disabled(env, s2_mmu_idx, is_secure)) {
-        GetPhysAddrResult s2 = {};
-        uint64_t hcr;
-        int ret;
+    if (!arm_mmu_idx_is_stage1_of_2(mmu_idx)
+        || regime_translation_disabled(env, s2_mmu_idx, is_secure)) {
+        s2_mmu_idx = is_secure ? ARMMMUIdx_Phys_S : ARMMMUIdx_Phys_NS;
+        s2_phys = true;
+    }
 
-        ret = get_phys_addr_lpae(env, addr, MMU_DATA_LOAD, s2_mmu_idx,
-                                 is_secure, false, debug, &s2, fi);
-        if (ret) {
-            assert(fi->type != ARMFault_None);
-            fi->s2addr = addr;
-            fi->stage2 = true;
-            fi->s1ptw = true;
-            fi->s1ns = !is_secure;
-            return ~0;
+    if (unlikely(debug)) {
+        /*
+         * From gdbstub, do not use softmmu so that we don't modify the
+         * state of the cpu at all, including softmmu tlb contents.
+         */
+        if (s2_phys) {
+            *gphys = addr;
+            pte_attrs = 0;
+            pte_secure = is_secure;
+        } else {
+            GetPhysAddrResult s2 = { };
+            if (!get_phys_addr_lpae(env, addr, MMU_DATA_LOAD, s2_mmu_idx,
+                                    is_secure, false, debug, &s2, fi)) {
+                goto fail;
+            }
+            *gphys = s2.f.phys_addr;
+            pte_attrs = s2.cacheattrs.attrs;
+            pte_secure = s2.f.attrs.secure;
         }
+        *hphys = NULL;
+    } else {
+        CPUTLBEntryFull *full;
+        int flags;
 
-        hcr = arm_hcr_el2_eff_secstate(env, is_secure);
-        if ((hcr & HCR_PTW) && ptw_attrs_are_device(hcr, s2.cacheattrs)) {
+        env->tlb_fi = fi;
+        flags = probe_access_full(env, addr, MMU_DATA_LOAD,
+                                  arm_to_core_mmu_idx(s2_mmu_idx),
+                                  true, hphys, &full, 0);
+        env->tlb_fi = NULL;
+
+        if (unlikely(flags & TLB_INVALID_MASK)) {
+            goto fail;
+        }
+        *gphys = full->phys_addr;
+        pte_attrs = full->pte_attrs;
+        pte_secure = full->attrs.secure;
+    }
+
+    if (!s2_phys) {
+        uint64_t hcr = arm_hcr_el2_eff_secstate(env, is_secure);
+
+        if ((hcr & HCR_PTW) && S2_attrs_are_device(hcr, pte_attrs)) {
             /*
              * PTW set and S1 walk touched S2 Device memory:
              * generate Permission fault.
@@ -246,24 +278,25 @@  static hwaddr S1_ptw_translate(CPUARMState *env, ARMMMUIdx mmu_idx,
             fi->stage2 = true;
             fi->s1ptw = true;
             fi->s1ns = !is_secure;
-            return ~0;
+            return false;
         }
-
-        if (arm_is_secure_below_el3(env)) {
-            /* Check if page table walk is to secure or non-secure PA space. */
-            if (is_secure) {
-                is_secure = !(env->cp15.vstcr_el2 & VSTCR_SW);
-            } else {
-                is_secure = !(env->cp15.vtcr_el2 & VTCR_NSW);
-            }
-            *is_secure_ptr = is_secure;
-        } else {
-            assert(!is_secure);
-        }
-
-        addr = s2.f.phys_addr;
     }
-    return addr;
+
+    if (is_secure) {
+        /* Check if page table walk is to secure or non-secure PA space. */
+        *is_secure_ptr = !(pte_secure
+                           ? env->cp15.vstcr_el2 & VSTCR_SW
+                           : env->cp15.vtcr_el2 & VTCR_NSW);
+    }
+    return true;
+
+ fail:
+    assert(fi->type != ARMFault_None);
+    fi->s2addr = addr;
+    fi->stage2 = true;
+    fi->s1ptw = true;
+    fi->s1ns = !is_secure;
+    return false;
 }
 
 /* All loads done in the course of a page table walk go through here. */
@@ -271,56 +304,88 @@  static uint32_t arm_ldl_ptw(CPUARMState *env, hwaddr addr, bool is_secure,
                             ARMMMUIdx mmu_idx, bool debug, ARMMMUFaultInfo *fi)
 {
     CPUState *cs = env_cpu(env);
-    MemTxAttrs attrs = {};
-    MemTxResult result = MEMTX_OK;
-    AddressSpace *as;
+    void *hphys;
+    hwaddr gphys;
     uint32_t data;
+    bool be;
 
-    addr = S1_ptw_translate(env, mmu_idx, addr, &is_secure, debug, fi);
-    attrs.secure = is_secure;
-    as = arm_addressspace(cs, attrs);
-    if (fi->s1ptw) {
+    if (!S1_ptw_translate(env, mmu_idx, addr, &is_secure,
+                          &hphys, &gphys, debug, fi)) {
+        /* Failure. */
+        assert(fi->s1ptw);
         return 0;
     }
-    if (regime_translation_big_endian(env, mmu_idx)) {
-        data = address_space_ldl_be(as, addr, attrs, &result);
+
+    be = regime_translation_big_endian(env, mmu_idx);
+    if (likely(hphys)) {
+        /* Page tables are in RAM, and we have the host address. */
+        if (be) {
+            data = ldl_be_p(hphys);
+        } else {
+            data = ldl_le_p(hphys);
+        }
     } else {
-        data = address_space_ldl_le(as, addr, attrs, &result);
+        /* Page tables are in MMIO. */
+        MemTxAttrs attrs = { .secure = is_secure };
+        AddressSpace *as = arm_addressspace(cs, attrs);
+        MemTxResult result = MEMTX_OK;
+
+        if (be) {
+            data = address_space_ldl_be(as, gphys, attrs, &result);
+        } else {
+            data = address_space_ldl_le(as, gphys, attrs, &result);
+        }
+        if (unlikely(result != MEMTX_OK)) {
+            fi->type = ARMFault_SyncExternalOnWalk;
+            fi->ea = arm_extabort_type(result);
+            return 0;
+        }
     }
-    if (result == MEMTX_OK) {
-        return data;
-    }
-    fi->type = ARMFault_SyncExternalOnWalk;
-    fi->ea = arm_extabort_type(result);
-    return 0;
+    return data;
 }
 
 static uint64_t arm_ldq_ptw(CPUARMState *env, hwaddr addr, bool is_secure,
                             ARMMMUIdx mmu_idx, bool debug, ARMMMUFaultInfo *fi)
 {
     CPUState *cs = env_cpu(env);
-    MemTxAttrs attrs = {};
-    MemTxResult result = MEMTX_OK;
-    AddressSpace *as;
+    void *hphys;
+    hwaddr gphys;
     uint64_t data;
+    bool be;
 
-    addr = S1_ptw_translate(env, mmu_idx, addr, &is_secure, debug, fi);
-    attrs.secure = is_secure;
-    as = arm_addressspace(cs, attrs);
-    if (fi->s1ptw) {
+    if (!S1_ptw_translate(env, mmu_idx, addr, &is_secure,
+                          &hphys, &gphys, debug, fi)) {
+        /* Failure. */
+        assert(fi->s1ptw);
         return 0;
     }
-    if (regime_translation_big_endian(env, mmu_idx)) {
-        data = address_space_ldq_be(as, addr, attrs, &result);
+
+    be = regime_translation_big_endian(env, mmu_idx);
+    if (likely(hphys)) {
+        /* Page tables are in RAM, and we have the host address. */
+        if (be) {
+            data = ldq_be_p(hphys);
+        } else {
+            data = ldq_le_p(hphys);
+        }
     } else {
-        data = address_space_ldq_le(as, addr, attrs, &result);
+        /* Page tables are in MMIO. */
+        MemTxAttrs attrs = { .secure = is_secure };
+        AddressSpace *as = arm_addressspace(cs, attrs);
+        MemTxResult result = MEMTX_OK;
+
+        if (be) {
+            data = address_space_ldq_be(as, gphys, attrs, &result);
+        } else {
+            data = address_space_ldq_le(as, gphys, attrs, &result);
+        }
+        if (unlikely(result != MEMTX_OK)) {
+            fi->type = ARMFault_SyncExternalOnWalk;
+            fi->ea = arm_extabort_type(result);
+            return 0;
+        }
     }
-    if (result == MEMTX_OK) {
-        return data;
-    }
-    fi->type = ARMFault_SyncExternalOnWalk;
-    fi->ea = arm_extabort_type(result);
-    return 0;
+    return data;
 }
 
 static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index 3462a6ea14..69b0dc69df 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -208,10 +208,21 @@  bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                       bool probe, uintptr_t retaddr)
 {
     ARMCPU *cpu = ARM_CPU(cs);
-    ARMMMUFaultInfo fi = {};
     GetPhysAddrResult res = {};
+    ARMMMUFaultInfo local_fi, *fi;
     int ret;
 
+    /*
+     * Allow S1_ptw_translate to see any fault generated here.
+     * Since this may recurse, read and clear.
+     */
+    fi = cpu->env.tlb_fi;
+    if (fi) {
+        cpu->env.tlb_fi = NULL;
+    } else {
+        fi = memset(&local_fi, 0, sizeof(local_fi));
+    }
+
     /*
      * Walk the page table and (if the mapping exists) add the page
      * to the TLB.  On success, return true.  Otherwise, if probing,
@@ -220,7 +231,7 @@  bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
      */
     ret = get_phys_addr(&cpu->env, address, access_type,
                         core_to_arm_mmu_idx(&cpu->env, mmu_idx),
-                        &res, &fi);
+                        &res, fi);
     if (likely(!ret)) {
         /*
          * Map a single [sub]page. Regions smaller than our declared
@@ -242,7 +253,7 @@  bool arm_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     } else {
         /* now we have a real cpu fault */
         cpu_restore_state(cs, retaddr, true);
-        arm_deliver_fault(cpu, address, access_type, mmu_idx, &fi);
+        arm_deliver_fault(cpu, address, access_type, mmu_idx, fi);
     }
 }
 #else