From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37402) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJP0a-0005up-CC for qemu-devel@nongnu.org; Wed, 03 Oct 2012 09:29:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TJP0U-0006l1-Fn for qemu-devel@nongnu.org; Wed, 03 Oct 2012 09:29:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TJP0U-0006kp-7u for qemu-devel@nongnu.org; Wed, 03 Oct 2012 09:28:58 -0400 Date: Wed, 3 Oct 2012 10:28:06 -0300 From: Marcelo Tosatti 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-Disposition: inline In-Reply-To: <1389829631.4053667.1349056207458.JavaMail.root@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/1] kvmclock: fix guest stop notification List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong Cc: kvm list , Raghavendra K T , qemu list , Eric B Munson , Amit Shah , Paolo Bonzini , Laszlo Ersek , Andreas =?iso-8859-1?Q?F=E4rber?= 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 ab= out 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 gets > > > > 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 current = code? > which kind of context? >=20 > Someone told me he got call trace when shutdown guest by 'init 0', I di= dn'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_sta= te'. > > > > 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->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; > > > > } > > > > } > > > > } > > > > -- > > > > 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).