On 27 February 2017 at 08:15, Marc Bommert <m...@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
