Re: [Qemu-devel] [PATCH v2] target-arm: GIC: bug fixes for arm_gic.c

[Daniel Sangorrin <daniel.sangorrin@gmail.com>]

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