From mboxrd@z Thu Jan  1 00:00:00 1970
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Subject: Re: [PATCH v2] KVM: x86: some apic broadcast modes does not work
Date: Fri, 3 Oct 2014 14:49:12 +0200
Message-ID: <20141003124912.GA28730@potion.brq.redhat.com>
References: <1412285452-27062-1-git-send-email-namit@cs.technion.ac.il>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: pbonzini@redhat.com, kvm@vger.kernel.org, nadav.amit@gmail.com
To: Nadav Amit <namit@cs.technion.ac.il>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:34548 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751948AbaJCMtW (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 3 Oct 2014 08:49:22 -0400
Content-Disposition: inline
In-Reply-To: <1412285452-27062-1-git-send-email-namit@cs.technion.ac.il>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

2014-10-03 00:30+0300, Nadav Amit:
> KVM does not deliver x2APIC broadcast messages with physical mode.  I=
ntel SDM
> (10.12.9 ICR Operation in x2APIC Mode) states: "A destination ID valu=
e of
> FFFF_FFFFH is used for broadcast of interrupts in both logical destin=
ation and
> physical destination modes."
>=20
> In addition, the local-apic enables cluster mode broadcast. As Intel =
SDM
> 10.6.2.2 says: "Broadcast to all local APICs is achieved by setting a=
ll
> destination bits to one." This patch enables cluster mode broadcast.
>=20
> The fix tries to combine broadcast in different modes through a unifi=
ed code.
>=20
> One rare case occurs when the source of IPI has its APIC disabled.  I=
n such
> case, the source can still issue IPIs, but since the source is not ob=
liged to
> have the same LAPIC mode as the enabled ones, we cannot rely on it.
> Since it is a rare case, it is unoptimized and done on the slow-path.
>=20
> ---

Thanks!

Reviewed-by: Radim Kr=C4=8Dm=C3=A1=C5=99 <rkrcmar@redhat.com>

> Changes v1->v2:
> - Follow Radim's review: setting constants, preferring simplicity to =
marginal
>   performance gain, etc.
> - Combine the cluster mode and x2apic mode patches
>=20
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index b8345dd..ee04adf 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -68,6 +68,9 @@
>  #define MAX_APIC_VECTOR			256
>  #define APIC_VECTORS_PER_REG		32
> =20
> +#define APIC_BROADCAST			0xFF
> +#define X2APIC_BROADCAST		0xFFFFFFFFul

(int is better -- using long introduces an interesting feature with
 implicit retyping: (int)X2APIC_BROADCAST !=3D X2APIC_BROADCAST, and we
 don't compile with -Wtype-limits to notice it.  It poses no problem
 now, so I can change it in an inevitable cleanup series / convince lkm=
l
 to endorse stricter warnings.)

> +
>  #define VEC_POS(v) ((v) & (32 - 1))
>  #define REG_POS(v) (((v) >> 5) << 4)
> =20
> @@ -558,16 +563,25 @@ static void apic_set_tpr(struct kvm_lapic *apic=
, u32 tpr)
>  	apic_update_ppr(apic);
>  }
> =20
> -int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest)
> +static int kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest)

(bool is better.)

> +{
> +	return dest =3D=3D (apic_x2apic_mode(apic) ?
> +			X2APIC_BROADCAST : APIC_BROADCAST);
> +}