[virtio-comment] [PATCH V2] virtio-net: introduce admin control virtqueue

[virtio-comment] [PATCH V2] virtio-net: introduce admin control virtqueue

[Jason Wang]

When implementing virtual devices like SR-IOV or sub-function. We're
suffering from several issues:

- There's no management interface for management device to
  configure features, attributes for a virtual device
- Per virtual device control virtqueue could be very expensive as the
  number of virtual devices could be very large
- Virtualize per virtual device's control virtqueue could be very
  challenge as we need the support of DMA isolation at queue level

So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
allows the device to implement a single admin control virtqueue to
manage the features and attributes for a specific virtual device.

The idea is simple, a new virtual device id is introduced on top of
the existing virtio_net_ctrl structure. This id is transport or device
specific to uniquely identify a management or virtual device.

With this, we get a way of using management device (PF) to configure
per virtual device features and attributes. And since the admin
control virtqueue belongs to management device (PF), the DMA is
naturally isolated at device level instead of the queue level for per
virtual device control vq.

When the admin cvq is offered by management device and normal cvq is
offered by virtual device. A new command class is introduced decide
whether or not to accept commands from normal cvq for a virtual
device.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes from V1:
- use 'virtual device' instead of 'function'
- introuce trust command
- clairfy that the admin cvq could be used to configure management
  device itself
---
 content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 56 insertions(+), 3 deletions(-)

diff --git a/content.tex b/content.tex
index 620c0e2..989b4f6 100644
--- a/content.tex
+++ b/content.tex
@@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
 \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
     channel.
 
+\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
+    available.
+
 \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
     value and a type of calculated hash.
 
@@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
 
 The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
-negotiated) to send commands to manipulate various features of
-the device which would not easily map into the configuration
-space.
+negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
+commands to manipulate various features of the device which would not
+easily map into the configuration space.
 
 All commands are of the following form:
+is not negotiated:
 
 \begin{lstlisting}
 struct virtio_net_ctrl {
@@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 do except issue a diagnostic if \field{ack} is not
 VIRTIO_NET_OK.
 
+When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
+negotiated, the driver can use the admin control virtqueue of the
+management device to manipulate features of individual virtual devices
+where the control virtqueue is not easily implemented. The definition
+of management device and virtual device is transport or device
+specific. E.g in the case of PCI SR-IOV, the management device is
+implemented via the physical function (PF), then the virtual device is
+the virtual function (VF) in this case.
+
+All commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_net_admin_ctrl {
+        u32 virtual_device_id;
+        struct virtio_net_ctrl ctrl;
+};
+\end{lstlisting}
+
+The \field{virtual_device_id} is an unique transport or device specific
+identifier for a virtual device or management device. E.g for the case
+of PCI SR-IOV, it's the PCI function id. Management device MUST
+reserve 0 for \field{virtual_device_id} to identify itself.
+
 \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
 \label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
 
@@ -4308,6 +4335,32 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 according to the native endian of the guest rather than
 (necessarily when not using the legacy interface) little-endian.
 
+\paragraph{Setting Trust for Virtual Device}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
+
+If the VIRTIO_NET_F_ADMIN_CTRL_VQ is negotiated, the management device
+can accept operations via the control vq from the trusted virtual
+device.
+
+\begin{lstlisting}
+#define VIRTIO_NET_ADMIN_CTRL_TRUST 0
+ #define VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE 0
+\end{lstlisting}
+
+\devicenormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
+
+If the VIRTIO_NET_F_ADMIN_CTRL_VQ has been negotiated, the device MUST
+support VIRTIO_NET_ADMIN_CTRL_TRUST class
+command. VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE turns trust for the
+control virtqueue of a specific virtual device on and off. The command
+specific data is one byte containing 0(off) or 1(on).  When trust is
+off the device MUST fail any operations from the control virtqueue of
+the virtual device. The management device MUST NOT trust any virtual
+devices after reset.
+
+\drivernormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
+
+A driver SHOULD negotiate VIRTIO_NET_F_ADMIN_CTRL_VQ if the device
+offers it.
 
 \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 Types / Network Device / Legacy Interface: Framing Requirements}
-- 
2.25.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] [PATCH V2] virtio-net: introduce admin control virtqueue

[Haozhong Zhang]

[-- Attachment #1: Type: text/plain, Size: 8070 bytes --]

On 01/25/21 13:52, Jason Wang wrote:
> When implementing virtual devices like SR-IOV or sub-function. We're
> suffering from several issues:
> 
> - There's no management interface for management device to
>   configure features, attributes for a virtual device
> - Per virtual device control virtqueue could be very expensive as the
>   number of virtual devices could be very large
> - Virtualize per virtual device's control virtqueue could be very
>   challenge as we need the support of DMA isolation at queue level
> 
> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
> allows the device to implement a single admin control virtqueue to
> manage the features and attributes for a specific virtual device.
> 
> The idea is simple, a new virtual device id is introduced on top of
> the existing virtio_net_ctrl structure. This id is transport or device
> specific to uniquely identify a management or virtual device.
> 
> With this, we get a way of using management device (PF) to configure
> per virtual device features and attributes. And since the admin
> control virtqueue belongs to management device (PF), the DMA is
> naturally isolated at device level instead of the queue level for per
> virtual device control vq.
> 
> When the admin cvq is offered by management device and normal cvq is
> offered by virtual device. A new command class is introduced decide
> whether or not to accept commands from normal cvq for a virtual
> device.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V1:
> - use 'virtual device' instead of 'function'
> - introuce trust command
> - clairfy that the admin cvq could be used to configure management
>   device itself
> ---
>  content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 620c0e2..989b4f6 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>      channel.
>  
> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
> +    available.
> +
>  \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>      value and a type of calculated hash.
>  
> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>  
>  The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
> -negotiated) to send commands to manipulate various features of
> -the device which would not easily map into the configuration
> -space.
> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
> +commands to manipulate various features of the device which would not
> +easily map into the configuration space.
>  
>  All commands are of the following form:
> +is not negotiated:
>  
>  \begin{lstlisting}
>  struct virtio_net_ctrl {
> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  do except issue a diagnostic if \field{ack} is not
>  VIRTIO_NET_OK.
>  
> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
> +negotiated, the driver can use the admin control virtqueue of the
> +management device to manipulate features of individual virtual devices
> +where the control virtqueue is not easily implemented. The definition
> +of management device and virtual device is transport or device
> +specific. E.g in the case of PCI SR-IOV, the management device is
> +implemented via the physical function (PF), then the virtual device is
> +the virtual function (VF) in this case.
> +
> +All commands are of the following form:
> +
> +\begin{lstlisting}
> +struct virtio_net_admin_ctrl {
> +        u32 virtual_device_id;
> +        struct virtio_net_ctrl ctrl;
> +};
> +\end{lstlisting}
> +
> +The \field{virtual_device_id} is an unique transport or device specific
> +identifier for a virtual device or management device. E.g for the case
> +of PCI SR-IOV, it's the PCI function id. Management device MUST
> +reserve 0 for \field{virtual_device_id} to identify itself.
> +

Hi Jason,

One question about the valid scope of virtual_device_id. Consider a
case, that

0. A VF with PCI address "xx:yy.z" exists
1. The virtio driver commits an admin command with virtual_device_id
   "xx:yy.z" admin queue
2. Before the device processes the command, the virtio driver disables
   PF SRIOV, then re-enables it, and then creates a VF with the same
   address as "xx:yy.z".

Now, should the command committed in step 1 still be considered as
valid after step 2? If not, shall there be a flush or abort operation
to allow the driver to drop commands between step 1 and 2? Or simply
let the driver just wait until all admin commands have been processed.

Thanks,
Haozhong


>  \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
>  \label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
>  
> @@ -4308,6 +4335,32 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  according to the native endian of the guest rather than
>  (necessarily when not using the legacy interface) little-endian.
>  
> +\paragraph{Setting Trust for Virtual Device}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ is negotiated, the management device
> +can accept operations via the control vq from the trusted virtual
> +device.
> +
> +\begin{lstlisting}
> +#define VIRTIO_NET_ADMIN_CTRL_TRUST 0
> + #define VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE 0
> +\end{lstlisting}
> +
> +\devicenormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ has been negotiated, the device MUST
> +support VIRTIO_NET_ADMIN_CTRL_TRUST class
> +command. VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE turns trust for the
> +control virtqueue of a specific virtual device on and off. The command
> +specific data is one byte containing 0(off) or 1(on).  When trust is
> +off the device MUST fail any operations from the control virtqueue of
> +the virtual device. The management device MUST NOT trust any virtual
> +devices after reset.
> +
> +\drivernormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +A driver SHOULD negotiate VIRTIO_NET_F_ADMIN_CTRL_VQ if the device
> +offers it.
>  
>  \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>  Types / Network Device / Legacy Interface: Framing Requirements}
> -- 
> 2.25.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/
> 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [virtio-comment] [PATCH V2] virtio-net: introduce admin control virtqueue

[Jason Wang]

