[Qemu-devel] [PATCH] target-arm: handle address translations that start at level 3

[Qemu-devel] [PATCH] target-arm: handle address translations that start at level 3

[Peter Maydell]

The ARMv8 address translation system defines that a page table walk
starts at a level which depends on the translation granule size
and the number of bits of virtual address that need to be resolved.
Where the translation granule is 64KB and the guest sets the
TCR.TxSZ field to between 35 and 39, it's actually possible to
start at level 3 (the final level). QEMU's implementation failed
to handle this case, and so we would set level to 2 and behave
incorrectly (including invoking the C undefined behaviour of
shifting left by a negative number). Correct the code that
determines the starting level to deal with the start-at-3 case,
by replacing the if-else ladder with an expression derived from
the ARM ARM pseudocode version.

This error was detected by the Coverity scan, which spotted
the potential shift by a negative number.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index c47487a..b74d348 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4545,16 +4545,18 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         goto do_fault;
     }
 
-    /* The starting level depends on the virtual address size which can be
-     * up to 48-bits and the translation granule size.
+    /* The starting level depends on the virtual address size (which can be
+     * up to 48 bits) and the translation granule size. It indicates the number
+     * of strides (granule_sz bits at a time) needed to consume the bits
+     * of the input address. In the pseudocode this is:
+     *  level = 4 - RoundUp((inputsize - grainsize) / stride)
+     * where their 'inputsize' is our 'va_size - tsz', 'grainsize' is
+     * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
+     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
+     *     = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / granule_sz
+     *     = 4 - (va_size - tsz - 4) / granule_sz;
      */
-    if ((va_size - tsz) > (granule_sz * 4 + 3)) {
-        level = 0;
-    } else if ((va_size - tsz) > (granule_sz * 3 + 3)) {
-        level = 1;
-    } else {
-        level = 2;
-    }
+    level = 4 - (va_size - tsz - 4) / granule_sz;
 
     /* 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
-- 
1.9.1

Re: [Qemu-devel] [PATCH] target-arm: handle address translations that start at level 3

[Peter Maydell]

On 13 November 2014 14:56, Peter Maydell <peter.maydell@linaro.org> wrote:
> The ARMv8 address translation system defines that a page table walk
> starts at a level which depends on the translation granule size
> and the number of bits of virtual address that need to be resolved.
> Where the translation granule is 64KB and the guest sets the
> TCR.TxSZ field to between 35 and 39, it's actually possible to
> start at level 3 (the final level). QEMU's implementation failed
> to handle this case, and so we would set level to 2 and behave
> incorrectly (including invoking the C undefined behaviour of
> shifting left by a negative number). Correct the code that
> determines the starting level to deal with the start-at-3 case,
> by replacing the if-else ladder with an expression derived from
> the ARM ARM pseudocode version.
>
> This error was detected by the Coverity scan, which spotted
> the potential shift by a negative number.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Since this would be the sole patch in a target-arm pullreq
for 2.2rc2, I'm just going to apply it directly to master...

-- PMM