[v5] net/pcap: physical interface MAC address support

  Support for PCAP physical interface MAC with phy_mac=1 devarg.

Signed-off-by: Juhamatti Kuusisaari <juhamatti.kuusisaari@coriant.com>
---
 doc/guides/rel_notes/release_18_11.rst |   4 +
 drivers/net/pcap/rte_eth_pcap.c        | 119 +++++++++++++++++++++++--
 2 files changed, 118 insertions(+), 5 deletions(-)
  

On 9/6/2018 5:56 PM, Juhamatti Kuusisaari wrote:
> Support for PCAP physical interface MAC with phy_mac=1 devarg.
> 
> Signed-off-by: Juhamatti Kuusisaari <juhamatti.kuusisaari@coriant.com>

Hi Juhamatti,

Thanks for the patch.

Can you please add a little more details into commit log that why this new
feature is enabled, what is the use case?

Also can you please send new version of the patches as a reply to previous one,
- via "--in-reply-to" git send-email argument. And a history after commit log
what changed in new version. These helps tracing the multiple versions.

> ---
>  doc/guides/rel_notes/release_18_11.rst |   4 +

Please update doc/guides/nics/pcap_ring.rst to document new devargs.

>  drivers/net/pcap/rte_eth_pcap.c        | 119 +++++++++++++++++++++++--
>  2 files changed, 118 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/release_18_11.rst b/doc/guides/rel_notes/release_18_11.rst
> index 3ae6b3f58..70966740a 100644
> --- a/doc/guides/rel_notes/release_18_11.rst
> +++ b/doc/guides/rel_notes/release_18_11.rst
> @@ -54,6 +54,10 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =========================================================
>  
> +* **Added a devarg to use PCAP interface physical MAC address.**
> +  A new devarg ``phy_mac`` was introduced to allow users to use physical
> +  MAC address of the selected PCAP interface.
> +
>  
>  API Changes
>  -----------
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index e8810a171..8917c4c4d 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -7,6 +7,14 @@
>  #include <time.h>
>  
>  #include <net/if.h>
> +#include <sys/socket.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +#ifdef __FreeBSD__
> +#include <sys/sysctl.h>
> +#include <net/if_dl.h>
> +#endif
>  
>  #include <pcap.h>
>  
> @@ -17,6 +25,7 @@
>  #include <rte_malloc.h>
>  #include <rte_mbuf.h>
>  #include <rte_bus_vdev.h>
> +#include <rte_string_fns.h>
>  
>  #define RTE_ETH_PCAP_SNAPSHOT_LEN 65535
>  #define RTE_ETH_PCAP_SNAPLEN ETHER_MAX_JUMBO_FRAME_LEN
> @@ -29,6 +38,7 @@
>  #define ETH_PCAP_RX_IFACE_IN_ARG "rx_iface_in"
>  #define ETH_PCAP_TX_IFACE_ARG "tx_iface"
>  #define ETH_PCAP_IFACE_ARG    "iface"
> +#define ETH_PCAP_PHY_MAC_ARG  "phy_mac"
>  
>  #define ETH_PCAP_ARG_MAXLEN	64
>  
> @@ -87,6 +97,7 @@ static const char *valid_arguments[] = {
>  	ETH_PCAP_RX_IFACE_IN_ARG,
>  	ETH_PCAP_TX_IFACE_ARG,
>  	ETH_PCAP_IFACE_ARG,
> +	ETH_PCAP_PHY_MAC_ARG,
>  	NULL
>  };
>  
> @@ -904,12 +915,79 @@ pmd_init_internals(struct rte_vdev_device *vdev,
>  	return 0;
>  }
>  
> +static void eth_pcap_update_mac(const char *if_name, struct rte_eth_dev **eth_dev,
> +		const unsigned int numa_node)

Please follow convention, to have return type in a separate line above.

> +{
> +	void *mac_addrs;
> +	PMD_LOG(INFO, "Setting phy MAC for %s\n",
> +			if_name);

This can be single line. And please leave an empty line between variable
declaration and code.

> +#ifndef __FreeBSD__
> +	int if_fd = socket(AF_INET, SOCK_DGRAM, 0);
> +	if (if_fd != -1) {

To reduce the indentation of rest of the code, it helps to reverse the logic,
exit on failure and continue without indentation.

> +		struct ifreq ifr;
> +		strlcpy(ifr.ifr_name, if_name, sizeof(ifr.ifr_name));
> +		if (!ioctl(if_fd, SIOCGIFHWADDR, &ifr)) {
> +			mac_addrs = rte_zmalloc_socket(NULL, ETHER_ADDR_LEN,
> +					0, numa_node);
> +			if (mac_addrs) {
> +				(*eth_dev)->data->mac_addrs = mac_addrs;
> +				rte_memcpy((*eth_dev)->data->mac_addrs,
> +						ifr.ifr_addr.sa_data,
> +						ETHER_ADDR_LEN);
> +			}
> +		}
> +		close(if_fd);
> +	}
> +#else

There is an assumption that it is either Linux or FreeBSD, that is true for now
but Windows is coming :) Better to be safe here and do an "else if" for FreeBSD.

