Re: [PATCH v2] KVM: x86: some apic broadcast modes does not work

["Radim Krčmář" <rkrcmar@redhat.com>]

2014-10-03 00:30+0300, Nadav Amit:
> KVM does not deliver x2APIC broadcast messages with physical mode.  Intel SDM
> (10.12.9 ICR Operation in x2APIC Mode) states: "A destination ID value of
> FFFF_FFFFH is used for broadcast of interrupts in both logical destination and
> physical destination modes."
> 
> 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 all
> destination bits to one." This patch enables cluster mode broadcast.
> 
> The fix tries to combine broadcast in different modes through a unified code.
> 
> One rare case occurs when the source of IPI has its APIC disabled.  In such
> case, the source can still issue IPIs, but since the source is not obliged 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.
> 
> ---

Thanks!

Reviewed-by: Radim Krčmář <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
> 
> 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
>  
> +#define APIC_BROADCAST			0xFF
> +#define X2APIC_BROADCAST		0xFFFFFFFFul

(int is better -- using long introduces an interesting feature with
 implicit retyping: (int)X2APIC_BROADCAST != 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 lkml
 to endorse stricter warnings.)

> +
>  #define VEC_POS(v) ((v) & (32 - 1))
>  #define REG_POS(v) (((v) >> 5) << 4)
>  
> @@ -558,16 +563,25 @@ static void apic_set_tpr(struct kvm_lapic *apic, u32 tpr)
>  	apic_update_ppr(apic);
>  }
>  
> -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 == (apic_x2apic_mode(apic) ?
> +			X2APIC_BROADCAST : APIC_BROADCAST);
> +}