[PATCH v2 0/3] virtio_ring: Clean up code for virtio ring and pci

[PATCH v2 0/3] virtio_ring: Clean up code for virtio ring and pci

[Feng Liu via Virtualization]

This patch series performs a clean up of the code in virtio_ring and
virtio_pci, modifying it to conform with the Linux kernel coding style
guidance [1]. The modifications ensure the code easy to read and
understand. This small series does few short cleanups in the code.

Patch-1 Allow non power of 2 sizes for packed virtqueues.
Patch-2 Avoid using inline for small functions.
Patch-3 Use const to annotate read-only pointer params.

[1]
https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease

All of the patches have been verified based on the kernel code
commit 81ff855485a3 ("Merge tag 'i2c-for-6.3-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux")

Feng Liu (3):
  virtio_ring: Allow non power of 2 sizes for packed virtqueue
  virtio_ring: Avoid using inline for small functions
  virtio_ring: Use const to annotate read-only pointer params

 drivers/virtio/virtio_pci_modern.c |  5 ----
 drivers/virtio/virtio_ring.c       | 48 +++++++++++++++---------------
 include/linux/virtio.h             | 14 ++++-----
 3 files changed, 31 insertions(+), 36 deletions(-)

-- 
2.34.1

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

[PATCH v2 1/3] virtio_ring: Allow non power of 2 sizes for packed virtqueue

[Feng Liu via Virtualization]

According to the Virtio Specification, the Queue Size parameter of a
virtqueue corresponds to the maximum number of descriptors in that
queue, and it does not have to be a power of 2 for packed virtqueues.
However, the virtio_pci_modern driver enforced a power of 2 check for
virtqueue sizes, which is unnecessary and restrictive for packed
virtuqueue.

Split virtqueue still needs to check the virtqueue size is power_of_2
which has been done in vring_alloc_queue_split of the virtio_ring layer.

To validate this change, we tested various virtqueue sizes for packed
rings, including 128, 256, 512, 100, 200, 500, and 1000, with
CONFIG_PAGE_POISONING enabled, and all tests passed successfully.

Signed-off-by: Feng Liu <feliu@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>

---
v0 -> v1
feedbacks from Jason Wang and Michael S. Tsirkin
- remove power_of_2 check of virtqueue size

v1 -> v2
feedbacks from Parav Pandit and Jiri Pirko
- keep power_of_2 check of split virtqueue in vring_alloc_queue_split of
  virtio_ring layer.
---
 drivers/virtio/virtio_pci_modern.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 9e496e288cfa..6e713904d8e8 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -310,11 +310,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	if (!num || vp_modern_get_queue_enable(mdev, index))
 		return ERR_PTR(-ENOENT);
 
-	if (!is_power_of_2(num)) {
-		dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
-		return ERR_PTR(-EINVAL);
-	}
-
 	info->msix_vector = msix_vec;
 
 	/* create the vring */
-- 
2.34.1

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

[PATCH v2 2/3] virtio_ring: Avoid using inline for small functions

[Feng Liu via Virtualization]

Remove the inline keyword, according to kernel coding style [1], defining
inline functions is not necessary for samll functions.

It is verified with GCC 12.2.0, the generated code with/without inline
is the same. Additionally tested with kernel pktgen and iperf, and
verified the result, pps of the results are the same in the cases of
with/without inline.

[1]
https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease

Signed-off-by: Feng Liu <feliu@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/virtio/virtio_ring.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 41144b5246a8..a26fab91c59f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -233,8 +233,8 @@ static void vring_free(struct virtqueue *_vq);
 
 #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
 
-static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
-					  unsigned int total_sg)
+static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
+				   unsigned int total_sg)
 {
 	/*
 	 * If the host supports indirect descriptor tables, and we have multiple
@@ -349,7 +349,7 @@ static void vring_free_queue(struct virtio_device *vdev, size_t size,
  * making all of the arch DMA ops work on the vring device itself
  * is a mess.
  */
-static inline struct device *vring_dma_dev(const struct vring_virtqueue *vq)
+static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
 {
 	return vq->dma_dev;
 }
@@ -784,7 +784,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
 	}
 }
 
-static inline bool more_used_split(const struct vring_virtqueue *vq)
+static bool more_used_split(const struct vring_virtqueue *vq)
 {
 	return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev,
 			vq->split.vring.used->idx);
@@ -1172,12 +1172,12 @@ static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
 /*
  * Packed ring specific functions - *_packed().
  */
-static inline bool packed_used_wrap_counter(u16 last_used_idx)
+static bool packed_used_wrap_counter(u16 last_used_idx)
 {
 	return !!(last_used_idx & (1 << VRING_PACKED_EVENT_F_WRAP_CTR));
 }
 
-static inline u16 packed_last_used(u16 last_used_idx)
+static u16 packed_last_used(u16 last_used_idx)
 {
 	return last_used_idx & ~(-(1 << VRING_PACKED_EVENT_F_WRAP_CTR));
 }
@@ -1612,7 +1612,7 @@ static inline bool is_used_desc_packed(const struct vring_virtqueue *vq,
 	return avail == used && used == used_wrap_counter;
 }
 
-static inline bool more_used_packed(const struct vring_virtqueue *vq)
+static bool more_used_packed(const struct vring_virtqueue *vq)
 {
 	u16 last_used;
 	u16 last_used_idx;
-- 
2.34.1

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

[PATCH v2 3/3] virtio_ring: Use const to annotate read-only pointer params

[Feng Liu via Virtualization]

Add const to make the read-only pointer parameters clear, similar to
many existing functions.

Use `container_of_const` to implement `to_vvq`, which ensures the
const-ness of read-only parameters and avoids accidental modification
of their members.

Signed-off-by: Feng Liu <feliu@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>

---
v0 -> v1
feedbacks from Michael S. Tsirkin
- use `container_of_const` to implement `to_vvq`
---
 drivers/virtio/virtio_ring.c | 36 ++++++++++++++++++------------------
 include/linux/virtio.h       | 14 +++++++-------
 2 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a26fab91c59f..4c3bb0ddeb9b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
  * Helpers.
  */
 
-#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
+#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
 
-static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
+static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
 				   unsigned int total_sg)
 {
 	/*
@@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
  * unconditionally on data path.
  */
 
-static bool vring_use_dma_api(struct virtio_device *vdev)
+static bool vring_use_dma_api(const struct virtio_device *vdev)
 {
 	if (!virtio_has_dma_quirk(vdev))
 		return true;
@@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
 	return false;
 }
 
-size_t virtio_max_dma_size(struct virtio_device *vdev)
+size_t virtio_max_dma_size(const struct virtio_device *vdev)
 {
 	size_t max_segment_size = SIZE_MAX;
 
@@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
  */
 
 static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
-					   struct vring_desc *desc)
+					   const struct vring_desc *desc)
 {
 	u16 flags;
 
@@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
 }
 
 static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
-				     struct vring_desc_extra *extra)
+				     const struct vring_desc_extra *extra)
 {
 	u16 flags;
 
@@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
 }
 
 static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
-				   struct vring_packed_desc *desc)
+				    const struct vring_packed_desc *desc)
 {
 	u16 flags;
 
@@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
  * Returns the size of the vring.  This is mainly used for boasting to
  * userspace.  Unlike other operations, this need not be serialized.
  */
-unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
+unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
 {
 
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	return vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
 }
@@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
 
-bool virtqueue_is_broken(struct virtqueue *_vq)
+bool virtqueue_is_broken(const struct virtqueue *_vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	return READ_ONCE(vq->broken);
 }
@@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device *dev)
 }
 EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
 
-dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *_vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	BUG_ON(!vq->we_own_ring);
 
@@ -2881,9 +2881,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
 
-dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	BUG_ON(!vq->we_own_ring);
 
@@ -2895,9 +2895,9 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr);
 
-dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
+dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq)
 {
-	struct vring_virtqueue *vq = to_vvq(_vq);
+	const struct vring_virtqueue *vq = to_vvq(_vq);
 
 	BUG_ON(!vq->we_own_ring);
 
@@ -2910,7 +2910,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
 EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
 
 /* Only available for split ring */
-const struct vring *virtqueue_get_vring(struct virtqueue *vq)
+const struct vring *virtqueue_get_vring(const struct virtqueue *vq)
 {
 	return &to_vvq(vq)->split.vring;
 }
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 2b472514c49b..c4225653f949 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
 
 void *virtqueue_detach_unused_buf(struct virtqueue *vq);
 
-unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
+unsigned int virtqueue_get_vring_size(const struct virtqueue *vq);
 
-bool virtqueue_is_broken(struct virtqueue *vq);
+bool virtqueue_is_broken(const struct virtqueue *vq);
 
-const struct vring *virtqueue_get_vring(struct virtqueue *vq);
-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);
+const struct vring *virtqueue_get_vring(const struct virtqueue *vq);
+dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq);
+dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq);
+dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq);
 
 int virtqueue_resize(struct virtqueue *vq, u32 num,
 		     void (*recycle)(struct virtqueue *vq, void *buf));
@@ -147,7 +147,7 @@ int virtio_device_restore(struct virtio_device *dev);
 #endif
 void virtio_reset_device(struct virtio_device *dev);
 
-size_t virtio_max_dma_size(struct virtio_device *vdev);
+size_t virtio_max_dma_size(const struct virtio_device *vdev);
 
 #define virtio_device_for_each_vq(vdev, vq) \
 	list_for_each_entry(vq, &vdev->vqs, list)
-- 
2.34.1

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

Re: [PATCH v2 1/3] virtio_ring: Allow non power of 2 sizes for packed virtqueue

[Jason Wang]

On Thu, Mar 16, 2023 at 2:55 AM Feng Liu <feliu@nvidia.com> wrote:
>
> According to the Virtio Specification, the Queue Size parameter of a
> virtqueue corresponds to the maximum number of descriptors in that
> queue, and it does not have to be a power of 2 for packed virtqueues.
> However, the virtio_pci_modern driver enforced a power of 2 check for
> virtqueue sizes, which is unnecessary and restrictive for packed
> virtuqueue.
>
> Split virtqueue still needs to check the virtqueue size is power_of_2
> which has been done in vring_alloc_queue_split of the virtio_ring layer.
>
> To validate this change, we tested various virtqueue sizes for packed
> rings, including 128, 256, 512, 100, 200, 500, and 1000, with
> CONFIG_PAGE_POISONING enabled, and all tests passed successfully.
>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

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

Thanks

>
> ---
> v0 -> v1
> feedbacks from Jason Wang and Michael S. Tsirkin
> - remove power_of_2 check of virtqueue size
>
> v1 -> v2
> feedbacks from Parav Pandit and Jiri Pirko
> - keep power_of_2 check of split virtqueue in vring_alloc_queue_split of
>   virtio_ring layer.
> ---
>  drivers/virtio/virtio_pci_modern.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 9e496e288cfa..6e713904d8e8 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -310,11 +310,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>         if (!num || vp_modern_get_queue_enable(mdev, index))
>                 return ERR_PTR(-ENOENT);
>
> -       if (!is_power_of_2(num)) {
> -               dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
> -               return ERR_PTR(-EINVAL);
> -       }
> -
>         info->msix_vector = msix_vec;
>
>         /* create the vring */
> --
> 2.34.1
>

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

Re: [PATCH v2 2/3] virtio_ring: Avoid using inline for small functions

[Jason Wang]

On Thu, Mar 16, 2023 at 2:55 AM Feng Liu <feliu@nvidia.com> wrote:
>
> Remove the inline keyword, according to kernel coding style [1], defining
> inline functions is not necessary for samll functions.
>
> It is verified with GCC 12.2.0, the generated code with/without inline
> is the same. Additionally tested with kernel pktgen and iperf, and
> verified the result, pps of the results are the same in the cases of
> with/without inline.
>
> [1]
> https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease
>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

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

Thanks

> ---
>  drivers/virtio/virtio_ring.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 41144b5246a8..a26fab91c59f 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -233,8 +233,8 @@ static void vring_free(struct virtqueue *_vq);
>
>  #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
>
> -static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> -                                         unsigned int total_sg)
> +static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> +                                  unsigned int total_sg)
>  {
>         /*
>          * If the host supports indirect descriptor tables, and we have multiple
> @@ -349,7 +349,7 @@ static void vring_free_queue(struct virtio_device *vdev, size_t size,
>   * making all of the arch DMA ops work on the vring device itself
>   * is a mess.
>   */
> -static inline struct device *vring_dma_dev(const struct vring_virtqueue *vq)
> +static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
>  {
>         return vq->dma_dev;
>  }
> @@ -784,7 +784,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>         }
>  }
>
> -static inline bool more_used_split(const struct vring_virtqueue *vq)
> +static bool more_used_split(const struct vring_virtqueue *vq)
>  {
>         return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev,
>                         vq->split.vring.used->idx);
> @@ -1172,12 +1172,12 @@ static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
>  /*
>   * Packed ring specific functions - *_packed().
>   */
> -static inline bool packed_used_wrap_counter(u16 last_used_idx)
> +static bool packed_used_wrap_counter(u16 last_used_idx)
>  {
>         return !!(last_used_idx & (1 << VRING_PACKED_EVENT_F_WRAP_CTR));
>  }
>
> -static inline u16 packed_last_used(u16 last_used_idx)
> +static u16 packed_last_used(u16 last_used_idx)
>  {
>         return last_used_idx & ~(-(1 << VRING_PACKED_EVENT_F_WRAP_CTR));
>  }
> @@ -1612,7 +1612,7 @@ static inline bool is_used_desc_packed(const struct vring_virtqueue *vq,
>         return avail == used && used == used_wrap_counter;
>  }
>
> -static inline bool more_used_packed(const struct vring_virtqueue *vq)
> +static bool more_used_packed(const struct vring_virtqueue *vq)
>  {
>         u16 last_used;
>         u16 last_used_idx;
> --
> 2.34.1
>

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

Re: [PATCH v2 3/3] virtio_ring: Use const to annotate read-only pointer params

[Jason Wang]

On Thu, Mar 16, 2023 at 2:55 AM Feng Liu <feliu@nvidia.com> wrote:
>
> Add const to make the read-only pointer parameters clear, similar to
> many existing functions.
>
> Use `container_of_const` to implement `to_vvq`, which ensures the
> const-ness of read-only parameters and avoids accidental modification
> of their members.
>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

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

Thanks

>
> ---
> v0 -> v1
> feedbacks from Michael S. Tsirkin
> - use `container_of_const` to implement `to_vvq`
> ---
>  drivers/virtio/virtio_ring.c | 36 ++++++++++++++++++------------------
>  include/linux/virtio.h       | 14 +++++++-------
>  2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index a26fab91c59f..4c3bb0ddeb9b 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
>   * Helpers.
>   */
>
> -#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> +#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
>
> -static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> +static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
>                                    unsigned int total_sg)
>  {
>         /*
> @@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>   * unconditionally on data path.
>   */
>
> -static bool vring_use_dma_api(struct virtio_device *vdev)
> +static bool vring_use_dma_api(const struct virtio_device *vdev)
>  {
>         if (!virtio_has_dma_quirk(vdev))
>                 return true;
> @@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>         return false;
>  }
>
> -size_t virtio_max_dma_size(struct virtio_device *vdev)
> +size_t virtio_max_dma_size(const struct virtio_device *vdev)
>  {
>         size_t max_segment_size = SIZE_MAX;
>
> @@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
>   */
>
>  static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> -                                          struct vring_desc *desc)
> +                                          const struct vring_desc *desc)
>  {
>         u16 flags;
>
> @@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
>  }
>
>  static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> -                                    struct vring_desc_extra *extra)
> +                                    const struct vring_desc_extra *extra)
>  {
>         u16 flags;
>
> @@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
>  }
>
>  static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
> -                                  struct vring_packed_desc *desc)
> +                                   const struct vring_packed_desc *desc)
>  {
>         u16 flags;
>
> @@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
>   * Returns the size of the vring.  This is mainly used for boasting to
>   * userspace.  Unlike other operations, this need not be serialized.
>   */
> -unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
> +unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
>  {
>
> -       struct vring_virtqueue *vq = to_vvq(_vq);
> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>
>         return vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
>  }
> @@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
>  }
>  EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
>
> -bool virtqueue_is_broken(struct virtqueue *_vq)
> +bool virtqueue_is_broken(const struct virtqueue *_vq)
>  {
> -       struct vring_virtqueue *vq = to_vvq(_vq);
> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>
>         return READ_ONCE(vq->broken);
>  }
> @@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
>
> -dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *_vq)
>  {
> -       struct vring_virtqueue *vq = to_vvq(_vq);
> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>
>         BUG_ON(!vq->we_own_ring);
>
> @@ -2881,9 +2881,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
>
> -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
>  {
> -       struct vring_virtqueue *vq = to_vvq(_vq);
> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>
>         BUG_ON(!vq->we_own_ring);
>
> @@ -2895,9 +2895,9 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr);
>
> -dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq)
>  {
> -       struct vring_virtqueue *vq = to_vvq(_vq);
> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>
>         BUG_ON(!vq->we_own_ring);
>
> @@ -2910,7 +2910,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
>  EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
>
>  /* Only available for split ring */
> -const struct vring *virtqueue_get_vring(struct virtqueue *vq)
> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq)
>  {
>         return &to_vvq(vq)->split.vring;
>  }
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 2b472514c49b..c4225653f949 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
>
>  void *virtqueue_detach_unused_buf(struct virtqueue *vq);
>
> -unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
> +unsigned int virtqueue_get_vring_size(const struct virtqueue *vq);
>
> -bool virtqueue_is_broken(struct virtqueue *vq);
> +bool virtqueue_is_broken(const struct virtqueue *vq);
>
> -const struct vring *virtqueue_get_vring(struct virtqueue *vq);
> -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);
> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq);
> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq);
> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq);
> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq);
>
>  int virtqueue_resize(struct virtqueue *vq, u32 num,
>                      void (*recycle)(struct virtqueue *vq, void *buf));
> @@ -147,7 +147,7 @@ int virtio_device_restore(struct virtio_device *dev);
>  #endif
>  void virtio_reset_device(struct virtio_device *dev);
>
> -size_t virtio_max_dma_size(struct virtio_device *vdev);
> +size_t virtio_max_dma_size(const struct virtio_device *vdev);
>
>  #define virtio_device_for_each_vq(vdev, vq) \
>         list_for_each_entry(vq, &vdev->vqs, list)
> --
> 2.34.1
>

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

Re: [PATCH v2 1/3] virtio_ring: Allow non power of 2 sizes for packed virtqueue

[David Edmondson]

Feng Liu <feliu@nvidia.com> writes:

> According to the Virtio Specification, the Queue Size parameter of a
> virtqueue corresponds to the maximum number of descriptors in that
> queue, and it does not have to be a power of 2 for packed virtqueues.
> However, the virtio_pci_modern driver enforced a power of 2 check for
> virtqueue sizes, which is unnecessary and restrictive for packed
> virtuqueue.
>
> Split virtqueue still needs to check the virtqueue size is power_of_2
> which has been done in vring_alloc_queue_split of the virtio_ring layer.
>
> To validate this change, we tested various virtqueue sizes for packed
> rings, including 128, 256, 512, 100, 200, 500, and 1000, with
> CONFIG_PAGE_POISONING enabled, and all tests passed successfully.
>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
> v0 -> v1
> feedbacks from Jason Wang and Michael S. Tsirkin
> - remove power_of_2 check of virtqueue size
>
> v1 -> v2
> feedbacks from Parav Pandit and Jiri Pirko
> - keep power_of_2 check of split virtqueue in vring_alloc_queue_split of
>   virtio_ring layer.
> ---
>  drivers/virtio/virtio_pci_modern.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 9e496e288cfa..6e713904d8e8 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -310,11 +310,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>  	if (!num || vp_modern_get_queue_enable(mdev, index))
>  		return ERR_PTR(-ENOENT);
>  
> -	if (!is_power_of_2(num)) {
> -		dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
> -		return ERR_PTR(-EINVAL);
> -	}
> -
>  	info->msix_vector = msix_vec;
>  
>  	/* create the vring */
> -- 
> 2.34.1
-- 
Do not leave the building.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/3] virtio_ring: Avoid using inline for small functions

[David Edmondson]

Feng Liu <feliu@nvidia.com> writes:

