[Qemu-devel] [PATCH] correctly check ppr priority during interrupt injection

[Qemu-devel] [PATCH] correctly check ppr priority during interrupt injection

[Gleb Natapov]

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

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/hw/apic.c b/hw/apic.c
index 2f8376a..cbac75d 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -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.

Re: [Qemu-devel] [PATCH] correctly check ppr priority during interrupt injection

[Yoshiaki Tamura]

2011/2/6 Gleb Natapov <gleb@redhat.com>:
>
> TPR blocks all interrupts in a priority class, so simple "less or
> equal" check is not enough.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/hw/apic.c b/hw/apic.c
> index 2f8376a..cbac75d 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -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;

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

Yoshi

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

Re: [Qemu-devel] [PATCH] correctly check ppr priority during interrupt injection

[Jan Kiszka]

On 2011-02-07 00:45, Yoshiaki Tamura wrote:
> 2011/2/6 Gleb Natapov <gleb@redhat.com>:
>>
>> TPR blocks all interrupts in a priority class, so simple "less or
>> equal" check is not enough.
>>
>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 2f8376a..cbac75d 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -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;
> 
> 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..

>> +
>> +    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

Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>

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

Jan

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

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux