kvm: enable irq injection from interrupt context

To avoid bouncing irqfd injection out to workqueue context, we must support injecting irqs from local interrupt context. Doing this seems to only require disabling irqs locally.

RFC, completely untested, x86 only.

Thoughts?

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/x86/kvm/i8259.c |   12 ++++++++++++
 virt/kvm/eventfd.c   |   30 +++++++++---------------------
 virt/kvm/ioapic.c    |   34 ++++++++++++++++++++--------------
 3 files changed, 41 insertions(+), 35 deletions(-)

arch/x86/kvm/i8259.c +12 -0

index 8d10c06..294fa36 100644
@@ -198,8 +198,10 @@ void kvm_pic_update_irq(struct kvm_pic *s)
 int kvm_pic_set_irq(void *opaque, int irq, int level)
 {
 	struct kvm_pic *s = opaque;
+	unsigned long flags;
 	int ret = -1;
 
+	local_irq_save(flags);
 	pic_lock(s);
 	if (irq >= 0 && irq < PIC_NUM_PINS) {
 		ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);

@@ -208,6 +210,7 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
 				      s->pics[irq >> 3].imr, ret == 0);
 	}
 	pic_unlock(s);
+	local_irq_restore(flags);
 
 	return ret;
 }

@@ -235,8 +238,10 @@ static inline void pic_intack(struct kvm_kpic_state *s, int irq)
 int kvm_pic_read_irq(struct kvm *kvm)
 {
 	int irq, irq2, intno;
+	unsigned long flags;
 	struct kvm_pic *s = pic_irqchip(kvm);
 
+	local_irq_save(flags);
 	pic_lock(s);
 	irq = pic_get_irq(&s->pics[0]);
 	if (irq >= 0) {

@@ -263,6 +268,7 @@ int kvm_pic_read_irq(struct kvm *kvm)
 	}
 	pic_update_irq(s);
 	pic_unlock(s);
+	local_irq_restore(flags);
 
 	return intno;
 }

@@ -476,6 +482,7 @@ static int picdev_write(struct kvm_io_device *this,
 {
 	struct kvm_pic *s = to_pic(this);
 	unsigned char data = *(unsigned char *)val;
+	unsigned long flags;
 	if (!picdev_in_range(addr))
 		return -EOPNOTSUPP;
 

@@ -484,6 +491,7 @@ static int picdev_write(struct kvm_io_device *this,
 			printk(KERN_ERR "PIC: non byte write\n");
 		return 0;
 	}
+	local_irq_save(flags);
 	pic_lock(s);
 	switch (addr) {
 	case 0x20:

@@ -498,6 +506,7 @@ static int picdev_write(struct kvm_io_device *this,
 		break;
 	}
 	pic_unlock(s);
+	local_irq_restore(flags);
 	return 0;
 }
 

@@ -506,6 +515,7 @@ static int picdev_read(struct kvm_io_device *this,
 {
 	struct kvm_pic *s = to_pic(this);
 	unsigned char data = 0;
+	unsigned long flags;
 	if (!picdev_in_range(addr))
 		return -EOPNOTSUPP;
 

@@ -514,6 +524,7 @@ static int picdev_read(struct kvm_io_device *this,
 			printk(KERN_ERR "PIC: non byte read\n");
 		return 0;
 	}
+	local_irq_save(flags);
 	pic_lock(s);
 	switch (addr) {
 	case 0x20:

@@ -529,6 +540,7 @@ static int picdev_read(struct kvm_io_device *this,
 	}
 	*(unsigned char *)val = data;
 	pic_unlock(s);
+	local_irq_restore(flags);
 	return 0;
 }
 

virt/kvm/eventfd.c +9 -21

index 66cf65b..30ede90 100644
@@ -50,22 +50,11 @@ struct _irqfd {
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_t              wait;
-	struct work_struct        inject;
 	struct work_struct        shutdown;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;
 
-static void
-irqfd_inject(struct work_struct *work)
-{
-	struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
-	struct kvm *kvm = irqfd->kvm;
-
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
-}
-
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -82,12 +71,6 @@ irqfd_shutdown(struct work_struct *work)
 	eventfd_ctx_remove_wait_queue(irqfd->eventfd, &irqfd->wait, &cnt);
 
 	/*
-	 * We know no new events will be scheduled at this point, so block
-	 * until all previously outstanding events have completed
-	 */
-	flush_work(&irqfd->inject);
-
-	/*
 	 * It is now safe to release the object's resources
 	 */
 	eventfd_ctx_put(irqfd->eventfd);

@@ -128,7 +111,8 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 
 	if (flags & POLLIN)
 		/* An event has been signaled, inject an interrupt */
-		schedule_work(&irqfd->inject);
+		kvm_set_irq(irqfd->kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+			    irqfd->gsi, 1);
 
 	if (flags & POLLHUP) {
 		/* The eventfd is closing, detach from KVM */

@@ -179,7 +163,6 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
 	INIT_LIST_HEAD(&irqfd->list);
-	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
 
 	file = eventfd_fget(fd);

@@ -218,14 +201,19 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	events = file->f_op->poll(file, &irqfd->pt);
 
 	list_add_tail(&irqfd->list, &kvm->irqfds.items);
-	spin_unlock_irq(&kvm->irqfds.lock);
 
 	/*
 	 * Check if there was an event already pending on the eventfd
 	 * before we registered, and trigger it as if we didn't miss it.
+	 *
+	 * Do this under irqfds.lock, this way we can be sure
+	 * the irqfd is not going away.
 	 */
 	if (events & POLLIN)
-		schedule_work(&irqfd->inject);
+		kvm_set_irq(irqfd->kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+			    irqfd->gsi, 1);
+
+	spin_unlock_irq(&kvm->irqfds.lock);
 
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise

virt/kvm/ioapic.c +20 -14

index 0b9df83..cc1d2fe 100644
@@ -197,8 +197,9 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 	u32 mask = 1 << irq;
 	union kvm_ioapic_redirect_entry entry;
 	int ret = 1;
+	unsigned long flags;
 
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	old_irr = ioapic->irr;
 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
 		entry = ioapic->redirtbl[irq];

@@ -216,7 +217,7 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 		}
 		trace_kvm_ioapic_set_irq(entry.bits, irq, ret == 0);
 	}
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 
 	return ret;
 }

@@ -240,9 +241,9 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
 		 * is dropped it will be put into irr and will be delivered
 		 * after ack notifier returns.
 		 */
-		spin_unlock(&ioapic->lock);
+		spin_unlock_irqrestore(&ioapic->lock, flags);
 		kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i);
-		spin_lock(&ioapic->lock);
+		spin_lock_irqsave(&ioapic->lock, flags);
 
 		if (trigger_mode != IOAPIC_LEVEL_TRIG)
 			continue;

@@ -257,13 +258,14 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
+	unsigned long flags;
 
 	smp_rmb();
 	if (!test_bit(vector, ioapic->handled_vectors))
 		return;
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	__kvm_ioapic_update_eoi(ioapic, vector, trigger_mode);
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 }
 
 static inline struct kvm_ioapic *to_ioapic(struct kvm_io_device *dev)

@@ -281,6 +283,7 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 			    void *val)
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
+	unsigned long flags;
 	u32 result;
 	if (!ioapic_in_range(ioapic, addr))
 		return -EOPNOTSUPP;

