Re: [PATCH v1 03/13] KVM: x86/mmu: Automatically update iter->old_spte if cmpxchg fails

[Sean Christopherson <seanjc@google.com>]

On Mon, Dec 13, 2021, David Matlack wrote:
> Consolidate a bunch of code that was manually re-reading the spte if the
> cmpxchg fails. There is no extra cost of doing this because we already
> have the spte value as a result of the cmpxchg (and in fact this
> eliminates re-reading the spte), and none of the call sites depend on
> iter->old_spte retaining the stale spte value.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 50 ++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index b69e47e68307..656ebf5b20dc 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -492,16 +492,22 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
>   * and handle the associated bookkeeping.  Do not mark the page dirty
>   * in KVM's dirty bitmaps.
>   *
> + * If setting the SPTE fails because it has changed, iter->old_spte will be
> + * updated with the updated value of the spte.

First updated=>refreshed, second updated=>current?  More below.

> + *
>   * @kvm: kvm instance
>   * @iter: a tdp_iter instance currently on the SPTE that should be set
>   * @new_spte: The value the SPTE should be set to
>   * Returns: true if the SPTE was set, false if it was not. If false is returned,
> - *	    this function will have no side-effects.
> + *          this function will have no side-effects other than updating

s/updating/setting

> + *          iter->old_spte to the latest value of spte.

Strictly speaking, "latest" may not be true if yet another thread modifies the
SPTE.  Maybe this?

		iter->old_spte to the last known value of the SPTE.

>   */
>  static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
>  					   struct tdp_iter *iter,
>  					   u64 new_spte)
>  {
> +	u64 old_spte;
> +
>  	lockdep_assert_held_read(&kvm->mmu_lock);
>  
>  	/*
> @@ -515,9 +521,15 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
>  	 * Note, fast_pf_fix_direct_spte() can also modify TDP MMU SPTEs and
>  	 * does not hold the mmu_lock.
>  	 */
> -	if (cmpxchg64(rcu_dereference(iter->sptep), iter->old_spte,
> -		      new_spte) != iter->old_spte)
> +	old_spte = cmpxchg64(rcu_dereference(iter->sptep), iter->old_spte, new_spte);

To make this a bit easier to read, and to stay under 80 chars, how about
opportunistically doing this as well?

diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 656ebf5b20dc..64f1369f8638 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -506,6 +506,7 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
                                           struct tdp_iter *iter,
                                           u64 new_spte)
 {
+       u64 *sptep = rcu_dereference(iter->sptep);
        u64 old_spte;
 
        lockdep_assert_held_read(&kvm->mmu_lock);
@@ -521,7 +522,7 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
         * Note, fast_pf_fix_direct_spte() can also modify TDP MMU SPTEs and
         * does not hold the mmu_lock.
         */
-       old_spte = cmpxchg64(rcu_dereference(iter->sptep), iter->old_spte, new_spte);
+       old_spte = cmpxchg64(sptep, iter->old_spte, new_spte);
        if (old_spte != iter->old_spte) {
                /*
                 * The cmpxchg failed because the spte was updated by another

> +	if (old_spte != iter->old_spte) {
> +		/*
> +		 * The cmpxchg failed because the spte was updated by another
> +		 * thread so record the updated spte in old_spte.
> +		 */

Hmm, this is a bit awkward.  I think it's the double use of "updated" and the
somewhat ambiguous reference to "old_spte".  I'd also avoid "thread", as this
requires interference from not only a different task, but a different logical CPU
since iter->old_spte is refreshed if mmu_lock is dropped and reacquired.  And
"record" is an odd choice of word since it sounds like storing the current value
is only for logging/debugging.

Something like this?

		/*
		 * The entry was modified by a different logical CPU, refresh
		 * iter->old_spte with the current value so the caller operates
		 * on fresh data, e.g. if it retries tdp_mmu_set_spte_atomic().
		 */

Nits aside,

Reviewed-by: Sean Christopherson <seanjc@google.com>

> +		iter->old_spte = old_spte;
>  		return false;
> +	}
>  
>  	__handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte,
>  			      new_spte, iter->level, true);