Re: [PATCH] failover: specify an alternate MAC address

[Michael S. Tsirkin]

On Tue, Oct 26, 2021 at 05:03:05PM +0200, Laurent Vivier wrote:
> If the guest driver doesn't support the STANDBY feature, by default
> we keep the virtio-net device and don't hotplug the VFIO device,
> but in some cases, user can prefer to use the VFIO device rather
> than the virtio-net one. We can't unplug the virtio-net device
> (because on migration it is expected on the destination side) but
> we can keep both interfaces if the MAC addresses are different
> (to have the same MAC address can cause kernel crash with old
> kernel). The VFIO device will be unplugged before the migration
> like in the normal failover migration but without a failover device.
> 
> This patch adds a new property to the virtio-net device: "alt-mac"

I'd rather give it some kind of name that reflects that
it's related to failover, and to legacy guests not
supporting STANDBY.  failover-legacy-mac ?

> 
> If an alternate MAC address is provided with "alt-mac" and the
> STANDBY feature is not supported, both interfaces are plugged
> but the standby interface (virtio-net) MAC address is set to the
> value provided by the "alt-mac" parameter.
> 
> If the STANDBY feature is supported by guest and QEMU, the virtio-net
> failover acts as usual.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  docs/system/virtio-net-failover.rst | 17 ++++++++++
>  hw/net/virtio-net.c                 | 48 +++++++++++++++++++++++------
>  include/hw/virtio/virtio-net.h      |  6 ++++
>  3 files changed, 62 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/system/virtio-net-failover.rst 
> b/docs/system/virtio-net-failover.rst
> index 6002dc5d96e4..0b1699169ef6 100644
> --- a/docs/system/virtio-net-failover.rst
> +++ b/docs/system/virtio-net-failover.rst
> @@ -51,6 +51,23 @@ Usage
>    is only for pairing the devices within QEMU. The guest kernel module
>    net_failover will match devices with identical MAC addresses.
>  
> +  The VIRTIO_NET_F_STANDBY must be supported by both sides, QEMU and guest
> +  kernel, to allow the guest kernel module to pair the devices.
> +  If the kernel module doesn't support the feature, only the standby device
> +  (virtio-net) is plugged, this allows to do a live migration without any
> +  connection loss.
> +
> +  In this case, You can force QEMU to keep both interfaces by providing an

you ?

> +  alternate MAC address to the virtio-net device with the parameter alt-mac.
> +  Both interfaces will be available separately:
> +
> +  -device virtio-net-pci,netdev=hostnet1,id=net1,mac=52:54:00:6f:55:cc, \
> +    bus=root2,failover=on,alt-mac=52:54:00:12:34:56
> +
> +  As in the failover case, the (vfio)-pci device will be unplugged
> +  automatically before the migration and plugged back on the destination side
> +  after the migration.

That's a weird way to explain this. I'd rather the documentation
just listed both options in a straight forward way.
And there's no need to repeat here how feature is used etc.

E.g. along the lines of

        For legacy guests (including bios/EUFI) not supporting
        VIRTIO_NET_F_STANDBY, two options exist:
        
        1. if alt-mac has not been configured (default)
        only the standby virtio-net device is visible to the guest

        2. if alt-mac has been configured, virtio and vfio devices will be
        presented to guest as two NIC devices, with virtio using
        the alt-mac address.




> +
>  Hotplug
>  -------
>  
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index f2014d5ea0b3..7aba2ac78f87 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -45,6 +45,9 @@
>  #include "net_rx_pkt.h"
>  #include "hw/virtio/vhost.h"
>  
> +/* zero MAC address to check with */
> +static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
> +
>  #define VIRTIO_NET_VM_VERSION    11
>  
>  #define MAC_TABLE_ENTRIES    64
> @@ -126,7 +129,6 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
> uint8_t *config)
>      VirtIONet *n = VIRTIO_NET(vdev);
>      struct virtio_net_config netcfg;
>      NetClientState *nc = qemu_get_queue(n->nic);
> -    static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };
>  
>      int ret = 0;
>      memset(&netcfg, 0 , sizeof(struct virtio_net_config));
> @@ -871,10 +873,21 @@ static void failover_add_primary(VirtIONet *n, Error 
> **errp)
>      error_propagate(errp, err);
>  }
>  
> +static void failover_plug_primary(VirtIONet *n)
> +{
> +    Error *err = NULL;
> +
> +    qapi_event_send_failover_negotiated(n->netclient_name);
> +    qatomic_set(&n->failover_primary_hidden, false);
> +    failover_add_primary(n, &err);
> +    if (err) {
> +        warn_report_err(err);
> +    }
> +}
> +
>  static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
> -    Error *err = NULL;
>      int i;
>  
>      if (n->mtu_bypass_backend &&
> @@ -921,12 +934,22 @@ static void virtio_net_set_features(VirtIODevice *vdev, 
> uint64_t features)
>          memset(n->vlans, 0xff, MAX_VLAN >> 3);
>      }
>  
> -    if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
> -        qapi_event_send_failover_negotiated(n->netclient_name);
> -        qatomic_set(&n->failover_primary_hidden, false);
> -        failover_add_primary(n, &err);
> -        if (err) {
> -            warn_report_err(err);
> +    if (n->failover) {
> +        if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
> +            if (memcmp(&n->altmac, &zero, sizeof(zero)) != 0 &&
> +                memcmp(n->mac, &n->altmac, ETH_ALEN) == 0) {
> +                /*
> +                 * set_features can be called twice, without & with 
> F_STANDBY,
> +                 * so restore original MAC address
> +                 */
> +                memcpy(n->mac, &n->nic->conf->macaddr, sizeof(n->mac));
> +                qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> +            }
> +            failover_plug_primary(n);
> +        } else if (memcmp(&n->altmac, &zero, sizeof(zero)) != 0) {
> +            memcpy(n->mac, &n->altmac, ETH_ALEN);
> +            qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> +            failover_plug_primary(n);
>          }
>      }
>  }
> @@ -3595,9 +3618,15 @@ static bool primary_unplug_pending(void *opaque)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIONet *n = VIRTIO_NET(vdev);
>  
> -    if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
> +    if (!n->failover) {
>          return false;
>      }
> +
> +    if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY) &&
> +        memcmp(&n->altmac, &zero, sizeof(zero)) == 0) {
> +        return false;
> +    }
> +
>      primary = failover_find_primary_device(n);
>      return primary ? primary->pending_deleted_event : false;
>  }
> @@ -3672,6 +3701,7 @@ static Property virtio_net_properties[] = {
>      DEFINE_PROP_UINT32("rsc_interval", VirtIONet, rsc_timeout,
>                         VIRTIO_NET_RSC_DEFAULT_INTERVAL),
>      DEFINE_NIC_PROPERTIES(VirtIONet, nic_conf),
> +    DEFINE_PROP_MACADDR("alt-mac",  VirtIONet, altmac),
>      DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
>                         TX_TIMER_INTERVAL),
>      DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index eb87032627d2..c768c7662002 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -213,6 +213,12 @@ struct VirtIONet {
>      QDict *primary_opts;
>      bool primary_opts_from_json;
>      Notifier migration_state;
> +    /*
> +     * failover: to provide an alternate MAC address allows to keep both
> +     * cards, primary and stand-by, if the STANDBY feature is not supported
> +     * by the guest
> +     */
> +    MACAddr altmac;
>      VirtioNetRssData rss_data;
>      struct NetRxPkt *rx_pkt;
>      struct EBPFRSSContext ebpf_rss;
> -- 
> 2.31.1