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

[Jinjie Ruan]

On 2024/3/1 7:50, Richard Henderson wrote:
> 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.

I will try to change to implement this mapping scheme.

> 
>> @@ -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~