On Wed, 3 Apr 2024 at 04:16, Jinjie Ruan <ruanjin...@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
