[PATCH v2] report IRQ injection status to userspace.

[PATCH v2] report IRQ injection status to userspace.

[Gleb Natapov]

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index dbf527a..21c8445 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -925,6 +925,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		}
 		break;
+	case KVM_IRQ_LINE_STATUS:
 	case KVM_IRQ_LINE: {
 		struct kvm_irq_level irq_event;
 
@@ -932,10 +933,17 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (copy_from_user(&irq_event, argp, sizeof irq_event))
 			goto out;
 		if (irqchip_in_kernel(kvm)) {
+			__s32 status;
 			mutex_lock(&kvm->lock);
-			kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+			status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
 				    irq_event.irq, irq_event.level);
 			mutex_unlock(&kvm->lock);
+			if (ioctl == KVM_IRQ_LINE_STATUS) {
+				irq_event.status = status;
+				if (copy_to_user(argp, &irq_event,
+							sizeof irq_event))
+					goto out;
+			}
 			r = 0;
 		}
 		break;
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 55fd4c5..f0faf58 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -616,7 +616,7 @@ void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code);
 void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long cr2,
 			   u32 error_code);
 
-void kvm_pic_set_irq(void *opaque, int irq, int level);
+int kvm_pic_set_irq(void *opaque, int irq, int level);
 
 void kvm_inject_nmi(struct kvm_vcpu *vcpu);
 
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 179dcb0..2752016 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -76,12 +76,13 @@ void kvm_pic_clear_isr_ack(struct kvm *kvm)
 /*
  * set irq level. If an edge is detected, then the IRR is set to 1
  */
-static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
+static inline int pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
 {
-	int mask;
+	int mask, ret = 1;
 	mask = 1 << irq;
 	if (s->elcr & mask)	/* level triggered */
 		if (level) {
+			ret = !(s->irr & mask);
 			s->irr |= mask;
 			s->last_irr |= mask;
 		} else {
@@ -90,11 +91,15 @@ static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
 		}
 	else	/* edge triggered */
 		if (level) {
-			if ((s->last_irr & mask) == 0)
+			if ((s->last_irr & mask) == 0) {
+				ret = !(s->irr & mask);
 				s->irr |= mask;
+			}
 			s->last_irr |= mask;
 		} else
 			s->last_irr &= ~mask;
+
+	return (s->imr & mask) ? -1 : ret;
 }
 
 /*
@@ -171,16 +176,19 @@ void kvm_pic_update_irq(struct kvm_pic *s)
 	pic_unlock(s);
 }
 
-void kvm_pic_set_irq(void *opaque, int irq, int level)
+int kvm_pic_set_irq(void *opaque, int irq, int level)
 {
 	struct kvm_pic *s = opaque;
+	int ret = -1;
 
 	pic_lock(s);
 	if (irq >= 0 && irq < PIC_NUM_PINS) {
-		pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
+		ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
 		pic_update_irq(s);
 	}
 	pic_unlock(s);
+
+	return ret;
 }
 
 /*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e96edda..0ff98aa 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1852,6 +1852,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	create_pit_unlock:
 		mutex_unlock(&kvm->lock);
 		break;
+	case KVM_IRQ_LINE_STATUS:
 	case KVM_IRQ_LINE: {
 		struct kvm_irq_level irq_event;
 
@@ -1859,10 +1860,17 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (copy_from_user(&irq_event, argp, sizeof irq_event))
 			goto out;
 		if (irqchip_in_kernel(kvm)) {
+			__s32 status;
 			mutex_lock(&kvm->lock);
-			kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
-				    irq_event.irq, irq_event.level);
+			status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+					irq_event.irq, irq_event.level);
 			mutex_unlock(&kvm->lock);
+			if (ioctl == KVM_IRQ_LINE_STATUS) {
+				irq_event.status = status;
+				if (copy_to_user(argp, &irq_event,
+							sizeof irq_event))
+					goto out;
+			}
 			r = 0;
 		}
 		break;
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 869462c..cde0fde 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -48,7 +48,10 @@ struct kvm_irq_level {
 	 * For IA-64 (APIC model) IOAPIC0: irq 0-23; IOAPIC1: irq 24-47..
 	 * For X86 (standard AT mode) PIC0/1: irq 0-15. IOAPIC0: 0-23..
 	 */
