Re: [Qemu-devel] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code

[Michael Davidsaver]

On 02/18/2017 01:38 PM, Peter Maydell wrote:
> On 18 February 2017 at 17:45, Michael Davidsaver  
> wrote:
>> On 02/16/2017 09:11 AM, Peter Maydell wrote:
>>> I haven't actually checked real hardware behaviour, but I think
>>> we can fairly safely implement this as not checking the IPSR
>>> exception field. (We might as well go with the "reads 1 in
>>> handler mode" choice of UNKNOWN that the M3 documents, though.)
>>
>> For what it's worth, I dug up my TI TM4C1294 eval board and re-ran
>> test10.c [1] which is designed to probe this behavior by nesting
>> exceptions PendSV within SVC.  RETTOBASE is 0x800 in ICSR.
> 
> That's a Cortex-M4. From the test it looks like it
> has a different choice of UNKNOWN behaviour for
> the value in Handler mode, so real code in the field
> isn't going to be relying on that and it doesn't
> matter what we choose.

I've been away from arm/m for too long to claim any detailed knowledge
of the documentation.  My intent here is only to provide a data point w/
real hardware, not to interpret it.

> I don't think the test looks at the "what happens if the
> exception in the IPSR exception field isn't actually
> active" case, right?

Correct.

Re: [Qemu-devel] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code

[Peter Maydell]

On 18 February 2017 at 17:45, Michael Davidsaver  wrote:
> On 02/16/2017 09:11 AM, Peter Maydell wrote:
>> I haven't actually checked real hardware behaviour, but I think
>> we can fairly safely implement this as not checking the IPSR
>> exception field. (We might as well go with the "reads 1 in
>> handler mode" choice of UNKNOWN that the M3 documents, though.)
>
> For what it's worth, I dug up my TI TM4C1294 eval board and re-ran
> test10.c [1] which is designed to probe this behavior by nesting
> exceptions PendSV within SVC.  RETTOBASE is 0x800 in ICSR.

That's a Cortex-M4. From the test it looks like it
has a different choice of UNKNOWN behaviour for
the value in Handler mode, so real code in the field
isn't going to be relying on that and it doesn't
matter what we choose.

I don't think the test looks at the "what happens if the
exception in the IPSR exception field isn't actually
active" case, right?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code

[Michael Davidsaver]

On 02/16/2017 09:11 AM, Peter Maydell wrote:
> I haven't actually checked real hardware behaviour, but I think
> we can fairly safely implement this as not checking the IPSR
> exception field. (We might as well go with the "reads 1 in
> handler mode" choice of UNKNOWN that the M3 documents, though.)

For what it's worth, I dug up my TI TM4C1294 eval board and re-ran
test10.c [1] which is designed to probe this behavior by nesting
exceptions PendSV within SVC.  RETTOBASE is 0x800 in ICSR.

> 1..12
> # BASEPRI mask 00e0
> # DEBUG prio 00e0
> ok 1 -  ==  ICSR
> ok 2 -  ==  SHCSR
> # Call SVC
> # In SVC
> ok 3 - 080b == 080b ICSR
> ok 4 - 0080 == 0080 SHCSR
> # In PendSV
> ok 5 - 000e == 000e ICSR
> ok 6 - 0480 == 0480 SHCSR
> # Back in SVC
> ok 7 - 0003 == 0003 Back in SVC
> ok 8 - 080b == 080b ICSR
> ok 9 - 0080 == 0080 SHCSR
> # Back in main
> ok 10 - 0004 == 0004 Back in main
> ok 11 -  ==  ICSR
> ok 12 -  ==  SHCSR
> # Done

[1] https://github.com/mdavidsaver/baremetal/blob/qemutest/test10.c

Re: [Qemu-devel] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code

[Peter Maydell]

On 15 February 2017 at 13:34, Peter Maydell  wrote:
> On 15 February 2017 at 12:46, Alex Bennée  wrote:
>> 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.".)

FWIW, it took me five attempts at rewriting these loops before
I got a version that actually worked. I think I'm going to
take that as justification for my bias towards not rewriting
code that already works :-)

(The change I ended up with just uses a local 'startvec' instead
of modifying 'offset'.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code

[Peter Maydell]

On 15 February 2017 at 14:14, Alex Bennée  wrote:
> 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?

The cover letter has a pointer to the tests I've been using
(plus the usual "run a random stellaris image").

I think the benefit of the asserts is in warding off future
bugs; so I think they're worth keeping where we know something
should never happen but the code is complicated enough that
perhaps a future patch might put things in a state where the
impossible does happen.

Also, number of assert()s is to some extent a coding style
issue, and in this patchset I'm really taking Michael's code
and doing a review/cleanup pass on it, so I've tried not to
do too much rework of the "well I wouldn't have put quite this
many asserts in" flavour.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code

[Alex Bennée]

Peter Maydell  writes:

> On 15 February 2017 at 12:46, Alex Bennée  wrote:
>>
>> Peter Maydell  writes:
>>
>>> From: Michael Davidsaver 
>>>
>>> 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 
>>> [PMM: reworked, various bugs and stylistic cleanups]
>>> Signed-off-by: Peter Maydell 
>>> ---
>>>  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 @@

>>> +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(>gic, 0, MEMTXATTRS_UNSPECIFIED);
>>> -if (irq == 1023)
>>> -hw_error("Interrupt but no vector\n");
>>> -if (irq >= 32)
>>> -irq -= 16;
>>> -return irq;
>>> +CPUARMState *env = >cpu->env;
>>> +const int pending = s->vectpending;
>>> +const int running = nvic_exec_prio(s);
>>> +int pendgroupprio;

Re: [Qemu-devel] [PATCH 3/9] armv7m: Rewrite NVIC to not use any GIC code

[Alex Bennée]

Peter Maydell  writes:

> From: Michael Davidsaver 
>
> 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 
> [PMM: reworked, various bugs and stylistic cleanups]
> Signed-off-by: Peter Maydell 
> ---
>  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 @@
>  #include "hw/sysbus.h"
>  #include "qemu/timer.h"
>  #include "hw/arm/arm.h"
> +#include "target/arm/cpu.h"
>  #include "exec/address-spaces.h"
> -#include "gic_internal.h"
>  #include "qemu/log.h"
> +#include "trace.h"
> +
> +/* IRQ number counting:
> + *
> + * the num-irq property counts the number of external IRQ lines
> + *
> + * NVICState::num_irq counts the total number of exceptions
> + * (external IRQs, the 15 internal exceptions including reset,
> + * and one for the unused exception number 0).
> + *
> + * NVIC_MAX_IRQ is the highest permitted number of external IRQ lines.
> + *
> + * NVIC_MAX_VECTORS is the highest permitted number of exceptions.
> + *
> + * Iterating through all exceptions should typically be done with
> + * for (i = 1; i < s->num_irq; i++) to avoid the unused slot 0.
> + *
> + * The external qemu_irq lines are the NVIC's external IRQ lines,
> + * so line 0 is exception 16.
> + */
> +#define NVIC_FIRST_IRQ 16
> +#define NVIC_MAX_VECTORS 512
> +#define NVIC_MAX_IRQ (NVIC_MAX_VECTORS - NVIC_FIRST_IRQ)
> +
> +/* Effective running priority of the CPU when no exception is active
> + * (higher than the highest possible priority value)
> + */
> +#define NVIC_NOEXC_PRIO 0x100
> +
> +typedef struct VecInfo {
> +int16_t prio; /* priorities can range from -3 to 255 */
> +uint8_t enabled;
> +uint8_t pending;
> +uint8_t active;
> +uint8_t level; /* exceptions <=15 never set level */
> +} VecInfo;
>
>  typedef struct NVICState {
> -GICState gic;
> +/*< private >*/
> +SysBusDevice parent_obj;
> +/*< public >*/
> +
>  ARMCPU *cpu;
>
> +VecInfo vectors[NVIC_MAX_VECTORS];
>  uint32_t prigroup;
>
> +/* vectpending and exception_prio are both cached state that can
> + * be recalculated from the vectors[] array and the prigroup field.
> + */
> +unsigned int vectpending; /* highest prio pending enabled exception */
> +int exception_prio; /* group prio of the highest prio active exception */
> +

nit: the field comments could be more neatly rolled into the block
comment above them.

>  struct {
>  uint32_t control;
>  uint32_t reload;
>  int64_t tick;
>  QEMUTimer *timer;
>  } systick;
> +
>  MemoryRegion sysregmem;
> -MemoryRegion gic_iomem_alias;
>  MemoryRegion container;
> +
>  uint32_t num_irq;
> +qemu_irq excpout;
>  qemu_irq sysresetreq;
>  } NVICState;
>
>  #define TYPE_NVIC "armv7m_nvic"
> -/**
> - * NVICClass:
> - * @parent_reset: the parent class' reset handler.
> - *
> - * A model of the v7M NVIC and System Controller
> - */
> -typedef struct NVICClass {
> -/*< private >*/
> -ARMGICClass parent_class;
> -/*< public >*/
> -DeviceRealize parent_realize;
> -void (*parent_reset)(DeviceState *dev);
> -} NVICClass;
> -
> -#define NVIC_CLASS(klass) \
> -OBJECT_CLASS_CHECK(NVICClass, (klass), TYPE_NVIC)
> -#define NVIC_GET_CLASS(obj) \
> -OBJECT_GET_CLASS(NVICClass, (obj), TYPE_NVIC)
> +
>  #define NVIC(obj) \
>  OBJECT_CHECK(NVICState, (obj), TYPE_NVIC)
>
> @@ -125,47 +156,274 @@ static void systick_reset(NVICState *s)
>  timer_del(s->systick.timer);
>  }
>
> -/* The external routines use the hardware vector numbering, ie. the first
> -   IRQ is #16.  The internal GIC routines use #32 as the first IRQ.  */
> +static int nvic_pending_prio(NVICState *s)
> +{
> +/* return the priority of the current pending interrupt,
> + * or NVIC_NOEXC_PRIO if no interrupt is pending
> + */
> +return s->vectpending ?