[PATCH v4 0/3] virtio: introduce VIRTIO_F_RING_RESET for reset queue

[PATCH v4 0/3] virtio: introduce VIRTIO_F_RING_RESET for reset queue

[Xuan Zhuo]

Hi All:

This is a new version to support VIRTIO_F_RING_RESET. The feautre
extends the basic facility to allow the driver to reset a virtqueue.
This main motivation is to support the reset function of the queue of the
network device.

Please review.

v4:
   Cornelia Huck helped me more. Thanks.
   MMIO support this.

Thanks

Xuan Zhuo (3):
  virtio: introduce virtqueue reset as basic facility
  virtio: pci support virtqueue reset
  virtio: mmio support virtqueue reset

 content.tex | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

--
2.31.0

[PATCH v4 1/3] virtio: introduce virtqueue reset as basic facility

[Xuan Zhuo]

This patch allows the driver to reset a queue individually.

This is very common on general network equipment. By disabling a queue,
you can quickly reclaim the buffer currently on the queue. If necessary,
we can reinitialize the queue separately.

For example, when virtio-net implements support for AF_XDP, we need to
disable a queue to release all the original buffers when AF_XDP setup.
And quickly release all the AF_XDP buffers that have been placed in the
queue when AF_XDP exits.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 content.tex | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/content.tex b/content.tex
index 37c45d3..05b05ba 100644
--- a/content.tex
+++ b/content.tex
@@ -407,6 +407,47 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
 types. It is RECOMMENDED that devices generate version 4
 UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
 
+\section{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset}
+
+When VIRTIO_F_RING_RESET is negotiated, the driver can reset a virtqueue
+individually. The way to reset the virtqueue is transport specific.
+
+Virtqueue reset is divided into two parts. The driver reset a queue
+first, and then re-enable the queue. The re-enable part is optional.
+
+\subsection{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Reset}
+
+\devicenormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
+
+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.
+
+The device MUST re-initialize any state of a virtqueue that has been
+reset, including available and used state.
+
+\drivernormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
+
+After the driver tell the device to reset a queue, the driver MUST verify that
+the queue has actually been reset.
+
+After the queue has been successfully reset, the driver MAY release any
+resource associated with that virtqueue.
+
+\subsection{Virtqueue Re-enable}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
+
+This process is the same as the initialization process of a single queue during
+the initialization of the entire device.
+
+\devicenormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
+
+The device MUST receive the new configuration from the driver. (i.e. the maximum
+Queue Size.)
+
+\drivernormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
+
+The driver MUST apply for resources, set new configuration to the device, and
+finally activate the device.
+
 \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
 
 We start with an overview of device initialization, then expand on the
@@ -6673,6 +6714,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   transport specific.
   For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
 
+  \item[VIRTIO_F_RING_RESET(40)] This feature indicates
+  that the driver can reset a queue individually.
+  See \ref{sec:Virtqueues / Virtqueue Reset}.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
-- 
2.31.0

[PATCH v4 2/3] virtio: pci support virtqueue reset

[Xuan Zhuo]

PCI support virtqueue reset.

virtio_pci_common_cfg add "queue_reset" to support virtqueue reset.
The driver uses this to selectively reset the queue.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 content.tex | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/content.tex b/content.tex
index 05b05ba..1c35df5 100644
--- a/content.tex
+++ b/content.tex
@@ -900,6 +900,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         le64 queue_driver;              /* read-write */
         le64 queue_device;              /* read-write */
         le16 queue_notify_data;         /* read-only for driver */
+        le16 queue_reset;               /* read-write */
 };
 \end{lstlisting}
 
@@ -979,6 +980,12 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         may benefit from providing another value, for example an internal virtqueue
         identifier, or an internal offset related to the virtqueue number.
         \end{note}
+
+\item[\field{queue_reset}]
+        The driver uses this to selectively reset the queue.
+        This field exists only if VIRTIO_F_RING_RESET has been
+        negotiated. (see \ref{sec:Virtqueues / Virtqueue Reset}).
+
 \end{description}
 
 \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
@@ -1020,6 +1027,20 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 
 The device MUST present a 0 in \field{queue_enable} on reset.
 
+If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
+\field{queue_enable} after the driver has reset the virtqueue via
+\field{queue_reset}.
+
+If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
+\field{queue_reset} on reset.
+
+If VIRTIO_F_RING_RESET has been negotiated, The device MUST present a 0 in
+\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
+driver re-enables the queue via \field{queue_enable}. (see \ref{sec:Virtqueues / Virtqueue Reset}).
+
 The device MUST present a 0 in \field{queue_size} if the virtqueue
 corresponding to the current \field{queue_select} is unavailable.
 
@@ -1044,6 +1065,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
+been set up correctly. (see \ref{sec:Virtqueues / Virtqueue Reset}).
+
 \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
 
 The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG
-- 
2.31.0

[PATCH v4 3/3] virtio: mmio support virtqueue reset

[Xuan Zhuo]

mmio support virtqueue reset.

MMIO Device Register Layout "QueueReady" to support virtqueue reset.
The driver uses this to selectively reset the queue.

Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 content.tex | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/content.tex b/content.tex
index 1c35df5..3ff86b3 100644
--- a/content.tex
+++ b/content.tex
@@ -1848,7 +1848,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     Writing to this register selects the virtual queue that the
     following operations on \field{QueueNumMax}, \field{QueueNum}, \field{QueueReady},
     \field{QueueDescLow}, \field{QueueDescHigh}, \field{QueueDriverlLow}, \field{QueueDriverHigh},
-    \field{QueueDeviceLow} and \field{QueueDeviceHigh} apply to. The index
+    \field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset} apply to. The index
     number of the first queue is zero (0x0). 
   }
   \hline 
