Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Removal of callback synchronization from `vp_reset()` introduces a race condition during system shutdown, as `virtio_device_shutdown()` fails to synchronize callbacks after device reset. --
commit a9d5f056fe92ddf169045b3557dd34b627d1eb0b Author: Michael S. Tsirkin <[email protected]> virtio_pci: drop callback sync on reset The virtio core now synchronizes callbacks after reset, so the PCI transports no longer need to do it themselves. > diff --git a/drivers/virtio/virtio_pci_legacy.c > b/drivers/virtio/virtio_pci_legacy.c > index d9cbb02b35a11..8115aa39e01e9 100644 > --- a/drivers/virtio/virtio_pci_legacy.c > +++ b/drivers/virtio/virtio_pci_legacy.c > @@ -98,8 +98,6 @@ static void vp_reset(struct virtio_device *vdev) > /* Flush out the status write, and flush in device writes, > * including MSi-X interrupts, if any. */ > vp_legacy_get_status(&vp_dev->ldev); > - /* Flush pending VQ/configuration callbacks. */ > - vp_synchronize_vectors(vdev); [Severity: High] Does removing this synchronization introduce a race condition during system shutdown? During system shutdown, virtio_device_shutdown() in drivers/virtio/virtio.c calls dev->config->reset(dev) directly without calling virtio_reset_device(): drivers/virtio/virtio.c:virtio_device_shutdown() { ... dev->config->reset(dev); /* Missing virtio_synchronize_cbs(dev) here */ } The vp_reset() implementation flushes pending MSI-X interrupts, which can cause a configuration interrupt to fire. Because this patch removes the internal synchronization from vp_reset(), virtio_device_shutdown() returns immediately. The configuration interrupt (vp_config_changed()) could then run concurrently on another CPU, invoking driver callbacks while the system is tearing down the IOMMU or jumping into a kexec kernel. > } > > static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) > diff --git a/drivers/virtio/virtio_pci_modern.c > b/drivers/virtio/virtio_pci_modern.c > index b4249afd7f580..922e2df027de9 100644 > --- a/drivers/virtio/virtio_pci_modern.c > +++ b/drivers/virtio/virtio_pci_modern.c [ ... ] > @@ -563,9 +563,6 @@ static void vp_reset(struct virtio_device *vdev) > */ > while (vp_modern_get_status(mdev)) > msleep(1); > - > - /* Flush pending VQ/configuration callbacks. */ > - vp_synchronize_vectors(vdev); [Severity: High] Does removing the synchronization here present the same issue? This breaks the guarantee that interrupts flushed during the reset loop have finished before returning, which virtio_device_shutdown() relies upon to safely stop the device. > } > > static int vp_active_vq(struct virtqueue *vq, u16 msix_vec) -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
