[PATCH v3 0/4] VT-d posted-interrupts follow ups

[PATCH v3 0/4] VT-d posted-interrupts follow ups

[Feng Wu]

This series contains 4 patches:
[1/4]: Change back to remapped mode when posted mode is not used.
[2/4]: Add vector-hashing support to deliver lowest-priority
       interrupts for non VT-d PI case.
[3/4]: Add vector-hashing support to deliver lowest-priority
       interrupts for VT-d PI case.
[4/4]: Add some enhancement to the trace message for vt-d PI.

Detailed changelog is in each patch.

Feng Wu (4):
  KVM: Recover IRTE to remapped mode if the interrupt is not
    single-destination
  KVM: x86: Use vector-hashing to deliver lowest-priority interrupts
  KVM: x86: Add lowest-priority support for vt-d posted-interrupts
  KVM/VMX: Add host irq information in trace event when updating IRTE
    for posted interrupts

 arch/x86/include/asm/kvm_host.h |   6 ++-
 arch/x86/kvm/irq_comm.c         |  35 +++++++++----
 arch/x86/kvm/lapic.c            | 111 ++++++++++++++++++++++++++++++++++++----
 arch/x86/kvm/lapic.h            |   6 ++-
 arch/x86/kvm/trace.h            |  12 +++--
 arch/x86/kvm/vmx.c              |  13 ++++-
 arch/x86/kvm/x86.c              |   9 ++++
 arch/x86/kvm/x86.h              |   1 +
 8 files changed, 163 insertions(+), 30 deletions(-)

-- 
2.1.0

[PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Feng Wu]

When the interrupt is not single destination any more, we need
to change back IRTE to remapped mode explicitly.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 arch/x86/kvm/vmx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index e2951b6..13d14d4 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
 		 */
 
 		kvm_set_msi_irq(e, &irq);
-		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
+		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
+			/*
+			 * Make sure the IRTE is in remapped mode if
+			 * we don't handle it in posted mode.
+			 */
+			pi_set_sn(vcpu_to_pi_desc(vcpu));
+			ret = irq_set_vcpu_affinity(host_irq, NULL);
+			pi_clear_sn(vcpu_to_pi_desc(vcpu));
+
 			continue;
+		}
 
 		vcpu_info.pi_desc_addr = __pa(vcpu_to_pi_desc(vcpu));
 		vcpu_info.vector = irq.vector;
-- 
2.1.0

[PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Feng Wu]

Use vector-hashing to deliver lowest-priority interrupts, As an
example, modern Intel CPUs in server platform use this method to
handle lowest-priority interrupts.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
v3:
- Fix a bug for sparse topologies, in that case, vcpu_id is not equal
to the return value got by kvm_get_vcpu().
- Remove unnecessary check in fast irq delivery patch.
- print a error message only once for each guest when we find hardware
  disabled LAPIC during interrupt injection.

 arch/x86/include/asm/kvm_host.h |  2 ++
 arch/x86/kvm/irq_comm.c         | 27 +++++++++++++++++----
 arch/x86/kvm/lapic.c            | 52 ++++++++++++++++++++++++++++++++++++++---
 arch/x86/kvm/lapic.h            |  2 ++
 arch/x86/kvm/x86.c              |  9 +++++++
 arch/x86/kvm/x86.h              |  1 +
 6 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 44adbb8..5054810 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -754,6 +754,8 @@ struct kvm_arch {
 
 	bool irqchip_split;
 	u8 nr_reserved_ioapic_pins;
+
+	int disabled_lapic_found;
 };
 
 struct kvm_vm_stat {
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 8fc89ef..062e907 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -34,6 +34,7 @@
 #include "lapic.h"
 
 #include "hyperv.h"
+#include "x86.h"
 
 static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
 			   struct kvm *kvm, int irq_source_id, int level,
@@ -55,8 +56,10 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		struct kvm_lapic_irq *irq, unsigned long *dest_map)
 {
-	int i, r = -1;
+	int i, r = -1, idx = 0;
 	struct kvm_vcpu *vcpu, *lowest = NULL;
+	unsigned long dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
+	unsigned int dest_vcpus = 0;
 
 	if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
 			kvm_lowest_prio_delivery(irq)) {
@@ -67,6 +70,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	if (kvm_irq_delivery_to_apic_fast(kvm, src, irq, &r, dest_map))
 		return r;
 
+	memset(dest_vcpu_bitmap, 0, sizeof(dest_vcpu_bitmap));
+
 	kvm_for_each_vcpu(i, vcpu, kvm) {
 		if (!kvm_apic_present(vcpu))
 			continue;
@@ -80,13 +85,25 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 				r = 0;
 			r += kvm_apic_set_irq(vcpu, irq, dest_map);
 		} else if (kvm_lapic_enabled(vcpu)) {
-			if (!lowest)
-				lowest = vcpu;
-			else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
-				lowest = vcpu;
+			if (!kvm_vector_hashing_enabled()) {
+				if (!lowest)
+					lowest = vcpu;
+				else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
+					lowest = vcpu;
+			} else {
+				__set_bit(i, dest_vcpu_bitmap);
+				dest_vcpus++;
+			}
 		}
 	}
 
+	if (dest_vcpus != 0) {
+		idx = kvm_vector_2_index(irq->vector, dest_vcpus,
+					 dest_vcpu_bitmap, KVM_MAX_VCPUS);
+
+		lowest = kvm_get_vcpu(kvm, idx - 1);
+	}
+
 	if (lowest)
 		r = kvm_apic_set_irq(lowest, irq, dest_map);
 
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 36591fa..e1a449da 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -675,6 +675,22 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 	}
 }
 
+int kvm_vector_2_index(u32 vector, u32 dest_vcpus,
+		       const unsigned long *bitmap, u32 bitmap_size)
+{
+	u32 mod;
+	int i, idx = 0;
+
+	mod = vector % dest_vcpus;
+
+	for (i = 0; i <= mod; i++) {
+		idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
+		BUG_ON(idx > bitmap_size);
+	}
+
+	return idx;
+}
+
 bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
 {
@@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 
 		dst = map->logical_map[cid];
 
-		if (kvm_lowest_prio_delivery(irq)) {
+		if (!kvm_lowest_prio_delivery(irq))
+			goto set_irq;
+
+		if (!kvm_vector_hashing_enabled()) {
 			int l = -1;
 			for_each_set_bit(i, &bitmap, 16) {
 				if (!dst[i])
 					continue;
 				if (l < 0)
 					l = i;
-				else if (kvm_apic_compare_prio(dst[i]->vcpu, dst[l]->vcpu) < 0)
+				else if (kvm_apic_compare_prio(dst[i]->vcpu,
+							dst[l]->vcpu) < 0)
 					l = i;
 			}
-
 			bitmap = (l >= 0) ? 1 << l : 0;
+		} else {
+			int idx = 0;
+			unsigned int dest_vcpus = 0;
+
+			dest_vcpus = hweight16(bitmap);
+			if (dest_vcpus == 0)
+				goto out;
+
+			idx = kvm_vector_2_index(irq->vector,
+				dest_vcpus, &bitmap, 16);
+
+			/*
+			 * We may find a hardware disabled LAPIC here, if that
+			 * is the case, print out a error message once for each
+			 * guest and return.
+			 */
+			if (!dst[idx-1] &&
+				(kvm->arch.disabled_lapic_found == 0)) {
+				kvm->arch.disabled_lapic_found = 1;
+				printk(KERN_ERR
+					"Disabled LAPIC found during irq injection\n");
+				goto out;
+			}
+
+			bitmap = 0;
+			__set_bit(idx-1, &bitmap);
 		}
 	}
 
+set_irq:
 	for_each_set_bit(i, &bitmap, 16) {
 		if (!dst[i])
 			continue;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 41bdb35..d864601 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -175,4 +175,6 @@ void wait_lapic_expire(struct kvm_vcpu *vcpu);
 
 bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
 			struct kvm_vcpu **dest_vcpu);
+int kvm_vector_2_index(u32 vector, u32 dest_vcpus,
+		       const unsigned long *bitmap, u32 bitmap_size);
 #endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4244c2b..47daf77 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -123,6 +123,9 @@ module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
 unsigned int __read_mostly lapic_timer_advance_ns = 0;
 module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
 
+bool __read_mostly enable_vector_hashing = 1;
+module_param(enable_vector_hashing, bool, S_IRUGO);
+
 static bool __read_mostly backwards_tsc_observed = false;
 
 #define KVM_NR_SHARED_MSRS 16
@@ -8370,6 +8373,12 @@ int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
 	return kvm_x86_ops->update_pi_irte(kvm, host_irq, guest_irq, set);
 }
 
+bool kvm_vector_hashing_enabled(void)
+{
+	return enable_vector_hashing;
+}
+EXPORT_SYMBOL_GPL(kvm_vector_hashing_enabled);
+
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_exit);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_fast_mmio);
 EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_inj_virq);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index f2afa5f..04bd0f9 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -179,6 +179,7 @@ int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data);
 int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata);
 bool kvm_mtrr_check_gfn_range_consistency(struct kvm_vcpu *vcpu, gfn_t gfn,
 					  int page_num);
+bool kvm_vector_hashing_enabled(void);
 
 #define KVM_SUPPORTED_XCR0     (XFEATURE_MASK_FP | XFEATURE_MASK_SSE \
 				| XFEATURE_MASK_YMM | XFEATURE_MASK_BNDREGS \
-- 
2.1.0

[PATCH v3 3/4] KVM: x86: Add lowest-priority support for vt-d posted-interrupts

[Feng Wu]

Use vector-hashing to deliver lowest-priority interrupts for
VT-d posted-interrupts. This patch extends kvm_intr_is_single_vcpu()
to support lowest-priority handling.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
v3:
- Remove unnecessary check in fast irq delivery patch
- print a error message only once for each guest when we find hardware
  disabled LAPIC during interrupt injection.

 arch/x86/include/asm/kvm_host.h |  4 +--
 arch/x86/kvm/irq_comm.c         |  8 +++---
 arch/x86/kvm/lapic.c            | 59 +++++++++++++++++++++++++++++++++++------
 arch/x86/kvm/lapic.h            |  4 +--
 arch/x86/kvm/vmx.c              |  2 +-
 5 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 5054810..ce5a2ea 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1316,8 +1316,8 @@ int x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size);
 bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu);
 bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu);
 
-bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
-			     struct kvm_vcpu **dest_vcpu);
+bool kvm_intr_can_posting(struct kvm *kvm, struct kvm_lapic_irq *irq,
+			  struct kvm_vcpu **dest_vcpu);
 
 void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
 		     struct kvm_lapic_irq *irq);
diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 062e907..75cd02b 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -300,13 +300,13 @@ out:
 	return r;
 }
 
-bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
-			     struct kvm_vcpu **dest_vcpu)
+bool kvm_intr_can_posting(struct kvm *kvm, struct kvm_lapic_irq *irq,
+		struct kvm_vcpu **dest_vcpu)
 {
 	int i, r = 0;
 	struct kvm_vcpu *vcpu;
 
-	if (kvm_intr_is_single_vcpu_fast(kvm, irq, dest_vcpu))
+	if (kvm_intr_can_posting_fast(kvm, irq, dest_vcpu))
 		return true;
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
@@ -325,7 +325,7 @@ bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
 
 	return r == 1;
 }
-EXPORT_SYMBOL_GPL(kvm_intr_is_single_vcpu);
+EXPORT_SYMBOL_GPL(kvm_intr_can_posting);
 
 #define IOAPIC_ROUTING_ENTRY(irq) \
 	{ .gsi = irq, .type = KVM_IRQ_ROUTING_IRQCHIP,	\
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index e1a449da..d22e87e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -800,7 +800,22 @@ out:
 	return ret;
 }
 
-bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
+/*
+ * This routine tries to handler interrupts in posted mode, here is how
+ * it deals with different cases:
+ * - For single-destination interrupts, handle it in posted mode
+ * - Else if vector hashing is enabled and it is a lowest-priority
+ *   interrupt, handle it in posted mode and use the following mechanism
+ *   to find the destinaiton vCPU.
+ *	1. For lowest-priority interrupts, store all the possible
+ *	   destination vCPUs in an array.
+ *	2. Use "guest vector % max number of destination vCPUs" to find
+ *	   the right destination vCPU in the array for the lowest-priority
+ *	   interrupt.
+ * - Otherwise, use remapped mode to inject the interrupt.
+ */
+
+bool kvm_intr_can_posting_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
 			struct kvm_vcpu **dest_vcpu)
 {
 	struct kvm_apic_map *map;
@@ -841,16 +856,44 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
 		if (cid >= ARRAY_SIZE(map->logical_map))
 			goto out;
 
-		for_each_set_bit(i, &bitmap, 16) {
-			dst = map->logical_map[cid][i];
-			if (++r == 2)
+		if (kvm_vector_hashing_enabled() &&
+				kvm_lowest_prio_delivery(irq)) {
+			int idx = 0;
+			unsigned int dest_vcpus = 0;
+
+			dest_vcpus = hweight16(bitmap);
+			if (dest_vcpus == 0)
 				goto out;
-		}
 
-		if (dst && kvm_apic_present(dst->vcpu))
+			idx = kvm_vector_2_index(irq->vector, dest_vcpus,
+						 &bitmap, 16);
+
+			/*
+			 * We may find a hardware disabled LAPIC here, if that
+			 * is the case, print out a error message once for each
+			 * guest and return
+			 */
+			dst = map->logical_map[cid][idx-1];
+			if (!dst && (kvm->arch.disabled_lapic_found == 0)) {
+				kvm->arch.disabled_lapic_found = 1;
+				printk(KERN_ERR
+					"Disabled LAPIC found during irq injection\n");
+				goto out;
+			}
+
 			*dest_vcpu = dst->vcpu;
-		else
-			goto out;
+		} else {
+			for_each_set_bit(i, &bitmap, 16) {
+				dst = map->logical_map[cid][i];
+				if (++r == 2)
+					goto out;
+			}
+
+			if (dst && kvm_apic_present(dst->vcpu))
+				*dest_vcpu = dst->vcpu;
+			else
+				goto out;
+		}
 	}
 
 	ret = true;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index d864601..6145939 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -173,8 +173,8 @@ bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector);
 
 void wait_lapic_expire(struct kvm_vcpu *vcpu);
 
-bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
-			struct kvm_vcpu **dest_vcpu);
+bool kvm_intr_can_posting_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
+			       struct kvm_vcpu **dest_vcpu);
 int kvm_vector_2_index(u32 vector, u32 dest_vcpus,
 		       const unsigned long *bitmap, u32 bitmap_size);
 #endif
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 13d14d4..b909ea1 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10764,7 +10764,7 @@ static int vmx_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
 		 */
 
 		kvm_set_msi_irq(e, &irq);
