On Sat, 30 Mar 2024 at 10:33, Jinjie Ruan <ruanjin...@huawei.com> wrote:
>
> Implement icv_nmiar1_read() for icc_nmiar1_read(), so add definition for
> ICH_LR_EL2.NMI and ICH_AP1R_EL2.NMI bit.
>
> If FEAT_GICv3_NMI is supported, ich_ap_write() should consider
> ICV_AP1R_EL1.NMI
> bit. In icv_activate_irq() and icv_eoir_write(), the ICV_AP1R_EL1.NMI bit
> should be set or clear according to the Non-maskable property. And the RPR
> priority should also update the NMI bit according to the APR priority NMI bit.
>
> By the way, add gicv3_icv_nmiar1_read trace event.
>
> If the hpp irq is a NMI, the icv iar read should return 1022 and trap for
> NMI again
>
> Signed-off-by: Jinjie Ruan <ruanjin...@huawei.com>
> Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> v11:
> - Deal with NMI in the callers instead of ich_highest_active_virt_prio().
> - Set either NMI or a group-priority bit, not both.
> - Only set AP NMI bits in the 0 reg.
> - Handle NMI in hppvi_index(), icv_hppi_can_preempt() and icv_eoir_write().
> v10:
> - Rename ICH_AP1R_EL2_NMI to ICV_AP1R_EL1_NMI.
> - Add ICV_RPR_EL1_NMI definition.
> - Set ICV_RPR_EL1.NMI according to the ICV_AP1R<n>_EL1.NMI in
> ich_highest_active_virt_prio().
> v9:
> - Correct the INTID_NMI logic.
> v8:
> - Fix an unexpected interrupt bug when sending VNMI by running qemu VM.
> v7:
> - Add Reviewed-by.
> v6:
> - Implement icv_nmiar1_read().
> ---
> hw/intc/arm_gicv3_cpuif.c | 97 ++++++++++++++++++++++++++++++++++-----
> hw/intc/gicv3_internal.h | 4 ++
> hw/intc/trace-events | 1 +
> 3 files changed, 91 insertions(+), 11 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
> index f99f2570a6..a7bc44b30c 100644
> --- a/hw/intc/arm_gicv3_cpuif.c
> +++ b/hw/intc/arm_gicv3_cpuif.c
> @@ -157,6 +157,10 @@ static int ich_highest_active_virt_prio(GICv3CPUState
> *cs)
> int i;
> int aprmax = ich_num_aprs(cs);
>
> + if (cs->gic->nmi_support && cs->ich_apr[GICV3_G1NS][0] &
> ICV_AP1R_EL1_NMI) {
> + return 0x80;
This should be 0, not 0x80 -- see the register description for
ICH_LR<n>_EL2 Priority field: when NMI is 1, the virtual interrupt's
priority is treated as 0x0.
> + }
> +
> for (i = 0; i < aprmax; i++) {
> uint32_t apr = cs->ich_apr[GICV3_G0][i] |
> cs->ich_apr[GICV3_G1NS][i];
> @@ -191,6 +195,7 @@ static int hppvi_index(GICv3CPUState *cs)
> * correct behaviour.
> */
> int prio = 0xff;
> + bool nmi = false;
>
> if (!(cs->ich_vmcr_el2 & (ICH_VMCR_EL2_VENG0 | ICH_VMCR_EL2_VENG1))) {
> /* Both groups disabled, definitely nothing to do */
> @@ -200,6 +205,11 @@ static int hppvi_index(GICv3CPUState *cs)
> for (i = 0; i < cs->num_list_regs; i++) {
> uint64_t lr = cs->ich_lr_el2[i];
> int thisprio;
> + bool thisnmi = false;
> +
> + if (cs->gic->nmi_support) {
> + thisnmi = lr & ICH_LR_EL2_NMI;
> + }
You could write this a little more concisely as
bool thisnmi = cs->gic_nmi_support && (lr & ICH_LR_EL2_NMI);
> if (ich_lr_state(lr) != ICH_LR_EL2_STATE_PENDING) {
> /* Not Pending */
> @@ -219,9 +229,13 @@ static int hppvi_index(GICv3CPUState *cs)
>
> thisprio = ich_lr_prio(lr);
>
> - if (thisprio < prio) {
> + if ((thisprio < prio) || ((thisprio == prio) && (thisnmi & (!nmi))))
> {
> prio = thisprio;
> idx = i;
> +
> + if (cs->gic->nmi_support) {
> + nmi = thisnmi;
> + }
You don't need to check nmi_support here because if nmi_support
is false then thisnmi will also be false, and so we will never
set nmi to anything other than false.
> }
> }
>
> @@ -326,6 +340,12 @@ static bool icv_hppi_can_preempt(GICv3CPUState *cs,
> uint64_t lr)
> return true;
> }
>
> + if ((prio & mask) == (rprio & mask) &&
> + cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI) &&
> + (!(cs->ich_apr[GICV3_G1NS][0] & ICV_AP1R_EL1_NMI))) {
> + return true;
> + }
> +
> return false;
> }
This isn't the only change needed to icv_hppi_can_preempt():
virtual NMIs are never masked by the MPR, so that check also
must be changed. If we pull out a variable:
bool is_nmi = cs->gic->nmi_support && (lr & ICH_LR_EL2_NMI);
we can use that both to gate the vpmr check:
if (!is_nmi && prio >= vpmr) {
and also in the priority check you have here.
> @@ -736,13 +765,19 @@ static void icv_activate_irq(GICv3CPUState *cs, int
> idx, int grp)
> */
> uint32_t mask = icv_gprio_mask(cs, grp);
> int prio = ich_lr_prio(cs->ich_lr_el2[idx]) & mask;
> + bool nmi = cs->ich_lr_el2[idx] & ICH_LR_EL2_NMI;
> int aprbit = prio >> (8 - cs->vprebits);
> int regno = aprbit / 32;
> int regbit = aprbit % 32;
>
> cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
> cs->ich_lr_el2[idx] |= ICH_LR_EL2_STATE_ACTIVE_BIT;
> - cs->ich_apr[grp][regno] |= (1 << regbit);
> +
> + if (cs->gic->nmi_support && nmi) {
> + cs->ich_apr[grp][regno] |= ICV_AP1R_EL1_NMI;
> + } else {
> + cs->ich_apr[grp][regno] |= (1 << regbit);
> + }
> }
>
> static void icv_activate_vlpi(GICv3CPUState *cs)
> @@ -776,7 +811,11 @@ static uint64_t icv_iar_read(CPUARMState *env, const
> ARMCPRegInfo *ri)
> if (thisgrp == grp && icv_hppi_can_preempt(cs, lr)) {
> intid = ich_lr_vintid(lr);
> if (!gicv3_intid_is_special(intid)) {
> - icv_activate_irq(cs, idx, grp);
> + if (!(lr & ICH_LR_EL2_NMI)) {
This is missing checks on both whether the GIC has NMI support and
on whether the SCTLR NMI bit is set (compare pseudocode
VirtualReadIAR1()). I suggest defining a
bool nmi = cs->gic->nmi_support &&
(env->cp15.sctlr_el[arm_current_el(env)] & SCTLR_NMI) &&
(lr & ICH_LR_EL2_NMI);
and then you can check "if (!nmi)" here.
> + icv_activate_irq(cs, idx, grp);
> + } else {
> + intid = INTID_NMI;
> + }
> } else {
> /* Interrupt goes from Pending to Invalid */
> cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
> @@ -797,8 +836,32 @@ static uint64_t icv_iar_read(CPUARMState *env, const
> ARMCPRegInfo *ri)
>
> static uint64_t icv_nmiar1_read(CPUARMState *env, const ARMCPRegInfo *ri)
> {
> - /* todo */
> + GICv3CPUState *cs = icc_cs_from_env(env);
> + int idx = hppvi_index(cs);
> uint64_t intid = INTID_SPURIOUS;
> +
> + if (idx >= 0 && idx != HPPVI_INDEX_VLPI) {
> + uint64_t lr = cs->ich_lr_el2[idx];
> + int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
> +
> + if ((thisgrp == GICV3_G1NS) && (lr & ICH_LR_EL2_NMI)) {
> + intid = ich_lr_vintid(lr);
> + if (!gicv3_intid_is_special(intid)) {
> + icv_activate_irq(cs, idx, GICV3_G1NS);
> + } else {
> + /* Interrupt goes from Pending to Invalid */
> + cs->ich_lr_el2[idx] &= ~ICH_LR_EL2_STATE_PENDING_BIT;
> + /* We will now return the (bogus) ID from the list register,
> + * as per the pseudocode.
> + */
QEMU's coding style wants the /* on a line of its own for a
multiline comment.
> + }
> + }
> + }
> +
> + trace_gicv3_icv_nmiar1_read(gicv3_redist_affid(cs), intid);
> +
> + gicv3_cpuif_virt_update(cs);
> +
> return intid;
> }
> @@ -1504,6 +1573,7 @@ static void icv_eoir_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> int irq = value & 0xffffff;
> int grp = ri->crm == 8 ? GICV3_G0 : GICV3_G1NS;
> int idx, dropprio;
> + bool nmi = false;
>
> trace_gicv3_icv_eoir_write(ri->crm == 8 ? 0 : 1,
> gicv3_redist_affid(cs), value);
> @@ -1516,8 +1586,8 @@ static void icv_eoir_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> * error checks" (because that lets us avoid scanning the AP
> * registers twice).
> */
> - dropprio = icv_drop_prio(cs);
> - if (dropprio == 0xff) {
> + dropprio = icv_drop_prio(cs, &nmi);
> + if (dropprio == 0xff && !nmi) {
> /* No active interrupt. It is CONSTRAINED UNPREDICTABLE
> * whether the list registers are checked in this
> * situation; we choose not to.
> @@ -1539,8 +1609,9 @@ static void icv_eoir_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> uint64_t lr = cs->ich_lr_el2[idx];
> int thisgrp = (lr & ICH_LR_EL2_GROUP) ? GICV3_G1NS : GICV3_G0;
> int lr_gprio = ich_lr_prio(lr) & icv_gprio_mask(cs, grp);
> + int thisnmi = lr & ICH_LR_EL2_NMI;
This variable should be "bool". An "int" is 32 bits, so because
ICH_LR_EL2_NMI is bit 59, the value of "lr & ICH_LR_EL2_NMI" when
cast to int is always zero. Using bool avoids this bug and also
is consistent with the type we used for the 'nmi' variable we're
about to compare it with.
> - if (thisgrp == grp && lr_gprio == dropprio) {
> + if (thisgrp == grp && (lr_gprio == dropprio || thisnmi == nmi)) {
> if (!icv_eoi_split(env, cs) || irq >= GICV3_LPI_INTID_START) {
> /*
> * Priority drop and deactivate not split: deactivate irq
> now.
> @@ -2626,7 +2697,11 @@ static void ich_ap_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
>
> trace_gicv3_ich_ap_write(ri->crm & 1, regno, gicv3_redist_affid(cs),
> value);
>
> - cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
> + if (cs->gic->nmi_support) {
> + cs->ich_apr[grp][regno] = value & (0xFFFFFFFFU | ICV_AP1R_EL1_NMI);
> + } else {
> + cs->ich_apr[grp][regno] = value & 0xFFFFFFFFU;
> + }
> gicv3_cpuif_virt_irq_fiq_update(cs);
> }
thanks
-- PMM
