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 07:49:37 +0200 Message-ID: <51CA80F1.9070004@web.de> References: <20130624121915.GG18508@redhat.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="----enig2BPPWDAKAFXKNDLHPDGJJ" Cc: kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mout.web.de ([212.227.17.12]:59326 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751276Ab3FZFts (ORCPT ); Wed, 26 Jun 2013 01:49:48 -0400 In-Reply-To: <20130624121915.GG18508@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2BPPWDAKAFXKNDLHPDGJJ Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On 2013-06-24 14:19, Gleb Natapov wrote: > This reverts most of the f1ed0450a5fac7067590317cbf027f566b6ccbca. Afte= r > 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. >=20 > 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 *v= cpu) > return highest_irr; > } > =20 > -static void __apic_accept_irq(struct kvm_lapic *apic, int delivery_mod= e, > - int vector, int level, int trig_mode, > - unsigned long *dest_map); > +static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode= , > + 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. > =20 > -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq= , > - unsigned long *dest_map) > +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,= > + 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 *kvm,= 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 *kvm,= 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_mod= e, > - 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_mode= , > + 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 *a= pic, 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= ? Jan > + > + if (!result) { > if (trig_mode) > apic_debug("level trig mode repeatedly " > "for vector %d", vector); > @@ -697,7 +702,7 @@ static void __apic_accept_irq(struct kvm_lapic *api= c, int delivery_mode, > } > out: > trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode, > - trig_mode, vector, false); > + trig_mode, vector, !result); > break; > =20 > case APIC_DM_REMRD: > @@ -709,12 +714,14 @@ out: > break; > =20 > case APIC_DM_NMI: > + result =3D 1; > kvm_inject_nmi(vcpu); > kvm_vcpu_kick(vcpu); > break; > =20 > case APIC_DM_INIT: > if (!trig_mode || level) { > + result =3D 1; > /* assumes that there are only KVM_APIC_INIT/SIPI */ > apic->pending_events =3D (1UL << KVM_APIC_INIT); > /* make sure pending_events is visible before sending > @@ -731,6 +738,7 @@ out: > case APIC_DM_STARTUP: > apic_debug("SIPI to vcpu %d vector 0x%02x\n", > vcpu->vcpu_id, vector); > + result =3D 1; > apic->sipi_vector =3D vector; > /* make sure sipi_vector is visible for the receiver */ > smp_wmb(); > @@ -752,6 +760,7 @@ out: > delivery_mode); > break; > } > + return result; > } > =20 > int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcp= u2) > @@ -1461,7 +1470,7 @@ int apic_has_pending_timer(struct kvm_vcpu *vcpu)= > return 0; > } > =20 > -void kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type) > +int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type) > { > u32 reg =3D kvm_apic_get_reg(apic, lvt_type); > int vector, mode, trig_mode; > @@ -1470,8 +1479,10 @@ void kvm_apic_local_deliver(struct kvm_lapic *ap= ic, int lvt_type) > vector =3D reg & APIC_VECTOR_MASK; > mode =3D reg & APIC_MODE_MASK; > trig_mode =3D reg & APIC_LVT_LEVEL_TRIGGER; > - __apic_accept_irq(apic, mode, vector, 1, trig_mode, NULL); > + return __apic_accept_irq(apic, mode, vector, 1, trig_mode, > + NULL); > } > + return 0; > } > =20 > void kvm_apic_nmi_wd_deliver(struct kvm_vcpu *vcpu) > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index 61a73a0..c730ac9 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -57,9 +57,9 @@ void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *= tmr); > void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir); > int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest); > int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda); > -void kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq= , > - unsigned long *dest_map); > -void kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type); > +int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,= > + unsigned long *dest_map); > +int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type); > =20 > bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *= src, > struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map); > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c > index ef1817b..e2e6b44 100644 > --- a/virt/kvm/irq_comm.c > +++ b/virt/kvm/irq_comm.c > @@ -91,8 +91,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct = kvm_lapic *src, > if (!kvm_is_dm_lowest_prio(irq)) { > if (r < 0) > r =3D 0; > - kvm_apic_set_irq(vcpu, irq, dest_map); > - r++; > + r +=3D kvm_apic_set_irq(vcpu, irq, dest_map); > } else if (kvm_lapic_enabled(vcpu)) { > if (!lowest) > lowest =3D vcpu; > @@ -101,10 +100,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, stru= ct kvm_lapic *src, > } > } > =20 > - if (lowest) { > - kvm_apic_set_irq(lowest, irq, dest_map); > - r =3D 1; > - } > + if (lowest) > + r =3D kvm_apic_set_irq(lowest, irq, dest_map); > =20 > return r; > } > -- > Gleb. >=20 ------enig2BPPWDAKAFXKNDLHPDGJJ 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/ iEYEARECAAYFAlHKgPEACgkQitSsb3rl5xTCywCeKVjdfp2X7/l79M01FUA7Ejri wqoAoIGV44T4ewNENBngtOROhE2krmGu =hVMn -----END PGP SIGNATURE----- ------enig2BPPWDAKAFXKNDLHPDGJJ--