Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Thu, 21 Sep 2023 14:32:41 +0000
In-Reply-To: <adebc422-2937-48d7-20c1-aef2dc1ac436@redhat.com>
Mime-Version: 1.0
References: <20230916003916.2545000-1-seanjc@google.com> <20230916003916.2545000-3-seanjc@google.com>
 <adebc422-2937-48d7-20c1-aef2dc1ac436@redhat.com>
Message-ID: <ZQxUCc3BEHA91FgY@google.com>
Subject: Re: [PATCH 2/3] KVM: x86/mmu: Take "shared" instead of "as_id" TDP
 MMU's yield-safe iterator
From:   Sean Christopherson <seanjc@google.com>
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Pattara Teerapong <pteerapong@google.com>,
        David Stevens <stevensd@google.com>,
        Yiwei Zhang <zzyiwei@google.com>,
        Paul Hsia <paulhsia@google.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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.