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(>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
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(>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 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(>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(>env, ARM_FEATURE_V7)) {
> +goto bad_offset;
> +}
> return ((s->num_irq - NVIC_FIRST_IRQ) / 32) - 1;
> case 0xc: /* CPPWR */
> if (!arm_feature(>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(>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(>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(>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(>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(>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
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(>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(>cpu->env, ARM_FEATURE_V6) be used instead of
> > !arm_feature(>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 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(>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(>cpu->env, ARM_FEATURE_V6) be used instead of
> !arm_feature(>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 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(>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(>cpu->env, ARM_FEATURE_V6) be used instead of
!arm_feature(>cpu->env, ARM_FEATURE_V7)?
signature.asc
Description: PGP signature
