Re: [Qemu-devel] [PATCH v4 06/13] target-arm: Add computation of starting level for S2 PTW

[Peter Maydell]

On 14 October 2015 at 23:55, Edgar E. Iglesias <address@hidden> wrote:
> From: "Edgar E. Iglesias" <address@hidden>
>
> The starting level for S2 pagetable walks is computed
> differently from the S1 starting level. Implement the S2
> variant.
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>  target-arm/helper.c    | 117 
> +++++++++++++++++++++++++++++++++++++++++++------
>  target-arm/internals.h |  25 +++++++++++
>  2 files changed, 129 insertions(+), 13 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 79b4c03..8530f7e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6406,12 +6406,72 @@ typedef enum {
>      permission_fault = 3,
>  } MMUFaultType;
>
> +/*
> + * check_s2_startlevel
> + * @cpu:        ARMCPU
> + * @is_aa64:    True if the translation regime is in AArch64 state
> + * @startlevel: Suggested starting level
> + * @inputsize:  Bitsize of IPAs
> + * @stride:     Page-table stride (See the ARM ARM)
> + *
> + * Returns true if the suggested starting level is OK and false otherwise.
> + */
> +static bool check_s2_startlevel(ARMCPU *cpu, bool is_aa64, int startlevel,
> +                                int inputsize, int stride)
> +{
> +    /* Negative levels are never allowed.  */
> +    if (startlevel < 0) {
> +        return false;
> +    }
> +
> +    if (is_aa64) {
> +        unsigned int pamax = arm_pamax(cpu);
> +
> +        switch (stride) {
> +        case 13: /* 64KB Pages.  */
> +            if (startlevel < 1 || (startlevel == 0 && pamax <= 42)) {
> +                return false;
> +            }

I'm having trouble matching these up with the ARM ARM pseudocode,
which says for this case for instance
   if (level == 0 || (level == 1 && PAMax() <= 42)) then basefound = FALSE;

(as an aside, the pseudocode uses 'startlevel' for the raw SL0
field value and 'level' for the 3 - startlevel (or 2 - startlevel)
value, so it's a bit confusing to use startlevel for both here.)

> +            break;
> +        case 11: /* 16KB Pages.  */
> +            if (startlevel < 1 || (startlevel == 0 && pamax <= 40)) {
> +                return false;
> +            }
> +            break;
> +        case 9: /* 4KB Pages.  */
> +            if (startlevel == 0 && pamax <= 42) {
> +                return false;
> +            }
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }
> +    } else {
> +        const int grainsize = stride + 3;
> +        int startsizecheck;
> +
> +        /* AArch32 only supports 4KB pages. Assert on that.  */
> +        assert(stride == 9);
> +
> +        if (startlevel == 0) {
> +            return false;
> +        }
> +
> +        startsizecheck = inputsize - ((3 - startlevel) * stride + grainsize);
> +        if (startsizecheck < 1 || startsizecheck > stride + 4) {
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +
>  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>                                 int access_type, ARMMMUIdx mmu_idx,
>                                 hwaddr *phys_ptr, MemTxAttrs *txattrs, int 
> *prot,
>                                 target_ulong *page_size_ptr, uint32_t *fsr)
>  {
> -    CPUState *cs = CPU(arm_env_get_cpu(env));
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    CPUState *cs = CPU(cpu);
>      /* Read an LPAE long-descriptor translation table. */
>      MMUFaultType fault_type = translation_fault;
>      uint32_t level = 1;
> @@ -6560,18 +6620,49 @@ static bool 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. It indicates the 
> number
> -     * of strides (stride 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 'inputsize', 'grainsize' is
> -     * our 'stride + 3' and 'stride' is our 'stride'.
> -     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> -     *     = 4 - (inputsize - stride - 3 + stride - 1) / stride
> -     *     = 4 - (inputsize - 4) / stride;
> -     */
> -    level = 4 - (inputsize - 4) / stride;
> +    if (mmu_idx != ARMMMUIdx_S2NS) {
> +        /* 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 (stride 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 'inputsize', 'grainsize' is
> +         * our 'stride + 3' and 'stride' is our 'stride'.
> +         * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> +         * = 4 - (inputsize - stride - 3 + stride - 1) / stride
> +         * = 4 - (inputsize - 4) / stride;
> +         */
> +        level = 4 - (inputsize - 4) / stride;
> +    } else {
> +        /* For stage 2 translations the starting level is specified by the
> +         * VCTR_EL2.SL0 field (whose interpretation depends on the page size)

VTCR_EL2.

> +         */
> +        int startlevel = extract32(tcr->raw_tcr, 6, 2);
> +        bool ok;
> +
> +        if (va_size == 32 || stride == 9) {
> +            /* AArch32 or 4KB pages */
> +            startlevel = 2 - startlevel;
> +        } else {
> +            /* 16KB or 64KB pages */
> +            startlevel = 3 - startlevel;
> +        }
> +
> +        /* Check that the starting level is valid. */
> +        ok = check_s2_startlevel(cpu, va_size == 64, startlevel,
> +                                 inputsize, stride);
> +        if (!ok) {
> +            /* AArch64 reports these as level 0 faults.
> +             * AArch32 reports these as level 1 faults.
> +             */
> +            level = va_size == 64 ? 0 : 1;
> +            fault_type = translation_fault;
> +            goto do_fault;
> +        }
> +
> +        /* The starting level looks good, use it.  */
> +        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
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 36a56aa..8bd37eb 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -152,6 +152,31 @@ static inline void update_spsel(CPUARMState *env, 
> uint32_t imm)
>      aarch64_restore_sp(env, cur_el);
>  }
>
> +/*
> + * arm_pamax
> + * @cpu: ARMCPU
> + *
> + * Returns the implementation defined bit-width of physical addresses.
> + * The ARMv8 reference manuals refer to this as PAMax().
> + */
> +static inline unsigned int arm_pamax(ARMCPU *cpu)
> +{
> +    static const unsigned int pamax_map[] = {
> +        [0] = 32,
> +        [1] = 36,
> +        [2] = 40,
> +        [3] = 42,
> +        [4] = 44,
> +        [5] = 48,
> +    };
> +    unsigned int parange = extract32(cpu->id_aa64mmfr0, 0, 4);
> +
> +    /* id_aa64mmfr0 is a read-only register so values outside of the
> +     * supported mappings can be considered an implementation error.  */
> +    assert(parange < ARRAY_SIZE(pamax_map));
> +    return pamax_map[parange];
> +}
> +
>  /* Return true if extended addresses are enabled.
>   * This is always the case if our translation regime is 64 bit,
>   * but depends on TTBCR.EAE for 32 bit.
> --
> 1.9.1
>

looks ok otherwise

thanks
-- PMM