[Qemu-devel] [PATCH] pl190: Fix off-by-one error in priority handling when reading VECTADDR

[Qemu-devel] [PATCH] pl190: Fix off-by-one error in priority handling when reading VECTADDR

[Marc Bommert]

The "current" priority bit (1 << i) should also be set in s->prio_mask[i], if the interrupt is enabled. This will in turn cause the read operation of VECTADDR to return the correct vector of the pending interrupt.

---
hw/intc/pl190.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/pl190.c b/hw/intc/pl190.c
index 55ea15d..0369da8 100644
--- a/hw/intc/pl190.c
+++ b/hw/intc/pl190.c
@@ -80,12 +80,12 @@ static void pl190_update_vectors(PL190State *s)
mask = 0;
for (i = 0; i < 16; i++)
{
- s->prio_mask[i] = mask;
if (s->vect_control[i] & 0x20)
{
n = s->vect_control[i] & 0x1f;
mask |= 1 << n;
}
+ s->prio_mask[i] = mask;
}
s->prio_mask[16] = mask;
pl190_update(s);
--
2.5.0

Re: [Qemu-devel] [PATCH] pl190: Fix off-by-one error in priority handling when reading VECTADDR

[Peter Maydell]

On 27 February 2017 at 08:15, Marc Bommert <marc@brightwise.de> wrote:
> The "current" priority bit (1 << i) should also be set in
> s->prio_mask[i], if the interrupt is enabled. This will in turn
> cause the read operation of VECTADDR to return the correct vector
> of the pending interrupt.
>
> ---
> hw/intc/pl190.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/pl190.c b/hw/intc/pl190.c
> index 55ea15d..0369da8 100644
> --- a/hw/intc/pl190.c
> +++ b/hw/intc/pl190.c
> @@ -80,12 +80,12 @@ static void pl190_update_vectors(PL190State *s)
> mask = 0;
> for (i = 0; i < 16; i++)
> {
> - s->prio_mask[i] = mask;
> if (s->vect_control[i] & 0x20)
> {
> n = s->vect_control[i] & 0x1f;
> mask |= 1 << n;
> }
> + s->prio_mask[i] = mask;
> }
> s->prio_mask[16] = mask;
> pl190_update(s);

(Your email client seems to have removed all the leading spaces
from your patch, which makes it a bit tricky to read.)

The comment in pl190_read() about VECTADDR says
"an enabled interrupt X at priority P causes prio_mask[Y]
 to have bit X set for all Y > P", but your patch would
make that not be true.

I'm not very familiar with the PL190, but looking at pl190_update()
I think your proposed change will make us set the outbound IRQ
line incorrectly.

