[Qemu-devel] [PATCH] Bugfixes for interrupt numbering in XICS code

[Qemu-devel] [PATCH] Bugfixes for interrupt numbering in XICS code

[David Gibson]

The implementation of the XICS interrupt controller contains several
(difficult to trigger) bugs due to the fact that we were not 100%
consistent with which irq numbering we used.  In most places, global
numbers were used as handled by the presentation layer, however a few
functions took "local" numberings, that is the source number within
the interrupt source controller which is offset from the global
number.  In most cases the function and its caller agreed on this, but
in a few cases it didn't.

This patch cleans this up by always using global numbering.
Translation to the local number is now always and only done when we
look up the individual interrupt source state structure.  This should
remove the existing bugs and with luck reduce the chances of
re-introducing such bugs.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/xics.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/xics.c b/hw/xics.c
index 13a1d25..fd4c875 100644
--- a/hw/xics.c
+++ b/hw/xics.c
@@ -190,14 +190,14 @@ static int ics_valid_irq(struct ics_state *ics, uint32_t nr)
 static void ics_set_irq_msi(void *opaque, int nr, int val)
 {
     struct ics_state *ics = (struct ics_state *)opaque;
-    struct ics_irq_state *irq = ics->irqs + nr;
+    struct ics_irq_state *irq = ics->irqs + nr - ics->offset;
 
     if (val) {
         if (irq->priority == 0xff) {
             irq->masked_pending = 1;
             /* masked pending */ ;
         } else  {
-            icp_irq(ics->icp, irq->server, nr + ics->offset, irq->priority);
+            icp_irq(ics->icp, irq->server, nr, irq->priority);
         }
     }
 }
@@ -229,7 +229,7 @@ static void ics_resend_msi(struct ics_state *ics)
 static void ics_write_xive_msi(struct ics_state *ics, int nr, int server,
                                uint8_t priority)
 {
-    struct ics_irq_state *irq = ics->irqs + nr;
+    struct ics_irq_state *irq = ics->irqs + nr - ics->offset;
 
     irq->server = server;
     irq->priority = priority;
@@ -239,7 +239,7 @@ static void ics_write_xive_msi(struct ics_state *ics, int nr, int server,
     }
 
     irq->masked_pending = 0;
-    icp_irq(ics->icp, server, nr + ics->offset, priority);
+    icp_irq(ics->icp, server, nr, priority);
 }
 
 static void ics_reject(struct ics_state *ics, int nr)
@@ -334,7 +334,7 @@ static void rtas_set_xive(sPAPREnvironment *spapr, uint32_t token,
         return;
     }
 
-    ics_write_xive_msi(ics, nr - ics->offset, server, priority);
+    ics_write_xive_msi(ics, nr, server, priority);
 
     rtas_st(rets, 0, 0); /* Success */
 }
@@ -388,7 +388,7 @@ static void rtas_int_off(sPAPREnvironment *spapr, uint32_t token,
     struct ics_irq_state *irq = xics->irqs + (nr - xics->offset);
 
     irq->saved_priority = irq->priority;
-    ics_write_xive_msi(xics, nr - xics->offset, irq->server, 0xff);
+    ics_write_xive_msi(xics, nr, irq->server, 0xff);
 #endif
 
     rtas_st(rets, 0, 0); /* Success */
@@ -418,8 +418,7 @@ static void rtas_int_on(sPAPREnvironment *spapr, uint32_t token,
 #if 0
     struct ics_irq_state *irq = xics->irqs + (nr - xics->offset);
 
-    ics_write_xive_msi(xics, nr - xics->offset,
-                       irq->server, irq->saved_priority);
+    ics_write_xive_msi(xics, nr, irq->server, irq->saved_priority);
 #endif
 
     rtas_st(rets, 0, 0); /* Success */
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH] Bugfixes for interrupt numbering in XICS code

[David Gibson]

On Tue, Jul 26, 2011 at 10:19:36AM +1000, David Gibson wrote:
> The implementation of the XICS interrupt controller contains several
> (difficult to trigger) bugs due to the fact that we were not 100%
> consistent with which irq numbering we used.  In most places, global
> numbers were used as handled by the presentation layer, however a few
> functions took "local" numberings, that is the source number within
> the interrupt source controller which is offset from the global
> number.  In most cases the function and its caller agreed on this, but
> in a few cases it didn't.
> 
> This patch cleans this up by always using global numbering.
> Translation to the local number is now always and only done when we
> look up the individual interrupt source state structure.  This should
> remove the existing bugs and with luck reduce the chances of
> re-introducing such bugs.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Ugh.  Disregard, the bugfix has a bug, sorry.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson