From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC) Date: Mon, 16 Jan 2012 08:18:36 -0200 Message-ID: <20120116101836.GB7937@amt.cnet> References: <1324257267-17200-1-git-send-email-js@alien8.de> <20111223104031.GB24308@amt.cnet> <1326388071.12267.13.camel@tabernacle> <20120113105237.GB25595@amt.cnet> <1326455179.9160.7.camel@tabernacle> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org To: Julian Stecklina Return-path: Received: from mx1.redhat.com ([209.132.183.28]:33040 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754058Ab2APK5A (ORCPT ); Mon, 16 Jan 2012 05:57:00 -0500 Content-Disposition: inline In-Reply-To: <1326455179.9160.7.camel@tabernacle> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Jan 13, 2012 at 12:46:19PM +0100, Julian Stecklina wrote: > Am Freitag, den 13.01.2012, 08:52 -0200 schrieb Marcelo Tosatti: > > On Thu, Jan 12, 2012 at 06:07:51PM +0100, Julian Stecklina wrote: > > > Am Freitag, den 23.12.2011, 08:40 -0200 schrieb Marcelo Tosatti: > > > > On Mon, Dec 19, 2011 at 02:14:27AM +0100, Julian Stecklina wrot= e: > > > > > If the guest programs an IPI with level=3D0 (de-assert) and t= rig_mode=3D0 (edge), > > > > > it is erroneously treated as INIT de-assert and ignored, but = to quote the > > > > > spec: "For this delivery mode [INIT de-assert], the level fla= g must be set to > > > > > 0 and trigger mode flag to 1." > > > >=20 > > > > Yes, the implementation ignores INIT de-assert. Quoting the spe= c: > > > >=20 > > > > "(INIT Level De-assert) (Not supported in the Pentium 4 and Int= el Xeon > > > > processors.)" > > > >=20 > > > > Your patch below is not improving the implementation to be clos= er to the > > > > spec: it'll trigger the INIT state initialization with trig_mod= e =3D=3D 0 > > > > (which is not in accordance with your spec quote above). > > >=20 > > > After reading the spec again and consulting with the guy who wrot= e the > > > code triggering this, it seems the whole "if (level)" in the code= path > > > below is superfluous.=20 > >=20 > > No. Look at whats inside "if (level)": the mp_state assignment is t= he > > internal implementation of "delivers an INIT request to the target > > processor". > >=20 > > According to the spec, the INIT level de-assert=20 > >=20 > > "Sends a synchronization message to all the local APICs in the syst= em > > to set their arbitration IDs (stored in their Arb ID registers) to = the > > values of their APIC IDs (see Section 10.7, =E2=80=9CSystem and API= C Bus > > Arbitration=E2=80=9D)." > >=20 > > So if you remove the "if (level)" check, INIT de-assert will be emu= lated > > as INIT! >=20 > Newer processors don't support INIT level de-assert and will interpre= t > this as INIT. Without the "if (level)" check, KVM would behave in the > same way, thus not breaking code that actually runs on real processor= s. >=20 > For processors that still supported INIT level de-assert: If you look > into older specs (243192), you read: >=20 > 101 (INIT) ... INIT is treated as an edge triggered interrupt even if > programmed otherwise. >=20 > 101 (INIT Level De-assert) The trigger mode must also be set to 1 and > level mode to 0. >=20 > This means that if you don't set trigger mode to 1, you will get an I= NIT > instead of INIT level de-assert. This is where the current code in KV= M > is wrong. So with my original patch, KVM would behave like the old sp= ec > mandates (check for trigger mode). With the "if (level)" check remove= d, > it would behave like recent processors. Either way, the current code = is > bogus. >=20 > Regards, Julian Yes, the original patch is fine. Please resend it.