Re: [PATCH v3 08/12] KVM: Propagate vcpu explicitly to mark_page_dirty_in_slot()

From: David Woodhouse <dwmw2@infradead.org>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm <kvm@vger.kernel.org>,
	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, Marc Zyngier <maz@kernel.org>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Huacai Chen <chenhuacai@kernel.org>,
	Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
	Michael Ellerman <mpe@ellerman.id.au>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Anup Patel <anup.patel@wdc.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
	kvm-riscv@lists.infradead.org, linux-s390@vger.kernel.org
Subject: Re: [PATCH v3 08/12] KVM: Propagate vcpu explicitly to mark_page_dirty_in_slot()
Date: Thu, 18 Nov 2021 14:22:04 +0000	[thread overview]
Message-ID: <20b5952e76c54a3a5dfe5a898e3b835404ac6fb1.camel@infradead.org> (raw)
In-Reply-To: <4c48546b-eb4a-dff7-cc38-5df54f73f5d4@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2317 bytes --]

On Thu, 2021-11-18 at 13:04 +0100, Paolo Bonzini wrote:
> On 11/17/21 22:09, David Woodhouse wrote:
> > >   {
> > > -	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> > > +	struct kvm_vcpu *running_vcpu = kvm_get_running_vcpu();
> > > 
> > > +	WARN_ON_ONCE(vcpu && vcpu != running_vcpu);
> > >   	WARN_ON_ONCE(vcpu->kvm != kvm);
> > Ah, that one needs to be changed to check running_vcpu instead. Or this
> > needs to go first:
> > 
> > I think I prefer making the vCPU a required argument. If anyone's going to
> > pull a vCPU pointer out of their posterior, let the caller do it.
> > 
> 
> I understand that feeling, but still using the running vCPU is by far 
> the common case, and it's not worth adding a new function parameter to 
> all call sites.
> 
> What about using a separate function, possibly __-prefixed, for the case 
> where you have a very specific vCPU?

We could certainly do a kvm_vcpu_mark_page_dirty_in_slot() and reduce
the collateral damage. I think this patch wouldn't need to touch
anything outside virt/kvm/ if we did that.

But bikeshedding aside, it occurs to me that I have a more substantial
concern about this approach — are we even *allowed* to touch the dirty
ring of another vCPU? It seems to be based on the assumption that it's
entirely a per-cpu structure on the kernel side. Wouldn't we need a
spinlock in kvm_dirty_ring_push() at the very least?

At this point I'm somewhat tempted to remove the mark_page_dirty() call
from gfn_to_pfn_cache_invalidate_start() and move it to the tail of 
kvm_gfn_to_pfn_cache_refresh() instead, where it unpins and memunmaps
the page. (We'd still tell the *kernel* by calling kvm_set_pfn_dirty()
immediately in the invalidation, just defer the KVM mark_page_dirty()
to be done in the context of an actual vCPU.)

If I do that, I can drop this 'propagate vcpu' patch completely.

I'm already happy enough that I'll fix the Xen shared_info page by
*never* bothering to dirty_log for it, and I can wash my hands of that
problem.... if I stop typing now. But...

That leaves the one in TDP MMU handle_changed_spte_dirty_log() which
AFAICT can trigger the same crash seen by butt3rflyh4ck — can't that
happen from a thread where kvm_get_running_vcpu() is NULL too? For that
one I'm not sure.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]