@@ -289,7 +292,7 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 	ASSERT(!(addr & 0xf));	/* check alignment */
 
 	addr &= 0xff;
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	switch (addr) {
 	case IOAPIC_REG_SELECT:
 		result = ioapic->ioregsel;

@@ -303,7 +306,7 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 		result = 0;
 		break;
 	}
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 
 	switch (len) {
 	case 8:

@@ -324,6 +327,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 			     const void *val)
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
+	unsigned long flags;
 	u32 data;
 	if (!ioapic_in_range(ioapic, addr))
 		return -EOPNOTSUPP;

@@ -340,7 +344,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 	}
 
 	addr &= 0xff;
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	switch (addr) {
 	case IOAPIC_REG_SELECT:
 		ioapic->ioregsel = data;

@@ -358,7 +362,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 	default:
 		break;
 	}
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 	return 0;
 }
 

@@ -418,24 +422,26 @@ void kvm_ioapic_destroy(struct kvm *kvm)
 int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
+	unsigned long flags;
 	if (!ioapic)
 		return -EINVAL;
 
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	memcpy(state, ioapic, sizeof(struct kvm_ioapic_state));
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 	return 0;
 }
 
 int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
+	unsigned long flags;
 	if (!ioapic)
 		return -EINVAL;
 
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
 	update_handled_vectors(ioapic);
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 	return 0;
 }

Signature · 1 line

1.7.3.rc1.5.ge5969

Quoted 5 lines · Michael S. Tsirkin

To avoid bouncing irqfd injection out to workqueue context, we must support injecting irqs from local interrupt context. Doing this seems to only require disabling irqs locally.

Quoted 4 lines

RFC, completely untested, x86 only.

Thoughts?

We do not want to disable irqs for a long time and some of code paths under lock involve looping over all cpus. For MSI injection path is lockless and this is the only case that matters,

so inject irqs from
local interrupt context if it is MSI or from workqueue otherwise.

Quoted 315 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 arch/x86/kvm/i8259.c |   12 ++++++++++++
 virt/kvm/eventfd.c   |   30 +++++++++---------------------
 virt/kvm/ioapic.c    |   34 ++++++++++++++++++++--------------
 3 files changed, 41 insertions(+), 35 deletions(-)

arch/x86/kvm/i8259.c +12 -0

index 8d10c06..294fa36 100644
@@ -198,8 +198,10 @@ void kvm_pic_update_irq(struct kvm_pic *s)
 int kvm_pic_set_irq(void *opaque, int irq, int level)
 {
 	struct kvm_pic *s = opaque;
+	unsigned long flags;
 	int ret = -1;
 
+	local_irq_save(flags);
 	pic_lock(s);
 	if (irq >= 0 && irq < PIC_NUM_PINS) {
 		ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);

@@ -208,6 +210,7 @@ int kvm_pic_set_irq(void *opaque, int irq, int level)
 				      s->pics[irq >> 3].imr, ret == 0);
 	}
 	pic_unlock(s);
+	local_irq_restore(flags);
 
 	return ret;
 }

@@ -235,8 +238,10 @@ static inline void pic_intack(struct kvm_kpic_state *s, int irq)
 int kvm_pic_read_irq(struct kvm *kvm)
 {
 	int irq, irq2, intno;
+	unsigned long flags;
 	struct kvm_pic *s = pic_irqchip(kvm);
 
+	local_irq_save(flags);
 	pic_lock(s);
 	irq = pic_get_irq(&s->pics[0]);
 	if (irq >= 0) {

@@ -263,6 +268,7 @@ int kvm_pic_read_irq(struct kvm *kvm)
 	}
 	pic_update_irq(s);
 	pic_unlock(s);
+	local_irq_restore(flags);
 
 	return intno;
 }

