Re: [PATCH v2 19/27] i8259.c: properly cascade secondary i8259 from primary i8259

[Mark Cave-Ayland <mark.caveayland@xxxxxxxxxxx>]

On 16/10/2026 17:04, Peter Maydell wrote:

> On Fri, 16 Oct 2026 at 15:24, Mark Cave-Ayland
> <mark.caveayland@xxxxxxxxxxx> wrote:
> > Currently the cascaded output from the secondary i8259 is returned directly in
> > the pass_irqs[] array, instead of being connected to the relevant input of the
> > primary i8259.
> >
> > This is a bug fix since in a cascaded configuration on real hardware it is
> > possible (although unlikely) that the cascaded input from the secondary i8259
> > can be masked on the primary i8259.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.caveayland@xxxxxxxxxxx>
> > ---
> >   hw/intc/i8259.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> > index bfe01067b7..5f90edbe60 100644
> > --- a/hw/intc/i8259.c
> > +++ b/hw/intc/i8259.c
> > @@ -519,7 +519,8 @@ static void i8259_pic_realize(DeviceState *dev, Error 
> > **errp)
> >       }
> >
> >       /* Wire up secondary cascade */
> > -    qdev_connect_gpio_out(sec_dev, 0, s->pass_irqs[2]);
> > +    qdev_connect_gpio_out(sec_dev, 0,
> > +                          qdev_get_gpio_in(pri_dev, 2));
> Looking at this again, I'm not sure how this is a bugfix.
> Quoting patch 18, this is the code at this point, just
> for context:
>
> +    for (i = 0; i < 8; i++) {
> +        s->pass_irqs[i] = qdev_get_gpio_in(pri_dev, i);
> +    }
> +
> +    qdev_connect_gpio_out(pri_dev, 0, &s->i8259_primary_out_irq);
> +
> +    /* Secondary */
> +    sec_dev = DEVICE(&s->i8259[1]);
> +    qdev_prop_set_uint32(sec_dev, "iobase", 0xa0);
> +    qdev_prop_set_uint32(sec_dev, "elcr_addr", 0x4d1);
> +    qdev_prop_set_uint8(sec_dev, "elcr_mask", 0xde);
> +    if (!isa_realize_and_unref(ISA_DEVICE(sec_dev), s->isabus, errp)) {
> +        return;
> +    }
> +
> +    /* Wire up secondary cascade */
> +    qdev_connect_gpio_out(sec_dev, 0, s->pass_irqs[2]);
> +
> +    for (i = 8; i < ISA_NUM_IRQS; i++) {
> +        s->pass_irqs[i] = qdev_get_gpio_in(sec_dev, i - 8);
> +    }
>
> So before this patch, we connect sec_dev's gpio output 0 to
> s->pass_irqs[2]. The loop earlier in the code set that up:
>          s->pass_irqs[i] = qdev_get_gpio_in(pri_dev, i);
>
> Now we have:
>
> > +    qdev_connect_gpio_out(sec_dev, 0,
> > +                          qdev_get_gpio_in(pri_dev, 2));
> Isn't that exactly the same thing?
>
> I think it is indeed a bit clearer to directly grab the
> pri_dev input rather than pulling it back out of
> pass_irqs[2], but I don't think there's any behaviour
> change here.
(goes and looks)

Indeed, you're right. It seems that during the upstream rebase from my 
dev branch I accidentally dropped the following hunk:
diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
index 5fde3731ca..d713795ae0 100644
--- a/hw/intc/i8259.c
+++ b/hw/intc/i8259.c
@@ -504,6 +504,11 @@ static void i8259_pic_realize(DeviceState *dev, 
Error **errp)
     }

     for (i = 0; i < 8; i++) {
+        /* Cascaded input is not usable */
+        if (i == 2) {
+            continue;
+        }
+
         s->pass_irqs[i] = qdev_get_gpio_in(pri_dev, i);
     }

Along with removing the isa_pic global, my other plan for the future is 
to drop the passing of IRQ arrays around the x86 machine so this logic 
should eventually disappear.

ATB,

Mark.