[dpdk-dev] [PATCH] net/tap: allow user MAC to be passed as args

[dpdk-dev] [PATCH] net/tap: allow user MAC to be passed as args

[Vipin Varghese]

Allow TAP PMD to pass user desired MAC address as argument.
The argument value is processed as string, where each 2 bytes
are converted to HEX MAC address after validation.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 doc/guides/nics/tap.rst       |  6 +++++
 drivers/net/tap/rte_eth_tap.c | 62 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
index dc6f834..6b083c8 100644
--- a/doc/guides/nics/tap.rst
+++ b/doc/guides/nics/tap.rst
@@ -69,6 +69,12 @@ 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]``.
 
+   --vdev=net_tap0,mac="00:64:74:61:70:11"
+
+The MAC address will have a user value passed as string. The MAC address is in
+format with delimeter ``:``. The string is byte converted to hex and you get
+the actual MAC address: ``00:64:74:61:70:11``.
+
 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 29d6356..3489b04 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -49,7 +49,14 @@
 #define ETH_TAP_MAC_ARG         "mac"
 #define ETH_TAP_MAC_FIXED       "fixed"
 
+#define ETH_TAP_MAC_STR_FXD     1
+#define ETH_TAP_MAC_STR_USR     2
+#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
+#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
+#define ETH_TAP_MAC_ARG_FMT     "["ETH_TAP_MAC_FIXED "|" ETH_TAP_USR_MAC_FMT"]"
+
 static struct rte_vdev_driver pmd_tap_drv;
