> From: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Sent: 25 June 2025 06:09 PM
>
> On Tue, Jun 24, 2025 at 02:56:22PM -0400, Stefan Hajnoczi wrote:
> > On Mon, Jun 02, 2025 at 02:44:33AM +0000, Parav Pandit wrote:
> > > When the PCI device is surprise removed, requests may not complete
> > > the device as the VQ is marked as broken. Due to this, the disk
> > > deletion hangs.
> >
> > There are loops in the core virtio driver code that expect device
> > register reads to eventually return 0:
> > drivers/virtio/virtio_pci_modern.c:vp_reset()
> > drivers/virtio/virtio_pci_modern_dev.c:vp_modern_set_queue_reset()
> >
> > Is there a hang if these loops are hit when a device has been surprise
> > removed? I'm trying to understand whether surprise removal is fully
> > supported or whether this patch is one step in that direction.
> >
> > Apart from that, I'm happy with the virtio_blk.c aspects of the patch:
> > Reviewed-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
>
> Is this as simple as this?
>
Didn't audit the code where else the changes needed.
I had similar change a while ago, but I also recall hitting an assert in the pci layer somewhere during the vp_reset() flow.
Do not have lab access today. Will check back tomorrow and update.
> -->
>
>
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>
> ---
>
> diff --git a/drivers/virtio/virtio_pci_modern.c
> b/drivers/virtio/virtio_pci_modern.c
> index 7182f43ed055..df983fa9046a 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -555,8 +555,12 @@ 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 (vp_modern_get_status(mdev)) {
> + /* If device is removed meanwhile, it will never respond. */
> + if (!pci_device_is_present(vp_dev->pci_dev))
> + break;
> msleep(1);
> + }
>
> vp_modern_avq_cleanup(vdev);
>
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c
> b/drivers/virtio/virtio_pci_modern_dev.c
> index 0d3dbfaf4b23..7177ce0d63be 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -523,11 +523,19 @@ void vp_modern_set_queue_reset(struct
> virtio_pci_modern_device *mdev, u16 index)
> vp_iowrite16(index, &cfg->cfg.queue_select);
> vp_iowrite16(1, &cfg->queue_reset);
>
> - while (vp_ioread16(&cfg->queue_reset))
> + while (vp_ioread16(&cfg->queue_reset)) {
> + /* If device is removed meanwhile, it will never respond. */
> + if (!pci_device_is_present(vp_dev->pci_dev))
> + break;
> msleep(1);
> + }
>
> - while (vp_ioread16(&cfg->cfg.queue_enable))
> + while (vp_ioread16(&cfg->cfg.queue_enable)) {
> + /* If device is removed meanwhile, it will never respond. */
> + if (!pci_device_is_present(vp_dev->pci_dev))
> + break;
> msleep(1);
> + }
> }
> EXPORT_SYMBOL_GPL(vp_modern_set_queue_reset);
>
