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

[Gleb Natapov <gleb@redhat.com>]

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

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

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

--
			Gleb.