[PATCH net-next v2 0/4] Support MACsec VLAN

[PATCH net-next v2 0/4] Support MACsec VLAN

[Emeel Hakim]

Dear maintainers,

This patch series introduces support for hardware (HW) offload MACsec
devices with VLAN configuration. The patches address both scenarios
where the VLAN header is both the inner and outer header for MACsec.

The changes include:

1. Adding MACsec offload operation for VLAN.
2. Considering VLAN when accessing MACsec net device.
3. Currently offloading MACsec when it's configured over VLAN with
current MACsec TX steering rules would wrongly insert the MACsec sec tag
after inserting the VLAN header. This resulted in an ETHERNET | SECTAG |
VLAN packet when ETHERNET | VLAN | SECTAG is configured. The patche
handles this issue when configuring steering rules.
4. Adding MACsec rx_handler change support in case of a marked skb and a
mismatch on the dst MAC address.

Please review these changes and let me know if you have any feedback or
concerns.

Updates since v1:
- Consult vlan_features when adding NETIF_F_HW_MACSEC.
- Allow grep for the functions.
- Add helper function to get the macsec operation to allow the compiler
  to make some choice.

Thanks,
Emeel

Emeel Hakim (4):
  vlan: Add MACsec offload operations for VLAN interface
  net/mlx5: Support MACsec over VLAN
  net/mlx5: Consider VLAN interface in MACsec TX steering rules
  macsec: Add MACsec rx_handler change support

 .../mellanox/mlx5/core/en_accel/macsec.c      |  42 +++++---
 .../mellanox/mlx5/core/en_accel/macsec_fs.c   |   7 ++
 .../net/ethernet/mellanox/mlx5/core/en_main.c |   1 +
 drivers/net/macsec.c                          |   9 ++
 net/8021q/vlan_dev.c                          | 101 ++++++++++++++++++
 5 files changed, 144 insertions(+), 16 deletions(-)

-- 
2.21.3

[PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Emeel Hakim]

Add support for MACsec offload operations for VLAN driver
to allow offloading MACsec when VLAN's real device supports
Macsec offload by forwarding the offload request to it.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
		  - Allow grep for the functions.
		  - Add helper function to get the macsec operation to allow the compiler to make some choice.
 .../net/ethernet/mellanox/mlx5/core/en_main.c |   1 +
 net/8021q/vlan_dev.c                          | 101 ++++++++++++++++++
 2 files changed, 102 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 6db1aff8778d..5ecef26e83c6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -5076,6 +5076,7 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
 
 	netdev->vlan_features    |= NETIF_F_SG;
 	netdev->vlan_features    |= NETIF_F_HW_CSUM;
+	netdev->vlan_features    |= NETIF_F_HW_MACSEC;
 	netdev->vlan_features    |= NETIF_F_GRO;
 	netdev->vlan_features    |= NETIF_F_TSO;
 	netdev->vlan_features    |= NETIF_F_TSO6;
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 5920544e93e8..08d063f1e5b6 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -26,6 +26,7 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 #include <net/arp.h>
+#include <net/macsec.h>
 
 #include "vlan.h"
 #include "vlanproc.h"
@@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
 			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
 			   NETIF_F_ALL_FCOE;
 
+	if (real_dev->features & NETIF_F_HW_MACSEC)
+		dev->hw_features |= NETIF_F_HW_MACSEC;
+
 	dev->features |= dev->hw_features | NETIF_F_LLTX;
 	netif_inherit_tso_max(dev, real_dev);
 	if (dev->features & NETIF_F_VLAN_FEATURES)
@@ -803,6 +807,100 @@ static int vlan_dev_fill_forward_path(struct net_device_path_ctx *ctx,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_MACSEC)
+
+static const struct macsec_ops *vlan_get_macsec_ops(struct macsec_context *ctx)
+{
+	return vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops;
+}
+
+#define _BUILD_VLAN_MACSEC_MDO(mdo) \
+	const struct macsec_ops *ops; \
+	ops =  vlan_get_macsec_ops(ctx); \
+	return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP
+
+static int vlan_macsec_add_txsa(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(add_txsa);
+}
+
+static int vlan_macsec_upd_txsa(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(upd_txsa);
+}
+
+static int vlan_macsec_del_txsa(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(del_txsa);
+}
+
+static int vlan_macsec_add_rxsa(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(add_rxsa);
+}
+
+static int vlan_macsec_upd_rxsa(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(upd_rxsa);
+}
+
+static int vlan_macsec_del_rxsa(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(del_rxsa);
+}
+
+static int vlan_macsec_add_rxsc(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(add_rxsc);
+}
+
+static int vlan_macsec_upd_rxsc(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(upd_rxsc);
+}
+
+static int vlan_macsec_del_rxsc(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(del_rxsc);
+}
+
+static int vlan_macsec_add_secy(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(add_secy);
+}
+
+static int vlan_macsec_upd_secy(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(upd_secy);
+}
+
+static int vlan_macsec_del_secy(struct macsec_context *ctx)
+{
+_BUILD_VLAN_MACSEC_MDO(del_secy);
+}
+
+#undef _BUILD_VLAN_MACSEC_MDO
+
+#define VLAN_MACSEC_DECLARE_MDO(mdo) .mdo_ ## mdo = vlan_macsec_ ## mdo
+
+static const struct macsec_ops macsec_offload_ops = {
+	VLAN_MACSEC_DECLARE_MDO(add_txsa),
+	VLAN_MACSEC_DECLARE_MDO(upd_txsa),
+	VLAN_MACSEC_DECLARE_MDO(del_txsa),
+	VLAN_MACSEC_DECLARE_MDO(add_rxsc),
+	VLAN_MACSEC_DECLARE_MDO(upd_rxsc),
+	VLAN_MACSEC_DECLARE_MDO(del_rxsc),
+	VLAN_MACSEC_DECLARE_MDO(add_rxsa),
+	VLAN_MACSEC_DECLARE_MDO(upd_rxsa),
+	VLAN_MACSEC_DECLARE_MDO(del_rxsa),
+	VLAN_MACSEC_DECLARE_MDO(add_secy),
+	VLAN_MACSEC_DECLARE_MDO(upd_secy),
+	VLAN_MACSEC_DECLARE_MDO(del_secy),
+};
+
+#undef VLAN_MACSEC_DECLARE_MDO
+#endif
+
 static const struct ethtool_ops vlan_ethtool_ops = {
 	.get_link_ksettings	= vlan_ethtool_get_link_ksettings,
 	.get_drvinfo	        = vlan_ethtool_get_drvinfo,
@@ -869,6 +967,9 @@ void vlan_setup(struct net_device *dev)
 	dev->priv_destructor	= vlan_dev_free;
 	dev->ethtool_ops	= &vlan_ethtool_ops;
 
+#if IS_ENABLED(CONFIG_MACSEC)
+	dev->macsec_ops		= &macsec_offload_ops;
+#endif
 	dev->min_mtu		= 0;
 	dev->max_mtu		= ETH_MAX_MTU;
 
-- 
2.21.3

[PATCH net-next 2/4] net/mlx5: Support MACsec over VLAN

[Emeel Hakim]

MACsec device may have a VLAN device on top of it.
Detect MACsec state correctly under this condition,
and return the correct net device accordingly.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/macsec.c      | 42 ++++++++++++-------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 33b3620ea45c..f1646fa6737d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -4,6 +4,7 @@
 #include <linux/mlx5/device.h>
 #include <linux/mlx5/mlx5_ifc.h>
 #include <linux/xarray.h>
+#include <linux/if_vlan.h>
 
 #include "en.h"
 #include "lib/aso.h"
@@ -348,12 +349,21 @@ static void mlx5e_macsec_cleanup_sa(struct mlx5e_macsec *macsec,
 	sa->macsec_rule = NULL;
 }
 
+static inline struct mlx5e_priv *macsec_netdev_priv(const struct net_device *dev)
+{
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+	if (is_vlan_dev(dev))
+		return netdev_priv(vlan_dev_priv(dev)->real_dev);
+#endif
+	return netdev_priv(dev);
+}
+
 static int mlx5e_macsec_init_sa(struct macsec_context *ctx,
 				struct mlx5e_macsec_sa *sa,
 				bool encrypt,
 				bool is_tx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec *macsec = priv->macsec;
 	struct mlx5_macsec_rule_attrs rule_attrs;
 	struct mlx5_core_dev *mdev = priv->mdev;
@@ -427,7 +437,7 @@ static int macsec_rx_sa_active_update(struct macsec_context *ctx,
 				      struct mlx5e_macsec_sa *rx_sa,
 				      bool active)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec *macsec = priv->macsec;
 	int err = 0;
 
@@ -510,7 +520,7 @@ static int mlx5e_macsec_add_txsa(struct macsec_context *ctx)
 {
 	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
 	const struct macsec_tx_sa *ctx_tx_sa = ctx->sa.tx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_secy *secy = ctx->secy;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5_core_dev *mdev = priv->mdev;
@@ -585,7 +595,7 @@ static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
 {
 	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
 	const struct macsec_tx_sa *ctx_tx_sa = ctx->sa.tx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	u8 assoc_num = ctx->sa.assoc_num;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -645,7 +655,7 @@ static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_del_txsa(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	u8 assoc_num = ctx->sa.assoc_num;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -696,7 +706,7 @@ static u32 mlx5e_macsec_get_sa_from_hashtable(struct rhashtable *sci_hash, sci_t
 static int mlx5e_macsec_add_rxsc(struct macsec_context *ctx)
 {
 	struct mlx5e_macsec_rx_sc_xarray_element *sc_xarray_element;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sc *ctx_rx_sc = ctx->rx_sc;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -776,7 +786,7 @@ static int mlx5e_macsec_add_rxsc(struct macsec_context *ctx)
 
 static int mlx5e_macsec_upd_rxsc(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sc *ctx_rx_sc = ctx->rx_sc;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -854,7 +864,7 @@ static void macsec_del_rxsc_ctx(struct mlx5e_macsec *macsec, struct mlx5e_macsec
 
 static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
 	struct mlx5e_macsec *macsec;
@@ -890,8 +900,8 @@ static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
 
 static int mlx5e_macsec_add_rxsa(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sa *ctx_rx_sa = ctx->sa.rx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5_core_dev *mdev = priv->mdev;
 	u8 assoc_num = ctx->sa.assoc_num;
@@ -976,8 +986,8 @@ static int mlx5e_macsec_add_rxsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_upd_rxsa(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sa *ctx_rx_sa = ctx->sa.rx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	u8 assoc_num = ctx->sa.assoc_num;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -1033,7 +1043,7 @@ static int mlx5e_macsec_upd_rxsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_del_rxsa(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	sci_t sci = ctx->sa.rx_sa->sc->sci;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -1085,7 +1095,7 @@ static int mlx5e_macsec_del_rxsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_add_secy(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct net_device *dev = ctx->secy->netdev;
 	const struct net_device *netdev = ctx->netdev;
 	struct mlx5e_macsec_device *macsec_device;
@@ -1137,7 +1147,7 @@ static int mlx5e_macsec_add_secy(struct macsec_context *ctx)
 static int macsec_upd_secy_hw_address(struct macsec_context *ctx,
 				      struct mlx5e_macsec_device *macsec_device)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct net_device *dev = ctx->secy->netdev;
 	struct mlx5e_macsec *macsec = priv->macsec;
 	struct mlx5e_macsec_rx_sc *rx_sc, *tmp;
@@ -1184,8 +1194,8 @@ static int macsec_upd_secy_hw_address(struct macsec_context *ctx,
  */
 static int mlx5e_macsec_upd_secy(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	const struct net_device *dev = ctx->secy->netdev;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -1240,7 +1250,7 @@ static int mlx5e_macsec_upd_secy(struct macsec_context *ctx)
 
 static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc, *tmp;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -1741,7 +1751,7 @@ void mlx5e_macsec_offload_handle_rx_skb(struct net_device *netdev,
 {
 	struct mlx5e_macsec_rx_sc_xarray_element *sc_xarray_element;
 	u32 macsec_meta_data = be32_to_cpu(cqe->ft_metadata);
-	struct mlx5e_priv *priv = netdev_priv(netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(netdev);
 	struct mlx5e_macsec_rx_sc *rx_sc;
 	struct mlx5e_macsec *macsec;
 	u32  fs_id;
-- 
2.21.3

[PATCH net-next 3/4] net/mlx5: Consider VLAN interface in MACsec TX steering rules

[Emeel Hakim]

Offloading MACsec when its configured over VLAN with current MACsec
TX steering rules will wrongly insert MACsec sec tag after inserting
the VLAN header leading to a ETHERNET | SECTAG | VLAN packet when
ETHERNET | VLAN | SECTAG is configured.

The above issue is due to adding the SECTAG by HW which is a later
stage compared to the VLAN header insertion stage.

Detect such a case and adjust TX steering rules to insert the
SECTAG in the correct place by using reformat_param_0 field in
the packet reformat to indicate the offset of SECTAG from end of
the MAC header to account for VLANs in granularity of 4Bytes.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c   | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
index 5b658a5588c6..daaaaf344f77 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
@@ -4,6 +4,7 @@
 #include <net/macsec.h>
 #include <linux/netdevice.h>
 #include <linux/mlx5/qp.h>
+#include <linux/if_vlan.h>
 #include "fs_core.h"
 #include "en/fs.h"
 #include "en_accel/macsec_fs.h"
@@ -510,6 +511,8 @@ static void macsec_fs_tx_del_rule(struct mlx5e_macsec_fs *macsec_fs,
 	macsec_fs_tx_ft_put(macsec_fs);
 }
 
+#define MLX5_REFORMAT_PARAM_ADD_MACSEC_OFFSET_4_BYTES 1
+
 static union mlx5e_macsec_rule *
 macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 		      const struct macsec_context *macsec_ctx,
@@ -555,6 +558,10 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 	reformat_params.type = MLX5_REFORMAT_TYPE_ADD_MACSEC;
 	reformat_params.size = reformat_size;
 	reformat_params.data = reformatbf;
+
+	if (is_vlan_dev(macsec_ctx->netdev))
+		reformat_params.param_0 = MLX5_REFORMAT_PARAM_ADD_MACSEC_OFFSET_4_BYTES;
+
 	flow_act.pkt_reformat = mlx5_packet_reformat_alloc(macsec_fs->mdev,
 							   &reformat_params,
 							   MLX5_FLOW_NAMESPACE_EGRESS_MACSEC);
-- 
2.21.3

[PATCH net-next 4/4] macsec: Add MACsec rx_handler change support

[Emeel Hakim]

Offloading device drivers will mark offloaded MACsec SKBs with the
corresponding SCI in the skb_metadata_dst so the macsec rx handler will
know to which interface to divert those skbs, in case of a marked skb
and a mismatch on the dst MAC address, divert the skb to the macsec
net_device where the macsec rx_handler will be called.

Example of such a case is having a MACsec with VLAN as an inner header
ETHERNET | SECTAG | VLAN packet.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 drivers/net/macsec.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 25616247d7a5..88b00ea4af68 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1048,6 +1048,15 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 
 				__netif_rx(nskb);
 			}
+
+			if (md_dst && md_dst->type == METADATA_MACSEC &&
+			    (find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci))) {
+				skb->dev = ndev;
+				skb->pkt_type = PACKET_HOST;
+				ret = RX_HANDLER_ANOTHER;
+				goto out;
+			}
+
 			continue;
 		}
 
-- 
2.21.3

Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Sabrina Dubroca]

2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> Add support for MACsec offload operations for VLAN driver
> to allow offloading MACsec when VLAN's real device supports
> Macsec offload by forwarding the offload request to it.
> 
> Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> ---
> V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.

Uh? You're not actually doing that? You also dropped the
changes to vlan_dev_fix_features without explaining why.

[...]
> @@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
>  			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
>  			   NETIF_F_ALL_FCOE;
>  
> +	if (real_dev->features & NETIF_F_HW_MACSEC)
> +		dev->hw_features |= NETIF_F_HW_MACSEC;
> +
>  	dev->features |= dev->hw_features | NETIF_F_LLTX;
>  	netif_inherit_tso_max(dev, real_dev);
>  	if (dev->features & NETIF_F_VLAN_FEATURES)
> @@ -803,6 +807,100 @@ static int vlan_dev_fill_forward_path(struct net_device_path_ctx *ctx,
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_MACSEC)
> +
> +static const struct macsec_ops *vlan_get_macsec_ops(struct macsec_context *ctx)
                                                       ^ const?

> +{
> +	return vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops;
> +}
> +
> +#define _BUILD_VLAN_MACSEC_MDO(mdo) \
> +	const struct macsec_ops *ops; \
> +	ops =  vlan_get_macsec_ops(ctx); \
> +	return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP
> +
> +static int vlan_macsec_add_txsa(struct macsec_context *ctx)
> +{
> +_BUILD_VLAN_MACSEC_MDO(add_txsa);

Shouldn't those be indented?

> +}
> +

[...]
> +
> +#define VLAN_MACSEC_DECLARE_MDO(mdo) .mdo_ ## mdo = vlan_macsec_ ## mdo
> +
> +static const struct macsec_ops macsec_offload_ops = {
> +	VLAN_MACSEC_DECLARE_MDO(add_txsa),

This completely breaks the ability to use cscope when looking for
implementations of mdo_add_txsa. I'm not very fond of the c/p, but I
don't think we should be using macros at all here. At least to me,
being able to navigate directly from mdo_add_txsa to its
implementation without expanding the macro manually is important.

So, IMHO, those should be:

	.mdo_add_txsa = vlan_macsec_add_txsa,

(etc)

-- 
Sabrina

Re: [PATCH net-next v2 0/4] Support MACsec VLAN

[Leon Romanovsky]

On Wed, Mar 29, 2023 at 03:21:03PM +0300, Emeel Hakim wrote:
> Dear maintainers,
> 
> This patch series introduces support for hardware (HW) offload MACsec
> devices with VLAN configuration. The patches address both scenarios
> where the VLAN header is both the inner and outer header for MACsec.
> 
> The changes include:
> 
> 1. Adding MACsec offload operation for VLAN.
> 2. Considering VLAN when accessing MACsec net device.
> 3. Currently offloading MACsec when it's configured over VLAN with
> current MACsec TX steering rules would wrongly insert the MACsec sec tag
> after inserting the VLAN header. This resulted in an ETHERNET | SECTAG |
> VLAN packet when ETHERNET | VLAN | SECTAG is configured. The patche
> handles this issue when configuring steering rules.
> 4. Adding MACsec rx_handler change support in case of a marked skb and a
> mismatch on the dst MAC address.
> 
> Please review these changes and let me know if you have any feedback or
> concerns.
> 
> Updates since v1:
> - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> - Allow grep for the functions.
> - Add helper function to get the macsec operation to allow the compiler
>   to make some choice.

Please mark all your patches as vXXX and not only one.

Thanks

Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Leon Romanovsky]

On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > Add support for MACsec offload operations for VLAN driver
> > to allow offloading MACsec when VLAN's real device supports
> > Macsec offload by forwarding the offload request to it.
> > 
> > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > ---
> > V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> 
> Uh? You're not actually doing that? You also dropped the
> changes to vlan_dev_fix_features without explaining why.

vlan_dev_fix_features() relies on real_dev->vlan_features which was set
in mlx5 part of this patch.

  643 static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
  644         netdev_features_t features)
  645 {
  ...
  649
  650         lower_features = netdev_intersect_features((real_dev->vlan_features |
  651                                                     NETIF_F_RXCSUM),
  652                                                    real_dev->features);

This part ensure that once real_dev->vlan_features and real_dev->features have NETIF_F_HW_MACSEC,
the returned features will include NETIF_F_HW_MACSEC too.

> 
> [...]
> > @@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
> >  			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
> >  			   NETIF_F_ALL_FCOE;
> >  
> > +	if (real_dev->features & NETIF_F_HW_MACSEC)
> > +		dev->hw_features |= NETIF_F_HW_MACSEC;
> > +
> >  	dev->features |= dev->hw_features | NETIF_F_LLTX;
> >  	netif_inherit_tso_max(dev, real_dev);
> >  	if (dev->features & NETIF_F_VLAN_FEATURES)
> > @@ -803,6 +807,100 @@ static int vlan_dev_fill_forward_path(struct net_device_path_ctx *ctx,
> >  	return 0;
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_MACSEC)
> > +
> > +static const struct macsec_ops *vlan_get_macsec_ops(struct macsec_context *ctx)
>                                                        ^ const?
> 
> > +{
> > +	return vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops;
> > +}
> > +
> > +#define _BUILD_VLAN_MACSEC_MDO(mdo) \
> > +	const struct macsec_ops *ops; \
> > +	ops =  vlan_get_macsec_ops(ctx); \
> > +	return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP
> > +
> > +static int vlan_macsec_add_txsa(struct macsec_context *ctx)
> > +{
> > +_BUILD_VLAN_MACSEC_MDO(add_txsa);
> 
> Shouldn't those be indented?
> 
> > +}
> > +
> 
> [...]
> > +
> > +#define VLAN_MACSEC_DECLARE_MDO(mdo) .mdo_ ## mdo = vlan_macsec_ ## mdo
> > +
> > +static const struct macsec_ops macsec_offload_ops = {
> > +	VLAN_MACSEC_DECLARE_MDO(add_txsa),
> 
> This completely breaks the ability to use cscope when looking for
> implementations of mdo_add_txsa. I'm not very fond of the c/p, but I
> don't think we should be using macros at all here. At least to me,
> being able to navigate directly from mdo_add_txsa to its
> implementation without expanding the macro manually is important.
> 
> So, IMHO, those should be:
> 
> 	.mdo_add_txsa = vlan_macsec_add_txsa,

Completely agree with you.

> 
> (etc)
> 
> -- 
> Sabrina
> 

Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Sabrina Dubroca]

2023-03-29, 21:42:01 +0300, Leon Romanovsky wrote:
> On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> > 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > > Add support for MACsec offload operations for VLAN driver
> > > to allow offloading MACsec when VLAN's real device supports
> > > Macsec offload by forwarding the offload request to it.
> > > 
> > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > > ---
> > > V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> > 
> > Uh? You're not actually doing that? You also dropped the
> > changes to vlan_dev_fix_features without explaining why.
> 
> vlan_dev_fix_features() relies on real_dev->vlan_features which was set
> in mlx5 part of this patch.
> 
>   643 static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
>   644         netdev_features_t features)
>   645 {
>   ...
>   649
>   650         lower_features = netdev_intersect_features((real_dev->vlan_features |
>   651                                                     NETIF_F_RXCSUM),
>   652                                                    real_dev->features);
> 
> This part ensure that once real_dev->vlan_features and real_dev->features have NETIF_F_HW_MACSEC,
> the returned features will include NETIF_F_HW_MACSEC too.

Ok, thanks.

But back to the issue of vlan_features, in vlan_dev_init: I'm not
convinced NETIF_F_HW_MACSEC should be added to hw_features based on
->features. That would result in a new vlan device that can't offload
macsec at all if it was created at the wrong time (while the lower
device's macsec offload was temporarily disabled). AFAIU,
vlandev->hw_features should be based on realdev->vlan_features. I
don't see a reason to advertise a feature in the vlan device if we
won't ever be able to turn it on because it's not in ->vlan_features
("grmbl why can't I enable it, ethtool says it's here?!").


Emeel, I'm not a maintainer, but I don't think you should be reposting
until the existing discussion has settled down.

> > 
> > [...]
> > > @@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
> > >  			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
> > >  			   NETIF_F_ALL_FCOE;
> > >  
> > > +	if (real_dev->features & NETIF_F_HW_MACSEC)
> > > +		dev->hw_features |= NETIF_F_HW_MACSEC;
> > > +
> > >  	dev->features |= dev->hw_features | NETIF_F_LLTX;
> > >  	netif_inherit_tso_max(dev, real_dev);
> > >  	if (dev->features & NETIF_F_VLAN_FEATURES)

-- 
Sabrina

Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Jakub Kicinski]

On Thu, 30 Mar 2023 19:19:21 +0200 Sabrina Dubroca wrote:
> I don't think you should be reposting
> until the existing discussion has settled down.

Thanks for that note, very true, v3 discarded.

Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Leon Romanovsky]

On Thu, Mar 30, 2023 at 07:19:21PM +0200, Sabrina Dubroca wrote:
> 2023-03-29, 21:42:01 +0300, Leon Romanovsky wrote:
> > On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> > > 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > > > Add support for MACsec offload operations for VLAN driver
> > > > to allow offloading MACsec when VLAN's real device supports
> > > > Macsec offload by forwarding the offload request to it.
> > > > 
> > > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > > > ---
> > > > V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> > > 
> > > Uh? You're not actually doing that? You also dropped the
> > > changes to vlan_dev_fix_features without explaining why.
> > 
> > vlan_dev_fix_features() relies on real_dev->vlan_features which was set
> > in mlx5 part of this patch.
> > 
> >   643 static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
> >   644         netdev_features_t features)
> >   645 {
> >   ...
> >   649
> >   650         lower_features = netdev_intersect_features((real_dev->vlan_features |
> >   651                                                     NETIF_F_RXCSUM),
> >   652                                                    real_dev->features);
> > 
> > This part ensure that once real_dev->vlan_features and real_dev->features have NETIF_F_HW_MACSEC,
> > the returned features will include NETIF_F_HW_MACSEC too.
> 
> Ok, thanks.
> 
> But back to the issue of vlan_features, in vlan_dev_init: I'm not
> convinced NETIF_F_HW_MACSEC should be added to hw_features based on
> ->features. That would result in a new vlan device that can't offload
> macsec at all if it was created at the wrong time (while the lower
> device's macsec offload was temporarily disabled). 

Sorry, I'm new to this netdev features zoo, but if I read correctly 
Documentation/networking/netdev-features.rst, the ->features is the list
of enabled ones:

   29  2. netdev->features set contains features which are currently enabled
   30     for a device.  This should be changed only by network core or in
   31     error paths of ndo_set_features callback.

And user will have a chance to disable it for VLAN because it was added
to ->hw_features:

   24  1. netdev->hw_features set contains features whose state may possibly
   25     be changed (enabled or disabled) for a particular device by user's
   26     request.  This set should be initialized in ndo_init callback and not
   27     changed later.

So how can VLAN be created with NETIF_F_HW_MACSEC while real_dev mcasec
offload is disabled?

> AFAIU, vlandev->hw_features should be based on realdev->vlan_features. 

Is this macsec offloaded VLAN can be called "child VLAN device"?

   33  3. netdev->vlan_features set contains features whose state is inherited
   34     by child VLAN devices (limits netdev->features set).  This is currently
   35     used for all VLAN devices whether tags are stripped or inserted in
   36     hardware or software.

> I don't see a reason to advertise a feature in the vlan device if we
> won't ever be able to turn it on because it's not in ->vlan_features
> ("grmbl why can't I enable it, ethtool says it's here?!").
> 
> 
> Emeel, I'm not a maintainer, but I don't think you should be reposting
> until the existing discussion has settled down.
> 
> > > 
> > > [...]
> > > > @@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
> > > >  			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
> > > >  			   NETIF_F_ALL_FCOE;
> > > >  
> > > > +	if (real_dev->features & NETIF_F_HW_MACSEC)
> > > > +		dev->hw_features |= NETIF_F_HW_MACSEC;
> > > > +
> > > >  	dev->features |= dev->hw_features | NETIF_F_LLTX;
> > > >  	netif_inherit_tso_max(dev, real_dev);
> > > >  	if (dev->features & NETIF_F_VLAN_FEATURES)
> 
> -- 
> Sabrina
> 

Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Sabrina Dubroca]

2023-03-30, 21:56:56 +0300, Leon Romanovsky wrote:
> On Thu, Mar 30, 2023 at 07:19:21PM +0200, Sabrina Dubroca wrote:
> > 2023-03-29, 21:42:01 +0300, Leon Romanovsky wrote:
> > > On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> > > > 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > > > > Add support for MACsec offload operations for VLAN driver
> > > > > to allow offloading MACsec when VLAN's real device supports
> > > > > Macsec offload by forwarding the offload request to it.
> > > > > 
> > > > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > > > > ---
> > > > > V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> > > > 
> > > > Uh? You're not actually doing that? You also dropped the
> > > > changes to vlan_dev_fix_features without explaining why.
> > > 
> > > vlan_dev_fix_features() relies on real_dev->vlan_features which was set
> > > in mlx5 part of this patch.
> > > 
> > >   643 static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
> > >   644         netdev_features_t features)
> > >   645 {
> > >   ...
> > >   649
> > >   650         lower_features = netdev_intersect_features((real_dev->vlan_features |
> > >   651                                                     NETIF_F_RXCSUM),
> > >   652                                                    real_dev->features);
> > > 
> > > This part ensure that once real_dev->vlan_features and real_dev->features have NETIF_F_HW_MACSEC,
> > > the returned features will include NETIF_F_HW_MACSEC too.
> > 
> > Ok, thanks.
> > 
> > But back to the issue of vlan_features, in vlan_dev_init: I'm not
> > convinced NETIF_F_HW_MACSEC should be added to hw_features based on
> > ->features. That would result in a new vlan device that can't offload
> > macsec at all if it was created at the wrong time (while the lower
> > device's macsec offload was temporarily disabled). 
> 
> Sorry, I'm new to this netdev features zoo, but if I read correctly 
> Documentation/networking/netdev-features.rst, the ->features is the list
> of enabled ones:
> 
>    29  2. netdev->features set contains features which are currently enabled
>    30     for a device.  This should be changed only by network core or in
>    31     error paths of ndo_set_features callback.
> 
> And user will have a chance to disable it for VLAN because it was added
> to ->hw_features:
> 
>    24  1. netdev->hw_features set contains features whose state may possibly
>    25     be changed (enabled or disabled) for a particular device by user's
>    26     request.  This set should be initialized in ndo_init callback and not
>    27     changed later.
> 
> So how can VLAN be created with NETIF_F_HW_MACSEC while real_dev mcasec
> offload is disabled?

I'm proposing that be VLAN device be created with the capability
(->hw_features contains NETIF_F_HW_MACSEC) but disabled (->features
doesn't contain NETIF_F_HW_MACSEC). That way, if NETIF_F_HW_MACSEC is
re-enabled on the lower device, you don't need to destroy the VLAN
device to enable macsec offload on it as well. You still won't be able
to enable macsec offload on the VLAN device unless it's active on the
real NIC.

I think whether the lower device currently has NETIF_F_HW_MACSEC
should only affect whether you can enable the feature on the vlan
device right now. What feature is enabled at creation time should be
irrelevant.

> > AFAIU, vlandev->hw_features should be based on realdev->vlan_features. 
> 
> Is this macsec offloaded VLAN can be called "child VLAN device"?
>
>    33  3. netdev->vlan_features set contains features whose state is inherited
>    34     by child VLAN devices (limits netdev->features set).  This is currently
>    35     used for all VLAN devices whether tags are stripped or inserted in
>    36     hardware or software.

Yes. In this patch, we're talking about this situation:

          eth0  --------------> vlan0 --------------> macsec0
       real NIC
     (capable of               (can also
   offloading MACsec)        offload MACsec)

And vlan0 would be a "child VLAN device" of eth0.

"limits netdev->features set" is the netdev_intersect_features you
quoted in your previous email.

> > I don't see a reason to advertise a feature in the vlan device if we
> > won't ever be able to turn it on because it's not in ->vlan_features
> > ("grmbl why can't I enable it, ethtool says it's here?!").
> > 
> > 
> > Emeel, I'm not a maintainer, but I don't think you should be reposting
> > until the existing discussion has settled down.
> > 
> > > > 
> > > > [...]
> > > > > @@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
> > > > >  			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
> > > > >  			   NETIF_F_ALL_FCOE;
> > > > >  
> > > > > +	if (real_dev->features & NETIF_F_HW_MACSEC)
> > > > > +		dev->hw_features |= NETIF_F_HW_MACSEC;
> > > > > +
> > > > >  	dev->features |= dev->hw_features | NETIF_F_LLTX;
> > > > >  	netif_inherit_tso_max(dev, real_dev);
> > > > >  	if (dev->features & NETIF_F_VLAN_FEATURES)
> > 
> > -- 
> > Sabrina
> > 

-- 
Sabrina

RE: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Emeel Hakim]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, 30 March 2023 20:43
> To: Sabrina Dubroca <sd@queasysnail.net>
> Cc: Leon Romanovsky <leon@kernel.org>; Emeel Hakim <ehakim@nvidia.com>;
> davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN
> interface
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 30 Mar 2023 19:19:21 +0200 Sabrina Dubroca wrote:
> > I don't think you should be reposting
> > until the existing discussion has settled down.
> 
> Thanks for that note, very true, v3 discarded.

Sure, I thought it was closed after Leon's reply, so I addressed the comments and reposted.
My bad.

RE: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Emeel Hakim]

> -----Original Message-----
> From: Sabrina Dubroca <sd@queasysnail.net>
> Sent: Thursday, 30 March 2023 23:33
> To: Leon Romanovsky <leon@kernel.org>
> Cc: Emeel Hakim <ehakim@nvidia.com>; davem@davemloft.net; kuba@kernel.org;
> pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN
> interface
> 
> External email: Use caution opening links or attachments
> 
> 
> 2023-03-30, 21:56:56 +0300, Leon Romanovsky wrote:
> > On Thu, Mar 30, 2023 at 07:19:21PM +0200, Sabrina Dubroca wrote:
> > > 2023-03-29, 21:42:01 +0300, Leon Romanovsky wrote:
> > > > On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> > > > > 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > > > > > Add support for MACsec offload operations for VLAN driver to
> > > > > > allow offloading MACsec when VLAN's real device supports
> > > > > > Macsec offload by forwarding the offload request to it.
> > > > > >
> > > > > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > > > > > ---
> > > > > > V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> > > > >
> > > > > Uh? You're not actually doing that? You also dropped the changes
> > > > > to vlan_dev_fix_features without explaining why.
> > > >
> > > > vlan_dev_fix_features() relies on real_dev->vlan_features which
> > > > was set in mlx5 part of this patch.
> > > >
> > > >   643 static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
> > > >   644         netdev_features_t features)
> > > >   645 {
> > > >   ...
> > > >   649
> > > >   650         lower_features = netdev_intersect_features((real_dev-
> >vlan_features |
> > > >   651                                                     NETIF_F_RXCSUM),
> > > >   652                                                    real_dev->features);
> > > >
> > > > This part ensure that once real_dev->vlan_features and
> > > > real_dev->features have NETIF_F_HW_MACSEC, the returned features will
> include NETIF_F_HW_MACSEC too.
> > >
> > > Ok, thanks.
> > >
> > > But back to the issue of vlan_features, in vlan_dev_init: I'm not
> > > convinced NETIF_F_HW_MACSEC should be added to hw_features based on
> > > ->features. That would result in a new vlan device that can't
> > > ->offload
> > > macsec at all if it was created at the wrong time (while the lower
> > > device's macsec offload was temporarily disabled).
> >
> > Sorry, I'm new to this netdev features zoo, but if I read correctly
> > Documentation/networking/netdev-features.rst, the ->features is the
> > list of enabled ones:
> >
> >    29  2. netdev->features set contains features which are currently enabled
> >    30     for a device.  This should be changed only by network core or in
> >    31     error paths of ndo_set_features callback.
> >
> > And user will have a chance to disable it for VLAN because it was
> > added to ->hw_features:
> >
> >    24  1. netdev->hw_features set contains features whose state may possibly
> >    25     be changed (enabled or disabled) for a particular device by user's
> >    26     request.  This set should be initialized in ndo_init callback and not
> >    27     changed later.
> >
> > So how can VLAN be created with NETIF_F_HW_MACSEC while real_dev
> > mcasec offload is disabled?
> 
> I'm proposing that be VLAN device be created with the capability (->hw_features
> contains NETIF_F_HW_MACSEC) but disabled (->features doesn't contain
> NETIF_F_HW_MACSEC). That way, if NETIF_F_HW_MACSEC is re-enabled on the
> lower device, you don't need to destroy the VLAN device to enable macsec offload
> on it as well. You still won't be able to enable macsec offload on the VLAN device
> unless it's active on the real NIC.
> 
> I think whether the lower device currently has NETIF_F_HW_MACSEC should only
> affect whether you can enable the feature on the vlan device right now. What
> feature is enabled at creation time should be irrelevant.

Thanks for the proposal Sabrina, I'm also new to this netdev features zone so IIUC your'e 
proposing that we have NETIF_F_HW_MACSEC added to the dev->hw_features upon 
vlan_dev_init, but disabled (we don’t add it to dev->features) , and upon vlan_dev_fix_features
we check if the real_device have NETIF_F_HW_MACSEC enabled (after the intersect with the real_dev->vlan_features) 
and if so we add it to the features.

So something like:

static int vlan_dev_init(struct net_device *dev)
{
...
	dev->features |= dev->hw_features | NETIF_F_LLTX;
	dev->hw_features |= NETIF_F_HW_MACSEC;
...
}

static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
         netdev_features_t features)
{
...
 	if (lower_features &  NETIF_F_HW_MACSEC)
 		features |= NETIF_F_HW_MACSEC;

return features;
}

if this is the case then I totally agree that this is a better approach.

> > > AFAIU, vlandev->hw_features should be based on realdev->vlan_features.
> >
> > Is this macsec offloaded VLAN can be called "child VLAN device"?
> >
> >    33  3. netdev->vlan_features set contains features whose state is inherited
> >    34     by child VLAN devices (limits netdev->features set).  This is currently
> >    35     used for all VLAN devices whether tags are stripped or inserted in
> >    36     hardware or software.
> 
> Yes. In this patch, we're talking about this situation:
> 
>           eth0  --------------> vlan0 --------------> macsec0
>        real NIC
>      (capable of               (can also
>    offloading MACsec)        offload MACsec)
> 
> And vlan0 would be a "child VLAN device" of eth0.
> 
> "limits netdev->features set" is the netdev_intersect_features you quoted in your
> previous email.
> 
> > > I don't see a reason to advertise a feature in the vlan device if we
> > > won't ever be able to turn it on because it's not in ->vlan_features
> > > ("grmbl why can't I enable it, ethtool says it's here?!").
> > >
> > >
> > > Emeel, I'm not a maintainer, but I don't think you should be
> > > reposting until the existing discussion has settled down.
> > >
> > > > >
> > > > > [...]
> > > > > > @@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
> > > > > >                          NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
> > > > > >                          NETIF_F_ALL_FCOE;
> > > > > >
> > > > > > +     if (real_dev->features & NETIF_F_HW_MACSEC)
> > > > > > +             dev->hw_features |= NETIF_F_HW_MACSEC;
> > > > > > +
> > > > > >       dev->features |= dev->hw_features | NETIF_F_LLTX;
> > > > > >       netif_inherit_tso_max(dev, real_dev);
> > > > > >       if (dev->features & NETIF_F_VLAN_FEATURES)
> > >
> > > --
> > > Sabrina
> > >
> 
> --
> Sabrina

Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Sabrina Dubroca]

2023-04-03, 09:29:28 +0000, Emeel Hakim wrote:
> 
> 
> > -----Original Message-----
> > From: Sabrina Dubroca <sd@queasysnail.net>
> > Sent: Thursday, 30 March 2023 23:33
> > To: Leon Romanovsky <leon@kernel.org>
> > Cc: Emeel Hakim <ehakim@nvidia.com>; davem@davemloft.net; kuba@kernel.org;
> > pabeni@redhat.com; edumazet@google.com; netdev@vger.kernel.org
> > Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN
> > interface
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > 2023-03-30, 21:56:56 +0300, Leon Romanovsky wrote:
> > > On Thu, Mar 30, 2023 at 07:19:21PM +0200, Sabrina Dubroca wrote:
> > > > 2023-03-29, 21:42:01 +0300, Leon Romanovsky wrote:
> > > > > On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> > > > > > 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > > > > > > Add support for MACsec offload operations for VLAN driver to
> > > > > > > allow offloading MACsec when VLAN's real device supports
> > > > > > > Macsec offload by forwarding the offload request to it.
> > > > > > >
> > > > > > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > > > > > > ---
> > > > > > > V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> > > > > >
> > > > > > Uh? You're not actually doing that? You also dropped the changes
> > > > > > to vlan_dev_fix_features without explaining why.
> > > > >
> > > > > vlan_dev_fix_features() relies on real_dev->vlan_features which
> > > > > was set in mlx5 part of this patch.
> > > > >
> > > > >   643 static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
> > > > >   644         netdev_features_t features)
> > > > >   645 {
> > > > >   ...
> > > > >   649
> > > > >   650         lower_features = netdev_intersect_features((real_dev-
> > >vlan_features |
> > > > >   651                                                     NETIF_F_RXCSUM),
> > > > >   652                                                    real_dev->features);
> > > > >
> > > > > This part ensure that once real_dev->vlan_features and
> > > > > real_dev->features have NETIF_F_HW_MACSEC, the returned features will
> > include NETIF_F_HW_MACSEC too.
> > > >
> > > > Ok, thanks.
> > > >
> > > > But back to the issue of vlan_features, in vlan_dev_init: I'm not
> > > > convinced NETIF_F_HW_MACSEC should be added to hw_features based on
> > > > ->features. That would result in a new vlan device that can't
> > > > ->offload
> > > > macsec at all if it was created at the wrong time (while the lower
> > > > device's macsec offload was temporarily disabled).
> > >
> > > Sorry, I'm new to this netdev features zoo, but if I read correctly
> > > Documentation/networking/netdev-features.rst, the ->features is the
> > > list of enabled ones:
> > >
> > >    29  2. netdev->features set contains features which are currently enabled
> > >    30     for a device.  This should be changed only by network core or in
> > >    31     error paths of ndo_set_features callback.
> > >
> > > And user will have a chance to disable it for VLAN because it was
> > > added to ->hw_features:
> > >
> > >    24  1. netdev->hw_features set contains features whose state may possibly
> > >    25     be changed (enabled or disabled) for a particular device by user's
> > >    26     request.  This set should be initialized in ndo_init callback and not
> > >    27     changed later.
> > >
> > > So how can VLAN be created with NETIF_F_HW_MACSEC while real_dev
> > > mcasec offload is disabled?
> > 
> > I'm proposing that be VLAN device be created with the capability (->hw_features
> > contains NETIF_F_HW_MACSEC) but disabled (->features doesn't contain
> > NETIF_F_HW_MACSEC). That way, if NETIF_F_HW_MACSEC is re-enabled on the
> > lower device, you don't need to destroy the VLAN device to enable macsec offload
> > on it as well. You still won't be able to enable macsec offload on the VLAN device
> > unless it's active on the real NIC.
> > 
> > I think whether the lower device currently has NETIF_F_HW_MACSEC should only
> > affect whether you can enable the feature on the vlan device right now. What
> > feature is enabled at creation time should be irrelevant.
> 
> Thanks for the proposal Sabrina, I'm also new to this netdev features zone so IIUC your'e 
> proposing that we have NETIF_F_HW_MACSEC added to the dev->hw_features upon 
> vlan_dev_init, but disabled (we don’t add it to dev->features) , and upon vlan_dev_fix_features
> we check if the real_device have NETIF_F_HW_MACSEC enabled (after the intersect with the real_dev->vlan_features) 
> and if so we add it to the features.
> 
> So something like:
> 
> static int vlan_dev_init(struct net_device *dev)
> {
> ...
> 	dev->features |= dev->hw_features | NETIF_F_LLTX;
> 	dev->hw_features |= NETIF_F_HW_MACSEC;
> ...
> }

That would be adding the NETIF_F_HW_MACSEC to all VLAN devices,
whether the lower device advertises this feature or not. That's wrong.


What I had in mind was:

	if (real_dev->vlan_features & NETIF_F_HW_MACSEC)
		dev->hw_features |= NETIF_F_HW_MACSEC;


And we should enable it by default when the lower device has it
enabled, which would be the case with this:

@@ -572,6 +572,9 @@ static int vlan_dev_init(struct net_device *dev)
 			   NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
 			   NETIF_F_ALL_FCOE;
 
+	if (real_dev->vlan_features & NETIF_F_HW_MACSEC)
+		dev->hw_features |= NETIF_F_HW_MACSEC;
+
 	dev->features |= dev->hw_features | NETIF_F_LLTX;
 	netif_inherit_tso_max(dev, real_dev);
 	if (dev->features & NETIF_F_VLAN_FEATURES)


What I meant by "but disabled" in my previous email was that if the
lower device currently has NETIF_F_HW_MACSEC, the new vlan device
should also have it disabled, not that it should always be disabled on
creation.


> static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
>          netdev_features_t features)
> {
> ...
>  	if (lower_features &  NETIF_F_HW_MACSEC)
>  		features |= NETIF_F_HW_MACSEC;
> 
> return features;
> }

I don't think NETIF_F_HW_MACSEC is "special" enough to require hacks
in vlan_dev_fix_features. IMHO modifying vlan_dev_fix_features should
only happen if we have no other way to implement a consistent and
useful behavior. I don't think that's the case here.

-- 
Sabrina

RE: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Emeel Hakim]

> -----Original Message-----
> From: Sabrina Dubroca <sd@queasysnail.net>
> Sent: Tuesday, 4 April 2023 15:54
> To: Emeel Hakim <ehakim@nvidia.com>
> Cc: Leon Romanovsky <leon@kernel.org>; davem@davemloft.net;
> kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for
> VLAN interface
> 
> External email: Use caution opening links or attachments
> 
> 
> 2023-04-03, 09:29:28 +0000, Emeel Hakim wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sabrina Dubroca <sd@queasysnail.net>
> > > Sent: Thursday, 30 March 2023 23:33
> > > To: Leon Romanovsky <leon@kernel.org>
> > > Cc: Emeel Hakim <ehakim@nvidia.com>; davem@davemloft.net;
> > > kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> > > netdev@vger.kernel.org
> > > Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload
> > > operations for VLAN interface
> > >
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > 2023-03-30, 21:56:56 +0300, Leon Romanovsky wrote:
> > > > On Thu, Mar 30, 2023 at 07:19:21PM +0200, Sabrina Dubroca wrote:
> > > > > 2023-03-29, 21:42:01 +0300, Leon Romanovsky wrote:
> > > > > > On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> > > > > > > 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > > > > > > > Add support for MACsec offload operations for VLAN driver
> > > > > > > > to allow offloading MACsec when VLAN's real device
> > > > > > > > supports Macsec offload by forwarding the offload request to it.
> > > > > > > >
> > > > > > > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > > > > > > > ---
> > > > > > > > V1 -> V2: - Consult vlan_features when adding
> NETIF_F_HW_MACSEC.
> > > > > > >
> > > > > > > Uh? You're not actually doing that? You also dropped the
> > > > > > > changes to vlan_dev_fix_features without explaining why.
> > > > > >
> > > > > > vlan_dev_fix_features() relies on real_dev->vlan_features
> > > > > > which was set in mlx5 part of this patch.
> > > > > >
> > > > > >   643 static netdev_features_t vlan_dev_fix_features(struct net_device
> *dev,
> > > > > >   644         netdev_features_t features)
> > > > > >   645 {
> > > > > >   ...
> > > > > >   649
> > > > > >   650         lower_features = netdev_intersect_features((real_dev-
> > > >vlan_features |
> > > > > >   651                                                     NETIF_F_RXCSUM),
> > > > > >   652                                                    real_dev->features);
> > > > > >
> > > > > > This part ensure that once real_dev->vlan_features and
> > > > > > real_dev->features have NETIF_F_HW_MACSEC, the returned
> > > > > > features will
> > > include NETIF_F_HW_MACSEC too.
> > > > >
> > > > > Ok, thanks.
> > > > >
> > > > > But back to the issue of vlan_features, in vlan_dev_init: I'm
> > > > > not convinced NETIF_F_HW_MACSEC should be added to hw_features
> > > > > based on
> > > > > ->features. That would result in a new vlan device that can't
> > > > > ->offload
> > > > > macsec at all if it was created at the wrong time (while the
> > > > > lower device's macsec offload was temporarily disabled).
> > > >
> > > > Sorry, I'm new to this netdev features zoo, but if I read
> > > > correctly Documentation/networking/netdev-features.rst, the
> > > > ->features is the list of enabled ones:
> > > >
> > > >    29  2. netdev->features set contains features which are currently enabled
> > > >    30     for a device.  This should be changed only by network core or in
> > > >    31     error paths of ndo_set_features callback.
> > > >
> > > > And user will have a chance to disable it for VLAN because it was
> > > > added to ->hw_features:
> > > >
> > > >    24  1. netdev->hw_features set contains features whose state may
> possibly
> > > >    25     be changed (enabled or disabled) for a particular device by user's
> > > >    26     request.  This set should be initialized in ndo_init callback and not
> > > >    27     changed later.
> > > >
> > > > So how can VLAN be created with NETIF_F_HW_MACSEC while real_dev
> > > > mcasec offload is disabled?
> > >
> > > I'm proposing that be VLAN device be created with the capability
> > > (->hw_features contains NETIF_F_HW_MACSEC) but disabled (->features
> > > doesn't contain NETIF_F_HW_MACSEC). That way, if NETIF_F_HW_MACSEC
> > > is re-enabled on the lower device, you don't need to destroy the
> > > VLAN device to enable macsec offload on it as well. You still won't
> > > be able to enable macsec offload on the VLAN device unless it's active on the
> real NIC.
> > >
> > > I think whether the lower device currently has NETIF_F_HW_MACSEC
> > > should only affect whether you can enable the feature on the vlan
> > > device right now. What feature is enabled at creation time should be
> irrelevant.
> >
> > Thanks for the proposal Sabrina, I'm also new to this netdev features
> > zone so IIUC your'e proposing that we have NETIF_F_HW_MACSEC added to
> > the dev->hw_features upon vlan_dev_init, but disabled (we don’t add it
> > to dev->features) , and upon vlan_dev_fix_features we check if the
> > real_device have NETIF_F_HW_MACSEC enabled (after the intersect with the
> real_dev->vlan_features) and if so we add it to the features.
> >
> > So something like:
> >
> > static int vlan_dev_init(struct net_device *dev) { ...
> >       dev->features |= dev->hw_features | NETIF_F_LLTX;
> >       dev->hw_features |= NETIF_F_HW_MACSEC; ...
> > }
> 
> That would be adding the NETIF_F_HW_MACSEC to all VLAN devices, whether
> the lower device advertises this feature or not. That's wrong.
> 
> 
> What I had in mind was:
> 
>         if (real_dev->vlan_features & NETIF_F_HW_MACSEC)
>                 dev->hw_features |= NETIF_F_HW_MACSEC;
> 
> 
> And we should enable it by default when the lower device has it enabled, which
> would be the case with this:
> 
> @@ -572,6 +572,9 @@ static int vlan_dev_init(struct net_device *dev)
>                            NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
>                            NETIF_F_ALL_FCOE;
> 
> +       if (real_dev->vlan_features & NETIF_F_HW_MACSEC)
> +               dev->hw_features |= NETIF_F_HW_MACSEC;
> +
>         dev->features |= dev->hw_features | NETIF_F_LLTX;
>         netif_inherit_tso_max(dev, real_dev);
>         if (dev->features & NETIF_F_VLAN_FEATURES)
> 
> 
> What I meant by "but disabled" in my previous email was that if the lower device
> currently has NETIF_F_HW_MACSEC, the new vlan device should also have it
> disabled, not that it should always be disabled on creation.
> 

Thanks for the explanation its clear for me now, I tested it and its working for me.
I agree with this approach.
I can prepare a new version if we are closed on everything.
Should I send a v3 (since previous v3 got discarded) or I send it as a v4 ?

> > static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
> >          netdev_features_t features)
> > {
> > ...
> >       if (lower_features &  NETIF_F_HW_MACSEC)
> >               features |= NETIF_F_HW_MACSEC;
> >
> > return features;
> > }
> 
> I don't think NETIF_F_HW_MACSEC is "special" enough to require hacks in
> vlan_dev_fix_features. IMHO modifying vlan_dev_fix_features should only
> happen if we have no other way to implement a consistent and useful behavior. I
> don't think that's the case here.

I agree, no need to touch vlan_dev_fix_features with your approach.

> --
> Sabrina

Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Sabrina Dubroca]

2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> Add support for MACsec offload operations for VLAN driver
> to allow offloading MACsec when VLAN's real device supports
> Macsec offload by forwarding the offload request to it.
> 
> Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> ---
> V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> 		  - Allow grep for the functions.
> 		  - Add helper function to get the macsec operation to allow the compiler to make some choice.
>  .../net/ethernet/mellanox/mlx5/core/en_main.c |   1 +
>  net/8021q/vlan_dev.c                          | 101 ++++++++++++++++++
>  2 files changed, 102 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 6db1aff8778d..5ecef26e83c6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -5076,6 +5076,7 @@ static void mlx5e_build_nic_netdev(struct net_device *netdev)
>  
>  	netdev->vlan_features    |= NETIF_F_SG;
>  	netdev->vlan_features    |= NETIF_F_HW_CSUM;
> +	netdev->vlan_features    |= NETIF_F_HW_MACSEC;
>  	netdev->vlan_features    |= NETIF_F_GRO;
>  	netdev->vlan_features    |= NETIF_F_TSO;
>  	netdev->vlan_features    |= NETIF_F_TSO6;

IMO that shouldn't be part of this patch. One patch for VLAN to add
MACsec offload, then one for mlx5 to enable the feature.

-- 
Sabrina

Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Sabrina Dubroca]

2023-04-04, 14:37:58 +0000, Emeel Hakim wrote:
> 
> 
> > -----Original Message-----
> > From: Sabrina Dubroca <sd@queasysnail.net>
> > Sent: Tuesday, 4 April 2023 15:54
> > To: Emeel Hakim <ehakim@nvidia.com>
> > Cc: Leon Romanovsky <leon@kernel.org>; davem@davemloft.net;
> > kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> > netdev@vger.kernel.org
> > Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for
> > VLAN interface
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > 2023-04-03, 09:29:28 +0000, Emeel Hakim wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Sabrina Dubroca <sd@queasysnail.net>
> > > > Sent: Thursday, 30 March 2023 23:33
> > > > To: Leon Romanovsky <leon@kernel.org>
> > > > Cc: Emeel Hakim <ehakim@nvidia.com>; davem@davemloft.net;
> > > > kuba@kernel.org; pabeni@redhat.com; edumazet@google.com;
> > > > netdev@vger.kernel.org
> > > > Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload
> > > > operations for VLAN interface
> > > >
> > > > External email: Use caution opening links or attachments
> > > >
> > > >
> > > > 2023-03-30, 21:56:56 +0300, Leon Romanovsky wrote:
> > > > > On Thu, Mar 30, 2023 at 07:19:21PM +0200, Sabrina Dubroca wrote:
> > > > > > 2023-03-29, 21:42:01 +0300, Leon Romanovsky wrote:
> > > > > > > On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> > > > > > > > 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > > > > > > > > Add support for MACsec offload operations for VLAN driver
> > > > > > > > > to allow offloading MACsec when VLAN's real device
> > > > > > > > > supports Macsec offload by forwarding the offload request to it.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > > > > > > > > ---
> > > > > > > > > V1 -> V2: - Consult vlan_features when adding
> > NETIF_F_HW_MACSEC.
> > > > > > > >
> > > > > > > > Uh? You're not actually doing that? You also dropped the
> > > > > > > > changes to vlan_dev_fix_features without explaining why.
> > > > > > >
> > > > > > > vlan_dev_fix_features() relies on real_dev->vlan_features
> > > > > > > which was set in mlx5 part of this patch.
> > > > > > >
> > > > > > >   643 static netdev_features_t vlan_dev_fix_features(struct net_device
> > *dev,
> > > > > > >   644         netdev_features_t features)
> > > > > > >   645 {
> > > > > > >   ...
> > > > > > >   649
> > > > > > >   650         lower_features = netdev_intersect_features((real_dev-
> > > > >vlan_features |
> > > > > > >   651                                                     NETIF_F_RXCSUM),
> > > > > > >   652                                                    real_dev->features);
> > > > > > >
> > > > > > > This part ensure that once real_dev->vlan_features and
> > > > > > > real_dev->features have NETIF_F_HW_MACSEC, the returned
> > > > > > > features will
> > > > include NETIF_F_HW_MACSEC too.
> > > > > >
> > > > > > Ok, thanks.
> > > > > >
> > > > > > But back to the issue of vlan_features, in vlan_dev_init: I'm
> > > > > > not convinced NETIF_F_HW_MACSEC should be added to hw_features
> > > > > > based on
> > > > > > ->features. That would result in a new vlan device that can't
> > > > > > ->offload
> > > > > > macsec at all if it was created at the wrong time (while the
> > > > > > lower device's macsec offload was temporarily disabled).
> > > > >
> > > > > Sorry, I'm new to this netdev features zoo, but if I read
> > > > > correctly Documentation/networking/netdev-features.rst, the
> > > > > ->features is the list of enabled ones:
> > > > >
> > > > >    29  2. netdev->features set contains features which are currently enabled
> > > > >    30     for a device.  This should be changed only by network core or in
> > > > >    31     error paths of ndo_set_features callback.
> > > > >
> > > > > And user will have a chance to disable it for VLAN because it was
> > > > > added to ->hw_features:
> > > > >
> > > > >    24  1. netdev->hw_features set contains features whose state may
> > possibly
> > > > >    25     be changed (enabled or disabled) for a particular device by user's
> > > > >    26     request.  This set should be initialized in ndo_init callback and not
> > > > >    27     changed later.
> > > > >
> > > > > So how can VLAN be created with NETIF_F_HW_MACSEC while real_dev
> > > > > mcasec offload is disabled?
> > > >
> > > > I'm proposing that be VLAN device be created with the capability
> > > > (->hw_features contains NETIF_F_HW_MACSEC) but disabled (->features
> > > > doesn't contain NETIF_F_HW_MACSEC). That way, if NETIF_F_HW_MACSEC
> > > > is re-enabled on the lower device, you don't need to destroy the
> > > > VLAN device to enable macsec offload on it as well. You still won't
> > > > be able to enable macsec offload on the VLAN device unless it's active on the
> > real NIC.
> > > >
> > > > I think whether the lower device currently has NETIF_F_HW_MACSEC
> > > > should only affect whether you can enable the feature on the vlan
> > > > device right now. What feature is enabled at creation time should be
> > irrelevant.
> > >
> > > Thanks for the proposal Sabrina, I'm also new to this netdev features
> > > zone so IIUC your'e proposing that we have NETIF_F_HW_MACSEC added to
> > > the dev->hw_features upon vlan_dev_init, but disabled (we don’t add it
> > > to dev->features) , and upon vlan_dev_fix_features we check if the
> > > real_device have NETIF_F_HW_MACSEC enabled (after the intersect with the
> > real_dev->vlan_features) and if so we add it to the features.
> > >
> > > So something like:
> > >
> > > static int vlan_dev_init(struct net_device *dev) { ...
> > >       dev->features |= dev->hw_features | NETIF_F_LLTX;
> > >       dev->hw_features |= NETIF_F_HW_MACSEC; ...
> > > }
> > 
> > That would be adding the NETIF_F_HW_MACSEC to all VLAN devices, whether
> > the lower device advertises this feature or not. That's wrong.
> > 
> > 
> > What I had in mind was:
> > 
> >         if (real_dev->vlan_features & NETIF_F_HW_MACSEC)
> >                 dev->hw_features |= NETIF_F_HW_MACSEC;
> > 
> > 
> > And we should enable it by default when the lower device has it enabled, which
> > would be the case with this:
> > 
> > @@ -572,6 +572,9 @@ static int vlan_dev_init(struct net_device *dev)
> >                            NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
> >                            NETIF_F_ALL_FCOE;
> > 
> > +       if (real_dev->vlan_features & NETIF_F_HW_MACSEC)
> > +               dev->hw_features |= NETIF_F_HW_MACSEC;
> > +
> >         dev->features |= dev->hw_features | NETIF_F_LLTX;
> >         netif_inherit_tso_max(dev, real_dev);
> >         if (dev->features & NETIF_F_VLAN_FEATURES)
> > 
> > 
> > What I meant by "but disabled" in my previous email was that if the lower device
> > currently has NETIF_F_HW_MACSEC, the new vlan device should also have it
> > disabled, not that it should always be disabled on creation.
> > 
> 
> Thanks for the explanation its clear for me now, I tested it and its working for me.
> I agree with this approach.
> I can prepare a new version if we are closed on everything.
> Should I send a v3 (since previous v3 got discarded) or I send it as a v4 ?

I'd call that v4 so that nobody gets confused. It's going to be
different from the v3 you already posted. I'm going through the
patches again, I have a couple of other comments.

-- 
Sabrina

Re: [PATCH net-next 4/4] macsec: Add MACsec rx_handler change support

[Sabrina Dubroca]

2023-03-29, 15:21:07 +0300, Emeel Hakim wrote:
> Offloading device drivers will mark offloaded MACsec SKBs with the
> corresponding SCI in the skb_metadata_dst so the macsec rx handler will
> know to which interface to divert those skbs, in case of a marked skb
> and a mismatch on the dst MAC address, divert the skb to the macsec
> net_device where the macsec rx_handler will be called.

Sorry, I don't understand what you're trying to say here and in the
subject line.

To me, "Add MACsec rx_handler change support" sounds like you're
changing what function is used as ->rx_handler, which is not what this
patch is doing.

> Example of such a case is having a MACsec with VLAN as an inner header
> ETHERNET | SECTAG | VLAN packet.
> 
> Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> ---
>  drivers/net/macsec.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 25616247d7a5..88b00ea4af68 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -1048,6 +1048,15 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
>  
>  				__netif_rx(nskb);
>  			}
> +
> +			if (md_dst && md_dst->type == METADATA_MACSEC &&
> +			    (find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci))) {

We already do that exact find_rx_sc call earlier in the same loop,
can't we skip it now?

> +				skb->dev = ndev;
> +				skb->pkt_type = PACKET_HOST;
> +				ret = RX_HANDLER_ANOTHER;
> +				goto out;
> +			}
> +
>  			continue;
>  		}
>  
> -- 
> 2.21.3
> 

-- 
Sabrina

RE: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Emeel Hakim]

> -----Original Message-----
> From: Sabrina Dubroca <sd@queasysnail.net>
> Sent: Wednesday, 5 April 2023 12:27
> To: Emeel Hakim <ehakim@nvidia.com>
> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for
> VLAN interface
> 
> External email: Use caution opening links or attachments
> 
> 
> 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > Add support for MACsec offload operations for VLAN driver to allow
> > offloading MACsec when VLAN's real device supports Macsec offload by
> > forwarding the offload request to it.
> >
> > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > ---
> > V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> >                 - Allow grep for the functions.
> >                 - Add helper function to get the macsec operation to allow the compiler
> to make some choice.
> >  .../net/ethernet/mellanox/mlx5/core/en_main.c |   1 +
> >  net/8021q/vlan_dev.c                          | 101 ++++++++++++++++++
> >  2 files changed, 102 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index 6db1aff8778d..5ecef26e83c6 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -5076,6 +5076,7 @@ static void mlx5e_build_nic_netdev(struct
> > net_device *netdev)
> >
> >       netdev->vlan_features    |= NETIF_F_SG;
> >       netdev->vlan_features    |= NETIF_F_HW_CSUM;
> > +     netdev->vlan_features    |= NETIF_F_HW_MACSEC;
> >       netdev->vlan_features    |= NETIF_F_GRO;
> >       netdev->vlan_features    |= NETIF_F_TSO;
> >       netdev->vlan_features    |= NETIF_F_TSO6;
> 
> IMO that shouldn't be part of this patch. One patch for VLAN to add MACsec
> offload, then one for mlx5 to enable the feature.

I can split the patch into two patches and send all as new version, 
should It be as a new V3 (since last V3 got discarded) or I send as a V4? 

> --
> Sabrina

RE: [PATCH net-next 4/4] macsec: Add MACsec rx_handler change support

[Emeel Hakim]

> -----Original Message-----
> From: Sabrina Dubroca <sd@queasysnail.net>
> Sent: Wednesday, 5 April 2023 12:36
> To: Emeel Hakim <ehakim@nvidia.com>
> Cc: davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> edumazet@google.com; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 4/4] macsec: Add MACsec rx_handler change
> support
> 
> External email: Use caution opening links or attachments
> 
> 
> 2023-03-29, 15:21:07 +0300, Emeel Hakim wrote:
> > Offloading device drivers will mark offloaded MACsec SKBs with the
> > corresponding SCI in the skb_metadata_dst so the macsec rx handler
> > will know to which interface to divert those skbs, in case of a marked
> > skb and a mismatch on the dst MAC address, divert the skb to the
> > macsec net_device where the macsec rx_handler will be called.
> 
> Sorry, I don't understand what you're trying to say here and in the subject line.
> 
> To me, "Add MACsec rx_handler change support" sounds like you're changing
> what function is used as ->rx_handler, which is not what this patch is doing.
> 
> > Example of such a case is having a MACsec with VLAN as an inner header
> > ETHERNET | SECTAG | VLAN packet.
> >
> > Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> > ---
> >  drivers/net/macsec.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
> > 25616247d7a5..88b00ea4af68 100644
> > --- a/drivers/net/macsec.c
> > +++ b/drivers/net/macsec.c
> > @@ -1048,6 +1048,15 @@ static enum rx_handler_result
> > handle_not_macsec(struct sk_buff *skb)
> >
> >                               __netif_rx(nskb);
> >                       }
> > +
> > +                     if (md_dst && md_dst->type == METADATA_MACSEC &&
> > +                         (find_rx_sc(&macsec->secy,
> > + md_dst->u.macsec_info.sci))) {
> 
> We already do that exact find_rx_sc call earlier in the same loop, can't we skip it
> now?

I can save the result into a parameter and use that.
Thanks for pointing that.

> > +                             skb->dev = ndev;
> > +                             skb->pkt_type = PACKET_HOST;
> > +                             ret = RX_HANDLER_ANOTHER;
> > +                             goto out;
> > +                     }
> > +
> >                       continue;
> >               }
> >
> > --
> > 2.21.3
> >
> 
> --
> Sabrina