On 2026/09/03 01:50 PM, Kirill A. Korinsky wrote:
> OpenBSD exposes this by raising CTPR while handling interrupts. If an
> edge remains pending when EOI lowers the servicing priority, QEMU
> reasserts INT without checking CTPR; IACK then returns the spurious
> vector and consumes the masked edge before CTPR permits delivery.
>
> Signed-off-by: Kirill A. Korinsky <[email protected]>
> ---
> hw/intc/openpic.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 99d2a1e65e..6f9a562163 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -1033,8 +1033,9 @@ static void openpic_cpu_write_internal(void *opaque,
> hwaddr addr,
> n_IRQ = IRQ_get_next(opp, &dst->raised);
> if (n_IRQ != -1) {
> src = &opp->src[n_IRQ];
> - if (s_IRQ == -1 ||
> - IVPR_PRIORITY(src->ivpr) > dst->servicing.priority) {
> + if (IVPR_PRIORITY(src->ivpr) > dst->ctpr &&
> + (s_IRQ == -1 ||
> + IVPR_PRIORITY(src->ivpr) > dst->servicing.priority)) {
The original condition only gated the INT re-assertion on the servicing
priority, missing the CTPR dimension entirely. The new guard mirrors exactly
what IRQ_local_pipe() already does at the assertion path (line 292), so the
three callsites are now consistent.
> DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
> idx, n_IRQ);
> qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
> @@ -1070,12 +1071,13 @@ static uint32_t openpic_iack(OpenPICState *opp,
> IRQDest *dst, int cpu)
> }
>
> src = &opp->src[irq];
> - if (!(src->ivpr & IVPR_ACTIVITY_MASK) ||
> - !(IVPR_PRIORITY(src->ivpr) > dst->ctpr)) {
> + if (!(src->ivpr & IVPR_ACTIVITY_MASK)) {
> error_report("%s: bad raised IRQ %d ctpr %d ivpr 0x%08x",
> __func__, irq, dst->ctpr, src->ivpr);
> openpic_update_irq(opp, irq);
> retval = opp->spve;
> + } else if (IVPR_PRIORITY(src->ivpr) <= dst->ctpr) {
> + return opp->spve;
Splitting the old combined condition is the right approach. The !IVPR_ACTIVITY
and prio <= ctpr cases are semantically distinct:
- A missing ACTIVITY bit on a raised IRQ is a genuine inconsistency and deserves
the existing error_report + openpic_update_irq treatment.
- A CTPR-masked IRQ in raised.queue is expected by design — openpic_update_irq
intentionally keeps IRQs there even when masked - the below comment at
hw/intc/openpic.c:282 mentions about it. Treating it as a bug and calling
openpic_update_irq on it was incorrect.
/*
* Even if the interrupt doesn't have enough priority,
* it is still raised, in case ctpr is lowered later.
*/
The return opp->spve (early return) for the CTPR-masked case is also important —
it skips the edge-sensitive cleanup block that follows:
if (!src->level) {
src->ivpr &= ~IVPR_ACTIVITY_MASK;
src->pending = 0;
IRQ_resetbit(&dst->raised, irq);
}
Without the early return, a CTPR-masked edge IRQ would have its activity bit
cleared and be removed from raised.queue — silently losing it before CTPR
permits delivery.
Note: I'm not an OpenPIC expert, so take this as a code-walkthrough review, but
based on my understanding the logic looks correct to me.
Reviewed-by: Amit Machhiwal <[email protected]>
Thanks,
Amit
