Re: [PATCH] hw/intc: sifive_plic: fix hard-coded max priority level

[Jim Shu]

Hi Clément,

Thanks for your opinion. I think it's a good enhancement.

PLIC spec [1] says that interrupt source priority registers should be
WARL fields to allow software to determine "the number and position of
read-write bits" in each priority specification, if any. To simplify
discovery of supported priority values, each priority register must
support any combination of values in the bits that are variable within
the register, i.e., if there are two variable bits in the register,
all four combinations of values in those bits must operate as valid
priority levels.

I think "num_priorities + 1" should be power-of-2 and SW could
discover available bits of interrupt source priority.
I'll do this enhancement in the next version patch.

[1] 
https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-priorities

Thanks,
Jim Shu




On Mon, Sep 26, 2022 at 3:52 PM Clément Chigot <chigot@adacore.com> wrote:
>
> Hi Jim,
>
> On Sun, Sep 25, 2022 at 3:26 PM Jim Shu <jim.shu@sifive.com> wrote:
> >
> > The maximum priority level is hard-coded when writing to interrupt
> > priority register. However, when writing to priority threshold register,
> > the maximum priority level is from num_priorities Property which is
> > configured by platform.
> >
> > Also change interrupt priority register to use num_priorities Property
> > in maximum priority level.
> >
> > Signed-off-by: Emmanuel Blot <emmanuel.blot@sifive.com>
> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
> > Reviewed-by: Frank Chang <frank.chang@sifive.com>
> > ---
> >  hw/intc/sifive_plic.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> > index af4ae3630e..f864efa761 100644
> > --- a/hw/intc/sifive_plic.c
> > +++ b/hw/intc/sifive_plic.c
> > @@ -180,8 +180,10 @@ static void sifive_plic_write(void *opaque, hwaddr 
> > addr, uint64_t value,
> >      if (addr_between(addr, plic->priority_base, plic->num_sources << 2)) {
> >          uint32_t irq = ((addr - plic->priority_base) >> 2) + 1;
> >
> > -        plic->source_priority[irq] = value & 7;
> > -        sifive_plic_update(plic);
> > +        if (value <= plic->num_priorities) {
> > +            plic->source_priority[irq] = value;
> > +            sifive_plic_update(plic);
> > +        }
>
> If I'm not mistaking the documentation [1], these registers are WARL
> (Write-Any-Read-Legal). However, in your case, any value above
> "num_priorities" will be ignored.
>
> We had an issue related to that and ended up making a local patch.
> However, we are assuming that "num_priorities+1" is a power of 2
> (which is currently the case)
>
> -        plic->source_priority[irq] = value & 7;
> +        /* Interrupt Priority registers are Write-Any Read-Legal. Cleanup
> +         * incoming values before storing them.
> +         */
> +        plic->source_priority[irq] = value % (plic->num_priorities + 1);
>
> Note that it should also be done for target_priority a bit below.
> -            if (value <= plic->num_priorities) {
> -                plic->target_priority[addrid] = value;
> -                sifive_plic_update(plic);
> -            }
> +            /* Priority Thresholds registers are Write-Any Read-Legal. 
> Cleanup
> +             * incoming values before storing them.
> +             */
> +            plic->target_priority[addrid] = value % (plic->num_priorities + 
> 1);
> +            sifive_plic_update(plic);
>
> [1] https://static.dev.sifive.com/FE310-G000.pdf
>
> Thanks,
> Clément