[dpdk-dev] [PATCH] net/ixgbe: fix macsec setting

[dpdk-dev] [PATCH] net/ixgbe: fix macsec setting

[Sun GuinanX]

macsec setting is not valid when port is stopped.
In order to make it valid, the patch changes the setting
to where port is started.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c  | 160 ++++++++++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.h  |  15 +++
 drivers/net/ixgbe/rte_pmd_ixgbe.c |   9 ++
 3 files changed, 184 insertions(+)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index dbce7a80e..29455f7ee 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -379,6 +379,11 @@ static int ixgbe_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
 static int ixgbe_filter_restore(struct rte_eth_dev *dev);
 static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
 
+static void ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_ctrl *macsec_contrl);
+
+static void ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev);
+
 /*
  * Define VF Stats MACRO for Non "cleared on read" register
  */
@@ -2542,6 +2547,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	uint32_t *link_speeds;
 	struct ixgbe_tm_conf *tm_conf =
 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
+	struct ixgbe_macsec_ctrl *macsec_ctrl =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -2794,6 +2801,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	 */
 	ixgbe_dev_link_update(dev, 0);
 
+	/* setup the macsec ctrl register */
+	ixgbe_dev_macsec_ctrl_register_enable(dev, macsec_ctrl);
+
 	return 0;
 
 error:
@@ -2825,6 +2835,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* disable mecsec register */
+	ixgbe_dev_macsec_ctrl_register_disable(dev);
+
 	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
 
 	/* disable interrupts */
@@ -8816,6 +8829,153 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev *dev)
 	return 0;
 }
 
+/* macsec ctrl setup enable */
+void
+ixgbe_dev_macsec_ctrl_setup_enable(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_ctrl *macsec_ctrl)
+{
+	struct ixgbe_macsec_ctrl *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = macsec_ctrl->encrypt_en;
+	macsec->replayprotect_en = macsec_ctrl->replayprotect_en;
+}
+
+/* macsec ctrl setup disable */
+void
+ixgbe_dev_macsec_ctrl_setup_disable(struct rte_eth_dev *dev)
+{
+	struct ixgbe_macsec_ctrl *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = 0;
+	macsec->replayprotect_en = 0;
+}
+
+/* macsec ctrl register set */
+static void
+ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_ctrl *macsec_contrl)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Enable Ethernet CRC (required by MACsec offload) */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
+
+	/* Enable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
+	ctrl |= 0x3;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
+
+	/* Enable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
+		     IXGBE_LSECTXCTRL_AUTH;
+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
+	if (rp)
+		ctrl |= IXGBE_LSECRXCTRL_RP;
+	else
+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
+/* macsec ctrl register set */
+static void
+ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Disable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	/* Disable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
+
+
 RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 6e9ed2e10..e6cff8b3a 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -365,6 +365,12 @@ struct rte_flow {
 	void *rule;
 };
 
+/* MACsec Control structure */
+struct ixgbe_macsec_ctrl {
+	bool encrypt_en;		/* encrypt enabled */
+	bool replayprotect_en;		/* replayprotect enabled */
+};
+
 /*
  * Statistics counters collected by the MACsec
  */
@@ -471,6 +477,7 @@ struct ixgbe_adapter {
 	struct ixgbe_hw             hw;
 	struct ixgbe_hw_stats       stats;
 	struct ixgbe_macsec_stats   macsec_stats;
+	struct ixgbe_macsec_ctrl	macsec_ctrl;
 	struct ixgbe_hw_fdir_info   fdir;
 	struct ixgbe_interrupt      intr;
 	struct ixgbe_stat_mapping_registers stat_mappings;
@@ -523,6 +530,9 @@ int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
 #define IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->macsec_stats)
 
+#define IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(adapter) \
+	(&((struct ixgbe_adapter *)adapter)->macsec_ctrl)
+
 #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->intr)
 
@@ -741,6 +751,11 @@ int ixgbe_action_rss_same(const struct rte_flow_action_rss *comp,
 int ixgbe_config_rss_filter(struct rte_eth_dev *dev,
 		struct ixgbe_rte_flow_rss_conf *conf, bool add);
 
+void ixgbe_dev_macsec_ctrl_setup_enable(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_ctrl *macsec_ctrl);
+
+void ixgbe_dev_macsec_ctrl_setup_disable(struct rte_eth_dev *dev);
+
 static inline int
 ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
 			      uint16_t ethertype)
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c b/drivers/net/ixgbe/rte_pmd_ixgbe.c
index 9514f2cf5..608736e72 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
@@ -518,6 +518,7 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
 	uint32_t ctrl;
+	struct ixgbe_macsec_ctrl macsec_contrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
@@ -592,6 +593,12 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 	 */
 	ixgbe_enable_sec_tx_path_generic(hw);
 
+	macsec_contrl.encrypt_en = (bool)en;
+	macsec_contrl.replayprotect_en = (bool)rp;
+
+	/* Enable macsec setup  */
+	ixgbe_dev_macsec_ctrl_setup_enable(dev, &macsec_contrl);
+
 	return 0;
 }
 
@@ -656,6 +663,8 @@ rte_pmd_ixgbe_macsec_disable(uint16_t port)
 	 */
 	ixgbe_enable_sec_tx_path_generic(hw);
 
+	ixgbe_dev_macsec_ctrl_setup_disable(dev);
+
 	return 0;
 }
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH] net/ixgbe: fix macsec setting

[Ye Xiaolong]

Hi, Guinan

Overall the fix looks good to me, a few comments inline.

On 10/25, Sun GuinanX wrote:
>macsec setting is not valid when port is stopped.
>In order to make it valid, the patch changes the setting
>to where port is started.
>
>Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
>Cc: stable@dpdk.org
>
>Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
>---
> drivers/net/ixgbe/ixgbe_ethdev.c  | 160 ++++++++++++++++++++++++++++++
> drivers/net/ixgbe/ixgbe_ethdev.h  |  15 +++
> drivers/net/ixgbe/rte_pmd_ixgbe.c |   9 ++
> 3 files changed, 184 insertions(+)
>
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>index dbce7a80e..29455f7ee 100644
>--- a/drivers/net/ixgbe/ixgbe_ethdev.c
>+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>@@ -379,6 +379,11 @@ static int ixgbe_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
> static int ixgbe_filter_restore(struct rte_eth_dev *dev);
> static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
> 
>+static void ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
>+		struct ixgbe_macsec_ctrl *macsec_contrl);
>+
>+static void ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev);
>+
> /*
>  * Define VF Stats MACRO for Non "cleared on read" register
>  */
>@@ -2542,6 +2547,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> 	uint32_t *link_speeds;
> 	struct ixgbe_tm_conf *tm_conf =
> 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
>+	struct ixgbe_macsec_ctrl *macsec_ctrl =
>+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
> 
> 	PMD_INIT_FUNC_TRACE();
> 
>@@ -2794,6 +2801,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> 	 */
> 	ixgbe_dev_link_update(dev, 0);
> 
>+	/* setup the macsec ctrl register */
>+	ixgbe_dev_macsec_ctrl_register_enable(dev, macsec_ctrl);
>+
> 	return 0;
> 
> error:
>@@ -2825,6 +2835,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
> 
> 	PMD_INIT_FUNC_TRACE();
> 
>+	/* disable mecsec register */
>+	ixgbe_dev_macsec_ctrl_register_disable(dev);
>+
> 	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
> 
> 	/* disable interrupts */
>@@ -8816,6 +8829,153 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev *dev)
> 	return 0;
> }
> 
>+/* macsec ctrl setup enable */
>+void
>+ixgbe_dev_macsec_ctrl_setup_enable(struct rte_eth_dev *dev,
>+				struct ixgbe_macsec_ctrl *macsec_ctrl)

what about ixgbe_dev_macsec_setting_save?

>+{
>+	struct ixgbe_macsec_ctrl *macsec =
>+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
>+
>+	macsec->encrypt_en = macsec_ctrl->encrypt_en;
>+	macsec->replayprotect_en = macsec_ctrl->replayprotect_en;
>+}
>+
>+/* macsec ctrl setup disable */
>+void
>+ixgbe_dev_macsec_ctrl_setup_disable(struct rte_eth_dev *dev)

what about ixgbe_dev_macsec_setting_reset?

>+{
>+	struct ixgbe_macsec_ctrl *macsec =
>+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
>+
>+	macsec->encrypt_en = 0;
>+	macsec->replayprotect_en = 0;
>+}
>+
>+/* macsec ctrl register set */
>+static void
>+ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
>+				struct ixgbe_macsec_ctrl *macsec_contrl)
>+{
>+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>+	uint32_t ctrl;
>+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
>+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
>+
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_disable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 * The hardware support has been checked by
>+	 * ixgbe_disable_sec_rx_path().
>+	 */
>+	ixgbe_disable_sec_tx_path_generic(hw);
>+
>+	/* Enable Ethernet CRC (required by MACsec offload) */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
>+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
>+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
>+
>+	/* Enable the TX and RX crypto engines */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
>+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
>+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
>+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
>+	ctrl |= 0x3;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
>+
>+	/* Enable SA lookup */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
>+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
>+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
>+		     IXGBE_LSECTXCTRL_AUTH;
>+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
>+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
>+	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
>+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
>+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
>+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
>+	if (rp)
>+		ctrl |= IXGBE_LSECRXCTRL_RP;
>+	else
>+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
>+
>+	/* Start the data paths */
>+	ixgbe_enable_sec_rx_path(hw);
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_enable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 */
>+	ixgbe_enable_sec_tx_path_generic(hw);
>+}
>+
>+/* macsec ctrl register set */
>+static void
>+ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev)
>+{
>+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>+	uint32_t ctrl;
>+
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_disable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 * The hardware support has been checked by
>+	 * ixgbe_disable_sec_rx_path().
>+	 */
>+	ixgbe_disable_sec_tx_path_generic(hw);
>+
>+	/* Disable the TX and RX crypto engines */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
>+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
>+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
>+
>+	/* Disable SA lookup */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
>+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
>+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
>+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
>+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
>+
>+	/* Start the data paths */
>+	ixgbe_enable_sec_rx_path(hw);
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_enable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 */
>+	ixgbe_enable_sec_tx_path_generic(hw);
>+}

Above functions share a lot of code with rte_pmd_ixgbe_macsec_enable/disable,
try to refactor to reduce the duplication.

>+
>+
>+
> RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
> RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
> RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
>index 6e9ed2e10..e6cff8b3a 100644
>--- a/drivers/net/ixgbe/ixgbe_ethdev.h
>+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
>@@ -365,6 +365,12 @@ struct rte_flow {
> 	void *rule;
> };
> 
>+/* MACsec Control structure */
>+struct ixgbe_macsec_ctrl {

what about ixgbe_macsec_setting, it seems more like setting of macsec other than
the control structure.

>+	bool encrypt_en;		/* encrypt enabled */
>+	bool replayprotect_en;		/* replayprotect enabled */

what about using uint8_t to avoid further cast?


Thanks,
Xiaolong

[dpdk-dev] [PATCH v2] net/ixgbe: fix macsec setting

[Sun GuinanX]

macsec setting is not valid when port is stopped.
In order to make it valid, the patch changes the setting
to where port is started.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
---
v2:
* Modified function name
* Refactored the rte_pmd_ixgbe_macsec_enable/disable function
* Modified structure name
* Modified the data type of a structure variable
---
 drivers/net/ixgbe/ixgbe_ethdev.c  | 160 ++++++++++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.h  |  15 +++
 drivers/net/ixgbe/rte_pmd_ixgbe.c | 127 ++----------------------
 3 files changed, 182 insertions(+), 120 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index dbce7a80e..7ae7bc9ca 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -379,6 +379,11 @@ static int ixgbe_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
 static int ixgbe_filter_restore(struct rte_eth_dev *dev);
 static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
 
+static void ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_contrl);
+
+static void ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev);
+
 /*
  * Define VF Stats MACRO for Non "cleared on read" register
  */
@@ -2542,6 +2547,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	uint32_t *link_speeds;
 	struct ixgbe_tm_conf *tm_conf =
 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
+	struct ixgbe_macsec_setting *macsec_ctrl =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -2794,6 +2801,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	 */
 	ixgbe_dev_link_update(dev, 0);
 
+	/* setup the macsec ctrl register */
+	ixgbe_dev_macsec_ctrl_register_enable(dev, macsec_ctrl);
+
 	return 0;
 
 error:
@@ -2825,6 +2835,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* disable mecsec register */
+	ixgbe_dev_macsec_ctrl_register_disable(dev);
+
 	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
 
 	/* disable interrupts */
@@ -8816,6 +8829,153 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev *dev)
 	return 0;
 }
 
+/* macsec ctrl setup enable */
+void
+ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_ctrl)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = macsec_ctrl->encrypt_en;
+	macsec->replayprotect_en = macsec_ctrl->replayprotect_en;
+}
+
+/* macsec ctrl setup disable */
+void
+ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = 0;
+	macsec->replayprotect_en = 0;
+}
+
+/* macsec ctrl register set */
+static void
+ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_contrl)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Enable Ethernet CRC (required by MACsec offload) */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
+
+	/* Enable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
+	ctrl |= 0x3;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
+
+	/* Enable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
+		     IXGBE_LSECTXCTRL_AUTH;
+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
+	if (rp)
+		ctrl |= IXGBE_LSECRXCTRL_RP;
+	else
+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
+/* macsec ctrl register set */
+static void
+ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Disable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	/* Disable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
+
+
 RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 6e9ed2e10..6aa2cdbbc 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -365,6 +365,12 @@ struct rte_flow {
 	void *rule;
 };
 
+/* MACsec Control structure */
+struct ixgbe_macsec_setting {
+	uint8_t encrypt_en;		/* encrypt enabled */
+	uint8_t replayprotect_en;	/* replayprotect enabled */
+};
+
 /*
  * Statistics counters collected by the MACsec
  */
@@ -471,6 +477,7 @@ struct ixgbe_adapter {
 	struct ixgbe_hw             hw;
 	struct ixgbe_hw_stats       stats;
 	struct ixgbe_macsec_stats   macsec_stats;
+	struct ixgbe_macsec_setting	macsec_ctrl;
 	struct ixgbe_hw_fdir_info   fdir;
 	struct ixgbe_interrupt      intr;
 	struct ixgbe_stat_mapping_registers stat_mappings;
@@ -523,6 +530,9 @@ int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
 #define IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->macsec_stats)
 
+#define IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(adapter) \
+	(&((struct ixgbe_adapter *)adapter)->macsec_ctrl)
+
 #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->intr)
 
@@ -741,6 +751,11 @@ int ixgbe_action_rss_same(const struct rte_flow_action_rss *comp,
 int ixgbe_config_rss_filter(struct rte_eth_dev *dev,
 		struct ixgbe_rte_flow_rss_conf *conf, bool add);
 
+void ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_ctrl);
+
+void ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev);
+
 static inline int
 ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
 			      uint16_t ethertype)
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c b/drivers/net/ixgbe/rte_pmd_ixgbe.c
index 9514f2cf5..3a1474876 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
@@ -515,82 +515,18 @@ rte_pmd_ixgbe_set_vf_rate_limit(uint16_t port, uint16_t vf,
 int
 rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
+	struct ixgbe_macsec_setting macsec_setting;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
-
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	macsec_setting.encrypt_en = en;
+	macsec_setting.replayprotect_en = rp;
 
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Enable Ethernet CRC (required by MACsec offload) */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
-	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
-	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
-
-	/* Enable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
-	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
-	ctrl |= 0x3;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
-
-	/* Enable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
-		     IXGBE_LSECTXCTRL_AUTH;
-	ctrl |= IXGBE_LSECTXCTRL_AISCI;
-	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
-	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
-	if (rp)
-		ctrl |= IXGBE_LSECRXCTRL_RP;
-	else
-		ctrl &= ~IXGBE_LSECRXCTRL_RP;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	/* Enable macsec setup  */
+	ixgbe_dev_macsec_setting_save(dev, &macsec_setting);
 
 	return 0;
 }
@@ -598,63 +534,14 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 int
 rte_pmd_ixgbe_macsec_disable(uint16_t port)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
-
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Disable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	/* Disable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	/* Disable macsec setup  */
+	ixgbe_dev_macsec_setting_reset(dev);
 
 	return 0;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix macsec setting

[Ye Xiaolong]

On 10/29, Sun GuinanX wrote:
>macsec setting is not valid when port is stopped.
>In order to make it valid, the patch changes the setting
>to where port is started.
>
>Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
>Cc: stable@dpdk.org
>
>Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
>---
>v2:
>* Modified function name
>* Refactored the rte_pmd_ixgbe_macsec_enable/disable function
>* Modified structure name
>* Modified the data type of a structure variable
>---
> drivers/net/ixgbe/ixgbe_ethdev.c  | 160 ++++++++++++++++++++++++++++++
> drivers/net/ixgbe/ixgbe_ethdev.h  |  15 +++
> drivers/net/ixgbe/rte_pmd_ixgbe.c | 127 ++----------------------
> 3 files changed, 182 insertions(+), 120 deletions(-)
>
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>index dbce7a80e..7ae7bc9ca 100644
>--- a/drivers/net/ixgbe/ixgbe_ethdev.c
>+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>@@ -379,6 +379,11 @@ static int ixgbe_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
> static int ixgbe_filter_restore(struct rte_eth_dev *dev);
> static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
> 
>+static void ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
>+		struct ixgbe_macsec_setting *macsec_contrl);
>+
>+static void ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev);
>+
> /*
>  * Define VF Stats MACRO for Non "cleared on read" register
>  */
>@@ -2542,6 +2547,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> 	uint32_t *link_speeds;
> 	struct ixgbe_tm_conf *tm_conf =
> 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
>+	struct ixgbe_macsec_setting *macsec_ctrl =
>+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
> 
> 	PMD_INIT_FUNC_TRACE();
> 
>@@ -2794,6 +2801,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> 	 */
> 	ixgbe_dev_link_update(dev, 0);
> 
>+	/* setup the macsec ctrl register */
>+	ixgbe_dev_macsec_ctrl_register_enable(dev, macsec_ctrl);
>+
> 	return 0;
> 
> error:
>@@ -2825,6 +2835,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
> 
> 	PMD_INIT_FUNC_TRACE();
> 
>+	/* disable mecsec register */
>+	ixgbe_dev_macsec_ctrl_register_disable(dev);
>+
> 	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
> 
> 	/* disable interrupts */
>@@ -8816,6 +8829,153 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev *dev)
> 	return 0;
> }
> 
>+/* macsec ctrl setup enable */

This comment is not aligned with the function name, actually since this function
is quite straightforward, the comment is unnecessary.

>+void
>+ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
>+				struct ixgbe_macsec_setting *macsec_ctrl)
>+{
>+	struct ixgbe_macsec_setting *macsec =
>+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
>+
>+	macsec->encrypt_en = macsec_ctrl->encrypt_en;
>+	macsec->replayprotect_en = macsec_ctrl->replayprotect_en;
>+}
>+
>+/* macsec ctrl setup disable */

ditto

>+void
>+ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev)
>+{
>+	struct ixgbe_macsec_setting *macsec =
>+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
>+
>+	macsec->encrypt_en = 0;
>+	macsec->replayprotect_en = 0;
>+}
>+
>+/* macsec ctrl register set */
>+static void
>+ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
>+				struct ixgbe_macsec_setting *macsec_contrl)
>+{
>+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>+	uint32_t ctrl;
>+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
>+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
>+
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_disable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 * The hardware support has been checked by
>+	 * ixgbe_disable_sec_rx_path().
>+	 */
>+	ixgbe_disable_sec_tx_path_generic(hw);
>+
>+	/* Enable Ethernet CRC (required by MACsec offload) */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
>+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
>+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
>+
>+	/* Enable the TX and RX crypto engines */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
>+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
>+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
>+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
>+	ctrl |= 0x3;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
>+
>+	/* Enable SA lookup */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
>+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
>+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
>+		     IXGBE_LSECTXCTRL_AUTH;
>+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
>+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
>+	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
>+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
>+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
>+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
>+	if (rp)
>+		ctrl |= IXGBE_LSECRXCTRL_RP;
>+	else
>+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
>+
>+	/* Start the data paths */
>+	ixgbe_enable_sec_rx_path(hw);
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_enable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 */
>+	ixgbe_enable_sec_tx_path_generic(hw);
>+}
>+
>+/* macsec ctrl register set */
>+static void
>+ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev)
>+{
>+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>+	uint32_t ctrl;
>+
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_disable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 * The hardware support has been checked by
>+	 * ixgbe_disable_sec_rx_path().
>+	 */
>+	ixgbe_disable_sec_tx_path_generic(hw);
>+
>+	/* Disable the TX and RX crypto engines */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
>+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
>+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
>+
>+	/* Disable SA lookup */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
>+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
>+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
>+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
>+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
>+
>+	/* Start the data paths */
>+	ixgbe_enable_sec_rx_path(hw);
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_enable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 */
>+	ixgbe_enable_sec_tx_path_generic(hw);
>+}
>+
>+
>+

One empty line is enough here.

> RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
> RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
> RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
>diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
>index 6e9ed2e10..6aa2cdbbc 100644
>--- a/drivers/net/ixgbe/ixgbe_ethdev.h
>+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
>@@ -365,6 +365,12 @@ struct rte_flow {
> 	void *rule;
> };
> 
>+/* MACsec Control structure */

Comment is unaligned and unnecessary.

>+struct ixgbe_macsec_setting {
>+	uint8_t encrypt_en;		/* encrypt enabled */
>+	uint8_t replayprotect_en;	/* replayprotect enabled */
>+};
>+
> /*
>  * Statistics counters collected by the MACsec
>  */
>@@ -471,6 +477,7 @@ struct ixgbe_adapter {
> 	struct ixgbe_hw             hw;
> 	struct ixgbe_hw_stats       stats;
> 	struct ixgbe_macsec_stats   macsec_stats;
>+	struct ixgbe_macsec_setting	macsec_ctrl;
> 	struct ixgbe_hw_fdir_info   fdir;
> 	struct ixgbe_interrupt      intr;
> 	struct ixgbe_stat_mapping_registers stat_mappings;
>@@ -523,6 +530,9 @@ int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
> #define IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
> 	(&((struct ixgbe_adapter *)adapter)->macsec_stats)
> 
>+#define IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(adapter) \
>+	(&((struct ixgbe_adapter *)adapter)->macsec_ctrl)
>+
> #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
> 	(&((struct ixgbe_adapter *)adapter)->intr)
> 
>@@ -741,6 +751,11 @@ int ixgbe_action_rss_same(const struct rte_flow_action_rss *comp,
> int ixgbe_config_rss_filter(struct rte_eth_dev *dev,
> 		struct ixgbe_rte_flow_rss_conf *conf, bool add);
> 
>+void ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
>+		struct ixgbe_macsec_setting *macsec_ctrl);
>+
>+void ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev);
>+
> static inline int
> ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
> 			      uint16_t ethertype)
>diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c b/drivers/net/ixgbe/rte_pmd_ixgbe.c
>index 9514f2cf5..3a1474876 100644
>--- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
>+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
>@@ -515,82 +515,18 @@ rte_pmd_ixgbe_set_vf_rate_limit(uint16_t port, uint16_t vf,
> int
> rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
> {

I think there is usercase when dev is started, and user calls
rte_pmd_ixgbe_macsec_enable and he expects it will take effect, so we still
need to configure register (call ixgbe_dev_macsec_ctrl_register_enable) here
other than just caching the macsec setting.

>-	struct ixgbe_hw *hw;
> 	struct rte_eth_dev *dev;
>-	uint32_t ctrl;
>+	struct ixgbe_macsec_setting macsec_setting;
> 
> 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> 
> 	dev = &rte_eth_devices[port];
> 
>-	if (!is_ixgbe_supported(dev))
>-		return -ENOTSUP;
>-
>-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>+	macsec_setting.encrypt_en = en;
>+	macsec_setting.replayprotect_en = rp;
> 
>-	/* Stop the data paths */
>-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
>-		return -ENOTSUP;
>-	/**
>-	 * Workaround:
>-	 * As no ixgbe_disable_sec_rx_path equivalent is
>-	 * implemented for tx in the base code, and we are
>-	 * not allowed to modify the base code in DPDK, so
>-	 * just call the hand-written one directly for now.
>-	 * The hardware support has been checked by
>-	 * ixgbe_disable_sec_rx_path().
>-	 */
>-	ixgbe_disable_sec_tx_path_generic(hw);
>-
>-	/* Enable Ethernet CRC (required by MACsec offload) */
>-	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
>-	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
>-	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
>-
>-	/* Enable the TX and RX crypto engines */
>-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
>-	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
>-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
>-
>-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
>-	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
>-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
>-
>-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
>-	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
>-	ctrl |= 0x3;
>-	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
>-
>-	/* Enable SA lookup */
>-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
>-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
>-	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
>-		     IXGBE_LSECTXCTRL_AUTH;
>-	ctrl |= IXGBE_LSECTXCTRL_AISCI;
>-	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
>-	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
>-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
>-
>-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
>-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
>-	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
>-	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
>-	if (rp)
>-		ctrl |= IXGBE_LSECRXCTRL_RP;
>-	else
>-		ctrl &= ~IXGBE_LSECRXCTRL_RP;
>-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
>-
>-	/* Start the data paths */
>-	ixgbe_enable_sec_rx_path(hw);
>-	/**
>-	 * Workaround:
>-	 * As no ixgbe_enable_sec_rx_path equivalent is
>-	 * implemented for tx in the base code, and we are
>-	 * not allowed to modify the base code in DPDK, so
>-	 * just call the hand-written one directly for now.
>-	 */
>-	ixgbe_enable_sec_tx_path_generic(hw);
>+	/* Enable macsec setup  */
>+	ixgbe_dev_macsec_setting_save(dev, &macsec_setting);
> 
> 	return 0;
> }
>@@ -598,63 +534,14 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
> int
> rte_pmd_ixgbe_macsec_disable(uint16_t port)
> {

Ditto.

Thanks,
Xiaolong

Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix macsec setting

[Sun, GuinanX]

Hi, Xiaolong

On 10/29, Sun GuinanX wrote:
> >macsec setting is not valid when port is stopped.
> >In order to make it valid, the patch changes the setting to where port
> >is started.
> >
> >Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
> >---
> >v2:
> >* Modified function name
> >* Refactored the rte_pmd_ixgbe_macsec_enable/disable function
> >* Modified structure name
> >* Modified the data type of a structure variable
> >---
> > drivers/net/ixgbe/ixgbe_ethdev.c  | 160 ++++++++++++++++++++++++++++++
> >drivers/net/ixgbe/ixgbe_ethdev.h  |  15 +++
> >drivers/net/ixgbe/rte_pmd_ixgbe.c | 127 ++----------------------
> > 3 files changed, 182 insertions(+), 120 deletions(-)
> >
> >diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> >b/drivers/net/ixgbe/ixgbe_ethdev.c
> >index dbce7a80e..7ae7bc9ca 100644
> >--- a/drivers/net/ixgbe/ixgbe_ethdev.c
> >+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> >@@ -379,6 +379,11 @@ static int ixgbe_dev_udp_tunnel_port_del(struct
> >rte_eth_dev *dev,  static int ixgbe_filter_restore(struct rte_eth_dev
> >*dev);  static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
> >
> >+static void ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
> >+		struct ixgbe_macsec_setting *macsec_contrl);
> >+
> >+static void ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev
> >+*dev);
> >+
> > /*
> >  * Define VF Stats MACRO for Non "cleared on read" register
> >  */
> >@@ -2542,6 +2547,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> > 	uint32_t *link_speeds;
> > 	struct ixgbe_tm_conf *tm_conf =
> > 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
> >+	struct ixgbe_macsec_setting *macsec_ctrl =
> >+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data-
> >dev_private);
> >
> > 	PMD_INIT_FUNC_TRACE();
> >
> >@@ -2794,6 +2801,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> > 	 */
> > 	ixgbe_dev_link_update(dev, 0);
> >
> >+	/* setup the macsec ctrl register */
> >+	ixgbe_dev_macsec_ctrl_register_enable(dev, macsec_ctrl);
> >+
> > 	return 0;
> >
> > error:
> >@@ -2825,6 +2835,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
> >
> > 	PMD_INIT_FUNC_TRACE();
> >
> >+	/* disable mecsec register */
> >+	ixgbe_dev_macsec_ctrl_register_disable(dev);
> >+
> > 	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
> >
> > 	/* disable interrupts */
> >@@ -8816,6 +8829,153 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev
> *dev)
> > 	return 0;
> > }
> >
> >+/* macsec ctrl setup enable */
> 
> This comment is not aligned with the function name, actually since this function
> is quite straightforward, the comment is unnecessary.
Similar useless comments had been removed
> 
> >+void
> >+ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
> >+				struct ixgbe_macsec_setting *macsec_ctrl) {
> >+	struct ixgbe_macsec_setting *macsec =
> >+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data-
> >dev_private);
> >+
> >+	macsec->encrypt_en = macsec_ctrl->encrypt_en;
> >+	macsec->replayprotect_en = macsec_ctrl->replayprotect_en; }
> >+
> >+/* macsec ctrl setup disable */
> 
> ditto
Similar useless comments had been removed
> 
> >+void
> >+ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev) {
> >+	struct ixgbe_macsec_setting *macsec =
> >+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data-
> >dev_private);
> >+
> >+	macsec->encrypt_en = 0;
> >+	macsec->replayprotect_en = 0;
> >+}
> >+
> >+/* macsec ctrl register set */
> >+static void
> >+ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
> >+				struct ixgbe_macsec_setting *macsec_contrl) {
> >+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> >+	uint32_t ctrl;
> >+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
> >+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
> >+
> >+	/**
> >+	 * Workaround:
> >+	 * As no ixgbe_disable_sec_rx_path equivalent is
> >+	 * implemented for tx in the base code, and we are
> >+	 * not allowed to modify the base code in DPDK, so
> >+	 * just call the hand-written one directly for now.
> >+	 * The hardware support has been checked by
> >+	 * ixgbe_disable_sec_rx_path().
> >+	 */
> >+	ixgbe_disable_sec_tx_path_generic(hw);
> >+
> >+	/* Enable Ethernet CRC (required by MACsec offload) */
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
> >+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
> >+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
> >+
> >+	/* Enable the TX and RX crypto engines */
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
> >+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
> >+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
> >+
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
> >+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
> >+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
> >+
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
> >+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
> >+	ctrl |= 0x3;
> >+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
> >+
> >+	/* Enable SA lookup */
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
> >+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
> >+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
> >+		     IXGBE_LSECTXCTRL_AUTH;
> >+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
> >+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
> >+	ctrl |= IXGBE_MACSEC_PNTHRSH &
> IXGBE_LSECTXCTRL_PNTHRSH_MASK;
> >+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
> >+
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
> >+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
> >+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
> >+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
> >+	if (rp)
> >+		ctrl |= IXGBE_LSECRXCTRL_RP;
> >+	else
> >+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
> >+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
> >+
> >+	/* Start the data paths */
> >+	ixgbe_enable_sec_rx_path(hw);
> >+	/**
> >+	 * Workaround:
> >+	 * As no ixgbe_enable_sec_rx_path equivalent is
> >+	 * implemented for tx in the base code, and we are
> >+	 * not allowed to modify the base code in DPDK, so
> >+	 * just call the hand-written one directly for now.
> >+	 */
> >+	ixgbe_enable_sec_tx_path_generic(hw);
> >+}
> >+
> >+/* macsec ctrl register set */
> >+static void
> >+ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev) {
> >+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> >+	uint32_t ctrl;
> >+
> >+	/**
> >+	 * Workaround:
> >+	 * As no ixgbe_disable_sec_rx_path equivalent is
> >+	 * implemented for tx in the base code, and we are
> >+	 * not allowed to modify the base code in DPDK, so
> >+	 * just call the hand-written one directly for now.
> >+	 * The hardware support has been checked by
> >+	 * ixgbe_disable_sec_rx_path().
> >+	 */
> >+	ixgbe_disable_sec_tx_path_generic(hw);
> >+
> >+	/* Disable the TX and RX crypto engines */
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
> >+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
> >+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
> >+
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
> >+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
> >+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
> >+
> >+	/* Disable SA lookup */
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
> >+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
> >+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
> >+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
> >+
> >+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
> >+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
> >+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
> >+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
> >+
> >+	/* Start the data paths */
> >+	ixgbe_enable_sec_rx_path(hw);
> >+	/**
> >+	 * Workaround:
> >+	 * As no ixgbe_enable_sec_rx_path equivalent is
> >+	 * implemented for tx in the base code, and we are
> >+	 * not allowed to modify the base code in DPDK, so
> >+	 * just call the hand-written one directly for now.
> >+	 */
> >+	ixgbe_enable_sec_tx_path_generic(hw);
> >+}
> >+
> >+
> >+
> 
> One empty line is enough here.
had been removed
> 
> > RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
> >RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
> >RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic |
> >vfio-pci"); diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h
> >b/drivers/net/ixgbe/ixgbe_ethdev.h
> >index 6e9ed2e10..6aa2cdbbc 100644
> >--- a/drivers/net/ixgbe/ixgbe_ethdev.h
> >+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> >@@ -365,6 +365,12 @@ struct rte_flow {
> > 	void *rule;
> > };
> >
> >+/* MACsec Control structure */
> 
> Comment is unaligned and unnecessary.
had been removed
> 
> >+struct ixgbe_macsec_setting {
> >+	uint8_t encrypt_en;		/* encrypt enabled */
> >+	uint8_t replayprotect_en;	/* replayprotect enabled */
> >+};
> >+
> > /*
> >  * Statistics counters collected by the MACsec
> >  */
> >@@ -471,6 +477,7 @@ struct ixgbe_adapter {
> > 	struct ixgbe_hw             hw;
> > 	struct ixgbe_hw_stats       stats;
> > 	struct ixgbe_macsec_stats   macsec_stats;
> >+	struct ixgbe_macsec_setting	macsec_ctrl;
> > 	struct ixgbe_hw_fdir_info   fdir;
> > 	struct ixgbe_interrupt      intr;
> > 	struct ixgbe_stat_mapping_registers stat_mappings; @@ -523,6 +530,9
> >@@ int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
> >#define IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
> > 	(&((struct ixgbe_adapter *)adapter)->macsec_stats)
> >
> >+#define IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(adapter) \
> >+	(&((struct ixgbe_adapter *)adapter)->macsec_ctrl)
> >+
> > #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
> > 	(&((struct ixgbe_adapter *)adapter)->intr)
> >
> >@@ -741,6 +751,11 @@ int ixgbe_action_rss_same(const struct
> >rte_flow_action_rss *comp,  int ixgbe_config_rss_filter(struct rte_eth_dev
> *dev,
> > 		struct ixgbe_rte_flow_rss_conf *conf, bool add);
> >
> >+void ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
> >+		struct ixgbe_macsec_setting *macsec_ctrl);
> >+
> >+void ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev);
> >+
> > static inline int
> > ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
> > 			      uint16_t ethertype)
> >diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c
> >b/drivers/net/ixgbe/rte_pmd_ixgbe.c
> >index 9514f2cf5..3a1474876 100644
> >--- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
> >+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
> >@@ -515,82 +515,18 @@ rte_pmd_ixgbe_set_vf_rate_limit(uint16_t port,
> >uint16_t vf,  int  rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t
> >en, uint8_t rp)  {
> 
> I think there is usercase when dev is started, and user calls
> rte_pmd_ixgbe_macsec_enable and he expects it will take effect, so we still
> need to configure register (call ixgbe_dev_macsec_ctrl_register_enable) here
> other than just caching the macsec setting.
>
This kind of usercase processing will be added in the patch of V3.
 
> >-	struct ixgbe_hw *hw;
> > 	struct rte_eth_dev *dev;
> >-	uint32_t ctrl;
> >+	struct ixgbe_macsec_setting macsec_setting;
> >
> > 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> >
> > 	dev = &rte_eth_devices[port];
> >
> >-	if (!is_ixgbe_supported(dev))
> >-		return -ENOTSUP;
> >-
> >-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >+	macsec_setting.encrypt_en = en;
> >+	macsec_setting.replayprotect_en = rp;
> >
> >-	/* Stop the data paths */
> >-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
> >-		return -ENOTSUP;
> >-	/**
> >-	 * Workaround:
> >-	 * As no ixgbe_disable_sec_rx_path equivalent is
> >-	 * implemented for tx in the base code, and we are
> >-	 * not allowed to modify the base code in DPDK, so
> >-	 * just call the hand-written one directly for now.
> >-	 * The hardware support has been checked by
> >-	 * ixgbe_disable_sec_rx_path().
> >-	 */
> >-	ixgbe_disable_sec_tx_path_generic(hw);
> >-
> >-	/* Enable Ethernet CRC (required by MACsec offload) */
> >-	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
> >-	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
> >-	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
> >-
> >-	/* Enable the TX and RX crypto engines */
> >-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
> >-	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
> >-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
> >-
> >-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
> >-	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
> >-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
> >-
> >-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
> >-	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
> >-	ctrl |= 0x3;
> >-	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
> >-
> >-	/* Enable SA lookup */
> >-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
> >-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
> >-	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
> >-		     IXGBE_LSECTXCTRL_AUTH;
> >-	ctrl |= IXGBE_LSECTXCTRL_AISCI;
> >-	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
> >-	ctrl |= IXGBE_MACSEC_PNTHRSH &
> IXGBE_LSECTXCTRL_PNTHRSH_MASK;
> >-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
> >-
> >-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
> >-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
> >-	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
> >-	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
> >-	if (rp)
> >-		ctrl |= IXGBE_LSECRXCTRL_RP;
> >-	else
> >-		ctrl &= ~IXGBE_LSECRXCTRL_RP;
> >-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
> >-
> >-	/* Start the data paths */
> >-	ixgbe_enable_sec_rx_path(hw);
> >-	/**
> >-	 * Workaround:
> >-	 * As no ixgbe_enable_sec_rx_path equivalent is
> >-	 * implemented for tx in the base code, and we are
> >-	 * not allowed to modify the base code in DPDK, so
> >-	 * just call the hand-written one directly for now.
> >-	 */
> >-	ixgbe_enable_sec_tx_path_generic(hw);
> >+	/* Enable macsec setup  */
> >+	ixgbe_dev_macsec_setting_save(dev, &macsec_setting);
> >
> > 	return 0;
> > }
> >@@ -598,63 +534,14 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port,
> >uint8_t en, uint8_t rp)  int  rte_pmd_ixgbe_macsec_disable(uint16_t
> >port)  {
> 
> Ditto.
>
This kind of usercase processing will be added in the patch of V3.
 
> Thanks,
> Xiaolong

[dpdk-dev] [PATCH v3] net/ixgbe: fix macsec setting

[Sun GuinanX]

macsec setting is not valid when port is stopped.
In order to make it valid, the patch changes the setting
to where port is started.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
---
v3:
* Deleted extra comments
* Modified the function name of the setting register
* Increased the logic used to set the register in the port start state
v2:
* Modified function name
* Refactored the rte_pmd_ixgbe_macsec_enable/disable function
* Modified structure name
* Modified the data type of a structure variable
---
 drivers/net/ixgbe/ixgbe_ethdev.c  | 160 ++++++++++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.h  |  15 +++
 drivers/net/ixgbe/rte_pmd_ixgbe.c | 127 ++----------------------
 3 files changed, 182 insertions(+), 120 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index dbce7a80e..7ae7bc9ca 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -379,6 +379,11 @@ static int ixgbe_dev_udp_tunnel_port_del(struct rte_eth_dev *dev,
 static int ixgbe_filter_restore(struct rte_eth_dev *dev);
 static void ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev);
 
+static void ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_contrl);
+
+static void ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev);
+
 /*
  * Define VF Stats MACRO for Non "cleared on read" register
  */
@@ -2542,6 +2547,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	uint32_t *link_speeds;
 	struct ixgbe_tm_conf *tm_conf =
 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
+	struct ixgbe_macsec_setting *macsec_ctrl =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -2794,6 +2801,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	 */
 	ixgbe_dev_link_update(dev, 0);
 
+	/* setup the macsec ctrl register */
+	ixgbe_dev_macsec_ctrl_register_enable(dev, macsec_ctrl);
+
 	return 0;
 
 error:
@@ -2825,6 +2835,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* disable mecsec register */
+	ixgbe_dev_macsec_ctrl_register_disable(dev);
+
 	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
 
 	/* disable interrupts */
@@ -8816,6 +8829,153 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev *dev)
 	return 0;
 }
 
+/* macsec ctrl setup enable */
+void
+ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_ctrl)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = macsec_ctrl->encrypt_en;
+	macsec->replayprotect_en = macsec_ctrl->replayprotect_en;
+}
+
+/* macsec ctrl setup disable */
+void
+ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = 0;
+	macsec->replayprotect_en = 0;
+}
+
+/* macsec ctrl register set */
+static void
+ixgbe_dev_macsec_ctrl_register_enable(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_contrl)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Enable Ethernet CRC (required by MACsec offload) */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
+
+	/* Enable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
+	ctrl |= 0x3;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
+
+	/* Enable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
+		     IXGBE_LSECTXCTRL_AUTH;
+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
+	if (rp)
+		ctrl |= IXGBE_LSECRXCTRL_RP;
+	else
+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
+/* macsec ctrl register set */
+static void
+ixgbe_dev_macsec_ctrl_register_disable(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Disable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	/* Disable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
+
+
 RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 6e9ed2e10..6aa2cdbbc 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -365,6 +365,12 @@ struct rte_flow {
 	void *rule;
 };
 
+/* MACsec Control structure */
+struct ixgbe_macsec_setting {
+	uint8_t encrypt_en;		/* encrypt enabled */
+	uint8_t replayprotect_en;	/* replayprotect enabled */
+};
+
 /*
  * Statistics counters collected by the MACsec
  */
@@ -471,6 +477,7 @@ struct ixgbe_adapter {
 	struct ixgbe_hw             hw;
 	struct ixgbe_hw_stats       stats;
 	struct ixgbe_macsec_stats   macsec_stats;
+	struct ixgbe_macsec_setting	macsec_ctrl;
 	struct ixgbe_hw_fdir_info   fdir;
 	struct ixgbe_interrupt      intr;
 	struct ixgbe_stat_mapping_registers stat_mappings;
@@ -523,6 +530,9 @@ int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
 #define IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->macsec_stats)
 
+#define IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(adapter) \
+	(&((struct ixgbe_adapter *)adapter)->macsec_ctrl)
+
 #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->intr)
 
@@ -741,6 +751,11 @@ int ixgbe_action_rss_same(const struct rte_flow_action_rss *comp,
 int ixgbe_config_rss_filter(struct rte_eth_dev *dev,
 		struct ixgbe_rte_flow_rss_conf *conf, bool add);
 
+void ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_ctrl);
+
+void ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev);
+
 static inline int
 ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
 			      uint16_t ethertype)
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c b/drivers/net/ixgbe/rte_pmd_ixgbe.c
index 9514f2cf5..3a1474876 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
@@ -515,82 +515,18 @@ rte_pmd_ixgbe_set_vf_rate_limit(uint16_t port, uint16_t vf,
 int
 rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
+	struct ixgbe_macsec_setting macsec_setting;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
-
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	macsec_setting.encrypt_en = en;
+	macsec_setting.replayprotect_en = rp;
 
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Enable Ethernet CRC (required by MACsec offload) */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
-	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
-	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
-
-	/* Enable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
-	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
-	ctrl |= 0x3;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
-
-	/* Enable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
-		     IXGBE_LSECTXCTRL_AUTH;
-	ctrl |= IXGBE_LSECTXCTRL_AISCI;
-	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
-	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
-	if (rp)
-		ctrl |= IXGBE_LSECRXCTRL_RP;
-	else
-		ctrl &= ~IXGBE_LSECRXCTRL_RP;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	/* Enable macsec setup  */
+	ixgbe_dev_macsec_setting_save(dev, &macsec_setting);
 
 	return 0;
 }
@@ -598,63 +534,14 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 int
 rte_pmd_ixgbe_macsec_disable(uint16_t port)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
-
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Disable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	/* Disable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	/* Disable macsec setup  */
+	ixgbe_dev_macsec_setting_reset(dev);
 
 	return 0;
 }
-- 
2.17.1

[dpdk-dev] [PATCH v4] net/ixgbe: fix macsec setting

[Sun GuinanX]

macsec setting is not valid when port is stopped.
In order to make it valid, the patch changes the setting
to where port is started.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
---
v4:
* Deleted extra comments
* Modified function name
* Increased the logic used to set the register in the port start state
v3:
* Deleted extra comments
* Modified the function name of the setting register
* Increased the logic used to set the register in the port start state
v2:
* Modified function name
* Refactored the rte_pmd_ixgbe_macsec_enable/disable function
* Modified structure name
* Modified the data type of a structure variable
---
 drivers/net/ixgbe/ixgbe_ethdev.c  | 149 ++++++++++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.h  |  19 ++++
 drivers/net/ixgbe/rte_pmd_ixgbe.c | 125 ++-----------------------
 3 files changed, 175 insertions(+), 118 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index dbce7a80e..d33b152b5 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2542,6 +2542,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	uint32_t *link_speeds;
 	struct ixgbe_tm_conf *tm_conf =
 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
+	struct ixgbe_macsec_setting *macsec_ctrl =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -2794,6 +2796,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	 */
 	ixgbe_dev_link_update(dev, 0);
 
+	/* setup the macsec ctrl register */
+	ixgbe_dev_macsec_register_set(dev, macsec_ctrl);
+
 	return 0;
 
 error:
@@ -2825,6 +2830,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* disable mecsec register */
+	ixgbe_dev_macsec_register_reset(dev);
+
 	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
 
 	/* disable interrupts */
@@ -8816,6 +8824,147 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev *dev)
 	return 0;
 }
 
+void
+ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_ctrl)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = macsec_ctrl->encrypt_en;
+	macsec->replayprotect_en = macsec_ctrl->replayprotect_en;
+}
+
+void
+ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = 0;
+	macsec->replayprotect_en = 0;
+}
+
+void
+ixgbe_dev_macsec_register_set(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_contrl)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Enable Ethernet CRC (required by MACsec offload) */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
+
+	/* Enable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
+	ctrl |= 0x3;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
+
+	/* Enable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
+		     IXGBE_LSECTXCTRL_AUTH;
+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
+	if (rp)
+		ctrl |= IXGBE_LSECRXCTRL_RP;
+	else
+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
+void
+ixgbe_dev_macsec_register_reset(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Disable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	/* Disable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
 RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 6e9ed2e10..7e26b183e 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -365,6 +365,11 @@ struct rte_flow {
 	void *rule;
 };
 
+struct ixgbe_macsec_setting {
+	uint8_t encrypt_en;
+	uint8_t replayprotect_en;
+};
+
 /*
  * Statistics counters collected by the MACsec
  */
@@ -471,6 +476,7 @@ struct ixgbe_adapter {
 	struct ixgbe_hw             hw;
 	struct ixgbe_hw_stats       stats;
 	struct ixgbe_macsec_stats   macsec_stats;
+	struct ixgbe_macsec_setting	macsec_ctrl;
 	struct ixgbe_hw_fdir_info   fdir;
 	struct ixgbe_interrupt      intr;
 	struct ixgbe_stat_mapping_registers stat_mappings;
@@ -523,6 +529,9 @@ int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
 #define IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->macsec_stats)
 
+#define IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(adapter) \
+	(&((struct ixgbe_adapter *)adapter)->macsec_ctrl)
+
 #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->intr)
 
@@ -741,6 +750,16 @@ int ixgbe_action_rss_same(const struct rte_flow_action_rss *comp,
 int ixgbe_config_rss_filter(struct rte_eth_dev *dev,
 		struct ixgbe_rte_flow_rss_conf *conf, bool add);
 
+void ixgbe_dev_macsec_register_set(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_contrl);
+
+void ixgbe_dev_macsec_register_reset(struct rte_eth_dev *dev);
+
+void ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_ctrl);
+
+void ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev);
+
 static inline int
 ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
 			      uint16_t ethertype)
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c b/drivers/net/ixgbe/rte_pmd_ixgbe.c
index 9514f2cf5..d7305a0af 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
@@ -515,82 +515,19 @@ rte_pmd_ixgbe_set_vf_rate_limit(uint16_t port, uint16_t vf,
 int
 rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
+	struct ixgbe_macsec_setting macsec_setting;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
+	macsec_setting.encrypt_en = en;
+	macsec_setting.replayprotect_en = rp;
 
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	ixgbe_dev_macsec_setting_save(dev, &macsec_setting);
 
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Enable Ethernet CRC (required by MACsec offload) */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
-	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
-	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
-
-	/* Enable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
-	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
-	ctrl |= 0x3;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
-
-	/* Enable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
-		     IXGBE_LSECTXCTRL_AUTH;
-	ctrl |= IXGBE_LSECTXCTRL_AISCI;
-	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
-	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
-	if (rp)
-		ctrl |= IXGBE_LSECRXCTRL_RP;
-	else
-		ctrl &= ~IXGBE_LSECRXCTRL_RP;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	ixgbe_dev_macsec_register_set(dev, &macsec_setting);
 
 	return 0;
 }
@@ -598,63 +535,15 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 int
 rte_pmd_ixgbe_macsec_disable(uint16_t port)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
+	ixgbe_dev_macsec_setting_reset(dev);
 
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Disable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	/* Disable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	ixgbe_dev_macsec_register_reset(dev);
 
 	return 0;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH v4] net/ixgbe: fix macsec setting

[Ye Xiaolong]

Hi, Guinan

[snip]

On 10/30, Sun GuinanX wrote:
>+
>+void
>+ixgbe_dev_macsec_register_set(struct rte_eth_dev *dev,

I'd prefer to keep ixgbe_dev_macsec_register_enable since when it comes to 
HW register, `enable` is more accurate then `set` for this routine.  
And same for the below function, prefer to use disable, not reset.

Thanks,
Xiaolong

>+				struct ixgbe_macsec_setting *macsec_contrl)
>+{
>+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>+	uint32_t ctrl;
>+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
>+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
>+
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_disable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 * The hardware support has been checked by
>+	 * ixgbe_disable_sec_rx_path().
>+	 */
>+	ixgbe_disable_sec_tx_path_generic(hw);
>+
>+	/* Enable Ethernet CRC (required by MACsec offload) */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
>+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
>+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
>+
>+	/* Enable the TX and RX crypto engines */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
>+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
>+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
>+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
>+	ctrl |= 0x3;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
>+
>+	/* Enable SA lookup */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
>+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
>+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
>+		     IXGBE_LSECTXCTRL_AUTH;
>+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
>+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
>+	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
>+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
>+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
>+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
>+	if (rp)
>+		ctrl |= IXGBE_LSECRXCTRL_RP;
>+	else
>+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
>+
>+	/* Start the data paths */
>+	ixgbe_enable_sec_rx_path(hw);
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_enable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 */
>+	ixgbe_enable_sec_tx_path_generic(hw);
>+}
>+
>+void
>+ixgbe_dev_macsec_register_reset(struct rte_eth_dev *dev)
>+{
>+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>+	uint32_t ctrl;
>+
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_disable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 * The hardware support has been checked by
>+	 * ixgbe_disable_sec_rx_path().
>+	 */
>+	ixgbe_disable_sec_tx_path_generic(hw);
>+
>+	/* Disable the TX and RX crypto engines */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
>+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
>+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
>+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
>+
>+	/* Disable SA lookup */
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
>+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
>+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
>+
>+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
>+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
>+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
>+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
>+
>+	/* Start the data paths */
>+	ixgbe_enable_sec_rx_path(hw);
>+	/**
>+	 * Workaround:
>+	 * As no ixgbe_enable_sec_rx_path equivalent is
>+	 * implemented for tx in the base code, and we are
>+	 * not allowed to modify the base code in DPDK, so
>+	 * just call the hand-written one directly for now.
>+	 */
>+	ixgbe_enable_sec_tx_path_generic(hw);
>+}

[dpdk-dev] [PATCH v5] net/ixgbe: fix macsec setting

[Sun GuinanX]

macsec setting is not valid when port is stopped.
In order to make it valid, the patch changes the setting
to where port is started.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
---
v5:
* modified function name
v4:
* Deleted extra comments
* Modified function name
* Increased the logic used to set the register in the port start state
v3:
* Deleted extra comments
* Modified the function name of the setting register
* Increased the logic used to set the register in the port start state
v2:
* Modified function name
* Refactored the rte_pmd_ixgbe_macsec_enable/disable function
* Modified structure name
* Modified the data type of a structure variable
---
 drivers/net/ixgbe/ixgbe_ethdev.c  | 149 ++++++++++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.h  |  19 ++++
 drivers/net/ixgbe/rte_pmd_ixgbe.c | 125 ++-----------------------
 3 files changed, 175 insertions(+), 118 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index dbce7a80e..16a904ed3 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2542,6 +2542,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	uint32_t *link_speeds;
 	struct ixgbe_tm_conf *tm_conf =
 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
+	struct ixgbe_macsec_setting *macsec_ctrl =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -2794,6 +2796,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	 */
 	ixgbe_dev_link_update(dev, 0);
 
+	/* setup the macsec ctrl register */
+	ixgbe_dev_macsec_register_enable(dev, macsec_ctrl);
+
 	return 0;
 
 error:
@@ -2825,6 +2830,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* disable mecsec register */
+	ixgbe_dev_macsec_register_disable(dev);
+
 	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
 
 	/* disable interrupts */
@@ -8816,6 +8824,147 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev *dev)
 	return 0;
 }
 
+void
+ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_ctrl)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = macsec_ctrl->encrypt_en;
+	macsec->replayprotect_en = macsec_ctrl->replayprotect_en;
+}
+
+void
+ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(dev->data->dev_private);
+
+	macsec->encrypt_en = 0;
+	macsec->replayprotect_en = 0;
+}
+
+void
+ixgbe_dev_macsec_register_enable(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_contrl)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Enable Ethernet CRC (required by MACsec offload) */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
+
+	/* Enable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
+	ctrl |= 0x3;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
+
+	/* Enable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
+		     IXGBE_LSECTXCTRL_AUTH;
+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
+	if (rp)
+		ctrl |= IXGBE_LSECRXCTRL_RP;
+	else
+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
+void
+ixgbe_dev_macsec_register_disable(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Disable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	/* Disable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
 RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 6e9ed2e10..d111f481e 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -365,6 +365,11 @@ struct rte_flow {
 	void *rule;
 };
 
+struct ixgbe_macsec_setting {
+	uint8_t encrypt_en;
+	uint8_t replayprotect_en;
+};
+
 /*
  * Statistics counters collected by the MACsec
  */
@@ -471,6 +476,7 @@ struct ixgbe_adapter {
 	struct ixgbe_hw             hw;
 	struct ixgbe_hw_stats       stats;
 	struct ixgbe_macsec_stats   macsec_stats;
+	struct ixgbe_macsec_setting	macsec_ctrl;
 	struct ixgbe_hw_fdir_info   fdir;
 	struct ixgbe_interrupt      intr;
 	struct ixgbe_stat_mapping_registers stat_mappings;
@@ -523,6 +529,9 @@ int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
 #define IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->macsec_stats)
 
+#define IXGBE_DEV_PRIVATE_TO_MACSEC_CTRL(adapter) \
+	(&((struct ixgbe_adapter *)adapter)->macsec_ctrl)
+
 #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->intr)
 
@@ -741,6 +750,16 @@ int ixgbe_action_rss_same(const struct rte_flow_action_rss *comp,
 int ixgbe_config_rss_filter(struct rte_eth_dev *dev,
 		struct ixgbe_rte_flow_rss_conf *conf, bool add);
 
+void ixgbe_dev_macsec_register_enable(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_contrl);
+
+void ixgbe_dev_macsec_register_disable(struct rte_eth_dev *dev);
+
+void ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_ctrl);
+
+void ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev);
+
 static inline int
 ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
 			      uint16_t ethertype)
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c b/drivers/net/ixgbe/rte_pmd_ixgbe.c
index 9514f2cf5..073fe1e23 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
@@ -515,82 +515,19 @@ rte_pmd_ixgbe_set_vf_rate_limit(uint16_t port, uint16_t vf,
 int
 rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
+	struct ixgbe_macsec_setting macsec_setting;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
+	macsec_setting.encrypt_en = en;
+	macsec_setting.replayprotect_en = rp;
 
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	ixgbe_dev_macsec_setting_save(dev, &macsec_setting);
 
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Enable Ethernet CRC (required by MACsec offload) */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
-	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
-	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
-
-	/* Enable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
-	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
-	ctrl |= 0x3;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
-
-	/* Enable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
-		     IXGBE_LSECTXCTRL_AUTH;
-	ctrl |= IXGBE_LSECTXCTRL_AISCI;
-	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
-	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
-	if (rp)
-		ctrl |= IXGBE_LSECRXCTRL_RP;
-	else
-		ctrl &= ~IXGBE_LSECRXCTRL_RP;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	ixgbe_dev_macsec_register_enable(dev, &macsec_setting);
 
 	return 0;
 }
@@ -598,63 +535,15 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 int
 rte_pmd_ixgbe_macsec_disable(uint16_t port)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
+	ixgbe_dev_macsec_setting_reset(dev);
 
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Disable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	/* Disable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	ixgbe_dev_macsec_register_disable(dev);
 
 	return 0;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH v5] net/ixgbe: fix macsec setting

[Ye Xiaolong]

On 10/30, Sun GuinanX wrote:
>macsec setting is not valid when port is stopped.
>In order to make it valid, the patch changes the setting
>to where port is started.
>
>Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
>Cc: stable@dpdk.org

[snip]

>
>+void
>+ixgbe_dev_macsec_register_enable(struct rte_eth_dev *dev,
>+				struct ixgbe_macsec_setting *macsec_contrl)
>+{
>+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>+	uint32_t ctrl;
>+	uint8_t en = (uint8_t)macsec_contrl->encrypt_en;
>+	uint8_t rp = (uint8_t)macsec_contrl->replayprotect_en;

cast here is unnecessary, and contrl is not a valid word, prefer to use
macsec_setting instead.


Thanks,
Xiaolong

[dpdk-dev] [PATCH v6] net/ixgbe: fix macsec setting

[Sun GuinanX]

macsec setting is not valid when port is stopped.
In order to make it valid, the patch changes the setting
to where port is started.

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
---
v6:
* Modified inappropriate parameter naming
* Removed unnecessary type conversions
v5:
* modified function name
v4:
* Deleted extra comments
* Modified function name
* Increased the logic used to set the register in the port start state
v3:
* Deleted extra comments
* Modified the function name of the setting register
* Increased the logic used to set the register in the port start state
v2:
* Modified function name
* Refactored the rte_pmd_ixgbe_macsec_enable/disable function
* Modified structure name
* Modified the data type of a structure variable
---
 drivers/net/ixgbe/ixgbe_ethdev.c  | 149 ++++++++++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_ethdev.h  |  19 ++++
 drivers/net/ixgbe/rte_pmd_ixgbe.c | 125 ++-----------------------
 3 files changed, 175 insertions(+), 118 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index dbce7a80e..d43e11bd4 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2542,6 +2542,8 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	uint32_t *link_speeds;
 	struct ixgbe_tm_conf *tm_conf =
 		IXGBE_DEV_PRIVATE_TO_TM_CONF(dev->data->dev_private);
+	struct ixgbe_macsec_setting *macsec_ctrl =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -2794,6 +2796,9 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
 	 */
 	ixgbe_dev_link_update(dev, 0);
 
+	/* setup the macsec ctrl register */
+	ixgbe_dev_macsec_register_enable(dev, macsec_ctrl);
+
 	return 0;
 
 error:
@@ -2825,6 +2830,9 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 
 	PMD_INIT_FUNC_TRACE();
 
+	/* disable mecsec register */
+	ixgbe_dev_macsec_register_disable(dev);
+
 	rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev);
 
 	/* disable interrupts */
@@ -8816,6 +8824,147 @@ ixgbe_clear_all_l2_tn_filter(struct rte_eth_dev *dev)
 	return 0;
 }
 
+void
+ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_setting)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
+
+	macsec->encrypt_en = macsec_setting->encrypt_en;
+	macsec->replayprotect_en = macsec_setting->replayprotect_en;
+}
+
+void
+ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev)
+{
+	struct ixgbe_macsec_setting *macsec =
+		IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(dev->data->dev_private);
+
+	macsec->encrypt_en = 0;
+	macsec->replayprotect_en = 0;
+}
+
+void
+ixgbe_dev_macsec_register_enable(struct rte_eth_dev *dev,
+				struct ixgbe_macsec_setting *macsec_setting)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+	uint8_t en = macsec_setting->encrypt_en;
+	uint8_t rp = macsec_setting->replayprotect_en;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Enable Ethernet CRC (required by MACsec offload) */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
+	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
+	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
+
+	/* Enable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
+	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
+	ctrl |= 0x3;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
+
+	/* Enable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
+		     IXGBE_LSECTXCTRL_AUTH;
+	ctrl |= IXGBE_LSECTXCTRL_AISCI;
+	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
+	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
+	if (rp)
+		ctrl |= IXGBE_LSECRXCTRL_RP;
+	else
+		ctrl &= ~IXGBE_LSECRXCTRL_RP;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
+void
+ixgbe_dev_macsec_register_disable(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint32_t ctrl;
+
+	/**
+	 * Workaround:
+	 * As no ixgbe_disable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 * The hardware support has been checked by
+	 * ixgbe_disable_sec_rx_path().
+	 */
+	ixgbe_disable_sec_tx_path_generic(hw);
+
+	/* Disable the TX and RX crypto engines */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
+	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
+	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
+	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
+
+	/* Disable SA lookup */
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
+	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
+
+	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
+	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
+	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
+	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
+
+	/* Start the data paths */
+	ixgbe_enable_sec_rx_path(hw);
+	/**
+	 * Workaround:
+	 * As no ixgbe_enable_sec_rx_path equivalent is
+	 * implemented for tx in the base code, and we are
+	 * not allowed to modify the base code in DPDK, so
+	 * just call the hand-written one directly for now.
+	 */
+	ixgbe_enable_sec_tx_path_generic(hw);
+}
+
 RTE_PMD_REGISTER_PCI(net_ixgbe, rte_ixgbe_pmd);
 RTE_PMD_REGISTER_PCI_TABLE(net_ixgbe, pci_id_ixgbe_map);
 RTE_PMD_REGISTER_KMOD_DEP(net_ixgbe, "* igb_uio | uio_pci_generic | vfio-pci");
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 6e9ed2e10..5da6923a1 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -365,6 +365,11 @@ struct rte_flow {
 	void *rule;
 };
 
+struct ixgbe_macsec_setting {
+	uint8_t encrypt_en;
+	uint8_t replayprotect_en;
+};
+
 /*
  * Statistics counters collected by the MACsec
  */
@@ -471,6 +476,7 @@ struct ixgbe_adapter {
 	struct ixgbe_hw             hw;
 	struct ixgbe_hw_stats       stats;
 	struct ixgbe_macsec_stats   macsec_stats;
+	struct ixgbe_macsec_setting	macsec_setting;
 	struct ixgbe_hw_fdir_info   fdir;
 	struct ixgbe_interrupt      intr;
 	struct ixgbe_stat_mapping_registers stat_mappings;
@@ -523,6 +529,9 @@ int ixgbe_vf_representor_uninit(struct rte_eth_dev *ethdev);
 #define IXGBE_DEV_PRIVATE_TO_MACSEC_STATS(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->macsec_stats)
 
+#define IXGBE_DEV_PRIVATE_TO_MACSEC_SETTING(adapter) \
+	(&((struct ixgbe_adapter *)adapter)->macsec_setting)
+
 #define IXGBE_DEV_PRIVATE_TO_INTR(adapter) \
 	(&((struct ixgbe_adapter *)adapter)->intr)
 
@@ -741,6 +750,16 @@ int ixgbe_action_rss_same(const struct rte_flow_action_rss *comp,
 int ixgbe_config_rss_filter(struct rte_eth_dev *dev,
 		struct ixgbe_rte_flow_rss_conf *conf, bool add);
 
+void ixgbe_dev_macsec_register_enable(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_setting);
+
+void ixgbe_dev_macsec_register_disable(struct rte_eth_dev *dev);
+
+void ixgbe_dev_macsec_setting_save(struct rte_eth_dev *dev,
+		struct ixgbe_macsec_setting *macsec_setting);
+
+void ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev);
+
 static inline int
 ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
 			      uint16_t ethertype)
diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c b/drivers/net/ixgbe/rte_pmd_ixgbe.c
index 9514f2cf5..073fe1e23 100644
--- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
+++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
@@ -515,82 +515,19 @@ rte_pmd_ixgbe_set_vf_rate_limit(uint16_t port, uint16_t vf,
 int
 rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
+	struct ixgbe_macsec_setting macsec_setting;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
+	macsec_setting.encrypt_en = en;
+	macsec_setting.replayprotect_en = rp;
 
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	ixgbe_dev_macsec_setting_save(dev, &macsec_setting);
 
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Enable Ethernet CRC (required by MACsec offload) */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_HLREG0);
-	ctrl |= IXGBE_HLREG0_TXCRCEN | IXGBE_HLREG0_RXCRCSTRP;
-	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, ctrl);
-
-	/* Enable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl &= ~IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl &= ~IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXMINIFG);
-	ctrl &= ~IXGBE_SECTX_MINSECIFG_MASK;
-	ctrl |= 0x3;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXMINIFG, ctrl);
-
-	/* Enable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= en ? IXGBE_LSECTXCTRL_AUTH_ENCRYPT :
-		     IXGBE_LSECTXCTRL_AUTH;
-	ctrl |= IXGBE_LSECTXCTRL_AISCI;
-	ctrl &= ~IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	ctrl |= IXGBE_MACSEC_PNTHRSH & IXGBE_LSECTXCTRL_PNTHRSH_MASK;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_STRICT << IXGBE_LSECRXCTRL_EN_SHIFT;
-	ctrl &= ~IXGBE_LSECRXCTRL_PLSH;
-	if (rp)
-		ctrl |= IXGBE_LSECRXCTRL_RP;
-	else
-		ctrl &= ~IXGBE_LSECRXCTRL_RP;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	ixgbe_dev_macsec_register_enable(dev, &macsec_setting);
 
 	return 0;
 }
@@ -598,63 +535,15 @@ rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en, uint8_t rp)
 int
 rte_pmd_ixgbe_macsec_disable(uint16_t port)
 {
-	struct ixgbe_hw *hw;
 	struct rte_eth_dev *dev;
-	uint32_t ctrl;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
 	dev = &rte_eth_devices[port];
 
-	if (!is_ixgbe_supported(dev))
-		return -ENOTSUP;
+	ixgbe_dev_macsec_setting_reset(dev);
 
-	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-
-	/* Stop the data paths */
-	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
-		return -ENOTSUP;
-	/**
-	 * Workaround:
-	 * As no ixgbe_disable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 * The hardware support has been checked by
-	 * ixgbe_disable_sec_rx_path().
-	 */
-	ixgbe_disable_sec_tx_path_generic(hw);
-
-	/* Disable the TX and RX crypto engines */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL);
-	ctrl |= IXGBE_SECTXCTRL_SECTX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_SECRXCTRL);
-	ctrl |= IXGBE_SECRXCTRL_SECRX_DIS;
-	IXGBE_WRITE_REG(hw, IXGBE_SECRXCTRL, ctrl);
-
-	/* Disable SA lookup */
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECTXCTRL);
-	ctrl &= ~IXGBE_LSECTXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECTXCTRL_DISABLE;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECTXCTRL, ctrl);
-
-	ctrl = IXGBE_READ_REG(hw, IXGBE_LSECRXCTRL);
-	ctrl &= ~IXGBE_LSECRXCTRL_EN_MASK;
-	ctrl |= IXGBE_LSECRXCTRL_DISABLE << IXGBE_LSECRXCTRL_EN_SHIFT;
-	IXGBE_WRITE_REG(hw, IXGBE_LSECRXCTRL, ctrl);
-
-	/* Start the data paths */
-	ixgbe_enable_sec_rx_path(hw);
-	/**
-	 * Workaround:
-	 * As no ixgbe_enable_sec_rx_path equivalent is
-	 * implemented for tx in the base code, and we are
-	 * not allowed to modify the base code in DPDK, so
-	 * just call the hand-written one directly for now.
-	 */
-	ixgbe_enable_sec_tx_path_generic(hw);
+	ixgbe_dev_macsec_register_disable(dev);
 
 	return 0;
 }
-- 
2.17.1

Re: [dpdk-dev] [PATCH v6] net/ixgbe: fix macsec setting

[Ye Xiaolong]

On 10/31, Sun GuinanX wrote:
>macsec setting is not valid when port is stopped.
>In order to make it valid, the patch changes the setting
>to where port is started.
>
>Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
>Cc: stable@dpdk.org
>
>Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>

Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>

Applied to dpdk-next-net-intel. Thanks.

Re: [dpdk-dev] [PATCH v6] net/ixgbe: fix macsec setting

[Zhao1, Wei]

Hi, Guinian

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Sun GuinanX
> Sent: Thursday, October 31, 2019 7:32 PM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Sun, GuinanX <guinanx.sun@intel.com>;
> stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v6] net/ixgbe: fix macsec setting
> 
> macsec setting is not valid when port is stopped.
> In order to make it valid, the patch changes the setting to where port is
> started.
> 
> Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Sun GuinanX <guinanx.sun@intel.com>
> 19 ++++  drivers/net/ixgbe/rte_pmd_ixgbe.c | 125 ++-----------------------
>  3 files changed, 175 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/rte_pmd_ixgbe.c
> b/drivers/net/ixgbe/rte_pmd_ixgbe.c
> index 9514f2cf5..073fe1e23 100644
> --- a/drivers/net/ixgbe/rte_pmd_ixgbe.c
> +++ b/drivers/net/ixgbe/rte_pmd_ixgbe.c
> @@ -515,82 +515,19 @@ rte_pmd_ixgbe_set_vf_rate_limit(uint16_t port,
> uint16_t vf,  int  rte_pmd_ixgbe_macsec_enable(uint16_t port, uint8_t en,
> uint8_t rp)  {
> -	struct ixgbe_hw *hw;
>  	struct rte_eth_dev *dev;
> -	uint32_t ctrl;
> +	struct ixgbe_macsec_setting macsec_setting;
> 
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
> 
>  	dev = &rte_eth_devices[port];
> 
> -	if (!is_ixgbe_supported(dev))
> -		return -ENOTSUP;

This driver type check of is_ixgbe_supported(dev), is very important for PRIVATE API function,
You can not delete it, Please add it back, rte_pmd_ixgbe_macsec_disable() also has this problem.
Although this patch has been merged, please commit fix patch for it, thanks!



> +	macsec_setting.encrypt_en = en;
> +	macsec_setting.replayprotect_en = rp;
> 
> -	hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	ixgbe_dev_macsec_setting_save(dev, &macsec_setting);
> 
> -	/* Stop the data paths */
> -	if (ixgbe_disable_sec_rx_path(hw) != IXGBE_SUCCESS)
> -		return -ENOTSUP;