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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.09.2023 20:10, Shawn Anastasio wrote:
> 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.

Oh, indeed, I was looking at the wrong parameter. I'm sorry for the noise.
That said though, all of this could be avoided if read_atomic_size()
returned a value rather than using a pointer parameter. But that's
something for a future patch, I guess.

Jan