On Fri, May 07, 2021, Paolo Bonzini wrote: > On 06/05/21 20:42, Ben Gardon wrote: > > In preparation for lazily allocating the rmaps when the TDP MMU is in > > use, protect the rmaps with SRCU. Unfortunately, this requires > > propagating a pointer to struct kvm around to several functions. > > Thinking more about it, this is not needed because all reads of the rmap > array are guarded by the load-acquire of kvm->arch.memslots_have_rmaps. > That is, the pattern is always > > if (!load-acquire(memslot_have_rmaps)) > return; > ... = __gfn_to_rmap(...) > > slots->arch.rmap[x] = ... > store-release(memslot_have_rmaps, true) > > where the load-acquire/store-release have the same role that > srcu_dereference/rcu_assign_pointer had before this patch. > > We also know that any read that misses the check has the potential for a > NULL pointer dereference, so it *has* to be like that. > > That said, srcu_dereference has zero cost unless debugging options are > enabled, and it *is* true that the rmap can disappear if kvm->srcu is not > held, so I lean towards keeping this change and just changing the commit > message like this: > > --------- > Currently, rmaps are always allocated and published together with a new > memslot, so the srcu_dereference for the memslots array already ensures that > the memory pointed to by slots->arch.rmap is zero at the time > slots->arch.rmap. However, they still need to be accessed in an SRCU > read-side critical section, as the whole memslot can be deleted outside > SRCU. > -------- I disagree, sprinkling random and unnecessary __rcu/SRCU annotations does more harm than good. Adding the unnecessary tag could be quite misleading as it would imply the rmap pointers can _change_ independent of the memslots. Similary, adding rcu_assign_pointer() in alloc_memslot_rmap() implies that its safe to access the rmap after its pointer is assigned, and that's simply not true since an rmap array can be freed if rmap allocation for a different memslot fails. Accessing the rmap is safe if and only if all rmaps are allocated, i.e. if arch.memslots_have_rmaps is true, as you pointed out. Furthermore, to actually gain any protection from SRCU, there would have to be an synchronize_srcu() call after assigning the pointers, and that _does_ have an associated. Not to mention that to truly respect the __rcu annotation, deleting the rmaps would also have to be done "independently" with the correct rcu_assign_pointer() and synchronize_srcu() logic.