-	__u32 irq;
+	union {
+		__u32 irq;
+		__s32 status;
+	};
 	__u32 level;
 };
 
@@ -456,6 +459,7 @@ struct kvm_irq_routing {
 #define KVM_CREATE_PIT		  _IO(KVMIO,  0x64)
 #define KVM_GET_PIT		  _IOWR(KVMIO, 0x65, struct kvm_pit_state)
 #define KVM_SET_PIT		  _IOR(KVMIO,  0x66, struct kvm_pit_state)
+#define KVM_IRQ_LINE_STATUS	  _IOWR(KVMIO, 0x67, struct kvm_irq_level)
 #define KVM_REGISTER_COALESCED_MMIO \
 			_IOW(KVMIO,  0x67, struct kvm_coalesced_mmio_zone)
 #define KVM_UNREGISTER_COALESCED_MMIO \
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ce285e0..a8f91bb 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -109,7 +109,7 @@ struct kvm_memory_slot {
 
 struct kvm_kernel_irq_routing_entry {
 	u32 gsi;
-	void (*set)(struct kvm_kernel_irq_routing_entry *e,
+	int (*set)(struct kvm_kernel_irq_routing_entry *e,
 		    struct kvm *kvm, int level);
 	union {
 		struct {
@@ -351,7 +351,7 @@ void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
 				      struct kvm_irq_mask_notifier *kimn);
 void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask);
 
-void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
+int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index e85a2bc..8920a60 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -83,19 +83,22 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
 	return result;
 }
 
-static void ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
+static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
 {
 	union ioapic_redir_entry *pent;
+	int injected = -1;
 
 	pent = &ioapic->redirtbl[idx];
 
 	if (!pent->fields.mask) {
-		int injected = ioapic_deliver(ioapic, idx);
+		injected = ioapic_deliver(ioapic, idx);
 		if (injected && pent->fields.trig_mode == IOAPIC_LEVEL_TRIG)
 			pent->fields.remote_irr = 1;
 	}
 	if (!pent->fields.trig_mode)
 		ioapic->irr &= ~(1 << idx);
+
+	return injected;
 }
 
 static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
@@ -273,11 +276,12 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 	return r;
 }
 
-void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
+int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 {
 	u32 old_irr = ioapic->irr;
 	u32 mask = 1 << irq;
 	union ioapic_redir_entry entry;
+	int ret = 1;
 
 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
 		entry = ioapic->redirtbl[irq];
@@ -288,9 +292,10 @@ void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 			ioapic->irr |= mask;
 			if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
 			    || !entry.fields.remote_irr)
-				ioapic_service(ioapic, irq);
+				ret = ioapic_service(ioapic, irq);
 		}
 	}
+	return ret;
 }
 
 static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int gsi,
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 49c9581..a34bd5e 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -83,7 +83,7 @@ struct kvm_vcpu *kvm_get_lowest_prio_vcpu(struct kvm *kvm, u8 vector,
 				       unsigned long bitmap);
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
 int kvm_ioapic_init(struct kvm *kvm);
-void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
+int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
 void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
 u32 kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
 				u8 dest_mode);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index a797fa5..00bb29f 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -24,25 +24,28 @@
 
 #include "ioapic.h"
 
-static void kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
+static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
 			    struct kvm *kvm, int level)
 {
 #ifdef CONFIG_X86
-	kvm_pic_set_irq(pic_irqchip(kvm), e->irqchip.pin, level);
+	return kvm_pic_set_irq(pic_irqchip(kvm), e->irqchip.pin, level);
+#else
+	return -1;
 #endif
 }
 
-static void kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
+static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
 			       struct kvm *kvm, int level)
 {
-	kvm_ioapic_set_irq(kvm->arch.vioapic, e->irqchip.pin, level);
+	return kvm_ioapic_set_irq(kvm->arch.vioapic, e->irqchip.pin, level);
 }
 
 /* This should be called with the kvm->lock mutex held */
