[PATCH v6 0/6] Use eoi to track RTC interrupt delivery status

[PATCH v6 0/6] 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 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.

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

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

Yang Zhang (6):
  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: Use eoi to track RTC interrupt delivery status

 arch/x86/kvm/lapic.c |   35 +++++++++++++++------
 arch/x86/kvm/lapic.h |    7 +++-
 virt/kvm/ioapic.c    |   82 +++++++++++++++++++++++++++++++++++++++++++++-----
 virt/kvm/ioapic.h    |   17 +++++++++-
 virt/kvm/irq_comm.c  |   12 ++++----
 5 files changed, 125 insertions(+), 28 deletions(-)

[PATCH v6 1/6] 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 v6 2/6] KVM: Introduce struct rtc_status

[Yang Zhang]

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

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

diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 2fc61a5..cd30277 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 255
+#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 v6 3/6] 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..d7915a1 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 cd30277..7c01998 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

Re: [PATCH v6 3/6] KVM : Return destination vcpu on interrupt injection

[Gleb Natapov]

On Fri, Mar 22, 2013 at 01:24:02PM +0800, Yang Zhang wrote:
> 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..d7915a1 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);
> +
__set_bit()

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

--
			Gleb.

RE: [PATCH v6 3/6] KVM : Return destination vcpu on interrupt injection

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-22:
> On Fri, Mar 22, 2013 at 01:24:02PM +0800, Yang Zhang wrote:
>> 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..d7915a1 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);
>> +
> __set_bit()
no, __apic_accept_irq() may be called to deliver interrupt from IOAPIC and LAPIC interrupt.
Though the dest_map is only used by RTC interrupt now, it may be use by LAPIC interrupt in future. So it's better to use set_bit not __set_bit.

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


Best regards,
Yang

Re: [PATCH v6 3/6] KVM : Return destination vcpu on interrupt injection

[Gleb Natapov]

On Fri, Mar 22, 2013 at 07:50:11AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-22:
> > On Fri, Mar 22, 2013 at 01:24:02PM +0800, Yang Zhang wrote:
> >> 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..d7915a1 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);
> >> +
> > __set_bit()
> no, __apic_accept_irq() may be called to deliver interrupt from IOAPIC and LAPIC interrupt.
> Though the dest_map is only used by RTC interrupt now, it may be use by LAPIC interrupt in future. So it's better to use set_bit not __set_bit.
It is a caller responsibility to not provide pointer to the same memory
to concurrent function invocations.

--
			Gleb.

[PATCH v6 4/6] 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 |    8 ++++++++
 arch/x86/kvm/lapic.h |    2 ++
 virt/kvm/ioapic.c    |   26 ++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index d7915a1..7c17e82 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -94,6 +94,14 @@ static inline int apic_test_vector(int vec, void *bitmap)
 	return test_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
 }
 
+bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector)
+{
+	struct kvm_lapic *apic = vcpu->arch.apic;
+
+	return apic_test_vector(vector, apic->regs + APIC_ISR) ||
+		apic_test_vector(vector, apic->regs + APIC_IRR);
+}
+
 static inline void apic_set_vector(int vec, void *bitmap)
 {
 	set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 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..3897305 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -90,6 +90,30 @@ 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 != 8)
+		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;
+}
+
 static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
 {
 	union kvm_ioapic_redirect_entry *pent;
@@ -431,6 +455,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 +521,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;
-- 
1.7.1

Re: [PATCH v6 4/6] KVM: Add reset/restore rtc_status support

[Paolo Bonzini]

Il 22/03/2013 06:24, Yang Zhang ha scritto:
> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> +{
> +	struct kvm_vcpu *vcpu;
> +	int vector, i, pending_eoi = 0;
> +
> +	if (RTC_GSI != 8)

Please set it to -1U if not x86, and do

   if (RTC_GSI >= IOAPIC_NUM_PINS)
       return;

here.

Paolo

> +		return;
> +

RE: [PATCH v6 4/6] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Paolo Bonzini wrote on 2013-03-26:
> Il 22/03/2013 06:24, Yang Zhang ha scritto:
>> +static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>> +{
>> +	struct kvm_vcpu *vcpu;
>> +	int vector, i, pending_eoi = 0;
>> +
>> +	if (RTC_GSI != 8)
> 
> Please set it to -1U if not x86, and do
> 
>    if (RTC_GSI >= IOAPIC_NUM_PINS)
>        return;
> here.
Sure. It is more reasonable.

Best regards,
Yang

Re: [PATCH v6 4/6] KVM: Add reset/restore rtc_status support

[Paolo Bonzini]

Il 22/03/2013 06:24, Yang Zhang ha scritto:
> +	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);

Also, __set_bit.  If I understand correctly, dest_map is protected by
the ioapic spinlock.

Paolo

> +		}
> +	}
> +	ioapic->rtc_status.pending_eoi = pending_eoi;

RE: [PATCH v6 4/6] KVM: Add reset/restore rtc_status support

[Zhang, Yang Z]

Paolo Bonzini wrote on 2013-03-26:
> Il 22/03/2013 06:24, Yang Zhang ha scritto:
>> +	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);
> 
> Also, __set_bit.  If I understand correctly, dest_map is protected by
> the ioapic spinlock.
Yes, I already see it and changed it in version 7.

Best regards,
Yang

[PATCH v6 5/6] 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 3897305..c991e58 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -158,7 +158,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 v6 6/6] 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 c991e58..df16daf 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic *ioapic)
 	ioapic->rtc_status.pending_eoi = pending_eoi;
 }
 
