[PATCH v2 0/8] Use eoi to track RTC interrupt delivery status

[PATCH v2 0/8] 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 v1 to v2:
* Don't register a dumy ack notifier for RTC. Check it directly when calculating
  eoi exit bitmap.
* Use kvm_apic_pending_eoi() instead call apic_test_vector() directly.
* Check coalesced info before set ioapic->irr
* Calculate destination vcpu map of RTC when ioapic entry or apic(id,ldr/dfr) changed.
  And only set need_eoi when delivering RTC interrupt.

Yang Zhang (8):
  KVM: Parse ioapic entry to get destination vcpu
  KVM: Rename kvm_ioapic_make_eoibitmap_request to
    kvm_scan_ioapic_entry
  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: Add rtc irq to eoi exit bitmap
  KVM: Use eoi to track RTC interrupt delivery status

 arch/x86/kvm/lapic.c |   52 +++++++++++------
 arch/x86/kvm/lapic.h |    4 +
 virt/kvm/ioapic.c    |  158 ++++++++++++++++++++++++++++++++++++++++++++++----
 virt/kvm/ioapic.h    |   14 ++++-
 virt/kvm/irq_comm.c  |    4 +-
 5 files changed, 198 insertions(+), 34 deletions(-)

[PATCH v2 1/8] KVM: Parse ioapic entry to get destination vcpu

[Yang Zhang]

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

Get destination vcpu map from one ioapic entry.

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 02b51dd..b4339a5 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -145,28 +145,26 @@ 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)
+void kvm_get_dest_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
+				unsigned long *vcpu_map)
 {
 	struct kvm_lapic **dst;
 	struct kvm_apic_map *map;
 	unsigned long bitmap = 1;
+	struct kvm_vcpu *vcpu;
 	int i;
 
 	rcu_read_lock();
-	map = rcu_dereference(vcpu->kvm->arch.apic_map);
+	map = rcu_dereference(kvm->arch.apic_map);
 
-	if (unlikely(!map)) {
-		__set_bit(irq->vector, (unsigned long *)eoi_exit_bitmap);
+	if (unlikely(!map))
 		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);
+		if (irq->dest_id == 0xff) {
+			kvm_for_each_vcpu(i, vcpu, kvm)
+				if (apic_enabled(vcpu->arch.apic))
+					set_bit(vcpu->vcpu_id, vcpu_map);
 			goto out;
 		}
 		dst = &map->phys_map[irq->dest_id & 0xff];
@@ -181,17 +179,27 @@ void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
 	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;
-		}
+		set_bit(dst[i]->vcpu->vcpu_id, vcpu_map);
 	}
 
 out:
 	rcu_read_unlock();
 }
 
+void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
+				struct kvm_lapic_irq *irq,
+				u64 *eoi_exit_bitmap)
+{
+	DECLARE_BITMAP(vcpu_map, KVM_MAX_VCPUS);
+
+	memset(vcpu_map, 0, sizeof(vcpu_map));
+
+	kvm_get_dest_vcpu(vcpu->kvm, irq, vcpu_map);
+	if (test_bit(vcpu->vcpu_id, vcpu_map) ||
+			bitmap_empty(vcpu_map, sizeof(vcpu_map)))
+		__set_bit(irq->vector, (unsigned long *)eoi_exit_bitmap);
+}
+
 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..3a0f9d8 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -158,4 +158,7 @@ void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
 				struct kvm_lapic_irq *irq,
 				u64 *eoi_bitmap);
 
+void kvm_get_dest_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
+				unsigned long *vcpu_map);
+
 #endif
-- 
1.7.1

[PATCH v2 2/8] KVM: Rename kvm_ioapic_make_eoibitmap_request to kvm_scan_ioapic_entry

[Yang Zhang]

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

RTC interrupt coalesced need to parse ioapic entry to get destionation
vcpu too. Rename it to more common name.

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index b4339a5..12179b3 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -264,7 +264,7 @@ out:
 	if (old)
 		kfree_rcu(old, rcu);
 
-	kvm_ioapic_make_eoibitmap_request(kvm);
+	kvm_scan_ioapic_entry(kvm);
 }
 
 static inline void kvm_apic_set_id(struct kvm_lapic *apic, u8 id)
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index ce82b94..d087e07 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -143,7 +143,7 @@ void kvm_ioapic_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
 }
 EXPORT_SYMBOL_GPL(kvm_ioapic_calculate_eoi_exitmap);
 
-void kvm_ioapic_make_eoibitmap_request(struct kvm *kvm)
+void kvm_scan_ioapic_entry(struct kvm *kvm)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
 
@@ -193,7 +193,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_scan_ioapic_entry(ioapic->kvm);
 		break;
 	}
 }
@@ -493,7 +493,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_scan_ioapic_entry(kvm);
 	spin_unlock(&ioapic->lock);
 	return 0;
 }
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 0400a46..cebc123 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -82,7 +82,7 @@ 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_scan_ioapic_entry(struct kvm *kvm);
 void kvm_ioapic_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
 					u64 *eoi_exit_bitmap);
 
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index ff6d40e..15de3b7 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_scan_ioapic_entry(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_scan_ioapic_entry(kvm);
 }
 
 int kvm_request_irq_source_id(struct kvm *kvm)
-- 
1.7.1

[PATCH v2 3/8] KVM: Add vcpu info to ioapic_update_eoi()

[Yang Zhang]

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

Need to know which vcpu writes 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 12179b3..6fb22e3 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -790,7 +790,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 d087e07..4296116 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -264,8 +264,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;
 
@@ -304,12 +304,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);
 }
 
@@ -407,7 +407,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 cebc123..2001b61 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 v2 4/8] 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 2001b61..4904ca3 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);
+	struct kvm_irq_ack_notifier irq_ack_notifier;
+};
+
 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 v2 5/8] KVM: Recalculate destination vcpu map

[Yang Zhang]

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

Update destination vcpu map when ioapic entry or apic(id, ldr, dfr) is changed

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

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 4296116..659511d 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -87,6 +87,36 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
 	return result;
 }
 
+#ifdef CONFIG_X86
+static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
+{
+	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
+	struct kvm_lapic_irq irqe;
+
+	if (irq != 8 || entry->fields.mask)
+		return;
+
+	irqe.dest_id = entry->fields.dest_id;
+	irqe.vector = entry->fields.vector;
+	irqe.dest_mode = entry->fields.dest_mode;
+	irqe.trig_mode = entry->fields.trig_mode;
+	irqe.delivery_mode = entry->fields.delivery_mode << 8;
+	irqe.level = 1;
+	irqe.shorthand = 0;
+
+	bitmap_zero(ioapic->rtc_status.vcpu_map, KVM_MAX_VCPUS);
+
+	kvm_get_dest_vcpu(ioapic->kvm, &irqe, ioapic->rtc_status.vcpu_map);
+}
+
+#else
+
+static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
+{
+	return;
+}
+#endif
+
 static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
 {
 	union kvm_ioapic_redirect_entry *pent;
@@ -147,9 +177,13 @@ void kvm_scan_ioapic_entry(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);
+
+	rtc_irq_get_dest_vcpu(ioapic, 8);
+
+	if (kvm_apic_vid_enabled(kvm))
+		kvm_make_update_eoibitmap_request(kvm);
 }
 
 static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
-- 
1.7.1

[PATCH v2 6/8] 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 |    1 +
 virt/kvm/ioapic.c    |   33 +++++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6fb22e3..a223170 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 3a0f9d8..e2a03d1 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -160,5 +160,6 @@ void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
 
 void kvm_get_dest_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
 				unsigned long *vcpu_map);
+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 659511d..6266d1f 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -88,6 +88,27 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
 }
 
 #ifdef CONFIG_X86
+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.vcpu_map);
+		}
+	}
+	ioapic->rtc_status.need_eoi = need_eoi;
+}
+
 static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
 {
 	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
@@ -111,6 +132,16 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
 
 #else
 
+static void rtc_irq_reset(struct kvm_ioapic *ioapic)
+{
+	return;
+}
+
+static void rtc_irq_restore(struct kvm_ioapic *ioapic)
+{
+	return;
+}
+
 static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
 {
 	return;
@@ -462,6 +493,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);
 }
 
@@ -527,6 +559,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_scan_ioapic_entry(kvm);
 	spin_unlock(&ioapic->lock);
 	return 0;
-- 
1.7.1

[PATCH v2 7/8] KVM: Add rtc irq to eoi exit bitmap

[Yang Zhang]

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

Add rtc irq to eoi exit bitmap to force vmexit when virtual interrupt
delivery is enabled.

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

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 6266d1f..7e47da8 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -192,7 +192,7 @@ void kvm_ioapic_calculate_eoi_exitmap(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;
-- 
1.7.1

[PATCH v2 8/8] 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 |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 7e47da8..8d498e5 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -130,6 +130,48 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
 	kvm_get_dest_vcpu(ioapic->kvm, &irqe, ioapic->rtc_status.vcpu_map);
 }
 
