Sean Christopherson <[email protected]> writes: > On Thu, May 21, 2026, Fuad Tabba wrote: >> On Wed, 20 May 2026 at 22:44, Ackerley Tng <[email protected]> wrote: >> > >> > Fuad Tabba <[email protected]> writes: >> > >> > > >> > > [...snip...] >> > > >> > >> +unsigned long kvm_gmem_get_memory_attributes(struct kvm *kvm, gfn_t >> > >> gfn) >> > >> +{ >> > >> + struct kvm_memory_slot *slot = gfn_to_memslot(kvm, gfn); >> > >> + struct inode *inode; >> > >> + >> > >> + /* >> > >> + * If this gfn has no associated memslot, there's no chance of >> > >> the gfn >> > >> + * being backed by private memory, since guest_memfd must be >> > >> used for >> > >> + * private memory, and guest_memfd must be associated with some >> > >> memslot. >> > >> + */ >> > >> + if (!slot) >> > >> + return 0; >> > >> + >> > >> + CLASS(gmem_get_file, file)(slot); >> > >> + if (!file) >> > >> + return 0; >> > >> + >> > >> + inode = file_inode(file); >> > >> + >> > >> + /* >> > >> + * Rely on the maple tree's internal RCU lock to ensure a >> > >> + * stable result. This result can become stale as soon as the >> > >> + * lock is dropped, so the caller _must_ still protect >> > >> + * consumption of private vs. shared by checking >> > >> + * mmu_invalidate_retry_gfn() under mmu_lock to serialize >> > >> + * against ongoing attribute updates. >> > >> + */ >> > >> + return kvm_gmem_get_attributes(inode, kvm_gmem_get_index(slot, >> > >> gfn)); >> > >> +} >> > > >> > > Doesn't this imply that all consumers of kvm_mem_is_private() should >> > > validate the result using mmu_lock and the invalidation sequence? >> > >> > Let me know how I can improve the comment. >> >> Given Sean's context, the comment is good I think. I would quibble >> with the the "_must_ still protect" phrasing being a bit too strict. >> >> Maybe just soften it slightly to acknowledge the exception? Something like: >> >> * lock is dropped, so callers that require a strict result _must_ protect >> * consumption of private vs. shared by checking mmu_invalidate_retry_gfn() >> * under mmu_lock to serialize against ongoing attribute updates. Callers >> * doing lockless reads must be able to tolerate a stale result. >> >> That aligns the comment with how KVM is actually using it today. That >> said, this is nitpicking. Feel free to use or ignore. > > Hmm, I wonder if we can figure out a way to consolidate some documentation, > because this is _exactly_ the same pattern that x86's host_pfn_mapping_level() > deals with (see its big comment below). >
This would be great, are you thinking an actual comment or something in Documentation/? Perhaps we could iterate on this a little with me providing the newbie perspective. Do you want me to take a stab at writing something up? > There's also the stale comment in kvm_invalidate_memslot(), which, stating the > obvious, speaks to the memslot+SRCU side of things. > > Maybe it makes sense to to find a central location for one giant comment about > how how MMU notifier events and memslot+SRCU protections work? And then refer > to that in paths where some asset needs to be tied into MMU notifiers and/or > memslots+SRCU? > > [*] https://lore.kernel.org/all/[email protected] > > [...snip...] >