-void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
+int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
 {
 	struct kvm_kernel_irq_routing_entry *e;
 	unsigned long *irq_state = (unsigned long *)&kvm->arch.irq_states[irq];
+	int ret = -1;
 
 	/* Logical OR for level trig interrupt */
 	if (level)
@@ -55,8 +58,11 @@ void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
 	 * writes to the unused one.
 	 */
 	list_for_each_entry(e, &kvm->irq_routing, link)
-		if (e->gsi == irq)
-			e->set(e, kvm, !!(*irq_state));
+		if (e->gsi == irq) {
+			int r = e->set(e, kvm, !!(*irq_state));
+			ret = max(ret, r);
+		}
+	return ret;
 }
 
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi)
@@ -165,7 +171,7 @@ int setup_routing_entry(struct kvm_kernel_irq_routing_entry *e,
 			e->set = kvm_set_pic_irq;
 			break;
 		case KVM_IRQCHIP_PIC_SLAVE:
-				e->set = kvm_set_pic_irq;
+			e->set = kvm_set_pic_irq;
 			delta = 8;
 			break;
 		case KVM_IRQCHIP_IOAPIC:
--
			Gleb.

Re: [PATCH v2] report IRQ injection status to userspace.

[Gleb Natapov]

Use this one instead. Adds capabilities checks.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index dbf527a..d83d3ff 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -182,7 +182,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	switch (ext) {
 	case KVM_CAP_IRQCHIP:
 	case KVM_CAP_MP_STATE:
-
+	case KVM_CAP_IRQ_INJECT_STATUS:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:
@@ -925,6 +925,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			goto out;
 		}
 		break;
+	case KVM_IRQ_LINE_STATUS:
 	case KVM_IRQ_LINE: {
 		struct kvm_irq_level irq_event;
 
@@ -932,10 +933,17 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (copy_from_user(&irq_event, argp, sizeof irq_event))
 			goto out;
 		if (irqchip_in_kernel(kvm)) {
+			__s32 status;
 			mutex_lock(&kvm->lock);
-			kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+			status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
 				    irq_event.irq, irq_event.level);
 			mutex_unlock(&kvm->lock);
+			if (ioctl == KVM_IRQ_LINE_STATUS) {
+				irq_event.status = status;
+				if (copy_to_user(argp, &irq_event,
+							sizeof irq_event))
+					goto out;
+			}
 			r = 0;
 		}
 		break;
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 55fd4c5..f0faf58 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -616,7 +616,7 @@ void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code);
 void kvm_inject_page_fault(struct kvm_vcpu *vcpu, unsigned long cr2,
 			   u32 error_code);
 
-void kvm_pic_set_irq(void *opaque, int irq, int level);
+int kvm_pic_set_irq(void *opaque, int irq, int level);
 
 void kvm_inject_nmi(struct kvm_vcpu *vcpu);
 
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 179dcb0..2752016 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -76,12 +76,13 @@ void kvm_pic_clear_isr_ack(struct kvm *kvm)
 /*
  * set irq level. If an edge is detected, then the IRR is set to 1
  */
-static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
+static inline int pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
 {
-	int mask;
+	int mask, ret = 1;
 	mask = 1 << irq;
 	if (s->elcr & mask)	/* level triggered */
 		if (level) {
+			ret = !(s->irr & mask);
 			s->irr |= mask;
 			s->last_irr |= mask;
 		} else {
@@ -90,11 +91,15 @@ static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
 		}
 	else	/* edge triggered */
 		if (level) {
-			if ((s->last_irr & mask) == 0)
+			if ((s->last_irr & mask) == 0) {
+				ret = !(s->irr & mask);
 				s->irr |= mask;
+			}
 			s->last_irr |= mask;
 		} else
 			s->last_irr &= ~mask;
+
+	return (s->imr & mask) ? -1 : ret;
 }
 
 /*
@@ -171,16 +176,19 @@ void kvm_pic_update_irq(struct kvm_pic *s)
 	pic_unlock(s);
 }
 
-void kvm_pic_set_irq(void *opaque, int irq, int level)
+int kvm_pic_set_irq(void *opaque, int irq, int level)
 {
 	struct kvm_pic *s = opaque;
+	int ret = -1;
 
 	pic_lock(s);
 	if (irq >= 0 && irq < PIC_NUM_PINS) {
-		pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
+		ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
 		pic_update_irq(s);
 	}
 	pic_unlock(s);
+
+	return ret;
 }
 
 /*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e96edda..5754c37 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -995,6 +995,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 	case KVM_CAP_MP_STATE:
 	case KVM_CAP_SYNC_MMU:
 	case KVM_CAP_REINJECT_CONTROL:
+	case KVM_CAP_IRQ_INJECT_STATUS:
 		r = 1;
 		break;
 	case KVM_CAP_COALESCED_MMIO:
@@ -1852,6 +1853,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	create_pit_unlock:
 		mutex_unlock(&kvm->lock);
 		break;
+	case KVM_IRQ_LINE_STATUS:
 	case KVM_IRQ_LINE: {
 		struct kvm_irq_level irq_event;
 
@@ -1859,10 +1861,17 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		if (copy_from_user(&irq_event, argp, sizeof irq_event))
 			goto out;
 		if (irqchip_in_kernel(kvm)) {
+			__s32 status;
 			mutex_lock(&kvm->lock);
-			kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
-				    irq_event.irq, irq_event.level);
+			status = kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+					irq_event.irq, irq_event.level);
 			mutex_unlock(&kvm->lock);
+			if (ioctl == KVM_IRQ_LINE_STATUS) {
+				irq_event.status = status;
+				if (copy_to_user(argp, &irq_event,
+							sizeof irq_event))
+					goto out;
+			}
 			r = 0;
 		}
 		break;
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 869462c..968773d 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -48,7 +48,10 @@ struct kvm_irq_level {
 	 * For IA-64 (APIC model) IOAPIC0: irq 0-23; IOAPIC1: irq 24-47..
 	 * For X86 (standard AT mode) PIC0/1: irq 0-15. IOAPIC0: 0-23..
 	 */
-	__u32 irq;
+	union {
+		__u32 irq;
+		__s32 status;
+	};
 	__u32 level;
 };
 
@@ -402,6 +405,7 @@ struct kvm_trace_rec {
 #ifdef __KVM_HAVE_IOAPIC
 #define KVM_CAP_IRQ_ROUTING 25
 #endif
+#define KVM_CAP_IRQ_INJECT_STATUS 26
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -456,6 +460,7 @@ struct kvm_irq_routing {
 #define KVM_CREATE_PIT		  _IO(KVMIO,  0x64)
 #define KVM_GET_PIT		  _IOWR(KVMIO, 0x65, struct kvm_pit_state)
 #define KVM_SET_PIT		  _IOR(KVMIO,  0x66, struct kvm_pit_state)
+#define KVM_IRQ_LINE_STATUS	  _IOWR(KVMIO, 0x67, struct kvm_irq_level)
 #define KVM_REGISTER_COALESCED_MMIO \
 			_IOW(KVMIO,  0x67, struct kvm_coalesced_mmio_zone)
 #define KVM_UNREGISTER_COALESCED_MMIO \
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ce285e0..a8f91bb 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -109,7 +109,7 @@ struct kvm_memory_slot {
 
 struct kvm_kernel_irq_routing_entry {
 	u32 gsi;
-	void (*set)(struct kvm_kernel_irq_routing_entry *e,
+	int (*set)(struct kvm_kernel_irq_routing_entry *e,
 		    struct kvm *kvm, int level);
 	union {
 		struct {
@@ -351,7 +351,7 @@ void kvm_unregister_irq_mask_notifier(struct kvm *kvm, int irq,
 				      struct kvm_irq_mask_notifier *kimn);
 void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask);
 
-void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
+int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index e85a2bc..8920a60 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -83,19 +83,22 @@ static unsigned long ioapic_read_indirect(struct kvm_ioapic *ioapic,
 	return result;
 }
 
-static void ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
+static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx)
 {
 	union ioapic_redir_entry *pent;
+	int injected = -1;
 
 	pent = &ioapic->redirtbl[idx];
 
 	if (!pent->fields.mask) {
-		int injected = ioapic_deliver(ioapic, idx);
+		injected = ioapic_deliver(ioapic, idx);
 		if (injected && pent->fields.trig_mode == IOAPIC_LEVEL_TRIG)
 			pent->fields.remote_irr = 1;
 	}
 	if (!pent->fields.trig_mode)
 		ioapic->irr &= ~(1 << idx);
+
+	return injected;
 }
 
 static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
@@ -273,11 +276,12 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 	return r;
 }
 
-void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
+int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 {
 	u32 old_irr = ioapic->irr;
 	u32 mask = 1 << irq;
 	union ioapic_redir_entry entry;
+	int ret = 1;
 
 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
 		entry = ioapic->redirtbl[irq];
@@ -288,9 +292,10 @@ void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 			ioapic->irr |= mask;
 			if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
 			    || !entry.fields.remote_irr)
-				ioapic_service(ioapic, irq);
+				ret = ioapic_service(ioapic, irq);
 		}
 	}
+	return ret;
 }
 
 static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int gsi,
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index 49c9581..a34bd5e 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -83,7 +83,7 @@ struct kvm_vcpu *kvm_get_lowest_prio_vcpu(struct kvm *kvm, u8 vector,
 				       unsigned long bitmap);
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
 int kvm_ioapic_init(struct kvm *kvm);
-void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
+int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
 void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
 u32 kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
 				u8 dest_mode);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index a797fa5..00bb29f 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -24,25 +24,28 @@
 
 #include "ioapic.h"
 
