* [PATCH] hw/intc: sifive_plic: fix hard-coded max priority level
@ 2022-09-25 13:20 Jim Shu
2022-09-26 7:52 ` Clément Chigot
0 siblings, 1 reply; 3+ messages in thread
From: Jim Shu @ 2022-09-25 13:20 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: Jim Shu, Emmanuel Blot, Alistair Francis, Bin Meng, Palmer Dabbelt
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);
+ }
} else if (addr_between(addr, plic->pending_base,
plic->num_sources >> 3)) {
qemu_log_mask(LOG_GUEST_ERROR,
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/intc: sifive_plic: fix hard-coded max priority level
2022-09-25 13:20 [PATCH] hw/intc: sifive_plic: fix hard-coded max priority level Jim Shu
@ 2022-09-26 7:52 ` Clément Chigot
2022-09-28 15:53 ` Jim Shu
0 siblings, 1 reply; 3+ messages in thread
From: Clément Chigot @ 2022-09-26 7:52 UTC (permalink / raw)
To: Jim Shu
Cc: qemu-devel@nongnu.org Developers, qemu-riscv, Emmanuel Blot,
Alistair Francis, Bin Meng, Palmer Dabbelt
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/intc: sifive_plic: fix hard-coded max priority level
2022-09-26 7:52 ` Clément Chigot
@ 2022-09-28 15:53 ` Jim Shu
0 siblings, 0 replies; 3+ messages in thread
From: Jim Shu @ 2022-09-28 15:53 UTC (permalink / raw)
To: Clément Chigot
Cc: qemu-devel@nongnu.org Developers, qemu-riscv, Emmanuel Blot,
Alistair Francis, Bin Meng, Palmer Dabbelt
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-28 17:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-25 13:20 [PATCH] hw/intc: sifive_plic: fix hard-coded max priority level Jim Shu
2022-09-26 7:52 ` Clément Chigot
2022-09-28 15:53 ` Jim Shu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.