-		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
+		if (!kvm_intr_can_posting(kvm, &irq, &vcpu)) {
 			/*
 			 * Make sure the IRTE is in remapped mode if
 			 * we don't handle it in posted mode.
-- 
2.1.0

[PATCH v3 4/4] KVM/VMX: Add host irq information in trace event when updating IRTE for posted interrupts

[Feng Wu]

Add host irq information in trace event, so we can better understand
which irq is in posted mode.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 arch/x86/kvm/trace.h | 12 ++++++++----
 arch/x86/kvm/vmx.c   |  2 +-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index ad9f6a2..2f1ea2f 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -996,11 +996,13 @@ TRACE_EVENT(kvm_enter_smm,
  * Tracepoint for VT-d posted-interrupts.
  */
 TRACE_EVENT(kvm_pi_irte_update,
-	TP_PROTO(unsigned int vcpu_id, unsigned int gsi,
-		 unsigned int gvec, u64 pi_desc_addr, bool set),
-	TP_ARGS(vcpu_id, gsi, gvec, pi_desc_addr, set),
+	TP_PROTO(unsigned int host_irq, unsigned int vcpu_id,
+		 unsigned int gsi, unsigned int gvec,
+		 u64 pi_desc_addr, bool set),
+	TP_ARGS(host_irq, vcpu_id, gsi, gvec, pi_desc_addr, set),
 
 	TP_STRUCT__entry(
+		__field(	unsigned int,	host_irq	)
 		__field(	unsigned int,	vcpu_id		)
 		__field(	unsigned int,	gsi		)
 		__field(	unsigned int,	gvec		)
@@ -1009,6 +1011,7 @@ TRACE_EVENT(kvm_pi_irte_update,
 	),
 
 	TP_fast_assign(
+		__entry->host_irq	= host_irq;
 		__entry->vcpu_id	= vcpu_id;
 		__entry->gsi		= gsi;
 		__entry->gvec		= gvec;
@@ -1016,9 +1019,10 @@ TRACE_EVENT(kvm_pi_irte_update,
 		__entry->set		= set;
 	),
 
-	TP_printk("VT-d PI is %s for this irq, vcpu %u, gsi: 0x%x, "
+	TP_printk("VT-d PI is %s for irq %u, vcpu %u, gsi: 0x%x, "
 		  "gvec: 0x%x, pi_desc_addr: 0x%llx",
 		  __entry->set ? "enabled and being updated" : "disabled",
+		  __entry->host_irq,
 		  __entry->vcpu_id,
 		  __entry->gsi,
 		  __entry->gvec,
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b909ea1..1e110a2 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -10779,7 +10779,7 @@ static int vmx_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
 		vcpu_info.pi_desc_addr = __pa(vcpu_to_pi_desc(vcpu));
 		vcpu_info.vector = irq.vector;
 
-		trace_kvm_pi_irte_update(vcpu->vcpu_id, e->gsi,
+		trace_kvm_pi_irte_update(vcpu->vcpu_id, host_irq, e->gsi,
 				vcpu_info.vector, vcpu_info.pi_desc_addr, set);
 
 		if (set)
-- 
2.1.0

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Yang Zhang]

On 2016/1/20 9:42, Feng Wu wrote:
> When the interrupt is not single destination any more, we need
> to change back IRTE to remapped mode explicitly.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>   arch/x86/kvm/vmx.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index e2951b6..13d14d4 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
>   		 */
>
>   		kvm_set_msi_irq(e, &irq);
> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> +			/*
> +			 * Make sure the IRTE is in remapped mode if
> +			 * we don't handle it in posted mode.
> +			 */
> +			pi_set_sn(vcpu_to_pi_desc(vcpu));
> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
> +
>   			continue;
> +		}
>
>   		vcpu_info.pi_desc_addr = __pa(vcpu_to_pi_desc(vcpu));
>   		vcpu_info.vector = irq.vector;
>

I am still feel weird with this change: according the semantic of VT-d 
posted interrupt, the interrupt will injected to guest through posted 
notification and /proc/interrupts shows the same meaning. But now, 
without being aware of user, the interrupt changes to legacy way and it 
appears on different entry on /proc/interrupts. It looks weird.

Any comments? Paolo.

-- 
best regards
yang

RE: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Wu, Feng]

> -----Original Message-----
> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> Sent: Thursday, January 21, 2016 11:06 AM
> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> rkrcmar@redhat.com
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> interrupt is not single-destination
> 
> On 2016/1/20 9:42, Feng Wu wrote:
> > When the interrupt is not single destination any more, we need
> > to change back IRTE to remapped mode explicitly.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> >   arch/x86/kvm/vmx.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index e2951b6..13d14d4 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm
> *kvm, unsigned int host_irq,
> >   		 */
> >
> >   		kvm_set_msi_irq(e, &irq);
> > -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
> > +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> > +			/*
> > +			 * Make sure the IRTE is in remapped mode if
> > +			 * we don't handle it in posted mode.
> > +			 */
> > +			pi_set_sn(vcpu_to_pi_desc(vcpu));
> > +			ret = irq_set_vcpu_affinity(host_irq, NULL);
> > +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
> > +
> >   			continue;
> > +		}
> >
> >   		vcpu_info.pi_desc_addr = __pa(vcpu_to_pi_desc(vcpu));
> >   		vcpu_info.vector = irq.vector;
> >
> 
> I am still feel weird with this change: according the semantic of VT-d
> posted interrupt, the interrupt will injected to guest through posted
> notification and /proc/interrupts shows the same meaning. But now,
> without being aware of user, the interrupt changes to legacy way and it
> appears on different entry on /proc/interrupts. It looks weird.

I don't think it has problem here, IMO, this is exactly how it works.
There should be different entry for the interrupts in VT-d PI mode
and leagcy mode.

For VT-d PI mode, it is delivered by notification event, for legacy mode,
it is delivered by VFIO.

Thanks,
Feng

> 
> Any comments? Paolo.
> 
> --
> best regards
> yang

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Yang Zhang]

On 2016/1/21 11:14, Wu, Feng wrote:
>
>
>> -----Original Message-----
>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>> Sent: Thursday, January 21, 2016 11:06 AM
>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>> rkrcmar@redhat.com
>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
>> interrupt is not single-destination
>>
>> On 2016/1/20 9:42, Feng Wu wrote:
>>> When the interrupt is not single destination any more, we need
>>> to change back IRTE to remapped mode explicitly.
>>>
>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>> ---
>>>    arch/x86/kvm/vmx.c | 11 ++++++++++-
>>>    1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index e2951b6..13d14d4 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm
>> *kvm, unsigned int host_irq,
>>>    		 */
>>>
>>>    		kvm_set_msi_irq(e, &irq);
>>> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
>>> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
>>> +			/*
>>> +			 * Make sure the IRTE is in remapped mode if
>>> +			 * we don't handle it in posted mode.
>>> +			 */
>>> +			pi_set_sn(vcpu_to_pi_desc(vcpu));
>>> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
>>> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
>>> +
>>>    			continue;
>>> +		}
>>>
>>>    		vcpu_info.pi_desc_addr = __pa(vcpu_to_pi_desc(vcpu));
>>>    		vcpu_info.vector = irq.vector;
>>>
>>
>> I am still feel weird with this change: according the semantic of VT-d
>> posted interrupt, the interrupt will injected to guest through posted
>> notification and /proc/interrupts shows the same meaning. But now,
>> without being aware of user, the interrupt changes to legacy way and it
>> appears on different entry on /proc/interrupts. It looks weird.
>
> I don't think it has problem here, IMO, this is exactly how it works.
> There should be different entry for the interrupts in VT-d PI mode
> and leagcy mode.

I am not saying any problem here. Just feel weird. From a normal user's 
point, he has turned on the VT-d pi and according the semantic of VT-d 
pi, he should not observe the interrupt through legacy mode, but now he 
do see it. Maybe print out a message here will be helpful, like what you 
did for disabled lapic found during irq injection.

>
> For VT-d PI mode, it is delivered by notification event, for legacy mode,
> it is delivered by VFIO.
>
> Thanks,
> Feng
>
>>
>> Any comments? Paolo.
>>
>> --
>> best regards
>> yang


-- 
best regards
yang

RE: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Wu, Feng]

> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> Behalf Of Yang Zhang
> Sent: Thursday, January 21, 2016 11:35 AM
> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> rkrcmar@redhat.com
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> interrupt is not single-destination
> 
> On 2016/1/21 11:14, Wu, Feng wrote:
> >
> >
> >> -----Original Message-----
> >> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> >> Sent: Thursday, January 21, 2016 11:06 AM
> >> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> >> rkrcmar@redhat.com
> >> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> >> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> >> interrupt is not single-destination
> >>
> >> On 2016/1/20 9:42, Feng Wu wrote:
> >>> When the interrupt is not single destination any more, we need
> >>> to change back IRTE to remapped mode explicitly.
> >>>
> >>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> >>> ---
> >>>    arch/x86/kvm/vmx.c | 11 ++++++++++-
> >>>    1 file changed, 10 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >>> index e2951b6..13d14d4 100644
> >>> --- a/arch/x86/kvm/vmx.c
> >>> +++ b/arch/x86/kvm/vmx.c
> >>> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm
> >> *kvm, unsigned int host_irq,
> >>>    		 */
> >>>
> >>>    		kvm_set_msi_irq(e, &irq);
> >>> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
> >>> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> >>> +			/*
> >>> +			 * Make sure the IRTE is in remapped mode if
> >>> +			 * we don't handle it in posted mode.
> >>> +			 */
> >>> +			pi_set_sn(vcpu_to_pi_desc(vcpu));
> >>> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
> >>> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
> >>> +
> >>>    			continue;
> >>> +		}
> >>>
> >>>    		vcpu_info.pi_desc_addr = __pa(vcpu_to_pi_desc(vcpu));
> >>>    		vcpu_info.vector = irq.vector;
> >>>
> >>
> >> I am still feel weird with this change: according the semantic of VT-d
> >> posted interrupt, the interrupt will injected to guest through posted
> >> notification and /proc/interrupts shows the same meaning. But now,
> >> without being aware of user, the interrupt changes to legacy way and it
> >> appears on different entry on /proc/interrupts. It looks weird.
> >
> > I don't think it has problem here, IMO, this is exactly how it works.
> > There should be different entry for the interrupts in VT-d PI mode
> > and leagcy mode.
> 
> I am not saying any problem here. Just feel weird. From a normal user's
> point, he has turned on the VT-d pi and according the semantic of VT-d
> pi, he should not observe the interrupt through legacy mode, but now he
> do see it. Maybe print out a message here will be helpful, like what you
> did for disabled lapic found during irq injection.

Even VT-d PI is on, not all interrupts can be handled by it, the reason the
interrupts is changed back to legacy mode is because the user changes
the affinity, and it cannot be handle in PI mode, and hence legacy mode
is used. It is the user's behavior that cause this mode change, seems it is
not so weird to me. But add some message here is good idea, just like
what I did later in this function, I can also add the following trace
message here.

                trace_kvm_pi_irte_update(vcpu->vcpu_id, host_irq, e->gsi,
                                vcpu_info.vector, vcpu_info.pi_desc_addr, set);

Thanks,
Feng

> 
> >
> > For VT-d PI mode, it is delivered by notification event, for legacy mode,
> > it is delivered by VFIO.
> >
> > Thanks,
> > Feng
> >
> >>
> >> Any comments? Paolo.
> >>
> >> --
> >> best regards
> >> yang
> 
> 
> --
> best regards
> yang
> --
> 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

RE: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Tian, Kevin]

> From: Wu, Feng
> Sent: Thursday, January 21, 2016 12:43 PM
> 
> > -----Original Message-----
> > From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> > Behalf Of Yang Zhang
> > Sent: Thursday, January 21, 2016 11:35 AM
> > To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> > rkrcmar@redhat.com
> > Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> > Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> > interrupt is not single-destination
> >
> > On 2016/1/21 11:14, Wu, Feng wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> > >> Sent: Thursday, January 21, 2016 11:06 AM
> > >> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> > >> rkrcmar@redhat.com
> > >> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> > >> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> > >> interrupt is not single-destination
> > >>
> > >> On 2016/1/20 9:42, Feng Wu wrote:
> > >>> When the interrupt is not single destination any more, we need
> > >>> to change back IRTE to remapped mode explicitly.
> > >>>
> > >>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> > >>> ---
> > >>>    arch/x86/kvm/vmx.c | 11 ++++++++++-
> > >>>    1 file changed, 10 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > >>> index e2951b6..13d14d4 100644
> > >>> --- a/arch/x86/kvm/vmx.c
> > >>> +++ b/arch/x86/kvm/vmx.c
> > >>> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm
> > >> *kvm, unsigned int host_irq,
> > >>>    		 */
> > >>>
> > >>>    		kvm_set_msi_irq(e, &irq);
> > >>> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
> > >>> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> > >>> +			/*
> > >>> +			 * Make sure the IRTE is in remapped mode if
> > >>> +			 * we don't handle it in posted mode.
> > >>> +			 */
> > >>> +			pi_set_sn(vcpu_to_pi_desc(vcpu));
> > >>> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
> > >>> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
> > >>> +
> > >>>    			continue;
> > >>> +		}
> > >>>
> > >>>    		vcpu_info.pi_desc_addr = __pa(vcpu_to_pi_desc(vcpu));
> > >>>    		vcpu_info.vector = irq.vector;
> > >>>
> > >>
> > >> I am still feel weird with this change: according the semantic of VT-d
> > >> posted interrupt, the interrupt will injected to guest through posted
> > >> notification and /proc/interrupts shows the same meaning. But now,
> > >> without being aware of user, the interrupt changes to legacy way and it
> > >> appears on different entry on /proc/interrupts. It looks weird.
> > >
> > > I don't think it has problem here, IMO, this is exactly how it works.
> > > There should be different entry for the interrupts in VT-d PI mode
> > > and leagcy mode.
> >
> > I am not saying any problem here. Just feel weird. From a normal user's
> > point, he has turned on the VT-d pi and according the semantic of VT-d
> > pi, he should not observe the interrupt through legacy mode, but now he
> > do see it. Maybe print out a message here will be helpful, like what you
> > did for disabled lapic found during irq injection.
> 
> Even VT-d PI is on, not all interrupts can be handled by it, the reason the
> interrupts is changed back to legacy mode is because the user changes
> the affinity, and it cannot be handle in PI mode, and hence legacy mode
> is used. It is the user's behavior that cause this mode change, seems it is
> not so weird to me. But add some message here is good idea, just like
> what I did later in this function, I can also add the following trace
> message here.
> 
>                 trace_kvm_pi_irte_update(vcpu->vcpu_id, host_irq, e->gsi,
>                                 vcpu_info.vector, vcpu_info.pi_desc_addr, set);
> 
> Thanks,
> Feng
> 

Right. Whether the interrupt is delivered into guest directly with PI or with 
legacy mode, is completely agnostic to the guest. Guest can't tell
or set any expectation on the underlying delivering method, so there is no
guest visible impact. 

Thanks
Kevin

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Yang Zhang]

On 2016/1/21 12:42, Wu, Feng wrote:
>
>
>> -----Original Message-----
>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
>> Behalf Of Yang Zhang
>> Sent: Thursday, January 21, 2016 11:35 AM
>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>> rkrcmar@redhat.com
>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
>> interrupt is not single-destination
>>
>> On 2016/1/21 11:14, Wu, Feng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>> Sent: Thursday, January 21, 2016 11:06 AM
>>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>>>> rkrcmar@redhat.com
>>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>>>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
>>>> interrupt is not single-destination
>>>>
>>>> On 2016/1/20 9:42, Feng Wu wrote:
>>>>> When the interrupt is not single destination any more, we need
>>>>> to change back IRTE to remapped mode explicitly.
>>>>>
>>>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>>>> ---
>>>>>     arch/x86/kvm/vmx.c | 11 ++++++++++-
>>>>>     1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>> index e2951b6..13d14d4 100644
>>>>> --- a/arch/x86/kvm/vmx.c
>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm
>>>> *kvm, unsigned int host_irq,
>>>>>     		 */
>>>>>
>>>>>     		kvm_set_msi_irq(e, &irq);
>>>>> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
>>>>> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
>>>>> +			/*
>>>>> +			 * Make sure the IRTE is in remapped mode if
>>>>> +			 * we don't handle it in posted mode.
>>>>> +			 */
>>>>> +			pi_set_sn(vcpu_to_pi_desc(vcpu));
>>>>> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
>>>>> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
>>>>> +
>>>>>     			continue;
>>>>> +		}
>>>>>
>>>>>     		vcpu_info.pi_desc_addr = __pa(vcpu_to_pi_desc(vcpu));
>>>>>     		vcpu_info.vector = irq.vector;
>>>>>
>>>>
>>>> I am still feel weird with this change: according the semantic of VT-d
>>>> posted interrupt, the interrupt will injected to guest through posted
>>>> notification and /proc/interrupts shows the same meaning. But now,
>>>> without being aware of user, the interrupt changes to legacy way and it
>>>> appears on different entry on /proc/interrupts. It looks weird.
>>>
>>> I don't think it has problem here, IMO, this is exactly how it works.
>>> There should be different entry for the interrupts in VT-d PI mode
>>> and leagcy mode.
>>
>> I am not saying any problem here. Just feel weird. From a normal user's
>> point, he has turned on the VT-d pi and according the semantic of VT-d
>> pi, he should not observe the interrupt through legacy mode, but now he
>> do see it. Maybe print out a message here will be helpful, like what you
>> did for disabled lapic found during irq injection.
>
> Even VT-d PI is on, not all interrupts can be handled by it, the reason the

