Re: [Qemu-devel] [PATCH] target-arm: Fix handling of STM (user) with r15 in register list

[Greg Bellows]

On Tue, Mar 10, 2015 at 12:18 PM, Peter Maydell
<address@hidden> wrote:
> The A32 encoding of LDM distinguishes LDM (user) from LDM (exception
> return) based on whether r15 is in the register list. However for
> STM (user) there is no equivalent distinction. We were incorrectly
> treating "r15 in list" as indicating exception return for both LDM
> and STM, with the result that an STM (user) involving r15 went into
> an infinite loop. Fix this; note that the value stored for r15
> in this case is the current PC regardless of our current mode.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target-arm/translate.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 36868ed..ed8fcea 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -8859,17 +8859,23 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>          case 0x08:
>          case 0x09:
>              {
> -                int j, n, user, loaded_base;
> +                int j, n, loaded_base;
> +                bool exc_return = false;
> +                bool is_load = extract32(insn, 20, 1);
> +                bool user = false;
>                  TCGv_i32 loaded_var;
>                  /* load/store multiple words */
>                  /* XXX: store correct base if write back */
> -                user = 0;
>                  if (insn & (1 << 22)) {
> +                    /* LDM (user), LDM (exception return) and STM (user) */
>                      if (IS_USER(s))
>                          goto illegal_op; /* only usable in supervisor mode */
>
> -                    if ((insn & (1 << 15)) == 0)
> -                        user = 1;
> +                    if (is_load && extract32(insn, 15, 1)) {
> +                        exc_return = true;
> +                    } else {
> +                        user = true;
> +                    }
>                  }
>                  rn = (insn >> 16) & 0xf;
>                  addr = load_reg(s, rn);
> @@ -8903,7 +8909,7 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>                  j = 0;
>                  for(i=0;i<16;i++) {
>                      if (insn & (1 << i)) {
> -                        if (insn & (1 << 20)) {
> +                        if (is_load) {
>                              /* load */
>                              tmp = tcg_temp_new_i32();
>                              gen_aa32_ld32u(tmp, addr, get_mem_index(s));
> @@ -8968,7 +8974,7 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>                  if (loaded_base) {
>                      store_reg(s, rn, loaded_var);
>                  }
> -                if ((insn & (1 << 22)) && !user) {
> +                if (exc_return) {
>                      /* Restore CPSR from SPSR.  */
>                      tmp = load_cpu_field(spsr);
>                      gen_set_cpsr(tmp, CPSR_ERET_MASK);
> --
> 1.9.1
>
>

Reviewed-by: Greg Bellows <address@hidden>