hw/intc/armv7m_nvic.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
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
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)?
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
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
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
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.
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
© 2016 - 2024 Red Hat, Inc.