On Thu, 21 May 2026 at 14:31, Sean Christopherson <[email protected]> wrote: > > 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). > > 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]
This would fix a few related issues at once. sgtm /fuad /fuad > > /* > * Lookup the mapping level for @gfn in the current mm. > * > * WARNING! Use of host_pfn_mapping_level() requires the caller and the end > * consumer to be tied into KVM's handlers for MMU notifier events! > * > * There are several ways to safely use this helper: > * > * - Check mmu_invalidate_retry_gfn() after grabbing the mapping level, before > * consuming it. In this case, mmu_lock doesn't need to be held during the > * lookup, but it does need to be held while checking the MMU notifier. > * > * - Hold mmu_lock AND ensure there is no in-progress MMU notifier > invalidation > * event for the hva. This can be done by explicit checking the MMU > notifier > * or by ensuring that KVM already has a valid mapping that covers the hva. > * > * - Do not use the result to install new mappings, e.g. use the host mapping > * level only to decide whether or not to zap an entry. In this case, it's > * not required to hold mmu_lock (though it's highly likely the caller will > * want to hold mmu_lock anyways, e.g. to modify SPTEs). > * > * Note! The lookup can still race with modifications to host page tables, > but > * the above "rules" ensure KVM will not _consume_ the result of the walk if a > * race with the primary MMU occurs. > */
