[PATCH] do not enter vcpu again if it was stopped during IO

[PATCH] do not enter vcpu again if it was stopped during IO

[Gleb Natapov]

To prevent reentering vcpu after IO completion it is not enough
to set env->stopped since it is checked only in main loop but control
will not get there until next non-IO exit since kvm_run() will reenter
vcpu to complete IO instruction. Solve this by sending self-signal to
request exit after IO instruction completion.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/qemu-kvm.c b/qemu-kvm.c
index be1dac2..4f7cf6d 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -603,6 +603,10 @@ int kvm_run(CPUState *env)
     r = pre_kvm_run(kvm, env);
     if (r)
         return r;
+    if (env->exit_request) {
+        env->exit_request = 0;
+        pthread_kill(env->kvm_cpu_state.thread, SIG_IPI);
+    }
     r = ioctl(fd, KVM_RUN, 0);
 
     if (r == -1 && errno != EINTR && errno != EAGAIN) {
diff --git a/vl.c b/vl.c
index 9e9c176..dcfab13 100644
--- a/vl.c
+++ b/vl.c
@@ -1817,6 +1817,7 @@ void qemu_system_reset_request(void)
     }
     if (cpu_single_env) {
         cpu_single_env->stopped = 1;
+        cpu_exit(cpu_single_env);
     }
     qemu_notify_event();
 }
--
			Gleb.

Re: [PATCH] do not enter vcpu again if it was stopped during IO

[Marcelo Tosatti]

On Mon, Jun 21, 2010 at 12:01:52PM +0300, Gleb Natapov wrote:
> To prevent reentering vcpu after IO completion it is not enough
> to set env->stopped since it is checked only in main loop but control
> will not get there until next non-IO exit since kvm_run() will reenter
> vcpu to complete IO instruction. Solve this by sending self-signal to
> request exit after IO instruction completion.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index be1dac2..4f7cf6d 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -603,6 +603,10 @@ int kvm_run(CPUState *env)
>      r = pre_kvm_run(kvm, env);
>      if (r)
>          return r;
> +    if (env->exit_request) {
> +        env->exit_request = 0;
> +        pthread_kill(env->kvm_cpu_state.thread, SIG_IPI);
> +    }
>      r = ioctl(fd, KVM_RUN, 0);

Can't you check for env->stopped instead?

>  
>      if (r == -1 && errno != EINTR && errno != EAGAIN) {
> diff --git a/vl.c b/vl.c
> index 9e9c176..dcfab13 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1817,6 +1817,7 @@ void qemu_system_reset_request(void)
>      }
>      if (cpu_single_env) {
>          cpu_single_env->stopped = 1;
> +        cpu_exit(cpu_single_env);
>      }
>      qemu_notify_event();
>  }
> --
> 			Gleb.

Re: [PATCH] do not enter vcpu again if it was stopped during IO

[Gleb Natapov]

On Mon, Jun 21, 2010 at 05:43:04PM -0300, Marcelo Tosatti wrote:
> On Mon, Jun 21, 2010 at 12:01:52PM +0300, Gleb Natapov wrote:
> > To prevent reentering vcpu after IO completion it is not enough
> > to set env->stopped since it is checked only in main loop but control
> > will not get there until next non-IO exit since kvm_run() will reenter
> > vcpu to complete IO instruction. Solve this by sending self-signal to
> > request exit after IO instruction completion.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > index be1dac2..4f7cf6d 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -603,6 +603,10 @@ int kvm_run(CPUState *env)
> >      r = pre_kvm_run(kvm, env);
> >      if (r)
> >          return r;
> > +    if (env->exit_request) {
> > +        env->exit_request = 0;
> > +        pthread_kill(env->kvm_cpu_state.thread, SIG_IPI);
> > +    }
> >      r = ioctl(fd, KVM_RUN, 0);
> 
> Can't you check for env->stopped instead?
> 
Why it would be better? exit_request is used exactly for purpose to
notify cpu loop that is should exit. Sometimes it may be useful to
request cpu exit without stopping the cpu. I see gdbstub uses it, haven't
check if it applicable to kvm though.

> >  
> >      if (r == -1 && errno != EINTR && errno != EAGAIN) {
> > diff --git a/vl.c b/vl.c
> > index 9e9c176..dcfab13 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1817,6 +1817,7 @@ void qemu_system_reset_request(void)
> >      }
> >      if (cpu_single_env) {
> >          cpu_single_env->stopped = 1;
> > +        cpu_exit(cpu_single_env);
> >      }
> >      qemu_notify_event();
> >  }
> > --
> > 			Gleb.

--
			Gleb.

Re: [PATCH] do not enter vcpu again if it was stopped during IO

[Marcelo Tosatti]

On Tue, Jun 22, 2010 at 08:18:14AM +0300, Gleb Natapov wrote:
> On Mon, Jun 21, 2010 at 05:43:04PM -0300, Marcelo Tosatti wrote:
> > On Mon, Jun 21, 2010 at 12:01:52PM +0300, Gleb Natapov wrote:
> > > To prevent reentering vcpu after IO completion it is not enough
> > > to set env->stopped since it is checked only in main loop but control
> > > will not get there until next non-IO exit since kvm_run() will reenter
> > > vcpu to complete IO instruction. Solve this by sending self-signal to
> > > request exit after IO instruction completion.
> > > 
> > > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > > index be1dac2..4f7cf6d 100644
> > > --- a/qemu-kvm.c
> > > +++ b/qemu-kvm.c
> > > @@ -603,6 +603,10 @@ int kvm_run(CPUState *env)
> > >      r = pre_kvm_run(kvm, env);
> > >      if (r)
> > >          return r;
> > > +    if (env->exit_request) {
> > > +        env->exit_request = 0;
> > > +        pthread_kill(env->kvm_cpu_state.thread, SIG_IPI);
> > > +    }
> > >      r = ioctl(fd, KVM_RUN, 0);
> > 
> > Can't you check for env->stopped instead?
> > 
> Why it would be better? exit_request is used exactly for purpose to
> notify cpu loop that is should exit. Sometimes it may be useful to
> request cpu exit without stopping the cpu. I see gdbstub uses it, haven't
> check if it applicable to kvm though.

Makes sense.

Re: [PATCH] do not enter vcpu again if it was stopped during IO

[Avi Kivity]

On 06/21/2010 12:01 PM, Gleb Natapov wrote:
> To prevent reentering vcpu after IO completion it is not enough
> to set env->stopped since it is checked only in main loop but control
> will not get there until next non-IO exit since kvm_run() will reenter
> vcpu to complete IO instruction. Solve this by sending self-signal to
> request exit after IO instruction completion.
>
>    

Applied, thanks.

Does uq/master need such a patch?

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

Re: [PATCH] do not enter vcpu again if it was stopped during IO

[Gleb Natapov]

On Wed, Jun 23, 2010 at 11:22:15AM +0300, Avi Kivity wrote:
> On 06/21/2010 12:01 PM, Gleb Natapov wrote:
> >To prevent reentering vcpu after IO completion it is not enough
> >to set env->stopped since it is checked only in main loop but control
> >will not get there until next non-IO exit since kvm_run() will reenter
> >vcpu to complete IO instruction. Solve this by sending self-signal to
> >request exit after IO instruction completion.
> >
> 
> Applied, thanks.
> 
> Does uq/master need such a patch?
> 
The code is very different there. I looks to me broken too, but the fix
will be different.

--
			Gleb.

Re: [PATCH] do not enter vcpu again if it was stopped during IO

[Jan Kiszka]

Gleb Natapov wrote:
> To prevent reentering vcpu after IO completion it is not enough
> to set env->stopped since it is checked only in main loop but control
> will not get there until next non-IO exit since kvm_run() will reenter
> vcpu to complete IO instruction. Solve this by sending self-signal to
> request exit after IO instruction completion.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/qemu-kvm.c b/qemu-kvm.c
> index be1dac2..4f7cf6d 100644
> --- a/qemu-kvm.c
> +++ b/qemu-kvm.c
> @@ -603,6 +603,10 @@ int kvm_run(CPUState *env)
>      r = pre_kvm_run(kvm, env);
>      if (r)
>          return r;
> +    if (env->exit_request) {
> +        env->exit_request = 0;
> +        pthread_kill(env->kvm_cpu_state.thread, SIG_IPI);
> +    }
>      r = ioctl(fd, KVM_RUN, 0);
>  
>      if (r == -1 && errno != EINTR && errno != EAGAIN) {
> diff --git a/vl.c b/vl.c
> index 9e9c176..dcfab13 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1817,6 +1817,7 @@ void qemu_system_reset_request(void)
>      }
>      if (cpu_single_env) {
>          cpu_single_env->stopped = 1;
> +        cpu_exit(cpu_single_env);
>      }
>      qemu_notify_event();
>  }

What does this second hunk do, specifically in the context of I/O
processing? The changelog does not mention it explicitly (or I'm missing
the obvious).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] do not enter vcpu again if it was stopped during IO

[Gleb Natapov]

On Wed, Jun 23, 2010 at 12:16:46PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > To prevent reentering vcpu after IO completion it is not enough
> > to set env->stopped since it is checked only in main loop but control
> > will not get there until next non-IO exit since kvm_run() will reenter
> > vcpu to complete IO instruction. Solve this by sending self-signal to
> > request exit after IO instruction completion.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/qemu-kvm.c b/qemu-kvm.c
> > index be1dac2..4f7cf6d 100644
> > --- a/qemu-kvm.c
> > +++ b/qemu-kvm.c
> > @@ -603,6 +603,10 @@ int kvm_run(CPUState *env)
> >      r = pre_kvm_run(kvm, env);
> >      if (r)
> >          return r;
> > +    if (env->exit_request) {
> > +        env->exit_request = 0;
> > +        pthread_kill(env->kvm_cpu_state.thread, SIG_IPI);
> > +    }
> >      r = ioctl(fd, KVM_RUN, 0);
> >  
> >      if (r == -1 && errno != EINTR && errno != EAGAIN) {
> > diff --git a/vl.c b/vl.c
> > index 9e9c176..dcfab13 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1817,6 +1817,7 @@ void qemu_system_reset_request(void)
> >      }
> >      if (cpu_single_env) {
> >          cpu_single_env->stopped = 1;
> > +        cpu_exit(cpu_single_env);
> >      }
> >      qemu_notify_event();
> >  }
> 
> What does this second hunk do, specifically in the context of I/O
> processing? The changelog does not mention it explicitly (or I'm missing
> the obvious).
> 
It sets env->exit_request. If qemu_system_reset_request() inside io
handler (happens during S3) we should not return to vcpu till resume,
or terrible things will happen. You are probably right that it should
have been send as different patch.

--
			Gleb.