@@ -1965,6 +1965,12 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     0xffffffffffffffff.
   }
   \hline 
+  \mmioreg{QueueReset}{Virtual queue reset bit}{0x0c0}{RW}{%
+    Writing one (0x1) to selectively reset the queue.  Reading from this
+    register returns the last value written to it. Both read and write accesses
+    apply to the queue selected by writing to \field{QueueSel}.
+  }
+  \hline
   \mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{
     Reading from this register returns a value describing a version of the device-specific configuration space (see \field{Config}).
     The driver can then access the configuration space and, when finished, read \field{ConfigGeneration} again.
@@ -2000,6 +2006,20 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 
 The device MUST NOT access virtual queue contents when \field{QueueReady} is zero (0x0).
 
+If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
+\field{QueueReady} after the driver has reset the virtqueue via
+\field{QueueReset}.
+
+If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
+\field{QueueReset} on reset.
+
+If VIRTIO_F_RING_RESET has been negotiated, The device MUST present a 0 in
+\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
+driver re-enables the queue via \field{QueueReady}. (see \ref{sec:Virtqueues / Virtqueue Reset}).
+
 \drivernormative{\subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}
 The driver MUST NOT access memory locations not described in the
 table \ref{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}
@@ -2042,6 +2062,13 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
 The driver MUST write a value with a bit mask describing events it handled into \field{InterruptACK} when
 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
+been set up correctly. (see \ref{sec:Virtqueues / Virtqueue Reset}).
+
 \subsection{MMIO-specific Initialization And Device Operation}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation}
 
 \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization}
-- 
2.31.0

[virtio-dev] Re: [PATCH v4 1/3] virtio: introduce virtqueue reset as basic facility

[Cornelia Huck]

On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:

> This patch allows the driver to reset a queue individually.
>
> This is very common on general network equipment. By disabling a queue,
> you can quickly reclaim the buffer currently on the queue. If necessary,
> we can reinitialize the queue separately.
>
> For example, when virtio-net implements support for AF_XDP, we need to
> disable a queue to release all the original buffers when AF_XDP setup.
> And quickly release all the AF_XDP buffers that have been placed in the
> queue when AF_XDP exits.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  content.tex | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 37c45d3..05b05ba 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -407,6 +407,47 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
>  types. It is RECOMMENDED that devices generate version 4
>  UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
>  
> +\section{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset}
> +
> +When VIRTIO_F_RING_RESET is negotiated, the driver can reset a virtqueue
> +individually. The way to reset the virtqueue is transport specific.
> +
> +Virtqueue reset is divided into two parts. The driver reset a queue
> +first, and then re-enable the queue. The re-enable part is optional.

Maybe

"The driver first resets a queue and can afterwards optionally re-enable
it."

?

> +
> +\subsection{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Reset}
> +
> +\devicenormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
> +
> +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.
> +
> +The device MUST re-initialize any state of a virtqueue that has been
> +reset, including available and used state.
> +
> +\drivernormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
> +
> +After the driver tell the device to reset a queue, the driver MUST verify that

s/tell/tells/ (may have been my typo...)

> +the queue has actually been reset.
> +
> +After the queue has been successfully reset, the driver MAY release any
> +resource associated with that virtqueue.
> +
> +\subsection{Virtqueue Re-enable}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> +
> +This process is the same as the initialization process of a single queue during
> +the initialization of the entire device.
> +
> +\devicenormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> +
> +The device MUST receive the new configuration from the driver. (i.e. the maximum
> +Queue Size.)

Maybe

"The device MUST observe any queue configuration that may have been
changed by the driver, like the maximum queue size."

Although I'm not sure about 'observe'. Anybody have a better term?

> +
> +\drivernormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> +
> +The driver MUST apply for resources, set new configuration to the device, and
> +finally activate the device.

Maybe

"When re-enabling a queue, the driver MUST configure the queue resources
as during initial virtqueue discovery, but optionally with different
parameters."

?

What actually needs to be done is dependent on the transport anyway.

What do you mean by "activate the device"? I thought the device state
remained unchanged, it is just the individual queue that is reset and
re-enabled?

> +
>  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
>  
>  We start with an overview of device initialization, then expand on the
> @@ -6673,6 +6714,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>    transport specific.
>    For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
>  
> +  \item[VIRTIO_F_RING_RESET(40)] This feature indicates
> +  that the driver can reset a queue individually.
> +  See \ref{sec:Virtqueues / Virtqueue Reset}.
> +
>  \end{description}
>  
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}


---------------------------------------------------------------------
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 v4 2/3] virtio: pci support virtqueue reset

[Cornelia Huck]

On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:

> PCI support virtqueue reset.
>
> virtio_pci_common_cfg add "queue_reset" to support virtqueue reset.
> The driver uses this to selectively reset the queue.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  content.tex | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

(...)

> @@ -1020,6 +1027,20 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  
>  The device MUST present a 0 in \field{queue_enable} on reset.
>  
> +If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
> +\field{queue_enable} after the driver has reset the virtqueue via
> +\field{queue_reset}.
> +
> +If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
> +\field{queue_reset} on reset.
> +
> +If VIRTIO_F_RING_RESET has been negotiated, The device MUST present a 0 in

s/The/the/

> +\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
> +driver re-enables the queue via \field{queue_enable}. (see \ref{sec:Virtqueues / Virtqueue Reset}).

"...or the device is reset." ?

Maybe also

"The device MAY change the value of \field{queue_size} if the queue has
been reset." ?

