[PATCH kvm-unit-tests 0/2] s390x: diag288: Improve readability

[PATCH kvm-unit-tests 0/2] s390x: diag288: Improve readability

[Nico Boehr]

I had a somewhat hard time figuring out what diag288 does, in particular what
the number 424 is. This is an attempt to fix that by improving the naming.

While looking at the code, I also noticed we're missing a clobber for
r0, addressing in this series as well.

Nico Boehr (2):
  s390x: diag288: Add missing clobber
  s390x: diag288: Improve readability

 s390x/diag288.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH kvm-unit-tests 1/2] s390x: diag288: Add missing clobber

[Nico Boehr]

We clobber r0 and thus should let the compiler know we're doing so.

Because we change from basic to extended ASM, we need to change the
register names, as %r0 will be interpreted as a token in the assembler
template.

For consistency, we align with the common style in kvm-unit-tests which
is just 0.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/diag288.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/s390x/diag288.c b/s390x/diag288.c
index 072c04a5cbd6..da7b06c365bf 100644
--- a/s390x/diag288.c
+++ b/s390x/diag288.c
@@ -94,11 +94,12 @@ static void test_bite(void)
 	/* Arm watchdog */
 	lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
 	diag288(CODE_INIT, 15, ACTION_RESTART);
-	asm volatile("		larl	%r0, 1f\n"
-		     "		stg	%r0, 424\n"
+	asm volatile("		larl	0, 1f\n"
+		     "		stg	0, 424\n"
 		     "0:	nop\n"
 		     "		j	0b\n"
-		     "1:");
+		     "1:"
+		     : : : "0");
 	report_pass("restart");
 }
 
-- 
2.31.1

[PATCH kvm-unit-tests 2/2] s390x: diag288: Improve readability

[Nico Boehr]

Use a more descriptive name instead of the magic number 424 (address of
restart new PSW in the lowcore).

In addition, add a comment to make it more obvious what the ASM snippet
does.

Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
---
 s390x/diag288.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/s390x/diag288.c b/s390x/diag288.c
index da7b06c365bf..a2c263e38338 100644
--- a/s390x/diag288.c
+++ b/s390x/diag288.c
@@ -94,12 +94,15 @@ static void test_bite(void)
 	/* Arm watchdog */
 	lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
 	diag288(CODE_INIT, 15, ACTION_RESTART);
+	/* Wait for restart interruption */
 	asm volatile("		larl	0, 1f\n"
-		     "		stg	0, 424\n"
+		     "		stg	0, %[restart_new_psw]\n"
 		     "0:	nop\n"
 		     "		j	0b\n"
 		     "1:"
-		     : : : "0");
+		     :
+		     : [restart_new_psw] "T" (lc->restart_new_psw.addr)
+		     : "0");
 	report_pass("restart");
 }
 
-- 
2.31.1

Re: [PATCH kvm-unit-tests 2/2] s390x: diag288: Improve readability

[Heiko Carstens]

On Fri, Dec 17, 2021 at 11:31:37AM +0100, Nico Boehr wrote:
> Use a more descriptive name instead of the magic number 424 (address of
> restart new PSW in the lowcore).
> 
> In addition, add a comment to make it more obvious what the ASM snippet
> does.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>  s390x/diag288.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/s390x/diag288.c b/s390x/diag288.c
> index da7b06c365bf..a2c263e38338 100644
> --- a/s390x/diag288.c
> +++ b/s390x/diag288.c
> @@ -94,12 +94,15 @@ static void test_bite(void)
>  	/* Arm watchdog */
>  	lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
>  	diag288(CODE_INIT, 15, ACTION_RESTART);
> +	/* Wait for restart interruption */
>  	asm volatile("		larl	0, 1f\n"
> -		     "		stg	0, 424\n"
> +		     "		stg	0, %[restart_new_psw]\n"
>  		     "0:	nop\n"
>  		     "		j	0b\n"
>  		     "1:"
> -		     : : : "0");
> +		     :
> +		     : [restart_new_psw] "T" (lc->restart_new_psw.addr)

Even though it was wrong and missing before: this is an output not an input
parameter. Also, older compilers might fail if only the "T" constraint is
given (see gcc commit 3e4be43f69da ("S/390: Memory constraint cleanup")).
Which means: "=RT" would be correct. To be on the safe side, and to avoid
that gcc optimizes any potential prior C code away, I'd recommend to use
"+RT" in this case.

Also there is an ordering problem here: starting the time bomb before the
restart psw has been setup is racy. It is unlikely that this fails, but
still...

Correct would be to setup the restart psw, and then start the time
bomb. This would also allow to shorten the runtime of this test case to
1 second, instead of the 15 seconds it is running now.

It was all like that before, I know. Just some comments ;)

Re: [PATCH kvm-unit-tests 1/2] s390x: diag288: Add missing clobber

[Thomas Huth]

On 17/12/2021 11.31, Nico Boehr wrote:
> We clobber r0 and thus should let the compiler know we're doing so.
> 
> Because we change from basic to extended ASM, we need to change the
> register names, as %r0 will be interpreted as a token in the assembler
> template.
> 
> For consistency, we align with the common style in kvm-unit-tests which
> is just 0.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> ---
>   s390x/diag288.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/s390x/diag288.c b/s390x/diag288.c
> index 072c04a5cbd6..da7b06c365bf 100644
> --- a/s390x/diag288.c
> +++ b/s390x/diag288.c
> @@ -94,11 +94,12 @@ static void test_bite(void)
>   	/* Arm watchdog */
>   	lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
>   	diag288(CODE_INIT, 15, ACTION_RESTART);
> -	asm volatile("		larl	%r0, 1f\n"
> -		     "		stg	%r0, 424\n"
> +	asm volatile("		larl	0, 1f\n"
> +		     "		stg	0, 424\n"

Would it work to use %%r0 instead?

>   		     "0:	nop\n"
>   		     "		j	0b\n"
> -		     "1:");
> +		     "1:"
> +		     : : : "0");
>   	report_pass("restart");
>   }

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH kvm-unit-tests 2/2] s390x: diag288: Improve readability

[Janosch Frank]

On 12/17/21 12:08, Heiko Carstens wrote:
> On Fri, Dec 17, 2021 at 11:31:37AM +0100, Nico Boehr wrote:
>> Use a more descriptive name instead of the magic number 424 (address of
>> restart new PSW in the lowcore).
>>
>> In addition, add a comment to make it more obvious what the ASM snippet
>> does.
>>
>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
>> ---
>>   s390x/diag288.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/s390x/diag288.c b/s390x/diag288.c
>> index da7b06c365bf..a2c263e38338 100644
>> --- a/s390x/diag288.c
>> +++ b/s390x/diag288.c
>> @@ -94,12 +94,15 @@ static void test_bite(void)
>>   	/* Arm watchdog */
>>   	lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
>>   	diag288(CODE_INIT, 15, ACTION_RESTART);
>> +	/* Wait for restart interruption */
>>   	asm volatile("		larl	0, 1f\n"
>> -		     "		stg	0, 424\n"
>> +		     "		stg	0, %[restart_new_psw]\n"
>>   		     "0:	nop\n"
>>   		     "		j	0b\n"
>>   		     "1:"
>> -		     : : : "0");
>> +		     :
>> +		     : [restart_new_psw] "T" (lc->restart_new_psw.addr)
> 
> Even though it was wrong and missing before: this is an output not an input
> parameter. Also, older compilers might fail if only the "T" constraint is
> given (see gcc commit 3e4be43f69da ("S/390: Memory constraint cleanup")).
> Which means: "=RT" would be correct. To be on the safe side, and to avoid
> that gcc optimizes any potential prior C code away, I'd recommend to use
> "+RT" in this case.

Thanks for clearing that up, those intricate details are quite hard to 
find/remember if you only write inline assembly every few months.

> 
> Also there is an ordering problem here: starting the time bomb before the
> restart psw has been setup is racy. It is unlikely that this fails, but
> still...
> 
> Correct would be to setup the restart psw, and then start the time
> bomb. This would also allow to shorten the runtime of this test case to
> 1 second, instead of the 15 seconds it is running now.

While you are correct, the minimum value of the timer is 15s.
Racing that will be quite hard.

@Nico but yes, while you're at it you could switch that around so I 
don't have to explain that a second time.

> 
> It was all like that before, I know. Just some comments ;)
> 

Re: [PATCH kvm-unit-tests 1/2] s390x: diag288: Add missing clobber

[Janosch Frank]

On 12/17/21 14:47, Thomas Huth wrote:
> On 17/12/2021 11.31, Nico Boehr wrote:
>> We clobber r0 and thus should let the compiler know we're doing so.
>>
>> Because we change from basic to extended ASM, we need to change the
>> register names, as %r0 will be interpreted as a token in the assembler
>> template.
>>
>> For consistency, we align with the common style in kvm-unit-tests which
>> is just 0.
>>
>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
>> ---
>>    s390x/diag288.c | 7 ++++---
>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/s390x/diag288.c b/s390x/diag288.c
>> index 072c04a5cbd6..da7b06c365bf 100644
>> --- a/s390x/diag288.c
>> +++ b/s390x/diag288.c
>> @@ -94,11 +94,12 @@ static void test_bite(void)
>>    	/* Arm watchdog */
>>    	lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
>>    	diag288(CODE_INIT, 15, ACTION_RESTART);
>> -	asm volatile("		larl	%r0, 1f\n"
>> -		     "		stg	%r0, 424\n"
>> +	asm volatile("		larl	0, 1f\n"
>> +		     "		stg	0, 424\n"
> 
> Would it work to use %%r0 instead?

Yes, but I told him that looks weird, so that one is on me.
@claudio @thomas What's your preferred way of dealing with this?

> 
>>    		     "0:	nop\n"
>>    		     "		j	0b\n"
>> -		     "1:");
>> +		     "1:"
>> +		     : : : "0");
>>    	report_pass("restart");
>>    }
> 
> Anyway:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Re: [PATCH kvm-unit-tests 1/2] s390x: diag288: Add missing clobber

[Claudio Imbrenda]

On Fri, 17 Dec 2021 15:16:34 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 12/17/21 14:47, Thomas Huth wrote:
> > On 17/12/2021 11.31, Nico Boehr wrote:  
> >> We clobber r0 and thus should let the compiler know we're doing so.
> >>
> >> Because we change from basic to extended ASM, we need to change the
> >> register names, as %r0 will be interpreted as a token in the assembler
> >> template.
> >>
> >> For consistency, we align with the common style in kvm-unit-tests which
> >> is just 0.
> >>
> >> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
> >> ---
> >>    s390x/diag288.c | 7 ++++---
> >>    1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/s390x/diag288.c b/s390x/diag288.c
> >> index 072c04a5cbd6..da7b06c365bf 100644
> >> --- a/s390x/diag288.c
> >> +++ b/s390x/diag288.c
> >> @@ -94,11 +94,12 @@ static void test_bite(void)
> >>    	/* Arm watchdog */
> >>    	lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
> >>    	diag288(CODE_INIT, 15, ACTION_RESTART);
> >> -	asm volatile("		larl	%r0, 1f\n"
> >> -		     "		stg	%r0, 424\n"
> >> +	asm volatile("		larl	0, 1f\n"
> >> +		     "		stg	0, 424\n"  
> > 
> > Would it work to use %%r0 instead?  
> 
> Yes, but I told him that looks weird, so that one is on me.
> @claudio @thomas What's your preferred way of dealing with this?

I would prefer just 0 since that's what we use everywhere else too,
but I won't oppose %%r0 if there are strong arguments for it (but then
we need to decide a policy and stick to it)

> 
> >   
> >>    		     "0:	nop\n"
> >>    		     "		j	0b\n"
> >> -		     "1:");
> >> +		     "1:"
> >> +		     : : : "0");
> >>    	report_pass("restart");
> >>    }  
> > 
> > Anyway:
> > Reviewed-by: Thomas Huth <thuth@redhat.com>
> >   
> 

Re: [PATCH kvm-unit-tests 1/2] s390x: diag288: Add missing clobber

[Thomas Huth]

On 17/12/2021 15.16, Janosch Frank wrote:
> On 12/17/21 14:47, Thomas Huth wrote:
>> On 17/12/2021 11.31, Nico Boehr wrote:
>>> We clobber r0 and thus should let the compiler know we're doing so.
>>>
>>> Because we change from basic to extended ASM, we need to change the
>>> register names, as %r0 will be interpreted as a token in the assembler
>>> template.
>>>
>>> For consistency, we align with the common style in kvm-unit-tests which
>>> is just 0.
>>>
>>> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>
>>> ---
>>>    s390x/diag288.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/s390x/diag288.c b/s390x/diag288.c
>>> index 072c04a5cbd6..da7b06c365bf 100644
>>> --- a/s390x/diag288.c
>>> +++ b/s390x/diag288.c
>>> @@ -94,11 +94,12 @@ static void test_bite(void)
>>>        /* Arm watchdog */
>>>        lc->restart_new_psw.mask = extract_psw_mask() & ~PSW_MASK_EXT;
>>>        diag288(CODE_INIT, 15, ACTION_RESTART);
>>> -    asm volatile("        larl    %r0, 1f\n"
>>> -             "        stg    %r0, 424\n"
>>> +    asm volatile("        larl    0, 1f\n"
>>> +             "        stg    0, 424\n"
>>
>> Would it work to use %%r0 instead?
> 
> Yes, but I told him that looks weird, so that one is on me.
> @claudio @thomas What's your preferred way of dealing with this?

I think it's mostly a matter of taste. I slightly prefer %%r0 to just 0 so 
that it is clear from the first sight that it is a register and not an 
immediate constant.
Additionally, there used to be a problem with older versions of Clang that 
required the %%rX syntax, see:

  https://git.qemu.org/?p=qemu.git;a=commitdiff;h=052b66e7211af6

But we're not supporting those Clang versions for the kvm-unit-tests anyway, 
so that doesn't really count.

Thus, I don't mind too much, if everybody else prefers the bare numbers, 
then let's go with this.

  Thomas