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@xxxxxxxxxx>
>
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
