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

[PATCH 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 Remove unnecessary num zero check, which performs in power_of_2.
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

Feng Liu (3):
  virtio_pci_modern: Remove unnecessary num zero check
  virtio_ring: Avoid using inline for small functions
  virtio_ring: Use const to annotate read-only pointer params

 drivers/virtio/virtio_pci_modern.c |  4 ++--
 drivers/virtio/virtio_ring.c       | 35 +++++++++++++++---------------
 include/linux/virtio.h             | 12 +++++-----
 3 files changed, 25 insertions(+), 26 deletions(-)

-- 
2.34.1

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

[PATCH 1/3] virtio_pci_modern: Remove unnecessary num zero check

[Feng Liu via Virtualization]

is_power_of_2() already performs the zero check. Hence avoid duplicate
check. While at it, move the query of size check also adjacent to where
its used for the disabled vq.

Signed-off-by: Feng Liu <feliu@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Bodong Wang <bodong@nvidia.com>
---
 drivers/virtio/virtio_pci_modern.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 9e496e288cfa..3d7144f8f959 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -306,10 +306,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 		return ERR_PTR(-EINVAL);
 
 	/* Check if queue is either not available or already active. */
-	num = vp_modern_get_queue_size(mdev, index);
-	if (!num || vp_modern_get_queue_enable(mdev, index))
+	if (vp_modern_get_queue_enable(mdev, index))
 		return ERR_PTR(-ENOENT);
 
+	num = vp_modern_get_queue_size(mdev, index);
 	if (!is_power_of_2(num)) {
 		dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
 		return ERR_PTR(-EINVAL);
-- 
2.34.1

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

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

[Feng Liu via Virtualization]

According to kernel coding style [1], defining inline functions is not
necessary and beneficial for simple functions. Hence clean up the code
by removing the inline keyword.

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

Iperf and pps of pktgen for virtio-net didn't change before and after
the change.

[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: Parav Pandit <parav@nvidia.com>
Reviewed-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Bodong Wang <bodong@nvidia.com>
---
 drivers/virtio/virtio_ring.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 41144b5246a8..806cc44eae64 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -233,7 +233,7 @@ 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,
+static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
 					  unsigned int total_sg)
 {
 	/*
@@ -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 inline 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 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.

Signed-off-by: Feng Liu <feliu@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Gavin Li <gavinl@nvidia.com>
Reviewed-by: Bodong Wang <bodong@nvidia.com>
---
 drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
 include/linux/virtio.h       | 12 ++++++------
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 806cc44eae64..8010fd1d2047 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq);
 
 #define to_vvq(_vq) container_of(_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,7 +2786,7 @@ 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);
@@ -2819,7 +2819,7 @@ 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);
 
@@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
 }
 EXPORT_SYMBOL_GPL(virtqueue_is_broken);
 
-/*
- * This should prevent the device from being used, allowing drivers to
+/ This should prevent the device from being used, allowing drivers to
  * recover.  You may need to grab appropriate locks to flush.
  */
 void virtio_break_device(struct virtio_device *dev)
@@ -2881,7 +2880,7 @@ 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);
 
@@ -2895,7 +2894,7 @@ 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);
 
@@ -2910,7 +2909,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..36a79374e735 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));
-- 
2.34.1

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

Re: [PATCH 1/3] virtio_pci_modern: Remove unnecessary num zero check

[David Edmondson]

Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
writes:

> is_power_of_2() already performs the zero check. Hence avoid duplicate
> check. While at it, move the query of size check also adjacent to where
> its used for the disabled vq.
>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Bodong Wang <bodong@nvidia.com>

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

> ---
>  drivers/virtio/virtio_pci_modern.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 9e496e288cfa..3d7144f8f959 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -306,10 +306,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>  		return ERR_PTR(-EINVAL);
>  
>  	/* Check if queue is either not available or already active. */
> -	num = vp_modern_get_queue_size(mdev, index);
> -	if (!num || vp_modern_get_queue_enable(mdev, index))
> +	if (vp_modern_get_queue_enable(mdev, index))
>  		return ERR_PTR(-ENOENT);
>  
> +	num = vp_modern_get_queue_size(mdev, index);
>  	if (!is_power_of_2(num)) {
>  		dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
>  		return ERR_PTR(-EINVAL);
> -- 
> 2.34.1
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
-- 
It's funny, I spent my whole life wanting to be talked about.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[David Edmondson]

Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
writes:

> According to kernel coding style [1], defining inline functions is not
> necessary and beneficial for simple functions. Hence clean up the code
> by removing the inline keyword.
>
> It is verified with GCC 12.2.0, the generated code with/without inline
> is same. Additionally tested with pktgen and iperf, and verified the
> result, the pps test results are the same in the cases of with/without
> inline.
>
> Iperf and pps of pktgen for virtio-net didn't change before and after
> the change.
>
> [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: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Bodong Wang <bodong@nvidia.com>

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

> ---
>  drivers/virtio/virtio_ring.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 41144b5246a8..806cc44eae64 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -233,7 +233,7 @@ 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,
> +static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>  					  unsigned int total_sg)
>  {
>  	/*
> @@ -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 inline 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
-- 
Would you offer your throat to the wolf with the red roses?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[David Edmondson]

Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
writes:

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

In many of the modified functions the local variable that is a cast of
the argument could also be const. Is there a reason not to do both at
the same time?

> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Bodong Wang <bodong@nvidia.com>
> ---
>  drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
>  include/linux/virtio.h       | 12 ++++++------
>  2 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 806cc44eae64..8010fd1d2047 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq);
>  
>  #define to_vvq(_vq) container_of(_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,7 +2786,7 @@ 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);
> @@ -2819,7 +2819,7 @@ 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);
>  
> @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_is_broken);
>  
> -/*
> - * This should prevent the device from being used, allowing drivers to
> +/ This should prevent the device from being used, allowing drivers to
>   * recover.  You may need to grab appropriate locks to flush.
>   */
>  void virtio_break_device(struct virtio_device *dev)
> @@ -2881,7 +2880,7 @@ 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);
>  
> @@ -2895,7 +2894,7 @@ 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);
>  
> @@ -2910,7 +2909,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..36a79374e735 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));
> -- 
> 2.34.1
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
-- 
Woke up in my clothes again this morning, don't know exactly where I am.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Feng Liu via Virtualization]

On 2023-03-07 04:14, David Edmondson wrote:
> External email: Use caution opening links or attachments
> 
> 
> Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
> writes:
> 
>> Add const to make the read-only pointer parameters clear, similar to
>> many existing functions.
> 
> In many of the modified functions the local variable that is a cast of
> the argument could also be const. Is there a reason not to do both at
> the same time?
> 