Suppose that only the interrupt programmed into VECTCNTL[0]
and VECTADDR[0] is active. We will initially set the IRQ line
(since s->priority is 17 and s->prio_mask[17] is 0xffffffff).
However when the guest reads the VICVectAddr register we want
to cause the IRQ line to no longer be asserted. (The PL190 TRM
states this in the "Interrupt priority logic" section:
"The highest priority request generates an IRQ interrupt if
the interrupt is not currently being serviced.") In the current
code this works because we will set s->priority to 0, and
s->prio_mask[0] is 0 and so the "level & s->prio_mask[s->priority]"
masking in pl190_update() clears the bit and we won't assert
the IRQ line. With your change, s->prio_mask[0] would have the
bit set for the interrupt number, and so the masking would
leave that bit set and leave IRQ asserted.

It also looks to me like this change breaks the logic for
reading VECTADDR, because instead of the loop stopping
with i as the priority of the highest set not-being-serviced
interrupt it will stop with i being one less than that.
(For instance if the interrupt for priority 2 is the highest
set not-being-serviced interrupt then with your change
we'll now set s->prio_mask[2] to have the relevant bit set,
which means the loop will terminate with i == 1, and we return
the vector address for interrupt priority 1, not 2 as we should.

Perhaps I'm missing something -- what's the wrong behaviour
that you're seeing that you're trying to fix ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] pl190: Fix off-by-one error in priority handling when reading VECTADDR

[Marc Bommert]

> Peter Maydell <peter.maydell@linaro.org> hat am 27. Februar 2017 um 15:07 geschrieben:
> 
> The comment in pl190_read() about VECTADDR says
> "an enabled interrupt X at priority P causes prio_mask[Y]
>  to have bit X set for all Y > P", but your patch would
> make that not be true.

Sorry, of course, the comment has to be modified: "... for all Y>=P"

> I'm not very familiar with the PL190, but looking at pl190_update()
> I think your proposed change will make us set the outbound IRQ
> line incorrectly.
> 
> Suppose that only the interrupt programmed into VECTCNTL[0]
> and VECTADDR[0] is active. We will initially set the IRQ line
> (since s->priority is 17 and s->prio_mask[17] is 0xffffffff).
> However when the guest reads the VICVectAddr register we want
> to cause the IRQ line to no longer be asserted. (The PL190 TRM
> states this in the "Interrupt priority logic" section:
> "The highest priority request generates an IRQ interrupt if
> the interrupt is not currently being serviced.") In the current
> code this works because we will set s->priority to 0, and
> s->prio_mask[0] is 0 and so the "level & s->prio_mask[s->priority]"
> masking in pl190_update() clears the bit and we won't assert
> the IRQ line. With your change, s->prio_mask[0] would have the
> bit set for the interrupt number, and so the masking would
> leave that bit set and leave IRQ asserted.

Where did you get the information that "when the guest reads the VICVectAddr register we want
to cause the IRQ line to no longer be asserted." Imho, the _write_ to vectaddr clears the interrupt request.

> It also looks to me like this change breaks the logic for
> reading VECTADDR, because instead of the loop stopping
> with i as the priority of the highest set not-being-serviced
> interrupt it will stop with i being one less than that.
> (For instance if the interrupt for priority 2 is the highest
> set not-being-serviced interrupt then with your change
> we'll now set s->prio_mask[2] to have the relevant bit set,
> which means the loop will terminate with i == 1, and we return
> the vector address for interrupt priority 1, not 2 as we should.

You're right that it changes the logic here, this is the actual fix. I don't understand why you want to deliver the "highest set not-being serviced interrupt vector". In fact, you have to deliver the currently active IRQ vector address. The PL190 TRM states for the VECTADDR register:

"The read/write VICVECTADDR Register, with address offset of 0x030, contains the Interrupt Service Routine (ISR) address of the currently active interrupt"

> Perhaps I'm missing something -- what's the wrong behaviour
> that you're seeing that you're trying to fix ?

I have a RTOS running which up to now only ran on real hardware. It performs a sequence of accesses to the PL190 as stated in the bug ticket. When reading back VECTADDR of the currently handled interrupt (interrupt source 1 mapped to vector 2), the PL190 emulation delivers VECTADDR3 instead and my OS fails to dispatch the interrupt to the proper handler, but counts a spurious interrupt.

To be more precise, the "current priority" s->priority is not raised to the priority of the currently handled interrupt, but to the subsequent priority in the priority range.

Maybe I can explain it in more detail later, I'm not at the code atm. Well, I'm quite sure about that fix, since I investigated about a day for this single line patch.

Thanks for your support.
Marc

Re: [Qemu-devel] [PATCH] pl190: Fix off-by-one error in priority handling when reading VECTADDR

[Peter Maydell]

On 27 February 2017 at 14:35, Marc Bommert <marc@brightwise.de> wrote:
>> Peter Maydell <peter.maydell@linaro.org> hat am 27. Februar 2017 um 15:07 geschrieben:
>> Suppose that only the interrupt programmed into VECTCNTL[0]
>> and VECTADDR[0] is active. We will initially set the IRQ line
>> (since s->priority is 17 and s->prio_mask[17] is 0xffffffff).
>> However when the guest reads the VICVectAddr register we want
>> to cause the IRQ line to no longer be asserted. (The PL190 TRM
>> states this in the "Interrupt priority logic" section:
>> "The highest priority request generates an IRQ interrupt if
>> the interrupt is not currently being serviced.") In the current
>> code this works because we will set s->priority to 0, and
>> s->prio_mask[0] is 0 and so the "level & s->prio_mask[s->priority]"
>> masking in pl190_update() clears the bit and we won't assert
>> the IRQ line. With your change, s->prio_mask[0] would have the
>> bit set for the interrupt number, and so the masking would
>> leave that bit set and leave IRQ asserted.
>
> Where did you get the information that "when the guest reads
> the VICVectAddr register we want  to cause the IRQ line to
> no longer be asserted." Imho, the _write_ to vectaddr clears
> the interrupt request.

See my quote from the PL190 TRM above. Also I think it's
clear from the TRM's suggested vectored interrupt flow sequence
which is:
 1  interrupt occurs
 2  CPU takes IRQ
 3  interrupt handler reads VICVectAddr
 4  interrupt handler stacks CPU registers
 5  interrupt handler reenables IRQ interrupts
 6  do the ISR work
 7  clear peripheral interrupt signal
 8  disable ISR, restore regs
 9  write VICVectAddr to clear interrupt in the PL190
 10 return from interrupt (reenabling IRQ)

If the PL190 continues to signal IRQ for an interrupt
that has had VICVectAddr read, then this sequence will
recurse infinitely because as the CPU reenables IRQ in
step 5 we'll take the IRQ interrupt again. The logic relies
on the PL190 not signalling IRQ once it's been acknowledged
via the VICVectAddr read (unless a higher priority interrupt
arrives, which should cause IRQ to be asserted so the guest
can recursively handle it).

There's also this text in the "About the VIC" section 2.1:
"Reading from the Vector Interrupt Address Register, VICVECTADDR,
provides the address of the ISR, and updates the interrupt
priority hardware that masks out the current, and any lower
priority interrupt requests."
 -- which definitely says that the read causes the current
interrupt to be masked out.

>> It also looks to me like this change breaks the logic for
>> reading VECTADDR, because instead of the loop stopping
>> with i as the priority of the highest set not-being-serviced
>> interrupt it will stop with i being one less than that.
>> (For instance if the interrupt for priority 2 is the highest
>> set not-being-serviced interrupt then with your change
>> we'll now set s->prio_mask[2] to have the relevant bit set,
>> which means the loop will terminate with i == 1, and we return
>> the vector address for interrupt priority 1, not 2 as we should.
>
> You're right that it changes the logic here, this is the actual
> fix. I don't understand why you want to deliver the "highest set
> not-being serviced interrupt vector". In fact, you have to
> deliver the currently active IRQ vector address. The PL190 TRM
> states for the VECTADDR register:
>
> "The read/write VICVECTADDR Register, with address offset of
> 0x030, contains the Interrupt Service Routine (ISR) address
> of the currently active interrupt"

At the point where the ISR reads VECTADDR, the currently active
interrupt *is* the highest set not being serviced interrupt vector.
(If your ISR reads it twice then that's a bad idea because
the Note in the TRM says "reading [...] at other times can
cause incorrect operation", but QEMU's implementation will
return the same value again until a higher priority interrupt
comes in or the interrupt is dismissed by writing to VECTADDR,
since the 2nd read won't change s->priority and we'll just
return s->vect_addr[s->priority] again.)

>> Perhaps I'm missing something -- what's the wrong behaviour
>> that you're seeing that you're trying to fix ?
>
> I have a RTOS running which up to now only ran on real hardware.
> It performs a sequence of accesses to the PL190 as stated in
> the bug ticket. When reading back VECTADDR of the currently
> handled interrupt (interrupt source 1 mapped to vector 2),
> the PL190 emulation delivers VECTADDR3 instead and my OS fails
> to dispatch the interrupt to the proper handler, but counts
> a spurious interrupt.

Ah, I missed that the bug ticket had a sequence of actions:

 1) Write INTENCLEAR to clear all interrupt enable bits
 2) Set all 16 vector control registers to zero
 3) Set vector address #2 to value 2
 4) Set vector control #2 to 0x21 (vector_interrupt_enable(0x20) |
vector_interrupt_source(0x1) )
 5) Enable interrupt 1 by writing 0x2 to INTENABLE
 6) In interrupt handler: read VECTADDR [should read 0x2
   (active IRQs vector address as set in step 3), reads 0x0
   (active vector address index 3 instead of index 2)]

