[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [virtio-comment] Re: [PATCH v6 2/5] virtio-net: Add flow filter capabilities read commands
> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Friday, November 24, 2023 11:01 AM
>
> On Fri, Nov 24, 2023 at 04:13:08AM +0000, Parav Pandit wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Friday, November 24, 2023 9:32 AM
> > >
> > > 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-
> > > l3Nm0yls3pZaul4lZiVj8x1s73Ed
> > > > > 6rOsmn6LfXc/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
> > >
> > - 1
> >
> > All points are discussed and not going to repeat 7th or 8th time.
> >
> > > There's nothing that prevents the config space from being
> > > implemented in a way other than registers.
> > >
> > It does.
> > It is discussed 7th or 8th time.
> >
> > All the points are discussed already.
> > [1]
> > https://docs.google.com/document/d/1Iyn-
> l3Nm0yls3pZaul4lZiVj8x1s73Ed6r
> > Osmn6LfXc/edit#heading=h.qexbtyc2jpwr
> >
> > many things are done over cvq and statistics capabilities over cvq is the latest
> approved.
> > I wont be able to repeat the content of document [1] anymore.
>
> The famous Cons: None document. Yea you pushed this through with
> statistics because hey it's just statistics.
Everyone agreed.
What more, see Jason's reply when explicitly asked in [2].
[2] https://lists.oasis-open.org/archives/virtio-comment/202308/msg00044.html
Snippet here:
"Adding cvq is much easier than inventing(duplicating) the work of a transport."
> Worst case devices can lie, it is not
> clear worrying about statistics when provisioning is important enough.
Device statistics cannot be broken when migrating.
> And
> you are now under the impression everyone got convinced when in reality
> everyone just got tired of arguing. This is not a good pattern to try and repeat.
> Let me repeat the relevant part here for
> you:
>
> So why shouldnât we just add some transport VQ on the owner device
> to transport the SIOV deviceâs configuration?
>
> Ans:
> Such addition means that hardware vendors need to build
> runtime configurations in 4 different ways.
> One way using CVQ for PCI PF and VFs
> 2nd way as backward compatible SIOV using ownerâs admin
> VQ
> 3rd way using SIOVâs own CVQ channel for TDISP
> 4th way using mix of backward compatible and secure and
> efficient way using #2 and #3.
>
>
> This is just a straw-man argument. No - we make a capability optional,
Capabilities are not optional.
The design has undergone carefully review to ensure that driver honors the capability, and it is migratable both.
You dint explain why capability should be optional.
The first patch has defined how the capabilities are used by the driver.
> we
> strongly suggest that *drivers* support both old and new mechanism, and
> then *devices* will only implement what's required.
There are other examples in the same document that makes things worst with old and new.
Also there is literally no way to enforce that driver supports both and new. It is just sounds like an excuse to force infinite config space.
The method proposed here is elegant and clearly promote one way to do things for driver and device with predictability.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]