-static void kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
+static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
 			    struct kvm *kvm, int level)
 {
 #ifdef CONFIG_X86
-	kvm_pic_set_irq(pic_irqchip(kvm), e->irqchip.pin, level);
+	return kvm_pic_set_irq(pic_irqchip(kvm), e->irqchip.pin, level);
+#else
+	return -1;
 #endif
 }
 
-static void kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
+static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
 			       struct kvm *kvm, int level)
 {
-	kvm_ioapic_set_irq(kvm->arch.vioapic, e->irqchip.pin, level);
+	return kvm_ioapic_set_irq(kvm->arch.vioapic, e->irqchip.pin, level);
 }
 
 /* This should be called with the kvm->lock mutex held */
-void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
+int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
 {
 	struct kvm_kernel_irq_routing_entry *e;
 	unsigned long *irq_state = (unsigned long *)&kvm->arch.irq_states[irq];
+	int ret = -1;
 
 	/* Logical OR for level trig interrupt */
 	if (level)
@@ -55,8 +58,11 @@ void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
 	 * writes to the unused one.
 	 */
 	list_for_each_entry(e, &kvm->irq_routing, link)
-		if (e->gsi == irq)
-			e->set(e, kvm, !!(*irq_state));
+		if (e->gsi == irq) {
+			int r = e->set(e, kvm, !!(*irq_state));
+			ret = max(ret, r);
+		}
+	return ret;
 }
 
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi)
@@ -165,7 +171,7 @@ int setup_routing_entry(struct kvm_kernel_irq_routing_entry *e,
 			e->set = kvm_set_pic_irq;
 			break;
 		case KVM_IRQCHIP_PIC_SLAVE:
-				e->set = kvm_set_pic_irq;
+			e->set = kvm_set_pic_irq;
 			delta = 8;
 			break;
 		case KVM_IRQCHIP_IOAPIC:
--
			Gleb.

Re: [PATCH v2] report IRQ injection status to userspace.

[Marcelo Tosatti]

Hi Gleb,

On Wed, Jan 21, 2009 at 02:34:28PM +0200, Gleb Natapov wrote:
> Use this one instead. Adds capabilities checks.
> 
> Signed-off-by: Gleb Natapov <gleb@redhat.com>

> index 179dcb0..2752016 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -76,12 +76,13 @@ void kvm_pic_clear_isr_ack(struct kvm *kvm)
>  /*
>   * set irq level. If an edge is detected, then the IRR is set to 1
>   */
> -static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
> +static inline int pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
>  {
> -	int mask;
> +	int mask, ret = 1;
>  	mask = 1 << irq;
>  	if (s->elcr & mask)	/* level triggered */
>  		if (level) {
> +			ret = !(s->irr & mask);
>  			s->irr |= mask;
>  			s->last_irr |= mask;
>  		} else {
> @@ -90,11 +91,15 @@ static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
>  		}
>  	else	/* edge triggered */
>  		if (level) {
> -			if ((s->last_irr & mask) == 0)
> +			if ((s->last_irr & mask) == 0) {
> +				ret = !(s->irr & mask);
>  				s->irr |= mask;
> +			}
>  			s->last_irr |= mask;
>  		} else
>  			s->last_irr &= ~mask;
> +
> +	return (s->imr & mask) ? -1 : ret;
>  }

Can you add some documentation, perhaps through definitions, like:

#define KVM_IRQ_LINE_STATUS_MASKED -1
#define KVM_IRQ_LINE_STATUS_FAIL 0
#define KVM_IRQ_LINE_STATUS_INJECTED 1

But with better names. This makes the kernel code more explicit too.

> -void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
>  {
>  	u32 old_irr = ioapic->irr;
>  	u32 mask = 1 << irq;
>  	union ioapic_redir_entry entry;
> +	int ret = 1;

-1 here ?

And then, in the userspace part, you consider -1 as "injected": 

diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
index 6d41a5e..9da4360 100644
--- a/qemu/hw/i8259.c
+++ b/qemu/hw/i8259.c
@@ -186,9 +186,14 @@ static void i8259_set_irq(void *opaque, int irq,
int level)
 {
     PicState2 *s = opaque;
 #ifdef KVM_CAP_IRQCHIP
-    if (kvm_enabled())
-       if (kvm_set_irq(irq, level))
-           return;
+    if (kvm_enabled()) {
+        int pic_ret;
+        if (kvm_set_irq(irq, level, &pic_ret)) {
+            if (pic_ret != 0)
+                apic_set_irq_delivered();
+            return;
+        }
+    }
 #endif

Is that what you intended ? 

Other than that looks fine to me.

Re: [PATCH v2] report IRQ injection status to userspace.

[Gleb Natapov]

On Mon, Jan 26, 2009 at 02:10:38PM -0200, Marcelo Tosatti wrote:
> Hi Gleb,
> 
> On Wed, Jan 21, 2009 at 02:34:28PM +0200, Gleb Natapov wrote:
> > Use this one instead. Adds capabilities checks.
> > 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> 
> > index 179dcb0..2752016 100644
> > --- a/arch/x86/kvm/i8259.c
> > +++ b/arch/x86/kvm/i8259.c
> > @@ -76,12 +76,13 @@ void kvm_pic_clear_isr_ack(struct kvm *kvm)
> >  /*
> >   * set irq level. If an edge is detected, then the IRR is set to 1
> >   */
> > -static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
> > +static inline int pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
> >  {
> > -	int mask;
> > +	int mask, ret = 1;
> >  	mask = 1 << irq;
> >  	if (s->elcr & mask)	/* level triggered */
> >  		if (level) {
> > +			ret = !(s->irr & mask);
> >  			s->irr |= mask;
> >  			s->last_irr |= mask;
> >  		} else {
> > @@ -90,11 +91,15 @@ static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level)
> >  		}
> >  	else	/* edge triggered */
> >  		if (level) {
> > -			if ((s->last_irr & mask) == 0)
> > +			if ((s->last_irr & mask) == 0) {
> > +				ret = !(s->irr & mask);
> >  				s->irr |= mask;
> > +			}
> >  			s->last_irr |= mask;
> >  		} else
> >  			s->last_irr &= ~mask;
> > +
> > +	return (s->imr & mask) ? -1 : ret;
> >  }
> 
> Can you add some documentation, perhaps through definitions, like:
> 
> #define KVM_IRQ_LINE_STATUS_MASKED -1
> #define KVM_IRQ_LINE_STATUS_FAIL 0
> #define KVM_IRQ_LINE_STATUS_INJECTED 1
> 
> But with better names. This makes the kernel code more explicit too.
> 
To find a good name is the hard problem ;) I'll resend.

> > -void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> > +int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
> >  {
> >  	u32 old_irr = ioapic->irr;
> >  	u32 mask = 1 << irq;
> >  	union ioapic_redir_entry entry;
> > +	int ret = 1;
> 
> -1 here ?
> 
I think 1 is better here. For level=0 we always want to report that interrupt
was injected and for the case of edge triggered interrupt and level=1
ioapic_service() will always be called. BTW it seems that expression
old_irr != ioapic->irr in:
            if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
                || !entry.fields.remote_irr)
                ret = ioapic_service(ioapic, irq);
Will always be true since for edge triggered interrupt irr is always
cleared by ioapic_service(). Am I right?

> And then, in the userspace part, you consider -1 as "injected": 
> 
> diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
> index 6d41a5e..9da4360 100644
> --- a/qemu/hw/i8259.c
> +++ b/qemu/hw/i8259.c
> @@ -186,9 +186,14 @@ static void i8259_set_irq(void *opaque, int irq,
> int level)
>  {
>      PicState2 *s = opaque;
>  #ifdef KVM_CAP_IRQCHIP
> -    if (kvm_enabled())
> -       if (kvm_set_irq(irq, level))
> -           return;
> +    if (kvm_enabled()) {
> +        int pic_ret;
> +        if (kvm_set_irq(irq, level, &pic_ret)) {
> +            if (pic_ret != 0)
> +                apic_set_irq_delivered();
> +            return;
> +        }
> +    }
>  #endif
> 
> Is that what you intended ? 
> 
Yes! If interrupt was lost due to making it should not be reinjected.

--
			Gleb.

Re: [PATCH v2] report IRQ injection status to userspace.

[Marcelo Tosatti]

On Tue, Jan 27, 2009 at 03:27:39PM +0200, Gleb Natapov wrote:
> > -1 here ?
> > 
> I think 1 is better here. For level=0 we always want to report that interrupt
> was injected and for the case of edge triggered interrupt and level=1
> ioapic_service() will always be called. BTW it seems that expression
> old_irr != ioapic->irr in:
>             if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
>                 || !entry.fields.remote_irr)
>                 ret = ioapic_service(ioapic, irq);
> Will always be true since for edge triggered interrupt irr is always
> cleared by ioapic_service(). Am I right?

Right, I was thinking about

	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {

Should return MASKED if irq is outside the acceptable range?

> > +        }
> > +    }
> >  #endif
> > 
> > Is that what you intended ? 
> > 
> Yes! If interrupt was lost due to making it should not be reinjected.