+static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
+			struct rtc_status *rtc_status, int irq)
+{
+	if (irq != RTC_GSI)
+		return;
+
+	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
+		--rtc_status->pending_eoi;
+
+	WARN_ON(rtc_status->pending_eoi < 0);
+}
+
+static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
+{
+	if (irq != RTC_GSI)
+		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)
 {
 	union kvm_ioapic_redirect_entry *pent;
@@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 {
 	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",
@@ -244,7 +268,14 @@ 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, NULL);
+	if (irq == RTC_GSI) {
+		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,
@@ -268,6 +299,11 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
 		ret = 1;
 	} else {
 		int edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
+
+		if (rtc_irq_check(ioapic, irq)) {
+			ret = 0; /* coalesced */
+			goto out;
+		}
 		ioapic->irr |= mask;
 		if ((edge && old_irr != ioapic->irr) ||
 		    (!edge && !entry.fields.remote_irr))
@@ -275,6 +311,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);
 
@@ -302,6 +339,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
 		if (ent->fields.vector != vector)
 			continue;
 
+		rtc_irq_ack_eoi(vcpu, &ioapic->rtc_status, i);
 		/*
 		 * We are dropping lock while calling ack notifiers because ack
 		 * notifier callbacks for assigned devices call into IOAPIC
-- 
1.7.1

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Gleb Natapov]

On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang@Intel.com>
> 
> Current interrupt coalescing logci which only used by RTC has conflict
> with Posted Interrupt.
> This patch introduces a new mechinism to use eoi to track interrupt:
> When delivering an interrupt to vcpu, the 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 c991e58..df16daf 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>  	ioapic->rtc_status.pending_eoi = pending_eoi;
>  }
>  
> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> +			struct rtc_status *rtc_status, int irq)
> +{
> +	if (irq != RTC_GSI)
> +		return;
> +
> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
> +		--rtc_status->pending_eoi;
> +
> +	WARN_ON(rtc_status->pending_eoi < 0);
> +}
> +
> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
> +{
> +	if (irq != RTC_GSI)
> +		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)
>  {
>  	union kvm_ioapic_redirect_entry *pent;
> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
>  {
>  	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",
> @@ -244,7 +268,14 @@ 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, NULL);
> +	if (irq == RTC_GSI) {
> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> +				ioapic->rtc_status.dest_map);
> +		ioapic->rtc_status.pending_eoi = ret;
We should track status only if IRQ_STATUS ioctl was used to inject an
interrupt.

> +	} 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,
> @@ -268,6 +299,11 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int irq_source_id,
>  		ret = 1;
>  	} else {
>  		int edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
> +
> +		if (rtc_irq_check(ioapic, irq)) {
> +			ret = 0; /* coalesced */
> +			goto out;
> +		}
>  		ioapic->irr |= mask;
>  		if ((edge && old_irr != ioapic->irr) ||
>  		    (!edge && !entry.fields.remote_irr))
> @@ -275,6 +311,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);
>  
> @@ -302,6 +339,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu *vcpu,
>  		if (ent->fields.vector != vector)
>  			continue;
>  
> +		rtc_irq_ack_eoi(vcpu, &ioapic->rtc_status, i);
>  		/*
>  		 * We are dropping lock while calling ack notifiers because ack
>  		 * notifier callbacks for assigned devices call into IOAPIC
> -- 
> 1.7.1

--
			Gleb.

RE: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-22:
> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
>> From: Yang Zhang <yang.z.zhang@Intel.com>
>> 
>> Current interrupt coalescing logci which only used by RTC has conflict
>> with Posted Interrupt.
>> This patch introduces a new mechinism to use eoi to track interrupt:
>> When delivering an interrupt to vcpu, the 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 c991e58..df16daf 100644
>> --- a/virt/kvm/ioapic.c
>> +++ b/virt/kvm/ioapic.c
>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic *ioapic)
>>  	ioapic->rtc_status.pending_eoi = pending_eoi;
>>  }
>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
>> +			struct rtc_status *rtc_status, int irq)
>> +{
>> +	if (irq != RTC_GSI)
>> +		return;
>> +
>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
>> +		--rtc_status->pending_eoi;
>> +
>> +	WARN_ON(rtc_status->pending_eoi < 0);
>> +}
>> +
>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
>> +{
>> +	if (irq != RTC_GSI)
>> +		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)
>>  {
>>  	union kvm_ioapic_redirect_entry *pent;
>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int
> irq)
>>  {
>>  	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",
>> @@ -244,7 +268,14 @@ 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, NULL);
>> +	if (irq == RTC_GSI) {
>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
>> +				ioapic->rtc_status.dest_map);
>> +		ioapic->rtc_status.pending_eoi = ret;
> We should track status only if IRQ_STATUS ioctl was used to inject an
> interrupt.
We already know RTC will use IRQ_STATUS ioctl. Why check it again?

>> +	} 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,
>> @@ -268,6 +299,11 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int
> irq, int irq_source_id,
>>  		ret = 1;
>>  	} else {
>>  		int edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
>> +
>> +		if (rtc_irq_check(ioapic, irq)) {
>> +			ret = 0; /* coalesced */
>> +			goto out;
>> +		}
>>  		ioapic->irr |= mask;
>>  		if ((edge && old_irr != ioapic->irr) ||
>>  		    (!edge && !entry.fields.remote_irr))
>> @@ -275,6 +311,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);
>> @@ -302,6 +339,7 @@ static void __kvm_ioapic_update_eoi(struct kvm_vcpu
> *vcpu,
>>  		if (ent->fields.vector != vector)
>>  			continue;
>> +		rtc_irq_ack_eoi(vcpu, &ioapic->rtc_status, i);
>>  		/*
>>  		 * We are dropping lock while calling ack notifiers because ack
>>  		 * notifier callbacks for assigned devices call into IOAPIC
>> --
>> 1.7.1
> 
> --
> 			Gleb.
> --
> 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 v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Gleb Natapov]

On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-22:
> > On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
> >> From: Yang Zhang <yang.z.zhang@Intel.com>
> >> 
> >> Current interrupt coalescing logci which only used by RTC has conflict
> >> with Posted Interrupt.
> >> This patch introduces a new mechinism to use eoi to track interrupt:
> >> When delivering an interrupt to vcpu, the 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 c991e58..df16daf 100644
> >> --- a/virt/kvm/ioapic.c
> >> +++ b/virt/kvm/ioapic.c
> >> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic *ioapic)
> >>  	ioapic->rtc_status.pending_eoi = pending_eoi;
> >>  }
> >> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> >> +			struct rtc_status *rtc_status, int irq)
> >> +{
> >> +	if (irq != RTC_GSI)
> >> +		return;
> >> +
> >> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
> >> +		--rtc_status->pending_eoi;
> >> +
> >> +	WARN_ON(rtc_status->pending_eoi < 0);
> >> +}
> >> +
> >> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
> >> +{
> >> +	if (irq != RTC_GSI)
> >> +		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)
> >>  {
> >>  	union kvm_ioapic_redirect_entry *pent;
> >> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int
> > irq)
> >>  {
> >>  	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",
> >> @@ -244,7 +268,14 @@ 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, NULL);
> >> +	if (irq == RTC_GSI) {
> >> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> >> +				ioapic->rtc_status.dest_map);
> >> +		ioapic->rtc_status.pending_eoi = ret;
> > We should track status only if IRQ_STATUS ioctl was used to inject an
> > interrupt.
> We already know RTC will use IRQ_STATUS ioctl. Why check it again?
> 
QEMU does. QEMU is not the only userspace.

--
			Gleb.

RE: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-22:
> On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-03-22:
>>> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>> 
>>>> Current interrupt coalescing logci which only used by RTC has conflict
>>>> with Posted Interrupt.
>>>> This patch introduces a new mechinism to use eoi to track interrupt:
>>>> When delivering an interrupt to vcpu, the 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 c991e58..df16daf 100644
>>>> --- a/virt/kvm/ioapic.c
>>>> +++ b/virt/kvm/ioapic.c
>>>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic
> *ioapic)
>>>>  	ioapic->rtc_status.pending_eoi = pending_eoi;
>>>>  }
>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
>>>> +			struct rtc_status *rtc_status, int irq)
>>>> +{
>>>> +	if (irq != RTC_GSI)
>>>> +		return;
>>>> +
>>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
>>>> +		--rtc_status->pending_eoi;
>>>> +
>>>> +	WARN_ON(rtc_status->pending_eoi < 0);
>>>> +}
>>>> +
>>>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
>>>> +{
>>>> +	if (irq != RTC_GSI)
>>>> +		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)
>>>>  {
>>>>  	union kvm_ioapic_redirect_entry *pent;
>>>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int
>>> irq)
>>>>  {
>>>>  	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",
>>>> @@ -244,7 +268,14 @@ 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, NULL);
>>>> +	if (irq == RTC_GSI) {
>>>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
>>>> +				ioapic->rtc_status.dest_map);
>>>> +		ioapic->rtc_status.pending_eoi = ret;
>>> We should track status only if IRQ_STATUS ioctl was used to inject an
>>> interrupt.
>> We already know RTC will use IRQ_STATUS ioctl. Why check it again?
>> 
> QEMU does. QEMU is not the only userspace.
And this will break other userspace.

Best regards,
Yang

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Gleb Natapov]

On Fri, Mar 22, 2013 at 08:25:21AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-22:
> > On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-03-22:
> >>> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
> >>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>> 
> >>>> Current interrupt coalescing logci which only used by RTC has conflict
> >>>> with Posted Interrupt.
> >>>> This patch introduces a new mechinism to use eoi to track interrupt:
> >>>> When delivering an interrupt to vcpu, the 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 c991e58..df16daf 100644
> >>>> --- a/virt/kvm/ioapic.c
> >>>> +++ b/virt/kvm/ioapic.c
> >>>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic
> > *ioapic)
> >>>>  	ioapic->rtc_status.pending_eoi = pending_eoi;
> >>>>  }
> >>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> >>>> +			struct rtc_status *rtc_status, int irq)
> >>>> +{
> >>>> +	if (irq != RTC_GSI)
> >>>> +		return;
> >>>> +
> >>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
> >>>> +		--rtc_status->pending_eoi;
> >>>> +
> >>>> +	WARN_ON(rtc_status->pending_eoi < 0);
> >>>> +}
> >>>> +
> >>>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
> >>>> +{
> >>>> +	if (irq != RTC_GSI)
> >>>> +		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)
> >>>>  {
> >>>>  	union kvm_ioapic_redirect_entry *pent;
> >>>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int
> >>> irq)
> >>>>  {
> >>>>  	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",
> >>>> @@ -244,7 +268,14 @@ 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, NULL);
> >>>> +	if (irq == RTC_GSI) {
> >>>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> >>>> +				ioapic->rtc_status.dest_map);
> >>>> +		ioapic->rtc_status.pending_eoi = ret;
> >>> We should track status only if IRQ_STATUS ioctl was used to inject an
> >>> interrupt.
> >> We already know RTC will use IRQ_STATUS ioctl. Why check it again?
> >> 
> > QEMU does. QEMU is not the only userspace.
> And this will break other userspace.
> 
How?

--
			Gleb.

RE: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-22:
> On Fri, Mar 22, 2013 at 08:25:21AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-03-22:
>>> On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-03-22:
>>>>> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>> 
>>>>>> Current interrupt coalescing logci which only used by RTC has conflict
>>>>>> with Posted Interrupt.
>>>>>> This patch introduces a new mechinism to use eoi to track interrupt:
>>>>>> When delivering an interrupt to vcpu, the 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 c991e58..df16daf 100644
>>>>>> --- a/virt/kvm/ioapic.c
>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic
>>> *ioapic)
>>>>>>  	ioapic->rtc_status.pending_eoi = pending_eoi;
>>>>>>  }
>>>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
>>>>>> +			struct rtc_status *rtc_status, int irq)
>>>>>> +{
>>>>>> +	if (irq != RTC_GSI)
>>>>>> +		return;
>>>>>> +
>>>>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
>>>>>> +		--rtc_status->pending_eoi;
>>>>>> +
>>>>>> +	WARN_ON(rtc_status->pending_eoi < 0);
>>>>>> +}
>>>>>> +
>>>>>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
>>>>>> +{
>>>>>> +	if (irq != RTC_GSI)
>>>>>> +		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)
>>>>>>  {
>>>>>>  	union kvm_ioapic_redirect_entry *pent;
>>>>>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic,
> int
>>>>> irq)
>>>>>>  {
>>>>>>  	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",
>>>>>> @@ -244,7 +268,14 @@ 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, NULL);
>>>>>> +	if (irq == RTC_GSI) {
>>>>>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
>>>>>> +				ioapic->rtc_status.dest_map);
>>>>>> +		ioapic->rtc_status.pending_eoi = ret;
>>>>> We should track status only if IRQ_STATUS ioctl was used to inject an
>>>>> interrupt.
>>>> We already know RTC will use IRQ_STATUS ioctl. Why check it again?
>>>> 
>>> QEMU does. QEMU is not the only userspace.
>> And this will break other userspace.
>> 
> How?
If other userspace has the reinjection logic for RTC, but it not uses IRQ_STATUS, then it cannot get the right coalescing info. If it also use IRQ_STATUS to get coalescing info, then we don't need the IRQ_STATUS check.

Best regards,
Yang

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Gleb Natapov]

On Fri, Mar 22, 2013 at 08:37:22AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-22:
> > On Fri, Mar 22, 2013 at 08:25:21AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-03-22:
> >>> On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2013-03-22:
> >>>>> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
> >>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>> 
> >>>>>> Current interrupt coalescing logci which only used by RTC has conflict
> >>>>>> with Posted Interrupt.
> >>>>>> This patch introduces a new mechinism to use eoi to track interrupt:
> >>>>>> When delivering an interrupt to vcpu, the 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 c991e58..df16daf 100644
> >>>>>> --- a/virt/kvm/ioapic.c
> >>>>>> +++ b/virt/kvm/ioapic.c
> >>>>>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic
> >>> *ioapic)
> >>>>>>  	ioapic->rtc_status.pending_eoi = pending_eoi;
> >>>>>>  }
> >>>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> >>>>>> +			struct rtc_status *rtc_status, int irq)
> >>>>>> +{
> >>>>>> +	if (irq != RTC_GSI)
> >>>>>> +		return;
> >>>>>> +
> >>>>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
> >>>>>> +		--rtc_status->pending_eoi;
> >>>>>> +
> >>>>>> +	WARN_ON(rtc_status->pending_eoi < 0);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
> >>>>>> +{
> >>>>>> +	if (irq != RTC_GSI)
> >>>>>> +		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)
> >>>>>>  {
> >>>>>>  	union kvm_ioapic_redirect_entry *pent;
> >>>>>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic,
> > int
> >>>>> irq)
> >>>>>>  {
> >>>>>>  	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",
> >>>>>> @@ -244,7 +268,14 @@ 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, NULL);
> >>>>>> +	if (irq == RTC_GSI) {
> >>>>>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> >>>>>> +				ioapic->rtc_status.dest_map);
> >>>>>> +		ioapic->rtc_status.pending_eoi = ret;
> >>>>> We should track status only if IRQ_STATUS ioctl was used to inject an
> >>>>> interrupt.
> >>>> We already know RTC will use IRQ_STATUS ioctl. Why check it again?
> >>>> 
> >>> QEMU does. QEMU is not the only userspace.
> >> And this will break other userspace.
> >> 
> > How?
> If other userspace has the reinjection logic for RTC, but it not uses IRQ_STATUS, then it cannot get the right coalescing info. If it also use IRQ_STATUS to get coalescing info, then we don't need the IRQ_STATUS check.
> 
If userspace does not care about irq status it does not use IRQ_STATUS
ioctl and we should not go extra mile to provide one. Not everyone cares
about running Windows as a guest.

--
			Gleb.

RE: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-22:
> On Fri, Mar 22, 2013 at 08:37:22AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-03-22:
>>> On Fri, Mar 22, 2013 at 08:25:21AM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-03-22:
>>>>> On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-03-22:
>>>>>>> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>> 
>>>>>>>> Current interrupt coalescing logci which only used by RTC has
>>>>>>>> conflict with Posted Interrupt. This patch introduces a new
>>>>>>>> mechinism to use eoi to track interrupt: When delivering an
>>>>>>>> interrupt to vcpu, the 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 c991e58..df16daf 100644
>>>>>>>> --- a/virt/kvm/ioapic.c
>>>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>>>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic
>>>>> *ioapic)
>>>>>>>>  	ioapic->rtc_status.pending_eoi = pending_eoi;
>>>>>>>>  }
>>>>>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
>>>>>>>> +			struct rtc_status *rtc_status, int irq)
>>>>>>>> +{
>>>>>>>> +	if (irq != RTC_GSI)
>>>>>>>> +		return;
>>>>>>>> +
>>>>>>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
>>>>>>>> +		--rtc_status->pending_eoi;
>>>>>>>> +
>>>>>>>> +	WARN_ON(rtc_status->pending_eoi < 0);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
>>>>>>>> +{
>>>>>>>> +	if (irq != RTC_GSI)
>>>>>>>> +		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)
>>>>>>>>  {
>>>>>>>>  	union kvm_ioapic_redirect_entry *pent;
>>>>>>>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic
> *ioapic,
>>> int
>>>>>>> irq)
>>>>>>>>  {
>>>>>>>>  	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",
>>>>>>>> @@ -244,7 +268,14 @@ 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,
>>>>>>>> NULL); +	if (irq == RTC_GSI) { +		ret =
>>>>>>>> kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
>>>>>>>> +				ioapic->rtc_status.dest_map);
>>>>>>>> +		ioapic->rtc_status.pending_eoi = ret;
>>>>>>> We should track status only if IRQ_STATUS ioctl was used to inject an
>>>>>>> interrupt.
>>>>>> We already know RTC will use IRQ_STATUS ioctl. Why check it again?
>>>>>> 
>>>>> QEMU does. QEMU is not the only userspace.
>>>> And this will break other userspace.
>>>> 
>>> How?
>> If other userspace has the reinjection logic for RTC, but it not uses IRQ_STATUS,
> then it cannot get the right coalescing info. If it also use IRQ_STATUS to get
> coalescing info, then we don't need the IRQ_STATUS check.
>> 
> If userspace does not care about irq status it does not use IRQ_STATUS
> ioctl and we should not go extra mile to provide one. Not everyone cares
> about running Windows as a guest.
I see your point. But if no windows guest running, RTC is hardly used by other guests and the overheard can be ignore.

Best regards,
Yang

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Gleb Natapov]

On Fri, Mar 22, 2013 at 08:51:47AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-22:
> > On Fri, Mar 22, 2013 at 08:37:22AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-03-22:
> >>> On Fri, Mar 22, 2013 at 08:25:21AM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2013-03-22:
> >>>>> On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote:
> >>>>>> Gleb Natapov wrote on 2013-03-22:
> >>>>>>> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
> >>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>>>> 
> >>>>>>>> Current interrupt coalescing logci which only used by RTC has
> >>>>>>>> conflict with Posted Interrupt. This patch introduces a new
> >>>>>>>> mechinism to use eoi to track interrupt: When delivering an
> >>>>>>>> interrupt to vcpu, the 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 c991e58..df16daf 100644
> >>>>>>>> --- a/virt/kvm/ioapic.c
> >>>>>>>> +++ b/virt/kvm/ioapic.c
> >>>>>>>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic
> >>>>> *ioapic)
> >>>>>>>>  	ioapic->rtc_status.pending_eoi = pending_eoi;
> >>>>>>>>  }
> >>>>>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> >>>>>>>> +			struct rtc_status *rtc_status, int irq)
> >>>>>>>> +{
> >>>>>>>> +	if (irq != RTC_GSI)
> >>>>>>>> +		return;
> >>>>>>>> +
> >>>>>>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
> >>>>>>>> +		--rtc_status->pending_eoi;
> >>>>>>>> +
> >>>>>>>> +	WARN_ON(rtc_status->pending_eoi < 0);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
> >>>>>>>> +{
> >>>>>>>> +	if (irq != RTC_GSI)
> >>>>>>>> +		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)
> >>>>>>>>  {
> >>>>>>>>  	union kvm_ioapic_redirect_entry *pent;
> >>>>>>>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic
> > *ioapic,
> >>> int
> >>>>>>> irq)
> >>>>>>>>  {
> >>>>>>>>  	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",
> >>>>>>>> @@ -244,7 +268,14 @@ 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,
> >>>>>>>> NULL); +	if (irq == RTC_GSI) { +		ret =
> >>>>>>>> kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> >>>>>>>> +				ioapic->rtc_status.dest_map);
> >>>>>>>> +		ioapic->rtc_status.pending_eoi = ret;
> >>>>>>> We should track status only if IRQ_STATUS ioctl was used to inject an
> >>>>>>> interrupt.
> >>>>>> We already know RTC will use IRQ_STATUS ioctl. Why check it again?
> >>>>>> 
> >>>>> QEMU does. QEMU is not the only userspace.
> >>>> And this will break other userspace.
> >>>> 
> >>> How?
> >> If other userspace has the reinjection logic for RTC, but it not uses IRQ_STATUS,
> > then it cannot get the right coalescing info. If it also use IRQ_STATUS to get
> > coalescing info, then we don't need the IRQ_STATUS check.
> >> 
> > If userspace does not care about irq status it does not use IRQ_STATUS
> > ioctl and we should not go extra mile to provide one. Not everyone cares
> > about running Windows as a guest.
> I see your point. But if no windows guest running, RTC is hardly used by other guests and the overheard can be ignore.
> 
Anyone can use RTC is Linux guest. Don't know about others.

--
			Gleb.

RE: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-03-22:
> On Fri, Mar 22, 2013 at 08:51:47AM +0000, Zhang, Yang Z wrote:
>> Gleb Natapov wrote on 2013-03-22:
>>> On Fri, Mar 22, 2013 at 08:37:22AM +0000, Zhang, Yang Z wrote:
>>>> Gleb Natapov wrote on 2013-03-22:
>>>>> On Fri, Mar 22, 2013 at 08:25:21AM +0000, Zhang, Yang Z wrote:
>>>>>> Gleb Natapov wrote on 2013-03-22:
>>>>>>> On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote:
>>>>>>>> Gleb Natapov wrote on 2013-03-22:
>>>>>>>>> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
>>>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
>>>>>>>>>> 
>>>>>>>>>> Current interrupt coalescing logci which only used by RTC has
>>>>>>>>>> conflict with Posted Interrupt. This patch introduces a new
>>>>>>>>>> mechinism to use eoi to track interrupt: When delivering an
>>>>>>>>>> interrupt to vcpu, the 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 c991e58..df16daf 100644
>>>>>>>>>> --- a/virt/kvm/ioapic.c
>>>>>>>>>> +++ b/virt/kvm/ioapic.c
>>>>>>>>>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic
>>>>>>> *ioapic)
>>>>>>>>>>  	ioapic->rtc_status.pending_eoi = pending_eoi;
>>>>>>>>>>  }
>>>>>>>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
>>>>>>>>>> +			struct rtc_status *rtc_status, int irq)
>>>>>>>>>> +{
>>>>>>>>>> +	if (irq != RTC_GSI)
>>>>>>>>>> +		return;
>>>>>>>>>> +
>>>>>>>>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
>>>>>>>>>> +		--rtc_status->pending_eoi;
>>>>>>>>>> +
>>>>>>>>>> +	WARN_ON(rtc_status->pending_eoi < 0);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
>>>>>>>>>> +{
>>>>>>>>>> +	if (irq != RTC_GSI)
>>>>>>>>>> +		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)
>>>>>>>>>>  {
>>>>>>>>>>  	union kvm_ioapic_redirect_entry *pent;
>>>>>>>>>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic
>>> *ioapic,
>>>>> int
>>>>>>>>> irq)
>>>>>>>>>>  {
>>>>>>>>>>  	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",
>>>>>>>>>> @@ -244,7 +268,14 @@ 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,
>>>>>>>>>> NULL); +	if (irq == RTC_GSI) { +		ret =
>>>>>>>>>> kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
>>>>>>>>>> +				ioapic->rtc_status.dest_map);
>>>>>>>>>> +		ioapic->rtc_status.pending_eoi = ret;
>>>>>>>>> We should track status only if IRQ_STATUS ioctl was used to inject an
>>>>>>>>> interrupt.
>>>>>>>> We already know RTC will use IRQ_STATUS ioctl. Why check it again?
>>>>>>>> 
>>>>>>> QEMU does. QEMU is not the only userspace.
>>>>>> And this will break other userspace.
>>>>>> 
>>>>> How?
>>>> If other userspace has the reinjection logic for RTC, but it not uses
> IRQ_STATUS,
>>> then it cannot get the right coalescing info. If it also use IRQ_STATUS to get
>>> coalescing info, then we don't need the IRQ_STATUS check.
>>>> 
>>> If userspace does not care about irq status it does not use IRQ_STATUS
>>> ioctl and we should not go extra mile to provide one. Not everyone cares
>>> about running Windows as a guest.
>> I see your point. But if no windows guest running, RTC is hardly used
>> by other guests and the overheard can be ignore.
>> 
> Anyone can use RTC is Linux guest. Don't know about others.
I see.
Since pass IRQ_STATUS to ioapic need to change many functions, how about add a variable in rtc_status:
struct rtc_status {
      bool IRQ_STATUS
};

And set it in kvm_vm_ioctl():
case KVM_IRQ_LINE_STATUS:
	if(irq == RTC_GSI && ioapic)
		ioapic->rtc_status.IRQ_STATUS = true;

Best regards,
Yang

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Gleb Natapov]

On Fri, Mar 22, 2013 at 10:50:55AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-22:
> > On Fri, Mar 22, 2013 at 08:51:47AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-03-22:
> >>> On Fri, Mar 22, 2013 at 08:37:22AM +0000, Zhang, Yang Z wrote:
> >>>> Gleb Natapov wrote on 2013-03-22:
> >>>>> On Fri, Mar 22, 2013 at 08:25:21AM +0000, Zhang, Yang Z wrote:
> >>>>>> Gleb Natapov wrote on 2013-03-22:
> >>>>>>> On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote:
> >>>>>>>> Gleb Natapov wrote on 2013-03-22:
> >>>>>>>>> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote:
> >>>>>>>>>> From: Yang Zhang <yang.z.zhang@Intel.com>
> >>>>>>>>>> 
> >>>>>>>>>> Current interrupt coalescing logci which only used by RTC has
> >>>>>>>>>> conflict with Posted Interrupt. This patch introduces a new
> >>>>>>>>>> mechinism to use eoi to track interrupt: When delivering an
> >>>>>>>>>> interrupt to vcpu, the 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 c991e58..df16daf 100644
> >>>>>>>>>> --- a/virt/kvm/ioapic.c
> >>>>>>>>>> +++ b/virt/kvm/ioapic.c
> >>>>>>>>>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic
> >>>>>>> *ioapic)
> >>>>>>>>>>  	ioapic->rtc_status.pending_eoi = pending_eoi;
> >>>>>>>>>>  }
> >>>>>>>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> >>>>>>>>>> +			struct rtc_status *rtc_status, int irq)
> >>>>>>>>>> +{
> >>>>>>>>>> +	if (irq != RTC_GSI)
> >>>>>>>>>> +		return;
> >>>>>>>>>> +
> >>>>>>>>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
> >>>>>>>>>> +		--rtc_status->pending_eoi;
> >>>>>>>>>> +
> >>>>>>>>>> +	WARN_ON(rtc_status->pending_eoi < 0);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq)
> >>>>>>>>>> +{
> >>>>>>>>>> +	if (irq != RTC_GSI)
> >>>>>>>>>> +		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)
> >>>>>>>>>>  {
> >>>>>>>>>>  	union kvm_ioapic_redirect_entry *pent;
> >>>>>>>>>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic
> >>> *ioapic,
> >>>>> int
> >>>>>>>>> irq)
> >>>>>>>>>>  {
> >>>>>>>>>>  	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",
> >>>>>>>>>> @@ -244,7 +268,14 @@ 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,
> >>>>>>>>>> NULL); +	if (irq == RTC_GSI) { +		ret =
> >>>>>>>>>> kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> >>>>>>>>>> +				ioapic->rtc_status.dest_map);
> >>>>>>>>>> +		ioapic->rtc_status.pending_eoi = ret;
> >>>>>>>>> We should track status only if IRQ_STATUS ioctl was used to inject an
> >>>>>>>>> interrupt.
> >>>>>>>> We already know RTC will use IRQ_STATUS ioctl. Why check it again?
> >>>>>>>> 
> >>>>>>> QEMU does. QEMU is not the only userspace.
> >>>>>> And this will break other userspace.
> >>>>>> 
> >>>>> How?
> >>>> If other userspace has the reinjection logic for RTC, but it not uses
> > IRQ_STATUS,
> >>> then it cannot get the right coalescing info. If it also use IRQ_STATUS to get
> >>> coalescing info, then we don't need the IRQ_STATUS check.
> >>>> 
> >>> If userspace does not care about irq status it does not use IRQ_STATUS
> >>> ioctl and we should not go extra mile to provide one. Not everyone cares
> >>> about running Windows as a guest.
> >> I see your point. But if no windows guest running, RTC is hardly used
> >> by other guests and the overheard can be ignore.
> >> 
> > Anyone can use RTC is Linux guest. Don't know about others.
> I see.
> Since pass IRQ_STATUS to ioapic need to change many functions, how about add a variable in rtc_status:
> struct rtc_status {
>       bool IRQ_STATUS
> };
> 
> And set it in kvm_vm_ioctl():
> case KVM_IRQ_LINE_STATUS:
> 	if(irq == RTC_GSI && ioapic)
> 		ioapic->rtc_status.IRQ_STATUS = true;
> 
I do not like that kind of hacks. Just put functions change in separate,
easy to review patch.

--
			Gleb.

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Paolo Bonzini]

Il 22/03/2013 09:54, Gleb Natapov ha scritto:
>>>> > >> 
>>> > > If userspace does not care about irq status it does not use IRQ_STATUS
>>> > > ioctl and we should not go extra mile to provide one. Not everyone cares
>>> > > about running Windows as a guest.
>> > I see your point. But if no windows guest running, RTC is hardly used by other guests and the overheard can be ignore.
>> > 
> Anyone can use RTC is Linux guest. Don't know about others.

I do not think the additional code complexity is worth the speedup.

The KVM Userspace That Won't Be Merged doesn't even emulate the RTC
interrupt at all.

Paolo

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Paolo Bonzini]

Il 22/03/2013 06:24, Yang Zhang ha scritto:
> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> +			struct rtc_status *rtc_status, int irq)
> +{
> +	if (irq != RTC_GSI)
> +		return;
> +
> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
> +		--rtc_status->pending_eoi;
> +
> +	WARN_ON(rtc_status->pending_eoi < 0);
> +}

This is the only case where you're passing the struct rtc_status instead
of the struct kvm_ioapic.  Please use the latter, and make it the first
argument.

> @@ -244,7 +268,14 @@ 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, NULL);
> +	if (irq == RTC_GSI) {
> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> +				ioapic->rtc_status.dest_map);
> +		ioapic->rtc_status.pending_eoi = ret;

I think you should either add a

    BUG_ON(ioapic->rtc_status.pending_eoi != 0);

or use "ioapic->rtc_status.pending_eoi += ret" (or both).

> +	} else
> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL);
> +
> +	return ret;
>  }


Paolo

RE: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Paolo Bonzini wrote on 2013-03-26:
> Il 22/03/2013 06:24, Yang Zhang ha scritto:
>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
>> +			struct rtc_status *rtc_status, int irq)
>> +{
>> +	if (irq != RTC_GSI)
>> +		return;
>> +
>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
>> +		--rtc_status->pending_eoi;
>> +
>> +	WARN_ON(rtc_status->pending_eoi < 0);
>> +}
> 
> This is the only case where you're passing the struct rtc_status instead
> of the struct kvm_ioapic.  Please use the latter, and make it the first
> argument.
>
>> @@ -244,7 +268,14 @@ 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, NULL);
>> +	if (irq == RTC_GSI) {
>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
>> +				ioapic->rtc_status.dest_map);
>> +		ioapic->rtc_status.pending_eoi = ret;
> 
> I think you should either add a
> 
>     BUG_ON(ioapic->rtc_status.pending_eoi != 0);
> or use "ioapic->rtc_status.pending_eoi += ret" (or both).
> 
There may malicious guest to write EOI more than once. And the pending_eoi will be negative. But it should not be a bug. Just WARN_ON is enough. And we already do it in ack_eoi. So don't need to do duplicated thing here.

Best regards,
Yang

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Paolo Bonzini]

Il 29/03/2013 04:25, Zhang, Yang Z ha scritto:
> Paolo Bonzini wrote on 2013-03-26:
>> Il 22/03/2013 06:24, Yang Zhang ha scritto:
>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
>>> +			struct rtc_status *rtc_status, int irq)
>>> +{
>>> +	if (irq != RTC_GSI)
>>> +		return;
>>> +
>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
>>> +		--rtc_status->pending_eoi;
>>> +
>>> +	WARN_ON(rtc_status->pending_eoi < 0);
>>> +}
>>
>> This is the only case where you're passing the struct rtc_status instead
>> of the struct kvm_ioapic.  Please use the latter, and make it the first
>> argument.
>>
>>> @@ -244,7 +268,14 @@ 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, NULL);
>>> +	if (irq == RTC_GSI) {
>>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
>>> +				ioapic->rtc_status.dest_map);
>>> +		ioapic->rtc_status.pending_eoi = ret;
>>
>> I think you should either add a
>>
>>     BUG_ON(ioapic->rtc_status.pending_eoi != 0);
>> or use "ioapic->rtc_status.pending_eoi += ret" (or both).
>>
> There may malicious guest to write EOI more than once. And the
> pending_eoi will be negative. But it should not be a bug. Just WARN_ON
> is enough. And we already do it in ack_eoi. So don't need to do
> duplicated thing here.

Even WARN_ON is too much if it is guest-triggerable.  But then it is
better to make it "+=", I think.

Paolo

RE: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Paolo Bonzini wrote on 2013-03-29:
> Il 29/03/2013 04:25, Zhang, Yang Z ha scritto:
>> Paolo Bonzini wrote on 2013-03-26:
>>> Il 22/03/2013 06:24, Yang Zhang ha scritto:
>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
>>>> +			struct rtc_status *rtc_status, int irq)
>>>> +{
>>>> +	if (irq != RTC_GSI)
>>>> +		return;
>>>> +
>>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
>>>> +		--rtc_status->pending_eoi;
>>>> +
>>>> +	WARN_ON(rtc_status->pending_eoi < 0);
>>>> +}
>>> 
>>> This is the only case where you're passing the struct rtc_status instead
>>> of the struct kvm_ioapic.  Please use the latter, and make it the first
>>> argument.
>>> 
>>>> @@ -244,7 +268,14 @@ 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, NULL);
>>>> +	if (irq == RTC_GSI) {
>>>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
>>>> +				ioapic->rtc_status.dest_map);
>>>> +		ioapic->rtc_status.pending_eoi = ret;
>>> 
>>> I think you should either add a
>>> 
>>>     BUG_ON(ioapic->rtc_status.pending_eoi != 0);
>>> or use "ioapic->rtc_status.pending_eoi += ret" (or both).
>>> 
>> There may malicious guest to write EOI more than once. And the
>> pending_eoi will be negative. But it should not be a bug. Just WARN_ON
>> is enough. And we already do it in ack_eoi. So don't need to do
>> duplicated thing here.
> 
> Even WARN_ON is too much if it is guest-triggerable.  But then it is
> better to make it "+=", I think.
No. If the above case happened, you will always hit the WARN_ON with "+=". 

Best regards,
Yang

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Gleb Natapov]

On Fri, Mar 29, 2013 at 03:25:16AM +0000, Zhang, Yang Z wrote:
> Paolo Bonzini wrote on 2013-03-26:
> > Il 22/03/2013 06:24, Yang Zhang ha scritto:
> >> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> >> +			struct rtc_status *rtc_status, int irq)
> >> +{
> >> +	if (irq != RTC_GSI)
> >> +		return;
> >> +
> >> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
> >> +		--rtc_status->pending_eoi;
> >> +
> >> +	WARN_ON(rtc_status->pending_eoi < 0);
> >> +}
> > 
> > This is the only case where you're passing the struct rtc_status instead
> > of the struct kvm_ioapic.  Please use the latter, and make it the first
> > argument.
> >
> >> @@ -244,7 +268,14 @@ 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, NULL);
> >> +	if (irq == RTC_GSI) {
> >> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> >> +				ioapic->rtc_status.dest_map);
> >> +		ioapic->rtc_status.pending_eoi = ret;
> > 
> > I think you should either add a
> > 
> >     BUG_ON(ioapic->rtc_status.pending_eoi != 0);
> > or use "ioapic->rtc_status.pending_eoi += ret" (or both).
> > 
> There may malicious guest to write EOI more than once. And the pending_eoi will be negative. But it should not be a bug. Just WARN_ON is enough. And we already do it in ack_eoi. So don't need to do duplicated thing here.
> 
Since we track vcpus that already called EOI and decrement pending_eoi
only once for each vcpu malicious guest cannot trigger it, but we
already do WARN_ON() in rtc_irq_ack_eoi(), so I am not sure we need
another one here. += will be correct (since pending_eoi == 0 here), but
confusing since it makes an impression that pending_eoi may not be zero.

--
			Gleb.

RE: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Zhang, Yang Z]

Gleb Natapov wrote on 2013-04-02:
> On Fri, Mar 29, 2013 at 03:25:16AM +0000, Zhang, Yang Z wrote:
>> Paolo Bonzini wrote on 2013-03-26:
>>> Il 22/03/2013 06:24, Yang Zhang ha scritto:
>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
>>>> +			struct rtc_status *rtc_status, int irq)
>>>> +{
>>>> +	if (irq != RTC_GSI)
>>>> +		return;
>>>> +
>>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
>>>> +		--rtc_status->pending_eoi;
>>>> +
>>>> +	WARN_ON(rtc_status->pending_eoi < 0);
>>>> +}
>>> 
>>> This is the only case where you're passing the struct rtc_status instead
>>> of the struct kvm_ioapic.  Please use the latter, and make it the first
>>> argument.
>>> 
>>>> @@ -244,7 +268,14 @@ 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, NULL);
>>>> +	if (irq == RTC_GSI) {
>>>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
>>>> +				ioapic->rtc_status.dest_map);
>>>> +		ioapic->rtc_status.pending_eoi = ret;
>>> 
>>> I think you should either add a
>>> 
>>>     BUG_ON(ioapic->rtc_status.pending_eoi != 0);
>>> or use "ioapic->rtc_status.pending_eoi += ret" (or both).
>>> 
>> There may malicious guest to write EOI more than once. And the pending_eoi
> will be negative. But it should not be a bug. Just WARN_ON is enough. And we
> already do it in ack_eoi. So don't need to do duplicated thing here.
>> 
> Since we track vcpus that already called EOI and decrement pending_eoi
> only once for each vcpu malicious guest cannot trigger it, but we
> already do WARN_ON() in rtc_irq_ack_eoi(), so I am not sure we need
> another one here. += will be correct (since pending_eoi == 0 here), but
> confusing since it makes an impression that pending_eoi may not be zero.
Yes, I also make the wrong impression.
With previous implementation, the pening_eoi may not be zero: Calculate the destination vcpu via parse IOAPIC entry, and if using lowest priority deliver mode, set all possible vcpus in dest_map even it doesn't receive it finally. At same time, a malicious guest can send IPI with same vector of RTC to those vcpus who is in dest_map but not have RTC interrupt. Then the pending_eoi will be negative.
Now, we set the dest_map with the vcpus who really received the interrupt. The above case cannot happen. So as you and Paolo suggested, it is better to use +=.

Best regards,
Yang

Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status

[Gleb Natapov]

On Wed, Apr 03, 2013 at 12:21:05AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-04-02:
> > On Fri, Mar 29, 2013 at 03:25:16AM +0000, Zhang, Yang Z wrote:
> >> Paolo Bonzini wrote on 2013-03-26:
> >>> Il 22/03/2013 06:24, Yang Zhang ha scritto:
> >>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu,
> >>>> +			struct rtc_status *rtc_status, int irq)
> >>>> +{
> >>>> +	if (irq != RTC_GSI)
> >>>> +		return;
> >>>> +
> >>>> +	if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map))
> >>>> +		--rtc_status->pending_eoi;
> >>>> +
> >>>> +	WARN_ON(rtc_status->pending_eoi < 0);
> >>>> +}
> >>> 
> >>> This is the only case where you're passing the struct rtc_status instead
> >>> of the struct kvm_ioapic.  Please use the latter, and make it the first
> >>> argument.
> >>> 
> >>>> @@ -244,7 +268,14 @@ 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, NULL);
> >>>> +	if (irq == RTC_GSI) {
> >>>> +		ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe,
> >>>> +				ioapic->rtc_status.dest_map);
> >>>> +		ioapic->rtc_status.pending_eoi = ret;
> >>> 
> >>> I think you should either add a
> >>> 
> >>>     BUG_ON(ioapic->rtc_status.pending_eoi != 0);
> >>> or use "ioapic->rtc_status.pending_eoi += ret" (or both).
> >>> 
> >> There may malicious guest to write EOI more than once. And the pending_eoi
> > will be negative. But it should not be a bug. Just WARN_ON is enough. And we
> > already do it in ack_eoi. So don't need to do duplicated thing here.
> >> 
> > Since we track vcpus that already called EOI and decrement pending_eoi
> > only once for each vcpu malicious guest cannot trigger it, but we
> > already do WARN_ON() in rtc_irq_ack_eoi(), so I am not sure we need
> > another one here. += will be correct (since pending_eoi == 0 here), but
> > confusing since it makes an impression that pending_eoi may not be zero.
> Yes, I also make the wrong impression.
> With previous implementation, the pening_eoi may not be zero: Calculate the destination vcpu via parse IOAPIC entry, and if using lowest priority deliver mode, set all possible vcpus in dest_map even it doesn't receive it finally. At same time, a malicious guest can send IPI with same vector of RTC to those vcpus who is in dest_map but not have RTC interrupt. Then the pending_eoi will be negative.
> Now, we set the dest_map with the vcpus who really received the interrupt. The above case cannot happen. So as you and Paolo suggested, it is better to use +=.
> 
I am not suggesting that it is better to use +=. We can add
BUG_ON(ioapic->rtc_status.pending_eoi != 0); but no need to resend
patches just for that.

--
			Gleb.