Re: [PATCH 0/2] virtio_pci_modern: fix vp_reset() hang on unresponsive device

On Sun, Aug 02, 2026 at 09:48:26PM +0200, Abhin Parekadan Jose wrote:
> On Sun, Aug 02, 2026 at 03:08:12PM -0400, Michael S. Tsirkin wrote:
> > On Sun, Aug 02, 2026 at 06:28:03PM +0000, Abhin Parekadan Jose wrote:
> > > On Sun, Aug 02, 2026 at 01:47:01PM -0400, Michael S. Tsirkin wrote:
> > > > On Sun, Aug 02, 2026 at 05:40:57PM +0000, Abhin Parekadan Jose wrote:
> > > > > While investigating a syzbot report of a WARN_ON_ONCE firing in
> > > > > virtio_dev_remove() [1],
> > > >
> > > >
> > > > And I responded to that syzbot report, and I quote:
> > > >
> > > > So it writes 0 into pci command, effectively killing the device,
> > > > and then is unhappy that the driver prints warnings?
> > > > Who thought it's a good idea? Why?
> > > 
> > > I was learning how to reproduce syzbot bugs when I found this
> > > issue by writing 0 to PCI_COMMAND to simulate an unresponsive
> > > device.
> > 
> > Yea I have no idea where does this syzbot "bug report"
> > come from. Poking at random at device registers is ... not
> > a very good idea.
> > 
> > > While doing that I noticed that echo 1 > /sys/../remove
> > > hung completely rather than just printing the warning. Since the
> > > device_status register lives in the virtio common config MMIO
> > > space and has defined values(based on the bits set) in the spec.
> > > I thought it made sense for virtio to detect this and handle it
> > > gracefully rather than spin forever, so I wrote up a small fix
> > > for that.
> > > 
> > > > > I found a related but more serious issue:
> > > > > vp_reset() in the modern virtio-pci transport can hang indefinitely
> > > > > if PCI_COMMAND memory-space decode is disabled while the device is
> > > > > bound (e.g. surprise removal, hardware fault, or -- as reproduced
> > > > > here -- a direct write to the PCI_COMMAND register). The status
> > > > > register poll loop has no way to distinguish "device still resetting"
> > > > > from "device unreachable," so it never terminates.
> > > > >
> > > > > Patch 1 adds a VIRTIO_STATUS_ERROR() check that recognizes an
> > > > > all-ones status read as invalid (per spec, bits 4-5 are reserved and
> > > > > can never legitimately be set) and warns once at the point the bad
> > > > > read actually happens.
> > > > >
> > > > > Patch 2 uses that check to break out of vp_reset()'s poll loop
> > > > > instead of spinning forever.
> > > >
> > > > Was all this including the cover letter written with ai assistance?
> > > > if yes pls disclose this.
> > > 
> > > Yes, I used AI assistance (Claude). The commit messages were written
> > > by me and then refined with AI for spelling and grammar; the cover
> > > letter was generated by Claude and reviewed by me.
> > 
> > I suggest limiting it to fixing spelling and grammar exclusively.  It
> > tends to do things like dramatize, e.g. "more serious issue", like it
> > did here.
> > 
> > > The code, testing,
> > > and debugging were done by me -- I reproduced the hang in QEMU,
> > > debugged to reach the hanging loop, and wrote the actual fix.
> > > 
> > > I should have disclosed this upfront. I'll do so in future
> > > submissions.
> > > 
> > > Do I need to add Assisted-by: Claude <claude-4-6-sonnet> to the
> > > commit messages?
> > 
> >     Assisted-by: Claude:claude-sonnet-4-6
> > 
> > 
> > > 
> > > P.S. This is my first kernel patch set.
> > 
> > 
> > Thanks, keep at it. Bonus points if you find a real fix for
> > issues raised in thread about surprise removal, see e.g. here
> > [email protected]
> > but don't expect it to be easy.
> > 
> 
> That looks interesting (haven't gone through in detail but got a gist of it).
> I'll try it out and make suggestions if I find a good solution.
> 
> As for the current patch set, does it make sense to drop macro
> `VIRTIO_STATUS_ERROR` and use `PCI_POSSIBLE_ERROR` to break out of the loop,
> I could test it by actually doing a surprise removal on qemu via the monitor
> or just drop this patch set

I'd drop this, I'm not interested in working around one source of hangs
if others in the same exact path remain unfixable.

> and try on [email protected] patch set?

it's not a question of "trying it on" it's a question of the fact that
the pci core serializes probe/removal events so a driver inside
probe/remove never sees the removal event. in this instance it
is polling so it can check (at the cost of adding cpu overhead,
mostly for nothing) but in most places it can't, we need the event
to reach it.

> > -- 
> > MST
> >