[Qemu-devel] [PATCH][ARM] Fix wrong destination register for smuad, smusd, smlad, smlsd

[Qemu-devel] [PATCH][ARM] Fix wrong destination register for smuad, smusd, smlad, smlsd

[Laurent Desnogues]

[-- Attachment #1: Type: text/plain, Size: 91 bytes --]

smuad, smusd, smlad, smlsd write the wrong register, resulting in
PC corruption.


Laurent

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: arm-smuad.patch --]
[-- Type: text/x-patch; name=arm-smuad.patch, Size: 494 bytes --]

--- svn-ref/target-arm/translate.c	2008-06-09 08:52:48.000000000 +0200
+++ svn/target-arm/translate.c	2008-06-23 11:25:40.000000000 +0200
@@ -6508,7 +6508,7 @@
                                 gen_helper_add_setq(tmp, tmp, tmp2);
                                 dead_tmp(tmp2);
                               }
-                            store_reg(s, rd, tmp);
+                            store_reg(s, rn, tmp);
                         }
                     }
                     break;

[Qemu-devel] Re: [PATCH][ARM] Fix wrong destination register for smuad, smusd, smlad, smlsd

[Laurent Desnogues]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

2008/6/23 Laurent Desnogues <laurent.desnogues@gmail.com>:
> smuad, smusd, smlad, smlsd write the wrong register, resulting in
> PC corruption.

Here is a better patch.

I will stop posting patches as I have the feeling the maintainer
doesn't care.  He might be rewriting everything and so my
patches are useless and a waste of time for everyone :-)

Laurent

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: arm-smuad-2.patch --]
[-- Type: text/x-patch; name=arm-smuad-2.patch, Size: 887 bytes --]

--- svn-ref/target-arm/translate.c	2008-06-09 08:52:48.000000000 +0200
+++ svn/target-arm/translate.c	2008-06-25 17:20:54.000000000 +0200
@@ -6502,13 +6502,13 @@
                             gen_storeq_reg(s, rn, rd, tmp2);
                         } else {
                             /* smuad, smusd, smlad, smlsd */
-                            if (rn != 15)
+                            if (rd != 15)
                               {
-                                tmp2 = load_reg(s, rn);
+                                tmp2 = load_reg(s, rd);
                                 gen_helper_add_setq(tmp, tmp, tmp2);
                                 dead_tmp(tmp2);
                               }
-                            store_reg(s, rd, tmp);
+                            store_reg(s, rn, tmp);
                         }
                     }
                     break;

Re: [Qemu-devel] Re: [PATCH][ARM] Fix wrong destination register for smuad, smusd, smlad, smlsd

[Paul Brook]

On Wednesday 25 June 2008, Laurent Desnogues wrote:
> 2008/6/23 Laurent Desnogues <laurent.desnogues@gmail.com>:
> > smuad, smusd, smlad, smlsd write the wrong register, resulting in
> > PC corruption.
>
> Here is a better patch.
>
> I will stop posting patches as I have the feeling the maintainer
> doesn't care.  He might be rewriting everything and so my
> patches are useless and a waste of time for everyone :-)

TBH It's hard to tell if these are well tested patches, or just quick hacks 
that you're throwing over the wall.

As above where it took you two tries. This isn't bad per se, but compete lack 
of explanation about what's different doesn't help. Some of your other 
patches have been prefixed with "I did not check the correctness of that 
instruction in general, I only made a change that looked logical" and "These 
are all *wild guesses*". As we know from the recent Debian SSL debacle 
making "a change that looked logical" can be fairly disastrous :-)

This means I'm unwilling to accept the patches a face value, and need to go 
through them with a fine tooth comb. This takes time, and you go in the queue 
with the dozens of other of patches, bugs and new features that need my 
attention.

Paul

Re: [Qemu-devel] Re: [PATCH][ARM] Fix wrong destination register for smuad, smusd, smlad, smlsd

[Laurent Desnogues]

On Wed, Jun 25, 2008 at 6:58 PM, Paul Brook <paul@codesourcery.com> wrote:
> On Wednesday 25 June 2008, Laurent Desnogues wrote:
>> 2008/6/23 Laurent Desnogues <laurent.desnogues@gmail.com>:
>> > smuad, smusd, smlad, smlsd write the wrong register, resulting in
>> > PC corruption.
>>
>> Here is a better patch.
>>
>> I will stop posting patches as I have the feeling the maintainer
>> doesn't care.  He might be rewriting everything and so my
>> patches are useless and a waste of time for everyone :-)
>
> TBH It's hard to tell if these are well tested patches, or just quick hacks
> that you're throwing over the wall.
>
> As above where it took you two tries. This isn't bad per se, but compete lack
> of explanation about what's different doesn't help. Some of your other
> patches have been prefixed with "I did not check the correctness of that
> instruction in general, I only made a change that looked logical" and "These
> are all *wild guesses*". As we know from the recent Debian SSL debacle
> making "a change that looked logical" can be fairly disastrous :-)

I am certainly over cautious as everyone should be when he doesn't
master all of a software :-)  So indeed when I propose some "wild
guess" it's all related to parts of qemu I am not sure to understand
and generally doesn't come with a patch. On the other hand when I
post a fix for the behaviour of an instruction I am sure it's OK except
when it contains multiple bugs (as was the case for that instruction).
I am now doing explicit tests to check each instruction does what
it should, so this mistake should not happen anymore.

> This means I'm unwilling to accept the patches a face value, and need to go
> through them with a fine tooth comb. This takes time, and you go in the queue
> with the dozens of other of patches, bugs and new features that need my
> attention.

That's enough of a feedback to me, I am not expecting you to
commit anything within a minute or even within days.  The lack of
any feedback from you made me wonder if you were not within a
complete rewrite of the ARM target that would have made my
patches useless and just some noise on the list :)

Anyway, are you interested in a patch that corrects enough of
v6 instructions that FFmpeg for ARMv6 works?  Would FFmpeg
be considered as a good enough test?


Laurent

Re: [Qemu-devel] Re: [PATCH][ARM] Fix wrong destination register for smuad, smusd, smlad, smlsd

[Paul Brook]

> Anyway, are you interested in a patch that corrects enough of
> v6 instructions that FFmpeg for ARMv6 works?  Would FFmpeg
> be considered as a good enough test?

I'm generally more interested in the description/explanation of the patches 
themselves.  Saying "FOO works" is nice, but not as nice as convincing me 
that you understand both the code you're changing, the arm architecture, and 
that the two agree :-)

Paul

Re: [Qemu-devel] Re: [PATCH][ARM] Fix wrong destination register for smuad, smusd, smlad, smlsd

[Laurent Desnogues]

On Wed, Jun 25, 2008 at 7:27 PM, Paul Brook <paul@codesourcery.com> wrote:
>> Anyway, are you interested in a patch that corrects enough of
>> v6 instructions that FFmpeg for ARMv6 works?  Would FFmpeg
>> be considered as a good enough test?
>
> I'm generally more interested in the description/explanation of the patches
> themselves.  Saying "FOO works" is nice, but not as nice as convincing me
> that you understand both the code you're changing, the arm architecture, and
> that the two agree :-)

That looks fair :-)  I will submit a patch tomorrow.  Is that
acceptable to submit as a single patch with some text
describing all the updates or should I split it instruction
by instruction?


Laurent

Re: [Qemu-devel] Re: [PATCH][ARM] Fix wrong destination register for smuad, smusd, smlad, smlsd

[Vincent Palatin]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

Paul wrote:
> I'm generally more interested in the description/explanation of the patches
> themselves.  Saying "FOO works" is nice, but not as nice as convincing me
> that you understand both the code you're changing, the arm architecture,
> and that the two agree :-)

