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

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

On 14/10/2026 13:04, Peter Maydell wrote:

> On Wed, 14 Oct 2026 at 12:09, Mark Cave-Ayland
> <mark.caveayland@xxxxxxxxxxx> wrote:
> > On 13/10/2026 17:17, Peter Maydell wrote:
> >
> > > On Wed, 16 Sept 2026 at 11:50, Mark Cave-Ayland
> > > <mark.caveayland@xxxxxxxxxxx> wrote:
> > > > +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.
> I think that every QOM class ought to define its own instance
> struct, even if it doesn't contain anything beyond the base
> class. This means that when we do need to add a field in future
> we don't have to go around and find the places which currently
> use the parent-class struct but really want the leaf-class
> struct. (There are places where we don't catch mistakes like
> this -- for instance I think if you don't update the
> .instance_size in a type then that doesn't get noticed:
> we've had a bug or too like that).
>
> We don't write that down in 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__wiki.qemu.org_Documentation_QOMConventions&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=c23RpsaH4D2MKyD3EPJTDa0BAxz6tV8aUJqVSoytEiY&m=_sMHrUpIgbF65xjh0xd4r-zuudhThxapWCIXp0orT8OH8CbTHKgmaw4SuUcNTpeW&s=9mPS_zIx9x3FD_6cxEpVK-5JyOlynQ3bsBTf44iWLPk&e=
> but we do have some similar rules about class structs that
> boil down to "either you're a leaf class and provide none
> of these things, or else you should provide them all; don't
> have a halfway-house 'we happen to only need X and Y but
> not Z right now' setup".
Yes, that's fine with me. I have done it that way previously, however 
there are some people on-list still advocating for the approach in this 
series.
If we decide that this is our preferred style then I think it's worth 
documenting in somewhere in 
https://qemu.readthedocs.io/en/master/devel/style.html#qemu-object-model-declarations 
so that we can point people (and LLMs) towards it.
> > > >    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.
> OK.
ATB,

Mark.