On Oct 1, 2014, at 3:09 AM, Radim Krčmář <rkrcmar@xxxxxxxxxx> wrote:
> 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 …)
Not in real OSes, but in some tests I run.
>
>> 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 …)
I meant if the ICR destination mode is logical (not the delivery-mode) and there is no more than one cluster (if clusters exist in this mode).
[It also applies to logical cluster-mode.] Does this phrasing makes sense to you?
>
>>
>> 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.
I actually submitted a fix for cluster mode - http://www.spinics.net/lists/kvm/msg106351.html
I now see Paolo did not apply it or responded. I will combine the two patches to one patch-set for v2.
>
>> + if (mda == (u32)-1)
>> + return 1; /* x2apic broadcast */
>
> (It would be nicer to use named symbol instead of a comment for
> explaining the code.)
Agreed. I looked for an existing constant, found none, and did not want to touch apicdef.h.
I will do so in v2.
>
>> @@ -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.
I agree that broadcast may be done more efficiently, but broadcast using shorthand is also done slowly today.
So I think it should go into different patch.
>
>> + 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 :)
Thanks for the feedback.
I completely agree with your revisions, yet I think they are orthogonal and should go into different patches.
Since my current project focus on validation of hypervisors, I am not sure I will have the time to do it well soon.
Regardless, I am not sure whether the performance impact of taking the slow-path is significant.
Nadav
--
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
