[PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

Currently when driver initiates a queue reset, device is expected
to communicate reset status to the driver by changing the value of the
queue_reset register twice. First to return value other than 1 when
reset is ongoing, later to return 1 when queue reset is completed.

However initially during the device reset time the queue reset value
is zero. queue_reset changes the value of the register to a different
value on reset completion. Yet another time queue_reset value is
expected to change when queue_select is reprogrammed.

For example in below flow, a created virtqueue, which is disabled
by driver leaves the queue state as
queue_enable = 0, queue_reset = 1.

example flow:
a) 0,0 -> device init time value
b) 1,0 -> vq is enabled by driver and working
c) 1,1 -> vq is enabled, driver initiated reset
d) 1,0 -> vq is enabled, but device is busy doing the reset
e) 0,1 -> vq reset is completed in the device and VQ is now disabled
(conflict with initial queue reset state #a above)

On next iteration, when queue_select selects the same VQ again,
without enablement, device is confused to return 1 or 0 because
it was reset once before via queue_reset register.

This demands complex device implementation to understand what
should be returned for a VQ that is reset using queue_reset register
vs other means.

Instead, it is better and efficient to maintain the same VQ state
on the device when queue reset is completed.

new proposed flow:
q_enable, q_reset
A) 0, 0 -> default, device init time
B) 1, 0 -> driver has enabled vq
C) 1, 1 -> driver started q reset
D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
(device communicates that its working on resetting VQ, consistent with #C)
E) 0, 0 -> q_reset by device is completed, q got disabled
(consistent with device init time #A)

Hence, this patch proposes a simple change to have reset register
polarity to be same as that of initial reset value.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/139
Fixes: 12998e738621 ("virtio: pci support virtqueue reset")
Fixes: a4ce81a83780 ("virtio: mmio support virtqueue reset")
Fixes: 3b5378d70a42 ("virtio: introduce virtqueue reset as basic facility")
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v1->v2:
- Fixed below comment from Xuan Zhuo
- Fixed the configuration and register layout text to reflect the polarity
v0->v1:
- Fixed below comments from Michael
- Removed example device side pseudo code for old and new polarity
- Removed unwanted articles 'a' and 'the'
- Dropped extra empty line
---
 content.tex | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/content.tex b/content.tex
index 060bdab..68b176b 100644
--- a/content.tex
+++ b/content.tex
@@ -1039,7 +1039,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 \field{queue_reset} after the virtqueue is enabled with \field{queue_enable}.
 
 The device MUST reset the queue when 1 is written to \field{queue_reset}, and
-present a 1 in \field{queue_reset} after the queue has been reset, until the
+present 0 in \field{queue_reset} after the queue has been reset, until the
 driver re-enables the queue via \field{queue_enable} or the device is reset.
 The device MUST present consistent default values after queue reset.
 (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
@@ -1069,10 +1069,13 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 The driver MUST NOT write a 0 to \field{queue_enable}.
 
 If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
-\field{queue_reset} to reset the queue, it MUST verify that the queue
-has been reset by reading back \field{queue_reset} and ensuring that it
-is 1. The driver MAY re-enable the queue by writing a 1 to
-\field{queue_enable} after ensuring that the other virtqueue fields have
+\field{queue_reset} to reset the queue, driver MUST verify that the queue
+has been reset by reading back \field{queue_reset} until device returns a value of 0.
+Device continue to report value of 1 for the \field{queue_reset} until device finishes
+the queue reset request. When the device completes the queue reset, \field{queue_reset}
+and \field{queue_enable} set to zero, indicating
+that specified queue is ready to be enabled again. The driver MAY re-enable the queue by writing 1 to
+\field{queue_enable} after ensuring that other virtqueue fields have
 been set up correctly. The driver MAY set driver-writeable queue configuration
 values to different values than those that were used before the queue reset.
 (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
@@ -2015,7 +2018,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 \field{QueueReset} after the virtqueue is enabled with \field{QueueReady}.
 
 The device MUST reset the queue when 1 is written to \field{QueueReset}, and
-present a 1 in \field{QueueReset} after the queue has been reset, until the
+present 0 in \field{QueueReset} after the queue has been reset, until the
 driver re-enables the queue via \field{QueueReady} or until the device is reset.
 The device MUST present consistent default values after queue reset.
 (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
@@ -2063,10 +2066,13 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value.
 
 If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
-\field{QueueReset} to reset the queue, it MUST verify that the queue
-has been reset by reading back \field{QueueReset} and ensuring that it
-is 1. The driver MAY re-enable the queue by writing a 1 to
-\field{QueueReady} after ensuring that the other virtqueue fields have
+\field{QueueReset} to reset the queue, driver MUST verify that the queue
+has been reset by reading back \field{QueueReset} until device returns a value of 0.
+Device continue to report value of 1 for the \field{QueueReset} until device finishes
+the reset. When the device completes the queue reset, \field{QueueReset} and
+\field{QueueReady} set to zero, indicating that specified queue is ready to be
+enabled again. The driver MAY re-enable the queue by writing 1 to
+\field{QueueReady} after ensuring that other virtqueue fields have
 been set up correctly. The driver MAY set driver-writeable queue configuration
 values to different values than those that were used before the queue reset.
 (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
-- 
1.8.3.1

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Jason Wang]

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com> wrote:
>
> Currently when driver initiates a queue reset, device is expected
> to communicate reset status to the driver by changing the value of the
> queue_reset register twice. First to return value other than 1 when
> reset is ongoing, later to return 1 when queue reset is completed.
>
> However initially during the device reset time the queue reset value
> is zero. queue_reset changes the value of the register to a different
> value on reset completion. Yet another time queue_reset value is
> expected to change when queue_select is reprogrammed.
>
> For example in below flow, a created virtqueue, which is disabled
> by driver leaves the queue state as
> queue_enable = 0, queue_reset = 1.
>
> example flow:
> a) 0,0 -> device init time value
> b) 1,0 -> vq is enabled by driver and working

Did you see my reply in V1? What's the reason for using write to clear
behavior that is different from the device status?

We can simply make this as 1, 1 here and let the driver write to 0 to
reset the virtqueue.

And if we do this, the queue_enable and queue_reset are always the
same, then we can simply reuse queue_enable.

Thanks

> c) 1,1 -> vq is enabled, driver initiated reset
> d) 1,0 -> vq is enabled, but device is busy doing the reset
> e) 0,1 -> vq reset is completed in the device and VQ is now disabled
> (conflict with initial queue reset state #a above)
>
> On next iteration, when queue_select selects the same VQ again,
> without enablement, device is confused to return 1 or 0 because
> it was reset once before via queue_reset register.
>
> This demands complex device implementation to understand what
> should be returned for a VQ that is reset using queue_reset register
> vs other means.
>
> Instead, it is better and efficient to maintain the same VQ state
> on the device when queue reset is completed.
>
> new proposed flow:
> q_enable, q_reset
> A) 0, 0 -> default, device init time
> B) 1, 0 -> driver has enabled vq
> C) 1, 1 -> driver started q reset
> D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> (device communicates that its working on resetting VQ, consistent with #C)
> E) 0, 0 -> q_reset by device is completed, q got disabled
> (consistent with device init time #A)
>
> Hence, this patch proposes a simple change to have reset register
> polarity to be same as that of initial reset value.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/139
> Fixes: 12998e738621 ("virtio: pci support virtqueue reset")
> Fixes: a4ce81a83780 ("virtio: mmio support virtqueue reset")
> Fixes: 3b5378d70a42 ("virtio: introduce virtqueue reset as basic facility")
> Signed-off-by: Parav Pandit <parav@nvidia.com>
>
> ---
> changelog:
> v1->v2:
> - Fixed below comment from Xuan Zhuo
> - Fixed the configuration and register layout text to reflect the polarity
> v0->v1:
> - Fixed below comments from Michael
> - Removed example device side pseudo code for old and new polarity
> - Removed unwanted articles 'a' and 'the'
> - Dropped extra empty line
> ---
>  content.tex | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index 060bdab..68b176b 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1039,7 +1039,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  \field{queue_reset} after the virtqueue is enabled with \field{queue_enable}.
>
>  The device MUST reset the queue when 1 is written to \field{queue_reset}, and
> -present a 1 in \field{queue_reset} after the queue has been reset, until the
> +present 0 in \field{queue_reset} after the queue has been reset, until the
>  driver re-enables the queue via \field{queue_enable} or the device is reset.
>  The device MUST present consistent default values after queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> @@ -1069,10 +1069,13 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  The driver MUST NOT write a 0 to \field{queue_enable}.
>
>  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> -\field{queue_reset} to reset the queue, it MUST verify that the queue
> -has been reset by reading back \field{queue_reset} and ensuring that it
> -is 1. The driver MAY re-enable the queue by writing a 1 to
> -\field{queue_enable} after ensuring that the other virtqueue fields have
> +\field{queue_reset} to reset the queue, driver MUST verify that the queue
> +has been reset by reading back \field{queue_reset} until device returns a value of 0.
> +Device continue to report value of 1 for the \field{queue_reset} until device finishes
> +the queue reset request. When the device completes the queue reset, \field{queue_reset}
> +and \field{queue_enable} set to zero, indicating
> +that specified queue is ready to be enabled again. The driver MAY re-enable the queue by writing 1 to
> +\field{queue_enable} after ensuring that other virtqueue fields have
>  been set up correctly. The driver MAY set driver-writeable queue configuration
>  values to different values than those that were used before the queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> @@ -2015,7 +2018,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>  \field{QueueReset} after the virtqueue is enabled with \field{QueueReady}.
>
>  The device MUST reset the queue when 1 is written to \field{QueueReset}, and
> -present a 1 in \field{QueueReset} after the queue has been reset, until the
> +present 0 in \field{QueueReset} after the queue has been reset, until the
>  driver re-enables the queue via \field{QueueReady} or until the device is reset.
>  The device MUST present consistent default values after queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> @@ -2063,10 +2066,13 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>  it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value.
>
>  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> -\field{QueueReset} to reset the queue, it MUST verify that the queue
> -has been reset by reading back \field{QueueReset} and ensuring that it
> -is 1. The driver MAY re-enable the queue by writing a 1 to
> -\field{QueueReady} after ensuring that the other virtqueue fields have
> +\field{QueueReset} to reset the queue, driver MUST verify that the queue
> +has been reset by reading back \field{QueueReset} until device returns a value of 0.
> +Device continue to report value of 1 for the \field{QueueReset} until device finishes
> +the reset. When the device completes the queue reset, \field{QueueReset} and
> +\field{QueueReady} set to zero, indicating that specified queue is ready to be
> +enabled again. The driver MAY re-enable the queue by writing 1 to
> +\field{QueueReady} after ensuring that other virtqueue fields have
>  been set up correctly. The driver MAY set driver-writeable queue configuration
>  values to different values than those that were used before the queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> --
> 1.8.3.1
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Xuan Zhuo]

On Wed, 27 Apr 2022 19:29:37 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> > Currently when driver initiates a queue reset, device is expected
> > to communicate reset status to the driver by changing the value of the
> > queue_reset register twice. First to return value other than 1 when
> > reset is ongoing, later to return 1 when queue reset is completed.
> >
> > However initially during the device reset time the queue reset value
> > is zero. queue_reset changes the value of the register to a different
> > value on reset completion. Yet another time queue_reset value is
> > expected to change when queue_select is reprogrammed.
> >
> > For example in below flow, a created virtqueue, which is disabled
> > by driver leaves the queue state as
> > queue_enable = 0, queue_reset = 1.
> >
> > example flow:
> > a) 0,0 -> device init time value
> > b) 1,0 -> vq is enabled by driver and working
>
> Did you see my reply in V1? What's the reason for using write to clear
> behavior that is different from the device status?
>
> We can simply make this as 1, 1 here and let the driver write to 0 to
> reset the virtqueue.
>
> And if we do this, the queue_enable and queue_reset are always the
> same, then we can simply reuse queue_enable.


The reason I added a new item(queue_reset) at that time was because there was
such a sentence:

	The driver MUST NOT write a 0 to \field{queue_enable}.

Thanks.