No, we can handle it but we don't do it due to the complexity.For 
example, we can use wake up vector to delivery the interrupt which still 
is in PI mode but doesn't require any mode change.

> interrupts is changed back to legacy mode is because the user changes
> the affinity, and it cannot be handle in PI mode, and hence legacy mode
> is used. It is the user's behavior that cause this mode change, seems it is
> not so weird to me. But add some message here is good idea, just like

Why user's behavior can change the mode? According the current design, 
there is no way for user to turn on/off dynamically.Why we need to 
rollback to legacy mode is we don't want to handle multi-destination 
interrupt in PI mode but it doesn't mean we cannot do it like i said before.


-- 
best regards
yang

RE: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Wu, Feng]

> -----Original Message-----
> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> Sent: Thursday, January 21, 2016 1:00 PM
> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> rkrcmar@redhat.com
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> interrupt is not single-destination
> 
> On 2016/1/21 12:42, Wu, Feng wrote:
> >
> >
> >> -----Original Message-----
> >> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> On
> >> Behalf Of Yang Zhang
> >> Sent: Thursday, January 21, 2016 11:35 AM
> >> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> >> rkrcmar@redhat.com
> >> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> >> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> >> interrupt is not single-destination
> >>
> >> On 2016/1/21 11:14, Wu, Feng wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> >>>> Sent: Thursday, January 21, 2016 11:06 AM
> >>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> >>>> rkrcmar@redhat.com
> >>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> >>>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if
> the
> >>>> interrupt is not single-destination
> >>>>
> >>>> On 2016/1/20 9:42, Feng Wu wrote:
> >>>>> When the interrupt is not single destination any more, we need
> >>>>> to change back IRTE to remapped mode explicitly.
> >>>>>
> >>>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> >>>>> ---
> >>>>>     arch/x86/kvm/vmx.c | 11 ++++++++++-
> >>>>>     1 file changed, 10 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >>>>> index e2951b6..13d14d4 100644
> >>>>> --- a/arch/x86/kvm/vmx.c
> >>>>> +++ b/arch/x86/kvm/vmx.c
> >>>>> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm
> >>>> *kvm, unsigned int host_irq,
> >>>>>     		 */
> >>>>>
> >>>>>     		kvm_set_msi_irq(e, &irq);
> >>>>> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
> >>>>> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> >>>>> +			/*
> >>>>> +			 * Make sure the IRTE is in remapped mode if
> >>>>> +			 * we don't handle it in posted mode.
> >>>>> +			 */
> >>>>> +			pi_set_sn(vcpu_to_pi_desc(vcpu));
> >>>>> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
> >>>>> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
> >>>>> +
> >>>>>     			continue;
> >>>>> +		}
> >>>>>
> >>>>>     		vcpu_info.pi_desc_addr =
> __pa(vcpu_to_pi_desc(vcpu));
> >>>>>     		vcpu_info.vector = irq.vector;
> >>>>>
> >>>>
> >>>> I am still feel weird with this change: according the semantic of VT-d
> >>>> posted interrupt, the interrupt will injected to guest through posted
> >>>> notification and /proc/interrupts shows the same meaning. But now,
> >>>> without being aware of user, the interrupt changes to legacy way and it
> >>>> appears on different entry on /proc/interrupts. It looks weird.
> >>>
> >>> I don't think it has problem here, IMO, this is exactly how it works.
> >>> There should be different entry for the interrupts in VT-d PI mode
> >>> and leagcy mode.
> >>
> >> I am not saying any problem here. Just feel weird. From a normal user's
> >> point, he has turned on the VT-d pi and according the semantic of VT-d
> >> pi, he should not observe the interrupt through legacy mode, but now he
> >> do see it. Maybe print out a message here will be helpful, like what you
> >> did for disabled lapic found during irq injection.
> >
> > Even VT-d PI is on, not all interrupts can be handled by it, the reason the
> 
> No, we can handle it but we don't do it due to the complexity.For
> example, we can use wake up vector to delivery the interrupt which still
> is in PI mode but doesn't require any mode change.

I mean, multi-cast and broadcast interrupts cannot be handled in PI mode.

> 
> > interrupts is changed back to legacy mode is because the user changes
> > the affinity, and it cannot be handle in PI mode, and hence legacy mode
> > is used. It is the user's behavior that cause this mode change, seems it is
> > not so weird to me. But add some message here is good idea, just like
> 
> Why user's behavior can change the mode? 

Like you mentioned before, if the interrupt is changed from single-destination
to multiple-destination by guest. And this is the reason of adding the rollback
logic here, right?

Thanks,
Feng

> According the current design,
> there is no way for user to turn on/off dynamically.Why we need to
> rollback to legacy mode is we don't want to handle multi-destination
> interrupt in PI mode but it doesn't mean we cannot do it like i said before.
> 
> 
> --
> best regards
> yang

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/20 9:42, Feng Wu wrote:
> Use vector-hashing to deliver lowest-priority interrupts, As an
> example, modern Intel CPUs in server platform use this method to
> handle lowest-priority interrupts.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
> v3:
> - Fix a bug for sparse topologies, in that case, vcpu_id is not equal
> to the return value got by kvm_get_vcpu().
> - Remove unnecessary check in fast irq delivery patch.
> - print a error message only once for each guest when we find hardware
>    disabled LAPIC during interrupt injection.
>
>   arch/x86/include/asm/kvm_host.h |  2 ++
>   arch/x86/kvm/irq_comm.c         | 27 +++++++++++++++++----
>   arch/x86/kvm/lapic.c            | 52 ++++++++++++++++++++++++++++++++++++++---
>   arch/x86/kvm/lapic.h            |  2 ++
>   arch/x86/kvm/x86.c              |  9 +++++++
>   arch/x86/kvm/x86.h              |  1 +
>   6 files changed, 85 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 44adbb8..5054810 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -754,6 +754,8 @@ struct kvm_arch {
>
>   	bool irqchip_split;
>   	u8 nr_reserved_ioapic_pins;
> +
> +	int disabled_lapic_found;
>   };
>
>   struct kvm_vm_stat {
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 8fc89ef..062e907 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -34,6 +34,7 @@
>   #include "lapic.h"
>
>   #include "hyperv.h"
> +#include "x86.h"
>
>   static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>   			   struct kvm *kvm, int irq_source_id, int level,
> @@ -55,8 +56,10 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
>   int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>   		struct kvm_lapic_irq *irq, unsigned long *dest_map)
>   {
> -	int i, r = -1;
> +	int i, r = -1, idx = 0;
>   	struct kvm_vcpu *vcpu, *lowest = NULL;
> +	unsigned long dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
> +	unsigned int dest_vcpus = 0;
>
>   	if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
>   			kvm_lowest_prio_delivery(irq)) {
> @@ -67,6 +70,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>   	if (kvm_irq_delivery_to_apic_fast(kvm, src, irq, &r, dest_map))
>   		return r;
>
> +	memset(dest_vcpu_bitmap, 0, sizeof(dest_vcpu_bitmap));
> +
>   	kvm_for_each_vcpu(i, vcpu, kvm) {
>   		if (!kvm_apic_present(vcpu))
>   			continue;
> @@ -80,13 +85,25 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>   				r = 0;
>   			r += kvm_apic_set_irq(vcpu, irq, dest_map);
>   		} else if (kvm_lapic_enabled(vcpu)) {
> -			if (!lowest)
> -				lowest = vcpu;
> -			else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
> -				lowest = vcpu;
> +			if (!kvm_vector_hashing_enabled()) {
> +				if (!lowest)
> +					lowest = vcpu;
> +				else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
> +					lowest = vcpu;
> +			} else {
> +				__set_bit(i, dest_vcpu_bitmap);
> +				dest_vcpus++;
> +			}
>   		}
>   	}
>
> +	if (dest_vcpus != 0) {
> +		idx = kvm_vector_2_index(irq->vector, dest_vcpus,
> +					 dest_vcpu_bitmap, KVM_MAX_VCPUS);
> +
> +		lowest = kvm_get_vcpu(kvm, idx - 1);
> +	}
> +
>   	if (lowest)
>   		r = kvm_apic_set_irq(lowest, irq, dest_map);
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 36591fa..e1a449da 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -675,6 +675,22 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>   	}
>   }
>
> +int kvm_vector_2_index(u32 vector, u32 dest_vcpus,
> +		       const unsigned long *bitmap, u32 bitmap_size)
> +{
> +	u32 mod;
> +	int i, idx = 0;
> +
> +	mod = vector % dest_vcpus;
> +
> +	for (i = 0; i <= mod; i++) {
> +		idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
> +		BUG_ON(idx > bitmap_size);
> +	}
> +
> +	return idx;
> +}
> +
>   bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>   		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
>   {
> @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>
>   		dst = map->logical_map[cid];
>
> -		if (kvm_lowest_prio_delivery(irq)) {
> +		if (!kvm_lowest_prio_delivery(irq))
> +			goto set_irq;
> +
> +		if (!kvm_vector_hashing_enabled()) {
>   			int l = -1;
>   			for_each_set_bit(i, &bitmap, 16) {
>   				if (!dst[i])
>   					continue;
>   				if (l < 0)
>   					l = i;
> -				else if (kvm_apic_compare_prio(dst[i]->vcpu, dst[l]->vcpu) < 0)
> +				else if (kvm_apic_compare_prio(dst[i]->vcpu,
> +							dst[l]->vcpu) < 0)
>   					l = i;
>   			}
> -
>   			bitmap = (l >= 0) ? 1 << l : 0;
> +		} else {
> +			int idx = 0;
> +			unsigned int dest_vcpus = 0;
> +
> +			dest_vcpus = hweight16(bitmap);
> +			if (dest_vcpus == 0)
> +				goto out;
> +
> +			idx = kvm_vector_2_index(irq->vector,
> +				dest_vcpus, &bitmap, 16);
> +
> +			/*
> +			 * We may find a hardware disabled LAPIC here, if that
> +			 * is the case, print out a error message once for each
> +			 * guest and return.
> +			 */
> +			if (!dst[idx-1] &&
> +				(kvm->arch.disabled_lapic_found == 0)) {
> +				kvm->arch.disabled_lapic_found = 1;
> +				printk(KERN_ERR
> +					"Disabled LAPIC found during irq injection\n");
> +				goto out;

What does "goto out" mean? Inject successfully or fail? According the 
value of ret which is set to ture here, it means inject successfully but 
i = -1.

> +			}
> +
> +			bitmap = 0;
> +			__set_bit(idx-1, &bitmap);

We can reuse the code like:
bitmap = (idx > 0) ? 1 << (idx - 1) : 0;

>   		}
>   	}
>
> +set_irq:
>   	for_each_set_bit(i, &bitmap, 16) {
>   		if (!dst[i])
>   			continue;
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 41bdb35..d864601 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -175,4 +175,6 @@ void wait_lapic_expire(struct kvm_vcpu *vcpu);
>
>   bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
>   			struct kvm_vcpu **dest_vcpu);
> +int kvm_vector_2_index(u32 vector, u32 dest_vcpus,
> +		       const unsigned long *bitmap, u32 bitmap_size);
>   #endif
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4244c2b..47daf77 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -123,6 +123,9 @@ module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
>   unsigned int __read_mostly lapic_timer_advance_ns = 0;
>   module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
>
> +bool __read_mostly enable_vector_hashing = 1;
> +module_param(enable_vector_hashing, bool, S_IRUGO);
> +
>   static bool __read_mostly backwards_tsc_observed = false;
>
>   #define KVM_NR_SHARED_MSRS 16
> @@ -8370,6 +8373,12 @@ int kvm_arch_update_irqfd_routing(struct kvm *kvm, unsigned int host_irq,
>   	return kvm_x86_ops->update_pi_irte(kvm, host_irq, guest_irq, set);
>   }
>
> +bool kvm_vector_hashing_enabled(void)
> +{
> +	return enable_vector_hashing;
> +}
> +EXPORT_SYMBOL_GPL(kvm_vector_hashing_enabled);
> +
>   EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_exit);
>   EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_fast_mmio);
>   EXPORT_TRACEPOINT_SYMBOL_GPL(kvm_inj_virq);
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index f2afa5f..04bd0f9 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -179,6 +179,7 @@ int kvm_mtrr_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data);
>   int kvm_mtrr_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata);
>   bool kvm_mtrr_check_gfn_range_consistency(struct kvm_vcpu *vcpu, gfn_t gfn,
>   					  int page_num);
> +bool kvm_vector_hashing_enabled(void);
>
>   #define KVM_SUPPORTED_XCR0     (XFEATURE_MASK_FP | XFEATURE_MASK_SSE \
>   				| XFEATURE_MASK_YMM | XFEATURE_MASK_BNDREGS \
>


