Re: [PATCH v3 1/2] target/arm: Support SError injection

[Gavin Shan]

On 2/16/20 2:41 PM, Richard Henderson wrote:

On 2/13/20 9:59 PM, Gavin Shan wrote:

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b0762a76c4..180e29fb83 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -78,7 +78,7 @@ static bool arm_cpu_has_work(CPUState *cs)
          && cs->interrupt_request &
          (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD
           | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ
-         | CPU_INTERRUPT_EXITTB);
+         | ARM_CPU_SERROR | CPU_INTERRUPT_EXITTB);

CPU_INTERRUPT_SERROR, not ARM_CPU_SERROR.

Yep, will be corrected in v4.

@@ -570,6 +573,16 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
              goto found;
          }
      }
+
+    if (interrupt_request & CPU_INTERRUPT_SERROR) {
+        excp_idx = EXCP_SERROR;
+        target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure);
+        if (arm_excp_unmasked(cs, excp_idx, target_el,
+                              cur_el, secure, hcr_el2)) {
+            goto found;
+        }
+    }
+
      return false;

found:

If you're intending to use Serror for NMI, perhaps it should be the first bit
tested, not the last.  Otherwise some bug that leaves a normal hard interrupt
line high will keep delivering the interrupt, and not the Serror.

As the comment at the top of the function says, the priority is implementation
defined, so we can put it anywhere we like.

Yes, SError will have highest priority in v4.

@@ -594,13 +607,26 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int 
interrupt_request)
       * (which depends on state like BASEPRI, FAULTMASK and the
       * currently active exception).
       */
-    if (interrupt_request & CPU_INTERRUPT_HARD
-        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
-        cs->exception_index = EXCP_IRQ;
-        cc->do_interrupt(cs);
-        ret = true;
+    if (!armv7m_nvic_can_take_pending_exception(env->nvic)) {
+        return false;
+    }
+
+    if (interrupt_request & CPU_INTERRUPT_HARD) {
+        excp_idx = EXCP_IRQ;
+        goto found;
      }
-    return ret;
+
+    if (interrupt_request & CPU_INTERRUPT_SERROR) {
+        excp_idx = EXCP_SERROR;
+        goto found;
+    }

Likewise.

Thanks, SError will have highest priority in v4.

-        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 4);
+        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 5);
      } else {
-        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 4);
+        qdev_init_gpio_in(DEVICE(cpu), arm_cpu_set_irq, 5);

I wonder if we should have an ARM_CPU_NUM_IRQ define so that this is more
automatic.

Yes, It makes sense. ARM_CPU_NUM_IRQ will be introduced in v4.

@@ -98,10 +100,11 @@ enum {
  #endif

/* Meanings of the ARMCPU object's four inbound GPIO lines */

-#define ARM_CPU_IRQ 0
-#define ARM_CPU_FIQ 1
-#define ARM_CPU_VIRQ 2
-#define ARM_CPU_VFIQ 3
+#define ARM_CPU_IRQ    0
+#define ARM_CPU_FIQ    1
+#define ARM_CPU_VIRQ   2
+#define ARM_CPU_VFIQ   3
+#define ARM_CPU_SERROR 4

Comment is now wrong about the count.

Yes, It will be corrected to "ARMCPU object's inbound GPIO lines" in v4.

Thanks,
Gavin