At 01:48 -0600 on 17 Aug (1502934495), Jan Beulich wrote: > >>> On 16.08.17 at 18:47, <andrew.coop...@citrix.com> wrote: > > On 16/08/17 16:23, Jan Beulich wrote: > >>>>> On 16.08.17 at 13:22, <andrew.coop...@citrix.com> 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@lists.xen.org https://lists.xen.org/xen-devel
