On Wed, 27 Mar 2019 09:29:56 PDT (-0700), address@hidden wrote:
> On Wed, Mar 27, 2019 at 3:29 AM Palmer Dabbelt <address@hidden> wrote:
>>
>> On Wed, 20 Mar 2019 17:46:09 PDT (-0700), Alistair Francis wrote:
>> > The irq is incorrectly calculated to be off by one. It has worked in the
>> > past as the priority_base offset has also been set incorrectly. We are
>> > about to fix the priority_base offset so first first the irq
>> > calculation.
>> >
>> > Signed-off-by: Alistair Francis <address@hidden>
>> > ---
>> > hw/riscv/sifive_plic.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
>> > index 1c703e1a37..70a85cd075 100644
>> > --- a/hw/riscv/sifive_plic.c
>> > +++ b/hw/riscv/sifive_plic.c
>> > @@ -206,7 +206,7 @@ static uint64_t sifive_plic_read(void *opaque, hwaddr
addr, unsigned size)
>> > if (addr >= plic->priority_base && /* 4 bytes per source */
>> > addr < plic->priority_base + (plic->num_sources << 2))
>> > {
>> > - uint32_t irq = (addr - plic->priority_base) >> 2;
>> > + uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
>> > if (RISCV_DEBUG_PLIC) {
>> > qemu_log("plic: read priority: irq=%d priority=%d\n",
>> > irq, plic->source_priority[irq]);
>> > @@ -279,7 +279,7 @@ static void sifive_plic_write(void *opaque, hwaddr
addr, uint64_t value,
>> > if (addr >= plic->priority_base && /* 4 bytes per source */
>> > addr < plic->priority_base + (plic->num_sources << 2))
>> > {
>> > - uint32_t irq = (addr - plic->priority_base) >> 2;
>> > + uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
>> > plic->source_priority[irq] = value & 7;
>> > if (RISCV_DEBUG_PLIC) {
>> > qemu_log("plic: write priority: irq=%d priority=%d\n",
>>
>> I think this breaks bisectability and should be merged with the
>> *_PLIC_PRIORITY_BASE modifications as otherwise we'll end up the priority
being
>> set for the brong IRQ.
>
> Good point, I can merge them together.
>
>>
>> I'm also not sure how this fixes the bug listed in the OpenSBI PR. As far
as I
>> can understand it, all this does is ensure that source 0 is actually treated
as
>> illegal (and shrinks the number of sources to match what's actually there,
but
>> that shouldn't fix the bug). That looks more like a cleanup than an actual
fix.
>
> The problem is that before we where off by one. We supported 0 - (n -
> 1) and this patch set changes QEMU to support 1 - n. This is because
> of the "addr < plic->priority_base + (plic->num_sources << 2)"
> comparison. As priority_base is now 0x04 instead of 0x00 the
> comparison will work correctly.
So something in OpenSBI is going through the entire set of sources and
initializing the priorities? That makes sense for the off-by-one, I was just
wondering because the array shrank so the specific offset in that error would
still be invalid with the new patch set.