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

["Michael S. Tsirkin" <mst@redhat.com>]

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.