kvm_set_irq: report coalesced for clear

RFC PATCH In review Michael S. Tsirkin 3 participants 16 messages Jul 18, 2012 – Jul 19, 2012

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Michael S. Tsirkin

author

2012-07-18 22:11 UTC #0 lore raw

RFC PATCH 1/1 kvm_set_irq: report coalesced for clear

This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.

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

---

This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do

	if (kvm_set_irq(...., 0))
		eventfd_signal()

without need for further tricks.

 arch/x86/include/asm/kvm_host.h |  9 ++++-----
 arch/x86/kvm/i8259.c            | 11 +++++++----
 virt/kvm/ioapic.c               | 17 ++++++++++-------
 3 files changed, 21 insertions(+), 16 deletions(-)

arch/x86/include/asm/kvm_host.h +4 -5

index fe6e9f1..6f5ed0a 100644
@@ -802,16 +802,15 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
 bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
 
-static inline int kvm_irq_line_state(unsigned long *irq_state,
+/* Tweak line state. Return true if state was changed. */
+static inline bool kvm_irq_line_state(unsigned long *irq_state,
 				     int irq_source_id, int level)
 {
 	/* Logical OR for level trig interrupt */
 	if (level)
-		__set_bit(irq_source_id, irq_state);
+		return !__test_and_set_bit(irq_source_id, irq_state);
 	else
-		__clear_bit(irq_source_id, irq_state);
-
-	return !!(*irq_state);
+		return __test_and_clear_bit(irq_source_id, irq_state);
 }
 
 int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);

arch/x86/kvm/i8259.c +7 -4

index 95b504b..44e3635 100644
@@ -190,12 +190,15 @@ void kvm_pic_update_irq(struct kvm_pic *s)
 
 int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
 {
-	int ret = -1;
-	int level;
+	int ret, level;
 
 	pic_lock(s);
-	if (irq >= 0 && irq < PIC_NUM_PINS) {
-		level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
+	if (irq < 0 || irq >= PIC_NUM_PINS) {
+		ret = -1;
+	} else if (!kvm_irq_line_state(&s->irq_states[irq], src_id, l)) {
+		ret = 0;
+	} else {
+		level = !!s->irq_states[irq];
 		ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
 		pic_update_irq(s);
 		trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,

virt/kvm/ioapic.c +10 -7

index 268b332..edc9445 100644
@@ -196,18 +196,21 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
 	u32 old_irr;
 	u32 mask = 1 << irq;
 	union kvm_ioapic_redirect_entry entry;
-	int ret = 1;
-	int level;
+	int ret, level;
 
 	spin_lock(&ioapic->lock);
 	old_irr = ioapic->irr;
-	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
+	if (irq < 0 || irq >= IOAPIC_NUM_PINS) {
+		ret = -1;
+	} else if (!kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l)) {
+		ret = 0;
+	} else {
 		entry = ioapic->redirtbl[irq];
-		level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
-		level ^= entry.fields.polarity;
-		if (!level)
+		level = (!!ioapic->irq_states[irq]) ^ entry.fields.polarity;
+		if (!level) {
 			ioapic->irr &= ~mask;
-		else {
+			ret = 1;
+		} else {
 			int edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
 			ioapic->irr |= mask;
 			if ((edge && old_irr != ioapic->irr) ||

Signature · 1 line

MST

Alex Williamson

2012-07-18 22:40 UTC #1 lore raw

Quoted 50 lines · Michael S. Tsirkin

This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.

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

---

This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do

	if (kvm_set_irq(...., 0))
		eventfd_signal()

without need for further tricks.

 arch/x86/include/asm/kvm_host.h |  9 ++++-----
 arch/x86/kvm/i8259.c            | 11 +++++++----
 virt/kvm/ioapic.c               | 17 ++++++++++-------
 3 files changed, 21 insertions(+), 16 deletions(-)

arch/x86/include/asm/kvm_host.h +4 -5

index fe6e9f1..6f5ed0a 100644
@@ -802,16 +802,15 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
 bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
 
-static inline int kvm_irq_line_state(unsigned long *irq_state,
+/* Tweak line state. Return true if state was changed. */
+static inline bool kvm_irq_line_state(unsigned long *irq_state,
 				     int irq_source_id, int level)
 {
 	/* Logical OR for level trig interrupt */
 	if (level)
-		__set_bit(irq_source_id, irq_state);
+		return !__test_and_set_bit(irq_source_id, irq_state);
 	else
-		__clear_bit(irq_source_id, irq_state);
-
-	return !!(*irq_state);
+		return __test_and_clear_bit(irq_source_id, irq_state);
 }
 
 int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);

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

index 95b504b..44e3635 100644
@@ -190,12 +190,15 @@ void kvm_pic_update_irq(struct kvm_pic *s)

arch/x86/kvm/i8259.c void kvm_pic_update_irq(struct kvm_pic *s) · line 196

Quoted 10 lines

arch/x86/kvm/i8259.c +2 -4

 int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
 {
-	int ret = -1;
-	int level;
+	int ret, level;
 
 	pic_lock(s);
-	if (irq >= 0 && irq < PIC_NUM_PINS) {
-		level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
+	if (irq < 0 || irq >= PIC_NUM_PINS) {

Why test this under lock? Return error before lock, then it becomes

int ret = 0;

if (irq < 0 || irq >= PIC_NUM_PINS)

	return -1;

pic_lock(s);

if (kvm_irq_line_state(&s->irq_states[irq], irq_source_id, level) {
	level = !!s->irq_states[irq];
	...
}

pic_unlock(s);

return ret;

Quoted 14 lines

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

+		ret = -1;
+	} else if (!kvm_irq_line_state(&s->irq_states[irq], src_id, l)) {
+		ret = 0;
+	} else {
+		level = !!s->irq_states[irq];
 		ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
 		pic_update_irq(s);
 		trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,

virt/kvm/ioapic.c +0 -0

index 268b332..edc9445 100644
@@ -196,18 +196,21 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
 	u32 old_irr;

virt/kvm/ioapic.c int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l) · line 203

Quoted 10 lines

virt/kvm/ioapic.c +2 -3

 	u32 mask = 1 << irq;
 	union kvm_ioapic_redirect_entry entry;
-	int ret = 1;
-	int level;
+	int ret, level;
 
 	spin_lock(&ioapic->lock);
 	old_irr = ioapic->irr;
-	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
+	if (irq < 0 || irq >= IOAPIC_NUM_PINS) {

Same here

Quoted 17 lines

virt/kvm/ioapic.c +8 -4

+		ret = -1;
+	} else if (!kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l)) {
+		ret = 0;
+	} else {
 		entry = ioapic->redirtbl[irq];
-		level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
-		level ^= entry.fields.polarity;
-		if (!level)
+		level = (!!ioapic->irq_states[irq]) ^ entry.fields.polarity;
+		if (!level) {
 			ioapic->irr &= ~mask;
-		else {
+			ret = 1;
+		} else {
 			int edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
 			ioapic->irr |= mask;
 			if ((edge && old_irr != ioapic->irr) ||

Michael S. Tsirkin

author

2012-07-18 22:50 UTC #2 lore raw

Quoted 117 lines · Alex Williamson

Quoted 50 lines · Michael S. Tsirkin

This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.

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

---

This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do

	if (kvm_set_irq(...., 0))
		eventfd_signal()

without need for further tricks.

 arch/x86/include/asm/kvm_host.h |  9 ++++-----
 arch/x86/kvm/i8259.c            | 11 +++++++----
 virt/kvm/ioapic.c               | 17 ++++++++++-------
 3 files changed, 21 insertions(+), 16 deletions(-)

arch/x86/include/asm/kvm_host.h +4 -5

index fe6e9f1..6f5ed0a 100644
@@ -802,16 +802,15 @@ int kvm_read_guest_page_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 void kvm_propagate_fault(struct kvm_vcpu *vcpu, struct x86_exception *fault);
 bool kvm_require_cpl(struct kvm_vcpu *vcpu, int required_cpl);
 
-static inline int kvm_irq_line_state(unsigned long *irq_state,
+/* Tweak line state. Return true if state was changed. */
+static inline bool kvm_irq_line_state(unsigned long *irq_state,
 				     int irq_source_id, int level)
 {
 	/* Logical OR for level trig interrupt */
 	if (level)
-		__set_bit(irq_source_id, irq_state);
+		return !__test_and_set_bit(irq_source_id, irq_state);
 	else
-		__clear_bit(irq_source_id, irq_state);
-
-	return !!(*irq_state);
+		return __test_and_clear_bit(irq_source_id, irq_state);
 }
 
 int kvm_pic_set_irq(struct kvm_pic *pic, int irq, int irq_source_id, int level);

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

index 95b504b..44e3635 100644
@@ -190,12 +190,15 @@ void kvm_pic_update_irq(struct kvm_pic *s)

arch/x86/kvm/i8259.c void kvm_pic_update_irq(struct kvm_pic *s) · line 196

Quoted 10 lines

arch/x86/kvm/i8259.c +2 -4

 int kvm_pic_set_irq(struct kvm_pic *s, int irq, int src_id, int l)
 {
-	int ret = -1;
-	int level;
+	int ret, level;
 
 	pic_lock(s);
-	if (irq >= 0 && irq < PIC_NUM_PINS) {
-		level = kvm_irq_line_state(&s->irq_states[irq], src_id, l);
+	if (irq < 0 || irq >= PIC_NUM_PINS) {

Why test this under lock? Return error before lock, then it becomes

int ret = 0;

if (irq < 0 || irq >= PIC_NUM_PINS)

	return -1;

pic_lock(s);

if (kvm_irq_line_state(&s->irq_states[irq], irq_source_id, level) {
	level = !!s->irq_states[irq];
	...
}

pic_unlock(s);

return ret;

Quoted 14 lines

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

+		ret = -1;
+	} else if (!kvm_irq_line_state(&s->irq_states[irq], src_id, l)) {
+		ret = 0;
+	} else {
+		level = !!s->irq_states[irq];
 		ret = pic_set_irq1(&s->pics[irq >> 3], irq & 7, level);
 		pic_update_irq(s);
 		trace_kvm_pic_set_irq(irq >> 3, irq & 7, s->pics[irq >> 3].elcr,

virt/kvm/ioapic.c +0 -0

index 268b332..edc9445 100644
@@ -196,18 +196,21 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l)
 	u32 old_irr;

virt/kvm/ioapic.c int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l) · line 203

Quoted 10 lines

virt/kvm/ioapic.c +2 -3

 	u32 mask = 1 << irq;
 	union kvm_ioapic_redirect_entry entry;
-	int ret = 1;
-	int level;
+	int ret, level;
 
 	spin_lock(&ioapic->lock);
 	old_irr = ioapic->irr;
-	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
+	if (irq < 0 || irq >= IOAPIC_NUM_PINS) {

Same here

Quoted 9 lines

virt/kvm/ioapic.c +5 -3

+		ret = -1;
+	} else if (!kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l)) {
+		ret = 0;
+	} else {
 		entry = ioapic->redirtbl[irq];
-		level = kvm_irq_line_state(&ioapic->irq_states[irq], src_id, l);
-		level ^= entry.fields.polarity;
-		if (!level)
+		level = (!!ioapic->irq_states[irq]) ^ entry.fields.polarity;

Quoted 9 lines

virt/kvm/ioapic.c int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int src_id, int l) · line 216
Quoted 8 lines
virt/kvm/ioapic.c +3 -1
+		if (!level) {
 			ioapic->irr &= ~mask;
-		else {
+			ret = 1;
+		} else {
 			int edge = (entry.fields.trig_mode == IOAPIC_EDGE_TRIG);
 			ioapic->irr |= mask;
 			if ((edge && old_irr != ioapic->irr) ||

Sure, go ahead. I just sent this to show how to address the locking with EOI patches, don't intent to pursue it myself.

Michael S. Tsirkin

author

2012-07-19 09:16 UTC #4 lore raw

Quoted 7 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---

Quoted 7 lines

Quoted 6 lines
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Why caller can't track line state?

Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...

Signature · 1 line

MST

Gleb Natapov

2012-07-19 09:21 UTC #5 lore raw

Quoted 14 lines · Michael S. Tsirkin

Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()

Quoted 5 lines

Quoted 1 line
Why caller can't track line state?
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...

We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?

--
Gleb.

Michael S. Tsirkin

author

2012-07-19 09:32 UTC #6 lore raw

Quoted 17 lines · Gleb Natapov

Quoted 16 lines · Michael S. Tsirkin
Quoted 14 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Why caller can't track line state?

Quoted 7 lines

Quoted 3 lines
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...
We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?

Hmm it's an interesting point. Looks like irqfds for level lose state across migration. Of course Alex wants to use them for assignment which currently disables migration, but we are talking about a generic API, so it's a problem that there's no way to retrieve the state.

Also migration is only one example. Duplicated state is generally nasty. We would need extra locking too which is not nice.

Quoted 2 lines

--
Gleb.

Gleb Natapov

2012-07-19 09:41 UTC #7 lore raw

Quoted 26 lines · Michael S. Tsirkin

Quoted 17 lines · Gleb Natapov
Quoted 16 lines · Michael S. Tsirkin
Quoted 14 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Why caller can't track line state?
Quoted 7 lines
Quoted 3 lines
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...
We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?

Quoted 5 lines

Hmm it's an interesting point. Looks like irqfds for level lose state across migration. Of course Alex wants to use them for assignment which currently disables migration, but we are talking about a generic API, so it's a problem that there's no way to retrieve the state.

There is no any problem. Source knows what the line status is. Furthermore this is a (benign) bug if device calls irq_set with the same level since it results in needless system calls. Qemu guilty of it and _that_ should be fixed.

Quoted 4 lines

Also migration is only one example. Duplicated state is generally nasty. We would need extra locking too which is not nice.

I don't know what extra locking you are talking about, but calling kvm_set_irq() repeatedly with the same level will do a lot of unnecessary locking in ioapic.

--
Gleb.

Michael S. Tsirkin

author

2012-07-19 10:26 UTC #8 lore raw

Quoted 27 lines · Gleb Natapov

Quoted 26 lines · Michael S. Tsirkin
Quoted 24 lines · Gleb Natapov
Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Quoted 5 lines
Quoted 1 line
Why caller can't track line state?
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...
We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?

Quoted 6 lines

Quoted 5 lines
Hmm it's an interesting point. Looks like irqfds for level lose state across migration. Of course Alex wants to use them for assignment which currently disables migration, but we are talking about a generic API, so it's a problem that there's no way to retrieve the state.
There is no any problem. Source knows what the line status is.

With EOIFD and level IRQFD, it does not.

Quoted 3 lines

Furthermore this is a (benign) bug if device calls irq_set with the same level since it results in needless system calls. Qemu guilty of it and _that_ should be fixed.

Fine but we are arguably returning a wrong result in that case:
set_irq twice to 0 return 1 each time. I would expect 0 the second time.

Quoted 7 lines

Quoted 4 lines
Also migration is only one example. Duplicated state is generally nasty. We would need extra locking too which is not nice.
I don't know what extra locking you are talking about, but calling kvm_set_irq() repeatedly with the same level will do a lot of unnecessary locking in ioapic.

I am talking about Alex's EOIFD. This is what this patch is trying to help.

Quoted 2 lines

--
Gleb.

Gleb Natapov

2012-07-19 10:54 UTC #9 lore raw

Quoted 28 lines · Michael S. Tsirkin

Quoted 27 lines · Gleb Natapov
Quoted 26 lines · Michael S. Tsirkin
Quoted 24 lines · Gleb Natapov
Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Quoted 5 lines
Quoted 1 line
Why caller can't track line state?
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...
We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?

Quoted 9 lines

Quoted 6 lines
Quoted 5 lines
Hmm it's an interesting point. Looks like irqfds for level lose state across migration. Of course Alex wants to use them for assignment which currently disables migration, but we are talking about a generic API, so it's a problem that there's no way to retrieve the state.
There is no any problem. Source knows what the line status is.
With EOIFD and level IRQFD, it does not.

So this is again eventfd and level interrupts incompatibility problem?

Quoted 7 lines

Quoted 3 lines
Furthermore this is a (benign) bug if device calls irq_set with the same level since it results in needless system calls. Qemu guilty of it and _that_ should be fixed.
Fine but we are arguably returning a wrong result in that case:
set_irq twice to 0 return 1 each time. I would expect 0 the second time.

It returns 0 if interrupt was coalesced. It was not.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Also migration is only one example. Duplicated state is generally nasty. We would need extra locking too which is not nice.

Quoted 7 lines

Quoted 3 lines
I don't know what extra locking you are talking about, but calling kvm_set_irq() repeatedly with the same level will do a lot of unnecessary locking in ioapic.
I am talking about Alex's EOIFD. This is what this patch is trying to help.

Can you point me to exact problem in Alex's patch?

--
Gleb.

Michael S. Tsirkin

author

2012-07-19 11:11 UTC #10 lore raw

Quoted 34 lines · Gleb Natapov

Quoted 33 lines · Michael S. Tsirkin
Quoted 32 lines · Gleb Natapov
Quoted 31 lines · Michael S. Tsirkin
Quoted 17 lines · Gleb Natapov
Quoted 16 lines · Michael S. Tsirkin
Quoted 14 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Why caller can't track line state?
Quoted 7 lines
Quoted 3 lines
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...
We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?
Hmm it's an interesting point. Looks like irqfds for level lose state across migration. Of course Alex wants to use them for assignment which currently disables migration, but we are talking about a generic API, so it's a problem that there's no way to retrieve the state.

Quoted 5 lines

Quoted 4 lines
Quoted 1 line
There is no any problem. Source knows what the line status is.
With EOIFD and level IRQFD, it does not.
So this is again eventfd and level interrupts incompatibility problem?

At some level, yes.

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
Furthermore this is a (benign) bug if device calls irq_set with the same level since it results in needless system calls. Qemu guilty of it and _that_ should be fixed.
Fine but we are arguably returning a wrong result in that case:
set_irq twice to 0 return 1 each time. I would expect 0 the second time.
It returns 0 if interrupt was coalesced. It was not.

Not really, if you call it with level 0 you always get 1 back. Look at kvm_ioapic_set_irq, see what happens if level is 0. It looks like a bug though a harmless one.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Also migration is only one example. Duplicated state is generally nasty. We would need extra locking too which is not nice.

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
I don't know what extra locking you are talking about, but calling kvm_set_irq() repeatedly with the same level will do a lot of unnecessary locking in ioapic.
I am talking about Alex's EOIFD. This is what this patch is trying to help.
Can you point me to exact problem in Alex's patch?

It's very simple. Alex adds an interface to clear the level automatically from guest on EOI. So the caller has no way to know the current state for a given source ID and can not restore it after migration.

Quoted 2 lines

--
Gleb.

Michael S. Tsirkin

author

2012-07-19 11:17 UTC #11 lore raw

Quoted 68 lines · Michael S. Tsirkin

Quoted 34 lines · Gleb Natapov
Quoted 33 lines · Michael S. Tsirkin
Quoted 32 lines · Gleb Natapov
Quoted 31 lines · Michael S. Tsirkin
Quoted 17 lines · Gleb Natapov
Quoted 16 lines · Michael S. Tsirkin
Quoted 14 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Why caller can't track line state?
Quoted 7 lines
Quoted 3 lines
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...
We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?
Hmm it's an interesting point. Looks like irqfds for level lose state across migration. Of course Alex wants to use them for assignment which currently disables migration, but we are talking about a generic API, so it's a problem that there's no way to retrieve the state.
Quoted 5 lines
Quoted 4 lines
Quoted 1 line
There is no any problem. Source knows what the line status is.
With EOIFD and level IRQFD, it does not.
So this is again eventfd and level interrupts incompatibility problem?
At some level, yes.
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
Furthermore this is a (benign) bug if device calls irq_set with the same level since it results in needless system calls. Qemu guilty of it and _that_ should be fixed.
Fine but we are arguably returning a wrong result in that case:
set_irq twice to 0 return 1 each time. I would expect 0 the second time.
It returns 0 if interrupt was coalesced. It was not.
Not really, if you call it with level 0 you always get 1 back. Look at kvm_ioapic_set_irq, see what happens if level is 0. It looks like a bug though a harmless one.
Quoted 12 lines
Quoted 12 lines
Quoted 8 lines
Quoted 5 lines
Also migration is only one example. Duplicated state is generally nasty. We would need extra locking too which is not nice.
I don't know what extra locking you are talking about, but calling kvm_set_irq() repeatedly with the same level will do a lot of unnecessary locking in ioapic.
I am talking about Alex's EOIFD. This is what this patch is trying to help.

Quoted 6 lines

Quoted 1 line
Can you point me to exact problem in Alex's patch?
It's very simple. Alex adds an interface to clear the level automatically from guest on EOI. So the caller has no way to know the current state for a given source ID and can not restore it after migration.

That's one problem :). The second problem is it is adding more spinlocks around kvm_set_irq, so if we want to avoid vcpu scans under spinlock, we'll have more work to do. I'm not sure how serious this second problem is, Avi nacked a patch because of a similar issue in the past but that had to deal with MSI.

Quoted 2 lines

Quoted 2 lines
--
Gleb.

Gleb Natapov

2012-07-19 11:25 UTC #12 lore raw

In reply to Michael S. Tsirkin

Quoted 35 lines · Michael S. Tsirkin

Quoted 34 lines · Gleb Natapov
Quoted 33 lines · Michael S. Tsirkin
Quoted 32 lines · Gleb Natapov
Quoted 31 lines · Michael S. Tsirkin
Quoted 17 lines · Gleb Natapov
Quoted 16 lines · Michael S. Tsirkin
Quoted 14 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Why caller can't track line state?
Quoted 7 lines
Quoted 3 lines
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...
We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?
Hmm it's an interesting point. Looks like irqfds for level lose state across migration. Of course Alex wants to use them for assignment which currently disables migration, but we are talking about a generic API, so it's a problem that there's no way to retrieve the state.

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
Quoted 1 line
There is no any problem. Source knows what the line status is.
With EOIFD and level IRQFD, it does not.
So this is again eventfd and level interrupts incompatibility problem?
At some level, yes.

So may be we shouldn't do that especially since you claim migration will not work.

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Furthermore this is a (benign) bug if device calls irq_set with the same level since it results in needless system calls. Qemu guilty of it and _that_ should be fixed.

Quoted 9 lines

Quoted 4 lines
Quoted 3 lines
Fine but we are arguably returning a wrong result in that case:
set_irq twice to 0 return 1 each time. I would expect 0 the second time.
It returns 0 if interrupt was coalesced. It was not.
Not really, if you call it with level 0 you always get 1 back. Look at kvm_ioapic_set_irq, see what happens if level is 0. It looks like a bug though a harmless one.

May be. What kvm_set_irq() return in case of level=0 was never important.

Quoted 12 lines

Quoted 12 lines
Quoted 12 lines
Quoted 8 lines
Quoted 5 lines
Also migration is only one example. Duplicated state is generally nasty. We would need extra locking too which is not nice.
I don't know what extra locking you are talking about, but calling kvm_set_irq() repeatedly with the same level will do a lot of unnecessary locking in ioapic.
I am talking about Alex's EOIFD. This is what this patch is trying to help.

Quoted 7 lines

Quoted 1 line
Can you point me to exact problem in Alex's patch?
It's very simple. Alex adds an interface to clear the level automatically from guest on EOI. So the caller has no way to know the current state for a given source ID and can not restore it after migration.

Yes, but caller (read device emulation) knows what real state is. The fact that EOI was called does not mean the line is at 0. Device should reevaluate its state and re-trigger the line again if needed.

--
Gleb.

Michael S. Tsirkin

author

2012-07-19 11:57 UTC #13 lore raw

Quoted 53 lines · Gleb Natapov

Quoted 35 lines · Michael S. Tsirkin
Quoted 34 lines · Gleb Natapov
Quoted 33 lines · Michael S. Tsirkin
Quoted 32 lines · Gleb Natapov
Quoted 31 lines · Michael S. Tsirkin
Quoted 17 lines · Gleb Natapov
Quoted 16 lines · Michael S. Tsirkin
Quoted 14 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Why caller can't track line state?
Quoted 7 lines
Quoted 3 lines
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...
We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?
Hmm it's an interesting point. Looks like irqfds for level lose state across migration. Of course Alex wants to use them for assignment which currently disables migration, but we are talking about a generic API, so it's a problem that there's no way to retrieve the state.
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
Quoted 1 line
There is no any problem. Source knows what the line status is.
With EOIFD and level IRQFD, it does not.
So this is again eventfd and level interrupts incompatibility problem?
At some level, yes.
So may be we shouldn't do that especially since you claim migration will not work.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Furthermore this is a (benign) bug if device calls irq_set with the same level since it results in needless system calls. Qemu guilty of it and _that_ should be fixed.
Fine but we are arguably returning a wrong result in that case:
set_irq twice to 0 return 1 each time. I would expect 0 the second time.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
It returns 0 if interrupt was coalesced. It was not.
Not really, if you call it with level 0 you always get 1 back. Look at kvm_ioapic_set_irq, see what happens if level is 0. It looks like a bug though a harmless one.
May be. What kvm_set_irq() return in case of level=0 was never important.

Absolutely. Now it'll be helpful to fix this for the EOI thing so that we can avoid signalling userspace in that case.

Quoted 14 lines

Quoted 14 lines
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Also migration is only one example. Duplicated state is generally nasty. We would need extra locking too which is not nice.
Quoted 7 lines
Quoted 3 lines
I don't know what extra locking you are talking about, but calling kvm_set_irq() repeatedly with the same level will do a lot of unnecessary locking in ioapic.
I am talking about Alex's EOIFD. This is what this patch is trying to help.
Can you point me to exact problem in Alex's patch?

Quoted 8 lines

Quoted 5 lines
It's very simple. Alex adds an interface to clear the level automatically from guest on EOI. So the caller has no way to know the current state for a given source ID and can not restore it after migration.
Yes, but caller (read device emulation) knows what real state is. The fact that EOI was called does not mean the line is at 0. Device should reevaluate its state and re-trigger the line again if needed.

Sounds reasonable, but let's document this property of level IRQFD.

Quoted 2 lines

--
Gleb.

Alex Williamson

2012-07-19 16:38 UTC #14 lore raw

Quoted 85 lines · Michael S. Tsirkin

Quoted 53 lines · Gleb Natapov
Quoted 35 lines · Michael S. Tsirkin
Quoted 34 lines · Gleb Natapov
Quoted 33 lines · Michael S. Tsirkin
Quoted 32 lines · Gleb Natapov
Quoted 31 lines · Michael S. Tsirkin
Quoted 17 lines · Gleb Natapov
Quoted 16 lines · Michael S. Tsirkin
Quoted 14 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
This creates a way to detect when kvm_set_irq(...,0) was run twice with the same source id by returning 0 in this case.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
This is on top of my bugfix patch. Uncompiled and untested. Alex, I think something like this patch will make it possible for you to simply do
	if (kvm_set_irq(...., 0))
		eventfd_signal()
Why caller can't track line state?
Quoted 7 lines
Quoted 3 lines
Why duplicate information? As we are finding it's not trivial to keep the two in sync. Think about migration etc ...
We do not migrate irq_states. The caller already have to have enough information to recreate its state and it should migrate the info, so why should we go all the way down the call chain to find something that is already known?
Hmm it's an interesting point. Looks like irqfds for level lose state across migration. Of course Alex wants to use them for assignment which currently disables migration, but we are talking about a generic API, so it's a problem that there's no way to retrieve the state.
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
Quoted 1 line
There is no any problem. Source knows what the line status is.
With EOIFD and level IRQFD, it does not.
So this is again eventfd and level interrupts incompatibility problem?
At some level, yes.
So may be we shouldn't do that especially since you claim migration will not work.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Furthermore this is a (benign) bug if device calls irq_set with the same level since it results in needless system calls. Qemu guilty of it and _that_ should be fixed.
Fine but we are arguably returning a wrong result in that case:
set_irq twice to 0 return 1 each time. I would expect 0 the second time.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
It returns 0 if interrupt was coalesced. It was not.
Not really, if you call it with level 0 you always get 1 back. Look at kvm_ioapic_set_irq, see what happens if level is 0. It looks like a bug though a harmless one.
May be. What kvm_set_irq() return in case of level=0 was never important.
Absolutely. Now it'll be helpful to fix this for the EOI thing so that we can avoid signalling userspace in that case.
Quoted 19 lines
Quoted 19 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Also migration is only one example. Duplicated state is generally nasty. We would need extra locking too which is not nice.
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
I don't know what extra locking you are talking about, but calling kvm_set_irq() repeatedly with the same level will do a lot of unnecessary locking in ioapic.
I am talking about Alex's EOIFD. This is what this patch is trying to help.
Can you point me to exact problem in Alex's patch?
It's very simple. Alex adds an interface to clear the level automatically from guest on EOI. So the caller has no way to know the current state for a given source ID and can not restore it after migration.

Quoted 5 lines

Quoted 3 lines
Yes, but caller (read device emulation) knows what real state is. The fact that EOI was called does not mean the line is at 0. Device should reevaluate its state and re-trigger the line again if needed.
Sounds reasonable, but let's document this property of level IRQFD.

Yes, the problem isn't the state. The original patch works just fine to mask and assert the interrupt every time the device signals and de-assert and unmask on every EOI. KVM doesn't need to track this for migration (not that we support migration, of course), we can always just send an unmask to the device to retrigger an interrupt if needed.

The thing Michael is trying to avoid is spurious assertions and de-assertions by tracking the state machine. Spurious assertions are not really a problem, at least for vfio where the interrupt is masked until kvm/qemu tells us to unmask it. So at any point in time we can reset the state machine with an unmask. Spurious unmasks are theoretically a problem if an IRQ is shared among multiple devices we can trigger unmasks for devices that haven't been asserted. vfio handles this pretty well though and recognizes the device isn't masked and does nothing.

Something I note out of this discussion is that while the spinlock I use to maintain the state machine is ugly, the lock has no contention. I don't think that's necessarily the case with pic_lock. Anyway, I think we can do w/o the spinlock altogether. Lock contention and spurious eois over level triggered interrupts is probably not worth worrying about.

Michael S. Tsirkin

author

2012-07-19 16:50 UTC #15 lore raw

Quoted 5 lines · Alex Williamson

Yes, the problem isn't the state. The original patch works just fine to mask and assert the interrupt every time the device signals and de-assert and unmask on every EOI. KVM doesn't need to track this for migration (not that we support migration, of course), we can always just send an unmask to the device to retrigger an interrupt if needed.

I agree, just let's document this in case emulated devices use EOIFD.

Quoted 10 lines

The thing Michael is trying to avoid is spurious assertions and de-assertions by tracking the state machine. Spurious assertions are not really a problem, at least for vfio where the interrupt is masked until kvm/qemu tells us to unmask it. So at any point in time we can reset the state machine with an unmask. Spurious unmasks are theoretically a problem if an IRQ is shared among multiple devices we can trigger unmasks for devices that haven't been asserted. vfio handles this pretty well though and recognizes the device isn't masked and does nothing.

Quoted 2 lines

Something I note out of this discussion is that while the spinlock I use to maintain the state machine is ugly, the lock has no contention.

Good point. Overall I'm convinced now it was a good idea.

Quoted 4 lines

I don't think that's necessarily the case with pic_lock. Anyway, I think we can do w/o the spinlock altogether. Lock contention and spurious eois over level triggered interrupts is probably not worth worrying about.

Thanks, good summary.

I think I can prove to you spurious wakeups are a problem though:
consider a setup where an IRQ is shared with a userspace device (e.g. console). If said userspace uses EOIFD you wake up userspace on each interrupt of an assigned device which is exactly what level IRQFD was designed to avoid.

Signature · 1 line

MST