[Qemu-devel] [PATCH] pl190: fix read of VECTADDR
Signed-off-by: Brendan Fennell --- hw/pl190.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/hw/pl190.c b/hw/pl190.c index cb50afb..d69d5be 100644 --- a/hw/pl190.c +++ b/hw/pl190.c @@ -133,7 +133,7 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t offset, s->priority = i; pl190_update(s); } -return s->vect_addr[s->priority]; +return s->vect_addr[s->priority - 1]; case 13: /* DEFVECTADDR */ return s->vect_addr[16]; default: -- 1.7.2.5
Re: [Qemu-devel] [PATCH] pl190: fix read of VECTADDR
On Sat, 18 Aug 2012, Peter Maydell wrote: On 18 August 2012 03:55, Brendan Fennell wrote: Signed-off-by: Brendan Fennell --- hw/pl190.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/hw/pl190.c b/hw/pl190.c index cb50afb..d69d5be 100644 --- a/hw/pl190.c +++ b/hw/pl190.c @@ -133,7 +133,7 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t offset, s->priority = i; pl190_update(s); } -return s->vect_addr[s->priority]; +return s->vect_addr[s->priority - 1]; case 13: /* DEFVECTADDR */ return s->vect_addr[16]; default: This doesn't look right -- if s->priority is zero then we'll read off the beginning of the array. What's the actual bug you're trying to fix here? The bug is that when, for example, interrupt 4 triggers the VECTADDR of interrupt 5 is returned by pl190_read(). Each s->prio_mask entry contains the interrupt mask for all *higher* priority interrupts, see pl190_update_vectors(). This means that s->prio_mask[0] is always zero (as zero is the highest priority), s->priority can never be zero as ((s->level | s->soft_level) & s->prio_mask[0]) is always zero. Therefore after the for loop in pl190_read() i is the index of the current highest priority interrupt + 1. Brendan. -- PMM
Re: [Qemu-devel] [PATCH] pl190: fix read of VECTADDR
On Sat, 18 Aug 2012, Peter Maydell wrote: On 18 August 2012 11:41, Brendan Fennell wrote: On Sat, 18 Aug 2012, Peter Maydell wrote: On 18 August 2012 03:55, Brendan Fennell wrote: Signed-off-by: Brendan Fennell --- hw/pl190.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/hw/pl190.c b/hw/pl190.c index cb50afb..d69d5be 100644 --- a/hw/pl190.c +++ b/hw/pl190.c @@ -133,7 +133,7 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t offset, s->priority = i; pl190_update(s); } -return s->vect_addr[s->priority]; +return s->vect_addr[s->priority - 1]; case 13: /* DEFVECTADDR */ return s->vect_addr[16]; default: This doesn't look right -- if s->priority is zero then we'll read off the beginning of the array. What's the actual bug you're trying to fix here? The bug is that when, for example, interrupt 4 triggers the VECTADDR of interrupt 5 is returned by pl190_read(). Each s->prio_mask entry contains the interrupt mask for all *higher* priority interrupts, see pl190_update_vectors(). This means that s->prio_mask[0] is always zero (as zero is the highest priority), s->priority can never be zero as ((s->level | s->soft_level) & s->prio_mask[0]) is always zero. Therefore after the for loop in pl190_read() i is the index of the current highest priority interrupt + 1. Yes, looking more closely, you're right (though that's not obvious at all...) But we set s->priority to i, which seems wrong -- s->priority should be the priority of the current active interrupt, and that's how we treat it in pl190_update() [we assert s->irq if there's a pending interrupt that's higher priority than the one we're currently servicing.] So I think the fix ought to be to change the s->prio_mask[i] in the loop to be s->prio_mask[i+1] instead. Then we'll exit the loop with i as the current highest priority interrupt, which is what the following code expects. Some sort of explanatory comment in the loop might also assist future readers :-) I agree, that's a better solution - I'll follow up with a new patch. Brendan. -- PMM
[Qemu-devel] [PATCH v2] pl190: fix read of VECTADDR
Signed-off-by: Brendan Fennell
---
hw/pl190.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/hw/pl190.c b/hw/pl190.c
index cb50afb..eddb531 100644
--- a/hw/pl190.c
+++ b/hw/pl190.c
@@ -120,7 +120,8 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t
offset,
current priority level to that of the current interrupt. */
for (i = 0; i < s->priority; i++)
{
-if ((s->level | s->soft_level) & s->prio_mask[i])
+/* Ensure that 'i' is current highest priority interrupt on exit */
+if ((s->level | s->soft_level) & s->prio_mask[i+1])
break;
}
/* Reading this value with no pending interrupts is undefined.
--
1.7.2.5
Re: [Qemu-devel] [PATCH v2] pl190: fix read of VECTADDR
On Mon, 20 Aug 2012, Peter Maydell wrote:
On 19 August 2012 11:59, Brendan Fennell wrote:
Signed-off-by: Brendan Fennell
---
hw/pl190.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/hw/pl190.c b/hw/pl190.c
index cb50afb..eddb531 100644
--- a/hw/pl190.c
+++ b/hw/pl190.c
@@ -120,7 +120,8 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t
offset,
current priority level to that of the current interrupt. */
for (i = 0; i < s->priority; i++)
{
-if ((s->level | s->soft_level) & s->prio_mask[i])
+/* Ensure that 'i' is current highest priority interrupt on exit */
+if ((s->level | s->soft_level) & s->prio_mask[i+1])
break;
}
/* Reading this value with no pending interrupts is undefined.
--
1.7.2.5
The technical content of this patch looks correct to me, and I've tested
it on a versatilepb Linux image. (Presumably Linux doesn't make use
of different vector addresses/priorities, which is why we haven't noticed
this bug before now.)
As Blue says, you need to fix the coding style issues (you can run
your patch through scripts/checkpatch.pl to help with this). checkpatch
is probably going to end up getting you to fix the indent on the
whole for() loop, which is fine -- we usually fix up the coding style
locally when we make a change. (the key bits of coding style here are
4 space indent, open-brace on same line as 'for' and 'if', braces
mandatory even for single line 'if' bodies.)
Thanks for the helpful feedback - checkpatch.pl does indeed point out the
coding style problems in the patch.
I also think we could improve on the comment text. Here's my
suggestion (replaces the current just-above-the-loop comment):
/* Read vector address at the start of an ISR. Increases the
* current priority level to that of the current interrupt.
*
* Since an enabled interrupt X at priority P causes prio_mask[Y]
* to have bit X set for all Y > P, this loop will stop with
* i == the priority of the highest priority set interrupt.
*/
That explains the situation more clearly, thanks. V3 to follow.
thanks
-- PMM
[Qemu-devel] [PATCH v3] pl190: fix read of VECTADDR
Signed-off-by: Brendan Fennell
---
hw/pl190.c | 18 --
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/hw/pl190.c b/hw/pl190.c
index cb50afb..b372da8 100644
--- a/hw/pl190.c
+++ b/hw/pl190.c
@@ -117,12 +117,18 @@ static uint64_t pl190_read(void *opaque,
target_phys_addr_t offset,
return s->protected;
case 12: /* VECTADDR */
/* Read vector address at the start of an ISR. Increases the
- current priority level to that of the current interrupt. */
-for (i = 0; i < s->priority; i++)
- {
-if ((s->level | s->soft_level) & s->prio_mask[i])
- break;
- }
+ * current priority level to that of the current interrupt.
+ *
+ * Since an enabled interrupt X at priority P causes prio_mask[Y]
+ * to have bit X set for all Y > P, this loop will stop with
+ * i == the priority of the highest priority set interrupt.
+ */
+for (i = 0; i < s->priority; i++) {
+if ((s->level | s->soft_level) & s->prio_mask[(i + 1)]) {
+break;
+}
+}
+
/* Reading this value with no pending interrupts is undefined.
We return the default address. */
if (i == PL190_NUM_PRIO)
--
1.7.2.5
Re: [Qemu-devel] [PATCH v3] pl190: fix read of VECTADDR
On Mon, 20 Aug 2012, Peter Maydell wrote: On 20 August 2012 18:59, Brendan Fennell wrote: Signed-off-by: Brendan Fennell Reviewed-by: Peter Maydell The () inside the [] aren't actually necessary, but I'm just going to fix those as I take this patch into the arm-devs queue, I think. I've also expanded the commit message a little: = Reading VECTADDR was causing us to set the current priority to the wrong value, the most obvious effect of which was that we would return the vector for the wrong interrupt as the result of the read. = What guest did you see this problem with? Since the QEMU 1.2 release is now less than 2 weeks away, and this bug has been present since the versatilepb platform was added 6 years ago, I'm reluctant to squeeze it into this release unless it's going to be hit by a lot of people. (1.3 is only 3 months later so it's not a huge deal for a minor fix to miss 1.2.) I'm working on a custom application, not a standard guest OS. I think it's safe to say this can wait for 1.3. Brendan. thanks -- PMM
