diff mbox

[RFC] hw/intc/arm_gic: handle Set-Active/Clear-Active registers

Message ID 20160905140944.5379-1-alex.bennee@linaro.org
State New
Headers show

Commit Message

Alex Bennée Sept. 5, 2016, 2:09 p.m. UTC 
I noticed while testing with modern kernels and -d guest_errors warnings
about invalid writes to the GIC. For GICv2 these registers certainly
should work so I've implemented both. As the code is common between all
the various GICs writes to GICD_ISACTIVERn is checked to ensure it is
not a RO register for v1 GICs.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
 hw/intc/arm_gic.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

-- 
2.9.3

Comments

Peter Maydell Sept. 5, 2016, 3:06 p.m. UTC | #1 
On 5 September 2016 at 15:09, Alex Bennée <alex.bennee@linaro.org> wrote:
> I noticed while testing with modern kernels and -d guest_errors warnings

> about invalid writes to the GIC. For GICv2 these registers certainly

> should work so I've implemented both. As the code is common between all

> the various GICs writes to GICD_ISACTIVERn is checked to ensure it is

> not a RO register for v1 GICs.


This is definitely a bug, and also the right way to fix it, so you
don't need to mark your patch 'RFC' :-)

Some minor review issues below.

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---

>  hw/intc/arm_gic.c | 33 +++++++++++++++++++++++++++++++--

>  1 file changed, 31 insertions(+), 2 deletions(-)

>

> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c

> index b30cc91..423a4ae 100644

> --- a/hw/intc/arm_gic.c

> +++ b/hw/intc/arm_gic.c

> @@ -972,9 +972,38 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,

>                  GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);

>              }

>          }

> +    } else if (offset < 0x380) {

> +        /* Interrupt Set-Active */

> +        irq = (offset - 0x300) * 8 + GIC_BASE_IRQ;

> +        if (irq >= s->num_irq || s->revision < 2)


Better to check "s->revision != 2" -- we still have the NVIC
code tangled up with the GIC, and on the NVIC these are R/O.

> +            goto bad_reg;

> +

> +        for (i = 0; i < 8; i++) {

> +            if (s->security_extn && !attrs.secure &&

> +                !GIC_TEST_GROUP(irq + i, 1 << cpu)) {

> +                continue; /* Ignore Non-secure access of Group0 IRQ */

> +            }

> +

> +            if (value & (1 << i)) {

> +                GIC_SET_ACTIVE(irq + i, 1 << cpu);


The mask parameter to GIC_SET_ACTIVE/GIC_CLEAR_ACTIVE should be
calculated like
                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;

-- compare the set-enable, clear-enable, etc write code.

I'm fairly sure that just setting the active bit (ie not also
trying to update the active-priority registers) is the correct
behaviour here, though the GIC spec is not clear to me on this point.

> +            }

> +        }

>      } else if (offset < 0x400) {

> -        /* Interrupt Active.  */

> -        goto bad_reg;

> +        /* Interrupt Clear-Active  */

> +        irq = (offset - 0x380) * 8 + GIC_BASE_IRQ;

> +        if (irq >= s->num_irq)

> +            goto bad_reg;


This is missing the check against s->revision.

> +

> +        for (i = 0; i < 8; i++) {

> +            if (s->security_extn && !attrs.secure &&

> +                !GIC_TEST_GROUP(irq + i, 1 << cpu)) {

> +                continue; /* Ignore Non-secure access of Group0 IRQ */

> +            }

> +

> +            if (value & (1 << i)) {

> +                GIC_CLEAR_ACTIVE(irq + i, 1 << cpu);

> +            }

> +        }

>      } else if (offset < 0x800) {

>          /* Interrupt Priority.  */

>          irq = (offset - 0x400) + GIC_BASE_IRQ;

> --

> 2.9.3


It looks like we don't implement reads of clear-active correctly
either.

thanks
-- PMM
Alex Bennée Sept. 5, 2016, 3:45 p.m. UTC | #2 
Peter Maydell <peter.maydell@linaro.org> writes:

> On 5 September 2016 at 15:09, Alex Bennée <alex.bennee@linaro.org> wrote:

>> I noticed while testing with modern kernels and -d guest_errors warnings

>> about invalid writes to the GIC. For GICv2 these registers certainly

>> should work so I've implemented both. As the code is common between all

>> the various GICs writes to GICD_ISACTIVERn is checked to ensure it is

>> not a RO register for v1 GICs.

>

> This is definitely a bug, and also the right way to fix it, so you

> don't need to mark your patch 'RFC' :-)

>

> Some minor review issues below.

>

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> ---

>>  hw/intc/arm_gic.c | 33 +++++++++++++++++++++++++++++++--

>>  1 file changed, 31 insertions(+), 2 deletions(-)

>>

>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c

>> index b30cc91..423a4ae 100644

>> --- a/hw/intc/arm_gic.c

>> +++ b/hw/intc/arm_gic.c

>> @@ -972,9 +972,38 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,

>>                  GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);

>>              }

>>          }

