Re: [virtio] Re: [PATCH v10 04/10] admin: introduce virtio admin virtqueues

[Jiri Pirko <jiri@nvidia.com>]

Mon, Mar 06, 2023 at 08:58:12AM CET, jasowang@redhat.com wrote:
>
>å 2023/3/3 08:05, Michael S. Tsirkin åé:
>> On Thu, Mar 02, 2023 at 03:40:07PM -0500, Stefan Hajnoczi wrote:
>> > On Thu, Mar 02, 2023 at 08:05:06AM -0500, Michael S. Tsirkin wrote:
>> > > The admin virtqueues will be the first interface to issue admin commands.
>> > > 
>> > > Currently virtio specification defines control virtqueue to manipulate
>> > > features and configuration of the device it operates on. However,
>> > > control virtqueue commands are device type specific, which makes it very
>> > > difficult to extend for device agnostic commands.
>> > > 
>> > > To support this requirement in a more generic way, this patch introduces
>> > > a new admin virtqueue interface.
>> > Is this referring to the existing virtio-net, virtio-scsi, etc control
>> > virtqueues?
>> > 
>> > I see the admin virtqueue as the virtqueue equivalent to transport
>> > feature bits. The admin queue does nothing device type-specific (net,
>> > scsi, etc) and instead focusses on transport and core virtio tasks.
>> > 
>> > Keeping the device-specific virtqueue separate from the admin virtqueue
>> > is simpler and has fewer potential problems. I don't think creating
>> > common infrastructure for device-specific control virtqueues across
>> > device types worthwhile or within the scope of this patch series.
>> yes this commit log is outdated. referred to original
>> proposal by Max which also planned to replace cvq.
>
>
>Any advantages of allowing PF to change VF's filters? If you're referring

Which filters are you referring to?


>provisioning, the semantic should be different:
>
>1) using admin virtqueue to provision attributes of mac, #vpqs
>2) once the provisioning is done, we should fail another provisioning request

Wait. Why are you assuming provisioning of any kind? In which patch this
is covered?


>
>But 1) should be different from what is used for ctrl vq which is designed
>for the use of the driver not the management.
>
>Thanks
>
>
>> will update.
>> 
>> 
>> > > We also support more than one admin virtqueue, for QoS and
>> > > scalability requirements.
>> > > 
>> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > > ---
>> > >   admin.tex   | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> > >   content.tex |  7 +++--
>> > >   2 files changed, 79 insertions(+), 2 deletions(-)
>> > > 
>> > > diff --git a/admin.tex b/admin.tex
>> > > index 7e28b77..3ffac2e 100644
>> > > --- a/admin.tex
>> > > +++ b/admin.tex
>> > > @@ -155,3 +155,77 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>> > >   \field{command_specific_data} and \field{command_specific_result}
>> > >   depends on these structures and is described separately or is
>> > >   implicit in the structure description.
>> > > +
>> > > +\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
>> > > +
>> > > +An administration virtqueue of an owner device is used to submit
>> > > +group administration commands. An owner device can have more
>> > > +than one administration virtqueue.
>> > > +
>> > > +If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one
>> > > +or more adminstration virtqueues. The number and locations of the
>> > > +administration virtqueues are exposed by the owner device in a transport
>> > > +specific manner.
>> > > +
>> > > +The driver submits commands by queueing them to an arbitrary
>> > > +administration virtqueue, and they are processed by the
>> > > +device in the order in which they are queued. It is the
>> > > +responsibility of the driver to ensure ordering for commands
>> > > +placed on different administration virtqueues, because they will
>> > > +be executed with no order constraints.
>> > Does "they are processed by the device in the order in which they are
>> > queued" mean only 1 admin command can be running at any given time and
>> > therefore they will complete in order? This would allow pipelining
>> > dependent commands but prevent long-running commands because the
>> > virtqueue is blocked while executing a command.
>> we have multiple vqs for that.
>> 
>> > > +
>> > > +Administration virtqueues are used as follows:
>> > > +\begin{itemize}
>> > > +\item The driver submits the command using the \field{struct virtio_admin_cmd}
>> > > +structure using a buffer consisting of two parts: a device-readable one followed by a
>> > > +device-writable one.
>> > > +\item the device-readable part includes fields from \field{opcode}
>> > > +through \field{command_specific_data}.
>> > > +\item the device-writeable buffer includes fields from \field{status}
>> > > +through \field{command_specific_result} inclusive.
>> > > +\end{itemize}
>> > > +
>> > > +For each command, this specification describes a distinct
>> > > +format structure used for \field{command_specific_data} and
>> > > +\field{command_specific_result}, the length of these fields
>> > > +depends on the command.
>> > > +
>> > > +However, to ensure forward compatibility
>> > > +\begin{itemize}
>> > > +\item drivers are allowed to submit buffers that are longer
>> > > +than what the device expects
>> > > +(that is, longer than the length of
>> > > +\field{opcode} through \field{command_specific_data}).
>> > > +This allows the driver to maintain
>> > > +a single format structure even if some structure fields are
>> > > +unused by the device.
>> > > +\item drivers are allowed to submit buffers that are shorter
>> > > +than what the device expects
>> > > +(that is, shorter than the length of \field{status} through
>> > > +\field{command_specific_result}). This allows the device to maintain
>> > > +a single format structure even if some structure fields are
>> > > +unused by the driver.
>> > > +\end{itemize}
>> > > +
>> > > +The device compares the length of each part (device-readable and
>> > > +device-writeable) of the buffer as submitted by driver to what it
>> > > +expects and then silently truncates the structures to either the
>> > > +length submitted by the driver, or the length described in this
>> > > +specification, whichever is shorter.  The device silently ignores
>> > > +any data falling outside the shorter of the two lengths. Any
>> > > +missing fields are interpreted as set to zero.
>> > > +
>> > > +Similarly, the driver compares the used buffer length
>> > > +of the buffer to what it expects and then silently
>> > > +truncates the structure to the used buffer length.
>> > > +The driver silently ignores any data falling outside
>> > > +the used buffer length reported by the device.  Any missing
>> > > +fields are interpreted as set to zero.
>> > > +
>> > > +This simplifies driver and device implementations since the
>> > > +driver/device can simply maintain a single large structure (such
>> > > +as a C structure) for a command and its result. As new versions
>> > > +of the specification are designed, new fields can be added to the
>> > > +tail of a structure, with the driver/device using the full
>> > > +structure without concern for versioning.
>> > > +>>>>>>> 0edc690... admin: introduce virtio admin virtqueues
>> > A merge conflict line crept into the patch?
>> yes. I fixed it I thought but somehow it's still there :(
>> 
>> > > diff --git a/content.tex b/content.tex
>> > > index ffe45c4..c8647c9 100644
>> > > --- a/content.tex
>> > > +++ b/content.tex
>> > > @@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
>> > >   \begin{description}
>> > >   \item[0 to 23, and 50 to 127] Feature bits for the specific device type
>> > > -\item[24 to 40] Feature bits reserved for extensions to the queue and
>> > > +\item[24 to 41] Feature bits reserved for extensions to the queue and
>> > >     feature negotiation mechanisms
>> > > -\item[41 to 49, and 128 and above] Feature bits reserved for future extensions.
>> > > +\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
>> > >   \end{description}
>> > >   \begin{note}
>> > > @@ -7682,6 +7682,9 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>> > >     that the driver can reset a queue individually.
>> > >     See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
>> > > +  \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
>> > > +  administration virtqueues.
>> > > +
>> > >   \end{description}
>> > >   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>> > > -- 
>> > > MST
>> > > 
>> 
>
>
>---------------------------------------------------------------------
>To unsubscribe from this mail list, you must leave the OASIS TC that
>generates this mail.  Follow this link to all your TCs in OASIS at:
>https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php
>