Re: [PATCH v3 13/34] xen/riscv: introduce cmpxchg.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-01-23 at 14:27 +0100, Jan Beulich wrote:
> On 23.01.2024 13:18, Oleksii wrote:
> > On Tue, 2024-01-23 at 11:28 +0100, Jan Beulich wrote:
> > > On 23.01.2024 11:15, Oleksii wrote:
> > > > On Mon, 2024-01-22 at 17:27 +0100, Jan Beulich wrote:
> > > > > On 22.12.2023 16:12, Oleksii Kurochko wrote:
> > > > > > +static inline unsigned long __xchg(volatile void *ptr,
> > > > > > unsigned
> > > > > > long x, int size)
> > > > > > +{
> > > > > > +    switch (size) {
> > > > > > +    case 1:
> > > > > > +        return __cmpxchg_case_1(ptr, (uint32_t)-1, x);
> > > > > > +    case 2:
> > > > > > +        return __cmpxchg_case_2(ptr, (uint32_t)-1, x);
> > > > > 
> > > > > How are these going to work? You'll compare against ~0, and
> > > > > if
> > > > > the
> > > > > value
> > > > > in memory isn't ~0, memory won't be updated; you will only
> > > > > (correctly)
> > > > > return the value found in memory.
> > > > > 
> > > > > Or wait - looking at __cmpxchg_case_{1,2}() far further down,
> > > > > you
> > > > > ignore
> > > > > "old" there. Which apparently means they'll work for the use
> > > > > here,
> > > > > but
> > > > > not for the use in __cmpxchg().
> > > > Yes, the trick is that old is ignored and is read in
> > > > __emulate_cmpxchg_case1_2() before __cmpxchg_case_4 is called:
> > > >     do
> > > > {                                                              
> > > >         read_val =
> > > > read_func(aligned_ptr);                            
> > > >         swapped_new = read_val &
> > > > ~mask;                               
> > > >         swapped_new |=
> > > > masked_new;                                    
> > > >         ret = cmpxchg_func(aligned_ptr, read_val,
> > > > swapped_new);       
> > > >     } while ( ret != read_val
> > > > );                                      
> > > > read_val it is 'old'.
> > > > 
> > > > But now I am not 100% sure that it is correct for __cmpxchg...
> > > 
> > > It just can't be correct - you can't ignore "old" there. I think
> > > you
> > > want simple cmpxchg primitives, which xchg then uses in a loop
> > > (while
> > > cmpxchg uses them plainly).
> > But xchg doesn't require 'old' value, so it should be ignored in
> > some
> > way by cmpxchg.
> 
> Well, no. If you have only cmpxchg, I think your only choice is - as
> said - to read the old value and then loop over cmpxchg until that
> succeeds. Not really different from other operations which need
> emulating using cmpxchg.
Then it looks like the main error in __emulate_cmpxchg_case1_2 is that
I read the value each time, so read_val = read_func(aligned_ptr); 
should be before the do {...} while(). Also, it would be better to
rename it to old_val or just old.

> 
> > > > > > +static always_inline unsigned short
> > > > > > __cmpxchg_case_2(volatile
> > > > > > uint32_t *ptr,
> > > > > > +                                                    
> > > > > > uint32_t
> > > > > > old,
> > > > > > +                                                    
> > > > > > uint32_t
> > > > > > new)
> > > > > > +{
> > > > > > +    (void) old;
> > > > > > +
> > > > > > +    if (((unsigned long)ptr & 3) == 3)
> > > > > > +    {
> > > > > > +#ifdef CONFIG_64BIT
> > > > > > +        return __emulate_cmpxchg_case1_2((uint64_t *)ptr,
> > > > > > new,
> > > > > > +                                         readq,
> > > > > > __cmpxchg_case_8,
> > > > > > 0xffffU);
> > > > > 
> > > > > What if ((unsigned long)ptr & 7) == 7 (which is a sub-case of
> > > > > what
> > > > > the
> > > > > if() above checks for? Isn't it more reasonable to require
> > > > > aligned
> > > > > 16-bit quantities here? Or if mis-aligned addresses are okay,
> > > > > you
> > > > > could
> > > > > as well emulate using __cmpxchg_case_4().
> > > > Yes, it will be more reasonable. I'll use IS_ALIGNED instead.
> > > 
> > > Not sure I get your use of "instead" here correctly. There's more
> > > to change here than just the if() condition.
> > I meant something like:
> > 
> > if ( IS_ALIGNED(ptr, 16) )
> >     __emulate_cmpxchg_case1_2(...);
> > else
> >     assert_failed("ptr isn't aligned\n");
> 
> Except that you'd better not use assert_failed() directly anywhere,
> and the above is easier as
> 
>     ASSERT(IS_ALIGNED(ptr, 16));
>     __emulate_cmpxchg_case1_2(...);
> 
> anyway (leaving aside that I guess you mean 2, not 16).
Yeah, it should be 2. Thanks.

~ Oleksii