qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
@ 2012-09-20  8:25 Amit Shah
  2012-09-20 12:46 ` Marcelo Tosatti
  2012-09-20 12:47 ` Marcelo Tosatti
  0 siblings, 2 replies; 8+ messages in thread
From: Amit Shah @ 2012-09-20  8:25 UTC (permalink / raw)
  To: qemu list, kvm list
  Cc: Raghavendra K T, Marcelo Tosatti, Eric B Munson, Amit Shah,
	Paolo Bonzini, Laszlo Ersek, Andreas Färber

Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest stop
notification, but it did it in a way that the stop notification would
never reach the kernel.  The kvm_vm_state_changed() function gets a
value of 0 for the 'running' parameter when the VM is stopped, making
all the code added previously dead code.

This patch reworks the code so that it's called when 'running' is 0,
which indicates the VM was stopped.

CC: Eric B Munson <emunson@mgebm.net>
CC: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
CC: Andreas Färber <afaerber@suse.de>
CC: Marcelo Tosatti <mtosatti@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/kvm/clock.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
index 824b978..f3427eb 100644
--- a/hw/kvm/clock.c
+++ b/hw/kvm/clock.c
@@ -71,18 +71,19 @@ static void kvmclock_vm_state_change(void *opaque, int running,
 
     if (running) {
         s->clock_valid = false;
+        return;
+    }
 
-        if (!cap_clock_ctrl) {
-            return;
-        }
-        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
-            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
-            if (ret) {
-                if (ret != -EINVAL) {
-                    fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
-                }
-                return;
+    if (!cap_clock_ctrl) {
+        return;
+    }
+    for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
+        ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
+        if (ret) {
+            if (ret != -EINVAL) {
+                fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
             }
+            return;
         }
     }
 }
