> +/* 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.
> + * For migration to succeed, (idstr+instance_id) must match those
> generated
> + * during QEMU start. Without unique idstr, QEMU will generate variable
> + * instance_id to distinguish devices, and that instance_id can change
> + * if a device is unplugged and plugged back, preventing migration.
> + */
> + .class_init = s390_pcibus_class_init,
This patch is already pretty big. I wonder if adding
s390_pci_bus_get_dev_path + setting get_dev_path could be its own patch
prior to this one - up to you.
[...]
> +static bool s390_pci_device_post_load_errp(void *opaque, int version_id,
> + Error **errp)
> +{
> + S390PCIBusDevice *pbdev = S390_PCI_DEVICE(opaque);
> +
> + /*
> + * Guard against the scenario that s390_pcihost_plug() of the target PCI
> + * device succeeded in the source QEMU, but failed on this destination
> + * QEMU, before migration state load. In this case we'll find
> !pbdev->pdev
> + * but the pbdev->state != ZPCI_FS_RESERVED as just loaded from the
> stream.
> + * Such value combination is invalid and migration should fail.
> + */
> + if (pbdev->state != ZPCI_FS_RESERVED && !pbdev->pdev) {
> + error_setg(errp, "zpci device uid 0x%x state %d has no PCI device",
> + pbdev->uid, pbdev->state);
> + return false;
> + }
> +
> + pbdev->zpci_fn.fid = pbdev->fid;
> + pbdev->zpci_fn.uid = pbdev->uid;
> +
> + /*
> + * 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.
> + */
> + 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) {
> + if (!pbdev->iommu) {
> + error_setg(errp, "iommu is NULL");
> + return false;
> + }
> + if (!s390_pci_ioat_validate(pbdev, pbdev->pba, pbdev->pal,
> + pbdev->g_iota, errp)) {
> + if (*errp) {
> + error_prepend(errp,
> + "invalid pba, pal or g_iota in migration
> stream: ");
> + } else {
> + error_setg(errp,
> + "invalid pba, pal or g_iota in migration stream");
> + }
> + return false;
> + }
> + if (s390_pci_is_translation_enabled(pbdev->g_iota)) {
> + s390_pci_iommu_enable(pbdev);
> + s390_pci_ioat_replay(pbdev);
> + } else {
> + /* TODO: unreachable until vfio passthrough migration is enabled
> */
> + s390_pci_iommu_direct_map_enable(pbdev);
I would further clarify that this is because rtr_avail is always set to
false for emulated devices today.
Also, besides vfio migration, it could also be reachable if were to ever
allow rtr_avail for emulated devices.
But I wonder: if we can never reach this code today, should this then be
a g_assert_not_reached() and improve the comment above to indicate that
if/when either emulated devices are allowed to set rtr_avail or
migration of VFIO devices are added, this path needs to be updated to
call s390_pci_iommu_direct_map_enable(pbdev)?
If/when either of those things happen in the future it would already
need to be on a machine-version boundary whether this call were in-place
or not. And then we don't worry about devices driving this codepath
that should not be able to do so.
[...]
> struct S390pciState {
> PCIHostState parent_obj;
> uint32_t next_idx;
> @@ -386,11 +390,14 @@ struct S390pciState {
> S390PCIBus *bus;
> GHashTable *iommu_table;
> GHashTable *zpci_table;
> - QTAILQ_HEAD(, SeiContainer) pending_sei;
> + SeiContainerList pending_sei;
> + /* Only used temporarily between migration pre_load and post_load. */
> + SeiContainerList pending_sei_stash;
Hrm, I don't love that -- but I can't think of a better solution either.
I guess if we find anything else that needs this kind of treatment in
the future we should create a single object chained off of S390pciState
to hold all of them vs adding more of these to S390pciState.
Thanks,
Matt
