Re: [Qemu-devel] [PATCH v5 09/37] target-arm: Fix VFP enables for AArch32 EL0 under AArch64 EL1

[Peter Crosthwaite]

On Sat, Mar 29, 2014 at 2:09 AM, Peter Maydell <address@hidden> wrote:
> The current A32/T32 decoder bases its "is VFP/Neon enabled?" check
> on the FPSCR.EN bit. This is correct if EL1 is AArch32, but for
> an AArch64 EL1 the logic is different: it must act as if FPSCR.EN
> is always set. Instead, trapping must happen according to CPACR
> bits for cp10/cp11; these cover all of FP/Neon, including the
> FPSCR/FPSID/MVFR register accesses which FPSCR.EN does not affect.
> Add support for CPACR checks (which are also required for ARMv7,
> but were unimplemented because Linux happens not to use them)
> and make sure they generate exceptions with the correct syndrome.
>
> We actually return incorrect syndrome information for cases
> where FP is disabled but the specific instruction bit pattern
> is unallocated: strictly these should be the Uncategorized
> exception, not a "SIMD disabled" exception. This should be
> mostly harmless, and the structure of the A32/T32 VFP/Neon
> decoder makes it painful to put the 'FP disabled?' checks in
> the right places.

Worth a well placed /* FIXME */ ?

>
> Signed-off-by: Peter Maydell <address@hidden>

Otherwise,

Reviewed-by: Peter Crosthwaite <address@hidden>

> ---
>  target-arm/cpu.h       | 10 +++++++++-
>  target-arm/translate.c | 19 +++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 72c4c7a..ff56519 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1104,6 +1104,8 @@ static inline int cpu_mmu_index (CPUARMState *env)
>  #define ARM_TBFLAG_CONDEXEC_MASK    (0xff << ARM_TBFLAG_CONDEXEC_SHIFT)
>  #define ARM_TBFLAG_BSWAP_CODE_SHIFT 16
>  #define ARM_TBFLAG_BSWAP_CODE_MASK  (1 << ARM_TBFLAG_BSWAP_CODE_SHIFT)
> +#define ARM_TBFLAG_CPACR_FPEN_SHIFT 17
> +#define ARM_TBFLAG_CPACR_FPEN_MASK  (1 << ARM_TBFLAG_CPACR_FPEN_SHIFT)
>
>  /* Bit usage when in AArch64 state */
>  #define ARM_TBFLAG_AA64_EL_SHIFT    0
> @@ -1128,6 +1130,8 @@ static inline int cpu_mmu_index (CPUARMState *env)
>      (((F) & ARM_TBFLAG_CONDEXEC_MASK) >> ARM_TBFLAG_CONDEXEC_SHIFT)
>  #define ARM_TBFLAG_BSWAP_CODE(F) \
>      (((F) & ARM_TBFLAG_BSWAP_CODE_MASK) >> ARM_TBFLAG_BSWAP_CODE_SHIFT)
> +#define ARM_TBFLAG_CPACR_FPEN(F) \
> +    (((F) & ARM_TBFLAG_CPACR_FPEN_MASK) >> ARM_TBFLAG_CPACR_FPEN_SHIFT)
>  #define ARM_TBFLAG_AA64_EL(F) \
>      (((F) & ARM_TBFLAG_AA64_EL_MASK) >> ARM_TBFLAG_AA64_EL_SHIFT)
>  #define ARM_TBFLAG_AA64_FPEN(F) \
> @@ -1161,9 +1165,13 @@ static inline void cpu_get_tb_cpu_state(CPUARMState 
> *env, target_ulong *pc,
>          if (privmode) {
>              *flags |= ARM_TBFLAG_PRIV_MASK;
>          }
> -        if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30)) {
> +        if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30)
> +            || arm_el_is_aa64(env, 1)) {
>              *flags |= ARM_TBFLAG_VFPEN_MASK;
>          }
> +        if (fpen == 3 || (fpen == 1 && arm_current_pl(env) != 0)) {
> +            *flags |= ARM_TBFLAG_CPACR_FPEN_MASK;
> +        }
>      }
>
>      *cs_base = 0;
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 84700ca..5d0c73f 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -2952,6 +2952,12 @@ static int disas_vfp_insn(CPUARMState * env, 
> DisasContext *s, uint32_t insn)
>      if (!arm_feature(env, ARM_FEATURE_VFP))
>          return 1;
>
> +    if (!s->cpacr_fpen) {
> +        gen_exception_insn(s, 4, EXCP_UDEF,
> +                           syn_fp_access_trap(1, 0xe, s->thumb));
> +        return 0;
> +    }
> +
>      if (!s->vfp_enabled) {
>          /* VFP disabled.  Only allow fmxr/fmrx to/from some control regs.  */
>          if ((insn & 0x0fe00fff) != 0x0ee00a10)
> @@ -4232,6 +4238,12 @@ static int disas_neon_ls_insn(CPUARMState * env, 
> DisasContext *s, uint32_t insn)
>      TCGv_i32 tmp2;
>      TCGv_i64 tmp64;
>
> +    if (!s->cpacr_fpen) {
> +        gen_exception_insn(s, 4, EXCP_UDEF,
> +                           syn_fp_access_trap(1, 0xe, s->thumb));
> +        return 0;
> +    }
> +
>      if (!s->vfp_enabled)
>        return 1;
>      VFP_DREG_D(rd, insn);
> @@ -4954,6 +4966,12 @@ static int disas_neon_data_insn(CPUARMState * env, 
> DisasContext *s, uint32_t ins
>      TCGv_i32 tmp, tmp2, tmp3, tmp4, tmp5;
>      TCGv_i64 tmp64;
>
> +    if (!s->cpacr_fpen) {
> +        gen_exception_insn(s, 4, EXCP_UDEF,
> +                           syn_fp_access_trap(1, 0xe, s->thumb));
> +        return 0;
> +    }
> +
>      if (!s->vfp_enabled)
>        return 1;
>      q = (insn & (1 << 6)) != 0;
> @@ -10736,6 +10754,7 @@ static inline void 
> gen_intermediate_code_internal(ARMCPU *cpu,
>  #if !defined(CONFIG_USER_ONLY)
>      dc->user = (ARM_TBFLAG_PRIV(tb->flags) == 0);
>  #endif
> +    dc->cpacr_fpen = ARM_TBFLAG_CPACR_FPEN(tb->flags);
>      dc->vfp_enabled = ARM_TBFLAG_VFPEN(tb->flags);
>      dc->vec_len = ARM_TBFLAG_VECLEN(tb->flags);
>      dc->vec_stride = ARM_TBFLAG_VECSTRIDE(tb->flags);
> --
> 1.9.0
>
>