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 >