Re: [Qemu-devel] [PATCH 3/4] net/virtio: add failover support

[Dr. David Alan Gilbert]

* Jens Freimann (address@hidden) wrote:
> This patch adds support to handle failover device pairs of a virtio-net
> device and a vfio-pci device, where the virtio-net acts as the standby
> device and the vfio-pci device as the primary.
> 
> The general idea is that we have a pair of devices, a vfio-pci and a
> emulated (virtio-net) device. Before migration the vfio device is
> unplugged and data flows to the emulated device, on the target side
> another vfio-pci device is plugged in to take over the data-path. In the
> guest the net_failover module will pair net devices with the same MAC
> address.
> 
> To achieve this we need:
> 
> 1. Provide a callback function for the should_be_hidden DeviceListener.
>    It is called when the primary device is plugged in. Evaluate the QOpt
>    passed in to check if it is the matching primary device. It returns
>    two values:
>      - one to signal if the device to be added is the matching
>        primary device
>      - another one to signal to qdev if it should actually
>        continue with adding the device or skip it.
> 
>    In the latter case it stores the device options in the VirtioNet
>    struct and the device is added once the VIRTIO_NET_F_STANDBY feature is
>    negotiated during virtio feature negotiation.
> 
> 2. Register a callback for migration status notifier. When called it
>    will unplug its primary device before the migration happens.
> 
> Signed-off-by: Jens Freimann <address@hidden>
> ---
>  hw/net/virtio-net.c            | 117 +++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-net.h |  12 ++++
>  2 files changed, 129 insertions(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index ffe0872fff..120eccbb98 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -12,6 +12,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/atomic.h"
>  #include "qemu/iov.h"
>  #include "hw/virtio/virtio.h"
>  #include "net/net.h"
> @@ -19,6 +20,10 @@
>  #include "net/tap.h"
>  #include "qemu/error-report.h"
>  #include "qemu/timer.h"
> +#include "qemu/option.h"
> +#include "qemu/option_int.h"
> +#include "qemu/config-file.h"
> +#include "qapi/qmp/qdict.h"
>  #include "hw/virtio/virtio-net.h"
>  #include "net/vhost_net.h"
>  #include "net/announce.h"
> @@ -29,6 +34,8 @@
>  #include "migration/misc.h"
>  #include "standard-headers/linux/ethtool.h"
>  #include "trace.h"
> +#include "monitor/qdev.h"
> +#include "hw/pci/pci.h"
>  
>  #define VIRTIO_NET_VM_VERSION    11
>  
> @@ -364,6 +371,9 @@ static void virtio_net_set_status(struct VirtIODevice 
> *vdev, uint8_t status)
>      }
>  }
>  
> +
> +static void virtio_net_primary_plug_timer(void *opaque);
> +
>  static void virtio_net_set_link_status(NetClientState *nc)
>  {
>      VirtIONet *n = qemu_get_nic_opaque(nc);
> @@ -786,6 +796,14 @@ static void virtio_net_set_features(VirtIODevice *vdev, 
> uint64_t features)
>      } else {
>          memset(n->vlans, 0xff, MAX_VLAN >> 3);
>      }
> +
> +    if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
> +        atomic_set(&n->primary_should_be_hidden, false);
> +        if (n->primary_device_timer)
> +            timer_mod(n->primary_device_timer,
> +                qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> +                4000);
> +    }

What's this magic timer constant and why?