+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;
+		}
+	}
+}
+
+static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
+			struct rtc_status *rtc_status, int irq)
+{
+	if (irq != 8)
+		return;
+
+	if (test_bit(vcpu->vcpu_id, rtc_status->vcpu_map))
+		--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
 
 static void rtc_irq_reset(struct kvm_ioapic *ioapic)
@@ -146,6 +188,22 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
 {
 	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 int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
@@ -282,6 +340,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);
 }
 
@@ -306,6 +366,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))
@@ -313,6 +378,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);
 
@@ -340,6 +406,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 v2 4/8] KVM: Introduce struct rtc_status

[Gleb Natapov]

On Mon, Mar 18, 2013 at 03:24:35PM +0800, Yang Zhang wrote:
> 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 2001b61..4904ca3 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);
> +	struct kvm_irq_ack_notifier irq_ack_notifier;
If we do not register ack notifier any more why do you need this here?
Also give the structure more kvmish name.

> +};
> +
>  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

--
			Gleb.

Re: [PATCH v2 5/8] KVM: Recalculate destination vcpu map

[Gleb Natapov]

On Mon, Mar 18, 2013 at 03:24:36PM +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> Update destination vcpu map when ioapic entry or apic(id, ldr, dfr) is changed
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  virt/kvm/ioapic.c |   38 ++++++++++++++++++++++++++++++++++++--
>  1 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 4296116..659511d 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -87,6 +87,36 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
>  	return result;
>  }
>  
> +#ifdef CONFIG_X86
> +static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
> +{
> +	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
> +	struct kvm_lapic_irq irqe;
> +
You cannot access ioapic without taking ioapic lock.

> +	if (irq != 8 || entry->fields.mask)
> +		return;
> +
> +	irqe.dest_id = entry->fields.dest_id;
> +	irqe.vector = entry->fields.vector;
> +	irqe.dest_mode = entry->fields.dest_mode;
> +	irqe.trig_mode = entry->fields.trig_mode;
> +	irqe.delivery_mode = entry->fields.delivery_mode << 8;
> +	irqe.level = 1;
> +	irqe.shorthand = 0;
> +
> +	bitmap_zero(ioapic->rtc_status.vcpu_map, KVM_MAX_VCPUS);
> +
> +	kvm_get_dest_vcpu(ioapic->kvm, &irqe, ioapic->rtc_status.vcpu_map);
> +}
> +
> +#else
> +
> +static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
> +{
> +	return;
> +}
> +#endif
> +
>  static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
>  {
>  	union kvm_ioapic_redirect_entry *pent;
> @@ -147,9 +177,13 @@ void kvm_scan_ioapic_entry(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);
> +
> +	rtc_irq_get_dest_vcpu(ioapic, 8);
> +
> +	if (kvm_apic_vid_enabled(kvm))
> +		kvm_make_update_eoibitmap_request(kvm);
>  }
>  
>  static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
> -- 
> 1.7.1

--
			Gleb.

Re: [PATCH v2 8/8] KVM: Use eoi to track RTC interrupt delivery status

[Gleb Natapov]

On Mon, Mar 18, 2013 at 03:24:39PM +0800, Yang Zhang wrote:
> 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 |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 67 insertions(+), 0 deletions(-)
> 
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 7e47da8..8d498e5 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -130,6 +130,48 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>  	kvm_get_dest_vcpu(ioapic->kvm, &irqe, ioapic->rtc_status.vcpu_map);
>  }
>  
> +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;
> +		}
> +	}
> +}
> +
> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> +			struct rtc_status *rtc_status, int irq)
> +{
> +	if (irq != 8)
> +		return;
> +
> +	if (test_bit(vcpu->vcpu_id, rtc_status->vcpu_map))
If you do not use test_and_clear_bit() here the WARN_ON() bellow can
be triggered by a malicious guest. Lets define rtc_status->expected_eoi
bitmap and copy vcpu_map into expected_eoi on each RTC irq.

> +		--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
>  
>  static void rtc_irq_reset(struct kvm_ioapic *ioapic)
> @@ -146,6 +188,22 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>  {
>  	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 int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
> @@ -282,6 +340,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);
>  }
>  
> @@ -306,6 +366,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))
> @@ -313,6 +378,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);
>  
> @@ -340,6 +406,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

