RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Heng Qi > Sent: Thursday, February 9, 2023 12:21 AM > Thanks for your reply. > >>> I had one last question. Why do we need to inform the > >> hash_report_tunnel_type of the outer header in the virtio_net_hdr? > >>> Is this for debug? Or is there a use case that will process this value? > >> The driver may use it to do some statistical information, or do some > >> rx classification based on the rx hash, and we'd better not hide > >> information from the driver. > >> > > Statistical information is better gathered via stats, instead of adding > > such code > in driver data path. > > A cautionary note : the source of hash_report_tunnel comes from the > discussion here > https://lists.oasis-open.org/archives/virtio-dev/202211/msg00063.html > If I understand correctly, if virtio-net-hdr also has hash_report_tunnel, how > does the driver do statistics? I am asking to avoid having hash_report_tunnel in virtio_net_hdr if it is only for the statistics purpose. If it has more use than just statistics, I like to understand its use in rx packet processing flow, if there is any and to describe in commit log too. Below [1] says about driver doing some rx classification. It is unclear on inner or outer. The response is not very clear to me, about how this field is useful. [1] https://lists.oasis-open.org/archives/virtio-dev/202211/msg00063.html If driver needs to know the statistics of it, it can query them from the device by introducing statistics cmd. - 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: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 9:09 AM > > > header: it allow users inside the tunnel control queueing outside. > > > By observing packet loss some information leaks between tunnels. > > > > > I likely didn't understand. Can you please explain? > > > > Queuing is always done on the inner header with/without encapsulation. > > Hash is always reported for inner header. > > It is only adding the ability to hash even when outer header exists. > > > If hashing just on outer header (currently the only option) then a given > tunnel > all lands in a given queue. > Just keep that queue separate and users of this tunnel can not learn whether > other queues are overflowing, and can not overflow other queues. > > > If you hash inner header then user can flood device with packets of a given > connection and the same connection in a different tunnel hashes to the same > queue. Now one tunnel can > - cause DoS for another tunnel > - cause packet loss or latency triggering possible security bugs within guest > - detect that another tunnel is using the connection by > detecting its own packet loss or increased latency > Yes. It can lead to above issues. Steering on inner is on best effort based sw implementations running on top of net device. To avoid above issues, a hierarchical model is needed. I am not aware of any. To my knowledge, usually who care for above issues end up using a different net device for each VNI and achieve the desired hierarchy. > > > If queuing to be decided based on outer header (hash), then that is > > different. > > Hashing both inner and outer in a flat q structure unlikely works, right? > > Because both hashes can result in different q selection. > > > That's the point. > > Is there any precedent in OSes for configuring things like this that we can > look > at? > ethtool -N (not yet part of virtio) is the closest match that can steer based on inner and outer both, but it is not hierarchical, and it is orthogonal to this feature. > > > > > > > Ideas for solving this they all involve hashing both inner and outer > > > header: > > > 1- report two sets of hashes. overkill? > > > 2- hash both headers together > > > 2- add salt. can come from driver or device itself > > > > > > More ideas? > > > > > > -- > > > 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: [virtio-comment] RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Feb 08, 2023 at 02:05:52PM +, Parav Pandit wrote: > > From: Michael S. Tsirkin > > Sent: Wednesday, February 8, 2023 8:52 AM > > > > On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > > > > > From: Michael S. Tsirkin > > > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > > > From: Heng Qi > > > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > > > > > [..] > > > > > > >> > > > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > > > In struct virtio_net_config we need two fields. > > > > > > > a. supported_hash_types (already exists) b. > > > > > > > supported_hash_tunnel_type > > > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > > > -> calculation is > > > > > > supported. > > > > > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > > > a. hash_report (already exists) b. hash_tunnel_type 8 bits -> > > > > > > > absolute value indicating which outer header > > > > > > exists when inner header hash calculated. > > > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > > > clearer that its > > > > > > type. > > > > > > > > > > > > Sure. > > > > > > > > > > > > Thanks for your reply. > > > > > > > > > > I had one last question. Why do we need to inform the > > > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > > > Is this for debug? Or is there a use case that will process this > > > > > value? > > > > > > > > Well we have hash_report which is kind of similar (and also kind of > > > > pointless but I think it's there because WHQL wants it). > > > Hash_report is useful. It tells hash_value is in which namespace > > > (ipv4-tcp/ipv4 > > udp etc). > > > OS can use this value to find tcp connection in a given namespace. > > > > > > > Maybe we can steal some bits > > > > from there instead of a new field? > > > > > > > I do not have problem adding extra bits. I just don't find that just > > > telling that > > its vxlan or nvgre to the OS is useful. > > > If OS needs to know about outer header details, it needs to know the VNI > > information than just telling vxlan. > > > > This does make sense. > > > > > > > > > > > > I have a follow up question though: are we only hashing the inner > > > > header or both inner and outer header? Somewhat confused on this. > > > > > > > I understood as inner header. But worth to describe it. May be there. > > > Need to > > read v8 patch. > > > > Hmm. I just realized that there's a security problem with hashing just the > > inner > > header: it allow users inside the tunnel control queueing outside. > > By observing packet loss some information leaks between tunnels. > > > Ah I know now. > We are leaking outer header information inside the virtio net hdr, and outer > header might be already stripped off by a different entity. > > I think the use case here is it's the same sw entity that owns the virtio net > device does the encap/decap too. No not exactly, we are leaking info between encap tunnels. -- 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: [virtio-comment] RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Feb 08, 2023 at 02:00:14PM +, Parav Pandit wrote: > > From: Michael S. Tsirkin > > Sent: Wednesday, February 8, 2023 8:52 AM > > > > On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > > > > > From: Michael S. Tsirkin > > > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > > > From: Heng Qi > > > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > > > > > [..] > > > > > > >> > > > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > > > In struct virtio_net_config we need two fields. > > > > > > > a. supported_hash_types (already exists) b. > > > > > > > supported_hash_tunnel_type > > > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > > > -> calculation is > > > > > > supported. > > > > > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > > > a. hash_report (already exists) b. hash_tunnel_type 8 bits -> > > > > > > > absolute value indicating which outer header > > > > > > exists when inner header hash calculated. > > > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > > > clearer that its > > > > > > type. > > > > > > > > > > > > Sure. > > > > > > > > > > > > Thanks for your reply. > > > > > > > > > > I had one last question. Why do we need to inform the > > > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > > > Is this for debug? Or is there a use case that will process this > > > > > value? > > > > > > > > Well we have hash_report which is kind of similar (and also kind of > > > > pointless but I think it's there because WHQL wants it). > > > Hash_report is useful. It tells hash_value is in which namespace > > > (ipv4-tcp/ipv4 > > udp etc). > > > OS can use this value to find tcp connection in a given namespace. > > > > > > > Maybe we can steal some bits > > > > from there instead of a new field? > > > > > > > I do not have problem adding extra bits. I just don't find that just > > > telling that > > its vxlan or nvgre to the OS is useful. > > > If OS needs to know about outer header details, it needs to know the VNI > > information than just telling vxlan. > > > > This does make sense. > > > > > > > > > > > > I have a follow up question though: are we only hashing the inner > > > > header or both inner and outer header? Somewhat confused on this. > > > > > > > I understood as inner header. But worth to describe it. May be there. > > > Need to > > read v8 patch. > > > > Hmm. I just realized that there's a security problem with hashing just the > > inner > > header: it allow users inside the tunnel control queueing outside. > > By observing packet loss some information leaks between tunnels. > > > I likely didn't understand. Can you please explain? > > Queuing is always done on the inner header with/without encapsulation. > Hash is always reported for inner header. > It is only adding the ability to hash even when outer header exists. If hashing just on outer header (currently the only option) then a given tunnel all lands in a given queue. Just keep that queue separate and users of this tunnel can not learn whether other queues are overflowing, and can not overflow other queues. If you hash inner header then user can flood device with packets of a given connection and the same connection in a different tunnel hashes to the same queue. Now one tunnel can - cause DoS for another tunnel - cause packet loss or latency triggering possible security bugs within guest - detect that another tunnel is using the connection by detecting its own packet loss or increased latency > If queuing to be decided based on outer header (hash), then that is different. > Hashing both inner and outer in a flat q structure unlikely works, right? > Because both hashes can result in different q selection. That's the point. Is there any precedent in OSes for configuring things like this that we can look at? > > > > Ideas for solving this they all involve hashing both inner and outer > > header: > > 1- report two sets of hashes. overkill? > > 2- hash both headers together > > 2- add salt. can come from driver or device itself > > > > More ideas? > > > > -- > > 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: [virtio-comment] RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 8:52 AM > > On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > > > From: Michael S. Tsirkin > > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > > From: Heng Qi > > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > > > [..] > > > > > >> > > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > > In struct virtio_net_config we need two fields. > > > > > > a. supported_hash_types (already exists) b. > > > > > > supported_hash_tunnel_type > > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > > -> calculation is > > > > > supported. > > > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > > a. hash_report (already exists) b. hash_tunnel_type 8 bits -> > > > > > > absolute value indicating which outer header > > > > > exists when inner header hash calculated. > > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > > clearer that its > > > > > type. > > > > > > > > > > Sure. > > > > > > > > > > Thanks for your reply. > > > > > > > > I had one last question. Why do we need to inform the > > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > > Is this for debug? Or is there a use case that will process this value? > > > > > > Well we have hash_report which is kind of similar (and also kind of > > > pointless but I think it's there because WHQL wants it). > > Hash_report is useful. It tells hash_value is in which namespace > > (ipv4-tcp/ipv4 > udp etc). > > OS can use this value to find tcp connection in a given namespace. > > > > > Maybe we can steal some bits > > > from there instead of a new field? > > > > > I do not have problem adding extra bits. I just don't find that just > > telling that > its vxlan or nvgre to the OS is useful. > > If OS needs to know about outer header details, it needs to know the VNI > information than just telling vxlan. > > This does make sense. > > > > > > > > I have a follow up question though: are we only hashing the inner > > > header or both inner and outer header? Somewhat confused on this. > > > > > I understood as inner header. But worth to describe it. May be there. Need > > to > read v8 patch. > > Hmm. I just realized that there's a security problem with hashing just the > inner > header: it allow users inside the tunnel control queueing outside. > By observing packet loss some information leaks between tunnels. > Ah I know now. We are leaking outer header information inside the virtio net hdr, and outer header might be already stripped off by a different entity. I think the use case here is it's the same sw entity that owns the virtio net device does the encap/decap 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: [virtio-comment] RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 8:52 AM > > On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > > > From: Michael S. Tsirkin > > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > > From: Heng Qi > > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > > > [..] > > > > > >> > > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > > In struct virtio_net_config we need two fields. > > > > > > a. supported_hash_types (already exists) b. > > > > > > supported_hash_tunnel_type > > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > > -> calculation is > > > > > supported. > > > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > > a. hash_report (already exists) b. hash_tunnel_type 8 bits -> > > > > > > absolute value indicating which outer header > > > > > exists when inner header hash calculated. > > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > > clearer that its > > > > > type. > > > > > > > > > > Sure. > > > > > > > > > > Thanks for your reply. > > > > > > > > I had one last question. Why do we need to inform the > > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > > Is this for debug? Or is there a use case that will process this value? > > > > > > Well we have hash_report which is kind of similar (and also kind of > > > pointless but I think it's there because WHQL wants it). > > Hash_report is useful. It tells hash_value is in which namespace > > (ipv4-tcp/ipv4 > udp etc). > > OS can use this value to find tcp connection in a given namespace. > > > > > Maybe we can steal some bits > > > from there instead of a new field? > > > > > I do not have problem adding extra bits. I just don't find that just > > telling that > its vxlan or nvgre to the OS is useful. > > If OS needs to know about outer header details, it needs to know the VNI > information than just telling vxlan. > > This does make sense. > > > > > > > > I have a follow up question though: are we only hashing the inner > > > header or both inner and outer header? Somewhat confused on this. > > > > > I understood as inner header. But worth to describe it. May be there. Need > > to > read v8 patch. > > Hmm. I just realized that there's a security problem with hashing just the > inner > header: it allow users inside the tunnel control queueing outside. > By observing packet loss some information leaks between tunnels. > I likely didn't understand. Can you please explain? Queuing is always done on the inner header with/without encapsulation. Hash is always reported for inner header. It is only adding the ability to hash even when outer header exists. If queuing to be decided based on outer header (hash), then that is different. Hashing both inner and outer in a flat q structure unlikely works, right? Because both hashes can result in different q selection. > > Ideas for solving this they all involve hashing both inner and outer > header: > 1- report two sets of hashes. overkill? > 2- hash both headers together > 2- add salt. can come from driver or device itself > > More ideas? > > -- > 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: [virtio-comment] RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Feb 08, 2023 at 01:38:36PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Wednesday, February 8, 2023 8:32 AM > > > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > > From: Heng Qi > > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > > > [..] > > > > >> > > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > > In struct virtio_net_config we need two fields. > > > > > a. supported_hash_types (already exists) b. > > > > > supported_hash_tunnel_type > > > > > -> bitmap indicating for which outer headers, inner hash > > > > > -> calculation is > > > > supported. > > > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > > a. hash_report (already exists) > > > > > b. hash_tunnel_type 8 bits -> absolute value indicating which > > > > > outer header > > > > exists when inner header hash calculated. > > > > > You already have it in your patch named as hash_report_tunnel. > > > > > May be better to name as hash_report_tunnel_type to make it > > > > > clearer that its > > > > type. > > > > > > > > Sure. > > > > > > > > Thanks for your reply. > > > > > > I had one last question. Why do we need to inform the > > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > > Is this for debug? Or is there a use case that will process this value? > > > > Well we have hash_report which is kind of similar (and also kind of > > pointless > > but I think it's there because WHQL wants it). > Hash_report is useful. It tells hash_value is in which namespace > (ipv4-tcp/ipv4 udp etc). > OS can use this value to find tcp connection in a given namespace. > > > Maybe we can steal some bits > > from there instead of a new field? > > > I do not have problem adding extra bits. I just don't find that just telling > that its vxlan or nvgre to the OS is useful. > If OS needs to know about outer header details, it needs to know the VNI > information than just telling vxlan. This does make sense. > > > > I have a follow up question though: are we only hashing the inner header or > > both inner and outer header? Somewhat confused on this. > > > I understood as inner header. But worth to describe it. May be there. Need to > read v8 patch. Hmm. I just realized that there's a security problem with hashing just the inner header: it allow users inside the tunnel control queueing outside. By observing packet loss some information leaks between tunnels. Ideas for solving this they all involve hashing both inner and outer header: 1- report two sets of hashes. overkill? 2- hash both headers together 2- add salt. can come from driver or device itself More ideas? -- 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: [virtio-comment] RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, February 8, 2023 8:32 AM > > On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > > From: Heng Qi > > > Sent: Tuesday, February 7, 2023 10:25 PM > > > > [..] > > > >> > > > >> Do you think we need both hash_types and hash_tunnel_types? > > > > In struct virtio_net_config we need two fields. > > > > a. supported_hash_types (already exists) b. > > > > supported_hash_tunnel_type > > > > -> bitmap indicating for which outer headers, inner hash > > > > -> calculation is > > > supported. > > > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > > > > In struct virtio_net_hdr we need two fields. > > > > a. hash_report (already exists) > > > > b. hash_tunnel_type 8 bits -> absolute value indicating which > > > > outer header > > > exists when inner header hash calculated. > > > > You already have it in your patch named as hash_report_tunnel. > > > > May be better to name as hash_report_tunnel_type to make it > > > > clearer that its > > > type. > > > > > > Sure. > > > > > > Thanks for your reply. > > > > I had one last question. Why do we need to inform the > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > Is this for debug? Or is there a use case that will process this value? > > Well we have hash_report which is kind of similar (and also kind of pointless > but I think it's there because WHQL wants it). Hash_report is useful. It tells hash_value is in which namespace (ipv4-tcp/ipv4 udp etc). OS can use this value to find tcp connection in a given namespace. > Maybe we can steal some bits > from there instead of a new field? > I do not have problem adding extra bits. I just don't find that just telling that its vxlan or nvgre to the OS is useful. If OS needs to know about outer header details, it needs to know the VNI information than just telling vxlan. > > I have a follow up question though: are we only hashing the inner header or > both inner and outer header? Somewhat confused on this. > I understood as inner header. But worth to describe it. May be there. Need to read v8 patch. - 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: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Feb 08, 2023 at 05:18:32AM +, Parav Pandit wrote: > > From: Heng Qi > > Sent: Tuesday, February 7, 2023 10:25 PM > > [..] > > >> > > >> Do you think we need both hash_types and hash_tunnel_types? > > > In struct virtio_net_config we need two fields. > > > a. supported_hash_types (already exists) b. supported_hash_tunnel_type > > > -> bitmap indicating for which outer headers, inner hash calculation is > > supported. > > > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > > > > In struct virtio_net_hdr we need two fields. > > > a. hash_report (already exists) > > > b. hash_tunnel_type 8 bits -> absolute value indicating which outer header > > exists when inner header hash calculated. > > > You already have it in your patch named as hash_report_tunnel. > > > May be better to name as hash_report_tunnel_type to make it clearer that > > > its > > type. > > > > Sure. > > > > Thanks for your reply. > > I had one last question. Why do we need to inform the hash_report_tunnel_type > of the outer header in the virtio_net_hdr? > Is this for debug? Or is there a use case that will process this value? Well we have hash_report which is kind of similar (and also kind of pointless but I think it's there because WHQL wants it). Maybe we can steal some bits from there instead of a new field? I have a follow up question though: are we only hashing the inner header or both inner and outer header? Somewhat confused on this. In fact, CC Yuri for thoughts and suggestions from windows side of things. -- MST - 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 v7] virtio-net: support inner header hash
> From: Heng Qi > Sent: Wednesday, February 8, 2023 1:11 AM > > > 在 2023/2/8 下午1:18, Parav Pandit 写道: > >> From: Heng Qi > >> Sent: Tuesday, February 7, 2023 10:25 PM > > [..] > Do you think we need both hash_types and hash_tunnel_types? > >>> In struct virtio_net_config we need two fields. > >>> a. supported_hash_types (already exists) b. > >>> supported_hash_tunnel_type > >>> -> bitmap indicating for which outer headers, inner hash calculation > >>> -> is > >> supported. > >> > >> Thanks for the suggestion, we seem to have reached an agreement. > >> > >>> In struct virtio_net_hdr we need two fields. > >>> a. hash_report (already exists) > >>> b. hash_tunnel_type 8 bits -> absolute value indicating which outer > >>> header > >> exists when inner header hash calculated. > >>> You already have it in your patch named as hash_report_tunnel. > >>> May be better to name as hash_report_tunnel_type to make it clearer > >>> that its > >> type. > >> > >> Sure. > >> > >> Thanks for your reply. > > I had one last question. Why do we need to inform the > hash_report_tunnel_type of the outer header in the virtio_net_hdr? > > Is this for debug? Or is there a use case that will process this value? > > The driver may use it to do some statistical information, or do some rx > classification based on the rx hash, and we'd better not hide information from > the driver. > Statistical information is better gathered via stats, instead of adding such code in driver data path. It is not about hiding. It has onus on the data path to detect the tunnel type and place that virtio_net_hdr. So lets find one use of it in data path which sw entity will use it.
RE: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
> From: Heng Qi > Sent: Tuesday, February 7, 2023 10:25 PM [..] > >> > >> Do you think we need both hash_types and hash_tunnel_types? > > In struct virtio_net_config we need two fields. > > a. supported_hash_types (already exists) b. supported_hash_tunnel_type > > -> bitmap indicating for which outer headers, inner hash calculation is > supported. > > Thanks for the suggestion, we seem to have reached an agreement. > > > > > In struct virtio_net_hdr we need two fields. > > a. hash_report (already exists) > > b. hash_tunnel_type 8 bits -> absolute value indicating which outer header > exists when inner header hash calculated. > > You already have it in your patch named as hash_report_tunnel. > > May be better to name as hash_report_tunnel_type to make it clearer that its > type. > > Sure. > > Thanks for your reply. I had one last question. Why do we need to inform the hash_report_tunnel_type of the outer header in the virtio_net_hdr? Is this for debug? Or is there a use case that will process this value? - 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 v7] virtio-net: support inner header hash
> From: Heng Qi
> Sent: Tuesday, February 7, 2023 9:31 PM
>
> 在 2023/1/31 下午1:28, Heng Qi 写道:
> > On Mon, Jan 16, 2023 at 04:42:11PM +0800, Jason Wang wrote:
> >> 在 2023/1/16 16:01, Heng Qi 写道:
> >>> On Wed, Jan 11, 2023 at 04:45:12AM -0500, Michael S. Tsirkin wrote:
> On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> > If the tunnel is used to encapsulate the packets, the hash
> > calculated using the 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.
> >
> > We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related
> bitmasks
> > in \field{hash_types}, which instructs the device to calculate the
> > hash using the inner headers of tunnel-encapsulated packets.
> > Besides, values in \field{hash_report_tunnel} are added to report tunnel
> types.
> >
> > Fixes:
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > github.com%2Foasis-tcs%2Fvirtio-
> spec%2Fissues%2F151=05%7C01%7
> >
> Cparav%40nvidia.com%7C12be6c51b3c04860c01608db097c796a%7C43083d15
> 7
> >
> 27340c1b7db39efd9ccc17a%7C0%7C0%7C638114202991221369%7CUnknown
> %7CT
> >
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ
> >
> XVCI6Mn0%3D%7C3000%7C%7C%7C=6feFgy94LMCYfG0NKPlHCr1AvkC
> OH4v9
> > %2BnLomXklNqs%3D=0
> >
> > Reviewed-by: Jason Wang
> > Signed-off-by: Heng Qi
> > Signed-off-by: Xuan Zhuo
> > ---
> > v6:
> > 1. Modify the wording of some sentences for clarity. @Michael S.
> Tsirkin
> > 2. Fix some syntax issues. @Michael S. Tsirkin
> >
> > v5:
> > 1. Fix some syntax and capitalization issues. @Michael S.
> > Tsirkin
> > 2. Use encapsulated/encaptulation uniformly. @Michael S. Tsirkin
> > 3. Move the links to introduction section. @Michael S. Tsirkin
> > 4. Clarify some sentences. @Michael S. Tsirkin
> >
> > v4:
> > 1. Clarify some paragraphs. @Cornelia Huck
> > 2. Fix the u8 type. @Cornelia Huck
> >
> > v3:
> > 1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to
> VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
> > 2. Make things clearer. @Jason Wang @Michael S. Tsirkin
> > 3. Keep the possibility to use inner hash for automatic receive
> > steering.
> @Jason Wang
> > 4. Add the "Tunnel packet" paragraph to avoid repeating the GRE
> > etc. many times. @Michael S. Tsirkin
> >
> > v2:
> > 1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason
> > Wang
> > 2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}.
> > @Jason Wang, @Michael S. Tsirkin
> >
> > v1:
> > 1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
> > 2. Clarify some paragraphs. @Jason Wang
> > 3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE.
> @Yuri
> > Benditovich
> >
> > content.tex | 191
> +--
> > introduction.tex | 19 +
> > 2 files changed, 203 insertions(+), 7 deletions(-)
> >
> > diff --git a/content.tex b/content.tex index e863709..7845f6c
> > 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device
> Types / Network Device / Feature bits
> > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through
> control
> > channel.
> > +\item[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner
> > +header hash for GRE, VXLAN and GENEVE tunnel-encapsulated
> packets.
> > +
> > \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> coalescing.
> > \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4
> packets.
> > @@ -3095,7 +3098,8 @@ \subsection{Feature bits}\label{sec:Device
> Types / Network Device / Feature bits
> >to several segments when each of these smaller packets has UDP
> header.
> > \item[VIRTIO_NET_F_HASH_REPORT(57)] Device can report per-packet
> hash
> > -value and a type of calculated hash.
> > +value, a type of calculated hash, and, if
> VIRTIO_NET_F_HASH_TUNNEL
> > +is negotiated, an encapsulation packet type.
> > \item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the exact
> \field{hdr_len}
> > value. Device benefits from knowing the exact header length.
> > @@ -3140,6 +3144,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_HASH_TUNNEL] Requires
>
[virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Jan 11, 2023 at 12:45:06PM +0800, Jason Wang wrote:
> On Wed, Jan 11, 2023 at 11:23 AM Heng Qi wrote:
> >
> >
> >
> > 在 2023/1/10 下午3:26, Heng Qi 写道:
> > > On Tue, Jan 10, 2023 at 12:57:38AM -0500, Michael S. Tsirkin wrote:
> > >> On Tue, Jan 10, 2023 at 12:25:02AM -0500, Michael S. Tsirkin wrote:
> > This will give extra pressure on the management stack, e.g it requires
> > the device to have an out of spec way for introspection.
> >
> > Thanks
> > >>> As I tried to explain this is already the case. Feature bits do not
> > >>> describe device capabilities fully, some of them are in config space.
>
> Yes.
>
> > >> To be precise, this does not necessarily require introspection, but
> > >> it does require management control over config space
> > >> such as supported hash types just like it has control over feature bits.
> > >> E.g. QEMU currently seems to hard-code these to
> > >> #define VIRTIO_NET_RSS_SUPPORTED_HASHES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 |
> > >> \
> > >> VIRTIO_NET_RSS_HASH_TYPE_TCPv4
> > >> | \
> > >> VIRTIO_NET_RSS_HASH_TYPE_UDPv4
> > >> | \
> > >> VIRTIO_NET_RSS_HASH_TYPE_IPv6
> > >> | \
> > >> VIRTIO_NET_RSS_HASH_TYPE_TCPv6
> > >> | \
> > >> VIRTIO_NET_RSS_HASH_TYPE_UDPv6
> > >> | \
> > >> VIRTIO_NET_RSS_HASH_TYPE_IP_EX
> > >> | \
> > >>
> > >> VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | \
> > >>
> > >> VIRTIO_NET_RSS_HASH_TYPE_UDP_EX)
> > >>
> > >> but there's no reason not to give management control over these.
>
> Note that the management expects the migration compatibility to work
> with machine types. So it needs a way to disable some tunnel hash
> types to make it work for old machine types.
yes.
This means qemu will need to create properties for these things
and control through machine type compatibility machinery.
For those not hacking qemu - "machine type" is
a string roughly describing a version of guest/host interface used.
> > > Yes, QEMU has requirements for live migration: the PCI config space will
> > > be
> > > checked in get_pci_config_device(), and if src and dst are inconsistent,
> > > it
> > > will prompt that the live migration failed.
>
> It might be too late since it can't work for the second run (unlike
> subsection).
This is really a low level detail of qemu. I'm not sure how important
this is for the spec.
> >
> > To be clearer, I mean \filed{supported_hash_types} in structure
> > virtio_net_config.
>
> Yes.
>
> Thanks
>
> >
> > Thanks.
> >
> > > In fact, this is also done within our group. Live migration requires that
> > > the two VMs have the same rss configuration, otherwise the migration will
> > > fail.
> > >
> > > Therefore, it seems that we can regularize the description of
> > > VIRTIO_NET_F_HASH_TUNNEL into
> > > "[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner header hash for
> > > tunnel-encapsulated packets.",
> > > and use different hash_types to help the migration determine whether it
> > > can succeed.
> > >
> > > Thanks.
> > >
> > >> --
> > >> MST
> > > This publicly archived list offers a means to provide input to the
> > > OASIS Virtual I/O Device (VIRTIO) TC.
> > >
> > > In order to verify user consent to the Feedback License terms and
> > > to minimize spam in the list archive, subscription is required
> > > before posting.
> > >
> > > Subscribe: virtio-comment-subscr...@lists.oasis-open.org
> > > Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org
> > > List help: virtio-comment-h...@lists.oasis-open.org
> > > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > > List Guidelines:
> > > https://www.oasis-open.org/policies-guidelines/mailing-lists
> > > Committee: https://www.oasis-open.org/committees/virtio/
> > > Join OASIS: https://www.oasis-open.org/join/
> >
-
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: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Wed, Jan 11, 2023 at 11:23 AM Heng Qi wrote:
>
>
>
> 在 2023/1/10 下午3:26, Heng Qi 写道:
> > On Tue, Jan 10, 2023 at 12:57:38AM -0500, Michael S. Tsirkin wrote:
> >> On Tue, Jan 10, 2023 at 12:25:02AM -0500, Michael S. Tsirkin wrote:
> This will give extra pressure on the management stack, e.g it requires
> the device to have an out of spec way for introspection.
>
> Thanks
> >>> As I tried to explain this is already the case. Feature bits do not
> >>> describe device capabilities fully, some of them are in config space.
Yes.
> >> To be precise, this does not necessarily require introspection, but
> >> it does require management control over config space
> >> such as supported hash types just like it has control over feature bits.
> >> E.g. QEMU currently seems to hard-code these to
> >> #define VIRTIO_NET_RSS_SUPPORTED_HASHES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \
> >> VIRTIO_NET_RSS_HASH_TYPE_TCPv4 |
> >> \
> >> VIRTIO_NET_RSS_HASH_TYPE_UDPv4 |
> >> \
> >> VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \
> >> VIRTIO_NET_RSS_HASH_TYPE_TCPv6 |
> >> \
> >> VIRTIO_NET_RSS_HASH_TYPE_UDPv6 |
> >> \
> >> VIRTIO_NET_RSS_HASH_TYPE_IP_EX |
> >> \
> >> VIRTIO_NET_RSS_HASH_TYPE_TCP_EX
> >> | \
> >> VIRTIO_NET_RSS_HASH_TYPE_UDP_EX)
> >>
> >> but there's no reason not to give management control over these.
Note that the management expects the migration compatibility to work
with machine types. So it needs a way to disable some tunnel hash
types to make it work for old machine types.
> > Yes, QEMU has requirements for live migration: the PCI config space will be
> > checked in get_pci_config_device(), and if src and dst are inconsistent, it
> > will prompt that the live migration failed.
It might be too late since it can't work for the second run (unlike subsection).
>
> To be clearer, I mean \filed{supported_hash_types} in structure
> virtio_net_config.
Yes.
Thanks
>
> Thanks.
>
> > In fact, this is also done within our group. Live migration requires that
> > the two VMs have the same rss configuration, otherwise the migration will
> > fail.
> >
> > Therefore, it seems that we can regularize the description of
> > VIRTIO_NET_F_HASH_TUNNEL into
> > "[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner header hash for
> > tunnel-encapsulated packets.",
> > and use different hash_types to help the migration determine whether it can
> > succeed.
> >
> > Thanks.
> >
> >> --
> >> MST
> > This publicly archived list offers a means to provide input to the
> > OASIS Virtual I/O Device (VIRTIO) TC.
> >
> > In order to verify user consent to the Feedback License terms and
> > to minimize spam in the list archive, subscription is required
> > before posting.
> >
> > Subscribe: virtio-comment-subscr...@lists.oasis-open.org
> > Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org
> > List help: virtio-comment-h...@lists.oasis-open.org
> > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > List Guidelines:
> > https://www.oasis-open.org/policies-guidelines/mailing-lists
> > Committee: https://www.oasis-open.org/committees/virtio/
> > Join OASIS: https://www.oasis-open.org/join/
>
-
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: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
在 2023/1/10 下午3:26, Heng Qi 写道:
On Tue, Jan 10, 2023 at 12:57:38AM -0500, Michael S. Tsirkin wrote:
On Tue, Jan 10, 2023 at 12:25:02AM -0500, Michael S. Tsirkin wrote:
This will give extra pressure on the management stack, e.g it requires
the device to have an out of spec way for introspection.
Thanks
As I tried to explain this is already the case. Feature bits do not
describe device capabilities fully, some of them are in config space.
To be precise, this does not necessarily require introspection, but
it does require management control over config space
such as supported hash types just like it has control over feature bits.
E.g. QEMU currently seems to hard-code these to
#define VIRTIO_NET_RSS_SUPPORTED_HASHES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \
VIRTIO_NET_RSS_HASH_TYPE_TCPv4 | \
VIRTIO_NET_RSS_HASH_TYPE_UDPv4 | \
VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \
VIRTIO_NET_RSS_HASH_TYPE_TCPv6 | \
VIRTIO_NET_RSS_HASH_TYPE_UDPv6 | \
VIRTIO_NET_RSS_HASH_TYPE_IP_EX | \
VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | \
VIRTIO_NET_RSS_HASH_TYPE_UDP_EX)
but there's no reason not to give management control over these.
Yes, QEMU has requirements for live migration: the PCI config space will be
checked in get_pci_config_device(), and if src and dst are inconsistent, it
will prompt that the live migration failed.
To be clearer, I mean \filed{supported_hash_types} in structure
virtio_net_config.
Thanks.
In fact, this is also done within our group. Live migration requires that
the two VMs have the same rss configuration, otherwise the migration will fail.
Therefore, it seems that we can regularize the description of
VIRTIO_NET_F_HASH_TUNNEL into
"[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner header hash for
tunnel-encapsulated packets.",
and use different hash_types to help the migration determine whether it can
succeed.
Thanks.
--
MST
This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.
In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.
Subscribe: virtio-comment-subscr...@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org
List help: virtio-comment-h...@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/
-
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 v7] virtio-net: support inner header hash
On Tue, Jan 10, 2023 at 12:57:38AM -0500, Michael S. Tsirkin wrote: > On Tue, Jan 10, 2023 at 12:25:02AM -0500, Michael S. Tsirkin wrote: > > > This will give extra pressure on the management stack, e.g it requires > > > the device to have an out of spec way for introspection. > > > > > > Thanks > > > > As I tried to explain this is already the case. Feature bits do not > > describe device capabilities fully, some of them are in config space. > > To be precise, this does not necessarily require introspection, but > it does require management control over config space > such as supported hash types just like it has control over feature bits. > E.g. QEMU currently seems to hard-code these to > #define VIRTIO_NET_RSS_SUPPORTED_HASHES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \ > VIRTIO_NET_RSS_HASH_TYPE_TCPv4 | \ > VIRTIO_NET_RSS_HASH_TYPE_UDPv4 | \ > VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \ > VIRTIO_NET_RSS_HASH_TYPE_TCPv6 | \ > VIRTIO_NET_RSS_HASH_TYPE_UDPv6 | \ > VIRTIO_NET_RSS_HASH_TYPE_IP_EX | \ > VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | \ > VIRTIO_NET_RSS_HASH_TYPE_UDP_EX) > > but there's no reason not to give management control over these. Yes, QEMU has requirements for live migration: the PCI config space will be checked in get_pci_config_device(), and if src and dst are inconsistent, it will prompt that the live migration failed. In fact, this is also done within our group. Live migration requires that the two VMs have the same rss configuration, otherwise the migration will fail. Therefore, it seems that we can regularize the description of VIRTIO_NET_F_HASH_TUNNEL into "[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner header hash for tunnel-encapsulated packets.", and use different hash_types to help the migration determine whether it can succeed. Thanks. > > -- > MST - 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 v7] virtio-net: support inner header hash
On Tue, Jan 10, 2023 at 12:25:02AM -0500, Michael S. Tsirkin wrote: > > This will give extra pressure on the management stack, e.g it requires > > the device to have an out of spec way for introspection. > > > > Thanks > > As I tried to explain this is already the case. Feature bits do not > describe device capabilities fully, some of them are in config space. To be precise, this does not necessarily require introspection, but it does require management control over config space such as supported hash types just like it has control over feature bits. E.g. QEMU currently seems to hard-code these to #define VIRTIO_NET_RSS_SUPPORTED_HASHES (VIRTIO_NET_RSS_HASH_TYPE_IPv4 | \ VIRTIO_NET_RSS_HASH_TYPE_TCPv4 | \ VIRTIO_NET_RSS_HASH_TYPE_UDPv4 | \ VIRTIO_NET_RSS_HASH_TYPE_IPv6 | \ VIRTIO_NET_RSS_HASH_TYPE_TCPv6 | \ VIRTIO_NET_RSS_HASH_TYPE_UDPv6 | \ VIRTIO_NET_RSS_HASH_TYPE_IP_EX | \ VIRTIO_NET_RSS_HASH_TYPE_TCP_EX | \ VIRTIO_NET_RSS_HASH_TYPE_UDP_EX) but there's no reason not to give management control over these. -- MST - 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 v7] virtio-net: support inner header hash
On Tue, Jan 10, 2023 at 10:06:53AM +0800, Jason Wang wrote:
> On Mon, Jan 9, 2023 at 7:34 PM Michael S. Tsirkin wrote:
> >
> > On Mon, Jan 09, 2023 at 04:59:26PM +0800, Jason Wang wrote:
> > > On Mon, Jan 9, 2023 at 10:43 AM Heng Qi wrote:
> > > >
> > > > On Fri, Jan 06, 2023 at 01:59:38AM -0500, Michael S. Tsirkin wrote:
> > > > > On Fri, Jan 06, 2023 at 02:42:21PM +0800, Heng Qi wrote:
> > > > > > On Fri, Jan 06, 2023 at 12:27:04AM -0500, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> > > > > > > > If the tunnel is used to encapsulate the packets, the hash
> > > > > > > > calculated
> > > > > > > > using the 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.
> > > > > > > >
> > > > > > > > We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related
> > > > > > > > bitmasks
> > > > > > > > in \field{hash_types}, which instructs the device to calculate
> > > > > > > > the
> > > > > > > > hash using the inner headers of tunnel-encapsulated packets.
> > > > > > > > Besides,
> > > > > > > > values in \field{hash_report_tunnel} are added to report tunnel
> > > > > > > > types.
> > > > > > > >
> > > > > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/151
> > > > > > > >
> > > > > > > > Reviewed-by: Jason Wang
> > > > > > > > Signed-off-by: Heng Qi
> > > > > > > > Signed-off-by: Xuan Zhuo
> > > > > > >
> > > > > > >
> > > > > > > ok close to being ready. a couple of minor comments.
> > > > > > >
> > > > > > > > ---
> > > > > > > > v6:
> > > > > > > > 1. Modify the wording of some sentences for clarity.
> > > > > > > > @Michael S. Tsirkin
> > > > > > > > 2. Fix some syntax issues. @Michael S. Tsirkin
> > > > > > > >
> > > > > > > > v5:
> > > > > > > > 1. Fix some syntax and capitalization issues. @Michael
> > > > > > > > S. Tsirkin
> > > > > > > > 2. Use encapsulated/encaptulation uniformly. @Michael
> > > > > > > > S. Tsirkin
> > > > > > > > 3. Move the links to introduction section. @Michael S.
> > > > > > > > Tsirkin
> > > > > > > > 4. Clarify some sentences. @Michael S. Tsirkin
> > > > > > > >
> > > > > > > > v4:
> > > > > > > > 1. Clarify some paragraphs. @Cornelia Huck
> > > > > > > > 2. Fix the u8 type. @Cornelia Huck
> > > > > > > >
> > > > > > > > v3:
> > > > > > > > 1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to
> > > > > > > > VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
> > > > > > > > 2. Make things clearer. @Jason Wang @Michael S. Tsirkin
> > > > > > > > 3. Keep the possibility to use inner hash for automatic
> > > > > > > > receive steering. @Jason Wang
> > > > > > > > 4. Add the "Tunnel packet" paragraph to avoid repeating
> > > > > > > > the GRE etc. many times. @Michael S. Tsirkin
> > > > > > > >
> > > > > > > > v2:
> > > > > > > > 1. Add a feature bit for GRE/VXLAN/GENEVE inner hash.
> > > > > > > > @Jason Wang
> > > > > > > > 2. Chang \field{hash_tunnel} to
> > > > > > > > \field{hash_report_tunnel}. @Jason Wang, @Michael S. Tsirkin
> > > > > > > >
> > > > > > > > v1:
> > > > > > > > 1. Remove the patch for the bitmask fix. @Michael S.
> > > > > > > > Tsirkin
> > > > > > > > 2. Clarify some paragraphs. @Jason Wang
> > > > > > > > 3. Add \field{hash_tunnel} and
> > > > > > > > VIRTIO_NET_HASH_REPORT_GRE. @Yuri Benditovich
> > > > > > > >
> > > > > > > > content.tex | 191
> > > > > > > > +--
> > > > > > > > introduction.tex | 19 +
> > > > > > > > 2 files changed, 203 insertions(+), 7 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/content.tex b/content.tex
> > > > > > > > index e863709..7845f6c 100644
> > > > > > > > --- a/content.tex
> > > > > > > > +++ b/content.tex
> > > > > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature
> > > > > > > > bits}\label{sec:Device Types / Network Device / Feature bits
> > > > > > > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through
> > > > > > > > control
> > > > > > > > channel.
> > > > > > > >
> > > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner
> > > > > > > > +header hash for GRE, VXLAN and GENEVE tunnel-encapsulated
> > > > > > > > packets.
> > > > > > >
> > > > > > > I would probably drop the list of tunnel types here.
> > > > > >
> > > > > > Do you mean to use "Device supports inner header hash for
> > > > > > tunnel-encapsulated packets." instead? Why? We do want to use this
> > > > > > feature bit to indicate that the device supports inner hashing of
> > > > > > GRE, VXLAN and GENEVE encapsulated packets. As in the v3 discussion
> > > > > > https://lists.oasis-open.org/archives/virtio-dev/202212/msg00024.html
> > > > > > ,
> > > > > > we discussed using VIRTIO_NET_F_HASH_TUNNEL to replace
> > >
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Mon, Jan 9, 2023 at 7:34 PM Michael S. Tsirkin wrote:
>
> On Mon, Jan 09, 2023 at 04:59:26PM +0800, Jason Wang wrote:
> > On Mon, Jan 9, 2023 at 10:43 AM Heng Qi wrote:
> > >
> > > On Fri, Jan 06, 2023 at 01:59:38AM -0500, Michael S. Tsirkin wrote:
> > > > On Fri, Jan 06, 2023 at 02:42:21PM +0800, Heng Qi wrote:
> > > > > On Fri, Jan 06, 2023 at 12:27:04AM -0500, Michael S. Tsirkin wrote:
> > > > > > On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> > > > > > > If the tunnel is used to encapsulate the packets, the hash
> > > > > > > calculated
> > > > > > > using the 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.
> > > > > > >
> > > > > > > We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related bitmasks
> > > > > > > in \field{hash_types}, which instructs the device to calculate the
> > > > > > > hash using the inner headers of tunnel-encapsulated packets.
> > > > > > > Besides,
> > > > > > > values in \field{hash_report_tunnel} are added to report tunnel
> > > > > > > types.
> > > > > > >
> > > > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/151
> > > > > > >
> > > > > > > Reviewed-by: Jason Wang
> > > > > > > Signed-off-by: Heng Qi
> > > > > > > Signed-off-by: Xuan Zhuo
> > > > > >
> > > > > >
> > > > > > ok close to being ready. a couple of minor comments.
> > > > > >
> > > > > > > ---
> > > > > > > v6:
> > > > > > > 1. Modify the wording of some sentences for clarity.
> > > > > > > @Michael S. Tsirkin
> > > > > > > 2. Fix some syntax issues. @Michael S. Tsirkin
> > > > > > >
> > > > > > > v5:
> > > > > > > 1. Fix some syntax and capitalization issues. @Michael S.
> > > > > > > Tsirkin
> > > > > > > 2. Use encapsulated/encaptulation uniformly. @Michael S.
> > > > > > > Tsirkin
> > > > > > > 3. Move the links to introduction section. @Michael S.
> > > > > > > Tsirkin
> > > > > > > 4. Clarify some sentences. @Michael S. Tsirkin
> > > > > > >
> > > > > > > v4:
> > > > > > > 1. Clarify some paragraphs. @Cornelia Huck
> > > > > > > 2. Fix the u8 type. @Cornelia Huck
> > > > > > >
> > > > > > > v3:
> > > > > > > 1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to
> > > > > > > VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
> > > > > > > 2. Make things clearer. @Jason Wang @Michael S. Tsirkin
> > > > > > > 3. Keep the possibility to use inner hash for automatic
> > > > > > > receive steering. @Jason Wang
> > > > > > > 4. Add the "Tunnel packet" paragraph to avoid repeating
> > > > > > > the GRE etc. many times. @Michael S. Tsirkin
> > > > > > >
> > > > > > > v2:
> > > > > > > 1. Add a feature bit for GRE/VXLAN/GENEVE inner hash.
> > > > > > > @Jason Wang
> > > > > > > 2. Chang \field{hash_tunnel} to
> > > > > > > \field{hash_report_tunnel}. @Jason Wang, @Michael S. Tsirkin
> > > > > > >
> > > > > > > v1:
> > > > > > > 1. Remove the patch for the bitmask fix. @Michael S.
> > > > > > > Tsirkin
> > > > > > > 2. Clarify some paragraphs. @Jason Wang
> > > > > > > 3. Add \field{hash_tunnel} and
> > > > > > > VIRTIO_NET_HASH_REPORT_GRE. @Yuri Benditovich
> > > > > > >
> > > > > > > content.tex | 191
> > > > > > > +--
> > > > > > > introduction.tex | 19 +
> > > > > > > 2 files changed, 203 insertions(+), 7 deletions(-)
> > > > > > >
> > > > > > > diff --git a/content.tex b/content.tex
> > > > > > > index e863709..7845f6c 100644
> > > > > > > --- a/content.tex
> > > > > > > +++ b/content.tex
> > > > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device
> > > > > > > Types / Network Device / Feature bits
> > > > > > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through
> > > > > > > control
> > > > > > > channel.
> > > > > > >
> > > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner
> > > > > > > +header hash for GRE, VXLAN and GENEVE tunnel-encapsulated
> > > > > > > packets.
> > > > > >
> > > > > > I would probably drop the list of tunnel types here.
> > > > >
> > > > > Do you mean to use "Device supports inner header hash for
> > > > > tunnel-encapsulated packets." instead? Why? We do want to use this
> > > > > feature bit to indicate that the device supports inner hashing of
> > > > > GRE, VXLAN and GENEVE encapsulated packets. As in the v3 discussion
> > > > > https://lists.oasis-open.org/archives/virtio-dev/202212/msg00024.html
> > > > > ,
> > > > > we discussed using VIRTIO_NET_F_HASH_TUNNEL to replace
> > > > > VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER and plan to use
> > > > > VIRTIO_NET_F_HASH_TUNNEL_XYZ for future extensions.
> > > >
> > > > So imagine we add a new tunnel type. Let's say there's VXLAN v2.
> > > > why would we need a new feature bit? I think a new hash type
> > > > will be
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Mon, Jan 09, 2023 at 04:59:26PM +0800, Jason Wang wrote:
> On Mon, Jan 9, 2023 at 10:43 AM Heng Qi wrote:
> >
> > On Fri, Jan 06, 2023 at 01:59:38AM -0500, Michael S. Tsirkin wrote:
> > > On Fri, Jan 06, 2023 at 02:42:21PM +0800, Heng Qi wrote:
> > > > On Fri, Jan 06, 2023 at 12:27:04AM -0500, Michael S. Tsirkin wrote:
> > > > > On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> > > > > > If the tunnel is used to encapsulate the packets, the hash
> > > > > > calculated
> > > > > > using the 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.
> > > > > >
> > > > > > We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related bitmasks
> > > > > > in \field{hash_types}, which instructs the device to calculate the
> > > > > > hash using the inner headers of tunnel-encapsulated packets.
> > > > > > Besides,
> > > > > > values in \field{hash_report_tunnel} are added to report tunnel
> > > > > > types.
> > > > > >
> > > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/151
> > > > > >
> > > > > > Reviewed-by: Jason Wang
> > > > > > Signed-off-by: Heng Qi
> > > > > > Signed-off-by: Xuan Zhuo
> > > > >
> > > > >
> > > > > ok close to being ready. a couple of minor comments.
> > > > >
> > > > > > ---
> > > > > > v6:
> > > > > > 1. Modify the wording of some sentences for clarity.
> > > > > > @Michael S. Tsirkin
> > > > > > 2. Fix some syntax issues. @Michael S. Tsirkin
> > > > > >
> > > > > > v5:
> > > > > > 1. Fix some syntax and capitalization issues. @Michael S.
> > > > > > Tsirkin
> > > > > > 2. Use encapsulated/encaptulation uniformly. @Michael S.
> > > > > > Tsirkin
> > > > > > 3. Move the links to introduction section. @Michael S.
> > > > > > Tsirkin
> > > > > > 4. Clarify some sentences. @Michael S. Tsirkin
> > > > > >
> > > > > > v4:
> > > > > > 1. Clarify some paragraphs. @Cornelia Huck
> > > > > > 2. Fix the u8 type. @Cornelia Huck
> > > > > >
> > > > > > v3:
> > > > > > 1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to
> > > > > > VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
> > > > > > 2. Make things clearer. @Jason Wang @Michael S. Tsirkin
> > > > > > 3. Keep the possibility to use inner hash for automatic
> > > > > > receive steering. @Jason Wang
> > > > > > 4. Add the "Tunnel packet" paragraph to avoid repeating the
> > > > > > GRE etc. many times. @Michael S. Tsirkin
> > > > > >
> > > > > > v2:
> > > > > > 1. Add a feature bit for GRE/VXLAN/GENEVE inner hash.
> > > > > > @Jason Wang
> > > > > > 2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}.
> > > > > > @Jason Wang, @Michael S. Tsirkin
> > > > > >
> > > > > > v1:
> > > > > > 1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
> > > > > > 2. Clarify some paragraphs. @Jason Wang
> > > > > > 3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE.
> > > > > > @Yuri Benditovich
> > > > > >
> > > > > > content.tex | 191
> > > > > > +--
> > > > > > introduction.tex | 19 +
> > > > > > 2 files changed, 203 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/content.tex b/content.tex
> > > > > > index e863709..7845f6c 100644
> > > > > > --- a/content.tex
> > > > > > +++ b/content.tex
> > > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device
> > > > > > Types / Network Device / Feature bits
> > > > > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through
> > > > > > control
> > > > > > channel.
> > > > > >
> > > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner
> > > > > > +header hash for GRE, VXLAN and GENEVE tunnel-encapsulated
> > > > > > packets.
> > > > >
> > > > > I would probably drop the list of tunnel types here.
> > > >
> > > > Do you mean to use "Device supports inner header hash for
> > > > tunnel-encapsulated packets." instead? Why? We do want to use this
> > > > feature bit to indicate that the device supports inner hashing of
> > > > GRE, VXLAN and GENEVE encapsulated packets. As in the v3 discussion
> > > > https://lists.oasis-open.org/archives/virtio-dev/202212/msg00024.html ,
> > > > we discussed using VIRTIO_NET_F_HASH_TUNNEL to replace
> > > > VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER and plan to use
> > > > VIRTIO_NET_F_HASH_TUNNEL_XYZ for future extensions.
> > >
> > > So imagine we add a new tunnel type. Let's say there's VXLAN v2.
> > > why would we need a new feature bit? I think a new hash type
> > > will be sufficient. No?
> >
> > If the description for VIRTIO_NET_F_HASH_TUNNEL is as follows:
> > "[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner header hash for
> > tunnel-encapsulated packets.".
> > Then the following may happen
> > 1. For VXLANv2, if both device src and device dst have
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v7] virtio-net: support inner header hash
On Mon, Jan 9, 2023 at 10:43 AM Heng Qi wrote:
>
> On Fri, Jan 06, 2023 at 01:59:38AM -0500, Michael S. Tsirkin wrote:
> > On Fri, Jan 06, 2023 at 02:42:21PM +0800, Heng Qi wrote:
> > > On Fri, Jan 06, 2023 at 12:27:04AM -0500, Michael S. Tsirkin wrote:
> > > > On Wed, Jan 04, 2023 at 03:14:01PM +0800, Heng Qi wrote:
> > > > > If the tunnel is used to encapsulate the packets, the hash calculated
> > > > > using the 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.
> > > > >
> > > > > We add a feature bit VIRTIO_NET_F_HASH_TUNNEL and related bitmasks
> > > > > in \field{hash_types}, which instructs the device to calculate the
> > > > > hash using the inner headers of tunnel-encapsulated packets. Besides,
> > > > > values in \field{hash_report_tunnel} are added to report tunnel types.
> > > > >
> > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/151
> > > > >
> > > > > Reviewed-by: Jason Wang
> > > > > Signed-off-by: Heng Qi
> > > > > Signed-off-by: Xuan Zhuo
> > > >
> > > >
> > > > ok close to being ready. a couple of minor comments.
> > > >
> > > > > ---
> > > > > v6:
> > > > > 1. Modify the wording of some sentences for clarity. @Michael
> > > > > S. Tsirkin
> > > > > 2. Fix some syntax issues. @Michael S. Tsirkin
> > > > >
> > > > > v5:
> > > > > 1. Fix some syntax and capitalization issues. @Michael S.
> > > > > Tsirkin
> > > > > 2. Use encapsulated/encaptulation uniformly. @Michael S.
> > > > > Tsirkin
> > > > > 3. Move the links to introduction section. @Michael S. Tsirkin
> > > > > 4. Clarify some sentences. @Michael S. Tsirkin
> > > > >
> > > > > v4:
> > > > > 1. Clarify some paragraphs. @Cornelia Huck
> > > > > 2. Fix the u8 type. @Cornelia Huck
> > > > >
> > > > > v3:
> > > > > 1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to
> > > > > VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
> > > > > 2. Make things clearer. @Jason Wang @Michael S. Tsirkin
> > > > > 3. Keep the possibility to use inner hash for automatic
> > > > > receive steering. @Jason Wang
> > > > > 4. Add the "Tunnel packet" paragraph to avoid repeating the
> > > > > GRE etc. many times. @Michael S. Tsirkin
> > > > >
> > > > > v2:
> > > > > 1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason
> > > > > Wang
> > > > > 2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}.
> > > > > @Jason Wang, @Michael S. Tsirkin
> > > > >
> > > > > v1:
> > > > > 1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
> > > > > 2. Clarify some paragraphs. @Jason Wang
> > > > > 3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE.
> > > > > @Yuri Benditovich
> > > > >
> > > > > content.tex | 191
> > > > > +--
> > > > > introduction.tex | 19 +
> > > > > 2 files changed, 203 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/content.tex b/content.tex
> > > > > index e863709..7845f6c 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device
> > > > > Types / Network Device / Feature bits
> > > > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> > > > > channel.
> > > > >
> > > > > +\item[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner
> > > > > +header hash for GRE, VXLAN and GENEVE tunnel-encapsulated
> > > > > packets.
> > > >
> > > > I would probably drop the list of tunnel types here.
> > >
> > > Do you mean to use "Device supports inner header hash for
> > > tunnel-encapsulated packets." instead? Why? We do want to use this
> > > feature bit to indicate that the device supports inner hashing of
> > > GRE, VXLAN and GENEVE encapsulated packets. As in the v3 discussion
> > > https://lists.oasis-open.org/archives/virtio-dev/202212/msg00024.html ,
> > > we discussed using VIRTIO_NET_F_HASH_TUNNEL to replace
> > > VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER and plan to use
> > > VIRTIO_NET_F_HASH_TUNNEL_XYZ for future extensions.
> >
> > So imagine we add a new tunnel type. Let's say there's VXLAN v2.
> > why would we need a new feature bit? I think a new hash type
> > will be sufficient. No?
>
> If the description for VIRTIO_NET_F_HASH_TUNNEL is as follows:
> "[VIRTIO_NET_F_HASH_TUNNEL(52)] Device supports inner header hash for
> tunnel-encapsulated packets.".
> Then the following may happen
> 1. For VXLANv2, if both device src and device dst have negotiated this
> feature, it is assumed that
>device src supports VXLAN and VXLANv2, but device dst may only support
> VXLAN, not VXLANv2.
> 2. For other encapsulation protocols such as ip in ip, after device src and
> device dst have
>negotiated this feature, it is assumed that device src supports GRE,
> VXLAN, GENEVE and ip in ip,
>
