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

[Tim Deegan <tim@xxxxxxx>]

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

Cheers,

Tim.

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