Re: [PATCH v4 1/5] xen/ppc: Implement atomic.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2023 00:50, Shawn Anastasio wrote:
> Implement atomic.h for PPC, based off of the original Xen 3.2
> implementation. This implementation depends on some functions that are
> not yet defined (notably __cmpxchg), so it can't yet be used.
> 
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Just one remaining question:

> +static always_inline void read_atomic_size(const volatile void *p, void *res,
> +                                           unsigned int size)
> +{
> +    ASSERT(IS_ALIGNED((vaddr_t)p, size));
> +    switch ( size )
> +    {
> +    case 1:
> +        *(uint8_t *)res = read_u8_atomic(p);
> +        break;
> +    case 2:
> +        *(uint16_t *)res = read_u16_atomic(p);
> +        break;
> +    case 4:
> +        *(uint32_t *)res = read_u32_atomic(p);
> +        break;
> +    case 8:
> +        *(uint64_t *)res = read_u64_atomic(p);
> +        break;
> +    default:
> +        __bad_atomic_read(p, res);
> +        break;
> +    }
> +}
> +
> +static always_inline void write_atomic_size(volatile void *p, const void 
> *val,
> +                                            unsigned int size)
> +{
> +    ASSERT(IS_ALIGNED((vaddr_t)p, size));
> +    switch ( size )
> +    {
> +    case 1:
> +        write_u8_atomic(p, *(const uint8_t *)val);
> +        break;
> +    case 2:
> +        write_u16_atomic(p, *(const uint16_t *)val);
> +        break;
> +    case 4:
> +        write_u32_atomic(p, *(const uint32_t *)val);
> +        break;
> +    case 8:
> +        write_u64_atomic(p, *(const uint64_t *)val);
> +        break;
> +    default:
> +        __bad_atomic_size();
> +        break;
> +    }
> +}

These two functions being as similar as is possible, ...

> +#define read_atomic(p)                                                       
>   \
> +    ({                                                                       
>   \
> +        union {                                                              
>   \
> +            typeof(*(p)) val;                                                
>   \
> +            char c[sizeof(*(p))];                                            
>   \
> +        } x_;                                                                
>   \
> +        read_atomic_size(p, x_.c, sizeof(*(p)));                             
>   \
> +        x_.val;                                                              
>   \
> +    })

... is there a reason you keep using a union here ...

> +#define write_atomic(p, x)                                                   
>   \
> +    do                                                                       
>   \
> +    {                                                                        
>   \
> +        typeof(*(p)) x_ = (x);                                               
>   \
> +        write_atomic_size(p, &x_, sizeof(*(p)));                             
>   \
> +    } while ( 0 )

... while you did away with its use here?

Jan