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.sangor...@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.sangor...@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.sangor...@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