Should it always set that field to the currently maximum supported queue
size (assuming that can change dynamically)? Do we need some kind of
synchronization for those changes?

> +
>  The device MUST present a 0 in \field{queue_size} if the virtqueue
>  corresponding to the current \field{queue_select} is unavailable.
>  
> @@ -1044,6 +1065,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
> +been set up correctly. (see \ref{sec:Virtqueues / Virtqueue Reset}).

Maybe also something like

"The driver MAY set driver-writeable queue configuration values to
different values than those that were used before the queue reset."

?

> +
>  \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
>  
>  The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG


---------------------------------------------------------------------
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 v4 3/3] virtio: mmio support virtqueue reset

[Cornelia Huck]

On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:

> mmio support virtqueue reset.
>
> MMIO Device Register Layout "QueueReady" to support virtqueue reset.
> The driver uses this to selectively reset the queue.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  content.tex | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/content.tex b/content.tex
> index 1c35df5..3ff86b3 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1848,7 +1848,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>      Writing to this register selects the virtual queue that the
>      following operations on \field{QueueNumMax}, \field{QueueNum}, \field{QueueReady},
>      \field{QueueDescLow}, \field{QueueDescHigh}, \field{QueueDriverlLow}, \field{QueueDriverHigh},
> -    \field{QueueDeviceLow} and \field{QueueDeviceHigh} apply to. The index
> +    \field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset} apply to. The index
>      number of the first queue is zero (0x0). 
>    }
>    \hline 
> @@ -1965,6 +1965,12 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>      0xffffffffffffffff.
>    }
>    \hline 
> +  \mmioreg{QueueReset}{Virtual queue reset bit}{0x0c0}{RW}{%
> +    Writing one (0x1) to selectively reset the queue.  Reading from this

Hm...

"If VIRTIO_F_RING_RESET has been negotiated, writing one (0x1) to this
register selectively resets the queue."

?

> +    register returns the last value written to it. Both read and write accesses

Doesn't the register's value return to 0 if the queue is re-eanbled?

> +    apply to the queue selected by writing to \field{QueueSel}.
> +  }
> +  \hline
>    \mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{
>      Reading from this register returns a value describing a version of the device-specific configuration space (see \field{Config}).
>      The driver can then access the configuration space and, when finished, read \field{ConfigGeneration} again.
> @@ -2000,6 +2006,20 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>  
>  The device MUST NOT access virtual queue contents when \field{QueueReady} is zero (0x0).
>  
> +If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
> +\field{QueueReady} after the driver has reset the virtqueue via
> +\field{QueueReset}.
> +
> +If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
> +\field{QueueReset} on reset.
> +
> +If VIRTIO_F_RING_RESET has been negotiated, The device MUST present a 0 in
> +\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
> +driver re-enables the queue via \field{QueueReady}. (see \ref{sec:Virtqueues / Virtqueue Reset}).

"...or until the device is reset."

> +
>  \drivernormative{\subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}
>  The driver MUST NOT access memory locations not described in the
>  table \ref{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}
> @@ -2042,6 +2062,13 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>  The driver MUST write a value with a bit mask describing events it handled into \field{InterruptACK} when
>  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
> +been set up correctly. (see \ref{sec:Virtqueues / Virtqueue Reset}).

Similar commments as for the PCI part regarding the various
queue-specific configuration values.

> +
>  \subsection{MMIO-specific Initialization And Device Operation}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation}
>  
>  \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization}

This approach for MMIO looks sane to me (although I'm not an expert.)


---------------------------------------------------------------------
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 v4 1/3] virtio: introduce virtqueue reset as basic facility

[Xuan Zhuo]

On Tue, 28 Sep 2021 12:06:01 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
> On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> > This patch allows the driver to reset a queue individually.
> >
> > This is very common on general network equipment. By disabling a queue,
> > you can quickly reclaim the buffer currently on the queue. If necessary,
> > we can reinitialize the queue separately.
> >
> > For example, when virtio-net implements support for AF_XDP, we need to
> > disable a queue to release all the original buffers when AF_XDP setup.
> > And quickly release all the AF_XDP buffers that have been placed in the
> > queue when AF_XDP exits.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  content.tex | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 37c45d3..05b05ba 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -407,6 +407,47 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
> >  types. It is RECOMMENDED that devices generate version 4
> >  UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
> >
> > +\section{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset}
> > +
> > +When VIRTIO_F_RING_RESET is negotiated, the driver can reset a virtqueue
> > +individually. The way to reset the virtqueue is transport specific.
> > +
> > +Virtqueue reset is divided into two parts. The driver reset a queue
> > +first, and then re-enable the queue. The re-enable part is optional.
>
> Maybe
>
> "The driver first resets a queue and can afterwards optionally re-enable
> it."
>
> ?
>
> > +
> > +\subsection{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > +
> > +\devicenormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > +
> > +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.
> > +
> > +The device MUST re-initialize any state of a virtqueue that has been
> > +reset, including available and used state.
> > +
> > +\drivernormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > +
> > +After the driver tell the device to reset a queue, the driver MUST verify that
>
> s/tell/tells/ (may have been my typo...)
>
> > +the queue has actually been reset.
> > +
> > +After the queue has been successfully reset, the driver MAY release any
> > +resource associated with that virtqueue.
> > +
> > +\subsection{Virtqueue Re-enable}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> > +
> > +This process is the same as the initialization process of a single queue during
> > +the initialization of the entire device.
> > +
> > +\devicenormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> > +
> > +The device MUST receive the new configuration from the driver. (i.e. the maximum
> > +Queue Size.)
>
> Maybe
>
> "The device MUST observe any queue configuration that may have been
> changed by the driver, like the maximum queue size."
>
> Although I'm not sure about 'observe'. Anybody have a better term?
>
> > +
> > +\drivernormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> > +
> > +The driver MUST apply for resources, set new configuration to the device, and
> > +finally activate the device.
>
> Maybe
>
> "When re-enabling a queue, the driver MUST configure the queue resources
> as during initial virtqueue discovery, but optionally with different
> parameters."
>
> ?
>
> What actually needs to be done is dependent on the transport anyway.
>
> What do you mean by "activate the device"? I thought the device state
> remained unchanged, it is just the individual queue that is reset and
> re-enabled?

Yes.

Thanks very much.

>
> > +
> >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> >
> >  We start with an overview of device initialization, then expand on the
> > @@ -6673,6 +6714,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >    transport specific.
> >    For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
> >
> > +  \item[VIRTIO_F_RING_RESET(40)] This feature indicates
> > +  that the driver can reset a queue individually.
> > +  See \ref{sec:Virtqueues / Virtqueue Reset}.
> > +
> >  \end{description}
> >
> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>

---------------------------------------------------------------------
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] Re: [PATCH v4 1/3] virtio: introduce virtqueue reset as basic facility