+static unsigned char user_mac[ETHER_ADDR_LEN];
 
 static const char *valid_arguments[] = {
 	ETH_TAP_IFACE_ARG,
@@ -1397,13 +1404,20 @@ enum ioctl_mode {
 		pmd->txq[i].fd = -1;
 	}
 
-	if (fixed_mac_type) {
+	if (fixed_mac_type == ETH_TAP_MAC_STR_FXD) {
 		/* 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 if (fixed_mac_type == ETH_TAP_MAC_STR_USR) {
+		RTE_LOG(INFO, PMD,
+			"%s; user MAC (%02x:%02x:%02x:%02x:%02x:%02x) argument\n",
+			pmd->name,
+			user_mac[0], user_mac[1], user_mac[2],
+			user_mac[3], user_mac[4], user_mac[5]);
+			rte_memcpy(&pmd->eth_addr, user_mac, ETHER_ADDR_LEN);
 	} else {
 		eth_random_addr((uint8_t *)&pmd->eth_addr);
 	}
@@ -1577,10 +1591,48 @@ enum ioctl_mode {
 	     const char *value,
 	     void *extra_args)
 {
-	if (value &&
-	    !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
-		*(int *)extra_args = 1;
+	char mac_temp[20] = {0}, *mac_byte = NULL;
+	unsigned int index = 0;
+
+	if (!value)
+		return 0;
+
+	if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
+			strlen(ETH_TAP_MAC_FIXED))) {
+		*(int *)extra_args = ETH_TAP_MAC_STR_FXD;
+		goto success;
+	}
+
+	if (strlen(value) == 17) {
+		strncpy(mac_temp, value, 18);
+		mac_temp[19] = '\0';
+		mac_byte = strtok(mac_temp, ":");
+
+		while ((mac_byte != NULL) &&
+				strspn(mac_byte, ETH_TAP_CMP_MAC_FMT) &&
+				strspn((mac_byte + 1), ETH_TAP_CMP_MAC_FMT) &&
+				strlen(mac_byte) == 2) {
+			user_mac[index] = strtoul(mac_byte, NULL, 16);
+			mac_byte = strtok(NULL, ":");
+			index += 1;
+		}
+
+		if (index != 6)
+			goto error;
+
+		*(int *)extra_args = ETH_TAP_MAC_STR_USR;
+	} else {
+		goto error;
+	}
+
+success:
+	RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
 	return 0;
+
+error:
+	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
+		value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
+	return -1;
 }
 
 /* Open a TAP interface device.
@@ -1716,5 +1768,5 @@ enum ioctl_mode {
 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_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT
 			      ETH_TAP_REMOTE_ARG "=<string>");
-- 
1.9.1

Re: [dpdk-dev] [PATCH] net/tap: allow user MAC to be passed as args

[Pascal Mazon]

Hi,

You didn't address my request about not using a global value. Was there
a good reason?

I paste it here again as a reminder:

  Can you also not use a global value for user_mac, but instead change the
  last argument for eth_dev_tap_create():
  Use directly a char mac[ETHER_ADDR_LEN], automatic variable from
  rte_pmd_tap_probe().
  In set_mac_type(), you can check either for "fixed" or a correct custom
  mac address.
  Then eth_dev_tap_create() can check if the provided mac is empty (!fixed
  and !custom_mac), to generate a random one.

Additional comments inline.

Best regards,
Pascal

On 31/01/2018 19:22, Vipin Varghese wrote:
> Allow TAP PMD to pass user desired MAC address as argument.
> The argument value is processed as string, where each 2 bytes
> are converted to HEX MAC address after validation.
>
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
>  doc/guides/nics/tap.rst       |  6 +++++
>  drivers/net/tap/rte_eth_tap.c | 62 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 63 insertions(+), 5 deletions(-)
>
> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
> index dc6f834..6b083c8 100644
> --- a/doc/guides/nics/tap.rst
> +++ b/doc/guides/nics/tap.rst
> @@ -69,6 +69,12 @@ 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]``.
>  
> +   --vdev=net_tap0,mac="00:64:74:61:70:11"
> +
> +The MAC address will have a user value passed as string. The MAC address is in
> +format with delimeter ``:``. The string is byte converted to hex and you get
> +the actual MAC address: ``00:64:74:61:70:11``.
> +
>  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 29d6356..3489b04 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -49,7 +49,14 @@
>  #define ETH_TAP_MAC_ARG         "mac"
>  #define ETH_TAP_MAC_FIXED       "fixed"
>  
> +#define ETH_TAP_MAC_STR_FXD     1
> +#define ETH_TAP_MAC_STR_USR     2
> +#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
> +#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
> +#define ETH_TAP_MAC_ARG_FMT     "["ETH_TAP_MAC_FIXED "|" ETH_TAP_USR_MAC_FMT"]"
> +
>  static struct rte_vdev_driver pmd_tap_drv;
> +static unsigned char user_mac[ETHER_ADDR_LEN];
>  
>  static const char *valid_arguments[] = {
>  	ETH_TAP_IFACE_ARG,
> @@ -1397,13 +1404,20 @@ enum ioctl_mode {
>  		pmd->txq[i].fd = -1;
>  	}
>  
> -	if (fixed_mac_type) {
> +	if (fixed_mac_type == ETH_TAP_MAC_STR_FXD) {
>  		/* 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 if (fixed_mac_type == ETH_TAP_MAC_STR_USR) {
> +		RTE_LOG(INFO, PMD,
> +			"%s; user MAC (%02x:%02x:%02x:%02x:%02x:%02x) argument\n",
Shouldn't it be a colon there? "%s:"
> +			pmd->name,
> +			user_mac[0], user_mac[1], user_mac[2],
> +			user_mac[3], user_mac[4], user_mac[5]);
> +			rte_memcpy(&pmd->eth_addr, user_mac, ETHER_ADDR_LEN);
>  	} else {
>  		eth_random_addr((uint8_t *)&pmd->eth_addr);
>  	}
> @@ -1577,10 +1591,48 @@ enum ioctl_mode {
>  	     const char *value,
>  	     void *extra_args)
>  {
> -	if (value &&
> -	    !strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED)))
> -		*(int *)extra_args = 1;
> +	char mac_temp[20] = {0}, *mac_byte = NULL;
Instead of hardcoded values, I'd use
mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1]
> +	unsigned int index = 0;
> +
> +	if (!value)
> +		return 0;
> +
> +	if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
> +			strlen(ETH_TAP_MAC_FIXED))) {
> +		*(int *)extra_args = ETH_TAP_MAC_STR_FXD;
> +		goto success;
> +	}
> +
> +	if (strlen(value) == 17) {
And here 17 => strlen(ETH_TAP_USR_MAC_FMT)
> +		strncpy(mac_temp, value, 18);
> +		mac_temp[19] = '\0';
Instead of those two lines, I'd rather have snprintf(mac_temp,
sizeof(mac_temp), "%s", value).
It handles the trailing \0 nicely.
> +		mac_byte = strtok(mac_temp, ":");
> +
> +		while ((mac_byte != NULL) &&
> +				strspn(mac_byte, ETH_TAP_CMP_MAC_FMT) &&
> +				strspn((mac_byte + 1), ETH_TAP_CMP_MAC_FMT) &&
> +				strlen(mac_byte) == 2) {
> +			user_mac[index] = strtoul(mac_byte, NULL, 16);
> +			mac_byte = strtok(NULL, ":");
> +			index += 1;
> +		}
> +
> +		if (index != 6)
> +			goto error;
> +
> +		*(int *)extra_args = ETH_TAP_MAC_STR_USR;
> +	} else {
> +		goto error;
> +	}
> +
> +success:
> +	RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
>  	return 0;
> +
> +error:
> +	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
> +		value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
> +	return -1;
>  }
>  
>  /* Open a TAP interface device.
> @@ -1716,5 +1768,5 @@ enum ioctl_mode {
>  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_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT
>  			      ETH_TAP_REMOTE_ARG "=<string>");

Re: [dpdk-dev] [PATCH] net/tap: allow user MAC to be passed as args

[Varghese, Vipin]

Hi Pascal,

Sincere apologizes, I think I missed out since rework was asked. Please find my answers inline to the comment

> -----Original Message-----
> From: Pascal Mazon [mailto:pascal.mazon@6wind.com]
> Sent: Friday, February 2, 2018 9:16 AM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Jain, Deepak K
> <deepak.k.jain@intel.com>
> Subject: Re: [PATCH] net/tap: allow user MAC to be passed as args
> 
> Hi,
> 
> You didn't address my request about not using a global value. Was there a good
> reason?
> 
> I paste it here again as a reminder:
> 
>   Can you also not use a global value for user_mac, but instead change the
>   last argument for eth_dev_tap_create():
>   Use directly a char mac[ETHER_ADDR_LEN], automatic variable from
>   rte_pmd_tap_probe().
>   In set_mac_type(), you can check either for "fixed" or a correct custom
>   mac address.
>   Then eth_dev_tap_create() can check if the provided mac is empty (!fixed
>   and !custom_mac), to generate a random one.

Last argument for eth_dev_tap_create is ' int fixed_mac_type '. Would like me to change this to 'uint64_t fixed_mac_type' to accommodate the MAC address?

Note: Should we change the API arguments?

> 
> Additional comments inline.
> 
> Best regards,
> Pascal
> 
> On 31/01/2018 19:22, Vipin Varghese wrote:


<Snipped>

> >  #define ETH_TAP_MAC_ARG         "mac"
> >  #define ETH_TAP_MAC_FIXED       "fixed"
> >
> > +#define ETH_TAP_MAC_STR_FXD     1
> > +#define ETH_TAP_MAC_STR_USR     2
> > +#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
> > +#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
> > +#define ETH_TAP_MAC_ARG_FMT     "["ETH_TAP_MAC_FIXED "|"
> ETH_TAP_USR_MAC_FMT"]"
> > +
> >  static struct rte_vdev_driver pmd_tap_drv;
> > +static unsigned char user_mac[ETHER_ADDR_LEN];
> >
> >  static const char *valid_arguments[] = {
> >  	ETH_TAP_IFACE_ARG,
> > @@ -1397,13 +1404,20 @@ enum ioctl_mode {
> >  		pmd->txq[i].fd = -1;
> >  	}
> >
> > -	if (fixed_mac_type) {
> > +	if (fixed_mac_type == ETH_TAP_MAC_STR_FXD) {
> >  		/* 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 if (fixed_mac_type == ETH_TAP_MAC_STR_USR) {
> > +		RTE_LOG(INFO, PMD,
> > +			"%s; user MAC (%02x:%02x:%02x:%02x:%02x:%02x)
> argument\n",
> Shouldn't it be a colon there? "%s:"

Ok, I can make this change.

<Snipped>

> > +	char mac_temp[20] = {0}, *mac_byte = NULL;
> Instead of hardcoded values, I'd use
> mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1]

Ok, I can make this change.

<Snipped>

> > +
> > +	if (strlen(value) == 17) {
> And here 17 => strlen(ETH_TAP_USR_MAC_FMT)

Ok

> > +		strncpy(mac_temp, value, 18);
> > +		mac_temp[19] = '\0';
> Instead of those two lines, I'd rather have snprintf(mac_temp,
> sizeof(mac_temp), "%s", value).
> It handles the trailing \0 nicely.

OK, I will check the same.

> > +		mac_byte = strtok(mac_temp, ":");


<Snipped>

Re: [dpdk-dev] [PATCH] net/tap: allow user MAC to be passed as args

[Varghese, Vipin]

Hi Pascal,

Waiting for your inputs in changing the function argument from int32_t to uint64_t.

Thanks
Vipin Varghese

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Varghese, Vipin
> Sent: Friday, February 2, 2018 9:50 AM
> To: Pascal Mazon <pascal.mazon@6wind.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Jain, Deepak K
> <deepak.k.jain@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/tap: allow user MAC to be passed as args
> 
> Hi Pascal,
> 
> Sincere apologizes, I think I missed out since rework was asked. Please find my
> answers inline to the comment
> 
> > -----Original Message-----
> > From: Pascal Mazon [mailto:pascal.mazon@6wind.com]
> > Sent: Friday, February 2, 2018 9:16 AM
> > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Jain, Deepak K
> > <deepak.k.jain@intel.com>
> > Subject: Re: [PATCH] net/tap: allow user MAC to be passed as args
> >
> > Hi,
> >
> > You didn't address my request about not using a global value. Was
> > there a good reason?
> >
> > I paste it here again as a reminder:
> >
> >   Can you also not use a global value for user_mac, but instead change the
> >   last argument for eth_dev_tap_create():
> >   Use directly a char mac[ETHER_ADDR_LEN], automatic variable from
> >   rte_pmd_tap_probe().
> >   In set_mac_type(), you can check either for "fixed" or a correct custom
> >   mac address.
> >   Then eth_dev_tap_create() can check if the provided mac is empty (!fixed
> >   and !custom_mac), to generate a random one.
> 
> Last argument for eth_dev_tap_create is ' int fixed_mac_type '. Would like me
> to change this to 'uint64_t fixed_mac_type' to accommodate the MAC address?
> 
> Note: Should we change the API arguments?
> 
> >
> > Additional comments inline.
> >
> > Best regards,
> > Pascal
> >
> > On 31/01/2018 19:22, Vipin Varghese wrote:
> 
> 
> <Snipped>
> 
> > >  #define ETH_TAP_MAC_ARG         "mac"
> > >  #define ETH_TAP_MAC_FIXED       "fixed"
> > >
> > > +#define ETH_TAP_MAC_STR_FXD     1
> > > +#define ETH_TAP_MAC_STR_USR     2
> > > +#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
> > > +#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
> > > +#define ETH_TAP_MAC_ARG_FMT     "["ETH_TAP_MAC_FIXED "|"
> > ETH_TAP_USR_MAC_FMT"]"
> > > +
> > >  static struct rte_vdev_driver pmd_tap_drv;
> > > +static unsigned char user_mac[ETHER_ADDR_LEN];
> > >
> > >  static const char *valid_arguments[] = {
> > >  	ETH_TAP_IFACE_ARG,
> > > @@ -1397,13 +1404,20 @@ enum ioctl_mode {
> > >  		pmd->txq[i].fd = -1;
> > >  	}
> > >
> > > -	if (fixed_mac_type) {
> > > +	if (fixed_mac_type == ETH_TAP_MAC_STR_FXD) {
> > >  		/* 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 if (fixed_mac_type == ETH_TAP_MAC_STR_USR) {
> > > +		RTE_LOG(INFO, PMD,
> > > +			"%s; user MAC (%02x:%02x:%02x:%02x:%02x:%02x)
> > argument\n",
> > Shouldn't it be a colon there? "%s:"
> 
> Ok, I can make this change.
> 
> <Snipped>
> 
> > > +	char mac_temp[20] = {0}, *mac_byte = NULL;
> > Instead of hardcoded values, I'd use
> > mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1]
> 
> Ok, I can make this change.
> 
> <Snipped>
> 
> > > +
> > > +	if (strlen(value) == 17) {
> > And here 17 => strlen(ETH_TAP_USR_MAC_FMT)
> 
> Ok
> 
> > > +		strncpy(mac_temp, value, 18);
> > > +		mac_temp[19] = '\0';
> > Instead of those two lines, I'd rather have snprintf(mac_temp,
> > sizeof(mac_temp), "%s", value).
> > It handles the trailing \0 nicely.
> 
> OK, I will check the same.
> 
> > > +		mac_byte = strtok(mac_temp, ":");
> 
> 
> <Snipped>

[dpdk-dev] [PATCH v2] net/tap: allow user MAC to be passed as args

[Pascal Mazon]

From: Vipin Varghese <vipin.varghese@intel.com>

Allow TAP PMD to pass user desired MAC address as argument.
The argument value is processed as string, where each 2 bytes
are converted to HEX MAC address after validation.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
---

Hi Vipin,

I suggest the following patch for your MAC address argument, if that
suits you.
I removed ETH_TAP_MAC_STR_FXD and ETH_TAP_MAC_STR_USR that were not
really useful. I find it nicer to deal with MAC mostly inside
set_mac_type().
What do you think? Can you test that it fits your needs?

Best regards,
Pascal

 doc/guides/nics/tap.rst       |  6 ++++
 drivers/net/tap/rte_eth_tap.c | 80 +++++++++++++++++++++++++++++--------------
 2 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
index dc6f834ca0a1..6b083c846b21 100644
--- a/doc/guides/nics/tap.rst
+++ b/doc/guides/nics/tap.rst
@@ -69,6 +69,12 @@ 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]``.
 
+   --vdev=net_tap0,mac="00:64:74:61:70:11"
+
+The MAC address will have a user value passed as string. The MAC address is in
+format with delimeter ``:``. The string is byte converted to hex and you get
+the actual MAC address: ``00:64:74:61:70:11``.
+
 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 29d635613c0b..d9e5ec92b8f0 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -49,6 +49,10 @@
 #define ETH_TAP_MAC_ARG         "mac"
 #define ETH_TAP_MAC_FIXED       "fixed"
 
+#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
+#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
+#define ETH_TAP_MAC_ARG_FMT     "["ETH_TAP_MAC_FIXED "|" ETH_TAP_USR_MAC_FMT"]"
+
 static struct rte_vdev_driver pmd_tap_drv;
 
 static const char *valid_arguments[] = {
@@ -278,15 +282,8 @@ tap_rx_offload_get_queue_capa(void)
 static bool
 tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
 {
-	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
-	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
-	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
-
-	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
-	    offloads)
-		return false;
-	if ((port_offloads ^ offloads) & port_supp_offloads)
-		return false;
+	(void)dev;
+	(void)offloads;
 	return true;
 }
 
@@ -1335,7 +1332,7 @@ static const struct eth_dev_ops ops = {
 
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
-		   char *remote_iface, int fixed_mac_type)
+		   char *remote_iface, struct ether_addr *mac_addr)
 {
 	int numa_node = rte_socket_id();
 	struct rte_eth_dev *dev;
@@ -1397,16 +1394,10 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 		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 {
+	if (is_zero_ether_addr(mac_addr))
 		eth_random_addr((uint8_t *)&pmd->eth_addr);
-	}
+	else
+		rte_memcpy(&pmd->eth_addr, mac_addr, sizeof(mac_addr));
 
 	/* Immediately create the netdevice (this will create the 1st queue). */
 	/* rx queue */
@@ -1577,10 +1568,47 @@ 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;
+	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte = NULL;
+	struct ether_addr *user_mac = extra_args;
+	unsigned int index = 0;
+
+	if (!value)
+		return 0;
+
+	if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
+			strlen(ETH_TAP_MAC_FIXED))) {
+		static int iface_idx;
+
+		/* fixed mac = 00:64:74:61:70:<iface_idx> */
+		memcpy((char *)user_mac->addr_bytes, "\0dtap", ETHER_ADDR_LEN);
+		user_mac->addr_bytes[ETHER_ADDR_LEN - 1] = iface_idx++ + '0';
+		goto success;
+	}
+
+	if (strlen(value) != strlen(ETH_TAP_USR_MAC_FMT))
+		goto error;
+
+	snprintf(mac_temp, sizeof(mac_temp), "%s", value);
+	mac_byte = strtok(mac_temp, ":");
+
+	while ((mac_byte != NULL) &&
+			strspn(mac_byte, ETH_TAP_CMP_MAC_FMT) &&
+			strlen(mac_byte) == 2) {
+		user_mac->addr_bytes[index++] = (char) strtoul(mac_byte, NULL, 16);
+		mac_byte = strtok(NULL, ":");
+	}
+
+	if (index != 6)
+		goto error;
+
+success:
+	RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
 	return 0;
+
+error:
+	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
+		value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
+	return -1;
 }
 
 /* Open a TAP interface device.
@@ -1594,7 +1622,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	int speed;
 	char tap_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
-	int fixed_mac_type = 0;
+	struct ether_addr user_mac = {0};
 
 	name = rte_vdev_device_name(dev);
 	params = rte_vdev_device_args(dev);
@@ -1640,7 +1668,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 				ret = rte_kvargs_process(kvlist,
 							 ETH_TAP_MAC_ARG,
 							 &set_mac_type,
-							 &fixed_mac_type);
+							 &user_mac);
 				if (ret == -1)
 					goto leave;
 			}
@@ -1651,7 +1679,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
 	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
 		name, tap_name);
 
-	ret = eth_dev_tap_create(dev, tap_name, remote_iface, fixed_mac_type);
+	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
 
 leave:
 	if (ret == -1) {
@@ -1716,5 +1744,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_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT
 			      ETH_TAP_REMOTE_ARG "=<string>");
-- 
2.11.0

Re: [dpdk-dev] [PATCH v2] net/tap: allow user MAC to be passed as args

[Varghese, Vipin]

Hi Pascal,

Thanks for the update, I see your point instead of mixing up with int32_t and uint64_t; converting to ' struct ether_addr' makes sense.

I will check internally and thanks once again for the suggestion.

Thanks
Vipin Varghese

> -----Original Message-----
> From: Pascal Mazon [mailto:pascal.mazon@6wind.com]
> Sent: Monday, February 5, 2018 3:55 PM
> To: dev@dpdk.org; Varghese, Vipin <vipin.varghese@intel.com>
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Jain, Deepak K
> <deepak.k.jain@intel.com>; Pascal Mazon <pascal.mazon@6wind.com>
> Subject: [PATCH v2] net/tap: allow user MAC to be passed as args
> 
> From: Vipin Varghese <vipin.varghese@intel.com>
> 
> Allow TAP PMD to pass user desired MAC address as argument.
> The argument value is processed as string, where each 2 bytes are converted to
> HEX MAC address after validation.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
> 
> Hi Vipin,
> 
> I suggest the following patch for your MAC address argument, if that suits you.
> I removed ETH_TAP_MAC_STR_FXD and ETH_TAP_MAC_STR_USR that were not
> really useful. I find it nicer to deal with MAC mostly inside set_mac_type().
> What do you think? Can you test that it fits your needs?
> 
> Best regards,
> Pascal
> 
>  doc/guides/nics/tap.rst       |  6 ++++
>  drivers/net/tap/rte_eth_tap.c | 80 +++++++++++++++++++++++++++++-----------
> ---
>  2 files changed, 60 insertions(+), 26 deletions(-)
> 
> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst index
> dc6f834ca0a1..6b083c846b21 100644
> --- a/doc/guides/nics/tap.rst
> +++ b/doc/guides/nics/tap.rst
> @@ -69,6 +69,12 @@ 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]``.
> 
> +   --vdev=net_tap0,mac="00:64:74:61:70:11"
> +
> +The MAC address will have a user value passed as string. The MAC
> +address is in format with delimeter ``:``. The string is byte converted
> +to hex and you get the actual MAC address: ``00:64:74:61:70:11``.
> +
>  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
> 29d635613c0b..d9e5ec92b8f0 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -49,6 +49,10 @@
>  #define ETH_TAP_MAC_ARG         "mac"
>  #define ETH_TAP_MAC_FIXED       "fixed"
> 
> +#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
> +#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
> +#define ETH_TAP_MAC_ARG_FMT     "["ETH_TAP_MAC_FIXED "|"
> ETH_TAP_USR_MAC_FMT"]"
> +
>  static struct rte_vdev_driver pmd_tap_drv;
> 
>  static const char *valid_arguments[] = { @@ -278,15 +282,8 @@
> tap_rx_offload_get_queue_capa(void)
>  static bool
>  tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)  {
> -	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
> -	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
> -	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
> -
> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> -	    offloads)
> -		return false;
> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> -		return false;
> +	(void)dev;
> +	(void)offloads;
>  	return true;
>  }
> 
> @@ -1335,7 +1332,7 @@ static const struct eth_dev_ops ops = {
> 
>  static int
>  eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> -		   char *remote_iface, int fixed_mac_type)
> +		   char *remote_iface, struct ether_addr *mac_addr)
>  {
>  	int numa_node = rte_socket_id();
>  	struct rte_eth_dev *dev;
> @@ -1397,16 +1394,10 @@ eth_dev_tap_create(struct rte_vdev_device *vdev,
> char *tap_name,
>  		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 {
> +	if (is_zero_ether_addr(mac_addr))
>  		eth_random_addr((uint8_t *)&pmd->eth_addr);
> -	}
> +	else
> +		rte_memcpy(&pmd->eth_addr, mac_addr, sizeof(mac_addr));
> 
>  	/* Immediately create the netdevice (this will create the 1st queue). */
>  	/* rx queue */
> @@ -1577,10 +1568,47 @@ 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;
> +	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte =
> NULL;
> +	struct ether_addr *user_mac = extra_args;
> +	unsigned int index = 0;
> +
> +	if (!value)
> +		return 0;
> +
> +	if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
> +			strlen(ETH_TAP_MAC_FIXED))) {
> +		static int iface_idx;
> +
> +		/* fixed mac = 00:64:74:61:70:<iface_idx> */
> +		memcpy((char *)user_mac->addr_bytes, "\0dtap",
> ETHER_ADDR_LEN);
> +		user_mac->addr_bytes[ETHER_ADDR_LEN - 1] = iface_idx++ +
> '0';
> +		goto success;
> +	}
> +
> +	if (strlen(value) != strlen(ETH_TAP_USR_MAC_FMT))
> +		goto error;
> +
> +	snprintf(mac_temp, sizeof(mac_temp), "%s", value);
> +	mac_byte = strtok(mac_temp, ":");
> +
> +	while ((mac_byte != NULL) &&
> +			strspn(mac_byte, ETH_TAP_CMP_MAC_FMT) &&
> +			strlen(mac_byte) == 2) {
> +		user_mac->addr_bytes[index++] = (char) strtoul(mac_byte,
> NULL, 16);
> +		mac_byte = strtok(NULL, ":");
> +	}
> +
> +	if (index != 6)
> +		goto error;
> +
> +success:
> +	RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
>  	return 0;
> +
> +error:
> +	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
> +		value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
> +	return -1;
>  }
> 
>  /* Open a TAP interface device.
> @@ -1594,7 +1622,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>  	int speed;
>  	char tap_name[RTE_ETH_NAME_MAX_LEN];
>  	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> -	int fixed_mac_type = 0;
> +	struct ether_addr user_mac = {0};
> 
>  	name = rte_vdev_device_name(dev);
>  	params = rte_vdev_device_args(dev);
> @@ -1640,7 +1668,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>  				ret = rte_kvargs_process(kvlist,
>  							 ETH_TAP_MAC_ARG,
>  							 &set_mac_type,
> -							 &fixed_mac_type);
> +							 &user_mac);
>  				if (ret == -1)
>  					goto leave;
>  			}
> @@ -1651,7 +1679,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>  	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
>  		name, tap_name);
> 
> -	ret = eth_dev_tap_create(dev, tap_name, remote_iface,
> fixed_mac_type);
> +	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
> 
>  leave:
>  	if (ret == -1) {
> @@ -1716,5 +1744,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_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT
>  			      ETH_TAP_REMOTE_ARG "=<string>");
> --
> 2.11.0

Re: [dpdk-dev] [PATCH v2] net/tap: allow user MAC to be passed as args

[Varghese, Vipin]

Hi Pascal,

Please find my observation inline

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Varghese, Vipin
> Sent: Monday, February 5, 2018 3:59 PM
> To: Pascal Mazon <pascal.mazon@6wind.com>; dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Jain, Deepak K
> <deepak.k.jain@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] net/tap: allow user MAC to be passed as args
> 
> Hi Pascal,
> 
> Thanks for the update, I see your point instead of mixing up with int32_t and
> uint64_t; converting to ' struct ether_addr' makes sense.
> 
> I will check internally and thanks once again for the suggestion.
> 
> Thanks
> Vipin Varghese
> 
> > -----Original Message-----
> > From: Pascal Mazon [mailto:pascal.mazon@6wind.com]
> > Sent: Monday, February 5, 2018 3:55 PM
> > To: dev@dpdk.org; Varghese, Vipin <vipin.varghese@intel.com>
> > Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Jain, Deepak K
> > <deepak.k.jain@intel.com>; Pascal Mazon <pascal.mazon@6wind.com>
> > Subject: [PATCH v2] net/tap: allow user MAC to be passed as args
> >
> > From: Vipin Varghese <vipin.varghese@intel.com>
> >
> > Allow TAP PMD to pass user desired MAC address as argument.
> > The argument value is processed as string, where each 2 bytes are
> > converted to HEX MAC address after validation.
> >
> > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> > Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> > ---
> >
> > Hi Vipin,
> >
> > I suggest the following patch for your MAC address argument, if that suits you.
> > I removed ETH_TAP_MAC_STR_FXD and ETH_TAP_MAC_STR_USR that were
> not
> > really useful. I find it nicer to deal with MAC mostly inside set_mac_type().
> > What do you think? Can you test that it fits your needs?
> >
> > Best regards,
> > Pascal
> >
> >  doc/guides/nics/tap.rst       |  6 ++++
> >  drivers/net/tap/rte_eth_tap.c | 80
> > +++++++++++++++++++++++++++++-----------
> > ---
> >  2 files changed, 60 insertions(+), 26 deletions(-)
> >
> > diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst index
> > dc6f834ca0a1..6b083c846b21 100644
> > --- a/doc/guides/nics/tap.rst
> > +++ b/doc/guides/nics/tap.rst

<Snipped>

> > +
> > +	while ((mac_byte != NULL) &&
> > +			strspn(mac_byte, ETH_TAP_CMP_MAC_FMT) &&

Added ' strspn(mac_byte + 1, ETH_TAP_CMP_MAC_FMT) &&'; because 2 character validation was not taking place

<Snipped>

> >
> >  /* Open a TAP interface device.
> > @@ -1594,7 +1622,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> >  	int speed;
> >  	char tap_name[RTE_ETH_NAME_MAX_LEN];
> >  	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> > -	int fixed_mac_type = 0;
> > +	struct ether_addr user_mac = {0};

Changed to ' struct ether_addr user_mac = { .addr_bytes={0}};'; to prevent compilation failure.

> >
> >  	name = rte_vdev_device_name(dev);
> >  	params = rte_vdev_device_args(dev);
> > @@ -1640,7 +1668,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> >  				ret = rte_kvargs_process(kvlist,
> >  							 ETH_TAP_MAC_ARG,
> >  							 &set_mac_type,
> > -							 &fixed_mac_type);
> > +							 &user_mac);
> >  				if (ret == -1)
> >  					goto leave;
> >  			}
> > @@ -1651,7 +1679,7 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
> >  	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
> >  		name, tap_name);
> >
> > -	ret = eth_dev_tap_create(dev, tap_name, remote_iface,
> > fixed_mac_type);
> > +	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
> >
> >  leave:
> >  	if (ret == -1) {
> > @@ -1716,5 +1744,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_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT
> >  			      ETH_TAP_REMOTE_ARG "=<string>");
> > --
> > 2.11.0

With these changes it is reviewed from my end.

Re: [dpdk-dev] [PATCH v2] net/tap: allow user MAC to be passed as args

[Ferruh Yigit]

On 2/5/2018 3:54 PM, Pascal Mazon wrote:
> From: Vipin Varghese <vipin.varghese@intel.com>
> 
> Allow TAP PMD to pass user desired MAC address as argument.
> The argument value is processed as string, where each 2 bytes
> are converted to HEX MAC address after validation.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>

<...>

 @@ -278,15 +282,8 @@ tap_rx_offload_get_queue_capa(void)
>  static bool
>  tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
>  {
> -	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
> -	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
> -	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
> -
> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> -	    offloads)
> -		return false;
> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> -		return false;
> +	(void)dev;
> +	(void)offloads;

This part seems unrelated with the patch?

[dpdk-dev] [PATCH v1] net/tap: allow user MAC to be passed as args

[Vipin Varghese]

Allow TAP PMD to pass user desired MAC address as argument.
The argument value is processed as string delimited by  ':',
is parsed and converted to HEX MAC address after validation.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
---

Changes:
 - seggrated the function for hex sting validation - Ferruh
 - Fixed the logic lookup for MAC address
---
 drivers/net/tap/rte_eth_tap.c | 75 +++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 17 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 9d39384..5f67d51 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -48,6 +48,10 @@
 #define ETH_TAP_MAC_ARG         "mac"
 #define ETH_TAP_MAC_FIXED       "fixed"
 
+#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
+#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
+#define ETH_TAP_MAC_ARG_FMT     ETH_TAP_MAC_FIXED "|" ETH_TAP_USR_MAC_FMT
+
 static struct rte_vdev_driver pmd_tap_drv;
 
 static const char *valid_arguments[] = {
@@ -1335,7 +1339,7 @@ enum ioctl_mode {
 
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
-		   char *remote_iface, int fixed_mac_type)
+		   char *remote_iface, struct ether_addr *mac_addr)
 {
 	int numa_node = rte_socket_id();
 	struct rte_eth_dev *dev;
@@ -1397,16 +1401,10 @@ enum ioctl_mode {
 		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 {
+	if (is_zero_ether_addr(mac_addr))
 		eth_random_addr((uint8_t *)&pmd->eth_addr);
-	}
+	else
+		rte_memcpy(&pmd->eth_addr, mac_addr, sizeof(mac_addr));
 
 	/* Immediately create the netdevice (this will create the 1st queue). */
 	/* rx queue */
