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

[Jinjie Ruan]

On 2024/3/22 2:28, Peter Maydell wrote:
> On Thu, 21 Mar 2024 at 15:46, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Something somewhere needs to implement "if SCTLR_ELx.NMI is 0 then
>> we don't take EXCP_VINMI etc but instead (maybe) EXCP_VIRQ etc".
>> At the moment nothing does that:
>>  * arm_cpu_update_vinmi() doesn't look at the NMI bit before
>>    deciding whether to set CPU_INTERRUPT_VINMI
>>  * in arm_excp_unmasked() if NMI is 0 then allIntMask takes its
>>    default value of false and so arm_excp_unmasked() returns true,
>>    so VINMI is not masked
>>  * arm_cpu_exec_interrupt() doesn't look at the NMI bit before
>>    deciding whether to check the CPU_INTERRUPT_VINMI bit in interrupt_request
>>
>> So even if SCTLR_ELx.NMI is 0 we'll still try to take a VINMI
>> if it's set up in the HCR_EL2 bits.
>>
>> However we do this the required behaviour is that if NMI is 0
>> then it is as if the interrupt doesn't have superpriority and
>> it falls back to being handled as an ordinary IRQ, VIRQ, VFIQ etc.
>> I think the best place to do this is probably here in
>> arm_cpu_exec_interrupt() -- if SCTLR_ELx.NMI isn't set then
>> treat the VFNMI bit like VFIQ, the VINMI bit like VIRQ, and
>> the NMI bit like IRQ.
> 
> Folding in something like this I think will work:
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 91c2896de0f..797ae3eb805 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -837,7 +837,8 @@ static bool arm_cpu_exec_interrupt(CPUState *cs,
> int interrupt_request)
> 
>      /* The prioritization of interrupts is IMPLEMENTATION DEFINED. */
> 
> -    if (cpu_isar_feature(aa64_nmi, env_archcpu(env))) {
> +    if (cpu_isar_feature(aa64_nmi, env_archcpu(env)) &&
> +        (arm_sctlr(env, cur_el) & SCTLR_NMI)) {
>          if (interrupt_request & CPU_INTERRUPT_NMI) {
>              excp_idx = EXCP_NMI;
>              target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, 
> secure);
> @@ -862,7 +863,22 @@ static bool arm_cpu_exec_interrupt(CPUState *cs,
> int interrupt_request)
>                  goto found;
>              }
>          }
> +    } else {
> +        /*
> +         * NMI disabled: interrupts with superpriority are handled
> +         * as if they didn't have it
> +         */
> +        if (interrupt_request & CPU_INTERRUPT_NMI) {
> +            interrupt_request |= CPU_INTERRUPT_HARD;

The CPU_INTERRUPT_NMI and CPU_INTERRUPT_HARD are set simultaneously,
should the CPU_INTERRUPT_NMI be cleared?

> +        }
> +        if (interrupt_request & CPU_INTERRUPT_VINMI) {
> +            interrupt_request |= CPU_INTERRUPT_VIRQ;
> +        }
> +        if (interrupt_request & CPU_INTERRUPT_VFNMI) {
> +            interrupt_request |= CPU_INTERRUPT_VFIQ;
> +        }
>      }> +
>      if (interrupt_request & CPU_INTERRUPT_FIQ) {
>          excp_idx = EXCP_FIQ;
>          target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure);
> 
> 
>> What semantics do we intend for the VINMI/VFNMI bits in interrupt_request
>> and for the incoming IRQ, FIQ, NMI lines? The GIC spec suggests
>> (but doesn't mandate) that NMI could be signalled by asserting
>> both NMI and IRQ, and plain IRQ by asserting just IRQ (table 4-6
>> in the GIC spec). I think the GIC changes in this patchset assert
>> only the NMI line for an IRQNMI, and not both NMI and IRQ. That's OK
>> and I think makes more sense for QEMU than signalling both lines,
>> but it's not the same as what we wind up doing with the handling
>> of the HCR_EL2 bits in these functions, because you don't change
>> the existing arm_cpu_update_virq() so that it only sets the
>> CPU_INTERRUPT_VIRQ bit if this is a VIRQ and not a VIRQNMI.
>> So if the guest sets HCR_EL2.VI and HCRX_EL2.VINMI then
>> arm_cpu_update_virq() will say "this is a VIRQ" and also
>> arm_cpu_update_vinmi() will say "This is a VINMI" and so both bits
>> get set in the interrupt_request field.
>>
>> I think the fix for this is probably to have arm_cpu_update_virq()
>> and arm_cpu_update_vfiq() check that this is not a VINMI/VFNMI,
>> so we only set 1 bit in interrupt_request, not 2.
> 
> And for this a change like:
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 03a48a41366..91c2896de0f 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -926,7 +926,8 @@ void arm_cpu_update_virq(ARMCPU *cpu)
>      CPUARMState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
> 
> -    bool new_state = (env->cp15.hcr_el2 & HCR_VI) ||
> +    bool new_state = ((arm_hcr_el2_eff(env) & HCR_VI) &&
> +                      !(arm_hcrx_el2_eff(env) & HCRX_VINMI)) ||
>          (env->irq_line_state & CPU_INTERRUPT_VIRQ);
> 
>      if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VIRQ) != 0)) {
> @@ -947,7 +948,8 @@ void arm_cpu_update_vfiq(ARMCPU *cpu)
>      CPUARMState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
> 
> -    bool new_state = (env->cp15.hcr_el2 & HCR_VF) ||
> +    bool new_state = ((arm_hcr_el2_eff(env) & HCR_VF) &&
> +                      !(arm_hcrx_el2_eff(env) & HCRX_VFNMI)) ||
>          (env->irq_line_state & CPU_INTERRUPT_VFIQ);
> 
>      if (new_state != ((cs->interrupt_request & CPU_INTERRUPT_VFIQ) != 0)) {
> 
> 
> thanks
> -- PMM