[Jason Wang]

On Wed, Sep 29, 2021 at 10:01 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 28 Sep 2021 12:06:01 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
> > On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > > This patch allows the driver to reset a queue individually.
> > >
> > > This is very common on general network equipment. By disabling a queue,
> > > you can quickly reclaim the buffer currently on the queue. If necessary,
> > > we can reinitialize the queue separately.
> > >
> > > For example, when virtio-net implements support for AF_XDP, we need to
> > > disable a queue to release all the original buffers when AF_XDP setup.
> > > And quickly release all the AF_XDP buffers that have been placed in the
> > > queue when AF_XDP exits.
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >  content.tex | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 45 insertions(+)
> > >
> > > diff --git a/content.tex b/content.tex
> > > index 37c45d3..05b05ba 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -407,6 +407,47 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
> > >  types. It is RECOMMENDED that devices generate version 4
> > >  UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
> > >

Btw, we need to add this into the section of "Virtqueues"

> > > +\section{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset}
> > > +
> > > +When VIRTIO_F_RING_RESET is negotiated, the driver can reset a virtqueue
> > > +individually. The way to reset the virtqueue is transport specific.
> > > +
> > > +Virtqueue reset is divided into two parts. The driver reset a queue
> > > +first, and then re-enable the queue. The re-enable part is optional.
> >
> > Maybe
> >
> > "The driver first resets a queue and can afterwards optionally re-enable
> > it."
> >
> > ?
> >
> > > +
> > > +\subsection{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > > +
> > > +\devicenormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > > +
> > > +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.
> > > +
> > > +The device MUST re-initialize any state of a virtqueue that has been
> > > +reset, including available and used state.

Maybe "The device must reset any state..."

> > > +
> > > +\drivernormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > > +
> > > +After the driver tell the device to reset a queue, the driver MUST verify that
> >
> > s/tell/tells/ (may have been my typo...)
> >
> > > +the queue has actually been reset.
> > > +
> > > +After the queue has been successfully reset, the driver MAY release any
> > > +resource associated with that virtqueue.
> > > +
> > > +\subsection{Virtqueue Re-enable}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> > > +
> > > +This process is the same as the initialization process of a single queue during
> > > +the initialization of the entire device.
> > > +
> > > +\devicenormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> > > +
> > > +The device MUST receive the new configuration from the driver. (i.e. the maximum
> > > +Queue Size.)
> >
> > Maybe
> >
> > "The device MUST observe any queue configuration that may have been
> > changed by the driver, like the maximum queue size."
> >
> > Although I'm not sure about 'observe'. Anybody have a better term?

I wonder if this is implied in the queue_enable or we need to explain
the semantics of "queue_enable" instead of here. Considering we list
queue reset and basic facility, we need to explicitly add queue enable
into the basic facility first.

> >
> > > +
> > > +\drivernormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> > > +
> > > +The driver MUST apply for resources, set new configuration to the device, and
> > > +finally activate the device.
> >
> > Maybe
> >
> > "When re-enabling a queue, the driver MUST configure the queue resources
> > as during initial virtqueue discovery, but optionally with different
> > parameters."
> >
> > ?

If we make the queue enablement as a subsection, we can move this
there. Then we don't need to differ enable and re-enable.

Thanks

> >
> > What actually needs to be done is dependent on the transport anyway.
> >
> > What do you mean by "activate the device"? I thought the device state
> > remained unchanged, it is just the individual queue that is reset and
> > re-enabled?
>
> Yes.
>
> Thanks very much.
>
> >
> > > +
> > >  \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> > >
> > >  We start with an overview of device initialization, then expand on the
> > > @@ -6673,6 +6714,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >    transport specific.
> > >    For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
> > >
> > > +  \item[VIRTIO_F_RING_RESET(40)] This feature indicates
> > > +  that the driver can reset a queue individually.
> > > +  See \ref{sec:Virtqueues / Virtqueue Reset}.
> > > +
> > >  \end{description}
> > >
> > >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>

Re: [PATCH v4 2/3] virtio: pci support virtqueue reset

[Xuan Zhuo]

On Tue, 28 Sep 2021 12:20:46 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
> On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> > PCI support virtqueue reset.
> >
> > virtio_pci_common_cfg add "queue_reset" to support virtqueue reset.
> > The driver uses this to selectively reset the queue.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  content.tex | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
>
> (...)
>
> > @@ -1020,6 +1027,20 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >
> >  The device MUST present a 0 in \field{queue_enable} on reset.
> >
> > +If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
> > +\field{queue_enable} after the driver has reset the virtqueue via
> > +\field{queue_reset}.
> > +
> > +If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in
> > +\field{queue_reset} on reset.
> > +
> > +If VIRTIO_F_RING_RESET has been negotiated, The device MUST present a 0 in
>
> s/The/the/
>
> > +\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
> > +driver re-enables the queue via \field{queue_enable}. (see \ref{sec:Virtqueues / Virtqueue Reset}).
>
> "...or the device is reset." ?
>

    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
    driver re-enables the queue via \field{queue_enable} or the device is reset. (see \ref{sec:Virtqueues / Virtqueue Reset}).

Is that so? Doesn't it feel necessary?


> Maybe also
>
> "The device MAY change the value of \field{queue_size} if the queue has
> been reset." ?
>
> Should it always set that field to the currently maximum supported queue
> size (assuming that can change dynamically)? Do we need some kind of
> synchronization for those changes?

When the queue is reset, all states of this queue MUST be modified to the
initial value. For example, queue_size MUST be reset to the maximum value
supported by the device. Because in the last reset queue or the entire device
reset process, the driver will modify the queue_size of the device so that its
value may be less than the maximum value.

Thanks.

>
> > +
> >  The device MUST present a 0 in \field{queue_size} if the virtqueue
> >  corresponding to the current \field{queue_select} is unavailable.
> >
> > @@ -1044,6 +1065,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
> > +been set up correctly. (see \ref{sec:Virtqueues / Virtqueue Reset}).
>
> Maybe also something like
>
> "The driver MAY set driver-writeable queue configuration values to
> different values than those that were used before the queue reset."
>
> ?
>
> > +
> >  \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
> >
> >  The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG
>

Re: [virtio-dev] Re: [PATCH v4 1/3] virtio: introduce virtqueue reset as basic facility

[Cornelia Huck]

On Wed, Sep 29 2021, Jason Wang <jasowang@redhat.com> wrote:

> On Wed, Sep 29, 2021 at 10:01 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>>
>> On Tue, 28 Sep 2021 12:06:01 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
>> > On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>> >
>> > > This patch allows the driver to reset a queue individually.
>> > >
>> > > This is very common on general network equipment. By disabling a queue,
>> > > you can quickly reclaim the buffer currently on the queue. If necessary,
>> > > we can reinitialize the queue separately.
>> > >
>> > > For example, when virtio-net implements support for AF_XDP, we need to
>> > > disable a queue to release all the original buffers when AF_XDP setup.
>> > > And quickly release all the AF_XDP buffers that have been placed in the
>> > > queue when AF_XDP exits.
>> > >
>> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
>> > > ---
>> > >  content.tex | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> > >  1 file changed, 45 insertions(+)
>> > >
>> > > diff --git a/content.tex b/content.tex
>> > > index 37c45d3..05b05ba 100644
>> > > --- a/content.tex
>> > > +++ b/content.tex
>> > > @@ -407,6 +407,47 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
>> > >  types. It is RECOMMENDED that devices generate version 4
>> > >  UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
>> > >
>
> Btw, we need to add this into the section of "Virtqueues"


Hm, isn't it already?

>
>> > > +\section{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset}
>> > > +
>> > > +When VIRTIO_F_RING_RESET is negotiated, the driver can reset a virtqueue
>> > > +individually. The way to reset the virtqueue is transport specific.
>> > > +
>> > > +Virtqueue reset is divided into two parts. The driver reset a queue
>> > > +first, and then re-enable the queue. The re-enable part is optional.
>> >
>> > Maybe
>> >
>> > "The driver first resets a queue and can afterwards optionally re-enable
>> > it."
>> >
>> > ?
>> >
>> > > +
>> > > +\subsection{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Reset}
>> > > +
>> > > +\devicenormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
>> > > +
>> > > +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.
>> > > +
>> > > +The device MUST re-initialize any state of a virtqueue that has been
>> > > +reset, including available and used state.
>
> Maybe "The device must reset any state..."

"reset to defaults"? Otherwise, it seems a bit redundant.

>
>> > > +
>> > > +\drivernormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
>> > > +
>> > > +After the driver tell the device to reset a queue, the driver MUST verify that
>> >
>> > s/tell/tells/ (may have been my typo...)
>> >
>> > > +the queue has actually been reset.
>> > > +
>> > > +After the queue has been successfully reset, the driver MAY release any
>> > > +resource associated with that virtqueue.
>> > > +
>> > > +\subsection{Virtqueue Re-enable}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
>> > > +
>> > > +This process is the same as the initialization process of a single queue during
>> > > +the initialization of the entire device.
>> > > +
>> > > +\devicenormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
>> > > +
>> > > +The device MUST receive the new configuration from the driver. (i.e. the maximum
>> > > +Queue Size.)
>> >
>> > Maybe
>> >
>> > "The device MUST observe any queue configuration that may have been
>> > changed by the driver, like the maximum queue size."
>> >
>> > Although I'm not sure about 'observe'. Anybody have a better term?
>
> I wonder if this is implied in the queue_enable or we need to explain
> the semantics of "queue_enable" instead of here. Considering we list
> queue reset and basic facility, we need to explicitly add queue enable
> into the basic facility first.

I'm wondering whether we need to clarify explicitly that the driver may
re-enable the queue with different parameters?

>
>> >
>> > > +
>> > > +\drivernormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
>> > > +
>> > > +The driver MUST apply for resources, set new configuration to the device, and
>> > > +finally activate the device.
>> >
>> > Maybe
>> >
>> > "When re-enabling a queue, the driver MUST configure the queue resources
>> > as during initial virtqueue discovery, but optionally with different
>> > parameters."
>> >
>> > ?
>
> If we make the queue enablement as a subsection, we can move this
> there. Then we don't need to differ enable and re-enable.

Yes, I notice we are a bit light on details about queue
discovery/enablement. It's basically all in the transport-specific sections.


---------------------------------------------------------------------
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 v4 2/3] virtio: pci support virtqueue reset

