Re: KVM: Warn if mark_page_dirty() is called without an active vCPU

From: Sean Christopherson <seanjc@google.com>
To: David Woodhouse <dwmw2@infradead.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	butt3rflyh4ck <butterflyhuangxx@gmail.com>,
	kvm@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: KVM: Warn if mark_page_dirty() is called without an active vCPU
Date: Mon, 22 Nov 2021 17:01:59 +0000	[thread overview]
Message-ID: <YZvNB0ByFmdEkUVX@google.com> (raw)
In-Reply-To: <e8f40b8765f2feefb653d8a67e487818f66581aa.camel@infradead.org>

On Sat, Nov 20, 2021, David Woodhouse wrote:
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 6c5083f2eb50..72c6453bcef4 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3020,12 +3020,17 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>  			     struct kvm_memory_slot *memslot,
>  		 	     gfn_t gfn)
>  {
> +	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> +
> +	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))

Maybe use KVM_BUG_ON?  And two separate WARNs are probably overkill.

	if (KVM_BUG_ON(!vcpu || vcpu->kvm != kvm, kvm))

I'd also prefer to not retrieve the vCPU in the dirty_bitmap path, at least not
until it's necessary (for the proposed dirty quota throttling), though that's not
a strong preference.

> +		return;
> +
>  	if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
>  		unsigned long rel_gfn = gfn - memslot->base_gfn;
>  		u32 slot = (memslot->as_id << 16) | memslot->id;
>  
>  		if (kvm->dirty_ring_size)
> -			kvm_dirty_ring_push(kvm_dirty_ring_get(kvm),
> +			kvm_dirty_ring_push(&vcpu->dirty_ring,
>  					    slot, rel_gfn);

This can now squeeze on a single line.

			kvm_dirty_ring_push(&vcpu->dirty_ring, slot, rel_gfn);

>  		else
>  			set_bit_le(rel_gfn, memslot->dirty_bitmap);
> -- 
> 2.31.1
> 