From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 872AEA00BE for ; Tue, 29 Oct 2019 10:06:42 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4E51F1BF05; Tue, 29 Oct 2019 10:06:42 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id ED0231BED2; Tue, 29 Oct 2019 10:06:38 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Oct 2019 02:06:37 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,243,1569308400"; d="scan'208";a="211690142" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17]) by orsmga002.jf.intel.com with ESMTP; 29 Oct 2019 02:06:36 -0700 Date: Tue, 29 Oct 2019 17:03:06 +0800 From: Ye Xiaolong To: Sun GuinanX Cc: dev@dpdk.org, Wenzhuo Lu , Qiming Yang , stable@dpdk.org Message-ID: <20191029090306.GA106706@intel.com> References: <20191025092140.37288-1-guinanx.sun@intel.com> <20191029163211.3254-1-guinanx.sun@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191029163211.3254-1-guinanx.sun@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v2] net/ixgbe: fix macsec setting X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "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 >--- >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