That assumes guests won't mask the interrupt temporarily in the irqchip,
hope that is OK (as Avi noted earlier guests use CPU to mask irqs
temporarily, most of the time).

Re: [PATCH v2] report IRQ injection status to userspace.

[Gleb Natapov]

On Tue, Jan 27, 2009 at 01:41:07PM -0200, Marcelo Tosatti wrote:
> On Tue, Jan 27, 2009 at 03:27:39PM +0200, Gleb Natapov wrote:
> > > -1 here ?
> > > 
> > I think 1 is better here. For level=0 we always want to report that interrupt
> > was injected and for the case of edge triggered interrupt and level=1
> > ioapic_service() will always be called. BTW it seems that expression
> > old_irr != ioapic->irr in:
> >             if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
> >                 || !entry.fields.remote_irr)
> >                 ret = ioapic_service(ioapic, irq);
> > Will always be true since for edge triggered interrupt irr is always
> > cleared by ioapic_service(). Am I right?
> 
> Right, I was thinking about
> 
> 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> 
> Should return MASKED if irq is outside the acceptable range?
> 
Is this ever can be false? Should we BUG() if irq is out of range?

> > > +        }
> > > +    }
> > >  #endif
> > > 
> > > Is that what you intended ? 
> > > 
> > Yes! If interrupt was lost due to making it should not be reinjected.
> 
> That assumes guests won't mask the interrupt temporarily in the irqchip,
> hope that is OK (as Avi noted earlier guests use CPU to mask irqs
> temporarily, most of the time).
And if a guest masks interrupts it can't complain that some are lost. I
haven't seen Windows masking RTC irq.

