[PATCH] net/tap: add support for fixed mac addresses

[PATCH] net/tap: add support for fixed mac addresses

[Keith Wiles]

Support for a fixed MAC address for testing with the last octet
incrementing by one for each interface defined with the new 'mac=fixed'
string on the --vdev option. The default option is still to randomize
the MAC address for each tap interface.

Signed-off-by: Keith Wiles <keith.wiles@intel.com>
---
 doc/guides/nics/tap.rst       | 13 +++++++++++-
 drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
index 5c5ba5357..e3819836a 100644
--- a/doc/guides/nics/tap.rst
+++ b/doc/guides/nics/tap.rst
@@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
 along with being able to be used as a network connection to the DPDK
 application. The method enable one or more interfaces is to use the
 ``--vdev=net_tap0`` option on the DPDK application command line. Each
-``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
+``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
 and so on.
 
 The interface name can be changed by adding the ``iface=foo0``, for example::
@@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
 
    --vdev=net_tap0,iface=foo0,speed=25000
 
+Normally the PMD will generate random MAC address, but when testing or with
+a static configurations the developer may need a fixed MAC address style.
+Using the option ``mac=fixed`` you can create a fixed known MAC address::
+
+   --vdev=net_tap0,mac=fixed
+
+The MAC address will be fixed value with the last octet incrementing by one
+each time for each interface string containing ``mac=fixed``. The MAC address
+is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
+and you get ``00:64:74:61:70:[00-FF]``.
+
 It is possible to specify a remote netdevice to capture packets from by adding
 ``remote=foo1``, for example::
 
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 347a80741..7a676c588 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -71,6 +71,13 @@
 #define ETH_TAP_IFACE_ARG       "iface"
 #define ETH_TAP_SPEED_ARG       "speed"
 #define ETH_TAP_REMOTE_ARG      "remote"
+#define ETH_TAP_MAC_ARG		"mac"
+
+#ifdef IFF_MULTI_QUEUE
+#define RTE_PMD_TAP_MAX_QUEUES	16
+#else
+#define RTE_PMD_TAP_MAX_QUEUES	1
+#endif
 
 #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
 #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
@@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
 	ETH_TAP_IFACE_ARG,
 	ETH_TAP_SPEED_ARG,
 	ETH_TAP_REMOTE_ARG,
+	ETH_TAP_MAC_ARG,
 	NULL
 };
 
 static int tap_unit;
+static int fixed_mac_type;
 
 static volatile uint32_t tap_trigger;	/* Rx trigger */
 
@@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
 			   ETHER_ADDR_LEN);
 	} else {
-		eth_random_addr((uint8_t *)&pmd->eth_addr);
+		if (fixed_mac_type) {
+			static int iface_idx;
+
+			pmd->eth_addr.addr_bytes[0] = 0x00;
+			pmd->eth_addr.addr_bytes[1] = 'd';
+			pmd->eth_addr.addr_bytes[2] = 't';
+			pmd->eth_addr.addr_bytes[3] = 'a';
+			pmd->eth_addr.addr_bytes[4] = 'p';
+			pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
+		} else
+			eth_random_addr((uint8_t *)&pmd->eth_addr);
 	}
 
 	return 0;
@@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
 	return 0;
 }
 
+static int
+set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
+{
+	/* Assume random mac address */
+	*(int *)extra_args = 0;
+	if (value && !strcasecmp("fixed", value))
+		*(int *)extra_args = 1;
+	return 0;
+}
+
 /* Open a TAP interface device.
  */
 static int
@@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
 		 DEFAULT_TAP_NAME, tap_unit++);
 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
 
+	fixed_mac_type = 0;
 	if (params && (params[0] != '\0')) {
 		RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
 
@@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
 				if (ret == -1)
 					goto leave;
 			}
+
+			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
+				ret = rte_kvargs_process(kvlist,
+							 ETH_TAP_MAC_ARG,
+							 &set_mac_type,
+							 &fixed_mac_type);
+				if (ret == -1)
+					goto leave;
+			}
 		}
 	}
 	pmd_link.link_speed = speed;
@@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
 };
 RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
 RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
-RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
+RTE_PMD_REGISTER_PARAM_STRING(net_tap,
+			      "iface=<string>,"
+			      "speed=N,"
+			      "remote=<string>,"
+			      "mac=fixed");
-- 
2.12.1.430.gafd672630

Re: [PATCH] net/tap: add support for fixed mac addresses

[Pascal Mazon]

Hi Keith,

I have a few comments on your patch, see inline.

On Mon, 10 Apr 2017 13:18:50 -0500
Keith Wiles <keith.wiles@intel.com> wrote:

> Support for a fixed MAC address for testing with the last octet
> incrementing by one for each interface defined with the new 'mac=fixed'
> string on the --vdev option. The default option is still to randomize
> the MAC address for each tap interface.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> ---
>  doc/guides/nics/tap.rst       | 13 +++++++++++-
>  drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
> index 5c5ba5357..e3819836a 100644
> --- a/doc/guides/nics/tap.rst
> +++ b/doc/guides/nics/tap.rst
> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
>  along with being able to be used as a network connection to the DPDK
>  application. The method enable one or more interfaces is to use the
>  ``--vdev=net_tap0`` option on the DPDK application command line. Each
> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
>  and so on.
>  
>  The interface name can be changed by adding the ``iface=foo0``, for example::
> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
>  
>     --vdev=net_tap0,iface=foo0,speed=25000
>  
> +Normally the PMD will generate random MAC address, but when testing or with

"random MAC" -> "a random MAC"

> +a static configurations the developer may need a fixed MAC address style.

"configurations" -> "configuration"

> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
> +
> +   --vdev=net_tap0,mac=fixed
> +
> +The MAC address will be fixed value with the last octet incrementing by one

"be" -> "have a"

> +each time for each interface string containing ``mac=fixed``. The MAC address

"each time" -> ""

> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex

" convert" -> ". Convert"

> +and you get ``00:64:74:61:70:[00-FF]``.
> +
>  It is possible to specify a remote netdevice to capture packets from by adding
>  ``remote=foo1``, for example::
>  
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 347a80741..7a676c588 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -1,7 +1,7 @@
>  /*-
>   *   BSD LICENSE
>   *
> - *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.

Shouldn't it be "2016-2017"?

>   *   All rights reserved.
>   *
>   *   Redistribution and use in source and binary forms, with or without
> @@ -71,6 +71,13 @@
>  #define ETH_TAP_IFACE_ARG       "iface"
>  #define ETH_TAP_SPEED_ARG       "speed"
>  #define ETH_TAP_REMOTE_ARG      "remote"
> +#define ETH_TAP_MAC_ARG		"mac"

You used tabs instead of spaces.

> +
> +#ifdef IFF_MULTI_QUEUE
> +#define RTE_PMD_TAP_MAX_QUEUES	16
> +#else
> +#define RTE_PMD_TAP_MAX_QUEUES	1
> +#endif

Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).

>  
>  #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
>  #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
>  	ETH_TAP_IFACE_ARG,
>  	ETH_TAP_SPEED_ARG,
>  	ETH_TAP_REMOTE_ARG,
> +	ETH_TAP_MAC_ARG,
>  	NULL
>  };
>  
>  static int tap_unit;
> +static int fixed_mac_type;

There is no need for a global variable, especially as the value should not be the same for each driver instance.
Typically when one tap uses "mac=fixed" and the next one doesn't.
More comments bellow for that.

>  
>  static volatile uint32_t tap_trigger;	/* Rx trigger */
>  
> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
>  		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
>  			   ETHER_ADDR_LEN);
>  	} else {
> -		eth_random_addr((uint8_t *)&pmd->eth_addr);
> +		if (fixed_mac_type) {
> +			static int iface_idx;
> +
> +			pmd->eth_addr.addr_bytes[0] = 0x00;
> +			pmd->eth_addr.addr_bytes[1] = 'd';
> +			pmd->eth_addr.addr_bytes[2] = 't';
> +			pmd->eth_addr.addr_bytes[3] = 'a';
> +			pmd->eth_addr.addr_bytes[4] = 'p';
> +			pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
> +		} else
> +			eth_random_addr((uint8_t *)&pmd->eth_addr);

To avoid checkpatch warning, use else { }.

>  	}
>  
>  	return 0;
> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
>  	return 0;
>  }
>  
> +static int
> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
> +{
> +	/* Assume random mac address */
> +	*(int *)extra_args = 0;

With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.

> +	if (value && !strcasecmp("fixed", value))

I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?

> +		*(int *)extra_args = 1;
> +	return 0;
> +}
> +
>  /* Open a TAP interface device.
>   */
>  static int
> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
>  		 DEFAULT_TAP_NAME, tap_unit++);
>  	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
>  
> +	fixed_mac_type = 0;

Turn fixed_mac_type to an automatic variable, local to this function.
And add the argument to eth_dev_tap_create().

>  	if (params && (params[0] != '\0')) {
>  		RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
>  
> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
>  				if (ret == -1)
>  					goto leave;
>  			}
> +
> +			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
> +				ret = rte_kvargs_process(kvlist,
> +							 ETH_TAP_MAC_ARG,
> +							 &set_mac_type,
> +							 &fixed_mac_type);
> +				if (ret == -1)
> +					goto leave;
> +			}
>  		}
>  	}
>  	pmd_link.link_speed = speed;
> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
>  };
>  RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
>  RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
> +			      "iface=<string>,"
> +			      "speed=N,"
> +			      "remote=<string>,"
> +			      "mac=fixed");

Indeed, I forgot to update that when I introduced the remote!

That's it for me,
Thank you.

Pascal

Re: [PATCH] net/tap: add support for fixed mac addresses

[Ferruh Yigit]

On 4/11/2017 8:18 AM, Pascal Mazon wrote:
> Hi Keith,
> 
> I have a few comments on your patch, see inline.
> 
> On Mon, 10 Apr 2017 13:18:50 -0500
> Keith Wiles <keith.wiles@intel.com> wrote:
> 
>> Support for a fixed MAC address for testing with the last octet
>> incrementing by one for each interface defined with the new 'mac=fixed'
>> string on the --vdev option. The default option is still to randomize
>> the MAC address for each tap interface.
>>
>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>

<...>

>>  RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
>>  RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
>> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>> +			      "iface=<string>,"
>> +			      "speed=N,"
>> +			      "remote=<string>,"
>> +			      "mac=fixed");
> 
> Indeed, I forgot to update that when I introduced the remote!

Hi Pascal,

If you can send a patch for this, it can be quickly merged, so it won't
have to be in this patch.

> 
> That's it for me,
> Thank you.
> 
> Pascal
> 

Re: [PATCH] net/tap: add support for fixed mac addresses

[Wiles, Keith]

> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> Hi Keith,
> 
> I have a few comments on your patch, see inline.
> 
> On Mon, 10 Apr 2017 13:18:50 -0500
> Keith Wiles <keith.wiles@intel.com> wrote:
> 
>> Support for a fixed MAC address for testing with the last octet
>> incrementing by one for each interface defined with the new 'mac=fixed'
>> string on the --vdev option. The default option is still to randomize
>> the MAC address for each tap interface.
>> 
>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>> ---
>> doc/guides/nics/tap.rst       | 13 +++++++++++-
>> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 58 insertions(+), 4 deletions(-)
>> 
>> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
>> index 5c5ba5357..e3819836a 100644
>> --- a/doc/guides/nics/tap.rst
>> +++ b/doc/guides/nics/tap.rst
>> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
>> along with being able to be used as a network connection to the DPDK
>> application. The method enable one or more interfaces is to use the
>> ``--vdev=net_tap0`` option on the DPDK application command line. Each
>> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
>> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
>> and so on.
>> 
>> The interface name can be changed by adding the ``iface=foo0``, for example::
>> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
>> 
>>    --vdev=net_tap0,iface=foo0,speed=25000
>> 
>> +Normally the PMD will generate random MAC address, but when testing or with
> 
> "random MAC" -> "a random MAC"
> 
>> +a static configurations the developer may need a fixed MAC address style.
> 
> "configurations" -> "configuration"
> 
>> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
>> +
>> +   --vdev=net_tap0,mac=fixed
>> +
>> +The MAC address will be fixed value with the last octet incrementing by one
> 
> "be" -> "have a"
> 
>> +each time for each interface string containing ``mac=fixed``. The MAC address
> 
> "each time" -> ""
> 
>> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
> 
> " convert" -> ". Convert"
> 
>> +and you get ``00:64:74:61:70:[00-FF]``.
>> +
>> It is possible to specify a remote netdevice to capture packets from by adding
>> ``remote=foo1``, for example::
>> 
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 347a80741..7a676c588 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -1,7 +1,7 @@
>> /*-
>>  *   BSD LICENSE
>>  *
>> - *   Copyright(c) 2016 Intel Corporation. All rights reserved.
>> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> 
> Shouldn't it be "2016-2017"?
> 
>>  *   All rights reserved.
>>  *
>>  *   Redistribution and use in source and binary forms, with or without
>> @@ -71,6 +71,13 @@
>> #define ETH_TAP_IFACE_ARG       "iface"
>> #define ETH_TAP_SPEED_ARG       "speed"
>> #define ETH_TAP_REMOTE_ARG      "remote"
>> +#define ETH_TAP_MAC_ARG		"mac"
> 
> You used tabs instead of spaces.
> 
>> +
>> +#ifdef IFF_MULTI_QUEUE
>> +#define RTE_PMD_TAP_MAX_QUEUES	16
>> +#else
>> +#define RTE_PMD_TAP_MAX_QUEUES	1
>> +#endif
> 
> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).

This should have been removed in your patch and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?

I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.

> 
>> 
>> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
>> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
>> 	ETH_TAP_IFACE_ARG,
>> 	ETH_TAP_SPEED_ARG,
>> 	ETH_TAP_REMOTE_ARG,
>> +	ETH_TAP_MAC_ARG,
>> 	NULL
>> };
>> 
>> static int tap_unit;
>> +static int fixed_mac_type;
> 
> There is no need for a global variable, especially as the value should not be the same for each driver instance.
> Typically when one tap uses "mac=fixed" and the next one doesn't.
> More comments bellow for that.
> 
>> 
>> static volatile uint32_t tap_trigger;	/* Rx trigger */
>> 
>> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
>> 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
>> 			   ETHER_ADDR_LEN);
>> 	} else {
>> -		eth_random_addr((uint8_t *)&pmd->eth_addr);
>> +		if (fixed_mac_type) {
>> +			static int iface_idx;
>> +
>> +			pmd->eth_addr.addr_bytes[0] = 0x00;
>> +			pmd->eth_addr.addr_bytes[1] = 'd';
>> +			pmd->eth_addr.addr_bytes[2] = 't';
>> +			pmd->eth_addr.addr_bytes[3] = 'a';
>> +			pmd->eth_addr.addr_bytes[4] = 'p';
>> +			pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
>> +		} else
>> +			eth_random_addr((uint8_t *)&pmd->eth_addr);
> 
> To avoid checkpatch warning, use else { }.

Ferruh, states this is OK and does not need to have the { } added. I will not add the { } here.

> 
>> 	}
>> 
>> 	return 0;
>> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
>> 	return 0;
>> }
>> 
>> +static int
>> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
>> +{
>> +	/* Assume random mac address */
>> +	*(int *)extra_args = 0;
> 
> With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.

It does need to be set of each instance of the call to this routine even if I remove the global. Each interface can have a different state.

> 
>> +	if (value && !strcasecmp("fixed", value))
> 
> I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?

I will use the already existing macro ETH_TAP_MAC_ARG instead.
> 
>> +		*(int *)extra_args = 1;
>> +	return 0;
>> +}
>> +
>> /* Open a TAP interface device.
>>  */
>> static int
>> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
>> 		 DEFAULT_TAP_NAME, tap_unit++);
>> 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
>> 
>> +	fixed_mac_type = 0;
> 
> Turn fixed_mac_type to an automatic variable, local to this function.
> And add the argument to eth_dev_tap_create().

Need to have the global to exist, because I need to be able to test the variable later. The variable could be placed in a allocated structure, but does it really matter to have a static variable here?

> 
>> 	if (params && (params[0] != '\0')) {
>> 		RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
>> 
>> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
>> 				if (ret == -1)
>> 					goto leave;
>> 			}
>> +
>> +			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
>> +				ret = rte_kvargs_process(kvlist,
>> +							 ETH_TAP_MAC_ARG,
>> +							 &set_mac_type,
>> +							 &fixed_mac_type);
>> +				if (ret == -1)
>> +					goto leave;
>> +			}
>> 		}
>> 	}
>> 	pmd_link.link_speed = speed;
>> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
>> };
>> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
>> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
>> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>> +			      "iface=<string>,"
>> +			      "speed=N,"
>> +			      "remote=<string>,"
>> +			      "mac=fixed");
> 
> Indeed, I forgot to update that when I introduced the remote!
> 
> That's it for me,
> Thank you.
> 
> Pascal

Regards,
Keith

Re: [PATCH] net/tap: add support for fixed mac addresses

[Wiles, Keith]

