virtio: Improve queue_reset polarity to match to default reset state

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 <[email protected]>

---
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(-)

content.tex +16 -11

index 060bdab..68b176b 100644
@@ -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: [email protected] For additional commands, e-mail: [email protected]

Quoted 11 lines · 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.

Quoted 7 lines

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

Quoted 115 lines

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 <[email protected]>

---
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(-)

content.tex +16 -11

index 060bdab..68b176b 100644
@@ -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: [email protected] For additional commands, e-mail: [email protected]

Quoted 20 lines · Jason Wang

Quoted 11 lines · 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.

Quoted 7 lines

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

Quoted 8 lines

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.

Quoted 121 lines

Thanks

Quoted 115 lines

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 <[email protected]>

---
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(-)

content.tex +16 -11

index 060bdab..68b176b 100644
@@ -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: [email protected] For additional commands, e-mail: [email protected]

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

Quoted 4 lines · Parav Pandit

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.

Quoted 13 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

Quoted 4 lines · Parav Pandit

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?

Quoted 6 lines

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.

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 19 lines · Parav Pandit

Quoted 19 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

Quoted 4 lines · Parav Pandit

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.

Quoted 1 line

Yes, I know we can make this work using new feature bit + single

queue_enable register.

Quoted 1 line

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.

Quoted 2 lines

I was not sure how drastic that would be at this point in the spec release cycle

that Michael highlighted.

Quoted 1 line

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.

Quoted 24 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 19 lines · Parav Pandit

Quoted 19 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

Quoted 4 lines · Parav Pandit

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.

Quoted 7 lines

Quoted 1 line

Yes, I know we can make this work using new feature bit + single

queue_enable register.

Quoted 1 line

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.

Quoted 1 line

Quoted 7 lines

Quoted 1 line

I was not sure how drastic that would be at this point in the spec release cycle

that Michael highlighted.

Quoted 1 line

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.

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:44 AM

Quoted 10 lines · Parav Pandit

Quoted 10 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 5 lines · Parav Pandit

Quoted 5 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <[email protected]>

Quoted 18 lines · wrote:

Quoted 18 lines

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 1 line

I replied that in v0 to Michael.

Quoted 4 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 7 lines

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

Quoted 5 lines

Quoted 3 lines

I was not sure how drastic that would be at this point in the spec release cycle

that Michael highlighted.

Quoted 1 line

Hence, I proposed a minimal change fix to queue_reset register given

timeline.

Quoted 5 lines

Quoted 4 lines

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.

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

Quoted 11 lines

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.

Quoted 6 lines

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.

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 3:32 PM

Quoted 1 line

I think further, it appears a real bug that requires so special handling in the

device for next years to carry.

Quoted 11 lines

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?

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 1 line

But hey, the destination side needs to treat this special 0xa and convert to

internal q_yet_busy stage.

Quoted 2 lines

And this answer Jason and myself why queue_enable shouldn't be

overloaded for this busy_wait register.

Quoted 2 lines

Hence queue_reset register seems the right choice with the fix.

Quoted 1 line

Starting to think of workaround even before the spec release is not

elegant.

Quoted 1 line

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.

Quoted 10 lines

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.

Quoted 6 lines

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.

From: Jason Wang <[email protected]>
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.

Quoted 5 lines

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?

Quoted 1 line

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?

Quoted 4 lines

When this virtio device is restored on the other side, HV and device doesn't

know how to deal with this.

Quoted 2 lines

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.

Quoted 1 line

But hey, the destination side needs to treat this special 0xa and convert to

internal q_yet_busy stage.

Quoted 2 lines

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.

Quoted 10 lines

From: Jason Wang <[email protected]>
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.

Quoted 5 lines

Quoted 5 lines

VQ state returned
q_enable=1, q_reset = 0.

Quoted 7 lines

Quoted 1 line

HV doesn't know what q_reset=0 mean here, is it 0 because it was never

reset?

Quoted 1 line

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.

Quoted 19 lines

Quoted 4 lines

When this virtio device is restored on the other side, HV and device doesn't

know how to deal with this.

Quoted 2 lines

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.

Quoted 1 line

But hey, the destination side needs to treat this special 0xa and convert to

internal q_yet_busy stage.

Quoted 2 lines

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.

"""

Quoted 3 lines

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

Quoted 6 lines

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

Quoted 7 lines · Jason Wang

Quoted 6 lines

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

Quoted 3 lines

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

Quoted 34 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:44 AM

Quoted 10 lines · Parav Pandit

Quoted 10 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 5 lines · Parav Pandit

Quoted 5 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <[email protected]>

Quoted 18 lines · wrote:

Quoted 18 lines

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 1 line

I replied that in v0 to Michael.

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 12 lines

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 7 lines

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

Quoted 5 lines

Quoted 3 lines

I was not sure how drastic that would be at this point in the spec release cycle

that Michael highlighted.

Quoted 1 line

Hence, I proposed a minimal change fix to queue_reset register given

Quoted 8 lines

timeline.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 1 line

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

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 3:28 PM

Quoted 40 lines · Parav Pandit

Quoted 40 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:44 AM

Quoted 11 lines · Parav Pandit

Quoted 11 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 6 lines · Parav Pandit

Quoted 6 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <[email protected]>

Quoted 23 lines · wrote:

Quoted 23 lines

Quoted 12 lines

Quoted 12 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

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.

Quoted 8 lines

Make sense.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

Quoted 6 lines

Quoted 3 lines

I was not sure how drastic that would be at this point in the spec release cycle

that Michael highlighted.

Quoted 2 lines

Hence, I proposed a minimal change fix to queue_reset register given

Quoted 8 lines

Quoted 8 lines

timeline.

Quoted 5 lines

Quoted 4 lines

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

Quoted 6 lines

Yes.

Quoted 1 line

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

Quoted 1 line

And maintain those weird code in device side and extra state bits burning

some expensive chip resource.

Quoted 3 lines

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.

Quoted 1 line

May be time for virtio spec to take this opportunity now and be bit agile on

it.

Quoted 1 line

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.

Quoted 29 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 13 lines · Parav Pandit

Quoted 13 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

Quoted 4 lines · Parav Pandit

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 1 line

I replied that in v0 to Michael.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 3 lines

I was not sure how drastic that would be at this point in the spec release cycle

that Michael highlighted.

Quoted 1 line

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.

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 11:15 PM

Quoted 11 lines · Parav Pandit

Quoted 11 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 5 lines · Parav Pandit

Quoted 5 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <[email protected]>

Quoted 23 lines · wrote:

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 1 line

Yes, I know we can make this work using new feature bit + single

queue_enable register.

Quoted 1 line

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?

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 41 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 11:15 PM

Quoted 9 lines · Parav Pandit

Quoted 9 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 4 lines · Parav Pandit

Quoted 4 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <[email protected]>

Quoted 14 lines · wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 41 lines · Parav Pandit

Quoted 41 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 11:15 PM

Quoted 9 lines · Parav Pandit

Quoted 9 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 4 lines · Parav Pandit

Quoted 4 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <[email protected]>

Quoted 14 lines · wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 1 line

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

Quoted 65 lines · Jason Wang

Quoted 41 lines · Parav Pandit

Quoted 41 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 11:15 PM

Quoted 9 lines · Parav Pandit

Quoted 9 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 4 lines · Parav Pandit

Quoted 4 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <[email protected]>

Quoted 14 lines · wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines

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

Quoted 67 lines · Michael S. Tsirkin

Quoted 65 lines · Jason Wang

Quoted 41 lines · Parav Pandit

Quoted 41 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 11:15 PM

Quoted 9 lines · Parav Pandit

Quoted 9 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 4 lines · Parav Pandit

Quoted 4 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <[email protected]>

Quoted 14 lines · wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 2 lines

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,

?

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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?

Quoted 1 line

- disable the queue

What's the benefit of separating the functions here?

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 10 lines

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.

Quoted 4 lines

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

Quoted 15 lines

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

Quoted 68 lines · Jason Wang

Quoted 67 lines · Michael S. Tsirkin

Quoted 65 lines · Jason Wang

Quoted 41 lines · Parav Pandit

Quoted 41 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 11:15 PM

Quoted 9 lines · Parav Pandit

Quoted 9 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Wednesday, April 27, 2022 11:30 AM

Quoted 4 lines · Parav Pandit

Quoted 4 lines

From: Jason Wang <[email protected]>
Sent: Wednesday, April 27, 2022 7:30 AM

On Wed, Apr 27, 2022 at 6:26 PM Parav Pandit <[email protected]>

Quoted 14 lines · wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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

Quoted 4 lines

Quoted 2 lines

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.

Quoted 24 lines

What
happen if driver do

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

?

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

Quoted 5 lines

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

Quoted 3 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 26 lines

Quoted 11 lines

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.

Quoted 4 lines

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

Quoted 3 lines

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?

Quoted 15 lines

Quoted 15 lines

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

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 <[email protected]>

---
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(-)

content.tex +1 -1

index 060bdab..68b176b 100644
@@ -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}).

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

content.tex +1 -5

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

From: Cornelia Huck <[email protected]>
Sent: Wednesday, April 27, 2022 8:48 AM

Quoted 2 lines

+\field{queue_reset} to reset the queue, driver MUST verify that the +queue

s/driver/the driver/

Quoted 1 line

+has been reset by reading back \field{queue_reset} until device returns a

value of 0.

s/device/the device/

Review

Quoted 3 lines

(quoted diff) +2 -0

+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?

Review

Quoted 4 lines

When the device completes the queue reset, \field{queue_reset}

(quoted diff) +2 -0

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

Similar comments as for the pci section.

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 <[email protected]>

---
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(-)

content.tex +0 -0

index 060bdab..68b176b 100644

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

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

content.tex +8 -5

 \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}).

Signature · 6 lines

1.8.3.1


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

From: [email protected] <[email protected]> On Behalf Of Michael S. Tsirkin

Quoted 1 line · Parav Pandit

Signed-off-by Parav Pandit <[email protected]>

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

Quoted 4 lines

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

Quoted 2 lines

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?

Quoted 2 lines

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

Quoted 1 line

+Device continue

continues

Quoted 1 line

to report

we say present elsewhere, let's be consistent

Quoted 2 lines

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.

Quoted 2 lines

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

are set to 0

Review

Quoted 3 lines

, indicating

(quoted diff) +2 -0

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

Quoted 3 lines

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

Quoted 1 line

(see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue

Reset}).

Quoted 2 lines

(quoted diff) +0 -0

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

From: Cornelia Huck <[email protected]>
Sent: Thursday, April 28, 2022 3:33 AM

On Thu, Apr 28 2022, Parav Pandit <[email protected]> wrote:

Quoted 1 line

All above comments are taken care now with Cornelia's wording for your

input and by correctly updating device side section now.

Quoted 1 line

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?