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
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;
+}
