MIPS64 timer bug
v2 cover MIPS64 timer bug
We found this on a custom MIPS64 based board. Basically we were seeing udelay and friends do odd things occasionally. The problem appears to be when we read the c0 32-bit counter sometimes we get unexpected values when this is returned as an unsigned long on a 64-bit system. I've written a little test program (below) that demonstrates the what I think is problem, it'll work on a x86_64 desktop. I don't know the ins and outs of the C specifications to comment on if this is a compiler bug but I still think we should be treating the c0 registers as unsigned regardless.
$ cat test.c #include <stdio.h>
int main(int argc, char *argv[])
{
int a;
unsigned long b;
a = 0x80000000;
b = a;
printf("sizeof(int) = %zu\n", sizeof(int));
printf("sizeof(unsigned long) = %zu\n", sizeof(unsigned long));
printf("a = %d\n", a);
printf("a = %x\n", a);
printf("b = %lu\n", b);
printf("b = %lx\n", b);
return 0;}
Which yields the following output
$ ./test sizeof(int) = 4 sizeof(unsigned long) = 8 a = -2147483648 a = 80000000 b = 18446744071562067968 b = ffffffff80000000
And for completeness
$ gcc --version gcc (Ubuntu 4.8.4-2ubuntu1~14.04) 4.8.4 Copyright (C) 2013 Free Software Foundation, Inc.
Our embedded target is using
$ mips64-octeon-linux-gnu-gcc --version mips64-octeon-linux-gnu-gcc (GCC) 4.7.0 Copyright (C) 2012 Free Software Foundation, Inc.
Changes in v2:
- Use Rob's current email address
Chris Packham (1):
mips: Use unsigned int when reading c0 registers
arch/mips/include/asm/mipsregs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Signature · 1 line
2.5.0.rc0
RFC PATCH v2 1/1 mips: Use unsigned int when reading c0 registers
In commit a18a477 (MIPS: use common code from lib/time.c) MIPS platforms started using common the common timer functions which are based around the fact that many platforms have a 32-bit free running counter register that can be used see commit 8dfafdd (Introduce common timer functions).
Even MIPS64 has such a 32-bit register (some have an additional 64-bit free running counter, but that's something for another time).
The problem is that in __read_32bit_c0_register() we read the value from this register into an _signed_ int and as it's returned up the call chain to timer_read_counter() it gets assigned to an unsigned long. On a 32-bit system there is no problem. On a 64-bit system odd things happen, sign extension seems to kick in and all of a sudden if the counter register happens to have the MSb (i.e. the sign bit) set the negative int gets sign extended into a very large unsigned long value. This in turn throws out things from get_ticks() up.
Update __read_32bit_c0_register() and __read_32bit_c0_ctrl_register() to use "unsigned int res;" instead of "int res;". There seems to be little reason to treat these register values as signed. They are either counters (which by definition are unsigned) or are made up of various bit fields to be interpreted as per the CPU datasheet.
---
Changes in v2:
- Use Rob's current email address
arch/mips/include/asm/mipsregs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/mips/include/asm/mipsregs.h +2 -2
@@ -594,7 +594,7 @@ do { \*/ #define __read_32bit_c0_register (source, sel) \ -({ int __res; \ +({ unsigned int __res; \ if (sel == 0) \ __asm__ __volatile__( \ "mfc0\t%0, " #source "\n\t" \
@@ -676,7 +676,7 @@ do { \* On RM7000/RM9000 these are uses to access cop0 set 1 registers */ #define __read_32bit_c0_ctrl_register (source) \ -({ int __res; \ +({ unsigned int __res; \ __asm__ __volatile__( \ "cfc0\t%0, " #source "\n\t" \ : "=r" (__res)); \
Signature · 1 line
2.5.0.rc0
RFC PATCH v2 1/1 mips: Use unsigned int when reading c0 registers
Hi Chris,
sorry for the delay.
Am 14.07.2015 um 12:54 schrieb Chris Packham:
Quoted 17 lines
In commit a18a477 (MIPS: use common code from
lib/time.c) MIPS platforms started using common the common timer functions which are based around the fact that many platforms have a 32-bit free running counter register that can be used see commit 8dfafdd (Introduce common timer functions).Even MIPS64 has such a 32-bit register (some have an additional 64-bit free running counter, but that's something for another time).
The problem is that in
__read_32bit_c0_register()we read the value from this register into an _signed_ int and as it's returned up the call chain totimer_read_counter()it gets assigned to an unsigned long. On a 32-bit system there is no problem. On a 64-bit system odd things happen, sign extension seems to kick in and all of a sudden if the counter register happens to have the MSb (i.e. the sign bit) set the negative int gets sign extended into a very large unsigned long value. This in turn throws out things fromget_ticks()up.
Quoted 5 lines
Update
__read_32bit_c0_register()and__read_32bit_c0_ctrl_register()to use "unsigned int res;" instead of "int res;". There seems to be little reason to treat these register values as signed. They are either counters (which by definition are unsigned) or are made up of various bit fields to be interpreted as per the CPU datasheet.
I agree that those macros should always use unsigned int's. Also some similar but newer macros use unsigned int's. But that header file is imported from Linux kernel and I'd like to keep it in sync. Could you post a similar patch to Linux MIPS mailing list? Maybe someone there know why signed int's are used and if a change would have side-effects. Thanks.
Quoted 35 lines
Reported-by Sachin Surendran <[email protected]>Signed-off-by Chris Packham <[email protected]>---
Changes in v2:
- Use Rob's current email addressarch/mips/include/asm/mipsregs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/mips/include/asm/mipsregs.h +2
-2@@ -594,7 +594,7 @@ do { \*/ #define __read_32bit_c0_register (source, sel) \ -({ int __res; \ +({ unsigned int __res; \ if (sel == 0) \ __asm__ __volatile__( \ "mfc0\t%0, " #source "\n\t" \@@ -676,7 +676,7 @@ do { \* On RM7000/RM9000 these are uses to access cop0 set 1 registers */ #define __read_32bit_c0_ctrl_register (source) \ -({ int __res; \ +({ unsigned int __res; \ __asm__ __volatile__( \ "cfc0\t%0, " #source "\n\t" \ : "=r" (__res)); \
Signature · 1 line
- Daniel
RFC PATCH v2 1/1 mips: Use unsigned int when reading c0 registers
Quoted 3 lines · Daniel Schwierzeck
Hi Chris,
sorry for the delay.
No problem. It only just occurred to me that it's probably peak holiday season for people in the northern hemisphere.
Quoted 24 lines
Am 14.07.2015 um 12:54 schrieb Chris Packham:
Quoted 17 lines
In commit a18a477 (MIPS: use common code from
lib/time.c) MIPS platforms started using common the common timer functions which are based around the fact that many platforms have a 32-bit free running counter register that can be used see commit 8dfafdd (Introduce common timer functions).Even MIPS64 has such a 32-bit register (some have an additional 64-bit free running counter, but that's something for another time).
The problem is that in
__read_32bit_c0_register()we read the value from this register into an _signed_ int and as it's returned up the call chain totimer_read_counter()it gets assigned to an unsigned long. On a 32-bit system there is no problem. On a 64-bit system odd things happen, sign extension seems to kick in and all of a sudden if the counter register happens to have the MSb (i.e. the sign bit) set the negative int gets sign extended into a very large unsigned long value. This in turn throws out things fromget_ticks()up.Quoted 5 lines
Update
__read_32bit_c0_register()and__read_32bit_c0_ctrl_register()to use "unsigned int res;" instead of "int res;". There seems to be little reason to treat these register values as signed. They are either counters (which by definition are unsigned) or are made up of various bit fields to be interpreted as per the CPU datasheet.
Quoted 6 lines
I agree that those macros should always use unsigned int's. Also some similar but newer macros use unsigned int's. But that header file is imported from Linux kernel and I'd like to keep it in sync. Could you post a similar patch to Linux MIPS mailing list? Maybe someone there know why signed int's are used and if a change would have side-effects. Thanks.
OK I'll go looking there, they may have already fixed it.
Quoted 39 lines
Quoted 28 lines
Reported-by Sachin Surendran <[email protected]>Signed-off-by Chris Packham <[email protected]>---
Changes in v2:
- Use Rob's current email addressarch/mips/include/asm/mipsregs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/mips/include/asm/mipsregs.h +1
-1@@ -594,7 +594,7 @@ do { \*/ #define __read_32bit_c0_register (source, sel) \ -({ int __res; \ +({ unsigned int __res; \ if (sel == 0) \ __asm__ __volatile__( \ "mfc0\t%0, " #source "\n\t" \arch/mips/include/asm/mipsregs.h do { \ · line 682
Quoted 8 lines
arch/mips/include/asm/mipsregs.h +1
-1@@ -676,7 +676,7 @@ do { \* On RM7000/RM9000 these are uses to access cop0 set 1 registers */ #define __read_32bit_c0_ctrl_register (source) \ -({ int __res; \ +({ unsigned int __res; \ __asm__ __volatile__( \ "cfc0\t%0, " #source "\n\t" \ : "=r" (__res)); \--
- Daniel
RFC PATCH v2 1/1 mips: Use unsigned int when reading c0 registers
Quoted 33 lines · Chris Packham
Quoted 3 lines · Daniel Schwierzeck
Hi Chris,
sorry for the delay.
No problem. It only just occurred to me that it's probably peak holiday season for people in the northern hemisphere.
Quoted 24 lines
Am 14.07.2015 um 12:54 schrieb Chris Packham:
Quoted 22 lines
In commit a18a477 (MIPS: use common code from
lib/time.c) MIPS platforms started using common the common timer functions which are based around the fact that many platforms have a 32-bit free running counter register that can be used see commit 8dfafdd (Introduce common timer functions).Even MIPS64 has such a 32-bit register (some have an additional 64-bit free running counter, but that's something for another time).
The problem is that in
__read_32bit_c0_register()we read the value from this register into an _signed_ int and as it's returned up the call chain totimer_read_counter()it gets assigned to an unsigned long. On a 32-bit system there is no problem. On a 64-bit system odd things happen, sign extension seems to kick in and all of a sudden if the counter register happens to have the MSb (i.e. the sign bit) set the negative int gets sign extended into a very large unsigned long value. This in turn throws out things fromget_ticks()up.Update
__read_32bit_c0_register()and__read_32bit_c0_ctrl_register()to use "unsigned int res;" instead of "int res;". There seems to be little reason to treat these register values as signed. They are either counters (which by definition are unsigned) or are made up of various bit fields to be interpreted as per the CPU datasheet.
Quoted 9 lines
Quoted 6 lines
I agree that those macros should always use unsigned int's. Also some similar but newer macros use unsigned int's. But that header file is imported from Linux kernel and I'd like to keep it in sync. Could you post a similar patch to Linux MIPS mailing list? Maybe someone there know why signed int's are used and if a change would have side-effects. Thanks.
OK I'll go looking there, they may have already fixed it.
Linux patch is working it's way through
http://www.linux-mips.org/archives/linux-mips/2015-07/msg00262.html.
Looks like I've missed the merge window for 4.2 so this is queued up for 4.3. It doesn't appear to be in any public repo yet. If I wait for the Linux 4.3 merge window to open before syncing this change to u-boot I'll miss the 2015.10 merge window.
How do you want to proceed? Would it be possible to apply this patch to u-boot now with the knowledge that it could be sync'd in the future.
Quoted 39 lines
Quoted 39 lines
Quoted 28 lines
Reported-by Sachin Surendran <[email protected]>Signed-off-by Chris Packham <[email protected]>---
Changes in v2:
- Use Rob's current email addressarch/mips/include/asm/mipsregs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/mips/include/asm/mipsregs.h +1
-1@@ -594,7 +594,7 @@ do { \*/ #define __read_32bit_c0_register (source, sel) \ -({ int __res; \ +({ unsigned int __res; \ if (sel == 0) \ __asm__ __volatile__( \ "mfc0\t%0, " #source "\n\t" \arch/mips/include/asm/mipsregs.h do { \ · line 682
Quoted 8 lines
arch/mips/include/asm/mipsregs.h +1
-1@@ -676,7 +676,7 @@ do { \* On RM7000/RM9000 these are uses to access cop0 set 1 registers */ #define __read_32bit_c0_ctrl_register (source) \ -({ int __res; \ +({ unsigned int __res; \ __asm__ __volatile__( \ "cfc0\t%0, " #source "\n\t" \ : "=r" (__res)); \--
- Daniel
RFC PATCH v2 1/1 mips: Use unsigned int when reading c0 registers
In reply to Chris PackhamAm 14.07.2015 um 12:54 schrieb Chris Packham:
Quoted 26 lines
In commit a18a477 (MIPS: use common code from
lib/time.c) MIPS platforms started using common the common timer functions which are based around the fact that many platforms have a 32-bit free running counter register that can be used see commit 8dfafdd (Introduce common timer functions).Even MIPS64 has such a 32-bit register (some have an additional 64-bit free running counter, but that's something for another time).
The problem is that in
__read_32bit_c0_register()we read the value from this register into an _signed_ int and as it's returned up the call chain totimer_read_counter()it gets assigned to an unsigned long. On a 32-bit system there is no problem. On a 64-bit system odd things happen, sign extension seems to kick in and all of a sudden if the counter register happens to have the MSb (i.e. the sign bit) set the negative int gets sign extended into a very large unsigned long value. This in turn throws out things fromget_ticks()up.Update
__read_32bit_c0_register()and__read_32bit_c0_ctrl_register()to use "unsigned int res;" instead of "int res;". There seems to be little reason to treat these register values as signed. They are either counters (which by definition are unsigned) or are made up of various bit fields to be interpreted as per the CPU datasheet.Reported-by Sachin Surendran <[email protected]>Signed-off-by Chris Packham <[email protected]>
Quoted 7 lines
---
Changes in v2:
- Use Rob's current email addressarch/mips/include/asm/mipsregs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
applied to u-boot-mips/next, thanks
Signature · 1 line
- Daniel