[Qemu-devel] [PATCH] pl190: fix read of VECTADDR

[Qemu-devel] [PATCH] pl190: fix read of VECTADDR

[Brendan Fennell]

Signed-off-by: Brendan Fennell <bfennell@skynet.ie>
---
 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

[Peter Maydell]

On 18 August 2012 03:55, Brendan Fennell <bfennell@skynet.ie> wrote:
> Signed-off-by: Brendan Fennell <bfennell@skynet.ie>
> ---
>  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?

-- PMM

Re: [Qemu-devel] [PATCH] pl190: fix read of VECTADDR

[Brendan Fennell]

On Sat, 18 Aug 2012, Peter Maydell wrote:

> On 18 August 2012 03:55, Brendan Fennell <bfennell@skynet.ie> wrote:
>> Signed-off-by: Brendan Fennell <bfennell@skynet.ie>
>> ---
>>  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

[Peter Maydell]

On 18 August 2012 11:41, Brendan Fennell <bfennell@skynet.ie> wrote:
>
>
> On Sat, 18 Aug 2012, Peter Maydell wrote:
>
>> On 18 August 2012 03:55, Brendan Fennell <bfennell@skynet.ie> wrote:
>>>
>>> Signed-off-by: Brendan Fennell <bfennell@skynet.ie>
>>> ---
>>>  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

Re: [Qemu-devel] [PATCH] pl190: fix read of VECTADDR

[Brendan Fennell]

On Sat, 18 Aug 2012, Peter Maydell wrote:

> On 18 August 2012 11:41, Brendan Fennell <bfennell@skynet.ie> wrote:
>>
>>
>> On Sat, 18 Aug 2012, Peter Maydell wrote:
>>
>>> On 18 August 2012 03:55, Brendan Fennell <bfennell@skynet.ie> wrote:
>>>>
>>>> Signed-off-by: Brendan Fennell <bfennell@skynet.ie>
>>>> ---
>>>>  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
>
>