[PATCH v4 0/7] Use eoi to track RTC interrupt delivery status

[PATCH v4 0/7] Use eoi to track RTC interrupt delivery status

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

Current interrupt coalescing logci which only used by RTC has conflict
with Posted Interrupt.

This patch introduces a new mechinism to use eoi to track interrupt:
When delivering an interrupt to vcpu, the need_eoi set to number of
vcpu that received the interrupt. And decrease it when each vcpu writing
eoi. No subsequent RTC interrupt can deliver to vcpu until all vcpus
write eoi.

Changes from v3 to v4
* Call kvm_apic_match_dest() to check destination vcpu.
* Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
  or apic register (id, ldr, dfr) is changed.

Changes from v2 to v3:
* Remove unused viarable irq_ack_notifier.
* Acquire ioapic->lock before calculte destination vcpu map.
* Copy vcpu_map to expected_eoi_timap on each RTC irq and clear it on eoi.
 

Yang Zhang (7):
  KVM: Call kvm_apic_match_dest() to check destination vcpu
  KVM: Call common update function when ioapic entry changed.
  KVM: Add vcpu info to ioapic_update_eoi()
  KVM: Introduce struct rtc_status
  KVM: Recalculate destination vcpu map
  KVM: Add reset/restore rtc_status support
  KVM: Use eoi to track RTC interrupt delivery status

 arch/ia64/kvm/lapic.h    |    6 --
 arch/x86/kvm/lapic.c     |   59 +++---------------
 arch/x86/kvm/lapic.h     |    4 +-
 arch/x86/kvm/vmx.c       |    3 +
 arch/x86/kvm/x86.c       |   11 ++-
 include/linux/kvm_host.h |    4 +-
 virt/kvm/ioapic.c        |  152 ++++++++++++++++++++++++++++++++++++++++------
 virt/kvm/ioapic.h        |   19 ++++--
 virt/kvm/irq_comm.c      |    4 +-
 virt/kvm/kvm_main.c      |    4 +-
 10 files changed, 175 insertions(+), 91 deletions(-)

[PATCH v4 1/7] KVM: Call kvm_apic_match_dest() to check destination vcpu

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

For a given vcpu, kvm_apic_match_dest() will tell you whether
the vcpu in the destination list quickly. Drop kvm_calculate_eoi_exitmap()
and use kvm_apic_match_dest() instead.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 arch/x86/kvm/lapic.c |   47 -----------------------------------------------
 arch/x86/kvm/lapic.h |    4 ----
 virt/kvm/ioapic.c    |    7 +++++--
 3 files changed, 5 insertions(+), 53 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 02b51dd..ee14f94 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -145,53 +145,6 @@ static inline int kvm_apic_id(struct kvm_lapic *apic)
 	return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff;
 }
 
-void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
-				struct kvm_lapic_irq *irq,
-				u64 *eoi_exit_bitmap)
-{
-	struct kvm_lapic **dst;
-	struct kvm_apic_map *map;
-	unsigned long bitmap = 1;
-	int i;
-
-	rcu_read_lock();
-	map = rcu_dereference(vcpu->kvm->arch.apic_map);
-
-	if (unlikely(!map)) {
-		__set_bit(irq->vector, (unsigned long *)eoi_exit_bitmap);
-		goto out;
-	}
-
-	if (irq->dest_mode == 0) { /* physical mode */
-		if (irq->delivery_mode == APIC_DM_LOWEST ||
-				irq->dest_id == 0xff) {
-			__set_bit(irq->vector,
-				  (unsigned long *)eoi_exit_bitmap);
-			goto out;
-		}
-		dst = &map->phys_map[irq->dest_id & 0xff];
-	} else {
-		u32 mda = irq->dest_id << (32 - map->ldr_bits);
-
-		dst = map->logical_map[apic_cluster_id(map, mda)];
-
-		bitmap = apic_logical_id(map, mda);
-	}
-
-	for_each_set_bit(i, &bitmap, 16) {
-		if (!dst[i])
-			continue;
-		if (dst[i]->vcpu == vcpu) {
-			__set_bit(irq->vector,
-				  (unsigned long *)eoi_exit_bitmap);
-			break;
-		}
-	}
-
-out:
-	rcu_read_unlock();
-}
-
 static void recalculate_apic_map(struct kvm *kvm)
 {
 	struct kvm_apic_map *new, *old = NULL;
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 1676d34..5806ef8 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -154,8 +154,4 @@ static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr)
 	return ldr & map->lid_mask;
 }
 
-void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
-				struct kvm_lapic_irq *irq,
-				u64 *eoi_bitmap);
-
 #endif
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index ce82b94..4443075 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -135,8 +135,11 @@ void kvm_ioapic_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
 			irqe.dest_id = e->fields.dest_id;
 			irqe.vector = e->fields.vector;
 			irqe.dest_mode = e->fields.dest_mode;
-			irqe.delivery_mode = e->fields.delivery_mode << 8;
-			kvm_calculate_eoi_exitmap(vcpu, &irqe, eoi_exit_bitmap);
+
+			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
+						irqe.dest_id, irqe.dest_mode))
+				__set_bit(irqe.vector,
+					(unsigned long *)eoi_exit_bitmap);
 		}
 	}
 	spin_unlock(&ioapic->lock);
-- 
1.7.1

[PATCH v4 2/7] KVM: Call common update function when ioapic entry changed.

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

Both RTC irq status and EOI exit bitmap need to be updated when
ioapic changed or vcpu's id/ldr/dfr changed. So use common function
instead eoi exit bitmap specific function.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 arch/ia64/kvm/lapic.h    |    6 ------
 arch/x86/kvm/lapic.c     |    2 +-
 arch/x86/kvm/vmx.c       |    3 +++
 arch/x86/kvm/x86.c       |   11 +++++++----
 include/linux/kvm_host.h |    4 ++--
 virt/kvm/ioapic.c        |   27 ++++++++++++++++-----------
 virt/kvm/ioapic.h        |    7 +++----
 virt/kvm/irq_comm.c      |    4 ++--
 virt/kvm/kvm_main.c      |    4 ++--
 9 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/arch/ia64/kvm/lapic.h b/arch/ia64/kvm/lapic.h
index c3e2935..c5f92a9 100644
--- a/arch/ia64/kvm/lapic.h
+++ b/arch/ia64/kvm/lapic.h
@@ -27,10 +27,4 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq);
 #define kvm_apic_present(x) (true)
 #define kvm_lapic_enabled(x) (true)
 
-static inline bool kvm_apic_vid_enabled(void)
-{
-	/* IA64 has no apicv supporting, do nothing here */
-	return false;
-}
-
 #endif
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index ee14f94..1210866 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -209,7 +209,7 @@ out:
 	if (old)
 		kfree_rcu(old, rcu);
 
-	kvm_ioapic_make_eoibitmap_request(kvm);
+	kvm_vcpu_scan_ioapic(kvm);
 }
 
 static inline void kvm_apic_set_id(struct kvm_lapic *apic, u8 id)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c1b3041..6b25e17 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6362,6 +6362,9 @@ static void vmx_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr)
 
 static void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
 {
+	if (!vmx_vm_has_apicv(vcpu->kvm))
+		return;
+
 	vmcs_write64(EOI_EXIT_BITMAP0, eoi_exit_bitmap[0]);
 	vmcs_write64(EOI_EXIT_BITMAP1, eoi_exit_bitmap[1]);
 	vmcs_write64(EOI_EXIT_BITMAP2, eoi_exit_bitmap[2]);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3c5bb6f..a959d81 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5631,13 +5631,16 @@ static void kvm_gen_update_masterclock(struct kvm *kvm)
 #endif
 }
 
-static void update_eoi_exitmap(struct kvm_vcpu *vcpu)
+static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
 {
 	u64 eoi_exit_bitmap[4];
 
+	if (!kvm_lapic_enabled(vcpu))
+		return;
+
 	memset(eoi_exit_bitmap, 0, 32);
 
-	kvm_ioapic_calculate_eoi_exitmap(vcpu, eoi_exit_bitmap);
+	kvm_ioapic_scan_entry(vcpu, (unsigned long *)eoi_exit_bitmap);
 	kvm_x86_ops->load_eoi_exitmap(vcpu, eoi_exit_bitmap);
 }
 
@@ -5694,8 +5697,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			kvm_handle_pmu_event(vcpu);
 		if (kvm_check_request(KVM_REQ_PMI, vcpu))
 			kvm_deliver_pmi(vcpu);
-		if (kvm_check_request(KVM_REQ_EOIBITMAP, vcpu))
-			update_eoi_exitmap(vcpu);
+		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
+			vcpu_scan_ioapic(vcpu);
 	}
 
 	if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win) {
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 722cae7..9fe6433 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -123,7 +123,7 @@ static inline bool is_error_page(struct page *page)
 #define KVM_REQ_MASTERCLOCK_UPDATE 19
 #define KVM_REQ_MCLOCK_INPROGRESS 20
 #define KVM_REQ_EPR_EXIT          21
-#define KVM_REQ_EOIBITMAP         22
+#define KVM_REQ_SCAN_IOAPIC       22
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID		0
 #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID	1
@@ -538,7 +538,7 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu);
 void kvm_flush_remote_tlbs(struct kvm *kvm);
 void kvm_reload_remote_mmus(struct kvm *kvm);
 void kvm_make_mclock_inprogress_request(struct kvm *kvm);
-void kvm_make_update_eoibitmap_request(struct kvm *kvm);
+void kvm_make_scan_ioapic_request(struct kvm *kvm);
 
 long kvm_arch_dev_ioctl(struct file *filp,
 			unsigned int ioctl, unsigned long arg);
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 4443075..e7c460c 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -116,8 +116,8 @@ static void update_handled_vectors(struct kvm_ioapic *ioapic)
 	smp_wmb();
 }
 
-void kvm_ioapic_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
-					u64 *eoi_exit_bitmap)
+void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
+			unsigned long *eoi_exit_bitmap)
 {
 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
 	union kvm_ioapic_redirect_entry *e;
@@ -125,7 +125,6 @@ void kvm_ioapic_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
 	int index;
 
 	spin_lock(&ioapic->lock);
-	/* traverse ioapic entry to set eoi exit bitmap*/
 	for (index = 0; index < IOAPIC_NUM_PINS; index++) {
 		e = &ioapic->redirtbl[index];
 		if (!e->fields.mask &&
@@ -135,25 +134,31 @@ void kvm_ioapic_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
 			irqe.dest_id = e->fields.dest_id;
 			irqe.vector = e->fields.vector;
 			irqe.dest_mode = e->fields.dest_mode;
+			irqe.shorthand = 0;
 
 			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
 						irqe.dest_id, irqe.dest_mode))
-				__set_bit(irqe.vector,
-					(unsigned long *)eoi_exit_bitmap);
+				__set_bit(irqe.vector, eoi_exit_bitmap);
 		}
 	}
 	spin_unlock(&ioapic->lock);
 }
-EXPORT_SYMBOL_GPL(kvm_ioapic_calculate_eoi_exitmap);
 
-void kvm_ioapic_make_eoibitmap_request(struct kvm *kvm)
+#ifdef CONFIG_X86
+void kvm_vcpu_scan_ioapic(struct kvm *kvm)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 
-	if (!kvm_apic_vid_enabled(kvm) || !ioapic)
+	if (!ioapic)
 		return;
-	kvm_make_update_eoibitmap_request(kvm);
+	kvm_make_scan_ioapic_request(kvm);
 }
+#else
+void kvm_vcpu_scan_ioapic(struct kvm *kvm)
+{
+	return;
+}
+#endif
 
 static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 {
@@ -196,7 +201,7 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 		if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG
 		    && ioapic->irr & (1 << index))
 			ioapic_service(ioapic, index);
-		kvm_ioapic_make_eoibitmap_request(ioapic->kvm);
+		kvm_vcpu_scan_ioapic(ioapic->kvm);
 		break;
 	}
 }
@@ -496,7 +501,7 @@ int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 	spin_lock(&ioapic->lock);
 	memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
 	update_handled_vectors(ioapic);
-	kvm_ioapic_make_eoibitmap_request(kvm);
+	kvm_vcpu_scan_ioapic(kvm);
 	spin_unlock(&ioapic->lock);
 	return 0;
 }
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 0400a46..e3538ba 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -82,9 +82,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		struct kvm_lapic_irq *irq);
 int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
 int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state);
-void kvm_ioapic_make_eoibitmap_request(struct kvm *kvm);
-void kvm_ioapic_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
-					u64 *eoi_exit_bitmap);
-
+void kvm_vcpu_scan_ioapic(struct kvm *kvm);
+void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
+			unsigned long *eoi_exit_bitmap);
 
 #endif
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index ff6d40e..751883c 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -284,7 +284,7 @@ void kvm_register_irq_ack_notifier(struct kvm *kvm,
 	mutex_lock(&kvm->irq_lock);
 	hlist_add_head_rcu(&kian->link, &kvm->irq_ack_notifier_list);
 	mutex_unlock(&kvm->irq_lock);
-	kvm_ioapic_make_eoibitmap_request(kvm);
+	kvm_vcpu_scan_ioapic(kvm);
 }
 
 void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
@@ -294,7 +294,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
 	hlist_del_init_rcu(&kian->link);
 	mutex_unlock(&kvm->irq_lock);
 	synchronize_rcu();
-	kvm_ioapic_make_eoibitmap_request(kvm);
+	kvm_vcpu_scan_ioapic(kvm);
 }
 
 int kvm_request_irq_source_id(struct kvm *kvm)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index adc68fe..2481820 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -217,9 +217,9 @@ void kvm_make_mclock_inprogress_request(struct kvm *kvm)
 	make_all_cpus_request(kvm, KVM_REQ_MCLOCK_INPROGRESS);
 }
 
-void kvm_make_update_eoibitmap_request(struct kvm *kvm)
+void kvm_make_scan_ioapic_request(struct kvm *kvm)
 {
-	make_all_cpus_request(kvm, KVM_REQ_EOIBITMAP);
+	make_all_cpus_request(kvm, KVM_REQ_SCAN_IOAPIC);
 }
 
 int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
-- 
1.7.1

[PATCH v4 3/7] KVM: Add vcpu info to ioapic_update_eoi()

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

Add vcpu info to ioapic_update_eoi, so we can know which vcpu
issued this EOI.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 arch/x86/kvm/lapic.c |    2 +-
 virt/kvm/ioapic.c    |   12 ++++++------
 virt/kvm/ioapic.h    |    3 ++-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 1210866..ed80244 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -735,7 +735,7 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
 			trigger_mode = IOAPIC_LEVEL_TRIG;
 		else
 			trigger_mode = IOAPIC_EDGE_TRIG;
-		kvm_ioapic_update_eoi(apic->vcpu->kvm, vector, trigger_mode);
+		kvm_ioapic_update_eoi(apic->vcpu, vector, trigger_mode);
 	}
 }
 
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index e7c460c..ddf9414 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -272,8 +272,8 @@ void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id)
 	spin_unlock(&ioapic->lock);
 }
 
-static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
-				     int trigger_mode)
+static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
+			struct kvm_ioapic *ioapic, int vector, int trigger_mode)
 {
 	int i;
 
@@ -312,12 +312,12 @@ bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector)
 	return test_bit(vector, ioapic->handled_vectors);
 }
 
-void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode)
+void kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, int vector, int trigger_mode)
 {
-	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
+	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
 
 	spin_lock(&ioapic->lock);
-	__kvm_ioapic_update_eoi(ioapic, vector, trigger_mode);
+	__kvm_ioapic_update_eoi(vcpu, ioapic, vector, trigger_mode);
 	spin_unlock(&ioapic->lock);
 }
 
@@ -415,7 +415,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 		break;
 #ifdef	CONFIG_IA64
 	case IOAPIC_REG_EOI:
-		__kvm_ioapic_update_eoi(ioapic, data, IOAPIC_LEVEL_TRIG);
+		__kvm_ioapic_update_eoi(NULL, ioapic, data, IOAPIC_LEVEL_TRIG);
 		break;
 #endif
 
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index e3538ba..1f16088 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -70,7 +70,8 @@ static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm)
 int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 		int short_hand, int dest, int dest_mode);
 int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
-void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
+void kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu, int vector,
+			int trigger_mode);
 bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
 int kvm_ioapic_init(struct kvm *kvm);
 void kvm_ioapic_destroy(struct kvm *kvm);
-- 
1.7.1

[PATCH v4 4/7] KVM: Introduce struct rtc_status

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 virt/kvm/ioapic.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 1f16088..b727657 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -34,6 +34,12 @@ struct kvm_vcpu;
 #define	IOAPIC_INIT			0x5
 #define	IOAPIC_EXTINT			0x7
 
+struct rtc_status {
+	int need_eoi;
+	DECLARE_BITMAP(vcpu_map, KVM_MAX_VCPUS);
+	DECLARE_BITMAP(expected_eoi_bitmap, KVM_MAX_VCPUS);
+};
+
 struct kvm_ioapic {
 	u64 base_address;
 	u32 ioregsel;
@@ -47,6 +53,9 @@ struct kvm_ioapic {
 	void (*ack_notifier)(void *opaque, int irq);
 	spinlock_t lock;
 	DECLARE_BITMAP(handled_vectors, 256);
+#ifdef CONFIG_X86
+	struct rtc_status rtc_status;
+#endif
 };
 
 #ifdef DEBUG
-- 
1.7.1

[PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
or apic register (id, ldr, dfr) is changed.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 virt/kvm/ioapic.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index ddf9414..91b4c08 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
 {
 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
 	union kvm_ioapic_redirect_entry *e;
+	unsigned long *rtc_map = ioapic->rtc_status.vcpu_map;
 	struct kvm_lapic_irq irqe;
 	int index;
 
@@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
 		if (!e->fields.mask &&
 			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
 			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
-				 index))) {
+				 index) || index == 8)) {
 			irqe.dest_id = e->fields.dest_id;
 			irqe.vector = e->fields.vector;
 			irqe.dest_mode = e->fields.dest_mode;
 			irqe.shorthand = 0;
 
 			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
-						irqe.dest_id, irqe.dest_mode))
+						irqe.dest_id, irqe.dest_mode)) {
 				__set_bit(irqe.vector, eoi_exit_bitmap);
+				if (index == 8)
+					__set_bit(vcpu->vcpu_id, rtc_map);
+			} else if (index == 8)
+				__clear_bit(vcpu->vcpu_id, rtc_map);
 		}
 	}
 	spin_unlock(&ioapic->lock);
-- 
1.7.1

[PATCH v4 6/7] KVM: Add reset/restore rtc_status support

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

reset/restore rtc_status when ioapic reset/restore.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 arch/x86/kvm/lapic.c |    8 ++++++++
 arch/x86/kvm/lapic.h |    2 ++
 virt/kvm/ioapic.c    |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index ed80244..e95312e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -94,6 +94,14 @@ static inline int apic_test_vector(int vec, void *bitmap)
 	return test_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
 }
 
+bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	return apic_test_vector(vector, apic->regs + APIC_ISR) ||
+		apic_test_vector(vector, apic->regs + APIC_IRR);
+}
+
 static inline void apic_set_vector(int vec, void *bitmap)
 {
 	set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 5806ef8..38815b0 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -154,4 +154,6 @@ static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr)
 	return ldr & map->lid_mask;
 }
 
+bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector);
+
 #endif
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 91b4c08..2b35123 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -158,11 +158,43 @@ void kvm_vcpu_scan_ioapic(struct kvm *kvm)
 		return;
 	kvm_make_scan_ioapic_request(kvm);
 }
+
+static void rtc_irq_reset(struct kvm_ioapic *ioapic)
+{
+	ioapic->rtc_status.need_eoi = 0;
+	bitmap_zero(ioapic->rtc_status.vcpu_map, KVM_MAX_VCPUS);
+}
+
+static void rtc_irq_restore(struct kvm_ioapic *ioapic)
+{
+	struct kvm_vcpu *vcpu;
+	int vector, i, need_eoi = 0, rtc_pin = 8;
+
+	vector = ioapic->redirtbl[rtc_pin].fields.vector;
+	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
+		if (kvm_apic_pending_eoi(vcpu, vector)) {
+			need_eoi++;
+			set_bit(vcpu->vcpu_id,
+				ioapic->rtc_status.expected_eoi_bitmap);
+		}
+	}
+	ioapic->rtc_status.need_eoi = need_eoi;
+}
 #else
 void kvm_vcpu_scan_ioapic(struct kvm *kvm)
 {
 	return;
 }
+
+static void rtc_irq_reset(struct kvm_ioapic *ioapic)
+{
+	return;
+}
+
+static void rtc_irq_restore(struct kvm_ioapic *ioapic)
+{
+	return;
+}
 #endif
 
 static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
@@ -441,6 +473,7 @@ void kvm_ioapic_reset(struct kvm_ioapic *ioapic)
 	ioapic->ioregsel = 0;
 	ioapic->irr = 0;
 	ioapic->id = 0;
+	rtc_irq_reset(ioapic);
 	update_handled_vectors(ioapic);
 }
 
@@ -506,6 +539,7 @@ int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 	spin_lock(&ioapic->lock);
 	memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
 	update_handled_vectors(ioapic);
+	rtc_irq_restore(ioapic);
 	kvm_vcpu_scan_ioapic(kvm);
 	spin_unlock(&ioapic->lock);
 	return 0;
-- 
1.7.1

[PATCH v4 7/7] KVM: Use eoi to track RTC interrupt delivery status

[Yang Zhang]

From: Yang Zhang <yang.z.zhang@Intel.com>

Current interrupt coalescing logci which only used by RTC has conflict
with Posted Interrupt.
This patch introduces a new mechinism to use eoi to track interrupt:
When delivering an interrupt to vcpu, the need_eoi set to number of
vcpu that received the interrupt. And decrease it when each vcpu writing
eoi. No subsequent RTC interrupt can deliver to vcpu until all vcpus
write eoi.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 virt/kvm/ioapic.c |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 2b35123..2165f5a 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -180,6 +180,50 @@ static void rtc_irq_restore(struct kvm_ioapic *ioapic)
 	}
 	ioapic->rtc_status.need_eoi = need_eoi;
 }
+
+static void rtc_irq_set_eoi(struct kvm_ioapic *ioapic, int irq)
+{
+	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
+
+	if (irq != 8)
+		return;
+
+	if (likely(!bitmap_empty(ioapic->rtc_status.vcpu_map, KVM_MAX_VCPUS))) {
+		if (entry->fields.delivery_mode == APIC_DM_LOWEST)
+			ioapic->rtc_status.need_eoi = 1;
+		else {
+			int weight;
+			weight = bitmap_weight(ioapic->rtc_status.vcpu_map,
+					sizeof(ioapic->rtc_status.vcpu_map));
+			ioapic->rtc_status.need_eoi = weight;
+		}
+		bitmap_copy(ioapic->rtc_status.expected_eoi_bitmap,
+				ioapic->rtc_status.vcpu_map, KVM_MAX_VCPUS);
+	}
+}
+
+static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
+			struct rtc_status *rtc_status, int irq)
+{
+	if (irq != 8)
+		return;
+
+	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->expected_eoi_bitmap))
+		--rtc_status->need_eoi;
+
+	WARN_ON(rtc_status->need_eoi < 0);
+}
+
+static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
+{
+	if (irq != 8)
+		return false;
+
+	if (ioapic->rtc_status.need_eoi > 0)
+		return true; /* coalesced */
+
+	return false;
+}
 #else
 void kvm_vcpu_scan_ioapic(struct kvm *kvm)
 {
@@ -195,6 +239,22 @@ static void rtc_irq_restore(struct kvm_ioapic *ioapic)
 {
 	return;
 }
+
+static void rtc_irq_set_eoi(struct kvm_ioapic *ioapic, int irq)
+{
+	return;
+}
+
+static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
+			struct rtc_status *rtc_status, int irq)
+{
+	return;
+}
+
+static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
+{
+	return false;
+}
 #endif
 
 static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
@@ -262,6 +322,8 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 	irqe.level = 1;
 	irqe.shorthand = 0;
 
+	rtc_irq_set_eoi(ioapic, irq);
+
 	return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
 }
 
@@ -286,6 +348,11 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
 		ret = 1;
 	} else {
 		int edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
+
+		if (rtc_irq_check(ioapic, irq)) {
+			ret = 0; /* coalesced */
+			goto out;
+		}
 		ioapic->irr |= mask;
 		if ((edge && old_irr != ioapic->irr) ||
 		    (!edge && !entry.fields.remote_irr))
@@ -293,6 +360,7 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
 		else
 			ret = 0; /* report coalesced interrupt */
 	}
+out:
 	trace_kvm_ioapic_set_irq(entry.bits, irq, ret == 0);
 	spin_unlock(&ioapic->lock);
 
@@ -320,6 +388,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
 		if (ent->fields.vector != vector)
 			continue;
 
+		rtc_irq_ack_eoi(vcpu, &ioapic->rtc_status, i);
 		/*
 		 * We are dropping lock while calling ack notifiers because ack
 		 * notifier callbacks for assigned devices call into IOAPIC
-- 
1.7.1

Re: [PATCH v4 1/7] KVM: Call kvm_apic_match_dest() to check destination vcpu

[Gleb Natapov]

On Wed, Mar 20, 2013 at 07:36:13PM +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> For a given vcpu, kvm_apic_match_dest() will tell you whether
> the vcpu in the destination list quickly. Drop kvm_calculate_eoi_exitmap()
> and use kvm_apic_match_dest() instead.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  arch/x86/kvm/lapic.c |   47 -----------------------------------------------
>  arch/x86/kvm/lapic.h |    4 ----
>  virt/kvm/ioapic.c    |    7 +++++--
>  3 files changed, 5 insertions(+), 53 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 02b51dd..ee14f94 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -145,53 +145,6 @@ static inline int kvm_apic_id(struct kvm_lapic *apic)
>  	return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff;
>  }
>  
> -void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
> -				struct kvm_lapic_irq *irq,
> -				u64 *eoi_exit_bitmap)
> -{
> -	struct kvm_lapic **dst;
> -	struct kvm_apic_map *map;
> -	unsigned long bitmap = 1;
> -	int i;
> -
> -	rcu_read_lock();
> -	map = rcu_dereference(vcpu->kvm->arch.apic_map);
> -
> -	if (unlikely(!map)) {
> -		__set_bit(irq->vector, (unsigned long *)eoi_exit_bitmap);
> -		goto out;
> -	}
> -
> -	if (irq->dest_mode == 0) { /* physical mode */
> -		if (irq->delivery_mode == APIC_DM_LOWEST ||
> -				irq->dest_id == 0xff) {
> -			__set_bit(irq->vector,
> -				  (unsigned long *)eoi_exit_bitmap);
> -			goto out;
> -		}
> -		dst = &map->phys_map[irq->dest_id & 0xff];
> -	} else {
> -		u32 mda = irq->dest_id << (32 - map->ldr_bits);
> -
> -		dst = map->logical_map[apic_cluster_id(map, mda)];
> -
> -		bitmap = apic_logical_id(map, mda);
> -	}
> -
> -	for_each_set_bit(i, &bitmap, 16) {
> -		if (!dst[i])
> -			continue;
> -		if (dst[i]->vcpu == vcpu) {
> -			__set_bit(irq->vector,
> -				  (unsigned long *)eoi_exit_bitmap);
> -			break;
> -		}
> -	}
> -
> -out:
> -	rcu_read_unlock();
> -}
> -
>  static void recalculate_apic_map(struct kvm *kvm)
>  {
>  	struct kvm_apic_map *new, *old = NULL;
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 1676d34..5806ef8 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -154,8 +154,4 @@ static inline u16 apic_logical_id(struct kvm_apic_map *map, u32 ldr)
>  	return ldr & map->lid_mask;
>  }
>  
> -void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
> -				struct kvm_lapic_irq *irq,
> -				u64 *eoi_bitmap);
> -
>  #endif
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index ce82b94..4443075 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -135,8 +135,11 @@ void kvm_ioapic_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
>  			irqe.dest_id = e->fields.dest_id;
>  			irqe.vector = e->fields.vector;
>  			irqe.dest_mode = e->fields.dest_mode;
> -			irqe.delivery_mode = e->fields.delivery_mode << 8;
> -			kvm_calculate_eoi_exitmap(vcpu, &irqe, eoi_exit_bitmap);
> +
> +			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
> +						irqe.dest_id, irqe.dest_mode))
> +				__set_bit(irqe.vector,
> +					(unsigned long *)eoi_exit_bitmap);
Nice! But you do not need irqe any more.

>  		}
>  	}
>  	spin_unlock(&ioapic->lock);
> -- 
> 1.7.1

--
			Gleb.

Re: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Gleb Natapov]

On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
> or apic register (id, ldr, dfr) is changed.
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  virt/kvm/ioapic.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index ddf9414..91b4c08 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
>  {
>  	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
>  	union kvm_ioapic_redirect_entry *e;
> +	unsigned long *rtc_map = ioapic->rtc_status.vcpu_map;
>  	struct kvm_lapic_irq irqe;
>  	int index;
>  
> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
>  		if (!e->fields.mask &&
>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
>  			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
> -				 index))) {
> +				 index) || index == 8)) {
>  			irqe.dest_id = e->fields.dest_id;
>  			irqe.vector = e->fields.vector;
>  			irqe.dest_mode = e->fields.dest_mode;
>  			irqe.shorthand = 0;
>  
>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
> -						irqe.dest_id, irqe.dest_mode))
> +						irqe.dest_id, irqe.dest_mode)) {
>  				__set_bit(irqe.vector, eoi_exit_bitmap);
> +				if (index == 8)
> +					__set_bit(vcpu->vcpu_id, rtc_map);
> +			} else if (index == 8)
> +				__clear_bit(vcpu->vcpu_id, rtc_map);
rtc_map bitmap is accessed from different vcpus simultaneously so access
has to be atomic. We also have a race:

vcpu0                               iothread
ioapic config changes
request scan ioapic
                                     inject rtc interrupt
                                     use old vcpu mask
scan_ioapic()
recalculate vcpu mask

So this approach (suggested by me :() will not work.

Need to think about it some more. May be your idea of building a bitmap
while injecting the interrupt is the way to go indeed: pass a pointer to
a bitmap to kvm_irq_delivery_to_apic() and build it there. Pass NULL
pointer if caller does not need to track vcpus.

--
			Gleb.

RE: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-20:
> On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
>> or apic register (id, ldr, dfr) is changed.
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  virt/kvm/ioapic.c |    9 +++++++--
>>  1 files changed, 7 insertions(+), 2 deletions(-)
>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>> index ddf9414..91b4c08 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
>>  { 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; 	union
>>  kvm_ioapic_redirect_entry *e; +	unsigned long *rtc_map =
>>  ioapic->rtc_status.vcpu_map; 	struct kvm_lapic_irq irqe; 	int index;
>> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
>>  		if (!e->fields.mask &&
>>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
>>  			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
>> -				 index))) {
>> +				 index) || index == 8)) {
>>  			irqe.dest_id = e->fields.dest_id;
>>  			irqe.vector = e->fields.vector;
>>  			irqe.dest_mode = e->fields.dest_mode;
>>  			irqe.shorthand = 0;
>>  
>>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
>> -						irqe.dest_id, irqe.dest_mode))
>> +						irqe.dest_id, irqe.dest_mode)) {
>>  				__set_bit(irqe.vector, eoi_exit_bitmap);
>> +				if (index == 8)
>> +					__set_bit(vcpu->vcpu_id, rtc_map);
>> +			} else if (index == 8)
>> +				__clear_bit(vcpu->vcpu_id, rtc_map);
> rtc_map bitmap is accessed from different vcpus simultaneously so access
> has to be atomic. We also have a race:
> 
> vcpu0                               iothread
> ioapic config changes
> request scan ioapic
>                                      inject rtc interrupt
>                                      use old vcpu mask
> scan_ioapic()
> recalculate vcpu mask
> 
> So this approach (suggested by me :() will not work.
> 
> Need to think about it some more. May be your idea of building a bitmap
> while injecting the interrupt is the way to go indeed: pass a pointer to
> a bitmap to kvm_irq_delivery_to_apic() and build it there. Pass NULL
> pointer if caller does not need to track vcpus.
Or, we can block inject rtc interrupt during recalculate vcpu map.

if(need_eoi > 0 && in_recalculating)
return coalesced

Best regards,
Yang

Re: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Gleb Natapov]

On Thu, Mar 21, 2013 at 03:42:46AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-20:
> > On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
> >> From: Yang Zhang <yang.z.zhang@Intel.com>
> >> 
> >> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
> >> or apic register (id, ldr, dfr) is changed.
> >> 
> >> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >> ---
> >>  virt/kvm/ioapic.c |    9 +++++++--
> >>  1 files changed, 7 insertions(+), 2 deletions(-)
> >> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> >> index ddf9414..91b4c08 100644
> >> --- a/virt/kvm/ioapic.c
> >> +++ b/virt/kvm/ioapic.c
> >> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
> >>  { 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; 	union
> >>  kvm_ioapic_redirect_entry *e; +	unsigned long *rtc_map =
> >>  ioapic->rtc_status.vcpu_map; 	struct kvm_lapic_irq irqe; 	int index;
> >> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
> >>  		if (!e->fields.mask &&
> >>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
> >>  			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
> >> -				 index))) {
> >> +				 index) || index == 8)) {
> >>  			irqe.dest_id = e->fields.dest_id;
> >>  			irqe.vector = e->fields.vector;
> >>  			irqe.dest_mode = e->fields.dest_mode;
> >>  			irqe.shorthand = 0;
> >>  
> >>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
> >> -						irqe.dest_id, irqe.dest_mode))
> >> +						irqe.dest_id, irqe.dest_mode)) {
> >>  				__set_bit(irqe.vector, eoi_exit_bitmap);
> >> +				if (index == 8)
> >> +					__set_bit(vcpu->vcpu_id, rtc_map);
> >> +			} else if (index == 8)
> >> +				__clear_bit(vcpu->vcpu_id, rtc_map);
> > rtc_map bitmap is accessed from different vcpus simultaneously so access
> > has to be atomic. We also have a race:
> > 
> > vcpu0                               iothread
> > ioapic config changes
> > request scan ioapic
> >                                      inject rtc interrupt
> >                                      use old vcpu mask
> > scan_ioapic()
> > recalculate vcpu mask
> > 
> > So this approach (suggested by me :() will not work.
> > 
> > Need to think about it some more. May be your idea of building a bitmap
> > while injecting the interrupt is the way to go indeed: pass a pointer to
> > a bitmap to kvm_irq_delivery_to_apic() and build it there. Pass NULL
> > pointer if caller does not need to track vcpus.
> Or, we can block inject rtc interrupt during recalculate vcpu map.
> 
> if(need_eoi > 0 && in_recalculating)
> return coalesced
> 
This should be ||. Then you need to maintain in_recalculating and
recalculations requests may overlap. Too complex and fragile.
 
--
			Gleb.

RE: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-21:
> On Thu, Mar 21, 2013 at 03:42:46AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-03-20:
>>> On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>> 
>>>> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
>>>> or apic register (id, ldr, dfr) is changed.
>>>> 
>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>> ---
>>>>  virt/kvm/ioapic.c |    9 +++++++--
>>>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>>>> index ddf9414..91b4c08 100644
>>>> --- a/virt/kvm/ioapic.c
>>>> +++ b/virt/kvm/ioapic.c
>>>> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
>>>>  { 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; 	union
>>>>  kvm_ioapic_redirect_entry *e; +	unsigned long *rtc_map =
>>>>  ioapic->rtc_status.vcpu_map; 	struct kvm_lapic_irq irqe; 	int index;
>>>> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu
> *vcpu,
>>>>  		if (!e->fields.mask &&
>>>>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
>>>>  			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
>>>> -				 index))) {
>>>> +				 index) || index == 8)) {
>>>>  			irqe.dest_id = e->fields.dest_id;
>>>>  			irqe.vector = e->fields.vector;
>>>>  			irqe.dest_mode = e->fields.dest_mode;
>>>>  			irqe.shorthand = 0;
>>>>  
>>>>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
>>>> -						irqe.dest_id, irqe.dest_mode))
>>>> +						irqe.dest_id, irqe.dest_mode)) {
>>>>  				__set_bit(irqe.vector, eoi_exit_bitmap);
>>>> +				if (index == 8)
>>>> +					__set_bit(vcpu->vcpu_id, rtc_map);
>>>> +			} else if (index == 8)
>>>> +				__clear_bit(vcpu->vcpu_id, rtc_map);
>>> rtc_map bitmap is accessed from different vcpus simultaneously so access
>>> has to be atomic. We also have a race:
>>> 
>>> vcpu0                               iothread
>>> ioapic config changes
>>> request scan ioapic
>>>                                      inject rtc interrupt
>>>                                      use old vcpu mask
>>> scan_ioapic()
>>> recalculate vcpu mask
>>> 
>>> So this approach (suggested by me :() will not work.
>>> 
>>> Need to think about it some more. May be your idea of building a bitmap
>>> while injecting the interrupt is the way to go indeed: pass a pointer to
>>> a bitmap to kvm_irq_delivery_to_apic() and build it there. Pass NULL
>>> pointer if caller does not need to track vcpus.
>> Or, we can block inject rtc interrupt during recalculate vcpu map.
>> 
>> if(need_eoi > 0 && in_recalculating)
>> return coalesced
>> 
> This should be ||. Then you need to maintain in_recalculating and
> recalculations requests may overlap. Too complex and fragile.
It should not be too complex. How about the following logic?

when make scan ioapic request:
kvm_vcpu_scan_ioapic()
{
kvm_for_each_vcpu()
	in_recalculating++;
}

Then on each vcpu's request handler:
vcpu_scan_ioapic()
{
in_recalculating--;
}

And when delivering RTC interrupt:
if(need_eoi > 0 || in_recalculating)
	return coalesced


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


Best regards,
Yang

Re: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Gleb Natapov]