-- 
best regards
yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Yang Zhang
> Sent: Thursday, January 21, 2016 1:24 PM
> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> rkrcmar@redhat.com
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> On 2016/1/20 9:42, Feng Wu wrote:
> > Use vector-hashing to deliver lowest-priority interrupts, As an
> > example, modern Intel CPUs in server platform use this method to
> > handle lowest-priority interrupts.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> >   bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic
> *src,
> >   		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
> >   {
> > @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm
> *kvm, struct kvm_lapic *src,
> >
> >   		dst = map->logical_map[cid];
> >
> > -		if (kvm_lowest_prio_delivery(irq)) {
> > +		if (!kvm_lowest_prio_delivery(irq))
> > +			goto set_irq;
> > +
> > +		if (!kvm_vector_hashing_enabled()) {
> >   			int l = -1;
> >   			for_each_set_bit(i, &bitmap, 16) {
> >   				if (!dst[i])
> >   					continue;
> >   				if (l < 0)
> >   					l = i;
> > -				else if (kvm_apic_compare_prio(dst[i]->vcpu,
> dst[l]->vcpu) < 0)
> > +				else if (kvm_apic_compare_prio(dst[i]->vcpu,
> > +							dst[l]->vcpu) < 0)
> >   					l = i;
> >   			}
> > -
> >   			bitmap = (l >= 0) ? 1 << l : 0;
> > +		} else {
> > +			int idx = 0;
> > +			unsigned int dest_vcpus = 0;
> > +
> > +			dest_vcpus = hweight16(bitmap);
> > +			if (dest_vcpus == 0)
> > +				goto out;
> > +
> > +			idx = kvm_vector_2_index(irq->vector,
> > +				dest_vcpus, &bitmap, 16);
> > +
> > +			/*
> > +			 * We may find a hardware disabled LAPIC here, if
> that
> > +			 * is the case, print out a error message once for each
> > +			 * guest and return.
> > +			 */
> > +			if (!dst[idx-1] &&
> > +				(kvm->arch.disabled_lapic_found == 0)) {
> > +				kvm->arch.disabled_lapic_found = 1;
> > +				printk(KERN_ERR
> > +					"Disabled LAPIC found during irq
> injection\n");
> > +				goto out;
> 
> What does "goto out" mean? Inject successfully or fail? According the
> value of ret which is set to ture here, it means inject successfully but
> i = -1.
> 

Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
to false like another function, I should add a "ret = false' here. We should
failed to inject the interrupt since hardware disabled LAPIC is found.

Thanks,
Feng

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Yang Zhang]

On 2016/1/21 13:07, Wu, Feng wrote:
>
>
>> -----Original Message-----
>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>> Sent: Thursday, January 21, 2016 1:00 PM
>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>> rkrcmar@redhat.com
>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
>> interrupt is not single-destination
>>
>> On 2016/1/21 12:42, Wu, Feng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>> On
>>>> Behalf Of Yang Zhang
>>>> Sent: Thursday, January 21, 2016 11:35 AM
>>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>>>> rkrcmar@redhat.com
>>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>>>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
>>>> interrupt is not single-destination
>>>>
>>>> On 2016/1/21 11:14, Wu, Feng wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>>>> Sent: Thursday, January 21, 2016 11:06 AM
>>>>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>>>>>> rkrcmar@redhat.com
>>>>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>>>>>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if
>> the
>>>>>> interrupt is not single-destination
>>>>>>
>>>>>> On 2016/1/20 9:42, Feng Wu wrote:
>>>>>>> When the interrupt is not single destination any more, we need
>>>>>>> to change back IRTE to remapped mode explicitly.
>>>>>>>
>>>>>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>>>>>> ---
>>>>>>>      arch/x86/kvm/vmx.c | 11 ++++++++++-
>>>>>>>      1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>>>> index e2951b6..13d14d4 100644
>>>>>>> --- a/arch/x86/kvm/vmx.c
>>>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>>>> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm
>>>>>> *kvm, unsigned int host_irq,
>>>>>>>      		 */
>>>>>>>
>>>>>>>      		kvm_set_msi_irq(e, &irq);
>>>>>>> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
>>>>>>> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
>>>>>>> +			/*
>>>>>>> +			 * Make sure the IRTE is in remapped mode if
>>>>>>> +			 * we don't handle it in posted mode.
>>>>>>> +			 */
>>>>>>> +			pi_set_sn(vcpu_to_pi_desc(vcpu));
>>>>>>> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
>>>>>>> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
>>>>>>> +
>>>>>>>      			continue;
>>>>>>> +		}
>>>>>>>
>>>>>>>      		vcpu_info.pi_desc_addr =
>> __pa(vcpu_to_pi_desc(vcpu));
>>>>>>>      		vcpu_info.vector = irq.vector;
>>>>>>>
>>>>>>
>>>>>> I am still feel weird with this change: according the semantic of VT-d
>>>>>> posted interrupt, the interrupt will injected to guest through posted
>>>>>> notification and /proc/interrupts shows the same meaning. But now,
>>>>>> without being aware of user, the interrupt changes to legacy way and it
>>>>>> appears on different entry on /proc/interrupts. It looks weird.
>>>>>
>>>>> I don't think it has problem here, IMO, this is exactly how it works.
>>>>> There should be different entry for the interrupts in VT-d PI mode
>>>>> and leagcy mode.
>>>>
>>>> I am not saying any problem here. Just feel weird. From a normal user's
>>>> point, he has turned on the VT-d pi and according the semantic of VT-d
>>>> pi, he should not observe the interrupt through legacy mode, but now he
>>>> do see it. Maybe print out a message here will be helpful, like what you
>>>> did for disabled lapic found during irq injection.
>>>
>>> Even VT-d PI is on, not all interrupts can be handled by it, the reason the
>>
>> No, we can handle it but we don't do it due to the complexity.For
>> example, we can use wake up vector to delivery the interrupt which still
>> is in PI mode but doesn't require any mode change.
>
> I mean, multi-cast and broadcast interrupts cannot be handled in PI mode.

We may have different understanding on PI mode. My understanding is if 
we set the IRTE to PI format, than the subsequent interrupt will be 
handled in PI mode. multi-cast and broadcast interrupts cannot be 
injected to guest directly but it doesn't mean cannot be handled in PI 
mode. As i said, we can handle it in wake up vector or via other 
approach.But it is much complexity.

I agree that rollback to legacy mode is the best choice, but may need 
some additional messages to tell the user(host administrator) why we 
change to legacy mode. I think not all of them are familiar with the 
detail of VT-d PI. If they find there are still some interrupts goto 
legacy mode even they have turned on PI, they may get confused.

>
>>
>>> interrupts is changed back to legacy mode is because the user changes
>>> the affinity, and it cannot be handle in PI mode, and hence legacy mode
>>> is used. It is the user's behavior that cause this mode change, seems it is
>>> not so weird to me. But add some message here is good idea, just like
>>
>> Why user's behavior can change the mode?
>
> Like you mentioned before, if the interrupt is changed from single-destination
> to multiple-destination by guest. And this is the reason of adding the rollback
> logic here, right?

The user means the host administrator.

>
> Thanks,
> Feng
>
>> According the current design,
>> there is no way for user to turn on/off dynamically.Why we need to
>> rollback to legacy mode is we don't want to handle multi-destination
>> interrupt in PI mode but it doesn't mean we cannot do it like i said before.
>>
>>
>> --
>> best regards
>> yang


-- 
best regards
yang

RE: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Wu, Feng]

> -----Original Message-----
> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> Sent: Thursday, January 21, 2016 1:36 PM
> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> rkrcmar@redhat.com
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> interrupt is not single-destination
> 
> On 2016/1/21 13:07, Wu, Feng wrote:
> >
> >
> >> -----Original Message-----
> >> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> >> Sent: Thursday, January 21, 2016 1:00 PM
> >> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> >> rkrcmar@redhat.com
> >> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> >> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> >> interrupt is not single-destination
> >>
> >> On 2016/1/21 12:42, Wu, Feng wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
> >> On
> >>>> Behalf Of Yang Zhang
> >>>> Sent: Thursday, January 21, 2016 11:35 AM
> >>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> >>>> rkrcmar@redhat.com
> >>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> >>>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if
> the
> >>>> interrupt is not single-destination
> >>>>
> >>>> On 2016/1/21 11:14, Wu, Feng wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> >>>>>> Sent: Thursday, January 21, 2016 11:06 AM
> >>>>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> >>>>>> rkrcmar@redhat.com
> >>>>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> >>>>>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if
> >> the
> >>>>>> interrupt is not single-destination
> >>>>>>
> >>>>>> On 2016/1/20 9:42, Feng Wu wrote:
> >>>>>>> When the interrupt is not single destination any more, we need
> >>>>>>> to change back IRTE to remapped mode explicitly.
> >>>>>>>
> >>>>>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> >>>>>>> ---
> >>>>>>>      arch/x86/kvm/vmx.c | 11 ++++++++++-
> >>>>>>>      1 file changed, 10 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >>>>>>> index e2951b6..13d14d4 100644
> >>>>>>> --- a/arch/x86/kvm/vmx.c
> >>>>>>> +++ b/arch/x86/kvm/vmx.c
> >>>>>>> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct
> kvm
> >>>>>> *kvm, unsigned int host_irq,
> >>>>>>>      		 */
> >>>>>>>
> >>>>>>>      		kvm_set_msi_irq(e, &irq);
> >>>>>>> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
> >>>>>>> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> >>>>>>> +			/*
> >>>>>>> +			 * Make sure the IRTE is in remapped mode if
> >>>>>>> +			 * we don't handle it in posted mode.
> >>>>>>> +			 */
> >>>>>>> +			pi_set_sn(vcpu_to_pi_desc(vcpu));
> >>>>>>> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
> >>>>>>> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
> >>>>>>> +
> >>>>>>>      			continue;
> >>>>>>> +		}
> >>>>>>>
> >>>>>>>      		vcpu_info.pi_desc_addr =
> >> __pa(vcpu_to_pi_desc(vcpu));
> >>>>>>>      		vcpu_info.vector = irq.vector;
> >>>>>>>
> >>>>>>
> >>>>>> I am still feel weird with this change: according the semantic of VT-d
> >>>>>> posted interrupt, the interrupt will injected to guest through posted
> >>>>>> notification and /proc/interrupts shows the same meaning. But now,
> >>>>>> without being aware of user, the interrupt changes to legacy way and
> it
> >>>>>> appears on different entry on /proc/interrupts. It looks weird.
> >>>>>
> >>>>> I don't think it has problem here, IMO, this is exactly how it works.
> >>>>> There should be different entry for the interrupts in VT-d PI mode
> >>>>> and leagcy mode.
> >>>>
> >>>> I am not saying any problem here. Just feel weird. From a normal user's
> >>>> point, he has turned on the VT-d pi and according the semantic of VT-d
> >>>> pi, he should not observe the interrupt through legacy mode, but now
> he
> >>>> do see it. Maybe print out a message here will be helpful, like what you
> >>>> did for disabled lapic found during irq injection.
> >>>
> >>> Even VT-d PI is on, not all interrupts can be handled by it, the reason the
> >>
> >> No, we can handle it but we don't do it due to the complexity.For
> >> example, we can use wake up vector to delivery the interrupt which still
> >> is in PI mode but doesn't require any mode change.
> >
> > I mean, multi-cast and broadcast interrupts cannot be handled in PI mode.
> 
> We may have different understanding on PI mode. My understanding is if
> we set the IRTE to PI format, than the subsequent interrupt will be
> handled in PI mode. multi-cast and broadcast interrupts cannot be
> injected to guest directly but it doesn't mean cannot be handled in PI
> mode. As i said, we can handle it in wake up vector or via other
> approach.But it is much complexity.

For the multicast/broastcast, we cannot set the related IRTE in PI
mode, since we cannot set only one destination in IRTE. If an interrupt
is for multiple destination, how can you use VT-d PI to injection it
to all the destinations?

Thanks,
Feng

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/21 13:33, Wu, Feng wrote:
>
>
>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>> owner@vger.kernel.org] On Behalf Of Yang Zhang
>> Sent: Thursday, January 21, 2016 1:24 PM
>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>> rkrcmar@redhat.com
>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
>> priority interrupts
>>
>> On 2016/1/20 9:42, Feng Wu wrote:
>>> Use vector-hashing to deliver lowest-priority interrupts, As an
>>> example, modern Intel CPUs in server platform use this method to
>>> handle lowest-priority interrupts.
>>>
>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>> ---
>>>    bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic
>> *src,
>>>    		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
>>>    {
>>> @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm
>> *kvm, struct kvm_lapic *src,
>>>
>>>    		dst = map->logical_map[cid];
>>>
>>> -		if (kvm_lowest_prio_delivery(irq)) {
>>> +		if (!kvm_lowest_prio_delivery(irq))
>>> +			goto set_irq;
>>> +
>>> +		if (!kvm_vector_hashing_enabled()) {
>>>    			int l = -1;
>>>    			for_each_set_bit(i, &bitmap, 16) {
>>>    				if (!dst[i])
>>>    					continue;
>>>    				if (l < 0)
>>>    					l = i;
>>> -				else if (kvm_apic_compare_prio(dst[i]->vcpu,
>> dst[l]->vcpu) < 0)
>>> +				else if (kvm_apic_compare_prio(dst[i]->vcpu,
>>> +							dst[l]->vcpu) < 0)
>>>    					l = i;
>>>    			}
>>> -
>>>    			bitmap = (l >= 0) ? 1 << l : 0;
>>> +		} else {
>>> +			int idx = 0;
>>> +			unsigned int dest_vcpus = 0;
>>> +
>>> +			dest_vcpus = hweight16(bitmap);
>>> +			if (dest_vcpus == 0)
>>> +				goto out;
>>> +
>>> +			idx = kvm_vector_2_index(irq->vector,
>>> +				dest_vcpus, &bitmap, 16);
>>> +
>>> +			/*
>>> +			 * We may find a hardware disabled LAPIC here, if
>> that
>>> +			 * is the case, print out a error message once for each
>>> +			 * guest and return.
>>> +			 */
>>> +			if (!dst[idx-1] &&
>>> +				(kvm->arch.disabled_lapic_found == 0)) {
>>> +				kvm->arch.disabled_lapic_found = 1;
>>> +				printk(KERN_ERR
>>> +					"Disabled LAPIC found during irq
>> injection\n");
>>> +				goto out;
>>
>> What does "goto out" mean? Inject successfully or fail? According the
>> value of ret which is set to ture here, it means inject successfully but
>> i = -1.
>>
>
> Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
> to false like another function, I should add a "ret = false' here. We should
> failed to inject the interrupt since hardware disabled LAPIC is found.

I remember we have discussed that even the LAPIC is software disabled, 
it still can respond to some interrupts like INIT, NMI, SMI, and SIPI 
messages. Isn't current logic still problematically?

-- 
best regards
yang

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Yang Zhang]

On 2016/1/21 13:41, Wu, Feng wrote:
>
>
>> -----Original Message-----
>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>> Sent: Thursday, January 21, 2016 1:36 PM
>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>> rkrcmar@redhat.com
>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
>> interrupt is not single-destination
>>
>> On 2016/1/21 13:07, Wu, Feng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>> Sent: Thursday, January 21, 2016 1:00 PM
>>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>>>> rkrcmar@redhat.com
>>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>>>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
>>>> interrupt is not single-destination
>>>>
>>>> On 2016/1/21 12:42, Wu, Feng wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org]
>>>> On
>>>>>> Behalf Of Yang Zhang
>>>>>> Sent: Thursday, January 21, 2016 11:35 AM
>>>>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>>>>>> rkrcmar@redhat.com
>>>>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>>>>>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if
>> the
>>>>>> interrupt is not single-destination
>>>>>>
>>>>>> On 2016/1/21 11:14, Wu, Feng wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>>>>>> Sent: Thursday, January 21, 2016 11:06 AM
>>>>>>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>>>>>>>> rkrcmar@redhat.com
>>>>>>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>>>>>>>> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if
>>>> the
>>>>>>>> interrupt is not single-destination
>>>>>>>>
>>>>>>>> On 2016/1/20 9:42, Feng Wu wrote:
>>>>>>>>> When the interrupt is not single destination any more, we need
>>>>>>>>> to change back IRTE to remapped mode explicitly.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>>>>>>>> ---
>>>>>>>>>       arch/x86/kvm/vmx.c | 11 ++++++++++-
>>>>>>>>>       1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>>>>>> index e2951b6..13d14d4 100644
>>>>>>>>> --- a/arch/x86/kvm/vmx.c
>>>>>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>>>>>> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct
>> kvm
>>>>>>>> *kvm, unsigned int host_irq,
>>>>>>>>>       		 */
>>>>>>>>>
>>>>>>>>>       		kvm_set_msi_irq(e, &irq);
>>>>>>>>> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
>>>>>>>>> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
>>>>>>>>> +			/*
>>>>>>>>> +			 * Make sure the IRTE is in remapped mode if
>>>>>>>>> +			 * we don't handle it in posted mode.
>>>>>>>>> +			 */
>>>>>>>>> +			pi_set_sn(vcpu_to_pi_desc(vcpu));
>>>>>>>>> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
>>>>>>>>> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
>>>>>>>>> +
>>>>>>>>>       			continue;
>>>>>>>>> +		}
>>>>>>>>>
>>>>>>>>>       		vcpu_info.pi_desc_addr =
>>>> __pa(vcpu_to_pi_desc(vcpu));
>>>>>>>>>       		vcpu_info.vector = irq.vector;
>>>>>>>>>
>>>>>>>>
>>>>>>>> I am still feel weird with this change: according the semantic of VT-d
>>>>>>>> posted interrupt, the interrupt will injected to guest through posted
>>>>>>>> notification and /proc/interrupts shows the same meaning. But now,
>>>>>>>> without being aware of user, the interrupt changes to legacy way and
>> it
>>>>>>>> appears on different entry on /proc/interrupts. It looks weird.
>>>>>>>
>>>>>>> I don't think it has problem here, IMO, this is exactly how it works.
>>>>>>> There should be different entry for the interrupts in VT-d PI mode
>>>>>>> and leagcy mode.
>>>>>>
>>>>>> I am not saying any problem here. Just feel weird. From a normal user's
>>>>>> point, he has turned on the VT-d pi and according the semantic of VT-d
>>>>>> pi, he should not observe the interrupt through legacy mode, but now
>> he
>>>>>> do see it. Maybe print out a message here will be helpful, like what you
>>>>>> did for disabled lapic found during irq injection.
>>>>>
>>>>> Even VT-d PI is on, not all interrupts can be handled by it, the reason the
>>>>
>>>> No, we can handle it but we don't do it due to the complexity.For
>>>> example, we can use wake up vector to delivery the interrupt which still
>>>> is in PI mode but doesn't require any mode change.
>>>
>>> I mean, multi-cast and broadcast interrupts cannot be handled in PI mode.
>>
>> We may have different understanding on PI mode. My understanding is if
>> we set the IRTE to PI format, than the subsequent interrupt will be
>> handled in PI mode. multi-cast and broadcast interrupts cannot be
>> injected to guest directly but it doesn't mean cannot be handled in PI
>> mode. As i said, we can handle it in wake up vector or via other
>> approach.But it is much complexity.
>
> For the multicast/broastcast, we cannot set the related IRTE in PI
> mode, since we cannot set only one destination in IRTE. If an interrupt
> is for multiple destination, how can you use VT-d PI to injection it
> to all the destinations?

You may still not get my point. Anyway, it doesn't matter. Rollback to 
legacy mode still is the best choice so far.

-- 
best regards
yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -----Original Message-----
> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> Sent: Thursday, January 21, 2016 1:43 PM
> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> rkrcmar@redhat.com
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> On 2016/1/21 13:33, Wu, Feng wrote:
> >
> >
> >> -----Original Message-----
> >> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> >> owner@vger.kernel.org] On Behalf Of Yang Zhang
> >> Sent: Thursday, January 21, 2016 1:24 PM
> >> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> >> rkrcmar@redhat.com
> >> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> >> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver
> lowest-
> >> priority interrupts
> >>
> >> On 2016/1/20 9:42, Feng Wu wrote:
> >>> Use vector-hashing to deliver lowest-priority interrupts, As an
> >>> example, modern Intel CPUs in server platform use this method to
> >>> handle lowest-priority interrupts.
> >>>
> >>> Signed-off-by: Feng Wu <feng.wu@intel.com>
> >>> ---
> >>>    bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic
> >> *src,
> >>>    		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
> >>>    {
> >>> @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm
> >> *kvm, struct kvm_lapic *src,
> >>>
> >>>    		dst = map->logical_map[cid];
> >>>
> >>> -		if (kvm_lowest_prio_delivery(irq)) {
> >>> +		if (!kvm_lowest_prio_delivery(irq))
> >>> +			goto set_irq;
> >>> +
> >>> +		if (!kvm_vector_hashing_enabled()) {
> >>>    			int l = -1;
> >>>    			for_each_set_bit(i, &bitmap, 16) {
> >>>    				if (!dst[i])
> >>>    					continue;
> >>>    				if (l < 0)
> >>>    					l = i;
> >>> -				else if (kvm_apic_compare_prio(dst[i]->vcpu,
> >> dst[l]->vcpu) < 0)
> >>> +				else if (kvm_apic_compare_prio(dst[i]->vcpu,
> >>> +							dst[l]->vcpu) < 0)
> >>>    					l = i;
> >>>    			}
> >>> -
> >>>    			bitmap = (l >= 0) ? 1 << l : 0;
> >>> +		} else {
> >>> +			int idx = 0;
> >>> +			unsigned int dest_vcpus = 0;
> >>> +
> >>> +			dest_vcpus = hweight16(bitmap);
> >>> +			if (dest_vcpus == 0)
> >>> +				goto out;
> >>> +
> >>> +			idx = kvm_vector_2_index(irq->vector,
> >>> +				dest_vcpus, &bitmap, 16);
> >>> +
> >>> +			/*
> >>> +			 * We may find a hardware disabled LAPIC here, if
> >> that
> >>> +			 * is the case, print out a error message once for each
> >>> +			 * guest and return.
> >>> +			 */
> >>> +			if (!dst[idx-1] &&
> >>> +				(kvm->arch.disabled_lapic_found == 0)) {
> >>> +				kvm->arch.disabled_lapic_found = 1;
> >>> +				printk(KERN_ERR
> >>> +					"Disabled LAPIC found during irq
> >> injection\n");
> >>> +				goto out;
> >>
> >> What does "goto out" mean? Inject successfully or fail? According the
> >> value of ret which is set to ture here, it means inject successfully but
> >> i = -1.
> >>
> >
> > Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
> > to false like another function, I should add a "ret = false' here. We should
> > failed to inject the interrupt since hardware disabled LAPIC is found.
> 
> I remember we have discussed that even the LAPIC is software disabled,
> it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
> messages. Isn't current logic still problematically?

I don't think there are problems, here we only cover lowest-priority mode.

Thanks,
Feng

> 
> --
> best regards
> yang

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/21 13:46, Wu, Feng wrote:
>
>
>> -----Original Message-----
>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>> Sent: Thursday, January 21, 2016 1:43 PM
>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>> rkrcmar@redhat.com
>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
>> priority interrupts
>>
>> On 2016/1/21 13:33, Wu, Feng wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>>>> owner@vger.kernel.org] On Behalf Of Yang Zhang
>>>> Sent: Thursday, January 21, 2016 1:24 PM
>>>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>>>> rkrcmar@redhat.com
>>>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>>>> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver
>> lowest-
>>>> priority interrupts
>>>>
>>>> On 2016/1/20 9:42, Feng Wu wrote:
>>>>> Use vector-hashing to deliver lowest-priority interrupts, As an
>>>>> example, modern Intel CPUs in server platform use this method to
>>>>> handle lowest-priority interrupts.
>>>>>
>>>>> Signed-off-by: Feng Wu <feng.wu@intel.com>
>>>>> ---
>>>>>     bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic
>>>> *src,
>>>>>     		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
>>>>>     {
>>>>> @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm
>>>> *kvm, struct kvm_lapic *src,
>>>>>
>>>>>     		dst = map->logical_map[cid];
>>>>>
>>>>> -		if (kvm_lowest_prio_delivery(irq)) {
>>>>> +		if (!kvm_lowest_prio_delivery(irq))
>>>>> +			goto set_irq;
>>>>> +
>>>>> +		if (!kvm_vector_hashing_enabled()) {
>>>>>     			int l = -1;
>>>>>     			for_each_set_bit(i, &bitmap, 16) {
>>>>>     				if (!dst[i])
>>>>>     					continue;
>>>>>     				if (l < 0)
>>>>>     					l = i;
>>>>> -				else if (kvm_apic_compare_prio(dst[i]->vcpu,
>>>> dst[l]->vcpu) < 0)
>>>>> +				else if (kvm_apic_compare_prio(dst[i]->vcpu,
>>>>> +							dst[l]->vcpu) < 0)
>>>>>     					l = i;
>>>>>     			}
>>>>> -
>>>>>     			bitmap = (l >= 0) ? 1 << l : 0;
>>>>> +		} else {
>>>>> +			int idx = 0;
>>>>> +			unsigned int dest_vcpus = 0;
>>>>> +
>>>>> +			dest_vcpus = hweight16(bitmap);
>>>>> +			if (dest_vcpus == 0)
>>>>> +				goto out;
>>>>> +
>>>>> +			idx = kvm_vector_2_index(irq->vector,
>>>>> +				dest_vcpus, &bitmap, 16);
>>>>> +
>>>>> +			/*
>>>>> +			 * We may find a hardware disabled LAPIC here, if
>>>> that
>>>>> +			 * is the case, print out a error message once for each
>>>>> +			 * guest and return.
>>>>> +			 */
>>>>> +			if (!dst[idx-1] &&
>>>>> +				(kvm->arch.disabled_lapic_found == 0)) {
>>>>> +				kvm->arch.disabled_lapic_found = 1;
>>>>> +				printk(KERN_ERR
>>>>> +					"Disabled LAPIC found during irq
>>>> injection\n");
>>>>> +				goto out;
>>>>
>>>> What does "goto out" mean? Inject successfully or fail? According the
>>>> value of ret which is set to ture here, it means inject successfully but
>>>> i = -1.
>>>>
>>>
>>> Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
>>> to false like another function, I should add a "ret = false' here. We should
>>> failed to inject the interrupt since hardware disabled LAPIC is found.
>>
>> I remember we have discussed that even the LAPIC is software disabled,
>> it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
>> messages. Isn't current logic still problematically?
>
> I don't think there are problems, here we only cover lowest-priority mode.

Does Intel SDM said those interrupts cannot be delivered on 
lowest-priority mode?

CC Jun.

Hi Jun,

Do you know whether INIT, NMI, SMI, and SIPI can be delivered through 
lowest-priority mode? I didn't find SDM says no.

-- 
best regards
yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -----Original Message-----
> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
> Sent: Thursday, January 21, 2016 1:58 PM
> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
> rkrcmar@redhat.com
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> >>
> >> I remember we have discussed that even the LAPIC is software disabled,
> >> it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
> >> messages. Isn't current logic still problematically?
> >
> > I don't think there are problems, here we only cover lowest-priority mode.
> 
> Does Intel SDM said those interrupts cannot be delivered on
> lowest-priority mode?

Fixed, Lowest-priority, SMI, NMI, INIT are all "Delivery Mode", once it is
Lowest-priority, it cannot be other type, afaik.

Thanks,
Feng

> 
> CC Jun.
> 
> Hi Jun,
> 
> Do you know whether INIT, NMI, SMI, and SIPI can be delivered through
> lowest-priority mode? I didn't find SDM says no.
> 
> --
> best regards
> yang

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/21 14:02, Wu, Feng wrote:
>
>
>> -----Original Message-----
>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>> Sent: Thursday, January 21, 2016 1:58 PM
>> To: Wu, Feng <feng.wu@intel.com>; pbonzini@redhat.com;
>> rkrcmar@redhat.com
>> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
>> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
>> priority interrupts
>>
>>>>
>>>> I remember we have discussed that even the LAPIC is software disabled,
>>>> it still can respond to some interrupts like INIT, NMI, SMI, and SIPI
>>>> messages. Isn't current logic still problematically?
>>>
>>> I don't think there are problems, here we only cover lowest-priority mode.
>>
>> Does Intel SDM said those interrupts cannot be delivered on
>> lowest-priority mode?
>
> Fixed, Lowest-priority, SMI, NMI, INIT are all "Delivery Mode", once it is
> Lowest-priority, it cannot be other type, afaik.

You are correct, I missed it with physical and logical mode. Also, i 
noticed you have the check at the beginning:

+		if (!kvm_lowest_prio_delivery(irq))
+			goto set_irq;

-- 
best regards
yang

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Radim Krčmář]

2016-01-20 09:42+0800, Feng Wu:
> When the interrupt is not single destination any more, we need
> to change back IRTE to remapped mode explicitly.
> 
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm *kvm, unsigned int host_irq,
> -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
> +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> +			/*
> +			 * Make sure the IRTE is in remapped mode if
> +			 * we don't handle it in posted mode.
> +			 */
> +			pi_set_sn(vcpu_to_pi_desc(vcpu));

What could go wrong if we didn't suppress notifications here?

Thanks.

> +			ret = irq_set_vcpu_affinity(host_irq, NULL);
> +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
> +
>  			continue;
> +		}

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[rkrcmar]

2016-01-21 13:44+0800, Yang Zhang:
> On 2016/1/21 13:41, Wu, Feng wrote:
>>>From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>We may have different understanding on PI mode. My understanding is if
>>>we set the IRTE to PI format, than the subsequent interrupt will be
>>>handled in PI mode. multi-cast and broadcast interrupts cannot be
>>>injected to guest directly but it doesn't mean cannot be handled in PI
>>>mode. As i said, we can handle it in wake up vector or via other
>>>approach.But it is much complexity.

KVM has to intercept the interrupt, so we'd need to trigger a deferred
work from the notification handler to send the multicast.
Reusing existing PI vectors would mean slowing them down, so we should
define a new PI notification vector just for this purpose, which would
be confusing in /proc/interrupts anyway.
On top of that, we'd need to define new PIRR array(s) and create unique
PID for every IRTE, to avoid parsing those PIRR arrays as the vector is
stored in IRTE ... it's going a bit too far, I guess.

>>For the multicast/broastcast, we cannot set the related IRTE in PI
>>mode, since we cannot set only one destination in IRTE. If an interrupt
>>is for multiple destination, how can you use VT-d PI to injection it
>>to all the destinations?
> 
> You may still not get my point. Anyway, it doesn't matter. Rollback to
> legacy mode still is the best choice so far.

I think we can't do much better than we do now.

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[rkrcmar]

2016-01-21 05:33+0000, Wu, Feng:
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>> owner@vger.kernel.org] On Behalf Of Yang Zhang
>> On 2016/1/20 9:42, Feng Wu wrote:
>> > +			/*
>> > +			 * We may find a hardware disabled LAPIC here, if
>> that
>> > +			 * is the case, print out a error message once for each
>> > +			 * guest and return.
>> > +			 */
>> > +			if (!dst[idx-1] &&
>> > +				(kvm->arch.disabled_lapic_found == 0)) {
>> > +				kvm->arch.disabled_lapic_found = 1;
>> > +				printk(KERN_ERR
>> > +					"Disabled LAPIC found during irq
>> injection\n");
>> > +				goto out;
>> 
>> What does "goto out" mean? Inject successfully or fail? According the
>> value of ret which is set to ture here, it means inject successfully but

(true actually means that fast path did the job and slow path isn't
 needed.)

>> i = -1.

(I think there isn't a practical difference between *r=-1 and *r=0.)

> Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
> to false like another function, I should add a "ret = false' here. We should
> failed to inject the interrupt since hardware disabled LAPIC is found.

'ret = true' is the better one.  We know that the interrupt is not
deliverable [1], so there's no point in trying to deliver with the slow
path.  We behave similarly when the interrupt targets a single disabled
APIC.

---
1: Well ... it's possible that slowpath would deliver it thanks to
   different handling of disabled APICs, but it's undefined behavior,
   so it doesn't matter matter if we don't try.

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Radim Krčmář]

2016-01-20 09:42+0800, Feng Wu:
> Use vector-hashing to deliver lowest-priority interrupts, As an
> example, modern Intel CPUs in server platform use this method to
> handle lowest-priority interrupts.
> 
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---

Functionality looks good, so I had a lot of stylistic comments, sorry :)

> v3:
> - Fix a bug for sparse topologies, in that case, vcpu_id is not equal
> to the return value got by kvm_get_vcpu().
> - Remove unnecessary check in fast irq delivery patch.
> - print a error message only once for each guest when we find hardware
>   disabled LAPIC during interrupt injection.
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> @@ -754,6 +754,8 @@ struct kvm_arch {
>  
>  	bool irqchip_split;
>  	u8 nr_reserved_ioapic_pins;
> +
> +	int disabled_lapic_found;

Fits into "bool".

>  };
>  
>  struct kvm_vm_stat {
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -34,6 +34,7 @@
>  #include "lapic.h"
>  
>  #include "hyperv.h"
> +#include "x86.h"
>  
>  static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>  			   struct kvm *kvm, int irq_source_id, int level,
> @@ -55,8 +56,10 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
>  int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  		struct kvm_lapic_irq *irq, unsigned long *dest_map)
>  {
> -	int i, r = -1;
> +	int i, r = -1, idx = 0;

(No need to initialize idx.)

>  	struct kvm_vcpu *vcpu, *lowest = NULL;
> +	unsigned long dest_vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
> +	unsigned int dest_vcpus = 0;
>  
>  	if (irq->dest_mode == 0 && irq->dest_id == 0xff &&
>  			kvm_lowest_prio_delivery(irq)) {
> @@ -80,13 +85,25 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
>  				r = 0;
>  			r += kvm_apic_set_irq(vcpu, irq, dest_map);
>  		} else if (kvm_lapic_enabled(vcpu)) {
> -			if (!lowest)
> -				lowest = vcpu;
> -			else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
> -				lowest = vcpu;
> +			if (!kvm_vector_hashing_enabled()) {
> +				if (!lowest)
> +					lowest = vcpu;
> +				else if (kvm_apic_compare_prio(vcpu, lowest) < 0)
> +					lowest = vcpu;
> +			} else {
> +				__set_bit(i, dest_vcpu_bitmap);
> +				dest_vcpus++;
> +			}
>  		}
>  	}
>  
> +	if (dest_vcpus != 0) {

(I think it's ok to do 'int idx = kvm...')

> +		idx = kvm_vector_2_index(irq->vector, dest_vcpus,
> +					 dest_vcpu_bitmap, KVM_MAX_VCPUS);
> +
> +		lowest = kvm_get_vcpu(kvm, idx - 1);
> +	}
> +
>  	if (lowest)
>  		r = kvm_apic_set_irq(lowest, irq, dest_map);
>  
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> @@ -675,6 +675,22 @@ bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
>  	}
>  }
>  
> +int kvm_vector_2_index(u32 vector, u32 dest_vcpus,

(The "2" in name is inconsistent, other functions use "to".)

> +		       const unsigned long *bitmap, u32 bitmap_size)
> +{
> +	u32 mod;
> +	int i, idx = 0;
> +
> +	mod = vector % dest_vcpus;
> +
> +	for (i = 0; i <= mod; i++) {
> +		idx = find_next_bit(bitmap, bitmap_size, idx) + 1;

I'd remove this "+ 1".  Current users don't check for errors and always
do "- 1".  The new error value could be 'idx = bitmap_size', with u32 as
return type.

> +		BUG_ON(idx > bitmap_size);
> +	}
> +
> +	return idx;
> +}
> +
>  bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
>  {
> @@ -727,21 +743,51 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
>  
>  		dst = map->logical_map[cid];
>  
> -		if (kvm_lowest_prio_delivery(irq)) {
> +		if (!kvm_lowest_prio_delivery(irq))
> +			goto set_irq;
> +
> +		if (!kvm_vector_hashing_enabled()) {
>  			int l = -1;
>  			for_each_set_bit(i, &bitmap, 16) {
>  				if (!dst[i])
>  					continue;
>  				if (l < 0)
>  					l = i;
> -				else if (kvm_apic_compare_prio(dst[i]->vcpu, dst[l]->vcpu) < 0)
> +				else if (kvm_apic_compare_prio(dst[i]->vcpu,
> +							dst[l]->vcpu) < 0)
>  					l = i;
>  			}
> -
>  			bitmap = (l >= 0) ? 1 << l : 0;
> +		} else {
> +			int idx = 0;
> +			unsigned int dest_vcpus = 0;

(No need to zero them.  Compiler will optimize it, but it increases the
 cognitive load on readers.)

> +
> +			dest_vcpus = hweight16(bitmap);
> +			if (dest_vcpus == 0)
> +				goto out;
> +
> +			idx = kvm_vector_2_index(irq->vector,
> +				dest_vcpus, &bitmap, 16);
> +
> +			/*
> +			 * We may find a hardware disabled LAPIC here, if that
> +			 * is the case, print out a error message once for each
> +			 * guest and return.
> +			 */
> +			if (!dst[idx-1] &&
> +				(kvm->arch.disabled_lapic_found == 0)) {

('!kvm->arch.disabled_lapic_found' would make it fit on one line.)

> +				kvm->arch.disabled_lapic_found = 1;
> +				printk(KERN_ERR

KERN_INFO is the maximal applicable level (and the appropriate one).
It's not an error on host side, just a pointer that the guest does
something stupid.

> +					"Disabled LAPIC found during irq injection\n");
> +				goto out;
> +			}
> +
> +			bitmap = 0;
> +			__set_bit(idx-1, &bitmap);
>  		}
>  	}
>  
> +set_irq:
>  	for_each_set_bit(i, &bitmap, 16) {
>  		if (!dst[i])
>  			continue;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> @@ -123,6 +123,9 @@ module_param(tsc_tolerance_ppm, uint, S_IRUGO | S_IWUSR);
>  unsigned int __read_mostly lapic_timer_advance_ns = 0;
>  module_param(lapic_timer_advance_ns, uint, S_IRUGO | S_IWUSR);
>  
> +bool __read_mostly enable_vector_hashing = 1;
> +module_param(enable_vector_hashing, bool, S_IRUGO);

I think the parameter is well described even without "enable" prefix,
thanks to "bool" type.

Re: [PATCH v3 3/4] KVM: x86: Add lowest-priority support for vt-d posted-interrupts

[Radim Krčmář]

2016-01-20 09:42+0800, Feng Wu:
> Use vector-hashing to deliver lowest-priority interrupts for
> VT-d posted-interrupts. This patch extends kvm_intr_is_single_vcpu()
> to support lowest-priority handling.
> 
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
> v3:
> - Remove unnecessary check in fast irq delivery patch
> - print a error message only once for each guest when we find hardware
>   disabled LAPIC during interrupt injection.
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> @@ -1316,8 +1316,8 @@ int x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, u32 size);
>  bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu);
>  bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu);
>  
> -bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
> -			     struct kvm_vcpu **dest_vcpu);
> +bool kvm_intr_can_posting(struct kvm *kvm, struct kvm_lapic_irq *irq,

I prefer the original one;  I think it's better to describe function
than intent in names -- intention should be obvious from its use.

> +			  struct kvm_vcpu **dest_vcpu);
>  
>  void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
>  		     struct kvm_lapic_irq *irq);
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> @@ -300,13 +300,13 @@ out:
>  	return r;
>  }
>  
> -bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
> -			     struct kvm_vcpu **dest_vcpu)
> +bool kvm_intr_can_posting(struct kvm *kvm, struct kvm_lapic_irq *irq,
> +		struct kvm_vcpu **dest_vcpu)
>  {
>  	int i, r = 0;
>  	struct kvm_vcpu *vcpu;
>  
> -	if (kvm_intr_is_single_vcpu_fast(kvm, irq, dest_vcpu))
> +	if (kvm_intr_can_posting_fast(kvm, irq, dest_vcpu))
>  		return true;

There is one pitfall:  xAPIC flat logical broadcast returns false, but
lowest priority is defined for it (practically isn't a broadcast) and
the rest of this function doesn't check for lowest priority, so the
interrupt won't be posted.

We could modify our _fast functions to cover 0xff in flat logical, but
ignoring this case isn't bad either ... it can happen only with 8 VCPU
guests.  I'd just comment that we did it on purpose. :)

>  
>  	kvm_for_each_vcpu(i, vcpu, kvm) {

[Comments for the rest mirror [2/4].]

Re: [PATCH v3 4/4] KVM/VMX: Add host irq information in trace event when updating IRTE for posted interrupts

[Radim Krčmář]

2016-01-20 09:42+0800, Feng Wu:
> Add host irq information in trace event, so we can better understand
> which irq is in posted mode.
> 
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

RE: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Wu, Feng]

> -----Original Message-----
> From: Radim Krčmář [mailto:rkrcmar@redhat.com]
> Sent: Friday, January 22, 2016 12:20 AM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: pbonzini@redhat.com; linux-kernel@vger.kernel.org;
> kvm@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> interrupt is not single-destination
> 
> 2016-01-20 09:42+0800, Feng Wu:
> > When the interrupt is not single destination any more, we need
> > to change back IRTE to remapped mode explicitly.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > @@ -10764,8 +10764,17 @@ static int vmx_update_pi_irte(struct kvm
> *kvm, unsigned int host_irq,
> > -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
> > +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
> > +			/*
> > +			 * Make sure the IRTE is in remapped mode if
> > +			 * we don't handle it in posted mode.
> > +			 */
> > +			pi_set_sn(vcpu_to_pi_desc(vcpu));
> 
> What could go wrong if we didn't suppress notifications here?

This is a good question. I also thought about this before, but after
thinking it a bit more, seems we don't need to do this. 
If we don't do this, the in-flight interrupts will continue to be
delivered in PI mode while we are changing it to remapped
mode in IRTE. Even if we do this, the in-flight interrupts are
also delivered in PI mode before setting 'SN' anyway, so seems
we really don't need this, what is your opinion?

Thanks,
Feng

Thanks,
Feng

> 
> Thanks.
> 
> > +			ret = irq_set_vcpu_affinity(host_irq, NULL);
> > +			pi_clear_sn(vcpu_to_pi_desc(vcpu));
> > +
> >  			continue;
> > +		}

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -----Original Message-----
> From: rkrcmar@redhat.com [mailto:rkrcmar@redhat.com]
> Sent: Friday, January 22, 2016 1:21 AM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: Yang Zhang <yang.zhang.wz@gmail.com>; pbonzini@redhat.com; linux-
> kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts



> > Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
> > to false like another function, I should add a "ret = false' here. We should
> > failed to inject the interrupt since hardware disabled LAPIC is found.
> 
> 'ret = true' is the better one.  We know that the interrupt is not
> deliverable [1], so there's no point in trying to deliver with the slow
> path.  We behave similarly when the interrupt targets a single disabled
> APIC.

Oh, yes, you are right, Thanks a lot!

Thanks,
Feng

> 
> ---
> 1: Well ... it's possible that slowpath would deliver it thanks to
>    different handling of disabled APICs, but it's undefined behavior,
>    so it doesn't matter matter if we don't try.

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Yang Zhang]

On 2016/1/22 0:35, rkrcmar@redhat.com wrote:
> 2016-01-21 13:44+0800, Yang Zhang:
>> On 2016/1/21 13:41, Wu, Feng wrote:
>>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>> We may have different understanding on PI mode. My understanding is if
>>>> we set the IRTE to PI format, than the subsequent interrupt will be
>>>> handled in PI mode. multi-cast and broadcast interrupts cannot be
>>>> injected to guest directly but it doesn't mean cannot be handled in PI
>>>> mode. As i said, we can handle it in wake up vector or via other
>>>> approach.But it is much complexity.
>
> KVM has to intercept the interrupt, so we'd need to trigger a deferred
> work from the notification handler to send the multicast.
> Reusing existing PI vectors would mean slowing them down, so we should
> define a new PI notification vector just for this purpose, which would
> be confusing in /proc/interrupts anyway.
> On top of that, we'd need to define new PIRR array(s) and create unique
> PID for every IRTE, to avoid parsing those PIRR arrays as the vector is
> stored in IRTE ... it's going a bit too far, I guess.

Not so complicated. We can reuse the wake up vector and check whether 
the interrupt is multicast when one of destination vcpu handles it. If 
it is multicast, then also notifies other vcpus. It is totally handed in 
PI mode and we already have the wakeup vector in /proc/interrupts.

>
>>> For the multicast/broastcast, we cannot set the related IRTE in PI
>>> mode, since we cannot set only one destination in IRTE. If an interrupt
>>> is for multiple destination, how can you use VT-d PI to injection it
>>> to all the destinations?
>>
>> You may still not get my point. Anyway, it doesn't matter. Rollback to
>> legacy mode still is the best choice so far.
>
> I think we can't do much better than we do now.



-- 
best regards
yang

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Yang Zhang]

On 2016/1/22 1:21, rkrcmar@redhat.com wrote:
> 2016-01-21 05:33+0000, Wu, Feng:
>>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>>> owner@vger.kernel.org] On Behalf Of Yang Zhang
>>> On 2016/1/20 9:42, Feng Wu wrote:
>>>> +			/*
>>>> +			 * We may find a hardware disabled LAPIC here, if
>>> that
>>>> +			 * is the case, print out a error message once for each
>>>> +			 * guest and return.
>>>> +			 */
>>>> +			if (!dst[idx-1] &&
>>>> +				(kvm->arch.disabled_lapic_found == 0)) {
>>>> +				kvm->arch.disabled_lapic_found = 1;
>>>> +				printk(KERN_ERR
>>>> +					"Disabled LAPIC found during irq
>>> injection\n");
>>>> +				goto out;
>>>
>>> What does "goto out" mean? Inject successfully or fail? According the
>>> value of ret which is set to ture here, it means inject successfully but
>
> (true actually means that fast path did the job and slow path isn't
>   needed.)
>
>>> i = -1.
>
> (I think there isn't a practical difference between *r=-1 and *r=0.)

Currently, if *r == -1, the remote_irr may get set. But it seems wrong. 
I need to have a double check to see whether it is a bug in current code.

>
>> Oh, I didn't notice 'ret' is initialized to true, I thought it was initialized
>> to false like another function, I should add a "ret = false' here. We should
>> failed to inject the interrupt since hardware disabled LAPIC is found.
>
> 'ret = true' is the better one.  We know that the interrupt is not
> deliverable [1], so there's no point in trying to deliver with the slow
> path.  We behave similarly when the interrupt targets a single disabled
> APIC.
>
> ---
> 1: Well ... it's possible that slowpath would deliver it thanks to
>     different handling of disabled APICs, but it's undefined behavior,

why it is undefined behavior? Besides, why we will keep two different 
handling logic for the fast path and slow path? It looks weird.

>     so it doesn't matter matter if we don't try.
>


-- 
best regards
yang

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -----Original Message-----
> From: Radim Krčmář [mailto:rkrcmar@redhat.com]
> Sent: Friday, January 22, 2016 3:50 AM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: pbonzini@redhat.com; linux-kernel@vger.kernel.org;
> kvm@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> 2016-01-20 09:42+0800, Feng Wu:
> > Use vector-hashing to deliver lowest-priority interrupts, As an
> > example, modern Intel CPUs in server platform use this method to
> > handle lowest-priority interrupts.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> 
> Functionality looks good, so I had a lot of stylistic comments, sorry :)

Any comments are welcome! Thank you! :)

> 
> > +		       const unsigned long *bitmap, u32 bitmap_size)
> > +{
> > +	u32 mod;
> > +	int i, idx = 0;
> > +
> > +	mod = vector % dest_vcpus;
> > +
> > +	for (i = 0; i <= mod; i++) {
> > +		idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
> 
> I'd remove this "+ 1".  Current users don't check for errors and always
> do "- 1".  The new error value could be 'idx = bitmap_size', with u32 as
> return type.
> 

Does the following code look good to you:

        u32 mod;
        int i, idx = -1;

        mod = vector % dest_vcpus;

        for (i = 0; i <= mod; i++) {
                idx = find_next_bit(bitmap, bitmap_size, idx + 1);
                BUG_ON(idx == bitmap_size);
        }

        return idx;

Thanks,
Feng

RE: [PATCH v3 3/4] KVM: x86: Add lowest-priority support for vt-d posted-interrupts

[Wu, Feng]

> -----Original Message-----
> From: Radim Krčmář [mailto:rkrcmar@redhat.com]
> Sent: Friday, January 22, 2016 4:17 AM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: pbonzini@redhat.com; linux-kernel@vger.kernel.org;
> kvm@vger.kernel.org
> Subject: Re: [PATCH v3 3/4] KVM: x86: Add lowest-priority support for vt-d
> posted-interrupts
> 
> 2016-01-20 09:42+0800, Feng Wu:
> > Use vector-hashing to deliver lowest-priority interrupts for
> > VT-d posted-interrupts. This patch extends kvm_intr_is_single_vcpu()
> > to support lowest-priority handling.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> > v3:
> > - Remove unnecessary check in fast irq delivery patch
> > - print a error message only once for each guest when we find hardware
> >   disabled LAPIC during interrupt injection.
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> > @@ -1316,8 +1316,8 @@ int x86_set_memory_region(struct kvm *kvm, int
> id, gpa_t gpa, u32 size);
> >  bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu);
> >  bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu);
> >
> > -bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
> > -			     struct kvm_vcpu **dest_vcpu);
> > +bool kvm_intr_can_posting(struct kvm *kvm, struct kvm_lapic_irq *irq,
> 
> I prefer the original one;  I think it's better to describe function
> than intent in names -- intention should be obvious from its use.
> 
> > +			  struct kvm_vcpu **dest_vcpu);
> >
> >  void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
> >  		     struct kvm_lapic_irq *irq);
> > diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> > @@ -300,13 +300,13 @@ out:
> >  	return r;
> >  }
> >
> > -bool kvm_intr_is_single_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
> > -			     struct kvm_vcpu **dest_vcpu)
> > +bool kvm_intr_can_posting(struct kvm *kvm, struct kvm_lapic_irq *irq,
> > +		struct kvm_vcpu **dest_vcpu)
> >  {
> >  	int i, r = 0;
> >  	struct kvm_vcpu *vcpu;
> >
> > -	if (kvm_intr_is_single_vcpu_fast(kvm, irq, dest_vcpu))
> > +	if (kvm_intr_can_posting_fast(kvm, irq, dest_vcpu))
> >  		return true;
> 
> There is one pitfall:  xAPIC flat logical broadcast returns false,

Do you mean kvm_intr_can_posting_fast() returns false for
xAPIC flat logical lowest-priority broadcast?

After carefully read the code for several times, I still cannot
find the reason, could you please give more hints?

BTW, I noticed there is  a "if(irq->dest_id == 0xFF) goto out;" in
this function, but it is for the physical dest mode. I am not
sure you mean this.

> but lowest priority is defined for it (practically isn't a broadcast) and
> the rest of this function doesn't check for lowest priority, so the
> interrupt won't be posted.
> 
> We could modify our _fast functions to cover 0xff in flat logical, but
> ignoring this case isn't bad either ... it can happen only with 8 VCPU
> guests. 

Could you please elaborate a bit more why only for the 8 VCPU guests?
Thanks a lot!

Thanks,
Feng

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Radim Krcmár]

2016-01-22 01:49+0000, Wu, Feng:
>> From: Radim Krčmář [mailto:rkrcmar@redhat.com]
>> 2016-01-20 09:42+0800, Feng Wu:
>> > -		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu))
>> > +		if (!kvm_intr_is_single_vcpu(kvm, &irq, &vcpu)) {
>> > +			/*
>> > +			 * Make sure the IRTE is in remapped mode if
>> > +			 * we don't handle it in posted mode.
>> > +			 */
>> > +			pi_set_sn(vcpu_to_pi_desc(vcpu));
>> 
>> What could go wrong if we didn't suppress notifications here?
> 
> This is a good question. I also thought about this before, but after
> thinking it a bit more, seems we don't need to do this. 
> If we don't do this, the in-flight interrupts will continue to be
> delivered in PI mode while we are changing it to remapped
> mode in IRTE. Even if we do this, the in-flight interrupts are
> also delivered in PI mode before setting 'SN' anyway, so seems
> we really don't need this, what is your opinion?

I'd remove it.

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[rkrcmar]

2016-01-22 10:03+0800, Yang Zhang:
> On 2016/1/22 0:35, rkrcmar@redhat.com wrote:
>>2016-01-21 13:44+0800, Yang Zhang:
>>>On 2016/1/21 13:41, Wu, Feng wrote:
>>>>>From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>>>We may have different understanding on PI mode. My understanding is if
>>>>>we set the IRTE to PI format, than the subsequent interrupt will be
>>>>>handled in PI mode. multi-cast and broadcast interrupts cannot be
>>>>>injected to guest directly but it doesn't mean cannot be handled in PI
>>>>>mode. As i said, we can handle it in wake up vector or via other
>>>>>approach.But it is much complexity.
>>
>>KVM has to intercept the interrupt, so we'd need to trigger a deferred
>>work from the notification handler to send the multicast.
>>Reusing existing PI vectors would mean slowing them down, so we should
>>define a new PI notification vector just for this purpose, which would
>>be confusing in /proc/interrupts anyway.
>>On top of that, we'd need to define new PIRR array(s) and create unique
>>PID for every IRTE, to avoid parsing those PIRR arrays as the vector is
>>stored in IRTE ... it's going a bit too far, I guess.
> 
> Not so complicated. We can reuse the wake up vector and check whether the
> interrupt is multicast when one of destination vcpu handles it.

I'm not sure what you mean now ... I guess it is:
- Deliver the interrupt to a guest VCPU and relay the multicast to other
  VCPUs.  No, it's strictly worse than intercepting it in the host.

- Modify host's wakeup vector handler to send the multicast.
  It's so complicated, because all information you start with in the
  host is a vector number.  You start with no idea what the multicast
  interrupt should be.

  We could add per-multicast PID to the list of parsed PIDs in
  wakeup_handler and use PID->multicast interrupt mapping to tell which
  interrupt we should send, but that seems worse than just delivering a
  non-remapped interrupt.

  Also, if wakeup vector were used for wakeup and multicast, we'd be
  uselessly doing work, because we can't tell which reason triggered the
  interrupt before finishing one part -- using separate vectors for that
  would be a bit nicer.

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[rkrcmar]

2016-01-22 12:00+0800, Yang Zhang:
> On 2016/1/22 1:21, rkrcmar@redhat.com wrote:
>>(I think there isn't a practical difference between *r=-1 and *r=0.)
> 
> Currently, if *r == -1, the remote_irr may get set. But it seems wrong. I

Yeah ...

> need to have a double check to see whether it is a bug in current code.

Looking forward to the patch!

Thanks.

>>'ret = true' is the better one.  We know that the interrupt is not
>>deliverable [1], so there's no point in trying to deliver with the slow
>>path.  We behave similarly when the interrupt targets a single disabled
>>APIC.
>>
>>---
>>1: Well ... it's possible that slowpath would deliver it thanks to
>>    different handling of disabled APICs, but it's undefined behavior,
> 
> why it is undefined behavior? Besides, why we will keep two different
> handling logic for the fast path and slow path? It looks weird.

It does look very weird ... the slow path would require refactoring,
though, so we save effort without a considerable drawback.
(I would love if it behaved identically, but I don't want to force it on
 someone and likely won't do it myself ...)

I consider it undefined because SMD says that an OS musn't configure
this behavior and doesn't say what should happen if the OS does => we
could do anything.  (Killing the guest would be great for debugging OS
issues, but ours behavior is fairly conservative.)

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Radim Krcmár]

2016-01-22 05:12+0000, Wu, Feng:
>> From: Radim Krčmář [mailto:rkrcmar@redhat.com]
>> 2016-01-20 09:42+0800, Feng Wu:
>> > +{
>> > +	u32 mod;
>> > +	int i, idx = 0;
>> > +
>> > +	mod = vector % dest_vcpus;
>> > +
>> > +	for (i = 0; i <= mod; i++) {
>> > +		idx = find_next_bit(bitmap, bitmap_size, idx) + 1;
>> 
>> I'd remove this "+ 1".  Current users don't check for errors and always
>> do "- 1".  The new error value could be 'idx = bitmap_size', with u32 as
>> return type.
>> 
> 
> Does the following code look good to you:
> 
>         u32 mod;
>         int i, idx = -1;
> 
>         mod = vector % dest_vcpus;
> 
>         for (i = 0; i <= mod; i++) {
>                 idx = find_next_bit(bitmap, bitmap_size, idx + 1);
>                 BUG_ON(idx == bitmap_size);
>         }
> 
>         return idx;

It's ok, thanks.

Re: [PATCH v3 3/4] KVM: x86: Add lowest-priority support for vt-d posted-interrupts

[Radim Krcmár]

2016-01-22 05:12+0000, Wu, Feng:
>> From: Radim Krčmář [mailto:rkrcmar@redhat.com]
>> 2016-01-20 09:42+0800, Feng Wu:
>>> -	if (kvm_intr_is_single_vcpu_fast(kvm, irq, dest_vcpu))
>>> +	if (kvm_intr_can_posting_fast(kvm, irq, dest_vcpu))
>>>  		return true;
>> 
>> There is one pitfall:  xAPIC flat logical broadcast returns false,
> 
> Do you mean kvm_intr_can_posting_fast() returns false for
> xAPIC flat logical lowest-priority broadcast?

I did.

> After carefully read the code for several times, I still cannot
> find the reason, could you please give more hints?

You are right, there isn't a problem in the code.

> BTW, I noticed there is  a "if(irq->dest_id == 0xFF) goto out;" in
> this function, but it is for the physical dest mode. I am not
> sure you mean this.

I didn't check if my assumptions were wrong.  I'm sorry.

>> but lowest priority is defined for it (practically isn't a broadcast) and
>> the rest of this function doesn't check for lowest priority, so the
>> interrupt won't be posted.
>> 
>> We could modify our _fast functions to cover 0xff in flat logical, but
>> ignoring this case isn't bad either ... it can happen only with 8 VCPU
>> guests. 
> 
> Could you please elaborate a bit more why only for the 8 VCPU guests?

xAPIC flat logical doesn't forbid lowest priority broadcasts, but lowest
priority delivery still needs to have all destinations enabled, which
can only happen with 8 VCPUs.

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Yang Zhang]

On 2016/1/22 21:31, rkrcmar@redhat.com wrote:
> 2016-01-22 10:03+0800, Yang Zhang:
>> On 2016/1/22 0:35, rkrcmar@redhat.com wrote:
>>> 2016-01-21 13:44+0800, Yang Zhang:
>>>> On 2016/1/21 13:41, Wu, Feng wrote:
>>>>>> From: Yang Zhang [mailto:yang.zhang.wz@gmail.com]
>>>>>> We may have different understanding on PI mode. My understanding is if
>>>>>> we set the IRTE to PI format, than the subsequent interrupt will be
>>>>>> handled in PI mode. multi-cast and broadcast interrupts cannot be
>>>>>> injected to guest directly but it doesn't mean cannot be handled in PI
>>>>>> mode. As i said, we can handle it in wake up vector or via other
>>>>>> approach.But it is much complexity.
>>>
>>> KVM has to intercept the interrupt, so we'd need to trigger a deferred
>>> work from the notification handler to send the multicast.
>>> Reusing existing PI vectors would mean slowing them down, so we should
>>> define a new PI notification vector just for this purpose, which would
>>> be confusing in /proc/interrupts anyway.
>>> On top of that, we'd need to define new PIRR array(s) and create unique
>>> PID for every IRTE, to avoid parsing those PIRR arrays as the vector is
>>> stored in IRTE ... it's going a bit too far, I guess.
>>
>> Not so complicated. We can reuse the wake up vector and check whether the
>> interrupt is multicast when one of destination vcpu handles it.
>
> I'm not sure what you mean now ... I guess it is:
> - Deliver the interrupt to a guest VCPU and relay the multicast to other
>    VCPUs.  No, it's strictly worse than intercepting it in the host.

It is still handled in host context not guest context. The wakeup event 
cannot be consumed like posted event. So it relies on hypervisor to 
inject the interrupt to guest. We can add the check at this point.


>
> - Modify host's wakeup vector handler to send the multicast.
>    It's so complicated, because all information you start with in the
>    host is a vector number.  You start with no idea what the multicast
>    interrupt should be.
>
>    We could add per-multicast PID to the list of parsed PIDs in
>    wakeup_handler and use PID->multicast interrupt mapping to tell which
>    interrupt we should send, but that seems worse than just delivering a
>    non-remapped interrupt.
>
>    Also, if wakeup vector were used for wakeup and multicast, we'd be
>    uselessly doing work, because we can't tell which reason triggered the
>    interrupt before finishing one part -- using separate vectors for that
>    would be a bit nicer.
>


-- 
best regards
yang

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Paolo Bonzini]

On 22/01/2016 14:05, Radim Krcmár wrote:
> > This is a good question. I also thought about this before, but after
> > thinking it a bit more, seems we don't need to do this. 
> > If we don't do this, the in-flight interrupts will continue to be
> > delivered in PI mode while we are changing it to remapped
> > mode in IRTE. Even if we do this, the in-flight interrupts are
> > also delivered in PI mode before setting 'SN' anyway, so seems
> > we really don't need this, what is your opinion?
> I'd remove it.

It may be necessary because IRTE writes (128 bits) are not atomic.

If so, no need to send v5, I'll add it back.

Paolo

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Paolo Bonzini]

On 22/01/2016 15:01, Radim Krcmár wrote:
>>         for (i = 0; i <= mod; i++) {
>>                 idx = find_next_bit(bitmap, bitmap_size, idx + 1);
>>                 BUG_ON(idx == bitmap_size);
>>         }

WARN_ON, not BUG_ON.

Paolo

RE: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Wu, Feng]

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Monday, January 25, 2016 8:23 PM
> To: Radim Krcmár <rkrcmar@redhat.com>; Wu, Feng <feng.wu@intel.com>
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> interrupt is not single-destination
> 
> 
> 
> On 22/01/2016 14:05, Radim Krcmár wrote:
> > > This is a good question. I also thought about this before, but after
> > > thinking it a bit more, seems we don't need to do this.
> > > If we don't do this, the in-flight interrupts will continue to be
> > > delivered in PI mode while we are changing it to remapped
> > > mode in IRTE. Even if we do this, the in-flight interrupts are
> > > also delivered in PI mode before setting 'SN' anyway, so seems
> > > we really don't need this, what is your opinion?
> > I'd remove it.
> 
> It may be necessary because IRTE writes (128 bits) are not atomic.

IRTE is updated atomically, I added the patch to support this. Please
refer to 344cb4e0b6f3a0dbef0643eacb4946338eb228c0.

Thanks,
Feng

> 
> If so, no need to send v5, I'll add it back.
> 
> Paolo

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Paolo Bonzini]

