Re: [Xen-devel] read_atomic, write_atomic, add_sized

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 12/06/17 11:55, Jan Beulich wrote:
>>>> On 12.06.17 at 12:24, <julien.grall@xxxxxxx> wrote:
>> I am trying to understand why we decided to implement the helpers 
>> read_atomic, write_atomic, add_sized in assembly rather than directly in C.
>>
>> AFAICT implementation in C similar to Linux helpers WRITE_ONCE/READ_ONCE 
>> would work here. Did I miss anything?
> 
> For one at least our current ACCESS_ONCE() doesn't allow non-
> scalar types to be read/written, whereas {read,write}_atomic()
> solely look at sizeof().

What is the rationale behind this?
I would assume the ACCESS_ONCE() does not care about atomicity, it is
basically a pretty version of a volatile cast to tell the compiler to
cache the value (and not re-read at will). And so it should be able to
read an arbitrarily sized data structure, one chunk after the other, but
only each chunk once? This is what Linux supports, at least:
        switch (size) {
        ...
        default:
                barrier();
                __builtin_memcpy((void *)res, (const void *)p, size);
                barrier();
        }

Or does ACCESS_ONCE() cover more than just this volatile property?

> Plus ACCESS_ONCE() doesn't enforce a
> single instruction to be used in the resulting assembly - while the
> compiler may not fold multiple accesses, it still may break them
> up if it wishes to (but of course it usually won't it the whole thing
> can be expressed with a single instruction).

So looking at the implementation of our single-instruction read_atomic()
implementation, I can't find one point:
All of i386, x86_64, arm and arm64 seem to guarantee atomicity for a
native data type *only* if the address is naturally aligned.
"Newer" x86 CPUs seems to guarantee atomicity even for unaligned
addresses when the data hits and fits in the cache, but I think this is
nothing we can check for or rely on.
So I don't see how this alignment is enforced here. I took code snippets
from atomic.h and compiled a test case, and indeed passing in a pointer
to an unaligned uint32_t (from a packed struct) resulted in a single,
but unaligned access:
        ldr     x3, [x29,#57]
and:
        mov    0x11(%rsp),%edx

x29 is the frame pointer, so it' strictly aligned as it's derived from
the stack pointer, which makes x29 + 57 not aligned.

So read_atomic() is not atomic in this case.

I don't see an simple and generic way out of this, as there does not
seem to be an easy (or cheap) way of checking for unaligned pointers at
compile time (although I think the compiler could know this).

Can we live with silently assuming aligned variables in the Xen code
(due to ABI requirements)? That wouldn't cover crafted or computed
pointers, but chances are we use &some_var anyway as a *_atomic() argument?

Cheers,
Andre.

> For add_sized() it's even worse: The macro enforces (on x86) an
> operation on a memory operand (i.e. again a single instruction).
> "ACCESS_ONCE(x) += n", otoh, may (and iirc normally will) be
> translated to a memory of x, addition of the read value with n,
> and a memory write.
> 
>> Note that the naming is also confusing as it is easily to mix with the 
>> atomic_read, atomic_write helpers.
> 
> Well, yes, unfortunately.
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel