Re: [PATCH v2 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field

[Clément Chigot]

Hi Jim,

On Fri, Sep 30, 2022 at 2:32 PM Jim Shu <jim.shu@sifive.com> wrote:
>
> PLIC spec [1] requires interrupt source priority registers are WARL
> field and the number of supported priority is power-of-2 to simplify SW
> discovery.
>
> Existing QEMU RISC-V machine (e.g. shakti_c) don't strictly follow PLIC
> spec, whose number of supported priority is not power-of-2. Just change
> each bit of interrupt priority register to WARL field when the number of
> supported priority is power-of-2.
>
> [1] 
> https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc#interrupt-priorities
>
> Signed-off-by: Jim Shu <jim.shu@sifive.com>
> ---
>  hw/intc/sifive_plic.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> index f864efa761..218ccff8bd 100644
> --- a/hw/intc/sifive_plic.c
> +++ b/hw/intc/sifive_plic.c
> @@ -180,7 +180,15 @@ 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;
>
> -        if (value <= plic->num_priorities) {
> +        if ((plic->num_priorities + 1) & (plic->num_priorities)) {

That's the opposite. If n is a power of 2, n & (n-1) == 0 (eg 8 & 7 ==
 0, 9 & 8 == 8).
Note that n must be positive too. But I'm not sure it matters here.
I'll let you decide.

> +            /*
> +             * if "num_priorities + 1" is power-of-2, make each register bit 
> of
> +             * interrupt priority WARL (Write-Any-Read-Legal). Just filter
> +             * out the access to unsupported priority bits.
> +             */
> +            plic->source_priority[irq] = value % (plic->num_priorities + 1);
> +            sifive_plic_update(plic);
> +        } else if (value <= plic->num_priorities) {
>              plic->source_priority[irq] = value;
>              sifive_plic_update(plic);
>          }
> @@ -207,7 +215,16 @@ static void sifive_plic_write(void *opaque, hwaddr addr, 
> uint64_t value,
>          uint32_t contextid = (addr & (plic->context_stride - 1));
>
>          if (contextid == 0) {
> -            if (value <= plic->num_priorities) {
> +            if ((plic->num_priorities + 1) & (plic->num_priorities)) {

Same.

> +                /*
> +                 * if "num_priorities + 1" is power-of-2, each register bit 
> of
> +                 * interrupt priority is WARL (Write-Any-Read-Legal). Just
> +                 * filter out the access to unsupported priority bits.
> +                 */
> +                plic->target_priority[addrid] = value %
> +                                                (plic->num_priorities + 1);
> +                sifive_plic_update(plic);
> +            } else if (value <= plic->num_priorities) {
>                  plic->target_priority[addrid] = value;
>                  sifive_plic_update(plic);
>              }
> --
> 2.17.1

Clément