QOM address space handling

QOM address space handling

[Mark Cave-Ayland]

Hi all,

This email follows on from my investigation of intermittent Travis-CI failures in 
make check's device-introspect test when trying to add the patch at 
https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg06093.html to my last 
qemu-sparc pull request.

The patch itself seems fairly harmless: moving the sun4u-iommu device as a QOM child 
of the sabre PCI host bridge device. So why was "make check" randomly segfaulting on 
Travis-CI?

The hardest part was trying to reproduce the issue to debug it: eventually after a 
number of Travis-CI runs I discovered I could generate the same problem locally if I 
ran "make check" around 15-20 times in a row, and that gave me a backtrace that 
looked like this:

0x0000000000614b69 in address_space_init (as=0x16f684d8,
root=0x16f68530, name=0x9a1db2 "iommu-as") at ../softmmu/memory.c:2780
2780        QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link);
(gdb) bt
#0  0x0000000000614b69 in address_space_init (as=0x16f684d8,
  root=0x16f68530, name=0x9a1db2 "iommu-as") at
../softmmu/memory.c:2780
#1  0x00000000005b8f6a in iommu_init (obj=0x16f681c0) at ../hw/sparc64/sun4u_iommu.c:301
#2  0x000000000070a997 in object_init_with_type (obj=0x16f681c0,
  ti=0x1629fac0) at ../qom/object.c:375

With the debugger attached I was able to figure out what was happening: the 
sun4u-iommu device creates the iommu-as address space during instance init, but 
doesn't have a corresponding instance finalize to remove it which leaves a dangling 
pointer in the address_spaces QTAILQ.

Normally this doesn't matter because IOMMUs are created once during machine init, but 
device-introspect-test instantiates sun4u-iommu (and with the patch sabre also adds 
it as a child object during instance init) which adds more dangling pointers to the 
address_spaces list. Every so often the dangling pointers end up pointing to memory 
that gets reused by another QOM object, eventually causing random segfaults during 
instance finalize and/or property iteration.

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?

As part of this work I hacked up an address_space_count() function in memory.c that 
returns the size of the address_spaces QTAILQ and added a printf() to display the 
value during instance init and finalize which demonstrates the problem nicely. This 
means it should be possible to add a similar to check to device-introspect-test in 
future to prevent similar errors from happening again.


ATB,

Mark.

Re: QOM address space handling

[Paolo Bonzini]

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.

Thanks,

Paolo

> As part of this work I hacked up an address_space_count() function in 
> memory.c that returns the size of the address_spaces QTAILQ and added a 
> printf() to display the value during instance init and finalize which 
> demonstrates the problem nicely. This means it should be possible to add 
> a similar to check to device-introspect-test in future to prevent 
> similar errors from happening again.

Re: QOM address space handling

[Eduardo Habkost]

CCing Paolo, the Memory API maintainer.

On Tue, Nov 10, 2020 at 11:14:39AM +0000, Mark Cave-Ayland wrote:
> Hi all,
> 
> This email follows on from my investigation of intermittent Travis-CI
> failures in make check's device-introspect test when trying to add the patch
> at https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg06093.html to my
> last qemu-sparc pull request.
> 
> The patch itself seems fairly harmless: moving the sun4u-iommu device as a
> QOM child of the sabre PCI host bridge device. So why was "make check"
> randomly segfaulting on Travis-CI?
> 
> The hardest part was trying to reproduce the issue to debug it: eventually
> after a number of Travis-CI runs I discovered I could generate the same
> problem locally if I ran "make check" around 15-20 times in a row, and that
> gave me a backtrace that looked like this:
> 
> 0x0000000000614b69 in address_space_init (as=0x16f684d8,
> root=0x16f68530, name=0x9a1db2 "iommu-as") at ../softmmu/memory.c:2780
> 2780        QTAILQ_INSERT_TAIL(&address_spaces, as, address_spaces_link);
> (gdb) bt
> #0  0x0000000000614b69 in address_space_init (as=0x16f684d8,
>  root=0x16f68530, name=0x9a1db2 "iommu-as") at
> ../softmmu/memory.c:2780
> #1  0x00000000005b8f6a in iommu_init (obj=0x16f681c0) at ../hw/sparc64/sun4u_iommu.c:301
> #2  0x000000000070a997 in object_init_with_type (obj=0x16f681c0,
>  ti=0x1629fac0) at ../qom/object.c:375
> 
> With the debugger attached I was able to figure out what was happening: the
> sun4u-iommu device creates the iommu-as address space during instance init,
> but doesn't have a corresponding instance finalize to remove it which leaves
> a dangling pointer in the address_spaces QTAILQ.
> 
> Normally this doesn't matter because IOMMUs are created once during machine
> init, but device-introspect-test instantiates sun4u-iommu (and with the
> patch sabre also adds it as a child object during instance init) which adds
> more dangling pointers to the address_spaces list. Every so often the
> dangling pointers end up pointing to memory that gets reused by another QOM
> object, eventually causing random segfaults during instance finalize and/or
> property iteration.
> 
> 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'd say (2) is preferred, as we don't expect object_new(T) to
have any side effects outside the object instance state.  Most
address_space_init() calls in the code today seem to be in
realize functions.

