* [Qemu-devel] [PATCH] target-arm: GIC: bug fixes for arm_gic.c
@ 2012-12-07 2:21 Daniel Sangorrin
2012-12-07 5:14 ` Daniel Sangorrin
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Sangorrin @ 2012-12-07 2:21 UTC (permalink / raw)
To: qemu-devel
Hi, I found some bugs in the way the IRQ number is calculated
at certain places in arm_gic.c. Perhaps there are a few more
errors that I didn't notice. These bugs were not noticeable
when running Linux as a guest, but I found them when running
my own porting of a multicore real-time OS (TOPPERS/FMP).
NOTE: the main problem I had was that the target CPUs where not
set correctly. Instead of GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
it should read as GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
I have tested the patch both with FMP and with a recent Linux, but
please review it since I'm not an expert on QEMU and this is my
first patch.
target-arm: bug fixes for arm_gic.c
The IRQ number was not calculated correctly in the
function gic_dist_writeb for accesses to set/clear
pending and set/clear enable.
Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com>
---
hw/arm_gic.c | 90 ++++++++++++++++++++++++++++++++--------------------------
1 file changed, 50 insertions(+), 40 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index f9e423f..f3f233c 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -368,71 +368,81 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
} else if (offset < 0x180) {
/* Interrupt Set Enable. */
irq = (offset - 0x100) * 8 + GIC_BASE_IRQ;
+ for (i = 0; i < 8; i++) {
+ if (value & (1 << i)) {
+ irq = irq + i;
+ break;
+ }
+ }
if (irq >= s->num_irq)
goto bad_reg;
if (irq < 16)
- value = 0xff;
- for (i = 0; i < 8; i++) {
- if (value & (1 << i)) {
- int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
- int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+ value = 0xff;
+ int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
+ int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
- if (!GIC_TEST_ENABLED(irq + i, cm)) {
- DPRINTF("Enabled IRQ %d\n", irq + i);
- }
- GIC_SET_ENABLED(irq + i, cm);
- /* If a raised level triggered IRQ enabled then mark
- is as pending. */
- if (GIC_TEST_LEVEL(irq + i, mask)
- && !GIC_TEST_TRIGGER(irq + i)) {
- DPRINTF("Set %d pending mask %x\n", irq + i, mask);
- GIC_SET_PENDING(irq + i, mask);
- }
- }
+ if (!GIC_TEST_ENABLED(irq, cm)) {
+ DPRINTF("Enabled IRQ %d\n", irq);
+ }
+ GIC_SET_ENABLED(irq, cm);
+ /* If a raised level triggered IRQ enabled then mark is as pending */
+ if (GIC_TEST_LEVEL(irq, mask) && !GIC_TEST_TRIGGER(irq)) {
+ DPRINTF("Set %d pending mask %x\n", irq, mask);
+ GIC_SET_PENDING(irq, mask);
}
} else if (offset < 0x200) {
/* Interrupt Clear Enable. */
irq = (offset - 0x180) * 8 + GIC_BASE_IRQ;
- if (irq >= s->num_irq)
- goto bad_reg;
- if (irq < 16)
- value = 0;
for (i = 0; i < 8; i++) {
if (value & (1 << i)) {
- int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
-
- if (GIC_TEST_ENABLED(irq + i, cm)) {
- DPRINTF("Disabled IRQ %d\n", irq + i);
- }
- GIC_CLEAR_ENABLED(irq + i, cm);
+ irq = irq + i;
+ break;
}
}
- } else if (offset < 0x280) {
- /* Interrupt Set Pending. */
- irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
if (irq >= s->num_irq)
goto bad_reg;
if (irq < 16)
- irq = 0;
+ value = 0;
+
+ int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+ if (GIC_TEST_ENABLED(irq, cm)) {
+ DPRINTF("Disabled IRQ %d\n", irq);
+ }
+ GIC_CLEAR_ENABLED(irq, cm);
+ } else if (offset < 0x280) {
+ /* Interrupt Set Pending. */
+ irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
for (i = 0; i < 8; i++) {
if (value & (1 << i)) {
- GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
+ irq = irq + i;
+ break;
}
}
+
+ if (irq >= s->num_irq) {
+ goto bad_reg;
+ }
+ if (irq < 16) {
+ irq = 0;
+ }
+
+ GIC_SET_PENDING(irq, GIC_TARGET(irq));
} else if (offset < 0x300) {
/* Interrupt Clear Pending. */
irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
- if (irq >= s->num_irq)
- goto bad_reg;
for (i = 0; i < 8; i++) {
- /* ??? This currently clears the pending bit for all CPUs, even
- for per-CPU interrupts. It's unclear whether this is the
- corect behavior. */
- if (value & (1 << i)) {
- GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
- }
+ if (value & (1 << i)) {
+ irq = irq + i;
+ break;
+ }
}
+
+ if (irq >= s->num_irq) {
+ goto bad_reg;
+ }
+
+ GIC_CLEAR_PENDING(irq, ALL_CPU_MASK);
} else if (offset < 0x400) {
/* Interrupt Active. */
goto bad_reg;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH] target-arm: GIC: bug fixes for arm_gic.c
2012-12-07 2:21 [Qemu-devel] [PATCH] target-arm: GIC: bug fixes for arm_gic.c Daniel Sangorrin
@ 2012-12-07 5:14 ` Daniel Sangorrin
2012-12-07 8:07 ` [Qemu-devel] [PATCH v2] " Daniel Sangorrin
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Sangorrin @ 2012-12-07 5:14 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, paul
Hi, I found some bugs in the way the IRQ number is calculated
at certain places in arm_gic.c. Perhaps there are a few more
errors that I didn't notice. These bugs were not noticeable
when running Linux as a guest, but I found them when running
my own porting of a multicore real-time OS (TOPPERS/FMP).
NOTE: the main problem I had was that the target CPUs where not
set correctly. Instead of GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
it should read as GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
I have tested the patch both with FMP and with a recent Linux, but
please review it since I'm not an expert on QEMU and this is my
first patch.
target-arm: bug fixes for arm_gic.c
The IRQ number was not calculated correctly in the
function gic_dist_writeb for accesses to set/clear
pending and set/clear enable.
Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com>
---
hw/arm_gic.c | 90 ++++++++++++++++++++++++++++++++--------------------------
1 file changed, 50 insertions(+), 40 deletions(-)
diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index f9e423f..f3f233c 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -368,71 +368,81 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
} else if (offset < 0x180) {
/* Interrupt Set Enable. */
irq = (offset - 0x100) * 8 + GIC_BASE_IRQ;
+ for (i = 0; i < 8; i++) {
+ if (value & (1 << i)) {
+ irq = irq + i;
+ break;
+ }
+ }
if (irq >= s->num_irq)
goto bad_reg;
if (irq < 16)
- value = 0xff;
- for (i = 0; i < 8; i++) {
- if (value & (1 << i)) {
- int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
- int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+ value = 0xff;
+ int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
+ int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
- if (!GIC_TEST_ENABLED(irq + i, cm)) {
- DPRINTF("Enabled IRQ %d\n", irq + i);
- }
- GIC_SET_ENABLED(irq + i, cm);
- /* If a raised level triggered IRQ enabled then mark
- is as pending. */
- if (GIC_TEST_LEVEL(irq + i, mask)
- && !GIC_TEST_TRIGGER(irq + i)) {
- DPRINTF("Set %d pending mask %x\n", irq + i, mask);
- GIC_SET_PENDING(irq + i, mask);
- }
- }
+ if (!GIC_TEST_ENABLED(irq, cm)) {
+ DPRINTF("Enabled IRQ %d\n", irq);
+ }
+ GIC_SET_ENABLED(irq, cm);
+ /* If a raised level triggered IRQ enabled then mark is as pending */
+ if (GIC_TEST_LEVEL(irq, mask) && !GIC_TEST_TRIGGER(irq)) {
+ DPRINTF("Set %d pending mask %x\n", irq, mask);
+ GIC_SET_PENDING(irq, mask);
}
} else if (offset < 0x200) {
/* Interrupt Clear Enable. */
irq = (offset - 0x180) * 8 + GIC_BASE_IRQ;
- if (irq >= s->num_irq)
- goto bad_reg;
- if (irq < 16)
- value = 0;
for (i = 0; i < 8; i++) {
if (value & (1 << i)) {
- int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
-
- if (GIC_TEST_ENABLED(irq + i, cm)) {
- DPRINTF("Disabled IRQ %d\n", irq + i);
- }
- GIC_CLEAR_ENABLED(irq + i, cm);
+ irq = irq + i;
+ break;
}
}
- } else if (offset < 0x280) {
- /* Interrupt Set Pending. */
- irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
if (irq >= s->num_irq)
goto bad_reg;
if (irq < 16)
- irq = 0;
+ value = 0;
+
+ int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+ if (GIC_TEST_ENABLED(irq, cm)) {
+ DPRINTF("Disabled IRQ %d\n", irq);
+ }
+ GIC_CLEAR_ENABLED(irq, cm);
+ } else if (offset < 0x280) {
+ /* Interrupt Set Pending. */
+ irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
for (i = 0; i < 8; i++) {
if (value & (1 << i)) {
- GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
+ irq = irq + i;
+ break;
}
}
+
+ if (irq >= s->num_irq) {
+ goto bad_reg;
+ }
+ if (irq < 16) {
+ irq = 0;
+ }
+
+ GIC_SET_PENDING(irq, GIC_TARGET(irq));
} else if (offset < 0x300) {
/* Interrupt Clear Pending. */
irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
- if (irq >= s->num_irq)
- goto bad_reg;
for (i = 0; i < 8; i++) {
- /* ??? This currently clears the pending bit for all CPUs, even
- for per-CPU interrupts. It's unclear whether this is the
- corect behavior. */
- if (value & (1 << i)) {
- GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
- }
+ if (value & (1 << i)) {
+ irq = irq + i;
+ break;
+ }
}
+
+ if (irq >= s->num_irq) {
+ goto bad_reg;
+ }
+
+ GIC_CLEAR_PENDING(irq, ALL_CPU_MASK);
} else if (offset < 0x400) {
/* Interrupt Active. */
goto bad_reg;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v2] target-arm: GIC: bug fixes for arm_gic.c
2012-12-07 5:14 ` Daniel Sangorrin
@ 2012-12-07 8:07 ` Daniel Sangorrin
2012-12-07 12:16 ` Peter Maydell
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Sangorrin @ 2012-12-07 8:07 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, paul
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(-)
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;
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) {
--
1.7.9.5
On Fri, Dec 7, 2012 at 2:14 PM, Daniel Sangorrin
<daniel.sangorrin@gmail.com> wrote:
> Hi, I found some bugs in the way the IRQ number is calculated
> at certain places in arm_gic.c. Perhaps there are a few more
> errors that I didn't notice. These bugs were not noticeable
> when running Linux as a guest, but I found them when running
> my own porting of a multicore real-time OS (TOPPERS/FMP).
>
> NOTE: the main problem I had was that the target CPUs where not
> set correctly. Instead of GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
> it should read as GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
>
> I have tested the patch both with FMP and with a recent Linux, but
> please review it since I'm not an expert on QEMU and this is my
> first patch.
>
> target-arm: bug fixes for arm_gic.c
>
> The IRQ number was not calculated correctly in the
> function gic_dist_writeb for accesses to set/clear
> pending and set/clear enable.
>
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com>
> ---
> hw/arm_gic.c | 90 ++++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 50 insertions(+), 40 deletions(-)
>
> diff --git a/hw/arm_gic.c b/hw/arm_gic.c
> index f9e423f..f3f233c 100644
> --- a/hw/arm_gic.c
> +++ b/hw/arm_gic.c
> @@ -368,71 +368,81 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
> } else if (offset < 0x180) {
> /* Interrupt Set Enable. */
> irq = (offset - 0x100) * 8 + GIC_BASE_IRQ;
> + for (i = 0; i < 8; i++) {
> + if (value & (1 << i)) {
> + irq = irq + i;
> + break;
> + }
> + }
> if (irq >= s->num_irq)
> goto bad_reg;
> if (irq < 16)
> - value = 0xff;
> - for (i = 0; i < 8; i++) {
> - if (value & (1 << i)) {
> - int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
> - int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> + value = 0xff;
> + int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
> + int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
>
> - if (!GIC_TEST_ENABLED(irq + i, cm)) {
> - DPRINTF("Enabled IRQ %d\n", irq + i);
> - }
> - GIC_SET_ENABLED(irq + i, cm);
> - /* If a raised level triggered IRQ enabled then mark
> - is as pending. */
> - if (GIC_TEST_LEVEL(irq + i, mask)
> - && !GIC_TEST_TRIGGER(irq + i)) {
> - DPRINTF("Set %d pending mask %x\n", irq + i, mask);
> - GIC_SET_PENDING(irq + i, mask);
> - }
> - }
> + if (!GIC_TEST_ENABLED(irq, cm)) {
> + DPRINTF("Enabled IRQ %d\n", irq);
> + }
> + GIC_SET_ENABLED(irq, cm);
> + /* If a raised level triggered IRQ enabled then mark is as pending */
> + if (GIC_TEST_LEVEL(irq, mask) && !GIC_TEST_TRIGGER(irq)) {
> + DPRINTF("Set %d pending mask %x\n", irq, mask);
> + GIC_SET_PENDING(irq, mask);
> }
> } else if (offset < 0x200) {
> /* Interrupt Clear Enable. */
> irq = (offset - 0x180) * 8 + GIC_BASE_IRQ;
> - if (irq >= s->num_irq)
> - goto bad_reg;
> - if (irq < 16)
> - value = 0;
> for (i = 0; i < 8; i++) {
> if (value & (1 << i)) {
> - int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> -
> - if (GIC_TEST_ENABLED(irq + i, cm)) {
> - DPRINTF("Disabled IRQ %d\n", irq + i);
> - }
> - GIC_CLEAR_ENABLED(irq + i, cm);
> + irq = irq + i;
> + break;
> }
> }
> - } else if (offset < 0x280) {
> - /* Interrupt Set Pending. */
> - irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
> if (irq >= s->num_irq)
> goto bad_reg;
> if (irq < 16)
> - irq = 0;
> + value = 0;
> +
> + int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
>
> + if (GIC_TEST_ENABLED(irq, cm)) {
> + DPRINTF("Disabled IRQ %d\n", irq);
> + }
> + GIC_CLEAR_ENABLED(irq, cm);
> + } else if (offset < 0x280) {
> + /* Interrupt Set Pending. */
> + irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
> for (i = 0; i < 8; i++) {
> if (value & (1 << i)) {
> - GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
> + irq = irq + i;
> + break;
> }
> }
> +
> + if (irq >= s->num_irq) {
> + goto bad_reg;
> + }
> + if (irq < 16) {
> + irq = 0;
> + }
> +
> + GIC_SET_PENDING(irq, GIC_TARGET(irq));
> } else if (offset < 0x300) {
> /* Interrupt Clear Pending. */
> irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
> - if (irq >= s->num_irq)
> - goto bad_reg;
> for (i = 0; i < 8; i++) {
> - /* ??? This currently clears the pending bit for all CPUs, even
> - for per-CPU interrupts. It's unclear whether this is the
> - corect behavior. */
> - if (value & (1 << i)) {
> - GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
> - }
> + if (value & (1 << i)) {
> + irq = irq + i;
> + break;
> + }
> }
> +
> + if (irq >= s->num_irq) {
> + goto bad_reg;
> + }
> +
> + GIC_CLEAR_PENDING(irq, ALL_CPU_MASK);
> } else if (offset < 0x400) {
> /* Interrupt Active. */
> goto bad_reg;
> --
> 1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-arm: GIC: bug fixes for arm_gic.c
2012-12-07 8:07 ` [Qemu-devel] [PATCH v2] " Daniel Sangorrin
@ 2012-12-07 12:16 ` Peter Maydell
2012-12-07 14:31 ` Daniel Sangorrin
0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2012-12-07 12:16 UTC (permalink / raw)
To: Daniel Sangorrin; +Cc: qemu-devel, paul
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-arm: GIC: bug fixes for arm_gic.c
2012-12-07 12:16 ` Peter Maydell
@ 2012-12-07 14:31 ` Daniel Sangorrin
2012-12-07 14:38 ` Peter Maydell
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Sangorrin @ 2012-12-07 14:31 UTC (permalink / raw)
To: Peter Maydell; +Cc: johannes.winter, qemu-devel, paul
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-arm: GIC: bug fixes for arm_gic.c
2012-12-07 14:31 ` Daniel Sangorrin
@ 2012-12-07 14:38 ` Peter Maydell
2012-12-07 15:58 ` Johannes Winter
0 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2012-12-07 14:38 UTC (permalink / raw)
To: Daniel Sangorrin; +Cc: johannes.winter, qemu-devel, paul
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-arm: GIC: bug fixes for arm_gic.c
2012-12-07 14:38 ` Peter Maydell
@ 2012-12-07 15:58 ` Johannes Winter
2012-12-20 8:44 ` Daniel Sangorrin
0 siblings, 1 reply; 8+ messages in thread
From: Johannes Winter @ 2012-12-07 15:58 UTC (permalink / raw)
To: Peter Maydell; +Cc: Daniel Sangorrin, qemu-devel, paul
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2] target-arm: GIC: bug fixes for arm_gic.c
2012-12-07 15:58 ` Johannes Winter
@ 2012-12-20 8:44 ` Daniel Sangorrin
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Sangorrin @ 2012-12-20 8:44 UTC (permalink / raw)
To: Johannes Winter; +Cc: Peter Maydell, qemu-devel, paul
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-12-20 8:45 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-07 2:21 [Qemu-devel] [PATCH] target-arm: GIC: bug fixes for arm_gic.c Daniel Sangorrin
2012-12-07 5:14 ` Daniel Sangorrin
2012-12-07 8:07 ` [Qemu-devel] [PATCH v2] " Daniel Sangorrin
2012-12-07 12:16 ` Peter Maydell
2012-12-07 14:31 ` Daniel Sangorrin
2012-12-07 14:38 ` Peter Maydell
2012-12-07 15:58 ` Johannes Winter
2012-12-20 8:44 ` Daniel Sangorrin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).