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

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

A way to disable the virtio-net driver at startup is to trigger an
error in the kernel probing function. We can do that by disabling
the RX queues. I tried to add a function to disable the queue that
does that by setting the queue vring "num" value to 0 (and re-enable the
queue by setting back to the default value).

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 (2):
  virtio: add a way to disable a queue
  virtio: failover: define the default device to use in case of error

 include/hw/virtio/virtio-net.h |  1 +
 include/hw/virtio/virtio.h     |  2 ++
 hw/net/virtio-net.c            | 34 ++++++++++++++++++++++++++++++++++
 hw/virtio/virtio.c             | 10 ++++++++++
 4 files changed, 47 insertions(+)

-- 
2.31.1




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

* [PATCH 1/2] virtio: add a way to disable a queue
  2021-07-29 19:19 [PATCH 0/2] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Laurent Vivier
@ 2021-07-29 19:19 ` Laurent Vivier
  2021-08-02  4:50   ` Jason Wang
  2021-07-29 19:19 ` [PATCH 2/2] virtio: failover: define the default device to use in case of error Laurent Vivier
  1 sibling, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2021-07-29 19:19 UTC (permalink / raw)
  To: qemu-devel; +Cc: Juan Quintela, Jason Wang, Jens Freimann, Michael S. Tsirkin

Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue
by setting vring.num to 0 (or num_default).
This is needed to be able to disable a guest driver from the host side

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 include/hw/virtio/virtio.h |  2 ++
 hw/virtio/virtio.c         | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 8bab9cfb7507..6a3f71b4cd88 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -251,6 +251,8 @@ void virtio_config_modern_writel(VirtIODevice *vdev,
                                  uint32_t addr, uint32_t data);
 void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
 hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
+void virtio_queue_enable(VirtIODevice *vdev, int n);
+void virtio_queue_disable(VirtIODevice *vdev, int n);
 void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
 int virtio_queue_get_num(VirtIODevice *vdev, int n);
 int virtio_queue_get_max_num(VirtIODevice *vdev, int n);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 874377f37a70..fa5228c1a2d6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2244,6 +2244,16 @@ void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
     virtio_init_region_cache(vdev, n);
 }
 
+void virtio_queue_disable(VirtIODevice *vdev, int n)
+{
+    vdev->vq[n].vring.num = 0;
+}
+
+void virtio_queue_enable(VirtIODevice *vdev, int n)
+{
+    vdev->vq[n].vring.num = vdev->vq[n].vring.num_default;
+}
+
 void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
 {
     /* Don't allow guest to flip queue between existent and
-- 
2.31.1



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

* [PATCH 2/2] virtio: failover: define the default device to use in case of error
  2021-07-29 19:19 [PATCH 0/2] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Laurent Vivier
  2021-07-29 19:19 ` [PATCH 1/2] virtio: add a way to disable a queue Laurent Vivier
@ 2021-07-29 19:19 ` Laurent Vivier
  1 sibling, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2021-07-29 19:19 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            | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

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..6fe0a09a263b 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -891,6 +891,39 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
     Error *err = NULL;
     int i;
 
+    /*
+     * 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.
+     */
+    if (n->failover && !n->failover_default) {
+        if (!virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
+            /* disable the first queue to disable the driver */
+            virtio_queue_disable(vdev, 0);
+            /*
+             * as the virtio-net driver is disable we can plug back the
+             * failover primary device
+             */
+            qatomic_set(&n->failover_primary_hidden, false);
+            failover_add_primary(n, &err);
+            if (err) {
+                warn_report_err(err);
+            }
+            return;
+        } else {
+          /*
+           * if the driver renegotiates features, we need to re-enable
+           * the queue
+           */
+          virtio_queue_enable(vdev, 0);
+        }
+    }
+
     if (n->mtu_bypass_backend &&
             !virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_MTU)) {
         features &= ~(1ULL << VIRTIO_NET_F_MTU);
@@ -3625,6 +3658,7 @@ 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(),
 };
 
-- 
2.31.1



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

* Re: [PATCH 1/2] virtio: add a way to disable a queue
  2021-07-29 19:19 ` [PATCH 1/2] virtio: add a way to disable a queue Laurent Vivier
