hw/misc/stm32_rcc: Correct offset-to-irq calculation

[PATCH] hw/misc/stm32_rcc: Correct offset-to-irq calculation

Posted by Peter Maydell 2 weeks, 2 days ago

In the STM32 RCC, there is a block of 5 "enable" registers, each of
which has 32 bits; each bit determines the level of one of the 5 * 32
= 160 enable_irq output lines.  The code calculates the irq to be
worked on using
  irq_offset = ((addr - STM32_RCC_AHB1_ENR) / 4) * 32;

This assumes that the registers are all consecutive; however, there
is a gap between the AHB1/2/3 registers and the APB1/2 registers, so
for the APB1/2 registers we calculate a number that is 32 too high
and can index off the end of the enable_irq[] array.

The handling of the reset registers has an identical bug.

Adjust the calculation of irq_offset to cope with the gap, and fix
the case labels so accesses to the gap fall into the default
LOG_UNIMP rather than being treated as if they were an actual
register.

Coverity CID: 1663683, 1663686
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/stm32_rcc.c         | 31 +++++++++++++++++++++++++++----
 include/hw/misc/stm32_rcc.h |  6 +++++-
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/hw/misc/stm32_rcc.c b/hw/misc/stm32_rcc.c
index 74ea29b156..5cfb39e560 100644
--- a/hw/misc/stm32_rcc.c
+++ b/hw/misc/stm32_rcc.c
@@ -53,6 +53,27 @@ static uint64_t stm32_rcc_read(void *opaque, hwaddr addr, unsigned int size)
     return value;
 }
 
