On Wed, May 19, 2021 at 07:56:51AM +0200, Philippe Mathieu-Daudé wrote: > On 5/18/21 10:15 PM, Peter Maydell wrote: > > On Tue, 18 May 2021 at 19:38, Philippe Mathieu-Daudé <phi...@redhat.com> > > wrote: > >> > >> When the pointer alignment is known to be safe, we can > >> directly swap the data in place, without having to rely > >> on the compiler builtin code. > >> > >> Load/store methods expecting aligned pointer use the 'a' > >> infix. For example to read a 16-bit unsigned value stored > >> in little endianess at an unaligned pointer: > >> > >> val = lduw_le_p(&unaligned_ptr); > >> > >> then to store it in big endianess at an aligned pointer: > >> > >> stw_be_ap(&aligned_ptr, val); > > > > It sounded from the bug report as if the desired effect > > was "this access is atomic". Nothing in the documentation here > > makes that guarantee of the implementation -- it merely imposes > > an extra requirement on the caller that the pointer alignment > > is "safe" (which term it does not define...) and a valid > > implementation would be to implement the "aligned" versions > > identically to the "unaligned" versions... > > > > Q: should the functions at the bottom of this stack of APIs > > be using something from the atomic.h header? If not, why not? > > Do we need any of the other atomic primitives ? > > I'll defer this question to Stefan/Paolo...
Stricly speaking qatomic_read() and qatomic_set() are necessary for __ATOMIC_RELAXED semantics. The comment in atomic.h mentions this generally has no effect on generated code. That's probably because aligned 16/32/64-bit accesses don't tear on modern CPUs so no special instructions are needed. This is why not using atomic.h usually works. Even qatomic_read()/qatomic_set() is too strong since the doc comment mentions it prevents the compiler moving other loads/stores past the atomic operation. The hw/virtio/ code already has explicit memory barriers and doesn't need the additional compiler barrier. For safety I suggest using qatomic_read()/qatomic_set(). Stefan
signature.asc
Description: PGP signature
