Re: [PATCH] KVM: x86: x2apic broadcast with physical mode does not work

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

2014-09-29 21:15+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."

Btw. have you hit it in the wild?  (It was there so long that I suppose
that all OSes use a shorthand ...)

> The fix tries to combine broadcast in different modes.  Broadcast can be done
> in the fast-path if the delivery-mode is logical and there is only a single
> cluster.  Otherwise, do it slowly.

(Flat logical xapic or current logical x2apic; it doesn't have to do
 much with clusters from Intel's vocabulary ...)

> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> @@ -558,17 +560,21 @@ 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)

(My kingdom for an explanation of how u16 got here.)

> +int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)

(bool would be better, but we should convert the whole stack, so it's a
 material for a new patch.)

>  {
> -	return dest == 0xff || kvm_apic_id(apic) == dest;
> +	u32 broadcast = apic_x2apic_mode(apic) ? (u32)-1 : 0xff;
> +
> +	return (dest == broadcast || kvm_apic_id(apic) == dest);

I'd introduce a helper for this, as it makes some sense to use it later

  static inline u32 kvm_apic_broadcast(struct kvm_lapic *apic)
  {
  	return apic_x2apic_mode(apic) ? (u32)-1 : 0xff;
  }

> -int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
> +int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)

(And a horse for reasons behind dest/mda schism.)

>  {
>  	int result = 0;
>  	u32 logical_id;
>  

>  	if (apic_x2apic_mode(apic)) {

No need to be x2apic specific, we could use kvm_apic_broadcast() to
"optimize" the xapic path as well.

xapic broadcast is 0xff in flat and cluster mode, so it would actually
be another bugfix -- we miss the latter one right now; not that it has
any users.

> +		if (mda == (u32)-1)
> +			return 1; /* x2apic broadcast */

(It would be nicer to use named symbol instead of a comment for
 explaining the code.)

> @@ -657,9 +663,13 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  	if (!map)
>  		goto out;
>  
> +	/* broadcast on physical or multiple clusters is done slowly */
> +	if (irq->dest_id == map->broadcast &&

(If we dropped the second && condition, broadcast check could be moved
 before we rcu_dereference the map.  Using kvm_apic_broadcast instead.)

> +	    (irq->dest_mode == 0 || map->cid_mask != 0))

It's a clever and pretty hacky use of cid_mask[1] ... I think it would
be better to have a broadcast fast-path for all modes, so we would do
something like this

  if (irq->dest == kvm_apic_broadcast(apic))
  	return HANDLE_BROADCAST(...);

in the end and having 'return 0' fallback for now is closer to our goal.

> +		goto out;


---
1: When we fix logical x2apic, it will have cid_mask > 0 and hopefully
   no-one uses it now, broken and capped at one 16 CPU cluster.
   That would leave us with flat logical mode that supports up to 8
   CPUs, which is a hard performance decision -- I think that most
   guests will use other modes and the universe will lose more cycles on
   checking for this than it would on broadcasting through slow path :)