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

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

[Alex Bennée]

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(-)

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;
-- 
2.9.3

Re: [Qemu-devel] [RFC PATCH] hw/intc/arm_gic: handle Set-Active/Clear-Active registers

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [RFC PATCH] hw/intc/arm_gic: handle Set-Active/Clear-Active registers
Type: series
Message-id: 20160905140944.5379-1-alex.bennee@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c424725 hw/intc/arm_gic: handle Set-Active/Clear-Active registers

=== OUTPUT BEGIN ===
Checking PATCH 1/1: hw/intc/arm_gic: handle Set-Active/Clear-Active registers...
ERROR: braces {} are necessary for all arms of this statement
#29: FILE: hw/intc/arm_gic.c:978:
+        if (irq >= s->num_irq || s->revision < 2)
[...]

ERROR: braces {} are necessary for all arms of this statement
#47: FILE: hw/intc/arm_gic.c:994:
+        if (irq >= s->num_irq)
[...]

total: 2 errors, 0 warnings, 40 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

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

[Peter Maydell]

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

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

[Alex Bennée]

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

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

[Peter Maydell]

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