patches for DPDK stable branches
 help / color / Atom feed
* [dpdk-stable] [PATCH] net/ixgbe: fix macsec setting
@ 2019-10-25  9:21 Sun GuinanX
  2019-10-29  3:24 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
  2019-10-29 16:32 ` [dpdk-stable] [PATCH v2] " Sun GuinanX
  0 siblings, 2 replies; 12+ messages in thread
From: Sun GuinanX @ 2019-10-25  9:21 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Qiming Yang, Sun GuinanX, stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] net/ixgbe: fix macsec setting
  2019-10-25  9:21 [dpdk-stable] [PATCH] net/ixgbe: fix macsec setting Sun GuinanX
@ 2019-10-29  3:24 ` " Ye Xiaolong
  2019-10-29 16:32 ` [dpdk-stable] [PATCH v2] " Sun GuinanX
  1 sibling, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-10-29  3:24 UTC (permalink / raw)
  To: Sun GuinanX; +Cc: dev, Wenzhuo Lu, Qiming Yang, stable

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/ixgbe: fix macsec setting
  2019-10-29 16:32 ` [dpdk-stable] [PATCH v2] " Sun GuinanX
@ 2019-10-29  9:03   ` " Ye Xiaolong
  2019-10-30  3:19     ` Sun, GuinanX
  2019-10-30 10:43   ` [dpdk-stable] [PATCH v3] " Sun GuinanX
  1 sibling, 1 reply; 12+ messages in thread
From: Ye Xiaolong @ 2019-10-29  9:03 UTC (permalink / raw)
  To: Sun GuinanX; +Cc: dev, Wenzhuo Lu, Qiming Yang, stable

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [PATCH v2] net/ixgbe: fix macsec setting
  2019-10-25  9:21 [dpdk-stable] [PATCH] net/ixgbe: fix macsec setting Sun GuinanX
  2019-10-29  3:24 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
@ 2019-10-29 16:32 ` " Sun GuinanX
  2019-10-29  9:03   ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
  2019-10-30 10:43   ` [dpdk-stable] [PATCH v3] " Sun GuinanX
  1 sibling, 2 replies; 12+ messages in thread
From: Sun GuinanX @ 2019-10-29 16:32 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Qiming Yang, Sun GuinanX, stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/ixgbe: fix macsec setting
  2019-10-29  9:03   ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
@ 2019-10-30  3:19     ` Sun, GuinanX
  0 siblings, 0 replies; 12+ messages in thread
From: Sun, GuinanX @ 2019-10-30  3:19 UTC (permalink / raw)
  To: Ye, Xiaolong; +Cc: dev, Lu, Wenzhuo, Yang, Qiming, stable

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v4] net/ixgbe: fix macsec setting
  2019-10-30 11:31     ` [dpdk-stable] [PATCH v4] " Sun GuinanX
@ 2019-10-30  5:34       ` " Ye Xiaolong
  2019-10-30 14:27       ` [dpdk-stable] [PATCH v5] " Sun GuinanX
  1 sibling, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-10-30  5:34 UTC (permalink / raw)
  To: Sun GuinanX; +Cc: dev, Wenzhuo Lu, Qiming Yang, stable

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);
>+}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [PATCH v3] net/ixgbe: fix macsec setting
  2019-10-29 16:32 ` [dpdk-stable] [PATCH v2] " Sun GuinanX
  2019-10-29  9:03   ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
@ 2019-10-30 10:43   ` " Sun GuinanX
  2019-10-30 11:31     ` [dpdk-stable] [PATCH v4] " Sun GuinanX
  1 sibling, 1 reply; 12+ messages in thread
From: Sun GuinanX @ 2019-10-30 10:43 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Qiming Yang, Sun GuinanX, stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [PATCH v4] net/ixgbe: fix macsec setting
  2019-10-30 10:43   ` [dpdk-stable] [PATCH v3] " Sun GuinanX
@ 2019-10-30 11:31     ` " Sun GuinanX
  2019-10-30  5:34       ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
  2019-10-30 14:27       ` [dpdk-stable] [PATCH v5] " Sun GuinanX
  0 siblings, 2 replies; 12+ messages in thread
From: Sun GuinanX @ 2019-10-30 11:31 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Qiming Yang, Sun GuinanX, stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [PATCH v5] net/ixgbe: fix macsec setting
  2019-10-30 11:31     ` [dpdk-stable] [PATCH v4] " Sun GuinanX
  2019-10-30  5:34       ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
@ 2019-10-30 14:27       ` " Sun GuinanX
  2019-10-31  2:12         ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
  2019-10-31 11:31         ` [dpdk-stable] [PATCH v6] " Sun GuinanX
  1 sibling, 2 replies; 12+ messages in thread
From: Sun GuinanX @ 2019-10-30 14:27 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Qiming Yang, Sun GuinanX, stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v5] net/ixgbe: fix macsec setting
  2019-10-30 14:27       ` [dpdk-stable] [PATCH v5] " Sun GuinanX
@ 2019-10-31  2:12         ` " Ye Xiaolong
  2019-10-31 11:31         ` [dpdk-stable] [PATCH v6] " Sun GuinanX
  1 sibling, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-10-31  2:12 UTC (permalink / raw)
  To: Sun GuinanX; +Cc: dev, Wenzhuo Lu, Qiming Yang, stable

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [PATCH v6] net/ixgbe: fix macsec setting
  2019-10-30 14:27       ` [dpdk-stable] [PATCH v5] " Sun GuinanX
  2019-10-31  2:12         ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
@ 2019-10-31 11:31         ` " Sun GuinanX
  2019-11-01  2:25           ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
  1 sibling, 1 reply; 12+ messages in thread
From: Sun GuinanX @ 2019-10-31 11:31 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Qiming Yang, Sun GuinanX, stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v6] net/ixgbe: fix macsec setting
  2019-10-31 11:31         ` [dpdk-stable] [PATCH v6] " Sun GuinanX
@ 2019-11-01  2:25           ` " Ye Xiaolong
  0 siblings, 0 replies; 12+ messages in thread
From: Ye Xiaolong @ 2019-11-01  2:25 UTC (permalink / raw)
  To: Sun GuinanX; +Cc: dev, Wenzhuo Lu, Qiming Yang, stable

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-25  9:21 [dpdk-stable] [PATCH] net/ixgbe: fix macsec setting Sun GuinanX
2019-10-29  3:24 ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
2019-10-29 16:32 ` [dpdk-stable] [PATCH v2] " Sun GuinanX
2019-10-29  9:03   ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
2019-10-30  3:19     ` Sun, GuinanX
2019-10-30 10:43   ` [dpdk-stable] [PATCH v3] " Sun GuinanX
2019-10-30 11:31     ` [dpdk-stable] [PATCH v4] " Sun GuinanX
2019-10-30  5:34       ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
2019-10-30 14:27       ` [dpdk-stable] [PATCH v5] " Sun GuinanX
2019-10-31  2:12         ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong
2019-10-31 11:31         ` [dpdk-stable] [PATCH v6] " Sun GuinanX
2019-11-01  2:25           ` [dpdk-stable] [dpdk-dev] " Ye Xiaolong

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox