report IRQ injection status to userspace.

PATCH In review Gleb Natapov 3 participants 11 messages Jan 21, 2009 – Feb 11, 2009

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Marcelo Tosatti

2009-01-26 16:11 UTC #2 lore raw

Hi Gleb,

Quoted 28 lines · Gleb Natapov

Use this one instead. Adds capabilities checks.

Signed-off-by Gleb Natapov <[email protected]>

index 179dcb0..2752016 100644

arch/x86/kvm/i8259.c +3 -3

@@ -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)

arch/x86/kvm/i8259.c static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level) · line 103

Quoted 10 lines

arch/x86/kvm/i8259.c +5 -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.

arch/x86/kvm/i8259.c

Quoted 7 lines

arch/x86/kvm/i8259.c +1 -1

-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)
 {

arch/x86/kvm/i8259.c +1 -0

 	u32 old_irr = ioapic->irr;
 	u32 mask = 1 << irq;
 	union ioapic_redir_entry entry;
+	int ret = 1;

-1 here ?

And then, in the userspace part, you consider -1 as "injected":

qemu/hw/i8259.c +0 -0

index 6d41a5e..9da4360 100644
@@ -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 ?

Other than that looks fine to me.

Gleb Natapov

author

2009-01-27 13:29 UTC #3 lore raw

Quoted 44 lines · Marcelo Tosatti

Hi Gleb,

Quoted 28 lines · Gleb Natapov

Use this one instead. Adds capabilities checks.

Signed-off-by Gleb Natapov <[email protected]>

index 179dcb0..2752016 100644

arch/x86/kvm/i8259.c +3 -3

@@ -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)

arch/x86/kvm/i8259.c static inline void pic_set_irq1(struct kvm_kpic_state *s, int irq, int level) · line 103

Quoted 10 lines

arch/x86/kvm/i8259.c +5 -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:

Quoted 6 lines

#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.

Quoted 10 lines

arch/x86/kvm/i8259.c

Quoted 7 lines

arch/x86/kvm/i8259.c +1 -1

-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)
 {

arch/x86/kvm/i8259.c +1 -0

 	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?

Quoted 24 lines

And then, in the userspace part, you consider -1 as "injected":

qemu/hw/i8259.c +8 -3

index 6d41a5e..9da4360 100644
@@ -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

Quoted 2 lines

Is that what you intended ?

Yes! If interrupt was lost due to making it should not be reinjected.

--
Gleb.

Marcelo Tosatti

2009-01-27 15:41 UTC #4 lore raw

Quoted 11 lines · Gleb Natapov

Quoted 2 lines
-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?

Quoted 7 lines

Quoted 6 lines
(quoted diff) +2 -0
+        }
+    }
 #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).

Gleb Natapov

author

2009-01-28 16:39 UTC #5 lore raw

Quoted 13 lines · Marcelo Tosatti

Quoted 11 lines · Gleb Natapov
Quoted 2 lines
-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?

Quoted 6 lines

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?

Quoted 11 lines

Quoted 7 lines
Quoted 6 lines
(quoted diff) +2 -0
+        }
+    }
 #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.

Marcelo Tosatti

2009-01-28 19:10 UTC #6 lore raw

Quoted 14 lines · Gleb Natapov

Quoted 13 lines · Marcelo Tosatti
Quoted 11 lines · Gleb Natapov
Quoted 2 lines
-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?

Quoted 7 lines

Quoted 6 lines
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.

Quoted 5 lines

Quoted 3 lines
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.

Avi Kivity

2009-02-02 14:04 UTC #7 lore raw

In reply to Gleb Natapov

Quoted 10 lines · Gleb Natapov

Quoted 7 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

author

2009-02-02 14:06 UTC #8 lore raw

Quoted 5 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov
Quoted 4 lines
Right, I was thinking about
	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
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.

Avi Kivity

2009-02-02 14:23 UTC #9 lore raw

Quoted 12 lines · Gleb Natapov

Quoted 10 lines · Avi Kivity
Quoted 8 lines · Gleb Natapov
Quoted 8 lines
Right, I was thinking about
	if (irq >= 0 && irq < IOAPIC_NUM_PINS) {
Should return MASKED if irq is outside the acceptable range?

Quoted 9 lines

Quoted 7 lines
Quoted 4 lines
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.

Signature · 1 line

error compiling committee.c: too many arguments to function

Marcelo Tosatti

2009-02-11 20:46 UTC #10 lore raw

Quoted 18 lines · Avi Kivity

Quoted 17 lines · Gleb Natapov
Quoted 15 lines · Avi Kivity
Quoted 10 lines · Gleb Natapov
Quoted 7 lines
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.

Quoted 8 lines

Quoted 2 lines
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 ?