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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 9/11/23 6:33 AM, Jan Beulich wrote:
> 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>
>

Thanks.

> 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?
>

Yes. In the case of read_atomic the caller is allowed (expected, even)
to pass in const pointers for `p`, which wouldn't work if a simple
typeof(*(p)) declaration was used since it would inherit the constness
and thus wouldn't be passable to read_atomic_size.

In the case of write_atomic though, there is no need to support const
`p`, so no union is necessary.

> Jan

Thanks,
Shawn