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

[Wanpeng Li <wanpeng.li@linux.intel.com>]

On Fri, Oct 03, 2014 at 12:30:52AM +0300, Nadav Amit wrote:
>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.
>
>---
>
>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>
>---

Reviewed-by: Wanpeng Li <wanpeng.li@linux.intel.com>

> arch/x86/include/asm/kvm_host.h |  2 +-
> arch/x86/kvm/lapic.c            | 28 ++++++++++++++++++++++------
> arch/x86/kvm/lapic.h            |  4 ++--
> virt/kvm/ioapic.h               |  2 +-
> 4 files changed, 26 insertions(+), 10 deletions(-)
>
>diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>index 7d603a7..6f2be83 100644
>--- a/arch/x86/include/asm/kvm_host.h
>+++ b/arch/x86/include/asm/kvm_host.h
>@@ -542,7 +542,7 @@ struct kvm_apic_map {
> 	struct rcu_head rcu;
> 	u8 ldr_bits;
> 	/* fields bellow are used to decode ldr values in different modes */
>-	u32 cid_shift, cid_mask, lid_mask;
>+	u32 cid_shift, cid_mask, lid_mask, broadcast;
> 	struct kvm_lapic *phys_map[256];
> 	/* first index is cluster id second is cpu id in a cluster */
> 	struct kvm_lapic *logical_map[16][16];
>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
>+
> #define VEC_POS(v) ((v) & (32 - 1))
> #define REG_POS(v) (((v) >> 5) << 4)
> 
>@@ -149,6 +152,7 @@ static void recalculate_apic_map(struct kvm *kvm)
> 	new->cid_shift = 8;
> 	new->cid_mask = 0;
> 	new->lid_mask = 0xff;
>+	new->broadcast = APIC_BROADCAST;
> 
> 	kvm_for_each_vcpu(i, vcpu, kvm) {
> 		struct kvm_lapic *apic = vcpu->arch.apic;
>@@ -170,6 +174,7 @@ static void recalculate_apic_map(struct kvm *kvm)
> 			new->cid_shift = 16;
> 			new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1;
> 			new->lid_mask = 0xffff;
>+			new->broadcast = X2APIC_BROADCAST;
> 		} else if (kvm_apic_sw_enabled(apic) &&
> 				!new->cid_mask /* flat mode */ &&
> 				kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_CLUSTER) {
>@@ -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)
>+{
>+	return dest == (apic_x2apic_mode(apic) ?
>+			X2APIC_BROADCAST : APIC_BROADCAST);
>+}
>+
>+int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
> {
>-	return dest == 0xff || kvm_apic_id(apic) == dest;
>+	return kvm_apic_id(apic) == dest || kvm_apic_broadcast(apic, dest);
> }
> 
>-int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
>+int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
> {
> 	int result = 0;
> 	u32 logical_id;
> 
>+	if (kvm_apic_broadcast(apic, mda))
>+		return 1;
>+
> 	if (apic_x2apic_mode(apic)) {
> 		logical_id = kvm_apic_get_reg(apic, APIC_LDR);
> 		return logical_id & mda;
>@@ -595,7 +609,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda)
> }
> 
> int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>-			   int short_hand, int dest, int dest_mode)
>+			   int short_hand, unsigned int dest, int dest_mode)
> {
> 	int result = 0;
> 	struct kvm_lapic *target = vcpu->arch.apic;
>@@ -657,9 +671,11 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> 	if (!map)
> 		goto out;
> 
>+	if (irq->dest_id == map->broadcast)
>+		goto out;
>+
> 	if (irq->dest_mode == 0) { /* physical mode */
>-		if (irq->delivery_mode == APIC_DM_LOWEST ||
>-				irq->dest_id == 0xff)
>+		if (irq->delivery_mode == APIC_DM_LOWEST)
> 			goto out;
> 		dst = &map->phys_map[irq->dest_id & 0xff];
> 	} else {
>diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>index 6a11845..3fd6c22 100644
>--- a/arch/x86/kvm/lapic.h
>+++ b/arch/x86/kvm/lapic.h
>@@ -55,8 +55,8 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu);
> 
> void kvm_apic_update_tmr(struct kvm_vcpu *vcpu, u32 *tmr);
> void kvm_apic_update_irr(struct kvm_vcpu *vcpu, u32 *pir);
>-int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest);
>-int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda);
>+int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest);
>+int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda);
> int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
> 		unsigned long *dest_map);
> int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
>diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
>index e23b706..31725a3 100644
>--- a/virt/kvm/ioapic.h
>+++ b/virt/kvm/ioapic.h
>@@ -83,7 +83,7 @@ static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm)
> 
> void kvm_rtc_eoi_tracking_restore_one(struct kvm_vcpu *vcpu);
> int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>-		int short_hand, int dest, int dest_mode);
>+		int short_hand, unsigned int dest, int dest_mode);
> int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
> void kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, int vector,
> 			int trigger_mode);
>-- 
>1.9.1
>
>--
>To unsubscribe from this list: send the line "unsubscribe kvm" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html