diff mbox

[v2] target-arm: GIC: bug fixes for arm_gic.c

Message ID CAEpva6rMZu31dHDV1kN_XBjC4atJcDK-KxxL35A6XLTwcReySg@mail.gmail.com
State New
Headers show

Commit Message

Daniel Sangorrin Dec. 7, 2012, 8:07 a.m. UTC 
Sorry, it seems that I did not understand the flow in the function
"hw/arm_gic.c:gic_dist_writeb()" and made some mistaken assumptions in
my previous patch. Please do not apply the previous patch, and apply
this one instead if you consider that it is correct.

target-arm: fix bug in irq value on arm_gic.c

Fix a small bug that was using an incorrect IRQ
value in the function gic_dist_writeb.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com>
---
 hw/arm_gic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

                 if (!GIC_TEST_ENABLED(irq + i, cm)) {
@@ -417,7 +417,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,

         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
-                GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
+                GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
             }
         }
     } else if (offset < 0x300) {

Comments

Peter Maydell Dec. 7, 2012, 12:16 p.m. UTC | #1 
On 7 December 2012 08:07, Daniel Sangorrin <daniel.sangorrin@gmail.com> wrote:
> Sorry, it seems that I did not understand the flow in the function
> "hw/arm_gic.c:gic_dist_writeb()" and made some mistaken assumptions in
> my previous patch. Please do not apply the previous patch, and apply
> this one instead if you consider that it is correct.
>
> target-arm: fix bug in irq value on arm_gic.c
>
> Fix a small bug that was using an incorrect IRQ
> value in the function gic_dist_writeb.
>
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com>

Thanks -- nice catch! The code change in this patch is correct,
but there are a minor couple of formatting issues with it.
If you can fix those and resend I can apply it to arm-devs.next.

Firstly, you should send patches as emails which only include
the commit message and patch (this one has a preamble 'Sorry...'
and a quoted copy of your previous email); otherwise when they
are applied this preamble ends up in the git commit log.
Secondly, it would be good to be more specific about the
problem being fixed (something like "Fix a bug where interrupts
not set pending on the correct target CPUs when they are
triggered by writes to the Interrupt Set Enable or Set Pending
registers").

(git format-patch / git send-email send mail in the right
format.)

> ---
>  hw/arm_gic.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm_gic.c b/hw/arm_gic.c
> index f9e423f..64d4e23 100644
> --- a/hw/arm_gic.c
> +++ b/hw/arm_gic.c
> @@ -374,7 +374,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>            value = 0xff;
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
> -                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
> +                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) :
> GIC_TARGET(irq + i);

This line is too long. If you run scripts/checkpatch.pl on your patch
you'll see that it complains about this. (Also, since your mailer
is wrapping long lines, the result is a patch that doesn't apply
cleanly or display properly in patchwork:
http://patchwork.ozlabs.org/patch/204420/
)

I'd suggest a line break after the '='.

>                  int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
>
>                  if (!GIC_TEST_ENABLED(irq + i, cm)) {
> @@ -417,7 +417,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
> -                GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
> +                GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
>              }
>          }
>      } else if (offset < 0x300) {
> --

If you don't have time to make these fixes that's OK -- I can
fix the patch up in my tree. But they're good practice if you
want to send more patches to QEMU in future ;-)

-- PMM
Daniel Sangorrin Dec. 7, 2012, 2:31 p.m. UTC | #2 
Peter,

