[PATCH] KVM: Ensure the exit frequency to QEmu for coalesced MMIO

[PATCH] KVM: Ensure the exit frequency to QEmu for coalesced MMIO

[Sheng Yang]

The default action of calesced MMIO is, cache the writing in buffer, until:
1. The buffer is full.
2. Or the exit to QEmu due to other reasons.

But this would result in a very late writing in some condition.
1. The each time write to MMIO content is small.
2. The writing interval is big.
3. No need for input or accessing other devices frequently.

This issue was observed in a experimental embbed system. The test image simply
print "test" every 1 seconds. The output in QEmu meets expectation, but the
output in KVM is delayed for seconds.

To solve this issue, a timeout is added, to ensure userspace exit freqency is
high enough(mostly target for video now) to write the buffered MMIO data.
Current the maximum exit interval is 1/25s(so 25 times exit to userspace per
second at least, pretty low compared to normal environment), and reused
KVM_EXIT_IRQ_WINDOW_OPEN as exit reason, for it would doing nothing at all in
userspace handler.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/include/asm/kvm_host.h |    2 ++
 arch/x86/kvm/x86.c              |   31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index a1f0b5d..eb8bb20 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -365,6 +365,8 @@ struct kvm_vcpu_arch {
 	unsigned long singlestep_rip;
 	/* fields used by HYPER-V emulation */
 	u64 hv_vapic;
+
+	ktime_t latest_userspace_exit_time;
 };
 
 struct kvm_mem_alias {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 56a90a6..0b05f11 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4339,6 +4339,20 @@ out:
 	return r;
 }
 
+#ifdef CONFIG_KVM_MMIO
+
+#define KVM_USERSPACE_MMIO_MAX_INTERVAL (NSEC_PER_SEC / 25)
+static bool mmio_need_exit_to_userspace(struct kvm_vcpu *vcpu)
+{
+	ktime_t gap, now = ktime_get();
+
+	gap = ktime_sub(now, vcpu->arch.latest_userspace_exit_time);
+	if (ktime_to_ns(gap) > KVM_USERSPACE_MMIO_MAX_INTERVAL)
+		return true;
+
+	return false;
+}
+#endif
 
 static int __vcpu_run(struct kvm_vcpu *vcpu)
 {
@@ -4404,6 +4418,10 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			kvm_resched(vcpu);
 			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
 		}
+#ifdef CONFIG_KVM_MMIO
+		if (mmio_need_exit_to_userspace(vcpu))
+			r = 0;
+#endif
 	}
 
 	srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
@@ -4463,6 +4481,16 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 
 	r = __vcpu_run(vcpu);
 
+#ifdef CONFIG_KVM_MMIO
+	if (mmio_need_exit_to_userspace(vcpu)) {
+		/* Use KVM_EXIT_IRQ_WINDOW_OPEN because userspace would do
+		 * nothing to handle it */
+		kvm_run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;
+		r = 0;
+	}
+	vcpu->arch.latest_userspace_exit_time = ktime_get();
+#endif
+
 out:
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
@@ -5455,6 +5483,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 		goto fail_mmu_destroy;
 	}
 	vcpu->arch.mcg_cap = KVM_MAX_MCE_BANKS;
+#ifdef CONFIG_KVM_MMIO
+	vcpu->arch.latest_userspace_exit_time = ktime_get();
+#endif
 
 	return 0;
 
-- 
1.5.4.5

Re: [PATCH] KVM: Ensure the exit frequency to QEmu for coalesced MMIO

[Sheng Yang]

On Wednesday 20 January 2010 16:35:59 Sheng Yang wrote:
> The default action of calesced MMIO is, cache the writing in buffer, until:
> 1. The buffer is full.
> 2. Or the exit to QEmu due to other reasons.
> 
> But this would result in a very late writing in some condition.
> 1. The each time write to MMIO content is small.
> 2. The writing interval is big.
> 3. No need for input or accessing other devices frequently.