On 2021/1/25 下午2:31, Haozhong Zhang wrote:
> On 01/25/21 13:52, Jason Wang wrote:
>> When implementing virtual devices like SR-IOV or sub-function. We're
>> suffering from several issues:
>>
>> - There's no management interface for management device to
>>    configure features, attributes for a virtual device
>> - Per virtual device control virtqueue could be very expensive as the
>>    number of virtual devices could be very large
>> - Virtualize per virtual device's control virtqueue could be very
>>    challenge as we need the support of DMA isolation at queue level
>>
>> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
>> allows the device to implement a single admin control virtqueue to
>> manage the features and attributes for a specific virtual device.
>>
>> The idea is simple, a new virtual device id is introduced on top of
>> the existing virtio_net_ctrl structure. This id is transport or device
>> specific to uniquely identify a management or virtual device.
>>
>> With this, we get a way of using management device (PF) to configure
>> per virtual device features and attributes. And since the admin
>> control virtqueue belongs to management device (PF), the DMA is
>> naturally isolated at device level instead of the queue level for per
>> virtual device control vq.
>>
>> When the admin cvq is offered by management device and normal cvq is
>> offered by virtual device. A new command class is introduced decide
>> whether or not to accept commands from normal cvq for a virtual
>> device.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Changes from V1:
>> - use 'virtual device' instead of 'function'
>> - introuce trust command
>> - clairfy that the admin cvq could be used to configure management
>>    device itself
>> ---
>>   content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 56 insertions(+), 3 deletions(-)
>>
>> diff --git a/content.tex b/content.tex
>> index 620c0e2..989b4f6 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>>   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>>       channel.
>>   
>> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
>> +    available.
>> +
>>   \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>>       value and a type of calculated hash.
>>   
>> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>   \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>>   
>>   The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
>> -negotiated) to send commands to manipulate various features of
>> -the device which would not easily map into the configuration
>> -space.
>> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
>> +commands to manipulate various features of the device which would not
>> +easily map into the configuration space.
>>   
>>   All commands are of the following form:
>> +is not negotiated:
>>   
>>   \begin{lstlisting}
>>   struct virtio_net_ctrl {
>> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   do except issue a diagnostic if \field{ack} is not
>>   VIRTIO_NET_OK.
>>   
>> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
>> +negotiated, the driver can use the admin control virtqueue of the
>> +management device to manipulate features of individual virtual devices
>> +where the control virtqueue is not easily implemented. The definition
>> +of management device and virtual device is transport or device
>> +specific. E.g in the case of PCI SR-IOV, the management device is
>> +implemented via the physical function (PF), then the virtual device is
>> +the virtual function (VF) in this case.
>> +
>> +All commands are of the following form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_net_admin_ctrl {
>> +        u32 virtual_device_id;
>> +        struct virtio_net_ctrl ctrl;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{virtual_device_id} is an unique transport or device specific
>> +identifier for a virtual device or management device. E.g for the case
>> +of PCI SR-IOV, it's the PCI function id. Management device MUST
>> +reserve 0 for \field{virtual_device_id} to identify itself.
>> +
> Hi Jason,
>
> One question about the valid scope of virtual_device_id. Consider a
> case, that
>
> 0. A VF with PCI address "xx:yy.z" exists
> 1. The virtio driver commits an admin command with virtual_device_id
>     "xx:yy.z" admin queue
> 2. Before the device processes the command, the virtio driver disables
>     PF SRIOV, then re-enables it, and then creates a VF with the same
>     address as "xx:yy.z".
>
> Now, should the command committed in step 1 still be considered as
> valid after step 2? If not, shall there be a flush or abort operation
> to allow the driver to drop commands between step 1 and 2? Or simply
> let the driver just wait until all admin commands have been processed.


Good point. My understanding is:

 From device level: the effect of the command ties to the lifecycle of 
the device (e.g VF). So if virtual device (VF) is destroyed (e.g SR-IOV 
is disabled), the command won't be effective any more even if it is 
re-created later. And if the virtual device is destroyed during 
processing the admin command, the management device (PF) must fail the 
command.

 From driver level: The driver should synchronize between the admin 
control vq processing and virtual devices create/destroy to avoid 
unexpected results.

Does this make sense?

I think we can clarify this in the spec.

Thanks


>
> Thanks,
> Haozhong
>
>
>>   \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
>>   \label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
>>   
>> @@ -4308,6 +4335,32 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   according to the native endian of the guest rather than
>>   (necessarily when not using the legacy interface) little-endian.
>>   
>> +\paragraph{Setting Trust for Virtual Device}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ is negotiated, the management device
>> +can accept operations via the control vq from the trusted virtual
>> +device.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_NET_ADMIN_CTRL_TRUST 0
>> + #define VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE 0
>> +\end{lstlisting}
>> +
>> +\devicenormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ has been negotiated, the device MUST
>> +support VIRTIO_NET_ADMIN_CTRL_TRUST class
>> +command. VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE turns trust for the
>> +control virtqueue of a specific virtual device on and off. The command
>> +specific data is one byte containing 0(off) or 1(on).  When trust is
>> +off the device MUST fail any operations from the control virtqueue of
>> +the virtual device. The management device MUST NOT trust any virtual
>> +devices after reset.
>> +
>> +\drivernormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +A driver SHOULD negotiate VIRTIO_NET_F_ADMIN_CTRL_VQ if the device
>> +offers it.
>>   
>>   \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>>   Types / Network Device / Legacy Interface: Framing Requirements}
>> -- 
>> 2.25.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/
>>
>


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] [PATCH V2] virtio-net: introduce admin control virtqueue

[Haozhong Zhang]

[-- Attachment #1: Type: text/plain, Size: 6557 bytes --]

On 01/25/21 14:45, Jason Wang wrote:
> 
> On 2021/1/25 下午2:31, Haozhong Zhang wrote:
> > On 01/25/21 13:52, Jason Wang wrote:
> > > When implementing virtual devices like SR-IOV or sub-function. We're
> > > suffering from several issues:
> > > 
> > > - There's no management interface for management device to
> > >    configure features, attributes for a virtual device
> > > - Per virtual device control virtqueue could be very expensive as the
> > >    number of virtual devices could be very large
> > > - Virtualize per virtual device's control virtqueue could be very
> > >    challenge as we need the support of DMA isolation at queue level
> > > 
> > > So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
> > > allows the device to implement a single admin control virtqueue to
> > > manage the features and attributes for a specific virtual device.
> > > 
> > > The idea is simple, a new virtual device id is introduced on top of
> > > the existing virtio_net_ctrl structure. This id is transport or device
> > > specific to uniquely identify a management or virtual device.
> > > 
> > > With this, we get a way of using management device (PF) to configure
> > > per virtual device features and attributes. And since the admin
> > > control virtqueue belongs to management device (PF), the DMA is
> > > naturally isolated at device level instead of the queue level for per
> > > virtual device control vq.
> > > 
> > > When the admin cvq is offered by management device and normal cvq is
> > > offered by virtual device. A new command class is introduced decide
> > > whether or not to accept commands from normal cvq for a virtual
> > > device.
> > > 
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > Changes from V1:
> > > - use 'virtual device' instead of 'function'
> > > - introuce trust command
> > > - clairfy that the admin cvq could be used to configure management
> > >    device itself
> > > ---
> > >   content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 56 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 620c0e2..989b4f6 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
> > >   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> > >       channel.
> > > +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
> > > +    available.
> > > +
> > >   \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
> > >       value and a type of calculated hash.
> > > @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> > >   \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
> > >   The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
> > > -negotiated) to send commands to manipulate various features of
> > > -the device which would not easily map into the configuration
> > > -space.
> > > +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
> > > +commands to manipulate various features of the device which would not
> > > +easily map into the configuration space.
> > >   All commands are of the following form:
> > > +is not negotiated:
> > >   \begin{lstlisting}
> > >   struct virtio_net_ctrl {
> > > @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > >   do except issue a diagnostic if \field{ack} is not
> > >   VIRTIO_NET_OK.
> > > +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
> > > +negotiated, the driver can use the admin control virtqueue of the
> > > +management device to manipulate features of individual virtual devices
> > > +where the control virtqueue is not easily implemented. The definition
> > > +of management device and virtual device is transport or device
> > > +specific. E.g in the case of PCI SR-IOV, the management device is
> > > +implemented via the physical function (PF), then the virtual device is
> > > +the virtual function (VF) in this case.
> > > +
> > > +All commands are of the following form:
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_net_admin_ctrl {
> > > +        u32 virtual_device_id;
> > > +        struct virtio_net_ctrl ctrl;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The \field{virtual_device_id} is an unique transport or device specific
> > > +identifier for a virtual device or management device. E.g for the case
> > > +of PCI SR-IOV, it's the PCI function id. Management device MUST
> > > +reserve 0 for \field{virtual_device_id} to identify itself.
> > > +
> > Hi Jason,
> > 
> > One question about the valid scope of virtual_device_id. Consider a
> > case, that
> > 
> > 0. A VF with PCI address "xx:yy.z" exists
> > 1. The virtio driver commits an admin command with virtual_device_id
> >     "xx:yy.z" admin queue
> > 2. Before the device processes the command, the virtio driver disables
> >     PF SRIOV, then re-enables it, and then creates a VF with the same
> >     address as "xx:yy.z".
> > 
> > Now, should the command committed in step 1 still be considered as
> > valid after step 2? If not, shall there be a flush or abort operation
> > to allow the driver to drop commands between step 1 and 2? Or simply
> > let the driver just wait until all admin commands have been processed.
> 
> 
> Good point. My understanding is:
> 
> From device level: the effect of the command ties to the lifecycle of the
> device (e.g VF). So if virtual device (VF) is destroyed (e.g SR-IOV is
> disabled), the command won't be effective any more even if it is re-created
> later. And if the virtual device is destroyed during processing the admin
> command, the management device (PF) must fail the command.
> 
> From driver level: The driver should synchronize between the admin control
> vq processing and virtual devices create/destroy to avoid unexpected
> results.
>
> Does this make sense?
> 
> I think we can clarify this in the spec.

I guess the above virtual device destroy also includes the case that
the virtual device is already in NEEDS_RESET status. If so, all make
sense. I think the driver can synchronize easily, for example by just
waiting all commands targeted to that VF failed.

Haozhong

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [virtio-comment] [PATCH V2] virtio-net: introduce admin control virtqueue

[Jason Wang]

On 2021/1/25 下午3:05, Haozhong Zhang wrote:
> On 01/25/21 14:45, Jason Wang wrote:
>> On 2021/1/25 下午2:31, Haozhong Zhang wrote:
>>> On 01/25/21 13:52, Jason Wang wrote:
>>>> When implementing virtual devices like SR-IOV or sub-function. We're
>>>> suffering from several issues:
>>>>
>>>> - There's no management interface for management device to
>>>>     configure features, attributes for a virtual device
>>>> - Per virtual device control virtqueue could be very expensive as the
>>>>     number of virtual devices could be very large
>>>> - Virtualize per virtual device's control virtqueue could be very
>>>>     challenge as we need the support of DMA isolation at queue level
>>>>
>>>> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
>>>> allows the device to implement a single admin control virtqueue to
>>>> manage the features and attributes for a specific virtual device.
>>>>
>>>> The idea is simple, a new virtual device id is introduced on top of
>>>> the existing virtio_net_ctrl structure. This id is transport or device
>>>> specific to uniquely identify a management or virtual device.
>>>>
>>>> With this, we get a way of using management device (PF) to configure
>>>> per virtual device features and attributes. And since the admin
>>>> control virtqueue belongs to management device (PF), the DMA is
>>>> naturally isolated at device level instead of the queue level for per
>>>> virtual device control vq.
>>>>
>>>> When the admin cvq is offered by management device and normal cvq is
>>>> offered by virtual device. A new command class is introduced decide
>>>> whether or not to accept commands from normal cvq for a virtual
>>>> device.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>> Changes from V1:
>>>> - use 'virtual device' instead of 'function'
>>>> - introuce trust command
>>>> - clairfy that the admin cvq could be used to configure management
>>>>     device itself
>>>> ---
>>>>    content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>    1 file changed, 56 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 620c0e2..989b4f6 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>>>>    \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>>>>        channel.
>>>> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
>>>> +    available.
>>>> +
>>>>    \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>>>>        value and a type of calculated hash.
>>>> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>>>    \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>>>>    The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
>>>> -negotiated) to send commands to manipulate various features of
>>>> -the device which would not easily map into the configuration
>>>> -space.
>>>> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
>>>> +commands to manipulate various features of the device which would not
>>>> +easily map into the configuration space.
>>>>    All commands are of the following form:
>>>> +is not negotiated:
>>>>    \begin{lstlisting}
>>>>    struct virtio_net_ctrl {
>>>> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>    do except issue a diagnostic if \field{ack} is not
>>>>    VIRTIO_NET_OK.
>>>> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
>>>> +negotiated, the driver can use the admin control virtqueue of the
>>>> +management device to manipulate features of individual virtual devices
>>>> +where the control virtqueue is not easily implemented. The definition
>>>> +of management device and virtual device is transport or device
>>>> +specific. E.g in the case of PCI SR-IOV, the management device is
>>>> +implemented via the physical function (PF), then the virtual device is
>>>> +the virtual function (VF) in this case.
>>>> +
>>>> +All commands are of the following form:
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_net_admin_ctrl {
>>>> +        u32 virtual_device_id;
>>>> +        struct virtio_net_ctrl ctrl;
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +The \field{virtual_device_id} is an unique transport or device specific
>>>> +identifier for a virtual device or management device. E.g for the case
>>>> +of PCI SR-IOV, it's the PCI function id. Management device MUST
>>>> +reserve 0 for \field{virtual_device_id} to identify itself.
>>>> +
>>> Hi Jason,
>>>
>>> One question about the valid scope of virtual_device_id. Consider a
>>> case, that
>>>
>>> 0. A VF with PCI address "xx:yy.z" exists
>>> 1. The virtio driver commits an admin command with virtual_device_id
>>>      "xx:yy.z" admin queue
>>> 2. Before the device processes the command, the virtio driver disables
>>>      PF SRIOV, then re-enables it, and then creates a VF with the same
>>>      address as "xx:yy.z".
>>>
>>> Now, should the command committed in step 1 still be considered as
>>> valid after step 2? If not, shall there be a flush or abort operation
>>> to allow the driver to drop commands between step 1 and 2? Or simply
>>> let the driver just wait until all admin commands have been processed.
>>
>> Good point. My understanding is:
>>
>>  From device level: the effect of the command ties to the lifecycle of the
>> device (e.g VF). So if virtual device (VF) is destroyed (e.g SR-IOV is
>> disabled), the command won't be effective any more even if it is re-created
>> later. And if the virtual device is destroyed during processing the admin
>> command, the management device (PF) must fail the command.
>>
>>  From driver level: The driver should synchronize between the admin control
>> vq processing and virtual devices create/destroy to avoid unexpected
>> results.
>>
>> Does this make sense?
>>
>> I think we can clarify this in the spec.
> I guess the above virtual device destroy also includes the case that
> the virtual device is already in NEEDS_RESET status.


We can provide that, but in your case the virtual device is going to be 
destroyed soon, I'm not sure how much it can help.


> If so, all make
> sense. I think the driver can synchronize easily, for example by just
> waiting all commands targeted to that VF failed.


Before disabling SR-IOV, PF driver needs to send the notification to VF 
drivers to sync admin cvq commands with its shutdown. I'm not sure which 
way is more easier either of your proposal should work:

1) busy wait for the completion of the command
2) let the PF to deal with the possible failure

Thanks


>
> Haozhong


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] [PATCH V2] virtio-net: introduce admin control virtqueue

