Re: [RFC PATCH v3 18/21] hw/intc/arm_gicv3: Implement NMI interrupt prioirty

[Richard Henderson]

On 2/23/24 00:32, Jinjie Ruan via wrote:
> If GICD_CTLR_DS bit is zero and the NMI is non-secure, the NMI prioirty
> is higher than 0x80, otherwise it is higher than 0x0. And save NMI
> super prioirty information in hppi.superprio to deliver NMI exception.
> Since both GICR and GICD can deliver NMI, it is both necessary to check
> whether the pending irq is NMI in gicv3_redist_update_noirqset and
> gicv3_update_noirqset. And In irqbetter(), only a non-NMI with the same
> priority and a smaller interrupt number can be preempted but not NMI.
>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> v3:
> - Add missing brace
> ---
>   hw/intc/arm_gicv3.c | 63 ++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 56 insertions(+), 7 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
> index 0b8f79a122..75999edd19 100644
> --- a/hw/intc/arm_gicv3.c
> +++ b/hw/intc/arm_gicv3.c
> @@ -21,7 +21,7 @@
>   #include "hw/intc/arm_gicv3.h"
>   #include "gicv3_internal.h"
>   
> -static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t prio)
> +static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t prio, bool is_nmi)
>   {
>       /* Return true if this IRQ at this priority should take
>        * precedence over the current recorded highest priority
> @@ -33,11 +33,21 @@ static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t 
> prio)
>       if (prio < cs->hppi.prio) {
>           return true;
>       }
> +
> +    /*
> +     * Current highest prioirity pending interrupt is not a NMI
> +     * and the new IRQ is a NMI with same priority.
> +     */
> +    if (prio == cs->hppi.prio && !cs->hppi.superprio && is_nmi) {

It would be best to not mix terminology -- superpriority or nmi but not a mix.  It's 
unfortunate that the manual does so...

It is very tempting expand prio to more bits so that all of the rest of this 
Just Works.
Because of...

> +            if (superprio) {
> +                is_nmi = 1;
> +
> +                /* DS = 0 & Non-secure NMI */
> +                if ((!(cs->gic->gicd_ctlr & GICD_CTLR_DS)) &&
> +                    extract32(cs->gicr_igroupr0, i, 1)) {
> +                    prio = 0x80;
> +                } else {
> +                    prio = 0x0;
> +                }
> +            } else {
> +               is_nmi = 0;
> +               prio = cs->gicr_ipriorityr[i];

... the need to check GICD_CTLR_DS for interpreting superpriority within the continuum, 
per section 4.8.1 (NMI prioritization), it would seem that we could map

    Secure NMI         -> 0
    Non-secure NMI     -> 0x100
    prio 0x00 .. 0xff  -> prio * 2 + 1

which matches the ordering in Figure 4-6.

> @@ -240,10 +271,28 @@ static void gicv3_update_noirqset(GICv3State *s, int 
> start, int len)
>                */
>               continue;
>           }
> -        prio = s->gicd_ipriority[i];
> -        if (irqbetter(cs, i, prio)) {
> +
> +        superprio = *gic_bmp_ptr32(s->superprio, i);
> +        /* NMI */
> +        if (superprio & (1 << (i & 0x1f))) {
> +            is_nmi = 1;
> +
> +            /* DS = 0 & Non-secure NMI */
> +            if ((!(s->gicd_ctlr & GICD_CTLR_DS)) &&
> +                gicv3_gicd_group_test(s, i)) {
> +                    prio = 0x80;
> +            } else {
> +                    prio = 0x0;
> +            }
> +        } else {
> +            is_nmi = 0;
> +            prio = s->gicd_ipriority[i];
> +        }

In any case, let's not have two copies of this resolution.


r~