At the time being, __put_user()/__get_user() and friends only use
D-form addressing, with 0 offset. Ex:
lwz reg1, 0(reg2)
Give the compiler the opportunity to use other adressing modes
whenever possible, to get more optimised code.
Hereunder is a small exemple:
struct test {
u32 item1;
u16 item2;
u8 item3;
u64 item4;
};
int set_test_user(struct test __user *from, struct test __user *to)
{
int err;
u32 item1;
u16 item2;
u8 item3;
u64 item4;
err = __get_user(item1, &from->item1);
err |= __get_user(item2, &from->item2);
err |= __get_user(item3, &from->item3);
err |= __get_user(item4, &from->item4);
err |= __put_user(item1, &to->item1);
err |= __put_user(item2, &to->item2);
err |= __put_user(item3, &to->item3);
err |= __put_user(item4, &to->item4);
return err;
}
Before the patch:
00000df0 <set_test_user>:
df0: 94 21 ff f0 stwu r1,-16(r1)
df4: 39 40 00 00 li r10,0
df8: 93 c1 00 08 stw r30,8(r1)
dfc: 93 e1 00 0c stw r31,12(r1)
e00: 7d 49 53 78 mr r9,r10
e04: 80 a3 00 00 lwz r5,0(r3)
e08: 38 e3 00 04 addi r7,r3,4
e0c: 7d 46 53 78 mr r6,r10
e10: a0 e7 00 00 lhz r7,0(r7)
e14: 7d 29 33 78 or r9,r9,r6
e18: 39 03 00 06 addi r8,r3,6
e1c: 7d 46 53 78 mr r6,r10
e20: 89 08 00 00 lbz r8,0(r8)
e24: 7d 29 33 78 or r9,r9,r6
e28: 38 63 00 08 addi r3,r3,8
e2c: 7d 46 53 78 mr r6,r10
e30: 83 c3 00 00 lwz r30,0(r3)
e34: 83 e3 00 04 lwz r31,4(r3)
e38: 7d 29 33 78 or r9,r9,r6
e3c: 7d 43 53 78 mr r3,r10
e40: 90 a4 00 00 stw r5,0(r4)
e44: 7d 29 1b 78 or r9,r9,r3
e48: 38 c4 00 04 addi r6,r4,4
e4c: 7d 43 53 78 mr r3,r10
e50: b0 e6 00 00 sth r7,0(r6)
e54: 7d 29 1b 78 or r9,r9,r3
e58: 38 e4 00 06 addi r7,r4,6
e5c: 7d 43 53 78 mr r3,r10
e60: 99 07 00 00 stb r8,0(r7)
e64: 7d 23 1b 78 or r3,r9,r3
e68: 38 84 00 08 addi r4,r4,8
e6c: 93 c4 00 00 stw r30,0(r4)
e70: 93 e4 00 04 stw r31,4(r4)
e74: 7c 63 53 78 or r3,r3,r10
e78: 83 c1 00 08 lwz r30,8(r1)
e7c: 83 e1 00 0c lwz r31,12(r1)
e80: 38 21 00 10 addi r1,r1,16
e84: 4e 80 00 20 blr
After the patch:
00000dbc <set_test_user>:
dbc: 39 40 00 00 li r10,0
dc0: 7d 49 53 78 mr r9,r10
dc4: 80 03 00 00 lwz r0,0(r3)
dc8: 7d 48 53 78 mr r8,r10
dcc: a1 63 00 04 lhz r11,4(r3)
dd0: 7d 29 43 78 or r9,r9,r8
dd4: 7d 48 53 78 mr r8,r10
dd8: 88 a3 00 06 lbz r5,6(r3)
ddc: 7d 29 43 78 or r9,r9,r8
de0: 7d 48 53 78 mr r8,r10
de4: 80 c3 00 08 lwz r6,8(r3)
de8: 80 e3 00 0c lwz r7,12(r3)
dec: 7d 29 43 78 or r9,r9,r8
df0: 7d 43 53 78 mr r3,r10
df4: 90 04 00 00 stw r0,0(r4)
df8: 7d 29 1b 78 or r9,r9,r3
dfc: 7d 43 53 78 mr r3,r10
e00: b1 64 00 04 sth r11,4(r4)
e04: 7d 29 1b 78 or r9,r9,r3
e08: 7d 43 53 78 mr r3,r10
e0c: 98 a4 00 06 stb r5,6(r4)
e10: 7d 23 1b 78 or r3,r9,r3
e14: 90 c4 00 08 stw r6,8(r4)
e18: 90 e4 00 0c stw r7,12(r4)
e1c: 7c 63 53 78 or r3,r3,r10
e20: 4e 80 00 20 blr
Signed-off-by: Christophe Leroy <[email protected]>
Reviewed-by: Segher Boessenkool <[email protected]>
---
v2:
- Added <> modifier in __put_user_asm() and __get_user_asm()
- Removed %U2 in __put_user_asm2() and __get_user_asm2()
- Reworded the commit log
---
arch/powerpc/include/asm/uaccess.h | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 7c811442b607..9365b59495a2 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -114,7 +114,7 @@ extern long __put_user_bad(void);
*/
#define __put_user_asm(x, addr, err, op) \
__asm__ __volatile__( \
- "1: " op " %1,0(%2) # put_user\n" \
+ "1: " op "%U2%X2 %1,%2 # put_user\n" \
"2:\n" \
".section .fixup,\"ax\"\n" \
"3: li %0,%3\n" \
@@ -122,7 +122,7 @@ extern long __put_user_bad(void);
".previous\n" \
EX_TABLE(1b, 3b) \
: "=r" (err) \
- : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
+ : "r" (x), "m<>" (*addr), "i" (-EFAULT), "0" (err))
#ifdef __powerpc64__
#define __put_user_asm2(x, ptr, retval) \
@@ -130,8 +130,8 @@ extern long __put_user_bad(void);
#else /* __powerpc64__ */
#define __put_user_asm2(x, addr, err) \
__asm__ __volatile__( \
- "1: stw %1,0(%2)\n" \
- "2: stw %1+1,4(%2)\n" \
+ "1: stw%X2 %1,%2\n" \
+ "2: stw%X2 %L1,%L2\n" \
"3:\n" \
".section .fixup,\"ax\"\n" \
"4: li %0,%3\n" \
@@ -140,7 +140,7 @@ extern long __put_user_bad(void);
EX_TABLE(1b, 4b) \
EX_TABLE(2b, 4b) \
: "=r" (err) \
- : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
+ : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err))
#endif /* __powerpc64__ */
#define __put_user_size_allowed(x, ptr, size, retval) \
@@ -260,7 +260,7 @@ extern long __get_user_bad(void);
#define __get_user_asm(x, addr, err, op) \
__asm__ __volatile__( \
- "1: "op" %1,0(%2) # get_user\n" \
+ "1: "op"%U2%X2 %1, %2 # get_user\n" \
"2:\n" \
".section .fixup,\"ax\"\n" \
"3: li %0,%3\n" \
@@ -269,7 +269,7 @@ extern long __get_user_bad(void);
".previous\n" \
EX_TABLE(1b, 3b) \
: "=r" (err), "=r" (x) \
- : "b" (addr), "i" (-EFAULT), "0" (err))
+ : "m<>" (*addr), "i" (-EFAULT), "0" (err))
#ifdef __powerpc64__
#define __get_user_asm2(x, addr, err) \
@@ -277,8 +277,8 @@ extern long __get_user_bad(void);
#else /* __powerpc64__ */
#define __get_user_asm2(x, addr, err) \
__asm__ __volatile__( \
- "1: lwz %1,0(%2)\n" \
- "2: lwz %1+1,4(%2)\n" \
+ "1: lwz%X2 %1, %2\n" \
+ "2: lwz%X2 %L1, %L2\n" \
"3:\n" \
".section .fixup,\"ax\"\n" \
"4: li %0,%3\n" \
@@ -289,7 +289,7 @@ extern long __get_user_bad(void);
EX_TABLE(1b, 4b) \
EX_TABLE(2b, 4b) \
: "=r" (err), "=&r" (x) \
- : "b" (addr), "i" (-EFAULT), "0" (err))
+ : "m" (*addr), "i" (-EFAULT), "0" (err))
#endif /* __powerpc64__ */
#define __get_user_size_allowed(x, ptr, size, retval) \
@@ -299,10 +299,10 @@ do { \
if (size > sizeof(x)) \
(x) = __get_user_bad(); \
switch (size) { \
- case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
- case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
- case 4: __get_user_asm(x, ptr, retval, "lwz"); break; \
- case 8: __get_user_asm2(x, ptr, retval); break; \
+ case 1: __get_user_asm(x, (u8 __user *)ptr, retval, "lbz"); break; \
+ case 2: __get_user_asm(x, (u16 __user *)ptr, retval, "lhz"); break; \
+ case 4: __get_user_asm(x, (u32 __user *)ptr, retval, "lwz"); break; \
+ case 8: __get_user_asm2(x, (u64 __user *)ptr, retval); break; \
default: (x) = __get_user_bad(); \
} \
} while (0)
--
2.25.0
Hi Michael,
I see this patch is marked as "defered" in patchwork, but I can't see
any related discussion. Is it normal ?
Christophe
Le 16/04/2020 à 14:39, Christophe Leroy a écrit :
> At the time being, __put_user()/__get_user() and friends only use
> D-form addressing, with 0 offset. Ex:
>
> lwz reg1, 0(reg2)
>
> Give the compiler the opportunity to use other adressing modes
> whenever possible, to get more optimised code.
>
> Hereunder is a small exemple:
>
> struct test {
> u32 item1;
> u16 item2;
> u8 item3;
> u64 item4;
> };
>
> int set_test_user(struct test __user *from, struct test __user *to)
> {
> int err;
> u32 item1;
> u16 item2;
> u8 item3;
> u64 item4;
>
> err = __get_user(item1, &from->item1);
> err |= __get_user(item2, &from->item2);
> err |= __get_user(item3, &from->item3);
> err |= __get_user(item4, &from->item4);
>
> err |= __put_user(item1, &to->item1);
> err |= __put_user(item2, &to->item2);
> err |= __put_user(item3, &to->item3);
> err |= __put_user(item4, &to->item4);
>
> return err;
> }
>
> Before the patch:
>
> 00000df0 <set_test_user>:
> df0: 94 21 ff f0 stwu r1,-16(r1)
> df4: 39 40 00 00 li r10,0
> df8: 93 c1 00 08 stw r30,8(r1)
> dfc: 93 e1 00 0c stw r31,12(r1)
> e00: 7d 49 53 78 mr r9,r10
> e04: 80 a3 00 00 lwz r5,0(r3)
> e08: 38 e3 00 04 addi r7,r3,4
> e0c: 7d 46 53 78 mr r6,r10
> e10: a0 e7 00 00 lhz r7,0(r7)
> e14: 7d 29 33 78 or r9,r9,r6
> e18: 39 03 00 06 addi r8,r3,6
> e1c: 7d 46 53 78 mr r6,r10
> e20: 89 08 00 00 lbz r8,0(r8)
> e24: 7d 29 33 78 or r9,r9,r6
> e28: 38 63 00 08 addi r3,r3,8
> e2c: 7d 46 53 78 mr r6,r10
> e30: 83 c3 00 00 lwz r30,0(r3)
> e34: 83 e3 00 04 lwz r31,4(r3)
> e38: 7d 29 33 78 or r9,r9,r6
> e3c: 7d 43 53 78 mr r3,r10
> e40: 90 a4 00 00 stw r5,0(r4)
> e44: 7d 29 1b 78 or r9,r9,r3
> e48: 38 c4 00 04 addi r6,r4,4
> e4c: 7d 43 53 78 mr r3,r10
> e50: b0 e6 00 00 sth r7,0(r6)
> e54: 7d 29 1b 78 or r9,r9,r3
> e58: 38 e4 00 06 addi r7,r4,6
> e5c: 7d 43 53 78 mr r3,r10
> e60: 99 07 00 00 stb r8,0(r7)
> e64: 7d 23 1b 78 or r3,r9,r3
> e68: 38 84 00 08 addi r4,r4,8
> e6c: 93 c4 00 00 stw r30,0(r4)
> e70: 93 e4 00 04 stw r31,4(r4)
> e74: 7c 63 53 78 or r3,r3,r10
> e78: 83 c1 00 08 lwz r30,8(r1)
> e7c: 83 e1 00 0c lwz r31,12(r1)
> e80: 38 21 00 10 addi r1,r1,16
> e84: 4e 80 00 20 blr
>
> After the patch:
>
> 00000dbc <set_test_user>:
> dbc: 39 40 00 00 li r10,0
> dc0: 7d 49 53 78 mr r9,r10
> dc4: 80 03 00 00 lwz r0,0(r3)
> dc8: 7d 48 53 78 mr r8,r10
> dcc: a1 63 00 04 lhz r11,4(r3)
> dd0: 7d 29 43 78 or r9,r9,r8
> dd4: 7d 48 53 78 mr r8,r10
> dd8: 88 a3 00 06 lbz r5,6(r3)
> ddc: 7d 29 43 78 or r9,r9,r8
> de0: 7d 48 53 78 mr r8,r10
> de4: 80 c3 00 08 lwz r6,8(r3)
> de8: 80 e3 00 0c lwz r7,12(r3)
> dec: 7d 29 43 78 or r9,r9,r8
> df0: 7d 43 53 78 mr r3,r10
> df4: 90 04 00 00 stw r0,0(r4)
> df8: 7d 29 1b 78 or r9,r9,r3
> dfc: 7d 43 53 78 mr r3,r10
> e00: b1 64 00 04 sth r11,4(r4)
> e04: 7d 29 1b 78 or r9,r9,r3
> e08: 7d 43 53 78 mr r3,r10
> e0c: 98 a4 00 06 stb r5,6(r4)
> e10: 7d 23 1b 78 or r3,r9,r3
> e14: 90 c4 00 08 stw r6,8(r4)
> e18: 90 e4 00 0c stw r7,12(r4)
> e1c: 7c 63 53 78 or r3,r3,r10
> e20: 4e 80 00 20 blr
>
> Signed-off-by: Christophe Leroy <[email protected]>
> Reviewed-by: Segher Boessenkool <[email protected]>
> ---
> v2:
> - Added <> modifier in __put_user_asm() and __get_user_asm()
> - Removed %U2 in __put_user_asm2() and __get_user_asm2()
> - Reworded the commit log
> ---
> arch/powerpc/include/asm/uaccess.h | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 7c811442b607..9365b59495a2 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -114,7 +114,7 @@ extern long __put_user_bad(void);
> */
> #define __put_user_asm(x, addr, err, op) \
> __asm__ __volatile__( \
> - "1: " op " %1,0(%2) # put_user\n" \
> + "1: " op "%U2%X2 %1,%2 # put_user\n" \
> "2:\n" \
> ".section .fixup,\"ax\"\n" \
> "3: li %0,%3\n" \
> @@ -122,7 +122,7 @@ extern long __put_user_bad(void);
> ".previous\n" \
> EX_TABLE(1b, 3b) \
> : "=r" (err) \
> - : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
> + : "r" (x), "m<>" (*addr), "i" (-EFAULT), "0" (err))
>
> #ifdef __powerpc64__
> #define __put_user_asm2(x, ptr, retval) \
> @@ -130,8 +130,8 @@ extern long __put_user_bad(void);
> #else /* __powerpc64__ */
> #define __put_user_asm2(x, addr, err) \
> __asm__ __volatile__( \
> - "1: stw %1,0(%2)\n" \
> - "2: stw %1+1,4(%2)\n" \
> + "1: stw%X2 %1,%2\n" \
> + "2: stw%X2 %L1,%L2\n" \
> "3:\n" \
> ".section .fixup,\"ax\"\n" \
> "4: li %0,%3\n" \
> @@ -140,7 +140,7 @@ extern long __put_user_bad(void);
> EX_TABLE(1b, 4b) \
> EX_TABLE(2b, 4b) \
> : "=r" (err) \
> - : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
> + : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err))
> #endif /* __powerpc64__ */
>
> #define __put_user_size_allowed(x, ptr, size, retval) \
> @@ -260,7 +260,7 @@ extern long __get_user_bad(void);
>
> #define __get_user_asm(x, addr, err, op) \
> __asm__ __volatile__( \
> - "1: "op" %1,0(%2) # get_user\n" \
> + "1: "op"%U2%X2 %1, %2 # get_user\n" \
> "2:\n" \
> ".section .fixup,\"ax\"\n" \
> "3: li %0,%3\n" \
> @@ -269,7 +269,7 @@ extern long __get_user_bad(void);
> ".previous\n" \
> EX_TABLE(1b, 3b) \
> : "=r" (err), "=r" (x) \
> - : "b" (addr), "i" (-EFAULT), "0" (err))
> + : "m<>" (*addr), "i" (-EFAULT), "0" (err))
>
> #ifdef __powerpc64__
> #define __get_user_asm2(x, addr, err) \
> @@ -277,8 +277,8 @@ extern long __get_user_bad(void);
> #else /* __powerpc64__ */
> #define __get_user_asm2(x, addr, err) \
> __asm__ __volatile__( \
> - "1: lwz %1,0(%2)\n" \
> - "2: lwz %1+1,4(%2)\n" \
> + "1: lwz%X2 %1, %2\n" \
> + "2: lwz%X2 %L1, %L2\n" \
> "3:\n" \
> ".section .fixup,\"ax\"\n" \
> "4: li %0,%3\n" \
> @@ -289,7 +289,7 @@ extern long __get_user_bad(void);
> EX_TABLE(1b, 4b) \
> EX_TABLE(2b, 4b) \
> : "=r" (err), "=&r" (x) \
> - : "b" (addr), "i" (-EFAULT), "0" (err))
> + : "m" (*addr), "i" (-EFAULT), "0" (err))
> #endif /* __powerpc64__ */
>
> #define __get_user_size_allowed(x, ptr, size, retval) \
> @@ -299,10 +299,10 @@ do { \
> if (size > sizeof(x)) \
> (x) = __get_user_bad(); \
> switch (size) { \
> - case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
> - case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
> - case 4: __get_user_asm(x, ptr, retval, "lwz"); break; \
> - case 8: __get_user_asm2(x, ptr, retval); break; \
> + case 1: __get_user_asm(x, (u8 __user *)ptr, retval, "lbz"); break; \
> + case 2: __get_user_asm(x, (u16 __user *)ptr, retval, "lhz"); break; \
> + case 4: __get_user_asm(x, (u32 __user *)ptr, retval, "lwz"); break; \
> + case 8: __get_user_asm2(x, (u64 __user *)ptr, retval); break; \
> default: (x) = __get_user_bad(); \
> } \
> } while (0)
>
Christophe Leroy <[email protected]> writes:
> Hi Michael,
>
> I see this patch is marked as "defered" in patchwork, but I can't see
> any related discussion. Is it normal ?
Because it uses the "m<>" constraint which didn't work on GCC 4.6.
https://github.com/linuxppc/issues/issues/297
So we should be able to pick it up for v5.9 hopefully.
cheers
> Le 16/04/2020 à 14:39, Christophe Leroy a écrit :
>> At the time being, __put_user()/__get_user() and friends only use
>> D-form addressing, with 0 offset. Ex:
>>
>> lwz reg1, 0(reg2)
>>
>> Give the compiler the opportunity to use other adressing modes
>> whenever possible, to get more optimised code.
>>
>> Hereunder is a small exemple:
>>
>> struct test {
>> u32 item1;
>> u16 item2;
>> u8 item3;
>> u64 item4;
>> };
>>
>> int set_test_user(struct test __user *from, struct test __user *to)
>> {
>> int err;
>> u32 item1;
>> u16 item2;
>> u8 item3;
>> u64 item4;
>>
>> err = __get_user(item1, &from->item1);
>> err |= __get_user(item2, &from->item2);
>> err |= __get_user(item3, &from->item3);
>> err |= __get_user(item4, &from->item4);
>>
>> err |= __put_user(item1, &to->item1);
>> err |= __put_user(item2, &to->item2);
>> err |= __put_user(item3, &to->item3);
>> err |= __put_user(item4, &to->item4);
>>
>> return err;
>> }
>>
>> Before the patch:
>>
>> 00000df0 <set_test_user>:
>> df0: 94 21 ff f0 stwu r1,-16(r1)
>> df4: 39 40 00 00 li r10,0
>> df8: 93 c1 00 08 stw r30,8(r1)
>> dfc: 93 e1 00 0c stw r31,12(r1)
>> e00: 7d 49 53 78 mr r9,r10
>> e04: 80 a3 00 00 lwz r5,0(r3)
>> e08: 38 e3 00 04 addi r7,r3,4
>> e0c: 7d 46 53 78 mr r6,r10
>> e10: a0 e7 00 00 lhz r7,0(r7)
>> e14: 7d 29 33 78 or r9,r9,r6
>> e18: 39 03 00 06 addi r8,r3,6
>> e1c: 7d 46 53 78 mr r6,r10
>> e20: 89 08 00 00 lbz r8,0(r8)
>> e24: 7d 29 33 78 or r9,r9,r6
>> e28: 38 63 00 08 addi r3,r3,8
>> e2c: 7d 46 53 78 mr r6,r10
>> e30: 83 c3 00 00 lwz r30,0(r3)
>> e34: 83 e3 00 04 lwz r31,4(r3)
>> e38: 7d 29 33 78 or r9,r9,r6
>> e3c: 7d 43 53 78 mr r3,r10
>> e40: 90 a4 00 00 stw r5,0(r4)
>> e44: 7d 29 1b 78 or r9,r9,r3
>> e48: 38 c4 00 04 addi r6,r4,4
>> e4c: 7d 43 53 78 mr r3,r10
>> e50: b0 e6 00 00 sth r7,0(r6)
>> e54: 7d 29 1b 78 or r9,r9,r3
>> e58: 38 e4 00 06 addi r7,r4,6
>> e5c: 7d 43 53 78 mr r3,r10
>> e60: 99 07 00 00 stb r8,0(r7)
>> e64: 7d 23 1b 78 or r3,r9,r3
>> e68: 38 84 00 08 addi r4,r4,8
>> e6c: 93 c4 00 00 stw r30,0(r4)
>> e70: 93 e4 00 04 stw r31,4(r4)
>> e74: 7c 63 53 78 or r3,r3,r10
>> e78: 83 c1 00 08 lwz r30,8(r1)
>> e7c: 83 e1 00 0c lwz r31,12(r1)
>> e80: 38 21 00 10 addi r1,r1,16
>> e84: 4e 80 00 20 blr
>>
>> After the patch:
>>
>> 00000dbc <set_test_user>:
>> dbc: 39 40 00 00 li r10,0
>> dc0: 7d 49 53 78 mr r9,r10
>> dc4: 80 03 00 00 lwz r0,0(r3)
>> dc8: 7d 48 53 78 mr r8,r10
>> dcc: a1 63 00 04 lhz r11,4(r3)
>> dd0: 7d 29 43 78 or r9,r9,r8
>> dd4: 7d 48 53 78 mr r8,r10
>> dd8: 88 a3 00 06 lbz r5,6(r3)
>> ddc: 7d 29 43 78 or r9,r9,r8
>> de0: 7d 48 53 78 mr r8,r10
>> de4: 80 c3 00 08 lwz r6,8(r3)
>> de8: 80 e3 00 0c lwz r7,12(r3)
>> dec: 7d 29 43 78 or r9,r9,r8
>> df0: 7d 43 53 78 mr r3,r10
>> df4: 90 04 00 00 stw r0,0(r4)
>> df8: 7d 29 1b 78 or r9,r9,r3
>> dfc: 7d 43 53 78 mr r3,r10
>> e00: b1 64 00 04 sth r11,4(r4)
>> e04: 7d 29 1b 78 or r9,r9,r3
>> e08: 7d 43 53 78 mr r3,r10
>> e0c: 98 a4 00 06 stb r5,6(r4)
>> e10: 7d 23 1b 78 or r3,r9,r3
>> e14: 90 c4 00 08 stw r6,8(r4)
>> e18: 90 e4 00 0c stw r7,12(r4)
>> e1c: 7c 63 53 78 or r3,r3,r10
>> e20: 4e 80 00 20 blr
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> Reviewed-by: Segher Boessenkool <[email protected]>
>> ---
>> v2:
>> - Added <> modifier in __put_user_asm() and __get_user_asm()
>> - Removed %U2 in __put_user_asm2() and __get_user_asm2()
>> - Reworded the commit log
>> ---
>> arch/powerpc/include/asm/uaccess.h | 28 ++++++++++++++--------------
>> 1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index 7c811442b607..9365b59495a2 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -114,7 +114,7 @@ extern long __put_user_bad(void);
>> */
>> #define __put_user_asm(x, addr, err, op) \
>> __asm__ __volatile__( \
>> - "1: " op " %1,0(%2) # put_user\n" \
>> + "1: " op "%U2%X2 %1,%2 # put_user\n" \
>> "2:\n" \
>> ".section .fixup,\"ax\"\n" \
>> "3: li %0,%3\n" \
>> @@ -122,7 +122,7 @@ extern long __put_user_bad(void);
>> ".previous\n" \
>> EX_TABLE(1b, 3b) \
>> : "=r" (err) \
>> - : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
>> + : "r" (x), "m<>" (*addr), "i" (-EFAULT), "0" (err))
>>
>> #ifdef __powerpc64__
>> #define __put_user_asm2(x, ptr, retval) \
>> @@ -130,8 +130,8 @@ extern long __put_user_bad(void);
>> #else /* __powerpc64__ */
>> #define __put_user_asm2(x, addr, err) \
>> __asm__ __volatile__( \
>> - "1: stw %1,0(%2)\n" \
>> - "2: stw %1+1,4(%2)\n" \
>> + "1: stw%X2 %1,%2\n" \
>> + "2: stw%X2 %L1,%L2\n" \
>> "3:\n" \
>> ".section .fixup,\"ax\"\n" \
>> "4: li %0,%3\n" \
>> @@ -140,7 +140,7 @@ extern long __put_user_bad(void);
>> EX_TABLE(1b, 4b) \
>> EX_TABLE(2b, 4b) \
>> : "=r" (err) \
>> - : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
>> + : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err))
>> #endif /* __powerpc64__ */
>>
>> #define __put_user_size_allowed(x, ptr, size, retval) \
>> @@ -260,7 +260,7 @@ extern long __get_user_bad(void);
>>
>> #define __get_user_asm(x, addr, err, op) \
>> __asm__ __volatile__( \
>> - "1: "op" %1,0(%2) # get_user\n" \
>> + "1: "op"%U2%X2 %1, %2 # get_user\n" \
>> "2:\n" \
>> ".section .fixup,\"ax\"\n" \
>> "3: li %0,%3\n" \
>> @@ -269,7 +269,7 @@ extern long __get_user_bad(void);
>> ".previous\n" \
>> EX_TABLE(1b, 3b) \
>> : "=r" (err), "=r" (x) \
>> - : "b" (addr), "i" (-EFAULT), "0" (err))
>> + : "m<>" (*addr), "i" (-EFAULT), "0" (err))
>>
>> #ifdef __powerpc64__
>> #define __get_user_asm2(x, addr, err) \
>> @@ -277,8 +277,8 @@ extern long __get_user_bad(void);
>> #else /* __powerpc64__ */
>> #define __get_user_asm2(x, addr, err) \
>> __asm__ __volatile__( \
>> - "1: lwz %1,0(%2)\n" \
>> - "2: lwz %1+1,4(%2)\n" \
>> + "1: lwz%X2 %1, %2\n" \
>> + "2: lwz%X2 %L1, %L2\n" \
>> "3:\n" \
>> ".section .fixup,\"ax\"\n" \
>> "4: li %0,%3\n" \
>> @@ -289,7 +289,7 @@ extern long __get_user_bad(void);
>> EX_TABLE(1b, 4b) \
>> EX_TABLE(2b, 4b) \
>> : "=r" (err), "=&r" (x) \
>> - : "b" (addr), "i" (-EFAULT), "0" (err))
>> + : "m" (*addr), "i" (-EFAULT), "0" (err))
>> #endif /* __powerpc64__ */
>>
>> #define __get_user_size_allowed(x, ptr, size, retval) \
>> @@ -299,10 +299,10 @@ do { \
>> if (size > sizeof(x)) \
>> (x) = __get_user_bad(); \
>> switch (size) { \
>> - case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
>> - case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
>> - case 4: __get_user_asm(x, ptr, retval, "lwz"); break; \
>> - case 8: __get_user_asm2(x, ptr, retval); break; \
>> + case 1: __get_user_asm(x, (u8 __user *)ptr, retval, "lbz"); break; \
>> + case 2: __get_user_asm(x, (u16 __user *)ptr, retval, "lhz"); break; \
>> + case 4: __get_user_asm(x, (u32 __user *)ptr, retval, "lwz"); break; \
>> + case 8: __get_user_asm2(x, (u64 __user *)ptr, retval); break; \
>> default: (x) = __get_user_bad(); \
>> } \
>> } while (0)
>>
Michael Ellerman <[email protected]> writes:
> Christophe Leroy <[email protected]> writes:
>> Hi Michael,
>>
>> I see this patch is marked as "defered" in patchwork, but I can't see
>> any related discussion. Is it normal ?
>
> Because it uses the "m<>" constraint which didn't work on GCC 4.6.
>
> https://github.com/linuxppc/issues/issues/297
>
> So we should be able to pick it up for v5.9 hopefully.
It seems to break the build with the kernel.org 4.9.4 compiler and
corenet64_smp_defconfig:
+ make -s CC=powerpc64-linux-gnu-gcc -j 160
In file included from /linux/include/linux/uaccess.h:11:0,
from /linux/include/linux/sched/task.h:11,
from /linux/include/linux/sched/signal.h:9,
from /linux/include/linux/rcuwait.h:6,
from /linux/include/linux/percpu-rwsem.h:7,
from /linux/include/linux/fs.h:33,
from /linux/include/linux/huge_mm.h:8,
from /linux/include/linux/mm.h:675,
from /linux/arch/powerpc/kernel/signal_32.c:17:
/linux/arch/powerpc/kernel/signal_32.c: In function 'save_user_regs.isra.14.constprop':
/linux/arch/powerpc/include/asm/uaccess.h:161:2: error: 'asm' operand has impossible constraints
__asm__ __volatile__( \
^
/linux/arch/powerpc/include/asm/uaccess.h:197:12: note: in expansion of macro '__put_user_asm'
case 4: __put_user_asm(x, ptr, retval, "stw"); break; \
^
/linux/arch/powerpc/include/asm/uaccess.h:206:2: note: in expansion of macro '__put_user_size_allowed'
__put_user_size_allowed(x, ptr, size, retval); \
^
/linux/arch/powerpc/include/asm/uaccess.h:220:2: note: in expansion of macro '__put_user_size'
__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
^
/linux/arch/powerpc/include/asm/uaccess.h:96:2: note: in expansion of macro '__put_user_nocheck'
__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
^
/linux/arch/powerpc/kernel/signal_32.c:120:7: note: in expansion of macro '__put_user'
if (__put_user((unsigned int)gregs[i], &frame->mc_gregs[i]))
^
/linux/scripts/Makefile.build:280: recipe for target 'arch/powerpc/kernel/signal_32.o' failed
make[3]: *** [arch/powerpc/kernel/signal_32.o] Error 1
make[3]: *** Waiting for unfinished jobs....
In file included from /linux/include/linux/uaccess.h:11:0,
from /linux/include/linux/sched/task.h:11,
from /linux/include/linux/sched/signal.h:9,
from /linux/include/linux/rcuwait.h:6,
from /linux/include/linux/percpu-rwsem.h:7,
from /linux/include/linux/fs.h:33,
from /linux/include/linux/huge_mm.h:8,
from /linux/include/linux/mm.h:675,
from /linux/arch/powerpc/kernel/signal_64.c:12:
/linux/arch/powerpc/kernel/signal_64.c: In function '__se_sys_swapcontext':
/linux/arch/powerpc/include/asm/uaccess.h:319:2: error: 'asm' operand has impossible constraints
__asm__ __volatile__( \
^
/linux/arch/powerpc/include/asm/uaccess.h:359:10: note: in expansion of macro '__get_user_asm'
case 1: __get_user_asm(x, (u8 __user *)ptr, retval, "lbz"); break; \
^
/linux/arch/powerpc/include/asm/uaccess.h:370:2: note: in expansion of macro '__get_user_size_allowed'
__get_user_size_allowed(x, ptr, size, retval); \
^
/linux/arch/powerpc/include/asm/uaccess.h:393:3: note: in expansion of macro '__get_user_size'
__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
^
/linux/arch/powerpc/include/asm/uaccess.h:94:2: note: in expansion of macro '__get_user_nocheck'
__get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)
^
/linux/arch/powerpc/kernel/signal_64.c:672:9: note: in expansion of macro '__get_user'
|| __get_user(tmp, (u8 __user *) new_ctx + ctx_size - 1))
^
/linux/scripts/Makefile.build:280: recipe for target 'arch/powerpc/kernel/signal_64.o' failed
make[3]: *** [arch/powerpc/kernel/signal_64.o] Error 1
/linux/scripts/Makefile.build:497: recipe for target 'arch/powerpc/kernel' failed
make[2]: *** [arch/powerpc/kernel] Error 2
/linux/Makefile:1756: recipe for target 'arch/powerpc' failed
make[1]: *** [arch/powerpc] Error 2
Makefile:185: recipe for target '__sub-make' failed
make: *** [__sub-make] Error 2
cheers
Le 30/06/2020 à 03:19, Michael Ellerman a écrit :
> Michael Ellerman <[email protected]> writes:
>> Christophe Leroy <[email protected]> writes:
>>> Hi Michael,
>>>
>>> I see this patch is marked as "defered" in patchwork, but I can't see
>>> any related discussion. Is it normal ?
>>
>> Because it uses the "m<>" constraint which didn't work on GCC 4.6.
>>
>> https://github.com/linuxppc/issues/issues/297
>>
>> So we should be able to pick it up for v5.9 hopefully.
>
> It seems to break the build with the kernel.org 4.9.4 compiler and
> corenet64_smp_defconfig:
Looks like 4.9.4 doesn't accept "m<>" constraint either.
Changing it to "m" make it build.
Christophe
>
> + make -s CC=powerpc64-linux-gnu-gcc -j 160
> In file included from /linux/include/linux/uaccess.h:11:0,
> from /linux/include/linux/sched/task.h:11,
> from /linux/include/linux/sched/signal.h:9,
> from /linux/include/linux/rcuwait.h:6,
> from /linux/include/linux/percpu-rwsem.h:7,
> from /linux/include/linux/fs.h:33,
> from /linux/include/linux/huge_mm.h:8,
> from /linux/include/linux/mm.h:675,
> from /linux/arch/powerpc/kernel/signal_32.c:17:
> /linux/arch/powerpc/kernel/signal_32.c: In function 'save_user_regs.isra.14.constprop':
> /linux/arch/powerpc/include/asm/uaccess.h:161:2: error: 'asm' operand has impossible constraints
> __asm__ __volatile__( \
> ^
> /linux/arch/powerpc/include/asm/uaccess.h:197:12: note: in expansion of macro '__put_user_asm'
> case 4: __put_user_asm(x, ptr, retval, "stw"); break; \
> ^
> /linux/arch/powerpc/include/asm/uaccess.h:206:2: note: in expansion of macro '__put_user_size_allowed'
> __put_user_size_allowed(x, ptr, size, retval); \
> ^
> /linux/arch/powerpc/include/asm/uaccess.h:220:2: note: in expansion of macro '__put_user_size'
> __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> ^
> /linux/arch/powerpc/include/asm/uaccess.h:96:2: note: in expansion of macro '__put_user_nocheck'
> __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
> ^
> /linux/arch/powerpc/kernel/signal_32.c:120:7: note: in expansion of macro '__put_user'
> if (__put_user((unsigned int)gregs[i], &frame->mc_gregs[i]))
> ^
> /linux/scripts/Makefile.build:280: recipe for target 'arch/powerpc/kernel/signal_32.o' failed
> make[3]: *** [arch/powerpc/kernel/signal_32.o] Error 1
> make[3]: *** Waiting for unfinished jobs....
> In file included from /linux/include/linux/uaccess.h:11:0,
> from /linux/include/linux/sched/task.h:11,
> from /linux/include/linux/sched/signal.h:9,
> from /linux/include/linux/rcuwait.h:6,
> from /linux/include/linux/percpu-rwsem.h:7,
> from /linux/include/linux/fs.h:33,
> from /linux/include/linux/huge_mm.h:8,
> from /linux/include/linux/mm.h:675,
> from /linux/arch/powerpc/kernel/signal_64.c:12:
> /linux/arch/powerpc/kernel/signal_64.c: In function '__se_sys_swapcontext':
> /linux/arch/powerpc/include/asm/uaccess.h:319:2: error: 'asm' operand has impossible constraints
> __asm__ __volatile__( \
> ^
> /linux/arch/powerpc/include/asm/uaccess.h:359:10: note: in expansion of macro '__get_user_asm'
> case 1: __get_user_asm(x, (u8 __user *)ptr, retval, "lbz"); break; \
> ^
> /linux/arch/powerpc/include/asm/uaccess.h:370:2: note: in expansion of macro '__get_user_size_allowed'
> __get_user_size_allowed(x, ptr, size, retval); \
> ^
> /linux/arch/powerpc/include/asm/uaccess.h:393:3: note: in expansion of macro '__get_user_size'
> __get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
> ^
> /linux/arch/powerpc/include/asm/uaccess.h:94:2: note: in expansion of macro '__get_user_nocheck'
> __get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)
> ^
> /linux/arch/powerpc/kernel/signal_64.c:672:9: note: in expansion of macro '__get_user'
> || __get_user(tmp, (u8 __user *) new_ctx + ctx_size - 1))
> ^
> /linux/scripts/Makefile.build:280: recipe for target 'arch/powerpc/kernel/signal_64.o' failed
> make[3]: *** [arch/powerpc/kernel/signal_64.o] Error 1
> /linux/scripts/Makefile.build:497: recipe for target 'arch/powerpc/kernel' failed
> make[2]: *** [arch/powerpc/kernel] Error 2
> /linux/Makefile:1756: recipe for target 'arch/powerpc' failed
> make[1]: *** [arch/powerpc] Error 2
> Makefile:185: recipe for target '__sub-make' failed
> make: *** [__sub-make] Error 2
>
>
> cheers
>
On Tue, Jun 30, 2020 at 04:55:05PM +0200, Christophe Leroy wrote:
> Le 30/06/2020 ? 03:19, Michael Ellerman a ?crit?:
> >Michael Ellerman <[email protected]> writes:
> >>Because it uses the "m<>" constraint which didn't work on GCC 4.6.
> >>
> >>https://github.com/linuxppc/issues/issues/297
> >>
> >>So we should be able to pick it up for v5.9 hopefully.
> >
> >It seems to break the build with the kernel.org 4.9.4 compiler and
> >corenet64_smp_defconfig:
>
> Looks like 4.9.4 doesn't accept "m<>" constraint either.
The evidence contradicts this assertion.
> Changing it to "m" make it build.
But that just means something else is wrong.
> >+ make -s CC=powerpc64-linux-gnu-gcc -j 160
> >In file included from /linux/include/linux/uaccess.h:11:0,
> > from /linux/include/linux/sched/task.h:11,
> > from /linux/include/linux/sched/signal.h:9,
> > from /linux/include/linux/rcuwait.h:6,
> > from /linux/include/linux/percpu-rwsem.h:7,
> > from /linux/include/linux/fs.h:33,
> > from /linux/include/linux/huge_mm.h:8,
> > from /linux/include/linux/mm.h:675,
> > from /linux/arch/powerpc/kernel/signal_32.c:17:
> >/linux/arch/powerpc/kernel/signal_32.c: In function
> >'save_user_regs.isra.14.constprop':
> >/linux/arch/powerpc/include/asm/uaccess.h:161:2: error: 'asm' operand has
> >impossible constraints
> > __asm__ __volatile__( \
> > ^
> >/linux/arch/powerpc/include/asm/uaccess.h:197:12: note: in expansion of
> >macro '__put_user_asm'
> > case 4: __put_user_asm(x, ptr, retval, "stw"); break; \
> > ^
> >/linux/arch/powerpc/include/asm/uaccess.h:206:2: note: in expansion of
> >macro '__put_user_size_allowed'
> > __put_user_size_allowed(x, ptr, size, retval); \
> > ^
> >/linux/arch/powerpc/include/asm/uaccess.h:220:2: note: in expansion of
> >macro '__put_user_size'
> > __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> > ^
> >/linux/arch/powerpc/include/asm/uaccess.h:96:2: note: in expansion of
> >macro '__put_user_nocheck'
> > __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
> > ^
> >/linux/arch/powerpc/kernel/signal_32.c:120:7: note: in expansion of macro
> >'__put_user'
> > if (__put_user((unsigned int)gregs[i], &frame->mc_gregs[i]))
> > ^
Can we see what that was after the macro jungle? Like, the actual
preprocessed code?
Also, what GCC version *does* work on this?
Segher
On 06/30/2020 04:33 PM, Segher Boessenkool wrote:
> On Tue, Jun 30, 2020 at 04:55:05PM +0200, Christophe Leroy wrote:
>> Le 30/06/2020 à 03:19, Michael Ellerman a écrit :
>>> Michael Ellerman <[email protected]> writes:
>>>> Because it uses the "m<>" constraint which didn't work on GCC 4.6.
>>>>
>>>> https://github.com/linuxppc/issues/issues/297
>>>>
>>>> So we should be able to pick it up for v5.9 hopefully.
>>>
>>> It seems to break the build with the kernel.org 4.9.4 compiler and
>>> corenet64_smp_defconfig:
>>
>> Looks like 4.9.4 doesn't accept "m<>" constraint either.
>
> The evidence contradicts this assertion.
>
>> Changing it to "m" make it build.
>
> But that just means something else is wrong.
>
>>> + make -s CC=powerpc64-linux-gnu-gcc -j 160
>>> In file included from /linux/include/linux/uaccess.h:11:0,
>>> from /linux/include/linux/sched/task.h:11,
>>> from /linux/include/linux/sched/signal.h:9,
>>> from /linux/include/linux/rcuwait.h:6,
>>> from /linux/include/linux/percpu-rwsem.h:7,
>>> from /linux/include/linux/fs.h:33,
>>> from /linux/include/linux/huge_mm.h:8,
>>> from /linux/include/linux/mm.h:675,
>>> from /linux/arch/powerpc/kernel/signal_32.c:17:
>>> /linux/arch/powerpc/kernel/signal_32.c: In function
>>> 'save_user_regs.isra.14.constprop':
>>> /linux/arch/powerpc/include/asm/uaccess.h:161:2: error: 'asm' operand has
>>> impossible constraints
>>> __asm__ __volatile__( \
>>> ^
>>> /linux/arch/powerpc/include/asm/uaccess.h:197:12: note: in expansion of
>>> macro '__put_user_asm'
>>> case 4: __put_user_asm(x, ptr, retval, "stw"); break; \
>>> ^
>>> /linux/arch/powerpc/include/asm/uaccess.h:206:2: note: in expansion of
>>> macro '__put_user_size_allowed'
>>> __put_user_size_allowed(x, ptr, size, retval); \
>>> ^
>>> /linux/arch/powerpc/include/asm/uaccess.h:220:2: note: in expansion of
>>> macro '__put_user_size'
>>> __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
>>> ^
>>> /linux/arch/powerpc/include/asm/uaccess.h:96:2: note: in expansion of
>>> macro '__put_user_nocheck'
>>> __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>>> ^
>>> /linux/arch/powerpc/kernel/signal_32.c:120:7: note: in expansion of macro
>>> '__put_user'
>>> if (__put_user((unsigned int)gregs[i], &frame->mc_gregs[i]))
>>> ^
>
> Can we see what that was after the macro jungle? Like, the actual
> preprocessed code?
>
Sorry for previous misunderstanding
Here is the code:
#define __put_user_asm(x, addr, err, op) \
__asm__ __volatile__( \
"1: " op "%U2%X2 %1,%2 # put_user\n" \
"2:\n" \
".section .fixup,\"ax\"\n" \
"3: li %0,%3\n" \
" b 2b\n" \
".previous\n" \
EX_TABLE(1b, 3b) \
: "=r" (err) \
: "r" (x), "m<>" (*addr), "i" (-EFAULT), "0" (err))
Christophe
Hi again,
Thanks for your work so far!
On Tue, Jun 30, 2020 at 06:53:39PM +0000, Christophe Leroy wrote:
> On 06/30/2020 04:33 PM, Segher Boessenkool wrote:
> >>>+ make -s CC=powerpc64-linux-gnu-gcc -j 160
> >>>In file included from /linux/include/linux/uaccess.h:11:0,
> >>> from /linux/include/linux/sched/task.h:11,
> >>> from /linux/include/linux/sched/signal.h:9,
> >>> from /linux/include/linux/rcuwait.h:6,
> >>> from /linux/include/linux/percpu-rwsem.h:7,
> >>> from /linux/include/linux/fs.h:33,
> >>> from /linux/include/linux/huge_mm.h:8,
> >>> from /linux/include/linux/mm.h:675,
> >>> from /linux/arch/powerpc/kernel/signal_32.c:17:
> >>>/linux/arch/powerpc/kernel/signal_32.c: In function
> >>>'save_user_regs.isra.14.constprop':
> >>>/linux/arch/powerpc/include/asm/uaccess.h:161:2: error: 'asm' operand has
> >>>impossible constraints
> >>> __asm__ __volatile__( \
> >>> ^
> >>>/linux/arch/powerpc/include/asm/uaccess.h:197:12: note: in expansion of
> >>>macro '__put_user_asm'
> >>> case 4: __put_user_asm(x, ptr, retval, "stw"); break; \
> >>> ^
> >>>/linux/arch/powerpc/include/asm/uaccess.h:206:2: note: in expansion of
> >>>macro '__put_user_size_allowed'
> >>> __put_user_size_allowed(x, ptr, size, retval); \
> >>> ^
> >>>/linux/arch/powerpc/include/asm/uaccess.h:220:2: note: in expansion of
> >>>macro '__put_user_size'
> >>> __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> >>> ^
> >>>/linux/arch/powerpc/include/asm/uaccess.h:96:2: note: in expansion of
> >>>macro '__put_user_nocheck'
> >>> __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
> >>> ^
> >>>/linux/arch/powerpc/kernel/signal_32.c:120:7: note: in expansion of macro
> >>>'__put_user'
> >>> if (__put_user((unsigned int)gregs[i], &frame->mc_gregs[i]))
> >>> ^
> >
> >Can we see what that was after the macro jungle? Like, the actual
> >preprocessed code?
>
> Sorry for previous misunderstanding
>
> Here is the code:
>
> #define __put_user_asm(x, addr, err, op) \
> __asm__ __volatile__( \
> "1: " op "%U2%X2 %1,%2 # put_user\n" \
> "2:\n" \
> ".section .fixup,\"ax\"\n" \
> "3: li %0,%3\n" \
> " b 2b\n" \
> ".previous\n" \
> EX_TABLE(1b, 3b) \
> : "=r" (err) \
> : "r" (x), "m<>" (*addr), "i" (-EFAULT), "0" (err))
Yeah I don't see it. I'll have to look at compiler debug dumps, but I
don't have any working 4.9 around, and I cannot reproduce this with
either older or newer compilers.
It is complainig that constrain_operands just does not work *at all* on
this "m<>" constraint apparently, which doesn't make much sense.
I'll try later when I have more time, sorry :-/
Segher
Le 30/06/2020 à 23:18, Segher Boessenkool a écrit :
> Hi again,
>
> Thanks for your work so far!
>
> On Tue, Jun 30, 2020 at 06:53:39PM +0000, Christophe Leroy wrote:
>> On 06/30/2020 04:33 PM, Segher Boessenkool wrote:
>>>>> + make -s CC=powerpc64-linux-gnu-gcc -j 160
>>>>> In file included from /linux/include/linux/uaccess.h:11:0,
>>>>> from /linux/include/linux/sched/task.h:11,
>>>>> from /linux/include/linux/sched/signal.h:9,
>>>>> from /linux/include/linux/rcuwait.h:6,
>>>>> from /linux/include/linux/percpu-rwsem.h:7,
>>>>> from /linux/include/linux/fs.h:33,
>>>>> from /linux/include/linux/huge_mm.h:8,
>>>>> from /linux/include/linux/mm.h:675,
>>>>> from /linux/arch/powerpc/kernel/signal_32.c:17:
>>>>> /linux/arch/powerpc/kernel/signal_32.c: In function
>>>>> 'save_user_regs.isra.14.constprop':
>>>>> /linux/arch/powerpc/include/asm/uaccess.h:161:2: error: 'asm' operand has
>>>>> impossible constraints
>>>>> __asm__ __volatile__( \
>>>>> ^
>>>>> /linux/arch/powerpc/include/asm/uaccess.h:197:12: note: in expansion of
>>>>> macro '__put_user_asm'
>>>>> case 4: __put_user_asm(x, ptr, retval, "stw"); break; \
>>>>> ^
>>>>> /linux/arch/powerpc/include/asm/uaccess.h:206:2: note: in expansion of
>>>>> macro '__put_user_size_allowed'
>>>>> __put_user_size_allowed(x, ptr, size, retval); \
>>>>> ^
>>>>> /linux/arch/powerpc/include/asm/uaccess.h:220:2: note: in expansion of
>>>>> macro '__put_user_size'
>>>>> __put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
>>>>> ^
>>>>> /linux/arch/powerpc/include/asm/uaccess.h:96:2: note: in expansion of
>>>>> macro '__put_user_nocheck'
>>>>> __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
>>>>> ^
>>>>> /linux/arch/powerpc/kernel/signal_32.c:120:7: note: in expansion of macro
>>>>> '__put_user'
>>>>> if (__put_user((unsigned int)gregs[i], &frame->mc_gregs[i]))
>>>>> ^
>>>
>>> Can we see what that was after the macro jungle? Like, the actual
>>> preprocessed code?
>>
>> Sorry for previous misunderstanding
>>
>> Here is the code:
>>
>> #define __put_user_asm(x, addr, err, op) \
>> __asm__ __volatile__( \
>> "1: " op "%U2%X2 %1,%2 # put_user\n" \
>> "2:\n" \
>> ".section .fixup,\"ax\"\n" \
>> "3: li %0,%3\n" \
>> " b 2b\n" \
>> ".previous\n" \
>> EX_TABLE(1b, 3b) \
>> : "=r" (err) \
>> : "r" (x), "m<>" (*addr), "i" (-EFAULT), "0" (err))
>
> Yeah I don't see it. I'll have to look at compiler debug dumps, but I
> don't have any working 4.9 around, and I cannot reproduce this with
> either older or newer compilers.
I reproduced it with 4.8.5
>
> It is complainig that constrain_operands just does not work *at all* on
> this "m<>" constraint apparently, which doesn't make much sense.
>
Here is a small reproducer:
#include <linux/elf.h>
#include <linux/ptrace.h>
#include <linux/uaccess.h>
struct mcontext {
elf_gregset_t32 mc_gregs;
elf_fpregset_t mc_fregs;
unsigned int mc_pad[2];
elf_vrregset_t32 mc_vregs __attribute__((__aligned__(16)));
elf_vsrreghalf_t32 mc_vsregs __attribute__((__aligned__(16)));
};
int save_general_regs(struct pt_regs *regs, struct mcontext __user *frame)
{
elf_greg_t64 *gregs = (elf_greg_t64 *)regs;
int i;
for (i = 0; i <= PT_RESULT; i ++) {
if (i == 14)
i = 32;
if (__put_user((unsigned int)gregs[i], &frame->mc_gregs[i]))
return -EFAULT;
}
return 0;
}
If you remove the "if i == 14 ..." you get no failure.
Preprocessor result:
int save_general_regs(struct pt_regs *regs, struct mcontext *frame)
{
elf_greg_t64 *gregs = (elf_greg_t64 *)regs;
int i;
for (i = 0; i <= 43; i ++) {
if (i == 14)
i = 32;
if (({ long __pu_err; __typeof__(*((&frame->mc_gregs[i]))) *__pu_addr
= ((&frame->mc_gregs[i])); __typeof__(*((&frame->mc_gregs[i]))) __pu_val
= ((__typeof__(*(&frame->mc_gregs[i])))((unsigned int)gregs[i]));
__typeof__(sizeof(*(&frame->mc_gregs[i]))) __pu_size =
(sizeof(*(&frame->mc_gregs[i]))); if (!(((unsigned long)__pu_addr) >=
0x8000000000000000ul)) might_fault(); (void)0; do {
allow_write_to_user(__pu_addr, __pu_size); do { __pu_err = 0; switch
(__pu_size) { case 1: __asm__ __volatile__( "1: " "stb" "%U2%X2 %1,%2 #
put_user\n" "2:\n" ".section .fixup,\"ax\"\n" "3: li %0,%3\n" " b 2b\n"
".previous\n" ".section __ex_table,\"a\";" " " ".balign 4;" " " ".long
(1b) - . ;" " " ".long (3b) - . ;" " " ".previous" " " : "=r" (__pu_err)
: "r" (__pu_val), "m<>" (*__pu_addr), "i" (-14), "0" (__pu_err)); break;
case 2: __asm__ __volatile__( "1: " "sth" "%U2%X2 %1,%2 # put_user\n"
"2:\n" ".section .fixup,\"ax\"\n" "3: li %0,%3\n" " b 2b\n"
".previous\n" ".section __ex_table,\"a\";" " " ".balign 4;" " " ".long
(1b) - . ;" " " ".long (3b) - . ;" " " ".previous" " " : "=r" (__pu_err)
: "r" (__pu_val), "m<>" (*__pu_addr), "i" (-14), "0" (__pu_err)); break;
case 4: __asm__ __volatile__( "1: " "stw" "%U2%X2 %1,%2 # put_user\n"
"2:\n" ".section .fixup,\"ax\"\n" "3: li %0,%3\n" " b 2b\n"
".previous\n" ".section __ex_table,\"a\";" " " ".balign 4;" " " ".long
(1b) - . ;" " " ".long (3b) - . ;" " " ".previous" " " : "=r" (__pu_err)
: "r" (__pu_val), "m<>" (*__pu_addr), "i" (-14), "0" (__pu_err)); break;
case 8: __asm__ __volatile__( "1: " "std" "%U2%X2 %1,%2 # put_user\n"
"2:\n" ".section .fixup,\"ax\"\n" "3: li %0,%3\n" " b 2b\n"
".previous\n" ".section __ex_table,\"a\";" " " ".balign 4;" " " ".long
(1b) - . ;" " " ".long (3b) - . ;" " " ".previous" " " : "=r" (__pu_err)
: "r" (__pu_val), "m<>" (*__pu_addr), "i" (-14), "0" (__pu_err)); break;
default: __put_user_bad(); } } while (0);
prevent_write_to_user(__pu_addr, __pu_size); } while (0); __pu_err; }))
return -14;
}
return 0;
}
Christophe
Le 30/06/2020 à 03:19, Michael Ellerman a écrit :
> Michael Ellerman <[email protected]> writes:
>> Christophe Leroy <[email protected]> writes:
>>> Hi Michael,
>>>
>>> I see this patch is marked as "defered" in patchwork, but I can't see
>>> any related discussion. Is it normal ?
>>
>> Because it uses the "m<>" constraint which didn't work on GCC 4.6.
>>
>> https://github.com/linuxppc/issues/issues/297
>>
>> So we should be able to pick it up for v5.9 hopefully.
>
> It seems to break the build with the kernel.org 4.9.4 compiler and
> corenet64_smp_defconfig:
Most likely a GCC bug ?
It seems the problem vanishes with patch
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/173de3b659fa3a5f126a0eb170522cccd909950f.1594125164.git.christophe.leroy@csgroup.eu/
Christophe
Le 07/07/2020 à 14:44, Christophe Leroy a écrit :
>
>
> Le 30/06/2020 à 03:19, Michael Ellerman a écrit :
>> Michael Ellerman <[email protected]> writes:
>>> Christophe Leroy <[email protected]> writes:
>>>> Hi Michael,
>>>>
>>>> I see this patch is marked as "defered" in patchwork, but I can't see
>>>> any related discussion. Is it normal ?
>>>
>>> Because it uses the "m<>" constraint which didn't work on GCC 4.6.
>>>
>>> https://github.com/linuxppc/issues/issues/297
>>>
>>> So we should be able to pick it up for v5.9 hopefully.
>>
>> It seems to break the build with the kernel.org 4.9.4 compiler and
>> corenet64_smp_defconfig:
>
> Most likely a GCC bug ?
>
> It seems the problem vanishes with patch
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/173de3b659fa3a5f126a0eb170522cccd909950f.1594125164.git.christophe.leroy@csgroup.eu/
>
Same kind of issue in signal_64.c now.
The following patch fixes it:
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/810bd8840ef990a200f58c9dea9abe767ca02a3a.1594146723.git.christophe.leroy@csgroup.eu/
Christophe
Le 07/07/2020 à 21:02, Christophe Leroy a écrit :
>
>
> Le 07/07/2020 à 14:44, Christophe Leroy a écrit :
>>
>>
>> Le 30/06/2020 à 03:19, Michael Ellerman a écrit :
>>> Michael Ellerman <[email protected]> writes:
>>>> Christophe Leroy <[email protected]> writes:
>>>>> Hi Michael,
>>>>>
>>>>> I see this patch is marked as "defered" in patchwork, but I can't see
>>>>> any related discussion. Is it normal ?
>>>>
>>>> Because it uses the "m<>" constraint which didn't work on GCC 4.6.
>>>>
>>>> https://github.com/linuxppc/issues/issues/297
>>>>
>>>> So we should be able to pick it up for v5.9 hopefully.
>>>
>>> It seems to break the build with the kernel.org 4.9.4 compiler and
>>> corenet64_smp_defconfig:
>>
>> Most likely a GCC bug ?
>>
>> It seems the problem vanishes with patch
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/173de3b659fa3a5f126a0eb170522cccd909950f.1594125164.git.christophe.leroy@csgroup.eu/
>>
>
> Same kind of issue in signal_64.c now.
>
> The following patch fixes it:
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/810bd8840ef990a200f58c9dea9abe767ca02a3a.1594146723.git.christophe.leroy@csgroup.eu/
>
>
This time I confirm, with the two above mentioned patches, it builds OK
with 4.9, see
http://kisskb.ellerman.id.au/kisskb/head/810bd8840ef990a200f58c9dea9abe767ca02a3a/
Christophe
Le 08/07/2020 à 06:49, Christophe Leroy a écrit :
>
>
> Le 07/07/2020 à 21:02, Christophe Leroy a écrit :
>>
>>
>> Le 07/07/2020 à 14:44, Christophe Leroy a écrit :
>>>
>>>
>>> Le 30/06/2020 à 03:19, Michael Ellerman a écrit :
>>>> Michael Ellerman <[email protected]> writes:
>>>>> Christophe Leroy <[email protected]> writes:
>>>>>> Hi Michael,
>>>>>>
>>>>>> I see this patch is marked as "defered" in patchwork, but I can't see
>>>>>> any related discussion. Is it normal ?
>>>>>
>>>>> Because it uses the "m<>" constraint which didn't work on GCC 4.6.
>>>>>
>>>>> https://github.com/linuxppc/issues/issues/297
>>>>>
>>>>> So we should be able to pick it up for v5.9 hopefully.
>>>>
>>>> It seems to break the build with the kernel.org 4.9.4 compiler and
>>>> corenet64_smp_defconfig:
>>>
>>> Most likely a GCC bug ?
>>>
>>> It seems the problem vanishes with patch
>>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/173de3b659fa3a5f126a0eb170522cccd909950f.1594125164.git.christophe.leroy@csgroup.eu/
>>>
>>
>> Same kind of issue in signal_64.c now.
>>
>> The following patch fixes it:
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/810bd8840ef990a200f58c9dea9abe767ca02a3a.1594146723.git.christophe.leroy@csgroup.eu/
>>
>>
>
> This time I confirm, with the two above mentioned patches, it builds OK
> with 4.9, see
> http://kisskb.ellerman.id.au/kisskb/head/810bd8840ef990a200f58c9dea9abe767ca02a3a/
>
>
I see you've merged those patches that make the issue disappear, yet not
this patch yet. I guess you are still a bit chilly about it, so I split
it in two parts for you to safely take patch 1 as soon as possible while
handling the "m<>" constraint subject more carefully via
https://github.com/linuxppc/issues/issues/297 in a later stage.
Anyway, it seems that GCC doesn't make much use of the "m<>" and the
pre-update form. Most of the benefit of flexible addressing seems to be
achieved with patch 1 ie without the "m<>" constraint and update form.
Christophe
On Wed, Aug 12, 2020 at 02:32:51PM +0200, Christophe Leroy wrote:
> Anyway, it seems that GCC doesn't make much use of the "m<>" and the
> pre-update form.
GCC does not use update form outside of inner loops much. Did you
expect anything else?
> Most of the benefit of flexible addressing seems to be
> achieved with patch 1 ie without the "m<>" constraint and update form.
Yes.
Segher