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

[Peter Maydell]

On Mon, 25 Mar 2024 at 08:52, Jinjie Ruan <ruanjinjie@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 <ruanjinjie@huawei.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 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 | 79 +++++++++++++++++++++++++++++++++------
>  hw/intc/gicv3_internal.h  |  4 ++
>  hw/intc/trace-events      |  1 +
>  3 files changed, 73 insertions(+), 11 deletions(-)

I haven't done a full review of this yet, but it looks like some of
the parts that applied to physical interrupts apply here too, eg
 * don't do the RPR NMI bit handling in ich_highest_active_virt_prio(),
   deal with NMI in the callers
 * in the AP registers, set either NMI or a group-priority bit, not both
 * AP NMI bits are only in the 0 reg, so checking doesn't need to be
   inside the for loop

You'll also need to update hppvi_index() so it accounts for NMIs
when it's finding the highest priority interrupt in the list registers:
compare the HighestPriorityVirtualInterrupt() pseudocode function.

thanks
-- PMM