[Haozhong Zhang]

[-- Attachment #1: Type: text/plain, Size: 9085 bytes --]

On 01/25/21 15:48, Jason Wang wrote:
> 
> On 2021/1/25 下午3:05, Haozhong Zhang wrote:
> > On 01/25/21 14:45, Jason Wang wrote:
> > > On 2021/1/25 下午2:31, Haozhong Zhang wrote:
> > > > On 01/25/21 13:52, Jason Wang wrote:
> > > > > When implementing virtual devices like SR-IOV or sub-function. We're
> > > > > suffering from several issues:
> > > > > 
> > > > > - There's no management interface for management device to
> > > > >     configure features, attributes for a virtual device
> > > > > - Per virtual device control virtqueue could be very expensive as the
> > > > >     number of virtual devices could be very large
> > > > > - Virtualize per virtual device's control virtqueue could be very
> > > > >     challenge as we need the support of DMA isolation at queue level
> > > > > 
> > > > > So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
> > > > > allows the device to implement a single admin control virtqueue to
> > > > > manage the features and attributes for a specific virtual device.
> > > > > 
> > > > > The idea is simple, a new virtual device id is introduced on top of
> > > > > the existing virtio_net_ctrl structure. This id is transport or device
> > > > > specific to uniquely identify a management or virtual device.
> > > > > 
> > > > > With this, we get a way of using management device (PF) to configure
> > > > > per virtual device features and attributes. And since the admin
> > > > > control virtqueue belongs to management device (PF), the DMA is
> > > > > naturally isolated at device level instead of the queue level for per
> > > > > virtual device control vq.
> > > > > 
> > > > > When the admin cvq is offered by management device and normal cvq is
> > > > > offered by virtual device. A new command class is introduced decide
> > > > > whether or not to accept commands from normal cvq for a virtual
> > > > > device.
> > > > > 
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > ---
> > > > > Changes from V1:
> > > > > - use 'virtual device' instead of 'function'
> > > > > - introuce trust command
> > > > > - clairfy that the admin cvq could be used to configure management
> > > > >     device itself
> > > > > ---
> > > > >    content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> > > > >    1 file changed, 56 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/content.tex b/content.tex
> > > > > index 620c0e2..989b4f6 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
> > > > >    \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> > > > >        channel.
> > > > > +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
> > > > > +    available.
> > > > > +
> > > > >    \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
> > > > >        value and a type of calculated hash.
> > > > > @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> > > > >    \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
> > > > >    The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
> > > > > -negotiated) to send commands to manipulate various features of
> > > > > -the device which would not easily map into the configuration
> > > > > -space.
> > > > > +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
> > > > > +commands to manipulate various features of the device which would not
> > > > > +easily map into the configuration space.
> > > > >    All commands are of the following form:
> > > > > +is not negotiated:
> > > > >    \begin{lstlisting}
> > > > >    struct virtio_net_ctrl {
> > > > > @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > >    do except issue a diagnostic if \field{ack} is not
> > > > >    VIRTIO_NET_OK.
> > > > > +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
> > > > > +negotiated, the driver can use the admin control virtqueue of the
> > > > > +management device to manipulate features of individual virtual devices
> > > > > +where the control virtqueue is not easily implemented. The definition
> > > > > +of management device and virtual device is transport or device
> > > > > +specific. E.g in the case of PCI SR-IOV, the management device is
> > > > > +implemented via the physical function (PF), then the virtual device is
> > > > > +the virtual function (VF) in this case.
> > > > > +
> > > > > +All commands are of the following form:
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_net_admin_ctrl {
> > > > > +        u32 virtual_device_id;
> > > > > +        struct virtio_net_ctrl ctrl;
> > > > > +};
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +The \field{virtual_device_id} is an unique transport or device specific
> > > > > +identifier for a virtual device or management device. E.g for the case
> > > > > +of PCI SR-IOV, it's the PCI function id. Management device MUST
> > > > > +reserve 0 for \field{virtual_device_id} to identify itself.
> > > > > +
> > > > Hi Jason,
> > > > 
> > > > One question about the valid scope of virtual_device_id. Consider a
> > > > case, that
> > > > 
> > > > 0. A VF with PCI address "xx:yy.z" exists
> > > > 1. The virtio driver commits an admin command with virtual_device_id
> > > >      "xx:yy.z" admin queue
> > > > 2. Before the device processes the command, the virtio driver disables
> > > >      PF SRIOV, then re-enables it, and then creates a VF with the same
> > > >      address as "xx:yy.z".
> > > > 
> > > > Now, should the command committed in step 1 still be considered as
> > > > valid after step 2? If not, shall there be a flush or abort operation
> > > > to allow the driver to drop commands between step 1 and 2? Or simply
> > > > let the driver just wait until all admin commands have been processed.
> > > 
> > > Good point. My understanding is:
> > > 
> > >  From device level: the effect of the command ties to the lifecycle of the
> > > device (e.g VF). So if virtual device (VF) is destroyed (e.g SR-IOV is
> > > disabled), the command won't be effective any more even if it is re-created
> > > later. And if the virtual device is destroyed during processing the admin
> > > command, the management device (PF) must fail the command.
> > > 
> > >  From driver level: The driver should synchronize between the admin control
> > > vq processing and virtual devices create/destroy to avoid unexpected
> > > results.
> > > 
> > > Does this make sense?
> > > 
> > > I think we can clarify this in the spec.
> > I guess the above virtual device destroy also includes the case that
> > the virtual device is already in NEEDS_RESET status.
> 
> 
> We can provide that, but in your case the virtual device is going to be
> destroyed soon, I'm not sure how much it can help.
> 
> 
> > If so, all make
> > sense. I think the driver can synchronize easily, for example by just
> > waiting all commands targeted to that VF failed.
> 
> 
> Before disabling SR-IOV, PF driver needs to send the notification to VF
> drivers to sync admin cvq commands with its shutdown.

I think this notification can be used by the driver as a "barrier":
- all commands for this VF before the barrier that have not been
  completed can be ignored (e.g., if a failure status is later
  returned for such a command, the driver will not need to do anything
  as the device has been destroyed)
- all commands for this VF after the barrier are for the new (or
  re-enabled) VF.

Then the driver would not need to busy wait for all commands before
the barrier to complete before it can perform other operations on that
VF (e.g., disable/re-enabling...).

> I'm not sure which way
> is more easier either of your proposal should work:
> 
> 1) busy wait for the completion of the command
> 2) let the PF to deal with the possible failure
> 
> Thanks
> 
> 
> > 
> > Haozhong
> 
> 
> 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/
> 
- Haozhong Zhang


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[virtio-comment] Re: [PATCH V2] virtio-net: introduce admin control virtqueue

[Cornelia Huck]

On Mon, 25 Jan 2021 13:52:02 +0800
Jason Wang <jasowang@redhat.com> wrote:

> When implementing virtual devices like SR-IOV or sub-function. We're
> suffering from several issues:
> 
> - There's no management interface for management device to
>   configure features, attributes for a virtual device
> - Per virtual device control virtqueue could be very expensive as the
>   number of virtual devices could be very large
> - Virtualize per virtual device's control virtqueue could be very
>   challenge as we need the support of DMA isolation at queue level
> 
> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
> allows the device to implement a single admin control virtqueue to
> manage the features and attributes for a specific virtual device.
> 
> The idea is simple, a new virtual device id is introduced on top of
> the existing virtio_net_ctrl structure. This id is transport or device
> specific to uniquely identify a management or virtual device.
> 
> With this, we get a way of using management device (PF) to configure
> per virtual device features and attributes. And since the admin
> control virtqueue belongs to management device (PF), the DMA is
> naturally isolated at device level instead of the queue level for per
> virtual device control vq.
> 
> When the admin cvq is offered by management device and normal cvq is
> offered by virtual device. A new command class is introduced decide
> whether or not to accept commands from normal cvq for a virtual
> device.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V1:
> - use 'virtual device' instead of 'function'
> - introuce trust command
> - clairfy that the admin cvq could be used to configure management
>   device itself
> ---
>  content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 620c0e2..989b4f6 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>      channel.
>  
> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
> +    available.
> +
>  \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>      value and a type of calculated hash.
>  
> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>  
>  The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
> -negotiated) to send commands to manipulate various features of
> -the device which would not easily map into the configuration
> -space.
> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
> +commands to manipulate various features of the device which would not
> +easily map into the configuration space.
>  
>  All commands are of the following form:
> +is not negotiated:

Stray hunk.

>  
>  \begin{lstlisting}
>  struct virtio_net_ctrl {
> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  do except issue a diagnostic if \field{ack} is not
>  VIRTIO_NET_OK.
>  
> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
> +negotiated, the driver can use the admin control virtqueue of the
> +management device to manipulate features of individual virtual devices
> +where the control virtqueue is not easily implemented. The definition
> +of management device and virtual device is transport or device
> +specific. E.g in the case of PCI SR-IOV, the management device is
> +implemented via the physical function (PF), then the virtual device is
> +the virtual function (VF) in this case.

"...if the management device is implemented via the physical function
(PF), the virtual device is implemented via the virtual function (VF)."
?

> +
> +All commands are of the following form:
> +
> +\begin{lstlisting}
> +struct virtio_net_admin_ctrl {
> +        u32 virtual_device_id;
> +        struct virtio_net_ctrl ctrl;
> +};
> +\end{lstlisting}
> +
> +The \field{virtual_device_id} is an unique transport or device specific
> +identifier for a virtual device or management device. E.g for the case
> +of PCI SR-IOV, it's the PCI function id. Management device MUST

"...it is the PCI function id of the virtual function." ?

s/Management device/The management device/

> +reserve 0 for \field{virtual_device_id} to identify itself.

"reserve", or rather "use"?

That also implies that virtual_device_id must be != 0 for virtual
devices, doesn't it?

> +
>  \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
>  \label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
>  
> @@ -4308,6 +4335,32 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  according to the native endian of the guest rather than
>  (necessarily when not using the legacy interface) little-endian.
>  
> +\paragraph{Setting Trust for Virtual Device}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ is negotiated, the management device
> +can accept operations via the control vq from the trusted virtual

"can accept the following operations" ?

(in addition to existing control commands?)

> +device.
> +
> +\begin{lstlisting}
> +#define VIRTIO_NET_ADMIN_CTRL_TRUST 0
> + #define VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE 0
> +\end{lstlisting}
> +
> +\devicenormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ has been negotiated, the device MUST

s/If the/If/

> +support VIRTIO_NET_ADMIN_CTRL_TRUST class

s/support/support the/

> +command. VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE turns trust for the
> +control virtqueue of a specific virtual device on and off. The command
> +specific data is one byte containing 0(off) or 1(on).  When trust is
> +off the device MUST fail any operations from the control virtqueue of
> +the virtual device. The management device MUST NOT trust any virtual
> +devices after reset.

A bit further up, it is stated that the management device can accept
operations from a "trusted virtual device". From this statement here, I
get that the trust state can be turned on and off by the virtual device.

Does the management device get to choose if it wants to trust a virtual
device, i.e. can it fail enabling the trust?

> +
> +\drivernormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +A driver SHOULD negotiate VIRTIO_NET_F_ADMIN_CTRL_VQ if the device
> +offers it.
>  
>  \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>  Types / Network Device / Legacy Interface: Framing Requirements}


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] [PATCH V2] virtio-net: introduce admin control virtqueue

[Jason Wang]

On 2021/1/25 下午6:22, Haozhong Zhang wrote:
> On 01/25/21 15:48, Jason Wang wrote:
>> On 2021/1/25 下午3:05, Haozhong Zhang wrote:
>>> On 01/25/21 14:45, Jason Wang wrote:
>>>> On 2021/1/25 下午2:31, Haozhong Zhang wrote:
>>>>> On 01/25/21 13:52, Jason Wang wrote:
>>>>>> When implementing virtual devices like SR-IOV or sub-function. We're
>>>>>> suffering from several issues:
>>>>>>
>>>>>> - There's no management interface for management device to
>>>>>>      configure features, attributes for a virtual device
>>>>>> - Per virtual device control virtqueue could be very expensive as the
>>>>>>      number of virtual devices could be very large
>>>>>> - Virtualize per virtual device's control virtqueue could be very
>>>>>>      challenge as we need the support of DMA isolation at queue level
>>>>>>
>>>>>> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
>>>>>> allows the device to implement a single admin control virtqueue to
>>>>>> manage the features and attributes for a specific virtual device.
>>>>>>
>>>>>> The idea is simple, a new virtual device id is introduced on top of
>>>>>> the existing virtio_net_ctrl structure. This id is transport or device
>>>>>> specific to uniquely identify a management or virtual device.
>>>>>>
>>>>>> With this, we get a way of using management device (PF) to configure
>>>>>> per virtual device features and attributes. And since the admin
>>>>>> control virtqueue belongs to management device (PF), the DMA is
>>>>>> naturally isolated at device level instead of the queue level for per
>>>>>> virtual device control vq.
>>>>>>
>>>>>> When the admin cvq is offered by management device and normal cvq is
>>>>>> offered by virtual device. A new command class is introduced decide
>>>>>> whether or not to accept commands from normal cvq for a virtual
>>>>>> device.
>>>>>>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>> ---
>>>>>> Changes from V1:
>>>>>> - use 'virtual device' instead of 'function'
>>>>>> - introuce trust command
>>>>>> - clairfy that the admin cvq could be used to configure management
>>>>>>      device itself
>>>>>> ---
>>>>>>     content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>     1 file changed, 56 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index 620c0e2..989b4f6 100644
>>>>>> --- a/content.tex
>>>>>> +++ b/content.tex
>>>>>> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>>>>>>     \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>>>>>>         channel.
>>>>>> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
>>>>>> +    available.
>>>>>> +
>>>>>>     \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>>>>>>         value and a type of calculated hash.
>>>>>> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>>>>>     \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>>>>>>     The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
>>>>>> -negotiated) to send commands to manipulate various features of
>>>>>> -the device which would not easily map into the configuration
>>>>>> -space.
>>>>>> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
>>>>>> +commands to manipulate various features of the device which would not
>>>>>> +easily map into the configuration space.
>>>>>>     All commands are of the following form:
>>>>>> +is not negotiated:
>>>>>>     \begin{lstlisting}
>>>>>>     struct virtio_net_ctrl {
>>>>>> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>>     do except issue a diagnostic if \field{ack} is not
>>>>>>     VIRTIO_NET_OK.
>>>>>> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
>>>>>> +negotiated, the driver can use the admin control virtqueue of the
>>>>>> +management device to manipulate features of individual virtual devices
>>>>>> +where the control virtqueue is not easily implemented. The definition
>>>>>> +of management device and virtual device is transport or device
>>>>>> +specific. E.g in the case of PCI SR-IOV, the management device is
>>>>>> +implemented via the physical function (PF), then the virtual device is
>>>>>> +the virtual function (VF) in this case.
>>>>>> +
>>>>>> +All commands are of the following form:
>>>>>> +
>>>>>> +\begin{lstlisting}
>>>>>> +struct virtio_net_admin_ctrl {
>>>>>> +        u32 virtual_device_id;
>>>>>> +        struct virtio_net_ctrl ctrl;
>>>>>> +};
>>>>>> +\end{lstlisting}
>>>>>> +
>>>>>> +The \field{virtual_device_id} is an unique transport or device specific
>>>>>> +identifier for a virtual device or management device. E.g for the case
>>>>>> +of PCI SR-IOV, it's the PCI function id. Management device MUST
>>>>>> +reserve 0 for \field{virtual_device_id} to identify itself.
>>>>>> +
>>>>> Hi Jason,
>>>>>
>>>>> One question about the valid scope of virtual_device_id. Consider a
>>>>> case, that
>>>>>
>>>>> 0. A VF with PCI address "xx:yy.z" exists
>>>>> 1. The virtio driver commits an admin command with virtual_device_id
>>>>>       "xx:yy.z" admin queue
>>>>> 2. Before the device processes the command, the virtio driver disables
>>>>>       PF SRIOV, then re-enables it, and then creates a VF with the same
>>>>>       address as "xx:yy.z".
>>>>>
>>>>> Now, should the command committed in step 1 still be considered as
>>>>> valid after step 2? If not, shall there be a flush or abort operation
>>>>> to allow the driver to drop commands between step 1 and 2? Or simply
>>>>> let the driver just wait until all admin commands have been processed.
>>>> Good point. My understanding is:
>>>>
>>>>   From device level: the effect of the command ties to the lifecycle of the
>>>> device (e.g VF). So if virtual device (VF) is destroyed (e.g SR-IOV is
>>>> disabled), the command won't be effective any more even if it is re-created
>>>> later. And if the virtual device is destroyed during processing the admin
>>>> command, the management device (PF) must fail the command.
>>>>
>>>>   From driver level: The driver should synchronize between the admin control
>>>> vq processing and virtual devices create/destroy to avoid unexpected
>>>> results.
>>>>
>>>> Does this make sense?
>>>>
>>>> I think we can clarify this in the spec.
>>> I guess the above virtual device destroy also includes the case that
>>> the virtual device is already in NEEDS_RESET status.
>>
>> We can provide that, but in your case the virtual device is going to be
>> destroyed soon, I'm not sure how much it can help.
>>
>>
>>> If so, all make
>>> sense. I think the driver can synchronize easily, for example by just
>>> waiting all commands targeted to that VF failed.
>>
>> Before disabling SR-IOV, PF driver needs to send the notification to VF
>> drivers to sync admin cvq commands with its shutdown.
> I think this notification can be used by the driver as a "barrier":
> - all commands for this VF before the barrier that have not been
>    completed can be ignored (e.g., if a failure status is later
>    returned for such a command, the driver will not need to do anything
>    as the device has been destroyed)
> - all commands for this VF after the barrier are for the new (or
>    re-enabled) VF.
>
> Then the driver would not need to busy wait for all commands before
> the barrier to complete before it can perform other operations on that
> VF (e.g., disable/re-enabling...).


Just to make sure we are in the same page. For "notification", I meant a 
software approach to sync SRIOV disabling with VF driver shutdown.

For sync with VF. In the linux kernel implementation of 
pci_disable_sriov(). It will shutdown VF drivers before disabling the 
SRIOV via config space. So for VF it works like before, the driver needs 
to use subsystem specific way to sync between device shutdown and cvq 
command. E.g in the virtio-net, we sync through RTNL lock (see 
virtnet_remove). So it was guaranteed that the VF is not destroyed when 
we're sending and waiting for the result of VF.

For sync with SRIOV enable/disable with admin cvq. It's the 
responsibility of the driver to deal with the synchronization correctly. 
(E.g in linux and virtio-net RTNL is a good candidate). At the device 
level, it's not guaranteed that the driver is properly destroyed. So 
when the device is receiving admin cvq command when the virtual device 
is not existed, the device must fail this command.

So my understanding is that for the case of Linux virito-net, RTNL lock 
is the "barrier" that you want.

Thanks




>
>> I'm not sure which way
>> is more easier either of your proposal should work:
>>
>> 1) busy wait for the completion of the command
>> 2) let the PF to deal with the possible failure
>>
>> Thanks
>>
>>
>>> Haozhong
>>
>> 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/
>>
> - Haozhong Zhang
>


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 V2] virtio-net: introduce admin control virtqueue

[Jason Wang]

On 2021/1/25 下午8:54, Cornelia Huck wrote:
> On Mon, 25 Jan 2021 13:52:02 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> When implementing virtual devices like SR-IOV or sub-function. We're
>> suffering from several issues:
>>
>> - There's no management interface for management device to
>>    configure features, attributes for a virtual device
>> - Per virtual device control virtqueue could be very expensive as the
>>    number of virtual devices could be very large
>> - Virtualize per virtual device's control virtqueue could be very
>>    challenge as we need the support of DMA isolation at queue level
>>
>> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
>> allows the device to implement a single admin control virtqueue to
>> manage the features and attributes for a specific virtual device.
>>
>> The idea is simple, a new virtual device id is introduced on top of
>> the existing virtio_net_ctrl structure. This id is transport or device
>> specific to uniquely identify a management or virtual device.
>>
>> With this, we get a way of using management device (PF) to configure
>> per virtual device features and attributes. And since the admin
>> control virtqueue belongs to management device (PF), the DMA is
>> naturally isolated at device level instead of the queue level for per
>> virtual device control vq.
>>
>> When the admin cvq is offered by management device and normal cvq is
>> offered by virtual device. A new command class is introduced decide
>> whether or not to accept commands from normal cvq for a virtual
>> device.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Changes from V1:
>> - use 'virtual device' instead of 'function'
>> - introuce trust command
>> - clairfy that the admin cvq could be used to configure management
>>    device itself
>> ---
>>   content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 56 insertions(+), 3 deletions(-)
>>
>> diff --git a/content.tex b/content.tex
>> index 620c0e2..989b4f6 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>>   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>>       channel.
>>   
>> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
>> +    available.
>> +
>>   \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>>       value and a type of calculated hash.
>>   
>> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>   \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>>   
>>   The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
>> -negotiated) to send commands to manipulate various features of
>> -the device which would not easily map into the configuration
>> -space.
>> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
>> +commands to manipulate various features of the device which would not
>> +easily map into the configuration space.
>>   
>>   All commands are of the following form:
>> +is not negotiated:
> Stray hunk.


