Re: [Qemu-devel] [PATCH v3 31/31] target-arm: Add v8 mmu translation support

[Hu Tao]

On Wed, Feb 26, 2014 at 10:49:59AM +0800, Hu Tao wrote:
> On Sat, Feb 15, 2014 at 04:07:24PM +0000, Peter Maydell wrote:
> > From: Rob Herring <address@hidden>
> > 
> > Add support for v8 page table walks. This supports stage 1 translations
> > for 4KB, 16KB and 64KB page sizes starting with 0 or 1 level.
> > 
> > Signed-off-by: Rob Herring <address@hidden>
> > [PMM: fix style nits, fold in 16/64K page support patch, use
> >  arm_el_is_aa64() to decide whether to do 64 bit page table walk]
> > Signed-off-by: Peter Maydell <address@hidden>
> > ---
> >  target-arm/helper.c | 85 
> > +++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 56 insertions(+), 29 deletions(-)
> > 
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 2d66165..740bf42 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -7,7 +7,7 @@
> >  #include "qemu/bitops.h"
> >  
> >  #ifndef CONFIG_USER_ONLY
> > -static inline int get_phys_addr(CPUARMState *env, uint32_t address,
> > +static inline int get_phys_addr(CPUARMState *env, target_ulong address,
> >                                  int access_type, int is_user,
> >                                  hwaddr *phys_ptr, int *prot,
> >                                  target_ulong *page_size);
> > @@ -1062,8 +1062,9 @@ static void par_write(CPUARMState *env, const 
> > ARMCPRegInfo *ri, uint64_t value)
> >   */
> >  static inline bool extended_addresses_enabled(CPUARMState *env)
> >  {
> > -    return arm_feature(env, ARM_FEATURE_LPAE)
> > -        && (env->cp15.c2_control & (1U << 31));
> > +    return arm_feature(env, ARM_FEATURE_V8)
> > +        || (arm_feature(env, ARM_FEATURE_LPAE)
> > +        && (env->cp15.c2_control & (1U << 31)));
> >  }
> >  
> >  static CPAccessResult ats_access(CPUARMState *env, const ARMCPRegInfo *ri)
> > @@ -3291,7 +3292,7 @@ typedef enum {
> >      permission_fault = 3,
> >  } MMUFaultType;
> >  
> > -static int get_phys_addr_lpae(CPUARMState *env, uint32_t address,
> > +static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
> >                                int access_type, int is_user,
> >                                hwaddr *phys_ptr, int *prot,
> >                                target_ulong *page_size_ptr)
> > @@ -3300,26 +3301,28 @@ static int get_phys_addr_lpae(CPUARMState *env, 
> > uint32_t address,
> >      MMUFaultType fault_type = translation_fault;
> >      uint32_t level = 1;
> >      uint32_t epd;
> > -    uint32_t tsz;
> > +    int32_t tsz;
> > +    uint32_t tg;
> >      uint64_t ttbr;
> >      int ttbr_select;
> > -    int n;
> > -    hwaddr descaddr;
> > +    hwaddr descaddr, descmask;
> >      uint32_t tableattrs;
> >      target_ulong page_size;
> >      uint32_t attrs;
> > +    int32_t granule_sz = 9;
> > +    int32_t va_size = arm_el_is_aa64(env, 1) ? 64 : 32;
> >  
> >      /* Determine whether this address is in the region controlled by
> >       * TTBR0 or TTBR1 (or if it is in neither region and should fault).
> >       * This is a Non-secure PL0/1 stage 1 translation, so controlled by
> >       * TTBCR/TTBR0/TTBR1 in accordance with ARM ARM DDI0406C table B-32:
> >       */
> > -    uint32_t t0sz = extract32(env->cp15.c2_control, 0, 3);
> > -    uint32_t t1sz = extract32(env->cp15.c2_control, 16, 3);
> > -    if (t0sz && !extract32(address, 32 - t0sz, t0sz)) {
> > +    uint32_t t0sz = extract32(env->cp15.c2_control, 0, 5);
> > +    uint32_t t1sz = extract32(env->cp15.c2_control, 16, 5);
> 
> t0sz is bit [5:0], so shouldn't we extract 6 bits here? same for t1sz.
> 
> > +    if (t0sz && !extract64(address, va_size - t0sz, t0sz)) {
> >          /* there is a ttbr0 region and we are in it (high bits all zero) */
> >          ttbr_select = 0;
> > -    } else if (t1sz && !extract32(~address, 32 - t1sz, t1sz)) {
> > +    } else if (t1sz && !extract64(~address, va_size - t1sz, t1sz)) {
> >          /* there is a ttbr1 region and we are in it (high bits all one) */
> >          ttbr_select = 1;
> >      } else if (!t0sz) {
> 
> Can't be true for Aarch64. the VA address space has a maximum address width
> of 48 bits(page D5-1712 of ARM DDI 0487A.a), that means t0sz and t1sz should
> have a minimum value of 16.

It doesn't matter here. Maybe we should check the value when writing to
TCR_EL1. What's the behaviour when writing an invalid tsz to TCR_EL1?