> Remove the inline keyword, according to kernel coding style [1], defining
> inline functions is not necessary for samll functions.
>
> It is verified with GCC 12.2.0, the generated code with/without inline
> is the same. Additionally tested with kernel pktgen and iperf, and
> verified the result, pps of the results are the same in the cases of
> with/without inline.
>
> [1]
> https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease
>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
>  drivers/virtio/virtio_ring.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 41144b5246a8..a26fab91c59f 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -233,8 +233,8 @@ static void vring_free(struct virtqueue *_vq);
>  
>  #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
>  
> -static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> -					  unsigned int total_sg)
> +static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> +				   unsigned int total_sg)
>  {
>  	/*
>  	 * If the host supports indirect descriptor tables, and we have multiple
> @@ -349,7 +349,7 @@ static void vring_free_queue(struct virtio_device *vdev, size_t size,
>   * making all of the arch DMA ops work on the vring device itself
>   * is a mess.
>   */
> -static inline struct device *vring_dma_dev(const struct vring_virtqueue *vq)
> +static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
>  {
>  	return vq->dma_dev;
>  }
> @@ -784,7 +784,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>  	}
>  }
>  
> -static inline bool more_used_split(const struct vring_virtqueue *vq)
> +static bool more_used_split(const struct vring_virtqueue *vq)
>  {
>  	return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev,
>  			vq->split.vring.used->idx);
> @@ -1172,12 +1172,12 @@ static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
>  /*
>   * Packed ring specific functions - *_packed().
>   */
> -static inline bool packed_used_wrap_counter(u16 last_used_idx)
> +static bool packed_used_wrap_counter(u16 last_used_idx)
>  {
>  	return !!(last_used_idx & (1 << VRING_PACKED_EVENT_F_WRAP_CTR));
>  }
>  
> -static inline u16 packed_last_used(u16 last_used_idx)
> +static u16 packed_last_used(u16 last_used_idx)
>  {
>  	return last_used_idx & ~(-(1 << VRING_PACKED_EVENT_F_WRAP_CTR));
>  }
> @@ -1612,7 +1612,7 @@ static inline bool is_used_desc_packed(const struct vring_virtqueue *vq,
>  	return avail == used && used == used_wrap_counter;
>  }
>  
> -static inline bool more_used_packed(const struct vring_virtqueue *vq)
> +static bool more_used_packed(const struct vring_virtqueue *vq)
>  {
>  	u16 last_used;
>  	u16 last_used_idx;
> -- 
> 2.34.1
-- 
But are you safe Miss Gradenko?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/3] virtio_ring: Use const to annotate read-only pointer params

[David Edmondson]

Feng Liu <feliu@nvidia.com> writes:

> Add const to make the read-only pointer parameters clear, similar to
> many existing functions.
>
> Use `container_of_const` to implement `to_vvq`, which ensures the
> const-ness of read-only parameters and avoids accidental modification
> of their members.
>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>

Reviewed-by: David Edmondson <david.edmondson@oracle.com>

> ---
> v0 -> v1
> feedbacks from Michael S. Tsirkin
> - use `container_of_const` to implement `to_vvq`
> ---
>  drivers/virtio/virtio_ring.c | 36 ++++++++++++++++++------------------
>  include/linux/virtio.h       | 14 +++++++-------
>  2 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index a26fab91c59f..4c3bb0ddeb9b 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
>   * Helpers.
>   */
>  
> -#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> +#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
>  
> -static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> +static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
>  				   unsigned int total_sg)
>  {
>  	/*
> @@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>   * unconditionally on data path.
>   */
>  
> -static bool vring_use_dma_api(struct virtio_device *vdev)
> +static bool vring_use_dma_api(const struct virtio_device *vdev)
>  {
>  	if (!virtio_has_dma_quirk(vdev))
>  		return true;
> @@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>  	return false;
>  }
>  
> -size_t virtio_max_dma_size(struct virtio_device *vdev)
> +size_t virtio_max_dma_size(const struct virtio_device *vdev)
>  {
>  	size_t max_segment_size = SIZE_MAX;
>  
> @@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
>   */
>  
>  static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> -					   struct vring_desc *desc)
> +					   const struct vring_desc *desc)
>  {
>  	u16 flags;
>  
> @@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
>  }
>  
>  static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> -				     struct vring_desc_extra *extra)
> +				     const struct vring_desc_extra *extra)
>  {
>  	u16 flags;
>  
> @@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
>  }
>  
>  static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
> -				   struct vring_packed_desc *desc)
> +				    const struct vring_packed_desc *desc)
>  {
>  	u16 flags;
>  
> @@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
>   * Returns the size of the vring.  This is mainly used for boasting to
>   * userspace.  Unlike other operations, this need not be serialized.
>   */
> -unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
> +unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
>  {
>  
> -	struct vring_virtqueue *vq = to_vvq(_vq);
> +	const struct vring_virtqueue *vq = to_vvq(_vq);
>  
>  	return vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
>  }
> @@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
>  }
>  EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
>  
> -bool virtqueue_is_broken(struct virtqueue *_vq)
> +bool virtqueue_is_broken(const struct virtqueue *_vq)
>  {
> -	struct vring_virtqueue *vq = to_vvq(_vq);
> +	const struct vring_virtqueue *vq = to_vvq(_vq);
>  
>  	return READ_ONCE(vq->broken);
>  }
> @@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
>  
> -dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *_vq)
>  {
> -	struct vring_virtqueue *vq = to_vvq(_vq);
> +	const struct vring_virtqueue *vq = to_vvq(_vq);
>  
>  	BUG_ON(!vq->we_own_ring);
>  
> @@ -2881,9 +2881,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
>  
> -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
>  {
> -	struct vring_virtqueue *vq = to_vvq(_vq);
> +	const struct vring_virtqueue *vq = to_vvq(_vq);
>  
>  	BUG_ON(!vq->we_own_ring);
>  
> @@ -2895,9 +2895,9 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr);
>  
> -dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq)
>  {
> -	struct vring_virtqueue *vq = to_vvq(_vq);
> +	const struct vring_virtqueue *vq = to_vvq(_vq);
>  
>  	BUG_ON(!vq->we_own_ring);
>  
> @@ -2910,7 +2910,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
>  EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
>  
>  /* Only available for split ring */
> -const struct vring *virtqueue_get_vring(struct virtqueue *vq)
> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq)
>  {
>  	return &to_vvq(vq)->split.vring;
>  }
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 2b472514c49b..c4225653f949 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
>  
>  void *virtqueue_detach_unused_buf(struct virtqueue *vq);
>  
> -unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
> +unsigned int virtqueue_get_vring_size(const struct virtqueue *vq);
>  
> -bool virtqueue_is_broken(struct virtqueue *vq);
> +bool virtqueue_is_broken(const struct virtqueue *vq);
>  
> -const struct vring *virtqueue_get_vring(struct virtqueue *vq);
> -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);
> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq);
> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq);
> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq);
> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq);
>  
>  int virtqueue_resize(struct virtqueue *vq, u32 num,
>  		     void (*recycle)(struct virtqueue *vq, void *buf));
> @@ -147,7 +147,7 @@ int virtio_device_restore(struct virtio_device *dev);
>  #endif
>  void virtio_reset_device(struct virtio_device *dev);
>  
> -size_t virtio_max_dma_size(struct virtio_device *vdev);
> +size_t virtio_max_dma_size(const struct virtio_device *vdev);
>  
>  #define virtio_device_for_each_vq(vdev, vq) \
>  	list_for_each_entry(vq, &vdev->vqs, list)
> -- 
> 2.34.1
-- 
I do believe it's Madame Joy.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/3] virtio_ring: Allow non power of 2 sizes for packed virtqueue

[Feng Liu via Virtualization]

On 2023-03-16 p.m.11:16, Jason Wang wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Mar 16, 2023 at 2:55 AM Feng Liu <feliu@nvidia.com> wrote:
>>
>> According to the Virtio Specification, the Queue Size parameter of a
>> virtqueue corresponds to the maximum number of descriptors in that
>> queue, and it does not have to be a power of 2 for packed virtqueues.
>> However, the virtio_pci_modern driver enforced a power of 2 check for
>> virtqueue sizes, which is unnecessary and restrictive for packed
>> virtuqueue.
>>
>> Split virtqueue still needs to check the virtqueue size is power_of_2
>> which has been done in vring_alloc_queue_split of the virtio_ring layer.
>>
>> To validate this change, we tested various virtqueue sizes for packed
>> rings, including 128, 256, 512, 100, 200, 500, and 1000, with
>> CONFIG_PAGE_POISONING enabled, and all tests passed successfully.
>>
>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> Thanks
> 
Hi Michael & Jason
	Could you please help to take these reviewed/acked patches forward? 
Thanks so much

>>
>> ---
>> v0 -> v1
>> feedbacks from Jason Wang and Michael S. Tsirkin
>> - remove power_of_2 check of virtqueue size
>>
>> v1 -> v2
>> feedbacks from Parav Pandit and Jiri Pirko
>> - keep power_of_2 check of split virtqueue in vring_alloc_queue_split of
>>    virtio_ring layer.
>> ---
>>   drivers/virtio/virtio_pci_modern.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>> index 9e496e288cfa..6e713904d8e8 100644
>> --- a/drivers/virtio/virtio_pci_modern.c
>> +++ b/drivers/virtio/virtio_pci_modern.c
>> @@ -310,11 +310,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>>          if (!num || vp_modern_get_queue_enable(mdev, index))
>>                  return ERR_PTR(-ENOENT);
>>
>> -       if (!is_power_of_2(num)) {
>> -               dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
>> -               return ERR_PTR(-EINVAL);
>> -       }
>> -
>>          info->msix_vector = msix_vec;
>>
>>          /* create the vring */
>> --
>> 2.34.1
>>
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/3] virtio_ring: Avoid using inline for small functions

[Feng Liu via Virtualization]

On 2023-03-16 p.m.11:16, Jason Wang wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Mar 16, 2023 at 2:55 AM Feng Liu <feliu@nvidia.com> wrote:
>>
>> Remove the inline keyword, according to kernel coding style [1], defining
>> inline functions is not necessary for samll functions.
>>
>> It is verified with GCC 12.2.0, the generated code with/without inline
>> is the same. Additionally tested with kernel pktgen and iperf, and
>> verified the result, pps of the results are the same in the cases of
>> with/without inline.
>>
>> [1]
>> https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease
>>
>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> Thanks
> 
Hi Michael & Jason
         Could you please help to take these reviewed/acked patches forward?
Thanks so much