Will fix.


>
>>   
>>   \begin{lstlisting}
>>   struct virtio_net_ctrl {
>> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   do except issue a diagnostic if \field{ack} is not
>>   VIRTIO_NET_OK.
>>   
>> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
>> +negotiated, the driver can use the admin control virtqueue of the
>> +management device to manipulate features of individual virtual devices
>> +where the control virtqueue is not easily implemented. The definition
>> +of management device and virtual device is transport or device
>> +specific. E.g in the case of PCI SR-IOV, the management device is
>> +implemented via the physical function (PF), then the virtual device is
>> +the virtual function (VF) in this case.
> "...if the management device is implemented via the physical function
> (PF), the virtual device is implemented via the virtual function (VF)."
> ?


Yes, looks better.


>
>> +
>> +All commands are of the following form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_net_admin_ctrl {
>> +        u32 virtual_device_id;
>> +        struct virtio_net_ctrl ctrl;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{virtual_device_id} is an unique transport or device specific
>> +identifier for a virtual device or management device. E.g for the case
>> +of PCI SR-IOV, it's the PCI function id. Management device MUST
> "...it is the PCI function id of the virtual function." ?
>
> s/Management device/The management device/


Will fix.


>
>> +reserve 0 for \field{virtual_device_id} to identify itself.
> "reserve", or rather "use"?


Fine.


>
> That also implies that virtual_device_id must be != 0 for virtual
> devices, doesn't it?


Yes, that's for simplicity. I'm not sure how useful it we had multiple 
management device and in that case we need a transport specific way to 
know the id of management device itself.


>
>> +
>>   \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
>>   \label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
>>   
>> @@ -4308,6 +4335,32 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   according to the native endian of the guest rather than
>>   (necessarily when not using the legacy interface) little-endian.
>>   
>> +\paragraph{Setting Trust for Virtual Device}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ is negotiated, the management device
>> +can accept operations via the control vq from the trusted virtual
> "can accept the following operations" ?
>
> (in addition to existing control commands?)


Fine.


>
>> +device.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_NET_ADMIN_CTRL_TRUST 0
>> + #define VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE 0
>> +\end{lstlisting}
>> +
>> +\devicenormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ has been negotiated, the device MUST
> s/If the/If/
>
>> +support VIRTIO_NET_ADMIN_CTRL_TRUST class
> s/support/support the/


Will fix.


>
>> +command. VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE turns trust for the
>> +control virtqueue of a specific virtual device on and off. The command
>> +specific data is one byte containing 0(off) or 1(on).  When trust is
>> +off the device MUST fail any operations from the control virtqueue of
>> +the virtual device. The management device MUST NOT trust any virtual
>> +devices after reset.
> A bit further up, it is stated that the management device can accept
> operations from a "trusted virtual device". From this statement here, I
> get that the trust state can be turned on and off by the virtual device.


There's some misunderstanding that needs to be clarified in the next 
version.

Firstly, for the virtual device, it's not expected to expose an admin 
cvq and the trust state could not be turned on and off by virtual device 
then.

The only want to turn on/off is the admin control vq belongs to 
management device.


>
> Does the management device get to choose if it wants to trust a virtual
> device, i.e. can it fail enabling the trust?


Yes, the idea is to let management device to choose which virtual device 
is trust. For the possible failure, I think we should allow the device 
to fail the enabling, e.g when the device is lacking resources for 
virtual devices (e.g mac address tables etc).

Thanks


>
>> +
>> +\drivernormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +A driver SHOULD negotiate VIRTIO_NET_F_ADMIN_CTRL_VQ if the device
>> +offers it.
>>   
>>   \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>>   Types / Network Device / Legacy Interface: Framing Requirements}


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 V2] virtio-net: introduce admin control virtqueue

[Michael S. Tsirkin]

On Wed, Jan 27, 2021 at 11:30:14AM +0800, Jason Wang wrote:
> > 
> > Does the management device get to choose if it wants to trust a virtual
> > device, i.e. can it fail enabling the trust?
> 
> 
> Yes, the idea is to let management device to choose which virtual device is
> trust. For the possible failure, I think we should allow the device to fail
> the enabling, e.g when the device is lacking resources for virtual devices
> (e.g mac address tables etc).
> 
> Thanks

Wrt SRIOV at least, the standard model is to have the PF control VFs.
I am not saying trusting some VF instead has no uses but
unless someone requested it I'd stick to the standard model
initially.

-- 
MST


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 V2] virtio-net: introduce admin control virtqueue

[Michael S. Tsirkin]

On Mon, Jan 25, 2021 at 01:52:02PM +0800, Jason Wang wrote:
> When implementing virtual devices like SR-IOV or sub-function. We're
> suffering from several issues:
> 
> - There's no management interface for management device to
>   configure features, attributes for a virtual device
> - Per virtual device control virtqueue could be very expensive as the
>   number of virtual devices could be very large
> - Virtualize per virtual device's control virtqueue could be very
>   challenge as we need the support of DMA isolation at queue level
> 
> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
> allows the device to implement a single admin control virtqueue to
> manage the features and attributes for a specific virtual device.
> 
> The idea is simple, a new virtual device id is introduced on top of
> the existing virtio_net_ctrl structure. This id is transport or device
> specific to uniquely identify a management or virtual device.
> 
> With this, we get a way of using management device (PF) to configure
> per virtual device features and attributes. And since the admin
> control virtqueue belongs to management device (PF), the DMA is
> naturally isolated at device level instead of the queue level for per
> virtual device control vq.
> 
> When the admin cvq is offered by management device and normal cvq is
> offered by virtual device. A new command class is introduced decide
> whether or not to accept commands from normal cvq for a virtual
> device.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V1:
> - use 'virtual device' instead of 'function'
> - introuce trust command
> - clairfy that the admin cvq could be used to configure management
>   device itself
> ---
>  content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 620c0e2..989b4f6 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>      channel.
>  
> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
> +    available.
> +
>  \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>      value and a type of calculated hash.
>  
> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>  
>  The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
> -negotiated) to send commands to manipulate various features of
> -the device which would not easily map into the configuration
> -space.
> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
> +commands to manipulate various features of the device which would not
> +easily map into the configuration space.
>  
>  All commands are of the following form:
> +is not negotiated:
>  
>  \begin{lstlisting}
>  struct virtio_net_ctrl {
> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  do except issue a diagnostic if \field{ack} is not
>  VIRTIO_NET_OK.
>  
> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
> +negotiated, the driver can use the admin control virtqueue of the
> +management device to manipulate features of individual virtual devices
> +where the control virtqueue is not easily implemented. The definition
> +of management device and virtual device is transport or device
> +specific. E.g in the case of PCI SR-IOV, the management device is
> +implemented via the physical function (PF), then the virtual device is
> +the virtual function (VF) in this case.
> +
> +All commands are of the following form:
> +
> +\begin{lstlisting}
> +struct virtio_net_admin_ctrl {
> +        u32 virtual_device_id;
> +        struct virtio_net_ctrl ctrl;
> +};
> +\end{lstlisting}
> +
> +The \field{virtual_device_id} is an unique transport or device specific
> +identifier for a virtual device or management device. E.g for the case
> +of PCI SR-IOV, it's the PCI function id. Management device MUST
> +reserve 0 for \field{virtual_device_id} to identify itself.
> +
>  \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
>  \label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
>  
> @@ -4308,6 +4335,32 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  according to the native endian of the guest rather than
>  (necessarily when not using the legacy interface) little-endian.
>  
> +\paragraph{Setting Trust for Virtual Device}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ is negotiated, the management device
> +can accept operations via the control vq from the trusted virtual
> +device.
> +
> +\begin{lstlisting}
> +#define VIRTIO_NET_ADMIN_CTRL_TRUST 0
> + #define VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE 0
> +\end{lstlisting}
> +
> +\devicenormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ has been negotiated, the device MUST
> +support VIRTIO_NET_ADMIN_CTRL_TRUST class
> +command. VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE turns trust for the
> +control virtqueue of a specific virtual device on and off. The command
> +specific data is one byte containing 0(off) or 1(on).  When trust is
> +off the device MUST fail any operations from the control virtqueue of
> +the virtual device. The management device MUST NOT trust any virtual
> +devices after reset.

Looks like trust here refers to ability to send command such as
VIRTIO_NET_CTRL_MAC and VIRTIO_NET_CTRL_VLAN.
However cvq also supports commands such as
VIRTIO_NET_CTRL_MQ which don't seem to require trust.
It looks like breaking VIRTIO_NET_CTRL_ANNOUNCE will also break
migration.

Also, consider VIRTIO_NET_F_CTRL_MAC_ADDR. Isn't it reasonable to
assume setting MAC succeeds with this being available.

Thus, an idea. How about controlling features instead?

And with that, the interface becomes generic and applicable
to all devices not just net ...

> +
> +\drivernormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +A driver SHOULD negotiate VIRTIO_NET_F_ADMIN_CTRL_VQ if the device
> +offers it.
>  \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>  Types / Network Device / Legacy Interface: Framing Requirements}
> -- 
> 2.25.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 V2] virtio-net: introduce admin control virtqueue

[Jason Wang]

On 2021/1/27 下午6:59, Michael S. Tsirkin wrote:
> On Mon, Jan 25, 2021 at 01:52:02PM +0800, Jason Wang wrote:
>> When implementing virtual devices like SR-IOV or sub-function. We're
>> suffering from several issues:
>>
>> - There's no management interface for management device to
>>    configure features, attributes for a virtual device
>> - Per virtual device control virtqueue could be very expensive as the
>>    number of virtual devices could be very large
>> - Virtualize per virtual device's control virtqueue could be very
>>    challenge as we need the support of DMA isolation at queue level
>>
>> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
>> allows the device to implement a single admin control virtqueue to
>> manage the features and attributes for a specific virtual device.
>>
>> The idea is simple, a new virtual device id is introduced on top of
>> the existing virtio_net_ctrl structure. This id is transport or device
>> specific to uniquely identify a management or virtual device.
>>
>> With this, we get a way of using management device (PF) to configure
>> per virtual device features and attributes. And since the admin
>> control virtqueue belongs to management device (PF), the DMA is
>> naturally isolated at device level instead of the queue level for per
>> virtual device control vq.
>>
>> When the admin cvq is offered by management device and normal cvq is
>> offered by virtual device. A new command class is introduced decide
>> whether or not to accept commands from normal cvq for a virtual
>> device.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Changes from V1:
>> - use 'virtual device' instead of 'function'
>> - introuce trust command
>> - clairfy that the admin cvq could be used to configure management
>>    device itself
>> ---
>>   content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 56 insertions(+), 3 deletions(-)
>>
>> diff --git a/content.tex b/content.tex
>> index 620c0e2..989b4f6 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>>   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>>       channel.
>>   
>> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
>> +    available.
>> +
>>   \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>>       value and a type of calculated hash.
>>   
>> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>   \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>>   
>>   The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
>> -negotiated) to send commands to manipulate various features of
>> -the device which would not easily map into the configuration
>> -space.
>> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
>> +commands to manipulate various features of the device which would not
>> +easily map into the configuration space.
>>   
>>   All commands are of the following form:
>> +is not negotiated:
>>   
>>   \begin{lstlisting}
>>   struct virtio_net_ctrl {
>> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   do except issue a diagnostic if \field{ack} is not
>>   VIRTIO_NET_OK.
>>   
>> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
>> +negotiated, the driver can use the admin control virtqueue of the
>> +management device to manipulate features of individual virtual devices
>> +where the control virtqueue is not easily implemented. The definition
>> +of management device and virtual device is transport or device
>> +specific. E.g in the case of PCI SR-IOV, the management device is
>> +implemented via the physical function (PF), then the virtual device is
>> +the virtual function (VF) in this case.
>> +
>> +All commands are of the following form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_net_admin_ctrl {
>> +        u32 virtual_device_id;
>> +        struct virtio_net_ctrl ctrl;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{virtual_device_id} is an unique transport or device specific
>> +identifier for a virtual device or management device. E.g for the case
>> +of PCI SR-IOV, it's the PCI function id. Management device MUST
>> +reserve 0 for \field{virtual_device_id} to identify itself.
>> +
>>   \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
>>   \label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
>>   
>> @@ -4308,6 +4335,32 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   according to the native endian of the guest rather than
>>   (necessarily when not using the legacy interface) little-endian.
>>   
>> +\paragraph{Setting Trust for Virtual Device}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ is negotiated, the management device
>> +can accept operations via the control vq from the trusted virtual
>> +device.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_NET_ADMIN_CTRL_TRUST 0
>> + #define VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE 0
>> +\end{lstlisting}
>> +
>> +\devicenormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ has been negotiated, the device MUST
>> +support VIRTIO_NET_ADMIN_CTRL_TRUST class
>> +command. VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE turns trust for the
>> +control virtqueue of a specific virtual device on and off. The command
>> +specific data is one byte containing 0(off) or 1(on).  When trust is
>> +off the device MUST fail any operations from the control virtqueue of
>> +the virtual device. The management device MUST NOT trust any virtual
>> +devices after reset.
> Looks like trust here refers to ability to send command such as
> VIRTIO_NET_CTRL_MAC and VIRTIO_NET_CTRL_VLAN.
> However cvq also supports commands such as
> VIRTIO_NET_CTRL_MQ which don't seem to require trust.
> It looks like breaking VIRTIO_NET_CTRL_ANNOUNCE will also break
> migration.


Yes, indeed.


>
> Also, consider VIRTIO_NET_F_CTRL_MAC_ADDR. Isn't it reasonable to
> assume setting MAC succeeds with this being available.


AFAIK, for current kernel SRIOV, trust is disabled by default for  a 
specific VF


>
> Thus, an idea. How about controlling features instead?
>
> And with that, the interface becomes generic and applicable
> to all devices not just net ...


Yes, so I think we probably need more than this e.g I had plan to 
introduce a general admin virtqueue which could be use to replace the 
transport specific way to configure/probe virtual devices. I think we 
can add this new features controlling command via that virtqueue.

So if this makes sense. I will drop the trust command in this patch and 
let admin cvq go first. Then I can post general admin virtqueue patch.

Does this make sense?

Thanks


>
>> +
>> +\drivernormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +A driver SHOULD negotiate VIRTIO_NET_F_ADMIN_CTRL_VQ if the device
>> +offers it.
>>   \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>>   Types / Network Device / Legacy Interface: Framing Requirements}
>> -- 
>> 2.25.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 V2] virtio-net: introduce admin control virtqueue

[Cornelia Huck]

On Thu, 28 Jan 2021 10:58:49 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2021/1/27 下午6:59, Michael S. Tsirkin wrote:

> > Thus, an idea. How about controlling features instead?
> >
> > And with that, the interface becomes generic and applicable
> > to all devices not just net ...  
> 
> 
> Yes, so I think we probably need more than this e.g I had plan to 
> introduce a general admin virtqueue which could be use to replace the 
> transport specific way to configure/probe virtual devices. I think we 
> can add this new features controlling command via that virtqueue.
> 
> So if this makes sense. I will drop the trust command in this patch and 
> let admin cvq go first. Then I can post general admin virtqueue patch.
> 
> Does this make sense?

So, you'd introduce a generic admin virtqueue and give individual
device types the possibility to use it for device type specific
commands?


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] [PATCH V2] virtio-net: introduce admin control virtqueue

[Halil Pasic]

On Mon, 25 Jan 2021 13:52:02 +0800
Jason Wang <jasowang@redhat.com> wrote:

> When implementing virtual devices like SR-IOV or sub-function. We're
> suffering from several issues:
> 
> - There's no management interface for management device to
>   configure features, attributes for a virtual device
> - Per virtual device control virtqueue could be very expensive as the
>   number of virtual devices could be very large
> - Virtualize per virtual device's control virtqueue could be very
>   challenge as we need the support of DMA isolation at queue level
> 
> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
> allows the device to implement a single admin control virtqueue to
> manage the features and attributes for a specific virtual device.
> 
> The idea is simple, a new virtual device id is introduced on top of
> the existing virtio_net_ctrl structure. This id is transport or device
> specific to uniquely identify a management or virtual device.
> 
> With this, we get a way of using management device (PF) to configure
> per virtual device features and attributes. And since the admin
> control virtqueue belongs to management device (PF), the DMA is
> naturally isolated at device level instead of the queue level for per
> virtual device control vq.
> 
> When the admin cvq is offered by management device and normal cvq is
> offered by virtual device. A new command class is introduced decide
> whether or not to accept commands from normal cvq for a virtual
> device.
> 

First, sorry for being late. Second, I'm still struggling with putting
my mental model together. I will ask my questions at the correspondig
paragraphs.

> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V1:
> - use 'virtual device' instead of 'function'
> - introuce trust command
> - clairfy that the admin cvq could be used to configure management
>   device itself
> ---
>  content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 620c0e2..989b4f6 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>      channel.
>  
> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
> +    available.
> +
>  \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>      value and a type of calculated hash.
>  
> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>  
>  The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
> -negotiated) to send commands to manipulate various features of
> -the device which would not easily map into the configuration
> -space.
> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
> +commands to manipulate various features of the device which would not
> +easily map into the configuration space.

Let's assume device offers both VIRTIO_NET_F_ADMIN_CTRL_VQ and
VIRTIO_NET_F_CTRL_VQ, but driver accepts only VIRTIO_NET_F_CTRL_VQ,
because it is an old driver that only knows about VIRTIO_NET_F_CTRL_VQ.
What happens then? Do we expect the control queue virtqueue to just
work?

I read the proposal like VIRTIO_NET_F_ADMIN_CTRL_VQ and
VIRTIO_NET_F_CTRL_VQ are mutually exclusive features, and not stacking
features, but my confidence in this regard is very low. If they are
mutually exclusive, we should make the feature bits mutually exclusive
as well.

>  
>  All commands are of the following form:
> +is not negotiated:
>  
>  \begin{lstlisting}
>  struct virtio_net_ctrl {
> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  do except issue a diagnostic if \field{ack} is not
>  VIRTIO_NET_OK.
>  
> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
> +negotiated, the driver can use the admin control virtqueue of the

There is no such thing defined in the spec like 'admin control
virtqueue'. This is the first and only mention. Do you mean the
controlq (as defined in 5.1.2 a.k.a 'control virtqueue')?

> +management device to manipulate features of individual virtual devices
> +where the control virtqueue is not easily implemented. The definition
> +of management device and virtual device is transport or device
> +specific. 

Is the management device a virtio network device that offered the
VIRTIO_NET_F_ADMIN_CTRL_VQ feature bit? If the answer is yes, does
that device otherwise work like any other virtio network device.

> E.g in the case of PCI SR-IOV, the management device is
> +implemented via the physical function (PF), then the virtual device is
> +the virtual function (VF) in this case.
> +

I'm very superficially acquainted with PCI SR-IOV, maybe that's why I
don't understand everything right away. Please bear with me.

> +All commands are of the following form:
> +
> +\begin{lstlisting}
> +struct virtio_net_admin_ctrl {
> +        u32 virtual_device_id;
> +        struct virtio_net_ctrl ctrl;
> +};
> +\end{lstlisting}
> +
> +The \field{virtual_device_id} is an unique transport or device specific
> +identifier for a virtual device or management device. E.g for the case
> +of PCI SR-IOV, it's the PCI function id. Management device MUST
> +reserve 0 for \field{virtual_device_id} to identify itself.
> +

How is a portable driver supposed to obtain this transport or device
specific ID?

Let me elaborate. Let's say I'm a guest and I happen to have a virtio-net
device, which ain't capable doing the usual controlq stuff via the usual
controlq, so I have to use this new mechanism (if I, for example, want to
set the MAC address for it. To do so, I need to know the
virtual_device_id of my virtual virtio-net device, to be able to put it
in my virtio_net_admin_ctl command, and that command I need to put into
some admin control virtqueue.

The paragraph above suggests, that this virtqueue is not the controlq of
my virtual virtio-net device I'm trying to control, but a virtqueue that
belongs to the management device.

Obviously for a non-PCI device, it ain't making no sense to define this
device id as the PCI function id. I guess, this is where the 'transport
specific' comes from. But then shouldn't we define the transport specific
part in a transport specific section?

Also please notice, that the structure virtio_net_admin_ctrl is defined
in the transport agnostic part, that is it has to work for any
transport. Which implies that any other transport must use ids that fit
32 bits. 



>  \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
>  \label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
>  
> @@ -4308,6 +4335,32 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  according to the native endian of the guest rather than
>  (necessarily when not using the legacy interface) little-endian.
>  
> +\paragraph{Setting Trust for Virtual Device}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ is negotiated, the management device
> +can accept operations via the control vq from the trusted virtual
> +device.
> +
> +\begin{lstlisting}
> +#define VIRTIO_NET_ADMIN_CTRL_TRUST 0
> + #define VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE 0
> +\end{lstlisting}
> +
> +\devicenormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ has been negotiated, the device MUST
> +support VIRTIO_NET_ADMIN_CTRL_TRUST class
> +command. 

What does it mean to support the class? I guess it is still allowed
to post VIRTIO_NET_ERR in the ack field, because the device may be
untrusted.

What should the device do when it encounters a not supported class?

> VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE turns trust for the
> +control virtqueue of a specific virtual device on and off. The command
> +specific data is one byte containing 0(off) or 1(on).  When trust is
> +off the device MUST fail any operations from the control virtqueue of
> +the virtual device. The management device MUST NOT trust any virtual
> +devices after reset.

This reads like, the famous admin control virtqueue may actually be the
controlq of the device, that one wants to control, if that device was
previously made trusted by a VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE command.

From the rest of the thread, it appears that we are going to drop the
trust stuff. That makes the question can a trusted virtual device control
another virtual device (by putting not its own id, but the other devices
id into the command) moot.

But it also makes the question how is a guest going to control it's
virtual virtio-net device a more relevant one, because that virtual
device being trusted, and using its controlq is not an option any more.

Regards,
Halil


> +
> +\drivernormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
> +
> +A driver SHOULD negotiate VIRTIO_NET_F_ADMIN_CTRL_VQ if the device
> +offers it.
>  
>  \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>  Types / Network Device / Legacy Interface: Framing Requirements}


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 V2] virtio-net: introduce admin control virtqueue

[Jason Wang]

On 2021/1/29 下午6:57, Cornelia Huck wrote:
> On Thu, 28 Jan 2021 10:58:49 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2021/1/27 下午6:59, Michael S. Tsirkin wrote:
>>> Thus, an idea. How about controlling features instead?
>>>
>>> And with that, the interface becomes generic and applicable
>>> to all devices not just net ...
>>
>> Yes, so I think we probably need more than this e.g I had plan to
>> introduce a general admin virtqueue which could be use to replace the
>> transport specific way to configure/probe virtual devices. I think we
>> can add this new features controlling command via that virtqueue.
>>
>> So if this makes sense. I will drop the trust command in this patch and
>> let admin cvq go first. Then I can post general admin virtqueue patch.
>>
>> Does this make sense?
> So, you'd introduce a generic admin virtqueue and give individual
> device types the possibility to use it for device type specific
> commands?


This is mainly for the trust command. The idea is to have a admin 
virtqueue that is used to manage the features that belongs to a virtual 
device. And in the admin virtqueue, we can introduce a command like:

VIRTIO_ADMIN_CTRL_DEV_FEATURES

that is used to control the features set of a specific virtual device. 
Then there's no need for a specialized trust command in the admin 
control vq for virtio-net.

E.g we can filter out VIRTIO_NET_F_CTRL_MAC_ADDR if we don't want to set 
mac address via control virtqueue, then the virtual device can't see 
this feature via cvq (or admin cvq).

Thanks




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] [PATCH V2] virtio-net: introduce admin control virtqueue

[Jason Wang]

On 2021/1/30 上午6:48, Halil Pasic wrote:
> On Mon, 25 Jan 2021 13:52:02 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> When implementing virtual devices like SR-IOV or sub-function. We're
>> suffering from several issues:
>>
>> - There's no management interface for management device to
>>    configure features, attributes for a virtual device
>> - Per virtual device control virtqueue could be very expensive as the
>>    number of virtual devices could be very large
>> - Virtualize per virtual device's control virtqueue could be very
>>    challenge as we need the support of DMA isolation at queue level
>>
>> So this patch introduces the feature of VIRTIO_NET_CTRL_ADMIN_VQ. This
>> allows the device to implement a single admin control virtqueue to
>> manage the features and attributes for a specific virtual device.
>>
>> The idea is simple, a new virtual device id is introduced on top of
>> the existing virtio_net_ctrl structure. This id is transport or device
>> specific to uniquely identify a management or virtual device.
>>
>> With this, we get a way of using management device (PF) to configure
>> per virtual device features and attributes. And since the admin
>> control virtqueue belongs to management device (PF), the DMA is
>> naturally isolated at device level instead of the queue level for per
>> virtual device control vq.
>>
>> When the admin cvq is offered by management device and normal cvq is
>> offered by virtual device. A new command class is introduced decide
>> whether or not to accept commands from normal cvq for a virtual
>> device.
>>
> First, sorry for being late. Second, I'm still struggling with putting
> my mental model together. I will ask my questions at the correspondig
> paragraphs.


Your feedback is high appreciated.


>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Changes from V1:
>> - use 'virtual device' instead of 'function'
>> - introuce trust command
>> - clairfy that the admin cvq could be used to configure management
>>    device itself
>> ---
>>   content.tex | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 56 insertions(+), 3 deletions(-)
>>
>> diff --git a/content.tex b/content.tex
>> index 620c0e2..989b4f6 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -2940,6 +2940,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>>   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>>       channel.
>>   
>> +\item[VIRTIO_NET_F_ADMIN_CTRL_VQ(56)] Admin control channel is
>> +    available.
>> +
>>   \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet hash
>>       value and a type of calculated hash.
>>   
>> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>   \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>>   
>>   The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
>> -negotiated) to send commands to manipulate various features of
>> -the device which would not easily map into the configuration
>> -space.
>> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
>> +commands to manipulate various features of the device which would not
>> +easily map into the configuration space.
> Let's assume device offers both VIRTIO_NET_F_ADMIN_CTRL_VQ and
> VIRTIO_NET_F_CTRL_VQ, but driver accepts only VIRTIO_NET_F_CTRL_VQ,
> because it is an old driver that only knows about VIRTIO_NET_F_CTRL_VQ.
> What happens then? Do we expect the control queue virtqueue to just
> work?


I think the answer is yes, my understanding is that it's better to 
provide backward compatibility for the old drivers.


>
> I read the proposal like VIRTIO_NET_F_ADMIN_CTRL_VQ and
> VIRTIO_NET_F_CTRL_VQ are mutually exclusive features, and not stacking
> features, but my confidence in this regard is very low. If they are
> mutually exclusive, we should make the feature bits mutually exclusive
> as well.


It works like:

If VIRTIO_NET_F_CTRL_VQ is negotiated but not 
VIRTIO_NET_F_ADMIN_CTRL_VQ, the control virtqueue will use the old 
command format.

If both are negotiated, the control virtqueue will use the new format.


>
>>   
>>   All commands are of the following form:
>> +is not negotiated:
>>   
>>   \begin{lstlisting}
>>   struct virtio_net_ctrl {
>> @@ -3864,6 +3868,29 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   do except issue a diagnostic if \field{ack} is not
>>   VIRTIO_NET_OK.
>>   
>> +When both VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_ADMIN_CTRL_VQ are
>> +negotiated, the driver can use the admin control virtqueue of the
> There is no such thing defined in the spec like 'admin control
> virtqueue'. This is the first and only mention. Do you mean the
> controlq (as defined in 5.1.2 a.k.a 'control virtqueue')?


Kind of, it means the cvq that could be used for manage virtual devices.


>
>> +management device to manipulate features of individual virtual devices
>> +where the control virtqueue is not easily implemented. The definition
>> +of management device and virtual device is transport or device
>> +specific.
> Is the management device a virtio network device that offered the
> VIRTIO_NET_F_ADMIN_CTRL_VQ feature bit? If the answer is yes, does
> that device otherwise work like any other virtio network device.


Good question, I think the answer is yes. This patch basically extends 
the virtio-net cvq API, so the management device should be a virtio-net 
device as well.


>
>> E.g in the case of PCI SR-IOV, the management device is
>> +implemented via the physical function (PF), then the virtual device is
>> +the virtual function (VF) in this case.
>> +
> I'm very superficially acquainted with PCI SR-IOV, maybe that's why I
> don't understand everything right away. Please bear with me.


I agree things will easily be complicated if we talk too much about the 
virtual devices or do you have any suggestions?


>
>> +All commands are of the following form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_net_admin_ctrl {
>> +        u32 virtual_device_id;
>> +        struct virtio_net_ctrl ctrl;
>> +};
>> +\end{lstlisting}
>> +
>> +The \field{virtual_device_id} is an unique transport or device specific
>> +identifier for a virtual device or management device. E.g for the case
>> +of PCI SR-IOV, it's the PCI function id. Management device MUST
>> +reserve 0 for \field{virtual_device_id} to identify itself.
>> +
> How is a portable driver supposed to obtain this transport or device
> specific ID?


Good question. The idea is:

1) if we had transport specific feature like VIRTIO_F_SR_IOV, the id is 
simply PCI function id
2) if virtio support it's own IOV feature (that is mutually exclusive 
with the transport specific one), the id must be obtained via a virtio 
method.

For 2) I plan to introduce a general admin virtqueue that is used for 
virtio specific device IOV. In that implementation, the virtual device 
must be created and destroyed via admin virtqueue. During device 
creation a unique ID is needed then the driver should know/allocate the 
id in advance.


>
> Let me elaborate. Let's say I'm a guest and I happen to have a virtio-net
> device, which ain't capable doing the usual controlq stuff via the usual
> controlq, so I have to use this new mechanism (if I, for example, want to
> set the MAC address for it. To do so, I need to know the
> virtual_device_id of my virtual virtio-net device, to be able to put it
> in my virtio_net_admin_ctl command, and that command I need to put into
> some admin control virtqueue.
>
> The paragraph above suggests, that this virtqueue is not the controlq of
> my virtual virtio-net device I'm trying to control, but a virtqueue that
> belongs to the management device.
>
> Obviously for a non-PCI device, it ain't making no sense to define this
> device id as the PCI function id. I guess, this is where the 'transport
> specific' comes from. But then shouldn't we define the transport specific
> part in a transport specific section?


I think we need keep the general format here but maybe we can add a 
reference to the transport specific part that describe the id in detail.


>
> Also please notice, that the structure virtio_net_admin_ctrl is defined
> in the transport agnostic part, that is it has to work for any
> transport. Which implies that any other transport must use ids that fit
> 32 bits.


Yes, if it's not sufficient, I will increase it to 64 bits.


>
>
>
>>   \paragraph{Packet Receive Filtering}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Packet Receive Filtering}
>>   \label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
>>   
>> @@ -4308,6 +4335,32 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>>   according to the native endian of the guest rather than
>>   (necessarily when not using the legacy interface) little-endian.
>>   
>> +\paragraph{Setting Trust for Virtual Device}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ is negotiated, the management device
>> +can accept operations via the control vq from the trusted virtual
>> +device.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_NET_ADMIN_CTRL_TRUST 0
>> + #define VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE 0
>> +\end{lstlisting}
>> +
>> +\devicenormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +If the VIRTIO_NET_F_ADMIN_CTRL_VQ has been negotiated, the device MUST
>> +support VIRTIO_NET_ADMIN_CTRL_TRUST class
>> +command.
> What does it mean to support the class? I guess it is still allowed
> to post VIRTIO_NET_ERR in the ack field, because the device may be
> untrusted.


It's basically a way to allow or deny the commands that is sent via 
control virtqueue belong to a virtual device.


>
> What should the device do when it encounters a not supported class?


Just fail the command.


>
>> VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE turns trust for the
>> +control virtqueue of a specific virtual device on and off. The command
>> +specific data is one byte containing 0(off) or 1(on).  When trust is
>> +off the device MUST fail any operations from the control virtqueue of
>> +the virtual device. The management device MUST NOT trust any virtual
>> +devices after reset.
> This reads like, the famous admin control virtqueue may actually be the
> controlq of the device, that one wants to control, if that device was
> previously made trusted by a VIRTIO_NET_ADMIN_CTRL_TRUST_ENABLE command.
>
>  From the rest of the thread, it appears that we are going to drop the
> trust stuff. That makes the question can a trusted virtual device control
> another virtual device (by putting not its own id, but the other devices
> id into the command) moot.


It looks like a nesting somehow, but I think we should not allow to 
export admin control virtqueue for a non management device and there 
will be a single management device for a specific virtual device.


>
> But it also makes the question how is a guest going to control it's
> virtual virtio-net device a more relevant one, because that virtual
> device being trusted, and using its controlq is not an option any more.


So in an ideal case, when we had admin cvq, it's better not offer a cvq 
for a virtual device. In this case, the cvq of this virtual device is 
mediated by management device(drivers). So we don't need command like 
trust since the command could be failed by the driver of management 
device easily.

But I'm not sure how hard (or whether or not it's too late to forbid cvq 
of a virtual device if admin cvq is offered by the device). So the trust 
stuffs is basically for this case: virtual device has cvq, mgmt device 
has admin cvq.

Thanks