@@ -476,6 +482,7 @@ static int picdev_write(struct kvm_io_device *this,
 {
 	struct kvm_pic *s = to_pic(this);
 	unsigned char data = *(unsigned char *)val;
+	unsigned long flags;
 	if (!picdev_in_range(addr))
 		return -EOPNOTSUPP;
 

@@ -484,6 +491,7 @@ static int picdev_write(struct kvm_io_device *this,
 			printk(KERN_ERR "PIC: non byte write\n");
 		return 0;
 	}
+	local_irq_save(flags);
 	pic_lock(s);
 	switch (addr) {
 	case 0x20:

@@ -498,6 +506,7 @@ static int picdev_write(struct kvm_io_device *this,
 		break;
 	}
 	pic_unlock(s);
+	local_irq_restore(flags);
 	return 0;
 }
 

@@ -506,6 +515,7 @@ static int picdev_read(struct kvm_io_device *this,
 {
 	struct kvm_pic *s = to_pic(this);
 	unsigned char data = 0;
+	unsigned long flags;
 	if (!picdev_in_range(addr))
 		return -EOPNOTSUPP;
 

@@ -514,6 +524,7 @@ static int picdev_read(struct kvm_io_device *this,
 			printk(KERN_ERR "PIC: non byte read\n");
 		return 0;
 	}
+	local_irq_save(flags);
 	pic_lock(s);
 	switch (addr) {
 	case 0x20:

@@ -529,6 +540,7 @@ static int picdev_read(struct kvm_io_device *this,
 	}
 	*(unsigned char *)val = data;
 	pic_unlock(s);
+	local_irq_restore(flags);
 	return 0;
 }
 

virt/kvm/eventfd.c +9 -21

index 66cf65b..30ede90 100644
@@ -50,22 +50,11 @@ struct _irqfd {
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_t              wait;
-	struct work_struct        inject;
 	struct work_struct        shutdown;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;
 
-static void
-irqfd_inject(struct work_struct *work)
-{
-	struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
-	struct kvm *kvm = irqfd->kvm;
-
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
-	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
-}
-
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -82,12 +71,6 @@ irqfd_shutdown(struct work_struct *work)
 	eventfd_ctx_remove_wait_queue(irqfd->eventfd, &irqfd->wait, &cnt);
 
 	/*
-	 * We know no new events will be scheduled at this point, so block
-	 * until all previously outstanding events have completed
-	 */
-	flush_work(&irqfd->inject);
-
-	/*
 	 * It is now safe to release the object's resources
 	 */
 	eventfd_ctx_put(irqfd->eventfd);

@@ -128,7 +111,8 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 
 	if (flags & POLLIN)
 		/* An event has been signaled, inject an interrupt */
-		schedule_work(&irqfd->inject);
+		kvm_set_irq(irqfd->kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+			    irqfd->gsi, 1);
 
 	if (flags & POLLHUP) {
 		/* The eventfd is closing, detach from KVM */

@@ -179,7 +163,6 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
 	INIT_LIST_HEAD(&irqfd->list);
-	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
 
 	file = eventfd_fget(fd);

@@ -218,14 +201,19 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 	events = file->f_op->poll(file, &irqfd->pt);
 
 	list_add_tail(&irqfd->list, &kvm->irqfds.items);
-	spin_unlock_irq(&kvm->irqfds.lock);
 
 	/*
 	 * Check if there was an event already pending on the eventfd
 	 * before we registered, and trigger it as if we didn't miss it.
+	 *
+	 * Do this under irqfds.lock, this way we can be sure
+	 * the irqfd is not going away.
 	 */
 	if (events & POLLIN)
-		schedule_work(&irqfd->inject);
+		kvm_set_irq(irqfd->kvm, KVM_USERSPACE_IRQ_SOURCE_ID,
+			    irqfd->gsi, 1);
+
+	spin_unlock_irq(&kvm->irqfds.lock);
 
 	/*
 	 * do not drop the file until the irqfd is fully initialized, otherwise

virt/kvm/ioapic.c +20 -14

index 0b9df83..cc1d2fe 100644
@@ -197,8 +197,9 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 	u32 mask = 1 << irq;
 	union kvm_ioapic_redirect_entry entry;
 	int ret = 1;
+	unsigned long flags;
 
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	old_irr = ioapic->irr;
 	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
 		entry = ioapic->redirtbl[irq];

@@ -216,7 +217,7 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level)
 		}
 		trace_kvm_ioapic_set_irq(entry.bits, irq, ret == 0);
 	}
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 
 	return ret;
 }

@@ -240,9 +241,9 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
 		 * is dropped it will be put into irr and will be delivered
 		 * after ack notifier returns.
 		 */
-		spin_unlock(&ioapic->lock);
+		spin_unlock_irqrestore(&ioapic->lock, flags);
 		kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i);
-		spin_lock(&ioapic->lock);
+		spin_lock_irqsave(&ioapic->lock, flags);
 
 		if (trigger_mode != IOAPIC_LEVEL_TRIG)
 			continue;

@@ -257,13 +258,14 @@ static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector,
 void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
+	unsigned long flags;
 
 	smp_rmb();
 	if (!test_bit(vector, ioapic->handled_vectors))
 		return;
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	__kvm_ioapic_update_eoi(ioapic, vector, trigger_mode);
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 }
 
 static inline struct kvm_ioapic *to_ioapic(struct kvm_io_device *dev)

@@ -281,6 +283,7 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 			    void *val)
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
+	unsigned long flags;
 	u32 result;
 	if (!ioapic_in_range(ioapic, addr))
 		return -EOPNOTSUPP;

@@ -289,7 +292,7 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 	ASSERT(!(addr & 0xf));	/* check alignment */
 
 	addr &= 0xff;
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	switch (addr) {
 	case IOAPIC_REG_SELECT:
 		result = ioapic->ioregsel;

@@ -303,7 +306,7 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len,
 		result = 0;
 		break;
 	}
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 
 	switch (len) {
 	case 8:

@@ -324,6 +327,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 			     const void *val)
 {
 	struct kvm_ioapic *ioapic = to_ioapic(this);
+	unsigned long flags;
 	u32 data;
 	if (!ioapic_in_range(ioapic, addr))
 		return -EOPNOTSUPP;

@@ -340,7 +344,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 	}
 
 	addr &= 0xff;
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	switch (addr) {
 	case IOAPIC_REG_SELECT:
 		ioapic->ioregsel = data;

@@ -358,7 +362,7 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len,
 	default:
 		break;
 	}
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 	return 0;
 }
 

@@ -418,24 +422,26 @@ void kvm_ioapic_destroy(struct kvm *kvm)
 int kvm_get_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
+	unsigned long flags;
 	if (!ioapic)
 		return -EINVAL;
 
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	memcpy(state, ioapic, sizeof(struct kvm_ioapic_state));
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 	return 0;
 }
 
 int kvm_set_ioapic(struct kvm *kvm, struct kvm_ioapic_state *state)
 {
 	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
+	unsigned long flags;
 	if (!ioapic)
 		return -EINVAL;
 
-	spin_lock(&ioapic->lock);
+	spin_lock_irqsave(&ioapic->lock, flags);
 	memcpy(ioapic, state, sizeof(struct kvm_ioapic_state));
 	update_handled_vectors(ioapic);
-	spin_unlock(&ioapic->lock);
+	spin_unlock_irqrestore(&ioapic->lock, flags);
 	return 0;
 }

Signature · 1 line

1.7.3.rc1.5.ge5969

--
Gleb.

Quoted 8 lines · Gleb Natapov

Quoted 7 lines · Michael S. Tsirkin

To avoid bouncing irqfd injection out to workqueue context, we must support injecting irqs from local interrupt context. Doing this seems to only require disabling irqs locally.

RFC, completely untested, x86 only.

Quoted 5 lines

Quoted 2 lines

Thoughts?

We do not want to disable irqs for a long time and some of code paths under lock involve looping over all cpus. For MSI injection path is lockless and this is the only case that matters,

MSI only appeared in rhel6, older guests still use level interrupts.

Which paths require looping over all cpus? Do PCI interrupts need this?

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

So they are already slow for other reasons.

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Gleb Natapov

Quoted 7 lines · Michael S. Tsirkin

To avoid bouncing irqfd injection out to workqueue context, we must support injecting irqs from local interrupt context. Doing this seems to only require disabling irqs locally.

RFC, completely untested, x86 only.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Thoughts?

We do not want to disable irqs for a long time and some of code paths under lock involve looping over all cpus. For MSI injection path is lockless and this is the only case that matters,

MSI only appeared in rhel6, older guests still use level interrupts.

So they are already slow for other reasons.

Quoted 3 lines

Which paths require looping over all cpus? Do PCI interrupts need this?

All interrupts need it. IOAPIC has a loop to find dst cpu for interrupt. Pic has a loop to find cpu in virtual wire mode.

--
Gleb.

Quoted 4 lines · Gleb Natapov

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

So they are already slow for other reasons.

Exactly, for example they need to exit to userspace to ack the interrupt. That's far slower than the workqueue.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 3 lines · Avi Kivity

Quoted 2 lines · Gleb Natapov

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

Quoted 5 lines

Quoted 2 lines

So they are already slow for other reasons.

Exactly, for example they need to exit to userspace to ack the interrupt. That's far slower than the workqueue.

Well, this is not exactly comparable: you might get same irq asserted multiple times and only deasserted once.

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

So they are already slow for other reasons.

Quoted 6 lines

Quoted 2 lines

Exactly, for example they need to exit to userspace to ack the interrupt. That's far slower than the workqueue.

Well, this is not exactly comparable: you might get same irq asserted multiple times and only deasserted once.

Are we talking about level interrupts? Why would you assert level triggered interrupt multiple times before deasserting it?

--
Gleb.

Quoted 11 lines · Gleb Natapov

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

So they are already slow for other reasons.

Exactly, for example they need to exit to userspace to ack the interrupt. That's far slower than the workqueue.

Quoted 5 lines

Quoted 3 lines

Well, this is not exactly comparable: you might get same irq asserted multiple times and only deasserted once.

Are we talking about level interrupts? Why would you assert level triggered interrupt multiple times before deasserting it?

User of irqfd has no way to know what current interrupt level is. So it has to keep asserting.

Quoted 15 lines · Michael S. Tsirkin

Quoted 14 lines · Gleb Natapov

Quoted 13 lines · Michael S. Tsirkin

Quoted 8 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

So they are already slow for other reasons.

Exactly, for example they need to exit to userspace to ack the interrupt. That's far slower than the workqueue.

Well, this is not exactly comparable: you might get same irq asserted multiple times and only deasserted once.

Quoted 6 lines

Quoted 2 lines

Are we talking about level interrupts? Why would you assert level triggered interrupt multiple times before deasserting it?

User of irqfd has no way to know what current interrupt level is. So it has to keep asserting.

Why can't it keep track of current level?

--
Gleb.

Quoted 19 lines · Gleb Natapov

Quoted 18 lines · Michael S. Tsirkin

Quoted 16 lines · Gleb Natapov

Quoted 4 lines · Michael S. Tsirkin

Quoted 3 lines · Avi Kivity

Quoted 2 lines · Gleb Natapov

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

So they are already slow for other reasons.

Exactly, for example they need to exit to userspace to ack the interrupt. That's far slower than the workqueue.

Well, this is not exactly comparable: you might get same irq asserted multiple times and only deasserted once.

Are we talking about level interrupts? Why would you assert level triggered interrupt multiple times before deasserting it?

Quoted 4 lines

Quoted 3 lines

User of irqfd has no way to know what current interrupt level is. So it has to keep asserting.

Why can't it keep track of current level?

This breaks the model: eventfd user is unaware of PCI, levels and such:
it just signals the event. Remember that asserts are done from e.g. vhost-net, deasserts need to be handled by qemu.

Quoted 23 lines · Michael S. Tsirkin

Quoted 22 lines · Gleb Natapov

Quoted 21 lines · Michael S. Tsirkin

Quoted 8 lines · Gleb Natapov

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

So they are already slow for other reasons.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

Exactly, for example they need to exit to userspace to ack the interrupt. That's far slower than the workqueue.

Well, this is not exactly comparable: you might get same irq asserted multiple times and only deasserted once.

Are we talking about level interrupts? Why would you assert level triggered interrupt multiple times before deasserting it?

User of irqfd has no way to know what current interrupt level is. So it has to keep asserting.

Quoted 6 lines

Quoted 1 line

Why can't it keep track of current level?

This breaks the model: eventfd user is unaware of PCI, levels and such:
it just signals the event. Remember that asserts are done from e.g. vhost-net, deasserts need to be handled by qemu.

eventfd user implements HW and it knows exactly what type of interrupt this HW generates.

--
Gleb.

Quoted 26 lines · Gleb Natapov

Quoted 25 lines · Michael S. Tsirkin

Quoted 23 lines · Gleb Natapov

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Gleb Natapov

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

So they are already slow for other reasons.

Exactly, for example they need to exit to userspace to ack the interrupt. That's far slower than the workqueue.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

Well, this is not exactly comparable: you might get same irq asserted multiple times and only deasserted once.

Are we talking about level interrupts? Why would you assert level triggered interrupt multiple times before deasserting it?

User of irqfd has no way to know what current interrupt level is. So it has to keep asserting.

Why can't it keep track of current level?

Quoted 6 lines

Quoted 4 lines

This breaks the model: eventfd user is unaware of PCI, levels and such:
it just signals the event. Remember that asserts are done from e.g. vhost-net, deasserts need to be handled by qemu.

eventfd user implements HW and it knows exactly what type of interrupt this HW generates.

We haver two users: qemu does deasserts, vhost-net does asserts.

Another application is out of process virtio (sandboxing!).

Again, pci stuff needs to stay in qemu.

Quoted 27 lines · Michael S. Tsirkin

Quoted 26 lines · Gleb Natapov

Quoted 25 lines · Michael S. Tsirkin

Quoted 23 lines · Gleb Natapov

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Gleb Natapov

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov

Quoted 2 lines

MSI only appeared in rhel6, older guests still use level interrupts.

So they are already slow for other reasons.

Exactly, for example they need to exit to userspace to ack the interrupt. That's far slower than the workqueue.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

Well, this is not exactly comparable: you might get same irq asserted multiple times and only deasserted once.

Are we talking about level interrupts? Why would you assert level triggered interrupt multiple times before deasserting it?

User of irqfd has no way to know what current interrupt level is. So it has to keep asserting.

Why can't it keep track of current level?

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

This breaks the model: eventfd user is unaware of PCI, levels and such:
it just signals the event. Remember that asserts are done from e.g. vhost-net, deasserts need to be handled by qemu.

eventfd user implements HW and it knows exactly what type of interrupt this HW generates.

We haver two users: qemu does deasserts, vhost-net does asserts.

Well this is broken. You want KVM to track level for you and this is wrong. KVM does this anyway because it can't relay on devise model to behave correctly [0], but in your case it is designed to behave incorrectly.

Interrupt type is a device property. PCI devices just happen to be level triggered according to PCI spec. What if you want to use vhost-net to implement network device which has active-low interrupt line? [1]

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Quoted 1 line

Another application is out of process virtio (sandboxing!).

It will still assert and de-assert irq at the same code, so it will be able to track irq line status.

Quoted 2 lines

Again, pci stuff needs to stay in qemu.

Nothing to do with PCI whatsoever.

[0] most qemu devices behave incorrectly and trigger level irq more then needed.

[1] this is how correct PCI device should behave but we override polarity in ACPI, but now incorrect behaviour is deeply designed into vhost-net.

--
Gleb.

Quoted 6 lines

Quoted 1 line

We haver two users: qemu does deasserts, vhost-net does asserts.

Well this is broken. You want KVM to track level for you and this is wrong. KVM does this anyway because it can't relay on devise model to behave correctly [0], but in your case it is designed to behave incorrectly.

Quoted 3 lines

Interrupt type is a device property. PCI devices just happen to be level triggered according to PCI spec. What if you want to use vhost-net to implement network device which has active-low interrupt line? [1]

The polarity would have to be reversed in gsi (irq line can be shared, all devices must be active high or low consistently).

Quoted 3 lines

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Yes, it can know about polarity even though I think it's cleaner to do this per gsi. But it can not track line status as line is shared with other devices.

Quoted 8 lines

Quoted 2 lines

Another application is out of process virtio (sandboxing!).

It will still assert and de-assert irq at the same code, so it will be able to track irq line status.

Quoted 2 lines

Again, pci stuff needs to stay in qemu.

Quoted 4 lines

Nothing to do with PCI whatsoever.

[0] most qemu devices behave incorrectly and trigger level irq more then needed.

Which devices?

pci core tracks line status and will never assert the same line multiple times.

Quoted 4 lines

[1] this is how correct PCI device should behave but we override polarity in ACPI, but now incorrect behaviour is deeply designed into vhost-net.

Not really, vhost net signals an eventfd. What happens then is up to kvm.

Quoted 6 lines · Michael S. Tsirkin

Quoted 6 lines

Quoted 1 line

We haver two users: qemu does deasserts, vhost-net does asserts.

Well this is broken. You want KVM to track level for you and this is wrong. KVM does this anyway because it can't relay on devise model to behave correctly [0], but in your case it is designed to behave incorrectly.

Quoted 7 lines

Quoted 3 lines

Interrupt type is a device property. PCI devices just happen to be level triggered according to PCI spec. What if you want to use vhost-net to implement network device which has active-low interrupt line? [1]

The polarity would have to be reversed in gsi (irq line can be shared, all devices must be active high or low consistently).

There are gsi dedicated to PCI. They can be shared only between PCI devices.

Quoted 7 lines

Quoted 3 lines

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Yes, it can know about polarity even though I think it's cleaner to do this per gsi. But it can not track line status as line is shared with other devices.

It should track only device's line status.

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Another application is out of process virtio (sandboxing!).

It will still assert and de-assert irq at the same code, so it will be able to track irq line status.

Quoted 2 lines

Again, pci stuff needs to stay in qemu.

Quoted 6 lines

Quoted 4 lines

Nothing to do with PCI whatsoever.

[0] most qemu devices behave incorrectly and trigger level irq more then needed.

Which devices?

Most of them. They just call update_irq_status() or something and re-assert interrupt regardless of what previous status was.

Quoted 2 lines

pci core tracks line status and will never assert the same line multiple times.

That's good if pci core does this, but device shouldn't even try it.

Quoted 8 lines

Quoted 4 lines

[1] this is how correct PCI device should behave but we override polarity in ACPI, but now incorrect behaviour is deeply designed into vhost-net.

Not really, vhost net signals an eventfd. What happens then is up to kvm.

That is what current broken design does and it works, but if you want to save unneeded calls into kvm fix design.

--
Gleb.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Quoted 4 lines

Quoted 3 lines

Yes, it can know about polarity even though I think it's cleaner to do this per gsi. But it can not track line status as line is shared with other devices.

It should track only device's line status.

There is no such thing as device's line status on real hardware, either. Devices do not drive INT# high: they drive it low (all the time) or do not drive it at all.

Quoted 21 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin

Quoted 6 lines

Quoted 1 line

We haver two users: qemu does deasserts, vhost-net does asserts.

Well this is broken. You want KVM to track level for you and this is wrong. KVM does this anyway because it can't relay on devise model to behave correctly [0], but in your case it is designed to behave incorrectly.

Quoted 7 lines

Quoted 3 lines

Interrupt type is a device property. PCI devices just happen to be level triggered according to PCI spec. What if you want to use vhost-net to implement network device which has active-low interrupt line? [1]

The polarity would have to be reversed in gsi (irq line can be shared, all devices must be active high or low consistently).

There are gsi dedicated to PCI. They can be shared only between PCI devices.

Quoted 4 lines

Quoted 3 lines

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Quoted 4 lines

Quoted 3 lines

Yes, it can know about polarity even though I think it's cleaner to do this per gsi. But it can not track line status as line is shared with other devices.

It should track only device's line status.

There is no such thing as device's line status on real hardware, either. Devices do not drive INT# high: they drive it low (all the time) or do not drive it at all.

Or consider express, the spec explicitly says:
"Note: Duplicate Assert_INTx/Deassert_INTx Messages have no effect, but
are not errors."

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 2 lines

Another application is out of process virtio (sandboxing!).

It will still assert and de-assert irq at the same code, so it will be able to track irq line status.

Quoted 2 lines

Again, pci stuff needs to stay in qemu.

Nothing to do with PCI whatsoever.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

[0] most qemu devices behave incorrectly and trigger level irq more then needed.

Which devices?

Most of them. They just call update_irq_status() or something and re-assert interrupt regardless of what previous status was.

At least for PCI devices, these calls do nothing if status does not change.

Quoted 4 lines

Quoted 3 lines

pci core tracks line status and will never assert the same line multiple times.

That's good if pci core does this, but device shouldn't even try it.

I disagree. We don't want to duplicate a ton of code all over the codebase.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

[1] this is how correct PCI device should behave but we override polarity in ACPI, but now incorrect behaviour is deeply designed into vhost-net.

Not really, vhost net signals an eventfd. What happens then is up to kvm.

Quoted 2 lines

That is what current broken design does and it works, but if you want to save unneeded calls into kvm fix design.

The interface seems clean enough: vhost handles virtio ring, qemu/kvm handle pci. Making vhost aware of pci breaks this, I would not call that fixing the design.

Quoted 5 lines · Michael S. Tsirkin

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Quoted 9 lines

Quoted 4 lines

Quoted 3 lines

Yes, it can know about polarity even though I think it's cleaner to do this per gsi. But it can not track line status as line is shared with other devices.

It should track only device's line status.

There is no such thing as device's line status on real hardware, either. Devices do not drive INT# high: they drive it low (all the time) or do not drive it at all.

That's just an implementation detail. Devices either assert INT# or they do not. Tying the wires together constitutes an AND gate. This gate has to be modelled somewhere, currently it's in qemu's pci emulation.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 5 lines

That's just an implementation detail. Devices either assert INT# or they do not. Tying the wires together constitutes an AND gate. This gate has to be modelled somewhere, currently it's in qemu's pci emulation.

Right. kvm in kernel has this as well, we need to keep this in kvm kernel if we want to support level with irqfd.

Where it does not belong is individual devices: these should be able to assert INTx multiple times and it should have no effect, as per spec.

Quoted 16 lines · Avi Kivity

Quoted 5 lines · Michael S. Tsirkin

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Quoted 9 lines

Quoted 4 lines

Quoted 3 lines

Yes, it can know about polarity even though I think it's cleaner to do this per gsi. But it can not track line status as line is shared with other devices.

It should track only device's line status.

There is no such thing as device's line status on real hardware, either. Devices do not drive INT# high: they drive it low (all the time) or do not drive it at all.

Quoted 4 lines

That's just an implementation detail. Devices either assert INT# or they do not. Tying the wires together constitutes an AND gate. This gate has to be modelled somewhere, currently it's in qemu's pci emulation.

Right. kvm in kernel has this as well, we need to keep this in kvm kernel if we want to support level with irqfd. Where it does not belong is individual devices: these should be able to assert INTx multiple times
and it should have no effect, as per spec.

Quoted 25 lines · Michael S. Tsirkin

Quoted 24 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin

Quoted 6 lines

Quoted 1 line

We haver two users: qemu does deasserts, vhost-net does asserts.

Well this is broken. You want KVM to track level for you and this is wrong. KVM does this anyway because it can't relay on devise model to behave correctly [0], but in your case it is designed to behave incorrectly.

Quoted 7 lines

Quoted 3 lines

Interrupt type is a device property. PCI devices just happen to be level triggered according to PCI spec. What if you want to use vhost-net to implement network device which has active-low interrupt line? [1]

The polarity would have to be reversed in gsi (irq line can be shared, all devices must be active high or low consistently).

There are gsi dedicated to PCI. They can be shared only between PCI devices.

Quoted 7 lines

Quoted 3 lines

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Yes, it can know about polarity even though I think it's cleaner to do this per gsi. But it can not track line status as line is shared with other devices.

Quoted 5 lines

Quoted 1 line

It should track only device's line status.

There is no such thing as device's line status on real hardware, either. Devices do not drive INT# high: they drive it low (all the time) or do not drive it at all.

Same thing, other naming. Device either drive it low (irq_set(1)) or not (irq_set(0)).

Quoted 18 lines

Or consider express, the spec explicitly says:
"Note: Duplicate Assert_INTx/Deassert_INTx Messages have no effect, but
are not errors."

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

Another application is out of process virtio (sandboxing!).

It will still assert and de-assert irq at the same code, so it will be able to track irq line status.

Quoted 2 lines

Again, pci stuff needs to stay in qemu.

Nothing to do with PCI whatsoever.

[0] most qemu devices behave incorrectly and trigger level irq more then needed.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Which devices?

Most of them. They just call update_irq_status() or something and re-assert interrupt regardless of what previous status was.

At least for PCI devices, these calls do nothing if status does not change.

I am not sure what code are you locking at. e1000 device emulation doesn't check previous line status before calling qemu_set_irq().

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

pci core tracks line status and will never assert the same line multiple times.

That's good if pci core does this, but device shouldn't even try it.

I disagree. We don't want to duplicate a ton of code all over the codebase.

So abstract it into a function. It shouldn't be part of PCI emulation.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

[1] this is how correct PCI device should behave but we override polarity in ACPI, but now incorrect behaviour is deeply designed into vhost-net.

Not really, vhost net signals an eventfd. What happens then is up to kvm.

Quoted 7 lines

Quoted 2 lines

That is what current broken design does and it works, but if you want to save unneeded calls into kvm fix design.

The interface seems clean enough: vhost handles virtio ring, qemu/kvm handle pci. Making vhost aware of pci breaks this, I would not call that fixing the design.

Once again. Nothing to do with PCI, everything to do with device emulation. And I propose to abstract interrupt assertion part into irqfd, not into vhost.

--
Gleb.

Quoted 28 lines · Gleb Natapov

Quoted 27 lines · Michael S. Tsirkin

Quoted 25 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin

Quoted 6 lines

Quoted 1 line

We haver two users: qemu does deasserts, vhost-net does asserts.

Well this is broken. You want KVM to track level for you and this is wrong. KVM does this anyway because it can't relay on devise model to behave correctly [0], but in your case it is designed to behave incorrectly.

Quoted 7 lines

Quoted 3 lines

Interrupt type is a device property. PCI devices just happen to be level triggered according to PCI spec. What if you want to use vhost-net to implement network device which has active-low interrupt line? [1]

The polarity would have to be reversed in gsi (irq line can be shared, all devices must be active high or low consistently).

There are gsi dedicated to PCI. They can be shared only between PCI devices.

Quoted 7 lines

Quoted 3 lines

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Yes, it can know about polarity even though I think it's cleaner to do this per gsi. But it can not track line status as line is shared with other devices.

It should track only device's line status.

Quoted 5 lines

Quoted 3 lines

There is no such thing as device's line status on real hardware, either. Devices do not drive INT# high: they drive it low (all the time) or do not drive it at all.

Same thing, other naming. Device either drive it low (irq_set(1)) or not (irq_set(0)).

Well, if I drive it low any number of times it should hae no effect.

Quoted 22 lines

Quoted 22 lines

Or consider express, the spec explicitly says:
"Note: Duplicate Assert_INTx/Deassert_INTx Messages have no effect, but
are not errors."

Quoted 16 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Another application is out of process virtio (sandboxing!).

It will still assert and de-assert irq at the same code, so it will be able to track irq line status.

Quoted 2 lines

Again, pci stuff needs to stay in qemu.

Quoted 6 lines

Quoted 4 lines

Nothing to do with PCI whatsoever.

[0] most qemu devices behave incorrectly and trigger level irq more then needed.

Which devices?

Most of them. They just call update_irq_status() or something and re-assert interrupt regardless of what previous status was.

Quoted 3 lines

Quoted 1 line

At least for PCI devices, these calls do nothing if status does not change.

I am not sure what code are you locking at. e1000 device emulation doesn't check previous line status before calling qemu_set_irq().

Right. If you dig through useless levels of indirection, you will see that each PCI device has 4 pin levels, when one of these changes this makes it up level to the parent bus, and so on.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

pci core tracks line status and will never assert the same line multiple times.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

That's good if pci core does this, but device shouldn't even try it.

I disagree. We don't want to duplicate a ton of code all over the codebase.

So abstract it into a function. It shouldn't be part of PCI emulation.

I don't know what you mean by this, send a patch and we can discuss?

Note that when I patches PCI interrupt handling for compliance I made it mimic hardware as closely as possible: devices can send any # of assert/deassert messages, bus discards duplicates.

Quoted 19 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

[1] this is how correct PCI device should behave but we override polarity in ACPI, but now incorrect behaviour is deeply designed into vhost-net.

Not really, vhost net signals an eventfd. What happens then is up to kvm.

Quoted 7 lines

Quoted 2 lines

That is what current broken design does and it works, but if you want to save unneeded calls into kvm fix design.

The interface seems clean enough: vhost handles virtio ring, qemu/kvm handle pci. Making vhost aware of pci breaks this, I would not call that fixing the design.

Once again. Nothing to do with PCI, everything to do with device emulation. And I propose to abstract interrupt assertion part into irqfd, not into vhost.

Quoted 2 lines

--
Gleb.

This could work. KVM would need to find all irqfd objects mapped to gsi and notify them on deassert. No idea how hard this is.

Quoted 32 lines · Michael S. Tsirkin

Quoted 31 lines · Gleb Natapov

Quoted 30 lines · Michael S. Tsirkin

Quoted 17 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin

Quoted 6 lines

Quoted 1 line

We haver two users: qemu does deasserts, vhost-net does asserts.

Well this is broken. You want KVM to track level for you and this is wrong. KVM does this anyway because it can't relay on devise model to behave correctly [0], but in your case it is designed to behave incorrectly.

Quoted 7 lines

Quoted 3 lines

Interrupt type is a device property. PCI devices just happen to be level triggered according to PCI spec. What if you want to use vhost-net to implement network device which has active-low interrupt line? [1]

The polarity would have to be reversed in gsi (irq line can be shared, all devices must be active high or low consistently).

There are gsi dedicated to PCI. They can be shared only between PCI devices.

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

If you want to split parts that asserts irq and de-asserts it then we should have irqfd that tracks line status and knows interrupt line polarity.

Yes, it can know about polarity even though I think it's cleaner to do this per gsi. But it can not track line status as line is shared with other devices.

It should track only device's line status.

There is no such thing as device's line status on real hardware, either. Devices do not drive INT# high: they drive it low (all the time) or do not drive it at all.

Quoted 5 lines

Quoted 2 lines

Same thing, other naming. Device either drive it low (irq_set(1)) or not (irq_set(0)).

Well, if I drive it low any number of times it should hae no effect.

There is no meaning of "driving low the line multiple times" on real HW. You either drive it low or not. We try to emulate this with individual "drive low/high" events in software.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Or consider express, the spec explicitly says:
"Note: Duplicate Assert_INTx/Deassert_INTx Messages have no effect, but

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

are not errors."

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

Another application is out of process virtio (sandboxing!).

It will still assert and de-assert irq at the same code, so it will be able to track irq line status.

Quoted 2 lines

Again, pci stuff needs to stay in qemu.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Nothing to do with PCI whatsoever.

[0] most qemu devices behave incorrectly and trigger level irq more then needed.

Which devices?

Most of them. They just call update_irq_status() or something and re-assert interrupt regardless of what previous status was.

At least for PCI devices, these calls do nothing if status does not change.

Quoted 6 lines

Quoted 2 lines

I am not sure what code are you locking at. e1000 device emulation doesn't check previous line status before calling qemu_set_irq().

Right. If you dig through useless levels of indirection, you will see that each PCI device has 4 pin levels, when one of these changes this makes it up level to the parent bus, and so on.

Yes. Qemu PCI level does it right. Ideally device would not even invoke this logic if interrupt status haven't changed.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

Quoted 4 lines

pci core tracks line status and will never assert the same line multiple times.

That's good if pci core does this, but device shouldn't even try it.

I disagree. We don't want to duplicate a ton of code all over the codebase.

Quoted 3 lines

Quoted 1 line

So abstract it into a function. It shouldn't be part of PCI emulation.

I don't know what you mean by this, send a patch and we can discuss?

I don't care enough to send patch. Just remember previous irq status and do not call qemu_set_irq() if it doesn't change. Three lines of code.

Quoted 4 lines

Note that when I patches PCI interrupt handling for compliance I made it mimic hardware as closely as possible: devices can send any # of assert/deassert messages, bus discards duplicates.

Qemu PCI code is correct as far as I can see. Not all devices are connected via PCI and there is not need to go through couple of layer of indirection to figure out that nothing should be done.

Quoted 19 lines

Quoted 19 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

[1] this is how correct PCI device should behave but we override polarity in ACPI, but now incorrect behaviour is deeply designed into vhost-net.

Not really, vhost net signals an eventfd. What happens then is up to kvm.

Quoted 7 lines

Quoted 2 lines

That is what current broken design does and it works, but if you want to save unneeded calls into kvm fix design.

The interface seems clean enough: vhost handles virtio ring, qemu/kvm handle pci. Making vhost aware of pci breaks this, I would not call that fixing the design.

Once again. Nothing to do with PCI, everything to do with device emulation. And I propose to abstract interrupt assertion part into irqfd, not into vhost.

Quoted 7 lines

Quoted 2 lines

--
Gleb.

This could work. KVM would need to find all irqfd objects mapped to gsi and notify them on deassert. No idea how hard this is.

What for? Device emulation should do de-assert.

--
Gleb.

Quoted 12 lines

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

There is no such thing as device's line status on real hardware, either. Devices do not drive INT# high: they drive it low (all the time) or do not drive it at all.

Same thing, other naming. Device either drive it low (irq_set(1)) or not (irq_set(0)).

Well, if I drive it low any number of times it should hae no effect.

There is no meaning of "driving low the line multiple times" on real HW. You either drive it low or not. We try to emulate this with individual "drive low/high" events in software.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Or consider express, the spec explicitly says:
"Note: Duplicate Assert_INTx/Deassert_INTx Messages have no effect, but

Express has assert and deassert messages. It might be easier for you to think in these terms. level 1: assert, level 0: deassert. Seems a simple model and what we do models this pretty well.

Quoted 23 lines

Quoted 23 lines

Quoted 22 lines

Quoted 12 lines

are not errors."

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 2 lines

Another application is out of process virtio (sandboxing!).

It will still assert and de-assert irq at the same code, so it will be able to track irq line status.

Quoted 2 lines

Again, pci stuff needs to stay in qemu.

Nothing to do with PCI whatsoever.

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

[0] most qemu devices behave incorrectly and trigger level irq more then needed.

Which devices?

Most of them. They just call update_irq_status() or something and re-assert interrupt regardless of what previous status was.

At least for PCI devices, these calls do nothing if status does not change.

I am not sure what code are you locking at. e1000 device emulation doesn't check previous line status before calling qemu_set_irq().

Quoted 5 lines

Quoted 3 lines

Right. If you dig through useless levels of indirection, you will see that each PCI device has 4 pin levels, when one of these changes this makes it up level to the parent bus, and so on.

Yes. Qemu PCI level does it right. Ideally device would not even invoke this logic if interrupt status haven't changed.

It needs to call *some* function to check status
and assert, right? qemu_set_irq is that function.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 6 lines

Quoted 5 lines

pci core tracks line status and will never assert the same line multiple times.

That's good if pci core does this, but device shouldn't even try it.

I disagree. We don't want to duplicate a ton of code all over the codebase.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

So abstract it into a function. It shouldn't be part of PCI emulation.

I don't know what you mean by this, send a patch and we can discuss?

I don't care enough to send patch. Just remember previous irq status and do not call qemu_set_irq() if it doesn't change. Three lines of code.

Heh, we have a ton of devices to support.

And then we need to migrate this extra status, and make sure it's in sync with PCI code. We'll end up with much more more than 3 lines all of it in a very sensitive and hard to test parts code.

Quoted 4 lines

Quoted 4 lines

Note that when I patches PCI interrupt handling for compliance I made it mimic hardware as closely as possible: devices can send any # of assert/deassert messages, bus discards duplicates.

Quoted 4 lines

Qemu PCI code is correct as far as I can see. Not all devices are connected via PCI and there is not need to go through couple of layer of indirection to figure out that nothing should be done.

If we want to remove the indirection I would be much more interested to remove it for all cases, not just when nothing should be done.

Quoted 23 lines

Quoted 23 lines

Quoted 22 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

[1] this is how correct PCI device should behave but we override polarity in ACPI, but now incorrect behaviour is deeply designed into vhost-net.

Not really, vhost net signals an eventfd. What happens then is up to kvm.

Quoted 7 lines

Quoted 2 lines

That is what current broken design does and it works, but if you want to save unneeded calls into kvm fix design.

The interface seems clean enough: vhost handles virtio ring, qemu/kvm handle pci. Making vhost aware of pci breaks this, I would not call that fixing the design.

Once again. Nothing to do with PCI, everything to do with device emulation. And I propose to abstract interrupt assertion part into irqfd, not into vhost.

--
Gleb.

Quoted 5 lines

Quoted 4 lines

This could work. KVM would need to find all irqfd objects mapped to gsi and notify them on deassert. No idea how hard this is.

What for? Device emulation should do de-assert.

Sorry, but at this point I have no idea what you call device emulation.

qemu has code to de-assert. vhost has code to assert.

I would like to optimize level interrupts and stop driving scheduler insane if at all possible.

Quoted 3 lines

Quoted 1 line

What for? Device emulation should do de-assert.

Sorry, but at this point I have no idea what you call device emulation.

The same thing everyone calls device emulation. In case of virtio-net it is in hw/virtio-net.c. If vhost-net is in use device emulation is split between userspace and kernel, but it is still just device emulation.

Quoted 1 line

qemu has code to de-assert. vhost has code to assert.

Good. So qemu will de-assert. So what do you mean by "KVM would need to find all irqfd objects mapped to gsi and notify them on deassert"

Quoted 3 lines

I would like to optimize level interrupts and stop driving scheduler insane if at all possible.

Worthy goal. Do it in irqfd. Irqfd shouldn't call kvm_set_irq() if irq level hasn't changed.

Quoted 6 lines · Gleb Natapov

Quoted 3 lines

Quoted 1 line

What for? Device emulation should do de-assert.

Sorry, but at this point I have no idea what you call device emulation.

The same thing everyone calls device emulation. In case of virtio-net it is in hw/virtio-net.c. If vhost-net is in use device emulation is split between userspace and kernel, but it is still just device emulation.

case in point, virtio net does not know about pci at all, so it can not deassert.

Quoted 5 lines

Quoted 1 line

qemu has code to de-assert. vhost has code to assert.

Good. So qemu will de-assert. So what do you mean by "KVM would need to find all irqfd objects mapped to gsi and notify them on deassert"

Quoted 5 lines

Quoted 3 lines

I would like to optimize level interrupts and stop driving scheduler insane if at all possible.

Worthy goal. Do it in irqfd. Irqfd shouldn't call kvm_set_irq() if irq level hasn't changed.

Right. Then it needs to know about deasserts. It does not get this

information, so when kvm gets deassert ioctl it should locate irqfd object and set level to 0.
See?

Quoted 12 lines · Michael S. Tsirkin

Quoted 6 lines · Gleb Natapov

Quoted 3 lines

Quoted 1 line

What for? Device emulation should do de-assert.

Sorry, but at this point I have no idea what you call device emulation.

The same thing everyone calls device emulation. In case of virtio-net it is in hw/virtio-net.c. If vhost-net is in use device emulation is split between userspace and kernel, but it is still just device emulation.

case in point, virtio net does not know about pci at all, so it can not deassert.

I don't see what PCI has to do with it. virtio-net device implementation is split between several files. virtio-net/virtio-ring/virtio-pci. All of them still implement one emulated device. Whoever implemented virtio-net initially decided to put irq ack register into PCI config space, so code that does de-assert is in virtio-pci,
but it is still part of emulated
device.

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

qemu has code to de-assert. vhost has code to assert.

Good. So qemu will de-assert. So what do you mean by "KVM would need to find all irqfd objects mapped to gsi and notify them on deassert"

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

I would like to optimize level interrupts and stop driving scheduler insane if at all possible.

Worthy goal. Do it in irqfd. Irqfd shouldn't call kvm_set_irq() if irq level hasn't changed.

Right. Then it needs to know about deasserts. It does not get this

Dessert should be done by qemu writing 0 into irqfd.

Assertion and
de-assertion should be done through irqfd.

Quoted 3 lines

information, so when kvm gets deassert ioctl it should locate irqfd object and set level to 0.
See?

No.

--
Gleb.

Quoted 29 lines · Gleb Natapov

Quoted 12 lines · Michael S. Tsirkin

Quoted 6 lines · Gleb Natapov

Quoted 3 lines

Quoted 1 line

What for? Device emulation should do de-assert.

Sorry, but at this point I have no idea what you call device emulation.

The same thing everyone calls device emulation. In case of virtio-net it is in hw/virtio-net.c. If vhost-net is in use device emulation is split between userspace and kernel, but it is still just device emulation.

case in point, virtio net does not know about pci at all, so it can not deassert.

I don't see what PCI has to do with it. virtio-net device implementation is split between several files. virtio-net/virtio-ring/virtio-pci. All of them still implement one emulated device. Whoever implemented virtio-net initially decided to put irq ack register into PCI config space, so code that does de-assert is in virtio-pci,
but it is still part of emulated
device.

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

qemu has code to de-assert. vhost has code to assert.

Good. So qemu will de-assert. So what do you mean by "KVM would need to find all irqfd objects mapped to gsi and notify them on deassert"

Quoted 3 lines

I would like to optimize level interrupts and stop driving scheduler insane if at all possible.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

Worthy goal. Do it in irqfd. Irqfd shouldn't call kvm_set_irq() if irq level hasn't changed.

Right. Then it needs to know about deasserts. It does not get this

Dessert should be done by qemu writing 0 into irqfd.

writing 0 to eventfd does nothing. The way to deassert irq

is currently through ioctl and it seems entirely sensible to me to keep it that way.

Also - which irqfd :)
We need multiple irqfds to map to a single gsi, with kvm doing OR on them.

