All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one
@ 2021-08-09 17:13 Laurent Vivier
  2021-08-09 17:13 ` [PATCH v2 1/1] virtio: failover: define the default device to use in case of error Laurent Vivier
  2021-08-11  4:17 ` [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Jason Wang
  0 siblings, 2 replies; 8+ messages in thread
From: Laurent Vivier @ 2021-08-09 17:13 UTC (permalink / raw)
  To: qemu-devel; +Cc: Juan Quintela, Jason Wang, Jens Freimann, Michael S. Tsirkin

v2: use validate_features() to disable the guest driver rather
    than setting vring.num to 0.

With failover, when the guest virtio-net driver doesn't support the
STANDBY feature, the primary device is not plugged and only the virtio-net
device is kept. Doing like that we can migrate the machine and
keep the network connection.

But in some cases, when performance is more important than availability
we would prefer to keep the VFIO device rather than the virtio-net one,
even if it means to lose the network connection during the migration of
the machine.

To do that we can't simply unplug the virtio-net device and plug the
VFIO one because for the migration the initial state must be kept
(virtio-net plugged, VFIO unplugged) but we can try to disable the
virtio-net driver and plug the VFIO card, so the initial state is
correct (the virtio-net card is plugged, but disabled in guest, and
the VFIO card is unplugged before migration).

This change doesn't impact the case when guest and host support
the STANDBY feature.

I've introduced the "failover-default" property to virtio-net device
to set which device to keep (failover-default=true keeps the virtio-net
device, =off the other one).

For example, with a guest driver that doesn't support STANDBY:

  ...
  -device virtio-net-pci,id=virtio0,failover=on,failover-default=on \
  -device vfio-pci,host=$PCI,id=hostdev0,failover_pair_id=virtio0 \
  ...

  [root@localhost ~]# ip a
  1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1
      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
      inet 127.0.0.1/8 scope host lo
         valid_lft forever preferred_lft forever
      inet6 ::1/128 scope host
         valid_lft forever preferred_lft forever
  2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP q0
      link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
      inet 192.168.20.2/24 brd 192.168.20.255 scope global eth0
         valid_lft forever preferred_lft forever
      inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
         valid_lft forever preferred_lft forever
  # ethtool -i eth0
  driver: virtio_net
  version: 1.0.0
  firmware-version:
  expansion-rom-version:
  bus-info: 0000:04:00.0
  supports-statistics: no
  supports-test: no
  supports-eeprom-access: no
  supports-register-dump: no
  supports-priv-flags: no

  ...
  -device virtio-net-pci,id=virtio0,failover=on,failover-default=off \
  -device vfio-pci,host=$PCI,id=hostdev0,failover_pair_id=virtio0 \
  ...

  [root@localhost ~]# ip a
  1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1
      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
      inet 127.0.0.1/8 scope host lo
         valid_lft forever preferred_lft forever
      inet6 ::1/128 scope host
         valid_lft forever preferred_lft forever
  2: enp2s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 100
      link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
      inet 192.168.20.2/24 brd 192.168.20.255 scope global enp2s0
         valid_lft forever preferred_lft forever
      inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
         valid_lft forever preferred_lft forever
  [root@localhost ~]# ethtool -i enp2s0
  driver: i40evf
  version: 1.6.27-k
  firmware-version: N/A
  expansion-rom-version:
  bus-info: 0000:02:00.0
  supports-statistics: yes
  supports-test: no
  supports-eeprom-access: no
  supports-register-dump: no
  supports-priv-flags: no

With guest driver that supports STANDBY, we would always have:

  [root@localhost ~]# ip a
  1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group defau0
      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
      inet 127.0.0.1/8 scope host lo
         valid_lft forever preferred_lft forever
      inet6 ::1/128 scope host
         valid_lft forever preferred_lft forever
  2: enp4s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP gr0
      link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
      inet 192.168.20.2/24 brd 192.168.20.255 scope global noprefixroute enp4s0
         valid_lft forever preferred_lft forever
      inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
         valid_lft forever preferred_lft forever
  3: enp4s0nsby: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel master0
      link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
  4: enp2s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq master enp4s0 st0
      link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
  [root@localhost ~]# ethtool -i enp4s0
  driver: net_failover
  version: 0.1
  firmware-version:
  expansion-rom-version:
  bus-info:
  supports-statistics: no
  supports-test: no
  supports-eeprom-access: no
  supports-register-dump: no
  supports-priv-flags: no
  [root@localhost ~]# ethtool -i enp4s0nsby
  driver: virtio_net
  version: 1.0.0
  firmware-version:
  expansion-rom-version:
  bus-info: 0000:04:00.0
  supports-statistics: yes
  supports-test: no
  supports-eeprom-access: no
  supports-register-dump: no
  supports-priv-flags: no
  [root@localhost ~]# ethtool -i enp2s0
  driver: iavf
  version: 4.18.0-310.el8.x86_64
  firmware-version: N/A
  expansion-rom-version:
  bus-info: 0000:02:00.0
  supports-statistics: yes
  supports-test: no
  supports-eeprom-access: no
  supports-register-dump: no
  supports-priv-flags: yes

Laurent Vivier (1):
  virtio: failover: define the default device to use in case of error

 include/hw/virtio/virtio-net.h |  1 +
 hw/net/virtio-net.c            | 49 +++++++++++++++++++++++++++++-----
 2 files changed, 44 insertions(+), 6 deletions(-)

-- 
2.31.1




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/1] virtio: failover: define the default device to use in case of error
  2021-08-09 17:13 [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Laurent Vivier
@ 2021-08-09 17:13 ` Laurent Vivier
  2021-08-11  4:18   ` Jason Wang
  2021-08-23 23:25   ` Michael S. Tsirkin
  2021-08-11  4:17 ` [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Jason Wang
  1 sibling, 2 replies; 8+ messages in thread
From: Laurent Vivier @ 2021-08-09 17:13 UTC (permalink / raw)
  To: qemu-devel; +Cc: Juan Quintela, Jason Wang, Jens Freimann, Michael S. Tsirkin

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 force the guest driver to be disabled. Then, we can
hotplug the VFIO device that will be unplugged before the migration
like in the normal failover migration but without the failover device.

This patch adds a new property to virtio-net device: "failover-default".

By default, "failover-default" is set to true and thus the default NIC
to use if the failover cannot be enabled is the virtio-net device
(this is what is done until now with the virtio-net failover).

If "failover-default" is set to false, in case of error, the virtio-net
device is not the default anymore and the failover primary device
is used instead.

If the STANDBY feature is supported by guest and host, the virtio-net
failover acts as usual.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 include/hw/virtio/virtio-net.h |  1 +
 hw/net/virtio-net.c            | 49 +++++++++++++++++++++++++++++-----
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 824a69c23f06..ab77930a327e 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -208,6 +208,7 @@ struct VirtIONet {
     /* primary failover device is hidden*/
     bool failover_primary_hidden;
     bool failover;
+    bool failover_default;
     DeviceListener primary_listener;
     Notifier migration_state;
     VirtioNetRssData rss_data;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 16d20cdee52a..972c03232a96 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -935,12 +935,23 @@ 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 the virtio-net driver has the STANDBY feature, we can plug the primary
+     * if not but is not the default failover device,
+     * we need to plug the primary alone and the virtio-net driver will
+     * be disabled in the validate_features() function but validate_features()
+     * is only available with virtio 1.0 spec
+     */
+    if (n->failover) {
+        if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY) ||
+           (virtio_has_feature(features, VIRTIO_F_VERSION_1) &&
+            !n->failover_default)) {
+            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);
+            }
         }
     }
 }
