On Mon, May 19, 2025, James Houghton wrote: > On Mon, May 19, 2025 at 9:37 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > On Sat, May 17, 2025, Paolo Bonzini wrote: > > > On 5/16/25 23:54, Sean Christopherson wrote: > > > > + /* > > > > + * Write mmu_page_hash exactly once as there may be concurrent readers, > > > > + * e.g. to check for shadowed PTEs in mmu_try_to_unsync_pages(). Note, > > > > + * mmu_lock must be held for write to add (or remove) shadow pages, and > > > > + * so readers are guaranteed to see an empty list for their current > > > > + * mmu_lock critical section. > > > > + */ > > > > + WRITE_ONCE(kvm->arch.mmu_page_hash, h); > > > > > > Use smp_store_release here (unlike READ_ONCE(), it's technically incorrect > > > to use WRITE_ONCE() here!), > > > > Can you elaborate why? Due to my x86-centric life, my memory ordering knowledge > > is woefully inadequate. > > The compiler must be prohibited from reordering stores preceding this > WRITE_ONCE() to after it. > > In reality, the only stores that matter will be from within > kvcalloc(), and the important bits of it will not be inlined, so it's > unlikely that the compiler would actually do such reordering. But it's > nonetheless allowed. :) barrier() is precisely what is needed to > prohibit this; smp_store_release() on x86 is merely barrier() + > WRITE_ONCE(). > > Semantically, smp_store_release() is what you mean to write, as Paolo > said. We're not really *only* preventing torn accesses, we also need > to ensure that any threads that read kvm->arch.mmu_page_hash can > actually use that result (i.e., that all the stores from the > kvcalloc() are visible). Ah, that's what I was missing. It's not KVM's stores that are theoretically problematic, it's the zeroing of kvm->arch.mmu_page_hash that needs protection. Thanks! > This sounds a little bit weird for x86 code, but compiler reordering is still > a possibility. > > And I also agree with what Paolo said about smp_load_acquire(). :) > > Thanks Paolo. Please correct me if I'm wrong above. > > > > with a remark that it pairs with kvm_get_mmu_page_hash(). That's both more > > > accurate and leads to a better comment than "write exactly once". > >
