[kvm-unit-tests,1/9] s390x: fix TEST BLOCK tests
diff mbox

Message ID 20180110215348.315-2-david@redhat.com
State New
Headers show

Commit Message

David Hildenbrand Jan. 10, 2018, 9:53 p.m. UTC
While new compilers like
	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
Complain, that R1 is missing, old compilers like
	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
will complain that R1 is not valid.

According to the architecture, R1 is valid but ignored. No idea why this
was changed (in a way that programs no longer compile).

Anyhow, let's just specify the instruction explicitly.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 s390x/intercept.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Thomas Huth Jan. 11, 2018, 8:51 a.m. UTC | #1
On 10.01.2018 22:53, David Hildenbrand wrote:
> While new compilers like
> 	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
> Complain, that R1 is missing, old compilers like
> 	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
> will complain that R1 is not valid.
> 
> According to the architecture, R1 is valid but ignored. No idea why this
> was changed (in a way that programs no longer compile).

What a pity :-(

> Anyhow, let's just specify the instruction explicitly.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  s390x/intercept.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/s390x/intercept.c b/s390x/intercept.c
> index 59e5fca..99dde0d 100644
> --- a/s390x/intercept.c
> +++ b/s390x/intercept.c
> @@ -139,7 +139,7 @@ static void test_testblock(void)
>  
>  	asm volatile (
>  		" lghi	%%r0,0\n"
> -		" tb	%1\n"
> +		" .insn	rre,0xb22c0000,0,%1\n"
>  		" ipm	%0\n"
>  		" srl	%0,28\n"
>  		: "=d" (cc)
> @@ -150,12 +150,12 @@ static void test_testblock(void)
>  
>  	expect_pgm_int();
>  	low_prot_enable();
> -	asm volatile (" tb %0 " : : "r"(4096));
> +	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(4096));
>  	low_prot_disable();
>  	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
>  
>  	expect_pgm_int();
> -	asm volatile (" tb %0 " : : "r"(-4096));
> +	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(-4096));
>  	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
>  }
>  
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>
Christian Borntraeger Jan. 11, 2018, 3:56 p.m. UTC | #2
On 01/11/2018 09:51 AM, Thomas Huth wrote:
> On 10.01.2018 22:53, David Hildenbrand wrote:
>> While new compilers like
>> 	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
>> Complain, that R1 is missing, old compilers like
>> 	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
>> will complain that R1 is not valid.
>>
>> According to the architecture, R1 is valid but ignored. No idea why this
>> was changed (in a way that programs no longer compile).
> 
> What a pity :-(

IIR, this was a bugfix. gcc is also used for TPF and they actually use TB and
the old binutils support for TB was architecturally wrong. Since TB is really
of no use for Linux this should not matter that much apart from the testcases
to test the KVM implementation.
David Hildenbrand Jan. 11, 2018, 4 p.m. UTC | #3
On 11.01.2018 16:56, Christian Borntraeger wrote:
> 
> 
> On 01/11/2018 09:51 AM, Thomas Huth wrote:
>> On 10.01.2018 22:53, David Hildenbrand wrote:
>>> While new compilers like
>>> 	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
>>> Complain, that R1 is missing, old compilers like
>>> 	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
>>> will complain that R1 is not valid.
>>>
>>> According to the architecture, R1 is valid but ignored. No idea why this
>>> was changed (in a way that programs no longer compile).
>>
>> What a pity :-(
> 
> IIR, this was a bugfix. gcc is also used for TPF and they actually use TB and
> the old binutils support for TB was architecturally wrong. Since TB is really
> of no use for Linux this should not matter that much apart from the testcases
> to test the KVM implementation.
> 

Well they managed to break code, so this was indeed a bad decision.
Andreas Krebbel Jan. 11, 2018, 5:54 p.m. UTC | #4
On 01/11/2018 05:00 PM, David Hildenbrand wrote:
> On 11.01.2018 16:56, Christian Borntraeger wrote:
>>
>>
>> On 01/11/2018 09:51 AM, Thomas Huth wrote:
>>> On 10.01.2018 22:53, David Hildenbrand wrote:
>>>> While new compilers like
>>>> 	s390x-linux-gnu-gcc (GCC) 7.2.1 20170915 (Red Hat Cross 7.2.1-1)
>>>> Complain, that R1 is missing, old compilers like
>>>> 	gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
>>>> will complain that R1 is not valid.
>>>>
>>>> According to the architecture, R1 is valid but ignored. No idea why this
>>>> was changed (in a way that programs no longer compile).
>>>
>>> What a pity :-(
>>
>> IIR, this was a bugfix. gcc is also used for TPF and they actually use TB and
>> the old binutils support for TB was architecturally wrong. Since TB is really
>> of no use for Linux this should not matter that much apart from the testcases
>> to test the KVM implementation.
>>
> 
> Well they managed to break code, so this was indeed a bad decision.

With that Binutils change we only report an error for code which was broken anyway.

When writing this testcase the right thing would have been to report a Binutils bug instead of
writing a testcase which uses an instruction which isn't part of the POP.

-Andreas-
David Hildenbrand Jan. 11, 2018, 7:11 p.m. UTC | #5
>> Well they managed to break code, so this was indeed a bad decision.
> 
> With that Binutils change we only report an error for code which was broken anyway.

Well it worked, so it wasn't broken. I would just have liked this
instruction to be fixed in a backwards compatible way (e.g. warning - or
is it just a warning and my gcc flags treat them as errors?).

> 
> When writing this testcase the right thing would have been to report a Binutils bug instead of
> writing a testcase which uses an instruction which isn't part of the POP.

Huh?

PoP - "Control Instructions" - 10-176 - "TEST BLOCK". Even can find it
in the Pop from 2004.

But anyhow, we have this fixed now. Was just wondering, why our
_working_ code suddenly broke (and now we have to use .insn to make it
compile with GCC in general).

Thanks.

> 
> -Andreas-
>
Andreas Krebbel Jan. 11, 2018, 8:33 p.m. UTC | #6
On 01/11/2018 08:11 PM, David Hildenbrand wrote:
> 
>>> Well they managed to break code, so this was indeed a bad decision.
>>
>> With that Binutils change we only report an error for code which was broken anyway.
> 
> Well it worked, so it wasn't broken. I would just have liked this
> instruction to be fixed in a backwards compatible way (e.g. warning - or
> is it just a warning and my gcc flags treat them as errors?).

One operand was missing. The ommitted operand was just 0 then. The instruction never did what the
user expected. That nobody complained about using it that way doesn't mean it was correct.

>> When writing this testcase the right thing would have been to report a Binutils bug instead of
>> writing a testcase which uses an instruction which isn't part of the POP.
> 
> Huh?
> 
> PoP - "Control Instructions" - 10-176 - "TEST BLOCK". Even can find it
> in the Pop from 2004.

It wasn't in the POP the way the testcase used it - with just one operand. It was a bug in Binutils
since the very beginning probably.

> But anyhow, we have this fixed now. Was just wondering, why our
> _working_ code suddenly broke (and now we have to use .insn to make it
> compile with GCC in general)
-Andreas-

Patch
diff mbox

diff --git a/s390x/intercept.c b/s390x/intercept.c
index 59e5fca..99dde0d 100644
--- a/s390x/intercept.c
+++ b/s390x/intercept.c
@@ -139,7 +139,7 @@  static void test_testblock(void)
 
 	asm volatile (
 		" lghi	%%r0,0\n"
-		" tb	%1\n"
+		" .insn	rre,0xb22c0000,0,%1\n"
 		" ipm	%0\n"
 		" srl	%0,28\n"
 		: "=d" (cc)
@@ -150,12 +150,12 @@  static void test_testblock(void)
 
 	expect_pgm_int();
 	low_prot_enable();
-	asm volatile (" tb %0 " : : "r"(4096));
+	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(4096));
 	low_prot_disable();
 	check_pgm_int_code(PGM_INT_CODE_PROTECTION);
 
 	expect_pgm_int();
-	asm volatile (" tb %0 " : : "r"(-4096));
+	asm volatile (" .insn	rre,0xb22c0000,0,%0\n" : : "r"(-4096));
 	check_pgm_int_code(PGM_INT_CODE_ADDRESSING);
 }