[virtio-comment] [PATCH 0/2] Support enabling virtqueue after DRIVER_OK

[virtio-comment] [PATCH 0/2] Support enabling virtqueue after DRIVER_OK

[Parav Pandit]

Summary:
========
This patch enables driver to create virtqueues after DRIVER_OK
status bit is set.

This patch take the inspiration from the thread [2] with credits to
Eugenio PÃrez.

Details:
========
Currently, a virtqueue must be enabled before driver has set the
DRIVER_OK status bit.

spec citation to section "Driver Requirements: Device Initialization"

"Perform device-specific setup, including discovery of virtqueues
for the device, optional per-bus setup, reading and possibly writing
the device’s virtio configuration space, and population of virtqueues."

This implies that a virtqueue must be enabled before reaching the
DRIVER_OK stage. There was no explicit mention about ability to
enable the virtqueue after DRIVER_OK stage.

In following usecases, creating a virtqueue after DRIVER_OK phase is
desired.

Use cases:
=========
1. Dynamically create aq when administrative commands to be used.
2. Dynamically create the net device tx/rxq when device is
   opened when deploying for a container.
   In a container, number of virtqueues to be used may be <= max queues.
3. Dynamically create flow filter queues of netdevice when
   ARFS or ethtool filters are enabled as listed in [1].
4. Dynamically create rtc functionality related read virtqueue only
   when net device when timestamping to be used.
5. When XDP program is set, one can create additional XDP specific
   queues without affecting existing queues.

Hence, This patch introduces an existing queue enable and disable
(aka reset) facility and a new feature bit to explicitly indicate such
support by the device.

With this feature, drivers can skip optional queues creation during
driver init time. For example, a Linux net device driver
can create/destroy the transmit and receive queues when net device's
ndo_open() and ndo_stop() callbacks are invoked respectively.

As side effect, it also enables driver to not consume MSI-X vectors for
the PCI device at driver load time and utilize(enable) individual vector
when needed. This further helps on some cpus at high scale where a
interrupt line may not be available from the platform.

[1] https://lists.oasis-open.org/archives/virtio-comment/202308/msg00263.html
[2] https://lists.oasis-open.org/archives/virtio-comment/202306/msg00097.html

Patch summary:
==============
patch-1 fixes to add virtqueue reset conformance references
patch-2 adds VIRTIO_F_RING_DYNAMIC feature bit, requirements

Please review.

Signed-off-by: Parav Pandit <parav@nvidia.com>

Parav Pandit (2):
  conformance: Add missing virtqueue reset conformance references
  content: Support enabling virtqueue after DRIVER_OK stage

 conformance.tex |  4 ++++
 content.tex     | 54 ++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 3 deletions(-)

-- 
2.34.1


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

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

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

[virtio-comment] [PATCH 1/2] conformance: Add missing virtqueue reset conformance references

[Parav Pandit]

Add the missing references to the virtqueue reset related conformance
requirements.

Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 conformance.tex | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/conformance.tex b/conformance.tex
index dc00e84..863f9c5 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -76,6 +76,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Reset}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
+\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
@@ -162,6 +163,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
+\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
-- 
2.34.1


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

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

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

[virtio-comment] Re: [PATCH 1/2] conformance: Add missing virtqueue reset conformance references

[Xuan Zhuo]

On Sat, 9 Sep 2023 17:32:17 +0300, Parav Pandit <parav@nvidia.com> wrote:
> Add the missing references to the virtqueue reset related conformance
> requirements.
>
> Signed-off-by: Parav Pandit <parav@nvidia.com>

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


> ---
>  conformance.tex | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/conformance.tex b/conformance.tex
> index dc00e84..863f9c5 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -76,6 +76,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Reset}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
> +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> @@ -162,6 +163,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> --
> 2.34.1
>

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

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

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

[virtio-comment] [PATCH 2/2] content: Support enabling virtqueue after DRIVER_OK stage

[Parav Pandit]

Currently, a virtqueue must be enabled before driver has set the
DRIVER_OK status bit.

spec citation to section "Driver Requirements: Device Initialization"

"Perform device-specific setup, including discovery of virtqueues
for the device, optional per-bus setup, reading and possibly writing
the device’s virtio configuration space, and population of virtqueues."

This implies that a virtqueue must be enabled before reaching the
DRIVER_OK stage. There was no explicit mention about ability to
enable the virtqueue after DRIVER_OK stage.

In following usecases, creating a virtqueue after DRIVER_OK phase is
desired.

1. Dynamically create aq when administrative commands to be used.
2. Dynamically create the net device tx/rxq when device is
   opened when deploying for a container.
   In a container, number of virtqueues to be used may be <= max queues.
3. Dynamically create flow filter queues of netdevice when
   ARFS or ethtool filters are enabled as listed in [1].
4. Dynamically create rtc functionality related read virtqueue only
   when net device when timestamping to be used.
5. When XDP program is set, one can create additional XDP specific
   queues without affecting existing queues.

Hence, This patch introduces an existing queue enable and disable
(aka reset) facility and a new feature bit to explicitly indicate such
support by the device.

With this feature, drivers can skip optional queues creation during
driver init time. For example, a Linux net device driver
can create/destroy the transmit and receive queues when net device's
ndo_open() and ndo_stop() callbacks are invoked respectively.

As side effect, it also enables driver to not consume MSI-X vectors for
the PCI device at driver load time and utilize(enable) individual vector
when needed. This further helps on some cpus at high scale where a
interrupt line may not be available from the platform.

[1] https://lists.oasis-open.org/archives/virtio-comment/202308/msg00263.html

Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 conformance.tex |  2 ++
 content.tex     | 54 ++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/conformance.tex b/conformance.tex
index 863f9c5..07b0b7b 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -77,6 +77,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
+\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
@@ -164,6 +165,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
+\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
diff --git a/content.tex b/content.tex
index 0a62dce..91aee25 100644
--- a/content.tex
+++ b/content.tex
@@ -102,7 +102,7 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 \item[24 to 41] Feature bits reserved for extensions to the queue and
   feature negotiation mechanisms
 
-\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
+\item[43 to 49, and 128 and above] Feature bits reserved for future extensions.
 \end{description}
 
 \begin{note}
@@ -440,6 +440,42 @@ \subsubsection{Virtqueue Re-enable}\label{sec:Basic Facilities of a Virtio Devic
 as during initial virtqueue discovery, but optionally with different
 parameters.
 
+\subsection{Dynamic Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues}
+
+When VIRTIO_F_RING_DYNAMIC is not negotiated, the virtqueues are
+enabled when the device is initialized, i.e. after the driver has ensured that
+the device has set the FEATURES_OK status bit and before the driver setting
+the DRIVER_OK status bit.
+
+When VIRTIO_F_RING_DYNAMIC is negotiated, the driver can avoid enabling the queue
+before setting the DRIVER_OK status bit; the driver can enable the queue after
+the driver has set the DRIVER_OK status bit. The queue enable mechanism is
+transport specific.
+
+\devicenormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
+
+When VIRTIO_F_RING_DYNAMIC is negotiated, the device MUST allow enabling the
+virtqueue which was not enabled during the device initialization phase
+i.e. after the device has set the FEATURES_OK status bit and before the
+driver has set the DRIVER_OK status bit.
+
+\drivernormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
+
+When VIRTIO_F_RING_DYNAMIC is negotiated,
+\begin{itemize}
+\item
+the driver MAY not enable all the virtqueues during the device initialization phase;
+
+\item the driver MAY enable some or all the virtqueues after the driver has set the
+	DRIVER_OK status bit.
+
+\item the driver MAY enable all the virtqueues during the device initialization phase;
+\end{itemize}
+
+The driver interested in enabling and disabling the virtqueue dynamically
+after setting the DRIVER_OK status bit MUST negotiate VIRTIO_F_RING_DYNAMIC
+and VIRTIO_F_RING_RESET features.
+
 \input{split-ring.tex}
 
 \input{packed-ring.tex}
@@ -537,7 +573,9 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
 
 \item\label{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} Perform device-specific setup, including discovery of virtqueues for the
    device, optional per-bus setup, reading and possibly writing the
-   device's virtio configuration space, and population of virtqueues.
+   device's virtio configuration space.
+
+\item\label{itm:General Initialization And Device Operation / Device Initialization / Virtqueue Setup} Configure and enable the virtqueues.
 
 \item\label{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit.  At this point the device is
    ``live''.
@@ -551,6 +589,10 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
 The driver MUST NOT send any buffer available notifications to
 the device before setting DRIVER_OK.
 
+The driver MAY skip step
+\ref{itm:General Initialization And Device Operation / Device Initialization / Virtqueue Setup}
+when driver has negotiated VIRTIO_F_RING_DYNAMIC feature.
+
 \subsection{Legacy Interface: Device Initialization}\label{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
 Legacy devices did not support the FEATURES_OK status bit, and thus did
 not have a graceful way for the device to indicate unsupported feature
@@ -569,7 +611,7 @@ \subsection{Legacy Interface: Device Initialization}\label{sec:General Initializ
 Initialization / Re-read FEATURES-OK} were omitted, and steps
 \ref{itm:General Initialization And Device Operation /
 Device Initialization / Read feature bits},
-\ref{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} and \ref{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK}
+\ref{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup}, \ref{itm:General Initialization And Device Operation / Device Initialization / Virtqueue Setup} and \ref{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK}
 were conflated.
 
 Therefore, when using the legacy interface:
@@ -872,8 +914,14 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
 	handling features reserved for future use.
 
+  \item[VIRTIO_F_RING_DYNAMIC(42)] This feature indicates
+  that the driver can enable a virtqueue individually after the driver has
+  set the status bit of DRIVER_OK.
+  See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues}.
+
 \end{description}
 
+
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
 
 A driver MUST accept VIRTIO_F_VERSION_1 if it is offered.  A driver
-- 
2.34.1


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

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

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

[virtio-comment] Re: [PATCH 2/2] content: Support enabling virtqueue after DRIVER_OK stage

[Xuan Zhuo]

On Sat, 9 Sep 2023 17:32:18 +0300, Parav Pandit <parav@nvidia.com> wrote:
> Currently, a virtqueue must be enabled before driver has set the
> DRIVER_OK status bit.
>
> spec citation to section "Driver Requirements: Device Initialization"
>
> "Perform device-specific setup, including discovery of virtqueues
> for the device, optional per-bus setup, reading and possibly writing
> the device’s virtio configuration space, and population of virtqueues."
>
> This implies that a virtqueue must be enabled before reaching the
> DRIVER_OK stage. There was no explicit mention about ability to
> enable the virtqueue after DRIVER_OK stage.
>
> In following usecases, creating a virtqueue after DRIVER_OK phase is
> desired.
>
> 1. Dynamically create aq when administrative commands to be used.
> 2. Dynamically create the net device tx/rxq when device is
>    opened when deploying for a container.
>    In a container, number of virtqueues to be used may be <= max queues.
> 3. Dynamically create flow filter queues of netdevice when
>    ARFS or ethtool filters are enabled as listed in [1].
> 4. Dynamically create rtc functionality related read virtqueue only
>    when net device when timestamping to be used.
> 5. When XDP program is set, one can create additional XDP specific
>    queues without affecting existing queues.
>
> Hence, This patch introduces an existing queue enable and disable
> (aka reset) facility and a new feature bit to explicitly indicate such
> support by the device.
>
> With this feature, drivers can skip optional queues creation during
> driver init time. For example, a Linux net device driver
> can create/destroy the transmit and receive queues when net device's
> ndo_open() and ndo_stop() callbacks are invoked respectively.
>
> As side effect, it also enables driver to not consume MSI-X vectors for
> the PCI device at driver load time and utilize(enable) individual vector
> when needed. This further helps on some cpus at high scale where a
> interrupt line may not be available from the platform.
>
> [1] https://lists.oasis-open.org/archives/virtio-comment/202308/msg00263.html
>
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
>  conformance.tex |  2 ++
>  content.tex     | 54 ++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/conformance.tex b/conformance.tex
> index 863f9c5..07b0b7b 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -77,6 +77,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}

s/devicenormative/drivernormative/


>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> @@ -164,6 +165,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> diff --git a/content.tex b/content.tex
> index 0a62dce..91aee25 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -102,7 +102,7 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
>  \item[24 to 41] Feature bits reserved for extensions to the queue and
>    feature negotiation mechanisms
>
> -\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
> +\item[43 to 49, and 128 and above] Feature bits reserved for future extensions.
>  \end{description}
>
>  \begin{note}
> @@ -440,6 +440,42 @@ \subsubsection{Virtqueue Re-enable}\label{sec:Basic Facilities of a Virtio Devic
>  as during initial virtqueue discovery, but optionally with different
>  parameters.
>
> +\subsection{Dynamic Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues}
> +
> +When VIRTIO_F_RING_DYNAMIC is not negotiated, the virtqueues are
> +enabled when the device is initialized, i.e. after the driver has ensured that
> +the device has set the FEATURES_OK status bit and before the driver setting
> +the DRIVER_OK status bit.
> +
> +When VIRTIO_F_RING_DYNAMIC is negotiated, the driver can avoid enabling the queue
> +before setting the DRIVER_OK status bit; the driver can enable the queue after
> +the driver has set the DRIVER_OK status bit. The queue enable mechanism is
> +transport specific.
> +
> +\devicenormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> +
> +When VIRTIO_F_RING_DYNAMIC is negotiated, the device MUST allow enabling the
> +virtqueue which was not enabled during the device initialization phase
> +i.e. after the device has set the FEATURES_OK status bit and before the
> +driver has set the DRIVER_OK status bit.
> +
> +\drivernormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> +
> +When VIRTIO_F_RING_DYNAMIC is negotiated,
> +\begin{itemize}
> +\item
> +the driver MAY not enable all the virtqueues during the device initialization phase;

Remove the \n after \item

> +
> +\item the driver MAY enable some or all the virtqueues after the driver has set the
> +	DRIVER_OK status bit.
> +
> +\item the driver MAY enable all the virtqueues during the device initialization phase;
> +\end{itemize}
> +
> +The driver interested in enabling and disabling the virtqueue dynamically
> +after setting the DRIVER_OK status bit MUST negotiate VIRTIO_F_RING_DYNAMIC
> +and VIRTIO_F_RING_RESET features.
> +
>  \input{split-ring.tex}
>
>  \input{packed-ring.tex}
> @@ -537,7 +573,9 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
>
>  \item\label{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} Perform device-specific setup, including discovery of virtqueues for the
>     device, optional per-bus setup, reading and possibly writing the
> -   device's virtio configuration space, and population of virtqueues.
> +   device's virtio configuration space.
> +
> +\item\label{itm:General Initialization And Device Operation / Device Initialization / Virtqueue Setup} Configure and enable the virtqueues.
>
>  \item\label{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit.  At this point the device is
>     ``live''.
> @@ -551,6 +589,10 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
>  The driver MUST NOT send any buffer available notifications to
>  the device before setting DRIVER_OK.
>
> +The driver MAY skip step
> +\ref{itm:General Initialization And Device Operation / Device Initialization / Virtqueue Setup}
> +when driver has negotiated VIRTIO_F_RING_DYNAMIC feature.
> +
>  \subsection{Legacy Interface: Device Initialization}\label{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
>  Legacy devices did not support the FEATURES_OK status bit, and thus did
>  not have a graceful way for the device to indicate unsupported feature
> @@ -569,7 +611,7 @@ \subsection{Legacy Interface: Device Initialization}\label{sec:General Initializ
>  Initialization / Re-read FEATURES-OK} were omitted, and steps
>  \ref{itm:General Initialization And Device Operation /
>  Device Initialization / Read feature bits},
> -\ref{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} and \ref{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK}
> +\ref{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup}, \ref{itm:General Initialization And Device Operation / Device Initialization / Virtqueue Setup} and \ref{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK}
>  were conflated.
>
>  Therefore, when using the legacy interface:
> @@ -872,8 +914,14 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>  	handling features reserved for future use.
>
> +  \item[VIRTIO_F_RING_DYNAMIC(42)] This feature indicates
> +  that the driver can enable a virtqueue individually after the driver has
> +  set the status bit of DRIVER_OK.
> +  See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues}.
> +
>  \end{description}
>
> +

No add new empty line, if we do not need it.


Thanks.

>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>
>  A driver MUST accept VIRTIO_F_VERSION_1 if it is offered.  A driver
> --
> 2.34.1
>

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

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

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

RE: [virtio-comment] Re: [PATCH 2/2] content: Support enabling virtqueue after DRIVER_OK stage

[Parav Pandit]

> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> open.org> On Behalf Of Xuan Zhuo
> Sent: Wednesday, September 13, 2023 1:10 PM
> 
> On Sat, 9 Sep 2023 17:32:18 +0300, Parav Pandit <parav@nvidia.com> wrote:
> > Currently, a virtqueue must be enabled before driver has set the
> > DRIVER_OK status bit.
> >
> > spec citation to section "Driver Requirements: Device Initialization"
> >
> > "Perform device-specific setup, including discovery of virtqueues for
> > the device, optional per-bus setup, reading and possibly writing the
> > device’s virtio configuration space, and population of virtqueues."
> >
> > This implies that a virtqueue must be enabled before reaching the
> > DRIVER_OK stage. There was no explicit mention about ability to enable
> > the virtqueue after DRIVER_OK stage.
> >
> > In following usecases, creating a virtqueue after DRIVER_OK phase is
> > desired.
> >
> > 1. Dynamically create aq when administrative commands to be used.
> > 2. Dynamically create the net device tx/rxq when device is
> >    opened when deploying for a container.
> >    In a container, number of virtqueues to be used may be <= max queues.
> > 3. Dynamically create flow filter queues of netdevice when
> >    ARFS or ethtool filters are enabled as listed in [1].
> > 4. Dynamically create rtc functionality related read virtqueue only
> >    when net device when timestamping to be used.
> > 5. When XDP program is set, one can create additional XDP specific
> >    queues without affecting existing queues.
> >
> > Hence, This patch introduces an existing queue enable and disable (aka
> > reset) facility and a new feature bit to explicitly indicate such
> > support by the device.
> >
> > With this feature, drivers can skip optional queues creation during
> > driver init time. For example, a Linux net device driver can
> > create/destroy the transmit and receive queues when net device's
> > ndo_open() and ndo_stop() callbacks are invoked respectively.
> >
> > As side effect, it also enables driver to not consume MSI-X vectors
> > for the PCI device at driver load time and utilize(enable) individual
> > vector when needed. This further helps on some cpus at high scale
> > where a interrupt line may not be available from the platform.
> >
> > [1]
> > https://lists.oasis-open.org/archives/virtio-comment/202308/msg00263.h
> > tml
> >
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > ---
> >  conformance.tex |  2 ++
> >  content.tex     | 54 ++++++++++++++++++++++++++++++++++++++++++++++--
> -
> >  2 files changed, 53 insertions(+), 3 deletions(-)
> >
> > diff --git a/conformance.tex b/conformance.tex index 863f9c5..07b0b7b
> > 100644
> > --- a/conformance.tex
> > +++ b/conformance.tex
> > @@ -77,6 +77,7 @@ \section{Conformance Targets}\label{sec:Conformance
> > / Conformance Targets}  \item \ref{drivernormative:Basic Facilities of
> > a Virtio Device / Device Configuration Space}  \item
> > \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
> > \item \ref{drivernormative:Basic Facilities of a Virtio Device /
> > Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > +\item \ref{devicenormative:Basic Facilities of a Virtio Device /
> > +Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> 
> s/devicenormative/drivernormative/
> 
> 
> >  \item \ref{drivernormative:Basic Facilities of a Virtio Device /
> > Message Framing}  \item \ref{drivernormative:Basic Facilities of a
> > Virtio Device / Virtqueues / The Virtqueue Descriptor Table}  \item
> > \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues
> > / The Virtqueue Descriptor Table / Indirect Descriptors} @@ -164,6
> > +165,7 @@ \section{Conformance Targets}\label{sec:Conformance /
> > Conformance Targets}  \item \ref{devicenormative:Basic Facilities of a
> > Virtio Device / Device Reset}  \item \ref{devicenormative:Basic
> > Facilities of a Virtio Device / Device Configuration Space}  \item
> > \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues
> > / Virtqueue Reset / Virtqueue Reset}
> > +\item \ref{devicenormative:Basic Facilities of a Virtio Device /
> > +Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> >  \item \ref{devicenormative:Basic Facilities of a Virtio Device /
> > Message Framing}  \item \ref{devicenormative:Basic Facilities of a
> > Virtio Device / Virtqueues / The Virtqueue Descriptor Table}  \item
> > \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues
> > / The Virtqueue Descriptor Table / Indirect Descriptors} diff --git
> > a/content.tex b/content.tex index 0a62dce..91aee25 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -102,7 +102,7 @@ \section{Feature Bits}\label{sec:Basic Facilities
> > of a Virtio Device / Feature B
> >  \item[24 to 41] Feature bits reserved for extensions to the queue and
> >    feature negotiation mechanisms
> >
> > -\item[42 to 49, and 128 and above] Feature bits reserved for future
> extensions.
> > +\item[43 to 49, and 128 and above] Feature bits reserved for future
> extensions.
> >  \end{description}
> >
> >  \begin{note}
> > @@ -440,6 +440,42 @@ \subsubsection{Virtqueue
> > Re-enable}\label{sec:Basic Facilities of a Virtio Devic  as during
> > initial virtqueue discovery, but optionally with different  parameters.
> >
> > +\subsection{Dynamic Virtqueues}\label{sec:Basic Facilities of a
> > +Virtio Device / Virtqueues / Dynamic Virtqueues}
> > +
> > +When VIRTIO_F_RING_DYNAMIC is not negotiated, the virtqueues are
> > +enabled when the device is initialized, i.e. after the driver has
> > +ensured that the device has set the FEATURES_OK status bit and before
> > +the driver setting the DRIVER_OK status bit.
> > +
> > +When VIRTIO_F_RING_DYNAMIC is negotiated, the driver can avoid
> > +enabling the queue before setting the DRIVER_OK status bit; the
> > +driver can enable the queue after the driver has set the DRIVER_OK
> > +status bit. The queue enable mechanism is transport specific.
> > +
> > +\devicenormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a
> > +Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> > +
> > +When VIRTIO_F_RING_DYNAMIC is negotiated, the device MUST allow
> > +enabling the virtqueue which was not enabled during the device
> > +initialization phase i.e. after the device has set the FEATURES_OK
> > +status bit and before the driver has set the DRIVER_OK status bit.
> > +
> > +\drivernormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a
> > +Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> > +
> > +When VIRTIO_F_RING_DYNAMIC is negotiated, \begin{itemize} \item the
> > +driver MAY not enable all the virtqueues during the device
> > +initialization phase;
> 
> Remove the \n after \item
> 
> > +
> > +\item the driver MAY enable some or all the virtqueues after the driver has
> set the
> > +	DRIVER_OK status bit.
> > +
> > +\item the driver MAY enable all the virtqueues during the device
> > +initialization phase; \end{itemize}
> > +
> > +The driver interested in enabling and disabling the virtqueue
> > +dynamically after setting the DRIVER_OK status bit MUST negotiate
> > +VIRTIO_F_RING_DYNAMIC and VIRTIO_F_RING_RESET features.
> > +
> >  \input{split-ring.tex}
> >
> >  \input{packed-ring.tex}
> > @@ -537,7 +573,9 @@ \section{Device Initialization}\label{sec:General
> > Initialization And Device Oper
> >
> >  \item\label{itm:General Initialization And Device Operation / Device
> Initialization / Device-specific Setup} Perform device-specific setup, including
> discovery of virtqueues for the
> >     device, optional per-bus setup, reading and possibly writing the
> > -   device's virtio configuration space, and population of virtqueues.
> > +   device's virtio configuration space.
> > +
> > +\item\label{itm:General Initialization And Device Operation / Device
> Initialization / Virtqueue Setup} Configure and enable the virtqueues.
> >
> >  \item\label{itm:General Initialization And Device Operation / Device
> Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit.  At this point the
> device is
> >     ``live''.
> > @@ -551,6 +589,10 @@ \section{Device Initialization}\label{sec:General
> > Initialization And Device Oper  The driver MUST NOT send any buffer
> > available notifications to  the device before setting DRIVER_OK.
> >
> > +The driver MAY skip step
> > +\ref{itm:General Initialization And Device Operation / Device
> > +Initialization / Virtqueue Setup} when driver has negotiated
> VIRTIO_F_RING_DYNAMIC feature.
> > +
> >  \subsection{Legacy Interface: Device
> > Initialization}\label{sec:General Initialization And Device Operation
> > / Device Initialization / Legacy Interface: Device Initialization}
> > Legacy devices did not support the FEATURES_OK status bit, and thus
> > did  not have a graceful way for the device to indicate unsupported
> > feature @@ -569,7 +611,7 @@ \subsection{Legacy Interface: Device
> > Initialization}\label{sec:General Initializ  Initialization / Re-read
> > FEATURES-OK} were omitted, and steps  \ref{itm:General Initialization
> > And Device Operation /  Device Initialization / Read feature bits},
> > -\ref{itm:General Initialization And Device Operation / Device
> > Initialization / Device-specific Setup} and \ref{itm:General
> > Initialization And Device Operation / Device Initialization / Set
> > DRIVER-OK}
> > +\ref{itm:General Initialization And Device Operation / Device
> > +Initialization / Device-specific Setup}, \ref{itm:General
> > +Initialization And Device Operation / Device Initialization /
> > +Virtqueue Setup} and \ref{itm:General Initialization And Device
> > +Operation / Device Initialization / Set DRIVER-OK}
> >  were conflated.
> >
> >  Therefore, when using the legacy interface:
> > @@ -872,8 +914,14 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
> Feature Bits}
> >  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
> >  	handling features reserved for future use.
> >
> > +  \item[VIRTIO_F_RING_DYNAMIC(42)] This feature indicates  that the
> > + driver can enable a virtqueue individually after the driver has  set
> > + the status bit of DRIVER_OK.
> > +  See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Dynamic
> Virtqueues}.
> > +
> >  \end{description}
> >
> > +
> 
> No add new empty line, if we do not need it.
> 

Ack. Fixing it.

[virtio-comment] Re: [PATCH 2/2] content: Support enabling virtqueue after DRIVER_OK stage

[Michael S. Tsirkin]

On Sat, Sep 09, 2023 at 05:32:18PM +0300, Parav Pandit wrote:
> Currently, a virtqueue must be enabled before driver has set the
> DRIVER_OK status bit.
> 
> spec citation to section "Driver Requirements: Device Initialization"
> 
> "Perform device-specific setup, including discovery of virtqueues
> for the device, optional per-bus setup, reading and possibly writing
> the device’s virtio configuration space, and population of virtqueues."
> 
> This implies that a virtqueue must be enabled before reaching the
> DRIVER_OK stage. There was no explicit mention about ability to
> enable the virtqueue after DRIVER_OK stage.
> 
> In following usecases, creating a virtqueue after DRIVER_OK phase is
> desired.
> 
> 1. Dynamically create aq when administrative commands to be used.
> 2. Dynamically create the net device tx/rxq when device is
>    opened when deploying for a container.
>    In a container, number of virtqueues to be used may be <= max queues.
> 3. Dynamically create flow filter queues of netdevice when
>    ARFS or ethtool filters are enabled as listed in [1].
> 4. Dynamically create rtc functionality related read virtqueue only
>    when net device when timestamping to be used.
> 5. When XDP program is set, one can create additional XDP specific
>    queues without affecting existing queues.
> 
> Hence, This patch introduces an existing queue enable and disable
> (aka reset) facility and a new feature bit to explicitly indicate such
> support by the device.
> 
> With this feature, drivers can skip optional queues creation during
> driver init time. For example, a Linux net device driver
> can create/destroy the transmit and receive queues when net device's
> ndo_open() and ndo_stop() callbacks are invoked respectively.
> 
> As side effect, it also enables driver to not consume MSI-X vectors for
> the PCI device at driver load time and utilize(enable) individual vector
> when needed. This further helps on some cpus at high scale where a
> interrupt line may not be available from the platform.
> 
> [1] https://lists.oasis-open.org/archives/virtio-comment/202308/msg00263.html


I'm not arguing against this functionality - Eugenio wanted it too -
but the use-cases of MSI looks suspect.

If you want to save on vectors isn't the thing to do actually
to allow changing the vector in the PCI mapping? Or not even that -
change vectors in the MSIX table.


> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
>  conformance.tex |  2 ++
>  content.tex     | 54 ++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 53 insertions(+), 3 deletions(-)
> 
> diff --git a/conformance.tex b/conformance.tex
> index 863f9c5..07b0b7b 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -77,6 +77,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> @@ -164,6 +165,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
> diff --git a/content.tex b/content.tex
> index 0a62dce..91aee25 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -102,7 +102,7 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
>  \item[24 to 41] Feature bits reserved for extensions to the queue and
>    feature negotiation mechanisms
>  
> -\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
> +\item[43 to 49, and 128 and above] Feature bits reserved for future extensions.
>  \end{description}
>  
>  \begin{note}
> @@ -440,6 +440,42 @@ \subsubsection{Virtqueue Re-enable}\label{sec:Basic Facilities of a Virtio Devic
>  as during initial virtqueue discovery, but optionally with different
>  parameters.
>  
> +\subsection{Dynamic Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues}
> +
> +When VIRTIO_F_RING_DYNAMIC is not negotiated, the virtqueues are
> +enabled when the device is initialized, i.e. after the driver has ensured that
> +the device has set the FEATURES_OK status bit and before the driver setting
> +the DRIVER_OK status bit.

Avoid passive voice pls. I think you mean driver enables not queues are
enabled.


> +When VIRTIO_F_RING_DYNAMIC is negotiated, the driver can avoid enabling the queue
> +before setting the DRIVER_OK status bit; the driver can enable the queue after
> +the driver has set the DRIVER_OK status bit. The queue enable mechanism is
> +transport specific.

I think you mean "specific queues" not "the queue".



Something else to consider is how this interacts with the ring reset
feature. I think you must allow ring reset before driver ok too, right?


> +
> +\devicenormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> +
> +When VIRTIO_F_RING_DYNAMIC is negotiated, the device MUST allow enabling the
> +virtqueue which was not enabled during the device initialization phase
> +i.e. after the device has set the FEATURES_OK status bit and before the
> +driver has set the DRIVER_OK status bit.
> +
> +\drivernormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> +
> +When VIRTIO_F_RING_DYNAMIC is negotiated,
> +\begin{itemize}
> +\item
> +the driver MAY not enable all the virtqueues during the device initialization phase;

MAY not sounds like it's not allowed. just drop this?


> +
> +\item the driver MAY enable some or all the virtqueues after the driver has set the
> +	DRIVER_OK status bit.
> +
> +\item the driver MAY enable all the virtqueues during the device initialization phase;
> +\end{itemize}
> +
> +The driver interested

let's be more formal pls. 
- if it does not negotiate it SHOULD enable before
- if it does negotiate it MAY enable after

> in enabling and disabling the virtqueue dynamically
> +after setting the DRIVER_OK status bit MUST negotiate VIRTIO_F_RING_DYNAMIC
> +and VIRTIO_F_RING_RESET features.

what does this have to do with reset?


So what is your assumption on existing devices. Do you think no drivers
enable before DRIVER_OK, or do you think some might?`


> +
>  \input{split-ring.tex}
>  
>  \input{packed-ring.tex}
> @@ -537,7 +573,9 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
>  
>  \item\label{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} Perform device-specific setup, including discovery of virtqueues for the
>     device, optional per-bus setup, reading and possibly writing the
> -   device's virtio configuration space, and population of virtqueues.
> +   device's virtio configuration space.
> +
> +\item\label{itm:General Initialization And Device Operation / Device Initialization / Virtqueue Setup} Configure and enable the virtqueues.
>  
>  \item\label{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit.  At this point the device is
>     ``live''.
> @@ -551,6 +589,10 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
>  The driver MUST NOT send any buffer available notifications to
>  the device before setting DRIVER_OK.
>  
> +The driver MAY skip step
> +\ref{itm:General Initialization And Device Operation / Device Initialization / Virtqueue Setup}
> +when driver has negotiated VIRTIO_F_RING_DYNAMIC feature.
> +
>  \subsection{Legacy Interface: Device Initialization}\label{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
>  Legacy devices did not support the FEATURES_OK status bit, and thus did
>  not have a graceful way for the device to indicate unsupported feature
> @@ -569,7 +611,7 @@ \subsection{Legacy Interface: Device Initialization}\label{sec:General Initializ
>  Initialization / Re-read FEATURES-OK} were omitted, and steps
>  \ref{itm:General Initialization And Device Operation /
>  Device Initialization / Read feature bits},
> -\ref{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} and \ref{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK}
> +\ref{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup}, \ref{itm:General Initialization And Device Operation / Device Initialization / Virtqueue Setup} and \ref{itm:General Initialization And Device Operation / Device Initialization / Set DRIVER-OK}
>  were conflated.
>  
>  Therefore, when using the legacy interface:
> @@ -872,8 +914,14 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>  	handling features reserved for future use.
>  
> +  \item[VIRTIO_F_RING_DYNAMIC(42)] This feature indicates
> +  that the driver can enable a virtqueue individually after the driver has

virtqueues

> +  set the status bit of DRIVER_OK.
> +  See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Dynamic Virtqueues}.
> +
>  \end{description}
>  
> +
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>  
>  A driver MUST accept VIRTIO_F_VERSION_1 if it is offered.  A driver
> -- 
> 2.34.1


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

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

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

[virtio-comment] RE: [PATCH 2/2] content: Support enabling virtqueue after DRIVER_OK stage

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, September 13, 2023 2:25 PM
> 
I missed your email. Sorry for the delay.
Please find response below.

> On Sat, Sep 09, 2023 at 05:32:18PM +0300, Parav Pandit wrote:
> > Currently, a virtqueue must be enabled before driver has set the
> > DRIVER_OK status bit.
> >
> > spec citation to section "Driver Requirements: Device Initialization"
> >
> > "Perform device-specific setup, including discovery of virtqueues for
> > the device, optional per-bus setup, reading and possibly writing the
> > device’s virtio configuration space, and population of virtqueues."
> >
> > This implies that a virtqueue must be enabled before reaching the
> > DRIVER_OK stage. There was no explicit mention about ability to enable
> > the virtqueue after DRIVER_OK stage.
> >
> > In following usecases, creating a virtqueue after DRIVER_OK phase is
> > desired.
> >
> > 1. Dynamically create aq when administrative commands to be used.
> > 2. Dynamically create the net device tx/rxq when device is
> >    opened when deploying for a container.
> >    In a container, number of virtqueues to be used may be <= max queues.
> > 3. Dynamically create flow filter queues of netdevice when
> >    ARFS or ethtool filters are enabled as listed in [1].
> > 4. Dynamically create rtc functionality related read virtqueue only
> >    when net device when timestamping to be used.
> > 5. When XDP program is set, one can create additional XDP specific
> >    queues without affecting existing queues.
> >
> > Hence, This patch introduces an existing queue enable and disable (aka
> > reset) facility and a new feature bit to explicitly indicate such
> > support by the device.
> >
> > With this feature, drivers can skip optional queues creation during
> > driver init time. For example, a Linux net device driver can
> > create/destroy the transmit and receive queues when net device's
> > ndo_open() and ndo_stop() callbacks are invoked respectively.
> >
> > As side effect, it also enables driver to not consume MSI-X vectors
> > for the PCI device at driver load time and utilize(enable) individual
> > vector when needed. This further helps on some cpus at high scale
> > where a interrupt line may not be available from the platform.
> >
> > [1]
> > https://lists.oasis-open.org/archives/virtio-comment/202308/msg00263.h
> > tml
> 
> 
> I'm not arguing against this functionality - Eugenio wanted it too - but the use-
> cases of MSI looks suspect.
> 
> If you want to save on vectors isn't the thing to do actually to allow changing
> the vector in the PCI mapping? Or not even that - change vectors in the MSIX
> table.
> 
MSIX vector in the device tells the about supported vectors.
In some systems, a system is running out of vectors even though device has it, and vq is not used either.
So it may be able to improve such cases, but I agree that it is not top priority. It is a byproduct gain.

More below.

> 
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > ---
> >  conformance.tex |  2 ++
> >  content.tex     | 54 ++++++++++++++++++++++++++++++++++++++++++++++--
> -
> >  2 files changed, 53 insertions(+), 3 deletions(-)
> >
> > diff --git a/conformance.tex b/conformance.tex index 863f9c5..07b0b7b
> > 100644
> > --- a/conformance.tex
> > +++ b/conformance.tex
> > @@ -77,6 +77,7 @@ \section{Conformance Targets}\label{sec:Conformance
> > / Conformance Targets}  \item \ref{drivernormative:Basic Facilities of
> > a Virtio Device / Device Configuration Space}  \item
> > \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
> > \item \ref{drivernormative:Basic Facilities of a Virtio Device /
> > Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > +\item \ref{devicenormative:Basic Facilities of a Virtio Device /
> > +Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> >  \item \ref{drivernormative:Basic Facilities of a Virtio Device /
> > Message Framing}  \item \ref{drivernormative:Basic Facilities of a
> > Virtio Device / Virtqueues / The Virtqueue Descriptor Table}  \item
> > \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues
> > / The Virtqueue Descriptor Table / Indirect Descriptors} @@ -164,6
> > +165,7 @@ \section{Conformance Targets}\label{sec:Conformance /
> > Conformance Targets}  \item \ref{devicenormative:Basic Facilities of a
> > Virtio Device / Device Reset}  \item \ref{devicenormative:Basic
> > Facilities of a Virtio Device / Device Configuration Space}  \item
> > \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues
> > / Virtqueue Reset / Virtqueue Reset}
> > +\item \ref{devicenormative:Basic Facilities of a Virtio Device /
> > +Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> >  \item \ref{devicenormative:Basic Facilities of a Virtio Device /
> > Message Framing}  \item \ref{devicenormative:Basic Facilities of a
> > Virtio Device / Virtqueues / The Virtqueue Descriptor Table}  \item
> > \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues
> > / The Virtqueue Descriptor Table / Indirect Descriptors} diff --git
> > a/content.tex b/content.tex index 0a62dce..91aee25 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -102,7 +102,7 @@ \section{Feature Bits}\label{sec:Basic Facilities
> > of a Virtio Device / Feature B
> >  \item[24 to 41] Feature bits reserved for extensions to the queue and
> >    feature negotiation mechanisms
> >
> > -\item[42 to 49, and 128 and above] Feature bits reserved for future
> extensions.
> > +\item[43 to 49, and 128 and above] Feature bits reserved for future
> extensions.
> >  \end{description}
> >
> >  \begin{note}
> > @@ -440,6 +440,42 @@ \subsubsection{Virtqueue
> > Re-enable}\label{sec:Basic Facilities of a Virtio Devic  as during
> > initial virtqueue discovery, but optionally with different  parameters.
> >
> > +\subsection{Dynamic Virtqueues}\label{sec:Basic Facilities of a
> > +Virtio Device / Virtqueues / Dynamic Virtqueues}
> > +
> > +When VIRTIO_F_RING_DYNAMIC is not negotiated, the virtqueues are
> > +enabled when the device is initialized, i.e. after the driver has
> > +ensured that the device has set the FEATURES_OK status bit and before
> > +the driver setting the DRIVER_OK status bit.
> 
> Avoid passive voice pls. I think you mean driver enables not queues are
> enabled.
>
Yes, will fix it as,
the driver enables the virtqueues when device is initialized, ..
 
> 
> > +When VIRTIO_F_RING_DYNAMIC is negotiated, the driver can avoid
> > +enabling the queue before setting the DRIVER_OK status bit; the
> > +driver can enable the queue after the driver has set the DRIVER_OK
> > +status bit. The queue enable mechanism is transport specific.
> 
> I think you mean "specific queues" not "the queue".
> 
Yes, will fix this too.
> 
> 
> Something else to consider is how this interacts with the ring reset feature. I
> think you must allow ring reset before driver ok too, right?
> 
I couldn’t find a use case of why one wants to reset the queue which is not live until driver ok.
The _dynamic feature does not need to interact with ring reset, because _dynamic enables to do things after driver_ok.

> 
> > +
> > +\devicenormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a
> > +Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> > +
> > +When VIRTIO_F_RING_DYNAMIC is negotiated, the device MUST allow
> > +enabling the virtqueue which was not enabled during the device
> > +initialization phase i.e. after the device has set the FEATURES_OK
> > +status bit and before the driver has set the DRIVER_OK status bit.
> > +
> > +\drivernormative{\paragraph}{Enable Virtqueues}{Basic Facilities of a
> > +Virtio Device / Virtqueues / Dynamic Virtqueues / Enable Virtqueues}
> > +
> > +When VIRTIO_F_RING_DYNAMIC is negotiated, \begin{itemize} \item the
> > +driver MAY not enable all the virtqueues during the device
> > +initialization phase;
> 
> MAY not sounds like it's not allowed. just drop this?
> 
Since this is in the driver's normative, it is allowed by the device.

I think it is fine given the below listed item.
May be we swap both the items, that makes it clear?
> 
> > +
> > +\item the driver MAY enable some or all the virtqueues after the driver has
> set the
> > +	DRIVER_OK status bit.
> > +
> > +\item the driver MAY enable all the virtqueues during the device
> > +initialization phase; \end{itemize}
> > +
> > +The driver interested
> 
> let's be more formal pls.
> - if it does not negotiate it SHOULD enable before
If it does not negotiate it MIST enable before.

> - if it does negotiate it MAY enable after
If it negotiates it MAY enable before or after.
Do you see a problem with adding "before?

> 
> > in enabling and disabling the virtqueue dynamically
> > +after setting the DRIVER_OK status bit MUST negotiate
> > +VIRTIO_F_RING_DYNAMIC and VIRTIO_F_RING_RESET features.
> 
> what does this have to do with reset?
> 
This normative is probably not needed. Basically I wanted to capture a programming notion that,
If one wants to do enable/disable after driver ok, both feature bits are needed, only dynamic is not enough.

Should I move it out of normative and capture in the theory section?

> 
> So what is your assumption on existing devices. Do you think no drivers enable
> before DRIVER_OK, or do you think some might?`
> 
I probably didn’t follow the question.
My understanding is:
Existing drivers must enable before driver_ok.
And if they are enabled before driver_ok, only than one can reset them.

Existing devices may allow enable before or after driver_ok and driver has no way to know if the device really does that or not.

> 
> > +
> >  \input{split-ring.tex}
> >
> >  \input{packed-ring.tex}
> > @@ -537,7 +573,9 @@ \section{Device Initialization}\label{sec:General
> > Initialization And Device Oper
> >
> >  \item\label{itm:General Initialization And Device Operation / Device
> Initialization / Device-specific Setup} Perform device-specific setup, including
> discovery of virtqueues for the
> >     device, optional per-bus setup, reading and possibly writing the
> > -   device's virtio configuration space, and population of virtqueues.
> > +   device's virtio configuration space.
> > +
> > +\item\label{itm:General Initialization And Device Operation / Device
> Initialization / Virtqueue Setup} Configure and enable the virtqueues.
> >
> >  \item\label{itm:General Initialization And Device Operation / Device
> Initialization / Set DRIVER-OK} Set the DRIVER_OK status bit.  At this point the
> device is
> >     ``live''.
> > @@ -551,6 +589,10 @@ \section{Device Initialization}\label{sec:General
> > Initialization And Device Oper  The driver MUST NOT send any buffer
> > available notifications to  the device before setting DRIVER_OK.
> >
> > +The driver MAY skip step
> > +\ref{itm:General Initialization And Device Operation / Device
> > +Initialization / Virtqueue Setup} when driver has negotiated
> VIRTIO_F_RING_DYNAMIC feature.
> > +
> >  \subsection{Legacy Interface: Device
> > Initialization}\label{sec:General Initialization And Device Operation
> > / Device Initialization / Legacy Interface: Device Initialization}
> > Legacy devices did not support the FEATURES_OK status bit, and thus
> > did  not have a graceful way for the device to indicate unsupported
> > feature @@ -569,7 +611,7 @@ \subsection{Legacy Interface: Device
> > Initialization}\label{sec:General Initializ  Initialization / Re-read
> > FEATURES-OK} were omitted, and steps  \ref{itm:General Initialization
> > And Device Operation /  Device Initialization / Read feature bits},
> > -\ref{itm:General Initialization And Device Operation / Device
> > Initialization / Device-specific Setup} and \ref{itm:General
> > Initialization And Device Operation / Device Initialization / Set
> > DRIVER-OK}
> > +\ref{itm:General Initialization And Device Operation / Device
> > +Initialization / Device-specific Setup}, \ref{itm:General
> > +Initialization And Device Operation / Device Initialization /
> > +Virtqueue Setup} and \ref{itm:General Initialization And Device
> > +Operation / Device Initialization / Set DRIVER-OK}
> >  were conflated.
> >
> >  Therefore, when using the legacy interface:
> > @@ -872,8 +914,14 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
> Feature Bits}
> >  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
> for
> >  	handling features reserved for future use.
> >
> > +  \item[VIRTIO_F_RING_DYNAMIC(42)] This feature indicates  that the
> > + driver can enable a virtqueue individually after the driver has
> 
> virtqueues
Ack.
> 
> > +  set the status bit of DRIVER_OK.
> > +  See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Dynamic
> Virtqueues}.