> On Apr 11, 2017, at 8:23 AM, Wiles, Keith <keith.wiles@intel.com> wrote:
> 
> 
>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>> 
>> Hi Keith,
>> 
>> I have a few comments on your patch, see inline.
>> 
>> On Mon, 10 Apr 2017 13:18:50 -0500
>> Keith Wiles <keith.wiles@intel.com> wrote:
>> 
>>> Support for a fixed MAC address for testing with the last octet
>>> incrementing by one for each interface defined with the new 'mac=fixed'
>>> string on the --vdev option. The default option is still to randomize
>>> the MAC address for each tap interface.
>>> 
>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>>> ---
>>> doc/guides/nics/tap.rst       | 13 +++++++++++-
>>> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
>>> 2 files changed, 58 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
>>> index 5c5ba5357..e3819836a 100644
>>> --- a/doc/guides/nics/tap.rst
>>> +++ b/doc/guides/nics/tap.rst
>>> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
>>> along with being able to be used as a network connection to the DPDK
>>> application. The method enable one or more interfaces is to use the
>>> ``--vdev=net_tap0`` option on the DPDK application command line. Each
>>> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
>>> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
>>> and so on.
>>> 
>>> The interface name can be changed by adding the ``iface=foo0``, for example::
>>> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
>>> 
>>>   --vdev=net_tap0,iface=foo0,speed=25000
>>> 
>>> +Normally the PMD will generate random MAC address, but when testing or with
>> 
>> "random MAC" -> "a random MAC"
>> 
>>> +a static configurations the developer may need a fixed MAC address style.
>> 
>> "configurations" -> "configuration"
>> 
>>> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
>>> +
>>> +   --vdev=net_tap0,mac=fixed
>>> +
>>> +The MAC address will be fixed value with the last octet incrementing by one
>> 
>> "be" -> "have a"
>> 
>>> +each time for each interface string containing ``mac=fixed``. The MAC address
>> 
>> "each time" -> ""
>> 
>>> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
>> 
>> " convert" -> ". Convert"
>> 
>>> +and you get ``00:64:74:61:70:[00-FF]``.
>>> +
>>> It is possible to specify a remote netdevice to capture packets from by adding
>>> ``remote=foo1``, for example::
>>> 
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 347a80741..7a676c588 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -1,7 +1,7 @@
>>> /*-
>>> *   BSD LICENSE
>>> *
>>> - *   Copyright(c) 2016 Intel Corporation. All rights reserved.
>>> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
>> 
>> Shouldn't it be "2016-2017"?
>> 
>>> *   All rights reserved.
>>> *
>>> *   Redistribution and use in source and binary forms, with or without
>>> @@ -71,6 +71,13 @@
>>> #define ETH_TAP_IFACE_ARG       "iface"
>>> #define ETH_TAP_SPEED_ARG       "speed"
>>> #define ETH_TAP_REMOTE_ARG      "remote"
>>> +#define ETH_TAP_MAC_ARG		"mac"
>> 
>> You used tabs instead of spaces.
>> 
>>> +
>>> +#ifdef IFF_MULTI_QUEUE
>>> +#define RTE_PMD_TAP_MAX_QUEUES	16
>>> +#else
>>> +#define RTE_PMD_TAP_MAX_QUEUES	1
>>> +#endif
>> 
>> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
> 
> This should have been removed in your patch and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
> 
> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.
> 
>> 
>>> 
>>> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
>>> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>>> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
>>> 	ETH_TAP_IFACE_ARG,
>>> 	ETH_TAP_SPEED_ARG,
>>> 	ETH_TAP_REMOTE_ARG,
>>> +	ETH_TAP_MAC_ARG,
>>> 	NULL
>>> };
>>> 
>>> static int tap_unit;
>>> +static int fixed_mac_type;
>> 
>> There is no need for a global variable, especially as the value should not be the same for each driver instance.
>> Typically when one tap uses "mac=fixed" and the next one doesn't.
>> More comments bellow for that.
>> 
>>> 
>>> static volatile uint32_t tap_trigger;	/* Rx trigger */
>>> 
>>> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
>>> 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
>>> 			   ETHER_ADDR_LEN);
>>> 	} else {
>>> -		eth_random_addr((uint8_t *)&pmd->eth_addr);
>>> +		if (fixed_mac_type) {
>>> +			static int iface_idx;
>>> +
>>> +			pmd->eth_addr.addr_bytes[0] = 0x00;
>>> +			pmd->eth_addr.addr_bytes[1] = 'd';
>>> +			pmd->eth_addr.addr_bytes[2] = 't';
>>> +			pmd->eth_addr.addr_bytes[3] = 'a';
>>> +			pmd->eth_addr.addr_bytes[4] = 'p';
>>> +			pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
>>> +		} else
>>> +			eth_random_addr((uint8_t *)&pmd->eth_addr);
>> 
>> To avoid checkpatch warning, use else { }.
> 
> Ferruh, states this is OK and does not need to have the { } added. I will not add the { } here.
> 
>> 
>>> 	}
>>> 
>>> 	return 0;
>>> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
>>> 	return 0;
>>> }
>>> 
>>> +static int
>>> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
>>> +{
>>> +	/* Assume random mac address */
>>> +	*(int *)extra_args = 0;
>> 
>> With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.
> 
> It does need to be set of each instance of the call to this routine even if I remove the global. Each interface can have a different state.
> 
>> 
>>> +	if (value && !strcasecmp("fixed", value))
>> 
>> I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?
> 
> I will use the already existing macro ETH_TAP_MAC_ARG instead.

Opps! I will need a macros for this one.

>> 
>>> +		*(int *)extra_args = 1;
>>> +	return 0;
>>> +}
>>> +
>>> /* Open a TAP interface device.
>>> */
>>> static int
>>> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
>>> 		 DEFAULT_TAP_NAME, tap_unit++);
>>> 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
>>> 
>>> +	fixed_mac_type = 0;
>> 
>> Turn fixed_mac_type to an automatic variable, local to this function.
>> And add the argument to eth_dev_tap_create().
> 
> Need to have the global to exist, because I need to be able to test the variable later. The variable could be placed in a allocated structure, but does it really matter to have a static variable here?
> 
>> 
>>> 	if (params && (params[0] != '\0')) {
>>> 		RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
>>> 
>>> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
>>> 				if (ret == -1)
>>> 					goto leave;
>>> 			}
>>> +
>>> +			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
>>> +				ret = rte_kvargs_process(kvlist,
>>> +							 ETH_TAP_MAC_ARG,
>>> +							 &set_mac_type,
>>> +							 &fixed_mac_type);
>>> +				if (ret == -1)
>>> +					goto leave;
>>> +			}
>>> 		}
>>> 	}
>>> 	pmd_link.link_speed = speed;
>>> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
>>> };
>>> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
>>> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
>>> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>>> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>>> +			      "iface=<string>,"
>>> +			      "speed=N,"
>>> +			      "remote=<string>,"
>>> +			      "mac=fixed");
>> 
>> Indeed, I forgot to update that when I introduced the remote!
>> 
>> That's it for me,
>> Thank you.
>> 
>> Pascal
> 
> Regards,
> Keith
> 

Regards,
Keith

Re: [PATCH] net/tap: add support for fixed mac addresses

[Ferruh Yigit]

On 4/11/2017 2:23 PM, Wiles, Keith wrote:
> 
>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> Hi Keith,
>>
>> I have a few comments on your patch, see inline.
>>
>> On Mon, 10 Apr 2017 13:18:50 -0500
>> Keith Wiles <keith.wiles@intel.com> wrote:
>>
>>> Support for a fixed MAC address for testing with the last octet
>>> incrementing by one for each interface defined with the new 'mac=fixed'
>>> string on the --vdev option. The default option is still to randomize
>>> the MAC address for each tap interface.
>>>
>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>

<...>

>>> +
>>> +#ifdef IFF_MULTI_QUEUE
>>> +#define RTE_PMD_TAP_MAX_QUEUES	16
>>> +#else
>>> +#define RTE_PMD_TAP_MAX_QUEUES	1
>>> +#endif
>>
>> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
> 
> This should have been removed in your patch 

That is already done, and patch is in main repo:
Fixes: 947d949de7de ("net/tap: fix max queues redefinition")

> and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?

Pascal already send the patch and it is in next-net now:
Fixes: f0e5085f4677 ("net/tap: update driver param string")

> 
> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.

There is no more updated expected, using latest next-net should be OK to
submit your patch.

Thanks,
ferruh

Re: [PATCH] net/tap: add support for fixed mac addresses

[Wiles, Keith]

> On Apr 11, 2017, at 8:39 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
> 
> On 4/11/2017 2:23 PM, Wiles, Keith wrote:
>> 
>>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>> 
>>> Hi Keith,
>>> 
>>> I have a few comments on your patch, see inline.
>>> 
>>> On Mon, 10 Apr 2017 13:18:50 -0500
>>> Keith Wiles <keith.wiles@intel.com> wrote:
>>> 
>>>> Support for a fixed MAC address for testing with the last octet
>>>> incrementing by one for each interface defined with the new 'mac=fixed'
>>>> string on the --vdev option. The default option is still to randomize
>>>> the MAC address for each tap interface.
>>>> 
>>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> 
> <...>
> 
>>>> +
>>>> +#ifdef IFF_MULTI_QUEUE
>>>> +#define RTE_PMD_TAP_MAX_QUEUES	16
>>>> +#else
>>>> +#define RTE_PMD_TAP_MAX_QUEUES	1
>>>> +#endif
>>> 
>>> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
>> 
>> This should have been removed in your patch 
> 
> That is already done, and patch is in main repo:
> Fixes: 947d949de7de ("net/tap: fix max queues redefinition")
> 
>> and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
> 
> Pascal already send the patch and it is in next-net now:
> Fixes: f0e5085f4677 ("net/tap: update driver param string")
> 
>> 
>> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.
> 
> There is no more updated expected, using latest next-net should be OK to
> submit your patch.

OK

> 
> Thanks,
> ferruh

Regards,
Keith

Re: [PATCH] net/tap: add support for fixed mac addresses

[Pascal Mazon]

On Tue, 11 Apr 2017 13:23:24 +0000
"Wiles, Keith" <keith.wiles@intel.com> wrote:

> 
> > On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> > 
> > Hi Keith,
> > 
> > I have a few comments on your patch, see inline.
> > 
> > On Mon, 10 Apr 2017 13:18:50 -0500
> > Keith Wiles <keith.wiles@intel.com> wrote:
> > 
> >> Support for a fixed MAC address for testing with the last octet
> >> incrementing by one for each interface defined with the new 'mac=fixed'
> >> string on the --vdev option. The default option is still to randomize
> >> the MAC address for each tap interface.
> >> 
> >> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> >> ---
> >> doc/guides/nics/tap.rst       | 13 +++++++++++-
> >> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
> >> 2 files changed, 58 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
> >> index 5c5ba5357..e3819836a 100644
> >> --- a/doc/guides/nics/tap.rst
> >> +++ b/doc/guides/nics/tap.rst
> >> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
> >> along with being able to be used as a network connection to the DPDK
> >> application. The method enable one or more interfaces is to use the
> >> ``--vdev=net_tap0`` option on the DPDK application command line. Each
> >> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
> >> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
> >> and so on.
> >> 
> >> The interface name can be changed by adding the ``iface=foo0``, for example::
> >> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
> >> 
> >>    --vdev=net_tap0,iface=foo0,speed=25000
> >> 
> >> +Normally the PMD will generate random MAC address, but when testing or with
> > 
> > "random MAC" -> "a random MAC"
> > 
> >> +a static configurations the developer may need a fixed MAC address style.
> > 
> > "configurations" -> "configuration"
> > 
> >> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
> >> +
> >> +   --vdev=net_tap0,mac=fixed
> >> +
> >> +The MAC address will be fixed value with the last octet incrementing by one
> > 
> > "be" -> "have a"
> > 
> >> +each time for each interface string containing ``mac=fixed``. The MAC address
> > 
> > "each time" -> ""
> > 
> >> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
> > 
> > " convert" -> ". Convert"
> > 
> >> +and you get ``00:64:74:61:70:[00-FF]``.
> >> +
> >> It is possible to specify a remote netdevice to capture packets from by adding
> >> ``remote=foo1``, for example::
> >> 
> >> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> >> index 347a80741..7a676c588 100644
> >> --- a/drivers/net/tap/rte_eth_tap.c
> >> +++ b/drivers/net/tap/rte_eth_tap.c
> >> @@ -1,7 +1,7 @@
> >> /*-
> >>  *   BSD LICENSE
> >>  *
> >> - *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> >> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
> > 
> > Shouldn't it be "2016-2017"?
> > 
> >>  *   All rights reserved.
> >>  *
> >>  *   Redistribution and use in source and binary forms, with or without
> >> @@ -71,6 +71,13 @@
> >> #define ETH_TAP_IFACE_ARG       "iface"
> >> #define ETH_TAP_SPEED_ARG       "speed"
> >> #define ETH_TAP_REMOTE_ARG      "remote"
> >> +#define ETH_TAP_MAC_ARG		"mac"
> > 
> > You used tabs instead of spaces.
> > 
> >> +
> >> +#ifdef IFF_MULTI_QUEUE
> >> +#define RTE_PMD_TAP_MAX_QUEUES	16
> >> +#else
> >> +#define RTE_PMD_TAP_MAX_QUEUES	1
> >> +#endif
> > 
> > Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
> 
> This should have been removed in your patch and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
> 
> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.

Ferruh details how it's been integrated in next-net, nothing to add there :)

> 
> > 
> >> 
> >> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
> >> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
> >> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
> >> 	ETH_TAP_IFACE_ARG,
> >> 	ETH_TAP_SPEED_ARG,
> >> 	ETH_TAP_REMOTE_ARG,
> >> +	ETH_TAP_MAC_ARG,
> >> 	NULL
> >> };
> >> 
> >> static int tap_unit;
> >> +static int fixed_mac_type;
> > 
> > There is no need for a global variable, especially as the value should not be the same for each driver instance.
> > Typically when one tap uses "mac=fixed" and the next one doesn't.
> > More comments bellow for that.
> > 
> >> 
> >> static volatile uint32_t tap_trigger;	/* Rx trigger */
> >> 
> >> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
> >> 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
> >> 			   ETHER_ADDR_LEN);
> >> 	} else {
> >> -		eth_random_addr((uint8_t *)&pmd->eth_addr);
> >> +		if (fixed_mac_type) {
> >> +			static int iface_idx;

I'd suggest using something like this, as there's no need for so much lines to do the same:

char mac[ETHER_ADDR_LEN] = "\x00\x64\x74\x61\x70\x00"; /* 0:d:t:a:p:0 */

mac[ETHER_ADDR_LEN-1] = iface_idx++;
rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);


> >> +
> >> +			pmd->eth_addr.addr_bytes[0] = 0x00;
> >> +			pmd->eth_addr.addr_bytes[1] = 'd';
> >> +			pmd->eth_addr.addr_bytes[2] = 't';
> >> +			pmd->eth_addr.addr_bytes[3] = 'a';
> >> +			pmd->eth_addr.addr_bytes[4] = 'p';
> >> +			pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
> >> +		} else
> >> +			eth_random_addr((uint8_t *)&pmd->eth_addr);
> > 
> > To avoid checkpatch warning, use else { }.
> 
> Ferruh, states this is OK and does not need to have the { } added. I will not add the { } here.

Very well!

> 
> > 
> >> 	}
> >> 
> >> 	return 0;
> >> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
> >> 	return 0;
> >> }
> >> 
> >> +static int
> >> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
> >> +{
> >> +	/* Assume random mac address */
> >> +	*(int *)extra_args = 0;
> > 
> > With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.
> 
> It does need to be set of each instance of the call to this routine even if I remove the global. Each interface can have a different state.
> 
> > 
> >> +	if (value && !strcasecmp("fixed", value))
> > 
> > I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?
> 
> I will use the already existing macro ETH_TAP_MAC_ARG instead.
> > 
> >> +		*(int *)extra_args = 1;
> >> +	return 0;
> >> +}
> >> +
> >> /* Open a TAP interface device.
> >>  */
> >> static int
> >> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
> >> 		 DEFAULT_TAP_NAME, tap_unit++);
> >> 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
> >> 
> >> +	fixed_mac_type = 0;
> > 
> > Turn fixed_mac_type to an automatic variable, local to this function.
> > And add the argument to eth_dev_tap_create().
> 
> Need to have the global to exist, because I need to be able to test the variable later. The variable could be placed in a allocated structure, but does it really matter to have a static variable here?

I disagree here, I've seen fixed_mac_type only used during probing, in rte_pmd_tap_probe() and rte_eth_dev_tap_create().
The case is similar to the "iface" param, we need it during probing, where it is used appropriately (either setting the pmd->eth_addr for your fixed mac, or setting pmd->remote_iface for iface).

> 
> > 
> >> 	if (params && (params[0] != '\0')) {
> >> 		RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
> >> 
> >> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
> >> 				if (ret == -1)
> >> 					goto leave;
> >> 			}
> >> +
> >> +			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
> >> +				ret = rte_kvargs_process(kvlist,
> >> +							 ETH_TAP_MAC_ARG,
> >> +							 &set_mac_type,
> >> +							 &fixed_mac_type);
> >> +				if (ret == -1)
> >> +					goto leave;
> >> +			}
> >> 		}
> >> 	}
> >> 	pmd_link.link_speed = speed;
> >> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
> >> };
> >> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
> >> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
> >> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
> >> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
> >> +			      "iface=<string>,"
> >> +			      "speed=N,"
> >> +			      "remote=<string>,"
> >> +			      "mac=fixed");
> > 
> > Indeed, I forgot to update that when I introduced the remote!
> > 
> > That's it for me,
> > Thank you.
> > 
> > Pascal
> 
> Regards,
> Keith
> 