On 25/01/2016 13:26, Wu, Feng wrote:
>> > It may be necessary because IRTE writes (128 bits) are not atomic.
> IRTE is updated atomically, I added the patch to support this. Please
> refer to 344cb4e0b6f3a0dbef0643eacb4946338eb228c0.

Great, I hadn't noticed that patch.  Thanks.

Paolo

RE: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Wu, Feng]

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Monday, January 25, 2016 8:39 PM
> To: Wu, Feng <feng.wu@intel.com>; Radim Krcmár <rkrcmar@redhat.com>
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> interrupt is not single-destination
> 
> 
> 
> On 25/01/2016 13:26, Wu, Feng wrote:
> >> > It may be necessary because IRTE writes (128 bits) are not atomic.
> > IRTE is updated atomically, I added the patch to support this. Please
> > refer to 344cb4e0b6f3a0dbef0643eacb4946338eb228c0.
> 
> Great, I hadn't noticed that patch.  Thanks.

I might forget to cc KVM mailing list, sorry for that!

Thanks,
Feng

> 
> Paolo

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[rkrcmar]

2016-01-25 09:49+0800, Yang Zhang:
> On 2016/1/22 21:31, rkrcmar@redhat.com wrote:
>>2016-01-22 10:03+0800, Yang Zhang:
>>>Not so complicated. We can reuse the wake up vector and check whether the
>>>interrupt is multicast when one of destination vcpu handles it.
>>
>>I'm not sure what you mean now ... I guess it is:
>>- Deliver the interrupt to a guest VCPU and relay the multicast to other
>>   VCPUs.  No, it's strictly worse than intercepting it in the host.
> 
> It is still handled in host context not guest context. The wakeup event
> cannot be consumed like posted event.

Ok.  ("when one of destination vcpu handles it" confused me into
thinking that you'd like to handle it with the notification vector.)

>                                       So it relies on hypervisor to inject
> the interrupt to guest. We can add the check at this point.

Yes, but I don't think we want to do that, because of following
drawbacks:

>>- Modify host's wakeup vector handler to send the multicast.
>>   It's so complicated, because all information you start with in the
>>   host is a vector number.  You start with no idea what the multicast
>>   interrupt should be.
>>
>>   We could add per-multicast PID to the list of parsed PIDs in
>>   wakeup_handler and use PID->multicast interrupt mapping to tell which
>>   interrupt we should send, but that seems worse than just delivering a
>>   non-remapped interrupt.

(should have been "remapped, but non-posted".)

>>   Also, if wakeup vector were used for wakeup and multicast, we'd be
>>   uselessly doing work, because we can't tell which reason triggered the
>>   interrupt before finishing one part -- using separate vectors for that
>>   would be a bit nicer.

(imprecise -- we would always have to check for ON bit of all PIDs from
 blocked VCPUs, for the original meaning of wakeup vector, and always
 either read the PIRR or check for ON bit of all PIDs that encode
 multicast interrupts;  then we have to clear ON bits for multicasts.)


---
There might be a benefit of using posted interrupts for host interrupts
when we run out of free interrupt vectors:  we could start using vectors
by multiple sources through posted interrupts, if using posted
interrupts is the fastest way to distinguish the interrupt source.

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Radim Krcmár]

2016-01-25 12:26+0000, Wu, Feng:
>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
>> It may be necessary because IRTE writes (128 bits) are not atomic.
> 
> IRTE is updated atomically, I added the patch to support this. Please
> refer to 344cb4e0b6f3a0dbef0643eacb4946338eb228c0.

I also think that SN bit is not affected by atomicity: if the IRTE could
have been read half-updated while changing from posted to non-posted,
then it wouldn't point to the correct PID, because its address is not
within 64 bits, so the SN bit wouldn't matter.

IRTE invalidation seems important in VT-d ...

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Radim Krcmár]

2016-01-25 13:25+0100, Paolo Bonzini:
> On 22/01/2016 15:01, Radim Krcmár wrote:
>>>         for (i = 0; i <= mod; i++) {
>>>                 idx = find_next_bit(bitmap, bitmap_size, idx + 1);
>>>                 BUG_ON(idx == bitmap_size);
>>>         }
> 
> WARN_ON, not BUG_ON.

Callers don't check the return value for an error, because every error
is a BUG now.  I think that we should check if we return bitmap_size.
(Current paths could dereference NULL or throw unrelated warnings.)

Also, WARN_ON_ONCE?

Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Paolo Bonzini]

On 25/01/2016 16:20, Radim Krcmár wrote:
> 2016-01-25 13:25+0100, Paolo Bonzini:
>> On 22/01/2016 15:01, Radim Krcmár wrote:
>>>>         for (i = 0; i <= mod; i++) {
>>>>                 idx = find_next_bit(bitmap, bitmap_size, idx + 1);
>>>>                 BUG_ON(idx == bitmap_size);
>>>>         }
>>
>> WARN_ON, not BUG_ON.
> 
> Callers don't check the return value for an error, because every error
> is a BUG now.  I think that we should check if we return bitmap_size.
> (Current paths could dereference NULL or throw unrelated warnings.)

