From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH 4/5] net/mlx5: use Netlink to enable promisc/allmulti Date: Wed, 14 Mar 2018 18:11:11 +0100 Message-ID: <20180314171111.GO3994@6wind.com> References: <9236b53dbd8149ae9969b8daa359e9ea1facf2d3.1520944256.git.nelio.laranjeiro@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Yongseok Koh , dev@dpdk.org To: Nelio Laranjeiro Return-path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id 16C176CA3 for ; Wed, 14 Mar 2018 18:11:25 +0100 (CET) Received: by mail-wm0-f68.google.com with SMTP id u10so5292130wmu.4 for ; Wed, 14 Mar 2018 10:11:25 -0700 (PDT) Content-Disposition: inline In-Reply-To: <9236b53dbd8149ae9969b8daa359e9ea1facf2d3.1520944256.git.nelio.laranjeiro@6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Mar 13, 2018 at 01:50:38PM +0100, Nelio Laranjeiro wrote: > VF devices are not able to receive promisc or allmulti traffic unless it > fully requests it though Netlink. This will cause the request to be > processed by the PF which will handle the request and enable it. > > This requires the VF to be trusted by the PF. > > Signed-off-by: Nelio Laranjeiro Usual comments regarding "vf" => "nl", caps and typos. More nits below. > --- > drivers/net/mlx5/mlx5.h | 2 + > drivers/net/mlx5/mlx5_trigger.c | 27 ++++++++++- > drivers/net/mlx5/mlx5_vf.c | 102 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 130 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h > index a4333e6a5..245235641 100644 > --- a/drivers/net/mlx5/mlx5.h > +++ b/drivers/net/mlx5/mlx5.h > @@ -303,5 +303,7 @@ int mlx5_mr_verify(struct rte_eth_dev *dev); > 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); > +int mlx5_vf_promisc(struct rte_eth_dev *dev, int enable); > +int mlx5_vf_allmulti(struct rte_eth_dev *dev, int enable); > > #endif /* RTE_PMD_MLX5_H_ */ > diff --git a/drivers/net/mlx5/mlx5_trigger.c b/drivers/net/mlx5/mlx5_trigger.c > index 6bb4ffb14..3f21797ff 100644 > --- a/drivers/net/mlx5/mlx5_trigger.c > +++ b/drivers/net/mlx5/mlx5_trigger.c > @@ -278,8 +278,23 @@ mlx5_traffic_enable(struct rte_eth_dev *dev) > unsigned int j; > int ret; > > - if (priv->isolated) > + if (priv->isolated) { > + if (priv->config.vf) { > + if (dev->data->promiscuous) { > + ret = mlx5_vf_promisc(dev, 1); > + rte_errno = ret; > + if (ret) > + goto error; > + } > + if (dev->data->all_multicast) { > + ret = mlx5_vf_allmulti(dev, 1); > + rte_errno = ret; > + if (ret) > + goto error; > + } > + } Looks like this block should be run no matter what. You should put it before the check on priv->isolated and drop the two next chunks. Note there seems to be missing rollback code in case of error. > return 0; > + } > if (dev->data->promiscuous) { > struct rte_flow_item_eth promisc = { > .dst.addr_bytes = "\x00\x00\x00\x00\x00\x00", > @@ -287,6 +302,11 @@ mlx5_traffic_enable(struct rte_eth_dev *dev) > .type = 0, > }; > > + if (priv->config.vf) { > + ret = mlx5_vf_promisc(dev, 1); > + if (ret) > + goto error; > + } > ret = mlx5_ctrl_flow(dev, &promisc, &promisc); > if (ret) > goto error; > @@ -298,6 +318,11 @@ mlx5_traffic_enable(struct rte_eth_dev *dev) > .type = 0, > }; > > + if (priv->config.vf) { > + ret = mlx5_vf_allmulti(dev, 1); > + if (ret) > + goto error; > + } > ret = mlx5_ctrl_flow(dev, &multicast, &multicast); > if (ret) > goto error; > diff --git a/drivers/net/mlx5/mlx5_vf.c b/drivers/net/mlx5/mlx5_vf.c > index 3db8ee0eb..cf71e79d9 100644 > --- a/drivers/net/mlx5/mlx5_vf.c > +++ b/drivers/net/mlx5/mlx5_vf.c > @@ -447,3 +447,105 @@ mlx5_vf_mac_addr_flush(struct rte_eth_dev *dev) > } > return 0; > } > + > +/** > + * Enable promisc/allmulti though Netlink though => through, also missing period. I suggest to write promisc and allmulti in full in documentation (as "promiscuous" and "all multicast" modes). > + * > + * @param dev > + * Pointer to Ethernet device structure. > + * @param flags > + * IFF_PROMISC for promiscuous, IFF_ALLMULTI for allmulti. > + * @param en en => enable > + * 1 to enable, 0 to disable. => Nonzero to enable, disable otherwise. > + * > + * @return > + * 0 on success, a negative errno value otherwise and rte_errno is set. > + */ > +static int > +mlx5_vf_device_flags(struct rte_eth_dev *dev, uint32_t flags, int enable) > +{ > + int iface_idx = mlx5_vf_iface_idx(dev); > + struct { > + struct nlmsghdr hdr; > + struct ifinfomsg ifi; > + } req = { > + .hdr = { > + .nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg)), > + .nlmsg_type = RTM_NEWLINK, > + .nlmsg_flags = NLM_F_REQUEST, > + }, > + .ifi = { > + .ifi_flags = enable ? flags : 0, > + .ifi_change = flags, > + .ifi_index = iface_idx, > + }, > + }; > + int fd; > + int ret; > + > + assert(!(flags & ~(IFF_PROMISC | IFF_ALLMULTI))); > + fd = rte_nl_init(NETLINK_ROUTE); > + if (fd < 0) { > + rte_errno = errno; > + goto error; > + } > + ret = rte_nl_send(fd, &req.hdr); > + if (ret == -1) { > + rte_errno = errno; > + goto error; > + } > + rte_nl_final(fd); > + return 0; > +error: > + if (fd >= 0) > + rte_nl_final(fd); > + return -rte_errno; > +} > + > +/** > + * Enable promisc though Netlink Missing period, another suggestion: promisc => promiscuous mode. > + * > + * @param dev > + * Pointer to Ethernet device structure. > + * @param en > + * 1 to enable promisc, 0 to disable it. => Nonzero to enable, disable otherwise. > + * > + * @return > + * 0 on success, a negative errno value otherwise and rte_errno is set. > + */ > +int > +mlx5_vf_promisc(struct rte_eth_dev *dev, int enable) > +{ > + int ret = mlx5_vf_device_flags(dev, IFF_PROMISC, enable); > + > + if (ret) > + DRV_LOG(DEBUG, > + "port %u cannot %s promisc mode: Netlink error %s", > + dev->data->port_id, enable ? "enable" : "disable", > + strerror(rte_errno)); > + return ret; > +} > + > +/** > + * Enable allmulti though Netlink though => through > + * > + * @param dev > + * Pointer to Ethernet device structure. > + * @param en > + * 1 to enable promisc, 0 to disable it. > + * > + * @return > + * 0 on success, a negative errno value otherwise and rte_errno is set. > + */ > +int > +mlx5_vf_allmulti(struct rte_eth_dev *dev, int enable) > +{ > + int ret = mlx5_vf_device_flags(dev, IFF_ALLMULTI, enable); > + > + if (ret) > + DRV_LOG(DEBUG, > + "port %u cannot %s allmulti mode: Netlink error %s", > + dev->data->port_id, enable ? "enable" : "disable", > + strerror(rte_errno)); > + return ret; > +} > -- > 2.11.0 > -- Adrien Mazarguil 6WIND