Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1312815 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/4] VT-d posted-interrupts follow ups Feng Wu <feng.wu@intel.com> - 2016-01-20 03:10 +0100
    [PATCH v3 4/4] KVM/VMX: Add host irq information in trace event when updating IRTE for posted interrupts Feng Wu <feng.wu@intel.com> - 2016-01-20 03:10 +0100
      Re: [PATCH v3 4/4] KVM/VMX: Add host irq information in trace event  when updating IRTE for posted interrupts Radim Krčmář <rkrcmar@redhat.com> - 2016-01-21 21:20 +0100
    [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver lowest-priority interrupts Feng Wu <feng.wu@intel.com> - 2016-01-20 03:10 +0100
      Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2016-01-21 06:30 +0100
        RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2016-01-21 06:40 +0100
          RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2016-01-21 06:50 +0100
            Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2016-01-21 07:00 +0100
              Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2016-01-21 07:10 +0100
              RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2016-01-21 07:10 +0100
          Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2016-01-21 06:50 +0100
          Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts "rkrcmar@redhat.com" <rkrcmar@redhat.com> - 2016-01-21 18:30 +0100
            RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2016-01-22 03:10 +0100
            Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Yang Zhang <yang.zhang.wz@gmail.com> - 2016-01-22 05:10 +0100
              Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts "rkrcmar@redhat.com" <rkrcmar@redhat.com> - 2016-01-22 14:50 +0100
      Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Radim Krčmář <rkrcmar@redhat.com> - 2016-01-21 21:00 +0100
        RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2016-01-22 06:20 +0100
          Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Radim Krcmár <rkrcmar@redhat.com> - 2016-01-22 15:10 +0100
            Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Paolo Bonzini <pbonzini@redhat.com> - 2016-01-25 13:30 +0100
              Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Radim Krcmár <rkrcmar@redhat.com> - 2016-01-25 16:30 +0100
                Re: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts Paolo Bonzini <pbonzini@redhat.com> - 2016-01-25 17:20 +0100
                  RE: [PATCH v3 2/4] KVM: x86: Use vector-hashing to deliver  lowest-priority interrupts "Wu, Feng" <feng.wu@intel.com> - 2016-01-26 02:20 +0100

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1313897 — 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>
> ---
> 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

[toc] | [prev] | [next] | [standalone]

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

> -----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

[toc] | [prev] | [next] | [standalone]

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

> -----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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

> -----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

[toc] | [prev] | [next] | [standalone]

#1313930 — 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?

-- 
best regards
yang

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

> -----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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.)

[toc] | [prev] | [next] | [standalone]

#1314475 — 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 :)

> 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.

[toc] | [prev] | [next] | [standalone]

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

> -----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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web