On 9/10/2018 5:55 PM, Juhamatti Kuusisaari wrote:
> At the moment, PCAP interfaces use dummy MAC by default. This change
> adds support for selecting PCAP physical interface MAC with phy_mac=1
> devarg. This allows to setup packet flows using the physical interface
> MAC.
>
> Signed-off-by: Juhamatti Kuusisaari <juhamatti.kuusisaari@coriant.com>
>
> ---
> v6:
> * Review changes:
> * Clarified devarg applicability (applies to iface-only)
> * Introduced detailed documentation for the devarg
> * More checkpatches-fixes
> v5-v3:
> * FreeBSD related compilation and checkpatches-fixes
> v2:
> * FreeBSD support introduced
> * Release notes added
> v1:
> * phy_mac=1 devarg support
<...>
> +#elif defined(__FreeBSD__)
Just to double check did you check/verify below code on FreeBSD?
<...>
> @@ -955,6 +1034,10 @@ eth_from_pcaps_common(struct rte_vdev_device *vdev,
> else
> (*internals)->if_index = if_nametoindex(pair->value);
>
> + if (phy_mac && pair) /* phy_mac arg is applied only to iface */
Having this comment is good, but "iface" is so generic, it may be confusing for
beyond this context, what about "only if iface devarg provided" kind of detail?
<...>
> @@ -989,6 +1073,19 @@ 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) {
> + const int phy_mac = atoi(value);
> + int *enable_phy_mac = extra_args;
> +
> + if (phy_mac)
> + *enable_phy_mac = 1;
> + }
> + return 0;
> +}
This is causing build error because of "key" not used, needs __rte_unused marker.
<...>
> @@ -1031,6 +1129,16 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
> * reading / writing
> */
> if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) {
> + /*
> + * We check first whether we want to use phy MAC of the PCAP
> + * interface.
> + */
> + if (rte_kvargs_count(kvlist, ETH_PCAP_PHY_MAC_ARG)) {
Do you need count check at all?
> + ret = rte_kvargs_process(kvlist, ETH_PCAP_PHY_MAC_ARG,
> + &select_phy_mac, &phy_mac);
> + if (ret < 0)
> + goto free_kvlist;
> + }
I would prefer to see this block below ETH_PCAP_IFACE_ARG check, this block is
for "iface", so it makes more sense to me first verify it, later verify phy_mac
Hello Ferruh,
> On 9/10/2018 5:55 PM, Juhamatti Kuusisaari wrote:
> > At the moment, PCAP interfaces use dummy MAC by default. This change
> > adds support for selecting PCAP physical interface MAC with phy_mac=1
> > devarg. This allows to setup packet flows using the physical interface
> > MAC.
> >
> > Signed-off-by: Juhamatti Kuusisaari <juhamatti.kuusisaari@coriant.com>
> >
> > ---
> > v6:
> > * Review changes:
> > * Clarified devarg applicability (applies to iface-only)
> > * Introduced detailed documentation for the devarg
> > * More checkpatches-fixes
> > v5-v3:
> > * FreeBSD related compilation and checkpatches-fixes
> > v2:
> > * FreeBSD support introduced
> > * Release notes added
> > v1:
> > * phy_mac=1 devarg support
>
> <...>
>
> > +#elif defined(__FreeBSD__)
>
> Just to double check did you check/verify below code on FreeBSD?
I did run a manual test of the added code and seems to work. I think it is still better to run DPDK on it to make sure everything works, so I'll do that before sending next revision.
>
> <...>
>
> > @@ -955,6 +1034,10 @@ eth_from_pcaps_common(struct
> rte_vdev_device *vdev,
> > else
> > (*internals)->if_index = if_nametoindex(pair->value);
> >
> > + if (phy_mac && pair) /* phy_mac arg is applied only to iface */
>
> Having this comment is good, but "iface" is so generic, it may be confusing for
> beyond this context, what about "only if iface devarg provided" kind of detail?
Yes, let's elaborate that.
> <...>
>
> > @@ -989,6 +1073,19 @@ 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) {
> > + const int phy_mac = atoi(value);
> > + int *enable_phy_mac = extra_args;
> > +
> > + if (phy_mac)
> > + *enable_phy_mac = 1;
> > + }
> > + return 0;
> > +}
>
> This is causing build error because of "key" not used, needs __rte_unused
> marker.
Yes, need to fix this.
> <...>
>
> > @@ -1031,6 +1129,16 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
> > * reading / writing
> > */
> > if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) {
> > + /*
> > + * We check first whether we want to use phy MAC of the
> PCAP
> > + * interface.
> > + */
> > + if (rte_kvargs_count(kvlist, ETH_PCAP_PHY_MAC_ARG)) {
>
> Do you need count check at all?
I think it is needed, as the arg may not exist there. At least to me calling process directly does not feel right, even if it would work.
> > + ret = rte_kvargs_process(kvlist,
> ETH_PCAP_PHY_MAC_ARG,
> > + &select_phy_mac, &phy_mac);
> > + if (ret < 0)
> > + goto free_kvlist;
> > + }
>
> I would prefer to see this block below ETH_PCAP_IFACE_ARG check, this
> block is
> for "iface", so it makes more sense to me first verify it, later verify phy_mac
Sure, I'll add it.
As there is already pcap-related MAC address patch merged, I'll need to do a rebase and recheck this patch functionality on top of that. It will take some time (which is a scarce resource at the moment), so I'll try to get this in again after 18.11.
Thanks,
--
Juhamatti
@@ -96,6 +96,15 @@ The different stream types are:
iface=eth0
+Runtime Config Options
+^^^^^^^^^^^^^^^^^^^^^^
+
+- ``Use PCAP interface physical MAC``
+
+ In case ``iface=`` configuration is set, user may want to use the selected interface's physical MAC
+ address. This can be done with a ``devarg`` ``phy_mac``, for example::
+ iface=eth0,phy_mac=1
+
Examples of Usage
^^^^^^^^^^^^^^^^^
@@ -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
-----------
@@ -7,6 +7,14 @@
#include <time.h>
#include <net/if.h>
+#include <sys/socket.h>
+#include <sys/ioctl.h>
+#include <unistd.h>
+
+#if defined(__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,80 @@ 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)
+{
+ void *mac_addrs;
+ struct ifreq ifr;
+
+ PMD_LOG(INFO, "Setting phy MAC for %s\n", if_name);
+#if !defined(__FreeBSD__)
+ int if_fd = socket(AF_INET, SOCK_DGRAM, 0);
+ if (if_fd == -1)
+ return;
+
+ 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);
+#elif defined(__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_malloc(NULL, len, 0);
+ 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;
+ 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 +1034,10 @@ eth_from_pcaps_common(struct rte_vdev_device *vdev,
else
(*internals)->if_index = if_nametoindex(pair->value);
+ if (phy_mac && pair) /* phy_mac arg is applied only to iface */
+ eth_pcap_update_mac(pair->value, eth_dev,
+ vdev->device.numa_node);
+
return 0;
}
@@ -962,7 +1045,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 +1053,8 @@ 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, ð_dev);
+ tx_queues, nb_tx_queues, kvlist,
+ &internals, phy_mac, ð_dev);
if (ret < 0)
return ret;
@@ -989,6 +1073,19 @@ 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) {
+ const int phy_mac = atoi(value);
+ int *enable_phy_mac = extra_args;
+
+ 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);
@@ -1031,6 +1129,16 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
* reading / writing
*/
if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) {
+ /*
+ * We check first whether we want to use phy MAC of the 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;
+ }
ret = rte_kvargs_process(kvlist, ETH_PCAP_IFACE_ARG,
&open_rx_tx_iface, &pcaps);
@@ -1084,7 +1192,8 @@ 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 +1237,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)
{