Re: [PATCH v2 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field
Hi Clément,
> > > @@ -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.
> >
num_priorities is a uint32_t variable so that n is always positive.
Re: [PATCH v2 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field
hi Clément,
Thank you very much.
I'll fix it in the next version patch.
Thanks,
Jim Shu
On Fri, Sep 30, 2022 at 8:58 PM Clément Chigot wrote:
>
> Hi Jim,
>
> On Fri, Sep 30, 2022 at 2:32 PM Jim Shu 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
> > ---
> > 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
Re: [PATCH v2 2/2] hw/intc: sifive_plic: change interrupt priority register to WARL field
Hi Jim,
On Fri, Sep 30, 2022 at 2:32 PM Jim Shu 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
> ---
> 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
