From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 1/1] kvmclock: fix guest stop notification Date: Wed, 3 Oct 2012 10:28:06 -0300 Message-ID: <20121003132806.GA6141@amt.cnet> References: <20120930230516.GA3923@amt.cnet> <1389829631.4053667.1349056207458.JavaMail.root@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: qemu list , kvm list , Eric B Munson , Raghavendra K T , Andreas =?iso-8859-1?Q?F=E4rber?= , Paolo Bonzini , Laszlo Ersek , Amit Shah To: Amos Kong Return-path: Received: from mx1.redhat.com ([209.132.183.28]:23362 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755215Ab2JCN27 (ORCPT ); Wed, 3 Oct 2012 09:28:59 -0400 Content-Disposition: inline In-Reply-To: <1389829631.4053667.1349056207458.JavaMail.root@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 >=20 > In commitlog of f349c12c0434e29c79ecde89029320c4002f7253:=20 >=20 > ## This patch uses the qemu Notifier system to tell the guest it _is = about to be_ stopped >=20 >=20 > > > > notification, but it did it in a way that the stop notification > > > > would > > > > never reach the kernel. The kvm_vm_state_changed() function ge= ts > > > > a > > > > value of 0 for the 'running' parameter when the VM is stopped, > > > > making > > > > all the code added previously dead code. > > > >=20 > > > > This patch reworks the code so that it's called when 'running' = is > > > > 0, > > > > which indicates the VM was stopped. >=20 > Amit, did you touch any real issue? guest gets call trace with curren= t code? > which kind of context? >=20 > Someone told me he got call trace when shutdown guest by 'init 0', I = didn't > verify this issue. >=20 > > > > CC: Eric B Munson > > > > CC: Raghavendra K T > > > > CC: Andreas F=E4rber > > > > CC: Marcelo Tosatti > > > > CC: Paolo Bonzini > > > > CC: Laszlo Ersek > > > > Signed-off-by: Amit Shah > > > > --- > > > > hw/kvm/clock.c | 21 +++++++++++---------- > > > > 1 files changed, 11 insertions(+), 10 deletions(-) > > > >=20 > > > > 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, >=20 >=20 > 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). >=20 > 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_s= tate'. > > > > if (running) { > > > > s->clock_valid =3D false; > > > > + return; > > > > + } > > > > =20 > > > > - if (!cap_clock_ctrl) { > > > > - return; > > > > - } > > > > - for (penv =3D first_cpu; penv !=3D NULL; penv =3D > > > > penv->next_cpu) { > > > > - ret =3D kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0)= ; > > > > - if (ret) { > > > > - if (ret !=3D -EINVAL) { > > > > - fprintf(stderr, "%s: %s\n", __func__, > > > > strerror(-ret)); > > > > - } > > > > - return; > > > > + if (!cap_clock_ctrl) { > > > > + return; > > > > + } > > > > + for (penv =3D first_cpu; penv !=3D NULL; penv =3D penv->ne= xt_cpu) > > > > { > > > > + ret =3D kvm_vcpu_ioctl(penv, KVM_KVMCLOCK_CTRL, 0); > > > > + if (ret) { > > > > + if (ret !=3D -EINVAL) { > > > > + fprintf(stderr, "%s: %s\n", __func__, > > > > strerror(-ret)); > > > > } > > > > + return; > > > > } > > > > } > > > > } > > > > -- > > > > 1.7.7.6 > > >=20 > > > ACK > > >=20 > > > Avi, please merge through uq/master. > >=20 > > NACK, guest should be notified when the VM is starting, not > > when stopping. >=20 > # 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).