Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f444790d-3bc7-9870-576e-29f30354a63b@redhat.com>
Date:   Tue, 1 Mar 2022 21:12:39 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v3 20/28] KVM: x86/mmu: Allow yielding when zapping GFNs
 for defunct TDP MMU root
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        David Hildenbrand <david@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, David Matlack <dmatlack@google.com>,
        Ben Gardon <bgardon@google.com>,
        Mingwei Zhang <mizhang@google.com>
References: <20220226001546.360188-1-seanjc@google.com>
 <20220226001546.360188-21-seanjc@google.com>
 <28276890-c90c-e9a9-3cab-15264617ef5a@redhat.com>
 <Yh53V23gSJ6jphnS@google.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <Yh53V23gSJ6jphnS@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/1/22 20:43, Sean Christopherson wrote:
>> and after spending quite some time I wonder if all this should just be
>>
>>          if (refcount_dec_not_one(&root->tdp_mmu_root_count))
>>                  return;
>>
>> 	if (!xchg(&root->role.invalid, true) {
> 
> The refcount being '1' means there's another task currently using root, marking
> the root invalid will mean checks on the root's validity are non-deterministic
> for the other task.

Do you mean it's not possible to use refcount_dec_not_one, otherwise 
kvm_tdp_mmu_get_root is not guaranteed to reject the root?

>> 	 	tdp_mmu_zap_root(kvm, root, shared);
>>
>> 		/*
>> 		 * Do not assume the refcount is still 1: because
>> 		 * tdp_mmu_zap_root can yield, a different task
>> 		 * might have grabbed a reference to this root.
>> 		 *
>> 	        if (refcount_dec_not_one(&root->tdp_mmu_root_count))
> 
> This is wrong, _this_ task can't drop a reference taken by the other task.

This is essentially the "kvm_tdp_mmu_put_root(kvm, root, shared);" (or 
"goto beginning_of_function;") part of your patch.

>>          	        return;
>> 	}
>>
>> 	/*
>> 	 * The root is invalid, and its reference count has reached
>> 	 * zero.  It must have been zapped either in the "if" above or
>> 	 * by someone else, and we're definitely the last thread to see
>> 	 * it apart from RCU-protected page table walks.
>> 	 */
>> 	refcount_set(&root->tdp_mmu_root_count, 0);
> 
> Not sure what you intended here, KVM should never force a refcount to '0'.

It's turning a refcount_dec_not_one into a refcount_dec_and_test.  It 
seems legit to me, because the only refcount_inc_not_zero is in a 
write-side critical section.  If the refcount goes to zero on the 
read-side, the root is gone for good.

> xchg() is a very good idea.  The smp_mb_*() stuff was carried over from the previous
> version where this sequence set another flag in addition to role.invalid.
> 
> Is this less funky (untested)?
> 
> 	/*
> 	 * Invalidate the root to prevent it from being reused by a vCPU while
> 	 * the root is being zapped, i.e. to allow yielding while zapping the
> 	 * root (see below).
> 	 *
> 	 * Free the root if it's already invalid.  Invalid roots must be zapped
> 	 * before their last reference is put, i.e. there's no work to be done,
> 	 * and all roots must be invalidated before they're freed (this code).
> 	 * Re-zapping invalid roots would put KVM into an infinite loop.
> 	 *
> 	 * Note, xchg() provides an implicit barrier to ensure role.invalid is
> 	 * visible if a concurrent reader acquires a reference after the root's
> 	 * refcount is reset.
> 	 */
> 	if (xchg(root->role.invalid, true))
> 		spin_lock(&kvm->arch.tdp_mmu_pages_lock);
> 		list_del_rcu(&root->link);
> 		spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
> 
> 		call_rcu(&root->rcu_head, tdp_mmu_free_sp_rcu_callback);
> 		return;
> 	}

Based on my own version, I guess you mean (without comments due to 
family NMI):

         if (!refcount_dec_and_test(&root->tdp_mmu_root_count))
                 return;

	if (!xchg(&root->role.invalid, true) {
		refcount_set(&root->tdp_mmu_count, 1);
	 	tdp_mmu_zap_root(kvm, root, shared);
	        if (!refcount_dec_and_test(&root->tdp_mmu_root_count))
         	        return;
	}

         spin_lock(&kvm->arch.tdp_mmu_pages_lock);
         list_del_rcu(&root->link);
         spin_unlock(&kvm->arch.tdp_mmu_pages_lock);
         call_rcu(&root->rcu_head, tdp_mmu_free_sp_rcu_callback);

Paolo