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

[PATCH v7 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 pending_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 v6 to v7
* Only track the RTC interrupt when userspace uses *_LINE_* ioctl.
* Call rtc_irq_restore() after lapic is restored.
* Rebase on top of KVM.
 
Changes from v5 to v6
* Move set dest_map logic into __apic_accept_irq().
* Use RTC_GSI to distinguish different platform, and drop all CONFIG_X86.
* Rebase on top of KVM.

Changes from v4 to v5
* Calculate destination vcpu on interrupt injection not hook into ioapic
  modification.
* Rebase on top of KVM.

Yang Zhang (7):
  KVM: Add vcpu info to ioapic_update_eoi()
  KVM: Introduce struct rtc_status
  KVM : Return destination vcpu on interrupt injection
  KVM: Add reset/restore rtc_status support
  KVM : Force vmexit with virtual interrupt delivery
  KVM: Let ioapic know the irq line status
  KVM: Use eoi to track RTC interrupt delivery status

 arch/x86/kvm/i8254.c     |    4 +-
 arch/x86/kvm/lapic.c     |   36 ++++++++++----
 arch/x86/kvm/lapic.h     |    7 ++-
 arch/x86/kvm/x86.c       |    6 ++-
 include/linux/kvm_host.h |   11 +++--
 virt/kvm/assigned-dev.c  |   13 +++--
 virt/kvm/eventfd.c       |   15 ++++--
 virt/kvm/ioapic.c        |  115 +++++++++++++++++++++++++++++++++++++++------
 virt/kvm/ioapic.h        |   20 +++++++-
 virt/kvm/irq_comm.c      |   31 +++++++-----
 virt/kvm/kvm_main.c      |    3 +-
 11 files changed, 196 insertions(+), 65 deletions(-)

[PATCH v7 1/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 a8e9369..d3e322a 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -786,7 +786,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 5ba005c..9379386 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -267,8 +267,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;
 
@@ -307,12 +307,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);
 }
 
@@ -410,7 +410,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 0400a46..2fc61a5 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 v7 2/7] KVM: Introduce struct rtc_status

[Yang Zhang]

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

rtc_status is used to track RTC interrupt delivery status. The pending_eoi
will be increased by vcpu who received RTC interrupt and will be decreased
when EOI to this interrupt.
Also, we use dest_map to record the destination vcpu to avoid the case that
vcpu who didn't get the RTC interupt, but issued EOI with same vector of RTC
and descreased pending_eoi by mistake.

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

diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 2fc61a5..87cd94b 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -34,6 +34,17 @@ struct kvm_vcpu;
 #define	IOAPIC_INIT			0x5
 #define	IOAPIC_EXTINT			0x7
 
+#ifdef CONFIG_X86
+#define RTC_GSI 8
+#else
+#define RTC_GSI -1U
+#endif
+
+struct rtc_status {
+	int pending_eoi;
+	DECLARE_BITMAP(dest_map, KVM_MAX_VCPUS);
+};
+
 struct kvm_ioapic {
 	u64 base_address;
 	u32 ioregsel;
@@ -47,6 +58,7 @@ struct kvm_ioapic {
 	void (*ack_notifier)(void *opaque, int irq);
 	spinlock_t lock;
 	DECLARE_BITMAP(handled_vectors, 256);
+	struct rtc_status rtc_status;
 };
 
 #ifdef DEBUG
-- 
1.7.1

[PATCH v7 3/7] KVM : Return destination vcpu on interrupt injection

[Yang Zhang]

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

Add a new parameter to know vcpus who received the interrupt.

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d3e322a..96ab160 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -431,14 +431,16 @@ int kvm_lapic_find_highest_irr(struct kvm_vcpu *vcpu)
 }
 
 static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
-			     int vector, int level, int trig_mode);
+			     int vector, int level, int trig_mode,
+			     unsigned long *dest_map);
 
-int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq)
+int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
+		unsigned long *dest_map)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
 	return __apic_accept_irq(apic, irq->delivery_mode, irq->vector,
-			irq->level, irq->trig_mode);
+			irq->level, irq->trig_mode, dest_map);
 }
 
 static int pv_eoi_put_user(struct kvm_vcpu *vcpu, u8 val)
@@ -611,7 +613,7 @@ int kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 }
 
 bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
-		struct kvm_lapic_irq *irq, int *r)
+		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map)
 {
 	struct kvm_apic_map *map;
 	unsigned long bitmap = 1;
@@ -622,7 +624,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 	*r = -1;
 
 	if (irq->shorthand == APIC_DEST_SELF) {
-		*r = kvm_apic_set_irq(src->vcpu, irq);
+		*r = kvm_apic_set_irq(src->vcpu, irq, dest_map);
 		return true;
 	}
 
@@ -667,7 +669,7 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
 			continue;
 		if (*r < 0)
 			*r = 0;
-		*r += kvm_apic_set_irq(dst[i]->vcpu, irq);
+		*r += kvm_apic_set_irq(dst[i]->vcpu, irq, dest_map);
 	}
 
 	ret = true;
@@ -681,7 +683,8 @@ out:
  * Return 1 if successfully added and 0 if discarded.
  */
 static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
-			     int vector, int level, int trig_mode)
+			     int vector, int level, int trig_mode,
+			     unsigned long *dest_map)
 {
 	int result = 0;
 	struct kvm_vcpu *vcpu = apic->vcpu;
@@ -694,6 +697,9 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		if (unlikely(!apic_enabled(apic)))
 			break;
 
+		if (dest_map)
+			__set_bit(vcpu->vcpu_id, dest_map);
+
 		if (trig_mode) {
 			apic_debug("level trig mode for vector %d", vector);
 			apic_set_vector(vector, apic->regs + APIC_TMR);
@@ -852,7 +858,7 @@ static void apic_send_ipi(struct kvm_lapic *apic)
 		   irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode,
 		   irq.vector);
 
-	kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq);
+	kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL);
 }
 
 static u32 apic_get_tmcct(struct kvm_lapic *apic)
@@ -1488,7 +1494,8 @@ int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
 		vector = reg & APIC_VECTOR_MASK;
 		mode = reg & APIC_MODE_MASK;
 		trig_mode = reg & APIC_LVT_LEVEL_TRIGGER;
-		return __apic_accept_irq(apic, mode, vector, 1, trig_mode);
+		return __apic_accept_irq(apic, mode, vector, 1, trig_mode,
+					NULL);
 	}
 	return 0;
 }
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 2c721b9..967519c 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -55,11 +55,12 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu);
 
 int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest);
 int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda);
-int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq);
+int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq,
+		unsigned long *dest_map);
 int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
 
 bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src,
-		struct kvm_lapic_irq *irq, int *r);
+		struct kvm_lapic_irq *irq, int *r, unsigned long *dest_map);
 
 u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
 void kvm_set_apic_base(struct kvm_vcpu *vcpu, u64 data);
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 9379386..8664812 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -220,7 +220,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 	irqe.level = 1;
 	irqe.shorthand = 0;
 
-	return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe);
+	return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
 }
 
 int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 87cd94b..761e5b5 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -92,7 +92,7 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
 void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
 void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
-		struct kvm_lapic_irq *irq);
+		struct kvm_lapic_irq *irq, unsigned long *dest_map);
 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);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index e9073cf..2f07d2e 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -63,7 +63,7 @@ inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
 }
 
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
-		struct kvm_lapic_irq *irq)
+		struct kvm_lapic_irq *irq, unsigned long *dest_map)
 {
 	int i, r = -1;
 	struct kvm_vcpu *vcpu, *lowest = NULL;
@@ -74,7 +74,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		irq->delivery_mode = APIC_DM_FIXED;
 	}
 
-	if (kvm_irq_delivery_to_apic_fast(kvm, src, irq, &r))
+	if (kvm_irq_delivery_to_apic_fast(kvm, src, irq, &r, dest_map))
 		return r;
 
 	kvm_for_each_vcpu(i, vcpu, kvm) {
@@ -88,7 +88,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 		if (!kvm_is_dm_lowest_prio(irq)) {
 			if (r < 0)
 				r = 0;
-			r += kvm_apic_set_irq(vcpu, irq);
+			r += kvm_apic_set_irq(vcpu, irq, dest_map);
 		} else if (kvm_lapic_enabled(vcpu)) {
 			if (!lowest)
 				lowest = vcpu;
@@ -98,7 +98,7 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	}
 
 	if (lowest)
-		r = kvm_apic_set_irq(lowest, irq);
+		r = kvm_apic_set_irq(lowest, irq, dest_map);
 
 	return r;
 }
@@ -130,7 +130,7 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
 
 	kvm_set_msi_irq(e, &irq);
 
-	return kvm_irq_delivery_to_apic(kvm, NULL, &irq);
+	return kvm_irq_delivery_to_apic(kvm, NULL, &irq, NULL);
 }
 
 
@@ -142,7 +142,7 @@ static int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *e,
 
 	kvm_set_msi_irq(e, &irq);
 
-	if (kvm_irq_delivery_to_apic_fast(kvm, NULL, &irq, &r))
+	if (kvm_irq_delivery_to_apic_fast(kvm, NULL, &irq, &r, NULL))
 		return r;
 	else
 		return -EWOULDBLOCK;
-- 
1.7.1

[PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Yang Zhang]

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

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 96ab160..9c041fa 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));
@@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu,
 	apic->highest_isr_cache = -1;
 	kvm_x86_ops->hwapic_isr_update(vcpu->kvm, apic_find_highest_isr(apic));
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
+	kvm_rtc_irq_restore(vcpu);
 }
 
 void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 967519c..004d2ad 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
 	return vcpu->arch.apic->pending_events;
 }
 
+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 8664812..0b12b17 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
 	return result;
 }
 
+static void rtc_irq_reset(struct kvm_ioapic *ioapic)
+{
+	ioapic->rtc_status.pending_eoi = 0;
+	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
+}
+
+static void rtc_irq_restore(struct kvm_ioapic *ioapic)
+{
+	struct kvm_vcpu *vcpu;
+	int vector, i, pending_eoi = 0;
+
+	if (RTC_GSI >= IOAPIC_NUM_PINS)
+		return;
+
+	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
+	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
+		if (kvm_apic_pending_eoi(vcpu, vector)) {
+			pending_eoi++;
+			__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
+		}
+	}
+	ioapic->rtc_status.pending_eoi = pending_eoi;
+}
+
+void kvm_rtc_irq_restore(struct kvm_vcpu *vcpu)
+{
+	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
+	int vector;
+
+	if (!ioapic)
+		return;
+
+	spin_lock(&ioapic->lock);
+	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
+	if (kvm_apic_pending_eoi(vcpu, vector)) {
+		__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
+		ioapic->rtc_status.pending_eoi++;
+	}
+	spin_unlock(&ioapic->lock);
+}
+
 static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
 {
 	union kvm_ioapic_redirect_entry *pent;
@@ -431,6 +472,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);
 }
 
@@ -496,6 +538,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_ioapic_make_eoibitmap_request(kvm);
 	spin_unlock(&ioapic->lock);
 	return 0;
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 761e5b5..2692873 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -79,6 +79,7 @@ static inline struct kvm_ioapic *ioapic_irqchip(struct kvm *kvm)
 	return kvm->arch.vioapic;
 }
 
