| Message ID | 20090126161038.GB3894@amt.cnet (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 27, 2009 at 03:27:39PM +0200, Gleb Natapov wrote: > > -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? Right, I was thinking about if (irq >= 0 && irq < IOAPIC_NUM_PINS) { Should return MASKED if irq is outside the acceptable range? > > + } > > + } > > #endif > > > > Is that what you intended ? > > > Yes! If interrupt was lost due to making it should not be reinjected. That assumes guests won't mask the interrupt temporarily in the irqchip, hope that is OK (as Avi noted earlier guests use CPU to mask irqs temporarily, most of the time). -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 27, 2009 at 01:41:07PM -0200, Marcelo Tosatti wrote: > On Tue, Jan 27, 2009 at 03:27:39PM +0200, Gleb Natapov wrote: > > > -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? > > Right, I was thinking about > > if (irq >= 0 && irq < IOAPIC_NUM_PINS) { > > Should return MASKED if irq is outside the acceptable range? > Is this ever can be false? Should we BUG() if irq is out of range? > > > + } > > > + } > > > #endif > > > > > > Is that what you intended ? > > > > > Yes! If interrupt was lost due to making it should not be reinjected. > > That assumes guests won't mask the interrupt temporarily in the irqchip, > hope that is OK (as Avi noted earlier guests use CPU to mask irqs > temporarily, most of the time). And if a guest masks interrupts it can't complain that some are lost. I haven't seen Windows masking RTC irq. -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 28, 2009 at 06:37:36PM +0200, Gleb Natapov wrote: > On Tue, Jan 27, 2009 at 01:41:07PM -0200, Marcelo Tosatti wrote: > > On Tue, Jan 27, 2009 at 03:27:39PM +0200, Gleb Natapov wrote: > > > > -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? > > > > Right, I was thinking about > > > > if (irq >= 0 && irq < IOAPIC_NUM_PINS) { > > > > Should return MASKED if irq is outside the acceptable range? > > > Is this ever can be false? Should we BUG() if irq is out of range? If qemu-kvm passes it ouf range IRQ yes. Its just a nitpicking, ignore it. > > That assumes guests won't mask the interrupt temporarily in the irqchip, > > hope that is OK (as Avi noted earlier guests use CPU to mask irqs > > temporarily, most of the time). > And if a guest masks interrupts it can't complain that some are lost. I > haven't seen Windows masking RTC irq. Makes sense. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 02, 2009 at 04:04:55PM +0200, Avi Kivity wrote: > Gleb Natapov wrote: >>> Right, I was thinking about >>> >>> if (irq >= 0 && irq < IOAPIC_NUM_PINS) { >>> >>> Should return MASKED if irq is outside the acceptable range? >>> >>> >> Is this ever can be false? Should we BUG() if irq is out of range? >> >> > > Yes, the number ultimately comes from userspace. > So may be -EINVAL should be returned to userspace? -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Gleb Natapov wrote: >> Right, I was thinking about >> >> if (irq >= 0 && irq < IOAPIC_NUM_PINS) { >> >> Should return MASKED if irq is outside the acceptable range? >> >> > Is this ever can be false? Should we BUG() if irq is out of range? > > Yes, the number ultimately comes from userspace.
Gleb Natapov wrote: > On Mon, Feb 02, 2009 at 04:04:55PM +0200, Avi Kivity wrote: > >> Gleb Natapov wrote: >> >>>> Right, I was thinking about >>>> >>>> if (irq >= 0 && irq < IOAPIC_NUM_PINS) { >>>> >>>> Should return MASKED if irq is outside the acceptable range? >>>> >>>> >>>> >>> Is this ever can be false? Should we BUG() if irq is out of range? >>> >>> >>> >> Yes, the number ultimately comes from userspace. >> >> > So may be -EINVAL should be returned to userspace? > Mmm, not sure. An out-of-bounds number here could be caused by userspace generating the wrong irq line number, or by the guest misconfiguring interrupts. We should error out on userspace bugs, but not guest bugs.
On Mon, Feb 02, 2009 at 04:23:40PM +0200, Avi Kivity wrote: > Gleb Natapov wrote: >> On Mon, Feb 02, 2009 at 04:04:55PM +0200, Avi Kivity wrote: >> >>> Gleb Natapov wrote: >>> >>>>> Right, I was thinking about >>>>> >>>>> if (irq >= 0 && irq < IOAPIC_NUM_PINS) { >>>>> >>>>> Should return MASKED if irq is outside the acceptable range? >>>>> >>>>> >>>> Is this ever can be false? Should we BUG() if irq is out of range? >>>> >>>> >>> Yes, the number ultimately comes from userspace. >>> >>> >> So may be -EINVAL should be returned to userspace? >> > > Mmm, not sure. An out-of-bounds number here could be caused by > userspace generating the wrong irq line number, or by the guest > misconfiguring interrupts. > > We should error out on userspace bugs, but not guest bugs. I don't see this applied yet ? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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