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

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

[Peter Maydell]

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 <peter.maydell@linaro.org>
---
 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

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
<peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
> ---
>  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 <greg.bellows@linaro.org>

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

[Peter Maydell]

On 17 March 2015 at 17:24, Greg Bellows <greg.bellows@linaro.org> wrote:
> On Tue, Mar 10, 2015 at 12:18 PM, Peter Maydell
> <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>

> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>

Thanks for the review, though this got committed to master yesterday :-)

-- PMM

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

[Greg Bellows]

On Tue, Mar 17, 2015 at 10:26 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 17 March 2015 at 17:24, Greg Bellows <greg.bellows@linaro.org> wrote:
>> On Tue, Mar 10, 2015 at 12:18 PM, Peter Maydell
>> <peter.maydell@linaro.org> 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 <peter.maydell@linaro.org>
>
>> Reviewed-by: Greg Bellows <greg.bellows@linaro.org>
>
> Thanks for the review, though this got committed to master yesterday :-)
>
> -- PMM

Oh missed that and I checked the archive and did not see any review-by
and figured you wanted to get it in...

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

[Peter Maydell]

On 17 March 2015 at 17:28, Greg Bellows <greg.bellows@linaro.org> wrote:
> Oh missed that and I checked the archive and did not see any review-by
> and figured you wanted to get it in...

No problem; even belated review is worthwhile, because if you'd
spotted a problem we could have fixed it.

-- PMM