On Thu, Aug 28, 2025 at 06:59:26AM +0000, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin <m...@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 <m...@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
