Re: [RFC PATCH v5 18/22] hw/intc/arm_gicv3: Implement NMI interrupt prioirty

[Richard Henderson]

On 2/29/24 03:10, 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>
> ---
> v4:
> - Replace is_nmi with has_superprio to not a mix NMI and superpriority.
> - Update the comment in irqbetter().
> - Extract gicv3_get_priority() to avoid code repeat.
> ---
> v3:
> - Add missing brace
> ---
>   hw/intc/arm_gicv3.c | 71 ++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 63 insertions(+), 8 deletions(-)
>
> diff --git a/hw/intc/arm_gicv3.c b/hw/intc/arm_gicv3.c
> index 0b8f79a122..1d16a53b23 100644
> --- a/hw/intc/arm_gicv3.c
> +++ b/hw/intc/arm_gicv3.c
> @@ -21,7 +21,8 @@
>   #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 has_superprio)
>   {
>       /* Return true if this IRQ at this priority should take
>        * precedence over the current recorded highest priority
> @@ -33,11 +34,24 @@ static bool irqbetter(GICv3CPUState *cs, int irq, uint8_t 
> prio)
>       if (prio < cs->hppi.prio) {
>           return true;
>       }
> +
> +    /*
> +     * Current highest prioirity pending interrupt is an IRQ without
> +     * superpriority, the new IRQ with superpriority has same priority
> +     * should signal to the CPU as it have the priority higher than
> +     * the labelled 0x80 or 0x00.
> +     */
> +    if (prio == cs->hppi.prio && !cs->hppi.superprio && has_superprio) {
> +        return true;
> +    }
> +
>       /* If multiple pending interrupts have the same priority then it is an
>        * IMPDEF choice which of them to signal to the CPU. We choose to
> -     * signal the one with the lowest interrupt number.
> +     * signal the one with the lowest interrupt number if they don't have
> +     * superpriority.
>        */
> -    if (prio == cs->hppi.prio && irq <= cs->hppi.irq) {
> +    if (prio == cs->hppi.prio && !cs->hppi.superprio &&
> +        !has_superprio && irq <= cs->hppi.irq) {
>           return true;
>       }
>       return false;
> @@ -129,6 +143,35 @@ static uint32_t gicr_int_pending(GICv3CPUState *cs)
>       return pend;
>   }
>   
> +static bool gicv3_get_priority(GICv3CPUState *cs, bool is_redist,
> +                               uint32_t superprio, uint8_t *prio, int irq)
> +{
> +    bool has_superprio = false;
> +
> +    if (superprio) {
> +        has_superprio = true;
> +
> +        /* DS = 0 & Non-secure NMI */
> +        if (!(cs->gic->gicd_ctlr & GICD_CTLR_DS) &&
> +            ((is_redist && extract32(cs->gicr_igroupr0, irq, 1)) ||
> +             (!is_redist && gicv3_gicd_group_test(cs->gic, irq)))) {
> +            *prio = 0x80;
> +        } else {
> +            *prio = 0x0;
> +        }
> +    } else {
> +        has_superprio = false;
> +
> +        if (is_redist) {
> +            *prio = cs->gicr_ipriorityr[irq];
> +        } else {
> +            *prio = cs->gic->gicd_ipriority[irq];
> +        }
> +    }
> +
> +    return has_superprio;
> +}

Did you not like the idea to map {priority, !superpriority} into a single value?

It would eliminate the change in irqbetter(), which is a bit more complex than 
it needs to be.

> @@ -152,10 +197,13 @@ static void gicv3_redist_update_noirqset(GICv3CPUState 
> *cs)
>               if (!(pend & (1 << i))) {
>                   continue;
>               }
> -            prio = cs->gicr_ipriorityr[i];
> -            if (irqbetter(cs, i, prio)) {
> +            superprio = extract32(cs->gicr_isuperprio, i, 1);
> +            has_superprio = gicv3_get_priority(cs, true, superprio, &prio, i);

It would allow moving the read of gicr_isuperprio into gicv3_get_priority(), alongside the 
read of gicr_ipriorityr.

Is there a bug here not handling is_redist for GCIR_INMI*?

> @@ -168,7 +216,7 @@ static void gicv3_redist_update_noirqset(GICv3CPUState *cs)
>       if ((cs->gicr_ctlr & GICR_CTLR_ENABLE_LPIS) && cs->gic->lpi_enable &&
>           (cs->gic->gicd_ctlr & GICD_CTLR_EN_GRP1NS) &&
>           (cs->hpplpi.prio != 0xff)) {
> -        if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio)) {
> +        if (irqbetter(cs, cs->hpplpi.irq, cs->hpplpi.prio, false)) {

Always passing false here is incorrect -- again missing the redistributor nmi 
bit?


r~