A non-recursive implementation allows to make better use of the
branch predictor, avoids function calls, and makes the implementation of
new features only for a subset of region table levels easier.

We can now directly compare our implementation to the KVM gaccess
implementation in arch/s390/kvm/gaccess.c:guest_translate().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/mmu_helper.c | 212 ++++++++++++++++++++------------------
 1 file changed, 112 insertions(+), 100 deletions(-)

diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
index a114fb1628..6b34c4c7b4 100644
--- a/target/s390x/mmu_helper.c
+++ b/target/s390x/mmu_helper.c
@@ -114,107 +114,16 @@ static inline bool read_table_entry(CPUS390XState *env, hwaddr gaddr,
     return true;
 }
 
-/* Decode page table entry (normal 4KB page) */
-static int mmu_translate_pte(CPUS390XState *env, target_ulong vaddr,
-                             uint64_t asc, uint64_t pt_entry,
-                             target_ulong *raddr, int *flags, int rw, bool exc)
-{
-    if (pt_entry & PAGE_ENTRY_I) {
-        return PGM_PAGE_TRANS;
-    }
-    if (pt_entry & PAGE_ENTRY_0) {
-        return PGM_TRANS_SPEC;
-    }
-    if (pt_entry & PAGE_ENTRY_P) {
-        *flags &= ~PAGE_WRITE;
-    }
-
-    *raddr = pt_entry & TARGET_PAGE_MASK;
-    return 0;
-}
-
-/* Decode segment table entry */
-static int mmu_translate_segment(CPUS390XState *env, target_ulong vaddr,
-                                 uint64_t asc, uint64_t st_entry,
-                                 target_ulong *raddr, int *flags, int rw,
-                                 bool exc)
-{
-    uint64_t origin, offs, pt_entry;
-
-    if (st_entry & SEGMENT_ENTRY_P) {
-        *flags &= ~PAGE_WRITE;
-    }
-
-    if ((st_entry & SEGMENT_ENTRY_FC) && (env->cregs[0] & CR0_EDAT)) {
-        /* Decode EDAT1 segment frame absolute address (1MB page) */
-        *raddr = (st_entry & SEGMENT_ENTRY_SFAA) |
-                 (vaddr & ~SEGMENT_ENTRY_SFAA);
-        return 0;
-    }
-
-    /* Look up 4KB page entry */
-    origin = st_entry & SEGMENT_ENTRY_ORIGIN;
-    offs = VADDR_PAGE_TX(vaddr) * 8;
-    if (!read_table_entry(env, origin + offs, &pt_entry)) {
-        return PGM_ADDRESSING;
-    }
-    return mmu_translate_pte(env, vaddr, asc, pt_entry, raddr, flags, rw, exc);
-}
-
-/* Decode region table entries */
-static int mmu_translate_region(CPUS390XState *env, target_ulong vaddr,
-                                uint64_t asc, uint64_t entry, int level,
-                                target_ulong *raddr, int *flags, int rw,
-                                bool exc)
-{
-    uint64_t origin, offs, new_entry;
-    const int pchks[4] = {
-        PGM_SEGMENT_TRANS, PGM_REG_THIRD_TRANS,
-        PGM_REG_SEC_TRANS, PGM_REG_FIRST_TRANS
-    };
-
-    origin = entry & REGION_ENTRY_ORIGIN;
-    offs = (vaddr >> (17 + 11 * level / 4)) & 0x3ff8;
-
-    if (!read_table_entry(env, origin + offs, &new_entry)) {
-        return PGM_ADDRESSING;
-    }
-
-    if (new_entry & REGION_ENTRY_I) {
-        return pchks[level / 4];
-    }
-
-    if ((new_entry & REGION_ENTRY_TT) != level) {
-        return PGM_TRANS_SPEC;
-    }
-
-    if (level == ASCE_TYPE_SEGMENT) {
-        return mmu_translate_segment(env, vaddr, asc, new_entry, raddr, flags,
-                                     rw, exc);
-    }
-
-    /* Check region table offset and length */
-    offs = (vaddr >> (28 + 11 * (level - 4) / 4)) & 3;
-    if (offs < ((new_entry & REGION_ENTRY_TF) >> 6)
-        || offs > (new_entry & REGION_ENTRY_TL)) {
-        return pchks[level / 4 - 1];
-    }
-
-    if ((env->cregs[0] & CR0_EDAT) && (new_entry & REGION_ENTRY_P)) {
-        *flags &= ~PAGE_WRITE;
-    }
-
-    /* yet another region */
-    return mmu_translate_region(env, vaddr, asc, new_entry, level - 4,
-                                raddr, flags, rw, exc);
-}
-
 static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
                               uint64_t asc, uint64_t asce, target_ulong *raddr,
                               int *flags, int rw, bool exc)
 {
+    const bool edat1 = (env->cregs[0] & CR0_EDAT) &&
+                       s390_has_feat(S390_FEAT_EDAT);
     const int asce_tl = asce & ASCE_TABLE_LENGTH;
-    int level;
+    const int asce_p = asce & ASCE_PRIVATE_SPACE;
+    hwaddr gaddr = asce & ASCE_ORIGIN;
+    uint64_t entry;
 
     if (asce & ASCE_REAL_SPACE) {
         /* direct mapping */
@@ -222,12 +131,12 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
         return 0;
     }
 
-    level = asce & ASCE_TYPE_MASK;
-    switch (level) {
+    switch (asce & ASCE_TYPE_MASK) {
     case ASCE_TYPE_REGION1:
         if (VADDR_REGION1_TL(vaddr) > asce_tl) {
             return PGM_REG_FIRST_TRANS;
         }
+        gaddr += VADDR_REGION1_TX(vaddr) * 8;
         break;
     case ASCE_TYPE_REGION2:
         if (VADDR_REGION1_TX(vaddr)) {
@@ -236,6 +145,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
         if (VADDR_REGION2_TL(vaddr) > asce_tl) {
             return PGM_REG_SEC_TRANS;
         }
+        gaddr += VADDR_REGION2_TX(vaddr) * 8;
         break;
     case ASCE_TYPE_REGION3:
         if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr)) {
@@ -244,6 +154,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
         if (VADDR_REGION3_TL(vaddr) > asce_tl) {
             return PGM_REG_THIRD_TRANS;
         }
+        gaddr += VADDR_REGION3_TX(vaddr) * 8;
         break;
     case ASCE_TYPE_SEGMENT:
         if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr) ||
@@ -253,11 +164,112 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
         if (VADDR_SEGMENT_TL(vaddr) > asce_tl) {
             return PGM_SEGMENT_TRANS;
         }
+        gaddr += VADDR_SEGMENT_TX(vaddr) * 8;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    switch (asce & ASCE_TYPE_MASK) {
+    case ASCE_TYPE_REGION1:
+        if (!read_table_entry(env, gaddr, &entry)) {
+            return PGM_ADDRESSING;
+        }
+        if (entry & REGION_ENTRY_I) {
+            return PGM_REG_FIRST_TRANS;
+        }
+        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
+            return PGM_TRANS_SPEC;
+        }
+        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
+            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
+            return PGM_REG_SEC_TRANS;
+        }
+        if (edat1 && (entry & REGION_ENTRY_P)) {
+            *flags &= ~PAGE_WRITE;
+        }
+        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
+        /* fall through */
+    case ASCE_TYPE_REGION2:
+        if (!read_table_entry(env, gaddr, &entry)) {
+            return PGM_ADDRESSING;
+        }
+        if (entry & REGION_ENTRY_I) {
+            return PGM_REG_SEC_TRANS;
+        }
+        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
+            return PGM_TRANS_SPEC;
+        }
+        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
+            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
+            return PGM_REG_THIRD_TRANS;
+        }
+        if (edat1 && (entry & REGION_ENTRY_P)) {
+            *flags &= ~PAGE_WRITE;
+        }
+        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
+        /* fall through */
+    case ASCE_TYPE_REGION3:
+        if (!read_table_entry(env, gaddr, &entry)) {
+            return PGM_ADDRESSING;
+        }
+        if (entry & REGION_ENTRY_I) {
+            return PGM_REG_THIRD_TRANS;
+        }
+        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
+            return PGM_TRANS_SPEC;
+        }
+        if (edat1 && (entry & REGION_ENTRY_P)) {
+            *flags &= ~PAGE_WRITE;
+        }
+        if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
+            VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
+            return PGM_SEGMENT_TRANS;
+        }
+        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_SEGMENT_TX(vaddr) * 8;
+        /* fall through */
+    case ASCE_TYPE_SEGMENT:
+        if (!read_table_entry(env, gaddr, &entry)) {
+            return PGM_ADDRESSING;
+        }
+        if (entry & SEGMENT_ENTRY_I) {
+            return PGM_SEGMENT_TRANS;
+        }
+        if ((entry & SEGMENT_ENTRY_TT) != SEGMENT_ENTRY_TT_SEGMENT) {
+            return PGM_TRANS_SPEC;
+        }
+        if ((entry & SEGMENT_ENTRY_CS) && asce_p) {
+            return PGM_TRANS_SPEC;
+        }
+        if (entry & SEGMENT_ENTRY_P) {
+            *flags &= ~PAGE_WRITE;
+        }
+        if (edat1 && (entry & SEGMENT_ENTRY_FC)) {
+            *raddr = (entry & SEGMENT_ENTRY_SFAA) |
+                     (vaddr & ~SEGMENT_ENTRY_SFAA);
+            return 0;
+        }
+        gaddr = (entry & SEGMENT_ENTRY_ORIGIN) + VADDR_PAGE_TX(vaddr) * 8;
         break;