>> ---
>>   drivers/virtio/virtio_ring.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index 41144b5246a8..a26fab91c59f 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -233,8 +233,8 @@ static void vring_free(struct virtqueue *_vq);
>>
>>   #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
>>
>> -static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>> -                                         unsigned int total_sg)
>> +static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>> +                                  unsigned int total_sg)
>>   {
>>          /*
>>           * If the host supports indirect descriptor tables, and we have multiple
>> @@ -349,7 +349,7 @@ static void vring_free_queue(struct virtio_device *vdev, size_t size,
>>    * making all of the arch DMA ops work on the vring device itself
>>    * is a mess.
>>    */
>> -static inline struct device *vring_dma_dev(const struct vring_virtqueue *vq)
>> +static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
>>   {
>>          return vq->dma_dev;
>>   }
>> @@ -784,7 +784,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
>>          }
>>   }
>>
>> -static inline bool more_used_split(const struct vring_virtqueue *vq)
>> +static bool more_used_split(const struct vring_virtqueue *vq)
>>   {
>>          return vq->last_used_idx != virtio16_to_cpu(vq->vq.vdev,
>>                          vq->split.vring.used->idx);
>> @@ -1172,12 +1172,12 @@ static int virtqueue_resize_split(struct virtqueue *_vq, u32 num)
>>   /*
>>    * Packed ring specific functions - *_packed().
>>    */
>> -static inline bool packed_used_wrap_counter(u16 last_used_idx)
>> +static bool packed_used_wrap_counter(u16 last_used_idx)
>>   {
>>          return !!(last_used_idx & (1 << VRING_PACKED_EVENT_F_WRAP_CTR));
>>   }
>>
>> -static inline u16 packed_last_used(u16 last_used_idx)
>> +static u16 packed_last_used(u16 last_used_idx)
>>   {
>>          return last_used_idx & ~(-(1 << VRING_PACKED_EVENT_F_WRAP_CTR));
>>   }
>> @@ -1612,7 +1612,7 @@ static inline bool is_used_desc_packed(const struct vring_virtqueue *vq,
>>          return avail == used && used == used_wrap_counter;
>>   }
>>
>> -static inline bool more_used_packed(const struct vring_virtqueue *vq)
>> +static bool more_used_packed(const struct vring_virtqueue *vq)
>>   {
>>          u16 last_used;
>>          u16 last_used_idx;
>> --
>> 2.34.1
>>
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/3] virtio_ring: Use const to annotate read-only pointer params

[Feng Liu via Virtualization]

On 2023-03-16 p.m.11:17, Jason Wang wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Mar 16, 2023 at 2:55 AM Feng Liu <feliu@nvidia.com> wrote:
>>
>> Add const to make the read-only pointer parameters clear, similar to
>> many existing functions.
>>
>> Use `container_of_const` to implement `to_vvq`, which ensures the
>> const-ness of read-only parameters and avoids accidental modification
>> of their members.
>>
>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> Thanks
> 
Hi Michael & Jason
         Could you please help to take these reviewed/acked patches forward?
Thanks so much


>>
>> ---
>> v0 -> v1
>> feedbacks from Michael S. Tsirkin
>> - use `container_of_const` to implement `to_vvq`
>> ---
>>   drivers/virtio/virtio_ring.c | 36 ++++++++++++++++++------------------
>>   include/linux/virtio.h       | 14 +++++++-------
>>   2 files changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index a26fab91c59f..4c3bb0ddeb9b 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
>>    * Helpers.
>>    */
>>
>> -#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
>> +#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
>>
>> -static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>> +static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
>>                                     unsigned int total_sg)
>>   {
>>          /*
>> @@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>>    * unconditionally on data path.
>>    */
>>
>> -static bool vring_use_dma_api(struct virtio_device *vdev)
>> +static bool vring_use_dma_api(const struct virtio_device *vdev)
>>   {
>>          if (!virtio_has_dma_quirk(vdev))
>>                  return true;
>> @@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>>          return false;
>>   }
>>
>> -size_t virtio_max_dma_size(struct virtio_device *vdev)
>> +size_t virtio_max_dma_size(const struct virtio_device *vdev)
>>   {
>>          size_t max_segment_size = SIZE_MAX;
>>
>> @@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
>>    */
>>
>>   static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
>> -                                          struct vring_desc *desc)
>> +                                          const struct vring_desc *desc)
>>   {
>>          u16 flags;
>>
>> @@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
>>   }
>>
>>   static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
>> -                                    struct vring_desc_extra *extra)
>> +                                    const struct vring_desc_extra *extra)
>>   {
>>          u16 flags;
>>
>> @@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
>>   }
>>
>>   static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
>> -                                  struct vring_packed_desc *desc)
>> +                                   const struct vring_packed_desc *desc)
>>   {
>>          u16 flags;
>>
>> @@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
>>    * Returns the size of the vring.  This is mainly used for boasting to
>>    * userspace.  Unlike other operations, this need not be serialized.
>>    */
>> -unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
>> +unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
>>   {
>>
>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>
>>          return vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
>>   }
>> @@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
>>   }
>>   EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
>>
>> -bool virtqueue_is_broken(struct virtqueue *_vq)
>> +bool virtqueue_is_broken(const struct virtqueue *_vq)
>>   {
>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>
>>          return READ_ONCE(vq->broken);
>>   }
>> @@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
>>
>> -dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
>> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *_vq)
>>   {
>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>
>>          BUG_ON(!vq->we_own_ring);
>>
>> @@ -2881,9 +2881,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
>>   }
>>   EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
>>
>> -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
>> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
>>   {
>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>
>>          BUG_ON(!vq->we_own_ring);
>>
>> @@ -2895,9 +2895,9 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
>>   }
>>   EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr);
>>
>> -dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
>> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq)
>>   {
>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>
>>          BUG_ON(!vq->we_own_ring);
>>
>> @@ -2910,7 +2910,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
>>   EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
>>
>>   /* Only available for split ring */
>> -const struct vring *virtqueue_get_vring(struct virtqueue *vq)
>> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq)
>>   {
>>          return &to_vvq(vq)->split.vring;
>>   }
>> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
>> index 2b472514c49b..c4225653f949 100644
>> --- a/include/linux/virtio.h
>> +++ b/include/linux/virtio.h
>> @@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
>>
>>   void *virtqueue_detach_unused_buf(struct virtqueue *vq);
>>
>> -unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
>> +unsigned int virtqueue_get_vring_size(const struct virtqueue *vq);
>>
>> -bool virtqueue_is_broken(struct virtqueue *vq);
>> +bool virtqueue_is_broken(const struct virtqueue *vq);
>>
>> -const struct vring *virtqueue_get_vring(struct virtqueue *vq);
>> -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);
>> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq);
>> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq);
>> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq);
>> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq);
>>
>>   int virtqueue_resize(struct virtqueue *vq, u32 num,
>>                       void (*recycle)(struct virtqueue *vq, void *buf));
>> @@ -147,7 +147,7 @@ int virtio_device_restore(struct virtio_device *dev);
>>   #endif
>>   void virtio_reset_device(struct virtio_device *dev);
>>
>> -size_t virtio_max_dma_size(struct virtio_device *vdev);
>> +size_t virtio_max_dma_size(const struct virtio_device *vdev);
>>
>>   #define virtio_device_for_each_vq(vdev, vq) \
>>          list_for_each_entry(vq, &vdev->vqs, list)
>> --
>> 2.34.1
>>
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 3/3] virtio_ring: Use const to annotate read-only pointer params

[Michael S. Tsirkin]

On Thu, Mar 30, 2023 at 02:22:44PM -0400, Feng Liu wrote:
> 
> 
> On 2023-03-16 p.m.11:17, Jason Wang wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Thu, Mar 16, 2023 at 2:55 AM Feng Liu <feliu@nvidia.com> wrote:
> > > 
> > > Add const to make the read-only pointer parameters clear, similar to
> > > many existing functions.
> > > 
> > > Use `container_of_const` to implement `to_vvq`, which ensures the
> > > const-ness of read-only parameters and avoids accidental modification
> > > of their members.
> > > 
> > > Signed-off-by: Feng Liu <feliu@nvidia.com>
> > > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> > 
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > 
> > Thanks
> > 
> Hi Michael & Jason
>         Could you please help to take these reviewed/acked patches forward?
> Thanks so much


this is not going in this linux, only the next.

