> From: Michael S. Tsirkin <mst@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> > > 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.
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?
> > > 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.
