| Message ID | 20220925132010.5336-1-jim.shu@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/intc: sifive_plic: fix hard-coded max priority level | expand |
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
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
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); + } } else if (addr_between(addr, plic->pending_base, plic->num_sources >> 3)) { qemu_log_mask(LOG_GUEST_ERROR,