From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id 34C7D986524 for ; Thu, 27 Jan 2022 14:47:01 +0000 (UTC) Message-ID: Date: Thu, 27 Jan 2022 16:46:52 +0200 MIME-Version: 1.0 References: <20220124093918.34371-1-mgurtovoy@nvidia.com> <20220124093918.34371-2-mgurtovoy@nvidia.com> <1b77bbb8-a488-a2a8-1374-b8691356ac15@redhat.com> <7baddbd5-71af-d207-3e61-a842b5844fa0@nvidia.com> From: Max Gurtovoy In-Reply-To: Subject: Re: [virtio-comment] [PATCH v2 1/4] Add virtio Admin Virtqueue Content-Language: en-US Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable To: Jason Wang Cc: virtio-comment@lists.oasis-open.org List-ID: On 1/27/2022 3:03 PM, Jason Wang wrote: > On Thu, Jan 27, 2022 at 6:25 PM Max Gurtovoy wrote= : >> >> On 1/27/2022 5:14 AM, Jason Wang wrote: >>> =E5=9C=A8 2022/1/24 =E4=B8=8B=E5=8D=885:39, Max Gurtovoy =E5=86=99=E9= =81=93: >>>> 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 ve= ry >>>> difficult to extend for device agnostic commands. >>>> >>>> To support this requirement in elegant way, this patch introduces a ne= w >>>> admin virtqueue. Admin virtqueue will use the same command format for >>>> all >>>> types of virtio devices. >>>> >>>> Manipulate features via admin virtqueue is asynchronous, scalable, eas= y >>>> to extend and doesn't require additional and expensive on-die resource= s >>>> 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 >>>> Signed-off-by: Max Gurtovoy >>>> --- >>>> 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. When you'll add SF to virtio spec we'll create new commands for it with=20 SF_id. Again, this was discussed with Parav in V1 and was close. Please don't=20 re-open. > >> 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=20 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=20 1 + 2 without a class. > >>> >>>> + 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=20 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=20 feature 44,45, 46 and for transport X, Y, Z. > > 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=3Dhttps%3A%2F%2Flis= ts.oasis-open.org%2Farchives%2Fvirtio-comment%2F&data=3D04%7C01%7Cmgurt= ovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db= 39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM= C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata= =3DhkU2cd8o0F%2FTH%2F%2BHA8GQzVKZHPIDpuHBLMgVbjQSvN4%3D&reserved=3D0 >>> Feedback License: >>> https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fwww= .oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&data=3D04%7C01%7Cmgu= rtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7= db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjo= iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdat= a=3DjqtoxmxY5oMIJ8X4R%2FCADdAwYHFNP%2Fix%2Frue8xqf8%2F0%3D&reserved=3D0 >>> List Guidelines: >>> https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fwww= .oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&data=3D04%7C01%7C= mgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c= 1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJW= IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&s= data=3Da3bGp1iZqbK8dy2zMV3Umypft1kIL%2Bugjc8sZRZaX1M%3D&reserved=3D0 >>> Committee: >>> https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fwww= .oasis-open.org%2Fcommittees%2Fvirtio%2F&data=3D04%7C01%7Cmgurtovoy%40n= vidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9cc= c17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM= DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=3DH5cMIM= FYWJT1AVO9%2FLrJ8KDidyuFhuE%2Bb7xFTlKNDLI%3D&reserved=3D0 >>> Join OASIS: >>> https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fwww= .oasis-open.org%2Fjoin%2F&data=3D04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8e= fa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C= 637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu= MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=3DRY2TmLGYxJLQkX6GQVUxX= OL7GeoWR6EqL8kStesrmD0%3D&reserved=3D0 >>> > > 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=3Dhttps= %3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F&data=3D04%= 7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15= 727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb= 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300= 0&sdata=3DhkU2cd8o0F%2FTH%2F%2BHA8GQzVKZHPIDpuHBLMgVbjQSvN4%3D&rese= rved=3D0 > > Feedback License: https://nam11.safelinks.protection.outlook.com/?url=3Dh= ttps%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&data= =3D04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43= 083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWF= pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D= %7C3000&sdata=3DjqtoxmxY5oMIJ8X4R%2FCADdAwYHFNP%2Fix%2Frue8xqf8%2F0%3D&= amp;reserved=3D0 > > List Guidelines: https://nam11.safelinks.protection.outlook.com/?url=3Dht= tps%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&da= ta=3D04%7C01%7Cmgurtovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C= 43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CT= WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%= 3D%7C3000&sdata=3Da3bGp1iZqbK8dy2zMV3Umypft1kIL%2Bugjc8sZRZaX1M%3D&= reserved=3D0 > > Committee: https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3A= %2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&data=3D04%7C01%7Cmgur= tovoy%40nvidia.com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7d= b39efd9ccc17a%7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi= MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata= =3DH5cMIMFYWJT1AVO9%2FLrJ8KDidyuFhuE%2Bb7xFTlKNDLI%3D&reserved=3D0 > > Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=3Dhttps%3= A%2F%2Fwww.oasis-open.org%2Fjoin%2F&data=3D04%7C01%7Cmgurtovoy%40nvidia= .com%7Cc8efa048a41e4be4d35508d9e195694e%7C43083d15727340c1b7db39efd9ccc17a%= 7C0%7C0%7C637788855745609706%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC= JQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=3DRY2TmLGYxJL= QkX6GQVUxXOL7GeoWR6EqL8kStesrmD0%3D&reserved=3D0 > 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-lis= ts Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/