[PATCH 0/2] riscv: hw/intc: Fixes for standard conformance

[PATCH 0/2] riscv: hw/intc: 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

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

[Alistair Francis]

On Thu, Sep 19, 2024 at 12:04 AM Sergey Makarov <s.makarov@syntacore.com> wrote:
>
> 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>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 2/2] hw/intc: Don't clear pending bits on IRQ lowering

[Alistair Francis]

On Thu, Sep 19, 2024 at 12:03 AM Sergey Makarov <s.makarov@syntacore.com> wrote:
>
> 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>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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] riscv: hw/intc: Fixes for standard conformance

[Alistair Francis]

On Thu, Sep 19, 2024 at 12:04 AM 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

Thanks!

Applied to riscv-to-apply.next

Alistair

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

[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 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