[Qemu-devel] [PATCH][SPARC] Fix the shift instructions for the SPARC target

[Qemu-devel] [PATCH][SPARC] Fix the shift instructions for the SPARC target

[Aurelien Jarno]

Hi all,

The shift instructions on the SPARC target currently take into account
the whole register as the shift count. According to the SPARC v8 and v9
manuals, only the lower 5 bits should be taken into account for 32-bit 
instructions (SLL, SRL, SRA), and only the lower 6 bits for 64-bit 
instructions (SLLX, SRLX, SRAX).

The patch below fixes that. Note that SLL and SLLX are now different, as 
they don't take into account the same number of bits. Please apply.

Thanks,
Aurelien


Index: target-sparc/op.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/op.c,v
retrieving revision 1.26
diff -u -d -p -r1.26 op.c
--- target-sparc/op.c	23 Mar 2007 20:01:20 -0000	1.26
+++ target-sparc/op.c	25 Mar 2007 13:50:45 -0000
@@ -965,38 +965,43 @@ void OPPROTO op_logic_T0_cc(void)
 
 void OPPROTO op_sll(void)
 {
-    T0 <<= T1;
+    T0 <<= (T1 & 0x1f);
 }
 
 #ifdef TARGET_SPARC64
+void OPPROTO op_sllx(void)
+{
+    T0 <<= (T1 & 0x2f);
+}
+
 void OPPROTO op_srl(void)
 {
-    T0 = (T0 & 0xffffffff) >> T1;
+    T0 = (T0 & 0xffffffff) >> (T1 & 0x1f);
 }
 
 void OPPROTO op_srlx(void)
 {
-    T0 >>= T1;
+    T0 >>= (T1 & 0x2f);
 }
 
 void OPPROTO op_sra(void)
 {
-    T0 = ((int32_t) (T0 & 0xffffffff)) >> T1;
+    T0 = ((int32_t) (T0 & 0xffffffff)) >> (T1 & 0x1f);
 }
 
 void OPPROTO op_srax(void)
 {
-    T0 = ((int64_t) T0) >> T1;
+    T0 = ((int64_t) T0) >> (T1 & 0x2f);
 }
 #else
 void OPPROTO op_srl(void)
 {
-    T0 >>= T1;
+    T0 >>= (T1 & 0x1f);
 }
 
 void OPPROTO op_sra(void)
 {
-    T0 = ((int32_t) T0) >> T1;
+    T0 = ((int32_t) T0) >> (T1 & 0x1f);
 }
 #endif
 
Index: target-sparc/translate.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/translate.c,v
retrieving revision 1.38
diff -u -d -p -r1.38 translate.c
--- target-sparc/translate.c	25 Mar 2007 07:55:52 -0000	1.38
+++ target-sparc/translate.c	25 Mar 2007 13:50:45 -0000
@@ -1693,7 +1693,7 @@ static void disas_sparc_insn(DisasContex
 		}
 #endif
 #ifdef TARGET_SPARC64
-	    } else if (xop == 0x25) { /* sll, V9 sllx ( == sll) */
+	    } else if (xop == 0x25) { /* sll, V9 sllx */
                 rs1 = GET_FIELD(insn, 13, 17);
 		gen_movl_reg_T0(rs1);
 		if (IS_IMM) {	/* immediate */
@@ -1703,7 +1703,10 @@ static void disas_sparc_insn(DisasContex
                     rs2 = GET_FIELD(insn, 27, 31);
                     gen_movl_reg_T1(rs2);
                 }
-		gen_op_sll();
+		if (insn & (1 << 12))
+		    gen_op_sllx();
+		else
+		    gen_op_sll();
 		gen_movl_T0_reg(rd);
 	    } else if (xop == 0x26) { /* srl, V9 srlx */
                 rs1 = GET_FIELD(insn, 13, 17);
 

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

RE: [Qemu-devel] [PATCH][SPARC] Fix the shift instructions for theSPARC target

[Blue Swirl]

>The shift instructions on the SPARC target currently take into account
>the whole register as the shift count. According to the SPARC v8 and v9
>manuals, only the lower 5 bits should be taken into account for 32-bit
>instructions (SLL, SRL, SRA), and only the lower 6 bits for 64-bit
>instructions (SLLX, SRLX, SRAX).
>
>The patch below fixes that. Note that SLL and SLLX are now different, as
>they don't take into account the same number of bits. Please apply.

Can you check what happens in real hardware, especially in the case when the 
shift amount is in a register, not immediate, and the value is either >32 or 
==32?

The 64-bit mask should be 0x3f, not 0x2f.

_________________________________________________________________
Don't just search. Find. Check out the new MSN Search! 
http://search.msn.com/

Re: [Qemu-devel] [PATCH][SPARC] Fix the shift instructions for theSPARC target

[Aurelien Jarno]

On Sun, Apr 01, 2007 at 11:32:06AM +0200, Blue Swirl wrote:
> >The shift instructions on the SPARC target currently take into account
> >the whole register as the shift count. According to the SPARC v8 and v9
> >manuals, only the lower 5 bits should be taken into account for 32-bit
> >instructions (SLL, SRL, SRA), and only the lower 6 bits for 64-bit
> >instructions (SLLX, SRLX, SRAX).
> >
> >The patch below fixes that. Note that SLL and SLLX are now different, as
> >they don't take into account the same number of bits. Please apply.
> 
> Can you check what happens in real hardware, especially in the case when 
> the shift amount is in a register, not immediate, and the value is either 
> >32 or ==32?

I have just made the test on real hardware. Specifying a value of 32 leave
the register unchanged. Specifying a value > 32 only takes the lower 5
bits of the value. So exactly as specified in the manual.

> The 64-bit mask should be 0x3f, not 0x2f.

Oops you are right, please find an updated patch below.


Index: target-sparc/op.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/op.c,v
retrieving revision 1.26
diff -u -d -p -r1.26 op.c
--- target-sparc/op.c	23 Mar 2007 20:01:20 -0000	1.26
+++ target-sparc/op.c	25 Mar 2007 13:50:45 -0000
@@ -965,38 +965,43 @@ void OPPROTO op_logic_T0_cc(void)
 
 void OPPROTO op_sll(void)
 {
-    T0 <<= T1;
+    T0 <<= (T1 & 0x1f);
 }
 
 #ifdef TARGET_SPARC64
+void OPPROTO op_sllx(void)
+{
+    T0 <<= (T1 & 0x3f);
+}
+
 void OPPROTO op_srl(void)
 {
-    T0 = (T0 & 0xffffffff) >> T1;
+    T0 = (T0 & 0xffffffff) >> (T1 & 0x1f);
 }
 
 void OPPROTO op_srlx(void)
 {
-    T0 >>= T1;
+    T0 >>= (T1 & 0x3f);
 }
 
 void OPPROTO op_sra(void)
 {
-    T0 = ((int32_t) (T0 & 0xffffffff)) >> T1;
+    T0 = ((int32_t) (T0 & 0xffffffff)) >> (T1 & 0x1f);
 }
 
 void OPPROTO op_srax(void)
 {
-    T0 = ((int64_t) T0) >> T1;
+    T0 = ((int64_t) T0) >> (T1 & 0x3f);
 }
 #else
 void OPPROTO op_srl(void)
 {
-    T0 >>= T1;
+    T0 >>= (T1 & 0x1f);
 }
 
 void OPPROTO op_sra(void)
 {
-    T0 = ((int32_t) T0) >> T1;
+    T0 = ((int32_t) T0) >> (T1 & 0x1f);
 }
 #endif
 
Index: target-sparc/translate.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/translate.c,v
retrieving revision 1.38
diff -u -d -p -r1.38 translate.c
--- target-sparc/translate.c	25 Mar 2007 07:55:52 -0000	1.38
+++ target-sparc/translate.c	25 Mar 2007 13:50:45 -0000
@@ -1693,7 +1693,7 @@ static void disas_sparc_insn(DisasContex
 		}
 #endif
 #ifdef TARGET_SPARC64
-	    } else if (xop == 0x25) { /* sll, V9 sllx ( == sll) */
+	    } else if (xop == 0x25) { /* sll, V9 sllx */
                 rs1 = GET_FIELD(insn, 13, 17);
 		gen_movl_reg_T0(rs1);
 		if (IS_IMM) {	/* immediate */
@@ -1703,7 +1703,10 @@ static void disas_sparc_insn(DisasContex
                     rs2 = GET_FIELD(insn, 27, 31);
                     gen_movl_reg_T1(rs2);
                 }
-		gen_op_sll();
+		if (insn & (1 << 12))
+		    gen_op_sllx();
+		else
+		    gen_op_sll();
 		gen_movl_T0_reg(rd);
 	    } else if (xop == 0x26) { /* srl, V9 srlx */
                 rs1 = GET_FIELD(insn, 13, 17);
 


-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net