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: sta...@dpdk.org > >Signed-off-by: Sun GuinanX <guinanx....@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