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 wrote: > > > > > If the guest programs an IPI with level=0 (de-assert) and trig_mode=0 (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 flag must be set to > > > > > 0 and trigger mode flag to 1." > > > > > > > > Yes, the implementation ignores INIT de-assert. Quoting the spec: > > > > > > > > "(INIT Level De-assert) (Not supported in the Pentium 4 and Intel Xeon > > > > processors.)" > > > > > > > > Your patch below is not improving the implementation to be closer to the > > > > spec: it'll trigger the INIT state initialization with trig_mode == 0 > > > > (which is not in accordance with your spec quote above). > > > > > > After reading the spec again and consulting with the guy who wrote the > > > code triggering this, it seems the whole "if (level)" in the code path > > > below is superfluous. > > > > No. Look at whats inside "if (level)": the mp_state assignment is the > > internal implementation of "delivers an INIT request to the target > > processor". > > > > According to the spec, the INIT level de-assert > > > > "Sends a synchronization message to all the local APICs in the system > > to set their arbitration IDs (stored in their Arb ID registers) to the > > values of their APIC IDs (see Section 10.7, “System and APIC Bus > > Arbitration”)." > > > > So if you remove the "if (level)" check, INIT de-assert will be emulated > > as INIT! > > Newer processors don't support INIT level de-assert and will interpret > this as INIT. Without the "if (level)" check, KVM would behave in the > same way, thus not breaking code that actually runs on real processors. > > For processors that still supported INIT level de-assert: If you look > into older specs (243192), you read: > > 101 (INIT) ... INIT is treated as an edge triggered interrupt even if > programmed otherwise. > > 101 (INIT Level De-assert) The trigger mode must also be set to 1 and > level mode to 0. > > This means that if you don't set trigger mode to 1, you will get an INIT > instead of INIT level de-assert. This is where the current code in KVM > is wrong. So with my original patch, KVM would behave like the old spec > mandates (check for trigger mode). With the "if (level)" check removed, > it would behave like recent processors. Either way, the current code is > bogus. > > Regards, Julian Yes, the original patch is fine. Please resend it. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
