Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M
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 > --- > 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)? signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M
On 11 July 2018 at 14:25, Stefan Hajnoczi 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 >> --- >> 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
On Wed, Jul 11, 2018 at 02:33:14PM +0100, Peter Maydell wrote: > On 11 July 2018 at 14:25, Stefan Hajnoczi 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 > >> --- > >> 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 signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M
On 10 July 2018 at 16:33, Julia Suvorova 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 > --- > 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 = 0xfa05 | (s->prigroup[attrs.secure] << 8); > +val = 0xfa05; > +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
Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M
On 17.07.2018 15:58, Peter Maydell wrote: On 10 July 2018 at 16:33, Julia Suvorova 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 --- 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
On 18 July 2018 at 14:59, Julia Suvorova wrote: > On 17.07.2018 15:58, Peter Maydell wrote: >> >> On 10 July 2018 at 16:33, Julia Suvorova 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 >>> --- >>> 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