From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH 1/3] KVM: x86: Relax accept conditions of kvm_apic_accept_pic_intr Date: Sat, 18 Oct 2008 10:29:35 +0200 Message-ID: <48F99E6F.8080700@web.de> References: <20081015142748.385784583@mchn012c.ww002.siemens.net> <20081015142748.606503565@mchn012c.ww002.siemens.net> <200810171311.11309.sheng@linux.intel.com> <48F8488E.9070700@siemens.com> <20081017163530.GA20831@yukikaze> <48F8CCC5.8060502@web.de> <20081017174722.GA24078@yukikaze> <48F8D1BD.5050709@web.de> <48F8D570.5010308@web.de> <20081018024416.GA24881@yukikaze> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig217E894F7E8D919EB2F83AB2" Cc: kvm@vger.kernel.org, avi@redhat.com, jiajun.xu@intel.com, Jan Kiszka To: Sheng Yang Return-path: Received: from fmmailgate02.web.de ([217.72.192.227]:38813 "EHLO fmmailgate02.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750745AbYJRI3l (ORCPT ); Sat, 18 Oct 2008 04:29:41 -0400 In-Reply-To: <20081018024416.GA24881@yukikaze> Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig217E894F7E8D919EB2F83AB2 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Sheng Yang wrote: > On Fri, Oct 17, 2008 at 08:12:00PM +0200, Jan Kiszka wrote: >> Now I checked also the BIOS KVM is shipping, and the MP Feature byte 2= , >> bit 7 (IMCRP) is cleared, thus KVM is providing the Virtual Wire mode.= >> Looking at Figure 3-3 of the MP spec, one can see that the PIC's outpu= t >> is connected to the LVT0 line in this mode, and that this line is >> connected to all CPUs in the system. So I can't help concluding that a= ) >> QEMU's implementation is correct and b) my patch is correct as well. O= r >> please tell me where I'm wrong now... >=20 > Frankly speaking, here are two apporoaches. Both are OK to work. You > insisted the QEmu method, emulate that line connect all lapic's LVT0. A= nd I > insisted to follow the current solution, the dot-line of virtual wire m= ode > in the spec, then make NMI watchdog as a separate thing, impact others = as > small as possible. Ack. >=20 > When I wrote NMI watchdog, I don't want to involve PIC, for it's specia= l > case of PIC usage. So I think it's OK to not emulate the path here, the= n use > apic_local_deliver() to send the interrupt directly, not through the PI= C > path. If PIC involved, that's another path. Current QEmu covered this, > pic_request_irq() send to every vcpu, emulate that whole LVT0 line. Our= KVM > choose a different way, we just assume PIC only connect to LVT0 of BSP,= for > others should be disabled. That's save a lot when you have a lot of vcp= us, > as you said. Yes, I came across this assumption that only the BSP can receive PIC interrupts as well in the meantime. I tried to first enhance the accuracy of KVMs virtual wire mode and then optimize it the way proposed for the NMI watchdog. However, I had to give up as I realized the this assumption is too deeply hooked into the KVM design. Nevertheless, one minor inaccuracy can and should be fixed (will repost as true patch after more testing): If the APIC is disabled, there will be no PIC interrupt forwarding. This should also be fixed in QEMU. --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1071,17 +1071,15 @@ int kvm_apic_has_interrupt(struct kvm_vc =20 int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu) { + struct kvm_lapic *apic =3D vcpu->arch.apic; u32 lvt0 =3D apic_get_reg(vcpu->arch.apic, APIC_LVT0); - int r =3D 0; =20 - if (vcpu->vcpu_id =3D=3D 0) { - if (!apic_hw_enabled(vcpu->arch.apic)) - r =3D 1; - if ((lvt0 & APIC_LVT_MASKED) =3D=3D 0 && - GET_APIC_DELIVERY_MODE(lvt0) =3D=3D APIC_MODE_EXTINT) - r =3D 1; - } - return r; + /* Virtual Wire mode, but we only deliver to the BSP. */ + if (vcpu->vcpu_id =3D=3D 0 && apic_hw_enabled(apic) + && !(lvt0 & APIC_LVT_MASKED) + && GET_APIC_DELIVERY_MODE(lvt0) =3D=3D APIC_MODE_EXTINT) + return 1; + return 0; } =20 void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu) >=20 > So currently, QEmu emulate virtual wire mode well, and KVM do some > simplification, only connect to BSP. Both of them follow this in each's= > code. And for KVM, the change to kvm_apic_accept_pic_intr() broke this > assumption. Now we only work PIC with BSP, but check all the vcpus. I d= on't > think that's a good combination. I think we are not likely do more to > improve our PIC connection method, so NMI watchdog in KVM was designed = as a > separate thing, as a special case, and should be the only special case.= Agreed. I'm preparing patches to take this into account while fixing the current NMI watchdog implementation. >=20 > kvm_cpu_has_interrupt() called every time before VM entry to check if > there are any intr can be injected. If lapic got none, > it would check kvm_apic_accept_pic_intr(). Check every vcpu or only che= ck > vcpu0, would bring about (vcpu_nr - 1) * ((vm_exit_nr - lapic_has_intr_= nr) / > vcpu_nr)(if we assume vmexit on every vcpu is the mostly compatiable) m= ore > times to do the judgment on other vcpus here. And normally, the latter > number would tens of thousand to hundreds of thousands. If you care abo= ut > 1000 per vcpu's touch in pit, why you don't care about them here? As I said, that case would have only mattered in an improved version if any VCPU > 1 had its LVT0 unmasked - similar optimization like for NMI WD. But things are more tricky as the PIC code and its users are not prepared to dispatch the PIC vector to multiple sinks. That finally convinced me stopping my rework. The effort became too high compared to the accuracy gain that hardly any OS may need. Jan --------------enig217E894F7E8D919EB2F83AB2 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.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkj5nnMACgkQniDOoMHTA+mv4ACeJUV8hbA61cEw3LzLb5fZG2qR uG4AnjjX7BZbCMFyvRxUqh9QBw3GIskt =YzH7 -----END PGP SIGNATURE----- --------------enig217E894F7E8D919EB2F83AB2--