[Qemu-devel] [PATCH] fix ARMv7 MOV R15, xxx operation

[Qemu-devel] [PATCH] fix ARMv7 MOV R15, xxx operation

[Riihimaki Juha]

The behavior of several ARM mode commands where the destination  
register is R15 has changed in ARMv7 to mimic the behavior of the BX  
instruction. While this calls for a little bit wider fix in the code,  
this patch fixes the issue for the special case of MOV instruction.

Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
---

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 3cef021..7d9a934 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6201,17 +6212,24 @@ static void disas_arm_insn(CPUState * env,  
DisasContext *s)
                  gen_op_logic_T0_cc();
              break;
          case 0x0d:
-            if (logic_cc && rd == 15) {
-                /* MOVS r15, ... is used for exception return.  */
-                if (IS_USER(s))
-                    goto illegal_op;
-                gen_op_movl_T0_T1();
-                gen_exception_return(s);
-            } else {
-                gen_movl_reg_T1(s, rd);
-                if (logic_cc)
-                    gen_op_logic_T1_cc();
+            if (rd == 15) {
+                if (logic_cc) {
+                    /* MOVS r15, ... is used for exception return.  */
+                    if (IS_USER(s))
+                        goto illegal_op;
+                    gen_op_movl_T0_T1();
+                    gen_exception_return(s);
+                    break;
+                } else if (ENABLE_ARCH_7) {
+                    tmp = new_tmp();
+                    tcg_gen_mov_i32(tmp, cpu_T[1]);
+                    gen_bx(s, tmp);
+                    break;
+                }
              }
+            gen_movl_reg_T1(s, rd);
+            if (logic_cc)
+                gen_op_logic_T1_cc();
              break;
          case 0x0e:
              gen_op_bicl_T0_T1();

Re: [Qemu-devel] [PATCH] fix ARMv7 MOV R15, xxx operation

[Paul Brook]

On Friday 13 March 2009, Riihimaki Juha wrote:
> The behavior of several ARM mode commands where the destination
> register is R15 has changed in ARMv7 to mimic the behavior of the BX
> instruction. While this calls for a little bit wider fix in the code,
> this patch fixes the issue for the special case of MOV instruction.

No. If you want to fix this, do it properly.

Paul

Re: [Qemu-devel] [PATCH] fix ARMv7 MOV R15, xxx operation

[Riihimaki Juha]

On Mar 13, 2009, at 13:52, ext Paul Brook wrote:

> On Friday 13 March 2009, Riihimaki Juha wrote:
>> The behavior of several ARM mode commands where the destination
>> register is R15 has changed in ARMv7 to mimic the behavior of the BX
>> instruction. While this calls for a little bit wider fix in the code,
>> this patch fixes the issue for the special case of MOV instruction.
>
> No. If you want to fix this, do it properly.
>

Would you like to give an opinion on how it should be implemented? I  
suppose changing the store_reg and gen_movl_reg_TN functions in target- 
arm/translate.c to include an extra check when destination is R15 like  
"if in ARM state and arch >= 7 then bx else..." might do the trick  
more generally but do you think it introduces the change in a too wide  
scope?

Juha

Re: [Qemu-devel] [PATCH] fix ARMv7 MOV R15, xxx operation

[Laurent Desnogues]

On Fri, Mar 13, 2009 at 3:18 PM, Riihimaki Juha
<Juha.Riihimaki@nokia.com> wrote:
> I suppose
> changing the store_reg and gen_movl_reg_TN functions in
> target-arm/translate.c to include an extra check when destination is R15
> like "if in ARM state and arch >= 7 then bx else..." might do the trick more
> generally but do you think it introduces the change in a too wide scope?

I'm afraid that'd be too wide as it would effect ld/st exclusive,
gen_exception_return and gen_lookup_tb (plus iwmmxt code).


Laurent

Re: [Qemu-devel] [PATCH] fix ARMv7 MOV R15, xxx operation

[Riihimaki Juha]

On Mar 13, 2009, at 17:13, ext Laurent Desnogues wrote:

> On Fri, Mar 13, 2009 at 3:18 PM, Riihimaki Juha
> <Juha.Riihimaki@nokia.com> wrote:
>> I suppose
>> changing the store_reg and gen_movl_reg_TN functions in
>> target-arm/translate.c to include an extra check when destination  
>> is R15
>> like "if in ARM state and arch >= 7 then bx else..." might do the  
>> trick more
>> generally but do you think it introduces the change in a too wide  
>> scope?
>
> I'm afraid that'd be too wide as it would effect ld/st exclusive,
> gen_exception_return and gen_lookup_tb (plus iwmmxt code).
>

Thanks, that is what I thought as well. It would also have slowed down  
thumb processing due to the extra check introduced in all stores to  
r15 even though the changed functionality only affects arm state. I  
guess then that there is no generic place where this could be easily  
fixed but instead the handling of all affected commands would need to  
be changed similarly to the patch that I sent for the mov command?  
Seems a little bit clumsy approach to duplicate the same piece of code  
in several places imho so if you have any better suggestions...

On a sidenote, I also noticed that the arm emulation currently allows  
jumping to unaligned memory addresses in arm mode since it only clears  
the least significant bit when storing to r15. However, since armv6  
the two least significant bits should be automatically ignored. This  
hasn't caused me any problems (yet) since I guess programs rarely rely  
on this feature.


Juha

Re: [Qemu-devel] [PATCH] fix ARMv7 MOV R15, xxx operation

[Paul Brook]

> Seems a little bit clumsy approach to duplicate the same piece of code
> in several places imho so if you have any better suggestions...

That's the whole point of functions.

Paul

Re: [Qemu-devel] [PATCH] fix ARMv7 MOV R15, xxx operation

[Riihimaki Juha]

On Mar 13, 2009, at 20:29, ext Paul Brook wrote:

>> Seems a little bit clumsy approach to duplicate the same piece of  
>> code
>> in several places imho so if you have any better suggestions...
>
> That's the whole point of functions.
>

Sorry, I was just hoping to avoid duplicating code in several places,  
be that 5 lines of code, a function call, a macro or whatever. But I  
guess no such luck.


Juha

Re: [Qemu-devel] [PATCH] fix ARMv7 MOV R15, xxx operation

[Paul Brook]

On Friday 13 March 2009, Riihimaki Juha wrote:
> On Mar 13, 2009, at 20:29, ext Paul Brook wrote:
> >> Seems a little bit clumsy approach to duplicate the same piece of
> >> code
> >> in several places imho so if you have any better suggestions...
> >
> > That's the whole point of functions.
>
> Sorry, I was just hoping to avoid duplicating code in several places,
> be that 5 lines of code, a function call, a macro or whatever. But I
> guess no such luck.

We already have a function used for setting the value of a register[1] 
(store_reg). You just need to use a slightly different instruction when that 
write is interworked.

Paul

[1] There are a few other obsolete variants like gen_movl_reg_TN, but they 
need to go away anyway.