"No need for exiting to the QEmu in a period" should be more precise...

-- 
regards
Yang, Sheng

> 
> This issue was observed in a experimental embbed system. The test image
>  simply print "test" every 1 seconds. The output in QEmu meets expectation,
>  but the output in KVM is delayed for seconds.
> 
> To solve this issue, a timeout is added, to ensure userspace exit freqency
>  is high enough(mostly target for video now) to write the buffered MMIO
>  data. Current the maximum exit interval is 1/25s(so 25 times exit to
>  userspace per second at least, pretty low compared to normal environment),
>  and reused KVM_EXIT_IRQ_WINDOW_OPEN as exit reason, for it would doing
>  nothing at all in userspace handler.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/include/asm/kvm_host.h |    2 ++
>  arch/x86/kvm/x86.c              |   31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h
>  b/arch/x86/include/asm/kvm_host.h index a1f0b5d..eb8bb20 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -365,6 +365,8 @@ struct kvm_vcpu_arch {
>  	unsigned long singlestep_rip;
>  	/* fields used by HYPER-V emulation */
>  	u64 hv_vapic;
> +
> +	ktime_t latest_userspace_exit_time;
>  };
> 
>  struct kvm_mem_alias {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 56a90a6..0b05f11 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4339,6 +4339,20 @@ out:
>  	return r;
>  }
> 
> +#ifdef CONFIG_KVM_MMIO
> +
> +#define KVM_USERSPACE_MMIO_MAX_INTERVAL (NSEC_PER_SEC / 25)
> +static bool mmio_need_exit_to_userspace(struct kvm_vcpu *vcpu)
> +{
> +	ktime_t gap, now = ktime_get();
> +
> +	gap = ktime_sub(now, vcpu->arch.latest_userspace_exit_time);
> +	if (ktime_to_ns(gap) > KVM_USERSPACE_MMIO_MAX_INTERVAL)
> +		return true;
> +
> +	return false;
> +}
> +#endif
> 
>  static int __vcpu_run(struct kvm_vcpu *vcpu)
>  {
> @@ -4404,6 +4418,10 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
>  			kvm_resched(vcpu);
>  			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
>  		}
> +#ifdef CONFIG_KVM_MMIO
> +		if (mmio_need_exit_to_userspace(vcpu))
> +			r = 0;
> +#endif
>  	}
> 
>  	srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
> @@ -4463,6 +4481,16 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu,
>  struct kvm_run *kvm_run)
> 
>  	r = __vcpu_run(vcpu);
> 
> +#ifdef CONFIG_KVM_MMIO
> +	if (mmio_need_exit_to_userspace(vcpu)) {
> +		/* Use KVM_EXIT_IRQ_WINDOW_OPEN because userspace would do
> +		 * nothing to handle it */
> +		kvm_run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;
> +		r = 0;
> +	}
> +	vcpu->arch.latest_userspace_exit_time = ktime_get();
> +#endif
> +
>  out:
>  	if (vcpu->sigset_active)
>  		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
> @@ -5455,6 +5483,9 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>  		goto fail_mmu_destroy;
>  	}
>  	vcpu->arch.mcg_cap = KVM_MAX_MCE_BANKS;
> +#ifdef CONFIG_KVM_MMIO
> +	vcpu->arch.latest_userspace_exit_time = ktime_get();
> +#endif
> 
>  	return 0;
> 

Re: [PATCH] KVM: Ensure the exit frequency to QEmu for coalesced MMIO

[Avi Kivity]