On Thu, Mar 21, 2013 at 05:30:32AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-21:
> > On Thu, Mar 21, 2013 at 03:42:46AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-03-20:
> >>> On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
> >>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>> 
> >>>> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
> >>>> or apic register (id, ldr, dfr) is changed.
> >>>> 
> >>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >>>> ---
> >>>>  virt/kvm/ioapic.c |    9 +++++++--
> >>>>  1 files changed, 7 insertions(+), 2 deletions(-)
> >>>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> >>>> index ddf9414..91b4c08 100644
> >>>> --- a/virt/kvm/ioapic.c
> >>>> +++ b/virt/kvm/ioapic.c
> >>>> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
> >>>>  { 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; 	union
> >>>>  kvm_ioapic_redirect_entry *e; +	unsigned long *rtc_map =
> >>>>  ioapic->rtc_status.vcpu_map; 	struct kvm_lapic_irq irqe; 	int index;
> >>>> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu
> > *vcpu,
> >>>>  		if (!e->fields.mask &&
> >>>>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
> >>>>  			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
> >>>> -				 index))) {
> >>>> +				 index) || index == 8)) {
> >>>>  			irqe.dest_id = e->fields.dest_id;
> >>>>  			irqe.vector = e->fields.vector;
> >>>>  			irqe.dest_mode = e->fields.dest_mode;
> >>>>  			irqe.shorthand = 0;
> >>>>  
> >>>>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
> >>>> -						irqe.dest_id, irqe.dest_mode))
> >>>> +						irqe.dest_id, irqe.dest_mode)) {
> >>>>  				__set_bit(irqe.vector, eoi_exit_bitmap);
> >>>> +				if (index == 8)
> >>>> +					__set_bit(vcpu->vcpu_id, rtc_map);
> >>>> +			} else if (index == 8)
> >>>> +				__clear_bit(vcpu->vcpu_id, rtc_map);
> >>> rtc_map bitmap is accessed from different vcpus simultaneously so access
> >>> has to be atomic. We also have a race:
> >>> 
> >>> vcpu0                               iothread
> >>> ioapic config changes
> >>> request scan ioapic
> >>>                                      inject rtc interrupt
> >>>                                      use old vcpu mask
> >>> scan_ioapic()
> >>> recalculate vcpu mask
> >>> 
> >>> So this approach (suggested by me :() will not work.
> >>> 
> >>> Need to think about it some more. May be your idea of building a bitmap
> >>> while injecting the interrupt is the way to go indeed: pass a pointer to
> >>> a bitmap to kvm_irq_delivery_to_apic() and build it there. Pass NULL
> >>> pointer if caller does not need to track vcpus.
> >> Or, we can block inject rtc interrupt during recalculate vcpu map.
> >> 
> >> if(need_eoi > 0 && in_recalculating)
> >> return coalesced
> >> 
> > This should be ||. Then you need to maintain in_recalculating and
> > recalculations requests may overlap. Too complex and fragile.
> It should not be too complex. How about the following logic?
> 
> when make scan ioapic request:
> kvm_vcpu_scan_ioapic()
> {
> kvm_for_each_vcpu()
> 	in_recalculating++;
> }
> 
> Then on each vcpu's request handler:
> vcpu_scan_ioapic()
> {
> in_recalculating--;
> }
> 
kvm_vcpu_scan_ioapic() can be called more often then vcpu_scan_ioapic()

> And when delivering RTC interrupt:
> if(need_eoi > 0 || in_recalculating)
> 	return coalesced
> 
> 
> > 
> > --
> > 			Gleb.
> > --
> > 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
> 
> 
> Best regards,
> Yang
> 

--
			Gleb.

RE: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-21:
> On Thu, Mar 21, 2013 at 05:30:32AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-03-21:
>>> On Thu, Mar 21, 2013 at 03:42:46AM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-03-20:
>>>>> On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>> 
>>>>>> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
>>>>>> or apic register (id, ldr, dfr) is changed.
>>>>>> 
>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>> ---
>>>>>>  virt/kvm/ioapic.c |    9 +++++++--
>>>>>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>>>>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>>>>>> index ddf9414..91b4c08 100644
>>>>>> --- a/virt/kvm/ioapic.c
>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu
> *vcpu,
>>>>>>  { 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; 	union
>>>>>>  kvm_ioapic_redirect_entry *e; +	unsigned long *rtc_map =
>>>>>>  ioapic->rtc_status.vcpu_map; 	struct kvm_lapic_irq irqe; 	int index;
>>>>>> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu
>>> *vcpu,
>>>>>>  		if (!e->fields.mask &&
>>>>>>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
>>>>>>  			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
>>>>>> -				 index))) {
>>>>>> +				 index) || index == 8)) {
>>>>>>  			irqe.dest_id = e->fields.dest_id;
>>>>>>  			irqe.vector = e->fields.vector;
>>>>>>  			irqe.dest_mode = e->fields.dest_mode;
>>>>>>  			irqe.shorthand = 0;
>>>>>>  
>>>>>>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
>>>>>> -						irqe.dest_id, irqe.dest_mode))
>>>>>> +						irqe.dest_id, irqe.dest_mode)) {
>>>>>>  				__set_bit(irqe.vector, eoi_exit_bitmap);
>>>>>> +				if (index == 8)
>>>>>> +					__set_bit(vcpu->vcpu_id, rtc_map);
>>>>>> +			} else if (index == 8)
>>>>>> +				__clear_bit(vcpu->vcpu_id, rtc_map);
>>>>> rtc_map bitmap is accessed from different vcpus simultaneously so access
>>>>> has to be atomic. We also have a race:
>>>>> 
>>>>> vcpu0                               iothread
>>>>> ioapic config changes
>>>>> request scan ioapic
>>>>>                                      inject rtc interrupt
>>>>>                                      use old vcpu mask
>>>>> scan_ioapic()
>>>>> recalculate vcpu mask
>>>>> 
>>>>> So this approach (suggested by me :() will not work.
>>>>> 
>>>>> Need to think about it some more. May be your idea of building a bitmap
>>>>> while injecting the interrupt is the way to go indeed: pass a pointer to
>>>>> a bitmap to kvm_irq_delivery_to_apic() and build it there. Pass NULL
>>>>> pointer if caller does not need to track vcpus.
>>>> Or, we can block inject rtc interrupt during recalculate vcpu map.
>>>> 
>>>> if(need_eoi > 0 && in_recalculating)
>>>> return coalesced
>>>> 
>>> This should be ||. Then you need to maintain in_recalculating and
>>> recalculations requests may overlap. Too complex and fragile.
>> It should not be too complex. How about the following logic?
>> 
>> when make scan ioapic request:
>> kvm_vcpu_scan_ioapic()
>> {
>> kvm_for_each_vcpu()
>> 	in_recalculating++;
>> }
>> 
>> Then on each vcpu's request handler:
>> vcpu_scan_ioapic()
>> {
>> in_recalculating--;
>> }
>> 
> kvm_vcpu_scan_ioapic() can be called more often then vcpu_scan_ioapic()
Ok. I see your point. Maybe we need to rollback to old idea.

Can you pick the first two patches? If rollback to old way, it will not touch those code.

> 
>> And when delivering RTC interrupt:
>> if(need_eoi > 0 || in_recalculating)
>> 	return coalesced
>> 
>> 
>>> 
>>> --
>>> 			Gleb.
>>> --
>>> 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
>> 
>> 
>> Best regards,
>> Yang
>> 
> 
> --
> 			Gleb.


Best regards,
Yang

Re: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Gleb Natapov]

