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.
> 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...
> 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.)
> }
>
> 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.
> }
>
> 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.
> *table |= (address >> 18) & 0x3ffc;
> return true;
> --
thanks
-- PMM