--
			Gleb.

RE: [PATCH v2 4/8] KVM: Introduce struct rtc_status

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-18:
> On Mon, Mar 18, 2013 at 03:24:35PM +0800, Yang Zhang wrote:
>> 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 2001b61..4904ca3 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);
>> +	struct kvm_irq_ack_notifier irq_ack_notifier;
> If we do not register ack notifier any more why do you need this here?
> Also give the structure more kvmish name.
You are right. It's a mistake to leave it here.

>> +};
>> +
>>  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
> 
> --
> 			Gleb.


Best regards,
Yang

RE: [PATCH v2 5/8] KVM: Recalculate destination vcpu map

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-18:
> On Mon, Mar 18, 2013 at 03:24:36PM +0800, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> Update destination vcpu map when ioapic entry or apic(id, ldr, dfr) is changed
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  virt/kvm/ioapic.c |   38 ++++++++++++++++++++++++++++++++++++--
>>  1 files changed, 36 insertions(+), 2 deletions(-)
>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>> index 4296116..659511d 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -87,6 +87,36 @@ static unsigned long ioapic_read_indirect(struct
> kvm_ioapic *ioapic,
>>  	return result;
>>  }
>> +#ifdef CONFIG_X86
>> +static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>> +{
>> +	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
>> +	struct kvm_lapic_irq irqe;
>> +
> You cannot access ioapic without taking ioapic lock.
Right.
 
>> +	if (irq != 8 || entry->fields.mask)
>> +		return;
>> +
>> +	irqe.dest_id = entry->fields.dest_id;
>> +	irqe.vector = entry->fields.vector;
>> +	irqe.dest_mode = entry->fields.dest_mode;
>> +	irqe.trig_mode = entry->fields.trig_mode;
>> +	irqe.delivery_mode = entry->fields.delivery_mode << 8;
>> +	irqe.level = 1;
>> +	irqe.shorthand = 0;
>> +
>> +	bitmap_zero(ioapic->rtc_status.vcpu_map, KVM_MAX_VCPUS);
>> +
>> +	kvm_get_dest_vcpu(ioapic->kvm, &irqe, ioapic->rtc_status.vcpu_map);
>> +}
>> +
>> +#else
>> +
>> +static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>> +{
>> +	return;
>> +}
>> +#endif
>> +
>>  static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
>>  { 	union kvm_ioapic_redirect_entry *pent; @@ -147,9 +177,13 @@ void
>>  kvm_scan_ioapic_entry(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);
>> +
>> +	rtc_irq_get_dest_vcpu(ioapic, 8);
>> +
>> +	if (kvm_apic_vid_enabled(kvm))
>> +		kvm_make_update_eoibitmap_request(kvm);
>>  }
>>  
>>  static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>> --
>> 1.7.1
> 
> --
> 			Gleb.


Best regards,
Yang

RE: [PATCH v2 8/8] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-18:
> On Mon, Mar 18, 2013 at 03:24:39PM +0800, Yang Zhang wrote:
>> 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 |   67
>>  +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed,
>>  67 insertions(+), 0 deletions(-)
>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>> index 7e47da8..8d498e5 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -130,6 +130,48 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic
> *ioapic, int irq)
>>  	kvm_get_dest_vcpu(ioapic->kvm, &irqe, ioapic->rtc_status.vcpu_map);
>>  }
>> +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; +		} +	} +} + +static void
>> rtc_irq_ack_eoi(struct kvm_vcpu *vcpu, +			struct rtc_status
>> *rtc_status, int irq) +{ +	if (irq != 8) +		return; + +	if
>> (test_bit(vcpu->vcpu_id, rtc_status->vcpu_map))
> If you do not use test_and_clear_bit() here the WARN_ON() bellow can
> be triggered by a malicious guest. Lets define rtc_status->expected_eoi
> bitmap and copy vcpu_map into expected_eoi on each RTC irq.
Sure.
 
>> +		--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
>>  
>>  static void rtc_irq_reset(struct kvm_ioapic *ioapic)
>> @@ -146,6 +188,22 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic
> *ioapic, int irq)
>>  {
>>  	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 int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
>> @@ -282,6 +340,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);
>>  }
>> @@ -306,6 +366,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))
>> @@ -313,6 +378,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);
>> @@ -340,6 +406,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
> 
> --
> 			Gleb.


Best regards,
Yang