>
> Thanks
>
> > c) 1,1 -> vq is enabled, driver initiated reset
> > d) 1,0 -> vq is enabled, but device is busy doing the reset
> > e) 0,1 -> vq reset is completed in the device and VQ is now disabled
> > (conflict with initial queue reset state #a above)
> >
> > On next iteration, when queue_select selects the same VQ again,
> > without enablement, device is confused to return 1 or 0 because
> > it was reset once before via queue_reset register.
> >
> > This demands complex device implementation to understand what
> > should be returned for a VQ that is reset using queue_reset register
> > vs other means.
> >
> > Instead, it is better and efficient to maintain the same VQ state
> > on the device when queue reset is completed.
> >
> > new proposed flow:
> > q_enable, q_reset
> > A) 0, 0 -> default, device init time
> > B) 1, 0 -> driver has enabled vq
> > C) 1, 1 -> driver started q reset
> > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> > (device communicates that its working on resetting VQ, consistent with #C)
> > E) 0, 0 -> q_reset by device is completed, q got disabled
> > (consistent with device init time #A)
> >
> > Hence, this patch proposes a simple change to have reset register
> > polarity to be same as that of initial reset value.
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/139
> > Fixes: 12998e738621 ("virtio: pci support virtqueue reset")
> > Fixes: a4ce81a83780 ("virtio: mmio support virtqueue reset")
> > Fixes: 3b5378d70a42 ("virtio: introduce virtqueue reset as basic facility")
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> >
> > ---
> > changelog:
> > v1->v2:
> > - Fixed below comment from Xuan Zhuo
> > - Fixed the configuration and register layout text to reflect the polarity
> > v0->v1:
> > - Fixed below comments from Michael
> > - Removed example device side pseudo code for old and new polarity
> > - Removed unwanted articles 'a' and 'the'
> > - Dropped extra empty line
> > ---
> >  content.tex | 26 ++++++++++++++++----------
> >  1 file changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index 060bdab..68b176b 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -1039,7 +1039,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >  \field{queue_reset} after the virtqueue is enabled with \field{queue_enable}.
> >
> >  The device MUST reset the queue when 1 is written to \field{queue_reset}, and
> > -present a 1 in \field{queue_reset} after the queue has been reset, until the
> > +present 0 in \field{queue_reset} after the queue has been reset, until the
> >  driver re-enables the queue via \field{queue_enable} or the device is reset.
> >  The device MUST present consistent default values after queue reset.
> >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> > @@ -1069,10 +1069,13 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >  The driver MUST NOT write a 0 to \field{queue_enable}.
> >
> >  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> > -\field{queue_reset} to reset the queue, it MUST verify that the queue
> > -has been reset by reading back \field{queue_reset} and ensuring that it
> > -is 1. The driver MAY re-enable the queue by writing a 1 to
> > -\field{queue_enable} after ensuring that the other virtqueue fields have
> > +\field{queue_reset} to reset the queue, driver MUST verify that the queue
> > +has been reset by reading back \field{queue_reset} until device returns a value of 0.
> > +Device continue to report value of 1 for the \field{queue_reset} until device finishes
> > +the queue reset request. When the device completes the queue reset, \field{queue_reset}
> > +and \field{queue_enable} set to zero, indicating
> > +that specified queue is ready to be enabled again. The driver MAY re-enable the queue by writing 1 to
> > +\field{queue_enable} after ensuring that other virtqueue fields have
> >  been set up correctly. The driver MAY set driver-writeable queue configuration
> >  values to different values than those that were used before the queue reset.
> >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> > @@ -2015,7 +2018,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> >  \field{QueueReset} after the virtqueue is enabled with \field{QueueReady}.
> >
> >  The device MUST reset the queue when 1 is written to \field{QueueReset}, and
> > -present a 1 in \field{QueueReset} after the queue has been reset, until the
> > +present 0 in \field{QueueReset} after the queue has been reset, until the
> >  driver re-enables the queue via \field{QueueReady} or until the device is reset.
> >  The device MUST present consistent default values after queue reset.
> >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> > @@ -2063,10 +2066,13 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> >  it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value.
> >
> >  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> > -\field{QueueReset} to reset the queue, it MUST verify that the queue
> > -has been reset by reading back \field{QueueReset} and ensuring that it
> > -is 1. The driver MAY re-enable the queue by writing a 1 to
> > -\field{QueueReady} after ensuring that the other virtqueue fields have
> > +\field{QueueReset} to reset the queue, driver MUST verify that the queue
> > +has been reset by reading back \field{QueueReset} until device returns a value of 0.
> > +Device continue to report value of 1 for the \field{QueueReset} until device finishes
> > +the reset. When the device completes the queue reset, \field{QueueReset} and
> > +\field{QueueReady} set to zero, indicating that specified queue is ready to be
> > +enabled again. The driver MAY re-enable the queue by writing 1 to
> > +\field{QueueReady} after ensuring that other virtqueue fields have
> >  been set up correctly. The driver MAY set driver-writeable queue configuration
> >  values to different values than those that were used before the queue reset.
> >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> > --
> > 1.8.3.1
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >
>

[virtio-dev] Re: [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Cornelia Huck]

On Wed, Apr 27 2022, Parav Pandit <parav@nvidia.com> wrote:

> Currently when driver initiates a queue reset, device is expected
> to communicate reset status to the driver by changing the value of the
> queue_reset register twice. First to return value other than 1 when
> reset is ongoing, later to return 1 when queue reset is completed.
>
> However initially during the device reset time the queue reset value
> is zero. queue_reset changes the value of the register to a different
> value on reset completion. Yet another time queue_reset value is
> expected to change when queue_select is reprogrammed.
>
> For example in below flow, a created virtqueue, which is disabled
> by driver leaves the queue state as
> queue_enable = 0, queue_reset = 1.
>
> example flow:
> a) 0,0 -> device init time value
> b) 1,0 -> vq is enabled by driver and working
> c) 1,1 -> vq is enabled, driver initiated reset
> d) 1,0 -> vq is enabled, but device is busy doing the reset
> e) 0,1 -> vq reset is completed in the device and VQ is now disabled
> (conflict with initial queue reset state #a above)
>
> On next iteration, when queue_select selects the same VQ again,
> without enablement, device is confused to return 1 or 0 because
> it was reset once before via queue_reset register.
>
> This demands complex device implementation to understand what
> should be returned for a VQ that is reset using queue_reset register
> vs other means.
>
> Instead, it is better and efficient to maintain the same VQ state
> on the device when queue reset is completed.
>
> new proposed flow:
> q_enable, q_reset
> A) 0, 0 -> default, device init time
> B) 1, 0 -> driver has enabled vq
> C) 1, 1 -> driver started q reset
> D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> (device communicates that its working on resetting VQ, consistent with #C)
> E) 0, 0 -> q_reset by device is completed, q got disabled
> (consistent with device init time #A)
>
> Hence, this patch proposes a simple change to have reset register
> polarity to be same as that of initial reset value.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/139
> Fixes: 12998e738621 ("virtio: pci support virtqueue reset")
> Fixes: a4ce81a83780 ("virtio: mmio support virtqueue reset")
> Fixes: 3b5378d70a42 ("virtio: introduce virtqueue reset as basic facility")
> Signed-off-by: Parav Pandit <parav@nvidia.com>
>
> ---
> changelog:
> v1->v2:
> - Fixed below comment from Xuan Zhuo
> - Fixed the configuration and register layout text to reflect the polarity
> v0->v1:
> - Fixed below comments from Michael
> - Removed example device side pseudo code for old and new polarity
> - Removed unwanted articles 'a' and 'the'
> - Dropped extra empty line
> ---
>  content.tex | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>

[No comment about the actual change, as I have not yet sufficiently
thought that through, but some remarks regarding the wording]

> diff --git a/content.tex b/content.tex
> index 060bdab..68b176b 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1039,7 +1039,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  \field{queue_reset} after the virtqueue is enabled with \field{queue_enable}.
>  
>  The device MUST reset the queue when 1 is written to \field{queue_reset}, and
> -present a 1 in \field{queue_reset} after the queue has been reset, until the
> +present 0 in \field{queue_reset} after the queue has been reset, until the
>  driver re-enables the queue via \field{queue_enable} or the device is reset.
>  The device MUST present consistent default values after queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> @@ -1069,10 +1069,13 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  The driver MUST NOT write a 0 to \field{queue_enable}.
>  
>  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> -\field{queue_reset} to reset the queue, it MUST verify that the queue
> -has been reset by reading back \field{queue_reset} and ensuring that it
> -is 1. The driver MAY re-enable the queue by writing a 1 to
> -\field{queue_enable} after ensuring that the other virtqueue fields have
> +\field{queue_reset} to reset the queue, driver MUST verify that the queue

s/driver/the driver/

> +has been reset by reading back \field{queue_reset} until device returns a value of 0.

s/device/the device/

> +Device continue to report value of 1 for the \field{queue_reset} until device finishes
> +the queue reset request.

This looks a bit odd: Shouldn't that go into the normative section for
the device instead?

> When the device completes the queue reset, \field{queue_reset}
> +and \field{queue_enable} set to zero, indicating
> +that specified queue is ready to be enabled again.

Same here. I think a statement that the driver MUST NOT consider queue
reset to be complete until it reads back 0 in both queue_reset and
queue_enable is more appropriate here.

> The driver MAY re-enable the queue by writing 1 to
> +\field{queue_enable} after ensuring that other virtqueue fields have
>  been set up correctly. The driver MAY set driver-writeable queue configuration
>  values to different values than those that were used before the queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> @@ -2015,7 +2018,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>  \field{QueueReset} after the virtqueue is enabled with \field{QueueReady}.
>  
>  The device MUST reset the queue when 1 is written to \field{QueueReset}, and
> -present a 1 in \field{QueueReset} after the queue has been reset, until the
> +present 0 in \field{QueueReset} after the queue has been reset, until the
>  driver re-enables the queue via \field{QueueReady} or until the device is reset.
>  The device MUST present consistent default values after queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> @@ -2063,10 +2066,13 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>  it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value.
>  
>  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> -\field{QueueReset} to reset the queue, it MUST verify that the queue
> -has been reset by reading back \field{QueueReset} and ensuring that it
> -is 1. The driver MAY re-enable the queue by writing a 1 to
> -\field{QueueReady} after ensuring that the other virtqueue fields have
> +\field{QueueReset} to reset the queue, driver MUST verify that the queue
> +has been reset by reading back \field{QueueReset} until device returns a value of 0.
> +Device continue to report value of 1 for the \field{QueueReset} until device finishes
> +the reset. When the device completes the queue reset, \field{QueueReset} and
> +\field{QueueReady} set to zero, indicating that specified queue is ready to be
> +enabled again. The driver MAY re-enable the queue by writing 1 to
> +\field{QueueReady} after ensuring that other virtqueue fields have
>  been set up correctly. The driver MAY set driver-writeable queue configuration
>  values to different values than those that were used before the queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).

Similar comments as for the pci section.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, April 27, 2022 7:30 AM
> 
> On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> > example flow:
> > a) 0,0 -> device init time value
> > b) 1,0 -> vq is enabled by driver and working
> 
> Did you see my reply in V1? What's the reason for using write to clear
> behavior that is different from the device status?
> 
> We can simply make this as 1, 1 here and let the driver write to 0 to reset the
> virtqueue.
> 
> And if we do this, the queue_enable and queue_reset are always the same,
> then we can simply reuse queue_enable.
> 
Yes, I know we can make this work using new feature bit + single queue_enable register.
I replied that in v0 to Michael.

I was not sure how drastic that would be at this point in the spec release cycle that Michael highlighted.
Hence, I proposed a minimal change fix to queue_reset register given timeline.

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Michael S. Tsirkin]

On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> 
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, April 27, 2022 7:30 AM
> > 
> > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > > example flow:
> > > a) 0,0 -> device init time value
> > > b) 1,0 -> vq is enabled by driver and working
> > 
> > Did you see my reply in V1? What's the reason for using write to clear
> > behavior that is different from the device status?
> > 
> > We can simply make this as 1, 1 here and let the driver write to 0 to reset the
> > virtqueue.
> > 
> > And if we do this, the queue_enable and queue_reset are always the same,
> > then we can simply reuse queue_enable.
> > 
> Yes, I know we can make this work using new feature bit + single queue_enable register.
> I replied that in v0 to Michael.

A bigger question in my eyes is that down the road we might want to
be able to stop the ring without having it lose state.
The natural interface for that seems to be writing 0 to queue enable.

> I was not sure how drastic that would be at this point in the spec release cycle that Michael highlighted.
> Hence, I proposed a minimal change fix to queue_reset register given timeline.

Well if accepted this proposal is going to delay the release anyway.  If
we are doing a new feature then that can love alongside the one that is
already in the spec.



-- 
MST

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, April 27, 2022 11:30 AM
> 
> On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Wednesday, April 27, 2022 7:30 AM
> > >
> > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > > example flow:
> > > > a) 0,0 -> device init time value
> > > > b) 1,0 -> vq is enabled by driver and working
> > >
> > > Did you see my reply in V1? What's the reason for using write to
> > > clear behavior that is different from the device status?
> > >
> > > We can simply make this as 1, 1 here and let the driver write to 0
> > > to reset the virtqueue.
> > >
> > > And if we do this, the queue_enable and queue_reset are always the
> > > same, then we can simply reuse queue_enable.
> > >
> > Yes, I know we can make this work using new feature bit + single
> queue_enable register.
> > I replied that in v0 to Michael.
> 
> A bigger question in my eyes is that down the road we might want to be able to
> stop the ring without having it lose state.
> The natural interface for that seems to be writing 0 to queue enable.
Why queue_enable and not queue_reset?

to me this interface is unlikely performant and useful for such case.
When we want to pause/stop the VQ and query the state we need performant scheme, that can even work in a batch for all the VQs.
At that point programming 64 registers to pause/stop VQ without losing state and querying its indices etc won't be scalable with register interface.
I imagine a AQ (likely) or some other interface.
 
> 
> > I was not sure how drastic that would be at this point in the spec release cycle
> that Michael highlighted.
> > Hence, I proposed a minimal change fix to queue_reset register given timeline.
> 
> Well if accepted this proposal is going to delay the release anyway.  If we are
> doing a new feature then that can love alongside the one that is already in the
> spec.
I didn't quite understand your point.
This queue_reset in its current state (with/without) this proposed fix is mostly usable within the guest for dynamic VQ creation/deletion to connect to ethtool/xdp or more.

I don't see a need to delay the fix, to a larger feature that needs more than just start/stop button.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Michael S. Tsirkin]

On Wed, Apr 27, 2022 at 03:39:40PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, April 27, 2022 11:30 AM
> > 
> > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > >
> > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > > example flow:
> > > > > a) 0,0 -> device init time value
> > > > > b) 1,0 -> vq is enabled by driver and working
> > > >
> > > > Did you see my reply in V1? What's the reason for using write to
> > > > clear behavior that is different from the device status?
> > > >
> > > > We can simply make this as 1, 1 here and let the driver write to 0
> > > > to reset the virtqueue.
> > > >
> > > > And if we do this, the queue_enable and queue_reset are always the
> > > > same, then we can simply reuse queue_enable.
> > > >
> > > Yes, I know we can make this work using new feature bit + single
> > queue_enable register.
> > > I replied that in v0 to Michael.
> > 
> > A bigger question in my eyes is that down the road we might want to be able to
> > stop the ring without having it lose state.
> > The natural interface for that seems to be writing 0 to queue enable.
> Why queue_enable and not queue_reset?

If what to disable ring without reset then writing into reset
seems unintuitive.

> to me this interface is unlikely performant and useful for such case.
> When we want to pause/stop the VQ and query the state we need performant scheme, that can even work in a batch for all the VQs.
> At that point programming 64 registers to pause/stop VQ without losing state and querying its indices etc won't be scalable with register interface.
> I imagine a AQ (likely) or some other interface.
>  
> > 
> > > I was not sure how drastic that would be at this point in the spec release cycle
> > that Michael highlighted.
> > > Hence, I proposed a minimal change fix to queue_reset register given timeline.
> > 
> > Well if accepted this proposal is going to delay the release anyway.  If we are
> > doing a new feature then that can love alongside the one that is already in the
> > spec.
> I didn't quite understand your point.

I understood your "given timeline" to mean "to avoid delays in 1.2
release". My point is any material change will mean a delay at this
time.


> This queue_reset in its current state (with/without) this proposed fix is mostly usable within the guest for dynamic VQ creation/deletion to connect to ethtool/xdp or more.
> 
> I don't see a need to delay the fix, to a larger feature that needs more than just start/stop button.



-- 
MST

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, April 27, 2022 11:44 AM
> 
> On Wed, Apr 27, 2022 at 03:39:40PM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, April 27, 2022 11:30 AM
> > >
> > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > >
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > >
> > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com>
> wrote:
> > > > > >
> > > > > > example flow:
> > > > > > a) 0,0 -> device init time value
> > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > >
> > > > > Did you see my reply in V1? What's the reason for using write to
> > > > > clear behavior that is different from the device status?
> > > > >
> > > > > We can simply make this as 1, 1 here and let the driver write to
> > > > > 0 to reset the virtqueue.
> > > > >
> > > > > And if we do this, the queue_enable and queue_reset are always
> > > > > the same, then we can simply reuse queue_enable.
> > > > >
> > > > Yes, I know we can make this work using new feature bit + single
> > > queue_enable register.
> > > > I replied that in v0 to Michael.
> > >
> > > A bigger question in my eyes is that down the road we might want to
> > > be able to stop the ring without having it lose state.
> > > The natural interface for that seems to be writing 0 to queue enable.
> > Why queue_enable and not queue_reset?
> 
> If what to disable ring without reset then writing into reset seems
> unintuitive.
> 
True. I assume you want to start the queue again later, hence the stop/start.
Make sense.

> > to me this interface is unlikely performant and useful for such case.
> > When we want to pause/stop the VQ and query the state we need
> performant scheme, that can even work in a batch for all the VQs.
> > At that point programming 64 registers to pause/stop VQ without losing
> state and querying its indices etc won't be scalable with register interface.
> > I imagine a AQ (likely) or some other interface.
> >
> > >
> > > > I was not sure how drastic that would be at this point in the spec
> > > > release cycle
> > > that Michael highlighted.
> > > > Hence, I proposed a minimal change fix to queue_reset register given
> timeline.
> > >
> > > Well if accepted this proposal is going to delay the release anyway.
> > > If we are doing a new feature then that can love alongside the one
> > > that is already in the spec.
> > I didn't quite understand your point.
> 
> I understood your "given timeline" to mean "to avoid delays in 1.2 release".
Yes.

> My point is any material change will mean a delay at this time.
But this is so basic.
It's hard to gaze at this spec for coming years and the code to see,
Hey sometimes 0 means disabled, sometime 0 means still enabled, sometime 1 means enabled, and sometimes 1 means now disabled...
And maintain those weird code in device side and extra state bits burning some expensive chip resource.

Is removing from 1.2 is equal delay to get is fixed in 1.3?
If yes, I make humble request to fix this and have errata.
Some of the professional standard bodies release the spec and short after that errata/ratification follows the release that resolve such small issues.
May be time for virtio spec to take this opportunity now and be bit agile on it.
Your call. :)

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Parav Pandit
> Sent: Wednesday, April 27, 2022 11:58 AM
> 
> But this is so basic.
> It's hard to gaze at this spec for coming years and the code to see, Hey
> sometimes 0 means disabled, sometime 0 means still enabled, sometime 1
> means enabled, and sometimes 1 means now disabled...
> And maintain those weird code in device side and extra state bits burning
> some expensive chip resource.
> 
> Is removing from 1.2 is equal delay to get is fixed in 1.3?
> If yes, I make humble request to fix this and have errata.
> Some of the professional standard bodies release the spec and short after
> that errata/ratification follows the release that resolve such small issues.
> May be time for virtio spec to take this opportunity now and be bit agile on it.
> Your call. :)


I think further, it appears a real bug that requires so special handling in the device for next years to carry.

Imagine this sequence.
1. A virtio device is handed over to guest VM
2. A virtio device started queue_reset sequence,
So register values are:
queue_enable = 1, q_reset=1
2. guest VM poled the q_reset register.
Queue_enable = 1, q_reset = 0 (because device is doing the resetting the queue)
3. HV suspend the VM and queried the VQ state
VQ state returned 
q_enable=1, q_reset = 0.

HV doesn't know what q_reset=0 mean here, is it 0 because it was never reset?
Or it is 0 because GVM Started the reset, but reset didn't finish?

When this virtio device is restored on the other side, HV and device doesn't know how to deal with this.

A WA that all devices will implement is, not returning 0, in step_2, but return say q_reset = 0xa to indicate that its other than 1 and other than 0.
But hey, the destination side needs to treat this special 0xa and convert to internal q_yet_busy stage.

And this answer Jason and myself why queue_enable shouldn't be overloaded for this busy_wait register.
Hence queue_reset register seems the right choice with the fix.

Starting to think of workaround even before the spec release is not elegant.
Lets please fix this, bug effect is expanding beyond the primary virtio device itself.

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Michael S. Tsirkin]

On Wed, Apr 27, 2022 at 03:57:35PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, April 27, 2022 11:44 AM
> > 
> > On Wed, Apr 27, 2022 at 03:39:40PM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Wednesday, April 27, 2022 11:30 AM
> > > >
> > > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > > >
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > > >
> > > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com>
> > wrote:
> > > > > > >
> > > > > > > example flow:
> > > > > > > a) 0,0 -> device init time value
> > > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > > >
> > > > > > Did you see my reply in V1? What's the reason for using write to
> > > > > > clear behavior that is different from the device status?
> > > > > >
> > > > > > We can simply make this as 1, 1 here and let the driver write to
> > > > > > 0 to reset the virtqueue.
> > > > > >
> > > > > > And if we do this, the queue_enable and queue_reset are always
> > > > > > the same, then we can simply reuse queue_enable.
> > > > > >
> > > > > Yes, I know we can make this work using new feature bit + single
> > > > queue_enable register.
> > > > > I replied that in v0 to Michael.
> > > >
> > > > A bigger question in my eyes is that down the road we might want to
> > > > be able to stop the ring without having it lose state.
> > > > The natural interface for that seems to be writing 0 to queue enable.
> > > Why queue_enable and not queue_reset?
> > 
> > If what to disable ring without reset then writing into reset seems
> > unintuitive.
> > 
> True. I assume you want to start the queue again later, hence the stop/start.
> Make sense.
> 
> > > to me this interface is unlikely performant and useful for such case.
> > > When we want to pause/stop the VQ and query the state we need
> > performant scheme, that can even work in a batch for all the VQs.
> > > At that point programming 64 registers to pause/stop VQ without losing
> > state and querying its indices etc won't be scalable with register interface.
> > > I imagine a AQ (likely) or some other interface.
> > >
> > > >
> > > > > I was not sure how drastic that would be at this point in the spec
> > > > > release cycle
> > > > that Michael highlighted.
> > > > > Hence, I proposed a minimal change fix to queue_reset register given
> > timeline.
> > > >
> > > > Well if accepted this proposal is going to delay the release anyway.
> > > > If we are doing a new feature then that can love alongside the one
> > > > that is already in the spec.
> > > I didn't quite understand your point.
> > 
> > I understood your "given timeline" to mean "to avoid delays in 1.2 release".
> Yes.
> 
> > My point is any material change will mean a delay at this time.
> But this is so basic.
> It's hard to gaze at this spec for coming years and the code to see,
> Hey sometimes 0 means disabled, sometime 0 means still enabled, sometime 1 means enabled, and sometimes 1 means now disabled...
> And maintain those weird code in device side and extra state bits burning some expensive chip resource.
> Is removing from 1.2 is equal delay to get is fixed in 1.3?
> If yes, I make humble request to fix this and have errata.
> Some of the professional standard bodies release the spec and short after that errata/ratification follows the release that resolve such small issues.
> May be time for virtio spec to take this opportunity now and be bit agile on it.
> Your call. :)

I would suggest waiting for results of public review. The TC can then
decide whether to release spec as is and then another version with bug
fixes, or delay this one.



-- 
MST

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, April 27, 2022 3:28 PM
> 
> On Wed, Apr 27, 2022 at 03:57:35PM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, April 27, 2022 11:44 AM
> > >
> > > On Wed, Apr 27, 2022 at 03:39:40PM +0000, Parav Pandit wrote:
> > > >
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Wednesday, April 27, 2022 11:30 AM
> > > > >
> > > > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > > > >
> > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > > > >
> > > > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit
> > > > > > > <parav@nvidia.com>
> > > wrote:
> > > > > > > >
> > > > > > > > example flow:
> > > > > > > > a) 0,0 -> device init time value
> > > > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > > > >
> > > > > > > Did you see my reply in V1? What's the reason for using
> > > > > > > write to clear behavior that is different from the device status?
> > > > > > >
> > > > > > > We can simply make this as 1, 1 here and let the driver
> > > > > > > write to
> > > > > > > 0 to reset the virtqueue.
> > > > > > >
> > > > > > > And if we do this, the queue_enable and queue_reset are
> > > > > > > always the same, then we can simply reuse queue_enable.
> > > > > > >
> > > > > > Yes, I know we can make this work using new feature bit +
> > > > > > single
> > > > > queue_enable register.
> > > > > > I replied that in v0 to Michael.
> > > > >
> > > > > A bigger question in my eyes is that down the road we might want
> > > > > to be able to stop the ring without having it lose state.
> > > > > The natural interface for that seems to be writing 0 to queue enable.
> > > > Why queue_enable and not queue_reset?
> > >
> > > If what to disable ring without reset then writing into reset seems
> > > unintuitive.
> > >
> > True. I assume you want to start the queue again later, hence the
> stop/start.
> > Make sense.
> >
> > > > to me this interface is unlikely performant and useful for such case.
> > > > When we want to pause/stop the VQ and query the state we need
> > > performant scheme, that can even work in a batch for all the VQs.
> > > > At that point programming 64 registers to pause/stop VQ without
> > > > losing
> > > state and querying its indices etc won't be scalable with register
> interface.
> > > > I imagine a AQ (likely) or some other interface.
> > > >
> > > > >
> > > > > > I was not sure how drastic that would be at this point in the
> > > > > > spec release cycle
> > > > > that Michael highlighted.
> > > > > > Hence, I proposed a minimal change fix to queue_reset register
> > > > > > given
> > > timeline.
> > > > >
> > > > > Well if accepted this proposal is going to delay the release anyway.
> > > > > If we are doing a new feature then that can love alongside the
> > > > > one that is already in the spec.
> > > > I didn't quite understand your point.
> > >
> > > I understood your "given timeline" to mean "to avoid delays in 1.2
> release".
> > Yes.
> >
> > > My point is any material change will mean a delay at this time.
> > But this is so basic.
> > It's hard to gaze at this spec for coming years and the code to see,
> > Hey sometimes 0 means disabled, sometime 0 means still enabled,
> sometime 1 means enabled, and sometimes 1 means now disabled...
> > And maintain those weird code in device side and extra state bits burning
> some expensive chip resource.
> > Is removing from 1.2 is equal delay to get is fixed in 1.3?
> > If yes, I make humble request to fix this and have errata.
> > Some of the professional standard bodies release the spec and short after
> that errata/ratification follows the release that resolve such small issues.
> > May be time for virtio spec to take this opportunity now and be bit agile on
> it.
> > Your call. :)
> 
> I would suggest waiting for results of public review. The TC can then decide
> whether to release spec as is and then another version with bug fixes, or
> delay this one.
Ok.
I will post v3 addressing Cornelia's comments.

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Michael S. Tsirkin]

On Wed, Apr 27, 2022 at 04:15:05PM +0000, Parav Pandit wrote:
> 
> > From: Parav Pandit
> > Sent: Wednesday, April 27, 2022 11:58 AM
> > 
> > But this is so basic.
> > It's hard to gaze at this spec for coming years and the code to see, Hey
> > sometimes 0 means disabled, sometime 0 means still enabled, sometime 1
> > means enabled, and sometimes 1 means now disabled...
> > And maintain those weird code in device side and extra state bits burning
> > some expensive chip resource.
> > 
> > Is removing from 1.2 is equal delay to get is fixed in 1.3?
> > If yes, I make humble request to fix this and have errata.
> > Some of the professional standard bodies release the spec and short after
> > that errata/ratification follows the release that resolve such small issues.
> > May be time for virtio spec to take this opportunity now and be bit agile on it.
> > Your call. :)
> 
> 
> I think further, it appears a real bug that requires so special handling in the device for next years to carry.
> 
> Imagine this sequence.
> 1. A virtio device is handed over to guest VM
> 2. A virtio device started queue_reset sequence,
> So register values are:
> queue_enable = 1, q_reset=1
> 2. guest VM poled the q_reset register.
> Queue_enable = 1, q_reset = 0 (because device is doing the resetting the queue)
> 3. HV suspend the VM and queried the VQ state
> VQ state returned 
> q_enable=1, q_reset = 0.
> 
> HV doesn't know what q_reset=0 mean here, is it 0 because it was never reset?
> Or it is 0 because GVM Started the reset, but reset didn't finish?
> 
> When this virtio device is restored on the other side, HV and device doesn't know how to deal with this.
> 
> A WA that all devices will implement is, not returning 0, in step_2, but return say q_reset = 0xa to indicate that its other than 1 and other than 0.
> But hey, the destination side needs to treat this special 0xa and convert to internal q_yet_busy stage.
> 
> And this answer Jason and myself why queue_enable shouldn't be overloaded for this busy_wait register.
> Hence queue_reset register seems the right choice with the fix.
> 
> Starting to think of workaround even before the spec release is not elegant.
> Lets please fix this, bug effect is expanding beyond the primary virtio device itself.

Thanks Parav. Updating the github issue with more motivation would be a
good idea.

-- 
MST

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Michael S. Tsirkin]

On Wed, Apr 27, 2022 at 01:25:59PM +0300, Parav Pandit wrote:
> Currently when driver initiates a queue reset, device is expected
> to communicate reset status to the driver by changing the value of the
> queue_reset register twice. First to return value other than 1 when
> reset is ongoing, later to return 1 when queue reset is completed.
> 
> However initially during the device reset time the queue reset value
> is zero. queue_reset changes the value of the register to a different
> value on reset completion. Yet another time queue_reset value is
> expected to change when queue_select is reprogrammed.
> 
> For example in below flow, a created virtqueue, which is disabled
> by driver leaves the queue state as
> queue_enable = 0, queue_reset = 1.
> 
> example flow:
> a) 0,0 -> device init time value
> b) 1,0 -> vq is enabled by driver and working
> c) 1,1 -> vq is enabled, driver initiated reset
> d) 1,0 -> vq is enabled, but device is busy doing the reset
> e) 0,1 -> vq reset is completed in the device and VQ is now disabled
> (conflict with initial queue reset state #a above)
> 
> On next iteration, when queue_select selects the same VQ again,
> without enablement, device is confused to return 1 or 0 because
> it was reset once before via queue_reset register.
> 
> This demands complex device implementation to understand what
> should be returned for a VQ that is reset using queue_reset register
> vs other means.
> 
> Instead, it is better and efficient to maintain the same VQ state
> on the device when queue reset is completed.
> 
> new proposed flow:
> q_enable, q_reset
> A) 0, 0 -> default, device init time
> B) 1, 0 -> driver has enabled vq
> C) 1, 1 -> driver started q reset
> D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> (device communicates that its working on resetting VQ, consistent with #C)
> E) 0, 0 -> q_reset by device is completed, q got disabled
> (consistent with device init time #A)
> 
> Hence, this patch proposes a simple change to have reset register
> polarity to be same as that of initial reset value.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/139
> Fixes: 12998e738621 ("virtio: pci support virtqueue reset")
> Fixes: a4ce81a83780 ("virtio: mmio support virtqueue reset")
> Fixes: 3b5378d70a42 ("virtio: introduce virtqueue reset as basic facility")
> Signed-off-by: Parav Pandit <parav@nvidia.com>

Parav, spec comments need to be posted to virtio-comment please.


> ---
> changelog:
> v1->v2:
> - Fixed below comment from Xuan Zhuo
> - Fixed the configuration and register layout text to reflect the polarity
> v0->v1:
> - Fixed below comments from Michael
> - Removed example device side pseudo code for old and new polarity
> - Removed unwanted articles 'a' and 'the'
> - Dropped extra empty line
> ---
>  content.tex | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 060bdab..68b176b 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1039,7 +1039,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  \field{queue_reset} after the virtqueue is enabled with \field{queue_enable}.
>  
>  The device MUST reset the queue when 1 is written to \field{queue_reset}, and
> -present a 1 in \field{queue_reset} after the queue has been reset, until the
> +present 0 in \field{queue_reset} after the queue has been reset,

... ok

> until the
>  driver re-enables the queue via \field{queue_enable} or the device is reset.

this part is just confusing. neither reset nor queue_enable will set queue_reset.
just drop it?


>  The device MUST present consistent default values after queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> @@ -1069,10 +1069,13 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  The driver MUST NOT write a 0 to \field{queue_enable}.
>  
>  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> -\field{queue_reset} to reset the queue, it MUST verify that the queue
> -has been reset by reading back \field{queue_reset} and ensuring that it
> -is 1. The driver MAY re-enable the queue by writing a 1 to
> -\field{queue_enable} after ensuring that the other virtqueue fields have
> +\field{queue_reset} to reset the queue, driver MUST verify that the queue
> +has been reset by reading back \field{queue_reset} until device returns a value of 0.
> +Device continue

continues

> to report

we say present elsewhere, let's be consistent

> value of 1 for the \field{queue_reset} until device finishes
> +the queue reset request.

I'd just say "until after queue reset" consistent with what we say
elsewhere.

> When the device completes the queue reset, \field{queue_reset}
> +and \field{queue_enable} set to zero

are set to 0

>, indicating
> +that specified queue is ready to be enabled again. The driver MAY re-enable the queue by writing 1 to

the part describing how the device works should be moved to the device
section and be more specific with MUST/MAY.

> +\field{queue_enable} after ensuring that other virtqueue fields have
>  been set up correctly. The driver MAY set driver-writeable queue configuration
>  values to different values than those that were used before the queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> @@ -2015,7 +2018,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi

same comments below.

>  \field{QueueReset} after the virtqueue is enabled with \field{QueueReady}.
>  
>  The device MUST reset the queue when 1 is written to \field{QueueReset}, and
> -present a 1 in \field{QueueReset} after the queue has been reset, until the
> +present 0 in \field{QueueReset} after the queue has been reset, until the
>  driver re-enables the queue via \field{QueueReady} or until the device is reset.
>  The device MUST present consistent default values after queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> @@ -2063,10 +2066,13 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>  it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value.
>  
>  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> -\field{QueueReset} to reset the queue, it MUST verify that the queue
> -has been reset by reading back \field{QueueReset} and ensuring that it
> -is 1. The driver MAY re-enable the queue by writing a 1 to
> -\field{QueueReady} after ensuring that the other virtqueue fields have
> +\field{QueueReset} to reset the queue, driver MUST verify that the queue
> +has been reset by reading back \field{QueueReset} until device returns a value of 0.
> +Device continue to report value of 1 for the \field{QueueReset} until device finishes
> +the reset. When the device completes the queue reset, \field{QueueReset} and
> +\field{QueueReady} set to zero, indicating that specified queue is ready to be
> +enabled again. The driver MAY re-enable the queue by writing 1 to
> +\field{QueueReady} after ensuring that other virtqueue fields have
>  been set up correctly. The driver MAY set driver-writeable queue configuration
>  values to different values than those that were used before the queue reset.
>  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> -- 
> 1.8.3.1
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Wednesday, April 27, 2022 8:48 AM

[..]
> > +\field{queue_reset} to reset the queue, driver MUST verify that the
> > +queue
> 
> s/driver/the driver/
> 
Done.

> > +has been reset by reading back \field{queue_reset} until device returns a
> value of 0.
> 
> s/device/the device/
Done.

> 
> > +Device continue to report value of 1 for the \field{queue_reset}
> > +until device finishes the queue reset request.
> 
> This looks a bit odd: Shouldn't that go into the normative section for the
> device instead?
Yes, Michael also had same comment.
I am moving there.

> 
> > When the device completes the queue reset, \field{queue_reset}
> > +and \field{queue_enable} set to zero, indicating that specified queue
> > +is ready to be enabled again.
> 
> Same here. I think a statement that the driver MUST NOT consider queue
> reset to be complete until it reads back 0 in both queue_reset and
> queue_enable is more appropriate here.
> 
This was very useful. I had hard time put the wording as driver centric way.
Thanks a lot.

[..]

> Similar comments as for the pci section.

Fixed.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> Behalf Of Michael S. Tsirkin
> On Wed, Apr 27, 2022 at 01:25:59PM +0300, Parav Pandit wrote:
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> Parav, spec comments need to be posted to virtio-comment please.
> 
I missed it, you said previously too.
Added now starting this email and v3.


> >  The device MUST reset the queue when 1 is written to
> > \field{queue_reset}, and -present a 1 in \field{queue_reset} after the
> > queue has been reset, until the
> > +present 0 in \field{queue_reset} after the queue has been reset,
> 
> ... ok
> 
> > until the
> >  driver re-enables the queue via \field{queue_enable} or the device is
> reset.
> 
> this part is just confusing. neither reset nor queue_enable will set
> queue_reset.
> just drop it?
> 
Yeah.
I took Cornelia's suggested draft and rewrote the device and driver side description.

> > +\field{queue_reset} to reset the queue, driver MUST verify that the
> > +queue has been reset by reading back \field{queue_reset} until device
> returns a value of 0.
> > +Device continue
> 
> continues
> 
> > to report
> 
> we say present elsewhere, let's be consistent
> 
> > value of 1 for the \field{queue_reset} until device finishes
> > +the queue reset request.
> 
> I'd just say "until after queue reset" consistent with what we say elsewhere.
> 
> > When the device completes the queue reset, \field{queue_reset}
> > +and \field{queue_enable} set to zero
> 
> are set to 0
> 
> >, indicating
> > +that specified queue is ready to be enabled again. The driver MAY
> > +re-enable the queue by writing 1 to
> 
> the part describing how the device works should be moved to the device
> section and be more specific with MUST/MAY.
> 
> > +\field{queue_enable} after ensuring that other virtqueue fields have
> >  been set up correctly. The driver MAY set driver-writeable queue
> > configuration  values to different values than those that were used before
> the queue reset.
> >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue
> Reset}).
> > @@ -2015,7 +2018,7 @@ \subsection{MMIO Device Register
> > Layout}\label{sec:Virtio Transport Options / Vi
> 
> same comments below.
> 

All above comments are taken care now with Cornelia's wording for your input and by correctly updating device side section now.
Sending v3.
Thanks.

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, April 27, 2022 3:32 PM

> > I think further, it appears a real bug that requires so special handling in the
> device for next years to carry.
> >
> > Imagine this sequence.
> > 1. A virtio device is handed over to guest VM 2. A virtio device
> > started queue_reset sequence, So register values are:
> > queue_enable = 1, q_reset=1
> > 2. guest VM poled the q_reset register.
> > Queue_enable = 1, q_reset = 0 (because device is doing the resetting
> > the queue) 3. HV suspend the VM and queried the VQ state VQ state
> > returned q_enable=1, q_reset = 0.
> >
> > HV doesn't know what q_reset=0 mean here, is it 0 because it was never
> reset?
> > Or it is 0 because GVM Started the reset, but reset didn't finish?
> >
> > When this virtio device is restored on the other side, HV and device doesn't
> know how to deal with this.
> >
> > A WA that all devices will implement is, not returning 0, in step_2, but
> return say q_reset = 0xa to indicate that its other than 1 and other than 0.
> > But hey, the destination side needs to treat this special 0xa and convert to
> internal q_yet_busy stage.
> >
> > And this answer Jason and myself why queue_enable shouldn't be
> overloaded for this busy_wait register.
> > Hence queue_reset register seems the right choice with the fix.
> >
> > Starting to think of workaround even before the spec release is not
> elegant.
> > Lets please fix this, bug effect is expanding beyond the primary virtio
> device itself.
> 
> Thanks Parav. Updating the github issue with more motivation would be a
> good idea.
Done.

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Jason Wang]

On Wed, Apr 27, 2022 at 11:39 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, April 27, 2022 11:30 AM
> >
> > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > >
> > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > > example flow:
> > > > > a) 0,0 -> device init time value
> > > > > b) 1,0 -> vq is enabled by driver and working
> > > >
> > > > Did you see my reply in V1? What's the reason for using write to
> > > > clear behavior that is different from the device status?
> > > >
> > > > We can simply make this as 1, 1 here and let the driver write to 0
> > > > to reset the virtqueue.
> > > >
> > > > And if we do this, the queue_enable and queue_reset are always the
> > > > same, then we can simply reuse queue_enable.
> > > >
> > > Yes, I know we can make this work using new feature bit + single
> > queue_enable register.
> > > I replied that in v0 to Michael.
> >
> > A bigger question in my eyes is that down the road we might want to be able to
> > stop the ring without having it lose state.
> > The natural interface for that seems to be writing 0 to queue enable.
> Why queue_enable and not queue_reset?
>
> to me this interface is unlikely performant and useful for such case.
> When we want to pause/stop the VQ and query the state we need performant scheme, that can even work in a batch for all the VQs.
> At that point programming 64 registers to pause/stop VQ without losing state and querying its indices etc won't be scalable with register interface.

The register interface to sync indices has already been implemented in
real hardware for years.

> I imagine a AQ (likely) or some other interface.

So did the queue_enable registers, we need to write 1 to queue_enable
for each virtqueue before DRIVER_OK.

Where to allow writing 0 to queue_enable is orthogonal to scalability.

Thanks

>
> >
> > > I was not sure how drastic that would be at this point in the spec release cycle
> > that Michael highlighted.
> > > Hence, I proposed a minimal change fix to queue_reset register given timeline.
> >
> > Well if accepted this proposal is going to delay the release anyway.  If we are
> > doing a new feature then that can love alongside the one that is already in the
> > spec.
> I didn't quite understand your point.
> This queue_reset in its current state (with/without) this proposed fix is mostly usable within the guest for dynamic VQ creation/deletion to connect to ethtool/xdp or more.
>
> I don't see a need to delay the fix, to a larger feature that needs more than just start/stop button.
>

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, April 27, 2022 11:15 PM
> 
> On Wed, Apr 27, 2022 at 11:39 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, April 27, 2022 11:30 AM
> > >
> > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > >
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > >
> > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com>
> wrote:
> > > > > >
> > > > > > example flow:
> > > > > > a) 0,0 -> device init time value
> > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > >
> > > > > Did you see my reply in V1? What's the reason for using write to
> > > > > clear behavior that is different from the device status?
> > > > >
> > > > > We can simply make this as 1, 1 here and let the driver write to
> > > > > 0 to reset the virtqueue.
> > > > >
> > > > > And if we do this, the queue_enable and queue_reset are always
> > > > > the same, then we can simply reuse queue_enable.
> > > > >
> > > > Yes, I know we can make this work using new feature bit + single
> > > queue_enable register.
> > > > I replied that in v0 to Michael.
> > >
> > > A bigger question in my eyes is that down the road we might want to
> > > be able to stop the ring without having it lose state.
> > > The natural interface for that seems to be writing 0 to queue enable.
> > Why queue_enable and not queue_reset?
> >
> > to me this interface is unlikely performant and useful for such case.
> > When we want to pause/stop the VQ and query the state we need
> performant scheme, that can even work in a batch for all the VQs.
> > At that point programming 64 registers to pause/stop VQ without losing
> state and querying its indices etc won't be scalable with register interface.
> 
> The register interface to sync indices has already been implemented in real
> hardware for years.
> 
Sure. I meant to that when we want to pause a VQ and restart later use-case will require more plumbing than just enable/disable register.
And to do that, a register interface won't be performant/scalable. So for the wider use case this may not be the good choice.

And I explained the other reason that we lose the state information with this busy-wait register in the other reply to V2 and summary in the github issue too on Michael's request.

> > I imagine a AQ (likely) or some other interface.
> 
> So did the queue_enable registers, we need to write 1 to queue_enable for
> each virtqueue before DRIVER_OK.
> 
> Where to allow writing 0 to queue_enable is orthogonal to scalability.

Sure, lets sync after you get chance to go through my other reply to Michael about why with single busy-wait register we lose the state.
And hence, queue_reset register (with the fix) is better.

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Jason Wang]

在 2022/4/28 00:15, Parav Pandit 写道:
>> From: Parav Pandit
>> Sent: Wednesday, April 27, 2022 11:58 AM
>>
>> But this is so basic.
>> It's hard to gaze at this spec for coming years and the code to see, Hey
>> sometimes 0 means disabled, sometime 0 means still enabled, sometime 1
>> means enabled, and sometimes 1 means now disabled...
>> And maintain those weird code in device side and extra state bits burning
>> some expensive chip resource.
>>
>> Is removing from 1.2 is equal delay to get is fixed in 1.3?
>> If yes, I make humble request to fix this and have errata.
>> Some of the professional standard bodies release the spec and short after
>> that errata/ratification follows the release that resolve such small issues.
>> May be time for virtio spec to take this opportunity now and be bit agile on it.
>> Your call. :)
>
> I think further, it appears a real bug that requires so special handling in the device for next years to carry.
>
> Imagine this sequence.
> 1. A virtio device is handed over to guest VM
> 2. A virtio device started queue_reset sequence,
> So register values are:
> queue_enable = 1, q_reset=1
> 2. guest VM poled the q_reset register.
> Queue_enable = 1, q_reset = 0 (because device is doing the resetting the queue)
> 3. HV suspend the VM and queried the VQ state


Hypervisor needs to trap the access to q_reset, otherwise there will be 
a race anyhow. So it knows the device is being reset and only after the 
device rest is finished, it can suspend a VM.


> VQ state returned
> q_enable=1, q_reset = 0.
>
> HV doesn't know what q_reset=0 mean here, is it 0 because it was never reset?
> Or it is 0 because GVM Started the reset, but reset didn't finish?


If the device can change any register value (e.g q_reset), it's not 
stopped. We can't do the migration anyhow. No?


>
> When this virtio device is restored on the other side, HV and device doesn't know how to deal with this.
>
> A WA that all devices will implement is, not returning 0, in step_2, but return say q_reset = 0xa to indicate that its other than 1 and other than 0.
> But hey, the destination side needs to treat this special 0xa and convert to internal q_yet_busy stage.
>
> And this answer Jason and myself why queue_enable shouldn't be overloaded for this busy_wait register.


Well, we've already had busy waiting register that is device_status.

"""

After writing 0 todevice_status, the driver MUST wait for a read 
ofdevice_statusto return 0 before reinitializing the device.

"""

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);
}

We don't see any live migration issue with this.

Thanks


> Hence queue_reset register seems the right choice with the fix.
>
> Starting to think of workaround even before the spec release is not elegant.
> Lets please fix this, bug effect is expanding beyond the primary virtio device itself.
>

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Jason Wang]

在 2022/4/28 11:24, Parav Pandit 写道:
>
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Wednesday, April 27, 2022 11:15 PM
>>
>> On Wed, Apr 27, 2022 at 11:39 PM Parav Pandit <parav@nvidia.com> wrote:
>>>
>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>> Sent: Wednesday, April 27, 2022 11:30 AM
>>>>
>>>> On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
>>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>>> Sent: Wednesday, April 27, 2022 7:30 AM
>>>>>>
>>>>>> On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com>
>> wrote:
>>>>>>> example flow:
>>>>>>> a) 0,0 -> device init time value
>>>>>>> b) 1,0 -> vq is enabled by driver and working
>>>>>> Did you see my reply in V1? What's the reason for using write to
>>>>>> clear behavior that is different from the device status?
>>>>>>
>>>>>> We can simply make this as 1, 1 here and let the driver write to
>>>>>> 0 to reset the virtqueue.
>>>>>>
>>>>>> And if we do this, the queue_enable and queue_reset are always
>>>>>> the same, then we can simply reuse queue_enable.
>>>>>>
>>>>> Yes, I know we can make this work using new feature bit + single
>>>> queue_enable register.
>>>>> I replied that in v0 to Michael.
>>>> A bigger question in my eyes is that down the road we might want to
>>>> be able to stop the ring without having it lose state.
>>>> The natural interface for that seems to be writing 0 to queue enable.
>>> Why queue_enable and not queue_reset?
>>>
>>> to me this interface is unlikely performant and useful for such case.
>>> When we want to pause/stop the VQ and query the state we need
>> performant scheme, that can even work in a batch for all the VQs.
>>> At that point programming 64 registers to pause/stop VQ without losing
>> state and querying its indices etc won't be scalable with register interface.
>>
>> The register interface to sync indices has already been implemented in real
>> hardware for years.
>>
> Sure. I meant to that when we want to pause a VQ and restart later use-case will require more plumbing than just enable/disable register.
> And to do that, a register interface won't be performant/scalable. So for the wider use case this may not be the good choice.
>
> And I explained the other reason that we lose the state information with this busy-wait register in the other reply to V2 and summary in the github issue too on Michael's request.


I've replied to the thread.


>
>>> I imagine a AQ (likely) or some other interface.
>> So did the queue_enable registers, we need to write 1 to queue_enable for
>> each virtqueue before DRIVER_OK.
>>
>> Where to allow writing 0 to queue_enable is orthogonal to scalability.
> Sure, lets sync after you get chance to go through my other reply to Michael about why with single busy-wait register we lose the state.
> And hence, queue_reset register (with the fix) is better.


Note that the MMIO transport allows writing 0 to queue_ready.

Thanks

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Jason Wang]

On Wed, Apr 27, 2022 at 7:46 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 27 Apr 2022 19:29:37 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > > Currently when driver initiates a queue reset, device is expected
> > > to communicate reset status to the driver by changing the value of the
> > > queue_reset register twice. First to return value other than 1 when
> > > reset is ongoing, later to return 1 when queue reset is completed.
> > >
> > > However initially during the device reset time the queue reset value
> > > is zero. queue_reset changes the value of the register to a different
> > > value on reset completion. Yet another time queue_reset value is
> > > expected to change when queue_select is reprogrammed.
> > >
> > > For example in below flow, a created virtqueue, which is disabled
> > > by driver leaves the queue state as
> > > queue_enable = 0, queue_reset = 1.
> > >
> > > example flow:
> > > a) 0,0 -> device init time value
> > > b) 1,0 -> vq is enabled by driver and working
> >
> > Did you see my reply in V1? What's the reason for using write to clear
> > behavior that is different from the device status?
> >
> > We can simply make this as 1, 1 here and let the driver write to 0 to
> > reset the virtqueue.
> >
> > And if we do this, the queue_enable and queue_reset are always the
> > same, then we can simply reuse queue_enable.
>
>
> The reason I added a new item(queue_reset) at that time was because there was
> such a sentence:
>
>         The driver MUST NOT write a 0 to \field{queue_enable}.
>
> Thanks.

Right, but this is not something we can't change with a new feature

If VIRTIO_F_RING_RESET is not negotiated, the driver MUST NOT write 0 ...

Thanks

>
>
> >
> > Thanks
> >
> > > c) 1,1 -> vq is enabled, driver initiated reset
> > > d) 1,0 -> vq is enabled, but device is busy doing the reset
> > > e) 0,1 -> vq reset is completed in the device and VQ is now disabled
> > > (conflict with initial queue reset state #a above)
> > >
> > > On next iteration, when queue_select selects the same VQ again,
> > > without enablement, device is confused to return 1 or 0 because
> > > it was reset once before via queue_reset register.
> > >
> > > This demands complex device implementation to understand what
> > > should be returned for a VQ that is reset using queue_reset register
> > > vs other means.
> > >
> > > Instead, it is better and efficient to maintain the same VQ state
> > > on the device when queue reset is completed.
> > >
> > > new proposed flow:
> > > q_enable, q_reset
> > > A) 0, 0 -> default, device init time
> > > B) 1, 0 -> driver has enabled vq
> > > C) 1, 1 -> driver started q reset
> > > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq
> > > (device communicates that its working on resetting VQ, consistent with #C)
> > > E) 0, 0 -> q_reset by device is completed, q got disabled
> > > (consistent with device init time #A)
> > >
> > > Hence, this patch proposes a simple change to have reset register
> > > polarity to be same as that of initial reset value.
> > >
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/139
> > > Fixes: 12998e738621 ("virtio: pci support virtqueue reset")
> > > Fixes: a4ce81a83780 ("virtio: mmio support virtqueue reset")
> > > Fixes: 3b5378d70a42 ("virtio: introduce virtqueue reset as basic facility")
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > >
> > > ---
> > > changelog:
> > > v1->v2:
> > > - Fixed below comment from Xuan Zhuo
> > > - Fixed the configuration and register layout text to reflect the polarity
> > > v0->v1:
> > > - Fixed below comments from Michael
> > > - Removed example device side pseudo code for old and new polarity
> > > - Removed unwanted articles 'a' and 'the'
> > > - Dropped extra empty line
> > > ---
> > >  content.tex | 26 ++++++++++++++++----------
> > >  1 file changed, 16 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/content.tex b/content.tex
> > > index 060bdab..68b176b 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -1039,7 +1039,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> > >  \field{queue_reset} after the virtqueue is enabled with \field{queue_enable}.
> > >
> > >  The device MUST reset the queue when 1 is written to \field{queue_reset}, and
> > > -present a 1 in \field{queue_reset} after the queue has been reset, until the
> > > +present 0 in \field{queue_reset} after the queue has been reset, until the
> > >  driver re-enables the queue via \field{queue_enable} or the device is reset.
> > >  The device MUST present consistent default values after queue reset.
> > >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> > > @@ -1069,10 +1069,13 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> > >  The driver MUST NOT write a 0 to \field{queue_enable}.
> > >
> > >  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> > > -\field{queue_reset} to reset the queue, it MUST verify that the queue
> > > -has been reset by reading back \field{queue_reset} and ensuring that it
> > > -is 1. The driver MAY re-enable the queue by writing a 1 to
> > > -\field{queue_enable} after ensuring that the other virtqueue fields have
> > > +\field{queue_reset} to reset the queue, driver MUST verify that the queue
> > > +has been reset by reading back \field{queue_reset} until device returns a value of 0.
> > > +Device continue to report value of 1 for the \field{queue_reset} until device finishes
> > > +the queue reset request. When the device completes the queue reset, \field{queue_reset}
> > > +and \field{queue_enable} set to zero, indicating
> > > +that specified queue is ready to be enabled again. The driver MAY re-enable the queue by writing 1 to
> > > +\field{queue_enable} after ensuring that other virtqueue fields have
> > >  been set up correctly. The driver MAY set driver-writeable queue configuration
> > >  values to different values than those that were used before the queue reset.
> > >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> > > @@ -2015,7 +2018,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> > >  \field{QueueReset} after the virtqueue is enabled with \field{QueueReady}.
> > >
> > >  The device MUST reset the queue when 1 is written to \field{QueueReset}, and
> > > -present a 1 in \field{QueueReset} after the queue has been reset, until the
> > > +present 0 in \field{QueueReset} after the queue has been reset, until the
> > >  driver re-enables the queue via \field{QueueReady} or until the device is reset.
> > >  The device MUST present consistent default values after queue reset.
> > >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> > > @@ -2063,10 +2066,13 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
> > >  it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value.
> > >
> > >  If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to
> > > -\field{QueueReset} to reset the queue, it MUST verify that the queue
> > > -has been reset by reading back \field{QueueReset} and ensuring that it
> > > -is 1. The driver MAY re-enable the queue by writing a 1 to
> > > -\field{QueueReady} after ensuring that the other virtqueue fields have
> > > +\field{QueueReset} to reset the queue, driver MUST verify that the queue
> > > +has been reset by reading back \field{QueueReset} until device returns a value of 0.
> > > +Device continue to report value of 1 for the \field{QueueReset} until device finishes
> > > +the reset. When the device completes the queue reset, \field{QueueReset} and
> > > +\field{QueueReady} set to zero, indicating that specified queue is ready to be
> > > +enabled again. The driver MAY re-enable the queue by writing 1 to
> > > +\field{QueueReady} after ensuring that other virtqueue fields have
> > >  been set up correctly. The driver MAY set driver-writeable queue configuration
> > >  values to different values than those that were used before the queue reset.
> > >  (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
> > > --
> > > 1.8.3.1
> > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > >
> >
>

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, April 27, 2022 11:41 PM
> 
> 
> Hypervisor needs to trap the access to q_reset, otherwise there will be a
> race anyhow. So it knows the device is being reset and only after the device
> rest is finished, it can suspend a VM.
> 
The state can come from the device directly without the trap via HV to device direct access.

> 
> > VQ state returned
> > q_enable=1, q_reset = 0.
> >
> > HV doesn't know what q_reset=0 mean here, is it 0 because it was never
> reset?
> > Or it is 0 because GVM Started the reset, but reset didn't finish?
> 
> 
> If the device can change any register value (e.g q_reset), it's not stopped.
> We can't do the migration anyhow. No?
The state is in the device, so depending on the device type it may be able to store its internal state.

> 
> 
> >
> > When this virtio device is restored on the other side, HV and device doesn't
> know how to deal with this.
> >
> > A WA that all devices will implement is, not returning 0, in step_2, but
> return say q_reset = 0xa to indicate that its other than 1 and other than 0.
> > But hey, the destination side needs to treat this special 0xa and convert to
> internal q_yet_busy stage.
> >
> > And this answer Jason and myself why queue_enable shouldn't be
> overloaded for this busy_wait register.
> 
> 
> Well, we've already had busy waiting register that is device_status.
> 
> """
> 
> After writing 0 todevice_status, the driver MUST wait for a read
> ofdevice_statusto return 0 before reinitializing the device.
> 
I think yes, we can overload q_enable busy wait to register like device_status.
Q_enable turns zero only when queue is disabled, till that time driver polls for it and gets 1.

In this approach, external query entity will not be able to see ongoing queue reset.
This may be ok because device is anyway maintaining its internal state regardless.

queue_reset removal has far more editorial changes.
Before I update the v4 to incorporate, please discuss/sync with Michael or others what is best course of action from spec timing perspective.
 I am ok either way to draft either by 
(a) removal of queue_reset register and allow queue_disable by replacing RING_RESET definition in v4
Or
(b) fixing the polarity as done in v3

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Michael S. Tsirkin]

On Thu, Apr 28, 2022 at 11:43:16AM +0800, Jason Wang wrote:
> 
> 在 2022/4/28 11:24, Parav Pandit 写道:
> > 
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Wednesday, April 27, 2022 11:15 PM
> > > 
> > > On Wed, Apr 27, 2022 at 11:39 PM Parav Pandit <parav@nvidia.com> wrote:
> > > > 
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Wednesday, April 27, 2022 11:30 AM
> > > > > 
> > > > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > > > > 
> > > > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com>
> > > wrote:
> > > > > > > > example flow:
> > > > > > > > a) 0,0 -> device init time value
> > > > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > > > > Did you see my reply in V1? What's the reason for using write to
> > > > > > > clear behavior that is different from the device status?
> > > > > > > 
> > > > > > > We can simply make this as 1, 1 here and let the driver write to
> > > > > > > 0 to reset the virtqueue.
> > > > > > > 
> > > > > > > And if we do this, the queue_enable and queue_reset are always
> > > > > > > the same, then we can simply reuse queue_enable.
> > > > > > > 
> > > > > > Yes, I know we can make this work using new feature bit + single
> > > > > queue_enable register.
> > > > > > I replied that in v0 to Michael.
> > > > > A bigger question in my eyes is that down the road we might want to
> > > > > be able to stop the ring without having it lose state.
> > > > > The natural interface for that seems to be writing 0 to queue enable.
> > > > Why queue_enable and not queue_reset?
> > > > 
> > > > to me this interface is unlikely performant and useful for such case.
> > > > When we want to pause/stop the VQ and query the state we need
> > > performant scheme, that can even work in a batch for all the VQs.
> > > > At that point programming 64 registers to pause/stop VQ without losing
> > > state and querying its indices etc won't be scalable with register interface.
> > > 
> > > The register interface to sync indices has already been implemented in real
> > > hardware for years.
> > > 
> > Sure. I meant to that when we want to pause a VQ and restart later use-case will require more plumbing than just enable/disable register.
> > And to do that, a register interface won't be performant/scalable. So for the wider use case this may not be the good choice.
> > 
> > And I explained the other reason that we lose the state information with this busy-wait register in the other reply to V2 and summary in the github issue too on Michael's request.
> 
> 
> I've replied to the thread.
> 
> 
> > 
> > > > I imagine a AQ (likely) or some other interface.
> > > So did the queue_enable registers, we need to write 1 to queue_enable for
> > > each virtqueue before DRIVER_OK.
> > > 
> > > Where to allow writing 0 to queue_enable is orthogonal to scalability.
> > Sure, lets sync after you get chance to go through my other reply to Michael about why with single busy-wait register we lose the state.
> > And hence, queue_reset register (with the fix) is better.
> 
> 
> Note that the MMIO transport allows writing 0 to queue_ready.
> 
> Thanks

... without the side effect of resetting the queue state.
I think ideally there should be text in MMIO stating it does not
support the reset feature at the moment, so devices should not expose
this feature and drivers should not acknowledge this if exposed by device
but should not fail if it's exposed (for future expansion).
This is an existing spec issue, should be a separate patch.

Regarding MMIO it is not exactly clear but I think the assumption
always was you don't disable queues after DRIVER_OK.

Just thinking aloud, I am wondering:


So in the proposal, the new queue reset register does two things:
- reset the internal queue state
- disable the queue


It seems possible to split the functionality: queue reset would just
disable counters, queue enable would disable queue. If we do
that:
1- We need to specify what happens if we reset counters while ring is enabled.
   I guess queue then has to go until ring is empty, then stop?
   Draining queue like this has value for e.g. snapshots.
   An alternative is to prohibit this transition.

   By the way current spec does not make it clear what happens if
   driver keeps adding buffers to the queue after writing to
   reset. It's probably a good idea to specify that device
   should not wait to drain the queue, it should just reset,
   but again this is an existing defect so maybe a separate patch.

2- what happens if queue is disabled but
   without reset, and queue size changes? We do not specify.
   Maybe that has to be prohibited.

I conclude that at some point, there is a chance we will want
to add VIRTIO_RING_F_DISABLE making queue enable writeable for
all transports after DRIVER_OK.
If we do, we might want the reset register behaviour to change
if VIRTIO_RING_F_DISABLE has been negotiated.
Parav from hardware point of view, would you say such a register where
behaviour changes depending on feature bit be too messy?


Overall, this makes me feel existing two register interface is
fine, reusing queue enable would make future expansion harder
while making queue enable writeable would be a new feature
and needs its own feature bit.


-- 
MST

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Jason Wang]

On Thu, Apr 28, 2022 at 12:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Apr 28, 2022 at 11:43:16AM +0800, Jason Wang wrote:
> >
> > 在 2022/4/28 11:24, Parav Pandit 写道:
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Wednesday, April 27, 2022 11:15 PM
> > > >
> > > > On Wed, Apr 27, 2022 at 11:39 PM Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Sent: Wednesday, April 27, 2022 11:30 AM
> > > > > >
> > > > > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > > > > >
> > > > > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com>
> > > > wrote:
> > > > > > > > > example flow:
> > > > > > > > > a) 0,0 -> device init time value
> > > > > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > > > > > Did you see my reply in V1? What's the reason for using write to
> > > > > > > > clear behavior that is different from the device status?
> > > > > > > >
> > > > > > > > We can simply make this as 1, 1 here and let the driver write to
> > > > > > > > 0 to reset the virtqueue.
> > > > > > > >
> > > > > > > > And if we do this, the queue_enable and queue_reset are always
> > > > > > > > the same, then we can simply reuse queue_enable.
> > > > > > > >
> > > > > > > Yes, I know we can make this work using new feature bit + single
> > > > > > queue_enable register.
> > > > > > > I replied that in v0 to Michael.
> > > > > > A bigger question in my eyes is that down the road we might want to
> > > > > > be able to stop the ring without having it lose state.
> > > > > > The natural interface for that seems to be writing 0 to queue enable.
> > > > > Why queue_enable and not queue_reset?
> > > > >
> > > > > to me this interface is unlikely performant and useful for such case.
> > > > > When we want to pause/stop the VQ and query the state we need
> > > > performant scheme, that can even work in a batch for all the VQs.
> > > > > At that point programming 64 registers to pause/stop VQ without losing
> > > > state and querying its indices etc won't be scalable with register interface.
> > > >
> > > > The register interface to sync indices has already been implemented in real
> > > > hardware for years.
> > > >
> > > Sure. I meant to that when we want to pause a VQ and restart later use-case will require more plumbing than just enable/disable register.
> > > And to do that, a register interface won't be performant/scalable. So for the wider use case this may not be the good choice.
> > >
> > > And I explained the other reason that we lose the state information with this busy-wait register in the other reply to V2 and summary in the github issue too on Michael's request.
> >
> >
> > I've replied to the thread.
> >
> >
> > >
> > > > > I imagine a AQ (likely) or some other interface.
> > > > So did the queue_enable registers, we need to write 1 to queue_enable for
> > > > each virtqueue before DRIVER_OK.
> > > >
> > > > Where to allow writing 0 to queue_enable is orthogonal to scalability.
> > > Sure, lets sync after you get chance to go through my other reply to Michael about why with single busy-wait register we lose the state.
> > > And hence, queue_reset register (with the fix) is better.
> >
> >
> > Note that the MMIO transport allows writing 0 to queue_ready.
> >
> > Thanks
>
> ... without the side effect of resetting the queue state.

It's unclear in the spec that whether or not we had such side effects.

> I think ideally there should be text in MMIO stating it does not
> support the reset feature at the moment,

So do you mean the state is held after writing 0 to queue_ready? What
happen if driver do

write 0 to queue_enable,
avail_idx = 0,
write 1 to queue_enable,

?

> so devices should not expose
> this feature and drivers should not acknowledge this if exposed by device
> but should not fail if it's exposed (for future expansion).

I'm not sure I understand this, but it looks too late to do that? (We
don't have a feature for this).

Or we can simply forbid writing 0 to queue_ready until we refine this
behaviour with a feature.

> This is an existing spec issue, should be a separate patch.
>
> Regarding MMIO it is not exactly clear but I think the assumption
> always was you don't disable queues after DRIVER_OK.
>
> Just thinking aloud, I am wondering:
>
>
> So in the proposal, the new queue reset register does two things:
> - reset the internal queue state

Does the reset also mean stop?

> - disable the queue

What's the benefit of separating the functions here?

>
>
> It seems possible to split the functionality: queue reset would just
> disable counters, queue enable would disable queue.

This requires hardware to synchronize the queue_reset with datapath
internally which looks a little bit complicated.

> If we do
> that:
> 1- We need to specify what happens if we reset counters while ring is enabled.
>    I guess queue then has to go until ring is empty, then stop?
>    Draining queue like this has value for e.g. snapshots.
>    An alternative is to prohibit this transition.

Does this mean we only support queue_reset after disabling the queue?
If yes, this basically go back to using a single queue_enable
register.

>
>    By the way current spec does not make it clear what happens if
>    driver keeps adding buffers to the queue after writing to
>    reset. It's probably a good idea to specify that device
>    should not wait to drain the queue, it should just reset,
>    but again this is an existing defect so maybe a separate patch.
>
> 2- what happens if queue is disabled but
>    without reset, and queue size changes? We do not specify.
>    Maybe that has to be prohibited.

Yes, after we enable a queue.

>
> I conclude that at some point, there is a chance we will want
> to add VIRTIO_RING_F_DISABLE making queue enable writeable for
> all transports after DRIVER_OK.

It looks to me it's better to have a single feature for disable/rest.
And it's self-contained if:

1) it holds a virtualenv state (indices)
2) allows the driver to modify the indices after it has been stopped
(with another feature).

This seems to cover all the use cases.

Thanks

> If we do, we might want the reset register behaviour to change
> if VIRTIO_RING_F_DISABLE has been negotiated.
> Parav from hardware point of view, would you say such a register where
> behaviour changes depending on feature bit be too messy?
>
>
> Overall, this makes me feel existing two register interface is
> fine, reusing queue enable would make future expansion harder
> while making queue enable writeable would be a new feature
> and needs its own feature bit.
>
>
> --
> MST
>

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Jason Wang]

On Thu, Apr 28, 2022 at 12:00 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, April 27, 2022 11:41 PM
> >
> >
> > Hypervisor needs to trap the access to q_reset, otherwise there will be a
> > race anyhow. So it knows the device is being reset and only after the device
> > rest is finished, it can suspend a VM.
> >
> The state can come from the device directly without the trap via HV to device direct access.

So this complicates the implementation of the hardware:

1) it needs to track a state like busy resetting which seems unnecessary
2) in the case of nesting, we need to trap anyhow

>
> >
> > > VQ state returned
> > > q_enable=1, q_reset = 0.
> > >
> > > HV doesn't know what q_reset=0 mean here, is it 0 because it was never
> > reset?
> > > Or it is 0 because GVM Started the reset, but reset didn't finish?
> >
> >
> > If the device can change any register value (e.g q_reset), it's not stopped.
> > We can't do the migration anyhow. No?
> The state is in the device, so depending on the device type it may be able to store its internal state.
>
> >
> >
> > >
> > > When this virtio device is restored on the other side, HV and device doesn't
> > know how to deal with this.
> > >
> > > A WA that all devices will implement is, not returning 0, in step_2, but
> > return say q_reset = 0xa to indicate that its other than 1 and other than 0.
> > > But hey, the destination side needs to treat this special 0xa and convert to
> > internal q_yet_busy stage.
> > >
> > > And this answer Jason and myself why queue_enable shouldn't be
> > overloaded for this busy_wait register.
> >
> >
> > Well, we've already had busy waiting register that is device_status.
> >
> > """
> >
> > After writing 0 todevice_status, the driver MUST wait for a read
> > ofdevice_statusto return 0 before reinitializing the device.
> >
> I think yes, we can overload q_enable busy wait to register like device_status.
> Q_enable turns zero only when queue is disabled, till that time driver polls for it and gets 1.
>
> In this approach, external query entity will not be able to see ongoing queue reset.

Yes, this can simplify a lot, there's no much value in migrating
ongoing queue rest.

> This may be ok because device is anyway maintaining its internal state regardless.
>
> queue_reset removal has far more editorial changes.
> Before I update the v4 to incorporate, please discuss/sync with Michael or others what is best course of action from spec timing perspective.
>  I am ok either way to draft either by
> (a) removal of queue_reset register and allow queue_disable by replacing RING_RESET definition in v4
> Or
> (b) fixing the polarity as done in v3

Ok.

Thanks

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Michael S. Tsirkin]

On Thu, Apr 28, 2022 at 02:10:46PM +0800, Jason Wang wrote:
> On Thu, Apr 28, 2022 at 12:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Apr 28, 2022 at 11:43:16AM +0800, Jason Wang wrote:
> > >
> > > 在 2022/4/28 11:24, Parav Pandit 写道:
> > > >
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Wednesday, April 27, 2022 11:15 PM
> > > > >
> > > > > On Wed, Apr 27, 2022 at 11:39 PM Parav Pandit <parav@nvidia.com> wrote:
> > > > > >
> > > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > Sent: Wednesday, April 27, 2022 11:30 AM
> > > > > > >
> > > > > > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > > > > > >
> > > > > > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com>
> > > > > wrote:
> > > > > > > > > > example flow:
> > > > > > > > > > a) 0,0 -> device init time value
> > > > > > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > > > > > > Did you see my reply in V1? What's the reason for using write to
> > > > > > > > > clear behavior that is different from the device status?
> > > > > > > > >
> > > > > > > > > We can simply make this as 1, 1 here and let the driver write to
> > > > > > > > > 0 to reset the virtqueue.
> > > > > > > > >
> > > > > > > > > And if we do this, the queue_enable and queue_reset are always
> > > > > > > > > the same, then we can simply reuse queue_enable.
> > > > > > > > >
> > > > > > > > Yes, I know we can make this work using new feature bit + single
> > > > > > > queue_enable register.
> > > > > > > > I replied that in v0 to Michael.
> > > > > > > A bigger question in my eyes is that down the road we might want to
> > > > > > > be able to stop the ring without having it lose state.
> > > > > > > The natural interface for that seems to be writing 0 to queue enable.
> > > > > > Why queue_enable and not queue_reset?
> > > > > >
> > > > > > to me this interface is unlikely performant and useful for such case.
> > > > > > When we want to pause/stop the VQ and query the state we need
> > > > > performant scheme, that can even work in a batch for all the VQs.
> > > > > > At that point programming 64 registers to pause/stop VQ without losing
> > > > > state and querying its indices etc won't be scalable with register interface.
> > > > >
> > > > > The register interface to sync indices has already been implemented in real
> > > > > hardware for years.
> > > > >
> > > > Sure. I meant to that when we want to pause a VQ and restart later use-case will require more plumbing than just enable/disable register.
> > > > And to do that, a register interface won't be performant/scalable. So for the wider use case this may not be the good choice.
> > > >
> > > > And I explained the other reason that we lose the state information with this busy-wait register in the other reply to V2 and summary in the github issue too on Michael's request.
> > >
> > >
> > > I've replied to the thread.
> > >
> > >
> > > >
> > > > > > I imagine a AQ (likely) or some other interface.
> > > > > So did the queue_enable registers, we need to write 1 to queue_enable for
> > > > > each virtqueue before DRIVER_OK.
> > > > >
> > > > > Where to allow writing 0 to queue_enable is orthogonal to scalability.
> > > > Sure, lets sync after you get chance to go through my other reply to Michael about why with single busy-wait register we lose the state.
> > > > And hence, queue_reset register (with the fix) is better.
> > >
> > >
> > > Note that the MMIO transport allows writing 0 to queue_ready.
> > >
> > > Thanks
> >
> > ... without the side effect of resetting the queue state.
> 
> It's unclear in the spec that whether or not we had such side effects.

...and that makes it pretty useless without a new feature bit

> > I think ideally there should be text in MMIO stating it does not
> > support the reset feature at the moment,
> 
> So do you mean the state is held after writing 0 to queue_ready?

I said nothing about QueueReady. I just said that we did not
add the reset register to MMIO yet, MMIO drivers should be
careful not to negotiate the reset feature bit until we
do and they know the reset register offset.

> What
> happen if driver do
> 
> write 0 to queue_enable,
> avail_idx = 0,
> write 1 to queue_enable,
> 
> ?
> 
> > so devices should not expose
> > this feature and drivers should not acknowledge this if exposed by device
> > but should not fail if it's exposed (for future expansion).
> 
> I'm not sure I understand this, but it looks too late to do that? (We
> don't have a feature for this).
> 
> Or we can simply forbid writing 0 to queue_ready until we refine this
> behaviour with a feature.
> 
> > This is an existing spec issue, should be a separate patch.
> >
> > Regarding MMIO it is not exactly clear but I think the assumption
> > always was you don't disable queues after DRIVER_OK.
> >
> > Just thinking aloud, I am wondering:
> >
> >
> > So in the proposal, the new queue reset register does two things:
> > - reset the internal queue state
> 
> Does the reset also mean stop?

After a queue has been reset by the driver, the device MUST NOT execute
any requests from that virtqueue, or notify the driver for it.

so apparently yes


> > - disable the queue
> 
> What's the benefit of separating the functions here?

I am not separating anything *here*, just listing what happens.
Below I list some ideas for why disable without reset might be useful.

> >
> >
> > It seems possible to split the functionality: queue reset would just
> > disable counters, queue enable would disable queue.
> 
> This requires hardware to synchronize the queue_reset with datapath
> internally which looks a little bit complicated.

this is already the case.

> > If we do
> > that:
> > 1- We need to specify what happens if we reset counters while ring is enabled.
> >    I guess queue then has to go until ring is empty, then stop?
> >    Draining queue like this has value for e.g. snapshots.
> >    An alternative is to prohibit this transition.
> 
> Does this mean we only support queue_reset after disabling the queue?
> If yes, this basically go back to using a single queue_enable
> register.

I do not think queue enable resets internal registers such as wrap
count.

> >
> >    By the way current spec does not make it clear what happens if
> >    driver keeps adding buffers to the queue after writing to
> >    reset. It's probably a good idea to specify that device
> >    should not wait to drain the queue, it should just reset,
> >    but again this is an existing defect so maybe a separate patch.
> >
> > 2- what happens if queue is disabled but
> >    without reset, and queue size changes? We do not specify.
> >    Maybe that has to be prohibited.
> 
> Yes, after we enable a queue.
> 
> >
> > I conclude that at some point, there is a chance we will want
> > to add VIRTIO_RING_F_DISABLE making queue enable writeable for
> > all transports after DRIVER_OK.
> 
> It looks to me it's better to have a single feature for disable/rest.
> And it's self-contained if:
> 
> 1) it holds a virtualenv state (indices)
> 2) allows the driver to modify the indices after it has been stopped
> (with another feature).
> 
> This seems to cover all the use cases.
> 
> Thanks

I am not 100% sure what you advocate for here. Dropping RESET
feature from spec completely for now, and replacing in 1.3 with
a feature that allows driver to modify internal device state?
Or something else?



> > If we do, we might want the reset register behaviour to change
> > if VIRTIO_RING_F_DISABLE has been negotiated.
> > Parav from hardware point of view, would you say such a register where
> > behaviour changes depending on feature bit be too messy?
> >
> >
> > Overall, this makes me feel existing two register interface is
> > fine, reusing queue enable would make future expansion harder
> > while making queue enable writeable would be a new feature
> > and needs its own feature bit.
> >
> >
> > --
> > MST
> >

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Michael S. Tsirkin]

On Thu, Apr 28, 2022 at 02:13:42PM +0800, Jason Wang wrote:
> > queue_reset removal has far more editorial changes.
> > Before I update the v4 to incorporate, please discuss/sync with Michael or others what is best course of action from spec timing perspective.
> >  I am ok either way to draft either by
> > (a) removal of queue_reset register and allow queue_disable by replacing RING_RESET definition in v4
> > Or
> > (b) fixing the polarity as done in v3
> 
> Ok.
> 
> Thanks

So Jason, what is your take? I am inclined to keep queue reset register
and just flip the polarity. Less spec work and semantics are more or
less clear. Not helpful for migration - limited to queue
resize, watchdog and similar uses - but if we are expanding
scope to migration that makes it imo not 1.2 material.

-- 
MST

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Jason Wang]

On Thu, Apr 28, 2022 at 2:30 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Apr 28, 2022 at 02:13:42PM +0800, Jason Wang wrote:
> > > queue_reset removal has far more editorial changes.
> > > Before I update the v4 to incorporate, please discuss/sync with Michael or others what is best course of action from spec timing perspective.
> > >  I am ok either way to draft either by
> > > (a) removal of queue_reset register and allow queue_disable by replacing RING_RESET definition in v4
> > > Or
> > > (b) fixing the polarity as done in v3
> >
> > Ok.
> >
> > Thanks
>
> So Jason, what is your take?

I don't object Parav's idea. Just want to seek if there's something better.

> I am inclined to keep queue reset register
> and just flip the polarity. Less spec work and semantics are more or
> less clear. Not helpful for migration - limited to queue
> resize, watchdog and similar uses - but if we are expanding
> scope to migration that makes it imo not 1.2 material.

That's fine consider queue_reset has more users than just migration.

Thanks

>
> --
> MST
>

[virtio-comment] RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Cornelia Huck]

On Thu, Apr 28 2022, Parav Pandit <parav@nvidia.com> wrote:

> All above comments are taken care now with Cornelia's wording for your input and by correctly updating device side section now.
> Sending v3.

Honestly, I think it's a bit early for sending a v3 already, since it
seems we've not reached complete agreement yet what this should look
like in the end, and what we can/should do for 1.2 and what should be
deferred for 1.3. At least, it's about as clear as mud to me. Maybe
someone can summarize?


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Jason Wang]

On Thu, Apr 28, 2022 at 2:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Apr 28, 2022 at 02:10:46PM +0800, Jason Wang wrote:
> > On Thu, Apr 28, 2022 at 12:56 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Apr 28, 2022 at 11:43:16AM +0800, Jason Wang wrote:
> > > >
> > > > 在 2022/4/28 11:24, Parav Pandit 写道:
> > > > >
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Wednesday, April 27, 2022 11:15 PM
> > > > > >
> > > > > > On Wed, Apr 27, 2022 at 11:39 PM Parav Pandit <parav@nvidia.com> wrote:
> > > > > > >
> > > > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > Sent: Wednesday, April 27, 2022 11:30 AM
> > > > > > > >
> > > > > > > > On Wed, Apr 27, 2022 at 02:51:36PM +0000, Parav Pandit wrote:
> > > > > > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > > > > > Sent: Wednesday, April 27, 2022 7:30 AM
> > > > > > > > > >
> > > > > > > > > > On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <parav@nvidia.com>
> > > > > > wrote:
> > > > > > > > > > > example flow:
> > > > > > > > > > > a) 0,0 -> device init time value
> > > > > > > > > > > b) 1,0 -> vq is enabled by driver and working
> > > > > > > > > > Did you see my reply in V1? What's the reason for using write to
> > > > > > > > > > clear behavior that is different from the device status?
> > > > > > > > > >
> > > > > > > > > > We can simply make this as 1, 1 here and let the driver write to
> > > > > > > > > > 0 to reset the virtqueue.
> > > > > > > > > >
> > > > > > > > > > And if we do this, the queue_enable and queue_reset are always
> > > > > > > > > > the same, then we can simply reuse queue_enable.
> > > > > > > > > >
> > > > > > > > > Yes, I know we can make this work using new feature bit + single
> > > > > > > > queue_enable register.
> > > > > > > > > I replied that in v0 to Michael.
> > > > > > > > A bigger question in my eyes is that down the road we might want to
> > > > > > > > be able to stop the ring without having it lose state.
> > > > > > > > The natural interface for that seems to be writing 0 to queue enable.
> > > > > > > Why queue_enable and not queue_reset?
> > > > > > >
> > > > > > > to me this interface is unlikely performant and useful for such case.
> > > > > > > When we want to pause/stop the VQ and query the state we need
> > > > > > performant scheme, that can even work in a batch for all the VQs.
> > > > > > > At that point programming 64 registers to pause/stop VQ without losing
> > > > > > state and querying its indices etc won't be scalable with register interface.
> > > > > >
> > > > > > The register interface to sync indices has already been implemented in real
> > > > > > hardware for years.
> > > > > >
> > > > > Sure. I meant to that when we want to pause a VQ and restart later use-case will require more plumbing than just enable/disable register.
> > > > > And to do that, a register interface won't be performant/scalable. So for the wider use case this may not be the good choice.
> > > > >
> > > > > And I explained the other reason that we lose the state information with this busy-wait register in the other reply to V2 and summary in the github issue too on Michael's request.
> > > >
> > > >
> > > > I've replied to the thread.
> > > >
> > > >
> > > > >
> > > > > > > I imagine a AQ (likely) or some other interface.
> > > > > > So did the queue_enable registers, we need to write 1 to queue_enable for
> > > > > > each virtqueue before DRIVER_OK.
> > > > > >
> > > > > > Where to allow writing 0 to queue_enable is orthogonal to scalability.
> > > > > Sure, lets sync after you get chance to go through my other reply to Michael about why with single busy-wait register we lose the state.
> > > > > And hence, queue_reset register (with the fix) is better.
> > > >
> > > >
> > > > Note that the MMIO transport allows writing 0 to queue_ready.
> > > >
> > > > Thanks
> > >
> > > ... without the side effect of resetting the queue state.
> >
> > It's unclear in the spec that whether or not we had such side effects.
>
> ...and that makes it pretty useless without a new feature bit
>
> > > I think ideally there should be text in MMIO stating it does not
> > > support the reset feature at the moment,
> >
> > So do you mean the state is held after writing 0 to queue_ready?
>
> I said nothing about QueueReady. I just said that we did not
> add the reset register to MMIO yet, MMIO drivers should be
> careful not to negotiate the reset feature bit until we
> do and they know the reset register offset.
>
> > What
> > happen if driver do
> >
> > write 0 to queue_enable,
> > avail_idx = 0,
> > write 1 to queue_enable,
> >
> > ?
> >
> > > so devices should not expose
> > > this feature and drivers should not acknowledge this if exposed by device
> > > but should not fail if it's exposed (for future expansion).
> >
> > I'm not sure I understand this, but it looks too late to do that? (We
> > don't have a feature for this).
> >
> > Or we can simply forbid writing 0 to queue_ready until we refine this
> > behaviour with a feature.
> >
> > > This is an existing spec issue, should be a separate patch.
> > >
> > > Regarding MMIO it is not exactly clear but I think the assumption
> > > always was you don't disable queues after DRIVER_OK.
> > >
> > > Just thinking aloud, I am wondering:
> > >
> > >
> > > So in the proposal, the new queue reset register does two things:
> > > - reset the internal queue state
> >
> > Does the reset also mean stop?
>
> After a queue has been reset by the driver, the device MUST NOT execute
> any requests from that virtqueue, or notify the driver for it.
>
> so apparently yes
>
>
> > > - disable the queue
> >
> > What's the benefit of separating the functions here?
>
> I am not separating anything *here*, just listing what happens.
> Below I list some ideas for why disable without reset might be useful.
>
> > >
> > >
> > > It seems possible to split the functionality: queue reset would just
> > > disable counters, queue enable would disable queue.
> >
> > This requires hardware to synchronize the queue_reset with datapath
> > internally which looks a little bit complicated.
>
> this is already the case.
>
> > > If we do
> > > that:
> > > 1- We need to specify what happens if we reset counters while ring is enabled.
> > >    I guess queue then has to go until ring is empty, then stop?
> > >    Draining queue like this has value for e.g. snapshots.
> > >    An alternative is to prohibit this transition.
> >
> > Does this mean we only support queue_reset after disabling the queue?
> > If yes, this basically go back to using a single queue_enable
> > register.
>
> I do not think queue enable resets internal registers such as wrap
> count.

Right, but it looks like something we need to clarify in the spec. Or
we can introduce new feature to formalize the behaviour.

>
> > >
> > >    By the way current spec does not make it clear what happens if
> > >    driver keeps adding buffers to the queue after writing to
> > >    reset. It's probably a good idea to specify that device
> > >    should not wait to drain the queue, it should just reset,
> > >    but again this is an existing defect so maybe a separate patch.
> > >
> > > 2- what happens if queue is disabled but
> > >    without reset, and queue size changes? We do not specify.
> > >    Maybe that has to be prohibited.
> >
> > Yes, after we enable a queue.
> >
> > >
> > > I conclude that at some point, there is a chance we will want
> > > to add VIRTIO_RING_F_DISABLE making queue enable writeable for
> > > all transports after DRIVER_OK.
> >
> > It looks to me it's better to have a single feature for disable/rest.
> > And it's self-contained if:
> >
> > 1) it holds a virtualenv state (indices)
> > 2) allows the driver to modify the indices after it has been stopped
> > (with another feature).
> >
> > This seems to cover all the use cases.
> >
> > Thanks
>
> I am not 100% sure what you advocate for here. Dropping RESET
> feature from spec completely for now, and replacing in 1.3 with
> a feature that allows driver to modify internal device state?

Just want to propose something that is more functional complete. It
depends on whether we need a per virtqueue stop-and-modify. (I can't
think of one now) So If a per device stop and resume is sufficient. We
don't need to bother.

Thanks

> Or something else?
>
>
>
> > > If we do, we might want the reset register behaviour to change
> > > if VIRTIO_RING_F_DISABLE has been negotiated.
> > > Parav from hardware point of view, would you say such a register where
> > > behaviour changes depending on feature bit be too messy?
> > >
> > >
> > > Overall, this makes me feel existing two register interface is
> > > fine, reusing queue enable would make future expansion harder
> > > while making queue enable writeable would be a new feature
> > > and needs its own feature bit.
> > >
> > >
> > > --
> > > MST
> > >
>

RE: [virtio-dev] [PATCH v2] virtio: Improve queue_reset polarity to match to default reset state

[Parav Pandit]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Thursday, April 28, 2022 3:33 AM
> 
> On Thu, Apr 28 2022, Parav Pandit <parav@nvidia.com> wrote:
> 
> > All above comments are taken care now with Cornelia's wording for your
> input and by correctly updating device side section now.
> > Sending v3.
> 
> Honestly, I think it's a bit early for sending a v3 already, since it seems we've
> not reached complete agreement yet what this should look like in the end,
> and what we can/should do for 1.2 and what should be deferred for 1.3. At
> least, it's about as clear as mud to me. Maybe someone can summarize?

Summary:
1. Michel suggested to continue to keep queue_reset register with the proposed fixes of v3
2. Jason is ok with the use cases that we see on hand are addressed with this register fix
3. Xuan's comments are addressed already in v2.
4. Comments posted from you, Michael and Jason are taken care in v3.
Waiting for reviews of v3 from Xuan, Jason, Michael for 1.2 merge.
5. github issue is up to date

I derive that we should get the fixes in 1.2.
Any complex advancement for other use cases is likely in 1.3.

Michael,
Please correct me if above is not accurate.
Did I miss anything for MMIO in v3?