Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f604b6038c4a8bad5123e1f1f14b15c2190f28e9.camel@linux.ibm.com>
Subject: Re: [PATCH 5/5] s390/uaccess: add cmpxchg_user_key()
From:   Janis Schoetterl-Glausch <scgl@linux.ibm.com>
To:     Heiko Carstens <hca@linux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>, kvm@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-s390@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Shuah Khan <shuah@kernel.org>,
        Sven Schnelle <svens@linux.ibm.com>
Date:   Wed, 09 Nov 2022 16:46:29 +0100
In-Reply-To: <Y2J8axs+bcQ2dO/l@osiris>
References: <20221012205609.2811294-1-scgl@linux.ibm.com>
         <20221012205609.2811294-2-scgl@linux.ibm.com> <Y2J61LWSV+HolIeT@osiris>
         <Y2J8axs+bcQ2dO/l@osiris>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Precedence: bulk

On Wed, 2022-11-02 at 15:19 +0100, Heiko Carstens wrote:
> Add cmpxchg_user_key() which allows to execute a compare and exchange
> on a user space address. This allows also to specify a storage key
> which makes sure that key-controlled protection is considered.
> 
> This is based on a patch written by Janis Schoetterl-Glausch.
> 
> Link: https://lore.kernel.org/all/20220930210751.225873-2-scgl@linux.ibm.com
> Cc: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>  arch/s390/include/asm/uaccess.h | 183 ++++++++++++++++++++++++++++++++
>  1 file changed, 183 insertions(+)
> 
> diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
> index f7038b800cc3..9bbdecb80e06 100644
> --- a/arch/s390/include/asm/uaccess.h
> +++ b/arch/s390/include/asm/uaccess.h
> @@ -390,4 +390,187 @@ do {									\
>  		goto err_label;						\
>  } while (0)
>  
> +void __cmpxchg_user_key_called_with_bad_pointer(void);
> +
> +static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
> +					      __uint128_t old, __uint128_t new,
> +					      unsigned long key, int size)
> +{
> +	int rc = 0;
> +
> +	switch (size) {
> +	case 1: {
> +		unsigned int prev, tmp, shift;
> +
> +		shift = (3 ^ (address & 3)) << 3;
> +		address ^= address & 3;
> +		asm volatile(
> +			"	spka	0(%[key])\n"
> +			"	sacf	256\n"
> +			"0:	l	%[prev],%[address]\n"
> +			"1:	nr	%[prev],%[mask]\n"
> +			"	lr	%[tmp],%[prev]\n"
> +			"	or	%[prev],%[old]\n"
> +			"	or	%[tmp],%[new]\n"
> +			"2:	cs	%[prev],%[tmp],%[address]\n"
> +			"3:	jnl	4f\n"
> +			"	xr	%[tmp],%[prev]\n"
> +			"	nr	%[tmp],%[mask]\n"

Are you only entertaining cosmetic changes to cmpxchg.h?
The loop condition being imprecise seems non-ideal.

> +			"	jnz	1b\n"
> +			"4:	sacf	768\n"
> +			"	spka	%[default_key]\n"
> +			EX_TABLE_UA_LOAD_REG(0b, 4b, %[rc], %[prev])
> +			EX_TABLE_UA_LOAD_REG(1b, 4b, %[rc], %[prev])
> +			EX_TABLE_UA_LOAD_REG(2b, 4b, %[rc], %[prev])
> +			EX_TABLE_UA_LOAD_REG(3b, 4b, %[rc], %[prev])
> +			: [rc] "+&d" (rc),
> +			  [prev] "=&d" (prev),
> +			  [tmp] "=&d" (tmp),
> +			  [address] "+Q" (*(int *)address)
> +			: [old] "d" (((unsigned int)old & 0xff) << shift),
> +			  [new] "d" (((unsigned int)new & 0xff) << shift),
> +			  [mask] "d" (~(0xff << shift)),
> +			  [key] "a" (key),

Why did you get rid of the << 4 shift?
That's inconsistent with the other uaccess functions that take an access key.

> +			  [default_key] "J" (PAGE_DEFAULT_KEY)
> +			: "memory", "cc");
> +		*(unsigned char *)uval = prev >> shift;
> +		return rc;
> +	}

[...]