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

[Heiko Carstens <hca@linux.ibm.com>]

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