Re: [PATCH v2 6/8] KVM: Add reset/restore rtc_status support

[Marcelo Tosatti]

On Mon, Mar 18, 2013 at 03:24:37PM +0800, Yang Zhang wrote:
> 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 |    1 +
>  virt/kvm/ioapic.c    |   33 +++++++++++++++++++++++++++++++++
>  3 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6fb22e3..a223170 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);
> +}
> +

Should hook into kvm_lapic_reset and kvm_vcpu_ioctl_set_lapic to
generate updates.

>  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 3a0f9d8..e2a03d1 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -160,5 +160,6 @@ void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
>  
>  void kvm_get_dest_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
>  				unsigned long *vcpu_map);
> +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 659511d..6266d1f 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -88,6 +88,27 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
>  }
>  
>  #ifdef CONFIG_X86
> +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.vcpu_map);

Why set bit on vcpu_map here?

Re: [PATCH v2 6/8] KVM: Add reset/restore rtc_status support

[Marcelo Tosatti]

On Mon, Mar 18, 2013 at 03:24:37PM +0800, Yang Zhang wrote:
> 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 |    1 +
>  virt/kvm/ioapic.c    |   33 +++++++++++++++++++++++++++++++++
>  3 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6fb22e3..a223170 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 3a0f9d8..e2a03d1 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -160,5 +160,6 @@ void kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
>  
>  void kvm_get_dest_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
>  				unsigned long *vcpu_map);
> +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 659511d..6266d1f 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -88,6 +88,27 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
>  }
>  
>  #ifdef CONFIG_X86
> +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.vcpu_map);
> +		}
> +	}
> +	ioapic->rtc_status.need_eoi = need_eoi;
> +}
> +
>  static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>  {
>  	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
> @@ -111,6 +132,16 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>  
>  #else
>  
> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
> +{
> +	return;
> +}
> +
> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> +{
> +	return;
> +}
> +
>  static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>  {
>  	return;
> @@ -462,6 +493,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);
>  }

Should also zero the counter if the OS resets the IOAPIC (think reboot
via triple-fault with unacked RTC interrupt in ISR/IRR).

Re: [PATCH v2 8/8] KVM: Use eoi to track RTC interrupt delivery status

[Marcelo Tosatti]

On Mon, Mar 18, 2013 at 03:24:39PM +0800, Yang Zhang wrote:
> 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 |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 67 insertions(+), 0 deletions(-)
> 
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 7e47da8..8d498e5 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -130,6 +130,48 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>  	kvm_get_dest_vcpu(ioapic->kvm, &irqe, ioapic->rtc_status.vcpu_map);
>  }
>  
> +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;
> +		}
> +	}
> +}

Why two bitmaps are necessary? One should be enough.

RE: [PATCH v2 6/8] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Marcelo Tosatti wrote on 2013-03-20:
> On Mon, Mar 18, 2013 at 03:24:37PM +0800, Yang Zhang wrote:
>> 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 |    1 +
>>  virt/kvm/ioapic.c    |   33 +++++++++++++++++++++++++++++++++
>>  3 files changed, 42 insertions(+), 0 deletions(-)
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 6fb22e3..a223170 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);
>> +}
>> +
> 
> Should hook into kvm_lapic_reset and kvm_vcpu_ioctl_set_lapic to
> generate updates.
rtc_irq_restore will be called after lapic is restored. What's the problem if we no hook into the two function?

>>  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
>> 3a0f9d8..e2a03d1 100644 --- a/arch/x86/kvm/lapic.h +++
>> b/arch/x86/kvm/lapic.h @@ -160,5 +160,6 @@ void
>> kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
>> 
>>  void kvm_get_dest_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
>>  				unsigned long *vcpu_map);
>> +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 659511d..6266d1f 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -88,6 +88,27 @@ static unsigned long ioapic_read_indirect(struct
> kvm_ioapic *ioapic,
>>  }
>>  
>>  #ifdef CONFIG_X86
>> +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.vcpu_map);
> 
> Why set bit on vcpu_map here?
We will set need_eoi here. And if target vcpu is not in vcpu_map, then it will not update need_eoi on EOI.

Best regards,
Yang

RE: [PATCH v2 6/8] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Marcelo Tosatti wrote on 2013-03-20:
> On Mon, Mar 18, 2013 at 03:24:37PM +0800, Yang Zhang wrote:
>> 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 |    1 +
>>  virt/kvm/ioapic.c    |   33 +++++++++++++++++++++++++++++++++
>>  3 files changed, 42 insertions(+), 0 deletions(-)
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 6fb22e3..a223170 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
>> 3a0f9d8..e2a03d1 100644 --- a/arch/x86/kvm/lapic.h +++
>> b/arch/x86/kvm/lapic.h @@ -160,5 +160,6 @@ void
>> kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
>> 
>>  void kvm_get_dest_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
>>  				unsigned long *vcpu_map);
>> +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 659511d..6266d1f 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -88,6 +88,27 @@ static unsigned long ioapic_read_indirect(struct
> kvm_ioapic *ioapic,
>>  }
>>  
>>  #ifdef CONFIG_X86
>> +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.vcpu_map);
>> +		}
>> +	}
>> +	ioapic->rtc_status.need_eoi = need_eoi;
>> +}
>> +
>>  static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>>  {
>>  	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
>> @@ -111,6 +132,16 @@ static void rtc_irq_get_dest_vcpu(struct
>> kvm_ioapic *ioapic, int irq)
>> 
>>  #else
>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
>> +{
>> +	return;
>> +}
>> +
>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>> +{
>> +	return;
>> +}
>> +
>>  static void rtc_irq_get_dest_vcpu(struct kvm_ioapic *ioapic, int irq)
>>  { 	return; @@ -462,6 +493,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); }
> 
> Should also zero the counter if the OS resets the IOAPIC (think reboot
> via triple-fault with unacked RTC interrupt in ISR/IRR).
rtc_irq_reset() already did it.

Best regards,
Yang

RE: [PATCH v2 8/8] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Marcelo Tosatti wrote on 2013-03-20:
> On Mon, Mar 18, 2013 at 03:24:39PM +0800, Yang Zhang wrote:
>> 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 |   67
>>  +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed,
>>  67 insertions(+), 0 deletions(-)
>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
>> index 7e47da8..8d498e5 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -130,6 +130,48 @@ static void rtc_irq_get_dest_vcpu(struct kvm_ioapic
> *ioapic, int irq)
>>  	kvm_get_dest_vcpu(ioapic->kvm, &irqe, ioapic->rtc_status.vcpu_map);
>>  }
>> +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; +		} +	} +}
> 
> Why two bitmaps are necessary? One should be enough.
On eoi, it will clear the bitmap. So we need two bitmap, one only updated when rtc destination vcpu changed and one is copy of it for EOI check.


Best regards,
Yang

RE: [PATCH v2 6/8] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

I have send out the latest patch(v4). Please give comments for the latest one, because some issues you point out may not exist on latest patch. If it still exits. please point out it again.

Zhang, Yang Z wrote on 2013-03-20:
> Marcelo Tosatti wrote on 2013-03-20:
>> On Mon, Mar 18, 2013 at 03:24:37PM +0800, Yang Zhang wrote:
>>> 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 |    1 +
>>>  virt/kvm/ioapic.c    |   33 +++++++++++++++++++++++++++++++++
>>>  3 files changed, 42 insertions(+), 0 deletions(-)
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index 6fb22e3..a223170 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);
>>> +}
>>> +
>> 
>> Should hook into kvm_lapic_reset and kvm_vcpu_ioctl_set_lapic to
>> generate updates.
> rtc_irq_restore will be called after lapic is restored. What's the problem if we no
> hook into the two function?
> 
>>>  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
>>> 3a0f9d8..e2a03d1 100644 --- a/arch/x86/kvm/lapic.h +++
>>> b/arch/x86/kvm/lapic.h @@ -160,5 +160,6 @@ void
>>> kvm_calculate_eoi_exitmap(struct kvm_vcpu *vcpu,
>>> 
>>>  void kvm_get_dest_vcpu(struct kvm *kvm, struct kvm_lapic_irq *irq,
>>>  				unsigned long *vcpu_map);
>>> +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 659511d..6266d1f 100644
>>> --- a/virt/kvm/ioapic.c
>>> +++ b/virt/kvm/ioapic.c
>>> @@ -88,6 +88,27 @@ static unsigned long ioapic_read_indirect(struct
>> kvm_ioapic *ioapic,
>>>  }
>>>  
>>>  #ifdef CONFIG_X86
>>> +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.vcpu_map);
>> 
>> Why set bit on vcpu_map here?
> We will set need_eoi here. And if target vcpu is not in vcpu_map, then it will not
> update need_eoi on EOI.
> 
> Best regards,
> Yang


Best regards,
Yang