[Qemu-devel] [PATCH for-2.4] hw/intc/arm_gic_common.c: Reset all registers

[Qemu-devel] [PATCH for-2.4] hw/intc/arm_gic_common.c: Reset all registers

[Peter Maydell]

The arm_gic_common reset function was missing reset code for
several of the GIC's state fields:
 * bpr[]
 * abpr[]
 * priority1[]
 * priority2[]
 * sgi_pending[]
 * irq_target[] (SMP configurations only)

These probably went unnoticed because most guests will either
never touch them, or will write to them in the process of
configuring the GIC before enabling interrupts.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The reason for using loops to set these array elements to 0 rather
than using memset() is that to support "directly boot a kernel in
NS on a TZ-aware GIC and CPU" we need to support resetting the
priority registers (most notably the CPU priority mask) to 0x80
rather than 0.

I found this via code review rather than because it triggered
any kind of misbehaviour.

last_active[] does not need any reset, I believe.

 hw/intc/arm_gic_common.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 044ad66..a64d071 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -123,7 +123,7 @@ static void arm_gic_common_realize(DeviceState *dev, Error **errp)
 static void arm_gic_common_reset(DeviceState *dev)
 {
     GICState *s = ARM_GIC_COMMON(dev);
-    int i;
+    int i, j;
     memset(s->irq_state, 0, GIC_MAXIRQ * sizeof(gic_irq_state));
     for (i = 0 ; i < s->num_cpu; i++) {
         if (s->revision == REV_11MPCORE) {
@@ -135,15 +135,30 @@ static void arm_gic_common_reset(DeviceState *dev)
         s->running_irq[i] = 1023;
         s->running_priority[i] = 0x100;
         s->cpu_ctlr[i] = 0;
+        s->bpr[i] = GIC_MIN_BPR;
+        s->abpr[i] = GIC_MIN_ABPR;
+        for (j = 0; j < GIC_INTERNAL; j++) {
+            s->priority1[j][i] = 0;
+        }
+        for (j = 0; j < GIC_NR_SGIS; j++) {
+            s->sgi_pending[j][i] = 0;
+        }
     }
     for (i = 0; i < GIC_NR_SGIS; i++) {
         GIC_SET_ENABLED(i, ALL_CPU_MASK);
         GIC_SET_EDGE_TRIGGER(i);
     }
-    if (s->num_cpu == 1) {
+
+    for (i = 0; i < ARRAY_SIZE(s->priority2); i++) {
+        s->priority2[i] = 0;
+    }
+
+    for (i = 0; i < GIC_MAXIRQ; i++) {
         /* For uniprocessor GICs all interrupts always target the sole CPU */
-        for (i = 0; i < GIC_MAXIRQ; i++) {
+        if (s->num_cpu == 1) {
             s->irq_target[i] = 1;
+        } else {
+            s->irq_target[i] = 0;
         }
     }
     s->ctlr = 0;
-- 
1.9.1

Re: [Qemu-devel] [PATCH for-2.4] hw/intc/arm_gic_common.c: Reset all registers

[Peter Maydell]

On 29 June 2015 at 19:25, Peter Maydell <peter.maydell@linaro.org> wrote:
> The arm_gic_common reset function was missing reset code for
> several of the GIC's state fields:
>  * bpr[]
>  * abpr[]
>  * priority1[]
>  * priority2[]
>  * sgi_pending[]
>  * irq_target[] (SMP configurations only)
>
> These probably went unnoticed because most guests will either
> never touch them, or will write to them in the process of
> configuring the GIC before enabling interrupts.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The reason for using loops to set these array elements to 0 rather
> than using memset() is that to support "directly boot a kernel in
> NS on a TZ-aware GIC and CPU" we need to support resetting the
> priority registers (most notably the CPU priority mask) to 0x80
> rather than 0.
>
> I found this via code review rather than because it triggered
> any kind of misbehaviour.
>
> last_active[] does not need any reset, I believe.
>
>  hw/intc/arm_gic_common.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)

I'd like to get this in before hardfreeze, ideally, so if
anybody has time to review it on Monday that would be great.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.4] hw/intc/arm_gic_common.c: Reset all registers

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 952 bytes --]

On 04/07/2015 9:24 pm, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> On 4 July 2015 at 00:11, Edgar E. Iglesias <edgar.iglesias@gmail.com>
wrote:
> >
> >
> > On Tue, Jun 30, 2015 at 4:25 AM, Peter Maydell <peter.maydell@linaro.org
>
> > wrote:
> >>
> >> The arm_gic_common reset function was missing reset code for
> >> several of the GIC's state fields:
> >>  * bpr[]
> >>  * abpr[]
> >>  * priority1[]
> >>  * priority2[]
> >>  * sgi_pending[]
> >>  * irq_target[] (SMP configurations only)
> >>
> >> These probably went unnoticed because most guests will either
> >> never touch them, or will write to them in the process of
> >> configuring the GIC before enabling interrupts.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> >
> >
> > Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
> I assume you meant to send that to the list? :-)

Yes sorry :-)

Cheers,
Edgar

>
> Thanks for the review.
>
> -- PMM

[-- Attachment #2: Type: text/html, Size: 1675 bytes --]