[2/6] arm64: uaccess: Use named asm operands for __in_range
diff mbox series

Message ID 20200311180416.6509-3-richard.henderson@linaro.org
State New
Headers show
Series
  • arm64: gcc asm flag outputs
Related show

Commit Message

Richard Henderson March 11, 2020, 6:04 p.m. UTC
With zero change of behavior, use %[] syntax for the asm
operands instead of numbered operands.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 arch/arm64/include/asm/uaccess.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Comments

Robin Murphy March 11, 2020, 7:08 p.m. UTC | #1
On 11/03/2020 6:04 pm, Richard Henderson wrote:
> With zero change of behavior, use %[] syntax for the asm
> operands instead of numbered operands.

For any particular reason? It's very deliberate that a mere 4 
instructions have over twice as many lines of comments here, and IMO 
making the code more verbose only serves to distract from the 
explanation of what's actually happening. In particular, the value 
represented by %0 (the conceptual X') never corresponds to the variable 
"addr", so naming it "addr" provides the opposite of clarity.

Robin.

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   arch/arm64/include/asm/uaccess.h | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 32fc8061aa76..ceb1d79eab09 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -75,19 +75,21 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
>   	asm volatile(
>   	// A + B <= C + 1 for all A,B,C, in four easy steps:
>   	// 1: X = A + B; X' = X % 2^64
> -	"	adds	%0, %3, %2\n"
> +	"	adds	%[addr], %[addr], %[size]\n"
>   	// 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4
> -	"	csel	%1, xzr, %1, hi\n"
> +	"	csel	%[limit], xzr, %[limit], hi\n"
>   	// 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X'
>   	//    to compensate for the carry flag being set in step 4. For
>   	//    X > 2^64, X' merely has to remain nonzero, which it does.
> -	"	csinv	%0, %0, xzr, cc\n"
> +	"	csinv	%[addr], %[addr], xzr, cc\n"
>   	// 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1
>   	//    comes from the carry in being clear. Otherwise, we are
>   	//    testing X' - C == 0, subject to the previous adjustments.
> -	"	sbcs	xzr, %0, %1\n"
> -	"	cset	%0, ls\n"
> -	: "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc");
> +	"	sbcs	xzr, %[addr], %[limit]\n"
> +	"       cset    %[addr], ls\n"
> +	: [addr] "=&r" (ret), [limit] "+r" (limit)
> +	: [size] "Ir" (size), "0" (addr)
> +	: "cc");
>   
>   	return ret;
>   }
>
Richard Henderson March 11, 2020, 9:48 p.m. UTC | #2
On 3/11/20 12:08 PM, Robin Murphy wrote:
> On 11/03/2020 6:04 pm, Richard Henderson wrote:
>> With zero change of behavior, use %[] syntax for the asm
>> operands instead of numbered operands.
> 
> For any particular reason?

When we get to the third patch and add CC_SET(le), all of the operand numbers
will change, and I found that more confusing than not.