>
> Regards,
> Halil
>
>
>> +
>> +\drivernormative{\subparagraph}{Setting Trust for Virtual Device}{Device Types / Network Device / Device Operation / Control Virtqueue / Setting Trust for Virtual Device}
>> +
>> +A driver SHOULD negotiate VIRTIO_NET_F_ADMIN_CTRL_VQ if the device
>> +offers it.
>>   
>>   \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>>   Types / Network Device / Legacy Interface: Framing Requirements}


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] [PATCH V2] virtio-net: introduce admin control virtqueue

[Cornelia Huck]

On Mon, 1 Feb 2021 12:09:15 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2021/1/30 上午6:48, Halil Pasic wrote:
> > On Mon, 25 Jan 2021 13:52:02 +0800
> > Jason Wang <jasowang@redhat.com> wrote:

> >> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> >>   \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
> >>   
> >>   The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
> >> -negotiated) to send commands to manipulate various features of
> >> -the device which would not easily map into the configuration
> >> -space.
> >> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
> >> +commands to manipulate various features of the device which would not
> >> +easily map into the configuration space.  
> > Let's assume device offers both VIRTIO_NET_F_ADMIN_CTRL_VQ and
> > VIRTIO_NET_F_CTRL_VQ, but driver accepts only VIRTIO_NET_F_CTRL_VQ,
> > because it is an old driver that only knows about VIRTIO_NET_F_CTRL_VQ.
> > What happens then? Do we expect the control queue virtqueue to just
> > work?  
> 
> 
> I think the answer is yes, my understanding is that it's better to 
> provide backward compatibility for the old drivers.

I agree, I'd expect the device to just use the CTRL_VQ semantics, or
fail feature negotiation.

> 
> 
> >
> > I read the proposal like VIRTIO_NET_F_ADMIN_CTRL_VQ and
> > VIRTIO_NET_F_CTRL_VQ are mutually exclusive features, and not stacking
> > features, but my confidence in this regard is very low. If they are
> > mutually exclusive, we should make the feature bits mutually exclusive
> > as well.  
> 
> 
> It works like:
> 
> If VIRTIO_NET_F_CTRL_VQ is negotiated but not 
> VIRTIO_NET_F_ADMIN_CTRL_VQ, the control virtqueue will use the old 
> command format.
> 
> If both are negotiated, the control virtqueue will use the new format.

What if ADMIN_CTRL_VQ is negotiated, but not CTRL_VQ? I assume that
this is an invalid combination that should not be accepted?

(...)

> >> +All commands are of the following form:
> >> +
> >> +\begin{lstlisting}
> >> +struct virtio_net_admin_ctrl {
> >> +        u32 virtual_device_id;
> >> +        struct virtio_net_ctrl ctrl;
> >> +};
> >> +\end{lstlisting}
> >> +
> >> +The \field{virtual_device_id} is an unique transport or device specific
> >> +identifier for a virtual device or management device. E.g for the case
> >> +of PCI SR-IOV, it's the PCI function id. Management device MUST
> >> +reserve 0 for \field{virtual_device_id} to identify itself.
> >> +  
> > How is a portable driver supposed to obtain this transport or device
> > specific ID?  
> 
> 
> Good question. The idea is:
> 
> 1) if we had transport specific feature like VIRTIO_F_SR_IOV, the id is 
> simply PCI function id
> 2) if virtio support it's own IOV feature (that is mutually exclusive 
> with the transport specific one), the id must be obtained via a virtio 
> method.
> 
> For 2) I plan to introduce a general admin virtqueue that is used for 
> virtio specific device IOV. In that implementation, the virtual device 
> must be created and destroyed via admin virtqueue. During device 
> creation a unique ID is needed then the driver should know/allocate the 
> id in advance.
> 
> 
> >
> > Let me elaborate. Let's say I'm a guest and I happen to have a virtio-net
> > device, which ain't capable doing the usual controlq stuff via the usual
> > controlq, so I have to use this new mechanism (if I, for example, want to
> > set the MAC address for it. To do so, I need to know the
> > virtual_device_id of my virtual virtio-net device, to be able to put it
> > in my virtio_net_admin_ctl command, and that command I need to put into
> > some admin control virtqueue.
> >
> > The paragraph above suggests, that this virtqueue is not the controlq of
> > my virtual virtio-net device I'm trying to control, but a virtqueue that
> > belongs to the management device.
> >
> > Obviously for a non-PCI device, it ain't making no sense to define this
> > device id as the PCI function id. I guess, this is where the 'transport
> > specific' comes from. But then shouldn't we define the transport specific
> > part in a transport specific section?  
> 
> 
> I think we need keep the general format here but maybe we can add a 
> reference to the transport specific part that describe the id in detail.
> 
> 
> >
> > Also please notice, that the structure virtio_net_admin_ctrl is defined
> > in the transport agnostic part, that is it has to work for any
> > transport. Which implies that any other transport must use ids that fit
> > 32 bits.  
> 
> 
> Yes, if it's not sufficient, I will increase it to 64 bits.

Should we maybe use the generic uuid format to identify the device in
the commands?


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 V2] virtio-net: introduce admin control virtqueue

[Cornelia Huck]

On Mon, 1 Feb 2021 11:42:31 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2021/1/29 下午6:57, Cornelia Huck wrote:
> > On Thu, 28 Jan 2021 10:58:49 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >  
> >> On 2021/1/27 下午6:59, Michael S. Tsirkin wrote:  
> >>> Thus, an idea. How about controlling features instead?
> >>>
> >>> And with that, the interface becomes generic and applicable
> >>> to all devices not just net ...  
> >>
> >> Yes, so I think we probably need more than this e.g I had plan to
> >> introduce a general admin virtqueue which could be use to replace the
> >> transport specific way to configure/probe virtual devices. I think we
> >> can add this new features controlling command via that virtqueue.
> >>
> >> So if this makes sense. I will drop the trust command in this patch and
> >> let admin cvq go first. Then I can post general admin virtqueue patch.
> >>
> >> Does this make sense?  
> > So, you'd introduce a generic admin virtqueue and give individual
> > device types the possibility to use it for device type specific
> > commands?  
> 
> 
> This is mainly for the trust command. The idea is to have a admin 
> virtqueue that is used to manage the features that belongs to a virtual 
> device. And in the admin virtqueue, we can introduce a command like:
> 
> VIRTIO_ADMIN_CTRL_DEV_FEATURES
> 
> that is used to control the features set of a specific virtual device. 
> Then there's no need for a specialized trust command in the admin 
> control vq for virtio-net.
> 
> E.g we can filter out VIRTIO_NET_F_CTRL_MAC_ADDR if we don't want to set 
> mac address via control virtqueue, then the virtual device can't see 
> this feature via cvq (or admin cvq).

Ok, I think I have to see the result, but it seems workable.


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] [PATCH V2] virtio-net: introduce admin control virtqueue

[Jason Wang]

On 2021/2/1 下午11:49, Cornelia Huck wrote:
> On Mon, 1 Feb 2021 12:09:15 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2021/1/30 上午6:48, Halil Pasic wrote:
>>> On Mon, 25 Jan 2021 13:52:02 +0800
>>> Jason Wang <jasowang@redhat.com> wrote:
>>>> @@ -3840,11 +3843,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>>>>    \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue}
>>>>    
>>>>    The driver uses the control virtqueue (if VIRTIO_NET_F_CTRL_VQ is
>>>> -negotiated) to send commands to manipulate various features of
>>>> -the device which would not easily map into the configuration
>>>> -space.
>>>> +negotiated but VIRTIO_NET_F_ADMIN_CTRL_VQ is not negotiated) to send
>>>> +commands to manipulate various features of the device which would not
>>>> +easily map into the configuration space.
>>> Let's assume device offers both VIRTIO_NET_F_ADMIN_CTRL_VQ and
>>> VIRTIO_NET_F_CTRL_VQ, but driver accepts only VIRTIO_NET_F_CTRL_VQ,
>>> because it is an old driver that only knows about VIRTIO_NET_F_CTRL_VQ.
>>> What happens then? Do we expect the control queue virtqueue to just
>>> work?
>>
>> I think the answer is yes, my understanding is that it's better to
>> provide backward compatibility for the old drivers.
> I agree, I'd expect the device to just use the CTRL_VQ semantics, or
> fail feature negotiation.
>
>>
>>> I read the proposal like VIRTIO_NET_F_ADMIN_CTRL_VQ and
>>> VIRTIO_NET_F_CTRL_VQ are mutually exclusive features, and not stacking
>>> features, but my confidence in this regard is very low. If they are
>>> mutually exclusive, we should make the feature bits mutually exclusive
>>> as well.
>>
>> It works like:
>>
>> If VIRTIO_NET_F_CTRL_VQ is negotiated but not
>> VIRTIO_NET_F_ADMIN_CTRL_VQ, the control virtqueue will use the old
>> command format.
>>
>> If both are negotiated, the control virtqueue will use the new format.
> What if ADMIN_CTRL_VQ is negotiated, but not CTRL_VQ? I assume that
> this is an invalid combination that should not be accepted?


Yes, I will clarify this in the next version.


>
> (...)
>
>>>> +All commands are of the following form:
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_net_admin_ctrl {
>>>> +        u32 virtual_device_id;
>>>> +        struct virtio_net_ctrl ctrl;
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +The \field{virtual_device_id} is an unique transport or device specific
>>>> +identifier for a virtual device or management device. E.g for the case
>>>> +of PCI SR-IOV, it's the PCI function id. Management device MUST
>>>> +reserve 0 for \field{virtual_device_id} to identify itself.
>>>> +
>>> How is a portable driver supposed to obtain this transport or device
>>> specific ID?
>>
>> Good question. The idea is:
>>
>> 1) if we had transport specific feature like VIRTIO_F_SR_IOV, the id is
>> simply PCI function id
>> 2) if virtio support it's own IOV feature (that is mutually exclusive
>> with the transport specific one), the id must be obtained via a virtio
>> method.
>>
>> For 2) I plan to introduce a general admin virtqueue that is used for
>> virtio specific device IOV. In that implementation, the virtual device
>> must be created and destroyed via admin virtqueue. During device
>> creation a unique ID is needed then the driver should know/allocate the
>> id in advance.
>>
>>
>>> Let me elaborate. Let's say I'm a guest and I happen to have a virtio-net
>>> device, which ain't capable doing the usual controlq stuff via the usual
>>> controlq, so I have to use this new mechanism (if I, for example, want to
>>> set the MAC address for it. To do so, I need to know the
>>> virtual_device_id of my virtual virtio-net device, to be able to put it
>>> in my virtio_net_admin_ctl command, and that command I need to put into
>>> some admin control virtqueue.
>>>
>>> The paragraph above suggests, that this virtqueue is not the controlq of
>>> my virtual virtio-net device I'm trying to control, but a virtqueue that
>>> belongs to the management device.
>>>
>>> Obviously for a non-PCI device, it ain't making no sense to define this
>>> device id as the PCI function id. I guess, this is where the 'transport
>>> specific' comes from. But then shouldn't we define the transport specific
>>> part in a transport specific section?
>>
>> I think we need keep the general format here but maybe we can add a
>> reference to the transport specific part that describe the id in detail.
>>
>>
>>> Also please notice, that the structure virtio_net_admin_ctrl is defined
>>> in the transport agnostic part, that is it has to work for any
>>> transport. Which implies that any other transport must use ids that fit
>>> 32 bits.
>>
>> Yes, if it's not sufficient, I will increase it to 64 bits.
> Should we maybe use the generic uuid format to identify the device in
> the commands?


Right, will do.

Thanks


>
> 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/
>


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/