[PATCH v8 00/16] virtio pci support VIRTIO_F_RING_RESET (refactor vring)

[PATCH v8 00/16] virtio pci support VIRTIO_F_RING_RESET (refactor vring)

[Xuan Zhuo]

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

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

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

This patch set implements the refactoring of vring. Finally, the
virtuque_resize() interface is provided based on the reset function of the
transport layer.

Test environment:
    Host: 4.19.91
    Qemu: QEMU emulator version 6.2.50 (with vq reset support)
    Test Cmd:  ethtool -G eth1 rx $1 tx $2; ethtool -g eth1

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

Please review. Thanks.

v8:
  1. Provide a virtqueue_reset() interface directly
  2. Split the two patch sets, this is the first part
  3. Add independent allocation helper for allocating state, extra

v7:
  1. fix #6 subject typo
  2. fix #6 ring_size_in_bytes is uninitialized
  3. check by: make W=12

v6:
  1. virtio_pci: use synchronize_irq(irq) to sync the irq callbacks
  2. Introduce virtqueue_reset_vring() to implement the reset of vring during
     the reset process. May use the old vring if num of the vq not change.
  3. find_vqs() support sizes to special the max size of each vq

v5:
  1. add virtio-net support set_ringparam

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 (16):
  virtio: add helper virtqueue_get_vring_max_size()
  virtio: struct virtio_config_ops add callbacks for queue_reset
  virtio_ring: update the document of the virtqueue_detach_unused_buf
    for queue reset
  virtio_ring: remove the arg vq of vring_alloc_desc_extra()
  virtio_ring: extract the logic of freeing vring
  virtio_ring: split: extract the logic of alloc queue
  virtio_ring: split: extract the logic of alloc state and extra
  virtio_ring: split: extract the logic of attach vring
  virtio_ring: split: extract the logic of vq init
  virtio_ring: split: implement virtqueue_resize_split()
  virtio_ring: packed: extract the logic of alloc queue
  virtio_ring: packed: extract the logic of alloc state and extra
  virtio_ring: packed: extract the logic of attach vring
  virtio_ring: packed: extract the logic of vq init
  virtio_ring: packed: implement virtqueue_resize_packed()
  virtio_ring: introduce virtqueue_resize()

 drivers/virtio/virtio_mmio.c       |   2 +
 drivers/virtio/virtio_pci_legacy.c |   2 +
 drivers/virtio/virtio_pci_modern.c |   2 +
 drivers/virtio/virtio_ring.c       | 604 ++++++++++++++++++++++-------
 include/linux/virtio.h             |   5 +
 include/linux/virtio_config.h      |  12 +
 6 files changed, 494 insertions(+), 133 deletions(-)

--
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 01/16] virtio: add helper virtqueue_get_vring_max_size()

[Xuan Zhuo]

Record the maximum queue num supported by the device.

virtio-net can display the maximum (supported by hardware) ring size in
ethtool -g eth0.

When the subsequent patch implements vring reset, it can judge whether
the ring size passed by the driver is legal based on this.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/virtio/virtio_mmio.c       |  2 ++
 drivers/virtio/virtio_pci_legacy.c |  2 ++
 drivers/virtio/virtio_pci_modern.c |  2 ++
 drivers/virtio/virtio_ring.c       | 14 ++++++++++++++
 include/linux/virtio.h             |  2 ++
 5 files changed, 22 insertions(+)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 56128b9c46eb..a41abc8051b9 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -390,6 +390,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index,
 		goto error_new_virtqueue;
 	}
 
+	vq->num_max = num;
+
 	/* Activate the queue */
 	writel(virtqueue_get_vring_size(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NUM);
 	if (vm_dev->version == 1) {
diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 34141b9abe27..b68934fe6b5d 100644
--- a/drivers/virtio/virtio_pci_legacy.c
+++ b/drivers/virtio/virtio_pci_legacy.c
@@ -135,6 +135,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	if (!vq)
 		return ERR_PTR(-ENOMEM);
 
+	vq->num_max = num;
+
 	q_pfn = virtqueue_get_desc_addr(vq) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT;
 	if (q_pfn >> 32) {
 		dev_err(&vp_dev->pci_dev->dev,
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 5455bc041fb6..86d301f272b8 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -218,6 +218,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	if (!vq)
 		return ERR_PTR(-ENOMEM);
 
+	vq->num_max = num;
+
 	/* activate the queue */
 	vp_modern_set_queue_size(mdev, index, virtqueue_get_vring_size(vq));
 	vp_modern_queue_address(mdev, index, virtqueue_get_desc_addr(vq),
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 962f1477b1fa..b87130c8f312 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2371,6 +2371,20 @@ void vring_transport_features(struct virtio_device *vdev)
 }
 EXPORT_SYMBOL_GPL(vring_transport_features);
 
+/**
+ * virtqueue_get_vring_max_size - return the max size of the virtqueue's vring
+ * @_vq: the struct virtqueue containing the vring of interest.
+ *
+ * Returns the max size of the vring.
+ *
+ * Unlike other operations, this need not be serialized.
+ */
+unsigned int virtqueue_get_vring_max_size(struct virtqueue *_vq)
+{
+	return _vq->num_max;
+}
+EXPORT_SYMBOL_GPL(virtqueue_get_vring_max_size);
+
 /**
  * virtqueue_get_vring_size - return the size of the virtqueue's vring
  * @_vq: the struct virtqueue containing the vring of interest.
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 72292a62cd90..d59adc4be068 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -31,6 +31,7 @@ struct virtqueue {
 	struct virtio_device *vdev;
 	unsigned int index;
 	unsigned int num_free;
+	unsigned int num_max;
 	void *priv;
 };
 
@@ -80,6 +81,7 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
 
 void *virtqueue_detach_unused_buf(struct virtqueue *vq);
 
+unsigned int virtqueue_get_vring_max_size(struct virtqueue *vq);
 unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
 
 bool virtqueue_is_broken(struct virtqueue *vq);
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 01/16] virtio: add helper virtqueue_get_vring_max_size()

[Cornelia Huck]

On Mon, Mar 14 2022, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:

> Record the maximum queue num supported by the device.
>
> virtio-net can display the maximum (supported by hardware) ring size in
> ethtool -g eth0.
>
> When the subsequent patch implements vring reset, it can judge whether
> the ring size passed by the driver is legal based on this.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Acked-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/virtio/virtio_mmio.c       |  2 ++
>  drivers/virtio/virtio_pci_legacy.c |  2 ++
>  drivers/virtio/virtio_pci_modern.c |  2 ++
>  drivers/virtio/virtio_ring.c       | 14 ++++++++++++++
>  include/linux/virtio.h             |  2 ++
>  5 files changed, 22 insertions(+)

Don't you also need to init this for ccw (even though we won't do ring
reset there), just for completeness? (Any other transports?)

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 01/16] virtio: add helper virtqueue_get_vring_max_size()

[Michael S. Tsirkin]

On Mon, Mar 14, 2022 at 10:50:08AM +0100, Cornelia Huck wrote:
> On Mon, Mar 14 2022, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> 
> > Record the maximum queue num supported by the device.
> >
> > virtio-net can display the maximum (supported by hardware) ring size in
> > ethtool -g eth0.
> >
> > When the subsequent patch implements vring reset, it can judge whether
> > the ring size passed by the driver is legal based on this.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  drivers/virtio/virtio_mmio.c       |  2 ++
> >  drivers/virtio/virtio_pci_legacy.c |  2 ++
> >  drivers/virtio/virtio_pci_modern.c |  2 ++
> >  drivers/virtio/virtio_ring.c       | 14 ++++++++++++++
> >  include/linux/virtio.h             |  2 ++
> >  5 files changed, 22 insertions(+)
> 
> Don't you also need to init this for ccw (even though we won't do ring
> reset there), just for completeness? (Any other transports?)

rpmsg?

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 01/16] virtio: add helper virtqueue_get_vring_max_size()

[Xuan Zhuo]

On Mon, 14 Mar 2022 07:18:27 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Mar 14, 2022 at 10:50:08AM +0100, Cornelia Huck wrote:
> > On Mon, Mar 14 2022, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > > Record the maximum queue num supported by the device.
> > >
> > > virtio-net can display the maximum (supported by hardware) ring size in
> > > ethtool -g eth0.
> > >
> > > When the subsequent patch implements vring reset, it can judge whether
> > > the ring size passed by the driver is legal based on this.
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  drivers/virtio/virtio_mmio.c       |  2 ++
> > >  drivers/virtio/virtio_pci_legacy.c |  2 ++
> > >  drivers/virtio/virtio_pci_modern.c |  2 ++
> > >  drivers/virtio/virtio_ring.c       | 14 ++++++++++++++
> > >  include/linux/virtio.h             |  2 ++
> > >  5 files changed, 22 insertions(+)
> >
> > Don't you also need to init this for ccw (even though we won't do ring
> > reset there), just for completeness? (Any other transports?)
>
> rpmsg?

There should be these.

 arch/um/drivers/virtio_uml.c             |  1 +
 drivers/platform/mellanox/mlxbf-tmfifo.c |  2 ++
 drivers/remoteproc/remoteproc_virtio.c   |  2 ++
 drivers/s390/virtio/virtio_ccw.c         |  3 +++
 drivers/virtio/virtio_mmio.c             |  2 ++
 drivers/virtio/virtio_pci_legacy.c       |  2 ++
 drivers/virtio/virtio_pci_modern.c       |  2 ++
 drivers/virtio/virtio_vdpa.c             |  2 ++

It will be included in the next version.

Thanks.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 01/16] virtio: add helper virtqueue_get_vring_max_size()

[Michael S. Tsirkin]

On Mon, Mar 14, 2022 at 07:21:18PM +0800, Xuan Zhuo wrote:
> On Mon, 14 Mar 2022 07:18:27 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, Mar 14, 2022 at 10:50:08AM +0100, Cornelia Huck wrote:
> > > On Mon, Mar 14 2022, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > > Record the maximum queue num supported by the device.
> > > >
> > > > virtio-net can display the maximum (supported by hardware) ring size in
> > > > ethtool -g eth0.
> > > >
> > > > When the subsequent patch implements vring reset, it can judge whether
> > > > the ring size passed by the driver is legal based on this.
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  drivers/virtio/virtio_mmio.c       |  2 ++
> > > >  drivers/virtio/virtio_pci_legacy.c |  2 ++
> > > >  drivers/virtio/virtio_pci_modern.c |  2 ++
> > > >  drivers/virtio/virtio_ring.c       | 14 ++++++++++++++
> > > >  include/linux/virtio.h             |  2 ++
> > > >  5 files changed, 22 insertions(+)
> > >
> > > Don't you also need to init this for ccw (even though we won't do ring
> > > reset there), just for completeness? (Any other transports?)
> >
> > rpmsg?
> 
> There should be these.
> 
>  arch/um/drivers/virtio_uml.c             |  1 +
>  drivers/platform/mellanox/mlxbf-tmfifo.c |  2 ++
>  drivers/remoteproc/remoteproc_virtio.c   |  2 ++
>  drivers/s390/virtio/virtio_ccw.c         |  3 +++
>  drivers/virtio/virtio_mmio.c             |  2 ++
>  drivers/virtio/virtio_pci_legacy.c       |  2 ++
>  drivers/virtio/virtio_pci_modern.c       |  2 ++
>  drivers/virtio/virtio_vdpa.c             |  2 ++
> 
> It will be included in the next version.

Hmm so merge window is open but we don't have a final version yet.
Not sure it can make it in this merge window ..


> Thanks.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 02/16] virtio: struct virtio_config_ops add callbacks for queue_reset

[Xuan Zhuo]

Performing reset on a queue is divided into four steps:

 1. transport: notify the device to reset the queue
 2. vring:     recycle the buffer submitted
 3. vring:     reset/resize the vring (may re-alloc)
 4. transport: mmap vring to device, and enable the queue

In order to support queue reset, add two callbacks(reset_vq,
enable_reset_vq) in struct virtio_config_ops to implement steps 1 and 4.

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

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 4d107ad31149..d4adcd0e1c57 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -74,6 +74,16 @@ 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 (optional).
+ *	vq: the virtqueue
+ *	Returns 0 on success or error status
+ *	reset_vq will guarantee that the callbacks are disabled and synchronized.
+ *	Except for the callback, the caller should guarantee that the vring is
+ *	not accessed by any functions of virtqueue.
+ * @enable_reset_vq: enable a reset queue
+ *	vq: the virtqueue
+ *	Returns 0 on success or error status
+ *	If reset_vq is set, then enable_reset_vq must also be set.
  */
 typedef void vq_callback_t(struct virtqueue *);
 struct virtio_config_ops {
@@ -100,6 +110,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);
 };
 
 /* If driver didn't advertise the feature, it will never appear. */
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 03/16] virtio_ring: update the document of the virtqueue_detach_unused_buf for queue reset

[Xuan Zhuo]

Added documentation for virtqueue_detach_unused_buf, allowing it to be
called on queue reset.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b87130c8f312..f1807f6b06a5 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2127,8 +2127,8 @@ EXPORT_SYMBOL_GPL(virtqueue_enable_cb_delayed);
  * @_vq: the struct virtqueue we're talking about.
  *
  * Returns NULL or the "data" token handed to virtqueue_add_*().
- * This is not valid on an active queue; it is useful only for device
- * shutdown.
+ * This is not valid on an active queue; it is useful for device
+ * shutdown or the reset queue.
  */
 void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
 {
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 04/16] virtio_ring: remove the arg vq of vring_alloc_desc_extra()

[Xuan Zhuo]

The parameter vq of vring_alloc_desc_extra() is useless. This patch
removes this parameter.

Subsequent patches will call this function to avoid passing useless
arguments.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index f1807f6b06a5..cb6010750a94 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1636,8 +1636,7 @@ static void *virtqueue_detach_unused_buf_packed(struct virtqueue *_vq)
 	return NULL;
 }
 
-static struct vring_desc_extra *vring_alloc_desc_extra(struct vring_virtqueue *vq,
-						       unsigned int num)
+static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num)
 {
 	struct vring_desc_extra *desc_extra;
 	unsigned int i;
@@ -1755,7 +1754,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	/* Put everything in free lists. */
 	vq->free_head = 0;
 
-	vq->packed.desc_extra = vring_alloc_desc_extra(vq, num);
+	vq->packed.desc_extra = vring_alloc_desc_extra(num);
 	if (!vq->packed.desc_extra)
 		goto err_desc_extra;
 
@@ -2233,7 +2232,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	if (!vq->split.desc_state)
 		goto err_state;
 
-	vq->split.desc_extra = vring_alloc_desc_extra(vq, vring.num);
+	vq->split.desc_extra = vring_alloc_desc_extra(vring.num);
 	if (!vq->split.desc_extra)
 		goto err_extra;
 
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 04/16] virtio_ring: remove the arg vq of vring_alloc_desc_extra()

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> The parameter vq of vring_alloc_desc_extra() is useless. This patch
> removes this parameter.
>
> Subsequent patches will call this function to avoid passing useless
> arguments.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


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


> ---
>   drivers/virtio/virtio_ring.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index f1807f6b06a5..cb6010750a94 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1636,8 +1636,7 @@ static void *virtqueue_detach_unused_buf_packed(struct virtqueue *_vq)
>   	return NULL;
>   }
>   
> -static struct vring_desc_extra *vring_alloc_desc_extra(struct vring_virtqueue *vq,
> -						       unsigned int num)
> +static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num)
>   {
>   	struct vring_desc_extra *desc_extra;
>   	unsigned int i;
> @@ -1755,7 +1754,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
>   	/* Put everything in free lists. */
>   	vq->free_head = 0;
>   
> -	vq->packed.desc_extra = vring_alloc_desc_extra(vq, num);
> +	vq->packed.desc_extra = vring_alloc_desc_extra(num);
>   	if (!vq->packed.desc_extra)
>   		goto err_desc_extra;
>   
> @@ -2233,7 +2232,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>   	if (!vq->split.desc_state)
>   		goto err_state;
>   
> -	vq->split.desc_extra = vring_alloc_desc_extra(vq, vring.num);
> +	vq->split.desc_extra = vring_alloc_desc_extra(vring.num);
>   	if (!vq->split.desc_extra)
>   		goto err_extra;
>   

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 05/16] virtio_ring: extract the logic of freeing vring

[Xuan Zhuo]

Introduce __vring_free() to free the vring of vq.

Subsequent patches will use __vring_free() alone.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index cb6010750a94..763532a2ffe4 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2301,14 +2301,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_free(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	spin_lock(&vq->vq.vdev->vqs_list_lock);
-	list_del(&_vq->list);
-	spin_unlock(&vq->vq.vdev->vqs_list_lock);
-
 	if (vq->we_own_ring) {
 		if (vq->packed_ring) {
 			vring_free_queue(vq->vq.vdev,
@@ -2339,6 +2335,18 @@ 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);
+
+	spin_lock(&vq->vq.vdev->vqs_list_lock);
+	list_del(&_vq->list);
+	spin_unlock(&vq->vq.vdev->vqs_list_lock);
+
+	__vring_free(_vq);
+
 	kfree(vq);
 }
 EXPORT_SYMBOL_GPL(vring_del_virtqueue);
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 06/16] virtio_ring: split: extract the logic of alloc queue

[Xuan Zhuo]

Separate the logic of split to create vring queue.

This feature is required for subsequent virtuqueue reset vring.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 763532a2ffe4..d7667f74c42b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -915,23 +915,15 @@ static void *virtqueue_detach_unused_buf_split(struct virtqueue *_vq)
 	return NULL;
 }
 
