Re: [PATCH] Revert "virtio_pci: Support surprise removal of virtio pci device"

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

On Thu, Aug 28, 2025 at 06:59:26AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: 28 August 2025 12:04 PM
> > 
> > On Thu, Aug 28, 2025 at 06:23:02AM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: 27 August 2025 04:19 PM
> > > >
> > > > On Wed, Aug 27, 2025 at 06:21:28AM -0400, Michael S. Tsirkin wrote:
> > > > > On Tue, Aug 26, 2025 at 06:52:11PM +0000, Parav Pandit wrote:
> > > > > > > > > If it does not, and a user pull out the working device,
> > > > > > > > > how does your patch help?
> > > > > > > > >
> > > > > > > > A driver must tell that it will not follow broken ancient
> > > > > > > > behaviour and at that
> > > > > > > point device would stop its ancient backward compatibility mode.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > I don't know what is "ancient backward compatibility mode".
> > > > > > >
> > > > > > Let me explain.
> > > > > > Sadly, CSPs virtio pci device implementation is done such a way
> > > > > > that, it
> > > > works with ancient Linux kernel which does not have commit
> > > > 43bb40c5b9265.
> > > > >
> > > > >
> > > > > OK we are getting new information here.
> > > > >
> > > > > So let me summarize. There's a virtual system that pretends, to
> > > > > the guest, that device was removed by surprise removal, but
> > > > > actually device is there and is still doing DMA.
> > > > > Is that a fair summary?
> > > >
> > > Yes.
> > >
> > > > If that is the case, the thing to do would be to try and detect the
> > > > fake removal and then work with device as usual - device not doing
> > > > DMA after removal is pretty fundamental, after all.
> > > >
> > > The issue is: one can build the device to stop the DMA.
> > > There is no predictable combination for the driver and device that can work
> > for the user.
> > > For example,
> > > Device that stops the dma will not work before the commit 43bb40c5b9265.
> > > Device that continues the dma will not work with whatever new
> > implementation done in future kernels.
> > >
> > > Hence the capability negotiation would be needed so that device can stop the
> > DMA, config interrupts etc.
> > 
> > So this is a broken implementation at the pci level. We really can't fix removal
> > for this device at all, except by fixing the device. 
> The device to be told how to behave with/without commit 43bb40c5b9265.
> Not sure what you mean by 'fix the device'.
> 
> Users are running stable kernel that has commit 43bb40c5b9265 and its broken setup for them.
> 
> > Whatever works, works by
> > chance.  Feature negotiation in spec is not the way to fix that, but some work
> > arounds in the driver to skip the device are acceptable, mostly to not bother
> > with it.
> >
> Why not?
> It sounds like we need feature bit like VERSION_1 or ORDER_PLATFORM.


Because the device is out of spec (PCI spec which virtio references).

Besides the bug is not in the device, it's in the pci emulation.


> To _fix_ a stable kernel, if you have a suggestion, please suggest.
> 
> > Pls document exactly how this pci looks. Does it have an id we can use to detect
> > it?
> >
> CSPs have different device and vendor id for vnet, blk vfs.
> Is that what you mean by id?

vendor id is one way, yes. maybe a revision check, too.

> > > > For example, how about reading device control+status?
> > > >
> > > Most platforms read 0xffff on non-existing device, but not sure if this the
> > standard or well defined.
> > 
> > IIRC it's in the pci spec as a note.
> > 
> Checking.
> 
> > > > If we get all ones device has been removed If we get 0 in bus
> > > > master: device has been removed but re-inserted Anything else is a
> > > > fake removal
> > > >
> > > Bus master check may pass, right returning all 1s, even if the device is
> > removed, isn't it?
> > 
> > 
> > So we check all ones 1st, only check bus master if not all ones?
> >
> Pci subsystem typically checks the vendor and device ids, and if its not all 1s, its safe enough check.
> 
> How about a fix something like this:
> 
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -746,12 +746,16 @@ 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);
> +       u32 v;
> 
>         /*
>          * Device is marked broken on surprise removal so that virtio upper
>          * layers can abort any ongoing operation.
> +        * Make sure that device is truly removed by directly interacting
> +        * with the device (and not just depend on the slot registers).
>          */
> -       if (!pci_device_is_present(pci_dev))
> +       if (!pci_device_is_present(pci_dev) &&
> +           !pci_bus_read_dev_vendor_id(pci_dev->bus, pci_dev->devfn, &v, 0))
>                 virtio_break_device(&vp_dev->vdev);
> 
> So if the device is still there, it let it go through its usual cleanup flow.
> And post this fix, a proper implementation with callback etc that you described can be implemented.


I don't have a big problem with this, but I don't understand the
scenario now again. report_error_detected relies on dev->error_state and
bus read. error_state is set on AER reporting an error. This is
not what you described.

Does the patch actually solve the problem for you?

Also can we limit this to a specific vendor id, or something like that?


I also still like the idea of reading dev control and status, since
it always bothered me that there's a theoretical chance that device
is re-inserted and bus read will succeed. Or maybe I'm imagining it.


-- 
MST