Re: [Qemu-devel] [virtio-dev] [PATCH v3 2/5] virtio_net: Add support for "Data Path Switching" during Live Migration.

[Cornelia Huck]

On Mon,  7 Jan 2019 17:29:41 -0500
Venu Busireddy <address@hidden> wrote:

> Added a new event, FAILOVER_STANDBY_CHANGED, which is emitted whenever
> the status of the virtio_net driver in the guest changes (either the
> guest successfully loads the driver after the F_STANDBY feature bit
> is negotiated, or the guest unloads the driver or reboots). Management
> stack can use this event to determine when to plug/unplug the VF device
> to/from the guest.
> 
> Also, the Virtual Functions will be automatically removed from the guest
> if the guest is rebooted. To properly identify the VFIO devices that
> must be removed, a new property named "failover-primary" is added to
> the vfio-pci devices. Only the vfio-pci devices that have this property
> enabled are removed from the guest upon reboot.
> 
> Signed-off-by: Venu Busireddy <address@hidden>
> ---
>  hw/acpi/pcihp.c      | 27 +++++++++++++++++++++++++++
>  hw/net/virtio-net.c  | 24 ++++++++++++++++++++++++
>  hw/vfio/pci.c        |  3 +++
>  hw/vfio/pci.h        |  1 +
>  include/hw/pci/pci.h |  1 +
>  qapi/net.json        | 28 ++++++++++++++++++++++++++++
>  6 files changed, 84 insertions(+)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 80d42e1..2a3ffd3 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -176,6 +176,25 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, 
> unsigned bsel, unsigned slo
>      }
>  }
>  
> +static void acpi_pcihp_cleanup_failover_primary(AcpiPciHpState *s, int bsel)
> +{
> +    BusChild *kid, *next;
> +    PCIBus *bus = acpi_pcihp_find_hotplug_bus(s, bsel);
> +
> +    if (!bus) {
> +        return;
> +    }
> +    QTAILQ_FOREACH_SAFE(kid, &bus->qbus.children, sibling, next) {
> +        DeviceState *qdev = kid->child;
> +        PCIDevice *pdev = PCI_DEVICE(qdev);
> +        int slot = PCI_SLOT(pdev->devfn);
> +
> +        if (pdev->failover_primary) {
> +            s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
> +        }
> +    }
> +}
> +
>  static void acpi_pcihp_update_hotplug_bus(AcpiPciHpState *s, int bsel)
>  {
>      BusChild *kid, *next;
> @@ -207,6 +226,14 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
>      int i;
>  
>      for (i = 0; i < ACPI_PCIHP_MAX_HOTPLUG_BUS; ++i) {
> +        /*
> +         * Set the acpi_pcihp_pci_status[].down bits of all the
> +         * failover_primary devices so that the devices are ejected
> +         * from the guest. We can't use the qdev_unplug() as well as the
> +         * hotplug_handler to unplug the devices, because the guest may
> +         * not be in a state to cooperate.
> +         */
> +        acpi_pcihp_cleanup_failover_primary(s, i);
>          acpi_pcihp_update_hotplug_bus(s, i);
>      }
>  }


It seems that you rely on acpi to get the processing right. On a
non-acpi system, you won't get the required changes done. Maybe only
advertise the failover feature if you are actually on a system that
supports handling of the primary correctly (which, at least currently,
means a system with acpi)?

> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 411f8fb..7b1bcde 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -248,6 +248,29 @@ static void virtio_net_drop_tx_queue_data(VirtIODevice 
> *vdev, VirtQueue *vq)
>      }
>  }
>  
> +static void virtio_net_failover_notify_event(VirtIONet *n, uint8_t status)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> +
> +    if (virtio_has_feature(vdev->guest_features, VIRTIO_NET_F_STANDBY)) {
> +        const char *ncn = n->netclient_name;
> +        gchar *path = object_get_canonical_path(OBJECT(n->qdev));
> +        /*
> +         * Emit FAILOVER_STANDBY_CHANGED event with enabled=true
> +         *   when the status transitions from 0 to VIRTIO_CONFIG_S_DRIVER_OK
> +         * Emit FAILOVER_STANDBY_CHANGED event with enabled=false
> +         *   when the status transitions from VIRTIO_CONFIG_S_DRIVER_OK to 0
> +         */
> +        if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> +                (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK))) {
> +            qapi_event_send_failover_standby_changed(!!ncn, ncn, path, true);
> +        } else if ((!(status & VIRTIO_CONFIG_S_DRIVER_OK)) &&
> +                (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +            qapi_event_send_failover_standby_changed(!!ncn, ncn, path, 
> false);
> +        }

Do you also need a notification if something goes wrong in the guest
and it sets VIRTIO_CONFIG_S_FAILED?

> +    }
> +}
> +
>  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);