Re: [Xen-devel] [PATCH v2 2/4] xen/x86: Drop unnecessary barriers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.08.17 at 16:47, <tim@xxxxxxx> wrote:
> At 01:48 -0600 on 17 Aug (1502934495), Jan Beulich wrote:
>> >>> On 16.08.17 at 18:47, <andrew.cooper3@xxxxxxxxxx> wrote:
>> > On 16/08/17 16:23, Jan Beulich wrote:
>> >>>>> On 16.08.17 at 13:22, <andrew.cooper3@xxxxxxxxxx> wrote:
>> >>> --- a/xen/arch/x86/mm/shadow/multi.c
>> >>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> >>> @@ -3112,7 +3112,6 @@ static int sh_page_fault(struct vcpu *v,
>> >>>       * will make sure no inconsistent mapping being translated into
>> >>>       * shadow page table. */
>> >>>      version = atomic_read(&d->arch.paging.shadow.gtable_dirty_version);
>> >>> -    rmb();
>> >>>      walk_ok = sh_walk_guest_tables(v, va, &gw, error_code);
>> >> Isn't this supposed to make sure version is being read first? I.e.
>> >> doesn't this at least need to be barrier()?
>> > 
>> > atomic_read() is not free to be reordered by the compiler.  It is an asm
>> > volatile with a volatile memory reference.
>> 
>> Oh, right - I did forget about the volatiles there (since generally,
>> like in Linux, we appear to try to avoid volatile).
> 
> FWIW, I don't think that's quite right.  The GCC docs I have say that
> "volatile" will stop the compiler from omitting an asm altogether, or
> hoisting it out of a loop (on the assumption that it will always
> produce the same output for the same inputs).  And that "the compiler
> can move even volatile 'asm' instructions relative to other code,
> including across jump instructions."

Oh, I had talked about the volatile qualifiers, no the volatile asm-s.

Jan


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