* Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
@ 2017-04-05 17:08 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2017-04-05 17:08 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Greg Hackmann, Herbert Xu, David S . Miller, Catalin Marinas,
Will Deacon, linux-kernel@vger.kernel.org,
linux-crypto@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Grant Grundler,
Michael Davidson
Hoi Matthias!
On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
> From: Greg Hackmann <ghackmann@google.com>
>
> The current definition of ASM_EXPORT doesn't work properly with clang,
> according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
> gcc specific behavior. Change the constraint from an intermediate to an
> output expression which works with both gcc and clang.
>
> From: Greg Hackmann <ghackmann@google.com>
> Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Greg Hackmann <ghackmann@google.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> arch/arm64/crypto/sha1-ce-glue.c | 2 +-
> arch/arm64/crypto/sha2-ce-glue.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
> index aefda9868627..c71e94ba0e43 100644
> --- a/arch/arm64/crypto/sha1-ce-glue.c
> +++ b/arch/arm64/crypto/sha1-ce-glue.c
> @@ -18,7 +18,7 @@
> #include <linux/module.h>
>
> #define ASM_EXPORT(sym, val) \
> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>
> MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
> index 7cd587564a41..381b5fb2dcb2 100644
> --- a/arch/arm64/crypto/sha2-ce-glue.c
> +++ b/arch/arm64/crypto/sha2-ce-glue.c
> @@ -18,7 +18,7 @@
> #include <linux/module.h>
>
> #define ASM_EXPORT(sym, val) \
> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>
> MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
I am fine with this change, although I would really like to add a
better reference to the commit log. It is *very* difficult to find any
documentation regarding non-trivial uses of inline asm constraints,
and if %c0 is the correct syntax, surely we can quote something better
than a LLVM bugzilla entry? Also, where does the distinction between
'intermediate' vs 'output' expression come from?
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
@ 2017-04-05 17:08 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2017-04-05 17:08 UTC (permalink / raw)
To: linux-arm-kernel
Hoi Matthias!
On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
> From: Greg Hackmann <ghackmann@google.com>
>
> The current definition of ASM_EXPORT doesn't work properly with clang,
> according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
> gcc specific behavior. Change the constraint from an intermediate to an
> output expression which works with both gcc and clang.
>
> From: Greg Hackmann <ghackmann@google.com>
> Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Greg Hackmann <ghackmann@google.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> arch/arm64/crypto/sha1-ce-glue.c | 2 +-
> arch/arm64/crypto/sha2-ce-glue.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
> index aefda9868627..c71e94ba0e43 100644
> --- a/arch/arm64/crypto/sha1-ce-glue.c
> +++ b/arch/arm64/crypto/sha1-ce-glue.c
> @@ -18,7 +18,7 @@
> #include <linux/module.h>
>
> #define ASM_EXPORT(sym, val) \
> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>
> MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
> index 7cd587564a41..381b5fb2dcb2 100644
> --- a/arch/arm64/crypto/sha2-ce-glue.c
> +++ b/arch/arm64/crypto/sha2-ce-glue.c
> @@ -18,7 +18,7 @@
> #include <linux/module.h>
>
> #define ASM_EXPORT(sym, val) \
> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>
> MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
I am fine with this change, although I would really like to add a
better reference to the commit log. It is *very* difficult to find any
documentation regarding non-trivial uses of inline asm constraints,
and if %c0 is the correct syntax, surely we can quote something better
than a LLVM bugzilla entry? Also, where does the distinction between
'intermediate' vs 'output' expression come from?
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
2017-04-05 17:08 ` Ard Biesheuvel
@ 2017-04-05 17:21 ` Robin Murphy
-1 siblings, 0 replies; 14+ messages in thread
From: Robin Murphy @ 2017-04-05 17:21 UTC (permalink / raw)
To: Ard Biesheuvel, Matthias Kaehlcke
Cc: Herbert Xu, Grant Grundler, Catalin Marinas, Will Deacon,
linux-kernel@vger.kernel.org, Greg Hackmann, Michael Davidson,
linux-crypto@vger.kernel.org, David S . Miller,
linux-arm-kernel@lists.infradead.org
On 05/04/17 18:08, Ard Biesheuvel wrote:
> Hoi Matthias!
>
> On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
>> From: Greg Hackmann <ghackmann@google.com>
>>
>> The current definition of ASM_EXPORT doesn't work properly with clang,
>> according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
>> gcc specific behavior. Change the constraint from an intermediate to an
>> output expression which works with both gcc and clang.
>>
>> From: Greg Hackmann <ghackmann@google.com>
>> Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
>> Signed-off-by: Greg Hackmann <ghackmann@google.com>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>> arch/arm64/crypto/sha1-ce-glue.c | 2 +-
>> arch/arm64/crypto/sha2-ce-glue.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
>> index aefda9868627..c71e94ba0e43 100644
>> --- a/arch/arm64/crypto/sha1-ce-glue.c
>> +++ b/arch/arm64/crypto/sha1-ce-glue.c
>> @@ -18,7 +18,7 @@
>> #include <linux/module.h>
>>
>> #define ASM_EXPORT(sym, val) \
>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>
>> MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
>> index 7cd587564a41..381b5fb2dcb2 100644
>> --- a/arch/arm64/crypto/sha2-ce-glue.c
>> +++ b/arch/arm64/crypto/sha2-ce-glue.c
>> @@ -18,7 +18,7 @@
>> #include <linux/module.h>
>>
>> #define ASM_EXPORT(sym, val) \
>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>
>> MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>
> I am fine with this change, although I would really like to add a
> better reference to the commit log. It is *very* difficult to find any
> documentation regarding non-trivial uses of inline asm constraints,
> and if %c0 is the correct syntax, surely we can quote something better
> than a LLVM bugzilla entry? Also, where does the distinction between
> 'intermediate' vs 'output' expression come from?
FWIW, GCC docs do say (under the helpfully-obvious "x86 operand
modifiers" section[1]):
c Require a constant operand and print the constant
expression with no punctuation.
Which more or less makes sense in this this context too. As an aside,
though, since this is emitting a general integer argument to an
assembler directive, and not an operand to an ADD instruction, how come
we're using "I" and not "i" as the constraint in the first place?
Robin.
[1]:https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#x86-Operand-Modifiers
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
@ 2017-04-05 17:21 ` Robin Murphy
0 siblings, 0 replies; 14+ messages in thread
From: Robin Murphy @ 2017-04-05 17:21 UTC (permalink / raw)
To: linux-arm-kernel
On 05/04/17 18:08, Ard Biesheuvel wrote:
> Hoi Matthias!
>
> On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
>> From: Greg Hackmann <ghackmann@google.com>
>>
>> The current definition of ASM_EXPORT doesn't work properly with clang,
>> according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
>> gcc specific behavior. Change the constraint from an intermediate to an
>> output expression which works with both gcc and clang.
>>
>> From: Greg Hackmann <ghackmann@google.com>
>> Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
>> Signed-off-by: Greg Hackmann <ghackmann@google.com>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>> arch/arm64/crypto/sha1-ce-glue.c | 2 +-
>> arch/arm64/crypto/sha2-ce-glue.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
>> index aefda9868627..c71e94ba0e43 100644
>> --- a/arch/arm64/crypto/sha1-ce-glue.c
>> +++ b/arch/arm64/crypto/sha1-ce-glue.c
>> @@ -18,7 +18,7 @@
>> #include <linux/module.h>
>>
>> #define ASM_EXPORT(sym, val) \
>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>
>> MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
>> index 7cd587564a41..381b5fb2dcb2 100644
>> --- a/arch/arm64/crypto/sha2-ce-glue.c
>> +++ b/arch/arm64/crypto/sha2-ce-glue.c
>> @@ -18,7 +18,7 @@
>> #include <linux/module.h>
>>
>> #define ASM_EXPORT(sym, val) \
>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>
>> MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>
> I am fine with this change, although I would really like to add a
> better reference to the commit log. It is *very* difficult to find any
> documentation regarding non-trivial uses of inline asm constraints,
> and if %c0 is the correct syntax, surely we can quote something better
> than a LLVM bugzilla entry? Also, where does the distinction between
> 'intermediate' vs 'output' expression come from?
FWIW, GCC docs do say (under the helpfully-obvious "x86 operand
modifiers" section[1]):
c Require a constant operand and print the constant
expression with no punctuation.
Which more or less makes sense in this this context too. As an aside,
though, since this is emitting a general integer argument to an
assembler directive, and not an operand to an ADD instruction, how come
we're using "I" and not "i" as the constraint in the first place?
Robin.
[1]:https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#x86-Operand-Modifiers
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
2017-04-05 17:21 ` Robin Murphy
(?)
@ 2017-04-05 17:38 ` Ard Biesheuvel
-1 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2017-04-05 17:38 UTC (permalink / raw)
To: Robin Murphy
Cc: Herbert Xu, Grant Grundler, Catalin Marinas, Will Deacon,
linux-kernel@vger.kernel.org, Greg Hackmann, Michael Davidson,
Matthias Kaehlcke, linux-crypto@vger.kernel.org, David S . Miller,
linux-arm-kernel@lists.infradead.org
On 5 April 2017 at 18:21, Robin Murphy <robin.murphy@arm.com> wrote:
> On 05/04/17 18:08, Ard Biesheuvel wrote:
>> Hoi Matthias!
>>
>> On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
>>> From: Greg Hackmann <ghackmann@google.com>
>>>
>>> The current definition of ASM_EXPORT doesn't work properly with clang,
>>> according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
>>> gcc specific behavior. Change the constraint from an intermediate to an
>>> output expression which works with both gcc and clang.
>>>
>>> From: Greg Hackmann <ghackmann@google.com>
>>> Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
>>> Signed-off-by: Greg Hackmann <ghackmann@google.com>
>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>> ---
>>> arch/arm64/crypto/sha1-ce-glue.c | 2 +-
>>> arch/arm64/crypto/sha2-ce-glue.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
>>> index aefda9868627..c71e94ba0e43 100644
>>> --- a/arch/arm64/crypto/sha1-ce-glue.c
>>> +++ b/arch/arm64/crypto/sha1-ce-glue.c
>>> @@ -18,7 +18,7 @@
>>> #include <linux/module.h>
>>>
>>> #define ASM_EXPORT(sym, val) \
>>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>>
>>> MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
>>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>>> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
>>> index 7cd587564a41..381b5fb2dcb2 100644
>>> --- a/arch/arm64/crypto/sha2-ce-glue.c
>>> +++ b/arch/arm64/crypto/sha2-ce-glue.c
>>> @@ -18,7 +18,7 @@
>>> #include <linux/module.h>
>>>
>>> #define ASM_EXPORT(sym, val) \
>>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>>
>>> MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
>>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>>
>> I am fine with this change, although I would really like to add a
>> better reference to the commit log. It is *very* difficult to find any
>> documentation regarding non-trivial uses of inline asm constraints,
>> and if %c0 is the correct syntax, surely we can quote something better
>> than a LLVM bugzilla entry? Also, where does the distinction between
>> 'intermediate' vs 'output' expression come from?
>
> FWIW, GCC docs do say (under the helpfully-obvious "x86 operand
> modifiers" section[1]):
>
> c Require a constant operand and print the constant
> expression with no punctuation.
>
> Which more or less makes sense in this this context too. As an aside,
> though, since this is emitting a general integer argument to an
> assembler directive, and not an operand to an ADD instruction, how come
> we're using "I" and not "i" as the constraint in the first place?
>
No reason. "I" came to mind when writing the code, and worked as
expected. Perhaps we should just fix that at the same time.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
@ 2017-04-05 17:38 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2017-04-05 17:38 UTC (permalink / raw)
To: Robin Murphy
Cc: Matthias Kaehlcke, Herbert Xu, Grant Grundler, Catalin Marinas,
Will Deacon, linux-kernel@vger.kernel.org, Greg Hackmann,
Michael Davidson, linux-crypto@vger.kernel.org, David S . Miller,
linux-arm-kernel@lists.infradead.org
On 5 April 2017 at 18:21, Robin Murphy <robin.murphy@arm.com> wrote:
> On 05/04/17 18:08, Ard Biesheuvel wrote:
>> Hoi Matthias!
>>
>> On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
>>> From: Greg Hackmann <ghackmann@google.com>
>>>
>>> The current definition of ASM_EXPORT doesn't work properly with clang,
>>> according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
>>> gcc specific behavior. Change the constraint from an intermediate to an
>>> output expression which works with both gcc and clang.
>>>
>>> From: Greg Hackmann <ghackmann@google.com>
>>> Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
>>> Signed-off-by: Greg Hackmann <ghackmann@google.com>
>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>> ---
>>> arch/arm64/crypto/sha1-ce-glue.c | 2 +-
>>> arch/arm64/crypto/sha2-ce-glue.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
>>> index aefda9868627..c71e94ba0e43 100644
>>> --- a/arch/arm64/crypto/sha1-ce-glue.c
>>> +++ b/arch/arm64/crypto/sha1-ce-glue.c
>>> @@ -18,7 +18,7 @@
>>> #include <linux/module.h>
>>>
>>> #define ASM_EXPORT(sym, val) \
>>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>>
>>> MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
>>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>>> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
>>> index 7cd587564a41..381b5fb2dcb2 100644
>>> --- a/arch/arm64/crypto/sha2-ce-glue.c
>>> +++ b/arch/arm64/crypto/sha2-ce-glue.c
>>> @@ -18,7 +18,7 @@
>>> #include <linux/module.h>
>>>
>>> #define ASM_EXPORT(sym, val) \
>>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>>
>>> MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
>>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>>
>> I am fine with this change, although I would really like to add a
>> better reference to the commit log. It is *very* difficult to find any
>> documentation regarding non-trivial uses of inline asm constraints,
>> and if %c0 is the correct syntax, surely we can quote something better
>> than a LLVM bugzilla entry? Also, where does the distinction between
>> 'intermediate' vs 'output' expression come from?
>
> FWIW, GCC docs do say (under the helpfully-obvious "x86 operand
> modifiers" section[1]):
>
> c Require a constant operand and print the constant
> expression with no punctuation.
>
> Which more or less makes sense in this this context too. As an aside,
> though, since this is emitting a general integer argument to an
> assembler directive, and not an operand to an ADD instruction, how come
> we're using "I" and not "i" as the constraint in the first place?
>
No reason. "I" came to mind when writing the code, and worked as
expected. Perhaps we should just fix that at the same time.
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
@ 2017-04-05 17:38 ` Ard Biesheuvel
0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2017-04-05 17:38 UTC (permalink / raw)
To: linux-arm-kernel
On 5 April 2017 at 18:21, Robin Murphy <robin.murphy@arm.com> wrote:
> On 05/04/17 18:08, Ard Biesheuvel wrote:
>> Hoi Matthias!
>>
>> On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
>>> From: Greg Hackmann <ghackmann@google.com>
>>>
>>> The current definition of ASM_EXPORT doesn't work properly with clang,
>>> according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
>>> gcc specific behavior. Change the constraint from an intermediate to an
>>> output expression which works with both gcc and clang.
>>>
>>> From: Greg Hackmann <ghackmann@google.com>
>>> Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
>>> Signed-off-by: Greg Hackmann <ghackmann@google.com>
>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>> ---
>>> arch/arm64/crypto/sha1-ce-glue.c | 2 +-
>>> arch/arm64/crypto/sha2-ce-glue.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
>>> index aefda9868627..c71e94ba0e43 100644
>>> --- a/arch/arm64/crypto/sha1-ce-glue.c
>>> +++ b/arch/arm64/crypto/sha1-ce-glue.c
>>> @@ -18,7 +18,7 @@
>>> #include <linux/module.h>
>>>
>>> #define ASM_EXPORT(sym, val) \
>>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>>
>>> MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
>>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>>> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
>>> index 7cd587564a41..381b5fb2dcb2 100644
>>> --- a/arch/arm64/crypto/sha2-ce-glue.c
>>> +++ b/arch/arm64/crypto/sha2-ce-glue.c
>>> @@ -18,7 +18,7 @@
>>> #include <linux/module.h>
>>>
>>> #define ASM_EXPORT(sym, val) \
>>> - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>>> + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
>>>
>>> MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
>>> MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>>
>> I am fine with this change, although I would really like to add a
>> better reference to the commit log. It is *very* difficult to find any
>> documentation regarding non-trivial uses of inline asm constraints,
>> and if %c0 is the correct syntax, surely we can quote something better
>> than a LLVM bugzilla entry? Also, where does the distinction between
>> 'intermediate' vs 'output' expression come from?
>
> FWIW, GCC docs do say (under the helpfully-obvious "x86 operand
> modifiers" section[1]):
>
> c Require a constant operand and print the constant
> expression with no punctuation.
>
> Which more or less makes sense in this this context too. As an aside,
> though, since this is emitting a general integer argument to an
> assembler directive, and not an operand to an ADD instruction, how come
> we're using "I" and not "i" as the constraint in the first place?
>
No reason. "I" came to mind when writing the code, and worked as
expected. Perhaps we should just fix that at the same time.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
2017-04-05 17:08 ` Ard Biesheuvel
(?)
@ 2017-04-05 17:43 ` Matthias Kaehlcke
-1 siblings, 0 replies; 14+ messages in thread
From: Matthias Kaehlcke @ 2017-04-05 17:43 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Herbert Xu, Grant Grundler, Catalin Marinas, Will Deacon,
linux-kernel@vger.kernel.org, Greg Hackmann, Michael Davidson,
linux-crypto@vger.kernel.org, Robin Murphy, David S . Miller,
linux-arm-kernel@lists.infradead.org
Hoi Ard!
El Wed, Apr 05, 2017 at 06:08:37PM +0100 Ard Biesheuvel ha dit:
> On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
> > From: Greg Hackmann <ghackmann@google.com>
> >
> > The current definition of ASM_EXPORT doesn't work properly with clang,
> > according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
> > gcc specific behavior. Change the constraint from an intermediate to an
> > output expression which works with both gcc and clang.
> >
> > From: Greg Hackmann <ghackmann@google.com>
> > Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
> > Signed-off-by: Greg Hackmann <ghackmann@google.com>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > arch/arm64/crypto/sha1-ce-glue.c | 2 +-
> > arch/arm64/crypto/sha2-ce-glue.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
> > index aefda9868627..c71e94ba0e43 100644
> > --- a/arch/arm64/crypto/sha1-ce-glue.c
> > +++ b/arch/arm64/crypto/sha1-ce-glue.c
> > @@ -18,7 +18,7 @@
> > #include <linux/module.h>
> >
> > #define ASM_EXPORT(sym, val) \
> > - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> > + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
> >
> > MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
> > MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
> > diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
> > index 7cd587564a41..381b5fb2dcb2 100644
> > --- a/arch/arm64/crypto/sha2-ce-glue.c
> > +++ b/arch/arm64/crypto/sha2-ce-glue.c
> > @@ -18,7 +18,7 @@
> > #include <linux/module.h>
> >
> > #define ASM_EXPORT(sym, val) \
> > - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> > + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
> >
> > MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
> > MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>
> I am fine with this change, although I would really like to add a
> better reference to the commit log. It is *very* difficult to find any
> documentation regarding non-trivial uses of inline asm constraints,
> and if %c0 is the correct syntax, surely we can quote something better
> than a LLVM bugzilla entry? Also, where does the distinction between
> 'intermediate' vs 'output' expression come from?
To be honest assembly is not really my area of expertise and I relayed
the information from the LLVM bug. The link to the gcc doc on
x86(!) operand modifiers posted by Robin Murphy (thanks!) should be
helpful to provide a more accurate commit message.
Thanks
Matthias
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
@ 2017-04-05 17:43 ` Matthias Kaehlcke
0 siblings, 0 replies; 14+ messages in thread
From: Matthias Kaehlcke @ 2017-04-05 17:43 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Greg Hackmann, Herbert Xu, David S . Miller, Catalin Marinas,
Will Deacon, linux-kernel@vger.kernel.org,
linux-crypto@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Grant Grundler,
Michael Davidson, Robin Murphy
Hoi Ard!
El Wed, Apr 05, 2017 at 06:08:37PM +0100 Ard Biesheuvel ha dit:
> On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
> > From: Greg Hackmann <ghackmann@google.com>
> >
> > The current definition of ASM_EXPORT doesn't work properly with clang,
> > according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
> > gcc specific behavior. Change the constraint from an intermediate to an
> > output expression which works with both gcc and clang.
> >
> > From: Greg Hackmann <ghackmann@google.com>
> > Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
> > Signed-off-by: Greg Hackmann <ghackmann@google.com>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > arch/arm64/crypto/sha1-ce-glue.c | 2 +-
> > arch/arm64/crypto/sha2-ce-glue.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
> > index aefda9868627..c71e94ba0e43 100644
> > --- a/arch/arm64/crypto/sha1-ce-glue.c
> > +++ b/arch/arm64/crypto/sha1-ce-glue.c
> > @@ -18,7 +18,7 @@
> > #include <linux/module.h>
> >
> > #define ASM_EXPORT(sym, val) \
> > - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> > + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
> >
> > MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
> > MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
> > diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
> > index 7cd587564a41..381b5fb2dcb2 100644
> > --- a/arch/arm64/crypto/sha2-ce-glue.c
> > +++ b/arch/arm64/crypto/sha2-ce-glue.c
> > @@ -18,7 +18,7 @@
> > #include <linux/module.h>
> >
> > #define ASM_EXPORT(sym, val) \
> > - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> > + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
> >
> > MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
> > MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>
> I am fine with this change, although I would really like to add a
> better reference to the commit log. It is *very* difficult to find any
> documentation regarding non-trivial uses of inline asm constraints,
> and if %c0 is the correct syntax, surely we can quote something better
> than a LLVM bugzilla entry? Also, where does the distinction between
> 'intermediate' vs 'output' expression come from?
To be honest assembly is not really my area of expertise and I relayed
the information from the LLVM bug. The link to the gcc doc on
x86(!) operand modifiers posted by Robin Murphy (thanks!) should be
helpful to provide a more accurate commit message.
Thanks
Matthias
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT
@ 2017-04-05 17:43 ` Matthias Kaehlcke
0 siblings, 0 replies; 14+ messages in thread
From: Matthias Kaehlcke @ 2017-04-05 17:43 UTC (permalink / raw)
To: linux-arm-kernel
Hoi Ard!
El Wed, Apr 05, 2017 at 06:08:37PM +0100 Ard Biesheuvel ha dit:
> On 5 April 2017 at 17:56, Matthias Kaehlcke <mka@chromium.org> wrote:
> > From: Greg Hackmann <ghackmann@google.com>
> >
> > The current definition of ASM_EXPORT doesn't work properly with clang,
> > according to https://bugs.llvm.org//show_bug.cgi?id=27250#c3 it relies on
> > gcc specific behavior. Change the constraint from an intermediate to an
> > output expression which works with both gcc and clang.
> >
> > From: Greg Hackmann <ghackmann@google.com>
> > Commit-message-by: Matthias Kaehlcke <mka@chromium.org>
> > Signed-off-by: Greg Hackmann <ghackmann@google.com>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > arch/arm64/crypto/sha1-ce-glue.c | 2 +-
> > arch/arm64/crypto/sha2-ce-glue.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
> > index aefda9868627..c71e94ba0e43 100644
> > --- a/arch/arm64/crypto/sha1-ce-glue.c
> > +++ b/arch/arm64/crypto/sha1-ce-glue.c
> > @@ -18,7 +18,7 @@
> > #include <linux/module.h>
> >
> > #define ASM_EXPORT(sym, val) \
> > - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> > + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
> >
> > MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
> > MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
> > diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
> > index 7cd587564a41..381b5fb2dcb2 100644
> > --- a/arch/arm64/crypto/sha2-ce-glue.c
> > +++ b/arch/arm64/crypto/sha2-ce-glue.c
> > @@ -18,7 +18,7 @@
> > #include <linux/module.h>
> >
> > #define ASM_EXPORT(sym, val) \
> > - asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> > + asm(".globl " #sym "; .set " #sym ", %c0" :: "I"(val));
> >
> > MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
> > MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>
> I am fine with this change, although I would really like to add a
> better reference to the commit log. It is *very* difficult to find any
> documentation regarding non-trivial uses of inline asm constraints,
> and if %c0 is the correct syntax, surely we can quote something better
> than a LLVM bugzilla entry? Also, where does the distinction between
> 'intermediate' vs 'output' expression come from?
To be honest assembly is not really my area of expertise and I relayed
the information from the LLVM bug. The link to the gcc doc on
x86(!) operand modifiers posted by Robin Murphy (thanks!) should be
helpful to provide a more accurate commit message.
Thanks
Matthias
^ permalink raw reply [flat|nested] 14+ messages in thread