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

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

[Xuan Zhuo]

On Tue, 11 Apr 2023 09:38:32 +0800, Angus Chen <angus.chen@jaguarmicro.com> 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.
>
> Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>

LGTM

Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


> ---
>  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@redhat.com>
>   */
>
> +#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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Jason Wang]

On Tue, Apr 11, 2023 at 9:39 AM Angus Chen <angus.chen@jaguarmicro.com> 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.

Thanks

>
> Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
> ---
>  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@redhat.com>
>   */
>
> +#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
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Jason Wang]

On Tue, Apr 11, 2023 at 2:36 PM Angus Chen <angus.chen@jaguarmicro.com> wrote:
>
> Hi.
>
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, April 11, 2023 1:24 PM
> > To: Angus Chen <angus.chen@jaguarmicro.com>
> > Cc: mst@redhat.com; virtualization@lists.linux-foundation.org;
> > linux-kernel@vger.kernel.org
> > 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@jaguarmicro.com>
> > 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.
>   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 .

I think you can do mediation in your hypervisor.

When trapping set_status(), the hypervisor will not return until it
reads 0 from the hardware?

Thanks

> >
> > Thanks
> >
> > >
> > > Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
> > > ---
> > >  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@redhat.com>
> > >   */
> > >
> > > +#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
> > >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 02:39:34PM +0800, Jason Wang wrote:
> On Tue, Apr 11, 2023 at 2:36 PM Angus Chen <angus.chen@jaguarmicro.com> wrote:
> >
> > Hi.
> >
> > > -----Original Message-----
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Tuesday, April 11, 2023 1:24 PM
> > > To: Angus Chen <angus.chen@jaguarmicro.com>
> > > Cc: mst@redhat.com; virtualization@lists.linux-foundation.org;
> > > linux-kernel@vger.kernel.org
> > > 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@jaguarmicro.com>
> > > 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.
> >   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 .
> 
> I think you can do mediation in your hypervisor.
> 
> When trapping set_status(), the hypervisor will not return until it
> reads 0 from the hardware?
> 
> Thanks

Note that for legacy guests, 0 status write is not the only way
to reset the device, writing 0 into pa is another.



> > >
> > > Thanks
> > >
> > > >
> > > > Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
> > > > ---
> > > >  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@redhat.com>
> > > >   */
> > > >
> > > > +#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
> > > >
> >

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 06:36:39AM +0000, Angus Chen wrote:
> Hi.
> 
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, April 11, 2023 1:24 PM
> > To: Angus Chen <angus.chen@jaguarmicro.com>
> > Cc: mst@redhat.com; virtualization@lists.linux-foundation.org;
> > linux-kernel@vger.kernel.org
> > 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@jaguarmicro.com>
> > 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?


>   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@jaguarmicro.com>
> > > ---
> > >  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@redhat.com>
> > >   */
> > >
> > > +#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
> > >
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 06:49:03AM +0000, Angus Chen wrote:
> 
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, April 11, 2023 2:47 PM
> > To: Angus Chen <angus.chen@jaguarmicro.com>
> > Cc: Jason Wang <jasowang@redhat.com>;
> > virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> > 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@redhat.com>
> > > > Sent: Tuesday, April 11, 2023 1:24 PM
> > > > To: Angus Chen <angus.chen@jaguarmicro.com>
> > > > Cc: mst@redhat.com; virtualization@lists.linux-foundation.org;
> > > > linux-kernel@vger.kernel.org
> > > > 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@jaguarmicro.com>
> > > > 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?


> > 
> > 
> > >   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@jaguarmicro.com>
> > > > > ---
> > > > >  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@redhat.com>
> > > > >   */
> > > > >
> > > > > +#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
> > > > >
> > >
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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@redhat.com>
> > Sent: Tuesday, April 11, 2023 2:56 PM
> > To: Angus Chen <angus.chen@jaguarmicro.com>
> > Cc: Jason Wang <jasowang@redhat.com>;
> > virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> > 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@redhat.com>
> > > > Sent: Tuesday, April 11, 2023 2:47 PM
> > > > To: Angus Chen <angus.chen@jaguarmicro.com>
> > > > Cc: Jason Wang <jasowang@redhat.com>;
> > > > virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> > > > 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@redhat.com>
> > > > > > Sent: Tuesday, April 11, 2023 1:24 PM
> > > > > > To: Angus Chen <angus.chen@jaguarmicro.com>
> > > > > > Cc: mst@redhat.com; virtualization@lists.linux-foundation.org;
> > > > > > linux-kernel@vger.kernel.org
> > > > > > 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@jaguarmicro.com>
> > > > > > 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@jaguarmicro.com>
> > > > > > > ---
> > > > > > >  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@redhat.com>
> > > > > > >   */
> > > > > > >
> > > > > > > +#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
> > > > > > >
> > > > >
> > >
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 07:30:48AM +0000, Angus Chen wrote:
> 
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, April 11, 2023 3:21 PM
> > To: Angus Chen <angus.chen@jaguarmicro.com>
> > Cc: Jason Wang <jasowang@redhat.com>;
> > virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] virtio_pci: Wait for legacy device to be reset
> > 
> > On Tue, Apr 11, 2023 at 07:17:14AM +0000, Angus Chen wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Tuesday, April 11, 2023 2:56 PM
> > > > To: Angus Chen <angus.chen@jaguarmicro.com>
> > > > Cc: Jason Wang <jasowang@redhat.com>;
> > > > virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> > > > 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@redhat.com>
> > > > > > Sent: Tuesday, April 11, 2023 2:47 PM
> > > > > > To: Angus Chen <angus.chen@jaguarmicro.com>
> > > > > > Cc: Jason Wang <jasowang@redhat.com>;
> > > > > > virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org
> > > > > > 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@redhat.com>
> > > > > > > > Sent: Tuesday, April 11, 2023 1:24 PM
> > > > > > > > To: Angus Chen <angus.chen@jaguarmicro.com>
> > > > > > > > Cc: mst@redhat.com; virtualization@lists.linux-foundation.org;
> > > > > > > > linux-kernel@vger.kernel.org
> > > > > > > > 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@jaguarmicro.com>
> > > > > > > > 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
> It's have the virtio capabilities. 
> When it is used in bare metal ，
>    it just use PCI_VENDOR_ID_REDHAT_QUMRANET.
> When it use in vm,
>  It is just like :
> { PCI_DEVICE_SUB(PCI_VENDOR_ID_REDHAT_QUMRANET,
>              VIRTIO_TRANS_ID_NET,
>              PCI_VENDOR_ID_JAGUARMICRO,
>              VIRTIO_ID_NET) },
>     { PCI_DEVICE_SUB(PCI_VENDOR_ID_REDHAT_QUMRANET,
>              VIRTIO_TRANS_ID_BLOCK,
>              PCI_VENDOR_ID_JAGUARMICRO,
>              VIRTIO_ID_BLOCK) },
> 

Okay. So it's a transitional device. The question, then,
is how come you end up with drivers/virtio/virtio_pci_legacy.c
with a modern linux at all?
Did you by chance set force_legacy = true ?


> > 
> > > >
> > > > > >
> > > > > >
> > > > > > >   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@jaguarmicro.com>
> > > > > > > > > ---
> > > > > > > > >  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@redhat.com>
> > > > > > > > >   */
> > > > > > > > >
> > > > > > > > > +#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
> > > > > > > > >
> > > > > > >
> > > > >
> > >
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 10:57:51AM +0000, Angus Chen wrote:
> > > > Yes, not very clear still. What are the device and vendor ID of the
> > > > card? Does it have the virtio capabilities?
> > > > Thanks
> > > It's have the virtio capabilities.
> > > When it is used in bare metal ，
> > >    it just use PCI_VENDOR_ID_REDHAT_QUMRANET.
> > > When it use in vm,
> > >  It is just like :
> > > { PCI_DEVICE_SUB(PCI_VENDOR_ID_REDHAT_QUMRANET,
> > >              VIRTIO_TRANS_ID_NET,
> > >              PCI_VENDOR_ID_JAGUARMICRO,
> > >              VIRTIO_ID_NET) },
> > >     { PCI_DEVICE_SUB(PCI_VENDOR_ID_REDHAT_QUMRANET,
> > >              VIRTIO_TRANS_ID_BLOCK,
> > >              PCI_VENDOR_ID_JAGUARMICRO,
> > >              VIRTIO_ID_BLOCK) },
> > >
> > 
> > Okay. So it's a transitional device. The question, then,
> > is how come you end up with drivers/virtio/virtio_pci_legacy.c
> > with a modern linux at all?
> > Did you by chance set force_legacy = true ?
> Yes,I can provide more information about it.
> We can test it use force_legacy ,or we use a server with multi os but it use only one card.
> Thanks.

Okay, so I don't yet see lots of value of merging this patch upstream.
The problematic code does not run by default as the modern driver is
used, and applying the patch upstream does nothing to fix the multi-os
case.

What did I miss?


-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 09:38:32AM +0800, Angus Chen wrote:
> We read the status of device after reset,
> It is not guaranteed that the device be reseted successfully.

Sorry not guaranteed by what? I am guessing you have a legacy device
that does not reset fully on write, and you need to wait?

> We can use a while loop to make sure that,like the modern device did.
> The spec is not request it ,but it work.
> 
> Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>

Generally I don't much like touching legacy, no telling what
that will do. Case in point, is your device a pure
legacy device or a transitional device?

> ---
>  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@redhat.com>
>   */
>  
> +#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);

The problem with this is that it will break surprise
removal even worse than it's already broken.


>  	/* Flush pending VQ/configuration callbacks. */
>  	vp_synchronize_vectors(vdev);
>  }
> -- 
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 06:39:16AM +0000, Angus Chen wrote:
> Hi mst.
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, April 11, 2023 2:30 PM
> > To: Angus Chen <angus.chen@jaguarmicro.com>
> > Cc: jasowang@redhat.com; virtualization@lists.linux-foundation.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] virtio_pci: Wait for legacy device to be reset
> > 
> > On Tue, Apr 11, 2023 at 09:38:32AM +0800, Angus Chen wrote:
> > > We read the status of device after reset,
> > > It is not guaranteed that the device be reseted successfully.
> > 
> > Sorry not guaranteed by what? I am guessing you have a legacy device
> > that does not reset fully on write, and you need to wait?
>  When the card not finished reset, the read only return the middle state of card.
> > 
> > > We can use a while loop to make sure that,like the modern device did.
> > > The spec is not request it ,but it work.
> > >
> > > Signed-off-by: Angus Chen <angus.chen@jaguarmicro.com>
> > 
> > Generally I don't much like touching legacy, no telling what
> > that will do. Case in point, is your device a pure
> > legacy device or a transitional device?
>  Yes.,we have a real card which is use vitio spec.

So is it a transitional device?


>  Thank you.
> > 
> > > ---
> > >  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@redhat.com>
> > >   */
> > >
> > > +#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);
> > 
> > The problem with this is that it will break surprise
> > removal even worse than it's already broken.
> > 
> > 
> > >  	/* Flush pending VQ/configuration callbacks. */
> > >  	vp_synchronize_vectors(vdev);
> > >  }
> > > --
> > > 2.25.1
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization