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