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 11:49:09 +0100	[thread overview]
Message-ID: <32b00203-e093-8ffc-a75b-27557b5ee6b1@redhat.com> (raw)
In-Reply-To: <2b400dbb16818da49fb599b9182788ff9896dcda.camel@infradead.org>

On 11/12/21 10:54, David Woodhouse wrote:
>>> 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.
> 
> ... that anything accessing these will *still* need to do so in atomic
> context. There's an atomic access which might fail, and then you fall
> back to a context in which you can sleep to refresh the mapping. and
> you *still* need to perform the actual access with the spinlock held to
> protect against concurrent invalidation.
> 
> So let's take a look... for posted_intr_desc_addr, that host physical
> address is actually written to the VMCS02, isn't it?
> 
> Thinking about the case where the target page is being invalidated
> while the vCPU is running... surely in that case the only 'correct'
> solution is that the vCPU needs to be kicked out of non-root mode
> before the invalidate_range() notifier completes?

Yes.

> That would have worked nicely if the MMU notifier could call
> scru_synchronize() on invalidation. Can it kick the vCPU and wait for
> it to exit though?

Yes, there's kvm_make_all_cpus_request (see 
kvm_arch_mmu_notifier_invalidate_range).  It can sleep, which is 
theoretically wrong---but in practice non-blockable invalidations only 
occur from the OOM reaper, so no CPU can be running.  If we care, we can 
return early from kvm_arch_mmu_notifier_invalidate_range for 
non-blockable invalidations.

> Don't get me wrong, a big part of me *loves* the idea that the hairiest
> part of my Xen event channel delivery is actually a bug fix that we
> need in the kernel anyway, and then the rest of it is simple and
> uncontentious.
> 
> (ISTR the virtual apic page is a bit different because it's only an
> *address* and it doesn't even have to be backed by real memory at the
> corresponding HPA? Otherwise it's basically the same issue?)

We do back it by real memory anyway, so it's the same.

Paolo