[PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[Ilie Halip]

Building with clang & LLVM_IAS=1 leads to an error:
    arch/s390/lib/test_unwind.c:179:4: error: invalid register pair
                        "       mvcl    %%r1,%%r1\n"
                        ^

The test creates an invalid instruction that would trap at runtime, but the
LLVM inline assembler tries to validate it at compile time too.

Use the raw instruction opcode instead.

Link: https://github.com/ClangBuiltLinux/linux/issues/1421
Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Ilie Halip <ilie.halip@gmail.com>
---
 arch/s390/lib/test_unwind.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/lib/test_unwind.c b/arch/s390/lib/test_unwind.c
index cfc5f5557c06..d342bc884b94 100644
--- a/arch/s390/lib/test_unwind.c
+++ b/arch/s390/lib/test_unwind.c
@@ -176,7 +176,7 @@ static noinline int unwindme_func4(struct unwindme *u)
 		 * trigger specification exception
 		 */
 		asm volatile(
-			"	mvcl	%%r1,%%r1\n"
+			"	.insn e,0x0e11\n"	/* mvcl	%%r1,%%r1" */
 			"0:	nopr	%%r7\n"
 			EX_TABLE(0b, 0b)
 			:);
-- 
2.25.1

Re: [PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[Nick Desaulniers]

On Wed, Nov 17, 2021 at 9:48 AM Ilie Halip <ilie.halip@gmail.com> wrote:
>
> Building with clang & LLVM_IAS=1 leads to an error:
>     arch/s390/lib/test_unwind.c:179:4: error: invalid register pair
>                         "       mvcl    %%r1,%%r1\n"
>                         ^
>
> The test creates an invalid instruction that would trap at runtime, but the
> LLVM inline assembler tries to validate it at compile time too.
>
> Use the raw instruction opcode instead.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1421
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Ilie Halip <ilie.halip@gmail.com>

Ilie, thanks for the patch!

So if I understand
https://sourceware.org/binutils/docs/as/s390-Directives.html#s390-Directives
https://sourceware.org/binutils/docs/as/s390-Formats.html
that `e,` prefix is for 16B opcodes?

LGTM, thanks again.
Suggested-by: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

I triple checked that GAS, clang, and GNU objdump are in agreement in
terms of encoding here.

> ---
>  arch/s390/lib/test_unwind.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/s390/lib/test_unwind.c b/arch/s390/lib/test_unwind.c
> index cfc5f5557c06..d342bc884b94 100644
> --- a/arch/s390/lib/test_unwind.c
> +++ b/arch/s390/lib/test_unwind.c
> @@ -176,7 +176,7 @@ static noinline int unwindme_func4(struct unwindme *u)
>                  * trigger specification exception
>                  */
>                 asm volatile(
> -                       "       mvcl    %%r1,%%r1\n"
> +                       "       .insn e,0x0e11\n"       /* mvcl %%r1,%%r1" */
>                         "0:     nopr    %%r7\n"
>                         EX_TABLE(0b, 0b)
>                         :);
> --
> 2.25.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[Christian Borntraeger]

Am 19.11.21 um 02:10 schrieb Nick Desaulniers:
> On Wed, Nov 17, 2021 at 9:48 AM Ilie Halip <ilie.halip@gmail.com> wrote:
>>
>> Building with clang & LLVM_IAS=1 leads to an error:
>>      arch/s390/lib/test_unwind.c:179:4: error: invalid register pair
>>                          "       mvcl    %%r1,%%r1\n"
>>                          ^
>>
>> The test creates an invalid instruction that would trap at runtime, but the
>> LLVM inline assembler tries to validate it at compile time too.
>>
>> Use the raw instruction opcode instead.
>>
>> Link: https://github.com/ClangBuiltLinux/linux/issues/1421
>> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
>> Signed-off-by: Ilie Halip <ilie.halip@gmail.com>
> 
> Ilie, thanks for the patch!
> 
> So if I understand
> https://sourceware.org/binutils/docs/as/s390-Directives.html#s390-Directives
> https://sourceware.org/binutils/docs/as/s390-Formats.html
> that `e,` prefix is for 16B opcodes?

e is an instruction format as specified by the architecture.
See http://publibfp.dhe.ibm.com/epubs/pdf/a227832c.pdf
without any parameters.
Normally RR would be the right thing for MVCL, but since
we try to build an invalid opcode without the assembler
noticing (ab)using e seem like a safer approach.

> 
> LGTM, thanks again.
> Suggested-by: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

added those and added my RB. applied to the s390 tree. Thanks


> 
> I triple checked that GAS, clang, and GNU objdump are in agreement in
> terms of encoding here.
> 
>> ---
>>   arch/s390/lib/test_unwind.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/lib/test_unwind.c b/arch/s390/lib/test_unwind.c
>> index cfc5f5557c06..d342bc884b94 100644
>> --- a/arch/s390/lib/test_unwind.c
>> +++ b/arch/s390/lib/test_unwind.c
>> @@ -176,7 +176,7 @@ static noinline int unwindme_func4(struct unwindme *u)
>>                   * trigger specification exception
>>                   */
>>                  asm volatile(
>> -                       "       mvcl    %%r1,%%r1\n"
>> +                       "       .insn e,0x0e11\n"       /* mvcl %%r1,%%r1" */
>>                          "0:     nopr    %%r7\n"
>>                          EX_TABLE(0b, 0b)
>>                          :);
>> --
>> 2.25.1
>>
> 
> 

Re: [PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[Christian Borntraeger]

Am 19.11.21 um 10:39 schrieb Christian Borntraeger:
> 
> 
> Am 19.11.21 um 02:10 schrieb Nick Desaulniers:
>> On Wed, Nov 17, 2021 at 9:48 AM Ilie Halip <ilie.halip@gmail.com> wrote:
>>>
>>> Building with clang & LLVM_IAS=1 leads to an error:
>>>      arch/s390/lib/test_unwind.c:179:4: error: invalid register pair
>>>                          "       mvcl    %%r1,%%r1\n"
>>>                          ^
>>>
>>> The test creates an invalid instruction that would trap at runtime, but the
>>> LLVM inline assembler tries to validate it at compile time too.
>>>
>>> Use the raw instruction opcode instead.
>>>
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/1421
>>> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
>>> Signed-off-by: Ilie Halip <ilie.halip@gmail.com>
>>
>> Ilie, thanks for the patch!
>>
>> So if I understand
>> https://sourceware.org/binutils/docs/as/s390-Directives.html#s390-Directives
>> https://sourceware.org/binutils/docs/as/s390-Formats.html
>> that `e,` prefix is for 16B opcodes?
> 
> e is an instruction format as specified by the architecture.
> See http://publibfp.dhe.ibm.com/epubs/pdf/a227832c.pdf

(page 5-3 for the instruction formats and page 7-289 for MVCL)

Re: [PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[Heiko Carstens]

On Fri, Nov 19, 2021 at 10:39:15AM +0100, Christian Borntraeger wrote:
> > So if I understand
> > https://sourceware.org/binutils/docs/as/s390-Directives.html#s390-Directives
> > https://sourceware.org/binutils/docs/as/s390-Formats.html
> > that `e,` prefix is for 16B opcodes?
> 
> e is an instruction format as specified by the architecture.
> See http://publibfp.dhe.ibm.com/epubs/pdf/a227832c.pdf
> without any parameters.
> Normally RR would be the right thing for MVCL, but since
> we try to build an invalid opcode without the assembler
> noticing (ab)using e seem like a safer approach.
> > 
> > LGTM, thanks again.
> > Suggested-by: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> added those and added my RB. applied to the s390 tree. Thanks
..
> > > diff --git a/arch/s390/lib/test_unwind.c b/arch/s390/lib/test_unwind.c
> > > index cfc5f5557c06..d342bc884b94 100644
> > > --- a/arch/s390/lib/test_unwind.c
> > > +++ b/arch/s390/lib/test_unwind.c
> > > @@ -176,7 +176,7 @@ static noinline int unwindme_func4(struct unwindme *u)
> > >                   * trigger specification exception
> > >                   */
> > >                  asm volatile(
> > > -                       "       mvcl    %%r1,%%r1\n"
> > > +                       "       .insn e,0x0e11\n"       /* mvcl %%r1,%%r1" */

Sorry, I disagree with this. As you said above rr would be the correct
format for this instruction. If we go for the e format then we should
also use an instruction with e format.
Which in this case would simply be an illegal opcode, which would be
sufficient for what this code is good for: ".insn e,0x0000".

Plus a fixup of the comment above, since this would generate an
operation insteand of a specification exception. Just a generic
"exception" would be good enough for the comment.

Re: [PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[Christian Borntraeger]

Am 19.11.21 um 11:54 schrieb Heiko Carstens:
> On Fri, Nov 19, 2021 at 10:39:15AM +0100, Christian Borntraeger wrote:
>>> So if I understand
>>> https://sourceware.org/binutils/docs/as/s390-Directives.html#s390-Directives
>>> https://sourceware.org/binutils/docs/as/s390-Formats.html
>>> that `e,` prefix is for 16B opcodes?
>>
>> e is an instruction format as specified by the architecture.
>> See http://publibfp.dhe.ibm.com/epubs/pdf/a227832c.pdf
>> without any parameters.
>> Normally RR would be the right thing for MVCL, but since
>> we try to build an invalid opcode without the assembler
>> noticing (ab)using e seem like a safer approach.
>>>
>>> LGTM, thanks again.
>>> Suggested-by: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
>>> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>>
>> added those and added my RB. applied to the s390 tree. Thanks
> ..
>>>> diff --git a/arch/s390/lib/test_unwind.c b/arch/s390/lib/test_unwind.c
>>>> index cfc5f5557c06..d342bc884b94 100644
>>>> --- a/arch/s390/lib/test_unwind.c
>>>> +++ b/arch/s390/lib/test_unwind.c
>>>> @@ -176,7 +176,7 @@ static noinline int unwindme_func4(struct unwindme *u)
>>>>                    * trigger specification exception
>>>>                    */
>>>>                   asm volatile(
>>>> -                       "       mvcl    %%r1,%%r1\n"
>>>> +                       "       .insn e,0x0e11\n"       /* mvcl %%r1,%%r1" */
> 
> Sorry, I disagree with this. As you said above rr would be the correct
> format for this instruction. If we go for the e format then we should
> also use an instruction with e format.
> Which in this case would simply be an illegal opcode, which would be
> sufficient for what this code is good for: ".insn e,0x0000".

Why not simply use .short then?

Re: [PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[Heiko Carstens]

On Fri, Nov 19, 2021 at 11:57:05AM +0100, Christian Borntraeger wrote:
> > > > > -                       "       mvcl    %%r1,%%r1\n"
> > > > > +                       "       .insn e,0x0e11\n"       /* mvcl %%r1,%%r1" */
> > 
> > Sorry, I disagree with this. As you said above rr would be the correct
> > format for this instruction. If we go for the e format then we should
> > also use an instruction with e format.
> > Which in this case would simply be an illegal opcode, which would be
> > sufficient for what this code is good for: ".insn e,0x0000".
> 
> Why not simply use .short then?

.short bypasses all sanity checks while .insn does not, so I think
that should be preferred. But I don't care too much.

Re: [PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[Christian Borntraeger]

Am 19.11.21 um 12:09 schrieb Heiko Carstens:
> On Fri, Nov 19, 2021 at 11:57:05AM +0100, Christian Borntraeger wrote:
>>>>>> -                       "       mvcl    %%r1,%%r1\n"
>>>>>> +                       "       .insn e,0x0e11\n"       /* mvcl %%r1,%%r1" */
>>>
>>> Sorry, I disagree with this. As you said above rr would be the correct
>>> format for this instruction. If we go for the e format then we should
>>> also use an instruction with e format.
>>> Which in this case would simply be an illegal opcode, which would be
>>> sufficient for what this code is good for: ".insn e,0x0000".
>>
>> Why not simply use .short then?
> 
> .short bypasses all sanity checks while .insn does not, so I think
> that should be preferred. But I don't care too much.

Heiko,
I am fine with ".insn e,0x0000" and the a changed comment that changes "specification exception" to "operation exception".
Do you want Ilie to resend or simply fixup?

Re: [PATCH] s390/test_unwind: use raw opcode instead of invalid instruction

[Heiko Carstens]

On Fri, Nov 19, 2021 at 03:12:03PM +0100, Christian Borntraeger wrote:
> Am 19.11.21 um 12:09 schrieb Heiko Carstens:
> > On Fri, Nov 19, 2021 at 11:57:05AM +0100, Christian Borntraeger wrote:
> > > > > > > -                       "       mvcl    %%r1,%%r1\n"
> > > > > > > +                       "       .insn e,0x0e11\n"       /* mvcl %%r1,%%r1" */
> > > > 
> > > > Sorry, I disagree with this. As you said above rr would be the correct
> > > > format for this instruction. If we go for the e format then we should
> > > > also use an instruction with e format.
> > > > Which in this case would simply be an illegal opcode, which would be
> > > > sufficient for what this code is good for: ".insn e,0x0000".
> > > 
> > > Why not simply use .short then?
> > 
> > .short bypasses all sanity checks while .insn does not, so I think
> > that should be preferred. But I don't care too much.
> 
> Heiko,
> I am fine with ".insn e,0x0000" and the a changed comment that
> changes "specification exception" to "operation exception".  Do you
> want Ilie to resend or simply fixup?

I'll simply change it. Let's don't spend more time on this.