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