On 2025/09/12 6:37, Peter Xu wrote:
On Thu, Sep 11, 2025 at 12:47:24PM +0900, Akihiko Odaki wrote:
On 2025/09/11 5:41, Peter Xu wrote:
On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
Children are automatically unparented so manually unparenting is
unnecessary.
Worse, automatic unparenting happens before the insntance_finalize()
callback of the parent gets called, so object_unparent() calls in
the callback will refer to objects that are already unparented, which
is semantically incorrect.
Signed-off-by: Akihiko Odaki <od...@rsg.ci.i.u-tokyo.ac.jp>
---
hw/vfio/pci.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index
07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e
100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
vfio_region_finalize(&bar->region);
if (bar->mr) {
assert(bar->size);
- object_unparent(OBJECT(bar->mr));
g_free(bar->mr);
bar->mr = NULL;
}
@@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
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);
}
}
So the 2nd object_unparent() here should be no-op, seeing empty list of
properties (but shouldn't causing anything severe), is that correct?
No. The object is finalized with the first object_unparent() if there is no
referrer other than the parent. The second object_unparent() will access the
finalized, invalid object in that case.
Yes, it's logically wrong. I was trying to understand the impact when it's
invoked. In this specific case of above two changes, I believe both MR
structs are still available, so it does look fine, e.g. nothing would crash.
For example, I think it doesn't need to copy stable if there's no real
functional issue involved.
You are right. Cc: stable is unnecessary.
I think it still makes some sense to theoretically allow object_unparent()
to happen, at least when it happens before owner's finalize(). IIUC that
was the intention of the doc, pairing the memory_region_init*() operation.
Perhaps so, but this patch is only about the case where object_unparent() is
called in finalize().
You ignored my other comment. That (using object_new() on MRs) was what I
was thinking might be better than a split model you discussed here, so
that's also a comment for patch 1 of your series.
I'm not sure what you mean by the "split model".
This change removes object_unparent() in vfio_bars_finalize().
object_new() will allow removing even g_free(), but we can do these
changes incrementally:
1. remove object_unparent() in finalize(),
which fixes a semantic problem (this patch)
2. allow object_new() for MRs and remove g_free() in finalize()
as a refactoring.
So I suggest focusing on object_unparent() in finalize() to keep this
patch and review concise.
Btw, this patch also didn't change all occurances of such for VFIO?
E.g. there's at least vfio_vga_quirk_finalize(). I didn't check the rest.
Indeed. I only removed object_unparent() calls hw/vfio/pci.c because it
was mentioned in the documentation. I suspect you will find more cases
that subregions are used in instance_finalize() in general if you check
the code base; "[PATCH 11/22] vfio-user: Do not delete the subregion"
also removes memory_region_del_subregion() during finalization, for example.
Regards,
Akihiko Odaki
