From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v2] report IRQ injection status to userspace. Date: Tue, 27 Jan 2009 15:27:39 +0200 Message-ID: <20090127132739.GG15778@redhat.com> References: <20090121113227.GH27675@redhat.com> <20090121123428.GI27675@redhat.com> <20090126161038.GB3894@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , kvm@vger.kernel.org To: Marcelo Tosatti Return-path: Received: from mx2.redhat.com ([66.187.237.31]:53686 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753765AbZA0N3m (ORCPT ); Tue, 27 Jan 2009 08:29:42 -0500 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n0RDTgZg021123 for ; Tue, 27 Jan 2009 08:29:42 -0500 Content-Disposition: inline In-Reply-To: <20090126161038.GB3894@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: 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 > > > 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.