Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification

[Amos Kong <akong@redhat.com>]

----- 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