Hi,David

In order to prevent the content of a pointer parameter from being 
modified and increase the readability of the function, it is recommended 
to add the 'const' keyword to the parameter. This is not necessary for 
local variables and non-pointer parameters, as they are only stored on 
the stack and do not affect the original value or structure member 
passed into the function. Therefore, in this case, the 'const' keyword 
is only added to pointer parameters.


>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>> Reviewed-by: Gavin Li <gavinl@nvidia.com>
>> Reviewed-by: Bodong Wang <bodong@nvidia.com>
>> ---
>>   drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
>>   include/linux/virtio.h       | 12 ++++++------
>>   2 files changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index 806cc44eae64..8010fd1d2047 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq);
>>
>>   #define to_vvq(_vq) container_of(_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,7 +2786,7 @@ 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);
>> @@ -2819,7 +2819,7 @@ 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);
>>
>> @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
>>   }
>>   EXPORT_SYMBOL_GPL(virtqueue_is_broken);
>>
>> -/*
>> - * This should prevent the device from being used, allowing drivers to
>> +/ This should prevent the device from being used, allowing drivers to
>>    * recover.  You may need to grab appropriate locks to flush.
>>    */
>>   void virtio_break_device(struct virtio_device *dev)
>> @@ -2881,7 +2880,7 @@ 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);
>>
>> @@ -2895,7 +2894,7 @@ 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);
>>
>> @@ -2910,7 +2909,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..36a79374e735 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));
>> --
>> 2.34.1
>>
>> _______________________________________________
>> Virtualization mailing list
>> Virtualization@lists.linux-foundation.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linuxfoundation.org%2Fmailman%2Flistinfo%2Fvirtualization&data=05%7C01%7Cfeliu%40nvidia.com%7C6265cc6ac03e484167ba08db1eec5428%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638137772595544505%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=74eHEHUMF89nTG6hrrdMpkl43DJWvC9xCjwqNOny%2FQE%3D&reserved=0
> --
> Woke up in my clothes again this morning, don't know exactly where I am.

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

Re: [PATCH 1/3] virtio_pci_modern: Remove unnecessary num zero check

[Jason Wang]

On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
>
> is_power_of_2() already performs the zero check. Hence avoid duplicate
> check. While at it, move the query of size check also adjacent to where
> its used for the disabled vq.
>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Bodong Wang <bodong@nvidia.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 9e496e288cfa..3d7144f8f959 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -306,10 +306,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>                 return ERR_PTR(-EINVAL);
>
>         /* Check if queue is either not available or already active. */
> -       num = vp_modern_get_queue_size(mdev, index);
> -       if (!num || vp_modern_get_queue_enable(mdev, index))
> +       if (vp_modern_get_queue_enable(mdev, index))
>                 return ERR_PTR(-ENOENT);

Spec allows non power of 2 size for packed virtqueue, so I think we
should fix it in this way.

"""
Queue Size corresponds to the maximum number of descriptors in the
virtqueue5. The Queue Size value does not have to be a power of 2.
"""

Thanks

