Re: [PATCH v11 18/23] hw/intc/arm_gicv3: Handle icv_nmiar1_read() for icc_nmiar1_read()

[Peter Maydell]

On Wed, 3 Apr 2024 at 04:16, Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> On 2024/4/3 0:12, Peter Maydell wrote:
> >> @@ -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);
>
> The nmi_support check is redundant, as if FEAT_GICv3_NMI is unsupported,
> the ICH_LR_EL2.NMI is RES0, so if ICH_LR_EL2.NMI is 1, FEAT_GICv3_NMI
> has been surely realized.

As far as I can see you haven't changed ich_lr_write() to enforce
that, though, so the guest can write 1 to the NMI bit even if the
GIC doesn't support FEAT_GICv3_NMI. If you want to skip checking
nmi_support here you need to enforce that the NMI bit in the LR
is 0 in ich_lr_write().

thanks
-- PMM