[PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT

[PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT

[Matthias Kaehlcke]

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>");
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT

[Ard Biesheuvel]

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?

Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT

[Robin Murphy]

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
> 

Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT

[Ard Biesheuvel]

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.

Re: [PATCH] crypto: arm64/sha: use %c constraint code in ASM_EXPORT

[Matthias Kaehlcke]

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