+void kvm_rtc_irq_restore(struct kvm_vcpu *vcpu);
 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);
-- 
1.7.1

[PATCH v7 5/7] KVM : Force vmexit with virtual interrupt delivery

[Yang Zhang]

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

Need the EOI to track interrupt deliver status, so force vmexit
on EOI for rtc interrupt when enabling virtual interrupt delivery.

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 0b12b17..d01544a 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -175,7 +175,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 == RTC_GSI)) {
 			irqe.dest_id = e->fields.dest_id;
 			irqe.vector = e->fields.vector;
 			irqe.dest_mode = e->fields.dest_mode;
-- 
1.7.1

[PATCH v7 6/7] KVM: Let ioapic know the irq line status

[Yang Zhang]

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

Userspace may deliver RTC interrupt without query the status. So we
want to track RTC EOI for this case.

Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
---
 arch/x86/kvm/i8254.c     |    4 ++--
 arch/x86/kvm/x86.c       |    6 ++++--
 include/linux/kvm_host.h |   11 +++++++----
 virt/kvm/assigned-dev.c  |   13 +++++++------
 virt/kvm/eventfd.c       |   15 +++++++++------
 virt/kvm/ioapic.c        |   18 ++++++++++--------
 virt/kvm/ioapic.h        |    2 +-
 virt/kvm/irq_comm.c      |   19 ++++++++++++-------
 virt/kvm/kvm_main.c      |    3 ++-
 9 files changed, 54 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
index c1d30b2..412a5aa 100644
--- a/arch/x86/kvm/i8254.c
+++ b/arch/x86/kvm/i8254.c
@@ -290,8 +290,8 @@ static void pit_do_work(struct kthread_work *work)
 	}
 	spin_unlock(&ps->inject_lock);
 	if (inject) {
-		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1);
-		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0);
+		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 1, false);
+		kvm_set_irq(kvm, kvm->arch.vpit->irq_source_id, 0, 0, false);
 
 		/*
 		 * Provides NMI watchdog support via Virtual Wire mode.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2aaba81..5e85d8d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3484,13 +3484,15 @@ out:
 	return r;
 }
 
-int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_event)
+int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_event,
+			bool line_status)
 {
 	if (!irqchip_in_kernel(kvm))
 		return -ENXIO;
 
 	irq_event->status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
-					irq_event->irq, irq_event->level);
+					irq_event->irq, irq_event->level,
+					line_status);
 	return 0;
 }
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 1c0be23..7bcdb6b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -289,7 +289,8 @@ struct kvm_kernel_irq_routing_entry {
 	u32 gsi;
 	u32 type;
 	int (*set)(struct kvm_kernel_irq_routing_entry *e,
-		   struct kvm *kvm, int irq_source_id, int level);
+		   struct kvm *kvm, int irq_source_id, int level,
+		   bool line_status);
 	union {
 		struct {
 			unsigned irqchip;
@@ -588,7 +589,8 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 
 int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 				   struct kvm_userspace_memory_region *mem);
-int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level);
+int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level,
+			bool line_status);
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg);
 
@@ -719,10 +721,11 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 				   union kvm_ioapic_redirect_entry *entry,
 				   unsigned long *deliver_bitmask);
 #endif
-int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
+int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
+		bool line_status);
 int kvm_set_irq_inatomic(struct kvm *kvm, int irq_source_id, u32 irq, int level);
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
-		int irq_source_id, int level);
+		int irq_source_id, int level, bool line_status);
 bool kvm_irq_has_notifier(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index 3642239..f4c7f59 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -80,11 +80,12 @@ kvm_assigned_dev_raise_guest_irq(struct kvm_assigned_dev_kernel *assigned_dev,
 		spin_lock(&assigned_dev->intx_mask_lock);
 		if (!(assigned_dev->flags & KVM_DEV_ASSIGN_MASK_INTX))
 			kvm_set_irq(assigned_dev->kvm,
-				    assigned_dev->irq_source_id, vector, 1);
+				    assigned_dev->irq_source_id, vector, 1,
+				    false);
 		spin_unlock(&assigned_dev->intx_mask_lock);
 	} else
 		kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
-			    vector, 1);
+			    vector, 1, false);
 }
 
 static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
@@ -165,7 +166,7 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 		container_of(kian, struct kvm_assigned_dev_kernel,
 			     ack_notifier);
 
-	kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 0);
+	kvm_set_irq(dev->kvm, dev->irq_source_id, dev->guest_irq, 0, false);
 
 	spin_lock(&dev->intx_mask_lock);
 
@@ -188,7 +189,7 @@ static void kvm_assigned_dev_ack_irq(struct kvm_irq_ack_notifier *kian)
 
 		if (reassert)
 			kvm_set_irq(dev->kvm, dev->irq_source_id,
-				    dev->guest_irq, 1);
+				    dev->guest_irq, 1, false);
 	}
 
 	spin_unlock(&dev->intx_mask_lock);
@@ -202,7 +203,7 @@ static void deassign_guest_irq(struct kvm *kvm,
 						&assigned_dev->ack_notifier);
 
 	kvm_set_irq(assigned_dev->kvm, assigned_dev->irq_source_id,
-		    assigned_dev->guest_irq, 0);
+		    assigned_dev->guest_irq, 0, false);
 
 	if (assigned_dev->irq_source_id != -1)
 		kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
@@ -901,7 +902,7 @@ static int kvm_vm_ioctl_set_pci_irq_mask(struct kvm *kvm,
 	if (match->irq_requested_type & KVM_DEV_IRQ_GUEST_INTX) {
 		if (assigned_dev->flags & KVM_DEV_ASSIGN_MASK_INTX) {
 			kvm_set_irq(match->kvm, match->irq_source_id,
-				    match->guest_irq, 0);
+				    match->guest_irq, 0, false);
 			/*
 			 * Masking at hardware-level is performed on demand,
 			 * i.e. when an IRQ actually arrives at the host.
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 020522e..36512f3 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -100,11 +100,13 @@ irqfd_inject(struct work_struct *work)
 	struct kvm *kvm = irqfd->kvm;
 
 	if (!irqfd->resampler) {
-		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1,
+				false);
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0,
+				false);
 	} else
 		kvm_set_irq(kvm, KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID,
-			    irqfd->gsi, 1);
+			    irqfd->gsi, 1, false);
 }
 
 /*
@@ -121,7 +123,7 @@ irqfd_resampler_ack(struct kvm_irq_ack_notifier *kian)
 	resampler = container_of(kian, struct _irqfd_resampler, notifier);
 
 	kvm_set_irq(resampler->kvm, KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID,
-		    resampler->notifier.gsi, 0);
+		    resampler->notifier.gsi, 0, false);
 
 	rcu_read_lock();
 
@@ -146,7 +148,7 @@ irqfd_resampler_shutdown(struct _irqfd *irqfd)
 		list_del(&resampler->link);
 		kvm_unregister_irq_ack_notifier(kvm, &resampler->notifier);
 		kvm_set_irq(kvm, KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID,
-			    resampler->notifier.gsi, 0);
+			    resampler->notifier.gsi, 0, false);
 		kfree(resampler);
 	}
 
@@ -225,7 +227,8 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 		irq = rcu_dereference(irqfd->irq_entry);
 		/* An event has been signaled, inject an interrupt */
 		if (irq)
-			kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+			kvm_set_msi(irq, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1,
+					false);
 		else
 			schedule_work(&irqfd->inject);
 		rcu_read_unlock();
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index d01544a..ea26a5a 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -50,7 +50,8 @@
 #else
 #define ioapic_debug(fmt, arg...)
 #endif
-static int ioapic_deliver(struct kvm_ioapic *vioapic, int irq);
+static int ioapic_deliver(struct kvm_ioapic *vioapic, int irq,
+		bool line_status);
 
 static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
 					  unsigned long addr,
@@ -131,7 +132,8 @@ void kvm_rtc_irq_restore(struct kvm_vcpu *vcpu)
 	spin_unlock(&ioapic->lock);
 }
 
-static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
+static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx,
+		bool line_status)
 {
 	union kvm_ioapic_redirect_entry *pent;
 	int injected = -1;
@@ -139,7 +141,7 @@ static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
 	pent = &ioapic->redirtbl[idx];
 
 	if (!pent->fields.mask) {
-		injected = ioapic_deliver(ioapic, idx);
+		injected = ioapic_deliver(ioapic, idx, line_status);
 		if (injected && pent->fields.trig_mode == IOAPIC_LEVEL_TRIG)
 			pent->fields.remote_irr = 1;
 	}
@@ -236,13 +238,13 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
 			kvm_fire_mask_notifiers(ioapic->kvm, KVM_IRQCHIP_IOAPIC, index, mask_after);
 		if (e->fields.trig_mode == IOAPIC_LEVEL_TRIG
 		    && ioapic->irr & (1 << index))
-			ioapic_service(ioapic, index);
+			ioapic_service(ioapic, index, false);
 		kvm_ioapic_make_eoibitmap_request(ioapic->kvm);
 		break;
 	}
 }
 
-static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
+static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool line_status)
 {
 	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
 	struct kvm_lapic_irq irqe;
@@ -265,7 +267,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 }
 
 int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
-		       int level)
+		       int level, bool line_status)
 {
 	u32 old_irr;
 	u32 mask = 1 << irq;
@@ -288,7 +290,7 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
 		ioapic->irr |= mask;
 		if ((edge && old_irr != ioapic->irr) ||
 		    (!edge && !entry.fields.remote_irr))
-			ret = ioapic_service(ioapic, irq);
+			ret = ioapic_service(ioapic, irq, line_status);
 		else
 			ret = 0; /* report coalesced interrupt */
 	}
@@ -337,7 +339,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
 		ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG);
 		ent->fields.remote_irr = 0;
 		if (!ent->fields.mask && (ioapic->irr & (1 << i)))
-			ioapic_service(ioapic, i);
+			ioapic_service(ioapic, i, false);
 	}
 }
 
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 2692873..2f48d92 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -89,7 +89,7 @@ bool kvm_ioapic_handles_vector(struct kvm *kvm, int vector);
 int kvm_ioapic_init(struct kvm *kvm);
 void kvm_ioapic_destroy(struct kvm *kvm);
 int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
-		       int level);
+		       int level, bool line_status);
 void kvm_ioapic_clear_all(struct kvm_ioapic *ioapic, int irq_source_id);
 void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
 int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index 2f07d2e..8efb580 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -35,7 +35,8 @@
 #include "ioapic.h"
 
 static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
-			   struct kvm *kvm, int irq_source_id, int level)
+			   struct kvm *kvm, int irq_source_id, int level,
+			   bool line_status)
 {
 #ifdef CONFIG_X86
 	struct kvm_pic *pic = pic_irqchip(kvm);
@@ -46,10 +47,12 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
 }
 
 static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