However, I wonder if we could make this simpler (and mistakes
less fatal) if we make AddressSpace a QOM child of the device.
Paolo, would it be too much overhead to make AddressSpace a QOM
object?

 
> As part of this work I hacked up an address_space_count() function in
> memory.c that returns the size of the address_spaces QTAILQ and added a
> printf() to display the value during instance init and finalize which
> demonstrates the problem nicely. This means it should be possible to add a
> similar to check to device-introspect-test in future to prevent similar
> errors from happening again.
> 
> 
> ATB,
> 
> Mark.
> 

-- 
Eduardo

Re: QOM address space handling

[Paolo Bonzini]

On 10/11/20 16:03, Eduardo Habkost wrote:
>> Does anyone have any arguments for which solution is preferred?
> I'd say (2) is preferred, as we don't expect object_new(T) to
> have any side effects outside the object instance state.

Since there are no listeners, the side effects of address_space_init() 
are relatively limited.  So doing it in instance_init is not a big deal.

> Most
> address_space_init() calls in the code today seem to be in
> realize functions.
> 
> However, I wonder if we could make this simpler (and mistakes
> less fatal) if we make AddressSpace a QOM child of the device.
> Paolo, would it be too much overhead to make AddressSpace a QOM
> object?

No, it wouldn't.  AddressSpace is already quite heavyweight.

Paolo

Re: QOM address space handling

[Eduardo Habkost]

On Tue, Nov 10, 2020 at 04:08:16PM +0100, Paolo Bonzini wrote:
> On 10/11/20 16:03, Eduardo Habkost wrote:
> > > Does anyone have any arguments for which solution is preferred?
> > I'd say (2) is preferred, as we don't expect object_new(T) to
> > have any side effects outside the object instance state.
> 
> Since there are no listeners, the side effects of address_space_init() are
> relatively limited.  So doing it in instance_init is not a big deal.
> 
> > Most
> > address_space_init() calls in the code today seem to be in
> > realize functions.
> > 
> > However, I wonder if we could make this simpler (and mistakes
> > less fatal) if we make AddressSpace a QOM child of the device.
> > Paolo, would it be too much overhead to make AddressSpace a QOM
> > object?
> 
> No, it wouldn't.  AddressSpace is already quite heavyweight.

I thought this was going to be an easy job, but call_rcu()
requires rcu_head to be the first struct field.  I assume it is
acceptable to use call_rcu1() + container_of() manually in this
case.

-- 
Eduardo

Re: QOM address space handling

[Eduardo Habkost]

On Tue, Nov 10, 2020 at 12:46:48PM -0500, Eduardo Habkost wrote:
> On Tue, Nov 10, 2020 at 04:08:16PM +0100, Paolo Bonzini wrote:
> > On 10/11/20 16:03, Eduardo Habkost wrote:
> > > > Does anyone have any arguments for which solution is preferred?
> > > I'd say (2) is preferred, as we don't expect object_new(T) to
> > > have any side effects outside the object instance state.
> > 
> > Since there are no listeners, the side effects of address_space_init() are
> > relatively limited.  So doing it in instance_init is not a big deal.
> > 
> > > Most
> > > address_space_init() calls in the code today seem to be in
> > > realize functions.
> > > 
> > > However, I wonder if we could make this simpler (and mistakes
> > > less fatal) if we make AddressSpace a QOM child of the device.
> > > Paolo, would it be too much overhead to make AddressSpace a QOM
> > > object?
> > 
> > No, it wouldn't.  AddressSpace is already quite heavyweight.
> 
> I thought this was going to be an easy job, but call_rcu()
> requires rcu_head to be the first struct field.  I assume it is
> acceptable to use call_rcu1() + container_of() manually in this
> case.

Wait.  What exactly prevents callers of address_space_destroy()
from freeing the area containing the AddressSpace struct before
do_address_space_destroy() gets a chance to be called?

-- 
Eduardo

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.

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

Re: QOM address space handling

[Paolo Bonzini]

On 18/12/20 23:32, Eduardo Habkost wrote:
> Who owns the FlatView reference, exactly?

