Re: [PATCH 2/2] virtio_pci_modern: avoid infinite loop in vp_reset() on invalid status

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Aug 02, 2026 at 05:40:59PM +0000, Abhin Parekadan Jose wrote:
> vp_reset() polls device_status in a tight loop, waiting for it to read
> back as 0 after the reset write. device_status is read via MMIO from
> the common configuration structure, which requires the PCI_COMMAND
> Memory Space Enable bit to be set. If that bit is cleared while the
> device is bound -- e.g. by writing 0x0000 to PCI_COMMAND (config space
> offset 4)

So don't do it?

> -- the MMIO read no longer reaches the device and returns
> the bus's synthesized all-ones response instead. Since that value can
> never legitimately clear to 0, the loop spins forever and hangs the
> caller.
> 
> Use VIRTIO_STATUS_ERROR() to recognize such values and bail out of the
> poll loop instead of looping indefinitely.

If you want to work on suprise removal, that is great, but
with actual surprise removal testing, please. I'm not
inclined to include changes when testing amounted
to illegally poking at pci command, and without much in the way
of what effect this has on the drivers.

In particular, please read cover.1752094439.git.mst@redhat.com - a thread
where we seem to have come to the conclusion that hangs where
surprise removal happens while the remove callback is in progress
are fundamentally unfixable without pci (and likely acpi) core
changes.

> 
> Signed-off-by: Abhin Parekadan Jose <abhinjoses@gmail.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 6d8ae2a6a8ca..209fa3b36c90 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -547,6 +547,7 @@ static void vp_reset(struct virtio_device *vdev)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>  	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
> +	u8 status;
>  
>  	/* 0 status means a reset. */
>  	vp_modern_set_status(mdev, 0);
> @@ -555,8 +556,11 @@ static void vp_reset(struct virtio_device *vdev)
>  	 * This will flush out the status write, and flush in device writes,
>  	 * including MSI-X interrupts, if any.
>  	 */
> -	while (vp_modern_get_status(mdev))
> +	while ((status = vp_modern_get_status(mdev))) {
> +		if (VIRTIO_STATUS_ERROR(status))
> +			break;
>  		msleep(1);
> +	}


I am not convinced we'll never use all status bits eventually.
Currently a single bit (32, bit 5) is unused.

And then this test will give false positives.

>  
>  	vp_modern_avq_cleanup(vdev);
>  
> -- 
> 2.51.1