diff mbox

[1/2,for-2.6] target-sparc: fix Nucleus quad LDD 128 bit access for windowed registers

Message ID b8d0b7c81437065b81416c00781e42ea09e2053c.1460625412.git.atar4qemu@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Artyom Tarasenko April 14, 2016, 9:29 a.m. UTC
Fix register offset calculation when regwptr is used.

Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
---
 target-sparc/ldst_helper.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Mark Cave-Ayland April 14, 2016, 1:42 p.m. UTC | #1
On 14/04/16 10:29, Artyom Tarasenko wrote:

> Fix register offset calculation when regwptr is used.
> 
> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  target-sparc/ldst_helper.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
> index 2b0221c..a383074 100644
> --- a/target-sparc/ldst_helper.c
> +++ b/target-sparc/ldst_helper.c
> @@ -2059,11 +2059,11 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd)
>                  bswap64s(&env->gregs[rd + 1]);
>              }
>          } else {
> -            env->regwptr[rd] = cpu_ldq_nucleus(env, addr);
> -            env->regwptr[rd + 1] = cpu_ldq_nucleus(env, addr + 8);
> +            env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr);
> +            env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8);
>              if (asi == 0x2c) {
> -                bswap64s(&env->regwptr[rd]);
> -                bswap64s(&env->regwptr[rd + 1]);
> +                bswap64s(&env->regwptr[rd - 8]);
> +                bswap64s(&env->regwptr[rd + 1 - 8]);
>              }
>          }
>          break;
> @@ -2076,8 +2076,8 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd)
>              env->gregs[rd] = helper_ld_asi(env, addr, asi, 4, 0);
>              env->gregs[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0);
>          } else {
> -            env->regwptr[rd] = helper_ld_asi(env, addr, asi, 4, 0);
> -            env->regwptr[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0);
> +            env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0);
> +            env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4, asi, 4, 0);
>          }
>          break;
>      }
> 

So it seems that without this patch any ldda on a non-global register is
placing the result into the wrong register which is fairly nasty.

The patch does apply, but only with fuzz, and checkpatch.pl reports
several warnings with spacing/line endings. If you can rebase to master
and resend then you can add my Reviewed-by and I'm okay with this for
2.6 - register corruption is not good.


ATB,

Mark.
Artyom Tarasenko April 14, 2016, 2:04 p.m. UTC | #2
On Thu, Apr 14, 2016 at 3:42 PM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 14/04/16 10:29, Artyom Tarasenko wrote:
>
>> Fix register offset calculation when regwptr is used.
>>
>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
>> ---
>>  target-sparc/ldst_helper.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
>> index 2b0221c..a383074 100644
>> --- a/target-sparc/ldst_helper.c
>> +++ b/target-sparc/ldst_helper.c
>> @@ -2059,11 +2059,11 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd)
>>                  bswap64s(&env->gregs[rd + 1]);
>>              }
>>          } else {
>> -            env->regwptr[rd] = cpu_ldq_nucleus(env, addr);
>> -            env->regwptr[rd + 1] = cpu_ldq_nucleus(env, addr + 8);
>> +            env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr);
>> +            env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8);
>>              if (asi == 0x2c) {
>> -                bswap64s(&env->regwptr[rd]);
>> -                bswap64s(&env->regwptr[rd + 1]);
>> +                bswap64s(&env->regwptr[rd - 8]);
>> +                bswap64s(&env->regwptr[rd + 1 - 8]);
>>              }
>>          }
>>          break;
>> @@ -2076,8 +2076,8 @@ void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd)
>>              env->gregs[rd] = helper_ld_asi(env, addr, asi, 4, 0);
>>              env->gregs[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0);
>>          } else {
>> -            env->regwptr[rd] = helper_ld_asi(env, addr, asi, 4, 0);
>> -            env->regwptr[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0);
>> +            env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0);
>> +            env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4, asi, 4, 0);
>>          }
>>          break;
>>      }
>>
>
> So it seems that without this patch any ldda on a non-global register is
> placing the result into the wrong register which is fairly nasty.
>
> The patch does apply, but only with fuzz, and checkpatch.pl reports
> several warnings with spacing/line endings. If you can rebase to master
> and resend then you can add my Reviewed-by and I'm okay with this for
> 2.6 - register corruption is not good.

Can you please show  the output of your checkpatch.pl report? I get:

 $  scripts/checkpatch.pl
0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch
total: 0 errors, 0 warnings, 25 lines checked
0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch has no
obvious style problems and is ready for Submission.
Mark Cave-Ayland April 14, 2016, 2:20 p.m. UTC | #3
On 14/04/16 15:04, Artyom Tarasenko wrote:

> Can you please show  the output of your checkpatch.pl report? I get:
> 
>  $  scripts/checkpatch.pl
> 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch
> total: 0 errors, 0 warnings, 25 lines checked
> 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch has no
> obvious style problems and is ready for Submission.

Okay I've just done an update pull and complete rebuild which fixes the
fuzz, but I'm still seeing line ending errors in checkpatch.pl:


$ ./scripts/checkpatch.pl /tmp/artyom/\[PATCH\ 1_2\ for-2.6\]\
target-sparc\:\ fix\ Nucleus\ quad\ LDD\ 128\ bit\ access\ for\
windowed\ registers.eml
ERROR: DOS line endings
#75: FILE: target-sparc/ldst_helper.c:2062:
+            env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr);^M$

ERROR: DOS line endings
#76: FILE: target-sparc/ldst_helper.c:2063:
+            env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8);^M$

ERROR: DOS line endings
#80: FILE: target-sparc/ldst_helper.c:2065:
+                bswap64s(&env->regwptr[rd - 8]);^M$

ERROR: DOS line endings
#81: FILE: target-sparc/ldst_helper.c:2066:
+                bswap64s(&env->regwptr[rd + 1 - 8]);^M$

ERROR: DOS line endings
#91: FILE: target-sparc/ldst_helper.c:2079:
+            env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0);^M$

ERROR: DOS line endings
#92: FILE: target-sparc/ldst_helper.c:2080:
+            env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4,
asi, 4, 0);^M$

total: 6 errors, 0 warnings, 25 lines checked

/tmp/artyom/[PATCH 1_2 for-2.6] target-sparc: fix Nucleus quad LDD 128
bit access for windowed registers.eml has style problems, please review.
 If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

and:

$ ./scripts/checkpatch.pl /tmp/artyom/\[PATCH\ 2_2\ for-2.6\]\
target-sparc\:\ fix\ Trap\ Based\ Address\ Register\ behavior\ for\
sparc64.eml
ERROR: DOS line endings
#85: FILE: target-sparc/int64_helper.c:161:
+    env->pc = env->tbr  & ~0x7fffULL;^M$

ERROR: DOS line endings
#86: FILE: target-sparc/int64_helper.c:162:
+    env->pc |= ((env->tl > 1) ? 1 << 14 : 0) | (intno << 5);^M$

total: 2 errors, 0 warnings, 11 lines checked

/tmp/artyom/[PATCH 2_2 for-2.6] target-sparc: fix Trap Based Address
Register behavior for sparc64.eml has style problems, please review.  If
any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.


Possibly the DOS line endings are being added by a mail program?


ATB,

Mark.
Artyom Tarasenko April 14, 2016, 2:49 p.m. UTC | #4
On Thu, Apr 14, 2016 at 4:20 PM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 14/04/16 15:04, Artyom Tarasenko wrote:
>
>> Can you please show  the output of your checkpatch.pl report? I get:
>>
>>  $  scripts/checkpatch.pl
>> 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch
>> total: 0 errors, 0 warnings, 25 lines checked
>> 0001-target-sparc-fix-Nucleus-quad-LDD-128-bit-access-for.patch has no
>> obvious style problems and is ready for Submission.
>
> Okay I've just done an update pull and complete rebuild which fixes the
> fuzz, but I'm still seeing line ending errors in checkpatch.pl:
>
>
> $ ./scripts/checkpatch.pl /tmp/artyom/\[PATCH\ 1_2\ for-2.6\]\
> target-sparc\:\ fix\ Nucleus\ quad\ LDD\ 128\ bit\ access\ for\
> windowed\ registers.eml
> ERROR: DOS line endings
> #75: FILE: target-sparc/ldst_helper.c:2062:
> +            env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr);^M$
>
> ERROR: DOS line endings
> #76: FILE: target-sparc/ldst_helper.c:2063:
> +            env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8);^M$
>
> ERROR: DOS line endings
> #80: FILE: target-sparc/ldst_helper.c:2065:
> +                bswap64s(&env->regwptr[rd - 8]);^M$
>
> ERROR: DOS line endings
> #81: FILE: target-sparc/ldst_helper.c:2066:
> +                bswap64s(&env->regwptr[rd + 1 - 8]);^M$
>
> ERROR: DOS line endings
> #91: FILE: target-sparc/ldst_helper.c:2079:
> +            env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0);^M$
>
> ERROR: DOS line endings
> #92: FILE: target-sparc/ldst_helper.c:2080:
> +            env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4,
> asi, 4, 0);^M$
>
> total: 6 errors, 0 warnings, 25 lines checked
>
> /tmp/artyom/[PATCH 1_2 for-2.6] target-sparc: fix Nucleus quad LDD 128
> bit access for windowed registers.eml has style problems, please review.
>  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> and:
>
> $ ./scripts/checkpatch.pl /tmp/artyom/\[PATCH\ 2_2\ for-2.6\]\
> target-sparc\:\ fix\ Trap\ Based\ Address\ Register\ behavior\ for\
> sparc64.eml
> ERROR: DOS line endings
> #85: FILE: target-sparc/int64_helper.c:161:
> +    env->pc = env->tbr  & ~0x7fffULL;^M$
>
> ERROR: DOS line endings
> #86: FILE: target-sparc/int64_helper.c:162:
> +    env->pc |= ((env->tl > 1) ? 1 << 14 : 0) | (intno << 5);^M$
>
> total: 2 errors, 0 warnings, 11 lines checked
>
> /tmp/artyom/[PATCH 2_2 for-2.6] target-sparc: fix Trap Based Address
> Register behavior for sparc64.eml has style problems, please review.  If
> any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
>
> Possibly the DOS line endings are being added by a mail program?

I use git send-email (which uses msmtp). Used to work fine.

Clicking on "download mbox" on
https://patchwork.ozlabs.org/patch/610402/ and running checkpatch.pl
reveals no errors.

Are you sure the problem is not on your side?

Kind regards,
Artyom
Mark Cave-Ayland April 14, 2016, 3:59 p.m. UTC | #5
On 14/04/16 15:49, Artyom Tarasenko wrote:

>> Possibly the DOS line endings are being added by a mail program?
> 
> I use git send-email (which uses msmtp). Used to work fine.
> 
> Clicking on "download mbox" on
> https://patchwork.ozlabs.org/patch/610402/ and running checkpatch.pl
> reveals no errors.
> 
> Are you sure the problem is not on your side?

In that case please accept my apologies - it seems something in that
patch is triggering my mail client to output CR+LF when exporting
messages. If I switch over to mutt then then indeed the patches appear
as they should.

I'm not sure if Richard is around at the moment, but they look trivial
enough and fix some nasty register corruption bugs so I think they are
worth adding to 2.6. If you can re-send with my R-B then I'll send a
pull request.


ATB,

Mark.
diff mbox

Patch

diff --git a/target-sparc/ldst_helper.c b/target-sparc/ldst_helper.c
index 2b0221c..a383074 100644
--- a/target-sparc/ldst_helper.c
+++ b/target-sparc/ldst_helper.c
@@ -2059,11 +2059,11 @@  void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd)
                 bswap64s(&env->gregs[rd + 1]);
             }
         } else {
-            env->regwptr[rd] = cpu_ldq_nucleus(env, addr);
-            env->regwptr[rd + 1] = cpu_ldq_nucleus(env, addr + 8);
+            env->regwptr[rd - 8] = cpu_ldq_nucleus(env, addr);
+            env->regwptr[rd + 1 - 8] = cpu_ldq_nucleus(env, addr + 8);
             if (asi == 0x2c) {
-                bswap64s(&env->regwptr[rd]);
-                bswap64s(&env->regwptr[rd + 1]);
+                bswap64s(&env->regwptr[rd - 8]);
+                bswap64s(&env->regwptr[rd + 1 - 8]);
             }
         }
         break;
@@ -2076,8 +2076,8 @@  void helper_ldda_asi(CPUSPARCState *env, target_ulong addr, int asi, int rd)
             env->gregs[rd] = helper_ld_asi(env, addr, asi, 4, 0);
             env->gregs[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0);
         } else {
-            env->regwptr[rd] = helper_ld_asi(env, addr, asi, 4, 0);
-            env->regwptr[rd + 1] = helper_ld_asi(env, addr + 4, asi, 4, 0);
+            env->regwptr[rd - 8] = helper_ld_asi(env, addr, asi, 4, 0);
+            env->regwptr[rd + 1 - 8] = helper_ld_asi(env, addr + 4, asi, 4, 0);
         }
         break;
     }