Skip to contentSkip to conversation

correctly check ppr priority during interrupt injection

PATCH Superseded Gleb Natapov 3 participants 3 messages Feb 6, 2011 – Feb 7, 2011

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 correctly check ppr priority during interrupt injection

TPR blocks all interrupts in a priority class, so simple "less or equal" check is not enough.

hw/apic.c +25 -10
index 2f8376a..cbac75d 100644
@@ -372,19 +372,33 @@ static int apic_get_arb_pri(APICState *s)
     return 0;
 }
 
-/* signal the CPU if an irq is pending */
-static void apic_update_irq(APICState *s)
+
+/* 
+ * <0 - low prio interrupt,
+ * 0  - no interrupt,
+ * >0 - interrupt number
+ */
+static int apic_irq_pending(APICState *s)
 {
     int irrv, ppr;
-    if (!(s->spurious_vec & APIC_SV_ENABLE))
-        return;
     irrv = get_highest_priority_int(s->irr);
     if (irrv < 0)
-        return;
+        return 0;
     ppr = apic_get_ppr(s);
     if (ppr && (irrv & 0xf0) <= (ppr & 0xf0))
+        return -1;
+
+    return irrv;
+}
+
+/* signal the CPU if an irq is pending */
+static void apic_update_irq(APICState *s)
+{
+    if (!(s->spurious_vec & APIC_SV_ENABLE))
         return;
-    cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
+    if (apic_irq_pending(s) > 0) {
+        cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
+    }
 }
 
 void apic_reset_irq_delivered(void)
@@ -590,12 +604,13 @@ int apic_get_interrupt(DeviceState *d)
     if (!(s->spurious_vec & APIC_SV_ENABLE))
         return -1;
 
-    /* XXX: spurious IRQ handling */
-    intno = get_highest_priority_int(s->irr);
-    if (intno < 0)
+    intno = apic_irq_pending(s);
+
+    if (intno == 0) {
         return -1;
-    if (s->tpr && intno <= s->tpr)
+    } else if (intno < 0) {
         return s->spurious_vec & 0xff;
+    }
     reset_bit(s->irr, intno);
     set_bit(s->isr, intno);
     apic_update_irq(s);

--
Gleb.

#1 lore raw

2011/2/6 Gleb Natapov <[email protected]>:

Quoted 23 lines

TPR blocks all interrupts in a priority class, so simple "less or equal" check is not enough.

hw/apic.c +7 -2
index 2f8376a..cbac75d 100644
@@ -372,19 +372,33 @@ static int apic_get_arb_pri(APICState *s)
     return 0;
  }
 
-/* signal the CPU if an irq is pending */
-static void apic_update_irq(APICState *s)
+
+/*
+ * <0 - low prio interrupt,
+ * 0  - no interrupt,
+ * >0 - interrupt number
+ */
+static int apic_irq_pending(APICState *s)
  {

hw/apic.c static int apic_get_arb_pri(APICState *s) · line 389

Quoted 10 lines
hw/apic.c +2 -3
     int irrv, ppr;
-    if (!(s->spurious_vec & APIC_SV_ENABLE))
-        return;
     irrv = get_highest_priority_int(s->irr);
     if (irrv < 0)
-        return;
+        return 0;
     ppr = apic_get_ppr(s);
     if (ppr && (irrv & 0xf0) <= (ppr & 0xf0))
+        return -1;

Just a minor point: braces are missing. checkpatch.pl would help.

Yoshi

Quoted 38 lines
hw/apic.c +16 -6
+
+    return irrv;
+}
+
+/* signal the CPU if an irq is pending */
+static void apic_update_irq(APICState *s)
+{
+    if (!(s->spurious_vec & APIC_SV_ENABLE))
         return;
-    cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
+    if (apic_irq_pending(s) > 0) {
+        cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
+    }
  }
 
  void apic_reset_irq_delivered(void)
@@ -590,12 +604,13 @@ int apic_get_interrupt(DeviceState *d)
     if (!(s->spurious_vec & APIC_SV_ENABLE))
         return -1;
 
-    /* XXX: spurious IRQ handling */
-    intno = get_highest_priority_int(s->irr);
-    if (intno < 0)
+    intno = apic_irq_pending(s);
+
+    if (intno == 0) {
         return -1;
-    if (s->tpr && intno <= s->tpr)
+    } else if (intno < 0) {
         return s->spurious_vec & 0xff;
+    }
     reset_bit(s->irr, intno);
     set_bit(s->isr, intno);
     apic_update_irq(s);
--

                       Gleb.

Reviewed-by
#2 lore raw
Quoted 29 lines · Yoshiaki Tamura

2011/2/6 Gleb Natapov <[email protected]>:

Quoted 28 lines

TPR blocks all interrupts in a priority class, so simple "less or equal" check is not enough.

hw/apic.c +7 -4
index 2f8376a..cbac75d 100644
@@ -372,19 +372,33 @@ static int apic_get_arb_pri(APICState *s)
    return 0;
 }
 
-/* signal the CPU if an irq is pending */
-static void apic_update_irq(APICState *s)
+
+/*
+ * <0 - low prio interrupt,
+ * 0  - no interrupt,
+ * >0 - interrupt number
+ */
+static int apic_irq_pending(APICState *s)
 {
    int irrv, ppr;
-    if (!(s->spurious_vec & APIC_SV_ENABLE))
-        return;
    irrv = get_highest_priority_int(s->irr);
    if (irrv < 0)
Quoted 8 lines

hw/apic.c static int apic_get_arb_pri(APICState *s) · line 389

Quoted 5 lines
hw/apic.c +2 -1
-        return;
+        return 0;
    ppr = apic_get_ppr(s);
    if (ppr && (irrv & 0xf0) <= (ppr & 0xf0))
+        return -1;

Just a minor point: braces are missing. checkpatch.pl would help.

Actually, the code above does not touch lines with missing braces (though I prefer to clean up the close environment of my patches as well) but..

Quoted 9 lines

hw/apic.c static int apic_get_arb_pri(APICState *s) · line 398

Quoted 9 lines
hw/apic.c +8 -0
+
+    return irrv;
+}
+
+/* signal the CPU if an irq is pending */
+static void apic_update_irq(APICState *s)
+{
+    if (!(s->spurious_vec & APIC_SV_ENABLE))
        return;

...here checkpatch would complain. Also about a trailing whitespace earlier.

Besides those minor issues, patch looks good and works fine. So you can add

on resubmission. Please also add a 0.14 tag, this is a must-have for stable IMO.

Jan

Quoted 29 lines
Quoted 29 lines
hw/apic.c +8 -6
-    cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
+    if (apic_irq_pending(s) > 0) {
+        cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
+    }
 }
 
 void apic_reset_irq_delivered(void)
@@ -590,12 +604,13 @@ int apic_get_interrupt(DeviceState *d)
    if (!(s->spurious_vec & APIC_SV_ENABLE))
        return -1;
 
-    /* XXX: spurious IRQ handling */
-    intno = get_highest_priority_int(s->irr);
-    if (intno < 0)
+    intno = apic_irq_pending(s);
+
+    if (intno == 0) {
        return -1;
-    if (s->tpr && intno <= s->tpr)
+    } else if (intno < 0) {
        return s->spurious_vec & 0xff;
+    }
    reset_bit(s->irr, intno);
    set_bit(s->isr, intno);
    apic_update_irq(s);
--
                       Gleb.
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux