Re: [PATCH 05/22] i8259.c: move parent_realize from PICClass to I8259CommonClass

[Peter Maydell <peter.maydell@xxxxxxxxxx>]

On Wed, 14 Oct 2026 at 14:58, Mark Cave-Ayland
<mark.caveayland@xxxxxxxxxxx> wrote:
>
> On 14/10/2026 12:00, Mark Cave-Ayland wrote:
>
> > On 13/10/2026 16:54, Peter Maydell wrote:
> >
> >> On Wed, 16 Sept 2026 at 11:49, Mark Cave-Ayland
> >> <mark.caveayland@xxxxxxxxxxx> wrote:
> >>>
> >>> Both the emulated and KVM i8259 implementations store a reference to the
> >>> parent_realize function in their respective classes. Start by moving
> >>> parent_realize from PICClass to I8259CommonClass and removing
> >>> PICClass since
> >>> it is no longer required.
> >>>
> >>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@xxxxxxxxxxx>
> >>
> >> This is one of those things where I think we're not 100% consistent
> >> about whether we think the parent_realize field should go in the
> >> child class struct or the parent class struct. The name "parent_realize"
> >> implies it ought to be in the child struct, though, and I think we
> >> do that more often than we put it in the parent class. This also
> >> makes some kind of separation-of-concerns sense too: it's the child
> >> that knows (a) that it exists at all and that (b) it wants to have a
> >> realize method that will mean it needs to chain back to the parent.
> >>
> >> The docs for device_class_set_parent_realize() claim the parent
> >> realize ptr is stored "usually in the instance structure" which
> >> I think is just completely wrong.
> >>
> >> I could be persuaded otherwise. As usual with QOM/qdev, I'm
> >> mostly interested in (a) consistency and (b) documenting what the
> >> "right thing" is. Realize, reset and unrealize are the three
> >> methods where QOM's lack of convenient support for "child
> >> implementation of method will need to chain to parent impl"
> >> is most awkward.
> > Yeah it's a tricky one. This patch was really just refactoring what was
> > already there, but I'm open to moving it into the instance if you think
> > that is better. I agree with your point about the child instance being
> > the one that knows it needs to chain back to the parent.
> >
> > I'll have an experiment and if it looks good, I'll move it to the
> > instance in v2.
> I did a search over the codebase and checked all references to
> DeviceRealize, and they are all stored in QOM classes and make use of
> device_class_set_parent_realize(). So for now I think we can say it is
> the documentation for device_class_set_parent_realize() that is wrong,
> and I will leave it this way in v2 of the series.

Yes, the *instance* part is definitely wrong. But there's still
"parent class struct" vs "child class struct". This patch puts
it in the parent class struct, and as I say above I kind of
prefer child class struct and think we do that the most often.

-- PMM