That's fairly right, but I'm not sure that the problem lays here.
Indeed, as Laurent mentioned in his mail on monday, my 2 previous patchs with 
fixes for ARM instructions were not integrated too while I hope I've put a 
fairly descriptive text with them ( I re-attach them if you have any comment)

--  
Vincent

---------- Forwarded message  ----------
Suject : [PATCH] ARM: fix carry flags for ARMv6 unsigned SIMD operations

[PATCH] ARM: fix carry flags for ARMv6 unsigned SIMD operations
On ARMv6 emulation, I have caught some cases where the GE flags were
badly set after a "uadd8" operation.
After a quick code review, it seems to be a bad cut-n-paste between
16-bit and 8-bit UADD/USUB, indeed UADD8/USUB8 tries to set GE bits by
pair instead of one at a time.
Besides, the addition operations (UADD8/UADD16) set GE bits to "NOT
carry" instead of "carry" (probably once again due to a copy of the 
substraction code which sets flags to "NOT borrow")

I attach a patch to fix those issues. (arith_ge.patch)


---------- Forwarded message  ----------
Subject: [PATCH] ARM: fix CPS instruction

I attach a patch with 2 fixes for the ARMv6 instruction "CPS".
According to ARM Reference Manual (DDI0100 A4.1.16), 
bit 5 is fixed to 0 (bit 4 is the MSB of the mode), so the instruction mask 
should be  0x0ff10020 not 0x0ff10010.
Besides, mmod flag is bit 17 (b14 is SBZ)


[-- Attachment #2: arith_ge.patch --]
[-- Type: text/x-diff, Size: 935 bytes --]

Index: target-arm/helper.c
===================================================================
--- target-arm/helper.c	(revision 4740)
+++ target-arm/helper.c	(working copy)
@@ -2059,7 +2059,7 @@
     uint32_t sum; \
     sum = (uint32_t)(uint16_t)(a) + (uint32_t)(uint16_t)(b); \
     RESULT(sum, n, 16); \
-    if ((sum >> 16) == 0) \
+    if ((sum >> 16) == 1) \
         ge |= 3 << (n * 2); \
     } while(0)
 
@@ -2067,8 +2067,8 @@
     uint32_t sum; \
     sum = (uint32_t)(uint8_t)(a) + (uint32_t)(uint8_t)(b); \
     RESULT(sum, n, 8); \
-    if ((sum >> 8) == 0) \
-        ge |= 3 << (n * 2); \
+    if ((sum >> 8) == 1) \
+        ge |= 1 << n; \
     } while(0)
 
 #define SUB16(a, b, n) do { \
@@ -2084,7 +2084,7 @@
     sum = (uint32_t)(uint8_t)(a) - (uint32_t)(uint8_t)(b); \
     RESULT(sum, n, 8); \
     if ((sum >> 8) == 0) \
-        ge |= 3 << (n * 2); \
+        ge |= 1 << n; \
     } while(0)
 
 #define PFX u


[-- Attachment #3: arm_cps.patch --]
[-- Type: text/x-diff, Size: 858 bytes --]

Index: target-arm/translate.c
===================================================================
--- target-arm/translate.c	(revision 4740)
+++ target-arm/translate.c	(working copy)
@@ -5801,7 +5801,7 @@
             /* Coprocessor double register transfer.  */
         } else if ((insn & 0x0f000010) == 0x0e000010) {
             /* Additional coprocessor register transfer.  */
-        } else if ((insn & 0x0ff10010) == 0x01000000) {
+        } else if ((insn & 0x0ff10020) == 0x01000000) {
             uint32_t mask;
             uint32_t val;
             /* cps (privileged) */
@@ -5818,7 +5818,7 @@
                 if (insn & (1 << 18))
                     val |= mask;
             }
-            if (insn & (1 << 14)) {
+            if (insn & (1 << 17)) {
                 mask |= CPSR_M;
                 val |= (insn & 0x1f);
             }