[virtio-comment] [PATCH v7 0/5] virtio-net: Support flow filter for receive packets

[virtio-comment] [PATCH v7 0/5] virtio-net: Support flow filter for receive packets

[Parav Pandit]

Summary:
========
This series improves virtio net receive packet steering to forward/steer
packets to specific RQ.

This basic functionality will enable Linux ethtool steering, Accelerated
receive flow steering (ARFS) as starting point, and more use cases in
future.

Problem statement:
==================
Currently packet allow/drop interface has few limitations.

1. Driver cannot add or delete an individual entry for mac and vlan.
2. Driver cannot select mac+vlan combination for which
    to allow/drop packet.
3. Driver cannot not set other commonly used packet match fields
    such as IP header fields, TCP, UDP, SCP header fields.
4. Driver cannot steer specific packets based on the match
   fields to specific receiveq.
5. Driver do not have multiple or dedicated virtqueues to
    perform flow filter requests in accelerated manner in
    the device.

Solution:
=========
Flow filter as a generic framework to overcome above limitations.

Overview:
=========
A flow filter defines the flow based on one or more match fields
of the packet, defines an action like drop/forward to RQ.

The flow filters are organized in flow filter groups so that their
processing can be ordered when multiple applications wants to use it.

Flow filters requests can be transported via control vq or dedicated
flow filter virtqueue so that it does not get intermixed with other
slow operations of cvq.

Flow filter requirements addressed by this series is worked by virtio
community at [1].

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179

It uses updated control vq command format from [2].

[1] https://lists.oasis-open.org/archives/virtio-comment/202308/msg00263.html
[2] https://lists.oasis-open.org/archives/virtio-comment/202310/msg00047.html

Patch summary:
==============
patch-1 adds theory of operation description for flow filter
patch-2 adds device capabilities cvq commands
patch-3 adds group add/delete commands
patch-4 adds flow filter match key, action and requests to transport via vq
patch-5 adds device and driver requirements

Please review.

Changelog:
==========
v6->v7:
- fixed plenty of grammar suggestions from Cornelia
- had to drop Michael's suggestion on placing things in net config space
  because:
  a. its huge in size
  b. different across member devices
  c. creating new dma interface adds more registers and duplicates functionality
     of cvq
  d. existing cvq provides single comm channel for get and set all rarely accessed
     config attributes and capabilities, provides single place for device implmentation
     to to get/set/query
  e. matches with recent net statistics cvq commands
v5->v6:
- pick next unique bit 65 as to avoid conflict with rss context feature
- fixed missing conformance links
- removed white spaces at end of line
v3->v5:
- removed left over dependencies of flow filter virtqueues
- removed partial sentence
v2->v3:
- removed dependency on dynamic queue infrastucture which is
  not yet ready
v1->v2:
- addressed comments from Satananda
- squashed with match fields definition patch of v1
- added length to the flexible array definition struct to benefit
  from future runtime length bound checkers listed in
  https://people.kernel.org/kees/bounded-flexible-arrays-in-c
- renamed value to key
- addressed comments from Satananda
- merged destination and action to one struct
- added vlan type match field
- kept space for types between l2, l3, l4 header match types
- renamed mask to mask_supported with shorter width
- made more fields reserved for future
- addressed comments from Heng
- grammar correction
- added field to indicate supported number of actions per flow
  filter match entry
- added missing documentation for max_flow_priorities_per_group
- fixed comments from Heng
- grammar corrections
- spelling corrections
- fixed spelling from initializaton to initialization
- added more requirements for multiple actions

v0->v1:
- addressed comments from Satananda
- added device requirement to return non zero value in fields_bmap
- added device requirement to not repeat filter type in response
- added driver requirement to order filter match field as it
  appears in the packet
- added device requirement to fail group delete command on existing
  flow entries
- added mask field in the type to indicate supported mask by device
  and also in later patch to use it to indicate mask on adding
  flow filter. As a result removed the mask_supported capability
  field

Parav Pandit (5):
  virtio-net: Add theory of operation for flow filter
  virtio-net: Add flow filter capabilities read commands
  virtio-net: Add flow filter group life cycle commands
  virtio-net: Add flow filter match entry, action and requests
  virtio-net: Add flow filter device and driver requirements

 device-types/net/description.tex        | 631 ++++++++++++++++++++++++
 device-types/net/device-conformance.tex |   1 +
 device-types/net/driver-conformance.tex |   1 +
 3 files changed, 633 insertions(+)

-- 
2.34.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] [PATCH v7 1/5] virtio-net: Add theory of operation for flow filter

[Parav Pandit]

Currently packet allow/drop interface has following limitations.

1. Driver can either select which MAC and VLANs to consider
for allowing/dropping packets, here, the driver has a
limitation that driver needs to supply full mac
table or full vlan table for each type. Driver cannot add or
delete an individual entry.

2. Driver cannot select mac+vlan combination for which
to allow/drop packet.

3. Driver cannot not set other commonly used packet match fields
such as IP header fields, TCP, UDP, SCP header fields.

4. Driver cannot steer specific packets based on the match
fields to specific receiveq.

5. Driver do not have multiple or dedicated virtqueues to
   perform flow filter requests in accelerated manner in
   the device.

Flow filter as a generic framework overcome above limitations.

As starting point it is useful to support at least two use cases.
a. ARFS
b. ethtool ntuple steering

In future it can be further extended for usecases such as
switching device, connection tracking or may be more.

The flow filter has following properties.

1. It is an extendible object that driver can add or delete.
2. It belongs to a flow filter group (group has priority).
3. Each flow filter is identified using a unique identifier(id),
   has priority, match fields, destination(rq) and action(allow/drop).
4. Flow filter has optionally mask too.

This patch adds theory of operation for flow filter functionality.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v6->v7:
- addressed comments from Cornelia
- plenty of grammar corrections suggested by Cornelia
- removed stale reference to flow filter virtqueue
- removed incorrect stale hunk of a label
v4->v5:
- to avoid feature bit overlap with rss context patch, pick next
  unique bit 65
v3->v4:
- removed flow filter virtqueue section as dynamic queues are
  not supported currently
v2->v3:
- removed dependency on the dynamic queue creation as the
  infrastructure is not yet ready
v1->v2:
- fixed comments from Heng
- grammar corrections
- spelling corrections
---
 device-types/net/description.tex | 78 ++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index aff5e08..03909ae 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -122,6 +122,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
     device with the same MAC address.
 
 \item[VIRTIO_NET_F_SPEED_DUPLEX(63)] Device reports speed and duplex.
+
+\item[VIRTIO_NET_F_FLOW_FILTER(65)] Device supports flow filter requests.
 \end{description}
 
 \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
@@ -153,6 +155,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
 \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
 \item[VIRTIO_NET_F_VQ_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
 \item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
+\item[VIRTIO_NET_F_FLOW_FILTER] Requires VIRTIO_NET_F_CTRL_VQ.
 \end{description}
 
 \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
@@ -1144,6 +1147,81 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 #define VIRTIO_NET_HASH_REPORT_UDPv6_EX        9
 \end{lstlisting}
 
+\subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Operation / Flow Filter}
+
+To forward a received packet to a specific receiveq or to drop the packet based
+on one or more fields of the packet, the device supports flow filter
+functionality. The flow filter can match the packet for a flow, take an action
+such as forward the packet to the specific receiveq or drop the packet.
+For example, the driver can request the device to forward received packets
+which match to a specific source and destination IP addresses and
+TCP ports to a specific receiveq.
+
+Each flow filter consists of one or more match keys, a flow filter priority,
+a flow filter identifier, an action to forward a packet to the destination
+or to drop the packet.
+
+The match fields also optionally consist of a match mask. When a mask is
+specified for the flow filter, first the packet fields are masked before
+matching with the fields of the flow filter.
+
+Each flow filter is independent of each other. The driver can add, replace
+and delete a flow filter in the device using a flow filter request.
+
+The device indicates the flow filter capabilities to the driver. These
+capabilities include various maximum device limits and
+supported packet match fields.
+
+The flow filters are grouped using a flow filter group. Each flow filter
+group has a priority. The device first applies the flow filters of the highest
+priority group to the received packet. If there is no match for the
+flow filters of such a group for the packet, the flow filters of the next
+priority group are applied, until there is a match for the packet from such
+a group or the last group has reached.
+
+A flow filter group can have one or more flow filters. Within a flow
+filter group, a packet may match multiple flow filters. In such a scenario,
+of the matching flow filters the one with the highest priority is applied,
+skipping any matching filters with a lower priority; if there is no match,
+the next higher priority flow filter is applied.
+
+\paragraph{Packet Processing Order}\label{sec:sec:Device Types / Network Device / Device Operation / Flow Filter / Packet Processing Order}
+
+If enabled, filtering and steering functionalities are applied to the received
+packet in the following order:
+
+\begin{itemize}
+\item apply device configuration done using control virtqueue commands
+      VIRTIO_NET_CTRL_RX, VIRTIO_NET_CTRL_MAC and VIRTIO_NET_CTRL_VLAN.
+\item apply flow filter configuration done using flow filter requests.
+\item apply device configuration done using command
+      VIRTIO_NET_CTRL_MQ_RSS_CONFIG.
+\end{itemize}
+
+While processing a received packet, if the packet is dropped at any stage,
+the following levels of processing is omitted.
+
+If a flow filter is matched for the packet, the following levels of processing
+are omitted as well.
+
+A few examples are:
+\begin{itemize}
+\item If the packet is dropped by the flow filter configuration, RSS
+      configuration is not applied to such a packet.
+\item If the packet is forwarded to a specific receiveq using flow filters,
+      RSS configuration is not applied to such a packet due to a match on the
+      flow filter request.
+\item If the packet is dropped due to VIRTIO_NET_CTRL_MAC configuration,
+      neither flow filters nor RSS configuration are applied to such a packet.
+\item If the packet does not find any match in any of the flow filter groups,
+      next level RSS device configuration is applied if its exists.
+\item If there are three flow filter groups configured as group_A, group_B
+      and group_C with respective priorities as 4, 5, and 6; flow filters of
+      group_C is applied first having highest group priority, if there is a match,
+      the flow filters of group_B and group_A are skipped; if there is no match for
+      the flow filters in group_C, the flow filters of next level group_B are applied.
+\end{itemize}
+
 \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
-- 
2.34.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] [PATCH v7 2/5] virtio-net: Add flow filter capabilities read commands

[Parav Pandit]

The device responds flow filter capabilities using two commands.
One command indicates generic flow filter device limits such as
number of flow filters, number of flow filter groups, support or
multiple transports etc.

The second command indicates supported match types, and fields
of the packet.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v6->v7:
- plenty of grammar corrections suggested by Cornelia
v2->v3:
- rebased on virtio-1.4 branch
- removed reference for flow filter virtqueue
v1->v2:
- addressed comments from Satananda
- added vlan type match field
- kept space for types between l2, l3, l4 header match types
- renamed mask to mask_supported with shorter width
- made more fields reserved for future
- addressed comments from Heng
- grammar correction
- added field to indicate supported number of actions per flow
  filter match entry
- added missing documentation for max_flow_priorities_per_group
v0->v1:
- added mask field in the type to indicate supported mask by device
  and also in later patch to use it to indicate mask on adding
  flow filter. As a result removed the mask_supported capability
  field
---
 device-types/net/description.tex | 207 ++++++++++++++++++++++++++++++-
 1 file changed, 206 insertions(+), 1 deletion(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 03909ae..10d92d9 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -1170,7 +1170,11 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
 
 The device indicates the flow filter capabilities to the driver. These
 capabilities include various maximum device limits and
-supported packet match fields.
+supported packet match fields. These control virtqueue
+commands are:
+\ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Capabilities Get}
+and
+\ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Match Capabilities Get}.
 
 The flow filters are grouped using a flow filter group. Each flow filter
 group has a priority. The device first applies the flow filters of the highest
@@ -1222,6 +1226,136 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
       the flow filters in group_C, the flow filters of next level group_B are applied.
 \end{itemize}
 
+\paragraph{Match Types and Fields}\label{sec:Device Types / Network Device / Device Operation / Flow Filter / Match Types and Fields}
+
+\begin{lstlisting}
+struct virtio_net_ff_match_type_cap {
+        le16 type;
+        u8 mask_supported;
+        u8 reserved[5];
+        le64 fields_bmap;
+};
+\end{lstlisting}
+
+The \field{type} corresponds to following table:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Type & Name & Description \\
+\hline \hline
+0   & VIRTIO_NET_FF_ETH_HDR & Ethernet header of the packet \\
+\hline
+0x1   & VIRTIO_NET_FF_VLAN_TAG_HDR & VLAN tag of the packet \\
+\hline
+0x200   & VIRTIO_NET_FF_IPV4_HDR & IPv4 header of the packet \\
+\hline
+0x300   & VIRTIO_NET_FF_IPV6_HDR & IPv6 header of the packet \\
+\hline
+0x400   & VIRTIO_NET_FF_TCP_HDR & TCP header of the packet \\
+\hline
+0x500   & VIRTIO_NET_FF_UDP_HDR & UDP header of the packet \\
+\hline
+other   & -    & reserved \\
+\hline
+\end{tabular}
+
+When \field{mask_supported} is set, for the specific \field{type}, the
+device can mask packet fields with the mask supplied in the flow
+filter match entry.
+
+For each \field{type} the \field{fields_bmap} indicates supported fields
+of the packet header which can be matched.
+
+For the \field{type} of VIRTIO_NET_FF_ETH_HDR, header fields
+are represented by a bitmap in \field{fields_bmap} as follows:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Bit & Name & Description \\
+\hline \hline
+0   & VIRTIO_NET_FF_DST_MAC & Destination MAC address in the packet \\
+\hline
+1   & VIRTIO_NET_FF_SRC_MAC & Source MAC address in the packet \\
+\hline
+2   & VIRTIO_NET_FF_ETHER_TYPE & Ether type in the packet \\
+\hline
+other   & -    & reserved \\
+\hline
+\end{tabular}
+
+For the \field{type} of VIRTIO_NET_FF_VLAN_TAG_HDR, VLAN tag fields
+are represented by a bitmap in \field{fields_bmap} as follows:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Bit & Name & Description \\
+\hline \hline
+0   & VIRTIO_NET_FF_VLAN_TAG_TCI & Vlan tag TCI 16-bit field \\
+\hline
+other   & -    & reserved \\
+\hline
+\end{tabular}
+
+For the \field{type} of VIRTIO_NET_FF_IPV4_HDR, header fields
+are represented by a bitmap in \field{fields_bmap} as follows:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Bit & Name & Description \\
+\hline \hline
+0   & VIRTIO_NET_FF_SRC_IPV4 & Source IPV4 address in the packet \\
+\hline
+1   & VIRTIO_NET_FF_DST_IPV4 & Destination IPV4 address in the packet \\
+\hline
+other   & -    & reserved \\
+\hline
+\end{tabular}
+
+For the \field{type} of VIRTIO_NET_FF_IPV6_HDR, header fields
+are represented by a bitmap in \field{fields_bmap} as follows:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Bit & Name & Description \\
+\hline \hline
+0   & VIRTIO_NET_FF_SRC_IPV6 & Source IPV6 address in the packet \\
+\hline
+1   & VIRTIO_NET_FF_DST_IPV6 & Destination IPV6 address in the packet \\
+\hline
+other   & -    & reserved \\
+\hline
+\end{tabular}
+
+For the \field{type} of VIRTIO_NET_FF_TCP_HDR, header fields
+are represented by a bitmap in \field{fields_bmap} as follows:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Bit & Name & Description \\
+\hline \hline
+0   & VIRTIO_NET_FF_SRC_TCP_PORT & Source TCP port in the packet \\
+\hline
+1   & VIRTIO_NET_FF_DST_TCP_PORT & Destination TCP port in the packet \\
+\hline
+other   & -    & reserved \\
+\hline
+\end{tabular}
+
+For the \field{type} of VIRTIO_NET_FF_UDP_HDR, header fields
+are represented by a bitmap in \field{fields_bmap} as follows:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Bit & Name & Description \\
+\hline \hline
+0   & VIRTIO_NET_FF_SRC_UDP_PORT & Source UDP port in the packet \\
+\hline
+1   & VIRTIO_NET_FF_DST_UDP_PORT & Destination UDP port in the packet \\
+\hline
+other   & -    & reserved  \\
+\hline
+\end{tabular}
+
 \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
@@ -2389,6 +2523,77 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 of the driver's records. In such cases, the driver should allocate additional
 space for the \field{command-specific-result} buffer.
 
+\paragraph{Flow Filter}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter}
+
+If the VIRTIO_NET_F_FLOW_FILTER feature is negotiated,
+
+\begin{itemize}
+\item the driver can send commands VIRTIO_NET_CTRL_FF_CAP_GET and
+VIRTIO_NET_CTRL_FF_MATCH_CAP_GET to query the flow filter
+capabilities of the device.
+\end{itemize}
+
+\begin{lstlisting}
+#define VIRTIO_NET_CTRL_FF 7
+ #define VIRTIO_NET_CTRL_FF_CAP_GET 0
+ #define VIRTIO_NET_CTRL_FF_MATCH_CAP_GET 1
+\end{lstlisting}
+
+\subparagraph{Flow Filter Capabilities Get}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Capabilities Get}
+
+The command VIRTIO_NET_CTRL_FF_CAP_GET provides the flow filter device capabilities.
+
+\begin{lstlisting}
+struct virtio_net_ctrl_ff_caps {
+        le16 max_match_fields;
+        le16 max_groups; /* valid group id = max_groups - 1 */
+        le32 max_ff_per_group;
+        le32 max_ff; /* max flow_id in add/del = max_ff - 1 */
+        le16 max_actions;
+        u8 max_flow_priorities_per_group;
+};
+\end{lstlisting}
+
+\field{max_groups} indicates total number of flow filter groups supported
+by the device whose group identifiers can be any value in the range from 0 to
+\field{max_groups - 1}. The flow filter group can have any priority in range
+of 0 to \field{max_groups - 1}.
+
+\field{max_ff_per_group} indicates the maximum number of
+flow filters per flow filter group which can be added by the driver.
+
+\field{max_ff} indicates the maximum number of flow filters across
+all the flow groups which can be added by the driver.
+
+\field{max_ff_priorities_per_group} indicates the maximum priority value
+of a flow filter within a group. A flow filter within a group can have any
+priority in range of zero to \field{max_ff_priorities_per_group - 1}.
+
+\field{max_match_fields} indicates maximum number of fields of a packet
+which can be matched by the device for a flow filter.
+
+\field{max_actions} indicates maximum number of actions for a flow filter
+that can be supplied.
+
+\field{max_flow_priorities_per_group} indicates maximum number of
+priorities supported by the device per flow filter group.
+
+\subparagraph{Flow Filter Match Capabilities Get}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Match Capabilities Get}
+
+The command VIRTIO_NET_CTRL_FF_MATCH_CAP_GET indicates which fields
+of the packet can be matched.
+
+\begin{lstlisting}
+struct virtio_net_ctrl_ff_match_types {
+        le32 num_entries;
+        struct virtio_net_ff_match_type_cap types[];
+};
+\end{lstlisting}
+
+\field{num_entries} indicates the length of the array \field{types}.
+Each array entry of \field{types} represents the fields of the packet
+which are supported for matching by the device.
+
 \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 Types / Network Device / Legacy Interface: Framing Requirements}
 
-- 
2.34.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] [PATCH v7 3/5] virtio-net: Add flow filter group life cycle commands

[Parav Pandit]

All the flow filters are managed in the flow filter group. The
device can have one or more flow filter groups. Each flow filter
group has its own priority. The group priority which
defines the packet processing order in the flow filter domain.

Add commands to add and delete the flow filter group.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
Signed-off-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>

---
changelog:
v6->v7:
- addressed comments from Cornelia
- reworded the description for group delete command
---
 device-types/net/description.tex | 35 ++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 10d92d9..e766b12 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -1188,6 +1188,11 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
 of the matching flow filters the one with the highest priority is applied,
 skipping any matching filters with a lower priority; if there is no match,
 the next higher priority flow filter is applied.
+The driver adds and deletes the flow filter group using a control
+virtqueue commands \ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Group Add}
+and
+\ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Group Delete}
+respectively.
 
 \paragraph{Packet Processing Order}\label{sec:sec:Device Types / Network Device / Device Operation / Flow Filter / Packet Processing Order}
 
@@ -2531,12 +2536,17 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \item the driver can send commands VIRTIO_NET_CTRL_FF_CAP_GET and
 VIRTIO_NET_CTRL_FF_MATCH_CAP_GET to query the flow filter
 capabilities of the device.
+\item the driver can send commands VIRTIO_NET_CTRL_FF_GROUP_ADD
+and VIRTIO_NET_CTRL_FF_GROUP_DEL for adding and deleting the
+flow group.
 \end{itemize}
 
 \begin{lstlisting}
 #define VIRTIO_NET_CTRL_FF 7
  #define VIRTIO_NET_CTRL_FF_CAP_GET 0
  #define VIRTIO_NET_CTRL_FF_MATCH_CAP_GET 1
+ #define VIRTIO_NET_CTRL_FF_GROUP_ADD 2
+ #define VIRTIO_NET_CTRL_FF_GROUP_DEL 3
 \end{lstlisting}
 
 \subparagraph{Flow Filter Capabilities Get}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Capabilities Get}
@@ -2594,6 +2604,31 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 Each array entry of \field{types} represents the fields of the packet
 which are supported for matching by the device.
 
+\subparagraph{Flow Filter Group Add}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Group Add}
+
+The command VIRTIO_NET_CTRL_FF_GROUP_ADD adds a new flow filter
+group with the supplied group identifier \field{id} with assigned
+priority \field{priority}.
+
+\begin{lstlisting}
+struct virtio_net_ctrl_ff_group_add {
+        le16 priority;	/* higher the value, higher priority */
+        le16 id;
+};
+\end{lstlisting}
+
+\subparagraph{Flow Filter Group Delete}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Group Delete}
+
+The command VIRTIO_NET_CTRL_FF_GROUP_DEL deletes the flow filter group
+identified by the group identifier \field{id} previously added using the command
+VIRTIO_NET_CTRL_FF_GROUP_ADD.
+
+\begin{lstlisting}
+struct virtio_net_ctrl_ff_group_del {
+        le16 id;
+};
+\end{lstlisting}
+
 \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 Types / Network Device / Legacy Interface: Framing Requirements}
 
-- 
2.34.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] [PATCH v7 4/5] virtio-net: Add flow filter match entry, action and requests

[Parav Pandit]

Define flow filter match key for the defined types.

Currently it covers the most common filter types and value
of Ethernet header, IP addresses, TCP and UDP ports.

Define generic flow filter add and delete requests and its transport
using a control virtqueue command and flow filter virtqueue(s).

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v6->v7:
- plenty of grammar corrections suggested by Cornelia
v2->v3:
- removed references to flow filter virtqueues
- removed one partial sentence
- added text for delete request
- aligned request and opcode values to just say request in the defines
v1->v2:
- squashed with match fields definition patch of v1
- added length to the flexible array definition struct to benefit
  from future runtime length bound checkers listed in
  https://people.kernel.org/kees/bounded-flexible-arrays-in-c
- renamed value to key
- addressed comments from Satananda
- merged destination and action to one struct
v0->v1:
- reworded add flow request text to consider optional mask
- replaced respond with set
- added mask flag to the type
---
 device-types/net/description.tex | 209 +++++++++++++++++++++++++++++++
 1 file changed, 209 insertions(+)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index e766b12..e7e80ab 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -1242,6 +1242,50 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
 };
 \end{lstlisting}
 
+\begin{lstlisting}
+struct virtio_ff_match_entry {
+        le16 type;
+        u8 mask_present;
+        u8 key_mask_len; /* sum of length of fields key and mask */
+        le64 fields_bmap;
+        u8 key[];
+        u8 mask[]; /* optional, only present when mask_present is set to 1 */
+};
+
+struct virtio_ff_match {
+       u8 num_entries; /* indicates number of valid entries */
+       u8 reserved[7];
+       struct virtio_ff_match_entry entries[];
+};
+
+#define VIRTIO_NET_FF_DEST_TYPE_RQ 0
+
+struct virtio_ff_action_forward {
+        u8 dest_type;
+        u8 reserved[3];
+        union {
+                le16 vq_index;
+                le32 reserved1;
+        };
+};
+
+#define VIRTIO_NET_FF_ACTION_DROP 0
+#define VIRTIO_NET_FF_ACTION_FORWARD 1
+
+struct virtio_ff_action_entry {
+        u8 action;
+        u8 len; /* indicates the length of value in bytes */
+        u8 value[];
+};
+
+struct virtio_ff_action {
+        u8 num_entries; /* indicates number of valid entries */
+        u8 reserved[7];
+        struct virtio_ff_action_entry entries[];
+};
+
+\end{lstlisting}
+
 The \field{type} corresponds to following table:
 
 \begin{tabular}{|l|l|l|}
@@ -1288,6 +1332,21 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
 \hline
 \end{tabular}
 
+For the \field{type} of VIRTIO_NET_FF_ETH_HDR, the match entries
+\field{key} and \field{mask} are in following format:
+
+\begin{lstlisting}
+struct virtio_net_ff_match_eth_hdr {
+        u8 dmac[6];
+        u8 smac[6];
+        le16 ether_type;
+};
+\end{lstlisting}
+
+\field{dmac} is valid when VIRTIO_NET_FF_DST_MAC is set.
+\field{smac} is valid when VIRTIO_NET_FF_SRC_MAC is set.
+\field{ether_type} is valid when VIRTIO_NET_FF_ETHER_TYPE is set.
+
 For the \field{type} of VIRTIO_NET_FF_VLAN_TAG_HDR, VLAN tag fields
 are represented by a bitmap in \field{fields_bmap} as follows:
 
@@ -1316,6 +1375,20 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
 \hline
 \end{tabular}
 
+For the \field{type} of VIRTIO_NET_FF_IPV4_HDR, the match entries
+\field{key} and \field{mask} are in following format:
+
+\begin{lstlisting}
+struct virtio_net_ff_match_ipv4_hdr {
+        le32 reserved[3];
+        le32 sip;
+        le32 dip;
+};
+\end{lstlisting}
+
+\field{sip} is valid when VIRTIO_NET_FF_SRC_IPV4 is set.
+\field{dip} is valid when VIRTIO_NET_FF_DST_IPV4 is set.
+
 For the \field{type} of VIRTIO_NET_FF_IPV6_HDR, header fields
 are represented by a bitmap in \field{fields_bmap} as follows:
 
@@ -1331,6 +1404,20 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
 \hline
 \end{tabular}
 
+For the \field{type} of VIRTIO_NET_FF_IPV4_HDR, the match entries
+\field{key} and \field{mask} are in following format:
+
+\begin{lstlisting}
+struct virtio_net_ff_match_ipv6_hdr {
+        le32 reserved[2];
+        u8 sip[16];
+        u8 dip[16];
+};
+\end{lstlisting}
+
+\field{sip} is valid when VIRTIO_NET_FF_SRC_IPV6 is set.
+\field{dip} is valid when VIRTIO_NET_FF_DST_IPV6 is set.
+
 For the \field{type} of VIRTIO_NET_FF_TCP_HDR, header fields
 are represented by a bitmap in \field{fields_bmap} as follows:
 
@@ -1346,6 +1433,20 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
 \hline
 \end{tabular}
 
+For the \field{type} of VIRTIO_NET_FF_TCP_HDR, the match entries
+\field{key} and \field{mask} are in following format:
+
+\begin{lstlisting}
+struct virtio_ndr_ff_match_tcp_hdr {
+        le16 sport;
+        le16 dport;
+        le32 reserved[4];
+};
+\end{lstlisting}
+
+\field{sport} is valid when VIRTIO_NET_FF_SRC_TCP_PORT is set.
+\field{dport} is valid when VIRTIO_NET_FF_DST_TCP_PORT is set.
+
 For the \field{type} of VIRTIO_NET_FF_UDP_HDR, header fields
 are represented by a bitmap in \field{fields_bmap} as follows:
 
@@ -1361,6 +1462,89 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
 \hline
 \end{tabular}
 
+For the \field{type} of VIRTIO_NET_FF_UDP_HDR, the match entries
+\field{key} and \field{mask} are in following format:
+
+\begin{lstlisting}
+struct virtio_ndr_ff_match_udp_hdr {
+        le16 sport;
+        le16 dport;
+        le32 reserved;
+};
+\end{lstlisting}
+
+\field{sport} is valid when VIRTIO_NET_FF_SRC_UDP_PORT is set.
+\field{dport} is valid when VIRTIO_NET_FF_DST_UDP_PORT is set.
+
+\paragraph{Flow Filter Request}
+\label{sec:Device Types / Network Device / Device Operation / Flow Filter / Flow Filter Request}
+
+Two flow filter requests are supported by the device.
+
+\begin{itemize}
+\item Add or replace a flow filter using a request \field{struct virtio_net_ff_add}.
+
+\item Delete an existing flow filter using a request \field{struct virtio_net_ff_del}.
+
+\end{itemize}
+
+\begin{lstlisting}
+#define VIRTIO_NET_FF_REQ_ADD 0
+#define VIRTIO_NET_FF_REQ_DEL 1
+
+struct virtio_net_ff_add {
+        u8 request; /* VIRTIO_NET_FF_REQ_ADD */
+        u8 priority;	/* higher the value, higher priority */
+        u16 group_id;
+        le32 id;
+        struct virtio_ff_match match;
+        struct virtio_ff_action action;
+};
+
+struct virtio_net_ff_del {
+        u8 request;  /* VIRTIO_NET_FF_REQ_DEL */
+        u8 reserved[3];
+        le32 id;
+};
+
+struct virtio_net_ff_result {
+        le16 status;
+};
+
+#define VIRTIO_NET_FF_RESULT_OK 0
+#define VIRTIO_NET_FF_RESULT_ERR 1
+
+struct virtio_net_ff_req {
+        /* Device-readable part */
+        union {
+                struct virtio_net_ff_add add;
+                struct virtio_net_ff_del del;
+        };
+        /* Device-writable part */
+        struct virtio_net_ff_result result;
+};
+\end{lstlisting}
+
+When adding a flow filter entry using request \field{struct virtio_net_ff_add},
+\field{match.match_entries} indidates the number of valid array entries
+\field{match.entries}.
+For each of the valid entry in \field{match.entries}, the fields \field{type}
+and \field{key} are in the format described in
+\ref{sec:Device Types / Network Device / Device Operation / Flow Filter / Match Types and Fields}.
+When \field{mask_present} is set, the field \field{mask} is present and has
+exact same format as \field{key}.
+
+The field \field{key_mask_len} represents the total length of fields
+\field{key} and \field{mask}.
+
+Previously added flow filter entries can be deleted by the driver using
+the VIRTIO_NET_FF_REQ_DEL request.
+
+When the device completes the request, \field{status} is updated
+by the device; when the request is successful, \field{status} is
+set to VIRTIO_NET_FF_RESULT_OK, on error, \field{status} is
+set to VIRTIO_NET_FF_RESULT_ERR.
+
 \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
@@ -2539,6 +2723,8 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \item the driver can send commands VIRTIO_NET_CTRL_FF_GROUP_ADD
 and VIRTIO_NET_CTRL_FF_GROUP_DEL for adding and deleting the
 flow group.
+\item the driver can send the command VIRTIO_NET_CTRL_FF_REQ to
+add or delete flow filter.
 \end{itemize}
 
 \begin{lstlisting}
@@ -2547,6 +2733,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
  #define VIRTIO_NET_CTRL_FF_MATCH_CAP_GET 1
  #define VIRTIO_NET_CTRL_FF_GROUP_ADD 2
  #define VIRTIO_NET_CTRL_FF_GROUP_DEL 3
+ #define VIRTIO_NET_CTRL_FF_REQ 4
 \end{lstlisting}
 
 \subparagraph{Flow Filter Capabilities Get}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Capabilities Get}
@@ -2629,6 +2816,28 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 };
 \end{lstlisting}
 
+\subparagraph{Flow Filter Requests}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Requests}
+
+The flow filter requests are transported using the 
+VIRTIO_NET_CTRL_FF_REQ command.
+
+\begin{lstlisting}
+struct virtio_net_ctrl_ff_req {
+        union {
+                struct virtio_net_ff_add add;
+                struct virtio_net_ff_del del;
+        };
+};
+
+\end{lstlisting}
+
+The \field{command-specific-data} is in format of
+\field{struct virtio_net_ctrl_ff_req}.
+
+When the flow filter request command is successful, the
+\field{command-specific-result} is in format of
+\field{struct virtio_net_ff_result}.
+
 \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 Types / Network Device / Legacy Interface: Framing Requirements}
 
-- 
2.34.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] [PATCH v7 5/5] virtio-net: Add flow filter device and driver requirements

[Parav Pandit]

The flow filter functionality consists of the following
four components.
Add driver and device requirements for it.

1. Device capabilities query for commands VIRTIO_NET_CTRL_FF_CAP_GET,
   VIRTIO_NET_CTRL_FF_MATCH_CAP_GET.
2. Flow filter group operation commands VIRTIO_NET_CTRL_FF_GROUP_ADD
   and VIRTIO_NET_CTRL_FF_GROUP_DEL.
3. Flow filter requests using command VIRTIO_NET_CTRL_FF_REQ and
   the structure virtio_net_ff_op for the flow filter virtqueue.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
Signed-off-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
changelog:
v5->v6:
- removed white spaces from end of line
- added missing conformance links
v2->v3:
- removed dependency on the dynamic queue creation as the
  infrastructure is not yet ready
v1->v2:
- fixed comments from Heng
- fixed spelling from initializaton to initialization
- added more requirements for multiple actions
v0->v1:
- addressed comments from Satananda
- added device requirement to return non zero value in fields_bmap
- added device requirement to not repeat filter type in response
- added driver requirement to order filter match field as it
  appears in the packet
- added device requirement to fail group delete command on existing
  flow entries
---
 device-types/net/description.tex        | 104 ++++++++++++++++++++++++
 device-types/net/device-conformance.tex |   1 +
 device-types/net/driver-conformance.tex |   1 +
 3 files changed, 106 insertions(+)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index e7e80ab..ae5baed 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -2838,6 +2838,110 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 \field{command-specific-result} is in format of
 \field{struct virtio_net_ff_result}.
 
+\devicenormative{\subparagraph}{Flow Filter}{Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter}
+
+When the VIRTIO_NET_F_FLOW_FILTER is negotiated, the device MUST support
+VIRTIO_NET_CTRL_FF_CAP_GET, VIRTIO_NET_CTRL_FF_MATCH_CAP_GET, VIRTIO_NET_CTRL_FF_GROUP_ADD,
+VIRTIO_NET_CTRL_FF_GROUP_DEL and VIRTIO_NET_CTRL_FF_REQ commands.
+
+When the VIRTIO_NET_F_FLOW_FILTER is not negotiated, the device MUST respond
+with error VIRTIO_NET_ERR for
+VIRTIO_NET_CTRL_FF_CAP_GET, VIRTIO_NET_CTRL_FF_MATCH_CAP_GET, VIRTIO_NET_CTRL_FF_GROUP_ADD,
+VIRTIO_NET_CTRL_FF_GROUP_DEL and VIRTIO_NET_CTRL_FF_REQ commands.
+
+When the command VIRTIO_NET_CTRL_FF_CAP_GET completes successfully, the device
+MUST set non zero value for fields
+\field{max_groups}, \field{max_ff_per_group}, \field{max_ff},
+\field{max_match_fields}, \field{max_flow_priorities_per_group} and
+\field{max_actions}.
+
+When the command VIRTIO_NET_CTRL_FF_MATCH_CAP_GET completes successfully,
+\begin{itemize}
+\item the device MUST set non zero value for fields \field{num_entries}, \field{fields_bmap}
+and set corresponding number of valid entries.
+\item the device MUST NOT repeat \field{type} in the \field{types}.
+\end{itemize}
+
+The device MUST set VIRTIO_NET_ERROR for the command
+VIRTIO_NET_CTRL_FF_GROUP_ADD if there are existing flow filters for the
+supplied group \field{id} or for the supplied \field{priority}.
+
+The device MUST set VIRTIO_NET_ERROR for the command
+VIRTIO_NET_CTRL_FF_GROUP_DEL if the group identified with \field{id}
+does not exist in the device.
+
+The device MUST set VIRTIO_NET_ERROR for the command
+VIRTIO_NET_CTRL_FF_GROUP_DEL if the group identified with \field{id}
+has one or more flow filters present in the group.
+
+The device MUST fail the operation VIRTIO_NET_FF_OP_ADD if the
+\field{match} contains duplicate \field{type}.
+
+The device MUST fail the operation VIRTIO_NET_FF_OP_DEL if the
+requested flow filter of identifier \field{id} do not exist in the
+the device.
+
+The device MUST fail the operation VIRTIO_NET_FF_OP_ADD if the
+\field{vq_index} in the \field{dest} is outside of the range.
+
+When the flow filter forwards the packet to the \field{vq_index} and
+if the receiveq is reset, the device MUST drop such packets.
+
+When the flow filter \field{action} is VIRTIO_NET_FF_ACTION_DROP,
+the device MUST ignore rest of the fields of
+\field{struct virtio_flow_action_entry}.
+
+When the driver has added multiple flow filters with same \field{priority}
+and for a packet if multiple flow filters MAY match such that it MAY result
+in different \field{action} or different \field{dest}, the device
+MAY apply any of the matching flow filters.
+
+The device MUST follow received packet processing ordering chain as following:
+
+The device SHOULD set \field{device_status} to DEVICE_NEEDS_RESET when
+the driver has not set the flow filter transport mode to
+VIRTIO_NET_FF_TRANSPORT_FFVQ and if the driver enables the flow filter
+virtqueue.
+
+The device MUST apply the actions of \field{struct virtio_flow_action} in same
+order as it is supplied by the driver when \field{num_entries} is greater than 1.
+
+\begin{itemize}
+\item Device configuration done using control virtqueue commands VIRTIO_NET_CTRL_RX,
+   VIRTIO_NET_CTRL_MAC and VIRTIO_NET_CTRL_VLAN.
+\item Flow filters programmed using flow filters functionality.
+\item Device configuration done using VIRTIO_NET_CTRL_MQ_RSS_CONFIG command.
+\end{itemize}
+
+When the device drops the packet due to the configuration done using the control
+virtqueue commands VIRTIO_NET_CTRL_RX or VIRTIO_NET_CTRL_MAC or VIRTIO_NET_CTRL_VLAN,
+the device MUST stop processing this packet for flow filters processing.
+
+When the device matches the flow filter for the packet and if the match is successful,
+the filter processing chain MUST stop, i.e. next level of processing MUST not be done.
+
+When the device perform flow filter match operations and if the operation
+result did not have any match, the receive packet processing continues to next level,
+i.e. to apply configuration done using VIRTIO_NET_CTRL_MQ_RSS_CONFIG command.
+
+\drivernormative{\subparagraph}{Flow Filter}{Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter}
+
+The driver MUST NOT send any flow filters specific commands having class code
+of VIRTIO_NET_CTRL_FF when VIRTIO_NET_F_FLOW_FILTER is not negotiated.
+
+The driver SHOULD NOT add multiple flow filters with same \field{priority}
+in a flow filter group, with overlapping match values.
+
+The driver SHOULD use different priority for different flow filters
+if multiple of the flow filters MAY match for a packet.
+
+The driver SHOULD set the \field{type} in \field{match_entries} as that of
+the order appears in the packet.
+
+The driver MUST NOT set \field{num_entries} in \field{struct virtio_ff_action}
+to more than \field{max_actions} supplied by the device in the
+\field{virtio_net_ctrl_ff_caps}.
+
 \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 Types / Network Device / Legacy Interface: Framing Requirements}
 
diff --git a/device-types/net/device-conformance.tex b/device-types/net/device-conformance.tex
index 52526e4..1228c27 100644
--- a/device-types/net/device-conformance.tex
+++ b/device-types/net/device-conformance.tex
@@ -16,4 +16,5 @@
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
 \item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Device Statistics}
+\item \ref{devicenormative:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter}
 \end{itemize}
diff --git a/device-types/net/driver-conformance.tex b/device-types/net/driver-conformance.tex
index c693c4f..9f9b63e 100644
--- a/device-types/net/driver-conformance.tex
+++ b/device-types/net/driver-conformance.tex
@@ -16,4 +16,5 @@
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Inner Header Hash}
 \item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Device Statistics}
+\item \ref{drivernormative:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter}
 \end{itemize}
-- 
2.34.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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Michael S. Tsirkin]

On Thu, Nov 23, 2023 at 11:21:16AM +0200, Parav Pandit wrote:
> The device responds flow filter capabilities using two commands.
> One command indicates generic flow filter device limits such as
> number of flow filters, number of flow filter groups, support or
> multiple transports etc.
> 
> The second command indicates supported match types, and fields
> of the packet.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Signed-off-by: Parav Pandit <parav@nvidia.com>

So I am still unsure about these commands.  What exactly is the point?

Patch 5/5 mandates that device validates all fields already.
Are there guests that will actually look at these
caps as opposed to just sending commands and looking
at the return status?


> ---
> changelog:
> v6->v7:
> - plenty of grammar corrections suggested by Cornelia
> v2->v3:
> - rebased on virtio-1.4 branch
> - removed reference for flow filter virtqueue
> v1->v2:
> - addressed comments from Satananda
> - added vlan type match field
> - kept space for types between l2, l3, l4 header match types
> - renamed mask to mask_supported with shorter width
> - made more fields reserved for future
> - addressed comments from Heng
> - grammar correction
> - added field to indicate supported number of actions per flow
>   filter match entry
> - added missing documentation for max_flow_priorities_per_group
> v0->v1:
> - added mask field in the type to indicate supported mask by device
>   and also in later patch to use it to indicate mask on adding
>   flow filter. As a result removed the mask_supported capability
>   field
> ---
>  device-types/net/description.tex | 207 ++++++++++++++++++++++++++++++-
>  1 file changed, 206 insertions(+), 1 deletion(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 03909ae..10d92d9 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -1170,7 +1170,11 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
>  
>  The device indicates the flow filter capabilities to the driver. These
>  capabilities include various maximum device limits and
> -supported packet match fields.
> +supported packet match fields. These control virtqueue
> +commands are:
> +\ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Capabilities Get}
> +and
> +\ref{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Match Capabilities Get}.
>  
>  The flow filters are grouped using a flow filter group. Each flow filter
>  group has a priority. The device first applies the flow filters of the highest
> @@ -1222,6 +1226,136 @@ \subsubsection{Flow Filter}\label{sec:Device Types / Network Device / Device Ope
>        the flow filters in group_C, the flow filters of next level group_B are applied.
>  \end{itemize}
>  
> +\paragraph{Match Types and Fields}\label{sec:Device Types / Network Device / Device Operation / Flow Filter / Match Types and Fields}
> +
> +\begin{lstlisting}
> +struct virtio_net_ff_match_type_cap {
> +        le16 type;
> +        u8 mask_supported;
> +        u8 reserved[5];
> +        le64 fields_bmap;
> +};
> +\end{lstlisting}
> +
> +The \field{type} corresponds to following table:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Type & Name & Description \\
> +\hline \hline
> +0   & VIRTIO_NET_FF_ETH_HDR & Ethernet header of the packet \\
> +\hline
> +0x1   & VIRTIO_NET_FF_VLAN_TAG_HDR & VLAN tag of the packet \\
> +\hline
> +0x200   & VIRTIO_NET_FF_IPV4_HDR & IPv4 header of the packet \\
> +\hline
> +0x300   & VIRTIO_NET_FF_IPV6_HDR & IPv6 header of the packet \\
> +\hline
> +0x400   & VIRTIO_NET_FF_TCP_HDR & TCP header of the packet \\
> +\hline
> +0x500   & VIRTIO_NET_FF_UDP_HDR & UDP header of the packet \\
> +\hline
> +other   & -    & reserved \\
> +\hline
> +\end{tabular}
> +
> +When \field{mask_supported} is set, for the specific \field{type}, the
> +device can mask packet fields with the mask supplied in the flow
> +filter match entry.
> +
> +For each \field{type} the \field{fields_bmap} indicates supported fields
> +of the packet header which can be matched.
> +
> +For the \field{type} of VIRTIO_NET_FF_ETH_HDR, header fields
> +are represented by a bitmap in \field{fields_bmap} as follows:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Bit & Name & Description \\
> +\hline \hline
> +0   & VIRTIO_NET_FF_DST_MAC & Destination MAC address in the packet \\
> +\hline
> +1   & VIRTIO_NET_FF_SRC_MAC & Source MAC address in the packet \\
> +\hline
> +2   & VIRTIO_NET_FF_ETHER_TYPE & Ether type in the packet \\
> +\hline
> +other   & -    & reserved \\
> +\hline
> +\end{tabular}
> +
> +For the \field{type} of VIRTIO_NET_FF_VLAN_TAG_HDR, VLAN tag fields
> +are represented by a bitmap in \field{fields_bmap} as follows:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Bit & Name & Description \\
> +\hline \hline
> +0   & VIRTIO_NET_FF_VLAN_TAG_TCI & Vlan tag TCI 16-bit field \\
> +\hline
> +other   & -    & reserved \\
> +\hline
> +\end{tabular}
> +
> +For the \field{type} of VIRTIO_NET_FF_IPV4_HDR, header fields
> +are represented by a bitmap in \field{fields_bmap} as follows:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Bit & Name & Description \\
> +\hline \hline
> +0   & VIRTIO_NET_FF_SRC_IPV4 & Source IPV4 address in the packet \\
> +\hline
> +1   & VIRTIO_NET_FF_DST_IPV4 & Destination IPV4 address in the packet \\
> +\hline
> +other   & -    & reserved \\
> +\hline
> +\end{tabular}
> +
> +For the \field{type} of VIRTIO_NET_FF_IPV6_HDR, header fields
> +are represented by a bitmap in \field{fields_bmap} as follows:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Bit & Name & Description \\
> +\hline \hline
> +0   & VIRTIO_NET_FF_SRC_IPV6 & Source IPV6 address in the packet \\
> +\hline
> +1   & VIRTIO_NET_FF_DST_IPV6 & Destination IPV6 address in the packet \\
> +\hline
> +other   & -    & reserved \\
> +\hline
> +\end{tabular}
> +
> +For the \field{type} of VIRTIO_NET_FF_TCP_HDR, header fields
> +are represented by a bitmap in \field{fields_bmap} as follows:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Bit & Name & Description \\
> +\hline \hline
> +0   & VIRTIO_NET_FF_SRC_TCP_PORT & Source TCP port in the packet \\
> +\hline
> +1   & VIRTIO_NET_FF_DST_TCP_PORT & Destination TCP port in the packet \\
> +\hline
> +other   & -    & reserved \\
> +\hline
> +\end{tabular}
> +
> +For the \field{type} of VIRTIO_NET_FF_UDP_HDR, header fields
> +are represented by a bitmap in \field{fields_bmap} as follows:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Bit & Name & Description \\
> +\hline \hline
> +0   & VIRTIO_NET_FF_SRC_UDP_PORT & Source UDP port in the packet \\
> +\hline
> +1   & VIRTIO_NET_FF_DST_UDP_PORT & Destination UDP port in the packet \\
> +\hline
> +other   & -    & reserved  \\
> +\hline
> +\end{tabular}
> +
>  \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
> @@ -2389,6 +2523,77 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  of the driver's records. In such cases, the driver should allocate additional
>  space for the \field{command-specific-result} buffer.
>  
> +\paragraph{Flow Filter}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter}
> +
> +If the VIRTIO_NET_F_FLOW_FILTER feature is negotiated,
> +
> +\begin{itemize}
> +\item the driver can send commands VIRTIO_NET_CTRL_FF_CAP_GET and
> +VIRTIO_NET_CTRL_FF_MATCH_CAP_GET to query the flow filter
> +capabilities of the device.
> +\end{itemize}
> +
> +\begin{lstlisting}
> +#define VIRTIO_NET_CTRL_FF 7
> + #define VIRTIO_NET_CTRL_FF_CAP_GET 0
> + #define VIRTIO_NET_CTRL_FF_MATCH_CAP_GET 1
> +\end{lstlisting}
> +
> +\subparagraph{Flow Filter Capabilities Get}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Capabilities Get}
> +
> +The command VIRTIO_NET_CTRL_FF_CAP_GET provides the flow filter device capabilities.
> +
> +\begin{lstlisting}
> +struct virtio_net_ctrl_ff_caps {
> +        le16 max_match_fields;
> +        le16 max_groups; /* valid group id = max_groups - 1 */
> +        le32 max_ff_per_group;
> +        le32 max_ff; /* max flow_id in add/del = max_ff - 1 */
> +        le16 max_actions;
> +        u8 max_flow_priorities_per_group;
> +};
> +\end{lstlisting}
> +
> +\field{max_groups} indicates total number of flow filter groups supported
> +by the device whose group identifiers can be any value in the range from 0 to
> +\field{max_groups - 1}. The flow filter group can have any priority in range
> +of 0 to \field{max_groups - 1}.
> +
> +\field{max_ff_per_group} indicates the maximum number of
> +flow filters per flow filter group which can be added by the driver.
> +
> +\field{max_ff} indicates the maximum number of flow filters across
> +all the flow groups which can be added by the driver.
> +
> +\field{max_ff_priorities_per_group} indicates the maximum priority value
> +of a flow filter within a group. A flow filter within a group can have any
> +priority in range of zero to \field{max_ff_priorities_per_group - 1}.
> +
> +\field{max_match_fields} indicates maximum number of fields of a packet
> +which can be matched by the device for a flow filter.
> +
> +\field{max_actions} indicates maximum number of actions for a flow filter
> +that can be supplied.
> +
> +\field{max_flow_priorities_per_group} indicates maximum number of
> +priorities supported by the device per flow filter group.
> +
> +\subparagraph{Flow Filter Match Capabilities Get}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Flow Filter / Flow Filter Match Capabilities Get}
> +
> +The command VIRTIO_NET_CTRL_FF_MATCH_CAP_GET indicates which fields
> +of the packet can be matched.
> +
> +\begin{lstlisting}
> +struct virtio_net_ctrl_ff_match_types {
> +        le32 num_entries;
> +        struct virtio_net_ff_match_type_cap types[];
> +};
> +\end{lstlisting}
> +
> +\field{num_entries} indicates the length of the array \field{types}.
> +Each array entry of \field{types} represents the fields of the packet
> +which are supported for matching by the device.
> +
>  \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
>  Types / Network Device / Legacy Interface: Framing Requirements}
>  
> -- 
> 2.34.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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, November 23, 2023 7:44 PM
> 
> On Thu, Nov 23, 2023 at 11:21:16AM +0200, Parav Pandit wrote:
> > The device responds flow filter capabilities using two commands.
> > One command indicates generic flow filter device limits such as number
> > of flow filters, number of flow filter groups, support or multiple
> > transports etc.
> >
> > The second command indicates supported match types, and fields of the
> > packet.
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> So I am still unsure about these commands.  What exactly is the point?
> 
Two reasons.
The device reports maximum capabilities that the driver knows upfront. So that it does not need to come to device to know when to fail.

In future one may want to provision certain max limits that device can have as they eventually will consume certain device hardware resources.

> Patch 5/5 mandates that device validates all fields already.

> Are there guests that will actually look at these caps as opposed to just
> sending commands and looking at the return status?
When the device reaches the limit it would not even send the command.

For example if the device supports only one group, it wont implement ethtool ntuple and will chose only arfs as one example.


> 
> 
> > ---
> > changelog:
> > v6->v7:
> > - plenty of grammar corrections suggested by Cornelia
> > v2->v3:
> > - rebased on virtio-1.4 branch
> > - removed reference for flow filter virtqueue
> > v1->v2:
> > - addressed comments from Satananda
> > - added vlan type match field
> > - kept space for types between l2, l3, l4 header match types
> > - renamed mask to mask_supported with shorter width
> > - made more fields reserved for future
> > - addressed comments from Heng
> > - grammar correction
> > - added field to indicate supported number of actions per flow
> >   filter match entry
> > - added missing documentation for max_flow_priorities_per_group
> > v0->v1:
> > - added mask field in the type to indicate supported mask by device
> >   and also in later patch to use it to indicate mask on adding
> >   flow filter. As a result removed the mask_supported capability
> >   field
> > ---
> >  device-types/net/description.tex | 207
> > ++++++++++++++++++++++++++++++-
> >  1 file changed, 206 insertions(+), 1 deletion(-)
> >
> > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > index 03909ae..10d92d9 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -1170,7 +1170,11 @@ \subsubsection{Flow Filter}\label{sec:Device
> > Types / Network Device / Device Ope
> >
> >  The device indicates the flow filter capabilities to the driver.
> > These  capabilities include various maximum device limits and
> > -supported packet match fields.
> > +supported packet match fields. These control virtqueue commands are:
> > +\ref{sec:Device Types / Network Device / Device Operation / Control
> > +Virtqueue / Flow Filter / Flow Filter Capabilities Get} and
> > +\ref{sec:Device Types / Network Device / Device Operation / Control
> Virtqueue / Flow Filter / Flow Filter Match Capabilities Get}.
> >
> >  The flow filters are grouped using a flow filter group. Each flow
> > filter  group has a priority. The device first applies the flow
> > filters of the highest @@ -1222,6 +1226,136 @@ \subsubsection{Flow
> Filter}\label{sec:Device Types / Network Device / Device Ope
> >        the flow filters in group_C, the flow filters of next level group_B are
> applied.
> >  \end{itemize}
> >
> > +\paragraph{Match Types and Fields}\label{sec:Device Types / Network
> > +Device / Device Operation / Flow Filter / Match Types and Fields}
> > +
> > +\begin{lstlisting}
> > +struct virtio_net_ff_match_type_cap {
> > +        le16 type;
> > +        u8 mask_supported;
> > +        u8 reserved[5];
> > +        le64 fields_bmap;
> > +};
> > +\end{lstlisting}
> > +
> > +The \field{type} corresponds to following table:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Type & Name & Description \\
> > +\hline \hline
> > +0   & VIRTIO_NET_FF_ETH_HDR & Ethernet header of the packet \\
> > +\hline
> > +0x1   & VIRTIO_NET_FF_VLAN_TAG_HDR & VLAN tag of the packet \\
> > +\hline
> > +0x200   & VIRTIO_NET_FF_IPV4_HDR & IPv4 header of the packet \\
> > +\hline
> > +0x300   & VIRTIO_NET_FF_IPV6_HDR & IPv6 header of the packet \\
> > +\hline
> > +0x400   & VIRTIO_NET_FF_TCP_HDR & TCP header of the packet \\
> > +\hline
> > +0x500   & VIRTIO_NET_FF_UDP_HDR & UDP header of the packet \\
> > +\hline
> > +other   & -    & reserved \\
> > +\hline
> > +\end{tabular}
> > +
> > +When \field{mask_supported} is set, for the specific \field{type},
> > +the device can mask packet fields with the mask supplied in the flow
> > +filter match entry.
> > +
> > +For each \field{type} the \field{fields_bmap} indicates supported
> > +fields of the packet header which can be matched.
> > +
> > +For the \field{type} of VIRTIO_NET_FF_ETH_HDR, header fields are
> > +represented by a bitmap in \field{fields_bmap} as follows:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Bit & Name & Description \\
> > +\hline \hline
> > +0   & VIRTIO_NET_FF_DST_MAC & Destination MAC address in the packet
> \\
> > +\hline
> > +1   & VIRTIO_NET_FF_SRC_MAC & Source MAC address in the packet \\
> > +\hline
> > +2   & VIRTIO_NET_FF_ETHER_TYPE & Ether type in the packet \\
> > +\hline
> > +other   & -    & reserved \\
> > +\hline
> > +\end{tabular}
> > +
> > +For the \field{type} of VIRTIO_NET_FF_VLAN_TAG_HDR, VLAN tag fields
> > +are represented by a bitmap in \field{fields_bmap} as follows:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Bit & Name & Description \\
> > +\hline \hline
> > +0   & VIRTIO_NET_FF_VLAN_TAG_TCI & Vlan tag TCI 16-bit field \\
> > +\hline
> > +other   & -    & reserved \\
> > +\hline
> > +\end{tabular}
> > +
> > +For the \field{type} of VIRTIO_NET_FF_IPV4_HDR, header fields are
> > +represented by a bitmap in \field{fields_bmap} as follows:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Bit & Name & Description \\
> > +\hline \hline
> > +0   & VIRTIO_NET_FF_SRC_IPV4 & Source IPV4 address in the packet \\
> > +\hline
> > +1   & VIRTIO_NET_FF_DST_IPV4 & Destination IPV4 address in the packet
> \\
> > +\hline
> > +other   & -    & reserved \\
> > +\hline
> > +\end{tabular}
> > +
> > +For the \field{type} of VIRTIO_NET_FF_IPV6_HDR, header fields are
> > +represented by a bitmap in \field{fields_bmap} as follows:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Bit & Name & Description \\
> > +\hline \hline
> > +0   & VIRTIO_NET_FF_SRC_IPV6 & Source IPV6 address in the packet \\
> > +\hline
> > +1   & VIRTIO_NET_FF_DST_IPV6 & Destination IPV6 address in the packet
> \\
> > +\hline
> > +other   & -    & reserved \\
> > +\hline
> > +\end{tabular}
> > +
> > +For the \field{type} of VIRTIO_NET_FF_TCP_HDR, header fields are
> > +represented by a bitmap in \field{fields_bmap} as follows:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Bit & Name & Description \\
> > +\hline \hline
> > +0   & VIRTIO_NET_FF_SRC_TCP_PORT & Source TCP port in the packet \\
> > +\hline
> > +1   & VIRTIO_NET_FF_DST_TCP_PORT & Destination TCP port in the packet
> \\
> > +\hline
> > +other   & -    & reserved \\
> > +\hline
> > +\end{tabular}
> > +
> > +For the \field{type} of VIRTIO_NET_FF_UDP_HDR, header fields are
> > +represented by a bitmap in \field{fields_bmap} as follows:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Bit & Name & Description \\
> > +\hline \hline
> > +0   & VIRTIO_NET_FF_SRC_UDP_PORT & Source UDP port in the packet \\
> > +\hline
> > +1   & VIRTIO_NET_FF_DST_UDP_PORT & Destination UDP port in the
> packet \\
> > +\hline
> > +other   & -    & reserved  \\
> > +\hline
> > +\end{tabular}
> > +
> >  \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 @@
> > -2389,6 +2523,77 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > Types / Network Device / Devi  of the driver's records. In such cases,
> > the driver should allocate additional  space for the \field{command-
> specific-result} buffer.
> >
> > +\paragraph{Flow Filter}\label{sec:Device Types / Network Device /
> > +Device Operation / Control Virtqueue / Flow Filter}
> > +
> > +If the VIRTIO_NET_F_FLOW_FILTER feature is negotiated,
> > +
> > +\begin{itemize}
> > +\item the driver can send commands VIRTIO_NET_CTRL_FF_CAP_GET and
> > +VIRTIO_NET_CTRL_FF_MATCH_CAP_GET to query the flow filter
> > +capabilities of the device.
> > +\end{itemize}
> > +
> > +\begin{lstlisting}
> > +#define VIRTIO_NET_CTRL_FF 7
> > + #define VIRTIO_NET_CTRL_FF_CAP_GET 0  #define
> > +VIRTIO_NET_CTRL_FF_MATCH_CAP_GET 1 \end{lstlisting}
> > +
> > +\subparagraph{Flow Filter Capabilities Get}\label{sec:Device Types /
> > +Network Device / Device Operation / Control Virtqueue / Flow Filter /
> > +Flow Filter Capabilities Get}
> > +
> > +The command VIRTIO_NET_CTRL_FF_CAP_GET provides the flow filter
> device capabilities.
> > +
> > +\begin{lstlisting}
> > +struct virtio_net_ctrl_ff_caps {
> > +        le16 max_match_fields;
> > +        le16 max_groups; /* valid group id = max_groups - 1 */
> > +        le32 max_ff_per_group;
> > +        le32 max_ff; /* max flow_id in add/del = max_ff - 1 */
> > +        le16 max_actions;
> > +        u8 max_flow_priorities_per_group; }; \end{lstlisting}
> > +
> > +\field{max_groups} indicates total number of flow filter groups
> > +supported by the device whose group identifiers can be any value in
> > +the range from 0 to \field{max_groups - 1}. The flow filter group can
> > +have any priority in range of 0 to \field{max_groups - 1}.
> > +
> > +\field{max_ff_per_group} indicates the maximum number of flow filters
> > +per flow filter group which can be added by the driver.
> > +
> > +\field{max_ff} indicates the maximum number of flow filters across
> > +all the flow groups which can be added by the driver.
> > +
> > +\field{max_ff_priorities_per_group} indicates the maximum priority
> > +value of a flow filter within a group. A flow filter within a group
> > +can have any priority in range of zero to
> \field{max_ff_priorities_per_group - 1}.
> > +
> > +\field{max_match_fields} indicates maximum number of fields of a
> > +packet which can be matched by the device for a flow filter.
> > +
> > +\field{max_actions} indicates maximum number of actions for a flow
> > +filter that can be supplied.
> > +
> > +\field{max_flow_priorities_per_group} indicates maximum number of
> > +priorities supported by the device per flow filter group.
> > +
> > +\subparagraph{Flow Filter Match Capabilities Get}\label{sec:Device
> > +Types / Network Device / Device Operation / Control Virtqueue / Flow
> > +Filter / Flow Filter Match Capabilities Get}
> > +
> > +The command VIRTIO_NET_CTRL_FF_MATCH_CAP_GET indicates which
> fields
> > +of the packet can be matched.
> > +
> > +\begin{lstlisting}
> > +struct virtio_net_ctrl_ff_match_types {
> > +        le32 num_entries;
> > +        struct virtio_net_ff_match_type_cap types[]; };
> > +\end{lstlisting}
> > +
> > +\field{num_entries} indicates the length of the array \field{types}.
> > +Each array entry of \field{types} represents the fields of the packet
> > +which are supported for matching by the device.
> > +
> >  \subsubsection{Legacy Interface: Framing
> > Requirements}\label{sec:Device  Types / Network Device / Legacy
> > Interface: Framing Requirements}
> >
> > --
> > 2.34.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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Michael S. Tsirkin]

On Thu, Nov 23, 2023 at 06:40:59PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, November 23, 2023 7:44 PM
> > 
> > On Thu, Nov 23, 2023 at 11:21:16AM +0200, Parav Pandit wrote:
> > > The device responds flow filter capabilities using two commands.
> > > One command indicates generic flow filter device limits such as number
> > > of flow filters, number of flow filter groups, support or multiple
> > > transports etc.
> > >
> > > The second command indicates supported match types, and fields of the
> > > packet.
> > >
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
> > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > 
> > So I am still unsure about these commands.  What exactly is the point?
> > 
> Two reasons.
> The device reports maximum capabilities that the driver knows upfront. So that it does not need to come to device to know when to fail.

But that requires driver to keep extra state which can easily get out of
sync. For what benefit?

> In future one may want to provision certain max limits that device can have as they eventually will consume certain device hardware resources.

I don't exactly see how this helps provisioning.


> > Patch 5/5 mandates that device validates all fields already.
> 
> > Are there guests that will actually look at these caps as opposed to just
> > sending commands and looking at the return status?
> When the device reaches the limit it would not even send the command.


That's more logic for the driver to implement. Why should it bother when it
can just send it?

> For example if the device supports only one group, it wont implement ethtool ntuple and will chose only arfs as one example.

Sounds like a contrived example why does device even expose flow steering
then?

> 
> > 
> > 
> > > ---
> > > changelog:
> > > v6->v7:
> > > - plenty of grammar corrections suggested by Cornelia
> > > v2->v3:
> > > - rebased on virtio-1.4 branch
> > > - removed reference for flow filter virtqueue
> > > v1->v2:
> > > - addressed comments from Satananda
> > > - added vlan type match field
> > > - kept space for types between l2, l3, l4 header match types
> > > - renamed mask to mask_supported with shorter width
> > > - made more fields reserved for future
> > > - addressed comments from Heng
> > > - grammar correction
> > > - added field to indicate supported number of actions per flow
> > >   filter match entry
> > > - added missing documentation for max_flow_priorities_per_group
> > > v0->v1:
> > > - added mask field in the type to indicate supported mask by device
> > >   and also in later patch to use it to indicate mask on adding
> > >   flow filter. As a result removed the mask_supported capability
> > >   field
> > > ---
> > >  device-types/net/description.tex | 207
> > > ++++++++++++++++++++++++++++++-
> > >  1 file changed, 206 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/device-types/net/description.tex
> > > b/device-types/net/description.tex
> > > index 03909ae..10d92d9 100644
> > > --- a/device-types/net/description.tex
> > > +++ b/device-types/net/description.tex
> > > @@ -1170,7 +1170,11 @@ \subsubsection{Flow Filter}\label{sec:Device
> > > Types / Network Device / Device Ope
> > >
> > >  The device indicates the flow filter capabilities to the driver.
> > > These  capabilities include various maximum device limits and
> > > -supported packet match fields.
> > > +supported packet match fields. These control virtqueue commands are:
> > > +\ref{sec:Device Types / Network Device / Device Operation / Control
> > > +Virtqueue / Flow Filter / Flow Filter Capabilities Get} and
> > > +\ref{sec:Device Types / Network Device / Device Operation / Control
> > Virtqueue / Flow Filter / Flow Filter Match Capabilities Get}.
> > >
> > >  The flow filters are grouped using a flow filter group. Each flow
> > > filter  group has a priority. The device first applies the flow
> > > filters of the highest @@ -1222,6 +1226,136 @@ \subsubsection{Flow
> > Filter}\label{sec:Device Types / Network Device / Device Ope
> > >        the flow filters in group_C, the flow filters of next level group_B are
> > applied.
> > >  \end{itemize}
> > >
> > > +\paragraph{Match Types and Fields}\label{sec:Device Types / Network
> > > +Device / Device Operation / Flow Filter / Match Types and Fields}
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_net_ff_match_type_cap {
> > > +        le16 type;
> > > +        u8 mask_supported;
> > > +        u8 reserved[5];
> > > +        le64 fields_bmap;
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The \field{type} corresponds to following table:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Type & Name & Description \\
> > > +\hline \hline
> > > +0   & VIRTIO_NET_FF_ETH_HDR & Ethernet header of the packet \\
> > > +\hline
> > > +0x1   & VIRTIO_NET_FF_VLAN_TAG_HDR & VLAN tag of the packet \\
> > > +\hline
> > > +0x200   & VIRTIO_NET_FF_IPV4_HDR & IPv4 header of the packet \\
> > > +\hline
> > > +0x300   & VIRTIO_NET_FF_IPV6_HDR & IPv6 header of the packet \\
> > > +\hline
> > > +0x400   & VIRTIO_NET_FF_TCP_HDR & TCP header of the packet \\
> > > +\hline
> > > +0x500   & VIRTIO_NET_FF_UDP_HDR & UDP header of the packet \\
> > > +\hline
> > > +other   & -    & reserved \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +When \field{mask_supported} is set, for the specific \field{type},
> > > +the device can mask packet fields with the mask supplied in the flow
> > > +filter match entry.
> > > +
> > > +For each \field{type} the \field{fields_bmap} indicates supported
> > > +fields of the packet header which can be matched.
> > > +
> > > +For the \field{type} of VIRTIO_NET_FF_ETH_HDR, header fields are
> > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Bit & Name & Description \\
> > > +\hline \hline
> > > +0   & VIRTIO_NET_FF_DST_MAC & Destination MAC address in the packet
> > \\
> > > +\hline
> > > +1   & VIRTIO_NET_FF_SRC_MAC & Source MAC address in the packet \\
> > > +\hline
> > > +2   & VIRTIO_NET_FF_ETHER_TYPE & Ether type in the packet \\
> > > +\hline
> > > +other   & -    & reserved \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +For the \field{type} of VIRTIO_NET_FF_VLAN_TAG_HDR, VLAN tag fields
> > > +are represented by a bitmap in \field{fields_bmap} as follows:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Bit & Name & Description \\
> > > +\hline \hline
> > > +0   & VIRTIO_NET_FF_VLAN_TAG_TCI & Vlan tag TCI 16-bit field \\
> > > +\hline
> > > +other   & -    & reserved \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +For the \field{type} of VIRTIO_NET_FF_IPV4_HDR, header fields are
> > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Bit & Name & Description \\
> > > +\hline \hline
> > > +0   & VIRTIO_NET_FF_SRC_IPV4 & Source IPV4 address in the packet \\
> > > +\hline
> > > +1   & VIRTIO_NET_FF_DST_IPV4 & Destination IPV4 address in the packet
> > \\
> > > +\hline
> > > +other   & -    & reserved \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +For the \field{type} of VIRTIO_NET_FF_IPV6_HDR, header fields are
> > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Bit & Name & Description \\
> > > +\hline \hline
> > > +0   & VIRTIO_NET_FF_SRC_IPV6 & Source IPV6 address in the packet \\
> > > +\hline
> > > +1   & VIRTIO_NET_FF_DST_IPV6 & Destination IPV6 address in the packet
> > \\
> > > +\hline
> > > +other   & -    & reserved \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +For the \field{type} of VIRTIO_NET_FF_TCP_HDR, header fields are
> > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Bit & Name & Description \\
> > > +\hline \hline
> > > +0   & VIRTIO_NET_FF_SRC_TCP_PORT & Source TCP port in the packet \\
> > > +\hline
> > > +1   & VIRTIO_NET_FF_DST_TCP_PORT & Destination TCP port in the packet
> > \\
> > > +\hline
> > > +other   & -    & reserved \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +For the \field{type} of VIRTIO_NET_FF_UDP_HDR, header fields are
> > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Bit & Name & Description \\
> > > +\hline \hline
> > > +0   & VIRTIO_NET_FF_SRC_UDP_PORT & Source UDP port in the packet \\
> > > +\hline
> > > +1   & VIRTIO_NET_FF_DST_UDP_PORT & Destination UDP port in the
> > packet \\
> > > +\hline
> > > +other   & -    & reserved  \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > >  \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 @@
> > > -2389,6 +2523,77 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > > Types / Network Device / Devi  of the driver's records. In such cases,
> > > the driver should allocate additional  space for the \field{command-
> > specific-result} buffer.
> > >
> > > +\paragraph{Flow Filter}\label{sec:Device Types / Network Device /
> > > +Device Operation / Control Virtqueue / Flow Filter}
> > > +
> > > +If the VIRTIO_NET_F_FLOW_FILTER feature is negotiated,
> > > +
> > > +\begin{itemize}
> > > +\item the driver can send commands VIRTIO_NET_CTRL_FF_CAP_GET and
> > > +VIRTIO_NET_CTRL_FF_MATCH_CAP_GET to query the flow filter
> > > +capabilities of the device.
> > > +\end{itemize}
> > > +
> > > +\begin{lstlisting}
> > > +#define VIRTIO_NET_CTRL_FF 7
> > > + #define VIRTIO_NET_CTRL_FF_CAP_GET 0  #define
> > > +VIRTIO_NET_CTRL_FF_MATCH_CAP_GET 1 \end{lstlisting}
> > > +
> > > +\subparagraph{Flow Filter Capabilities Get}\label{sec:Device Types /
> > > +Network Device / Device Operation / Control Virtqueue / Flow Filter /
> > > +Flow Filter Capabilities Get}
> > > +
> > > +The command VIRTIO_NET_CTRL_FF_CAP_GET provides the flow filter
> > device capabilities.
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_net_ctrl_ff_caps {
> > > +        le16 max_match_fields;
> > > +        le16 max_groups; /* valid group id = max_groups - 1 */
> > > +        le32 max_ff_per_group;
> > > +        le32 max_ff; /* max flow_id in add/del = max_ff - 1 */
> > > +        le16 max_actions;
> > > +        u8 max_flow_priorities_per_group; }; \end{lstlisting}
> > > +
> > > +\field{max_groups} indicates total number of flow filter groups
> > > +supported by the device whose group identifiers can be any value in
> > > +the range from 0 to \field{max_groups - 1}. The flow filter group can
> > > +have any priority in range of 0 to \field{max_groups - 1}.
> > > +
> > > +\field{max_ff_per_group} indicates the maximum number of flow filters
> > > +per flow filter group which can be added by the driver.
> > > +
> > > +\field{max_ff} indicates the maximum number of flow filters across
> > > +all the flow groups which can be added by the driver.
> > > +
> > > +\field{max_ff_priorities_per_group} indicates the maximum priority
> > > +value of a flow filter within a group. A flow filter within a group
> > > +can have any priority in range of zero to
> > \field{max_ff_priorities_per_group - 1}.
> > > +
> > > +\field{max_match_fields} indicates maximum number of fields of a
> > > +packet which can be matched by the device for a flow filter.
> > > +
> > > +\field{max_actions} indicates maximum number of actions for a flow
> > > +filter that can be supplied.
> > > +
> > > +\field{max_flow_priorities_per_group} indicates maximum number of
> > > +priorities supported by the device per flow filter group.
> > > +
> > > +\subparagraph{Flow Filter Match Capabilities Get}\label{sec:Device
> > > +Types / Network Device / Device Operation / Control Virtqueue / Flow
> > > +Filter / Flow Filter Match Capabilities Get}
> > > +
> > > +The command VIRTIO_NET_CTRL_FF_MATCH_CAP_GET indicates which
> > fields
> > > +of the packet can be matched.
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_net_ctrl_ff_match_types {
> > > +        le32 num_entries;
> > > +        struct virtio_net_ff_match_type_cap types[]; };
> > > +\end{lstlisting}
> > > +
> > > +\field{num_entries} indicates the length of the array \field{types}.
> > > +Each array entry of \field{types} represents the fields of the packet
> > > +which are supported for matching by the device.
> > > +
> > >  \subsubsection{Legacy Interface: Framing
> > > Requirements}\label{sec:Device  Types / Network Device / Legacy
> > > Interface: Framing Requirements}
> > >
> > > --
> > > 2.34.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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Friday, November 24, 2023 4:28 AM
> 
> On Thu, Nov 23, 2023 at 06:40:59PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Thursday, November 23, 2023 7:44 PM
> > >
> > > On Thu, Nov 23, 2023 at 11:21:16AM +0200, Parav Pandit wrote:
> > > > The device responds flow filter capabilities using two commands.
> > > > One command indicates generic flow filter device limits such as
> > > > number of flow filters, number of flow filter groups, support or
> > > > multiple transports etc.
> > > >
> > > > The second command indicates supported match types, and fields of
> > > > the packet.
> > > >
> > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > >
> > > So I am still unsure about these commands.  What exactly is the point?
> > >
> > Two reasons.
> > The device reports maximum capabilities that the driver knows upfront. So
> that it does not need to come to device to know when to fail.
> 
> But that requires driver to keep extra state which can easily get out of sync.
Not sure what you mean by get out of sync from whom?
Not from the device.

There are many capabilities here each with a different use.
For many caps, it is to know what is supported/not to decide in the driver.
Certain caps are max to know what is the range of id that driver can use like flow filter id, group id.

> For what benefit?
>
For functional work.
 
> > In future one may want to provision certain max limits that device can have
> as they eventually will consume certain device hardware resources.
> 
> I don't exactly see how this helps provisioning.
> 
Provisioning side will set parameters which will reflect these limits to be same on src and dst hypervisor when device migration is used.

> 
> > > Patch 5/5 mandates that device validates all fields already.
> >
> > > Are there guests that will actually look at these caps as opposed to
> > > just sending commands and looking at the return status?
> > When the device reaches the limit it would not even send the command.
> 
> 
> That's more logic for the driver to implement. Why should it bother when it
> can just send it?
>
It can just send it and fail and flood with error logs.
Also it cannot just send some random id based numbers that device does not support.

> > For example if the device supports only one group, it wont implement
> ethtool ntuple and will chose only arfs as one example.
> 
> Sounds like a contrived example why does device even expose flow steering
> then?
It is not contrived at all.

A device may be support one or multiple groups. Each group in the OS has multiple users.
One group is enough for either ARFS or ethool but not both.
Two groups can allow ARFS and ethtool to co-exists.
3 groups will allow in future to expose some queues to user apps.

> 
> >
> > >
> > >
> > > > ---
> > > > changelog:
> > > > v6->v7:
> > > > - plenty of grammar corrections suggested by Cornelia
> > > > v2->v3:
> > > > - rebased on virtio-1.4 branch
> > > > - removed reference for flow filter virtqueue
> > > > v1->v2:
> > > > - addressed comments from Satananda
> > > > - added vlan type match field
> > > > - kept space for types between l2, l3, l4 header match types
> > > > - renamed mask to mask_supported with shorter width
> > > > - made more fields reserved for future
> > > > - addressed comments from Heng
> > > > - grammar correction
> > > > - added field to indicate supported number of actions per flow
> > > >   filter match entry
> > > > - added missing documentation for max_flow_priorities_per_group
> > > > v0->v1:
> > > > - added mask field in the type to indicate supported mask by device
> > > >   and also in later patch to use it to indicate mask on adding
> > > >   flow filter. As a result removed the mask_supported capability
> > > >   field
> > > > ---
> > > >  device-types/net/description.tex | 207
> > > > ++++++++++++++++++++++++++++++-
> > > >  1 file changed, 206 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/device-types/net/description.tex
> > > > b/device-types/net/description.tex
> > > > index 03909ae..10d92d9 100644
> > > > --- a/device-types/net/description.tex
> > > > +++ b/device-types/net/description.tex
> > > > @@ -1170,7 +1170,11 @@ \subsubsection{Flow
> > > > Filter}\label{sec:Device Types / Network Device / Device Ope
> > > >
> > > >  The device indicates the flow filter capabilities to the driver.
> > > > These  capabilities include various maximum device limits and
> > > > -supported packet match fields.
> > > > +supported packet match fields. These control virtqueue commands
> are:
> > > > +\ref{sec:Device Types / Network Device / Device Operation /
> > > > +Control Virtqueue / Flow Filter / Flow Filter Capabilities Get}
> > > > +and \ref{sec:Device Types / Network Device / Device Operation /
> > > > +Control
> > > Virtqueue / Flow Filter / Flow Filter Match Capabilities Get}.
> > > >
> > > >  The flow filters are grouped using a flow filter group. Each flow
> > > > filter  group has a priority. The device first applies the flow
> > > > filters of the highest @@ -1222,6 +1226,136 @@ \subsubsection{Flow
> > > Filter}\label{sec:Device Types / Network Device / Device Ope
> > > >        the flow filters in group_C, the flow filters of next level
> > > > group_B are
> > > applied.
> > > >  \end{itemize}
> > > >
> > > > +\paragraph{Match Types and Fields}\label{sec:Device Types /
> > > > +Network Device / Device Operation / Flow Filter / Match Types and
> > > > +Fields}
> > > > +
> > > > +\begin{lstlisting}
> > > > +struct virtio_net_ff_match_type_cap {
> > > > +        le16 type;
> > > > +        u8 mask_supported;
> > > > +        u8 reserved[5];
> > > > +        le64 fields_bmap;
> > > > +};
> > > > +\end{lstlisting}
> > > > +
> > > > +The \field{type} corresponds to following table:
> > > > +
> > > > +\begin{tabular}{|l|l|l|}
> > > > +\hline
> > > > +Type & Name & Description \\
> > > > +\hline \hline
> > > > +0   & VIRTIO_NET_FF_ETH_HDR & Ethernet header of the packet \\
> > > > +\hline
> > > > +0x1   & VIRTIO_NET_FF_VLAN_TAG_HDR & VLAN tag of the packet \\
> > > > +\hline
> > > > +0x200   & VIRTIO_NET_FF_IPV4_HDR & IPv4 header of the packet \\
> > > > +\hline
> > > > +0x300   & VIRTIO_NET_FF_IPV6_HDR & IPv6 header of the packet \\
> > > > +\hline
> > > > +0x400   & VIRTIO_NET_FF_TCP_HDR & TCP header of the packet \\
> > > > +\hline
> > > > +0x500   & VIRTIO_NET_FF_UDP_HDR & UDP header of the packet \\
> > > > +\hline
> > > > +other   & -    & reserved \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +When \field{mask_supported} is set, for the specific
> > > > +\field{type}, the device can mask packet fields with the mask
> > > > +supplied in the flow filter match entry.
> > > > +
> > > > +For each \field{type} the \field{fields_bmap} indicates supported
> > > > +fields of the packet header which can be matched.
> > > > +
> > > > +For the \field{type} of VIRTIO_NET_FF_ETH_HDR, header fields are
> > > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > > +
> > > > +\begin{tabular}{|l|l|l|}
> > > > +\hline
> > > > +Bit & Name & Description \\
> > > > +\hline \hline
> > > > +0   & VIRTIO_NET_FF_DST_MAC & Destination MAC address in the
> packet
> > > \\
> > > > +\hline
> > > > +1   & VIRTIO_NET_FF_SRC_MAC & Source MAC address in the packet
> \\
> > > > +\hline
> > > > +2   & VIRTIO_NET_FF_ETHER_TYPE & Ether type in the packet \\
> > > > +\hline
> > > > +other   & -    & reserved \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +For the \field{type} of VIRTIO_NET_FF_VLAN_TAG_HDR, VLAN tag
> > > > +fields are represented by a bitmap in \field{fields_bmap} as follows:
> > > > +
> > > > +\begin{tabular}{|l|l|l|}
> > > > +\hline
> > > > +Bit & Name & Description \\
> > > > +\hline \hline
> > > > +0   & VIRTIO_NET_FF_VLAN_TAG_TCI & Vlan tag TCI 16-bit field \\
> > > > +\hline
> > > > +other   & -    & reserved \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +For the \field{type} of VIRTIO_NET_FF_IPV4_HDR, header fields are
> > > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > > +
> > > > +\begin{tabular}{|l|l|l|}
> > > > +\hline
> > > > +Bit & Name & Description \\
> > > > +\hline \hline
> > > > +0   & VIRTIO_NET_FF_SRC_IPV4 & Source IPV4 address in the packet \\
> > > > +\hline
> > > > +1   & VIRTIO_NET_FF_DST_IPV4 & Destination IPV4 address in the
> packet
> > > \\
> > > > +\hline
> > > > +other   & -    & reserved \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +For the \field{type} of VIRTIO_NET_FF_IPV6_HDR, header fields are
> > > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > > +
> > > > +\begin{tabular}{|l|l|l|}
> > > > +\hline
> > > > +Bit & Name & Description \\
> > > > +\hline \hline
> > > > +0   & VIRTIO_NET_FF_SRC_IPV6 & Source IPV6 address in the packet \\
> > > > +\hline
> > > > +1   & VIRTIO_NET_FF_DST_IPV6 & Destination IPV6 address in the
> packet
> > > \\
> > > > +\hline
> > > > +other   & -    & reserved \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +For the \field{type} of VIRTIO_NET_FF_TCP_HDR, header fields are
> > > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > > +
> > > > +\begin{tabular}{|l|l|l|}
> > > > +\hline
> > > > +Bit & Name & Description \\
> > > > +\hline \hline
> > > > +0   & VIRTIO_NET_FF_SRC_TCP_PORT & Source TCP port in the packet
> \\
> > > > +\hline
> > > > +1   & VIRTIO_NET_FF_DST_TCP_PORT & Destination TCP port in the
> packet
> > > \\
> > > > +\hline
> > > > +other   & -    & reserved \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +For the \field{type} of VIRTIO_NET_FF_UDP_HDR, header fields are
> > > > +represented by a bitmap in \field{fields_bmap} as follows:
> > > > +
> > > > +\begin{tabular}{|l|l|l|}
> > > > +\hline
> > > > +Bit & Name & Description \\
> > > > +\hline \hline
> > > > +0   & VIRTIO_NET_FF_SRC_UDP_PORT & Source UDP port in the
> packet \\
> > > > +\hline
> > > > +1   & VIRTIO_NET_FF_DST_UDP_PORT & Destination UDP port in the
> > > packet \\
> > > > +\hline
> > > > +other   & -    & reserved  \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > >  \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
> > > > @@
> > > > -2389,6 +2523,77 @@ \subsubsection{Control
> > > > Virtqueue}\label{sec:Device Types / Network Device / Devi  of the
> > > > driver's records. In such cases, the driver should allocate
> > > > additional  space for the \field{command-
> > > specific-result} buffer.
> > > >
> > > > +\paragraph{Flow Filter}\label{sec:Device Types / Network Device /
> > > > +Device Operation / Control Virtqueue / Flow Filter}
> > > > +
> > > > +If the VIRTIO_NET_F_FLOW_FILTER feature is negotiated,
> > > > +
> > > > +\begin{itemize}
> > > > +\item the driver can send commands VIRTIO_NET_CTRL_FF_CAP_GET
> and
> > > > +VIRTIO_NET_CTRL_FF_MATCH_CAP_GET to query the flow filter
> > > > +capabilities of the device.
> > > > +\end{itemize}
> > > > +
> > > > +\begin{lstlisting}
> > > > +#define VIRTIO_NET_CTRL_FF 7
> > > > + #define VIRTIO_NET_CTRL_FF_CAP_GET 0  #define
> > > > +VIRTIO_NET_CTRL_FF_MATCH_CAP_GET 1 \end{lstlisting}
> > > > +
> > > > +\subparagraph{Flow Filter Capabilities Get}\label{sec:Device
> > > > +Types / Network Device / Device Operation / Control Virtqueue /
> > > > +Flow Filter / Flow Filter Capabilities Get}
> > > > +
> > > > +The command VIRTIO_NET_CTRL_FF_CAP_GET provides the flow filter
> > > device capabilities.
> > > > +
> > > > +\begin{lstlisting}
> > > > +struct virtio_net_ctrl_ff_caps {
> > > > +        le16 max_match_fields;
> > > > +        le16 max_groups; /* valid group id = max_groups - 1 */
> > > > +        le32 max_ff_per_group;
> > > > +        le32 max_ff; /* max flow_id in add/del = max_ff - 1 */
> > > > +        le16 max_actions;
> > > > +        u8 max_flow_priorities_per_group; }; \end{lstlisting}
> > > > +
> > > > +\field{max_groups} indicates total number of flow filter groups
> > > > +supported by the device whose group identifiers can be any value
> > > > +in the range from 0 to \field{max_groups - 1}. The flow filter
> > > > +group can have any priority in range of 0 to \field{max_groups - 1}.
> > > > +
> > > > +\field{max_ff_per_group} indicates the maximum number of flow
> > > > +filters per flow filter group which can be added by the driver.
> > > > +
> > > > +\field{max_ff} indicates the maximum number of flow filters
> > > > +across all the flow groups which can be added by the driver.
> > > > +
> > > > +\field{max_ff_priorities_per_group} indicates the maximum
> > > > +priority value of a flow filter within a group. A flow filter
> > > > +within a group can have any priority in range of zero to
> > > \field{max_ff_priorities_per_group - 1}.
> > > > +
> > > > +\field{max_match_fields} indicates maximum number of fields of a
> > > > +packet which can be matched by the device for a flow filter.
> > > > +
> > > > +\field{max_actions} indicates maximum number of actions for a
> > > > +flow filter that can be supplied.
> > > > +
> > > > +\field{max_flow_priorities_per_group} indicates maximum number of
> > > > +priorities supported by the device per flow filter group.
> > > > +
> > > > +\subparagraph{Flow Filter Match Capabilities
> > > > +Get}\label{sec:Device Types / Network Device / Device Operation /
> > > > +Control Virtqueue / Flow Filter / Flow Filter Match Capabilities
> > > > +Get}
> > > > +
> > > > +The command VIRTIO_NET_CTRL_FF_MATCH_CAP_GET indicates
> which
> > > fields
> > > > +of the packet can be matched.
> > > > +
> > > > +\begin{lstlisting}
> > > > +struct virtio_net_ctrl_ff_match_types {
> > > > +        le32 num_entries;
> > > > +        struct virtio_net_ff_match_type_cap types[]; };
> > > > +\end{lstlisting}
> > > > +
> > > > +\field{num_entries} indicates the length of the array \field{types}.
> > > > +Each array entry of \field{types} represents the fields of the
> > > > +packet which are supported for matching by the device.
> > > > +
> > > >  \subsubsection{Legacy Interface: Framing
> > > > Requirements}\label{sec:Device  Types / Network Device / Legacy
> > > > Interface: Framing Requirements}
> > > >
> > > > --
> > > > 2.34.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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Michael S. Tsirkin]

On Fri, Nov 24, 2023 at 02:57:49AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Friday, November 24, 2023 4:28 AM
> > 
> > On Thu, Nov 23, 2023 at 06:40:59PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Thursday, November 23, 2023 7:44 PM
> > > >
> > > > On Thu, Nov 23, 2023 at 11:21:16AM +0200, Parav Pandit wrote:
> > > > > The device responds flow filter capabilities using two commands.
> > > > > One command indicates generic flow filter device limits such as
> > > > > number of flow filters, number of flow filter groups, support or
> > > > > multiple transports etc.
> > > > >
> > > > > The second command indicates supported match types, and fields of
> > > > > the packet.
> > > > >
> > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > >
> > > > So I am still unsure about these commands.  What exactly is the point?
> > > >
> > > Two reasons.
> > > The device reports maximum capabilities that the driver knows upfront. So
> > that it does not need to come to device to know when to fail.
> > 
> > But that requires driver to keep extra state which can easily get out of sync.
> Not sure what you mean by get out of sync from whom?
> Not from the device.

yes from the device. I can't say for sure how because I now see this
patch is implying driver requirements that you didn't document. I
thought the IDs can be any number. It appears that's not what you meant.
I can try and guess what you meant but I'd rather have you rewrite this
in a way that makes the meaning explicit.


> There are many capabilities here each with a different use.

And you are under the impression that dumping all kind of stuff in a
single place is good somehow?

> For many caps, it is to know what is supported/not to decide in the driver.

I can't parse this sentence.

> Certain caps are max to know what is the range of id that driver can use like flow filter id, group id.

Maybe then. But there's apparently no requirement for either device or
driver to put it in this range.

> > For what benefit?
> >
> For functional work.

You don't really explain what kind of work in this patchset, or in the
commit logs. Maybe it's obvious for you but not for everyone.

> > > In future one may want to provision certain max limits that device can have
> > as they eventually will consume certain device hardware resources.
> > 
> > I don't exactly see how this helps provisioning.
> > 
> Provisioning side will set parameters which will reflect these limits to be same on src and dst hypervisor when device migration is used.

But making provisioning depend on driver being fully loaded
and accessing the command is creating a chicken and egg problem.
Provisioning is much more likely to use some new admin commands
over an owner device. So this command is useless for it.



> > 
> > > > Patch 5/5 mandates that device validates all fields already.
> > >
> > > > Are there guests that will actually look at these caps as opposed to
> > > > just sending commands and looking at the return status?
> > > When the device reaches the limit it would not even send the command.
> > 
> > 
> > That's more logic for the driver to implement. Why should it bother when it
> > can just send it?
> >
> It can just send it and fail and flood with error logs.

If the spec says device can fail and that is expected then it won't flood.
If you don't want driver to send some commands you should say that
instead of saying device should filter them.

> Also it cannot just send some random id based numbers that device does not support.

Aha. I think I'm beginning to guess. You wrote:
	field{max_groups} indicates total number of flow filter groups supported
	by the device whose group identifiers can be any value in the range from 0 to
	\field{max_groups - 1}.
and you think this implies that it can't be out of that range.
That's not how logic works though ;)


> > > For example if the device supports only one group, it wont implement
> > ethtool ntuple and will chose only arfs as one example.
> > 
> > Sounds like a contrived example why does device even expose flow steering
> > then?
> It is not contrived at all.
> 
> A device may be support one or multiple groups. Each group in the OS has multiple users.
> One group is enough for either ARFS or ethool but not both.
> Two groups can allow ARFS and ethtool to co-exists.
> 3 groups will allow in future to expose some queues to user apps.

Oh I misunderstood.
Then why would driver decide to force a decision to ARFS
specifically?
It will likely leave this to the user.

-- 
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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Friday, November 24, 2023 11:29 AM
> 
> On Fri, Nov 24, 2023 at 02:57:49AM +0000, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Friday, November 24, 2023 4:28 AM
> > >
> > > On Thu, Nov 23, 2023 at 06:40:59PM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Thursday, November 23, 2023 7:44 PM
> > > > >
> > > > > On Thu, Nov 23, 2023 at 11:21:16AM +0200, Parav Pandit wrote:
> > > > > > The device responds flow filter capabilities using two commands.
> > > > > > One command indicates generic flow filter device limits such
> > > > > > as number of flow filters, number of flow filter groups,
> > > > > > support or multiple transports etc.
> > > > > >
> > > > > > The second command indicates supported match types, and fields
> > > > > > of the packet.
> > > > > >
> > > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
> > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > > >
> > > > > So I am still unsure about these commands.  What exactly is the point?
> > > > >
> > > > Two reasons.
> > > > The device reports maximum capabilities that the driver knows
> > > > upfront. So
> > > that it does not need to come to device to know when to fail.
> > >
> > > But that requires driver to keep extra state which can easily get out of sync.
> > Not sure what you mean by get out of sync from whom?
> > Not from the device.
> 
> yes from the device. I can't say for sure how because I now see this patch is
> implying driver requirements that you didn't document. I thought the IDs can
> be any number. 
It cannot be any number. The device requirement has captured it.
Couldn't see a lot of point of duplicating it.
The device requirement implies what driver has to do.
But if you insist, I can duplicate in the driver requirement too.

> It appears that's not what you meant.
> I can try and guess what you meant but I'd rather have you rewrite this in a
> way that makes the meaning explicit.
> 
If you insist, I can duplicate in driver requirements too.
> 
> > There are many capabilities here each with a different use.
> 
> And you are under the impression that dumping all kind of stuff in a single
> place is good somehow?
It is not dumped.
It is structured based on the functionality.
The config space proposal tends to dump everything in unstructured way at single place that cannot extended elegantly.

For example, Xuan's dynamic array cannot be placed next to other dynamic array of flow filter.

> 
> > For many caps, it is to know what is supported/not to decide in the driver.
> 
> I can't parse this sentence.
> 
The driver checks things upfront what is supported while serving the command, instead of random trial and error guess work game with he device.

> > Certain caps are max to know what is the range of id that driver can use like
> flow filter id, group id.
> 
> Maybe then. But there's apparently no requirement for either device or driver
> to put it in this range.
> 

I will add this requirement statements in device requirements and that will make things clear.
Good point.

> > > For what benefit?
> > >
> > For functional work.
> 
> You don't really explain what kind of work in this patchset, or in the commit
> logs. Maybe it's obvious for you but not for everyone.
What text are you expecting?
We have undergone all the requirements study that you conveniently choose to not participate for 2 months.

> 
> > > > In future one may want to provision certain max limits that device
> > > > can have
> > > as they eventually will consume certain device hardware resources.
> > >
> > > I don't exactly see how this helps provisioning.
> > >
> > Provisioning side will set parameters which will reflect these limits to be same
> on src and dst hypervisor when device migration is used.
> 
> But making provisioning depend on driver being fully loaded and accessing the
> command is creating a chicken and egg problem.
I didn't explain well likely.
Provisioning as you described, will be on owner device.
This command tells driver what is provisioned.

> Provisioning is much more likely to use some new admin commands over an
> owner device. So this command is useless for it.
> 
Provisioning owner device will use exact same structure while provisioning.

> 
> 
> > >
> > > > > Patch 5/5 mandates that device validates all fields already.
> > > >
> > > > > Are there guests that will actually look at these caps as
> > > > > opposed to just sending commands and looking at the return status?
> > > > When the device reaches the limit it would not even send the command.
> > >
> > >
> > > That's more logic for the driver to implement. Why should it bother
> > > when it can just send it?
> > >
> > It can just send it and fail and flood with error logs.
> 
> If the spec says device can fail and that is expected then it won't flood.
> If you don't want driver to send some commands you should say that instead
> of saying device should filter them.
I will add the normative in the driver requirement section.

> 
> > Also it cannot just send some random id based numbers that device does
> not support.
> 
> Aha. I think I'm beginning to guess. You wrote:
> 	field{max_groups} indicates total number of flow filter groups
> supported
> 	by the device whose group identifiers can be any value in the range
> from 0 to
> 	\field{max_groups - 1}.
> and you think this implies that it can't be out of that range.
> That's not how logic works though ;)
> 
Which logic?
The driver will honor the limit given to him.
The device will also naturally honor what it published to driver.

> 
> > > > For example if the device supports only one group, it wont
> > > > implement
> > > ethtool ntuple and will chose only arfs as one example.
> > >
> > > Sounds like a contrived example why does device even expose flow
> > > steering then?
> > It is not contrived at all.
> >
> > A device may be support one or multiple groups. Each group in the OS has
> multiple users.
> > One group is enough for either ARFS or ethool but not both.
> > Two groups can allow ARFS and ethtool to co-exists.
> > 3 groups will allow in future to expose some queues to user apps.
> 
> Oh I misunderstood.
> Then why would driver decide to force a decision to ARFS specifically?
> It will likely leave this to the user.
It is the driver implementation choice,
For example: 
if there is only one group, driver can choose to give strong preference, say for ARFS only.

Or it can let it be on first come first usage, for example, if ethtool wants to use first, it will allow it, not letting ARFS to work.




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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Michael S. Tsirkin]

On Fri, Nov 24, 2023 at 06:27:34AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Friday, November 24, 2023 11:29 AM
> > 
> > On Fri, Nov 24, 2023 at 02:57:49AM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Friday, November 24, 2023 4:28 AM
> > > >
> > > > On Thu, Nov 23, 2023 at 06:40:59PM +0000, Parav Pandit wrote:
> > > > >
> > > > >
> > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Sent: Thursday, November 23, 2023 7:44 PM
> > > > > >
> > > > > > On Thu, Nov 23, 2023 at 11:21:16AM +0200, Parav Pandit wrote:
> > > > > > > The device responds flow filter capabilities using two commands.
> > > > > > > One command indicates generic flow filter device limits such
> > > > > > > as number of flow filters, number of flow filter groups,
> > > > > > > support or multiple transports etc.
> > > > > > >
> > > > > > > The second command indicates supported match types, and fields
> > > > > > > of the packet.
> > > > > > >
> > > > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
> > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > > > >
> > > > > > So I am still unsure about these commands.  What exactly is the point?
> > > > > >
> > > > > Two reasons.
> > > > > The device reports maximum capabilities that the driver knows
> > > > > upfront. So
> > > > that it does not need to come to device to know when to fail.
> > > >
> > > > But that requires driver to keep extra state which can easily get out of sync.
> > > Not sure what you mean by get out of sync from whom?
> > > Not from the device.
> > 
> > yes from the device. I can't say for sure how because I now see this patch is
> > implying driver requirements that you didn't document. I thought the IDs can
> > be any number. 
> It cannot be any number. The device requirement has captured it.

Which requirement?

> Couldn't see a lot of point of duplicating it.
> The device requirement implies what driver has to do.

Nothing of the sort.

> But if you insist, I can duplicate in the driver requirement too.

I think spec should list the requirements we have not imply them.
In particular driver authors should be able to go over
driver conformance sections, use non-normative sections
to interpret them and then verify their conformance.
Device normative sections is not something that should interest them.
We don't always do a good job of this - not a reason to keep adding
to that problem.

> > It appears that's not what you meant.
> > I can try and guess what you meant but I'd rather have you rewrite this in a
> > way that makes the meaning explicit.
> > 
> If you insist, I can duplicate in driver requirements too.
> > 
> > > There are many capabilities here each with a different use.
> > 
> > And you are under the impression that dumping all kind of stuff in a single
> > place is good somehow?
> It is not dumped.
> It is structured based on the functionality.
> The config space proposal tends to dump everything in unstructured way at single place that cannot extended elegantly.
> 
> For example, Xuan's dynamic array cannot be placed next to other dynamic array of flow filter.

I don't know what "dynamic array" is exactly but generally,
Any kind of a big array is not appropriate in config space - I don't see any
of these here, though.


> > 
> > > For many caps, it is to know what is supported/not to decide in the driver.
> > 
> > I can't parse this sentence.
> > 
> The driver checks things upfront what is supported while serving the command, instead of random trial and error guess work game with he device.

There's no real guesswork with these specific commands: user types
an ethtool command, driver sends it on, it either succeeds or fails.
However if you feel it important to do checks in driver - OK.
But that makes cap query an init time thing.


> > > Certain caps are max to know what is the range of id that driver can use like
> > flow filter id, group id.
> > 
> > Maybe then. But there's apparently no requirement for either device or driver
> > to put it in this range.
> > 
> 
> I will add this requirement statements in device requirements and that will make things clear.
> Good point.
> 
> > > > For what benefit?
> > > >
> > > For functional work.
> > 
> > You don't really explain what kind of work in this patchset, or in the commit
> > logs. Maybe it's obvious for you but not for everyone.
> What text are you expecting?
> We have undergone all the requirements study that you conveniently choose to not participate for 2 months.

Yea ... I'm sorry. Things have been going on here, man.
I'm glad I'm back and able to participate though.

> > 
> > > > > In future one may want to provision certain max limits that device
> > > > > can have
> > > > as they eventually will consume certain device hardware resources.
> > > >
> > > > I don't exactly see how this helps provisioning.
> > > >
> > > Provisioning side will set parameters which will reflect these limits to be same
> > on src and dst hypervisor when device migration is used.
> > 
> > But making provisioning depend on driver being fully loaded and accessing the
> > command is creating a chicken and egg problem.
> I didn't explain well likely.
> Provisioning as you described, will be on owner device.
> This command tells driver what is provisioned.
> 
> > Provisioning is much more likely to use some new admin commands over an
> > owner device. So this command is useless for it.
> > 
> Provisioning owner device will use exact same structure while provisioning.

So the advantage of using config space is that we can have a single
generic provision command that gets device config space format. Your
approach will need some kind of net device specific thing.


> > 
> > 
> > > >
> > > > > > Patch 5/5 mandates that device validates all fields already.
> > > > >
> > > > > > Are there guests that will actually look at these caps as
> > > > > > opposed to just sending commands and looking at the return status?
> > > > > When the device reaches the limit it would not even send the command.
> > > >
> > > >
> > > > That's more logic for the driver to implement. Why should it bother
> > > > when it can just send it?
> > > >
> > > It can just send it and fail and flood with error logs.
> > 
> > If the spec says device can fail and that is expected then it won't flood.
> > If you don't want driver to send some commands you should say that instead
> > of saying device should filter them.
> I will add the normative in the driver requirement section.
> 
> > 
> > > Also it cannot just send some random id based numbers that device does
> > not support.
> > 
> > Aha. I think I'm beginning to guess. You wrote:
> > 	field{max_groups} indicates total number of flow filter groups
> > supported
> > 	by the device whose group identifiers can be any value in the range
> > from 0 to
> > 	\field{max_groups - 1}.
> > and you think this implies that it can't be out of that range.
> > That's not how logic works though ;)
> > 
> Which logic?

The aristotelian logic we commonly use.  You say "group identifiers can
be any value in the range".  You seem to mean "group identifiers can not
be any value not in the range". These two statements are not
equivalent and neither implies the other.


> The driver will honor the limit given to him.
> The device will also naturally honor what it published to driver.
> > 
> > > > > For example if the device supports only one group, it wont
> > > > > implement
> > > > ethtool ntuple and will chose only arfs as one example.
> > > >
> > > > Sounds like a contrived example why does device even expose flow
> > > > steering then?
> > > It is not contrived at all.
> > >
> > > A device may be support one or multiple groups. Each group in the OS has
> > multiple users.
> > > One group is enough for either ARFS or ethool but not both.
> > > Two groups can allow ARFS and ethtool to co-exists.
> > > 3 groups will allow in future to expose some queues to user apps.
> > 
> > Oh I misunderstood.
> > Then why would driver decide to force a decision to ARFS specifically?
> > It will likely leave this to the user.
> It is the driver implementation choice,
> For example: 
> if there is only one group, driver can choose to give strong preference, say for ARFS only.
> 
> Or it can let it be on first come first usage, for example, if ethtool wants to use first, it will allow it, not letting ARFS to work.
> 

Practically, drivers are highly unlikely to make the choice.
That is why it is reasonable not to have checks in the
driver if device is doing them anyway.
But OTOH if you want driver to do the checks then I don't see why we
should also add them to device unless there's a good reason to.
Less checks in devices -> cheaper simpler devices.

-- 
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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Friday, November 24, 2023 3:45 PM
> 
> On Fri, Nov 24, 2023 at 06:27:34AM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Friday, November 24, 2023 11:29 AM
> > >
> > > On Fri, Nov 24, 2023 at 02:57:49AM +0000, Parav Pandit wrote:
> > > >
> > > >
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Friday, November 24, 2023 4:28 AM
> > > > >
> > > > > On Thu, Nov 23, 2023 at 06:40:59PM +0000, Parav Pandit wrote:
> > > > > >
> > > > > >
> > > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > Sent: Thursday, November 23, 2023 7:44 PM
> > > > > > >
> > > > > > > On Thu, Nov 23, 2023 at 11:21:16AM +0200, Parav Pandit wrote:
> > > > > > > > The device responds flow filter capabilities using two commands.
> > > > > > > > One command indicates generic flow filter device limits
> > > > > > > > such as number of flow filters, number of flow filter
> > > > > > > > groups, support or multiple transports etc.
> > > > > > > >
> > > > > > > > The second command indicates supported match types, and
> > > > > > > > fields of the packet.
> > > > > > > >
> > > > > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/179
> > > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > > > > >
> > > > > > > So I am still unsure about these commands.  What exactly is the
> point?
> > > > > > >
> > > > > > Two reasons.
> > > > > > The device reports maximum capabilities that the driver knows
> > > > > > upfront. So
> > > > > that it does not need to come to device to know when to fail.
> > > > >
> > > > > But that requires driver to keep extra state which can easily get out of
> sync.
> > > > Not sure what you mean by get out of sync from whom?
> > > > Not from the device.
> > >
> > > yes from the device. I can't say for sure how because I now see this
> > > patch is implying driver requirements that you didn't document. I
> > > thought the IDs can be any number.
> > It cannot be any number. The device requirement has captured it.
> 
> Which requirement?
> 
This is the one I replied before that I will add as explicitly in device and driver requirements section.
I can see a disconnect for you.

> > Couldn't see a lot of point of duplicating it.
> > The device requirement implies what driver has to do.
> 
> Nothing of the sort.
> 
> > But if you insist, I can duplicate in the driver requirement too.
> 
> I think spec should list the requirements we have not imply them.
> In particular driver authors should be able to go over driver conformance
> sections, use non-normative sections to interpret them and then verify their
> conformance.
> Device normative sections is not something that should interest them.
> We don't always do a good job of this - not a reason to keep adding to that
> problem.
> 
Ok. Sounds fine. I will add the driver requirements for max limits.

> > > It appears that's not what you meant.
> > > I can try and guess what you meant but I'd rather have you rewrite
> > > this in a way that makes the meaning explicit.
> > >
> > If you insist, I can duplicate in driver requirements too.
> > >
> > > > There are many capabilities here each with a different use.
> > >
> > > And you are under the impression that dumping all kind of stuff in a
> > > single place is good somehow?
> > It is not dumped.
> > It is structured based on the functionality.
> > The config space proposal tends to dump everything in unstructured way at
> single place that cannot extended elegantly.
> >
> > For example, Xuan's dynamic array cannot be placed next to other dynamic
> array of flow filter.
> 
> I don't know what "dynamic array" is exactly but generally, Any kind of a big
> array is not appropriate in config space - I don't see any of these here, though.
Ah, now I see the disconnect.

There is.
struct virtio_net_ctrl_ff_match_types

> 
> 
> > >
> > > > For many caps, it is to know what is supported/not to decide in the
> driver.
> > >
> > > I can't parse this sentence.
> > >
> > The driver checks things upfront what is supported while serving the
> command, instead of random trial and error guess work game with he device.
> 
> There's no real guesswork with these specific commands: user types an
> ethtool command, driver sends it on, it either succeeds or fails.
It is a guess work without driver knowing if its supported or not.

> However if you feel it important to do checks in driver - OK.
Yes, because there are many parsing fields and good to know.

> But that makes cap query an init time thing.
Yes, caps does not change dynamically.
But sure it is different among different member devices and owner too.

> 
> 
> > > > Certain caps are max to know what is the range of id that driver
> > > > can use like
> > > flow filter id, group id.
> > >
> > > Maybe then. But there's apparently no requirement for either device
> > > or driver to put it in this range.
> > >
> >
> > I will add this requirement statements in device requirements and that will
> make things clear.
> > Good point.
> >
> > > > > For what benefit?
> > > > >
> > > > For functional work.
> > >
> > > You don't really explain what kind of work in this patchset, or in
> > > the commit logs. Maybe it's obvious for you but not for everyone.
> > What text are you expecting?
> > We have undergone all the requirements study that you conveniently
> choose to not participate for 2 months.
> 
> Yea ... I'm sorry. Things have been going on here, man.
> I'm glad I'm back and able to participate though.
> 
Great.

> > >
> > > > > > In future one may want to provision certain max limits that
> > > > > > device can have
> > > > > as they eventually will consume certain device hardware resources.
> > > > >
> > > > > I don't exactly see how this helps provisioning.
> > > > >
> > > > Provisioning side will set parameters which will reflect these
> > > > limits to be same
> > > on src and dst hypervisor when device migration is used.
> > >
> > > But making provisioning depend on driver being fully loaded and
> > > accessing the command is creating a chicken and egg problem.
> > I didn't explain well likely.
> > Provisioning as you described, will be on owner device.
> > This command tells driver what is provisioned.
> >
> > > Provisioning is much more likely to use some new admin commands over
> > > an owner device. So this command is useless for it.
> > >
> > Provisioning owner device will use exact same structure while provisioning.
> 
> So the advantage of using config space is that we can have a single generic
> provision command that gets device config space format. Your approach will
> need some kind of net device specific thing.
> 

I see the advantage. But it very small and largely hidden beneath the sw layers how a provisioning command sets things.
A provisioning command sets the value, it surfaces at different level.

Multiple device vendors want to follow the design pattern to use " most uses it is better to use a virtqueue to
update configuration information"

Instead of RO = config space, RW = cvq.

The more pragmatic design pattern is:
driver initialization time => feature bit + virtio config space.
Driver runtime => cvq.

> 
> > >
> > >
> > > > >
> > > > > > > Patch 5/5 mandates that device validates all fields already.
> > > > > >
> > > > > > > Are there guests that will actually look at these caps as
> > > > > > > opposed to just sending commands and looking at the return
> status?
> > > > > > When the device reaches the limit it would not even send the
> command.
> > > > >
> > > > >
> > > > > That's more logic for the driver to implement. Why should it
> > > > > bother when it can just send it?
> > > > >
> > > > It can just send it and fail and flood with error logs.
> > >
> > > If the spec says device can fail and that is expected then it won't flood.
> > > If you don't want driver to send some commands you should say that
> > > instead of saying device should filter them.
> > I will add the normative in the driver requirement section.
> >
> > >
> > > > Also it cannot just send some random id based numbers that device
> > > > does
> > > not support.
> > >
> > > Aha. I think I'm beginning to guess. You wrote:
> > > 	field{max_groups} indicates total number of flow filter groups
> > > supported
> > > 	by the device whose group identifiers can be any value in the range
> > > from 0 to
> > > 	\field{max_groups - 1}.
> > > and you think this implies that it can't be out of that range.
> > > That's not how logic works though ;)
> > >
> > Which logic?
> 
> The aristotelian logic we commonly use.  You say "group identifiers can be any
> value in the range".  You seem to mean "group identifiers can not be any value
> not in the range". These two statements are not equivalent and neither implies
> the other.
I fail to see the difference between the two English sentence you wrote.
Can you please have an example, how second is any different?

In my example max_groups = 5, so group id must be from 0 to 4.
It can be any value.

In device and driver requirements, I will write the explicit line that it MUST be in this range.

> 
> 
> > The driver will honor the limit given to him.
> > The device will also naturally honor what it published to driver.
> > >
> > > > > > For example if the device supports only one group, it wont
> > > > > > implement
> > > > > ethtool ntuple and will chose only arfs as one example.
> > > > >
> > > > > Sounds like a contrived example why does device even expose flow
> > > > > steering then?
> > > > It is not contrived at all.
> > > >
> > > > A device may be support one or multiple groups. Each group in the
> > > > OS has
> > > multiple users.
> > > > One group is enough for either ARFS or ethool but not both.
> > > > Two groups can allow ARFS and ethtool to co-exists.
> > > > 3 groups will allow in future to expose some queues to user apps.
> > >
> > > Oh I misunderstood.
> > > Then why would driver decide to force a decision to ARFS specifically?
> > > It will likely leave this to the user.
> > It is the driver implementation choice, For example:
> > if there is only one group, driver can choose to give strong preference, say for
> ARFS only.
> >
> > Or it can let it be on first come first usage, for example, if ethtool wants to
> use first, it will allow it, not letting ARFS to work.
> >
> 
> Practically, drivers are highly unlikely to make the choice.
It will make the choice when there is only one group available.

> That is why it is reasonable not to have checks in the driver if device is doing
> them anyway.
The groups particularly have the priority as well. So the check in device does not work.

> But OTOH if you want driver to do the checks then I don't see why we should
> also add them to device unless there's a good reason to.
> Less checks in devices -> cheaper simpler devices.
> 
The cheaper devices can always place UINT_32 max values and ignore any checking etc.

The check in the device will be done as it uses such an id for its own bookkeeping.

> --
> 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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Michael S. Tsirkin]

On Mon, Nov 27, 2023 at 10:19:47AM +0000, Parav Pandit wrote:
> > > > It appears that's not what you meant.
> > > > I can try and guess what you meant but I'd rather have you rewrite
> > > > this in a way that makes the meaning explicit.
> > > >
> > > If you insist, I can duplicate in driver requirements too.
> > > >
> > > > > There are many capabilities here each with a different use.
> > > >
> > > > And you are under the impression that dumping all kind of stuff in a
> > > > single place is good somehow?
> > > It is not dumped.
> > > It is structured based on the functionality.
> > > The config space proposal tends to dump everything in unstructured way at
> > single place that cannot extended elegantly.
> > >
> > > For example, Xuan's dynamic array cannot be placed next to other dynamic
> > array of flow filter.
> > 
> > I don't know what "dynamic array" is exactly but generally, Any kind of a big
> > array is not appropriate in config space - I don't see any of these here, though.
> Ah, now I see the disconnect.
> 
> There is.
> struct virtio_net_ctrl_ff_match_types

Now I think I don't understand what this array does at all.
Why are there multiple entries with each entry also including
a mask of multiple fields?


> > 
> > 
> > > >
> > > > > For many caps, it is to know what is supported/not to decide in the
> > driver.
> > > >
> > > > I can't parse this sentence.
> > > >
> > > The driver checks things upfront what is supported while serving the
> > command, instead of random trial and error guess work game with he device.
> > 
> > There's no real guesswork with these specific commands: user types an
> > ethtool command, driver sends it on, it either succeeds or fails.
> It is a guess work without driver knowing if its supported or not.

What we do not need is both driver and device checking
these values.


> > However if you feel it important to do checks in driver - OK.
> Yes, because there are many parsing fields and good to know.
> 
> > But that makes cap query an init time thing.
> Yes, caps does not change dynamically.
> But sure it is different among different member devices and owner too.
> 
> > 
> > 
> > > > > Certain caps are max to know what is the range of id that driver
> > > > > can use like
> > > > flow filter id, group id.
> > > >
> > > > Maybe then. But there's apparently no requirement for either device
> > > > or driver to put it in this range.
> > > >
> > >
> > > I will add this requirement statements in device requirements and that will
> > make things clear.
> > > Good point.
> > >
> > > > > > For what benefit?
> > > > > >
> > > > > For functional work.
> > > >
> > > > You don't really explain what kind of work in this patchset, or in
> > > > the commit logs. Maybe it's obvious for you but not for everyone.
> > > What text are you expecting?
> > > We have undergone all the requirements study that you conveniently
> > choose to not participate for 2 months.
> > 
> > Yea ... I'm sorry. Things have been going on here, man.
> > I'm glad I'm back and able to participate though.
> > 
> Great.
> 
> > > >
> > > > > > > In future one may want to provision certain max limits that
> > > > > > > device can have
> > > > > > as they eventually will consume certain device hardware resources.
> > > > > >
> > > > > > I don't exactly see how this helps provisioning.
> > > > > >
> > > > > Provisioning side will set parameters which will reflect these
> > > > > limits to be same
> > > > on src and dst hypervisor when device migration is used.
> > > >
> > > > But making provisioning depend on driver being fully loaded and
> > > > accessing the command is creating a chicken and egg problem.
> > > I didn't explain well likely.
> > > Provisioning as you described, will be on owner device.
> > > This command tells driver what is provisioned.
> > >
> > > > Provisioning is much more likely to use some new admin commands over
> > > > an owner device. So this command is useless for it.
> > > >
> > > Provisioning owner device will use exact same structure while provisioning.
> > 
> > So the advantage of using config space is that we can have a single generic
> > provision command that gets device config space format. Your approach will
> > need some kind of net device specific thing.
> > 
> 
> I see the advantage. But it very small and largely hidden beneath the sw layers how a provisioning command sets things.
> A provisioning command sets the value, it surfaces at different level.
> 
> Multiple device vendors want to follow the design pattern to use " most uses it is better to use a virtqueue to
> update configuration information"
> 
> Instead of RO = config space, RW = cvq.
> 
> The more pragmatic design pattern is:
> driver initialization time => feature bit + virtio config space.
> Driver runtime => cvq.

yes. capability check is initialization time though.


> > 
> > > >
> > > >
> > > > > >
> > > > > > > > Patch 5/5 mandates that device validates all fields already.
> > > > > > >
> > > > > > > > Are there guests that will actually look at these caps as
> > > > > > > > opposed to just sending commands and looking at the return
> > status?
> > > > > > > When the device reaches the limit it would not even send the
> > command.
> > > > > >
> > > > > >
> > > > > > That's more logic for the driver to implement. Why should it
> > > > > > bother when it can just send it?
> > > > > >
> > > > > It can just send it and fail and flood with error logs.
> > > >
> > > > If the spec says device can fail and that is expected then it won't flood.
> > > > If you don't want driver to send some commands you should say that
> > > > instead of saying device should filter them.
> > > I will add the normative in the driver requirement section.
> > >
> > > >
> > > > > Also it cannot just send some random id based numbers that device
> > > > > does
> > > > not support.
> > > >
> > > > Aha. I think I'm beginning to guess. You wrote:
> > > > 	field{max_groups} indicates total number of flow filter groups
> > > > supported
> > > > 	by the device whose group identifiers can be any value in the range
> > > > from 0 to
> > > > 	\field{max_groups - 1}.
> > > > and you think this implies that it can't be out of that range.
> > > > That's not how logic works though ;)
> > > >
> > > Which logic?
> > 
> > The aristotelian logic we commonly use.  You say "group identifiers can be any
> > value in the range".  You seem to mean "group identifiers can not be any value
> > not in the range". These two statements are not equivalent and neither implies
> > the other.
> I fail to see the difference between the two English sentence you wrote.
> Can you please have an example, how second is any different?
> 
> In my example max_groups = 5, so group id must be from 0 to 4.

No, your sentence does not say it must. It says it can be in range.
Can it be out of range, e.g. 5? You don't state either way.
The second sentence says it can not be out of range so 5 is illegal.
Are there illegal values inside the range? E.g. might 3 be illegal?
According to second sentence, maybe. According to 1st sentence, 3
is legal.

> It can be any value.

:(


> In device and driver requirements, I will write the explicit line that it MUST be in this range.

Then it will be different.

> > 
> > 
> > > The driver will honor the limit given to him.
> > > The device will also naturally honor what it published to driver.
> > > >
> > > > > > > For example if the device supports only one group, it wont
> > > > > > > implement
> > > > > > ethtool ntuple and will chose only arfs as one example.
> > > > > >
> > > > > > Sounds like a contrived example why does device even expose flow
> > > > > > steering then?
> > > > > It is not contrived at all.
> > > > >
> > > > > A device may be support one or multiple groups. Each group in the
> > > > > OS has
> > > > multiple users.
> > > > > One group is enough for either ARFS or ethool but not both.
> > > > > Two groups can allow ARFS and ethtool to co-exists.
> > > > > 3 groups will allow in future to expose some queues to user apps.
> > > >
> > > > Oh I misunderstood.
> > > > Then why would driver decide to force a decision to ARFS specifically?
> > > > It will likely leave this to the user.
> > > It is the driver implementation choice, For example:
> > > if there is only one group, driver can choose to give strong preference, say for
> > ARFS only.
> > >
> > > Or it can let it be on first come first usage, for example, if ethtool wants to
> > use first, it will allow it, not letting ARFS to work.
> > >
> > 
> > Practically, drivers are highly unlikely to make the choice.
> It will make the choice when there is only one group available.
> 
> > That is why it is reasonable not to have checks in the driver if device is doing
> > them anyway.
> The groups particularly have the priority as well. So the check in device does not work.

I don't know what that means.

> > But OTOH if you want driver to do the checks then I don't see why we should
> > also add them to device unless there's a good reason to.
> > Less checks in devices -> cheaper simpler devices.
> > 
> The cheaper devices can always place UINT_32 max values and ignore any checking etc.

Then won't driver expect inifinite # of groups to be supported?

> The check in the device will be done as it uses such an id for its own bookkeeping.
> 
I don't know what that means.
> > --
> > 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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, November 27, 2023 4:53 PM
> 
> On Mon, Nov 27, 2023 at 10:19:47AM +0000, Parav Pandit wrote:
> > > > > It appears that's not what you meant.
> > > > > I can try and guess what you meant but I'd rather have you
> > > > > rewrite this in a way that makes the meaning explicit.
> > > > >
> > > > If you insist, I can duplicate in driver requirements too.
> > > > >
> > > > > > There are many capabilities here each with a different use.
> > > > >
> > > > > And you are under the impression that dumping all kind of stuff
> > > > > in a single place is good somehow?
> > > > It is not dumped.
> > > > It is structured based on the functionality.
> > > > The config space proposal tends to dump everything in unstructured
> > > > way at
> > > single place that cannot extended elegantly.
> > > >
> > > > For example, Xuan's dynamic array cannot be placed next to other
> > > > dynamic
> > > array of flow filter.
> > >
> > > I don't know what "dynamic array" is exactly but generally, Any kind
> > > of a big array is not appropriate in config space - I don't see any of these
> here, though.
> > Ah, now I see the disconnect.
> >
> > There is.
> > struct virtio_net_ctrl_ff_match_types
> 
> Now I think I don't understand what this array does at all.
> Why are there multiple entries with each entry also including a mask of
> multiple fields?
> 
Each entry indicates a bitmask of supported fields that can be filtered.
The type indicates how to interpret each fields_map.
Any new type for MPLS can be added easily by defining its type and mask.
And same type enums and field mask also used by the data path too.

> 
> > >
> > >
> > > > >
> > > > > > For many caps, it is to know what is supported/not to decide
> > > > > > in the
> > > driver.
> > > > >
> > > > > I can't parse this sentence.
> > > > >
> > > > The driver checks things upfront what is supported while serving
> > > > the
> > > command, instead of random trial and error guess work game with he
> device.
> > >
> > > There's no real guesswork with these specific commands: user types
> > > an ethtool command, driver sends it on, it either succeeds or fails.
> > It is a guess work without driver knowing if its supported or not.
> 
> What we do not need is both driver and device checking these values.
> 
Driver is mainly to using to know its valid ranges and to fail the commands early.
Device is referring it for any validations that it may need to do.

> 
> > > However if you feel it important to do checks in driver - OK.
> > Yes, because there are many parsing fields and good to know.
> >
> > > But that makes cap query an init time thing.
> > Yes, caps does not change dynamically.
> > But sure it is different among different member devices and owner too.
> >
> > >
> > >
> > > > > > Certain caps are max to know what is the range of id that
> > > > > > driver can use like
> > > > > flow filter id, group id.
> > > > >
> > > > > Maybe then. But there's apparently no requirement for either
> > > > > device or driver to put it in this range.
> > > > >
> > > >
> > > > I will add this requirement statements in device requirements and
> > > > that will
> > > make things clear.
> > > > Good point.
> > > >
> > > > > > > For what benefit?
> > > > > > >
> > > > > > For functional work.
> > > > >
> > > > > You don't really explain what kind of work in this patchset, or
> > > > > in the commit logs. Maybe it's obvious for you but not for everyone.
> > > > What text are you expecting?
> > > > We have undergone all the requirements study that you conveniently
> > > choose to not participate for 2 months.
> > >
> > > Yea ... I'm sorry. Things have been going on here, man.
> > > I'm glad I'm back and able to participate though.
> > >
> > Great.
> >
> > > > >
> > > > > > > > In future one may want to provision certain max limits
> > > > > > > > that device can have
> > > > > > > as they eventually will consume certain device hardware resources.
> > > > > > >
> > > > > > > I don't exactly see how this helps provisioning.
> > > > > > >
> > > > > > Provisioning side will set parameters which will reflect these
> > > > > > limits to be same
> > > > > on src and dst hypervisor when device migration is used.
> > > > >
> > > > > But making provisioning depend on driver being fully loaded and
> > > > > accessing the command is creating a chicken and egg problem.
> > > > I didn't explain well likely.
> > > > Provisioning as you described, will be on owner device.
> > > > This command tells driver what is provisioned.
> > > >
> > > > > Provisioning is much more likely to use some new admin commands
> > > > > over an owner device. So this command is useless for it.
> > > > >
> > > > Provisioning owner device will use exact same structure while
> provisioning.
> > >
> > > So the advantage of using config space is that we can have a single
> > > generic provision command that gets device config space format. Your
> > > approach will need some kind of net device specific thing.
> > >
> >
> > I see the advantage. But it very small and largely hidden beneath the sw
> layers how a provisioning command sets things.
> > A provisioning command sets the value, it surfaces at different level.
> >
> > Multiple device vendors want to follow the design pattern to use "
> > most uses it is better to use a virtqueue to update configuration information"
> >
> > Instead of RO = config space, RW = cvq.
> >
> > The more pragmatic design pattern is:
> > driver initialization time => feature bit + virtio config space.
> > Driver runtime => cvq.
> 
> yes. capability check is initialization time though.
> 
Right, for runtime config like this cap, it is not in the initialization time needed.

> 
> > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > > > Patch 5/5 mandates that device validates all fields already.
> > > > > > > >
> > > > > > > > > Are there guests that will actually look at these caps
> > > > > > > > > as opposed to just sending commands and looking at the
> > > > > > > > > return
> > > status?
> > > > > > > > When the device reaches the limit it would not even send
> > > > > > > > the
> > > command.
> > > > > > >
> > > > > > >
> > > > > > > That's more logic for the driver to implement. Why should it
> > > > > > > bother when it can just send it?
> > > > > > >
> > > > > > It can just send it and fail and flood with error logs.
> > > > >
> > > > > If the spec says device can fail and that is expected then it won't flood.
> > > > > If you don't want driver to send some commands you should say
> > > > > that instead of saying device should filter them.
> > > > I will add the normative in the driver requirement section.
> > > >
> > > > >
> > > > > > Also it cannot just send some random id based numbers that
> > > > > > device does
> > > > > not support.
> > > > >
> > > > > Aha. I think I'm beginning to guess. You wrote:
> > > > > 	field{max_groups} indicates total number of flow filter groups
> > > > > supported
> > > > > 	by the device whose group identifiers can be any value in the
> > > > > range from 0 to
> > > > > 	\field{max_groups - 1}.
> > > > > and you think this implies that it can't be out of that range.
> > > > > That's not how logic works though ;)
> > > > >
> > > > Which logic?
> > >
> > > The aristotelian logic we commonly use.  You say "group identifiers
> > > can be any value in the range".  You seem to mean "group identifiers
> > > can not be any value not in the range". These two statements are not
> > > equivalent and neither implies the other.
> > I fail to see the difference between the two English sentence you wrote.
> > Can you please have an example, how second is any different?
> >
> > In my example max_groups = 5, so group id must be from 0 to 4.
> 
> No, your sentence does not say it must. It says it can be in range.
> Can it be out of range, e.g. 5? You don't state either way.
I understand. I will that normative in v7.

> The second sentence says it can not be out of range so 5 is illegal.
> Are there illegal values inside the range? E.g. might 3 be illegal?
> According to second sentence, maybe. According to 1st sentence, 3 is legal.
> 
> > It can be any value.
> 
> :(
> 
> 
> > In device and driver requirements, I will write the explicit line that it MUST be
> in this range.
> 
> Then it will be different.
Will write as two different normative respective for each section.
For driver it is SHOULD, for device it is MUST wherever applicable.

> 
> > >
> > >
> > > > The driver will honor the limit given to him.
> > > > The device will also naturally honor what it published to driver.
> > > > >
> > > > > > > > For example if the device supports only one group, it wont
> > > > > > > > implement
> > > > > > > ethtool ntuple and will chose only arfs as one example.
> > > > > > >
> > > > > > > Sounds like a contrived example why does device even expose
> > > > > > > flow steering then?
> > > > > > It is not contrived at all.
> > > > > >
> > > > > > A device may be support one or multiple groups. Each group in
> > > > > > the OS has
> > > > > multiple users.
> > > > > > One group is enough for either ARFS or ethool but not both.
> > > > > > Two groups can allow ARFS and ethtool to co-exists.
> > > > > > 3 groups will allow in future to expose some queues to user apps.
> > > > >
> > > > > Oh I misunderstood.
> > > > > Then why would driver decide to force a decision to ARFS specifically?
> > > > > It will likely leave this to the user.
> > > > It is the driver implementation choice, For example:
> > > > if there is only one group, driver can choose to give strong
> > > > preference, say for
> > > ARFS only.
> > > >
> > > > Or it can let it be on first come first usage, for example, if
> > > > ethtool wants to
> > > use first, it will allow it, not letting ARFS to work.
> > > >
> > >
> > > Practically, drivers are highly unlikely to make the choice.
> > It will make the choice when there is only one group available.
> >
> > > That is why it is reasonable not to have checks in the driver if
> > > device is doing them anyway.
> > The groups particularly have the priority as well. So the check in device does
> not work.
> 
> I don't know what that means.
> 
Packet processing order is defined by the group.
You should have participated in Aug or before when all of these were discussed.
Anyways, next time you can participate in other features.

> > > But OTOH if you want driver to do the checks then I don't see why we
> > > should also add them to device unless there's a good reason to.
> > > Less checks in devices -> cheaper simpler devices.
> > >
> > The cheaper devices can always place UINT_32 max values and ignore any
> checking etc.
> 
> Then won't driver expect inifinite # of groups to be supported?
> 
A cheaper device can place anything it wants.

> > The check in the device will be done as it uses such an id for its own
> bookkeeping.
> >
> I don't know what that means.
> > > --
> > > 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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Michael S. Tsirkin]

On Mon, Nov 27, 2023 at 11:33:45AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, November 27, 2023 4:53 PM
> > 
> > On Mon, Nov 27, 2023 at 10:19:47AM +0000, Parav Pandit wrote:
> > > > > > It appears that's not what you meant.
> > > > > > I can try and guess what you meant but I'd rather have you
> > > > > > rewrite this in a way that makes the meaning explicit.
> > > > > >
> > > > > If you insist, I can duplicate in driver requirements too.
> > > > > >
> > > > > > > There are many capabilities here each with a different use.
> > > > > >
> > > > > > And you are under the impression that dumping all kind of stuff
> > > > > > in a single place is good somehow?
> > > > > It is not dumped.
> > > > > It is structured based on the functionality.
> > > > > The config space proposal tends to dump everything in unstructured
> > > > > way at
> > > > single place that cannot extended elegantly.
> > > > >
> > > > > For example, Xuan's dynamic array cannot be placed next to other
> > > > > dynamic
> > > > array of flow filter.
> > > >
> > > > I don't know what "dynamic array" is exactly but generally, Any kind
> > > > of a big array is not appropriate in config space - I don't see any of these
> > here, though.
> > > Ah, now I see the disconnect.
> > >
> > > There is.
> > > struct virtio_net_ctrl_ff_match_types
> > 
> > Now I think I don't understand what this array does at all.
> > Why are there multiple entries with each entry also including a mask of
> > multiple fields?
> > 
> Each entry indicates a bitmask of supported fields that can be filtered.
> The type indicates how to interpret each fields_map.
> Any new type for MPLS can be added easily by defining its type and mask.
> And same type enums and field mask also used by the data path too.

Oh wait I think I begin to remember now. There are 12 bits defined so
far and you made it this huge array. Right? With code
to parse and format it all. Just add a 64 bit
field and it will last us long enough. No parsing
and formatting just #define.


> > 
> > > >
> > > >
> > > > > >
> > > > > > > For many caps, it is to know what is supported/not to decide
> > > > > > > in the
> > > > driver.
> > > > > >
> > > > > > I can't parse this sentence.
> > > > > >
> > > > > The driver checks things upfront what is supported while serving
> > > > > the
> > > > command, instead of random trial and error guess work game with he
> > device.
> > > >
> > > > There's no real guesswork with these specific commands: user types
> > > > an ethtool command, driver sends it on, it either succeeds or fails.
> > > It is a guess work without driver knowing if its supported or not.
> > 
> > What we do not need is both driver and device checking these values.
> > 
> Driver is mainly to using to know its valid ranges and to fail the commands early.
> Device is referring it for any validations that it may need to do.
> 
> > 
> > > > However if you feel it important to do checks in driver - OK.
> > > Yes, because there are many parsing fields and good to know.
> > >
> > > > But that makes cap query an init time thing.
> > > Yes, caps does not change dynamically.
> > > But sure it is different among different member devices and owner too.
> > >
> > > >
> > > >
> > > > > > > Certain caps are max to know what is the range of id that
> > > > > > > driver can use like
> > > > > > flow filter id, group id.
> > > > > >
> > > > > > Maybe then. But there's apparently no requirement for either
> > > > > > device or driver to put it in this range.
> > > > > >
> > > > >
> > > > > I will add this requirement statements in device requirements and
> > > > > that will
> > > > make things clear.
> > > > > Good point.
> > > > >
> > > > > > > > For what benefit?
> > > > > > > >
> > > > > > > For functional work.
> > > > > >
> > > > > > You don't really explain what kind of work in this patchset, or
> > > > > > in the commit logs. Maybe it's obvious for you but not for everyone.
> > > > > What text are you expecting?
> > > > > We have undergone all the requirements study that you conveniently
> > > > choose to not participate for 2 months.
> > > >
> > > > Yea ... I'm sorry. Things have been going on here, man.
> > > > I'm glad I'm back and able to participate though.
> > > >
> > > Great.
> > >
> > > > > >
> > > > > > > > > In future one may want to provision certain max limits
> > > > > > > > > that device can have
> > > > > > > > as they eventually will consume certain device hardware resources.
> > > > > > > >
> > > > > > > > I don't exactly see how this helps provisioning.
> > > > > > > >
> > > > > > > Provisioning side will set parameters which will reflect these
> > > > > > > limits to be same
> > > > > > on src and dst hypervisor when device migration is used.
> > > > > >
> > > > > > But making provisioning depend on driver being fully loaded and
> > > > > > accessing the command is creating a chicken and egg problem.
> > > > > I didn't explain well likely.
> > > > > Provisioning as you described, will be on owner device.
> > > > > This command tells driver what is provisioned.
> > > > >
> > > > > > Provisioning is much more likely to use some new admin commands
> > > > > > over an owner device. So this command is useless for it.
> > > > > >
> > > > > Provisioning owner device will use exact same structure while
> > provisioning.
> > > >
> > > > So the advantage of using config space is that we can have a single
> > > > generic provision command that gets device config space format. Your
> > > > approach will need some kind of net device specific thing.
> > > >
> > >
> > > I see the advantage. But it very small and largely hidden beneath the sw
> > layers how a provisioning command sets things.
> > > A provisioning command sets the value, it surfaces at different level.
> > >
> > > Multiple device vendors want to follow the design pattern to use "
> > > most uses it is better to use a virtqueue to update configuration information"
> > >
> > > Instead of RO = config space, RW = cvq.
> > >
> > > The more pragmatic design pattern is:
> > > driver initialization time => feature bit + virtio config space.
> > > Driver runtime => cvq.
> > 
> > yes. capability check is initialization time though.
> > 
> Right, for runtime config like this cap, it is not in the initialization time needed.
> 
> > 
> > > >
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > > > > Patch 5/5 mandates that device validates all fields already.
> > > > > > > > >
> > > > > > > > > > Are there guests that will actually look at these caps
> > > > > > > > > > as opposed to just sending commands and looking at the
> > > > > > > > > > return
> > > > status?
> > > > > > > > > When the device reaches the limit it would not even send
> > > > > > > > > the
> > > > command.
> > > > > > > >
> > > > > > > >
> > > > > > > > That's more logic for the driver to implement. Why should it
> > > > > > > > bother when it can just send it?
> > > > > > > >
> > > > > > > It can just send it and fail and flood with error logs.
> > > > > >
> > > > > > If the spec says device can fail and that is expected then it won't flood.
> > > > > > If you don't want driver to send some commands you should say
> > > > > > that instead of saying device should filter them.
> > > > > I will add the normative in the driver requirement section.
> > > > >
> > > > > >
> > > > > > > Also it cannot just send some random id based numbers that
> > > > > > > device does
> > > > > > not support.
> > > > > >
> > > > > > Aha. I think I'm beginning to guess. You wrote:
> > > > > > 	field{max_groups} indicates total number of flow filter groups
> > > > > > supported
> > > > > > 	by the device whose group identifiers can be any value in the
> > > > > > range from 0 to
> > > > > > 	\field{max_groups - 1}.
> > > > > > and you think this implies that it can't be out of that range.
> > > > > > That's not how logic works though ;)
> > > > > >
> > > > > Which logic?
> > > >
> > > > The aristotelian logic we commonly use.  You say "group identifiers
> > > > can be any value in the range".  You seem to mean "group identifiers
> > > > can not be any value not in the range". These two statements are not
> > > > equivalent and neither implies the other.
> > > I fail to see the difference between the two English sentence you wrote.
> > > Can you please have an example, how second is any different?
> > >
> > > In my example max_groups = 5, so group id must be from 0 to 4.
> > 
> > No, your sentence does not say it must. It says it can be in range.
> > Can it be out of range, e.g. 5? You don't state either way.
> I understand. I will that normative in v7.
> 
> > The second sentence says it can not be out of range so 5 is illegal.
> > Are there illegal values inside the range? E.g. might 3 be illegal?
> > According to second sentence, maybe. According to 1st sentence, 3 is legal.
> > 
> > > It can be any value.
> > 
> > :(
> > 
> > 
> > > In device and driver requirements, I will write the explicit line that it MUST be
> > in this range.
> > 
> > Then it will be different.
> Will write as two different normative respective for each section.
> For driver it is SHOULD, for device it is MUST wherever applicable.
> 
> > 
> > > >
> > > >
> > > > > The driver will honor the limit given to him.
> > > > > The device will also naturally honor what it published to driver.
> > > > > >
> > > > > > > > > For example if the device supports only one group, it wont
> > > > > > > > > implement
> > > > > > > > ethtool ntuple and will chose only arfs as one example.
> > > > > > > >
> > > > > > > > Sounds like a contrived example why does device even expose
> > > > > > > > flow steering then?
> > > > > > > It is not contrived at all.
> > > > > > >
> > > > > > > A device may be support one or multiple groups. Each group in
> > > > > > > the OS has
> > > > > > multiple users.
> > > > > > > One group is enough for either ARFS or ethool but not both.
> > > > > > > Two groups can allow ARFS and ethtool to co-exists.
> > > > > > > 3 groups will allow in future to expose some queues to user apps.
> > > > > >
> > > > > > Oh I misunderstood.
> > > > > > Then why would driver decide to force a decision to ARFS specifically?
> > > > > > It will likely leave this to the user.
> > > > > It is the driver implementation choice, For example:
> > > > > if there is only one group, driver can choose to give strong
> > > > > preference, say for
> > > > ARFS only.
> > > > >
> > > > > Or it can let it be on first come first usage, for example, if
> > > > > ethtool wants to
> > > > use first, it will allow it, not letting ARFS to work.
> > > > >
> > > >
> > > > Practically, drivers are highly unlikely to make the choice.
> > > It will make the choice when there is only one group available.
> > >
> > > > That is why it is reasonable not to have checks in the driver if
> > > > device is doing them anyway.
> > > The groups particularly have the priority as well. So the check in device does
> > not work.
> > 
> > I don't know what that means.
> > 
> Packet processing order is defined by the group.
> You should have participated in Aug or before when all of these were discussed.
> Anyways, next time you can participate in other features.
> 
> > > > But OTOH if you want driver to do the checks then I don't see why we
> > > > should also add them to device unless there's a good reason to.
> > > > Less checks in devices -> cheaper simpler devices.
> > > >
> > > The cheaper devices can always place UINT_32 max values and ignore any
> > checking etc.
> > 
> > Then won't driver expect inifinite # of groups to be supported?
> > 
> A cheaper device can place anything it wants.
> 
> > > The check in the device will be done as it uses such an id for its own
> > bookkeeping.
> > >
> > I don't know what that means.
> > > > --
> > > > 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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, November 27, 2023 5:10 PM
> 
> On Mon, Nov 27, 2023 at 11:33:45AM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Monday, November 27, 2023 4:53 PM
> > >
> > > On Mon, Nov 27, 2023 at 10:19:47AM +0000, Parav Pandit wrote:
> > > > > > > It appears that's not what you meant.
> > > > > > > I can try and guess what you meant but I'd rather have you
> > > > > > > rewrite this in a way that makes the meaning explicit.
> > > > > > >
> > > > > > If you insist, I can duplicate in driver requirements too.
> > > > > > >
> > > > > > > > There are many capabilities here each with a different use.
> > > > > > >
> > > > > > > And you are under the impression that dumping all kind of
> > > > > > > stuff in a single place is good somehow?
> > > > > > It is not dumped.
> > > > > > It is structured based on the functionality.
> > > > > > The config space proposal tends to dump everything in
> > > > > > unstructured way at
> > > > > single place that cannot extended elegantly.
> > > > > >
> > > > > > For example, Xuan's dynamic array cannot be placed next to
> > > > > > other dynamic
> > > > > array of flow filter.
> > > > >
> > > > > I don't know what "dynamic array" is exactly but generally, Any
> > > > > kind of a big array is not appropriate in config space - I don't
> > > > > see any of these
> > > here, though.
> > > > Ah, now I see the disconnect.
> > > >
> > > > There is.
> > > > struct virtio_net_ctrl_ff_match_types
> > >
> > > Now I think I don't understand what this array does at all.
> > > Why are there multiple entries with each entry also including a mask
> > > of multiple fields?
> > >
> > Each entry indicates a bitmask of supported fields that can be filtered.
> > The type indicates how to interpret each fields_map.
> > Any new type for MPLS can be added easily by defining its type and mask.
> > And same type enums and field mask also used by the data path too.
> 
> Oh wait I think I begin to remember now. There are 12 bits defined so far and
> you made it this huge array. Right? With code to parse and format it all. Just
> add a 64 bit field and it will last us long enough. No parsing and formatting just
> #define.
The infrastructure defines 12 bits only because this is first minimal viable work.
There will be more fields to be added for more types.
It is not good to mix up all the fields and bit definitions.

And there are per device max limits which is way more than 12 bits.

Your attitude to take narrow view and trying to kill this command is not good either.
> 
> 
> > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > > For many caps, it is to know what is supported/not to
> > > > > > > > decide in the
> > > > > driver.
> > > > > > >
> > > > > > > I can't parse this sentence.
> > > > > > >
> > > > > > The driver checks things upfront what is supported while
> > > > > > serving the
> > > > > command, instead of random trial and error guess work game with
> > > > > he
> > > device.
> > > > >
> > > > > There's no real guesswork with these specific commands: user
> > > > > types an ethtool command, driver sends it on, it either succeeds or fails.
> > > > It is a guess work without driver knowing if its supported or not.
> > >
> > > What we do not need is both driver and device checking these values.
> > >
> > Driver is mainly to using to know its valid ranges and to fail the commands
> early.
> > Device is referring it for any validations that it may need to do.
> >
> > >
> > > > > However if you feel it important to do checks in driver - OK.
> > > > Yes, because there are many parsing fields and good to know.
> > > >
> > > > > But that makes cap query an init time thing.
> > > > Yes, caps does not change dynamically.
> > > > But sure it is different among different member devices and owner too.
> > > >
> > > > >
> > > > >
> > > > > > > > Certain caps are max to know what is the range of id that
> > > > > > > > driver can use like
> > > > > > > flow filter id, group id.
> > > > > > >
> > > > > > > Maybe then. But there's apparently no requirement for either
> > > > > > > device or driver to put it in this range.
> > > > > > >
> > > > > >
> > > > > > I will add this requirement statements in device requirements
> > > > > > and that will
> > > > > make things clear.
> > > > > > Good point.
> > > > > >
> > > > > > > > > For what benefit?
> > > > > > > > >
> > > > > > > > For functional work.
> > > > > > >
> > > > > > > You don't really explain what kind of work in this patchset,
> > > > > > > or in the commit logs. Maybe it's obvious for you but not for
> everyone.
> > > > > > What text are you expecting?
> > > > > > We have undergone all the requirements study that you
> > > > > > conveniently
> > > > > choose to not participate for 2 months.
> > > > >
> > > > > Yea ... I'm sorry. Things have been going on here, man.
> > > > > I'm glad I'm back and able to participate though.
> > > > >
> > > > Great.
> > > >
> > > > > > >
> > > > > > > > > > In future one may want to provision certain max limits
> > > > > > > > > > that device can have
> > > > > > > > > as they eventually will consume certain device hardware
> resources.
> > > > > > > > >
> > > > > > > > > I don't exactly see how this helps provisioning.
> > > > > > > > >
> > > > > > > > Provisioning side will set parameters which will reflect
> > > > > > > > these limits to be same
> > > > > > > on src and dst hypervisor when device migration is used.
> > > > > > >
> > > > > > > But making provisioning depend on driver being fully loaded
> > > > > > > and accessing the command is creating a chicken and egg problem.
> > > > > > I didn't explain well likely.
> > > > > > Provisioning as you described, will be on owner device.
> > > > > > This command tells driver what is provisioned.
> > > > > >
> > > > > > > Provisioning is much more likely to use some new admin
> > > > > > > commands over an owner device. So this command is useless for it.
> > > > > > >
> > > > > > Provisioning owner device will use exact same structure while
> > > provisioning.
> > > > >
> > > > > So the advantage of using config space is that we can have a
> > > > > single generic provision command that gets device config space
> > > > > format. Your approach will need some kind of net device specific thing.
> > > > >
> > > >
> > > > I see the advantage. But it very small and largely hidden beneath
> > > > the sw
> > > layers how a provisioning command sets things.
> > > > A provisioning command sets the value, it surfaces at different level.
> > > >
> > > > Multiple device vendors want to follow the design pattern to use "
> > > > most uses it is better to use a virtqueue to update configuration
> information"
> > > >
> > > > Instead of RO = config space, RW = cvq.
> > > >
> > > > The more pragmatic design pattern is:
> > > > driver initialization time => feature bit + virtio config space.
> > > > Driver runtime => cvq.
> > >
> > > yes. capability check is initialization time though.
> > >
> > Right, for runtime config like this cap, it is not in the initialization time
> needed.
> >
> > >
> > > > >
> > > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > > > Patch 5/5 mandates that device validates all fields already.
> > > > > > > > > >
> > > > > > > > > > > Are there guests that will actually look at these
> > > > > > > > > > > caps as opposed to just sending commands and looking
> > > > > > > > > > > at the return
> > > > > status?
> > > > > > > > > > When the device reaches the limit it would not even
> > > > > > > > > > send the
> > > > > command.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > That's more logic for the driver to implement. Why
> > > > > > > > > should it bother when it can just send it?
> > > > > > > > >
> > > > > > > > It can just send it and fail and flood with error logs.
> > > > > > >
> > > > > > > If the spec says device can fail and that is expected then it won't
> flood.
> > > > > > > If you don't want driver to send some commands you should
> > > > > > > say that instead of saying device should filter them.
> > > > > > I will add the normative in the driver requirement section.
> > > > > >
> > > > > > >
> > > > > > > > Also it cannot just send some random id based numbers that
> > > > > > > > device does
> > > > > > > not support.
> > > > > > >
> > > > > > > Aha. I think I'm beginning to guess. You wrote:
> > > > > > > 	field{max_groups} indicates total number of flow filter
> > > > > > > groups supported
> > > > > > > 	by the device whose group identifiers can be any value in
> > > > > > > the range from 0 to
> > > > > > > 	\field{max_groups - 1}.
> > > > > > > and you think this implies that it can't be out of that range.
> > > > > > > That's not how logic works though ;)
> > > > > > >
> > > > > > Which logic?
> > > > >
> > > > > The aristotelian logic we commonly use.  You say "group
> > > > > identifiers can be any value in the range".  You seem to mean
> > > > > "group identifiers can not be any value not in the range". These
> > > > > two statements are not equivalent and neither implies the other.
> > > > I fail to see the difference between the two English sentence you wrote.
> > > > Can you please have an example, how second is any different?
> > > >
> > > > In my example max_groups = 5, so group id must be from 0 to 4.
> > >
> > > No, your sentence does not say it must. It says it can be in range.
> > > Can it be out of range, e.g. 5? You don't state either way.
> > I understand. I will that normative in v7.
> >
> > > The second sentence says it can not be out of range so 5 is illegal.
> > > Are there illegal values inside the range? E.g. might 3 be illegal?
> > > According to second sentence, maybe. According to 1st sentence, 3 is legal.
> > >
> > > > It can be any value.
> > >
> > > :(
> > >
> > >
> > > > In device and driver requirements, I will write the explicit line
> > > > that it MUST be
> > > in this range.
> > >
> > > Then it will be different.
> > Will write as two different normative respective for each section.
> > For driver it is SHOULD, for device it is MUST wherever applicable.
> >
> > >
> > > > >
> > > > >
> > > > > > The driver will honor the limit given to him.
> > > > > > The device will also naturally honor what it published to driver.
> > > > > > >
> > > > > > > > > > For example if the device supports only one group, it
> > > > > > > > > > wont implement
> > > > > > > > > ethtool ntuple and will chose only arfs as one example.
> > > > > > > > >
> > > > > > > > > Sounds like a contrived example why does device even
> > > > > > > > > expose flow steering then?
> > > > > > > > It is not contrived at all.
> > > > > > > >
> > > > > > > > A device may be support one or multiple groups. Each group
> > > > > > > > in the OS has
> > > > > > > multiple users.
> > > > > > > > One group is enough for either ARFS or ethool but not both.
> > > > > > > > Two groups can allow ARFS and ethtool to co-exists.
> > > > > > > > 3 groups will allow in future to expose some queues to user apps.
> > > > > > >
> > > > > > > Oh I misunderstood.
> > > > > > > Then why would driver decide to force a decision to ARFS
> specifically?
> > > > > > > It will likely leave this to the user.
> > > > > > It is the driver implementation choice, For example:
> > > > > > if there is only one group, driver can choose to give strong
> > > > > > preference, say for
> > > > > ARFS only.
> > > > > >
> > > > > > Or it can let it be on first come first usage, for example, if
> > > > > > ethtool wants to
> > > > > use first, it will allow it, not letting ARFS to work.
> > > > > >
> > > > >
> > > > > Practically, drivers are highly unlikely to make the choice.
> > > > It will make the choice when there is only one group available.
> > > >
> > > > > That is why it is reasonable not to have checks in the driver if
> > > > > device is doing them anyway.
> > > > The groups particularly have the priority as well. So the check in
> > > > device does
> > > not work.
> > >
> > > I don't know what that means.
> > >
> > Packet processing order is defined by the group.
> > You should have participated in Aug or before when all of these were
> discussed.
> > Anyways, next time you can participate in other features.
> >
> > > > > But OTOH if you want driver to do the checks then I don't see
> > > > > why we should also add them to device unless there's a good reason to.
> > > > > Less checks in devices -> cheaper simpler devices.
> > > > >
> > > > The cheaper devices can always place UINT_32 max values and ignore
> > > > any
> > > checking etc.
> > >
> > > Then won't driver expect inifinite # of groups to be supported?
> > >
> > A cheaper device can place anything it wants.
> >
> > > > The check in the device will be done as it uses such an id for its
> > > > own
> > > bookkeeping.
> > > >
> > > I don't know what that means.
> > > > > --
> > > > > 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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Michael S. Tsirkin]

On Mon, Nov 27, 2023 at 11:50:24AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, November 27, 2023 5:10 PM
> > 
> > On Mon, Nov 27, 2023 at 11:33:45AM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Monday, November 27, 2023 4:53 PM
> > > >
> > > > On Mon, Nov 27, 2023 at 10:19:47AM +0000, Parav Pandit wrote:
> > > > > > > > It appears that's not what you meant.
> > > > > > > > I can try and guess what you meant but I'd rather have you
> > > > > > > > rewrite this in a way that makes the meaning explicit.
> > > > > > > >
> > > > > > > If you insist, I can duplicate in driver requirements too.
> > > > > > > >
> > > > > > > > > There are many capabilities here each with a different use.
> > > > > > > >
> > > > > > > > And you are under the impression that dumping all kind of
> > > > > > > > stuff in a single place is good somehow?
> > > > > > > It is not dumped.
> > > > > > > It is structured based on the functionality.
> > > > > > > The config space proposal tends to dump everything in
> > > > > > > unstructured way at
> > > > > > single place that cannot extended elegantly.
> > > > > > >
> > > > > > > For example, Xuan's dynamic array cannot be placed next to
> > > > > > > other dynamic
> > > > > > array of flow filter.
> > > > > >
> > > > > > I don't know what "dynamic array" is exactly but generally, Any
> > > > > > kind of a big array is not appropriate in config space - I don't
> > > > > > see any of these
> > > > here, though.
> > > > > Ah, now I see the disconnect.
> > > > >
> > > > > There is.
> > > > > struct virtio_net_ctrl_ff_match_types
> > > >
> > > > Now I think I don't understand what this array does at all.
> > > > Why are there multiple entries with each entry also including a mask
> > > > of multiple fields?
> > > >
> > > Each entry indicates a bitmask of supported fields that can be filtered.
> > > The type indicates how to interpret each fields_map.
> > > Any new type for MPLS can be added easily by defining its type and mask.
> > > And same type enums and field mask also used by the data path too.
> > 
> > Oh wait I think I begin to remember now. There are 12 bits defined so far and
> > you made it this huge array. Right? With code to parse and format it all. Just
> > add a 64 bit field and it will last us long enough. No parsing and formatting just
> > #define.
> The infrastructure defines 12 bits only because this is first minimal viable work.
> There will be more fields to be added for more types.

Maybe.  Let's see a list and see if we even get close to 32 bits.

> It is not good to mix up all the fields and bit definitions.

Not good how?  It's definitely much simpler than what you wrote.
Does it matter that bits for a given transport are not all
consequitive? I do not see why it matters.


> And there are per device max limits which is way more than 12 bits.

These are just a couple of 32 bit fields.

-- 
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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, November 27, 2023 6:03 PM
> 
> On Mon, Nov 27, 2023 at 11:50:24AM +0000, Parav Pandit wrote:
> >
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Monday, November 27, 2023 5:10 PM
> > >
> > > On Mon, Nov 27, 2023 at 11:33:45AM +0000, Parav Pandit wrote:
> > > >
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Monday, November 27, 2023 4:53 PM
> > > > >
> > > > > On Mon, Nov 27, 2023 at 10:19:47AM +0000, Parav Pandit wrote:
> > > > > > > > > It appears that's not what you meant.
> > > > > > > > > I can try and guess what you meant but I'd rather have
> > > > > > > > > you rewrite this in a way that makes the meaning explicit.
> > > > > > > > >
> > > > > > > > If you insist, I can duplicate in driver requirements too.
> > > > > > > > >
> > > > > > > > > > There are many capabilities here each with a different use.
> > > > > > > > >
> > > > > > > > > And you are under the impression that dumping all kind
> > > > > > > > > of stuff in a single place is good somehow?
> > > > > > > > It is not dumped.
> > > > > > > > It is structured based on the functionality.
> > > > > > > > The config space proposal tends to dump everything in
> > > > > > > > unstructured way at
> > > > > > > single place that cannot extended elegantly.
> > > > > > > >
> > > > > > > > For example, Xuan's dynamic array cannot be placed next to
> > > > > > > > other dynamic
> > > > > > > array of flow filter.
> > > > > > >
> > > > > > > I don't know what "dynamic array" is exactly but generally,
> > > > > > > Any kind of a big array is not appropriate in config space -
> > > > > > > I don't see any of these
> > > > > here, though.
> > > > > > Ah, now I see the disconnect.
> > > > > >
> > > > > > There is.
> > > > > > struct virtio_net_ctrl_ff_match_types
> > > > >
> > > > > Now I think I don't understand what this array does at all.
> > > > > Why are there multiple entries with each entry also including a
> > > > > mask of multiple fields?
> > > > >
> > > > Each entry indicates a bitmask of supported fields that can be filtered.
> > > > The type indicates how to interpret each fields_map.
> > > > Any new type for MPLS can be added easily by defining its type and
> mask.
> > > > And same type enums and field mask also used by the data path too.
> > >
> > > Oh wait I think I begin to remember now. There are 12 bits defined
> > > so far and you made it this huge array. Right? With code to parse
> > > and format it all. Just add a 64 bit field and it will last us long
> > > enough. No parsing and formatting just #define.
> > The infrastructure defines 12 bits only because this is first minimal viable
> work.
> > There will be more fields to be added for more types.
> 
> Maybe.  Let's see a list and see if we even get close to 32 bits.
> 
There are many ether types to filter based on the device.
Many types of IPv4 and IPV6 header fields.

https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml

> > It is not good to mix up all the fields and bit definitions.
> 
> Not good how? 
I provided example why it will be so bad to mix up the socket options under one socket option command.

>  It's definitely much simpler than what you wrote.
Simpler for spec != simpler to implement in device.

> Does it matter that bits for a given transport are not all consequitive? I do not
> see why it matters.
> 
It is surely not elegant to spread things around that way. 
Actually it is not good for TLV definition in the data path flow filter commands.

> 
> > And there are per device max limits which is way more than 12 bits.
> 
> These are just a couple of 32 bit fields.
> 
And they can be different that cannot be de-duplicated across different member devices.

The proposal is not based on the count or RO/RW etc.
It is based on when it needs to be accessed, as they are not init time early use to initialize the device, they are over cvq.
> --
> 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 v7 2/5] virtio-net: Add flow filter capabilities read commands

[Michael S. Tsirkin]

On Mon, Nov 27, 2023 at 12:49:35PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, November 27, 2023 6:03 PM
> > 
> > On Mon, Nov 27, 2023 at 11:50:24AM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Monday, November 27, 2023 5:10 PM
> > > >
> > > > On Mon, Nov 27, 2023 at 11:33:45AM +0000, Parav Pandit wrote:
> > > > >
> > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Sent: Monday, November 27, 2023 4:53 PM
> > > > > >
> > > > > > On Mon, Nov 27, 2023 at 10:19:47AM +0000, Parav Pandit wrote:
> > > > > > > > > > It appears that's not what you meant.
> > > > > > > > > > I can try and guess what you meant but I'd rather have
> > > > > > > > > > you rewrite this in a way that makes the meaning explicit.
> > > > > > > > > >
> > > > > > > > > If you insist, I can duplicate in driver requirements too.
> > > > > > > > > >
> > > > > > > > > > > There are many capabilities here each with a different use.
> > > > > > > > > >
> > > > > > > > > > And you are under the impression that dumping all kind
> > > > > > > > > > of stuff in a single place is good somehow?
> > > > > > > > > It is not dumped.
> > > > > > > > > It is structured based on the functionality.
> > > > > > > > > The config space proposal tends to dump everything in
> > > > > > > > > unstructured way at
> > > > > > > > single place that cannot extended elegantly.
> > > > > > > > >
> > > > > > > > > For example, Xuan's dynamic array cannot be placed next to
> > > > > > > > > other dynamic
> > > > > > > > array of flow filter.
> > > > > > > >
> > > > > > > > I don't know what "dynamic array" is exactly but generally,
> > > > > > > > Any kind of a big array is not appropriate in config space -
> > > > > > > > I don't see any of these
> > > > > > here, though.
> > > > > > > Ah, now I see the disconnect.
> > > > > > >
> > > > > > > There is.
> > > > > > > struct virtio_net_ctrl_ff_match_types
> > > > > >
> > > > > > Now I think I don't understand what this array does at all.
> > > > > > Why are there multiple entries with each entry also including a
> > > > > > mask of multiple fields?
> > > > > >
> > > > > Each entry indicates a bitmask of supported fields that can be filtered.
> > > > > The type indicates how to interpret each fields_map.
> > > > > Any new type for MPLS can be added easily by defining its type and
> > mask.
> > > > > And same type enums and field mask also used by the data path too.
> > > >
> > > > Oh wait I think I begin to remember now. There are 12 bits defined
> > > > so far and you made it this huge array. Right? With code to parse
> > > > and format it all. Just add a 64 bit field and it will last us long
> > > > enough. No parsing and formatting just #define.
> > > The infrastructure defines 12 bits only because this is first minimal viable
> > work.
> > > There will be more fields to be added for more types.
> > 
> > Maybe.  Let's see a list and see if we even get close to 32 bits.
> > 
> There are many ether types to filter based on the device.
> Many types of IPv4 and IPV6 header fields.
> 
> https://www.iana.org/assignments/ieee-802-numbers/ieee-802-numbers.xhtml

If I grep for RFC there I get 31 types. Looks like 64 will last us
for a while.


> > > It is not good to mix up all the fields and bit definitions.
> > 
> > Not good how? 
> I provided example why it will be so bad to mix up the socket options under one socket option command.
> 
> >  It's definitely much simpler than what you wrote.
> Simpler for spec != simpler to implement in device.

Not just spec. Simpler for drivers too. And I suspect that yes a bitmap
is simpler in device too. 100% sure in a software device.

> > Does it matter that bits for a given transport are not all consequitive? I do not
> > see why it matters.
> > 
> It is surely not elegant to spread things around that way. 
> Actually it is not good for TLV definition in the data path flow filter commands.

That TLV thing still needs some thought. Here again,
we can have a single config space array and it's nice and clean
or we need to invent 

> > 
> > > And there are per device max limits which is way more than 12 bits.
> > 
> > These are just a couple of 32 bit fields.
> > 
> And they can be different that cannot be de-duplicated across different member devices.

why would they be different? only if provisioned.  But again, just
access it over DMA then it is not a problem that they are different. The
objection is to using a device specific mechanism as opposed to building
a generic config space over DMA.

> The proposal is not based on the count or RO/RW etc.
> It is based on when it needs to be accessed, as they are not init time early use to initialize the device, they are over cvq.

all these limits are very clearly init time. For example max id can be
used to allocate an array.

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