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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.06.17 at 17:19, <andre.przywara@xxxxxxx> wrote:
> 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();
>       }

That's a question to be answered by the parties involved in
commit 9d5617cd89 ("xen/arm: Fix ARM build following c/s
11c397c").

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

Not that I would know of any.

>> 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.

Anyone using deliberately unaligned pointers has to take great
care of what (s)he does with the pointer anyway. Generic
constructs like these are meant to be used on objects which
the compiler would produce without humans breaking basic
assumptions.

> 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)?

As per above - very much yes imo.

Jan


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