[Cornelia Huck]

On Wed, Sep 29 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:

> On Tue, 28 Sep 2021 12:20:46 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
>> On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>> > +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
>> > +driver re-enables the queue via \field{queue_enable}. (see \ref{sec:Virtqueues / Virtqueue Reset}).
>>
>> "...or the device is reset." ?
>>
>
>     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
>     driver re-enables the queue via \field{queue_enable} or the device is reset. (see \ref{sec:Virtqueues / Virtqueue Reset}).
>
> Is that so? Doesn't it feel necessary?

Is queue_reset supposed to persist across device reset? That feels a bit
odd to me.

>
>
>> Maybe also
>>
>> "The device MAY change the value of \field{queue_size} if the queue has
>> been reset." ?
>>
>> Should it always set that field to the currently maximum supported queue
>> size (assuming that can change dynamically)? Do we need some kind of
>> synchronization for those changes?
>
> When the queue is reset, all states of this queue MUST be modified to the
> initial value. For example, queue_size MUST be reset to the maximum value
> supported by the device. Because in the last reset queue or the entire device
> reset process, the driver will modify the queue_size of the device so that its
> value may be less than the maximum value.

I think the question is whether the device may choose a different
initial maximum value once the queue has been reset. If it does change
the value, there may be a race where the driver has reset the queue,
read queue_reset back, read the max queue size, and the device only then
changing the max queue size. Maybe I'm overthinking this.


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

