[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH] nvic: Change NVIC to support ARMv6-M |
Date: |
Tue, 17 Jul 2018 13:58:59 +0100 |
On 10 July 2018 at 16:33, Julia Suvorova <address@hidden> 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 <address@hidden>
> ---
> 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