@@ -1567,15 +1565,58 @@ enum ioctl_mode {
 	return 0;
 }
 
+static int parse_user_mac(struct ether_addr *user_mac,
+		const char *value)
+{
+	unsigned int index = 0;
+	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte = NULL;
+
+	if (user_mac == NULL || value == NULL)
+		return 0;
+
+	snprintf(mac_temp, sizeof(mac_temp), "%s", value);
+	mac_byte = strtok(mac_temp, ":");
+
+	while ((mac_byte != NULL) &&
+			(strlen(mac_byte) <= 2) &&
+			(strlen(mac_byte) == strspn(mac_byte,
+					ETH_TAP_CMP_MAC_FMT))) {
+		user_mac->addr_bytes[index++] = strtoul(mac_byte, NULL, 16);
+		mac_byte = strtok(NULL, ":");
+	}
+
+	return index;
+}
+
 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;
+	struct ether_addr *user_mac = extra_args;
+
+	if (!value)
+		return 0;
+
+	if (!strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED))) {
+		static int iface_idx;
+
+		/* fixed mac = 00:64:74:61:70:<iface_idx> */
+		memcpy((char *)user_mac->addr_bytes, "\0dtap", ETHER_ADDR_LEN);
+		user_mac->addr_bytes[ETHER_ADDR_LEN - 1] = iface_idx++ + '0';
+		goto success;
+	}
+
+	if (parse_user_mac(user_mac, value) != 6)
+		goto error;
+success:
+	RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
 	return 0;
