Re: [PATCH for-4.14 1/3] xen/arm: atomic: Allow read_atomic() to be used in more cases

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 07/05/2020 21:34, Stefano Stabellini wrote:
> On Thu, 7 May 2020, Julien Grall wrote:
> > Hi,
> >
> > On 07/05/2020 21:29, Stefano Stabellini wrote:
> > > >    #define read_atomic(p) ({
> > > > \
> > > > -    typeof(*p) __x;                                                     \
> > > > -    switch ( sizeof(*p) ) {                                             \
> > > > -    case 1: __x = (typeof(*p))read_u8_atomic((uint8_t *)p); break;      \
> > > > -    case 2: __x = (typeof(*p))read_u16_atomic((uint16_t *)p); break;    \
> > > > -    case 4: __x = (typeof(*p))read_u32_atomic((uint32_t *)p); break;    \
> > > > -    case 8: __x = (typeof(*p))read_u64_atomic((uint64_t *)p); break;    \
> > > > -    default: __x = 0; __bad_atomic_size(); break;                       \
> > > > -    }                                                                   \
> > > > -    __x;                                                                \
> > > > +    union { typeof(*p) val; char c[0]; } x_;                            \
> > > > +    read_atomic_size(p, x_.c, sizeof(*p));                              \
> > > Wouldn't it be better to pass x_ as follows:
> > >
> > >       read_atomic_size(p, &x_, sizeof(*p));
> > I am not sure to understand this. Are you suggesting to pass a pointer to the
> > union?
> Yes. Would it cause a problem that I couldn't spot?
You defeat the purpose of an union by casting it to something else (even 
if it is void *).
The goal of the union is to be able to access a value in different way 
through a member. So x_.c is more union friendly and makes easier to 
understand why it was implemented like this.
Cheers,

--
Julien Grall