On 06/25/2018 07:05 AM, Paolo Bonzini wrote: > On 23/06/2018 02:46, Junaid Shahid wrote: > > Aha, so my confusion here comes before the read of the GPTE is used > after kvm_mmu_sync_pages, but it's a cached value that was read *before* > sp->unsync. If you take the cached read into account, it definitely > looks like the canonical example. The two-CPU case is much easier to > follow. > > So let's put both memory barriers... which unintuitively don't pair with > each other. Something like: > > 1) /* > * When reading sp->unsync after a vmexit, the processor might have > * some SPTEs with SPTE.W=1 cached in the TLB. The vmexit already > * ensures that sp->unsync is read after the TLB fill (x86 doesn't > * reorder reads, and vmentry/vmexit provides a compiler barrier); > * here we ensure that sp->unsync is written before the SPTE. > * Otherwise, a TLB flush might skip the synchronization of the pages > * we're write protecting: <insert example> > */ > smp_wmb(); > > 2) /* > * Read sp->unsync before the spinlock. sp->unsync is written in > * kvm_mmu->page_fault with mmu_lock held; this barrier pairs with > * spin_lock's acquire barrier, which guarantees that the spinlock > * is written before sp->unsync. > */ > smp_rmb(); > > Feel free to make it less verbose. :) > Hmm. I am not really convinced of the necessity of an smp_rmb() between the sp->unsync test and the spin_lock (I assume that is where you mean to place the smp_rmb, correct?), though a READ_ONCE of sp->unsync may be warranted. To illustrate, consider that even if the read of sp->unsync were to happen after the spin_lock, would it really be any different than what used to happen before this change? Thanks, Junaid
