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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Oct 27, 2021 at 11:59:45AM +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:
> "failover-legacy-mac"
> 
> If an alternate MAC address is provided with "failover-legacy-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 "failover-legacy-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>

Wait a second. What if config is read before features are set?
Are we going to provide a legacy or a new mac?
I guess current guests do not do this, but the spec does allow this,
and then the mac will apparently change for the guests.

It might be cleaner to just add a PRIMARY_MAC feature -
would need guest work though ...


> ---
> 
> Notes:
>     v2: rename alt-mac to failover-legacy-mac
>         update doc with text provided by MST
> 
>  docs/system/virtio-net-failover.rst | 10 ++++++
>  hw/net/virtio-net.c                 | 48 +++++++++++++++++++++++------
>  include/hw/virtio/virtio-net.h      |  6 ++++
>  3 files changed, 55 insertions(+), 9 deletions(-)
> 
> diff --git a/docs/system/virtio-net-failover.rst b/docs/system/virtio-net-failover.rst
> index 6002dc5d96e4..99f21cd55ef7 100644
> --- a/docs/system/virtio-net-failover.rst
> +++ b/docs/system/virtio-net-failover.rst
> @@ -51,6 +51,16 @@ Usage
>    is only for pairing the devices within QEMU. The guest kernel module
>    net_failover will match devices with identical MAC addresses.
>  
> +  For legacy guests (including bios/EUFI) not supporting VIRTIO_NET_F_STANDBY,
> +  two options exist:
> +
> +  1. if failover-legacy-mac has not been configured (default)
> +     only the standby virtio-net device is visible to the guest
> +
> +  2. if failover-legacy-mac has been configured, virtio and vfio devices will
> +     be presented to guest as two NIC devices, with virtio using the
> +     failover-legacy-mac address.
> +
>  Hotplug
>  -------
>  
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index f2014d5ea0b3..0d47d287de14 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->legacy_mac, &zero, sizeof(zero)) != 0 &&
> +                memcmp(n->mac, &n->legacy_mac, ETH_ALEN) == 0) {
> +                /*
> +                 * set_features can be called twice, without & with F_STANDBY,
> +                 * so restore original MAC address

Restore can be confusing. Let's just say that we set it to XX.

> +                 */
> +                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->legacy_mac, &zero, sizeof(zero)) != 0) {

add a comment here too?

> +            memcpy(n->mac, &n->legacy_mac, 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->legacy_mac, &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("failover-legacy-mac",  VirtIONet, legacy_mac),
>      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..4b9523def5fb 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 legacy_mac;
>      VirtioNetRssData rss_data;
>      struct NetRxPkt *rx_pkt;
>      struct EBPFRSSContext ebpf_rss;
> -- 
> 2.31.1