Virtualization - Sep 2023 - [PATCH vfio 11/11] vfio/virtio: Introduce a vfio driver over virtio devices

On Fri, Sep 22, 2023 at 8:25?PM Parav Pandit <parav at nvidia.com>
wrote:
>
>
> > From: Jason Gunthorpe <jgg at nvidia.com>
> > 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?
> 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?

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.
>

> From: Jason Wang <jasowang at redhat.com>
> Sent: Monday, September 25, 2023 8:00 AM
> 
> On Fri, Sep 22, 2023 at 8:25?PM Parav Pandit <parav at nvidia.com>
wrote:
> >
> >
> > > From: Jason Gunthorpe <jgg at nvidia.com>
> > > 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.
> >