[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code
From: |
Alex Bennée |
Subject: |
Re: [Qemu-arm] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code |
Date: |
Wed, 15 Feb 2017 14:14:07 +0000 |
User-agent: |
mu4e 0.9.19; emacs 25.2.3 |
Peter Maydell <address@hidden> writes:
> On 15 February 2017 at 12:46, Alex Bennée <address@hidden> wrote:
>>
>> Peter Maydell <address@hidden> writes:
>>
>>> From: Michael Davidsaver <address@hidden>
>>>
>>> Despite some superficial similarities of register layout, the
>>> M-profile NVIC is really very different from the A-profile GIC.
>>> Our current attempt to reuse the GIC code means that we have
>>> significant bugs in our NVIC.
>>>
>>> Implement the NVIC as an entirely separate device, to give
>>> us somewhere we can get the behaviour correct.
>>>
>>> This initial commit does not attempt to implement exception
>>> priority escalation, since the GIC-based code didn't either.
>>> It does fix a few bugs in passing:
>>> * ICSR.RETTOBASE polarity was wrong and didn't account for
>>> internal exceptions
>>> * ICSR.VECTPENDING was 16 too high if the pending exception
>>> was for an external interrupt
>>> * UsageFault, BusFault and MemFault were not disabled on reset
>>> as they are supposed to be
>>>
>>> Signed-off-by: Michael Davidsaver <address@hidden>
>>> [PMM: reworked, various bugs and stylistic cleanups]
>>> Signed-off-by: Peter Maydell <address@hidden>
>>> ---
>>> hw/intc/armv7m_nvic.c | 721
>>> ++++++++++++++++++++++++++++++++++++++++----------
>>> hw/intc/trace-events | 15 ++
>>> 2 files changed, 592 insertions(+), 144 deletions(-)
>>>
>>> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
>>> index ce22001..e319077 100644
>>> --- a/hw/intc/armv7m_nvic.c
>>> +++ b/hw/intc/armv7m_nvic.c
>>> @@ -17,48 +17,79 @@
<snip>
>>> +static bool nvic_rettobase(NVICState *s)
>>> +{
>>> + int irq, nhand = 0;
>>> +
>>> + for (irq = ARMV7M_EXCP_RESET; irq < s->num_irq; irq++) {
>>> + if (s->vectors[irq].active) {
>>
>> I would expect && !what the ISPR is reporting. However looking at the
>> code it doesn't look like we ever report an exception in the IPSR
>> (assuming HELPER(v7m_mrs) is the right one).
>
> See xpsr_read() in target/arm/cpu.h -- we report
> v7m.exception in the IPSR bits.
So depending on your re-reading of the latest spec should we we count
s->vectors[irq].active && s->vect_pending != irq?
>
>>> + nhand++;
>>> + if (nhand == 2) {
>>> + break;
>>> + }
>>> + }
>>> + }
>>> +
>>> + return nhand == 1;
>>> +}
>>> +
>>> +/* Return the value of the ISCR ISRPENDING bit:
>>> + * 1 if an external interrupt is pending
>>> + * 0 if no external interrupt is pending
>>> + */
>>> +static bool nvic_isrpending(NVICState *s)
>>> +{
>>> + int irq;
>>> +
>>> + /* We can shortcut if the highest priority pending interrupt
>>> + * happens to be external or if there is nothing pending.
>>> + */
>>> + if (s->vectpending > NVIC_FIRST_IRQ) {
>>> + return true;
>>> + }
>>> + if (s->vectpending == 0) {
>>> + return false;
>>> + }
>>> +
>>> + for (irq = NVIC_FIRST_IRQ; irq < s->num_irq; irq++) {
>>
>> Should NVIC_FIRST_IRQ be renamed to NVIC_FIRST_EXTERNAL_IRQ?
>
> The internal ones aren't IRQs, they're exceptions.
> The terminology is a bit confusing, though.
Ahh ok. Maybe just a comment to that effect by the define?
>
>>> + if (s->vectors[irq].pending) {
>>> + return true;
>>> + }
>>> + }
>>> + return false;
>>> +}
>>> +
>>> +/* Return a mask word which clears the subpriority bits from
>>> + * a priority value for an M-profile exception, leaving only
>>> + * the group priority.
>>> + */
>>> +static inline uint32_t nvic_gprio_mask(NVICState *s)
>>> +{
>>> + return ~0U << (s->prigroup + 1);
>>
>> I wonder if it is worth adding MAKE_32BIT_MASK to bitops.h?
>
> MAKE_64BIT_MASK would work here too. This is the same way
> arm_gicv3_cpuif.c writes it, though.
>
>>> +/* caller must call nvic_irq_update() after this */
>>> +static void set_prio(NVICState *s, unsigned irq, uint8_t prio)
>>> +{
>>> + assert(irq > ARMV7M_EXCP_NMI); /* only use for configurable prios */
>>> + assert(irq < s->num_irq);
>>> +
>>> + s->vectors[irq].prio = prio;
>>
>> So this means the negative priorities are hardwired parts of the NVIC
>> for NMI/RESET? Maybe we should make that clearer in the comment for
>> VecInfo.
>
> Sure. (Yes, the priorities for NMI and HardFault are architecturally
> hardwired. I suspect that in hardware they are not in fact represented
> as negative numbers :-))
>
>>> /* Make pending IRQ active. */
>>> int armv7m_nvic_acknowledge_irq(void *opaque)
>>> {
>>> NVICState *s = (NVICState *)opaque;
>>> - uint32_t irq;
>>> -
>>> - irq = gic_acknowledge_irq(&s->gic, 0, MEMTXATTRS_UNSPECIFIED);
>>> - if (irq == 1023)
>>> - hw_error("Interrupt but no vector\n");
>>> - if (irq >= 32)
>>> - irq -= 16;
>>> - return irq;
>>> + CPUARMState *env = &s->cpu->env;
>>> + const int pending = s->vectpending;
>>> + const int running = nvic_exec_prio(s);
>>> + int pendgroupprio;
>>> + VecInfo *vec;
>>> +
>>> + assert(pending > ARMV7M_EXCP_RESET && pending < s->num_irq);
>>> +
>>> + vec = &s->vectors[pending];
>>> +
>>> + assert(vec->enabled);
>>> + assert(vec->pending);
>>> +
>>> + pendgroupprio = vec->prio & nvic_gprio_mask(s);
>>> + assert(pendgroupprio < running);
>>
>> I'm all for asserts to declare what the API is expecting. These are
>> starting to look like being unsure of the nvic being in the correct
>> state. Are they left over from debugging?
>
> They're mostly left over from Michael's code where I didn't
> see any reason to specifically delete them. For this particular
> assert the situation is quite complicated -- we know the
> pending priority must be such that we can take this
> exception, but that is only true because the code
> in target/arm is required to only try to acknowledge
> (take) the exception if the priority permits it,
> which in turn is the result of a combination of the
> conditions that the NVIC applies to decide whether to
> assert the interrupt line and the conditions applied
> in arm_v7m_cpu_exec_interrupt() to decide whether to
> call the CPU do_interrupt hook. That's quite a lot of
> moving parts which need to all agree, which I think makes
> an assert() justified.
I guess it would be easier to remove the asserts if we had run test
cases that explicitly exercised all this code. What are you currently
running to test this code?
>
>>> @@ -428,28 +713,81 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr
>>> addr,
>>> {
>>> NVICState *s = (NVICState *)opaque;
>>> uint32_t offset = addr;
>>> - int i;
>>> + unsigned i, end;
>>> uint32_t val;
>>>
>>> switch (offset) {
>>> + /* reads of set and clear both return the status */
>>> + case 0x100 ... 0x13f: /* NVIC Set enable */
>>> + offset += 0x80;
>>> + /* fall through */
>>> + case 0x180 ... 0x1bf: /* NVIC Clear enable */
>>> + val = 0;
>>> + offset = offset - 0x180 + NVIC_FIRST_IRQ; /* vector # */
>>
>> Can we not calculate a vector index rather than abusing the meaning of
>> offset while switching on it?
>
> Yeah, we could. (This is just a case where I thought "I could
> rewrite the code Michael did initially, but it doesn't quite
> reach my threshold for doing that just because it's not the
> way I'd have written it.".)
>
>
>>> + /* include space for internal exception vectors */
>>> + s->num_irq += NVIC_FIRST_IRQ;
>>> +
>>> + /* The NVIC and system controller register area starts at 0xe000e000
>>> + * and looks like this:
>>
>> s/system controller register area/System Control Space (SCS)/ to make it
>> easier to find in the ARM ARM.
>
> OK.
>
> thanks
> -- PMM
--
Alex Bennée
- Re: [Qemu-arm] [Qemu-devel] [PATCH 2/9] armv7m: Implement reading and writing of PRIGROUP, (continued)
- [Qemu-arm] [PATCH 5/9] arm: gic: Remove references to NVIC, Peter Maydell, 2017/02/02
- [Qemu-arm] [PATCH 1/9] armv7m: Rename nvic_state to NVICState, Peter Maydell, 2017/02/02
- [Qemu-arm] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code, Peter Maydell, 2017/02/02
- Re: [Qemu-arm] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code, Peter Maydell, 2017/02/16
- Re: [Qemu-arm] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code, Michael Davidsaver, 2017/02/18
- Re: [Qemu-arm] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code, Peter Maydell, 2017/02/18
- Re: [Qemu-arm] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code, Michael Davidsaver, 2017/02/19
- Re: [Qemu-arm] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code, Peter Maydell, 2017/02/16
Re: [Qemu-arm] [PATCH 0/9] Rewrite NVIC to not depend on the GIC, Peter Maydell, 2017/02/10