Re: [PATCH v4 2/3] virtio: pci support virtqueue reset

[Jason Wang]

On Thu, Sep 30, 2021 at 12:33 AM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Wed, Sep 29 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> > On Tue, 28 Sep 2021 12:20:46 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
> >> On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >> > +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
> >> > +driver re-enables the queue via \field{queue_enable}. (see \ref{sec:Virtqueues / Virtqueue Reset}).
> >>
> >> "...or the device is reset." ?
> >>
> >
> >     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
> >     driver re-enables the queue via \field{queue_enable} or the device is reset. (see \ref{sec:Virtqueues / Virtqueue Reset}).
> >
> > Is that so? Doesn't it feel necessary?
>
> Is queue_reset supposed to persist across device reset? That feels a bit
> odd to me.

I think it's better to follow e.g the pci "status".

Driver writes 1 to queue_reset. And the device notified the completion
by presenting 0. This looks easier to be dealt with during device
reset.


>
> >
> >
> >> Maybe also
> >>
> >> "The device MAY change the value of \field{queue_size} if the queue has
> >> been reset." ?
> >>
> >> Should it always set that field to the currently maximum supported queue
> >> size (assuming that can change dynamically)? Do we need some kind of
> >> synchronization for those changes?
> >
> > When the queue is reset, all states of this queue MUST be modified to the
> > initial value. For example, queue_size MUST be reset to the maximum value
> > supported by the device. Because in the last reset queue or the entire device
> > reset process, the driver will modify the queue_size of the device so that its
> > value may be less than the maximum value.
>
> I think the question is whether the device may choose a different
> initial maximum value once the queue has been reset. If it does change
> the value, there may be a race where the driver has reset the queue,
> read queue_reset back, read the max queue size, and the device only then
> changing the max queue size. Maybe I'm overthinking this.

I think the driver should do the exact same steps as the device initialization.

Not sure it's worth mentioning here.

Thanks

>

Re: [virtio-dev] Re: [PATCH v4 1/3] virtio: introduce virtqueue reset as basic facility

[Jason Wang]

On Thu, Sep 30, 2021 at 12:24 AM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Wed, Sep 29 2021, Jason Wang <jasowang@redhat.com> wrote:
>
> > On Wed, Sep 29, 2021 at 10:01 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >>
> >> On Tue, 28 Sep 2021 12:06:01 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
> >> > On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >> >
> >> > > This patch allows the driver to reset a queue individually.
> >> > >
> >> > > This is very common on general network equipment. By disabling a queue,
> >> > > you can quickly reclaim the buffer currently on the queue. If necessary,
> >> > > we can reinitialize the queue separately.
> >> > >
> >> > > For example, when virtio-net implements support for AF_XDP, we need to
> >> > > disable a queue to release all the original buffers when AF_XDP setup.
> >> > > And quickly release all the AF_XDP buffers that have been placed in the
> >> > > queue when AF_XDP exits.
> >> > >
> >> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> >> > > ---
> >> > >  content.tex | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >> > >  1 file changed, 45 insertions(+)
> >> > >
> >> > > diff --git a/content.tex b/content.tex
> >> > > index 37c45d3..05b05ba 100644
> >> > > --- a/content.tex
> >> > > +++ b/content.tex
> >> > > @@ -407,6 +407,47 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
> >> > >  types. It is RECOMMENDED that devices generate version 4
> >> > >  UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
> >> > >
> >
> > Btw, we need to add this into the section of "Virtqueues"
>
>
> Hm, isn't it already?

Looks not, this follows the section of "Exporting Objects".

