[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH v2 1/4] Add virtio Admin Virtqueue
On Thu, Jan 27, 2022 at 10:47 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
>
>
> On 1/27/2022 3:03 PM, Jason Wang wrote:
> > On Thu, Jan 27, 2022 at 6:25 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> >>
> >> On 1/27/2022 5:14 AM, Jason Wang wrote:
> >>> å 2022/1/24 äå5:39, Max Gurtovoy åé:
> >>>> In one of the many use cases a user wants to manipulate features and
> >>>> configuration of the virtio devices regardless of the device type
> >>>> (net/block/console). Some of this configuration is generic enough. i.e
> >>>> Number of MSI-X vectors of a virtio PCI VF device. There is a need to do
> >>>> such features query and manipulation by its parent PCI PF.
> >>>>
> >>>> 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 elegant way, this patch introduces a new
> >>>> admin virtqueue. Admin virtqueue will use the same command format for
> >>>> all
> >>>> types of virtio devices.
> >>>>
> >>>> Manipulate features via admin virtqueue is asynchronous, scalable, easy
> >>>> to extend and doesn't require additional and expensive on-die resources
> >>>> to be allocated for every new feature that will be added in the future.
> >>>>
> >>>> Subsequent patches make use of this admin virtqueue.
> >>>>
> >>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
> >>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> >>>> ---
> >>>> admin-virtq.tex | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>> content.tex | 9 +++--
> >>>> 2 files changed, 96 insertions(+), 2 deletions(-)
> >>>> create mode 100644 admin-virtq.tex
> >>>>
> >>>> diff --git a/admin-virtq.tex b/admin-virtq.tex
> >>>> new file mode 100644
> >>>> index 0000000..1a41c22
> >>>> --- /dev/null
> >>>> +++ b/admin-virtq.tex
> >>>> @@ -0,0 +1,89 @@
> >>>> +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio
> >>>> Device / Admin Virtqueues}
> >>>> +
> >>>> +Admin virtqueue is used to send administrative commands to manipulate
> >>>> +various features of the device and/or to manipulate various features,
> >>>> +if possible, of another device within the same group (e.g. PCI VFs of
> >>>> +a parent PCI PF device are grouped together. These devices can be
> >>>> +optionally managed by its parent PCI PF using its admin virtqueue.).
> >>>> +
> >>>> +Use of Admin virtqueue is negotiated by the VIRTIO_F_ADMIN_VQ
> >>>> +feature bit.
> >>>> +
> >>>> +Admin virtqueue index may vary among different device types.
> >>>> +
> >>>> +Regardless of device offering VIRTIO_F_IN_ORDER, admin queue command
> >>>> buffers
> >>>> +are used by the device in out of order manner.
> >>>> +
> >>>> +The Admin command set defines the commands that may be issued only
> >>>> to the admin
> >>>> +virtqueue. Each virtio device that advertises the VIRTIO_F_ADMIN_VQ
> >>>> feature, MUST
> >>>> +support all the mandatory admin commands. A device MAY support also
> >>>> one or more
> >>>> +optional admin commands. All commands are of the following form:
> >>>
> >>> Do we need a way for advertising the supported optional commands (or
> >>> features bits)? Otherwise dealing with the compatibility will result
> >>> of per command probing.
> >>>
> >>>
> >>>> +
> >>>> +\begin{lstlisting}
> >>>> +struct virtio_admin_cmd {
> >>>> + /* Device-readable part */
> >>>> + u16 command;
> >>>
> >>> Two questions:
> >>>
> >>> 1) It looks to me it's better to have a explicit device_id here other
> >>> than embed it in each command
> >> this was already discussed.
> >>
> >> We agreed that some commands are not referring to different device so
> >> no, we don't need to put it in generic structure.
> >>
> >> I'm not against putting such id but we don't have an exact idea how will
> >> SF device_id will look like.
> >>
> >> Maybe it will be some 128 bit uuid ? maybe u64 ?
> > How do you know u16 is sufficient for command?
>
> it's sufficient for VFs and this is the subject of the proposed commands.
So VIRTIO_ADMIN_CAPS_IDENTIFY maps to 3 commands:
VIRTIO_ADMIN_PCI_SRIOV_ATTRS,
VIRTIO_ADMIN_PCI_VF_INTERRUPT_CONFIG_SET,
VIRTIO_ADMIN_PCI_VF_INTERRUPT_CONFIG_GET
Unless you do 1:1 mapping between cap bit and command, you will be
short of commands if you're using u16.
>
> When you'll add SF to virtio spec we'll create new commands for it with
> SF_id.
>
> Again, this was discussed with Parav in V1 and was close. Please don't
> re-open.
I don't think so, you can reserve a special device id for the PF itself.
>
> >
> >> how can we guess ?
> > You know it's you but not me that tries to propose this solution, right?
> I'm proposing a solution but you ask questions that we answer but
> somehow they are asked again.
> >
> >>
> >>> 2) virtio-net cvq use class/command which seems more elegant, e.g all
> >>> commands belongs to MSI could be grouped into the same class
> >> why can't we do it in u16 command opcode ?
> > Where did you see I say you can't use u16?
>
> you suggest to replace it to class + command.
>
> I don't understand why.
>
> If it's not mandatory, please ack.
>
>
> >
> >> are you open for changes or
> >> should we copy/paste from net cvq ?
> > Net cvq comes first, it's natural to ask why you do things differently.
>
> I answered why.
>
> Admin command set has a different purpose.
>
> >
> >> Let's ask differently. Why we need class/command structure ? why is it
> >> more elegant ?
> > Don't you see my reply above?
> >
> > "
> > commands belongs to MSI could be grouped into the same class
> > "
>
> It's doesn't explain why class A + commands 1, 2 is better than opcodes
> 1 + 2 without a class.
Well, if you read how cvq work it's not hard to get the conclusion,
It's easier to be mapped to a feature bit. You just map a class and that's all.
Otherwise, you need to feature X: command A,B,C,D ....
>
>
> >
> >>>
> >>>> + u8 command_specific_data[];
> >>>> +
> >>>> + /* Device-writable part */
> >>>> + u8 status;
> >>>> + u8 command_specific_error;
> >>>
> >>> I wonder the reason why not using a single field for the above two
> >>> fields. (Or any value for separating command specific error out, we
> >>> don't do that for virtio-net cvq).
> >> each command have its own specific errors.
> >>
> >> virtio net cvq is not generic - it's a net device cvqueue.
> > We're talking about the command format, not its semantics, right?
> >
> > It's command format is pretty general.
>
> in the Admin command set we would like to have better format that will
> allow users/drivers to better understand device error.
>
> It is very common in many HW devices and specs out there.
>
> I hope it doesn't critical for you as a concept.
>
> >
> >> To make it generic we need to separate.
> >>
> >>>
> >>>> + u8 command_specific_result[];
> >>>> +};
> >>>> +\end{lstlisting}
> >>>> +
> >>>> +The following table describes the generic Admin status codes:
> >>>> +
> >>>> +\begin{tabular}{|l|l|l|}
> >>>> +\hline
> >>>> +Opcode & Status & Description \\
> >>>> +\hline \hline
> >>>> +00h & VIRTIO_ADMIN_STATUS_OK & successful completion \\
> >>>> +\hline
> >>>> +01h & VIRTIO_ADMIN_STATUS_CS_ERR & command specific error \\
> >>>> +\hline
> >>>> +02h & VIRTIO_ADMIN_STATUS_COMMAND_UNSUPPORTED & unsupported or
> >>>> invalid opcode \\
> >>>> +\hline
> >>>> +\end{tabular}
> >>>> +
> >>>> +The \field{command} and \field{command_specific_data} are
> >>>> +set by the driver, and the device sets the \field{status}, the
> >>>> +\field{command_specific_error} and the \field{command_specific_result},
> >>>> +if needed.
> >>>> +
> >>>> +The \field{command_specific_error} should be inspected by the driver
> >>>> only if \field{status} is set to
> >>>> +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the content
> >>>> of \field{command_specific_error}
> >>>> +holds the command specific error. If \field{status} is not set to
> >>>> VIRTIO_ADMIN_STATUS_CS_ERR, the
> >>>> +\field{command_specific_error} value is undefined.
> >>>> +
> >>>> +The following table describes the Admin command set:
> >>>> +
> >>>> +\begin{tabular}{|l|l|l|}
> >>>> +\hline
> >>>> +Opcode & Command & M/O \\
> >>>> +\hline \hline
> >>>> +0000h & VIRTIO_ADMIN_CAPS_IDENTIFY & M \\
> >>>> +\hline
> >>>> +0001h - 7FFFh & Generic admin cmds & - \\
> >>>> +\hline
> >>>> +8000h - FFFFh & Reserved & - \\
> >>>> +\hline
> >>>> +\end{tabular}
> >>>
> >>> See above, I'd suggest to use class/command as virtio-net cvq.
> >> see my comment above.
> I answered above.
> >>>
> >>>> +
> >>>> +\subsection{VIRTIO ADMIN CAPS IDENTIFY command}\label{sec:Basic
> >>>> Facilities of a Virtio Device / Admin Virtqueues / VIRTIO ADMIN CAPS
> >>>> IDENTIFY command}
> >>>> +
> >>>> +The VIRTIO_ADMIN_CAPS_IDENTIFY command has no command specific data
> >>>> set by the driver.
> >>>> +This command upon success, returns a data buffer that describes
> >>>> information about the supported
> >>>> +admin capabilities by the device. This information is of form:
> >>>> +\begin{lstlisting}
> >>>> +struct virtio_admin_caps_identify_result {
> >>>> + /*
> >>>> + * caps[0] - Bit 0x0 - Bit 0x7 are reserved
> >>>> + * caps[1] - Bit 0x0 - Bit 0x7 are reserved
> >>>> + * caps[2] - Bit 0x0 - Bit 0x7 are reserved
> >>>> + * ....
> >>>> + * caps[8191] - Bit 0x0 - Bit 0x7 are reserved
> >>>> + */
> >>>> + u8 caps[8192];
> >>>> +};
> >>>
> >>> Ok, so I see the identify command. But I wonder if we can do that via
> >>> the feature bits?
> >> It doesn't scale. I tried it. It doesn't work.
> > What prevents you from improving the scalability instead of trying to
> > revinting a duplicated mechanism?
>
> It was already discussed.
>
> Spec doesn't scale, too many constrains between command-A,B,C and
> feature 44,45, 46 and for transport X, Y, Z.
What I meant is, if you feel the current feature negotiation doesn't
scale, you can improve it instead of reinventing it partially (e.g no
negotiation) .
Thanks
>
> >
> > Thanks
> >
> >>
> >> Thanks.
> >>
> >>> Thanks
> >>>
> >>>
> >>>> +\end{lstlisting}
> >>>> diff --git a/content.tex b/content.tex
> >>>> index 32de668..c524fab 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] 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 and above] Feature bits reserved for future extensions.
> >>>> +\item[42 and above] Feature bits reserved for future extensions.
> >>>> \end{description}
> >>>> \begin{note}
> >>>> @@ -449,6 +449,8 @@ \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]}.
> >>>> +\input{admin-virtq.tex}
> >>>> +
> >>>> \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
> >>>> @@ -6847,6 +6849,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 supports administration virtqueue negotiation.
> >>>> +
> >>>> \end{description}
> >>>> \drivernormative{\section}{Reserved Feature Bits}{Reserved
> >>>> Feature Bits}
> >>>
> >>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=hkU2cd8o0F%2FTH%2F%2BHA8GQzVKZHPIDpuHBLMgVbjQSvN4%3D&reserved=0
> >>> Feedback License:
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=jqtoxmxY5oMIJ8X4R%2FCADdAwYHFNP%2Fix%2Frue8xqf8%2F0%3D&reserved=0
> >>> List Guidelines:
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=a3bGp1iZqbK8dy2zMV3Umypft1kIL%2Bugjc8sZRZaX1M%3D&reserved=0
> >>> Committee:
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=H5cMIMFYWJT1AVO9%2FLrJ8KDidyuFhuE%2Bb7xFTlKNDLI%3D&reserved=0
> >>> Join OASIS:
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=RY2TmLGYxJLQkX6GQVUxXOL7GeoWR6EqL8kStesrmD0%3D&reserved=0
> >>>
> >
> > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=hkU2cd8o0F%2FTH%2F%2BHA8GQzVKZHPIDpuHBLMgVbjQSvN4%3D&reserved=0
> >
> > Feedback License: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=jqtoxmxY5oMIJ8X4R%2FCADdAwYHFNP%2Fix%2Frue8xqf8%2F0%3D&reserved=0
> >
> > List Guidelines: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=a3bGp1iZqbK8dy2zMV3Umypft1kIL%2Bugjc8sZRZaX1M%3D&reserved=0
> >
> > Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=H5cMIMFYWJT1AVO9%2FLrJ8KDidyuFhuE%2Bb7xFTlKNDLI%3D&reserved=0
> >
> > Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=RY2TmLGYxJLQkX6GQVUxXOL7GeoWR6EqL8kStesrmD0%3D&reserved=0
> >
>
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]