Re: [PATCH v3 39/42] target/arm: Don't shift attrs in get_phys_addr_lpae

[Peter Maydell]

On Sat, 1 Oct 2022 at 17:49, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Leave the upper and lower attributes in the place they originate
> from in the descriptor.  Shifting them around is confusing, since
> one cannot read the bit numbers out of the manual.  Also, new
> attributes have been added which would alter the shifts.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/ptw.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index 01a27b30fb..c68fd73617 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -1071,7 +1071,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> uint64_t address,
>      hwaddr descaddr, indexmask, indexmask_grainsize;
>      uint32_t tableattrs;
>      target_ulong page_size;
> -    uint32_t attrs;
> +    uint64_t attrs;
>      int32_t stride;
>      int addrsize, inputsize, outputsize;
>      uint64_t tcr = regime_tcr(env, mmu_idx);
> @@ -1341,49 +1341,48 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> uint64_t address,
>      descaddr &= ~(page_size - 1);
>      descaddr |= (address & (page_size - 1));
>      /* Extract attributes from the descriptor */
> -    attrs = extract64(descriptor, 2, 10)
> -        | (extract64(descriptor, 52, 12) << 10);
> +    attrs = descriptor & (MAKE_64BIT_MASK(2, 10) | MAKE_64BIT_MASK(52, 12));

So bit positions for the lower-part bits need to have 2 added,
and bit positions for upper-part bits need to have 42 added.

>      if (mmu_idx == ARMMMUIdx_Stage2 || mmu_idx == ARMMMUIdx_Stage2_S) {
>          ns = mmu_idx == ARMMMUIdx_Stage2;
> -        xn = extract32(attrs, 11, 2);
> +        xn = extract64(attrs, 54, 2);

...so this one looks wrong, should be 53 ?

>          result->f.prot = get_S2prot(env, ap, xn, s1_is_el0);
>      } else {
> -        ns = extract32(attrs, 3, 1);
> -        xn = extract32(attrs, 12, 1);
> -        pxn = extract32(attrs, 11, 1);
> +        ns = extract32(attrs, 5, 1);
> +        xn = extract64(attrs, 54, 1);
> +        pxn = extract64(attrs, 53, 1);

Compare here where bit 11 in the old code is 53 in the new.

>          result->f.prot = get_S1prot(env, mmu_idx, aarch64, ap, ns, xn, pxn);
>      }
>

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

thanks
-- PMM