[Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Julia Suvorova]

The differences from ARMv7-M NVIC are:
  * ARMv6-M only supports up to 32 external interrupts
   (configurable feature already). The ICTR is reserved.
  * Active Bit Register is reserved.
  * ARMv6-M supports 4 priority levels against 256 in ARMv7-M.

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 hw/intc/armv7m_nvic.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 38aaf3dc8e..8545c87caa 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -420,6 +420,10 @@ static void set_prio(NVICState *s, unsigned irq, bool secure, uint8_t prio)
     assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios */
     assert(irq < s->num_irq);
 
+    if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
+        prio &= 0xc0;
+    }
+
     if (secure) {
         assert(exc_is_banked(irq));
         s->sec_vectors[irq].prio = prio;
@@ -777,6 +781,9 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
 
     switch (offset) {
     case 4: /* Interrupt Control Type.  */
+        if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) {
+            goto bad_offset;
+        }
         return ((s->num_irq - NVIC_FIRST_IRQ) / 32) - 1;
     case 0xc: /* CPPWR */
         if (!arm_feature(&cpu->env, ARM_FEATURE_V8)) {
@@ -850,7 +857,10 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
     case 0xd08: /* Vector Table Offset.  */
         return cpu->env.v7m.vecbase[attrs.secure];
     case 0xd0c: /* Application Interrupt/Reset Control (AIRCR) */
-        val = 0xfa050000 | (s->prigroup[attrs.secure] << 8);
+        val = 0xfa050000;
+        if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
+            val |= (s->prigroup[attrs.secure] << 8);
+        }
         if (attrs.secure) {
             /* s->aircr stores PRIS, BFHFNMINS, SYSRESETREQS */
             val |= cpu->env.v7m.aircr;
@@ -1274,6 +1284,10 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
                               "Setting VECTRESET when not in DEBUG mode "
                               "is UNPREDICTABLE\n");
             }
+            if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) {
+                nvic_irq_update(s);
+                break;
+            }
             s->prigroup[attrs.secure] = extract32(value,
                                                   R_V7M_AIRCR_PRIGROUP_SHIFT,
                                                   R_V7M_AIRCR_PRIGROUP_LENGTH);
@@ -1785,6 +1799,11 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
         break;
     case 0x300 ... 0x33f: /* NVIC Active */
         val = 0;
+
+        if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
+            break;
+        }
+
         startvec = 8 * (offset - 0x300) + NVIC_FIRST_IRQ; /* vector # */
 
         for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; i++) {
@@ -2160,13 +2179,15 @@ static Property props_nvic[] = {
 
 static void armv7m_nvic_reset(DeviceState *dev)
 {
-    int resetprio;
+    int resetprio, resetprigroup;
     NVICState *s = NVIC(dev);
 
     memset(s->vectors, 0, sizeof(s->vectors));
     memset(s->sec_vectors, 0, sizeof(s->sec_vectors));
-    s->prigroup[M_REG_NS] = 0;
-    s->prigroup[M_REG_S] = 0;
+
+    resetprigroup = arm_feature(&s->cpu->env, ARM_FEATURE_V7) ? 0 : 5;
+    s->prigroup[M_REG_NS] = resetprigroup;
+    s->prigroup[M_REG_S] = resetprigroup;
 
     s->vectors[ARMV7M_EXCP_NMI].enabled = 1;
     /* MEM, BUS, and USAGE are enabled through
-- 
2.17.1

Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

On Tue, Jul 10, 2018 at 06:33:35PM +0300, Julia Suvorova via Qemu-devel wrote:
> The differences from ARMv7-M NVIC are:
>   * ARMv6-M only supports up to 32 external interrupts
>    (configurable feature already). The ICTR is reserved.
>   * Active Bit Register is reserved.
>   * ARMv6-M supports 4 priority levels against 256 in ARMv7-M.
> 
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  hw/intc/armv7m_nvic.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 38aaf3dc8e..8545c87caa 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -420,6 +420,10 @@ static void set_prio(NVICState *s, unsigned irq, bool secure, uint8_t prio)
>      assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios */
>      assert(irq < s->num_irq);
>  
> +    if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {

ARMv8-M also has the NVIC:
https://static.docs.arm.com/ddi0553/a/DDI0553A_e_armv8m_arm.pdf

Should arm_feature(&s->cpu->env, ARM_FEATURE_V6) be used instead of
!arm_feature(&s->cpu->env, ARM_FEATURE_V7)?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Peter Maydell]

On 11 July 2018 at 14:25, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jul 10, 2018 at 06:33:35PM +0300, Julia Suvorova via Qemu-devel wrote:
>> The differences from ARMv7-M NVIC are:
>>   * ARMv6-M only supports up to 32 external interrupts
>>    (configurable feature already). The ICTR is reserved.
>>   * Active Bit Register is reserved.
>>   * ARMv6-M supports 4 priority levels against 256 in ARMv7-M.
>>
>> Signed-off-by: Julia Suvorova <jusual@mail.ru>
>> ---
>>  hw/intc/armv7m_nvic.c | 29 +++++++++++++++++++++++++----
>>  1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index 38aaf3dc8e..8545c87caa 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -420,6 +420,10 @@ static void set_prio(NVICState *s, unsigned irq, bool secure, uint8_t prio)
>>      assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios */
>>      assert(irq < s->num_irq);
>>
>> +    if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
>
> ARMv8-M also has the NVIC:
> https://static.docs.arm.com/ddi0553/a/DDI0553A_e_armv8m_arm.pdf
>
> Should arm_feature(&s->cpu->env, ARM_FEATURE_V6) be used instead of
> !arm_feature(&s->cpu->env, ARM_FEATURE_V7)?

All of v6M, v7M and v8M cores will set ARM_FEATURE_V6, so
testing on it doesn't distinguish them.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Wed, Jul 11, 2018 at 02:33:14PM +0100, Peter Maydell wrote:
> On 11 July 2018 at 14:25, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Tue, Jul 10, 2018 at 06:33:35PM +0300, Julia Suvorova via Qemu-devel wrote:
> >> The differences from ARMv7-M NVIC are:
> >>   * ARMv6-M only supports up to 32 external interrupts
> >>    (configurable feature already). The ICTR is reserved.
> >>   * Active Bit Register is reserved.
> >>   * ARMv6-M supports 4 priority levels against 256 in ARMv7-M.
> >>
> >> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> >> ---
> >>  hw/intc/armv7m_nvic.c | 29 +++++++++++++++++++++++++----
> >>  1 file changed, 25 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> >> index 38aaf3dc8e..8545c87caa 100644
> >> --- a/hw/intc/armv7m_nvic.c
> >> +++ b/hw/intc/armv7m_nvic.c
> >> @@ -420,6 +420,10 @@ static void set_prio(NVICState *s, unsigned irq, bool secure, uint8_t prio)
> >>      assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios */
> >>      assert(irq < s->num_irq);
> >>
> >> +    if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
> >
> > ARMv8-M also has the NVIC:
> > https://static.docs.arm.com/ddi0553/a/DDI0553A_e_armv8m_arm.pdf
> >
> > Should arm_feature(&s->cpu->env, ARM_FEATURE_V6) be used instead of
> > !arm_feature(&s->cpu->env, ARM_FEATURE_V7)?
> 
> All of v6M, v7M and v8M cores will set ARM_FEATURE_V6, so
> testing on it doesn't distinguish them.

Okay.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Peter Maydell]

On 10 July 2018 at 16:33, Julia Suvorova <jusual@mail.ru> wrote:
> The differences from ARMv7-M NVIC are:
>   * ARMv6-M only supports up to 32 external interrupts
>    (configurable feature already). The ICTR is reserved.
>   * Active Bit Register is reserved.
>   * ARMv6-M supports 4 priority levels against 256 in ARMv7-M.
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  hw/intc/armv7m_nvic.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 38aaf3dc8e..8545c87caa 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -420,6 +420,10 @@ static void set_prio(NVICState *s, unsigned irq, bool secure, uint8_t prio)
>      assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios */
>      assert(irq < s->num_irq);
>
> +    if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
> +        prio &= 0xc0;

Rather than hard-coding this, I think we should have a
num_prio_bits field in the NVICState struct which defines
how many bits of priority are implemented. This would be
2 for v6M and 8 otherwise, and can be set in
armv7m_nvic_realize. Then the mask is
  MAKE_64BIT_MASK(8 - num_prio_bits, num_prio_bits);
(For v8M the number of priority bits is configurable.)

> +    }
> +
>      if (secure) {
>          assert(exc_is_banked(irq));
>          s->sec_vectors[irq].prio = prio;
> @@ -777,6 +781,9 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
>
>      switch (offset) {
>      case 4: /* Interrupt Control Type.  */
> +        if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) {
> +            goto bad_offset;
> +        }
>          return ((s->num_irq - NVIC_FIRST_IRQ) / 32) - 1;
>      case 0xc: /* CPPWR */
>          if (!arm_feature(&cpu->env, ARM_FEATURE_V8)) {
> @@ -850,7 +857,10 @@ static uint32_t nvic_readl(NVICState *s, uint32_t offset, MemTxAttrs attrs)
>      case 0xd08: /* Vector Table Offset.  */
>          return cpu->env.v7m.vecbase[attrs.secure];
>      case 0xd0c: /* Application Interrupt/Reset Control (AIRCR) */
> -        val = 0xfa050000 | (s->prigroup[attrs.secure] << 8);
> +        val = 0xfa050000;
> +        if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
> +            val |= (s->prigroup[attrs.secure] << 8);
> +        }

I think this change is unnecessary, because if we're v6M then
there is no way for the guest to change s->prigroup[] and it will
always be zero, so whether we OR it in or not makes no difference.
(See below about prigroup[]'s reset value.)

>          if (attrs.secure) {
>              /* s->aircr stores PRIS, BFHFNMINS, SYSRESETREQS */
>              val |= cpu->env.v7m.aircr;
> @@ -1274,6 +1284,10 @@ static void nvic_writel(NVICState *s, uint32_t offset, uint32_t value,
>                                "Setting VECTRESET when not in DEBUG mode "
>                                "is UNPREDICTABLE\n");
>              }
> +            if (!arm_feature(&cpu->env, ARM_FEATURE_V7)) {
> +                nvic_irq_update(s);
> +                break;
> +            }
>              s->prigroup[attrs.secure] = extract32(value,
>                                                    R_V7M_AIRCR_PRIGROUP_SHIFT,
>                                                    R_V7M_AIRCR_PRIGROUP_LENGTH);

I would prefer to guard the s->prigroup[] update by wrapping it in
    if (arm_feature(&cpu->env, ARM_FEATURE_V7) {
        s->prigroup[attrs.secure] = ...
    }

rather than having an early exit via break here.

> @@ -1785,6 +1799,11 @@ static MemTxResult nvic_sysreg_read(void *opaque, hwaddr addr,
>          break;
>      case 0x300 ... 0x33f: /* NVIC Active */
>          val = 0;
> +
> +        if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
> +            break;
> +        }
> +
>          startvec = 8 * (offset - 0x300) + NVIC_FIRST_IRQ; /* vector # */
>
>          for (i = 0, end = size * 8; i < end && startvec + i < s->num_irq; i++) {
> @@ -2160,13 +2179,15 @@ static Property props_nvic[] = {
>
>  static void armv7m_nvic_reset(DeviceState *dev)
>  {
> -    int resetprio;
> +    int resetprio, resetprigroup;
>      NVICState *s = NVIC(dev);
>
>      memset(s->vectors, 0, sizeof(s->vectors));
>      memset(s->sec_vectors, 0, sizeof(s->sec_vectors));
> -    s->prigroup[M_REG_NS] = 0;
> -    s->prigroup[M_REG_S] = 0;
> +
> +    resetprigroup = arm_feature(&s->cpu->env, ARM_FEATURE_V7) ? 0 : 5;
> +    s->prigroup[M_REG_NS] = resetprigroup;
> +    s->prigroup[M_REG_S] = resetprigroup;

I don't think we should need this -- prigroup[] can reset to 0 always.
For v8M-baseline this is definitely true -- a v8M baseline CPU has
a prigroup value of 0, meaning that the priority register fields
have bits [7:1] as priority and [0] as subgroup priority, regardless
of how many of the low bits in each field are RAZ/WI. When all
the subgroup priority bits are RAZ/WI then effectively there is
only group priority. For v6M although the Arm ARM doesn't describe
it this way you can implement it like that, and I think it makes
things more consistent with v7M/v8M if we do.

(You can see from the QEMU implementation that there's no behaviour
difference from the choice of prigroup[] value -- the only use of
prigroup[] is in nvic_gprio_mask(), which is used to mask raw
priority values from the registers. Since for v6M the raw priority
values will always have zeroes in the bottom 6 bits, it doesn't
matter whether nvic_gprio_mask() returns 0xc0 (for a prigroup value
of 5) or 0xfe (for a prigroup value of 0).)

>      s->vectors[ARMV7M_EXCP_NMI].enabled = 1;
>      /* MEM, BUS, and USAGE are enabled through
> --
> 2.17.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Julia Suvorova]

On 17.07.2018 15:58, Peter Maydell wrote:
> On 10 July 2018 at 16:33, Julia Suvorova <jusual@mail.ru> wrote:
>> The differences from ARMv7-M NVIC are:
>>    * ARMv6-M only supports up to 32 external interrupts
>>     (configurable feature already). The ICTR is reserved.
>>    * Active Bit Register is reserved.
>>    * ARMv6-M supports 4 priority levels against 256 in ARMv7-M.
>>
>> Signed-off-by: Julia Suvorova <jusual@mail.ru>
>> ---
>>   hw/intc/armv7m_nvic.c | 29 +++++++++++++++++++++++++----
>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>> index 38aaf3dc8e..8545c87caa 100644
>> --- a/hw/intc/armv7m_nvic.c
>> +++ b/hw/intc/armv7m_nvic.c
>> @@ -420,6 +420,10 @@ static void set_prio(NVICState *s, unsigned irq, bool secure, uint8_t prio)
>>       assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios */
>>       assert(irq < s->num_irq);
>>
>> +    if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
>> +        prio &= 0xc0;
> 
> Rather than hard-coding this, I think we should have a
> num_prio_bits field in the NVICState struct which defines
> how many bits of priority are implemented. This would be
> 2 for v6M and 8 otherwise, and can be set in
> armv7m_nvic_realize. Then the mask is
>    MAKE_64BIT_MASK(8 - num_prio_bits, num_prio_bits);
> (For v8M the number of priority bits is configurable.)

Do I understand correctly that the check in armv7m_nvic_realize is for
Baseline, not only v6m, because Baseline has only 4 priority levels too?
And num_prio_bits should be just a field, not a property?

Best regards, Julia Suvorova.

Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M

[Peter Maydell]

On 18 July 2018 at 14:59, Julia Suvorova <jusual@mail.ru> wrote:
> On 17.07.2018 15:58, Peter Maydell wrote:
>>
>> On 10 July 2018 at 16:33, Julia Suvorova <jusual@mail.ru> wrote:
>>>
>>> The differences from ARMv7-M NVIC are:
>>>    * ARMv6-M only supports up to 32 external interrupts
>>>     (configurable feature already). The ICTR is reserved.
>>>    * Active Bit Register is reserved.
>>>    * ARMv6-M supports 4 priority levels against 256 in ARMv7-M.
>>>
>>> Signed-off-by: Julia Suvorova <jusual@mail.ru>
>>> ---
>>>   hw/intc/armv7m_nvic.c | 29 +++++++++++++++++++++++++----
>>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>>> index 38aaf3dc8e..8545c87caa 100644
>>> --- a/hw/intc/armv7m_nvic.c
>>> +++ b/hw/intc/armv7m_nvic.c
>>> @@ -420,6 +420,10 @@ static void set_prio(NVICState *s, unsigned irq,
>>> bool secure, uint8_t prio)
>>>       assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios
>>> */
>>>       assert(irq < s->num_irq);
>>>
>>> +    if (!arm_feature(&s->cpu->env, ARM_FEATURE_V7)) {
>>> +        prio &= 0xc0;
>>
>>
>> Rather than hard-coding this, I think we should have a
>> num_prio_bits field in the NVICState struct which defines
>> how many bits of priority are implemented. This would be
>> 2 for v6M and 8 otherwise, and can be set in
>> armv7m_nvic_realize. Then the mask is
>>    MAKE_64BIT_MASK(8 - num_prio_bits, num_prio_bits);
>> (For v8M the number of priority bits is configurable.)
>
>
> Do I understand correctly that the check in armv7m_nvic_realize is for
> Baseline, not only v6m, because Baseline has only 4 priority levels too?
> And num_prio_bits should be just a field, not a property?

Make it for v6M -- v8M baseline doesn't specify a particular
number of priority levels, you can do anything. You can leave
it as a field and we'll bump it up to a property if we need it
(probably if/when we do a v8M baseline CPU model); though
if you want to make it a full property that's fine too.

thanks
-- PMM