| Message ID | 20180710153335.1232-1-jusual@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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(-)