> From: Michael S. Tsirkin <m...@redhat.com>
> Sent: 22 August 2025 03:52 PM
>
> On Fri, Aug 22, 2025 at 12:17:06PM +0300, Parav Pandit wrote:
> > This reverts commit 43bb40c5b926 ("virtio_pci: Support surprise removal of
> virtio pci device").
> >
> > Virtio drivers and PCI devices have never fully supported true
> > surprise (aka hot unplug) removal. Drivers historically continued
> > processing and waiting for pending I/O and even continued synchronous
> > device reset during surprise removal. Devices have also continued
> > completing I/Os, doing DMA and allowing device reset after surprise
> > removal to support such drivers.
> >
> > Supporting it correctly would require a new device capability
>
> If a device is removed, it is removed.
This is how it was implemented and none of the virtio drivers supported it.
So vendors had stepped away from such device implementation.
(not just us).
> Windows drivers supported this since
> forever and it's just a Linux bug that it does not handle all the cases.
Internal tests showed the opposite that hotplug driver + virtio didn't handle
the surprise removal either.
Can you please share the starting version of the Windows driver that actually
does not wait for the CVQ commands and outstanding requests for block etc
drivers?
We have even seen window driver that corrupts the CVQ command buffer when CVQ
command times out!
> This is not
> something you can handle with a capability.
>
The sad truth in my view is that capability is needed so that device can know
how to do _sane_ stop as driver told exactly to do so.
For sure if device stops the DMA, the device is unusable until kernel 6.20 or
whatever new kernel arrives.
>
>
>
>
> > and
> > driver negotiation in the virtio specification to safely stop I/O and
> > free queue memory. Failure to do so either breaks all the existing
> > drivers with call trace listed in the commit or crashes the host on
> > continuing the DMA.
>
> If the device is gone, then DMA does not continue.
>
This is exactly how we built the device.
And device behaved correctly, it resulted in the call trace on multiple driver
hands in net, block area.
Commit 43bb40c5b926 partially fixed it.
However, device does not know when to behave correctly vs behave as driver
expects.
And to do this reliability a capability is needed.
> IIUC what is going on for you, is that you have developed a surprise removal
> emulation that pretends to remove the device but actually the device is doing
> DMA. So of course things break then.
>
Not really. The device was built correctly to do surprise removal and stop the
DMA.
That caused the call traces. One of them listed in 43bb40c5b926.
So to maintain backward compatibility, device had to graceful removal as
expected by these drivers.
> > Hence, until such specification and devices are invented, restore the
> > previous behavior of treating surprise removal as graceful removal to
> > avoid regressions and maintain system stability same as before the
> > commit 43bb40c5b926 ("virtio_pci: Support surprise removal of virtio pci
> device").
> >
> > As explained above, previous analysis of solving this only in driver
> > was incomplete and non-reliable at [1] and at [2]; Hence reverting
> > commit
> > 43bb40c5b926 ("virtio_pci: Support surprise removal of virtio pci
> > device") is still the best stand to restore failures of virtio net and
> > block devices.
> >
> > [1]
> >
> https://lore.kernel.org/virtualization/CY8PR12MB719506CC5613EB100BC6C6
> > 38dc...@cy8pr12mb7195.namprd12.prod.outlook.com/#t
>
>
> I can only repeat what I said then, this is not how we do kernel development.
>
Kernel development does not promote breaking users.
Here users are affected with this incomplete kernel behaviour.
Virtio subsystem (multiple drivers) including marked stable kernel lacks the
contract on when to expect DMA completions and when not.
> > [2]
> > https://lore.kernel.org/virtualization/20250602024358.57114-1-parav@nv
> > idia.com/
>
> What was missing here, is handling corner cases. So let us please try to
> handle
> them.
>
> Here is how I would try to do it:
>
> - add a new driver callback
> - start a periodic timer task in virtio core on remove
> - in the timer, probe that the device is still present.
> if not, invoke a driver callback
> - cancel the task on device reset
>
Multiple users request to restore the stability before adding the surprise
removal support in reliable way.
Lets be practical.
Each net, blk, fs, console, crypto has different way of flushing the pending
requests.
It is unlikely to find a single stable kernel where all the virtio devices
would have support for above.
So until above grant work is added to new kernel, user deserves to have
stability restored in stable kernels.
Hence this request to revert multiple times from the users.
> If you do not have the time, let me know and I will try to look into it.
>
> > Fixes: 43bb40c5b926 ("virtio_pci: Support surprise removal of virtio
> > pci device")
> > Cc: sta...@vger.kernel.org
> > Reported-by: lirongq...@baidu.com
> > Closes:
> > https://lore.kernel.org/virtualization/c45dd68698cd47238c55fb73ca9b474
> > 1...@baidu.com/
> > Signed-off-by: Parav Pandit <pa...@nvidia.com>
> > ---
> > drivers/virtio/virtio_pci_common.c | 7 -------
> > 1 file changed, 7 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c
> > b/drivers/virtio/virtio_pci_common.c
> > index d6d79af44569..dba5eb2eaff9 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -747,13 +747,6 @@ 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.26.2
