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

[Wiles, Keith]

> On Apr 11, 2017, at 8:23 AM, Wiles, Keith <keith.wiles at intel.com> wrote:
> 
> 
>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon at 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 at 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 at 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