[PATCH RFC untested] kvm_set_irq: report coalesced for clear

[PATCH RFC untested] kvm_set_irq: report coalesced for clear

[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 <mst@redhat.com>
---

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(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index fe6e9f1..6f5ed0a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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);
diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
index 95b504b..44e3635 100644
--- a/arch/x86/kvm/i8259.c
+++ b/arch/x86/kvm/i8259.c
@@ -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,
diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index 268b332..edc9445 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -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) ||
-- 
MST

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Alex Williamson]

On Thu, 2012-07-19 at 01:11 +0300, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
> 
> 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(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index fe6e9f1..6f5ed0a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -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);
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 95b504b..44e3635 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -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) {

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;


> +		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,
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 268b332..edc9445 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -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) {

Same here

> +		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) ||

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Michael S. Tsirkin]

On Wed, Jul 18, 2012 at 04:40:38PM -0600, Alex Williamson wrote:
> On Thu, 2012-07-19 at 01:11 +0300, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> > ---
> > 
> > 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(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index fe6e9f1..6f5ed0a 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -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);
> > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > index 95b504b..44e3635 100644
> > --- a/arch/x86/kvm/i8259.c
> > +++ b/arch/x86/kvm/i8259.c
> > @@ -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) {
> 
> 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;
> 
> 
> > +		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,
> > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> > index 268b332..edc9445 100644
> > --- a/virt/kvm/ioapic.c
> > +++ b/virt/kvm/ioapic.c
> > @@ -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) {
> 
> Same here
> 
> > +		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) ||
> 


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

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Gleb Natapov]

On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> ---
> 
> 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?

> 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(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index fe6e9f1..6f5ed0a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -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);
> diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> index 95b504b..44e3635 100644
> --- a/arch/x86/kvm/i8259.c
> +++ b/arch/x86/kvm/i8259.c
> @@ -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,
> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
> index 268b332..edc9445 100644
> --- a/virt/kvm/ioapic.c
> +++ b/virt/kvm/ioapic.c
> @@ -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) ||
> -- 
> MST

--
			Gleb.

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Michael S. Tsirkin]

On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> > ---
> > 
> > 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 ...

-- 
MST

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Gleb Natapov]

On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > 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 <mst@redhat.com>
> > > ---
> > > 
> > > 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 ...
> 
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.

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Michael S. Tsirkin]

On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > 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 <mst@redhat.com>
> > > > ---
> > > > 
> > > > 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 ...
> > 
> 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.

> --
> 			Gleb.

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Gleb Natapov]

On Thu, Jul 19, 2012 at 12:33:29PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> > On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > > 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 <mst@redhat.com>
> > > > > ---
> > > > > 
> > > > > 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 ...
> > > 
> > 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.
> 
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.

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

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Michael S. Tsirkin]

On Thu, Jul 19, 2012 at 12:41:24PM +0300, Gleb Natapov wrote:
> On Thu, Jul 19, 2012 at 12:33:29PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> > > On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > > > 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 <mst@redhat.com>
> > > > > > ---
> > > > > > 
> > > > > > 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 ...
> > > > 
> > > 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.
> > 
> There is no any problem. Source knows what the line status is.

With EOIFD and level IRQFD, it does not.

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

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


> --
> 			Gleb.

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Gleb Natapov]

On Thu, Jul 19, 2012 at 01:26:48PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 19, 2012 at 12:41:24PM +0300, Gleb Natapov wrote:
> > On Thu, Jul 19, 2012 at 12:33:29PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> > > > On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > > > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > > > > 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 <mst@redhat.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > 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 ...
> > > > > 
> > > > 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.
> > > 
> > 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?

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

> 
> > > 
> > > 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.
> 
Can you point me to exact problem in Alex's patch?

--
			Gleb.

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Michael S. Tsirkin]

On Thu, Jul 19, 2012 at 01:54:53PM +0300, Gleb Natapov wrote:
> On Thu, Jul 19, 2012 at 01:26:48PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 19, 2012 at 12:41:24PM +0300, Gleb Natapov wrote:
> > > On Thu, Jul 19, 2012 at 12:33:29PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> > > > > On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > > > > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > > > > > 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 <mst@redhat.com>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > 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 ...
> > > > > > 
> > > > > 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.
> > > > 
> > > 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.

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

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

