ethtool NFC/ntuple API questions

ethtool NFC/ntuple API questions

[Edward Cree]

I'm looking into adding IPv6 support to the ethtool flow steering API.  But,
I don't know "the unfortunate history of and subtle differences between the
RX n-tuple versus RX NFC commands".  In particular, would I need to add IPv6
support to both of them, or only one?  If one would be sufficient, which one
is preferred?
Also, is it necessary to duplicate the profusion of variants that the IPv4
flow specs have (3x struct ethtool_tcpip4_spec, 2x struct
ethtool_ah_espip4_spec, and struct ethtool_usrip4_spec), or should I just
make one struct that contains all the fields from those (I would say "the
union of their fields", but that might be confusing), and rely on flow_type
to indicate which fields are meaningful?
And, what exactly are the hdata fields in ethtool_flow_union and the
anonymous union in ethtool_rx_ntuple_flow_spec (they're not documented) and
why are they different sizes?

-Ed

Re: ethtool NFC/ntuple API questions

[Alexander Duyck]

On Wed, Jan 20, 2016 at 9:10 AM, Edward Cree <ecree@solarflare.com> wrote:
> I'm looking into adding IPv6 support to the ethtool flow steering API.  But,
> I don't know "the unfortunate history of and subtle differences between the
> RX n-tuple versus RX NFC commands".  In particular, would I need to add IPv6
> support to both of them, or only one?  If one would be sufficient, which one
> is preferred?

I'd say just focus on Rx NFC.  The NTUPLE interface is only really
used for legacy support on ixgbe if I recall correctly.  The original
implementation was badly broken, but because it went out we are stuck
supporting it.  Any new features can be added to the Rx NFC since that
is the interface that has the ability to both set and get filters.

> Also, is it necessary to duplicate the profusion of variants that the IPv4
> flow specs have (3x struct ethtool_tcpip4_spec, 2x struct
> ethtool_ah_espip4_spec, and struct ethtool_usrip4_spec), or should I just
> make one struct that contains all the fields from those (I would say "the
> union of their fields", but that might be confusing), and rely on flow_type
> to indicate which fields are meaningful?

I'd say just stick with the approach taken for IPv4 since it makes it
much more readable.  There were only really 4 types in use, but we
named each to make certain it was clear which one should be used for
each type.  To some extent the approach of relying on the flow_type is
already in use, it is just made clearer by specifying which union to
use for each type.

> And, what exactly are the hdata fields in ethtool_flow_union and the
> anonymous union in ethtool_rx_ntuple_flow_spec (they're not documented) and
> why are they different sizes?

The hdata is essentially just padding that defines the entire region
size.  For the user interface we have to reserve some amount of space,
and in order to make the flow definitions compatible with NTUPLE we
added extensions so that we could provide the information about the
VLAN header if needed.

The reason for the sizing difference is that the ethtool_flow_union is
half of the flow definition, the other half is stored in
ethtool_flow_ext.  We shimmed ethtool_flow_ext into Rx NFC in order to
work around the limitations of the NTUPLE filters.  The structure you
probably should be looking at for comparison to NTUPLE is
ethtool_rx_flow_spec, not ethtool_flow_union as that helps to tell the
whole story.

- Alex

Re: ethtool NFC/ntuple API questions

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3254 bytes --]

On Wed, 2016-01-20 at 09:53 -0800, Alexander Duyck wrote:
> On Wed, Jan 20, 2016 at 9:10 AM, Edward Cree <ecree@solarflare.com> wrote:
> > I'm looking into adding IPv6 support to the ethtool flow steering API.  But,
> > I don't know "the unfortunate history of and subtle differences between the
> > RX n-tuple versus RX NFC commands".  In particular, would I need to add IPv6
> > support to both of them, or only one?  If one would be sufficient, which one
> > is preferred?
> 
> I'd say just focus on Rx NFC.  The NTUPLE interface is only really
> used for legacy support on ixgbe if I recall correctly.

sfc also supported it for a while.

> The original
> implementation was badly broken, but because it went out we are stuck
> supporting it.  Any new features can be added to the Rx NFC since that
> is the interface that has the ability to both set and get filters.

Right.  In fact maybe the ntuple stuff could be removed from the UAPI
headers given it's no longer part of the actual UAPI.

> > Also, is it necessary to duplicate the profusion of variants that the IPv4
> > flow specs have (3x struct ethtool_tcpip4_spec, 2x struct
> > ethtool_ah_espip4_spec, and struct ethtool_usrip4_spec), or should I just
> > make one struct that contains all the fields from those (I would say "the
> > union of their fields", but that might be confusing), and rely on flow_type
> > to indicate which fields are meaningful?
> 
> I'd say just stick with the approach taken for IPv4 since it makes it
> much more readable.  There were only really 4 types in use, but we
> named each to make certain it was clear which one should be used for
> each type.  To some extent the approach of relying on the flow_type is
> already in use, it is just made clearer by specifying which union to
> use for each type.

I don't mind one way or the other.

> > And, what exactly are the hdata fields in ethtool_flow_union and the
> > anonymous union in ethtool_rx_ntuple_flow_spec (they're not documented) and
> > why are they different sizes?
> 
> The hdata is essentially just padding that defines the entire region
> size.  For the user interface we have to reserve some amount of space,
> and in order to make the flow definitions compatible with NTUPLE we
> added extensions so that we could provide the information about the
> VLAN header if needed.
> 
> The reason for the sizing difference is that the ethtool_flow_union is
> half of the flow definition, the other half is stored in
> ethtool_flow_ext.  We shimmed ethtool_flow_ext into Rx NFC in order to
> work around the limitations of the NTUPLE filters.  The structure you
> probably should be looking at for comparison to NTUPLE is
> ethtool_rx_flow_spec, not ethtool_flow_union as that helps to tell the
> whole story.

Right.  Further, we can extend ethtool_flow_ext *downwards* so long as
we shrink ethtool_flow_union by the same amount (and add a type flag
for the extension).

I already checked that ethtool_flow_union remained large enough to hold
IPv6 flow specs because I expected sfc would support them some day. :-)

Ben.

-- 
Ben Hutchings
The program is absolutely right; therefore, the computer must be wrong.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: ethtool NFC/ntuple API questions

[Edward Cree]

Thanks both, it's making more sense now.
One thing I'm still unclear about: why does struct ethtool_usrip4_spechave
the ip_ver field?  The struct can't be extended to cover ipv6, because the
address fields aren't big enough.  So what's it for?

Also, would it be appropriate to use struct in6_addr for IPv6 addresses, or
should I use __be32[4]?

-Ed

Re: ethtool NFC/ntuple API questions

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On Wed, 2016-01-20 at 19:12 +0000, Edward Cree wrote:
> Thanks both, it's making more sense now.
> One thing I'm still unclear about: why does struct ethtool_usrip4_spechave
> the ip_ver field?  The struct can't be extended to cover ipv6, because the
> address fields aren't big enough.  So what's it for?

It's also defined to always have the same value and mask!  It's a
design bug.

> Also, would it be appropriate to use struct in6_addr for IPv6 addresses, or
> should I use __be32[4]?

I think for consistency with the IPv4 structures it should be __be32[4].

Ben.

-- 
Ben Hutchings
The program is absolutely right; therefore, the computer must be wrong.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[RFC PATCH] ethtool: add IPv6 to the NFC API

[Edward Cree]

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
Haven't yet tried to write the ethtool client end of this (or a driver
implementation), but does it look reasonable?

 include/uapi/linux/ethtool.h | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 57fa390..74bef53 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -748,6 +748,24 @@ struct ethtool_usrip4_spec {
 	__u8    proto;
 };
 
+/**
+ * struct ethtool_ip6_spec - general flow specification for IPv6
+ * @ip6src: Source host
+ * @ip6dst: Destination host
+ * @l4_4_bytes: First 4 bytes of transport (layer 4) header
+ * @spi: Security parameters index, for %AH_V6_FLOW and %ESP_V6_FLOW
+ * @tos: Type-of-service
+ * @proto: Transport protocol number, for %IP6_USER_FLOW
+ */
+struct ethtool_ip6_spec {
+	__be32	ip6src[4];
+	__be32	ip6dst[4];
+	__be32	l4_4_bytes;
+	__be32	spi;
+	__u8	tos;
+	__u8	proto;
+};
+
 union ethtool_flow_union {
 	struct ethtool_tcpip4_spec		tcp_ip4_spec;
 	struct ethtool_tcpip4_spec		udp_ip4_spec;
@@ -755,6 +773,7 @@ union ethtool_flow_union {
 	struct ethtool_ah_espip4_spec		ah_ip4_spec;
 	struct ethtool_ah_espip4_spec		esp_ip4_spec;
 	struct ethtool_usrip4_spec		usr_ip4_spec;
+	struct ethtool_ip6_spec			ip6_spec;
 	struct ethhdr				ether_spec;
 	__u8					hdata[52];
 };
@@ -1367,18 +1386,19 @@ enum ethtool_sfeatures_retval_bits {
 #define	UDP_V4_FLOW	0x02	/* hash or spec (udp_ip4_spec) */
 #define	SCTP_V4_FLOW	0x03	/* hash or spec (sctp_ip4_spec) */
 #define	AH_ESP_V4_FLOW	0x04	/* hash only */
-#define	TCP_V6_FLOW	0x05	/* hash only */
-#define	UDP_V6_FLOW	0x06	/* hash only */
-#define	SCTP_V6_FLOW	0x07	/* hash only */
+#define	TCP_V6_FLOW	0x05	/* hash or spec (ip6_spec) */
+#define	UDP_V6_FLOW	0x06	/* hash or spec (ip6_spec) */
+#define	SCTP_V6_FLOW	0x07	/* hash or spec (ip6_spec) */
 #define	AH_ESP_V6_FLOW	0x08	/* hash only */
 #define	AH_V4_FLOW	0x09	/* hash or spec (ah_ip4_spec) */
 #define	ESP_V4_FLOW	0x0a	/* hash or spec (esp_ip4_spec) */
-#define	AH_V6_FLOW	0x0b	/* hash only */
-#define	ESP_V6_FLOW	0x0c	/* hash only */
+#define	AH_V6_FLOW	0x0b	/* hash or spec (ip6_spec) */
+#define	ESP_V6_FLOW	0x0c	/* hash or spec (ip6_spec) */
 #define	IP_USER_FLOW	0x0d	/* spec only (usr_ip4_spec) */
 #define	IPV4_FLOW	0x10	/* hash only */
 #define	IPV6_FLOW	0x11	/* hash only */
 #define	ETHER_FLOW	0x12	/* spec only (ether_spec) */
+#define	IP6_USER_FLOW	0x13	/* spec only (ip6_spec) */
 /* Flag to enable additional fields in struct ethtool_rx_flow_spec */
 #define	FLOW_EXT	0x80000000
 #define	FLOW_MAC_EXT	0x40000000

Re: [RFC PATCH] ethtool: add IPv6 to the NFC API

[Alexander Duyck]

On Thu, Jan 21, 2016 at 11:14 AM, Edward Cree <ecree@solarflare.com> wrote:
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
> Haven't yet tried to write the ethtool client end of this (or a driver
> implementation), but does it look reasonable?
>
>  include/uapi/linux/ethtool.h | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 57fa390..74bef53 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -748,6 +748,24 @@ struct ethtool_usrip4_spec {
>         __u8    proto;
>  };
>
> +/**
> + * struct ethtool_ip6_spec - general flow specification for IPv6
> + * @ip6src: Source host
> + * @ip6dst: Destination host
> + * @l4_4_bytes: First 4 bytes of transport (layer 4) header
> + * @spi: Security parameters index, for %AH_V6_FLOW and %ESP_V6_FLOW
> + * @tos: Type-of-service
> + * @proto: Transport protocol number, for %IP6_USER_FLOW
> + */
> +struct ethtool_ip6_spec {
> +       __be32  ip6src[4];
> +       __be32  ip6dst[4];
> +       __be32  l4_4_bytes;

The one concern I would have about the filter is that for protocols
like UDP and TCP which have 2 __be16 values in the l4_4_bytes field is
that it would be pretty easy to end up swapping src and dst and then
making a mess of it.  I really would like to see a ethtool_tcpip6_spec
just to make sure we have it laid out clearly that it should be src
port and then destination port.

> +       __be32  spi;

Is this supposed to be the flow label, or is this the IPSec security
parameter index?  If it is the flow label you may want to rename it.
If it is supposed to be the IPSec security parameter index this might
belong in a different flow definition since it is not actually a part
of the IPv6 header.

> +       __u8    tos;
> +       __u8    proto;
> +};
> +

Technically the name of the field for proto is nexthdr.

>  union ethtool_flow_union {
>         struct ethtool_tcpip4_spec              tcp_ip4_spec;
>         struct ethtool_tcpip4_spec              udp_ip4_spec;
> @@ -755,6 +773,7 @@ union ethtool_flow_union {
>         struct ethtool_ah_espip4_spec           ah_ip4_spec;
>         struct ethtool_ah_espip4_spec           esp_ip4_spec;
>         struct ethtool_usrip4_spec              usr_ip4_spec;
> +       struct ethtool_ip6_spec                 ip6_spec;
>         struct ethhdr                           ether_spec;
>         __u8                                    hdata[52];
>  };
> @@ -1367,18 +1386,19 @@ enum ethtool_sfeatures_retval_bits {
>  #define        UDP_V4_FLOW     0x02    /* hash or spec (udp_ip4_spec) */
>  #define        SCTP_V4_FLOW    0x03    /* hash or spec (sctp_ip4_spec) */
>  #define        AH_ESP_V4_FLOW  0x04    /* hash only */
> -#define        TCP_V6_FLOW     0x05    /* hash only */
> -#define        UDP_V6_FLOW     0x06    /* hash only */
> -#define        SCTP_V6_FLOW    0x07    /* hash only */
> +#define        TCP_V6_FLOW     0x05    /* hash or spec (ip6_spec) */
> +#define        UDP_V6_FLOW     0x06    /* hash or spec (ip6_spec) */
> +#define        SCTP_V6_FLOW    0x07    /* hash or spec (ip6_spec) */
>  #define        AH_ESP_V6_FLOW  0x08    /* hash only */
>  #define        AH_V4_FLOW      0x09    /* hash or spec (ah_ip4_spec) */
>  #define        ESP_V4_FLOW     0x0a    /* hash or spec (esp_ip4_spec) */
> -#define        AH_V6_FLOW      0x0b    /* hash only */
> -#define        ESP_V6_FLOW     0x0c    /* hash only */
> +#define        AH_V6_FLOW      0x0b    /* hash or spec (ip6_spec) */
> +#define        ESP_V6_FLOW     0x0c    /* hash or spec (ip6_spec) */
>  #define        IP_USER_FLOW    0x0d    /* spec only (usr_ip4_spec) */
>  #define        IPV4_FLOW       0x10    /* hash only */
>  #define        IPV6_FLOW       0x11    /* hash only */
>  #define        ETHER_FLOW      0x12    /* spec only (ether_spec) */
> +#define        IP6_USER_FLOW   0x13    /* spec only (ip6_spec) */
>  /* Flag to enable additional fields in struct ethtool_rx_flow_spec */
>  #define        FLOW_EXT        0x80000000
>  #define        FLOW_MAC_EXT    0x40000000

Re: [RFC PATCH] ethtool: add IPv6 to the NFC API

[Edward Cree]

On 21/01/16 22:48, Alexander Duyck wrote:
> On Thu, Jan 21, 2016 at 11:14 AM, Edward Cree <ecree@solarflare.com> wrote:
>> +       __be32  spi;
> Is this supposed to be the flow label, or is this the IPSec security
> parameter index?  If it is the flow label you may want to rename it.
> If it is supposed to be the IPSec security parameter index this might
> belong in a different flow definition since it is not actually a part
> of the IPv6 header.
It's the IPSec SPI; I just blindly copied what ethtool_ah_espip4_spec had.
I guess splitting out three different spec structs as per ipv4 would make
this somewhat clearer.
Would the flow label be useful thing to include?  I would have thought it'd
be a bit too short-lived normally.
>> +       __u8    tos;
>> +       __u8    proto;
>> +};
>> +
> Technically the name of the field for proto is nexthdr.
But will NICs filter on the actual nexthdr, or on the *last* nexthdr in the
chain of IPv6 options?  If the latter, calling it nexthdr might be misleading.
(I've just checked what sfc will do, it filters on the last nexthdr.)

Will spin a v2 shortly.

-Ed

[RFC PATCH v2] ethtool: add IPv6 to the NFC API

[Edward Cree]

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
changes from v1:
* split out separate spec structs for different flow types
* clarified the proto field in usr_ip6_spec
* changed IP6_USER_FLOW to 0x0e as I noticed there's a gap there

 include/uapi/linux/ethtool.h | 67 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 62 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 57fa390..ad805b9 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -748,6 +748,56 @@ struct ethtool_usrip4_spec {
 	__u8    proto;
 };
 
+/**
+ * struct ethtool_tcpip6_spec - flow specification for TCP/IPv6 etc.
+ * @ip6src: Source host
+ * @ip6dst: Destination host
+ * @psrc: Source port
+ * @pdst: Destination port
+ * @tos: Type-of-service
+ *
+ * This can be used to specify a TCP/IPv6, UDP/IPv6 or SCTP/IPv6 flow.
+ */
+struct ethtool_tcpip6_spec {
+	__be32	ip6src[4];
+	__be32	ip6dst[4];
+	__be16	psrc;
+	__be16	pdst;
+	__u8    tos;
+};
+
+/**
+ * struct ethtool_ah_espip6_spec - flow specification for IPsec/IPv6
+ * @ip6src: Source host
+ * @ip6dst: Destination host
+ * @spi: Security parameters index
+ * @tos: Type-of-service
+ *
+ * This can be used to specify an IPsec transport or tunnel over IPv6.
+ */
+struct ethtool_ah_espip6_spec {
+	__be32	ip6src[4];
+	__be32	ip6dst[4];
+	__be32	spi;
+	__u8    tos;
+};
+
+/**
+ * struct ethtool_usrip6_spec - general flow specification for IPv6
+ * @ip6src: Source host
+ * @ip6dst: Destination host
+ * @l4_4_bytes: First 4 bytes of transport (layer 4) header
+ * @tos: Type-of-service
+ * @proto: Transport protocol number (nexthdr after any Extension Headers)
+ */
+struct ethtool_usrip6_spec {
+	__be32	ip6src[4];
+	__be32	ip6dst[4];
+	__be32	l4_4_bytes;
+	__u8    tos;
+	__u8    proto;
+};
+
 union ethtool_flow_union {
 	struct ethtool_tcpip4_spec		tcp_ip4_spec;
 	struct ethtool_tcpip4_spec		udp_ip4_spec;
@@ -755,6 +805,12 @@ union ethtool_flow_union {
 	struct ethtool_ah_espip4_spec		ah_ip4_spec;
 	struct ethtool_ah_espip4_spec		esp_ip4_spec;
 	struct ethtool_usrip4_spec		usr_ip4_spec;
+	struct ethtool_tcpip6_spec		tcp_ip6_spec;
+	struct ethtool_tcpip6_spec		udp_ip6_spec;
+	struct ethtool_tcpip6_spec		sctp_ip6_spec;
+	struct ethtool_ah_espip6_spec		ah_ip6_spec;
+	struct ethtool_ah_espip6_spec		esp_ip6_spec;
+	struct ethtool_usrip6_spec		usr_ip6_spec;
 	struct ethhdr				ether_spec;
 	__u8					hdata[52];
 };
@@ -1367,15 +1423,16 @@ enum ethtool_sfeatures_retval_bits {
 #define	UDP_V4_FLOW	0x02	/* hash or spec (udp_ip4_spec) */
 #define	SCTP_V4_FLOW	0x03	/* hash or spec (sctp_ip4_spec) */
 #define	AH_ESP_V4_FLOW	0x04	/* hash only */
-#define	TCP_V6_FLOW	0x05	/* hash only */
-#define	UDP_V6_FLOW	0x06	/* hash only */
-#define	SCTP_V6_FLOW	0x07	/* hash only */
+#define	TCP_V6_FLOW	0x05	/* hash or spec (tcp_ip6_spec; nfc only) */
+#define	UDP_V6_FLOW	0x06	/* hash or spec (udp_ip6_spec; nfc only) */
+#define	SCTP_V6_FLOW	0x07	/* hash or spec (sctp_ip6_spec; nfc only) */
 #define	AH_ESP_V6_FLOW	0x08	/* hash only */
 #define	AH_V4_FLOW	0x09	/* hash or spec (ah_ip4_spec) */
 #define	ESP_V4_FLOW	0x0a	/* hash or spec (esp_ip4_spec) */
-#define	AH_V6_FLOW	0x0b	/* hash only */
-#define	ESP_V6_FLOW	0x0c	/* hash only */
+#define	AH_V6_FLOW	0x0b	/* hash or spec (ah_ip6_spec; nfc only) */
+#define	ESP_V6_FLOW	0x0c	/* hash or spec (esp_ip6_spec; nfc only) */
 #define	IP_USER_FLOW	0x0d	/* spec only (usr_ip4_spec) */
+#define	IP6_USER_FLOW	0x0e	/* spec only (usr_ip6_spec; nfc only) */
 #define	IPV4_FLOW	0x10	/* hash only */
 #define	IPV6_FLOW	0x11	/* hash only */
 #define	ETHER_FLOW	0x12	/* spec only (ether_spec) */

Re: [RFC PATCH v2] ethtool: add IPv6 to the NFC API

[Alexander Duyck]

On Fri, Jan 22, 2016 at 10:04 AM, Edward Cree <ecree@solarflare.com> wrote:
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
> changes from v1:
> * split out separate spec structs for different flow types
> * clarified the proto field in usr_ip6_spec
> * changed IP6_USER_FLOW to 0x0e as I noticed there's a gap there
>
>  include/uapi/linux/ethtool.h | 67 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index 57fa390..ad805b9 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -748,6 +748,56 @@ struct ethtool_usrip4_spec {
>         __u8    proto;
>  };
>
> +/**
> + * struct ethtool_tcpip6_spec - flow specification for TCP/IPv6 etc.
> + * @ip6src: Source host
> + * @ip6dst: Destination host
> + * @psrc: Source port
> + * @pdst: Destination port
> + * @tos: Type-of-service
> + *
> + * This can be used to specify a TCP/IPv6, UDP/IPv6 or SCTP/IPv6 flow.
> + */
> +struct ethtool_tcpip6_spec {
> +       __be32  ip6src[4];
> +       __be32  ip6dst[4];
> +       __be16  psrc;
> +       __be16  pdst;
> +       __u8    tos;
> +};
> +
> +/**
> + * struct ethtool_ah_espip6_spec - flow specification for IPsec/IPv6
> + * @ip6src: Source host
> + * @ip6dst: Destination host
> + * @spi: Security parameters index
> + * @tos: Type-of-service
> + *
> + * This can be used to specify an IPsec transport or tunnel over IPv6.
> + */
> +struct ethtool_ah_espip6_spec {
> +       __be32  ip6src[4];
> +       __be32  ip6dst[4];
> +       __be32  spi;
> +       __u8    tos;
> +};
> +
> +/**
> + * struct ethtool_usrip6_spec - general flow specification for IPv6
> + * @ip6src: Source host
> + * @ip6dst: Destination host
> + * @l4_4_bytes: First 4 bytes of transport (layer 4) header
> + * @tos: Type-of-service
> + * @proto: Transport protocol number (nexthdr after any Extension Headers)
> + */
> +struct ethtool_usrip6_spec {
> +       __be32  ip6src[4];
> +       __be32  ip6dst[4];
> +       __be32  l4_4_bytes;
> +       __u8    tos;
> +       __u8    proto;
> +};
> +

It might be better to refer to this as l4_proto so that it is clear
that this is specifying the protocol of the l4 header that the
l4_4_bytes will be pulled from.

It still might even be useful to add a nexthdr field since it is
possible that there may be NICs out there that don't support parsing
the extension headers.  In such a case they could block setting
protocol and use nexthdr instead.  It provides an indirect way of
communicating if the NIC supports parsing extension headers or not as
the NIC can block adding a filter on one mask being set or the other.

>  union ethtool_flow_union {
>         struct ethtool_tcpip4_spec              tcp_ip4_spec;
>         struct ethtool_tcpip4_spec              udp_ip4_spec;
> @@ -755,6 +805,12 @@ union ethtool_flow_union {
>         struct ethtool_ah_espip4_spec           ah_ip4_spec;
>         struct ethtool_ah_espip4_spec           esp_ip4_spec;
>         struct ethtool_usrip4_spec              usr_ip4_spec;
> +       struct ethtool_tcpip6_spec              tcp_ip6_spec;
> +       struct ethtool_tcpip6_spec              udp_ip6_spec;
> +       struct ethtool_tcpip6_spec              sctp_ip6_spec;
> +       struct ethtool_ah_espip6_spec           ah_ip6_spec;
> +       struct ethtool_ah_espip6_spec           esp_ip6_spec;
> +       struct ethtool_usrip6_spec              usr_ip6_spec;
>         struct ethhdr                           ether_spec;
>         __u8                                    hdata[52];
>  };
> @@ -1367,15 +1423,16 @@ enum ethtool_sfeatures_retval_bits {
>  #define        UDP_V4_FLOW     0x02    /* hash or spec (udp_ip4_spec) */
>  #define        SCTP_V4_FLOW    0x03    /* hash or spec (sctp_ip4_spec) */
>  #define        AH_ESP_V4_FLOW  0x04    /* hash only */
> -#define        TCP_V6_FLOW     0x05    /* hash only */
> -#define        UDP_V6_FLOW     0x06    /* hash only */
> -#define        SCTP_V6_FLOW    0x07    /* hash only */
> +#define        TCP_V6_FLOW     0x05    /* hash or spec (tcp_ip6_spec; nfc only) */
> +#define        UDP_V6_FLOW     0x06    /* hash or spec (udp_ip6_spec; nfc only) */
> +#define        SCTP_V6_FLOW    0x07    /* hash or spec (sctp_ip6_spec; nfc only) */
>  #define        AH_ESP_V6_FLOW  0x08    /* hash only */
>  #define        AH_V4_FLOW      0x09    /* hash or spec (ah_ip4_spec) */
>  #define        ESP_V4_FLOW     0x0a    /* hash or spec (esp_ip4_spec) */
> -#define        AH_V6_FLOW      0x0b    /* hash only */
> -#define        ESP_V6_FLOW     0x0c    /* hash only */
> +#define        AH_V6_FLOW      0x0b    /* hash or spec (ah_ip6_spec; nfc only) */
> +#define        ESP_V6_FLOW     0x0c    /* hash or spec (esp_ip6_spec; nfc only) */
>  #define        IP_USER_FLOW    0x0d    /* spec only (usr_ip4_spec) */
> +#define        IP6_USER_FLOW   0x0e    /* spec only (usr_ip6_spec; nfc only) */
>  #define        IPV4_FLOW       0x10    /* hash only */
>  #define        IPV6_FLOW       0x11    /* hash only */
>  #define        ETHER_FLOW      0x12    /* spec only (ether_spec) */

Re: [RFC PATCH v2] ethtool: add IPv6 to the NFC API

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

On Fri, 2016-01-22 at 18:04 +0000, Edward Cree wrote:
[...]
>  #define	IP_USER_FLOW	0x0d	/* spec only (usr_ip4_spec) */
> +#define	IP6_USER_FLOW	0x0e	/* spec only (usr_ip6_spec; nfc only) */
[...]

This should be named IPV6_USER_FLOW for consistency with the other NFC
type names.  It might also be worth adding IPV4_USER_FLOW as an alias
for IP_USER_FLOW.

Otherwise this looks fine.

Ben.

-- 
Ben Hutchings
Klipstein's 4th Law of Prototyping and Production:
                                    A fail-safe circuit will destroy others.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [RFC PATCH v2] ethtool: add IPv6 to the NFC API

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]

On Fri, 2016-01-22 at 10:54 -0800, Alexander Duyck wrote:
> On Fri, Jan 22, 2016 at 10:04 AM, Edward Cree <ecree@solarflare.com> wrote:
[...]
> > +/**
> > + * struct ethtool_usrip6_spec - general flow specification for IPv6
> > + * @ip6src: Source host
> > + * @ip6dst: Destination host
> > + * @l4_4_bytes: First 4 bytes of transport (layer 4) header
> > + * @tos: Type-of-service
> > + * @proto: Transport protocol number (nexthdr after any Extension Headers)
> > + */
> > +struct ethtool_usrip6_spec {
> > +       __be32  ip6src[4];
> > +       __be32  ip6dst[4];
> > +       __be32  l4_4_bytes;
> > +       __u8    tos;
> > +       __u8    proto;
> > +};
> > +
> 
> It might be better to refer to this as l4_proto so that it is clear
> that this is specifying the protocol of the l4 header that the
> l4_4_bytes will be pulled from.

The comment seems to make it fairly clear.

> It still might even be useful to add a nexthdr field since it is
> possible that there may be NICs out there that don't support parsing
> the extension headers.  In such a case they could block setting
> protocol and use nexthdr instead.  It provides an indirect way of
> communicating if the NIC supports parsing extension headers or not as
> the NIC can block adding a filter on one mask being set or the other.

I don't think a NIC can do any useful flow steering for IPv6 without
being able to parse and skip over the extension headers.  It would be
like trying to match flows by looking at IPv4 header options.

Ben.

-- 
Ben Hutchings
Klipstein's 4th Law of Prototyping and Production:
                                    A fail-safe circuit will destroy others.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]