-static struct virtqueue *vring_create_virtqueue_split(
-	unsigned int index,
-	unsigned int num,
-	unsigned int vring_align,
-	struct virtio_device *vdev,
-	bool weak_barriers,
-	bool may_reduce_num,
-	bool context,
-	bool (*notify)(struct virtqueue *),
-	void (*callback)(struct virtqueue *),
-	const char *name)
+static void *vring_alloc_queue_split(struct virtio_device *vdev,
+				     dma_addr_t *dma_addr,
+				     u32 *n,
+				     unsigned int vring_align,
+				     bool weak_barriers,
+				     bool may_reduce_num)
 {
-	struct virtqueue *vq;
 	void *queue = NULL;
-	dma_addr_t dma_addr;
-	size_t queue_size_in_bytes;
-	struct vring vring;
+	u32 num = *n;
 
 	/* We assume num is a power of 2. */
 	if (num & (num - 1)) {
@@ -942,7 +934,7 @@ static struct virtqueue *vring_create_virtqueue_split(
 	/* TODO: allocate each queue chunk individually */
 	for (; num && vring_size(num, vring_align) > PAGE_SIZE; num /= 2) {
 		queue = vring_alloc_queue(vdev, vring_size(num, vring_align),
-					  &dma_addr,
+					  dma_addr,
 					  GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
 		if (queue)
 			break;
@@ -956,11 +948,38 @@ static struct virtqueue *vring_create_virtqueue_split(
 	if (!queue) {
 		/* Try to get a single page. You are my only hope! */
 		queue = vring_alloc_queue(vdev, vring_size(num, vring_align),
-					  &dma_addr, GFP_KERNEL|__GFP_ZERO);
+					  dma_addr, GFP_KERNEL|__GFP_ZERO);
 	}
 	if (!queue)
 		return NULL;
 
+	*n = num;
+	return queue;
+}
+
+static struct virtqueue *vring_create_virtqueue_split(
+	unsigned int index,
+	unsigned int num,
+	unsigned int vring_align,
+	struct virtio_device *vdev,
+	bool weak_barriers,
+	bool may_reduce_num,
+	bool context,
+	bool (*notify)(struct virtqueue *),
+	void (*callback)(struct virtqueue *),
+	const char *name)
+{
+	size_t queue_size_in_bytes;
+	struct virtqueue *vq;
+	dma_addr_t dma_addr;
+	struct vring vring;
+	void *queue;
+
+	queue = vring_alloc_queue_split(vdev, &dma_addr, &num, vring_align,
+					weak_barriers, may_reduce_num);
+	if (!queue)
+		return NULL;
+
 	queue_size_in_bytes = vring_size(num, vring_align);
 	vring_init(&vring, num, queue, vring_align);
 
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 07/16] virtio_ring: split: extract the logic of alloc state and extra

[Xuan Zhuo]

Separate the logic of creating desc_state, desc_extra, and subsequent
patches will call it independently.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index d7667f74c42b..9b850188c38e 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2186,6 +2186,33 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
 }
 EXPORT_SYMBOL_GPL(vring_interrupt);
 
+static int __vring_alloc_state_extra_split(u32 num,
+					   struct vring_desc_state_split **desc_state,
+					   struct vring_desc_extra **desc_extra)
+{
+	struct vring_desc_state_split *state;
+	struct vring_desc_extra *extra;
+
+	state = kmalloc_array(num, sizeof(struct vring_desc_state_split), GFP_KERNEL);
+	if (!state)
+		goto err_state;
+
+	extra = vring_alloc_desc_extra(num);
+	if (!extra)
+		goto err_extra;
+
+	memset(state, 0, num * sizeof(struct vring_desc_state_split));
+
+	*desc_state = state;
+	*desc_extra = extra;
+	return 0;
+
+err_extra:
+	kfree(state);
+err_state:
+	return -ENOMEM;
+}
+
 /* Only available for split ring */
 struct virtqueue *__vring_new_virtqueue(unsigned int index,
 					struct vring vring,
@@ -2196,7 +2223,10 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 					void (*callback)(struct virtqueue *),
 					const char *name)
 {
+	struct vring_desc_state_split *state;
+	struct vring_desc_extra *extra;
 	struct vring_virtqueue *vq;
+	int err;
 
 	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
 		return NULL;
@@ -2246,30 +2276,22 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 					vq->split.avail_flags_shadow);
 	}
 
-	vq->split.desc_state = kmalloc_array(vring.num,
-			sizeof(struct vring_desc_state_split), GFP_KERNEL);
-	if (!vq->split.desc_state)
-		goto err_state;
+	err = __vring_alloc_state_extra_split(vring.num, &state, &extra);
+	if (err) {
+		kfree(vq);
+		return NULL;
+	}
 
-	vq->split.desc_extra = vring_alloc_desc_extra(vring.num);
-	if (!vq->split.desc_extra)
-		goto err_extra;
+	vq->split.desc_state = state;
+	vq->split.desc_extra = extra;
 
 	/* Put everything in free lists. */
 	vq->free_head = 0;
-	memset(vq->split.desc_state, 0, vring.num *
-			sizeof(struct vring_desc_state_split));
 
 	spin_lock(&vdev->vqs_list_lock);
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 	spin_unlock(&vdev->vqs_list_lock);
 	return &vq->vq;
-
-err_extra:
-	kfree(vq->split.desc_state);
-err_state:
-	kfree(vq);
-	return NULL;
 }
 EXPORT_SYMBOL_GPL(__vring_new_virtqueue);
 
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 07/16] virtio_ring: split: extract the logic of alloc state and extra

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> Separate the logic of creating desc_state, desc_extra, and subsequent
> patches will call it independently.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/virtio/virtio_ring.c | 52 +++++++++++++++++++++++++-----------
>   1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index d7667f74c42b..9b850188c38e 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2186,6 +2186,33 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
>   }
>   EXPORT_SYMBOL_GPL(vring_interrupt);
>   
> +static int __vring_alloc_state_extra_split(u32 num,
> +					   struct vring_desc_state_split **desc_state,
> +					   struct vring_desc_extra **desc_extra)


A nit here: I think we can simply remove the "__" prefix since:

1) We haven't had a version of helper without "__"

2) There're other internal helpers that doesn't use "__"

Thanks


> +{
> +	struct vring_desc_state_split *state;
> +	struct vring_desc_extra *extra;
> +
> +	state = kmalloc_array(num, sizeof(struct vring_desc_state_split), GFP_KERNEL);
> +	if (!state)
> +		goto err_state;
> +
> +	extra = vring_alloc_desc_extra(num);
> +	if (!extra)
> +		goto err_extra;
> +
> +	memset(state, 0, num * sizeof(struct vring_desc_state_split));
> +
> +	*desc_state = state;
> +	*desc_extra = extra;
> +	return 0;
> +
> +err_extra:
> +	kfree(state);
> +err_state:
> +	return -ENOMEM;
> +}
> +
>   /* Only available for split ring */
>   struct virtqueue *__vring_new_virtqueue(unsigned int index,
>   					struct vring vring,
> @@ -2196,7 +2223,10 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>   					void (*callback)(struct virtqueue *),
>   					const char *name)
>   {
> +	struct vring_desc_state_split *state;
> +	struct vring_desc_extra *extra;
>   	struct vring_virtqueue *vq;
> +	int err;
>   
>   	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
>   		return NULL;
> @@ -2246,30 +2276,22 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>   					vq->split.avail_flags_shadow);
>   	}
>   
> -	vq->split.desc_state = kmalloc_array(vring.num,
> -			sizeof(struct vring_desc_state_split), GFP_KERNEL);
> -	if (!vq->split.desc_state)
> -		goto err_state;
> +	err = __vring_alloc_state_extra_split(vring.num, &state, &extra);
> +	if (err) {
> +		kfree(vq);
> +		return NULL;
> +	}
>   
> -	vq->split.desc_extra = vring_alloc_desc_extra(vring.num);
> -	if (!vq->split.desc_extra)
> -		goto err_extra;
> +	vq->split.desc_state = state;
> +	vq->split.desc_extra = extra;
>   
>   	/* Put everything in free lists. */
>   	vq->free_head = 0;
> -	memset(vq->split.desc_state, 0, vring.num *
> -			sizeof(struct vring_desc_state_split));
>   
>   	spin_lock(&vdev->vqs_list_lock);
>   	list_add_tail(&vq->vq.list, &vdev->vqs);
>   	spin_unlock(&vdev->vqs_list_lock);
>   	return &vq->vq;
> -
> -err_extra:
> -	kfree(vq->split.desc_state);
> -err_state:
> -	kfree(vq);
> -	return NULL;
>   }
>   EXPORT_SYMBOL_GPL(__vring_new_virtqueue);
>   

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 07/16] virtio_ring: split: extract the logic of alloc state and extra

[Xuan Zhuo]

On Tue, 22 Mar 2022 14:33:34 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > Separate the logic of creating desc_state, desc_extra, and subsequent
> > patches will call it independently.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_ring.c | 52 +++++++++++++++++++++++++-----------
> >   1 file changed, 37 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index d7667f74c42b..9b850188c38e 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -2186,6 +2186,33 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
> >   }
> >   EXPORT_SYMBOL_GPL(vring_interrupt);
> >
> > +static int __vring_alloc_state_extra_split(u32 num,
> > +					   struct vring_desc_state_split **desc_state,
> > +					   struct vring_desc_extra **desc_extra)
>
>
> A nit here: I think we can simply remove the "__" prefix since:
>
> 1) We haven't had a version of helper without "__"
>
> 2) There're other internal helpers that doesn't use "__"

OK.

Thanks.

>
> Thanks
>
>
> > +{
> > +	struct vring_desc_state_split *state;
> > +	struct vring_desc_extra *extra;
> > +
> > +	state = kmalloc_array(num, sizeof(struct vring_desc_state_split), GFP_KERNEL);
> > +	if (!state)
> > +		goto err_state;
> > +
> > +	extra = vring_alloc_desc_extra(num);
> > +	if (!extra)
> > +		goto err_extra;
> > +
> > +	memset(state, 0, num * sizeof(struct vring_desc_state_split));
> > +
> > +	*desc_state = state;
> > +	*desc_extra = extra;
> > +	return 0;
> > +
> > +err_extra:
> > +	kfree(state);
> > +err_state:
> > +	return -ENOMEM;
> > +}
> > +
> >   /* Only available for split ring */
> >   struct virtqueue *__vring_new_virtqueue(unsigned int index,
> >   					struct vring vring,
> > @@ -2196,7 +2223,10 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
> >   					void (*callback)(struct virtqueue *),
> >   					const char *name)
> >   {
> > +	struct vring_desc_state_split *state;
> > +	struct vring_desc_extra *extra;
> >   	struct vring_virtqueue *vq;
> > +	int err;
> >
> >   	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> >   		return NULL;
> > @@ -2246,30 +2276,22 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
> >   					vq->split.avail_flags_shadow);
> >   	}
> >
> > -	vq->split.desc_state = kmalloc_array(vring.num,
> > -			sizeof(struct vring_desc_state_split), GFP_KERNEL);
> > -	if (!vq->split.desc_state)
> > -		goto err_state;
> > +	err = __vring_alloc_state_extra_split(vring.num, &state, &extra);
> > +	if (err) {
> > +		kfree(vq);
> > +		return NULL;
> > +	}
> >
> > -	vq->split.desc_extra = vring_alloc_desc_extra(vring.num);
> > -	if (!vq->split.desc_extra)
> > -		goto err_extra;
> > +	vq->split.desc_state = state;
> > +	vq->split.desc_extra = extra;
> >
> >   	/* Put everything in free lists. */
> >   	vq->free_head = 0;
> > -	memset(vq->split.desc_state, 0, vring.num *
> > -			sizeof(struct vring_desc_state_split));
> >
> >   	spin_lock(&vdev->vqs_list_lock);
> >   	list_add_tail(&vq->vq.list, &vdev->vqs);
> >   	spin_unlock(&vdev->vqs_list_lock);
> >   	return &vq->vq;
> > -
> > -err_extra:
> > -	kfree(vq->split.desc_state);
> > -err_state:
> > -	kfree(vq);
> > -	return NULL;
> >   }
> >   EXPORT_SYMBOL_GPL(__vring_new_virtqueue);
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 08/16] virtio_ring: split: extract the logic of attach vring

[Xuan Zhuo]

Separate the logic of attach vring, subsequent patches will call it
separately.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 9b850188c38e..6ed10c46d6d6 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2213,6 +2213,21 @@ static int __vring_alloc_state_extra_split(u32 num,
 	return -ENOMEM;
 }
 
+static void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
+					   struct vring vring,
+					   struct vring_desc_state_split *desc_state,
+					   struct vring_desc_extra *desc_extra)
+{
+	vq->vq.num_free = vring.num;
+
+	vq->split.vring = vring;
+	vq->split.queue_dma_addr = 0;
+	vq->split.queue_size_in_bytes = 0;
+
+	vq->split.desc_state = desc_state;
+	vq->split.desc_extra = desc_extra;
+}
+
 /* Only available for split ring */
 struct virtqueue *__vring_new_virtqueue(unsigned int index,
 					struct vring vring,
@@ -2239,7 +2254,6 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	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;
 	vq->notify = notify;
@@ -2261,10 +2275,6 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
 		vq->weak_barriers = false;
 
-	vq->split.queue_dma_addr = 0;
-	vq->split.queue_size_in_bytes = 0;
-
-	vq->split.vring = vring;
 	vq->split.avail_flags_shadow = 0;
 	vq->split.avail_idx_shadow = 0;
 
@@ -2282,8 +2292,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 		return NULL;
 	}
 
-	vq->split.desc_state = state;
-	vq->split.desc_extra = extra;
+	__vring_virtqueue_attach_split(vq, vring, state, extra);
 
 	/* Put everything in free lists. */
 	vq->free_head = 0;
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 09/16] virtio_ring: split: extract the logic of vq init

[Xuan Zhuo]

Separate the logic of initializing vq, and subsequent patches will call
it separately.

The feature of this part is that it does not depend on the information
passed by the upper layer and can be called repeatedly.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 6ed10c46d6d6..81bbfd65411e 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2228,6 +2228,41 @@ static void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
 	vq->split.desc_extra = desc_extra;
 }
 
+static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
+					 struct virtio_device *vdev)
+{
+	vq->packed_ring = false;
+	vq->we_own_ring = false;
+	vq->broken = false;
+	vq->last_used_idx = 0;
+	vq->event_triggered = false;
+	vq->num_added = 0;
+	vq->use_dma_api = vring_use_dma_api(vdev);
+#ifdef DEBUG
+	vq->in_use = false;
+	vq->last_add_time_valid = false;
+#endif
+
+	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
+
+	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
+		vq->weak_barriers = false;
+
+	vq->split.avail_flags_shadow = 0;
+	vq->split.avail_idx_shadow = 0;
+
+	/* No callback?  Tell other side not to bother us. */
+	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,
+					vq->split.avail_flags_shadow);
+	}
+
+	/* Put everything in free lists. */
+	vq->free_head = 0;
+}
+
 /* Only available for split ring */
 struct virtqueue *__vring_new_virtqueue(unsigned int index,
 					struct vring vring,
@@ -2250,41 +2285,15 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	if (!vq)
 		return NULL;
 
-	vq->packed_ring = false;
 	vq->vq.callback = callback;
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;
 	vq->vq.index = index;
-	vq->we_own_ring = false;
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
-	vq->broken = false;
-	vq->last_used_idx = 0;
-	vq->event_triggered = false;
-	vq->num_added = 0;
-	vq->use_dma_api = vring_use_dma_api(vdev);
-#ifdef DEBUG
-	vq->in_use = false;
-	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))
-		vq->weak_barriers = false;
-
-	vq->split.avail_flags_shadow = 0;
-	vq->split.avail_idx_shadow = 0;
-
-	/* No callback?  Tell other side not to bother us. */
-	if (!callback) {
-		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
-		if (!vq->event)
-			vq->split.vring.avail->flags = cpu_to_virtio16(vdev,
-					vq->split.avail_flags_shadow);
-	}
 
 	err = __vring_alloc_state_extra_split(vring.num, &state, &extra);
 	if (err) {
@@ -2293,9 +2302,7 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
 	}
 
 	__vring_virtqueue_attach_split(vq, vring, state, extra);
-
-	/* Put everything in free lists. */
-	vq->free_head = 0;
+	__vring_virtqueue_init_split(vq, vdev);
 
 	spin_lock(&vdev->vqs_list_lock);
 	list_add_tail(&vq->vq.list, &vdev->vqs);
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()

[Xuan Zhuo]

virtio ring split supports resize.

Only after the new vring is successfully allocated based on the new num,
we will release the old vring. In any case, an error is returned,
indicating that the vring still points to the old vring. In the case of
an error, we will re-initialize the state of the vring to ensure that
the vring can be used.

In addition, vring_align, may_reduce_num are necessary for reallocating
vring, so they are retained for creating vq.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 81bbfd65411e..a15869514146 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -139,6 +139,12 @@ struct vring_virtqueue {
 			/* DMA address and size information */
 			dma_addr_t queue_dma_addr;
 			size_t queue_size_in_bytes;
+
+			/* The parameters for creating vrings are reserved for
+			 * creating new vrings when enabling reset queue.
+			 */
+			u32 vring_align;
+			bool may_reduce_num;
 		} split;
 
 		/* Available for packed ring */
@@ -198,6 +204,16 @@ struct vring_virtqueue {
 #endif
 };
 
+static void __vring_free(struct virtqueue *_vq);
+static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
+					 struct virtio_device *vdev);
+static void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
+					   struct vring vring,
+					   struct vring_desc_state_split *desc_state,
+					   struct vring_desc_extra *desc_extra);
+static int __vring_alloc_state_extra_split(u32 num,
+					   struct vring_desc_state_split **desc_state,
+					   struct vring_desc_extra **desc_extra);
 
 /*
  * Helpers.
@@ -991,6 +1007,8 @@ static struct virtqueue *vring_create_virtqueue_split(
 		return NULL;
 	}
 
+	to_vvq(vq)->split.vring_align = vring_align;
+	to_vvq(vq)->split.may_reduce_num = may_reduce_num;
 	to_vvq(vq)->split.queue_dma_addr = dma_addr;
 	to_vvq(vq)->split.queue_size_in_bytes = queue_size_in_bytes;
 	to_vvq(vq)->we_own_ring = true;
@@ -998,6 +1016,50 @@ static struct virtqueue *vring_create_virtqueue_split(
 	return vq;
 }
 
+static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct virtio_device *vdev = _vq->vdev;
+	struct vring_desc_state_split *state;
+	struct vring_desc_extra *extra;
+	size_t queue_size_in_bytes;
+	dma_addr_t dma_addr;
+	struct vring vring;
+	int err = -ENOMEM;
+	void *queue;
+
+	BUG_ON(!vq->we_own_ring);
+
+	queue = vring_alloc_queue_split(vdev, &dma_addr, &num,
+					vq->split.vring_align,
+					vq->weak_barriers,
+					vq->split.may_reduce_num);
+	if (!queue)
+		goto init;
+
+	queue_size_in_bytes = vring_size(num, vq->split.vring_align);
+
+	err = __vring_alloc_state_extra_split(num, &state, &extra);
+	if (err) {
+		vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
+		goto init;
+	}
+
+	__vring_free(&vq->vq);
+
+	vring_init(&vring, num, queue, vq->split.vring_align);
+	__vring_virtqueue_attach_split(vq, vring, state, extra);
+	vq->split.queue_dma_addr = dma_addr;
+	vq->split.queue_size_in_bytes = queue_size_in_bytes;
+
+	err = 0;
+
+init:
+	__vring_virtqueue_init_split(vq, vdev);
+	vq->we_own_ring = true;
+	return err;
+}
+
 
 /*
  * Packed ring specific functions - *_packed().
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> virtio ring split supports resize.
>
> Only after the new vring is successfully allocated based on the new num,
> we will release the old vring. In any case, an error is returned,
> indicating that the vring still points to the old vring. In the case of
> an error, we will re-initialize the state of the vring to ensure that
> the vring can be used.
>
> In addition, vring_align, may_reduce_num are necessary for reallocating
> vring, so they are retained for creating vq.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/virtio/virtio_ring.c | 62 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 62 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 81bbfd65411e..a15869514146 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -139,6 +139,12 @@ struct vring_virtqueue {
>   			/* DMA address and size information */
>   			dma_addr_t queue_dma_addr;
>   			size_t queue_size_in_bytes;
> +
> +			/* The parameters for creating vrings are reserved for
> +			 * creating new vrings when enabling reset queue.
> +			 */
> +			u32 vring_align;
> +			bool may_reduce_num;
>   		} split;
>   
>   		/* Available for packed ring */
> @@ -198,6 +204,16 @@ struct vring_virtqueue {
>   #endif
>   };
>   
> +static void __vring_free(struct virtqueue *_vq);
> +static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
> +					 struct virtio_device *vdev);
> +static void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
> +					   struct vring vring,
> +					   struct vring_desc_state_split *desc_state,
> +					   struct vring_desc_extra *desc_extra);
> +static int __vring_alloc_state_extra_split(u32 num,
> +					   struct vring_desc_state_split **desc_state,
> +					   struct vring_desc_extra **desc_extra);
>   
>   /*
>    * Helpers.
> @@ -991,6 +1007,8 @@ static struct virtqueue *vring_create_virtqueue_split(
>   		return NULL;
>   	}
>   
> +	to_vvq(vq)->split.vring_align = vring_align;
> +	to_vvq(vq)->split.may_reduce_num = may_reduce_num;
>   	to_vvq(vq)->split.queue_dma_addr = dma_addr;
>   	to_vvq(vq)->split.queue_size_in_bytes = queue_size_in_bytes;
>   	to_vvq(vq)->we_own_ring = true;
> @@ -998,6 +1016,50 @@ static struct virtqueue *vring_create_virtqueue_split(
>   	return vq;
>   }
>   
> +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
> +{
> +	struct vring_virtqueue *vq = to_vvq(_vq);
> +	struct virtio_device *vdev = _vq->vdev;
> +	struct vring_desc_state_split *state;
> +	struct vring_desc_extra *extra;
> +	size_t queue_size_in_bytes;
> +	dma_addr_t dma_addr;
> +	struct vring vring;
> +	int err = -ENOMEM;
> +	void *queue;
> +
> +	BUG_ON(!vq->we_own_ring);


I don't see any checks in virtqueue_resize(). So I think it's better to 
either

1) return -EINVAL here

or

2) add a check in virtqueue_resize and fail there


> +
> +	queue = vring_alloc_queue_split(vdev, &dma_addr, &num,
> +					vq->split.vring_align,
> +					vq->weak_barriers,
> +					vq->split.may_reduce_num);
> +	if (!queue)
> +		goto init;
> +
> +	queue_size_in_bytes = vring_size(num, vq->split.vring_align);
> +
> +	err = __vring_alloc_state_extra_split(num, &state, &extra);
> +	if (err) {
> +		vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
> +		goto init;
> +	}
> +
> +	__vring_free(&vq->vq);
> +
> +	vring_init(&vring, num, queue, vq->split.vring_align);
> +	__vring_virtqueue_attach_split(vq, vring, state, extra);


I wonder if we need a symmetric virtqueue_resize_detach() internal helper.


> +	vq->split.queue_dma_addr = dma_addr;
> +	vq->split.queue_size_in_bytes = queue_size_in_bytes;
> +
> +	err = 0;
> +
> +init:
> +	__vring_virtqueue_init_split(vq, vdev);
> +	vq->we_own_ring = true;


Then we can leave this unchanged.

Thanks


> +	return err;
> +}
> +
>   
>   /*
>    * Packed ring specific functions - *_packed().

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()

[Xuan Zhuo]

On Tue, 22 Mar 2022 14:30:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > virtio ring split supports resize.
> >
> > Only after the new vring is successfully allocated based on the new num,
> > we will release the old vring. In any case, an error is returned,
> > indicating that the vring still points to the old vring. In the case of
> > an error, we will re-initialize the state of the vring to ensure that
> > the vring can be used.
> >
> > In addition, vring_align, may_reduce_num are necessary for reallocating
> > vring, so they are retained for creating vq.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_ring.c | 62 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 62 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 81bbfd65411e..a15869514146 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -139,6 +139,12 @@ struct vring_virtqueue {
> >   			/* DMA address and size information */
> >   			dma_addr_t queue_dma_addr;
> >   			size_t queue_size_in_bytes;
> > +
> > +			/* The parameters for creating vrings are reserved for
> > +			 * creating new vrings when enabling reset queue.
> > +			 */
> > +			u32 vring_align;
> > +			bool may_reduce_num;
> >   		} split;
> >
> >   		/* Available for packed ring */
> > @@ -198,6 +204,16 @@ struct vring_virtqueue {
> >   #endif
> >   };
> >
> > +static void __vring_free(struct virtqueue *_vq);
> > +static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
> > +					 struct virtio_device *vdev);
> > +static void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
> > +					   struct vring vring,
> > +					   struct vring_desc_state_split *desc_state,
> > +					   struct vring_desc_extra *desc_extra);
> > +static int __vring_alloc_state_extra_split(u32 num,
> > +					   struct vring_desc_state_split **desc_state,
> > +					   struct vring_desc_extra **desc_extra);
> >
> >   /*
> >    * Helpers.
> > @@ -991,6 +1007,8 @@ static struct virtqueue *vring_create_virtqueue_split(
> >   		return NULL;
> >   	}
> >
> > +	to_vvq(vq)->split.vring_align = vring_align;
> > +	to_vvq(vq)->split.may_reduce_num = may_reduce_num;
> >   	to_vvq(vq)->split.queue_dma_addr = dma_addr;
> >   	to_vvq(vq)->split.queue_size_in_bytes = queue_size_in_bytes;
> >   	to_vvq(vq)->we_own_ring = true;
> > @@ -998,6 +1016,50 @@ static struct virtqueue *vring_create_virtqueue_split(
> >   	return vq;
> >   }
> >
> > +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
> > +{
> > +	struct vring_virtqueue *vq = to_vvq(_vq);
> > +	struct virtio_device *vdev = _vq->vdev;
> > +	struct vring_desc_state_split *state;
> > +	struct vring_desc_extra *extra;
> > +	size_t queue_size_in_bytes;
> > +	dma_addr_t dma_addr;
> > +	struct vring vring;
> > +	int err = -ENOMEM;
> > +	void *queue;
> > +
> > +	BUG_ON(!vq->we_own_ring);
>
>
> I don't see any checks in virtqueue_resize(). So I think it's better to
> either
>
> 1) return -EINVAL here
>
> or
>
> 2) add a check in virtqueue_resize and fail there
>
>
> > +
> > +	queue = vring_alloc_queue_split(vdev, &dma_addr, &num,
> > +					vq->split.vring_align,
> > +					vq->weak_barriers,
> > +					vq->split.may_reduce_num);
> > +	if (!queue)
> > +		goto init;
> > +
> > +	queue_size_in_bytes = vring_size(num, vq->split.vring_align);
> > +
> > +	err = __vring_alloc_state_extra_split(num, &state, &extra);
> > +	if (err) {
> > +		vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
> > +		goto init;
> > +	}
> > +
> > +	__vring_free(&vq->vq);
> > +
> > +	vring_init(&vring, num, queue, vq->split.vring_align);
> > +	__vring_virtqueue_attach_split(vq, vring, state, extra);
>
>
> I wonder if we need a symmetric virtqueue_resize_detach() internal helper.

I think __vring_free() is somewhat similar to what you said about
virtqueue_resize_detach() .

>
>
> > +	vq->split.queue_dma_addr = dma_addr;
> > +	vq->split.queue_size_in_bytes = queue_size_in_bytes;
> > +
> > +	err = 0;
> > +
> > +init:
> > +	__vring_virtqueue_init_split(vq, vdev);
> > +	vq->we_own_ring = true;
>
>
> Then we can leave this unchanged.

I think you mean "vq->we_own_ring = true";

The reason for modifying we_own_ring alone is that in the general process of
creating a ring, __vring_virtqueue_init_split is called in
__vring_new_virtqueue. At this time, we_own_ring is false.
vring_create_virtqueue_split will change it to true. So after calling
__vring_virtqueue_init_split alone, we_own_ring is false.

I think it's possible to wrap __vring_virtqueue_init_split() again

static void vring_virtqueue_init_split(struct vring_virtqueue *vq,
					 struct virtio_device *vdev)
{
	__vring_virtqueue_init_split(vq, vdev);
	vq->we_own_ring = true;
}

Is this what you want?

Thanks.


>
> Thanks
>
>
> > +	return err;
> > +}
> > +
> >
> >   /*
> >    * Packed ring specific functions - *_packed().
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()

[Jason Wang]

在 2022/3/24 下午4:44, Xuan Zhuo 写道:
> On Tue, 22 Mar 2022 14:30:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
>>> virtio ring split supports resize.
>>>
>>> Only after the new vring is successfully allocated based on the new num,
>>> we will release the old vring. In any case, an error is returned,
>>> indicating that the vring still points to the old vring. In the case of
>>> an error, we will re-initialize the state of the vring to ensure that
>>> the vring can be used.
>>>
>>> In addition, vring_align, may_reduce_num are necessary for reallocating
>>> vring, so they are retained for creating vq.
>>>
>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> ---
>>>    drivers/virtio/virtio_ring.c | 62 ++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 62 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index 81bbfd65411e..a15869514146 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -139,6 +139,12 @@ struct vring_virtqueue {
>>>    			/* DMA address and size information */
>>>    			dma_addr_t queue_dma_addr;
>>>    			size_t queue_size_in_bytes;
>>> +
>>> +			/* The parameters for creating vrings are reserved for
>>> +			 * creating new vrings when enabling reset queue.
>>> +			 */
>>> +			u32 vring_align;
>>> +			bool may_reduce_num;
>>>    		} split;
>>>
>>>    		/* Available for packed ring */
>>> @@ -198,6 +204,16 @@ struct vring_virtqueue {
>>>    #endif
>>>    };
>>>
>>> +static void __vring_free(struct virtqueue *_vq);
>>> +static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
>>> +					 struct virtio_device *vdev);
>>> +static void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
>>> +					   struct vring vring,
>>> +					   struct vring_desc_state_split *desc_state,
>>> +					   struct vring_desc_extra *desc_extra);
>>> +static int __vring_alloc_state_extra_split(u32 num,
>>> +					   struct vring_desc_state_split **desc_state,
>>> +					   struct vring_desc_extra **desc_extra);
>>>
>>>    /*
>>>     * Helpers.
>>> @@ -991,6 +1007,8 @@ static struct virtqueue *vring_create_virtqueue_split(
>>>    		return NULL;
>>>    	}
>>>
>>> +	to_vvq(vq)->split.vring_align = vring_align;
>>> +	to_vvq(vq)->split.may_reduce_num = may_reduce_num;
>>>    	to_vvq(vq)->split.queue_dma_addr = dma_addr;
>>>    	to_vvq(vq)->split.queue_size_in_bytes = queue_size_in_bytes;
>>>    	to_vvq(vq)->we_own_ring = true;
>>> @@ -998,6 +1016,50 @@ static struct virtqueue *vring_create_virtqueue_split(
>>>    	return vq;
>>>    }
>>>
>>> +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
>>> +{
>>> +	struct vring_virtqueue *vq = to_vvq(_vq);
>>> +	struct virtio_device *vdev = _vq->vdev;
>>> +	struct vring_desc_state_split *state;
>>> +	struct vring_desc_extra *extra;
>>> +	size_t queue_size_in_bytes;
>>> +	dma_addr_t dma_addr;
>>> +	struct vring vring;
>>> +	int err = -ENOMEM;
>>> +	void *queue;
>>> +
>>> +	BUG_ON(!vq->we_own_ring);
>>
>> I don't see any checks in virtqueue_resize(). So I think it's better to
>> either
>>
>> 1) return -EINVAL here
>>
>> or
>>
>> 2) add a check in virtqueue_resize and fail there
>>
>>
>>> +
>>> +	queue = vring_alloc_queue_split(vdev, &dma_addr, &num,
>>> +					vq->split.vring_align,
>>> +					vq->weak_barriers,
>>> +					vq->split.may_reduce_num);
>>> +	if (!queue)
>>> +		goto init;
>>> +
>>> +	queue_size_in_bytes = vring_size(num, vq->split.vring_align);
>>> +
>>> +	err = __vring_alloc_state_extra_split(num, &state, &extra);
>>> +	if (err) {
>>> +		vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
>>> +		goto init;
>>> +	}
>>> +
>>> +	__vring_free(&vq->vq);
>>> +
>>> +	vring_init(&vring, num, queue, vq->split.vring_align);
>>> +	__vring_virtqueue_attach_split(vq, vring, state, extra);
>>
>> I wonder if we need a symmetric virtqueue_resize_detach() internal helper.
> I think __vring_free() is somewhat similar to what you said about
> virtqueue_resize_detach() .


So from what I'm understanding the code, the key point for attaching is:

         vq->split.vring = vring;

But this is not what __vring_free() did, it just free the resources.


>
>>
>>> +	vq->split.queue_dma_addr = dma_addr;
>>> +	vq->split.queue_size_in_bytes = queue_size_in_bytes;
>>> +
>>> +	err = 0;
>>> +
>>> +init:
>>> +	__vring_virtqueue_init_split(vq, vdev);
>>> +	vq->we_own_ring = true;
>>
>> Then we can leave this unchanged.
> I think you mean "vq->we_own_ring = true";
>
> The reason for modifying we_own_ring alone is that in the general process of
> creating a ring, __vring_virtqueue_init_split is called in
> __vring_new_virtqueue. At this time, we_own_ring is false.
> vring_create_virtqueue_split will change it to true. So after calling
> __vring_virtqueue_init_split alone, we_own_ring is false.
>
> I think it's possible to wrap __vring_virtqueue_init_split() again
>
> static void vring_virtqueue_init_split(struct vring_virtqueue *vq,
> 					 struct virtio_device *vdev)
> {
> 	__vring_virtqueue_init_split(vq, vdev);
> 	vq->we_own_ring = true;
> }
>
> Is this what you want?


Nope, I meant there are some transports that allocate the vring by 
themselves, we can't resize those vring.

See the callers of vring_new_virtqueue()

Thanks


>
> Thanks.
>
>
>> Thanks
>>
>>
>>> +	return err;
>>> +}
>>> +
>>>
>>>    /*
>>>     * Packed ring specific functions - *_packed().

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()

[Xuan Zhuo]

On Wed, 30 Mar 2022 11:48:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/3/24 下午4:44, Xuan Zhuo 写道:
> > On Tue, 22 Mar 2022 14:30:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> >>> virtio ring split supports resize.
> >>>
> >>> Only after the new vring is successfully allocated based on the new num,
> >>> we will release the old vring. In any case, an error is returned,
> >>> indicating that the vring still points to the old vring. In the case of
> >>> an error, we will re-initialize the state of the vring to ensure that
> >>> the vring can be used.
> >>>
> >>> In addition, vring_align, may_reduce_num are necessary for reallocating
> >>> vring, so they are retained for creating vq.
> >>>
> >>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>> ---
> >>>    drivers/virtio/virtio_ring.c | 62 ++++++++++++++++++++++++++++++++++++
> >>>    1 file changed, 62 insertions(+)
> >>>
> >>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> >>> index 81bbfd65411e..a15869514146 100644
> >>> --- a/drivers/virtio/virtio_ring.c
> >>> +++ b/drivers/virtio/virtio_ring.c
> >>> @@ -139,6 +139,12 @@ struct vring_virtqueue {
> >>>    			/* DMA address and size information */
> >>>    			dma_addr_t queue_dma_addr;
> >>>    			size_t queue_size_in_bytes;
> >>> +
> >>> +			/* The parameters for creating vrings are reserved for
> >>> +			 * creating new vrings when enabling reset queue.
> >>> +			 */
> >>> +			u32 vring_align;
> >>> +			bool may_reduce_num;
> >>>    		} split;
> >>>
> >>>    		/* Available for packed ring */
> >>> @@ -198,6 +204,16 @@ struct vring_virtqueue {
> >>>    #endif
> >>>    };
> >>>
> >>> +static void __vring_free(struct virtqueue *_vq);
> >>> +static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
> >>> +					 struct virtio_device *vdev);
> >>> +static void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
> >>> +					   struct vring vring,
> >>> +					   struct vring_desc_state_split *desc_state,
> >>> +					   struct vring_desc_extra *desc_extra);
> >>> +static int __vring_alloc_state_extra_split(u32 num,
> >>> +					   struct vring_desc_state_split **desc_state,
> >>> +					   struct vring_desc_extra **desc_extra);
> >>>
> >>>    /*
> >>>     * Helpers.
> >>> @@ -991,6 +1007,8 @@ static struct virtqueue *vring_create_virtqueue_split(
> >>>    		return NULL;
> >>>    	}
> >>>
> >>> +	to_vvq(vq)->split.vring_align = vring_align;
> >>> +	to_vvq(vq)->split.may_reduce_num = may_reduce_num;
> >>>    	to_vvq(vq)->split.queue_dma_addr = dma_addr;
> >>>    	to_vvq(vq)->split.queue_size_in_bytes = queue_size_in_bytes;
> >>>    	to_vvq(vq)->we_own_ring = true;
> >>> @@ -998,6 +1016,50 @@ static struct virtqueue *vring_create_virtqueue_split(
> >>>    	return vq;
> >>>    }
> >>>
> >>> +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
> >>> +{
> >>> +	struct vring_virtqueue *vq = to_vvq(_vq);
> >>> +	struct virtio_device *vdev = _vq->vdev;
> >>> +	struct vring_desc_state_split *state;
> >>> +	struct vring_desc_extra *extra;
> >>> +	size_t queue_size_in_bytes;
> >>> +	dma_addr_t dma_addr;
> >>> +	struct vring vring;
> >>> +	int err = -ENOMEM;
> >>> +	void *queue;
> >>> +
> >>> +	BUG_ON(!vq->we_own_ring);
> >>
> >> I don't see any checks in virtqueue_resize(). So I think it's better to
> >> either
> >>
> >> 1) return -EINVAL here
> >>
> >> or
> >>
> >> 2) add a check in virtqueue_resize and fail there
> >>
> >>
> >>> +
> >>> +	queue = vring_alloc_queue_split(vdev, &dma_addr, &num,
> >>> +					vq->split.vring_align,
> >>> +					vq->weak_barriers,
> >>> +					vq->split.may_reduce_num);
> >>> +	if (!queue)
> >>> +		goto init;
> >>> +
> >>> +	queue_size_in_bytes = vring_size(num, vq->split.vring_align);
> >>> +
> >>> +	err = __vring_alloc_state_extra_split(num, &state, &extra);
> >>> +	if (err) {
> >>> +		vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
> >>> +		goto init;
> >>> +	}
> >>> +
> >>> +	__vring_free(&vq->vq);
> >>> +
> >>> +	vring_init(&vring, num, queue, vq->split.vring_align);
> >>> +	__vring_virtqueue_attach_split(vq, vring, state, extra);
> >>
> >> I wonder if we need a symmetric virtqueue_resize_detach() internal helper.
> > I think __vring_free() is somewhat similar to what you said about
> > virtqueue_resize_detach() .
>
>
> So from what I'm understanding the code, the key point for attaching is:
>
>          vq->split.vring = vring;
>
> But this is not what __vring_free() did, it just free the resources.

OK.

>
>
> >
> >>
> >>> +	vq->split.queue_dma_addr = dma_addr;
> >>> +	vq->split.queue_size_in_bytes = queue_size_in_bytes;
> >>> +
> >>> +	err = 0;
> >>> +
> >>> +init:
> >>> +	__vring_virtqueue_init_split(vq, vdev);
> >>> +	vq->we_own_ring = true;
> >>
> >> Then we can leave this unchanged.
> > I think you mean "vq->we_own_ring = true";
> >
> > The reason for modifying we_own_ring alone is that in the general process of
> > creating a ring, __vring_virtqueue_init_split is called in
> > __vring_new_virtqueue. At this time, we_own_ring is false.
> > vring_create_virtqueue_split will change it to true. So after calling
> > __vring_virtqueue_init_split alone, we_own_ring is false.
> >
> > I think it's possible to wrap __vring_virtqueue_init_split() again
> >
> > static void vring_virtqueue_init_split(struct vring_virtqueue *vq,
> > 					 struct virtio_device *vdev)
> > {
> > 	__vring_virtqueue_init_split(vq, vdev);
> > 	vq->we_own_ring = true;
> > }
> >
> > Is this what you want?
>
>
> Nope, I meant there are some transports that allocate the vring by
> themselves, we can't resize those vring.
>
> See the callers of vring_new_virtqueue()

So, you mean, do not implement resize for we_own_ring is false.

Thanks.

>
> Thanks
>
>
> >
> > Thanks.
> >
> >
> >> Thanks
> >>
> >>
> >>> +	return err;
> >>> +}
> >>> +
> >>>
> >>>    /*
> >>>     * Packed ring specific functions - *_packed().
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 10/16] virtio_ring: split: implement virtqueue_resize_split()

[Jason Wang]

On Wed, Mar 30, 2022 at 2:15 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 30 Mar 2022 11:48:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/3/24 下午4:44, Xuan Zhuo 写道:
> > > On Tue, 22 Mar 2022 14:30:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > >>> virtio ring split supports resize.
> > >>>
> > >>> Only after the new vring is successfully allocated based on the new num,
> > >>> we will release the old vring. In any case, an error is returned,
> > >>> indicating that the vring still points to the old vring. In the case of
> > >>> an error, we will re-initialize the state of the vring to ensure that
> > >>> the vring can be used.
> > >>>
> > >>> In addition, vring_align, may_reduce_num are necessary for reallocating
> > >>> vring, so they are retained for creating vq.
> > >>>
> > >>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > >>> ---
> > >>>    drivers/virtio/virtio_ring.c | 62 ++++++++++++++++++++++++++++++++++++
> > >>>    1 file changed, 62 insertions(+)
> > >>>
> > >>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > >>> index 81bbfd65411e..a15869514146 100644
> > >>> --- a/drivers/virtio/virtio_ring.c
> > >>> +++ b/drivers/virtio/virtio_ring.c
> > >>> @@ -139,6 +139,12 @@ struct vring_virtqueue {
> > >>>                           /* DMA address and size information */
> > >>>                           dma_addr_t queue_dma_addr;
> > >>>                           size_t queue_size_in_bytes;
> > >>> +
> > >>> +                 /* The parameters for creating vrings are reserved for
> > >>> +                  * creating new vrings when enabling reset queue.
> > >>> +                  */
> > >>> +                 u32 vring_align;
> > >>> +                 bool may_reduce_num;
> > >>>                   } split;
> > >>>
> > >>>                   /* Available for packed ring */
> > >>> @@ -198,6 +204,16 @@ struct vring_virtqueue {
> > >>>    #endif
> > >>>    };
> > >>>
> > >>> +static void __vring_free(struct virtqueue *_vq);
> > >>> +static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
> > >>> +                                  struct virtio_device *vdev);
> > >>> +static void __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
> > >>> +                                    struct vring vring,
> > >>> +                                    struct vring_desc_state_split *desc_state,
> > >>> +                                    struct vring_desc_extra *desc_extra);
> > >>> +static int __vring_alloc_state_extra_split(u32 num,
> > >>> +                                    struct vring_desc_state_split **desc_state,
> > >>> +                                    struct vring_desc_extra **desc_extra);
> > >>>
> > >>>    /*
> > >>>     * Helpers.
> > >>> @@ -991,6 +1007,8 @@ static struct virtqueue *vring_create_virtqueue_split(
> > >>>                   return NULL;
> > >>>           }
> > >>>
> > >>> + to_vvq(vq)->split.vring_align = vring_align;
> > >>> + to_vvq(vq)->split.may_reduce_num = may_reduce_num;
> > >>>           to_vvq(vq)->split.queue_dma_addr = dma_addr;
> > >>>           to_vvq(vq)->split.queue_size_in_bytes = queue_size_in_bytes;
> > >>>           to_vvq(vq)->we_own_ring = true;
> > >>> @@ -998,6 +1016,50 @@ static struct virtqueue *vring_create_virtqueue_split(
> > >>>           return vq;
> > >>>    }
> > >>>
> > >>> +static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
> > >>> +{
> > >>> + struct vring_virtqueue *vq = to_vvq(_vq);
> > >>> + struct virtio_device *vdev = _vq->vdev;
> > >>> + struct vring_desc_state_split *state;
> > >>> + struct vring_desc_extra *extra;
> > >>> + size_t queue_size_in_bytes;
> > >>> + dma_addr_t dma_addr;
> > >>> + struct vring vring;
> > >>> + int err = -ENOMEM;
> > >>> + void *queue;
> > >>> +
> > >>> + BUG_ON(!vq->we_own_ring);
> > >>
> > >> I don't see any checks in virtqueue_resize(). So I think it's better to
> > >> either
> > >>
> > >> 1) return -EINVAL here
> > >>
> > >> or
> > >>
> > >> 2) add a check in virtqueue_resize and fail there
> > >>
> > >>
> > >>> +
> > >>> + queue = vring_alloc_queue_split(vdev, &dma_addr, &num,
> > >>> +                                 vq->split.vring_align,
> > >>> +                                 vq->weak_barriers,
> > >>> +                                 vq->split.may_reduce_num);
> > >>> + if (!queue)
> > >>> +         goto init;
> > >>> +
> > >>> + queue_size_in_bytes = vring_size(num, vq->split.vring_align);
> > >>> +
> > >>> + err = __vring_alloc_state_extra_split(num, &state, &extra);
> > >>> + if (err) {
> > >>> +         vring_free_queue(vdev, queue_size_in_bytes, queue, dma_addr);
> > >>> +         goto init;
> > >>> + }
> > >>> +
> > >>> + __vring_free(&vq->vq);
> > >>> +
> > >>> + vring_init(&vring, num, queue, vq->split.vring_align);
> > >>> + __vring_virtqueue_attach_split(vq, vring, state, extra);
> > >>
> > >> I wonder if we need a symmetric virtqueue_resize_detach() internal helper.
> > > I think __vring_free() is somewhat similar to what you said about
> > > virtqueue_resize_detach() .
> >
> >
> > So from what I'm understanding the code, the key point for attaching is:
> >
> >          vq->split.vring = vring;
> >
> > But this is not what __vring_free() did, it just free the resources.
>
> OK.
>
> >
> >
> > >
> > >>
> > >>> + vq->split.queue_dma_addr = dma_addr;
> > >>> + vq->split.queue_size_in_bytes = queue_size_in_bytes;
> > >>> +
> > >>> + err = 0;
> > >>> +
> > >>> +init:
> > >>> + __vring_virtqueue_init_split(vq, vdev);
> > >>> + vq->we_own_ring = true;
> > >>
> > >> Then we can leave this unchanged.
> > > I think you mean "vq->we_own_ring = true";
> > >
> > > The reason for modifying we_own_ring alone is that in the general process of
> > > creating a ring, __vring_virtqueue_init_split is called in
> > > __vring_new_virtqueue. At this time, we_own_ring is false.
> > > vring_create_virtqueue_split will change it to true. So after calling
> > > __vring_virtqueue_init_split alone, we_own_ring is false.
> > >
> > > I think it's possible to wrap __vring_virtqueue_init_split() again
> > >
> > > static void vring_virtqueue_init_split(struct vring_virtqueue *vq,
> > >                                      struct virtio_device *vdev)
> > > {
> > >     __vring_virtqueue_init_split(vq, vdev);
> > >     vq->we_own_ring = true;
> > > }
> > >
> > > Is this what you want?
> >
> >
> > Nope, I meant there are some transports that allocate the vring by
> > themselves, we can't resize those vring.
> >
> > See the callers of vring_new_virtqueue()
>
> So, you mean, do not implement resize for we_own_ring is false.

Yes, I think so. This is because the vring is not allocated by us,
even if we resize there's no way for the user to know about that.

Thanks

>
> Thanks.
>
> >
> > Thanks
> >
> >
> > >
> > > Thanks.
> > >
> > >
> > >> Thanks
> > >>
> > >>
> > >>> + return err;
> > >>> +}
> > >>> +
> > >>>
> > >>>    /*
> > >>>     * Packed ring specific functions - *_packed().
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 11/16] virtio_ring: packed: extract the logic of alloc queue

[Xuan Zhuo]

Separate the logic of packed to create vring queue.

For the convenience of passing parameters, add a structure
vring_packed.

This feature is required for subsequent virtuqueue reset vring.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a15869514146..96ff2dabda14 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -85,6 +85,18 @@ struct vring_desc_extra {
 	u16 next;			/* The next desc state in a list. */
 };
 