On Thu, Mar 21, 2013 at 05:39:46AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-21:
> > On Thu, Mar 21, 2013 at 05:30:32AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-03-21:
> >>> On Thu, Mar 21, 2013 at 03:42:46AM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2013-03-20:
> >>>>> On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
> >>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>> 
> >>>>>> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
> >>>>>> or apic register (id, ldr, dfr) is changed.
> >>>>>> 
> >>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>> ---
> >>>>>>  virt/kvm/ioapic.c |    9 +++++++--
> >>>>>>  1 files changed, 7 insertions(+), 2 deletions(-)
> >>>>>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> >>>>>> index ddf9414..91b4c08 100644
> >>>>>> --- a/virt/kvm/ioapic.c
> >>>>>> +++ b/virt/kvm/ioapic.c
> >>>>>> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu
> > *vcpu,
> >>>>>>  { 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; 	union
> >>>>>>  kvm_ioapic_redirect_entry *e; +	unsigned long *rtc_map =
> >>>>>>  ioapic->rtc_status.vcpu_map; 	struct kvm_lapic_irq irqe; 	int index;
> >>>>>> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu
> >>> *vcpu,
> >>>>>>  		if (!e->fields.mask &&
> >>>>>>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
> >>>>>>  			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
> >>>>>> -				 index))) {
> >>>>>> +				 index) || index == 8)) {
> >>>>>>  			irqe.dest_id = e->fields.dest_id;
> >>>>>>  			irqe.vector = e->fields.vector;
> >>>>>>  			irqe.dest_mode = e->fields.dest_mode;
> >>>>>>  			irqe.shorthand = 0;
> >>>>>>  
> >>>>>>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
> >>>>>> -						irqe.dest_id, irqe.dest_mode))
> >>>>>> +						irqe.dest_id, irqe.dest_mode)) {
> >>>>>>  				__set_bit(irqe.vector, eoi_exit_bitmap);
> >>>>>> +				if (index == 8)
> >>>>>> +					__set_bit(vcpu->vcpu_id, rtc_map);
> >>>>>> +			} else if (index == 8)
> >>>>>> +				__clear_bit(vcpu->vcpu_id, rtc_map);
> >>>>> rtc_map bitmap is accessed from different vcpus simultaneously so access
> >>>>> has to be atomic. We also have a race:
> >>>>> 
> >>>>> vcpu0                               iothread
> >>>>> ioapic config changes
> >>>>> request scan ioapic
> >>>>>                                      inject rtc interrupt
> >>>>>                                      use old vcpu mask
> >>>>> scan_ioapic()
> >>>>> recalculate vcpu mask
> >>>>> 
> >>>>> So this approach (suggested by me :() will not work.
> >>>>> 
> >>>>> Need to think about it some more. May be your idea of building a bitmap
> >>>>> while injecting the interrupt is the way to go indeed: pass a pointer to
> >>>>> a bitmap to kvm_irq_delivery_to_apic() and build it there. Pass NULL
> >>>>> pointer if caller does not need to track vcpus.
> >>>> Or, we can block inject rtc interrupt during recalculate vcpu map.
> >>>> 
> >>>> if(need_eoi > 0 && in_recalculating)
> >>>> return coalesced
> >>>> 
> >>> This should be ||. Then you need to maintain in_recalculating and
> >>> recalculations requests may overlap. Too complex and fragile.
> >> It should not be too complex. How about the following logic?
> >> 
> >> when make scan ioapic request:
> >> kvm_vcpu_scan_ioapic()
> >> {
> >> kvm_for_each_vcpu()
> >> 	in_recalculating++;
> >> }
> >> 
> >> Then on each vcpu's request handler:
> >> vcpu_scan_ioapic()
> >> {
> >> in_recalculating--;
> >> }
> >> 
> > kvm_vcpu_scan_ioapic() can be called more often then vcpu_scan_ioapic()
> Ok. I see your point. Maybe we need to rollback to old idea.
> 
> Can you pick the first two patches? If rollback to old way, it will not touch those code.
> 
First patch is great, but drop no longer needed irqe there. I do not see
the point of the second patch if the map will be built during injection.

--
			Gleb.

RE: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-21:
> On Thu, Mar 21, 2013 at 05:39:46AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-03-21:
>>> On Thu, Mar 21, 2013 at 05:30:32AM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-03-21:
>>>>> On Thu, Mar 21, 2013 at 03:42:46AM +0000, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-03-20:
>>>>>>> On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>> 
>>>>>>>> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
>>>>>>>> or apic register (id, ldr, dfr) is changed.
>>>>>>>> 
>>>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>> ---
>>>>>>>>  virt/kvm/ioapic.c |    9 +++++++--
>>>>>>>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>>>>>>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>>>>>>>> index ddf9414..91b4c08 100644
>>>>>>>> --- a/virt/kvm/ioapic.c
>>>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>>>> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu
>>> *vcpu,
>>>>>>>>  { 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; 	union
>>>>>>>>  kvm_ioapic_redirect_entry *e; +	unsigned long *rtc_map =
>>>>>>>>  ioapic->rtc_status.vcpu_map; 	struct kvm_lapic_irq irqe; 	int
> index;
>>>>>>>> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct
> kvm_vcpu
>>>>> *vcpu,
>>>>>>>>  		if (!e->fields.mask &&
>>>>>>>>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
>>>>>>>>  			 kvm_irq_has_notifier(ioapic->kvm,
> KVM_IRQCHIP_IOAPIC,
>>>>>>>> -				 index))) {
>>>>>>>> +				 index) || index == 8)) {
>>>>>>>>  			irqe.dest_id = e->fields.dest_id;
>>>>>>>>  			irqe.vector = e->fields.vector;
>>>>>>>>  			irqe.dest_mode = e->fields.dest_mode;
>>>>>>>>  			irqe.shorthand = 0;
>>>>>>>>  
>>>>>>>>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
>>>>>>>> -						irqe.dest_id, irqe.dest_mode))
>>>>>>>> +						irqe.dest_id, irqe.dest_mode)) {
>>>>>>>>  				__set_bit(irqe.vector, eoi_exit_bitmap);
>>>>>>>> +				if (index == 8)
>>>>>>>> +					__set_bit(vcpu->vcpu_id, rtc_map);
>>>>>>>> +			} else if (index == 8)
>>>>>>>> +				__clear_bit(vcpu->vcpu_id, rtc_map);
>>>>>>> rtc_map bitmap is accessed from different vcpus simultaneously so
>>>>>>> access has to be atomic. We also have a race:
>>>>>>> 
>>>>>>> vcpu0                               iothread
>>>>>>> ioapic config changes
>>>>>>> request scan ioapic
>>>>>>>                                      inject rtc interrupt
>>>>>>>                                      use old vcpu mask
>>>>>>> scan_ioapic()
>>>>>>> recalculate vcpu mask
>>>>>>> 
>>>>>>> So this approach (suggested by me :() will not work.
>>>>>>> 
>>>>>>> Need to think about it some more. May be your idea of building a
>>>>>>> bitmap while injecting the interrupt is the way to go indeed: pass
>>>>>>> a pointer to a bitmap to kvm_irq_delivery_to_apic() and build it
>>>>>>> there. Pass NULL pointer if caller does not need to track vcpus.
>>>>>> Or, we can block inject rtc interrupt during recalculate vcpu map.
>>>>>> 
>>>>>> if(need_eoi > 0 && in_recalculating)
>>>>>> return coalesced
>>>>>> 
>>>>> This should be ||. Then you need to maintain in_recalculating and
>>>>> recalculations requests may overlap. Too complex and fragile.
>>>> It should not be too complex. How about the following logic?
>>>> 
>>>> when make scan ioapic request:
>>>> kvm_vcpu_scan_ioapic()
>>>> {
>>>> kvm_for_each_vcpu()
>>>> 	in_recalculating++;
>>>> }
>>>> 
>>>> Then on each vcpu's request handler:
>>>> vcpu_scan_ioapic()
>>>> {
>>>> in_recalculating--;
>>>> }
>>>> 
>>> kvm_vcpu_scan_ioapic() can be called more often then vcpu_scan_ioapic()
>> Ok. I see your point. Maybe we need to rollback to old idea.
>> 
>> Can you pick the first two patches? If rollback to old way, it will not
>> touch those code.
>> 
> First patch is great, but drop no longer needed irqe there. I do not see
> the point of the second patch if the map will be built during injection.
Sure. I will resend the first patch.
And we need to rebuild TMR when ioapic entry changed. So the second patch will be used at that time. But it's ok to send it with APICv patch.

Best regards,
Yang

RE: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-20:
> On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
>> or apic register (id, ldr, dfr) is changed.
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  virt/kvm/ioapic.c |    9 +++++++--
>>  1 files changed, 7 insertions(+), 2 deletions(-)
>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>> index ddf9414..91b4c08 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
>>  { 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; 	union
>>  kvm_ioapic_redirect_entry *e; +	unsigned long *rtc_map =
>>  ioapic->rtc_status.vcpu_map; 	struct kvm_lapic_irq irqe; 	int index;
>> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
>>  		if (!e->fields.mask &&
>>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
>>  			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
>> -				 index))) {
>> +				 index) || index == 8)) {
>>  			irqe.dest_id = e->fields.dest_id;
>>  			irqe.vector = e->fields.vector;
>>  			irqe.dest_mode = e->fields.dest_mode;
>>  			irqe.shorthand = 0;
>>  
>>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
>> -						irqe.dest_id, irqe.dest_mode))
>> +						irqe.dest_id, irqe.dest_mode)) {
>>  				__set_bit(irqe.vector, eoi_exit_bitmap);
>> +				if (index == 8)
>> +					__set_bit(vcpu->vcpu_id, rtc_map);
>> +			} else if (index == 8)
>> +				__clear_bit(vcpu->vcpu_id, rtc_map);
> rtc_map bitmap is accessed from different vcpus simultaneously so access
> has to be atomic. We also have a race:
> 
> vcpu0                               iothread
> ioapic config changes
> request scan ioapic
>                                      inject rtc interrupt
>                                      use old vcpu mask
> scan_ioapic()
> recalculate vcpu mask
> 
> So this approach (suggested by me :() will not work.
> 
> Need to think about it some more. May be your idea of building a bitmap
> while injecting the interrupt is the way to go indeed: pass a pointer to
> a bitmap to kvm_irq_delivery_to_apic() and build it there. Pass NULL
> pointer if caller does not need to track vcpus.
How about build it in kvm_apic_set_irq()? It should be more straightforward.

Best regards,
Yang

Re: [PATCH v4 5/7] KVM: Recalculate destination vcpu map

[Gleb Natapov]

On Thu, Mar 21, 2013 at 07:02:39AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-20:
> > On Wed, Mar 20, 2013 at 07:36:17PM +0800, Yang Zhang wrote:
> >> From: Yang Zhang <yang.z.zhang@Intel.com>
> >> 
> >> Update RTC interrrupt's destination vcpu map when ioapic entry of RTC
> >> or apic register (id, ldr, dfr) is changed.
> >> 
> >> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >> ---
> >>  virt/kvm/ioapic.c |    9 +++++++--
> >>  1 files changed, 7 insertions(+), 2 deletions(-)
> >> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> >> index ddf9414..91b4c08 100644
> >> --- a/virt/kvm/ioapic.c
> >> +++ b/virt/kvm/ioapic.c
> >> @@ -121,6 +121,7 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
> >>  { 	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic; 	union
> >>  kvm_ioapic_redirect_entry *e; +	unsigned long *rtc_map =
> >>  ioapic->rtc_status.vcpu_map; 	struct kvm_lapic_irq irqe; 	int index;
> >> @@ -130,15 +131,19 @@ void kvm_ioapic_scan_entry(struct kvm_vcpu *vcpu,
> >>  		if (!e->fields.mask &&
> >>  			(e->fields.trig_mode == IOAPIC_LEVEL_TRIG ||
> >>  			 kvm_irq_has_notifier(ioapic->kvm, KVM_IRQCHIP_IOAPIC,
> >> -				 index))) {
> >> +				 index) || index == 8)) {
> >>  			irqe.dest_id = e->fields.dest_id;
> >>  			irqe.vector = e->fields.vector;
> >>  			irqe.dest_mode = e->fields.dest_mode;
> >>  			irqe.shorthand = 0;
> >>  
> >>  			if (kvm_apic_match_dest(vcpu, NULL, irqe.shorthand,
> >> -						irqe.dest_id, irqe.dest_mode))
> >> +						irqe.dest_id, irqe.dest_mode)) {
> >>  				__set_bit(irqe.vector, eoi_exit_bitmap);
> >> +				if (index == 8)
> >> +					__set_bit(vcpu->vcpu_id, rtc_map);
> >> +			} else if (index == 8)
> >> +				__clear_bit(vcpu->vcpu_id, rtc_map);
> > rtc_map bitmap is accessed from different vcpus simultaneously so access
> > has to be atomic. We also have a race:
> > 
> > vcpu0                               iothread
> > ioapic config changes
> > request scan ioapic
> >                                      inject rtc interrupt
> >                                      use old vcpu mask
> > scan_ioapic()
> > recalculate vcpu mask
> > 
> > So this approach (suggested by me :() will not work.
> > 
> > Need to think about it some more. May be your idea of building a bitmap
> > while injecting the interrupt is the way to go indeed: pass a pointer to
> > a bitmap to kvm_irq_delivery_to_apic() and build it there. Pass NULL
> > pointer if caller does not need to track vcpus.
> How about build it in kvm_apic_set_irq()? It should be more straightforward.
> 
Sure, pass a pointer there. Just do not access ioapic directly.

--
			Gleb.