> +	int mib[6];
> +	size_t len = 0;
> +	char *buf = NULL;
> +
> +	mib[0] = CTL_NET;
> +	mib[1] = AF_ROUTE;
> +	mib[2] = 0;
> +	mib[3] = AF_LINK;
> +	mib[4] = NET_RT_IFLIST;
> +	mib[5] = if_nametoindex(if_name);
> +
> +	if (sysctl(mib, 6, NULL, &len, NULL, 0) < 0)
> +		return;
> +
> +	if (len > 0) {
> +		struct if_msghdr	*ifm;
> +		struct sockaddr_dl	*sdl;
> +
> +		buf = rte_zmalloc_socket(NULL, len,
> +				0, numa_node);

This is an interim buffer, no need to use rte_zmalloc_socket API, rte_malloc or
regular malloc will do the same.
Also no need to break the line.

> +		if (buf) {
> +			if (sysctl(mib, 6, buf, &len, NULL, 0) < 0) {
> +				rte_free(buf);
> +				return;
> +			}
> +
> +			ifm = (struct if_msghdr *)buf;
> +			sdl = (struct sockaddr_dl *)(ifm + 1);
> +			mac_addrs = rte_zmalloc_socket(NULL, ETHER_ADDR_LEN,
> +					0, numa_node);
> +			if (mac_addrs) {
> +				(*eth_dev)->data->mac_addrs = mac_addrs;

Just as a heads up, right now the mac_addrs points to global variable so no need
to worry about it, but please beware of a patch does unique mac address for each
interface.
For that case will need to free the mac_addrs before this assignment, based on
which patch goes in, you may need to update this part.

Patch https://patches.dpdk.org/patch/44170/

> +				rte_memcpy((*eth_dev)->data->mac_addrs,
> +						LLADDR(sdl),
> +						ETHER_ADDR_LEN);
> +			}
> +		}
> +	}
> +	if (buf)
> +		rte_free(buf);
> +#endif
> +}
> +
>  static int
>  eth_from_pcaps_common(struct rte_vdev_device *vdev,
>  		struct pmd_devargs *rx_queues, const unsigned int nb_rx_queues,
>  		struct pmd_devargs *tx_queues, const unsigned int nb_tx_queues,
>  		struct rte_kvargs *kvlist, struct pmd_internals **internals,
> -		struct rte_eth_dev **eth_dev)
> +		const int phy_mac, struct rte_eth_dev **eth_dev)
>  {
>  	struct rte_kvargs_pair *pair = NULL;
>  	unsigned int k_idx;
> @@ -955,6 +1033,9 @@ eth_from_pcaps_common(struct rte_vdev_device *vdev,
>  	else
>  		(*internals)->if_index = if_nametoindex(pair->value);
>  
> +	if (phy_mac && pair)
> +		eth_pcap_update_mac(pair->value, eth_dev, vdev->device.numa_node);

This is a little misleading.

pair will be set only for ETH_PCAP_IFACE_ARG ("iface") argument, it won't work
with rx_iface[_in]/tx_iface arguments. Like below two are same:
1) iface=lo
2) rx_iface=lo,tx_iface=lo

But this code will work only with 1) not 2), better to mention in document.


Also pcap supports multiple queue by repeating devarg, like:
rx_iface=eth0,rx_iface=eth1,rx_iface=eth2,tx_pcap=tx.pcap
This will create tree Rx queues, one for each interface.

Right now "iface" argument is hardcoded to single queue, but without a good
reason, when it changes in the future to support multiple queue, ethdev will use
the MAC from first one, better to clarify in the doc update, or perhaps in the
code comment too.


> +
>  	return 0;
>  }
>  
> @@ -962,7 +1043,7 @@ static int
>  eth_from_pcaps(struct rte_vdev_device *vdev,
>  		struct pmd_devargs *rx_queues, const unsigned int nb_rx_queues,
>  		struct pmd_devargs *tx_queues, const unsigned int nb_tx_queues,
> -		struct rte_kvargs *kvlist, int single_iface,
> +		struct rte_kvargs *kvlist, int single_iface, int phy_mac,
>  		unsigned int using_dumpers)
>  {
>  	struct pmd_internals *internals = NULL;
> @@ -970,7 +1051,7 @@ eth_from_pcaps(struct rte_vdev_device *vdev,
>  	int ret;
>  
>  	ret = eth_from_pcaps_common(vdev, rx_queues, nb_rx_queues,
> -		tx_queues, nb_tx_queues, kvlist, &internals, &eth_dev);
> +			tx_queues, nb_tx_queues, kvlist, &internals, phy_mac, &eth_dev);

Please fix ./devtools/checkpatches.sh warnings.

>  
>  	if (ret < 0)
>  		return ret;
> @@ -989,6 +1070,22 @@ eth_from_pcaps(struct rte_vdev_device *vdev,
>  	return 0;
>  }
>  
> +static int
> +select_phy_mac(const char *key, const char *value, void *extra_args)
> +{
> +	if (extra_args && strcmp(key, ETH_PCAP_PHY_MAC_ARG) == 0) {

no need strcmp, it has been done by rte_kvargs_process().

> +		const int phy_mac = atoi(value);
> +		int *enable_phy_mac = extra_args;
> +
> +		if (phy_mac != 0 && phy_mac != 1)
> +			PMD_LOG(WARNING, "Value should be 0 or 1, set it as 1!");

According below logic value doesn't matter, which seems OK, what about removing
the log? You should document expected values in pcap doc.

> +
> +		if (phy_mac)
> +			*enable_phy_mac = 1;
> +	}
> +	return 0;
> +}
> +
>  static int
>  pmd_pcap_probe(struct rte_vdev_device *dev)
>  {
> @@ -999,6 +1096,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>  	struct pmd_devargs dumpers = {0};
>  	struct rte_eth_dev *eth_dev;
>  	int single_iface = 0;
> +	int phy_mac = 0;
>  	int ret;
>  
>  	name = rte_vdev_device_name(dev);
> @@ -1026,6 +1124,16 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>  	if (kvlist == NULL)
>  		return -1;
>  
> +	/*
> +	 * We check whether we want to use phy MAC of pcap interface.
> +	 */
> +	if (rte_kvargs_count(kvlist, ETH_PCAP_PHY_MAC_ARG)) {
> +		ret = rte_kvargs_process(kvlist, ETH_PCAP_PHY_MAC_ARG,
> +				&select_phy_mac, &phy_mac);
> +		if (ret < 0)
> +			goto free_kvlist;
> +	}
> +
>  	/*
>  	 * If iface argument is passed we open the NICs and use them for
>  	 * reading / writing
> @@ -1084,7 +1192,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>  
>  create_eth:
>  	ret = eth_from_pcaps(dev, &pcaps, pcaps.num_of_queue, &dumpers,
> -		dumpers.num_of_queue, kvlist, single_iface, is_tx_pcap);
> +			dumpers.num_of_queue, kvlist, single_iface, phy_mac, is_tx_pcap);
>  
>  free_kvlist:
>  	rte_kvargs_free(kvlist);
> @@ -1128,7 +1236,8 @@ RTE_PMD_REGISTER_PARAM_STRING(net_pcap,
>  	ETH_PCAP_RX_IFACE_ARG "=<ifc> "
>  	ETH_PCAP_RX_IFACE_IN_ARG "=<ifc> "
>  	ETH_PCAP_TX_IFACE_ARG "=<ifc> "
> -	ETH_PCAP_IFACE_ARG "=<ifc>");
> +	ETH_PCAP_IFACE_ARG "=<ifc> "
> +	ETH_PCAP_PHY_MAC_ARG "=<int>");
>  
>  RTE_INIT(eth_pcap_init_log)
>  {
>
  

Hello Ferruh,

> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Monday, September 10, 2018 3:04 PM
>
> Hi Juhamatti,
> 
> Thanks for the patch.
> 
> Can you please add a little more details into commit log that why this new
> feature is enabled, what is the use case?
> 
> Also can you please send new version of the patches as a reply to previous
> one,
> - via "--in-reply-to" git send-email argument. And a history after commit log
> what changed in new version. These helps tracing the multiple versions.

Thanks for the excellent comments and guidance. I did the changes and v6 has been sent to Patchwork queue. The use case is really just to use PCAP interface as one of the interfaces of the application and thus allowing to use also the physical MAC address of the interface. I tried to elaborate this a bit more, I hope it is at least better now.

Seems that I cannot easily get rid of __FreeBSD__ define checkpatches-nag. I assumed that it isn't really mandatory as I can see the same define used elsewhere.

BR,
--
 Juhamatti