Re: [Qemu-devel] [PATCH for-2.9 01/30] target-arm: Add VBAR support to ARM1176 CPUs

[Peter Maydell]

On 29 November 2016 at 15:43, Cédric Le Goater <address@hidden> wrote:
> ARM1176 CPUs support the Vector Base Address Register but currently,
> qemu only supports VBAR on ARMv7 CPUs. Fix this by adding a new
> feature ARM_FEATURE_VBAR which can used for ARMv7 and ARM1176 CPUs.
>
> The VBAR feature is always set for ARMv7 because some legacy boards
> require it even if this is not architecturally correct. However, to
> support arm1176 CPUs without TrustZone, which doesn't exist in real
> hardware but which is used in old qemu boards, we need to disable the
> feature when 'has_el3' is not set.
>
> Signed-off-by: Cédric Le Goater <address@hidden>
> ---
>  target-arm/cpu.c    |  6 ++++++
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 24 ++++++++++++++++++------
>  3 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 99f0dbebb9f6..1007e504248a 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -685,6 +685,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>           */
>          cpu->id_pfr1 &= ~0xf0;
>          cpu->id_aa64pfr0 &= ~0xf000;
> +
> +        /* Also disable VBAR support for boards using a arm1176 CPU
> +         * without EL3.
> +         */
> +        unset_feature(env, ARM_FEATURE_VBAR);
>      }
>
>      if (!cpu->has_pmu || !kvm_enabled()) {
> @@ -911,6 +916,7 @@ static void arm1176_initfn(Object *obj)
>
>      cpu->dtb_compatible = "arm,arm1176";
>      set_feature(&cpu->env, ARM_FEATURE_V6K);
> +    set_feature(&cpu->env, ARM_FEATURE_VBAR);
>      set_feature(&cpu->env, ARM_FEATURE_VFP);
>      set_feature(&cpu->env, ARM_FEATURE_VAPA);
>      set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index ca5c849ed65e..ab119e62ab0f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1125,6 +1125,7 @@ enum arm_features {
>      ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
>      ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
>      ARM_FEATURE_PMU, /* has PMU support */
> +    ARM_FEATURE_VBAR, /* has cp15 VBAR */
>  };
>
>  static inline int arm_feature(CPUARMState *env, int feature)
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b5b65caadf8a..d417c8ba802f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1252,12 +1252,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .writefn = pmintenclr_write },
> -    { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
> -      .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW, .writefn = vbar_write,
> -      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vbar_s),
> -                             offsetof(CPUARMState, cp15.vbar_ns) },
> -      .resetvalue = 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_RAW },
> @@ -1412,6 +1406,16 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
>      REGINFO_SENTINEL
>  };
>
> +static const ARMCPRegInfo vbar_cp_reginfo[] = {
> +    { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
> +      .access = PL1_RW, .writefn = vbar_write,
> +      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.vbar_s),
> +                             offsetof(CPUARMState, cp15.vbar_ns) },
> +      .resetvalue = 0 },
> +    REGINFO_SENTINEL
> +};
> +
>  #ifndef CONFIG_USER_ONLY
>
>  static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo 
> *ri,
> @@ -4506,6 +4510,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>      if (arm_feature(env, ARM_FEATURE_V6K)) {
>          define_arm_cp_regs(cpu, v6k_cp_reginfo);
>      }
> +    if (arm_feature(env, ARM_FEATURE_VBAR)) {
> +        define_arm_cp_regs(cpu, vbar_cp_reginfo);
> +    }
>      if (arm_feature(env, ARM_FEATURE_V7MP) &&
>          !arm_feature(env, ARM_FEATURE_MPU)) {
>          define_arm_cp_regs(cpu, v7mp_cp_reginfo);
> @@ -4543,6 +4550,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          };
>          define_one_arm_cp_reg(cpu, &clidr);
>          define_arm_cp_regs(cpu, v7_cp_reginfo);
> +
> +        /* Always define VBAR even if it doesn't exist in non-EL3
> +         * configs. This is needed by some legacy boards.
> +         */
> +        define_arm_cp_regs(cpu, vbar_cp_reginfo);
>          define_debug_regs(cpu);
>      } else {
>          define_arm_cp_regs(cpu, not_v7_cp_reginfo);
> --
> 2.7.4

This all seems overcomplicated to me. The aim is to define VBAR for:
 * CPUs with TrustZone/EL3
 * ARMv7 CPUs (even if no EL3, for legacy reasons)
right?

We do that by changing arm_cpu_realizefn():
 * add set_feature(arm, ARM_FEATURE_VBAR) to the things we enable
   when the ARM_FEATURE_V7 bit is set (with a comment about why)
 * add
    if (arm_feature(env, ARM_FEATURE_EL3)) {
       set_feature(env, ARM_FEATURE_VBAR));
    }
   in the appropriate place
and then register_cp_regs_for_features() just does
   if (arm_feature(env, ARM_FEATURE_VBAR)) {
       define_arm_cp_regs(cpu, vbar_cp_reginfo);
   }

You don't need to explicitly set the VBAR feature in the 1176
initfn, or unset set feature bits, or register vbar_cp_reginfo
in two separate places.

thanks
-- PMM