>
> +       num = vp_modern_get_queue_size(mdev, index);
>         if (!is_power_of_2(num)) {
>                 dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
>                 return ERR_PTR(-EINVAL);
> --
> 2.34.1
>

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

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

[Jason Wang]

On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
>
> According to kernel coding style [1], defining inline functions is not
> necessary and beneficial for simple functions. Hence clean up the code
> by removing the inline keyword.
>
> It is verified with GCC 12.2.0, the generated code with/without inline
> is same. Additionally tested with pktgen and iperf, and verified the
> result, the pps test results are the same in the cases of with/without
> inline.
>
> Iperf and pps of pktgen for virtio-net didn't change before and after
> the change.
>
> [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: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Bodong Wang <bodong@nvidia.com>
> ---
>  drivers/virtio/virtio_ring.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 41144b5246a8..806cc44eae64 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -233,7 +233,7 @@ 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,
> +static bool virtqueue_use_indirect(struct vring_virtqueue *vq,
>                                           unsigned int total_sg)
>  {
>         /*
> @@ -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 inline more_used_split(const struct vring_virtqueue *vq)

Typo here, s/inline/bool/?

Thanks

>  {
>         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 3/3] virtio_ring: Use const to annotate read-only pointer params

[Jason Wang]

On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
>
> Add const to make the read-only pointer parameters clear, similar to
> many existing functions.
>
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Bodong Wang <bodong@nvidia.com>
> ---
>  drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
>  include/linux/virtio.h       | 12 ++++++------
>  2 files changed, 18 insertions(+), 19 deletions(-)
>

[...]

>
> -/*
> - * This should prevent the device from being used, allowing drivers to
> +/ This should prevent the device from being used, allowing drivers to
>   * recover.  You may need to grab appropriate locks to flush.
>   */

Any reason for this change?

Thanks

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

Re: [PATCH 1/3] virtio_pci_modern: Remove unnecessary num zero check

[Michael S. Tsirkin]

On Wed, Mar 08, 2023 at 01:52:43PM +0800, Jason Wang wrote:
> On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
> >
> > is_power_of_2() already performs the zero check. Hence avoid duplicate
> > check. While at it, move the query of size check also adjacent to where
> > its used for the disabled vq.
> >
> > Signed-off-by: Feng Liu <feliu@nvidia.com>
> > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > Reviewed-by: Gavin Li <gavinl@nvidia.com>
> > Reviewed-by: Bodong Wang <bodong@nvidia.com>
> > ---
> >  drivers/virtio/virtio_pci_modern.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index 9e496e288cfa..3d7144f8f959 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -306,10 +306,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
> >                 return ERR_PTR(-EINVAL);
> >
> >         /* Check if queue is either not available or already active. */
> > -       num = vp_modern_get_queue_size(mdev, index);
> > -       if (!num || vp_modern_get_queue_enable(mdev, index))
> > +       if (vp_modern_get_queue_enable(mdev, index))
> >                 return ERR_PTR(-ENOENT);
> 
> Spec allows non power of 2 size for packed virtqueue, so I think we
> should fix it in this way.
> 
> """
> Queue Size corresponds to the maximum number of descriptors in the
> virtqueue5. The Queue Size value does not have to be a power of 2.
> """
> 
> Thanks

Oh yack. How come we never noticed :(

And it's been like this from day 1, too.


Let's fix this and backport everywhere we can.

> >
> > +       num = vp_modern_get_queue_size(mdev, index);
> >         if (!is_power_of_2(num)) {
> >                 dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
> >                 return ERR_PTR(-EINVAL);
> > --
> > 2.34.1
> >

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

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

[Michael S. Tsirkin]

On Tue, Mar 07, 2023 at 05:57:05AM +0200, Feng Liu wrote:
> @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_is_broken);
>  
> -/*
> - * This should prevent the device from being used, allowing drivers to
> +/ This should prevent the device from being used, allowing drivers to
>   * recover.  You may need to grab appropriate locks to flush.
>   */
>  void virtio_break_device(struct virtio_device *dev)

How was this tested? Does this even compile? Pls include testing info
in future postings.

-- 
MST

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

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

[Feng Liu via Virtualization]

On 2023-03-08 a.m.12:58, Jason Wang wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
>>
>> Add const to make the read-only pointer parameters clear, similar to
>> many existing functions.
>>
>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>> Reviewed-by: Gavin Li <gavinl@nvidia.com>
>> Reviewed-by: Bodong Wang <bodong@nvidia.com>
>> ---
>>   drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
>>   include/linux/virtio.h       | 12 ++++++------
>>   2 files changed, 18 insertions(+), 19 deletions(-)
>>
> 
> [...]
> 
>>
>> -/*
>> - * This should prevent the device from being used, allowing drivers to
>> +/ This should prevent the device from being used, allowing drivers to
>>    * recover.  You may need to grab appropriate locks to flush.
>>    */
> 
> Any reason for this change?
> 
Hi, Jason
The original comment of the code had a syntax problem and couldn't 
compile, I fixed it here

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

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

[Feng Liu via Virtualization]

On 2023-03-08 a.m.9:07, Feng Liu via Virtualization wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2023-03-08 a.m.12:58, Jason Wang wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
>>>
>>> Add const to make the read-only pointer parameters clear, similar to
>>> many existing functions.
>>>
>>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>> Reviewed-by: Gavin Li <gavinl@nvidia.com>
>>> Reviewed-by: Bodong Wang <bodong@nvidia.com>
>>> ---
>>>   drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
>>>   include/linux/virtio.h       | 12 ++++++------
>>>   2 files changed, 18 insertions(+), 19 deletions(-)
>>>
>>
>> [...]
>>
>>>
>>> -/*
>>> - * This should prevent the device from being used, allowing drivers to
>>> +/ This should prevent the device from being used, allowing drivers to
>>>    * recover.  You may need to grab appropriate locks to flush.
>>>    */
>>
>> Any reason for this change?
>>
> Hi, Jason
> The original comment of the code had a syntax problem and couldn't
> compile, I fixed it here
> 
Sorry, there is a mistake, I will fixed it

>> Thanks
>>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linuxfoundation.org%2Fmailman%2Flistinfo%2Fvirtualization&data=05%7C01%7Cfeliu%40nvidia.com%7C2d406388e866407a3a6408db1fde8819%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638138812857391646%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=oXojhNtxM4SKAzznawQSIGU11XqbhVCCUFB6DXJooIQ%3D&reserved=0
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote:
> On 2023-03-07 04:14, David Edmondson wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
> > writes:
> > 
> >> Add const to make the read-only pointer parameters clear, similar to
> >> many existing functions.
> > 
> > In many of the modified functions the local variable that is a cast of
> > the argument could also be const. Is there a reason not to do both at
> > the same time?
> > 
> 
> Hi,David
> 
> In order to prevent the content of a pointer parameter from being 
> modified and increase the readability of the function, it is recommended 
> to add the 'const' keyword to the parameter. This is not necessary for 
> local variables and non-pointer parameters, as they are only stored on 
> the stack and do not affect the original value or structure member 
> passed into the function. Therefore, in this case, the 'const' keyword 
> is only added to pointer parameters.

This makes no sense to me. If ytou cast away the const then it is
pointless.

> 
> >> Signed-off-by: Feng Liu <feliu@nvidia.com>
> >> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> >> Reviewed-by: Parav Pandit <parav@nvidia.com>
> >> Reviewed-by: Gavin Li <gavinl@nvidia.com>
> >> Reviewed-by: Bodong Wang <bodong@nvidia.com>
> >> ---
> >>   drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
> >>   include/linux/virtio.h       | 12 ++++++------
> >>   2 files changed, 18 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> >> index 806cc44eae64..8010fd1d2047 100644
> >> --- a/drivers/virtio/virtio_ring.c
> >> +++ b/drivers/virtio/virtio_ring.c
> >> @@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq);
> >>
> >>   #define to_vvq(_vq) container_of(_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,7 +2786,7 @@ 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);
> >> @@ -2819,7 +2819,7 @@ 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);
> >>
> >> @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
> >>   }
> >>   EXPORT_SYMBOL_GPL(virtqueue_is_broken);
> >>
> >> -/*
> >> - * This should prevent the device from being used, allowing drivers to
> >> +/ This should prevent the device from being used, allowing drivers to
> >>    * recover.  You may need to grab appropriate locks to flush.
> >>    */
> >>   void virtio_break_device(struct virtio_device *dev)
> >> @@ -2881,7 +2880,7 @@ 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);
> >>
> >> @@ -2895,7 +2894,7 @@ 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);
> >>
> >> @@ -2910,7 +2909,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..36a79374e735 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));
> >> --
> >> 2.34.1
> >>
> >> _______________________________________________
> >> Virtualization mailing list
> >> Virtualization@lists.linux-foundation.org
> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linuxfoundation.org%2Fmailman%2Flistinfo%2Fvirtualization&data=05%7C01%7Cfeliu%40nvidia.com%7C6265cc6ac03e484167ba08db1eec5428%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638137772595544505%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=74eHEHUMF89nTG6hrrdMpkl43DJWvC9xCjwqNOny%2FQE%3D&reserved=0
> > --
> > Woke up in my clothes again this morning, don't know exactly where I am.
> 

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

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

[Michael S. Tsirkin]

On Wed, Mar 08, 2023 at 09:07:49AM -0500, Feng Liu wrote:
> 
> 
> On 2023-03-08 a.m.12:58, Jason Wang wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
> > > 
> > > Add const to make the read-only pointer parameters clear, similar to
> > > many existing functions.
> > > 
> > > Signed-off-by: Feng Liu <feliu@nvidia.com>
> > > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > Reviewed-by: Gavin Li <gavinl@nvidia.com>
> > > Reviewed-by: Bodong Wang <bodong@nvidia.com>
> > > ---
> > >   drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
> > >   include/linux/virtio.h       | 12 ++++++------
> > >   2 files changed, 18 insertions(+), 19 deletions(-)
> > > 
> > 
> > [...]
> > 
> > > 
> > > -/*
> > > - * This should prevent the device from being used, allowing drivers to
> > > +/ This should prevent the device from being used, allowing drivers to
> > >    * recover.  You may need to grab appropriate locks to flush.
> > >    */
> > 
> > Any reason for this change?
> > 
> Hi, Jason
> The original comment of the code had a syntax problem and couldn't compile,
> I fixed it here

This is how it looked before your patch:

/*
 * This should prevent the device from being used, allowing drivers to
 * recover.  You may need to grab appropriate locks to flush.
 */

I see no problem here.


> > Thanks
> > 

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

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

[Feng Liu via Virtualization]

On 2023-03-08 a.m.9:16, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Mar 08, 2023 at 09:07:49AM -0500, Feng Liu wrote:
>>
>>
>> On 2023-03-08 a.m.12:58, Jason Wang wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
>>>>
>>>> Add const to make the read-only pointer parameters clear, similar to
>>>> many existing functions.
>>>>
>>>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>> Reviewed-by: Gavin Li <gavinl@nvidia.com>
>>>> Reviewed-by: Bodong Wang <bodong@nvidia.com>
>>>> ---
>>>>    drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
>>>>    include/linux/virtio.h       | 12 ++++++------
>>>>    2 files changed, 18 insertions(+), 19 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>>
>>>> -/*
>>>> - * This should prevent the device from being used, allowing drivers to
>>>> +/ This should prevent the device from being used, allowing drivers to
>>>>     * recover.  You may need to grab appropriate locks to flush.
>>>>     */
>>>
>>> Any reason for this change?
>>>
>> Hi, Jason
>> The original comment of the code had a syntax problem and couldn't compile,
>> I fixed it here
> 
> This is how it looked before your patch:
> 
> /*
>   * This should prevent the device from being used, allowing drivers to
>   * recover.  You may need to grab appropriate locks to flush.
>   */
> 
> I see no problem here.
> 
Yes, you are right. I made a mistake here, I will fix it
> 
>>> Thanks
>>>
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] virtio_pci_modern: Remove unnecessary num zero check

[Michael S. Tsirkin]

On Tue, Mar 07, 2023 at 05:57:03AM +0200, Feng Liu wrote:
> is_power_of_2() already performs the zero check. Hence avoid duplicate
> check. While at it, move the query of size check also adjacent to where
> its used for the disabled vq.
> 
> Signed-off-by: Feng Liu <feliu@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Gavin Li <gavinl@nvidia.com>
> Reviewed-by: Bodong Wang <bodong@nvidia.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 9e496e288cfa..3d7144f8f959 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -306,10 +306,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>  		return ERR_PTR(-EINVAL);
>  
>  	/* Check if queue is either not available or already active. */
> -	num = vp_modern_get_queue_size(mdev, index);
> -	if (!num || vp_modern_get_queue_enable(mdev, index))
> +	if (vp_modern_get_queue_enable(mdev, index))
>  		return ERR_PTR(-ENOENT);
>  
> +	num = vp_modern_get_queue_size(mdev, index);
>  	if (!is_power_of_2(num)) {
>  		dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
>  		return ERR_PTR(-EINVAL);


As Jason said the right thing to do is to remove the power of 2
limitation. However please don't just hack it up and post,
test it with a variety of queue sizes and with at least
PAGE_POISONING and as many debugging options as you can
to make sure this is not triggering bugs anywhere.

> -- 
> 2.34.1

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

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

[Michael S. Tsirkin]

On Wed, Mar 08, 2023 at 09:19:38AM -0500, Feng Liu wrote:
> 
> 
> On 2023-03-08 a.m.9:16, Michael S. Tsirkin wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Mar 08, 2023 at 09:07:49AM -0500, Feng Liu wrote:
> > > 
> > > 
> > > On 2023-03-08 a.m.12:58, Jason Wang wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
> > > > > 
> > > > > Add const to make the read-only pointer parameters clear, similar to
> > > > > many existing functions.
> > > > > 
> > > > > Signed-off-by: Feng Liu <feliu@nvidia.com>
> > > > > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > Reviewed-by: Gavin Li <gavinl@nvidia.com>
> > > > > Reviewed-by: Bodong Wang <bodong@nvidia.com>
> > > > > ---
> > > > >    drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
> > > > >    include/linux/virtio.h       | 12 ++++++------
> > > > >    2 files changed, 18 insertions(+), 19 deletions(-)
> > > > > 
> > > > 
> > > > [...]
> > > > 
> > > > > 
> > > > > -/*
> > > > > - * This should prevent the device from being used, allowing drivers to
> > > > > +/ This should prevent the device from being used, allowing drivers to
> > > > >     * recover.  You may need to grab appropriate locks to flush.
> > > > >     */
> > > > 
> > > > Any reason for this change?
> > > > 
> > > Hi, Jason
> > > The original comment of the code had a syntax problem and couldn't compile,
> > > I fixed it here
> > 
> > This is how it looked before your patch:
> > 
> > /*
> >   * This should prevent the device from being used, allowing drivers to
> >   * recover.  You may need to grab appropriate locks to flush.
> >   */
> > 
> > I see no problem here.
> > 
> Yes, you are right. I made a mistake here, I will fix it

Nice but the bigger problem is not the mistake - it is the posting of
untested code. It might be an ok thing to do - as long as you make it
super abundantrly clear that this is what it is, and explain why
you are posting it now and not after testing.

> > 
> > > > Thanks
> > > > 
> > 

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

Re: [PATCH 1/3] virtio_pci_modern: Remove unnecessary num zero check

[Feng Liu via Virtualization]

On 2023-03-08 a.m.9:23, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Mar 07, 2023 at 05:57:03AM +0200, Feng Liu wrote:
>> is_power_of_2() already performs the zero check. Hence avoid duplicate
>> check. While at it, move the query of size check also adjacent to where
>> its used for the disabled vq.
>>
>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>> Reviewed-by: Gavin Li <gavinl@nvidia.com>
>> Reviewed-by: Bodong Wang <bodong@nvidia.com>
>> ---
>>   drivers/virtio/virtio_pci_modern.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>> index 9e496e288cfa..3d7144f8f959 100644
>> --- a/drivers/virtio/virtio_pci_modern.c
>> +++ b/drivers/virtio/virtio_pci_modern.c
>> @@ -306,10 +306,10 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>>                return ERR_PTR(-EINVAL);
>>
>>        /* Check if queue is either not available or already active. */
>> -     num = vp_modern_get_queue_size(mdev, index);
>> -     if (!num || vp_modern_get_queue_enable(mdev, index))
>> +     if (vp_modern_get_queue_enable(mdev, index))
>>                return ERR_PTR(-ENOENT);
>>
>> +     num = vp_modern_get_queue_size(mdev, index);
>>        if (!is_power_of_2(num)) {
>>                dev_warn(&vp_dev->pci_dev->dev, "bad queue size %u", num);
>>                return ERR_PTR(-EINVAL);
> 
> 
> As Jason said the right thing to do is to remove the power of 2
> limitation. However please don't just hack it up and post,
> test it with a variety of queue sizes and with at least
> PAGE_POISONING and as many debugging options as you can
> to make sure this is not triggering bugs anywhere.
> 

Hi Michael and Jason

Thanks a lot for your tips. I'll be working on these things and fixing 
this in the next version of this patch series and add testing info

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

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

[Feng Liu via Virtualization]

On 2023-03-08 a.m.9:28, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Mar 08, 2023 at 09:19:38AM -0500, Feng Liu wrote:
>>
>>
>> On 2023-03-08 a.m.9:16, Michael S. Tsirkin wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Wed, Mar 08, 2023 at 09:07:49AM -0500, Feng Liu wrote:
>>>>
>>>>
>>>> On 2023-03-08 a.m.12:58, Jason Wang wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Tue, Mar 7, 2023 at 11:57 AM Feng Liu <feliu@nvidia.com> wrote:
>>>>>>
>>>>>> Add const to make the read-only pointer parameters clear, similar to
>>>>>> many existing functions.
>>>>>>
>>>>>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>>>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>>>> Reviewed-by: Gavin Li <gavinl@nvidia.com>
>>>>>> Reviewed-by: Bodong Wang <bodong@nvidia.com>
>>>>>> ---
>>>>>>     drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
>>>>>>     include/linux/virtio.h       | 12 ++++++------
>>>>>>     2 files changed, 18 insertions(+), 19 deletions(-)
>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>>
>>>>>> -/*
>>>>>> - * This should prevent the device from being used, allowing drivers to
>>>>>> +/ This should prevent the device from being used, allowing drivers to
>>>>>>      * recover.  You may need to grab appropriate locks to flush.
>>>>>>      */
>>>>>
>>>>> Any reason for this change?
>>>>>
>>>> Hi, Jason
>>>> The original comment of the code had a syntax problem and couldn't compile,
>>>> I fixed it here
>>>
>>> This is how it looked before your patch:
>>>
>>> /*
>>>    * This should prevent the device from being used, allowing drivers to
>>>    * recover.  You may need to grab appropriate locks to flush.
>>>    */
>>>
>>> I see no problem here.
>>>
>> Yes, you are right. I made a mistake here, I will fix it
> 
> Nice but the bigger problem is not the mistake - it is the posting of
> untested code. It might be an ok thing to do - as long as you make it
> super abundantrly clear that this is what it is, and explain why
> you are posting it now and not after testing.
> 

In fact, I compiled and tested locally. I just looked it up and it might 
be that I was missing a “git add” action which caused the problem. 
Before I post the patch in future, I will find a clean kernel source and 
apply my patch for testing instead of on the branch where the code is 
modified, so as to avoid this kind of problem from happening again. 
Very sorry for this problem, I will be careful and pay attention to it later

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

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

[Michael S. Tsirkin]

On Wed, Mar 08, 2023 at 09:40:03AM -0500, Feng Liu wrote:
> In fact, I compiled and tested locally. I just looked it up and it might be
> that I was missing a “git add” action which caused the problem. Before I
> post the patch in future, I will find a clean kernel source and apply my
> patch for testing instead of on the branch where the code is modified, so as
> to avoid this kind of problem from happening again. Very sorry for this
> problem, I will be careful and pay attention to it later

I have a pre-push hook since it was happening to me a lot with pushes:


#!/bin/sh

# An example hook script to verify what is about to be pushed.  Called by "git
# push" after it has checked the remote status, but before anything has been
# pushed.  If this script exits with a non-zero status nothing will be pushed.
#
# This hook is called with the following parameters:
#
# $1 -- Name of the remote to which the push is being done
# $2 -- URL to which the push is being done
#
# If pushing without using a named remote those arguments will be equal.
#
# Information about the commits which are being pushed is supplied as lines to
# the standard input in the form:
#
#   <local ref> <local sha1> <remote ref> <remote sha1>
#
# This sample shows how to prevent push of commits where the log message starts
# with "WIP" (work in progress).

remote="$1"
url="$2"

echo "Pre push hook for remote $url"

#if
#	echo $url |grep ^root@virtlab > /dev/null
#then
#	echo "Lab push no need to check"
#	exit 0
#fi
#
#if
#	echo $url |grep ^/ > /dev/null
#then
#	echo "Local push no need to check"
#	exit 0
#fi

checked=0
HEAD=`git rev-list -1 HEAD`
IFS=' '
while read local_ref local_sha remote_ref remote_sha
do
    if [ $checked = 0 ]
    then
	if [ "$local_sha" = $HEAD ]
	then
		echo "Pushing HEAD to remote. Checking that tree is clean."
		if
			git diff-index --quiet HEAD
		then
			echo -n # No differences
		else
			echo "DIFF in HEAD. Not pushed, stash or -no-verify!"
			exit 1
		fi
		checked=1
	fi
    fi
done

exit 0



Consider sticking this in a post commit hook maybe?

-- 
MST

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

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

[Feng Liu via Virtualization]

On 2023-03-08 a.m.9:47, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Mar 08, 2023 at 09:40:03AM -0500, Feng Liu wrote:
>> In fact, I compiled and tested locally. I just looked it up and it might be
>> that I was missing a “git add” action which caused the problem. Before I
>> post the patch in future, I will find a clean kernel source and apply my
>> patch for testing instead of on the branch where the code is modified, so as
>> to avoid this kind of problem from happening again. Very sorry for this
>> problem, I will be careful and pay attention to it later
> 
> I have a pre-push hook since it was happening to me a lot with pushes:
> 
> 
> #!/bin/sh
> 
> # An example hook script to verify what is about to be pushed.  Called by "git
> # push" after it has checked the remote status, but before anything has been
> # pushed.  If this script exits with a non-zero status nothing will be pushed.
> #
> # This hook is called with the following parameters:
> #
> # $1 -- Name of the remote to which the push is being done
> # $2 -- URL to which the push is being done
> #
> # If pushing without using a named remote those arguments will be equal.
> #
> # Information about the commits which are being pushed is supplied as lines to
> # the standard input in the form:
> #
> #   <local ref> <local sha1> <remote ref> <remote sha1>
> #
> # This sample shows how to prevent push of commits where the log message starts
> # with "WIP" (work in progress).
> 
> remote="$1"
> url="$2"
> 
> echo "Pre push hook for remote $url"
> 
> #if
> #       echo $url |grep ^root@virtlab > /dev/null
> #then
> #       echo "Lab push no need to check"
> #       exit 0
> #fi
> #
> #if
> #       echo $url |grep ^/ > /dev/null
> #then
> #       echo "Local push no need to check"
> #       exit 0
> #fi
> 
> checked=0
> HEAD=`git rev-list -1 HEAD`
> IFS=' '
> while read local_ref local_sha remote_ref remote_sha
> do
>      if [ $checked = 0 ]
>      then
>          if [ "$local_sha" = $HEAD ]
>          then
>                  echo "Pushing HEAD to remote. Checking that tree is clean."
>                  if
>                          git diff-index --quiet HEAD
>                  then
>                          echo -n # No differences
>                  else
>                          echo "DIFF in HEAD. Not pushed, stash or -no-verify!"
>                          exit 1
>                  fi
>                  checked=1
>          fi
>      fi
> done
> 
> exit 0
> 
> 
> 
> Consider sticking this in a post commit hook maybe?
> 

OK,thanks, will do

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

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

[Feng Liu via Virtualization]

On 2023-03-08 a.m.9:13, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote:
>> On 2023-03-07 04:14, David Edmondson wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
>>> writes:
>>>
>>>> Add const to make the read-only pointer parameters clear, similar to
>>>> many existing functions.
>>>
>>> In many of the modified functions the local variable that is a cast of
>>> the argument could also be const. Is there a reason not to do both at
>>> the same time?
>>>
>>
>> Hi,David
>>
>> In order to prevent the content of a pointer parameter from being
>> modified and increase the readability of the function, it is recommended
>> to add the 'const' keyword to the parameter. This is not necessary for
>> local variables and non-pointer parameters, as they are only stored on
>> the stack and do not affect the original value or structure member
>> passed into the function. Therefore, in this case, the 'const' keyword
>> is only added to pointer parameters.
> 
> This makes no sense to me. If ytou cast away the const then it is
> pointless.
> 

Hi, Michael

I really don't quite understand your point of view. Is a local variable 
that needs to be add const? Can you help to point out the specific 
problem/point ?

>>
>>>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>> Reviewed-by: Gavin Li <gavinl@nvidia.com>
>>>> Reviewed-by: Bodong Wang <bodong@nvidia.com>
>>>> ---
>>>>    drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
>>>>    include/linux/virtio.h       | 12 ++++++------
>>>>    2 files changed, 18 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>>> index 806cc44eae64..8010fd1d2047 100644
>>>> --- a/drivers/virtio/virtio_ring.c
>>>> +++ b/drivers/virtio/virtio_ring.c
>>>> @@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq);
>>>>
>>>>    #define to_vvq(_vq) container_of(_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,7 +2786,7 @@ 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);
>>>> @@ -2819,7 +2819,7 @@ 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);
>>>>
>>>> @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(virtqueue_is_broken);
>>>>
>>>> -/*
>>>> - * This should prevent the device from being used, allowing drivers to
>>>> +/ This should prevent the device from being used, allowing drivers to
>>>>     * recover.  You may need to grab appropriate locks to flush.
>>>>     */
>>>>    void virtio_break_device(struct virtio_device *dev)
>>>> @@ -2881,7 +2880,7 @@ 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);
>>>>
>>>> @@ -2895,7 +2894,7 @@ 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);
>>>>
>>>> @@ -2910,7 +2909,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..36a79374e735 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));
>>>> --
>>>> 2.34.1
>>>>
>>>> _______________________________________________
>>>> Virtualization mailing list
>>>> Virtualization@lists.linux-foundation.org
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linuxfoundation.org%2Fmailman%2Flistinfo%2Fvirtualization&data=05%7C01%7Cfeliu%40nvidia.com%7C0f6803f1797f448823ac08db1fdf67e5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638138816610707030%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FxggnD7U8o%2B%2BqcYnHN6nc%2BemGVRU1ia5sA4k%2FRTDD7U%3D&reserved=0
>>> --
>>> Woke up in my clothes again this morning, don't know exactly where I am.
>>
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Wed, Mar 08, 2023 at 10:59:57AM -0500, Feng Liu wrote:
> 
> 
> On 2023-03-08 a.m.9:13, Michael S. Tsirkin wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote:
> > > On 2023-03-07 04:14, David Edmondson wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
> > > > writes:
> > > > 
> > > > > Add const to make the read-only pointer parameters clear, similar to
> > > > > many existing functions.
> > > > 
> > > > In many of the modified functions the local variable that is a cast of
> > > > the argument could also be const. Is there a reason not to do both at
> > > > the same time?
> > > > 
> > > 
> > > Hi,David
> > > 
> > > In order to prevent the content of a pointer parameter from being
> > > modified and increase the readability of the function, it is recommended
> > > to add the 'const' keyword to the parameter. This is not necessary for
> > > local variables and non-pointer parameters, as they are only stored on
> > > the stack and do not affect the original value or structure member
> > > passed into the function. Therefore, in this case, the 'const' keyword
> > > is only added to pointer parameters.
> > 
> > This makes no sense to me. If ytou cast away the const then it is
> > pointless.
> > 
> 
> Hi, Michael
> 
> I really don't quite understand your point of view.
> Is a local variable that needs to be add const? Can you help to point
> out the specific problem/point ?

I just repeated what David said.  Basically most of these functions use
to_vvq which uses container_of which in turn loses const qualifier.
So your change is poinless since rest of code accesses vq through
to_vvq.

What to do? I don't like the idea of to_vvq_const.
So I propose a version of container_of using _Generic
which preserves the const qualifier.


#define container_of(ptr, type, member) \
    ({ \
        const void *__mptr = (ptr); \
        static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
                      __same_type(*(ptr), void),                        \
                      "pointer type mismatch in container_of()");       \
        _Generic((ptr), \
                 typeof(&((const type *)0)->member): \
                 (const type *)(__mptr - offsetof(type, member)), \
                 default: \
                 (type *)(__mptr - offsetof(type, member))); \
    })


I'll hack it up in a day or two and post.




> > > 
> > > > > Signed-off-by: Feng Liu <feliu@nvidia.com>
> > > > > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > Reviewed-by: Gavin Li <gavinl@nvidia.com>
> > > > > Reviewed-by: Bodong Wang <bodong@nvidia.com>
> > > > > ---
> > > > >    drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
> > > > >    include/linux/virtio.h       | 12 ++++++------
> > > > >    2 files changed, 18 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 806cc44eae64..8010fd1d2047 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq);
> > > > > 
> > > > >    #define to_vvq(_vq) container_of(_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,7 +2786,7 @@ 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);
> > > > > @@ -2819,7 +2819,7 @@ 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);
> > > > > 
> > > > > @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
> > > > >    }
> > > > >    EXPORT_SYMBOL_GPL(virtqueue_is_broken);
> > > > > 
> > > > > -/*
> > > > > - * This should prevent the device from being used, allowing drivers to
> > > > > +/ This should prevent the device from being used, allowing drivers to
> > > > >     * recover.  You may need to grab appropriate locks to flush.
> > > > >     */
> > > > >    void virtio_break_device(struct virtio_device *dev)
> > > > > @@ -2881,7 +2880,7 @@ 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);
> > > > > 
> > > > > @@ -2895,7 +2894,7 @@ 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);
> > > > > 
> > > > > @@ -2910,7 +2909,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..36a79374e735 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));
> > > > > --
> > > > > 2.34.1
> > > > > 
> > > > > _______________________________________________
> > > > > Virtualization mailing list
> > > > > Virtualization@lists.linux-foundation.org
> > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linuxfoundation.org%2Fmailman%2Flistinfo%2Fvirtualization&data=05%7C01%7Cfeliu%40nvidia.com%7C0f6803f1797f448823ac08db1fdf67e5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638138816610707030%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FxggnD7U8o%2B%2BqcYnHN6nc%2BemGVRU1ia5sA4k%2FRTDD7U%3D&reserved=0
> > > > --
> > > > Woke up in my clothes again this morning, don't know exactly where I am.
> > > 
> > 

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

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