+    default:
+        g_assert_not_reached();
+    }
+
+    if (!read_table_entry(env, gaddr, &entry)) {
+        return PGM_ADDRESSING;
+    }
+    if (entry & PAGE_ENTRY_I) {
+        return PGM_PAGE_TRANS;
+    }
+    if (entry & PAGE_ENTRY_0) {
+        return PGM_TRANS_SPEC;
+    }
+    if (entry & PAGE_ENTRY_P) {
+        *flags &= ~PAGE_WRITE;
     }
 
-    return mmu_translate_region(env, vaddr, asc, asce, level, raddr, flags, rw,
-                                exc);
+    *raddr = entry & TARGET_PAGE_MASK;
+    return 0;
 }
 
 static void mmu_handle_skey(target_ulong addr, int rw, int *flags)
-- 
2.21.0

On 27/09/2019 11.58, David Hildenbrand wrote:
> A non-recursive implementation allows to make better use of the
> branch predictor, avoids function calls, and makes the implementation of
> new features only for a subset of region table levels easier.
> 
> We can now directly compare our implementation to the KVM gaccess
> implementation in arch/s390/kvm/gaccess.c:guest_translate().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mmu_helper.c | 212 ++++++++++++++++++++------------------
>  1 file changed, 112 insertions(+), 100 deletions(-)
> 
> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c
> index a114fb1628..6b34c4c7b4 100644
> --- a/target/s390x/mmu_helper.c
> +++ b/target/s390x/mmu_helper.c
> @@ -114,107 +114,16 @@ static inline bool read_table_entry(CPUS390XState *env, hwaddr gaddr,
>      return true;
>  }
>  
> -/* Decode page table entry (normal 4KB page) */
> -static int mmu_translate_pte(CPUS390XState *env, target_ulong vaddr,
> -                             uint64_t asc, uint64_t pt_entry,
> -                             target_ulong *raddr, int *flags, int rw, bool exc)
> -{
> -    if (pt_entry & PAGE_ENTRY_I) {
> -        return PGM_PAGE_TRANS;
> -    }
> -    if (pt_entry & PAGE_ENTRY_0) {
> -        return PGM_TRANS_SPEC;
> -    }
> -    if (pt_entry & PAGE_ENTRY_P) {
> -        *flags &= ~PAGE_WRITE;
> -    }
> -
> -    *raddr = pt_entry & TARGET_PAGE_MASK;
> -    return 0;
> -}
> -
> -/* Decode segment table entry */
> -static int mmu_translate_segment(CPUS390XState *env, target_ulong vaddr,
> -                                 uint64_t asc, uint64_t st_entry,
> -                                 target_ulong *raddr, int *flags, int rw,
> -                                 bool exc)
> -{
> -    uint64_t origin, offs, pt_entry;
> -
> -    if (st_entry & SEGMENT_ENTRY_P) {
> -        *flags &= ~PAGE_WRITE;
> -    }
> -
> -    if ((st_entry & SEGMENT_ENTRY_FC) && (env->cregs[0] & CR0_EDAT)) {
> -        /* Decode EDAT1 segment frame absolute address (1MB page) */
> -        *raddr = (st_entry & SEGMENT_ENTRY_SFAA) |
> -                 (vaddr & ~SEGMENT_ENTRY_SFAA);
> -        return 0;
> -    }
> -
> -    /* Look up 4KB page entry */
> -    origin = st_entry & SEGMENT_ENTRY_ORIGIN;
> -    offs = VADDR_PAGE_TX(vaddr) * 8;
> -    if (!read_table_entry(env, origin + offs, &pt_entry)) {
> -        return PGM_ADDRESSING;
> -    }
> -    return mmu_translate_pte(env, vaddr, asc, pt_entry, raddr, flags, rw, exc);
> -}
> -
> -/* Decode region table entries */
> -static int mmu_translate_region(CPUS390XState *env, target_ulong vaddr,
> -                                uint64_t asc, uint64_t entry, int level,
> -                                target_ulong *raddr, int *flags, int rw,
> -                                bool exc)
> -{
> -    uint64_t origin, offs, new_entry;
> -    const int pchks[4] = {
> -        PGM_SEGMENT_TRANS, PGM_REG_THIRD_TRANS,
> -        PGM_REG_SEC_TRANS, PGM_REG_FIRST_TRANS
> -    };
> -
> -    origin = entry & REGION_ENTRY_ORIGIN;
> -    offs = (vaddr >> (17 + 11 * level / 4)) & 0x3ff8;
> -
> -    if (!read_table_entry(env, origin + offs, &new_entry)) {
> -        return PGM_ADDRESSING;
> -    }
> -
> -    if (new_entry & REGION_ENTRY_I) {
> -        return pchks[level / 4];
> -    }
> -
> -    if ((new_entry & REGION_ENTRY_TT) != level) {
> -        return PGM_TRANS_SPEC;
> -    }
> -
> -    if (level == ASCE_TYPE_SEGMENT) {
> -        return mmu_translate_segment(env, vaddr, asc, new_entry, raddr, flags,
> -                                     rw, exc);
> -    }
> -
> -    /* Check region table offset and length */
> -    offs = (vaddr >> (28 + 11 * (level - 4) / 4)) & 3;
> -    if (offs < ((new_entry & REGION_ENTRY_TF) >> 6)
> -        || offs > (new_entry & REGION_ENTRY_TL)) {
> -        return pchks[level / 4 - 1];
> -    }
> -
> -    if ((env->cregs[0] & CR0_EDAT) && (new_entry & REGION_ENTRY_P)) {
> -        *flags &= ~PAGE_WRITE;
> -    }
> -
> -    /* yet another region */
> -    return mmu_translate_region(env, vaddr, asc, new_entry, level - 4,
> -                                raddr, flags, rw, exc);
> -}
> -
>  static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>                                uint64_t asc, uint64_t asce, target_ulong *raddr,
>                                int *flags, int rw, bool exc)
>  {
> +    const bool edat1 = (env->cregs[0] & CR0_EDAT) &&
> +                       s390_has_feat(S390_FEAT_EDAT);
>      const int asce_tl = asce & ASCE_TABLE_LENGTH;
> -    int level;
> +    const int asce_p = asce & ASCE_PRIVATE_SPACE;
> +    hwaddr gaddr = asce & ASCE_ORIGIN;
> +    uint64_t entry;
>  
>      if (asce & ASCE_REAL_SPACE) {
>          /* direct mapping */
> @@ -222,12 +131,12 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>          return 0;
>      }
>  
> -    level = asce & ASCE_TYPE_MASK;
> -    switch (level) {
> +    switch (asce & ASCE_TYPE_MASK) {
>      case ASCE_TYPE_REGION1:
>          if (VADDR_REGION1_TL(vaddr) > asce_tl) {
>              return PGM_REG_FIRST_TRANS;
>          }
> +        gaddr += VADDR_REGION1_TX(vaddr) * 8;
>          break;
>      case ASCE_TYPE_REGION2:
>          if (VADDR_REGION1_TX(vaddr)) {
> @@ -236,6 +145,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>          if (VADDR_REGION2_TL(vaddr) > asce_tl) {
>              return PGM_REG_SEC_TRANS;
>          }
> +        gaddr += VADDR_REGION2_TX(vaddr) * 8;
>          break;
>      case ASCE_TYPE_REGION3:
>          if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr)) {
> @@ -244,6 +154,7 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>          if (VADDR_REGION3_TL(vaddr) > asce_tl) {
>              return PGM_REG_THIRD_TRANS;
>          }
> +        gaddr += VADDR_REGION3_TX(vaddr) * 8;
>          break;
>      case ASCE_TYPE_SEGMENT:
>          if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr) ||
> @@ -253,11 +164,112 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>          if (VADDR_SEGMENT_TL(vaddr) > asce_tl) {
>              return PGM_SEGMENT_TRANS;
>          }
> +        gaddr += VADDR_SEGMENT_TX(vaddr) * 8;
> +        break;
> +    default:
> +        g_assert_not_reached();

As far as I can see, all four cases are handled above, so this default
case should really not be necessary here.

> +    }
> +
> +    switch (asce & ASCE_TYPE_MASK) {
> +    case ASCE_TYPE_REGION1:
> +        if (!read_table_entry(env, gaddr, &entry)) {
> +            return PGM_ADDRESSING;
> +        }
> +        if (entry & REGION_ENTRY_I) {
> +            return PGM_REG_FIRST_TRANS;
> +        }
> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
> +            return PGM_TRANS_SPEC;
> +        }
> +        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
> +            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
> +            return PGM_REG_SEC_TRANS;
> +        }
> +        if (edat1 && (entry & REGION_ENTRY_P)) {
> +            *flags &= ~PAGE_WRITE;
> +        }
> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
> +        /* fall through */
> +    case ASCE_TYPE_REGION2:
> +        if (!read_table_entry(env, gaddr, &entry)) {
> +            return PGM_ADDRESSING;
> +        }
> +        if (entry & REGION_ENTRY_I) {
> +            return PGM_REG_SEC_TRANS;
> +        }
> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
> +            return PGM_TRANS_SPEC;
> +        }
> +        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
> +            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
> +            return PGM_REG_THIRD_TRANS;
> +        }
> +        if (edat1 && (entry & REGION_ENTRY_P)) {
> +            *flags &= ~PAGE_WRITE;
> +        }
> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
> +        /* fall through */
> +    case ASCE_TYPE_REGION3:
> +        if (!read_table_entry(env, gaddr, &entry)) {
> +            return PGM_ADDRESSING;
> +        }
> +        if (entry & REGION_ENTRY_I) {
> +            return PGM_REG_THIRD_TRANS;
> +        }
> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
> +            return PGM_TRANS_SPEC;
> +        }
> +        if (edat1 && (entry & REGION_ENTRY_P)) {
> +            *flags &= ~PAGE_WRITE;
> +        }