+
+error:
+	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
+		value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
+	return -1;
 }
 
 /* Open a TAP interface device.
@@ -1589,7 +1630,7 @@ enum ioctl_mode {
 	int speed;
 	char tap_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
-	int fixed_mac_type = 0;
+	struct ether_addr user_mac;
 
 	name = rte_vdev_device_name(dev);
 	params = rte_vdev_device_args(dev);
@@ -1626,7 +1667,7 @@ enum ioctl_mode {
 				ret = rte_kvargs_process(kvlist,
 							 ETH_TAP_MAC_ARG,
 							 &set_mac_type,
-							 &fixed_mac_type);
+							 &user_mac);
 				if (ret == -1)
 					goto leave;
 			}
@@ -1637,7 +1678,7 @@ enum ioctl_mode {
 	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
 		name, tap_name);
 
-	ret = eth_dev_tap_create(dev, tap_name, remote_iface, fixed_mac_type);
+	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
 
 leave:
 	if (ret == -1) {
@@ -1701,5 +1742,5 @@ enum ioctl_mode {
 RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
 RTE_PMD_REGISTER_PARAM_STRING(net_tap,
 			      ETH_TAP_IFACE_ARG "=<string> "
-			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
+			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT " "
 			      ETH_TAP_REMOTE_ARG "=<string>");
-- 
1.9.1

Re: [dpdk-dev] [PATCH v1] net/tap: allow user MAC to be passed as args

[Pascal Mazon]

Acked-by: Pascal Mazon <pascal.mazon@6wind.com>


On 12/02/2018 15:44, Vipin Varghese wrote:
> Allow TAP PMD to pass user desired MAC address as argument.
> The argument value is processed as string delimited by  ':',
> is parsed and converted to HEX MAC address after validation.
>
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
>
> Changes:
>  - seggrated the function for hex sting validation - Ferruh
>  - Fixed the logic lookup for MAC address
> ---
>  drivers/net/tap/rte_eth_tap.c | 75 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 58 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 9d39384..5f67d51 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -48,6 +48,10 @@
>  #define ETH_TAP_MAC_ARG         "mac"
>  #define ETH_TAP_MAC_FIXED       "fixed"
>  
> +#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
> +#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
> +#define ETH_TAP_MAC_ARG_FMT     ETH_TAP_MAC_FIXED "|" ETH_TAP_USR_MAC_FMT
> +
>  static struct rte_vdev_driver pmd_tap_drv;
>  
>  static const char *valid_arguments[] = {
> @@ -1335,7 +1339,7 @@ enum ioctl_mode {
>  
>  static int
>  eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
> -		   char *remote_iface, int fixed_mac_type)
> +		   char *remote_iface, struct ether_addr *mac_addr)
>  {
>  	int numa_node = rte_socket_id();
>  	struct rte_eth_dev *dev;
> @@ -1397,16 +1401,10 @@ enum ioctl_mode {
>  		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 {
> +	if (is_zero_ether_addr(mac_addr))
>  		eth_random_addr((uint8_t *)&pmd->eth_addr);
> -	}
> +	else
> +		rte_memcpy(&pmd->eth_addr, mac_addr, sizeof(mac_addr));
>  
>  	/* Immediately create the netdevice (this will create the 1st queue). */
>  	/* rx queue */
> @@ -1567,15 +1565,58 @@ enum ioctl_mode {
>  	return 0;
>  }
>  
> +static int parse_user_mac(struct ether_addr *user_mac,
> +		const char *value)
> +{
> +	unsigned int index = 0;
> +	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte = NULL;
> +
> +	if (user_mac == NULL || value == NULL)
> +		return 0;
> +
> +	snprintf(mac_temp, sizeof(mac_temp), "%s", value);
> +	mac_byte = strtok(mac_temp, ":");
> +
> +	while ((mac_byte != NULL) &&
> +			(strlen(mac_byte) <= 2) &&
> +			(strlen(mac_byte) == strspn(mac_byte,
> +					ETH_TAP_CMP_MAC_FMT))) {
> +		user_mac->addr_bytes[index++] = strtoul(mac_byte, NULL, 16);
> +		mac_byte = strtok(NULL, ":");
> +	}
> +
> +	return index;
> +}
> +
>  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;
> +	struct ether_addr *user_mac = extra_args;
> +
> +	if (!value)
> +		return 0;
> +
> +	if (!strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED))) {
> +		static int iface_idx;
> +
> +		/* fixed mac = 00:64:74:61:70:<iface_idx> */
> +		memcpy((char *)user_mac->addr_bytes, "\0dtap", ETHER_ADDR_LEN);
> +		user_mac->addr_bytes[ETHER_ADDR_LEN - 1] = iface_idx++ + '0';
> +		goto success;
> +	}
> +
> +	if (parse_user_mac(user_mac, value) != 6)
> +		goto error;
> +success:
> +	RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
>  	return 0;
> +
> +error:
> +	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
> +		value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
> +	return -1;
>  }
>  
>  /* Open a TAP interface device.
> @@ -1589,7 +1630,7 @@ enum ioctl_mode {
>  	int speed;
>  	char tap_name[RTE_ETH_NAME_MAX_LEN];
>  	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> -	int fixed_mac_type = 0;
> +	struct ether_addr user_mac;
>  
>  	name = rte_vdev_device_name(dev);
>  	params = rte_vdev_device_args(dev);
> @@ -1626,7 +1667,7 @@ enum ioctl_mode {
>  				ret = rte_kvargs_process(kvlist,
>  							 ETH_TAP_MAC_ARG,
>  							 &set_mac_type,
> -							 &fixed_mac_type);
> +							 &user_mac);
>  				if (ret == -1)
>  					goto leave;
>  			}
> @@ -1637,7 +1678,7 @@ enum ioctl_mode {
>  	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
>  		name, tap_name);
>  
> -	ret = eth_dev_tap_create(dev, tap_name, remote_iface, fixed_mac_type);
> +	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
>  
>  leave:
>  	if (ret == -1) {
> @@ -1701,5 +1742,5 @@ enum ioctl_mode {
>  RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
>  RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>  			      ETH_TAP_IFACE_ARG "=<string> "
> -			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
> +			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT " "
>  			      ETH_TAP_REMOTE_ARG "=<string>");

Re: [dpdk-dev] [PATCH v1] net/tap: allow user MAC to be passed as args

[Ferruh Yigit]

On 2/12/2018 2:44 PM, Vipin Varghese wrote:
> Allow TAP PMD to pass user desired MAC address as argument.
> The argument value is processed as string delimited by  ':',
> is parsed and converted to HEX MAC address after validation.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>

<...>

> @@ -1589,7 +1630,7 @@ enum ioctl_mode {
>  	int speed;
>  	char tap_name[RTE_ETH_NAME_MAX_LEN];
>  	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> -	int fixed_mac_type = 0;
> +	struct ether_addr user_mac;
>  
>  	name = rte_vdev_device_name(dev);
>  	params = rte_vdev_device_args(dev);
> @@ -1626,7 +1667,7 @@ enum ioctl_mode {
>  				ret = rte_kvargs_process(kvlist,
>  							 ETH_TAP_MAC_ARG,
>  							 &set_mac_type,
> -							 &fixed_mac_type);
> +							 &user_mac);
>  				if (ret == -1)
>  					goto leave;
>  			}
> @@ -1637,7 +1678,7 @@ enum ioctl_mode {
>  	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
>  		name, tap_name);
>  
> -	ret = eth_dev_tap_create(dev, tap_name, remote_iface, fixed_mac_type);
> +	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);

"user_mac" without initial value is leading error when no "mac" argument is
provided. It should be zeroed out.

Re: [dpdk-dev] [PATCH v1] net/tap: allow user MAC to be passed as args

[Varghese, Vipin]

Hi Ferruh,

You are correct about this, I will add initialization send a next version patch.

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, March 6, 2018 4:42 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> pascal.mazon@6wind.com
> Cc: Jain, Deepak K <deepak.k.jain@intel.com>
> Subject: Re: [PATCH v1] net/tap: allow user MAC to be passed as args
> 
> On 2/12/2018 2:44 PM, Vipin Varghese wrote:
> > Allow TAP PMD to pass user desired MAC address as argument.
> > The argument value is processed as string delimited by  ':', is parsed
> > and converted to HEX MAC address after validation.
> >
> > Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> > Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> 
> <...>
> 
> > @@ -1589,7 +1630,7 @@ enum ioctl_mode {
> >  	int speed;
> >  	char tap_name[RTE_ETH_NAME_MAX_LEN];
> >  	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> > -	int fixed_mac_type = 0;
> > +	struct ether_addr user_mac;
> >
> >  	name = rte_vdev_device_name(dev);
> >  	params = rte_vdev_device_args(dev);
> > @@ -1626,7 +1667,7 @@ enum ioctl_mode {
> >  				ret = rte_kvargs_process(kvlist,
> >  							 ETH_TAP_MAC_ARG,
> >  							 &set_mac_type,
> > -							 &fixed_mac_type);
> > +							 &user_mac);
> >  				if (ret == -1)
> >  					goto leave;
> >  			}
> > @@ -1637,7 +1678,7 @@ enum ioctl_mode {
> >  	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
> >  		name, tap_name);
> >
> > -	ret = eth_dev_tap_create(dev, tap_name, remote_iface,
> fixed_mac_type);
> > +	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
> 
> "user_mac" without initial value is leading error when no "mac" argument is
> provided. It should be zeroed out.

Re: [dpdk-dev] [PATCH v2] net/tap: allow user MAC to be passed as args

[Ferruh Yigit]

On 3/12/2018 8:21 PM, Vipin Varghese wrote:
> @@ -1591,7 +1632,7 @@ enum ioctl_mode {
>  	int speed;
>  	char tap_name[RTE_ETH_NAME_MAX_LEN];
>  	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> -	int fixed_mac_type = 0;
> +	struct ether_addr user_mac = { .addr_bytes = {0, 0, 0, 0, 0, 0} };

A single '0' will do the job J
struct ether_addr user_mac = { .addr_bytes = {0} };

Would you mind sending a new version?

Re: [dpdk-dev] [PATCH v2] net/tap: allow user MAC to be passed as args

[Varghese, Vipin]

Sure Ferruh, I will spin a new one.

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, March 12, 2018 8:39 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> pascal.mazon@6wind.com
> Subject: Re: [PATCH v2] net/tap: allow user MAC to be passed as args
> 
> On 3/12/2018 8:21 PM, Vipin Varghese wrote:
> > @@ -1591,7 +1632,7 @@ enum ioctl_mode {
> >  	int speed;
> >  	char tap_name[RTE_ETH_NAME_MAX_LEN];
> >  	char remote_iface[RTE_ETH_NAME_MAX_LEN];
> > -	int fixed_mac_type = 0;
> > +	struct ether_addr user_mac = { .addr_bytes = {0, 0, 0, 0, 0, 0} };
> 
> A single '0' will do the job J
> struct ether_addr user_mac = { .addr_bytes = {0} };
> 
> Would you mind sending a new version?

Re: [dpdk-dev] [PATCH v2] net/tap: allow user MAC to be passed as args

[Stephen Hemminger]

On Tue, 13 Mar 2018 01:51:34 +0530
Vipin Varghese <vipin.varghese@intel.com> wrote:

> +static int parse_user_mac(struct ether_addr *user_mac,
> +		const char *value)
> +{
> +	unsigned int index = 0;
> +	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte = NULL;
> +
> +	if (user_mac == NULL || value == NULL)
> +		return 0;
> +
> +	snprintf(mac_temp, sizeof(mac_temp), "%s", value);
> +	mac_byte = strtok(mac_temp, ":");
> +
> +	while ((mac_byte != NULL) &&
> +			(strlen(mac_byte) <= 2) &&
> +			(strlen(mac_byte) == strspn(mac_byte,
> +					ETH_TAP_CMP_MAC_FMT))) {
> +		user_mac->addr_bytes[index++] = strtoul(mac_byte, NULL, 16);
> +		mac_byte = strtok(NULL, ":");
> +	}
> +
> +	return index;
> +}

We need rte_ether_aton or better yet replace the DPDK custom definition of 
"struct ether_addr" with the with the one from Linux/BSD <netinet/ether.h>
There is no value in having a private version of all these routines...

Re: [dpdk-dev] [PATCH v3] net/tap: allow user MAC to be passed as args

[Ferruh Yigit]

On 3/12/2018 9:53 PM, Vipin Varghese wrote:
> Allow TAP PMD to pass user desired MAC address as argument.
> The argument value is processed as string delimited by  ':',
> is parsed and converted to HEX MAC address after validation.
> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> Acked-by: Pascal Mazon <pascal.mazon@6wind.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

[dpdk-dev] [PATCH v2] net/tap: allow user MAC to be passed as args

[Vipin Varghese]

Allow TAP PMD to pass user desired MAC address as argument.
The argument value is processed as string delimited by  ':',
is parsed and converted to HEX MAC address after validation.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Acked-by: Pascal Mazon <pascal.mazon@6wind.com>
---

Changes in V2:
 - updated the auto variable for user_mac - Ferruh
 - added documentation update - Vipin
---
 doc/guides/nics/tap.rst       |  6 ++++
 drivers/net/tap/rte_eth_tap.c | 75 +++++++++++++++++++++++++++++++++----------
 2 files changed, 64 insertions(+), 17 deletions(-)

diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
index ea61be3..76eb0bd 100644
--- a/doc/guides/nics/tap.rst
+++ b/doc/guides/nics/tap.rst
@@ -37,6 +37,12 @@ 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]``.
 
+   --vdev=net_tap0,mac="00:64:74:61:70:11"
+
+The MAC address will have a user value passed as string. The MAC address is in
+format with delimeter ``:``. The string is byte converted to hex and you get
+the actual MAC address: ``00:64:74:61:70:11``.
+
 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 f09db0e..6a20802 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -48,6 +48,10 @@
 #define ETH_TAP_MAC_ARG         "mac"
 #define ETH_TAP_MAC_FIXED       "fixed"
 
+#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
+#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
+#define ETH_TAP_MAC_ARG_FMT     ETH_TAP_MAC_FIXED "|" ETH_TAP_USR_MAC_FMT
+
 static struct rte_vdev_driver pmd_tap_drv;
 
 static const char *valid_arguments[] = {
@@ -1337,7 +1341,7 @@ enum ioctl_mode {
 
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
-		   char *remote_iface, int fixed_mac_type)
+		   char *remote_iface, struct ether_addr *mac_addr)
 {
 	int numa_node = rte_socket_id();
 	struct rte_eth_dev *dev;
@@ -1399,16 +1403,10 @@ enum ioctl_mode {
 		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 {
+	if (is_zero_ether_addr(mac_addr))
 		eth_random_addr((uint8_t *)&pmd->eth_addr);
-	}
+	else
+		rte_memcpy(&pmd->eth_addr, mac_addr, sizeof(mac_addr));
 
 	/* Immediately create the netdevice (this will create the 1st queue). */
 	/* rx queue */
