[PATCH] hw/arm/nvic: implement "num-prio-bits" property

[PATCH] hw/arm/nvic: implement "num-prio-bits" property

[Anton Kochkov]

Cortex-M NVIC can be configured with different amount of
the maximum available priority bits. FreeRTOS has asserts
that checks if the all unavailable priority bits are unset
after writing into this register in real hardware.
To allow setting this number depending on the machine or
configuration expose priority bits as QDev property
which is by default is set to 8 as it was hardcoded in the past.
Thus, existing code doesn't require any additional changes,
and it doesn't change the default behavior of NVIC.

Signed-off-by: Anton Kochkov <anton.kochkov@proton.me>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1122
---
 hw/intc/armv7m_nvic.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 1f7763964c..b8959d645d 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -2580,6 +2580,8 @@ static const VMStateDescription vmstate_nvic = {
 static Property props_nvic[] = {
     /* Number of external IRQ lines (so excluding the 16 internal exceptions) */
     DEFINE_PROP_UINT32("num-irq", NVICState, num_irq, 64),
+    /* Number of the maximum priority bits that can be used */
+    DEFINE_PROP_UINT8("num-prio-bits", NVICState, num_prio_bits, 8),
     DEFINE_PROP_END_OF_LIST()
 };

@@ -2690,7 +2692,9 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
     /* include space for internal exception vectors */
     s->num_irq += NVIC_FIRST_IRQ;

-    s->num_prio_bits = arm_feature(&s->cpu->env, ARM_FEATURE_V7) ? 8 : 2;
+    if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
+        s->num_prio_bits = 2;
+    }

     /*
      * This device provides a single memory region which covers the
--
2.37.1

Re: [PATCH] hw/arm/nvic: implement "num-prio-bits" property

[Peter Maydell]

On Sat, 13 Aug 2022 at 12:27, Anton Kochkov <anton.kochkov@proton.me> wrote:
>
> Cortex-M NVIC can be configured with different amount of
> the maximum available priority bits. FreeRTOS has asserts
> that checks if the all unavailable priority bits are unset
> after writing into this register in real hardware.

This assert in FreeRTOS is misguided, incidentally. It's
reasonable to assert that the CPU has at least enough priority
bits for whatever the OS build was configured for, but
asserting that it exactly matches is pointless unless you're
trying to write a test suite for "test that software model
exactly matches hardware".

> To allow setting this number depending on the machine or
> configuration expose priority bits as QDev property
> which is by default is set to 8 as it was hardcoded in the past.
> Thus, existing code doesn't require any additional changes,
> and it doesn't change the default behavior of NVIC.
>
> Signed-off-by: Anton Kochkov <anton.kochkov@proton.me>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1122
> ---
>  hw/intc/armv7m_nvic.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 1f7763964c..b8959d645d 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -2580,6 +2580,8 @@ static const VMStateDescription vmstate_nvic = {
>  static Property props_nvic[] = {
>      /* Number of external IRQ lines (so excluding the 16 internal exceptions) */
>      DEFINE_PROP_UINT32("num-irq", NVICState, num_irq, 64),
> +    /* Number of the maximum priority bits that can be used */
> +    DEFINE_PROP_UINT8("num-prio-bits", NVICState, num_prio_bits, 8),
>      DEFINE_PROP_END_OF_LIST()
>  };
>
> @@ -2690,7 +2692,9 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
>      /* include space for internal exception vectors */
>      s->num_irq += NVIC_FIRST_IRQ;
>
> -    s->num_prio_bits = arm_feature(&s->cpu->env, ARM_FEATURE_V7) ? 8 : 2;
> +    if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
> +        s->num_prio_bits = 2;
> +    }

Thanks for this patch. It's correct as far as it goes, but it's
missing some pieces.

Firstly, armv7m_nvic_realize now needs to do sanity checking
on the num_prio_bits value at the top of the function, the way
we do for num_irq, so that we return an error if the property
was set to an out of range value (ie less than 2 or more than 8).

Secondly, the armv7m container object needs to have an alias
property for this (because SoCs don't create the NVIC directly,
they create the armv7m object). That's just a one-liner in
armv7m_instance_init() to call object_property_add_alias() the
way we do already for "num-irq". You also need to add a line to
the comment in include/hw/arm/armv7m.h that documents the
properties of the container object describing the new property.

Thirdly, you need code in the board models which actually sets
the property to something other than the default when it creates
the armv7m container (at least for whichever board model you care
about, we don't necessarily have to fix all of them). Otherwise
this is all dead code that doesn't change QEMU's behaviour.

thanks
-- PMM