From: Ye Xiaolong <xiaolong.ye@intel.com> To: Sun GuinanX <guinanx.sun@intel.com> Cc: dev@dpdk.org, Wenzhuo Lu <wenzhuo.lu@intel.com>, Qiming Yang <qiming.yang@intel.com>, stable@dpdk.org Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] net/ixgbe: fix macsec setting Date: Tue, 29 Oct 2019 11:24:18 +0800 Message-ID: <20191029032418.GD12923@intel.com> (raw) In-Reply-To: <20191025092140.37288-1-guinanx.sun@intel.com> 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
next prev parent reply other threads:[~2019-10-29 3:27 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-25 9:21 [dpdk-stable] " Sun GuinanX 2019-10-29 3:24 ` Ye Xiaolong [this message] 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 2020-05-18 2:56 ` Zhao1, Wei
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191029032418.GD12923@intel.com \ --to=xiaolong.ye@intel.com \ --cc=dev@dpdk.org \ --cc=guinanx.sun@intel.com \ --cc=qiming.yang@intel.com \ --cc=stable@dpdk.org \ --cc=wenzhuo.lu@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
patches for DPDK stable branches This inbox may be cloned and mirrored by anyone: 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 Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.stable AGPL code for this site: git clone https://public-inbox.org/public-inbox.git