On 01/20/2010 10:35 AM, Sheng Yang wrote:
> The default action of calesced MMIO is, cache the writing in buffer, until:
> 1. The buffer is full.
> 2. Or the exit to QEmu due to other reasons.
>
> But this would result in a very late writing in some condition.
> 1. The each time write to MMIO content is small.
> 2. The writing interval is big.
> 3. No need for input or accessing other devices frequently.
>
> This issue was observed in a experimental embbed system. The test image simply
> print "test" every 1 seconds. The output in QEmu meets expectation, but the
> output in KVM is delayed for seconds.
>
> To solve this issue, a timeout is added, to ensure userspace exit freqency is
> high enough(mostly target for video now) to write the buffered MMIO data.
> Current the maximum exit interval is 1/25s(so 25 times exit to userspace per
> second at least, pretty low compared to normal environment), and reused
> KVM_EXIT_IRQ_WINDOW_OPEN as exit reason, for it would doing nothing at all in
> userspace handler.
>    

This can be done from userspace, by scheduling a SIGALRM in the desired 
frequency.  This way, userspace has control over the update frequency 
(for example, if the SDL window is minimized or VNC is disconnected, we 
don't need to poll).

I think we can even do this from the I/O thread, without stopping a 
vcpu, since the colaesced mmio page is not tied to a vcpu but is a vm 
property.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: Ensure the exit frequency to QEmu for coalesced MMIO

[Sheng Yang]

On Wednesday 20 January 2010 17:11:51 Avi Kivity wrote:
> On 01/20/2010 10:35 AM, Sheng Yang wrote:
> > The default action of calesced MMIO is, cache the writing in buffer,
> > until: 1. The buffer is full.
> > 2. Or the exit to QEmu due to other reasons.
> >
> > But this would result in a very late writing in some condition.
> > 1. The each time write to MMIO content is small.
> > 2. The writing interval is big.
> > 3. No need for input or accessing other devices frequently.
> >
> > This issue was observed in a experimental embbed system. The test image
> > simply print "test" every 1 seconds. The output in QEmu meets
> > expectation, but the output in KVM is delayed for seconds.
> >
> > To solve this issue, a timeout is added, to ensure userspace exit
> > freqency is high enough(mostly target for video now) to write the
> > buffered MMIO data. Current the maximum exit interval is 1/25s(so 25
> > times exit to userspace per second at least, pretty low compared to
> > normal environment), and reused KVM_EXIT_IRQ_WINDOW_OPEN as exit reason,
> > for it would doing nothing at all in userspace handler.
> 
> This can be done from userspace, by scheduling a SIGALRM in the desired
> frequency.  This way, userspace has control over the update frequency
> (for example, if the SDL window is minimized or VNC is disconnected, we
> don't need to poll).

I also thought of using alarm from userspace. But the issue I thought is a SIGALRM should shoot 
down a vcpu unconditionally. I think in the real world, this situation is not that common, but 
the SIGALRM would definitely bring overhead in every situation. So I choose the current method - 
though still not that elegant.
> 
> I think we can even do this from the I/O thread, without stopping a
> vcpu, since the colaesced mmio page is not tied to a vcpu but is a vm
> property.

This one sounds better. But I've taken a look at the current userspace code:

>#if defined(KVM_CAP_COALESCED_MMIO)
>    if (kvm_state->coalesced_mmio) {
>        struct kvm_coalesced_mmio_ring *ring =
>            (void *) run + kvm_state->coalesced_mmio * PAGE_SIZE;
>        while (ring->first != ring->last) {
>            cpu_physical_memory_rw(ring->coalesced_mmio[ring->first].phys_addr,
>                           &ring->coalesced_mmio[ring->first].data[0],
>                           ring->coalesced_mmio[ring->first].len, 1);
>            smp_wmb();
>            ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
>        }
>    }
>#endif

No protection for ring->first and ring->last? Seems it can writing the same element pointed by 
ring->first twice, then skip one element at (ring->first + 1)...

-- 
regards
Yang, Sheng

Re: [PATCH] KVM: Ensure the exit frequency to QEmu for coalesced MMIO

[Avi Kivity]

On 01/20/2010 11:34 AM, Sheng Yang wrote:
>
>> I think we can even do this from the I/O thread, without stopping a
>> vcpu, since the colaesced mmio page is not tied to a vcpu but is a vm
>> property.
>>      
> This one sounds better. But I've taken a look at the current userspace code:
>
>    
>> #if defined(KVM_CAP_COALESCED_MMIO)
>>     if (kvm_state->coalesced_mmio) {
>>         struct kvm_coalesced_mmio_ring *ring =
>>             (void *) run + kvm_state->coalesced_mmio * PAGE_SIZE;
>>         while (ring->first != ring->last) {
>>             cpu_physical_memory_rw(ring->coalesced_mmio[ring->first].phys_addr,
>>                            &ring->coalesced_mmio[ring->first].data[0],
>>                            ring->coalesced_mmio[ring->first].len, 1);
>>             smp_wmb();
>>             ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
>>         }
>>     }
>> #endif
>>      
> No protection for ring->first and ring->last? Seems it can writing the same element pointed by
> ring->first twice, then skip one element at (ring->first + 1)...
>    

ring->first is owned by userspace, while ring->last is owned by the 
kernel, so no protection is necessary except for the memory barrier.  
Can you elaborate on how it would fail?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: Ensure the exit frequency to QEmu for coalesced MMIO

[Sheng Yang]

On Wednesday 20 January 2010 17:47:48 Avi Kivity wrote:
> On 01/20/2010 11:34 AM, Sheng Yang wrote:
> >> I think we can even do this from the I/O thread, without stopping a
> >> vcpu, since the colaesced mmio page is not tied to a vcpu but is a vm
> >> property.
> >
> > This one sounds better. But I've taken a look at the current userspace 
code:
> >> #if defined(KVM_CAP_COALESCED_MMIO)
> >>     if (kvm_state->coalesced_mmio) {
> >>         struct kvm_coalesced_mmio_ring *ring =
> >>             (void *) run + kvm_state->coalesced_mmio * PAGE_SIZE;
> >>         while (ring->first != ring->last) {
> >>            
> >> cpu_physical_memory_rw(ring->coalesced_mmio[ring->first].phys_addr,
> >> &ring->coalesced_mmio[ring->first].data[0],
> >> ring->coalesced_mmio[ring->first].len, 1); smp_wmb();
> >>             ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
> >>         }
> >>     }
> >> #endif
> >
> > No protection for ring->first and ring->last? Seems it can writing the
> > same element pointed by ring->first twice, then skip one element at
> > (ring->first + 1)...
> 
> ring->first is owned by userspace, while ring->last is owned by the
> kernel, so no protection is necessary except for the memory barrier.
> Can you elaborate on how it would fail?
> 

This piece of code can only be executed on one thread/vcpu at same time? I 
think different vcpus accessing/modifying ring->first at the same time would 
cause problem.

But for a separate iothread which handle all userspace accessing, it should be 
fine.

-- 
regards
Yang, Sheng

Re: [PATCH] KVM: Ensure the exit frequency to QEmu for coalesced MMIO

[Avi Kivity]

On 01/20/2010 11:57 AM, Sheng Yang wrote:
>
>>> No protection for ring->first and ring->last? Seems it can writing the
>>> same element pointed by ring->first twice, then skip one element at
>>> (ring->first + 1)...
>>>        
>> ring->first is owned by userspace, while ring->last is owned by the
>> kernel, so no protection is necessary except for the memory barrier.
>> Can you elaborate on how it would fail?
>>
>>      
> This piece of code can only be executed on one thread/vcpu at same time? I
> think different vcpus accessing/modifying ring->first at the same time would
> cause problem.
>    

Yes, it's protected by qemu_lock.

> But for a separate iothread which handle all userspace accessing, it should be
> fine.
>    

coalesced mmio processing can happen as part of vcpu processing (due to 
an mmio exit) or by the iothread (forced by the display refresh timer 
for example), but still serialized by qemu_lock.

-- 
error compiling committee.c: too many arguments to function