On Oct 1, 2014, at 3:09 AM, Radim Krčmář <[email protected]> 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 <[email protected]> >> --- >> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