+static int reg_offset_to_irq_offset(hwaddr addr)
+{
+    /*
+     * The reset and enable registers aren't all consecutive. In getting the
+     * irq index from the register offset, we need to account for the gap
+     * between the AHB regs and the APB regs.
+     */
+    switch (addr) {
+    case STM32_RCC_AHB1_RSTR ... STM32_RCC_AHB3_RSTR:
+        return ((addr - STM32_RCC_AHB1_RSTR) / 4) * 32;
+    case STM32_RCC_APB1_RSTR ... STM32_RCC_APB2_RSTR:
+        return ((addr - STM32_RCC_APB1_RSTR) / 4) * 32 + STM32_RCC_N_AHB_IRQS;
+    case STM32_RCC_AHB1_ENR ... STM32_RCC_AHB3_ENR:
+        return ((addr - STM32_RCC_AHB1_ENR) / 4) * 32;
+    case STM32_RCC_APB1_ENR ... STM32_RCC_APB2_ENR:
+        return ((addr - STM32_RCC_APB1_ENR) / 4) * 32 + STM32_RCC_N_AHB_IRQS;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 static void stm32_rcc_write(void *opaque, hwaddr addr,
                             uint64_t val64, unsigned int size)
 {
@@ -69,11 +90,12 @@ static void stm32_rcc_write(void *opaque, hwaddr addr,
     }
 
     switch (addr) {
-    case STM32_RCC_AHB1_RSTR...STM32_RCC_APB2_RSTR:
+    case STM32_RCC_AHB1_RSTR ... STM32_RCC_AHB3_RSTR:
+    case STM32_RCC_APB1_RSTR ... STM32_RCC_APB2_RSTR:
         prev_value = s->regs[addr / 4];
         s->regs[addr / 4] = value;
 
-        irq_offset = ((addr - STM32_RCC_AHB1_RSTR) / 4) * 32;
+        irq_offset = reg_offset_to_irq_offset(addr);
         for (int i = 0; i < 32; i++) {
             new_value = extract32(value, i, 1);
             if (extract32(prev_value, i, 1) && !new_value) {
@@ -82,11 +104,12 @@ static void stm32_rcc_write(void *opaque, hwaddr addr,
             }
         }
         return;
-    case STM32_RCC_AHB1_ENR...STM32_RCC_APB2_ENR:
+    case STM32_RCC_AHB1_ENR ... STM32_RCC_AHB3_ENR:
+    case STM32_RCC_APB1_ENR ... STM32_RCC_APB2_ENR:
         prev_value = s->regs[addr / 4];
         s->regs[addr / 4] = value;
 
-        irq_offset = ((addr - STM32_RCC_AHB1_ENR) / 4) * 32;
+        irq_offset = reg_offset_to_irq_offset(addr);
         for (int i = 0; i < 32; i++) {
             new_value = extract32(value, i, 1);
             if (!extract32(prev_value, i, 1) && new_value) {
diff --git a/include/hw/misc/stm32_rcc.h b/include/hw/misc/stm32_rcc.h
index 4dccacc2db..a94781bf34 100644
--- a/include/hw/misc/stm32_rcc.h
+++ b/include/hw/misc/stm32_rcc.h
@@ -65,7 +65,11 @@
 
 #define STM32_RCC_NREGS ((STM32_RCC_DCKCFGR2 >> 2) + 1)
 #define STM32_RCC_PERIPHERAL_SIZE 0x400
-#define STM32_RCC_NIRQS (32 * 5) /* 32 bits per reg, 5 en/rst regs */
+
+/* 32 bits per reg, 3 AHB regs and 2 APB regs */
+#define STM32_RCC_N_AHB_IRQS (32 * 3)
+#define STM32_RCC_N_APB_IRQS (32 * 2)
+#define STM32_RCC_NIRQS (STM32_RCC_N_AHB_IRQS + STM32_RCC_N_APB_IRQS)
 
 #define STM32_RCC_GPIO_IRQ_OFFSET 0
 
-- 
2.43.0

Re: [PATCH] hw/misc/stm32_rcc: Correct offset-to-irq calculation

Posted by Alistair 2 weeks, 2 days ago

On Thu, 2026-07-09 at 11:48 +0100, Peter Maydell wrote:
> In the STM32 RCC, there is a block of 5 "enable" registers, each of
> which has 32 bits; each bit determines the level of one of the 5 * 32
> = 160 enable_irq output lines.  The code calculates the irq to be
> worked on using
>   irq_offset = ((addr - STM32_RCC_AHB1_ENR) / 4) * 32;
> 
> This assumes that the registers are all consecutive; however, there
> is a gap between the AHB1/2/3 registers and the APB1/2 registers, so
> for the APB1/2 registers we calculate a number that is 32 too high
> and can index off the end of the enable_irq[] array.
> 
> The handling of the reset registers has an identical bug.
> 
> Adjust the calculation of irq_offset to cope with the gap, and fix
> the case labels so accesses to the gap fall into the default
> LOG_UNIMP rather than being treated as if they were an actual
> register.
> 
> Coverity CID: 1663683, 1663686
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

Alistair

> ---
>  hw/misc/stm32_rcc.c         | 31 +++++++++++++++++++++++++++----
>  include/hw/misc/stm32_rcc.h |  6 +++++-
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/misc/stm32_rcc.c b/hw/misc/stm32_rcc.c
> index 74ea29b156..5cfb39e560 100644
> --- a/hw/misc/stm32_rcc.c
> +++ b/hw/misc/stm32_rcc.c
> @@ -53,6 +53,27 @@ static uint64_t stm32_rcc_read(void *opaque,
> hwaddr addr, unsigned int size)
>      return value;
>  }
>  
> +static int reg_offset_to_irq_offset(hwaddr addr)
> +{
> +    /*
> +     * The reset and enable registers aren't all consecutive. In
> getting the
> +     * irq index from the register offset, we need to account for
> the gap
> +     * between the AHB regs and the APB regs.
> +     */
> +    switch (addr) {
> +    case STM32_RCC_AHB1_RSTR ... STM32_RCC_AHB3_RSTR:
> +        return ((addr - STM32_RCC_AHB1_RSTR) / 4) * 32;
> +    case STM32_RCC_APB1_RSTR ... STM32_RCC_APB2_RSTR:
> +        return ((addr - STM32_RCC_APB1_RSTR) / 4) * 32 +
> STM32_RCC_N_AHB_IRQS;
> +    case STM32_RCC_AHB1_ENR ... STM32_RCC_AHB3_ENR:
> +        return ((addr - STM32_RCC_AHB1_ENR) / 4) * 32;
> +    case STM32_RCC_APB1_ENR ... STM32_RCC_APB2_ENR:
> +        return ((addr - STM32_RCC_APB1_ENR) / 4) * 32 +
> STM32_RCC_N_AHB_IRQS;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  static void stm32_rcc_write(void *opaque, hwaddr addr,
>                              uint64_t val64, unsigned int size)
>  {
> @@ -69,11 +90,12 @@ static void stm32_rcc_write(void *opaque, hwaddr
> addr,
>      }
>  
>      switch (addr) {
> -    case STM32_RCC_AHB1_RSTR...STM32_RCC_APB2_RSTR:
> +    case STM32_RCC_AHB1_RSTR ... STM32_RCC_AHB3_RSTR:
> +    case STM32_RCC_APB1_RSTR ... STM32_RCC_APB2_RSTR:
>          prev_value = s->regs[addr / 4];
>          s->regs[addr / 4] = value;
>  
> -        irq_offset = ((addr - STM32_RCC_AHB1_RSTR) / 4) * 32;
> +        irq_offset = reg_offset_to_irq_offset(addr);
>          for (int i = 0; i < 32; i++) {
>              new_value = extract32(value, i, 1);
>              if (extract32(prev_value, i, 1) && !new_value) {
> @@ -82,11 +104,12 @@ static void stm32_rcc_write(void *opaque, hwaddr
> addr,
>              }
>          }
>          return;
> -    case STM32_RCC_AHB1_ENR...STM32_RCC_APB2_ENR:
> +    case STM32_RCC_AHB1_ENR ... STM32_RCC_AHB3_ENR:
> +    case STM32_RCC_APB1_ENR ... STM32_RCC_APB2_ENR:
>          prev_value = s->regs[addr / 4];
>          s->regs[addr / 4] = value;
>  
> -        irq_offset = ((addr - STM32_RCC_AHB1_ENR) / 4) * 32;
> +        irq_offset = reg_offset_to_irq_offset(addr);
>          for (int i = 0; i < 32; i++) {
>              new_value = extract32(value, i, 1);
>              if (!extract32(prev_value, i, 1) && new_value) {
> diff --git a/include/hw/misc/stm32_rcc.h
> b/include/hw/misc/stm32_rcc.h
> index 4dccacc2db..a94781bf34 100644
> --- a/include/hw/misc/stm32_rcc.h
> +++ b/include/hw/misc/stm32_rcc.h
> @@ -65,7 +65,11 @@
>  
>  #define STM32_RCC_NREGS ((STM32_RCC_DCKCFGR2 >> 2) + 1)
>  #define STM32_RCC_PERIPHERAL_SIZE 0x400
> -#define STM32_RCC_NIRQS (32 * 5) /* 32 bits per reg, 5 en/rst regs
> */
> +
> +/* 32 bits per reg, 3 AHB regs and 2 APB regs */
> +#define STM32_RCC_N_AHB_IRQS (32 * 3)
> +#define STM32_RCC_N_APB_IRQS (32 * 2)
> +#define STM32_RCC_NIRQS (STM32_RCC_N_AHB_IRQS +
> STM32_RCC_N_APB_IRQS)
>  
>  #define STM32_RCC_GPIO_IRQ_OFFSET 0
>