Linux Virtualization - Mar 2023 - [PATCH 1/3] virtio_pci_modern: Remove unnecessary num zero check

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 at nvidia.com>
Reviewed-by: Jiri Pirko <jiri at nvidia.com>
Reviewed-by: Parav Pandit <parav at nvidia.com>
Reviewed-by: Gavin Li <gavinl at nvidia.com>
Reviewed-by: Bodong Wang <bodong at 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

Feng Liu via Virtualization <virtualization at 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 at nvidia.com>
> Reviewed-by: Jiri Pirko <jiri at nvidia.com>
> Reviewed-by: Parav Pandit <parav at nvidia.com>
> Reviewed-by: Gavin Li <gavinl at nvidia.com>
> Reviewed-by: Bodong Wang <bodong at nvidia.com>
Reviewed-by: David Edmondson <david.edmondson at 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 at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
-- 
It's funny, I spent my whole life wanting to be talked about.

On Tue, Mar 7, 2023 at 11:57?AM Feng Liu <feliu at 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 at nvidia.com>
> Reviewed-by: Jiri Pirko <jiri at nvidia.com>
> Reviewed-by: Parav Pandit <parav at nvidia.com>
> Reviewed-by: Gavin Li <gavinl at nvidia.com>
> Reviewed-by: Bodong Wang <bodong at 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
>

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 at nvidia.com>
> Reviewed-by: Jiri Pirko <jiri at nvidia.com>
> Reviewed-by: Parav Pandit <parav at nvidia.com>
> Reviewed-by: Gavin Li <gavinl at nvidia.com>
> Reviewed-by: Bodong Wang <bodong at 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