>> +    } else if (offset < 0x380) {

>> +        /* Interrupt Set-Active */

>> +        irq = (offset - 0x300) * 8 + GIC_BASE_IRQ;

>> +        if (irq >= s->num_irq || s->revision < 2)

>

> Better to check "s->revision != 2" -- we still have the NVIC

> code tangled up with the GIC, and on the NVIC these are R/O.

>

>> +            goto bad_reg;

>> +

>> +        for (i = 0; i < 8; i++) {

>> +            if (s->security_extn && !attrs.secure &&

>> +                !GIC_TEST_GROUP(irq + i, 1 << cpu)) {

>> +                continue; /* Ignore Non-secure access of Group0 IRQ */

>> +            }

>> +

>> +            if (value & (1 << i)) {

>> +                GIC_SET_ACTIVE(irq + i, 1 << cpu);

>

> The mask parameter to GIC_SET_ACTIVE/GIC_CLEAR_ACTIVE should be

> calculated like

>                 int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;

>

> -- compare the set-enable, clear-enable, etc write code.

>

> I'm fairly sure that just setting the active bit (ie not also

> trying to update the active-priority registers) is the correct

> behaviour here, though the GIC spec is not clear to me on this point.

>

>> +            }

>> +        }

>>      } else if (offset < 0x400) {

>> -        /* Interrupt Active.  */

>> -        goto bad_reg;

>> +        /* Interrupt Clear-Active  */

>> +        irq = (offset - 0x380) * 8 + GIC_BASE_IRQ;

>> +        if (irq >= s->num_irq)

>> +            goto bad_reg;

>

> This is missing the check against s->revision.


This was deliberate as the GICv1 reference was only mentioned in the
GICD_ISACTIVERn description. Unfortunately the only documentation for
GICs on silver.arm.com are for v2 and v3 so I had to guess what v1 had :-/

>

>> +

>> +        for (i = 0; i < 8; i++) {

>> +            if (s->security_extn && !attrs.secure &&

>> +                !GIC_TEST_GROUP(irq + i, 1 << cpu)) {

>> +                continue; /* Ignore Non-secure access of Group0 IRQ */

>> +            }

>> +

>> +            if (value & (1 << i)) {

>> +                GIC_CLEAR_ACTIVE(irq + i, 1 << cpu);

>> +            }

>> +        }

>>      } else if (offset < 0x800) {

>>          /* Interrupt Priority.  */

>>          irq = (offset - 0x400) + GIC_BASE_IRQ;

>> --

>> 2.9.3

>

> It looks like we don't implement reads of clear-active correctly

> either.


I'll have a look at that. I'll see if Drew's kvm-unit-tests for the GIC
exercise any of this code ;-)

Thanks for the review.

--
Alex Bennée
Peter Maydell Sept. 5, 2016, 4:30 p.m. UTC | #3 
On 5 September 2016 at 16:45, Alex Bennée <alex.bennee@linaro.org> wrote:
>

> Peter Maydell <peter.maydell@linaro.org> writes:

>

>> On 5 September 2016 at 15:09, Alex Bennée <alex.bennee@linaro.org> wrote:

>>>      } else if (offset < 0x400) {

>>> -        /* Interrupt Active.  */

>>> -        goto bad_reg;

>>> +        /* Interrupt Clear-Active  */

>>> +        irq = (offset - 0x380) * 8 + GIC_BASE_IRQ;

>>> +        if (irq >= s->num_irq)

>>> +            goto bad_reg;

>>

>> This is missing the check against s->revision.

>

> This was deliberate as the GICv1 reference was only mentioned in the

> GICD_ISACTIVERn description. Unfortunately the only documentation for

> GICs on silver.arm.com are for v2 and v3 so I had to guess what v1 had :-/


The GICv2 spec does actually say these registers don't exist
in v1: in the Table 4-1 Distributor register map, the entry
for GICD_ICACTIVERn is marked as having footnote 'e' applying,
which says "GICv2 only".

In the GICv1 spec range 0x380-0x3FC is marked Reserved (ie there
is no clear-active register set at all.) Ditto in the 11MPCore TRM
and the v7M ARM ARM. (The GICv1 register summary also appears in
the A9 MPCore TRM, though that's a bit of an obscure place to
know to look for it.)

As a rule of thumb, if unsure whether something exists in all
configurations, default to leaving it locked down/unimplemented.
We can always widen it later if guest software turns out to use
it, but it's much harder to later know we can remove something
safely once we've let a QEMU into the wild that implemented it.

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index b30cc91..423a4ae 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -972,9 +972,38 @@  static void gic_dist_writeb(void *opaque, hwaddr offset,
                 GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
             }
         }
+    } else if (offset < 0x380) {
+        /* Interrupt Set-Active */
+        irq = (offset - 0x300) * 8 + GIC_BASE_IRQ;
+        if (irq >= s->num_irq || s->revision < 2)
+            goto bad_reg;
+
+        for (i = 0; i < 8; i++) {
+            if (s->security_extn && !attrs.secure &&
+                !GIC_TEST_GROUP(irq + i, 1 << cpu)) {
+                continue; /* Ignore Non-secure access of Group0 IRQ */
+            }
+
+            if (value & (1 << i)) {
+                GIC_SET_ACTIVE(irq + i, 1 << cpu);
+            }
+        }
     } else if (offset < 0x400) {
-        /* Interrupt Active.  */
-        goto bad_reg;
+        /* Interrupt Clear-Active  */
+        irq = (offset - 0x380) * 8 + GIC_BASE_IRQ;
+        if (irq >= s->num_irq)
+            goto bad_reg;
+
+        for (i = 0; i < 8; i++) {
+            if (s->security_extn && !attrs.secure &&
+                !GIC_TEST_GROUP(irq + i, 1 << cpu)) {
+                continue; /* Ignore Non-secure access of Group0 IRQ */
+            }
+
+            if (value & (1 << i)) {
+                GIC_CLEAR_ACTIVE(irq + i, 1 << cpu);
+            }
+        }
     } else if (offset < 0x800) {
         /* Interrupt Priority.  */
         irq = (offset - 0x400) + GIC_BASE_IRQ;