Re: [RFC PATCH v8 06/23] target/arm: Add support for Non-maskable Interrupt

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 19 Mar 2024 at 18:51, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/19/24 07:28, Peter Maydell wrote:
> >>       switch (excp_idx) {
> >> +    case EXCP_NMI:
> >> +        pstate_unmasked = !allIntMask;
> >> +        break;
> >> +
> >> +    case EXCP_VNMI:
> >> +        if ((!(hcr_el2 & HCR_IMO) && !(hcr_el2 & HCR_FMO)) ||
> >> +             (hcr_el2 & HCR_TGE)) {
> >> +            /* VNMIs(VIRQs or VFIQs) are only taken when hypervized.  */
> >> +            return false;
> >> +        }
> >
> > VINMI and VFNMI aren't the same thing: do we definitely want to
> > merge them into one EXCP_VNMI ?
>
> We do not, which is why VFNMI is going through EXCP_VFIQ.  A previous version did, and I
> see the comment did not change to match the new implementation.

Why do we put VFNMI through VFIQ, though? They're not
the same thing either... I think I would find this less
confusing if we implemented what the spec says and distinguished
all of (IRQ, FIQ, IRQ-with-superpriority and FIQ-with-superpriority).

> > The use of the _eff() versions of the functions here is
> > correct but it introduces a new case where we need to
> > reevaluate the status of the VNMI etc interrupt status:
> > when we change from Secure to NonSecure or when we change
> > SCR_EL3.EEL2 or SCR_EL3.HXEN. We either need to make sure
> > we reevaluate when we drop from EL3 to EL2 (which would be
> > OK since VINMI and VFNMI can't be taken at EL3 and none of
> > these bits can change except at EL3) or else make the calls
> > to reevaluate them when we write to SCR_EL3. At least, I don't
> > think we currently reevaluate these bits on an EL change.
>
> We re-evaluate these bits on EL change via gicv3_cpuif_el_change_hook.

Only if the GIC is connected to the VIRQ and VFIQ interrupt lines,
which it doesn't in theory have to be (though in practice it
usually will be). Plus it feels a bit fragile against
somebody deciding to put in a "this line hasn't changed state
so don't bother calling the handler again" optimization in the
future.

thanks
-- PMM