Re: [Qemu-arm] [PATCH] target-arm: Add VBAR support to ARM1176 CPUs

[Peter Maydell]

On 23 August 2016 at 10:59, 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 is used for ARMv7 and ARM1176 CPUs.
>
> Signed-off-by: Cédric Le Goater <address@hidden>

This looks mostly good; one question below.

> Index: qemu-aspeed.git/target-arm/cpu.h
> ===================================================================
> --- qemu-aspeed.git.orig/target-arm/cpu.h
> +++ qemu-aspeed.git/target-arm/cpu.h
> @@ -1129,6 +1129,7 @@ enum arm_features {
>      ARM_FEATURE_V8_SHA256, /* implements SHA256 part of v8 Crypto Extensions 
> */
>      ARM_FEATURE_V8_PMULL, /* implements PMULL part of v8 Crypto Extensions */
>      ARM_FEATURE_THUMB_DSP, /* DSP insns supported in the Thumb encodings */
> +    ARM_FEATURE_VBAR, /* has cp15 VBAR (ARM1176) */

you don't need the bit in brackets, I think (it's not complete
anyway, since v7+ also has this).

>  };
>
>  static inline int arm_feature(CPUARMState *env, int feature)
> Index: qemu-aspeed.git/target-arm/cpu.c
> ===================================================================
> --- qemu-aspeed.git.orig/target-arm/cpu.c
> +++ qemu-aspeed.git/target-arm/cpu.c
> @@ -584,6 +584,7 @@ static void arm_cpu_realizefn(DeviceStat
>          set_feature(env, ARM_FEATURE_LPAE);
>      }
>      if (arm_feature(env, ARM_FEATURE_V7)) {
> +        set_feature(env, ARM_FEATURE_VBAR);
>          set_feature(env, ARM_FEATURE_VAPA);
>          set_feature(env, ARM_FEATURE_THUMB2);
>          set_feature(env, ARM_FEATURE_MPIDR);
> @@ -867,6 +868,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);

Is it sufficient to set ARM_FEATURE_VBAR in the realizefn
if FEATURE_V7 or FEATURE_EL3? (watch out that realizefn
may change the value of the FEATURE_EL3 bit partway down
so you'd need to do the check there) ?

We implement VBAR in v7-without-EL3 even though architecturally
it should only exist in v7-with-EL3 because we have some
legacy board models which we implement as without-EL3 but
where the guest (Linux) assumes it's running on a with-EL3
CPU in NS mode. I'd rather we stick to the architectural
definition for 1176 if we can rather than expanding the
"things we do which aren't architectural" set if there's
no legacy config that requires it. (If it is necessary
to define it for 1176 always then that's OK, I'd just
prefer not to unless we know we have to.)

We should probably also have a brief comment noting that
we define VBAR always in v7, even though architecturally
it doesn't exist in non-EL3 configs, for the benefit of
legacy board models.

(In v8 VBAR is required whether EL3 is implemented or not.)

thanks
-- PMM