[Feng Liu via Virtualization]

On 2023-03-08 a.m.11:25, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Mar 08, 2023 at 10:59:57AM -0500, Feng Liu wrote:
>>
>>
>> On 2023-03-08 a.m.9:13, Michael S. Tsirkin wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote:
>>>> On 2023-03-07 04:14, David Edmondson wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
>>>>> writes:
>>>>>
>>>>>> Add const to make the read-only pointer parameters clear, similar to
>>>>>> many existing functions.
>>>>>
>>>>> In many of the modified functions the local variable that is a cast of
>>>>> the argument could also be const. Is there a reason not to do both at
>>>>> the same time?
>>>>>
>>>>
>>>> Hi,David
>>>>
>>>> In order to prevent the content of a pointer parameter from being
>>>> modified and increase the readability of the function, it is recommended
>>>> to add the 'const' keyword to the parameter. This is not necessary for
>>>> local variables and non-pointer parameters, as they are only stored on
>>>> the stack and do not affect the original value or structure member
>>>> passed into the function. Therefore, in this case, the 'const' keyword
>>>> is only added to pointer parameters.
>>>
>>> This makes no sense to me. If ytou cast away the const then it is
>>> pointless.
>>>
>>
>> Hi, Michael
>>
>> I really don't quite understand your point of view.
>> Is a local variable that needs to be add const? Can you help to point
>> out the specific problem/point ?
> 
> I just repeated what David said.  Basically most of these functions use
> to_vvq which uses container_of which in turn loses const qualifier.
> So your change is poinless since rest of code accesses vq through
> to_vvq.
> 
> What to do? I don't like the idea of to_vvq_const.
> So I propose a version of container_of using _Generic
> which preserves the const qualifier.
> 
> 
> #define container_of(ptr, type, member) \
>      ({ \
>          const void *__mptr = (ptr); \
>          static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
>                        __same_type(*(ptr), void),                        \
>                        "pointer type mismatch in container_of()");       \
>          _Generic((ptr), \
>                   typeof(&((const type *)0)->member): \
>                   (const type *)(__mptr - offsetof(type, member)), \
>                   default: \
>                   (type *)(__mptr - offsetof(type, member))); \
>      })
> 
> 
> I'll hack it up in a day or two and post.
> 
> 
Hi, Michael
Ok, I see. Thanks for your explaination. I will push the next version of 
this patch after you post

> 
> 
>>>>
>>>>>> Signed-off-by: Feng Liu <feliu@nvidia.com>
>>>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>>>> Reviewed-by: Gavin Li <gavinl@nvidia.com>
>>>>>> Reviewed-by: Bodong Wang <bodong@nvidia.com>
>>>>>> ---
>>>>>>     drivers/virtio/virtio_ring.c | 25 ++++++++++++-------------
>>>>>>     include/linux/virtio.h       | 12 ++++++------
>>>>>>     2 files changed, 18 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>>>>> index 806cc44eae64..8010fd1d2047 100644
>>>>>> --- a/drivers/virtio/virtio_ring.c
>>>>>> +++ b/drivers/virtio/virtio_ring.c
>>>>>> @@ -233,7 +233,7 @@ static void vring_free(struct virtqueue *_vq);
>>>>>>
>>>>>>     #define to_vvq(_vq) container_of(_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,7 +2786,7 @@ 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);
>>>>>> @@ -2819,7 +2819,7 @@ 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);
>>>>>>
>>>>>> @@ -2827,8 +2827,7 @@ bool virtqueue_is_broken(struct virtqueue *_vq)
>>>>>>     }
>>>>>>     EXPORT_SYMBOL_GPL(virtqueue_is_broken);
>>>>>>
>>>>>> -/*
>>>>>> - * This should prevent the device from being used, allowing drivers to
>>>>>> +/ This should prevent the device from being used, allowing drivers to
>>>>>>      * recover.  You may need to grab appropriate locks to flush.
>>>>>>      */
>>>>>>     void virtio_break_device(struct virtio_device *dev)
>>>>>> @@ -2881,7 +2880,7 @@ 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);
>>>>>>
>>>>>> @@ -2895,7 +2894,7 @@ 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);
>>>>>>
>>>>>> @@ -2910,7 +2909,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..36a79374e735 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));
>>>>>> --
>>>>>> 2.34.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> Virtualization mailing list
>>>>>> Virtualization@lists.linux-foundation.org
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.linuxfoundation.org%2Fmailman%2Flistinfo%2Fvirtualization&data=05%7C01%7Cfeliu%40nvidia.com%7C7dc87cb48ac44bead13808db1ff1d29a%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638138895717817297%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ZT5o%2Fs2TWQT6yP7tUUDnV4ojKgKtUmEzSVpnDTs5PtU%3D&reserved=0
>>>>> --
>>>>> Woke up in my clothes again this morning, don't know exactly where I am.
>>>>
>>>
> 
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Wed, Mar 08, 2023 at 11:26:04AM -0500, Michael S. Tsirkin wrote:
> On Wed, Mar 08, 2023 at 10:59:57AM -0500, Feng Liu wrote:
> > 
> > 
> > On 2023-03-08 a.m.9:13, Michael S. Tsirkin wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote:
> > > > On 2023-03-07 04:14, David Edmondson wrote:
> > > > > External email: Use caution opening links or attachments
> > > > > 
> > > > > 
> > > > > Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
> > > > > writes:
> > > > > 
> > > > > > Add const to make the read-only pointer parameters clear, similar to
> > > > > > many existing functions.
> > > > > 
> > > > > In many of the modified functions the local variable that is a cast of
> > > > > the argument could also be const. Is there a reason not to do both at
> > > > > the same time?
> > > > > 
> > > > 
> > > > Hi,David
> > > > 
> > > > In order to prevent the content of a pointer parameter from being
> > > > modified and increase the readability of the function, it is recommended
> > > > to add the 'const' keyword to the parameter. This is not necessary for
> > > > local variables and non-pointer parameters, as they are only stored on
> > > > the stack and do not affect the original value or structure member
> > > > passed into the function. Therefore, in this case, the 'const' keyword
> > > > is only added to pointer parameters.
> > > 
> > > This makes no sense to me. If ytou cast away the const then it is
> > > pointless.
> > > 
> > 
> > Hi, Michael
> > 
> > I really don't quite understand your point of view.
> > Is a local variable that needs to be add const? Can you help to point
> > out the specific problem/point ?
> 
> I just repeated what David said.  Basically most of these functions use
> to_vvq which uses container_of which in turn loses const qualifier.
> So your change is poinless since rest of code accesses vq through
> to_vvq.
> 
> What to do? I don't like the idea of to_vvq_const.
> So I propose a version of container_of using _Generic
> which preserves the const qualifier.
> 
> 
> #define container_of(ptr, type, member) \
>     ({ \
>         const void *__mptr = (ptr); \
>         static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
>                       __same_type(*(ptr), void),                        \
>                       "pointer type mismatch in container_of()");       \
>         _Generic((ptr), \
>                  typeof(&((const type *)0)->member): \
>                  (const type *)(__mptr - offsetof(type, member)), \
>                  default: \
>                  (type *)(__mptr - offsetof(type, member))); \
>     })
> 
> 
> I'll hack it up in a day or two and post.
> 

Oh wait a second. There's already container_of_const.
So just use it in to_vvq.


-- 
MST

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

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

[Feng Liu via Virtualization]

On 2023-03-08 a.m.11:49, Michael S. Tsirkin wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, Mar 08, 2023 at 11:26:04AM -0500, Michael S. Tsirkin wrote:
>> On Wed, Mar 08, 2023 at 10:59:57AM -0500, Feng Liu wrote:
>>>
>>>
>>> On 2023-03-08 a.m.9:13, Michael S. Tsirkin wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On Tue, Mar 07, 2023 at 09:17:55PM +0000, Feng Liu wrote:
>>>>> On 2023-03-07 04:14, David Edmondson wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> Feng Liu via Virtualization <virtualization@lists.linux-foundation.org>
>>>>>> writes:
>>>>>>
>>>>>>> Add const to make the read-only pointer parameters clear, similar to
>>>>>>> many existing functions.
>>>>>>
>>>>>> In many of the modified functions the local variable that is a cast of
>>>>>> the argument could also be const. Is there a reason not to do both at
>>>>>> the same time?
>>>>>>
>>>>>
>>>>> Hi,David
>>>>>
>>>>> In order to prevent the content of a pointer parameter from being
>>>>> modified and increase the readability of the function, it is recommended
>>>>> to add the 'const' keyword to the parameter. This is not necessary for
>>>>> local variables and non-pointer parameters, as they are only stored on
>>>>> the stack and do not affect the original value or structure member
>>>>> passed into the function. Therefore, in this case, the 'const' keyword
>>>>> is only added to pointer parameters.
>>>>
>>>> This makes no sense to me. If ytou cast away the const then it is
>>>> pointless.
>>>>
>>>
>>> Hi, Michael
>>>
>>> I really don't quite understand your point of view.
>>> Is a local variable that needs to be add const? Can you help to point
>>> out the specific problem/point ?
>>
>> I just repeated what David said.  Basically most of these functions use
>> to_vvq which uses container_of which in turn loses const qualifier.
>> So your change is poinless since rest of code accesses vq through
>> to_vvq.
>>
>> What to do? I don't like the idea of to_vvq_const.
>> So I propose a version of container_of using _Generic
>> which preserves the const qualifier.
>>
>>
>> #define container_of(ptr, type, member) \
>>      ({ \
>>          const void *__mptr = (ptr); \
>>          static_assert(__same_type(*(ptr), ((type *)0)->member) ||       \
>>                        __same_type(*(ptr), void),                        \
>>                        "pointer type mismatch in container_of()");       \
>>          _Generic((ptr), \
>>                   typeof(&((const type *)0)->member): \
>>                   (const type *)(__mptr - offsetof(type, member)), \
>>                   default: \
>>                   (type *)(__mptr - offsetof(type, member))); \
>>      })
>>
>>
>> I'll hack it up in a day or two and post.
>>
> 
> Oh wait a second. There's already container_of_const.
> So just use it in to_vvq.
> 

Hi,Michael
    COOL! Sounds good! Will do that


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

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

[Michael S. Tsirkin]

On Thu, Mar 09, 2023 at 10:24:19AM +0100, Jiri Pirko wrote:
> >Nice but the bigger problem is not the mistake - it is the posting of
> >untested code. It might be an ok thing to do - as long as you make it
> >super abundantrly clear that this is what it is, and explain why
> >you are posting it now and not after testing.
> 
> Isn't there some ci running with posted patches here? That would uncover
> it right away. In netdev world, this is very convenient. Example:
> https://patchwork.kernel.org/project/netdevbpf/list/?series=727865
> Patch won't apply.

It is a best effort thing and is not an excuse to post untested patches
for review. If I see a patch I expect that the author made sure
the patch does what it's supposed to do.

-- 
MST

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