| Message ID | 1345258544-4740-1-git-send-email-bfennell@skynet.ie |
|---|---|
| State | New |
| Headers | show |
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
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 > >
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
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 > >
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:
Signed-off-by: Brendan Fennell <bfennell@skynet.ie> --- hw/pl190.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)