Re: [PATCH 3/9] xen/ppc: Implement atomic.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.08.2023 21:19, Shawn Anastasio wrote:
> On 8/7/23 11:13 AM, Jan Beulich wrote:
>> On 03.08.2023 01:02, Shawn Anastasio wrote:
>>> Implement atomic.h for PPC, based off of the original Xen 3.2
>>> implementation.
>>
>> Since likely that originally came from Linux, did you cross check that
>> Linux hasn't gained any bug fixes in the meantime?
> 
> I did -- the atomic barrier instructions used by linux have changed
> since this code was originally written, so I've updated them to be
> inline with modern linux.

Please mention this in the description then.

>>> +#include <xen/atomic.h>
>>> +
>>> +#include <asm/memory.h>
>>> +#include <asm/system.h>
>>> +
>>> +static inline int atomic_read(const atomic_t *v)
>>> +{
>>> +    return *(volatile int *)&v->counter;
>>> +}
>>> +
>>> +static inline int _atomic_read(atomic_t v)
>>> +{
>>> +    return v.counter;
>>> +}
>>> +
>>> +static inline void atomic_set(atomic_t *v, int i)
>>> +{
>>> +    v->counter = i;
>>> +}
>>> +
>>> +static inline void _atomic_set(atomic_t *v, int i)
>>> +{
>>> +    v->counter = i;
>>> +}
>>> +
>>> +void __bad_atomic_read(const volatile void *p, void *res);
>>> +void __bad_atomic_size(void);
>>> +
>>> +#define build_atomic_read(name, insn, type)                                
>>>     \
>>> +    static inline type name(const volatile type *addr)                     
>>>     \
>>> +    {                                                                      
>>>     \
>>> +        type ret;                                                          
>>>     \
>>> +        asm volatile ( insn "%U1%X1 %0,%1" : "=r"(ret) : "m<>"(*addr) );   
>>>     \
>>
>> As I think I had mentioned before, asm() contraints want a blank between
>> closing quote and opend paren. I.e. like this
>>
>>         asm volatile ( insn "%U1%X1 %0,%1" : "=r" (ret) : "m<>" (*addr) );
>>
> 
> My mistake, I went through and hand-formatted all of this code to try to
> be inline with Xen's style but forgot about the constraints.
> 
> As an aside, I don't suppose there is an automatic formatter somewhere
> that I've missed? I found an old clang-format fork that claims to add
> support for Xen's formatting[1] but it seems to only handle a subset of
> Xen's rules so I haven't found it very useful.
> 
> [1] https://github.com/NastyaVicodin/llvm-project/commits/main

Work there was recently revived, but we're not quite there yet, sorry.

>>> +#define read_atomic(p)                                                     
>>>     \
>>> +    ({                                                                     
>>>     \
>>> +        union {                                                            
>>>     \
>>> +            typeof(*(p)) val;                                              
>>>     \
>>> +            char c[0];                                                     
>>>     \
>>> +        } x_;                                                              
>>>     \
>>> +        read_atomic_size(p, x_.c, sizeof(*(p)));                           
>>>     \
>>> +        x_.val;                                                            
>>>     \
>>> +    })
>>> +
>>> +#define write_atomic(p, x)                                                 
>>>     \
>>> +    do                                                                     
>>>     \
>>> +    {                                                                      
>>>     \
>>> +        typeof(*(p)) x_ = (x);                                             
>>>     \
>>> +        write_atomic_size(p, &x_, sizeof(*(p)));                           
>>>     \
>>> +    } while ( 0 )
>>
>> Up to here you use underscore-suffixed locals, but then ...
>>
>>> +#define add_sized(p, x)                                                    
>>>     \
>>> +    ({                                                                     
>>>     \
>>> +        typeof(*(p)) __x = (x);                                            
>>>     \
>>
>> ... you have even two prefixing underscores here.
>>
> 
> The definitions of these macros were directly copied from elsewhere in
> Xen (x86 and arm). I can change them all to use underscore-suffixed
> local naming here, though.

We're still far away from eliminating all pre-existing issues. So copying
existing code is always at risk of then getting such comments. Again -
sorry for that, but otherwise we'll grow the number of issues.

>>> +        switch ( sizeof(*(p)) )                                            
>>>     \
>>> +        {                                                                  
>>>     \
>>> +        case 1:                                                            
>>>     \
>>> +            add_u8_sized((uint8_t *) (p), __x);                            
>>>     \
>>> +            break;                                                         
>>>     \
>>> +        case 2:                                                            
>>>     \
>>> +            add_u16_sized((uint16_t *) (p), __x);                          
>>>     \
>>> +            break;                                                         
>>>     \
>>> +        case 4:                                                            
>>>     \
>>> +            add_u32_sized((uint32_t *) (p), __x);                          
>>>     \
>>> +            break;                                                         
>>>     \
>>> +        default:                                                           
>>>     \
>>> +            __bad_atomic_size();                                           
>>>     \
>>> +            break;                                                         
>>>     \
>>> +        }                                                                  
>>>     \
>>> +    })
>>> +
>>> +static inline void atomic_add(int a, atomic_t *v)
>>> +{
>>> +    int t;
>>> +
>>> +    asm volatile ( "1: lwarx %0,0,%3\n"
>>> +                   "add %0,%2,%0\n"
>>> +                   "stwcx. %0,0,%3\n"
>>> +                   "bne- 1b"
>>> +                   : "=&r"(t), "=m"(v->counter)
>>> +                   : "r"(a), "r"(&v->counter), "m"(v->counter) : "cc" );
>>
>> "+m" and then drop the last input?
>>
> 
> Yes, that makes sense. Not sure why it was originally written that way
> but I'll change it.

I think this was attributed to not sufficiently well written documentation
for older gcc.

>>> +static inline int __atomic_add_unless(atomic_t *v, int a, int u)
>>> +{
>>> +   int c, old;
>>> +
>>> +   c = atomic_read(v);
>>> +   while (c != u && (old = atomic_cmpxchg((v), c, c + a)) != c)
>>
>> Btw, no real need to parenthesize v in cases like this one. Otoh a needs
>> parenthesizing.
> 
> FWIW this was copied directly from arm64/atomic.h, but seeing as its a
> normal function and not a macro I'm not sure I see why `a` would need
> parenthesis.

Oh, right you are. The parenthesized v misled me.

Jan