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~