--
			Gleb.

Re: [PATCH v2] report IRQ injection status to userspace.

[Marcelo Tosatti]

On Wed, Jan 28, 2009 at 06:37:36PM +0200, Gleb Natapov wrote:
> On Tue, Jan 27, 2009 at 01:41:07PM -0200, Marcelo Tosatti wrote:
> > On Tue, Jan 27, 2009 at 03:27:39PM +0200, Gleb Natapov wrote:
> > > > -1 here ?
> > > > 
> > > I think 1 is better here. For level=0 we always want to report that interrupt
> > > was injected and for the case of edge triggered interrupt and level=1
> > > ioapic_service() will always be called. BTW it seems that expression
> > > old_irr != ioapic->irr in:
> > >             if ((!entry.fields.trig_mode && old_irr != ioapic->irr)
> > >                 || !entry.fields.remote_irr)
> > >                 ret = ioapic_service(ioapic, irq);
> > > Will always be true since for edge triggered interrupt irr is always
> > > cleared by ioapic_service(). Am I right?
> > 
> > Right, I was thinking about
> > 
> > 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
> > 
> > Should return MASKED if irq is outside the acceptable range?
> > 
> Is this ever can be false? Should we BUG() if irq is out of range?

If qemu-kvm passes it ouf range IRQ yes. Its just a nitpicking, ignore
it.

