Re: [Qemu-devel] [PATCH v2 03/13] armv7m: Rewrite NVIC to not use any GIC code

[Alex Bennée]

Peter Maydell  writes:

> On 16 February 2017 at 16:35, Peter Maydell  wrote:
>> 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 
>
>> +case 0x400 ... 0x5ef: /* NVIC Priority */
>> +startvec = 8 * (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */
>> +
>> +for (i = 0; i < size; i++) {
>
> Just noticed this line should be
> +for (i = 0; i < size && startvec + i < s->num_irq; i++) {
>
> which brings it into line with the nvic_sysreg_read() code
> and prevents an assert() in set_prio() if the guest writes to
> registers beyond the end of the implemented IRQ range.
>
>> +set_prio(s, startvec + i, (value >> (i * 8)) & 0xff);
>> +}
>> +nvic_irq_update(s);
>> +return;
>
> Unless there's some other problem that means I need
> to respin anyway I propose to just squash in that fix.

With the squashed fix:

Reviewed-by: Alex Bennée 

>
> thanks
> -- PMM


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 03/13] armv7m: Rewrite NVIC to not use any GIC code

[Peter Maydell]

On 16 February 2017 at 16:35, Peter Maydell  wrote:
> 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 

> +case 0x400 ... 0x5ef: /* NVIC Priority */
> +startvec = 8 * (offset - 0x400) + NVIC_FIRST_IRQ; /* vector # */
> +
> +for (i = 0; i < size; i++) {

Just noticed this line should be
+for (i = 0; i < size && startvec + i < s->num_irq; i++) {

which brings it into line with the nvic_sysreg_read() code
and prevents an assert() in set_prio() if the guest writes to
registers beyond the end of the implemented IRQ range.

> +set_prio(s, startvec + i, (value >> (i * 8)) & 0xff);
> +}
> +nvic_irq_update(s);
> +return;

Unless there's some other problem that means I need
to respin anyway I propose to just squash in that fix.

thanks
-- PMM

[Qemu-devel] [PATCH v2 03/13] armv7m: Rewrite NVIC to not use any GIC code

[Peter Maydell]

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 | 738 --
 hw/intc/trace-events  |  15 +
 2 files changed, 609 insertions(+), 144 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index ce22001..f45b897 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -17,48 +17,88 @@
 #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.
+ *
+ * In the terminology of the architecture manual, "interrupts" are
+ * a subcategory of exception referring to the external interrupts
+ * (which are exception numbers NVIC_FIRST_IRQ and upward).
+ * For historical reasons QEMU tends to use "interrupt" and
+ * "exception" more or less interchangeably.
+ */
+#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 {
+/* Exception priorities can range from -3 to 255; only the unmodifiable
+ * priority values for RESET, NMI and HardFault can be negative.
+ */
+int16_t prio;
+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 */
+
 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 +165,283 @@ 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 ?