> 
> > > 
> > > ---
> > > v0 -> v1
> > > feedbacks from Michael S. Tsirkin
> > > - use `container_of_const` to implement `to_vvq`
> > > ---
> > >   drivers/virtio/virtio_ring.c | 36 ++++++++++++++++++------------------
> > >   include/linux/virtio.h       | 14 +++++++-------
> > >   2 files changed, 25 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index a26fab91c59f..4c3bb0ddeb9b 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
> > >    * Helpers.
> > >    */
> > > 
> > > -#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> > > +#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
> > > 
> > > -static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> > > +static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
> > >                                     unsigned int total_sg)
> > >   {
> > >          /*
> > > @@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> > >    * unconditionally on data path.
> > >    */
> > > 
> > > -static bool vring_use_dma_api(struct virtio_device *vdev)
> > > +static bool vring_use_dma_api(const struct virtio_device *vdev)
> > >   {
> > >          if (!virtio_has_dma_quirk(vdev))
> > >                  return true;
> > > @@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
> > >          return false;
> > >   }
> > > 
> > > -size_t virtio_max_dma_size(struct virtio_device *vdev)
> > > +size_t virtio_max_dma_size(const struct virtio_device *vdev)
> > >   {
> > >          size_t max_segment_size = SIZE_MAX;
> > > 
> > > @@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
> > >    */
> > > 
> > >   static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> > > -                                          struct vring_desc *desc)
> > > +                                          const struct vring_desc *desc)
> > >   {
> > >          u16 flags;
> > > 
> > > @@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
> > >   }
> > > 
> > >   static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> > > -                                    struct vring_desc_extra *extra)
> > > +                                    const struct vring_desc_extra *extra)
> > >   {
> > >          u16 flags;
> > > 
> > > @@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> > >   }
> > > 
> > >   static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
> > > -                                  struct vring_packed_desc *desc)
> > > +                                   const struct vring_packed_desc *desc)
> > >   {
> > >          u16 flags;
> > > 
> > > @@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
> > >    * Returns the size of the vring.  This is mainly used for boasting to
> > >    * userspace.  Unlike other operations, this need not be serialized.
> > >    */
> > > -unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
> > > +unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
> > >   {
> > > 
> > > -       struct vring_virtqueue *vq = to_vvq(_vq);
> > > +       const struct vring_virtqueue *vq = to_vvq(_vq);
> > > 
> > >          return vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
> > >   }
> > > @@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
> > >   }
> > >   EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
> > > 
> > > -bool virtqueue_is_broken(struct virtqueue *_vq)
> > > +bool virtqueue_is_broken(const struct virtqueue *_vq)
> > >   {
> > > -       struct vring_virtqueue *vq = to_vvq(_vq);
> > > +       const struct vring_virtqueue *vq = to_vvq(_vq);
> > > 
> > >          return READ_ONCE(vq->broken);
> > >   }
> > > @@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device *dev)
> > >   }
> > >   EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
> > > 
> > > -dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
> > > +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *_vq)
> > >   {
> > > -       struct vring_virtqueue *vq = to_vvq(_vq);
> > > +       const struct vring_virtqueue *vq = to_vvq(_vq);
> > > 
> > >          BUG_ON(!vq->we_own_ring);
> > > 
> > > @@ -2881,9 +2881,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
> > >   }
> > >   EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
> > > 
> > > -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
> > > +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
> > >   {
> > > -       struct vring_virtqueue *vq = to_vvq(_vq);
> > > +       const struct vring_virtqueue *vq = to_vvq(_vq);
> > > 
> > >          BUG_ON(!vq->we_own_ring);
> > > 
> > > @@ -2895,9 +2895,9 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
> > >   }
> > >   EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr);
> > > 
> > > -dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
> > > +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq)
> > >   {
> > > -       struct vring_virtqueue *vq = to_vvq(_vq);
> > > +       const struct vring_virtqueue *vq = to_vvq(_vq);
> > > 
> > >          BUG_ON(!vq->we_own_ring);
> > > 
> > > @@ -2910,7 +2910,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
> > >   EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
> > > 
> > >   /* Only available for split ring */
> > > -const struct vring *virtqueue_get_vring(struct virtqueue *vq)
> > > +const struct vring *virtqueue_get_vring(const struct virtqueue *vq)
> > >   {
> > >          return &to_vvq(vq)->split.vring;
> > >   }
> > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> > > index 2b472514c49b..c4225653f949 100644
> > > --- a/include/linux/virtio.h
> > > +++ b/include/linux/virtio.h
> > > @@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
> > > 
> > >   void *virtqueue_detach_unused_buf(struct virtqueue *vq);
> > > 
> > > -unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
> > > +unsigned int virtqueue_get_vring_size(const struct virtqueue *vq);
> > > 
> > > -bool virtqueue_is_broken(struct virtqueue *vq);
> > > +bool virtqueue_is_broken(const struct virtqueue *vq);
> > > 
> > > -const struct vring *virtqueue_get_vring(struct virtqueue *vq);
> > > -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);
> > > +const struct vring *virtqueue_get_vring(const struct virtqueue *vq);
> > > +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq);
> > > +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq);
> > > +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq);
> > > 
> > >   int virtqueue_resize(struct virtqueue *vq, u32 num,
> > >                       void (*recycle)(struct virtqueue *vq, void *buf));
> > > @@ -147,7 +147,7 @@ int virtio_device_restore(struct virtio_device *dev);
> > >   #endif
> > >   void virtio_reset_device(struct virtio_device *dev);
> > > 
> > > -size_t virtio_max_dma_size(struct virtio_device *vdev);
> > > +size_t virtio_max_dma_size(const struct virtio_device *vdev);
> > > 
> > >   #define virtio_device_for_each_vq(vdev, vq) \
> > >          list_for_each_entry(vq, &vdev->vqs, list)
> > > --
> > > 2.34.1
> > > 
> > 

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

Re: [PATCH v2 3/3] virtio_ring: Use const to annotate read-only pointer params

[Feng Liu via Virtualization]

On 2023-03-30 p.m.4:27, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, Mar 30, 2023 at 02:22:44PM -0400, Feng Liu wrote:
>>
>>
>> On 2023-03-16 p.m.11:17, Jason Wang wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Thu, Mar 16, 2023 at 2:55 AM Feng Liu <feliu@nvidia.com> wrote:
>>>>
>>>> Add const to make the read-only pointer parameters clear, similar to
>>>> many existing functions.
>>>>
>>>> Use `container_of_const` to implement `to_vvq`, which ensures the
>>>> const-ness of read-only parameters and avoids accidental modification
>>>> of their members.
>>>>
>>>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>
>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>>
>>> Thanks
>>>
>> Hi Michael & Jason
>>          Could you please help to take these reviewed/acked patches forward?
>> Thanks so much
> 
> 
> this is not going in this linux, only the next.
> 
Hi, Michael
	Thanks for the update.

>>
>>>>
>>>> ---
>>>> v0 -> v1
>>>> feedbacks from Michael S. Tsirkin
>>>> - use `container_of_const` to implement `to_vvq`
>>>> ---
>>>>    drivers/virtio/virtio_ring.c | 36 ++++++++++++++++++------------------
>>>>    include/linux/virtio.h       | 14 +++++++-------
>>>>    2 files changed, 25 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>>> index a26fab91c59f..4c3bb0ddeb9b 100644
>>>> --- a/drivers/virtio/virtio_ring.c
>>>> +++ b/drivers/virtio/virtio_ring.c
>>>> @@ -231,9 +231,9 @@ static void vring_free(struct virtqueue *_vq);
>>>>     * Helpers.
>>>>     */
>>>>
>>>> -#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
>>>> +#define to_vvq(_vq) container_of_const(_vq, struct vring_virtqueue, vq)
>>>>
>>>> -static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>>>> +static bool virtqueue_use_indirect(const struct vring_virtqueue *vq,
>>>>                                      unsigned int total_sg)
>>>>    {
>>>>           /*
>>>> @@ -269,7 +269,7 @@ static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>>>>     * unconditionally on data path.
>>>>     */
>>>>
>>>> -static bool vring_use_dma_api(struct virtio_device *vdev)
>>>> +static bool vring_use_dma_api(const struct virtio_device *vdev)
>>>>    {
>>>>           if (!virtio_has_dma_quirk(vdev))
>>>>                   return true;
>>>> @@ -289,7 +289,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
>>>>           return false;
>>>>    }
>>>>
>>>> -size_t virtio_max_dma_size(struct virtio_device *vdev)
>>>> +size_t virtio_max_dma_size(const struct virtio_device *vdev)
>>>>    {
>>>>           size_t max_segment_size = SIZE_MAX;
>>>>
>>>> @@ -423,7 +423,7 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
>>>>     */
>>>>
>>>>    static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
>>>> -                                          struct vring_desc *desc)
>>>> +                                          const struct vring_desc *desc)
>>>>    {
>>>>           u16 flags;
>>>>
>>>> @@ -1183,7 +1183,7 @@ static u16 packed_last_used(u16 last_used_idx)
>>>>    }
>>>>
>>>>    static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
>>>> -                                    struct vring_desc_extra *extra)
>>>> +                                    const struct vring_desc_extra *extra)
>>>>    {
>>>>           u16 flags;
>>>>
>>>> @@ -1206,7 +1206,7 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
>>>>    }
>>>>
>>>>    static void vring_unmap_desc_packed(const struct vring_virtqueue *vq,
>>>> -                                  struct vring_packed_desc *desc)
>>>> +                                   const struct vring_packed_desc *desc)
>>>>    {
>>>>           u16 flags;
>>>>
>>>> @@ -2786,10 +2786,10 @@ EXPORT_SYMBOL_GPL(vring_transport_features);
>>>>     * Returns the size of the vring.  This is mainly used for boasting to
>>>>     * userspace.  Unlike other operations, this need not be serialized.
>>>>     */
>>>> -unsigned int virtqueue_get_vring_size(struct virtqueue *_vq)
>>>> +unsigned int virtqueue_get_vring_size(const struct virtqueue *_vq)
>>>>    {
>>>>
>>>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>>>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>>>
>>>>           return vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
>>>>    }
>>>> @@ -2819,9 +2819,9 @@ void __virtqueue_unbreak(struct virtqueue *_vq)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(__virtqueue_unbreak);
>>>>
>>>> -bool virtqueue_is_broken(struct virtqueue *_vq)
>>>> +bool virtqueue_is_broken(const struct virtqueue *_vq)
>>>>    {
>>>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>>>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>>>
>>>>           return READ_ONCE(vq->broken);
>>>>    }
>>>> @@ -2868,9 +2868,9 @@ void __virtio_unbreak_device(struct virtio_device *dev)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(__virtio_unbreak_device);
>>>>
>>>> -dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
>>>> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *_vq)
>>>>    {
>>>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>>>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>>>
>>>>           BUG_ON(!vq->we_own_ring);
>>>>
>>>> @@ -2881,9 +2881,9 @@ dma_addr_t virtqueue_get_desc_addr(struct virtqueue *_vq)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(virtqueue_get_desc_addr);
>>>>
>>>> -dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
>>>> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *_vq)
>>>>    {
>>>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>>>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>>>
>>>>           BUG_ON(!vq->we_own_ring);
>>>>
>>>> @@ -2895,9 +2895,9 @@ dma_addr_t virtqueue_get_avail_addr(struct virtqueue *_vq)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(virtqueue_get_avail_addr);
>>>>
>>>> -dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
>>>> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *_vq)
>>>>    {
>>>> -       struct vring_virtqueue *vq = to_vvq(_vq);
>>>> +       const struct vring_virtqueue *vq = to_vvq(_vq);
>>>>
>>>>           BUG_ON(!vq->we_own_ring);
>>>>
>>>> @@ -2910,7 +2910,7 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *_vq)
>>>>    EXPORT_SYMBOL_GPL(virtqueue_get_used_addr);
>>>>
>>>>    /* Only available for split ring */
>>>> -const struct vring *virtqueue_get_vring(struct virtqueue *vq)
>>>> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq)
>>>>    {
>>>>           return &to_vvq(vq)->split.vring;
>>>>    }
>>>> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
>>>> index 2b472514c49b..c4225653f949 100644
>>>> --- a/include/linux/virtio.h
>>>> +++ b/include/linux/virtio.h
>>>> @@ -84,14 +84,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
>>>>
>>>>    void *virtqueue_detach_unused_buf(struct virtqueue *vq);
>>>>
>>>> -unsigned int virtqueue_get_vring_size(struct virtqueue *vq);
>>>> +unsigned int virtqueue_get_vring_size(const struct virtqueue *vq);
>>>>
>>>> -bool virtqueue_is_broken(struct virtqueue *vq);
>>>> +bool virtqueue_is_broken(const struct virtqueue *vq);
>>>>
>>>> -const struct vring *virtqueue_get_vring(struct virtqueue *vq);
>>>> -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);
>>>> +const struct vring *virtqueue_get_vring(const struct virtqueue *vq);
>>>> +dma_addr_t virtqueue_get_desc_addr(const struct virtqueue *vq);
>>>> +dma_addr_t virtqueue_get_avail_addr(const struct virtqueue *vq);
>>>> +dma_addr_t virtqueue_get_used_addr(const struct virtqueue *vq);
>>>>
>>>>    int virtqueue_resize(struct virtqueue *vq, u32 num,
>>>>                        void (*recycle)(struct virtqueue *vq, void *buf));
>>>> @@ -147,7 +147,7 @@ int virtio_device_restore(struct virtio_device *dev);
>>>>    #endif
>>>>    void virtio_reset_device(struct virtio_device *dev);
>>>>
>>>> -size_t virtio_max_dma_size(struct virtio_device *vdev);
>>>> +size_t virtio_max_dma_size(const struct virtio_device *vdev);
>>>>
>>>>    #define virtio_device_for_each_vq(vdev, vq) \
>>>>           list_for_each_entry(vq, &vdev->vqs, list)
>>>> --
>>>> 2.34.1
>>>>
>>>
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 0/3] virtio_ring: Clean up code for virtio ring and pci

[Xuan Zhuo]

Series:

Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Thanks.

On Wed, 15 Mar 2023 20:54:55 +0200, Feng Liu via Virtualization <virtualization@lists.linux-foundation.org> wrote:
> This patch series performs a clean up of the code in virtio_ring and
> virtio_pci, modifying it to conform with the Linux kernel coding style
> guidance [1]. The modifications ensure the code easy to read and
> understand. This small series does few short cleanups in the code.
>
> Patch-1 Allow non power of 2 sizes for packed virtqueues.
> Patch-2 Avoid using inline for small functions.
> Patch-3 Use const to annotate read-only pointer params.
>
> [1]
> https://www.kernel.org/doc/html/v6.2-rc3/process/coding-style.html#the-inline-disease
>
> All of the patches have been verified based on the kernel code
> commit 81ff855485a3 ("Merge tag 'i2c-for-6.3-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux")
>
> Feng Liu (3):
>   virtio_ring: Allow non power of 2 sizes for packed virtqueue
>   virtio_ring: Avoid using inline for small functions
>   virtio_ring: Use const to annotate read-only pointer params
>
>  drivers/virtio/virtio_pci_modern.c |  5 ----
>  drivers/virtio/virtio_ring.c       | 48 +++++++++++++++---------------
>  include/linux/virtio.h             | 14 ++++-----
>  3 files changed, 31 insertions(+), 36 deletions(-)
>
> --
> 2.34.1
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization