All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] KVM: minor APIC fixes and cleanups
@ 2015-01-29 21:48 Radim Krčmář
  2015-01-29 21:48 ` [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*() Radim Krčmář
                   ` (8 more replies)
  0 siblings, 9 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-01-29 21:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

This patch series is made our of three logical parts,

[1-3/8] are just a cleanup and could be omitted
[4-6/8] improve broadcast detection and unoptimized delivery
[7-8/8] handle mixed mode (by falling back to improvements from [5-6/9])

Radim Krčmář (8):
  KVM: x86: return bool from kvm_apic_match*()
  KVM: x86: cleanup kvm_apic_match_*()
  KVM: x86: replace 0 with APIC_DEST_PHYSICAL
  KVM: x86: fix x2apic logical address matching
  KVM: x86: use MDA for interrupt matching
  KVM: x86: allow mixed APIC mode broadcast
  KVM: x86: avoid logical_map when it is invalid
  KVM: x86: simplify kvm_apic_map

 arch/x86/include/asm/kvm_host.h |   4 +-
 arch/x86/kvm/ioapic.h           |   2 +-
 arch/x86/kvm/lapic.c            | 176 +++++++++++++++++++---------------------
 arch/x86/kvm/lapic.h            |  35 ++++----
 4 files changed, 106 insertions(+), 111 deletions(-)

-- 
2.2.2


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*()
  2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
@ 2015-01-29 21:48 ` Radim Krčmář
  2015-01-29 22:10   ` Joe Perches
  2015-01-29 21:48 ` [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*() Radim Krčmář
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 34+ messages in thread
From: Radim Krčmář @ 2015-01-29 21:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

And don't export the internal ones while at it.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/ioapic.h | 2 +-
 arch/x86/kvm/lapic.c  | 8 ++++----
 arch/x86/kvm/lapic.h  | 2 --
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/ioapic.h b/arch/x86/kvm/ioapic.h
index 3c9195535ffc..c2e36d934af4 100644
--- a/arch/x86/kvm/ioapic.h
+++ b/arch/x86/kvm/ioapic.h
@@ -98,7 +98,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,
+bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 		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,
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d1b7d42c3af0..478dd8bd653b 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -581,18 +581,18 @@ static void apic_set_tpr(struct kvm_lapic *apic, u32 tpr)
 	apic_update_ppr(apic);
 }
 
-static int kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest)
+static bool 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)
+static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
 {
 	return kvm_apic_id(apic) == dest || kvm_apic_broadcast(apic, dest);
 }
 
-int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
+static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 {
 	int result = 0;
 	u32 logical_id;
@@ -626,7 +626,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 	return result;
 }
 
-int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
+bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 			   int short_hand, unsigned int dest, int dest_mode)
 {
 	int result = 0;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 7054437944cd..c1ef25c89508 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -58,8 +58,6 @@ 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, 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);
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()
  2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
  2015-01-29 21:48 ` [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*() Radim Krčmář
@ 2015-01-29 21:48 ` Radim Krčmář
  2015-01-30  8:52   ` Paolo Bonzini
  2015-01-29 21:48 ` [PATCH 3/8] KVM: x86: replace 0 with APIC_DEST_PHYSICAL Radim Krčmář
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 34+ messages in thread
From: Radim Krčmář @ 2015-01-29 21:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

The majority of this patch turns
  result = 0; if (CODE) result = 1; return result;
into
  return CODE;
because we return bool now.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 478dd8bd653b..271e7d076879 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -594,42 +594,34 @@ static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
 
 static bool 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;
+		return true;
 
-	if (apic_x2apic_mode(apic)) {
-		logical_id = kvm_apic_get_reg(apic, APIC_LDR);
+	logical_id = kvm_apic_get_reg(apic, APIC_LDR);
+
+	if (apic_x2apic_mode(apic))
 		return logical_id & mda;
-	}
 
-	logical_id = GET_APIC_LOGICAL_ID(kvm_apic_get_reg(apic, APIC_LDR));
+	logical_id = GET_APIC_LOGICAL_ID(logical_id);
 
 	switch (kvm_apic_get_reg(apic, APIC_DFR)) {
 	case APIC_DFR_FLAT:
-		if (logical_id & mda)
-			result = 1;
-		break;
+		return logical_id & mda;
 	case APIC_DFR_CLUSTER:
-		if (((logical_id >> 4) == (mda >> 0x4))
-		    && (logical_id & mda & 0xf))
-			result = 1;
-		break;
+		return ((logical_id >> 4) == (mda >> 4))
+		       && (logical_id & mda & 0xf);
 	default:
 		apic_debug("Bad DFR vcpu %d: %08x\n",
 			   apic->vcpu->vcpu_id, kvm_apic_get_reg(apic, APIC_DFR));
-		break;
+		return false;
 	}
-
-	return result;
 }
 
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 			   int short_hand, unsigned int dest, int dest_mode)
 {
-	int result = 0;
 	struct kvm_lapic *target = vcpu->arch.apic;
 
 	apic_debug("target %p, source %p, dest 0x%x, "
@@ -641,27 +633,21 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 	case APIC_DEST_NOSHORT:
 		if (dest_mode == 0)
 			/* Physical mode. */
-			result = kvm_apic_match_physical_addr(target, dest);
+			return kvm_apic_match_physical_addr(target, dest);
 		else
 			/* Logical mode. */
-			result = kvm_apic_match_logical_addr(target, dest);
-		break;
+			return kvm_apic_match_logical_addr(target, dest);
 	case APIC_DEST_SELF:
-		result = (target == source);
-		break;
+		return target == source;
 	case APIC_DEST_ALLINC:
-		result = 1;
-		break;
+		return true;
 	case APIC_DEST_ALLBUT:
-		result = (target != source);
-		break;
+		return target != source;
 	default:
 		apic_debug("kvm: apic: Bad dest shorthand value %x\n",
 			   short_hand);
-		break;
+		return false;
 	}
-
-	return result;
 }
 
 bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 3/8] KVM: x86: replace 0 with APIC_DEST_PHYSICAL
  2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
  2015-01-29 21:48 ` [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*() Radim Krčmář
  2015-01-29 21:48 ` [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*() Radim Krčmář
@ 2015-01-29 21:48 ` Radim Krčmář
  2015-01-29 21:48 ` [PATCH 4/8] KVM: x86: fix x2apic logical address matching Radim Krčmář
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-01-29 21:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

To make the code self-documenting.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 271e7d076879..0ee743c6b4f1 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -631,11 +631,9 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 	ASSERT(target);
 	switch (short_hand) {
 	case APIC_DEST_NOSHORT:
-		if (dest_mode == 0)
-			/* Physical mode. */
+		if (dest_mode == APIC_DEST_PHYSICAL)
 			return kvm_apic_match_physical_addr(target, dest);
 		else
-			/* Logical mode. */
 			return kvm_apic_match_logical_addr(target, dest);
 	case APIC_DEST_SELF:
 		return target == source;
@@ -680,7 +678,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 
 	ret = true;
 
-	if (irq->dest_mode == 0) { /* physical mode */
+	if (irq->dest_mode == APIC_DEST_PHYSICAL) {
 		if (irq->dest_id >= ARRAY_SIZE(map->phys_map))
 			goto out;
 
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 4/8] KVM: x86: fix x2apic logical address matching
  2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
                   ` (2 preceding siblings ...)
  2015-01-29 21:48 ` [PATCH 3/8] KVM: x86: replace 0 with APIC_DEST_PHYSICAL Radim Krčmář
@ 2015-01-29 21:48 ` Radim Krčmář
  2015-01-29 21:48 ` [PATCH 5/8] KVM: x86: use MDA for interrupt matching Radim Krčmář
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-01-29 21:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

We cannot hit the bug now, but future patches will expose this path.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 0ee743c6b4f1..aae043f38548 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -602,7 +602,8 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 	logical_id = kvm_apic_get_reg(apic, APIC_LDR);
 
 	if (apic_x2apic_mode(apic))
-		return logical_id & mda;
+		return ((logical_id >> 16) == (mda >> 16))
+		       && (logical_id & mda & 0xffff);
 
 	logical_id = GET_APIC_LOGICAL_ID(logical_id);
 
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 5/8] KVM: x86: use MDA for interrupt matching
  2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
                   ` (3 preceding siblings ...)
  2015-01-29 21:48 ` [PATCH 4/8] KVM: x86: fix x2apic logical address matching Radim Krčmář
@ 2015-01-29 21:48 ` Radim Krčmář
  2015-01-30  9:03   ` Paolo Bonzini
  2015-01-29 21:48 ` [PATCH 6/8] KVM: x86: allow mixed APIC mode broadcast Radim Krčmář
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 34+ messages in thread
From: Radim Krčmář @ 2015-01-29 21:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

In mixed modes, we musn't deliver xAPIC IPIs like x2APIC and vice versa.
Instead of preserving the information in apic_send_ipi(), we regain it
by converting all destinations into correct APIC MDA in the slow path.
This allows easier reasoning about subsequent matching.

kvm_apic_broadcast() had an interesting design decision:
it didn't consider IOxAPIC 0xff as broadcast in x2APIC mode ...
everything worked because IOxAPIC can't set that in physical mode and
logical mode considered it as a message for first 8 VCPUs.
This patch interprets IOxAPIC 0xff as x2APIC broadcast.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/lapic.c | 40 +++++++++++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index aae043f38548..871ce6a2485b 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -581,15 +581,24 @@ static void apic_set_tpr(struct kvm_lapic *apic, u32 tpr)
 	apic_update_ppr(apic);
 }
 
-static bool kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest)
+static bool kvm_apic_broadcast(struct kvm_lapic *apic, u32 mda)
 {
-	return dest == (apic_x2apic_mode(apic) ?
-			X2APIC_BROADCAST : APIC_BROADCAST);
+	if (apic_x2apic_mode(apic))
+		return mda == X2APIC_BROADCAST;
+
+	/* XXX: verify that xAPIC accepts x2APIC broadcast */
+	return GET_APIC_DEST_FIELD(mda) == APIC_BROADCAST;
 }
 
