Re: [PATCH v11 15/16] s390x/pci: Implement migration for emulated devices

[Matthew Rosato <mjrosato@linux.ibm.com>]

> +/* 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