[Qemu-devel] [PATCH 0/4] arm_gic: Various fixes

[Qemu-devel] [PATCH 0/4] arm_gic: Various fixes

[luc.michel]

From: Luc MICHEL <luc.michel@git.antfield.fr>

Hi,

Here is a patch set for issues I found in the GIC. I found those by
writing unitary tests for the GIC, and compared the result against
real hardware (a Zynq UltraScale+ board with a GICv2).

The first patch prevents the GIC from signaling an IRQ that is in the
"active and pending" state. I encountered this bug in a test where I
split end of interrupt and interrupt deactivation. The GIC was
re-signaling the IRQ after priority drop if it was raised again, while
it has not been deactivated yet (and thus was in the "active and
pending" state).

The second patch returns a correct "Idle priority" value when reading
C_RPR if there is no active interrupt.

The last two patches fix issues around the Binary Point Register (the
group priority computation of group 1 IRQs when C_CTRL.CBPR is 0, and
the non-secure view of C_BPR when C_CTRL.CBPR is 1).

Luc MICHEL (4):
  hw/intc/arm_gic: Prevent the GIC from signaling an IRQ when it's
    "active and pending"
  hw/intc/arm_gic: Fix C_RPR value on idle priority
  hw/intc/arm_gic: Fix group priority computation for group 1 IRQs
  hw/intc/arm_gic: Fix the NS view of C_BPR when C_CTRL.CBPR is 1

 hw/intc/arm_gic.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

-- 
2.16.0

[Qemu-devel] [PATCH 1/4] hw/intc/arm_gic: Prevent the GIC from signaling an IRQ when it's "active and pending"

[luc.michel]

From: Luc MICHEL <luc.michel@git.antfield.fr>

In the GIC, when an IRQ is acknowledged, its state goes from "pending"
to:
   - "active" if the corresponding IRQ pin has been de-asserted
   - "active and pending" otherwise.
The GICv2 manual states that when a IRQ becomes active (or active and
pending), the GIC should either signal another (higher priority) IRQ to
the CPU if there is one, or de-assert the CPU IRQ pin.

The current implementation of the GIC in QEMU does not check if the
IRQ is already active when looking for pending interrupts with
sufficient priority in gic_update(). This can lead to signaling an
interrupt that is already active.

This usually happens when splitting priority drop and interrupt
deactivation. On priority drop, the IRQ stays active until deactivation.
If it becomes pending again, chances are that it will be incorrectly
selected as best_irq in gic_update().

This commit fixes this by checking if the IRQ is not already active when
looking for best_irq in gic_update().

Note that regarding the ARM11MPCore GIC version, the corresponding
manual is not clear on that point, but it has has no priority
drop/interrupt deactivation separation, so this case should not happen.

Signed-off-by: Luc MICHEL <luc.michel@git.antfield.fr>
---
 hw/intc/arm_gic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index d701e49ff9..dad383ea12 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -93,6 +93,7 @@ void gic_update(GICState *s)
         best_irq = 1023;
         for (irq = 0; irq < s->num_irq; irq++) {
             if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm) &&
+                (!GIC_TEST_ACTIVE(irq, cm)) &&
                 (irq < GIC_INTERNAL || GIC_TARGET(irq) & cm)) {
                 if (GIC_GET_PRIORITY(irq, cpu) < best_prio) {
                     best_prio = GIC_GET_PRIORITY(irq, cpu);
-- 
2.16.0

[Qemu-devel] [PATCH 2/4] hw/intc/arm_gic: Fix C_RPR value on idle priority

[luc.michel]

From: Luc MICHEL <luc.michel@git.antfield.fr>

When there is no active interrupts in the GIC, a read to the C_RPR
register should return the value of the "Idle priority", which is either
the maximum value an IRQ priority field can be set to, or 0xff.

Since the QEMU GIC model implements all the 8 priority bits, the Idle
priority is 0xff.

Internally, when there is no active interrupt, the running priority
value is 0x100. The gic_get_running_priority function returns an uint8_t
and thus, truncate this value to 0x00 when returning it. This is wrong since
a value of 0x00 correspond to the maximum possible priority.

This commit fixes the returned value when the internal value is 0x100.

Note that it is correct for the Non-Secure view to return 0xff even
though from the NS world point of view, only 7 priority bits are
implemented. The specification states that the Idle priority can be 0xff
even when not all the 8 priority bits are implemented. This has been
verified against a real GICv2 hardware on a Xilinx ZynqMP based board.

Regarding the ARM11MPCore version of the GIC, the specification is not
clear on that point, so this commit does not alter its behavior.

Signed-off-by: Luc MICHEL <luc.michel@git.antfield.fr>
---
 hw/intc/arm_gic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index dad383ea12..713de3084f 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -504,6 +504,11 @@ static void gic_set_cpu_control(GICState *s, int cpu, uint32_t value,
 
 static uint8_t gic_get_running_priority(GICState *s, int cpu, MemTxAttrs attrs)
 {
+    if ((s->revision != REV_11MPCORE) && (s->running_priority[cpu] > 0xff)) {
+        /* Idle priority */
+        return 0xff;
+    }
+
     if (s->security_extn && !attrs.secure) {
         if (s->running_priority[cpu] & 0x80) {
             /* Running priority in upper half of range: return the Non-secure
-- 
2.16.0

[Qemu-devel] [PATCH 3/4] hw/intc/arm_gic: Fix group priority computation for group 1 IRQs

[luc.michel]

From: Luc MICHEL <luc.michel@git.antfield.fr>

When determining the group priority of a group 1 IRQ, if C_CTRL.CBPR is
0, the non-secure BPR value is used. However, this value must be
incremented by one so that it matches the secure world number of
implemented priority bits (NS world has one less priority bit compared
to the Secure world).

Signed-off-by: Luc MICHEL <luc.michel@git.antfield.fr>
---
 hw/intc/arm_gic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 713de3084f..d0a41a89ae 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -256,7 +256,7 @@ static int gic_get_group_priority(GICState *s, int cpu, int irq)
     if (gic_has_groups(s) &&
         !(s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) &&
         GIC_TEST_GROUP(irq, (1 << cpu))) {
-        bpr = s->abpr[cpu];
+        bpr = s->abpr[cpu] - 1;
     } else {
         bpr = s->bpr[cpu];
     }
-- 
2.16.0

[Qemu-devel] [PATCH 4/4] hw/intc/arm_gic: Fix the NS view of C_BPR when C_CTRL.CBPR is 1

[luc.michel]

From: Luc MICHEL <luc.michel@git.antfield.fr>

When C_CTRL.CBPR is 1, the Non-Secure view of C_BPR is altered:
  - A Non-Secure read of C_BPR should return the BPR value plus 1,
  saturated to 7,
  - A Non-Secure write should be ignored.

Signed-off-by: Luc MICHEL <luc.michel@git.antfield.fr>
---
 hw/intc/arm_gic.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index d0a41a89ae..7418b7a082 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -1211,8 +1211,13 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
         break;
     case 0x08: /* Binary Point */
         if (s->security_extn && !attrs.secure) {
-            /* BPR is banked. Non-secure copy stored in ABPR. */
-            *data = s->abpr[cpu];
+            if (s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) {
+                /* NS view of BPR when CBPR is 1 */
+                *data = MIN(s->bpr[cpu] + 1, 7);
+            } else {
+                /* BPR is banked. Non-secure copy stored in ABPR. */
+                *data = s->abpr[cpu];
+            }
         } else {
             *data = s->bpr[cpu];
         }
@@ -1285,7 +1290,12 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
         break;
     case 0x08: /* Binary Point */
         if (s->security_extn && !attrs.secure) {
-            s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
+            if (s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) {
+                /* WI when CTLR is 1 */
+                return MEMTX_OK;
+            } else {
+                s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
+            }
         } else {
             s->bpr[cpu] = MAX(value & 0x7, GIC_MIN_BPR);
         }
-- 
2.16.0

Re: [Qemu-devel] [PATCH 4/4] hw/intc/arm_gic: Fix the NS view of C_BPR when C_CTRL.CBPR is 1

[Peter Maydell]

On 19 January 2018 at 14:57,  <luc.michel@greensocs.com> wrote:
> From: Luc MICHEL <luc.michel@git.antfield.fr>
>
> When C_CTRL.CBPR is 1, the Non-Secure view of C_BPR is altered:
>   - A Non-Secure read of C_BPR should return the BPR value plus 1,
>   saturated to 7,
>   - A Non-Secure write should be ignored.
>
> Signed-off-by: Luc MICHEL <luc.michel@git.antfield.fr>
> ---
>  hw/intc/arm_gic.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index d0a41a89ae..7418b7a082 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -1211,8 +1211,13 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>          break;
>      case 0x08: /* Binary Point */
>          if (s->security_extn && !attrs.secure) {
> -            /* BPR is banked. Non-secure copy stored in ABPR. */
> -            *data = s->abpr[cpu];
> +            if (s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) {
> +                /* NS view of BPR when CBPR is 1 */
> +                *data = MIN(s->bpr[cpu] + 1, 7);
> +            } else {
> +                /* BPR is banked. Non-secure copy stored in ABPR. */
> +                *data = s->abpr[cpu];
> +            }
>          } else {
>              *data = s->bpr[cpu];
>          }
> @@ -1285,7 +1290,12 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>          break;
>      case 0x08: /* Binary Point */
>          if (s->security_extn && !attrs.secure) {
> -            s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
> +            if (s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) {
> +                /* WI when CTLR is 1 */

should be "CBPR", yes?

> +                return MEMTX_OK;
> +            } else {
> +                s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
> +            }
>          } else {
>              s->bpr[cpu] = MAX(value & 0x7, GIC_MIN_BPR);
>          }
> --

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/4] hw/intc/arm_gic: Fix the NS view of C_BPR when C_CTRL.CBPR is 1

[Luc Michel]

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

On 01/22/2018 04:19 PM, Peter Maydell wrote:
> On 19 January 2018 at 14:57,  <luc.michel@greensocs.com> wrote:
>> From: Luc MICHEL <luc.michel@git.antfield.fr>
>>
>> When C_CTRL.CBPR is 1, the Non-Secure view of C_BPR is altered:
>>   - A Non-Secure read of C_BPR should return the BPR value plus 1,
>>   saturated to 7,
>>   - A Non-Secure write should be ignored.
>>
>> Signed-off-by: Luc MICHEL <luc.michel@git.antfield.fr>
>> ---
>>  hw/intc/arm_gic.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index d0a41a89ae..7418b7a082 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -1211,8 +1211,13 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>>          break;
>>      case 0x08: /* Binary Point */
>>          if (s->security_extn && !attrs.secure) {
>> -            /* BPR is banked. Non-secure copy stored in ABPR. */
>> -            *data = s->abpr[cpu];
>> +            if (s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) {
>> +                /* NS view of BPR when CBPR is 1 */
>> +                *data = MIN(s->bpr[cpu] + 1, 7);
>> +            } else {
>> +                /* BPR is banked. Non-secure copy stored in ABPR. */
>> +                *data = s->abpr[cpu];
>> +            }
>>          } else {
>>              *data = s->bpr[cpu];
>>          }
>> @@ -1285,7 +1290,12 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>>          break;
>>      case 0x08: /* Binary Point */
>>          if (s->security_extn && !attrs.secure) {
>> -            s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
>> +            if (s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) {
>> +                /* WI when CTLR is 1 */
> 
> should be "CBPR", yes?
Oops yes, sorry.

> 
>> +                return MEMTX_OK;
>> +            } else {
>> +                s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
>> +            }
>>          } else {
>>              s->bpr[cpu] = MAX(value & 0x7, GIC_MIN_BPR);
>>          }
>> --
> 
> thanks
> -- PMM
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

Re: [Qemu-devel] [PATCH 3/4] hw/intc/arm_gic: Fix group priority computation for group 1 IRQs

[Peter Maydell]

On 19 January 2018 at 14:57,  <luc.michel@greensocs.com> wrote:
> From: Luc MICHEL <luc.michel@git.antfield.fr>
>
> When determining the group priority of a group 1 IRQ, if C_CTRL.CBPR is
> 0, the non-secure BPR value is used. However, this value must be
> incremented by one so that it matches the secure world number of
> implemented priority bits (NS world has one less priority bit compared
> to the Secure world).
>
> Signed-off-by: Luc MICHEL <luc.michel@git.antfield.fr>
> ---
>  hw/intc/arm_gic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 713de3084f..d0a41a89ae 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -256,7 +256,7 @@ static int gic_get_group_priority(GICState *s, int cpu, int irq)
>      if (gic_has_groups(s) &&
>          !(s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) &&
>          GIC_TEST_GROUP(irq, (1 << cpu))) {
> -        bpr = s->abpr[cpu];
> +        bpr = s->abpr[cpu] - 1;

I would suggest an "assert(bpr >= 0);" here. (We have a similar
assert for GICv3 in icv_gprio_mask() and icc_gprio_mask() in
arm_gicv3_cpuif.c.)

>      } else {
>          bpr = s->bpr[cpu];
>      }
> --
> 2.16.0

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] arm_gic: Various fixes

[Peter Maydell]

On 19 January 2018 at 14:57,  <luc.michel@greensocs.com> wrote:
> Here is a patch set for issues I found in the GIC. I found those by
> writing unitary tests for the GIC, and compared the result against
> real hardware (a Zynq UltraScale+ board with a GICv2).
>
> The first patch prevents the GIC from signaling an IRQ that is in the
> "active and pending" state. I encountered this bug in a test where I
> split end of interrupt and interrupt deactivation. The GIC was
> re-signaling the IRQ after priority drop if it was raised again, while
> it has not been deactivated yet (and thus was in the "active and
> pending" state).
>
> The second patch returns a correct "Idle priority" value when reading
> C_RPR if there is no active interrupt.
>
> The last two patches fix issues around the Binary Point Register (the
> group priority computation of group 1 IRQs when C_CTRL.CBPR is 0, and
> the non-secure view of C_BPR when C_CTRL.CBPR is 1).

Thanks for this patchset, and in particular for the detailed
commit messages that made it really easy to review.
Since there were only a couple of minor nits in the patchset
(an extra assert, a comment typo) I'm going to take this into
target-arm.next and fix those things there, rather than ask you
to respin the patchset.

thanks
-- PMM