[PATCH v3 0/2] hw/arm/virt: Simulate NMI Injection

[PATCH v3 0/2] hw/arm/virt: Simulate NMI Injection

[Gavin Shan]

This series simulates the behavior of receiving NMI interrupt for "virt"
board. First of all, a new interrupt (SError) is supported for each CPU.
The backend is either sending error events through kvm module or emulating
the bahavior when TCG is enabled. The outcome is SError or data abort is
raised to crash guest.

For GICv2 or GICv3, a new IRQ line is added for each CPU and it's connected
to the (above) introduced SError interrupt. The IRQ line of CPU#0 is raised
when HMP/QMP "nmi" is issued, to crash the guest.

Testing
=======

After the HMP/QMP "nmi" is issued in the following 4 environment, the guest is
crashed as expected.

   Accel     Mode                  Crashed    Parameter
   ------------------------------------------------------------------------
   kvm       aarch64               yes        -machine virt -cpu host
   kvm       aarch32(cortex-a15)   yes        -machine virt -cpu host,aarch64=off
   tcg       aarch64               yes        -machine virt -cpu max
   tcg       aarch32(cortex-a15)   yes        -machine virt -cpu cortex-a15

Changelog
=========
v3:
   * Support SError injection for aarch32                 (Richard Henderson)
   * Export the SError injection through IRQ line         (Peter Maydell)
   * Removed RFC tag as it seems in correct track         (Gavin Shan)
v2: 
   * Redesigned to fully exploit SError interrupt

Gavin Shan (2):
  target/arm: Support SError injection
  hw/arm/virt: Simulate NMI injection

 hw/arm/virt.c                      | 34 ++++++++++++++-
 hw/intc/arm_gic_common.c           |  3 ++
 hw/intc/arm_gicv3_common.c         |  3 ++
 include/hw/intc/arm_gic_common.h   |  1 +
 include/hw/intc/arm_gicv3_common.h |  1 +
 target/arm/cpu.c                   | 69 ++++++++++++++++++++++++------
 target/arm/cpu.h                   | 17 +++++---
 target/arm/helper.c                |  6 +++
 target/arm/m_helper.c              |  8 ++++
 9 files changed, 122 insertions(+), 20 deletions(-)

-- 
2.23.0

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

[Gavin Shan]

This supports SError injection, which will be used by "virt" board to
simulating the behavior of NMI injection in next patch. As Peter Maydell
suggested, this adds a new interrupt (ARM_CPU_SERROR), which is parallel
to CPU_INTERRUPT_HARD. The backend depends on if kvm is enabled or not.
kvm_vcpu_ioctl(cpu, KVM_SET_VCPU_EVENTS) is leveraged to inject SError
or data abort to guest. When TCG is enabled, the behavior is simulated
by injecting SError and data abort to guest.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 target/arm/cpu.c      | 69 +++++++++++++++++++++++++++++++++++--------
 target/arm/cpu.h      | 17 ++++++-----
 target/arm/helper.c   |  6 ++++
 target/arm/m_helper.c |  8 +++++
 4 files changed, 81 insertions(+), 19 deletions(-)

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);
 }
 
 void arm_register_pre_el_change_hook(ARMCPU *cpu, ARMELChangeHookFn *hook,
@@ -449,6 +449,9 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
             return false;
         }
         return !(env->daif & PSTATE_I);
+    case EXCP_SERROR:
+       pstate_unmasked = !(env->daif & PSTATE_A);
+       break;
     default:
         g_assert_not_reached();
     }
@@ -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:
@@ -585,7 +598,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     CPUClass *cc = CPU_GET_CLASS(cs);
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    bool ret = false;
+    uint32_t excp_idx;
 
     /* ARMv7-M interrupt masking works differently than -A or -R.
      * There is no FIQ/IRQ distinction. Instead of I and F bits
@@ -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;
+    }
+
+    return false;
+
+found:
+    cs->exception_index = excp_idx;
+    cc->do_interrupt(cs);
+    return true;
 }
 #endif
 
@@ -656,7 +682,8 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level)
         [ARM_CPU_IRQ] = CPU_INTERRUPT_HARD,
         [ARM_CPU_FIQ] = CPU_INTERRUPT_FIQ,
         [ARM_CPU_VIRQ] = CPU_INTERRUPT_VIRQ,
-        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ
+        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ,
+        [ARM_CPU_SERROR] = CPU_INTERRUPT_SERROR,
     };
 
     if (level) {
@@ -676,6 +703,7 @@ static void arm_cpu_set_irq(void *opaque, int irq, int level)
         break;
     case ARM_CPU_IRQ:
     case ARM_CPU_FIQ:
+    case ARM_CPU_SERROR:
         if (level) {
             cpu_interrupt(cs, mask[irq]);
         } else {
@@ -693,8 +721,10 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level)
     ARMCPU *cpu = opaque;
     CPUARMState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
+    struct kvm_vcpu_events events;
     uint32_t linestate_bit;
     int irq_id;
+    bool inject_irq = true;
 
     switch (irq) {
     case ARM_CPU_IRQ:
@@ -705,6 +735,14 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level)
         irq_id = KVM_ARM_IRQ_CPU_FIQ;
         linestate_bit = CPU_INTERRUPT_FIQ;
         break;
+    case ARM_CPU_SERROR:
+        if (!kvm_has_vcpu_events()) {
+            return;
+        }
+
+        inject_irq = false;
+        linestate_bit = CPU_INTERRUPT_SERROR;
+        break;
     default:
         g_assert_not_reached();
     }
@@ -714,7 +752,14 @@ static void arm_cpu_kvm_set_irq(void *opaque, int irq, int level)
     } else {
         env->irq_line_state &= ~linestate_bit;
     }
-    kvm_arm_set_irq(cs->cpu_index, KVM_ARM_IRQ_TYPE_CPU, irq_id, !!level);
+
+    if (inject_irq) {
+        kvm_arm_set_irq(cs->cpu_index, KVM_ARM_IRQ_TYPE_CPU, irq_id, !!level);
+    } else if (level) {
+        memset(&events, 0, sizeof(events));
+        events.exception.serror_pending = 1;
+        kvm_vcpu_ioctl(cs, KVM_SET_VCPU_EVENTS, &events);
+    }
 #endif
 }
 
@@ -1064,9 +1109,9 @@ static void arm_cpu_initfn(Object *obj)
         /* VIRQ and VFIQ are unused with KVM but we add them to maintain
          * the same interface as non-KVM CPUs.
          */
-        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);
     }
 
     qdev_init_gpio_out(DEVICE(cpu), cpu->gt_timer_outputs,
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 0b3036c484..9a82378d6d 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -49,6 +49,7 @@
 #define EXCP_LAZYFP         20   /* v7M fault during lazy FP stacking */
 #define EXCP_LSERR          21   /* v8M LSERR SecureFault */
 #define EXCP_UNALIGNED      22   /* v7M UNALIGNED UsageFault */
+#define EXCP_SERROR         23   /* SError Interrupt */
 /* NB: add new EXCP_ defines to the array in arm_log_exception() too */
 
 #define ARMV7M_EXCP_RESET   1
@@ -79,9 +80,10 @@ enum {
 };
 
 /* ARM-specific interrupt pending bits.  */
-#define CPU_INTERRUPT_FIQ   CPU_INTERRUPT_TGT_EXT_1
-#define CPU_INTERRUPT_VIRQ  CPU_INTERRUPT_TGT_EXT_2
-#define CPU_INTERRUPT_VFIQ  CPU_INTERRUPT_TGT_EXT_3
+#define CPU_INTERRUPT_FIQ    CPU_INTERRUPT_TGT_EXT_1
+#define CPU_INTERRUPT_VIRQ   CPU_INTERRUPT_TGT_EXT_2
+#define CPU_INTERRUPT_VFIQ   CPU_INTERRUPT_TGT_EXT_3
+#define CPU_INTERRUPT_SERROR CPU_INTERRUPT_TGT_EXT_4
 
 /* The usual mapping for an AArch64 system register to its AArch32
  * counterpart is for the 32 bit world to have access to the lower
@@ -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
 
 /* ARM-specific extra insn start words:
  * 1: Conditional execution bits
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7d15d5c933..0a3b7e5be2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8487,6 +8487,7 @@ void arm_log_exception(int idx)
             [EXCP_LAZYFP] = "v7M exception during lazy FP stacking",
             [EXCP_LSERR] = "v8M LSERR UsageFault",
             [EXCP_UNALIGNED] = "v7M UNALIGNED UsageFault",
+            [EXCP_SERROR] = "SError Interrupt",
         };
 
         if (idx >= 0 && idx < ARRAY_SIZE(excnames)) {
@@ -8789,6 +8790,7 @@ static void arm_cpu_do_interrupt_aarch32_hyp(CPUState *cs)
         addr = 0x0c;
         break;
     case EXCP_DATA_ABORT:
+    case EXCP_SERROR:
         env->cp15.dfar_s = env->exception.vaddress;
         qemu_log_mask(CPU_LOG_INT, "...with HDFAR 0x%x\n",
                       (uint32_t)env->exception.vaddress);
@@ -8917,6 +8919,7 @@ static void arm_cpu_do_interrupt_aarch32(CPUState *cs)
         offset = 4;
         break;
     case EXCP_DATA_ABORT:
+    case EXCP_SERROR:
         A32_BANKED_CURRENT_REG_SET(env, dfsr, env->exception.fsr);
         A32_BANKED_CURRENT_REG_SET(env, dfar, env->exception.vaddress);
         qemu_log_mask(CPU_LOG_INT, "...with DFSR 0x%x DFAR 0x%x\n",
@@ -9078,6 +9081,9 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
     case EXCP_VFIQ:
         addr += 0x100;
         break;
+    case EXCP_SERROR:
+        addr += 0x180;
+        break;
     default:
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 33d414a684..a7271cc386 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2211,6 +2211,14 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
          * v7m_preserve_fp_state() helper function.
          */
         break;
+    case EXCP_SERROR:
+        env->v7m.cfsr[M_REG_NS] |=
+            (R_V7M_CFSR_PRECISERR_MASK | R_V7M_CFSR_BFARVALID_MASK);
+        env->v7m.bfar = env->exception.vaddress;
+        qemu_log_mask(CPU_LOG_INT,
+                      "...with CFSR.PRECISERR and BFAR 0x%x\n",
+                      env->v7m.bfar);
+        break;
     default:
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
         return; /* Never happens.  Keep compiler happy.  */
-- 
2.23.0

[PATCH v3 2/2] hw/arm/virt: Simulate NMI injection

[Gavin Shan]

This implements the backend to support HMP/QMP "nmi" command, which is
used to inject NMI interrupt to crash guest for debugging purpose. As
ARM architecture doesn't have NMI supported, so we're simulating the
behaviour by injecting SError or data abort to guest for "virt" board.

An additonal IRQ line is introduced for SError on each CPU. The IRQ line
is connected to SError exception handler. The IRQ line on CPU#0 is raised
when HMP/QMP "nmi" is issued to inject SError or data abort to crash guest.
Note the IRQ line can be shared with other devices who want to have the
capability of reporting errors in future.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c                      | 34 +++++++++++++++++++++++++++++-
 hw/intc/arm_gic_common.c           |  3 +++
 hw/intc/arm_gicv3_common.c         |  3 +++
 include/hw/intc/arm_gic_common.h   |  1 +
 include/hw/intc/arm_gicv3_common.h |  1 +
 5 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f788fe27d6..78549faa75 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -71,6 +71,8 @@
 #include "hw/mem/pc-dimm.h"
 #include "hw/mem/nvdimm.h"
 #include "hw/acpi/generic_event_device.h"
+#include "sysemu/hw_accel.h"
+#include "hw/nmi.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -690,7 +692,7 @@ static void create_gic(VirtMachineState *vms)
         } else if (vms->virt) {
             qemu_irq irq = qdev_get_gpio_in(vms->gic,
                                             ppibase + ARCH_GIC_MAINT_IRQ);
-            sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus, irq);
+            sysbus_connect_irq(gicbusdev, i + 5 * smp_cpus, irq);
         }
 
         qdev_connect_gpio_out_named(cpudev, "pmu-interrupt", 0,
@@ -704,6 +706,8 @@ static void create_gic(VirtMachineState *vms)
                            qdev_get_gpio_in(cpudev, ARM_CPU_VIRQ));
         sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus,
                            qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ));
+        sysbus_connect_irq(gicbusdev, i + 4 * smp_cpus,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_SERROR));
     }
 
     fdt_add_gic_node(vms);
@@ -2026,10 +2030,36 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
     return requested_pa_size > 40 ? requested_pa_size : 0;
 }
 
+
+static void do_inject_serror(CPUState *cpu, run_on_cpu_data data)
+{
+    VirtMachineState *vms = data.host_ptr;
+    GICv3State *gicv3;
+    GICState *gicv2;
+
+    cpu_synchronize_state(cpu);
+
+    if (vms->gic_version == 3) {
+        gicv3 = ARM_GICV3_COMMON(OBJECT(vms->gic));
+        qemu_irq_raise(gicv3->cpu[0].parent_serror);
+    } else {
+        gicv2 = ARM_GIC_COMMON(OBJECT(vms->gic));
+        qemu_irq_raise(gicv2->parent_serror[0]);
+    }
+}
+
+static void virt_inject_serror(NMIState *n, int cpu_index, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(n);
+
+    async_run_on_cpu(first_cpu, do_inject_serror, RUN_ON_CPU_HOST_PTR(vms));
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
     HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+    NMIClass *nc = NMI_CLASS(oc);
 
     mc->init = machvirt_init;
     /* Start with max_cpus set to 512, which is the maximum supported by KVM.
@@ -2058,6 +2088,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug_request = virt_machine_device_unplug_request_cb;
     mc->numa_mem_supported = true;
     mc->auto_enable_numa_with_memhp = true;
+    nc->nmi_monitor_handler = virt_inject_serror;
 }
 
 static void virt_instance_init(Object *obj)
@@ -2141,6 +2172,7 @@ static const TypeInfo virt_machine_info = {
     .instance_init = virt_instance_init,
     .interfaces = (InterfaceInfo[]) {
          { TYPE_HOTPLUG_HANDLER },
+         { TYPE_NMI },
          { }
     },
 };
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index e6c4fe7a5a..f39cefdeea 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -155,6 +155,9 @@ void gic_init_irqs_and_mmio(GICState *s, qemu_irq_handler handler,
     for (i = 0; i < s->num_cpu; i++) {
         sysbus_init_irq(sbd, &s->parent_vfiq[i]);
     }
+    for (i = 0; i < s->num_cpu; i++) {
+        sysbus_init_irq(sbd, &s->parent_serror[i]);
+    }
     if (s->virt_extn) {
         for (i = 0; i < s->num_cpu; i++) {
             sysbus_init_irq(sbd, &s->maintenance_irq[i]);
diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 58ef65f589..19a04449a0 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -288,6 +288,9 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
     for (i = 0; i < s->num_cpu; i++) {
         sysbus_init_irq(sbd, &s->cpu[i].parent_vfiq);
     }
+    for (i = 0; i < s->num_cpu; i++) {
+        sysbus_init_irq(sbd, &s->cpu[i].parent_serror);
+    }
 
     memory_region_init_io(&s->iomem_dist, OBJECT(s), ops, s,
                           "gicv3_dist", 0x10000);
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index b5585fec45..4cdeed7725 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -70,6 +70,7 @@ typedef struct GICState {
     qemu_irq parent_fiq[GIC_NCPU];
     qemu_irq parent_virq[GIC_NCPU];
     qemu_irq parent_vfiq[GIC_NCPU];
+    qemu_irq parent_serror[GIC_NCPU];
     qemu_irq maintenance_irq[GIC_NCPU];
 
     /* GICD_CTLR; for a GIC with the security extensions the NS banked version
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index 31ec9a1ae4..a025a04727 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -152,6 +152,7 @@ struct GICv3CPUState {
     qemu_irq parent_fiq;
     qemu_irq parent_virq;
     qemu_irq parent_vfiq;
+    qemu_irq parent_serror;
     qemu_irq maintenance_irq;
 
     /* Redistributor */
-- 
2.23.0

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

[Richard Henderson]

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.

> @@ -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.

> @@ -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.

> -        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.

> @@ -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.


r~

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

[Marc Zyngier]

Hi Gavin,

On 2020-02-14 05:59, Gavin Shan wrote:
> This supports SError injection, which will be used by "virt" board to
> simulating the behavior of NMI injection in next patch. As Peter 
> Maydell
> suggested, this adds a new interrupt (ARM_CPU_SERROR), which is 
> parallel
> to CPU_INTERRUPT_HARD. The backend depends on if kvm is enabled or not.
> kvm_vcpu_ioctl(cpu, KVM_SET_VCPU_EVENTS) is leveraged to inject SError
> or data abort to guest. When TCG is enabled, the behavior is simulated
> by injecting SError and data abort to guest.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  target/arm/cpu.c      | 69 +++++++++++++++++++++++++++++++++++--------
>  target/arm/cpu.h      | 17 ++++++-----
>  target/arm/helper.c   |  6 ++++
>  target/arm/m_helper.c |  8 +++++
>  4 files changed, 81 insertions(+), 19 deletions(-)
> 

[...]

> @@ -656,7 +682,8 @@ static void arm_cpu_set_irq(void *opaque, int irq,
> int level)
>          [ARM_CPU_IRQ] = CPU_INTERRUPT_HARD,
>          [ARM_CPU_FIQ] = CPU_INTERRUPT_FIQ,
>          [ARM_CPU_VIRQ] = CPU_INTERRUPT_VIRQ,
> -        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ
> +        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ,
> +        [ARM_CPU_SERROR] = CPU_INTERRUPT_SERROR,

I'm a bit concerned with this. It makes sense for a host, but doesn't
allow the SError signal to be virtualised (there should be a VSError
signal in this list that can be injected via HCR_EL2.VA, just like
VIRQ is injected by HCR_EL2.VI).

Given that people use QEMU as a development platform for hypervisors,
I'd really like this functionality to be supported from day-1.

There is also the whole RAS stuff which quite a lot of work, but let's
start at least with the full ARMv8.0 semantics.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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

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

[Gavin Shan]

Hi Marc,

On 2/16/20 11:34 PM, Marc Zyngier wrote:
> On 2020-02-14 05:59, Gavin Shan wrote:
>> This supports SError injection, which will be used by "virt" board to
>> simulating the behavior of NMI injection in next patch. As Peter Maydell
>> suggested, this adds a new interrupt (ARM_CPU_SERROR), which is parallel
>> to CPU_INTERRUPT_HARD. The backend depends on if kvm is enabled or not.
>> kvm_vcpu_ioctl(cpu, KVM_SET_VCPU_EVENTS) is leveraged to inject SError
>> or data abort to guest. When TCG is enabled, the behavior is simulated
>> by injecting SError and data abort to guest.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>  target/arm/cpu.c      | 69 +++++++++++++++++++++++++++++++++++--------
>>  target/arm/cpu.h      | 17 ++++++-----
>>  target/arm/helper.c   |  6 ++++
>>  target/arm/m_helper.c |  8 +++++
>>  4 files changed, 81 insertions(+), 19 deletions(-)
>>
> 
> [...]
> 
>> @@ -656,7 +682,8 @@ static void arm_cpu_set_irq(void *opaque, int irq,
>> int level)
>>          [ARM_CPU_IRQ] = CPU_INTERRUPT_HARD,
>>          [ARM_CPU_FIQ] = CPU_INTERRUPT_FIQ,
>>          [ARM_CPU_VIRQ] = CPU_INTERRUPT_VIRQ,
>> -        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ
>> +        [ARM_CPU_VFIQ] = CPU_INTERRUPT_VFIQ,
>> +        [ARM_CPU_SERROR] = CPU_INTERRUPT_SERROR,
> 
> I'm a bit concerned with this. It makes sense for a host, but doesn't
> allow the SError signal to be virtualised (there should be a VSError
> signal in this list that can be injected via HCR_EL2.VA, just like
> VIRQ is injected by HCR_EL2.VI).
> 
> Given that people use QEMU as a development platform for hypervisors,
> I'd really like this functionality to be supported from day-1.
> 
> There is also the whole RAS stuff which quite a lot of work, but let's
> start at least with the full ARMv8.0 semantics.
> 

Thanks for the comments. Yes, I think it's reasonable to support virtual
SError as well. Lets have a separate patch to support it in v4. I think
you were talking about HCR_EL2.VSE, which is defined as below in target/arm/cpu.h:

#define HCR_VSE       (1ULL << 8)

Thanks,
Gavin