Re: [PATCH] virtio_pci: Wait for legacy device to be reset

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 07:17:14AM +0000, Angus Chen wrote:
> 
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > Sent: Tuesday, April 11, 2023 2:56 PM
> > To: Angus Chen <angus.chen@xxxxxxxxxxxxxxx>
> > Cc: Jason Wang <jasowang@xxxxxxxxxx>;
> > virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] virtio_pci: Wait for legacy device to be reset
> > 
> > On Tue, Apr 11, 2023 at 06:49:03AM +0000, Angus Chen wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > > > Sent: Tuesday, April 11, 2023 2:47 PM
> > > > To: Angus Chen <angus.chen@xxxxxxxxxxxxxxx>
> > > > Cc: Jason Wang <jasowang@xxxxxxxxxx>;
> > > > virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > > Subject: Re: [PATCH] virtio_pci: Wait for legacy device to be reset
> > > >
> > > > On Tue, Apr 11, 2023 at 06:36:39AM +0000, Angus Chen wrote:
> > > > > Hi.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Jason Wang <jasowang@xxxxxxxxxx>
> > > > > > Sent: Tuesday, April 11, 2023 1:24 PM
> > > > > > To: Angus Chen <angus.chen@xxxxxxxxxxxxxxx>
> > > > > > Cc: mst@xxxxxxxxxx; virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx;
> > > > > > linux-kernel@xxxxxxxxxxxxxxx
> > > > > > Subject: Re: [PATCH] virtio_pci: Wait for legacy device to be reset
> > > > > >
> > > > > > On Tue, Apr 11, 2023 at 9:39 AM Angus Chen
> > > > <angus.chen@xxxxxxxxxxxxxxx>
> > > > > > wrote:
> > > > > > >
> > > > > > > We read the status of device after reset,
> > > > > > > It is not guaranteed that the device be reseted successfully.
> > > > > > > We can use a while loop to make sure that,like the modern device did.
> > > > > > > The spec is not request it ,but it work.
> > > > > >
> > > > > > The only concern is if it's too late to do this.
> > > > > >
> > > > > > Btw, any reason you want to have a legacy hardware implementation. It
> > > > > > will be very tricky to work correctly.
> > > > >   En,I found this in the real production environment some times about
> > one
> > > > year ago.
> > > > > and I fix this out of tree.our virtio card had been sold about thousands .
> > > > >
> > > > >   Now,we created a new card, it support virtio 0.95,1.0,1.1 etc.
> > > >
> > > > I am not 100% sure what does this mean.
> > > > So it's a transitional device then?
> > > Hi mst,it is a real card in the In cloud computing ,not transitional device.
> > 
> > yes, a real card. But you said it supports 0.95, 1.0 and 1.1 guests,
> > so this is what the virtio spec calls a transitional device, right?
> > 
> > 	To simplify transition from these earlier draft interfaces,
> > 	a device MAY implement:
> > 
> > 	\begin{description}
> > 	\item[Transitional Device]
> > 		a device supporting both drivers conforming to this
> > 		specification, and allowing legacy drivers.
> > 	\end{description}
> > 
> > or did I misunderstand?
> > 
> Yes, I'm not sure whether I make myself clear. 
> We support the vritio spec  0.95, 1.0 and 1.1 in the same card.
> And the numer of devices is 1k per one card.
> 
> Btw, thanks a lot for the work of redhat, we just Implement the virtio protocol by hardware.

Yes, not very clear still. What are the device and vendor ID of the
card? Does it have the virtio capabilities?
Thanks

> > 
> > > >
> > > >
> > > > >   And we use this host vdpa+ legacy virtio in vm to hot migration,we
> > found
> > > > that the
> > > > >   Legacy model often get the middle state value after reset and probe
> > again.
> > > > >   The Soc is Simulated by fpga which is run slower than the host,so the
> > same
> > > > bug
> > > > >   Is found more frequently when the host use the other kernel like
> > ubuntu or
> > > > centos8.
> > > > >
> > > > >   So we hope we can fix this by upstream .
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Angus Chen <angus.chen@xxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >  drivers/virtio/virtio_pci_legacy.c | 4 +++-
> > > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/virtio_pci_legacy.c
> > > > b/drivers/virtio/virtio_pci_legacy.c
> > > > > > > index 2257f1b3d8ae..f2d241563e4f 100644
> > > > > > > --- a/drivers/virtio/virtio_pci_legacy.c
> > > > > > > +++ b/drivers/virtio/virtio_pci_legacy.c
> > > > > > > @@ -14,6 +14,7 @@
> > > > > > >   *  Michael S. Tsirkin <mst@xxxxxxxxxx>
> > > > > > >   */
> > > > > > >
> > > > > > > +#include <linux/delay.h>
> > > > > > >  #include "linux/virtio_pci_legacy.h"
> > > > > > >  #include "virtio_pci_common.h"
> > > > > > >
> > > > > > > @@ -97,7 +98,8 @@ static void vp_reset(struct virtio_device *vdev)
> > > > > > >         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);
> > > > > > > +       while (vp_legacy_get_status(&vp_dev->ldev))
> > > > > > > +               msleep(1);
> > > > > > >         /* Flush pending VQ/configuration callbacks. */
> > > > > > >         vp_synchronize_vectors(vdev);
> > > > > > >  }
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >
> > > > >
> > >
>