Re: [virtio-dev] RE: [PATCH v9] virtio-net: support inner header hash
在 2023/2/22 下午2:21, Michael S. Tsirkin 写道: On Wed, Feb 22, 2023 at 10:34:39AM +0800, Heng Qi wrote: The user will figure out how to mitigate when such QoS is not available. Either to run in best-effort mode or mitigate differently. Yes, our cloud security and cloud network team will configure and use inner hash on dpdk. Sounds good. More practical for dpdk than Linux. Is there a chance that when the interface is close to be final, but before the vote, you post a patch to the dpdk list and get some acks from the maintainers, cc virtio-dev. This way we won't merge something that will then go unused? That would be best - do you have a prototype? Not yet, dpdk and the business team are waiting for our virtio specification, and they have stated as a business team that their implementation on dpdk will not necessarily be open sourced to the community. In fact I discussed with them the security issues between tunnels, and I will quote their solutions to tunnel attacks below, but this is a problem between the tunnels, not the introduction of inner hash. I don't think we need to focus too much on this, but I'll do my best to describe the security issues between tunnels in v10. " This is not a problem with the inner hash, it is a general problem with the outer hash. I communicated with our people who are doing cloud security (they are also one of the demanders of inner hash), and it is a common problem for one tunnel to attack another tunnel. For example, there is a tunnel t1; a tunnel t2; a tunnel endpoint VTEP0, and the vni id of t1 is id1, and the vni id of v2 is id2; a VM. At this time, regardless of the inner hash or the outer hash, the traffic of tunnel t1 and tunnel t2 will reach the VM through VTEP0 (whether it is a single queue or multiple queues), and may be placed on the same queue to cause queue overflow. Do note (and explain in spec?) that with just an outer hash and RSS it is possible to configure the tunnels to use distict queues. Impossible with this interface but arguably only works for a small number of tunnels anyway. # Solutions: More like mitigations. Yes, you are right. 1. Some current forwarding tools such as DPDK have good forwarding performance, and it is difficult to fill up the queue; Oh that's a good point. If driver is generally faster than the device and queues stay away from filling up there's no DoS. I'd add this to the spec. Ok. 2. or switch the attack traffic to the attack clusters; What is that? This is done by the monitoring part outside the tunnel, which is also an important mitigation method they mentioned to prevent DoS between tunnels. For example, the monitoring part cuts off, limits or redirects the abnormal traffic of the tunnel. 3. or connect the traffic of different tunnels to different network card ports or network devices. Not sure how this is relevant. These a distinct outer MAC - with this why do we need a tunnel? 4.. " - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9] virtio-net: support inner header hash
Hi, Jason. Long time no see. :)
在 2023/2/22 上午11:22, Jason Wang 写道:
在 2023/2/22 01:50, Michael S. Tsirkin 写道:
On Sat, Feb 18, 2023 at 10:37:15PM +0800, Heng Qi wrote:
+\subparagraph{Security risks between encapsulated packets and RSS}
+There may be potential security risks when encapsulated packets
using RSS to
+select queues for placement. When a user inside a tunnel tries to
control the
What do you mean by "user" here? Is it a remote or local one?
I mean a remote attacker who is not under the control of the tunnel owner.
Thanks.
+enqueuing of encapsulated packets, then the user can flood the
device with invaild
+packets, and the flooded packets may be hashed into the same queue
as packets in
+other normal tunnels, which causing the queue to overflow.
+
+This can pose several security risks:
+\begin{itemize}
+\item Encapsulated packets in the normal tunnels cannot be
enqueued due to queue
+ overflow, resulting in a large amount of packet loss.
+\item The delay and retransmission of packets in the normal
tunnels are extremely increased.
+\item The user can observe the traffic information and enqueue
information of other normal
+ tunnels, and conduct targeted DoS attacks.
+\end{\itemize}
+
Hmm with this all written out it sounds pretty severe.
I think we need first understand whether or not it's a problem that we
need to solve at spec level:
1) anything make encapsulated packets different or why we can't hit
this problem without encapsulation
2) whether or not it's the implementation details that the spec
doesn't need to care (or how it is solved in real NIC)
Thanks
At this point with no ways to mitigate, I don't feel this is something
e.g. Linux can enable. I am not going to nack the spec patch if
others find this somehow useful e.g. for dpdk.
How about CC e.g. dpdk devs or whoever else is going to use this
and asking them for the opinion?
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v6] virtio-net: support the virtqueue coalescing moderation
On Wed, Feb 22, 2023 at 04:13:06AM +, Parav Pandit wrote:
>
> > From: Heng Qi
> > Sent: Tuesday, February 21, 2023 10:22 PM
>
> > +\item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue
> > notification coalescing.
> > +
> s/notification/notifications
> should be plural as multiple notifications are coalesced like the below
> description of VIRTIO_NET_F_NOTF_COAL.
Hmm nope - this is the "reduced relative" construct. It's always in a singular
form. The full form would be "coalescing of notifications of virtqueues"
and that indeed would use a plural form. But technical writing is
usually using the reduced relative form.
> > \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> > coalescing.
> >
> > \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > @@ -139,6 +141,7 @@ \subsubsection{Feature bit
> > requirements}\label{sec:Device Types / Network Device
> > \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> > \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or
> > VIRTIO_NET_F_HOST_TSO6.
> > \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> > +\item[VIRTIO_NET_F_VQ_NOTF_COAL] 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} @@ -1508,6
> > +1511,14 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types /
> > Network Device / Devi If the VIRTIO_NET_F_NOTF_COAL feature is negotiated,
> > the driver can send control commands for dynamically changing the
> > coalescing
> > parameters.
> >
> > +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
> > +\begin{itemize}
> > +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
> > to set coalescing parameters of a given
> > + enabled transmit/receive virtqueue.
> > +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command
> > to a device, and the device responds with
> > + coalescing parameters of a given enabled transmit/receive virtqueue.
> > +\end{itemize}
> > +
> > \begin{note}
> > The behavior of the device in response to these commands is best-effort:
> > the device may generate notifications more or less frequently than
> > specified.
> > @@ -1519,25 +1530,76 @@ \subsubsection{Control
> > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > le32 max_usecs;
> > };
> >
> > +struct virtio_net_ctrl_coal_vq {
> > +le16 vqn;
> > +le16 reserved;
> > +struct virtio_net_ctrl_coal coal;
> > +};
> > +
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET 0
> > #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2 #define
> > + VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
> > \end{lstlisting}
> >
> > +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> > +VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
> > virtio_net_ctrl_coal
> > +structure to set \field{max_usecs} and \field{max_packets} for all
> > transmit/receive virtqueues.
> > +
> > +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the
> > +virtio_net_ctrl_coal_vq structure to set \field{max_usecs} and
> > \field{max_packets} for the supplied virtqueue number \field{vqn}.
> > +
> > +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of
> > +\field{max_usecs} and \field{max_packets} of the specified virtqueue
> > +from the device by setting \field{vqn} in the virtio_net_ctrl_coal_vq
> > structure.
> > +
>
> > +# Read/Write attributes for coalescing parameters \begin{itemize} \item
> > +For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> > + and \field{max_packets} are write-only for a driver.
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn},
>
> Virtio spec is using vq number and vq index terminology interchangeably.
> For example, a new patch about admin vq, uses aq_start_index.
>
> MMIO device has vq_index register too.
> I am inclined to vq_index given current state of spec
[mst@tuck virtio]$ git grep vq_index|wc -l
0
> and new additions by Michael.
admin vq? Good point, I will change that to vqn too.
> Michael,
> Can you please suggest vqn or vq_index to use?
[mst@tuck virtio]$ git grep vqn|wc -l
5
I'd say vqn has precedent.
> > \field{reserved}, \field{max_usecs}
> > + and \field{max_packets} are write-only for a driver.
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
> > and \field{reserved} are write-only
>
>
> > + for a driver, and, \field{max_usecs} and \field{max_packets} are
> > read-only
> > for a driver.
> Remove this trailing "for a driver", it is a duplicate.
Nope it's required, without it, it's unclear which side is doing reading
and which writing. Unfortunate that it's a bit verbose but we don't have
better terms in the spec right now.
> > +\end{itemize}
> > +
> > Coalescing
Re: [virtio-dev] RE: [PATCH v9] virtio-net: support inner header hash
On Wed, Feb 22, 2023 at 10:34:39AM +0800, Heng Qi wrote: > > The user will figure out how to mitigate when such QoS is not available. > > Either to run in best-effort mode or mitigate differently. > > Yes, our cloud security and cloud network team will configure and use inner > hash on dpdk. Sounds good. More practical for dpdk than Linux. Is there a chance that when the interface is close to be final, but before the vote, you post a patch to the dpdk list and get some acks from the maintainers, cc virtio-dev. This way we won't merge something that will then go unused? That would be best - do you have a prototype? > In fact I discussed with them the security issues between > tunnels, > and I will quote their solutions to tunnel attacks below, but this is a > problem between the tunnels, not the introduction of inner hash. > I don't think we need to focus too much on this, but I'll do my best to > describe the security issues between tunnels in v10. > > " > This is not a problem with the inner hash, it is a general problem with the > outer hash. > I communicated with our people who are doing cloud security (they are also > one of the demanders of inner hash), > and it is a common problem for one tunnel to attack another tunnel. > > For example, there is a tunnel t1; a tunnel t2; a tunnel endpoint VTEP0, and > the vni id of t1 is id1, and the vni id of v2 is id2; a VM. > > At this time, regardless of the inner hash or the outer hash, the traffic of > tunnel t1 and tunnel t2 will reach the VM through VTEP0 (whether it is a > single queue or multiple queues), > and may be placed on the same queue to cause queue overflow. Do note (and explain in spec?) that with just an outer hash and RSS it is possible to configure the tunnels to use distict queues. Impossible with this interface but arguably only works for a small number of tunnels anyway. > # Solutions: More like mitigations. > 1. Some current forwarding tools such as DPDK have good forwarding > performance, and it is difficult to fill up the queue; Oh that's a good point. If driver is generally faster than the device and queues stay away from filling up there's no DoS. I'd add this to the spec. > 2. or switch the attack traffic to the attack clusters; What is that? > 3. or connect the traffic of different tunnels to different network card > ports or network devices. Not sure how this is relevant. These a distinct outer MAC - with this why do we need a tunnel? > 4.. > " - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v6] virtio-net: support the virtqueue coalescing moderation
> From: Heng Qi
> Sent: Tuesday, February 21, 2023 10:22 PM
> +\item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue
> notification coalescing.
> +
s/notification/notifications
should be plural as multiple notifications are coalesced like the below
description of VIRTIO_NET_F_NOTF_COAL.
> \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> coalescing.
>
> \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> @@ -139,6 +141,7 @@ \subsubsection{Feature bit
> requirements}\label{sec:Device Types / Network Device
> \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or
> VIRTIO_NET_F_HOST_TSO6.
> \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> +\item[VIRTIO_NET_F_VQ_NOTF_COAL] 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} @@ -1508,6
> +1511,14 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types /
> Network Device / Devi If the VIRTIO_NET_F_NOTF_COAL feature is negotiated,
> the driver can send control commands for dynamically changing the coalescing
> parameters.
>
> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
> +\begin{itemize}
> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
> to set coalescing parameters of a given
> + enabled transmit/receive virtqueue.
> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command
> to a device, and the device responds with
> + coalescing parameters of a given enabled transmit/receive virtqueue.
> +\end{itemize}
> +
> \begin{note}
> The behavior of the device in response to these commands is best-effort:
> the device may generate notifications more or less frequently than specified.
> @@ -1519,25 +1530,76 @@ \subsubsection{Control
> Virtqueue}\label{sec:Device Types / Network Device / Devi
> le32 max_usecs;
> };
>
> +struct virtio_net_ctrl_coal_vq {
> +le16 vqn;
> +le16 reserved;
> +struct virtio_net_ctrl_coal coal;
> +};
> +
> #define VIRTIO_NET_CTRL_NOTF_COAL 6
> #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET 0
> #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2 #define
> + VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
> \end{lstlisting}
>
> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> +VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
> virtio_net_ctrl_coal
> +structure to set \field{max_usecs} and \field{max_packets} for all
> transmit/receive virtqueues.
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the
> +virtio_net_ctrl_coal_vq structure to set \field{max_usecs} and
> \field{max_packets} for the supplied virtqueue number \field{vqn}.
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of
> +\field{max_usecs} and \field{max_packets} of the specified virtqueue
> +from the device by setting \field{vqn} in the virtio_net_ctrl_coal_vq
> structure.
> +
> +# Read/Write attributes for coalescing parameters \begin{itemize} \item
> +For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn},
Virtio spec is using vq number and vq index terminology interchangeably.
For example, a new patch about admin vq, uses aq_start_index.
MMIO device has vq_index register too.
I am inclined to vq_index given current state of spec and new additions by
Michael.
Michael,
Can you please suggest vqn or vq_index to use?
> \field{reserved}, \field{max_usecs}
> + and \field{max_packets} are write-only for a driver.
> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn}
> and \field{reserved} are write-only
> + for a driver, and, \field{max_usecs} and \field{max_packets} are
> read-only
> for a driver.
Remove this trailing "for a driver", it is a duplicate.
> +\end{itemize}
> +
> Coalescing parameters:
> \begin{itemize}
> +\item \field{vqn}: The virtqueue number of an enabled transmit or receive
> virtqueue.
> \item \field{max_usecs} for RX: Maximum number of microseconds to delay a
> RX notification.
> \item \field{max_usecs} for TX: Maximum number of microseconds to delay a
> TX notification.
> \item \field{max_packets} for RX: Maximum number of packets to receive
> before a RX notification.
> \item \field{max_packets} for TX: Maximum number of packets to send before
> a TX notification.
> \end{itemize}
>
> -The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> +\field{vqn} points to an enabled transmit/receive virtqueue, and its value
> satisfies $ 0 \leq vqn < max_virtqueue_pairs \ast 2 $.
> +
This calculation description is not needed here. It is covered somewhere else.
> +\field{reserved} is reserved and it is ignored by a device.
> +
> +The class
[virtio-dev] Re: [PATCH v9] virtio-net: support inner header hash
在 2023/2/22 01:50, Michael S. Tsirkin 写道:
On Sat, Feb 18, 2023 at 10:37:15PM +0800, Heng Qi wrote:
+\subparagraph{Security risks between encapsulated packets and RSS}
+There may be potential security risks when encapsulated packets using RSS to
+select queues for placement. When a user inside a tunnel tries to control the
What do you mean by "user" here? Is it a remote or local one?
+enqueuing of encapsulated packets, then the user can flood the device with
invaild
+packets, and the flooded packets may be hashed into the same queue as packets
in
+other normal tunnels, which causing the queue to overflow.
+
+This can pose several security risks:
+\begin{itemize}
+\item Encapsulated packets in the normal tunnels cannot be enqueued due to
queue
+ overflow, resulting in a large amount of packet loss.
+\item The delay and retransmission of packets in the normal tunnels are
extremely increased.
+\item The user can observe the traffic information and enqueue information of
other normal
+ tunnels, and conduct targeted DoS attacks.
+\end{\itemize}
+
Hmm with this all written out it sounds pretty severe.
I think we need first understand whether or not it's a problem that we
need to solve at spec level:
1) anything make encapsulated packets different or why we can't hit this
problem without encapsulation
2) whether or not it's the implementation details that the spec doesn't
need to care (or how it is solved in real NIC)
Thanks
At this point with no ways to mitigate, I don't feel this is something
e.g. Linux can enable. I am not going to nack the spec patch if
others find this somehow useful e.g. for dpdk.
How about CC e.g. dpdk devs or whoever else is going to use this
and asking them for the opinion?
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v9] virtio-net: support inner header hash
在 2023/2/22 上午7:18, Michael S. Tsirkin 写道:
On Tue, Feb 21, 2023 at 10:32:11PM +, Parav Pandit wrote:
From: Michael S. Tsirkin
Sent: Tuesday, February 21, 2023 4:46 PM
What is this information driver can't observe? It sees all the packets after
all,
we are not stripping tunneling headers.
Just the tunnel type.
If/when that tunnel header is stripped, it gets complicated where tunnel type
is still present in the virtio_net_hdr because hash_report_tunnel feature bit
is negotiated.
whoever strips off the tunnel has I imagine strip off the virtio net hdr
too - everything else in it such as gso type refers to the outer packet.
I also don't really know what are upper layer drivers - for sure layering of
drivers is not covered in the spec for now so I am not sure what do you mean by
that. The risk I mentioned is leaking the information *on the network*.
Got it.
\begin{lstlisting} struct virtio_net_rss_config {
le32 hash_types;
+le32 hash_tunnel_types;
This field is not needed as device config space advertisement
for the support
is enough.
If the intent is to enable hashing for the specific tunnel(s),
an individual
command is better.
new command? I am not sure why we want that. why not handle
tunnels like we do other protocols?
I didn't follow.
We probably discussed in another thread that to set M bits, it is
wise to avoid
setting N other bits just to keep the command happy, where N >>> M
and these N have a very strong relation in hw resource setup and packet
steering.
Any examples of 'other protocols'?
#define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
#define VIRTIO_NET_HASH_TYPE_TCPv4 (1 << 1)
#define VIRTIO_NET_HASH_TYPE_UDPv4 (1 << 2)
this kind of thing.
I don't see how a tunnel is different fundamentally. Why does it
need its own field?
Driver is in control to enable/disable tunnel based inner hash acceleration
only when its needed.
This way certain data path hw parsers can be enabled/disabled.
Without this it will be always enabled even if there may not be any user of it.
Device has scope to optimize this flow.
I feel you misunderstand the question. Or maybe I misunderstand what you are
proposing. So tunnels need their own bits. But why a separate field and not
just
more bits along the existing ones?
Because the hashing is not covering the outer header contents.
We may be still not discussing the same.
So let me refresh the context.
The question of discussion was,
Scenario:
1. device advertises the ability to hash on the inner packet header.
2. device prefers that driver enable it only when it needs to use this extra
packet parser in hardware.
There are 3 options.
a. Because the feature is negotiated, it means it is enabled for all the tunnel
types.
Pros:
1. No need to extend cvq cmd.
Cons:
1. device parser is always enabled, and the driver never uses it. This may
result in inferior rx performance.
b. Since the feature is useful in a narrow case of sw-based vxlan etc driver,
better not to enable hw for it.
Hence, have the knob to explicitly enable in hw.
So have the cvq command.
b.1 should it be combined with the existing command?
Cons:
a. when the driver wants to enable hash on inner, it needs to supply the exact
same RSS config as before. Sw overhead with no gain.
b. device needs to parse new command value, compare with old config, and drop
the RSS config, just enable inner hashing hw parser.
Or destroy the old rss config and re-apply. This results in weird behavior for
the short interval with no apparent gain.
b.2 should it be on its own command?
Pros:
a. device and driver doesn't need to bother about b.1.a and b.1.b.
b. still benefits from not always enabling hw parser, as this is not a common
case.
c. has the ability to enable when needed.
I prefer b.1. With reporting of the tunnel type gone I don't see a
fundamental difference between hashing over tunneling types and other
protocol types we support. It's just a flag telling device over which
bits to calculate the hash. We don't have a separate command for hashing
of TCPv6, why have it for vxlan? Extending with more HASH_TYPE makes
total sense to me, seems to fit better with the existing design and will
make patch smaller.
+1.
It is infrequent to configure the *tunnel hash types* through commands,
and when configuring the *hash types*,
the hash key and indirection table are not required too.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v9] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Tuesday, February 21, 2023 6:18 PM > > The question of discussion was, > > Scenario: > > 1. device advertises the ability to hash on the inner packet header. > > 2. device prefers that driver enable it only when it needs to use this extra > packet parser in hardware. > > > > There are 3 options. > > a. Because the feature is negotiated, it means it is enabled for all the > > tunnel > types. > > Pros: > > 1. No need to extend cvq cmd. > > Cons: > > 1. device parser is always enabled, and the driver never uses it. This may > result in inferior rx performance. > > > > b. Since the feature is useful in a narrow case of sw-based vxlan etc > > driver, > better not to enable hw for it. > > Hence, have the knob to explicitly enable in hw. > > So have the cvq command. > > b.1 should it be combined with the existing command? > > Cons: > > a. when the driver wants to enable hash on inner, it needs to supply the > > exact > same RSS config as before. Sw overhead with no gain. > > b. device needs to parse new command value, compare with old config, and > drop the RSS config, just enable inner hashing hw parser. > > Or destroy the old rss config and re-apply. This results in weird behavior > > for > the short interval with no apparent gain. > > > > b.2 should it be on its own command? > > Pros: > > a. device and driver doesn't need to bother about b.1.a and b.1.b. > > b. still benefits from not always enabling hw parser, as this is not a > > common > case. > > c. has the ability to enable when needed. > > I prefer b.1. With reporting of the tunnel type gone I don't see a fundamental > difference between hashing over tunneling types and other protocol types we > support. It's just a flag telling device over which bits to calculate the > hash. We > don't have a separate command for hashing of TCPv6, why have it for vxlan? b.1 to always enable hw for multi-level packet processing is not very optimal for actual device implementation. The difference is one level of header vs second-level hashing. And new hash type values have zero use of it in sw. > Extending with more HASH_TYPE makes total sense to me, seems to fit better > with the existing design and will make patch smaller. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9] virtio-net: support inner header hash
On Tue, Feb 21, 2023 at 10:32:11PM +, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin
> > Sent: Tuesday, February 21, 2023 4:46 PM
> >
> > What is this information driver can't observe? It sees all the packets
> > after all,
> > we are not stripping tunneling headers.
> Just the tunnel type.
> If/when that tunnel header is stripped, it gets complicated where tunnel type
> is still present in the virtio_net_hdr because hash_report_tunnel feature bit
> is negotiated.
whoever strips off the tunnel has I imagine strip off the virtio net hdr
too - everything else in it such as gso type refers to the outer packet.
> > I also don't really know what are upper layer drivers - for sure layering of
> > drivers is not covered in the spec for now so I am not sure what do you
> > mean by
> > that. The risk I mentioned is leaking the information *on the network*.
> >
> Got it.
>
> >
> >
> >
> > > > > > \begin{lstlisting} struct virtio_net_rss_config {
> > > > > > > > le32 hash_types;
> > > > > > > > +le32 hash_tunnel_types;
> > > > > > > This field is not needed as device config space advertisement
> > > > > > > for the support
> > > > > > is enough.
> > > > > > >
> > > > > > > If the intent is to enable hashing for the specific tunnel(s),
> > > > > > > an individual
> > > > > > command is better.
> > > > > >
> > > > > > new command? I am not sure why we want that. why not handle
> > > > > > tunnels like we do other protocols?
> > > > >
> > > > > I didn't follow.
> > > > > We probably discussed in another thread that to set M bits, it is
> > > > > wise to avoid
> > > > setting N other bits just to keep the command happy, where N >>> M
> > > > and these N have a very strong relation in hw resource setup and packet
> > steering.
> > > > > Any examples of 'other protocols'?
> > > >
> > > > #define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
> > > > #define VIRTIO_NET_HASH_TYPE_TCPv4 (1 << 1)
> > > > #define VIRTIO_NET_HASH_TYPE_UDPv4 (1 << 2)
> > > >
> > > > this kind of thing.
> > > >
> > > > I don't see how a tunnel is different fundamentally. Why does it
> > > > need its own field?
> > >
> > > Driver is in control to enable/disable tunnel based inner hash
> > > acceleration
> > only when its needed.
> > > This way certain data path hw parsers can be enabled/disabled.
> > > Without this it will be always enabled even if there may not be any user
> > > of it.
> > > Device has scope to optimize this flow.
> >
> > I feel you misunderstand the question. Or maybe I misunderstand what you are
> > proposing. So tunnels need their own bits. But why a separate field and
> > not just
> > more bits along the existing ones?
>
> Because the hashing is not covering the outer header contents.
>
> We may be still not discussing the same.
> So let me refresh the context.
>
> The question of discussion was,
> Scenario:
> 1. device advertises the ability to hash on the inner packet header.
> 2. device prefers that driver enable it only when it needs to use this extra
> packet parser in hardware.
>
> There are 3 options.
> a. Because the feature is negotiated, it means it is enabled for all the
> tunnel types.
> Pros:
> 1. No need to extend cvq cmd.
> Cons:
> 1. device parser is always enabled, and the driver never uses it. This may
> result in inferior rx performance.
>
> b. Since the feature is useful in a narrow case of sw-based vxlan etc driver,
> better not to enable hw for it.
> Hence, have the knob to explicitly enable in hw.
> So have the cvq command.
> b.1 should it be combined with the existing command?
> Cons:
> a. when the driver wants to enable hash on inner, it needs to supply the
> exact same RSS config as before. Sw overhead with no gain.
> b. device needs to parse new command value, compare with old config, and drop
> the RSS config, just enable inner hashing hw parser.
> Or destroy the old rss config and re-apply. This results in weird behavior
> for the short interval with no apparent gain.
>
> b.2 should it be on its own command?
> Pros:
> a. device and driver doesn't need to bother about b.1.a and b.1.b.
> b. still benefits from not always enabling hw parser, as this is not a common
> case.
> c. has the ability to enable when needed.
I prefer b.1. With reporting of the tunnel type gone I don't see a
fundamental difference between hashing over tunneling types and other
protocol types we support. It's just a flag telling device over which
bits to calculate the hash. We don't have a separate command for hashing
of TCPv6, why have it for vxlan? Extending with more HASH_TYPE makes
total sense to me, seems to fit better with the existing design and will
make patch smaller.
--
MST
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v9] virtio-net: support inner header hash
> From: Michael S. Tsirkin
> Sent: Tuesday, February 21, 2023 4:46 PM
>
> What is this information driver can't observe? It sees all the packets after
> all,
> we are not stripping tunneling headers.
Just the tunnel type.
If/when that tunnel header is stripped, it gets complicated where tunnel type
is still present in the virtio_net_hdr because hash_report_tunnel feature bit
is negotiated.
> I also don't really know what are upper layer drivers - for sure layering of
> drivers is not covered in the spec for now so I am not sure what do you mean
> by
> that. The risk I mentioned is leaking the information *on the network*.
>
Got it.
>
>
>
> > > > > \begin{lstlisting} struct virtio_net_rss_config {
> > > > > > > le32 hash_types;
> > > > > > > +le32 hash_tunnel_types;
> > > > > > This field is not needed as device config space advertisement
> > > > > > for the support
> > > > > is enough.
> > > > > >
> > > > > > If the intent is to enable hashing for the specific tunnel(s),
> > > > > > an individual
> > > > > command is better.
> > > > >
> > > > > new command? I am not sure why we want that. why not handle
> > > > > tunnels like we do other protocols?
> > > >
> > > > I didn't follow.
> > > > We probably discussed in another thread that to set M bits, it is
> > > > wise to avoid
> > > setting N other bits just to keep the command happy, where N >>> M
> > > and these N have a very strong relation in hw resource setup and packet
> steering.
> > > > Any examples of 'other protocols'?
> > >
> > > #define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
> > > #define VIRTIO_NET_HASH_TYPE_TCPv4 (1 << 1)
> > > #define VIRTIO_NET_HASH_TYPE_UDPv4 (1 << 2)
> > >
> > > this kind of thing.
> > >
> > > I don't see how a tunnel is different fundamentally. Why does it
> > > need its own field?
> >
> > Driver is in control to enable/disable tunnel based inner hash acceleration
> only when its needed.
> > This way certain data path hw parsers can be enabled/disabled.
> > Without this it will be always enabled even if there may not be any user of
> > it.
> > Device has scope to optimize this flow.
>
> I feel you misunderstand the question. Or maybe I misunderstand what you are
> proposing. So tunnels need their own bits. But why a separate field and not
> just
> more bits along the existing ones?
Because the hashing is not covering the outer header contents.
We may be still not discussing the same.
So let me refresh the context.
The question of discussion was,
Scenario:
1. device advertises the ability to hash on the inner packet header.
2. device prefers that driver enable it only when it needs to use this extra
packet parser in hardware.
There are 3 options.
a. Because the feature is negotiated, it means it is enabled for all the tunnel
types.
Pros:
1. No need to extend cvq cmd.
Cons:
1. device parser is always enabled, and the driver never uses it. This may
result in inferior rx performance.
b. Since the feature is useful in a narrow case of sw-based vxlan etc driver,
better not to enable hw for it.
Hence, have the knob to explicitly enable in hw.
So have the cvq command.
b.1 should it be combined with the existing command?
Cons:
a. when the driver wants to enable hash on inner, it needs to supply the exact
same RSS config as before. Sw overhead with no gain.
b. device needs to parse new command value, compare with old config, and drop
the RSS config, just enable inner hashing hw parser.
Or destroy the old rss config and re-apply. This results in weird behavior for
the short interval with no apparent gain.
b.2 should it be on its own command?
Pros:
a. device and driver doesn't need to bother about b.1.a and b.1.b.
b. still benefits from not always enabling hw parser, as this is not a common
case.
c. has the ability to enable when needed.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9] virtio-net: support inner header hash
On Tue, Feb 21, 2023 at 09:36:06PM +, Parav Pandit wrote:
> > So you are saying either live with the problem (this is best effort yes?)
> Yes to best effort usage.
For sure something can be done to mitigate? How about randomizing the
key for example? That's in just like 1 minute of thinking. I am guessing
more can be done.
> >
> >
> > > > > > +\item The user can observe the traffic information and enqueue
> > > > > > +information
> > > > > > of other normal
> > > > > > + tunnels, and conduct targeted DoS attacks.
> > > > > Once hash_report_tunnel_types is removed, this second attack is no
> > > > > longer
> > > > applicable.
> > > > > Hence, please remove this too.
> > > >
> > > >
> > > > ?
> > > > I don't get how removing a field helps DoS.
> > > >
> > > I meant for the "observe and enqueue" part of the tunnel as not
> > > applicable.
> >
> > Sorry still don't get it :( I don't know what is the "observe and enqueue"
> > part of
> > the tunnel and what is not applicable. But maybe Heng Qi does.
> >
> Tunnel type such as vxlan/gre etc is not placed in the virtio_net_hdr.
> This way the net_hdr doesn't leak such information to upper layer drivers as
> it cannot observe it.
What is this information driver can't observe? It sees all the packets
after all, we are not stripping tunneling headers.
I also don't really know what are upper layer drivers - for sure
layering of drivers is not covered in the spec for now so I am not sure
what do you mean by that. The risk I mentioned is leaking the
information *on the network*.
> > > > \begin{lstlisting} struct virtio_net_rss_config {
> > > > > > le32 hash_types;
> > > > > > +le32 hash_tunnel_types;
> > > > > This field is not needed as device config space advertisement for
> > > > > the support
> > > > is enough.
> > > > >
> > > > > If the intent is to enable hashing for the specific tunnel(s), an
> > > > > individual
> > > > command is better.
> > > >
> > > > new command? I am not sure why we want that. why not handle tunnels
> > > > like we do other protocols?
> > >
> > > I didn't follow.
> > > We probably discussed in another thread that to set M bits, it is wise to
> > > avoid
> > setting N other bits just to keep the command happy, where N >>> M and these
> > N have a very strong relation in hw resource setup and packet steering.
> > > Any examples of 'other protocols'?
> >
> > #define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
> > #define VIRTIO_NET_HASH_TYPE_TCPv4 (1 << 1)
> > #define VIRTIO_NET_HASH_TYPE_UDPv4 (1 << 2)
> >
> > this kind of thing.
> >
> > I don't see how a tunnel is different fundamentally. Why does it need its
> > own
> > field?
>
> Driver is in control to enable/disable tunnel based inner hash acceleration
> only when its needed.
> This way certain data path hw parsers can be enabled/disabled.
> Without this it will be always enabled even if there may not be any user of
> it.
> Device has scope to optimize this flow.
I feel you misunderstand the question. Or maybe I misunderstand what you
are proposing. So tunnels need their own bits. But why a separate field
and not just more bits along the existing ones?
--
MST
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v9] virtio-net: support inner header hash
> From: Michael S. Tsirkin
> Sent: Tuesday, February 21, 2023 4:24 PM
>
> On Tue, Feb 21, 2023 at 07:29:20PM +, Parav Pandit wrote:
> > > > When a specific receive queue is shared to receive packets of
> > > > multiple
> > > tunnels, there is no quality of service for packets of multiple tunnels.
> > >
> > > "shared to receive" is not grammatical either :)
> > >
> > "Shared by multiple tunnels" will make it grammatical?
>
> I think so, yes.
>
> > > If you are talking about a security risk you need to explain
> > > 1- what is the threat, what configurations are affected.
> > > 2- what is the attack type: DOS, information leak, etc.
> > > 3- how to mitigate it
> > >
> > > This text touches a bit on 1 and 2 but not in an ordererly way.
> > >
> > >
> > it is best effort based.
> >
> > #3 is outside the scope of this patch set.
>
> Scope is from greek for "target". It's what we are aiming for.
> If we document a security risk then I would say yes we should aim to provide
> not just problems but solutions too.
>
> > > > +
> > > > > +This can pose several security risks:
> > > > > +\begin{itemize}
> > > > > +\item Encapsulated packets in the normal tunnels cannot be
> > > > > +enqueued due to
> > > > > queue
> > > > > + overflow, resulting in a large amount of packet loss.
> > > > > +\item The delay and retransmission of packets in the normal
> > > > > +tunnels are
> > > > > extremely increased.
> > > > This is something very protocol specific and doesn't belong here.
> > >
> > > I don't see how it's specific - many protocols have retransmission
> > > and are affected by delays. "extremely increased" sounds unrammatical to
> me though.
> > >
> > >
> > I am not sure where you want to lead this discussion.
>
> I just disagree that documenting timing effects does not belong in the spec.
>
> > I am trying to help the spec and feature definition to be compact enough to
> progress.
> >
> > It is specific to a protocol(s) and somehow arbitrarily concluded with a
> > large
> number of packet losses.
> > Maybe only one ICMP packet got dropped and retransmit was just one
> packet.
> > Maybe it was TCP with selective retransmit enabled/disabled.
> >
> > As far as receive side is concerned, it should say that there is no QoS
> > among
> different tunnels.
> > The user will figure out how to mitigate when such QoS is not available.
> Either to run in best-effort mode or mitigate differently.
>
> So you are saying either live with the problem (this is best effort yes?)
Yes to best effort usage.
>
>
> > > > > +\item The user can observe the traffic information and enqueue
> > > > > +information
> > > > > of other normal
> > > > > + tunnels, and conduct targeted DoS attacks.
> > > > Once hash_report_tunnel_types is removed, this second attack is no
> > > > longer
> > > applicable.
> > > > Hence, please remove this too.
> > >
> > >
> > > ?
> > > I don't get how removing a field helps DoS.
> > >
> > I meant for the "observe and enqueue" part of the tunnel as not applicable.
>
> Sorry still don't get it :( I don't know what is the "observe and enqueue"
> part of
> the tunnel and what is not applicable. But maybe Heng Qi does.
>
Tunnel type such as vxlan/gre etc is not placed in the virtio_net_hdr.
This way the net_hdr doesn't leak such information to upper layer drivers as it
cannot observe it.
> > > \begin{lstlisting} struct virtio_net_rss_config {
> > > > > le32 hash_types;
> > > > > +le32 hash_tunnel_types;
> > > > This field is not needed as device config space advertisement for
> > > > the support
> > > is enough.
> > > >
> > > > If the intent is to enable hashing for the specific tunnel(s), an
> > > > individual
> > > command is better.
> > >
> > > new command? I am not sure why we want that. why not handle tunnels
> > > like we do other protocols?
> >
> > I didn't follow.
> > We probably discussed in another thread that to set M bits, it is wise to
> > avoid
> setting N other bits just to keep the command happy, where N >>> M and these
> N have a very strong relation in hw resource setup and packet steering.
> > Any examples of 'other protocols'?
>
> #define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
> #define VIRTIO_NET_HASH_TYPE_TCPv4 (1 << 1)
> #define VIRTIO_NET_HASH_TYPE_UDPv4 (1 << 2)
>
> this kind of thing.
>
> I don't see how a tunnel is different fundamentally. Why does it need its own
> field?
Driver is in control to enable/disable tunnel based inner hash acceleration
only when its needed.
This way certain data path hw parsers can be enabled/disabled.
Without this it will be always enabled even if there may not be any user of it.
Device has scope to optimize this flow.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9] virtio-net: support inner header hash
On Tue, Feb 21, 2023 at 07:29:20PM +, Parav Pandit wrote:
> > > When a specific receive queue is shared to receive packets of multiple
> > tunnels, there is no quality of service for packets of multiple tunnels.
> >
> > "shared to receive" is not grammatical either :)
> >
> "Shared by multiple tunnels" will make it grammatical?
I think so, yes.
> > If you are talking about a security risk you need to explain
> > 1- what is the threat, what configurations are affected.
> > 2- what is the attack type: DOS, information leak, etc.
> > 3- how to mitigate it
> >
> > This text touches a bit on 1 and 2 but not in an ordererly way.
> >
> >
> it is best effort based.
>
> #3 is outside the scope of this patch set.
Scope is from greek for "target". It's what we are aiming for.
If we document a security risk then I would say yes we should aim
to provide not just problems but solutions too.
> > > +
> > > > +This can pose several security risks:
> > > > +\begin{itemize}
> > > > +\item Encapsulated packets in the normal tunnels cannot be
> > > > +enqueued due to
> > > > queue
> > > > + overflow, resulting in a large amount of packet loss.
> > > > +\item The delay and retransmission of packets in the normal
> > > > +tunnels are
> > > > extremely increased.
> > > This is something very protocol specific and doesn't belong here.
> >
> > I don't see how it's specific - many protocols have retransmission and are
> > affected by delays. "extremely increased" sounds unrammatical to me though.
> >
> >
> I am not sure where you want to lead this discussion.
I just disagree that documenting timing effects does not belong in the
spec.
> I am trying to help the spec and feature definition to be compact enough to
> progress.
>
> It is specific to a protocol(s) and somehow arbitrarily concluded with a
> large number of packet losses.
> Maybe only one ICMP packet got dropped and retransmit was just one packet.
> Maybe it was TCP with selective retransmit enabled/disabled.
>
> As far as receive side is concerned, it should say that there is no QoS among
> different tunnels.
> The user will figure out how to mitigate when such QoS is not available.
> Either to run in best-effort mode or mitigate differently.
So you are saying either live with the problem (this is best effort yes?)
or find your own solutions? Such as?
> > > > +\item The user can observe the traffic information and enqueue
> > > > +information
> > > > of other normal
> > > > + tunnels, and conduct targeted DoS attacks.
> > > Once hash_report_tunnel_types is removed, this second attack is no longer
> > applicable.
> > > Hence, please remove this too.
> >
> >
> > ?
> > I don't get how removing a field helps DoS.
> >
> I meant for the "observe and enqueue" part of the tunnel as not applicable.
Sorry still don't get it :( I don't know what is the "observe and enqueue" part
of the tunnel
and what is not applicable. But maybe Heng Qi does.
> > \begin{lstlisting} struct virtio_net_rss_config {
> > > > le32 hash_types;
> > > > +le32 hash_tunnel_types;
> > > This field is not needed as device config space advertisement for the
> > > support
> > is enough.
> > >
> > > If the intent is to enable hashing for the specific tunnel(s), an
> > > individual
> > command is better.
> >
> > new command? I am not sure why we want that. why not handle tunnels like
> > we do other protocols?
>
> I didn't follow.
> We probably discussed in another thread that to set M bits, it is wise to
> avoid setting N other bits just to keep the command happy, where N >>> M and
> these N have a very strong relation in hw resource setup and packet steering.
> Any examples of 'other protocols'?
#define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
#define VIRTIO_NET_HASH_TYPE_TCPv4 (1 << 1)
#define VIRTIO_NET_HASH_TYPE_UDPv4 (1 << 2)
this kind of thing.
I don't see how a tunnel is different fundamentally. Why does it need
its own field?
--
MST
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v9] virtio-net: support inner header hash
> From: Michael S. Tsirkin
> Sent: Tuesday, February 21, 2023 12:06 PM
>
> On Tue, Feb 21, 2023 at 04:20:59AM +, Parav Pandit wrote:
> >
> > > From: Heng Qi
> > > Sent: Saturday, February 18, 2023 9:37 AM
> >
> > > If the tunnel is used to encapsulate the packets, the hash
> > > calculated using the
> > s/hash calculated/hash is calculated
> >
> > > outer header of the receive packets is always fixed for the same
> > > flow packets, i.e. they will be steered to the same receive queue.
> > >
> > A little descriptive commit message like below reads better to me.
> > Currently, when a received packet is an encapsulated packet meaning there
> is an outer and an inner header, virtio device is unable to calculate the
> hash for
> the inner header.
> > Due to this limitation, multiple different flows identified by the inner
> > header
> for the same outer header result in selecting the same receive queue.
> > This effectively disables the RSS, resulting in poor receive performance.
> >
> > Hence, to overcome this limitation, a new feature is introduced using a
> feature bit VIRTIO_NET_F_HASH_TUNNEL.
> > This feature enables the device to advertise the capability to calculate the
> hash for the inner packet header.
> > Thereby regaining better RSS performance in presence of outer packet
> header.
>
> I think this is a good description however Parav I think it is important to
> make
> contributors write their own commit messages so they know what is the reason
> for the proposed change.
Sure. Contributor can rewrite it.
> What's good for the goose is good for the gander -
> contributors should explain why their change to spec is benefitial but
> reviewers
> should also explain why their changes to the patch are benefitial, and "reads
> better to me" does not cut it - it does not allow the contributor to improve
> with
> time. It's more than about a single contribution, see?
>
I provided an example template on how to write problem_description -> solution
commit log.
At the beginning, I said "little descriptive" to explain why it reads better.
But it seems, even more verbosity is needed even for the reviewer to suggest.
I didn't see this often happening by other reviewers, but I make a note of it
now, at least I can improve from this feedback.
I imagined that the contributor would see the pattern as problem->solution in
the example commit log and follow in the future patches.
Giving example of current patch was best to describe how to write it.
> In this case I would say the issue is that motivation for the change is never
> explained.
> > When a specific receive queue is shared to receive packets of multiple
> tunnels, there is no quality of service for packets of multiple tunnels.
>
> "shared to receive" is not grammatical either :)
>
"Shared by multiple tunnels" will make it grammatical?
> If you are talking about a security risk you need to explain
> 1- what is the threat, what configurations are affected.
> 2- what is the attack type: DOS, information leak, etc.
> 3- how to mitigate it
>
> This text touches a bit on 1 and 2 but not in an ordererly way.
>
>
it is best effort based.
#3 is outside the scope of this patch set.
> > +
> > > +This can pose several security risks:
> > > +\begin{itemize}
> > > +\item Encapsulated packets in the normal tunnels cannot be
> > > +enqueued due to
> > > queue
> > > + overflow, resulting in a large amount of packet loss.
> > > +\item The delay and retransmission of packets in the normal
> > > +tunnels are
> > > extremely increased.
> > This is something very protocol specific and doesn't belong here.
>
> I don't see how it's specific - many protocols have retransmission and are
> affected by delays. "extremely increased" sounds unrammatical to me though.
>
>
I am not sure where you want to lead this discussion.
I am trying to help the spec and feature definition to be compact enough to
progress.
It is specific to a protocol(s) and somehow arbitrarily concluded with a large
number of packet losses.
Maybe only one ICMP packet got dropped and retransmit was just one packet.
Maybe it was TCP with selective retransmit enabled/disabled.
As far as receive side is concerned, it should say that there is no QoS among
different tunnels.
The user will figure out how to mitigate when such QoS is not available. Either
to run in best-effort mode or mitigate differently.
> > > +\item The user can observe the traffic information and enqueue
> > > +information
> > > of other normal
> > > + tunnels, and conduct targeted DoS attacks.
> > Once hash_report_tunnel_types is removed, this second attack is no longer
> applicable.
> > Hence, please remove this too.
>
>
> ?
> I don't get how removing a field helps DoS.
>
I meant for the "observe and enqueue" part of the tunnel as not applicable.
> \begin{lstlisting} struct virtio_net_rss_config {
> > > le32 hash_types;
> > > +le32 hash_tunnel_types;
> > This field is not needed as
[virtio-dev] Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
On Tue, Feb 21, 2023 at 05:59:52PM +, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin
> > Sent: Tuesday, February 21, 2023 12:52 PM
> >
> > On Tue, Feb 21, 2023 at 05:50:09PM +, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin
> > > > Sent: Tuesday, February 21, 2023 12:42 PM
> > > > >
> > > > > What does "bits (for the driver)" mean? It made sense together
> > > > > with "read-only", but I would drop "(for the driver)" as well.
> > > >
> > > > Ouch Parav are you making search and replace changes without reading
> > > > the result? Pls don't.
> > > >
> > > It was wrong to keep the "for the driver".
> > > I will fix this.
> > >
> > > >
> > > > > > VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> > > > > >
> > > > > > \begin{lstlisting}
> > > > > > @@ -167,14 +167,14 @@ \subsection{Device configuration
> > > > layout}\label{sec:Device Types / Network Device
> > > > > > #define VIRTIO_NET_S_ANNOUNCE2
> > > > > > \end{lstlisting}
> > > > > >
> > > > > > -The following driver-read-only field,
> > > > > > \field{max_virtqueue_pairs} only exists if
> > > > > > +The following field, \field{max_virtqueue_pairs} only exists if
> > > > > > VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. This field
> > > > > > specifies the maximum number of each of transmit and receive
> > > > > > virtqueues (receiveq1\ldots receiveqN and transmitq1\ldots
> > > > > > transmitqN
> > > > > > respectively) that can be configured once at least one of these
> > > > > > features is
> > > > negotiated.
> > > > > >
> > > > > > -The following driver-read-only field, \field{mtu} only exists
> > > > > > if -VIRTIO_NET_F_MTU is set. This field specifies the maximum
> > > > > > MTU for the driver to
> > > > > > +The following field, \field{mtu} only exists if
> > > > > > +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU
> > > > > > +for the driver to
> > > > > > use.
> > > > > >
> > > > > > The following two fields, \field{speed} and \field{duplex},
> > > > > > only @@
> > > > > > -261,6 +261,8 @@ \subsection{Device configuration
> > > > > > layout}\label{sec:Device Types / Network Device
> > > > > >
> > > > > > \drivernormative{\subsubsection}{Device configuration
> > > > > > layout}{Device Types / Network Device / Device configuration
> > > > > > layout}
> > > > > >
> > > > > > +All the device configuration fields are read-only for the driver.
> > > > >
> > > > > Not sure if this makes a good normative clause, I would rather
> > > > > give the driver something actionable:
> > > > >
> > > > > "A driver SHOULD NOT try to write to any of the device
> > > > > configuration fields."
> > > >
> > > > Agree it's not a normative statement as is.
> > > > MUST NOT actually - they were always read only.
> > > > And no need to "try" just don't write period.
> > > >
> > > Saying driver must not write it, doesn't make it read only for the device.
> >
> > no but this is not what your patch said either. It's read only for the
> > driver.
> >
> It is read-only for the driver because the device doesn't treat them as
> writable.
> Do you mean it should be better written as,
>
> These fields are read-only for the driver, hence any writes by the driver to
> it will be ignored by the device.
>
> ?
I just concur with Cornelia.
> > > Hence, it should be mentioned as read-only fields, so when the driver
> > > writes
> > something to read-only fields, it can be considered as undefined behavior on
> > such fields.
> > >
> >
> > In the description not in the normative statements. normative sections just
> > tell
> > driver what it must and must not do, in the standard RFC terms.
> >
> Got it.
> I will shift them as read-only in the description section.
> And normative in the device and driver section.
> Device section:
> Any writes to config space fields is ignored by the device, because these are
> read-only fields for the driver.
writes is plural so "are ignored"
but more importantly use rfc terms in normative sections.
>
> Driver section:
> Driver must not write to read-only fields.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
> From: Michael S. Tsirkin
> Sent: Tuesday, February 21, 2023 12:52 PM
>
> On Tue, Feb 21, 2023 at 05:50:09PM +, Parav Pandit wrote:
> >
> >
> > > From: Michael S. Tsirkin
> > > Sent: Tuesday, February 21, 2023 12:42 PM
> > > >
> > > > What does "bits (for the driver)" mean? It made sense together
> > > > with "read-only", but I would drop "(for the driver)" as well.
> > >
> > > Ouch Parav are you making search and replace changes without reading
> > > the result? Pls don't.
> > >
> > It was wrong to keep the "for the driver".
> > I will fix this.
> >
> > >
> > > > > VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> > > > >
> > > > > \begin{lstlisting}
> > > > > @@ -167,14 +167,14 @@ \subsection{Device configuration
> > > layout}\label{sec:Device Types / Network Device
> > > > > #define VIRTIO_NET_S_ANNOUNCE2
> > > > > \end{lstlisting}
> > > > >
> > > > > -The following driver-read-only field,
> > > > > \field{max_virtqueue_pairs} only exists if
> > > > > +The following field, \field{max_virtqueue_pairs} only exists if
> > > > > VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. This field
> > > > > specifies the maximum number of each of transmit and receive
> > > > > virtqueues (receiveq1\ldots receiveqN and transmitq1\ldots
> > > > > transmitqN
> > > > > respectively) that can be configured once at least one of these
> > > > > features is
> > > negotiated.
> > > > >
> > > > > -The following driver-read-only field, \field{mtu} only exists
> > > > > if -VIRTIO_NET_F_MTU is set. This field specifies the maximum
> > > > > MTU for the driver to
> > > > > +The following field, \field{mtu} only exists if
> > > > > +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU
> > > > > +for the driver to
> > > > > use.
> > > > >
> > > > > The following two fields, \field{speed} and \field{duplex},
> > > > > only @@
> > > > > -261,6 +261,8 @@ \subsection{Device configuration
> > > > > layout}\label{sec:Device Types / Network Device
> > > > >
> > > > > \drivernormative{\subsubsection}{Device configuration
> > > > > layout}{Device Types / Network Device / Device configuration
> > > > > layout}
> > > > >
> > > > > +All the device configuration fields are read-only for the driver.
> > > >
> > > > Not sure if this makes a good normative clause, I would rather
> > > > give the driver something actionable:
> > > >
> > > > "A driver SHOULD NOT try to write to any of the device
> > > > configuration fields."
> > >
> > > Agree it's not a normative statement as is.
> > > MUST NOT actually - they were always read only.
> > > And no need to "try" just don't write period.
> > >
> > Saying driver must not write it, doesn't make it read only for the device.
>
> no but this is not what your patch said either. It's read only for the driver.
>
It is read-only for the driver because the device doesn't treat them as
writable.
Do you mean it should be better written as,
These fields are read-only for the driver, hence any writes by the driver to it
will be ignored by the device.
?
> > Hence, it should be mentioned as read-only fields, so when the driver writes
> something to read-only fields, it can be considered as undefined behavior on
> such fields.
> >
>
> In the description not in the normative statements. normative sections just
> tell
> driver what it must and must not do, in the standard RFC terms.
>
Got it.
I will shift them as read-only in the description section.
And normative in the device and driver section.
Device section:
Any writes to config space fields is ignored by the device, because these are
read-only fields for the driver.
Driver section:
Driver must not write to read-only fields.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
RE: [virtio-dev] RE: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Tuesday, February 21, 2023 12:44 PM > > On Tue, Feb 21, 2023 at 05:40:51PM +, Parav Pandit wrote: > > > > > From: Michael S. Tsirkin > > > Sent: Tuesday, February 21, 2023 12:14 PM > > > > The part that I am missing is, how do to reuse > > > > virtio_net_hash_config and say > > > ignore all the existing fields related to rss, but only consider > > > hash_tunnel_types? > > > > > > Like a union? The answer is, don't. Just lay out fields one after > > > another. > > > > > In that case driver needs to fill up all the fields which are not > > related to hash_tunnel_types and the device also needs to compare with > > the previous config and ignore it. Doesn’t look like a good use of > > existing commands and sw/fw usage for it. Shouldn’t we have the > > explicit command for setting tunnel types? > > I don't know what's proposed at this point, this is too vague. Proposal is to have new command like how Heng drafted in latest email. > I feel which tunnels to hash for inner header is not different from which > transports to hash. If device wants to know what changes it can compare. I > expect generally devices will just apply the new config without caring what > changed exactly. In a device when things are programmed, it often requires removing the previous configuration and re-apply the new one. This results in wrong steering for several tens of micro to milli seconds for no apparent reason. Hence, comparison is often needed for the best experience. And those are just overheads on the device and driver side without any apparent gain other than reusing some structure of rss. Hence, a separate command is more efficient choice.
[virtio-dev] Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
On Tue, Feb 21, 2023 at 05:50:09PM +, Parav Pandit wrote:
>
>
> > From: Michael S. Tsirkin
> > Sent: Tuesday, February 21, 2023 12:42 PM
> > >
> > > What does "bits (for the driver)" mean? It made sense together with
> > > "read-only", but I would drop "(for the driver)" as well.
> >
> > Ouch Parav are you making search and replace changes without reading the
> > result? Pls don't.
> >
> It was wrong to keep the "for the driver".
> I will fix this.
>
> >
> > > > VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> > > >
> > > > \begin{lstlisting}
> > > > @@ -167,14 +167,14 @@ \subsection{Device configuration
> > layout}\label{sec:Device Types / Network Device
> > > > #define VIRTIO_NET_S_ANNOUNCE2
> > > > \end{lstlisting}
> > > >
> > > > -The following driver-read-only field, \field{max_virtqueue_pairs}
> > > > only exists if
> > > > +The following field, \field{max_virtqueue_pairs} only exists if
> > > > VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. This field specifies
> > > > the maximum number of each of transmit and receive virtqueues
> > > > (receiveq1\ldots receiveqN and transmitq1\ldots transmitqN
> > > > respectively) that can be configured once at least one of these
> > > > features is
> > negotiated.
> > > >
> > > > -The following driver-read-only field, \field{mtu} only exists if
> > > > -VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for
> > > > the driver to
> > > > +The following field, \field{mtu} only exists if VIRTIO_NET_F_MTU is
> > > > +set. This field specifies the maximum MTU for the driver to
> > > > use.
> > > >
> > > > The following two fields, \field{speed} and \field{duplex}, only @@
> > > > -261,6 +261,8 @@ \subsection{Device configuration
> > > > layout}\label{sec:Device Types / Network Device
> > > >
> > > > \drivernormative{\subsubsection}{Device configuration
> > > > layout}{Device Types / Network Device / Device configuration layout}
> > > >
> > > > +All the device configuration fields are read-only for the driver.
> > >
> > > Not sure if this makes a good normative clause, I would rather give
> > > the driver something actionable:
> > >
> > > "A driver SHOULD NOT try to write to any of the device configuration
> > > fields."
> >
> > Agree it's not a normative statement as is.
> > MUST NOT actually - they were always read only.
> > And no need to "try" just don't write period.
> >
> Saying driver must not write it, doesn't make it read only for the device.
no but this is not what your patch said either. It's read only for the
driver.
> Hence, it should be mentioned as read-only fields, so when the driver writes
> something to read-only fields, it can be considered as undefined behavior on
> such fields.
>
In the description not in the normative statements. normative sections
just tell driver what it must and must not do, in the standard RFC
terms.
> > > > +
> > > > A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> > > > If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver
> > > > MUST set the physical address of the NIC to \field{mac}.
> > > > Otherwise, it SHOULD
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9] virtio-net: support inner header hash
On Sat, Feb 18, 2023 at 10:37:15PM +0800, Heng Qi wrote:
> +\subparagraph{Security risks between encapsulated packets and RSS}
> +There may be potential security risks when encapsulated packets using RSS to
> +select queues for placement. When a user inside a tunnel tries to control the
> +enqueuing of encapsulated packets, then the user can flood the device with
> invaild
> +packets, and the flooded packets may be hashed into the same queue as
> packets in
> +other normal tunnels, which causing the queue to overflow.
> +
> +This can pose several security risks:
> +\begin{itemize}
> +\item Encapsulated packets in the normal tunnels cannot be enqueued due to
> queue
> + overflow, resulting in a large amount of packet loss.
> +\item The delay and retransmission of packets in the normal tunnels are
> extremely increased.
> +\item The user can observe the traffic information and enqueue information
> of other normal
> + tunnels, and conduct targeted DoS attacks.
> +\end{\itemize}
> +
Hmm with this all written out it sounds pretty severe.
At this point with no ways to mitigate, I don't feel this is something
e.g. Linux can enable. I am not going to nack the spec patch if
others find this somehow useful e.g. for dpdk.
How about CC e.g. dpdk devs or whoever else is going to use this
and asking them for the opinion?
--
MST
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
> From: Michael S. Tsirkin
> Sent: Tuesday, February 21, 2023 12:42 PM
> >
> > What does "bits (for the driver)" mean? It made sense together with
> > "read-only", but I would drop "(for the driver)" as well.
>
> Ouch Parav are you making search and replace changes without reading the
> result? Pls don't.
>
It was wrong to keep the "for the driver".
I will fix this.
>
> > > VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> > >
> > > \begin{lstlisting}
> > > @@ -167,14 +167,14 @@ \subsection{Device configuration
> layout}\label{sec:Device Types / Network Device
> > > #define VIRTIO_NET_S_ANNOUNCE2
> > > \end{lstlisting}
> > >
> > > -The following driver-read-only field, \field{max_virtqueue_pairs}
> > > only exists if
> > > +The following field, \field{max_virtqueue_pairs} only exists if
> > > VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. This field specifies
> > > the maximum number of each of transmit and receive virtqueues
> > > (receiveq1\ldots receiveqN and transmitq1\ldots transmitqN
> > > respectively) that can be configured once at least one of these features
> > > is
> negotiated.
> > >
> > > -The following driver-read-only field, \field{mtu} only exists if
> > > -VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for
> > > the driver to
> > > +The following field, \field{mtu} only exists if VIRTIO_NET_F_MTU is
> > > +set. This field specifies the maximum MTU for the driver to
> > > use.
> > >
> > > The following two fields, \field{speed} and \field{duplex}, only @@
> > > -261,6 +261,8 @@ \subsection{Device configuration
> > > layout}\label{sec:Device Types / Network Device
> > >
> > > \drivernormative{\subsubsection}{Device configuration
> > > layout}{Device Types / Network Device / Device configuration layout}
> > >
> > > +All the device configuration fields are read-only for the driver.
> >
> > Not sure if this makes a good normative clause, I would rather give
> > the driver something actionable:
> >
> > "A driver SHOULD NOT try to write to any of the device configuration
> > fields."
>
> Agree it's not a normative statement as is.
> MUST NOT actually - they were always read only.
> And no need to "try" just don't write period.
>
Saying driver must not write it, doesn't make it read only for the device.
Hence, it should be mentioned as read-only fields, so when the driver writes
something to read-only fields, it can be considered as undefined behavior on
such fields.
> > > +
> > > A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> > > If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver
> > > MUST set the physical address of the NIC to \field{mac}.
> > > Otherwise, it SHOULD
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
On Tue, Feb 21, 2023 at 05:40:51PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Tuesday, February 21, 2023 12:14 PM > > > The part that I am missing is, how do to reuse virtio_net_hash_config and > > > say > > ignore all the existing fields related to rss, but only consider > > hash_tunnel_types? > > > > Like a union? The answer is, don't. Just lay out fields one after another. > > > In that case driver needs to fill up all the fields which are not > related to hash_tunnel_types and the device also needs to compare with > the previous config and ignore it. Doesn’t look like a good use of > existing commands and sw/fw usage for it. Shouldn’t we have the > explicit command for setting tunnel types? I don't know what's proposed at this point, this is too vague. I feel which tunnels to hash for inner header is not different from which transports to hash. If device wants to know what changes it can compare. I expect generally devices will just apply the new config without caring what changed exactly. -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
On Tue, Feb 21, 2023 at 04:37:16PM +0100, Cornelia Huck wrote:
> On Fri, Feb 17 2023, Parav Pandit wrote:
>
> > Device configuration fields are read only. Avoid duplicating this
> > description for multiple fields.
> >
> > Instead describe it one time and do it in the driver requirements
> > section.
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/161
> > Signed-off-by: Parav Pandit
> > ---
> > changelog:
> > v2->v3:
> > - split as new patch
> > ---
> > device-types/net/description.tex | 12 +++-
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > index a197e1a..81e1135 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -156,10 +156,10 @@ \subsubsection{Legacy Interface: Feature
> > bits}\label{sec:Device Types / Network
> > \subsection{Device configuration layout}\label{sec:Device Types / Network
> > Device / Device configuration layout}
> > \label{sec:Device Types / Block Device / Feature bits / Device
> > configuration layout}
> >
> > -Device configuration fields are listed below, they are read-only for a
> > driver. The \field{mac} address field
> > +Device configuration fields are listed below. The \field{mac} address field
>
> I would not remove this here, as I don't think we should move a simple
> statement into the conformance section (see below.) It does makes sense
> to remove the duplicate read-only annotations from the individual
> fields.
>
> > always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
> > \field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
> > -read-only bits (for the driver) are currently defined for the status field:
> > +bits (for the driver) are currently defined for the status field:
>
> What does "bits (for the driver)" mean? It made sense together with
> "read-only", but I would drop "(for the driver)" as well.
Ouch Parav are you making search and replace changes without
reading the result? Pls don't.
> > VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> >
> > \begin{lstlisting}
> > @@ -167,14 +167,14 @@ \subsection{Device configuration
> > layout}\label{sec:Device Types / Network Device
> > #define VIRTIO_NET_S_ANNOUNCE2
> > \end{lstlisting}
> >
> > -The following driver-read-only field, \field{max_virtqueue_pairs} only
> > exists if
> > +The following field, \field{max_virtqueue_pairs} only exists if
> > VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. This field specifies the
> > maximum number
> > of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
> > and transmitq1\ldots transmitqN respectively) that can be configured once
> > at least one of these features
> > is negotiated.
> >
> > -The following driver-read-only field, \field{mtu} only exists if
> > -VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the
> > driver to
> > +The following field, \field{mtu} only exists if VIRTIO_NET_F_MTU
> > +is set. This field specifies the maximum MTU for the driver to
> > use.
> >
> > The following two fields, \field{speed} and \field{duplex}, only
> > @@ -261,6 +261,8 @@ \subsection{Device configuration
> > layout}\label{sec:Device Types / Network Device
> >
> > \drivernormative{\subsubsection}{Device configuration layout}{Device Types
> > / Network Device / Device configuration layout}
> >
> > +All the device configuration fields are read-only for the driver.
>
> Not sure if this makes a good normative clause, I would rather give the
> driver something actionable:
>
> "A driver SHOULD NOT try to write to any of the device configuration
> fields."
Agree it's not a normative statement as is.
MUST NOT actually - they were always read only.
And no need to "try" just don't write period.
> > +
> > A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> > If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver MUST set
> > the physical address of the NIC to \field{mac}. Otherwise, it SHOULD
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
RE: [virtio-dev] RE: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Tuesday, February 21, 2023 12:14 PM > > The part that I am missing is, how do to reuse virtio_net_hash_config and > > say > ignore all the existing fields related to rss, but only consider > hash_tunnel_types? > > Like a union? The answer is, don't. Just lay out fields one after another. > In that case driver needs to fill up all the fields which are not related to hash_tunnel_types and the device also needs to compare with the previous config and ignore it. Doesn’t look like a good use of existing commands and sw/fw usage for it. Shouldn’t we have the explicit command for setting tunnel types?
[virtio-dev] RE: [virtio-comment] RE: [virtio-dev] RE: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
> From: Heng Qi
> Sent: Tuesday, February 21, 2023 12:17 PM
>
> 在 2023/2/22 上午12:50, Parav Pandit 写道:
> >> From: Heng Qi
> >> Sent: Tuesday, February 21, 2023 11:44 AM
> >>> Patch-1 to introduce the feature bit, description, and link to CVQ
> dependency.
> >>> Patch-2 for its link in virtio_net_config structure and description.
> >>> Patch-3 for new command touching control VQ pieces.
> >> Yes, and you seem to have missed my other replies in this thread:), I
> >> rephrased
> > Was it comment "And virtio_net_hash_config seems to suffice except for le16
> reserved[4]." ?
> > I don’t see these reserved fields in the current structure.
> >
> >> it here:
> >> virtio_net_hash_config seems to be reusable, as the v9 patch is
> >> doing, why don't we re-use it?
> >> The reason I can think of is to not expand the virtio_net_hash_config
> >> structure, just set a separate structure for VIRTIO_NET_F_HASH_TUNNEL
> >> and include hash_tunnel_types?
> > The part that I am missing is, how do to reuse virtio_net_hash_config and
> > say
> ignore all the existing fields related to rss, but only consider
> hash_tunnel_types?
>
> Are you referring to such a command?
>
> #define VIRTIO_NET_CTRL_HASH_TUNNEL_TYPE 1 struct
> virtio_net_tunnel_type_config {
> le32 hash_tunnel_types;
> };
>
> Thanks.
Yes.
Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
On Tue, Feb 21, 2023 at 04:50:56PM +, Parav Pandit wrote: > > > From: Heng Qi > > Sent: Tuesday, February 21, 2023 11:44 AM > > > > Patch-1 to introduce the feature bit, description, and link to CVQ > > > dependency. > > > Patch-2 for its link in virtio_net_config structure and description. > > > Patch-3 for new command touching control VQ pieces. > > > > Yes, and you seem to have missed my other replies in this thread:), I > > rephrased > Was it comment "And virtio_net_hash_config seems to suffice except for le16 > reserved[4]." ? > I don’t see these reserved fields in the current structure. > > > it here: > > virtio_net_hash_config seems to be reusable, as the v9 patch is doing, why > > don't we re-use it? > > The reason I can think of is to not expand the virtio_net_hash_config > > structure, > > just set a separate structure for VIRTIO_NET_F_HASH_TUNNEL and include > > hash_tunnel_types? > > The part that I am missing is, how do to reuse virtio_net_hash_config and say > ignore all the existing fields related to rss, but only consider > hash_tunnel_types? Like a union? The answer is, don't. Just lay out fields one after another. -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v9] virtio-net: support inner header hash
On Tue, Feb 21, 2023 at 04:20:59AM +, Parav Pandit wrote:
>
> > From: Heng Qi
> > Sent: Saturday, February 18, 2023 9:37 AM
>
> > If the tunnel is used to encapsulate the packets, the hash calculated using
> > the
> s/hash calculated/hash is calculated
>
> > outer header of the receive packets is always fixed for the same flow
> > packets,
> > i.e. they will be steered to the same receive queue.
> >
> A little descriptive commit message like below reads better to me.
> Currently, when a received packet is an encapsulated packet meaning there is
> an outer and an inner header, virtio device is unable to calculate the hash
> for the inner header.
> Due to this limitation, multiple different flows identified by the inner
> header for the same outer header result in selecting the same receive queue.
> This effectively disables the RSS, resulting in poor receive performance.
>
> Hence, to overcome this limitation, a new feature is introduced using a
> feature bit VIRTIO_NET_F_HASH_TUNNEL.
> This feature enables the device to advertise the capability to calculate the
> hash for the inner packet header.
> Thereby regaining better RSS performance in presence of outer packet header.
I think this is a good description however Parav I think it is important
to make contributors write their own commit messages so they know what
is the reason for the proposed change. What's good for the goose is good
for the gander - contributors should explain why their change to spec is
benefitial but reviewers should also explain why their changes to the
patch are benefitial, and "reads better to me" does not cut it - it does
not allow the contributor to improve with time. It's more than about a
single contribution, see?
In this case I would say the issue is that motivation for the
change is never explained.
I am yet to review the patchset.
>
> > We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related bitmasks in
> > \field{hash_tunnel_types}, which instructs the device to calculate the hash
> > using the inner headers of tunnel-encapsulated packets. Note that
> > VIRTIO_NET_F_HASH_TUNNEL only indicates the ability of the inner header
> > hash, and does not give the device the ability to use the hash value to
> > select a
> > receiving queue to place the packet.
> >
> > Also, a feature bit VIRTIO_NET_F_HASH_REPORT_TUNNEL are added to report
> > an encapsulation type, and the feature depends on
> > VIRTIO_NET_F_HASH_REPORT.
>
> As we discussed that tunnel type alone is not useful the sw, neither as an
> individual field nor merged with some other field.
> Hence, please remove this feature bit. HASH_TUNNEL is good enough.
> Please remove the references to it at more places below.
>
> > It only means that the encapsulation type can be reported, it cannot
> > instruct
> > the device to calculate the hash.
> >
>
> >
> > +\item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash
> > + for tunnel-encapsulated packets.
> > +
> > +\item[VIRTIO_NET_F_HASH_REPORT_TUNNEL(52)] Device can report an
> > encapsulation type.
> > +
> Please remove this.
>
> > \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> > coalescing.
> >
> > \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > @@ -3140,6 +3145,8 @@ \subsubsection{Feature bit
> > requirements}\label{sec:Device Types / Network Device
> > \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> > \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or
> > VIRTIO_NET_F_HOST_TSO6.
> > \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> > +\item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ.
> > +\item[VIRTIO_NET_F_HASH_REPORT_TUNNEL] Requires
> > VIRTIO_NET_F_HASH_REPORT.
> > \end{description}
> >
> > \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types /
> > Network Device / Feature bits / Legacy Interface: Feature bits} @@ -3199,20
> > +3206,27 @@ \subsection{Device configuration layout}\label{sec:Device Types
> > / Network Device
> > u8 rss_max_key_size;
> > le16 rss_max_indirection_table_length;
> > le32 supported_hash_types;
> > +le32 supported_tunnel_hash_types;
> > };
> > \end{lstlisting}
> > -The following field, \field{rss_max_key_size} only exists if
> > VIRTIO_NET_F_RSS
> > or VIRTIO_NET_F_HASH_REPORT is set.
> > +The following field, \field{rss_max_key_size} only exists if
> > VIRTIO_NET_F_RSS,
> > VIRTIO_NET_F_HASH_REPORT or VIRTIO_NET_F_HASH_TUNNEL is set.
> > It specifies the maximum supported length of RSS key in bytes.
> >
> > The following field, \field{rss_max_indirection_table_length} only exists
> > if
> > VIRTIO_NET_F_RSS is set.
> > It specifies the maximum number of 16-bit entries in RSS indirection table.
> >
> > The next field, \field{supported_hash_types} only exists if the device
> > supports
> > hash calculation, -i.e. if VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT is
> > set.
> > +i.e.
RE: [virtio-dev] RE: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
> From: Heng Qi > Sent: Tuesday, February 21, 2023 11:44 AM > > Patch-1 to introduce the feature bit, description, and link to CVQ > > dependency. > > Patch-2 for its link in virtio_net_config structure and description. > > Patch-3 for new command touching control VQ pieces. > > Yes, and you seem to have missed my other replies in this thread:), I > rephrased Was it comment "And virtio_net_hash_config seems to suffice except for le16 reserved[4]." ? I don’t see these reserved fields in the current structure. > it here: > virtio_net_hash_config seems to be reusable, as the v9 patch is doing, why > don't we re-use it? > The reason I can think of is to not expand the virtio_net_hash_config > structure, > just set a separate structure for VIRTIO_NET_F_HASH_TUNNEL and include > hash_tunnel_types? The part that I am missing is, how do to reuse virtio_net_hash_config and say ignore all the existing fields related to rss, but only consider hash_tunnel_types?
[virtio-dev] Re: [PATCH v3 1/2] virtio-net: Describe dev cfg fields read only
On Fri, Feb 17 2023, Parav Pandit wrote:
> Device configuration fields are read only. Avoid duplicating this
> description for multiple fields.
>
> Instead describe it one time and do it in the driver requirements
> section.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/161
> Signed-off-by: Parav Pandit
> ---
> changelog:
> v2->v3:
> - split as new patch
> ---
> device-types/net/description.tex | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/device-types/net/description.tex
> b/device-types/net/description.tex
> index a197e1a..81e1135 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -156,10 +156,10 @@ \subsubsection{Legacy Interface: Feature
> bits}\label{sec:Device Types / Network
> \subsection{Device configuration layout}\label{sec:Device Types / Network
> Device / Device configuration layout}
> \label{sec:Device Types / Block Device / Feature bits / Device configuration
> layout}
>
> -Device configuration fields are listed below, they are read-only for a
> driver. The \field{mac} address field
> +Device configuration fields are listed below. The \field{mac} address field
I would not remove this here, as I don't think we should move a simple
statement into the conformance section (see below.) It does makes sense
to remove the duplicate read-only annotations from the individual
fields.
> always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
> \field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
> -read-only bits (for the driver) are currently defined for the status field:
> +bits (for the driver) are currently defined for the status field:
What does "bits (for the driver)" mean? It made sense together with
"read-only", but I would drop "(for the driver)" as well.
> VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
>
> \begin{lstlisting}
> @@ -167,14 +167,14 @@ \subsection{Device configuration
> layout}\label{sec:Device Types / Network Device
> #define VIRTIO_NET_S_ANNOUNCE2
> \end{lstlisting}
>
> -The following driver-read-only field, \field{max_virtqueue_pairs} only
> exists if
> +The following field, \field{max_virtqueue_pairs} only exists if
> VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set. This field specifies the maximum
> number
> of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
> and transmitq1\ldots transmitqN respectively) that can be configured once at
> least one of these features
> is negotiated.
>
> -The following driver-read-only field, \field{mtu} only exists if
> -VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver
> to
> +The following field, \field{mtu} only exists if VIRTIO_NET_F_MTU
> +is set. This field specifies the maximum MTU for the driver to
> use.
>
> The following two fields, \field{speed} and \field{duplex}, only
> @@ -261,6 +261,8 @@ \subsection{Device configuration layout}\label{sec:Device
> Types / Network Device
>
> \drivernormative{\subsubsection}{Device configuration layout}{Device Types /
> Network Device / Device configuration layout}
>
> +All the device configuration fields are read-only for the driver.
Not sure if this makes a good normative clause, I would rather give the
driver something actionable:
"A driver SHOULD NOT try to write to any of the device configuration
fields."
> +
> A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> If the driver negotiates the VIRTIO_NET_F_MAC feature, the driver MUST set
> the physical address of the NIC to \field{mac}. Otherwise, it SHOULD
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
> From: Heng Qi
> Sent: Tuesday, February 21, 2023 8:34 AM
> > Even without a queue overflow, this shared receive queue may not have a
> balanced number of packets.
> > For example, tunnel-2 occupied 90% of the queue and left only 10% for
> tunnel-1.
> > So, your example is right (and extreme), a generic mention of QoS covers
> both aspects.
> >
> > Secondly "user inside the tunnel" is challenging to explain.
> > In above sentence talks specifically about the "receive queue" as an object.
> >
>
> "overflow" in my example. If we only use a one-sentence summary to describe
> tunnel risks, then I think this subparagraph is called "QoS problem for tunnel
> hash".
>
Yes, Tunnel Qos limitations
> >> struct virtio_net_rss_config {
> le32 hash_types;
> +le32 hash_tunnel_types;
> >>> This field is not needed as device config space advertisement for
> >>> the support
> >> is enough.
> >>
> >> If so, virtio_net_hash_config does not require hash_tunnel_types when
> >> it does not need to configure the specific tunnel(s).
> >>
> >>> If the intent is to enable hashing for the specific tunnel(s), an
> >>> individual
> >> command is better.
> >>
> >> Drivers MAY filter out some tunneling types when negotiating this feature.
> >> Do you think it would be better for us to add a separate command? I
> >> don't see tools like ethtool that can configure specific tunnels in
> >> userspace.
> >>
> > The reason I proposed different command is, Let's say we have only
> > single command.
> > Rss config command has many other fields unrelated to the inner hash.
>
> Yes. For inner hash, fields such as indirection table are irrelevant.
>
Right.
> > Hence, to enable inner hash, the driver needs to supply the exact same value
> for unrelated fields to the same value.
> > And device needs to compare it with the old value and maintain some sort of
> cache to derive that nothing changes from the previous hash config, hence,
> ignore hw configuration for rss.
> >
> > This mechanism slows down the command for the unrelated task.
>
> Totally agree.
>
> > Hence, I am considering a separate command that would be simple for the
> device and driver to implement.
>
> I agree with you. Do you want me to do it in this patch, or should we do it in
> another patch?
>
It should be in this patch set because enablement in the device is linked to
this capability exposed in this patch.
From patch split point of view,
Patch-1 to introduce the feature bit, description, and link to CVQ dependency.
Patch-2 for its link in virtio_net_config structure and description.
Patch-3 for new command touching control VQ pieces.
We can always squash the patch to single when/if it is hard to understand in
multiple patches.
Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
在 2023/2/21 下午8:47, Parav Pandit 写道:
From: virtio-comm...@lists.oasis-open.org On Behalf Of Heng Qi
Also, a feature bit VIRTIO_NET_F_HASH_REPORT_TUNNEL are added to
report an encapsulation type, and the feature depends on
VIRTIO_NET_F_HASH_REPORT.
As we discussed that tunnel type alone is not useful the sw, neither as an
individual field nor merged with some other field.
Hence, please remove this feature bit. HASH_TUNNEL is good enough.
Please remove the references to it at more places below.
If we don't want it at all, I'll remove it and references to it.
Ok. thanks.
[..]
#define VIRTIO_NET_HASH_TYPE_UDP_EX(1 << 8)
\end{lstlisting}
Lets please remove the below encoding.
Here is the encoding of the hash tunnel types, I guess you are referring to
remove the encoding of the hash_report_tunnel types?
Right.
[..]
+RSS to select queues for placement. When a user inside a tunnel
+tries to control the enqueuing of encapsulated packets, then the
+user can flood the device with invaild packets, and the flooded
+packets may be hashed into the same queue as packets in other normal
+tunnels, which causing
the queue to overflow.
Invalid packets are confusing and the wording of "which causing" is not
proper.
There is some duplicate wording below too.
I think above and below risk can be summarized in bit simpler manner.
How about,
When a specific receive queue is shared to receive packets of multiple
tunnels, there is no quality of service for packets of multiple tunnels.
+
I think this sentence can be used as a starting summary, and readers may still
need to expand the explanation.
Do you think the following description is ok?
"
When a specific receive queue is shared to receive encapsulating packets of
multiple tunnels, there is no quality of service for these packets of multiple
tunnels.
For example:
A user inside the tunnel floods a device with packets, then the packets are
hashed into the shared receive queue and cause the queue to overflow, and
this increases packet loss and latency for other tunnels.
"
Even without a queue overflow, this shared receive queue may not have a
balanced number of packets.
For example, tunnel-2 occupied 90% of the queue and left only 10% for tunnel-1.
So, your example is right (and extreme), a generic mention of QoS covers both
aspects.
Secondly "user inside the tunnel" is challenging to explain.
In above sentence talks specifically about the "receive queue" as an object.
struct virtio_net_rss_config {
le32 hash_types;
+le32 hash_tunnel_types;
This field is not needed as device config space advertisement for the support
is enough.
If so, virtio_net_hash_config does not require hash_tunnel_types when it does
not need to configure the specific tunnel(s).
If the intent is to enable hashing for the specific tunnel(s), an individual
command is better.
Drivers MAY filter out some tunneling types when negotiating this feature.
Do you think it would be better for us to add a separate command? I don't see
tools like ethtool that can configure specific tunnels in userspace.
The reason I proposed different command is,
Let's say we have only single command.
Rss config command has many other fields unrelated to the inner hash.
And virtio_net_hash_config seems to suffice except for le16 reserved[4].
Hence, to enable inner hash, the driver needs to supply the exact same value
for unrelated fields to the same value.
And device needs to compare it with the old value and maintain some sort of
cache to derive that nothing changes from the previous hash config, hence,
ignore hw configuration for rss.
This mechanism slows down the command for the unrelated task.
Hence, I am considering a separate command that would be simple for the device
and driver to implement.
Regardless, this new field cannot be in the middle of the new structure as it
breaks backward compatibility.
Yes, you are right. I'll fix this.
Thank you very much!
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
在 2023/2/21 下午8:47, Parav Pandit 写道:
From: virtio-comm...@lists.oasis-open.org On Behalf Of Heng Qi
Also, a feature bit VIRTIO_NET_F_HASH_REPORT_TUNNEL are added to
report an encapsulation type, and the feature depends on
VIRTIO_NET_F_HASH_REPORT.
As we discussed that tunnel type alone is not useful the sw, neither as an
individual field nor merged with some other field.
Hence, please remove this feature bit. HASH_TUNNEL is good enough.
Please remove the references to it at more places below.
If we don't want it at all, I'll remove it and references to it.
Ok. thanks.
[..]
#define VIRTIO_NET_HASH_TYPE_UDP_EX(1 << 8)
\end{lstlisting}
Lets please remove the below encoding.
Here is the encoding of the hash tunnel types, I guess you are referring to
remove the encoding of the hash_report_tunnel types?
Right.
[..]
+RSS to select queues for placement. When a user inside a tunnel
+tries to control the enqueuing of encapsulated packets, then the
+user can flood the device with invaild packets, and the flooded
+packets may be hashed into the same queue as packets in other normal
+tunnels, which causing
the queue to overflow.
Invalid packets are confusing and the wording of "which causing" is not
proper.
There is some duplicate wording below too.
I think above and below risk can be summarized in bit simpler manner.
How about,
When a specific receive queue is shared to receive packets of multiple
tunnels, there is no quality of service for packets of multiple tunnels.
+
I think this sentence can be used as a starting summary, and readers may still
need to expand the explanation.
Do you think the following description is ok?
"
When a specific receive queue is shared to receive encapsulating packets of
multiple tunnels, there is no quality of service for these packets of multiple
tunnels.
For example:
A user inside the tunnel floods a device with packets, then the packets are
hashed into the shared receive queue and cause the queue to overflow, and
this increases packet loss and latency for other tunnels.
"
Even without a queue overflow, this shared receive queue may not have a
balanced number of packets.
For example, tunnel-2 occupied 90% of the queue and left only 10% for tunnel-1.
So, your example is right (and extreme), a generic mention of QoS covers both
aspects.
Secondly "user inside the tunnel" is challenging to explain.
In above sentence talks specifically about the "receive queue" as an object.
"overflow" in my example. If we only use a one-sentence summary to
describe tunnel risks, then I think this subparagraph is called "QoS
problem for tunnel hash".
struct virtio_net_rss_config {
le32 hash_types;
+le32 hash_tunnel_types;
This field is not needed as device config space advertisement for the support
is enough.
If so, virtio_net_hash_config does not require hash_tunnel_types when it does
not need to configure the specific tunnel(s).
If the intent is to enable hashing for the specific tunnel(s), an individual
command is better.
Drivers MAY filter out some tunneling types when negotiating this feature.
Do you think it would be better for us to add a separate command? I don't see
tools like ethtool that can configure specific tunnels in userspace.
The reason I proposed different command is,
Let's say we have only single command.
Rss config command has many other fields unrelated to the inner hash.
Yes. For inner hash, fields such as indirection table are irrelevant.
Hence, to enable inner hash, the driver needs to supply the exact same value
for unrelated fields to the same value.
And device needs to compare it with the old value and maintain some sort of
cache to derive that nothing changes from the previous hash config, hence,
ignore hw configuration for rss.
This mechanism slows down the command for the unrelated task.
Totally agree.
Hence, I am considering a separate command that would be simple for the device
and driver to implement.
I agree with you. Do you want me to do it in this patch, or should we do
it in another patch?
Thanks! :)
Regardless, this new field cannot be in the middle of the new structure as it
breaks backward compatibility.
Yes, you are right. I'll fix this.
Thank you very much!
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [PATCH v3 0/2] virtio-net: Improve dev config layout
> From: Parav Pandit > Sent: Friday, February 17, 2023 10:45 AM > Patch summary: > patch-1: consolidate read only field at one place in driver requirements > patch-2: define device configuration layout before describing its fields. > > changelog: > v2->v3: > - split into two patches > - move read only description to driver requirements section These 2 patches are reviewed. Can you please open a ballot for it? Github issue: https://lists.oasis-open.org/archives/virtio-dev/202302/msg00405.html And the github description has the latest link this time. :) - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] RE: [PATCH v9] virtio-net: support inner header hash
> From: virtio-comm...@lists.oasis-open.org open.org> On Behalf Of Heng Qi
> >> Also, a feature bit VIRTIO_NET_F_HASH_REPORT_TUNNEL are added to
> >> report an encapsulation type, and the feature depends on
> >> VIRTIO_NET_F_HASH_REPORT.
> > As we discussed that tunnel type alone is not useful the sw, neither as an
> individual field nor merged with some other field.
> > Hence, please remove this feature bit. HASH_TUNNEL is good enough.
> > Please remove the references to it at more places below.
>
> If we don't want it at all, I'll remove it and references to it.
>
Ok. thanks.
[..]
> >> #define VIRTIO_NET_HASH_TYPE_UDP_EX(1 << 8)
> >> \end{lstlisting}
> >>
> > Lets please remove the below encoding.
>
> Here is the encoding of the hash tunnel types, I guess you are referring to
> remove the encoding of the hash_report_tunnel types?
>
Right.
[..]
> >> +RSS to select queues for placement. When a user inside a tunnel
> >> +tries to control the enqueuing of encapsulated packets, then the
> >> +user can flood the device with invaild packets, and the flooded
> >> +packets may be hashed into the same queue as packets in other normal
> >> +tunnels, which causing
> >> the queue to overflow.
> >>
> > Invalid packets are confusing and the wording of "which causing" is not
> proper.
> > There is some duplicate wording below too.
> >
> > I think above and below risk can be summarized in bit simpler manner.
> >
> > How about,
> >
> > When a specific receive queue is shared to receive packets of multiple
> tunnels, there is no quality of service for packets of multiple tunnels.
> >
> > +
>
> I think this sentence can be used as a starting summary, and readers may still
> need to expand the explanation.
> Do you think the following description is ok?
> "
> When a specific receive queue is shared to receive encapsulating packets of
> multiple tunnels, there is no quality of service for these packets of multiple
> tunnels.
> For example:
> A user inside the tunnel floods a device with packets, then the packets are
> hashed into the shared receive queue and cause the queue to overflow, and
> this increases packet loss and latency for other tunnels.
> "
Even without a queue overflow, this shared receive queue may not have a
balanced number of packets.
For example, tunnel-2 occupied 90% of the queue and left only 10% for tunnel-1.
So, your example is right (and extreme), a generic mention of QoS covers both
aspects.
Secondly "user inside the tunnel" is challenging to explain.
In above sentence talks specifically about the "receive queue" as an object.
> struct virtio_net_rss_config {
> >> le32 hash_types;
> >> +le32 hash_tunnel_types;
> > This field is not needed as device config space advertisement for the
> > support
> is enough.
>
> If so, virtio_net_hash_config does not require hash_tunnel_types when it does
> not need to configure the specific tunnel(s).
>
> >
> > If the intent is to enable hashing for the specific tunnel(s), an individual
> command is better.
>
> Drivers MAY filter out some tunneling types when negotiating this feature.
> Do you think it would be better for us to add a separate command? I don't see
> tools like ethtool that can configure specific tunnels in userspace.
>
The reason I proposed different command is,
Let's say we have only single command.
Rss config command has many other fields unrelated to the inner hash.
Hence, to enable inner hash, the driver needs to supply the exact same value
for unrelated fields to the same value.
And device needs to compare it with the old value and maintain some sort of
cache to derive that nothing changes from the previous hash config, hence,
ignore hw configuration for rss.
This mechanism slows down the command for the unrelated task.
Hence, I am considering a separate command that would be simple for the device
and driver to implement.
> >
> > Regardless, this new field cannot be in the middle of the new structure as
> > it
> breaks backward compatibility.
> >
>
> Yes, you are right. I'll fix this.
>
> Thank you very much!
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v5] virtio-net: support the virtqueue coalescing moderation
在 2023/2/21 下午7:48, David Edmondson 写道:
On Tuesday, 2023-02-21 at 16:38:52 +08, Heng Qi wrote:
...
+A device MAY set the coalescing parameter to a value close to a power of 2
value.
What is this about?
If it is intended to indicate that a device may use a value different to
that passed by the driver, more text to describe that would be
Yes. How about adding the following explanation to the "Operation"
subparagraph?
"
When a device receives the VIRTIO_NET_CTRL_NOTF_COAL commands to set an
coalescing parameter, it may set the parameter to a value close to a
power of 2. For example:
If the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command with \field{max_usecs} =
7 is received, the device may set \field{max_usecs} = 8 for a given
enabled virtqueue.
"
Thanks.
useful. If not, what does it mean?
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [PATCH v5] virtio-net: support the virtqueue coalescing moderation
On Tuesday, 2023-02-21 at 16:38:52 +08, Heng Qi wrote: > ... > +A device MAY set the coalescing parameter to a value close to a power of 2 > value. What is this about? If it is intended to indicate that a device may use a value different to that passed by the driver, more text to describe that would be useful. If not, what does it mean? -- Oh there ain't no way to say I love you more. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] [RFC QEMU] docs: vhost-user: Add custom memory mapping support
The current model of memory mapping at the back-end works fine with Qemu, where a standard call to mmap() for the respective file descriptor, passed from front-end, is generally all we need to do before the front-end can start accessing the guest memory. There are other complex cases though, where we need more information at the backend and need to do more than just an mmap() call. For example, Xen, a type-1 hypervisor, currently supports memory mapping via two different methods, foreign-mapping (via /dev/privcmd) and grant-dev (via /dev/gntdev). In both these cases, the back-end needs to call mmap() followed by an ioctl() (or vice-versa), and need to pass extra information via the ioctl(), like the Xen domain-id of the guest whose memory we are trying to map. Add a new protocol feature, 'VHOST_USER_PROTOCOL_F_CUSTOM_MMAP', which lets the back-end know about the additional memory mapping requirements. When this feature is negotiated, the front-end can send the 'VHOST_USER_CUSTOM_MMAP' message type to provide the additional information to the back-end. Signed-off-by: Viresh Kumar --- docs/interop/vhost-user.rst | 32 1 file changed, 32 insertions(+) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 3f18ab424eb0..f2b1d705593a 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -258,6 +258,23 @@ Inflight description :queue size: a 16-bit size of virtqueues +Custom mmap description +^^^ + ++---+---+ +| flags | value | ++---+---+ + +:flags: 64-bit bit field + +- Bit 0 is Xen foreign memory access flag - needs Xen foreign memory mapping. +- Bit 1 is Xen grant memory access flag - needs Xen grant memory mapping. + +:value: a 64-bit hypervisor specific value. + +- For Xen foreign or grant memory access, this is set with guest's xen domain + id. + C structure --- @@ -867,6 +884,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS 14 #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15 #define VHOST_USER_PROTOCOL_F_STATUS 16 + #define VHOST_USER_PROTOCOL_F_CUSTOM_MMAP 17 Front-end message types --- @@ -1422,6 +1440,20 @@ Front-end message types query the back-end for its device status as defined in the Virtio specification. +``VHOST_USER_CUSTOM_MMAP`` + :id: 41 + :equivalent ioctl: N/A + :request payload: Custom mmap description + :reply payload: N/A + + When the ``VHOST_USER_PROTOCOL_F_CUSTOM_MMAP`` protocol feature has been + successfully negotiated, this message is submitted by the front-end to + notify the back-end of the special memory mapping requirements, that the + back-end needs to take care of, while mapping any memory regions sent + over by the front-end. The front-end must send this message before + any memory-regions are sent to the back-end via ``VHOST_USER_SET_MEM_TABLE`` + or ``VHOST_USER_ADD_MEM_REG`` message types. + Back-end message types -- -- 2.31.1.272.g89b43f80a514 - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
