Re: [PATCH] docs/devel: Prohibit calling object_unparent() for memory region

[Akihiko Odaki]

On 2024/10/08 22:33, Peter Maydell wrote:
> On Thu, 29 Aug 2024 at 06:46, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Hi; sorry it's taken me so long to get back to this patch,
> but I've now re-read some of the discussion in the other threads.
> I generally agree with your reasoning and think we do need
> to update the docs here.
>
> I think we could be more specific in this commit message; I've
> suggested some expansions of it below. (Partly this is for
> my own benefit working through what's going on.)
>
> > Previously it was allowed to call object_unparent() for a memory region
> > in instance_finalize() of its parent. However, such a call typically
> > has no effect because child objects get unparented before
> > instance_finalize().
>
> "because child objects are properties of their owner object, and
> (since commit 76a6e1cc7cc3a) we delete an object's properties before
> calling the object's instance_finalize method. Deleting the
> child property will unparent the child; the later calls to
> object_unparent() in its owner's instance_finalize will do nothing.".
>
> > Worse, memory regions typically gets finalized when they get unparented
> > before instance_finalize().
>
> "before instance_finalize(), because the only reference to the
> MemoryRegion is the one we hold because it is a child property
> of its owner, and so when object_finalize_child_property()
> calls object_unref(child) the refcount drops to zero and
> we finalize the MR."
>
> > This means calling object_unparent() for
> > them in instance_finalize() is to call the function for an object
> > already finalized, which should be avoided.
>
> "This doesn't cause any bad effects in the common case where we
> know that the memory allocated for the MemoryRegion itself
> is still valid, such as in the common case where the MR
> struct is directly embedded in its owner's device state struct.
> But it could potentially cause a crash if the MemoryRegion
> struct was somewhere else and was freed.

It won't cause a crash even if the MR struct is not embedded as long as 
the data structure is freed in the instance_finalize callback. I suggest 
the following:

"This doesn't cause any bad effects in the common case where the
data structure is managed by the owner because the memory allocated for
the MemoryRegion itself is valid until the owner is also finalized.
However, it is still semantically wrong to touch the MemoryRegion after
its finalization and confusing."

Your other suggestions look good so I'll apply them with v2.

> Update the documentation to explicitly prohibit calling
> object_unparent() in instance_finalize."
>
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> > ---
> >   docs/devel/memory.rst | 14 +++++++-------
> >   1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> > index 69c5e3f914ac..83760279e3db 100644
> > --- a/docs/devel/memory.rst
> > +++ b/docs/devel/memory.rst
> > @@ -168,11 +168,10 @@ and VFIOQuirk in hw/vfio/pci.c.
>
> Don't we need also to change the paragraph before this, which
> reads:
>
> > If however the memory region is part of a dynamically allocated data
> > structure, you should call object_unparent() to destroy the memory region
> > before the data structure is freed.  For an example see VFIOMSIXInfo
> > and VFIOQuirk in hw/vfio/pci.c.
>
> ?
>
> Otherwise we have a paragraph that says "you should call
> object_unparent()" followed by one that says "do not call
> object_unparent()".

You are right. I'll remove the statement.

> (Also that paragraph's reference to VFIOMSIXInfo is now
> out of date -- in 2017 we removed the embedded MemoryRegion
> from that struct.)
>
> >   You must not destroy a memory region as long as it may be in use by a
> >   device or CPU.  In order to do this, as a general rule do not create or
> > -destroy memory regions dynamically during a device's lifetime, and only
> > -call object_unparent() in the memory region owner's instance_finalize
> > -callback.  The dynamically allocated data structure that contains the
> > -memory region then should obviously be freed in the instance_finalize
> > -callback as well.
> > +destroy memory regions dynamically during a device's lifetime, and do not
> > +call object_unparent().  The dynamically allocated data structure that contains
> > +the memory region then should be freed in the instance_finalize callback, which
> > +is called after it gets unparented.
>
> I think some of these lines are a touch over-length, and should
> be wrapped a bit earlier.

I'll wrap them by 72 columns.

> Also, this now says "don't unparent in finalize", but
> vfio_bars_finalize() has code which follows the old documentation:
>
>      if (vdev->vga) {
>          vfio_vga_quirk_finalize(vdev);
>          for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
>              object_unparent(OBJECT(&vdev->vga->region[i].mem));
>          }
>          g_free(vdev->vga);
>      }
>
> Is this wrong and needs changing?

I'll add a patch to remove object_unparent() calls from hw/vfio/pci.c 
with v2.

Regards,
Akihiko Odaki

> >   If you break this rule, the following situation can happen:
> >
> > @@ -199,8 +198,9 @@ but nevertheless it is used in a few places.
> >
> >   For regions that "have no owner" (NULL is passed at creation time), the
> >   machine object is actually used as the owner.  Since instance_finalize is
> > -never called for the machine object, you must never call object_unparent
> > -on regions that have no owner, unless they are aliases or containers.
> > +never called for the machine object, you must never free regions that have no
> > +owner, unless they are aliases or containers, which you can manually call
> > +object_unparent() for.
>
> thanks
> -- PMM