> --
> 			Gleb.

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Michael S. Tsirkin]

On Thu, Jul 19, 2012 at 02:12:13PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 19, 2012 at 01:54:53PM +0300, Gleb Natapov wrote:
> > On Thu, Jul 19, 2012 at 01:26:48PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jul 19, 2012 at 12:41:24PM +0300, Gleb Natapov wrote:
> > > > On Thu, Jul 19, 2012 at 12:33:29PM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> > > > > > On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > > > > > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > 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 <mst@redhat.com>
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > 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 ...
> > > > > > > 
> > > > > > 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.
> > > > > 
> > > > 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.
> 
> > > > 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.
> 
> > > 
> > > > > 
> > > > > 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.
> > > 
> > 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.

> > --
> > 			Gleb.

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Gleb Natapov]

On Thu, Jul 19, 2012 at 02:12:13PM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 19, 2012 at 01:54:53PM +0300, Gleb Natapov wrote:
> > On Thu, Jul 19, 2012 at 01:26:48PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jul 19, 2012 at 12:41:24PM +0300, Gleb Natapov wrote:
> > > > On Thu, Jul 19, 2012 at 12:33:29PM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> > > > > > On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > > > > > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > 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 <mst@redhat.com>
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > 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 ...
> > > > > > > 
> > > > > > 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.
> > > > > 
> > > > 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.

> > > > 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.
> 
May be. What kvm_set_irq() return in case of level=0 was never
important.

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

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Michael S. Tsirkin]

On Thu, Jul 19, 2012 at 02:25:29PM +0300, Gleb Natapov wrote:
> On Thu, Jul 19, 2012 at 02:12:13PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Jul 19, 2012 at 01:54:53PM +0300, Gleb Natapov wrote:
> > > On Thu, Jul 19, 2012 at 01:26:48PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Jul 19, 2012 at 12:41:24PM +0300, Gleb Natapov wrote:
> > > > > On Thu, Jul 19, 2012 at 12:33:29PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> > > > > > > On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > > > > > > > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > > > > > > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > > 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 <mst@redhat.com>
> > > > > > > > > > ---
> > > > > > > > > > 
> > > > > > > > > > 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 ...
> > > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > 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.
> > > > > 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.
> > 
> 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.

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

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

> --
> 			Gleb.

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Alex Williamson]

On Thu, 2012-07-19 at 14:57 +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 19, 2012 at 02:25:29PM +0300, Gleb Natapov wrote:
> > On Thu, Jul 19, 2012 at 02:12:13PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jul 19, 2012 at 01:54:53PM +0300, Gleb Natapov wrote:
> > > > On Thu, Jul 19, 2012 at 01:26:48PM +0300, Michael S. Tsirkin wrote:
> > > > > On Thu, Jul 19, 2012 at 12:41:24PM +0300, Gleb Natapov wrote:
> > > > > > On Thu, Jul 19, 2012 at 12:33:29PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Jul 19, 2012 at 12:21:07PM +0300, Gleb Natapov wrote:
> > > > > > > > On Thu, Jul 19, 2012 at 12:17:19PM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > On Thu, Jul 19, 2012 at 10:53:37AM +0300, Gleb Natapov wrote:
> > > > > > > > > > On Thu, Jul 19, 2012 at 01:11:53AM +0300, Michael S. Tsirkin wrote:
> > > > > > > > > > > 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 <mst@redhat.com>
> > > > > > > > > > > ---
> > > > > > > > > > > 
> > > > > > > > > > > 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 ...
> > > > > > > > > 
> > > > > > > > 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.
> > > > > > > 
> > > > > > 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.
> > > > > > 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.
> > > 
> > 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.
> 
> > > > > 
> > > > > > > 
> > > > > > > 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.
> > > > > 
> > > > 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.
> 
> 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.

Re: [PATCH RFC untested] kvm_set_irq: report coalesced for clear

[Michael S. Tsirkin]

On Thu, Jul 19, 2012 at 10:38:03AM -0600, Alex Williamson wrote:
> 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.

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

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

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

-- 
MST