RE: [PATCH 4/4] virtio_pci: Support surprise removal of virtio pci device

[Parav Pandit via Virtualization <virtualization@lists.linux-foundation.org>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Sunday, July 18, 2021 2:17 AM
> 
> On Sat, Jul 17, 2021 at 10:42:58AM +0300, Parav Pandit wrote:
> > When a virtio pci device undergo surprise removal (aka async removaln
> > in
> 
> typo
Fixing it. Checkpatch.pl and codespell, both didn't catch it. 😊

> 
> OK that's nice, but I suspect this is not enough.
> For example we need to also fix up all config space reads to handle all-ones
> patterns specially.
> 
> E.g.
> 
>         /* After writing 0 to device_status, the driver MUST wait for a read of
>          * device_status to return 0 before reinitializing the device.
>          * This will flush out the status write, and flush in device writes,
>          * including MSI-X interrupts, if any.
>          */
>         while (vp_modern_get_status(mdev))
>                 msleep(1);
> 
> lots of code in drivers needs to be fixed too.
Above one particularly known to us in the hot plug area.
Above fix is needed to close the race of hot plug and unplug happening in parallel, which is occurring today, but less common.
It is in my todo list to fix it.
Will take care of it in near future in other series.

> 
> I guess we need to annotate drivers somehow to mark up whether they
> support surprise removal? And maybe find a way to let host know?
What is the benefit of it? Who can make use of that information?

In virtio pci case, it is similar improvement to what nvme pci driver went through few years back to support hot plug/unplug.
Lets complete this of fixes to make it little more robust like nvme.

> 
> 
> 
> > ---
> >  drivers/virtio/virtio_pci_common.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c
> > b/drivers/virtio/virtio_pci_common.c
> > index 222d630c41fc..b35bb2d57f62 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -576,6 +576,13 @@ static void virtio_pci_remove(struct pci_dev
> *pci_dev)
> >  	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> >  	struct device *dev = get_device(&vp_dev->vdev.dev);
> >
> > +	/*
> > +	 * Device is marked broken on surprise removal so that virtio upper
> > +	 * layers can abort any ongoing operation.
> > +	 */
> > +	if (!pci_device_is_present(pci_dev))
> > +		virtio_break_device(&vp_dev->vdev);
> > +
> >  	pci_disable_sriov(pci_dev);
> >
> >  	unregister_virtio_device(&vp_dev->vdev);
> > --
> > 2.27.0

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