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

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

On Fri, Oct 23, 2015 at 05:26:52PM +0100, Peter Maydell wrote:
> On 14 October 2015 at 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > 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 <edgar.iglesias@xilinx.com>
> > ---
> >  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.

Fixed

Thanks!
Edgar