[PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET

[PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET

[Jianguo Sun]

Add CONFIG_VIRTIO_MMIO_POLL_RESET to as an option to enable
synchronous reset for the MMIO based transport for virtio.

Change-Id: Ifa7e29b4c0cfa26922535f921c6ab69eacdc4cfc
Signed-off-by: Jianguo Sun <quic_jianguos@quicinc.com>
Signed-off-by: Anant Goel <quic_anantg@quicinc.com>
---
 drivers/virtio/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 42a48ac763ee..fb17b1078d53 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -188,4 +188,12 @@ config VIRTIO_DEBUG
 
 	  If unsure, say N.
 
+config VIRTIO_MMIO_POLL_RESET
+	bool "Virti-mmio device synchronous reset support"
+	depends on VIRTIO_MMIO
+	help
+	  Say y here to enable synchronous reset for the MMIO transport based
+	  virtio device. After writing 0 to device_status, the driver must
+	  wait for a read of device_status to return 0 before reinitializing
+	  the device.
 endif # VIRTIO_MENU
-- 
2.17.1

[PATCH 2/2] virtio: Add polling virtio-mmio device reset completion

[Jianguo Sun]

We write 0 to device_status to initiate reset of a virtio-mmio device.
The reset operation itself may or may not be completed by the time
write instruction completes. Add polling device_status to return
0 to ensure reset completion before reinitializing the device.

Change-Id: If15d11d090dfd0d4972ad35f49af03e076872413
Signed-off-by: Jianguo Sun <quic_jianguos@quicinc.com>
Signed-off-by: Sreenad Menon <quic_sreemeno@quicinc.com>
Signed-off-by: Anant Goel <quic_anantg@quicinc.com>
---
 drivers/virtio/virtio_mmio.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 90e784e7b721..c34ad1782145 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -70,6 +70,7 @@
 #include <linux/virtio_config.h>
 #include <uapi/linux/virtio_mmio.h>
 #include <linux/virtio_ring.h>
+#include <linux/delay.h>
 
 
 
@@ -269,6 +270,13 @@ static void vm_reset(struct virtio_device *vdev)
 
 	/* 0 status means a reset. */
 	writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
+#ifdef CONFIG_VIRTIO_MMIO_POLL_RESET
+	/* After writing 0 to device_status, the driver MUST wait for a read of
+	 * device_status to return 0 before reinitializing the device.
+	 */
+	while (readl(vm_dev->base + VIRTIO_MMIO_STATUS))
+		usleep_range(1000, 1100);
+#endif
 }
 
 
-- 
2.17.1

Re: [PATCH 2/2] virtio: Add polling virtio-mmio device reset completion

[Jason Wang]

On Sun, Sep 22, 2024 at 8:35 PM Jianguo Sun <quic_jianguos@quicinc.com> wrote:
>
> We write 0 to device_status to initiate reset of a virtio-mmio device.
> The reset operation itself may or may not be completed by the time
> write instruction completes. Add polling device_status to return
> 0 to ensure reset completion before reinitializing the device.
>
> Change-Id: If15d11d090dfd0d4972ad35f49af03e076872413
> Signed-off-by: Jianguo Sun <quic_jianguos@quicinc.com>
> Signed-off-by: Sreenad Menon <quic_sreemeno@quicinc.com>
> Signed-off-by: Anant Goel <quic_anantg@quicinc.com>
> ---
>  drivers/virtio/virtio_mmio.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 90e784e7b721..c34ad1782145 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -70,6 +70,7 @@
>  #include <linux/virtio_config.h>
>  #include <uapi/linux/virtio_mmio.h>
>  #include <linux/virtio_ring.h>
> +#include <linux/delay.h>
>
>
>
> @@ -269,6 +270,13 @@ static void vm_reset(struct virtio_device *vdev)
>
>         /* 0 status means a reset. */
>         writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
> +#ifdef CONFIG_VIRTIO_MMIO_POLL_RESET
> +       /* After writing 0 to device_status, the driver MUST wait for a read of
> +        * device_status to return 0 before reinitializing the device.
> +        */
> +       while (readl(vm_dev->base + VIRTIO_MMIO_STATUS))
> +               usleep_range(1000, 1100);

I think we can just use what PCI did, which is msleep(1) here.

Thanks

> +#endif
>  }
>
>
> --
> 2.17.1
>

Re: [PATCH 2/2] virtio: Add polling virtio-mmio device reset completion

[Michael S. Tsirkin]

On Sun, Sep 22, 2024 at 08:34:35PM +0800, Jianguo Sun wrote:
> We write 0 to device_status to initiate reset of a virtio-mmio device.
> The reset operation itself may or may not be completed by the time
> write instruction completes. Add polling device_status to return
> 0 to ensure reset completion before reinitializing the device.
> 
> Change-Id: If15d11d090dfd0d4972ad35f49af03e076872413
> Signed-off-by: Jianguo Sun <quic_jianguos@quicinc.com>
> Signed-off-by: Sreenad Menon <quic_sreemeno@quicinc.com>
> Signed-off-by: Anant Goel <quic_anantg@quicinc.com>
> ---
>  drivers/virtio/virtio_mmio.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 90e784e7b721..c34ad1782145 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -70,6 +70,7 @@
>  #include <linux/virtio_config.h>
>  #include <uapi/linux/virtio_mmio.h>
>  #include <linux/virtio_ring.h>
> +#include <linux/delay.h>
>  
>  
>  
> @@ -269,6 +270,13 @@ static void vm_reset(struct virtio_device *vdev)
>  
>  	/* 0 status means a reset. */
>  	writel(0, vm_dev->base + VIRTIO_MMIO_STATUS);
> +#ifdef CONFIG_VIRTIO_MMIO_POLL_RESET
> +	/* After writing 0 to device_status, the driver MUST wait for a read of
> +	 * device_status to return 0 before reinitializing the device.
> +	 */

This is not in the spec, is it? First you need a spec patch. Then implement it.


> +	while (readl(vm_dev->base + VIRTIO_MMIO_STATUS))
> +		usleep_range(1000, 1100);
> +#endif
>  }
>  
>  
> -- 
> 2.17.1

Re: [PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET

[Jason Wang]

On Sun, Sep 22, 2024 at 8:35 PM Jianguo Sun <quic_jianguos@quicinc.com> wrote:
>
> Add CONFIG_VIRTIO_MMIO_POLL_RESET to as an option to enable
> synchronous reset for the MMIO based transport for virtio.
>
> Change-Id: Ifa7e29b4c0cfa26922535f921c6ab69eacdc4cfc
> Signed-off-by: Jianguo Sun <quic_jianguos@quicinc.com>
> Signed-off-by: Anant Goel <quic_anantg@quicinc.com>

Any reason this is not a feature but a config option?

> ---
>  drivers/virtio/Kconfig | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 42a48ac763ee..fb17b1078d53 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -188,4 +188,12 @@ config VIRTIO_DEBUG
>
>           If unsure, say N.
>
> +config VIRTIO_MMIO_POLL_RESET
> +       bool "Virti-mmio device synchronous reset support"
> +       depends on VIRTIO_MMIO
> +       help
> +         Say y here to enable synchronous reset for the MMIO transport based
> +         virtio device. After writing 0 to device_status, the driver must
> +         wait for a read of device_status to return 0 before reinitializing
> +         the device.
>  endif # VIRTIO_MENU
> --
> 2.17.1
>

Thanks

RE: [PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET

[Jianguo Sun (QUIC)]

Hi @Jason

Both hypervisor and backend device may need to support virtio-mmio device reset 
Adding CONFIG_VIRTIO_MMIO_POLL_RESET is for the system that has such support.

A config option (feature bit) is a better approach.
Thanks for the review.

BR
Jianguo

-----Original Message-----
From: Jason Wang <jasowang@redhat.com> 
Sent: Tuesday, September 24, 2024 3:39 PM
To: Jianguo Sun (QUIC) <quic_jianguos@quicinc.com>
Cc: mst@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; virtualization@lists.linux.dev; Anant Goel (QUIC) <quic_anantg@quicinc.com>
Subject: Re: [PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET

On Sun, Sep 22, 2024 at 8:35 PM Jianguo Sun <quic_jianguos@quicinc.com> wrote:
>
> Add CONFIG_VIRTIO_MMIO_POLL_RESET to as an option to enable 
> synchronous reset for the MMIO based transport for virtio.
>
> Change-Id: Ifa7e29b4c0cfa26922535f921c6ab69eacdc4cfc
> Signed-off-by: Jianguo Sun <quic_jianguos@quicinc.com>
> Signed-off-by: Anant Goel <quic_anantg@quicinc.com>

Any reason this is not a feature but a config option?

> ---
>  drivers/virtio/Kconfig | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig index 
> 42a48ac763ee..fb17b1078d53 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -188,4 +188,12 @@ config VIRTIO_DEBUG
>
>           If unsure, say N.
>
> +config VIRTIO_MMIO_POLL_RESET
> +       bool "Virti-mmio device synchronous reset support"
> +       depends on VIRTIO_MMIO
> +       help
> +         Say y here to enable synchronous reset for the MMIO transport based
> +         virtio device. After writing 0 to device_status, the driver must
> +         wait for a read of device_status to return 0 before reinitializing
> +         the device.
>  endif # VIRTIO_MENU
> --
> 2.17.1
>

Thanks

Re: [PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET

[Jason Wang]

On Thu, Sep 26, 2024 at 11:31 AM Jianguo Sun (QUIC)
<quic_jianguos@quicinc.com> wrote:
>
> Hi @Jason
>
> Both hypervisor and backend device may need to support virtio-mmio device reset

Then you need to patch the virtio spec first. Or it is already
supported by the spec?

> Adding CONFIG_VIRTIO_MMIO_POLL_RESET is for the system that has such support.

And we will probably have a feature or transport bit/flag for this.

Thanks

>
> A config option (feature bit) is a better approach.
> Thanks for the review.
>
> BR
> Jianguo
>
> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, September 24, 2024 3:39 PM
> To: Jianguo Sun (QUIC) <quic_jianguos@quicinc.com>
> Cc: mst@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; virtualization@lists.linux.dev; Anant Goel (QUIC) <quic_anantg@quicinc.com>
> Subject: Re: [PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET
>
> On Sun, Sep 22, 2024 at 8:35 PM Jianguo Sun <quic_jianguos@quicinc.com> wrote:
> >
> > Add CONFIG_VIRTIO_MMIO_POLL_RESET to as an option to enable
> > synchronous reset for the MMIO based transport for virtio.
> >
> > Change-Id: Ifa7e29b4c0cfa26922535f921c6ab69eacdc4cfc
> > Signed-off-by: Jianguo Sun <quic_jianguos@quicinc.com>
> > Signed-off-by: Anant Goel <quic_anantg@quicinc.com>
>
> Any reason this is not a feature but a config option?
>
> > ---
> >  drivers/virtio/Kconfig | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig index
> > 42a48ac763ee..fb17b1078d53 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -188,4 +188,12 @@ config VIRTIO_DEBUG
> >
> >           If unsure, say N.
> >
> > +config VIRTIO_MMIO_POLL_RESET
> > +       bool "Virti-mmio device synchronous reset support"
> > +       depends on VIRTIO_MMIO
> > +       help
> > +         Say y here to enable synchronous reset for the MMIO transport based
> > +         virtio device. After writing 0 to device_status, the driver must
> > +         wait for a read of device_status to return 0 before reinitializing
> > +         the device.
> >  endif # VIRTIO_MENU
> > --
> > 2.17.1
> >
>
> Thanks
>

Re: [PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET

[Michael S. Tsirkin]

On Thu, Sep 26, 2024 at 11:50:32AM +0800, Jason Wang wrote:
> On Thu, Sep 26, 2024 at 11:31 AM Jianguo Sun (QUIC)
> <quic_jianguos@quicinc.com> wrote:
> >
> > Hi @Jason
> >
> > Both hypervisor and backend device may need to support virtio-mmio device reset
> 
> Then you need to patch the virtio spec first. Or it is already
> supported by the spec?
> 
> > Adding CONFIG_VIRTIO_MMIO_POLL_RESET is for the system that has such support.
> 
> And we will probably have a feature or transport bit/flag for this.
> 
> Thanks

feature bits can't affect reset.
I don't know what a transport bit/flag is, in the context of MMIO.

We can just make it a recommendation.


> >
> > A config option (feature bit) is a better approach.
> > Thanks for the review.
> >
> > BR
> > Jianguo
> >
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, September 24, 2024 3:39 PM
> > To: Jianguo Sun (QUIC) <quic_jianguos@quicinc.com>
> > Cc: mst@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; virtualization@lists.linux.dev; Anant Goel (QUIC) <quic_anantg@quicinc.com>
> > Subject: Re: [PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET
> >
> > On Sun, Sep 22, 2024 at 8:35 PM Jianguo Sun <quic_jianguos@quicinc.com> wrote:
> > >
> > > Add CONFIG_VIRTIO_MMIO_POLL_RESET to as an option to enable
> > > synchronous reset for the MMIO based transport for virtio.
> > >
> > > Change-Id: Ifa7e29b4c0cfa26922535f921c6ab69eacdc4cfc
> > > Signed-off-by: Jianguo Sun <quic_jianguos@quicinc.com>
> > > Signed-off-by: Anant Goel <quic_anantg@quicinc.com>
> >
> > Any reason this is not a feature but a config option?
> >
> > > ---
> > >  drivers/virtio/Kconfig | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig index
> > > 42a48ac763ee..fb17b1078d53 100644
> > > --- a/drivers/virtio/Kconfig
> > > +++ b/drivers/virtio/Kconfig
> > > @@ -188,4 +188,12 @@ config VIRTIO_DEBUG
> > >
> > >           If unsure, say N.
> > >
> > > +config VIRTIO_MMIO_POLL_RESET
> > > +       bool "Virti-mmio device synchronous reset support"
> > > +       depends on VIRTIO_MMIO
> > > +       help
> > > +         Say y here to enable synchronous reset for the MMIO transport based
> > > +         virtio device. After writing 0 to device_status, the driver must
> > > +         wait for a read of device_status to return 0 before reinitializing
> > > +         the device.
> > >  endif # VIRTIO_MENU
> > > --
> > > 2.17.1
> > >
> >
> > Thanks
> >

Re: [PATCH 1/2] Virtio: Add CONFIG_VIRTIO_MMIO_POLL_RESET

[Michael S. Tsirkin]

On Sun, Sep 22, 2024 at 08:34:34PM +0800, Jianguo Sun wrote:
> Add CONFIG_VIRTIO_MMIO_POLL_RESET to as an option to enable
> synchronous reset for the MMIO based transport for virtio.
> Change-Id: Ifa7e29b4c0cfa26922535f921c6ab69eacdc4cfc

Don't put these in.

> Signed-off-by: Jianguo Sun <quic_jianguos@quicinc.com>
> Signed-off-by: Anant Goel <quic_anantg@quicinc.com>
> ---
>  drivers/virtio/Kconfig | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 42a48ac763ee..fb17b1078d53 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -188,4 +188,12 @@ config VIRTIO_DEBUG
>  
>  	  If unsure, say N.
>  
> +config VIRTIO_MMIO_POLL_RESET
> +	bool "Virti-mmio device synchronous reset support"
> +	depends on VIRTIO_MMIO
> +	help
> +	  Say y here to enable synchronous reset for the MMIO transport based
> +	  virtio device. After writing 0 to device_status, the driver must
> +	  wait for a read of device_status to return 0 before reinitializing
> +	  the device.

I see no real value in this config option as defined, users will have no
idea when to enable it. And you definitely do not want this
as a separate patch, you are adding an option but it does not work,
then you fix it in a separate patch. No need for this, it's
small enough.

>  endif # VIRTIO_MENU
> -- 
> 2.17.1