+struct vring_packed {
+	u32 num;
+	struct vring_packed_desc *ring;
+	struct vring_packed_desc_event *driver;
+	struct vring_packed_desc_event *device;
+	dma_addr_t ring_dma_addr;
+	dma_addr_t driver_event_dma_addr;
+	dma_addr_t device_event_dma_addr;
+	size_t ring_size_in_bytes;
+	size_t event_size_in_bytes;
+};
+
 struct vring_virtqueue {
 	struct virtqueue vq;
 
@@ -1735,45 +1747,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num)
 	return desc_extra;
 }
 
-static struct virtqueue *vring_create_virtqueue_packed(
-	unsigned int index,
-	unsigned int num,
-	unsigned int vring_align,
-	struct virtio_device *vdev,
-	bool weak_barriers,
-	bool may_reduce_num,
-	bool context,
-	bool (*notify)(struct virtqueue *),
-	void (*callback)(struct virtqueue *),
-	const char *name)
+static void vring_free_vring_packed(struct vring_packed *vring,
+				    struct virtio_device *vdev)
+{
+	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
+	struct vring_packed_desc_event *driver, *device;
+	size_t ring_size_in_bytes, event_size_in_bytes;
+	struct vring_packed_desc *ring;
+
+	ring                  = vring->ring;
+	driver                = vring->driver;
+	device                = vring->device;
+	ring_size_in_bytes    = vring->ring_size_in_bytes;
+	event_size_in_bytes   = vring->event_size_in_bytes;
+	ring_dma_addr         = vring->ring_dma_addr;
+	driver_event_dma_addr = vring->driver_event_dma_addr;
+	device_event_dma_addr = vring->device_event_dma_addr;
+
+	if (device)
+		vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
+
+	if (driver)
+		vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
+
+	if (ring)
+		vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
+}
+
+static int vring_alloc_queue_packed(struct vring_packed *vring,
+				    struct virtio_device *vdev,
+				    u32 num)
 {
-	struct vring_virtqueue *vq;
 	struct vring_packed_desc *ring;
 	struct vring_packed_desc_event *driver, *device;
 	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
 	size_t ring_size_in_bytes, event_size_in_bytes;
 
+	memset(vring, 0, sizeof(*vring));
+
 	ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
 
 	ring = vring_alloc_queue(vdev, ring_size_in_bytes,
 				 &ring_dma_addr,
 				 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
 	if (!ring)
-		goto err_ring;
+		goto err;
+
+	vring->num = num;
+	vring->ring = ring;
+	vring->ring_size_in_bytes = ring_size_in_bytes;
+	vring->ring_dma_addr = ring_dma_addr;
 
 	event_size_in_bytes = sizeof(struct vring_packed_desc_event);
+	vring->event_size_in_bytes = event_size_in_bytes;
 
 	driver = vring_alloc_queue(vdev, event_size_in_bytes,
 				   &driver_event_dma_addr,
 				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
 	if (!driver)
-		goto err_driver;
+		goto err;
+
+	vring->driver = driver;
+	vring->driver_event_dma_addr = driver_event_dma_addr;
 
 	device = vring_alloc_queue(vdev, event_size_in_bytes,
 				   &device_event_dma_addr,
 				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
 	if (!device)
-		goto err_device;
+		goto err;
+
+	vring->device = device;
+	vring->device_event_dma_addr = device_event_dma_addr;
+	return 0;
+
+err:
+	vring_free_vring_packed(vring, vdev);
+	return -ENOMEM;
+}
+
+static struct virtqueue *vring_create_virtqueue_packed(
+	unsigned int index,
+	unsigned int num,
+	unsigned int vring_align,
+	struct virtio_device *vdev,
+	bool weak_barriers,
+	bool may_reduce_num,
+	bool context,
+	bool (*notify)(struct virtqueue *),
+	void (*callback)(struct virtqueue *),
+	const char *name)
+{
+	struct vring_virtqueue *vq;
+	struct vring_packed vring;
+
+	if (vring_alloc_queue_packed(&vring, vdev, num))
+		goto err_vq;
 
 	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
 	if (!vq)
@@ -1805,17 +1873,17 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
 		vq->weak_barriers = false;
 
-	vq->packed.ring_dma_addr = ring_dma_addr;
-	vq->packed.driver_event_dma_addr = driver_event_dma_addr;
-	vq->packed.device_event_dma_addr = device_event_dma_addr;
+	vq->packed.ring_dma_addr = vring.ring_dma_addr;
+	vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr;
+	vq->packed.device_event_dma_addr = vring.device_event_dma_addr;
 
-	vq->packed.ring_size_in_bytes = ring_size_in_bytes;
-	vq->packed.event_size_in_bytes = event_size_in_bytes;
+	vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
+	vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
 
 	vq->packed.vring.num = num;
-	vq->packed.vring.desc = ring;
-	vq->packed.vring.driver = driver;
-	vq->packed.vring.device = device;
+	vq->packed.vring.desc = vring.ring;
+	vq->packed.vring.driver = vring.driver;
+	vq->packed.vring.device = vring.device;
 
 	vq->packed.next_avail_idx = 0;
 	vq->packed.avail_wrap_counter = 1;
@@ -1856,12 +1924,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 err_desc_state:
 	kfree(vq);
 err_vq:
-	vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
-err_device:
-	vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
-err_driver:
-	vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
-err_ring:
+	vring_free_vring_packed(&vring, vdev);
 	return NULL;
 }
 
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 11/16] virtio_ring: packed: extract the logic of alloc queue

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> Separate the logic of packed to create vring queue.
>
> For the convenience of passing parameters, add a structure
> vring_packed.
>
> This feature is required for subsequent virtuqueue reset vring.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/virtio/virtio_ring.c | 121 ++++++++++++++++++++++++++---------
>   1 file changed, 92 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index a15869514146..96ff2dabda14 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -85,6 +85,18 @@ struct vring_desc_extra {
>   	u16 next;			/* The next desc state in a list. */
>   };
>   
> +struct vring_packed {
> +	u32 num;
> +	struct vring_packed_desc *ring;
> +	struct vring_packed_desc_event *driver;
> +	struct vring_packed_desc_event *device;
> +	dma_addr_t ring_dma_addr;
> +	dma_addr_t driver_event_dma_addr;
> +	dma_addr_t device_event_dma_addr;
> +	size_t ring_size_in_bytes;
> +	size_t event_size_in_bytes;
> +};
> +


Interesting,  a nitpick here is that if it is only used by one helper, 
it's probably not worth to bother.

And if we want to do that, we need

1) use a separated patch

2) do it for split virtqueue as well

Thanks


>   struct vring_virtqueue {
>   	struct virtqueue vq;
>   
> @@ -1735,45 +1747,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num)
>   	return desc_extra;
>   }
>   
> -static struct virtqueue *vring_create_virtqueue_packed(
> -	unsigned int index,
> -	unsigned int num,
> -	unsigned int vring_align,
> -	struct virtio_device *vdev,
> -	bool weak_barriers,
> -	bool may_reduce_num,
> -	bool context,
> -	bool (*notify)(struct virtqueue *),
> -	void (*callback)(struct virtqueue *),
> -	const char *name)
> +static void vring_free_vring_packed(struct vring_packed *vring,
> +				    struct virtio_device *vdev)
> +{
> +	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
> +	struct vring_packed_desc_event *driver, *device;
> +	size_t ring_size_in_bytes, event_size_in_bytes;
> +	struct vring_packed_desc *ring;
> +
> +	ring                  = vring->ring;
> +	driver                = vring->driver;
> +	device                = vring->device;
> +	ring_size_in_bytes    = vring->ring_size_in_bytes;
> +	event_size_in_bytes   = vring->event_size_in_bytes;
> +	ring_dma_addr         = vring->ring_dma_addr;
> +	driver_event_dma_addr = vring->driver_event_dma_addr;
> +	device_event_dma_addr = vring->device_event_dma_addr;
> +
> +	if (device)
> +		vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> +
> +	if (driver)
> +		vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> +
> +	if (ring)
> +		vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> +}
> +
> +static int vring_alloc_queue_packed(struct vring_packed *vring,
> +				    struct virtio_device *vdev,
> +				    u32 num)
>   {
> -	struct vring_virtqueue *vq;
>   	struct vring_packed_desc *ring;
>   	struct vring_packed_desc_event *driver, *device;
>   	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
>   	size_t ring_size_in_bytes, event_size_in_bytes;
>   
> +	memset(vring, 0, sizeof(*vring));
> +
>   	ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
>   
>   	ring = vring_alloc_queue(vdev, ring_size_in_bytes,
>   				 &ring_dma_addr,
>   				 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>   	if (!ring)
> -		goto err_ring;
> +		goto err;
> +
> +	vring->num = num;
> +	vring->ring = ring;
> +	vring->ring_size_in_bytes = ring_size_in_bytes;
> +	vring->ring_dma_addr = ring_dma_addr;
>   
>   	event_size_in_bytes = sizeof(struct vring_packed_desc_event);
> +	vring->event_size_in_bytes = event_size_in_bytes;
>   
>   	driver = vring_alloc_queue(vdev, event_size_in_bytes,
>   				   &driver_event_dma_addr,
>   				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>   	if (!driver)
> -		goto err_driver;
> +		goto err;
> +
> +	vring->driver = driver;
> +	vring->driver_event_dma_addr = driver_event_dma_addr;
>   
>   	device = vring_alloc_queue(vdev, event_size_in_bytes,
>   				   &device_event_dma_addr,
>   				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>   	if (!device)
> -		goto err_device;
> +		goto err;
> +
> +	vring->device = device;
> +	vring->device_event_dma_addr = device_event_dma_addr;
> +	return 0;
> +
> +err:
> +	vring_free_vring_packed(vring, vdev);
> +	return -ENOMEM;
> +}
> +
> +static struct virtqueue *vring_create_virtqueue_packed(
> +	unsigned int index,
> +	unsigned int num,
> +	unsigned int vring_align,
> +	struct virtio_device *vdev,
> +	bool weak_barriers,
> +	bool may_reduce_num,
> +	bool context,
> +	bool (*notify)(struct virtqueue *),
> +	void (*callback)(struct virtqueue *),
> +	const char *name)
> +{
> +	struct vring_virtqueue *vq;
> +	struct vring_packed vring;
> +
> +	if (vring_alloc_queue_packed(&vring, vdev, num))
> +		goto err_vq;
>   
>   	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
>   	if (!vq)
> @@ -1805,17 +1873,17 @@ static struct virtqueue *vring_create_virtqueue_packed(
>   	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
>   		vq->weak_barriers = false;
>   
> -	vq->packed.ring_dma_addr = ring_dma_addr;
> -	vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> -	vq->packed.device_event_dma_addr = device_event_dma_addr;
> +	vq->packed.ring_dma_addr = vring.ring_dma_addr;
> +	vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr;
> +	vq->packed.device_event_dma_addr = vring.device_event_dma_addr;
>   
> -	vq->packed.ring_size_in_bytes = ring_size_in_bytes;
> -	vq->packed.event_size_in_bytes = event_size_in_bytes;
> +	vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
> +	vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
>   
>   	vq->packed.vring.num = num;
> -	vq->packed.vring.desc = ring;
> -	vq->packed.vring.driver = driver;
> -	vq->packed.vring.device = device;
> +	vq->packed.vring.desc = vring.ring;
> +	vq->packed.vring.driver = vring.driver;
> +	vq->packed.vring.device = vring.device;
>   
>   	vq->packed.next_avail_idx = 0;
>   	vq->packed.avail_wrap_counter = 1;
> @@ -1856,12 +1924,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
>   err_desc_state:
>   	kfree(vq);
>   err_vq:
> -	vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> -err_device:
> -	vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> -err_driver:
> -	vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> -err_ring:
> +	vring_free_vring_packed(&vring, vdev);
>   	return NULL;
>   }
>   

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 11/16] virtio_ring: packed: extract the logic of alloc queue

[Xuan Zhuo]

On Tue, 22 Mar 2022 14:38:12 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > Separate the logic of packed to create vring queue.
> >
> > For the convenience of passing parameters, add a structure
> > vring_packed.
> >
> > This feature is required for subsequent virtuqueue reset vring.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_ring.c | 121 ++++++++++++++++++++++++++---------
> >   1 file changed, 92 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index a15869514146..96ff2dabda14 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -85,6 +85,18 @@ struct vring_desc_extra {
> >   	u16 next;			/* The next desc state in a list. */
> >   };
> >
> > +struct vring_packed {
> > +	u32 num;
> > +	struct vring_packed_desc *ring;
> > +	struct vring_packed_desc_event *driver;
> > +	struct vring_packed_desc_event *device;
> > +	dma_addr_t ring_dma_addr;
> > +	dma_addr_t driver_event_dma_addr;
> > +	dma_addr_t device_event_dma_addr;
> > +	size_t ring_size_in_bytes;
> > +	size_t event_size_in_bytes;
> > +};
> > +
>
>
> Interesting,  a nitpick here is that if it is only used by one helper,
> it's probably not worth to bother.


Indeed only one helper will use it. Because vring_alloc_queue_packed will pass
too many variables. If we use function parameters, I think this function is too
ugly.


>
> And if we want to do that, we need
>
> 1) use a separated patch

If we split this patch again, we can only separate vring_free_vring_packed()
out.

>
> 2) do it for split virtqueue as well

Earlier versions, I did create a struct for split too, but you replied that it
wasn't worth it, so I removed the separate struct used by split.

Thanks.

>
> Thanks
>
>
> >   struct vring_virtqueue {
> >   	struct virtqueue vq;
> >
> > @@ -1735,45 +1747,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num)
> >   	return desc_extra;
> >   }
> >
> > -static struct virtqueue *vring_create_virtqueue_packed(
> > -	unsigned int index,
> > -	unsigned int num,
> > -	unsigned int vring_align,
> > -	struct virtio_device *vdev,
> > -	bool weak_barriers,
> > -	bool may_reduce_num,
> > -	bool context,
> > -	bool (*notify)(struct virtqueue *),
> > -	void (*callback)(struct virtqueue *),
> > -	const char *name)
> > +static void vring_free_vring_packed(struct vring_packed *vring,
> > +				    struct virtio_device *vdev)
> > +{
> > +	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
> > +	struct vring_packed_desc_event *driver, *device;
> > +	size_t ring_size_in_bytes, event_size_in_bytes;
> > +	struct vring_packed_desc *ring;
> > +
> > +	ring                  = vring->ring;
> > +	driver                = vring->driver;
> > +	device                = vring->device;
> > +	ring_size_in_bytes    = vring->ring_size_in_bytes;
> > +	event_size_in_bytes   = vring->event_size_in_bytes;
> > +	ring_dma_addr         = vring->ring_dma_addr;
> > +	driver_event_dma_addr = vring->driver_event_dma_addr;
> > +	device_event_dma_addr = vring->device_event_dma_addr;
> > +
> > +	if (device)
> > +		vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> > +
> > +	if (driver)
> > +		vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> > +
> > +	if (ring)
> > +		vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> > +}
> > +
> > +static int vring_alloc_queue_packed(struct vring_packed *vring,
> > +				    struct virtio_device *vdev,
> > +				    u32 num)
> >   {
> > -	struct vring_virtqueue *vq;
> >   	struct vring_packed_desc *ring;
> >   	struct vring_packed_desc_event *driver, *device;
> >   	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
> >   	size_t ring_size_in_bytes, event_size_in_bytes;
> >
> > +	memset(vring, 0, sizeof(*vring));
> > +
> >   	ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
> >
> >   	ring = vring_alloc_queue(vdev, ring_size_in_bytes,
> >   				 &ring_dma_addr,
> >   				 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> >   	if (!ring)
> > -		goto err_ring;
> > +		goto err;
> > +
> > +	vring->num = num;
> > +	vring->ring = ring;
> > +	vring->ring_size_in_bytes = ring_size_in_bytes;
> > +	vring->ring_dma_addr = ring_dma_addr;
> >
> >   	event_size_in_bytes = sizeof(struct vring_packed_desc_event);
> > +	vring->event_size_in_bytes = event_size_in_bytes;
> >
> >   	driver = vring_alloc_queue(vdev, event_size_in_bytes,
> >   				   &driver_event_dma_addr,
> >   				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> >   	if (!driver)
> > -		goto err_driver;
> > +		goto err;
> > +
> > +	vring->driver = driver;
> > +	vring->driver_event_dma_addr = driver_event_dma_addr;
> >
> >   	device = vring_alloc_queue(vdev, event_size_in_bytes,
> >   				   &device_event_dma_addr,
> >   				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> >   	if (!device)
> > -		goto err_device;
> > +		goto err;
> > +
> > +	vring->device = device;
> > +	vring->device_event_dma_addr = device_event_dma_addr;
> > +	return 0;
> > +
> > +err:
> > +	vring_free_vring_packed(vring, vdev);
> > +	return -ENOMEM;
> > +}
> > +
> > +static struct virtqueue *vring_create_virtqueue_packed(
> > +	unsigned int index,
> > +	unsigned int num,
> > +	unsigned int vring_align,
> > +	struct virtio_device *vdev,
> > +	bool weak_barriers,
> > +	bool may_reduce_num,
> > +	bool context,
> > +	bool (*notify)(struct virtqueue *),
> > +	void (*callback)(struct virtqueue *),
> > +	const char *name)
> > +{
> > +	struct vring_virtqueue *vq;
> > +	struct vring_packed vring;
> > +
> > +	if (vring_alloc_queue_packed(&vring, vdev, num))
> > +		goto err_vq;
> >
> >   	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
> >   	if (!vq)
> > @@ -1805,17 +1873,17 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >   	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> >   		vq->weak_barriers = false;
> >
> > -	vq->packed.ring_dma_addr = ring_dma_addr;
> > -	vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> > -	vq->packed.device_event_dma_addr = device_event_dma_addr;
> > +	vq->packed.ring_dma_addr = vring.ring_dma_addr;
> > +	vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr;
> > +	vq->packed.device_event_dma_addr = vring.device_event_dma_addr;
> >
> > -	vq->packed.ring_size_in_bytes = ring_size_in_bytes;
> > -	vq->packed.event_size_in_bytes = event_size_in_bytes;
> > +	vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
> > +	vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
> >
> >   	vq->packed.vring.num = num;
> > -	vq->packed.vring.desc = ring;
> > -	vq->packed.vring.driver = driver;
> > -	vq->packed.vring.device = device;
> > +	vq->packed.vring.desc = vring.ring;
> > +	vq->packed.vring.driver = vring.driver;
> > +	vq->packed.vring.device = vring.device;
> >
> >   	vq->packed.next_avail_idx = 0;
> >   	vq->packed.avail_wrap_counter = 1;
> > @@ -1856,12 +1924,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >   err_desc_state:
> >   	kfree(vq);
> >   err_vq:
> > -	vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> > -err_device:
> > -	vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> > -err_driver:
> > -	vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> > -err_ring:
> > +	vring_free_vring_packed(&vring, vdev);
> >   	return NULL;
> >   }
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 11/16] virtio_ring: packed: extract the logic of alloc queue

[Jason Wang]

在 2022/3/24 下午4:28, Xuan Zhuo 写道:
> On Tue, 22 Mar 2022 14:38:12 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
>>> Separate the logic of packed to create vring queue.
>>>
>>> For the convenience of passing parameters, add a structure
>>> vring_packed.
>>>
>>> This feature is required for subsequent virtuqueue reset vring.
>>>
>>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>>> ---
>>>    drivers/virtio/virtio_ring.c | 121 ++++++++++++++++++++++++++---------
>>>    1 file changed, 92 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index a15869514146..96ff2dabda14 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -85,6 +85,18 @@ struct vring_desc_extra {
>>>    	u16 next;			/* The next desc state in a list. */
>>>    };
>>>
>>> +struct vring_packed {
>>> +	u32 num;
>>> +	struct vring_packed_desc *ring;
>>> +	struct vring_packed_desc_event *driver;
>>> +	struct vring_packed_desc_event *device;
>>> +	dma_addr_t ring_dma_addr;
>>> +	dma_addr_t driver_event_dma_addr;
>>> +	dma_addr_t device_event_dma_addr;
>>> +	size_t ring_size_in_bytes;
>>> +	size_t event_size_in_bytes;
>>> +};
>>> +
>>
>> Interesting,  a nitpick here is that if it is only used by one helper,
>> it's probably not worth to bother.
>
> Indeed only one helper will use it. Because vring_alloc_queue_packed will pass
> too many variables. If we use function parameters, I think this function is too
> ugly.
>
>
>> And if we want to do that, we need
>>
>> 1) use a separated patch
> If we split this patch again, we can only separate vring_free_vring_packed()
> out.
>
>> 2) do it for split virtqueue as well
> Earlier versions, I did create a struct for split too, but you replied that it
> wasn't worth it, so I removed the separate struct used by split.


So I think let's just stick to the current style by passing individual 
parameters, and we can refactor it on top of this series.

This will let us to focus on the core logic.

Thanks


>
> Thanks.
>
>> Thanks
>>
>>
>>>    struct vring_virtqueue {
>>>    	struct virtqueue vq;
>>>
>>> @@ -1735,45 +1747,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num)
>>>    	return desc_extra;
>>>    }
>>>
>>> -static struct virtqueue *vring_create_virtqueue_packed(
>>> -	unsigned int index,
>>> -	unsigned int num,
>>> -	unsigned int vring_align,
>>> -	struct virtio_device *vdev,
>>> -	bool weak_barriers,
>>> -	bool may_reduce_num,
>>> -	bool context,
>>> -	bool (*notify)(struct virtqueue *),
>>> -	void (*callback)(struct virtqueue *),
>>> -	const char *name)
>>> +static void vring_free_vring_packed(struct vring_packed *vring,
>>> +				    struct virtio_device *vdev)
>>> +{
>>> +	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
>>> +	struct vring_packed_desc_event *driver, *device;
>>> +	size_t ring_size_in_bytes, event_size_in_bytes;
>>> +	struct vring_packed_desc *ring;
>>> +
>>> +	ring                  = vring->ring;
>>> +	driver                = vring->driver;
>>> +	device                = vring->device;
>>> +	ring_size_in_bytes    = vring->ring_size_in_bytes;
>>> +	event_size_in_bytes   = vring->event_size_in_bytes;
>>> +	ring_dma_addr         = vring->ring_dma_addr;
>>> +	driver_event_dma_addr = vring->driver_event_dma_addr;
>>> +	device_event_dma_addr = vring->device_event_dma_addr;
>>> +
>>> +	if (device)
>>> +		vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
>>> +
>>> +	if (driver)
>>> +		vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
>>> +
>>> +	if (ring)
>>> +		vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
>>> +}
>>> +
>>> +static int vring_alloc_queue_packed(struct vring_packed *vring,
>>> +				    struct virtio_device *vdev,
>>> +				    u32 num)
>>>    {
>>> -	struct vring_virtqueue *vq;
>>>    	struct vring_packed_desc *ring;
>>>    	struct vring_packed_desc_event *driver, *device;
>>>    	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
>>>    	size_t ring_size_in_bytes, event_size_in_bytes;
>>>
>>> +	memset(vring, 0, sizeof(*vring));
>>> +
>>>    	ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
>>>
>>>    	ring = vring_alloc_queue(vdev, ring_size_in_bytes,
>>>    				 &ring_dma_addr,
>>>    				 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>>>    	if (!ring)
>>> -		goto err_ring;
>>> +		goto err;
>>> +
>>> +	vring->num = num;
>>> +	vring->ring = ring;
>>> +	vring->ring_size_in_bytes = ring_size_in_bytes;
>>> +	vring->ring_dma_addr = ring_dma_addr;
>>>
>>>    	event_size_in_bytes = sizeof(struct vring_packed_desc_event);
>>> +	vring->event_size_in_bytes = event_size_in_bytes;
>>>
>>>    	driver = vring_alloc_queue(vdev, event_size_in_bytes,
>>>    				   &driver_event_dma_addr,
>>>    				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>>>    	if (!driver)
>>> -		goto err_driver;
>>> +		goto err;
>>> +
>>> +	vring->driver = driver;
>>> +	vring->driver_event_dma_addr = driver_event_dma_addr;
>>>
>>>    	device = vring_alloc_queue(vdev, event_size_in_bytes,
>>>    				   &device_event_dma_addr,
>>>    				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
>>>    	if (!device)
>>> -		goto err_device;
>>> +		goto err;
>>> +
>>> +	vring->device = device;
>>> +	vring->device_event_dma_addr = device_event_dma_addr;
>>> +	return 0;
>>> +
>>> +err:
>>> +	vring_free_vring_packed(vring, vdev);
>>> +	return -ENOMEM;
>>> +}
>>> +
>>> +static struct virtqueue *vring_create_virtqueue_packed(
>>> +	unsigned int index,
>>> +	unsigned int num,
>>> +	unsigned int vring_align,
>>> +	struct virtio_device *vdev,
>>> +	bool weak_barriers,
>>> +	bool may_reduce_num,
>>> +	bool context,
>>> +	bool (*notify)(struct virtqueue *),
>>> +	void (*callback)(struct virtqueue *),
>>> +	const char *name)
>>> +{
>>> +	struct vring_virtqueue *vq;
>>> +	struct vring_packed vring;
>>> +
>>> +	if (vring_alloc_queue_packed(&vring, vdev, num))
>>> +		goto err_vq;
>>>
>>>    	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
>>>    	if (!vq)
>>> @@ -1805,17 +1873,17 @@ static struct virtqueue *vring_create_virtqueue_packed(
>>>    	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
>>>    		vq->weak_barriers = false;
>>>
>>> -	vq->packed.ring_dma_addr = ring_dma_addr;
>>> -	vq->packed.driver_event_dma_addr = driver_event_dma_addr;
>>> -	vq->packed.device_event_dma_addr = device_event_dma_addr;
>>> +	vq->packed.ring_dma_addr = vring.ring_dma_addr;
>>> +	vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr;
>>> +	vq->packed.device_event_dma_addr = vring.device_event_dma_addr;
>>>
>>> -	vq->packed.ring_size_in_bytes = ring_size_in_bytes;
>>> -	vq->packed.event_size_in_bytes = event_size_in_bytes;
>>> +	vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
>>> +	vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
>>>
>>>    	vq->packed.vring.num = num;
>>> -	vq->packed.vring.desc = ring;
>>> -	vq->packed.vring.driver = driver;
>>> -	vq->packed.vring.device = device;
>>> +	vq->packed.vring.desc = vring.ring;
>>> +	vq->packed.vring.driver = vring.driver;
>>> +	vq->packed.vring.device = vring.device;
>>>
>>>    	vq->packed.next_avail_idx = 0;
>>>    	vq->packed.avail_wrap_counter = 1;
>>> @@ -1856,12 +1924,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
>>>    err_desc_state:
>>>    	kfree(vq);
>>>    err_vq:
>>> -	vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
>>> -err_device:
>>> -	vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
>>> -err_driver:
>>> -	vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
>>> -err_ring:
>>> +	vring_free_vring_packed(&vring, vdev);
>>>    	return NULL;
>>>    }
>>>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 11/16] virtio_ring: packed: extract the logic of alloc queue

[Xuan Zhuo]

On Wed, 30 Mar 2022 11:50:49 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/3/24 下午4:28, Xuan Zhuo 写道:
> > On Tue, 22 Mar 2022 14:38:12 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> >>> Separate the logic of packed to create vring queue.
> >>>
> >>> For the convenience of passing parameters, add a structure
> >>> vring_packed.
> >>>
> >>> This feature is required for subsequent virtuqueue reset vring.
> >>>
> >>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >>> ---
> >>>    drivers/virtio/virtio_ring.c | 121 ++++++++++++++++++++++++++---------
> >>>    1 file changed, 92 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> >>> index a15869514146..96ff2dabda14 100644
> >>> --- a/drivers/virtio/virtio_ring.c
> >>> +++ b/drivers/virtio/virtio_ring.c
> >>> @@ -85,6 +85,18 @@ struct vring_desc_extra {
> >>>    	u16 next;			/* The next desc state in a list. */
> >>>    };
> >>>
> >>> +struct vring_packed {
> >>> +	u32 num;
> >>> +	struct vring_packed_desc *ring;
> >>> +	struct vring_packed_desc_event *driver;
> >>> +	struct vring_packed_desc_event *device;
> >>> +	dma_addr_t ring_dma_addr;
> >>> +	dma_addr_t driver_event_dma_addr;
> >>> +	dma_addr_t device_event_dma_addr;
> >>> +	size_t ring_size_in_bytes;
> >>> +	size_t event_size_in_bytes;
> >>> +};
> >>> +
> >>
> >> Interesting,  a nitpick here is that if it is only used by one helper,
> >> it's probably not worth to bother.
> >
> > Indeed only one helper will use it. Because vring_alloc_queue_packed will pass
> > too many variables. If we use function parameters, I think this function is too
> > ugly.
> >
> >
> >> And if we want to do that, we need
> >>
> >> 1) use a separated patch
> > If we split this patch again, we can only separate vring_free_vring_packed()
> > out.
> >
> >> 2) do it for split virtqueue as well
> > Earlier versions, I did create a struct for split too, but you replied that it
> > wasn't worth it, so I removed the separate struct used by split.
>
>
> So I think let's just stick to the current style by passing individual
> parameters, and we can refactor it on top of this series.
>
> This will let us to focus on the core logic.

I am implementing the function of reusing the buffer mentioned by Michael, and
at this time there will be a lot of dependence on this structure. Because I
want to save some information(desc, desc_extra, desc_stete), these structures
are very useful at this time.

The only problem is that this patch set is too big. ^_^

Thanks.

>
> Thanks
>
>
> >
> > Thanks.
> >
> >> Thanks
> >>
> >>
> >>>    struct vring_virtqueue {
> >>>    	struct virtqueue vq;
> >>>
> >>> @@ -1735,45 +1747,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num)
> >>>    	return desc_extra;
> >>>    }
> >>>
> >>> -static struct virtqueue *vring_create_virtqueue_packed(
> >>> -	unsigned int index,
> >>> -	unsigned int num,
> >>> -	unsigned int vring_align,
> >>> -	struct virtio_device *vdev,
> >>> -	bool weak_barriers,
> >>> -	bool may_reduce_num,
> >>> -	bool context,
> >>> -	bool (*notify)(struct virtqueue *),
> >>> -	void (*callback)(struct virtqueue *),
> >>> -	const char *name)
> >>> +static void vring_free_vring_packed(struct vring_packed *vring,
> >>> +				    struct virtio_device *vdev)
> >>> +{
> >>> +	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
> >>> +	struct vring_packed_desc_event *driver, *device;
> >>> +	size_t ring_size_in_bytes, event_size_in_bytes;
> >>> +	struct vring_packed_desc *ring;
> >>> +
> >>> +	ring                  = vring->ring;
> >>> +	driver                = vring->driver;
> >>> +	device                = vring->device;
> >>> +	ring_size_in_bytes    = vring->ring_size_in_bytes;
> >>> +	event_size_in_bytes   = vring->event_size_in_bytes;
> >>> +	ring_dma_addr         = vring->ring_dma_addr;
> >>> +	driver_event_dma_addr = vring->driver_event_dma_addr;
> >>> +	device_event_dma_addr = vring->device_event_dma_addr;
> >>> +
> >>> +	if (device)
> >>> +		vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> >>> +
> >>> +	if (driver)
> >>> +		vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> >>> +
> >>> +	if (ring)
> >>> +		vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> >>> +}
> >>> +
> >>> +static int vring_alloc_queue_packed(struct vring_packed *vring,
> >>> +				    struct virtio_device *vdev,
> >>> +				    u32 num)
> >>>    {
> >>> -	struct vring_virtqueue *vq;
> >>>    	struct vring_packed_desc *ring;
> >>>    	struct vring_packed_desc_event *driver, *device;
> >>>    	dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
> >>>    	size_t ring_size_in_bytes, event_size_in_bytes;
> >>>
> >>> +	memset(vring, 0, sizeof(*vring));
> >>> +
> >>>    	ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
> >>>
> >>>    	ring = vring_alloc_queue(vdev, ring_size_in_bytes,
> >>>    				 &ring_dma_addr,
> >>>    				 GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> >>>    	if (!ring)
> >>> -		goto err_ring;
> >>> +		goto err;
> >>> +
> >>> +	vring->num = num;
> >>> +	vring->ring = ring;
> >>> +	vring->ring_size_in_bytes = ring_size_in_bytes;
> >>> +	vring->ring_dma_addr = ring_dma_addr;
> >>>
> >>>    	event_size_in_bytes = sizeof(struct vring_packed_desc_event);
> >>> +	vring->event_size_in_bytes = event_size_in_bytes;
> >>>
> >>>    	driver = vring_alloc_queue(vdev, event_size_in_bytes,
> >>>    				   &driver_event_dma_addr,
> >>>    				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> >>>    	if (!driver)
> >>> -		goto err_driver;
> >>> +		goto err;
> >>> +
> >>> +	vring->driver = driver;
> >>> +	vring->driver_event_dma_addr = driver_event_dma_addr;
> >>>
> >>>    	device = vring_alloc_queue(vdev, event_size_in_bytes,
> >>>    				   &device_event_dma_addr,
> >>>    				   GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> >>>    	if (!device)
> >>> -		goto err_device;
> >>> +		goto err;
> >>> +
> >>> +	vring->device = device;
> >>> +	vring->device_event_dma_addr = device_event_dma_addr;
> >>> +	return 0;
> >>> +
> >>> +err:
> >>> +	vring_free_vring_packed(vring, vdev);
> >>> +	return -ENOMEM;
> >>> +}
> >>> +
> >>> +static struct virtqueue *vring_create_virtqueue_packed(
> >>> +	unsigned int index,
> >>> +	unsigned int num,
> >>> +	unsigned int vring_align,
> >>> +	struct virtio_device *vdev,
> >>> +	bool weak_barriers,
> >>> +	bool may_reduce_num,
> >>> +	bool context,
> >>> +	bool (*notify)(struct virtqueue *),
> >>> +	void (*callback)(struct virtqueue *),
> >>> +	const char *name)
> >>> +{
> >>> +	struct vring_virtqueue *vq;
> >>> +	struct vring_packed vring;
> >>> +
> >>> +	if (vring_alloc_queue_packed(&vring, vdev, num))
> >>> +		goto err_vq;
> >>>
> >>>    	vq = kmalloc(sizeof(*vq), GFP_KERNEL);
> >>>    	if (!vq)
> >>> @@ -1805,17 +1873,17 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >>>    	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> >>>    		vq->weak_barriers = false;
> >>>
> >>> -	vq->packed.ring_dma_addr = ring_dma_addr;
> >>> -	vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> >>> -	vq->packed.device_event_dma_addr = device_event_dma_addr;
> >>> +	vq->packed.ring_dma_addr = vring.ring_dma_addr;
> >>> +	vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr;
> >>> +	vq->packed.device_event_dma_addr = vring.device_event_dma_addr;
> >>>
> >>> -	vq->packed.ring_size_in_bytes = ring_size_in_bytes;
> >>> -	vq->packed.event_size_in_bytes = event_size_in_bytes;
> >>> +	vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
> >>> +	vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
> >>>
> >>>    	vq->packed.vring.num = num;
> >>> -	vq->packed.vring.desc = ring;
> >>> -	vq->packed.vring.driver = driver;
> >>> -	vq->packed.vring.device = device;
> >>> +	vq->packed.vring.desc = vring.ring;
> >>> +	vq->packed.vring.driver = vring.driver;
> >>> +	vq->packed.vring.device = vring.device;
> >>>
> >>>    	vq->packed.next_avail_idx = 0;
> >>>    	vq->packed.avail_wrap_counter = 1;
> >>> @@ -1856,12 +1924,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> >>>    err_desc_state:
> >>>    	kfree(vq);
> >>>    err_vq:
> >>> -	vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> >>> -err_device:
> >>> -	vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> >>> -err_driver:
> >>> -	vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> >>> -err_ring:
> >>> +	vring_free_vring_packed(&vring, vdev);
> >>>    	return NULL;
> >>>    }
> >>>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 11/16] virtio_ring: packed: extract the logic of alloc queue

[Jason Wang]

On Wed, Mar 30, 2022 at 2:13 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 30 Mar 2022 11:50:49 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/3/24 下午4:28, Xuan Zhuo 写道:
> > > On Tue, 22 Mar 2022 14:38:12 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > >>> Separate the logic of packed to create vring queue.
> > >>>
> > >>> For the convenience of passing parameters, add a structure
> > >>> vring_packed.
> > >>>
> > >>> This feature is required for subsequent virtuqueue reset vring.
> > >>>
> > >>> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > >>> ---
> > >>>    drivers/virtio/virtio_ring.c | 121 ++++++++++++++++++++++++++---------
> > >>>    1 file changed, 92 insertions(+), 29 deletions(-)
> > >>>
> > >>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > >>> index a15869514146..96ff2dabda14 100644
> > >>> --- a/drivers/virtio/virtio_ring.c
> > >>> +++ b/drivers/virtio/virtio_ring.c
> > >>> @@ -85,6 +85,18 @@ struct vring_desc_extra {
> > >>>           u16 next;                       /* The next desc state in a list. */
> > >>>    };
> > >>>
> > >>> +struct vring_packed {
> > >>> + u32 num;
> > >>> + struct vring_packed_desc *ring;
> > >>> + struct vring_packed_desc_event *driver;
> > >>> + struct vring_packed_desc_event *device;
> > >>> + dma_addr_t ring_dma_addr;
> > >>> + dma_addr_t driver_event_dma_addr;
> > >>> + dma_addr_t device_event_dma_addr;
> > >>> + size_t ring_size_in_bytes;
> > >>> + size_t event_size_in_bytes;
> > >>> +};
> > >>> +
> > >>
> > >> Interesting,  a nitpick here is that if it is only used by one helper,
> > >> it's probably not worth to bother.
> > >
> > > Indeed only one helper will use it. Because vring_alloc_queue_packed will pass
> > > too many variables. If we use function parameters, I think this function is too
> > > ugly.
> > >
> > >
> > >> And if we want to do that, we need
> > >>
> > >> 1) use a separated patch
> > > If we split this patch again, we can only separate vring_free_vring_packed()
> > > out.
> > >
> > >> 2) do it for split virtqueue as well
> > > Earlier versions, I did create a struct for split too, but you replied that it
> > > wasn't worth it, so I removed the separate struct used by split.
> >
> >
> > So I think let's just stick to the current style by passing individual
> > parameters, and we can refactor it on top of this series.
> >
> > This will let us to focus on the core logic.
>
> I am implementing the function of reusing the buffer mentioned by Michael, and
> at this time there will be a lot of dependence on this structure. Because I
> want to save some information(desc, desc_extra, desc_stete), these structures
> are very useful at this time.
>
> The only problem is that this patch set is too big. ^_^

Ok, not a must, especially if Michael is fine with this.

Thanks

>
> Thanks.
>
> >
> > Thanks
> >
> >
> > >
> > > Thanks.
> > >
> > >> Thanks
> > >>
> > >>
> > >>>    struct vring_virtqueue {
> > >>>           struct virtqueue vq;
> > >>>
> > >>> @@ -1735,45 +1747,101 @@ static struct vring_desc_extra *vring_alloc_desc_extra(unsigned int num)
> > >>>           return desc_extra;
> > >>>    }
> > >>>
> > >>> -static struct virtqueue *vring_create_virtqueue_packed(
> > >>> - unsigned int index,
> > >>> - unsigned int num,
> > >>> - unsigned int vring_align,
> > >>> - struct virtio_device *vdev,
> > >>> - bool weak_barriers,
> > >>> - bool may_reduce_num,
> > >>> - bool context,
> > >>> - bool (*notify)(struct virtqueue *),
> > >>> - void (*callback)(struct virtqueue *),
> > >>> - const char *name)
> > >>> +static void vring_free_vring_packed(struct vring_packed *vring,
> > >>> +                             struct virtio_device *vdev)
> > >>> +{
> > >>> + dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
> > >>> + struct vring_packed_desc_event *driver, *device;
> > >>> + size_t ring_size_in_bytes, event_size_in_bytes;
> > >>> + struct vring_packed_desc *ring;
> > >>> +
> > >>> + ring                  = vring->ring;
> > >>> + driver                = vring->driver;
> > >>> + device                = vring->device;
> > >>> + ring_size_in_bytes    = vring->ring_size_in_bytes;
> > >>> + event_size_in_bytes   = vring->event_size_in_bytes;
> > >>> + ring_dma_addr         = vring->ring_dma_addr;
> > >>> + driver_event_dma_addr = vring->driver_event_dma_addr;
> > >>> + device_event_dma_addr = vring->device_event_dma_addr;
> > >>> +
> > >>> + if (device)
> > >>> +         vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> > >>> +
> > >>> + if (driver)
> > >>> +         vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> > >>> +
> > >>> + if (ring)
> > >>> +         vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> > >>> +}
> > >>> +
> > >>> +static int vring_alloc_queue_packed(struct vring_packed *vring,
> > >>> +                             struct virtio_device *vdev,
> > >>> +                             u32 num)
> > >>>    {
> > >>> - struct vring_virtqueue *vq;
> > >>>           struct vring_packed_desc *ring;
> > >>>           struct vring_packed_desc_event *driver, *device;
> > >>>           dma_addr_t ring_dma_addr, driver_event_dma_addr, device_event_dma_addr;
> > >>>           size_t ring_size_in_bytes, event_size_in_bytes;
> > >>>
> > >>> + memset(vring, 0, sizeof(*vring));
> > >>> +
> > >>>           ring_size_in_bytes = num * sizeof(struct vring_packed_desc);
> > >>>
> > >>>           ring = vring_alloc_queue(vdev, ring_size_in_bytes,
> > >>>                                    &ring_dma_addr,
> > >>>                                    GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> > >>>           if (!ring)
> > >>> -         goto err_ring;
> > >>> +         goto err;
> > >>> +
> > >>> + vring->num = num;
> > >>> + vring->ring = ring;
> > >>> + vring->ring_size_in_bytes = ring_size_in_bytes;
> > >>> + vring->ring_dma_addr = ring_dma_addr;
> > >>>
> > >>>           event_size_in_bytes = sizeof(struct vring_packed_desc_event);
> > >>> + vring->event_size_in_bytes = event_size_in_bytes;
> > >>>
> > >>>           driver = vring_alloc_queue(vdev, event_size_in_bytes,
> > >>>                                      &driver_event_dma_addr,
> > >>>                                      GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> > >>>           if (!driver)
> > >>> -         goto err_driver;
> > >>> +         goto err;
> > >>> +
> > >>> + vring->driver = driver;
> > >>> + vring->driver_event_dma_addr = driver_event_dma_addr;
> > >>>
> > >>>           device = vring_alloc_queue(vdev, event_size_in_bytes,
> > >>>                                      &device_event_dma_addr,
> > >>>                                      GFP_KERNEL|__GFP_NOWARN|__GFP_ZERO);
> > >>>           if (!device)
> > >>> -         goto err_device;
> > >>> +         goto err;
> > >>> +
> > >>> + vring->device = device;
> > >>> + vring->device_event_dma_addr = device_event_dma_addr;
> > >>> + return 0;
> > >>> +
> > >>> +err:
> > >>> + vring_free_vring_packed(vring, vdev);
> > >>> + return -ENOMEM;
> > >>> +}
> > >>> +
> > >>> +static struct virtqueue *vring_create_virtqueue_packed(
> > >>> + unsigned int index,
> > >>> + unsigned int num,
> > >>> + unsigned int vring_align,
> > >>> + struct virtio_device *vdev,
> > >>> + bool weak_barriers,
> > >>> + bool may_reduce_num,
> > >>> + bool context,
> > >>> + bool (*notify)(struct virtqueue *),
> > >>> + void (*callback)(struct virtqueue *),
> > >>> + const char *name)
> > >>> +{
> > >>> + struct vring_virtqueue *vq;
> > >>> + struct vring_packed vring;
> > >>> +
> > >>> + if (vring_alloc_queue_packed(&vring, vdev, num))
> > >>> +         goto err_vq;
> > >>>
> > >>>           vq = kmalloc(sizeof(*vq), GFP_KERNEL);
> > >>>           if (!vq)
> > >>> @@ -1805,17 +1873,17 @@ static struct virtqueue *vring_create_virtqueue_packed(
> > >>>           if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
> > >>>                   vq->weak_barriers = false;
> > >>>
> > >>> - vq->packed.ring_dma_addr = ring_dma_addr;
> > >>> - vq->packed.driver_event_dma_addr = driver_event_dma_addr;
> > >>> - vq->packed.device_event_dma_addr = device_event_dma_addr;
> > >>> + vq->packed.ring_dma_addr = vring.ring_dma_addr;
> > >>> + vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr;
> > >>> + vq->packed.device_event_dma_addr = vring.device_event_dma_addr;
> > >>>
> > >>> - vq->packed.ring_size_in_bytes = ring_size_in_bytes;
> > >>> - vq->packed.event_size_in_bytes = event_size_in_bytes;
> > >>> + vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
> > >>> + vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
> > >>>
> > >>>           vq->packed.vring.num = num;
> > >>> - vq->packed.vring.desc = ring;
> > >>> - vq->packed.vring.driver = driver;
> > >>> - vq->packed.vring.device = device;
> > >>> + vq->packed.vring.desc = vring.ring;
> > >>> + vq->packed.vring.driver = vring.driver;
> > >>> + vq->packed.vring.device = vring.device;
> > >>>
> > >>>           vq->packed.next_avail_idx = 0;
> > >>>           vq->packed.avail_wrap_counter = 1;
> > >>> @@ -1856,12 +1924,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
> > >>>    err_desc_state:
> > >>>           kfree(vq);
> > >>>    err_vq:
> > >>> - vring_free_queue(vdev, event_size_in_bytes, device, device_event_dma_addr);
> > >>> -err_device:
> > >>> - vring_free_queue(vdev, event_size_in_bytes, driver, driver_event_dma_addr);
> > >>> -err_driver:
> > >>> - vring_free_queue(vdev, ring_size_in_bytes, ring, ring_dma_addr);
> > >>> -err_ring:
> > >>> + vring_free_vring_packed(&vring, vdev);
> > >>>           return NULL;
> > >>>    }
> > >>>
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 12/16] virtio_ring: packed: extract the logic of alloc state and extra

[Xuan Zhuo]

Separate the logic for alloc desc_state and desc_extra, which will
be called separately by subsequent patches.

Use struct vring_packed to pass desc_state, desc_extra.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 96ff2dabda14..980bd77d8243 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -95,6 +95,9 @@ struct vring_packed {
 	dma_addr_t device_event_dma_addr;
 	size_t ring_size_in_bytes;
 	size_t event_size_in_bytes;
+
+	struct vring_desc_state_packed *desc_state;
+	struct vring_desc_extra *desc_extra;
 };
 
 struct vring_virtqueue {
@@ -1825,6 +1828,32 @@ static int vring_alloc_queue_packed(struct vring_packed *vring,
 	return -ENOMEM;
 }
 
+static int vring_alloc_state_extra_packed(u32 num, struct vring_packed *vring)
+{
+	struct vring_desc_state_packed *state;
+	struct vring_desc_extra *extra;
+
+	state = kmalloc_array(num, sizeof(struct vring_desc_state_packed), GFP_KERNEL);
+	if (!state)
+		goto err_desc_state;
+
+	memset(state, 0, num * sizeof(struct vring_desc_state_packed));
+
+	extra = vring_alloc_desc_extra(num);
+	if (!extra)
+		goto err_desc_extra;
+
+	vring->desc_state = state;
+	vring->desc_extra = extra;
+
+	return 0;
+
+err_desc_extra:
+	kfree(state);
+err_desc_state:
+	return -ENOMEM;
+}
+
 static struct virtqueue *vring_create_virtqueue_packed(
 	unsigned int index,
 	unsigned int num,
@@ -1839,6 +1868,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 {
 	struct vring_virtqueue *vq;
 	struct vring_packed vring;
+	int err;
 
 	if (vring_alloc_queue_packed(&vring, vdev, num))
 		goto err_vq;
@@ -1891,22 +1921,16 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	vq->packed.event_flags_shadow = 0;
 	vq->packed.avail_used_flags = 1 << VRING_PACKED_DESC_F_AVAIL;
 
-	vq->packed.desc_state = kmalloc_array(num,
-			sizeof(struct vring_desc_state_packed),
-			GFP_KERNEL);
-	if (!vq->packed.desc_state)
-		goto err_desc_state;
+	err = vring_alloc_state_extra_packed(num, &vring);
+	if (err)
+		goto err_state_extra;
 
-	memset(vq->packed.desc_state, 0,
-		num * sizeof(struct vring_desc_state_packed));
+	vq->packed.desc_state = vring.desc_state;
+	vq->packed.desc_extra = vring.desc_extra;
 
 	/* Put everything in free lists. */
 	vq->free_head = 0;
 
-	vq->packed.desc_extra = vring_alloc_desc_extra(num);
-	if (!vq->packed.desc_extra)
-		goto err_desc_extra;
-
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback) {
 		vq->packed.event_flags_shadow = VRING_PACKED_EVENT_FLAG_DISABLE;
@@ -1919,9 +1943,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	spin_unlock(&vdev->vqs_list_lock);
 	return &vq->vq;
 
-err_desc_extra:
-	kfree(vq->packed.desc_state);
-err_desc_state:
+err_state_extra:
 	kfree(vq);
 err_vq:
 	vring_free_vring_packed(&vring, vdev);
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 13/16] virtio_ring: packed: extract the logic of attach vring

[Xuan Zhuo]

Separate the logic of attach vring, the subsequent patch will call it
separately.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 980bd77d8243..e8d4de25e90b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1854,6 +1854,31 @@ static int vring_alloc_state_extra_packed(u32 num, struct vring_packed *vring)
 	return -ENOMEM;
 }
 
+static void vring_virtqueue_attach_packed(struct vring_virtqueue *vq,
+					  struct vring_packed *vring)
+{
+	u32 num;
+
+	num = vring->num;
+
+	vq->vq.num_free = num;
+
+	vq->packed.ring_dma_addr = vring->ring_dma_addr;
+	vq->packed.driver_event_dma_addr = vring->driver_event_dma_addr;
+	vq->packed.device_event_dma_addr = vring->device_event_dma_addr;
+
+	vq->packed.ring_size_in_bytes = vring->ring_size_in_bytes;
+	vq->packed.event_size_in_bytes = vring->event_size_in_bytes;
+
+	vq->packed.vring.num = num;
+	vq->packed.vring.desc = vring->ring;
+	vq->packed.vring.driver = vring->driver;
+	vq->packed.vring.device = vring->device;
+
+	vq->packed.desc_state = vring->desc_state;
+	vq->packed.desc_extra = vring->desc_extra;
+}
+
 static struct virtqueue *vring_create_virtqueue_packed(
 	unsigned int index,
 	unsigned int num,
@@ -1880,7 +1905,6 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	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;
 	vq->notify = notify;
@@ -1903,18 +1927,6 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
 		vq->weak_barriers = false;
 
-	vq->packed.ring_dma_addr = vring.ring_dma_addr;
-	vq->packed.driver_event_dma_addr = vring.driver_event_dma_addr;
-	vq->packed.device_event_dma_addr = vring.device_event_dma_addr;
-
-	vq->packed.ring_size_in_bytes = vring.ring_size_in_bytes;
-	vq->packed.event_size_in_bytes = vring.event_size_in_bytes;
-
-	vq->packed.vring.num = num;
-	vq->packed.vring.desc = vring.ring;
-	vq->packed.vring.driver = vring.driver;
-	vq->packed.vring.device = vring.device;
-
 	vq->packed.next_avail_idx = 0;
 	vq->packed.avail_wrap_counter = 1;
 	vq->packed.used_wrap_counter = 1;
@@ -1925,8 +1937,7 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	if (err)
 		goto err_state_extra;
 
-	vq->packed.desc_state = vring.desc_state;
-	vq->packed.desc_extra = vring.desc_extra;
+	vring_virtqueue_attach_packed(vq, &vring);
 
 	/* Put everything in free lists. */
 	vq->free_head = 0;
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 14/16] virtio_ring: packed: extract the logic of vq init

[Xuan Zhuo]

Separate the logic of initializing vq, and subsequent patches will call
it separately.

The characteristic of this part of the logic is that it does not depend
on the information passed by the upper layer, and can be called
repeatedly.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index e8d4de25e90b..73abc9e472ce 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1879,6 +1879,43 @@ static void vring_virtqueue_attach_packed(struct vring_virtqueue *vq,
 	vq->packed.desc_extra = vring->desc_extra;
 }
 
+static void vring_virtqueue_init_packed(struct vring_virtqueue *vq,
+					struct virtio_device *vdev)
+{
+	vq->we_own_ring = true;
+	vq->broken = false;
+	vq->last_used_idx = 0;
+	vq->event_triggered = false;
+	vq->num_added = 0;
+	vq->packed_ring = true;
+	vq->use_dma_api = vring_use_dma_api(vdev);
+#ifdef DEBUG
+	vq->in_use = false;
+	vq->last_add_time_valid = false;
+#endif
+
+	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
+
+	if (virtio_has_feature(vdev, VIRTIO_F_ORDER_PLATFORM))
+		vq->weak_barriers = false;
+
+	vq->packed.next_avail_idx = 0;
+	vq->packed.avail_wrap_counter = 1;
+	vq->packed.used_wrap_counter = 1;
+	vq->packed.event_flags_shadow = 0;
+	vq->packed.avail_used_flags = 1 << VRING_PACKED_DESC_F_AVAIL;
+
+	/* Put everything in free lists. */
+	vq->free_head = 0;
+
+	/* No callback?  Tell other side not to bother us. */
+	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);
+	}
+}
+
 static struct virtqueue *vring_create_virtqueue_packed(
 	unsigned int index,
 	unsigned int num,
@@ -1906,48 +1943,18 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	vq->vq.vdev = vdev;
 	vq->vq.name = name;
 	vq->vq.index = index;
-	vq->we_own_ring = true;
 	vq->notify = notify;
 	vq->weak_barriers = weak_barriers;
-	vq->broken = false;
-	vq->last_used_idx = 0;
-	vq->event_triggered = false;
-	vq->num_added = 0;
-	vq->packed_ring = true;
-	vq->use_dma_api = vring_use_dma_api(vdev);
-#ifdef DEBUG
-	vq->in_use = false;
-	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))
-		vq->weak_barriers = false;
-
-	vq->packed.next_avail_idx = 0;
-	vq->packed.avail_wrap_counter = 1;
-	vq->packed.used_wrap_counter = 1;
-	vq->packed.event_flags_shadow = 0;
-	vq->packed.avail_used_flags = 1 << VRING_PACKED_DESC_F_AVAIL;
 
 	err = vring_alloc_state_extra_packed(num, &vring);
 	if (err)
 		goto err_state_extra;
 
 	vring_virtqueue_attach_packed(vq, &vring);