Shouldn't that check be done below the next if-statement?

> +        if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
> +            VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
> +            return PGM_SEGMENT_TRANS;
> +        }
> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_SEGMENT_TX(vaddr) * 8;
> +        /* fall through */
> +    case ASCE_TYPE_SEGMENT:
> +        if (!read_table_entry(env, gaddr, &entry)) {
> +            return PGM_ADDRESSING;
> +        }
> +        if (entry & SEGMENT_ENTRY_I) {
> +            return PGM_SEGMENT_TRANS;
> +        }
> +        if ((entry & SEGMENT_ENTRY_TT) != SEGMENT_ENTRY_TT_SEGMENT) {
> +            return PGM_TRANS_SPEC;
> +        }
> +        if ((entry & SEGMENT_ENTRY_CS) && asce_p) {
> +            return PGM_TRANS_SPEC;
> +        }
> +        if (entry & SEGMENT_ENTRY_P) {
> +            *flags &= ~PAGE_WRITE;
> +        }
> +        if (edat1 && (entry & SEGMENT_ENTRY_FC)) {
> +            *raddr = (entry & SEGMENT_ENTRY_SFAA) |
> +                     (vaddr & ~SEGMENT_ENTRY_SFAA);
> +            return 0;
> +        }
> +        gaddr = (entry & SEGMENT_ENTRY_ORIGIN) + VADDR_PAGE_TX(vaddr) * 8;
>          break;
> +    default:
> +        g_assert_not_reached();

That default case could be dropped, too.

> +    }
> +
> +    if (!read_table_entry(env, gaddr, &entry)) {
> +        return PGM_ADDRESSING;
> +    }
> +    if (entry & PAGE_ENTRY_I) {
> +        return PGM_PAGE_TRANS;
> +    }
> +    if (entry & PAGE_ENTRY_0) {
> +        return PGM_TRANS_SPEC;
> +    }
> +    if (entry & PAGE_ENTRY_P) {
> +        *flags &= ~PAGE_WRITE;
>      }
>  
> -    return mmu_translate_region(env, vaddr, asc, asce, level, raddr, flags, rw,
> -                                exc);
> +    *raddr = entry & TARGET_PAGE_MASK;
> +    return 0;
>  }

 Thomas

>>          break;
>>      case ASCE_TYPE_SEGMENT:
>>          if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr) ||
>> @@ -253,11 +164,112 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>>          if (VADDR_SEGMENT_TL(vaddr) > asce_tl) {
>>              return PGM_SEGMENT_TRANS;
>>          }
>> +        gaddr += VADDR_SEGMENT_TX(vaddr) * 8;
>> +        break;
>> +    default:
>> +        g_assert_not_reached();
> 
> As far as I can see, all four cases are handled above, so this default
> case should really not be necessary here.

Yes, can drop.

> 
>> +    }
>> +
>> +    switch (asce & ASCE_TYPE_MASK) {
>> +    case ASCE_TYPE_REGION1:
>> +        if (!read_table_entry(env, gaddr, &entry)) {
>> +            return PGM_ADDRESSING;
>> +        }
>> +        if (entry & REGION_ENTRY_I) {
>> +            return PGM_REG_FIRST_TRANS;
>> +        }
>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>> +            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>> +            return PGM_REG_SEC_TRANS;
>> +        }
>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>> +            *flags &= ~PAGE_WRITE;
>> +        }
>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
>> +        /* fall through */
>> +    case ASCE_TYPE_REGION2:
>> +        if (!read_table_entry(env, gaddr, &entry)) {
>> +            return PGM_ADDRESSING;
>> +        }
>> +        if (entry & REGION_ENTRY_I) {
>> +            return PGM_REG_SEC_TRANS;
>> +        }
>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>> +            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>> +            return PGM_REG_THIRD_TRANS;
>> +        }
>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>> +            *flags &= ~PAGE_WRITE;
>> +        }
>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
>> +        /* fall through */
>> +    case ASCE_TYPE_REGION3:
>> +        if (!read_table_entry(env, gaddr, &entry)) {
>> +            return PGM_ADDRESSING;
>> +        }
>> +        if (entry & REGION_ENTRY_I) {
>> +            return PGM_REG_THIRD_TRANS;
>> +        }
>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>> +            *flags &= ~PAGE_WRITE;
>> +        }
> 
> Shouldn't that check be done below the next if-statement?

