[PATCH 0/2] implement set_mc_addr devop

[PATCH 0/2] implement set_mc_addr devop

[Nelio Laranjeiro]

Support eth_dev_ops.set_mc_addr_list().

Nelio Laranjeiro (2):
  net/mlx5: split MAC address add/remove code
  net/mlx5: implement multicast add list devop

 drivers/net/mlx5/mlx5.c        |   2 +
 drivers/net/mlx5/mlx5.h        |   2 +
 drivers/net/mlx5/mlx5_defs.h   |   7 +-
 drivers/net/mlx5/mlx5_ethdev.c |   2 +-
 drivers/net/mlx5/mlx5_mac.c    | 144 +++++++++++++++++++++++++++++----
 5 files changed, 139 insertions(+), 18 deletions(-)

-- 
2.17.0

[PATCH 1/2] net/mlx5: split MAC address add/remove code

[Nelio Laranjeiro]

Move some code in DPDK callbacks to add/remove MAC addresses to internal
function.  This modification will be necessary to handle implement the
devop set_mc_addr_list.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5_defs.h   |  4 +-
 drivers/net/mlx5/mlx5_ethdev.c |  2 +-
 drivers/net/mlx5/mlx5_mac.c    | 92 ++++++++++++++++++++++++++++------
 3 files changed, 80 insertions(+), 18 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index 3fb2f1480..e3aa03bef 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -13,8 +13,10 @@
 /* Reported driver name. */
 #define MLX5_DRIVER_NAME "net_mlx5"
 
+/* Maximum number of simultaneous unicast MAC addresses. */
+#define MLX5_MAX_UC_MAC_ADDRESSES 128
 /* Maximum number of simultaneous MAC addresses. */
-#define MLX5_MAX_MAC_ADDRESSES 128
+#define MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
 
 /* Maximum number of simultaneous VLAN filters. */
 #define MLX5_MAX_VLAN_IDS 128
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index ef44cc91f..99ac4cc5c 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -446,7 +446,7 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 		max = 65535;
 	info->max_rx_queues = max;
 	info->max_tx_queues = max;
-	info->max_mac_addrs = RTE_DIM(priv->mac);
+	info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES;
 	info->rx_queue_offload_capa = mlx5_get_rx_queue_offloads(dev);
 	info->rx_offload_capa = (mlx5_get_rx_port_offloads() |
 				 info->rx_queue_offload_capa);
diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index edc7a32ae..9470520b1 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -57,35 +57,32 @@ mlx5_get_mac(struct rte_eth_dev *dev, uint8_t (*mac)[ETHER_ADDR_LEN])
 }
 
 /**
- * DPDK callback to remove a MAC address.
+ * Remove a MAC address from the internal array.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
  * @param index
  *   MAC address index.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
  */
-void
-mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
+static int
+mlx5_internal_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 (vf)
 		mlx5_nl_mac_addr_remove(dev, &dev->data->mac_addrs[index],
 					index);
 	memset(&dev->data->mac_addrs[index], 0, sizeof(struct ether_addr));
-	if (!dev->data->promiscuous) {
-		ret = mlx5_traffic_restart(dev);
-		if (ret)
-			DRV_LOG(ERR, "port %u cannot restart traffic: %s",
-				dev->data->port_id, strerror(rte_errno));
-	}
+	return 0;
 }
 
 /**
- * DPDK callback to add a MAC address.
+ * Adds a MAC address to the internal array.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
@@ -93,15 +90,13 @@ mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
  *   MAC address to register.
  * @param index
  *   MAC address index.
- * @param vmdq
- *   VMDq pool index to associate address with (ignored).
  *
  * @return
  *   0 on success, a negative errno value otherwise and rte_errno is set.
  */
-int
-mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
-		  uint32_t index, uint32_t vmdq __rte_unused)
+static int
+mlx5_internal_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
+			   uint32_t index)
 {
 	struct priv *priv = dev->data->dev_private;
 	const int vf = priv->config.vf;
@@ -126,6 +121,71 @@ mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
 			return ret;
 	}
 	dev->data->mac_addrs[index] = *mac;
+	return 0;
+}
+
+/**
+ * DPDK callback to remove a MAC address.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param index
+ *   MAC address index.
+ */
+void
+mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
+{
+	int ret;
+
+	if (index >= MLX5_MAX_UC_MAC_ADDRESSES) {
+		rte_errno = ENOMEM;
+		DRV_LOG(ERR,
+			"port %u not enough room to add the MAC address: %s",
+			dev->data->port_id, strerror(rte_errno));
+		return;
+	}
+	ret = mlx5_internal_mac_addr_remove(dev, index);
+	if (ret < 0) {
+		DRV_LOG(ERR,
+			"port %u failed to remove the MAC address: %s",
+			dev->data->port_id, strerror(rte_errno));
+	}
+	if (!dev->data->promiscuous) {
+		ret = mlx5_traffic_restart(dev);
+		if (ret)
+			DRV_LOG(ERR, "port %u cannot restart traffic: %s",
+				dev->data->port_id, strerror(rte_errno));
+	}
+}
+
+/**
+ * DPDK callback to add a MAC address.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param mac_addr
+ *   MAC address to register.
+ * @param index
+ *   MAC address index.
+ * @param vmdq
+ *   VMDq pool index to associate address with (ignored).
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+int
+mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
+		  uint32_t index, uint32_t vmdq __rte_unused)
+{
+	int ret;
+
+	if (index >= MLX5_MAX_UC_MAC_ADDRESSES) {
+		rte_errno = ENOMEM;
+		return -rte_errno;
+	}
+	ret = mlx5_internal_mac_addr_add(dev, mac, index);
+	if (ret < 0)
+		return ret;
 	if (!dev->data->promiscuous)
 		return mlx5_traffic_restart(dev);
 	return 0;
-- 
2.17.0

[PATCH 2/2] net/mlx5: implement multicast add list devop

[Nelio Laranjeiro]

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5.c      |  2 ++
 drivers/net/mlx5/mlx5.h      |  2 ++
 drivers/net/mlx5/mlx5_defs.h |  5 +++-
 drivers/net/mlx5/mlx5_mac.c  | 52 ++++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 68783c3ac..887924d07 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set,
@@ -329,6 +330,7 @@ const struct eth_dev_ops mlx5_dev_ops_isolate = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set,
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 6ad41390a..64f025d22 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
 int mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
 		      uint32_t index, uint32_t vmdq);
 int mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);
+int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+			  struct ether_addr *mc_addr_set, uint32_t nb_mc_addr);
 
 /* mlx5_rss.c */
 
diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index e3aa03bef..5973742a7 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -15,8 +15,11 @@
 
 /* Maximum number of simultaneous unicast MAC addresses. */
 #define MLX5_MAX_UC_MAC_ADDRESSES 128
+/* Maximum number of simultaneous Multicast MAC addresses. */
+#define MLX5_MAX_MC_MAC_ADDRESSES 128
 /* Maximum number of simultaneous MAC addresses. */
-#define MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
+#define MLX5_MAX_MAC_ADDRESSES \
+	(MLX5_MAX_UC_MAC_ADDRESSES + MLX5_MAX_MC_MAC_ADDRESSES)
 
 /* Maximum number of simultaneous VLAN filters. */
 #define MLX5_MAX_VLAN_IDS 128
diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index 9470520b1..deeed9598 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -209,3 +209,55 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		dev->data->port_id);
 	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);
 }
+
+/**
+ * DPDK callback to set Multicast Addresses list.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param mac_addr_set
+ *   Multicast MAC address pointer array.
+ * @param nb_mac_addr
+ *   Number of entries in the array.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+int
+mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr)
+{
+	uint32_t i;
+	int ret;
+	int idx;
+
+	for (i = MLX5_MAX_UC_MAC_ADDRESSES;
+	     i != MLX5_MAX_UC_MAC_ADDRESSES + MLX5_MAX_MC_MAC_ADDRESSES;
+	     ++i)
+		if (!is_zero_ether_addr(&dev->data->mac_addrs[i])) {
+			ret = mlx5_internal_mac_addr_remove(dev, i);
+			if (ret < 0)
+				return ret;
+		}
+	for (idx = MLX5_MAX_UC_MAC_ADDRESSES, i = 0;
+	     i != nb_mc_addr;
+	     ++i) {
+		if (!is_multicast_ether_addr(mc_addr_set)) {
+			rte_errno = EINVAL;
+			goto rollback;
+		}
+		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++, idx++);
+		if (ret < 0)
+			return ret;
+	}
+	if (!dev->data->promiscuous)
+		return mlx5_traffic_restart(dev);
+	return 0;
+rollback:
+	ret = rte_errno; /* Save rte_errno before cleanup. */
+	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i != MLX5_MAX_MC_MAC_ADDRESSES; ++i)
+		if (!is_zero_ether_addr(&dev->data->mac_addrs[i]))
+			mlx5_internal_mac_addr_remove(dev, i);
+	rte_errno = ret; /* Restore rte_errno. */
+	return -rte_errno;
+}
-- 
2.17.0

[PATCH v2 0/3] net/mlx5: implement set_mc_addr devop

[Nelio Laranjeiro]

Support eth_dev_ops.set_mc_addr_list().

Changes in v2:

Simplify the code and are even more verifications.

Nelio Laranjeiro (3):
  net/mlx5: more checks on MAC addresses
  net/mlx5: split MAC address add/remove code
  net/mlx5: implement multicast add list devop

 drivers/net/mlx5/mlx5.c        |   2 +
 drivers/net/mlx5/mlx5.h        |   2 +
 drivers/net/mlx5/mlx5_defs.h   |   7 +-
 drivers/net/mlx5/mlx5_ethdev.c |   2 +-
 drivers/net/mlx5/mlx5_mac.c    | 117 ++++++++++++++++++++++++++++-----
 5 files changed, 112 insertions(+), 18 deletions(-)

-- 
2.17.0

[PATCH v2 1/3] net/mlx5: more checks on MAC addresses

[Nelio Laranjeiro]

Verify MAC address before further process.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5_mac.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index edc7a32ae..f24b5f759 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -71,7 +71,10 @@ mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
 	const int vf = priv->config.vf;
 	int ret;
 
-	assert(index < MLX5_MAX_MAC_ADDRESSES);
+	if (index >= MLX5_MAX_MAC_ADDRESSES)
+		return;
+	if (is_zero_ether_addr(&dev->data->mac_addrs[index]))
+		return;
 	if (vf)
 		mlx5_nl_mac_addr_remove(dev, &dev->data->mac_addrs[index],
 					index);
@@ -107,7 +110,14 @@ mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
 	const int vf = priv->config.vf;
 	unsigned int i;
 
-	assert(index < MLX5_MAX_MAC_ADDRESSES);
+	if (index >= MLX5_MAX_MAC_ADDRESSES) {
+		rte_errno = EINVAL;
+		return -rte_errno;
+	}
+	if (is_zero_ether_addr(mac)) {
+		rte_errno = EINVAL;
+		return -rte_errno;
+	}
 	/* First, make sure this address isn't already configured. */
 	for (i = 0; (i != MLX5_MAX_MAC_ADDRESSES); ++i) {
 		/* Skip this index, it's going to be reconfigured. */
-- 
2.17.0

[PATCH v2 2/3] net/mlx5: split MAC address add/remove code

[Nelio Laranjeiro]

Move some code in DPDK callbacks to add/remove MAC addresses to internal
function.  This modification will be necessary to handle implement the
devop set_mc_addr_list.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5_defs.h   |  4 +-
 drivers/net/mlx5/mlx5_ethdev.c |  2 +-
 drivers/net/mlx5/mlx5_mac.c    | 86 +++++++++++++++++++++++++---------
 3 files changed, 68 insertions(+), 24 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index 3fb2f1480..e3aa03bef 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -13,8 +13,10 @@
 /* Reported driver name. */
 #define MLX5_DRIVER_NAME "net_mlx5"
 
+/* Maximum number of simultaneous unicast MAC addresses. */
+#define MLX5_MAX_UC_MAC_ADDRESSES 128
 /* Maximum number of simultaneous MAC addresses. */
-#define MLX5_MAX_MAC_ADDRESSES 128
+#define MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
 
 /* Maximum number of simultaneous VLAN filters. */
 #define MLX5_MAX_VLAN_IDS 128
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index ef44cc91f..99ac4cc5c 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -446,7 +446,7 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 		max = 65535;
 	info->max_rx_queues = max;
 	info->max_tx_queues = max;
-	info->max_mac_addrs = RTE_DIM(priv->mac);
+	info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES;
 	info->rx_queue_offload_capa = mlx5_get_rx_queue_offloads(dev);
 	info->rx_offload_capa = (mlx5_get_rx_port_offloads() |
 				 info->rx_queue_offload_capa);
diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index f24b5f759..e9334ea4a 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -57,38 +57,30 @@ mlx5_get_mac(struct rte_eth_dev *dev, uint8_t (*mac)[ETHER_ADDR_LEN])
 }
 
 /**
- * DPDK callback to remove a MAC address.
+ * Remove a MAC address from the internal array.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
  * @param index
  *   MAC address index.
  */
-void
-mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
+static void
+mlx5_internal_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;
 
-	if (index >= MLX5_MAX_MAC_ADDRESSES)
-		return;
+	assert(index < MLX5_MAX_MAC_ADDRESSES);
 	if (is_zero_ether_addr(&dev->data->mac_addrs[index]))
 		return;
 	if (vf)
 		mlx5_nl_mac_addr_remove(dev, &dev->data->mac_addrs[index],
 					index);
 	memset(&dev->data->mac_addrs[index], 0, sizeof(struct ether_addr));
-	if (!dev->data->promiscuous) {
-		ret = mlx5_traffic_restart(dev);
-		if (ret)
-			DRV_LOG(ERR, "port %u cannot restart traffic: %s",
-				dev->data->port_id, strerror(rte_errno));
-	}
 }
 
 /**
- * DPDK callback to add a MAC address.
+ * Adds a MAC address to the internal array.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
@@ -96,24 +88,19 @@ mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
  *   MAC address to register.
  * @param index
  *   MAC address index.
- * @param vmdq
- *   VMDq pool index to associate address with (ignored).
  *
  * @return
  *   0 on success, a negative errno value otherwise and rte_errno is set.
  */
-int
-mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
-		  uint32_t index, uint32_t vmdq __rte_unused)
+static int
+mlx5_internal_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
+			   uint32_t index)
 {
 	struct priv *priv = dev->data->dev_private;
 	const int vf = priv->config.vf;
 	unsigned int i;
 
-	if (index >= MLX5_MAX_MAC_ADDRESSES) {
-		rte_errno = EINVAL;
-		return -rte_errno;
-	}
+	assert(index < MLX5_MAX_MAC_ADDRESSES);
 	if (is_zero_ether_addr(mac)) {
 		rte_errno = EINVAL;
 		return -rte_errno;
@@ -136,6 +123,61 @@ mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
 			return ret;
 	}
 	dev->data->mac_addrs[index] = *mac;
+	return 0;
+}
+
+/**
+ * DPDK callback to remove a MAC address.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param index
+ *   MAC address index.
+ */
+void
+mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
+{
+	int ret;
+
+	if (index >= MLX5_MAX_UC_MAC_ADDRESSES)
+		return;
+	mlx5_internal_mac_addr_remove(dev, index);
+	if (!dev->data->promiscuous) {
+		ret = mlx5_traffic_restart(dev);
+		if (ret)
+			DRV_LOG(ERR, "port %u cannot restart traffic: %s",
+				dev->data->port_id, strerror(rte_errno));
+	}
+}
+
+/**
+ * DPDK callback to add a MAC address.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param mac_addr
+ *   MAC address to register.
+ * @param index
+ *   MAC address index.
+ * @param vmdq
+ *   VMDq pool index to associate address with (ignored).
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+int
+mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
+		  uint32_t index, uint32_t vmdq __rte_unused)
+{
+	int ret;
+
+	if (index >= MLX5_MAX_UC_MAC_ADDRESSES) {
+		rte_errno = EINVAL;
+		return -rte_errno;
+	}
+	ret = mlx5_internal_mac_addr_add(dev, mac, index);
+	if (ret < 0)
+		return ret;
 	if (!dev->data->promiscuous)
 		return mlx5_traffic_restart(dev);
 	return 0;
-- 
2.17.0

[PATCH v2 3/3] net/mlx5: implement multicast add list devop

[Nelio Laranjeiro]

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5.c      |  2 ++
 drivers/net/mlx5/mlx5.h      |  2 ++
 drivers/net/mlx5/mlx5_defs.h |  5 ++++-
 drivers/net/mlx5/mlx5_mac.c  | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 68783c3ac..887924d07 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set,
@@ -329,6 +330,7 @@ const struct eth_dev_ops mlx5_dev_ops_isolate = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set,
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 6ad41390a..64f025d22 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
 int mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
 		      uint32_t index, uint32_t vmdq);
 int mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);
+int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+			  struct ether_addr *mc_addr_set, uint32_t nb_mc_addr);
 
 /* mlx5_rss.c */
 
diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index e3aa03bef..5973742a7 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -15,8 +15,11 @@
 
 /* Maximum number of simultaneous unicast MAC addresses. */
 #define MLX5_MAX_UC_MAC_ADDRESSES 128
+/* Maximum number of simultaneous Multicast MAC addresses. */
+#define MLX5_MAX_MC_MAC_ADDRESSES 128
 /* Maximum number of simultaneous MAC addresses. */
-#define MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
+#define MLX5_MAX_MAC_ADDRESSES \
+	(MLX5_MAX_UC_MAC_ADDRESSES + MLX5_MAX_MC_MAC_ADDRESSES)
 
 /* Maximum number of simultaneous VLAN filters. */
 #define MLX5_MAX_VLAN_IDS 128
diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index e9334ea4a..32560d55e 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -201,3 +201,36 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		dev->data->port_id);
 	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);
 }
+
+/**
+ * DPDK callback to set multicast addresses list.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param mac_addr_set
+ *   Multicast MAC address pointer array.
+ * @param nb_mac_addr
+ *   Number of entries in the array.
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+int
+mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr)
+{
+	uint32_t i;
+	int ret;
+
+	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i != MLX5_MAX_MAC_ADDRESSES; ++i)
+		mlx5_internal_mac_addr_remove(dev, i);
+	i = MLX5_MAX_UC_MAC_ADDRESSES;
+	while (nb_mc_addr--) {
+		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++, i++);
+		if (ret)
+			return ret;
+	}
+	if (!dev->data->promiscuous)
+		return mlx5_traffic_restart(dev);
+	return 0;
+}
-- 
2.17.0

Re: [PATCH v2 3/3] net/mlx5: implement multicast add list devop

[Shahaf Shuler]

Hi Nelio,

Small comments before I merge.

Wednesday, April 18, 2018 4:50 PM, Nelio Laranjeiro:
> Subject: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list
> devop
> 
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
>  drivers/net/mlx5/mlx5.c      |  2 ++
>  drivers/net/mlx5/mlx5.h      |  2 ++
>  drivers/net/mlx5/mlx5_defs.h |  5 ++++-  drivers/net/mlx5/mlx5_mac.c  | 33
> +++++++++++++++++++++++++++++++++
>  4 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> 68783c3ac..887924d07 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
>  	.mac_addr_remove = mlx5_mac_addr_remove,
>  	.mac_addr_add = mlx5_mac_addr_add,
>  	.mac_addr_set = mlx5_mac_addr_set,
> +	.set_mc_addr_list = mlx5_set_mc_addr_list,
>  	.mtu_set = mlx5_dev_set_mtu,
>  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
>  	.vlan_offload_set = mlx5_vlan_offload_set, @@ -329,6 +330,7 @@
> const struct eth_dev_ops mlx5_dev_ops_isolate = {
>  	.mac_addr_remove = mlx5_mac_addr_remove,
>  	.mac_addr_add = mlx5_mac_addr_add,
>  	.mac_addr_set = mlx5_mac_addr_set,
> +	.set_mc_addr_list = mlx5_set_mc_addr_list,
>  	.mtu_set = mlx5_dev_set_mtu,
>  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
>  	.vlan_offload_set = mlx5_vlan_offload_set, diff --git
> a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 6ad41390a..64f025d22 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev
> *dev, uint32_t index);  int mlx5_mac_addr_add(struct rte_eth_dev *dev,
> struct ether_addr *mac,
>  		      uint32_t index, uint32_t vmdq);  int
> mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr
> *mac_addr);
> +int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> +			  struct ether_addr *mc_addr_set, uint32_t
> nb_mc_addr);
> 
>  /* mlx5_rss.c */
> 
> diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
> index e3aa03bef..5973742a7 100644
> --- a/drivers/net/mlx5/mlx5_defs.h
> +++ b/drivers/net/mlx5/mlx5_defs.h
> @@ -15,8 +15,11 @@
> 
>  /* Maximum number of simultaneous unicast MAC addresses. */  #define
> MLX5_MAX_UC_MAC_ADDRESSES 128
> +/* Maximum number of simultaneous Multicast MAC addresses. */ #define
> +MLX5_MAX_MC_MAC_ADDRESSES 128
>  /* Maximum number of simultaneous MAC addresses. */ -#define
> MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
> +#define MLX5_MAX_MAC_ADDRESSES \
> +	(MLX5_MAX_UC_MAC_ADDRESSES +
> MLX5_MAX_MC_MAC_ADDRESSES)
> 
>  /* Maximum number of simultaneous VLAN filters. */  #define
> MLX5_MAX_VLAN_IDS 128 diff --git a/drivers/net/mlx5/mlx5_mac.c
> b/drivers/net/mlx5/mlx5_mac.c index e9334ea4a..32560d55e 100644
> --- a/drivers/net/mlx5/mlx5_mac.c
> +++ b/drivers/net/mlx5/mlx5_mac.c
> @@ -201,3 +201,36 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct
> ether_addr *mac_addr)
>  		dev->data->port_id);
>  	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);  }
> +
> +/**
> + * DPDK callback to set multicast addresses list.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + * @param mac_addr_set
> + *   Multicast MAC address pointer array.
> + * @param nb_mac_addr
> + *   Number of entries in the array.
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +int
> +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> +		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> +	uint32_t i;
> +	int ret;
> +

We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES before we start operate. 

> +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> MLX5_MAX_MAC_ADDRESSES; ++i)
> +		mlx5_internal_mac_addr_remove(dev, i);
> +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> +	while (nb_mc_addr--) {

Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip + warn if it is not. 

> +		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> i++);
> +		if (ret)
> +			return ret;
> +	}
> +	if (!dev->data->promiscuous)
> +		return mlx5_traffic_restart(dev);
> +	return 0;
> +}
> --
> 2.17.0

Re: [PATCH v2 3/3] net/mlx5: implement multicast add list devop

[Nélio Laranjeiro]

On Mon, Apr 23, 2018 at 05:52:08AM +0000, Shahaf Shuler wrote:
> Hi Nelio,
> 
> Small comments before I merge.
> 
> Wednesday, April 18, 2018 4:50 PM, Nelio Laranjeiro:
> > Subject: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add list
> > devop
> > 
> > Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5.c      |  2 ++
> >  drivers/net/mlx5/mlx5.h      |  2 ++
> >  drivers/net/mlx5/mlx5_defs.h |  5 ++++-  drivers/net/mlx5/mlx5_mac.c  | 33
> > +++++++++++++++++++++++++++++++++
> >  4 files changed, 41 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > 68783c3ac..887924d07 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
> >  	.mac_addr_remove = mlx5_mac_addr_remove,
> >  	.mac_addr_add = mlx5_mac_addr_add,
> >  	.mac_addr_set = mlx5_mac_addr_set,
> > +	.set_mc_addr_list = mlx5_set_mc_addr_list,
> >  	.mtu_set = mlx5_dev_set_mtu,
> >  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
> >  	.vlan_offload_set = mlx5_vlan_offload_set, @@ -329,6 +330,7 @@
> > const struct eth_dev_ops mlx5_dev_ops_isolate = {
> >  	.mac_addr_remove = mlx5_mac_addr_remove,
> >  	.mac_addr_add = mlx5_mac_addr_add,
> >  	.mac_addr_set = mlx5_mac_addr_set,
> > +	.set_mc_addr_list = mlx5_set_mc_addr_list,
> >  	.mtu_set = mlx5_dev_set_mtu,
> >  	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
> >  	.vlan_offload_set = mlx5_vlan_offload_set, diff --git
> > a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> > 6ad41390a..64f025d22 100644
> > --- a/drivers/net/mlx5/mlx5.h
> > +++ b/drivers/net/mlx5/mlx5.h
> > @@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev
> > *dev, uint32_t index);  int mlx5_mac_addr_add(struct rte_eth_dev *dev,
> > struct ether_addr *mac,
> >  		      uint32_t index, uint32_t vmdq);  int
> > mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr
> > *mac_addr);
> > +int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > +			  struct ether_addr *mc_addr_set, uint32_t
> > nb_mc_addr);
> > 
> >  /* mlx5_rss.c */
> > 
> > diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
> > index e3aa03bef..5973742a7 100644
> > --- a/drivers/net/mlx5/mlx5_defs.h
> > +++ b/drivers/net/mlx5/mlx5_defs.h
> > @@ -15,8 +15,11 @@
> > 
> >  /* Maximum number of simultaneous unicast MAC addresses. */  #define
> > MLX5_MAX_UC_MAC_ADDRESSES 128
> > +/* Maximum number of simultaneous Multicast MAC addresses. */ #define
> > +MLX5_MAX_MC_MAC_ADDRESSES 128
> >  /* Maximum number of simultaneous MAC addresses. */ -#define
> > MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
> > +#define MLX5_MAX_MAC_ADDRESSES \
> > +	(MLX5_MAX_UC_MAC_ADDRESSES +
> > MLX5_MAX_MC_MAC_ADDRESSES)
> > 
> >  /* Maximum number of simultaneous VLAN filters. */  #define
> > MLX5_MAX_VLAN_IDS 128 diff --git a/drivers/net/mlx5/mlx5_mac.c
> > b/drivers/net/mlx5/mlx5_mac.c index e9334ea4a..32560d55e 100644
> > --- a/drivers/net/mlx5/mlx5_mac.c
> > +++ b/drivers/net/mlx5/mlx5_mac.c
> > @@ -201,3 +201,36 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct
> > ether_addr *mac_addr)
> >  		dev->data->port_id);
> >  	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);  }
> > +
> > +/**
> > + * DPDK callback to set multicast addresses list.
> > + *
> > + * @param dev
> > + *   Pointer to Ethernet device structure.
> > + * @param mac_addr_set
> > + *   Multicast MAC address pointer array.
> > + * @param nb_mac_addr
> > + *   Number of entries in the array.
> > + *
> > + * @return
> > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +int
> > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > +		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> > +	uint32_t i;
> > +	int ret;
> > +
> 
> We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES before we start
> operate. 

This verification is done in the sub function.

Considering an application calling such API wants to remove/replace the
old list with new entries.
That this new one can be just an addition or totally different list or
even empty.
This new list can be larger than the amount of MAC addresses the PMD can
support.

There are two possibilities:

1. The list is too large:  the application will enable the all multicast
mode to receive the extra mac addresses it needs.
2. The list fits (or empty): no issues.

At the end the application can also call this API with an empty list to
clear it before/after enabling the "all multicast" mode.
The final result being the same, does it worse to add a duplicated 
verification?

Note: if an error happens the new list is not committed yet i.e. the
traffic remains untouched.

> > +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > MLX5_MAX_MAC_ADDRESSES; ++i)
> > +		mlx5_internal_mac_addr_remove(dev, i);
> > +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> > +	while (nb_mc_addr--) {
> 
> Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> + warn if it is not. 

Such verification should be done in the public API i.e. ethdev.

> > +		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > i++);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +	if (!dev->data->promiscuous)
> > +		return mlx5_traffic_restart(dev);
> > +	return 0;
> > +}
> > --
> > 2.17.0

Regards,

-- 
Nélio Laranjeiro
6WIND

Re: [PATCH v2 3/3] net/mlx5: implement multicast add list devop

[Shahaf Shuler]

Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:

[...]

> > > +/**
> > > + * DPDK callback to set multicast addresses list.
> > > + *
> > > + * @param dev
> > > + *   Pointer to Ethernet device structure.
> > > + * @param mac_addr_set
> > > + *   Multicast MAC address pointer array.
> > > + * @param nb_mac_addr
> > > + *   Number of entries in the array.
> > > + *
> > > + * @return
> > > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > > + */
> > > +int
> > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > > +		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> > > +	uint32_t i;
> > > +	int ret;
> > > +
> >
> > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES
> before we start
> > operate.
> 
> This verification is done in the sub function.

I see only assert. Did I missed anything? 

> 
> Considering an application calling such API wants to remove/replace the old
> list with new entries.
> That this new one can be just an addition or totally different list or even
> empty.
> This new list can be larger than the amount of MAC addresses the PMD can
> support.
> 
> There are two possibilities:
> 
> 1. The list is too large:  the application will enable the all multicast mode to
> receive the extra mac addresses it needs.

How can application know the size of the MC list?
only the UC size is being reported:
info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES

> 2. The list fits (or empty): no issues.
> 
> At the end the application can also call this API with an empty list to clear it
> before/after enabling the "all multicast" mode.
> The final result being the same, does it worse to add a duplicated
> verification?

At the current code if the list is too large and the PMD was compiled w/o debug mode it will results in seg fault. 

> 
> Note: if an error happens the new list is not committed yet i.e. the traffic
> remains untouched.
> 
> > > +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > > MLX5_MAX_MAC_ADDRESSES; ++i)
> > > +		mlx5_internal_mac_addr_remove(dev, i);
> > > +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> > > +	while (nb_mc_addr--) {
> >
> > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> > + warn if it is not.
> 
> Such verification should be done in the public API i.e. ethdev.

I don't understand. 
In the first patch of the series you add extra verification to check the mac address validity. But for the MC you claim it should be done on ethdev layer. 

It should be consistant. Either ethdev verify the MAC address or the PMD. If the first one, then there is no need to add the is_zero_ether_addr check on the first patch. 


> 
> > > +		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > > i++);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +	if (!dev->data->promiscuous)
> > > +		return mlx5_traffic_restart(dev);
> > > +	return 0;
> > > +}
> > > --
> > > 2.17.0
> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND

Re: [PATCH v2 3/3] net/mlx5: implement multicast add list devop

[Nélio Laranjeiro]

On Mon, Apr 23, 2018 at 07:57:36AM +0000, Shahaf Shuler wrote:
> Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:
> [...]
> > > > +/**
> > > > + * DPDK callback to set multicast addresses list.
> > > > + *
> > > > + * @param dev
> > > > + *   Pointer to Ethernet device structure.
> > > > + * @param mac_addr_set
> > > > + *   Multicast MAC address pointer array.
> > > > + * @param nb_mac_addr
> > > > + *   Number of entries in the array.
> > > > + *
> > > > + * @return
> > > > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > > > + */
> > > > +int
> > > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > > > +		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr) {
> > > > +	uint32_t i;
> > > > +	int ret;
> > > > +
> > >
> > > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES
> > before we start
> > > operate.
> > 
> > This verification is done in the sub function.
> 
> I see only assert. Did I missed anything? 

No.

> > Considering an application calling such API wants to remove/replace the old
> > list with new entries.
> > That this new one can be just an addition or totally different list or even
> > empty.
> > This new list can be larger than the amount of MAC addresses the PMD can
> > support.
> > 
> > There are two possibilities:
> > 
> > 1. The list is too large:  the application will enable the all multicast mode to
> > receive the extra mac addresses it needs.
> 
> How can application know the size of the MC list?
> only the UC size is being reported:
> info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES

Such information is not reported at all.  The application has to guess.

> > 2. The list fits (or empty): no issues.
> > 
> > At the end the application can also call this API with an empty list to clear it
> > before/after enabling the "all multicast" mode.
> > The final result being the same, does it worse to add a duplicated
> > verification?
> 
> At the current code if the list is too large and the PMD was compiled
> w/o debug mode it will results in seg fault. 

Right it needs a verification.

> > Note: if an error happens the new list is not committed yet i.e. the traffic
> > remains untouched.
> > 
> > > > +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > > > MLX5_MAX_MAC_ADDRESSES; ++i)
> > > > +		mlx5_internal_mac_addr_remove(dev, i);
> > > > +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> > > > +	while (nb_mc_addr--) {
> > >
> > > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to skip
> > > + warn if it is not.
> > 
> > Such verification should be done in the public API i.e. ethdev.
> 
> I don't understand. 
> In the first patch of the series you add extra verification to check
> the mac address validity.

It only verify the MAC address is not zero, it does not verify the MAC
address is valid in the function context (e.g. unicast in
mlx5_mac_addr_add()).

> But for the MC you claim it should be done on ethdev layer. 

Dito.

> It should be consistant. Either ethdev verify the MAC address or the
> PMD. If the first one, then there is no need to add the
> is_zero_ether_addr check on the first patch. 

It is consistent, the PMD only verify the MAC address is not zero and
this in both API.

> > > > +		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++,
> > > > i++);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +	if (!dev->data->promiscuous)
> > > > +		return mlx5_traffic_restart(dev);
> > > > +	return 0;
> > > > +}
> > > > --
> > > > 2.17.0

