| Message ID | 1414704538-17103-19-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 31 October 2014 10:26, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 October 2014 21:28, Greg Bellows <greg.bellows@linaro.org> wrote: > > From: Fabian Aggeler <aggelerf@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 <aggelerf@ethz.ch> > > Signed-off-by: Greg Bellows <greg.bellows@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 >
On 4 November 2014 22:46, Greg Bellows <greg.bellows@linaro.org> wrote: > > > On 31 October 2014 10:26, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On 30 October 2014 21:28, Greg Bellows <greg.bellows@linaro.org> wrote: >> > 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? No, raw_write() won't set the masks -- it just writes 32 or 64 bits to the field pointed to by fieldoffset. Which makes it a pretty obfuscated way of saying env->cp15.c2_control = 0; and I don't know why we do it this way currently. If we go to having a TTBCR struct we should just set all the fields directly here I think. -- PMM
Ah... I was confused and thinking of ttbcr_raw_write. Yes, raw_write does not actually touch the masks. The reason we used raw_write is that it updates the correct ri offset. We could be calling with the secure or non-secure ri so it was necessary to make sure the correct offset gets updated. Staying with this pattern, I introduced a new utility function raw_ptr() that gives back the pointer calculation based on the ri fieldoffset. For TCR, this will be the base of the TCR struct which we can then update. On 4 November 2014 17:27, Peter Maydell <peter.maydell@linaro.org> wrote: > On 4 November 2014 22:46, Greg Bellows <greg.bellows@linaro.org> wrote: > > > > > > On 31 October 2014 10:26, Peter Maydell <peter.maydell@linaro.org> > wrote: > >> > >> On 30 October 2014 21:28, Greg Bellows <greg.bellows@linaro.org> wrote: > >> > 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? > > No, raw_write() won't set the masks -- it just writes 32 or 64 bits > to the field pointed to by fieldoffset. Which makes it a pretty > obfuscated way of saying env->cp15.c2_control = 0; and I don't > know why we do it this way currently. If we go to having a TTBCR > struct we should just set all the fields directly here I think. > > -- PMM >
On 5 November 2014 15:09, Greg Bellows <greg.bellows@linaro.org> wrote: > Ah... I was confused and thinking of ttbcr_raw_write. Yes, raw_write does > not actually touch the masks. > > The reason we used raw_write is that it updates the correct ri offset. We > could be calling with the secure or non-secure ri so it was necessary to > make sure the correct offset gets updated. ...but this doesn't help if you need to also update the mask etc. > Staying with this pattern, I introduced a new utility function raw_ptr() > that gives back the pointer calculation based on the ri fieldoffset. For > TCR, this will be the base of the TCR struct which we can then update. Seems like overkill for one register, compared to just doing if (ri->secure == ...) thanks -- PMM
Maybe, but it is cleaner and works later on down the road when we add EL2 registers that use the same write functions. I'm fine either way if you feel strongly about it. On 5 November 2014 09:15, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 November 2014 15:09, Greg Bellows <greg.bellows@linaro.org> wrote: > > Ah... I was confused and thinking of ttbcr_raw_write. Yes, raw_write > does > > not actually touch the masks. > > > > The reason we used raw_write is that it updates the correct ri offset. > We > > could be calling with the secure or non-secure ri so it was necessary to > > make sure the correct offset gets updated. > > ...but this doesn't help if you need to also update the mask etc. > > > Staying with this pattern, I introduced a new utility function raw_ptr() > > that gives back the pointer calculation based on the ri fieldoffset. For > > TCR, this will be the base of the TCR struct which we can then update. > > Seems like overkill for one register, compared to just doing > if (ri->secure == ...) > > thanks > -- PMM >
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; + }; 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)); } 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; } 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); } *table |= (address >> 18) & 0x3ffc; return true;