> From: Michael S. Tsirkin <m...@redhat.com>
> Sent: 28 August 2025 02:53 PM
>
> 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.
>
Vendor and device id are as defined in virtio spec as ID 0x1AF4 and respective
device 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.
>
>
> 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.
>
When pci device is virtually removed from the slot error_state is updated using,
pci_dev_set_disconnected()
pci_dev_set_io_state()
> Does the patch actually solve the problem for you?
>
It should. I am going to check if this approach looks fine to you.
Please let me know.
> Also can we limit this to a specific vendor id, or something like that?
>
Its spec defined 0x1AF4.
>
> 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.
>
Re-insertion cannot happen in same slot until the previous slot is properly
cleaned up and bus number is not released.
User may still attempt to plug in in same (virtual) or physical slot, but it
will get different bus assigned as the previous one is not recycled yet because
cleanup didnt finish yet.
>
> --
> MST
