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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.09.2023 01:56, Shawn Anastasio wrote:
> On 9/11/23 6:33 AM, Jan Beulich wrote:
>> On 09.09.2023 00:50, Shawn Anastasio wrote:
>>> +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.

But the helper function's first parameter is const volatile void *, and
while you can't assign to a const variable, such a variable can of course
have an initializer (like you have it in the write case).

Jan