[PATCH 00/16] Support VIRTIO_F_RING_RESET for virtio-net and vhost-user in virtio pci

[PATCH 00/16] Support VIRTIO_F_RING_RESET for virtio-net and vhost-user in virtio pci

[Kangjie Xu]

The virtio queue reset function has already been defined in the virtio spec 1.2.
The relevant virtio spec information is here:

    https://github.com/oasis-tcs/virtio-spec/issues/124
    https://github.com/oasis-tcs/virtio-spec/issues/139

This patch set is to support this function in QEMU. It consists of two parts: virtio-net
and vhost-user. The patches 1-7 are the implementation for virtio-net and the patches
8-16 are for vhost-user. 

The process of virt queue reset can be concluded as:
1. The virtqueue is disabled when VIRTIO_PCI_COMMON_Q_RESET is written.
2. Then the virtqueue is restarted after the information of vrings is passed to QEMU and
VIRTIO_PCI_COMMON_Q_ENABLE is written.

Test environment:
    Host: 5.4.189
    Qemu: QEMU emulator version 7.0.50
    Guest: 5.19.0-rc3 (With vq reset support)
    DPDK: 22.07-rc1 (With vq reset support)
    Test Cmd: ethtool -g eth1; ethtool -G eth1 rx $1 tx $2; ethtool -g eth1;

    The frontend can resize the virtio queue, then virtio queue reset function should
    be triggered.

    The default is split mode, modify Qemu virtio-net to add PACKED feature to 
    test packed mode.

Guest Kernel Patch:
    https://lore.kernel.org/bpf/20220629065656.54420-1-xuanzhuo@linux.alibaba.com/

DPDK Code:
    https://github.com/middaywords/dpdk/commit/098c8e1dfae10b747da8dd8950a82890efca7bee

Please review. Thanks.

Kangjie Xu (9):
  vhost: add op to enable or disable a single vring
  vhost-user: enable/disable a single vring
  vhost: extract the logic of unmapping the vrings and desc
  vhost: introduce restart and release for vhost_dev's vqs
  vhost-net: introduce restart and stop for vhost_net's vqs
  virtio: introduce queue_enable in virtio
  virtio-net: support queue_enable for vhost-user
  virtio-net: support queue_reset for vhost-user
  vhost-net: vq reset feature bit support

Xuan Zhuo (7):
  virtio-pci: virtio_pci_common_cfg add queue_notify_data
  virtio: add VIRTIO_F_RING_RESET
  virtio: pci: virtio_pci_common_cfg add queue_reset
  virtio: introduce __virtio_queue_reset()
  virtio: introduce virtio_queue_reset()
  virtio-pci: support queue reset
  virtio-net: support queue reset

 hw/net/vhost_net.c                            | 56 ++++++++++++++++++
 hw/net/virtio-net.c                           | 40 +++++++++++++
 hw/virtio/vhost-user.c                        | 55 +++++++++++++++---
 hw/virtio/vhost.c                             | 49 ++++++++++++++--
 hw/virtio/virtio-pci.c                        | 17 ++++++
 hw/virtio/virtio.c                            | 57 +++++++++++++------
 include/hw/virtio/vhost-backend.h             |  4 ++
 include/hw/virtio/vhost.h                     |  6 ++
 include/hw/virtio/virtio-pci.h                |  1 +
 include/hw/virtio/virtio.h                    |  4 ++
 include/net/vhost_net.h                       |  5 ++
 .../standard-headers/linux/virtio_config.h    |  5 ++
 include/standard-headers/linux/virtio_pci.h   |  4 ++
 13 files changed, 274 insertions(+), 29 deletions(-)

-- 
2.32.0

[PATCH 01/16] virtio-pci: virtio_pci_common_cfg add queue_notify_data

[Kangjie Xu]

From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Add queue_notify_data in struct virtio_pci_common_cfg, which comes from
here https://github.com/oasis-tcs/virtio-spec/issues/89

Since I want to add queue_reset after queue_notify_data, I submitted
this patch first.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/standard-headers/linux/virtio_pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/standard-headers/linux/virtio_pci.h b/include/standard-headers/linux/virtio_pci.h
index db7a8e2fcb..598ebe9825 100644
--- a/include/standard-headers/linux/virtio_pci.h
+++ b/include/standard-headers/linux/virtio_pci.h
@@ -164,6 +164,7 @@ struct virtio_pci_common_cfg {
 	uint32_t queue_avail_hi;		/* read-write */
 	uint32_t queue_used_lo;		/* read-write */
 	uint32_t queue_used_hi;		/* read-write */
+	uint16_t queue_notify_data;	/* read-write */
 };
 
 /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
@@ -202,6 +203,7 @@ struct virtio_pci_cfg_cap {
 #define VIRTIO_PCI_COMMON_Q_AVAILHI	44
 #define VIRTIO_PCI_COMMON_Q_USEDLO	48
 #define VIRTIO_PCI_COMMON_Q_USEDHI	52
+#define VIRTIO_PCI_COMMON_Q_NOTIFY_DATA	56
 
 #endif /* VIRTIO_PCI_NO_MODERN */
 
-- 
2.32.0

Re: [PATCH 01/16] virtio-pci: virtio_pci_common_cfg add queue_notify_data

[Jason Wang]

在 2022/7/18 19:16, Kangjie Xu 写道:
> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> Add queue_notify_data in struct virtio_pci_common_cfg, which comes from
> here https://github.com/oasis-tcs/virtio-spec/issues/89
>
> Since I want to add queue_reset after queue_notify_data, I submitted
> this patch first.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


This should be done by script/update-linux-headers.sh.

Thanks


> ---
>   include/standard-headers/linux/virtio_pci.h | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/include/standard-headers/linux/virtio_pci.h b/include/standard-headers/linux/virtio_pci.h
> index db7a8e2fcb..598ebe9825 100644
> --- a/include/standard-headers/linux/virtio_pci.h
> +++ b/include/standard-headers/linux/virtio_pci.h
> @@ -164,6 +164,7 @@ struct virtio_pci_common_cfg {
>   	uint32_t queue_avail_hi;		/* read-write */
>   	uint32_t queue_used_lo;		/* read-write */
>   	uint32_t queue_used_hi;		/* read-write */
> +	uint16_t queue_notify_data;	/* read-write */
>   };
>   
>   /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
> @@ -202,6 +203,7 @@ struct virtio_pci_cfg_cap {
>   #define VIRTIO_PCI_COMMON_Q_AVAILHI	44
>   #define VIRTIO_PCI_COMMON_Q_USEDLO	48
>   #define VIRTIO_PCI_COMMON_Q_USEDHI	52
> +#define VIRTIO_PCI_COMMON_Q_NOTIFY_DATA	56
>   
>   #endif /* VIRTIO_PCI_NO_MODERN */
>   

Re: [PATCH 01/16] virtio-pci: virtio_pci_common_cfg add queue_notify_data

[Kangjie Xu]

在 2022/7/26 11:17, Jason Wang 写道:
>
> 在 2022/7/18 19:16, Kangjie Xu 写道:
>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>
>> Add queue_notify_data in struct virtio_pci_common_cfg, which comes from
>> here https://github.com/oasis-tcs/virtio-spec/issues/89
>>
>> Since I want to add queue_reset after queue_notify_data, I submitted
>> this patch first.
>>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
>
> This should be done by script/update-linux-headers.sh.
>
> Thanks
>
Will fix.

Thanks

>
>> ---
>>   include/standard-headers/linux/virtio_pci.h | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/include/standard-headers/linux/virtio_pci.h 
>> b/include/standard-headers/linux/virtio_pci.h
>> index db7a8e2fcb..598ebe9825 100644
>> --- a/include/standard-headers/linux/virtio_pci.h
>> +++ b/include/standard-headers/linux/virtio_pci.h
>> @@ -164,6 +164,7 @@ struct virtio_pci_common_cfg {
>>       uint32_t queue_avail_hi;        /* read-write */
>>       uint32_t queue_used_lo;        /* read-write */
>>       uint32_t queue_used_hi;        /* read-write */
>> +    uint16_t queue_notify_data;    /* read-write */
>>   };
>>     /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
>> @@ -202,6 +203,7 @@ struct virtio_pci_cfg_cap {
>>   #define VIRTIO_PCI_COMMON_Q_AVAILHI    44
>>   #define VIRTIO_PCI_COMMON_Q_USEDLO    48
>>   #define VIRTIO_PCI_COMMON_Q_USEDHI    52
>> +#define VIRTIO_PCI_COMMON_Q_NOTIFY_DATA    56
>>     #endif /* VIRTIO_PCI_NO_MODERN */

[PATCH 02/16] virtio: add VIRTIO_F_RING_RESET

[Kangjie Xu]

From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Added VIRTIO_F_RING_RESET, it came from here
https://github.com/oasis-tcs/virtio-spec/issues/124

This feature indicates that the driver can reset a queue individually.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/standard-headers/linux/virtio_config.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/standard-headers/linux/virtio_config.h b/include/standard-headers/linux/virtio_config.h
index 7acd8d4abc..47a7eef5e4 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -96,4 +96,9 @@
  * Does the device support Single Root I/O Virtualization?
  */
 #define VIRTIO_F_SR_IOV			37
+
+/*
+ * This feature indicates that the driver can reset a queue individually.
+ */
+#define VIRTIO_F_RING_RESET		40
 #endif /* _LINUX_VIRTIO_CONFIG_H */
-- 
2.32.0

[PATCH 03/16] virtio: pci: virtio_pci_common_cfg add queue_reset

[Kangjie Xu]

From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Add queue_reset in virtio_pci_common_cfg.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/standard-headers/linux/virtio_pci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/standard-headers/linux/virtio_pci.h b/include/standard-headers/linux/virtio_pci.h
index 598ebe9825..9feb865636 100644
--- a/include/standard-headers/linux/virtio_pci.h
+++ b/include/standard-headers/linux/virtio_pci.h
@@ -165,6 +165,7 @@ struct virtio_pci_common_cfg {
 	uint32_t queue_used_lo;		/* read-write */
 	uint32_t queue_used_hi;		/* read-write */
 	uint16_t queue_notify_data;	/* read-write */
+	uint16_t queue_reset;		/* read-write */
 };
 
 /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
@@ -204,6 +205,7 @@ struct virtio_pci_cfg_cap {
 #define VIRTIO_PCI_COMMON_Q_USEDLO	48
 #define VIRTIO_PCI_COMMON_Q_USEDHI	52
 #define VIRTIO_PCI_COMMON_Q_NOTIFY_DATA	56
+#define VIRTIO_PCI_COMMON_Q_RESET	58
 
 #endif /* VIRTIO_PCI_NO_MODERN */
 
-- 
2.32.0

[PATCH 04/16] virtio: introduce __virtio_queue_reset()

[Kangjie Xu]

From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Separate the logic of vq reset. This logic will be called directly
later.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/virtio/virtio.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 5d607aeaa0..67d54832a9 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2019,6 +2019,26 @@ static enum virtio_device_endian virtio_current_cpu_endian(void)
     }
 }
 
+static void __virtio_queue_reset(VirtIODevice *vdev, uint32_t i)
+{
+    vdev->vq[i].vring.desc = 0;
+    vdev->vq[i].vring.avail = 0;
+    vdev->vq[i].vring.used = 0;
+    vdev->vq[i].last_avail_idx = 0;
+    vdev->vq[i].shadow_avail_idx = 0;
+    vdev->vq[i].used_idx = 0;
+    vdev->vq[i].last_avail_wrap_counter = true;
+    vdev->vq[i].shadow_avail_wrap_counter = true;
+    vdev->vq[i].used_wrap_counter = true;
+    virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
+    vdev->vq[i].signalled_used = 0;
+    vdev->vq[i].signalled_used_valid = false;
+    vdev->vq[i].notification = true;
+    vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
+    vdev->vq[i].inuse = 0;
+    virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
+}
+
 void virtio_reset(void *opaque)
 {
     VirtIODevice *vdev = opaque;
@@ -2050,22 +2070,7 @@ void virtio_reset(void *opaque)
     virtio_notify_vector(vdev, vdev->config_vector);
 
     for(i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-        vdev->vq[i].vring.desc = 0;
-        vdev->vq[i].vring.avail = 0;
-        vdev->vq[i].vring.used = 0;
-        vdev->vq[i].last_avail_idx = 0;
-        vdev->vq[i].shadow_avail_idx = 0;
-        vdev->vq[i].used_idx = 0;
-        vdev->vq[i].last_avail_wrap_counter = true;
-        vdev->vq[i].shadow_avail_wrap_counter = true;
-        vdev->vq[i].used_wrap_counter = true;
-        virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
-        vdev->vq[i].signalled_used = 0;
-        vdev->vq[i].signalled_used_valid = false;
-        vdev->vq[i].notification = true;
-        vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
-        vdev->vq[i].inuse = 0;
-        virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
+        __virtio_queue_reset(vdev, i);
     }
 }
 
-- 
2.32.0

Re: [PATCH 04/16] virtio: introduce __virtio_queue_reset()

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> Separate the logic of vq reset. This logic will be called directly
> later.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   hw/virtio/virtio.c | 37 +++++++++++++++++++++----------------
>   1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 5d607aeaa0..67d54832a9 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2019,6 +2019,26 @@ static enum virtio_device_endian virtio_current_cpu_endian(void)
>       }
>   }
>   
> +static void __virtio_queue_reset(VirtIODevice *vdev, uint32_t i)
> +{
> +    vdev->vq[i].vring.desc = 0;
> +    vdev->vq[i].vring.avail = 0;
> +    vdev->vq[i].vring.used = 0;
> +    vdev->vq[i].last_avail_idx = 0;
> +    vdev->vq[i].shadow_avail_idx = 0;
> +    vdev->vq[i].used_idx = 0;
> +    vdev->vq[i].last_avail_wrap_counter = true;
> +    vdev->vq[i].shadow_avail_wrap_counter = true;
> +    vdev->vq[i].used_wrap_counter = true;
> +    virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
> +    vdev->vq[i].signalled_used = 0;
> +    vdev->vq[i].signalled_used_valid = false;
> +    vdev->vq[i].notification = true;
> +    vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
> +    vdev->vq[i].inuse = 0;
> +    virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
> +}
> +
>   void virtio_reset(void *opaque)
>   {
>       VirtIODevice *vdev = opaque;
> @@ -2050,22 +2070,7 @@ void virtio_reset(void *opaque)
>       virtio_notify_vector(vdev, vdev->config_vector);
>   
>       for(i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -        vdev->vq[i].vring.desc = 0;
> -        vdev->vq[i].vring.avail = 0;
> -        vdev->vq[i].vring.used = 0;
> -        vdev->vq[i].last_avail_idx = 0;
> -        vdev->vq[i].shadow_avail_idx = 0;
> -        vdev->vq[i].used_idx = 0;
> -        vdev->vq[i].last_avail_wrap_counter = true;
> -        vdev->vq[i].shadow_avail_wrap_counter = true;
> -        vdev->vq[i].used_wrap_counter = true;
> -        virtio_queue_set_vector(vdev, i, VIRTIO_NO_VECTOR);
> -        vdev->vq[i].signalled_used = 0;
> -        vdev->vq[i].signalled_used_valid = false;
> -        vdev->vq[i].notification = true;
> -        vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
> -        vdev->vq[i].inuse = 0;
> -        virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
> +        __virtio_queue_reset(vdev, i);
>       }
>   }
>   

[PATCH 05/16] virtio: introduce virtio_queue_reset()

[Kangjie Xu]

From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Introduce a new interface function virtio_queue_reset() to implement
reset for vq.

Add a new callback to VirtioDeviceClass for queue reset operation for
each child device.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/virtio/virtio.c         | 11 +++++++++++
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 67d54832a9..0e9d41366f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2039,6 +2039,17 @@ static void __virtio_queue_reset(VirtIODevice *vdev, uint32_t i)
     virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
 }
 
+void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
+{
+    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+
+    if (k->queue_reset) {
+        k->queue_reset(vdev, queue_index);
+    }
+
+    __virtio_queue_reset(vdev, queue_index);
+}
+
 void virtio_reset(void *opaque)
 {
     VirtIODevice *vdev = opaque;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index db1c0ddf6b..879394299b 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -130,6 +130,7 @@ struct VirtioDeviceClass {
     void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
     void (*reset)(VirtIODevice *vdev);
     void (*set_status)(VirtIODevice *vdev, uint8_t val);
+    void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
     /* For transitional devices, this is a bitmap of features
      * that are only exposed on the legacy interface but not
      * the modern one.
@@ -268,6 +269,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
                                       MemoryRegion *mr, bool assign);
 int virtio_set_status(VirtIODevice *vdev, uint8_t val);
 void virtio_reset(void *opaque);
+void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
 void virtio_update_irq(VirtIODevice *vdev);
 int virtio_set_features(VirtIODevice *vdev, uint64_t val);
 
-- 
2.32.0

Re: [PATCH 05/16] virtio: introduce virtio_queue_reset()

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> Introduce a new interface function virtio_queue_reset() to implement
> reset for vq.
>
> Add a new callback to VirtioDeviceClass for queue reset operation for
> each child device.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   hw/virtio/virtio.c         | 11 +++++++++++
>   include/hw/virtio/virtio.h |  2 ++
>   2 files changed, 13 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 67d54832a9..0e9d41366f 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2039,6 +2039,17 @@ static void __virtio_queue_reset(VirtIODevice *vdev, uint32_t i)
>       virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
>   }
>   
> +void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
> +{
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +
> +    if (k->queue_reset) {
> +        k->queue_reset(vdev, queue_index);
> +    }
> +
> +    __virtio_queue_reset(vdev, queue_index);
> +}
> +
>   void virtio_reset(void *opaque)
>   {
>       VirtIODevice *vdev = opaque;
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index db1c0ddf6b..879394299b 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -130,6 +130,7 @@ struct VirtioDeviceClass {
>       void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
>       void (*reset)(VirtIODevice *vdev);
>       void (*set_status)(VirtIODevice *vdev, uint8_t val);
> +    void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
>       /* For transitional devices, this is a bitmap of features
>        * that are only exposed on the legacy interface but not
>        * the modern one.
> @@ -268,6 +269,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
>                                         MemoryRegion *mr, bool assign);
>   int virtio_set_status(VirtIODevice *vdev, uint8_t val);
>   void virtio_reset(void *opaque);
> +void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
>   void virtio_update_irq(VirtIODevice *vdev);
>   int virtio_set_features(VirtIODevice *vdev, uint64_t val);
>   

[PATCH 06/16] virtio-pci: support queue reset

[Kangjie Xu]

From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

PCI devices support vq reset.

Based on this function, the driver can adjust the size of the ring, and
quickly recycle the buffer in the ring.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/virtio/virtio-pci.c         | 16 ++++++++++++++++
 include/hw/virtio/virtio-pci.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 45327f0b31..35e8a5101a 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1246,6 +1246,9 @@ static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
     case VIRTIO_PCI_COMMON_Q_USEDHI:
         val = proxy->vqs[vdev->queue_sel].used[1];
         break;
+    case VIRTIO_PCI_COMMON_Q_RESET:
+        val = proxy->vqs[vdev->queue_sel].reset;
+        break;
     default:
         val = 0;
     }
@@ -1333,6 +1336,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
                        ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
                        proxy->vqs[vdev->queue_sel].used[0]);
             proxy->vqs[vdev->queue_sel].enabled = 1;
+            proxy->vqs[vdev->queue_sel].reset = 0;
         } else {
             virtio_error(vdev, "wrong value for queue_enable %"PRIx64, val);
         }
@@ -1355,6 +1359,17 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
     case VIRTIO_PCI_COMMON_Q_USEDHI:
         proxy->vqs[vdev->queue_sel].used[1] = val;
         break;
+    case VIRTIO_PCI_COMMON_Q_RESET:
+        if (val == 1) {
+            proxy->vqs[vdev->queue_sel].reset = 1;
+
+            virtio_queue_reset(vdev, vdev->queue_sel);
+
+            /* mark reset complete */
+            proxy->vqs[vdev->queue_sel].reset = 0;
+            proxy->vqs[vdev->queue_sel].enabled = 0;
+        }
+        break;
     default:
         break;
     }
@@ -1950,6 +1965,7 @@ static void virtio_pci_reset(DeviceState *qdev)
 
     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
         proxy->vqs[i].enabled = 0;
+        proxy->vqs[i].reset = 0;
         proxy->vqs[i].num = 0;
         proxy->vqs[i].desc[0] = proxy->vqs[i].desc[1] = 0;
         proxy->vqs[i].avail[0] = proxy->vqs[i].avail[1] = 0;
diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
index 2446dcd9ae..e9290e2b94 100644
--- a/include/hw/virtio/virtio-pci.h
+++ b/include/hw/virtio/virtio-pci.h
@@ -117,6 +117,7 @@ typedef struct VirtIOPCIRegion {
 typedef struct VirtIOPCIQueue {
   uint16_t num;
   bool enabled;
+  bool reset;
   uint32_t desc[2];
   uint32_t avail[2];
   uint32_t used[2];
-- 
2.32.0

Re: [PATCH 06/16] virtio-pci: support queue reset

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> PCI devices support vq reset.
>
> Based on this function, the driver can adjust the size of the ring, and
> quickly recycle the buffer in the ring.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   hw/virtio/virtio-pci.c         | 16 ++++++++++++++++
>   include/hw/virtio/virtio-pci.h |  1 +
>   2 files changed, 17 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 45327f0b31..35e8a5101a 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1246,6 +1246,9 @@ static uint64_t virtio_pci_common_read(void *opaque, hwaddr addr,
>       case VIRTIO_PCI_COMMON_Q_USEDHI:
>           val = proxy->vqs[vdev->queue_sel].used[1];
>           break;
> +    case VIRTIO_PCI_COMMON_Q_RESET:
> +        val = proxy->vqs[vdev->queue_sel].reset;
> +        break;
>       default:
>           val = 0;
>       }
> @@ -1333,6 +1336,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>                          ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
>                          proxy->vqs[vdev->queue_sel].used[0]);
>               proxy->vqs[vdev->queue_sel].enabled = 1;
> +            proxy->vqs[vdev->queue_sel].reset = 0;
>           } else {
>               virtio_error(vdev, "wrong value for queue_enable %"PRIx64, val);
>           }
> @@ -1355,6 +1359,17 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>       case VIRTIO_PCI_COMMON_Q_USEDHI:
>           proxy->vqs[vdev->queue_sel].used[1] = val;
>           break;
> +    case VIRTIO_PCI_COMMON_Q_RESET:
> +        if (val == 1) {
> +            proxy->vqs[vdev->queue_sel].reset = 1;
> +
> +            virtio_queue_reset(vdev, vdev->queue_sel);
> +
> +            /* mark reset complete */


The code explain itself, so the comment could be removed.


> +            proxy->vqs[vdev->queue_sel].reset = 0;
> +            proxy->vqs[vdev->queue_sel].enabled = 0;
> +        }
> +        break;
>       default:
>           break;
>       }
> @@ -1950,6 +1965,7 @@ static void virtio_pci_reset(DeviceState *qdev)
>   
>       for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
>           proxy->vqs[i].enabled = 0;
> +        proxy->vqs[i].reset = 0;
>           proxy->vqs[i].num = 0;
>           proxy->vqs[i].desc[0] = proxy->vqs[i].desc[1] = 0;
>           proxy->vqs[i].avail[0] = proxy->vqs[i].avail[1] = 0;
> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index 2446dcd9ae..e9290e2b94 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -117,6 +117,7 @@ typedef struct VirtIOPCIRegion {
>   typedef struct VirtIOPCIQueue {
>     uint16_t num;
>     bool enabled;
> +  bool reset;


How is this migrated?

Thanks


>     uint32_t desc[2];
>     uint32_t avail[2];
>     uint32_t used[2];

[PATCH 07/16] virtio-net: support queue reset

[Kangjie Xu]

From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

virtio-net implements queue reset. Queued packets in the corresponding
queue pair are flushed or purged.

Queue reset is currently only implemented for non-vhosts.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/net/virtio-net.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7ad948ee7c..8396e21a67 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -531,6 +531,19 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
     return info;
 }
 
+static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+
+    if (!nc->peer) {
+        return;
+    }
+
+    qemu_flush_or_purge_queued_packets(nc->peer, true);
+    assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
+}
+
 static void virtio_net_reset(VirtIODevice *vdev)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
@@ -741,6 +754,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
     }
 
     if (!get_vhost_net(nc->peer)) {
+        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
         return features;
     }
 
@@ -3766,6 +3780,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
     vdc->set_features = virtio_net_set_features;
     vdc->bad_features = virtio_net_bad_features;
     vdc->reset = virtio_net_reset;
+    vdc->queue_reset = virtio_net_queue_reset;
     vdc->set_status = virtio_net_set_status;
     vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
     vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
-- 
2.32.0

Re: [PATCH 07/16] virtio-net: support queue reset

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> virtio-net implements queue reset. Queued packets in the corresponding
> queue pair are flushed or purged.
>
> Queue reset is currently only implemented for non-vhosts.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   hw/net/virtio-net.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7ad948ee7c..8396e21a67 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -531,6 +531,19 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
>       return info;
>   }
>   
> +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
> +{
> +    VirtIONet *n = VIRTIO_NET(vdev);
> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> +
> +    if (!nc->peer) {
> +        return;
> +    }
> +
> +    qemu_flush_or_purge_queued_packets(nc->peer, true);
> +    assert(!virtio_net_get_subqueue(nc)->async_tx.elem);


Let's try to reuse this function in virtio_net_reset().


> +}
> +
>   static void virtio_net_reset(VirtIODevice *vdev)
>   {
>       VirtIONet *n = VIRTIO_NET(vdev);
> @@ -741,6 +754,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>       }
>   
>       if (!get_vhost_net(nc->peer)) {
> +        virtio_add_feature(&features, VIRTIO_F_RING_RESET);


This breaks migration compatibility.

We probably need:

1) a new command line parameter
2) make it disabled for pre-7.2 machine

Thanks


>           return features;
>       }
>   
> @@ -3766,6 +3780,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
>       vdc->set_features = virtio_net_set_features;
>       vdc->bad_features = virtio_net_bad_features;
>       vdc->reset = virtio_net_reset;
> +    vdc->queue_reset = virtio_net_queue_reset;
>       vdc->set_status = virtio_net_set_status;
>       vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
>       vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;

Re: [PATCH 07/16] virtio-net: support queue reset

[Kangjie Xu]

在 2022/7/26 11:43, Jason Wang 写道:
>
> 在 2022/7/18 19:17, Kangjie Xu 写道:
>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>
>> virtio-net implements queue reset. Queued packets in the corresponding
>> queue pair are flushed or purged.
>>
>> Queue reset is currently only implemented for non-vhosts.
>>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   hw/net/virtio-net.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 7ad948ee7c..8396e21a67 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -531,6 +531,19 @@ static RxFilterInfo 
>> *virtio_net_query_rxfilter(NetClientState *nc)
>>       return info;
>>   }
>>   +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t 
>> queue_index)
>> +{
>> +    VirtIONet *n = VIRTIO_NET(vdev);
>> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
>> +
>> +    if (!nc->peer) {
>> +        return;
>> +    }
>> +
>> +    qemu_flush_or_purge_queued_packets(nc->peer, true);
>> +    assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
>
>
> Let's try to reuse this function in virtio_net_reset().
>
Yeah, I'll fix it.

Thanks.

>
>> +}
>> +
>>   static void virtio_net_reset(VirtIODevice *vdev)
>>   {
>>       VirtIONet *n = VIRTIO_NET(vdev);
>> @@ -741,6 +754,7 @@ static uint64_t 
>> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>       }
>>         if (!get_vhost_net(nc->peer)) {
>> +        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
>
>
> This breaks migration compatibility.
>
> We probably need:
>
> 1) a new command line parameter
> 2) make it disabled for pre-7.2 machine
>
> Thanks
>
>
Sorry, I don't get what is the meaning of "pre-7.2 machine". Could you 
please explain it?

Thanks

>>           return features;
>>       }
>>   @@ -3766,6 +3780,7 @@ static void virtio_net_class_init(ObjectClass 
>> *klass, void *data)
>>       vdc->set_features = virtio_net_set_features;
>>       vdc->bad_features = virtio_net_bad_features;
>>       vdc->reset = virtio_net_reset;
>> +    vdc->queue_reset = virtio_net_queue_reset;
>>       vdc->set_status = virtio_net_set_status;
>>       vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
>>       vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;

Re: [PATCH 07/16] virtio-net: support queue reset

[Jason Wang]

On Tue, Jul 26, 2022 at 3:02 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/7/26 11:43, Jason Wang 写道:
> >
> > 在 2022/7/18 19:17, Kangjie Xu 写道:
> >> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>
> >> virtio-net implements queue reset. Queued packets in the corresponding
> >> queue pair are flushed or purged.
> >>
> >> Queue reset is currently only implemented for non-vhosts.
> >>
> >> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >> ---
> >>   hw/net/virtio-net.c | 15 +++++++++++++++
> >>   1 file changed, 15 insertions(+)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 7ad948ee7c..8396e21a67 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -531,6 +531,19 @@ static RxFilterInfo
> >> *virtio_net_query_rxfilter(NetClientState *nc)
> >>       return info;
> >>   }
> >>   +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t
> >> queue_index)
> >> +{
> >> +    VirtIONet *n = VIRTIO_NET(vdev);
> >> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> >> +
> >> +    if (!nc->peer) {
> >> +        return;
> >> +    }
> >> +
> >> +    qemu_flush_or_purge_queued_packets(nc->peer, true);
> >> +    assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
> >
> >
> > Let's try to reuse this function in virtio_net_reset().
> >
> Yeah, I'll fix it.
>
> Thanks.
>
> >
> >> +}
> >> +
> >>   static void virtio_net_reset(VirtIODevice *vdev)
> >>   {
> >>       VirtIONet *n = VIRTIO_NET(vdev);
> >> @@ -741,6 +754,7 @@ static uint64_t
> >> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> >>       }
> >>         if (!get_vhost_net(nc->peer)) {
> >> +        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
> >
> >
> > This breaks migration compatibility.
> >
> > We probably need:
> >
> > 1) a new command line parameter
> > 2) make it disabled for pre-7.2 machine
> >
> > Thanks
> >
> >
> Sorry, I don't get what is the meaning of "pre-7.2 machine". Could you
> please explain it?

I meant for pre 7.2 machine type, we should make reset fault off by default.

Otherwise we break migration compatibility.

Thanks

>
> Thanks
>
> >>           return features;
> >>       }
> >>   @@ -3766,6 +3780,7 @@ static void virtio_net_class_init(ObjectClass
> >> *klass, void *data)
> >>       vdc->set_features = virtio_net_set_features;
> >>       vdc->bad_features = virtio_net_bad_features;
> >>       vdc->reset = virtio_net_reset;
> >> +    vdc->queue_reset = virtio_net_queue_reset;
> >>       vdc->set_status = virtio_net_set_status;
> >>       vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
> >>       vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
>

Re: [PATCH 07/16] virtio-net: support queue reset

[Kangjie Xu]

在 2022/7/27 13:00, Jason Wang 写道:
> On Tue, Jul 26, 2022 at 3:02 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>>
>> 在 2022/7/26 11:43, Jason Wang 写道:
>>> 在 2022/7/18 19:17, Kangjie Xu 写道:
>>>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>
>>>> virtio-net implements queue reset. Queued packets in the corresponding
>>>> queue pair are flushed or purged.
>>>>
>>>> Queue reset is currently only implemented for non-vhosts.
>>>>
>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> ---
>>>>    hw/net/virtio-net.c | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 7ad948ee7c..8396e21a67 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -531,6 +531,19 @@ static RxFilterInfo
>>>> *virtio_net_query_rxfilter(NetClientState *nc)
>>>>        return info;
>>>>    }
>>>>    +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t
>>>> queue_index)
>>>> +{
>>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
>>>> +
>>>> +    if (!nc->peer) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    qemu_flush_or_purge_queued_packets(nc->peer, true);
>>>> +    assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
>>>
>>> Let's try to reuse this function in virtio_net_reset().
>>>
>> Yeah, I'll fix it.
>>
>> Thanks.
>>
>>>> +}
>>>> +
>>>>    static void virtio_net_reset(VirtIODevice *vdev)
>>>>    {
>>>>        VirtIONet *n = VIRTIO_NET(vdev);
>>>> @@ -741,6 +754,7 @@ static uint64_t
>>>> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>>>        }
>>>>          if (!get_vhost_net(nc->peer)) {
>>>> +        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
>>>
>>> This breaks migration compatibility.
>>>
>>> We probably need:
>>>
>>> 1) a new command line parameter
>>> 2) make it disabled for pre-7.2 machine
>>>
>>> Thanks
>>>
>>>
>> Sorry, I don't get what is the meaning of "pre-7.2 machine". Could you
>> please explain it?
> I meant for pre 7.2 machine type, we should make reset fault off by default.
>
> Otherwise we break migration compatibility.
>
> Thanks

Sorry, I did not express myself clearly. Is "7.2" the version of a 
system or a module? If it is a system version, what is the system?

I did not have backgrond knowledge related to this part and will 
investigate migration issues afterwards.

Thanks.

>> Thanks
>>
>>>>            return features;
>>>>        }
>>>>    @@ -3766,6 +3780,7 @@ static void virtio_net_class_init(ObjectClass
>>>> *klass, void *data)
>>>>        vdc->set_features = virtio_net_set_features;
>>>>        vdc->bad_features = virtio_net_bad_features;
>>>>        vdc->reset = virtio_net_reset;
>>>> +    vdc->queue_reset = virtio_net_queue_reset;
>>>>        vdc->set_status = virtio_net_set_status;
>>>>        vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
>>>>        vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;

Re: [PATCH 07/16] virtio-net: support queue reset

[Jason Wang]

On Wed, Jul 27, 2022 at 2:25 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/7/27 13:00, Jason Wang 写道:
> > On Tue, Jul 26, 2022 at 3:02 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
> >>
> >> 在 2022/7/26 11:43, Jason Wang 写道:
> >>> 在 2022/7/18 19:17, Kangjie Xu 写道:
> >>>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>>>
> >>>> virtio-net implements queue reset. Queued packets in the corresponding
> >>>> queue pair are flushed or purged.
> >>>>
> >>>> Queue reset is currently only implemented for non-vhosts.
> >>>>
> >>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>>> ---
> >>>>    hw/net/virtio-net.c | 15 +++++++++++++++
> >>>>    1 file changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>> index 7ad948ee7c..8396e21a67 100644
> >>>> --- a/hw/net/virtio-net.c
> >>>> +++ b/hw/net/virtio-net.c
> >>>> @@ -531,6 +531,19 @@ static RxFilterInfo
> >>>> *virtio_net_query_rxfilter(NetClientState *nc)
> >>>>        return info;
> >>>>    }
> >>>>    +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t
> >>>> queue_index)
> >>>> +{
> >>>> +    VirtIONet *n = VIRTIO_NET(vdev);
> >>>> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> >>>> +
> >>>> +    if (!nc->peer) {
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    qemu_flush_or_purge_queued_packets(nc->peer, true);
> >>>> +    assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
> >>>
> >>> Let's try to reuse this function in virtio_net_reset().
> >>>
> >> Yeah, I'll fix it.
> >>
> >> Thanks.
> >>
> >>>> +}
> >>>> +
> >>>>    static void virtio_net_reset(VirtIODevice *vdev)
> >>>>    {
> >>>>        VirtIONet *n = VIRTIO_NET(vdev);
> >>>> @@ -741,6 +754,7 @@ static uint64_t
> >>>> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> >>>>        }
> >>>>          if (!get_vhost_net(nc->peer)) {
> >>>> +        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
> >>>
> >>> This breaks migration compatibility.
> >>>
> >>> We probably need:
> >>>
> >>> 1) a new command line parameter
> >>> 2) make it disabled for pre-7.2 machine
> >>>
> >>> Thanks
> >>>
> >>>
> >> Sorry, I don't get what is the meaning of "pre-7.2 machine". Could you
> >> please explain it?
> > I meant for pre 7.2 machine type, we should make reset fault off by default.
> >
> > Otherwise we break migration compatibility.
> >
> > Thanks
>
> Sorry, I did not express myself clearly. Is "7.2" the version of a
> system or a module? If it is a system version, what is the system?

It's the machine type to make sure the migration can work. (you can
get the list of those types via qemu -machine ?)

E.g you can start a 7.0 machine on Qemu 7.1 and it guarantees to be
migrated to 7.0 machine on Qemu 7.0.

>
> I did not have backgrond knowledge related to this part and will
> investigate migration issues afterwards.

Git grep hw_compat_7_0 may give you more hints.

Thanks

>
> Thanks.
>
> >> Thanks
> >>
> >>>>            return features;
> >>>>        }
> >>>>    @@ -3766,6 +3780,7 @@ static void virtio_net_class_init(ObjectClass
> >>>> *klass, void *data)
> >>>>        vdc->set_features = virtio_net_set_features;
> >>>>        vdc->bad_features = virtio_net_bad_features;
> >>>>        vdc->reset = virtio_net_reset;
> >>>> +    vdc->queue_reset = virtio_net_queue_reset;
> >>>>        vdc->set_status = virtio_net_set_status;
> >>>>        vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
> >>>>        vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
>

Re: [PATCH 07/16] virtio-net: support queue reset

[Kangjie Xu]

在 2022/7/27 14:59, Jason Wang 写道:
> On Wed, Jul 27, 2022 at 2:25 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>>
>> 在 2022/7/27 13:00, Jason Wang 写道:
>>> On Tue, Jul 26, 2022 at 3:02 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>>>> 在 2022/7/26 11:43, Jason Wang 写道:
>>>>> 在 2022/7/18 19:17, Kangjie Xu 写道:
>>>>>> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>>>
>>>>>> virtio-net implements queue reset. Queued packets in the corresponding
>>>>>> queue pair are flushed or purged.
>>>>>>
>>>>>> Queue reset is currently only implemented for non-vhosts.
>>>>>>
>>>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>>> ---
>>>>>>     hw/net/virtio-net.c | 15 +++++++++++++++
>>>>>>     1 file changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>> index 7ad948ee7c..8396e21a67 100644
>>>>>> --- a/hw/net/virtio-net.c
>>>>>> +++ b/hw/net/virtio-net.c
>>>>>> @@ -531,6 +531,19 @@ static RxFilterInfo
>>>>>> *virtio_net_query_rxfilter(NetClientState *nc)
>>>>>>         return info;
>>>>>>     }
>>>>>>     +static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t
>>>>>> queue_index)
>>>>>> +{
>>>>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>>>> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
>>>>>> +
>>>>>> +    if (!nc->peer) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    qemu_flush_or_purge_queued_packets(nc->peer, true);
>>>>>> +    assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
>>>>> Let's try to reuse this function in virtio_net_reset().
>>>>>
>>>> Yeah, I'll fix it.
>>>>
>>>> Thanks.
>>>>
>>>>>> +}
>>>>>> +
>>>>>>     static void virtio_net_reset(VirtIODevice *vdev)
>>>>>>     {
>>>>>>         VirtIONet *n = VIRTIO_NET(vdev);
>>>>>> @@ -741,6 +754,7 @@ static uint64_t
>>>>>> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>>>>>         }
>>>>>>           if (!get_vhost_net(nc->peer)) {
>>>>>> +        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
>>>>> This breaks migration compatibility.
>>>>>
>>>>> We probably need:
>>>>>
>>>>> 1) a new command line parameter
>>>>> 2) make it disabled for pre-7.2 machine
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>> Sorry, I don't get what is the meaning of "pre-7.2 machine". Could you
>>>> please explain it?
>>> I meant for pre 7.2 machine type, we should make reset fault off by default.
>>>
>>> Otherwise we break migration compatibility.
>>>
>>> Thanks
>> Sorry, I did not express myself clearly. Is "7.2" the version of a
>> system or a module? If it is a system version, what is the system?
> It's the machine type to make sure the migration can work. (you can
> get the list of those types via qemu -machine ?)
>
> E.g you can start a 7.0 machine on Qemu 7.1 and it guarantees to be
> migrated to 7.0 machine on Qemu 7.0.
>
>> I did not have backgrond knowledge related to this part and will
>> investigate migration issues afterwards.
> Git grep hw_compat_7_0 may give you more hints.
>
> Thanks

Okay, I get it.

Thanks for your guidelines.


>
>> Thanks.
>>
>>>> Thanks
>>>>
>>>>>>             return features;
>>>>>>         }
>>>>>>     @@ -3766,6 +3780,7 @@ static void virtio_net_class_init(ObjectClass
>>>>>> *klass, void *data)
>>>>>>         vdc->set_features = virtio_net_set_features;
>>>>>>         vdc->bad_features = virtio_net_bad_features;
>>>>>>         vdc->reset = virtio_net_reset;
>>>>>> +    vdc->queue_reset = virtio_net_queue_reset;
>>>>>>         vdc->set_status = virtio_net_set_status;
>>>>>>         vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
>>>>>>         vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;

[PATCH 08/16] vhost: add op to enable or disable a single vring

[Kangjie Xu]

The interface to set enable status for a single vring is lacked in
VhostOps, since the vhost_set_vring_enable_op will manipulate all
virtqueues in a device.

Resetting a single vq will rely on this interface. It requires a
reply to indicate that the reset operation is finished, so the
parameter, wait_for_reply, is added.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 include/hw/virtio/vhost-backend.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index eab46d7f0b..7bddd1e9a0 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -81,6 +81,9 @@ typedef int (*vhost_set_backend_cap_op)(struct vhost_dev *dev);
 typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
 typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
 typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
+typedef int (*vhost_set_single_vring_enable_op)(struct vhost_dev *dev,
+                                                int index, int enable,
+                                                bool wait_for_reply);
 typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
                                          int enable);
 typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