The AddressSpace.  The device creates the AddressSpace, which holds a 
reference to the MemoryRegion through FlatView and AddressSpaceDispatch, 
which holds a reference to the device.

By destroying the address space that it created, the device can break 
the reference loop.

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

The reference loop is owned by the device, which breaks it through 
unrealize (called by unparent).

instance_finalize by definition cannot break reference loops, so this 
means that my suggestion of using address_space_init in instance_init 
was wrong.

Thanks,

Paolo

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


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

Re: QOM address space handling

[Eduardo Habkost]

On Sun, Dec 20, 2020 at 10:25:25AM +0100, Paolo Bonzini wrote:
> On 18/12/20 23:32, Eduardo Habkost wrote:
> > Who owns the FlatView reference, exactly?
> 
> The AddressSpace.  The device creates the AddressSpace, which holds a
> reference to the MemoryRegion through FlatView and AddressSpaceDispatch,
> which holds a reference to the device.
> 
> By destroying the address space that it created, the device can break the
> reference loop.
> 
> > 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?
> 
> The reference loop is owned by the device, which breaks it through unrealize
> (called by unparent).
> 
> instance_finalize by definition cannot break reference loops, so this means
> that my suggestion of using address_space_init in instance_init was wrong.

Once we fix that, I suggest we add an assertion to make it
illegal to call object_ref() on an object during instance_init.

Do we know how many address_space_init() calls in instance_init
we have today?

-- 
Eduardo

Re: QOM address space handling

[Paolo Bonzini]

On 21/12/20 19:54, Eduardo Habkost wrote:
> On Sun, Dec 20, 2020 at 10:25:25AM +0100, Paolo Bonzini wrote:
>> On 18/12/20 23:32, Eduardo Habkost wrote:
>>> Who owns the FlatView reference, exactly?
>>
>> The AddressSpace.  The device creates the AddressSpace, which holds a
>> reference to the MemoryRegion through FlatView and AddressSpaceDispatch,
>> which holds a reference to the device.
>>
>> By destroying the address space that it created, the device can break the
>> reference loop.
>>
>>> 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?
>>
>> The reference loop is owned by the device, which breaks it through unrealize
>> (called by unparent).
>>
>> instance_finalize by definition cannot break reference loops, so this means
>> that my suggestion of using address_space_init in instance_init was wrong.
> 
> Once we fix that, I suggest we add an assertion to make it
> illegal to call object_ref() on an object during instance_init.

It's not necessarily illegal.  You can for example call a function that 
internally does

     object_ref(obj);
     oc->func(obj);
     object_unref(obj);

But perhaps we could assert that refcount == 1 on exit.

> Do we know how many address_space_init() calls in instance_init
> we have today?

I can see them in raven_pcihost_initfn, in sun4?_iommu.c's iommu_init 
and xtensa_cpu_initfn, I think that's all.

Paolo

Re: QOM address space handling

[Eduardo Habkost]

On Mon, Dec 21, 2020 at 08:16:00PM +0100, Paolo Bonzini wrote:
> On 21/12/20 19:54, Eduardo Habkost wrote:
> > On Sun, Dec 20, 2020 at 10:25:25AM +0100, Paolo Bonzini wrote:
> > > On 18/12/20 23:32, Eduardo Habkost wrote:
> > > > Who owns the FlatView reference, exactly?
> > > 
> > > The AddressSpace.  The device creates the AddressSpace, which holds a
> > > reference to the MemoryRegion through FlatView and AddressSpaceDispatch,
> > > which holds a reference to the device.
> > > 
> > > By destroying the address space that it created, the device can break the
> > > reference loop.
> > > 
> > > > 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?
> > > 
> > > The reference loop is owned by the device, which breaks it through unrealize
> > > (called by unparent).
> > > 
> > > instance_finalize by definition cannot break reference loops, so this means
> > > that my suggestion of using address_space_init in instance_init was wrong.
> > 
> > Once we fix that, I suggest we add an assertion to make it
> > illegal to call object_ref() on an object during instance_init.
> 
> It's not necessarily illegal.  You can for example call a function that
> internally does
> 
>     object_ref(obj);
>     oc->func(obj);
>     object_unref(obj);

Oh, right.

> 
> But perhaps we could assert that refcount == 1 on exit.

That would be more difficult to debug, but would work.

> 
> > Do we know how many address_space_init() calls in instance_init
> > we have today?
> 
> I can see them in raven_pcihost_initfn, in sun4?_iommu.c's iommu_init and
> xtensa_cpu_initfn, I think that's all.

There's usb_ehci_init() too.

-- 
Eduardo