On 31 October 2014 10:26, Peter Maydell <peter.mayd...@linaro.org> wrote:

> On 30 October 2014 21:28, Greg Bellows <greg.bell...@linaro.org> wrote:
> > From: Fabian Aggeler <aggel...@ethz.ch>
> >
> > Since TTBCR is banked we will bank c2_mask and c2_base_mask too. This
> > avoids recalculating them on switches from secure to non-secure world.
>
> These fields are part of our TTBCR internal representation; we
> should bank the whole TTBCR in one patch, not split over two.
>

Squashed the TTBCR and C2 mask patches in v9.


>
> > Signed-off-by: Fabian Aggeler <aggel...@ethz.ch>
> > Signed-off-by: Greg Bellows <greg.bell...@linaro.org>
> >
> > ---
> >
> > v5 -> v6
> > - Switch to use distinct CPREG secure flags
> >
> > v4 -> v5
> > - Changed c2_mask updates to use the TTBCR cpreg bank flag for selcting
> the
> >   secure bank instead of the A32_BANKED_CURRENT macro.  This more
> accurately
> >   chooses the correct bank matching that of the TTBCR being accessed.
> > ---
> >  target-arm/cpu.h    | 10 ++++++++--
> >  target-arm/helper.c | 24 ++++++++++++++++++------
> >  2 files changed, 26 insertions(+), 8 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index f125bdd..6e9f1c3 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -226,8 +226,14 @@ typedef struct CPUARMState {
> >              };
> >              uint64_t tcr_el[4];
> >          };
> > -        uint32_t c2_mask; /* MMU translation table base selection
> mask.  */
> > -        uint32_t c2_base_mask; /* MMU translation table base 0 mask. */
> > +        struct { /* MMU translation table base selection mask. */
> > +            uint32_t c2_mask_ns;
> > +            uint32_t c2_mask_s;
> > +        };
> > +        struct { /* MMU translation table base 0 mask. */
> > +            uint32_t c2_base_mask_ns;
> > +            uint32_t c2_base_mask_s;
> > +        };
>
> I think we should actually have:
>     typedef struct {
>         uint64_t raw_ttbcr;
>         uint32_t mask;
>         uint32_t base_mask;
>     } TTBCR;
>
>     and then have TTBCR ttbcr[2];
>
> and not use the BANKED_REG_SET/GET macros here...
>

I like the cleanliness of this approach but it does not take into
consideration the tcr_el[] fields.  I have instead changed the naming to
TCR/tcr_el and have added 4 entries instead of 2.  This is more consistent
with the other changes in the patch set.


>
> >          uint32_t c2_data; /* MPU data cachable bits.  */
> >          uint32_t c2_insn; /* MPU instruction cachable bits.  */
> >          uint32_t c3; /* MMU domain access control register
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 896b40d..27eaf9c 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -1584,8 +1584,14 @@ static void vmsa_ttbcr_raw_write(CPUARMState
> *env, const ARMCPRegInfo *ri,
> >       * and the c2_mask and c2_base_mask values are meaningless.
> >       */
> >      raw_write(env, ri, value);
> > -    env->cp15.c2_mask = ~(((uint32_t)0xffffffffu) >> maskshift);
> > -    env->cp15.c2_base_mask = ~((uint32_t)0x3fffu >> maskshift);
> > +
> > +    /* Update the masks corresponding to the the TTBCR bank being
> written */
> > +    A32_BANKED_REG_SET(env, c2_mask,
> > +                       ARM_CP_SECSTATE_TEST(ri, ARM_CP_SECSTATE_S),
> > +                       ~(((uint32_t)0xffffffffu) >> maskshift));
> > +    A32_BANKED_REG_SET(env, c2_base_mask,
> > +                       ARM_CP_SECSTATE_TEST(ri, ARM_CP_SECSTATE_S),
> > +                       ~((uint32_t)0x3fffu >> maskshift));
>
> ... so this turns into:
>
>     TTBCR t = env->cp15.ttbcr[ri->secure];
>
>     t->raw_ttbcr = value;
>     t->mask = ~(((uint32_t)0xffffffffu) >> maskshift);
>     t->base_mask = ~((uint32_t)0x3fffu >> maskshift);
>
> (XXX did we make ri->secure be a 0/1 or is it 1/2 ? anyway you get the
> idea.)
>
>
Changed in v9.  ri->secure is 1/2 as both bitsor neither may be set at
definition time.


> >  }
> >
> >  static void vmsa_ttbcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > @@ -1604,9 +1610,15 @@ static void vmsa_ttbcr_write(CPUARMState *env,
> const ARMCPRegInfo *ri,
> >
> >  static void vmsa_ttbcr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> >  {
> > -    env->cp15.c2_base_mask = 0xffffc000u;
> > +    /* Rest both the TTBCR as well as the masks corresponding to the
> bank of
> > +     * the TTBCR being reset.
> > +     */
> > +    A32_BANKED_REG_SET(env, c2_base_mask,
> > +                       ARM_CP_SECSTATE_TEST(ri, ARM_CP_SECSTATE_S),
> > +                       0xffffc000u);
> > +    A32_BANKED_REG_SET(env, c2_mask,
> > +                       ARM_CP_SECSTATE_TEST(ri, ARM_CP_SECSTATE_S), 0);
> >      raw_write(env, ri, 0);
> > -    env->cp15.c2_mask = 0;
>
> Similarly this will be much cleaner.
>

Changed in v9.  Question on reset.  We call raw_write() which also sets the
masks, but we set the masks separately here too, but different values.  It
seems we should only need to set them in raw_write() is this true?


>
> >  }
> >
> >  static void vmsa_tcr_el1_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > @@ -4440,7 +4452,7 @@ static bool get_level1_table_address(CPUARMState
> *env, uint32_t *table,
> >       * AArch32 there is a secure and non-secure instance of the
> translation
> >       * table registers.
> >       */
> > -    if (address & env->cp15.c2_mask) {
> > +    if (address & A32_BANKED_CURRENT_REG_GET(env, c2_mask)) {
> >          if (A32_BANKED_CURRENT_REG_GET(env, ttbcr) & TTBCR_PD1) {
> >              /* Translation table walk disabled for TTBR1 */
> >              return false;
> > @@ -4452,7 +4464,7 @@ static bool get_level1_table_address(CPUARMState
> *env, uint32_t *table,
> >              return false;
> >          }
> >          *table = A32_BANKED_CURRENT_REG_GET(env, ttbr0) &
> > -                 env->cp15.c2_base_mask;
> > +                 A32_BANKED_CURRENT_REG_GET(env, c2_base_mask);
> >      }
>
> and again here you can get a pointer to the correct TTBCR
> struct and just reference it.
>


Yes.  Changed all accesses in v9.


>
> >      *table |= (address >> 18) & 0x3ffc;
> >      return true;
> > --
>
> thanks
> -- PMM
>

Reply via email to