Does it matter? The flags are irrelevant in case we return an exception,
so the order shouldn't matter.

> 
>> +        if (VADDR_SEGMENT_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>> +            VADDR_SEGMENT_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>> +            return PGM_SEGMENT_TRANS;
>> +        }
>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_SEGMENT_TX(vaddr) * 8;
>> +        /* fall through */
>> +    case ASCE_TYPE_SEGMENT:
>> +        if (!read_table_entry(env, gaddr, &entry)) {
>> +            return PGM_ADDRESSING;
>> +        }
>> +        if (entry & SEGMENT_ENTRY_I) {
>> +            return PGM_SEGMENT_TRANS;
>> +        }
>> +        if ((entry & SEGMENT_ENTRY_TT) != SEGMENT_ENTRY_TT_SEGMENT) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if ((entry & SEGMENT_ENTRY_CS) && asce_p) {
>> +            return PGM_TRANS_SPEC;
>> +        }
>> +        if (entry & SEGMENT_ENTRY_P) {
>> +            *flags &= ~PAGE_WRITE;
>> +        }
>> +        if (edat1 && (entry & SEGMENT_ENTRY_FC)) {
>> +            *raddr = (entry & SEGMENT_ENTRY_SFAA) |
>> +                     (vaddr & ~SEGMENT_ENTRY_SFAA);
>> +            return 0;
>> +        }
>> +        gaddr = (entry & SEGMENT_ENTRY_ORIGIN) + VADDR_PAGE_TX(vaddr) * 8;
>>          break;
>> +    default:
>> +        g_assert_not_reached();
> 
> That default case could be dropped, too.

