[PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET

[PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET

[Xuan Zhuo]

The virtio spec already supports the virtio queue reset function. This patch set
is to add this function to the kernel. The relevant virtio spec information is
here:

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

Also regarding MMIO support for queue reset, I plan to support it after this
patch is passed.

Please review. Thanks.

v4:
  1. just the code of virtio, without virtio-net
  2. Performing reset on a queue is divided into these steps:
    1. reset_vq: reset one vq
    2. recycle the buffer from vq by virtqueue_detach_unused_buf()
    3. release the ring of the vq by vring_release_virtqueue()
    4. enable_reset_vq: re-enable the reset queue
  3. Simplify the parameters of enable_reset_vq()
  4. add container structures for virtio_pci_common_cfg

v3:
  1. keep vq, irq unreleased

Xuan Zhuo (14):
  virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
  virtio: queue_reset: add VIRTIO_F_RING_RESET
  virtio_ring: queue_reset: add function vring_setup_virtqueue()
  virtio_ring: queue_reset: split: add __vring_init_virtqueue()
  virtio_ring: queue_reset: split: support enable reset queue
  virtio_ring: queue_reset: packed: support enable reset queue
  virtio_ring: queue_reset: extract the release function of the vq ring
  virtio_ring: queue_reset: add vring_release_virtqueue()
  virtio: queue_reset: struct virtio_config_ops add callbacks for
    queue_reset
  virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
    option functions
  virtio_pci: queue_reset: release vq by vp_dev->vqs
  virtio_pci: queue_reset: setup_vq() support vring_setup_virtqueue()
  virtio_pci: queue_reset: vp_setup_vq() support ring_num
  virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

 drivers/virtio/virtio_pci_common.c     |  65 +++++++---
 drivers/virtio/virtio_pci_common.h     |  11 +-
 drivers/virtio/virtio_pci_legacy.c     |   5 +-
 drivers/virtio/virtio_pci_modern.c     |  99 ++++++++++++--
 drivers/virtio/virtio_pci_modern_dev.c |  36 +++++
 drivers/virtio/virtio_ring.c           | 173 ++++++++++++++++++-------
 include/linux/virtio.h                 |   6 +
 include/linux/virtio_config.h          |  13 ++
 include/linux/virtio_pci_modern.h      |   2 +
 include/linux/virtio_ring.h            |  37 ++++--
 include/uapi/linux/virtio_config.h     |   7 +-
 include/uapi/linux/virtio_pci.h        |  14 ++
 12 files changed, 375 insertions(+), 93 deletions(-)

--
2.31.0

[PATCH v4 01/14] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data

[Xuan Zhuo]

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

For not breaks uABI, add a new struct virtio_pci_common_cfg_notify.

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/uapi/linux/virtio_pci.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index 3a86f36d7e3d..22bec9bd0dfc 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -166,6 +166,13 @@ struct virtio_pci_common_cfg {
 	__le32 queue_used_hi;		/* read-write */
 };
 
+struct virtio_pci_common_cfg_notify {
+	struct virtio_pci_common_cfg cfg;
+
+	__le16 queue_notify_data;	/* read-write */
+	__le16 padding;
+};
+
 /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
 struct virtio_pci_cfg_cap {
 	struct virtio_pci_cap cap;
-- 
2.31.0

[PATCH v4 02/14] virtio: queue_reset: add VIRTIO_F_RING_RESET

[Xuan Zhuo]

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

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

diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index b5eda06f0d57..0862be802ff8 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -52,7 +52,7 @@
  * rest are per-device feature bits.
  */
 #define VIRTIO_TRANSPORT_F_START	28
-#define VIRTIO_TRANSPORT_F_END		38
+#define VIRTIO_TRANSPORT_F_END		41
 
 #ifndef VIRTIO_CONFIG_NO_LEGACY
 /* Do we get callbacks when the ring is completely used, even if we've
@@ -92,4 +92,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 /* _UAPI_LINUX_VIRTIO_CONFIG_H */
-- 
2.31.0

[PATCH v4 03/14] virtio_ring: queue_reset: add function vring_setup_virtqueue()

[Xuan Zhuo]

Added function vring_setup_virtqueue() to allow passing existing vq
without reallocating vq.

The purpose of adding this function is to not break the form of
vring_create_virtqueue().

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c |  7 ++++---
 include/linux/virtio_ring.h  | 37 ++++++++++++++++++++++++++----------
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 028b05d44546..766f4fd8cf06 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2253,7 +2253,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 }
 EXPORT_SYMBOL_GPL(__vring_new_virtqueue);
 
-struct virtqueue *vring_create_virtqueue(
+struct virtqueue *vring_setup_virtqueue(
 	unsigned int index,
 	unsigned int num,
 	unsigned int vring_align,
@@ -2263,7 +2263,8 @@ struct virtqueue *vring_create_virtqueue(
 	bool context,
 	bool (*notify)(struct virtqueue *),
 	void (*callback)(struct virtqueue *),
-	const char *name)
+	const char *name,
+	struct virtqueue *vq)
 {
 
 	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
@@ -2275,7 +2276,7 @@ struct virtqueue *vring_create_virtqueue(
 			vdev, weak_barriers, may_reduce_num,
 			context, notify, callback, name);
 }
-EXPORT_SYMBOL_GPL(vring_create_virtqueue);
+EXPORT_SYMBOL_GPL(vring_setup_virtqueue);
 
 /* Only available for split ring */
 struct virtqueue *vring_new_virtqueue(unsigned int index,
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index b485b13fa50b..e90323fce4bf 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -65,16 +65,33 @@ struct virtqueue;
  * expected.  The caller should query virtqueue_get_vring_size to learn
  * the actual size of the ring.
  */
-struct virtqueue *vring_create_virtqueue(unsigned int index,
-					 unsigned int num,
-					 unsigned int vring_align,
-					 struct virtio_device *vdev,
-					 bool weak_barriers,
-					 bool may_reduce_num,
-					 bool ctx,
-					 bool (*notify)(struct virtqueue *vq),
-					 void (*callback)(struct virtqueue *vq),
-					 const char *name);
+struct virtqueue *vring_setup_virtqueue(unsigned int index,
+					unsigned int num,
+					unsigned int vring_align,
+					struct virtio_device *vdev,
+					bool weak_barriers,
+					bool may_reduce_num,
+					bool ctx,
+					bool (*notify)(struct virtqueue *vq),
+					void (*callback)(struct virtqueue *vq),
+					const char *name,
+					struct virtqueue *vq);
+
+static inline struct virtqueue *vring_create_virtqueue(unsigned int index,
+						       unsigned int num,
+						       unsigned int vring_align,
+						       struct virtio_device *vdev,
+						       bool weak_barriers,
+						       bool may_reduce_num,
+						       bool ctx,
+						       bool (*notify)(struct virtqueue *vq),
+						       void (*callback)(struct virtqueue *vq),
+						       const char *name)
+{
+	return vring_setup_virtqueue(index, num, vring_align, vdev,
+				     weak_barriers, may_reduce_num, ctx,
+				     notify, callback, name, NULL);
+}
 
 /* Creates a virtqueue with a custom layout. */
 struct virtqueue *__vring_new_virtqueue(unsigned int index,
-- 
2.31.0

[PATCH v4 04/14] virtio_ring: queue_reset: split: add __vring_init_virtqueue()

[Xuan Zhuo]

Extract vq's initialization function __vring_init_virtqueue() from
__vring_new_virtqueue()

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 61 +++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 766f4fd8cf06..30be9173c263 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2167,23 +2167,17 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
 EXPORT_SYMBOL_GPL(vring_interrupt);
 
 /* Only available for split ring */
-struct virtqueue *__vring_new_virtqueue(unsigned int index,
-					struct vring vring,
-					struct virtio_device *vdev,
-					bool weak_barriers,
-					bool context,
-					bool (*notify)(struct virtqueue *),
-					void (*callback)(struct virtqueue *),
-					const char *name)
+static int __vring_init_virtqueue(struct virtqueue *_vq,
+				  unsigned int index,
+				  struct vring vring,
+				  struct virtio_device *vdev,
+				  bool weak_barriers,
+				  bool context,
+				  bool (*notify)(struct virtqueue *),
+				  void (*callback)(struct virtqueue *),
+				  const char *name)
 {
-	struct vring_virtqueue *vq;
-
-	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
-		return NULL;
-
-	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
-	if (!vq)
-		return NULL;
+	struct vring_virtqueue *vq = to_vvq(_vq);
 
 	vq->packed_ring = false;
 	vq->vq.callback = callback;
@@ -2243,13 +2237,42 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	spin_lock(&vdev->vqs_list_lock);
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 	spin_unlock(&vdev->vqs_list_lock);
-	return &vq->vq;
+	return 0;
 
 err_extra:
 	kfree(vq->split.desc_state);
 err_state:
-	kfree(vq);
-	return NULL;
+	return -ENOMEM;
+}
+
+struct virtqueue *__vring_new_virtqueue(unsigned int index,
+					struct vring vring,
+					struct virtio_device *vdev,
+					bool weak_barriers,
+					bool context,
+					bool (*notify)(struct virtqueue *),
+					void (*callback)(struct virtqueue *),
+					const char *name)
+{
+	struct vring_virtqueue *vq;
+	int err;
+
+	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
+		return NULL;
+
+	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
+	if (!vq)
+		return NULL;
+
+	err = __vring_init_virtqueue(&vq->vq, index, vring, vdev, weak_barriers,
+				     context, notify, callback, name);
+
+	if (err) {
+		kfree(vq);
+		return NULL;
+	}
+
+	return &vq->vq;
 }
 EXPORT_SYMBOL_GPL(__vring_new_virtqueue);
 
-- 
2.31.0

[PATCH v4 05/14] virtio_ring: queue_reset: split: support enable reset queue

[Xuan Zhuo]

The purpose of this patch is to make vring split support re-enable reset
vq.

Based on whether the incoming vq passed by vring_setup_virtqueue() is
NULL or not, distinguish whether it is a normal create virtqueue or
re-enable a reset queue.

When re-enable a reset queue, reuse the original callback, name, priv,
indirect.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 30be9173c263..8530f144329b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -198,6 +198,16 @@ struct vring_virtqueue {
 #endif
 };
 
+static int __vring_init_virtqueue(struct virtqueue *_vq,
+				  unsigned int index,
+				  struct vring vring,
+				  struct virtio_device *vdev,
+				  bool weak_barriers,
+				  bool context,
+				  bool (*notify)(struct virtqueue *),
+				  void (*callback)(struct virtqueue *),
+				  const char *name,
+				  bool reset);
 
 /*
  * Helpers.
@@ -925,9 +935,9 @@ static struct virtqueue *vring_create_virtqueue_split(
 	bool context,
 	bool (*notify)(struct virtqueue *),
 	void (*callback)(struct virtqueue *),
-	const char *name)
+	const char *name,
+	struct virtqueue *vq)
 {
-	struct virtqueue *vq;
 	void *queue = NULL;
 	dma_addr_t dma_addr;
 	size_t queue_size_in_bytes;
@@ -964,12 +974,17 @@ static struct virtqueue *vring_create_virtqueue_split(
 	queue_size_in_bytes = vring_size(num, vring_align);
 	vring_init(&vring, num, queue, vring_align);
 
-	vq = __vring_new_virtqueue(index, vring, vdev, weak_barriers, context,
-				   notify, callback, name);
 	if (!vq) {
-		vring_free_queue(vdev, queue_size_in_bytes, queue,
-				 dma_addr);
-		return NULL;
+		vq = __vring_new_virtqueue(index, vring, vdev, weak_barriers,
+					   context, notify, callback, name);
+		if (!vq)
+			goto err;
+
+	} else {
+		if (__vring_init_virtqueue(vq, index, vring, vdev,
+					   weak_barriers, context, notify,
+					   callback, name, true))
+			goto err;
 	}
 
 	to_vvq(vq)->split.queue_dma_addr = dma_addr;
@@ -977,6 +992,9 @@ static struct virtqueue *vring_create_virtqueue_split(
 	to_vvq(vq)->we_own_ring = true;
 
 	return vq;
+err:
+	vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
+	return NULL;
 }
 
 
@@ -2175,14 +2193,21 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
 				  bool context,
 				  bool (*notify)(struct virtqueue *),
 				  void (*callback)(struct virtqueue *),
-				  const char *name)
+				  const char *name,
+				  bool reset)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
+	if (!reset) {
+		vq->vq.callback = callback;
+		vq->vq.name = name;
+		vq->vq.priv = NULL;
+		vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
+			!context;
+	}
+
 	vq->packed_ring = false;
-	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
-	vq->vq.name = name;
 	vq->vq.num_free = vring.num;
 	vq->vq.index = index;
 	vq->we_own_ring = false;
@@ -2198,8 +2223,6 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
 	vq->last_add_time_valid = false;
 #endif
 
-	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
-		!context;
 	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
 
 	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
@@ -2213,7 +2236,7 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
 	vq->split.avail_idx_shadow = 0;
 
 	/* No callback?  Tell other side not to bother us. */
-	if (!callback) {
+	if (!vq->vq.callback) {
 		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
 		if (!vq->event)
 			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
@@ -2265,7 +2288,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 		return NULL;
 
 	err = __vring_init_virtqueue(&vq->vq, index, vring, vdev, weak_barriers,
-				     context, notify, callback, name);
+				     context, notify, callback, name, false);
 
 	if (err) {
 		kfree(vq);
@@ -2297,7 +2320,7 @@ struct virtqueue *vring_setup_virtqueue(
 
 	return vring_create_virtqueue_split(index, num, vring_align,
 			vdev, weak_barriers, may_reduce_num,
-			context, notify, callback, name);
+			context, notify, callback, name, vq);
 }
 EXPORT_SYMBOL_GPL(vring_setup_virtqueue);
 
-- 
2.31.0

[PATCH v4 06/14] virtio_ring: queue_reset: packed: support enable reset queue

[Xuan Zhuo]

The purpose of this patch is to make vring packed support re-enable reset
vq.

Based on whether the incoming vq passed by vring_setup_virtqueue() is
NULL or not, distinguish whether it is a normal create virtqueue or
re-enable a reset queue.

When re-enable a reset queue, reuse the original callback, name, priv,
indirect.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 8530f144329b..5a524ee8b542 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1681,7 +1681,8 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	bool context,
 	bool (*notify)(struct virtqueue *),
 	void (*callback)(struct virtqueue *),
-	const char *name)
+	const char *name,
+	struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq;
 	struct vring_packed_desc *ring;
@@ -1711,13 +1712,23 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	if (!device)
 		goto err_device;
 
-	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
-	if (!vq)
-		goto err_vq;
+	if (_vq) {
+		vq = to_vvq(_vq);
+	} else {
+		vq = kmalloc(sizeof(*vq), GFP_KERNEL);
+		if (!vq)
+			goto err_vq;
+	}
+
+	if (!_vq) {
+		vq->vq.callback = callback;
+		vq->vq.name = name;
+		vq->vq.priv = NULL;
+		vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
+			!context;
+	}
 
-	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
-	vq->vq.name = name;
 	vq->vq.num_free = num;
 	vq->vq.index = index;
 	vq->we_own_ring = true;
@@ -1734,8 +1745,6 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	vq->last_add_time_valid = false;
 #endif
 
-	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
-		!context;
 	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
 
 	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
@@ -1776,7 +1785,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 		goto err_desc_extra;
 
 	/* No callback?  Tell other side not to bother us. */
-	if (!callback) {
+	if (!vq->vq.callback) {
 		vq->packed.event_flags_shadow = VRING_PACKED_EVENT_FLAG_DISABLE;
 		vq->packed.vring.driver->flags =
 			cpu_to_le16(vq->packed.event_flags_shadow);
@@ -1790,7 +1799,8 @@ static struct virtqueue *vring_create_virtqueue_packed(
 err_desc_extra:
 	kfree(vq->packed.desc_state);
 err_desc_state:
-	kfree(vq);
+	if (!_vq)
+		kfree(vq);
 err_vq:
 	vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
 err_device:
@@ -2316,7 +2326,7 @@ struct virtqueue *vring_setup_virtqueue(
 	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
 		return vring_create_virtqueue_packed(index, num, vring_align,
 				vdev, weak_barriers, may_reduce_num,
-				context, notify, callback, name);
+				context, notify, callback, name, vq);
 
 	return vring_create_virtqueue_split(index, num, vring_align,
 			vdev, weak_barriers, may_reduce_num,
-- 
2.31.0

[PATCH v4 07/14] virtio_ring: queue_reset: extract the release function of the vq ring

[Xuan Zhuo]

Extract a function __vring_del_virtqueue() from vring_del_virtqueue() to
handle releasing vq's ring.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5a524ee8b542..f5e5fec6d904 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2357,12 +2357,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
 }
 EXPORT_SYMBOL_GPL(vring_new_virtqueue);
 
-void vring_del_virtqueue(struct virtqueue *_vq)
+static void __vring_del_virtqueue(struct vring_virtqueue *vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
-
 	spin_lock(&vq->vq.vdev->vqs_list_lock);
-	list_del(&_vq->list);
+	list_del(&vq->vq.list);
 	spin_unlock(&vq->vq.vdev->vqs_list_lock);
 
 	if (vq->we_own_ring) {
@@ -2395,6 +2393,13 @@ void vring_del_virtqueue(struct virtqueue *_vq)
 		kfree(vq->split.desc_state);
 		kfree(vq->split.desc_extra);
 	}
+}
+
+void vring_del_virtqueue(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	__vring_del_virtqueue(vq);
 	kfree(vq);
 }
 EXPORT_SYMBOL_GPL(vring_del_virtqueue);
-- 
2.31.0

[PATCH v4 08/14] virtio_ring: queue_reset: add vring_release_virtqueue()

[Xuan Zhuo]

Added vring_release_virtqueue() to release the ring of the vq.

In this process, vq is removed from the vdev->vqs queue. And the memory
of the ring is released

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_ring.c | 13 ++++++++++++-
 include/linux/virtio.h       |  5 +++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index f5e5fec6d904..b8747df8dc1f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2363,6 +2363,8 @@ static void __vring_del_virtqueue(struct vring_virtqueue *vq)
 	list_del(&vq->vq.list);
 	spin_unlock(&vq->vq.vdev->vqs_list_lock);
 
+	INIT_LIST_HEAD(&vq->vq.list);
+
 	if (vq->we_own_ring) {
 		if (vq->packed_ring) {
 			vring_free_queue(vq->vq.vdev,
@@ -2399,11 +2401,20 @@ void vring_del_virtqueue(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	__vring_del_virtqueue(vq);
+	if (!list_empty(&vq->vq.list))
+		__vring_del_virtqueue(vq);
 	kfree(vq);
 }
 EXPORT_SYMBOL_GPL(vring_del_virtqueue);
 
+void vring_release_virtqueue(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+
+	__vring_del_virtqueue(vq);
+}
+EXPORT_SYMBOL_GPL(vring_release_virtqueue);
+
 /* Manipulates transport-specific feature bits. */
 void vring_transport_features(struct virtio_device *vdev)
 {
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 41edbc01ffa4..dd1657c3a488 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -195,4 +195,9 @@ void unregister_virtio_driver(struct virtio_driver *drv);
 #define module_virtio_driver(__virtio_driver) \
 	module_driver(__virtio_driver, register_virtio_driver, \
 			unregister_virtio_driver)
+/*
+ * Resets a virtqueue. Just frees the ring, not free vq.
+ * This function must be called after reset_vq().
+ */
+void vring_release_virtqueue(struct virtqueue *vq);
 #endif /* _LINUX_VIRTIO_H */
-- 
2.31.0

[PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset

[Xuan Zhuo]

Performing reset on a queue is divided into four steps:

1. reset_vq: reset one vq
2. recycle the buffer from vq by virtqueue_detach_unused_buf()
3. release the ring of the vq by vring_release_virtqueue()
4. enable_reset_vq: re-enable the reset queue

So add two callbacks reset_vq, enable_reset_vq to struct
virtio_config_ops.

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

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 4d107ad31149..0d01a64f2576 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -74,6 +74,17 @@ struct virtio_shm_region {
  * @set_vq_affinity: set the affinity for a virtqueue (optional).
  * @get_vq_affinity: get the affinity for a virtqueue (optional).
  * @get_shm_region: get a shared memory region based on the index.
+ * @reset_vq: reset a queue individually
+ *	vq: the virtqueue
+ *	Returns 0 on success or error status
+ *	After successfully calling this, be sure to call
+ *	virtqueue_detach_unused_buf() to recycle the buffer in the ring, and
+ *	then call vring_release_virtqueue() to release the vq ring.
+ * @enable_reset_vq: enable a reset queue
+ *	vq: the virtqueue
+ *	ring_num: specify ring num for the vq to be re-enabled. 0 means use the
+ *	          default value. MUST be a power of 2.
+ *	Returns 0 on success or error status
  */
 typedef void vq_callback_t(struct virtqueue *);
 struct virtio_config_ops {
@@ -100,6 +111,8 @@ struct virtio_config_ops {
 			int index);
 	bool (*get_shm_region)(struct virtio_device *vdev,
 			       struct virtio_shm_region *region, u8 id);
+	int (*reset_vq)(struct virtqueue *vq);
+	int (*enable_reset_vq)(struct virtqueue *vq, u16 ring_num);
 };
 
 /* If driver didn't advertise the feature, it will never appear. */
-- 
2.31.0

[PATCH v4 10/14] virtio_pci: queue_reset: update struct virtio_pci_common_cfg and option functions

[Xuan Zhuo]

Add queue_reset in virtio_pci_common_cfg, and add related operation
functions.

For not breaks uABI, add a new struct virtio_pci_common_cfg_reset.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_modern_dev.c | 36 ++++++++++++++++++++++++++
 include/linux/virtio_pci_modern.h      |  2 ++
 include/uapi/linux/virtio_pci.h        |  7 +++++
 3 files changed, 45 insertions(+)

diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
index e11ed748e661..a35106edb27b 100644
--- a/drivers/virtio/virtio_pci_modern_dev.c
+++ b/drivers/virtio/virtio_pci_modern_dev.c
@@ -3,6 +3,7 @@
 #include <linux/virtio_pci_modern.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/delay.h>
 
 /*
  * vp_modern_map_capability - map a part of virtio pci capability
@@ -463,6 +464,41 @@ void vp_modern_set_status(struct virtio_pci_modern_device *mdev,
 }
 EXPORT_SYMBOL_GPL(vp_modern_set_status);
 
+/*
+ * vp_modern_get_queue_reset - get the queue reset status
+ * @mdev: the modern virtio-pci device
+ * @index: queue index
+ */
+int vp_modern_get_queue_reset(struct virtio_pci_modern_device *mdev, u16 index)
+{
+	struct virtio_pci_common_cfg_reset __iomem *cfg;
+
+	cfg = (struct virtio_pci_common_cfg_reset *)mdev->common;
+
+	vp_iowrite16(index, &cfg->cfg.queue_select);
+	return vp_ioread16(&cfg->queue_reset);
+}
+EXPORT_SYMBOL_GPL(vp_modern_get_queue_reset);
+
+/*
+ * vp_modern_set_queue_reset - reset the queue
+ * @mdev: the modern virtio-pci device
+ * @index: queue index
+ */
+void vp_modern_set_queue_reset(struct virtio_pci_modern_device *mdev, u16 index)
+{
+	struct virtio_pci_common_cfg_reset __iomem *cfg;
+
+	cfg = (struct virtio_pci_common_cfg_reset *)mdev->common;
+
+	vp_iowrite16(index, &cfg->cfg.queue_select);
+	vp_iowrite16(1, &cfg->queue_reset);
+
+	while (vp_ioread16(&cfg->queue_reset) != 1)
+		msleep(1);
+}
+EXPORT_SYMBOL_GPL(vp_modern_set_queue_reset);
+
 /*
  * vp_modern_queue_vector - set the MSIX vector for a specific virtqueue
  * @mdev: the modern virtio-pci device
diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
index eb2bd9b4077d..cc4154dd7b28 100644
--- a/include/linux/virtio_pci_modern.h
+++ b/include/linux/virtio_pci_modern.h
@@ -106,4 +106,6 @@ void __iomem * vp_modern_map_vq_notify(struct virtio_pci_modern_device *mdev,
 				       u16 index, resource_size_t *pa);
 int vp_modern_probe(struct virtio_pci_modern_device *mdev);
 void vp_modern_remove(struct virtio_pci_modern_device *mdev);
+int vp_modern_get_queue_reset(struct virtio_pci_modern_device *mdev, u16 index);
+void vp_modern_set_queue_reset(struct virtio_pci_modern_device *mdev, u16 index);
 #endif
diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
index 22bec9bd0dfc..d9462efd6ce8 100644
--- a/include/uapi/linux/virtio_pci.h
+++ b/include/uapi/linux/virtio_pci.h
@@ -173,6 +173,13 @@ struct virtio_pci_common_cfg_notify {
 	__le16 padding;
 };
 
+struct virtio_pci_common_cfg_reset {
+	struct virtio_pci_common_cfg cfg;
+
+	__le16 queue_notify_data;	/* read-write */
+	__le16 queue_reset;		/* read-write */
+};
+
 /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
 struct virtio_pci_cfg_cap {
 	struct virtio_pci_cap cap;
-- 
2.31.0

[PATCH v4 11/14] virtio_pci: queue_reset: release vq by vp_dev->vqs

[Xuan Zhuo]

In the process of queue reset, vq leaves vdev->vqs, so the original
processing logic may miss some vq. So modify the processing method of
releasing vq. Release vq by listing vqs.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_common.c | 22 ++++++++++++++++++----
 drivers/virtio/virtio_pci_common.h |  2 ++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index fdbde1db5ec5..6b2573ec1ae8 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -260,12 +260,20 @@ static void vp_del_vq(struct virtqueue *vq)
 void vp_del_vqs(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtqueue *vq, *n;
-	int i;
+	struct virtio_pci_vq_info *info;
+	struct virtqueue *vq;
+	int i, v;
+
+	for (i = 0; i < vp_dev->nvqs; ++i) {
+
+		info = vp_dev->vqs[i];
+		if (!info)
+			continue;
+
+		vq = info->vq;
 
-	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
 		if (vp_dev->per_vq_vectors) {
-			int v = vp_dev->vqs[vq->index]->msix_vector;
+			v = info->msix_vector;
 
 			if (v != VIRTIO_MSI_NO_VECTOR) {
 				int irq = pci_irq_vector(vp_dev->pci_dev, v);
@@ -275,6 +283,7 @@ void vp_del_vqs(struct virtio_device *vdev)
 			}
 		}
 		vp_del_vq(vq);
+		vp_dev->vqs[i] = NULL;
 	}
 	vp_dev->per_vq_vectors = false;
 
@@ -308,6 +317,7 @@ void vp_del_vqs(struct virtio_device *vdev)
 	vp_dev->msix_affinity_masks = NULL;
 	kfree(vp_dev->vqs);
 	vp_dev->vqs = NULL;
+	vp_dev->nvqs = 0;
 }
 
 static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
@@ -324,6 +334,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
 	if (!vp_dev->vqs)
 		return -ENOMEM;
 
+	vp_dev->nvqs = nvqs;
+
 	if (per_vq_vectors) {
 		/* Best option: one for change interrupt, one per vq. */
 		nvectors = 1;
@@ -395,6 +407,8 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
 	if (!vp_dev->vqs)
 		return -ENOMEM;
 
+	vp_dev->nvqs = nvqs;
+
 	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt, IRQF_SHARED,
 			dev_name(&vdev->dev), vp_dev);
 	if (err)
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 23f6c5c678d5..392d990b7c73 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -60,6 +60,8 @@ struct virtio_pci_device {
 	/* array of all queues for house-keeping */
 	struct virtio_pci_vq_info **vqs;
 
+	u32 nvqs;
+
 	/* MSI-X support */
 	int msix_enabled;
 	int intx_enabled;
-- 
2.31.0

[PATCH v4 12/14] virtio_pci: queue_reset: setup_vq() support vring_setup_virtqueue()

[Xuan Zhuo]

modern setup_vq() replaces vring_create_virtqueue() with
vring_setup_virtqueue()

vp_setup_vq() can pass the original vq(from info->vq) to re-enable vq.

Allow direct calls to vp_setup_vq() in virtio_pci_modern.c

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_common.c | 31 ++++++++++++++++++------------
 drivers/virtio/virtio_pci_common.h |  8 +++++++-
 drivers/virtio/virtio_pci_legacy.c |  4 ++--
 drivers/virtio/virtio_pci_modern.c | 12 ++++++------
 4 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 6b2573ec1ae8..5a4f750a0b97 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -205,28 +205,33 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
 	return err;
 }
 
-static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
-				     void (*callback)(struct virtqueue *vq),
-				     const char *name,
-				     bool ctx,
-				     u16 msix_vec)
+struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned int index,
+			      void (*callback)(struct virtqueue *vq),
+			      const char *name,
+			      bool ctx,
+			      u16 msix_vec)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtio_pci_vq_info *info = kmalloc(sizeof *info, GFP_KERNEL);
+	struct virtio_pci_vq_info *info;
 	struct virtqueue *vq;
 	unsigned long flags;
 
-	/* fill out our structure that represents an active queue */
-	if (!info)
-		return ERR_PTR(-ENOMEM);
+	info = vp_dev->vqs[index];
+	if (!info) {
+		info = kzalloc(sizeof(*info), GFP_KERNEL);
+
+		/* fill out our structure that represents an active queue */
+		if (!info)
+			return ERR_PTR(-ENOMEM);
+	}
 
 	vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
-			      msix_vec);
+			      msix_vec, info->vq);
 	if (IS_ERR(vq))
 		goto out_info;
 
 	info->vq = vq;
-	if (callback) {
+	if (vq->callback) {
 		spin_lock_irqsave(&vp_dev->lock, flags);
 		list_add(&info->node, &vp_dev->virtqueues);
 		spin_unlock_irqrestore(&vp_dev->lock, flags);
@@ -238,7 +243,9 @@ static struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned index,
 	return vq;
 
 out_info:
-	kfree(info);
+	if (!info->vq)
+		kfree(info);
+
 	return vq;
 }
 
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 392d990b7c73..696e3f6a493b 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -84,7 +84,8 @@ struct virtio_pci_device {
 				      void (*callback)(struct virtqueue *vq),
 				      const char *name,
 				      bool ctx,
-				      u16 msix_vec);
+				      u16 msix_vec,
+				      struct virtqueue *vq);
 	void (*del_vq)(struct virtio_pci_vq_info *info);
 
 	u16 (*config_vector)(struct virtio_pci_device *vp_dev, u16 vector);
@@ -117,6 +118,11 @@ int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		struct virtqueue *vqs[], vq_callback_t *callbacks[],
 		const char * const names[], const bool *ctx,
 		struct irq_affinity *desc);
+struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned int index,
+			      void (*callback)(struct virtqueue *vq),
+			      const char *name,
+			      bool ctx,
+			      u16 msix_vec);
 const char *vp_bus_name(struct virtio_device *vdev);
 
 /* Setup the affinity for a virtqueue:
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index b3f8128b7983..aa28a02d11f2 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -113,9 +113,9 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
 				  bool ctx,
-				  u16 msix_vec)
+				  u16 msix_vec,
+				  struct virtqueue *vq)
 {
-	struct virtqueue *vq;
 	u16 num;
 	int err;
 	u64 q_pfn;
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 5455bc041fb6..5af82948f0ae 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -187,11 +187,11 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 				  void (*callback)(struct virtqueue *vq),
 				  const char *name,
 				  bool ctx,
-				  u16 msix_vec)
+				  u16 msix_vec,
+				  struct virtqueue *vq)
 {
 
 	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
-	struct virtqueue *vq;
 	u16 num;
 	int err;
 
@@ -211,10 +211,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	info->msix_vector = msix_vec;
 
 	/* create the vring */
-	vq = vring_create_virtqueue(index, num,
-				    SMP_CACHE_BYTES, &vp_dev->vdev,
-				    true, true, ctx,
-				    vp_notify, callback, name);
+	vq = vring_setup_virtqueue(index, num,
+				   SMP_CACHE_BYTES, &vp_dev->vdev,
+				   true, true, ctx,
+				   vp_notify, callback, name, vq);
 	if (!vq)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.31.0

[PATCH v4 13/14] virtio_pci: queue_reset: vp_setup_vq() support ring_num

[Xuan Zhuo]

vp_setup_vq() adds parameter ring_num to allow specifying ring num
during setup.

This can be used to implement virtio-net support set_ringparm(ethtool
-G eth0 rx 128 tx 128)

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_common.c | 8 ++++----
 drivers/virtio/virtio_pci_common.h | 5 +++--
 drivers/virtio/virtio_pci_legacy.c | 3 ++-
 drivers/virtio/virtio_pci_modern.c | 9 ++++++++-
 4 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 5a4f750a0b97..cb01eb0cb2e4 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -209,7 +209,7 @@ struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned int index,
 			      void (*callback)(struct virtqueue *vq),
 			      const char *name,
 			      bool ctx,
-			      u16 msix_vec)
+			      u16 msix_vec, u16 ring_num)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
 	struct virtio_pci_vq_info *info;
@@ -226,7 +226,7 @@ struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned int index,
 	}
 
 	vq = vp_dev->setup_vq(vp_dev, info, index, callback, name, ctx,
-			      msix_vec, info->vq);
+			      msix_vec, info->vq, ring_num);
 	if (IS_ERR(vq))
 		goto out_info;
 
@@ -375,7 +375,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
 			msix_vec = VP_MSIX_VQ_VECTOR;
 		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
 				     ctx ? ctx[i] : false,
-				     msix_vec);
+				     msix_vec, 0);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
@@ -430,7 +430,7 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned nvqs,
 		}
 		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
 				     ctx ? ctx[i] : false,
-				     VIRTIO_MSI_NO_VECTOR);
+				     VIRTIO_MSI_NO_VECTOR, 0);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto out_del_vqs;
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 696e3f6a493b..41051d29152c 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -85,7 +85,8 @@ struct virtio_pci_device {
 				      const char *name,
 				      bool ctx,
 				      u16 msix_vec,
-				      struct virtqueue *vq);
+				      struct virtqueue *vq,
+				      u16 ring_num);
 	void (*del_vq)(struct virtio_pci_vq_info *info);
 
 	u16 (*config_vector)(struct virtio_pci_device *vp_dev, u16 vector);
@@ -122,7 +123,7 @@ struct virtqueue *vp_setup_vq(struct virtio_device *vdev, unsigned int index,
 			      void (*callback)(struct virtqueue *vq),
 			      const char *name,
 			      bool ctx,
-			      u16 msix_vec);
+			      u16 msix_vec, u16 ring_num);
 const char *vp_bus_name(struct virtio_device *vdev);
 
 /* Setup the affinity for a virtqueue:
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index aa28a02d11f2..9bc41b764624 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -114,7 +114,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 				  const char *name,
 				  bool ctx,
 				  u16 msix_vec,
-				  struct virtqueue *vq)
+				  struct virtqueue *vq,
+				  u16 ring_num)
 {
 	u16 num;
 	int err;
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 5af82948f0ae..d29d40bf0b45 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -188,7 +188,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 				  const char *name,
 				  bool ctx,
 				  u16 msix_vec,
-				  struct virtqueue *vq)
+				  struct virtqueue *vq,
+				  u16 ring_num)
 {
 
 	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
@@ -203,6 +204,12 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	if (!num || vp_modern_get_queue_enable(mdev, index))
 		return ERR_PTR(-ENOENT);
 
+	if (ring_num) {
+		if (ring_num > num)
+			return ERR_PTR(-ENOENT);
+		num = ring_num;
+	}
+
 	if (num & (num - 1)) {
 		dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
 		return ERR_PTR(-EINVAL);
-- 
2.31.0

[PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Xuan Zhuo]

This patch implements virtio pci support for QUEUE RESET.

Performing reset on a queue is divided into these steps:

1. reset_vq: reset one vq
2. recycle the buffer from vq by virtqueue_detach_unused_buf()
3. release the ring of the vq by vring_release_virtqueue()
4. enable_reset_vq: re-enable the reset queue

This patch implements reset_vq, enable_reset_vq in the pci scenario

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/virtio/virtio_pci_common.c |  8 +--
 drivers/virtio/virtio_pci_modern.c | 80 ++++++++++++++++++++++++++++--
 drivers/virtio/virtio_ring.c       |  2 +
 include/linux/virtio.h             |  1 +
 4 files changed, 85 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index cb01eb0cb2e4..303637ac4914 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -255,9 +255,11 @@ static void vp_del_vq(struct virtqueue *vq)
 	struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
 	unsigned long flags;
 
-	spin_lock_irqsave(&vp_dev->lock, flags);
-	list_del(&info->node);
-	spin_unlock_irqrestore(&vp_dev->lock, flags);
+	if (!vq->reset) {
+		spin_lock_irqsave(&vp_dev->lock, flags);
+		list_del(&info->node);
+		spin_unlock_irqrestore(&vp_dev->lock, flags);
+	}
 
 	vp_dev->del_vq(info);
 	kfree(info);
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index d29d40bf0b45..cc45515eda50 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
 	if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
 			pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
 		__virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
+
+	if (features & BIT_ULL(VIRTIO_F_RING_RESET))
+		__virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
 }
 
 /* virtio config->finalize_features() implementation */
@@ -176,6 +179,70 @@ static void vp_reset(struct virtio_device *vdev)
 	vp_disable_cbs(vdev);
 }
 
+static int vp_modern_reset_vq(struct virtqueue *vq)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
+	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
+	struct virtio_pci_vq_info *info;
+	unsigned long flags;
+	u16 msix_vec;
+
+	if (!virtio_has_feature(vq->vdev, VIRTIO_F_RING_RESET))
+		return -ENOENT;
+
+	vp_modern_set_queue_reset(mdev, vq->index);
+
+	info = vp_dev->vqs[vq->index];
+	msix_vec = info->msix_vector;
+
+	/* Disable VQ callback. */
+	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
+		disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
+
+	/* delete vq */
+	spin_lock_irqsave(&vp_dev->lock, flags);
+	list_del(&info->node);
+	spin_unlock_irqrestore(&vp_dev->lock, flags);
+
+	vq->reset = true;
+
+	INIT_LIST_HEAD(&info->node);
+
+	return 0;
+}
+
+static int vp_modern_enable_reset_vq(struct virtqueue *vq, u16 ring_num)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
+	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
+	struct virtio_pci_vq_info *info;
+	struct virtqueue *_vq;
+	u16 msix_vec;
+
+	if (!vq->reset)
+		return -EPERM;
+
+	/* check queue reset status */
+	if (vp_modern_get_queue_reset(mdev, vq->index) != 1)
+		return -EBUSY;
+
+	info = vp_dev->vqs[vq->index];
+	_vq = vp_setup_vq(vq->vdev, vq->index, NULL, NULL, NULL,
+			 info->msix_vector, ring_num);
+	if (IS_ERR(_vq)) {
+		vq->reset = true;
+		return PTR_ERR(_vq);
+	}
+
+	vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
+
+	msix_vec = vp_dev->vqs[vq->index]->msix_vector;
+	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
+		enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
+
+	return 0;
+}
+
 static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
 {
 	return vp_modern_config_vector(&vp_dev->mdev, vector);
@@ -231,10 +298,13 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 				virtqueue_get_avail_addr(vq),
 				virtqueue_get_used_addr(vq));
 
-	vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index, NULL);
 	if (!vq->priv) {
-		err = -ENOMEM;
-		goto err_map_notify;
+		vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index,
+								   NULL);
+		if (!vq->priv) {
+			err = -ENOMEM;
+			goto err_map_notify;
+		}
 	}
 
 	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
@@ -402,6 +472,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
 	.set_vq_affinity = vp_set_vq_affinity,
 	.get_vq_affinity = vp_get_vq_affinity,
 	.get_shm_region  = vp_get_shm_region,
+	.reset_vq	 = vp_modern_reset_vq,
+	.enable_reset_vq = vp_modern_enable_reset_vq,
 };
 
 static const struct virtio_config_ops virtio_pci_config_ops = {
@@ -420,6 +492,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 	.set_vq_affinity = vp_set_vq_affinity,
 	.get_vq_affinity = vp_get_vq_affinity,
 	.get_shm_region  = vp_get_shm_region,
+	.reset_vq	 = vp_modern_reset_vq,
+	.enable_reset_vq = vp_modern_enable_reset_vq,
 };
 
 /* the PCI probing function */
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b8747df8dc1f..4f6028e1e2d9 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1731,6 +1731,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	vq->vq.vdev = vdev;
 	vq->vq.num_free = num;
 	vq->vq.index = index;
+	vq->vq.reset = false;
 	vq->we_own_ring = true;
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
@@ -2220,6 +2221,7 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
 	vq->vq.vdev = vdev;
 	vq->vq.num_free = vring.num;
 	vq->vq.index = index;
+	vq->vq.reset = false;
 	vq->we_own_ring = false;
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index dd1657c3a488..5d4817d79f3f 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -32,6 +32,7 @@ struct virtqueue {
 	unsigned int index;
 	unsigned int num_free;
 	void *priv;
+	bool reset;
 };
 
 int virtqueue_add_outbuf(struct virtqueue *vq,
-- 
2.31.0

Re: [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET

[Jason Wang]

On Wed, Feb 9, 2022 at 8:29 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> The virtio spec already supports the virtio queue reset function. This patch set
> is to add this function to the kernel. The relevant virtio spec information is
> here:
>
>     https://github.com/oasis-tcs/virtio-spec/issues/124
>
> Also regarding MMIO support for queue reset, I plan to support it after this
> patch is passed.

So I had an idea, we can implement ethtool_set_ringparam() in this
series to get one real users.

But this came into another question: it looks to me current virito-net
just uses the maximum ring size, so it basically means we just can
decrease the number from startup, so I wonder how much value if we
don't limit the startup queue size to a dedicated value.

Thanks

>
> Please review. Thanks.
>
> v4:
>   1. just the code of virtio, without virtio-net
>   2. Performing reset on a queue is divided into these steps:
>     1. reset_vq: reset one vq
>     2. recycle the buffer from vq by virtqueue_detach_unused_buf()
>     3. release the ring of the vq by vring_release_virtqueue()
>     4. enable_reset_vq: re-enable the reset queue
>   3. Simplify the parameters of enable_reset_vq()
>   4. add container structures for virtio_pci_common_cfg
>
> v3:
>   1. keep vq, irq unreleased
>
> Xuan Zhuo (14):
>   virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
>   virtio: queue_reset: add VIRTIO_F_RING_RESET
>   virtio_ring: queue_reset: add function vring_setup_virtqueue()
>   virtio_ring: queue_reset: split: add __vring_init_virtqueue()
>   virtio_ring: queue_reset: split: support enable reset queue
>   virtio_ring: queue_reset: packed: support enable reset queue
>   virtio_ring: queue_reset: extract the release function of the vq ring
>   virtio_ring: queue_reset: add vring_release_virtqueue()
>   virtio: queue_reset: struct virtio_config_ops add callbacks for
>     queue_reset
>   virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
>     option functions
>   virtio_pci: queue_reset: release vq by vp_dev->vqs
>   virtio_pci: queue_reset: setup_vq() support vring_setup_virtqueue()
>   virtio_pci: queue_reset: vp_setup_vq() support ring_num
>   virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
>
>  drivers/virtio/virtio_pci_common.c     |  65 +++++++---
>  drivers/virtio/virtio_pci_common.h     |  11 +-
>  drivers/virtio/virtio_pci_legacy.c     |   5 +-
>  drivers/virtio/virtio_pci_modern.c     |  99 ++++++++++++--
>  drivers/virtio/virtio_pci_modern_dev.c |  36 +++++
>  drivers/virtio/virtio_ring.c           | 173 ++++++++++++++++++-------
>  include/linux/virtio.h                 |   6 +
>  include/linux/virtio_config.h          |  13 ++
>  include/linux/virtio_pci_modern.h      |   2 +
>  include/linux/virtio_ring.h            |  37 ++++--
>  include/uapi/linux/virtio_config.h     |   7 +-
>  include/uapi/linux/virtio_pci.h        |  14 ++
>  12 files changed, 375 insertions(+), 93 deletions(-)
>
> --
> 2.31.0
>

Re: [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET

[Xuan Zhuo]

On Fri, 11 Feb 2022 13:40:52 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Feb 9, 2022 at 8:29 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > The virtio spec already supports the virtio queue reset function. This patch set
> > is to add this function to the kernel. The relevant virtio spec information is
> > here:
> >
> >     https://github.com/oasis-tcs/virtio-spec/issues/124
> >
> > Also regarding MMIO support for queue reset, I plan to support it after this
> > patch is passed.
>
> So I had an idea, we can implement ethtool_set_ringparam() in this
> series to get one real users.

I agree, in fact my local test is using this feature.

>
> But this came into another question: it looks to me current virito-net
> just uses the maximum ring size, so it basically means we just can
> decrease the number from startup, so I wonder how much value if we
> don't limit the startup queue size to a dedicated value.

I also have this consideration, so I want to add a virtio-net module parameter
to specify an initial value.

This initial value also has another meaning. In order to achieve high
performance, the backend can provide a large ring size capability, but we also
hope that the ring size can only be increased through ethtool -G when the user
needs it.

To implement this function, we need to add a new parameter to find_vqs().

If there is no problem, I will bring this function in the next version.

Thannks.


>
> Thanks
>
> >
> > Please review. Thanks.
> >
> > v4:
> >   1. just the code of virtio, without virtio-net
> >   2. Performing reset on a queue is divided into these steps:
> >     1. reset_vq: reset one vq
> >     2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> >     3. release the ring of the vq by vring_release_virtqueue()
> >     4. enable_reset_vq: re-enable the reset queue
> >   3. Simplify the parameters of enable_reset_vq()
> >   4. add container structures for virtio_pci_common_cfg
> >
> > v3:
> >   1. keep vq, irq unreleased
> >
> > Xuan Zhuo (14):
> >   virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
> >   virtio: queue_reset: add VIRTIO_F_RING_RESET
> >   virtio_ring: queue_reset: add function vring_setup_virtqueue()
> >   virtio_ring: queue_reset: split: add __vring_init_virtqueue()
> >   virtio_ring: queue_reset: split: support enable reset queue
> >   virtio_ring: queue_reset: packed: support enable reset queue
> >   virtio_ring: queue_reset: extract the release function of the vq ring
> >   virtio_ring: queue_reset: add vring_release_virtqueue()
> >   virtio: queue_reset: struct virtio_config_ops add callbacks for
> >     queue_reset
> >   virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
> >     option functions
> >   virtio_pci: queue_reset: release vq by vp_dev->vqs
> >   virtio_pci: queue_reset: setup_vq() support vring_setup_virtqueue()
> >   virtio_pci: queue_reset: vp_setup_vq() support ring_num
> >   virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
> >
> >  drivers/virtio/virtio_pci_common.c     |  65 +++++++---
> >  drivers/virtio/virtio_pci_common.h     |  11 +-
> >  drivers/virtio/virtio_pci_legacy.c     |   5 +-
> >  drivers/virtio/virtio_pci_modern.c     |  99 ++++++++++++--
> >  drivers/virtio/virtio_pci_modern_dev.c |  36 +++++
> >  drivers/virtio/virtio_ring.c           | 173 ++++++++++++++++++-------
> >  include/linux/virtio.h                 |   6 +
> >  include/linux/virtio_config.h          |  13 ++
> >  include/linux/virtio_pci_modern.h      |   2 +
> >  include/linux/virtio_ring.h            |  37 ++++--
> >  include/uapi/linux/virtio_config.h     |   7 +-
> >  include/uapi/linux/virtio_pci.h        |  14 ++
> >  12 files changed, 375 insertions(+), 93 deletions(-)
> >
> > --
> > 2.31.0
> >
>

Re: [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset

[Jason Wang]

在 2022/2/9 下午8:28, Xuan Zhuo 写道:
> Performing reset on a queue is divided into four steps:
>
> 1. reset_vq: reset one vq
> 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> 3. release the ring of the vq by vring_release_virtqueue()
> 4. enable_reset_vq: re-enable the reset queue
>
> So add two callbacks reset_vq, enable_reset_vq to struct
> virtio_config_ops.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   include/linux/virtio_config.h | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 4d107ad31149..0d01a64f2576 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -74,6 +74,17 @@ struct virtio_shm_region {
>    * @set_vq_affinity: set the affinity for a virtqueue (optional).
>    * @get_vq_affinity: get the affinity for a virtqueue (optional).
>    * @get_shm_region: get a shared memory region based on the index.
> + * @reset_vq: reset a queue individually


This needs to be marked as optional I think.


> + *	vq: the virtqueue
> + *	Returns 0 on success or error status


It looks to me the caller should also guarantee that the vring is not 
accsed by any functions e.g NAPI.


> + *	After successfully calling this, be sure to call
> + *	virtqueue_detach_unused_buf() to recycle the buffer in the ring, and
> + *	then call vring_release_virtqueue() to release the vq ring.
> + * @enable_reset_vq: enable a reset queue
> + *	vq: the virtqueue
> + *	ring_num: specify ring num for the vq to be re-enabled. 0 means use the
> + *	          default value. MUST be a power of 2.


Note that we don't have power of 2 requirement for packed virtqueue.

And I wonder if it's cleaner to have a find_vq() ops instead to dealing 
with the re-allocation and possible size change, or have a dedicated 
helper to set vring size so driver can do.

reset_vq()

virtqueue_set_vring_size()

enable_reset_vq()


> + *	Returns 0 on success or error status
>    */
>   typedef void vq_callback_t(struct virtqueue *);
>   struct virtio_config_ops {
> @@ -100,6 +111,8 @@ struct virtio_config_ops {
>   			int index);
>   	bool (*get_shm_region)(struct virtio_device *vdev,
>   			       struct virtio_shm_region *region, u8 id);
> +	int (*reset_vq)(struct virtqueue *vq);
> +	int (*enable_reset_vq)(struct virtqueue *vq, u16 ring_num);


Note that the current implement is best-effort, so it's not guarantee 
that we can have a vring with ring_num, we may get less under memory 
pressure or even fail. We probably need to have a pamater to mandate the 
ring_num otherwise user may surprise to see a decreased size of the ring 
when a increasing is actually requested.

Thanks


>   };
>   
>   /* If driver didn't advertise the feature, it will never appear. */

Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Jason Wang]

在 2022/2/9 下午8:29, Xuan Zhuo 写道:
> This patch implements virtio pci support for QUEUE RESET.
>
> Performing reset on a queue is divided into these steps:
>
> 1. reset_vq: reset one vq
> 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> 3. release the ring of the vq by vring_release_virtqueue()
> 4. enable_reset_vq: re-enable the reset queue
>
> This patch implements reset_vq, enable_reset_vq in the pci scenario
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/virtio/virtio_pci_common.c |  8 +--
>   drivers/virtio/virtio_pci_modern.c | 80 ++++++++++++++++++++++++++++--
>   drivers/virtio/virtio_ring.c       |  2 +
>   include/linux/virtio.h             |  1 +
>   4 files changed, 85 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index cb01eb0cb2e4..303637ac4914 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -255,9 +255,11 @@ static void vp_del_vq(struct virtqueue *vq)
>   	struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
>   	unsigned long flags;
>   
> -	spin_lock_irqsave(&vp_dev->lock, flags);
> -	list_del(&info->node);
> -	spin_unlock_irqrestore(&vp_dev->lock, flags);
> +	if (!vq->reset) {
> +		spin_lock_irqsave(&vp_dev->lock, flags);
> +		list_del(&info->node);
> +		spin_unlock_irqrestore(&vp_dev->lock, flags);
> +	}
>   
>   	vp_dev->del_vq(info);
>   	kfree(info);
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index d29d40bf0b45..cc45515eda50 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
>   	if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
>   			pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
>   		__virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> +
> +	if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> +		__virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
>   }
>   
>   /* virtio config->finalize_features() implementation */
> @@ -176,6 +179,70 @@ static void vp_reset(struct virtio_device *vdev)
>   	vp_disable_cbs(vdev);
>   }
>   
> +static int vp_modern_reset_vq(struct virtqueue *vq)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> +	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> +	struct virtio_pci_vq_info *info;
> +	unsigned long flags;
> +	u16 msix_vec;
> +
> +	if (!virtio_has_feature(vq->vdev, VIRTIO_F_RING_RESET))
> +		return -ENOENT;
> +
> +	vp_modern_set_queue_reset(mdev, vq->index);
> +
> +	info = vp_dev->vqs[vq->index];
> +	msix_vec = info->msix_vector;
> +
> +	/* Disable VQ callback. */
> +	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> +		disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));


I think we need a comment to explain why per_vq_mode needs to be dealt 
with differently.


> +
> +	/* delete vq */
> +	spin_lock_irqsave(&vp_dev->lock, flags);
> +	list_del(&info->node);
> +	spin_unlock_irqrestore(&vp_dev->lock, flags);


So I don't see where vring is freed and vp_setup_vq() may try to 
allocate new memory, won't it be a memory leak in this case?

Thanks


> +
> +	vq->reset = true;
> +
> +	INIT_LIST_HEAD(&info->node);
> +
> +	return 0;
> +}
> +
> +static int vp_modern_enable_reset_vq(struct virtqueue *vq, u16 ring_num)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> +	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> +	struct virtio_pci_vq_info *info;
> +	struct virtqueue *_vq;
> +	u16 msix_vec;
> +
> +	if (!vq->reset)
> +		return -EPERM;
> +
> +	/* check queue reset status */
> +	if (vp_modern_get_queue_reset(mdev, vq->index) != 1)
> +		return -EBUSY;
> +
> +	info = vp_dev->vqs[vq->index];
> +	_vq = vp_setup_vq(vq->vdev, vq->index, NULL, NULL, NULL,
> +			 info->msix_vector, ring_num);
> +	if (IS_ERR(_vq)) {
> +		vq->reset = true;
> +		return PTR_ERR(_vq);
> +	}
> +
> +	vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
> +
> +	msix_vec = vp_dev->vqs[vq->index]->msix_vector;
> +	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> +		enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> +
> +	return 0;
> +}
> +
>   static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
>   {
>   	return vp_modern_config_vector(&vp_dev->mdev, vector);
> @@ -231,10 +298,13 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>   				virtqueue_get_avail_addr(vq),
>   				virtqueue_get_used_addr(vq));
>   
> -	vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index, NULL);
>   	if (!vq->priv) {
> -		err = -ENOMEM;
> -		goto err_map_notify;
> +		vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index,
> +								   NULL);
> +		if (!vq->priv) {
> +			err = -ENOMEM;
> +			goto err_map_notify;
> +		}


This seems unrelated or an artifact of previous patches?

Thanks


>   	}
>   
>   	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
> @@ -402,6 +472,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
>   	.set_vq_affinity = vp_set_vq_affinity,
>   	.get_vq_affinity = vp_get_vq_affinity,
>   	.get_shm_region  = vp_get_shm_region,
> +	.reset_vq	 = vp_modern_reset_vq,
> +	.enable_reset_vq = vp_modern_enable_reset_vq,
>   };
>   
>   static const struct virtio_config_ops virtio_pci_config_ops = {
> @@ -420,6 +492,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
>   	.set_vq_affinity = vp_set_vq_affinity,
>   	.get_vq_affinity = vp_get_vq_affinity,
>   	.get_shm_region  = vp_get_shm_region,
> +	.reset_vq	 = vp_modern_reset_vq,
> +	.enable_reset_vq = vp_modern_enable_reset_vq,
>   };
>   
>   /* the PCI probing function */
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index b8747df8dc1f..4f6028e1e2d9 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1731,6 +1731,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
>   	vq->vq.vdev = vdev;
>   	vq->vq.num_free = num;
>   	vq->vq.index = index;
> +	vq->vq.reset = false;
>   	vq->we_own_ring = true;
>   	vq->notify = notify;
>   	vq->weak_barriers = weak_barriers;
> @@ -2220,6 +2221,7 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
>   	vq->vq.vdev = vdev;
>   	vq->vq.num_free = vring.num;
>   	vq->vq.index = index;
> +	vq->vq.reset = false;
>   	vq->we_own_ring = false;
>   	vq->notify = notify;
>   	vq->weak_barriers = weak_barriers;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index dd1657c3a488..5d4817d79f3f 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -32,6 +32,7 @@ struct virtqueue {
>   	unsigned int index;
>   	unsigned int num_free;
>   	void *priv;
> +	bool reset;
>   };
>   
>   int virtqueue_add_outbuf(struct virtqueue *vq,

Re: [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset

[Xuan Zhuo]

On Fri, 11 Feb 2022 14:49:03 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/2/9 下午8:28, Xuan Zhuo 写道:
> > Performing reset on a queue is divided into four steps:
> >
> > 1. reset_vq: reset one vq
> > 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> > 3. release the ring of the vq by vring_release_virtqueue()
> > 4. enable_reset_vq: re-enable the reset queue
> >
> > So add two callbacks reset_vq, enable_reset_vq to struct
> > virtio_config_ops.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   include/linux/virtio_config.h | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > index 4d107ad31149..0d01a64f2576 100644
> > --- a/include/linux/virtio_config.h
> > +++ b/include/linux/virtio_config.h
> > @@ -74,6 +74,17 @@ struct virtio_shm_region {
> >    * @set_vq_affinity: set the affinity for a virtqueue (optional).
> >    * @get_vq_affinity: get the affinity for a virtqueue (optional).
> >    * @get_shm_region: get a shared memory region based on the index.
> > + * @reset_vq: reset a queue individually
>
>
> This needs to be marked as optional I think.

OK.

>
>
> > + *	vq: the virtqueue
> > + *	Returns 0 on success or error status
>
>
> It looks to me the caller should also guarantee that the vring is not
> accsed by any functions e.g NAPI.

OK.

>
>
> > + *	After successfully calling this, be sure to call
> > + *	virtqueue_detach_unused_buf() to recycle the buffer in the ring, and
> > + *	then call vring_release_virtqueue() to release the vq ring.
> > + * @enable_reset_vq: enable a reset queue
> > + *	vq: the virtqueue
> > + *	ring_num: specify ring num for the vq to be re-enabled. 0 means use the
> > + *	          default value. MUST be a power of 2.
>
>
> Note that we don't have power of 2 requirement for packed virtqueue.


So the following check here does not seem reasonable. (virtio_pci_modern.c)

static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
				  struct virtio_pci_vq_info *info,
				  unsigned index,
				  void (*callback)(struct virtqueue *vq),
				  const char *name,
				  bool ctx,
				  u16 msix_vec)
{

	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
	struct virtqueue *vq;
	u16 num;
	int err;

	if (index >= vp_modern_get_num_queues(mdev))
		return ERR_PTR(-ENOENT);

	/* Check if queue is either not available or already active. */
	num = vp_modern_get_queue_size(mdev, index);
	if (!num || vp_modern_get_queue_enable(mdev, index))
		return ERR_PTR(-ENOENT);

	if (num & (num - 1)) {
		dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
		return ERR_PTR(-EINVAL);
	}
	^^^^^^^^^^^^^^^^^^^^^^^

>
> And I wonder if it's cleaner to have a find_vq() ops instead to dealing
> with the re-allocation and possible size change, or have a dedicated
> helper to set vring size so driver can do.
>
> reset_vq()
>
> virtqueue_set_vring_size()
>
> enable_reset_vq()

I like to add a dedicated helper.

>
>
> > + *	Returns 0 on success or error status
> >    */
> >   typedef void vq_callback_t(struct virtqueue *);
> >   struct virtio_config_ops {
> > @@ -100,6 +111,8 @@ struct virtio_config_ops {
> >   			int index);
> >   	bool (*get_shm_region)(struct virtio_device *vdev,
> >   			       struct virtio_shm_region *region, u8 id);
> > +	int (*reset_vq)(struct virtqueue *vq);
> > +	int (*enable_reset_vq)(struct virtqueue *vq, u16 ring_num);
>
>
> Note that the current implement is best-effort, so it's not guarantee
> that we can have a vring with ring_num, we may get less under memory
> pressure or even fail. We probably need to have a pamater to mandate the
> ring_num otherwise user may surprise to see a decreased size of the ring
> when a increasing is actually requested.

1. We can add a helper to specify max ring num.
2. Or after specifying ring num, in case of failure, return directly.

I prefer #1

Thanks.

>
> Thanks
>
>
> >   };
> >
> >   /* If driver didn't advertise the feature, it will never appear. */
>

Re: [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET

[Jason Wang]

On Fri, Feb 11, 2022 at 2:34 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Fri, 11 Feb 2022 13:40:52 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Wed, Feb 9, 2022 at 8:29 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > The virtio spec already supports the virtio queue reset function. This patch set
> > > is to add this function to the kernel. The relevant virtio spec information is
> > > here:
> > >
> > >     https://github.com/oasis-tcs/virtio-spec/issues/124
> > >
> > > Also regarding MMIO support for queue reset, I plan to support it after this
> > > patch is passed.
> >
> > So I had an idea, we can implement ethtool_set_ringparam() in this
> > series to get one real users.
>
> I agree, in fact my local test is using this feature.

Right.

>
> >
> > But this came into another question: it looks to me current virito-net
> > just uses the maximum ring size, so it basically means we just can
> > decrease the number from startup, so I wonder how much value if we
> > don't limit the startup queue size to a dedicated value.
>
> I also have this consideration, so I want to add a virtio-net module parameter
> to specify an initial value.

But module parameters are not user friendly. Let it be changed via
ethtool should be sufficient.

>
> This initial value also has another meaning. In order to achieve high
> performance, the backend can provide a large ring size capability, but we also
> hope that the ring size can only be increased through ethtool -G when the user
> needs it.
>
> To implement this function, we need to add a new parameter to find_vqs().
>
> If there is no problem, I will bring this function in the next version.

I think so, and we need to choose a sane default value. I guess
something up to 1024 should be sufficient.

Thanks

>
> Thannks.
>
>
> >
> > Thanks
> >
> > >
> > > Please review. Thanks.
> > >
> > > v4:
> > >   1. just the code of virtio, without virtio-net
> > >   2. Performing reset on a queue is divided into these steps:
> > >     1. reset_vq: reset one vq
> > >     2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> > >     3. release the ring of the vq by vring_release_virtqueue()
> > >     4. enable_reset_vq: re-enable the reset queue
> > >   3. Simplify the parameters of enable_reset_vq()
> > >   4. add container structures for virtio_pci_common_cfg
> > >
> > > v3:
> > >   1. keep vq, irq unreleased
> > >
> > > Xuan Zhuo (14):
> > >   virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
> > >   virtio: queue_reset: add VIRTIO_F_RING_RESET
> > >   virtio_ring: queue_reset: add function vring_setup_virtqueue()
> > >   virtio_ring: queue_reset: split: add __vring_init_virtqueue()
> > >   virtio_ring: queue_reset: split: support enable reset queue
> > >   virtio_ring: queue_reset: packed: support enable reset queue
> > >   virtio_ring: queue_reset: extract the release function of the vq ring
> > >   virtio_ring: queue_reset: add vring_release_virtqueue()
> > >   virtio: queue_reset: struct virtio_config_ops add callbacks for
> > >     queue_reset
> > >   virtio_pci: queue_reset: update struct virtio_pci_common_cfg and
> > >     option functions
> > >   virtio_pci: queue_reset: release vq by vp_dev->vqs
> > >   virtio_pci: queue_reset: setup_vq() support vring_setup_virtqueue()
> > >   virtio_pci: queue_reset: vp_setup_vq() support ring_num
> > >   virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
> > >
> > >  drivers/virtio/virtio_pci_common.c     |  65 +++++++---
> > >  drivers/virtio/virtio_pci_common.h     |  11 +-
> > >  drivers/virtio/virtio_pci_legacy.c     |   5 +-
> > >  drivers/virtio/virtio_pci_modern.c     |  99 ++++++++++++--
> > >  drivers/virtio/virtio_pci_modern_dev.c |  36 +++++
> > >  drivers/virtio/virtio_ring.c           | 173 ++++++++++++++++++-------
> > >  include/linux/virtio.h                 |   6 +
> > >  include/linux/virtio_config.h          |  13 ++
> > >  include/linux/virtio_pci_modern.h      |   2 +
> > >  include/linux/virtio_ring.h            |  37 ++++--
> > >  include/uapi/linux/virtio_config.h     |   7 +-
> > >  include/uapi/linux/virtio_pci.h        |  14 ++
> > >  12 files changed, 375 insertions(+), 93 deletions(-)
> > >
> > > --
> > > 2.31.0
> > >
> >
>

Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Xuan Zhuo]

On Fri, 11 Feb 2022 15:05:40 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/2/9 下午8:29, Xuan Zhuo 写道:
> > This patch implements virtio pci support for QUEUE RESET.
> >
> > Performing reset on a queue is divided into these steps:
> >
> > 1. reset_vq: reset one vq
> > 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> > 3. release the ring of the vq by vring_release_virtqueue()
> > 4. enable_reset_vq: re-enable the reset queue
> >
> > This patch implements reset_vq, enable_reset_vq in the pci scenario
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_pci_common.c |  8 +--
> >   drivers/virtio/virtio_pci_modern.c | 80 ++++++++++++++++++++++++++++--
> >   drivers/virtio/virtio_ring.c       |  2 +
> >   include/linux/virtio.h             |  1 +
> >   4 files changed, 85 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index cb01eb0cb2e4..303637ac4914 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -255,9 +255,11 @@ static void vp_del_vq(struct virtqueue *vq)
> >   	struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> >   	unsigned long flags;
> >
> > -	spin_lock_irqsave(&vp_dev->lock, flags);
> > -	list_del(&info->node);
> > -	spin_unlock_irqrestore(&vp_dev->lock, flags);
> > +	if (!vq->reset) {
> > +		spin_lock_irqsave(&vp_dev->lock, flags);
> > +		list_del(&info->node);
> > +		spin_unlock_irqrestore(&vp_dev->lock, flags);
> > +	}
> >
> >   	vp_dev->del_vq(info);
> >   	kfree(info);
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index d29d40bf0b45..cc45515eda50 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
> >   	if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> >   			pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> >   		__virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > +
> > +	if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> > +		__virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> >   }
> >
> >   /* virtio config->finalize_features() implementation */
> > @@ -176,6 +179,70 @@ static void vp_reset(struct virtio_device *vdev)
> >   	vp_disable_cbs(vdev);
> >   }
> >
> > +static int vp_modern_reset_vq(struct virtqueue *vq)
> > +{
> > +	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > +	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > +	struct virtio_pci_vq_info *info;
> > +	unsigned long flags;
> > +	u16 msix_vec;
> > +
> > +	if (!virtio_has_feature(vq->vdev, VIRTIO_F_RING_RESET))
> > +		return -ENOENT;
> > +
> > +	vp_modern_set_queue_reset(mdev, vq->index);
> > +
> > +	info = vp_dev->vqs[vq->index];
> > +	msix_vec = info->msix_vector;
> > +
> > +	/* Disable VQ callback. */
> > +	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > +		disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
>
>
> I think we need a comment to explain why per_vq_mode needs to be dealt
> with differently.

OK.

>
>
> > +
> > +	/* delete vq */
> > +	spin_lock_irqsave(&vp_dev->lock, flags);
> > +	list_del(&info->node);
> > +	spin_unlock_irqrestore(&vp_dev->lock, flags);
>
>
> So I don't see where vring is freed and vp_setup_vq() may try to
> allocate new memory, won't it be a memory leak in this case?

1. reset_vq: reset one vq
2. recycle the buffer from vq by virtqueue_detach_unused_buf()
3. release the ring of the vq by vring_release_virtqueue()
4. enable_reset_vq: re-enable the reset queue

vring_release_virtqueue() (#8 patch) will release the vring.
That is called by the driver.

I think I should add a check to vp_modern_enable_reset_vq() that
vring_release_virtqueue() has already been called.

Thanks

>
> Thanks
>
>
> > +
> > +	vq->reset = true;
> > +
> > +	INIT_LIST_HEAD(&info->node);
> > +
> > +	return 0;
> > +}
> > +
> > +static int vp_modern_enable_reset_vq(struct virtqueue *vq, u16 ring_num)
> > +{
> > +	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > +	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > +	struct virtio_pci_vq_info *info;
> > +	struct virtqueue *_vq;
> > +	u16 msix_vec;
> > +
> > +	if (!vq->reset)
> > +		return -EPERM;
> > +
> > +	/* check queue reset status */
> > +	if (vp_modern_get_queue_reset(mdev, vq->index) != 1)
> > +		return -EBUSY;
> > +
> > +	info = vp_dev->vqs[vq->index];
> > +	_vq = vp_setup_vq(vq->vdev, vq->index, NULL, NULL, NULL,
> > +			 info->msix_vector, ring_num);
> > +	if (IS_ERR(_vq)) {
> > +		vq->reset = true;
> > +		return PTR_ERR(_vq);
> > +	}
> > +
> > +	vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
> > +
> > +	msix_vec = vp_dev->vqs[vq->index]->msix_vector;
> > +	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > +		enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> > +
> > +	return 0;
> > +}
> > +
> >   static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> >   {
> >   	return vp_modern_config_vector(&vp_dev->mdev, vector);
> > @@ -231,10 +298,13 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> >   				virtqueue_get_avail_addr(vq),
> >   				virtqueue_get_used_addr(vq));
> >
> > -	vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index, NULL);
> >   	if (!vq->priv) {
> > -		err = -ENOMEM;
> > -		goto err_map_notify;
> > +		vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index,
> > +								   NULL);
> > +		if (!vq->priv) {
> > +			err = -ENOMEM;
> > +			goto err_map_notify;
> > +		}
>
>
> This seems unrelated or an artifact of previous patches?
>
> Thanks
>
>
> >   	}
> >
> >   	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
> > @@ -402,6 +472,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> >   	.set_vq_affinity = vp_set_vq_affinity,
> >   	.get_vq_affinity = vp_get_vq_affinity,
> >   	.get_shm_region  = vp_get_shm_region,
> > +	.reset_vq	 = vp_modern_reset_vq,
> > +	.enable_reset_vq = vp_modern_enable_reset_vq,
> >   };
> >
> >   static const struct virtio_config_ops virtio_pci_config_ops = {
> > @@ -420,6 +492,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> >   	.set_vq_affinity = vp_set_vq_affinity,
> >   	.get_vq_affinity = vp_get_vq_affinity,
> >   	.get_shm_region  = vp_get_shm_region,
> > +	.reset_vq	 = vp_modern_reset_vq,
> > +	.enable_reset_vq = vp_modern_enable_reset_vq,
> >   };
> >
> >   /* the PCI probing function */
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index b8747df8dc1f..4f6028e1e2d9 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1731,6 +1731,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >   	vq->vq.vdev = vdev;
> >   	vq->vq.num_free = num;
> >   	vq->vq.index = index;
> > +	vq->vq.reset = false;
> >   	vq->we_own_ring = true;
> >   	vq->notify = notify;
> >   	vq->weak_barriers = weak_barriers;
> > @@ -2220,6 +2221,7 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
> >   	vq->vq.vdev = vdev;
> >   	vq->vq.num_free = vring.num;
> >   	vq->vq.index = index;
> > +	vq->vq.reset = false;
> >   	vq->we_own_ring = false;
> >   	vq->notify = notify;
> >   	vq->weak_barriers = weak_barriers;
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index dd1657c3a488..5d4817d79f3f 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -32,6 +32,7 @@ struct virtqueue {
> >   	unsigned int index;
> >   	unsigned int num_free;
> >   	void *priv;
> > +	bool reset;
> >   };
> >
> >   int virtqueue_add_outbuf(struct virtqueue *vq,
>

Re: [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset

[Jason Wang]

On Fri, Feb 11, 2022 at 3:13 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Fri, 11 Feb 2022 14:49:03 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/2/9 下午8:28, Xuan Zhuo 写道:
> > > Performing reset on a queue is divided into four steps:
> > >
> > > 1. reset_vq: reset one vq
> > > 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> > > 3. release the ring of the vq by vring_release_virtqueue()
> > > 4. enable_reset_vq: re-enable the reset queue
> > >
> > > So add two callbacks reset_vq, enable_reset_vq to struct
> > > virtio_config_ops.
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >   include/linux/virtio_config.h | 13 +++++++++++++
> > >   1 file changed, 13 insertions(+)
> > >
> > > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> > > index 4d107ad31149..0d01a64f2576 100644
> > > --- a/include/linux/virtio_config.h
> > > +++ b/include/linux/virtio_config.h
> > > @@ -74,6 +74,17 @@ struct virtio_shm_region {
> > >    * @set_vq_affinity: set the affinity for a virtqueue (optional).
> > >    * @get_vq_affinity: get the affinity for a virtqueue (optional).
> > >    * @get_shm_region: get a shared memory region based on the index.
> > > + * @reset_vq: reset a queue individually
> >
> >
> > This needs to be marked as optional I think.
>
> OK.
>
> >
> >
> > > + * vq: the virtqueue
> > > + * Returns 0 on success or error status
> >
> >
> > It looks to me the caller should also guarantee that the vring is not
> > accsed by any functions e.g NAPI.
>
> OK.
>
> >
> >
> > > + * After successfully calling this, be sure to call
> > > + * virtqueue_detach_unused_buf() to recycle the buffer in the ring, and
> > > + * then call vring_release_virtqueue() to release the vq ring.
> > > + * @enable_reset_vq: enable a reset queue
> > > + * vq: the virtqueue
> > > + * ring_num: specify ring num for the vq to be re-enabled. 0 means use the
> > > + *           default value. MUST be a power of 2.
> >
> >
> > Note that we don't have power of 2 requirement for packed virtqueue.
>
>
> So the following check here does not seem reasonable. (virtio_pci_modern.c)

Rethink about this, looks like I was wrong.

Though packed virtqueue doesn't mandate power of 2, the actual queue
format should be invisible to a driver.

So forcing power of 2 is correct (since it was required by split virtqueue)

>
> static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>                                   struct virtio_pci_vq_info *info,
>                                   unsigned index,
>                                   void (*callback)(struct virtqueue *vq),
>                                   const char *name,
>                                   bool ctx,
>                                   u16 msix_vec)
> {
>
>         struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
>         struct virtqueue *vq;
>         u16 num;
>         int err;
>
>         if (index >= vp_modern_get_num_queues(mdev))
>                 return ERR_PTR(-ENOENT);
>
>         /* Check if queue is either not available or already active. */
>         num = vp_modern_get_queue_size(mdev, index);
>         if (!num || vp_modern_get_queue_enable(mdev, index))
>                 return ERR_PTR(-ENOENT);
>
>         if (num & (num - 1)) {
>                 dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
>                 return ERR_PTR(-EINVAL);
>         }
>         ^^^^^^^^^^^^^^^^^^^^^^^
>
> >
> > And I wonder if it's cleaner to have a find_vq() ops instead to dealing
> > with the re-allocation and possible size change, or have a dedicated
> > helper to set vring size so driver can do.
> >
> > reset_vq()
> >
> > virtqueue_set_vring_size()
> >
> > enable_reset_vq()
>
> I like to add a dedicated helper.
>
> >
> >
> > > + * Returns 0 on success or error status
> > >    */
> > >   typedef void vq_callback_t(struct virtqueue *);
> > >   struct virtio_config_ops {
> > > @@ -100,6 +111,8 @@ struct virtio_config_ops {
> > >                     int index);
> > >     bool (*get_shm_region)(struct virtio_device *vdev,
> > >                            struct virtio_shm_region *region, u8 id);
> > > +   int (*reset_vq)(struct virtqueue *vq);
> > > +   int (*enable_reset_vq)(struct virtqueue *vq, u16 ring_num);
> >
> >
> > Note that the current implement is best-effort, so it's not guarantee
> > that we can have a vring with ring_num, we may get less under memory
> > pressure or even fail. We probably need to have a pamater to mandate the
> > ring_num otherwise user may surprise to see a decreased size of the ring
> > when a increasing is actually requested.
>
> 1. We can add a helper to specify max ring num.
> 2. Or after specifying ring num, in case of failure, return directly.
>
> I prefer #1

That's fine.

Thanks

>
> Thanks.
>
> >
> > Thanks
> >
> >
> > >   };
> > >
> > >   /* If driver didn't advertise the feature, it will never appear. */
> >
>

Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Jason Wang]

On Fri, Feb 11, 2022 at 3:24 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Fri, 11 Feb 2022 15:05:40 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/2/9 下午8:29, Xuan Zhuo 写道:
> > > This patch implements virtio pci support for QUEUE RESET.
> > >
> > > Performing reset on a queue is divided into these steps:
> > >
> > > 1. reset_vq: reset one vq
> > > 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> > > 3. release the ring of the vq by vring_release_virtqueue()
> > > 4. enable_reset_vq: re-enable the reset queue
> > >
> > > This patch implements reset_vq, enable_reset_vq in the pci scenario
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >   drivers/virtio/virtio_pci_common.c |  8 +--
> > >   drivers/virtio/virtio_pci_modern.c | 80 ++++++++++++++++++++++++++++--
> > >   drivers/virtio/virtio_ring.c       |  2 +
> > >   include/linux/virtio.h             |  1 +
> > >   4 files changed, 85 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > index cb01eb0cb2e4..303637ac4914 100644
> > > --- a/drivers/virtio/virtio_pci_common.c
> > > +++ b/drivers/virtio/virtio_pci_common.c
> > > @@ -255,9 +255,11 @@ static void vp_del_vq(struct virtqueue *vq)
> > >     struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> > >     unsigned long flags;
> > >
> > > -   spin_lock_irqsave(&vp_dev->lock, flags);
> > > -   list_del(&info->node);
> > > -   spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > +   if (!vq->reset) {
> > > +           spin_lock_irqsave(&vp_dev->lock, flags);
> > > +           list_del(&info->node);
> > > +           spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > +   }
> > >
> > >     vp_dev->del_vq(info);
> > >     kfree(info);
> > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > index d29d40bf0b45..cc45515eda50 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > >     if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > >                     pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > >             __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > +
> > > +   if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> > > +           __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> > >   }
> > >
> > >   /* virtio config->finalize_features() implementation */
> > > @@ -176,6 +179,70 @@ static void vp_reset(struct virtio_device *vdev)
> > >     vp_disable_cbs(vdev);
> > >   }
> > >
> > > +static int vp_modern_reset_vq(struct virtqueue *vq)
> > > +{
> > > +   struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > +   struct virtio_pci_vq_info *info;
> > > +   unsigned long flags;
> > > +   u16 msix_vec;
> > > +
> > > +   if (!virtio_has_feature(vq->vdev, VIRTIO_F_RING_RESET))
> > > +           return -ENOENT;
> > > +
> > > +   vp_modern_set_queue_reset(mdev, vq->index);
> > > +
> > > +   info = vp_dev->vqs[vq->index];
> > > +   msix_vec = info->msix_vector;
> > > +
> > > +   /* Disable VQ callback. */
> > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > +           disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> >
> >
> > I think we need a comment to explain why per_vq_mode needs to be dealt
> > with differently.
>
> OK.
>
> >
> >
> > > +
> > > +   /* delete vq */
> > > +   spin_lock_irqsave(&vp_dev->lock, flags);
> > > +   list_del(&info->node);
> > > +   spin_unlock_irqrestore(&vp_dev->lock, flags);
> >
> >
> > So I don't see where vring is freed and vp_setup_vq() may try to
> > allocate new memory, won't it be a memory leak in this case?
>
> 1. reset_vq: reset one vq
> 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> 3. release the ring of the vq by vring_release_virtqueue()
> 4. enable_reset_vq: re-enable the reset queue
>
> vring_release_virtqueue() (#8 patch) will release the vring.
> That is called by the driver.
>
> I think I should add a check to vp_modern_enable_reset_vq() that
> vring_release_virtqueue() has already been called.

I wonder if we can have a better API.

Consider we know there's a requirement of vring re-allocation. I
wonder how about adding per vq config ops like:

del_vq()
find_vq()

We can limit them only after a virtqueue is reset before it is
enabled. We can have a full allocation on the resources e.g interrupt
(if some codes could be reused).

Then a driver can do
reset_vq()
detach_unused_buf()
del_vq
find_vq() /* with new parameters like ring_num and others like find_vqs() */
enable_reset_vq()

?

Thanks

>
> Thanks
>
> >
> > Thanks
> >
> >
> > > +
> > > +   vq->reset = true;
> > > +
> > > +   INIT_LIST_HEAD(&info->node);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static int vp_modern_enable_reset_vq(struct virtqueue *vq, u16 ring_num)
> > > +{
> > > +   struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > +   struct virtio_pci_vq_info *info;
> > > +   struct virtqueue *_vq;
> > > +   u16 msix_vec;
> > > +
> > > +   if (!vq->reset)
> > > +           return -EPERM;
> > > +
> > > +   /* check queue reset status */
> > > +   if (vp_modern_get_queue_reset(mdev, vq->index) != 1)
> > > +           return -EBUSY;
> > > +
> > > +   info = vp_dev->vqs[vq->index];
> > > +   _vq = vp_setup_vq(vq->vdev, vq->index, NULL, NULL, NULL,
> > > +                    info->msix_vector, ring_num);
> > > +   if (IS_ERR(_vq)) {
> > > +           vq->reset = true;
> > > +           return PTR_ERR(_vq);
> > > +   }
> > > +
> > > +   vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
> > > +
> > > +   msix_vec = vp_dev->vqs[vq->index]->msix_vector;
> > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > +           enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> > > +
> > > +   return 0;
> > > +}
> > > +
> > >   static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> > >   {
> > >     return vp_modern_config_vector(&vp_dev->mdev, vector);
> > > @@ -231,10 +298,13 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> > >                             virtqueue_get_avail_addr(vq),
> > >                             virtqueue_get_used_addr(vq));
> > >
> > > -   vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index, NULL);
> > >     if (!vq->priv) {
> > > -           err = -ENOMEM;
> > > -           goto err_map_notify;
> > > +           vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index,
> > > +                                                              NULL);
> > > +           if (!vq->priv) {
> > > +                   err = -ENOMEM;
> > > +                   goto err_map_notify;
> > > +           }
> >
> >
> > This seems unrelated or an artifact of previous patches?
> >
> > Thanks
> >
> >
> > >     }
> > >
> > >     if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
> > > @@ -402,6 +472,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> > >     .set_vq_affinity = vp_set_vq_affinity,
> > >     .get_vq_affinity = vp_get_vq_affinity,
> > >     .get_shm_region  = vp_get_shm_region,
> > > +   .reset_vq        = vp_modern_reset_vq,
> > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > >   };
> > >
> > >   static const struct virtio_config_ops virtio_pci_config_ops = {
> > > @@ -420,6 +492,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> > >     .set_vq_affinity = vp_set_vq_affinity,
> > >     .get_vq_affinity = vp_get_vq_affinity,
> > >     .get_shm_region  = vp_get_shm_region,
> > > +   .reset_vq        = vp_modern_reset_vq,
> > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > >   };
> > >
> > >   /* the PCI probing function */
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index b8747df8dc1f..4f6028e1e2d9 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -1731,6 +1731,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> > >     vq->vq.vdev = vdev;
> > >     vq->vq.num_free = num;
> > >     vq->vq.index = index;
> > > +   vq->vq.reset = false;
> > >     vq->we_own_ring = true;
> > >     vq->notify = notify;
> > >     vq->weak_barriers = weak_barriers;
> > > @@ -2220,6 +2221,7 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
> > >     vq->vq.vdev = vdev;
> > >     vq->vq.num_free = vring.num;
> > >     vq->vq.index = index;
> > > +   vq->vq.reset = false;
> > >     vq->we_own_ring = false;
> > >     vq->notify = notify;
> > >     vq->weak_barriers = weak_barriers;
> > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > index dd1657c3a488..5d4817d79f3f 100644
> > > --- a/include/linux/virtio.h
> > > +++ b/include/linux/virtio.h
> > > @@ -32,6 +32,7 @@ struct virtqueue {
> > >     unsigned int index;
> > >     unsigned int num_free;
> > >     void *priv;
> > > +   bool reset;
> > >   };
> > >
> > >   int virtqueue_add_outbuf(struct virtqueue *vq,
> >
>

Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Xuan Zhuo]

On Fri, 11 Feb 2022 15:45:46 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Fri, Feb 11, 2022 at 3:24 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Fri, 11 Feb 2022 15:05:40 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > 在 2022/2/9 下午8:29, Xuan Zhuo 写道:
> > > > This patch implements virtio pci support for QUEUE RESET.
> > > >
> > > > Performing reset on a queue is divided into these steps:
> > > >
> > > > 1. reset_vq: reset one vq
> > > > 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> > > > 3. release the ring of the vq by vring_release_virtqueue()
> > > > 4. enable_reset_vq: re-enable the reset queue
> > > >
> > > > This patch implements reset_vq, enable_reset_vq in the pci scenario
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >   drivers/virtio/virtio_pci_common.c |  8 +--
> > > >   drivers/virtio/virtio_pci_modern.c | 80 ++++++++++++++++++++++++++++--
> > > >   drivers/virtio/virtio_ring.c       |  2 +
> > > >   include/linux/virtio.h             |  1 +
> > > >   4 files changed, 85 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > index cb01eb0cb2e4..303637ac4914 100644
> > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > @@ -255,9 +255,11 @@ static void vp_del_vq(struct virtqueue *vq)
> > > >     struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> > > >     unsigned long flags;
> > > >
> > > > -   spin_lock_irqsave(&vp_dev->lock, flags);
> > > > -   list_del(&info->node);
> > > > -   spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > > +   if (!vq->reset) {
> > > > +           spin_lock_irqsave(&vp_dev->lock, flags);
> > > > +           list_del(&info->node);
> > > > +           spin_unlock_irqrestore(&vp_dev->lock, flags);
> > > > +   }
> > > >
> > > >     vp_dev->del_vq(info);
> > > >     kfree(info);
> > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > index d29d40bf0b45..cc45515eda50 100644
> > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > > >     if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > >                     pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > > >             __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > +
> > > > +   if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> > > > +           __virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> > > >   }
> > > >
> > > >   /* virtio config->finalize_features() implementation */
> > > > @@ -176,6 +179,70 @@ static void vp_reset(struct virtio_device *vdev)
> > > >     vp_disable_cbs(vdev);
> > > >   }
> > > >
> > > > +static int vp_modern_reset_vq(struct virtqueue *vq)
> > > > +{
> > > > +   struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > > +   struct virtio_pci_vq_info *info;
> > > > +   unsigned long flags;
> > > > +   u16 msix_vec;
> > > > +
> > > > +   if (!virtio_has_feature(vq->vdev, VIRTIO_F_RING_RESET))
> > > > +           return -ENOENT;
> > > > +
> > > > +   vp_modern_set_queue_reset(mdev, vq->index);
> > > > +
> > > > +   info = vp_dev->vqs[vq->index];
> > > > +   msix_vec = info->msix_vector;
> > > > +
> > > > +   /* Disable VQ callback. */
> > > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > > +           disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> > >
> > >
> > > I think we need a comment to explain why per_vq_mode needs to be dealt
> > > with differently.
> >
> > OK.
> >
> > >
> > >
> > > > +
> > > > +   /* delete vq */
> > > > +   spin_lock_irqsave(&vp_dev->lock, flags);
> > > > +   list_del(&info->node);
> > > > +   spin_unlock_irqrestore(&vp_dev->lock, flags);
> > >
> > >
> > > So I don't see where vring is freed and vp_setup_vq() may try to
> > > allocate new memory, won't it be a memory leak in this case?
> >
> > 1. reset_vq: reset one vq
> > 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> > 3. release the ring of the vq by vring_release_virtqueue()
> > 4. enable_reset_vq: re-enable the reset queue
> >
> > vring_release_virtqueue() (#8 patch) will release the vring.
> > That is called by the driver.
> >
> > I think I should add a check to vp_modern_enable_reset_vq() that
> > vring_release_virtqueue() has already been called.
>
> I wonder if we can have a better API.
>
> Consider we know there's a requirement of vring re-allocation. I
> wonder how about adding per vq config ops like:
>
> del_vq()
> find_vq()
>
> We can limit them only after a virtqueue is reset before it is
> enabled. We can have a full allocation on the resources e.g interrupt
> (if some codes could be reused).

Do you mean including interrupts, vq are all released?

Actually my first version did this, and Michael didn't like it, so I changed to
only release vring.

Thanks.

>
> Then a driver can do
> reset_vq()
> detach_unused_buf()
> del_vq
> find_vq() /* with new parameters like ring_num and others like find_vqs() */
> enable_reset_vq()
>
> ?
>
> Thanks
>
> >
> > Thanks
> >
> > >
> > > Thanks
> > >
> > >
> > > > +
> > > > +   vq->reset = true;
> > > > +
> > > > +   INIT_LIST_HEAD(&info->node);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static int vp_modern_enable_reset_vq(struct virtqueue *vq, u16 ring_num)
> > > > +{
> > > > +   struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > > > +   struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > > > +   struct virtio_pci_vq_info *info;
> > > > +   struct virtqueue *_vq;
> > > > +   u16 msix_vec;
> > > > +
> > > > +   if (!vq->reset)
> > > > +           return -EPERM;
> > > > +
> > > > +   /* check queue reset status */
> > > > +   if (vp_modern_get_queue_reset(mdev, vq->index) != 1)
> > > > +           return -EBUSY;
> > > > +
> > > > +   info = vp_dev->vqs[vq->index];
> > > > +   _vq = vp_setup_vq(vq->vdev, vq->index, NULL, NULL, NULL,
> > > > +                    info->msix_vector, ring_num);
> > > > +   if (IS_ERR(_vq)) {
> > > > +           vq->reset = true;
> > > > +           return PTR_ERR(_vq);
> > > > +   }
> > > > +
> > > > +   vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
> > > > +
> > > > +   msix_vec = vp_dev->vqs[vq->index]->msix_vector;
> > > > +   if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > > > +           enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > >   static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> > > >   {
> > > >     return vp_modern_config_vector(&vp_dev->mdev, vector);
> > > > @@ -231,10 +298,13 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> > > >                             virtqueue_get_avail_addr(vq),
> > > >                             virtqueue_get_used_addr(vq));
> > > >
> > > > -   vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index, NULL);
> > > >     if (!vq->priv) {
> > > > -           err = -ENOMEM;
> > > > -           goto err_map_notify;
> > > > +           vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index,
> > > > +                                                              NULL);
> > > > +           if (!vq->priv) {
> > > > +                   err = -ENOMEM;
> > > > +                   goto err_map_notify;
> > > > +           }
> > >
> > >
> > > This seems unrelated or an artifact of previous patches?
> > >
> > > Thanks
> > >
> > >
> > > >     }
> > > >
> > > >     if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
> > > > @@ -402,6 +472,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> > > >     .set_vq_affinity = vp_set_vq_affinity,
> > > >     .get_vq_affinity = vp_get_vq_affinity,
> > > >     .get_shm_region  = vp_get_shm_region,
> > > > +   .reset_vq        = vp_modern_reset_vq,
> > > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > > >   };
> > > >
> > > >   static const struct virtio_config_ops virtio_pci_config_ops = {
> > > > @@ -420,6 +492,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> > > >     .set_vq_affinity = vp_set_vq_affinity,
> > > >     .get_vq_affinity = vp_get_vq_affinity,
> > > >     .get_shm_region  = vp_get_shm_region,
> > > > +   .reset_vq        = vp_modern_reset_vq,
> > > > +   .enable_reset_vq = vp_modern_enable_reset_vq,
> > > >   };
> > > >
> > > >   /* the PCI probing function */
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index b8747df8dc1f..4f6028e1e2d9 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -1731,6 +1731,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> > > >     vq->vq.vdev = vdev;
> > > >     vq->vq.num_free = num;
> > > >     vq->vq.index = index;
> > > > +   vq->vq.reset = false;
> > > >     vq->we_own_ring = true;
> > > >     vq->notify = notify;
> > > >     vq->weak_barriers = weak_barriers;
> > > > @@ -2220,6 +2221,7 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
> > > >     vq->vq.vdev = vdev;
> > > >     vq->vq.num_free = vring.num;
> > > >     vq->vq.index = index;
> > > > +   vq->vq.reset = false;
> > > >     vq->we_own_ring = false;
> > > >     vq->notify = notify;
> > > >     vq->weak_barriers = weak_barriers;
> > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > index dd1657c3a488..5d4817d79f3f 100644
> > > > --- a/include/linux/virtio.h
> > > > +++ b/include/linux/virtio.h
> > > > @@ -32,6 +32,7 @@ struct virtqueue {
> > > >     unsigned int index;
> > > >     unsigned int num_free;
> > > >     void *priv;
> > > > +   bool reset;
> > > >   };
> > > >
> > > >   int virtqueue_add_outbuf(struct virtqueue *vq,
> > >
> >
>

Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET

[Xuan Zhuo]

On Fri, 11 Feb 2022 15:05:40 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/2/9 下午8:29, Xuan Zhuo 写道:
> > This patch implements virtio pci support for QUEUE RESET.
> >
> > Performing reset on a queue is divided into these steps:
> >
> > 1. reset_vq: reset one vq
> > 2. recycle the buffer from vq by virtqueue_detach_unused_buf()
> > 3. release the ring of the vq by vring_release_virtqueue()
> > 4. enable_reset_vq: re-enable the reset queue
> >
> > This patch implements reset_vq, enable_reset_vq in the pci scenario
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_pci_common.c |  8 +--
> >   drivers/virtio/virtio_pci_modern.c | 80 ++++++++++++++++++++++++++++--
> >   drivers/virtio/virtio_ring.c       |  2 +
> >   include/linux/virtio.h             |  1 +
> >   4 files changed, 85 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index cb01eb0cb2e4..303637ac4914 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -255,9 +255,11 @@ static void vp_del_vq(struct virtqueue *vq)
> >   	struct virtio_pci_vq_info *info = vp_dev->vqs[vq->index];
> >   	unsigned long flags;
> >
> > -	spin_lock_irqsave(&vp_dev->lock, flags);
> > -	list_del(&info->node);
> > -	spin_unlock_irqrestore(&vp_dev->lock, flags);
> > +	if (!vq->reset) {
> > +		spin_lock_irqsave(&vp_dev->lock, flags);
> > +		list_del(&info->node);
> > +		spin_unlock_irqrestore(&vp_dev->lock, flags);
> > +	}
> >
> >   	vp_dev->del_vq(info);
> >   	kfree(info);
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index d29d40bf0b45..cc45515eda50 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -34,6 +34,9 @@ static void vp_transport_features(struct virtio_device *vdev, u64 features)
> >   	if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> >   			pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> >   		__virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > +
> > +	if (features & BIT_ULL(VIRTIO_F_RING_RESET))
> > +		__virtio_set_bit(vdev, VIRTIO_F_RING_RESET);
> >   }
> >
> >   /* virtio config->finalize_features() implementation */
> > @@ -176,6 +179,70 @@ static void vp_reset(struct virtio_device *vdev)
> >   	vp_disable_cbs(vdev);
> >   }
> >
> > +static int vp_modern_reset_vq(struct virtqueue *vq)
> > +{
> > +	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > +	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > +	struct virtio_pci_vq_info *info;
> > +	unsigned long flags;
> > +	u16 msix_vec;
> > +
> > +	if (!virtio_has_feature(vq->vdev, VIRTIO_F_RING_RESET))
> > +		return -ENOENT;
> > +
> > +	vp_modern_set_queue_reset(mdev, vq->index);
> > +
> > +	info = vp_dev->vqs[vq->index];
> > +	msix_vec = info->msix_vector;
> > +
> > +	/* Disable VQ callback. */
> > +	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > +		disable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
>
>
> I think we need a comment to explain why per_vq_mode needs to be dealt
> with differently.
>
>
> > +
> > +	/* delete vq */
> > +	spin_lock_irqsave(&vp_dev->lock, flags);
> > +	list_del(&info->node);
> > +	spin_unlock_irqrestore(&vp_dev->lock, flags);
>
>
> So I don't see where vring is freed and vp_setup_vq() may try to
> allocate new memory, won't it be a memory leak in this case?
>
> Thanks
>
>
> > +
> > +	vq->reset = true;
> > +
> > +	INIT_LIST_HEAD(&info->node);
> > +
> > +	return 0;
> > +}
> > +
> > +static int vp_modern_enable_reset_vq(struct virtqueue *vq, u16 ring_num)
> > +{
> > +	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> > +	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> > +	struct virtio_pci_vq_info *info;
> > +	struct virtqueue *_vq;
> > +	u16 msix_vec;
> > +
> > +	if (!vq->reset)
> > +		return -EPERM;
> > +
> > +	/* check queue reset status */
> > +	if (vp_modern_get_queue_reset(mdev, vq->index) != 1)
> > +		return -EBUSY;
> > +
> > +	info = vp_dev->vqs[vq->index];
> > +	_vq = vp_setup_vq(vq->vdev, vq->index, NULL, NULL, NULL,
> > +			 info->msix_vector, ring_num);
> > +	if (IS_ERR(_vq)) {
> > +		vq->reset = true;
> > +		return PTR_ERR(_vq);
> > +	}
> > +
> > +	vp_modern_set_queue_enable(&vp_dev->mdev, vq->index, true);
> > +
> > +	msix_vec = vp_dev->vqs[vq->index]->msix_vector;
> > +	if (vp_dev->per_vq_vectors && msix_vec != VIRTIO_MSI_NO_VECTOR)
> > +		enable_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec));
> > +
> > +	return 0;
> > +}
> > +
> >   static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector)
> >   {
> >   	return vp_modern_config_vector(&vp_dev->mdev, vector);
> > @@ -231,10 +298,13 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> >   				virtqueue_get_avail_addr(vq),
> >   				virtqueue_get_used_addr(vq));
> >
> > -	vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index, NULL);
> >   	if (!vq->priv) {
> > -		err = -ENOMEM;
> > -		goto err_map_notify;
> > +		vq->priv = (void __force *)vp_modern_map_vq_notify(mdev, index,
> > +								   NULL);
> > +		if (!vq->priv) {
> > +			err = -ENOMEM;
> > +			goto err_map_notify;
> > +		}
>
>
> This seems unrelated or an artifact of previous patches?

Sorry, I missed this.

Since re-enable reset queue will call setup_vq() again. Added check to not call
vp_modern_map_vq_notify() repeatedly, only called when vq->priv == NULL.

I think it would be better to implement this in a separate patch.

Thanks.


>
> Thanks
>
>
> >   	}
> >
> >   	if (msix_vec != VIRTIO_MSI_NO_VECTOR) {
> > @@ -402,6 +472,8 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> >   	.set_vq_affinity = vp_set_vq_affinity,
> >   	.get_vq_affinity = vp_get_vq_affinity,
> >   	.get_shm_region  = vp_get_shm_region,
> > +	.reset_vq	 = vp_modern_reset_vq,
> > +	.enable_reset_vq = vp_modern_enable_reset_vq,
> >   };
> >
> >   static const struct virtio_config_ops virtio_pci_config_ops = {
> > @@ -420,6 +492,8 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> >   	.set_vq_affinity = vp_set_vq_affinity,
> >   	.get_vq_affinity = vp_get_vq_affinity,
> >   	.get_shm_region  = vp_get_shm_region,
> > +	.reset_vq	 = vp_modern_reset_vq,
> > +	.enable_reset_vq = vp_modern_enable_reset_vq,
> >   };
> >
> >   /* the PCI probing function */
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index b8747df8dc1f..4f6028e1e2d9 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -1731,6 +1731,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >   	vq->vq.vdev = vdev;
> >   	vq->vq.num_free = num;
> >   	vq->vq.index = index;
> > +	vq->vq.reset = false;
> >   	vq->we_own_ring = true;
> >   	vq->notify = notify;
> >   	vq->weak_barriers = weak_barriers;
> > @@ -2220,6 +2221,7 @@ static int __vring_init_virtqueue(struct virtqueue *_vq,
> >   	vq->vq.vdev = vdev;
> >   	vq->vq.num_free = vring.num;
> >   	vq->vq.index = index;
> > +	vq->vq.reset = false;
> >   	vq->we_own_ring = false;
> >   	vq->notify = notify;
> >   	vq->weak_barriers = weak_barriers;
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index dd1657c3a488..5d4817d79f3f 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -32,6 +32,7 @@ struct virtqueue {
> >   	unsigned int index;
> >   	unsigned int num_free;
> >   	void *priv;
> > +	bool reset;
> >   };
> >
> >   int virtqueue_add_outbuf(struct virtqueue *vq,
>