Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT de-assert. (LAPIC)

[Marcelo Tosatti <mtosatti@redhat.com>]

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.