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

[Janosch Frank <frankja@linux.ibm.com>]

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 ;)
>