Re: QOM address space handling

[Mark Cave-Ayland]

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

#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

#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

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?


ATB,

Mark.