From: Jason Wang <jasowang@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Parav Pandit <parav@nvidia.com>,
"virtio-comment@lists.oasis-open.org"
<virtio-comment@lists.oasis-open.org>,
"cohuck@redhat.com" <cohuck@redhat.com>,
"sburla@marvell.com" <sburla@marvell.com>,
Shahaf Shuler <shahafs@nvidia.com>,
"si-wei.liu@oracle.com" <si-wei.liu@oracle.com>,
"xuanzhuo@linux.alibaba.com" <xuanzhuo@linux.alibaba.com>,
Heng Qi <hengqi@linux.alibaba.com>
Subject: Re: [virtio-comment] Re: [PATCH v6 2/5] virtio-net: Add flow filter capabilities read commands
Date: Wed, 13 Dec 2023 12:48:55 +0800 [thread overview]
Message-ID: <CACGkMEu3+-M6VmnSQgBvRPnNvgYOPOO3vfjH5Y+vjgL51ciiug@mail.gmail.com> (raw)
In-Reply-To: <20231212105018-mutt-send-email-mst@kernel.org>
On Tue, Dec 12, 2023 at 11:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Nov 24, 2023 at 12:02:23PM +0800, Jason Wang wrote:
> > On Wed, Nov 22, 2023 at 10:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Nov 22, 2023 at 02:10:29PM +0000, Parav Pandit wrote:
> > > >
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Wednesday, November 22, 2023 7:32 PM
> > > > >
> > > > > On Fri, Nov 10, 2023 at 02:38:50PM +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>
> > > > > >
> > > > > > ---
> > > > > > changelog:
> > > > > > 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 furture
> > > > > > - 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 | 208
> > > > > > ++++++++++++++++++++++++++++++-
> > > > > > 1 file changed, 206 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/device-types/net/description.tex
> > > > > > b/device-types/net/description.tex
> > > > > > index 30220b5..eccd8d6 100644
> > > > > > --- a/device-types/net/description.tex
> > > > > > +++ b/device-types/net/description.tex
> > > > > > @@ -1173,7 +1173,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 @@ -1224,7 +1228,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}
> > > > > >
> > > > > > -\label{sec:Device Types / Network Device / Device Operation / Control
> > > > > > Virtqueue / Setting Promiscuous Mode}%old label for latexdiff
> > > > > > +\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 the \field{mask_supported} is set, for the specific
> > > > > > +\field{type}, the device can perform masking 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} are following:
> > > > > > +
> > > > > > +\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} are following:
> > > > > > +
> > > > > > +\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} are following:
> > > > > > +
> > > > > > +\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} are following:
> > > > > > +
> > > > > > +\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} are following:
> > > > > > +
> > > > > > +\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} are following:
> > > > > > +
> > > > > > +\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}
> > > > > > +
> > > > >
> > > > > This is such an elaborate structure to report just 12 read only bits.
> > > > > Please let's just follow the example of le32 supported_tunnel_types and add
> > > > > l32 supported_flow_control.
> > > > >
> > > > supported_tunnel_types was let go because cvq is efficient.
> > > > None of these fields are needed for init time configuration of the driver before DRIVER_OK.
> > >
> > > I really basically disagree. Whether control flow is supported can
> > > easily influence how many VQs are needed.
> > >
> > >
> > > >
> > > > It is a very narrow view of 12 bits. It is going to grow and many.
> > > > This is far more structured for each type done here.
> > > >
> > > > >
> > > > > After you were trying to add kilobytes to megabytes of memory - I see little
> > > > > reason to save 12 RO bits that can be shared by any number of VFs.
> > > > >
> > > > Completely wrong reason and very late review and also does not align with every other command we did.
> > >
> > > which other command? hash and rss are like this: capability in config
> > > space config through cvq. For the same reason.
> > >
> > > > > However, I do think we should create an option to access config space over
> > > > > DMA (preferably admin commands). Let's add this quickly and then we don't
> > > > > need to worry about legacy guests accessing flow filter through MMIO.
> > > >
> > > > CVQ is already there as single interface forget and set for rss, rss context, vq moderation, statistics, flow filter caps and more.
> > > > No reason to bifurcate.
> > >
> > > The reason is so that we have a single consistent view where e.g. you want
> > > to provision a device with a specific capability you just
> > > specify how its config space looks.
> > >
> > > If you start shuffling capabilities off to VQs that will be much
> > > much harder.
> > >
> > > > I won't be able to absorb this comment of DMA interface.
> > > > If I discuss further, I will repeat the whole document [1] and I will avoid that now.
> > > >
> > > > [1] https://docs.google.com/document/d/1Iyn-l3Nm0yls3pZaul4lZiVj8x1s73Ed6rOsmn6LfXc/edit#heading=h.qexbtyc2jpwr
> > >
> > >
> > > I really worry about how provisioning will work. And I do not at all
> > > cherish replicating all of these query capability commands for provisioning.
> >
> > +1
> >
> > There's nothing that prevents the config space from being implemented
> > in a way other than registers.
>
> Jason I'm not sure what does your +1 mean here. Are you ok with the
> patchset as is? While I feel config space would be nicer I also find it
> unlikely drivers will actually bother to check the supported commands.
> So from that POV, for this use-case I am not too stressed out.
I meant I prefer config space access.
1) It simplifies the provisioning and compatibility check a lot.
2) We don't need a mini driver to probe the features
We could invent a DMA interface for config space if necessary
(actually there's one in the transport vq series).
Thanks
>
> --
> 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/
next prev parent reply other threads:[~2023-12-13 4:50 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-10 12:38 [virtio-comment] [PATCH v6 0/5] virtio-net: Support flow filter for receive packets Parav Pandit
2023-11-10 12:38 ` [virtio-comment] [PATCH v6 1/5] virtio-net: Add theory of operation for flow filter Parav Pandit
2023-11-22 13:22 ` [virtio-comment] " Cornelia Huck
2023-11-22 13:30 ` [virtio-comment] " Parav Pandit
2023-11-10 12:38 ` [virtio-comment] [PATCH v6 2/5] virtio-net: Add flow filter capabilities read commands Parav Pandit
2023-11-22 13:38 ` [virtio-comment] " Cornelia Huck
2023-11-22 13:44 ` [virtio-comment] " Parav Pandit
2023-11-22 14:02 ` [virtio-comment] " Michael S. Tsirkin
2023-11-22 14:10 ` [virtio-comment] " Parav Pandit
2023-11-22 14:51 ` [virtio-comment] " Michael S. Tsirkin
2023-11-22 15:00 ` [virtio-comment] " Parav Pandit
2023-11-24 4:02 ` [virtio-comment] " Jason Wang
2023-11-24 4:13 ` Parav Pandit
2023-11-24 5:31 ` Michael S. Tsirkin
2023-11-24 5:40 ` Parav Pandit
2023-11-24 6:02 ` Jason Wang
2023-11-24 6:41 ` Parav Pandit
2023-11-24 9:13 ` Michael S. Tsirkin
2023-11-27 10:19 ` Parav Pandit
2023-11-27 11:10 ` Michael S. Tsirkin
2023-11-27 11:12 ` Parav Pandit
2023-11-27 12:52 ` Michael S. Tsirkin
2023-11-27 12:58 ` Parav Pandit
2023-11-27 13:05 ` Michael S. Tsirkin
2023-11-24 6:18 ` Michael S. Tsirkin
2023-11-24 6:31 ` Parav Pandit
2023-11-24 11:33 ` Michael S. Tsirkin
2023-11-27 10:19 ` Parav Pandit
2023-11-27 11:28 ` Michael S. Tsirkin
2023-11-27 11:40 ` Parav Pandit
2023-11-27 11:47 ` Michael S. Tsirkin
2023-11-27 11:54 ` Parav Pandit
2023-11-27 12:42 ` Michael S. Tsirkin
2023-11-27 13:05 ` Parav Pandit
2023-11-27 13:12 ` Michael S. Tsirkin
2023-11-27 13:06 ` Cornelia Huck
2023-11-27 13:14 ` Michael S. Tsirkin
2023-11-24 6:03 ` Jason Wang
2023-11-24 5:32 ` Michael S. Tsirkin
2023-11-24 5:53 ` Parav Pandit
2023-11-24 6:06 ` Michael S. Tsirkin
2023-11-24 6:27 ` Parav Pandit
2023-11-24 10:27 ` Michael S. Tsirkin
2023-11-27 10:19 ` Parav Pandit
2023-11-27 11:37 ` Michael S. Tsirkin
2023-11-27 11:47 ` Parav Pandit
2023-12-12 15:52 ` Michael S. Tsirkin
2023-12-13 4:48 ` Jason Wang [this message]
2023-11-10 12:38 ` [virtio-comment] [PATCH v6 3/5] virtio-net: Add flow filter group life cycle commands Parav Pandit
2023-11-22 13:42 ` [virtio-comment] " Cornelia Huck
2023-11-10 12:38 ` [virtio-comment] [PATCH v6 4/5] virtio-net: Add flow filter match entry, action and requests Parav Pandit
2023-11-22 13:52 ` [virtio-comment] " Cornelia Huck
2023-11-10 12:38 ` [virtio-comment] [PATCH v6 5/5] virtio-net: Add flow filter device and driver requirements Parav Pandit
2023-11-20 6:47 ` [virtio-comment] RE: [PATCH v6 0/5] virtio-net: Support flow filter for receive packets Parav Pandit
2023-11-21 12:54 ` Parav Pandit
2023-11-22 11:26 ` Cornelia Huck
2023-11-22 12:03 ` Parav Pandit
2023-11-23 10:22 ` Parav Pandit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACGkMEu3+-M6VmnSQgBvRPnNvgYOPOO3vfjH5Y+vjgL51ciiug@mail.gmail.com \
--to=jasowang@redhat.com \
--cc=cohuck@redhat.com \
--cc=hengqi@linux.alibaba.com \
--cc=mst@redhat.com \
--cc=parav@nvidia.com \
--cc=sburla@marvell.com \
--cc=shahafs@nvidia.com \
--cc=si-wei.liu@oracle.com \
--cc=virtio-comment@lists.oasis-open.org \
--cc=xuanzhuo@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).