[PATCH 0/2] Fixes for standard conformance

[PATCH 0/2] Fixes for standard conformance

[Sergey Makarov]

*** Patchset goal ***

This patchset aims to improve standard conformance for SiFive PLIC
implementation.

*** Testing cases ***

Currently there are no automated tests for these changes, but there
are several test cases, with which these changes may be checked:
1. Zeroth priority register can be checked by reading it after
   writing to it. Without patch its value would be the same which
   is written there, but with it it would be zero;
2. Trigger call of `sifive_plic_irq_request` with level 0.
   Without second patch it will clear pending bit, but with it
   pending bit won't be cleared.
If anyone knows how this can be turned into automated test, help
would be appreciated.

Sergey Makarov (2):
  hw/intc: Make zeroth priority register read-only
  hw/intc: Don't clear pending bits on IRQ lowering

 hw/intc/sifive_plic.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH 1/2] hw/intc: Make zeroth priority register read-only

[Sergey Makarov]

According to PLIC specification chapter 4, zeroth
priority register is reserved. Discard writes to
this register.

Signed-off-by: Sergey Makarov <s.makarov@syntacore.com>
---
 hw/intc/sifive_plic.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index e559f11805..3f3ee96ebc 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -189,8 +189,13 @@ 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;
-
-        if (((plic->num_priorities + 1) & plic->num_priorities) == 0) {
+        if (irq == 0) {
+            /* IRQ 0 source prioority is reserved */
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Invalid source priority write 0x%"
+                          HWADDR_PRIx "\n", __func__, addr);
+            return;
+        } else if (((plic->num_priorities + 1) & plic->num_priorities) == 0) {
             /*
              * if "num_priorities + 1" is power-of-2, make each register bit of
              * interrupt priority WARL (Write-Any-Read-Legal). Just filter
-- 
2.34.1

[PATCH 2/2] hw/intc: Don't clear pending bits on IRQ lowering

[Sergey Makarov]

According to PLIC specification (chapter 5), there
is only one case, when interrupt is claimed. Fix
PLIC controller to match this behavior.

Signed-off-by: Sergey Makarov <s.makarov@syntacore.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 3f3ee96ebc..deec162630 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -354,8 +354,10 @@ static void sifive_plic_irq_request(void *opaque, int irq, int level)
 {
     SiFivePLICState *s = opaque;
 
-    sifive_plic_set_pending(s, irq, level > 0);
-    sifive_plic_update(s);
+    if (level > 0) {
+        sifive_plic_set_pending(s, irq, true);
+        sifive_plic_update(s);
+    }
 }
 
 static void sifive_plic_realize(DeviceState *dev, Error **errp)
-- 
2.34.1

Re: [PATCH 0/2] Fixes for standard conformance

[Peter Maydell]

On Wed, 11 Sept 2024 at 23:30, Sergey Makarov <s.makarov@syntacore.com> wrote:
>
> *** Patchset goal ***
>
> This patchset aims to improve standard conformance for SiFive PLIC
> implementation.
>
> *** Testing cases ***
>
> Currently there are no automated tests for these changes, but there
> are several test cases, with which these changes may be checked:
> 1. Zeroth priority register can be checked by reading it after
>    writing to it. Without patch its value would be the same which
>    is written there, but with it it would be zero;
> 2. Trigger call of `sifive_plic_irq_request` with level 0.
>    Without second patch it will clear pending bit, but with it
>    pending bit won't be cleared.
> If anyone knows how this can be turned into automated test, help
> would be appreciated.
>
> Sergey Makarov (2):
>   hw/intc: Make zeroth priority register read-only
>   hw/intc: Don't clear pending bits on IRQ lowering
>
>  hw/intc/sifive_plic.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

When you're writing cover letter and patch subject lines,
please can you make them say what part of the codebase
they're touching? This cover letter doesn't say anything in
the subject line that would tell the reader it's about riscv,
for instance, and nor do the patch line subjects.

This is helpful because people scan subject lines to see
whether the patchset is something they're interested in
or not.

thanks
-- PMM