Yes, can do.

Thanks!


-- 

Thanks,

David / dhildenb

On 01/10/2019 10.17, David Hildenbrand wrote:
> 
>>>          break;
>>>      case ASCE_TYPE_SEGMENT:
>>>          if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr) ||
>>> @@ -253,11 +164,112 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>>>          if (VADDR_SEGMENT_TL(vaddr) > asce_tl) {
>>>              return PGM_SEGMENT_TRANS;
>>>          }
>>> +        gaddr += VADDR_SEGMENT_TX(vaddr) * 8;
>>> +        break;
>>> +    default:
>>> +        g_assert_not_reached();
>>
>> As far as I can see, all four cases are handled above, so this default
>> case should really not be necessary here.
> 
> Yes, can drop.
> 
>>
>>> +    }
>>> +
>>> +    switch (asce & ASCE_TYPE_MASK) {
>>> +    case ASCE_TYPE_REGION1:
>>> +        if (!read_table_entry(env, gaddr, &entry)) {
>>> +            return PGM_ADDRESSING;
>>> +        }
>>> +        if (entry & REGION_ENTRY_I) {
>>> +            return PGM_REG_FIRST_TRANS;
>>> +        }
>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
>>> +            return PGM_TRANS_SPEC;
>>> +        }
>>> +        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>> +            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>> +            return PGM_REG_SEC_TRANS;
>>> +        }
>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>> +            *flags &= ~PAGE_WRITE;
>>> +        }
>>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
>>> +        /* fall through */
>>> +    case ASCE_TYPE_REGION2:
>>> +        if (!read_table_entry(env, gaddr, &entry)) {
>>> +            return PGM_ADDRESSING;
>>> +        }
>>> +        if (entry & REGION_ENTRY_I) {
>>> +            return PGM_REG_SEC_TRANS;
>>> +        }
>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
>>> +            return PGM_TRANS_SPEC;
>>> +        }
>>> +        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>> +            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>> +            return PGM_REG_THIRD_TRANS;
>>> +        }
>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>> +            *flags &= ~PAGE_WRITE;
>>> +        }
>>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
>>> +        /* fall through */
>>> +    case ASCE_TYPE_REGION3:
>>> +        if (!read_table_entry(env, gaddr, &entry)) {
>>> +            return PGM_ADDRESSING;
>>> +        }
>>> +        if (entry & REGION_ENTRY_I) {
>>> +            return PGM_REG_THIRD_TRANS;
>>> +        }
>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
>>> +            return PGM_TRANS_SPEC;
>>> +        }
>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>> +            *flags &= ~PAGE_WRITE;
>>> +        }
>>
>> Shouldn't that check be done below the next if-statement?
> 
> Does it matter? The flags are irrelevant in case we return an exception,
> so the order shouldn't matter.

Hmm, it likely does not matter, but you've got it the other way round in
all other cases, so I'd vote for doing it here this way, too, for
consistency.

 Thomas

