Re: [PATCH 5/5] s390/uaccess: add cmpxchg_user_key()

[Janis Schoetterl-Glausch]

On Wed, 2022-11-09 at 23:24 +0100, Heiko Carstens wrote:
> On Wed, Nov 09, 2022 at 04:46:29PM +0100, Janis Schoetterl-Glausch wrote:
> > On Wed, 2022-11-02 at 15:19 +0100, Heiko Carstens wrote:
> > > +	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?
> 
> I fail to parse what you are trying to say. Please elaborate.
> 
> > The loop condition being imprecise seems non-ideal.
> 
> What exactly is imprecise?

The loop retries the CS if bits outside the target byte changed instead
of retrying until the target byte differs from the old value.
So if you attempt to exchange (prev_left_0 old_byte prev_right_0) and 
that fails because the word at the address is (prev_left_1 x prev_right_1)
where both x != old_byte and one of the prev_*_1 values differs from the respective
prev_*_0 value, the CS is retried. If there were a native 1 byte compare and swap,
the exchange would just fail here. Instead the loop retries the CS until the margin
values are stable and it can infer from that that the CS failed because of the target value.
(Assuming that doesn't change to the old_byte value.)

It's not a problem, but it struck me as non-ideal, which is why for v2 I inverted the mask
after using it to punch the hole for the old/new values.
Then you can use it to test if bits inside the target byte differ.

That's why I asked about cmpxchg.h. If you don't want non-cosmetic changes to the existing
cmpxchg function and consistency of the new key checked function, then obviously the loop
condition needs to be the same.
> 
> > > +			  [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.
> 
> That's not only inconsistent, but also a bug.
> Thank you for pointing this out. Will be fixed.

Well, you could pass in the shifted key as argument, but yeah.