On 260727 13:53, Farhan Ali wrote:
On 7/23/2026 8:09 PM, Konstantin Shkolnyy wrote:Implement zPCI device state migration, consequently enabling migration of VMs that have emulated PCI devices, whether virtio or not. Migration is allowed for devices whose function handle has the FH_SHM_EMUL bit set. For these devices QEMU will save and restore the state of its zPCI emulator.This will enable emulated PCI migration starting with s390-ccw- virtio-11.1.Passthrough devices will continue to block migration. Signed-off-by: Konstantin Shkolnyy <[email protected]> --- hw/s390x/s390-pci-bus.c | 187 +++++++++++++++++++++++++++++++- hw/s390x/s390-pci-inst.c | 2 +- hw/s390x/s390-virtio-ccw.c | 4 + include/hw/s390x/s390-pci-bus.h | 4 + 4 files changed, 191 insertions(+), 6 deletions(-) diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index a94e24a2ac..655667482d 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c @@ -26,6 +26,7 @@ #include "hw/pci/pci_bridge.h" #include "hw/pci/msi.h" #include "exec/cpu-common.h" +#include "migration/blocker.h" #include "qemu/error-report.h" #include "qemu/module.h" #include "system/physmem.h" @@ -34,6 +35,11 @@ #include "trace.h" +static const Property phb_props[] = { + DEFINE_PROP_BOOL("x-zpci-emul-dev-migr-enabled", S390pciState, + emul_dev_migr_enabled, true), +}; + S390pciState *s390_get_phb(void) { static S390pciState *phb; @@ -917,6 +923,23 @@ static void set_pbdev_info(S390PCIBusDevice *pbdev) pbdev->pci_group = s390_group_find(ZPCI_DEFAULT_FN_GRP); }+static int s390_set_emul_dev_migration_blocker(S390pciState *s, Error **errp)+{ + if (s->emul_dev_migr_enabled) { + return 0; + } + error_setg(&s->emul_dev_migr_blocker,+ "Migration disabled for emulated zPCI devices on this machine type");+ return migrate_add_blocker(&s->emul_dev_migr_blocker, errp); +} + +static void s390_clear_emul_dev_migration_blocker(S390pciState *s) +{ + if (s->emul_dev_migr_blocker) { + migrate_del_blocker(&s->emul_dev_migr_blocker); + } +} + static void s390_pcihost_realize(DeviceState *dev, Error **errp) { PCIBus *b;@@ -952,6 +975,7 @@ static void s390_pcihost_realize(DeviceState *dev, Error **errp)css_register_io_adapters(CSS_IO_ADAPTER_PCI, true, false, S390_ADAPTER_SUPPRESSIBLE, errp); s390_pcihost_kvm_realize(); + s390_set_emul_dev_migration_blocker(s, errp); } static void s390_pcihost_unrealize(DeviceState *dev) @@ -959,6 +983,8 @@ static void s390_pcihost_unrealize(DeviceState *dev) S390PCIGroup *group; S390pciState *s = S390_PCI_HOST_BRIDGE(dev); + s390_clear_emul_dev_migration_blocker(s); + while (!QTAILQ_EMPTY(&s->zpci_groups)) { group = QTAILQ_FIRST(&s->zpci_groups); QTAILQ_REMOVE(&s->zpci_groups, group, link);@@ -1129,6 +1155,27 @@ static int s390_pci_interp_plug(S390pciState *s, S390PCIBusDevice *pbdev)return 0; }+static int s390_set_passthrough_migration_blocker(S390PCIBusDevice *pbdev,+ Error **errp) +{ + pbdev->passthrough_migr_blocker = NULL; + + if (pbdev->fh & FH_SHM_EMUL) { + return 0; + } + error_setg(&pbdev->passthrough_migr_blocker, + "Migration blocked by passthrough zPCI device "+ "fh 0x%x uid %d fid %d", pbdev->fh, pbdev->uid, pbdev- >fid);+ return migrate_add_blocker(&pbdev->passthrough_migr_blocker, errp); +} ++static void s390_clear_passthrough_migration_blocker(S390PCIBusDevice *pbdev)+{ + if (pbdev->passthrough_migr_blocker) { + migrate_del_blocker(&pbdev->passthrough_migr_blocker); + } +} +static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,Error **errp) {@@ -1248,6 +1295,11 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,return; } + if (s390_set_passthrough_migration_blocker(pbdev, errp)) { + s390_pci_msix_free(pbdev); + return; + }Just curious, does this need to be after s390_pci_msix_init()? That way on error we avoid setting up the msix and subsequently freeing here.
It doesn't. I can swap them.
+ if (dev->hotplugged) { s390_pci_generate_plug_event(HP_EVENT_TO_CONFIGURED , pbdev->fh, pbdev->fid);@@ -1284,6 +1336,8 @@ static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,return; } + s390_clear_passthrough_migration_blocker(pbdev); + s390_pci_generate_plug_event(HP_EVENT_STANDBY_TO_RESERVED, pbdev->fh, pbdev->fid); bus = pci_get_bus(pci_dev);@@ -1451,6 +1505,7 @@ static void s390_pcihost_class_init(ObjectClass *klass, const void *data)hc->unplug_request = s390_pcihost_unplug_request; hc->unplug = s390_pcihost_unplug; msi_nonbroken = true; + device_class_set_props(dc, phb_props); } static const TypeInfo s390_pcihost_info = { @@ -1464,10 +1519,33 @@ static const TypeInfo s390_pcihost_info = { } }; +/* Return a unique bus "path" for zpci device */ +static char *s390_pci_bus_get_dev_path(DeviceState *dev) +{ + S390PCIBusDevice *pbdev = S390_PCI_DEVICE(dev); + return g_strdup_printf("uid-%04x", pbdev->uid); +} + +static void s390_pcibus_class_init(ObjectClass *oc, const void *data) +{ + BusClass *bc = BUS_CLASS(oc); + bc->get_dev_path = s390_pci_bus_get_dev_path; +} + static const TypeInfo s390_pcibus_info = { .name = TYPE_S390_PCI_BUS, .parent = TYPE_BUS, .instance_size = sizeof(S390PCIBus), + /*+ * Implement get_dev_path() to provide each zpci device with a unique + * stable UID-based bus "path". The "path" is used as part of idstr in the+ ^ migration stream, making idstr unique and instance_id always 0.Typo here with "^ migration stream"
OK.
+ * For migration to succeed, (idstr+instance_id) must match those generated + * during QEMU start. Without unique idstr, QEMU will generate variable + * instance_id to distinquish devices, and that instance_id can changeTypo "distinquish" -> distinguished
It's not a typo. This "to distiquish" is trying to say "for the purpose of distinguishing".
+ * if a device is unplugged and plugged back, preventing migration. + */ + .class_init = s390_pcibus_class_init, }; static uint16_t s390_pci_generate_uid(S390pciState *s)@@ -1613,13 +1691,112 @@ static const Property s390_pci_device_properties[] = {true), }; -static const VMStateDescription s390_pci_device_vmstate = { - .name = TYPE_S390_PCI_DEVICE, +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 andI 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.
+ * 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); + if (found_pbdev == pbdev) { + g_hash_table_remove(s390_get_phb()->zpci_table, &pbdev->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; +} + +static const VMStateDescription s390_pci_device_vmstate = { + .name = TYPE_S390_PCI_DEVICE, + .version_id = 1, + .minimum_version_id = 1, + .pre_load = s390_pci_device_pre_load, + .post_load = s390_pci_device_post_load, + .fields = (const VMStateField[]) { + VMSTATE_UINT32(state, S390PCIBusDevice), + VMSTATE_UINT16(uid, S390PCIBusDevice), + VMSTATE_UINT32(idx, S390PCIBusDevice), + VMSTATE_UINT32(fh, S390PCIBusDevice), + VMSTATE_UINT32(fid, S390PCIBusDevice), + VMSTATE_BOOL(fid_defined, S390PCIBusDevice), + VMSTATE_UINT64(fmb_addr, S390PCIBusDevice), + VMSTATE_UINT32(fmb.format, S390PCIBusDevice), + VMSTATE_UINT32(fmb.sample, S390PCIBusDevice), + VMSTATE_UINT64(fmb.last_update, S390PCIBusDevice), + VMSTATE_UINT64_ARRAY(fmb.counter, S390PCIBusDevice, + ARRAY_SIZE(((S390PCIBusDevice *)0)->fmb.counter)), + VMSTATE_UINT64(fmb.fmt0.dma_rbytes, S390PCIBusDevice), + VMSTATE_UINT64(fmb.fmt0.dma_wbytes, S390PCIBusDevice), + VMSTATE_UINT8(isc, S390PCIBusDevice), + VMSTATE_UINT16(noi, S390PCIBusDevice), + VMSTATE_UINT8(sum, S390PCIBusDevice), + VMSTATE_UINT8(pft, S390PCIBusDevice), + VMSTATE_UINT64(routes.adapter.ind_addr, S390PCIBusDevice), + VMSTATE_UINT64(routes.adapter.summary_addr, S390PCIBusDevice), + VMSTATE_UINT64(routes.adapter.ind_offset, S390PCIBusDevice), + VMSTATE_UINT32(routes.adapter.summary_offset, S390PCIBusDevice), + VMSTATE_UINT32(routes.adapter.adapter_id, S390PCIBusDevice), + VMSTATE_BOOL(iommu_enabled, S390PCIBusDevice), + VMSTATE_UINT64(g_iota, S390PCIBusDevice), + VMSTATE_UINT64(pba, S390PCIBusDevice), + VMSTATE_UINT64(pal, S390PCIBusDevice), + VMSTATE_UINT64(max_dma_limit, S390PCIBusDevice), + VMSTATE_PTR_TO_IND_ADDR(summary_ind, S390PCIBusDevice), + VMSTATE_PTR_TO_IND_ADDR(indicator, S390PCIBusDevice), + VMSTATE_BOOL(pci_unplug_request_processed, S390PCIBusDevice), + VMSTATE_BOOL(unplug_requested, S390PCIBusDevice), + VMSTATE_BOOL(interp, S390PCIBusDevice), + VMSTATE_BOOL(forwarding_assist, S390PCIBusDevice), + VMSTATE_BOOL(aif, S390PCIBusDevice), + VMSTATE_BOOL(rtr_avail, S390PCIBusDevice), + VMSTATE_END_OF_LIST() + } };static void s390_pci_device_class_init(ObjectClass *klass, const void *data)diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index f93db10c81..6b0742d143 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c@@ -1122,7 +1122,7 @@ static int fmb_do_update(S390PCIBusDevice *pbdev, int offset, uint64_t val,return ret; } -static void fmb_update(void *opaque) +void fmb_update(void *opaque) { S390PCIBusDevice *pbdev = opaque; int64_t t = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL); diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 25a9fa4955..d8975da4f1 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c@@ -931,6 +931,10 @@ static void ccw_machine_11_1_instance_options(MachineState *machine)static void ccw_machine_11_1_class_options(MachineClass *mc) { + static GlobalProperty compat[] = {+ { TYPE_S390_PCI_HOST_BRIDGE, "x-zpci-emul-dev-migr-enabled", "off" },+ }; + compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); } DEFINE_CCW_MACHINE_AS_LATEST(11, 1);diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390- pci-bus.hindex 17ecf3e0da..7386404ecc 100644 --- a/include/hw/s390x/s390-pci-bus.h +++ b/include/hw/s390x/s390-pci-bus.h @@ -340,6 +340,7 @@ struct S390PCIBusDevice { uint16_t uid; uint32_t idx; uint32_t fh; + Error *passthrough_migr_blocker; uint32_t fid; bool fid_defined; uint64_t fmb_addr; @@ -393,6 +394,8 @@ struct S390pciState { QTAILQ_HEAD(, S390PCIDMACount) zpci_dma_limit; QTAILQ_HEAD(, S390PCIGroup) zpci_groups; uint8_t next_sim_grp; + bool emul_dev_migr_enabled; + Error *emul_dev_migr_blocker;Nit: Maybe this should be renamed as emul_dev_migr_error?
"_blocker" is pretty consistently used for these throughout QEMU, that's why I used it too. Despite it being an "Error*", it's actually (mis)used as a "handle" representing the blocker.
}; S390pciState *s390_get_phb(void);@@ -418,5 +421,6 @@ S390PCIBusDevice *s390_pci_find_dev_by_pci(S390pciState *s,S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,S390PCIBusDevice *pbdev);void s390_pci_ism_reset(void); +void fmb_update(void *opaque); #endif