-			      struct kvm *kvm, int irq_source_id, int level)
+			      struct kvm *kvm, int irq_source_id, int level,
+			      bool line_status)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
-	return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level);
+	return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level,
+				line_status);
 }
 
 inline static bool kvm_is_dm_lowest_prio(struct kvm_lapic_irq *irq)
@@ -121,7 +124,7 @@ static inline void kvm_set_msi_irq(struct kvm_kernel_irq_routing_entry *e,
 }
 
 int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
-		struct kvm *kvm, int irq_source_id, int level)
+		struct kvm *kvm, int irq_source_id, int level, bool line_status)
 {
 	struct kvm_lapic_irq irq;
 
@@ -159,7 +162,7 @@ int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
 	route.msi.address_hi = msi->address_hi;
 	route.msi.data = msi->data;
 
-	return kvm_set_msi(&route, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+	return kvm_set_msi(&route, kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1, false);
 }
 
 /*
@@ -168,7 +171,8 @@ int kvm_send_userspace_msi(struct kvm *kvm, struct kvm_msi *msi)
  *  = 0   Interrupt was coalesced (previous irq is still pending)
  *  > 0   Number of CPUs interrupt was delivered to
  */
-int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)
+int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,
+		bool line_status)
 {
 	struct kvm_kernel_irq_routing_entry *e, irq_set[KVM_NR_IRQCHIPS];
 	int ret = -1, i = 0;
@@ -189,7 +193,8 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)
 
 	while(i--) {
 		int r;
-		r = irq_set[i].set(&irq_set[i], kvm, irq_source_id, level);
+		r = irq_set[i].set(&irq_set[i], kvm, irq_source_id, level,
+				line_status);
 		if (r < 0)
 			continue;
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ff71541..6c4842a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2258,7 +2258,8 @@ static long kvm_vm_ioctl(struct file *filp,
 		if (copy_from_user(&irq_event, argp, sizeof irq_event))
 			goto out;
 
-		r = kvm_vm_ioctl_irq_line(kvm, &irq_event);
+		r = kvm_vm_ioctl_irq_line(kvm, &irq_event,
+					ioctl == KVM_IRQ_LINE_STATUS);
 		if (r)
 			goto out;
 
-- 
1.7.1

[PATCH v7 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 pending_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 |   40 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index ea26a5a..96a226d 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -132,6 +132,29 @@ void kvm_rtc_irq_restore(struct kvm_vcpu *vcpu)
 	spin_unlock(&ioapic->lock);
 }
 
+static void rtc_irq_ack_eoi(struct kvm_ioapic *ioapic, struct kvm_vcpu *vcpu,
+			int irq)
+{
+	if (irq != RTC_GSI)
+		return;
+
+	if (test_and_clear_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map))
+		--ioapic->rtc_status.pending_eoi;
+
+	WARN_ON(ioapic->rtc_status.pending_eoi < 0);
+}
+
+static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq, bool line_status)
+{
+	if (irq != RTC_GSI || !line_status)
+		return false;
+
+	if (ioapic->rtc_status.pending_eoi > 0)
+		return true; /* coalesced */
+
+	return false;
+}
+
 static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx,
 		bool line_status)
 {
@@ -248,6 +271,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool line_status)
 {
 	union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq];
 	struct kvm_lapic_irq irqe;
+	int ret;
 
 	ioapic_debug("dest=%x dest_mode=%x delivery_mode=%x "
 		     "vector=%x trig_mode=%x\n",
@@ -263,7 +287,14 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq, bool line_status)
 	irqe.level = 1;
 	irqe.shorthand = 0;
 
-	return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
+	if (irq == RTC_GSI && line_status) {
+		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
+				ioapic->rtc_status.dest_map);
+		ioapic->rtc_status.pending_eoi = ret;
+	} else
+		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
+
+	return ret;
 }
 
 int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
@@ -287,6 +318,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, line_status)) {
+			ret = 0; /* coalesced */
+			goto out;
+		}
 		ioapic->irr |= mask;
 		if ((edge && old_irr != ioapic->irr) ||
 		    (!edge && !entry.fields.remote_irr))
@@ -294,6 +330,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);
 
@@ -321,6 +358,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
 		if (ent->fields.vector != vector)
 			continue;
 