-- 
1.7.7.6

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
  2012-09-20  8:25 [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification Amit Shah
@ 2012-09-20 12:46 ` Marcelo Tosatti
  2012-09-30 23:05   ` Marcelo Tosatti
  2012-09-20 12:47 ` Marcelo Tosatti
  1 sibling, 1 reply; 8+ messages in thread
From: Marcelo Tosatti @ 2012-09-20 12:46 UTC (permalink / raw)
  To: Amit Shah
  Cc: kvm list, Raghavendra K T, qemu list, Eric B Munson,
	Paolo Bonzini, Laszlo Ersek, Andreas Färber

On Thu, Sep 20, 2012 at 01:55:20PM +0530, Amit Shah wrote:
> Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest stop
> notification, but it did it in a way that the stop notification would
> never reach the kernel.  The kvm_vm_state_changed() function gets a
> value of 0 for the 'running' parameter when the VM is stopped, making
> all the code added previously dead code.
> 
> This patch reworks the code so that it's called when 'running' is 0,
> which indicates the VM was stopped.
> 
> CC: Eric B Munson <emunson@mgebm.net>
> CC: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> CC: Andreas Färber <afaerber@suse.de>
> CC: Marcelo Tosatti <mtosatti@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/kvm/clock.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
> index 824b978..f3427eb 100644
> --- a/hw/kvm/clock.c
> +++ b/hw/kvm/clock.c
> @@ -71,18 +71,19 @@ static void kvmclock_vm_state_change(void *opaque, int running,
>  
>      if (running) {
>          s->clock_valid = false;
> +        return;
> +    }
>  
> -        if (!cap_clock_ctrl) {
> -            return;
> -        }
> -        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> -            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> -            if (ret) {
> -                if (ret != -EINVAL) {
> -                    fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
> -                }
> -                return;
> +    if (!cap_clock_ctrl) {
> +        return;
> +    }
> +    for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> +        ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> +        if (ret) {
> +            if (ret != -EINVAL) {
> +                fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
>              }
> +            return;
>          }
>      }
>  }
> -- 
> 1.7.7.6

ACK

Avi, please merge through uq/master.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
  2012-09-20  8:25 [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification Amit Shah
  2012-09-20 12:46 ` Marcelo Tosatti
@ 2012-09-20 12:47 ` Marcelo Tosatti
  1 sibling, 0 replies; 8+ messages in thread
From: Marcelo Tosatti @ 2012-09-20 12:47 UTC (permalink / raw)
  To: Amit Shah
  Cc: kvm list, Raghavendra K T, qemu list, Eric B Munson,
	Paolo Bonzini, Laszlo Ersek, Andreas Färber


Amit, should please use "uq/master" in the subject to help
the person who is merging patches.

On Thu, Sep 20, 2012 at 01:55:20PM +0530, Amit Shah wrote:
> Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest stop
> notification, but it did it in a way that the stop notification would
> never reach the kernel.  The kvm_vm_state_changed() function gets a
> value of 0 for the 'running' parameter when the VM is stopped, making
> all the code added previously dead code.
> 
> This patch reworks the code so that it's called when 'running' is 0,
> which indicates the VM was stopped.
> 
> CC: Eric B Munson <emunson@mgebm.net>
> CC: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> CC: Andreas Färber <afaerber@suse.de>
> CC: Marcelo Tosatti <mtosatti@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/kvm/clock.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
> index 824b978..f3427eb 100644
> --- a/hw/kvm/clock.c
> +++ b/hw/kvm/clock.c
> @@ -71,18 +71,19 @@ static void kvmclock_vm_state_change(void *opaque, int running,
>  
>      if (running) {
>          s->clock_valid = false;
> +        return;
> +    }
>  
> -        if (!cap_clock_ctrl) {
> -            return;
> -        }
> -        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> -            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> -            if (ret) {
> -                if (ret != -EINVAL) {
> -                    fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
> -                }
> -                return;
> +    if (!cap_clock_ctrl) {
> +        return;
> +    }
> +    for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> +        ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> +        if (ret) {
> +            if (ret != -EINVAL) {
> +                fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
>              }
> +            return;
>          }
>      }
>  }
> -- 
> 1.7.7.6

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
  2012-09-20 12:46 ` Marcelo Tosatti
@ 2012-09-30 23:05   ` Marcelo Tosatti
  2012-10-01  1:50     ` Amos Kong
  2012-10-08 10:40     ` Amit Shah
  0 siblings, 2 replies; 8+ messages in thread
From: Marcelo Tosatti @ 2012-09-30 23:05 UTC (permalink / raw)
  To: Amit Shah
  Cc: kvm list, Raghavendra K T, qemu list, Eric B Munson,
	Paolo Bonzini, Laszlo Ersek, Andreas Färber

On Thu, Sep 20, 2012 at 09:46:41AM -0300, Marcelo Tosatti wrote:
> On Thu, Sep 20, 2012 at 01:55:20PM +0530, Amit Shah wrote:
> > Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest stop
> > notification, but it did it in a way that the stop notification would
> > never reach the kernel.  The kvm_vm_state_changed() function gets a
> > value of 0 for the 'running' parameter when the VM is stopped, making
> > all the code added previously dead code.
> > 
> > This patch reworks the code so that it's called when 'running' is 0,
> > which indicates the VM was stopped.
> > 
> > CC: Eric B Munson <emunson@mgebm.net>
> > CC: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> > CC: Andreas Färber <afaerber@suse.de>
> > CC: Marcelo Tosatti <mtosatti@redhat.com>
> > CC: Paolo Bonzini <pbonzini@redhat.com>
> > CC: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/kvm/clock.c |   21 +++++++++++----------
> >  1 files changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
> > index 824b978..f3427eb 100644
> > --- a/hw/kvm/clock.c
> > +++ b/hw/kvm/clock.c
> > @@ -71,18 +71,19 @@ static void kvmclock_vm_state_change(void *opaque, int running,
> >  
> >      if (running) {
> >          s->clock_valid = false;
> > +        return;
> > +    }
> >  
> > -        if (!cap_clock_ctrl) {
> > -            return;
> > -        }
> > -        for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> > -            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > -            if (ret) {
> > -                if (ret != -EINVAL) {
> > -                    fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
> > -                }
> > -                return;
> > +    if (!cap_clock_ctrl) {
> > +        return;
> > +    }
> > +    for (penv = first_cpu; penv != NULL; penv = penv->next_cpu) {
> > +        ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > +        if (ret) {
> > +            if (ret != -EINVAL) {
> > +                fprintf(stderr, "%s: %s\n", __func__, strerror(-ret));
> >              }
> > +            return;
> >          }
> >      }
> >  }
> > -- 
> > 1.7.7.6
> 
> ACK
> 
> Avi, please merge through uq/master.

NACK, guest should be notified when the VM is starting, not
when stopping.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
  2012-09-30 23:05   ` Marcelo Tosatti
@ 2012-10-01  1:50     ` Amos Kong
  2012-10-03 13:28       ` Marcelo Tosatti
  2012-10-08 10:43       ` Amit Shah
  2012-10-08 10:40     ` Amit Shah
  1 sibling, 2 replies; 8+ messages in thread
From: Amos Kong @ 2012-10-01  1:50 UTC (permalink / raw)
  To: Marcelo Tosatti
  Cc: kvm list, Raghavendra K T, qemu list, Eric B Munson, Amit Shah,
	Paolo Bonzini, Laszlo Ersek, Andreas Färber

----- Original Message -----
> On Thu, Sep 20, 2012 at 09:46:41AM -0300, Marcelo Tosatti wrote:
> > On Thu, Sep 20, 2012 at 01:55:20PM +0530, Amit Shah wrote:
> > > Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest
> > > stop

In commitlog of f349c12c0434e29c79ecde89029320c4002f7253: 

## This patch uses the qemu Notifier system to tell the guest it _is about to be_ stopped


> > > notification, but it did it in a way that the stop notification
> > > would
> > > never reach the kernel.  The kvm_vm_state_changed() function gets
> > > a
> > > value of 0 for the 'running' parameter when the VM is stopped,
> > > making
> > > all the code added previously dead code.
> > > 
> > > This patch reworks the code so that it's called when 'running' is
> > > 0,
> > > which indicates the VM was stopped.

Amit, did you touch any real issue? guest gets call trace with current code?
which kind of context?

Someone told me he got call trace when shutdown guest by 'init 0', I didn't
verify this issue.

> > > CC: Eric B Munson <emunson@mgebm.net>
> > > CC: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> > > CC: Andreas Färber <afaerber@suse.de>
> > > CC: Marcelo Tosatti <mtosatti@redhat.com>
> > > CC: Paolo Bonzini <pbonzini@redhat.com>
> > > CC: Laszlo Ersek <lersek@redhat.com>
> > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > ---
> > >  hw/kvm/clock.c |   21 +++++++++++----------
> > >  1 files changed, 11 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
> > > index 824b978..f3427eb 100644
> > > --- a/hw/kvm/clock.c
> > > +++ b/hw/kvm/clock.c
> > > @@ -71,18 +71,19 @@ static void kvmclock_vm_state_change(void
> > > *opaque, int running,


I found this function is only called when resume vm
(here running is 1, it means vm is already resumed?
we don't call that ioctl _before_ resume).

kvmclock_vm_state_change() is not called when I stop vm
through qemu monitor command.


> > >  
> > >      if (running) {
> > >          s->clock_valid = false;
> > > +        return;
> > > +    }
> > >  
> > > -        if (!cap_clock_ctrl) {
> > > -            return;
> > > -        }
> > > -        for (penv = first_cpu; penv != NULL; penv =
> > > penv->next_cpu) {
> > > -            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > > -            if (ret) {
> > > -                if (ret != -EINVAL) {
> > > -                    fprintf(stderr, "%s: %s\n", __func__,
> > > strerror(-ret));
> > > -                }
> > > -                return;
> > > +    if (!cap_clock_ctrl) {
> > > +        return;
> > > +    }
> > > +    for (penv = first_cpu; penv != NULL; penv = penv->next_cpu)
> > > {
> > > +        ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > > +        if (ret) {
> > > +            if (ret != -EINVAL) {
> > > +                fprintf(stderr, "%s: %s\n", __func__,
> > > strerror(-ret));
> > >              }
> > > +            return;
> > >          }
> > >      }
> > >  }
> > > --
> > > 1.7.7.6
> > 
> > ACK
> > 
> > Avi, please merge through uq/master.
> 
> NACK, guest should be notified when the VM is starting, not
> when stopping.

# from api.txt
ioctl (KVM_CAP_KVMCLOCK_CTRL) can be called any time _after_ pausing
the vcpu, but _before_ it is resumed.


Thanks, Amos

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
  2012-10-01  1:50     ` Amos Kong
@ 2012-10-03 13:28       ` Marcelo Tosatti
  2012-10-08 10:43       ` Amit Shah
  1 sibling, 0 replies; 8+ messages in thread
From: Marcelo Tosatti @ 2012-10-03 13:28 UTC (permalink / raw)
  To: Amos Kong
  Cc: kvm list, Raghavendra K T, qemu list, Eric B Munson, Amit Shah,
	Paolo Bonzini, Laszlo Ersek, Andreas Färber

On Sun, Sep 30, 2012 at 09:50:07PM -0400, Amos Kong wrote:
> ----- Original Message -----
> > On Thu, Sep 20, 2012 at 09:46:41AM -0300, Marcelo Tosatti wrote:
> > > On Thu, Sep 20, 2012 at 01:55:20PM +0530, Amit Shah wrote:
> > > > Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest
> > > > stop
> 
> In commitlog of f349c12c0434e29c79ecde89029320c4002f7253: 
> 
> ## This patch uses the qemu Notifier system to tell the guest it _is about to be_ stopped
> 
> 
> > > > notification, but it did it in a way that the stop notification
> > > > would
> > > > never reach the kernel.  The kvm_vm_state_changed() function gets
> > > > a
> > > > value of 0 for the 'running' parameter when the VM is stopped,
> > > > making
> > > > all the code added previously dead code.
> > > > 
> > > > This patch reworks the code so that it's called when 'running' is
> > > > 0,
> > > > which indicates the VM was stopped.
> 
> Amit, did you touch any real issue? guest gets call trace with current code?
> which kind of context?
> 
> Someone told me he got call trace when shutdown guest by 'init 0', I didn't
> verify this issue.
> 
> > > > CC: Eric B Munson <emunson@mgebm.net>
> > > > CC: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
> > > > CC: Andreas Färber <afaerber@suse.de>
> > > > CC: Marcelo Tosatti <mtosatti@redhat.com>
> > > > CC: Paolo Bonzini <pbonzini@redhat.com>
> > > > CC: Laszlo Ersek <lersek@redhat.com>
> > > > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > > > ---
> > > >  hw/kvm/clock.c |   21 +++++++++++----------
> > > >  1 files changed, 11 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/hw/kvm/clock.c b/hw/kvm/clock.c
> > > > index 824b978..f3427eb 100644
> > > > --- a/hw/kvm/clock.c
> > > > +++ b/hw/kvm/clock.c
> > > > @@ -71,18 +71,19 @@ static void kvmclock_vm_state_change(void
> > > > *opaque, int running,
> 
> 
> I found this function is only called when resume vm
> (here running is 1, it means vm is already resumed?
> we don't call that ioctl _before_ resume).
> 
> kvmclock_vm_state_change() is not called when I stop vm
> through qemu monitor command.

void vm_start(void)
{
    if (!runstate_is_running()) {
        cpu_enable_ticks();
        runstate_set(RUN_STATE_RUNNING);
        vm_state_notify(1, RUN_STATE_RUNNING);
        resume_all_vcpus();
        monitor_protocol_event(QEVENT_RESUME, NULL);
    }
}

'running' is a bad name that causes confusion because it refers to the
present moment (which is not precise). IMO, better name would be 'new_state'.

> > > >      if (running) {
> > > >          s->clock_valid = false;
> > > > +        return;
> > > > +    }
> > > >  
> > > > -        if (!cap_clock_ctrl) {
> > > > -            return;
> > > > -        }
> > > > -        for (penv = first_cpu; penv != NULL; penv =
> > > > penv->next_cpu) {
> > > > -            ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > > > -            if (ret) {
> > > > -                if (ret != -EINVAL) {
> > > > -                    fprintf(stderr, "%s: %s\n", __func__,
> > > > strerror(-ret));
> > > > -                }
> > > > -                return;
> > > > +    if (!cap_clock_ctrl) {
> > > > +        return;
> > > > +    }
> > > > +    for (penv = first_cpu; penv != NULL; penv = penv->next_cpu)
> > > > {
> > > > +        ret = kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0);
> > > > +        if (ret) {
> > > > +            if (ret != -EINVAL) {
> > > > +                fprintf(stderr, "%s: %s\n", __func__,
> > > > strerror(-ret));
> > > >              }
> > > > +            return;
> > > >          }
> > > >      }
> > > >  }
> > > > --
> > > > 1.7.7.6
> > > 
> > > ACK
> > > 
> > > Avi, please merge through uq/master.
> > 
> > NACK, guest should be notified when the VM is starting, not
> > when stopping.
> 
> # from api.txt
> ioctl (KVM_CAP_KVMCLOCK_CTRL) can be called any time _after_ pausing
> the vcpu, but _before_ it is resumed.

This is before its actually resumed. From the QEMU code pov, "actually
resumed" would be the point where it calls ioctl(vcpu_fd, KVM_RUN).

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
  2012-09-30 23:05   ` Marcelo Tosatti
  2012-10-01  1:50     ` Amos Kong
@ 2012-10-08 10:40     ` Amit Shah
  1 sibling, 0 replies; 8+ messages in thread
From: Amit Shah @ 2012-10-08 10:40 UTC (permalink / raw)
  To: Marcelo Tosatti
  Cc: kvm list, Raghavendra K T, qemu list, Eric B Munson,
	Paolo Bonzini, Laszlo Ersek, Andreas Färber

On (Sun) 30 Sep 2012 [20:05:16], Marcelo Tosatti wrote:
> On Thu, Sep 20, 2012 at 09:46:41AM -0300, Marcelo Tosatti wrote:
> > On Thu, Sep 20, 2012 at 01:55:20PM +0530, Amit Shah wrote:
> > > Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest stop
> > > notification, but it did it in a way that the stop notification would
> > > never reach the kernel.  The kvm_vm_state_changed() function gets a
> > > value of 0 for the 'running' parameter when the VM is stopped, making
> > > all the code added previously dead code.
> > > 
> > > This patch reworks the code so that it's called when 'running' is 0,
> > > which indicates the VM was stopped.

...

> NACK, guest should be notified when the VM is starting, not
> when stopping.

Ah, right.

		Amit

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification
  2012-10-01  1:50     ` Amos Kong
  2012-10-03 13:28       ` Marcelo Tosatti
@ 2012-10-08 10:43       ` Amit Shah
  1 sibling, 0 replies; 8+ messages in thread
From: Amit Shah @ 2012-10-08 10:43 UTC (permalink / raw)
  To: Amos Kong
  Cc: Marcelo Tosatti, kvm list, Raghavendra K T, qemu list,
	Eric B Munson, Paolo Bonzini, Laszlo Ersek, Andreas Färber

On (Sun) 30 Sep 2012 [21:50:07], Amos Kong wrote:
> ----- Original Message -----
> > On Thu, Sep 20, 2012 at 09:46:41AM -0300, Marcelo Tosatti wrote:
> > > On Thu, Sep 20, 2012 at 01:55:20PM +0530, Amit Shah wrote:
> > > > Commit f349c12c0434e29c79ecde89029320c4002f7253 added the guest
> > > > stop
> 
> In commitlog of f349c12c0434e29c79ecde89029320c4002f7253: 
> 
> ## This patch uses the qemu Notifier system to tell the guest it _is about to be_ stopped
> 
> 
> > > > notification, but it did it in a way that the stop notification
> > > > would
> > > > never reach the kernel.  The kvm_vm_state_changed() function gets
> > > > a
> > > > value of 0 for the 'running' parameter when the VM is stopped,
> > > > making
> > > > all the code added previously dead code.
> > > > 
> > > > This patch reworks the code so that it's called when 'running' is
> > > > 0,
> > > > which indicates the VM was stopped.
> 
> Amit, did you touch any real issue? guest gets call trace with current code?
> which kind of context?

I guess you're asking for a testcase to trigger softlockups?

Run a VM, make it do some work (like kernel compile).  Then, 'stop'
from the monitor for a few minutes.  Later, on 'cont', the softlockup
detector in the guest wakes up and shows a warning message mentioning
the cpus were stuck for <n> seconds.

For this particular patch, though, I didn't really test things; just
'found' this by examining code.  But as Marcelo points out, this patch
is wrong.

> Someone told me he got call trace when shutdown guest by 'init 0', I didn't
> verify this issue.

That sounds like a completely different thing, unless the trace is
invoked by the softlockup detector.

		Amit

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2012-10-08 10:43 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-20  8:25 [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification Amit Shah
2012-09-20 12:46 ` Marcelo Tosatti
2012-09-30 23:05   ` Marcelo Tosatti
2012-10-01  1:50     ` Amos Kong
2012-10-03 13:28       ` Marcelo Tosatti
2012-10-08 10:43       ` Amit Shah
2012-10-08 10:40     ` Amit Shah
2012-09-20 12:47 ` Marcelo Tosatti

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).