Re: [RFC PATCH 0/11] Rework gfn_to_pfn_cache

From: Paolo Bonzini <pbonzini@redhat.com>
To: David Woodhouse <dwmw2@infradead.org>, kvm <kvm@vger.kernel.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Joao Martins <joao.m.martins@oracle.com>,
	"jmattson@google.com" <jmattson@google.com>,
	"wanpengli@tencent.com" <wanpengli@tencent.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"vkuznets@redhat.com" <vkuznets@redhat.com>,
	"mtosatti@redhat.com" <mtosatti@redhat.com>,
	"joro@8bytes.org" <joro@8bytes.org>,
	karahmed@amazon.com
Subject: Re: [RFC PATCH 0/11] Rework gfn_to_pfn_cache
Date: Mon, 15 Nov 2021 19:50:21 +0100	[thread overview]
Message-ID: <3a2a9a8c-db98-b770-78e2-79f5880ce4ed@redhat.com> (raw)
In-Reply-To: <95fae9cf56b1a7f0a5f2b9a1934e29e924908ff2.camel@infradead.org>

On 11/15/21 17:47, David Woodhouse wrote:
> So... a user of this must check the validity after setting its mode to
> IN_GUEST_MODE, and the invalidation must make a request and wake any
> vCPU(s) which might be using it.

Yes, though the check is implicit in the existing call to 
kvm_vcpu_exit_request(vcpu).

> I moved the invalidation to the invalidate_range MMU notifier, as
> discussed. But that's where the plan falls down a little bit because
> IIUC, that one can't sleep at all.

Which is a problem in the existing code, too.  It hasn't broken yet 
because invalidate_range() is _usually_ called with no spinlocks taken 
(the only caller that does call with a spinlock taken seems to be 
hugetlb_cow).

Once the dust settles, we need to add non_block_start/end around calls 
to ops->invalidate_range.

> I need to move it *back*  to
> invalidate_range_start() where I had it before, if I want to let it
> wait for vCPUs to exit. Which means... that the cache 'refresh' call
> must wait until the mmu_notifier_count reaches zero? Am I allowed to do > that, and make the "There can be only one waiter" comment in
> kvm_mmu_notifier_invalidate_range_end() no longer true?

You can also update the cache while taking the mmu_lock for read, and 
retry if mmu_notifier_retry_hva tells you to do so.  Looking at the 
scenario from commit e649b3f0188 you would have:

       (Invalidator) kvm_mmu_notifier_invalidate_range_start()
       (Invalidator) write_lock(mmu_lock)
       (Invalidator) increment mmu_notifier_count
       (Invalidator) write_unlock(mmu_lock)
       (Invalidator) request KVM_REQ_APIC_PAGE_RELOAD
       (KVM VCPU) vcpu_enter_guest()
       (KVM VCPU) kvm_vcpu_reload_apic_access_page()
    +  (KVM VCPU) read_lock(mmu_lock)
    +  (KVM VCPU) mmu_notifier_retry_hva()
    +  (KVM VCPU) read_unlock(mmu_lock)
    +  (KVM VCPU) retry! (mmu_notifier_count>1)
       (Invalidator) actually unmap page
    +  (Invalidator) kvm_mmu_notifier_invalidate_range_end()
    +  (Invalidator) write_lock(mmu_lock)
    +  (Invalidator) decrement mmu_notifier_count
    +  (Invalidator) write_unlock(mmu_lock)
    +  (KVM VCPU) vcpu_enter_guest()
    +  (KVM VCPU) kvm_vcpu_reload_apic_access_page()
    +  (KVM VCPU) mmu_notifier_retry_hva()

Changing mn_memslots_update_rcuwait to a waitq (and renaming it to 
mn_invalidate_waitq) is of course also a possibility.

Also, for the small requests: since you are at it, can you add the code 
in a new file under virt/kvm/?

Paolo

> I was also pondering whether to introduce a new arch-independent
> KVM_REQ_GPC_INVALIDATE, or let it be arch-dependent and make it a field
> of the cache, so that users can raise whatever requests they like?