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 B6D3FA00BE for ; Tue, 29 Oct 2019 04:27:53 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 838161BEE7; Tue, 29 Oct 2019 04:27:53 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 7AD6C1BEC3; Tue, 29 Oct 2019 04:27:50 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Oct 2019 20:27:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,242,1569308400"; d="scan'208";a="205352984" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17]) by FMSMGA003.fm.intel.com with ESMTP; 28 Oct 2019 20:27:47 -0700 Date: Tue, 29 Oct 2019 11:24:18 +0800 From: Ye Xiaolong To: Sun GuinanX Cc: dev@dpdk.org, Wenzhuo Lu , Qiming Yang , stable@dpdk.org Message-ID: <20191029032418.GD12923@intel.com> References: <20191025092140.37288-1-guinanx.sun@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191025092140.37288-1-guinanx.sun@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] 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" 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 >--- > 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