Re: [PATCH] net/tap: add support for fixed mac addresses

[Wiles, Keith]

> On Apr 11, 2017, at 8:54 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> On Tue, 11 Apr 2017 13:23:24 +0000
> "Wiles, Keith" <keith.wiles@intel.com> wrote:
> 
>> 
>>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>> 
>>> Hi Keith,
>>> 
>>> I have a few comments on your patch, see inline.
>>> 
>>> On Mon, 10 Apr 2017 13:18:50 -0500
>>> Keith Wiles <keith.wiles@intel.com> wrote:
>>> 
>>>> Support for a fixed MAC address for testing with the last octet
>>>> incrementing by one for each interface defined with the new 'mac=fixed'
>>>> string on the --vdev option. The default option is still to randomize
>>>> the MAC address for each tap interface.
>>>> 
>>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>>>> ---
>>>> doc/guides/nics/tap.rst       | 13 +++++++++++-
>>>> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
>>>> 2 files changed, 58 insertions(+), 4 deletions(-)
>>>> 
>>>> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
>>>> index 5c5ba5357..e3819836a 100644
>>>> --- a/doc/guides/nics/tap.rst
>>>> +++ b/doc/guides/nics/tap.rst
>>>> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
>>>> along with being able to be used as a network connection to the DPDK
>>>> application. The method enable one or more interfaces is to use the
>>>> ``--vdev=net_tap0`` option on the DPDK application command line. Each
>>>> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
>>>> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
>>>> and so on.
>>>> 
>>>> The interface name can be changed by adding the ``iface=foo0``, for example::
>>>> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
>>>> 
>>>>   --vdev=net_tap0,iface=foo0,speed=25000
>>>> 
>>>> +Normally the PMD will generate random MAC address, but when testing or with
>>> 
>>> "random MAC" -> "a random MAC"
>>> 
>>>> +a static configurations the developer may need a fixed MAC address style.
>>> 
>>> "configurations" -> "configuration"
>>> 
>>>> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
>>>> +
>>>> +   --vdev=net_tap0,mac=fixed
>>>> +
>>>> +The MAC address will be fixed value with the last octet incrementing by one
>>> 
>>> "be" -> "have a"
>>> 
>>>> +each time for each interface string containing ``mac=fixed``. The MAC address
>>> 
>>> "each time" -> ""
>>> 
>>>> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
>>> 
>>> " convert" -> ". Convert"
>>> 
>>>> +and you get ``00:64:74:61:70:[00-FF]``.
>>>> +
>>>> It is possible to specify a remote netdevice to capture packets from by adding
>>>> ``remote=foo1``, for example::
>>>> 
>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>> index 347a80741..7a676c588 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>> @@ -1,7 +1,7 @@
>>>> /*-
>>>> *   BSD LICENSE
>>>> *
>>>> - *   Copyright(c) 2016 Intel Corporation. All rights reserved.
>>>> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
>>> 
>>> Shouldn't it be "2016-2017"?
>>> 
>>>> *   All rights reserved.
>>>> *
>>>> *   Redistribution and use in source and binary forms, with or without
>>>> @@ -71,6 +71,13 @@
>>>> #define ETH_TAP_IFACE_ARG       "iface"
>>>> #define ETH_TAP_SPEED_ARG       "speed"
>>>> #define ETH_TAP_REMOTE_ARG      "remote"
>>>> +#define ETH_TAP_MAC_ARG		"mac"
>>> 
>>> You used tabs instead of spaces.
>>> 
>>>> +
>>>> +#ifdef IFF_MULTI_QUEUE
>>>> +#define RTE_PMD_TAP_MAX_QUEUES	16
>>>> +#else
>>>> +#define RTE_PMD_TAP_MAX_QUEUES	1
>>>> +#endif
>>> 
>>> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
>> 
>> This should have been removed in your patch and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
>> 
>> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.
> 
> Ferruh details how it's been integrated in next-net, nothing to add there :)
> 
>> 
>>> 
>>>> 
>>>> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
>>>> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>>>> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
>>>> 	ETH_TAP_IFACE_ARG,
>>>> 	ETH_TAP_SPEED_ARG,
>>>> 	ETH_TAP_REMOTE_ARG,
>>>> +	ETH_TAP_MAC_ARG,
>>>> 	NULL
>>>> };
>>>> 
>>>> static int tap_unit;
>>>> +static int fixed_mac_type;
>>> 
>>> There is no need for a global variable, especially as the value should not be the same for each driver instance.
>>> Typically when one tap uses "mac=fixed" and the next one doesn't.
>>> More comments bellow for that.
>>> 
>>>> 
>>>> static volatile uint32_t tap_trigger;	/* Rx trigger */
>>>> 
>>>> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
>>>> 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
>>>> 			   ETHER_ADDR_LEN);
>>>> 	} else {
>>>> -		eth_random_addr((uint8_t *)&pmd->eth_addr);
>>>> +		if (fixed_mac_type) {
>>>> +			static int iface_idx;
> 
> I'd suggest using something like this, as there's no need for so much lines to do the same:
> 
> char mac[ETHER_ADDR_LEN] = "\x00\x64\x74\x61\x70\x00"; /* 0:d:t:a:p:0 */
> 
> mac[ETHER_ADDR_LEN-1] = iface_idx++;
> rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);

Here is the above code:
			static int iface_idx;
			char mac[ETHER_ADDR_LEN] =
				"\x00\x64\x74\x61\x70\x00"; /* 0:d:t:a:p:0 */
			mac[ETHER_ADDR_LEN - 1] = iface_idx++;
			rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);

Not much in the way of fewer lines and not really that readable, so your objection here is only that you would have coded it differently not that it is incorrect, right? Personally I like the code below as it is easier to ready IMHO, but I will covert it your suggestion.

> 
> 
>>>> +
>>>> +			pmd->eth_addr.addr_bytes[0] = 0x00;
>>>> +			pmd->eth_addr.addr_bytes[1] = 'd';
>>>> +			pmd->eth_addr.addr_bytes[2] = 't';
>>>> +			pmd->eth_addr.addr_bytes[3] = 'a';
>>>> +			pmd->eth_addr.addr_bytes[4] = 'p';
>>>> +			pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
>>>> +		} else
>>>> +			eth_random_addr((uint8_t *)&pmd->eth_addr);
>>> 
>>> To avoid checkpatch warning, use else { }.
>> 
>> Ferruh, states this is OK and does not need to have the { } added. I will not add the { } here.
> 
> Very well!
> 
>> 
>>> 
>>>> 	}
>>>> 
>>>> 	return 0;
>>>> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
>>>> 	return 0;
>>>> }
>>>> 
>>>> +static int
>>>> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
>>>> +{
>>>> +	/* Assume random mac address */
>>>> +	*(int *)extra_args = 0;
>>> 
>>> With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.
>> 
>> It does need to be set of each instance of the call to this routine even if I remove the global. Each interface can have a different state.
>> 
>>> 
>>>> +	if (value && !strcasecmp("fixed", value))
>>> 
>>> I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?
>> 
>> I will use the already existing macro ETH_TAP_MAC_ARG instead.
>>> 
>>>> +		*(int *)extra_args = 1;
>>>> +	return 0;
>>>> +}
>>>> +
>>>> /* Open a TAP interface device.
>>>> */
>>>> static int
>>>> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
>>>> 		 DEFAULT_TAP_NAME, tap_unit++);
>>>> 	memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
>>>> 
>>>> +	fixed_mac_type = 0;
>>> 
>>> Turn fixed_mac_type to an automatic variable, local to this function.
>>> And add the argument to eth_dev_tap_create().
>> 
>> Need to have the global to exist, because I need to be able to test the variable later. The variable could be placed in a allocated structure, but does it really matter to have a static variable here?
> 
> I disagree here, I've seen fixed_mac_type only used during probing, in rte_pmd_tap_probe() and rte_eth_dev_tap_create().
> The case is similar to the "iface" param, we need it during probing, where it is used appropriately (either setting the pmd->eth_addr for your fixed mac, or setting pmd->remote_iface for iface).

This code is not really close to “iface” code as the iface code has a place to put the string in the pmd structure and the pmd->eth_addr is setup later on in the code. I would have to add a holding place in the pmd structure to hold the random or fixed mac address or flag to be used in the routine eth_dev_tap_create().

Maybe I am just being slow this morning, but I do not see how you want the code to be done here as I do not see away around a flag someplace.

> 
>> 
>>> 
>>>> 	if (params && (params[0] != '\0')) {
>>>> 		RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
>>>> 
>>>> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
>>>> 				if (ret == -1)
>>>> 					goto leave;
>>>> 			}
>>>> +
>>>> +			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
>>>> +				ret = rte_kvargs_process(kvlist,
>>>> +							 ETH_TAP_MAC_ARG,
>>>> +							 &set_mac_type,
>>>> +							 &fixed_mac_type);
>>>> +				if (ret == -1)
>>>> +					goto leave;
>>>> +			}
>>>> 		}
>>>> 	}
>>>> 	pmd_link.link_speed = speed;
>>>> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
>>>> };
>>>> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
>>>> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
>>>> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>>>> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>>>> +			      "iface=<string>,"
>>>> +			      "speed=N,"
>>>> +			      "remote=<string>,"
>>>> +			      "mac=fixed");
>>> 
>>> Indeed, I forgot to update that when I introduced the remote!
>>> 
>>> That's it for me,
>>> Thank you.
>>> 
>>> Pascal
>> 
>> Regards,
>> Keith

Regards,
Keith

[PATCH] net/tap: add support for fixed mac addresses

[Pascal Mazon]

Support for a fixed MAC address for testing with the last octet
incrementing by one for each interface defined with the new 'mac=fixed'
string on the --vdev option. The default option is still to randomize
the MAC address for each tap interface.

Signed-off-by: Keith Wiles <keith.wiles@intel.com>
Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
---

Hi Keith, I've made the changes I suggested.

It does as expected use a mac incremented by 1 for each new tap device,
without a global variable for fixed_mac_type.

With the more clean "\0dtap" initialization for mac[], I do think it
looks better, don't you?
I agree my version with \x did little improvement to yours, and would
be a question of style.
What do you say of this patch?

 doc/guides/nics/tap.rst       | 13 ++++++++++++-
 drivers/net/tap/rte_eth_tap.c | 39 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
index 5c5ba5357bba..f3ee95d2897b 100644
--- a/doc/guides/nics/tap.rst
+++ b/doc/guides/nics/tap.rst
@@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
 along with being able to be used as a network connection to the DPDK
 application. The method enable one or more interfaces is to use the
 ``--vdev=net_tap0`` option on the DPDK application command line. Each
-``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
+``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
 and so on.
 
 The interface name can be changed by adding the ``iface=foo0``, for example::
@@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
 
    --vdev=net_tap0,iface=foo0,speed=25000
 
+Normally the PMD will generate a random MAC address, but when testing or with
+a static configuration the developer may need a fixed MAC address style.
+Using the option ``mac=fixed`` you can create a fixed known MAC address::
+
+   --vdev=net_tap0,mac=fixed
+
+The MAC address will have a fixed value with the last octet incrementing by one
+for each interface string containing ``mac=fixed``. The MAC address is formatted
+as 00:'d':'t':'a':'p':[00-FF]. Convert the characters to hex and you get the
+actual MAC address: ``00:64:74:61:70:[00-FF]``.
+
 It is possible to specify a remote netdevice to capture packets from by adding
 ``remote=foo1``, for example::
 
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index cd82a7e09c75..2092f5d4184b 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2016-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -71,6 +71,8 @@
 #define ETH_TAP_IFACE_ARG       "iface"
 #define ETH_TAP_SPEED_ARG       "speed"
 #define ETH_TAP_REMOTE_ARG      "remote"
+#define ETH_TAP_MAC_ARG         "mac"
+#define ETH_TAP_MAC_FIXED       "fixed"
 
 #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
 #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
@@ -81,6 +83,7 @@ static const char *valid_arguments[] = {
 	ETH_TAP_IFACE_ARG,
 	ETH_TAP_SPEED_ARG,
 	ETH_TAP_REMOTE_ARG,
+	ETH_TAP_MAC_ARG,
 	NULL
 };
 
@@ -1131,7 +1134,8 @@ tap_kernel_support(struct pmd_internals *pmd)
 }
 
 static int
-eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
+eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
+		   int fixed_mac_type)
 {
 	int numa_node = rte_socket_id();
 	struct rte_eth_dev *dev = NULL;
@@ -1229,6 +1233,13 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
 		}
 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
 			   ETHER_ADDR_LEN);
+	} else if (fixed_mac_type) {
+		/* fixed mac = 00:64:74:61:70:<iface_idx> */
+		static int iface_idx;
+		char mac[ETHER_ADDR_LEN] = "\0dtap";
+
+		mac[ETHER_ADDR_LEN - 1] = iface_idx++;
+		rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);
 	} else {
 		eth_random_addr((uint8_t *)&pmd->eth_addr);
 	}
@@ -1285,6 +1296,17 @@ set_remote_iface(const char *key __rte_unused,
 	return 0;
 }
 
+static int
+set_mac_type(const char *key __rte_unused,
+	     const char *value,
+	     void *extra_args)
+{
+	if (value &&
+	    !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
+		*(int *)extra_args = 1;
+       return 0;
+}
+
 /* Open a TAP interface device.
  */
 static int
@@ -1295,6 +1317,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
 	int speed;
 	char tap_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
+	int fixed_mac_type = 0;
 
 	speed = ETH_SPEED_NUM_10G;
 	snprintf(tap_name, sizeof(tap_name), "%s%d",
@@ -1332,6 +1355,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
 				if (ret == -1)
 					goto leave;
 			}
+
+			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
+				ret = rte_kvargs_process(kvlist,
+							 ETH_TAP_MAC_ARG,
+							 &set_mac_type,
+							 &fixed_mac_type);
+				if (ret == -1)
+					goto leave;
+			}
 		}
 	}
 	pmd_link.link_speed = speed;
@@ -1339,7 +1371,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
 	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
 		name, tap_name);
 
