* [PATCH v4 01/14] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 02/14] virtio: queue_reset: add VIRTIO_F_RING_RESET Xuan Zhuo
` (13 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 02/14] virtio: queue_reset: add VIRTIO_F_RING_RESET
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 01/14] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 03/14] virtio_ring: queue_reset: add function vring_setup_virtqueue() Xuan Zhuo
` (12 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 03/14] virtio_ring: queue_reset: add function vring_setup_virtqueue()
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 01/14] virtio_pci: struct virtio_pci_common_cfg add queue_notify_data Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 02/14] virtio: queue_reset: add VIRTIO_F_RING_RESET Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 04/14] virtio_ring: queue_reset: split: add __vring_init_virtqueue() Xuan Zhuo
` (11 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 04/14] virtio_ring: queue_reset: split: add __vring_init_virtqueue()
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (2 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 03/14] virtio_ring: queue_reset: add function vring_setup_virtqueue() Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 05/14] virtio_ring: queue_reset: split: support enable reset queue Xuan Zhuo
` (10 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 05/14] virtio_ring: queue_reset: split: support enable reset queue
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (3 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 04/14] virtio_ring: queue_reset: split: add __vring_init_virtqueue() Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 06/14] virtio_ring: queue_reset: packed: " Xuan Zhuo
` (9 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 06/14] virtio_ring: queue_reset: packed: support enable reset queue
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (4 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 05/14] virtio_ring: queue_reset: split: support enable reset queue Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 07/14] virtio_ring: queue_reset: extract the release function of the vq ring Xuan Zhuo
` (8 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 07/14] virtio_ring: queue_reset: extract the release function of the vq ring
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (5 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 06/14] virtio_ring: queue_reset: packed: " Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 08/14] virtio_ring: queue_reset: add vring_release_virtqueue() Xuan Zhuo
` (7 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 08/14] virtio_ring: queue_reset: add vring_release_virtqueue()
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (6 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 07/14] virtio_ring: queue_reset: extract the release function of the vq ring Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset Xuan Zhuo
` (6 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (7 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 08/14] virtio_ring: queue_reset: add vring_release_virtqueue() Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-11 6:49 ` Jason Wang
2022-02-09 12:28 ` [PATCH v4 10/14] virtio_pci: queue_reset: update struct virtio_pci_common_cfg and option functions Xuan Zhuo
` (5 subsequent siblings)
14 siblings, 1 reply; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset
2022-02-09 12:28 ` [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset Xuan Zhuo
@ 2022-02-11 6:49 ` Jason Wang
2022-02-11 7:07 ` Xuan Zhuo
0 siblings, 1 reply; 26+ messages in thread
From: Jason Wang @ 2022-02-11 6:49 UTC (permalink / raw)
To: Xuan Zhuo, linux-kernel; +Cc: Michael S. Tsirkin, virtualization
在 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. */
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset
2022-02-11 6:49 ` Jason Wang
@ 2022-02-11 7:07 ` Xuan Zhuo
2022-02-11 7:36 ` Jason Wang
0 siblings, 1 reply; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-11 7:07 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization, linux-kernel
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. */
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset
2022-02-11 7:07 ` Xuan Zhuo
@ 2022-02-11 7:36 ` Jason Wang
0 siblings, 0 replies; 26+ messages in thread
From: Jason Wang @ 2022-02-11 7:36 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization, linux-kernel
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. */
> >
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 10/14] virtio_pci: queue_reset: update struct virtio_pci_common_cfg and option functions
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (8 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 09/14] virtio: queue_reset: struct virtio_config_ops add callbacks for queue_reset Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 11/14] virtio_pci: queue_reset: release vq by vp_dev->vqs Xuan Zhuo
` (4 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 11/14] virtio_pci: queue_reset: release vq by vp_dev->vqs
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (9 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 10/14] virtio_pci: queue_reset: update struct virtio_pci_common_cfg and option functions Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:28 ` [PATCH v4 12/14] virtio_pci: queue_reset: setup_vq() support vring_setup_virtqueue() Xuan Zhuo
` (3 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 12/14] virtio_pci: queue_reset: setup_vq() support vring_setup_virtqueue()
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (10 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 11/14] virtio_pci: queue_reset: release vq by vp_dev->vqs Xuan Zhuo
@ 2022-02-09 12:28 ` Xuan Zhuo
2022-02-09 12:29 ` [PATCH v4 13/14] virtio_pci: queue_reset: vp_setup_vq() support ring_num Xuan Zhuo
` (2 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 13/14] virtio_pci: queue_reset: vp_setup_vq() support ring_num
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (11 preceding siblings ...)
2022-02-09 12:28 ` [PATCH v4 12/14] virtio_pci: queue_reset: setup_vq() support vring_setup_virtqueue() Xuan Zhuo
@ 2022-02-09 12:29 ` Xuan Zhuo
2022-02-09 12:29 ` [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET Xuan Zhuo
2022-02-11 5:40 ` [PATCH v4 00/14] virtio pci " Jason Wang
14 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:29 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (12 preceding siblings ...)
2022-02-09 12:29 ` [PATCH v4 13/14] virtio_pci: queue_reset: vp_setup_vq() support ring_num Xuan Zhuo
@ 2022-02-09 12:29 ` Xuan Zhuo
2022-02-11 7:05 ` Jason Wang
2022-02-11 5:40 ` [PATCH v4 00/14] virtio pci " Jason Wang
14 siblings, 1 reply; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-09 12:29 UTC (permalink / raw)
To: linux-kernel; +Cc: Michael S. Tsirkin, Jason Wang, virtualization
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
2022-02-09 12:29 ` [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET Xuan Zhuo
@ 2022-02-11 7:05 ` Jason Wang
2022-02-11 7:21 ` Xuan Zhuo
2022-02-14 2:50 ` Xuan Zhuo
0 siblings, 2 replies; 26+ messages in thread
From: Jason Wang @ 2022-02-11 7:05 UTC (permalink / raw)
To: Xuan Zhuo, linux-kernel; +Cc: Michael S. Tsirkin, virtualization
在 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,
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
2022-02-11 7:05 ` Jason Wang
@ 2022-02-11 7:21 ` Xuan Zhuo
2022-02-11 7:45 ` Jason Wang
2022-02-14 2:50 ` Xuan Zhuo
1 sibling, 1 reply; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-11 7:21 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization, linux-kernel
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,
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
2022-02-11 7:21 ` Xuan Zhuo
@ 2022-02-11 7:45 ` Jason Wang
2022-02-11 8:24 ` Xuan Zhuo
0 siblings, 1 reply; 26+ messages in thread
From: Jason Wang @ 2022-02-11 7:45 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization, linux-kernel
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,
> >
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
2022-02-11 7:45 ` Jason Wang
@ 2022-02-11 8:24 ` Xuan Zhuo
0 siblings, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-11 8:24 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization, linux-kernel
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,
> > >
> >
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET
2022-02-11 7:05 ` Jason Wang
2022-02-11 7:21 ` Xuan Zhuo
@ 2022-02-14 2:50 ` Xuan Zhuo
1 sibling, 0 replies; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-14 2:50 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization, linux-kernel
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,
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET
2022-02-09 12:28 [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET Xuan Zhuo
` (13 preceding siblings ...)
2022-02-09 12:29 ` [PATCH v4 14/14] virtio_pci: queue_reset: support VIRTIO_F_RING_RESET Xuan Zhuo
@ 2022-02-11 5:40 ` Jason Wang
2022-02-11 6:27 ` Xuan Zhuo
14 siblings, 1 reply; 26+ messages in thread
From: Jason Wang @ 2022-02-11 5:40 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: linux-kernel, Michael S. Tsirkin, virtualization
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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET
2022-02-11 5:40 ` [PATCH v4 00/14] virtio pci " Jason Wang
@ 2022-02-11 6:27 ` Xuan Zhuo
2022-02-11 7:09 ` Jason Wang
0 siblings, 1 reply; 26+ messages in thread
From: Xuan Zhuo @ 2022-02-11 6:27 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, Michael S. Tsirkin, virtualization
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
> >
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 00/14] virtio pci support VIRTIO_F_RING_RESET
2022-02-11 6:27 ` Xuan Zhuo
@ 2022-02-11 7:09 ` Jason Wang
0 siblings, 0 replies; 26+ messages in thread
From: Jason Wang @ 2022-02-11 7:09 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: linux-kernel, Michael S. Tsirkin, virtualization
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
> > >
> >
>
^ permalink raw reply [flat|nested] 26+ messages in thread