Re: [Qemu-devel] [PATCH 07/24] target-arm: A64: Make cache ID registers visible to AArch64

[Peter Crosthwaite]

On Wed, Jan 22, 2014 at 6:12 AM, Peter Maydell <address@hidden> wrote:
> Make the cache ID system registers (CLIDR, CCSELR, CCSIDR, CTR)
> visible to AArch64. These are mostly simple 64-bit extensions of the
> existing 32 bit system registers and so can share reginfo definitions.
> CTR needs to have a split definition, but we can clean up the
> temporary user-mode implementation in favour of using the CPU-specified
> reset value, and implement the system-mode-required semantics of
> restricting its EL0 accessibility if SCTLR.UCT is not set.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target-arm/cpu.c    |  2 ++
>  target-arm/cpu.h    |  2 +-
>  target-arm/cpu64.c  |  1 +
>  target-arm/helper.c | 33 +++++++++++++++++++++++----------
>  4 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index c1e55c8..1e08802 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -91,6 +91,8 @@ static void arm_cpu_reset(CPUState *s)
>          env->aarch64 = 1;
>  #if defined(CONFIG_USER_ONLY)
>          env->pstate = PSTATE_MODE_EL0t;
> +        /* Userspace expects access to CTL_EL0 */
> +        env->cp15.c1_sys |= SCTLR_UCT;
>  #else
>          env->pstate = PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F
>              | PSTATE_MODE_EL1h;
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1aa4495..1966a19 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -166,7 +166,7 @@ typedef struct CPUARMState {
>      /* System control coprocessor (cp15) */
>      struct {
>          uint32_t c0_cpuid;
> -        uint32_t c0_cssel; /* Cache size selection.  */
> +        uint64_t c0_cssel; /* Cache size selection.  */
>          uint32_t c1_sys; /* System control register.  */
>          uint32_t c1_coproc; /* Coprocessor access register.  */
>          uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index a639c2e..8426bf1 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -45,6 +45,7 @@ static void aarch64_any_initfn(Object *obj)
>      set_feature(&cpu->env, ARM_FEATURE_ARM_DIV);
>      set_feature(&cpu->env, ARM_FEATURE_V7MP);
>      set_feature(&cpu->env, ARM_FEATURE_AARCH64);
> +    cpu->ctr = 0x80030003; /* 32 byte I and D cacheline size, VIPT icache */
>  }
>  #endif
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 205e36a..204d7c3 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -676,9 +676,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>      { .name = "SCR", .cp = 15, .crn = 1, .crm = 1, .opc1 = 0, .opc2 = 0,
>        .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_scr),
>        .resetvalue = 0, },
> -    { .name = "CCSIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
> +    { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 0,
>        .access = PL1_R, .readfn = ccsidr_read, .type = ARM_CP_NO_MIGRATE },
> -    { .name = "CSSELR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
> +    { .name = "CSSELR", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 2, .opc2 = 0,
>        .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c0_cssel),
>        .writefn = csselr_write, .resetvalue = 0 },
>      /* Auxiliary ID register: this actually has an IMPDEF value but for now
> @@ -1601,13 +1603,6 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>      { .name = "FPSR", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 4, .crm = 4,
>        .access = PL0_RW, .readfn = aa64_fpsr_read, .writefn = aa64_fpsr_write 
> },
> -    /* This claims a 32 byte cacheline size for icache and dcache, VIPT 
> icache.
> -     * It will eventually need to have a CPU-specified reset value.
> -     */
> -    { .name = "CTR_EL0", .state = ARM_CP_STATE_AA64,
> -      .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 0, .crm = 0,
> -      .access = PL0_R, .type = ARM_CP_CONST,
> -      .resetvalue = 0x80030003 },
>      /* Prohibit use of DC ZVA. OPTME: implement DC ZVA and allow its use.
>       * For system mode the DZP bit here will need to be computed, not 
> constant.
>       */
> @@ -1627,6 +1622,20 @@ static int sctlr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>      return 0;
>  }
>
> +static int ctr_el0_read(CPUARMState *env, const ARMCPRegInfo *ri,
> +                        uint64_t *value)
> +{
> +    /* This register is constant, but only accessible in AArch64 EL0 if
> +     * SCTLR.UCT is set.
> +     */
> +    ARMCPU *cpu = arm_env_get_cpu(env);
> +    if (arm_current_pl(env) == 0 && !(env->cp15.c1_sys & SCTLR_UCT)) {
> +        return EXCP_UDEF;
> +    }

There seem to be multiple instances in this series where you fallback
to open coded R/W accessor functions for the sake of access checks. Is
it better to define a bool check_access() fn hook in ARMCPRegInfo and
leave the actual write/read behaviour to the data driven mechanisms?
This may also minimise the need for raw_write hook usages as it serves
to isolate the actual state change into its own self contained
definition (whether open coded or not).

> +    *value = cpu->ctr;
> +    return 0;
> +}
> +
>  void register_cp_regs_for_features(ARMCPU *cpu)
>  {
>      /* Register all the coprocessor registers based on feature bits */
> @@ -1711,7 +1720,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              .raw_readfn = raw_read, .raw_writefn = raw_write,
>          };
>          ARMCPRegInfo clidr = {
> -            .name = "CLIDR", .cp = 15, .crn = 0, .crm = 0, .opc1 = 1, .opc2 
> = 1,
> +            .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
> +            .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1,
>              .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->clidr
>          };
>          define_one_arm_cp_reg(cpu, &pmcr);
> @@ -1790,6 +1800,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              { .name = "CTR",
>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 1,
>                .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = cpu->ctr 
> },
> +            { .name = "CTR_EL0", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 3, .opc2 = 1, .crn = 0, .crm = 0,
> +              .access = PL0_R, .readfn = ctr_el0_read },

Then you can make this consisent with AA32 which simply hooks up the
.resetvalue to the desired magic number.

Regards,
Peter

>              { .name = "TCMTR",
>                .cp = 15, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 2,
>                .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
> --
> 1.8.5
>
>