On 260728 16:29, Farhan Ali wrote:
On 7/28/2026 1:59 PM, Konstantin Shkolnyy wrote:
On 260728 12:46, Farhan Ali wrote:
On 7/27/2026 4:23 PM, Konstantin Shkolnyy wrote:
+static int s390_pci_device_pre_load(void *opaque)
+{
+ S390PCIBusDevice *pbdev = S390_PCI_DEVICE(opaque);
+ S390PCIBusDevice *found_pbdev;
+
+ /*
+ * Make sure pbdev is removed from the table before state
load. The change
+ * of pbdev->idx means it needs to be moved to a different
position anyway,
+ * and is illegal while in the table. But be careful to not
remove
+ * instead another pbdev whose state might have been loaded
earlier and
I think "instead" is not needed in the statement above.
Maybe I am missing something, but could you help me understand why
do we need to remove the pbdev if we found at a particular idx? If
there is a collision with idx, ie on destination we have a
different device with the same idx, are we removing a valid device?
I see that this is not the best comment. How about this variant:
* State loading can change pbdev->idx. Therefore, make sure
pbdev is removed
* from the table before that happens. The table type used stores
a pointer
* to pbdev->idx and becomes corrupt if idx is changed from
outside. But be
* careful to not remove instead another pbdev whose state might
have been
* loaded earlier and that got assigned this idx value and had
therefore
* already replaced our pbdev in the table. post_load() will
reinsert our
* pbdev into the table.
So pbdev->idx is autogenerated when we create the device at
destination, since we are migrating the idx from source this could
change on destination and corrupt the hash table?
Exactly.
Then do we need to migrate the idx?
idx is part of "function handle". So, it should remain unchanged after
migration.
+ * that has then replaced our pbdev. (post_load() will put
our pbdev back.)
+ */
+ found_pbdev = g_hash_table_lookup(s390_get_phb()->zpci_table,
&pbdev->idx);
+ assert(found_pbdev);
AFAIU the pbdev->idx at pre_load() would be what we assign
automatically at the destination. In that case wouldn't this assert
always be true?
Yes. This assert documents that a pbdev is always expected to be found
for this "autogenerated when we create the device at destination"
pbdev->idx - either the pbdev we are currently processing, or another
pbdev that was "loaded" earlier with the same idx value (which came
from the source) and inserted by post_load() into zpci_table.
+ if (found_pbdev == pbdev) {
similarly this should also be true?
No always true, as described above.
Okay this makes a lot more sense to me now, thanks for the explanation!
+ g_hash_table_remove(s390_get_phb()->zpci_table, &pbdev->idx);
Don't we have to free this idx now? so reverse of s390_pci_alloc_idx()?
AFA I can see, simply removing pbdev->idx from zpci_table already
"frees" the idx value for reuse. s390_pci_alloc_idx() interprets s-
>next_idx as the "next mostly likely free" idx value. It then checks
if zpci_table has this idx; if it does, it checks idx+1, etc. until it
doesn't.
It seems, we could add "s->next_idx = pbdev->idx;" here to make it
pick this idx next time instead of larger values, but it's not
strictly necessary. Do you think we should do it?
We seem to be doing it in s390_pci_interp_plug() [1] so maybe we should
just to close this gap? In reality though this "leak" is not a concern
for resource exhaustion.
[1] https://elixir.bootlin.com/qemu/v11.1.0-rc1/source/hw/s390x/s390-
pci-bus.c#L1072
However, s390_pcihost_unplug() doesn't change s->next_idx after calling
g_hash_table_remove().
In practice, since we have 64K values to go through before the idx
allocator wraps around and starts searching from 0, it seems unlikely to
happen. (We'd have to plug PCI devices 64K times.)
The purpose of s->next_idx appears to be to help allocation to happen on
the first try without checking zpci_table many times.
If we just keep incrementing s->next_idx and never set it back, it'll
allocate on the first try 64K times.
If, however, we set it back after g_hash_table_remove(), the next
allocation will be quick, but subsequent ones might have to scan
zpci_table before finding a free value. In this implementation, the
scanning we try to avoid might happen in practice long before we reach
64K allocations. On top of that, we'll then reuse old "function handle"
values - not that I know of a problem with that, but...
So, it's extra code to set s->next_idx back and I don't see how it's useful.
Thanks
Farhan
+ }
+
+ return 0;
+}
+
+static int s390_pci_device_post_load(void *opaque, int version_id)
+{
+ S390PCIBusDevice *pbdev = S390_PCI_DEVICE(opaque);
+
/*
- * TODO: add state handling here, so migration works at least
with
- * emulated pci devices on s390x
+ * Now that pbdev->idx has been loaded, use it to place pbdev
back into
+ * the table. This may replace a different not-yet-state-
loaded pbdev,
+ * but pre_load() handles this case.
*/
- .unmigratable = 1,
+ g_hash_table_replace(s390_get_phb()->zpci_table, &pbdev->idx,
pbdev);
+
+ /*
+ * Regenerate IOMMU state, including IOTLB contents and QEMU
memory regions.
+ */
+ if (pbdev->iommu_enabled) {
+ assert(pbdev->iommu);
+ if (s390_pci_is_translation_enabled(pbdev->g_iota)) {
+ s390_pci_iommu_enable(pbdev);
+ s390_pci_ioat_replay(pbdev);
+ } else {
+ s390_pci_iommu_direct_map_enable(pbdev);
+ }
+ }
+
+ /*
+ * Guest sets fmb_addr by mpcifc.ZPCI_MOD_FC_SET_MEASURE
instruction,
+ * whose handler consequently starts fmb_timer. We may need
to restart it.
+ */
+ if (pbdev->fmb_addr) {
+ assert(!pbdev->fmb_timer);
+ assert(pbdev->pci_group);
+ pbdev->fmb_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
+ fmb_update, pbdev);
+ timer_mod(pbdev->fmb_timer,
+ qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
+ pbdev->pci_group->zpci_group.mui);
+ }
+ return 0;
+}