> > That assumes guests won't mask the interrupt temporarily in the irqchip,
> > hope that is OK (as Avi noted earlier guests use CPU to mask irqs
> > temporarily, most of the time).
> And if a guest masks interrupts it can't complain that some are lost. I
> haven't seen Windows masking RTC irq.

Makes sense.

Re: [PATCH v2] report IRQ injection status to userspace.

[Gleb Natapov]

On Mon, Feb 02, 2009 at 04:04:55PM +0200, Avi Kivity wrote:
> Gleb Natapov wrote:
>>> Right, I was thinking about
>>>
>>> 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
>>>
>>> Should return MASKED if irq is outside the acceptable range?
>>>
>>>     
>> Is this ever can be false? Should we BUG() if irq is out of range?
>>
>>   
>
> Yes, the number ultimately comes from userspace.
>
So may be -EINVAL should be returned to userspace?

--
			Gleb.

Re: [PATCH v2] report IRQ injection status to userspace.

[Avi Kivity]

Gleb Natapov wrote:
>> Right, I was thinking about
>>
>> 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
>>
>> Should return MASKED if irq is outside the acceptable range?
>>
>>     
> Is this ever can be false? Should we BUG() if irq is out of range?
>
>   

Yes, the number ultimately comes from userspace.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2] report IRQ injection status to userspace.

[Avi Kivity]

Gleb Natapov wrote:
> On Mon, Feb 02, 2009 at 04:04:55PM +0200, Avi Kivity wrote:
>   
>> Gleb Natapov wrote:
>>     
>>>> Right, I was thinking about
>>>>
>>>> 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
>>>>
>>>> Should return MASKED if irq is outside the acceptable range?
>>>>
>>>>     
>>>>         
>>> Is this ever can be false? Should we BUG() if irq is out of range?
>>>
>>>   
>>>       
>> Yes, the number ultimately comes from userspace.
>>
>>     
> So may be -EINVAL should be returned to userspace?
>   

Mmm, not sure.  An out-of-bounds number here could be caused by 
userspace generating the wrong irq line number, or by the guest 
misconfiguring interrupts.

We should error out on userspace bugs, but not guest bugs.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2] report IRQ injection status to userspace.

[Marcelo Tosatti]

On Mon, Feb 02, 2009 at 04:23:40PM +0200, Avi Kivity wrote:
> Gleb Natapov wrote:
>> On Mon, Feb 02, 2009 at 04:04:55PM +0200, Avi Kivity wrote:
>>   
>>> Gleb Natapov wrote:
>>>     
>>>>> Right, I was thinking about
>>>>>
>>>>> 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
>>>>>
>>>>> Should return MASKED if irq is outside the acceptable range?
>>>>>
>>>>>             
>>>> Is this ever can be false? Should we BUG() if irq is out of range?
>>>>
>>>>         
>>> Yes, the number ultimately comes from userspace.
>>>
>>>     
>> So may be -EINVAL should be returned to userspace?
>>   
>
> Mmm, not sure.  An out-of-bounds number here could be caused by  
> userspace generating the wrong irq line number, or by the guest  
> misconfiguring interrupts.
>
> We should error out on userspace bugs, but not guest bugs.

I don't see this applied yet ?