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

[Clément Chigot]

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