>  }
>  
>  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
> @@ -2626,6 +2644,87 @@ void virtio_net_set_netclient_name(VirtIONet *n, const 
> char *name,
>      n->netclient_type = g_strdup(type);
>  }
>  
> +static void virtio_net_primary_plug_timer(void *opaque)
> +{
> +    VirtIONet *n = opaque;
> +    Error *err = NULL;
> +
> +    if (n->primary_device_dict)
> +        n->primary_device_opts = 
> qemu_opts_from_qdict(qemu_find_opts("device"),
> +            n->primary_device_dict, &err);
> +    if (n->primary_device_opts) {
> +        n->primary_dev = qdev_device_add(n->primary_device_opts, &err);
> +        error_setg(&err, "virtio_net: couldn't plug in primary device");
> +        return;
> +    }
> +    if (!n->primary_device_dict && err) {
> +        if (n->primary_device_timer) {
> +            timer_mod(n->primary_device_timer,
> +                qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> +                100);

same here.


> +        }
> +    }
> +}
> +
> +static void virtio_net_handle_migration_primary(VirtIONet *n,
> +                                                MigrationState *s)
> +{
> +    Error *err = NULL;
> +    bool should_be_hidden = atomic_read(&n->primary_should_be_hidden);
> +
> +    n->primary_dev = qdev_find_recursive(sysbus_get_default(),
> +            n->primary_device_id);
> +    if (!n->primary_dev) {
> +        error_setg(&err, "virtio_net: couldn't find primary device");

There's something broken with the error handling in this function - the
'err' never goes anywhere - I don't think it ever gets printed or
reported or stops the migration.

> +    }
> +    if (migration_in_setup(s) && !should_be_hidden && n->primary_dev) {
> +        qdev_unplug(n->primary_dev, &err);

Not knowing unplug well; can you just explain - is that device hard
unplugged and it's gone by the time this function returns or is it still
hanging around for some indeterminate time?

> +        if (!err) {
> +            atomic_set(&n->primary_should_be_hidden, true);
> +            n->primary_dev = NULL;
> +        }
> +    } else if (migration_has_failed(s)) {
> +        if (should_be_hidden && !n->primary_dev) {
> +            /* We already unplugged the device let's plugged it back */
> +            n->primary_dev = qdev_device_add(n->primary_device_opts, &err);
> +        }
> +    }
> +}
> +
> +static void migration_state_notifier(Notifier *notifier, void *data)
> +{
> +    MigrationState *s = data;
> +    VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
> +    virtio_net_handle_migration_primary(n, s);
> +}
> +
> +static void virtio_net_primary_should_be_hidden(DeviceListener *listener,
> +            QemuOpts *device_opts, bool *match_found, bool *res)
> +{
> +    VirtIONet *n = container_of(listener, VirtIONet, primary_listener);
> +
> +    if (device_opts) {
> +        n->primary_device_dict = qemu_opts_to_qdict(device_opts,
> +                n->primary_device_dict);
> +    }
> +    g_free(n->standby_id);
> +    n->standby_id = g_strdup(qdict_get_try_str(n->primary_device_dict,
> +                             "standby"));
> +    if (n->standby_id) {
> +        *match_found = true;
> +    }
> +    /* primary_should_be_hidden is set during feature negotiation */
> +    if (atomic_read(&n->primary_should_be_hidden) && *match_found) {
> +        *res = true;
> +    } else if (*match_found)  {
> +        n->primary_device_dict = qemu_opts_to_qdict(device_opts,
> +                n->primary_device_dict);
> +        *res = false;
> +    }
> +    g_free(n->primary_device_id);
> +    n->primary_device_id = g_strdup(device_opts->id);
> +}
> +
>  static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -2656,6 +2755,18 @@ static void virtio_net_device_realize(DeviceState 
> *dev, Error **errp)
>          n->host_features |= (1ULL << VIRTIO_NET_F_SPEED_DUPLEX);
>      }
>  
> +    if (n->failover) {
> +        n->primary_listener.should_be_hidden =
> +            virtio_net_primary_should_be_hidden;
> +        atomic_set(&n->primary_should_be_hidden, true);
> +        device_listener_register(&n->primary_listener);
> +        n->migration_state.notify = migration_state_notifier;
> +        add_migration_state_change_notifier(&n->migration_state);
> +        n->host_features |= (1ULL << VIRTIO_NET_F_STANDBY);
> +        n->primary_device_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
> +                                     virtio_net_primary_plug_timer, n);
> +    }
> +
>      virtio_net_set_config_size(n, n->host_features);
>      virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
>  
> @@ -2778,6 +2889,11 @@ static void virtio_net_device_unrealize(DeviceState 
> *dev, Error **errp)
>      g_free(n->mac_table.macs);
>      g_free(n->vlans);
>  
> +    g_free(n->primary_device_id);
> +    g_free(n->standby_id);
> +    qobject_unref(n->primary_device_dict);
> +    n->primary_device_dict = NULL;
> +
>      max_queues = n->multiqueue ? n->max_queues : 1;
>      for (i = 0; i < max_queues; i++) {
>          virtio_net_del_queue(n, i);
> @@ -2885,6 +3001,7 @@ static Property virtio_net_properties[] = {
>                       true),
>      DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
>      DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
> +    DEFINE_PROP_BOOL("failover", VirtIONet, failover, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index b96f0c643f..c2bb6ada44 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -18,6 +18,7 @@
>  #include "standard-headers/linux/virtio_net.h"
>  #include "hw/virtio/virtio.h"
>  #include "net/announce.h"
> +#include "qemu/option_int.h"
>  
>  #define TYPE_VIRTIO_NET "virtio-net-device"
>  #define VIRTIO_NET(obj) \
> @@ -43,6 +44,7 @@ typedef struct virtio_net_conf
>      int32_t speed;
>      char *duplex_str;
>      uint8_t duplex;
> +    char *primary_id_str;
>  } virtio_net_conf;
>  
>  /* Coalesced packets type & status */
> @@ -185,6 +187,16 @@ struct VirtIONet {
>      AnnounceTimer announce_timer;
>      bool needs_vnet_hdr_swap;
>      bool mtu_bypass_backend;
> +    QemuOpts *primary_device_opts;
> +    QDict *primary_device_dict;
> +    DeviceState *primary_dev;
> +    char *primary_device_id;
> +    char *standby_id;
> +    bool primary_should_be_hidden;
> +    bool failover;
> +    DeviceListener primary_listener;
> +    QEMUTimer *primary_device_timer;
> +    Notifier migration_state;
>  };
>  
>  void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
> -- 
> 2.21.0
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK