[PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Sridhar Samudrala]

Fix support for 16 bit source/dest port matches in ixgbe model.
u32 uses a single 32-bit key value for both source and destination ports
starting at offset 0. So replace the 2 functions with a single function
that takes this key value/mask to program both source and dest ports.

Remove the incorrect check for mask in ixgbe_configure_clsu32()

Tested with the following filters:

 #tc qdisc add dev p4p1 ingress
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 800:0:1 u32 ht 800: \
	match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop

 #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
	handle 800:0:1 u32
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 1: u32 divisor 1
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 800:0:10 u32 ht 800: link 1: \
	offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
 #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
	handle 1:0:10 u32 ht 1: \
	match tcp src 1024 ffff match tcp dst 80 ffff action drop

Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  3 +--
 drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 17 ++++-------------
 2 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 10ccd96..c520f98 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8348,8 +8348,7 @@ static int ixgbe_configure_clsu32(struct ixgbe_adapter *adapter,
 		int j;
 
 		for (j = 0; field_ptr[j].val; j++) {
-			if (field_ptr[j].off == off &&
-			    field_ptr[j].mask == m) {
+			if (field_ptr[j].off == off) {
 				field_ptr[j].val(input, &mask, val, m);
 				input->filter.formatted.flow_type |=
 					field_ptr[j].type;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
index ce48872..40d1730 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_model.h
@@ -65,28 +65,19 @@ static struct ixgbe_mat_field ixgbe_ipv4_fields[] = {
 	{ .val = NULL } /* terminal node */
 };
 
-static inline int ixgbe_mat_prgm_sport(struct ixgbe_fdir_filter *input,
+static inline int ixgbe_mat_prgm_ports(struct ixgbe_fdir_filter *input,
 				       union ixgbe_atr_input *mask,
 				       u32 val, u32 m)
 {
 	input->filter.formatted.src_port = val & 0xffff;
 	mask->formatted.src_port = m & 0xffff;
-	return 0;
-};
-
-static inline int ixgbe_mat_prgm_dport(struct ixgbe_fdir_filter *input,
-				       union ixgbe_atr_input *mask,
-				       u32 val, u32 m)
-{
-	input->filter.formatted.dst_port = val & 0xffff;
-	mask->formatted.dst_port = m & 0xffff;
+	input->filter.formatted.dst_port = val >> 16;
+	mask->formatted.dst_port = m  >> 16;
 	return 0;
 };
 
 static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
-	{.off = 0, .mask = 0xffff, .val = ixgbe_mat_prgm_sport,
-	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
-	{.off = 2, .mask = 0xffff, .val = ixgbe_mat_prgm_dport,
+	{.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
 	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
 	{ .val = NULL } /* terminal node */
 };
-- 
2.1.0

Re: [Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[John Fastabend]

On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
> Fix support for 16 bit source/dest port matches in ixgbe model.
> u32 uses a single 32-bit key value for both source and destination ports
> starting at offset 0. So replace the 2 functions with a single function
> that takes this key value/mask to program both source and dest ports.
> 
> Remove the incorrect check for mask in ixgbe_configure_clsu32()
> 
> Tested with the following filters:
> 
>  #tc qdisc add dev p4p1 ingress
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 800:0:1 u32 ht 800: \
> 	match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
> 
>  #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 800:0:1 u32
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 1: u32 divisor 1
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 800:0:10 u32 ht 800: link 1: \
> 	offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
> 	handle 1:0:10 u32 ht 1: \
> 	match tcp src 1024 ffff match tcp dst 80 ffff action drop
> 
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---

But this will break setting only dst port or only src port. Do we
actually need three signatures to match? Something like,

  static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
	{.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
  	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
	{.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
  	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
	{.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
  	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
  	{ .val = NULL } /* terminal node */
  };

Also just a reminder if we get multiple fields in a ixgbe_mat_field
struct we need to abort out of the for loop in the cls_u32 configure
function. Actually we can probably just push that as its own patch
to make the core function more versatile/usable.

Thanks,
John

Re: [Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Samudrala, Sridhar]

On 3/4/2016 12:41 PM, John Fastabend wrote:
> On 16-03-04 11:47 AM, Sridhar Samudrala wrote:
>> Fix support for 16 bit source/dest port matches in ixgbe model.
>> u32 uses a single 32-bit key value for both source and destination ports
>> starting at offset 0. So replace the 2 functions with a single function
>> that takes this key value/mask to program both source and dest ports.
>>
>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>
>> Tested with the following filters:
>>
>>   #tc qdisc add dev p4p1 ingress
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 800:0:1 u32 ht 800: \
>> 	match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>
>>   #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 800:0:1 u32
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 1: u32 divisor 1
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 800:0:10 u32 ht 800: link 1: \
>> 	offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6 ff
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>> 	handle 1:0:10 u32 ht 1: \
>> 	match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
> But this will break setting only dst port or only src port.

No. This will not break specifying only src or dst port. The value/mask for the
unspecified port will be set to zero. So it should be fine.

For ex:
	match tcp src 1024 ffff match tcp dst 80 ffff
		=> match 04000050/ffffffff at nexthdr+0
	match tcp src 1024 ffff
		=> match 04000000/ffff0000 at nexthdr+0
	match tcp dst 80 ffff
		=> match 00000050/0000ffff at nexthdr+0


>   Do we
> actually need three signatures to match? Something like,
>
>    static struct ixgbe_mat_field ixgbe_tcp_fields[] = {
> 	{.off = 0, .mask = 0xffffffff, .val = ixgbe_mat_prgm_ports,
>    	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
> 	{.off = 0, .mask = 0xffff0000, .val = ixgbe_mat_prgm_dport,
>    	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
> 	{.off = 0, .mask = 0x0000ffff, .val = ixgbe_mat_prgm_sport,
>    	 .type = IXGBE_ATR_FLOW_TYPE_TCPV4},
>    	{ .val = NULL } /* terminal node */
>    };
>
> Also just a reminder if we get multiple fields in a ixgbe_mat_field
> struct we need to abort out of the for loop in the cls_u32 configure
> function. Actually we can probably just push that as its own patch
> to make the core function more versatile/usable.
>
> Thanks,
> John

Re: [Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Jeff Kirsher]

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

On Fri, 2016-03-04 at 11:47 -0800, Sridhar Samudrala wrote:
> Fix support for 16 bit source/dest port matches in ixgbe model.
> u32 uses a single 32-bit key value for both source and destination
> ports
> starting at offset 0. So replace the 2 functions with a single
> function
> that takes this key value/mask to program both source and dest ports.
> 
> Remove the incorrect check for mask in ixgbe_configure_clsu32()
> 
> Tested with the following filters:
> 
>  #tc qdisc add dev p4p1 ingress
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 800:0:1 u32 ht 800: \
>         match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
> 
>  #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 800:0:1 u32
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 1: u32 divisor 1
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 800:0:10 u32 ht 800: link 1: \
>         offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6
> ff
>  #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>         handle 1:0:10 u32 ht 1: \
>         match tcp src 1024 ffff match tcp dst 80 ffff action drop
> 
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  3 +--
>  drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 17 ++++----------
> ---
>  2 files changed, 5 insertions(+), 15 deletions(-)

Is this v2?  If not, why are you re-sending a patch that is already in
my queue?  If so, where is the changelog so we know what changed in
this updated v2 of the patch?

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

Re: [Intel-wired-lan] [PATCH net-next] net: ixgbe: Fix cls_u32 offload support for ports and fields with masks.

[Samudrala, Sridhar]

On 3/4/2016 2:10 PM, Jeff Kirsher wrote:
> On Fri, 2016-03-04 at 11:47 -0800, Sridhar Samudrala wrote:
>> Fix support for 16 bit source/dest port matches in ixgbe model.
>> u32 uses a single 32-bit key value for both source and destination
>> ports
>> starting at offset 0. So replace the 2 functions with a single
>> function
>> that takes this key value/mask to program both source and dest ports.
>>
>> Remove the incorrect check for mask in ixgbe_configure_clsu32()
>>
>> Tested with the following filters:
>>
>>   #tc qdisc add dev p4p1 ingress
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 800:0:1 u32 ht 800: \
>>          match ip dst 11.0.0.1/24 match ip src 11.0.0.2/24 action drop
>>
>>   #tc filter del dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 800:0:1 u32
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 1: u32 divisor 1
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 800:0:10 u32 ht 800: link 1: \
>>          offset at 0 mask 0f00 shift 6 plus 0 eat match ip protocol 6
>> ff
>>   #tc filter add dev p4p1 parent ffff: protocol ip prio 99 \
>>          handle 1:0:10 u32 ht 1: \
>>          match tcp src 1024 ffff match tcp dst 80 ffff action drop
>>
>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |  3 +--
>>   drivers/net/ethernet/intel/ixgbe/ixgbe_model.h | 17 ++++----------
>> ---
>>   2 files changed, 5 insertions(+), 15 deletions(-)
> Is this v2?  If not, why are you re-sending a patch that is already in
> my queue?  If so, where is the changelog so we know what changed in
> this updated v2 of the patch?
No. This is another patch that fixes other issues on top of the previous 
one.

Thanks
Sridhar