[PATCH v2] KVM: MMU: Inform users of mmio generation wraparound

[PATCH v2] KVM: MMU: Inform users of mmio generation wraparound

[Takuya Yoshikawa]

From: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>

Without this information, users will just see unexpected performance
problems and there is little chance we will get good reports from them:
note that mmio generation is increased even when we just start, or stop,
dirty logging for some memory slot, in which case users cannot expect
all shadow pages to be zapped.

printk_ratelimited() is used for this taking into account the problems
that we can see the information many times when we start multiple VMs
and guests can trigger this by reading ROM in a loop for example.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
 Interestingly, I saw this information printed twice every time.
 Looks like current_mmio_gen can become mmio_max_gen...

 arch/x86/kvm/mmu.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index c60c5da..54e3968 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
 	 * The max value is MMIO_MAX_GEN - 1 since it is not called
 	 * when mark memslot invalid.
 	 */
-	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
+	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
+		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
 		kvm_mmu_invalidate_zap_all_pages(kvm);
+	}
 }
 
 static int mmu_shrink(struct shrinker *shrink, struct shrink_control *sc)
-- 
1.7.9.5

Re: [PATCH v2] KVM: MMU: Inform users of mmio generation wraparound

[Paolo Bonzini]

Il 20/06/2013 18:34, Takuya Yoshikawa ha scritto:
> From: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> 
> Without this information, users will just see unexpected performance
> problems and there is little chance we will get good reports from them:
> note that mmio generation is increased even when we just start, or stop,
> dirty logging for some memory slot, in which case users cannot expect
> all shadow pages to be zapped.
> 
> printk_ratelimited() is used for this taking into account the problems
> that we can see the information many times when we start multiple VMs
> and guests can trigger this by reading ROM in a loop for example.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
>  Interestingly, I saw this information printed twice every time.
>  Looks like current_mmio_gen can become mmio_max_gen...
>  arch/x86/kvm/mmu.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index c60c5da..54e3968 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
>  	 * The max value is MMIO_MAX_GEN - 1 since it is not called
>  	 * when mark memslot invalid.
>  	 */
> -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
> +	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {

There is an off-by-one here (that I can fix in a separate patch).  The
right test is >= MMIO_MAX_GEN, since we have:

#define MMIO_MAX_GEN                    ((1 << MMIO_GEN_SHIFT) - 1)

Paolo

> +		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
>  		kvm_mmu_invalidate_zap_all_pages(kvm);
> +	}
>  }
>  
>  static int mmu_shrink(struct shrinker *shrink, struct shrink_control *sc)
> 

Re: [PATCH v2] KVM: MMU: Inform users of mmio generation wraparound

[Takuya Yoshikawa]

On Thu, 20 Jun 2013 23:29:22 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> > @@ -4385,8 +4385,10 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm)
> >  	 * The max value is MMIO_MAX_GEN - 1 since it is not called
> >  	 * when mark memslot invalid.
> >  	 */
> > -	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1)))
> > +	if (unlikely(kvm_current_mmio_generation(kvm) >= (MMIO_MAX_GEN - 1))) {
> 
> There is an off-by-one here (that I can fix in a separate patch).  The
> right test is >= MMIO_MAX_GEN, since we have:
> 
> #define MMIO_MAX_GEN                    ((1 << MMIO_GEN_SHIFT) - 1)
> 
> Paolo

As Xiao's comment says, this is intentional because when we delete a
memory slot, we increase slots->generation twice: first for setting
the invalid flag, then for actually commiting the slot.

To avoid this double zap_all(), we need to adjust the generation to
make it really wraparound when we hit:

        kvm_current_mmio_generation(kvm) == MMIO_MAX_GEN - 1

> 
> > +		printk_ratelimited(KERN_INFO "kvm: zapping shadow pages for mmio generation wraparound\n");
> >  		kvm_mmu_invalidate_zap_all_pages(kvm);
> > +	}
> >  }

BTW, I'm now thinking of another way to know this information.

If it's possible to check slots->generation, plus current_mmio_gen if needed,
on some events, administrators can know how many times such wraparounds
happened before.

They do not need to do tracing all the time for that, just hitting one event
later makes it possible to know if the guest was doing something problematic
before.

I'm now looking for the best trace point for that.  Probably somewhere in
arch_commit_memory().

	Takuya