Re: regression from c/s 22071:c5aed2e049bc (ept: Put locks around ept_get_entry) ?

[Keir Fraser <keir@xen.org>]

On 16/12/2010 16:22, "Jan Beulich" <JBeulich@novell.com> wrote:

>> Probably a similar assumption to what we make in x86_64's pte_write_atomic()
>> implementation? Possibly pte_{read,write}_atomic() should cast the pte
>> pointer to volatile, and the EPT reads/writes should be similarly wrapped in
>> macros which do casting. I'm sure we make various other assumptions about
>> read/write atomicity in Xen, but aiming to fix them as we find them is maybe
>> not a bad idea.
>> 
>> If that sounds good, I can propose a patch?
> 
> Oh, yes. I didn't even consider there might be more places.
> 
> What I'm surprised about is you suggesting to take the "volatile"
> route instead of the barrier() one...

I don't think barrier() would solve the problem at hand. The idiom we are
dealing with is something like:
 x = *px;
 [barrier()]
 <mess with fields in x>
 [barrier()]
 *px = x;

I don't see that adding the bracketed barrier() calls above ensures that the
access to *px are done in a single atomic instruction. There's nothing
touching non-local variables between the two barrier()s, so for example the
code that messes with x could be moved after the second barrier() and then
the compiler could choose to mess with *px directly if it wishes.

The issue is not one of serialisation or code ordering. It is one of
memory-access atomicity. Thus it seems to me that volatile is the correct
approach therefore. Perhaps *(volatile type *)px = x or, really, even better
I should define some {read,write}_atomic{8,16,32,64} accessor functions
which use inline asm to absolutely definitely emit a single atomic 'mov'
instruction.

Make sense?

 -- Keir