> In
> particular, the value represented by %0 (the conceptual X') never corresponds
> to the variable "addr", so naming it "addr" provides the opposite of clarity.

Would you simply prefer a different name for the operand?


r~


>> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
>> index 32fc8061aa76..ceb1d79eab09 100644
>> --- a/arch/arm64/include/asm/uaccess.h
>> +++ b/arch/arm64/include/asm/uaccess.h
>> @@ -75,19 +75,21 @@ static inline unsigned long __range_ok(const void __user
>> *addr, unsigned long si
>>       asm volatile(
>>       // A + B <= C + 1 for all A,B,C, in four easy steps:
>>       // 1: X = A + B; X' = X % 2^64
>> -    "    adds    %0, %3, %2\n"
>> +    "    adds    %[addr], %[addr], %[size]\n"
>>       // 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4
>> -    "    csel    %1, xzr, %1, hi\n"
>> +    "    csel    %[limit], xzr, %[limit], hi\n"
>>       // 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X'
>>       //    to compensate for the carry flag being set in step 4. For
>>       //    X > 2^64, X' merely has to remain nonzero, which it does.
>> -    "    csinv    %0, %0, xzr, cc\n"
>> +    "    csinv    %[addr], %[addr], xzr, cc\n"
>>       // 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1
>>       //    comes from the carry in being clear. Otherwise, we are
>>       //    testing X' - C == 0, subject to the previous adjustments.
>> -    "    sbcs    xzr, %0, %1\n"
>> -    "    cset    %0, ls\n"
>> -    : "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc");
>> +    "    sbcs    xzr, %[addr], %[limit]\n"
>> +    "       cset    %[addr], ls\n"
>> +    : [addr] "=&r" (ret), [limit] "+r" (limit)
>> +    : [size] "Ir" (size), "0" (addr)
>> +    : "cc");
>>         return ret;
>>   }
Robin Murphy March 13, 2020, 4:14 p.m. UTC | #3
On 2020-03-11 9:48 pm, Richard Henderson wrote:
> On 3/11/20 12:08 PM, Robin Murphy wrote:
>> On 11/03/2020 6:04 pm, Richard Henderson wrote:
>>> With zero change of behavior, use %[] syntax for the asm
>>> operands instead of numbered operands.
>>
>> For any particular reason?
> 
> When we get to the third patch and add CC_SET(le), all of the operand numbers
> will change, and I found that more confusing than not.
> 
>> In
>> particular, the value represented by %0 (the conceptual X') never corresponds
>> to the variable "addr", so naming it "addr" provides the opposite of clarity.
> 
> Would you simply prefer a different name for the operand?

If we were to go down this route, I think it might actually make sense 
to split it up further to separate the "private scratch register" and 
"return value" concerns.

Robin.

> 
> 
> r~
> 
> 
>>> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
>>> index 32fc8061aa76..ceb1d79eab09 100644
>>> --- a/arch/arm64/include/asm/uaccess.h
>>> +++ b/arch/arm64/include/asm/uaccess.h
>>> @@ -75,19 +75,21 @@ static inline unsigned long __range_ok(const void __user
>>> *addr, unsigned long si
>>>        asm volatile(
>>>        // A + B <= C + 1 for all A,B,C, in four easy steps:
>>>        // 1: X = A + B; X' = X % 2^64
>>> -    "    adds    %0, %3, %2\n"
>>> +    "    adds    %[addr], %[addr], %[size]\n"
>>>        // 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4
>>> -    "    csel    %1, xzr, %1, hi\n"
>>> +    "    csel    %[limit], xzr, %[limit], hi\n"
>>>        // 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X'
>>>        //    to compensate for the carry flag being set in step 4. For
>>>        //    X > 2^64, X' merely has to remain nonzero, which it does.
>>> -    "    csinv    %0, %0, xzr, cc\n"
>>> +    "    csinv    %[addr], %[addr], xzr, cc\n"
>>>        // 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1
>>>        //    comes from the carry in being clear. Otherwise, we are
>>>        //    testing X' - C == 0, subject to the previous adjustments.
>>> -    "    sbcs    xzr, %0, %1\n"
>>> -    "    cset    %0, ls\n"
>>> -    : "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc");
>>> +    "    sbcs    xzr, %[addr], %[limit]\n"
>>> +    "       cset    %[addr], ls\n"
>>> +    : [addr] "=&r" (ret), [limit] "+r" (limit)
>>> +    : [size] "Ir" (size), "0" (addr)
>>> +    : "cc");
>>>          return ret;
>>>    }

Patch
diff mbox series

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 32fc8061aa76..ceb1d79eab09 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -75,19 +75,21 @@  static inline unsigned long __range_ok(const void __user *addr, unsigned long si
 	asm volatile(
 	// A + B <= C + 1 for all A,B,C, in four easy steps:
 	// 1: X = A + B; X' = X % 2^64
-	"	adds	%0, %3, %2\n"
+	"	adds	%[addr], %[addr], %[size]\n"
 	// 2: Set C = 0 if X > 2^64, to guarantee X' > C in step 4
-	"	csel	%1, xzr, %1, hi\n"
+	"	csel	%[limit], xzr, %[limit], hi\n"
 	// 3: Set X' = ~0 if X >= 2^64. For X == 2^64, this decrements X'
 	//    to compensate for the carry flag being set in step 4. For
 	//    X > 2^64, X' merely has to remain nonzero, which it does.
-	"	csinv	%0, %0, xzr, cc\n"
+	"	csinv	%[addr], %[addr], xzr, cc\n"
 	// 4: For X < 2^64, this gives us X' - C - 1 <= 0, where the -1
 	//    comes from the carry in being clear. Otherwise, we are
 	//    testing X' - C == 0, subject to the previous adjustments.
-	"	sbcs	xzr, %0, %1\n"
-	"	cset	%0, ls\n"
-	: "=&r" (ret), "+r" (limit) : "Ir" (size), "0" (addr) : "cc");
+	"	sbcs	xzr, %[addr], %[limit]\n"
+	"       cset    %[addr], ls\n"
+	: [addr] "=&r" (ret), [limit] "+r" (limit)
+	: [size] "Ir" (size), "0" (addr)
+	: "cc");
 
 	return ret;
 }