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

[Parav Pandit <parav@nvidia.com>]

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