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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 9/12/23 1:48 AM, Jan Beulich wrote:
> 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).
>

Yes, however the second parameter to read_atomic_size is a void *. If
read_atomic macro's local `x_` was declared with a simple typeof(*(p))
then it would inherit the const and thus couldn't be passed as the
second parameter to read_atomic_size.

> Jan

Thanks,
Shawn