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;

Not sure what I was thinking... I've rewritten all of it to match the
specs (I hope).

> 
> (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.)


Yes, I've changed the naming of to better match specs.

Thanks!
Edgar


> 
> > +            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