On 01.10.19 10:23, Thomas Huth wrote:
> On 01/10/2019 10.17, David Hildenbrand wrote:
>>
>>>>          break;
>>>>      case ASCE_TYPE_SEGMENT:
>>>>          if (VADDR_REGION1_TX(vaddr) || VADDR_REGION2_TX(vaddr) ||
>>>> @@ -253,11 +164,112 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
>>>>          if (VADDR_SEGMENT_TL(vaddr) > asce_tl) {
>>>>              return PGM_SEGMENT_TRANS;
>>>>          }
>>>> +        gaddr += VADDR_SEGMENT_TX(vaddr) * 8;
>>>> +        break;
>>>> +    default:
>>>> +        g_assert_not_reached();
>>>
>>> As far as I can see, all four cases are handled above, so this default
>>> case should really not be necessary here.
>>
>> Yes, can drop.
>>
>>>
>>>> +    }
>>>> +
>>>> +    switch (asce & ASCE_TYPE_MASK) {
>>>> +    case ASCE_TYPE_REGION1:
>>>> +        if (!read_table_entry(env, gaddr, &entry)) {
>>>> +            return PGM_ADDRESSING;
>>>> +        }
>>>> +        if (entry & REGION_ENTRY_I) {
>>>> +            return PGM_REG_FIRST_TRANS;
>>>> +        }
>>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION1) {
>>>> +            return PGM_TRANS_SPEC;
>>>> +        }
>>>> +        if (VADDR_REGION2_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>>> +            VADDR_REGION2_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>>> +            return PGM_REG_SEC_TRANS;
>>>> +        }
>>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>>> +            *flags &= ~PAGE_WRITE;
>>>> +        }
>>>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION2_TX(vaddr) * 8;
>>>> +        /* fall through */
>>>> +    case ASCE_TYPE_REGION2:
>>>> +        if (!read_table_entry(env, gaddr, &entry)) {
>>>> +            return PGM_ADDRESSING;
>>>> +        }
>>>> +        if (entry & REGION_ENTRY_I) {
>>>> +            return PGM_REG_SEC_TRANS;
>>>> +        }
>>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION2) {
>>>> +            return PGM_TRANS_SPEC;
>>>> +        }
>>>> +        if (VADDR_REGION3_TL(vaddr) < (entry & REGION_ENTRY_TF) >> 6 ||
>>>> +            VADDR_REGION3_TL(vaddr) > (entry & REGION_ENTRY_TL)) {
>>>> +            return PGM_REG_THIRD_TRANS;
>>>> +        }
>>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>>> +            *flags &= ~PAGE_WRITE;
>>>> +        }
>>>> +        gaddr = (entry & REGION_ENTRY_ORIGIN) + VADDR_REGION3_TX(vaddr) * 8;
>>>> +        /* fall through */
>>>> +    case ASCE_TYPE_REGION3:
>>>> +        if (!read_table_entry(env, gaddr, &entry)) {
>>>> +            return PGM_ADDRESSING;
>>>> +        }
>>>> +        if (entry & REGION_ENTRY_I) {
>>>> +            return PGM_REG_THIRD_TRANS;
>>>> +        }
>>>> +        if ((entry & REGION_ENTRY_TT) != REGION_ENTRY_TT_REGION3) {
>>>> +            return PGM_TRANS_SPEC;
>>>> +        }
>>>> +        if (edat1 && (entry & REGION_ENTRY_P)) {
>>>> +            *flags &= ~PAGE_WRITE;
>>>> +        }
>>>
>>> Shouldn't that check be done below the next if-statement?
>>
>> Does it matter? The flags are irrelevant in case we return an exception,
>> so the order shouldn't matter.
> 
> Hmm, it likely does not matter, but you've got it the other way round in
> all other cases, so I'd vote for doing it here this way, too, for
> consistency.

Oh, in this case, sure! Thanks!

> 
>  Thomas
> 


-- 

Thanks,

David / dhildenb

On 9/27/19 2:58 AM, David Hildenbrand wrote:
> A non-recursive implementation allows to make better use of the
> branch predictor, avoids function calls, and makes the implementation of
> new features only for a subset of region table levels easier.
> 
> We can now directly compare our implementation to the KVM gaccess
> implementation in arch/s390/kvm/gaccess.c:guest_translate().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/mmu_helper.c | 212 ++++++++++++++++++++------------------
>  1 file changed, 112 insertions(+), 100 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~