>
> >
> >> > > +\section{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset}
> >> > > +
> >> > > +When VIRTIO_F_RING_RESET is negotiated, the driver can reset a virtqueue
> >> > > +individually. The way to reset the virtqueue is transport specific.
> >> > > +
> >> > > +Virtqueue reset is divided into two parts. The driver reset a queue
> >> > > +first, and then re-enable the queue. The re-enable part is optional.
> >> >
> >> > Maybe
> >> >
> >> > "The driver first resets a queue and can afterwards optionally re-enable
> >> > it."
> >> >
> >> > ?
> >> >
> >> > > +
> >> > > +\subsection{Virtqueue Reset}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Reset}
> >> > > +
> >> > > +\devicenormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
> >> > > +
> >> > > +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.
> >> > > +
> >> > > +The device MUST re-initialize any state of a virtqueue that has been
> >> > > +reset, including available and used state.
> >
> > Maybe "The device must reset any state..."
>
> "reset to defaults"? Otherwise, it seems a bit redundant.

Right.

>
> >
> >> > > +
> >> > > +\drivernormative{\subsubsection}{Virtqueue Reset}{Virtqueues / Virtqueue Reset / Virtqueue Reset}
> >> > > +
> >> > > +After the driver tell the device to reset a queue, the driver MUST verify that
> >> >
> >> > s/tell/tells/ (may have been my typo...)
> >> >
> >> > > +the queue has actually been reset.
> >> > > +
> >> > > +After the queue has been successfully reset, the driver MAY release any
> >> > > +resource associated with that virtqueue.
> >> > > +
> >> > > +\subsection{Virtqueue Re-enable}\label{sec:Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> >> > > +
> >> > > +This process is the same as the initialization process of a single queue during
> >> > > +the initialization of the entire device.
> >> > > +
> >> > > +\devicenormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> >> > > +
> >> > > +The device MUST receive the new configuration from the driver. (i.e. the maximum
> >> > > +Queue Size.)
> >> >
> >> > Maybe
> >> >
> >> > "The device MUST observe any queue configuration that may have been
> >> > changed by the driver, like the maximum queue size."
> >> >
> >> > Although I'm not sure about 'observe'. Anybody have a better term?
> >
> > I wonder if this is implied in the queue_enable or we need to explain
> > the semantics of "queue_enable" instead of here. Considering we list
> > queue reset and basic facility, we need to explicitly add queue enable
> > into the basic facility first.
>
> I'm wondering whether we need to clarify explicitly that the driver may
> re-enable the queue with different parameters?

I think we need, my understanding is that at least the driver can set
a different queue address etc.

>
> >
> >> >
> >> > > +
> >> > > +\drivernormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
> >> > > +
> >> > > +The driver MUST apply for resources, set new configuration to the device, and
> >> > > +finally activate the device.
> >> >
> >> > Maybe
> >> >
> >> > "When re-enabling a queue, the driver MUST configure the queue resources
> >> > as during initial virtqueue discovery, but optionally with different
> >> > parameters."
> >> >
> >> > ?
> >
> > If we make the queue enablement as a subsection, we can move this
> > there. Then we don't need to differ enable and re-enable.
>
> Yes, I notice we are a bit light on details about queue
> discovery/enablement. It's basically all in the transport-specific sections.
>

Yes.
Thanks

Re: [virtio-dev] Re: [PATCH v4 1/3] virtio: introduce virtqueue reset as basic facility

[Cornelia Huck]

On Thu, Sep 30 2021, Jason Wang <jasowang@redhat.com> wrote:

> On Thu, Sep 30, 2021 at 12:24 AM Cornelia Huck <cohuck@redhat.com> wrote:
>>
>> On Wed, Sep 29 2021, Jason Wang <jasowang@redhat.com> wrote:
>>
>> > On Wed, Sep 29, 2021 at 10:01 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>> >>
>> >> On Tue, 28 Sep 2021 12:06:01 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
>> >> > On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>> >> > > @@ -407,6 +407,47 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
>> >> > >  types. It is RECOMMENDED that devices generate version 4
>> >> > >  UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
>> >> > >
>> >
>> > Btw, we need to add this into the section of "Virtqueues"
>>
>>
>> Hm, isn't it already?
>
> Looks not, this follows the section of "Exporting Objects".

Ah, now I see. Yes, this needs to be moved.

>> >> > > +\devicenormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
>> >> > > +
>> >> > > +The device MUST receive the new configuration from the driver. (i.e. the maximum
>> >> > > +Queue Size.)
>> >> >
>> >> > Maybe
>> >> >
>> >> > "The device MUST observe any queue configuration that may have been
>> >> > changed by the driver, like the maximum queue size."
>> >> >
>> >> > Although I'm not sure about 'observe'. Anybody have a better term?
>> >
>> > I wonder if this is implied in the queue_enable or we need to explain
>> > the semantics of "queue_enable" instead of here. Considering we list
>> > queue reset and basic facility, we need to explicitly add queue enable
>> > into the basic facility first.
>>
>> I'm wondering whether we need to clarify explicitly that the driver may
>> re-enable the queue with different parameters?
>
> I think we need, my understanding is that at least the driver can set
> a different queue address etc.

Yes, that seems fundamental.

Maybe we can move that to the individual transports, and note there for
the individual fields that they may be populated by different values
after a queue reset? Although I do not like that very much. However, I'm
struggling a bit with a good wording here.

>
>>
>> >
>> >> >
>> >> > > +
>> >> > > +\drivernormative{\subsubsection}{Virtqueue Re-enable}{Virtqueues / Virtqueue Reset / Virtqueue Re-enable}
>> >> > > +
>> >> > > +The driver MUST apply for resources, set new configuration to the device, and
>> >> > > +finally activate the device.
>> >> >
>> >> > Maybe
>> >> >
>> >> > "When re-enabling a queue, the driver MUST configure the queue resources
>> >> > as during initial virtqueue discovery, but optionally with different
>> >> > parameters."
>> >> >
>> >> > ?
>> >
>> > If we make the queue enablement as a subsection, we can move this
>> > there. Then we don't need to differ enable and re-enable.
>>
>> Yes, I notice we are a bit light on details about queue
>> discovery/enablement. It's basically all in the transport-specific sections.
>>
>
> Yes.
> Thanks


---------------------------------------------------------------------
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 v4 2/3] virtio: pci support virtqueue reset

