Re: [RFC PATCH 14/25] qemu/bswap: Introduce load/store for aligned pointer

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

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é <philmd@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]