Quoted 10 lines

Assertion and
de-assertion should be done through irqfd.

Quoted 3 lines

information, so when kvm gets deassert ioctl it should locate irqfd object and set level to 0.
See?

No.

--
Gleb.

Quoted 1 line

writing 0 to eventfd does nothing. The way to deassert irq

That is implementation detail of current irqfd. It was designed for MSI not level triggered interrupts.

Quoted 3 lines · Gleb Natapov

Quoted 1 line

writing 0 to eventfd does nothing. The way to deassert irq

That is implementation detail of current irqfd. It was designed for MSI not level triggered interrupts.

Maybe we should add a check that gsi is mapped to MSI (or unmapped) then? Level which switches to 1 and back to 0 immediately will be racy anyway
...

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines · Gleb Natapov

Quoted 1 line

writing 0 to eventfd does nothing. The way to deassert irq

That is implementation detail of current irqfd. It was designed for MSI not level triggered interrupts.

Quoted 4 lines

Maybe we should add a check that gsi is mapped to MSI (or unmapped) then? Level which switches to 1 and back to 0 immediately will be racy anyway
...

Add a check where?

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 11 lines · Avi Kivity

Quoted 9 lines · Michael S. Tsirkin

Quoted 3 lines · Gleb Natapov

Quoted 1 line