@@ -1569,15 +1567,58 @@ enum ioctl_mode {
 	return 0;
 }
 
+static int parse_user_mac(struct ether_addr *user_mac,
+		const char *value)
+{
+	unsigned int index = 0;
+	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte = NULL;
+
+	if (user_mac == NULL || value == NULL)
+		return 0;
+
+	snprintf(mac_temp, sizeof(mac_temp), "%s", value);
+	mac_byte = strtok(mac_temp, ":");
+
+	while ((mac_byte != NULL) &&
+			(strlen(mac_byte) <= 2) &&
+			(strlen(mac_byte) == strspn(mac_byte,
+					ETH_TAP_CMP_MAC_FMT))) {
+		user_mac->addr_bytes[index++] = strtoul(mac_byte, NULL, 16);
+		mac_byte = strtok(NULL, ":");
+	}
+
+	return index;
+}
+
 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;
+	struct ether_addr *user_mac = extra_args;
+
+	if (!value)
+		return 0;
+
+	if (!strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED))) {
+		static int iface_idx;
+
+		/* fixed mac = 00:64:74:61:70:<iface_idx> */
+		memcpy((char *)user_mac->addr_bytes, "\0dtap", ETHER_ADDR_LEN);
+		user_mac->addr_bytes[ETHER_ADDR_LEN - 1] = iface_idx++ + '0';
+		goto success;
+	}
+
+	if (parse_user_mac(user_mac, value) != 6)
+		goto error;
+success:
+	RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
 	return 0;
+
+error:
+	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
+		value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
+	return -1;
 }
 
 /* Open a TAP interface device.
@@ -1591,7 +1632,7 @@ enum ioctl_mode {
 	int speed;
 	char tap_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
-	int fixed_mac_type = 0;
+	struct ether_addr user_mac = { .addr_bytes = {0, 0, 0, 0, 0, 0} };
 
 	name = rte_vdev_device_name(dev);
 	params = rte_vdev_device_args(dev);
@@ -1628,7 +1669,7 @@ enum ioctl_mode {
 				ret = rte_kvargs_process(kvlist,
 							 ETH_TAP_MAC_ARG,
 							 &set_mac_type,
-							 &fixed_mac_type);
+							 &user_mac);
 				if (ret == -1)
 					goto leave;
 			}
@@ -1639,7 +1680,7 @@ enum ioctl_mode {
 	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
 		name, tap_name);
 
-	ret = eth_dev_tap_create(dev, tap_name, remote_iface, fixed_mac_type);
+	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
 
 leave:
 	if (ret == -1) {
@@ -1703,5 +1744,5 @@ enum ioctl_mode {
 RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
 RTE_PMD_REGISTER_PARAM_STRING(net_tap,
 			      ETH_TAP_IFACE_ARG "=<string> "
-			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
+			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT " "
 			      ETH_TAP_REMOTE_ARG "=<string>");
-- 
1.9.1

[dpdk-dev] [PATCH v3] net/tap: allow user MAC to be passed as args

[Vipin Varghese]

Allow TAP PMD to pass user desired MAC address as argument.
The argument value is processed as string delimited by  ':',
is parsed and converted to HEX MAC address after validation.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Acked-by: Pascal Mazon <pascal.mazon@6wind.com>
---

Changed in V3:
 - change the default init for user_mac - Ferruh

Changes in V2:
 - updated the auto variable for user_mac - Ferruh
 - added documentation update - Vipin
---
 doc/guides/nics/tap.rst       |  6 ++++
 drivers/net/tap/rte_eth_tap.c | 75 +++++++++++++++++++++++++++++++++----------
 2 files changed, 64 insertions(+), 17 deletions(-)

diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
index ea61be3..76eb0bd 100644
--- a/doc/guides/nics/tap.rst
+++ b/doc/guides/nics/tap.rst
@@ -37,6 +37,12 @@ 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]``.
 
+   --vdev=net_tap0,mac="00:64:74:61:70:11"
+
+The MAC address will have a user value passed as string. The MAC address is in
+format with delimeter ``:``. The string is byte converted to hex and you get
+the actual MAC address: ``00:64:74:61:70:11``.
+
 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 f09db0e..3e4f7a8 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -48,6 +48,10 @@
 #define ETH_TAP_MAC_ARG         "mac"
 #define ETH_TAP_MAC_FIXED       "fixed"
 
+#define ETH_TAP_USR_MAC_FMT     "xx:xx:xx:xx:xx:xx"
+#define ETH_TAP_CMP_MAC_FMT     "0123456789ABCDEFabcdef"
+#define ETH_TAP_MAC_ARG_FMT     ETH_TAP_MAC_FIXED "|" ETH_TAP_USR_MAC_FMT
+
 static struct rte_vdev_driver pmd_tap_drv;
 
 static const char *valid_arguments[] = {
@@ -1337,7 +1341,7 @@ enum ioctl_mode {
 
 static int
 eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
-		   char *remote_iface, int fixed_mac_type)
+		   char *remote_iface, struct ether_addr *mac_addr)
 {
 	int numa_node = rte_socket_id();
 	struct rte_eth_dev *dev;
@@ -1399,16 +1403,10 @@ enum ioctl_mode {
 		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 {
+	if (is_zero_ether_addr(mac_addr))
 		eth_random_addr((uint8_t *)&pmd->eth_addr);
-	}
+	else
+		rte_memcpy(&pmd->eth_addr, mac_addr, sizeof(mac_addr));
 
 	/* Immediately create the netdevice (this will create the 1st queue). */
 	/* rx queue */
@@ -1569,15 +1567,58 @@ enum ioctl_mode {
 	return 0;
 }
 
+static int parse_user_mac(struct ether_addr *user_mac,
+		const char *value)
+{
+	unsigned int index = 0;
+	char mac_temp[strlen(ETH_TAP_USR_MAC_FMT) + 1], *mac_byte = NULL;
+
+	if (user_mac == NULL || value == NULL)
+		return 0;
+
+	snprintf(mac_temp, sizeof(mac_temp), "%s", value);
+	mac_byte = strtok(mac_temp, ":");
+
+	while ((mac_byte != NULL) &&
+			(strlen(mac_byte) <= 2) &&
+			(strlen(mac_byte) == strspn(mac_byte,
+					ETH_TAP_CMP_MAC_FMT))) {
+		user_mac->addr_bytes[index++] = strtoul(mac_byte, NULL, 16);
+		mac_byte = strtok(NULL, ":");
+	}
+
+	return index;
+}
+
 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;
+	struct ether_addr *user_mac = extra_args;
+
+	if (!value)
+		return 0;
+
+	if (!strncasecmp(ETH_TAP_MAC_FIXED, value, strlen(ETH_TAP_MAC_FIXED))) {
+		static int iface_idx;
+
+		/* fixed mac = 00:64:74:61:70:<iface_idx> */
+		memcpy((char *)user_mac->addr_bytes, "\0dtap", ETHER_ADDR_LEN);
+		user_mac->addr_bytes[ETHER_ADDR_LEN - 1] = iface_idx++ + '0';
+		goto success;
+	}
+
+	if (parse_user_mac(user_mac, value) != 6)
+		goto error;
+success:
+	RTE_LOG(DEBUG, PMD, "TAP user MAC param (%s)\n", value);
 	return 0;
+
+error:
+	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s|%s)\n",
+		value, ETH_TAP_MAC_FIXED, ETH_TAP_USR_MAC_FMT);
+	return -1;
 }
 
 /* Open a TAP interface device.
@@ -1591,7 +1632,7 @@ enum ioctl_mode {
 	int speed;
 	char tap_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
-	int fixed_mac_type = 0;
+	struct ether_addr user_mac = { .addr_bytes = {0} };
 
 	name = rte_vdev_device_name(dev);
 	params = rte_vdev_device_args(dev);
@@ -1628,7 +1669,7 @@ enum ioctl_mode {
 				ret = rte_kvargs_process(kvlist,
 							 ETH_TAP_MAC_ARG,
 							 &set_mac_type,
-							 &fixed_mac_type);
+							 &user_mac);
 				if (ret == -1)
 					goto leave;
 			}
@@ -1639,7 +1680,7 @@ enum ioctl_mode {
 	RTE_LOG(NOTICE, PMD, "Initializing pmd_tap for %s as %s\n",
 		name, tap_name);
 
-	ret = eth_dev_tap_create(dev, tap_name, remote_iface, fixed_mac_type);
+	ret = eth_dev_tap_create(dev, tap_name, remote_iface, &user_mac);
 
 leave:
 	if (ret == -1) {
@@ -1703,5 +1744,5 @@ enum ioctl_mode {
 RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
 RTE_PMD_REGISTER_PARAM_STRING(net_tap,
 			      ETH_TAP_IFACE_ARG "=<string> "
-			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
+			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT " "
 			      ETH_TAP_REMOTE_ARG "=<string>");
-- 
1.9.1

Re: [dpdk-dev] [PATCH v3] net/tap: allow user MAC to be passed as args

[Ferruh Yigit]

On 3/12/2018 5:45 PM, Ferruh Yigit wrote:
> On 3/12/2018 9:53 PM, Vipin Varghese wrote:
>> Allow TAP PMD to pass user desired MAC address as argument.
>> The argument value is processed as string delimited by  ':',
>> is parsed and converted to HEX MAC address after validation.
>>
>> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
>> Acked-by: Pascal Mazon <pascal.mazon@6wind.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

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

Re: [dpdk-dev] [PATCH v2] net/tap: allow user mac to be passed as args

[Ferruh Yigit]

On 12/21/2017 4:01 PM, Vipin Varghese wrote:
> Added fixes for user TAP user MAC
> 1) user format to RTE_PMD_REGISTER_PARAM_STRING
> 2) TAP to the RTE_LOG in absence of dynamic RTE_LOG
> 3) Boundary case for MAC string added
> > ---------------------------------------------------------------
> 
> Other Details:
> 1) not to extract "string to mac" conversion to its own function
> 2) set_mac_type does not take any pmd or device name
> 3) Segault for boundary cases 'mac="01:01:01:01:01:01', not found
> 4) Added user MAC format string

Hi Vipin,

Patch version changes shouldn't be part of git commit. But it is good to put
them after "---" in commit log. Can you please update the commit log?

Also please add maintainer to the cc/to .

> 
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>

<...>

> -				strncpy(macTemp, value, 18);
> -
> -				macByte = strtok(macTemp, ":");
> -				while ((macByte != NULL) &&
> -					(strspn(macByte, "0123456789ABCDEFabcdef")) &&
> -					(strspn((macByte + 1), "0123456789ABCDEFabcdef")) &&
> -					(strlen(macByte) == 2)) {
> -					user_mac[index] = strtoul(macByte, NULL, 16);
> -					macByte = strtok(NULL, ":");
> +				strncpy(mac_temp, value, 18);
> +				mac_temp[19] = "\0";

This cause a build error [1], should be used '\0':

[1]
...drivers/net/tap/rte_eth_tap.c: In function ‘set_mac_type’:
...drivers/net/tap/rte_eth_tap.c:1467:18: error: assignment makes integer from
pointer without a cast [-Werror=int-conversion]
     mac_temp[19] = "\0";
                  ^
<....>

> -			      ETH_TAP_IFACE_ARG "=<string> "
> -			      ETH_TAP_SPEED_ARG "=<int> "
> -			      ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED " "
> -			      ETH_TAP_REMOTE_ARG "=<string>");
> +	ETH_TAP_IFACE_ARG "=<string> "
> +	ETH_TAP_SPEED_ARG "=<int> "
> +	ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED "|" ETH_TAP_USER_MAC_FMT " "
> +	ETH_TAP_REMOTE_ARG "=<string>");

This also build a build error [2] because of how ETH_TAP_USER_MAC_FMT defined [3].

[2]
.../drivers/net/tap/rte_eth_tap.c: At top level:
.../drivers/net/tap/rte_eth_tap.c:45:33: error: called object is not a function
or function pointer
 #define ETH_TAP_IFACE_ARG       "iface"
                                 ^
.../build/include/rte_dev.h:265:25: note: in definition of macro
‘RTE_PMD_REGISTER_PARAM_STRING’
 __attribute__((used)) = str
                         ^~~
.../drivers/net/tap/rte_eth_tap.c:1628:2: note: in expansion of macro
‘ETH_TAP_IFACE_ARG’
  ETH_TAP_IFACE_ARG "=<string> "
  ^~~~~~~~~~~~~~~~~
.../drivers/net/tap/rte_eth_tap.c:1630:65: error: expected ‘,’ or ‘;’ before
string constant
  ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED "|" ETH_TAP_USER_MAC_FMT " "
                                                                 ^
.../build/include/rte_dev.h:265:25: note: in definition of macro
‘RTE_PMD_REGISTER_PARAM_STRING’
 __attribute__((used)) = str
                         ^~~

[3]
#define ETH_TAP_USER_MAC_FMT ("xx:xx:xx:xx:xx:xx")
parenthesis ...

[dpdk-dev] [PATCH v2] net/tap: allow user mac to be passed as args

[Vipin Varghese]

Added fixes for user TAP user MAC
1) user format to RTE_PMD_REGISTER_PARAM_STRING
2) TAP to the RTE_LOG in absence of dynamic RTE_LOG
3) Boundary case for MAC string added

---------------------------------------------------------------

Other Details:
1) not to extract "string to mac" conversion to its own function
2) set_mac_type does not take any pmd or device name
3) Segault for boundary cases 'mac="01:01:01:01:01:01', not found
4) Added user MAC format string

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 drivers/net/tap/rte_eth_tap.c | 80 ++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 39 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 0c53458..85c12af 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -81,6 +81,11 @@
 #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
 #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
 
+#define MAC_STRING_NULL (0)
+#define MAC_STRING_FIXED (1)
+#define MAC_STRING_USER (2)
+#define ETH_TAP_USER_MAC_FMT ("xx:xx:xx:xx:xx:xx")
+
 static unsigned char user_mac[ETHER_ADDR_LEN];
 
 static struct rte_vdev_driver pmd_tap_drv;
@@ -1293,17 +1298,18 @@ enum ioctl_mode {
 		pmd->txq[i].fd = -1;
 	}
 
-	if (fixed_mac_type == 1) {
+	if (fixed_mac_type == MAC_STRING_FIXED) {
 		/* 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 if (fixed_mac_type == 2) {
-		/* user mac is recieved */
+	} else if (fixed_mac_type == MAC_STRING_USER) {
+		/* user mac is received */
 		RTE_LOG(INFO, PMD,
-			"Using user MAC (%02x:%02x:%02x:%02x:%02x:%02x)\n",
+			"%s; Using user MAC (%02x:%02x:%02x:%02x:%02x:%02x) argument\n",
+			pmd->name,
 			user_mac[0], user_mac[1], user_mac[2],
 			user_mac[3], user_mac[4], user_mac[5]);
 		rte_memcpy(&pmd->eth_addr, user_mac, ETHER_ADDR_LEN);
@@ -1476,53 +1482,49 @@ enum ioctl_mode {
 }
 
 static int
-set_mac_type(const char *key __rte_unused,
-	     const char *value,
-	     void *extra_args)
+set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
 {
-	char macTemp[20], *macByte = NULL;
+	char mac_temp[20] = {0}, *mac_byte = NULL;
 	unsigned int index = 0;
 
 	if (value) {
+		RTE_LOG(DEBUG, PMD, "TAP user MAC (%s) to set.\n", value);
+
 		if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
 			strlen(ETH_TAP_MAC_FIXED))) {
-			*(int *)extra_args = 1;
+			*(int *)extra_args = MAC_STRING_FIXED;
 		} else {
-			RTE_LOG(INFO, PMD, "User shared MAC param (%s)\n",
-				value);
-
 			/* desired format aa:bb:cc:dd:ee:ff:11 */
 			if (strlen(value) == 17) {
-				strncpy(macTemp, value, 18);
-
-				macByte = strtok(macTemp, ":");
-				while ((macByte != NULL) &&
-					(strspn(macByte, "0123456789ABCDEFabcdef")) &&
-					(strspn((macByte + 1), "0123456789ABCDEFabcdef")) &&
-					(strlen(macByte) == 2)) {
-					user_mac[index] = strtoul(macByte, NULL, 16);
-					macByte = strtok(NULL, ":");
+				strncpy(mac_temp, value, 18);
+				mac_temp[19] = "\0";
+				mac_byte = strtok(mac_temp, ":");
+
+				while ((mac_byte != NULL) &&
+					(strspn(mac_byte, "0123456789ABCDEFabcdef")) &&
+					(strspn((mac_byte + 1), "0123456789ABCDEFabcdef")) &&
+					(strlen(mac_byte) == 2)) {
+					user_mac[index] = strtoul(mac_byte, NULL, 16);
+					mac_byte = strtok(NULL, ":");
 					index += 1;
 				}
 
-				if (index != 6) {
-					RTE_LOG(ERR, PMD, " failure in MAC value (%s) at %u\n",
-						macByte, index + 1);
-					return -1;
-				}
+				if (index != 6)
+					goto error;
 
-				RTE_LOG(DEBUG, PMD, " User MAC (%s) considered\n",
-					value);
-				*(int *)extra_args = 2;
-			} else {
-				RTE_LOG(ERR, PMD, " Mismatch on format for (%s)\n",
-					value);
-				return -1;
-			}
+				*(int *)extra_args = MAC_STRING_USER;
+			} else
+				goto error;
 		}
+		RTE_LOG(DEBUG, PMD, "TAP user MAC (%s) considered\n", value);
 	}
 
 	return 0;
+
+error:
+	RTE_LOG(ERR, PMD, "TAP user MAC (%s) is not in format (%s)\n",
+		value, ETH_TAP_USER_MAC_FMT);
+	return -1;
 }
 
 /* Open a TAP interface device.
@@ -1536,7 +1538,7 @@ enum ioctl_mode {
 	int speed;
 	char tap_name[RTE_ETH_NAME_MAX_LEN];
 	char remote_iface[RTE_ETH_NAME_MAX_LEN];
-	int fixed_mac_type = 0;
+	int fixed_mac_type = MAC_STRING_NULL;
 
 	name = rte_vdev_device_name(dev);
 	params = rte_vdev_device_args(dev);
@@ -1656,7 +1658,7 @@ enum ioctl_mode {
 RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
 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>");
+	ETH_TAP_IFACE_ARG "=<string> "
+	ETH_TAP_SPEED_ARG "=<int> "
+	ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_FIXED "|" ETH_TAP_USER_MAC_FMT " "
+	ETH_TAP_REMOTE_ARG "=<string>");
-- 
1.9.1