Re: [PATCH] KVM: s390: get rid of register asm usage

[Thomas Huth <thuth@redhat.com>]

On 21/06/2021 16.03, Heiko Carstens wrote:
> Using register asm statements has been proven to be very error prone,
> especially when using code instrumentation where gcc may add function
> calls, which clobbers register contents in an unexpected way.
> 
> Therefore get rid of register asm statements in kvm code, even though
> there is currently nothing wrong with them. This way we know for sure
> that this bug class won't be introduced here.
> 
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>   arch/s390/kvm/kvm-s390.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 1296fc10f80c..4b7b24f07790 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -329,31 +329,31 @@ static void allow_cpu_feat(unsigned long nr)
>   
>   static inline int plo_test_bit(unsigned char nr)
>   {
> -	register unsigned long r0 asm("0") = (unsigned long) nr | 0x100;
> +	unsigned long function = (unsigned long) nr | 0x100;
>   	int cc;
>   
>   	asm volatile(
> +		"	lgr	0,%[function]\n"
>   		/* Parameter registers are ignored for "test bit" */
>   		"	plo	0,0,0,0(0)\n"
>   		"	ipm	%0\n"
>   		"	srl	%0,28\n"
>   		: "=d" (cc)
> -		: "d" (r0)
> -		: "cc");
> +		: [function] "d" (function)
> +		: "cc", "0");
>   	return cc == 0;
>   }
>   
>   static __always_inline void __insn32_query(unsigned int opcode, u8 *query)
>   {
> -	register unsigned long r0 asm("0") = 0;	/* query function */
> -	register unsigned long r1 asm("1") = (unsigned long) query;
> -
>   	asm volatile(
> -		/* Parameter regs are ignored */
> +		"	lghi	0,0\n"
> +		"	lgr	1,%[query]\n"
> +		/* Parameter registers are ignored */
>   		"	.insn	rrf,%[opc] << 16,2,4,6,0\n"
>   		:
> -		: "d" (r0), "a" (r1), [opc] "i" (opcode)
> -		: "cc", "memory");
> +		: [query] "d" ((unsigned long)query), [opc] "i" (opcode)

Wouldn't it be better to keep the "a" constraint instead of "d" to avoid 
that the compiler ever passes the "query" value in r0 ?
Otherwise the query value might get trashed if it is passed in r0...

> +		: "cc", "memory", "0", "1");
>   }

With "a" instead of "d":

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