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] /* * 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. */
