From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH] KVM: Fix RTC interrupt coalescing tracking Date: Wed, 26 Jun 2013 08:26:43 +0200 Message-ID: <51CA89A3.7020709@web.de> References: <20130624121915.GG18508@redhat.com> <51CA80F1.9070004@web.de> <20130626061540.GA1038@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="----enig2EURATORQFQBTLMGDVUFR" Cc: kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mout.web.de ([212.227.15.3]:58402 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751374Ab3FZG0v (ORCPT ); Wed, 26 Jun 2013 02:26:51 -0400 In-Reply-To: <20130626061540.GA1038@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2EURATORQFQBTLMGDVUFR Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2013-06-26 08:15, Gleb Natapov wrote: > On Wed, Jun 26, 2013 at 07:49:37AM +0200, Jan Kiszka wrote: >> On 2013-06-24 14:19, Gleb Natapov wrote: >>> This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. Af= ter >>> the commit kvm_apic_set_irq() no longer returns accurate information >>> about interrupt injection status if injection is done into disabled >>> APIC. RTC interrupt coalescing tracking relies on the information to = be >>> accurate and cannot recover if it is not. >>> >>> Signed-off-by: Gleb Natapov >>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >>> index 9d75193..9f4bea8 100644 >>> --- a/arch/x86/kvm/lapic.c >>> +++ b/arch/x86/kvm/lapic.c >>> @@ -405,17 +405,17 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu = *vcpu) >>> return highest_irr; >>> } >>> =20 >>> -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_m= ode, >>> - int vector, int level, int trig_mode, >>> - unsigned long *dest_map); >>> +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mo= de, >>> + int vector, int level, int trig_mode, >>> + unsigned long *dest_map); >> >> I still think __apic_accept_irq should unconditionally inject, and the= >> test for acpi_enabled belongs into kvm_apic_set_irq. Why should >> __apic_accept_irq accept non-APIC_DM_FIXED messages if the APIC is off= ? >> See below for another reason to refactor this part of the interface. >> > 10.4.7.2 Local APIC State After It Has Been Software Disabled >=20 > The local APIC will respond normally to INIT, NMI, SMI, and SIPI > messages. OK, I see. >=20 >>> =20 >>> -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *i= rq, >>> - unsigned long *dest_map) >>> +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *ir= q, >>> + unsigned long *dest_map) >>> { >>> struct kvm_lapic *apic =3D vcpu->arch.apic; >>> =20 >>> - __apic_accept_irq(apic, irq->delivery_mode, irq->vector, >>> - irq->level, irq->trig_mode, dest_map); >>> + return __apic_accept_irq(apic, irq->delivery_mode, irq->vector, >>> + irq->level, irq->trig_mode, dest_map); >>> } >>> =20 >>> static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val) >>> @@ -608,8 +608,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kv= m, struct kvm_lapic *src, >>> *r =3D -1; >>> =20 >>> if (irq->shorthand =3D=3D APIC_DEST_SELF) { >>> - kvm_apic_set_irq(src->vcpu, irq, dest_map); >>> - *r =3D 1; >>> + *r =3D kvm_apic_set_irq(src->vcpu, irq, dest_map); >>> return true; >>> } >>> =20 >>> @@ -654,8 +653,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kv= m, struct kvm_lapic *src, >>> continue; >>> if (*r < 0) >>> *r =3D 0; >>> - kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map); >>> - *r +=3D 1; >>> + *r +=3D kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map); >>> } >>> =20 >>> ret =3D true; >>> @@ -664,11 +662,15 @@ out: >>> return ret; >>> } >>> =20 >>> -/* Set an IRQ pending in the lapic. */ >>> -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_m= ode, >>> - int vector, int level, int trig_mode, >>> - unsigned long *dest_map) >>> +/* >>> + * Add a pending IRQ into lapic. >>> + * Return 1 if successfully added and 0 if discarded. >>> + */ >>> +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mo= de, >>> + int vector, int level, int trig_mode, >>> + unsigned long *dest_map) >>> { >>> + int result =3D 0; >>> struct kvm_vcpu *vcpu =3D apic->vcpu; >>> =20 >>> switch (delivery_mode) { >>> @@ -682,10 +684,13 @@ static void __apic_accept_irq(struct kvm_lapic = *apic, int delivery_mode, >>> if (dest_map) >>> __set_bit(vcpu->vcpu_id, dest_map); >>> =20 >>> - if (kvm_x86_ops->deliver_posted_interrupt) >>> + if (kvm_x86_ops->deliver_posted_interrupt) { >>> + result =3D 1; >>> kvm_x86_ops->deliver_posted_interrupt(vcpu, vector); >>> - else { >>> - if (apic_test_and_set_irr(vector, apic)) { >>> + } else { >>> + result =3D !apic_test_and_set_irr(vector, apic); >> >> This part of the revert makes no sense. If deliver_posted_interrupt is= >> on, we don't have this feedback anymore, thus we decided to remove it,= no? >> > Agree, but I wanted to do clear revert and fix on top. Fine with me, let's write a separate fix. Jan ------enig2EURATORQFQBTLMGDVUFR Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlHKiaQACgkQitSsb3rl5xRL+ACfTx5mc+lrAya8lmU6qjelFbmI D4wAnRDZeVlR1RwAHZTPy/S5MPefwx/I =UNyp -----END PGP SIGNATURE----- ------enig2EURATORQFQBTLMGDVUFR--