-static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
+static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 mda)
 {
-	return kvm_apic_id(apic) == dest || kvm_apic_broadcast(apic, dest);
+	if (kvm_apic_broadcast(apic, mda))
+		return true;
+
+	if (apic_x2apic_mode(apic))
+		return mda == kvm_apic_id(apic);
+
+	return mda == SET_APIC_DEST_FIELD(kvm_apic_id(apic));
 }
 
 static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
@@ -606,6 +615,7 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 		       && (logical_id & mda & 0xffff);
 
 	logical_id = GET_APIC_LOGICAL_ID(logical_id);
+	mda = GET_APIC_DEST_FIELD(mda);
 
 	switch (kvm_apic_get_reg(apic, APIC_DFR)) {
 	case APIC_DFR_FLAT:
@@ -620,10 +630,26 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
 	}
 }
 
+/* KVM APIC implementation has two quirks
+ *  - dest always begins at 0 while xAPIC MDA has offset 24,
+ *  - IOxAPIC messages have to be delivered (directly) to x2APIC.
+ */
+static u32
+kvm_apic_mda(unsigned int dest, struct kvm_lapic *ipi, bool x2apic_dest)
+{
+	bool x2apic_mda = ipi ? apic_x2apic_mode(ipi) : x2apic_dest;
+
+	if (!ipi && dest == APIC_BROADCAST)
+		dest = X2APIC_BROADCAST;
+
+	return x2apic_mda ? dest : SET_APIC_DEST_FIELD(dest);
+}
+
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 			   int short_hand, unsigned int dest, int dest_mode)
 {
 	struct kvm_lapic *target = vcpu->arch.apic;
+	u32 mda = kvm_apic_mda(dest, source, apic_x2apic_mode(target));
 
 	apic_debug("target %p, source %p, dest 0x%x, "
 		   "dest_mode 0x%x, short_hand 0x%x\n",
@@ -633,9 +659,9 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 	switch (short_hand) {
 	case APIC_DEST_NOSHORT:
 		if (dest_mode == APIC_DEST_PHYSICAL)
-			return kvm_apic_match_physical_addr(target, dest);
+			return kvm_apic_match_physical_addr(target, mda);
 		else
-			return kvm_apic_match_logical_addr(target, dest);
+			return kvm_apic_match_logical_addr(target, mda);
 	case APIC_DEST_SELF:
 		return target == source;
 	case APIC_DEST_ALLINC:
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 6/8] KVM: x86: allow mixed APIC mode broadcast
  2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
                   ` (4 preceding siblings ...)
  2015-01-29 21:48 ` [PATCH 5/8] KVM: x86: use MDA for interrupt matching Radim Krčmář
@ 2015-01-29 21:48 ` Radim Krčmář
  2015-01-29 21:48 ` [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid Radim Krčmář
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-01-29 21:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

Broadcast allowed only one global APIC mode, but mixed modes are
theoretically possible.  x2APIC IPI doesn't mean 0xff as broadcast,
the rest does.

(We also save one rcu dereference with broadcasts.)

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/lapic.c            | 13 ++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 843bea0e70fd..26d0f0f646d3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -556,7 +556,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, broadcast;
+	u32 cid_shift, cid_mask, lid_mask;
 	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 871ce6a2485b..fab007509047 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -151,7 +151,6 @@ 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;
@@ -163,7 +162,6 @@ static void recalculate_apic_map(struct kvm *kvm)
 			new->ldr_bits = 32;
 			new->cid_shift = 16;
 			new->cid_mask = new->lid_mask = 0xffff;
-			new->broadcast = X2APIC_BROADCAST;
 		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
 			if (kvm_apic_get_reg(apic, APIC_DFR) ==
 							APIC_DFR_CLUSTER) {
@@ -683,6 +681,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 	struct kvm_lapic **dst;
 	int i;
 	bool ret = false;
+	bool x2apic_ipi = src && apic_x2apic_mode(src);
 
 	*r = -1;
 
@@ -694,15 +693,19 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 	if (irq->shorthand)
 		return false;
 
+	/* XXX: A superset of x2APIC broadcasts is fine in practice as long as
+	 * we don't support APIC ID > 0xfeffffff.
+	 */
+	if ((x2apic_ipi ? GET_APIC_DEST_FIELD(irq->dest_id) : irq->dest_id)
+	    == APIC_BROADCAST)
+		return false;
+
 	rcu_read_lock();
 	map = rcu_dereference(kvm->arch.apic_map);
 
 	if (!map)
 		goto out;
 
-	if (irq->dest_id == map->broadcast)
-		goto out;
-
 	ret = true;
 
 	if (irq->dest_mode == APIC_DEST_PHYSICAL) {
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid
  2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
                   ` (5 preceding siblings ...)
  2015-01-29 21:48 ` [PATCH 6/8] KVM: x86: allow mixed APIC mode broadcast Radim Krčmář
@ 2015-01-29 21:48 ` Radim Krčmář
  2015-01-30  9:19   ` Paolo Bonzini
  2015-01-30  9:38   ` Paolo Bonzini
  2015-01-29 21:48 ` [PATCH 8/8] KVM: x86: simplify kvm_apic_map Radim Krčmář
  2015-01-30  9:22 ` [PATCH 0/8] KVM: minor APIC fixes and cleanups Paolo Bonzini
  8 siblings, 2 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-01-29 21:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

We want to support mixed modes and the easiest solution is to avoid
optimizing those weird and unlikely scenarios.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/lapic.c            | 16 ++++++++++++++++
 arch/x86/kvm/lapic.h            |  4 ++++
 3 files changed, 21 insertions(+)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 26d0f0f646d3..fec3188cabbb 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -554,6 +554,7 @@ struct kvm_arch_memory_slot {
 
 struct kvm_apic_map {
 	struct rcu_head rcu;
+	u8 mode;
 	u8 ldr_bits;
 	/* fields bellow are used to decode ldr values in different modes */
 	u32 cid_shift, cid_mask, lid_mask;
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index fab007509047..621d9df6ac63 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -162,16 +162,19 @@ static void recalculate_apic_map(struct kvm *kvm)
 			new->ldr_bits = 32;
 			new->cid_shift = 16;
 			new->cid_mask = new->lid_mask = 0xffff;
+			new->mode |= KVM_APIC_MODE_X2APIC;
 		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
 			if (kvm_apic_get_reg(apic, APIC_DFR) ==
 							APIC_DFR_CLUSTER) {
 				new->cid_shift = 4;
 				new->cid_mask = 0xf;
 				new->lid_mask = 0xf;
+				new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
 			} else {
 				new->cid_shift = 8;
 				new->cid_mask = 0;
 				new->lid_mask = 0xff;
+				new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
 			}
 		}
 
@@ -201,6 +204,13 @@ static void recalculate_apic_map(struct kvm *kvm)
 
 		if (aid < ARRAY_SIZE(new->phys_map))
 			new->phys_map[aid] = apic;
+
+		/* The logical map is definitely wrong if we have multiple
+		 * modes at the same time.  Physical is still right though.
+		 */
+		if (hweight8(new->mode) != 1)
+			continue;
+
 		if (lid && cid < ARRAY_SIZE(new->logical_map))
 			new->logical_map[cid][ffs(lid) - 1] = apic;
 	}
@@ -720,6 +730,12 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 		if (cid >= ARRAY_SIZE(map->logical_map))
 			goto out;
 
+		if (hweight8(map->mode) != 1) {
+			/* Not deliverable with optimized map. */
+			ret = false;
+			goto out;
+		}
+
 		dst = map->logical_map[cid];
 
 		bitmap = apic_logical_id(map, mda);
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index c1ef25c89508..fd0197a93862 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -8,6 +8,10 @@
 #define KVM_APIC_INIT		0
 #define KVM_APIC_SIPI		1
 
+#define KVM_APIC_MODE_XAPIC_FLAT            (1 << 0)
+#define KVM_APIC_MODE_XAPIC_CLUSTER         (1 << 1)
+#define KVM_APIC_MODE_X2APIC                (1 << 2)
+
 struct kvm_timer {
 	struct hrtimer timer;
 	s64 period; 				/* unit: ns */
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH 8/8] KVM: x86: simplify kvm_apic_map
  2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
                   ` (6 preceding siblings ...)
  2015-01-29 21:48 ` [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid Radim Krčmář
@ 2015-01-29 21:48 ` Radim Krčmář
  2015-01-30  9:18   ` Paolo Bonzini
  2015-01-30  9:22 ` [PATCH 0/8] KVM: minor APIC fixes and cleanups Paolo Bonzini
  8 siblings, 1 reply; 34+ messages in thread
From: Radim Krčmář @ 2015-01-29 21:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

recalculate_apic_map() uses two passes over all VCPUs.  This is a relic
from time when we selected a global mode in the first pass and set up
the optimized table in the second pass (to have a consistent mode).

Recent changes made mixed mode unoptimized and we can do it in one pass.
Format of logical MDA is a function of the mode, so we encode it in
apic_logical_id() and drop obsoleted variables from the struct.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  3 --
 arch/x86/kvm/lapic.c            | 78 ++++++++++-------------------------------
 arch/x86/kvm/lapic.h            | 29 ++++++++-------
 3 files changed, 36 insertions(+), 74 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index fec3188cabbb..14b6b0fd17b1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -555,9 +555,6 @@ struct kvm_arch_memory_slot {
 struct kvm_apic_map {
 	struct rcu_head rcu;
 	u8 mode;
-	u8 ldr_bits;
-	/* fields bellow are used to decode ldr values in different modes */
-	u32 cid_shift, cid_mask, lid_mask;
 	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 621d9df6ac63..f74557791a77 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -146,49 +146,6 @@ static void recalculate_apic_map(struct kvm *kvm)
 	if (!new)
 		goto out;
 
-	new->ldr_bits = 8;
-	/* flat mode is default */
-	new->cid_shift = 8;
-	new->cid_mask = 0;
-	new->lid_mask = 0xff;
-
-	kvm_for_each_vcpu(i, vcpu, kvm) {
-		struct kvm_lapic *apic = vcpu->arch.apic;
-
-		if (!kvm_apic_present(vcpu))
-			continue;
-
-		if (apic_x2apic_mode(apic)) {
-			new->ldr_bits = 32;
-			new->cid_shift = 16;
-			new->cid_mask = new->lid_mask = 0xffff;
-			new->mode |= KVM_APIC_MODE_X2APIC;
-		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
-			if (kvm_apic_get_reg(apic, APIC_DFR) ==
-							APIC_DFR_CLUSTER) {
-				new->cid_shift = 4;
-				new->cid_mask = 0xf;
-				new->lid_mask = 0xf;
-				new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
-			} else {
-				new->cid_shift = 8;
-				new->cid_mask = 0;
-				new->lid_mask = 0xff;
-				new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
-			}
-		}
-
-		/*
-		 * All APICs have to be configured in the same mode by an OS.
-		 * We take advatage of this while building logical id loockup
-		 * table. After reset APICs are in software disabled mode, so if
-		 * we find apic with different setting we assume this is the mode
-		 * OS wants all apics to be in; build lookup table accordingly.
-		 */
-		if (kvm_apic_sw_enabled(apic))
-			break;
-	}
-
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		struct kvm_lapic *apic = vcpu->arch.apic;
 		u16 cid, lid;
@@ -198,17 +155,22 @@ static void recalculate_apic_map(struct kvm *kvm)
 			continue;
 
 		aid = kvm_apic_id(apic);
-		ldr = kvm_apic_get_reg(apic, APIC_LDR);
-		cid = apic_cluster_id(new, ldr);
-		lid = apic_logical_id(new, ldr);
-
 		if (aid < ARRAY_SIZE(new->phys_map))
 			new->phys_map[aid] = apic;
 
-		/* The logical map is definitely wrong if we have multiple
-		 * modes at the same time.  Physical is still right though.
-		 */
-		if (hweight8(new->mode) != 1)
+		ldr = kvm_apic_get_reg(apic, APIC_LDR);
+
+		if (apic_x2apic_mode(apic)) {
+			new->mode |= KVM_APIC_MODE_X2APIC;
+		} else if (ldr) {
+			ldr = GET_APIC_LOGICAL_ID(ldr);
+			if (kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_FLAT)
+				new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
+			else
+				new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
+		}
+
+		if (!apic_logical_id(new, ldr, &cid, &lid))
 			continue;
 
 		if (lid && cid < ARRAY_SIZE(new->logical_map))
@@ -724,22 +686,20 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 
 		dst = &map->phys_map[irq->dest_id];
 	} else {
-		u32 mda = irq->dest_id << (32 - map->ldr_bits);
-		u16 cid = apic_cluster_id(map, mda);
+		u16 cid;
 
-		if (cid >= ARRAY_SIZE(map->logical_map))
-			goto out;
-
-		if (hweight8(map->mode) != 1) {
+		if (!apic_logical_id(map, irq->dest_id, &cid, (u16 *)&bitmap))
+		{
 			/* Not deliverable with optimized map. */
 			ret = false;
 			goto out;
 		}
 
+		if (cid >= ARRAY_SIZE(map->logical_map))
+			goto out;
+
 		dst = map->logical_map[cid];
 
-		bitmap = apic_logical_id(map, mda);
-
 		if (irq->delivery_mode == APIC_DM_LOWEST) {
 			int l = -1;
 			for_each_set_bit(i, &bitmap, 16) {
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index fd0197a93862..320716e301a8 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -151,19 +151,24 @@ static inline bool kvm_apic_vid_enabled(struct kvm *kvm)
 	return kvm_x86_ops->vm_has_apicv(kvm);
 }
 
-static inline u16 apic_cluster_id(struct kvm_apic_map *map, u32 ldr)
+static inline bool
+apic_logical_id(struct kvm_apic_map *map, u32 ldr, u16 *cid, u16 *lid)
 {
-	u16 cid;
-	ldr >>= 32 - map->ldr_bits;
-	cid = (ldr >> map->cid_shift) & map->cid_mask;
-
-	return cid;
-}
-
-static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr)
-{
-	ldr >>= (32 - map->ldr_bits);
-	return ldr & map->lid_mask;
+	switch (map->mode) {
+	case KVM_APIC_MODE_XAPIC_FLAT:
+		*cid = 0;
+		*lid = ldr & 0xff;
+		return true;
+	case KVM_APIC_MODE_XAPIC_CLUSTER:
+		*cid = (ldr >> 4) & 0xf;
+		*lid = ldr & 0xf;
+		return true;
+	case KVM_APIC_MODE_X2APIC:
+		*cid = ldr >> 16;
+		*lid = ldr & 0xffff;
+		return true;
+	}
+	return false;
 }
 
 static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*()
  2015-01-29 21:48 ` [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*() Radim Krčmář
@ 2015-01-29 22:10   ` Joe Perches
  2015-01-30  8:50     ` Paolo Bonzini
  0 siblings, 1 reply; 34+ messages in thread
From: Joe Perches @ 2015-01-29 22:10 UTC (permalink / raw)
  To: Radim Krčmář
  Cc: linux-kernel, kvm, Paolo Bonzini, Nadav Amit, Gleb Natapov

On Thu, 2015-01-29 at 22:48 +0100, Radim Krčmář wrote:
> And don't export the internal ones while at it.
[]
> -int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
> +static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>  {
>  	int result = 0;
>  	u32 logical_id;
> @@ -626,7 +626,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>  	return result;
>  }
>  
> -int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
> +bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  			   int short_hand, unsigned int dest, int dest_mode)
>  {
>  	int result = 0;

Perhaps these result variables should be bool.



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*()
  2015-01-29 22:10   ` Joe Perches
@ 2015-01-30  8:50     ` Paolo Bonzini
  0 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30  8:50 UTC (permalink / raw)
  To: Joe Perches, Radim Krčmář
  Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov



On 29/01/2015 23:10, Joe Perches wrote:
> On Thu, 2015-01-29 at 22:48 +0100, Radim Krčmář wrote:
>> And don't export the internal ones while at it.
> []
>> -int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>> +static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>>  {
>>  	int result = 0;
>>  	u32 logical_id;
>> @@ -626,7 +626,7 @@ int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>>  	return result;
>>  }
>>  
>> -int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>> +bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>>  			   int short_hand, unsigned int dest, int dest_mode)
>>  {
>>  	int result = 0;
> 
> Perhaps these result variables should be bool.

See patch 2.

Paolo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()
  2015-01-29 21:48 ` [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*() Radim Krčmář
@ 2015-01-30  8:52   ` Paolo Bonzini
  2015-01-30 13:06     ` Radim Krčmář
  2015-02-02 14:26     ` Radim Krčmář
  0 siblings, 2 replies; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30  8:52 UTC (permalink / raw)
  To: Radim Krčmář, linux-kernel; +Cc: kvm, Nadav Amit, Gleb Natapov



On 29/01/2015 22:48, Radim Krčmář wrote:
> The majority of this patch turns
>   result = 0; if (CODE) result = 1; return result;
> into
>   return CODE;
> because we return bool now.

Added a bunch of "!= 0" to avoid automagic conversion to bool.

Paolo

> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/lapic.c | 44 +++++++++++++++-----------------------------
>  1 file changed, 15 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 478dd8bd653b..271e7d076879 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -594,42 +594,34 @@ static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
>  
>  static bool 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;
> +		return true;
>  
> -	if (apic_x2apic_mode(apic)) {
> -		logical_id = kvm_apic_get_reg(apic, APIC_LDR);
> +	logical_id = kvm_apic_get_reg(apic, APIC_LDR);
> +
> +	if (apic_x2apic_mode(apic))
>  		return logical_id & mda;
> -	}
>  
> -	logical_id = GET_APIC_LOGICAL_ID(kvm_apic_get_reg(apic, APIC_LDR));
> +	logical_id = GET_APIC_LOGICAL_ID(logical_id);
>  
>  	switch (kvm_apic_get_reg(apic, APIC_DFR)) {
>  	case APIC_DFR_FLAT:
> -		if (logical_id & mda)
> -			result = 1;
> -		break;
> +		return logical_id & mda;
>  	case APIC_DFR_CLUSTER:
> -		if (((logical_id >> 4) == (mda >> 0x4))
> -		    && (logical_id & mda & 0xf))
> -			result = 1;
> -		break;
> +		return ((logical_id >> 4) == (mda >> 4))
> +		       && (logical_id & mda & 0xf);
>  	default:
>  		apic_debug("Bad DFR vcpu %d: %08x\n",
>  			   apic->vcpu->vcpu_id, kvm_apic_get_reg(apic, APIC_DFR));
> -		break;
> +		return false;
>  	}
> -
> -	return result;
>  }
>  
>  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  			   int short_hand, unsigned int dest, int dest_mode)
>  {
> -	int result = 0;
>  	struct kvm_lapic *target = vcpu->arch.apic;
>  
>  	apic_debug("target %p, source %p, dest 0x%x, "
> @@ -641,27 +633,21 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  	case APIC_DEST_NOSHORT:
>  		if (dest_mode == 0)
>  			/* Physical mode. */
> -			result = kvm_apic_match_physical_addr(target, dest);
> +			return kvm_apic_match_physical_addr(target, dest);
>  		else
>  			/* Logical mode. */
> -			result = kvm_apic_match_logical_addr(target, dest);
> -		break;
> +			return kvm_apic_match_logical_addr(target, dest);
>  	case APIC_DEST_SELF:
> -		result = (target == source);
> -		break;
> +		return target == source;
>  	case APIC_DEST_ALLINC:
> -		result = 1;
> -		break;
> +		return true;
>  	case APIC_DEST_ALLBUT:
> -		result = (target != source);
> -		break;
> +		return target != source;
>  	default:
>  		apic_debug("kvm: apic: Bad dest shorthand value %x\n",
>  			   short_hand);
> -		break;
> +		return false;
>  	}
> -
> -	return result;
>  }
>  
>  bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 5/8] KVM: x86: use MDA for interrupt matching
  2015-01-29 21:48 ` [PATCH 5/8] KVM: x86: use MDA for interrupt matching Radim Krčmář
@ 2015-01-30  9:03   ` Paolo Bonzini
  2015-01-30 13:09     ` Radim Krčmář
  0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30  9:03 UTC (permalink / raw)
  To: Radim Krčmář, linux-kernel; +Cc: kvm, Nadav Amit, Gleb Natapov



On 29/01/2015 22:48, Radim Krčmář wrote:
> In mixed modes, we musn't deliver xAPIC IPIs like x2APIC and vice versa.
> Instead of preserving the information in apic_send_ipi(), we regain it
> by converting all destinations into correct APIC MDA in the slow path.
> This allows easier reasoning about subsequent matching.
> 
> kvm_apic_broadcast() had an interesting design decision:
> it didn't consider IOxAPIC 0xff as broadcast in x2APIC mode ...
> everything worked because IOxAPIC can't set that in physical mode and
> logical mode considered it as a message for first 8 VCPUs.
> This patch interprets IOxAPIC 0xff as x2APIC broadcast.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/lapic.c | 40 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index aae043f38548..871ce6a2485b 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -581,15 +581,24 @@ static void apic_set_tpr(struct kvm_lapic *apic, u32 tpr)
>  	apic_update_ppr(apic);
>  }
>  
> -static bool kvm_apic_broadcast(struct kvm_lapic *apic, u32 dest)
> +static bool kvm_apic_broadcast(struct kvm_lapic *apic, u32 mda)
>  {
> -	return dest == (apic_x2apic_mode(apic) ?
> -			X2APIC_BROADCAST : APIC_BROADCAST);
> +	if (apic_x2apic_mode(apic))
> +		return mda == X2APIC_BROADCAST;
> +
> +	/* XXX: verify that xAPIC accepts x2APIC broadcast */
> +	return GET_APIC_DEST_FIELD(mda) == APIC_BROADCAST;
>  }
>  
> -static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 dest)
> +static bool kvm_apic_match_physical_addr(struct kvm_lapic *apic, u32 mda)
>  {
> -	return kvm_apic_id(apic) == dest || kvm_apic_broadcast(apic, dest);
> +	if (kvm_apic_broadcast(apic, mda))
> +		return true;
> +
> +	if (apic_x2apic_mode(apic))
> +		return mda == kvm_apic_id(apic);
> +
> +	return mda == SET_APIC_DEST_FIELD(kvm_apic_id(apic));
>  }
>  
>  static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
> @@ -606,6 +615,7 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>  		       && (logical_id & mda & 0xffff);
>  
>  	logical_id = GET_APIC_LOGICAL_ID(logical_id);
> +	mda = GET_APIC_DEST_FIELD(mda);
>  
>  	switch (kvm_apic_get_reg(apic, APIC_DFR)) {
>  	case APIC_DFR_FLAT:
> @@ -620,10 +630,26 @@ static bool kvm_apic_match_logical_addr(struct kvm_lapic *apic, u32 mda)
>  	}
>  }
>  
> +/* KVM APIC implementation has two quirks
> + *  - dest always begins at 0 while xAPIC MDA has offset 24,
> + *  - IOxAPIC messages have to be delivered (directly) to x2APIC.
> + */
> +static u32
> +kvm_apic_mda(unsigned int dest, struct kvm_lapic *ipi, bool x2apic_dest)

Please pass two struct kvm_lapic, so that you can write

	bool ipi = source != NULL;
	bool x2apic_mda = apic_x2apic_mode(ipi ? source : dest);

Looks a little nicer to me at least.

> +{
> +	bool x2apic_mda = ipi ? apic_x2apic_mode(ipi) : x2apic_dest;
> +
> +	if (!ipi && dest == APIC_BROADCAST)
> +		dest = X2APIC_BROADCAST;

This works, but it is not super-clear that you are shifting left by 24
here, and right in kvm_apic_broadcast().  What if you just make it

	if (!ipi && dest == APIC_BROADCAST && x2apic_mda)
		return X2APIC_BROADCAST.

?

Paolo

> +
> +	return x2apic_mda ? dest : SET_APIC_DEST_FIELD(dest);
> +}
> +
>  bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  			   int short_hand, unsigned int dest, int dest_mode)
>  {
>  	struct kvm_lapic *target = vcpu->arch.apic;
> +	u32 mda = kvm_apic_mda(dest, source, apic_x2apic_mode(target));
>  
>  	apic_debug("target %p, source %p, dest 0x%x, "
>  		   "dest_mode 0x%x, short_hand 0x%x\n",
> @@ -633,9 +659,9 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  	switch (short_hand) {
>  	case APIC_DEST_NOSHORT:
>  		if (dest_mode == APIC_DEST_PHYSICAL)
> -			return kvm_apic_match_physical_addr(target, dest);
> +			return kvm_apic_match_physical_addr(target, mda);
>  		else
> -			return kvm_apic_match_logical_addr(target, dest);
> +			return kvm_apic_match_logical_addr(target, mda);
>  	case APIC_DEST_SELF:
>  		return target == source;
>  	case APIC_DEST_ALLINC:
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map
  2015-01-29 21:48 ` [PATCH 8/8] KVM: x86: simplify kvm_apic_map Radim Krčmář
@ 2015-01-30  9:18   ` Paolo Bonzini
  2015-01-30 15:14     ` Radim Krčmář
  0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30  9:18 UTC (permalink / raw)
  To: Radim Krčmář, linux-kernel; +Cc: kvm, Nadav Amit, Gleb Natapov



On 29/01/2015 22:48, Radim Krčmář wrote:
> recalculate_apic_map() uses two passes over all VCPUs.  This is a relic
> from time when we selected a global mode in the first pass and set up
> the optimized table in the second pass (to have a consistent mode).
> 
> Recent changes made mixed mode unoptimized and we can do it in one pass.
> Format of logical MDA is a function of the mode, so we encode it in
> apic_logical_id() and drop obsoleted variables from the struct.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  3 --
>  arch/x86/kvm/lapic.c            | 78 ++++++++++-------------------------------
>  arch/x86/kvm/lapic.h            | 29 ++++++++-------
>  3 files changed, 36 insertions(+), 74 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index fec3188cabbb..14b6b0fd17b1 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -555,9 +555,6 @@ struct kvm_arch_memory_slot {
>  struct kvm_apic_map {
>  	struct rcu_head rcu;
>  	u8 mode;
> -	u8 ldr_bits;
> -	/* fields bellow are used to decode ldr values in different modes */
> -	u32 cid_shift, cid_mask, lid_mask;
>  	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 621d9df6ac63..f74557791a77 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -146,49 +146,6 @@ static void recalculate_apic_map(struct kvm *kvm)
>  	if (!new)
>  		goto out;
>  
> -	new->ldr_bits = 8;
> -	/* flat mode is default */
> -	new->cid_shift = 8;
> -	new->cid_mask = 0;
> -	new->lid_mask = 0xff;
> -
> -	kvm_for_each_vcpu(i, vcpu, kvm) {
> -		struct kvm_lapic *apic = vcpu->arch.apic;
> -
> -		if (!kvm_apic_present(vcpu))
> -			continue;
> -
> -		if (apic_x2apic_mode(apic)) {
> -			new->ldr_bits = 32;
> -			new->cid_shift = 16;
> -			new->cid_mask = new->lid_mask = 0xffff;
> -			new->mode |= KVM_APIC_MODE_X2APIC;
> -		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
> -			if (kvm_apic_get_reg(apic, APIC_DFR) ==
> -							APIC_DFR_CLUSTER) {
> -				new->cid_shift = 4;
> -				new->cid_mask = 0xf;
> -				new->lid_mask = 0xf;
> -				new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
> -			} else {
> -				new->cid_shift = 8;
> -				new->cid_mask = 0;
> -				new->lid_mask = 0xff;
> -				new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
> -			}
> -		}
> -
> -		/*
> -		 * All APICs have to be configured in the same mode by an OS.
> -		 * We take advatage of this while building logical id loockup
> -		 * table. After reset APICs are in software disabled mode, so if
> -		 * we find apic with different setting we assume this is the mode
> -		 * OS wants all apics to be in; build lookup table accordingly.
> -		 */
> -		if (kvm_apic_sw_enabled(apic))
> -			break;
> -	}
> -
>  	kvm_for_each_vcpu(i, vcpu, kvm) {
>  		struct kvm_lapic *apic = vcpu->arch.apic;
>  		u16 cid, lid;
> @@ -198,17 +155,22 @@ static void recalculate_apic_map(struct kvm *kvm)
>  			continue;
>  
>  		aid = kvm_apic_id(apic);
> -		ldr = kvm_apic_get_reg(apic, APIC_LDR);
> -		cid = apic_cluster_id(new, ldr);
> -		lid = apic_logical_id(new, ldr);
> -
>  		if (aid < ARRAY_SIZE(new->phys_map))
>  			new->phys_map[aid] = apic;
>  
> -		/* The logical map is definitely wrong if we have multiple
> -		 * modes at the same time.  Physical is still right though.
> -		 */
> -		if (hweight8(new->mode) != 1)
> +		ldr = kvm_apic_get_reg(apic, APIC_LDR);
> +
> +		if (apic_x2apic_mode(apic)) {
> +			new->mode |= KVM_APIC_MODE_X2APIC;
> +		} else if (ldr) {
> +			ldr = GET_APIC_LOGICAL_ID(ldr);
> +			if (kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_FLAT)
> +				new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
> +			else
> +				new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
> +		}
> +
> +		if (!apic_logical_id(new, ldr, &cid, &lid))
>  			continue;
>  
>  		if (lid && cid < ARRAY_SIZE(new->logical_map))
> @@ -724,22 +686,20 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  
>  		dst = &map->phys_map[irq->dest_id];
>  	} else {
> -		u32 mda = irq->dest_id << (32 - map->ldr_bits);
> -		u16 cid = apic_cluster_id(map, mda);
> +		u16 cid;
>  
> -		if (cid >= ARRAY_SIZE(map->logical_map))
> -			goto out;
> -
> -		if (hweight8(map->mode) != 1) {
> +		if (!apic_logical_id(map, irq->dest_id, &cid, (u16 *)&bitmap))
> +		{
>  			/* Not deliverable with optimized map. */
>  			ret = false;
>  			goto out;
>  		}
>  
> +		if (cid >= ARRAY_SIZE(map->logical_map))
> +			goto out;
> +
>  		dst = map->logical_map[cid];
>  
> -		bitmap = apic_logical_id(map, mda);
> -
>  		if (irq->delivery_mode == APIC_DM_LOWEST) {
>  			int l = -1;
>  			for_each_set_bit(i, &bitmap, 16) {
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index fd0197a93862..320716e301a8 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -151,19 +151,24 @@ static inline bool kvm_apic_vid_enabled(struct kvm *kvm)
>  	return kvm_x86_ops->vm_has_apicv(kvm);
>  }
>  
> -static inline u16 apic_cluster_id(struct kvm_apic_map *map, u32 ldr)
> +static inline bool
> +apic_logical_id(struct kvm_apic_map *map, u32 ldr, u16 *cid, u16 *lid)
>  {
> -	u16 cid;
> -	ldr >>= 32 - map->ldr_bits;
> -	cid = (ldr >> map->cid_shift) & map->cid_mask;
> -
> -	return cid;
> -}
> -
> -static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr)
> -{
> -	ldr >>= (32 - map->ldr_bits);
> -	return ldr & map->lid_mask;
> +	switch (map->mode) {
> +	case KVM_APIC_MODE_XAPIC_FLAT:
> +		*cid = 0;
> +		*lid = ldr & 0xff;
> +		return true;
> +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> +		*cid = (ldr >> 4) & 0xf;
> +		*lid = ldr & 0xf;
> +		return true;
> +	case KVM_APIC_MODE_X2APIC:
> +		*cid = ldr >> 16;
> +		*lid = ldr & 0xffff;
> +		return true;
> +	}

We need some optimization here.  You can make the defines equal to the
size of the lid: CLUSTER = 1 << 3, FLAT = 1 << 2, X2APIC = 1 << 4:

	BUILD_BUG_ON(...FLAT != 4);
	BUILD_BUG_ON(...CLUSTER != 8);
	BUILD_BUG_ON(...X2APIC != 16);

	lid_bits = mode;
	cid_bits = mode & (16 | 4);
	lid_mask = (1 << lid_bits) - 1;
	cid_mask = (1 << cid_bits) - 1;

	*cid = (ldr >> lid_bits) & cid_mask;
	*lid = ldr & lid_mask;

Please move this to lapic.c since you are at it.

Paolo

> +	return false;
>  }
>  
>  static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid
  2015-01-29 21:48 ` [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid Radim Krčmář
@ 2015-01-30  9:19   ` Paolo Bonzini
  2015-01-30 14:21     ` Radim Krčmář
  2015-01-30  9:38   ` Paolo Bonzini
  1 sibling, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30  9:19 UTC (permalink / raw)
  To: Radim Krčmář, linux-kernel; +Cc: kvm, Nadav Amit, Gleb Natapov



On 29/01/2015 22:48, Radim Krčmář wrote:
> We want to support mixed modes and the easiest solution is to avoid
> optimizing those weird and unlikely scenarios.

Hmm, what happens if use maxcpus to disable some CPUs at startup?  Could
the disabled CPUs remain in xapic mode while the others move to x2apic?

Paolo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/8] KVM: minor APIC fixes and cleanups
  2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
                   ` (7 preceding siblings ...)
  2015-01-29 21:48 ` [PATCH 8/8] KVM: x86: simplify kvm_apic_map Radim Krčmář
@ 2015-01-30  9:22 ` Paolo Bonzini
  2015-01-30 15:20   ` Radim Krčmář
  8 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30  9:22 UTC (permalink / raw)
  To: Radim Krčmář, linux-kernel; +Cc: kvm, Nadav Amit, Gleb Natapov



On 29/01/2015 22:48, Radim Krčmář wrote:
> This patch series is made our of three logical parts,
> 
> [1-3/8] are just a cleanup and could be omitted
> [4-6/8] improve broadcast detection and unoptimized delivery
> [7-8/8] handle mixed mode (by falling back to improvements from [5-6/9])

I applied patches 1-4 and commented on patch 5.

I'm afraid that patches 7-8 could lead to missed optimizations in some cases.

Regarding patch 6, perhaps there's a way to avoid this:

+	/* XXX: A superset of x2APIC broadcasts is fine in practice as long as
+	 * we don't support APIC ID > 0xfeffffff.
+	 */

It's ugly. :)

Paolo

> 
> Radim Krčmář (8):
>   KVM: x86: return bool from kvm_apic_match*()
>   KVM: x86: cleanup kvm_apic_match_*()
>   KVM: x86: replace 0 with APIC_DEST_PHYSICAL
>   KVM: x86: fix x2apic logical address matching
>   KVM: x86: use MDA for interrupt matching
>   KVM: x86: allow mixed APIC mode broadcast
>   KVM: x86: avoid logical_map when it is invalid
>   KVM: x86: simplify kvm_apic_map
> 
>  arch/x86/include/asm/kvm_host.h |   4 +-
>  arch/x86/kvm/ioapic.h           |   2 +-
>  arch/x86/kvm/lapic.c            | 176 +++++++++++++++++++---------------------
>  arch/x86/kvm/lapic.h            |  35 ++++----
>  4 files changed, 106 insertions(+), 111 deletions(-)
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid
  2015-01-29 21:48 ` [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid Radim Krčmář
  2015-01-30  9:19   ` Paolo Bonzini
@ 2015-01-30  9:38   ` Paolo Bonzini
  2015-01-30 14:56     ` Radim Krčmář
  1 sibling, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30  9:38 UTC (permalink / raw)
  To: Radim Krčmář, linux-kernel; +Cc: kvm, Nadav Amit, Gleb Natapov



On 29/01/2015 22:48, Radim Krčmář wrote:
> We want to support mixed modes and the easiest solution is to avoid
> optimizing those weird and unlikely scenarios.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/lapic.c            | 16 ++++++++++++++++
>  arch/x86/kvm/lapic.h            |  4 ++++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 26d0f0f646d3..fec3188cabbb 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -554,6 +554,7 @@ struct kvm_arch_memory_slot {
>  
>  struct kvm_apic_map {
>  	struct rcu_head rcu;
> +	u8 mode;
>  	u8 ldr_bits;
>  	/* fields bellow are used to decode ldr values in different modes */
>  	u32 cid_shift, cid_mask, lid_mask;
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index fab007509047..621d9df6ac63 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -162,16 +162,19 @@ static void recalculate_apic_map(struct kvm *kvm)
>  			new->ldr_bits = 32;
>  			new->cid_shift = 16;
>  			new->cid_mask = new->lid_mask = 0xffff;
> +			new->mode |= KVM_APIC_MODE_X2APIC;
>  		} else if (kvm_apic_get_reg(apic, APIC_LDR)) {
>  			if (kvm_apic_get_reg(apic, APIC_DFR) ==
>  							APIC_DFR_CLUSTER) {
>  				new->cid_shift = 4;
>  				new->cid_mask = 0xf;
>  				new->lid_mask = 0xf;
> +				new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER;
>  			} else {
>  				new->cid_shift = 8;
>  				new->cid_mask = 0;
>  				new->lid_mask = 0xff;
> +				new->mode |= KVM_APIC_MODE_XAPIC_FLAT;
>  			}
>  		}
>  
> @@ -201,6 +204,13 @@ static void recalculate_apic_map(struct kvm *kvm)
>  
>  		if (aid < ARRAY_SIZE(new->phys_map))
>  			new->phys_map[aid] = apic;
> +
> +		/* The logical map is definitely wrong if we have multiple
> +		 * modes at the same time.  Physical is still right though.
> +		 */
> +		if (hweight8(new->mode) != 1)

Better (more optimized):

	if (new->mode & (new->mode - 1))

Please add a comment to kvm_irq_delivery_to_apic_fast to explain what
you are doing.

> +			continue;
> +
>  		if (lid && cid < ARRAY_SIZE(new->logical_map))
>  			new->logical_map[cid][ffs(lid) - 1] = apic;
>  	}
> @@ -720,6 +730,12 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  		if (cid >= ARRAY_SIZE(map->logical_map))
>  			goto out;
>  
> +		if (hweight8(map->mode) != 1) {
> +			/* Not deliverable with optimized map. */
> +			ret = false;
> +			goto out;
> +		}

Put this before the computation of cid and mda.  The cid and mda are all
wrong with a mixed map, and the result of the "if" before is influenced
by the wrong cid.  Fixed by patch 8, but better get it right here.

Paolo

>  		dst = map->logical_map[cid];
>  
>  		bitmap = apic_logical_id(map, mda);
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index c1ef25c89508..fd0197a93862 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -8,6 +8,10 @@
>  #define KVM_APIC_INIT		0
>  #define KVM_APIC_SIPI		1
>  
> +#define KVM_APIC_MODE_XAPIC_FLAT            (1 << 0)
> +#define KVM_APIC_MODE_XAPIC_CLUSTER         (1 << 1)
> +#define KVM_APIC_MODE_X2APIC                (1 << 2)
> +
>  struct kvm_timer {
>  	struct hrtimer timer;
>  	s64 period; 				/* unit: ns */
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()
  2015-01-30  8:52   ` Paolo Bonzini
@ 2015-01-30 13:06     ` Radim Krčmář
  2015-02-02 14:26     ` Radim Krčmář
  1 sibling, 0 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-01-30 13:06 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-01-30 09:52+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > The majority of this patch turns
> >   result = 0; if (CODE) result = 1; return result;
> > into
> >   return CODE;
> > because we return bool now.
> 
> Added a bunch of "!= 0" to avoid automagic conversion to bool.

Makes sense, (valid code isn't automatically desirable)
thank you.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 5/8] KVM: x86: use MDA for interrupt matching
  2015-01-30  9:03   ` Paolo Bonzini
@ 2015-01-30 13:09     ` Radim Krčmář
  0 siblings, 0 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-01-30 13:09 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-01-30 10:03+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > +/* KVM APIC implementation has two quirks
> > + *  - dest always begins at 0 while xAPIC MDA has offset 24,
> > + *  - IOxAPIC messages have to be delivered (directly) to x2APIC.
> > + */
> > +static u32
> > +kvm_apic_mda(unsigned int dest, struct kvm_lapic *ipi, bool x2apic_dest)
> 
> Please pass two struct kvm_lapic, so that you can write
> 
> 	bool ipi = source != NULL;
> 	bool x2apic_mda = apic_x2apic_mode(ipi ? source : dest);
> 
> Looks a little nicer to me at least.

Definitely.

> > +{
> > +	bool x2apic_mda = ipi ? apic_x2apic_mode(ipi) : x2apic_dest;
> > +
> > +	if (!ipi && dest == APIC_BROADCAST)
> > +		dest = X2APIC_BROADCAST;
> 
> This works, but it is not super-clear that you are shifting left by 24
> here, and right in kvm_apic_broadcast().  What if you just make it
> 
> 	if (!ipi && dest == APIC_BROADCAST && x2apic_mda)
> 		return X2APIC_BROADCAST.

It's better, it will be that way in v2 of [5-8/8], thanks.

(I was using it twice in earlier iterations and didn't think again.)

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid
  2015-01-30  9:19   ` Paolo Bonzini
@ 2015-01-30 14:21     ` Radim Krčmář
  2015-01-30 14:21       ` Paolo Bonzini
  0 siblings, 1 reply; 34+ messages in thread
From: Radim Krčmář @ 2015-01-30 14:21 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-01-30 10:19+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > We want to support mixed modes and the easiest solution is to avoid
> > optimizing those weird and unlikely scenarios.
> 
> Hmm, what happens if use maxcpus to disable some CPUs at startup?

Their APICs are kept sw-disabled in xAPIC mode.

>                                                                    Could
> the disabled CPUs remain in xapic mode while the others move to x2apic?

Yes, but it would only matter if the OS set LDR on them.
I mark x2APIC with perfectly configured xAPIC on unused processors as
"weird" ... (Linux 3.18 and 2.6.32 doesn't do it.)

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid
  2015-01-30 14:21     ` Radim Krčmář
@ 2015-01-30 14:21       ` Paolo Bonzini
  0 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30 14:21 UTC (permalink / raw)
  To: Radim Krčmář; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov



On 30/01/2015 15:21, Radim Krčmář wrote:
> Yes, but it would only matter if the OS set LDR on them.
> I mark x2APIC with perfectly configured xAPIC on unused processors as
> "weird" ... (Linux 3.18 and 2.6.32 doesn't do it.)

Ok.

Paolo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid
  2015-01-30  9:38   ` Paolo Bonzini
@ 2015-01-30 14:56     ` Radim Krčmář
  2015-01-30 15:10       ` Paolo Bonzini
  0 siblings, 1 reply; 34+ messages in thread
From: Radim Krčmář @ 2015-01-30 14:56 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-01-30 10:38+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > +		if (hweight8(new->mode) != 1)
> 
> Better (more optimized):
> 
> 	if (new->mode & (new->mode - 1))

True, hweight needs to have X86_FEATURE_POPCNT to be efficient ...

Do you know of a difference with it?
  new->mode & (new->mode - 1)   |  hweight8(new->mode) != 1
    lea    -0x1(%rax),%edi      |    popcnt %edi,%eax
    test   %eax,%edi            |    cmp    $1,%eax

> Please add a comment to kvm_irq_delivery_to_apic_fast to explain what
> you are doing.

Would naming it kvm_apic_need_slow_delivery(), or something, be enough?

> > +		if (hweight8(map->mode) != 1) {
> > +			/* Not deliverable with optimized map. */
> > +			ret = false;
> > +			goto out;
> > +		}
> 
> Put this before the computation of cid and mda.  The cid and mda are all
> wrong with a mixed map, and the result of the "if" before is influenced
> by the wrong cid.  Fixed by patch 8, but better get it right here.

Will do,

thanks.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid
  2015-01-30 14:56     ` Radim Krčmář
@ 2015-01-30 15:10       ` Paolo Bonzini
  2015-01-30 17:09         ` Radim Krčmář
  0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30 15:10 UTC (permalink / raw)
  To: Radim Krčmář; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov



On 30/01/2015 15:56, Radim Krčmář wrote:
> 2015-01-30 10:38+0100, Paolo Bonzini:
>> On 29/01/2015 22:48, Radim Krčmář wrote:
>>> +		if (hweight8(new->mode) != 1)
>>
>> Better (more optimized):
>>
>> 	if (new->mode & (new->mode - 1))
> 
> True, hweight needs to have X86_FEATURE_POPCNT to be efficient ...
> 
> Do you know of a difference with it?
>   new->mode & (new->mode - 1)   |  hweight8(new->mode) != 1
>     lea    -0x1(%rax),%edi      |    popcnt %edi,%eax
>     test   %eax,%edi            |    cmp    $1,%eax

x & (x - 1) is really hweight8(new->mode) > 1.  So if new->mode == 0 it
would have a different result.

>> Please add a comment to kvm_irq_delivery_to_apic_fast to explain what
>> you are doing.
> 
> Would naming it kvm_apic_need_slow_delivery(), or something, be enough?

Or kvm_apic_map_valid() perhaps?

Paolo

>>> +		if (hweight8(map->mode) != 1) {
>>> +			/* Not deliverable with optimized map. */
>>> +			ret = false;
>>> +			goto out;
>>> +		}
>>
>> Put this before the computation of cid and mda.  The cid and mda are all
>> wrong with a mixed map, and the result of the "if" before is influenced
>> by the wrong cid.  Fixed by patch 8, but better get it right here.
> 
> Will do,
> 
> thanks.
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map
  2015-01-30  9:18   ` Paolo Bonzini
@ 2015-01-30 15:14     ` Radim Krčmář
  2015-01-30 15:23       ` Paolo Bonzini
  0 siblings, 1 reply; 34+ messages in thread
From: Radim Krčmář @ 2015-01-30 15:14 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-01-30 10:18+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > +static inline bool
> > +apic_logical_id(struct kvm_apic_map *map, u32 ldr, u16 *cid, u16 *lid)
> >  {
> > +	switch (map->mode) {
> > +	case KVM_APIC_MODE_XAPIC_FLAT:
> > +		*cid = 0;
> > +		*lid = ldr & 0xff;
> > +		return true;
> > +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> > +		*cid = (ldr >> 4) & 0xf;
> > +		*lid = ldr & 0xf;
> > +		return true;
> > +	case KVM_APIC_MODE_X2APIC:
> > +		*cid = ldr >> 16;
> > +		*lid = ldr & 0xffff;
> > +		return true;
> > +	}
> 
> We need some optimization here.  You can make the defines equal to the
> size of the lid: CLUSTER = 1 << 3, FLAT = 1 << 2, X2APIC = 1 << 4:
> 
> 	BUILD_BUG_ON(...FLAT != 4);
> 	BUILD_BUG_ON(...CLUSTER != 8);

(Swapped.)

> 	BUILD_BUG_ON(...X2APIC != 16);

(Check the mode and return false here.)

> 	lid_bits = mode;
> 	cid_bits = mode & (16 | 4);
> 	lid_mask = (1 << lid_bits) - 1;
> 	cid_mask = (1 << cid_bits) - 1;
> 
> 	*cid = (ldr >> lid_bits) & cid_mask;
> 	*lid = ldr & lid_mask;

Would jump predictor fail on the switch?  Or is size of the code that
important?  This code is shorter, but is going to execute far more
operations, so I think it would be slower ... (And harder to read.)

> Please move this to lapic.c since you are at it.

Ok.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/8] KVM: minor APIC fixes and cleanups
  2015-01-30  9:22 ` [PATCH 0/8] KVM: minor APIC fixes and cleanups Paolo Bonzini
@ 2015-01-30 15:20   ` Radim Krčmář
  2015-01-30 15:24     ` Paolo Bonzini
  0 siblings, 1 reply; 34+ messages in thread
From: Radim Krčmář @ 2015-01-30 15:20 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-01-30 10:22+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > This patch series is made our of three logical parts,
> > 
> > [1-3/8] are just a cleanup and could be omitted
> > [4-6/8] improve broadcast detection and unoptimized delivery
> > [7-8/8] handle mixed mode (by falling back to improvements from [5-6/9])
> 
> I applied patches 1-4 and commented on patch 5.
> 
> I'm afraid that patches 7-8 could lead to missed optimizations in some cases.

True, I'll do more research on complexity of its assembly.

> Regarding patch 6, perhaps there's a way to avoid this:
> 
> +	/* XXX: A superset of x2APIC broadcasts is fine in practice as long as
> +	 * we don't support APIC ID > 0xfeffffff.
> +	 */
> 
> It's ugly. :)

Yeah, there is: don't deliver x2APIC broadcasts to xAPIC.
(I'm not even sure if it is correct ...)

Without that delivery, we could do something like
  if (irq->dest_id == (x2apic_ipi ? X2APIC_BROADCAST : APIC_BROADCAST))

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map
  2015-01-30 15:14     ` Radim Krčmář
@ 2015-01-30 15:23       ` Paolo Bonzini
  2015-01-30 16:57         ` Radim Krčmář
  0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30 15:23 UTC (permalink / raw)
  To: Radim Krčmář; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov



On 30/01/2015 16:14, Radim Krčmář wrote:
> > > +	case KVM_APIC_MODE_XAPIC_FLAT:
> > > +		*cid = 0;
> > > +		*lid = ldr & 0xff;
> > > +		return true;
> > > +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> > > +		*cid = (ldr >> 4) & 0xf;
> > > +		*lid = ldr & 0xf;
> > > +		return true;
> > > +	case KVM_APIC_MODE_X2APIC:
> > > +		*cid = ldr >> 16;
> > > +		*lid = ldr & 0xffff;
> > > +		return true;
> > > +	}
> 
>> > 	lid_bits = mode;
>> > 	cid_bits = mode & (16 | 4);
>> > 	lid_mask = (1 << lid_bits) - 1;
>> > 	cid_mask = (1 << cid_bits) - 1;
>> > 
>> > 	*cid = (ldr >> lid_bits) & cid_mask;
>> > 	*lid = ldr & lid_mask;
> Would jump predictor fail on the switch?  Or is size of the code that
> important?  This code is shorter, but is going to execute far more
> operations, so I think it would be slower ... (And harder to read.)

Considering the additional comparisons for the switch, I don't think
it's going to execute far more operations...

Paolo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 0/8] KVM: minor APIC fixes and cleanups
  2015-01-30 15:20   ` Radim Krčmář
@ 2015-01-30 15:24     ` Paolo Bonzini
  0 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30 15:24 UTC (permalink / raw)
  To: Radim Krčmář; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov



On 30/01/2015 16:20, Radim Krčmář wrote:
>> > +	/* XXX: A superset of x2APIC broadcasts is fine in practice as long as
>> > +	 * we don't support APIC ID > 0xfeffffff.
>> > +	 */
>> > 
>> > It's ugly. :)
> Yeah, there is: don't deliver x2APIC broadcasts to xAPIC.
> (I'm not even sure if it is correct ...)
> 
> Without that delivery, we could do something like
>   if (irq->dest_id == (x2apic_ipi ? X2APIC_BROADCAST : APIC_BROADCAST))

Nadav, do you know what real hardware does?

Paolo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map
  2015-01-30 15:23       ` Paolo Bonzini
@ 2015-01-30 16:57         ` Radim Krčmář
  2015-01-30 21:15           ` Paolo Bonzini
  0 siblings, 1 reply; 34+ messages in thread
From: Radim Krčmář @ 2015-01-30 16:57 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-01-30 16:23+0100, Paolo Bonzini:
> On 30/01/2015 16:14, Radim Krčmář wrote:
> > > > +	case KVM_APIC_MODE_XAPIC_FLAT:
> > > > +		*cid = 0;
> > > > +		*lid = ldr & 0xff;
> > > > +		return true;
> > > > +	case KVM_APIC_MODE_XAPIC_CLUSTER:
> > > > +		*cid = (ldr >> 4) & 0xf;
> > > > +		*lid = ldr & 0xf;
> > > > +		return true;
> > > > +	case KVM_APIC_MODE_X2APIC:
> > > > +		*cid = ldr >> 16;
> > > > +		*lid = ldr & 0xffff;
> > > > +		return true;
> > > > +	}
> > 
> >> > 	lid_bits = mode;
> >> > 	cid_bits = mode & (16 | 4);
> >> > 	lid_mask = (1 << lid_bits) - 1;
> >> > 	cid_mask = (1 << cid_bits) - 1;
> >> > 
> >> > 	*cid = (ldr >> lid_bits) & cid_mask;
> >> > 	*lid = ldr & lid_mask;
> > Would jump predictor fail on the switch?  Or is size of the code that
> > important?  This code is shorter, but is going to execute far more
> > operations, so I think it would be slower ... (And harder to read.)
> 
> Considering the additional comparisons for the switch, I don't think
> it's going to execute far more operations...

(A quick assembly comparison [1].)

As optimizations go, we could drop the "&" on cid as "cid < 16" is
checked later, so mode=4 practically does nothing ... Not the best for
future bugs, but still pretty safe -- only x2APIC can set a value that
would require the "&" and it can't have valid XAPIC mode, so u32 still
protects us enough.

  *cid = ldr >> mode;
  *lid = ldr & ((1 << mode) - 1);

Which is probably faster a solution where we don't shuffle switch cases
to jump to x2APIC first.  A comparison is at the very bottom [2].

Would that be ok in v2?


---
1:  To make it easier, it wasn't inlined, sorry.

    There are four parts, first one compares the whole functions and
    three subsequent compare each switch case, with common head/tail
    omitted.  (We don't care about performance when the map is invalid.)

    The optimized version is always first and next to it is the old one.

0000000000001700 <apic_logical_id>:  (ALI from now on)
  1700:	callq  1705 <ALI+0x5>     1700:	callq  1705 <ALI+0x5>
  1705:	push   %rbp               1705:	movzbl 0x10(%rdi),%eax
  1706:	movzbl 0x10(%rdi),%r8d    1709:	push   %rbp
  170b:	mov    %rcx,%r10          170a:	mov    %rsp,%rbp
  170e:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  1710:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  1713:	lea    -0x1(%r8),%ecx     1711:	cmp    $0x10,%al
  1717:	test   %r8d,%ecx          1713:	je     1740 <ALI+0x40>
  171a:	jne    1752 <ALI+0x52>    1715:	cmp    $0x4,%al
  171c:	mov    %r8d,%edi          1717:	je     1720 <ALI+0x20>
  171f:	mov    $0x1,%eax          1719:	xor    %eax,%eax
  1724:	mov    %esi,%r8d          171b:	pop    %rbp
  1727:	mov    %edi,%ecx          171c:	retq   
  1729:	mov    %eax,%r9d          171d:	nopl   (%rax)
  172c:	shr    %cl,%r8d           1720:	mov    %esi,%eax
  172f:	and    $0x14,%ecx         1722:	and    $0xf,%esi
  1732:	shl    %cl,%r9d           1725:	shr    $0x4,%eax
  1735:	mov    %edi,%ecx          1728:	and    $0xf,%eax
  1737:	shl    %cl,%eax           172b:	mov    %ax,(%rdx)
  1739:	sub    $0x1,%r9d          172e:	mov    $0x1,%eax
  173d:	sub    $0x1,%eax          1733:	mov    %si,(%rcx)
  1740:	and    %r9d,%r8d          1736:	pop    %rbp
  1743:	and    %esi,%eax          1737:	retq   
  1745:	mov    %r8w,(%rdx)        1738:	nopl   0x0(%rax,%rax,1)
  1749:	mov    %ax,(%r10)         173f:	
  174d:	mov    $0x1,%eax          1740:	mov    %esi,%eax
  1752:	pop    %rbp               1742:	shr    $0x10,%eax
  1753:	retq                      1745:	mov    %ax,(%rdx)
       	                          1748:	mov    $0x1,%eax
       	                          174d:	mov    %si,(%rcx)
       	                          1750:	pop    %rbp
       	                          1751:	retq   
       	                          1752:	nopw   0x0(%rax,%rax,1)
       	                          1758:	xor    %eax,%eax
       	                          175a:	and    $0xff,%si
       	                          175f:	mov    %ax,(%rdx)
       	                          1762:	mov    $0x1,%eax
       	                          1767:	mov    %si,(%rcx)
       	                          176a:	pop    %rbp
       	                          176b:	retq   



  170b:	mov    %rcx,%r10          170a:	mov    %rsp,%rbp
  170e:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  1710:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  1713:	lea    -0x1(%r8),%ecx         [jump]
  1717:	test   %r8d,%ecx          1758:	xor    %eax,%eax
  171a:	jne    1752 <ALI+0x52>    175a:	and    $0xff,%si
  171c:	mov    %r8d,%edi          175f:	mov    %ax,(%rdx)
  171f:	mov    $0x1,%eax          1762:	mov    $0x1,%eax
  1724:	mov    %esi,%r8d          1767:	mov    %si,(%rcx)
  1727:	mov    %edi,%ecx          
  1729:	mov    %eax,%r9d          
  172c:	shr    %cl,%r8d           
  172f:	and    $0x14,%ecx         
  1732:	shl    %cl,%r9d           
  1735:	mov    %edi,%ecx          
  1737:	shl    %cl,%eax           
  1739:	sub    $0x1,%r9d          
  173d:	sub    $0x1,%eax          
  1740:	and    %r9d,%r8d          
  1743:	and    %esi,%eax          
  1745:	mov    %r8w,(%rdx)        
  1749:	mov    %ax,(%r10)         
  174d:	mov    $0x1,%eax          


  170b:	mov    %rcx,%r10          170a:	mov    %rsp,%rbp
  170e:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  1710:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  1713:	lea    -0x1(%r8),%ecx     1711:	cmp    $0x10,%al
  1717:	test   %r8d,%ecx          1713:	je     1740 <ALI+0x40>
  171a:	jne    1752 <ALI+0x52>        [jump]
  171c:	mov    %r8d,%edi          1740:	mov    %esi,%eax
  171f:	mov    $0x1,%eax          1742:	shr    $0x10,%eax
  1724:	mov    %esi,%r8d          1745:	mov    %ax,(%rdx)
  1727:	mov    %edi,%ecx          1748:	mov    $0x1,%eax
  1729:	mov    %eax,%r9d          174d:	mov    %si,(%rcx)
  172c:	shr    %cl,%r8d           
  172f:	and    $0x14,%ecx         
  1732:	shl    %cl,%r9d           
  1735:	mov    %edi,%ecx          
  1737:	shl    %cl,%eax           
  1739:	sub    $0x1,%r9d          
  173d:	sub    $0x1,%eax          
  1740:	and    %r9d,%r8d          
  1743:	and    %esi,%eax          
  1745:	mov    %r8w,(%rdx)        
  1749:	mov    %ax,(%r10)         
  174d:	mov    $0x1,%eax          


  170b:	mov    %rcx,%r10          170a:	mov    %rsp,%rbp        
  170e:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  1710:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  1713:	lea    -0x1(%r8),%ecx     1711:	cmp    $0x10,%al
  1717:	test   %r8d,%ecx          1713:	je     1740 <ALI+0x40>
  171a:	jne    1752 <ALI+0x52>    1715:	cmp    $0x4,%al
  171c:	mov    %r8d,%edi          1717:	je     1720 <ALI+0x20>
  171f:	mov    $0x1,%eax              [jump]
  1724:	mov    %esi,%r8d          1720:	mov    %esi,%eax
  1727:	mov    %edi,%ecx          1722:	and    $0xf,%esi
  1729:	mov    %eax,%r9d          1725:	shr    $0x4,%eax
  172c:	shr    %cl,%r8d           1728:	and    $0xf,%eax
  172f:	and    $0x14,%ecx         172b:	mov    %ax,(%rdx)
  1732:	shl    %cl,%r9d           172e:	mov    $0x1,%eax
  1735:	mov    %edi,%ecx          1733:	mov    %si,(%rcx)
  1737:	shl    %cl,%eax           
  1739:	sub    $0x1,%r9d          
  173d:	sub    $0x1,%eax          
  1740:	and    %r9d,%r8d          
  1743:	and    %esi,%eax          
  1745:	mov    %r8w,(%rdx)        
  1749:	mov    %ax,(%r10)         
  174d:	mov    $0x1,%eax          


2: A comparison of aggressively optimized ALI with the worst case
   scenario, where x2APIC is the third jump.

  16e5:	mov    %rcx,%r9           170a:	mov    %rsp,%rbp        
  16ed:	xor    %eax,%eax          170d:	cmp    $0x8,%al
  16ef:	mov    %rsp,%rbp          170f:	je     1758 <ALI+0x58>
  16f2:	lea    -0x1(%rcx),%r8d    1711:	cmp    $0x10,%al
  16f6:	test   %ecx,%r8d          1713:	je     1740 <ALI+0x40>
  16f9:	jne    1717 <ALI+0x37>    1715:	cmp    $0x4,%al
  16fb:	mov    %esi,%eax          1717:	je     1720 <ALI+0x20>
  16fd:	shr    %cl,%eax               [jump]
  16ff:	mov    %ax,(%rdx)         1720:	mov    %esi,%eax
  1702:	mov    $0x1,%eax          1722:	and    $0xf,%esi
  1707:	shl    %cl,%eax           1725:	shr    $0x4,%eax
  1709:	sub    $0x1,%eax          1728:	and    $0xf,%eax
  170c:	and    %esi,%eax          172b:	mov    %ax,(%rdx)
  170e:	mov    %ax,(%r9)          172e:	mov    $0x1,%eax
  1712:	mov    $0x1,%eax          1733:	mov    %si,(%rcx)

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid
  2015-01-30 15:10       ` Paolo Bonzini
@ 2015-01-30 17:09         ` Radim Krčmář
  0 siblings, 0 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-01-30 17:09 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-01-30 16:10+0100, Paolo Bonzini:
> On 30/01/2015 15:56, Radim Krčmář wrote:
> > Do you know of a difference with it?
> >   new->mode & (new->mode - 1)   |  hweight8(new->mode) != 1
> >     lea    -0x1(%rax),%edi      |    popcnt %edi,%eax
> >     test   %eax,%edi            |    cmp    $1,%eax
> 
> x & (x - 1) is really hweight8(new->mode) > 1.  So if new->mode == 0 it
> would have a different result.

(I was thinking if execution profile of those two instructions isn't
 vastly different.

 ">" in this check works too ... later it seemed like it was pushing
 lucky coincidence too far, so it ended as "!=" :)

> >> Please add a comment to kvm_irq_delivery_to_apic_fast to explain what
> >> you are doing.
> > 
> > Would naming it kvm_apic_need_slow_delivery(), or something, be enough?
> 
> Or kvm_apic_map_valid() perhaps?

Sure, thanks.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 8/8] KVM: x86: simplify kvm_apic_map
  2015-01-30 16:57         ` Radim Krčmář
@ 2015-01-30 21:15           ` Paolo Bonzini
  0 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2015-01-30 21:15 UTC (permalink / raw)
  To: Radim Krčmář; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov



On 30/01/2015 17:57, Radim Krčmář wrote:
> As optimizations go, we could drop the "&" on cid as "cid < 16" is
> checked later, so mode=4 practically does nothing ... Not the best for
> future bugs, but still pretty safe -- only x2APIC can set a value that
> would require the "&" and it can't have valid XAPIC mode, so u32 still
> protects us enough.
> 
>   *cid = ldr >> mode;
>   *lid = ldr & ((1 << mode) - 1);
> 
> Which is probably faster a solution where we don't shuffle switch cases
> to jump to x2APIC first.  A comparison is at the very bottom [2].
> 
> Would that be ok in v2?

Yes, this is okay.  Thanks for looking at it!

Paolo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()
  2015-01-30  8:52   ` Paolo Bonzini
  2015-01-30 13:06     ` Radim Krčmář
@ 2015-02-02 14:26     ` Radim Krčmář
  2015-02-02 14:28       ` Paolo Bonzini
  2015-02-02 14:29       ` Radim Krčmář
  1 sibling, 2 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-02-02 14:26 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-01-30 09:52+0100, Paolo Bonzini:
> On 29/01/2015 22:48, Radim Krčmář wrote:
> > The majority of this patch turns
> >   result = 0; if (CODE) result = 1; return result;
> > into
> >   return CODE;
> > because we return bool now.
> 
> Added a bunch of "!= 0" to avoid automagic conversion to bool.

(I haven't checked it earlier ... sorry.)

> > -		if (((logical_id >> 4) == (mda >> 0x4))
> > -		    && (logical_id & mda & 0xf))
> > -			result = 1;
> > -		break;
> > +		return ((logical_id >> 4) == (mda >> 4))
> > +		       && (logical_id & mda & 0xf);

was merged as

+		return ((logical_id >> 4) == (mda >> 4))
+		       && (logical_id & mda & 0xf) != 0;

but it has to be parenthesized ('&&' has lower precedence than '!=').

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()
  2015-02-02 14:26     ` Radim Krčmář
@ 2015-02-02 14:28       ` Paolo Bonzini
  2015-02-02 14:30         ` Radim Krčmář
  2015-02-02 14:29       ` Radim Krčmář
  1 sibling, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2015-02-02 14:28 UTC (permalink / raw)
  To: Radim Krčmář; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov



On 02/02/2015 15:26, Radim Krčmář wrote:
>>> > > +		return ((logical_id >> 4) == (mda >> 4))
>>> > > +		       && (logical_id & mda & 0xf);
> was merged as
> 
> +		return ((logical_id >> 4) == (mda >> 4))
> +		       && (logical_id & mda & 0xf) != 0;
> 
> but it has to be parenthesized ('&&' has lower precedence than '!=').

Lower precedence means that the merged version is right (unless my brain
went bonkers, which I cannot exclude).  "!=" has higher precedence and
thus it is implicitly parenthesized.

In fact the first comparison could have its parentheses removed as well.

Paolo

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()
  2015-02-02 14:26     ` Radim Krčmář
  2015-02-02 14:28       ` Paolo Bonzini
@ 2015-02-02 14:29       ` Radim Krčmář
  1 sibling, 0 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-02-02 14:29 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-02-02 15:26+0100, Radim Krčmář:
> 2015-01-30 09:52+0100, Paolo Bonzini:
> +		return ((logical_id >> 4) == (mda >> 4))
> +		       && (logical_id & mda & 0xf) != 0;
> 
> but it has to be parenthesized ('&&' has lower precedence than '!=').

No, my bad, I understood it now.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*()
  2015-02-02 14:28       ` Paolo Bonzini
@ 2015-02-02 14:30         ` Radim Krčmář
  0 siblings, 0 replies; 34+ messages in thread
From: Radim Krčmář @ 2015-02-02 14:30 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: linux-kernel, kvm, Nadav Amit, Gleb Natapov

2015-02-02 15:28+0100, Paolo Bonzini:
> 
> 
> On 02/02/2015 15:26, Radim Krčmář wrote:
> >>> > > +		return ((logical_id >> 4) == (mda >> 4))
> >>> > > +		       && (logical_id & mda & 0xf);
> > was merged as
> > 
> > +		return ((logical_id >> 4) == (mda >> 4))
> > +		       && (logical_id & mda & 0xf) != 0;
> > 
> > but it has to be parenthesized ('&&' has lower precedence than '!=').
> 
> Lower precedence means that the merged version is right (unless my brain
> went bonkers, which I cannot exclude).  "!=" has higher precedence and
> thus it is implicitly parenthesized.
> 
> In fact the first comparison could have its parentheses removed as well.

Yes, it could be,
  logical_id >> 4 == mda >> 4 && (logical_id & mda & 0xf) != 0

I missed the point and thought that you wanted to turn the whole
expression into bool, when it already was one.

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2015-02-02 14:31 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-29 21:48 [PATCH 0/8] KVM: minor APIC fixes and cleanups Radim Krčmář
2015-01-29 21:48 ` [PATCH 1/8] KVM: x86: return bool from kvm_apic_match*() Radim Krčmář
2015-01-29 22:10   ` Joe Perches
2015-01-30  8:50     ` Paolo Bonzini
2015-01-29 21:48 ` [PATCH 2/8] KVM: x86: cleanup kvm_apic_match_*() Radim Krčmář
2015-01-30  8:52   ` Paolo Bonzini
2015-01-30 13:06     ` Radim Krčmář
2015-02-02 14:26     ` Radim Krčmář
2015-02-02 14:28       ` Paolo Bonzini
2015-02-02 14:30         ` Radim Krčmář
2015-02-02 14:29       ` Radim Krčmář
2015-01-29 21:48 ` [PATCH 3/8] KVM: x86: replace 0 with APIC_DEST_PHYSICAL Radim Krčmář
2015-01-29 21:48 ` [PATCH 4/8] KVM: x86: fix x2apic logical address matching Radim Krčmář
2015-01-29 21:48 ` [PATCH 5/8] KVM: x86: use MDA for interrupt matching Radim Krčmář
2015-01-30  9:03   ` Paolo Bonzini
2015-01-30 13:09     ` Radim Krčmář
2015-01-29 21:48 ` [PATCH 6/8] KVM: x86: allow mixed APIC mode broadcast Radim Krčmář
2015-01-29 21:48 ` [PATCH 7/8] KVM: x86: avoid logical_map when it is invalid Radim Krčmář
2015-01-30  9:19   ` Paolo Bonzini
2015-01-30 14:21     ` Radim Krčmář
2015-01-30 14:21       ` Paolo Bonzini
2015-01-30  9:38   ` Paolo Bonzini
2015-01-30 14:56     ` Radim Krčmář
2015-01-30 15:10       ` Paolo Bonzini
2015-01-30 17:09         ` Radim Krčmář
2015-01-29 21:48 ` [PATCH 8/8] KVM: x86: simplify kvm_apic_map Radim Krčmář
2015-01-30  9:18   ` Paolo Bonzini
2015-01-30 15:14     ` Radim Krčmář
2015-01-30 15:23       ` Paolo Bonzini
2015-01-30 16:57         ` Radim Krčmář
2015-01-30 21:15           ` Paolo Bonzini
2015-01-30  9:22 ` [PATCH 0/8] KVM: minor APIC fixes and cleanups Paolo Bonzini
2015-01-30 15:20   ` Radim Krčmář
2015-01-30 15:24     ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.