[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] pl190: fix read of VECTADDR
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH] pl190: fix read of VECTADDR |
Date: |
Sat, 18 Aug 2012 13:20:48 +0100 |
On 18 August 2012 11:41, Brendan Fennell <address@hidden> wrote:
>
>
> On Sat, 18 Aug 2012, Peter Maydell wrote:
>
>> On 18 August 2012 03:55, Brendan Fennell <address@hidden> wrote:
>>>
>>> Signed-off-by: Brendan Fennell <address@hidden>
>>> ---
>>> 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 :-)
-- PMM