From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH] KVM: Don't mistreat edge-triggered INIT IPI as INIT
 de-assert. (LAPIC)
Date: Fri, 13 Jan 2012 08:52:37 -0200
Message-ID: <20120113105237.GB25595@amt.cnet>
References: <1324257267-17200-1-git-send-email-js@alien8.de>
 <20111223104031.GB24308@amt.cnet>
 <1326388071.12267.13.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 <js@alien8.de>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:38495 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752721Ab2AMKxw (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 13 Jan 2012 05:53:52 -0500
Content-Disposition: inline
In-Reply-To: <1326388071.12267.13.camel@tabernacle>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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=3D0 (de-assert) and trig_=
mode=3D0 (edge),
> > > it is erroneously treated as INIT de-assert and ignored, but to q=
uote the
> > > spec: "For this delivery mode [INIT de-assert], the level flag mu=
st be set to
> > > 0 and trigger mode flag to 1."
> >=20
> > Yes, the implementation ignores INIT de-assert. Quoting the spec:
> >=20
> > "(INIT Level De-assert) (Not supported in the Pentium 4 and Intel X=
eon
> > processors.)"
> >=20
> > Your patch below is not improving the implementation to be closer t=
o the
> > spec: it'll trigger the INIT state initialization with trig_mode =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 wrote th=
e
> code triggering this, it seems the whole "if (level)" in the code pat=
h
> below is superfluous.=20

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=20

"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, =E2=80=9CSystem and APIC Bu=
s
Arbitration=E2=80=9D)."

So if you remove the "if (level)" check, INIT de-assert will be emulate=
d
as INIT!

> Regarding level my spec says:
>=20
> "This flag has no meaning in Pentium 4 and Intel Xeon processors, and
> will always be issued as a 1."
>=20
> Judging from Section 10.2 this means every CPU where the LAPICs
> communicate over the system bus (everything after and including the P=
4).
> Thus the if can be removed, since its condition is always true
> (regardless of what the programmer actually programmed into the level
> field).
>=20
> If there is no objection, I'll submit a revised patch.
>=20
> > > Signed-off-by: Julian Stecklina <js@alien8.de>
> > > ---
> > >  arch/x86/kvm/lapic.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >=20
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index a7f3e65..260770d 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -433,7 +433,7 @@ static int __apic_accept_irq(struct kvm_lapic=
 *apic, int delivery_mode,
> > >  		break;
> > > =20
> > >  	case APIC_DM_INIT:
> > > -		if (level) {
> > > +		if (!trig_mode || level) {
> > >  			result =3D 1;
> > >  			vcpu->arch.mp_state =3D KVM_MP_STATE_INIT_RECEIVED;
> > >  			kvm_make_request(KVM_REQ_EVENT, vcpu);
> > > --=20
> > > 1.7.7.4
> > >=20
>=20
> Regards, Julian
>=20