You can probably just return a random number (e.g. zero or
find_first_bit) if the bug is hit.  But really, the bug is easy enough
to verify that BUG_ON might even be okay...

Paolo

RE: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Wu, Feng]

> -----Original Message-----
> From: Radim Krcmár [mailto:rkrcmar@redhat.com]
> Sent: Monday, January 25, 2016 10:06 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>; linux-kernel@vger.kernel.org;
> kvm@vger.kernel.org
> Subject: Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the
> interrupt is not single-destination
> 
> 2016-01-25 12:26+0000, Wu, Feng:
> >> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> >> It may be necessary because IRTE writes (128 bits) are not atomic.
> >
> > IRTE is updated atomically, I added the patch to support this. Please
> > refer to 344cb4e0b6f3a0dbef0643eacb4946338eb228c0.
> 
> I also think that SN bit is not affected by atomicity: if the IRTE could
> have been read half-updated while changing from posted to non-posted,
> then it wouldn't point to the correct PID, because its address is not
> within 64 bits, so the SN bit wouldn't matter.

Yes, like the comments in the commit, we should atomically update the
IRTE in PI case (PI -> non-PI, non-PI -> PI), without which, it cannot
guarantee the correctness, since 'pda' is not within 64 bits, like Radim
pointed out above.

Thanks,
Feng

> 
> IRTE invalidation seems important in VT-d ...

RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts

[Wu, Feng]

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Tuesday, January 26, 2016 12:15 AM
> To: Radim Krcmár <rkrcmar@redhat.com>
> Cc: Wu, Feng <feng.wu@intel.com>; linux-kernel@vger.kernel.org;
> kvm@vger.kernel.org
> Subject: Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-
> priority interrupts
> 
> 
> 
> On 25/01/2016 16:20, Radim Krcmár wrote:
> > 2016-01-25 13:25+0100, Paolo Bonzini:
> >> On 22/01/2016 15:01, Radim Krcmár wrote:
> >>>>         for (i = 0; i <= mod; i++) {
> >>>>                 idx = find_next_bit(bitmap, bitmap_size, idx + 1);
> >>>>                 BUG_ON(idx == bitmap_size);
> >>>>         }
> >>
> >> WARN_ON, not BUG_ON.
> >
> > Callers don't check the return value for an error, because every error
> > is a BUG now.  I think that we should check if we return bitmap_size.
> > (Current paths could dereference NULL or throw unrelated warnings.)
> 
> You can probably just return a random number (e.g. zero or
> find_first_bit) if the bug is hit.  But really, the bug is easy enough
> to verify that BUG_ON might even be okay...

Thanks a lot for your comments, Radim and Paolo! Any other comments
to other patches in this series. If this is the only comments, do I need to
send v5?

Thanks,
Feng

> 
> Paolo

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Yang Zhang]

On 2016/1/25 21:59, rkrcmar@redhat.com wrote:
> 2016-01-25 09:49+0800, Yang Zhang:
>> On 2016/1/22 21:31, rkrcmar@redhat.com wrote:
>>> 2016-01-22 10:03+0800, Yang Zhang:
>>>> Not so complicated. We can reuse the wake up vector and check whether the
>>>> interrupt is multicast when one of destination vcpu handles it.
>>>
>>> I'm not sure what you mean now ... I guess it is:
>>> - Deliver the interrupt to a guest VCPU and relay the multicast to other
>>>    VCPUs.  No, it's strictly worse than intercepting it in the host.
>>
>> It is still handled in host context not guest context. The wakeup event
>> cannot be consumed like posted event.
>
> Ok.  ("when one of destination vcpu handles it" confused me into
> thinking that you'd like to handle it with the notification vector.)

Sorry for my poor english. :(

>
>>                                        So it relies on hypervisor to inject
>> the interrupt to guest. We can add the check at this point.
>
> Yes, but I don't think we want to do that, because of following
> drawbacks:
>
>>> - Modify host's wakeup vector handler to send the multicast.
>>>    It's so complicated, because all information you start with in the
>>>    host is a vector number.  You start with no idea what the multicast
>>>    interrupt should be.
>>>
>>>    We could add per-multicast PID to the list of parsed PIDs in
>>>    wakeup_handler and use PID->multicast interrupt mapping to tell which
>>>    interrupt we should send, but that seems worse than just delivering a
>>>    non-remapped interrupt.
>
> (should have been "remapped, but non-posted".)
>
>>>    Also, if wakeup vector were used for wakeup and multicast, we'd be
>>>    uselessly doing work, because we can't tell which reason triggered the
>>>    interrupt before finishing one part -- using separate vectors for that
>>>    would be a bit nicer.
>
> (imprecise -- we would always have to check for ON bit of all PIDs from
>   blocked VCPUs, for the original meaning of wakeup vector, and always

This is what KVM does currently.

>   either read the PIRR or check for ON bit of all PIDs that encode
>   multicast interrupts;  then we have to clear ON bits for multicasts.)

Also, most part of work is covered by current logic except checking the 
multicast.

>
>
> ---
> There might be a benefit of using posted interrupts for host interrupts
> when we run out of free interrupt vectors:  we could start using vectors
> by multiple sources through posted interrupts, if using posted

Do you mean per vcpu posted interrupts?

> interrupts is the fastest way to distinguish the interrupt source.

-- 
best regards
yang

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[rkrcmar]

2016-01-26 09:44+0800, Yang Zhang:
> On 2016/1/25 21:59, rkrcmar@redhat.com wrote:
>>2016-01-25 09:49+0800, Yang Zhang:
>>>On 2016/1/22 21:31, rkrcmar@redhat.com wrote:
>>>>2016-01-22 10:03+0800, Yang Zhang:
>>>>>Not so complicated. We can reuse the wake up vector and check whether the
>>>>>interrupt is multicast when one of destination vcpu handles it.
>>>>
>>>>I'm not sure what you mean now ... I guess it is:
>>>>- Deliver the interrupt to a guest VCPU and relay the multicast to other
>>>>   VCPUs.  No, it's strictly worse than intercepting it in the host.
>>>
>>>It is still handled in host context not guest context. The wakeup event
>>>cannot be consumed like posted event.
>>
>>Ok.  ("when one of destination vcpu handles it" confused me into
>>thinking that you'd like to handle it with the notification vector.)
> 
> Sorry for my poor english. :(

It's good.  Ambiguity is hard to avoid if a reader doesn't want to
assume only the most likely meaning.

>>>>   Also, if wakeup vector were used for wakeup and multicast, we'd be
>>>>   uselessly doing work, because we can't tell which reason triggered the
>>>>   interrupt before finishing one part -- using separate vectors for that
>>>>   would be a bit nicer.
>>
>>(imprecise -- we would always have to check for ON bit of all PIDs from
>>  blocked VCPUs, for the original meaning of wakeup vector, and always
> 
> This is what KVM does currently.

Yep.

>>  either read the PIRR or check for ON bit of all PIDs that encode
>>  multicast interrupts;  then we have to clear ON bits for multicasts.)
> 
> Also, most part of work is covered by current logic except checking the
> multicast.

We could reuse the setup that gets us to wakeup_handler, but there is
nothing to share in the handler itself.  Sharing a handler means that we
always have to execute both parts.

We must create new PID anyway and compared to the extra work needed for
multicast handling, a new vector + handler is a relatively small code
investment that adds clarity to the design (and performance).

(Taking the vector splitting to the extreme, we'd improve performance if
 we added a vector per assigned device.  That is practically the same as
 non-posted mode, just more complicated.)

>>---
>>There might be a benefit of using posted interrupts for host interrupts
>>when we run out of free interrupt vectors:  we could start using vectors
>>by multiple sources through posted interrupts, if using posted
> 
> Do you mean per vcpu posted interrupts?

I mean using posting for host device interrupts (no virt involved).

Let's say we have 300 devices for one CPU and CPU has 200 useable
vectors.  We have 100 device interrupts that need to be shared in some
vectors and using posting might be faster than directly checking
multiple devices.

(I couldn't come up with a plausible scenario where we might want to use
 posting for host interrupts.)

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[Yang Zhang]

On 2016/1/27 2:22, rkrcmar@redhat.com wrote:
> 2016-01-26 09:44+0800, Yang Zhang:
>> On 2016/1/25 21:59, rkrcmar@redhat.com wrote:
>>> 2016-01-25 09:49+0800, Yang Zhang:
>>>> On 2016/1/22 21:31, rkrcmar@redhat.com wrote:
>>>>> 2016-01-22 10:03+0800, Yang Zhang:
>>>>>> Not so complicated. We can reuse the wake up vector and check whether the
>>>>>> interrupt is multicast when one of destination vcpu handles it.
>>>>>
>>>>> I'm not sure what you mean now ... I guess it is:
>>>>> - Deliver the interrupt to a guest VCPU and relay the multicast to other
>>>>>    VCPUs.  No, it's strictly worse than intercepting it in the host.
>>>>
>>>> It is still handled in host context not guest context. The wakeup event
>>>> cannot be consumed like posted event.
>>>
>>> Ok.  ("when one of destination vcpu handles it" confused me into
>>> thinking that you'd like to handle it with the notification vector.)
>>
>> Sorry for my poor english. :(
>
> It's good.  Ambiguity is hard to avoid if a reader doesn't want to
> assume only the most likely meaning.
>
>>>>>    Also, if wakeup vector were used for wakeup and multicast, we'd be
>>>>>    uselessly doing work, because we can't tell which reason triggered the
>>>>>    interrupt before finishing one part -- using separate vectors for that
>>>>>    would be a bit nicer.
>>>
>>> (imprecise -- we would always have to check for ON bit of all PIDs from
>>>   blocked VCPUs, for the original meaning of wakeup vector, and always
>>
>> This is what KVM does currently.
>
> Yep.
>
>>>   either read the PIRR or check for ON bit of all PIDs that encode
>>>   multicast interrupts;  then we have to clear ON bits for multicasts.)
>>
>> Also, most part of work is covered by current logic except checking the
>> multicast.
>
> We could reuse the setup that gets us to wakeup_handler, but there is
> nothing to share in the handler itself.  Sharing a handler means that we
> always have to execute both parts.

I don't quite understand it. There is nothing need to be modified for 
wakeup logic. The only thing we need to do is add the checking before 
the vcpu pick up the pending interrupt(This is happened in VCPU context, 
not in handler).

>
> We must create new PID anyway and compared to the extra work needed for
> multicast handling, a new vector + handler is a relatively small code
> investment that adds clarity to the design (and performance).

No new PID is needed. If the target vcpu is running, no additional work 
is required in wakeup handler. If target vcpu is not running, the 
current logic will wake up the vcpu, then let vcpu itself to check 
whether pending interrupt is a multicast and handle it in vcpu's context.

>
> (Taking the vector splitting to the extreme, we'd improve performance if
>   we added a vector per assigned device.  That is practically the same as
>   non-posted mode, just more complicated.)
>
>>> ---
>>> There might be a benefit of using posted interrupts for host interrupts
>>> when we run out of free interrupt vectors:  we could start using vectors
>>> by multiple sources through posted interrupts, if using posted
>>
>> Do you mean per vcpu posted interrupts?
>
> I mean using posting for host device interrupts (no virt involved).
>
> Let's say we have 300 devices for one CPU and CPU has 200 useable
> vectors.  We have 100 device interrupts that need to be shared in some
> vectors and using posting might be faster than directly checking
> multiple devices.

Yes, this is an good point.

-- 
best regards
yang

Re: [PATCH v3 1/4] KVM: Recover IRTE to remapped mode if the interrupt is not single-destination

[rkrcmar]

2016-01-27 10:07+0800, Yang Zhang:
> On 2016/1/27 2:22, rkrcmar@redhat.com wrote:
>>2016-01-26 09:44+0800, Yang Zhang:
>>>On 2016/1/25 21:59, rkrcmar@redhat.com wrote:
>>>>>>   Also, if wakeup vector were used for wakeup and multicast, we'd be
>>>>>>   uselessly doing work, because we can't tell which reason triggered the
>>>>>>   interrupt before finishing one part -- using separate vectors for that
>>>>>>   would be a bit nicer.
>>>>
>>>>(imprecise -- we would always have to check for ON bit of all PIDs from
>>>>  blocked VCPUs, for the original meaning of wakeup vector, and always
>>>>  either read the PIRR or check for ON bit of all PIDs that encode
>>>>  multicast interrupts;  then we have to clear ON bits for multicasts.)
>>>
>>>Also, most part of work is covered by current logic except checking the
>>>multicast.
>>
>>We could reuse the setup that gets us to wakeup_handler, but there is
>>nothing to share in the handler itself.  Sharing a handler means that we
>>always have to execute both parts.
> 
> I don't quite understand it. There is nothing need to be modified for wakeup
> logic. The only thing we need to do is add the checking before the vcpu pick
> up the pending interrupt(This is happened in VCPU context, not in handler).

I see, there are few problems with that.

>>We must create new PID anyway and compared to the extra work needed for
>>multicast handling, a new vector + handler is a relatively small code
>>investment that adds clarity to the design (and performance).
> 
> No new PID is needed. If the target vcpu is running, no additional work is
> required in wakeup handler. If target vcpu is not running, the current logic
> will wake up the vcpu, then let vcpu itself to check whether pending
> interrupt is a multicast and handle it in vcpu's context.

We do need a new PID.  The existing VCPU PID switches between wakeup
vector and notification vector, so if the VCPU was running when the
device triggered an interrupt, we'd deliver the posted interrupt without
exiting, but we need to handle the interrupt in the host.

=> We need at least one PID that is never set to notification vector.

Reusing VCPU's PIRR is in new PID(s) is not doable.
Parsing PIRR would be our only option of recognizing multicast
interrupts and if the guest configured many sources to send the same
vector, we'd have to do unacceptable things to tell which one was
triggered.

=> We also need at least on one new PIRR.

Handling the interrupt in VCPU context doesn't pose any advantage and we
even want to do it outside, because all VCPUs can be running when the
interrupt arrives and can therefore be posted further.

I hope I covered other disadvantages of PIDs and PIRRs earlier.