Regards,

-- 
Nélio Laranjeiro
6WIND

Re: [PATCH v2 3/3] net/mlx5: implement multicast add list devop

[Shahaf Shuler]

Monday, April 23, 2018 12:34 PM, Nélio Laranjeiro:
> Subject: Re: [dpdk-dev] [PATCH v2 3/3] net/mlx5: implement multicast add
> list devop
> 
> On Mon, Apr 23, 2018 at 07:57:36AM +0000, Shahaf Shuler wrote:
> > Monday, April 23, 2018 10:33 AM, Nélio Laranjeiro:
> > [...]
> > > > > +/**
> > > > > + * DPDK callback to set multicast addresses list.
> > > > > + *
> > > > > + * @param dev
> > > > > + *   Pointer to Ethernet device structure.
> > > > > + * @param mac_addr_set
> > > > > + *   Multicast MAC address pointer array.
> > > > > + * @param nb_mac_addr
> > > > > + *   Number of entries in the array.
> > > > > + *
> > > > > + * @return
> > > > > + *   0 on success, a negative errno value otherwise and rte_errno is
> set.
> > > > > + */
> > > > > +int
> > > > > +mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
> > > > > +		      struct ether_addr *mc_addr_set, uint32_t
> nb_mc_addr) {
> > > > > +	uint32_t i;
> > > > > +	int ret;
> > > > > +
> > > >
> > > > We should check nb_mc_addr < MLX5_MAX_MC_MAC_ADDRESSES
> > > before we start
> > > > operate.
> > >
> > > This verification is done in the sub function.
> >
> > I see only assert. Did I missed anything?
> 
> No.
> 
> > > Considering an application calling such API wants to remove/replace
> > > the old list with new entries.
> > > That this new one can be just an addition or totally different list
> > > or even empty.
> > > This new list can be larger than the amount of MAC addresses the PMD
> > > can support.
> > >
> > > There are two possibilities:
> > >
> > > 1. The list is too large:  the application will enable the all
> > > multicast mode to receive the extra mac addresses it needs.
> >
> > How can application know the size of the MC list?
> > only the UC size is being reported:
> > info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES
> 
> Such information is not reported at all.  The application has to guess.
> 
> > > 2. The list fits (or empty): no issues.
> > >
> > > At the end the application can also call this API with an empty list
> > > to clear it before/after enabling the "all multicast" mode.
> > > The final result being the same, does it worse to add a duplicated
> > > verification?
> >
> > At the current code if the list is too large and the PMD was compiled
> > w/o debug mode it will results in seg fault.
> 
> Right it needs a verification.
> 
> > > Note: if an error happens the new list is not committed yet i.e. the
> > > traffic remains untouched.
> > >
> > > > > +	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i !=
> > > > > MLX5_MAX_MAC_ADDRESSES; ++i)
> > > > > +		mlx5_internal_mac_addr_remove(dev, i);
> > > > > +	i = MLX5_MAX_UC_MAC_ADDRESSES;
> > > > > +	while (nb_mc_addr--) {
> > > >
> > > > Maybe worth checking is_multicast_ether_addr(mc_addr_set) and to
> > > > skip
> > > > + warn if it is not.
> > >
> > > Such verification should be done in the public API i.e. ethdev.
> >
> > I don't understand.
> > In the first patch of the series you add extra verification to check
> > the mac address validity.
> 
> It only verify the MAC address is not zero, it does not verify the MAC address
> is valid in the function context (e.g. unicast in mlx5_mac_addr_add()).
> 
> > But for the MC you claim it should be done on ethdev layer.
> 
> Dito.
> 
> > It should be consistant. Either ethdev verify the MAC address or the
> > PMD. If the first one, then there is no need to add the
> > is_zero_ether_addr check on the first patch.
> 
> It is consistent, the PMD only verify the MAC address is not zero and this in
> both API.

OK,
Then I wait for the next version for merge. 

> 
> > > > > +		ret = mlx5_internal_mac_addr_add(dev,
> mc_addr_set++,
> > > > > i++);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > +	if (!dev->data->promiscuous)
> > > > > +		return mlx5_traffic_restart(dev);
> > > > > +	return 0;
> > > > > +}
> > > > > --
> > > > > 2.17.0
> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND

Re: [PATCH v2 0/3] net/mlx5: implement set_mc_addr devop

[Adrien Mazarguil]

On Wed, Apr 18, 2018 at 03:50:03PM +0200, Nelio Laranjeiro wrote:
> Support eth_dev_ops.set_mc_addr_list().
> 
> Changes in v2:
> 
> Simplify the code and are even more verifications.
> 
> Nelio Laranjeiro (3):
>   net/mlx5: more checks on MAC addresses
>   net/mlx5: split MAC address add/remove code
>   net/mlx5: implement multicast add list devop
> 
>  drivers/net/mlx5/mlx5.c        |   2 +
>  drivers/net/mlx5/mlx5.h        |   2 +
>  drivers/net/mlx5/mlx5_defs.h   |   7 +-
>  drivers/net/mlx5/mlx5_ethdev.c |   2 +-
>  drivers/net/mlx5/mlx5_mac.c    | 117 ++++++++++++++++++++++++++++-----
>  5 files changed, 112 insertions(+), 18 deletions(-)

For the series,

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

[PATCH v3 0/3] net/mlx5: implement set_mc_addr devop

[Nelio Laranjeiro]

Support eth_dev_ops.set_mc_addr_list().

Changes in v3:

Check PMD as enough resources to process the Multicast address list.

Changes in v2:

Simplify the code and are even more verifications.

Nelio Laranjeiro (3):
  net/mlx5: more checks on MAC addresses
  net/mlx5: split MAC address add/remove code
  net/mlx5: implement multicast add list devop

 drivers/net/mlx5/mlx5.c        |   2 +
 drivers/net/mlx5/mlx5.h        |   2 +
 drivers/net/mlx5/mlx5_defs.h   |   7 +-
 drivers/net/mlx5/mlx5_ethdev.c |   2 +-
 drivers/net/mlx5/mlx5_mac.c    | 113 ++++++++++++++++++++++++++++-----
 5 files changed, 108 insertions(+), 18 deletions(-)

-- 
2.17.0

[PATCH v3 1/3] net/mlx5: more checks on MAC addresses

[Nelio Laranjeiro]

Verify MAC address before further process.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5_mac.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index edc7a32ae..f24b5f759 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -71,7 +71,10 @@ mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
 	const int vf = priv->config.vf;
 	int ret;
 
-	assert(index < MLX5_MAX_MAC_ADDRESSES);
+	if (index >= MLX5_MAX_MAC_ADDRESSES)
+		return;
+	if (is_zero_ether_addr(&dev->data->mac_addrs[index]))
+		return;
 	if (vf)
 		mlx5_nl_mac_addr_remove(dev, &dev->data->mac_addrs[index],
 					index);
@@ -107,7 +110,14 @@ mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
 	const int vf = priv->config.vf;
 	unsigned int i;
 
-	assert(index < MLX5_MAX_MAC_ADDRESSES);
+	if (index >= MLX5_MAX_MAC_ADDRESSES) {
+		rte_errno = EINVAL;
+		return -rte_errno;
+	}
+	if (is_zero_ether_addr(mac)) {
+		rte_errno = EINVAL;
+		return -rte_errno;
+	}
 	/* First, make sure this address isn't already configured. */
 	for (i = 0; (i != MLX5_MAX_MAC_ADDRESSES); ++i) {
 		/* Skip this index, it's going to be reconfigured. */
-- 
2.17.0

[PATCH v3 2/3] net/mlx5: split MAC address add/remove code

[Nelio Laranjeiro]

Move some code in DPDK callbacks to add/remove MAC addresses to internal
function.  This modification will be necessary to handle implement the
devop set_mc_addr_list.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5_defs.h   |  4 +-
 drivers/net/mlx5/mlx5_ethdev.c |  2 +-
 drivers/net/mlx5/mlx5_mac.c    | 86 +++++++++++++++++++++++++---------
 3 files changed, 68 insertions(+), 24 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index 3fb2f1480..e3aa03bef 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -13,8 +13,10 @@
 /* Reported driver name. */
 #define MLX5_DRIVER_NAME "net_mlx5"
 
+/* Maximum number of simultaneous unicast MAC addresses. */
+#define MLX5_MAX_UC_MAC_ADDRESSES 128
 /* Maximum number of simultaneous MAC addresses. */
-#define MLX5_MAX_MAC_ADDRESSES 128
+#define MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
 
 /* Maximum number of simultaneous VLAN filters. */
 #define MLX5_MAX_VLAN_IDS 128
diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index ef44cc91f..99ac4cc5c 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -446,7 +446,7 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info)
 		max = 65535;
 	info->max_rx_queues = max;
 	info->max_tx_queues = max;
-	info->max_mac_addrs = RTE_DIM(priv->mac);
+	info->max_mac_addrs = MLX5_MAX_UC_MAC_ADDRESSES;
 	info->rx_queue_offload_capa = mlx5_get_rx_queue_offloads(dev);
 	info->rx_offload_capa = (mlx5_get_rx_port_offloads() |
 				 info->rx_queue_offload_capa);
diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index f24b5f759..e9334ea4a 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -57,38 +57,30 @@ mlx5_get_mac(struct rte_eth_dev *dev, uint8_t (*mac)[ETHER_ADDR_LEN])
 }
 
 /**
- * DPDK callback to remove a MAC address.
+ * Remove a MAC address from the internal array.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
  * @param index
  *   MAC address index.
  */
-void
-mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
+static void
+mlx5_internal_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;
 
-	if (index >= MLX5_MAX_MAC_ADDRESSES)
-		return;
+	assert(index < MLX5_MAX_MAC_ADDRESSES);
 	if (is_zero_ether_addr(&dev->data->mac_addrs[index]))
 		return;
 	if (vf)
 		mlx5_nl_mac_addr_remove(dev, &dev->data->mac_addrs[index],
 					index);
 	memset(&dev->data->mac_addrs[index], 0, sizeof(struct ether_addr));
-	if (!dev->data->promiscuous) {
-		ret = mlx5_traffic_restart(dev);
-		if (ret)
-			DRV_LOG(ERR, "port %u cannot restart traffic: %s",
-				dev->data->port_id, strerror(rte_errno));
-	}
 }
 
 /**
- * DPDK callback to add a MAC address.
+ * Adds a MAC address to the internal array.
  *
  * @param dev
  *   Pointer to Ethernet device structure.
@@ -96,24 +88,19 @@ mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
  *   MAC address to register.
  * @param index
  *   MAC address index.
- * @param vmdq
- *   VMDq pool index to associate address with (ignored).
  *
  * @return
  *   0 on success, a negative errno value otherwise and rte_errno is set.
  */
-int
-mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
-		  uint32_t index, uint32_t vmdq __rte_unused)
+static int
+mlx5_internal_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
+			   uint32_t index)
 {
 	struct priv *priv = dev->data->dev_private;
 	const int vf = priv->config.vf;
 	unsigned int i;
 
-	if (index >= MLX5_MAX_MAC_ADDRESSES) {
-		rte_errno = EINVAL;
-		return -rte_errno;
-	}
+	assert(index < MLX5_MAX_MAC_ADDRESSES);
 	if (is_zero_ether_addr(mac)) {
 		rte_errno = EINVAL;
 		return -rte_errno;
@@ -136,6 +123,61 @@ mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
 			return ret;
 	}
 	dev->data->mac_addrs[index] = *mac;
+	return 0;
+}
+
+/**
+ * DPDK callback to remove a MAC address.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param index
+ *   MAC address index.
+ */
+void
+mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index)
+{
+	int ret;
+
+	if (index >= MLX5_MAX_UC_MAC_ADDRESSES)
+		return;
+	mlx5_internal_mac_addr_remove(dev, index);
+	if (!dev->data->promiscuous) {
+		ret = mlx5_traffic_restart(dev);
+		if (ret)
+			DRV_LOG(ERR, "port %u cannot restart traffic: %s",
+				dev->data->port_id, strerror(rte_errno));
+	}
+}
+
+/**
+ * DPDK callback to add a MAC address.
+ *
+ * @param dev
+ *   Pointer to Ethernet device structure.
+ * @param mac_addr
+ *   MAC address to register.
+ * @param index
+ *   MAC address index.
+ * @param vmdq
+ *   VMDq pool index to associate address with (ignored).
+ *
+ * @return
+ *   0 on success, a negative errno value otherwise and rte_errno is set.
+ */
+int
+mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
+		  uint32_t index, uint32_t vmdq __rte_unused)
+{
+	int ret;
+
+	if (index >= MLX5_MAX_UC_MAC_ADDRESSES) {
+		rte_errno = EINVAL;
+		return -rte_errno;
+	}
+	ret = mlx5_internal_mac_addr_add(dev, mac, index);
+	if (ret < 0)
+		return ret;
 	if (!dev->data->promiscuous)
 		return mlx5_traffic_restart(dev);
 	return 0;
-- 
2.17.0

[PATCH v3 3/3] net/mlx5: implement multicast add list devop

[Nelio Laranjeiro]

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
---
 drivers/net/mlx5/mlx5.c      |  2 ++
 drivers/net/mlx5/mlx5.h      |  2 ++
 drivers/net/mlx5/mlx5_defs.h |  5 ++++-
 drivers/net/mlx5/mlx5_mac.c  | 29 +++++++++++++++++++++++++++++
 4 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 68783c3ac..887924d07 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -277,6 +277,7 @@ const struct eth_dev_ops mlx5_dev_ops = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set,
@@ -329,6 +330,7 @@ const struct eth_dev_ops mlx5_dev_ops_isolate = {
 	.mac_addr_remove = mlx5_mac_addr_remove,
 	.mac_addr_add = mlx5_mac_addr_add,
 	.mac_addr_set = mlx5_mac_addr_set,
+	.set_mc_addr_list = mlx5_set_mc_addr_list,
 	.mtu_set = mlx5_dev_set_mtu,
 	.vlan_strip_queue_set = mlx5_vlan_strip_queue_set,
 	.vlan_offload_set = mlx5_vlan_offload_set,
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 6ad41390a..64f025d22 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -203,6 +203,8 @@ void mlx5_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index);
 int mlx5_mac_addr_add(struct rte_eth_dev *dev, struct ether_addr *mac,
 		      uint32_t index, uint32_t vmdq);
 int mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr);
+int mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+			  struct ether_addr *mc_addr_set, uint32_t nb_mc_addr);
 
 /* mlx5_rss.c */
 
diff --git a/drivers/net/mlx5/mlx5_defs.h b/drivers/net/mlx5/mlx5_defs.h
index e3aa03bef..5973742a7 100644
--- a/drivers/net/mlx5/mlx5_defs.h
+++ b/drivers/net/mlx5/mlx5_defs.h
@@ -15,8 +15,11 @@
 
 /* Maximum number of simultaneous unicast MAC addresses. */
 #define MLX5_MAX_UC_MAC_ADDRESSES 128
+/* Maximum number of simultaneous Multicast MAC addresses. */
+#define MLX5_MAX_MC_MAC_ADDRESSES 128
 /* Maximum number of simultaneous MAC addresses. */
-#define MLX5_MAX_MAC_ADDRESSES MLX5_MAX_UC_MAC_ADDRESSES
+#define MLX5_MAX_MAC_ADDRESSES \
+	(MLX5_MAX_UC_MAC_ADDRESSES + MLX5_MAX_MC_MAC_ADDRESSES)
 
 /* Maximum number of simultaneous VLAN filters. */
 #define MLX5_MAX_VLAN_IDS 128
diff --git a/drivers/net/mlx5/mlx5_mac.c b/drivers/net/mlx5/mlx5_mac.c
index e9334ea4a..672a47619 100644
--- a/drivers/net/mlx5/mlx5_mac.c
+++ b/drivers/net/mlx5/mlx5_mac.c
@@ -201,3 +201,32 @@ mlx5_mac_addr_set(struct rte_eth_dev *dev, struct ether_addr *mac_addr)
 		dev->data->port_id);
 	return mlx5_mac_addr_add(dev, mac_addr, 0, 0);
 }
+
+/**
+ * DPDK callback to set multicast addresses list.
+ *
+ * @see rte_eth_dev_set_mc_addr_list()
+ */
+int
+mlx5_set_mc_addr_list(struct rte_eth_dev *dev,
+		      struct ether_addr *mc_addr_set, uint32_t nb_mc_addr)
+{
+	uint32_t i;
+	int ret;
+
+	if (nb_mc_addr >= MLX5_MAX_MC_MAC_ADDRESSES) {
+		rte_errno = ENOSPC;
+		return -rte_errno;
+	}
+	for (i = MLX5_MAX_UC_MAC_ADDRESSES; i != MLX5_MAX_MAC_ADDRESSES; ++i)
+		mlx5_internal_mac_addr_remove(dev, i);
+	i = MLX5_MAX_UC_MAC_ADDRESSES;
+	while (nb_mc_addr--) {
+		ret = mlx5_internal_mac_addr_add(dev, mc_addr_set++, i++);
+		if (ret)
+			return ret;
+	}
+	if (!dev->data->promiscuous)
+		return mlx5_traffic_restart(dev);
+	return 0;
+}
-- 
2.17.0

Re: [PATCH v3 0/3] net/mlx5: implement set_mc_addr devop

[Shahaf Shuler]

Monday, April 23, 2018 2:09 PM, Nelio Laranjeiro:
> Subject: [dpdk-dev] [PATCH v3 0/3] net/mlx5: implement set_mc_addr
> devop
> 
> Support eth_dev_ops.set_mc_addr_list().
> 
> Changes in v3:
> 
> Check PMD as enough resources to process the Multicast address list.
> 
> Changes in v2:
> 
> Simplify the code and are even more verifications.
> 
> Nelio Laranjeiro (3):
>   net/mlx5: more checks on MAC addresses
>   net/mlx5: split MAC address add/remove code
>   net/mlx5: implement multicast add list devop
> 
>  drivers/net/mlx5/mlx5.c        |   2 +
>  drivers/net/mlx5/mlx5.h        |   2 +
>  drivers/net/mlx5/mlx5_defs.h   |   7 +-
>  drivers/net/mlx5/mlx5_ethdev.c |   2 +-
>  drivers/net/mlx5/mlx5_mac.c    | 113 ++++++++++++++++++++++++++++---
> --
>  5 files changed, 108 insertions(+), 18 deletions(-)

Applied to next-net-mlx, thanks. 

> 
> --
> 2.17.0