Re: QEMU device refcounting when device creates a container MR

[Peter Maydell]

On Thu, 10 Mar 2022 at 13:19, Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Mar 09, 2022 at 11:40:15AM +0100, Philippe Mathieu-Daudé wrote:
> > Cc'ing David / Peter
> >
> > On 9/3/22 11:33, Peter Maydell wrote:
> > > Hi; does anybody know how device reference counting is supposed
> > > to work when the device creates a "container" MemoryRegion which
> > > it then puts some of its own subregions in to?
> > >
> > > As far as I can see when you do memory_region_add_subregion it
> > > increases the refcount on the owner of the subregion. So if a
> > > device creates a container MR in its own init or realize method
> > > and adds sub-MRs that it owns to that container, this increases
> > > the refcount on the device permanently, and so the device won't
> > > ever be deinited.
> > >
> > > As a specific example, the usb-chipidea device does this in its
> > > init method, so if you run the arm device-introspect-test under
> > > leak-sanitizer it complains about a memory leak that happens
> > > when the device is put through the "init-introspect-deref" cycle.
>
> I'm not extremely sure about this, but.. does it mean that the device may
> better put any of the add-subregion operations into realize() rather than
> instance_init()?  Then in the unrealize() of the devices we should do
> proper del-subregion to release these refcounts.

That would be one option. In the specific case of chipidea it would
alter the ordering in which the container gets set up, which is
currently:
 * chipidea's MRs added to container (in chipidea instance_init)
 * usb-ehci's MRs added to container (in usb_ehci_realize)
Because chipidea is a subclass, moving its "add MRs to container"
to its own realize would put them after the addition of the
usb-ehci MRs. I haven't checked yet whether this would change
the semantics. (If it does, we can probably fix it by using
explicit priorities, which we should anyway if the various
sub-region MRs overlap.)

We don't document anywhere that this must be done in realize
and not instance_init, though.

> Otherwise indeed I don't see a good way to destroy the device anymore,
> because the assumption is after device initialized, only with that will the
> object_unref() continue to work on the device..

The other theory I had was "maybe if you put an MR into another
MR and they both have the same owner then don't bump the refcount"
but I haven't thought that through at all.

> That means, perhaps in object_init_with_type() we should make sure the
> object refcount==1 after the ->instance_init() call?

That's probably a useful invariant, which I bet we don't
currently get right for every object :-)

-- PMM