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

[Marcelo Tosatti <mtosatti@redhat.com>]

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