Re: [RFC PATCH v6 19/23] hw/intc/arm_gicv3: Implement NMI interrupt prioirty

[Jinjie Ruan]

On 2024/3/6 6:36, Richard Henderson wrote:
> On 3/4/24 21:03, Jinjie Ruan via wrote:
>> -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,22 @@ static bool irqbetter(GICv3CPUState *cs, int irq,
>> uint8_t prio)
>>       if (prio < cs->hppi.prio) {
>>           return true;
>>       }
>> +
>> +    /*
>> +     * The same priority IRQ with superpriority 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;
> 
> This should be reordered for clarity:
> 
>     if (prio != cs->hppi.prio) {
>         return prio < cs->hppi.prio;
>     }
> 
>     if (has_superprio != cs->hppi.superprio) {
>         return has_superprio;
>     }
> 
>     return irq <= cs->hppa.irq;
> 
> So that we do not have to keep incorporating previous tests.

This looks much cleaner.

> 
> 
> 
>> @@ -129,6 +141,43 @@ static uint32_t gicr_int_pending(GICv3CPUState *cs)
>>       return pend;
>>   }
>>   +static bool gicv3_get_priority(GICv3CPUState *cs, bool is_redist,
>> +                               uint8_t *prio, int irq)
>> +{
>> +    bool has_superprio = false;
>> +    uint32_t superprio = 0x0;
>> +
>> +    if (is_redist) {
>> +        superprio = extract32(cs->gicr_isuperprio, irq, 1);
>> +    } else {
>> +        superprio = *gic_bmp_ptr32(cs->gic->superprio, irq);
>> +        superprio = superprio & (1 << (irq & 0x1f));
>> +    }
>> +
>> +    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;
>> +}
> 
> has_superprio == superprio -- you can eliminate has_superprio,
> or even leverage the code path:
> 
>     if (superprio) {
>         ...
>         return true;
>     }
> 
>     if (is_redist)
>     ...
>     return false;
> 
>> @@ -168,9 +219,10 @@ 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)) {
> 
> New argument should be hpplpi.superprio.
> 
> There are several places where we're setting irq and prio which might
> need to clear superprio, e.g. update_for_one_lpi.  But also anywhere
> else that deals with PendingIrq.
> 
>> +        cs->hppi.superprio = 0x0;
> 
> false, not 0x0.
> 
> 
> r~