Re: [PATCH v3] KVM: x86: Fix recording of guest steal time / preempted status

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: [PATCH v3] KVM: x86: Fix recording of guest steal time / preempted status
Date: Fri, 12 Nov 2021 10:31:42 +0100	[thread overview]
Message-ID: <40d7d808-dce6-a541-18dc-b0c7f4d6586c@redhat.com> (raw)
In-Reply-To: <96cef64bf7927b6a0af2173b0521032f620551e4.camel@infradead.org>

On 11/12/21 09:28, David Woodhouse wrote:
> I do not recall that we'd actually reached a conclusion that we *will*
> make the gfn_to_pfn cache generally usable in that fashion. The latest
> I knew of that discussion was my message at
> https://lore.kernel.org/kvm/55a5d4e3fbd29dd55e276b97eeaefd0411b3290b.camel@infradead.org/
> in which I said I'd be a whole lot happier with that if we could do it
> with RCU instead of an rwlock — but I don't think we can because we'd
> need to call synchronize_srcu() in the MMU notifier callback that might
> not be permitted to sleep?

Why do you have a problem with the rwlock?  If it's per-cache, and it's 
mostly taken within vCPU context (with the exception of Xen), contention 
should be nonexistent.

> I'm also slightly less comfortable with having the MMU notifier work
> through an arbitrary *list* of gfn_to_pfn caches that it potentially
> needs to invalidate, but that is very much a minor concern compared
> with the first.
> 
> I started looking through the nested code which is the big user of this
> facility.

Yes, that's also where I got stuck in my first attempt a few months ago. 
  I agree that it can be changed to use gfn-to-hva caches, except for 
the vmcs12->posted_intr_desc_addr and vmcs12->virtual_apic_page_addr.

Paolo

> The important part of the gfn_to_pfn mapping as I've used it
> for Xen event channel delivery is the fast path in the common case,
> falling back to a slow path that needs to sleep, to revalidate the
> mapping. That fast vs. slow path (with a workqueue) already existed for
> irqfd delivery and I just needed to hook into it in the right places.
> 
> I didn't see anything in nested code that would benefit from that same
> setup, and AFAICT it should all be running with current->mm == kvm->mm
> so surely it ought to be able to just access things using the userspace
> HVA and sleep if necessary?
> 
> (There's an *entirely* gratuitous one in nested_cache_shadow_vmcs12()
> which does a map/memcpy/unmap that really ought to be kvm_read_guest().
> I'll send a patch for that shortly)
> 
> 