+		rtc_irq_ack_eoi(ioapic, vcpu, i);
 		/*
 		 * We are dropping lock while calling ack notifiers because ack
 		 * notifier callbacks for assigned devices call into IOAPIC
-- 
1.7.1

Re: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Gleb Natapov]

On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> ---
>  arch/x86/kvm/lapic.c |    9 +++++++++
>  arch/x86/kvm/lapic.h |    2 ++
>  virt/kvm/ioapic.c    |   43 +++++++++++++++++++++++++++++++++++++++++++
>  virt/kvm/ioapic.h    |    1 +
>  4 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 96ab160..9c041fa 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));
> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu *vcpu,
>  	apic->highest_isr_cache = -1;
>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm, apic_find_highest_isr(apic));
>  	kvm_make_request(KVM_REQ_EVENT, vcpu);
> +	kvm_rtc_irq_restore(vcpu);
>  }
>  
>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 967519c..004d2ad 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct kvm_vcpu *vcpu)
>  	return vcpu->arch.apic->pending_events;
>  }
>  
> +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 8664812..0b12b17 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
>  	return result;
>  }
>  
> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
> +{
> +	ioapic->rtc_status.pending_eoi = 0;
> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
> +}
> +
> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> +{
> +	struct kvm_vcpu *vcpu;
> +	int vector, i, pending_eoi = 0;
> +
> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
> +		return;
> +
> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
> +			pending_eoi++;
> +			__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
You should cleat dest_map at the beginning to get rid of stale bits.

> +		}
> +	}
> +	ioapic->rtc_status.pending_eoi = pending_eoi;
> +}
> +
> +void kvm_rtc_irq_restore(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
> +	int vector;
> +
> +	if (!ioapic)
> +		return;
> +
Can this be called if ioapic == NULL?
Should check for if (RTC_GSI >= IOAPIC_NUM_PINS) here too.

> +	spin_lock(&ioapic->lock);
> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
> +	if (kvm_apic_pending_eoi(vcpu, vector)) {
> +		__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> +		ioapic->rtc_status.pending_eoi++;
The bit may have been set already. The logic should be:

new_val = kvm_apic_pending_eoi(vcpu, vector)
old_val = set_bit(vcpu_id, dest_map)

if (new_val == old_val)
	return;

if (new_val) {
	__set_bit(vcpu_id, dest_map);
	pending_eoi++;
} else {
	__clear_bit(vcpu_id, dest_map);
	pending_eoi--;
}

The naming of above two functions are not good either. Call
them something like kvm_rtc_eoi_tracking_restore_all() and
kvm_rtc_eoi_tracking_restore_one().  And _all should call _one() for
each vcpu. Make __rtc_irq_eoi_tracking_restore_one() that does not
take ioapic lock and call it from kvm_rtc_eoi_tracking_restore_one()
surrounded by locks.

--
			Gleb.

RE: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-04-04:
> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>> ---
>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h |    2 ++
>>  virt/kvm/ioapic.c    |   43
>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h    |   
>>  1 + 4 files changed, 55 insertions(+), 0 deletions(-)
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 96ab160..9c041fa 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));
>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu
> *vcpu,
>>  	apic->highest_isr_cache = -1;
>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT, vcpu);
>>  +	kvm_rtc_irq_restore(vcpu); }
>>  
>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>> index 967519c..004d2ad 100644
>> --- a/arch/x86/kvm/lapic.h
>> +++ b/arch/x86/kvm/lapic.h
>> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct
> kvm_vcpu *vcpu)
>>  	return vcpu->arch.apic->pending_events;
>>  }
>> +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 8664812..0b12b17 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct
> kvm_ioapic *ioapic,
>>  	return result;
>>  }
>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
>> +{
>> +	ioapic->rtc_status.pending_eoi = 0;
>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
>> +}
>> +
>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>> +{
>> +	struct kvm_vcpu *vcpu;
>> +	int vector, i, pending_eoi = 0;
>> +
>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
>> +		return;
>> +
>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
>> +			pending_eoi++;
>> +			__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> You should cleat dest_map at the beginning to get rid of stale bits.
I thought kvm_set_ioapic is called only after save/restore or migration. And the ioapic should be reset successfully before call it. So the dest_map is empty before call rtc_irq_restore().
But it is possible kvm_set_ioapic is called beside save/restore or migration. Right?

> 
>> +		}
>> +	}
>> +	ioapic->rtc_status.pending_eoi = pending_eoi;
>> +}
>> +
>> +void kvm_rtc_irq_restore(struct kvm_vcpu *vcpu)
>> +{
>> +	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
>> +	int vector;
>> +
>> +	if (!ioapic)
>> +		return;
>> +
> Can this be called if ioapic == NULL?
Yes. IIRC, unit test will test lapic function without ioapic.

> Should check for if (RTC_GSI >= IOAPIC_NUM_PINS) here too.
Not necessary. kvm_rtc_irq_restore is called from "arch/x86/" and we have the defination:
#ifdef CONFIG_X86
#define RTC_GSI 8

The check will be false always. As the logic you suggested below, this check is necessary for _all() not _one();

> 
>> +	spin_lock(&ioapic->lock);
>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
>> +	if (kvm_apic_pending_eoi(vcpu, vector)) {
>> +		__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
>> +		ioapic->rtc_status.pending_eoi++;
> The bit may have been set already. The logic should be:
Right.

>
> 
> new_val = kvm_apic_pending_eoi(vcpu, vector)
> old_val = set_bit(vcpu_id, dest_map)
> 
> if (new_val == old_val)
> 	return;
> 
> if (new_val) {
> 	__set_bit(vcpu_id, dest_map);
> 	pending_eoi++;
> } else {
> 	__clear_bit(vcpu_id, dest_map);
> 	pending_eoi--;
> }
> 
> The naming of above two functions are not good either. Call
> them something like kvm_rtc_eoi_tracking_restore_all() and
> kvm_rtc_eoi_tracking_restore_one().  And _all should call _one() for
> each vcpu. Make __rtc_irq_eoi_tracking_restore_one() that does not
> take ioapic lock and call it from kvm_rtc_eoi_tracking_restore_one()
> surrounded by locks.
Ok. Just confirm whether I am understanding correct:

kvm_rtc_eoi_tracking_restore_all():
{
for_each_vcpu:
	kvm_rtc_eoi_tracking_restore_one():
}

kvm_rtc_eoi_tracking_restore_one():
{
lock();
__rtc_irq_eoi_tracking_restore_one():
unlock();
}

kvm_set_ioapic()
{
kvm_rtc_eoi_tracking_restore_all();
}

kvm_apic_post_state_restore()
{
kvm_rtc_eoi_tracking_restore_one();
}

> 
> --
> 			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 v7 4/7] KVM: Add reset/restore rtc_status support

[Gleb Natapov]

On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-04-04:
> > On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
> >> From: Yang Zhang <yang.z.zhang@Intel.com>
> >> 
> >> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >> ---
> >>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h |    2 ++
> >>  virt/kvm/ioapic.c    |   43
> >>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h    |   
> >>  1 + 4 files changed, 55 insertions(+), 0 deletions(-)
> >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >> index 96ab160..9c041fa 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));
> >> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct kvm_vcpu
> > *vcpu,
> >>  	apic->highest_isr_cache = -1;
> >>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> >>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT, vcpu);
> >>  +	kvm_rtc_irq_restore(vcpu); }
> >>  
> >>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
> >> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> >> index 967519c..004d2ad 100644
> >> --- a/arch/x86/kvm/lapic.h
> >> +++ b/arch/x86/kvm/lapic.h
> >> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct
> > kvm_vcpu *vcpu)
> >>  	return vcpu->arch.apic->pending_events;
> >>  }
> >> +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 8664812..0b12b17 100644
> >> --- a/virt/kvm/ioapic.c
> >> +++ b/virt/kvm/ioapic.c
> >> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct
> > kvm_ioapic *ioapic,
> >>  	return result;
> >>  }
> >> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
> >> +{
> >> +	ioapic->rtc_status.pending_eoi = 0;
> >> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
> >> +}
> >> +
> >> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> >> +{
> >> +	struct kvm_vcpu *vcpu;
> >> +	int vector, i, pending_eoi = 0;
> >> +
> >> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
> >> +		return;
> >> +
> >> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
> >> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
> >> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
> >> +			pending_eoi++;
> >> +			__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> > You should cleat dest_map at the beginning to get rid of stale bits.
> I thought kvm_set_ioapic is called only after save/restore or migration. And the ioapic should be reset successfully before call it. So the dest_map is empty before call rtc_irq_restore().
> But it is possible kvm_set_ioapic is called beside save/restore or migration. Right?
> 
First of all userspace should not care when it calls kvm_set_ioapic()
the kernel need to do the right thing. Second, believe it or not,
kvm_ioapic_reset() is not called during system reset. Instead userspace
reset it by calling kvm_set_ioapic() with ioapic state after reset.

> > 
> >> +		}
> >> +	}
> >> +	ioapic->rtc_status.pending_eoi = pending_eoi;
> >> +}
> >> +
> >> +void kvm_rtc_irq_restore(struct kvm_vcpu *vcpu)
> >> +{
> >> +	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
> >> +	int vector;
> >> +
> >> +	if (!ioapic)
> >> +		return;
> >> +
> > Can this be called if ioapic == NULL?
> Yes. IIRC, unit test will test lapic function without ioapic.
Unit test is a guest code. This has nothing to do with a guest code.
Unit test is not the one who creates lapic.

> 
> > Should check for if (RTC_GSI >= IOAPIC_NUM_PINS) here too.
> Not necessary. kvm_rtc_irq_restore is called from "arch/x86/" and we have the defination:
> #ifdef CONFIG_X86
> #define RTC_GSI 8
> 
> The check will be false always. As the logic you suggested below, this check is necessary for _all() not _one();
OK.

> 
> > 
> >> +	spin_lock(&ioapic->lock);
> >> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
> >> +	if (kvm_apic_pending_eoi(vcpu, vector)) {
> >> +		__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> >> +		ioapic->rtc_status.pending_eoi++;
> > The bit may have been set already. The logic should be:
> Right.
> 
> >
> > 
> > new_val = kvm_apic_pending_eoi(vcpu, vector)
> > old_val = set_bit(vcpu_id, dest_map)
> > 
> > if (new_val == old_val)
> > 	return;
> > 
> > if (new_val) {
> > 	__set_bit(vcpu_id, dest_map);
> > 	pending_eoi++;
> > } else {
> > 	__clear_bit(vcpu_id, dest_map);
> > 	pending_eoi--;
> > }
> > 
> > The naming of above two functions are not good either. Call
> > them something like kvm_rtc_eoi_tracking_restore_all() and
> > kvm_rtc_eoi_tracking_restore_one().  And _all should call _one() for
> > each vcpu. Make __rtc_irq_eoi_tracking_restore_one() that does not
> > take ioapic lock and call it from kvm_rtc_eoi_tracking_restore_one()
> > surrounded by locks.
> Ok. Just confirm whether I am understanding correct:
> 
> kvm_rtc_eoi_tracking_restore_all():
> {
> for_each_vcpu:
> 	kvm_rtc_eoi_tracking_restore_one():
        __rtc_irq_eoi_tracking_restore_one();
Since caller will have the lock already.

> }
> 
> kvm_rtc_eoi_tracking_restore_one():
> {
> lock();
> __rtc_irq_eoi_tracking_restore_one():
> unlock();
> }
> 
> kvm_set_ioapic()
> {
> kvm_rtc_eoi_tracking_restore_all();
> }
> 
> kvm_apic_post_state_restore()
> {
> kvm_rtc_eoi_tracking_restore_one();
> }
> 
Yes.

--
			Gleb.

RE: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-04-07:
> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-04-04:
>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>> 
>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>> ---
>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h |    2 ++
>>>>  virt/kvm/ioapic.c    |   43
>>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h | 1 +
>>>>  4 files changed, 55 insertions(+), 0 deletions(-)
>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>> index 96ab160..9c041fa 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));
>>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
> kvm_vcpu
>>> *vcpu,
>>>>  	apic->highest_isr_cache = -1;
>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
>>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
>>>>  
>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>>>> index 967519c..004d2ad 100644
>>>> --- a/arch/x86/kvm/lapic.h
>>>> +++ b/arch/x86/kvm/lapic.h
>>>> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct
>>> kvm_vcpu *vcpu)
>>>>  	return vcpu->arch.apic->pending_events;
>>>>  }
>>>> +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 8664812..0b12b17 100644
>>>> --- a/virt/kvm/ioapic.c
>>>> +++ b/virt/kvm/ioapic.c
>>>> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct
>>> kvm_ioapic *ioapic,
>>>>  	return result;
>>>>  }
>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
>>>> +{
>>>> +	ioapic->rtc_status.pending_eoi = 0;
>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
>>>> +}
>>>> +
>>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>>>> +{
>>>> +	struct kvm_vcpu *vcpu;
>>>> +	int vector, i, pending_eoi = 0;
>>>> +
>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
>>>> +		return;
>>>> +
>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
>>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
>>>> +			pending_eoi++;
>>>> +			__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
>>> You should cleat dest_map at the beginning to get rid of stale bits.
>> I thought kvm_set_ioapic is called only after save/restore or migration. And the
> ioapic should be reset successfully before call it. So the dest_map is empty
> before call rtc_irq_restore().
>> But it is possible kvm_set_ioapic is called beside save/restore or
>> migration. Right?
>> 
> First of all userspace should not care when it calls kvm_set_ioapic()
> the kernel need to do the right thing. Second, believe it or not,
> kvm_ioapic_reset() is not called during system reset. Instead userspace
> reset it by calling kvm_set_ioapic() with ioapic state after reset.
Ok. I see. As the logic you suggested, it will clear dest_map if no pending eoi in vcpu, so we don't need to do it again.

> 
>>> 
>>>> +		}
>>>> +	}
>>>> +	ioapic->rtc_status.pending_eoi = pending_eoi;
>>>> +}
>>>> +
>>>> +void kvm_rtc_irq_restore(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	struct kvm_ioapic *ioapic = vcpu->kvm->arch.vioapic;
>>>> +	int vector;
>>>> +
>>>> +	if (!ioapic)
>>>> +		return;
>>>> +
>>> Can this be called if ioapic == NULL?
>> Yes. IIRC, unit test will test lapic function without ioapic.
> Unit test is a guest code. This has nothing to do with a guest code.
> Unit test is not the one who creates lapic.
Ok. Then !ioapic is unnecessary.

>> 
>>> Should check for if (RTC_GSI >= IOAPIC_NUM_PINS) here too.
>> Not necessary. kvm_rtc_irq_restore is called from "arch/x86/" and we
>> have the defination: #ifdef CONFIG_X86 #define RTC_GSI 8
>> 
>> The check will be false always. As the logic you suggested below, this check is
> necessary for _all() not _one();
> OK.
> 
>> 
>>> 
>>>> +	spin_lock(&ioapic->lock);
>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
>>>> +	if (kvm_apic_pending_eoi(vcpu, vector)) {
>>>> +		__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
>>>> +		ioapic->rtc_status.pending_eoi++;
>>> The bit may have been set already. The logic should be:
>> Right.
>> 
>>> 
>>> 
>>> new_val = kvm_apic_pending_eoi(vcpu, vector)
>>> old_val = set_bit(vcpu_id, dest_map)
>>> 
>>> if (new_val == old_val)
>>> 	return;
>>> 
>>> if (new_val) {
>>> 	__set_bit(vcpu_id, dest_map);
>>> 	pending_eoi++;
>>> } else {
>>> 	__clear_bit(vcpu_id, dest_map);
>>> 	pending_eoi--;
>>> }
>>> 
>>> The naming of above two functions are not good either. Call
>>> them something like kvm_rtc_eoi_tracking_restore_all() and
>>> kvm_rtc_eoi_tracking_restore_one().  And _all should call _one() for
>>> each vcpu. Make __rtc_irq_eoi_tracking_restore_one() that does not
>>> take ioapic lock and call it from kvm_rtc_eoi_tracking_restore_one()
>>> surrounded by locks.
>> Ok. Just confirm whether I am understanding correct:
>> 
>> kvm_rtc_eoi_tracking_restore_all():
>> {
>> for_each_vcpu:
>> 	kvm_rtc_eoi_tracking_restore_one():
>         __rtc_irq_eoi_tracking_restore_one();
> Since caller will have the lock already.
> 
>> }
>> 
>> kvm_rtc_eoi_tracking_restore_one():
>> {
>> lock();
>> __rtc_irq_eoi_tracking_restore_one():
>> unlock();
>> }
>> 
>> kvm_set_ioapic()
>> {
>> kvm_rtc_eoi_tracking_restore_all();
>> }
>> 
>> kvm_apic_post_state_restore()
>> {
>> kvm_rtc_eoi_tracking_restore_one();
>> }
>> 
> Yes.
> 
> --
> 			Gleb.


Best regards,
Yang

Re: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Gleb Natapov]

On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-04-07:
> > On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-04-04:
> >>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
> >>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>> 
> >>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >>>> ---
> >>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h |    2 ++
> >>>>  virt/kvm/ioapic.c    |   43
> >>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h | 1 +
> >>>>  4 files changed, 55 insertions(+), 0 deletions(-)
> >>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >>>> index 96ab160..9c041fa 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));
> >>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
> > kvm_vcpu
> >>> *vcpu,
> >>>>  	apic->highest_isr_cache = -1;
> >>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> >>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
> >>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
> >>>>  
> >>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
> >>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> >>>> index 967519c..004d2ad 100644
> >>>> --- a/arch/x86/kvm/lapic.h
> >>>> +++ b/arch/x86/kvm/lapic.h
> >>>> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct
> >>> kvm_vcpu *vcpu)
> >>>>  	return vcpu->arch.apic->pending_events;
> >>>>  }
> >>>> +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 8664812..0b12b17 100644
> >>>> --- a/virt/kvm/ioapic.c
> >>>> +++ b/virt/kvm/ioapic.c
> >>>> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct
> >>> kvm_ioapic *ioapic,
> >>>>  	return result;
> >>>>  }
> >>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
> >>>> +{
> >>>> +	ioapic->rtc_status.pending_eoi = 0;
> >>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
> >>>> +}
> >>>> +
> >>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> >>>> +{
> >>>> +	struct kvm_vcpu *vcpu;
> >>>> +	int vector, i, pending_eoi = 0;
> >>>> +
> >>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
> >>>> +		return;
> >>>> +
> >>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
> >>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
> >>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
> >>>> +			pending_eoi++;
> >>>> +			__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> >>> You should cleat dest_map at the beginning to get rid of stale bits.
> >> I thought kvm_set_ioapic is called only after save/restore or migration. And the
> > ioapic should be reset successfully before call it. So the dest_map is empty
> > before call rtc_irq_restore().
> >> But it is possible kvm_set_ioapic is called beside save/restore or
> >> migration. Right?
> >> 
> > First of all userspace should not care when it calls kvm_set_ioapic()
> > the kernel need to do the right thing. Second, believe it or not,
> > kvm_ioapic_reset() is not called during system reset. Instead userspace
> > reset it by calling kvm_set_ioapic() with ioapic state after reset.
> Ok. I see. As the logic you suggested, it will clear dest_map if no pending eoi in vcpu, so we don't need to do it again.
> 
You again rely on userspace doing thing in certain manner. What is
set_lapic() is never called? Kernel internal state have to be correct
after each ioctl call.

--
			Gleb.

RE: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-04-07:
> On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-04-07:
>>> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-04-04:
>>>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>> 
>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>> ---
>>>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h |    2
>>>>>>  ++ virt/kvm/ioapic.c    |   43
>>>>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h | 1
>>>>>>  + 4 files changed, 55 insertions(+), 0 deletions(-)
>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>>>> index 96ab160..9c041fa 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));
>>>>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
>>> kvm_vcpu
>>>>> *vcpu,
>>>>>>  	apic->highest_isr_cache = -1;
>>>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>>>>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
>>>>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
>>>>>>  
>>>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
>>>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>>>>>> index 967519c..004d2ad 100644
>>>>>> --- a/arch/x86/kvm/lapic.h
>>>>>> +++ b/arch/x86/kvm/lapic.h
>>>>>> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct
>>>>> kvm_vcpu *vcpu)
>>>>>>  	return vcpu->arch.apic->pending_events;
>>>>>>  }
>>>>>> +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 8664812..0b12b17 100644
>>>>>> --- a/virt/kvm/ioapic.c
>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct
>>>>> kvm_ioapic *ioapic,
>>>>>>  	return result;
>>>>>>  }
>>>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
>>>>>> +{
>>>>>> +	ioapic->rtc_status.pending_eoi = 0;
>>>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
>>>>>> +}
>>>>>> +
>>>>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>>>>>> +{
>>>>>> +	struct kvm_vcpu *vcpu;
>>>>>> +	int vector, i, pending_eoi = 0;
>>>>>> +
>>>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
>>>>>> +		return;
>>>>>> +
>>>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
>>>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
>>>>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
>>>>>> +			pending_eoi++;
>>>>>> +			__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
>>>>> You should cleat dest_map at the beginning to get rid of stale bits.
>>>> I thought kvm_set_ioapic is called only after save/restore or migration. And
> the
>>> ioapic should be reset successfully before call it. So the dest_map is empty
>>> before call rtc_irq_restore().
>>>> But it is possible kvm_set_ioapic is called beside save/restore or
>>>> migration. Right?
>>>> 
>>> First of all userspace should not care when it calls kvm_set_ioapic()
>>> the kernel need to do the right thing. Second, believe it or not,
>>> kvm_ioapic_reset() is not called during system reset. Instead userspace
>>> reset it by calling kvm_set_ioapic() with ioapic state after reset.
>> Ok. I see. As the logic you suggested, it will clear dest_map if no
>> pending eoi in vcpu, so we don't need to do it again.
>> 
> You again rely on userspace doing thing in certain manner. What is
> set_lapic() is never called? Kernel internal state have to be correct
> after each ioctl call.
Sorry. I cannot figure out what's the problem if don't clear dest_map? Can you elaborate it?

Best regards,
Yang

Re: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Gleb Natapov]

On Sun, Apr 07, 2013 at 01:05:02PM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-04-07:
> > On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-04-07:
> >>> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2013-04-04:
> >>>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
> >>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>> 
> >>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>> ---
> >>>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h |    2
> >>>>>>  ++ virt/kvm/ioapic.c    |   43
> >>>>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h | 1
> >>>>>>  + 4 files changed, 55 insertions(+), 0 deletions(-)
> >>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >>>>>> index 96ab160..9c041fa 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));
> >>>>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
> >>> kvm_vcpu
> >>>>> *vcpu,
> >>>>>>  	apic->highest_isr_cache = -1;
> >>>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> >>>>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
> >>>>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
> >>>>>>  
> >>>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
> >>>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> >>>>>> index 967519c..004d2ad 100644
> >>>>>> --- a/arch/x86/kvm/lapic.h
> >>>>>> +++ b/arch/x86/kvm/lapic.h
> >>>>>> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct
> >>>>> kvm_vcpu *vcpu)
> >>>>>>  	return vcpu->arch.apic->pending_events;
> >>>>>>  }
> >>>>>> +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 8664812..0b12b17 100644
> >>>>>> --- a/virt/kvm/ioapic.c
> >>>>>> +++ b/virt/kvm/ioapic.c
> >>>>>> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct
> >>>>> kvm_ioapic *ioapic,
> >>>>>>  	return result;
> >>>>>>  }
> >>>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
> >>>>>> +{
> >>>>>> +	ioapic->rtc_status.pending_eoi = 0;
> >>>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> >>>>>> +{
> >>>>>> +	struct kvm_vcpu *vcpu;
> >>>>>> +	int vector, i, pending_eoi = 0;
> >>>>>> +
> >>>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
> >>>>>> +		return;
> >>>>>> +
> >>>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
> >>>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
> >>>>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
> >>>>>> +			pending_eoi++;
> >>>>>> +			__set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> >>>>> You should cleat dest_map at the beginning to get rid of stale bits.
> >>>> I thought kvm_set_ioapic is called only after save/restore or migration. And
> > the
> >>> ioapic should be reset successfully before call it. So the dest_map is empty
> >>> before call rtc_irq_restore().
> >>>> But it is possible kvm_set_ioapic is called beside save/restore or
> >>>> migration. Right?
> >>>> 
> >>> First of all userspace should not care when it calls kvm_set_ioapic()
> >>> the kernel need to do the right thing. Second, believe it or not,
> >>> kvm_ioapic_reset() is not called during system reset. Instead userspace
> >>> reset it by calling kvm_set_ioapic() with ioapic state after reset.
> >> Ok. I see. As the logic you suggested, it will clear dest_map if no
> >> pending eoi in vcpu, so we don't need to do it again.
> >> 
> > You again rely on userspace doing thing in certain manner. What is
> > set_lapic() is never called? Kernel internal state have to be correct
> > after each ioctl call.
> Sorry. I cannot figure out what's the problem if don't clear dest_map? Can you elaborate it?
> 
What is not obvious about it? If there is a bit in dest_map that should
be cleared after rtc_irq_restore() it will not.

--
			Gleb.

RE: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-04-07:
> On Sun, Apr 07, 2013 at 01:05:02PM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-04-07:
>>> On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-04-07:
>>>>> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-04-04:
>>>>>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>> 
>>>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>> ---
>>>>>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h | 2
>>>>>>>>  ++ virt/kvm/ioapic.c    |   43
>>>>>>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h |
>>>>>>>>  1 + 4 files changed, 55 insertions(+), 0 deletions(-)
>>>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>>>>>> index 96ab160..9c041fa 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));
>>>>>>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
>>>>> kvm_vcpu
>>>>>>> *vcpu,
>>>>>>>>  	apic->highest_isr_cache = -1;
>>>>>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>>>>>>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
>>>>>>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
>>>>>>>>  
>>>>>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
>>>>>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>>>>>>>> index 967519c..004d2ad 100644
>>>>>>>> --- a/arch/x86/kvm/lapic.h
>>>>>>>> +++ b/arch/x86/kvm/lapic.h
>>>>>>>> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct
>>>>>>> kvm_vcpu *vcpu)
>>>>>>>>  	return vcpu->arch.apic->pending_events;
>>>>>>>>  }
>>>>>>>> +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 8664812..0b12b17 100644
>>>>>>>> --- a/virt/kvm/ioapic.c
>>>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>>>> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct
>>>>>>> kvm_ioapic *ioapic,
>>>>>>>>  	return result;
>>>>>>>>  }
>>>>>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
>>>>>>>> +{
>>>>>>>> +	ioapic->rtc_status.pending_eoi = 0;
>>>>>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>>>>>>>> +{
>>>>>>>> +	struct kvm_vcpu *vcpu;
>>>>>>>> +	int vector, i, pending_eoi = 0;
>>>>>>>> +
>>>>>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
>>>>>>>> +		return;
>>>>>>>> +
>>>>>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
>>>>>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
>>>>>>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
>>>>>>>> +			pending_eoi++;
>>>>>>>> +			__set_bit(vcpu->vcpu_id,
> ioapic->rtc_status.dest_map);
>>>>>>> You should cleat dest_map at the beginning to get rid of stale bits.
>>>>>> I thought kvm_set_ioapic is called only after save/restore or migration.
> And
>>> the
>>>>> ioapic should be reset successfully before call it. So the dest_map is empty
>>>>> before call rtc_irq_restore().
>>>>>> But it is possible kvm_set_ioapic is called beside save/restore or
>>>>>> migration. Right?
>>>>>> 
>>>>> First of all userspace should not care when it calls kvm_set_ioapic()
>>>>> the kernel need to do the right thing. Second, believe it or not,
>>>>> kvm_ioapic_reset() is not called during system reset. Instead userspace
>>>>> reset it by calling kvm_set_ioapic() with ioapic state after reset.
>>>> Ok. I see. As the logic you suggested, it will clear dest_map if no
>>>> pending eoi in vcpu, so we don't need to do it again.
>>>> 
>>> You again rely on userspace doing thing in certain manner. What is
>>> set_lapic() is never called? Kernel internal state have to be correct
>>> after each ioctl call.
>> Sorry. I cannot figure out what's the problem if don't clear dest_map?
>> Can you elaborate it?
>> 
> What is not obvious about it? If there is a bit in dest_map that should
> be cleared after rtc_irq_restore() it will not.
Why it will not? If new_val is false, and the old_val is true. __clear_bit() will clear the dest_map. Am I wrong?

new_val = kvm_apic_pending_eoi(vcpu, vector);
old_val = test_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);

if (new_val == old_val)
        return;

if (new_val) {
        __set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
        ioapic->rtc_status.pending_eoi++;
} else {
        __clear_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
        ioapic->rtc_status.pending_eoi--;
}

Best regards,
Yang

Re: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Gleb Natapov]

On Sun, Apr 07, 2013 at 01:16:51PM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-04-07:
> > On Sun, Apr 07, 2013 at 01:05:02PM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-04-07:
> >>> On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2013-04-07:
> >>>>> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
> >>>>>> Gleb Natapov wrote on 2013-04-04:
> >>>>>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
> >>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>>>> 
> >>>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>>>> ---
> >>>>>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h | 2
> >>>>>>>>  ++ virt/kvm/ioapic.c    |   43
> >>>>>>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h |
> >>>>>>>>  1 + 4 files changed, 55 insertions(+), 0 deletions(-)
> >>>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >>>>>>>> index 96ab160..9c041fa 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));
> >>>>>>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
> >>>>> kvm_vcpu
> >>>>>>> *vcpu,
> >>>>>>>>  	apic->highest_isr_cache = -1;
> >>>>>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> >>>>>>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
> >>>>>>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
> >>>>>>>>  
> >>>>>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
> >>>>>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> >>>>>>>> index 967519c..004d2ad 100644
> >>>>>>>> --- a/arch/x86/kvm/lapic.h
> >>>>>>>> +++ b/arch/x86/kvm/lapic.h
> >>>>>>>> @@ -170,4 +170,6 @@ static inline bool kvm_apic_has_events(struct
> >>>>>>> kvm_vcpu *vcpu)
> >>>>>>>>  	return vcpu->arch.apic->pending_events;
> >>>>>>>>  }
> >>>>>>>> +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 8664812..0b12b17 100644
> >>>>>>>> --- a/virt/kvm/ioapic.c
> >>>>>>>> +++ b/virt/kvm/ioapic.c
> >>>>>>>> @@ -90,6 +90,47 @@ static unsigned long ioapic_read_indirect(struct
> >>>>>>> kvm_ioapic *ioapic,
> >>>>>>>>  	return result;
> >>>>>>>>  }
> >>>>>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
> >>>>>>>> +{
> >>>>>>>> +	ioapic->rtc_status.pending_eoi = 0;
> >>>>>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> >>>>>>>> +{
> >>>>>>>> +	struct kvm_vcpu *vcpu;
> >>>>>>>> +	int vector, i, pending_eoi = 0;
> >>>>>>>> +
> >>>>>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
> >>>>>>>> +		return;
> >>>>>>>> +
> >>>>>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
> >>>>>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
> >>>>>>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
> >>>>>>>> +			pending_eoi++;
> >>>>>>>> +			__set_bit(vcpu->vcpu_id,
> > ioapic->rtc_status.dest_map);
> >>>>>>> You should cleat dest_map at the beginning to get rid of stale bits.
> >>>>>> I thought kvm_set_ioapic is called only after save/restore or migration.
> > And
> >>> the
> >>>>> ioapic should be reset successfully before call it. So the dest_map is empty
> >>>>> before call rtc_irq_restore().
> >>>>>> But it is possible kvm_set_ioapic is called beside save/restore or
> >>>>>> migration. Right?
> >>>>>> 
> >>>>> First of all userspace should not care when it calls kvm_set_ioapic()
> >>>>> the kernel need to do the right thing. Second, believe it or not,
> >>>>> kvm_ioapic_reset() is not called during system reset. Instead userspace
> >>>>> reset it by calling kvm_set_ioapic() with ioapic state after reset.
> >>>> Ok. I see. As the logic you suggested, it will clear dest_map if no
> >>>> pending eoi in vcpu, so we don't need to do it again.
> >>>> 
> >>> You again rely on userspace doing thing in certain manner. What is
> >>> set_lapic() is never called? Kernel internal state have to be correct
> >>> after each ioctl call.
> >> Sorry. I cannot figure out what's the problem if don't clear dest_map?
> >> Can you elaborate it?
> >> 
> > What is not obvious about it? If there is a bit in dest_map that should
> > be cleared after rtc_irq_restore() it will not.
> Why it will not? If new_val is false, and the old_val is true. __clear_bit() will clear the dest_map. Am I wrong?
> 
Ah, yes with new logic since we go over all vcpus and calculate new
value for each one in theory it should be fine, but if we add cpu
destruction this will be no longer true.

> new_val = kvm_apic_pending_eoi(vcpu, vector);
Which reminds me there are more bugs in the current code. It is not
enough to call kvm_apic_pending_eoi() to check the new value. You need to
see if the entry is masked and vcpu is the destination of the interrupt too.
 
> old_val = test_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> 
> if (new_val == old_val)
>         return;
> 
> if (new_val) {
>         __set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
>         ioapic->rtc_status.pending_eoi++;
> } else {
>         __clear_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
>         ioapic->rtc_status.pending_eoi--;
> }
> 
> Best regards,
> Yang
> 

--
			Gleb.

RE: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-04-07:
> On Sun, Apr 07, 2013 at 01:16:51PM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-04-07:
>>> On Sun, Apr 07, 2013 at 01:05:02PM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-04-07:
>>>>> On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-04-07:
>>>>>>> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
>>>>>>>> Gleb Natapov wrote on 2013-04-04:
>>>>>>>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
>>>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>>>> 
>>>>>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>>>> ---
>>>>>>>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h | 2
>>>>>>>>>>  ++ virt/kvm/ioapic.c    |   43
>>>>>>>>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h
>>>>>>>>>>  | 1 + 4 files changed, 55 insertions(+), 0 deletions(-)
>>>>>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>>>>>>>> index 96ab160..9c041fa 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));
>>>>>>>>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
>>>>>>> kvm_vcpu
>>>>>>>>> *vcpu,
>>>>>>>>>>  	apic->highest_isr_cache = -1;
>>>>>>>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>>>>>>>>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
>>>>>>>>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
>>>>>>>>>>  
>>>>>>>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
>>>>>>>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>>>>>>>>>> index 967519c..004d2ad 100644
>>>>>>>>>> --- a/arch/x86/kvm/lapic.h
>>>>>>>>>> +++ b/arch/x86/kvm/lapic.h
>>>>>>>>>> @@ -170,4 +170,6 @@ static inline bool
> kvm_apic_has_events(struct
>>>>>>>>> kvm_vcpu *vcpu)
>>>>>>>>>>  	return vcpu->arch.apic->pending_events;
>>>>>>>>>>  }
>>>>>>>>>> +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 8664812..0b12b17 100644
>>>>>>>>>> --- a/virt/kvm/ioapic.c
>>>>>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>>>>>> @@ -90,6 +90,47 @@ static unsigned long
> ioapic_read_indirect(struct
>>>>>>>>> kvm_ioapic *ioapic,
>>>>>>>>>>  	return result;
>>>>>>>>>>  }
>>>>>>>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
>>>>>>>>>> +{
>>>>>>>>>> +	ioapic->rtc_status.pending_eoi = 0;
>>>>>>>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>>>>>>>>>> +{
>>>>>>>>>> +	struct kvm_vcpu *vcpu;
>>>>>>>>>> +	int vector, i, pending_eoi = 0;
>>>>>>>>>> +
>>>>>>>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
>>>>>>>>>> +		return;
>>>>>>>>>> +
>>>>>>>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
>>>>>>>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
>>>>>>>>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
>>>>>>>>>> +			pending_eoi++;
>>>>>>>>>> +			__set_bit(vcpu->vcpu_id,
>>> ioapic->rtc_status.dest_map);
>>>>>>>>> You should cleat dest_map at the beginning to get rid of stale bits.
>>>>>>>> I thought kvm_set_ioapic is called only after save/restore or migration.
>>> And
>>>>> the
>>>>>>> ioapic should be reset successfully before call it. So the
>>>>>>> dest_map is empty before call rtc_irq_restore().
>>>>>>>> But it is possible kvm_set_ioapic is called beside save/restore or
>>>>>>>> migration. Right?
>>>>>>>> 
>>>>>>> First of all userspace should not care when it calls kvm_set_ioapic()
>>>>>>> the kernel need to do the right thing. Second, believe it or not,
>>>>>>> kvm_ioapic_reset() is not called during system reset. Instead userspace
>>>>>>> reset it by calling kvm_set_ioapic() with ioapic state after reset.
>>>>>> Ok. I see. As the logic you suggested, it will clear dest_map if no
>>>>>> pending eoi in vcpu, so we don't need to do it again.
>>>>>> 
>>>>> You again rely on userspace doing thing in certain manner. What is
>>>>> set_lapic() is never called? Kernel internal state have to be correct
>>>>> after each ioctl call.
>>>> Sorry. I cannot figure out what's the problem if don't clear dest_map?
>>>> Can you elaborate it?
>>>> 
>>> What is not obvious about it? If there is a bit in dest_map that should
>>> be cleared after rtc_irq_restore() it will not.
>> Why it will not? If new_val is false, and the old_val is true.
>> __clear_bit() will clear the dest_map. Am I wrong?
>> 
> Ah, yes with new logic since we go over all vcpus and calculate new
> value for each one in theory it should be fine, but if we add cpu
> destruction this will be no longer true.
Yes. Given cpu destruction, it is wrong. Do you think we should clear dest_map now or delay it until cpu destruction is ready?

> 
>> new_val = kvm_apic_pending_eoi(vcpu, vector);
> Which reminds me there are more bugs in the current code. It is not
> enough to call kvm_apic_pending_eoi() to check the new value. You need to
> see if the entry is masked and vcpu is the destination of the interrupt too.
Yes, you are right.

> 
>> old_val = test_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
>> 
>> if (new_val == old_val)
>>         return;
>> if (new_val) {
>>         __set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
>>         ioapic->rtc_status.pending_eoi++; } else {
>>         __clear_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
>>         ioapic->rtc_status.pending_eoi--;
>> }
>> 
>> Best regards,
>> Yang
>> 
> 
> --
> 			Gleb.


Best regards,
Yang

Re: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Gleb Natapov]

On Sun, Apr 07, 2013 at 01:46:57PM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-04-07:
> > On Sun, Apr 07, 2013 at 01:16:51PM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-04-07:
> >>> On Sun, Apr 07, 2013 at 01:05:02PM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2013-04-07:
> >>>>> On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
> >>>>>> Gleb Natapov wrote on 2013-04-07:
> >>>>>>> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
> >>>>>>>> Gleb Natapov wrote on 2013-04-04:
> >>>>>>>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
> >>>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>>>>>> 
> >>>>>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h | 2
> >>>>>>>>>>  ++ virt/kvm/ioapic.c    |   43
> >>>>>>>>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h
> >>>>>>>>>>  | 1 + 4 files changed, 55 insertions(+), 0 deletions(-)
> >>>>>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >>>>>>>>>> index 96ab160..9c041fa 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));
> >>>>>>>>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
> >>>>>>> kvm_vcpu
> >>>>>>>>> *vcpu,
> >>>>>>>>>>  	apic->highest_isr_cache = -1;
> >>>>>>>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> >>>>>>>>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
> >>>>>>>>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
> >>>>>>>>>>  
> >>>>>>>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
> >>>>>>>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> >>>>>>>>>> index 967519c..004d2ad 100644
> >>>>>>>>>> --- a/arch/x86/kvm/lapic.h
> >>>>>>>>>> +++ b/arch/x86/kvm/lapic.h
> >>>>>>>>>> @@ -170,4 +170,6 @@ static inline bool
> > kvm_apic_has_events(struct
> >>>>>>>>> kvm_vcpu *vcpu)
> >>>>>>>>>>  	return vcpu->arch.apic->pending_events;
> >>>>>>>>>>  }
> >>>>>>>>>> +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 8664812..0b12b17 100644
> >>>>>>>>>> --- a/virt/kvm/ioapic.c
> >>>>>>>>>> +++ b/virt/kvm/ioapic.c
> >>>>>>>>>> @@ -90,6 +90,47 @@ static unsigned long
> > ioapic_read_indirect(struct
> >>>>>>>>> kvm_ioapic *ioapic,
> >>>>>>>>>>  	return result;
> >>>>>>>>>>  }
> >>>>>>>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
> >>>>>>>>>> +{
> >>>>>>>>>> +	ioapic->rtc_status.pending_eoi = 0;
> >>>>>>>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> >>>>>>>>>> +{
> >>>>>>>>>> +	struct kvm_vcpu *vcpu;
> >>>>>>>>>> +	int vector, i, pending_eoi = 0;
> >>>>>>>>>> +
> >>>>>>>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
> >>>>>>>>>> +		return;
> >>>>>>>>>> +
> >>>>>>>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
> >>>>>>>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
> >>>>>>>>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
> >>>>>>>>>> +			pending_eoi++;
> >>>>>>>>>> +			__set_bit(vcpu->vcpu_id,
> >>> ioapic->rtc_status.dest_map);
> >>>>>>>>> You should cleat dest_map at the beginning to get rid of stale bits.
> >>>>>>>> I thought kvm_set_ioapic is called only after save/restore or migration.
> >>> And
> >>>>> the
> >>>>>>> ioapic should be reset successfully before call it. So the
> >>>>>>> dest_map is empty before call rtc_irq_restore().
> >>>>>>>> But it is possible kvm_set_ioapic is called beside save/restore or
> >>>>>>>> migration. Right?
> >>>>>>>> 
> >>>>>>> First of all userspace should not care when it calls kvm_set_ioapic()
> >>>>>>> the kernel need to do the right thing. Second, believe it or not,
> >>>>>>> kvm_ioapic_reset() is not called during system reset. Instead userspace
> >>>>>>> reset it by calling kvm_set_ioapic() with ioapic state after reset.
> >>>>>> Ok. I see. As the logic you suggested, it will clear dest_map if no
> >>>>>> pending eoi in vcpu, so we don't need to do it again.
> >>>>>> 
> >>>>> You again rely on userspace doing thing in certain manner. What is
> >>>>> set_lapic() is never called? Kernel internal state have to be correct
> >>>>> after each ioctl call.
> >>>> Sorry. I cannot figure out what's the problem if don't clear dest_map?
> >>>> Can you elaborate it?
> >>>> 
> >>> What is not obvious about it? If there is a bit in dest_map that should
> >>> be cleared after rtc_irq_restore() it will not.
> >> Why it will not? If new_val is false, and the old_val is true.
> >> __clear_bit() will clear the dest_map. Am I wrong?
> >> 
> > Ah, yes with new logic since we go over all vcpus and calculate new
> > value for each one in theory it should be fine, but if we add cpu
> > destruction this will be no longer true.
> Yes. Given cpu destruction, it is wrong. Do you think we should clear dest_map now or delay it until cpu destruction is ready?
> 
We will surely forgot it at that point and this is slow path. Lets clear
it.

> > 
> >> new_val = kvm_apic_pending_eoi(vcpu, vector);
> > Which reminds me there are more bugs in the current code. It is not
> > enough to call kvm_apic_pending_eoi() to check the new value. You need to
> > see if the entry is masked and vcpu is the destination of the interrupt too.
> Yes, you are right.
> 
> > 
> >> old_val = test_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> >> 
> >> if (new_val == old_val)
> >>         return;
> >> if (new_val) {
> >>         __set_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> >>         ioapic->rtc_status.pending_eoi++; } else {
> >>         __clear_bit(vcpu->vcpu_id, ioapic->rtc_status.dest_map);
> >>         ioapic->rtc_status.pending_eoi--;
> >> }
> >> 
> >> Best regards,
> >> Yang
> >> 
> > 
> > --
> > 			Gleb.
> 
> 
> Best regards,
> Yang
> 

--
			Gleb.

RE: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-04-07:
> On Sun, Apr 07, 2013 at 01:16:51PM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-04-07:
>>> On Sun, Apr 07, 2013 at 01:05:02PM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-04-07:
>>>>> On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-04-07:
>>>>>>> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
>>>>>>>> Gleb Natapov wrote on 2013-04-04:
>>>>>>>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
>>>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>>>> 
>>>>>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>>>> ---
>>>>>>>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h | 2
>>>>>>>>>>  ++ virt/kvm/ioapic.c    |   43
>>>>>>>>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h
>>>>>>>>>>  | 1 + 4 files changed, 55 insertions(+), 0 deletions(-)
>>>>>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>>>>>>>> index 96ab160..9c041fa 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));
>>>>>>>>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
>>>>>>> kvm_vcpu
>>>>>>>>> *vcpu,
>>>>>>>>>>  	apic->highest_isr_cache = -1;
>>>>>>>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>>>>>>>>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
>>>>>>>>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
>>>>>>>>>>  
>>>>>>>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
>>>>>>>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>>>>>>>>>> index 967519c..004d2ad 100644
>>>>>>>>>> --- a/arch/x86/kvm/lapic.h
>>>>>>>>>> +++ b/arch/x86/kvm/lapic.h
>>>>>>>>>> @@ -170,4 +170,6 @@ static inline bool
> kvm_apic_has_events(struct
>>>>>>>>> kvm_vcpu *vcpu)
>>>>>>>>>>  	return vcpu->arch.apic->pending_events;
>>>>>>>>>>  }
>>>>>>>>>> +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 8664812..0b12b17 100644
>>>>>>>>>> --- a/virt/kvm/ioapic.c
>>>>>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>>>>>> @@ -90,6 +90,47 @@ static unsigned long
> ioapic_read_indirect(struct
>>>>>>>>> kvm_ioapic *ioapic,
>>>>>>>>>>  	return result;
>>>>>>>>>>  }
>>>>>>>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
>>>>>>>>>> +{
>>>>>>>>>> +	ioapic->rtc_status.pending_eoi = 0;
>>>>>>>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>>>>>>>>>> +{
>>>>>>>>>> +	struct kvm_vcpu *vcpu;
>>>>>>>>>> +	int vector, i, pending_eoi = 0;
>>>>>>>>>> +
>>>>>>>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
>>>>>>>>>> +		return;
>>>>>>>>>> +
>>>>>>>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
>>>>>>>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
>>>>>>>>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
>>>>>>>>>> +			pending_eoi++;
>>>>>>>>>> +			__set_bit(vcpu->vcpu_id,
>>> ioapic->rtc_status.dest_map);
>>>>>>>>> You should cleat dest_map at the beginning to get rid of stale bits.
>>>>>>>> I thought kvm_set_ioapic is called only after save/restore or migration.
>>> And
>>>>> the
>>>>>>> ioapic should be reset successfully before call it. So the
>>>>>>> dest_map is empty before call rtc_irq_restore().
>>>>>>>> But it is possible kvm_set_ioapic is called beside save/restore or
>>>>>>>> migration. Right?
>>>>>>>> 
>>>>>>> First of all userspace should not care when it calls kvm_set_ioapic()
>>>>>>> the kernel need to do the right thing. Second, believe it or not,
>>>>>>> kvm_ioapic_reset() is not called during system reset. Instead userspace
>>>>>>> reset it by calling kvm_set_ioapic() with ioapic state after reset.
>>>>>> Ok. I see. As the logic you suggested, it will clear dest_map if no
>>>>>> pending eoi in vcpu, so we don't need to do it again.
>>>>>> 
>>>>> You again rely on userspace doing thing in certain manner. What is
>>>>> set_lapic() is never called? Kernel internal state have to be correct
>>>>> after each ioctl call.
>>>> Sorry. I cannot figure out what's the problem if don't clear dest_map?
>>>> Can you elaborate it?
>>>> 
>>> What is not obvious about it? If there is a bit in dest_map that should
>>> be cleared after rtc_irq_restore() it will not.
>> Why it will not? If new_val is false, and the old_val is true.
>> __clear_bit() will clear the dest_map. Am I wrong?
>> 
> Ah, yes with new logic since we go over all vcpus and calculate new
> value for each one in theory it should be fine, but if we add cpu
> destruction this will be no longer true.
> 
>> new_val = kvm_apic_pending_eoi(vcpu, vector);
> Which reminds me there are more bugs in the current code. It is not
> enough to call kvm_apic_pending_eoi() to check the new value. You need to
> see if the entry is masked and vcpu is the destination of the interrupt too.
No. kvm_apic_pending_eoi() is the right way. IOAPIC entry may change after vcpu received it before issue EOI, and we should not rely on the entry.
For example:
vcpu A received the interrupt, pending in IRR
mask entry
migration happened

The only problem is we may account the interrupt from non-IOAPIC(from IPI) into RTC interrupt. But it's ok, we will clear pending_eoi in EOI regardless of source.

Best regards,
Yang

Re: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Gleb Natapov]

On Mon, Apr 08, 2013 at 11:21:34AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-04-07:
> > On Sun, Apr 07, 2013 at 01:16:51PM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-04-07:
> >>> On Sun, Apr 07, 2013 at 01:05:02PM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2013-04-07:
> >>>>> On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
> >>>>>> Gleb Natapov wrote on 2013-04-07:
> >>>>>>> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
> >>>>>>>> Gleb Natapov wrote on 2013-04-04:
> >>>>>>>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
> >>>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>>>>>> 
> >>>>>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h | 2
> >>>>>>>>>>  ++ virt/kvm/ioapic.c    |   43
> >>>>>>>>>>  +++++++++++++++++++++++++++++++++++++++++++ virt/kvm/ioapic.h
> >>>>>>>>>>  | 1 + 4 files changed, 55 insertions(+), 0 deletions(-)
> >>>>>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> >>>>>>>>>> index 96ab160..9c041fa 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));
> >>>>>>>>>> @@ -1665,6 +1673,7 @@ void kvm_apic_post_state_restore(struct
> >>>>>>> kvm_vcpu
> >>>>>>>>> *vcpu,
> >>>>>>>>>>  	apic->highest_isr_cache = -1;
> >>>>>>>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
> >>>>>>>>>>  apic_find_highest_isr(apic)); 	kvm_make_request(KVM_REQ_EVENT,
> >>>>>>>>>>  vcpu); +	kvm_rtc_irq_restore(vcpu); }
> >>>>>>>>>>  
> >>>>>>>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
> >>>>>>>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> >>>>>>>>>> index 967519c..004d2ad 100644
> >>>>>>>>>> --- a/arch/x86/kvm/lapic.h
> >>>>>>>>>> +++ b/arch/x86/kvm/lapic.h
> >>>>>>>>>> @@ -170,4 +170,6 @@ static inline bool
> > kvm_apic_has_events(struct
> >>>>>>>>> kvm_vcpu *vcpu)
> >>>>>>>>>>  	return vcpu->arch.apic->pending_events;
> >>>>>>>>>>  }
> >>>>>>>>>> +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 8664812..0b12b17 100644
> >>>>>>>>>> --- a/virt/kvm/ioapic.c
> >>>>>>>>>> +++ b/virt/kvm/ioapic.c
> >>>>>>>>>> @@ -90,6 +90,47 @@ static unsigned long
> > ioapic_read_indirect(struct
> >>>>>>>>> kvm_ioapic *ioapic,
> >>>>>>>>>>  	return result;
> >>>>>>>>>>  }
> >>>>>>>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic)
> >>>>>>>>>> +{
> >>>>>>>>>> +	ioapic->rtc_status.pending_eoi = 0;
> >>>>>>>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> >>>>>>>>>> +{
> >>>>>>>>>> +	struct kvm_vcpu *vcpu;
> >>>>>>>>>> +	int vector, i, pending_eoi = 0;
> >>>>>>>>>> +
> >>>>>>>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS)
> >>>>>>>>>> +		return;
> >>>>>>>>>> +
> >>>>>>>>>> +	vector = ioapic->redirtbl[RTC_GSI].fields.vector;
> >>>>>>>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) {
> >>>>>>>>>> +		if (kvm_apic_pending_eoi(vcpu, vector)) {
> >>>>>>>>>> +			pending_eoi++;
> >>>>>>>>>> +			__set_bit(vcpu->vcpu_id,
> >>> ioapic->rtc_status.dest_map);
> >>>>>>>>> You should cleat dest_map at the beginning to get rid of stale bits.
> >>>>>>>> I thought kvm_set_ioapic is called only after save/restore or migration.
> >>> And
> >>>>> the
> >>>>>>> ioapic should be reset successfully before call it. So the
> >>>>>>> dest_map is empty before call rtc_irq_restore().
> >>>>>>>> But it is possible kvm_set_ioapic is called beside save/restore or
> >>>>>>>> migration. Right?
> >>>>>>>> 
> >>>>>>> First of all userspace should not care when it calls kvm_set_ioapic()
> >>>>>>> the kernel need to do the right thing. Second, believe it or not,
> >>>>>>> kvm_ioapic_reset() is not called during system reset. Instead userspace
> >>>>>>> reset it by calling kvm_set_ioapic() with ioapic state after reset.
> >>>>>> Ok. I see. As the logic you suggested, it will clear dest_map if no
> >>>>>> pending eoi in vcpu, so we don't need to do it again.
> >>>>>> 
> >>>>> You again rely on userspace doing thing in certain manner. What is
> >>>>> set_lapic() is never called? Kernel internal state have to be correct
> >>>>> after each ioctl call.
> >>>> Sorry. I cannot figure out what's the problem if don't clear dest_map?
> >>>> Can you elaborate it?
> >>>> 
> >>> What is not obvious about it? If there is a bit in dest_map that should
> >>> be cleared after rtc_irq_restore() it will not.
> >> Why it will not? If new_val is false, and the old_val is true.
> >> __clear_bit() will clear the dest_map. Am I wrong?
> >> 
> > Ah, yes with new logic since we go over all vcpus and calculate new
> > value for each one in theory it should be fine, but if we add cpu
> > destruction this will be no longer true.
> > 
> >> new_val = kvm_apic_pending_eoi(vcpu, vector);
> > Which reminds me there are more bugs in the current code. It is not
> > enough to call kvm_apic_pending_eoi() to check the new value. You need to
> > see if the entry is masked and vcpu is the destination of the interrupt too.
> No. kvm_apic_pending_eoi() is the right way. IOAPIC entry may change after vcpu received it before issue EOI, and we should not rely on the entry.
> For example:
> vcpu A received the interrupt, pending in IRR
> mask entry
> migration happened
> 
> The only problem is we may account the interrupt from non-IOAPIC(from IPI) into RTC interrupt. But it's ok, we will clear pending_eoi in EOI regardless of source.
> 
With apicv this EOI may never come. We have to at least check that the vcpu
is a destination for the interrupt.

--
			Gleb.

RE: [PATCH v7 4/7] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-04-08:
> On Mon, Apr 08, 2013 at 11:21:34AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-04-07:
>>> On Sun, Apr 07, 2013 at 01:16:51PM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-04-07:
>>>>> On Sun, Apr 07, 2013 at 01:05:02PM +0000, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-04-07:
>>>>>>> On Sun, Apr 07, 2013 at 12:39:32PM +0000, Zhang, Yang Z wrote:
>>>>>>>> Gleb Natapov wrote on 2013-04-07:
>>>>>>>>> On Sun, Apr 07, 2013 at 02:30:15AM +0000, Zhang, Yang Z wrote:
>>>>>>>>>> Gleb Natapov wrote on 2013-04-04:
>>>>>>>>>>> On Mon, Apr 01, 2013 at 08:40:13AM +0800, Yang Zhang wrote:
>>>>>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>>>>>> 
>>>>>>>>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  arch/x86/kvm/lapic.c |    9 +++++++++ arch/x86/kvm/lapic.h |
>>>>>>>>>>>>  2 ++ virt/kvm/ioapic.c    |   43
>>>>>>>>>>>>  +++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>  virt/kvm/ioapic.h | 1 + 4 files changed, 55 insertions(+), 0
>>>>>>>>>>>>  deletions(-)
>>>>>>>>>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>>>>>>>>>> index 96ab160..9c041fa 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));
>>>>>>>>>>>> @@ -1665,6 +1673,7 @@ void
> kvm_apic_post_state_restore(struct
>>>>>>>>> kvm_vcpu
>>>>>>>>>>> *vcpu,
>>>>>>>>>>>>  	apic->highest_isr_cache = -1;
>>>>>>>>>>>>  	kvm_x86_ops->hwapic_isr_update(vcpu->kvm,
>>>>>>>>>>>>  apic_find_highest_isr(apic));
>>>>>>>>>>>>  	kvm_make_request(KVM_REQ_EVENT, vcpu);
>>>>>>>>>>>>  +	kvm_rtc_irq_restore(vcpu); }
>>>>>>>>>>>>  
>>>>>>>>>>>>  void __kvm_migrate_apic_timer(struct kvm_vcpu *vcpu)
>>>>>>>>>>>> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>>>>>>>>>>>> index 967519c..004d2ad 100644
>>>>>>>>>>>> --- a/arch/x86/kvm/lapic.h
>>>>>>>>>>>> +++ b/arch/x86/kvm/lapic.h
>>>>>>>>>>>> @@ -170,4 +170,6 @@ static inline bool
>>> kvm_apic_has_events(struct
>>>>>>>>>>> kvm_vcpu *vcpu)
>>>>>>>>>>>>  	return vcpu->arch.apic->pending_events;
>>>>>>>>>>>>  }
>>>>>>>>>>>> +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 8664812..0b12b17 100644
>>>>>>>>>>>> --- a/virt/kvm/ioapic.c
>>>>>>>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>>>>>>>> @@ -90,6 +90,47 @@ static unsigned long
>>> ioapic_read_indirect(struct
>>>>>>>>>>> kvm_ioapic *ioapic,
>>>>>>>>>>>>  	return result;
>>>>>>>>>>>>  }
>>>>>>>>>>>> +static void rtc_irq_reset(struct kvm_ioapic *ioapic) +{
>>>>>>>>>>>> +	ioapic->rtc_status.pending_eoi = 0;
>>>>>>>>>>>> +	bitmap_zero(ioapic->rtc_status.dest_map, KVM_MAX_VCPUS); +}
>>>>>>>>>>>> + +static void rtc_irq_restore(struct kvm_ioapic *ioapic) +{
>>>>>>>>>>>> +	struct kvm_vcpu *vcpu; +	int vector, i, pending_eoi = 0; +
>>>>>>>>>>>> +	if (RTC_GSI >= IOAPIC_NUM_PINS) +		return; + +	vector =
>>>>>>>>>>>> ioapic->redirtbl[RTC_GSI].fields.vector;
>>>>>>>>>>>> +	kvm_for_each_vcpu(i, vcpu, ioapic->kvm) { +		if
>>>>>>>>>>>> (kvm_apic_pending_eoi(vcpu, vector)) { +			pending_eoi++;
>>>>>>>>>>>> +			__set_bit(vcpu->vcpu_id,
>>>>> ioapic->rtc_status.dest_map);
>>>>>>>>>>> You should cleat dest_map at the beginning to get rid of stale bits.
>>>>>>>>>> I thought kvm_set_ioapic is called only after save/restore or
> migration.
>>>>> And
>>>>>>> the
>>>>>>>>> ioapic should be reset successfully before call it. So the
>>>>>>>>> dest_map is empty before call rtc_irq_restore().
>>>>>>>>>> But it is possible kvm_set_ioapic is called beside save/restore or
>>>>>>>>>> migration. Right?
>>>>>>>>>> 
>>>>>>>>> First of all userspace should not care when it calls
>>>>>>>>> kvm_set_ioapic() the kernel need to do the right thing. Second,
>>>>>>>>> believe it or not, kvm_ioapic_reset() is not called during
>>>>>>>>> system reset. Instead userspace reset it by calling
>>>>>>>>> kvm_set_ioapic() with ioapic state after reset.
>>>>>>>> Ok. I see. As the logic you suggested, it will clear dest_map if no
>>>>>>>> pending eoi in vcpu, so we don't need to do it again.
>>>>>>>> 
>>>>>>> You again rely on userspace doing thing in certain manner. What is
>>>>>>> set_lapic() is never called? Kernel internal state have to be correct
>>>>>>> after each ioctl call.
>>>>>> Sorry. I cannot figure out what's the problem if don't clear dest_map?
>>>>>> Can you elaborate it?
>>>>>> 
>>>>> What is not obvious about it? If there is a bit in dest_map that should
>>>>> be cleared after rtc_irq_restore() it will not.
>>>> Why it will not? If new_val is false, and the old_val is true.
>>>> __clear_bit() will clear the dest_map. Am I wrong?
>>>> 
>>> Ah, yes with new logic since we go over all vcpus and calculate new
>>> value for each one in theory it should be fine, but if we add cpu
>>> destruction this will be no longer true.
>>> 
>>>> new_val = kvm_apic_pending_eoi(vcpu, vector);
>>> Which reminds me there are more bugs in the current code. It is not
>>> enough to call kvm_apic_pending_eoi() to check the new value. You need to
>>> see if the entry is masked and vcpu is the destination of the interrupt too.
>> No. kvm_apic_pending_eoi() is the right way. IOAPIC entry may change
>> after vcpu received it before issue EOI, and we should not rely on the
>> entry. For example: vcpu A received the interrupt, pending in IRR mask
>> entry migration happened
>> 
>> The only problem is we may account the interrupt from non-IOAPIC(from IPI)
> into RTC interrupt. But it's ok, we will clear pending_eoi in EOI regardless of
> source.
>> 
> With apicv this EOI may never come. We have to at least check that the vcpu
> is a destination for the interrupt.
You are right. This is necessary/

Best regards,
Yang