[v2] target-arm: Fix descriptor address masking in ARM address translation

Commit Message

Sergey Sorokin April 18, 2016, 4:27 p.m. UTC

There is a bug in ARM address translation regime with a long-descriptor
format. On the descriptor reading its address is formed from an index
which is a part of the input address. And on the first iteration this index
is incorrectly masked with 'grainsize' mask. But it can be wider according
to pseudo-code.
On the other hand on the iterations other than first the descriptor address
is formed from the previous level descriptor by masking with 'descaddrmask'
value. It always clears just 12 lower bits, but it must clear 'grainsize'
lower bits instead according to pseudo-code.
The patch fixes both cases.

Signed-off-by: Sergey Sorokin <[e-mail]>
---
Fixed a comment before the calculation of 'descaddrmask' value.

 target-arm/helper.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

Comments

Peter Maydell May 4, 2016, 4:59 p.m. UTC | #1

On 18 April 2016 at 17:27, Sergey Sorokin <[e-mail]> wrote:
> There is a bug in ARM address translation regime with a long-descriptor
> format. On the descriptor reading its address is formed from an index
> which is a part of the input address. And on the first iteration this index
> is incorrectly masked with 'grainsize' mask. But it can be wider according
> to pseudo-code.
> On the other hand on the iterations other than first the descriptor address
> is formed from the previous level descriptor by masking with 'descaddrmask'
> value. It always clears just 12 lower bits, but it must clear 'grainsize'
> lower bits instead according to pseudo-code.
> The patch fixes both cases.
>
> Signed-off-by: Sergey Sorokin <[e-mail]>
>      /* The address field in the descriptor goes up to bit 39 for ARMv7
> -     * but up to bit 47 for ARMv8.
> +     * but up to bit 47 for ARMv8, but we use the descaddrmask
> +     * up to bit 39 for AArch32, because we don't need other bits in that case
> +     * to construct next descriptor address (anyway they should be all zeroes).
>       */
> -    if (arm_feature(env, ARM_FEATURE_V8)) {
> -        descaddrmask = 0xfffffffff000ULL;
> -    } else {
> -        descaddrmask = 0xfffffff000ULL;
> -    }
> +    descaddrmask = ((1ull << (va_size == 64 ? 48 : 40)) - 1) &
> +                   ~indexmask_grainsize;

I still think we are going to end up wanting to revert the
"look at va_size rather than ARM_FEATURE_V8" part of this when
we come to implement AddressSize faults, but let's get this
bug fix in for now rather than continuing to argue about it.

Applied to target-arm.next, thanks.

-- PMM

Patch

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 09638b2..9bf6f17 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7248,7 +7248,7 @@  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     uint32_t tg;
     uint64_t ttbr;
     int ttbr_select;
-    hwaddr descaddr, descmask;
+    hwaddr descaddr, indexmask, indexmask_grainsize;
     uint32_t tableattrs;
     target_ulong page_size;
     uint32_t attrs;
@@ -7437,28 +7437,20 @@  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         level = startlevel;
     }
 
-    /* Clear the vaddr bits which aren't part of the within-region address,
-     * so that we don't have to special case things when calculating the
-     * first descriptor address.
-     */
-    if (va_size != inputsize) {
-        address &= (1ULL << inputsize) - 1;
-    }
-
-    descmask = (1ULL << (stride + 3)) - 1;
+    indexmask_grainsize = (1ULL << (stride + 3)) - 1;
+    indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
 
     /* Now we can extract the actual base address from the TTBR */
     descaddr = extract64(ttbr, 0, 48);
-    descaddr &= ~((1ULL << (inputsize - (stride * (4 - level)))) - 1);
+    descaddr &= ~indexmask;
 
     /* The address field in the descriptor goes up to bit 39 for ARMv7
-     * but up to bit 47 for ARMv8.
+     * but up to bit 47 for ARMv8, but we use the descaddrmask
+     * up to bit 39 for AArch32, because we don't need other bits in that case
+     * to construct next descriptor address (anyway they should be all zeroes).
      */
-    if (arm_feature(env, ARM_FEATURE_V8)) {
-        descaddrmask = 0xfffffffff000ULL;
-    } else {
-        descaddrmask = 0xfffffff000ULL;
-    }
+    descaddrmask = ((1ull << (va_size == 64 ? 48 : 40)) - 1) &
+                   ~indexmask_grainsize;
 
     /* Secure accesses start with the page table in secure memory and
      * can be downgraded to non-secure at any step. Non-secure accesses
@@ -7470,7 +7462,7 @@  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         uint64_t descriptor;
         bool nstable;
 
-        descaddr |= (address >> (stride * (4 - level))) & descmask;
+        descaddr |= (address >> (stride * (4 - level))) & indexmask;
         descaddr &= ~7ULL;
         nstable = extract32(tableattrs, 4, 1);
         descriptor = arm_ldq_ptw(cs, descaddr, !nstable, mmu_idx, fsr, fi);
@@ -7493,6 +7485,7 @@  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
              */
             tableattrs |= extract64(descriptor, 59, 5);
             level++;
+            indexmask = indexmask_grainsize;
             continue;
         }
         /* Block entry at level 1 or 2, or page entry at level 3.