[PATCH] failover: specify an alternate MAC address

Laurent Vivier posted 1 patch 2 years, 6 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211026150305.51962-1-lvivier@redhat.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>
There is a newer version of this series
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(-)
[PATCH] failover: specify an alternate MAC address
Posted by Laurent Vivier 2 years, 6 months ago
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"

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
+  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.
+
 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


Re: [PATCH] failover: specify an alternate MAC address
Posted by Michael S. Tsirkin 2 years, 6 months ago
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