[dpdk-dev,v1] net/tap: allow user mac to be passed as args
Checks
Commit Message
One of the uses cases from customer site is use TAP PMD to intake
user specific MAC address during probe. This allows applications
make use of interfaces with desired MAC.
Extending MAC argumentinfrastructure for tap PMD; we pass custom
MAC address in string format (sample - 11:22:33:44:55:66).
Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
drivers/net/tap/rte_eth_tap.c | 56 +++++++++++++++++++++++++++++++++++++++----
1 file changed, 52 insertions(+), 4 deletions(-)
Comments
On 11/30/2017 11:49 AM, Vipin Varghese wrote:
> One of the uses cases from customer site is use TAP PMD to intake
> user specific MAC address during probe. This allows applications
> make use of interfaces with desired MAC.
>
> Extending MAC argumentinfrastructure for tap PMD; we pass custom
> MAC address in string format (sample - 11:22:33:44:55:66).
Overall lgtm, please check a few comments below.
>
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 56 +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 6b27679..0c53458 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -81,6 +81,8 @@
> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>
> +static unsigned char user_mac[ETHER_ADDR_LEN];
> +
> static struct rte_vdev_driver pmd_tap_drv;
>
> static const char *valid_arguments[] = {
> @@ -1291,13 +1293,20 @@ enum ioctl_mode {
> pmd->txq[i].fd = -1;
> }
>
> - if (fixed_mac_type) {
> + if (fixed_mac_type == 1) {
Instead of hardcoded type values 1 & 2, can you please use macros?
> /* 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 */
s/recieved/received
> + RTE_LOG(INFO, PMD,
> + "Using user MAC (%02x:%02x:%02x:%02x:%02x:%02x)\n",
> + 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);
> }
> @@ -1471,9 +1480,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 macTemp[20], *macByte = NULL;
> + unsigned int index = 0;
> +
> + if (value) {
> + if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
> + strlen(ETH_TAP_MAC_FIXED))) {
> + *(int *)extra_args = 1;
> + } else {
> + RTE_LOG(INFO, PMD, "User shared MAC param (%s)\n",
> + value);
Is this log needs to be in INFO level, since there is already and info level log
when MAC set, what about making this debug?
> +
> + /* desired format aa:bb:cc:dd:ee:ff:11 */
Can you please update RTE_PMD_REGISTER_PARAM_STRING with new param, put expected
format info there as well?
> + 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, ":");
> + index += 1;
> + }
I would extract the string to mac logic into its own function, but up to you.
> +
> + if (index != 6) {
> + RTE_LOG(ERR, PMD, " failure in MAC value (%s) at %u\n",
> + macByte, index + 1);
> + return -1;
And this is not related to this patch, but just as reminder, when a virtual
driver probe fails vdev bus stops probing with an error, so all remaining
virtual devices are not probed, in case one might want to fix ;)
> + }
> +
> + RTE_LOG(DEBUG, PMD, " User MAC (%s) considered\n",
"User defined MAC" ? And can you please add an port identifier, port_id or
device name or interface name, for the case there are multiple tap device to
identify which one get user defined MAC.
> + value);
> + *(int *)extra_args = 2;
> + } else {
> + RTE_LOG(ERR, PMD, " Mismatch on format for (%s)\n",
> + value);
> + return -1;
> + }
> + }
> + }
> +
> return 0;
> }
>
>
Hi,
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.
/* desired format aa:bb:cc:dd:ee:ff:11 */ The :11 goes beyond
standard MAC addresses ;-)
The commit log has few mistakes:
- missing "to" after "applications"
- "argumentinfrastructure"
I otherwise concur with Ferruh's remarks.
Best regards,
Pascal
On 07/12/2017 21:11, Ferruh Yigit wrote:
> On 11/30/2017 11:49 AM, Vipin Varghese wrote:
>> One of the uses cases from customer site is use TAP PMD to intake
>> user specific MAC address during probe. This allows applications
>> make use of interfaces with desired MAC.
>>
>> Extending MAC argumentinfrastructure for tap PMD; we pass custom
>> MAC address in string format (sample - 11:22:33:44:55:66).
> Overall lgtm, please check a few comments below.
>
>> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 56 +++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 52 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 6b27679..0c53458 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -81,6 +81,8 @@
>> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
>> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>>
>> +static unsigned char user_mac[ETHER_ADDR_LEN];
>> +
>> static struct rte_vdev_driver pmd_tap_drv;
>>
>> static const char *valid_arguments[] = {
>> @@ -1291,13 +1293,20 @@ enum ioctl_mode {
>> pmd->txq[i].fd = -1;
>> }
>>
>> - if (fixed_mac_type) {
>> + if (fixed_mac_type == 1) {
> Instead of hardcoded type values 1 & 2, can you please use macros?
>
>> /* 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 */
> s/recieved/received
>
>> + RTE_LOG(INFO, PMD,
>> + "Using user MAC (%02x:%02x:%02x:%02x:%02x:%02x)\n",
>> + 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);
>> }
>> @@ -1471,9 +1480,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 macTemp[20], *macByte = NULL;
>> + unsigned int index = 0;
>> +
>> + if (value) {
>> + if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
>> + strlen(ETH_TAP_MAC_FIXED))) {
>> + *(int *)extra_args = 1;
>> + } else {
>> + RTE_LOG(INFO, PMD, "User shared MAC param (%s)\n",
>> + value);
> Is this log needs to be in INFO level, since there is already and info level log
> when MAC set, what about making this debug?
>
>> +
>> + /* desired format aa:bb:cc:dd:ee:ff:11 */
> Can you please update RTE_PMD_REGISTER_PARAM_STRING with new param, put expected
> format info there as well?
>
>> + 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, ":");
>> + index += 1;
>> + }
> I would extract the string to mac logic into its own function, but up to you.
>
>> +
>> + if (index != 6) {
>> + RTE_LOG(ERR, PMD, " failure in MAC value (%s) at %u\n",
>> + macByte, index + 1);
>> + return -1;
> And this is not related to this patch, but just as reminder, when a virtual
> driver probe fails vdev bus stops probing with an error, so all remaining
> virtual devices are not probed, in case one might want to fix ;)
>
>> + }
>> +
>> + RTE_LOG(DEBUG, PMD, " User MAC (%s) considered\n",
> "User defined MAC" ? And can you please add an port identifier, port_id or
> device name or interface name, for the case there are multiple tap device to
> identify which one get user defined MAC.
>
>> + value);
>> + *(int *)extra_args = 2;
>> + } else {
>> + RTE_LOG(ERR, PMD, " Mismatch on format for (%s)\n",
>> + value);
>> + return -1;
>> + }
>> + }
>> + }
>> +
>> return 0;
>> }
>>
>>
On 11/30/2017 11:49 AM, Vipin Varghese wrote:
> One of the uses cases from customer site is use TAP PMD to intake user
> specific MAC address during probe. This allows applications make use
> of interfaces with desired MAC.
>
> Extending MAC argumentinfrastructure for tap PMD; we pass custom MAC
> address in string format (sample - 11:22:33:44:55:66).
Overall lgtm, please check a few comments below.
>
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 56
> +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c
> b/drivers/net/tap/rte_eth_tap.c index 6b27679..0c53458 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -81,6 +81,8 @@
> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0) #define
> FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>
> +static unsigned char user_mac[ETHER_ADDR_LEN];
> +
> static struct rte_vdev_driver pmd_tap_drv;
>
> static const char *valid_arguments[] = { @@ -1291,13 +1293,20 @@ enum
> ioctl_mode {
> pmd->txq[i].fd = -1;
> }
>
> - if (fixed_mac_type) {
> + if (fixed_mac_type == 1) {
Instead of hardcoded type values 1 & 2, can you please use macros?
>> Ok, Adding MACROs MAC_STRING_NULL, MAC_STRING_FIXED and MAC_STRING_USER
> /* 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 */
s/recieved/received
>> Ok
> + RTE_LOG(INFO, PMD,
> + "Using user MAC (%02x:%02x:%02x:%02x:%02x:%02x)\n",
> + 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);
> }
> @@ -1471,9 +1480,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 macTemp[20], *macByte = NULL;
> + unsigned int index = 0;
> +
> + if (value) {
> + if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
> + strlen(ETH_TAP_MAC_FIXED))) {
> + *(int *)extra_args = 1;
> + } else {
> + RTE_LOG(INFO, PMD, "User shared MAC param (%s)\n",
> + value);
Is this log needs to be in INFO level, since there is already and info level log when MAC set, what about making this debug?
>> Ok
> +
> + /* desired format aa:bb:cc:dd:ee:ff:11 */
Can you please update RTE_PMD_REGISTER_PARAM_STRING with new param, put expected format info there as well?
>> Ok
> + 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, ":");
> + index += 1;
> + }
I would extract the string to mac logic into its own function, but up to you.
>> not done
> +
> + if (index != 6) {
> + RTE_LOG(ERR, PMD, " failure in MAC value (%s) at %u\n",
> + macByte, index + 1);
> + return -1;
And this is not related to this patch, but just as reminder, when a virtual driver probe fails vdev bus stops probing with an error, so all remaining virtual devices are not probed, in case one might want to fix ;)
> + }
> +
> + RTE_LOG(DEBUG, PMD, " User MAC (%s) considered\n",
"User defined MAC" ? And can you please add an port identifier, port_id or device name or interface name, for the case there are multiple tap device to identify which one get user defined MAC.
VV> not done, each device in DPDK rte_eal_init is sequentially scanned and probed, hence log starts with 'iface,[parameters]', then probe, create and init is called for TAP PMD one after another.
With minimalistic logging style the current format is kept.
> + value);
> + *(int *)extra_args = 2;
> + } else {
> + RTE_LOG(ERR, PMD, " Mismatch on format for (%s)\n",
> + value);
> + return -1;
> + }
> + }
> + }
> +
> return 0;
> }
>
>
@@ -81,6 +81,8 @@
#define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
#define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
+static unsigned char user_mac[ETHER_ADDR_LEN];
+
static struct rte_vdev_driver pmd_tap_drv;
static const char *valid_arguments[] = {
@@ -1291,13 +1293,20 @@ enum ioctl_mode {
pmd->txq[i].fd = -1;
}
- if (fixed_mac_type) {
+ if (fixed_mac_type == 1) {
/* 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 */
+ RTE_LOG(INFO, PMD,
+ "Using user MAC (%02x:%02x:%02x:%02x:%02x:%02x)\n",
+ 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);
}
@@ -1471,9 +1480,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 macTemp[20], *macByte = NULL;
+ unsigned int index = 0;
+
+ if (value) {
+ if (!strncasecmp(ETH_TAP_MAC_FIXED, value,
+ strlen(ETH_TAP_MAC_FIXED))) {
+ *(int *)extra_args = 1;
+ } 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, ":");
+ index += 1;
+ }
+
+ if (index != 6) {
+ RTE_LOG(ERR, PMD, " failure in MAC value (%s) at %u\n",
+ macByte, index + 1);
+ return -1;
+ }
+
+ 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;
+ }
+ }
+ }
+
return 0;
}