Re: [PATCH 2/3] KVM: x86/mmu: Take "shared" instead of "as_id" TDP MMU's yield-safe iterator

[Sean Christopherson <seanjc@google.com>]

On Thu, Sep 21, 2023, Paolo Bonzini wrote:
> On 9/16/23 02:39, Sean Christopherson wrote:
> > Replace the address space ID in for_each_tdp_mmu_root_yield_safe() with a
> > shared (vs. exclusive) param, and have the walker iterate over all address
> > spaces as all callers want to process all address spaces.  Drop the @as_id
> > param as well as the manual address space iteration in callers.
> > 
> > Add the @shared param even though the two current callers pass "false"
> > unconditionally, as the main reason for refactoring the walker is to
> > simplify using it to zap invalid TDP MMU roots, which is done with
> > mmu_lock held for read.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> 
> You konw what, I don't really like the "bool shared" arguments anymore.

Yeah, I don't like the "shared" arguments either.  Never did, but they are necessary
for some paths, and I don't see an obviously better solution. :-/

> For example, neither tdp_mmu_next_root nor kvm_tdp_mmu_put_root need to know
> if the lock is taken for read or write; protection is achieved via RCU and
> tdp_mmu_pages_lock.  It's more self-documenting to remove the argument and
> assert that the lock is taken.
> 
> Likewise, the argument is more or less unnecessary in the
> for_each_*_tdp_mmu_root_yield_safe() macros.  Many users check for the lock
> before calling it; and all of them either call small functions that do the
> check, or end up calling tdp_mmu_set_spte_atomic() and
> tdp_mmu_iter_set_spte(), so the per-iteration checks are also overkill.

Agreed.
 
> It may be useful to a few assertions to make up for the lost check before
> the first execution of the body of for_each_*_tdp_mmu_root_yield_safe(), but
> even this is more for documentation reasons than to catch actual bugs.

I think it's more than sufficient, arguably even better, to document which paths
*require* mmu_lock be held for read vs. write, and which paths work with either.

> I'll send a v2.

Can we do a cleanup of the @shared arguments on top?  I would like to keep the
diff reasonably small to minimize the v6.1 backport.