writing 0 to eventfd does nothing. The way to deassert irq

That is implementation detail of current irqfd. It was designed for MSI not level triggered interrupts.

Maybe we should add a check that gsi is mapped to MSI (or unmapped) then? Level which switches to 1 and back to 0 immediately will be racy anyway
...

Quoted 1 line

Add a check where?

I would make sure that if you bind irqfd to a non-MSI GSI, signalling it has no effect.

Quoted 11 lines · Avi Kivity

Quoted 9 lines · Michael S. Tsirkin

Quoted 3 lines · Gleb Natapov

Quoted 1 line

writing 0 to eventfd does nothing. The way to deassert irq

That is implementation detail of current irqfd. It was designed for MSI not level triggered interrupts.

Maybe we should add a check that gsi is mapped to MSI (or unmapped) then? Level which switches to 1 and back to 0 immediately will be racy anyway
...

Quoted 1 line

Add a check where?

I would make sure that if you bind irqfd to a non-MSI GSI, signalling it has no effect.

Quoted 14 lines · Michael S. Tsirkin

Quoted 12 lines · Avi Kivity

Quoted 9 lines · Michael S. Tsirkin

Quoted 3 lines · Gleb Natapov

Quoted 1 line

writing 0 to eventfd does nothing. The way to deassert irq

That is implementation detail of current irqfd. It was designed for MSI not level triggered interrupts.

Maybe we should add a check that gsi is mapped to MSI (or unmapped) then? Level which switches to 1 and back to 0 immediately will be racy anyway
...

Add a check where?

Quoted 3 lines

I would make sure that if you bind irqfd to a non-MSI GSI, signalling it has no effect.

Why would you do that? I am not against checking per se, but why a user of the API can't check for that?

--
Gleb.

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Avi Kivity

Quoted 9 lines · Michael S. Tsirkin

Quoted 3 lines · Gleb Natapov

Quoted 1 line

writing 0 to eventfd does nothing. The way to deassert irq

That is implementation detail of current irqfd. It was designed for MSI not level triggered interrupts.

Maybe we should add a check that gsi is mapped to MSI (or unmapped) then? Level which switches to 1 and back to 0 immediately will be racy anyway
...

Quoted 5 lines

Quoted 1 line

Add a check where?

I would make sure that if you bind irqfd to a non-MSI GSI, signalling it has no effect.

Certainly. We should also update the documentation, when we have irqfd documentation.

Signature · 1 line

error compiling committee.c: too many arguments to function