Re: [PATCH v13 00/24] target/arm: Implement FEAT_NMI and FEAT_GICv3_NMI

[Peter Maydell]

On Sun, 7 Apr 2024 at 09:19, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
>
> This patch set implements FEAT_NMI and FEAT_GICv3_NMI for ARMv8. These
> introduce support for a new category of interrupts in the architecture
> which we can use to provide NMI like functionality.

I had one last loose end I wanted to tidy up, and I got round
to working through reading the spec about it today. This is
the question of what the "is NMI enabled?" test should be
in the code in arm_gicv3_cpuif.c.

The spec wording isn't always super clear, but there are several
things here:

 * FEAT_NMI : the changes to the CPU proper which implement
   superpriority for IRQ and FIQ, PSTATE.ALLINT, etc etc.
 * FEAT_GICv3_NMI : the changes to the CPU interface for
   GICv3 NMI handling. Any CPU with FEAT_NMI and FEAT_GICv3
   must have this.
 * NMI support in the IRI (Interrupt Routing Infrastructure,
   i.e. all the bits of the GIC that aren't the cpuif; the
   distributor and redistributors). Table 3-1 in the GIC spec
   says that you can have an IRI without NMI support connected
   to a CPU which does have NMI support. This is what the ID
   register bit GICD_TYPER.NMI reports.

At the moment this patchset conflates FEAT_GICv3_NMI and
the NMI support in the IRI. The effect of this is that we
allow a machine model to create a CPU with FEAT_NMI but
without FEAT_GICv3_NMI in the cpuif, and we don't allow
a setup where the CPU and cpuif have NMI support but the
IRI does not. (This will actually happen with this patchset
with the sbsa-ref machine and -cpu max, because we haven't
(yet) made sbsa-ref enable NMI in the GIC device when the
CPU has NMI support.)

For a Linux guest this doesn't make much difference, because
Linux will only enable NMI support if it finds it in both
the IRI and the CPU, but I think it would be better to
get the enable-tests right as these can be awkward to change
after the fact in a backwards-compatible way.

I think this is easy to fix -- we can add a new bool field
GICv3CPUState::nmi_support which we initialize in
gicv3_init_cpuif() if the CPU has FEAT_NMI, and make the
checks in arm_gicv3_cpuif.c check cs->nmi_support instead
of cs->gic->nmi_support. That looks like this squashed into
patch 18:

diff --git a/include/hw/intc/arm_gicv3_common.h
b/include/hw/intc/arm_gicv3_common.h
index 88533749ebb..cd09bee3bc4 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -225,6 +225,13 @@ struct GICv3CPUState {

     /* This is temporary working state, to avoid a malloc in gicv3_update() */
     bool seenbetter;
+
+    /*
+     * Whether the CPU interface has NMI support (FEAT_GICv3_NMI). The
+     * CPU interface may support NMIs even when the GIC proper (what the
+     * spec calls the IRI; the redistributors and distributor) does not.
+     */
+    bool nmi_support;
 };

 /*
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 2457b7bca23..715909d0f7d 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -21,6 +21,7 @@
 #include "hw/irq.h"
 #include "cpu.h"
 #include "target/arm/cpregs.h"
+#include "target/arm/cpu-features.h"
 #include "sysemu/tcg.h"
 #include "sysemu/qtest.h"

@@ -839,7 +840,7 @@ static int icc_highest_active_prio(GICv3CPUState *cs)
      */
     int i;

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         /*
          * If an NMI is active this takes precedence over anything else
          * for priority purposes; the NMI bit is only in the AP1R0 bit.
@@ -1285,7 +1286,7 @@ static void icc_drop_prio(GICv3CPUState *cs, int grp)
             continue;
         }

-        if (i == 0 && cs->gic->nmi_support && (*papr & ICC_AP1R_EL1_NMI)) {
+        if (i == 0 && cs->nmi_support && (*papr & ICC_AP1R_EL1_NMI)) {
             *papr &= (~ICC_AP1R_EL1_NMI);
             break;
         }
@@ -1324,7 +1325,7 @@ static int icc_highest_active_group(GICv3CPUState *cs)
      */
     int i;

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         if (cs->icc_apr[GICV3_G1][0] & ICC_AP1R_EL1_NMI) {
             return GICV3_G1;
         }
@@ -1787,7 +1788,7 @@ static void icc_ap_write(CPUARMState *env, const
ARMCPRegInfo *ri,
         return;
     }

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         cs->icc_apr[grp][regno] = value & (0xFFFFFFFFU | ICC_AP1R_EL1_NMI);
     } else {
         cs->icc_apr[grp][regno] = value & 0xFFFFFFFFU;
@@ -1901,7 +1902,7 @@ static uint64_t icc_rpr_read(CPUARMState *env,
const ARMCPRegInfo *ri)
         }
     }

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         /* NMI info is reported in the high bits of RPR */
         if (arm_feature(env, ARM_FEATURE_EL3) && !arm_is_secure(env)) {
             if (cs->icc_apr[GICV3_G1NS][0] & ICC_AP1R_EL1_NMI) {
@@ -2961,7 +2962,16 @@ void gicv3_init_cpuif(GICv3State *s)
          */
         define_arm_cp_regs(cpu, gicv3_cpuif_reginfo);

-        if (s->nmi_support) {
+        /*
+         * If the CPU implements FEAT_NMI and FEAT_GICv3 it must also
+         * implement FEAT_GICv3_NMI, which is the CPU interface part
+         * of NMI support. This is distinct from whether the GIC proper
+         * (redistributors and distributor) have NMI support. In QEMU
+         * that is a property of the GIC device in s->nmi_support;
+         * cs->nmi_support indicates the CPU interface's support.
+         */
+        if (cpu_isar_feature(aa64_nmi, cpu)) {
+            cs->nmi_support = true;
             define_arm_cp_regs(cpu, gicv3_cpuif_gicv3_nmi_reginfo);
         }

plus this squashed into patch 19:

diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 20a8e1f2fe4..b1f6c16ffef 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -566,7 +566,7 @@ static void icv_ap_write(CPUARMState *env, const
ARMCPRegInfo *ri,

     trace_gicv3_icv_ap_write(ri->crm & 1, regno,
gicv3_redist_affid(cs), value);

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
     } else {
         cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
@@ -1510,7 +1510,7 @@ static int icv_drop_prio(GICv3CPUState *cs, bool *nmi)
             continue;
         }

-        if (i == 0 && cs->gic->nmi_support && (*papr1 & ICV_AP1R_EL1_NMI)) {
+        if (i == 0 && cs->nmi_support && (*papr1 & ICV_AP1R_EL1_NMI)) {
             *papr1 &= (~ICV_AP1R_EL1_NMI);
             *nmi = true;
             return 0xff;
@@ -2699,7 +2699,7 @@ static void ich_ap_write(CPUARMState *env, const
ARMCPRegInfo *ri,

     trace_gicv3_ich_ap_write(ri->crm & 1, regno,
gicv3_redist_affid(cs), value);

-    if (cs->gic->nmi_support) {
+    if (cs->nmi_support) {
         cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
     } else {
         cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
@@ -2821,7 +2821,7 @@ static void ich_lr_write(CPUARMState *env, const
ARMCPRegInfo *ri,
     }

     /* Enforce RES0 bit in NMI field when FEAT_GICv3_NMI is not implemented */
-    if (!cs->gic->nmi_support) {
+    if (!cs->nmi_support) {
         value &= ~ICH_LR_EL2_NMI;
     }

The comments and commit message for patch 24 also need tweaking,
because they are written assuming that FEAT_GICv3_NMI means
"NMI support in the GIC proper", not "NMI support in the cpuif".

Since those changes are not too complicated, and I made them
locally anyway since I wanted to confirm that my plan was
workable, my proposal is that I will apply these fixes while
I take this series into target-arm.next for 9.1.

So I've applied this series to target-arm.next with the above
changes (preparatory to doing a pull request tail end of next
week once we release 9.0). Let me know if you'd prefer something
else.

thanks
-- PMM