Thanks for your kind explanation. I will fix the patch and send it
again next Monday (hopefully I won't make any mistake this time). And
yes, I would like to collaborate with more patches in the future so it
might be good practice.

In particular, I'm interested in helping to make current TrustZone
support by Johannes Winter
(https://github.com/jowinter/qemu-trustzone) mainstream. I can
contribute by testing it or adding features that are not yet
implemented.

Best regards
Daniel Sangorrin

On Fri, Dec 7, 2012 at 9:16 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 December 2012 08:07, Daniel Sangorrin <daniel.sangorrin@gmail.com> wrote:
>> Sorry, it seems that I did not understand the flow in the function
>> "hw/arm_gic.c:gic_dist_writeb()" and made some mistaken assumptions in
>> my previous patch. Please do not apply the previous patch, and apply
>> this one instead if you consider that it is correct.
>>
>> target-arm: fix bug in irq value on arm_gic.c
>>
>> Fix a small bug that was using an incorrect IRQ
>> value in the function gic_dist_writeb.
>>
>> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com>
>
> Thanks -- nice catch! The code change in this patch is correct,
> but there are a minor couple of formatting issues with it.
> If you can fix those and resend I can apply it to arm-devs.next.
>
> Firstly, you should send patches as emails which only include
> the commit message and patch (this one has a preamble 'Sorry...'
> and a quoted copy of your previous email); otherwise when they
> are applied this preamble ends up in the git commit log.
> Secondly, it would be good to be more specific about the
> problem being fixed (something like "Fix a bug where interrupts
> not set pending on the correct target CPUs when they are
> triggered by writes to the Interrupt Set Enable or Set Pending
> registers").
>
> (git format-patch / git send-email send mail in the right
> format.)
>
>> ---
>>  hw/arm_gic.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm_gic.c b/hw/arm_gic.c
>> index f9e423f..64d4e23 100644
>> --- a/hw/arm_gic.c
>> +++ b/hw/arm_gic.c
>> @@ -374,7 +374,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>>            value = 0xff;
>>          for (i = 0; i < 8; i++) {
>>              if (value & (1 << i)) {
>> -                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
>> +                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) :
>> GIC_TARGET(irq + i);
>
> This line is too long. If you run scripts/checkpatch.pl on your patch
> you'll see that it complains about this. (Also, since your mailer
> is wrapping long lines, the result is a patch that doesn't apply
> cleanly or display properly in patchwork:
> http://patchwork.ozlabs.org/patch/204420/
> )
>
> I'd suggest a line break after the '='.
>
>>                  int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
>>
>>                  if (!GIC_TEST_ENABLED(irq + i, cm)) {
>> @@ -417,7 +417,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>>
>>          for (i = 0; i < 8; i++) {
>>              if (value & (1 << i)) {
>> -                GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
>> +                GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
>>              }
>>          }
>>      } else if (offset < 0x300) {
>> --
>
> If you don't have time to make these fixes that's OK -- I can
> fix the patch up in my tree. But they're good practice if you
> want to send more patches to QEMU in future ;-)
>
> -- PMM
Peter Maydell Dec. 7, 2012, 2:38 p.m. UTC | #3 
On 7 December 2012 14:31, Daniel Sangorrin <daniel.sangorrin@gmail.com> wrote:
> Thanks for your kind explanation. I will fix the patch and send it
> again next Monday (hopefully I won't make any mistake this time). And
> yes, I would like to collaborate with more patches in the future so it
> might be good practice.

OK, cool.

> In particular, I'm interested in helping to make current TrustZone
> support by Johannes Winter
> (https://github.com/jowinter/qemu-trustzone) mainstream. I can
> contribute by testing it or adding features that are not yet
> implemented.

I'd certainly be interested in seeing proper TrustZone emulation
support upstream if you and Johannes want to get it into shape
to submit, yes.

-- PMM
Johannes Winter Dec. 7, 2012, 3:58 p.m. UTC | #4 
On 07.12.2012 15:38, Peter Maydell wrote:
[...]
> On 7 December 2012 14:31, Daniel Sangorrin <daniel.sangorrin@gmail.com> wrote:
 > [...]
>> In particular, I'm interested in helping to make current TrustZone
>> support by Johannes Winter
>> (https://github.com/jowinter/qemu-trustzone) mainstream. I can
>> contribute by testing it or adding features that are not yet
>> implemented.
>
> I'd certainly be interested in seeing proper TrustZone emulation
> support upstream if you and Johannes want to get it into shape
> to submit, yes.
>
> -- PMM
[...]

I am definitely interested in bringing the TrustZone patches from my 
github fork of QEMU into a shape, which allow integration into upstream. 
Any input, help and feedback on that is highly appreciated.

It would be great to have an outline on how this integration process 
could proceed. From my perspective it seems to be most appropriate to 
first focus on the patches which are immediately related to 
secure/normal world register banking and to the ARM-specific parts of 
QEMU's binary code translator.

Kind regards,
Johannes Winter
Daniel Sangorrin Dec. 20, 2012, 8:44 a.m. UTC | #5 
On Sat, Dec 8, 2012 at 12:58 AM, Johannes Winter
<johannes.winter@iaik.tugraz.at> wrote:
> On 07.12.2012 15:38, Peter Maydell wrote:
> [...]
>
>> On 7 December 2012 14:31, Daniel Sangorrin <daniel.sangorrin@gmail.com>
>> wrote:
>
>> [...]
>
>>> In particular, I'm interested in helping to make current TrustZone
>>> support by Johannes Winter
>>> (https://github.com/jowinter/qemu-trustzone) mainstream. I can
>>> contribute by testing it or adding features that are not yet
>>> implemented.
>>
>>
>> I'd certainly be interested in seeing proper TrustZone emulation
>> support upstream if you and Johannes want to get it into shape
>> to submit, yes.
>>
>> -- PMM
>
> [...]
>
> I am definitely interested in bringing the TrustZone patches from my github
> fork of QEMU into a shape, which allow integration into upstream. Any input,
> help and feedback on that is highly appreciated.
>
> It would be great to have an outline on how this integration process could
> proceed. From my perspective it seems to be most appropriate to first focus
> on the patches which are immediately related to secure/normal world register
> banking and to the ARM-specific parts of QEMU's binary code translator.
>
> Kind regards,
> Johannes Winter

Sorry for being silent during the last days. I was preparing the
porting of our open-source Trustzone monitor (SafeG) to QEMU. The good
news is that it is working and it can be downloaded here (it runs an
RTOS called FMP on the trusted world and Linux on the non-trusted
world). Note: I also have a smaller bare-metal test, which can be
useful for small tests.

http://www.toppers.jp/download.cgi/safeg-0.4-qemu-14dec2012.tar.gz

I agree with you Johannes. Let's start simple. I will try to find time
this month to do something about it.

Best regards,
Daniel
diff mbox

Patch

diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index f9e423f..64d4e23 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -374,7 +374,7 @@  static void gic_dist_writeb(void *opaque, hwaddr offset,
           value = 0xff;
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
-                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
+                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) :
GIC_TARGET(irq + i);
                 int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;