Re: [PATCH 13/22] i8259.c: introduce TYPE_I8259_PIC device

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

On 13/10/2026 17:17, Peter Maydell wrote:

> On Wed, 16 Sept 2026 at 11:50, Mark Cave-Ayland
> <mark.caveayland@xxxxxxxxxxx> wrote:
> > This device represents a PIC for an x86 machine consisting of 2 cascaded i8259
> > devices wired accordingly.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.caveayland@xxxxxxxxxxx>
> > ---
> >   hw/intc/i8259.c | 133 ++++++++++++++++++++++++++++++++++++++++++------
> >   1 file changed, 118 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/intc/i8259.c b/hw/intc/i8259.c
> > index af33b71591..fcc94e12f8 100644
> > --- a/hw/intc/i8259.c
> > +++ b/hw/intc/i8259.c
> > @@ -25,7 +25,9 @@
> >   #include "qemu/osdep.h"
> >   #include "hw/intc/i8259.h"
> >   #include "hw/core/irq.h"
> > +#include "hw/core/qdev-properties.h"
> >   #include "hw/isa/isa.h"
> > +#include "qapi/error.h"
> >   #include "qemu/timer.h"
> >   #include "qemu/log.h"
> >   #include "hw/isa/i8259_internal.h"
> > @@ -36,6 +38,22 @@
> >
> >   #define TYPE_I8259 "isa-i8259"
> >
> > +#define TYPE_I8259_PIC "isa-i8259-pic"
> > +
> > +struct I8259PICState {
> > +    DeviceState parent_obj;
> > +
> > +    qemu_irq pic_out_irq;
> > +    qemu_irq pass_irqs[ISA_NUM_IRQS];
> > +
> > +    ISABus *isabus;
> > +    IRQState i8259_primary_out_irq;
> > +    I8259CommonState i8259[2];
> This allocates only enough space for the common base class's
> instance struct, but later we do an object_initialize_child()
> of the child class. If the child class's state struct is
> ever bigger than the common class, this will break.
That's true. In theory this should be something like I8259State, however 
I didn't think it was worth adding a specific 
OBJECT_DECLARE_SIMPLE_TYPE() if there were no extra fields. I did 
quickly check that if this happens, then the assert() in 
object_initialize() fires so if this did happen then we'd find out 
really quickly.
> >   static const TypeInfo i8259_type_infos[] = {
> >       {
> >           .name       = TYPE_I8259,
> >           .parent     = TYPE_I8259_COMMON,
> >           .class_init = i8259_class_init,
> >       },
> > +    {
> > +        .name          = TYPE_I8259_PIC,
> > +        .parent        = TYPE_DEVICE,
> Devices that directly inherit from TYPE_DEVICE are awkward,
> because they never get reset. Conviently this device doesn't
> currently need a reset method, but still, is this a good idea?
In this case I think it's correct, because the PIC device in this case 
is a container for 2 cascaded i8259s which are connected separately to 
the ISA bus. Therefore each i8259 will have its reset triggered via a 
reset of the ISA bus.
The container itself can't exist on the ISA bus because it would then 
appear as a separate device which doesn't make sense in this case.
> > +        .class_init    = i8259_pic_class_init,
> > +        .instance_init = i8259_pic_init,
> > +        .instance_size = sizeof(I8259PICState),
> > +    },
> >   };
> >
> >   DEFINE_TYPES(i8259_type_infos)
> -- PMM

ATB,

Mark.