On Sun, 7 Apr 2024 at 09:19, Jinjie Ruan <ruanjin...@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