I don't see why step 6 gives you index 3, though. At that
point s->priority should be 17, s->prio_mask[0] = 0,
s->prio_mask[1] = 0, s->prio_mask[2] = 0, s->prio_mask[3] = 1 << 1,
etc. s->level is 1 << 1.
The loop
        for (i = 0; i < s->priority; i++) {
            if ((s->level | s->soft_level) & s->prio_mask[i + 1]) {
                break;
            }
        }
will terminate with i == 2 (since s->prio_mask[3] is
the first one with a set bit matching level). We then
set s->priority to 2 and return the vect_addr[2] entry.

> To be more precise, the "current priority" s->priority is not
> raised to the priority of the currently handled interrupt, but
> to the subsequent priority in the priority range.

I don't see how this happens in the code: we stop the
loop with i one less than the s->prio_mask[] array
entry we check, because of the i+1 in the array index.

If you have some guest code I could reproduce this with
that would be helpful.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] pl190: Fix off-by-one error in priority handling when reading VECTADDR

[Marc Bommert]

> Peter Maydell <peter.maydell@linaro.org> hat am 27. Februar 2017 um 16:27 geschrieben:
> 
> 
> On 27 February 2017 at 14:35, Marc Bommert <marc@brightwise.de> wrote:
> >> Peter Maydell <peter.maydell@linaro.org> hat am 27. Februar 2017 um 15:07 geschrieben:
> >> Suppose that only the interrupt programmed into VECTCNTL[0]
> >> and VECTADDR[0] is active. We will initially set the IRQ line
> >> (since s->priority is 17 and s->prio_mask[17] is 0xffffffff).
> >> However when the guest reads the VICVectAddr register we want
> >> to cause the IRQ line to no longer be asserted. (The PL190 TRM
> >> states this in the "Interrupt priority logic" section:
> >> "The highest priority request generates an IRQ interrupt if
> >> the interrupt is not currently being serviced.") In the current
> >> code this works because we will set s->priority to 0, and
> >> s->prio_mask[0] is 0 and so the "level & s->prio_mask[s->priority]"
> >> masking in pl190_update() clears the bit and we won't assert
> >> the IRQ line. With your change, s->prio_mask[0] would have the
> >> bit set for the interrupt number, and so the masking would
> >> leave that bit set and leave IRQ asserted.
> >
> > Where did you get the information that "when the guest reads
> > the VICVectAddr register we want  to cause the IRQ line to
> > no longer be asserted." Imho, the _write_ to vectaddr clears
> > the interrupt request.
> 
> See my quote from the PL190 TRM above. Also I think it's
> clear from the TRM's suggested vectored interrupt flow sequence
> which is:
>  1  interrupt occurs
>  2  CPU takes IRQ
>  3  interrupt handler reads VICVectAddr
>  4  interrupt handler stacks CPU registers
>  5  interrupt handler reenables IRQ interrupts
>  6  do the ISR work
>  7  clear peripheral interrupt signal
>  8  disable ISR, restore regs
>  9  write VICVectAddr to clear interrupt in the PL190
>  10 return from interrupt (reenabling IRQ)
> 
> If the PL190 continues to signal IRQ for an interrupt
> that has had VICVectAddr read, then this sequence will
> recurse infinitely because as the CPU reenables IRQ in
> step 5 we'll take the IRQ interrupt again. The logic relies
> on the PL190 not signalling IRQ once it's been acknowledged
> via the VICVectAddr read (unless a higher priority interrupt
> arrives, which should cause IRQ to be asserted so the guest
> can recursively handle it).
> 
> There's also this text in the "About the VIC" section 2.1:
> "Reading from the Vector Interrupt Address Register, VICVECTADDR,
> provides the address of the ISR, and updates the interrupt
> priority hardware that masks out the current, and any lower
> priority interrupt requests."
>  -- which definitely says that the read causes the current
> interrupt to be masked out.
> 
> >> It also looks to me like this change breaks the logic for
> >> reading VECTADDR, because instead of the loop stopping
> >> with i as the priority of the highest set not-being-serviced
> >> interrupt it will stop with i being one less than that.
> >> (For instance if the interrupt for priority 2 is the highest
> >> set not-being-serviced interrupt then with your change
> >> we'll now set s->prio_mask[2] to have the relevant bit set,
> >> which means the loop will terminate with i == 1, and we return
> >> the vector address for interrupt priority 1, not 2 as we should.
> >
> > You're right that it changes the logic here, this is the actual
> > fix. I don't understand why you want to deliver the "highest set
> > not-being serviced interrupt vector". In fact, you have to
> > deliver the currently active IRQ vector address. The PL190 TRM
> > states for the VECTADDR register:
> >
> > "The read/write VICVECTADDR Register, with address offset of
> > 0x030, contains the Interrupt Service Routine (ISR) address
> > of the currently active interrupt"
> 
> At the point where the ISR reads VECTADDR, the currently active
> interrupt *is* the highest set not being serviced interrupt vector.
> (If your ISR reads it twice then that's a bad idea because
> the Note in the TRM says "reading [...] at other times can
> cause incorrect operation", but QEMU's implementation will
> return the same value again until a higher priority interrupt
> comes in or the interrupt is dismissed by writing to VECTADDR,
> since the 2nd read won't change s->priority and we'll just
> return s->vect_addr[s->priority] again.)
> 
> >> Perhaps I'm missing something -- what's the wrong behaviour
> >> that you're seeing that you're trying to fix ?
> >
> > I have a RTOS running which up to now only ran on real hardware.
> > It performs a sequence of accesses to the PL190 as stated in
> > the bug ticket. When reading back VECTADDR of the currently
> > handled interrupt (interrupt source 1 mapped to vector 2),
> > the PL190 emulation delivers VECTADDR3 instead and my OS fails
> > to dispatch the interrupt to the proper handler, but counts
> > a spurious interrupt.
> 
> Ah, I missed that the bug ticket had a sequence of actions:
> 
>  1) Write INTENCLEAR to clear all interrupt enable bits
>  2) Set all 16 vector control registers to zero
>  3) Set vector address #2 to value 2
>  4) Set vector control #2 to 0x21 (vector_interrupt_enable(0x20) |
> vector_interrupt_source(0x1) )
>  5) Enable interrupt 1 by writing 0x2 to INTENABLE
>  6) In interrupt handler: read VECTADDR [should read 0x2
>    (active IRQs vector address as set in step 3), reads 0x0
>    (active vector address index 3 instead of index 2)]
> 
> I don't see why step 6 gives you index 3, though. At that
> point s->priority should be 17, s->prio_mask[0] = 0,
> s->prio_mask[1] = 0, s->prio_mask[2] = 0, s->prio_mask[3] = 1 << 1,
> etc. s->level is 1 << 1.
> The loop
>         for (i = 0; i < s->priority; i++) {
>             if ((s->level | s->soft_level) & s->prio_mask[i + 1]) {
>                 break;
>             }
>         }
> will terminate with i == 2 (since s->prio_mask[3] is
> the first one with a set bit matching level). We then
> set s->priority to 2 and return the vect_addr[2] entry.
> 
> > To be more precise, the "current priority" s->priority is not
> > raised to the priority of the currently handled interrupt, but
> > to the subsequent priority in the priority range.
> 
> I don't see how this happens in the code: we stop the
> loop with i one less than the s->prio_mask[] array
> entry we check, because of the i+1 in the array index.
> 
> If you have some guest code I could reproduce this with
> that would be helpful.
> 
> thanks
> -- PMM
>

Hello Peter, you are completely right. The bug isn't in master and my patch is to be rejected.
There was once a version of pl190.c (like uhhm, around 2011) which didn't have the [i+1] in the array index and somehow (please don't ask) it made it into my qemu build. That's why the returned vector was broken. Of course, the fix was just to index the array with [i+1] instead of changing the prio_mask[i]. Regarding my confusion with the long mailing list delay, that's a triple fault. Shame on me.

Thank you for pointing this out.
Marc

Re: [Qemu-devel] [PATCH] pl190: Fix off-by-one error in priority handling when reading VECTADDR

[Peter Maydell]

On 27 February 2017 at 16:42, Marc Bommert <marc@brightwise.de> wrote:
> Hello Peter, you are completely right. The bug isn't in master
> and my patch is to be rejected. There was once a version of pl190.c
> (like uhhm, around 2011) which didn't have the [i+1] in the array
> index and somehow (please don't ask) it made it into my qemu build.

Ah, yes -- commit 14c126baf1c was that bugfix.

> Thank you for pointing this out.

No problem -- glad we've figured out what's going on.
(You should probably check you don't have any other ancient
wrong copies of source files lying around :-))

thanks
-- PMM