On Tue, Mar 13, 2018 at 01:50:37PM +0100, Nelio Laranjeiro wrote:
> VF devices are not able to receive traffic unless it fully requests it
> though Netlink. This will cause the request to be processed by the PF
> which will add/remove the MAC address to the VF table if the VF is trusted.
>
> Signed-off-by: Nelio Laranjeiro <nelio.laranje...@6wind.com>
Same basic comments as on the previous patch:
- Check random capitalization in documentation and comments.
- "vf" => "nl" for functions, objects and files that aren't really
VF-specific but involve Netlink.
More below.
> ---
> drivers/net/mlx5/Makefile | 3 +-
> drivers/net/mlx5/mlx5.c | 7 +
> drivers/net/mlx5/mlx5.h | 6 +
> drivers/net/mlx5/mlx5_mac.c | 25 +++-
> drivers/net/mlx5/mlx5_vf.c | 315
> ++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 353 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile
> index afda4118f..bbda0d1ff 100644
> --- a/drivers/net/mlx5/Makefile
> +++ b/drivers/net/mlx5/Makefile
> @@ -59,6 +59,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_rss.c
> SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_mr.c
> SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_flow.c
> SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_socket.c
> +SRCS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += mlx5_vf.c
>
> ifeq ($(CONFIG_RTE_LIBRTE_MLX5_DLOPEN_DEPS),y)
> INSTALL-$(CONFIG_RTE_LIBRTE_MLX5_PMD)-lib += $(LIB_GLUE)
> @@ -84,7 +85,7 @@ LDLIBS += -libverbs -lmlx5
> endif
> LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
> LDLIBS += -lrte_ethdev -lrte_net -lrte_kvargs
> -LDLIBS += -lrte_bus_pci
> +LDLIBS += -lrte_bus_pci -lrte_netlink
>
> # A few warnings cannot be avoided in external headers.
> CFLAGS += -Wno-error=cast-qual
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index d5cc85d19..e966884bd 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -205,6 +205,13 @@ mlx5_dev_close(struct rte_eth_dev *dev)
> rte_free(priv->reta_idx);
> if (priv->primary_socket)
> mlx5_socket_uninit(dev);
> + if (priv->config.vf) {
> + ret = mlx5_vf_mac_addr_flush(dev);
> + if (ret)
> + DRV_LOG(WARNING,
> + "port %u some MAC address remains configured",
> + dev->data->port_id);
> + }
> ret = mlx5_hrxq_ibv_verify(dev);
> if (ret)
> DRV_LOG(WARNING, "port %u some hash Rx queue still remain",
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
> index 9191b2949..a4333e6a5 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -298,4 +298,10 @@ struct mlx5_mr *mlx5_mr_get(struct rte_eth_dev *dev,
> struct rte_mempool *mp);
> int mlx5_mr_release(struct mlx5_mr *mr);
> int mlx5_mr_verify(struct rte_eth_dev *dev);
>
> +/* mlx5_vf.c */
> +
> +int mlx5_vf_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac);
> +int mlx5_vf_mac_addr_remove(struct rte_eth_dev *dev, struct ether_addr *mac);
> +int mlx5_vf_mac_addr_flush(struct rte_eth_dev *dev);
> +
> #endif /* RTE_PMD_MLX5_H_ */
> diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
> index 01c7ba17a..5137ad11d 100644
> --- a/drivers/net/mlx5/mlx5_mac.c
> +++ b/drivers/net/mlx5/mlx5_mac.c
> @@ -67,11 +67,24 @@ mlx5_get_mac(struct rte_eth_dev *dev, uint8_t
> (*mac)[ETHER_ADDR_LEN])
> void
> mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
> {
> + struct priv *priv = dev->data->dev_private;
> + const int vf = priv->config.vf;
> + int ret;
> +
> assert(index < MLX5_MAX_MAC_ADDRESSES);
> + if (index && vf) {
I don't think checking index is necessary here. mlx5_vf_mac_addr_remove()
will fail if the MAC in question happens to be the last one configured on
the netdevice, but depending on external configuration, it's not necessarily
the same as mac_addrs[0].
Checking "vf" only seems more reliable.
> + ret = mlx5_vf_mac_addr_remove(dev,
> + &dev->data->mac_addrs[index]);
> + if (ret) {
> + DRV_LOG(WARNING,
> + "port %u cannot remove mac address at index %d",
> + dev->data->port_id, index);
> + return;
> + }
> + }
> memset(&dev->data->mac_addrs[index], 0, sizeof(struct ether_addr));
> if (!dev->data->promiscuous) {
> - int ret = mlx5_traffic_restart(dev);
> -
> + ret = mlx5_traffic_restart(dev);
> if (ret)
> DRV_LOG(ERR, "port %u cannot remove mac address: %s",
> dev->data->port_id, strerror(rte_errno));
> @@ -97,6 +110,8 @@ int
> mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
> uint32_t index, uint32_t vmdq __rte_unused)
> {
> + struct priv *priv = dev->data->dev_private;
> + const int vf = priv->config.vf;
> unsigned int i;
>
> assert(index < MLX5_MAX_MAC_ADDRESSES);
> @@ -111,6 +126,12 @@ mlx5_mac_addr_add(struct rte_eth_dev *dev, struct
> ether_addr *mac,
> rte_errno = EADDRINUSE;
> return -rte_errno;
> }
> + if (index && vf) {
Same comment as mlx5_mac_addr_remove() regarding index.
> + int ret = mlx5_vf_mac_addr_add(dev, mac);
> +
> + if (ret)
> + return ret;
> + }
> dev->data->mac_addrs[index] = *mac;
> if (!dev->data->promiscuous)
> return mlx5_traffic_restart(dev);
> diff --git a/drivers/net/mlx5/mlx5_vf.c b/drivers/net/mlx5/mlx5_vf.c
> index 357407f56..3db8ee0eb 100644
> --- a/drivers/net/mlx5/mlx5_vf.c
> +++ b/drivers/net/mlx5/mlx5_vf.c
> @@ -11,12 +11,29 @@
> #include "mlx5.h"
> #include "mlx5_utils.h"
>
> +/*
> + * Define NDA_RTA as defined in iproute2 sources.
> + *
> + * see in iproute2 sources file include/libnetlink.h
> + */
> +#ifndef NDA_RTA
> +#define NDA_RTA(r) \
> + ((struct rtattr *)(((char *)(r)) + NLMSG_ALIGN(sizeof(struct ndmsg))))
> +#endif
> +
I suggest an internal MLX5_* macro instead, just in case.
> /* Data exchanged between Netlink library and PMD. */
> struct mlx5_vf_iface {
> struct rte_eth_dev *dev; /**< Pointer to Ethernet device. */
> int iface_idx; /**< Network Interface index. */
> };
>
> +/* Add/Remove mac address through Metlink */
Remove => remove
Metlink => Netlink
> +struct mlx5_vf_mac_addr {
> + struct ether_addr (*mac)[];
> + /**< MAC Address handled by the device. */
Address => address
> + int mac_n; /**< Number of address in the array. */
address => addresses
(Please check the remaining typos and extra caps.)
> +};
> +
> /**
> * Parse Netlink message to retrieve the interface index.
> *
> @@ -132,3 +149,301 @@ mlx5_vf_iface_idx(struct rte_eth_dev *dev)
> dev->data->port_id, strerror(rte_errno));
> return -rte_errno;
> }
> +
> +/**
> + * Parse Netlink message to retrieve the bridge MAC address.
> + *
> + * @param nh
> + * Pointer to Netlink Message Header.
> + * @param arg
> + * PMD data register with this callback.
> + *
> + * @return
> + * 0 on success, -1 otherwise.
How about a negative errno value in case of error for consistency?
> + */
> +static int
> +mlx5_vf_mac_addr_cb(struct nlmsghdr *nh, void *arg)
> +{
> + struct mlx5_vf_mac_addr *data = arg;
> + struct ndmsg *r = NLMSG_DATA(nh);
> + struct rtattr *attribute;
> + int len;
> +
> + len = nh->nlmsg_len - NLMSG_LENGTH(sizeof(*r));
> + for (attribute = NDA_RTA(r);
> + RTA_OK(attribute, len);
> + attribute = RTA_NEXT(attribute, len)) {
> + if (attribute->rta_type == NDA_LLADDR) {
> + if (data->mac_n == MLX5_MAX_MAC_ADDRESSES) {
> + DRV_LOG(WARNING,
> + "not enough room to finalise the"
> + " request");
> + return -1;
How about -ENOMEM?
> + }
> +#ifndef NDEBUG
> + struct ether_addr m;
> +
> + memcpy(&m, RTA_DATA(attribute), ETHER_ADDR_LEN);
> + DRV_LOG(DEBUG,
> + "brige MAC address"
brige => bridge
> + " %02X:%02X:%02X:%02X:%02X:%02X",
> + m.addr_bytes[0], m.addr_bytes[1],
> + m.addr_bytes[2], m.addr_bytes[3],
> + m.addr_bytes[4], m.addr_bytes[5]);
> +#endif
> + memcpy(&(*data->mac)[data->mac_n++],
> + RTA_DATA(attribute), ETHER_ADDR_LEN);
> + }
> + }
> + return 0;
> +}
> +
> +/**
> + * Get bridge MAC addresses.
> + *
> + * @param dev
> + * Pointer to Ethernet device.
> + * @param mac[out]
> + * Pointer to the array table of MAC addresses to fill.
> + * Its size should be of MLX5_MAX_MAC_ADDRESSES.
> + * @param mac_n[out]
> + * Number of entries filled in MAC array.
> + *
> + * @return
> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> + */
Since the mac_n is only an output value and the size of mac is fixed to
MLX5_MAX_MAC_ADDRESSES, I suggest to return the value of mac_n (positive in
case of success) and remove it from the parameter list.
> +static int
> +mlx5_vf_mac_addr_list(struct rte_eth_dev *dev, struct ether_addr (*mac)[],
Another suggestion for clarity: (*mac)[MLX5_MAX_MAC_ADDRESSES]
> + int *mac_n)
> +{
> + int iface_idx = mlx5_vf_iface_idx(dev);
> + struct {
> + struct nlmsghdr hdr;
> + struct ifinfomsg ifm;
> + } req = {
> + .hdr = {
> + .nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)),
> + .nlmsg_type = RTM_GETNEIGH,
> + .nlmsg_flags = NLM_F_DUMP | NLM_F_REQUEST,
> + },
> + .ifm = {
> + .ifi_family = PF_BRIDGE,
> + .ifi_index = iface_idx,
> + },
> + };
> + struct mlx5_vf_mac_addr data = {
> + .mac = mac,
> + .mac_n = 0,
> + };
> + int fd;
> + int ret;
> +
> + fd = rte_nl_init(RTMGRP_LINK);
> + if (fd < 0) {
> + rte_errno = errno;
> + goto error;
> + }
> + ret = rte_nl_request(fd, &req.hdr, &req.ifm, sizeof(struct ifinfomsg));
> + if (ret < 0) {
> + rte_errno = errno;
> + goto error;
> + }
> + ret = rte_nl_recv(fd, mlx5_vf_mac_addr_cb, &data);
> + if (ret < 0) {
> + rte_errno = errno;
> + goto error;
> + }
> + rte_nl_final(fd);
> + *mac_n = data.mac_n;
> + return 0;
> +error:
> + if (fd >= 0)
> + rte_nl_final(fd);
> + DRV_LOG(DEBUG, "port %u cannot retrieve MAC address list %s",
> + dev->data->port_id, strerror(rte_errno));
> + return -rte_errno;
> +}
> +
> +/**
> + * Modify the mac address neighbour table with Netlink.
mac => MAC
Metlink => Netlink
> + *
> + * @param dev
> + * Pointer to Ethernet device.
> + * @param mac
> + * MAC address to consider.
> + * @param add
> + * 1 to add the MAC address, 0 to remove the MAC address.
> + *
> + * @return
> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +static int
> +mlx5_vf_mac_addr_modify(struct rte_eth_dev *dev, struct ether_addr *mac,
> + int add)
> +{
> + int iface_idx = mlx5_vf_iface_idx(dev);
> + struct {
> + struct nlmsghdr hdr;
> + struct ndmsg ndm;
> + struct rtattr rta;
> + } req = {
> + .hdr = {
> + .nlmsg_len = NLMSG_LENGTH(sizeof(struct ndmsg)),
> + .nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE |
> + NLM_F_EXCL | NLM_F_ACK,
> + .nlmsg_type = add ? RTM_NEWNEIGH : RTM_DELNEIGH,
> + },
> + .ndm = {
> + .ndm_family = PF_BRIDGE,
> + .ndm_state = NUD_NOARP | NUD_PERMANENT,
> + .ndm_ifindex = iface_idx,
> + .ndm_flags = NTF_SELF,
> + },
> + .rta = {
> + .rta_type = NDA_LLADDR,
> + .rta_len = RTA_LENGTH(ETHER_ADDR_LEN),
> + },
> + };
> + int fd;
> + int ret;
> +
> + memcpy(RTA_DATA(&req.rta), mac, ETHER_ADDR_LEN);
> + req.hdr.nlmsg_len = NLMSG_ALIGN(req.hdr.nlmsg_len) +
> + RTA_ALIGN(req.rta.rta_len);
> + fd = rte_nl_init(RTMGRP_LINK);
> + if (fd < 0) {
> + rte_errno = errno;
> + goto error;
> + }
> + ret = rte_nl_send(fd, &req.hdr);
> + if (ret == -1) {
> + rte_errno = errno;
> + goto error;
> + }
> + ret = rte_nl_recv_ack(fd);
> + if (ret == -1) {
> + rte_errno = errno;
> + goto error;
> + }
> + rte_nl_final(fd);
> + return 0;
> +error:
> + if (fd >= 0)
> + rte_nl_final(fd);
> + DRV_LOG(DEBUG,
> + "port %u cannot %s MAC address %02X:%02X:%02X:%02X:%02X:%02X"
> + " %s",
> + dev->data->port_id,
> + add ? "add" : "remove",
> + mac->addr_bytes[0], mac->addr_bytes[1],
> + mac->addr_bytes[2], mac->addr_bytes[3],
> + mac->addr_bytes[4], mac->addr_bytes[5],
> + strerror(rte_errno));
> + return -rte_errno;
> +}
> +
> +/**
> + * add a MAC address.
add => Add (can't stop!)
> + *
> + * @param dev
> + * Pointer to Ethernet device.
> + * @param mac_addr
> + * MAC address to register.
> + *
> + * @return
> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +int
> +mlx5_vf_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac)
> +{
> + struct ether_addr macs[MLX5_MAX_MAC_ADDRESSES];
> + int macs_n = 0;
> + int i;
> + int mac_index = MLX5_MAX_MAC_ADDRESSES;
> + int ret;
> +
> + ret = mlx5_vf_mac_addr_list(dev, &macs, &macs_n);
> + if (ret)
> + return ret;
> + for (i = 0; i != macs_n; ++i) {
> + if (!memcmp(&macs[i], mac, ETHER_ADDR_LEN)) {
> + mac_index = i;
> + break;
> + }
> + }
> + if (mac_index != MLX5_MAX_MAC_ADDRESSES) {
> + DRV_LOG(INFO, "port %u MAC address already added",
added => present
> + dev->data->port_id);
> + rte_errno = EADDRINUSE;
> + return -rte_errno;
> + }
> + return mlx5_vf_mac_addr_modify(dev, mac, 1);
> +}
> +
> +/**
> + * Remove a MAC address.
> + *
> + * @param dev
> + * Pointer to Ethernet device.
> + * @param index
> + * MAC address to remove.
> + *
> + * @return
> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +int
> +mlx5_vf_mac_addr_remove(struct rte_eth_dev *dev, struct ether_addr *mac)
> +{
> + struct ether_addr macs[MLX5_MAX_MAC_ADDRESSES];
> + int macs_n = 0;
> + int i;
> + int mac_index = MLX5_MAX_MAC_ADDRESSES;
> + int ret;
> +
> + ret = mlx5_vf_mac_addr_list(dev, &macs, &macs_n);
> + if (ret)
> + return ret;
> + for (i = 0; i != macs_n; ++i) {
> + if (!memcmp(&macs[i], mac, ETHER_ADDR_LEN)) {
> + mac_index = i;
> + break;
> + }
> + }
> + if (mac_index == MLX5_MAX_MAC_ADDRESSES) {
> + DRV_LOG(INFO, "port %u MAC address not found",
> + dev->data->port_id);
> + rte_errno = EINVAL;
How about EADDRNOTAVAIL (a better counterpart for EADDRINUSE)?
> + return -rte_errno;
> + }
> + return mlx5_vf_mac_addr_modify(dev, mac, 0);
> +}
> +
> +/**
> + * Flush all added MAC addresses.
> + *
> + * @param dev
> + * Pointer to Ethernet device.
> + *
> + * @return
> + * 0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +int
> +mlx5_vf_mac_addr_flush(struct rte_eth_dev *dev)
> +{
> + int i;
> + const struct ether_addr mac_null = { .addr_bytes = { 0 }, };
> +
> + /* Skip the default mac at index 0. */
I'm not sure, what if index 0 happens to be yet another secondary MAC
address of the netdevice at this point? I think the PMD should attempt to
flush them all starting from the end of the MAC array and not care about
errors; this function should return void.
> + for (i = 1; i != MLX5_MAX_MAC_ADDRESSES; ++i) {
> + struct ether_addr *m = &dev->data->mac_addrs[i];
> +
> + if (memcmp(&mac_null, m, ETHER_ADDR_LEN)) {
> + int ret;
> +
> + ret = mlx5_vf_mac_addr_remove(dev, m);
> + if (ret)
> + return ret;
> + }
> + }
> + return 0;
> +}
--
Adrien Mazarguil
6WIND