@@ -155,6 +158,7 @@ typedef struct VhostOps {
     vhost_set_owner_op vhost_set_owner;
     vhost_reset_device_op vhost_reset_device;
     vhost_get_vq_index_op vhost_get_vq_index;
+    vhost_set_single_vring_enable_op vhost_set_single_vring_enable;
     vhost_set_vring_enable_op vhost_set_vring_enable;
     vhost_requires_shm_log_op vhost_requires_shm_log;
     vhost_migration_done_op vhost_migration_done;
-- 
2.32.0

Re: [PATCH 08/16] vhost: add op to enable or disable a single vring

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> The interface to set enable status for a single vring is lacked in
> VhostOps, since the vhost_set_vring_enable_op will manipulate all
> virtqueues in a device.
>
> Resetting a single vq will rely on this interface. It requires a
> reply to indicate that the reset operation is finished, so the
> parameter, wait_for_reply, is added.


The wait reply seems to be a implementation specific thing. Can we hide it?

Thanks


>
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   include/hw/virtio/vhost-backend.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index eab46d7f0b..7bddd1e9a0 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -81,6 +81,9 @@ typedef int (*vhost_set_backend_cap_op)(struct vhost_dev *dev);
>   typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
>   typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
>   typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
> +typedef int (*vhost_set_single_vring_enable_op)(struct vhost_dev *dev,
> +                                                int index, int enable,
> +                                                bool wait_for_reply);
>   typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
>                                            int enable);
>   typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
> @@ -155,6 +158,7 @@ typedef struct VhostOps {
>       vhost_set_owner_op vhost_set_owner;
>       vhost_reset_device_op vhost_reset_device;
>       vhost_get_vq_index_op vhost_get_vq_index;
> +    vhost_set_single_vring_enable_op vhost_set_single_vring_enable;
>       vhost_set_vring_enable_op vhost_set_vring_enable;
>       vhost_requires_shm_log_op vhost_requires_shm_log;
>       vhost_migration_done_op vhost_migration_done;

Re: [PATCH 08/16] vhost: add op to enable or disable a single vring

[Kangjie Xu]

在 2022/7/26 11:49, Jason Wang 写道:
>
> 在 2022/7/18 19:17, Kangjie Xu 写道:
>> The interface to set enable status for a single vring is lacked in
>> VhostOps, since the vhost_set_vring_enable_op will manipulate all
>> virtqueues in a device.
>>
>> Resetting a single vq will rely on this interface. It requires a
>> reply to indicate that the reset operation is finished, so the
>> parameter, wait_for_reply, is added.
>
>
> The wait reply seems to be a implementation specific thing. Can we 
> hide it?
>
> Thanks
>
I do not hide wait_for_reply here because when stopping the queue, a 
reply is needed to ensure that the message has been processed and queue 
has been disabled.

When restarting the queue, we do not need a reply, which is the same as 
what qemu does in vhost_dev_start().

So I add this parameter to distinguish the two cases.

I think one alternative implementation is to add a interface in 
VhostOps: queue_reset(). In this way details can be hidden. What do you 
think of this solution? Does it look better?

Thanks

>
>>
>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   include/hw/virtio/vhost-backend.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/include/hw/virtio/vhost-backend.h 
>> b/include/hw/virtio/vhost-backend.h
>> index eab46d7f0b..7bddd1e9a0 100644
>> --- a/include/hw/virtio/vhost-backend.h
>> +++ b/include/hw/virtio/vhost-backend.h
>> @@ -81,6 +81,9 @@ typedef int (*vhost_set_backend_cap_op)(struct 
>> vhost_dev *dev);
>>   typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
>>   typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
>>   typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
>> +typedef int (*vhost_set_single_vring_enable_op)(struct vhost_dev *dev,
>> +                                                int index, int enable,
>> +                                                bool wait_for_reply);
>>   typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
>>                                            int enable);
>>   typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
>> @@ -155,6 +158,7 @@ typedef struct VhostOps {
>>       vhost_set_owner_op vhost_set_owner;
>>       vhost_reset_device_op vhost_reset_device;
>>       vhost_get_vq_index_op vhost_get_vq_index;
>> +    vhost_set_single_vring_enable_op vhost_set_single_vring_enable;
>>       vhost_set_vring_enable_op vhost_set_vring_enable;
>>       vhost_requires_shm_log_op vhost_requires_shm_log;
>>       vhost_migration_done_op vhost_migration_done;

Re: [PATCH 08/16] vhost: add op to enable or disable a single vring

[Jason Wang]

On Tue, Jul 26, 2022 at 2:39 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/7/26 11:49, Jason Wang 写道:
> >
> > 在 2022/7/18 19:17, Kangjie Xu 写道:
> >> The interface to set enable status for a single vring is lacked in
> >> VhostOps, since the vhost_set_vring_enable_op will manipulate all
> >> virtqueues in a device.
> >>
> >> Resetting a single vq will rely on this interface. It requires a
> >> reply to indicate that the reset operation is finished, so the
> >> parameter, wait_for_reply, is added.
> >
> >
> > The wait reply seems to be a implementation specific thing. Can we
> > hide it?
> >
> > Thanks
> >
> I do not hide wait_for_reply here because when stopping the queue, a
> reply is needed to ensure that the message has been processed and queue
> has been disabled.

This needs to be done at vhost-backend level instead of the general vhost code.

E.g vhost-kernel or vDPA is using ioctl() which is synchronous.

>
> When restarting the queue, we do not need a reply, which is the same as
> what qemu does in vhost_dev_start().
>
> So I add this parameter to distinguish the two cases.
>
> I think one alternative implementation is to add a interface in
> VhostOps: queue_reset(). In this way details can be hidden. What do you
> think of this solution? Does it look better?

Let me ask it differently, under which case can we call this function
with wait_for_reply = false?

Thanks

>
> Thanks
>
> >
> >>
> >> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> >> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >> ---
> >>   include/hw/virtio/vhost-backend.h | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/include/hw/virtio/vhost-backend.h
> >> b/include/hw/virtio/vhost-backend.h
> >> index eab46d7f0b..7bddd1e9a0 100644
> >> --- a/include/hw/virtio/vhost-backend.h
> >> +++ b/include/hw/virtio/vhost-backend.h
> >> @@ -81,6 +81,9 @@ typedef int (*vhost_set_backend_cap_op)(struct
> >> vhost_dev *dev);
> >>   typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
> >>   typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
> >>   typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
> >> +typedef int (*vhost_set_single_vring_enable_op)(struct vhost_dev *dev,
> >> +                                                int index, int enable,
> >> +                                                bool wait_for_reply);
> >>   typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
> >>                                            int enable);
> >>   typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
> >> @@ -155,6 +158,7 @@ typedef struct VhostOps {
> >>       vhost_set_owner_op vhost_set_owner;
> >>       vhost_reset_device_op vhost_reset_device;
> >>       vhost_get_vq_index_op vhost_get_vq_index;
> >> +    vhost_set_single_vring_enable_op vhost_set_single_vring_enable;
> >>       vhost_set_vring_enable_op vhost_set_vring_enable;
> >>       vhost_requires_shm_log_op vhost_requires_shm_log;
> >>       vhost_migration_done_op vhost_migration_done;
>

Re: [PATCH 08/16] vhost: add op to enable or disable a single vring

[Kangjie Xu]

在 2022/7/27 12:55, Jason Wang 写道:
> On Tue, Jul 26, 2022 at 2:39 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>>
>> 在 2022/7/26 11:49, Jason Wang 写道:
>>> 在 2022/7/18 19:17, Kangjie Xu 写道:
>>>> The interface to set enable status for a single vring is lacked in
>>>> VhostOps, since the vhost_set_vring_enable_op will manipulate all
>>>> virtqueues in a device.
>>>>
>>>> Resetting a single vq will rely on this interface. It requires a
>>>> reply to indicate that the reset operation is finished, so the
>>>> parameter, wait_for_reply, is added.
>>>
>>> The wait reply seems to be a implementation specific thing. Can we
>>> hide it?
>>>
>>> Thanks
>>>
>> I do not hide wait_for_reply here because when stopping the queue, a
>> reply is needed to ensure that the message has been processed and queue
>> has been disabled.
> This needs to be done at vhost-backend level instead of the general vhost code.
>
> E.g vhost-kernel or vDPA is using ioctl() which is synchronous.
Yeah, I understand your concerns, will fix it in the next version.
>> When restarting the queue, we do not need a reply, which is the same as
>> what qemu does in vhost_dev_start().
>>
>> So I add this parameter to distinguish the two cases.
>>
>> I think one alternative implementation is to add a interface in
>> VhostOps: queue_reset(). In this way details can be hidden. What do you
>> think of this solution? Does it look better?
> Let me ask it differently, under which case can we call this function
> with wait_for_reply = false?
>
> Thanks

Cases when we do not need to wait until that the reply has been 
processed. Actually in dev_start(), or dev_stop(), they do not wait for 
replies when enabling/disabling vqs.

Specifically, vhost_set_vring_enable() will call it with wait_for_reply 
= false.

In our vq reset scenario, it should be synchronous because the driver 
need to modify queues after the device stop using the vq in the backend. 
Undefined errors will occur If the device is still using the queue when 
the driver is making modifications to it,

Back to our implementation, we will not expose this parameter in the 
next version.

Thanks.

>> Thanks
>>
>>>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> ---
>>>>    include/hw/virtio/vhost-backend.h | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/include/hw/virtio/vhost-backend.h
>>>> b/include/hw/virtio/vhost-backend.h
>>>> index eab46d7f0b..7bddd1e9a0 100644
>>>> --- a/include/hw/virtio/vhost-backend.h
>>>> +++ b/include/hw/virtio/vhost-backend.h
>>>> @@ -81,6 +81,9 @@ typedef int (*vhost_set_backend_cap_op)(struct
>>>> vhost_dev *dev);
>>>>    typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
>>>>    typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
>>>>    typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
>>>> +typedef int (*vhost_set_single_vring_enable_op)(struct vhost_dev *dev,
>>>> +                                                int index, int enable,
>>>> +                                                bool wait_for_reply);
>>>>    typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
>>>>                                             int enable);
>>>>    typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
>>>> @@ -155,6 +158,7 @@ typedef struct VhostOps {
>>>>        vhost_set_owner_op vhost_set_owner;
>>>>        vhost_reset_device_op vhost_reset_device;
>>>>        vhost_get_vq_index_op vhost_get_vq_index;
>>>> +    vhost_set_single_vring_enable_op vhost_set_single_vring_enable;
>>>>        vhost_set_vring_enable_op vhost_set_vring_enable;
>>>>        vhost_requires_shm_log_op vhost_requires_shm_log;
>>>>        vhost_migration_done_op vhost_migration_done;

Re: [PATCH 08/16] vhost: add op to enable or disable a single vring

[Jason Wang]

On Wed, Jul 27, 2022 at 3:05 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/7/27 12:55, Jason Wang 写道:
> > On Tue, Jul 26, 2022 at 2:39 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
> >>
> >> 在 2022/7/26 11:49, Jason Wang 写道:
> >>> 在 2022/7/18 19:17, Kangjie Xu 写道:
> >>>> The interface to set enable status for a single vring is lacked in
> >>>> VhostOps, since the vhost_set_vring_enable_op will manipulate all
> >>>> virtqueues in a device.
> >>>>
> >>>> Resetting a single vq will rely on this interface. It requires a
> >>>> reply to indicate that the reset operation is finished, so the
> >>>> parameter, wait_for_reply, is added.
> >>>
> >>> The wait reply seems to be a implementation specific thing. Can we
> >>> hide it?
> >>>
> >>> Thanks
> >>>
> >> I do not hide wait_for_reply here because when stopping the queue, a
> >> reply is needed to ensure that the message has been processed and queue
> >> has been disabled.
> > This needs to be done at vhost-backend level instead of the general vhost code.
> >
> > E.g vhost-kernel or vDPA is using ioctl() which is synchronous.
> Yeah, I understand your concerns, will fix it in the next version.
> >> When restarting the queue, we do not need a reply, which is the same as
> >> what qemu does in vhost_dev_start().
> >>
> >> So I add this parameter to distinguish the two cases.
> >>
> >> I think one alternative implementation is to add a interface in
> >> VhostOps: queue_reset(). In this way details can be hidden. What do you
> >> think of this solution? Does it look better?
> > Let me ask it differently, under which case can we call this function
> > with wait_for_reply = false?
> >
> > Thanks
>
> Cases when we do not need to wait until that the reply has been
> processed. Actually in dev_start(), or dev_stop(),

But we don't need to send virtqueue reset requests in those cases?

> they do not wait for
> replies when enabling/disabling vqs.
>
> Specifically, vhost_set_vring_enable() will call it with wait_for_reply
> = false.
>
> In our vq reset scenario, it should be synchronous because the driver
> need to modify queues after the device stop using the vq in the backend.
> Undefined errors will occur If the device is still using the queue when
> the driver is making modifications to it,
>
> Back to our implementation, we will not expose this parameter in the
> next version.

Ok.

Thanks

>
> Thanks.
>
> >> Thanks
> >>
> >>>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> >>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>>> ---
> >>>>    include/hw/virtio/vhost-backend.h | 4 ++++
> >>>>    1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/include/hw/virtio/vhost-backend.h
> >>>> b/include/hw/virtio/vhost-backend.h
> >>>> index eab46d7f0b..7bddd1e9a0 100644
> >>>> --- a/include/hw/virtio/vhost-backend.h
> >>>> +++ b/include/hw/virtio/vhost-backend.h
> >>>> @@ -81,6 +81,9 @@ typedef int (*vhost_set_backend_cap_op)(struct
> >>>> vhost_dev *dev);
> >>>>    typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
> >>>>    typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
> >>>>    typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
> >>>> +typedef int (*vhost_set_single_vring_enable_op)(struct vhost_dev *dev,
> >>>> +                                                int index, int enable,
> >>>> +                                                bool wait_for_reply);
> >>>>    typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
> >>>>                                             int enable);
> >>>>    typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
> >>>> @@ -155,6 +158,7 @@ typedef struct VhostOps {
> >>>>        vhost_set_owner_op vhost_set_owner;
> >>>>        vhost_reset_device_op vhost_reset_device;
> >>>>        vhost_get_vq_index_op vhost_get_vq_index;
> >>>> +    vhost_set_single_vring_enable_op vhost_set_single_vring_enable;
> >>>>        vhost_set_vring_enable_op vhost_set_vring_enable;
> >>>>        vhost_requires_shm_log_op vhost_requires_shm_log;
> >>>>        vhost_migration_done_op vhost_migration_done;
>

Re: [PATCH 08/16] vhost: add op to enable or disable a single vring

[Kangjie Xu]

在 2022/7/28 10:41, Jason Wang 写道:
> On Wed, Jul 27, 2022 at 3:05 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>>
>> 在 2022/7/27 12:55, Jason Wang 写道:
>>> On Tue, Jul 26, 2022 at 2:39 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>>>> 在 2022/7/26 11:49, Jason Wang 写道:
>>>>> 在 2022/7/18 19:17, Kangjie Xu 写道:
>>>>>> The interface to set enable status for a single vring is lacked in
>>>>>> VhostOps, since the vhost_set_vring_enable_op will manipulate all
>>>>>> virtqueues in a device.
>>>>>>
>>>>>> Resetting a single vq will rely on this interface. It requires a
>>>>>> reply to indicate that the reset operation is finished, so the
>>>>>> parameter, wait_for_reply, is added.
>>>>> The wait reply seems to be a implementation specific thing. Can we
>>>>> hide it?
>>>>>
>>>>> Thanks
>>>>>
>>>> I do not hide wait_for_reply here because when stopping the queue, a
>>>> reply is needed to ensure that the message has been processed and queue
>>>> has been disabled.
>>> This needs to be done at vhost-backend level instead of the general vhost code.
>>>
>>> E.g vhost-kernel or vDPA is using ioctl() which is synchronous.
>> Yeah, I understand your concerns, will fix it in the next version.
>>>> When restarting the queue, we do not need a reply, which is the same as
>>>> what qemu does in vhost_dev_start().
>>>>
>>>> So I add this parameter to distinguish the two cases.
>>>>
>>>> I think one alternative implementation is to add a interface in
>>>> VhostOps: queue_reset(). In this way details can be hidden. What do you
>>>> think of this solution? Does it look better?
>>> Let me ask it differently, under which case can we call this function
>>> with wait_for_reply = false?
>>>
>>> Thanks
>> Cases when we do not need to wait until that the reply has been
>> processed. Actually in dev_start(), or dev_stop(),
> But we don't need to send virtqueue reset requests in those cases?

You're right, we don't need to do this in those cases.

Thanks

>> they do not wait for
>> replies when enabling/disabling vqs.
>>
>> Specifically, vhost_set_vring_enable() will call it with wait_for_reply
>> = false.
>>
>> In our vq reset scenario, it should be synchronous because the driver
>> need to modify queues after the device stop using the vq in the backend.
>> Undefined errors will occur If the device is still using the queue when
>> the driver is making modifications to it,
>>
>> Back to our implementation, we will not expose this parameter in the
>> next version.
> Ok.
>
> Thanks
>
>> Thanks.
>>
>>>> Thanks
>>>>
>>>>>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>>>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>>>> ---
>>>>>>     include/hw/virtio/vhost-backend.h | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/include/hw/virtio/vhost-backend.h
>>>>>> b/include/hw/virtio/vhost-backend.h
>>>>>> index eab46d7f0b..7bddd1e9a0 100644
>>>>>> --- a/include/hw/virtio/vhost-backend.h
>>>>>> +++ b/include/hw/virtio/vhost-backend.h
>>>>>> @@ -81,6 +81,9 @@ typedef int (*vhost_set_backend_cap_op)(struct
>>>>>> vhost_dev *dev);
>>>>>>     typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
>>>>>>     typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
>>>>>>     typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
>>>>>> +typedef int (*vhost_set_single_vring_enable_op)(struct vhost_dev *dev,
>>>>>> +                                                int index, int enable,
>>>>>> +                                                bool wait_for_reply);
>>>>>>     typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
>>>>>>                                              int enable);
>>>>>>     typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
>>>>>> @@ -155,6 +158,7 @@ typedef struct VhostOps {
>>>>>>         vhost_set_owner_op vhost_set_owner;
>>>>>>         vhost_reset_device_op vhost_reset_device;
>>>>>>         vhost_get_vq_index_op vhost_get_vq_index;
>>>>>> +    vhost_set_single_vring_enable_op vhost_set_single_vring_enable;
>>>>>>         vhost_set_vring_enable_op vhost_set_vring_enable;
>>>>>>         vhost_requires_shm_log_op vhost_requires_shm_log;
>>>>>>         vhost_migration_done_op vhost_migration_done;

[PATCH 09/16] vhost-user: enable/disable a single vring

[Kangjie Xu]

Implement the vhost_set_single_vring_enable, which is to enable or
disable a single vring.

The parameter wait_for_reply is added to help for some cases such as
vq reset.

Meanwhile, vhost_user_set_vring_enable() is refactored.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/virtio/vhost-user.c | 55 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 75b8df21a4..5a80a415f0 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -267,6 +267,8 @@ struct scrub_regions {
     int fd_idx;
 };
 
+static int enforce_reply(struct vhost_dev *dev, const VhostUserMsg *msg);
+
 static bool ioeventfd_enabled(void)
 {
     return !kvm_enabled() || kvm_eventfds_enabled();
@@ -1198,6 +1200,49 @@ static int vhost_user_set_vring_base(struct vhost_dev *dev,
     return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
 }
 
+
+static int vhost_user_set_single_vring_enable(struct vhost_dev *dev,
+                                              int index,
+                                              int enable,
+                                              bool wait_for_reply)
+{
+    int ret;
+
+    if (index < dev->vq_index || index >= dev->vq_index + dev->nvqs) {
+        return -EINVAL;
+    }
+
+    struct vhost_vring_state state = {
+        .index = index,
+        .num   = enable,
+    };
+
+    VhostUserMsg msg = {
+        .hdr.request = VHOST_USER_SET_VRING_ENABLE,
+        .hdr.flags = VHOST_USER_VERSION,
+        .payload.state = state,
+        .hdr.size = sizeof(msg.payload.state),
+    };
+
+    bool reply_supported = virtio_has_feature(dev->protocol_features,
+                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
+
+    if (reply_supported && wait_for_reply) {
+        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
+    }
+
+    ret = vhost_user_write(dev, &msg, NULL, 0);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (wait_for_reply) {
+        return enforce_reply(dev, &msg);
+    }
+
+    return ret;
+}
+
 static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
 {
     int i;
@@ -1207,13 +1252,8 @@ static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
     }
 
     for (i = 0; i < dev->nvqs; ++i) {
-        int ret;
-        struct vhost_vring_state state = {
-            .index = dev->vq_index + i,
-            .num   = enable,
-        };
-
-        ret = vhost_set_vring(dev, VHOST_USER_SET_VRING_ENABLE, &state);
+        int ret = vhost_user_set_single_vring_enable(dev, dev->vq_index + i,
+                                                     enable, false);
         if (ret < 0) {
             /*
              * Restoring the previous state is likely infeasible, as well as
@@ -2627,6 +2667,7 @@ const VhostOps user_ops = {
         .vhost_set_owner = vhost_user_set_owner,
         .vhost_reset_device = vhost_user_reset_device,
         .vhost_get_vq_index = vhost_user_get_vq_index,
+        .vhost_set_single_vring_enable = vhost_user_set_single_vring_enable,
         .vhost_set_vring_enable = vhost_user_set_vring_enable,
         .vhost_requires_shm_log = vhost_user_requires_shm_log,
         .vhost_migration_done = vhost_user_migration_done,
-- 
2.32.0

Re: [PATCH 09/16] vhost-user: enable/disable a single vring

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> Implement the vhost_set_single_vring_enable, which is to enable or
> disable a single vring.
>
> The parameter wait_for_reply is added to help for some cases such as
> vq reset.
>
> Meanwhile, vhost_user_set_vring_enable() is refactored.
>
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   hw/virtio/vhost-user.c | 55 ++++++++++++++++++++++++++++++++++++------
>   1 file changed, 48 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 75b8df21a4..5a80a415f0 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -267,6 +267,8 @@ struct scrub_regions {
>       int fd_idx;
>   };
>   
> +static int enforce_reply(struct vhost_dev *dev, const VhostUserMsg *msg);
> +
>   static bool ioeventfd_enabled(void)
>   {
>       return !kvm_enabled() || kvm_eventfds_enabled();
> @@ -1198,6 +1200,49 @@ static int vhost_user_set_vring_base(struct vhost_dev *dev,
>       return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
>   }
>   
> +
> +static int vhost_user_set_single_vring_enable(struct vhost_dev *dev,
> +                                              int index,
> +                                              int enable,
> +                                              bool wait_for_reply)
> +{
> +    int ret;
> +
> +    if (index < dev->vq_index || index >= dev->vq_index + dev->nvqs) {
> +        return -EINVAL;
> +    }
> +
> +    struct vhost_vring_state state = {
> +        .index = index,
> +        .num   = enable,
> +    };
> +
> +    VhostUserMsg msg = {
> +        .hdr.request = VHOST_USER_SET_VRING_ENABLE,
> +        .hdr.flags = VHOST_USER_VERSION,
> +        .payload.state = state,
> +        .hdr.size = sizeof(msg.payload.state),
> +    };
> +
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    if (reply_supported && wait_for_reply) {
> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> +    }


Do we need to fail if !realy_supported && wait_for_reply?

Thanks



> +
> +    ret = vhost_user_write(dev, &msg, NULL, 0);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    if (wait_for_reply) {
> +        return enforce_reply(dev, &msg);
> +    }
> +
> +    return ret;
> +}
> +
>   static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
>   {
>       int i;
> @@ -1207,13 +1252,8 @@ static int vhost_user_set_vring_enable(struct vhost_dev *dev, int enable)
>       }
>   
>       for (i = 0; i < dev->nvqs; ++i) {
> -        int ret;
> -        struct vhost_vring_state state = {
> -            .index = dev->vq_index + i,
> -            .num   = enable,
> -        };
> -
> -        ret = vhost_set_vring(dev, VHOST_USER_SET_VRING_ENABLE, &state);
> +        int ret = vhost_user_set_single_vring_enable(dev, dev->vq_index + i,
> +                                                     enable, false);
>           if (ret < 0) {
>               /*
>                * Restoring the previous state is likely infeasible, as well as
> @@ -2627,6 +2667,7 @@ const VhostOps user_ops = {
>           .vhost_set_owner = vhost_user_set_owner,
>           .vhost_reset_device = vhost_user_reset_device,
>           .vhost_get_vq_index = vhost_user_get_vq_index,
> +        .vhost_set_single_vring_enable = vhost_user_set_single_vring_enable,
>           .vhost_set_vring_enable = vhost_user_set_vring_enable,
>           .vhost_requires_shm_log = vhost_user_requires_shm_log,
>           .vhost_migration_done = vhost_user_migration_done,

Re: [PATCH 09/16] vhost-user: enable/disable a single vring

[Kangjie Xu]

在 2022/7/26 12:07, Jason Wang 写道:
>
> 在 2022/7/18 19:17, Kangjie Xu 写道:
>> Implement the vhost_set_single_vring_enable, which is to enable or
>> disable a single vring.
>>
>> The parameter wait_for_reply is added to help for some cases such as
>> vq reset.
>>
>> Meanwhile, vhost_user_set_vring_enable() is refactored.
>>
>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   hw/virtio/vhost-user.c | 55 ++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 48 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index 75b8df21a4..5a80a415f0 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -267,6 +267,8 @@ struct scrub_regions {
>>       int fd_idx;
>>   };
>>   +static int enforce_reply(struct vhost_dev *dev, const VhostUserMsg 
>> *msg);
>> +
>>   static bool ioeventfd_enabled(void)
>>   {
>>       return !kvm_enabled() || kvm_eventfds_enabled();
>> @@ -1198,6 +1200,49 @@ static int vhost_user_set_vring_base(struct 
>> vhost_dev *dev,
>>       return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
>>   }
>>   +
>> +static int vhost_user_set_single_vring_enable(struct vhost_dev *dev,
>> +                                              int index,
>> +                                              int enable,
>> +                                              bool wait_for_reply)
>> +{
>> +    int ret;
>> +
>> +    if (index < dev->vq_index || index >= dev->vq_index + dev->nvqs) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    struct vhost_vring_state state = {
>> +        .index = index,
>> +        .num   = enable,
>> +    };
>> +
>> +    VhostUserMsg msg = {
>> +        .hdr.request = VHOST_USER_SET_VRING_ENABLE,
>> +        .hdr.flags = VHOST_USER_VERSION,
>> +        .payload.state = state,
>> +        .hdr.size = sizeof(msg.payload.state),
>> +    };
>> +
>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>> + VHOST_USER_PROTOCOL_F_REPLY_ACK);
>> +
>> +    if (reply_supported && wait_for_reply) {
>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>> +    }
>
>
> Do we need to fail if !realy_supported && wait_for_reply?
>
> Thanks
>
>
I guess you mean "should we fail if VHOST_USER_PROTOCOL_F_REPLY_ACK 
feature is not supported?".

The implementation here is similar to that in vhost_user_set_vring_addr().

If this feature is not supported, it will call enforce_reply(), then 
call vhost_user_get_features() to get a reply.

Since the messages will be processed sequentailly in DPDK, success of 
enforce_reply() means the previous message VHOST_USER_SET_VRING_ENABLE 
has been processed.

Thanks

>
>> +
>> +    ret = vhost_user_write(dev, &msg, NULL, 0);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    if (wait_for_reply) {
>> +        return enforce_reply(dev, &msg);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static int vhost_user_set_vring_enable(struct vhost_dev *dev, int 
>> enable)
>>   {
>>       int i;
>> @@ -1207,13 +1252,8 @@ static int vhost_user_set_vring_enable(struct 
>> vhost_dev *dev, int enable)
>>       }
>>         for (i = 0; i < dev->nvqs; ++i) {
>> -        int ret;
>> -        struct vhost_vring_state state = {
>> -            .index = dev->vq_index + i,
>> -            .num   = enable,
>> -        };
>> -
>> -        ret = vhost_set_vring(dev, VHOST_USER_SET_VRING_ENABLE, 
>> &state);
>> +        int ret = vhost_user_set_single_vring_enable(dev, 
>> dev->vq_index + i,
>> +                                                     enable, false);
>>           if (ret < 0) {
>>               /*
>>                * Restoring the previous state is likely infeasible, 
>> as well as
>> @@ -2627,6 +2667,7 @@ const VhostOps user_ops = {
>>           .vhost_set_owner = vhost_user_set_owner,
>>           .vhost_reset_device = vhost_user_reset_device,
>>           .vhost_get_vq_index = vhost_user_get_vq_index,
>> +        .vhost_set_single_vring_enable = 
>> vhost_user_set_single_vring_enable,
>>           .vhost_set_vring_enable = vhost_user_set_vring_enable,
>>           .vhost_requires_shm_log = vhost_user_requires_shm_log,
>>           .vhost_migration_done = vhost_user_migration_done,

Re: [PATCH 09/16] vhost-user: enable/disable a single vring

[Jason Wang]

On Tue, Jul 26, 2022 at 1:27 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/7/26 12:07, Jason Wang 写道:
> >
> > 在 2022/7/18 19:17, Kangjie Xu 写道:
> >> Implement the vhost_set_single_vring_enable, which is to enable or
> >> disable a single vring.
> >>
> >> The parameter wait_for_reply is added to help for some cases such as
> >> vq reset.
> >>
> >> Meanwhile, vhost_user_set_vring_enable() is refactored.
> >>
> >> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> >> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >> ---
> >>   hw/virtio/vhost-user.c | 55 ++++++++++++++++++++++++++++++++++++------
> >>   1 file changed, 48 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >> index 75b8df21a4..5a80a415f0 100644
> >> --- a/hw/virtio/vhost-user.c
> >> +++ b/hw/virtio/vhost-user.c
> >> @@ -267,6 +267,8 @@ struct scrub_regions {
> >>       int fd_idx;
> >>   };
> >>   +static int enforce_reply(struct vhost_dev *dev, const VhostUserMsg
> >> *msg);
> >> +
> >>   static bool ioeventfd_enabled(void)
> >>   {
> >>       return !kvm_enabled() || kvm_eventfds_enabled();
> >> @@ -1198,6 +1200,49 @@ static int vhost_user_set_vring_base(struct
> >> vhost_dev *dev,
> >>       return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
> >>   }
> >>   +
> >> +static int vhost_user_set_single_vring_enable(struct vhost_dev *dev,
> >> +                                              int index,
> >> +                                              int enable,
> >> +                                              bool wait_for_reply)
> >> +{
> >> +    int ret;
> >> +
> >> +    if (index < dev->vq_index || index >= dev->vq_index + dev->nvqs) {
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    struct vhost_vring_state state = {
> >> +        .index = index,
> >> +        .num   = enable,
> >> +    };
> >> +
> >> +    VhostUserMsg msg = {
> >> +        .hdr.request = VHOST_USER_SET_VRING_ENABLE,
> >> +        .hdr.flags = VHOST_USER_VERSION,
> >> +        .payload.state = state,
> >> +        .hdr.size = sizeof(msg.payload.state),
> >> +    };
> >> +
> >> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> >> + VHOST_USER_PROTOCOL_F_REPLY_ACK);
> >> +
> >> +    if (reply_supported && wait_for_reply) {
> >> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> >> +    }
> >
> >
> > Do we need to fail if !realy_supported && wait_for_reply?
> >
> > Thanks
> >
> >
> I guess you mean "should we fail if VHOST_USER_PROTOCOL_F_REPLY_ACK
> feature is not supported?".
>
> The implementation here is similar to that in vhost_user_set_vring_addr().
>
> If this feature is not supported, it will call enforce_reply(), then
> call vhost_user_get_features() to get a reply.

Ok, so you meant we can then fallback to VHOST_USER_GET_FEATURES? I
wonder how robust is this, e.g is the bakcend required to process
vhost-user request in order?

Thanks

>
> Since the messages will be processed sequentailly in DPDK, success of
> enforce_reply() means the previous message VHOST_USER_SET_VRING_ENABLE
> has been processed.
>
> Thanks
>
> >
> >> +
> >> +    ret = vhost_user_write(dev, &msg, NULL, 0);
> >> +    if (ret < 0) {
> >> +        return ret;
> >> +    }
> >> +
> >> +    if (wait_for_reply) {
> >> +        return enforce_reply(dev, &msg);
> >> +    }
> >> +
> >> +    return ret;
> >> +}
> >> +
> >>   static int vhost_user_set_vring_enable(struct vhost_dev *dev, int
> >> enable)
> >>   {
> >>       int i;
> >> @@ -1207,13 +1252,8 @@ static int vhost_user_set_vring_enable(struct
> >> vhost_dev *dev, int enable)
> >>       }
> >>         for (i = 0; i < dev->nvqs; ++i) {
> >> -        int ret;
> >> -        struct vhost_vring_state state = {
> >> -            .index = dev->vq_index + i,
> >> -            .num   = enable,
> >> -        };
> >> -
> >> -        ret = vhost_set_vring(dev, VHOST_USER_SET_VRING_ENABLE,
> >> &state);
> >> +        int ret = vhost_user_set_single_vring_enable(dev,
> >> dev->vq_index + i,
> >> +                                                     enable, false);
> >>           if (ret < 0) {
> >>               /*
> >>                * Restoring the previous state is likely infeasible,
> >> as well as
> >> @@ -2627,6 +2667,7 @@ const VhostOps user_ops = {
> >>           .vhost_set_owner = vhost_user_set_owner,
> >>           .vhost_reset_device = vhost_user_reset_device,
> >>           .vhost_get_vq_index = vhost_user_get_vq_index,
> >> +        .vhost_set_single_vring_enable =
> >> vhost_user_set_single_vring_enable,
> >>           .vhost_set_vring_enable = vhost_user_set_vring_enable,
> >>           .vhost_requires_shm_log = vhost_user_requires_shm_log,
> >>           .vhost_migration_done = vhost_user_migration_done,
>

Re: [PATCH 09/16] vhost-user: enable/disable a single vring

[Kangjie Xu]

在 2022/7/27 12:51, Jason Wang 写道:
> On Tue, Jul 26, 2022 at 1:27 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>>
>> 在 2022/7/26 12:07, Jason Wang 写道:
>>> 在 2022/7/18 19:17, Kangjie Xu 写道:
>>>> Implement the vhost_set_single_vring_enable, which is to enable or
>>>> disable a single vring.
>>>>
>>>> The parameter wait_for_reply is added to help for some cases such as
>>>> vq reset.
>>>>
>>>> Meanwhile, vhost_user_set_vring_enable() is refactored.
>>>>
>>>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> ---
>>>>    hw/virtio/vhost-user.c | 55 ++++++++++++++++++++++++++++++++++++------
>>>>    1 file changed, 48 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>>> index 75b8df21a4..5a80a415f0 100644
>>>> --- a/hw/virtio/vhost-user.c
>>>> +++ b/hw/virtio/vhost-user.c
>>>> @@ -267,6 +267,8 @@ struct scrub_regions {
>>>>        int fd_idx;
>>>>    };
>>>>    +static int enforce_reply(struct vhost_dev *dev, const VhostUserMsg
>>>> *msg);
>>>> +
>>>>    static bool ioeventfd_enabled(void)
>>>>    {
>>>>        return !kvm_enabled() || kvm_eventfds_enabled();
>>>> @@ -1198,6 +1200,49 @@ static int vhost_user_set_vring_base(struct
>>>> vhost_dev *dev,
>>>>        return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
>>>>    }
>>>>    +
>>>> +static int vhost_user_set_single_vring_enable(struct vhost_dev *dev,
>>>> +                                              int index,
>>>> +                                              int enable,
>>>> +                                              bool wait_for_reply)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    if (index < dev->vq_index || index >= dev->vq_index + dev->nvqs) {
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    struct vhost_vring_state state = {
>>>> +        .index = index,
>>>> +        .num   = enable,
>>>> +    };
>>>> +
>>>> +    VhostUserMsg msg = {
>>>> +        .hdr.request = VHOST_USER_SET_VRING_ENABLE,
>>>> +        .hdr.flags = VHOST_USER_VERSION,
>>>> +        .payload.state = state,
>>>> +        .hdr.size = sizeof(msg.payload.state),
>>>> +    };
>>>> +
>>>> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
>>>> + VHOST_USER_PROTOCOL_F_REPLY_ACK);
>>>> +
>>>> +    if (reply_supported && wait_for_reply) {
>>>> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>>>> +    }
>>>
>>> Do we need to fail if !realy_supported && wait_for_reply?
>>>
>>> Thanks
>>>
>>>
>> I guess you mean "should we fail if VHOST_USER_PROTOCOL_F_REPLY_ACK
>> feature is not supported?".
>>
>> The implementation here is similar to that in vhost_user_set_vring_addr().
>>
>> If this feature is not supported, it will call enforce_reply(), then
>> call vhost_user_get_features() to get a reply.
> Ok, so you meant we can then fallback to VHOST_USER_GET_FEATURES? I
> wonder how robust is this, e.g is the bakcend required to process
> vhost-user request in order?
>
> Thanks
Yes, we rely on VHOST_USER_GET_FEATURES message to ensure that 
VHOST_USER_SET_VRING_ENABLE has been processed.

It's not robust. I reviewed the vhost-user protocol in qemu doc, 
actually it does not specify that the backend should process them in order.

I think adding a new vhost protocol message can fix this issue. The new 
invented message should reset the queue, and have a blocked read to 
ensure the message has been processed.

Thanks

>> Since the messages will be processed sequentailly in DPDK, success of
>> enforce_reply() means the previous message VHOST_USER_SET_VRING_ENABLE
>> has been processed.
>>
>> Thanks
>>
>>>> +
>>>> +    ret = vhost_user_write(dev, &msg, NULL, 0);
>>>> +    if (ret < 0) {
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    if (wait_for_reply) {
>>>> +        return enforce_reply(dev, &msg);
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>    static int vhost_user_set_vring_enable(struct vhost_dev *dev, int
>>>> enable)
>>>>    {
>>>>        int i;
>>>> @@ -1207,13 +1252,8 @@ static int vhost_user_set_vring_enable(struct
>>>> vhost_dev *dev, int enable)
>>>>        }
>>>>          for (i = 0; i < dev->nvqs; ++i) {
>>>> -        int ret;
>>>> -        struct vhost_vring_state state = {
>>>> -            .index = dev->vq_index + i,
>>>> -            .num   = enable,
>>>> -        };
>>>> -
>>>> -        ret = vhost_set_vring(dev, VHOST_USER_SET_VRING_ENABLE,
>>>> &state);
>>>> +        int ret = vhost_user_set_single_vring_enable(dev,
>>>> dev->vq_index + i,
>>>> +                                                     enable, false);
>>>>            if (ret < 0) {
>>>>                /*
>>>>                 * Restoring the previous state is likely infeasible,
>>>> as well as
>>>> @@ -2627,6 +2667,7 @@ const VhostOps user_ops = {
>>>>            .vhost_set_owner = vhost_user_set_owner,
>>>>            .vhost_reset_device = vhost_user_reset_device,
>>>>            .vhost_get_vq_index = vhost_user_get_vq_index,
>>>> +        .vhost_set_single_vring_enable =
>>>> vhost_user_set_single_vring_enable,
>>>>            .vhost_set_vring_enable = vhost_user_set_vring_enable,
>>>>            .vhost_requires_shm_log = vhost_user_requires_shm_log,
>>>>            .vhost_migration_done = vhost_user_migration_done,

[PATCH 10/16] vhost: extract the logic of unmapping the vrings and desc

[Kangjie Xu]

Introduce vhost_virtqueue_unmap() to ummap the vrings and desc
of a virtqueue.

The function will be used later.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/virtio/vhost.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 0827d631c0..e467dfc7bc 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1197,6 +1197,19 @@ fail_alloc_desc:
     return r;
 }
 
+static void vhost_virtqueue_unmap(struct vhost_dev *dev,
+                                  struct VirtIODevice *vdev,
+                                  struct vhost_virtqueue *vq,
+                                  unsigned idx)
+{
+    vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
+                       1, virtio_queue_get_used_size(vdev, idx));
+    vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
+                       0, virtio_queue_get_avail_size(vdev, idx));
+    vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
+                       0, virtio_queue_get_desc_size(vdev, idx));
+}
+
 static void vhost_virtqueue_stop(struct vhost_dev *dev,
                                     struct VirtIODevice *vdev,
                                     struct vhost_virtqueue *vq,
@@ -1235,12 +1248,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
                                                 vhost_vq_index);
     }
 
-    vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
-                       1, virtio_queue_get_used_size(vdev, idx));
-    vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
-                       0, virtio_queue_get_avail_size(vdev, idx));
-    vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
-                       0, virtio_queue_get_desc_size(vdev, idx));
+    vhost_virtqueue_unmap(dev, vdev, vq, idx);
 }
 
 static void vhost_eventfd_add(MemoryListener *listener,
-- 
2.32.0

Re: [PATCH 10/16] vhost: extract the logic of unmapping the vrings and desc

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> Introduce vhost_virtqueue_unmap() to ummap the vrings and desc
> of a virtqueue.
>
> The function will be used later.
>
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   hw/virtio/vhost.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 0827d631c0..e467dfc7bc 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1197,6 +1197,19 @@ fail_alloc_desc:
>       return r;
>   }
>   
> +static void vhost_virtqueue_unmap(struct vhost_dev *dev,
> +                                  struct VirtIODevice *vdev,
> +                                  struct vhost_virtqueue *vq,
> +                                  unsigned idx)
> +{
> +    vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
> +                       1, virtio_queue_get_used_size(vdev, idx));
> +    vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
> +                       0, virtio_queue_get_avail_size(vdev, idx));
> +    vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
> +                       0, virtio_queue_get_desc_size(vdev, idx));
> +}
> +
>   static void vhost_virtqueue_stop(struct vhost_dev *dev,
>                                       struct VirtIODevice *vdev,
>                                       struct vhost_virtqueue *vq,
> @@ -1235,12 +1248,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
>                                                   vhost_vq_index);
>       }
>   
> -    vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
> -                       1, virtio_queue_get_used_size(vdev, idx));
> -    vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
> -                       0, virtio_queue_get_avail_size(vdev, idx));
> -    vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
> -                       0, virtio_queue_get_desc_size(vdev, idx));
> +    vhost_virtqueue_unmap(dev, vdev, vq, idx);
>   }
>   
>   static void vhost_eventfd_add(MemoryListener *listener,

[PATCH 11/16] vhost: introduce restart and release for vhost_dev's vqs

[Kangjie Xu]

Introduce vhost_dev_virtqueue_restart(), which can restart the
virtqueue when the vhost has already started running.

Meanwhile, vhost_dev_virtqueue_release(), which can ummap the
vrings and the desc of a specific vq of a device.

Combining the two functions, we can reset a virtqueue with a
started vhost.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/virtio/vhost.c         | 29 +++++++++++++++++++++++++++++
 include/hw/virtio/vhost.h |  6 ++++++
 2 files changed, 35 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e467dfc7bc..d158d71866 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1904,3 +1904,32 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
 
     return -ENOSYS;
 }
+
+void vhost_dev_virtqueue_release(struct vhost_dev *hdev, VirtIODevice *vdev,
+                                 int vq_index)
+{
+    int idx = vq_index - hdev->vq_index;
+
+    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
+
+    vhost_virtqueue_unmap(hdev,
+                          vdev,
+                          hdev->vqs + idx,
+                          hdev->vq_index + idx);
+}
+
+int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice *vdev,
+                                int vq_index)
+{
+    int idx = vq_index - hdev->vq_index;
+    int r = 0;
+
+    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
+
+    r = vhost_virtqueue_start(hdev,
+                              vdev,
+                              hdev->vqs + idx,
+                              hdev->vq_index + idx);
+
+    return r;
+}
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index a346f23d13..7df7dbe24d 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -277,6 +277,12 @@ bool vhost_has_free_slot(void);
 int vhost_net_set_backend(struct vhost_dev *hdev,
                           struct vhost_vring_file *file);
 
+
+void vhost_dev_virtqueue_release(struct vhost_dev *hdev, VirtIODevice *vdev,
+                                 int vq_index);
+int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice *vdev,
+                                int vq_index);
+
 int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
 
 void vhost_dev_reset_inflight(struct vhost_inflight *inflight);
-- 
2.32.0

Re: [PATCH 11/16] vhost: introduce restart and release for vhost_dev's vqs

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> Introduce vhost_dev_virtqueue_restart(), which can restart the
> virtqueue when the vhost has already started running.
>
> Meanwhile, vhost_dev_virtqueue_release(), which can ummap the
> vrings and the desc of a specific vq of a device.
>
> Combining the two functions, we can reset a virtqueue with a
> started vhost.
>
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   hw/virtio/vhost.c         | 29 +++++++++++++++++++++++++++++
>   include/hw/virtio/vhost.h |  6 ++++++
>   2 files changed, 35 insertions(+)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index e467dfc7bc..d158d71866 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1904,3 +1904,32 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
>   
>       return -ENOSYS;
>   }
> +
> +void vhost_dev_virtqueue_release(struct vhost_dev *hdev, VirtIODevice *vdev,
> +                                 int vq_index)
> +{
> +    int idx = vq_index - hdev->vq_index;
> +
> +    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
> +
> +    vhost_virtqueue_unmap(hdev,
> +                          vdev,
> +                          hdev->vqs + idx,
> +                          hdev->vq_index + idx);
> +}


Anything wrong that makes you can't use vhost_virtqueue_stop() here?

Thanks


> +
> +int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice *vdev,
> +                                int vq_index)
> +{
> +    int idx = vq_index - hdev->vq_index;
> +    int r = 0;
> +
> +    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
> +
> +    r = vhost_virtqueue_start(hdev,
> +                              vdev,
> +                              hdev->vqs + idx,
> +                              hdev->vq_index + idx);
> +
> +    return r;
> +}
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index a346f23d13..7df7dbe24d 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -277,6 +277,12 @@ bool vhost_has_free_slot(void);
>   int vhost_net_set_backend(struct vhost_dev *hdev,
>                             struct vhost_vring_file *file);
>   
> +
> +void vhost_dev_virtqueue_release(struct vhost_dev *hdev, VirtIODevice *vdev,
> +                                 int vq_index);
> +int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice *vdev,
> +                                int vq_index);
> +
>   int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
>   
>   void vhost_dev_reset_inflight(struct vhost_inflight *inflight);

Re: [PATCH 11/16] vhost: introduce restart and release for vhost_dev's vqs

[Kangjie Xu]

在 2022/7/26 12:13, Jason Wang 写道:
>
> 在 2022/7/18 19:17, Kangjie Xu 写道:
>> Introduce vhost_dev_virtqueue_restart(), which can restart the
>> virtqueue when the vhost has already started running.
>>
>> Meanwhile, vhost_dev_virtqueue_release(), which can ummap the
>> vrings and the desc of a specific vq of a device.
>>
>> Combining the two functions, we can reset a virtqueue with a
>> started vhost.
>>
>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   hw/virtio/vhost.c         | 29 +++++++++++++++++++++++++++++
>>   include/hw/virtio/vhost.h |  6 ++++++
>>   2 files changed, 35 insertions(+)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index e467dfc7bc..d158d71866 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1904,3 +1904,32 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
>>         return -ENOSYS;
>>   }
>> +
>> +void vhost_dev_virtqueue_release(struct vhost_dev *hdev, 
>> VirtIODevice *vdev,
>> +                                 int vq_index)
>> +{
>> +    int idx = vq_index - hdev->vq_index;
>> +
>> +    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
>> +
>> +    vhost_virtqueue_unmap(hdev,
>> +                          vdev,
>> +                          hdev->vqs + idx,
>> +                          hdev->vq_index + idx);
>> +}
>
>
> Anything wrong that makes you can't use vhost_virtqueue_stop() here?
>
> Thanks
>
>
Yeah, vhost_virtqueue_stop() will result in all the queues in the device 
being stopped.

In the scenario of DPDK-OVS, vhost_virtqueue_stop will send 
VHOST_USER_GET_VRING_BASE to DPDK. This message is meant to destroy the 
queue.

However, In DPDK, VHOST_USER_GET_VRING_BASE message will lead to 
vhost_destroy_device_notify(). This function will disable all the queues 
in the device.

Thus, when restarting the queue, other queues except the restarted one 
are still disabled.

On the other hand, I think it may be not necessary to destroy the queue. 
We can simply disable it. When restarting the queue, it will be 
intialized again.

I'm sorry I forgot to cc in the last email, so I resend this email.

Thanks
>> +
>> +int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice 
>> *vdev,
>> +                                int vq_index)
>> +{
>> +    int idx = vq_index - hdev->vq_index;
>> +    int r = 0;
>> +
>> +    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
>> +
>> +    r = vhost_virtqueue_start(hdev,
>> +                              vdev,
>> +                              hdev->vqs + idx,
>> +                              hdev->vq_index + idx);
>> +
>> +    return r;
>> +}
>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>> index a346f23d13..7df7dbe24d 100644
>> --- a/include/hw/virtio/vhost.h
>> +++ b/include/hw/virtio/vhost.h
>> @@ -277,6 +277,12 @@ bool vhost_has_free_slot(void);
>>   int vhost_net_set_backend(struct vhost_dev *hdev,
>>                             struct vhost_vring_file *file);
>>   +
>> +void vhost_dev_virtqueue_release(struct vhost_dev *hdev, 
>> VirtIODevice *vdev,
>> +                                 int vq_index);
>> +int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice 
>> *vdev,
>> +                                int vq_index);
>> +
>>   int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, 
>> int write);
>>     void vhost_dev_reset_inflight(struct vhost_inflight *inflight);

Re: [PATCH 11/16] vhost: introduce restart and release for vhost_dev's vqs

[Kangjie Xu]

在 2022/7/27 12:47, Jason Wang 写道:
> On Tue, Jul 26, 2022 at 1:13 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>>
>> 在 2022/7/26 12:13, Jason Wang 写道:
>>> 在 2022/7/18 19:17, Kangjie Xu 写道:
>>>> Introduce vhost_dev_virtqueue_restart(), which can restart the
>>>> virtqueue when the vhost has already started running.
>>>>
>>>> Meanwhile, vhost_dev_virtqueue_release(), which can ummap the
>>>> vrings and the desc of a specific vq of a device.
>>>>
>>>> Combining the two functions, we can reset a virtqueue with a
>>>> started vhost.
>>>>
>>>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>> ---
>>>>    hw/virtio/vhost.c         | 29 +++++++++++++++++++++++++++++
>>>>    include/hw/virtio/vhost.h |  6 ++++++
>>>>    2 files changed, 35 insertions(+)
>>>>
>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>> index e467dfc7bc..d158d71866 100644
>>>> --- a/hw/virtio/vhost.c
>>>> +++ b/hw/virtio/vhost.c
>>>> @@ -1904,3 +1904,32 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
>>>>          return -ENOSYS;
>>>>    }
>>>> +
>>>> +void vhost_dev_virtqueue_release(struct vhost_dev *hdev,
>>>> VirtIODevice *vdev,
>>>> +                                 int vq_index)
>>>> +{
>>>> +    int idx = vq_index - hdev->vq_index;
>>>> +
>>>> +    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
>>>> +
>>>> +    vhost_virtqueue_unmap(hdev,
>>>> +                          vdev,
>>>> +                          hdev->vqs + idx,
>>>> +                          hdev->vq_index + idx);
>>>> +}
>>>
>>> Anything wrong that makes you can't use vhost_virtqueue_stop() here?
>>>
>>> Thanks
>>>
>> Yeah, vhost_virtqueue_stop() will result in all the queues in the device
>> being stopped.
> This sounds a defect of the backend. I think we'd better not
> workaround it in the qemu.
Yeah, I agree.
>> In the scenario of DPDK-OVS, vhost_virtqueue_stop will send
>> VHOST_USER_GET_VRING_BASE to DPDK. This message is meant to destroy the
>> queue.
> That's tricky.
>
>> However, In DPDK, VHOST_USER_GET_VRING_BASE message will lead to
>> vhost_destroy_device_notify(). This function will disable all the queues
>> in the device.
> Ok. I wonder how the reset is implemented in DPDK? It looks to me a
> new type of request is required.
>
Currently, DPDK does not support reset of queue level, but it supports 
device-level reset. VHOST_USER_RESET_DEVICE request exists in vhost 
protocol but it is not used in vhost-net module in QEMU.

It seems like there is no way for the driver to directly reset the 
device in vhost user net (If we use ethtool -r <eth_name>, we will get 
operation not supported.)

Actually we can plug the driver in and out to perform a device reset 
operation, but it will be applied to all the devices. I take it for 
example here.

If you plug out the driver(modprobe -r virtio-net), vhost_dev_stop() 
will be called in qemu. DPDK will receive VHOST_USER_GET_VRING_BASE 
messages for each virtqueue.

Then you can plug in the driver(modprobe virtio-net), vhost_dev_start() 
will be called in qemu. The process is same as when we boot the machine, 
DPDK will receive:

     VHOST_USER_SET_VRING_NUM, VHOST_USER_SET_VRING_BASE, 
VHOST_USER_SET_VRING_ADDR, VHOST_USER_SET_VRING_KICK, 
VHOST_USER_SET_VRING_CALL

in order to start a virtqueue.

Our vq reset implemenation does not need any change to the DPDK except 
adding the feature bit, since we reuse all the request types in 
vhost-user protocol.

I agree that we should add a new request in vhost-user protocol to 
support vq reset, we'll implement it in the next version.

>> Thus, when restarting the queue, other queues except the restarted one
>> are still disabled.
>>
>> On the other hand, I think it may be not necessary to destroy the queue.
>> We can simply disable it. When restarting the queue, it will be
>> intialized again.
> So for vhost-net kernel, we can simply set NULL as the backend for a
> specific virtqueue. We just need to refactor the stop codes (which did
> it at vhost device level instead of queue level currently).
>
> Thanks

Yes, I think so.

Thanks

>> Thanks
>>
>>>> +
>>>> +int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice
>>>> *vdev,
>>>> +                                int vq_index)
>>>> +{
>>>> +    int idx = vq_index - hdev->vq_index;
>>>> +    int r = 0;
>>>> +
>>>> +    idx = hdev->vhost_ops->vhost_get_vq_index(hdev, idx);
>>>> +
>>>> +    r = vhost_virtqueue_start(hdev,
>>>> +                              vdev,
>>>> +                              hdev->vqs + idx,
>>>> +                              hdev->vq_index + idx);
>>>> +
>>>> +    return r;
>>>> +}
>>>> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>>>> index a346f23d13..7df7dbe24d 100644
>>>> --- a/include/hw/virtio/vhost.h
>>>> +++ b/include/hw/virtio/vhost.h
>>>> @@ -277,6 +277,12 @@ bool vhost_has_free_slot(void);
>>>>    int vhost_net_set_backend(struct vhost_dev *hdev,
>>>>                              struct vhost_vring_file *file);
>>>>    +
>>>> +void vhost_dev_virtqueue_release(struct vhost_dev *hdev,
>>>> VirtIODevice *vdev,
>>>> +                                 int vq_index);
>>>> +int vhost_dev_virtqueue_restart(struct vhost_dev *hdev, VirtIODevice
>>>> *vdev,
>>>> +                                int vq_index);
>>>> +
>>>>    int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova,
>>>> int write);
>>>>      void vhost_dev_reset_inflight(struct vhost_inflight *inflight);

[PATCH 12/16] vhost-net: introduce restart and stop for vhost_net's vqs

[Kangjie Xu]

Introduce vhost_virtqueue_restart(), which can restart the
virtqueue when the vhost net started running before.

Introduce vhost_virtqueue_stop(), which can disable the vq
and unmap vrings and the desc of the vq. When disabling the
vq, the function is blocked and waits for a reply.

Combining the two functions, we can reset a virtqueue with a
started vhost net.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/net/vhost_net.c      | 55 +++++++++++++++++++++++++++++++++++++++++
 include/net/vhost_net.h |  5 ++++
 2 files changed, 60 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index ccac5b7a64..4f5f034c11 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -514,3 +514,58 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu)
 
     return vhost_ops->vhost_net_set_mtu(&net->dev, mtu);
 }
+
+void vhost_virtqueue_stop(VirtIODevice *vdev, NetClientState *nc,
+                          int vq_index)
+{
+    VHostNetState *net = get_vhost_net(nc->peer);
+    const VhostOps *vhost_ops = net->dev.vhost_ops;
+    int r;
+
+    assert(vhost_ops);
+
+    r = vhost_ops->vhost_set_single_vring_enable(&net->dev, vq_index, 0, true);
+    if (r < 0) {
+        goto err_queue_disable;
+    }
+
+    vhost_dev_virtqueue_release(&net->dev, vdev, vq_index);
+
+    return;
+
+err_queue_disable:
+    error_report("Error when releasing the qeuue.");
+}
+
+int vhost_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
+                            int vq_index)
+{
+    VHostNetState *net = get_vhost_net(nc->peer);
+    const VhostOps *vhost_ops = net->dev.vhost_ops;
+    int r;
+
+    if (!net->dev.started) {
+        return 0;
+    }
+
+    assert(vhost_ops);
+
+    r = vhost_dev_virtqueue_restart(&net->dev, vdev, vq_index);
+    if (r < 0) {
+        goto err_start;
+    }
+
+    r = vhost_ops->vhost_set_single_vring_enable(&net->dev, vq_index, 1,
+                                                 false);
+    if (r < 0) {
+        goto err_start;
+    }
+
+    return 0;
+
+err_start:
+    error_report("Error when restarting the queue.");
+    vhost_dev_stop(&net->dev, vdev);
+
+    return r;
+}
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 387e913e4e..fcb09e36ef 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -48,4 +48,9 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net);
 
 int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
 
+void vhost_virtqueue_stop(VirtIODevice *vdev, NetClientState *nc,
+                          int vq_index);
+int vhost_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
+                            int vq_index);
+
 #endif
-- 
2.32.0

Re: [PATCH 12/16] vhost-net: introduce restart and stop for vhost_net's vqs

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> Introduce vhost_virtqueue_restart(), which can restart the
> virtqueue when the vhost net started running before.
>
> Introduce vhost_virtqueue_stop(), which can disable the vq
> and unmap vrings and the desc of the vq. When disabling the
> vq, the function is blocked and waits for a reply.
>
> Combining the two functions, we can reset a virtqueue with a
> started vhost net.
>
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   hw/net/vhost_net.c      | 55 +++++++++++++++++++++++++++++++++++++++++
>   include/net/vhost_net.h |  5 ++++
>   2 files changed, 60 insertions(+)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index ccac5b7a64..4f5f034c11 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -514,3 +514,58 @@ int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu)
>   
>       return vhost_ops->vhost_net_set_mtu(&net->dev, mtu);
>   }
> +
> +void vhost_virtqueue_stop(VirtIODevice *vdev, NetClientState *nc,
> +                          int vq_index)


Let's rename this as vhost_net_virtqueue_stop()


> +{
> +    VHostNetState *net = get_vhost_net(nc->peer);
> +    const VhostOps *vhost_ops = net->dev.vhost_ops;
> +    int r;
> +
> +    assert(vhost_ops);
> +
> +    r = vhost_ops->vhost_set_single_vring_enable(&net->dev, vq_index, 0, true);
> +    if (r < 0) {
> +        goto err_queue_disable;
> +    }
> +
> +    vhost_dev_virtqueue_release(&net->dev, vdev, vq_index);
> +
> +    return;
> +
> +err_queue_disable:
> +    error_report("Error when releasing the qeuue.");
> +}
> +
> +int vhost_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
> +                            int vq_index)
> +{
> +    VHostNetState *net = get_vhost_net(nc->peer);
> +    const VhostOps *vhost_ops = net->dev.vhost_ops;
> +    int r;
> +
> +    if (!net->dev.started) {
> +        return 0;
> +    }
> +
> +    assert(vhost_ops);
> +
> +    r = vhost_dev_virtqueue_restart(&net->dev, vdev, vq_index);
> +    if (r < 0) {
> +        goto err_start;
> +    }
> +
> +    r = vhost_ops->vhost_set_single_vring_enable(&net->dev, vq_index, 1,
> +                                                 false);


So it looks nothing vhost_net specific but vhost. And why not do 
set_single_vring_enable in vhost_dev_virtqueue_restart() (btw, the name 
of this function looks confusing).

Thanks


> +    if (r < 0) {
> +        goto err_start;
> +    }
> +
> +    return 0;
> +
> +err_start:
> +    error_report("Error when restarting the queue.");
> +    vhost_dev_stop(&net->dev, vdev);
> +
> +    return r;
> +}
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index 387e913e4e..fcb09e36ef 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -48,4 +48,9 @@ uint64_t vhost_net_get_acked_features(VHostNetState *net);
>   
>   int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
>   
> +void vhost_virtqueue_stop(VirtIODevice *vdev, NetClientState *nc,
> +                          int vq_index);
> +int vhost_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
> +                            int vq_index);
> +
>   #endif

Re: [PATCH 12/16] vhost-net: introduce restart and stop for vhost_net's vqs

[Kangjie Xu]

在 2022/7/26 12:16, Jason Wang 写道:
>
> 在 2022/7/18 19:17, Kangjie Xu 写道:
>> Introduce vhost_virtqueue_restart(), which can restart the
>> virtqueue when the vhost net started running before.
>>
>> Introduce vhost_virtqueue_stop(), which can disable the vq
>> and unmap vrings and the desc of the vq. When disabling the
>> vq, the function is blocked and waits for a reply.
>>
>> Combining the two functions, we can reset a virtqueue with a
>> started vhost net.
>>
>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   hw/net/vhost_net.c      | 55 +++++++++++++++++++++++++++++++++++++++++
>>   include/net/vhost_net.h |  5 ++++
>>   2 files changed, 60 insertions(+)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index ccac5b7a64..4f5f034c11 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -514,3 +514,58 @@ int vhost_net_set_mtu(struct vhost_net *net, 
>> uint16_t mtu)
>>         return vhost_ops->vhost_net_set_mtu(&net->dev, mtu);
>>   }
>> +
>> +void vhost_virtqueue_stop(VirtIODevice *vdev, NetClientState *nc,
>> +                          int vq_index)
>
>
> Let's rename this as vhost_net_virtqueue_stop()
>
Yeah, I agree.

Thanks

>
>> +{
>> +    VHostNetState *net = get_vhost_net(nc->peer);
>> +    const VhostOps *vhost_ops = net->dev.vhost_ops;
>> +    int r;
>> +
>> +    assert(vhost_ops);
>> +
>> +    r = vhost_ops->vhost_set_single_vring_enable(&net->dev, 
>> vq_index, 0, true);
>> +    if (r < 0) {
>> +        goto err_queue_disable;
>> +    }
>> +
>> +    vhost_dev_virtqueue_release(&net->dev, vdev, vq_index);
>> +
>> +    return;
>> +
>> +err_queue_disable:
>> +    error_report("Error when releasing the qeuue.");
>> +}
>> +
>> +int vhost_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
>> +                            int vq_index)
>> +{
>> +    VHostNetState *net = get_vhost_net(nc->peer);
>> +    const VhostOps *vhost_ops = net->dev.vhost_ops;
>> +    int r;
>> +
>> +    if (!net->dev.started) {
>> +        return 0;
>> +    }
>> +
>> +    assert(vhost_ops);
>> +
>> +    r = vhost_dev_virtqueue_restart(&net->dev, vdev, vq_index);
>> +    if (r < 0) {
>> +        goto err_start;
>> +    }
>> +
>> +    r = vhost_ops->vhost_set_single_vring_enable(&net->dev, 
>> vq_index, 1,
>> +                                                 false);
>
>
> So it looks nothing vhost_net specific but vhost. And why not do 
> set_single_vring_enable in vhost_dev_virtqueue_restart() (btw, the 
> name of this function looks confusing).
>
> Thanks
>
>
I thought it was a convention that virtio-net will call vhost_net first, 
and cannot call vhost directly. So I implement it in this way.

It's no probem to move it to vhost. I'll fix it.

Thanks

>> +    if (r < 0) {
>> +        goto err_start;
>> +    }
>> +
>> +    return 0;
>> +
>> +err_start:
>> +    error_report("Error when restarting the queue.");
>> +    vhost_dev_stop(&net->dev, vdev);
>> +
>> +    return r;
>> +}
>> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
>> index 387e913e4e..fcb09e36ef 100644
>> --- a/include/net/vhost_net.h
>> +++ b/include/net/vhost_net.h
>> @@ -48,4 +48,9 @@ uint64_t vhost_net_get_acked_features(VHostNetState 
>> *net);
>>     int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu);
>>   +void vhost_virtqueue_stop(VirtIODevice *vdev, NetClientState *nc,
>> +                          int vq_index);
>> +int vhost_virtqueue_restart(VirtIODevice *vdev, NetClientState *nc,
>> +                            int vq_index);
>> +
>>   #endif

[PATCH 13/16] virtio: introduce queue_enable in virtio

[Kangjie Xu]

Introduce the interface queue_enable() in VirtioDeviceClass and the
fucntion virtio_queue_enable() in virtio, it can be called when
VIRTIO_PCI_COMMON_Q_ENABLE is written.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/virtio/virtio-pci.c     | 1 +
 hw/virtio/virtio.c         | 9 +++++++++
 include/hw/virtio/virtio.h | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 35e8a5101a..85e1840479 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1335,6 +1335,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
                        proxy->vqs[vdev->queue_sel].avail[0],
                        ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
                        proxy->vqs[vdev->queue_sel].used[0]);
+            virtio_queue_enable(vdev, vdev->queue_sel);
             proxy->vqs[vdev->queue_sel].enabled = 1;
             proxy->vqs[vdev->queue_sel].reset = 0;
         } else {
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 0e9d41366f..82eb9dd4f2 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2050,6 +2050,15 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
     __virtio_queue_reset(vdev, queue_index);
 }
 
+void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
+{
+    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+
+    if (k->queue_enable) {
+        k->queue_enable(vdev, queue_index);
+    }
+}
+
 void virtio_reset(void *opaque)
 {
     VirtIODevice *vdev = opaque;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 879394299b..085997d8f3 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -131,6 +131,7 @@ struct VirtioDeviceClass {
     void (*reset)(VirtIODevice *vdev);
     void (*set_status)(VirtIODevice *vdev, uint8_t val);
     void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
+    void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
     /* For transitional devices, this is a bitmap of features
      * that are only exposed on the legacy interface but not
      * the modern one.
@@ -270,6 +271,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
 int virtio_set_status(VirtIODevice *vdev, uint8_t val);
 void virtio_reset(void *opaque);
 void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
+void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
 void virtio_update_irq(VirtIODevice *vdev);
 int virtio_set_features(VirtIODevice *vdev, uint64_t val);
 
-- 
2.32.0

Re: [PATCH 13/16] virtio: introduce queue_enable in virtio

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> Introduce the interface queue_enable() in VirtioDeviceClass and the
> fucntion virtio_queue_enable() in virtio, it can be called when
> VIRTIO_PCI_COMMON_Q_ENABLE is written.


I'd suggest to split this series into two.

1) queue_enable

2) queue_reset

The patch looks good to me.

Thanks


>
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   hw/virtio/virtio-pci.c     | 1 +
>   hw/virtio/virtio.c         | 9 +++++++++
>   include/hw/virtio/virtio.h | 2 ++
>   3 files changed, 12 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 35e8a5101a..85e1840479 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1335,6 +1335,7 @@ static void virtio_pci_common_write(void *opaque, hwaddr addr,
>                          proxy->vqs[vdev->queue_sel].avail[0],
>                          ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
>                          proxy->vqs[vdev->queue_sel].used[0]);
> +            virtio_queue_enable(vdev, vdev->queue_sel);
>               proxy->vqs[vdev->queue_sel].enabled = 1;
>               proxy->vqs[vdev->queue_sel].reset = 0;
>           } else {
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 0e9d41366f..82eb9dd4f2 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2050,6 +2050,15 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
>       __virtio_queue_reset(vdev, queue_index);
>   }
>   
> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> +{
> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> +
> +    if (k->queue_enable) {
> +        k->queue_enable(vdev, queue_index);
> +    }
> +}
> +
>   void virtio_reset(void *opaque)
>   {
>       VirtIODevice *vdev = opaque;
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 879394299b..085997d8f3 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -131,6 +131,7 @@ struct VirtioDeviceClass {
>       void (*reset)(VirtIODevice *vdev);
>       void (*set_status)(VirtIODevice *vdev, uint8_t val);
>       void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
> +    void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
>       /* For transitional devices, this is a bitmap of features
>        * that are only exposed on the legacy interface but not
>        * the modern one.
> @@ -270,6 +271,7 @@ int virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
>   int virtio_set_status(VirtIODevice *vdev, uint8_t val);
>   void virtio_reset(void *opaque);
>   void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
>   void virtio_update_irq(VirtIODevice *vdev);
>   int virtio_set_features(VirtIODevice *vdev, uint64_t val);
>   

Re: [PATCH 13/16] virtio: introduce queue_enable in virtio

[Kangjie Xu]

在 2022/7/26 12:17, Jason Wang 写道:
>
> 在 2022/7/18 19:17, Kangjie Xu 写道:
>> Introduce the interface queue_enable() in VirtioDeviceClass and the
>> fucntion virtio_queue_enable() in virtio, it can be called when
>> VIRTIO_PCI_COMMON_Q_ENABLE is written.
>
>
> I'd suggest to split this series into two.
>
> 1) queue_enable
>
> 2) queue_reset
>
> The patch looks good to me.
>
> Thanks
>
Okay, will fix.

Thanks

>
>>
>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   hw/virtio/virtio-pci.c     | 1 +
>>   hw/virtio/virtio.c         | 9 +++++++++
>>   include/hw/virtio/virtio.h | 2 ++
>>   3 files changed, 12 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 35e8a5101a..85e1840479 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1335,6 +1335,7 @@ static void virtio_pci_common_write(void 
>> *opaque, hwaddr addr,
>> proxy->vqs[vdev->queue_sel].avail[0],
>> ((uint64_t)proxy->vqs[vdev->queue_sel].used[1]) << 32 |
>> proxy->vqs[vdev->queue_sel].used[0]);
>> +            virtio_queue_enable(vdev, vdev->queue_sel);
>>               proxy->vqs[vdev->queue_sel].enabled = 1;
>>               proxy->vqs[vdev->queue_sel].reset = 0;
>>           } else {
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 0e9d41366f..82eb9dd4f2 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2050,6 +2050,15 @@ void virtio_queue_reset(VirtIODevice *vdev, 
>> uint32_t queue_index)
>>       __virtio_queue_reset(vdev, queue_index);
>>   }
>>   +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
>> +{
>> +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>> +
>> +    if (k->queue_enable) {
>> +        k->queue_enable(vdev, queue_index);
>> +    }
>> +}
>> +
>>   void virtio_reset(void *opaque)
>>   {
>>       VirtIODevice *vdev = opaque;
>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>> index 879394299b..085997d8f3 100644
>> --- a/include/hw/virtio/virtio.h
>> +++ b/include/hw/virtio/virtio.h
>> @@ -131,6 +131,7 @@ struct VirtioDeviceClass {
>>       void (*reset)(VirtIODevice *vdev);
>>       void (*set_status)(VirtIODevice *vdev, uint8_t val);
>>       void (*queue_reset)(VirtIODevice *vdev, uint32_t queue_index);
>> +    void (*queue_enable)(VirtIODevice *vdev, uint32_t queue_index);
>>       /* For transitional devices, this is a bitmap of features
>>        * that are only exposed on the legacy interface but not
>>        * the modern one.
>> @@ -270,6 +271,7 @@ int 
>> virtio_queue_set_host_notifier_mr(VirtIODevice *vdev, int n,
>>   int virtio_set_status(VirtIODevice *vdev, uint8_t val);
>>   void virtio_reset(void *opaque);
>>   void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index);
>> +void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index);
>>   void virtio_update_irq(VirtIODevice *vdev);
>>   int virtio_set_features(VirtIODevice *vdev, uint64_t val);

[PATCH 14/16] virtio-net: support queue_enable for vhost-user

[Kangjie Xu]

Support queue enable in vhost-user scenario. It will be called when
a vq reset operation is performed and the vq will be restared.

It should be noted that we can restart the vq when the vhost has
already started. When launching a new vhost-user device, the vhost
is not started and all vqs are not initalized until
VIRTIO_PCI_COMMON_STATUS is written. Thus, we should use vhost_started
to differentiate the two cases: vq reset and device start.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/net/virtio-net.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 8396e21a67..2c26e2ef73 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -544,6 +544,25 @@ static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
     assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
 }
 
+static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
+    int r;
+
+    if (!nc->peer || !vdev->vhost_started) {
+        return;
+    }
+
+    if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
+        r = vhost_virtqueue_restart(vdev, nc, queue_index);
+        if (r < 0) {
+            error_report("unable to restart vhost net virtqueue: %d, "
+                            "when resetting the queue", queue_index);
+        }
+    }
+}
+
 static void virtio_net_reset(VirtIODevice *vdev)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
@@ -3781,6 +3800,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
     vdc->bad_features = virtio_net_bad_features;
     vdc->reset = virtio_net_reset;
     vdc->queue_reset = virtio_net_queue_reset;
+    vdc->queue_enable = virtio_net_queue_enable;
     vdc->set_status = virtio_net_set_status;
     vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
     vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
-- 
2.32.0

Re: [PATCH 14/16] virtio-net: support queue_enable for vhost-user

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> Support queue enable in vhost-user scenario. It will be called when
> a vq reset operation is performed and the vq will be restared.
>
> It should be noted that we can restart the vq when the vhost has
> already started. When launching a new vhost-user device, the vhost
> is not started and all vqs are not initalized until
> VIRTIO_PCI_COMMON_STATUS is written. Thus, we should use vhost_started
> to differentiate the two cases: vq reset and device start.
>
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   hw/net/virtio-net.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 8396e21a67..2c26e2ef73 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -544,6 +544,25 @@ static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
>       assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
>   }
>   
> +static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> +{
> +    VirtIONet *n = VIRTIO_NET(vdev);
> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> +    int r;
> +
> +    if (!nc->peer || !vdev->vhost_started) {
> +        return;
> +    }
> +
> +    if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> +        r = vhost_virtqueue_restart(vdev, nc, queue_index);
> +        if (r < 0) {
> +            error_report("unable to restart vhost net virtqueue: %d, "
> +                            "when resetting the queue", queue_index);
> +        }
> +    }
> +}


So we don't check queue_enable in vhost_dev_start(), does this mean the 
queue_enable is actually meaningless (since the virtqueue is already 
started there)?

And another issue is

peet_attach/peer_detach() "abuse" vhost_set_vring_enable(). This 
probably means we need to invent new type of request instead of re-using 
VHOST_USER_SET_VRING_ENABLE.

Thanks


> +
>   static void virtio_net_reset(VirtIODevice *vdev)
>   {
>       VirtIONet *n = VIRTIO_NET(vdev);
> @@ -3781,6 +3800,7 @@ static void virtio_net_class_init(ObjectClass *klass, void *data)
>       vdc->bad_features = virtio_net_bad_features;
>       vdc->reset = virtio_net_reset;
>       vdc->queue_reset = virtio_net_queue_reset;
> +    vdc->queue_enable = virtio_net_queue_enable;
>       vdc->set_status = virtio_net_set_status;
>       vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
>       vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;

Re: [PATCH 14/16] virtio-net: support queue_enable for vhost-user

[Kangjie Xu]

在 2022/7/26 12:25, Jason Wang 写道:
>
> 在 2022/7/18 19:17, Kangjie Xu 写道:
>> Support queue enable in vhost-user scenario. It will be called when
>> a vq reset operation is performed and the vq will be restared.
>>
>> It should be noted that we can restart the vq when the vhost has
>> already started. When launching a new vhost-user device, the vhost
>> is not started and all vqs are not initalized until
>> VIRTIO_PCI_COMMON_STATUS is written. Thus, we should use vhost_started
>> to differentiate the two cases: vq reset and device start.
>>
>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> ---
>>   hw/net/virtio-net.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 8396e21a67..2c26e2ef73 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -544,6 +544,25 @@ static void virtio_net_queue_reset(VirtIODevice 
>> *vdev, uint32_t queue_index)
>>       assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
>>   }
>>   +static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t 
>> queue_index)
>> +{
>> +    VirtIONet *n = VIRTIO_NET(vdev);
>> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
>> +    int r;
>> +
>> +    if (!nc->peer || !vdev->vhost_started) {
>> +        return;
>> +    }
>> +
>> +    if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
>> +        r = vhost_virtqueue_restart(vdev, nc, queue_index);
>> +        if (r < 0) {
>> +            error_report("unable to restart vhost net virtqueue: %d, "
>> +                            "when resetting the queue", queue_index);
>> +        }
>> +    }
>> +}
>
>
> So we don't check queue_enable in vhost_dev_start(), does this mean 
> the queue_enable is actually meaningless (since the virtqueue is 
> already started there)?
>
> And another issue is
>
> peet_attach/peer_detach() "abuse" vhost_set_vring_enable(). This 
> probably means we need to invent new type of request instead of 
> re-using VHOST_USER_SET_VRING_ENABLE.
>
> Thanks
>
>
1. Yes, we don't need queue_enable in vhost_dev_start(), queue_enable is 
only useful when restarting the queue. I name it as queue_enable() 
simply because it is called when VIRTIO_PCI_COMMON_Q_ENABLE is written. 
Would it look better if we rename it as "queue_reenable"?

2. I think inventing a new type of vhost-protocol message can be a good 
choice. However, I don't know much about the vhost protocol. If we want 
to add a new message in vhost protocol, except the documentation and the 
code in qemu, Do we need to submit patches to other projects, e.g. some 
projects like virtio-spec?

Thanks

>> +
>>   static void virtio_net_reset(VirtIODevice *vdev)
>>   {
>>       VirtIONet *n = VIRTIO_NET(vdev);
>> @@ -3781,6 +3800,7 @@ static void virtio_net_class_init(ObjectClass 
>> *klass, void *data)
>>       vdc->bad_features = virtio_net_bad_features;
>>       vdc->reset = virtio_net_reset;
>>       vdc->queue_reset = virtio_net_queue_reset;
>> +    vdc->queue_enable = virtio_net_queue_enable;
>>       vdc->set_status = virtio_net_set_status;
>>       vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
>>       vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;

Re: [PATCH 14/16] virtio-net: support queue_enable for vhost-user

[Jason Wang]

On Tue, Jul 26, 2022 at 2:54 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/7/26 12:25, Jason Wang 写道:
> >
> > 在 2022/7/18 19:17, Kangjie Xu 写道:
> >> Support queue enable in vhost-user scenario. It will be called when
> >> a vq reset operation is performed and the vq will be restared.
> >>
> >> It should be noted that we can restart the vq when the vhost has
> >> already started. When launching a new vhost-user device, the vhost
> >> is not started and all vqs are not initalized until
> >> VIRTIO_PCI_COMMON_STATUS is written. Thus, we should use vhost_started
> >> to differentiate the two cases: vq reset and device start.
> >>
> >> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> >> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >> ---
> >>   hw/net/virtio-net.c | 20 ++++++++++++++++++++
> >>   1 file changed, 20 insertions(+)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 8396e21a67..2c26e2ef73 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -544,6 +544,25 @@ static void virtio_net_queue_reset(VirtIODevice
> >> *vdev, uint32_t queue_index)
> >>       assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
> >>   }
> >>   +static void virtio_net_queue_enable(VirtIODevice *vdev, uint32_t
> >> queue_index)
> >> +{
> >> +    VirtIONet *n = VIRTIO_NET(vdev);
> >> +    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(queue_index));
> >> +    int r;
> >> +
> >> +    if (!nc->peer || !vdev->vhost_started) {
> >> +        return;
> >> +    }
> >> +
> >> +    if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> >> +        r = vhost_virtqueue_restart(vdev, nc, queue_index);
> >> +        if (r < 0) {
> >> +            error_report("unable to restart vhost net virtqueue: %d, "
> >> +                            "when resetting the queue", queue_index);
> >> +        }
> >> +    }
> >> +}
> >
> >
> > So we don't check queue_enable in vhost_dev_start(), does this mean
> > the queue_enable is actually meaningless (since the virtqueue is
> > already started there)?
> >
> > And another issue is
> >
> > peet_attach/peer_detach() "abuse" vhost_set_vring_enable(). This
> > probably means we need to invent new type of request instead of
> > re-using VHOST_USER_SET_VRING_ENABLE.
> >
> > Thanks
> >
> >
> 1. Yes, we don't need queue_enable in vhost_dev_start(), queue_enable is
> only useful when restarting the queue. I name it as queue_enable()
> simply because it is called when VIRTIO_PCI_COMMON_Q_ENABLE is written.
> Would it look better if we rename it as "queue_reenable"?

I think the right approach is probably:

1) when VERSION_1 is negotiated, only start the virtqueue when queue_enable is 1
2) when VERSION_1 is not negotiated, start virtqueue when DRIVER_OK
(vhost_dev_start())

?

>
> 2. I think inventing a new type of vhost-protocol message can be a good
> choice. However, I don't know much about the vhost protocol. If we want
> to add a new message in vhost protocol, except the documentation and the
> code in qemu, Do we need to submit patches to other projects, e.g. some
> projects like virtio-spec?

Probably not since vhost-user doesn't belong to the spec currently.
The doc in qemu should be sufficient.

Thanks

>
> Thanks
>
> >> +
> >>   static void virtio_net_reset(VirtIODevice *vdev)
> >>   {
> >>       VirtIONet *n = VIRTIO_NET(vdev);
> >> @@ -3781,6 +3800,7 @@ static void virtio_net_class_init(ObjectClass
> >> *klass, void *data)
> >>       vdc->bad_features = virtio_net_bad_features;
> >>       vdc->reset = virtio_net_reset;
> >>       vdc->queue_reset = virtio_net_queue_reset;
> >> +    vdc->queue_enable = virtio_net_queue_enable;
> >>       vdc->set_status = virtio_net_set_status;
> >>       vdc->guest_notifier_mask = virtio_net_guest_notifier_mask;
> >>       vdc->guest_notifier_pending = virtio_net_guest_notifier_pending;
>

[PATCH 15/16] virtio-net: support queue_reset for vhost-user

[Kangjie Xu]

Support queue reset in vhost-user scenario.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/net/virtio-net.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 2c26e2ef73..0747ffe71c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -540,6 +540,10 @@ static void virtio_net_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
         return;
     }
 
+    if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
+        vhost_virtqueue_stop(vdev, nc, queue_index);
+    }
+
     qemu_flush_or_purge_queued_packets(nc->peer, true);
     assert(!virtio_net_get_subqueue(nc)->async_tx.elem);
 }
-- 
2.32.0

[PATCH 16/16] vhost-net: vq reset feature bit support

[Kangjie Xu]

Add support for negotation of vq reset feature bit.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/net/vhost_net.c  | 1 +
 hw/net/virtio-net.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 4f5f034c11..de910f6466 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -73,6 +73,7 @@ static const int user_feature_bits[] = {
     VIRTIO_NET_F_MTU,
     VIRTIO_F_IOMMU_PLATFORM,
     VIRTIO_F_RING_PACKED,
+    VIRTIO_F_RING_RESET,
     VIRTIO_NET_F_RSS,
     VIRTIO_NET_F_HASH_REPORT,
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 0747ffe71c..a8b299067a 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -757,6 +757,8 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
 
     virtio_add_feature(&features, VIRTIO_NET_F_MAC);
 
+    virtio_add_feature(&features, VIRTIO_F_RING_RESET);
+
     if (!peer_has_vnet_hdr(n)) {
         virtio_clear_feature(&features, VIRTIO_NET_F_CSUM);
         virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO4);
@@ -777,7 +779,6 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
     }
 
     if (!get_vhost_net(nc->peer)) {
-        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
         return features;
     }
 
-- 
2.32.0

Re: [PATCH 16/16] vhost-net: vq reset feature bit support

[Jason Wang]

在 2022/7/18 19:17, Kangjie Xu 写道:
> Add support for negotation of vq reset feature bit.
>
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


I'd suggest to add support for vhost-net kernel as well. It looks much 
more easier than vhost-user (I guess a stop/start would do the trick).

Thanks


> ---
>   hw/net/vhost_net.c  | 1 +
>   hw/net/virtio-net.c | 3 ++-
>   2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 4f5f034c11..de910f6466 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -73,6 +73,7 @@ static const int user_feature_bits[] = {
>       VIRTIO_NET_F_MTU,
>       VIRTIO_F_IOMMU_PLATFORM,
>       VIRTIO_F_RING_PACKED,
> +    VIRTIO_F_RING_RESET,
>       VIRTIO_NET_F_RSS,
>       VIRTIO_NET_F_HASH_REPORT,
>   
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 0747ffe71c..a8b299067a 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -757,6 +757,8 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>   
>       virtio_add_feature(&features, VIRTIO_NET_F_MAC);
>   
> +    virtio_add_feature(&features, VIRTIO_F_RING_RESET);
> +
>       if (!peer_has_vnet_hdr(n)) {
>           virtio_clear_feature(&features, VIRTIO_NET_F_CSUM);
>           virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO4);
> @@ -777,7 +779,6 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>       }
>   
>       if (!get_vhost_net(nc->peer)) {
> -        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
>           return features;
>       }
>   

Re: [PATCH 16/16] vhost-net: vq reset feature bit support

[Kangjie Xu]

在 2022/7/26 12:28, Jason Wang 写道:
>
> 在 2022/7/18 19:17, Kangjie Xu 写道:
>> Add support for negotation of vq reset feature bit.
>>
>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
>
> I'd suggest to add support for vhost-net kernel as well. It looks much 
> more easier than vhost-user (I guess a stop/start would do the trick).
>
> Thanks
>
>
Yeah, we've planned to support it in the future.

Thanks

>> ---
>>   hw/net/vhost_net.c  | 1 +
>>   hw/net/virtio-net.c | 3 ++-
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index 4f5f034c11..de910f6466 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -73,6 +73,7 @@ static const int user_feature_bits[] = {
>>       VIRTIO_NET_F_MTU,
>>       VIRTIO_F_IOMMU_PLATFORM,
>>       VIRTIO_F_RING_PACKED,
>> +    VIRTIO_F_RING_RESET,
>>       VIRTIO_NET_F_RSS,
>>       VIRTIO_NET_F_HASH_REPORT,
>>   diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 0747ffe71c..a8b299067a 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -757,6 +757,8 @@ static uint64_t 
>> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>         virtio_add_feature(&features, VIRTIO_NET_F_MAC);
>>   +    virtio_add_feature(&features, VIRTIO_F_RING_RESET);
>> +
>>       if (!peer_has_vnet_hdr(n)) {
>>           virtio_clear_feature(&features, VIRTIO_NET_F_CSUM);
>>           virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO4);
>> @@ -777,7 +779,6 @@ static uint64_t 
>> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>       }
>>         if (!get_vhost_net(nc->peer)) {
>> -        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
>>           return features;
>>       }

Re: [PATCH 16/16] vhost-net: vq reset feature bit support

[Jason Wang]

On Tue, Jul 26, 2022 at 2:24 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>
>
> 在 2022/7/26 12:28, Jason Wang 写道:
> >
> > 在 2022/7/18 19:17, Kangjie Xu 写道:
> >> Add support for negotation of vq reset feature bit.
> >>
> >> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> >> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >
> >
> > I'd suggest to add support for vhost-net kernel as well. It looks much
> > more easier than vhost-user (I guess a stop/start would do the trick).
> >
> > Thanks
> >
> >
> Yeah, we've planned to support it in the future.

If it's possible, I suggest to implement in this series. It would be
easier since it current kernel support it already (via SET_BACKEND).

Thanks

>
> Thanks
>
> >> ---
> >>   hw/net/vhost_net.c  | 1 +
> >>   hw/net/virtio-net.c | 3 ++-
> >>   2 files changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >> index 4f5f034c11..de910f6466 100644
> >> --- a/hw/net/vhost_net.c
> >> +++ b/hw/net/vhost_net.c
> >> @@ -73,6 +73,7 @@ static const int user_feature_bits[] = {
> >>       VIRTIO_NET_F_MTU,
> >>       VIRTIO_F_IOMMU_PLATFORM,
> >>       VIRTIO_F_RING_PACKED,
> >> +    VIRTIO_F_RING_RESET,
> >>       VIRTIO_NET_F_RSS,
> >>       VIRTIO_NET_F_HASH_REPORT,
> >>   diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 0747ffe71c..a8b299067a 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -757,6 +757,8 @@ static uint64_t
> >> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> >>         virtio_add_feature(&features, VIRTIO_NET_F_MAC);
> >>   +    virtio_add_feature(&features, VIRTIO_F_RING_RESET);
> >> +
> >>       if (!peer_has_vnet_hdr(n)) {
> >>           virtio_clear_feature(&features, VIRTIO_NET_F_CSUM);
> >>           virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO4);
> >> @@ -777,7 +779,6 @@ static uint64_t
> >> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
> >>       }
> >>         if (!get_vhost_net(nc->peer)) {
> >> -        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
> >>           return features;
> >>       }
>

Re: [PATCH 16/16] vhost-net: vq reset feature bit support

[Kangjie Xu]

在 2022/7/27 12:53, Jason Wang 写道:
> On Tue, Jul 26, 2022 at 2:24 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
>>
>> 在 2022/7/26 12:28, Jason Wang 写道:
>>> 在 2022/7/18 19:17, Kangjie Xu 写道:
>>>> Add support for negotation of vq reset feature bit.
>>>>
>>>> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>>
>>> I'd suggest to add support for vhost-net kernel as well. It looks much
>>> more easier than vhost-user (I guess a stop/start would do the trick).
>>>
>>> Thanks
>>>
>>>
>> Yeah, we've planned to support it in the future.
> If it's possible, I suggest to implement in this series. It would be
> easier since it current kernel support it already (via SET_BACKEND).
>
> Thanks

Okay, in the next version, we will add the implementation for 
vhost-kernel vq reset.

Thanks

>> Thanks
>>
>>>> ---
>>>>    hw/net/vhost_net.c  | 1 +
>>>>    hw/net/virtio-net.c | 3 ++-
>>>>    2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>> index 4f5f034c11..de910f6466 100644
>>>> --- a/hw/net/vhost_net.c
>>>> +++ b/hw/net/vhost_net.c
>>>> @@ -73,6 +73,7 @@ static const int user_feature_bits[] = {
>>>>        VIRTIO_NET_F_MTU,
>>>>        VIRTIO_F_IOMMU_PLATFORM,
>>>>        VIRTIO_F_RING_PACKED,
>>>> +    VIRTIO_F_RING_RESET,
>>>>        VIRTIO_NET_F_RSS,
>>>>        VIRTIO_NET_F_HASH_REPORT,
>>>>    diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 0747ffe71c..a8b299067a 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -757,6 +757,8 @@ static uint64_t
>>>> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>>>          virtio_add_feature(&features, VIRTIO_NET_F_MAC);
>>>>    +    virtio_add_feature(&features, VIRTIO_F_RING_RESET);
>>>> +
>>>>        if (!peer_has_vnet_hdr(n)) {
>>>>            virtio_clear_feature(&features, VIRTIO_NET_F_CSUM);
>>>>            virtio_clear_feature(&features, VIRTIO_NET_F_HOST_TSO4);
>>>> @@ -777,7 +779,6 @@ static uint64_t
>>>> virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>>>>        }
>>>>          if (!get_vhost_net(nc->peer)) {
>>>> -        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
>>>>            return features;
>>>>        }

Re: [PATCH 00/16] Support VIRTIO_F_RING_RESET for virtio-net and vhost-user in virtio pci

[Kangjie Xu]

Do you have any comments or suggestions about improvements to this patch 
set?


Regards,

Kangjie

在 2022/7/18 19:16, Kangjie Xu 写道:
> The virtio queue reset function has already been defined in the virtio spec 1.2.
> The relevant virtio spec information is here:
>
>      https://github.com/oasis-tcs/virtio-spec/issues/124
>      https://github.com/oasis-tcs/virtio-spec/issues/139
>
> This patch set is to support this function in QEMU. It consists of two parts: virtio-net
> and vhost-user. The patches 1-7 are the implementation for virtio-net and the patches
> 8-16 are for vhost-user.
>
> The process of virt queue reset can be concluded as:
> 1. The virtqueue is disabled when VIRTIO_PCI_COMMON_Q_RESET is written.
> 2. Then the virtqueue is restarted after the information of vrings is passed to QEMU and
> VIRTIO_PCI_COMMON_Q_ENABLE is written.
>
> Test environment:
>      Host: 5.4.189
>      Qemu: QEMU emulator version 7.0.50
>      Guest: 5.19.0-rc3 (With vq reset support)
>      DPDK: 22.07-rc1 (With vq reset support)
>      Test Cmd: ethtool -g eth1; ethtool -G eth1 rx $1 tx $2; ethtool -g eth1;
>
>      The frontend can resize the virtio queue, then virtio queue reset function should
>      be triggered.
>
>      The default is split mode, modify Qemu virtio-net to add PACKED feature to
>      test packed mode.
>
> Guest Kernel Patch:
>      https://lore.kernel.org/bpf/20220629065656.54420-1-xuanzhuo@linux.alibaba.com/
>
> DPDK Code:
>      https://github.com/middaywords/dpdk/commit/098c8e1dfae10b747da8dd8950a82890efca7bee
>
> Please review. Thanks.
>
> Kangjie Xu (9):
>    vhost: add op to enable or disable a single vring
>    vhost-user: enable/disable a single vring
>    vhost: extract the logic of unmapping the vrings and desc
>    vhost: introduce restart and release for vhost_dev's vqs
>    vhost-net: introduce restart and stop for vhost_net's vqs
>    virtio: introduce queue_enable in virtio
>    virtio-net: support queue_enable for vhost-user
>    virtio-net: support queue_reset for vhost-user
>    vhost-net: vq reset feature bit support
>
> Xuan Zhuo (7):
>    virtio-pci: virtio_pci_common_cfg add queue_notify_data
>    virtio: add VIRTIO_F_RING_RESET
>    virtio: pci: virtio_pci_common_cfg add queue_reset
>    virtio: introduce __virtio_queue_reset()
>    virtio: introduce virtio_queue_reset()
>    virtio-pci: support queue reset
>    virtio-net: support queue reset
>
>   hw/net/vhost_net.c                            | 56 ++++++++++++++++++
>   hw/net/virtio-net.c                           | 40 +++++++++++++
>   hw/virtio/vhost-user.c                        | 55 +++++++++++++++---
>   hw/virtio/vhost.c                             | 49 ++++++++++++++--
>   hw/virtio/virtio-pci.c                        | 17 ++++++
>   hw/virtio/virtio.c                            | 57 +++++++++++++------
>   include/hw/virtio/vhost-backend.h             |  4 ++
>   include/hw/virtio/vhost.h                     |  6 ++
>   include/hw/virtio/virtio-pci.h                |  1 +
>   include/hw/virtio/virtio.h                    |  4 ++
>   include/net/vhost_net.h                       |  5 ++
>   .../standard-headers/linux/virtio_config.h    |  5 ++
>   include/standard-headers/linux/virtio_pci.h   |  4 ++
>   13 files changed, 274 insertions(+), 29 deletions(-)
>

Re: [PATCH 00/16] Support VIRTIO_F_RING_RESET for virtio-net and vhost-user in virtio pci

[Jason Wang]

On Mon, Jul 25, 2022 at 10:34 AM Kangjie Xu
<kangjie.xu@linux.alibaba.com> wrote:
>
> Do you have any comments or suggestions about improvements to this patch
> set?

Will have a look and give feedback no later than the end of this week.

Thanks

>
>
> Regards,
>
> Kangjie
>
> 在 2022/7/18 19:16, Kangjie Xu 写道:
> > The virtio queue reset function has already been defined in the virtio spec 1.2.
> > The relevant virtio spec information is here:
> >
> >      https://github.com/oasis-tcs/virtio-spec/issues/124
> >      https://github.com/oasis-tcs/virtio-spec/issues/139
> >
> > This patch set is to support this function in QEMU. It consists of two parts: virtio-net
> > and vhost-user. The patches 1-7 are the implementation for virtio-net and the patches
> > 8-16 are for vhost-user.
> >
> > The process of virt queue reset can be concluded as:
> > 1. The virtqueue is disabled when VIRTIO_PCI_COMMON_Q_RESET is written.
> > 2. Then the virtqueue is restarted after the information of vrings is passed to QEMU and
> > VIRTIO_PCI_COMMON_Q_ENABLE is written.
> >
> > Test environment:
> >      Host: 5.4.189
> >      Qemu: QEMU emulator version 7.0.50
> >      Guest: 5.19.0-rc3 (With vq reset support)
> >      DPDK: 22.07-rc1 (With vq reset support)
> >      Test Cmd: ethtool -g eth1; ethtool -G eth1 rx $1 tx $2; ethtool -g eth1;
> >
> >      The frontend can resize the virtio queue, then virtio queue reset function should
> >      be triggered.
> >
> >      The default is split mode, modify Qemu virtio-net to add PACKED feature to
> >      test packed mode.
> >
> > Guest Kernel Patch:
> >      https://lore.kernel.org/bpf/20220629065656.54420-1-xuanzhuo@linux.alibaba.com/
> >
> > DPDK Code:
> >      https://github.com/middaywords/dpdk/commit/098c8e1dfae10b747da8dd8950a82890efca7bee
> >
> > Please review. Thanks.
> >
> > Kangjie Xu (9):
> >    vhost: add op to enable or disable a single vring
> >    vhost-user: enable/disable a single vring
> >    vhost: extract the logic of unmapping the vrings and desc
> >    vhost: introduce restart and release for vhost_dev's vqs
> >    vhost-net: introduce restart and stop for vhost_net's vqs
> >    virtio: introduce queue_enable in virtio
> >    virtio-net: support queue_enable for vhost-user
> >    virtio-net: support queue_reset for vhost-user
> >    vhost-net: vq reset feature bit support
> >
> > Xuan Zhuo (7):
> >    virtio-pci: virtio_pci_common_cfg add queue_notify_data
> >    virtio: add VIRTIO_F_RING_RESET
> >    virtio: pci: virtio_pci_common_cfg add queue_reset
> >    virtio: introduce __virtio_queue_reset()
> >    virtio: introduce virtio_queue_reset()
> >    virtio-pci: support queue reset
> >    virtio-net: support queue reset
> >
> >   hw/net/vhost_net.c                            | 56 ++++++++++++++++++
> >   hw/net/virtio-net.c                           | 40 +++++++++++++
> >   hw/virtio/vhost-user.c                        | 55 +++++++++++++++---
> >   hw/virtio/vhost.c                             | 49 ++++++++++++++--
> >   hw/virtio/virtio-pci.c                        | 17 ++++++
> >   hw/virtio/virtio.c                            | 57 +++++++++++++------
> >   include/hw/virtio/vhost-backend.h             |  4 ++
> >   include/hw/virtio/vhost.h                     |  6 ++
> >   include/hw/virtio/virtio-pci.h                |  1 +
> >   include/hw/virtio/virtio.h                    |  4 ++
> >   include/net/vhost_net.h                       |  5 ++
> >   .../standard-headers/linux/virtio_config.h    |  5 ++
> >   include/standard-headers/linux/virtio_pci.h   |  4 ++
> >   13 files changed, 274 insertions(+), 29 deletions(-)
> >
>