[PATCH 0/3] virtio: fix for assertion failure: virtio_net_get_subqueue(nc)->async_tx.elem failed

[PATCH 0/3] virtio: fix for assertion failure: virtio_net_get_subqueue(nc)->async_tx.elem failed

[Xuan Zhuo]

In the current design, we stop the device from operating on the vring
during per-queue reset by resetting the structure VirtQueue.

But before the reset operation, when recycling some resources, we should
stop referencing new vring resources.

This bug is caused by this reason.

    https://gitlab.com/qemu-project/qemu/-/issues/1451

Before we reset the structure, we called the ->queue_reset callback to let the
device reclaim resources. Here virtio-net tries to release the packets sent
asynchronously, but during this process virtio_net_flush_tx() will be called,
and new data will be sent again. This leads to asserted.

     assert(!virtio_net_get_subqueue(nc)->async_tx.elem);

This patch set introduce new item "reset" into struct VirtQueue, then device can
know this virtqueue is per-queue reset state.

Xuan Zhuo (3):
  virtio: move struct VirtQueue to include file
  virtio: struct VirtQueue introduce reset
  virtio-net: virtio_net_flush_tx() check for per-queue reset

 hw/net/virtio-net.c        |  2 +-
 hw/virtio/virtio.c         | 57 ++++++--------------------------------
 include/hw/virtio/virtio.h | 55 ++++++++++++++++++++++++++++++++++--
 3 files changed, 62 insertions(+), 52 deletions(-)

--
2.32.0.3.g01195cf9f

[PATCH 1/3] virtio: move struct VirtQueue to include file

[Xuan Zhuo]

This patch move struct VirtQueue into virtio.h.

In order to implement Queue Reset, we have to record the queue reset
status of in struct VirtQueue and provide it to device.

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f35178f5fc..03077b2ecf 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -101,60 +101,11 @@ typedef struct VRingMemoryRegionCaches {
     MemoryRegionCache used;
 } VRingMemoryRegionCaches;
 
-typedef struct VRing
-{
-    unsigned int num;
-    unsigned int num_default;
-    unsigned int align;
-    hwaddr desc;
-    hwaddr avail;
-    hwaddr used;
-    VRingMemoryRegionCaches *caches;
-} VRing;
-
 typedef struct VRingPackedDescEvent {
     uint16_t off_wrap;
     uint16_t flags;
 } VRingPackedDescEvent ;
 
-struct VirtQueue
-{
-    VRing vring;
-    VirtQueueElement *used_elems;
-
-    /* Next head to pop */
-    uint16_t last_avail_idx;
-    bool last_avail_wrap_counter;
-
-    /* Last avail_idx read from VQ. */
-    uint16_t shadow_avail_idx;
-    bool shadow_avail_wrap_counter;
-
-    uint16_t used_idx;
-    bool used_wrap_counter;
-
-    /* Last used index value we have signalled on */
-    uint16_t signalled_used;
-
-    /* Last used index value we have signalled on */
-    bool signalled_used_valid;
-
-    /* Notification enabled? */
-    bool notification;
-
-    uint16_t queue_index;
-
-    unsigned int inuse;
-
-    uint16_t vector;
-    VirtIOHandleOutput handle_output;
-    VirtIODevice *vdev;
-    EventNotifier guest_notifier;
-    EventNotifier host_notifier;
-    bool host_notifier_enabled;
-    QLIST_ENTRY(VirtQueue) node;
-};
-
 const char *virtio_device_names[] = {
     [VIRTIO_ID_NET] = "virtio-net",
     [VIRTIO_ID_BLOCK] = "virtio-blk",
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 77c6c55929..1c0d77c670 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -214,6 +214,56 @@ struct VirtioDeviceClass {
     struct vhost_dev *(*get_vhost)(VirtIODevice *vdev);
 };
 
+typedef struct VRingMemoryRegionCaches VRingMemoryRegionCaches;
+typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
+
+typedef struct VRing {
+    unsigned int num;
+    unsigned int num_default;
+    unsigned int align;
+    hwaddr desc;
+    hwaddr avail;
+    hwaddr used;
+    VRingMemoryRegionCaches *caches;
+} VRing;
+
+struct VirtQueue {
+    VRing vring;
+    VirtQueueElement *used_elems;
+
+    /* Next head to pop */
+    uint16_t last_avail_idx;
+    bool last_avail_wrap_counter;
+
+    /* Last avail_idx read from VQ. */
+    uint16_t shadow_avail_idx;
+    bool shadow_avail_wrap_counter;
+
+    uint16_t used_idx;
+    bool used_wrap_counter;
+
+    /* Last used index value we have signalled on */
+    uint16_t signalled_used;
+
+    /* Last used index value we have signalled on */
+    bool signalled_used_valid;
+
+    /* Notification enabled? */
+    bool notification;
+
+    uint16_t queue_index;
+
+    unsigned int inuse;
+
+    uint16_t vector;
+    VirtIOHandleOutput handle_output;
+    VirtIODevice *vdev;
+    EventNotifier guest_notifier;
+    EventNotifier host_notifier;
+    bool host_notifier_enabled;
+    QLIST_ENTRY(VirtQueue) node;
+};
+
 void virtio_instance_init_common(Object *proxy_obj, void *data,
                                  size_t vdev_size, const char *vdev_name);
 
@@ -226,8 +276,6 @@ void virtio_error(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 /* Set the child bus name. */
 void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
 
-typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
-
 VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
                             VirtIOHandleOutput handle_output);
 
-- 
2.32.0.3.g01195cf9f

[PATCH 2/3] virtio: struct VirtQueue introduce reset

[Xuan Zhuo]

 In the current design, we stop the device from operating on the vring
 during per-queue reset by resetting the structure VirtQueue.

 But before the reset operation, when recycling some resources, we should
 stop referencing new vring resources. For example, when recycling
 virtio-net's asynchronous sending resources, virtio-net should be able
 to perceive that the current queue is in the per-queue reset state, and
 stop sending new packets from the tx queue.

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

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 03077b2ecf..907d5b8bde 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2030,6 +2030,12 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
 {
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
 
+    /*
+     * Mark this queue is per-queue reset status. The device should release the
+     * references of the vring, and not refer more new vring item.
+     */
+    vdev->vq[queue_index].reset = true;
+
     if (k->queue_reset) {
         k->queue_reset(vdev, queue_index);
     }
@@ -2053,6 +2059,8 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
     }
     */
 
+    vdev->vq[queue_index].reset = false;
+
     if (k->queue_enable) {
         k->queue_enable(vdev, queue_index);
     }
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 1c0d77c670..b888538d09 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -251,6 +251,9 @@ struct VirtQueue {
     /* Notification enabled? */
     bool notification;
 
+    /* Per-Queue Reset status */
+    bool reset;
+
     uint16_t queue_index;
 
     unsigned int inuse;
-- 
2.32.0.3.g01195cf9f

[PATCH 3/3] virtio-net: virtio_net_flush_tx() check for per-queue reset

[Xuan Zhuo]

Check whether it is per-queue reset state in virtio_net_flush_tx().

Before per-queue reset, we need to recover async tx resources. At this
time, virtio_net_flush_tx() is called, but we should not try to send
new packets, so virtio_net_flush_tx() should check the current
per-queue reset state.

Fixes: 7dc6be52 ("virtio-net: support queue reset")
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1451
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/net/virtio-net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3ae909041a..e7e651e915 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2627,7 +2627,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
     VirtQueueElement *elem;
     int32_t num_packets = 0;
     int queue_index = vq2q(virtio_get_queue_index(q->tx_vq));
-    if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+    if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || q->tx_vq->reset) {
         return num_packets;
     }
 
-- 
2.32.0.3.g01195cf9f

Re: [PATCH 2/3] virtio: struct VirtQueue introduce reset

[Michael S. Tsirkin]

On Sat, Jan 28, 2023 at 03:17:23PM +0800, Xuan Zhuo wrote:
>  In the current design, we stop the device from operating on the vring
>  during per-queue reset by resetting the structure VirtQueue.
> 
>  But before the reset operation, when recycling some resources, we should
>  stop referencing new vring resources. For example, when recycling
>  virtio-net's asynchronous sending resources, virtio-net should be able
>  to perceive that the current queue is in the per-queue reset state, and
>  stop sending new packets from the tx queue.
> 
>  Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  hw/virtio/virtio.c         | 8 ++++++++
>  include/hw/virtio/virtio.h | 3 +++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 03077b2ecf..907d5b8bde 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2030,6 +2030,12 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
>  {
>      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
>  
> +    /*
> +     * Mark this queue is per-queue reset status. The device should release the
> +     * references of the vring, and not refer more new vring item.
> +     */
> +    vdev->vq[queue_index].reset = true;
> +
>      if (k->queue_reset) {
>          k->queue_reset(vdev, queue_index);
>      }
> @@ -2053,6 +2059,8 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
>      }
>      */
>  
> +    vdev->vq[queue_index].reset = false;
> +
>      if (k->queue_enable) {
>          k->queue_enable(vdev, queue_index);
>      }
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 1c0d77c670..b888538d09 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -251,6 +251,9 @@ struct VirtQueue {
>      /* Notification enabled? */
>      bool notification;
>  
> +    /* Per-Queue Reset status */
> +    bool reset;
> +
>      uint16_t queue_index;
>  

Reset state makes no sense. It seems to imply queue_reset
in the spec. And for extra fun there's "reset" in the pci
proxy which means "virtio_queue_reset is in progress" - I have no
idea what uses it though - it is not guest visible.  First what is it?
It actually means "queue has been reset and not has not been enabled since".
So disabled_by_reset maybe?

Second this hack helps make the change minimal
so it's helpful for stable, but it's ugly in that it
duplicates the reverse of enabled value - we don't really
care what disabled it in practice.

With the fixups above I can apply so it's easier to backport, but later
a patch on top should clean it all up, perhaps by adding
"enabled" in VirtQueue. We should also get rid of "reset" in the proxy
unless there's some way it's useful which I don't currently see.



>      unsigned int inuse;
> -- 
> 2.32.0.3.g01195cf9f

Re: [PATCH 1/3] virtio: move struct VirtQueue to include file

[Michael S. Tsirkin]

On Sat, Jan 28, 2023 at 03:17:22PM +0800, Xuan Zhuo wrote:
> This patch move struct VirtQueue into virtio.h.
> 
> In order to implement Queue Reset, we have to record the queue reset
> status of in struct VirtQueue and provide it to device.
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

So add an API please, no need to move the struct.
This patch will go away then.

> ---
>  hw/virtio/virtio.c         | 49 -----------------------------------
>  include/hw/virtio/virtio.h | 52 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 50 insertions(+), 51 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f35178f5fc..03077b2ecf 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -101,60 +101,11 @@ typedef struct VRingMemoryRegionCaches {
>      MemoryRegionCache used;
>  } VRingMemoryRegionCaches;
>  
> -typedef struct VRing
> -{
> -    unsigned int num;
> -    unsigned int num_default;
> -    unsigned int align;
> -    hwaddr desc;
> -    hwaddr avail;
> -    hwaddr used;
> -    VRingMemoryRegionCaches *caches;
> -} VRing;
> -
>  typedef struct VRingPackedDescEvent {
>      uint16_t off_wrap;
>      uint16_t flags;
>  } VRingPackedDescEvent ;
>  
> -struct VirtQueue
> -{
> -    VRing vring;
> -    VirtQueueElement *used_elems;
> -
> -    /* Next head to pop */
> -    uint16_t last_avail_idx;
> -    bool last_avail_wrap_counter;
> -
> -    /* Last avail_idx read from VQ. */
> -    uint16_t shadow_avail_idx;
> -    bool shadow_avail_wrap_counter;
> -
> -    uint16_t used_idx;
> -    bool used_wrap_counter;
> -
> -    /* Last used index value we have signalled on */
> -    uint16_t signalled_used;
> -
> -    /* Last used index value we have signalled on */
> -    bool signalled_used_valid;
> -
> -    /* Notification enabled? */
> -    bool notification;
> -
> -    uint16_t queue_index;
> -
> -    unsigned int inuse;
> -
> -    uint16_t vector;
> -    VirtIOHandleOutput handle_output;
> -    VirtIODevice *vdev;
> -    EventNotifier guest_notifier;
> -    EventNotifier host_notifier;
> -    bool host_notifier_enabled;
> -    QLIST_ENTRY(VirtQueue) node;
> -};
> -
>  const char *virtio_device_names[] = {
>      [VIRTIO_ID_NET] = "virtio-net",
>      [VIRTIO_ID_BLOCK] = "virtio-blk",
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 77c6c55929..1c0d77c670 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -214,6 +214,56 @@ struct VirtioDeviceClass {
>      struct vhost_dev *(*get_vhost)(VirtIODevice *vdev);
>  };
>  
> +typedef struct VRingMemoryRegionCaches VRingMemoryRegionCaches;
> +typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
> +
> +typedef struct VRing {
> +    unsigned int num;
> +    unsigned int num_default;
> +    unsigned int align;
> +    hwaddr desc;
> +    hwaddr avail;
> +    hwaddr used;
> +    VRingMemoryRegionCaches *caches;
> +} VRing;
> +
> +struct VirtQueue {
> +    VRing vring;
> +    VirtQueueElement *used_elems;
> +
> +    /* Next head to pop */
> +    uint16_t last_avail_idx;
> +    bool last_avail_wrap_counter;
> +
> +    /* Last avail_idx read from VQ. */
> +    uint16_t shadow_avail_idx;
> +    bool shadow_avail_wrap_counter;
> +
> +    uint16_t used_idx;
> +    bool used_wrap_counter;
> +
> +    /* Last used index value we have signalled on */
> +    uint16_t signalled_used;
> +
> +    /* Last used index value we have signalled on */
> +    bool signalled_used_valid;
> +
> +    /* Notification enabled? */
> +    bool notification;
> +
> +    uint16_t queue_index;
> +
> +    unsigned int inuse;
> +
> +    uint16_t vector;
> +    VirtIOHandleOutput handle_output;
> +    VirtIODevice *vdev;
> +    EventNotifier guest_notifier;
> +    EventNotifier host_notifier;
> +    bool host_notifier_enabled;
> +    QLIST_ENTRY(VirtQueue) node;
> +};
> +
>  void virtio_instance_init_common(Object *proxy_obj, void *data,
>                                   size_t vdev_size, const char *vdev_name);
>  
> @@ -226,8 +276,6 @@ void virtio_error(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
>  /* Set the child bus name. */
>  void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
>  
> -typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
> -
>  VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>                              VirtIOHandleOutput handle_output);
>  
> -- 
> 2.32.0.3.g01195cf9f

Re: [PATCH 1/3] virtio: move struct VirtQueue to include file

[Xuan Zhuo]

On Sat, 28 Jan 2023 05:23:46 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sat, Jan 28, 2023 at 03:17:22PM +0800, Xuan Zhuo wrote:
> > This patch move struct VirtQueue into virtio.h.
> >
> > In order to implement Queue Reset, we have to record the queue reset
> > status of in struct VirtQueue and provide it to device.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>
> So add an API please, no need to move the struct.
> This patch will go away then.

OK.

Thanks.


>
> > ---
> >  hw/virtio/virtio.c         | 49 -----------------------------------
> >  include/hw/virtio/virtio.h | 52 ++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 50 insertions(+), 51 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index f35178f5fc..03077b2ecf 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -101,60 +101,11 @@ typedef struct VRingMemoryRegionCaches {
> >      MemoryRegionCache used;
> >  } VRingMemoryRegionCaches;
> >
> > -typedef struct VRing
> > -{
> > -    unsigned int num;
> > -    unsigned int num_default;
> > -    unsigned int align;
> > -    hwaddr desc;
> > -    hwaddr avail;
> > -    hwaddr used;
> > -    VRingMemoryRegionCaches *caches;
> > -} VRing;
> > -
> >  typedef struct VRingPackedDescEvent {
> >      uint16_t off_wrap;
> >      uint16_t flags;
> >  } VRingPackedDescEvent ;
> >
> > -struct VirtQueue
> > -{
> > -    VRing vring;
> > -    VirtQueueElement *used_elems;
> > -
> > -    /* Next head to pop */
> > -    uint16_t last_avail_idx;
> > -    bool last_avail_wrap_counter;
> > -
> > -    /* Last avail_idx read from VQ. */
> > -    uint16_t shadow_avail_idx;
> > -    bool shadow_avail_wrap_counter;
> > -
> > -    uint16_t used_idx;
> > -    bool used_wrap_counter;
> > -
> > -    /* Last used index value we have signalled on */
> > -    uint16_t signalled_used;
> > -
> > -    /* Last used index value we have signalled on */
> > -    bool signalled_used_valid;
> > -
> > -    /* Notification enabled? */
> > -    bool notification;
> > -
> > -    uint16_t queue_index;
> > -
> > -    unsigned int inuse;
> > -
> > -    uint16_t vector;
> > -    VirtIOHandleOutput handle_output;
> > -    VirtIODevice *vdev;
> > -    EventNotifier guest_notifier;
> > -    EventNotifier host_notifier;
> > -    bool host_notifier_enabled;
> > -    QLIST_ENTRY(VirtQueue) node;
> > -};
> > -
> >  const char *virtio_device_names[] = {
> >      [VIRTIO_ID_NET] = "virtio-net",
> >      [VIRTIO_ID_BLOCK] = "virtio-blk",
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 77c6c55929..1c0d77c670 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -214,6 +214,56 @@ struct VirtioDeviceClass {
> >      struct vhost_dev *(*get_vhost)(VirtIODevice *vdev);
> >  };
> >
> > +typedef struct VRingMemoryRegionCaches VRingMemoryRegionCaches;
> > +typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
> > +
> > +typedef struct VRing {
> > +    unsigned int num;
> > +    unsigned int num_default;
> > +    unsigned int align;
> > +    hwaddr desc;
> > +    hwaddr avail;
> > +    hwaddr used;
> > +    VRingMemoryRegionCaches *caches;
> > +} VRing;
> > +
> > +struct VirtQueue {
> > +    VRing vring;
> > +    VirtQueueElement *used_elems;
> > +
> > +    /* Next head to pop */
> > +    uint16_t last_avail_idx;
> > +    bool last_avail_wrap_counter;
> > +
> > +    /* Last avail_idx read from VQ. */
> > +    uint16_t shadow_avail_idx;
> > +    bool shadow_avail_wrap_counter;
> > +
> > +    uint16_t used_idx;
> > +    bool used_wrap_counter;
> > +
> > +    /* Last used index value we have signalled on */
> > +    uint16_t signalled_used;
> > +
> > +    /* Last used index value we have signalled on */
> > +    bool signalled_used_valid;
> > +
> > +    /* Notification enabled? */
> > +    bool notification;
> > +
> > +    uint16_t queue_index;
> > +
> > +    unsigned int inuse;
> > +
> > +    uint16_t vector;
> > +    VirtIOHandleOutput handle_output;
> > +    VirtIODevice *vdev;
> > +    EventNotifier guest_notifier;
> > +    EventNotifier host_notifier;
> > +    bool host_notifier_enabled;
> > +    QLIST_ENTRY(VirtQueue) node;
> > +};
> > +
> >  void virtio_instance_init_common(Object *proxy_obj, void *data,
> >                                   size_t vdev_size, const char *vdev_name);
> >
> > @@ -226,8 +276,6 @@ void virtio_error(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
> >  /* Set the child bus name. */
> >  void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
> >
> > -typedef void (*VirtIOHandleOutput)(VirtIODevice *, VirtQueue *);
> > -
> >  VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
> >                              VirtIOHandleOutput handle_output);
> >
> > --
> > 2.32.0.3.g01195cf9f
>

Re: [PATCH 2/3] virtio: struct VirtQueue introduce reset

[Xuan Zhuo]

On Sat, 28 Jan 2023 05:22:05 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sat, Jan 28, 2023 at 03:17:23PM +0800, Xuan Zhuo wrote:
> >  In the current design, we stop the device from operating on the vring
> >  during per-queue reset by resetting the structure VirtQueue.
> >
> >  But before the reset operation, when recycling some resources, we should
> >  stop referencing new vring resources. For example, when recycling
> >  virtio-net's asynchronous sending resources, virtio-net should be able
> >  to perceive that the current queue is in the per-queue reset state, and
> >  stop sending new packets from the tx queue.
> >
> >  Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  hw/virtio/virtio.c         | 8 ++++++++
> >  include/hw/virtio/virtio.h | 3 +++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 03077b2ecf..907d5b8bde 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -2030,6 +2030,12 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
> >  {
> >      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >
> > +    /*
> > +     * Mark this queue is per-queue reset status. The device should release the
> > +     * references of the vring, and not refer more new vring item.
> > +     */
> > +    vdev->vq[queue_index].reset = true;
> > +
> >      if (k->queue_reset) {
> >          k->queue_reset(vdev, queue_index);
> >      }
> > @@ -2053,6 +2059,8 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> >      }
> >      */
> >
> > +    vdev->vq[queue_index].reset = false;
> > +
> >      if (k->queue_enable) {
> >          k->queue_enable(vdev, queue_index);
> >      }
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 1c0d77c670..b888538d09 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -251,6 +251,9 @@ struct VirtQueue {
> >      /* Notification enabled? */
> >      bool notification;
> >
> > +    /* Per-Queue Reset status */
> > +    bool reset;
> > +
> >      uint16_t queue_index;
> >
>
> Reset state makes no sense. It seems to imply queue_reset
> in the spec. And for extra fun there's "reset" in the pci
> proxy which means "virtio_queue_reset is in progress" - I have no
> idea what uses it though - it is not guest visible.  First what is it?
> It actually means "queue has been reset and not has not been enabled since".
> So disabled_by_reset maybe?


In fact, when reading this, the queue has not been reset,
so prepare_for_reset?

>
> Second this hack helps make the change minimal
> so it's helpful for stable, but it's ugly in that it
> duplicates the reverse of enabled value - we don't really
> care what disabled it in practice.
>
> With the fixups above I can apply so it's easier to backport, but later
> a patch on top should clean it all up, perhaps by adding
> "enabled" in VirtQueue. We should also get rid of "reset" in the proxy
> unless there's some way it's useful which I don't currently see.
>

I have some confusion, I don't understand what you mean.

Why did we remove the "reset" in the proxy?

I agree to rename the "reset".

Thanks.

>
>
> >      unsigned int inuse;
> > --
> > 2.32.0.3.g01195cf9f
>

Re: [PATCH 2/3] virtio: struct VirtQueue introduce reset

[Michael S. Tsirkin]

On Sat, Jan 28, 2023 at 06:41:09PM +0800, Xuan Zhuo wrote:
> On Sat, 28 Jan 2023 05:22:05 -0500, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Sat, Jan 28, 2023 at 03:17:23PM +0800, Xuan Zhuo wrote:
> > >  In the current design, we stop the device from operating on the vring
> > >  during per-queue reset by resetting the structure VirtQueue.
> > >
> > >  But before the reset operation, when recycling some resources, we should
> > >  stop referencing new vring resources. For example, when recycling
> > >  virtio-net's asynchronous sending resources, virtio-net should be able
> > >  to perceive that the current queue is in the per-queue reset state, and
> > >  stop sending new packets from the tx queue.
> > >
> > >  Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >  hw/virtio/virtio.c         | 8 ++++++++
> > >  include/hw/virtio/virtio.h | 3 +++
> > >  2 files changed, 11 insertions(+)
> > >
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 03077b2ecf..907d5b8bde 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -2030,6 +2030,12 @@ void virtio_queue_reset(VirtIODevice *vdev, uint32_t queue_index)
> > >  {
> > >      VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> > >
> > > +    /*
> > > +     * Mark this queue is per-queue reset status. The device should release the
> > > +     * references of the vring, and not refer more new vring item.
> > > +     */
> > > +    vdev->vq[queue_index].reset = true;
> > > +
> > >      if (k->queue_reset) {
> > >          k->queue_reset(vdev, queue_index);
> > >      }
> > > @@ -2053,6 +2059,8 @@ void virtio_queue_enable(VirtIODevice *vdev, uint32_t queue_index)
> > >      }
> > >      */
> > >
> > > +    vdev->vq[queue_index].reset = false;
> > > +
> > >      if (k->queue_enable) {
> > >          k->queue_enable(vdev, queue_index);
> > >      }
> > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > index 1c0d77c670..b888538d09 100644
> > > --- a/include/hw/virtio/virtio.h
> > > +++ b/include/hw/virtio/virtio.h
> > > @@ -251,6 +251,9 @@ struct VirtQueue {
> > >      /* Notification enabled? */
> > >      bool notification;
> > >
> > > +    /* Per-Queue Reset status */
> > > +    bool reset;
> > > +
> > >      uint16_t queue_index;
> > >
> >
> > Reset state makes no sense. It seems to imply queue_reset
> > in the spec. And for extra fun there's "reset" in the pci
> > proxy which means "virtio_queue_reset is in progress" - I have no
> > idea what uses it though - it is not guest visible.  First what is it?
> > It actually means "queue has been reset and not has not been enabled since".
> > So disabled_by_reset maybe?
> 
> 
> In fact, when reading this, the queue has not been reset,
> so prepare_for_reset?

Makes it sound like it's some kind of temporary state where
it is not - it will stay like this until enabled.
As this makes no practical difference that it is set to
early, just set it later for consistency.

> >
> > Second this hack helps make the change minimal
> > so it's helpful for stable, but it's ugly in that it
> > duplicates the reverse of enabled value - we don't really
> > care what disabled it in practice.
> >
> > With the fixups above I can apply so it's easier to backport, but later
> > a patch on top should clean it all up, perhaps by adding
> > "enabled" in VirtQueue. We should also get rid of "reset" in the proxy
> > unless there's some way it's useful which I don't currently see.
> >
> 
> I have some confusion, I don't understand what you mean.
> 
> Why did we remove the "reset" in the proxy?

We did not but we should.
Why we should remove "reset" in the proxy?
Because guest can never read it as != 0:

    case VIRTIO_PCI_COMMON_Q_RESET:
        if (val == 1) {
            proxy->vqs[vdev->queue_sel].reset = 1;

            virtio_queue_reset(vdev, vdev->queue_sel);

            proxy->vqs[vdev->queue_sel].reset = 0;
            proxy->vqs[vdev->queue_sel].enabled = 0;
        }
        break;

from guest's POV reset is atomic and so does not need
a variable to track state.


> I agree to rename the "reset".
> 
> Thanks.
> 
> >
> >
> > >      unsigned int inuse;
> > > --
> > > 2.32.0.3.g01195cf9f
> >