On 5/7/26 3:31 AM, Thomas Huth wrote: > On 06/05/2026 21.45, Matthew Rosato wrote: >> On 5/6/26 1:41 PM, Thomas Huth wrote: >>> From: Thomas Huth <[email protected]> >>> >>> The elements that get removed with QTAILQ_REMOVE are never referenced >>> afterwards anymore, so the corresponding memory should get freed. >>> >>> Signed-off-by: Thomas Huth <[email protected]> >>> --- >>> hw/s390x/s390-pci-bus.c | 1 + >>> hw/s390x/s390-pci-vfio.c | 1 + >>> 2 files changed, 2 insertions(+) >>> >> >> Thanks Thomas! Code looks good and I did some regression testing with >> this patch applied: >> >> Reviewed-by: Matthew Rosato <[email protected]> >> Tested-by: Matthew Rosato <[email protected]> >> >> Should we also consider adding: >> >> Fixes: b354d5d804 ("s390x/pci: clean up s390 PCI groups") >> Fixes: 37fa32de70 ("s390x/pci: Honor DMA limits set by vfio") > > Yes, that makes sense. > > Actually, looking at the commit description of b354d5d804, I think this > code has been added with a wrong assumption: The unrealize method is not > called during system reset.
So, yes, you're right that this isn't doing what was intended per the commit message... > So to free the memory, this should likely be added to a reset handler > instead? Could you maybe have a look? But I also don't think that we really want to arbitrarily clear this list on a machine reset. As written, it's only OK to free this list arbitrarily if we are 1) ending the process anyway or 2) can expect another call to s390_pcihost_realize() + device plug (s390_pcihost_plug) events for all devices in order to re-establish new group pointers. When we do, say, a reboot or kexec in the guest, because the associated devices did not go through an unplug they continue to use the reference to their pci_group which should be OK -- the underlying vfio device will still give the same group data post-reset. (Or, if it's an emulated device, it will today always use the same QEMU-defined group) I think the only issue is that when we do actually unplug vfio-pci devices, we can potentially orphan groups in the list that are no longer used. Fortunately there is an upper-limit of 256 entries; while the S390PCIGroup supports a full int, s390-pci-vfio.c will base all group IDs from a uint8_t. But it does leave a theoretical issue of what if the device is also unplugged from the host, and a new device later comes along that re-uses the host group ID with new group info, and then you pass that device through to this guest. So: I wonder if the proper thing to do is to refcount the groups during plug and remove them from the list during unplug when refcount hits 0 (and of course, add the missing g_free here) -- with special handling for the default group that should never go away. Which is probably more than you signed on for with this patch ;) If you'd like, I can instead sort this out and give you a reported-by? Meanwhile you could still split out the fix to s390_pci_end_dma_count() and send that one again by itself (w/ Fixes: 37fa32de70) Thanks, Matt