-	ret = eth_dev_tap_create(name, tap_name, remote_iface);
+	ret = eth_dev_tap_create(name, tap_name, remote_iface, fixed_mac_type);
 
 leave:
 	if (ret == -1) {
@@ -1397,4 +1429,5 @@ RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
 RTE_PMD_REGISTER_PARAM_STRING(net_tap,
 			      ETH_TAP_IFACE_ARG "=<string> "
 			      ETH_TAP_SPEED_ARG "=<int> "
+			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
 			      ETH_TAP_REMOTE_ARG "=<string>");
-- 
2.12.0.306.g4a9b9b3

Re: [PATCH] net/tap: add support for fixed mac addresses

[Wiles, Keith]

> On Apr 11, 2017, at 10:09 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> Support for a fixed MAC address for testing with the last octet
> incrementing by one for each interface defined with the new 'mac=fixed'
> string on the --vdev option. The default option is still to randomize
> the MAC address for each tap interface.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
> 
> Hi Keith, I've made the changes I suggested.
> 
> It does as expected use a mac incremented by 1 for each new tap device,
> without a global variable for fixed_mac_type.
> 
> With the more clean "\0dtap" initialization for mac[], I do think it
> looks better, don't you?
> I agree my version with \x did little improvement to yours, and would
> be a question of style.
> What do you say of this patch?

The patch looks fine, I guess I did not want to change the function API (for some strange reason), but I agree removing the static works.

I can remove my patch and allow this one to replace it.

> 
> doc/guides/nics/tap.rst       | 13 ++++++++++++-
> drivers/net/tap/rte_eth_tap.c | 39 ++++++++++++++++++++++++++++++++++++---
> 2 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
> index 5c5ba5357bba..f3ee95d2897b 100644
> --- a/doc/guides/nics/tap.rst
> +++ b/doc/guides/nics/tap.rst
> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
> along with being able to be used as a network connection to the DPDK
> application. The method enable one or more interfaces is to use the
> ``--vdev=net_tap0`` option on the DPDK application command line. Each
> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
> and so on.
> 
> The interface name can be changed by adding the ``iface=foo0``, for example::
> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
> 
>    --vdev=net_tap0,iface=foo0,speed=25000
> 
> +Normally the PMD will generate a random MAC address, but when testing or with
> +a static configuration the developer may need a fixed MAC address style.
> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
> +
> +   --vdev=net_tap0,mac=fixed
> +
> +The MAC address will have a fixed value with the last octet incrementing by one
> +for each interface string containing ``mac=fixed``. The MAC address is formatted
> +as 00:'d':'t':'a':'p':[00-FF]. Convert the characters to hex and you get the
> +actual MAC address: ``00:64:74:61:70:[00-FF]``.
> +
> It is possible to specify a remote netdevice to capture packets from by adding
> ``remote=foo1``, for example::
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index cd82a7e09c75..2092f5d4184b 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -1,7 +1,7 @@
> /*-
>  *   BSD LICENSE
>  *
> - *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> + *   Copyright(c) 2016-2017 Intel Corporation. All rights reserved.
>  *   All rights reserved.
>  *
>  *   Redistribution and use in source and binary forms, with or without
> @@ -71,6 +71,8 @@
> #define ETH_TAP_IFACE_ARG       "iface"
> #define ETH_TAP_SPEED_ARG       "speed"
> #define ETH_TAP_REMOTE_ARG      "remote"
> +#define ETH_TAP_MAC_ARG         "mac"
> +#define ETH_TAP_MAC_FIXED       "fixed"
> 
> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
> @@ -81,6 +83,7 @@ static const char *valid_arguments[] = {
> 	ETH_TAP_IFACE_ARG,
> 	ETH_TAP_SPEED_ARG,
> 	ETH_TAP_REMOTE_ARG,
> +	ETH_TAP_MAC_ARG,
> 	NULL
> };
> 
> @@ -1131,7 +1134,8 @@ tap_kernel_support(struct pmd_internals *pmd)
> }
> 
> static int
> -eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
> +eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
> +		   int fixed_mac_type)
> {
> 	int numa_node = rte_socket_id();
> 	struct rte_eth_dev *dev = NULL;
> @@ -1229,6 +1233,13 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
> 		}
> 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
> 			   ETHER_ADDR_LEN);
> +	} else if (fixed_mac_type) {
> +		/* fixed mac = 00:64:74:61:70:<iface_idx> */
> +		static int iface_idx;
> +		char mac[ETHER_ADDR_LEN] = "\0dtap";
> +
> +		mac[ETHER_ADDR_LEN - 1] = iface_idx++;
> +		rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);
> 	} else {
> 		eth_random_addr((uint8_t *)&pmd->eth_addr);
> 	}
> @@ -1285,6 +1296,17 @@ set_remote_iface(const char *key __rte_unused,
> 	return 0;
> }
> 
> +static int
> +set_mac_type(const char *key __rte_unused,
> +	     const char *value,
> +	     void *extra_args)
> +{
> +	if (value &&
> +	    !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
> +		*(int *)extra_args = 1;
> +       return 0;
> +}
> +
> /* Open a TAP interface device.
>  */
> static int
> @@ -1295,6 +1317,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
> 	int speed;
> 	char tap_name[RTE_ETH_NAME_MAX_LEN];
> 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> +	int fixed_mac_type = 0;
> 
> 	speed = ETH_SPEED_NUM_10G;
> 	snprintf(tap_name, sizeof(tap_name), "%s%d",
> @@ -1332,6 +1355,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
> 				if (ret == -1)
> 					goto leave;
> 			}
> +
> +			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
> +				ret = rte_kvargs_process(kvlist,
> +							 ETH_TAP_MAC_ARG,
> +							 &set_mac_type,
> +							 &fixed_mac_type);
> +				if (ret == -1)
> +					goto leave;
> +			}
> 		}
> 	}
> 	pmd_link.link_speed = speed;
> @@ -1339,7 +1371,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
> 	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
> 		name, tap_name);
> 
> -	ret = eth_dev_tap_create(name, tap_name, remote_iface);
> +	ret = eth_dev_tap_create(name, tap_name, remote_iface, fixed_mac_type);
> 
> leave:
> 	if (ret == -1) {
> @@ -1397,4 +1429,5 @@ RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
> RTE_PMD_REGISTER_PARAM_STRING(net_tap,
> 			      ETH_TAP_IFACE_ARG "=<string> "
> 			      ETH_TAP_SPEED_ARG "=<int> "
> +			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
> 			      ETH_TAP_REMOTE_ARG "=<string>");
> -- 
> 2.12.0.306.g4a9b9b3
> 

Regards,
Keith

Re: [PATCH] net/tap: add support for fixed mac addresses

[Wiles, Keith]

> On Apr 11, 2017, at 10:09 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> Support for a fixed MAC address for testing with the last octet
> incrementing by one for each interface defined with the new 'mac=fixed'
> string on the --vdev option. The default option is still to randomize
> the MAC address for each tap interface.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
> 
> Hi Keith, I've made the changes I suggested.
> 
> It does as expected use a mac incremented by 1 for each new tap device,
> without a global variable for fixed_mac_type.
> 
> With the more clean "\0dtap" initialization for mac[], I do think it
> looks better, don't you?
> I agree my version with \x did little improvement to yours, and would
> be a question of style.
> What do you say of this patch?
> 
> doc/guides/nics/tap.rst       | 13 ++++++++++++-
> drivers/net/tap/rte_eth_tap.c | 39 ++++++++++++++++++++++++++++++++++++---
> 2 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
> index 5c5ba5357bba..f3ee95d2897b 100644
> --- a/doc/guides/nics/tap.rst
> +++ b/doc/guides/nics/tap.rst
> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
> along with being able to be used as a network connection to the DPDK
> application. The method enable one or more interfaces is to use the
> ``--vdev=net_tap0`` option on the DPDK application command line. Each
> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
> and so on.
> 
> The interface name can be changed by adding the ``iface=foo0``, for example::
> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
> 
>    --vdev=net_tap0,iface=foo0,speed=25000
> 
> +Normally the PMD will generate a random MAC address, but when testing or with
> +a static configuration the developer may need a fixed MAC address style.
> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
> +
> +   --vdev=net_tap0,mac=fixed
> +
> +The MAC address will have a fixed value with the last octet incrementing by one
> +for each interface string containing ``mac=fixed``. The MAC address is formatted
> +as 00:'d':'t':'a':'p':[00-FF]. Convert the characters to hex and you get the
> +actual MAC address: ``00:64:74:61:70:[00-FF]``.
> +
> It is possible to specify a remote netdevice to capture packets from by adding
> ``remote=foo1``, for example::
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index cd82a7e09c75..2092f5d4184b 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -1,7 +1,7 @@
> /*-
>  *   BSD LICENSE
>  *
> - *   Copyright(c) 2016 Intel Corporation. All rights reserved.
> + *   Copyright(c) 2016-2017 Intel Corporation. All rights reserved.
>  *   All rights reserved.
>  *
>  *   Redistribution and use in source and binary forms, with or without
> @@ -71,6 +71,8 @@
> #define ETH_TAP_IFACE_ARG       "iface"
> #define ETH_TAP_SPEED_ARG       "speed"
> #define ETH_TAP_REMOTE_ARG      "remote"
> +#define ETH_TAP_MAC_ARG         "mac"
> +#define ETH_TAP_MAC_FIXED       "fixed"
> 
> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
> @@ -81,6 +83,7 @@ static const char *valid_arguments[] = {
> 	ETH_TAP_IFACE_ARG,
> 	ETH_TAP_SPEED_ARG,
> 	ETH_TAP_REMOTE_ARG,
> +	ETH_TAP_MAC_ARG,
> 	NULL
> };
> 
> @@ -1131,7 +1134,8 @@ tap_kernel_support(struct pmd_internals *pmd)
> }
> 
> static int
> -eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
> +eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
> +		   int fixed_mac_type)
> {
> 	int numa_node = rte_socket_id();
> 	struct rte_eth_dev *dev = NULL;
> @@ -1229,6 +1233,13 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
> 		}
> 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
> 			   ETHER_ADDR_LEN);
> +	} else if (fixed_mac_type) {
> +		/* fixed mac = 00:64:74:61:70:<iface_idx> */
> +		static int iface_idx;
> +		char mac[ETHER_ADDR_LEN] = "\0dtap";
> +
> +		mac[ETHER_ADDR_LEN - 1] = iface_idx++;
> +		rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);
> 	} else {
> 		eth_random_addr((uint8_t *)&pmd->eth_addr);
> 	}

This does not show up as a change in the patch, I thought it should.

