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