[Cornelia Huck]

On Thu, Sep 30 2021, Jason Wang <jasowang@redhat.com> wrote:

> On Thu, Sep 30, 2021 at 12:33 AM Cornelia Huck <cohuck@redhat.com> wrote:
>>
>> On Wed, Sep 29 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>>
>> > On Tue, 28 Sep 2021 12:20:46 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
>> >> On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>> >> > +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
>> >> > +driver re-enables the queue via \field{queue_enable}. (see \ref{sec:Virtqueues / Virtqueue Reset}).
>> >>
>> >> "...or the device is reset." ?
>> >>
>> >
>> >     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
>> >     driver re-enables the queue via \field{queue_enable} or the device is reset. (see \ref{sec:Virtqueues / Virtqueue Reset}).
>> >
>> > Is that so? Doesn't it feel necessary?
>>
>> Is queue_reset supposed to persist across device reset? That feels a bit
>> odd to me.
>
> I think it's better to follow e.g the pci "status".
>
> Driver writes 1 to queue_reset. And the device notified the completion
> by presenting 0. This looks easier to be dealt with during device
> reset.

Indeed, that might be more clear, let's go with that (same with MMIO.)

>
>
>>
>> >
>> >
>> >> Maybe also
>> >>
>> >> "The device MAY change the value of \field{queue_size} if the queue has
>> >> been reset." ?
>> >>
>> >> Should it always set that field to the currently maximum supported queue
>> >> size (assuming that can change dynamically)? Do we need some kind of
>> >> synchronization for those changes?
>> >
>> > When the queue is reset, all states of this queue MUST be modified to the
>> > initial value. For example, queue_size MUST be reset to the maximum value
>> > supported by the device. Because in the last reset queue or the entire device
>> > reset process, the driver will modify the queue_size of the device so that its
>> > value may be less than the maximum value.
>>
>> I think the question is whether the device may choose a different
>> initial maximum value once the queue has been reset. If it does change
>> the value, there may be a race where the driver has reset the queue,
>> read queue_reset back, read the max queue size, and the device only then
>> changing the max queue size. Maybe I'm overthinking this.
>
> I think the driver should do the exact same steps as the device initialization.
>
> Not sure it's worth mentioning here.

Hm, we might have similar problems there... but they are probably not
really problems in practice. So just a note that the device may fill in
the fields with different values might be enough. Like

"The device MAY present different default values after queue reset."

?


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

Re: [PATCH v4 2/3] virtio: pci support virtqueue reset

[Jason Wang]

On Thu, Sep 30, 2021 at 7:08 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Thu, Sep 30 2021, Jason Wang <jasowang@redhat.com> wrote:
>
> > On Thu, Sep 30, 2021 at 12:33 AM Cornelia Huck <cohuck@redhat.com> wrote:
> >>
> >> On Wed, Sep 29 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >>
> >> > On Tue, 28 Sep 2021 12:20:46 +0200, Cornelia Huck <cohuck@redhat.com> wrote:
> >> >> On Tue, Sep 28 2021, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >> >> > +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
> >> >> > +driver re-enables the queue via \field{queue_enable}. (see \ref{sec:Virtqueues / Virtqueue Reset}).
> >> >>
> >> >> "...or the device is reset." ?
> >> >>
> >> >
> >> >     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
> >> >     driver re-enables the queue via \field{queue_enable} or the device is reset. (see \ref{sec:Virtqueues / Virtqueue Reset}).
> >> >
> >> > Is that so? Doesn't it feel necessary?
> >>
> >> Is queue_reset supposed to persist across device reset? That feels a bit
> >> odd to me.
> >
> > I think it's better to follow e.g the pci "status".
> >
> > Driver writes 1 to queue_reset. And the device notified the completion
> > by presenting 0. This looks easier to be dealt with during device
> > reset.
>
> Indeed, that might be more clear, let's go with that (same with MMIO.)
>
> >
> >
> >>
> >> >
> >> >
> >> >> Maybe also
> >> >>
> >> >> "The device MAY change the value of \field{queue_size} if the queue has
> >> >> been reset." ?
> >> >>
> >> >> Should it always set that field to the currently maximum supported queue
> >> >> size (assuming that can change dynamically)? Do we need some kind of
> >> >> synchronization for those changes?
> >> >
> >> > When the queue is reset, all states of this queue MUST be modified to the
> >> > initial value. For example, queue_size MUST be reset to the maximum value
> >> > supported by the device. Because in the last reset queue or the entire device
> >> > reset process, the driver will modify the queue_size of the device so that its
> >> > value may be less than the maximum value.
> >>
> >> I think the question is whether the device may choose a different
> >> initial maximum value once the queue has been reset. If it does change
> >> the value, there may be a race where the driver has reset the queue,
> >> read queue_reset back, read the max queue size, and the device only then
> >> changing the max queue size. Maybe I'm overthinking this.
> >
> > I think the driver should do the exact same steps as the device initialization.
> >
> > Not sure it's worth mentioning here.
>
> Hm, we might have similar problems there... but they are probably not
> really problems in practice. So just a note that the device may fill in
> the fields with different values might be enough. Like
>
> "The device MAY present different default values after queue reset."
>
> ?
>

That's fine.

Thanks