Re: [Qemu-devel] [PATCH v3 2/9] target-arm: Add computation of starting level for S2 PTW

[Edgar E. Iglesias]

On Wed, Oct 07, 2015 at 01:24:27PM +0100, Alex Bennée wrote:
> 
> Edgar E. Iglesias <address@hidden> writes:
> 
> > 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 | 39 +++++++++++++++++++++++++++------------
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 5a5e5f0..507324f 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -6549,18 +6549,33 @@ 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 (granule_sz 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 'va_size - tsz', 'grainsize' is
> > -     * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
> > -     * Applying the usual "rounded up m/n is (m+n-1)/n" and simplifying:
> > -     *     = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / 
> > granule_sz
> > -     *     = 4 - (va_size - tsz - 4) / granule_sz;
> > -     */
> > -    level = 4 - (va_size - tsz - 4) / granule_sz;
> > +    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 (granule_sz 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 'va_size - tsz', 'grainsize' is
> > +         * our 'granule_sz + 3' and 'stride' is our 'granule_sz'.
> > +         * Applying the usual "rounded up m/n is (m+n-1)/n" and 
> > simplifying:
> > +         * = 4 - (va_size - tsz - granule_sz - 3 + granule_sz - 1) / 
> > granule_sz
> > +         * = 4 - (va_size - tsz - 4) / granule_sz;
> > +         */
> > +        level = 4 - (va_size - tsz - 4) / granule_sz;
> > +    } else {
> > +        unsigned int startlevel = extract32(tcr->raw_tcr, 6, 2);
> 
> Maybe an assert(startlevel<3) would be useful?

Good catch here. There is actually a trap for some of the bad startlevel cases.
I'll implement that for the next version.


> 
> > +
> > +        /* For stage 2 translations the starting level is specified by the
> > +         * VCTR_EL2.SL0 field (whose interpretation depends on the page 
> > size)
> > +         */
> > +        if (granule_sz == 9) {
> > +            /* 4K pages */
> > +            level = 2 - startlevel;
> > +        } else {
> > +            /* 16K or 64K pages */
> > +            level = 3 - 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
> 
> Reviewed-by: Alex Bennée <address@hidden>

Thanks, I'll hold the RB until you see the next version with the traps for bad
start-levels.

Cheers,
Edgar