@ 2021-08-02  4:50   ` Jason Wang
  2021-08-02  8:42     ` Laurent Vivier
  0 siblings, 1 reply; 9+ messages in thread
From: Jason Wang @ 2021-08-02  4:50 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel
  Cc: Juan Quintela, Jens Freimann, Michael S. Tsirkin


在 2021/7/30 上午3:19, Laurent Vivier 写道:
> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue
> by setting vring.num to 0 (or num_default).
> This is needed to be able to disable a guest driver from the host side


I suspect this won't work correclty for vhost.

And I believe we should only do this after the per queue 
enabling/disabling is supported by the spec.

(only MMIO support that AFAIK)

Thanks


>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>   include/hw/virtio/virtio.h |  2 ++
>   hw/virtio/virtio.c         | 10 ++++++++++
>   2 files changed, 12 insertions(+)
>
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 8bab9cfb7507..6a3f71b4cd88 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -251,6 +251,8 @@ void virtio_config_modern_writel(VirtIODevice *vdev,
>                                    uint32_t addr, uint32_t data);
>   void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
>   hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
> +void virtio_queue_enable(VirtIODevice *vdev, int n);
> +void virtio_queue_disable(VirtIODevice *vdev, int n);
>   void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
>   int virtio_queue_get_num(VirtIODevice *vdev, int n);
>   int virtio_queue_get_max_num(VirtIODevice *vdev, int n);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 874377f37a70..fa5228c1a2d6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2244,6 +2244,16 @@ void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
>       virtio_init_region_cache(vdev, n);
>   }
>   
> +void virtio_queue_disable(VirtIODevice *vdev, int n)
> +{
> +    vdev->vq[n].vring.num = 0;
> +}
> +
> +void virtio_queue_enable(VirtIODevice *vdev, int n)
> +{
> +    vdev->vq[n].vring.num = vdev->vq[n].vring.num_default;
> +}
> +
>   void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
>   {
>       /* Don't allow guest to flip queue between existent and



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

* Re: [PATCH 1/2] virtio: add a way to disable a queue
  2021-08-02  4:50   ` Jason Wang
@ 2021-08-02  8:42     ` Laurent Vivier
  2021-08-06  6:25       ` Jason Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2021-08-02  8:42 UTC (permalink / raw)
  To: Jason Wang, qemu-devel; +Cc: Juan Quintela, Jens Freimann, Michael S. Tsirkin

On 02/08/2021 06:50, Jason Wang wrote:
> 
> 在 2021/7/30 上午3:19, Laurent Vivier 写道:
>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue
>> by setting vring.num to 0 (or num_default).
>> This is needed to be able to disable a guest driver from the host side
> 
> 
> I suspect this won't work correclty for vhost.

With my test it seems to work with vhost too.

> 
> And I believe we should only do this after the per queue enabling/disabling is supported
> by the spec.
> 
> (only MMIO support that AFAIK)

I don't want to modify the spec.

I need something that works without modifying existing (old) drivers.

The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is
too old (i.e. it doesn't support STANDBY feature).

Setting vring.num to 0 forces the kernel driver to exit on error in the probe function.
It's what I want: the device is present but disabled (the driver is not loaded).

Any other suggestion?

Thanks,
Laurent

> Thanks
> 
> 
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   include/hw/virtio/virtio.h |  2 ++
>>   hw/virtio/virtio.c         | 10 ++++++++++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 8bab9cfb7507..6a3f71b4cd88 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -251,6 +251,8 @@ void virtio_config_modern_writel(VirtIODevice *vdev,
>>                                    uint32_t addr, uint32_t data);
>>   void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
>>   hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
>> +void virtio_queue_enable(VirtIODevice *vdev, int n);
>> +void virtio_queue_disable(VirtIODevice *vdev, int n);
>>   void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
>>   int virtio_queue_get_num(VirtIODevice *vdev, int n);
>>   int virtio_queue_get_max_num(VirtIODevice *vdev, int n);
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 874377f37a70..fa5228c1a2d6 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2244,6 +2244,16 @@ void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
>>       virtio_init_region_cache(vdev, n);
>>   }
>>   +void virtio_queue_disable(VirtIODevice *vdev, int n)
>> +{
>> +    vdev->vq[n].vring.num = 0;
>> +}
>> +
>> +void virtio_queue_enable(VirtIODevice *vdev, int n)
>> +{
>> +    vdev->vq[n].vring.num = vdev->vq[n].vring.num_default;
>> +}
>> +
>>   void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
>>   {
>>       /* Don't allow guest to flip queue between existent and
> 



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

* Re: [PATCH 1/2] virtio: add a way to disable a queue
  2021-08-02  8:42     ` Laurent Vivier
@ 2021-08-06  6:25       ` Jason Wang
  2021-08-06  7:27         ` Laurent Vivier
  0 siblings, 1 reply; 9+ messages in thread
From: Jason Wang @ 2021-08-06  6:25 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel
  Cc: Juan Quintela, Jens Freimann, Michael S. Tsirkin


在 2021/8/2 下午4:42, Laurent Vivier 写道:
> On 02/08/2021 06:50, Jason Wang wrote:
>> 在 2021/7/30 上午3:19, Laurent Vivier 写道:
>>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue
>>> by setting vring.num to 0 (or num_default).
>>> This is needed to be able to disable a guest driver from the host side
>>
>> I suspect this won't work correclty for vhost.
> With my test it seems to work with vhost too.


So setting 0 will lead -EINVAL to be returned during 
VHOST_SET_VRING_NUM. I think qemu will warn the failure in this case.

What's more important, it's not guaranteed to work for the case of 
vhost-user or vhost-vDPA.


>
>> And I believe we should only do this after the per queue enabling/disabling is supported
>> by the spec.
>>
>> (only MMIO support that AFAIK)
> I don't want to modify the spec.
>
> I need something that works without modifying existing (old) drivers.
>
> The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is
> too old (i.e. it doesn't support STANDBY feature).
>
> Setting vring.num to 0 forces the kernel driver to exit on error in the probe function.
> It's what I want: the device is present but disabled (the driver is not loaded).
>
> Any other suggestion?


I think we should probably disable the device instead of doing it per 
virtqueue.

Thanks


>
> Thanks,
> Laurent
>
>> Thanks
>>
>>
>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>> ---
>>>    include/hw/virtio/virtio.h |  2 ++
>>>    hw/virtio/virtio.c         | 10 ++++++++++
>>>    2 files changed, 12 insertions(+)
>>>
>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>> index 8bab9cfb7507..6a3f71b4cd88 100644
>>> --- a/include/hw/virtio/virtio.h
>>> +++ b/include/hw/virtio/virtio.h
>>> @@ -251,6 +251,8 @@ void virtio_config_modern_writel(VirtIODevice *vdev,
>>>                                     uint32_t addr, uint32_t data);
>>>    void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
>>>    hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
>>> +void virtio_queue_enable(VirtIODevice *vdev, int n);
>>> +void virtio_queue_disable(VirtIODevice *vdev, int n);
>>>    void virtio_queue_set_num(VirtIODevice *vdev, int n, int num);
>>>    int virtio_queue_get_num(VirtIODevice *vdev, int n);
>>>    int virtio_queue_get_max_num(VirtIODevice *vdev, int n);
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 874377f37a70..fa5228c1a2d6 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -2244,6 +2244,16 @@ void virtio_queue_set_rings(VirtIODevice *vdev, int n, hwaddr desc,
>>>        virtio_init_region_cache(vdev, n);
>>>    }
>>>    +void virtio_queue_disable(VirtIODevice *vdev, int n)
>>> +{
>>> +    vdev->vq[n].vring.num = 0;
>>> +}
>>> +
>>> +void virtio_queue_enable(VirtIODevice *vdev, int n)
>>> +{
>>> +    vdev->vq[n].vring.num = vdev->vq[n].vring.num_default;
>>> +}
>>> +
>>>    void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
>>>    {
>>>        /* Don't allow guest to flip queue between existent and



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

* Re: [PATCH 1/2] virtio: add a way to disable a queue
  2021-08-06  6:25       ` Jason Wang
@ 2021-08-06  7:27         ` Laurent Vivier
  2021-08-09  3:01           ` Jason Wang
  0 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2021-08-06  7:27 UTC (permalink / raw)
  To: Jason Wang, qemu-devel; +Cc: Juan Quintela, Jens Freimann, Michael S. Tsirkin

On 06/08/2021 08:25, Jason Wang wrote:
> 
> 在 2021/8/2 下午4:42, Laurent Vivier 写道:
>> On 02/08/2021 06:50, Jason Wang wrote:
>>> 在 2021/7/30 上午3:19, Laurent Vivier 写道:
>>>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue
>>>> by setting vring.num to 0 (or num_default).
>>>> This is needed to be able to disable a guest driver from the host side
>>>
>>> I suspect this won't work correclty for vhost.
>> With my test it seems to work with vhost too.
> 
> 
> So setting 0 will lead -EINVAL to be returned during VHOST_SET_VRING_NUM. I think qemu
> will warn the failure in this case.

I didn't see any error when I tried. I will check the code.

> What's more important, it's not guaranteed to work for the case of vhost-user or vhost-vDPA.

Perhaps we can target only the vhost host case, as this is used for failover and usually
the virtio-net device is backed by a bridge on same network as the VFIO device?

> 
> 
>>
>>> And I believe we should only do this after the per queue enabling/disabling is supported
>>> by the spec.
>>>
>>> (only MMIO support that AFAIK)
>> I don't want to modify the spec.
>>
>> I need something that works without modifying existing (old) drivers.
>>
>> The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is
>> too old (i.e. it doesn't support STANDBY feature).
>>
>> Setting vring.num to 0 forces the kernel driver to exit on error in the probe function.
>> It's what I want: the device is present but disabled (the driver is not loaded).
>>
>> Any other suggestion?
> 
> 
> I think we should probably disable the device instead of doing it per virtqueue.
> 

I tried to use virtio_set_disabled() but it doesn't work.
Perhaps it's too late when I call the function (I need to do that in
virtio_net_set_features()). What I want is to prevent the load of the driver in the guest
kernel to hide the virtio-net device. Setting vring.num to 0 triggers an error in the
driver probe function and prevents the load of the driver.

Thanks,
Laurent



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

* Re: [PATCH 1/2] virtio: add a way to disable a queue
  2021-08-06  7:27         ` Laurent Vivier
@ 2021-08-09  3:01           ` Jason Wang
  2021-08-09 16:12             ` Laurent Vivier
  0 siblings, 1 reply; 9+ messages in thread
From: Jason Wang @ 2021-08-09  3:01 UTC (permalink / raw)
  To: Laurent Vivier, qemu-devel
  Cc: Juan Quintela, Jens Freimann, Michael S. Tsirkin


在 2021/8/6 下午3:27, Laurent Vivier 写道:
> On 06/08/2021 08:25, Jason Wang wrote:
>> 在 2021/8/2 下午4:42, Laurent Vivier 写道:
>>> On 02/08/2021 06:50, Jason Wang wrote:
>>>> 在 2021/7/30 上午3:19, Laurent Vivier 写道:
>>>>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue
>>>>> by setting vring.num to 0 (or num_default).
>>>>> This is needed to be able to disable a guest driver from the host side
>>>> I suspect this won't work correclty for vhost.
>>> With my test it seems to work with vhost too.
>>
>> So setting 0 will lead -EINVAL to be returned during VHOST_SET_VRING_NUM. I think qemu
>> will warn the failure in this case.
> I didn't see any error when I tried. I will check the code.
>
>> What's more important, it's not guaranteed to work for the case of vhost-user or vhost-vDPA.
> Perhaps we can target only the vhost host case, as this is used for failover and usually
> the virtio-net device is backed by a bridge on same network as the VFIO device?


Probably not, it should be a general feature that can work for all types 
of virtio/vhost backends.


>
>>
>>>> And I believe we should only do this after the per queue enabling/disabling is supported
>>>> by the spec.
>>>>
>>>> (only MMIO support that AFAIK)
>>> I don't want to modify the spec.
>>>
>>> I need something that works without modifying existing (old) drivers.
>>>
>>> The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is
>>> too old (i.e. it doesn't support STANDBY feature).
>>>
>>> Setting vring.num to 0 forces the kernel driver to exit on error in the probe function.
>>> It's what I want: the device is present but disabled (the driver is not loaded).
>>>
>>> Any other suggestion?
>>
>> I think we should probably disable the device instead of doing it per virtqueue.
>>
> I tried to use virtio_set_disabled() but it doesn't work.
> Perhaps it's too late when I call the function (I need to do that in
> virtio_net_set_features()). What I want is to prevent the load of the driver in the guest
> kernel to hide the virtio-net device. Setting vring.num to 0 triggers an error in the
> driver probe function and prevents the load of the driver.


How about fail the validate_features() in this case?

Thanks


>
> Thanks,
> Laurent
>



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

* Re: [PATCH 1/2] virtio: add a way to disable a queue
  2021-08-09  3:01           ` Jason Wang
@ 2021-08-09 16:12             ` Laurent Vivier
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2021-08-09 16:12 UTC (permalink / raw)
  To: Jason Wang, qemu-devel; +Cc: Juan Quintela, Jens Freimann, Michael S. Tsirkin

On 09/08/2021 05:01, Jason Wang wrote:
> 
> 在 2021/8/6 下午3:27, Laurent Vivier 写道:
>> On 06/08/2021 08:25, Jason Wang wrote:
>>> 在 2021/8/2 下午4:42, Laurent Vivier 写道:
>>>> On 02/08/2021 06:50, Jason Wang wrote:
>>>>> 在 2021/7/30 上午3:19, Laurent Vivier 写道:
>>>>>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a queue
>>>>>> by setting vring.num to 0 (or num_default).
>>>>>> This is needed to be able to disable a guest driver from the host side
>>>>> I suspect this won't work correclty for vhost.
>>>> With my test it seems to work with vhost too.
>>>
>>> So setting 0 will lead -EINVAL to be returned during VHOST_SET_VRING_NUM. I think qemu
>>> will warn the failure in this case.
>> I didn't see any error when I tried. I will check the code.
>>
>>> What's more important, it's not guaranteed to work for the case of vhost-user or
>>> vhost-vDPA.
>> Perhaps we can target only the vhost host case, as this is used for failover and usually
>> the virtio-net device is backed by a bridge on same network as the VFIO device?
> 
> 
> Probably not, it should be a general feature that can work for all types of virtio/vhost
> backends.
> 
> 
>>
>>>
>>>>> And I believe we should only do this after the per queue enabling/disabling is supported
>>>>> by the spec.
>>>>>
>>>>> (only MMIO support that AFAIK)
>>>> I don't want to modify the spec.
>>>>
>>>> I need something that works without modifying existing (old) drivers.
>>>>
>>>> The idea is to be able to disable the virtio-net kernel driver from QEMU if the driver is
>>>> too old (i.e. it doesn't support STANDBY feature).
>>>>
>>>> Setting vring.num to 0 forces the kernel driver to exit on error in the probe function.
>>>> It's what I want: the device is present but disabled (the driver is not loaded).
>>>>
>>>> Any other suggestion?
>>>
>>> I think we should probably disable the device instead of doing it per virtqueue.
>>>
>> I tried to use virtio_set_disabled() but it doesn't work.
>> Perhaps it's too late when I call the function (I need to do that in
>> virtio_net_set_features()). What I want is to prevent the load of the driver in the guest
>> kernel to hide the virtio-net device. Setting vring.num to 0 triggers an error in the
>> driver probe function and prevents the load of the driver.
> 
> 
> How about fail the validate_features() in this case?

It's a good suggestion and it seems to work.

I'm going to send an updated patch.

Thanks,
Laurent



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

end of thread, other threads:[~2021-08-09 16:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-29 19:19 [PATCH 0/2] virtio: failover: allow to keep the VFIO device rather than the virtio-net one Laurent Vivier
2021-07-29 19:19 ` [PATCH 1/2] virtio: add a way to disable a queue Laurent Vivier
2021-08-02  4:50   ` Jason Wang
2021-08-02  8:42     ` Laurent Vivier
2021-08-06  6:25       ` Jason Wang
2021-08-06  7:27         ` Laurent Vivier
2021-08-09  3:01           ` Jason Wang
2021-08-09 16:12             ` Laurent Vivier
2021-07-29 19:19 ` [PATCH 2/2] virtio: failover: define the default device to use in case of error 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.