Re: [Qemu-arm] [PATCH 1/4] target-arm: Clean up trap/undef handling of SRS

[Sergey Fedorov]

On 11.02.2016 22:11, Peter Maydell wrote:
> The SRS instruction is:
>  * UNDEFINED in Hyp mode
>  * UNPREDICTABLE in User or System mode
>  * UNPREDICTABLE if the specified mode isn't accessible
>  * trapped to EL3 if EL3 is AArch64 and we are at Secure EL1
>
> Clean up the code to handle all these cases cleanly, including
> picking UNDEF as our choice of UNPREDICTABLE behaviour rather
> blindly trusting the mode field passed in the instruction.
> As part of this, move the check for IS_USER into gen_srs()
> itself rather than having it done by the caller.
>
> The exception is that we don't UNDEF for calls from System
> mode, which need a runtime check. This will be dealt with in
> the following commits.
>
> Signed-off-by: Peter Maydell <address@hidden>

Reviewed-by: Sergey Fedorov <address@hidden>

> ---
>  target-arm/translate.c | 66 
> ++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 61 insertions(+), 5 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index cf3dc33..7bceb05 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7578,8 +7578,67 @@ static void gen_srs(DisasContext *s,
>                      uint32_t mode, uint32_t amode, bool writeback)
>  {
>      int32_t offset;
> -    TCGv_i32 addr = tcg_temp_new_i32();
> -    TCGv_i32 tmp = tcg_const_i32(mode);
> +    TCGv_i32 addr, tmp;
> +    bool undef = false;
> +
> +    /* SRS is:
> +     * - trapped to EL3 if EL3 is AArch64 and we are at Secure EL1
> +     * - UNDEFINED in Hyp mode
> +     * - UNPREDICTABLE in User or System mode
> +     * - UNPREDICTABLE if the specified mode is:
> +     * -- not implemented
> +     * -- not a valid mode number
> +     * -- a mode that's at a higher exception level
> +     * -- Monitor, if we are Non-secure
> +     * For the UNPREDICTABLE cases we choose to UNDEF, except that for
> +     * "current mode is System" we will write a garbage SPSR.
> +     * (This is because we don't have access to our current mode here
> +     * and would have to do a runtime check to UNDEF for System.)
> +     */
> +    if (s->current_el == 1 && !s->ns) {
> +        gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(), 3);
> +        return;
> +    }
> +
> +    if (s->current_el == 0 || s->current_el == 2) {
> +        undef = true;
> +    }
> +
> +    switch (mode) {
> +    case ARM_CPU_MODE_USR:
> +    case ARM_CPU_MODE_FIQ:
> +    case ARM_CPU_MODE_IRQ:
> +    case ARM_CPU_MODE_SVC:
> +    case ARM_CPU_MODE_ABT:
> +    case ARM_CPU_MODE_UND:
> +    case ARM_CPU_MODE_SYS:
> +        break;
> +    case ARM_CPU_MODE_HYP:
> +        if (s->current_el == 1 || !arm_dc_feature(s, ARM_FEATURE_EL2)) {
> +            undef = true;
> +        }
> +        break;
> +    case ARM_CPU_MODE_MON:
> +        /* No need to check specifically for "are we non-secure" because
> +         * we've already made EL0 UNDEF and handled the trap for S-EL1;
> +         * so if this isn't EL3 then we must be non-secure.
> +         */
> +        if (s->current_el != 3) {
> +            undef = true;
> +        }
> +        break;
> +    default:
> +        undef = true;
> +    }
> +
> +    if (undef) {
> +        gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
> +                           default_exception_el(s));
> +        return;
> +    }
> +
> +    addr = tcg_temp_new_i32();
> +    tmp = tcg_const_i32(mode);
>      gen_helper_get_r13_banked(addr, cpu_env, tmp);
>      tcg_temp_free_i32(tmp);
>      switch (amode) {
> @@ -7739,9 +7798,6 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>              }
>          } else if ((insn & 0x0e5fffe0) == 0x084d0500) {
>              /* srs */
> -            if (IS_USER(s)) {
> -                goto illegal_op;
> -            }
>              ARCH(6);
>              gen_srs(s, (insn & 0x1f), (insn >> 23) & 3, insn & (1 << 21));
>              return;