Skip to contentSkip to conversation

ARM: fix do_div() bug in big-endian systems

PATCH New Lu Xiangyu 3 participants 6 messages Apr 11, 2014 – Apr 15, 2014 Arm (32-bit)

lore
Share & tools
lore

Lu Xiangyu

author
#0 lore raw

PATCH 1/1 ARM: fix do_div() bug in big-endian systems

From: Xiangyu Lu <[email protected]>

In big-endian systems, "%1" get the most significant part of the value, cause the instruction to get the wrong result.

When viewing ftrace record in big-endian ARM systems, we found that the timestamp errors:

swapper-0     [001]  1325.970000:      0:120:R ==> [001]    16:120:R events/1
events/1-16   [001]  1325.970000:      16:120:S ==> [001]    0:120:R swapper
swapper-0     [000]  1325.1000000:     0:120:R   + [000]    15:120:R events/0
swapper-0     [000]  1325.1000000:     0:120:R ==> [000]    15:120:R events/0
swapper-0     [000]  1326.030000:      0:120:R   + [000]  1150:120:R sshd
swapper-0     [000]  1326.030000:      0:120:R ==> [000]  1150:120:R sshd

When viewed ftrace records, it will call the do_div(n, base) function, which achieved arch/arm/include/asm/div64.h in. When n = 10000000, base = 1000000, in do_div(n, base) will execute "umull %Q0, %R0, %1, %Q2".

Cc <[email protected]> # 2.6.20+
Signed-off-by Alex Wu <[email protected]>
Signed-off-by Xiangyu Lu <[email protected]>
---
 arch/arm/include/asm/div64.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/div64.h +1 -1
index 191ada6..662c7bd 100644
@@ -156,7 +156,7 @@
 		/* Select the best insn combination to perform the   */	\
 		/* actual __m * __n / (__p << 64) operation.         */	\
 		if (!__c) {						\
-			asm (	"umull	%Q0, %R0, %1, %Q2\n\t"		\
+			asm (	"umull	%Q0, %R0, %Q1, %Q2\n\t"		\
 				"mov	%Q0, #0"			\
 				: "=&r" (__res)				\
 				: "r" (__m), "r" (__n)			\
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/1 ARM: fix do_div() bug in big-endian systems

Quoted 31 lines · Lu Xiangyu

From: Xiangyu Lu <[email protected]>

In big-endian systems, "%1" get the most significant part of the value, cause the instruction to get the wrong result.

When viewing ftrace record in big-endian ARM systems, we found that the timestamp errors:

swapper-0     [001]  1325.970000:      0:120:R ==> [001]    16:120:R events/1
events/1-16   [001]  1325.970000:      16:120:S ==> [001]    0:120:R swapper
swapper-0     [000]  1325.1000000:     0:120:R   + [000]    15:120:R events/0
swapper-0     [000]  1325.1000000:     0:120:R ==> [000]    15:120:R events/0
swapper-0     [000]  1326.030000:      0:120:R   + [000]  1150:120:R sshd
swapper-0     [000]  1326.030000:      0:120:R ==> [000]  1150:120:R sshd

When viewed ftrace records, it will call the do_div(n, base) function, which achieved arch/arm/include/asm/div64.h in. When n = 10000000, base = 1000000, in do_div(n, base) will execute "umull %Q0, %R0, %1, %Q2".

Cc <[email protected]> # 2.6.20+
Signed-off-by Alex Wu <[email protected]>
Signed-off-by Xiangyu Lu <[email protected]>
---
 arch/arm/include/asm/div64.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/div64.h +0 -0
index 191ada6..662c7bd 100644

arch/arm/include/asm/div64.h line 159

Quoted 5 lines
arch/arm/include/asm/div64.h +1 -1
@@ -156,7 +156,7 @@
 		/* Select the best insn combination to perform the   */	\
 		/* actual __m * __n / (__p << 64) operation.         */	\
 		if (!__c) {						\
-			asm (	"umull	%Q0, %R0, %1, %Q2\n\t"		\
+			asm (	"umull	%Q0, %R0, %Q1, %Q2\n\t"		\

This looks plausible: these if() clauses are all concerned with multiplying the low parts of __m and __n together, and this seems to be the only 64-bit asm operand reference where Q or R is suspiciously
missing: so it looks likely that "Q" is required here for consistency.

My understanding of the details of this code are limited: do you have a simple test case to demonstrate the error and the fix?

It should be sufficient to find two values a and b, where do_div(a,b) demonstrates the bug.

Cheers
---Dave

#2 lore raw

PATCH 1/1 ARM: fix do_div() bug in big-endian systems

Quoted 43 lines · Dave Martin
Quoted 31 lines · Lu Xiangyu

From: Xiangyu Lu <[email protected]>

In big-endian systems, "%1" get the most significant part of the value, cause the instruction to get the wrong result.

When viewing ftrace record in big-endian ARM systems, we found that the timestamp errors:

swapper-0     [001]  1325.970000:      0:120:R ==> [001]    16:120:R events/1
events/1-16   [001]  1325.970000:      16:120:S ==> [001]    0:120:R swapper
swapper-0     [000]  1325.1000000:     0:120:R   + [000]    15:120:R events/0
swapper-0     [000]  1325.1000000:     0:120:R ==> [000]    15:120:R events/0
swapper-0     [000]  1326.030000:      0:120:R   + [000]  1150:120:R sshd
swapper-0     [000]  1326.030000:      0:120:R ==> [000]  1150:120:R sshd

When viewed ftrace records, it will call the do_div(n, base) function, which achieved arch/arm/include/asm/div64.h in. When n = 10000000, base = 1000000, in do_div(n, base) will execute "umull %Q0, %R0, %1, %Q2".

Cc <[email protected]> # 2.6.20+
Signed-off-by Alex Wu <[email protected]>
Signed-off-by Xiangyu Lu <[email protected]>
---
 arch/arm/include/asm/div64.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/div64.h +0 -0
index 191ada6..662c7bd 100644

arch/arm/include/asm/div64.h line 159

Quoted 5 lines
arch/arm/include/asm/div64.h +1 -1
@@ -156,7 +156,7 @@
 		/* Select the best insn combination to perform the   */	\
 		/* actual __m * __n / (__p << 64) operation.         */	\
 		if (!__c) {						\
-			asm (	"umull	%Q0, %R0, %1, %Q2\n\t"		\
+			asm (	"umull	%Q0, %R0, %Q1, %Q2\n\t"		\

This looks plausible: these if() clauses are all concerned with multiplying the low parts of __m and __n together, and this seems to be the only 64-bit asm operand reference where Q or R is suspiciously
missing: so it looks likely that "Q" is required here for consistency.

Quoted 2 lines

My understanding of the details of this code are limited: do you have a simple test case to demonstrate the error and the fix?

No need -- it is indeed wrong on big endian and has been so for the last 7.5 years.

Nicolas

#3 lore raw

PATCH 1/1 ARM: fix do_div() bug in big-endian systems

Quoted 45 lines · Nicolas Pitre
Quoted 43 lines · Dave Martin
Quoted 31 lines · Lu Xiangyu

From: Xiangyu Lu <[email protected]>

In big-endian systems, "%1" get the most significant part of the value, cause the instruction to get the wrong result.

When viewing ftrace record in big-endian ARM systems, we found that the timestamp errors:

swapper-0     [001]  1325.970000:      0:120:R ==> [001]    16:120:R events/1
events/1-16   [001]  1325.970000:      16:120:S ==> [001]    0:120:R swapper
swapper-0     [000]  1325.1000000:     0:120:R   + [000]    15:120:R events/0
swapper-0     [000]  1325.1000000:     0:120:R ==> [000]    15:120:R events/0
swapper-0     [000]  1326.030000:      0:120:R   + [000]  1150:120:R sshd
swapper-0     [000]  1326.030000:      0:120:R ==> [000]  1150:120:R sshd

When viewed ftrace records, it will call the do_div(n, base) function, which achieved arch/arm/include/asm/div64.h in. When n = 10000000, base = 1000000, in do_div(n, base) will execute "umull %Q0, %R0, %1, %Q2".

Cc <[email protected]> # 2.6.20+
Signed-off-by Alex Wu <[email protected]>
Signed-off-by Xiangyu Lu <[email protected]>
---
 arch/arm/include/asm/div64.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/div64.h +0 -0
index 191ada6..662c7bd 100644

arch/arm/include/asm/div64.h line 159

Quoted 5 lines
arch/arm/include/asm/div64.h +1 -1
@@ -156,7 +156,7 @@
 		/* Select the best insn combination to perform the   */	\
 		/* actual __m * __n / (__p << 64) operation.         */	\
 		if (!__c) {						\
-			asm (	"umull	%Q0, %R0, %1, %Q2\n\t"		\
+			asm (	"umull	%Q0, %R0, %Q1, %Q2\n\t"		\

This looks plausible: these if() clauses are all concerned with multiplying the low parts of __m and __n together, and this seems to be the only 64-bit asm operand reference where Q or R is suspiciously
missing: so it looks likely that "Q" is required here for consistency.

Quoted 5 lines
Quoted 2 lines

My understanding of the details of this code are limited: do you have a simple test case to demonstrate the error and the fix?

No need -- it is indeed wrong on big endian and has been so for the last 7.5 years.

OK, well with that sanity-check on my reasoning I'm happy to:

Reviewed-by Dave Martin <[email protected]>

I suggest you go ahead and send it to Russell's patch system.

Cheers
---Dave

author
#4 lore raw

PATCH 1/1 ARM: fix do_div() bug in big-endian systems

Quoted 51 lines · Dave Martin
Quoted 44 lines · Nicolas Pitre
Quoted 42 lines · Dave Martin
Quoted 31 lines · Lu Xiangyu

From: Xiangyu Lu <[email protected]>

In big-endian systems, "%1" get the most significant part of the value, cause the instruction to get the wrong result.

When viewing ftrace record in big-endian ARM systems, we found that the timestamp errors:

swapper-0     [001]  1325.970000:      0:120:R ==> [001]    16:120:R events/1
events/1-16   [001]  1325.970000:      16:120:S ==> [001]    0:120:R swapper
swapper-0     [000]  1325.1000000:     0:120:R   + [000]    15:120:R events/0
swapper-0     [000]  1325.1000000:     0:120:R ==> [000]    15:120:R events/0
swapper-0     [000]  1326.030000:      0:120:R   + [000]  1150:120:R sshd
swapper-0     [000]  1326.030000:      0:120:R ==> [000]  1150:120:R sshd

When viewed ftrace records, it will call the do_div(n, base) function, which achieved arch/arm/include/asm/div64.h in. When n = 10000000, base = 1000000, in do_div(n, base) will execute "umull %Q0, %R0, %1, %Q2".

Cc <[email protected]> # 2.6.20+
Signed-off-by Alex Wu <[email protected]>
Signed-off-by Xiangyu Lu <[email protected]>
---
  arch/arm/include/asm/div64.h |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/div64.h +0 -0
index 191ada6..662c7bd 100644

arch/arm/include/asm/div64.h line 159

Quoted 5 lines
arch/arm/include/asm/div64.h +1 -1
@@ -156,7 +156,7 @@
  		/* Select the best insn combination to perform the   */	\
  		/* actual __m * __n / (__p << 64) operation.         */	\
  		if (!__c) {						\
-			asm (	"umull	%Q0, %R0, %1, %Q2\n\t"		\
+			asm (	"umull	%Q0, %R0, %Q1, %Q2\n\t"		\

This looks plausible: these if() clauses are all concerned with multiplying the low parts of __m and __n together, and this seems to be the only 64-bit asm operand reference where Q or R is suspiciously
missing: so it looks likely that "Q" is required here for consistency.

Quoted 4 lines
Quoted 2 lines

My understanding of the details of this code are limited: do you have a simple test case to demonstrate the error and the fix?

No need -- it is indeed wrong on big endian and has been so for the last 7.5 years.

OK, well with that sanity-check on my reasoning I'm happy to:

Quoted 8 lines
Reviewed-by Dave Martin <[email protected]>

I suggest you go ahead and send it to Russell's patch system.

Cheers
---Dave
.

OK, thanks.

#5 lore raw

PATCH 1/1 ARM: fix do_div() bug in big-endian systems

In reply to Lu Xiangyu
Quoted 15 lines · Lu Xiangyu

From: Xiangyu Lu <[email protected]>

In big-endian systems, "%1" get the most significant part of the value, cause the instruction to get the wrong result.

When viewing ftrace record in big-endian ARM systems, we found that the timestamp errors:

swapper-0     [001]  1325.970000:      0:120:R ==> [001]    16:120:R events/1
events/1-16   [001]  1325.970000:      16:120:S ==> [001]    0:120:R swapper
swapper-0     [000]  1325.1000000:     0:120:R   + [000]    15:120:R events/0
swapper-0     [000]  1325.1000000:     0:120:R ==> [000]    15:120:R events/0
swapper-0     [000]  1326.030000:      0:120:R   + [000]  1150:120:R sshd
swapper-0     [000]  1326.030000:      0:120:R ==> [000]  1150:120:R sshd
Quoted 7 lines

When viewed ftrace records, it will call the do_div(n, base) function, which achieved arch/arm/include/asm/div64.h in. When n = 10000000, base = 1000000, in do_div(n, base) will execute "umull %Q0, %R0, %1, %Q2".

Cc <[email protected]> # 2.6.20+
Signed-off-by Alex Wu <[email protected]>
Signed-off-by Xiangyu Lu <[email protected]>

Argh.. My bad.

Reviewed-by Nicolas Pitre <[email protected]>

Please send to RMK's patch system.

Quoted 26 lines
---
 arch/arm/include/asm/div64.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/div64.h +1 -1
index 191ada6..662c7bd 100644
@@ -156,7 +156,7 @@
 		/* Select the best insn combination to perform the   */	\
 		/* actual __m * __n / (__p << 64) operation.         */	\
 		if (!__c) {						\
-			asm (	"umull	%Q0, %R0, %1, %Q2\n\t"		\
+			asm (	"umull	%Q0, %R0, %Q1, %Q2\n\t"		\
 				"mov	%Q0, #0"			\
 				: "=&r" (__res)				\
 				: "r" (__m), "r" (__n)			\
Signature · 7 lines
1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel