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@xxxxxxxxxxxxxxxxx>
> ---
> 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 :)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
