| Message ID | 20200502160700.19573-2-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework {read, write}_atomic() | expand |
On Sat, 2 May 2020, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > The current implementation of read_atomic() on Arm will not allow to: > 1) Read a value from a pointer to const because the temporary > variable will be const and therefore it is not possible to assign > any value. This can be solved by using a union between the type and > a char[0]. > 2) Read a pointer value (e.g void *) because the switch contains > cast from other type than the size of a pointer. This can be solved by > by introducing a static inline for the switch and use void * for the > pointer. > > Reported-by: Juergen Gross <jgross@suse.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > xen/include/asm-arm/atomic.h | 37 +++++++++++++++++++++++++++--------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h > index e81bf80e305c..3c3d6bb04ee8 100644 > --- a/xen/include/asm-arm/atomic.h > +++ b/xen/include/asm-arm/atomic.h > @@ -71,18 +71,37 @@ build_add_sized(add_u32_sized, "", WORD, uint32_t) > #undef build_atomic_write > #undef build_add_sized > > +void __bad_atomic_read(const volatile void *p, void *res); > void __bad_atomic_size(void); > > +static always_inline void read_atomic_size(const volatile void *p, > + void *res, > + unsigned int 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; > + } > +} > + > #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)); ? In my tests both ways works. I prefer the version with &x_ but given that it works either way: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > + x_.val; \ > }) > > #define write_atomic(p, x) ({ \ > -- > 2.17.1 >
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? Cheers,
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?
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,
diff --git a/xen/include/asm-arm/atomic.h b/xen/include/asm-arm/atomic.h index e81bf80e305c..3c3d6bb04ee8 100644 --- a/xen/include/asm-arm/atomic.h +++ b/xen/include/asm-arm/atomic.h @@ -71,18 +71,37 @@ build_add_sized(add_u32_sized, "", WORD, uint32_t) #undef build_atomic_write #undef build_add_sized +void __bad_atomic_read(const volatile void *p, void *res); void __bad_atomic_size(void); +static always_inline void read_atomic_size(const volatile void *p, + void *res, + unsigned int 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; + } +} + #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)); \ + x_.val; \ }) #define write_atomic(p, x) ({ \