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? Then do we need to migrate the idx?
+ * 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?
+ if (found_pbdev == pbdev) {
similarly this should also be true?
+ 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()?
+ }
+
+ 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;
+}