@@ -3625,9 +3636,34 @@ static Property virtio_net_properties[] = {
     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_BOOL("failover-default", VirtIONet, failover_default, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+/* validate_features() is only available with VIRTIO_F_VERSION_1 */
+static int failover_validate_features(VirtIODevice *vdev)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+
+    /*
+     * 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 force the guest driver to be disabled. In this case, We can
+     * hotplug the VFIO device that will be unplugged before the migration
+     * like in the normal failover migration but without the failover device.
+     */
+    if (n->failover && !n->failover_default &&
+        !virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
+        /* disable virtio-net */
+        return -ENODEV;
+    }
+
+    return 0;
+}
+
 static void virtio_net_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -3651,6 +3687,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
     vdc->post_load = virtio_net_post_load_virtio;
     vdc->vmsd = &vmstate_virtio_net_device;
     vdc->primary_unplug_pending = primary_unplug_pending;
+    vdc->validate_features = failover_validate_features;
 }
 
 static const TypeInfo virtio_net_info = {
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one
  2021-08-09 17:13 [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Laurent Vivier
  2021-08-09 17:13 ` [PATCH v2 1/1] virtio: failover: define the default device to use in case of error Laurent Vivier
@ 2021-08-11  4:17 ` Jason Wang
  2021-08-11  6:27   ` Laurent Vivier
  1 sibling, 1 reply; 8+ messages in thread
From: Jason Wang @ 2021-08-11  4:17 UTC (permalink / raw)
  To: Laurent Vivier
  Cc: Juan Quintela, Jens Freimann, qemu-devel, Michael S. Tsirkin

On Tue, Aug 10, 2021 at 1:13 AM Laurent Vivier <lvivier@redhat.com> wrote:
>
> v2: use validate_features() to disable the guest driver rather
>     than setting vring.num to 0.
>
> With failover, when the guest virtio-net driver doesn't support the
> STANDBY feature, the primary device is not plugged and only the virtio-net
> device is kept. Doing like that we can migrate the machine and
> keep the network connection.
>
> But in some cases, when performance is more important than availability
> we would prefer to keep the VFIO device rather than the virtio-net one,
> even if it means to lose the network connection during the migration of
> the machine.

I think we still need to seek a way to recover the network after migration.

Thanks

>
> To do that we can't simply unplug the virtio-net device and plug the
> VFIO one because for the migration the initial state must be kept
> (virtio-net plugged, VFIO unplugged) but we can try to disable the
> virtio-net driver and plug the VFIO card, so the initial state is
> correct (the virtio-net card is plugged, but disabled in guest, and
> the VFIO card is unplugged before migration).
>
> This change doesn't impact the case when guest and host support
> the STANDBY feature.
>
> I've introduced the "failover-default" property to virtio-net device
> to set which device to keep (failover-default=true keeps the virtio-net
> device, =off the other one).
>
> For example, with a guest driver that doesn't support STANDBY:
>
>   ...
>   -device virtio-net-pci,id=virtio0,failover=on,failover-default=on \
>   -device vfio-pci,host=$PCI,id=hostdev0,failover_pair_id=virtio0 \
>   ...
>
>   [root@localhost ~]# ip a
>   1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1
>       link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>       inet 127.0.0.1/8 scope host lo
>          valid_lft forever preferred_lft forever
>       inet6 ::1/128 scope host
>          valid_lft forever preferred_lft forever
>   2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP q0
>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>       inet 192.168.20.2/24 brd 192.168.20.255 scope global eth0
>          valid_lft forever preferred_lft forever
>       inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
>          valid_lft forever preferred_lft forever
>   # ethtool -i eth0
>   driver: virtio_net
>   version: 1.0.0
>   firmware-version:
>   expansion-rom-version:
>   bus-info: 0000:04:00.0
>   supports-statistics: no
>   supports-test: no
>   supports-eeprom-access: no
>   supports-register-dump: no
>   supports-priv-flags: no
>
>   ...
>   -device virtio-net-pci,id=virtio0,failover=on,failover-default=off \
>   -device vfio-pci,host=$PCI,id=hostdev0,failover_pair_id=virtio0 \
>   ...
>
>   [root@localhost ~]# ip a
>   1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1
>       link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>       inet 127.0.0.1/8 scope host lo
>          valid_lft forever preferred_lft forever
>       inet6 ::1/128 scope host
>          valid_lft forever preferred_lft forever
>   2: enp2s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 100
>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>       inet 192.168.20.2/24 brd 192.168.20.255 scope global enp2s0
>          valid_lft forever preferred_lft forever
>       inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
>          valid_lft forever preferred_lft forever
>   [root@localhost ~]# ethtool -i enp2s0
>   driver: i40evf
>   version: 1.6.27-k
>   firmware-version: N/A
>   expansion-rom-version:
>   bus-info: 0000:02:00.0
>   supports-statistics: yes
>   supports-test: no
>   supports-eeprom-access: no
>   supports-register-dump: no
>   supports-priv-flags: no
>
> With guest driver that supports STANDBY, we would always have:
>
>   [root@localhost ~]# ip a
>   1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group defau0
>       link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>       inet 127.0.0.1/8 scope host lo
>          valid_lft forever preferred_lft forever
>       inet6 ::1/128 scope host
>          valid_lft forever preferred_lft forever
>   2: enp4s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP gr0
>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>       inet 192.168.20.2/24 brd 192.168.20.255 scope global noprefixroute enp4s0
>          valid_lft forever preferred_lft forever
>       inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
>          valid_lft forever preferred_lft forever
>   3: enp4s0nsby: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel master0
>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>   4: enp2s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq master enp4s0 st0
>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>   [root@localhost ~]# ethtool -i enp4s0
>   driver: net_failover
>   version: 0.1
>   firmware-version:
>   expansion-rom-version:
>   bus-info:
>   supports-statistics: no
>   supports-test: no
>   supports-eeprom-access: no
>   supports-register-dump: no
>   supports-priv-flags: no
>   [root@localhost ~]# ethtool -i enp4s0nsby
>   driver: virtio_net
>   version: 1.0.0
>   firmware-version:
>   expansion-rom-version:
>   bus-info: 0000:04:00.0
>   supports-statistics: yes
>   supports-test: no
>   supports-eeprom-access: no
>   supports-register-dump: no
>   supports-priv-flags: no
>   [root@localhost ~]# ethtool -i enp2s0
>   driver: iavf
>   version: 4.18.0-310.el8.x86_64
>   firmware-version: N/A
>   expansion-rom-version:
>   bus-info: 0000:02:00.0
>   supports-statistics: yes
>   supports-test: no
>   supports-eeprom-access: no
>   supports-register-dump: no
>   supports-priv-flags: yes
>
> Laurent Vivier (1):
>   virtio: failover: define the default device to use in case of error
>
>  include/hw/virtio/virtio-net.h |  1 +
>  hw/net/virtio-net.c            | 49 +++++++++++++++++++++++++++++-----
>  2 files changed, 44 insertions(+), 6 deletions(-)
>
> --
> 2.31.1
>
>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] virtio: failover: define the default device to use in case of error
  2021-08-09 17:13 ` [PATCH v2 1/1] virtio: failover: define the default device to use in case of error Laurent Vivier
@ 2021-08-11  4:18   ` Jason Wang
  2021-08-11  6:29     ` Laurent Vivier
  2021-08-23 23:25   ` Michael S. Tsirkin
  1 sibling, 1 reply; 8+ messages in thread
From: Jason Wang @ 2021-08-11  4:18 UTC (permalink / raw)
  To: Laurent Vivier
  Cc: Juan Quintela, Jens Freimann, qemu-devel, Michael S. Tsirkin

On Tue, Aug 10, 2021 at 1:14 AM Laurent Vivier <lvivier@redhat.com> 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 force the guest driver to be disabled. Then, we can
> hotplug the VFIO device that will be unplugged before the migration
> like in the normal failover migration but without the failover device.
>
> This patch adds a new property to virtio-net device: "failover-default".
>
> By default, "failover-default" is set to true and thus the default NIC
> to use if the failover cannot be enabled is the virtio-net device
> (this is what is done until now with the virtio-net failover).
>
> If "failover-default" is set to false, in case of error, the virtio-net
> device is not the default anymore and the failover primary device
> is used instead.
>
> If the STANDBY feature is supported by guest and host, the virtio-net
> failover acts as usual.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  include/hw/virtio/virtio-net.h |  1 +
>  hw/net/virtio-net.c            | 49 +++++++++++++++++++++++++++++-----
>  2 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 824a69c23f06..ab77930a327e 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -208,6 +208,7 @@ struct VirtIONet {
>      /* primary failover device is hidden*/
>      bool failover_primary_hidden;
>      bool failover;
> +    bool failover_default;
>      DeviceListener primary_listener;
>      Notifier migration_state;
>      VirtioNetRssData rss_data;
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 16d20cdee52a..972c03232a96 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -935,12 +935,23 @@ 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 the virtio-net driver has the STANDBY feature, we can plug the primary
> +     * if not but is not the default failover device,
> +     * we need to plug the primary alone and the virtio-net driver will
> +     * be disabled in the validate_features() function but validate_features()
> +     * is only available with virtio 1.0 spec
> +     */
> +    if (n->failover) {
> +        if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY) ||
> +           (virtio_has_feature(features, VIRTIO_F_VERSION_1) &&

I think STANDY implies VERSION_1.

And if we do this, it means it doesn't work for legacy drivers.

Not sure if it's an issue.

Thanks

> +            !n->failover_default)) {
> +            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);
> +            }
>          }
>      }
>  }
> @@ -3625,9 +3636,34 @@ static Property virtio_net_properties[] = {
>      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_BOOL("failover-default", VirtIONet, failover_default, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> +/* validate_features() is only available with VIRTIO_F_VERSION_1 */
> +static int failover_validate_features(VirtIODevice *vdev)
> +{
> +    VirtIONet *n = VIRTIO_NET(vdev);
> +
> +    /*
> +     * 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 force the guest driver to be disabled. In this case, We can
> +     * hotplug the VFIO device that will be unplugged before the migration
> +     * like in the normal failover migration but without the failover device.
> +     */
> +    if (n->failover && !n->failover_default &&
> +        !virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
> +        /* disable virtio-net */
> +        return -ENODEV;
> +    }
> +
> +    return 0;
> +}
> +
>  static void virtio_net_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -3651,6 +3687,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
>      vdc->post_load = virtio_net_post_load_virtio;
>      vdc->vmsd = &vmstate_virtio_net_device;
>      vdc->primary_unplug_pending = primary_unplug_pending;
> +    vdc->validate_features = failover_validate_features;
>  }
>
>  static const TypeInfo virtio_net_info = {
> --
> 2.31.1
>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one
  2021-08-11  4:17 ` [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Jason Wang
@ 2021-08-11  6:27   ` Laurent Vivier
  0 siblings, 0 replies; 8+ messages in thread
From: Laurent Vivier @ 2021-08-11  6:27 UTC (permalink / raw)
  To: Jason Wang; +Cc: Juan Quintela, Jens Freimann, qemu-devel, Michael S. Tsirkin

On 11/08/2021 06:17, Jason Wang wrote:
> On Tue, Aug 10, 2021 at 1:13 AM Laurent Vivier <lvivier@redhat.com> wrote:
>>
>> v2: use validate_features() to disable the guest driver rather
>>     than setting vring.num to 0.
>>
>> With failover, when the guest virtio-net driver doesn't support the
>> STANDBY feature, the primary device is not plugged and only the virtio-net
>> device is kept. Doing like that we can migrate the machine and
>> keep the network connection.
>>
>> But in some cases, when performance is more important than availability
>> we would prefer to keep the VFIO device rather than the virtio-net one,
>> even if it means to lose the network connection during the migration of
>> the machine.
> 
> I think we still need to seek a way to recover the network after migration.

If the network device is configured to have an IP address at boot, the IP will be restored
when the card is hotplugged. The only difference with virtio-net is _during_ the
migration. Witht virtio-net we keep the networking while the VM is migrated, without it we
lose the networking when the VFIO card is unplugged and restored when it is plugged back.

Thanks,
Laurent

> 
> Thanks
> 
>>
>> To do that we can't simply unplug the virtio-net device and plug the
>> VFIO one because for the migration the initial state must be kept
>> (virtio-net plugged, VFIO unplugged) but we can try to disable the
>> virtio-net driver and plug the VFIO card, so the initial state is
>> correct (the virtio-net card is plugged, but disabled in guest, and
>> the VFIO card is unplugged before migration).
>>
>> This change doesn't impact the case when guest and host support
>> the STANDBY feature.
>>
>> I've introduced the "failover-default" property to virtio-net device
>> to set which device to keep (failover-default=true keeps the virtio-net
>> device, =off the other one).
>>
>> For example, with a guest driver that doesn't support STANDBY:
>>
>>   ...
>>   -device virtio-net-pci,id=virtio0,failover=on,failover-default=on \
>>   -device vfio-pci,host=$PCI,id=hostdev0,failover_pair_id=virtio0 \
>>   ...
>>
>>   [root@localhost ~]# ip a
>>   1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1
>>       link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>>       inet 127.0.0.1/8 scope host lo
>>          valid_lft forever preferred_lft forever
>>       inet6 ::1/128 scope host
>>          valid_lft forever preferred_lft forever
>>   2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP q0
>>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>>       inet 192.168.20.2/24 brd 192.168.20.255 scope global eth0
>>          valid_lft forever preferred_lft forever
>>       inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
>>          valid_lft forever preferred_lft forever
>>   # ethtool -i eth0
>>   driver: virtio_net
>>   version: 1.0.0
>>   firmware-version:
>>   expansion-rom-version:
>>   bus-info: 0000:04:00.0
>>   supports-statistics: no
>>   supports-test: no
>>   supports-eeprom-access: no
>>   supports-register-dump: no
>>   supports-priv-flags: no
>>
>>   ...
>>   -device virtio-net-pci,id=virtio0,failover=on,failover-default=off \
>>   -device vfio-pci,host=$PCI,id=hostdev0,failover_pair_id=virtio0 \
>>   ...
>>
>>   [root@localhost ~]# ip a
>>   1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1
>>       link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>>       inet 127.0.0.1/8 scope host lo
>>          valid_lft forever preferred_lft forever
>>       inet6 ::1/128 scope host
>>          valid_lft forever preferred_lft forever
>>   2: enp2s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP qlen 100
>>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>>       inet 192.168.20.2/24 brd 192.168.20.255 scope global enp2s0
>>          valid_lft forever preferred_lft forever
>>       inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
>>          valid_lft forever preferred_lft forever
>>   [root@localhost ~]# ethtool -i enp2s0
>>   driver: i40evf
>>   version: 1.6.27-k
>>   firmware-version: N/A
>>   expansion-rom-version:
>>   bus-info: 0000:02:00.0
>>   supports-statistics: yes
>>   supports-test: no
>>   supports-eeprom-access: no
>>   supports-register-dump: no
>>   supports-priv-flags: no
>>
>> With guest driver that supports STANDBY, we would always have:
>>
>>   [root@localhost ~]# ip a
>>   1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group defau0
>>       link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>>       inet 127.0.0.1/8 scope host lo
>>          valid_lft forever preferred_lft forever
>>       inet6 ::1/128 scope host
>>          valid_lft forever preferred_lft forever
>>   2: enp4s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP gr0
>>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>>       inet 192.168.20.2/24 brd 192.168.20.255 scope global noprefixroute enp4s0
>>          valid_lft forever preferred_lft forever
>>       inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
>>          valid_lft forever preferred_lft forever
>>   3: enp4s0nsby: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel master0
>>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>>   4: enp2s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq master enp4s0 st0
>>       link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
>>   [root@localhost ~]# ethtool -i enp4s0
>>   driver: net_failover
>>   version: 0.1
>>   firmware-version:
>>   expansion-rom-version:
>>   bus-info:
>>   supports-statistics: no
>>   supports-test: no
>>   supports-eeprom-access: no
>>   supports-register-dump: no
>>   supports-priv-flags: no
>>   [root@localhost ~]# ethtool -i enp4s0nsby
>>   driver: virtio_net
>>   version: 1.0.0
>>   firmware-version:
>>   expansion-rom-version:
>>   bus-info: 0000:04:00.0
>>   supports-statistics: yes
>>   supports-test: no
>>   supports-eeprom-access: no
>>   supports-register-dump: no
>>   supports-priv-flags: no
>>   [root@localhost ~]# ethtool -i enp2s0
>>   driver: iavf
>>   version: 4.18.0-310.el8.x86_64
>>   firmware-version: N/A
>>   expansion-rom-version:
>>   bus-info: 0000:02:00.0
>>   supports-statistics: yes
>>   supports-test: no
>>   supports-eeprom-access: no
>>   supports-register-dump: no
>>   supports-priv-flags: yes
>>
>> Laurent Vivier (1):
>>   virtio: failover: define the default device to use in case of error
>>
>>  include/hw/virtio/virtio-net.h |  1 +
>>  hw/net/virtio-net.c            | 49 +++++++++++++++++++++++++++++-----
>>  2 files changed, 44 insertions(+), 6 deletions(-)
>>
>> --
>> 2.31.1
>>
>>
> 



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] virtio: failover: define the default device to use in case of error
  2021-08-11  4:18   ` Jason Wang
@ 2021-08-11  6:29     ` Laurent Vivier
  0 siblings, 0 replies; 8+ messages in thread
From: Laurent Vivier @ 2021-08-11  6:29 UTC (permalink / raw)
  To: Jason Wang; +Cc: Juan Quintela, Jens Freimann, qemu-devel, Michael S. Tsirkin

On 11/08/2021 06:18, Jason Wang wrote:
> On Tue, Aug 10, 2021 at 1:14 AM Laurent Vivier <lvivier@redhat.com> 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 force the guest driver to be disabled. Then, we can
>> hotplug the VFIO device that will be unplugged before the migration
>> like in the normal failover migration but without the failover device.
>>
>> This patch adds a new property to virtio-net device: "failover-default".
>>
>> By default, "failover-default" is set to true and thus the default NIC
>> to use if the failover cannot be enabled is the virtio-net device
>> (this is what is done until now with the virtio-net failover).
>>
>> If "failover-default" is set to false, in case of error, the virtio-net
>> device is not the default anymore and the failover primary device
>> is used instead.
>>
>> If the STANDBY feature is supported by guest and host, the virtio-net
>> failover acts as usual.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>  include/hw/virtio/virtio-net.h |  1 +
>>  hw/net/virtio-net.c            | 49 +++++++++++++++++++++++++++++-----
>>  2 files changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
>> index 824a69c23f06..ab77930a327e 100644
>> --- a/include/hw/virtio/virtio-net.h
>> +++ b/include/hw/virtio/virtio-net.h
>> @@ -208,6 +208,7 @@ struct VirtIONet {
>>      /* primary failover device is hidden*/
>>      bool failover_primary_hidden;
>>      bool failover;
>> +    bool failover_default;
>>      DeviceListener primary_listener;
>>      Notifier migration_state;
>>      VirtioNetRssData rss_data;
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 16d20cdee52a..972c03232a96 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -935,12 +935,23 @@ 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 the virtio-net driver has the STANDBY feature, we can plug the primary
>> +     * if not but is not the default failover device,
>> +     * we need to plug the primary alone and the virtio-net driver will
>> +     * be disabled in the validate_features() function but validate_features()
>> +     * is only available with virtio 1.0 spec
>> +     */
>> +    if (n->failover) {
>> +        if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY) ||
>> +           (virtio_has_feature(features, VIRTIO_F_VERSION_1) &&
> 
> I think STANDY implies VERSION_1.
> 
> And if we do this, it means it doesn't work for legacy drivers.
> 
> Not sure if it's an issue.

Yes, you're right. In case of a kernel driver that doesn't support STANDBY and that is not
version 1, the virtio-net device is disabled (and the VFIO device is not plugged).

Thanks,
Laurent



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] virtio: failover: define the default device to use in case of error
  2021-08-09 17:13 ` [PATCH v2 1/1] virtio: failover: define the default device to use in case of error Laurent Vivier
  2021-08-11  4:18   ` Jason Wang
@ 2021-08-23 23:25   ` Michael S. Tsirkin
  2021-08-26 15:56     ` Laurent Vivier
  1 sibling, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2021-08-23 23:25 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Jason Wang, Jens Freimann, qemu-devel, Juan Quintela

On Mon, Aug 09, 2021 at 07:13:42PM +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 force the guest driver to be disabled. Then, we can
> hotplug the VFIO device that will be unplugged before the migration
> like in the normal failover migration but without the failover device.
> 
> This patch adds a new property to virtio-net device: "failover-default".
> 
> By default, "failover-default" is set to true and thus the default NIC
> to use if the failover cannot be enabled is the virtio-net device
> (this is what is done until now with the virtio-net failover).
> 
> If "failover-default" is set to false, in case of error, the virtio-net
> device is not the default anymore and the failover primary device
> is used instead.
> 
> If the STANDBY feature is supported by guest and host, the virtio-net
> failover acts as usual.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Three things I dislike here. First this is limited to 1.0.
OTOH this is all about legacy guests without STANDBY,
would be nicer to support legacy.
Second: the reason we don't want both
virtio and VFIO is because their mac addresses match.
This tends to confuse guest tools.
I don't see this solved here.

Proposal: management supplies an extra dummy mac.
This mac is used with virtio and its link it down.
Link state reporting is also optional but
it has been there for many years.
If link state reporting is disabled then maybe do not
expose VFIO after all.

Third thing is option name. Does not hint at the fact that
for legacy guests we do not get failover at all.
Let's try to be more explicit please.



> ---
>  include/hw/virtio/virtio-net.h |  1 +
>  hw/net/virtio-net.c            | 49 +++++++++++++++++++++++++++++-----
>  2 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 824a69c23f06..ab77930a327e 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -208,6 +208,7 @@ struct VirtIONet {
>      /* primary failover device is hidden*/
>      bool failover_primary_hidden;
>      bool failover;
> +    bool failover_default;
>      DeviceListener primary_listener;
>      Notifier migration_state;
>      VirtioNetRssData rss_data;
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 16d20cdee52a..972c03232a96 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -935,12 +935,23 @@ 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 the virtio-net driver has the STANDBY feature, we can plug the primary
> +     * if not but is not the default failover device,
> +     * we need to plug the primary alone and the virtio-net driver will
> +     * be disabled in the validate_features() function but validate_features()
> +     * is only available with virtio 1.0 spec
> +     */
> +    if (n->failover) {
> +        if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY) ||
> +           (virtio_has_feature(features, VIRTIO_F_VERSION_1) &&
> +            !n->failover_default)) {
> +            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);
> +            }
>          }
>      }
>  }
> @@ -3625,9 +3636,34 @@ static Property virtio_net_properties[] = {
>      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_BOOL("failover-default", VirtIONet, failover_default, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +/* validate_features() is only available with VIRTIO_F_VERSION_1 */
> +static int failover_validate_features(VirtIODevice *vdev)
> +{
> +    VirtIONet *n = VIRTIO_NET(vdev);
> +
> +    /*
> +     * 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 force the guest driver to be disabled. In this case, We can
> +     * hotplug the VFIO device that will be unplugged before the migration
> +     * like in the normal failover migration but without the failover device.
> +     */
> +    if (n->failover && !n->failover_default &&
> +        !virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
> +        /* disable virtio-net */
> +        return -ENODEV;
> +    }
> +
> +    return 0;
> +}
> +
>  static void virtio_net_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -3651,6 +3687,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
>      vdc->post_load = virtio_net_post_load_virtio;
>      vdc->vmsd = &vmstate_virtio_net_device;
>      vdc->primary_unplug_pending = primary_unplug_pending;
> +    vdc->validate_features = failover_validate_features;
>  }
>  
>  static const TypeInfo virtio_net_info = {
> -- 
> 2.31.1



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/1] virtio: failover: define the default device to use in case of error
  2021-08-23 23:25   ` Michael S. Tsirkin
@ 2021-08-26 15:56     ` Laurent Vivier
  0 siblings, 0 replies; 8+ messages in thread
From: Laurent Vivier @ 2021-08-26 15:56 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Jason Wang, Jens Freimann, qemu-devel, Juan Quintela

On 24/08/2021 01:25, Michael S. Tsirkin wrote:
> On Mon, Aug 09, 2021 at 07:13:42PM +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 force the guest driver to be disabled. Then, we can
>> hotplug the VFIO device that will be unplugged before the migration
>> like in the normal failover migration but without the failover device.
>>
>> This patch adds a new property to virtio-net device: "failover-default".
>>
>> By default, "failover-default" is set to true and thus the default NIC
>> to use if the failover cannot be enabled is the virtio-net device
>> (this is what is done until now with the virtio-net failover).
>>
>> If "failover-default" is set to false, in case of error, the virtio-net
>> device is not the default anymore and the failover primary device
>> is used instead.
>>
>> If the STANDBY feature is supported by guest and host, the virtio-net
>> failover acts as usual.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Three things I dislike here. First this is limited to 1.0.
> OTOH this is all about legacy guests without STANDBY,
> would be nicer to support legacy.

So for that we could use my first approach to disable the driver by triggering an error in
the driver probe function. It was done in v1 by setting vring.num to 0.

> Second: the reason we don't want both
> virtio and VFIO is because their mac addresses match.
> This tends to confuse guest tools.
> I don't see this solved here.

If the driver is not loaded I don't think any guest tool can read the MAC address of the
virtio-net device.

In fact what we want is to use VFIO device when failover is not available. I think keeping
both can confuse the guest tools.

> 
> Proposal: management supplies an extra dummy mac.
> This mac is used with virtio and its link it down.
> Link state reporting is also optional but
> it has been there for many years.
> If link state reporting is disabled then maybe do not
> expose VFIO after all.

The problem by keeping both interfaces (vfio and virtio-net) outside the failover
configuration is the networking configuration (ifcfg) will be applied to the virtio-net
device as it's PCI address is used to identify the failover_net interface. So the guest
kernel will continue to configure and use the virtio-net device even if the vfio device is
not hidden. Of course, we also need to define an ifcfg for the vfio device if failover is
disabled.

> Third thing is option name. Does not hint at the fact that
> for legacy guests we do not get failover at all.
> Let's try to be more explicit please.

ok, but I think this is a minor problem we can discuss once the two previous ones are solved.

I can also try to implement a POC with your proposition: a different MAC address and the
link down state.

Thanks,
Laurent

> 
> 
>> ---
>>  include/hw/virtio/virtio-net.h |  1 +
>>  hw/net/virtio-net.c            | 49 +++++++++++++++++++++++++++++-----
>>  2 files changed, 44 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
>> index 824a69c23f06..ab77930a327e 100644
>> --- a/include/hw/virtio/virtio-net.h
>> +++ b/include/hw/virtio/virtio-net.h
>> @@ -208,6 +208,7 @@ struct VirtIONet {
>>      /* primary failover device is hidden*/
>>      bool failover_primary_hidden;
>>      bool failover;
>> +    bool failover_default;
>>      DeviceListener primary_listener;
>>      Notifier migration_state;
>>      VirtioNetRssData rss_data;
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 16d20cdee52a..972c03232a96 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -935,12 +935,23 @@ 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 the virtio-net driver has the STANDBY feature, we can plug the primary
>> +     * if not but is not the default failover device,
>> +     * we need to plug the primary alone and the virtio-net driver will
>> +     * be disabled in the validate_features() function but validate_features()
>> +     * is only available with virtio 1.0 spec
>> +     */
>> +    if (n->failover) {
>> +        if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY) ||
>> +           (virtio_has_feature(features, VIRTIO_F_VERSION_1) &&
>> +            !n->failover_default)) {
>> +            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);
>> +            }
>>          }
>>      }
>>  }
>> @@ -3625,9 +3636,34 @@ static Property virtio_net_properties[] = {
>>      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_BOOL("failover-default", VirtIONet, failover_default, true),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> +/* validate_features() is only available with VIRTIO_F_VERSION_1 */
>> +static int failover_validate_features(VirtIODevice *vdev)
>> +{
>> +    VirtIONet *n = VIRTIO_NET(vdev);
>> +
>> +    /*
>> +     * 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 force the guest driver to be disabled. In this case, We can
>> +     * hotplug the VFIO device that will be unplugged before the migration
>> +     * like in the normal failover migration but without the failover device.
>> +     */
>> +    if (n->failover && !n->failover_default &&
>> +        !virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
>> +        /* disable virtio-net */
>> +        return -ENODEV;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static void virtio_net_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -3651,6 +3687,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
>>      vdc->post_load = virtio_net_post_load_virtio;
>>      vdc->vmsd = &vmstate_virtio_net_device;
>>      vdc->primary_unplug_pending = primary_unplug_pending;
>> +    vdc->validate_features = failover_validate_features;
>>  }
>>  
>>  static const TypeInfo virtio_net_info = {
>> -- 
>> 2.31.1
> 



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-08-26 15:57 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09 17:13 [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Laurent Vivier
2021-08-09 17:13 ` [PATCH v2 1/1] virtio: failover: define the default device to use in case of error Laurent Vivier
2021-08-11  4:18   ` Jason Wang
2021-08-11  6:29     ` Laurent Vivier
2021-08-23 23:25   ` Michael S. Tsirkin
2021-08-26 15:56     ` Laurent Vivier
2021-08-11  4:17 ` [PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Jason Wang
2021-08-11  6:27   ` Laurent Vivier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.