Re: [Qemu-devel] [PATCH] target-arm: Fix descriptor address masking in ARM address translation

[Peter Maydell]

On 4 March 2016 at 23:04, Sergey Sorokin <address@hidden> 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.

This is pretty confusing to understand -- it might help if you
could give an example.

Is this something that only causes problems for the "concatenated
translation tables at the initial level" case (which is only
possible for S2 tables) ?

> Signed-off-by: Sergey Sorokin <address@hidden>
> ---
>  target-arm/helper.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index dec8e8b..b5f289c 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7243,7 +7243,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;
> @@ -7431,28 +7431,18 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>          }
>      }
>
> -    /* 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.
> +    /* The address field in the descriptor goes up to bit 39 for AArch32
> +     * but up to bit 47 for AArch64.
>       */

This is not correct -- the descriptor field widths are as the comment
states before your patch:
 * up to bit 39 for ARMv7
 * up to bit 47 for ARMv8 (whether AArch32 or AArch64)

See the v8 ARM ARM AArch32.TranslationTableWalkLD pseudocode and in
particular note the width which it uses for AddressSizeFault checks.

> -    if (arm_feature(env, ARM_FEATURE_V8)) {
> -        descaddrmask = 0xfffffffff000ULL;
> -    } else {
> -        descaddrmask = 0xfffffff000ULL;
> -    }
> +    descaddrmask = ((1ull << (va_size == 64 ? 48 : 40)) - 1) &
> +                   ~indexmask_grainsize;

...so this part of the patch is wrong.

>
>      /* Secure accesses start with the page table in secure memory and
>       * can be downgraded to non-secure at any step. Non-secure accesses
> @@ -7464,7 +7454,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);
> @@ -7487,6 +7477,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.
> --
> 1.9.3
>

thanks
-- PMM