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

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

[Brendan Fennell]

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

[Blue Swirl]

On Sun, Aug 19, 2012 at 10:59 AM, Brendan Fennell <bfennell@skynet.ie> wrote:
>
> Signed-off-by: Brendan Fennell <bfennell@skynet.ie>
> ---
>  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])

Missing braces and spaces around '+', please read CODING_STYLE.

>                break;
>            }
>          /* Reading this value with no pending interrupts is undefined.
> --
> 1.7.2.5
>
>

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

[Peter Maydell]

On 19 August 2012 11:59, Brendan Fennell <bfennell@skynet.ie> wrote:
>
> Signed-off-by: Brendan Fennell <bfennell@skynet.ie>
> ---
>  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.)

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.
         */

thanks
-- PMM

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

[Brendan Fennell]

On Mon, 20 Aug 2012, Peter Maydell wrote:

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