> @@ -1285,6 +1296,17 @@ set_remote_iface(const char *key __rte_unused,
> 	return 0;
> }
> 
> +static int
> +set_mac_type(const char *key __rte_unused,
> +	     const char *value,
> +	     void *extra_args)
> +{
> +	if (value &&
> +	    !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
> +		*(int *)extra_args = 1;
> +       return 0;
> +}
> +
> /* Open a TAP interface device.
>  */
> static int
> @@ -1295,6 +1317,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
> 	int speed;
> 	char tap_name[RTE_ETH_NAME_MAX_LEN];
> 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> +	int fixed_mac_type = 0;
> 
> 	speed = ETH_SPEED_NUM_10G;
> 	snprintf(tap_name, sizeof(tap_name), "%s%d",
> @@ -1332,6 +1355,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
> 				if (ret == -1)
> 					goto leave;
> 			}
> +
> +			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
> +				ret = rte_kvargs_process(kvlist,
> +							 ETH_TAP_MAC_ARG,
> +							 &set_mac_type,
> +							 &fixed_mac_type);
> +				if (ret == -1)
> +					goto leave;
> +			}
> 		}
> 	}
> 	pmd_link.link_speed = speed;
> @@ -1339,7 +1371,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
> 	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
> 		name, tap_name);
> 
> -	ret = eth_dev_tap_create(name, tap_name, remote_iface);
> +	ret = eth_dev_tap_create(name, tap_name, remote_iface, fixed_mac_type);
> 
> leave:
> 	if (ret == -1) {
> @@ -1397,4 +1429,5 @@ RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
> RTE_PMD_REGISTER_PARAM_STRING(net_tap,
> 			      ETH_TAP_IFACE_ARG "=<string> "
> 			      ETH_TAP_SPEED_ARG "=<int> "
> +			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
> 			      ETH_TAP_REMOTE_ARG "=<string>");
> -- 
> 2.12.0.306.g4a9b9b3
> 

Regards,
Keith

Re: [PATCH] net/tap: add support for fixed mac addresses

[Ferruh Yigit]

On 4/11/2017 4:42 PM, Wiles, Keith wrote:
> 
>> On Apr 11, 2017, at 10:09 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> Support for a fixed MAC address for testing with the last octet
>> incrementing by one for each interface defined with the new 'mac=fixed'
>> string on the --vdev option. The default option is still to randomize
>> the MAC address for each tap interface.
>>
>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>

<...>
> This does not show up as a change in the patch, I thought it should.

It does: http://dpdk.org/dev/patchwork/patch/23508/

Re: [PATCH] net/tap: add support for fixed mac addresses

[Wiles, Keith]

> On Apr 11, 2017, at 10:09 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> 
> Support for a fixed MAC address for testing with the last octet
> incrementing by one for each interface defined with the new 'mac=fixed'
> string on the --vdev option. The default option is still to randomize
> the MAC address for each tap interface.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> —

Just to be clear:
Acked-by: Keith Wiles <keith.wile@intel.com>

Regards,
Keith

[PATCH v2] net/tap: add support for fixed mac addresses

[Pascal Mazon]

Support for a fixed MAC address for testing with the last octet
incrementing by one for each interface defined with the new 'mac=fixed'
string on the --vdev option. The default option is still to randomize
the MAC address for each tap interface.

Signed-off-by: Keith Wiles <keith.wiles@intel.com>
Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
Acked-by: Keith Wiles <keith.wiles@intel.com>
---

v2:
  - fix spaces instead of tab in front of a return
  - MAC address generation should happen before checking for flower support.
    Otherwise the address could be unset with old kernels.

 doc/guides/nics/tap.rst       | 13 ++++++++++++-
 drivers/net/tap/rte_eth_tap.c | 45 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
index 5c5ba5357bba..f3ee95d2897b 100644
--- a/doc/guides/nics/tap.rst
+++ b/doc/guides/nics/tap.rst
@@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
 along with being able to be used as a network connection to the DPDK
 application. The method enable one or more interfaces is to use the
 ``--vdev=net_tap0`` option on the DPDK application command line. Each
-``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
+``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
 and so on.
 
 The interface name can be changed by adding the ``iface=foo0``, for example::
@@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
 
    --vdev=net_tap0,iface=foo0,speed=25000
 
+Normally the PMD will generate a random MAC address, but when testing or with
+a static configuration the developer may need a fixed MAC address style.
+Using the option ``mac=fixed`` you can create a fixed known MAC address::
+
+   --vdev=net_tap0,mac=fixed
+
+The MAC address will have a fixed value with the last octet incrementing by one
+for each interface string containing ``mac=fixed``. The MAC address is formatted
+as 00:'d':'t':'a':'p':[00-FF]. Convert the characters to hex and you get the
+actual MAC address: ``00:64:74:61:70:[00-FF]``.
+
 It is possible to specify a remote netdevice to capture packets from by adding
 ``remote=foo1``, for example::
 
diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index cd82a7e09c75..b75663838844 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2016 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2016-2017 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -71,6 +71,8 @@
 #define ETH_TAP_IFACE_ARG       "iface"
 #define ETH_TAP_SPEED_ARG       "speed"
 #define ETH_TAP_REMOTE_ARG      "remote"
+#define ETH_TAP_MAC_ARG         "mac"
+#define ETH_TAP_MAC_FIXED       "fixed"
 
 #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
 #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
@@ -81,6 +83,7 @@ static const char *valid_arguments[] = {
 	ETH_TAP_IFACE_ARG,
 	ETH_TAP_SPEED_ARG,
 	ETH_TAP_REMOTE_ARG,
+	ETH_TAP_MAC_ARG,
 	NULL
 };
 
@@ -1131,7 +1134,8 @@ tap_kernel_support(struct pmd_internals *pmd)
 }
 
 static int
-eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
+eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface,
+		   int fixed_mac_type)
 {
 	int numa_node = rte_socket_id();
 	struct rte_eth_dev *dev = NULL;
@@ -1202,6 +1206,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
 		pmd->txq[i].fd = -1;
 	}
 
+	if (fixed_mac_type) {
+		/* fixed mac = 00:64:74:61:70:<iface_idx> */
+		static int iface_idx;
+		char mac[ETHER_ADDR_LEN] = "\0dtap";
+
+		mac[ETHER_ADDR_LEN - 1] = iface_idx++;
+		rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);
+	} else {
+		eth_random_addr((uint8_t *)&pmd->eth_addr);
+	}
+
 	tap_kernel_support(pmd);
 	if (!pmd->flower_support)
 		return 0;
@@ -1229,8 +1244,6 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
 		}
 		rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
 			   ETHER_ADDR_LEN);
-	} else {
-		eth_random_addr((uint8_t *)&pmd->eth_addr);
 	}
 
 	return 0;
@@ -1285,6 +1298,17 @@ set_remote_iface(const char *key __rte_unused,
 	return 0;
 }
 
+static int
+set_mac_type(const char *key __rte_unused,
+	     const char *value,
+	     void *extra_args)
+{
+	if (value &&
+	    !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
+		*(int *)extra_args = 1;
+	return 0;
+}
+
 /* Open a TAP interface device.
  */
 static int
@@ -1295,6 +1319,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
 	int speed;
 	char tap_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
+	int fixed_mac_type = 0;
 
 	speed = ETH_SPEED_NUM_10G;
 	snprintf(tap_name, sizeof(tap_name), "%s%d",
@@ -1332,6 +1357,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
 				if (ret == -1)
 					goto leave;
 			}
+
+			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
+				ret = rte_kvargs_process(kvlist,
+							 ETH_TAP_MAC_ARG,
+							 &set_mac_type,
+							 &fixed_mac_type);
+				if (ret == -1)
+					goto leave;
+			}
 		}
 	}
 	pmd_link.link_speed = speed;
@@ -1339,7 +1373,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
 	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
 		name, tap_name);
 
-	ret = eth_dev_tap_create(name, tap_name, remote_iface);
+	ret = eth_dev_tap_create(name, tap_name, remote_iface, fixed_mac_type);
 
 leave:
 	if (ret == -1) {
@@ -1397,4 +1431,5 @@ RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
 RTE_PMD_REGISTER_PARAM_STRING(net_tap,
 			      ETH_TAP_IFACE_ARG "=<string> "
 			      ETH_TAP_SPEED_ARG "=<int> "
+			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
 			      ETH_TAP_REMOTE_ARG "=<string>");
-- 
2.12.0.306.g4a9b9b3

Re: [PATCH v2] net/tap: add support for fixed mac addresses

[Ferruh Yigit]

On 4/12/2017 8:30 AM, Pascal Mazon wrote:
> Support for a fixed MAC address for testing with the last octet
> incrementing by one for each interface defined with the new 'mac=fixed'
> string on the --vdev option. The default option is still to randomize
> the MAC address for each tap interface.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> Acked-by: Keith Wiles <keith.wiles@intel.com>
> ---
> 
> v2:
>   - fix spaces instead of tab in front of a return
>   - MAC address generation should happen before checking for flower support.
>     Otherwise the address could be unset with old kernels.
> 

Thanks for the patch, this will be considered for 17.08

Re: [PATCH v2] net/tap: add support for fixed mac addresses

[Ferruh Yigit]

On 4/12/2017 8:30 AM, Pascal Mazon wrote:
> Support for a fixed MAC address for testing with the last octet
> incrementing by one for each interface defined with the new 'mac=fixed'
> string on the --vdev option. The default option is still to randomize
> the MAC address for each tap interface.
> 
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> Acked-by: Keith Wiles <keith.wiles@intel.com>

Applied to dpdk-next-net/master, thanks.