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

[Peter Maydell <peter.maydell@xxxxxxxxxx>]

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://wiki.qemu.org/Documentation/QOMConventions
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".

> >>   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.

-- PMM