-
-	/* Put everything in free lists. */
-	vq->free_head = 0;
-
-	/* No callback?  Tell other side not to bother us. */
-	if (!callback) {
-		vq->packed.event_flags_shadow = VRING_PACKED_EVENT_FLAG_DISABLE;
-		vq->packed.vring.driver->flags =
-			cpu_to_le16(vq->packed.event_flags_shadow);
-	}
+	vring_virtqueue_init_packed(vq, vdev);
 
 	spin_lock(&vdev->vqs_list_lock);
 	list_add_tail(&vq->vq.list, &vdev->vqs);
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 15/16] virtio_ring: packed: implement virtqueue_resize_packed()

[Xuan Zhuo]

virtio ring packed supports resize.

Only after the new vring is successfully allocated based on the new num,
we will release the old vring. In any case, an error is returned,
indicating that the vring still points to the old vring. In the case of
an error, we will re-initialize the state of the vring to ensure that
the vring can be used.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 73abc9e472ce..fb0abf9a2f57 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1968,6 +1968,34 @@ static struct virtqueue *vring_create_virtqueue_packed(
 	return NULL;
 }
 
+static int virtqueue_resize_packed(struct virtqueue *_vq, u32 num)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct virtio_device *vdev = _vq->vdev;
+	struct vring_packed vring;
+	int err;
+
+	err = vring_alloc_queue_packed(&vring, vdev, num);
+	if (err)
+		goto err_queue;
+
+	err = vring_alloc_state_extra_packed(num, &vring);
+	if (err)
+		goto err_state_extra;
+
+	__vring_free(&vq->vq);
+
+	vring_virtqueue_attach_packed(vq, &vring);
+	vring_virtqueue_init_packed(vq, vdev);
+	return 0;
+
+err_state_extra:
+	vring_free_vring_packed(&vring, vdev);
+err_queue:
+	vring_virtqueue_init_packed(vq, vdev);
+	return -ENOMEM;
+}
+
 
 /*
  * Generic functions and exported symbols.
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v8 16/16] virtio_ring: introduce virtqueue_resize()

[Xuan Zhuo]

Introduce virtqueue_resize() to implement the resize of vring.
Based on these, the driver can dynamically adjust the size of the vring.
For example: ethtool -G.

virtqueue_resize() implements resize based on the vq reset function. In
case of failure to allocate a new vring, it will give up resize and use
the original vring.

During this process, if the re-enable reset vq fails, the vq can no
longer be used. Although the probability of this situation is not high.

The parameter recycle is used to recycle the buffer that is no longer
used.

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

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index fb0abf9a2f57..b1dde086a8a4 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2528,6 +2528,73 @@ struct virtqueue *vring_create_virtqueue(
 }
 EXPORT_SYMBOL_GPL(vring_create_virtqueue);
 
+/**
+ * virtqueue_resize - resize the vring of vq
+ * @vq: the struct virtqueue we're talking about.
+ * @num: new ring num
+ * @recycle: callback for recycle the useless buffer
+ *
+ * When it is really necessary to create a new vring, it will set the current vq
+ * into the reset state. Then call the passed cb to recycle the buffer that is
+ * no longer used. Only after the new vring is successfully created, the old
+ * vring will be released.
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted).
+ *
+ * Returns zero or a negative error.
+ * -ENOMEM: create new vring fail. But vq can still work
+ * -EBUSY:  reset/re-enable vq fail. vq may cannot work
+ * -ENOENT: not support resize
+ * -E2BIG/-EINVAL: param num error
+ */
+int virtqueue_resize(struct virtqueue *vq, u32 num,
+		     void (*recycle)(struct virtqueue *vq, void *buf))
+{
+	struct virtio_device *vdev = vq->vdev;
+	void *buf;
+	int err;
+
+	if (num > vq->num_max)
+		return -E2BIG;
+
+	if (!num)
+		return -EINVAL;
+
+	if (to_vvq(vq)->packed.vring.num == num)
+		return 0;
+
+	if (!vq->vdev->config->reset_vq)
+		return -ENOENT;
+
+	if (!vq->vdev->config->enable_reset_vq)
+		return -ENOENT;
+
+	err = vq->vdev->config->reset_vq(vq);
+	if (err) {
+		if (err != -ENOENT)
+			err = -EBUSY;
+		return err;
+	}
+
+	while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
+		recycle(vq, buf);
+
+	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
+		err = virtqueue_resize_packed(vq, num);
+	else
+		err = virtqueue_resize_split(vq, num);
+
+	if (err)
+		err = -ENOMEM;
+
+	if (vq->vdev->config->enable_reset_vq(vq))
+		return -EBUSY;
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(virtqueue_resize);
+
 /* Only available for split ring */
 struct virtqueue *vring_new_virtqueue(unsigned int index,
 				      unsigned int num,
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index d59adc4be068..c86ff02e0ca0 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -91,6 +91,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
 dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
 dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
 
+int virtqueue_resize(struct virtqueue *vq, u32 num,
+		     void (*recycle)(struct virtqueue *vq, void *buf));
+
 /**
  * virtio_device - representation of a device using virtio
  * @index: unique position on the virtio bus
-- 
2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 16/16] virtio_ring: introduce virtqueue_resize()

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> Introduce virtqueue_resize() to implement the resize of vring.
> Based on these, the driver can dynamically adjust the size of the vring.
> For example: ethtool -G.
>
> virtqueue_resize() implements resize based on the vq reset function. In
> case of failure to allocate a new vring, it will give up resize and use
> the original vring.
>
> During this process, if the re-enable reset vq fails, the vq can no
> longer be used. Although the probability of this situation is not high.
>
> The parameter recycle is used to recycle the buffer that is no longer
> used.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   drivers/virtio/virtio_ring.c | 67 ++++++++++++++++++++++++++++++++++++
>   include/linux/virtio.h       |  3 ++
>   2 files changed, 70 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index fb0abf9a2f57..b1dde086a8a4 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2528,6 +2528,73 @@ struct virtqueue *vring_create_virtqueue(
>   }
>   EXPORT_SYMBOL_GPL(vring_create_virtqueue);
>   
> +/**
> + * virtqueue_resize - resize the vring of vq
> + * @vq: the struct virtqueue we're talking about.
> + * @num: new ring num
> + * @recycle: callback for recycle the useless buffer
> + *
> + * When it is really necessary to create a new vring, it will set the current vq
> + * into the reset state. Then call the passed cb to recycle the buffer that is
> + * no longer used. Only after the new vring is successfully created, the old
> + * vring will be released.
> + *
> + * Caller must ensure we don't call this with other virtqueue operations
> + * at the same time (except where noted).
> + *
> + * Returns zero or a negative error.
> + * -ENOMEM: create new vring fail. But vq can still work
> + * -EBUSY:  reset/re-enable vq fail. vq may cannot work
> + * -ENOENT: not support resize
> + * -E2BIG/-EINVAL: param num error
> + */
> +int virtqueue_resize(struct virtqueue *vq, u32 num,
> +		     void (*recycle)(struct virtqueue *vq, void *buf))
> +{
> +	struct virtio_device *vdev = vq->vdev;
> +	void *buf;
> +	int err;
> +
> +	if (num > vq->num_max)
> +		return -E2BIG;
> +
> +	if (!num)
> +		return -EINVAL;
> +
> +	if (to_vvq(vq)->packed.vring.num == num)
> +		return 0;


Any reason we need to check a packed specific attribute here?


> +
> +	if (!vq->vdev->config->reset_vq)
> +		return -ENOENT;
> +
> +	if (!vq->vdev->config->enable_reset_vq)
> +		return -ENOENT;
> +
> +	err = vq->vdev->config->reset_vq(vq);
> +	if (err) {
> +		if (err != -ENOENT)
> +			err = -EBUSY;
> +		return err;
> +	}
> +
> +	while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
> +		recycle(vq, buf);
> +
> +	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> +		err = virtqueue_resize_packed(vq, num);
> +	else
> +		err = virtqueue_resize_split(vq, num);
> +
> +	if (err)
> +		err = -ENOMEM;


So this assumes that the -ENOMEM is the only possible error value for 
virtqueue_resize_xxx(). Is this true? (E.g wrong size)


> +
> +	if (vq->vdev->config->enable_reset_vq(vq))
> +		return -EBUSY;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_resize);
> +
>   /* Only available for split ring */
>   struct virtqueue *vring_new_virtqueue(unsigned int index,
>   				      unsigned int num,
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index d59adc4be068..c86ff02e0ca0 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -91,6 +91,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
>   dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
>   dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
>   
> +int virtqueue_resize(struct virtqueue *vq, u32 num,
> +		     void (*recycle)(struct virtqueue *vq, void *buf));


I wonder what's the advantages of coupling virtqueue_reset in 
virtqueue_resize().

It looks to me it wold be more flexible to let the driver do:

rest()

detach()

resize()

enable_reset()

One reason is that in the future we may want to add more functionality 
e.g switching PASID during virtqueue reset.

Thanks


> +
>   /**
>    * virtio_device - representation of a device using virtio
>    * @index: unique position on the virtio bus

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 16/16] virtio_ring: introduce virtqueue_resize()

[Xuan Zhuo]

On Tue, 22 Mar 2022 14:02:47 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > Introduce virtqueue_resize() to implement the resize of vring.
> > Based on these, the driver can dynamically adjust the size of the vring.
> > For example: ethtool -G.
> >
> > virtqueue_resize() implements resize based on the vq reset function. In
> > case of failure to allocate a new vring, it will give up resize and use
> > the original vring.
> >
> > During this process, if the re-enable reset vq fails, the vq can no
> > longer be used. Although the probability of this situation is not high.
> >
> > The parameter recycle is used to recycle the buffer that is no longer
> > used.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   drivers/virtio/virtio_ring.c | 67 ++++++++++++++++++++++++++++++++++++
> >   include/linux/virtio.h       |  3 ++
> >   2 files changed, 70 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index fb0abf9a2f57..b1dde086a8a4 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -2528,6 +2528,73 @@ struct virtqueue *vring_create_virtqueue(
> >   }
> >   EXPORT_SYMBOL_GPL(vring_create_virtqueue);
> >
> > +/**
> > + * virtqueue_resize - resize the vring of vq
> > + * @vq: the struct virtqueue we're talking about.
> > + * @num: new ring num
> > + * @recycle: callback for recycle the useless buffer
> > + *
> > + * When it is really necessary to create a new vring, it will set the current vq
> > + * into the reset state. Then call the passed cb to recycle the buffer that is
> > + * no longer used. Only after the new vring is successfully created, the old
> > + * vring will be released.
> > + *
> > + * Caller must ensure we don't call this with other virtqueue operations
> > + * at the same time (except where noted).
> > + *
> > + * Returns zero or a negative error.
> > + * -ENOMEM: create new vring fail. But vq can still work
> > + * -EBUSY:  reset/re-enable vq fail. vq may cannot work
> > + * -ENOENT: not support resize
> > + * -E2BIG/-EINVAL: param num error
> > + */
> > +int virtqueue_resize(struct virtqueue *vq, u32 num,
> > +		     void (*recycle)(struct virtqueue *vq, void *buf))
> > +{
> > +	struct virtio_device *vdev = vq->vdev;
> > +	void *buf;
> > +	int err;
> > +
> > +	if (num > vq->num_max)
> > +		return -E2BIG;
> > +
> > +	if (!num)
> > +		return -EINVAL;
> > +
> > +	if (to_vvq(vq)->packed.vring.num == num)
> > +		return 0;
>
>
> Any reason we need to check a packed specific attribute here?

This is a mistake. Sorry for this.

>
>
> > +
> > +	if (!vq->vdev->config->reset_vq)
> > +		return -ENOENT;
> > +
> > +	if (!vq->vdev->config->enable_reset_vq)
> > +		return -ENOENT;
> > +
> > +	err = vq->vdev->config->reset_vq(vq);
> > +	if (err) {
> > +		if (err != -ENOENT)
> > +			err = -EBUSY;
> > +		return err;
> > +	}
> > +
> > +	while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
> > +		recycle(vq, buf);
> > +
> > +	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> > +		err = virtqueue_resize_packed(vq, num);
> > +	else
> > +		err = virtqueue_resize_split(vq, num);
> > +
> > +	if (err)
> > +		err = -ENOMEM;
>
>
> So this assumes that the -ENOMEM is the only possible error value for
> virtqueue_resize_xxx(). Is this true? (E.g wrong size)

Yes, I want the user to know at which step the error is returned.

>
>
> > +
> > +	if (vq->vdev->config->enable_reset_vq(vq))
> > +		return -EBUSY;
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(virtqueue_resize);
> > +
> >   /* Only available for split ring */
> >   struct virtqueue *vring_new_virtqueue(unsigned int index,
> >   				      unsigned int num,
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index d59adc4be068..c86ff02e0ca0 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -91,6 +91,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
> >   dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
> >   dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
> >
> > +int virtqueue_resize(struct virtqueue *vq, u32 num,
> > +		     void (*recycle)(struct virtqueue *vq, void *buf));
>
>
> I wonder what's the advantages of coupling virtqueue_reset in
> virtqueue_resize().

This is Michael's comment on the previous version

>
> It looks to me it wold be more flexible to let the driver do:
>
> rest()
>
> detach()
>
> resize()
>
> enable_reset()
>
> One reason is that in the future we may want to add more functionality
> e.g switching PASID during virtqueue reset.

Michael, from Jason Wang's plan, should we go back to the original 4 api model?

Thanks.


>
> Thanks
>
>
> > +
> >   /**
> >    * virtio_device - representation of a device using virtio
> >    * @index: unique position on the virtio bus
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 16/16] virtio_ring: introduce virtqueue_resize()

[Michael S. Tsirkin]

On Thu, Mar 24, 2022 at 04:34:56PM +0800, Xuan Zhuo wrote:
> On Tue, 22 Mar 2022 14:02:47 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > > Introduce virtqueue_resize() to implement the resize of vring.
> > > Based on these, the driver can dynamically adjust the size of the vring.
> > > For example: ethtool -G.
> > >
> > > virtqueue_resize() implements resize based on the vq reset function. In
> > > case of failure to allocate a new vring, it will give up resize and use
> > > the original vring.
> > >
> > > During this process, if the re-enable reset vq fails, the vq can no
> > > longer be used. Although the probability of this situation is not high.
> > >
> > > The parameter recycle is used to recycle the buffer that is no longer
> > > used.
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >   drivers/virtio/virtio_ring.c | 67 ++++++++++++++++++++++++++++++++++++
> > >   include/linux/virtio.h       |  3 ++
> > >   2 files changed, 70 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index fb0abf9a2f57..b1dde086a8a4 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -2528,6 +2528,73 @@ struct virtqueue *vring_create_virtqueue(
> > >   }
> > >   EXPORT_SYMBOL_GPL(vring_create_virtqueue);
> > >
> > > +/**
> > > + * virtqueue_resize - resize the vring of vq
> > > + * @vq: the struct virtqueue we're talking about.
> > > + * @num: new ring num
> > > + * @recycle: callback for recycle the useless buffer
> > > + *
> > > + * When it is really necessary to create a new vring, it will set the current vq
> > > + * into the reset state. Then call the passed cb to recycle the buffer that is
> > > + * no longer used. Only after the new vring is successfully created, the old
> > > + * vring will be released.
> > > + *
> > > + * Caller must ensure we don't call this with other virtqueue operations
> > > + * at the same time (except where noted).
> > > + *
> > > + * Returns zero or a negative error.
> > > + * -ENOMEM: create new vring fail. But vq can still work
> > > + * -EBUSY:  reset/re-enable vq fail. vq may cannot work
> > > + * -ENOENT: not support resize
> > > + * -E2BIG/-EINVAL: param num error
> > > + */
> > > +int virtqueue_resize(struct virtqueue *vq, u32 num,
> > > +		     void (*recycle)(struct virtqueue *vq, void *buf))
> > > +{
> > > +	struct virtio_device *vdev = vq->vdev;
> > > +	void *buf;
> > > +	int err;
> > > +
> > > +	if (num > vq->num_max)
> > > +		return -E2BIG;
> > > +
> > > +	if (!num)
> > > +		return -EINVAL;
> > > +
> > > +	if (to_vvq(vq)->packed.vring.num == num)
> > > +		return 0;
> >
> >
> > Any reason we need to check a packed specific attribute here?
> 
> This is a mistake. Sorry for this.
> 
> >
> >
> > > +
> > > +	if (!vq->vdev->config->reset_vq)
> > > +		return -ENOENT;
> > > +
> > > +	if (!vq->vdev->config->enable_reset_vq)
> > > +		return -ENOENT;
> > > +
> > > +	err = vq->vdev->config->reset_vq(vq);
> > > +	if (err) {
> > > +		if (err != -ENOENT)
> > > +			err = -EBUSY;
> > > +		return err;
> > > +	}
> > > +
> > > +	while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
> > > +		recycle(vq, buf);
> > > +
> > > +	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> > > +		err = virtqueue_resize_packed(vq, num);
> > > +	else
> > > +		err = virtqueue_resize_split(vq, num);
> > > +
> > > +	if (err)
> > > +		err = -ENOMEM;
> >
> >
> > So this assumes that the -ENOMEM is the only possible error value for
> > virtqueue_resize_xxx(). Is this true? (E.g wrong size)
> 
> Yes, I want the user to know at which step the error is returned.
> 
> >
> >
> > > +
> > > +	if (vq->vdev->config->enable_reset_vq(vq))
> > > +		return -EBUSY;
> > > +
> > > +	return err;
> > > +}
> > > +EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > +
> > >   /* Only available for split ring */
> > >   struct virtqueue *vring_new_virtqueue(unsigned int index,
> > >   				      unsigned int num,
> > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > index d59adc4be068..c86ff02e0ca0 100644
> > > --- a/include/linux/virtio.h
> > > +++ b/include/linux/virtio.h
> > > @@ -91,6 +91,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
> > >   dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
> > >   dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
> > >
> > > +int virtqueue_resize(struct virtqueue *vq, u32 num,
> > > +		     void (*recycle)(struct virtqueue *vq, void *buf));
> >
> >
> > I wonder what's the advantages of coupling virtqueue_reset in
> > virtqueue_resize().
> 
> This is Michael's comment on the previous version
> 
> >
> > It looks to me it wold be more flexible to let the driver do:
> >
> > rest()
> >
> > detach()
> >
> > resize()
> >
> > enable_reset()
> >
> > One reason is that in the future we may want to add more functionality
> > e.g switching PASID during virtqueue reset.
> 
> Michael, from Jason Wang's plan, should we go back to the original 4 api model?
> 
> Thanks.


Jason, I feel a single api is preferable, because the need to do reset
as part of resize is an implementation detail, I can easily
imagine virtio spec being extended with a resize interface
which does not require a queue reset.

Makes sense to you?

> 
> >
> > Thanks
> >
> >
> > > +
> > >   /**
> > >    * virtio_device - representation of a device using virtio
> > >    * @index: unique position on the virtio bus
> >

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 16/16] virtio_ring: introduce virtqueue_resize()

[Jason Wang]

On Thu, Mar 24, 2022 at 6:41 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Mar 24, 2022 at 04:34:56PM +0800, Xuan Zhuo wrote:
> > On Tue, 22 Mar 2022 14:02:47 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > > > Introduce virtqueue_resize() to implement the resize of vring.
> > > > Based on these, the driver can dynamically adjust the size of the vring.
> > > > For example: ethtool -G.
> > > >
> > > > virtqueue_resize() implements resize based on the vq reset function. In
> > > > case of failure to allocate a new vring, it will give up resize and use
> > > > the original vring.
> > > >
> > > > During this process, if the re-enable reset vq fails, the vq can no
> > > > longer be used. Although the probability of this situation is not high.
> > > >
> > > > The parameter recycle is used to recycle the buffer that is no longer
> > > > used.
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >   drivers/virtio/virtio_ring.c | 67 ++++++++++++++++++++++++++++++++++++
> > > >   include/linux/virtio.h       |  3 ++
> > > >   2 files changed, 70 insertions(+)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index fb0abf9a2f57..b1dde086a8a4 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -2528,6 +2528,73 @@ struct virtqueue *vring_create_virtqueue(
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(vring_create_virtqueue);
> > > >
> > > > +/**
> > > > + * virtqueue_resize - resize the vring of vq
> > > > + * @vq: the struct virtqueue we're talking about.
> > > > + * @num: new ring num
> > > > + * @recycle: callback for recycle the useless buffer
> > > > + *
> > > > + * When it is really necessary to create a new vring, it will set the current vq
> > > > + * into the reset state. Then call the passed cb to recycle the buffer that is
> > > > + * no longer used. Only after the new vring is successfully created, the old
> > > > + * vring will be released.
> > > > + *
> > > > + * Caller must ensure we don't call this with other virtqueue operations
> > > > + * at the same time (except where noted).
> > > > + *
> > > > + * Returns zero or a negative error.
> > > > + * -ENOMEM: create new vring fail. But vq can still work
> > > > + * -EBUSY:  reset/re-enable vq fail. vq may cannot work
> > > > + * -ENOENT: not support resize
> > > > + * -E2BIG/-EINVAL: param num error
> > > > + */
> > > > +int virtqueue_resize(struct virtqueue *vq, u32 num,
> > > > +              void (*recycle)(struct virtqueue *vq, void *buf))
> > > > +{
> > > > + struct virtio_device *vdev = vq->vdev;
> > > > + void *buf;
> > > > + int err;
> > > > +
> > > > + if (num > vq->num_max)
> > > > +         return -E2BIG;
> > > > +
> > > > + if (!num)
> > > > +         return -EINVAL;
> > > > +
> > > > + if (to_vvq(vq)->packed.vring.num == num)
> > > > +         return 0;
> > >
> > >
> > > Any reason we need to check a packed specific attribute here?
> >
> > This is a mistake. Sorry for this.
> >
> > >
> > >
> > > > +
> > > > + if (!vq->vdev->config->reset_vq)
> > > > +         return -ENOENT;
> > > > +
> > > > + if (!vq->vdev->config->enable_reset_vq)
> > > > +         return -ENOENT;
> > > > +
> > > > + err = vq->vdev->config->reset_vq(vq);
> > > > + if (err) {
> > > > +         if (err != -ENOENT)
> > > > +                 err = -EBUSY;
> > > > +         return err;
> > > > + }
> > > > +
> > > > + while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
> > > > +         recycle(vq, buf);
> > > > +
> > > > + if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> > > > +         err = virtqueue_resize_packed(vq, num);
> > > > + else
> > > > +         err = virtqueue_resize_split(vq, num);
> > > > +
> > > > + if (err)
> > > > +         err = -ENOMEM;
> > >
> > >
> > > So this assumes that the -ENOMEM is the only possible error value for
> > > virtqueue_resize_xxx(). Is this true? (E.g wrong size)
> >
> > Yes, I want the user to know at which step the error is returned.
> >
> > >
> > >
> > > > +
> > > > + if (vq->vdev->config->enable_reset_vq(vq))
> > > > +         return -EBUSY;
> > > > +
> > > > + return err;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > > +
> > > >   /* Only available for split ring */
> > > >   struct virtqueue *vring_new_virtqueue(unsigned int index,
> > > >                                         unsigned int num,
> > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > > index d59adc4be068..c86ff02e0ca0 100644
> > > > --- a/include/linux/virtio.h
> > > > +++ b/include/linux/virtio.h
> > > > @@ -91,6 +91,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
> > > >   dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
> > > >   dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
> > > >
> > > > +int virtqueue_resize(struct virtqueue *vq, u32 num,
> > > > +              void (*recycle)(struct virtqueue *vq, void *buf));
> > >
> > >
> > > I wonder what's the advantages of coupling virtqueue_reset in
> > > virtqueue_resize().
> >
> > This is Michael's comment on the previous version
> >
> > >
> > > It looks to me it wold be more flexible to let the driver do:
> > >
> > > rest()
> > >
> > > detach()
> > >
> > > resize()
> > >
> > > enable_reset()
> > >
> > > One reason is that in the future we may want to add more functionality
> > > e.g switching PASID during virtqueue reset.
> >
> > Michael, from Jason Wang's plan, should we go back to the original 4 api model?
> >
> > Thanks.
>
>
> Jason, I feel a single api is preferable, because the need to do reset
> as part of resize is an implementation detail, I can easily
> imagine virtio spec being extended with a resize interface
> which does not require a queue reset.
>
> Makes sense to you?

Yes. I'm fine with the single API then.

Thanks

>
> >
> > >
> > > Thanks
> > >
> > >
> > > > +
> > > >   /**
> > > >    * virtio_device - representation of a device using virtio
> > > >    * @index: unique position on the virtio bus
> > >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 16/16] virtio_ring: introduce virtqueue_resize()

[Michael S. Tsirkin]

On Mon, Mar 14, 2022 at 05:34:55PM +0800, Xuan Zhuo wrote:
> Introduce virtqueue_resize() to implement the resize of vring.
> Based on these, the driver can dynamically adjust the size of the vring.
> For example: ethtool -G.
> 
> virtqueue_resize() implements resize based on the vq reset function. In
> case of failure to allocate a new vring, it will give up resize and use
> the original vring.
> 
> During this process, if the re-enable reset vq fails, the vq can no
> longer be used. Although the probability of this situation is not high.
> 
> The parameter recycle is used to recycle the buffer that is no longer
> used.
> 
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/virtio/virtio_ring.c | 67 ++++++++++++++++++++++++++++++++++++
>  include/linux/virtio.h       |  3 ++
>  2 files changed, 70 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index fb0abf9a2f57..b1dde086a8a4 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2528,6 +2528,73 @@ struct virtqueue *vring_create_virtqueue(
>  }
>  EXPORT_SYMBOL_GPL(vring_create_virtqueue);
>  
> +/**
> + * virtqueue_resize - resize the vring of vq
> + * @vq: the struct virtqueue we're talking about.
> + * @num: new ring num
> + * @recycle: callback for recycle the useless buffer
> + *
> + * When it is really necessary to create a new vring, it will set the current vq
> + * into the reset state. Then call the passed cb to recycle the buffer that is
> + * no longer used. Only after the new vring is successfully created, the old
> + * vring will be released.
> + *
> + * Caller must ensure we don't call this with other virtqueue operations
> + * at the same time (except where noted).
> + *
> + * Returns zero or a negative error.
> + * -ENOMEM: create new vring fail. But vq can still work
> + * -EBUSY:  reset/re-enable vq fail. vq may cannot work
> + * -ENOENT: not support resize
> + * -E2BIG/-EINVAL: param num error
> + */
> +int virtqueue_resize(struct virtqueue *vq, u32 num,
> +		     void (*recycle)(struct virtqueue *vq, void *buf))
> +{
> +	struct virtio_device *vdev = vq->vdev;
> +	void *buf;
> +	int err;
> +
> +	if (num > vq->num_max)
> +		return -E2BIG;
> +
> +	if (!num)
> +		return -EINVAL;
> +
> +	if (to_vvq(vq)->packed.vring.num == num)
> +		return 0;
> +
> +	if (!vq->vdev->config->reset_vq)
> +		return -ENOENT;
> +
> +	if (!vq->vdev->config->enable_reset_vq)
> +		return -ENOENT;
> +
> +	err = vq->vdev->config->reset_vq(vq);
> +	if (err) {
> +		if (err != -ENOENT)
> +			err = -EBUSY;
> +		return err;
> +	}
> +
> +	while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
> +		recycle(vq, buf);


So all this callback can do now is drop all buffers, and I think that is
not great.  Can we store them and invoke the callback after queue is
enabled?


> +
> +	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> +		err = virtqueue_resize_packed(vq, num);
> +	else
> +		err = virtqueue_resize_split(vq, num);
> +
> +	if (err)
> +		err = -ENOMEM;
> +
> +	if (vq->vdev->config->enable_reset_vq(vq))
> +		return -EBUSY;
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_resize);
> +
>  /* Only available for split ring */
>  struct virtqueue *vring_new_virtqueue(unsigned int index,
>  				      unsigned int num,
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index d59adc4be068..c86ff02e0ca0 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -91,6 +91,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
>  dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
>  dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
>  
> +int virtqueue_resize(struct virtqueue *vq, u32 num,
> +		     void (*recycle)(struct virtqueue *vq, void *buf));
> +
>  /**
>   * virtio_device - representation of a device using virtio
>   * @index: unique position on the virtio bus
> -- 
> 2.31.0

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 16/16] virtio_ring: introduce virtqueue_resize()

[Xuan Zhuo]

On Fri, 25 Mar 2022 06:33:19 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Mar 14, 2022 at 05:34:55PM +0800, Xuan Zhuo wrote:
> > Introduce virtqueue_resize() to implement the resize of vring.
> > Based on these, the driver can dynamically adjust the size of the vring.
> > For example: ethtool -G.
> >
> > virtqueue_resize() implements resize based on the vq reset function. In
> > case of failure to allocate a new vring, it will give up resize and use
> > the original vring.
> >
> > During this process, if the re-enable reset vq fails, the vq can no
> > longer be used. Although the probability of this situation is not high.
> >
> > The parameter recycle is used to recycle the buffer that is no longer
> > used.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/virtio/virtio_ring.c | 67 ++++++++++++++++++++++++++++++++++++
> >  include/linux/virtio.h       |  3 ++
> >  2 files changed, 70 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index fb0abf9a2f57..b1dde086a8a4 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -2528,6 +2528,73 @@ struct virtqueue *vring_create_virtqueue(
> >  }
> >  EXPORT_SYMBOL_GPL(vring_create_virtqueue);
> >
> > +/**
> > + * virtqueue_resize - resize the vring of vq
> > + * @vq: the struct virtqueue we're talking about.
> > + * @num: new ring num
> > + * @recycle: callback for recycle the useless buffer
> > + *
> > + * When it is really necessary to create a new vring, it will set the current vq
> > + * into the reset state. Then call the passed cb to recycle the buffer that is
> > + * no longer used. Only after the new vring is successfully created, the old
> > + * vring will be released.
> > + *
> > + * Caller must ensure we don't call this with other virtqueue operations
> > + * at the same time (except where noted).
> > + *
> > + * Returns zero or a negative error.
> > + * -ENOMEM: create new vring fail. But vq can still work
> > + * -EBUSY:  reset/re-enable vq fail. vq may cannot work
> > + * -ENOENT: not support resize
> > + * -E2BIG/-EINVAL: param num error
> > + */
> > +int virtqueue_resize(struct virtqueue *vq, u32 num,
> > +		     void (*recycle)(struct virtqueue *vq, void *buf))
> > +{
> > +	struct virtio_device *vdev = vq->vdev;
> > +	void *buf;
> > +	int err;
> > +
> > +	if (num > vq->num_max)
> > +		return -E2BIG;
> > +
> > +	if (!num)
> > +		return -EINVAL;
> > +
> > +	if (to_vvq(vq)->packed.vring.num == num)
> > +		return 0;
> > +
> > +	if (!vq->vdev->config->reset_vq)
> > +		return -ENOENT;
> > +
> > +	if (!vq->vdev->config->enable_reset_vq)
> > +		return -ENOENT;
> > +
> > +	err = vq->vdev->config->reset_vq(vq);
> > +	if (err) {
> > +		if (err != -ENOENT)
> > +			err = -EBUSY;
> > +		return err;
> > +	}
> > +
> > +	while ((buf = virtqueue_detach_unused_buf(vq)) != NULL)
> > +		recycle(vq, buf);
>
>
> So all this callback can do now is drop all buffers, and I think that is
> not great.  Can we store them and invoke the callback after queue is
> enabled?


OK, I will bring this feature in the next version.

Thanks.

>
>
> > +
> > +	if (virtio_has_feature(vdev, VIRTIO_F_RING_PACKED))
> > +		err = virtqueue_resize_packed(vq, num);
> > +	else
> > +		err = virtqueue_resize_split(vq, num);
> > +
> > +	if (err)
> > +		err = -ENOMEM;
> > +
> > +	if (vq->vdev->config->enable_reset_vq(vq))
> > +		return -EBUSY;
> > +
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(virtqueue_resize);
> > +
> >  /* Only available for split ring */
> >  struct virtqueue *vring_new_virtqueue(unsigned int index,
> >  				      unsigned int num,
> > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > index d59adc4be068..c86ff02e0ca0 100644
> > --- a/include/linux/virtio.h
> > +++ b/include/linux/virtio.h
> > @@ -91,6 +91,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *vq);
> >  dma_addr_t virtqueue_get_avail_addr(struct virtqueue *vq);
> >  dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
> >
> > +int virtqueue_resize(struct virtqueue *vq, u32 num,
> > +		     void (*recycle)(struct virtqueue *vq, void *buf));
> > +
> >  /**
> >   * virtio_device - representation of a device using virtio
> >   * @index: unique position on the virtio bus
> > --
> > 2.31.0
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 00/16] virtio pci support VIRTIO_F_RING_RESET (refactor vring)

[Jason Wang]

在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> The virtio spec already supports the virtio queue reset function. This patch set
> is to add this function to the kernel. The relevant virtio spec information is
> here:
>
>      https://github.com/oasis-tcs/virtio-spec/issues/124
>
> Also regarding MMIO support for queue reset, I plan to support it after this
> patch is passed.
>
> This patch set implements the refactoring of vring. Finally, the
> virtuque_resize() interface is provided based on the reset function of the
> transport layer.
>
> Test environment:
>      Host: 4.19.91
>      Qemu: QEMU emulator version 6.2.50 (with vq reset support)


It would be better if you can post Qemu code for review as well.


>      Test Cmd:  ethtool -G eth1 rx $1 tx $2; ethtool -g eth1
>
>      The default is split mode, modify Qemu virtio-net to add PACKED feature to test
>      packed mode.
>
> Please review. Thanks.


Looks good overall, see comments in individual patch.

I think the code is structured in a way that is not friendly to the 
reviewers. For next version, we can bring back the ethtool -G for 
virtio-net.

Thanks


>
> v8:
>    1. Provide a virtqueue_reset() interface directly
>    2. Split the two patch sets, this is the first part
>    3. Add independent allocation helper for allocating state, extra
>
> v7:
>    1. fix #6 subject typo
>    2. fix #6 ring_size_in_bytes is uninitialized
>    3. check by: make W=12
>
> v6:
>    1. virtio_pci: use synchronize_irq(irq) to sync the irq callbacks
>    2. Introduce virtqueue_reset_vring() to implement the reset of vring during
>       the reset process. May use the old vring if num of the vq not change.
>    3. find_vqs() support sizes to special the max size of each vq
>
> v5:
>    1. add virtio-net support set_ringparam
>
> 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 (16):
>    virtio: add helper virtqueue_get_vring_max_size()
>    virtio: struct virtio_config_ops add callbacks for queue_reset
>    virtio_ring: update the document of the virtqueue_detach_unused_buf
>      for queue reset
>    virtio_ring: remove the arg vq of vring_alloc_desc_extra()
>    virtio_ring: extract the logic of freeing vring
>    virtio_ring: split: extract the logic of alloc queue
>    virtio_ring: split: extract the logic of alloc state and extra
>    virtio_ring: split: extract the logic of attach vring
>    virtio_ring: split: extract the logic of vq init
>    virtio_ring: split: implement virtqueue_resize_split()
>    virtio_ring: packed: extract the logic of alloc queue
>    virtio_ring: packed: extract the logic of alloc state and extra
>    virtio_ring: packed: extract the logic of attach vring
>    virtio_ring: packed: extract the logic of vq init
>    virtio_ring: packed: implement virtqueue_resize_packed()
>    virtio_ring: introduce virtqueue_resize()
>
>   drivers/virtio/virtio_mmio.c       |   2 +
>   drivers/virtio/virtio_pci_legacy.c |   2 +
>   drivers/virtio/virtio_pci_modern.c |   2 +
>   drivers/virtio/virtio_ring.c       | 604 ++++++++++++++++++++++-------
>   include/linux/virtio.h             |   5 +
>   include/linux/virtio_config.h      |  12 +
>   6 files changed, 494 insertions(+), 133 deletions(-)
>
> --
> 2.31.0
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v8 00/16] virtio pci support VIRTIO_F_RING_RESET (refactor vring)

[Xuan Zhuo]

On Tue, 22 Mar 2022 14:40:51 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/3/14 下午5:34, Xuan Zhuo 写道:
> > The virtio spec already supports the virtio queue reset function. This patch set
> > is to add this function to the kernel. The relevant virtio spec information is
> > here:
> >
> >      https://github.com/oasis-tcs/virtio-spec/issues/124
> >
> > Also regarding MMIO support for queue reset, I plan to support it after this
> > patch is passed.
> >
> > This patch set implements the refactoring of vring. Finally, the
> > virtuque_resize() interface is provided based on the reset function of the
> > transport layer.
> >
> > Test environment:
> >      Host: 4.19.91
> >      Qemu: QEMU emulator version 6.2.50 (with vq reset support)
>
>
> It would be better if you can post Qemu code for review as well.

OK.

>
>
> >      Test Cmd:  ethtool -G eth1 rx $1 tx $2; ethtool -g eth1
> >
> >      The default is split mode, modify Qemu virtio-net to add PACKED feature to test
> >      packed mode.
> >
> > Please review. Thanks.
>
>
> Looks good overall, see comments in individual patch.
>
> I think the code is structured in a way that is not friendly to the
> reviewers. For next version, we can bring back the ethtool -G for
> virtio-net.

OK.

Thanks.


>
> Thanks
>
>
> >
> > v8:
> >    1. Provide a virtqueue_reset() interface directly
> >    2. Split the two patch sets, this is the first part
> >    3. Add independent allocation helper for allocating state, extra
> >
> > v7:
> >    1. fix #6 subject typo
> >    2. fix #6 ring_size_in_bytes is uninitialized
> >    3. check by: make W=12
> >
> > v6:
> >    1. virtio_pci: use synchronize_irq(irq) to sync the irq callbacks
> >    2. Introduce virtqueue_reset_vring() to implement the reset of vring during
> >       the reset process. May use the old vring if num of the vq not change.
> >    3. find_vqs() support sizes to special the max size of each vq
> >
> > v5:
> >    1. add virtio-net support set_ringparam
> >
> > 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 (16):
> >    virtio: add helper virtqueue_get_vring_max_size()
> >    virtio: struct virtio_config_ops add callbacks for queue_reset
> >    virtio_ring: update the document of the virtqueue_detach_unused_buf
> >      for queue reset
> >    virtio_ring: remove the arg vq of vring_alloc_desc_extra()
> >    virtio_ring: extract the logic of freeing vring
> >    virtio_ring: split: extract the logic of alloc queue
> >    virtio_ring: split: extract the logic of alloc state and extra
> >    virtio_ring: split: extract the logic of attach vring
> >    virtio_ring: split: extract the logic of vq init
> >    virtio_ring: split: implement virtqueue_resize_split()
> >    virtio_ring: packed: extract the logic of alloc queue
> >    virtio_ring: packed: extract the logic of alloc state and extra
> >    virtio_ring: packed: extract the logic of attach vring
> >    virtio_ring: packed: extract the logic of vq init
> >    virtio_ring: packed: implement virtqueue_resize_packed()
> >    virtio_ring: introduce virtqueue_resize()
> >
> >   drivers/virtio/virtio_mmio.c       |   2 +
> >   drivers/virtio/virtio_pci_legacy.c |   2 +
> >   drivers/virtio/virtio_pci_modern.c |   2 +
> >   drivers/virtio/virtio_ring.c       | 604 ++++++++++++++++++++++-------
> >   include/linux/virtio.h             |   5 +
> >   include/linux/virtio_config.h      |  12 +
> >   6 files changed, 494 insertions(+), 133 deletions(-)
> >
> > --
> > 2.31.0
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization