[PATCH master] virtio: pci: don't re-enable with 0 as argument

[PATCH master] virtio: pci: don't re-enable with 0 as argument

[Ahmad Fatoum]

Qemu prints an error during barebox shutdown when virtio-net was used:

  qemu-system-aarch64: wrong value for queue_enable 0

This warning was added a few years back in Qemu commit 10d35e5819:

 | virtio-pci: fix queue_enable write
 |
 | Spec said: The driver uses this to selectively prevent the device from
 | executing requests from this virtqueue. 1 - enabled; 0 - disabled.
 |
 | Though write 0 to queue_enable is forbidden by the spec, we should not
 | assume that the value is 1.
 |
 | Fix this by ignore the write value other than 1.
 |
 | Signed-off-by: Jason Wang <jasowang@redhat.com>
 | Message-Id: <20200610054351.15811-1-jasowang@redhat.com>
 | Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
 | Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
 | Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
 | Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
 | Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
 | Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Even older Qemu versions actually marked the virtqueue enabled when
queue_enable was written with any value, we should really stop writing
anything, but 1 into queue_enable in the removal path.

We already reset before deleting the virtqueues, which disables them.
This aligns us with what Linux is doing, except that Linux has some
MSIX cleanup logic in virtio_pci_del_vq. We don't have that, but we
will keep the function anyway to simplify future synchronization.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/virtio/virtio_pci_modern.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 26eefba85bea..2dd369b02e9a 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -223,14 +223,6 @@ static struct virtqueue *virtio_pci_setup_vq(struct virtio_device *vdev,
 
 static void virtio_pci_del_vq(struct virtqueue *vq)
 {
-	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
-	unsigned int index = vq->index;
-
-	iowrite16(index, &vp_dev->common->queue_select);
-
-	/* Select and deactivate the queue */
-	iowrite16(0, &vp_dev->common->queue_enable);
-
 	vring_del_virtqueue(vq);
 }
 
-- 
2.39.2

Re: [PATCH master] virtio: pci: don't re-enable with 0 as argument

[Sascha Hauer]

On Wed, Jan 03, 2024 at 11:13:54AM +0100, Ahmad Fatoum wrote:
> Qemu prints an error during barebox shutdown when virtio-net was used:
> 
>   qemu-system-aarch64: wrong value for queue_enable 0
> 
> This warning was added a few years back in Qemu commit 10d35e5819:
> 
>  | virtio-pci: fix queue_enable write
>  |
>  | Spec said: The driver uses this to selectively prevent the device from
>  | executing requests from this virtqueue. 1 - enabled; 0 - disabled.
>  |
>  | Though write 0 to queue_enable is forbidden by the spec, we should not
>  | assume that the value is 1.
>  |
>  | Fix this by ignore the write value other than 1.
>  |
>  | Signed-off-by: Jason Wang <jasowang@redhat.com>
>  | Message-Id: <20200610054351.15811-1-jasowang@redhat.com>
>  | Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>  | Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>  | Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>  | Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>  | Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>  | Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Even older Qemu versions actually marked the virtqueue enabled when
> queue_enable was written with any value, we should really stop writing
> anything, but 1 into queue_enable in the removal path.
> 
> We already reset before deleting the virtqueues, which disables them.
> This aligns us with what Linux is doing, except that Linux has some
> MSIX cleanup logic in virtio_pci_del_vq. We don't have that, but we
> will keep the function anyway to simplify future synchronization.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/virtio/virtio_pci_modern.c | 8 --------
>  1 file changed, 8 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 26eefba85bea..2dd369b02e9a 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -223,14 +223,6 @@ static struct virtqueue *virtio_pci_setup_vq(struct virtio_device *vdev,
>  
>  static void virtio_pci_del_vq(struct virtqueue *vq)
>  {
> -	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> -	unsigned int index = vq->index;
> -
> -	iowrite16(index, &vp_dev->common->queue_select);
> -
> -	/* Select and deactivate the queue */
> -	iowrite16(0, &vp_dev->common->queue_enable);
> -
>  	vring_del_virtqueue(vq);
>  }
>  
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |