> From: Jason Wang <jasowang@xxxxxxxxxx> > Sent: Monday, September 25, 2023 8:00 AM > > On Fri, Sep 22, 2023 at 8:25 PM Parav Pandit <parav@xxxxxxxxxx> wrote: > > > > > > > From: Jason Gunthorpe <jgg@xxxxxxxxxx> > > > Sent: Friday, September 22, 2023 5:53 PM > > > > > > > > And what's more, using MMIO BAR0 then it can work for legacy. > > > > > > Oh? How? Our team didn't think so. > > > > It does not. It was already discussed. > > The device reset in legacy is not synchronous. > > How do you know this? > Not sure the motivation of same discussion done in the OASIS with you and others in past. Anyways, please find the answer below. About reset, The legacy device specification has not enforced below cited 1.0 driver requirement of 1.0. "The driver SHOULD consider a driver-initiated reset complete when it reads device status as 0." [1] https://ozlabs.org/~rusty/virtio-spec/virtio-0.9.5.pdf > > The drivers do not wait for reset to complete; it was written for the sw > backend. > > Do you see there's a flush after reset in the legacy driver? > Yes. it only flushes the write by reading it. The driver does not get _wait_ for the reset to complete within the device like above. Please see the reset flow of 1.x device as below. In fact the comment of the 1.x device also needs to be updated to indicate that driver need to wait for the device to finish the reset. I will send separate patch for improving this comment of vp_reset() to match the spec. static void vp_reset(struct virtio_device *vdev) { struct virtio_pci_device *vp_dev = to_vp_device(vdev); struct virtio_pci_modern_device *mdev = &vp_dev->mdev; /* 0 status means a reset. */ vp_modern_set_status(mdev, 0); /* After writing 0 to device_status, the driver MUST wait for a read of * device_status to return 0 before reinitializing the device. * This will flush out the status write, and flush in device writes, * including MSI-X interrupts, if any. */ while (vp_modern_get_status(mdev)) msleep(1); /* Flush pending VQ/configuration callbacks. */ vp_synchronize_vectors(vdev); } > static void vp_reset(struct virtio_device *vdev) { > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > /* 0 status means a reset. */ > vp_legacy_set_status(&vp_dev->ldev, 0); > /* 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); > } > > Thanks > > > > > Hence MMIO BAR0 is not the best option in real implementations. > >
