Re: QOM address space handling

[Eduardo Habkost]

On Fri, Dec 18, 2020 at 07:49:24AM +0000, Mark Cave-Ayland wrote:
> On 10/11/2020 11:40, Paolo Bonzini wrote:
> 
> > On 10/11/20 12:14, Mark Cave-Ayland wrote:
> > > There are 2 possible solutions here: 1) ensure QOM objects that add
> > > address spaces during instance init have a corresponding instance
> > > finalize function to remove them or 2) move the creation of address
> > > spaces from instance init to realize.
> > > 
> > > Does anyone have any arguments for which solution is preferred?
> > 
> > I slightly prefer (1) because there could be cases where you also create
> > subdevices using that address space, and in order to set properties of
> > subdevices before realize, you would have to create the subdevices in
> > instance_init as well.
> 
> As discussed on IRC, I've been testing this approach but curiously found
> that if a device init function contains address_space_init() then the
> corresponding finalize function is never called during
> device-introspect-test.
> 
> Following on from Markus' comment about there being a refcounting issue, I
> spent a few hours yesterday poking this and indeed that seems to be the
> problem here: the generation of the flatview leaks references to the address
> space MemoryRegion.
> 
> Adding a bit of debugging to object.c's init and finalize paths in this
> particular case shows that the call to address_space_init() in
> sun4u_iommu.c's iommu_init() generates 3 references between the
> IOMMUMemoryRegion (iommu-sun4u) and its device owner (sun4u-iommu) during
> flatview construction:
> 
> #0  memory_region_ref (mr=0x5555565f43b0) at ../softmmu/memory.c:1792
> #1  0x0000555555a7050d in flatview_new (mr_root=0x5555565f43b0) at
> ../softmmu/memory.c:264
> #2  0x0000555555a71d7d in generate_memory_topology (mr=0x5555565f43b0) at
> ../softmmu/memory.c:729
> #3  0x0000555555a73290 in address_space_update_topology (as=0x5555565f4358)
> at ../softmmu/memory.c:1078
> #4  0x0000555555a77f01 in address_space_init (as=0x5555565f4358,
> root=0x5555565f43b0, name=0x555555e05eb1 "iommu-as") at
> ../softmmu/memory.c:2848
> 

This one is owned by the FlatView, and should be undone by
flatview_destroy().

> #0  memory_region_ref (mr=0x55555664ffb0) at ../softmmu/memory.c:1792
> #1  0x0000555555a7063d in flatview_insert (view=0x555556609350, pos=0,
> range=0x7fffffffe0d0) at ../softmmu/memory.c:283
> #2  0x0000555555a71aad in render_memory_region (view=0x555556609350,
> mr=0x55555664ffb0, base=0, clip=..., readonly=false, nonvolatile=false) at
> ../softmmu/memory.c:662
> #3  0x0000555555a71e02 in generate_memory_topology (mr=0x55555664ffb0) at
> ../softmmu/memory.c:732
> #4  0x0000555555a73284 in address_space_update_topology (as=0x55555664ff58)
> at ../softmmu/memory.c:1078
> #5  0x0000555555a77ef5 in address_space_init (as=0x55555664ff58,
> root=0x55555664ffb0, name=0x555555e05eb1 "iommu-as") at
> ../softmmu/memory.c:284
> 

This one should also be undone by flatview_destroy().

> #0  memory_region_ref (mr=0x55555664ffb0) at ../softmmu/memory.c:1792
> #1  0x0000555555b2479b in phys_section_add (map=0x5555565f6bd0,
> section=0x7fffffffe100) at ../softmmu/physmem.c:1095
> #2  0x0000555555b24b21 in register_multipage (fv=0x555556609350,
> section=0x7fffffffe100) at ../softmmu/physmem.c:1158
> #3  0x0000555555b24eea in flatview_add_to_dispatch (fv=0x555556609350,
> section=0x7fffffffe1c0) at ../softmmu/physmem.c:1198
> #4  0x0000555555a71e86 in generate_memory_topology (mr=0x55555664ffb0) at
> ../softmmu/memory.c:742
> #5  0x0000555555a73284 in address_space_update_topology (as=0x55555664ff58)
> at ../softmmu/memory.c:1078
> #6  0x0000555555a77ef5 in address_space_init (as=0x55555664ff58,
> root=0x55555664ffb0, name=0x555555e05eb1 "iommu-as") at
> ../softmmu/memory.c:2848
> 

This one is owned by the AddressSpaceDispatch, which is owned by
the FlatView.  It should be undone by flatview_destroy() ->
address_space_dispatch_free() -> phys_sections_free() ->
phys_section_destroy().

Now, who owns the FlatView reference, exactly?

If the FlatView reference is owned by the MemoryRegion, we have a
reference loop: the device holds a reference to the MemoryRegion,
which owns the FlatView, which holds a reference to the device.
In this case, who owns the reference loop and is responsible for
breaking it?

If the FlatView reference is not owned by the MemoryRegion, who
owns it?

> Seemingly it is these references that prevent sun4u-iommu's finalize
> function from being called by the final object_unref() once
> device-introspect-test for the sun4u-iommu device is finished.
> 
> Any thoughts as to the best way to resolve this? Certainly one solution is
> to simply move address_space_init()/address_space_destroy() from
> init/finalize to realize/unrealize if Paolo's comment about needing to set
> up address spaces is no longer valid, but then in this case is it possible
> to add an assert() to prevent developers calling address_space_init() from
> an init function accidentally?

-- 
Eduardo